[PATCH 30/82] target/arm: Convert arm_mmu_idx_to_el from switch to table

Richard Henderson posted 82 patches 4 months, 2 weeks ago
There is a newer version of this series
[PATCH 30/82] target/arm: Convert arm_mmu_idx_to_el from switch to table
Posted by Richard Henderson 4 months, 2 weeks ago
In an effort to keep all ARMMMUIdx data in one place, begin construction
of an info table describing all of the properties of the mmu_idx.  Begin
with the access EL.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/internals.h       |  3 +--
 target/arm/mmuidx-internal.h | 27 ++++++++++++++++++++++++
 target/arm/helper.c          | 27 ------------------------
 target/arm/mmuidx.c          | 41 ++++++++++++++++++++++++++++++++++++
 target/arm/meson.build       |  7 +++++-
 5 files changed, 75 insertions(+), 30 deletions(-)
 create mode 100644 target/arm/mmuidx-internal.h
 create mode 100644 target/arm/mmuidx.c

diff --git a/target/arm/internals.h b/target/arm/internals.h
index b6499683cc..2dc82330ec 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -34,6 +34,7 @@
 #include "system/memory.h"
 #include "syndrome.h"
 #include "cpu-features.h"
+#include "mmuidx-internal.h"
 
 /* register banks for CPU modes */
 #define BANK_USRSYS 0
@@ -986,8 +987,6 @@ static inline ARMMMUIdx core_to_aa64_mmu_idx(int mmu_idx)
     return mmu_idx | ARM_MMU_IDX_A;
 }
 
-int arm_mmu_idx_to_el(ARMMMUIdx mmu_idx);
-
 /* Return the MMU index for a v7M CPU in the specified security state */
 ARMMMUIdx arm_v7m_mmu_idx_for_secstate(CPUARMState *env, bool secstate);
 
diff --git a/target/arm/mmuidx-internal.h b/target/arm/mmuidx-internal.h
new file mode 100644
index 0000000000..35d41495cd
--- /dev/null
+++ b/target/arm/mmuidx-internal.h
@@ -0,0 +1,27 @@
+/*
+ * QEMU Arm software mmu index internal definitions
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef TARGET_ARM_MMUIDX_INTERNAL_H
+#define TARGET_ARM_MMUIDX_INTERNAL_H
+
+#include "mmuidx.h"
+#include "tcg/debug-assert.h"
+#include "hw/registerfields.h"
+
+
+FIELD(MMUIDXINFO, EL, 0, 2)
+FIELD(MMUIDXINFO, ELVALID, 2, 1)
+
+extern const uint32_t arm_mmuidx_table[ARM_MMU_IDX_M + 8];
+
+/* Return the exception level associated with this mmu index. */
+static inline int arm_mmu_idx_to_el(ARMMMUIdx idx)
+{
+    tcg_debug_assert((unsigned)idx < ARRAY_SIZE(arm_mmuidx_table));
+    tcg_debug_assert(FIELD_EX32(arm_mmuidx_table[idx], MMUIDXINFO, ELVALID));
+    return FIELD_EX32(arm_mmuidx_table[idx], MMUIDXINFO, EL);
+}
+
+#endif /* TARGET_ARM_MMUIDX_INTERNAL_H */
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 154bda3cd4..a97838a04e 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -9895,33 +9895,6 @@ int fp_exception_el(CPUARMState *env, int cur_el)
     return 0;
 }
 
-/* Return the exception level we're running at if this is our mmu_idx */
-int arm_mmu_idx_to_el(ARMMMUIdx mmu_idx)
-{
-    if (mmu_idx & ARM_MMU_IDX_M) {
-        return mmu_idx & ARM_MMU_IDX_M_PRIV;
-    }
-
-    switch (mmu_idx) {
-    case ARMMMUIdx_E10_0:
-    case ARMMMUIdx_E20_0:
-    case ARMMMUIdx_E30_0:
-        return 0;
-    case ARMMMUIdx_E10_1:
-    case ARMMMUIdx_E10_1_PAN:
-        return 1;
-    case ARMMMUIdx_E2:
-    case ARMMMUIdx_E20_2:
-    case ARMMMUIdx_E20_2_PAN:
-        return 2;
-    case ARMMMUIdx_E3:
-    case ARMMMUIdx_E30_3_PAN:
-        return 3;
-    default:
-        g_assert_not_reached();
-    }
-}
-
 #ifndef CONFIG_TCG
 ARMMMUIdx arm_v7m_mmu_idx_for_secstate(CPUARMState *env, bool secstate)
 {
diff --git a/target/arm/mmuidx.c b/target/arm/mmuidx.c
new file mode 100644
index 0000000000..309b1d68df
--- /dev/null
+++ b/target/arm/mmuidx.c
@@ -0,0 +1,41 @@
+/*
+ * QEMU Arm software mmu index definitions
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "mmuidx-internal.h"
+
+
+#define EL(X)  ((X << R_MMUIDXINFO_EL_SHIFT) | R_MMUIDXINFO_ELVALID_MASK)
+
+const uint32_t arm_mmuidx_table[ARM_MMU_IDX_M + 8] = {
+    /*
+     * A-profile.
+     */
+    [ARMMMUIdx_E10_0]           = EL(0),
+    [ARMMMUIdx_E10_1]           = EL(1),
+    [ARMMMUIdx_E10_1_PAN]       = EL(1),
+
+    [ARMMMUIdx_E20_0]           = EL(0),
+    [ARMMMUIdx_E20_2]           = EL(2),
+    [ARMMMUIdx_E20_2_PAN]       = EL(2),
+
+    [ARMMMUIdx_E2]              = EL(2),
+
+    [ARMMMUIdx_E3]              = EL(3),
+    [ARMMMUIdx_E30_0]           = EL(0),
+    [ARMMMUIdx_E30_3_PAN]       = EL(3),
+
+    /*
+     * M-profile.
+     */
+    [ARMMMUIdx_MUser]           = EL(0),
+    [ARMMMUIdx_MPriv]           = EL(1),
+    [ARMMMUIdx_MUserNegPri]     = EL(0),
+    [ARMMMUIdx_MPrivNegPri]     = EL(1),
+    [ARMMMUIdx_MSUser]          = EL(0),
+    [ARMMMUIdx_MSPriv]          = EL(1),
+    [ARMMMUIdx_MSUserNegPri]    = EL(0),
+    [ARMMMUIdx_MSPrivNegPri]    = EL(1),
+};
diff --git a/target/arm/meson.build b/target/arm/meson.build
index 07d9271aa4..91630a1f72 100644
--- a/target/arm/meson.build
+++ b/target/arm/meson.build
@@ -6,7 +6,12 @@ arm_ss.add(files(
 
 arm_ss.add(when: 'TARGET_AARCH64', if_true: files(
   'cpu64.c',
-  'gdbstub64.c'))
+  'gdbstub64.c'
+))
+
+arm_common_ss.add(files(
+  'mmuidx.c',
+))
 
 arm_system_ss = ss.source_set()
 arm_common_system_ss = ss.source_set()
