Re: [PATCH 05/14] target/arm: Make CP_ACCESS_TRAPs to AArch32 EL3 be Monitor traps

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> In system register access pseudocode the common pattern for
> AArch32 registers with access traps to EL3 is:
>
> at EL1 and EL2:
>   if HaveEL(EL3) && !ELUsingAArch32(EL3) && (SCR_EL3.TERR == 1) then
>      AArch64.AArch32SystemAccessTrap(EL3, 0x03);
>   elsif HaveEL(EL3) && ELUsingAArch32(EL3) && (SCR.TERR == 1) then
>      AArch32.TakeMonitorTrapException();
> at EL3:
>   if (PSTATE.M != M32_Monitor) && (SCR.TERR == 1) then
>      AArch32.TakeMonitorTrapException();

I was confused a little by my copy which was:

    elsif HaveEL(EL3) && !ELUsingAArch32(EL3) && SCR_EL3.TERR == '1' then
        if EL3SDDUndef() then
            UNDEFINED;
        else
            AArch64.AArch32SystemAccessTrap(EL3, 0x03);

But I think EL3SDDUndef() is always false for us as we don't have an
external debug interface.

>
> (taking as an example the ERRIDR access pseudocode).
>
> This implements the behaviour of (in this case) SCR.TERR that
> "Accesses to the specified registers from modes other than Monitor
> mode generate a Monitor Trap exception" and of SCR_EL3.TERR that
> "Accesses of the specified Error Record registers at EL2 and EL1
> are trapped to EL3, unless the instruction generates a higher
> priority exception".
>
> In QEMU we don't implement this pattern correctly in two ways:
>  * in access_check_cp_reg() we turn the CP_ACCESS_TRAP_EL3 into
>    an UNDEF, not a trap to Monitor mode
>  * in the access functions, we check trap bits like SCR.TERR
>    only when arm_current_el(env) < 3 -- this is correct for
>    AArch64 EL3, but misses the "trap non-Monitor-mode execution
>    at EL3 into Monitor mode" case for AArch32 EL3
>
> In this commit we fix the first of these two issues, by
> making access_check_cp_reg() handle CP_ACCESS_TRAP_EL3
> as a Monitor trap. This is a kind of exception that we haven't
> yet implemented(!), so we need a new EXCP_MON_TRAP for it.
>
> This diverges from the pseudocode approach, where every access check
> function explicitly checks for "if EL3 is AArch32" and takes a
> monitor trap; if we wanted to be closer to the pseudocode we could
> add a new CP_ACCESS_TRAP_MONITOR and make all the accessfns use it
> when appropriate.  But because there are no non-standard cases in the
> pseudocode (i.e.  where either it raises a Monitor trap that doesn't
> correspond to an AArch64 SystemAccessTrap or where it raises a
> SystemAccessTrap that doesn't correspond to a Monitor trap), handling
> this all in one place seems less likely to result in future bugs
> where we forgot again about this special case when writing an
> accessor.
>
> (The cc of stable here is because "hw/intc/arm_gicv3_cpuif: Don't
> downgrade monitor traps for AArch32 EL3" which is also cc:stable
> will implicitly use the new EXCP_MON_TRAP code path.)
>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/cpu.h           |  1 +
>  target/arm/helper.c        | 11 +++++++++++
>  target/arm/tcg/op_helper.c | 13 ++++++++++++-
>  3 files changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 2213c277348..4cb672c120b 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -62,6 +62,7 @@
>  #define EXCP_NMI            26
>  #define EXCP_VINMI          27
>  #define EXCP_VFNMI          28
> +#define EXCP_MON_TRAP       29   /* AArch32 trap to Monitor mode */
>  /* NB: add new EXCP_ defines to the array in arm_log_exception() too */
>  
>  #define ARMV7M_EXCP_RESET   1
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 5d9eca35c04..c5cd27b249f 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -9684,6 +9684,7 @@ void arm_log_exception(CPUState *cs)
>              [EXCP_NMI] = "NMI",
>              [EXCP_VINMI] = "Virtual IRQ NMI",
>              [EXCP_VFNMI] = "Virtual FIQ NMI",
> +            [EXCP_MON_TRAP] = "Monitor Trap",
>          };
>  
>          if (idx >= 0 && idx < ARRAY_SIZE(excnames)) {
> @@ -10250,6 +10251,16 @@ static void arm_cpu_do_interrupt_aarch32(CPUState 
> *cs)
>          mask = CPSR_A | CPSR_I | CPSR_F;
>          offset = 0;
>          break;
> +    case EXCP_MON_TRAP:
> +        new_mode = ARM_CPU_MODE_MON;
> +        addr = 0x04;
> +        mask = CPSR_A | CPSR_I | CPSR_F;
> +        if (env->thumb) {
> +            offset = 2;
> +        } else {
> +            offset = 4;
> +        }
> +        break;
>      default:
>          cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);
>          return; /* Never happens.  Keep compiler happy.  */
> diff --git a/target/arm/tcg/op_helper.c b/target/arm/tcg/op_helper.c
> index 1161d301b71..1ba727e8e9f 100644
> --- a/target/arm/tcg/op_helper.c
> +++ b/target/arm/tcg/op_helper.c
> @@ -758,6 +758,7 @@ const void *HELPER(access_check_cp_reg)(CPUARMState *env, 
> uint32_t key,
>      const ARMCPRegInfo *ri = get_arm_cp_reginfo(cpu->cp_regs, key);
>      CPAccessResult res = CP_ACCESS_OK;
>      int target_el;
> +    uint32_t excp;
>  
>      assert(ri != NULL);
>  
> @@ -851,8 +852,18 @@ const void *HELPER(access_check_cp_reg)(CPUARMState 
> *env, uint32_t key,
>      }
>  
>   fail:
> +    excp = EXCP_UDEF;
>      switch (res & ~CP_ACCESS_EL_MASK) {
>      case CP_ACCESS_TRAP:
> +        /*
> +         * If EL3 is AArch32 then there's no syndrome register; the cases
> +         * where we would raise a SystemAccessTrap to AArch64 EL3 all become
> +         * raising a Monitor trap exception. (Because there's no visible
> +         * syndrome it doesn't matter what we pass to raise_exception().)
> +         */
> +        if ((res & CP_ACCESS_EL_MASK) == 3 && !arm_el_is_aa64(env, 3)) {
> +            excp = EXCP_MON_TRAP;
> +        }
>          break;
>      case CP_ACCESS_TRAP_UNCATEGORIZED:
>          /* Only CP_ACCESS_TRAP traps are direct to a specified EL */
> @@ -888,7 +899,7 @@ const void *HELPER(access_check_cp_reg)(CPUARMState *env, 
> uint32_t key,
>          g_assert_not_reached();
>      }
>  
> -    raise_exception(env, EXCP_UDEF, syndrome, target_el);
> +    raise_exception(env, excp, syndrome, target_el);
>  }
>  
>  const void *HELPER(lookup_cp_reg)(CPUARMState *env, uint32_t key)

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro