Re: [PATCH v2 14/25] target/arm: Create helper_exception_swstep

[Peter Maydell]

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