diff mbox series

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

Message ID 20250306163925.2940297-7-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 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(-)

Comments

Richard Henderson March 6, 2025, 10:40 p.m. UTC | #1 
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~
diff mbox series

Patch

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 {