[PATCH 00/14] target/arm: Clean up some corner cases of sysreg traps

[PATCH 00/14] target/arm: Clean up some corner cases of sysreg traps

[Peter Maydell]

While reviewing Alex's recent secure timer patchset, I noticed a
bug where it was using CP_ACCESS_TRAP when CP_ACCESS_TRAP_UNCATEGORIZED
was wanted, and that we were making the same mistake elsewhere in
our existing code. This series started out as an attempt to fix
that and also rearrange the API so that it's harder to introduce
other instances of this bug in future. In the process I noticed
a different bug, where we weren't handling traps to AArch32
Monitor mode correctly, so the series fixes those as well.

The first four patches are fixes for the places where we were
using CP_ACCESS_TRAP when we wanted CP_ACCESS_TRAP_UNCATEGORIZED.
These are all situations where an attempt to access a sysreg
should UNDEF and we were incorrectly reporting it with a
syndrome value for a system register access trap. I've cc'd
these to stable as they are technically bugs, but really the impact
s very limited because I can't see a reason why any code except
test code would deliberately attempt a sysreg access that they
knew would take an exception and then look at the syndrome
value for it.

Patches 5, 6 and 7 together fix bugs in our handling of sysreg
traps that should go to AArch32 Monitor mode:
 * we were incorrectly triggering an UNDEF exception for these
   rather than a Monitor Trap, so the exception would go to
   the wrong vector table and the wrong CPU mode
 * in most cases we weren't trapping accesses from EL3
   non-Monitor modes to Monitor mode
This affects only:
 * trapping of ERRIDR via SCR.TERR
 * trapping of the debug channel registers via SDCR.TDCC
 * trapping of GICv3 registers via SCR.IRQ and SCR.FIQ
because most "trap sysreg access to EL3" situations are either for
AArch64 only registers or for trap bits that are AArch64 only.

Patch 8 is a trivial one removing an unnecessary clause in
an if() condition in the GIC cpuif access functions.

Patches 9-13 are the API change that tries to make the bug harder to
write: we add CP_ACCESS_TRAP_EL1 for "trap a sysreg access direct to
EL1". After all the bugfixes in the first half of the series, the
remaining uses of CP_ACCESS_TRAP are all in contexts where we know the
current EL is 0, so we can directly replace them with
CP_ACCESS_TRAP_EL1, and remove CP_ACCESS_TRAP entirely. We also rename
CP_ACCESS_TRAP_UNCATEGORIZED to CP_ACCESS_UNDEFINED, to be a clearer
parallel with the pseudocode's use of "UNDEFINED" in this situation.
The resulting
API is that an accessfn can only return:
 CP_ACCESS_OK for a success
 CP_ACCESS_UNDEFINED for an UNDEF
 CP_ACCESS_TRAP_EL{1,2,3} for a sysreg trap direct to an EL
and everything else is invalid. UNCATEGORIZED traps never go to a
specified target EL, and sysreg-traps always go to a specified target
EL, matching the pseudocode which either uses "UNDEFINED" or some kind
of SystemAccessTrap(ELx, ...).

Patch 14 fixes some issues with the WFI/WFX trap code, some of
which are like the above sysreg access check bugs (not using
Monitor Trap, not honouring traps in EL3 not-Monitor-mode),
plus one which I noticed while working on the code (it doesn't
use arm_sctlr() so will look at the wrong SCTLR when in EL2&0).

I've cc'd the relevant patches to stable, but I don't think
it's very likely that anybody ever ran into these bugs, because
they're all cases of "we did the wrong thing if code at an EL
below EL3 tried to do something it shouldn't". I don't think that
EL3 code generally uses the trap bits for trap-and-emulate
of functionality, only to prevent the lower ELs messing with
state it should not, and everything here with the exception of
SCR.IRQ and SCR.FIQ is pretty obscure anyway.

(Tested somewhat lightly, because I don't have any test images
that test AArch32 EL3 really.)

thanks
-- PMM

Peter Maydell (14):
  target/arm: Report correct syndrome for UNDEFINED CNTPS_*_EL1 from EL2
    and NS EL1
  target/arm: Report correct syndrome for UNDEFINED AT ops with wrong
    NSE,NS
  target/arm: Report correct syndrome for UNDEFINED S1E2 AT ops at EL3
  target/arm: Report correct syndrome for UNDEFINED LOR sysregs when
    NS=0
  target/arm: Make CP_ACCESS_TRAPs to AArch32 EL3 be Monitor traps
  hw/intc/arm_gicv3_cpuif: Don't downgrade monitor traps for AArch32 EL3
  target/arm: Honour SDCR.TDCC and SCR.TERR in AArch32 EL3 non-Monitor
    modes
  hw/intc/arm_gicv3_cpuif(): Remove redundant tests of is_a64()
  target/arm: Support CP_ACCESS_TRAP_EL1 as a CPAccessResult
  target/arm: Use CP_ACCESS_TRAP_EL1 for traps that are always to EL1
  target/arm: Use TRAP_UNCATEGORIZED for XScale CPAR traps
  target/arm: Remove CP_ACCESS_TRAP handling
  target/arm: Rename CP_ACCESS_TRAP_UNCATEGORIZED to CP_ACCESS_UNDEFINED
  target/arm: Correct errors in WFI/WFE trapping

 target/arm/cpregs.h        | 15 +++++---
 target/arm/cpu.h           |  6 +++
 hw/intc/arm_gicv3_cpuif.c  | 15 ++------
 target/arm/debug_helper.c  |  5 ++-
 target/arm/helper.c        | 75 ++++++++++++++++++++++----------------
 target/arm/tcg/op_helper.c | 71 ++++++++++++++++++++++--------------
 6 files changed, 107 insertions(+), 80 deletions(-)

-- 
2.34.1

[PATCH 01/14] target/arm: Report correct syndrome for UNDEFINED CNTPS_*_EL1 from EL2 and NS EL1

[Peter Maydell]

The access pseudocode for the CNTPS_TVAL_EL1, CNTPS_CTL_EL1 and
CNTPS_CVAL_EL1 secure timer registers says that they are UNDEFINED
from EL2 or NS EL1.  We incorrectly return CP_ACCESS_TRAP from the
access function in these cases, which means that we report the wrong
syndrome value to the target EL.

Use CP_ACCESS_TRAP_UNCATEGORIZED, which reports the correct syndrome
value for an UNDEFINED instruction.

Cc: qemu-stable@nongnu.org
Fixes: b4d3978c2fd ("target-arm: Add the AArch64 view of the Secure physical timer")
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 40bdfc851a5..c5245a20aaf 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -2385,7 +2385,7 @@ static CPAccessResult gt_stimer_access(CPUARMState *env,
     switch (arm_current_el(env)) {
     case 1:
         if (!arm_is_secure(env)) {
-            return CP_ACCESS_TRAP;
+            return CP_ACCESS_TRAP_UNCATEGORIZED;
         }
         if (!(env->cp15.scr_el3 & SCR_ST)) {
             return CP_ACCESS_TRAP_EL3;
@@ -2393,7 +2393,7 @@ static CPAccessResult gt_stimer_access(CPUARMState *env,
         return CP_ACCESS_OK;
     case 0:
     case 2:
-        return CP_ACCESS_TRAP;
+        return CP_ACCESS_TRAP_UNCATEGORIZED;
     case 3:
         return CP_ACCESS_OK;
     default:
-- 
2.34.1

[PATCH 02/14] target/arm: Report correct syndrome for UNDEFINED AT ops with wrong NSE, NS

[Peter Maydell]

R_NYXTL says that these AT insns should be UNDEFINED if they
would operate on an EL lower than EL3 and SCR_EL3.{NSE,NS} is
set to the Reserved {1, 0}. We were incorrectly reporting
them with the wrong syndrome; use CP_ACCESS_TRAP_UNCATEGORIZED
so they are reported as UNDEFINED.

Cc: qemu-stable@nongnu.org
Fixes: 1acd00ef1410 ("target/arm/helper: Check SCR_EL3.{NSE, NS} encoding for AT instructions")
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index c5245a20aaf..7ddeed0283f 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -3601,7 +3601,7 @@ static CPAccessResult at_e012_access(CPUARMState *env, const ARMCPRegInfo *ri,
      * scr_write() ensures that the NSE bit is not set otherwise.
      */
     if ((env->cp15.scr_el3 & (SCR_NSE | SCR_NS)) == SCR_NSE) {
-        return CP_ACCESS_TRAP;
+        return CP_ACCESS_TRAP_UNCATEGORIZED;
     }
     return CP_ACCESS_OK;
 }
-- 
2.34.1

[PATCH 03/14] target/arm: Report correct syndrome for UNDEFINED S1E2 AT ops at EL3

[Peter Maydell]

The pseudocode for AT S1E2R and AT S1E2W says that they should be
UNDEFINED if executed at EL3 when EL2 is not enabled. We were
incorrectly using CP_ACCESS_TRAP and reporting the wrong exception
syndrome as a result. Use CP_ACCESS_TRAP_UNCATEGORIZED.

Cc: qemu-stable@nongnu.org
Fixes: 2a47df953202e1 ("target-arm: Wire up AArch64 EL2 and EL3 address translation ops")
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 7ddeed0283f..74b556b6766 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -3611,7 +3611,7 @@ static CPAccessResult at_s1e2_access(CPUARMState *env, const ARMCPRegInfo *ri,
 {
     if (arm_current_el(env) == 3 &&
         !(env->cp15.scr_el3 & (SCR_NS | SCR_EEL2))) {
-        return CP_ACCESS_TRAP;
+        return CP_ACCESS_TRAP_UNCATEGORIZED;
     }
     return at_e012_access(env, ri, isread);
 }
-- 
2.34.1

[PATCH 04/14] target/arm: Report correct syndrome for UNDEFINED LOR sysregs when NS=0

[Peter Maydell]

The pseudocode for the accessors for the LOR sysregs says they
are UNDEFINED if SCR_EL3.NS is 0. We were reporting the wrong
syndrome value here; use CP_ACCESS_TRAP_UNCATEGORIZED.

Cc: qemu-stable@nongnu.org
Fixes: 2d7137c10faf ("target/arm: Implement the ARMv8.1-LOR extension")
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 74b556b6766..5d9eca35c04 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6750,8 +6750,8 @@ static CPAccessResult access_lor_other(CPUARMState *env,
                                        const ARMCPRegInfo *ri, bool isread)
 {
     if (arm_is_secure_below_el3(env)) {
-        /* Access denied in secure mode.  */
-        return CP_ACCESS_TRAP;
+        /* UNDEF if SCR_EL3.NS == 0 */
+        return CP_ACCESS_TRAP_UNCATEGORIZED;
     }
     return access_lor_ns(env, ri, isread);
 }
-- 
2.34.1

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

[Peter Maydell]

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.1

[PATCH 06/14] hw/intc/arm_gicv3_cpuif: Don't downgrade monitor traps for AArch32 EL3

[Peter Maydell]

In the gicv3_{irq,fiq,irqfiq}_access() functions, there is a check
which downgrades a CP_ACCESS_TRAP_EL3 to CP_ACCESS_TRAP if EL3 is not
AArch64.  This has been there since the GIC was first implemented,
but it isn't right: if we are trapping because of SCR.IRQ or SCR.FIQ
then we definitely want to be going to EL3 (doing
AArch32.TakeMonitorTrapException() in pseudocode terms).  We might
want to not take a trap at all, but we don't ever want to go to the
default target EL, because that would mean, for instance, taking a
trap to Hyp mode if the trapped access was made from Hyp mode.

(This might have been an attempt to work around our failure to
properly implement Monitor Traps.)

Remove the bogus check.

Cc: qemu-stable@nongnu.org
Fixes: 359fbe65e01e ("hw/intc/arm_gicv3: Implement GICv3 CPU interface registers")
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/arm_gicv3_cpuif.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
index 9cad8313a3a..8a715b3510b 100644
--- a/hw/intc/arm_gicv3_cpuif.c
+++ b/hw/intc/arm_gicv3_cpuif.c
@@ -2300,9 +2300,6 @@ static CPAccessResult gicv3_irqfiq_access(CPUARMState *env,
         }
     }
 
-    if (r == CP_ACCESS_TRAP_EL3 && !arm_el_is_aa64(env, 3)) {
-        r = CP_ACCESS_TRAP;
-    }
     return r;
 }
 
@@ -2365,9 +2362,6 @@ static CPAccessResult gicv3_fiq_access(CPUARMState *env,
         }
     }
 
-    if (r == CP_ACCESS_TRAP_EL3 && !arm_el_is_aa64(env, 3)) {
-        r = CP_ACCESS_TRAP;
-    }
     return r;
 }
 
@@ -2404,9 +2398,6 @@ static CPAccessResult gicv3_irq_access(CPUARMState *env,
         }
     }
 
