[PATCH 37/82] target/arm: Convert regime_is_user from switch to table

Richard Henderson posted 82 patches 4 months, 2 weeks ago
There is a newer version of this series
[PATCH 37/82] target/arm: Convert regime_is_user from switch to table
Posted by Richard Henderson 4 months, 2 weeks ago
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/internals.h       | 17 -----------------
 target/arm/mmuidx-internal.h | 12 ++++++++++++
 target/arm/mmuidx.c          |  6 ++++--
 3 files changed, 16 insertions(+), 19 deletions(-)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index ea210c7179..c6f3ae470b 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1034,23 +1034,6 @@ static inline bool regime_is_stage2(ARMMMUIdx mmu_idx)
     return mmu_idx == ARMMMUIdx_Stage2 || mmu_idx == ARMMMUIdx_Stage2_S;
 }
 
-static inline bool regime_is_user(ARMMMUIdx mmu_idx)
-{
-    switch (mmu_idx) {
-    case ARMMMUIdx_E10_0:
-    case ARMMMUIdx_E20_0:
-    case ARMMMUIdx_E30_0:
-    case ARMMMUIdx_Stage1_E0:
-    case ARMMMUIdx_MUser:
-    case ARMMMUIdx_MSUser:
-    case ARMMMUIdx_MUserNegPri:
-    case ARMMMUIdx_MSUserNegPri:
-        return true;
-    default:
-        return false;
-    }
-}
-
 /* Return the SCTLR value which controls this address translation regime */
 static inline uint64_t regime_sctlr(CPUARMState *env, ARMMMUIdx mmu_idx)
 {
diff --git a/target/arm/mmuidx-internal.h b/target/arm/mmuidx-internal.h
index 5a7259a557..ef1f94a23f 100644
--- a/target/arm/mmuidx-internal.h
+++ b/target/arm/mmuidx-internal.h
@@ -17,6 +17,7 @@ FIELD(MMUIDXINFO, REL, 3, 2)
 FIELD(MMUIDXINFO, RELVALID, 5, 1)
 FIELD(MMUIDXINFO, 2RANGES, 6, 1)
 FIELD(MMUIDXINFO, PAN, 7, 1)
+FIELD(MMUIDXINFO, USER, 8, 1)
 
 extern const uint32_t arm_mmuidx_table[ARM_MMU_IDX_M + 8];
 
@@ -62,4 +63,15 @@ static inline bool regime_is_pan(ARMMMUIdx idx)
     return FIELD_EX32(arm_mmuidx_table[idx], MMUIDXINFO, PAN);
 }
 
+/*
+ * Return true if the exception level associated with this mmu index is 0.
+ * Differs from arm_mmu_idx_to_el(idx) == 0 in that this allows querying
+ * Stage1 and Stage2 mmu indexes.
+ */
+static inline bool regime_is_user(ARMMMUIdx idx)
+{
+    tcg_debug_assert((unsigned)idx < ARRAY_SIZE(arm_mmuidx_table));
+    return FIELD_EX32(arm_mmuidx_table[idx], MMUIDXINFO, USER);
+}
+
 #endif /* TARGET_ARM_MMUIDX_INTERNAL_H */
diff --git a/target/arm/mmuidx.c b/target/arm/mmuidx.c
index 98db02b8e5..1c1e062bfe 100644
--- a/target/arm/mmuidx.c
+++ b/target/arm/mmuidx.c
@@ -7,10 +7,12 @@
 #include "mmuidx-internal.h"
 
 
-#define EL(X)  ((X << R_MMUIDXINFO_EL_SHIFT) | R_MMUIDXINFO_ELVALID_MASK)
+#define EL(X)  ((X << R_MMUIDXINFO_EL_SHIFT) | R_MMUIDXINFO_ELVALID_MASK | \
+                ((X == 0) << R_MMUIDXINFO_USER_SHIFT))
 #define REL(X) ((X << R_MMUIDXINFO_REL_SHIFT) | R_MMUIDXINFO_RELVALID_MASK)
 #define R2     R_MMUIDXINFO_2RANGES_MASK
 #define PAN    R_MMUIDXINFO_PAN_MASK
