diff mbox series

[v5,10/17] target/m68k: Implement TRAPcc

Message ID 20220527164807.135038-11-richard.henderson@linaro.org
State Superseded
Headers show
Series target/m68k: Conditional traps + trap cleanup | expand

Commit Message

Richard Henderson May 27, 2022, 4:48 p.m. UTC 
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/754
Reviewed-by: Laurent Vivier <laurent@vivier.eu>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/m68k/cpu.h          |  2 ++
 linux-user/m68k/cpu_loop.c |  1 +
 target/m68k/cpu.c          |  1 +
 target/m68k/op_helper.c    |  6 +----
 target/m68k/translate.c    | 49 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 54 insertions(+), 5 deletions(-)

Comments

Laurent Vivier May 31, 2022, 8:01 a.m. UTC | #1 
Le 27/05/2022 à 18:48, Richard Henderson a écrit :
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/754
> Reviewed-by: Laurent Vivier <laurent@vivier.eu>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/m68k/cpu.h          |  2 ++
>   linux-user/m68k/cpu_loop.c |  1 +
>   target/m68k/cpu.c          |  1 +
>   target/m68k/op_helper.c    |  6 +----
>   target/m68k/translate.c    | 49 ++++++++++++++++++++++++++++++++++++++
>   5 files changed, 54 insertions(+), 5 deletions(-)
> 
...
> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
> index 399d9232e4..c4fe8abc03 100644
> --- a/target/m68k/translate.c
> +++ b/target/m68k/translate.c
...
> @@ -6050,6 +6098,7 @@ void register_m68k_insns (CPUM68KState *env)
>       INSN(scc,       50c0, f0f8, CF_ISA_A); /* Scc.B Dx   */
>       INSN(scc,       50c0, f0c0, M68000);   /* Scc.B <EA> */
>       INSN(dbcc,      50c8, f0f8, M68000);
> +    INSN(trapcc,    50f8, f0f8, TRAPCC);
>       INSN(tpf,       51f8, fff8, CF_ISA_A);
>   
>       /* Branch instructions.  */

This one breaks Mark's series to support MacOS.

I think the new opcode short-circuits Scc one:

   ----------------
   IN: INITRSRCMGR
   0x408011d0:  st 0xa58
   Disassembler disagrees with translator over instruction decoding
   Please report this to qemu-devel@nongnu.org

The following patch seems to fix the problem:

diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index d5d73401b7cc..3b0e3d0b58f6 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -6119,9 +6119,9 @@ void register_m68k_insns (CPUM68KState *env)
      INSN(addsubq,   5000, f080, M68000);
      BASE(addsubq,   5080, f0c0);
      INSN(scc,       50c0, f0f8, CF_ISA_A); /* Scc.B Dx   */
+    INSN(trapcc,    50f8, f0f8, TRAPCC);
      INSN(scc,       50c0, f0c0, M68000);   /* Scc.B <EA> */
      INSN(dbcc,      50c8, f0f8, M68000);
-    INSN(trapcc,    50f8, f0f8, TRAPCC);
      INSN(trapcc,    51f8, fff8, CF_ISA_A); /* TPF (trapf) */

      /* Branch instructions.  */

Thanks,
Laurent
Richard Henderson May 31, 2022, 2:59 p.m. UTC | #2 
On 5/31/22 01:01, Laurent Vivier wrote:
> Le 27/05/2022 à 18:48, Richard Henderson a écrit :
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/754
>> Reviewed-by: Laurent Vivier <laurent@vivier.eu>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   target/m68k/cpu.h          |  2 ++
>>   linux-user/m68k/cpu_loop.c |  1 +
>>   target/m68k/cpu.c          |  1 +
>>   target/m68k/op_helper.c    |  6 +----
>>   target/m68k/translate.c    | 49 ++++++++++++++++++++++++++++++++++++++
>>   5 files changed, 54 insertions(+), 5 deletions(-)
>>
> ...
>> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
>> index 399d9232e4..c4fe8abc03 100644
>> --- a/target/m68k/translate.c
>> +++ b/target/m68k/translate.c
> ...
>> @@ -6050,6 +6098,7 @@ void register_m68k_insns (CPUM68KState *env)
>>       INSN(scc,       50c0, f0f8, CF_ISA_A); /* Scc.B Dx   */
>>       INSN(scc,       50c0, f0c0, M68000);   /* Scc.B <EA> */
>>       INSN(dbcc,      50c8, f0f8, M68000);
>> +    INSN(trapcc,    50f8, f0f8, TRAPCC);
>>       INSN(tpf,       51f8, fff8, CF_ISA_A);
>>       /* Branch instructions.  */
> 
> This one breaks Mark's series to support MacOS.
> 
> I think the new opcode short-circuits Scc one:
> 
>    ----------------
>    IN: INITRSRCMGR
>    0x408011d0:  st 0xa58
>    Disassembler disagrees with translator over instruction decoding
>    Please report this to qemu-devel@nongnu.org
> 
> The following patch seems to fix the problem:
> 
> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
> index d5d73401b7cc..3b0e3d0b58f6 100644
> --- a/target/m68k/translate.c
> +++ b/target/m68k/translate.c
> @@ -6119,9 +6119,9 @@ void register_m68k_insns (CPUM68KState *env)
>       INSN(addsubq,   5000, f080, M68000);
>       BASE(addsubq,   5080, f0c0);
>       INSN(scc,       50c0, f0f8, CF_ISA_A); /* Scc.B Dx   */
> +    INSN(trapcc,    50f8, f0f8, TRAPCC);
>       INSN(scc,       50c0, f0c0, M68000);   /* Scc.B <EA> */
>       INSN(dbcc,      50c8, f0f8, M68000);
> -    INSN(trapcc,    50f8, f0f8, TRAPCC);

Hmm.  That will completely hide trapcc -- you should have seen the new test case fail (and 
if not, the test case needs fixing).

These two insn overlap considerably:

    setcc  0101 cond:4 11 mode:3 reg:3
    trapcc 0101 cond:4 11 111 opmode:3

We need to select only the 3 valid opmodes:

     INSN(scc,       50c0, f0c0, M68000);   /* Scc.B <EA> */
     INSN(dbcc,      50c8, f0f8, M68000);
     INSN(trapcc,    50fa, f0fe, TRAPCC);   /* opmode 010, 011 */
     INSN(trapcc,    50fc, f0ff, TRAPCC);   /* opmode 100 */
     INSN(trapcc,    51fa, fffe, CP_ISA_A); /* TPF (trapf) opmode 010, 011 */
     INSN(trapcc,    51fc, ffff, CP_ISA_A); /* TPF (trapf) opmode 100 */

which are invalid mode/reg combinations for Scc.


