From ce9a42483c23c32cee233f648101a160b6604b45 Mon Sep 17 00:00:00 2001

From ce9a42483c23c32cee233f648101a160b6604b45 Mon Sep 17 00:00:00 2001

[Peter Maydell]

Newer Arm CPUs need not implement AArch32 at all exception levels
(and Armv9 forbids implementing AArch32 at any EL except EL0).
All our current CPU models implement both AArch32 and AArch64
at every exception levels, so we currently get away with failing
to enforce that the guest isn't trying to do an exception return
to AArch32 when the target EL doesn't support AArch32.

This patchset adds the missing checks:
 * SCR_EL3.RW has an effective value of 1 if EL2 is AArch64-only
 * HCR_EL2.RW is RAO/WI if EL1 is AArch64-only
 * return to AArch32 when no EL supports AArch32 is an illegal
   exception return

To do this it needs to start off with some cleanup. This is
because we need to add a cpu_isar_feature() check to
arm_el_is_aa64(), but we don't want to include cpu-features.h
in cpu.h. arm_el_is_aa64() is really an internal part of the
CPU implementation, so we can move it to internals.h; this
means also moving some other functions in cpu.h that are
defined as inline functions and which call arm_el_is_aa64().
Removing some unused macros from linux-user allows us to
avoid having linux-user include internals.h.

(No doubt there are other functions that could be moved out
of cpu.h, but I stuck to only the ones that I actually needed
to move.)

thanks
-- PMM

Peter Maydell (10):
  target/arm: Move A32_BANKED_REG_{GET,SET} macros to cpregs.h
  target/arm: Un-inline access_secure_reg()
  linux-user/aarch64: Remove unused get/put_user macros
  linux-user/arm: Remove unused get_put_user macros
  target/arm: Move arm_cpu_data_is_big_endian() etc to internals.h
  target/arm: Move arm_current_el() and arm_el_is_aa64() to internals.h
  target/arm: SCR_EL3.RW should be treated as 1 if EL2 doesn't support
    AArch32
  target/arm: HCR_EL2.RW should be RAO/WI if EL1 doesn't support AArch32
  target/arm: Add cpu local variable to exception_return helper
  target/arm: Forbid return to AArch32 when CPU is AArch64-only

 target/arm/cpregs.h           |  28 +++++++
 target/arm/cpu.h              | 153 +---------------------------------
 target/arm/internals.h        | 133 +++++++++++++++++++++++++++++
 hw/intc/arm_gicv3_cpuif.c     |   1 +
 linux-user/aarch64/cpu_loop.c |  48 -----------
 linux-user/arm/cpu_loop.c     |  43 +---------
 target/arm/arch_dump.c        |   1 +
 target/arm/helper.c           |  16 +++-
 target/arm/tcg/helper-a64.c   |  12 ++-
 target/arm/tcg/hflags.c       |   9 ++
 10 files changed, 202 insertions(+), 242 deletions(-)

-- 
2.43.0

[PATCH 01/10] target/arm: Move A32_BANKED_REG_{GET, SET} macros to cpregs.h

[Peter Maydell]

The A32_BANKED_REG_{GET,SET} macros are only used inside target/arm;
move their definitions to cpregs.h. There's no need to have them
defined in all the code that includes cpu.h.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpregs.h | 28 ++++++++++++++++++++++++++++
 target/arm/cpu.h    | 27 ---------------------------
 2 files changed, 28 insertions(+), 27 deletions(-)

diff --git a/target/arm/cpregs.h b/target/arm/cpregs.h
index 52377c6eb50..2183de8eda6 100644
--- a/target/arm/cpregs.h
+++ b/target/arm/cpregs.h
@@ -1157,4 +1157,32 @@ static inline bool arm_cpreg_traps_in_nv(const ARMCPRegInfo *ri)
     return ri->opc1 == 4 || ri->opc1 == 5;
 }
 
+/* Macros for accessing a specified CP register bank */
+#define A32_BANKED_REG_GET(_env, _regname, _secure)                     \
+    ((_secure) ? (_env)->cp15._regname##_s : (_env)->cp15._regname##_ns)
+
+#define A32_BANKED_REG_SET(_env, _regname, _secure, _val)       \
+    do {                                                        \
+        if (_secure) {                                          \
+            (_env)->cp15._regname##_s = (_val);                 \
+        } else {                                                \
+            (_env)->cp15._regname##_ns = (_val);                \
+        }                                                       \
+    } while (0)
+
+/*
+ * Macros for automatically accessing a specific CP register bank depending on
+ * the current secure state of the system.  These macros are not intended for
+ * supporting instruction translation reads/writes as these are dependent
+ * solely on the SCR.NS bit and not the mode.
+ */
+#define A32_BANKED_CURRENT_REG_GET(_env, _regname)                          \
+    A32_BANKED_REG_GET((_env), _regname,                                    \
+                       (arm_is_secure(_env) && !arm_el_is_aa64((_env), 3)))
+
+#define A32_BANKED_CURRENT_REG_SET(_env, _regname, _val)                    \
+    A32_BANKED_REG_SET((_env), _regname,                                    \
+                       (arm_is_secure(_env) && !arm_el_is_aa64((_env), 3)), \
+                       (_val))
+
 #endif /* TARGET_ARM_CPREGS_H */
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 215845c7e25..c360b74ded9 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2682,33 +2682,6 @@ static inline bool access_secure_reg(CPUARMState *env)
     return ret;
 }
 