+#define USER   R_MMUIDXINFO_USER_MASK
 
 const uint32_t arm_mmuidx_table[ARM_MMU_IDX_M + 8] = {
     /*
@@ -33,7 +35,7 @@ const uint32_t arm_mmuidx_table[ARM_MMU_IDX_M + 8] = {
     [ARMMMUIdx_Stage2_S]        = REL(2),
     [ARMMMUIdx_Stage2]          = REL(2),
 
-    [ARMMMUIdx_Stage1_E0]       = REL(1) | R2,
+    [ARMMMUIdx_Stage1_E0]       = REL(1) | R2 | USER,
     [ARMMMUIdx_Stage1_E1]       = REL(1) | R2,
     [ARMMMUIdx_Stage1_E1_PAN]   = REL(1) | R2 | PAN,
 
-- 
2.43.0
Re: [PATCH 37/82] target/arm: Convert regime_is_user from switch to table
Posted by Pierrick Bouvier 4 months, 2 weeks ago
On 7/27/25 1:02 AM, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/arm/internals.h       | 17 -----------------
>   target/arm/mmuidx-internal.h | 12 ++++++++++++
>   target/arm/mmuidx.c          |  6 ++++--
>   3 files changed, 16 insertions(+), 19 deletions(-)
> 
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index ea210c7179..c6f3ae470b 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -1034,23 +1034,6 @@ static inline bool regime_is_stage2(ARMMMUIdx mmu_idx)
>       return mmu_idx == ARMMMUIdx_Stage2 || mmu_idx == ARMMMUIdx_Stage2_S;
>   }
>   
> -static inline bool regime_is_user(ARMMMUIdx mmu_idx)
> -{
> -    switch (mmu_idx) {
> -    case ARMMMUIdx_E10_0:
> -    case ARMMMUIdx_E20_0:
> -    case ARMMMUIdx_E30_0:
> -    case ARMMMUIdx_Stage1_E0:
> -    case ARMMMUIdx_MUser:
> -    case ARMMMUIdx_MSUser:
> -    case ARMMMUIdx_MUserNegPri:
> -    case ARMMMUIdx_MSUserNegPri:
> -        return true;
> -    default:
> -        return false;
> -    }
> -}
> -
>   /* Return the SCTLR value which controls this address translation regime */
>   static inline uint64_t regime_sctlr(CPUARMState *env, ARMMMUIdx mmu_idx)
>   {
> diff --git a/target/arm/mmuidx-internal.h b/target/arm/mmuidx-internal.h
> index 5a7259a557..ef1f94a23f 100644
> --- a/target/arm/mmuidx-internal.h
> +++ b/target/arm/mmuidx-internal.h
> @@ -17,6 +17,7 @@ FIELD(MMUIDXINFO, REL, 3, 2)
>   FIELD(MMUIDXINFO, RELVALID, 5, 1)
>   FIELD(MMUIDXINFO, 2RANGES, 6, 1)
>   FIELD(MMUIDXINFO, PAN, 7, 1)
> +FIELD(MMUIDXINFO, USER, 8, 1)
>   
>   extern const uint32_t arm_mmuidx_table[ARM_MMU_IDX_M + 8];
>   
> @@ -62,4 +63,15 @@ static inline bool regime_is_pan(ARMMMUIdx idx)
>       return FIELD_EX32(arm_mmuidx_table[idx], MMUIDXINFO, PAN);
>   }
>   
> +/*
> + * Return true if the exception level associated with this mmu index is 0.
> + * Differs from arm_mmu_idx_to_el(idx) == 0 in that this allows querying
> + * Stage1 and Stage2 mmu indexes.
> + */
> +static inline bool regime_is_user(ARMMMUIdx idx)
> +{
> +    tcg_debug_assert((unsigned)idx < ARRAY_SIZE(arm_mmuidx_table));
> +    return FIELD_EX32(arm_mmuidx_table[idx], MMUIDXINFO, USER);
> +}
> +
>   #endif /* TARGET_ARM_MMUIDX_INTERNAL_H */
> diff --git a/target/arm/mmuidx.c b/target/arm/mmuidx.c
> index 98db02b8e5..1c1e062bfe 100644
> --- a/target/arm/mmuidx.c
> +++ b/target/arm/mmuidx.c
> @@ -7,10 +7,12 @@
>   #include "mmuidx-internal.h"
>   
>   
> -#define EL(X)  ((X << R_MMUIDXINFO_EL_SHIFT) | R_MMUIDXINFO_ELVALID_MASK)
> +#define EL(X)  ((X << R_MMUIDXINFO_EL_SHIFT) | R_MMUIDXINFO_ELVALID_MASK | \
> +                ((X == 0) << R_MMUIDXINFO_USER_SHIFT))
>   #define REL(X) ((X << R_MMUIDXINFO_REL_SHIFT) | R_MMUIDXINFO_RELVALID_MASK)
>   #define R2     R_MMUIDXINFO_2RANGES_MASK
>   #define PAN    R_MMUIDXINFO_PAN_MASK
> +#define USER   R_MMUIDXINFO_USER_MASK
>   
>   const uint32_t arm_mmuidx_table[ARM_MMU_IDX_M + 8] = {
>       /*
> @@ -33,7 +35,7 @@ const uint32_t arm_mmuidx_table[ARM_MMU_IDX_M + 8] = {
>       [ARMMMUIdx_Stage2_S]        = REL(2),
>       [ARMMMUIdx_Stage2]          = REL(2),
>   
> -    [ARMMMUIdx_Stage1_E0]       = REL(1) | R2,
> +    [ARMMMUIdx_Stage1_E0]       = REL(1) | R2 | USER,
>       [ARMMMUIdx_Stage1_E1]       = REL(1) | R2,
>       [ARMMMUIdx_Stage1_E1_PAN]   = REL(1) | R2 | PAN,
>   

Maybe I missed something, but what about other entries that were 
initially treated in the switch?
- ARMMMUIdx_E.0_0
- ARMMMUIdx_M*User
Re: [PATCH 37/82] target/arm: Convert regime_is_user from switch to table
Posted by Richard Henderson 4 months, 2 weeks ago
On 7/31/25 07:21, Pierrick Bouvier wrote:
>>   #include "mmuidx-internal.h"
>> -#define EL(X)  ((X << R_MMUIDXINFO_EL_SHIFT) | R_MMUIDXINFO_ELVALID_MASK)
>> +#define EL(X)  ((X << R_MMUIDXINFO_EL_SHIFT) | R_MMUIDXINFO_ELVALID_MASK | \
>> +                ((X == 0) << R_MMUIDXINFO_USER_SHIFT))
>>   #define REL(X) ((X << R_MMUIDXINFO_REL_SHIFT) | R_MMUIDXINFO_RELVALID_MASK)
>>   #define R2     R_MMUIDXINFO_2RANGES_MASK
>>   #define PAN    R_MMUIDXINFO_PAN_MASK
>> +#define USER   R_MMUIDXINFO_USER_MASK
>>   const uint32_t arm_mmuidx_table[ARM_MMU_IDX_M + 8] = {
>>       /*
>> @@ -33,7 +35,7 @@ const uint32_t arm_mmuidx_table[ARM_MMU_IDX_M + 8] = {
>>       [ARMMMUIdx_Stage2_S]        = REL(2),
>>       [ARMMMUIdx_Stage2]          = REL(2),
>> -    [ARMMMUIdx_Stage1_E0]       = REL(1) | R2,
>> +    [ARMMMUIdx_Stage1_E0]       = REL(1) | R2 | USER,
>>       [ARMMMUIdx_Stage1_E1]       = REL(1) | R2,
>>       [ARMMMUIdx_Stage1_E1_PAN]   = REL(1) | R2 | PAN,
> 
> Maybe I missed something, but what about other entries that were initially treated in the 
> switch?
> - ARMMMUIdx_E.0_0
> - ARMMMUIdx_M*User

See the change to EL().

I'm not sure why ARMMMUIdx_Stage1_* is excluded from arm_mmu_idx_to_el(), but I don't 
change that in this patch series.


r~

Re: [PATCH 37/82] target/arm: Convert regime_is_user from switch to table
Posted by Pierrick Bouvier 4 months, 2 weeks ago
On 7/31/25 8:53 PM, Richard Henderson wrote:
> On 7/31/25 07:21, Pierrick Bouvier wrote:
>>>    #include "mmuidx-internal.h"
>>> -#define EL(X)  ((X << R_MMUIDXINFO_EL_SHIFT) | R_MMUIDXINFO_ELVALID_MASK)
>>> +#define EL(X)  ((X << R_MMUIDXINFO_EL_SHIFT) | R_MMUIDXINFO_ELVALID_MASK | \
>>> +                ((X == 0) << R_MMUIDXINFO_USER_SHIFT))
>>>    #define REL(X) ((X << R_MMUIDXINFO_REL_SHIFT) | R_MMUIDXINFO_RELVALID_MASK)
>>>    #define R2     R_MMUIDXINFO_2RANGES_MASK
>>>    #define PAN    R_MMUIDXINFO_PAN_MASK
>>> +#define USER   R_MMUIDXINFO_USER_MASK
>>>    const uint32_t arm_mmuidx_table[ARM_MMU_IDX_M + 8] = {
>>>        /*
>>> @@ -33,7 +35,7 @@ const uint32_t arm_mmuidx_table[ARM_MMU_IDX_M + 8] = {
>>>        [ARMMMUIdx_Stage2_S]        = REL(2),
>>>        [ARMMMUIdx_Stage2]          = REL(2),
>>> -    [ARMMMUIdx_Stage1_E0]       = REL(1) | R2,
>>> +    [ARMMMUIdx_Stage1_E0]       = REL(1) | R2 | USER,
>>>        [ARMMMUIdx_Stage1_E1]       = REL(1) | R2,
>>>        [ARMMMUIdx_Stage1_E1_PAN]   = REL(1) | R2 | PAN,
>>
>> Maybe I missed something, but what about other entries that were initially treated in the
>> switch?
>> - ARMMMUIdx_E.0_0
>> - ARMMMUIdx_M*User
> 
> See the change to EL().
> 

Ok.

> I'm not sure why ARMMMUIdx_Stage1_* is excluded from arm_mmu_idx_to_el(), but I don't
> change that in this patch series.
> 

Maybe it could be more explicit to either tag all concerned entries with 
USER, than rely on EL(0) adding the magic, since this fails to apply for 
ARMMMUIdx_Stage1_E0. It's just a suggestion for readability though.

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

> 
> r~


Re: [PATCH 37/82] target/arm: Convert regime_is_user from switch to table
Posted by Peter Maydell 4 months, 2 weeks ago
On Fri, 1 Aug 2025 at 04:54, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 7/31/25 07:21, Pierrick Bouvier wrote:
> >>   #include "mmuidx-internal.h"
> >> -#define EL(X)  ((X << R_MMUIDXINFO_EL_SHIFT) | R_MMUIDXINFO_ELVALID_MASK)
> >> +#define EL(X)  ((X << R_MMUIDXINFO_EL_SHIFT) | R_MMUIDXINFO_ELVALID_MASK | \
> >> +                ((X == 0) << R_MMUIDXINFO_USER_SHIFT))
> >>   #define REL(X) ((X << R_MMUIDXINFO_REL_SHIFT) | R_MMUIDXINFO_RELVALID_MASK)
> >>   #define R2     R_MMUIDXINFO_2RANGES_MASK
> >>   #define PAN    R_MMUIDXINFO_PAN_MASK
> >> +#define USER   R_MMUIDXINFO_USER_MASK
> >>   const uint32_t arm_mmuidx_table[ARM_MMU_IDX_M + 8] = {
> >>       /*
> >> @@ -33,7 +35,7 @@ const uint32_t arm_mmuidx_table[ARM_MMU_IDX_M + 8] = {
> >>       [ARMMMUIdx_Stage2_S]        = REL(2),
> >>       [ARMMMUIdx_Stage2]          = REL(2),
> >> -    [ARMMMUIdx_Stage1_E0]       = REL(1) | R2,
> >> +    [ARMMMUIdx_Stage1_E0]       = REL(1) | R2 | USER,
> >>       [ARMMMUIdx_Stage1_E1]       = REL(1) | R2,
> >>       [ARMMMUIdx_Stage1_E1_PAN]   = REL(1) | R2 | PAN,
> >
> > Maybe I missed something, but what about other entries that were initially treated in the
> > switch?
> > - ARMMMUIdx_E.0_0
> > - ARMMMUIdx_M*User
>
> See the change to EL().
>
> I'm not sure why ARMMMUIdx_Stage1_* is excluded from arm_mmu_idx_to_el(), but I don't
> change that in this patch series.

It's always been that way through various refactorings
of the mmu index, back to commit c1e3781090b9d36 when
the function was added.

In practice we only use arm_mmu_idx_to_el() to get back to
the EL from the MMU index that we put into the TB flags.
So we know it's always one of the "complete translation"
index values, not a Stage1-only, Stage2-only or Phys index.

My guess is I originally put in the assert that enforced
that you don't call it with either a Stage2-only or a
Stage1-only mmuidx because I knew that couldn't happen and
it meant I could implement the idx-to-EL code for the
valid cases as "mmu_idx & 3" and didn't need to add
extra code to handle a Stage1-only index the function
would never see.

thanks
-- PMM