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();
(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)
--
2.34.1On 1/30/25 10:23, Peter Maydell wrote: > 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(); > > (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 alsocc: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> > --- Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
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
On Wed, 5 Feb 2025 at 12:52, Alex Bennée <alex.bennee@linaro.org> wrote: > > 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. Yes, that's correct; I simplified the pattern a bit above to remove some of the irrelevant detail. (EL3SDDUndef is also hiding an IMPDEF choice about what order to prioritize the traps in if you do implement external debug.) -- PMM
© 2016 - 2025 Red Hat, Inc.