-/* Macros for accessing a specified CP register bank */
-#define A32_BANKED_REG_GET(_env, _regname, _secure)    \
-    ((_secure) ? (_env)->cp15._regname##_s : (_env)->cp15._regname##_ns)
-
-#define A32_BANKED_REG_SET(_env, _regname, _secure, _val)   \
-    do {                                                \
-        if (_secure) {                                   \
-            (_env)->cp15._regname##_s = (_val);            \
-        } else {                                        \
-            (_env)->cp15._regname##_ns = (_val);           \
-        }                                               \
-    } while (0)
-
-/* Macros for automatically accessing a specific CP register bank depending on
- * the current secure state of the system.  These macros are not intended for
- * supporting instruction translation reads/writes as these are dependent
- * solely on the SCR.NS bit and not the mode.
- */
-#define A32_BANKED_CURRENT_REG_GET(_env, _regname)        \
-    A32_BANKED_REG_GET((_env), _regname,                \
-                       (arm_is_secure(_env) && !arm_el_is_aa64((_env), 3)))
-
-#define A32_BANKED_CURRENT_REG_SET(_env, _regname, _val)                       \
-    A32_BANKED_REG_SET((_env), _regname,                                    \
-                       (arm_is_secure(_env) && !arm_el_is_aa64((_env), 3)), \
-                       (_val))
-
 uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t excp_idx,
                                  uint32_t cur_el, bool secure);
 
-- 
2.43.0

Re: [PATCH 01/10] target/arm: Move A32_BANKED_REG_{GET, SET} macros to cpregs.h

[Richard Henderson]

On 3/6/25 08:39, Peter Maydell wrote:
> The A32_BANKED_REG_{GET,SET} macros are only used inside target/arm;
> move their definitions to cpregs.h. There's no need to have them
> defined in all the code that includes cpu.h.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/cpregs.h | 28 ++++++++++++++++++++++++++++
>   target/arm/cpu.h    | 27 ---------------------------
>   2 files changed, 28 insertions(+), 27 deletions(-)

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

r~

[PATCH 02/10] target/arm: Un-inline access_secure_reg()

[Peter Maydell]

We would like to move arm_el_is_aa64() to internals.h; however, it is
used by access_secure_reg().  Make that function not be inline, so
that it can stay in cpu.h.

access_secure_reg() is used only in two places:
 * in hflags.c
 * in the user-mode arm emulators, to decide whether to store
   the TLS value in the secure or non-secure banked field

The second of these is not on a super-hot path that would care about
the inlining (and incidentally will always use the NS banked field
because our user-mode CPUs never set ARM_FEATURE_EL3); put the
definition of access_secure_reg() in hflags.c, near its only use
inside target/arm.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu.h        | 12 +++---------
 target/arm/tcg/hflags.c |  9 +++++++++
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index c360b74ded9..5ae40f32491 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2666,21 +2666,15 @@ static inline bool arm_el_is_aa64(CPUARMState *env, int el)
     return aa64;
 }
 
-/* Function for determining whether guest cp register reads and writes should
+/*
+ * Function for determining whether guest cp register reads and writes should
  * access the secure or non-secure bank of a cp register.  When EL3 is
  * operating in AArch32 state, the NS-bit determines whether the secure
  * instance of a cp register should be used. When EL3 is AArch64 (or if
  * it doesn't exist at all) then there is no register banking, and all
  * accesses are to the non-secure version.
  */
-static inline bool access_secure_reg(CPUARMState *env)
-{
-    bool ret = (arm_feature(env, ARM_FEATURE_EL3) &&
-                !arm_el_is_aa64(env, 3) &&
-                !(env->cp15.scr_el3 & SCR_NS));
-
-    return ret;
-}
+bool access_secure_reg(CPUARMState *env);
 
 uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t excp_idx,
                                  uint32_t cur_el, bool secure);
diff --git a/target/arm/tcg/hflags.c b/target/arm/tcg/hflags.c
index 9e6a1869f94..8d79b8b7ae1 100644
--- a/target/arm/tcg/hflags.c
+++ b/target/arm/tcg/hflags.c
@@ -63,6 +63,15 @@ static bool aprofile_require_alignment(CPUARMState *env, int el, uint64_t sctlr)
 #endif
 }
 
+bool access_secure_reg(CPUARMState *env)
+{
+    bool ret = (arm_feature(env, ARM_FEATURE_EL3) &&
+                !arm_el_is_aa64(env, 3) &&
+                !(env->cp15.scr_el3 & SCR_NS));
+
+    return ret;
+}
+
 static CPUARMTBFlags rebuild_hflags_common(CPUARMState *env, int fp_el,
                                            ARMMMUIdx mmu_idx,
                                            CPUARMTBFlags flags)
-- 
2.43.0

Re: [PATCH 02/10] target/arm: Un-inline access_secure_reg()

[Richard Henderson]

On 3/6/25 08:39, Peter Maydell wrote:
> We would like to move arm_el_is_aa64() to internals.h; however, it is
> used by access_secure_reg().  Make that function not be inline, so
> that it can stay in cpu.h.
> 
> access_secure_reg() is used only in two places:
>   * in hflags.c
>   * in the user-mode arm emulators, to decide whether to store
>     the TLS value in the secure or non-secure banked field
> 
> The second of these is not on a super-hot path that would care about
> the inlining (and incidentally will always use the NS banked field
> because our user-mode CPUs never set ARM_FEATURE_EL3); put the
> definition of access_secure_reg() in hflags.c, near its only use
> inside target/arm.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/cpu.h        | 12 +++---------
>   target/arm/tcg/hflags.c |  9 +++++++++
>   2 files changed, 12 insertions(+), 9 deletions(-)

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

r~

[PATCH 03/10] linux-user/aarch64: Remove unused get/put_user macros

[Peter Maydell]

At the top of linux-user/aarch64/cpu_loop.c we define a set of
macros for reading and writing data and code words, but we never
use these macros. Delete them.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/aarch64/cpu_loop.c | 48 -----------------------------------
 1 file changed, 48 deletions(-)

diff --git a/linux-user/aarch64/cpu_loop.c b/linux-user/aarch64/cpu_loop.c
index c5d8a483a3f..fea43cefa6b 100644
--- a/linux-user/aarch64/cpu_loop.c
+++ b/linux-user/aarch64/cpu_loop.c
@@ -27,54 +27,6 @@
 #include "target/arm/syndrome.h"
 #include "target/arm/cpu-features.h"
 
