diff mbox series

[07/10] target/arm: SCR_EL3.RW should be treated as 1 if EL2 doesn't support AArch32

Message ID 20250306163925.2940297-8-peter.maydell@linaro.org
State New
Headers show
Series [01/10] target/arm: Move A32_BANKED_REG_{GET, SET} macros to cpregs.h | expand

Commit Message

Peter Maydell March 6, 2025, 4:39 p.m. UTC 
The definition of SCR_EL3.RW says that its effective value is 1 if:
 - EL2 is implemented and does not support AArch32, and SCR_EL3.NS is 1
 - the effective value of SCR_EL3.{EEL2,NS} is {1,0} (i.e. we are
   Secure and Secure EL2 is disabled)

We implement the second of these in arm_el_is_aa64(), but forgot the
first (because currently all our CPUs with AArch64 support AArch32 at
all exception levels).

Provide a new function arm_scr_rw_eff() to return the effective
value of SCR_EL3.RW, and use it in arm_el_is_aa64() and the other
places that currently look directly at the bit value.

(scr_write() enforces that the RW bit is RAO/WI if neither EL1 nor
EL2 have AArch32 support, but if EL1 does but EL2 does not then the
bit must still be writeable.)

This will mean that if code at EL3 attempts to perform an exception
return to AArch32 EL2 when EL2 is AArch64-only we will correctly
handle this as an illegal exception return: it will be caught by the
"return to an EL which is configured for a different register width"
check in HELPER(exception_return).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/internals.h | 24 +++++++++++++++++++++---
 target/arm/helper.c    |  4 ++--
 2 files changed, 23 insertions(+), 5 deletions(-)

Comments

Peter Maydell March 6, 2025, 5:12 p.m. UTC | #1 
On Thu, 6 Mar 2025 at 16:39, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> The definition of SCR_EL3.RW says that its effective value is 1 if:
>  - EL2 is implemented and does not support AArch32, and SCR_EL3.NS is 1
>  - the effective value of SCR_EL3.{EEL2,NS} is {1,0} (i.e. we are
>    Secure and Secure EL2 is disabled)
>
> We implement the second of these in arm_el_is_aa64(), but forgot the
> first (because currently all our CPUs with AArch64 support AArch32 at
> all exception levels).
>
> Provide a new function arm_scr_rw_eff() to return the effective
> value of SCR_EL3.RW, and use it in arm_el_is_aa64() and the other
> places that currently look directly at the bit value.
>
> (scr_write() enforces that the RW bit is RAO/WI if neither EL1 nor
> EL2 have AArch32 support, but if EL1 does but EL2 does not then the
> bit must still be writeable.)
>
> This will mean that if code at EL3 attempts to perform an exception
> return to AArch32 EL2 when EL2 is AArch64-only we will correctly
> handle this as an illegal exception return: it will be caught by the
> "return to an EL which is configured for a different register width"
> check in HELPER(exception_return).
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/internals.h | 24 +++++++++++++++++++++---
>  target/arm/helper.c    |  4 ++--
>  2 files changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index b3f732233f4..82a0e1f785f 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -392,6 +392,25 @@ static inline FloatRoundMode arm_rmode_to_sf(ARMFPRounding rmode)
>      return arm_rmode_to_sf_map[rmode];
>  }
>
> +/* Return the effective value of SCR_EL3.RW */
> +static inline bool arm_scr_rw_eff(CPUARMState *env)
> +{
> +    /*
> +     * SCR_EL3.RW has an effective value of 1 if:
> +     *  - we are NS and EL2 is implemented but doesn't support AArch32
> +     *  - we are S and EL2 is enabled (in which case it must be AArch64)
> +     */
> +    ARMCPU *cpu = env_archcpu(env);
> +    bool ns_and_no_aarch32_el2 = arm_feature(env, ARM_FEATURE_EL2) &&
> +        (env->cp15.scr_el3 & SCR_NS) &&
> +        !cpu_isar_feature(aa64_aa32_el1, cpu);

should be "aa64_aa32_el2"...

> +    bool s_and_el2_enabled =
> +        (env->cp15.scr_el3 & (SCR_NS | SCR_EEL2)) == SCR_EEL2;
> +
> +    return ns_and_no_aarch32_el2 || s_and_el2_enabled ||
> +        (env->cp15.scr_el3 & SCR_RW);
> +}
>


-- PMM
Richard Henderson March 6, 2025, 10:53 p.m. UTC | #2 
On 3/6/25 08:39, Peter Maydell wrote:
> +/* Return the effective value of SCR_EL3.RW */
> +static inline bool arm_scr_rw_eff(CPUARMState *env)
> +{
> +    /*
> +     * SCR_EL3.RW has an effective value of 1 if:
> +     *  - we are NS and EL2 is implemented but doesn't support AArch32
> +     *  - we are S and EL2 is enabled (in which case it must be AArch64)
> +     */
> +    ARMCPU *cpu = env_archcpu(env);
> +    bool ns_and_no_aarch32_el2 = arm_feature(env, ARM_FEATURE_EL2) &&
> +        (env->cp15.scr_el3 & SCR_NS) &&
> +        !cpu_isar_feature(aa64_aa32_el1, cpu);
> +    bool s_and_el2_enabled =
> +        (env->cp15.scr_el3 & (SCR_NS | SCR_EEL2)) == SCR_EEL2;
> +
> +    return ns_and_no_aarch32_el2 || s_and_el2_enabled ||
> +        (env->cp15.scr_el3 & SCR_RW);
> +}

The computation here can be simplified.

     if (env->cp15.scr_el3 & SCR_RW) {
         return true;
     }
     if (env->cp15.scr_el3 & SCR_NS) {
         return arm_feature(env, ARM_FEATURE_EL2) &&
                !cpu_isar_feature(aa64_aa32_el1, cpu);
     } else {
         return env->cp15.scr_el3 & SCR_EEL2;
     }

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


r~
diff mbox series

Patch

diff --git a/target/arm/internals.h b/target/arm/internals.h
index b3f732233f4..82a0e1f785f 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -392,6 +392,25 @@  static inline FloatRoundMode arm_rmode_to_sf(ARMFPRounding rmode)
     return arm_rmode_to_sf_map[rmode];
 }
 
+/* Return the effective value of SCR_EL3.RW */
+static inline bool arm_scr_rw_eff(CPUARMState *env)
+{
+    /*
+     * SCR_EL3.RW has an effective value of 1 if:
+     *  - we are NS and EL2 is implemented but doesn't support AArch32
+     *  - we are S and EL2 is enabled (in which case it must be AArch64)
+     */
+    ARMCPU *cpu = env_archcpu(env);
+    bool ns_and_no_aarch32_el2 = arm_feature(env, ARM_FEATURE_EL2) &&
+        (env->cp15.scr_el3 & SCR_NS) &&
+        !cpu_isar_feature(aa64_aa32_el1, cpu);
+    bool s_and_el2_enabled =
+        (env->cp15.scr_el3 & (SCR_NS | SCR_EEL2)) == SCR_EEL2;
+
+    return ns_and_no_aarch32_el2 || s_and_el2_enabled ||
+        (env->cp15.scr_el3 & SCR_RW);
+}
+
 /* Return true if the specified exception level is running in AArch64 state. */
 static inline bool arm_el_is_aa64(CPUARMState *env, int el)
 {
@@ -411,9 +430,8 @@  static inline bool arm_el_is_aa64(CPUARMState *env, int el)
         return aa64;
     }
 
-    if (arm_feature(env, ARM_FEATURE_EL3) &&
-        ((env->cp15.scr_el3 & SCR_NS) || !(env->cp15.scr_el3 & SCR_EEL2))) {
-        aa64 = aa64 && (env->cp15.scr_el3 & SCR_RW);
+    if (arm_feature(env, ARM_FEATURE_EL3)) {
+        aa64 = aa64 && arm_scr_rw_eff(env);
     }
 
     if (el == 2) {
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 71dead7241b..1085bff0ec5 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -9609,7 +9609,7 @@  uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t excp_idx,
     uint64_t hcr_el2;
 
     if (arm_feature(env, ARM_FEATURE_EL3)) {
-        rw = ((env->cp15.scr_el3 & SCR_RW) == SCR_RW);
+        rw = arm_scr_rw_eff(env);
     } else {
         /*
          * Either EL2 is the highest EL (and so the EL2 register width
@@ -10418,7 +10418,7 @@  static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
 
         switch (new_el) {
         case 3:
-            is_aa64 = (env->cp15.scr_el3 & SCR_RW) != 0;
+            is_aa64 = arm_scr_rw_eff(env);
             break;
         case 2:
             hcr = arm_hcr_el2_eff(env);