target/arm: Clean up of register field definitions

[PATCH] target/arm: Clean up of register field definitions

Posted by Gustavo Romero 4 months, 3 weeks ago

Clean up the definitions of NSW and NSA fields in the VTCR register.
These two fields are already defined properly using FIELD() so they are
actually duplications. Also, define the NSW and NSA fields in the
VSTCR register using FIELD() and remove their definitions based on VTCR
fields.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
---
 target/arm/internals.h | 8 +++-----
 target/arm/ptw.c       | 8 ++++----
 2 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index c4765e4489..052f7b641c 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -113,11 +113,6 @@ FIELD(DBGWCR, WT, 20, 1)
 FIELD(DBGWCR, MASK, 24, 5)
 FIELD(DBGWCR, SSCE, 29, 1)
 
-#define VTCR_NSW (1u << 29)
-#define VTCR_NSA (1u << 30)
-#define VSTCR_SW VTCR_NSW
-#define VSTCR_SA VTCR_NSA
-
 /* Bit definitions for CPACR (AArch32 only) */
 FIELD(CPACR, CP10, 20, 2)
 FIELD(CPACR, CP11, 22, 2)
@@ -220,6 +215,9 @@ FIELD(VTCR, NSA, 30, 1)
 FIELD(VTCR, DS, 32, 1)
 FIELD(VTCR, SL2, 33, 1)
 
+FIELD(VSTCR, SW, 29, 1)
+FIELD(VSTCR, SA, 30, 1)
+
 #define HCRX_ENAS0    (1ULL << 0)
 #define HCRX_ENALS    (1ULL << 1)
 #define HCRX_ENASR    (1ULL << 2)
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 561bf2678e..ed5c728eab 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -193,9 +193,9 @@ static ARMMMUIdx ptw_idx_for_stage_2(CPUARMState *env, ARMMMUIdx stage2idx)
         return ARMMMUIdx_Phys_Realm;
     case ARMSS_Secure:
         if (stage2idx == ARMMMUIdx_Stage2_S) {
-            s2walk_secure = !(env->cp15.vstcr_el2 & VSTCR_SW);
+            s2walk_secure = !(env->cp15.vstcr_el2 & R_VSTCR_SW_MASK);
         } else {
-            s2walk_secure = !(env->cp15.vtcr_el2 & VTCR_NSW);
+            s2walk_secure = !(env->cp15.vtcr_el2 & R_VTCR_NSW_MASK);
         }
         return s2walk_secure ? ARMMMUIdx_Phys_S : ARMMMUIdx_Phys_NS;
     default:
@@ -3372,9 +3372,9 @@ static bool get_phys_addr_twostage(CPUARMState *env, S1Translate *ptw,
      */
     if (in_space == ARMSS_Secure) {
         result->f.attrs.secure =
-            !(env->cp15.vstcr_el2 & (VSTCR_SA | VSTCR_SW))
+            !(env->cp15.vstcr_el2 & (R_VSTCR_SA_MASK | R_VSTCR_SW_MASK))
             && (ipa_secure
-                || !(env->cp15.vtcr_el2 & (VTCR_NSA | VTCR_NSW)));
+                || !(env->cp15.vtcr_el2 & (R_VTCR_NSA_MASK | R_VTCR_NSW_MASK)));
         result->f.attrs.space = arm_secure_to_space(result->f.attrs.secure);
     }
 
-- 
2.34.1

Re: [PATCH] target/arm: Clean up of register field definitions

Posted by Peter Maydell 4 months ago

On Fri, 25 Jul 2025 at 02:48, Gustavo Romero <gustavo.romero@linaro.org> wrote:
>
> Clean up the definitions of NSW and NSA fields in the VTCR register.
> These two fields are already defined properly using FIELD() so they are
> actually duplications. Also, define the NSW and NSA fields in the
> VSTCR register using FIELD() and remove their definitions based on VTCR
> fields.
>
> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>



Applied to target-arm.next for 10.2, thanks.

-- PMM

Re: [PATCH] target/arm: Clean up of register field definitions

Posted by Philippe Mathieu-Daudé 4 months, 3 weeks ago

Hi Gustavo,

