diff mbox series

[04/13] target/arm: Allow access to SPSR_hyp from hyp mode

Message ID 20240206132931.38376-5-peter.maydell@linaro.org
State Superseded
Headers show
Series hw/arm: Implement new machine mps3-an536 (Cortex-R52 MPS3 AN536 FPGA image) | expand

Commit Message

Peter Maydell Feb. 6, 2024, 1:29 p.m. UTC
Architecturally, the AArch32 MSR/MRS to/from banked register
instructions are UNPREDICTABLE for attempts to access a banked
register that the guest could access in a more direct way (e.g.
using this insn to access r8_fiq when already in FIQ mode).  QEMU has
chosen to UNDEF on all of these.

However, for the case of accessing SPSR_hyp from hyp mode, it turns
out that real hardware permits this, with the same effect as if the
guest had directly written to SPSR. Further, there is some
guest code out there that assumes it can do this, because it
happens to work on hardware: an example Cortex-R52 startup code
fragment uses this, and it got copied into various other places,
including Zephyr. Zephyr was fixed to not use this:
 https://github.com/zephyrproject-rtos/zephyr/issues/47330
but other examples are still out there, like the selftest
binary for the MPS3-AN536.

For convenience of being able to run guest code, permit
this UNPREDICTABLE access instead of UNDEFing it.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Last time this came up I preferred the "keep QEMU behaviour
as it is, try to get the guest code fixed" approach:
 https://www.mail-archive.com/qemu-devel@nongnu.org/msg899970.html
but as this is the second time I lean a bit more towards
behaving like the hardware.
---
 target/arm/tcg/op_helper.c | 43 ++++++++++++++++++++++++++------------
 target/arm/tcg/translate.c | 19 +++++++++++------
 2 files changed, 43 insertions(+), 19 deletions(-)

Comments

Richard Henderson Feb. 6, 2024, 9:46 p.m. UTC | #1
On 2/6/24 23:29, Peter Maydell wrote:
> Architecturally, the AArch32 MSR/MRS to/from banked register
> instructions are UNPREDICTABLE for attempts to access a banked
> register that the guest could access in a more direct way (e.g.
> using this insn to access r8_fiq when already in FIQ mode).  QEMU has
> chosen to UNDEF on all of these.
> 
> However, for the case of accessing SPSR_hyp from hyp mode, it turns
> out that real hardware permits this, with the same effect as if the
> guest had directly written to SPSR. Further, there is some
> guest code out there that assumes it can do this, because it
> happens to work on hardware: an example Cortex-R52 startup code
> fragment uses this, and it got copied into various other places,
> including Zephyr. Zephyr was fixed to not use this:
>   https://github.com/zephyrproject-rtos/zephyr/issues/47330
> but other examples are still out there, like the selftest
> binary for the MPS3-AN536.
> 
> For convenience of being able to run guest code, permit
> this UNPREDICTABLE access instead of UNDEFing it.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> Last time this came up I preferred the "keep QEMU behaviour
> as it is, try to get the guest code fixed" approach:
>   https://www.mail-archive.com/qemu-devel@nongnu.org/msg899970.html
> but as this is the second time I lean a bit more towards
> behaving like the hardware.
> ---
>   target/arm/tcg/op_helper.c | 43 ++++++++++++++++++++++++++------------
>   target/arm/tcg/translate.c | 19 +++++++++++------
>   2 files changed, 43 insertions(+), 19 deletions(-)

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

r~
diff mbox series

Patch