-- 
2.43.0
Re: [PATCH 30/82] target/arm: Convert arm_mmu_idx_to_el from switch to table
Posted by Pierrick Bouvier 4 months, 2 weeks ago
On 7/27/25 1:02 AM, Richard Henderson wrote:
> In an effort to keep all ARMMMUIdx data in one place, begin construction
> of an info table describing all of the properties of the mmu_idx.  Begin
> with the access EL.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/arm/internals.h       |  3 +--
>   target/arm/mmuidx-internal.h | 27 ++++++++++++++++++++++++
>   target/arm/helper.c          | 27 ------------------------
>   target/arm/mmuidx.c          | 41 ++++++++++++++++++++++++++++++++++++
>   target/arm/meson.build       |  7 +++++-
>   5 files changed, 75 insertions(+), 30 deletions(-)
>   create mode 100644 target/arm/mmuidx-internal.h
>   create mode 100644 target/arm/mmuidx.c

What's the benefit to explicitely size arm_mmuidx_table on declaration 
and definition?

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Re: [PATCH 30/82] target/arm: Convert arm_mmu_idx_to_el from switch to table
Posted by Pierrick Bouvier 4 months, 2 weeks ago
On 7/30/25 2:10 PM, Pierrick Bouvier wrote:
> On 7/27/25 1:02 AM, Richard Henderson wrote:
>> In an effort to keep all ARMMMUIdx data in one place, begin construction
>> of an info table describing all of the properties of the mmu_idx.  Begin
>> with the access EL.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>    target/arm/internals.h       |  3 +--
>>    target/arm/mmuidx-internal.h | 27 ++++++++++++++++++++++++
>>    target/arm/helper.c          | 27 ------------------------
>>    target/arm/mmuidx.c          | 41 ++++++++++++++++++++++++++++++++++++
>>    target/arm/meson.build       |  7 +++++-
>>    5 files changed, 75 insertions(+), 30 deletions(-)
>>    create mode 100644 target/arm/mmuidx-internal.h
>>    create mode 100644 target/arm/mmuidx.c
> 
> What's the benefit to explicitely size arm_mmuidx_table on declaration
> and definition?
>

I missed the:
tcg_debug_assert((unsigned)idx < ARRAY_SIZE(arm_mmuidx_table)), which 
does not see definition of the table.

Maybe it would be easier to declare size in the header, and reuse it on 
both sides (declaration, definition instead of ARM_MMU_IDX_M + 8).

> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>
Re: [PATCH 30/82] target/arm: Convert arm_mmu_idx_to_el from switch to table
Posted by Pierrick Bouvier 4 months, 2 weeks ago
On 7/30/25 2:12 PM, Pierrick Bouvier wrote:
> On 7/30/25 2:10 PM, Pierrick Bouvier wrote:
>> On 7/27/25 1:02 AM, Richard Henderson wrote:
>>> In an effort to keep all ARMMMUIdx data in one place, begin construction
>>> of an info table describing all of the properties of the mmu_idx.  Begin
>>> with the access EL.
>>>
>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>> ---
>>>     target/arm/internals.h       |  3 +--
>>>     target/arm/mmuidx-internal.h | 27 ++++++++++++++++++++++++
>>>     target/arm/helper.c          | 27 ------------------------
>>>     target/arm/mmuidx.c          | 41 ++++++++++++++++++++++++++++++++++++
>>>     target/arm/meson.build       |  7 +++++-
>>>     5 files changed, 75 insertions(+), 30 deletions(-)
>>>     create mode 100644 target/arm/mmuidx-internal.h
>>>     create mode 100644 target/arm/mmuidx.c
>>
>> What's the benefit to explicitely size arm_mmuidx_table on declaration
>> and definition?
>>
> 
> I missed the:
> tcg_debug_assert((unsigned)idx < ARRAY_SIZE(arm_mmuidx_table)), which
> does not see definition of the table.
> 
> Maybe it would be easier to declare size in the header, and reuse it on
> both sides (declaration, definition instead of ARM_MMU_IDX_M + 8).
>

Finally, the ARM_MMU_IDX_M + 8 is not a problem.
That said, given how much we repeat
(unsigned)idx < ARRAY_SIZE(arm_mmuidx_table),
it's definitely worth having a 'arm_mmuidx_is_valid' static inline 
function for that.

>> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>
>