Re: [Qemu-arm] [PATCH 2/2] target/arm: Fix routing of singlestep exceptions

[Alex Bennée]

Peter Maydell <address@hidden> writes:

> When generating an architectural single-step exception we were
> routing it to the "default exception level", which is to say
> the same exception level we execute at except that EL0 exceptions
> go to EL1. This is incorrect because the debug exception level
> can be configured by the guest for situations such as single
> stepping of EL0 and EL1 code by EL2.
>
> We have to track the target debug exception level in the TB
> flags, because it is dependent on CPU state like HCR_EL2.TGE
> and MDCR_EL2.TDE. (That we were previously calling the
> arm_debug_target_el() function to determine dc->ss_same_el
> is itself a bug, though one that would only have manifested
> as incorrect syndrome information.) Since we are out of TB
> flag bits unless we want to expand into the cs_base field,
> we share some bits with the M-profile only HANDLER and
> STACKCHECK bits, since only A-profile has this singlestep.
>
> Fixes: https://bugs.launchpad.net/qemu/+bug/1838913
> Signed-off-by: Peter Maydell <address@hidden>

Reviewed-by: Alex Bennée <address@hidden>
Tested-by: Alex Bennée <address@hidden>

> ---
> In theory it would be possible to use just a single TB flag
> bit, because other than the route_to_el2 bool, all the
> state arm_debug_target_el() checks is either constant or
> known from other TB flags. But I think trying to do this
> would be pretty hard to maintain and might well break
> anyway with future architectural changes.
>
> Slightly less painfully we could reclaim the existing
> TBFLAG_ANY_SS_ACTIVE, since the debug target EL can't
> be 0 and is irrelevant if SS is not active, so we
> could arrange for SS_ACTIVE to be DEBUG_TARGET_EL == 0.
> But we're going to have to overspill into cs_base pretty
> soon anyway so I'm not too keen on being very stingy with
> the current flags word at the expense of maintainability.
> ---
>  target/arm/cpu.h           |  5 +++++
>  target/arm/translate.h     | 15 +++++++++++----
>  target/arm/helper.c        |  6 ++++++
>  target/arm/translate-a64.c |  2 +-
>  target/arm/translate.c     |  4 +++-
>  5 files changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 94c990cddbd..23ca6c79144 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -3142,6 +3142,11 @@ FIELD(TBFLAG_ANY, PSTATE_SS, 26, 1)
>  /* Target EL if we take a floating-point-disabled exception */
>  FIELD(TBFLAG_ANY, FPEXC_EL, 24, 2)
>  FIELD(TBFLAG_ANY, BE_DATA, 23, 1)
> +/*
> + * For A-profile only, target EL for debug exceptions.
> + * Note that this overlaps with the M-profile-only HANDLER and STACKCHECK 
> bits.
> + */
> +FIELD(TBFLAG_ANY, DEBUG_TARGET_EL, 21, 2)
>
>  /* Bit usage when in AArch32 state: */
>  FIELD(TBFLAG_A32, THUMB, 0, 1)
> diff --git a/target/arm/translate.h b/target/arm/translate.h
> index 45053190baa..b65954c669b 100644
> --- a/target/arm/translate.h
> +++ b/target/arm/translate.h
> @@ -50,6 +50,8 @@ typedef struct DisasContext {
>      uint32_t svc_imm;
>      int aarch64;
>      int current_el;
> +    /* Debug target exception level for single-step exceptions */
> +    int debug_target_el;
>      GHashTable *cp_regs;
>      uint64_t features; /* CPU features bits */
>      /* Because unallocated encodings generate different exception syndrome
> @@ -70,8 +72,6 @@ typedef struct DisasContext {
>       * ie A64 LDX*, LDAX*, A32/T32 LDREX*, LDAEX*.
>       */
>      bool is_ldex;
> -    /* True if a single-step exception will be taken to the current EL */
> -    bool ss_same_el;
>      /* True if v8.3-PAuth is active.  */
>      bool pauth_active;
>      /* True with v8.5-BTI and SCTLR_ELx.BT* set.  */
> @@ -251,8 +251,15 @@ static inline void gen_exception(int excp, uint32_t 
> syndrome,
>  /* Generate an architectural singlestep exception */
>  static inline void gen_swstep_exception(DisasContext *s, int isv, int ex)
>  {
> -    gen_exception(EXCP_UDEF, syn_swstep(s->ss_same_el, isv, ex),
> -                  default_exception_el(s));
> +    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);
>  }
>
>  /*
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index b74c23a9bc0..24806c16ca2 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -11170,6 +11170,12 @@ void cpu_get_tb_cpu_state(CPUARMState *env, 
> target_ulong *pc,
>          }
>      }
>
> +    if (!arm_feature(env, ARM_FEATURE_M)) {
> +        int target_el = arm_debug_target_el(env);
> +
> +        flags = FIELD_DP32(flags, TBFLAG_ANY, DEBUG_TARGET_EL, target_el);
> +    }
> +
>      *pflags = flags;
>      *cs_base = 0;
>  }
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index f6729b96fd0..90850eadc1b 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -14180,7 +14180,7 @@ static void 
> aarch64_tr_init_disas_context(DisasContextBase *dcbase,
>      dc->ss_active = FIELD_EX32(tb_flags, TBFLAG_ANY, SS_ACTIVE);
>      dc->pstate_ss = FIELD_EX32(tb_flags, TBFLAG_ANY, PSTATE_SS);
>      dc->is_ldex = false;
> -    dc->ss_same_el = (arm_debug_target_el(env) == dc->current_el);
> +    dc->debug_target_el = FIELD_EX32(tb_flags, TBFLAG_ANY, DEBUG_TARGET_EL);
>
>      /* Bound the number of insns to execute to those left on the page.  */
>      bound = -(dc->base.pc_first | TARGET_PAGE_MASK) / 4;
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 19b9d8f2725..b32508cd2f9 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -11882,7 +11882,9 @@ static void 
> arm_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
>      dc->ss_active = FIELD_EX32(tb_flags, TBFLAG_ANY, SS_ACTIVE);
>      dc->pstate_ss = FIELD_EX32(tb_flags, TBFLAG_ANY, PSTATE_SS);
>      dc->is_ldex = false;
> -    dc->ss_same_el = false; /* Can't be true since EL_d must be AArch64 */
> +    if (!arm_feature(env, ARM_FEATURE_M)) {
> +        dc->debug_target_el = FIELD_EX32(tb_flags, TBFLAG_ANY, 
> DEBUG_TARGET_EL);
> +    }
>
>      dc->page_start = dc->base.pc_first & TARGET_PAGE_MASK;


--
Alex Bennée