Re: [PATCH v3 14/20] target/arm: Set PAN bit as required on exception entry

[Peter Maydell]

On Mon, 3 Feb 2020 at 14:47, Richard Henderson
<address@hidden> wrote:
>
> The PAN bit is preserved, or set as per SCTLR_ELx.SPAN,
> plus several other conditions listed in the ARM ARM.
>
> Signed-off-by: Richard Henderson <address@hidden>
> ---
> v2: Tidy preservation of CPSR_PAN in take_aarch32_exception (pmm).
> ---
>  target/arm/helper.c | 40 +++++++++++++++++++++++++++++++++++++---
>  1 file changed, 37 insertions(+), 3 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 18e4cbb63c..4c0eb7e7d9 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -8772,8 +8772,12 @@ static void take_aarch32_exception(CPUARMState *env, 
> int new_mode,
>                                     uint32_t mask, uint32_t offset,
>                                     uint32_t newpc)
>  {
> +    int new_el;
> +
>      /* Change the CPU state so as to actually take the exception. */
>      switch_mode(env, new_mode);
> +    new_el = arm_current_el(env);
> +
>      /*
>       * For exceptions taken to AArch32 we must clear the SS bit in both
>       * PSTATE and in the old-state value we save to SPSR_<mode>, so zero it 
> now.
> @@ -8786,7 +8790,7 @@ static void take_aarch32_exception(CPUARMState *env, 
> int new_mode,
>      env->uncached_cpsr = (env->uncached_cpsr & ~CPSR_M) | new_mode;
>      /* Set new mode endianness */
>      env->uncached_cpsr &= ~CPSR_E;
> -    if (env->cp15.sctlr_el[arm_current_el(env)] & SCTLR_EE) {
> +    if (env->cp15.sctlr_el[new_el] & SCTLR_EE) {
>          env->uncached_cpsr |= CPSR_E;
>      }
>      /* J and IL must always be cleared for exception entry */
> @@ -8797,6 +8801,12 @@ static void take_aarch32_exception(CPUARMState *env, 
> int new_mode,
>          env->thumb = (env->cp15.sctlr_el[2] & SCTLR_TE) != 0;
>          env->elr_el[2] = env->regs[15];
>      } else {
> +        /* CPSR.PAN is preserved unless target is EL1 and SCTLR.SPAN == 0. */
> +        if (cpu_isar_feature(aa64_pan, env_archcpu(env))
> +            && new_el == 1
> +            && !(env->cp15.sctlr_el[1] & SCTLR_SPAN)) {
> +            env->uncached_cpsr |= CPSR_PAN;
> +        }

This doesn't catch the "taking exception to EL3 and AArch32 is EL3"
case, which is also supposed to honour SCTLR.SPAN.

Given where this code is, we know we're taking an exception to
AArch32 and that we're not going to Hyp mode, so in fact every
case where we get here is one where we should honour SCTLR.SPAN
and I think we can just drop the "new_el == 1" part of the condition.

Otherwise
Reviewed-by: Peter Maydell <address@hidden>

thanks
-- PMM