| Message ID | 20220430175342.370628-11-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/m68k: Conditional traps + trap cleanup | expand |
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~
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; + 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. */
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(-)