On 25/7/25 03:47, Gustavo Romero wrote:
> Clean up the definitions of NSW and NSA fields in the VTCR register.
> These two fields are already defined properly using FIELD() so they are
> actually duplications. Also, define the NSW and NSA fields in the
> VSTCR register using FIELD() and remove their definitions based on VTCR
> fields.
> 
> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> ---
>   target/arm/internals.h | 8 +++-----
>   target/arm/ptw.c       | 8 ++++----
>   2 files changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index c4765e4489..052f7b641c 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -113,11 +113,6 @@ FIELD(DBGWCR, WT, 20, 1)
>   FIELD(DBGWCR, MASK, 24, 5)
>   FIELD(DBGWCR, SSCE, 29, 1)
>   
> -#define VTCR_NSW (1u << 29)
> -#define VTCR_NSA (1u << 30)
> -#define VSTCR_SW VTCR_NSW
> -#define VSTCR_SA VTCR_NSA
> -
>   /* Bit definitions for CPACR (AArch32 only) */
>   FIELD(CPACR, CP10, 20, 2)
>   FIELD(CPACR, CP11, 22, 2)
> @@ -220,6 +215,9 @@ FIELD(VTCR, NSA, 30, 1)
>   FIELD(VTCR, DS, 32, 1)
>   FIELD(VTCR, SL2, 33, 1)
>   
> +FIELD(VSTCR, SW, 29, 1)
> +FIELD(VSTCR, SA, 30, 1)
> +
>   #define HCRX_ENAS0    (1ULL << 0)
>   #define HCRX_ENALS    (1ULL << 1)
>   #define HCRX_ENASR    (1ULL << 2)
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index 561bf2678e..ed5c728eab 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -193,9 +193,9 @@ static ARMMMUIdx ptw_idx_for_stage_2(CPUARMState *env, ARMMMUIdx stage2idx)
>           return ARMMMUIdx_Phys_Realm;
>       case ARMSS_Secure:
>           if (stage2idx == ARMMMUIdx_Stage2_S) {
> -            s2walk_secure = !(env->cp15.vstcr_el2 & VSTCR_SW);
> +            s2walk_secure = !(env->cp15.vstcr_el2 & R_VSTCR_SW_MASK);

FYI register API provides helper macros:

             s2walk_secure = !FIELD_EX32(env->cp15.vstcr_el2, VSTCR, SW);

Re: [PATCH] target/arm: Clean up of register field definitions

Posted by Gustavo Romero 4 months, 3 weeks ago

Hi Phil,

On 7/25/25 10:18, Philippe Mathieu-Daudé wrote:
> Hi Gustavo,
> 
> On 25/7/25 03:47, Gustavo Romero wrote:
>> Clean up the definitions of NSW and NSA fields in the VTCR register.
>> These two fields are already defined properly using FIELD() so they are
>> actually duplications. Also, define the NSW and NSA fields in the
>> VSTCR register using FIELD() and remove their definitions based on VTCR
>> fields.
>>
>> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
>> ---
>>   target/arm/internals.h | 8 +++-----
>>   target/arm/ptw.c       | 8 ++++----
>>   2 files changed, 7 insertions(+), 9 deletions(-)
>>
>> diff --git a/target/arm/internals.h b/target/arm/internals.h
>> index c4765e4489..052f7b641c 100644
>> --- a/target/arm/internals.h
>> +++ b/target/arm/internals.h
>> @@ -113,11 +113,6 @@ FIELD(DBGWCR, WT, 20, 1)
>>   FIELD(DBGWCR, MASK, 24, 5)
>>   FIELD(DBGWCR, SSCE, 29, 1)
>> -#define VTCR_NSW (1u << 29)
>> -#define VTCR_NSA (1u << 30)
>> -#define VSTCR_SW VTCR_NSW
>> -#define VSTCR_SA VTCR_NSA
>> -
>>   /* Bit definitions for CPACR (AArch32 only) */
>>   FIELD(CPACR, CP10, 20, 2)
>>   FIELD(CPACR, CP11, 22, 2)
>> @@ -220,6 +215,9 @@ FIELD(VTCR, NSA, 30, 1)
>>   FIELD(VTCR, DS, 32, 1)
>>   FIELD(VTCR, SL2, 33, 1)
>> +FIELD(VSTCR, SW, 29, 1)
>> +FIELD(VSTCR, SA, 30, 1)
>> +
>>   #define HCRX_ENAS0    (1ULL << 0)
>>   #define HCRX_ENALS    (1ULL << 1)
>>   #define HCRX_ENASR    (1ULL << 2)
>> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
>> index 561bf2678e..ed5c728eab 100644
>> --- a/target/arm/ptw.c
>> +++ b/target/arm/ptw.c
>> @@ -193,9 +193,9 @@ static ARMMMUIdx ptw_idx_for_stage_2(CPUARMState *env, ARMMMUIdx stage2idx)
>>           return ARMMMUIdx_Phys_Realm;
>>       case ARMSS_Secure:
>>           if (stage2idx == ARMMMUIdx_Stage2_S) {
>> -            s2walk_secure = !(env->cp15.vstcr_el2 & VSTCR_SW);
>> +            s2walk_secure = !(env->cp15.vstcr_el2 & R_VSTCR_SW_MASK);
> 
> FYI register API provides helper macros:
> 
>              s2walk_secure = !FIELD_EX32(env->cp15.vstcr_el2, VSTCR, SW);
> 

Do you know which form is currently preferred? I see that R_<REGNAME>_<FIELD>_MASK is used a lot, .e.g, in helper.c.

Also, even tho the SW field in VSTCR is <= 31, VSTCR is a 64-bit register, so should I really use FIELD_EX32 (which works) or FIELD_EX64 (my first thought would be to use it, i.e. based on the register size, not the field)?

Thanks.


Cheers,
Gustavo

Re: [PATCH] target/arm: Clean up of register field definitions

Posted by Peter Maydell 4 months, 3 weeks ago

On Fri, 25 Jul 2025 at 15:38, Gustavo Romero <gustavo.romero@linaro.org> wrote:
>
> Hi Phil,
>
> On 7/25/25 10:18, Philippe Mathieu-Daudé wrote:
> > Hi Gustavo,
> >
> > On 25/7/25 03:47, Gustavo Romero wrote:
> >>           if (stage2idx == ARMMMUIdx_Stage2_S) {
> >> -            s2walk_secure = !(env->cp15.vstcr_el2 & VSTCR_SW);
> >> +            s2walk_secure = !(env->cp15.vstcr_el2 & R_VSTCR_SW_MASK);
> >
> > FYI register API provides helper macros:
> >
> >              s2walk_secure = !FIELD_EX32(env->cp15.vstcr_el2, VSTCR, SW);
> >
>
> Do you know which form is currently preferred? I see that R_<REGNAME>_<FIELD>_MASK is used a lot, .e.g, in helper.c.

Where a mask is the most simple or useful way to get what
you want, it's fine to use R_*_MASK directly; for instance
in this patch
 (env->cp15.vstcr_el2 & (R_VSTCR_SA_MASK | R_VSTCR_SW_MASK)
is better than
 (FIELD_EX64(env->cp15.vstcr_el2, VSTCR, SA) ||
  FIELD_EX64(env->cp15.vstcr_el2, VSTCR, SW))
and
  value |= R_FOO_BAR_MASK;
seems simpler than
  value = FIELD_DP64(value, FOO, BAR, 1);

I think (though this is to some extent a matter of personal taste).

Where we're using the mask to test whether a single bit field
is set or not, you could do it either way. I think in new code
I would probably use the FIELD_EX64 macro, but a test against
the mask value is fine too.

> Also, even tho the SW field in VSTCR is <= 31, VSTCR is a 64-bit register, so should I really use FIELD_EX32 (which works) or FIELD_EX64 (my first thought would be to use it, i.e. based on the register size, not the field)?

Use FIELD_EX64 for working on 64-bit values, yes.

Personally I don't feel strongly about any of this, so I
would be happy taking this patch the way it is.

-- PMM

Re: [PATCH] target/arm: Clean up of register field definitions

Posted by Gustavo Romero 4 months, 3 weeks ago

Hi Peter,

On 7/25/25 11:47, Peter Maydell wrote:
> On Fri, 25 Jul 2025 at 15:38, Gustavo Romero <gustavo.romero@linaro.org> wrote:
>>
>> Hi Phil,
>>
>> On 7/25/25 10:18, Philippe Mathieu-Daudé wrote:
>>> Hi Gustavo,
>>>
>>> On 25/7/25 03:47, Gustavo Romero wrote:
>>>>            if (stage2idx == ARMMMUIdx_Stage2_S) {
>>>> -            s2walk_secure = !(env->cp15.vstcr_el2 & VSTCR_SW);
>>>> +            s2walk_secure = !(env->cp15.vstcr_el2 & R_VSTCR_SW_MASK);
>>>
>>> FYI register API provides helper macros:
>>>
>>>               s2walk_secure = !FIELD_EX32(env->cp15.vstcr_el2, VSTCR, SW);
>>>
>>
>> Do you know which form is currently preferred? I see that R_<REGNAME>_<FIELD>_MASK is used a lot, .e.g, in helper.c.
> 
> Where a mask is the most simple or useful way to get what
> you want, it's fine to use R_*_MASK directly; for instance
> in this patch
>   (env->cp15.vstcr_el2 & (R_VSTCR_SA_MASK | R_VSTCR_SW_MASK)
> is better than
>   (FIELD_EX64(env->cp15.vstcr_el2, VSTCR, SA) ||
>    FIELD_EX64(env->cp15.vstcr_el2, VSTCR, SW))
> and
>    value |= R_FOO_BAR_MASK;
> seems simpler than
>    value = FIELD_DP64(value, FOO, BAR, 1);
> 
> I think (though this is to some extent a matter of personal taste).

I also prefer the mask for the most simple cases, like in 1 bit masks and where the code becomes more succinct.


> Where we're using the mask to test whether a single bit field
> is set or not, you could do it either way. I think in new code
> I would probably use the FIELD_EX64 macro, but a test against
> the mask value is fine too.
> 
>> Also, even tho the SW field in VSTCR is <= 31, VSTCR is a 64-bit register, so should I really use FIELD_EX32 (which works) or FIELD_EX64 (my first thought would be to use it, i.e. based on the register size, not the field)?
> 
> Use FIELD_EX64 for working on 64-bit values, yes.

Thanks for the clarifications.


> Personally I don't feel strongly about any of this, so I
> would be happy taking this patch the way it is.
> 
> -- PMM

OK, thanks.


Cheers,
Gustavo