| Message ID | 20220607024734.541321-15-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm: tidy exception routing | expand |
On Tue, 7 Jun 2022 at 03:58, Richard Henderson <richard.henderson@linaro.org> wrote: > > Move the computation from gen_swstep_exception into a helper. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/arm/helper.h | 1 + > target/arm/translate.h | 12 +++--------- > target/arm/debug_helper.c | 16 ++++++++++++++++ > 3 files changed, 20 insertions(+), 9 deletions(-) > > diff --git a/target/arm/helper.h b/target/arm/helper.h > index aca86612b4..afc0f1a462 100644 > --- a/target/arm/helper.h > +++ b/target/arm/helper.h > @@ -48,6 +48,7 @@ DEF_HELPER_2(exception_internal, noreturn, env, i32) > DEF_HELPER_4(exception_with_syndrome_el, noreturn, env, i32, i32, i32) > DEF_HELPER_3(exception_advsimdfp_access, noreturn, env, i32, i32) > DEF_HELPER_2(exception_bkpt_insn, noreturn, env, i32) > +DEF_HELPER_2(exception_swstep, noreturn, env, i32) > DEF_HELPER_2(exception_pc_alignment, noreturn, env, tl) > DEF_HELPER_1(setend, void, env) > DEF_HELPER_2(wfi, void, env, i32) > diff --git a/target/arm/translate.h b/target/arm/translate.h > index 04d45da54e..c720a7e26c 100644 > --- a/target/arm/translate.h > +++ b/target/arm/translate.h > @@ -350,15 +350,9 @@ static inline void gen_exception_advsimdfp_access(DisasContext *s, > /* Generate an architectural singlestep exception */ > static inline void gen_swstep_exception(DisasContext *s, int isv, int ex) > { > - bool same_el = (s->debug_target_el == s->current_el); > - > - /* > - * If singlestep is targeting a lower EL than the current one, > - * then s->ss_active must be false and we can never get here. > - */ > - assert(s->debug_target_el >= s->current_el); > - > - gen_exception(EXCP_UDEF, syn_swstep(same_el, isv, ex), s->debug_target_el); > + /* Fill in the same_el field of the syndrome in the helper. */ > + uint32_t syn = syn_swstep(false, isv, ex); > + gen_helper_exception_swstep(cpu_env, tcg_constant_i32(syn)); > } > > /* > diff --git a/target/arm/debug_helper.c b/target/arm/debug_helper.c > index a743061e89..a3a1b98de2 100644 > --- a/target/arm/debug_helper.c > +++ b/target/arm/debug_helper.c > @@ -487,6 +487,22 @@ void HELPER(exception_bkpt_insn)(CPUARMState *env, uint32_t syndrome) > raise_exception(env, EXCP_BKPT, syndrome, debug_el); > } > > +void HELPER(exception_swstep)(CPUARMState *env, uint32_t syndrome) > +{ > + int debug_el = arm_debug_target_el(env); > + int cur_el = arm_current_el(env); > + > + /* > + * If singlestep is targeting a lower EL than the current one, then > + * DisasContext.ss_active must be false and we can never get here. > + */ > + assert(debug_el >= cur_el); This is a little trickier than it first looks, because in the old setup the assert in gen_swstep_exception() is checking the translate time state (which corresponds to the EL we executed the insn in), whereas this assert is checked at runtime, so it happens after all the effects of the insn have taken place, which might include a change of exception level, if the insn is "eret". Similarly we'll now calculate the "same_el" bit based on the EL after execution of the eret, rather than the one before. I think however that: * the assertion is still fine, because we can only go down in EL (going up in EL means taking an exception, in which case we won't be here) * setting the same-el bit based on the cur_el after the eret changes it is actually fixing a bug in a corner case: - EL_D is using MDSCR_EL1.KDE == 1 to enable debug exceptions within EL_D itself - we singlestep an eret from EL_D to some lower EL Here the 'same EL' bit should be based on the EL we end up with after the 'eret' (architecturally we only take the swstep exception when we are in the Active-Pending state, which is after we have completed execution of the instruction proper), so it ought to be 0. But in the old code we calculate it using the DisasContext::current_el, so it would incorrectly be 1. (Writing a test case to demonstrate this theory is left as an exercise for the reader :-)) So as far as the code changes are concerned, Reviewed-by: Peter Maydell <peter.maydell@linaro.org> but since this is pretty subtle we should probably discuss it in the commit message, and definitely we should note that this is fixing a bug. thanks -- PMM
diff --git a/target/arm/helper.h b/target/arm/helper.h index aca86612b4..afc0f1a462 100644 --- a/target/arm/helper.h +++ b/target/arm/helper.h @@ -48,6 +48,7 @@ DEF_HELPER_2(exception_internal, noreturn, env, i32) DEF_HELPER_4(exception_with_syndrome_el, noreturn, env, i32, i32, i32) DEF_HELPER_3(exception_advsimdfp_access, noreturn, env, i32, i32) DEF_HELPER_2(exception_bkpt_insn, noreturn, env, i32) +DEF_HELPER_2(exception_swstep, noreturn, env, i32) DEF_HELPER_2(exception_pc_alignment, noreturn, env, tl) DEF_HELPER_1(setend, void, env) DEF_HELPER_2(wfi, void, env, i32) diff --git a/target/arm/translate.h b/target/arm/translate.h index 04d45da54e..c720a7e26c 100644 --- a/target/arm/translate.h +++ b/target/arm/translate.h @@ -350,15 +350,9 @@ static inline void gen_exception_advsimdfp_access(DisasContext *s, /* Generate an architectural singlestep exception */ static inline void gen_swstep_exception(DisasContext *s, int isv, int ex) { - bool same_el = (s->debug_target_el == s->current_el); - - /* - * If singlestep is targeting a lower EL than the current one, - * then s->ss_active must be false and we can never get here. - */ - assert(s->debug_target_el >= s->current_el); - - gen_exception(EXCP_UDEF, syn_swstep(same_el, isv, ex), s->debug_target_el); + /* Fill in the same_el field of the syndrome in the helper. */ + uint32_t syn = syn_swstep(false, isv, ex); + gen_helper_exception_swstep(cpu_env, tcg_constant_i32(syn)); } /* diff --git a/target/arm/debug_helper.c b/target/arm/debug_helper.c index a743061e89..a3a1b98de2 100644 --- a/target/arm/debug_helper.c +++ b/target/arm/debug_helper.c @@ -487,6 +487,22 @@ void HELPER(exception_bkpt_insn)(CPUARMState *env, uint32_t syndrome) raise_exception(env, EXCP_BKPT, syndrome, debug_el); } +void HELPER(exception_swstep)(CPUARMState *env, uint32_t syndrome) +{ + int debug_el = arm_debug_target_el(env); + int cur_el = arm_current_el(env); + + /* + * If singlestep is targeting a lower EL than the current one, then + * DisasContext.ss_active must be false and we can never get here. + */ + assert(debug_el >= cur_el); + if (debug_el == cur_el) { + syndrome |= 1 << ARM_EL_EC_SHIFT; + } + raise_exception(env, EXCP_UDEF, syndrome, debug_el); +} + #if !defined(CONFIG_USER_ONLY) vaddr arm_adjust_watchpoint_address(CPUState *cs, vaddr addr, int len)
Move the computation from gen_swstep_exception into a helper. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/arm/helper.h | 1 + target/arm/translate.h | 12 +++--------- target/arm/debug_helper.c | 16 ++++++++++++++++ 3 files changed, 20 insertions(+), 9 deletions(-)