-#define get_user_code_u32(x, gaddr, env)                \
-    ({ abi_long __r = get_user_u32((x), (gaddr));       \
-        if (!__r && bswap_code(arm_sctlr_b(env))) {     \
-            (x) = bswap32(x);                           \
-        }                                               \
-        __r;                                            \
-    })
-
-#define get_user_code_u16(x, gaddr, env)                \
-    ({ abi_long __r = get_user_u16((x), (gaddr));       \
-        if (!__r && bswap_code(arm_sctlr_b(env))) {     \
-            (x) = bswap16(x);                           \
-        }                                               \
-        __r;                                            \
-    })
-
-#define get_user_data_u32(x, gaddr, env)                \
-    ({ abi_long __r = get_user_u32((x), (gaddr));       \
-        if (!__r && arm_cpu_bswap_data(env)) {          \
-            (x) = bswap32(x);                           \
-        }                                               \
-        __r;                                            \
-    })
-
-#define get_user_data_u16(x, gaddr, env)                \
-    ({ abi_long __r = get_user_u16((x), (gaddr));       \
-        if (!__r && arm_cpu_bswap_data(env)) {          \
-            (x) = bswap16(x);                           \
-        }                                               \
-        __r;                                            \
-    })
-
-#define put_user_data_u32(x, gaddr, env)                \
-    ({ typeof(x) __x = (x);                             \
-        if (arm_cpu_bswap_data(env)) {                  \
-            __x = bswap32(__x);                         \
-        }                                               \
-        put_user_u32(__x, (gaddr));                     \
-    })
-
-#define put_user_data_u16(x, gaddr, env)                \
-    ({ typeof(x) __x = (x);                             \
-        if (arm_cpu_bswap_data(env)) {                  \
-            __x = bswap16(__x);                         \
-        }                                               \
-        put_user_u16(__x, (gaddr));                     \
-    })
-
 /* AArch64 main loop */
 void cpu_loop(CPUARMState *env)
 {
-- 
2.43.0

Re: [PATCH 03/10] linux-user/aarch64: Remove unused get/put_user macros

[Richard Henderson]

On 3/6/25 08:39, Peter Maydell wrote:
> At the top of linux-user/aarch64/cpu_loop.c we define a set of
> macros for reading and writing data and code words, but we never
> use these macros. Delete them.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   linux-user/aarch64/cpu_loop.c | 48 -----------------------------------
>   1 file changed, 48 deletions(-)

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

r~

[PATCH 04/10] linux-user/arm: Remove unused get_put_user macros

[Peter Maydell]

In linux-user/arm/cpu_loop.c we define a full set of get/put
macros for both code and data (since the endianness handling
is different between the two). However the only one we actually
use is get_user_code_u32(). Remove the rest.

We leave a comment noting how data-side accesses should be handled
for big-endian, because that's a subtle point and we just removed the
macros that were effectively documenting it.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/arm/cpu_loop.c | 43 ++++-----------------------------------
 1 file changed, 4 insertions(+), 39 deletions(-)

diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
index 10d8561f9b9..7416e3216ea 100644
--- a/linux-user/arm/cpu_loop.c
+++ b/linux-user/arm/cpu_loop.c
@@ -36,45 +36,10 @@
         __r;                                            \
     })
 
