Peter Maydell <peter.mayd...@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-sta...@nongnu.org
> Signed-off-by: Peter Maydell <peter.mayd...@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.ben...@linaro.org>
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
