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

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/754
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(-)

Le 30/04/2022 à 19:53, Richard Henderson a écrit :
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/754
> 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/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 000bb44cc3..5007b24c03 100644
> --- a/linux-user/m68k/cpu_loop.c
> +++ b/linux-user/m68k/cpu_loop.c
> @@ -48,6 +48,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;

Do we really need to read the data or do we only need to increment s->pc (as the data are only here 
to be available for the trap handler)?

> +    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.  */

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

On 5/25/22 14:40, Laurent Vivier wrote:
>> +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;
> 
> Do we really need to read the data or do we only need to increment s->pc (as the data are 
> only here to be available for the trap handler)?

We need to read the data to (1) trigger sigsegv when this insn crosses a page and (2) 
passing to tcg plugins.


r~

Le 26/05/2022 à 00:26, Richard Henderson a écrit :
> On 5/25/22 14:40, Laurent Vivier wrote:
>>> +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;
>>
>> Do we really need to read the data or do we only need to increment s->pc (as the data are only 
>> here to be available for the trap handler)?
> 
> We need to read the data to (1) trigger sigsegv when this insn crosses a page and (2) passing to tcg 
> plugins.
> 

For (1) I was wondering if the real CPU is actually doing it.

Nothing is said about it in the instruction definition.

Thanks,
Laurent

On 5/25/22 17:15, Laurent Vivier wrote:
> Le 26/05/2022 à 00:26, Richard Henderson a écrit :
>> On 5/25/22 14:40, Laurent Vivier wrote:
>>>> +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;
>>>
>>> Do we really need to read the data or do we only need to increment s->pc (as the data 
>>> are only here to be available for the trap handler)?
>>
>> We need to read the data to (1) trigger sigsegv when this insn crosses a page and (2) 
>> passing to tcg plugins.
>>
> 
> For (1) I was wondering if the real CPU is actually doing it.
> 
> Nothing is said about it in the instruction definition.

Surely the cpu reads cachelines at a time, so of course it would.

r~