-#define get_user_code_u16(x, gaddr, env)                \
-    ({ abi_long __r = get_user_u16((x), (gaddr));       \
-        if (!__r && bswap_code(arm_sctlr_b(env))) {     \
-            (x) = bswap16(x);                           \
-        }                                               \
-        __r;                                            \
-    })
-
-#define get_user_data_u32(x, gaddr, env)                \
-    ({ abi_long __r = get_user_u32((x), (gaddr));       \
-        if (!__r && arm_cpu_bswap_data(env)) {          \
-            (x) = bswap32(x);                           \
-        }                                               \
-        __r;                                            \
-    })
-
-#define get_user_data_u16(x, gaddr, env)                \
-    ({ abi_long __r = get_user_u16((x), (gaddr));       \
-        if (!__r && arm_cpu_bswap_data(env)) {          \
-            (x) = bswap16(x);                           \
-        }                                               \
-        __r;                                            \
-    })
-
-#define put_user_data_u32(x, gaddr, env)                \
-    ({ typeof(x) __x = (x);                             \
-        if (arm_cpu_bswap_data(env)) {                  \
-            __x = bswap32(__x);                         \
-        }                                               \
-        put_user_u32(__x, (gaddr));                     \
-    })
-
-#define put_user_data_u16(x, gaddr, env)                \
-    ({ typeof(x) __x = (x);                             \
-        if (arm_cpu_bswap_data(env)) {                  \
-            __x = bswap16(__x);                         \
-        }                                               \
-        put_user_u16(__x, (gaddr));                     \
-    })
+/*
+ * Note that if we need to do data accesses here, they should do a
+ * bswap if arm_cpu_bswap_data() returns true.
+ */
 
 /*
  * Similar to code in accel/tcg/user-exec.c, but outside the execution loop.
-- 
2.43.0

Re: [PATCH 04/10] linux-user/arm: Remove unused get_put_user macros

[Richard Henderson]

On 3/6/25 08:39, Peter Maydell wrote:
> In linux-user/arm/cpu_loop.c we define a full set of get/put
> macros for both code and data (since the endianness handling
> is different between the two). However the only one we actually
> use is get_user_code_u32(). Remove the rest.
> 
> We leave a comment noting how data-side accesses should be handled
> for big-endian, because that's a subtle point and we just removed the
> macros that were effectively documenting it.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   linux-user/arm/cpu_loop.c | 43 ++++-----------------------------------
>   1 file changed, 4 insertions(+), 39 deletions(-)

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

r~

[PATCH 05/10] target/arm: Move arm_cpu_data_is_big_endian() etc to internals.h

[Peter Maydell]

The arm_cpu_data_is_big_endian() and related functions are now used
only in target/arm; they can be moved to internals.h.

The motivation here is that we would like to move arm_current_el()
to internals.h.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu.h       | 48 ------------------------------------------
 target/arm/internals.h | 48 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 48 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 5ae40f32491..16c9083be61 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -3030,47 +3030,6 @@ static inline bool arm_sctlr_b(CPUARMState *env)
 
 uint64_t arm_sctlr(CPUARMState *env, int el);
 
-static inline bool arm_cpu_data_is_big_endian_a32(CPUARMState *env,
-                                                  bool sctlr_b)
-{
-#ifdef CONFIG_USER_ONLY
-    /*
-     * In system mode, BE32 is modelled in line with the
-     * architecture (as word-invariant big-endianness), where loads
-     * and stores are done little endian but from addresses which
-     * are adjusted by XORing with the appropriate constant. So the
-     * endianness to use for the raw data access is not affected by
-     * SCTLR.B.
-     * In user mode, however, we model BE32 as byte-invariant
-     * big-endianness (because user-only code cannot tell the
-     * difference), and so we need to use a data access endianness
-     * that depends on SCTLR.B.
-     */
-    if (sctlr_b) {
-        return true;
-    }
-#endif
-    /* In 32bit endianness is determined by looking at CPSR's E bit */
-    return env->uncached_cpsr & CPSR_E;
-}
-
-static inline bool arm_cpu_data_is_big_endian_a64(int el, uint64_t sctlr)
-{
-    return sctlr & (el ? SCTLR_EE : SCTLR_E0E);
-}
-
-/* Return true if the processor is in big-endian mode. */
-static inline bool arm_cpu_data_is_big_endian(CPUARMState *env)
-{
-    if (!is_a64(env)) {
-        return arm_cpu_data_is_big_endian_a32(env, arm_sctlr_b(env));
-    } else {
-        int cur_el = arm_current_el(env);
-        uint64_t sctlr = arm_sctlr(env, cur_el);
-        return arm_cpu_data_is_big_endian_a64(cur_el, sctlr);
-    }
-}
-
 #include "exec/cpu-all.h"
 
 /*
@@ -3256,13 +3215,6 @@ static inline bool bswap_code(bool sctlr_b)
 #endif
 }
 
-#ifdef CONFIG_USER_ONLY
-static inline bool arm_cpu_bswap_data(CPUARMState *env)
-{
-    return TARGET_BIG_ENDIAN ^ arm_cpu_data_is_big_endian(env);
-}
-#endif
-
 void cpu_get_tb_cpu_state(CPUARMState *env, vaddr *pc,
                           uint64_t *cs_base, uint32_t *flags);
 
diff --git a/target/arm/internals.h b/target/arm/internals.h
index a6ff228f9fd..70d1f88c20b 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -392,6 +392,54 @@ static inline FloatRoundMode arm_rmode_to_sf(ARMFPRounding rmode)
     return arm_rmode_to_sf_map[rmode];
 }
 
+static inline bool arm_cpu_data_is_big_endian_a32(CPUARMState *env,
+                                                  bool sctlr_b)
+{
+#ifdef CONFIG_USER_ONLY
+    /*
+     * In system mode, BE32 is modelled in line with the
+     * architecture (as word-invariant big-endianness), where loads
+     * and stores are done little endian but from addresses which
+     * are adjusted by XORing with the appropriate constant. So the
+     * endianness to use for the raw data access is not affected by
+     * SCTLR.B.
+     * In user mode, however, we model BE32 as byte-invariant
+     * big-endianness (because user-only code cannot tell the
+     * difference), and so we need to use a data access endianness
+     * that depends on SCTLR.B.
+     */
+    if (sctlr_b) {
+        return true;
+    }
+#endif
+    /* In 32bit endianness is determined by looking at CPSR's E bit */
+    return env->uncached_cpsr & CPSR_E;
+}
+
+static inline bool arm_cpu_data_is_big_endian_a64(int el, uint64_t sctlr)
+{
+    return sctlr & (el ? SCTLR_EE : SCTLR_E0E);
+}
+
+/* Return true if the processor is in big-endian mode. */
+static inline bool arm_cpu_data_is_big_endian(CPUARMState *env)
+{
+    if (!is_a64(env)) {
+        return arm_cpu_data_is_big_endian_a32(env, arm_sctlr_b(env));
+    } else {
+        int cur_el = arm_current_el(env);
+        uint64_t sctlr = arm_sctlr(env, cur_el);
+        return arm_cpu_data_is_big_endian_a64(cur_el, sctlr);
+    }
+}
+
+#ifdef CONFIG_USER_ONLY
+static inline bool arm_cpu_bswap_data(CPUARMState *env)
+{
+    return TARGET_BIG_ENDIAN ^ arm_cpu_data_is_big_endian(env);
+}
+#endif
+
 static inline void aarch64_save_sp(CPUARMState *env, int el)
 {
     if (env->pstate & PSTATE_SP) {
-- 
2.43.0

Re: [PATCH 05/10] target/arm: Move arm_cpu_data_is_big_endian() etc to internals.h

[Richard Henderson]

On 3/6/25 08:39, Peter Maydell wrote:
> The arm_cpu_data_is_big_endian() and related functions are now used
> only in target/arm; they can be moved to internals.h.
> 
> The motivation here is that we would like to move arm_current_el()
> to internals.h.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/arm/cpu.h       | 48 ------------------------------------------
>   target/arm/internals.h | 48 ++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 48 insertions(+), 48 deletions(-)

Is there a good reason to keep these inline?
The expansion of arm_cpu_data_is_big_endian is really quite large, it would seem.

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


r~

Re: [PATCH 05/10] target/arm: Move arm_cpu_data_is_big_endian() etc to internals.h

[Peter Maydell]

On Thu, 6 Mar 2025 at 22:13, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 3/6/25 08:39, Peter Maydell wrote:
> > The arm_cpu_data_is_big_endian() and related functions are now used
> > only in target/arm; they can be moved to internals.h.
> >
> > The motivation here is that we would like to move arm_current_el()
> > to internals.h.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >   target/arm/cpu.h       | 48 ------------------------------------------
> >   target/arm/internals.h | 48 ++++++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 48 insertions(+), 48 deletions(-)
>
> Is there a good reason to keep these inline?

Not particularly -- I was just going for the minimal changes
that would let me do what I wanted to do in the later
patches...

-- PMM

[PATCH 06/10] target/arm: Move arm_current_el() and arm_el_is_aa64() to internals.h

[Peter Maydell]

The functions arm_current_el() and arm_el_is_aa64() are used only in
target/arm and in hw/intc/arm_gicv3_cpuif.c.  They're functions that
query internal state of the CPU.  Move them out of cpu.h and into
internals.h.

This means we need to include internals.h in arm_gicv3_cpuif.c, but
this is justifiable because that file is implementing the GICv3 CPU
interface, which really is part of the CPU proper; we just ended up
implementing it in code in hw/intc/ for historical reasons.

The motivation for this move is that we'd like to change
arm_el_is_aa64() to add a condition that uses cpu_isar_feature();
but we don't want to include cpu-features.h in cpu.h.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu.h          | 66 --------------------------------------
 target/arm/internals.h    | 67 +++++++++++++++++++++++++++++++++++++++
 hw/intc/arm_gicv3_cpuif.c |  1 +
 target/arm/arch_dump.c    |  1 +
 4 files changed, 69 insertions(+), 66 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 16c9083be61..a779fd5ae94 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2633,39 +2633,6 @@ uint64_t arm_hcr_el2_eff_secstate(CPUARMState *env, ARMSecuritySpace space);
 uint64_t arm_hcr_el2_eff(CPUARMState *env);
 uint64_t arm_hcrx_el2_eff(CPUARMState *env);
 
-/* Return true if the specified exception level is running in AArch64 state. */
-static inline bool arm_el_is_aa64(CPUARMState *env, int el)
-{
-    /* This isn't valid for EL0 (if we're in EL0, is_a64() is what you want,
-     * and if we're not in EL0 then the state of EL0 isn't well defined.)
-     */
-    assert(el >= 1 && el <= 3);
-    bool aa64 = arm_feature(env, ARM_FEATURE_AARCH64);
-
-    /* The highest exception level is always at the maximum supported
-     * register width, and then lower levels have a register width controlled
-     * by bits in the SCR or HCR registers.
-     */
-    if (el == 3) {
-        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 (el == 2) {
-        return aa64;
-    }
-
-    if (arm_is_el2_enabled(env)) {
-        aa64 = aa64 && (env->cp15.hcr_el2 & HCR_RW);
-    }
-
-    return aa64;
-}
-
 /*
  * Function for determining whether guest cp register reads and writes should
  * access the secure or non-secure bank of a cp register.  When EL3 is
@@ -2697,39 +2664,6 @@ static inline bool arm_v7m_is_handler_mode(CPUARMState *env)
     return env->v7m.exception != 0;
 }
 
-/* Return the current Exception Level (as per ARMv8; note that this differs
- * from the ARMv7 Privilege Level).
- */
-static inline int arm_current_el(CPUARMState *env)
-{
-    if (arm_feature(env, ARM_FEATURE_M)) {
-        return arm_v7m_is_handler_mode(env) ||
-            !(env->v7m.control[env->v7m.secure] & 1);
-    }
-
-    if (is_a64(env)) {
-        return extract32(env->pstate, 2, 2);
-    }
-
-    switch (env->uncached_cpsr & 0x1f) {
-    case ARM_CPU_MODE_USR:
-        return 0;
-    case ARM_CPU_MODE_HYP:
-        return 2;
-    case ARM_CPU_MODE_MON:
-        return 3;
-    default:
-        if (arm_is_secure(env) && !arm_el_is_aa64(env, 3)) {
-            /* If EL3 is 32-bit then all secure privileged modes run in
-             * EL3
-             */
-            return 3;
-        }
-
-        return 1;
-    }
-}
-
 /**
  * write_list_to_cpustate
  * @cpu: ARMCPU
diff --git a/target/arm/internals.h b/target/arm/internals.h
index 70d1f88c20b..b3f732233f4 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -392,6 +392,73 @@ static inline FloatRoundMode arm_rmode_to_sf(ARMFPRounding rmode)
     return arm_rmode_to_sf_map[rmode];
 }
 
+/* Return true if the specified exception level is running in AArch64 state. */
+static inline bool arm_el_is_aa64(CPUARMState *env, int el)
+{
+    /*
+     * This isn't valid for EL0 (if we're in EL0, is_a64() is what you want,
+     * and if we're not in EL0 then the state of EL0 isn't well defined.)
+     */
+    assert(el >= 1 && el <= 3);
+    bool aa64 = arm_feature(env, ARM_FEATURE_AARCH64);
+
+    /*
+     * The highest exception level is always at the maximum supported
+     * register width, and then lower levels have a register width controlled
+     * by bits in the SCR or HCR registers.
+     */
+    if (el == 3) {
+        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 (el == 2) {
+        return aa64;
+    }
+
+    if (arm_is_el2_enabled(env)) {
+        aa64 = aa64 && (env->cp15.hcr_el2 & HCR_RW);
+    }
+
+    return aa64;
+}
+
+/*
+ * Return the current Exception Level (as per ARMv8; note that this differs
+ * from the ARMv7 Privilege Level).
+ */
+static inline int arm_current_el(CPUARMState *env)
+{
+    if (arm_feature(env, ARM_FEATURE_M)) {
+        return arm_v7m_is_handler_mode(env) ||
+            !(env->v7m.control[env->v7m.secure] & 1);
+    }
+
+    if (is_a64(env)) {
+        return extract32(env->pstate, 2, 2);
+    }
+
+    switch (env->uncached_cpsr & 0x1f) {
+    case ARM_CPU_MODE_USR:
+        return 0;
+    case ARM_CPU_MODE_HYP:
+        return 2;
+    case ARM_CPU_MODE_MON:
+        return 3;
+    default:
+        if (arm_is_secure(env) && !arm_el_is_aa64(env, 3)) {
+            /* If EL3 is 32-bit then all secure privileged modes run in EL3 */
+            return 3;
+        }
+
+        return 1;
+    }
+}
+
 static inline bool arm_cpu_data_is_big_endian_a32(CPUARMState *env,
                                                   bool sctlr_b)
 {
diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
index 7f1d071c198..de37465bc87 100644
--- a/hw/intc/arm_gicv3_cpuif.c
+++ b/hw/intc/arm_gicv3_cpuif.c
@@ -22,6 +22,7 @@
 #include "cpu.h"
 #include "target/arm/cpregs.h"
 #include "target/arm/cpu-features.h"
+#include "target/arm/internals.h"
 #include "system/tcg.h"
 #include "system/qtest.h"
 
diff --git a/target/arm/arch_dump.c b/target/arm/arch_dump.c
index 5c943dc27b5..c40df4e7fd7 100644
--- a/target/arm/arch_dump.c
+++ b/target/arm/arch_dump.c
@@ -23,6 +23,7 @@
 #include "elf.h"
 #include "system/dump.h"
 #include "cpu-features.h"
+#include "internals.h"
 
 /* struct user_pt_regs from arch/arm64/include/uapi/asm/ptrace.h */
 struct aarch64_user_regs {
-- 
2.43.0

Re: [PATCH 06/10] target/arm: Move arm_current_el() and arm_el_is_aa64() to internals.h

[Richard Henderson]

On 3/6/25 08:39, Peter Maydell wrote:
> The functions arm_current_el() and arm_el_is_aa64() are used only in
> target/arm and in hw/intc/arm_gicv3_cpuif.c.  They're functions that
> query internal state of the CPU.  Move them out of cpu.h and into
> internals.h.
> 
> This means we need to include internals.h in arm_gicv3_cpuif.c, but
> this is justifiable because that file is implementing the GICv3 CPU
> interface, which really is part of the CPU proper; we just ended up
> implementing it in code in hw/intc/ for historical reasons.
> 
> The motivation for this move is that we'd like to change
> arm_el_is_aa64() to add a condition that uses cpu_isar_feature();
> but we don't want to include cpu-features.h in cpu.h.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/arm/cpu.h          | 66 --------------------------------------
>   target/arm/internals.h    | 67 +++++++++++++++++++++++++++++++++++++++
>   hw/intc/arm_gicv3_cpuif.c |  1 +
>   target/arm/arch_dump.c    |  1 +
>   4 files changed, 69 insertions(+), 66 deletions(-)

Likewise, is there a good reason to keep arm_current_el inline?

I can see that a fair fraction of arm_el_is_aa64 calls have a constant second argument 
(and most of those check el3).  Therefore I can see that keeping that function inline can 
eliminate quite a lot of tests.

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


> +/* Return true if the specified exception level is running in AArch64 state. */
> +static inline bool arm_el_is_aa64(CPUARMState *env, int el)
> +{
> +    /*
> +     * This isn't valid for EL0 (if we're in EL0, is_a64() is what you want,
> +     * and if we're not in EL0 then the state of EL0 isn't well defined.)
> +     */
> +    assert(el >= 1 && el <= 3);
> +    bool aa64 = arm_feature(env, ARM_FEATURE_AARCH64);
> +
> +    /*
> +     * The highest exception level is always at the maximum supported
> +     * register width, and then lower levels have a register width controlled
> +     * by bits in the SCR or HCR registers.
> +     */
> +    if (el == 3) {
> +        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 (el == 2) {
> +        return aa64;
> +    }
> +
> +    if (arm_is_el2_enabled(env)) {
> +        aa64 = aa64 && (env->cp15.hcr_el2 & HCR_RW);
> +    }
> +
> +    return aa64;
> +}

I'll note that this would be clearer with early returns instead of &&.
E.g.

     if (!arm_feature(env, ARM_FEATURE_AARCH64)) {
         return false;
     }
     if (el == 3) {
         return true;
     }

     if (arm_feature(env, ARM_FEATURE_EL3)
         && (env->cp15.scr_el3 & SCR_RW)
         && ((env->cp15.scr_el3 & SCR_NS)
             || !(env->cp15.scr_el3 & SCR_EEL2))) {
         return false;
     }
     if (el == 2) {
         return true;
     }
     ...

But of course not changed with code movement.


r~

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

[Peter Maydell]

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);
+    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);
-- 
2.43.0

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

[Peter Maydell]

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

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

[Richard Henderson]

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~

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

[Peter Maydell]

On Thu, 6 Mar 2025 at 22:54, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> 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;
>      }

Yes, but at the cost of moving it quite a long
way from how the condition is specified in the Arm ARM...

-- PMM

[PATCH 08/10] target/arm: HCR_EL2.RW should be RAO/WI if EL1 doesn't support AArch32

[Peter Maydell]

When EL1 doesn't support AArch32, the HCR_EL2.RW bit is supposed to
be RAO/WI.  We don't enforce this.  This isn't a problem yet because
at the moment all of our CPU types with AArch64 support AArch32 at
all exception levels, but in the future this is likely to no longer
be true. Enforce the RAO/WI behaviour.

Note that we handle "reset value should honour RES1 bits" in the same
way that SCR_EL3 does, via a reset function.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 1085bff0ec5..6dc6f3858fc 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -5167,6 +5167,11 @@ static void do_hcr_write(CPUARMState *env, uint64_t value, uint64_t valid_mask)
     /* Clear RES0 bits.  */
     value &= valid_mask;
 
+    /* RW is RAO/WI if EL1 is AArch64 only */
+    if (!cpu_isar_feature(aa64_aa32_el1, cpu)) {
+        value |= HCR_RW;
+    }
+
     /*
      * These bits change the MMU setup:
      * HCR_VM enables stage 2 translation
@@ -5224,6 +5229,12 @@ static void hcr_writelow(CPUARMState *env, const ARMCPRegInfo *ri,
     do_hcr_write(env, value, MAKE_64BIT_MASK(32, 32));
 }
 
+static void hcr_reset(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    /* hcr_write will set the RES1 bits on an AArch64-only CPU */
+    hcr_write(env, ri, 0);
+}
+
 /*
  * Return the effective value of HCR_EL2, at the given security state.
  * Bits that are not included here:
@@ -5459,6 +5470,7 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
       .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 0,
       .access = PL2_RW, .fieldoffset = offsetof(CPUARMState, cp15.hcr_el2),
       .nv2_redirect_offset = 0x78,
+      .resetfn = hcr_reset,
       .writefn = hcr_write, .raw_writefn = raw_write },
     { .name = "HCR", .state = ARM_CP_STATE_AA32,
       .type = ARM_CP_ALIAS | ARM_CP_IO,
-- 
2.43.0

Re: [PATCH 08/10] target/arm: HCR_EL2.RW should be RAO/WI if EL1 doesn't support AArch32

[Richard Henderson]

On 3/6/25 08:39, Peter Maydell wrote:
> When EL1 doesn't support AArch32, the HCR_EL2.RW bit is supposed to
> be RAO/WI.  We don't enforce this.  This isn't a problem yet because
> at the moment all of our CPU types with AArch64 support AArch32 at
> all exception levels, but in the future this is likely to no longer
> be true. Enforce the RAO/WI behaviour.
> 
> Note that we handle "reset value should honour RES1 bits" in the same
> way that SCR_EL3 does, via a reset function.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/helper.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)

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

r~

[PATCH 09/10] target/arm: Add cpu local variable to exception_return helper

[Peter Maydell]

We already call env_archcpu() multiple times within the
exception_return helper function, and we're about to want to
add another use of the ARMCPU pointer. Add a local variable
cpu so we can call env_archcpu() just once.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/tcg/helper-a64.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/target/arm/tcg/helper-a64.c b/target/arm/tcg/helper-a64.c
index 32f0647ca4f..e2bdf07833d 100644
--- a/target/arm/tcg/helper-a64.c
+++ b/target/arm/tcg/helper-a64.c
@@ -631,6 +631,7 @@ static void cpsr_write_from_spsr_elx(CPUARMState *env,
 
 void HELPER(exception_return)(CPUARMState *env, uint64_t new_pc)
 {
+    ARMCPU *cpu = env_archcpu(env);
     int cur_el = arm_current_el(env);
     unsigned int spsr_idx = aarch64_banked_spsr_index(cur_el);
     uint32_t spsr = env->banked_spsr[spsr_idx];
@@ -682,7 +683,7 @@ void HELPER(exception_return)(CPUARMState *env, uint64_t new_pc)
     }
 
     bql_lock();
-    arm_call_pre_el_change_hook(env_archcpu(env));
+    arm_call_pre_el_change_hook(cpu);
     bql_unlock();
 
     if (!return_to_aa64) {
@@ -710,7 +711,7 @@ void HELPER(exception_return)(CPUARMState *env, uint64_t new_pc)
         int tbii;
 
         env->aarch64 = true;
-        spsr &= aarch64_pstate_valid_mask(&env_archcpu(env)->isar);
+        spsr &= aarch64_pstate_valid_mask(&cpu->isar);
         pstate_write(env, spsr);
         if (!arm_singlestep_active(env)) {
             env->pstate &= ~PSTATE_SS;
@@ -749,7 +750,7 @@ void HELPER(exception_return)(CPUARMState *env, uint64_t new_pc)
     aarch64_sve_change_el(env, cur_el, new_el, return_to_aa64);
 
     bql_lock();
-    arm_call_el_change_hook(env_archcpu(env));
+    arm_call_el_change_hook(cpu);
     bql_unlock();
 
     return;
-- 
2.43.0

Re: [PATCH 09/10] target/arm: Add cpu local variable to exception_return helper

[Richard Henderson]

On 3/6/25 08:39, Peter Maydell wrote:
> We already call env_archcpu() multiple times within the
> exception_return helper function, and we're about to want to
> add another use of the ARMCPU pointer. Add a local variable
> cpu so we can call env_archcpu() just once.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/tcg/helper-a64.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)

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

r~

[PATCH 10/10] target/arm: Forbid return to AArch32 when CPU is AArch64-only

[Peter Maydell]

In the Arm ARM, rule R_TYTWB states that returning to AArch32
is an illegal exception return if:
 * AArch32 is not supported at any exception level
 * the target EL is configured for AArch64 via SCR_EL3.RW
   or HCR_EL2.RW or via CPU state at reset

We check the second of these, but not the first (which can only be
relevant for the case of a return to EL0, because if AArch32 is not
supported at one of the higher ELs then the RW bits will have an
effective value of 1 and the the "configured for AArch64" condition
will hold also).

Add the missing condition. This isn't currently a bug because
all our CPUs support AArch32 at EL0, but future CPUs we add
might be 64-bit only.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/tcg/helper-a64.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/target/arm/tcg/helper-a64.c b/target/arm/tcg/helper-a64.c
index e2bdf07833d..9244848efed 100644
--- a/target/arm/tcg/helper-a64.c
+++ b/target/arm/tcg/helper-a64.c
@@ -678,6 +678,11 @@ void HELPER(exception_return)(CPUARMState *env, uint64_t new_pc)
         goto illegal_return;
     }
 
+    if (!return_to_aa64 && !cpu_isar_feature(aa64_aa32, cpu)) {
+        /* Return to AArch32 when CPU is AArch64-only */
+        goto illegal_return;
+    }
+
     if (new_el == 1 && (arm_hcr_el2_eff(env) & HCR_TGE)) {
         goto illegal_return;
     }
-- 
2.43.0

Re: [PATCH 10/10] target/arm: Forbid return to AArch32 when CPU is AArch64-only

[Richard Henderson]

On 3/6/25 08:39, Peter Maydell wrote:
> In the Arm ARM, rule R_TYTWB states that returning to AArch32
> is an illegal exception return if:
>   * AArch32 is not supported at any exception level
>   * the target EL is configured for AArch64 via SCR_EL3.RW
>     or HCR_EL2.RW or via CPU state at reset
> 
> We check the second of these, but not the first (which can only be
> relevant for the case of a return to EL0, because if AArch32 is not
> supported at one of the higher ELs then the RW bits will have an
> effective value of 1 and the the "configured for AArch64" condition
> will hold also).
> 
> Add the missing condition. This isn't currently a bug because
> all our CPUs support AArch32 at EL0, but future CPUs we add
> might be 64-bit only.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/tcg/helper-a64.c | 5 +++++
>   1 file changed, 5 insertions(+)

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

r~

Re: [PATCH 10/10] target/arm: Forbid return to AArch32 when CPU is AArch64-only

[Peter Maydell]

On Thu, 6 Mar 2025 at 16:39, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> In the Arm ARM, rule R_TYTWB states that returning to AArch32
> is an illegal exception return if:
>  * AArch32 is not supported at any exception level
>  * the target EL is configured for AArch64 via SCR_EL3.RW
>    or HCR_EL2.RW or via CPU state at reset
>
> We check the second of these, but not the first (which can only be
> relevant for the case of a return to EL0, because if AArch32 is not
> supported at one of the higher ELs then the RW bits will have an
> effective value of 1 and the the "configured for AArch64" condition
> will hold also).
>
> Add the missing condition. This isn't currently a bug because
> all our CPUs support AArch32 at EL0, but future CPUs we add
> might be 64-bit only.

I noticed today that actually we do already have a pure AArch64
CPU: a64fx. What saves us is that guests aren't silly enough
to deliberately try to return to nonexistent execution states.
I'll tweak the commit message to suit.

-- PMM

[PATCH 00/10] target/arm: Forbid exception returns to unimplemented AArch32 ELs

[Peter Maydell]

Apologies for the bogus subject line in the cover letter:
I accidentally put a leading ' ' into the file and then
git send-email interpreted that From line as the subject.
Intended subject line was:
[PATCH 00/10] target/arm: Forbid exception returns to unimplemented AArch32 ELs

-- PMM

On Thu, 6 Mar 2025 at 16:39, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Newer Arm CPUs need not implement AArch32 at all exception levels
> (and Armv9 forbids implementing AArch32 at any EL except EL0).
> All our current CPU models implement both AArch32 and AArch64
> at every exception levels, so we currently get away with failing
> to enforce that the guest isn't trying to do an exception return
> to AArch32 when the target EL doesn't support AArch32.
>
> This patchset adds the missing checks:
>  * SCR_EL3.RW has an effective value of 1 if EL2 is AArch64-only
>  * HCR_EL2.RW is RAO/WI if EL1 is AArch64-only
>  * return to AArch32 when no EL supports AArch32 is an illegal
>    exception return
>
> To do this it needs to start off with some cleanup. This is
> because we need to add a cpu_isar_feature() check to
> arm_el_is_aa64(), but we don't want to include cpu-features.h
> in cpu.h. arm_el_is_aa64() is really an internal part of the
> CPU implementation, so we can move it to internals.h; this
> means also moving some other functions in cpu.h that are
> defined as inline functions and which call arm_el_is_aa64().
> Removing some unused macros from linux-user allows us to
> avoid having linux-user include internals.h.
>
> (No doubt there are other functions that could be moved out
> of cpu.h, but I stuck to only the ones that I actually needed
> to move.)
>
> thanks
> -- PMM
>
> Peter Maydell (10):
>   target/arm: Move A32_BANKED_REG_{GET,SET} macros to cpregs.h
>   target/arm: Un-inline access_secure_reg()
>   linux-user/aarch64: Remove unused get/put_user macros
>   linux-user/arm: Remove unused get_put_user macros
>   target/arm: Move arm_cpu_data_is_big_endian() etc to internals.h
>   target/arm: Move arm_current_el() and arm_el_is_aa64() to internals.h
>   target/arm: SCR_EL3.RW should be treated as 1 if EL2 doesn't support
>     AArch32
>   target/arm: HCR_EL2.RW should be RAO/WI if EL1 doesn't support AArch32
>   target/arm: Add cpu local variable to exception_return helper
>   target/arm: Forbid return to AArch32 when CPU is AArch64-only
>
>  target/arm/cpregs.h           |  28 +++++++
>  target/arm/cpu.h              | 153 +---------------------------------
>  target/arm/internals.h        | 133 +++++++++++++++++++++++++++++
>  hw/intc/arm_gicv3_cpuif.c     |   1 +
>  linux-user/aarch64/cpu_loop.c |  48 -----------
>  linux-user/arm/cpu_loop.c     |  43 +---------
>  target/arm/arch_dump.c        |   1 +
>  target/arm/helper.c           |  16 +++-
>  target/arm/tcg/helper-a64.c   |  12 ++-
>  target/arm/tcg/hflags.c       |   9 ++
>  10 files changed, 202 insertions(+), 242 deletions(-)