diff mbox series

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

Message ID 20250130182309.717346-8-peter.maydell@linaro.org
State Superseded
Headers show
Series target/arm: Clean up some corner cases of sysreg traps | expand

Commit Message

Peter Maydell Jan. 30, 2025, 6:23 p.m. UTC 
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(-)

Comments

Alex Bennée Feb. 5, 2025, 2:40 p.m. UTC | #1 
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>
Richard Henderson Feb. 10, 2025, 7:20 p.m. UTC | #2 
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~
diff mbox series

Patch

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;