r~
Laurent Vivier May 31, 2022, 6:05 p.m. UTC | #3 
Le 31/05/2022 à 16:59, Richard Henderson a écrit :
> On 5/31/22 01:01, Laurent Vivier wrote:
>> Le 27/05/2022 à 18:48, Richard Henderson a écrit :
>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/754
>>> Reviewed-by: Laurent Vivier <laurent@vivier.eu>
>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>> ---
>>>   target/m68k/cpu.h          |  2 ++
>>>   linux-user/m68k/cpu_loop.c |  1 +
>>>   target/m68k/cpu.c          |  1 +
>>>   target/m68k/op_helper.c    |  6 +----
>>>   target/m68k/translate.c    | 49 ++++++++++++++++++++++++++++++++++++++
>>>   5 files changed, 54 insertions(+), 5 deletions(-)
>>>
>> ...
>>> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
>>> index 399d9232e4..c4fe8abc03 100644
>>> --- a/target/m68k/translate.c
>>> +++ b/target/m68k/translate.c
>> ...
>>> @@ -6050,6 +6098,7 @@ void register_m68k_insns (CPUM68KState *env)
>>>       INSN(scc,       50c0, f0f8, CF_ISA_A); /* Scc.B Dx   */
>>>       INSN(scc,       50c0, f0c0, M68000);   /* Scc.B <EA> */
>>>       INSN(dbcc,      50c8, f0f8, M68000);
>>> +    INSN(trapcc,    50f8, f0f8, TRAPCC);
>>>       INSN(tpf,       51f8, fff8, CF_ISA_A);
>>>       /* Branch instructions.  */
>>
>> This one breaks Mark's series to support MacOS.
>>
>> I think the new opcode short-circuits Scc one:
>>
>>    ----------------
>>    IN: INITRSRCMGR
>>    0x408011d0:  st 0xa58
>>    Disassembler disagrees with translator over instruction decoding
>>    Please report this to qemu-devel@nongnu.org
>>
>> The following patch seems to fix the problem:
>>
>> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
>> index d5d73401b7cc..3b0e3d0b58f6 100644
>> --- a/target/m68k/translate.c
>> +++ b/target/m68k/translate.c
>> @@ -6119,9 +6119,9 @@ void register_m68k_insns (CPUM68KState *env)
>>       INSN(addsubq,   5000, f080, M68000);
>>       BASE(addsubq,   5080, f0c0);
>>       INSN(scc,       50c0, f0f8, CF_ISA_A); /* Scc.B Dx   */
>> +    INSN(trapcc,    50f8, f0f8, TRAPCC);
>>       INSN(scc,       50c0, f0c0, M68000);   /* Scc.B <EA> */
>>       INSN(dbcc,      50c8, f0f8, M68000);
>> -    INSN(trapcc,    50f8, f0f8, TRAPCC);
> 
> Hmm.  That will completely hide trapcc -- you should have seen the new test case fail (and if not, 
> the test case needs fixing).

I ran "make check", thinking the test is run, and saw no failure...
and if I run "make check-tcg", I have:
   make: Nothing to be done for 'check-tcg'.
so what is the command to run the test?

Thanks,
Laurent
Richard Henderson May 31, 2022, 9:14 p.m. UTC | #4 
On 5/31/22 11:05, Laurent Vivier wrote:
>> Hmm.  That will completely hide trapcc -- you should have seen the new test case fail 
>> (and if not, the test case needs fixing).
> 
> I ran "make check", thinking the test is run, and saw no failure...
> and if I run "make check-tcg", I have:
>    make: Nothing to be done for 'check-tcg'.
> so what is the command to run the test?

make check-tcg.

If you get "nothing to be done", configure has not detected any cross-compilers (via 
docker or installed locally).  We usually list detected cross compilers at configure time. 
That's currently broken, but I saw Alex has a fix queued.


r~
diff mbox series

Patch

diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
index 558c3c67d6..4d8f48e8c7 100644
--- a/target/m68k/cpu.h
+++ b/target/m68k/cpu.h
@@ -534,6 +534,8 @@  enum m68k_features {
     M68K_FEATURE_MOVEC,
     /* Unaligned data accesses (680[2346]0) */
     M68K_FEATURE_UNALIGNED_DATA,
+    /* TRAPcc insn. (680[2346]0, and CPU32) */
+    M68K_FEATURE_TRAPCC,
 };
 
 static inline int m68k_feature(CPUM68KState *env, int feature)
diff --git a/linux-user/m68k/cpu_loop.c b/linux-user/m68k/cpu_loop.c
index fcf9220552..3d3033155f 100644
--- a/linux-user/m68k/cpu_loop.c
+++ b/linux-user/m68k/cpu_loop.c
@@ -47,6 +47,7 @@  void cpu_loop(CPUM68KState *env)
             force_sig_fault(TARGET_SIGILL, TARGET_ILL_ILLOPN, env->pc);
             break;
         case EXCP_CHK:
+        case EXCP_TRAPCC:
             force_sig_fault(TARGET_SIGFPE, TARGET_FPE_INTOVF, env->mmu.ar);
             break;
         case EXCP_DIV0:
diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
index c7aeb7da9c..5f778773d1 100644
--- a/target/m68k/cpu.c
+++ b/target/m68k/cpu.c
@@ -162,6 +162,7 @@  static void m68020_cpu_initfn(Object *obj)
     m68k_set_feature(env, M68K_FEATURE_CHK2);
     m68k_set_feature(env, M68K_FEATURE_MSP);
     m68k_set_feature(env, M68K_FEATURE_UNALIGNED_DATA);
+    m68k_set_feature(env, M68K_FEATURE_TRAPCC);
 }
 
 /*
diff --git a/target/m68k/op_helper.c b/target/m68k/op_helper.c
index aa62158eb9..61948d92bb 100644
--- a/target/m68k/op_helper.c
+++ b/target/m68k/op_helper.c
@@ -399,14 +399,10 @@  static void m68k_interrupt_all(CPUM68KState *env, int is_hw)
         do_stack_frame(env, &sp, 2, oldsr, 0, env->pc);
         break;
 
-    case EXCP_TRAPCC:
-        /* FIXME: addr is not only env->pc */
-        do_stack_frame(env, &sp, 2, oldsr, env->pc, env->pc);
-        break;
-
     case EXCP_CHK:
     case EXCP_DIV0:
     case EXCP_TRACE:
+    case EXCP_TRAPCC:
         do_stack_frame(env, &sp, 2, oldsr, env->mmu.ar, env->pc);
         break;
 
diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index 399d9232e4..c4fe8abc03 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -4879,6 +4879,54 @@  DISAS_INSN(trap)
     gen_exception(s, s->pc, EXCP_TRAP0 + (insn & 0xf));
 }
 
+static void do_trapcc(DisasContext *s, DisasCompare *c)
+{
+    if (c->tcond != TCG_COND_NEVER) {
+        TCGLabel *over = NULL;
+
+        update_cc_op(s);
+
+        if (c->tcond != TCG_COND_ALWAYS) {
+            /* Jump over if !c. */
+            over = gen_new_label();
+            tcg_gen_brcond_i32(tcg_invert_cond(c->tcond), c->v1, c->v2, over);
+        }
+
+        tcg_gen_movi_i32(QREG_PC, s->pc);
+        gen_raise_exception_format2(s, EXCP_TRAPCC, s->base.pc_next);
+
+        if (over != NULL) {
+            gen_set_label(over);
+            s->base.is_jmp = DISAS_NEXT;
+        }
+    }
+    free_cond(c);
+}
+
+DISAS_INSN(trapcc)
+{
+    DisasCompare c;
+
+    /* Consume and discard the immediate operand. */
+    switch (extract32(insn, 0, 3)) {
+    case 2: /* trapcc.w */
+        (void)read_im16(env, s);
+        break;
+    case 3: /* trapcc.l */
+        (void)read_im32(env, s);
+        break;
+    case 4: /* trapcc (no operand) */
+        break;
+    default:
+        /* Illegal insn */
+        disas_undef(env, s, insn);
+        return;
+    }
+
+    gen_cc_cond(&c, s, extract32(insn, 8, 4));
+    do_trapcc(s, &c);
+}
+
 static void gen_load_fcr(DisasContext *s, TCGv res, int reg)
 {
     switch (reg) {
@@ -6050,6 +6098,7 @@  void register_m68k_insns (CPUM68KState *env)
     INSN(scc,       50c0, f0f8, CF_ISA_A); /* Scc.B Dx   */
     INSN(scc,       50c0, f0c0, M68000);   /* Scc.B <EA> */
     INSN(dbcc,      50c8, f0f8, M68000);
+    INSN(trapcc,    50f8, f0f8, TRAPCC);
     INSN(tpf,       51f8, fff8, CF_ISA_A);
 
     /* Branch instructions.  */