-    if (r == CP_ACCESS_TRAP_EL3 && !arm_el_is_aa64(env, 3)) {
-        r = CP_ACCESS_TRAP;
-    }
     return r;
 }
 
-- 
2.34.1

[PATCH 07/14] target/arm: Honour SDCR.TDCC and SCR.TERR in AArch32 EL3 non-Monitor modes

[Peter Maydell]

There are not many traps in AArch32 which should trap to Monitor
mode, but these trap bits should trap not just lower ELs to Monitor
mode but also the non-Monitor modes running at EL3 (i.e.  Secure
System, Secure Undef, etc).

We get this wrong because the relevant access functions implement the
AArch64-style logic of
   if (el < 3 && trap_bit_set) {
       return CP_ACCESS_TRAP_EL3;
   }
which won't trap the non-Monitor modes at EL3.

Correct this error by using arm_is_el3_or_mon() instead, which
returns true when the CPU is at AArch64 EL3 or AArch32 Monitor mode.
(Since the new callsites are compiled also for the linux-user mode,
we need to provide a dummy implementation for CONFIG_USER_ONLY.)

This affects only:
 * trapping of ERRIDR via SCR.TERR
 * trapping of the debug channel registers via SDCR.TDCC
 * trapping of GICv3 registers via SCR.IRQ and SCR.FIQ
   (which we already used arm_is_el3_or_mon() for)

This patch changes the handling of SCR.TERR and SDCR.TDCC. This
patch only changes guest-visible behaviour for "-cpu max" on
the qemu-system-arm binary, because SCR.TERR
and SDCR.TDCC (and indeed the entire SDCR register) only arrived
in Armv8, and the only guest CPU we support which has any v8
features and also starts in AArch32 EL3 is the 32-bit 'max'.

Other uses of CP_ACCESS_TRAP_EL3 don't need changing:

 * uses in code paths that can't happen when EL3 is AArch32:
   access_trap_aa32s_el1, cpacr_access, cptr_access, nsacr_access
 * uses which are in accessfns for AArch64-only registers:
   gt_stimer_access, gt_cntpoff_access, access_hxen, access_tpidr2,
   access_smpri, access_smprimap, access_lor_ns, access_pauth,
   access_mte, access_tfsr_el2, access_scxtnum, access_fgt
 * trap bits which exist only in the AArch64 version of the
   trap register, not the AArch32 one:
   access_tpm, pmreg_access, access_dbgvcr32, access_tdra,
   access_tda, access_tdosa (TPM, TDA and TDOSA exist only in
   MDCR_EL3, not in SDCR, and we enforce this in sdcr_write())

Cc: qemu-stable@nongnu.org
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu.h          | 5 +++++
 target/arm/debug_helper.c | 3 ++-
 target/arm/helper.c       | 2 +-
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 4cb672c120b..ae1e8b1c779 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2570,6 +2570,11 @@ static inline bool arm_is_secure_below_el3(CPUARMState *env)
     return false;
 }
 