diff --git a/target/arm/tcg/op_helper.c b/target/arm/tcg/op_helper.c
index b5ac26061c7..c199b69fbff 100644
--- a/target/arm/tcg/op_helper.c
+++ b/target/arm/tcg/op_helper.c
@@ -570,10 +570,24 @@  static void msr_mrs_banked_exc_checks(CPUARMState *env, uint32_t tgtmode,
      */
     int curmode = env->uncached_cpsr & CPSR_M;
 
-    if (regno == 17) {
-        /* ELR_Hyp: a special case because access from tgtmode is OK */
-        if (curmode != ARM_CPU_MODE_HYP && curmode != ARM_CPU_MODE_MON) {
-            goto undef;
+    if (tgtmode == ARM_CPU_MODE_HYP) {
+        /*
+         * Handle Hyp target regs first because some are special cases
+         * which don't want the usual "not accessible from tgtmode" check.
+         */
+        switch (regno) {
+        case 16 ... 17: /* ELR_Hyp, SPSR_Hyp */
+            if (curmode != ARM_CPU_MODE_HYP && curmode != ARM_CPU_MODE_MON) {
+                goto undef;
+            }
+            break;
+        case 13:
+            if (curmode != ARM_CPU_MODE_MON) {
+                goto undef;
+            }
+            break;
+        default:
+            g_assert_not_reached();
         }
         return;
     }
@@ -604,13 +618,6 @@  static void msr_mrs_banked_exc_checks(CPUARMState *env, uint32_t tgtmode,
         }
     }
 
-    if (tgtmode == ARM_CPU_MODE_HYP) {
-        /* SPSR_Hyp, r13_hyp: accessible from Monitor mode only */
-        if (curmode != ARM_CPU_MODE_MON) {
-            goto undef;
-        }
-    }
-
     return;
 
 undef:
@@ -625,7 +632,12 @@  void HELPER(msr_banked)(CPUARMState *env, uint32_t value, uint32_t tgtmode,
 
     switch (regno) {
     case 16: /* SPSRs */
-        env->banked_spsr[bank_number(tgtmode)] = value;
+        if (tgtmode == (env->uncached_cpsr & CPSR_M)) {
+            /* Only happens for SPSR_Hyp access in Hyp mode */
+            env->spsr = value;
+        } else {
+            env->banked_spsr[bank_number(tgtmode)] = value;
+        }
         break;
     case 17: /* ELR_Hyp */
         env->elr_el[2] = value;
@@ -659,7 +671,12 @@  uint32_t HELPER(mrs_banked)(CPUARMState *env, uint32_t tgtmode, uint32_t regno)
 
     switch (regno) {
     case 16: /* SPSRs */
-        return env->banked_spsr[bank_number(tgtmode)];
+        if (tgtmode == (env->uncached_cpsr & CPSR_M)) {
+            /* Only happens for SPSR_Hyp access in Hyp mode */
+            return env->spsr;
+        } else {
+            return env->banked_spsr[bank_number(tgtmode)];
+        }
     case 17: /* ELR_Hyp */
         return env->elr_el[2];
     case 13:
diff --git a/target/arm/tcg/translate.c b/target/arm/tcg/translate.c
index 5fa82497238..f947c62c6be 100644
--- a/target/arm/tcg/translate.c
+++ b/target/arm/tcg/translate.c
@@ -2822,13 +2822,20 @@  static bool msr_banked_access_decode(DisasContext *s, int r, int sysm, int rn,
         break;
     case ARM_CPU_MODE_HYP:
         /*
-         * SPSR_hyp and r13_hyp can only be accessed from Monitor mode
-         * (and so we can forbid accesses from EL2 or below). elr_hyp
-         * can be accessed also from Hyp mode, so forbid accesses from
-         * EL0 or EL1.
+         * r13_hyp can only be accessed from Monitor mode, and so we
+         * can forbid accesses from EL2 or below.
+         * elr_hyp can be accessed also from Hyp mode, so forbid
+         * accesses from EL0 or EL1.
+         * SPSR_hyp is supposed to be in the same category as r13_hyp
+         * and UNPREDICTABLE if accessed from anything except Monitor
+         * mode. However there is some real-world code that will do
+         * it because at least some hardware happens to permit the
+         * access. (Notably a standard Cortex-R52 startup code fragment
+         * does this.) So we permit SPSR_hyp from Hyp mode also, to allow
+         * this (incorrect) guest code to run.
          */
-        if (!arm_dc_feature(s, ARM_FEATURE_EL2) || s->current_el < 2 ||
-            (s->current_el < 3 && *regno != 17)) {
+        if (!arm_dc_feature(s, ARM_FEATURE_EL2) || s->current_el < 2
+            || (s->current_el < 3 && *regno != 16 && *regno != 17)) {
             goto undef;
         }
         break;