Re: [Qemu-devel] [PATCH] target-arm: Fix arm_el_is_aa64() function to support EL2 and EL3

[Peter Maydell]

On 17 July 2015 at 17:01, Sergey Sorokin <address@hidden> wrote:
> Function arm_el_is_aa64() was fixed to support EL2 and EL3.
> It is needed for a future support of EL2 and/or EL3,
> and 32 bit EL1 support for ARMv8 cpu.
> ARM_FEATURE_AARCH64 flag means that the highest exception level is
> in Aarch64 state. The state of lower exception levels is controlled
> by the HCR_EL2.RW, SCR_EL3.RW and SCR_EL3.NS bits.
> If EL2 or EL3 is not permitted by the appropriate ARM_FEATURE flag,
> then the function arm_el_is_aa64() aborts on the attempt to get
> the bittness of this EL.

In general I like the approach of caching the bitness of the
exception level, given we're fairly free with calling that
function. This patch feels like it's trying to do too many
things at once, though. In particular why are we changing
the DisasContext flag el3_is_aa64 ? If that's necessary, it
should be its own separate patch with an explanation of why.

(OTOH given that we very rarely call arm_el_is_aa64() for
any value of EL other than '3' I do wonder if we need the
cache.)

> Signed-off-by: Sergey Sorokin <address@hidden>
> ---
>  hw/arm/boot.c              |  3 +++
>  target-arm/cpu.c           | 59 
> ++++++++++++++++++++++++++++++++++++++++++++++
>  target-arm/cpu.h           | 31 +++++++++++++-----------
>  target-arm/helper.c        | 18 +++++++++++++-
>  target-arm/translate-a64.c |  6 ++++-
>  target-arm/translate.c     |  6 ++++-
>  target-arm/translate.h     |  7 ++++--
>  7 files changed, 111 insertions(+), 19 deletions(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 5b969cd..6e25558 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -486,6 +486,9 @@ static void do_cpu_reset(void *opaque)
>                  if (!info->secure_boot) {
>                      /* Linux expects non-secure state */
>                      env->cp15.scr_el3 |= SCR_NS;
> +                    /* EL bittness can depend on SCR.NS bit, so we need
> +                     * recalc it. */

"bitness" -- you make the same typo elsewhere too.
(Also I prefer multiline comments in the style
 /* line one
  * line two
  */
)

> +                    calc_el_bitness(env);
>                  }
>              }
>
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 8b4323d..103a04f 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -79,6 +79,64 @@ static void cp_reg_reset(gpointer key, gpointer value, 
> gpointer opaque)
>      }
>  }
>
> +/* This function must be called if SCR_EL3.RW, SCR_EL3.NS
> + * or HCR_EL2.RW has been changed.
> + */
> +void calc_el_bitness(CPUARMState *env)
> +{
> +    /* -1 is invalid value */
> +    env->el_is_aa64[0] = -1;
> +
> +    /* If highest EL is not Aarch64, then all ELs are Aarch32. */

"AArch64" and "AArch32".

> +    if (!arm_feature(env, ARM_FEATURE_AARCH64)) {
> +        if (arm_feature(env, ARM_FEATURE_EL3)) {
> +            env->el_is_aa64[3] = 0;
> +        } else {
> +            env->el_is_aa64[3] = -1;
> +        }
> +
> +        if (arm_feature(env, ARM_FEATURE_EL2)) {
> +            env->el_is_aa64[2] = 0;
> +        } else {
> +            env->el_is_aa64[2] = -1;
> +        }
> +
> +        env->el_is_aa64[1] = 0;
> +        return;
> +    }
> +
> +    if (arm_feature(env, ARM_FEATURE_EL3)) {
> +        env->el_is_aa64[3] = 1;
> +        int8_t scr_rw = env->cp15.scr_el3 & SCR_RW ? 1 : 0;
> +
> +        if (arm_feature(env, ARM_FEATURE_EL2) && (env->cp15.scr_el3 & 
> SCR_NS)) {
> +            if (scr_rw) {
> +                env->el_is_aa64[2] = 1;
> +                env->el_is_aa64[1] = env->cp15.hcr_el2 & HCR_RW ? 1 : 0;
> +            } else {
> +                env->el_is_aa64[2] = 0;
> +                env->el_is_aa64[1] = 0;
> +            }
> +        } else {
> +            /* If there is no EL2 or if secure. */
> +            env->el_is_aa64[2] = -1;
> +            env->el_is_aa64[1] = scr_rw;
> +        }
> +    } else {
> +        env->el_is_aa64[3] = -1;
> +
> +        if (arm_feature(env, ARM_FEATURE_EL2)) {
> +            /* The highest EL is EL2, and it is Aarch64. */
> +            env->el_is_aa64[2] = 1;
> +            env->el_is_aa64[1] = env->cp15.hcr_el2 & HCR_RW ? 1 : 0;
> +        } else {
> +            /* The highest EL is EL1, and it is Aarch64. */
> +            env->el_is_aa64[2] = -1;
> +            env->el_is_aa64[1] = 1;
> +        }
> +    }

My instinct is that it should be possible to tighten up this function,
it feels like it's about twice as long as it really needs to be.

> +}
> +
>  /* CPUClass::reset() */
>  static void arm_cpu_reset(CPUState *s)
>  {
> @@ -128,6 +186,7 @@ static void arm_cpu_reset(CPUState *s)
>          env->cp15.cpacr_el1 = deposit64(env->cp15.cpacr_el1, 20, 4, 0xf);
>  #endif
>      }
> +    calc_el_bitness(env);
>
>  #if defined(CONFIG_USER_ONLY)
>      env->uncached_cpsr = ARM_CPU_MODE_USR;
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 7e89152..1df74dc 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -175,6 +175,8 @@ typedef struct CPUARMState {
>      uint64_t elr_el[4]; /* AArch64 exception link regs  */
>      uint64_t sp_el[4]; /* AArch64 banked stack pointers */
>
> +    int8_t el_is_aa64[4];

This needs a comment explaining what it is.

> +
>      /* System control coprocessor (cp15) */
>      struct {
>          uint32_t c0_cpuid;
> @@ -920,7 +922,7 @@ static inline bool arm_is_secure_below_el3(CPUARMState 
> *env)
>      if (arm_feature(env, ARM_FEATURE_EL3)) {
>          return !(env->cp15.scr_el3 & SCR_NS);
>      } else {
> -        /* If EL2 is not supported then the secure state is implementation
> +        /* If EL3 is not supported then the secure state is implementation
>           * defined, in which case QEMU defaults to non-secure.
>           */
>          return false;
> @@ -955,21 +957,21 @@ static inline bool arm_is_secure(CPUARMState *env)
>  }
>  #endif
>
> +/* This function must be called if SCR_EL3.RW, SCR_EL3.NS
> + * or HCR_EL2.RW has been changed.
> + */

Can you use the standard format for doc-comments for function prototypes,
please?

> +void calc_el_bitness(CPUARMState *env);
> +
>  /* Return true if the specified exception level is running in AArch64 state. 
> */
>  static inline bool arm_el_is_aa64(CPUARMState *env, int el)
>  {
> -    /* We don't currently support EL2, and this isn't valid for EL0
> -     * (if we're in EL0, is_a64() is what you want, and if we're not in EL0
> -     * then the state of EL0 isn't well defined.)
> +    /* This isn't valid for EL0 (if we're in EL0, is_a64() is what you want,
> +     * and if we're not in EL0 then the state of EL0 isn't well defined.)
>       */
> -    assert(el == 1 || el == 3);
> +    assert(el >= 1 && el <= 3);
> +    assert(env->el_is_aa64[el] >= 0);
>
> -    /* AArch64-capable CPUs always run with EL1 in AArch64 mode. This
> -     * is a QEMU-imposed simplification which we may wish to change later.
> -     * If we in future support EL2 and/or EL3, then the state of lower
> -     * exception levels is controlled by the HCR.RW and SCR.RW bits.
> -     */
> -    return arm_feature(env, ARM_FEATURE_AARCH64);
> +    return env->el_is_aa64[el];
>  }
>
>  /* Function for determing whether guest cp register reads and writes should
> @@ -1008,11 +1010,11 @@ static inline bool access_secure_reg(CPUARMState *env)
>   */
>  #define A32_BANKED_CURRENT_REG_GET(_env, _regname)        \
>      A32_BANKED_REG_GET((_env), _regname,                \
> -                       ((!arm_el_is_aa64((_env), 3) && arm_is_secure(_env))))
> +                       (arm_is_secure(_env) && !arm_el_is_aa64((_env), 3)))
>
>  #define A32_BANKED_CURRENT_REG_SET(_env, _regname, _val)                     
>   \
>      A32_BANKED_REG_SET((_env), _regname,                                    \
> -                       ((!arm_el_is_aa64((_env), 3) && 
> arm_is_secure(_env))),  \
> +                       (arm_is_secure(_env) && !arm_el_is_aa64((_env), 3)), \

What are these changes for?

>                         (_val))
>
>  void arm_cpu_list(FILE *f, fprintf_function cpu_fprintf);
> @@ -1573,7 +1575,8 @@ static inline bool arm_excp_unmasked(CPUState *cs, 
> unsigned int excp_idx,
>       * interrupt.
>       */
>      if ((target_el > cur_el) && (target_el != 1)) {
> -        if (arm_el_is_aa64(env, 3) || ((scr || hcr) && (!secure))) {
> +        if ((target_el == 3 && arm_el_is_aa64(env, 3)) ||
> +            ((scr || hcr) && (!secure))) {

What is this change for?

>              unmasked = 1;
>          }
>      }
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 01f0d0d..2cf7bee 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -856,7 +856,14 @@ static void scr_write(CPUARMState *env, const 
> ARMCPRegInfo *ri, uint64_t value)
>
>      /* Clear all-context RES0 bits.  */
>      value &= valid_mask;
> +    uint64_t old_value = raw_read(env, ri);
>      raw_write(env, ri, value);
> +
> +    /* We need to recalculate EL bitness if SCR_EL3.RW, SCR_EL3.NS
> +     * or HCR_EL2.RW has been changed. */
> +    if ((old_value ^ value) & (SCR_RW | SCR_NS)) {
> +        calc_el_bitness(env);
> +    }
>  }
>
>  static uint64_t ccsidr_read(CPUARMState *env, const ARMCPRegInfo *ri)
> @@ -2638,7 +2645,14 @@ static void hcr_write(CPUARMState *env, const 
> ARMCPRegInfo *ri, uint64_t value)
>      if ((raw_read(env, ri) ^ value) & (HCR_VM | HCR_PTW | HCR_DC)) {
>          tlb_flush(CPU(cpu), 1);
>      }
> +    uint64_t old_value = raw_read(env, ri);

QEMU coding style says variable declarations should be at the
start of the block.

>      raw_write(env, ri, value);
> +
> +    /* We need to recalculate EL bitness if SCR_EL3.RW, SCR_EL3.NS
> +     * or HCR_EL2.RW has been changed. */
> +    if ((old_value ^ value) & HCR_RW) {
> +        calc_el_bitness(env);
> +    }
>  }
>
>  static const ARMCPRegInfo el2_cp_reginfo[] = {
> @@ -4421,7 +4435,7 @@ uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t 
> excp_idx,
>      int scr;
>      int hcr;
>      int target_el;
> -    int is64 = arm_el_is_aa64(env, 3);
> +    int is64 = arm_feature(env, ARM_FEATURE_AARCH64);

Why this change?

>
>      switch (excp_idx) {
>      case EXCP_IRQ:
> @@ -4978,6 +4992,8 @@ void arm_cpu_do_interrupt(CPUState *cs)
>
>      if ((env->uncached_cpsr & CPSR_M) == ARM_CPU_MODE_MON) {
>          env->cp15.scr_el3 &= ~SCR_NS;
> +        /* EL bittness can depend on SCR.NS bit, so we need recalc it. */
> +        calc_el_bitness(env);

This seems weird. We're here because we're about to take an exception
to EL3. This transition should not change the register width of any
exception level, so why the recalc?

>      }
>
>      switch_mode (env, new_mode);
> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> index 689f2be..b834cf4 100644
> --- a/target-arm/translate-a64.c
> +++ b/target-arm/translate-a64.c
> @@ -10946,7 +10946,11 @@ void gen_intermediate_code_internal_a64(ARMCPU *cpu,
>      dc->condjmp = 0;
>
>      dc->aarch64 = 1;
> -    dc->el3_is_aa64 = arm_el_is_aa64(env, 3);
> +    /* If we are coming from secure EL0 in a system with a 32-bit EL3, then
> +     * there is no secure EL1, so we route exceptions to EL3.
> +     */
> +    dc->secure_routed_to_el3 = arm_feature(env, ARM_FEATURE_EL3) &&
> +                               !arm_el_is_aa64(env, 3);

This change definitely belongs in its own patch.

>      dc->thumb = 0;
>      dc->bswap_code = 0;
>      dc->condexec_mask = 0;
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 69ac18c..dde69e9 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -11172,7 +11172,11 @@ static inline void 
> gen_intermediate_code_internal(ARMCPU *cpu,
>      dc->condjmp = 0;
>
>      dc->aarch64 = 0;
> -    dc->el3_is_aa64 = arm_el_is_aa64(env, 3);
> +    /* If we are coming from secure EL0 in a system with a 32-bit EL3, then
> +     * there is no secure EL1, so we route exceptions to EL3.
> +     */
> +    dc->secure_routed_to_el3 = arm_feature(env, ARM_FEATURE_EL3) &&
> +                               !arm_el_is_aa64(env, 3);
>      dc->thumb = ARM_TBFLAG_THUMB(tb->flags);
>      dc->bswap_code = ARM_TBFLAG_BSWAP_CODE(tb->flags);
>      dc->condexec_mask = (ARM_TBFLAG_CONDEXEC(tb->flags) & 0xf) << 1;
> diff --git a/target-arm/translate.h b/target-arm/translate.h
> index 9ab978f..9fdec47 100644
> --- a/target-arm/translate.h
> +++ b/target-arm/translate.h
> @@ -23,7 +23,10 @@ typedef struct DisasContext {
>      ARMMMUIdx mmu_idx; /* MMU index to use for normal loads/stores */
>      bool ns;        /* Use non-secure CPREG bank on access */
>      int fp_excp_el; /* FP exception EL or 0 if enabled */
> -    bool el3_is_aa64;  /* Flag indicating whether EL3 is AArch64 or not */
> +    /* Flag indicating that an exceptions from the secure mode
> +     * are routed to EL3.
> +     */
> +    bool secure_routed_to_el3;
>      bool vfp_enabled; /* FP enabled via FPSCR.EN */
>      int vec_len;
>      int vec_stride;
> @@ -84,7 +87,7 @@ static inline int default_exception_el(DisasContext *s)
>       * exceptions can only be routed to ELs above 1, so we target the higher 
> of
>       * 1 or the current EL.
>       */
> -    return (s->mmu_idx == ARMMMUIdx_S1SE0 && !s->el3_is_aa64)
> +    return (s->mmu_idx == ARMMMUIdx_S1SE0 && s->secure_routed_to_el3)
>              ? 3 : MAX(1, s->current_el);
>  }
>

Since we're not saving/restoring the el_is_aa64[] array, we need
to re-calculate it when we restore the VM state, in the postload
hook.

thanks
-- PMM