+static inline bool arm_is_el3_or_mon(CPUARMState *env)
+{
+    return false;
+}
+
 static inline ARMSecuritySpace arm_security_space(CPUARMState *env)
 {
     return ARMSS_NonSecure;
diff --git a/target/arm/debug_helper.c b/target/arm/debug_helper.c
index 2212ef4a3b9..c3c1eb5f628 100644
--- a/target/arm/debug_helper.c
+++ b/target/arm/debug_helper.c
@@ -880,7 +880,8 @@ static CPAccessResult access_tdcc(CPUARMState *env, const ARMCPRegInfo *ri,
     if (el < 2 && (mdcr_el2_tda || mdcr_el2_tdcc)) {
         return CP_ACCESS_TRAP_EL2;
     }
-    if (el < 3 && ((env->cp15.mdcr_el3 & MDCR_TDA) || mdcr_el3_tdcc)) {
+    if (!arm_is_el3_or_mon(env) &&
+        ((env->cp15.mdcr_el3 & MDCR_TDA) || mdcr_el3_tdcc)) {
         return CP_ACCESS_TRAP_EL3;
     }
     return CP_ACCESS_OK;
diff --git a/target/arm/helper.c b/target/arm/helper.c
index c5cd27b249f..058a5af3aaf 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6103,7 +6103,7 @@ static CPAccessResult access_terr(CPUARMState *env, const ARMCPRegInfo *ri,
     if (el < 2 && (arm_hcr_el2_eff(env) & HCR_TERR)) {
         return CP_ACCESS_TRAP_EL2;
     }
-    if (el < 3 && (env->cp15.scr_el3 & SCR_TERR)) {
+    if (!arm_is_el3_or_mon(env) && (env->cp15.scr_el3 & SCR_TERR)) {
         return CP_ACCESS_TRAP_EL3;
     }
     return CP_ACCESS_OK;
-- 
2.34.1

[PATCH 08/14] hw/intc/arm_gicv3_cpuif(): Remove redundant tests of is_a64()

[Peter Maydell]

In the gicv3_{irq,fiq,irqfiq}_access() functions, in the
arm_current_el(env) == 3 case we do the following test:
    if (!is_a64(env) && !arm_is_el3_or_mon(env)) {
        r = CP_ACCESS_TRAP_EL3;
    }

In this check, the "!is_a64(env)" is redundant, because if
we are at EL3 and in AArch64 then arm_is_el3_or_mon() will
return true and we will skip the if() body anyway.

Remove the unnecessary tests.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/arm_gicv3_cpuif.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
index 8a715b3510b..7f1d071c198 100644
--- a/hw/intc/arm_gicv3_cpuif.c
+++ b/hw/intc/arm_gicv3_cpuif.c
@@ -2291,7 +2291,7 @@ static CPAccessResult gicv3_irqfiq_access(CPUARMState *env,
             r = CP_ACCESS_TRAP_EL3;
             break;
         case 3:
-            if (!is_a64(env) && !arm_is_el3_or_mon(env)) {
+            if (!arm_is_el3_or_mon(env)) {
                 r = CP_ACCESS_TRAP_EL3;
             }
             break;
@@ -2353,7 +2353,7 @@ static CPAccessResult gicv3_fiq_access(CPUARMState *env,
             r = CP_ACCESS_TRAP_EL3;
             break;
         case 3:
-            if (!is_a64(env) && !arm_is_el3_or_mon(env)) {
+            if (!arm_is_el3_or_mon(env)) {
                 r = CP_ACCESS_TRAP_EL3;
             }
             break;
@@ -2389,7 +2389,7 @@ static CPAccessResult gicv3_irq_access(CPUARMState *env,
             r = CP_ACCESS_TRAP_EL3;
             break;
         case 3:
-            if (!is_a64(env) && !arm_is_el3_or_mon(env)) {
+            if (!arm_is_el3_or_mon(env)) {
                 r = CP_ACCESS_TRAP_EL3;
             }
             break;
-- 
2.34.1

[PATCH 09/14] target/arm: Support CP_ACCESS_TRAP_EL1 as a CPAccessResult

[Peter Maydell]

In the CPAccessResult enum, the CP_ACCESS_TRAP* values indicate the
equivalent of the pseudocode AArch64.SystemAccessTrap(..., 0x18),
causing a trap to a specified exception level with a syndrome value
giving information about the failing instructions.  In the
pseudocode, such traps are always taken to a specified target EL.  We
support that for target EL of 2 or 3 via CP_ACCESS_TRAP_EL2 and
CP_ACCESS_TRAP_EL3, but the only way to take the access trap to EL1
currently is to use CP_ACCESS_TRAP, which takes the trap to the
"usual target EL" (EL1 if in EL0, otherwise to the current EL).

Add CP_ACCESS_TRAP_EL1 so that access functions can follow the
pseudocode more closely.

(Note that for the common case in the pseudocode of "trap to
EL2 if HCR_EL2.TGE is set, otherwise trap to EL1", we handle
this in raise_exception(), so access functions don't need to
special case it and can use CP_ACCESS_TRAP_EL1.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpregs.h        | 1 +
 target/arm/tcg/op_helper.c | 6 ++++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/target/arm/cpregs.h b/target/arm/cpregs.h
index 1759d9defbe..fbf5798069d 100644
--- a/target/arm/cpregs.h
+++ b/target/arm/cpregs.h
@@ -331,6 +331,7 @@ typedef enum CPAccessResult {
      * 0xc or 0x18).
      */
     CP_ACCESS_TRAP = (1 << 2),
+    CP_ACCESS_TRAP_EL1 = CP_ACCESS_TRAP | 1,
     CP_ACCESS_TRAP_EL2 = CP_ACCESS_TRAP | 2,
     CP_ACCESS_TRAP_EL3 = CP_ACCESS_TRAP | 3,
 
diff --git a/target/arm/tcg/op_helper.c b/target/arm/tcg/op_helper.c
index 1ba727e8e9f..c427118655d 100644
--- a/target/arm/tcg/op_helper.c
+++ b/target/arm/tcg/op_helper.c
@@ -781,7 +781,7 @@ const void *HELPER(access_check_cp_reg)(CPUARMState *env, uint32_t key,
      * the other trap takes priority. So we take the "check HSTR_EL2" path
      * for all of those cases.)
      */
-    if (res != CP_ACCESS_OK && ((res & CP_ACCESS_EL_MASK) == 0) &&
+    if (res != CP_ACCESS_OK && ((res & CP_ACCESS_EL_MASK) < 2) &&
         arm_current_el(env) == 0) {
         goto fail;
     }
@@ -887,6 +887,9 @@ const void *HELPER(access_check_cp_reg)(CPUARMState *env, uint32_t key,
     case 0:
         target_el = exception_target_el(env);
         break;
+    case 1:
+        assert(arm_current_el(env) < 2);
+        break;
     case 2:
         assert(arm_current_el(env) != 3);
         assert(arm_is_el2_enabled(env));
@@ -895,7 +898,6 @@ const void *HELPER(access_check_cp_reg)(CPUARMState *env, uint32_t key,
         assert(arm_feature(env, ARM_FEATURE_EL3));
         break;
     default:
-        /* No "direct" traps to EL1 */
         g_assert_not_reached();
     }
 
-- 
2.34.1

[PATCH 10/14] target/arm: Use CP_ACCESS_TRAP_EL1 for traps that are always to EL1

[Peter Maydell]

We currently use CP_ACCESS_TRAP in a number of access functions where
we know we're currently at EL0; in this case the "usual target EL"
is EL1, so CP_ACCESS_TRAP and CP_ACCESS_TRAP_EL1 behave the same.
Use CP_ACCESS_TRAP_EL1 to more closely match the pseudocode for
this sort of check.

Note that in the case of the access functions foc cacheop to
PoC or PoU, the code was correct but the comment was wrong:
SCTLR_EL1.UCI traps for DC CVAC, DC CIVAC, DC CVAP, DC CVADP,
DC CVAU and IC IVAU should be system access traps, not UNDEFs.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/debug_helper.c |  2 +-
 target/arm/helper.c       | 30 +++++++++++++++---------------
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/target/arm/debug_helper.c b/target/arm/debug_helper.c
index c3c1eb5f628..36bffde74e9 100644
--- a/target/arm/debug_helper.c
+++ b/target/arm/debug_helper.c
@@ -875,7 +875,7 @@ static CPAccessResult access_tdcc(CPUARMState *env, const ARMCPRegInfo *ri,
                                           (env->cp15.mdcr_el3 & MDCR_TDCC);
 
     if (el < 1 && mdscr_el1_tdcc) {
-        return CP_ACCESS_TRAP;
+        return CP_ACCESS_TRAP_EL1;
     }
     if (el < 2 && (mdcr_el2_tda || mdcr_el2_tdcc)) {
         return CP_ACCESS_TRAP_EL2;
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 058a5af3aaf..d1e26ec9d06 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -881,7 +881,7 @@ static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri,
     uint64_t mdcr_el2 = arm_mdcr_el2_eff(env);
 
     if (el == 0 && !(env->cp15.c9_pmuserenr & 1)) {
-        return CP_ACCESS_TRAP;
+        return CP_ACCESS_TRAP_EL1;
     }
     if (el < 2 && (mdcr_el2 & MDCR_TPM)) {
         return CP_ACCESS_TRAP_EL2;
@@ -2159,7 +2159,7 @@ static CPAccessResult teehbr_access(CPUARMState *env, const ARMCPRegInfo *ri,
                                     bool isread)
 {
     if (arm_current_el(env) == 0 && (env->teecr & 1)) {
-        return CP_ACCESS_TRAP;
+        return CP_ACCESS_TRAP_EL1;
     }
     return teecr_access(env, ri, isread);
 }
@@ -2239,7 +2239,7 @@ static CPAccessResult gt_cntfrq_access(CPUARMState *env, const ARMCPRegInfo *ri,
             cntkctl = env->cp15.c14_cntkctl;
         }
         if (!extract32(cntkctl, 0, 2)) {
-            return CP_ACCESS_TRAP;
+            return CP_ACCESS_TRAP_EL1;
         }
         break;
     case 1:
@@ -2278,7 +2278,7 @@ static CPAccessResult gt_counter_access(CPUARMState *env, int timeridx,
 
         /* CNT[PV]CT: not visible from PL0 if EL0[PV]CTEN is zero */
         if (!extract32(env->cp15.c14_cntkctl, timeridx, 1)) {
-            return CP_ACCESS_TRAP;
+            return CP_ACCESS_TRAP_EL1;
         }
         /* fall through */
     case 1:
@@ -2319,7 +2319,7 @@ static CPAccessResult gt_timer_access(CPUARMState *env, int timeridx,
          * EL0 if EL0[PV]TEN is zero.
          */
         if (!extract32(env->cp15.c14_cntkctl, 9 - timeridx, 1)) {
-            return CP_ACCESS_TRAP;
+            return CP_ACCESS_TRAP_EL1;
         }
         /* fall through */
 
@@ -4499,7 +4499,7 @@ static CPAccessResult aa64_daif_access(CPUARMState *env, const ARMCPRegInfo *ri,
                                        bool isread)
 {
     if (arm_current_el(env) == 0 && !(arm_sctlr(env, 0) & SCTLR_UMA)) {
-        return CP_ACCESS_TRAP;
+        return CP_ACCESS_TRAP_EL1;
     }
     return CP_ACCESS_OK;
 }
@@ -4589,9 +4589,9 @@ static CPAccessResult aa64_cacheop_poc_access(CPUARMState *env,
     /* Cache invalidate/clean to Point of Coherency or Persistence...  */
     switch (arm_current_el(env)) {
     case 0:
-        /* ... EL0 must UNDEF unless SCTLR_EL1.UCI is set.  */
+        /* ... EL0 must trap to EL1 unless SCTLR_EL1.UCI is set.  */
         if (!(arm_sctlr(env, 0) & SCTLR_UCI)) {
-            return CP_ACCESS_TRAP;
+            return CP_ACCESS_TRAP_EL1;
         }
         /* fall through */
     case 1:
@@ -4609,9 +4609,9 @@ static CPAccessResult do_cacheop_pou_access(CPUARMState *env, uint64_t hcrflags)
     /* Cache invalidate/clean to Point of Unification... */
     switch (arm_current_el(env)) {
     case 0:
-        /* ... EL0 must UNDEF unless SCTLR_EL1.UCI is set.  */
+        /* ... EL0 must trap to EL1 unless SCTLR_EL1.UCI is set.  */
         if (!(arm_sctlr(env, 0) & SCTLR_UCI)) {
-            return CP_ACCESS_TRAP;
+            return CP_ACCESS_TRAP_EL1;
         }
         /* fall through */
     case 1:
@@ -4651,7 +4651,7 @@ static CPAccessResult aa64_zva_access(CPUARMState *env, const ARMCPRegInfo *ri,
                 }
             } else {
                 if (!(env->cp15.sctlr_el[1] & SCTLR_DZE)) {
-                    return CP_ACCESS_TRAP;
+                    return CP_ACCESS_TRAP_EL1;
                 }
                 if (hcr & HCR_TDZ) {
                     return CP_ACCESS_TRAP_EL2;
@@ -6073,7 +6073,7 @@ static CPAccessResult ctr_el0_access(CPUARMState *env, const ARMCPRegInfo *ri,
                 }
             } else {
                 if (!(env->cp15.sctlr_el[1] & SCTLR_UCT)) {
-                    return CP_ACCESS_TRAP;
+                    return CP_ACCESS_TRAP_EL1;
                 }
                 if (hcr & HCR_TID2) {
                     return CP_ACCESS_TRAP_EL2;
@@ -6372,7 +6372,7 @@ static CPAccessResult access_tpidr2(CPUARMState *env, const ARMCPRegInfo *ri,
     if (el == 0) {
         uint64_t sctlr = arm_sctlr(env, el);
         if (!(sctlr & SCTLR_EnTP2)) {
-            return CP_ACCESS_TRAP;
+            return CP_ACCESS_TRAP_EL1;
         }
     }
     /* TODO: FEAT_FGT */
@@ -7172,7 +7172,7 @@ static CPAccessResult access_scxtnum(CPUARMState *env, const ARMCPRegInfo *ri,
             if (hcr & HCR_TGE) {
                 return CP_ACCESS_TRAP_EL2;
             }
-            return CP_ACCESS_TRAP;
+            return CP_ACCESS_TRAP_EL1;
         }
     } else if (el < 2 && (env->cp15.sctlr_el[2] & SCTLR_TSCXT)) {
         return CP_ACCESS_TRAP_EL2;
@@ -7292,7 +7292,7 @@ static CPAccessResult access_predinv(CPUARMState *env, const ARMCPRegInfo *ri,
     if (el == 0) {
         uint64_t sctlr = arm_sctlr(env, el);
         if (!(sctlr & SCTLR_EnRCTX)) {
-            return CP_ACCESS_TRAP;
+            return CP_ACCESS_TRAP_EL1;
         }
     } else if (el == 1) {
         uint64_t hcr = arm_hcr_el2_eff(env);
-- 
2.34.1

[PATCH 11/14] target/arm: Use TRAP_UNCATEGORIZED for XScale CPAR traps

[Peter Maydell]

On XScale CPUs, there is no EL2 or AArch64, so no syndrome register.
These traps are just UNDEFs in the traditional AArch32 sense, so
CP_ACCESS_TRAP_UNCATEGORIZED is more accurate than CP_ACCESS_TRAP.
This has no visible behavioural change, because the guest doesn't
have a way to see the syndrome value we generate.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/tcg/op_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/tcg/op_helper.c b/target/arm/tcg/op_helper.c
index c427118655d..c69d2ac643f 100644
--- a/target/arm/tcg/op_helper.c
+++ b/target/arm/tcg/op_helper.c
@@ -764,7 +764,7 @@ const void *HELPER(access_check_cp_reg)(CPUARMState *env, uint32_t key,
 
     if (arm_feature(env, ARM_FEATURE_XSCALE) && ri->cp < 14
         && extract32(env->cp15.c15_cpar, ri->cp, 1) == 0) {
-        res = CP_ACCESS_TRAP;
+        res = CP_ACCESS_TRAP_UNCATEGORIZED;
         goto fail;
     }
 
-- 
2.34.1

[PATCH 12/14] target/arm: Remove CP_ACCESS_TRAP handling

[Peter Maydell]

There are no longer any uses of CP_ACCESS_TRAP in access functions,
because we have converted them all to use either CP_ACCESS_TRAP_EL1
or CP_ACCESS_TRAP_UNCATEGORIZED, as appropriate. Remove the handling
of bare CP_ACCESS_TRAP from the access_check_cp_reg() helper, so that
it now asserts if an access function returns it.

Rename CP_ACCESS_TRAP to CP_ACCESS_TRAP_BIT, to make it clearer
that this is an internal-only definition, not something that
it makes sense to return from an access function. This should
help to avoid future bugs where we return the wrong syndrome
value by mistake.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpregs.h        | 11 ++++++-----
 target/arm/tcg/op_helper.c | 13 ++++++++-----
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/target/arm/cpregs.h b/target/arm/cpregs.h
index fbf5798069d..fb3b84baa1e 100644
--- a/target/arm/cpregs.h
+++ b/target/arm/cpregs.h
@@ -328,12 +328,13 @@ typedef enum CPAccessResult {
      * Access fails due to a configurable trap or enable which would
      * result in a categorized exception syndrome giving information about
      * the failing instruction (ie syndrome category 0x3, 0x4, 0x5, 0x6,
-     * 0xc or 0x18).
+     * 0xc or 0x18). These traps are always to a specified target EL,
+     * never to the usual target EL.
      */
-    CP_ACCESS_TRAP = (1 << 2),
-    CP_ACCESS_TRAP_EL1 = CP_ACCESS_TRAP | 1,
-    CP_ACCESS_TRAP_EL2 = CP_ACCESS_TRAP | 2,
-    CP_ACCESS_TRAP_EL3 = CP_ACCESS_TRAP | 3,
+    CP_ACCESS_TRAP_BIT = (1 << 2),
+    CP_ACCESS_TRAP_EL1 = CP_ACCESS_TRAP_BIT | 1,
+    CP_ACCESS_TRAP_EL2 = CP_ACCESS_TRAP_BIT | 2,
+    CP_ACCESS_TRAP_EL3 = CP_ACCESS_TRAP_BIT | 3,
 
     /*
      * Access fails and results in an exception syndrome 0x0 ("uncategorized").
diff --git a/target/arm/tcg/op_helper.c b/target/arm/tcg/op_helper.c
index c69d2ac643f..fcee11e29ad 100644
--- a/target/arm/tcg/op_helper.c
+++ b/target/arm/tcg/op_helper.c
@@ -853,21 +853,24 @@ 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:
+    switch (res) {
+        /* CP_ACCESS_TRAP* traps are always direct to a specified EL */
+    case CP_ACCESS_TRAP_EL3:
         /*
          * 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)) {
+        if (!arm_el_is_aa64(env, 3)) {
             excp = EXCP_MON_TRAP;
         }
         break;
+    case CP_ACCESS_TRAP_EL2:
+    case CP_ACCESS_TRAP_EL1:
+        break;
     case CP_ACCESS_TRAP_UNCATEGORIZED:
-        /* Only CP_ACCESS_TRAP traps are direct to a specified EL */
-        assert((res & CP_ACCESS_EL_MASK) == 0);
+        /* CP_ACCESS_TRAP_UNCATEGORIZED is never direct to a specified EL */
         if (cpu_isar_feature(aa64_ids, cpu) && isread &&
             arm_cpreg_in_idspace(ri)) {
             /*
-- 
2.34.1

[PATCH 13/14] target/arm: Rename CP_ACCESS_TRAP_UNCATEGORIZED to CP_ACCESS_UNDEFINED

[Peter Maydell]

CP_ACCESS_TRAP_UNCATEGORIZED is technically an accurate description
of what this return value from a cpreg accessfn does, but it's liable
to confusion because it doesn't match how the Arm ARM pseudocode
indicates this case. What it does is an EXCP_UDEF with a zero
("uncategorized") syndrome value, which is what an UNDEFINED instruction
does. The pseudocode uses "UNDEFINED" to show this; rename our
constant to CP_ACCESS_UNDEFINED to make the parallel clearer.

Commit created with
sed -i -e 's/CP_ACCESS_TRAP_UNCATEGORIZED/CP_ACCESS_UNDEFINED/' $(git grep -l CP_ACCESS_TRAP_UNCATEGORIZED)

plus manual editing of the comment.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpregs.h        |  5 +++--
 target/arm/helper.c        | 30 +++++++++++++++---------------
 target/arm/tcg/op_helper.c |  6 +++---
 3 files changed, 21 insertions(+), 20 deletions(-)

diff --git a/target/arm/cpregs.h b/target/arm/cpregs.h
index fb3b84baa1e..52377c6eb50 100644
--- a/target/arm/cpregs.h
+++ b/target/arm/cpregs.h
@@ -337,13 +337,14 @@ typedef enum CPAccessResult {
     CP_ACCESS_TRAP_EL3 = CP_ACCESS_TRAP_BIT | 3,
 
     /*
-     * Access fails and results in an exception syndrome 0x0 ("uncategorized").
+     * Access fails with UNDEFINED, i.e. an exception syndrome 0x0
+     * ("uncategorized"), which is what an undefined insn produces.
      * Note that this is not a catch-all case -- the set of cases which may
      * result in this failure is specifically defined by the architecture.
      * This trap is always to the usual target EL, never directly to a
      * specified target EL.
      */
-    CP_ACCESS_TRAP_UNCATEGORIZED = (2 << 2),
+    CP_ACCESS_UNDEFINED = (2 << 2),
 } CPAccessResult;
 
 /* Indexes into fgt_read[] */
diff --git a/target/arm/helper.c b/target/arm/helper.c
index d1e26ec9d06..1fee8fae127 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -285,7 +285,7 @@ static CPAccessResult access_el3_aa32ns(CPUARMState *env,
 {
     if (!is_a64(env) && arm_current_el(env) == 3 &&
         arm_is_secure_below_el3(env)) {
-        return CP_ACCESS_TRAP_UNCATEGORIZED;
+        return CP_ACCESS_UNDEFINED;
     }
     return CP_ACCESS_OK;
 }
@@ -310,7 +310,7 @@ static CPAccessResult access_trap_aa32s_el1(CPUARMState *env,
         return CP_ACCESS_TRAP_EL3;
     }
     /* This will be EL1 NS and EL2 NS, which just UNDEF */
-    return CP_ACCESS_TRAP_UNCATEGORIZED;
+    return CP_ACCESS_UNDEFINED;
 }
 
 /*
@@ -2246,7 +2246,7 @@ static CPAccessResult gt_cntfrq_access(CPUARMState *env, const ARMCPRegInfo *ri,
         if (!isread && ri->state == ARM_CP_STATE_AA32 &&
             arm_is_secure_below_el3(env)) {
             /* Accesses from 32-bit Secure EL1 UNDEF (*not* trap to EL3!) */
-            return CP_ACCESS_TRAP_UNCATEGORIZED;
+            return CP_ACCESS_UNDEFINED;
         }
         break;
     case 2:
@@ -2255,7 +2255,7 @@ static CPAccessResult gt_cntfrq_access(CPUARMState *env, const ARMCPRegInfo *ri,
     }
 
     if (!isread && el < arm_highest_el(env)) {
-        return CP_ACCESS_TRAP_UNCATEGORIZED;
+        return CP_ACCESS_UNDEFINED;
     }
 
     return CP_ACCESS_OK;
@@ -2385,7 +2385,7 @@ static CPAccessResult gt_stimer_access(CPUARMState *env,
     switch (arm_current_el(env)) {
     case 1:
         if (!arm_is_secure(env)) {
-            return CP_ACCESS_TRAP_UNCATEGORIZED;
+            return CP_ACCESS_UNDEFINED;
         }
         if (!(env->cp15.scr_el3 & SCR_ST)) {
             return CP_ACCESS_TRAP_EL3;
@@ -2393,7 +2393,7 @@ static CPAccessResult gt_stimer_access(CPUARMState *env,
         return CP_ACCESS_OK;
     case 0:
     case 2:
-        return CP_ACCESS_TRAP_UNCATEGORIZED;
+        return CP_ACCESS_UNDEFINED;
     case 3:
         return CP_ACCESS_OK;
     default:
@@ -3304,7 +3304,7 @@ static CPAccessResult ats_access(CPUARMState *env, const ARMCPRegInfo *ri,
                 }
                 return CP_ACCESS_TRAP_EL3;
             }
-            return CP_ACCESS_TRAP_UNCATEGORIZED;
+            return CP_ACCESS_UNDEFINED;
         }
     }
     return CP_ACCESS_OK;
@@ -3601,7 +3601,7 @@ static CPAccessResult at_e012_access(CPUARMState *env, const ARMCPRegInfo *ri,
      * scr_write() ensures that the NSE bit is not set otherwise.
      */
     if ((env->cp15.scr_el3 & (SCR_NSE | SCR_NS)) == SCR_NSE) {
-        return CP_ACCESS_TRAP_UNCATEGORIZED;
+        return CP_ACCESS_UNDEFINED;
     }
     return CP_ACCESS_OK;
 }
@@ -3611,7 +3611,7 @@ static CPAccessResult at_s1e2_access(CPUARMState *env, const ARMCPRegInfo *ri,
 {
     if (arm_current_el(env) == 3 &&
         !(env->cp15.scr_el3 & (SCR_NS | SCR_EEL2))) {
-        return CP_ACCESS_TRAP_UNCATEGORIZED;
+        return CP_ACCESS_UNDEFINED;
     }
     return at_e012_access(env, ri, isread);
 }
@@ -4684,7 +4684,7 @@ static CPAccessResult sp_el0_access(CPUARMState *env, const ARMCPRegInfo *ri,
          * Access to SP_EL0 is undefined if it's being used as
          * the stack pointer.
          */
-        return CP_ACCESS_TRAP_UNCATEGORIZED;
+        return CP_ACCESS_UNDEFINED;
     }
     return CP_ACCESS_OK;
 }
@@ -5674,7 +5674,7 @@ static CPAccessResult sel2_access(CPUARMState *env, const ARMCPRegInfo *ri,
     if (arm_current_el(env) == 3 || arm_is_secure_below_el3(env)) {
         return CP_ACCESS_OK;
     }
-    return CP_ACCESS_TRAP_UNCATEGORIZED;
+    return CP_ACCESS_UNDEFINED;
 }
 
 static const ARMCPRegInfo el2_sec_cp_reginfo[] = {
@@ -5710,7 +5710,7 @@ static CPAccessResult nsacr_access(CPUARMState *env, const ARMCPRegInfo *ri,
     if (isread) {
         return CP_ACCESS_OK;
     }
-    return CP_ACCESS_TRAP_UNCATEGORIZED;
+    return CP_ACCESS_UNDEFINED;
 }
 
 static const ARMCPRegInfo el3_cp_reginfo[] = {
@@ -5798,7 +5798,7 @@ static CPAccessResult e2h_access(CPUARMState *env, const ARMCPRegInfo *ri,
         return CP_ACCESS_OK;
     }
     if (!(arm_hcr_el2_eff(env) & HCR_E2H)) {
-        return CP_ACCESS_TRAP_UNCATEGORIZED;
+        return CP_ACCESS_UNDEFINED;
     }
     return CP_ACCESS_OK;
 }
@@ -5896,7 +5896,7 @@ static CPAccessResult el2_e2h_e12_access(CPUARMState *env,
     }
     /* FOO_EL12 aliases only exist when E2H is 1; otherwise they UNDEF */
     if (!(arm_hcr_el2_eff(env) & HCR_E2H)) {
-        return CP_ACCESS_TRAP_UNCATEGORIZED;
+        return CP_ACCESS_UNDEFINED;
     }
     if (ri->orig_accessfn) {
         return ri->orig_accessfn(env, ri->opaque, isread);
@@ -6751,7 +6751,7 @@ static CPAccessResult access_lor_other(CPUARMState *env,
 {
     if (arm_is_secure_below_el3(env)) {
         /* UNDEF if SCR_EL3.NS == 0 */
-        return CP_ACCESS_TRAP_UNCATEGORIZED;
+        return CP_ACCESS_UNDEFINED;
     }
     return access_lor_ns(env, ri, isread);
 }
diff --git a/target/arm/tcg/op_helper.c b/target/arm/tcg/op_helper.c
index fcee11e29ad..2230351a8f4 100644
--- a/target/arm/tcg/op_helper.c
+++ b/target/arm/tcg/op_helper.c
@@ -764,7 +764,7 @@ const void *HELPER(access_check_cp_reg)(CPUARMState *env, uint32_t key,
 
     if (arm_feature(env, ARM_FEATURE_XSCALE) && ri->cp < 14
         && extract32(env->cp15.c15_cpar, ri->cp, 1) == 0) {
-        res = CP_ACCESS_TRAP_UNCATEGORIZED;
+        res = CP_ACCESS_UNDEFINED;
         goto fail;
     }
 
@@ -869,8 +869,8 @@ const void *HELPER(access_check_cp_reg)(CPUARMState *env, uint32_t key,
     case CP_ACCESS_TRAP_EL2:
     case CP_ACCESS_TRAP_EL1:
         break;
-    case CP_ACCESS_TRAP_UNCATEGORIZED:
-        /* CP_ACCESS_TRAP_UNCATEGORIZED is never direct to a specified EL */
+    case CP_ACCESS_UNDEFINED:
+        /* CP_ACCESS_UNDEFINED is never direct to a specified EL */
         if (cpu_isar_feature(aa64_ids, cpu) && isread &&
             arm_cpreg_in_idspace(ri)) {
             /*
-- 
2.34.1

[PATCH 14/14] target/arm: Correct errors in WFI/WFE trapping

[Peter Maydell]

The code for WFI/WFE trapping has several errors:
 * it wasn't using arm_sctlr(), so it would look at SCTLR_EL1
   even if the CPU was in the EL2&0 translation regime
 * it was raising UNDEF, not Monitor Trap, for traps to
   AArch32 EL3 because of SCR.{TWE,TWI}
 * it was not honouring SCR.{TWE,TWI} when running in
   AArch32 at EL3 not in Monitor mode
 * it checked SCR.{TWE,TWI} even on v7 CPUs which don't have
   those bits

Fix these bugs.

Cc: qemu-stable@nongnu.org
Fixes: b1eced713d99 ("target-arm: Add WFx instruction trap support")
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/tcg/op_helper.c | 37 ++++++++++++++++++-------------------
 1 file changed, 18 insertions(+), 19 deletions(-)

diff --git a/target/arm/tcg/op_helper.c b/target/arm/tcg/op_helper.c
index 2230351a8f4..02c375d196d 100644
--- a/target/arm/tcg/op_helper.c
+++ b/target/arm/tcg/op_helper.c
@@ -313,15 +313,19 @@ void HELPER(check_bxj_trap)(CPUARMState *env, uint32_t rm)
 }
 
 #ifndef CONFIG_USER_ONLY
-/* Function checks whether WFx (WFI/WFE) instructions are set up to be trapped.
+/*
+ * Function checks whether WFx (WFI/WFE) instructions are set up to be trapped.
  * The function returns the target EL (1-3) if the instruction is to be trapped;
  * otherwise it returns 0 indicating it is not trapped.
+ * For a trap, *excp is updated with the EXCP_* trap type to use.
  */
-static inline int check_wfx_trap(CPUARMState *env, bool is_wfe)
+static inline int check_wfx_trap(CPUARMState *env, bool is_wfe, uint32_t *excp)
 {
     int cur_el = arm_current_el(env);
     uint64_t mask;
 
+    *excp = EXCP_UDEF;
+
     if (arm_feature(env, ARM_FEATURE_M)) {
         /* M profile cores can never trap WFI/WFE. */
         return 0;
@@ -331,18 +335,9 @@ static inline int check_wfx_trap(CPUARMState *env, bool is_wfe)
      * WFx instructions being trapped to EL1. These trap bits don't exist in v7.
      */
     if (cur_el < 1 && arm_feature(env, ARM_FEATURE_V8)) {
-        int target_el;
-
         mask = is_wfe ? SCTLR_nTWE : SCTLR_nTWI;
-        if (arm_is_secure_below_el3(env) && !arm_el_is_aa64(env, 3)) {
-            /* Secure EL0 and Secure PL1 is at EL3 */
-            target_el = 3;
-        } else {
-            target_el = 1;
-        }
-
-        if (!(env->cp15.sctlr_el[target_el] & mask)) {
-            return target_el;
+        if (!(arm_sctlr(env, cur_el) & mask)) {
+            return exception_target_el(env);
         }
     }
 
@@ -358,9 +353,12 @@ static inline int check_wfx_trap(CPUARMState *env, bool is_wfe)
     }
 
     /* We are not trapping to EL1 or EL2; trap to EL3 if SCR_EL3 requires it */
-    if (cur_el < 3) {
+    if (arm_feature(env, ARM_FEATURE_V8) && !arm_is_el3_or_mon(env)) {
         mask = (is_wfe) ? SCR_TWE : SCR_TWI;
         if (env->cp15.scr_el3 & mask) {
+            if (!arm_el_is_aa64(env, 3)) {
+                *excp = EXCP_MON_TRAP;
+            }
             return 3;
         }
     }
@@ -383,7 +381,8 @@ void HELPER(wfi)(CPUARMState *env, uint32_t insn_len)
     return;
 #else
     CPUState *cs = env_cpu(env);
-    int target_el = check_wfx_trap(env, false);
+    uint32_t excp;
+    int target_el = check_wfx_trap(env, false, &excp);
 
     if (cpu_has_work(cs)) {
         /* Don't bother to go into our "low power state" if
@@ -399,7 +398,7 @@ void HELPER(wfi)(CPUARMState *env, uint32_t insn_len)
             env->regs[15] -= insn_len;
         }
 
-        raise_exception(env, EXCP_UDEF, syn_wfx(1, 0xe, 0, insn_len == 2),
+        raise_exception(env, excp, syn_wfx(1, 0xe, 0, insn_len == 2),
                         target_el);
     }
 
@@ -424,7 +423,8 @@ void HELPER(wfit)(CPUARMState *env, uint64_t timeout)
 #else
     ARMCPU *cpu = env_archcpu(env);
     CPUState *cs = env_cpu(env);
-    int target_el = check_wfx_trap(env, false);
+    uint32_t excp;
+    int target_el = check_wfx_trap(env, false, &excp);
     /* The WFIT should time out when CNTVCT_EL0 >= the specified value. */
     uint64_t cntval = gt_get_countervalue(env);
     uint64_t offset = gt_virt_cnt_offset(env);
@@ -441,8 +441,7 @@ void HELPER(wfit)(CPUARMState *env, uint64_t timeout)
 
     if (target_el) {
         env->pc -= 4;
-        raise_exception(env, EXCP_UDEF, syn_wfx(1, 0xe, 0, false),
-                        target_el);
+        raise_exception(env, excp, syn_wfx(1, 0xe, 0, false), target_el);
     }
 
     if (uadd64_overflow(timeout, offset, &nexttick)) {
-- 
2.34.1

Re: [PATCH 11/14] target/arm: Use TRAP_UNCATEGORIZED for XScale CPAR traps

[Philippe Mathieu-Daudé]

On 30/1/25 19:23, Peter Maydell wrote:
> On XScale CPUs, there is no EL2 or AArch64, so no syndrome register.
> These traps are just UNDEFs in the traditional AArch32 sense, so
> CP_ACCESS_TRAP_UNCATEGORIZED is more accurate than CP_ACCESS_TRAP.
> This has no visible behavioural change, because the guest doesn't
> have a way to see the syndrome value we generate.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/arm/tcg/op_helper.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 13/14] target/arm: Rename CP_ACCESS_TRAP_UNCATEGORIZED to CP_ACCESS_UNDEFINED

[Philippe Mathieu-Daudé]

On 30/1/25 19:23, Peter Maydell wrote:
> CP_ACCESS_TRAP_UNCATEGORIZED is technically an accurate description
> of what this return value from a cpreg accessfn does, but it's liable
> to confusion because it doesn't match how the Arm ARM pseudocode
> indicates this case. What it does is an EXCP_UDEF with a zero
> ("uncategorized") syndrome value, which is what an UNDEFINED instruction
> does. The pseudocode uses "UNDEFINED" to show this; rename our
> constant to CP_ACCESS_UNDEFINED to make the parallel clearer.
> 
> Commit created with
> sed -i -e 's/CP_ACCESS_TRAP_UNCATEGORIZED/CP_ACCESS_UNDEFINED/' $(git grep -l CP_ACCESS_TRAP_UNCATEGORIZED)
> 
> plus manual editing of the comment.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/arm/cpregs.h        |  5 +++--
>   target/arm/helper.c        | 30 +++++++++++++++---------------
>   target/arm/tcg/op_helper.c |  6 +++---
>   3 files changed, 21 insertions(+), 20 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 01/14] target/arm: Report correct syndrome for UNDEFINED CNTPS_*_EL1 from EL2 and NS EL1

[Alex Bennée]

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

> The access pseudocode for the CNTPS_TVAL_EL1, CNTPS_CTL_EL1 and
> CNTPS_CVAL_EL1 secure timer registers says that they are UNDEFINED
> from EL2 or NS EL1.  We incorrectly return CP_ACCESS_TRAP from the
> access function in these cases, which means that we report the wrong
> syndrome value to the target EL.
>
> Use CP_ACCESS_TRAP_UNCATEGORIZED, which reports the correct syndrome
> value for an UNDEFINED instruction.
>
> Cc: qemu-stable@nongnu.org
> Fixes: b4d3978c2fd ("target-arm: Add the AArch64 view of the Secure physical timer")
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 04/14] target/arm: Report correct syndrome for UNDEFINED LOR sysregs when NS=0

[Alex Bennée]

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

> The pseudocode for the accessors for the LOR sysregs says they
> are UNDEFINED if SCR_EL3.NS is 0. We were reporting the wrong
> syndrome value here; use CP_ACCESS_TRAP_UNCATEGORIZED.
>
> Cc: qemu-stable@nongnu.org
> Fixes: 2d7137c10faf ("target/arm: Implement the ARMv8.1-LOR extension")
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

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

Re: [PATCH 06/14] hw/intc/arm_gicv3_cpuif: Don't downgrade monitor traps for AArch32 EL3

[Alex Bennée]

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

> In the gicv3_{irq,fiq,irqfiq}_access() functions, there is a check
> which downgrades a CP_ACCESS_TRAP_EL3 to CP_ACCESS_TRAP if EL3 is not
> AArch64.  This has been there since the GIC was first implemented,
> but it isn't right: if we are trapping because of SCR.IRQ or SCR.FIQ
> then we definitely want to be going to EL3 (doing
> AArch32.TakeMonitorTrapException() in pseudocode terms).  We might
> want to not take a trap at all, but we don't ever want to go to the
> default target EL, because that would mean, for instance, taking a
> trap to Hyp mode if the trapped access was made from Hyp mode.
>
> (This might have been an attempt to work around our failure to
> properly implement Monitor Traps.)
>
> Remove the bogus check.
>
> Cc: qemu-stable@nongnu.org
> Fixes: 359fbe65e01e ("hw/intc/arm_gicv3: Implement GICv3 CPU interface registers")
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

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

[Peter Maydell]

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

Re: [PATCH 07/14] target/arm: Honour SDCR.TDCC and SCR.TERR in AArch32 EL3 non-Monitor modes

[Alex Bennée]

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

> There are not many traps in AArch32 which should trap to Monitor
> mode, but these trap bits should trap not just lower ELs to Monitor
> mode but also the non-Monitor modes running at EL3 (i.e.  Secure
> System, Secure Undef, etc).
>
> We get this wrong because the relevant access functions implement the
> AArch64-style logic of
>    if (el < 3 && trap_bit_set) {
>        return CP_ACCESS_TRAP_EL3;
>    }
> which won't trap the non-Monitor modes at EL3.
>
> Correct this error by using arm_is_el3_or_mon() instead, which
> returns true when the CPU is at AArch64 EL3 or AArch32 Monitor mode.
> (Since the new callsites are compiled also for the linux-user mode,
> we need to provide a dummy implementation for CONFIG_USER_ONLY.)
>
> This affects only:
>  * trapping of ERRIDR via SCR.TERR
>  * trapping of the debug channel registers via SDCR.TDCC
>  * trapping of GICv3 registers via SCR.IRQ and SCR.FIQ
>    (which we already used arm_is_el3_or_mon() for)
>
> This patch changes the handling of SCR.TERR and SDCR.TDCC. This
> patch only changes guest-visible behaviour for "-cpu max" on
> the qemu-system-arm binary, because SCR.TERR
> and SDCR.TDCC (and indeed the entire SDCR register) only arrived
> in Armv8, and the only guest CPU we support which has any v8
> features and also starts in AArch32 EL3 is the 32-bit 'max'.
>
> Other uses of CP_ACCESS_TRAP_EL3 don't need changing:
>
>  * uses in code paths that can't happen when EL3 is AArch32:
>    access_trap_aa32s_el1, cpacr_access, cptr_access, nsacr_access
>  * uses which are in accessfns for AArch64-only registers:
>    gt_stimer_access, gt_cntpoff_access, access_hxen, access_tpidr2,
>    access_smpri, access_smprimap, access_lor_ns, access_pauth,
>    access_mte, access_tfsr_el2, access_scxtnum, access_fgt
>  * trap bits which exist only in the AArch64 version of the
>    trap register, not the AArch32 one:
>    access_tpm, pmreg_access, access_dbgvcr32, access_tdra,
>    access_tda, access_tdosa (TPM, TDA and TDOSA exist only in
>    MDCR_EL3, not in SDCR, and we enforce this in sdcr_write())
>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 08/14] hw/intc/arm_gicv3_cpuif(): Remove redundant tests of is_a64()

[Alex Bennée]

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

> In the gicv3_{irq,fiq,irqfiq}_access() functions, in the
> arm_current_el(env) == 3 case we do the following test:
>     if (!is_a64(env) && !arm_is_el3_or_mon(env)) {
>         r = CP_ACCESS_TRAP_EL3;
>     }
>
> In this check, the "!is_a64(env)" is redundant, because if
> we are at EL3 and in AArch64 then arm_is_el3_or_mon() will
> return true and we will skip the if() body anyway.
>
> Remove the unnecessary tests.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 00/14] target/arm: Clean up some corner cases of sysreg traps

[Peter Maydell]

Ping for review on patches 2, 3, 9, 10, 12, 14, please?

thanks
-- PMM

On Thu, 30 Jan 2025 at 18:23, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> While reviewing Alex's recent secure timer patchset, I noticed a
> bug where it was using CP_ACCESS_TRAP when CP_ACCESS_TRAP_UNCATEGORIZED
> was wanted, and that we were making the same mistake elsewhere in
> our existing code. This series started out as an attempt to fix
> that and also rearrange the API so that it's harder to introduce
> other instances of this bug in future. In the process I noticed
> a different bug, where we weren't handling traps to AArch32
> Monitor mode correctly, so the series fixes those as well.
>
> The first four patches are fixes for the places where we were
> using CP_ACCESS_TRAP when we wanted CP_ACCESS_TRAP_UNCATEGORIZED.
> These are all situations where an attempt to access a sysreg
> should UNDEF and we were incorrectly reporting it with a
> syndrome value for a system register access trap. I've cc'd
> these to stable as they are technically bugs, but really the impact
> s very limited because I can't see a reason why any code except
> test code would deliberately attempt a sysreg access that they
> knew would take an exception and then look at the syndrome
> value for it.
>
> Patches 5, 6 and 7 together fix bugs in our handling of sysreg
> traps that should go to AArch32 Monitor mode:
>  * we were incorrectly triggering an UNDEF exception for these
>    rather than a Monitor Trap, so the exception would go to
>    the wrong vector table and the wrong CPU mode
>  * in most cases we weren't trapping accesses from EL3
>    non-Monitor modes to Monitor mode
> This affects only:
>  * trapping of ERRIDR via SCR.TERR
>  * trapping of the debug channel registers via SDCR.TDCC
>  * trapping of GICv3 registers via SCR.IRQ and SCR.FIQ
> because most "trap sysreg access to EL3" situations are either for
> AArch64 only registers or for trap bits that are AArch64 only.
>
> Patch 8 is a trivial one removing an unnecessary clause in
> an if() condition in the GIC cpuif access functions.
>
> Patches 9-13 are the API change that tries to make the bug harder to
> write: we add CP_ACCESS_TRAP_EL1 for "trap a sysreg access direct to
> EL1". After all the bugfixes in the first half of the series, the
> remaining uses of CP_ACCESS_TRAP are all in contexts where we know the
> current EL is 0, so we can directly replace them with
> CP_ACCESS_TRAP_EL1, and remove CP_ACCESS_TRAP entirely. We also rename
> CP_ACCESS_TRAP_UNCATEGORIZED to CP_ACCESS_UNDEFINED, to be a clearer
> parallel with the pseudocode's use of "UNDEFINED" in this situation.
> The resulting
> API is that an accessfn can only return:
>  CP_ACCESS_OK for a success
>  CP_ACCESS_UNDEFINED for an UNDEF
>  CP_ACCESS_TRAP_EL{1,2,3} for a sysreg trap direct to an EL
> and everything else is invalid. UNCATEGORIZED traps never go to a
> specified target EL, and sysreg-traps always go to a specified target
> EL, matching the pseudocode which either uses "UNDEFINED" or some kind
> of SystemAccessTrap(ELx, ...).
>
> Patch 14 fixes some issues with the WFI/WFX trap code, some of
> which are like the above sysreg access check bugs (not using
> Monitor Trap, not honouring traps in EL3 not-Monitor-mode),
> plus one which I noticed while working on the code (it doesn't
> use arm_sctlr() so will look at the wrong SCTLR when in EL2&0).
>
> I've cc'd the relevant patches to stable, but I don't think
> it's very likely that anybody ever ran into these bugs, because
> they're all cases of "we did the wrong thing if code at an EL
> below EL3 tried to do something it shouldn't". I don't think that
> EL3 code generally uses the trap bits for trap-and-emulate
> of functionality, only to prevent the lower ELs messing with
> state it should not, and everything here with the exception of
> SCR.IRQ and SCR.FIQ is pretty obscure anyway.
>
> (Tested somewhat lightly, because I don't have any test images
> that test AArch32 EL3 really.)
>
> thanks
> -- PMM
>
> Peter Maydell (14):
>   target/arm: Report correct syndrome for UNDEFINED CNTPS_*_EL1 from EL2
>     and NS EL1
>   target/arm: Report correct syndrome for UNDEFINED AT ops with wrong
>     NSE,NS
>   target/arm: Report correct syndrome for UNDEFINED S1E2 AT ops at EL3
>   target/arm: Report correct syndrome for UNDEFINED LOR sysregs when
>     NS=0
>   target/arm: Make CP_ACCESS_TRAPs to AArch32 EL3 be Monitor traps
>   hw/intc/arm_gicv3_cpuif: Don't downgrade monitor traps for AArch32 EL3
>   target/arm: Honour SDCR.TDCC and SCR.TERR in AArch32 EL3 non-Monitor
>     modes
>   hw/intc/arm_gicv3_cpuif(): Remove redundant tests of is_a64()
>   target/arm: Support CP_ACCESS_TRAP_EL1 as a CPAccessResult
>   target/arm: Use CP_ACCESS_TRAP_EL1 for traps that are always to EL1
>   target/arm: Use TRAP_UNCATEGORIZED for XScale CPAR traps
>   target/arm: Remove CP_ACCESS_TRAP handling
>   target/arm: Rename CP_ACCESS_TRAP_UNCATEGORIZED to CP_ACCESS_UNDEFINED
>   target/arm: Correct errors in WFI/WFE trapping
>
>  target/arm/cpregs.h        | 15 +++++---
>  target/arm/cpu.h           |  6 +++
>  hw/intc/arm_gicv3_cpuif.c  | 15 ++------
>  target/arm/debug_helper.c  |  5 ++-
>  target/arm/helper.c        | 75 ++++++++++++++++++++++----------------
>  target/arm/tcg/op_helper.c | 71 ++++++++++++++++++++++--------------
>  6 files changed, 107 insertions(+), 80 deletions(-)
>
> --

Re: [PATCH 01/14] target/arm: Report correct syndrome for UNDEFINED CNTPS_*_EL1 from EL2 and NS EL1

[Richard Henderson]

On 1/30/25 10:22, Peter Maydell wrote:
> The access pseudocode for the CNTPS_TVAL_EL1, CNTPS_CTL_EL1 and
> CNTPS_CVAL_EL1 secure timer registers says that they are UNDEFINED
> from EL2 or NS EL1.  We incorrectly return CP_ACCESS_TRAP from the
> access function in these cases, which means that we report the wrong
> syndrome value to the target EL.
> 
> Use CP_ACCESS_TRAP_UNCATEGORIZED, which reports the correct syndrome
> value for an UNDEFINED instruction.
> 
> Cc:qemu-stable@nongnu.org
> Fixes: b4d3978c2fd ("target-arm: Add the AArch64 view of the Secure physical timer")
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/helper.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 02/14] target/arm: Report correct syndrome for UNDEFINED AT ops with wrong NSE, NS

[Richard Henderson]

On 1/30/25 10:22, Peter Maydell wrote:
> R_NYXTL says that these AT insns should be UNDEFINED if they
> would operate on an EL lower than EL3 and SCR_EL3.{NSE,NS} is
> set to the Reserved {1, 0}. We were incorrectly reporting
> them with the wrong syndrome; use CP_ACCESS_TRAP_UNCATEGORIZED
> so they are reported as UNDEFINED.
> 
> Cc:qemu-stable@nongnu.org
> Fixes: 1acd00ef1410 ("target/arm/helper: Check SCR_EL3.{NSE, NS} encoding for AT instructions")
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/helper.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 03/14] target/arm: Report correct syndrome for UNDEFINED S1E2 AT ops at EL3

[Richard Henderson]

On 1/30/25 10:22, Peter Maydell wrote:
> The pseudocode for AT S1E2R and AT S1E2W says that they should be
> UNDEFINED if executed at EL3 when EL2 is not enabled. We were
> incorrectly using CP_ACCESS_TRAP and reporting the wrong exception
> syndrome as a result. Use CP_ACCESS_TRAP_UNCATEGORIZED.
> 
> Cc: qemu-stable@nongnu.org
> Fixes: 2a47df953202e1 ("target-arm: Wire up AArch64 EL2 and EL3 address translation ops")
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/arm/helper.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 7ddeed0283f..74b556b6766 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -3611,7 +3611,7 @@ static CPAccessResult at_s1e2_access(CPUARMState *env, const ARMCPRegInfo *ri,
>   {
>       if (arm_current_el(env) == 3 &&
>           !(env->cp15.scr_el3 & (SCR_NS | SCR_EEL2))) {
> -        return CP_ACCESS_TRAP;
> +        return CP_ACCESS_TRAP_UNCATEGORIZED;

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

for the categorization fix.

BTW the line before seems like it would be clearer as arm_is_el2_enabled.  I think there 
no bug here because we don't register the cpreg unless EL2 is present, and EEL2 will not 
be set without FEAT_SEL2.  So all the checks have been done, but it's not obvious.


r~

Re: [PATCH 04/14] target/arm: Report correct syndrome for UNDEFINED LOR sysregs when NS=0

[Richard Henderson]

On 1/30/25 10:22, Peter Maydell wrote:
> The pseudocode for the accessors for the LOR sysregs says they
> are UNDEFINED if SCR_EL3.NS is 0. We were reporting the wrong
> syndrome value here; use CP_ACCESS_TRAP_UNCATEGORIZED.
> 
> Cc:qemu-stable@nongnu.org
> Fixes: 2d7137c10faf ("target/arm: Implement the ARMv8.1-LOR extension")
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/helper.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

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

[Richard Henderson]

On 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~

Re: [PATCH 06/14] hw/intc/arm_gicv3_cpuif: Don't downgrade monitor traps for AArch32 EL3

[Richard Henderson]

On 1/30/25 10:23, Peter Maydell wrote:
> In the gicv3_{irq,fiq,irqfiq}_access() functions, there is a check
> which downgrades a CP_ACCESS_TRAP_EL3 to CP_ACCESS_TRAP if EL3 is not
> AArch64.  This has been there since the GIC was first implemented,
> but it isn't right: if we are trapping because of SCR.IRQ or SCR.FIQ
> then we definitely want to be going to EL3 (doing
> AArch32.TakeMonitorTrapException() in pseudocode terms).  We might
> want to not take a trap at all, but we don't ever want to go to the
> default target EL, because that would mean, for instance, taking a
> trap to Hyp mode if the trapped access was made from Hyp mode.
> 
> (This might have been an attempt to work around our failure to
> properly implement Monitor Traps.)
> 
> Remove the bogus check.
> 
> Cc:qemu-stable@nongnu.org
> Fixes: 359fbe65e01e ("hw/intc/arm_gicv3: Implement GICv3 CPU interface registers")
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/intc/arm_gicv3_cpuif.c | 9 ---------
>   1 file changed, 9 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 07/14] target/arm: Honour SDCR.TDCC and SCR.TERR in AArch32 EL3 non-Monitor modes

[Richard Henderson]

On 1/30/25 10:23, Peter Maydell wrote:
> There are not many traps in AArch32 which should trap to Monitor
> mode, but these trap bits should trap not just lower ELs to Monitor
> mode but also the non-Monitor modes running at EL3 (i.e.  Secure
> System, Secure Undef, etc).
> 
> We get this wrong because the relevant access functions implement the
> AArch64-style logic of
>     if (el < 3 && trap_bit_set) {
>         return CP_ACCESS_TRAP_EL3;
>     }
> which won't trap the non-Monitor modes at EL3.
> 
> Correct this error by using arm_is_el3_or_mon() instead, which
> returns true when the CPU is at AArch64 EL3 or AArch32 Monitor mode.
> (Since the new callsites are compiled also for the linux-user mode,
> we need to provide a dummy implementation for CONFIG_USER_ONLY.)
> 
> This affects only:
>   * trapping of ERRIDR via SCR.TERR
>   * trapping of the debug channel registers via SDCR.TDCC
>   * trapping of GICv3 registers via SCR.IRQ and SCR.FIQ
>     (which we already used arm_is_el3_or_mon() for)
> 
> This patch changes the handling of SCR.TERR and SDCR.TDCC. This
> patch only changes guest-visible behaviour for "-cpu max" on
> the qemu-system-arm binary, because SCR.TERR
> and SDCR.TDCC (and indeed the entire SDCR register) only arrived
> in Armv8, and the only guest CPU we support which has any v8
> features and also starts in AArch32 EL3 is the 32-bit 'max'.
> 
> Other uses of CP_ACCESS_TRAP_EL3 don't need changing:
> 
>   * uses in code paths that can't happen when EL3 is AArch32:
>     access_trap_aa32s_el1, cpacr_access, cptr_access, nsacr_access
>   * uses which are in accessfns for AArch64-only registers:
>     gt_stimer_access, gt_cntpoff_access, access_hxen, access_tpidr2,
>     access_smpri, access_smprimap, access_lor_ns, access_pauth,
>     access_mte, access_tfsr_el2, access_scxtnum, access_fgt
>   * trap bits which exist only in the AArch64 version of the
>     trap register, not the AArch32 one:
>     access_tpm, pmreg_access, access_dbgvcr32, access_tdra,
>     access_tda, access_tdosa (TPM, TDA and TDOSA exist only in
>     MDCR_EL3, not in SDCR, and we enforce this in sdcr_write())
> 
> Cc:qemu-stable@nongnu.org
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 08/14] hw/intc/arm_gicv3_cpuif(): Remove redundant tests of is_a64()

[Richard Henderson]

On 1/30/25 10:23, Peter Maydell wrote:
> In the gicv3_{irq,fiq,irqfiq}_access() functions, in the
> arm_current_el(env) == 3 case we do the following test:
>      if (!is_a64(env) && !arm_is_el3_or_mon(env)) {
>          r = CP_ACCESS_TRAP_EL3;
>      }
> 
> In this check, the "!is_a64(env)" is redundant, because if
> we are at EL3 and in AArch64 then arm_is_el3_or_mon() will
> return true and we will skip the if() body anyway.
> 
> Remove the unnecessary tests.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/intc/arm_gicv3_cpuif.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 09/14] target/arm: Support CP_ACCESS_TRAP_EL1 as a CPAccessResult

[Richard Henderson]

On 1/30/25 10:23, Peter Maydell wrote:
> In the CPAccessResult enum, the CP_ACCESS_TRAP* values indicate the
> equivalent of the pseudocode AArch64.SystemAccessTrap(..., 0x18),
> causing a trap to a specified exception level with a syndrome value
> giving information about the failing instructions.  In the
> pseudocode, such traps are always taken to a specified target EL.  We
> support that for target EL of 2 or 3 via CP_ACCESS_TRAP_EL2 and
> CP_ACCESS_TRAP_EL3, but the only way to take the access trap to EL1
> currently is to use CP_ACCESS_TRAP, which takes the trap to the
> "usual target EL" (EL1 if in EL0, otherwise to the current EL).
> 
> Add CP_ACCESS_TRAP_EL1 so that access functions can follow the
> pseudocode more closely.
> 
> (Note that for the common case in the pseudocode of "trap to
> EL2 if HCR_EL2.TGE is set, otherwise trap to EL1", we handle
> this in raise_exception(), so access functions don't need to
> special case it and can use CP_ACCESS_TRAP_EL1.)
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/cpregs.h        | 1 +
>   target/arm/tcg/op_helper.c | 6 ++++--
>   2 files changed, 5 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 10/14] target/arm: Use CP_ACCESS_TRAP_EL1 for traps that are always to EL1

[Richard Henderson]

On 1/30/25 10:23, Peter Maydell wrote:
> We currently use CP_ACCESS_TRAP in a number of access functions where
> we know we're currently at EL0; in this case the "usual target EL"
> is EL1, so CP_ACCESS_TRAP and CP_ACCESS_TRAP_EL1 behave the same.
> Use CP_ACCESS_TRAP_EL1 to more closely match the pseudocode for
> this sort of check.
> 
> Note that in the case of the access functions foc cacheop to
> PoC or PoU, the code was correct but the comment was wrong:
> SCTLR_EL1.UCI traps for DC CVAC, DC CIVAC, DC CVAP, DC CVADP,
> DC CVAU and IC IVAU should be system access traps, not UNDEFs.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/debug_helper.c |  2 +-
>   target/arm/helper.c       | 30 +++++++++++++++---------------
>   2 files changed, 16 insertions(+), 16 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 11/14] target/arm: Use TRAP_UNCATEGORIZED for XScale CPAR traps

[Richard Henderson]

On 1/30/25 10:23, Peter Maydell wrote:
> On XScale CPUs, there is no EL2 or AArch64, so no syndrome register.
> These traps are just UNDEFs in the traditional AArch32 sense, so
> CP_ACCESS_TRAP_UNCATEGORIZED is more accurate than CP_ACCESS_TRAP.
> This has no visible behavioural change, because the guest doesn't
> have a way to see the syndrome value we generate.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/tcg/op_helper.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 12/14] target/arm: Remove CP_ACCESS_TRAP handling

[Richard Henderson]

On 1/30/25 10:23, Peter Maydell wrote:
> There are no longer any uses of CP_ACCESS_TRAP in access functions,
> because we have converted them all to use either CP_ACCESS_TRAP_EL1
> or CP_ACCESS_TRAP_UNCATEGORIZED, as appropriate. Remove the handling
> of bare CP_ACCESS_TRAP from the access_check_cp_reg() helper, so that
> it now asserts if an access function returns it.

Wording from an in-development patch? How can an access function return CP_ACCESS_TRAP 
when it has been removed?

That said, the code is correct.
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 13/14] target/arm: Rename CP_ACCESS_TRAP_UNCATEGORIZED to CP_ACCESS_UNDEFINED

[Richard Henderson]

On 1/30/25 10:23, Peter Maydell wrote:
> CP_ACCESS_TRAP_UNCATEGORIZED is technically an accurate description
> of what this return value from a cpreg accessfn does, but it's liable
> to confusion because it doesn't match how the Arm ARM pseudocode
> indicates this case. What it does is an EXCP_UDEF with a zero
> ("uncategorized") syndrome value, which is what an UNDEFINED instruction
> does. The pseudocode uses "UNDEFINED" to show this; rename our
> constant to CP_ACCESS_UNDEFINED to make the parallel clearer.
> 
> Commit created with
> sed -i -e 's/CP_ACCESS_TRAP_UNCATEGORIZED/CP_ACCESS_UNDEFINED/' $(git grep -l CP_ACCESS_TRAP_UNCATEGORIZED)
> 
> plus manual editing of the comment.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/cpregs.h        |  5 +++--
>   target/arm/helper.c        | 30 +++++++++++++++---------------
>   target/arm/tcg/op_helper.c |  6 +++---
>   3 files changed, 21 insertions(+), 20 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 14/14] target/arm: Correct errors in WFI/WFE trapping

[Richard Henderson]

On 1/30/25 10:23, Peter Maydell wrote:
> The code for WFI/WFE trapping has several errors:
>   * it wasn't using arm_sctlr(), so it would look at SCTLR_EL1
>     even if the CPU was in the EL2&0 translation regime
>   * it was raising UNDEF, not Monitor Trap, for traps to
>     AArch32 EL3 because of SCR.{TWE,TWI}
>   * it was not honouring SCR.{TWE,TWI} when running in
>     AArch32 at EL3 not in Monitor mode
>   * it checked SCR.{TWE,TWI} even on v7 CPUs which don't have
>     those bits
> 
> Fix these bugs.
> 
> Cc:qemu-stable@nongnu.org
> Fixes: b1eced713d99 ("target-arm: Add WFx instruction trap support")
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/tcg/op_helper.c | 37 ++++++++++++++++++-------------------
>   1 file changed, 18 insertions(+), 19 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 12/14] target/arm: Remove CP_ACCESS_TRAP handling

[Peter Maydell]

On Mon, 10 Feb 2025 at 19:35, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 1/30/25 10:23, Peter Maydell wrote:
> > There are no longer any uses of CP_ACCESS_TRAP in access functions,
> > because we have converted them all to use either CP_ACCESS_TRAP_EL1
> > or CP_ACCESS_TRAP_UNCATEGORIZED, as appropriate. Remove the handling
> > of bare CP_ACCESS_TRAP from the access_check_cp_reg() helper, so that
> > it now asserts if an access function returns it.
>
> Wording from an in-development patch? How can an access function return CP_ACCESS_TRAP
> when it has been removed?

I think more that my wording wasn't super clear. It's kind of
a two step thing: we make access_check_cp_reg assert() if you
return that value (4), and we also change the constant name
so it's clearer that you shouldn't be returning that value.

> That said, the code is correct.
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

thanks
-- PMM