[Xen-devel] [PATCH MM-PART2 RESEND v2 04/19] xen/arm: Rework HSCTLR_BASE

Julien Grall posted 19 patches 1 year, 11 months ago

[Xen-devel] [PATCH MM-PART2 RESEND v2 04/19] xen/arm: Rework HSCTLR_BASE

Posted by Julien Grall 1 year, 11 months ago
The current value of HSCTLR_BASE for Arm64 is pretty wrong. It would
actually turn on SCTLR_EL2.nAA (bit 6) on hardware implementing
ARMv8.4-LSE.

Furthermore, the documentation of what is cleared/set in SCTLR_EL2 is
also not correct and looks like to be a verbatim copy from Arm32.

HSCTLR_BASE is replaced with a bunch of per-architecture new defines
helping to understand better what is the initialie value for
SCTLR_EL2/HSCTLR.

Note the defines *_CLEAR are only used to check the state of each bits
are known.

Lastly, the documentation is dropped from arm{32,64}/head.S as it would
be pretty easy to get out-of-sync with the definitions.

Signed-off-by: Julien Grall <julien.grall@arm.com>

---
    Changes in v2:
        - Use BIT(..., UL) instead of _BITUL
---
 xen/arch/arm/arm32/head.S       | 12 +--------
 xen/arch/arm/arm64/head.S       | 10 +-------
 xen/include/asm-arm/processor.h | 54 ++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 55 insertions(+), 21 deletions(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 454d24537c..8a98607459 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -234,17 +234,7 @@ cpu_init_done:
         ldr   r0, =(TCR_RES1|TCR_SH0_IS|TCR_ORGN0_WBWA|TCR_IRGN0_WBWA|TCR_T0SZ(0))
         mcr   CP32(r0, HTCR)
 
-        /*
-         * Set up the HSCTLR:
-         * Exceptions in LE ARM,
-         * Low-latency IRQs disabled,
-         * Write-implies-XN disabled (for now),
-         * D-cache disabled (for now),
-         * I-cache enabled,
-         * Alignment checking enabled,
-         * MMU translation disabled (for now).
-         */
-        ldr   r0, =(HSCTLR_BASE|SCTLR_Axx_ELx_A)
+        ldr   r0, =HSCTLR_SET
         mcr   CP32(r0, HSCTLR)
 
         /*
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 8a6be3352e..4fe904c51d 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -363,15 +363,7 @@ skip_bss:
 
         msr   tcr_el2, x0
 
-        /* Set up the SCTLR_EL2:
-         * Exceptions in LE ARM,
-         * Low-latency IRQs disabled,
-         * Write-implies-XN disabled (for now),
-         * D-cache disabled (for now),
-         * I-cache enabled,
-         * Alignment checking disabled,
-         * MMU translation disabled (for now). */
-        ldr   x0, =(HSCTLR_BASE)
+        ldr   x0, =SCTLR_EL2_SET
         msr   SCTLR_EL2, x0
 
         /* Ensure that any exceptions encountered at EL2
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index bbcba061ca..9afc3786c5 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -127,6 +127,9 @@
 #define SCTLR_A32_ELx_TE    BIT(30, UL)
 #define SCTLR_A32_ELx_FI    BIT(21, UL)
 
+/* Common bits for SCTLR_ELx for Arm64 */
+#define SCTLR_A64_ELx_SA    BIT(3, UL)
+
 /* Common bits for SCTLR_ELx on all architectures */
 #define SCTLR_Axx_ELx_EE    BIT(25, UL)
 #define SCTLR_Axx_ELx_WXN   BIT(19, UL)
@@ -135,7 +138,56 @@
 #define SCTLR_Axx_ELx_A     BIT(1, UL)
 #define SCTLR_Axx_ELx_M     BIT(0, UL)
 
-#define HSCTLR_BASE     _AC(0x30c51878,U)
+#ifdef CONFIG_ARM_32
+
+#define HSCTLR_RES1     (BIT( 3, UL) | BIT( 4, UL) | BIT( 5, UL) |\
+                         BIT( 6, UL) | BIT(11, UL) | BIT(16, UL) |\
+                         BIT(18, UL) | BIT(22, UL) | BIT(23, UL) |\
+                         BIT(28, UL) | BIT(29, UL))
+
+#define HSCTLR_RES0     (BIT(7, UL)  | BIT(8, UL)  | BIT(9, UL)  | BIT(10, UL) |\
+                         BIT(13, UL) | BIT(14, UL) | BIT(15, UL) | BIT(17, UL) |\
+                         BIT(20, UL) | BIT(24, UL) | BIT(26, UL) | BIT(27, UL) |\
+                         BIT(31, UL))
+
+/* Initial value for HSCTLR */
+#define HSCTLR_SET      (HSCTLR_RES1    | SCTLR_Axx_ELx_A   | SCTLR_Axx_ELx_I)
+
+#define HSCTLR_CLEAR    (HSCTLR_RES0        | SCTLR_Axx_ELx_M   |\
+                         SCTLR_Axx_ELx_C    | SCTLR_Axx_ELx_WXN |\
+                         SCTLR_A32_ELx_FI   | SCTLR_Axx_ELx_EE  |\
+                         SCTLR_A32_ELx_TE)
+
+#if (HSCTLR_SET ^ HSCTLR_CLEAR) != 0xffffffffU
+#error "Inconsistent HSCTLR set/clear bits"
+#endif
+
+#else
+
+#define SCTLR_EL2_RES1  (BIT( 4, UL) | BIT( 5, UL) | BIT(11, UL) |\
+                         BIT(16, UL) | BIT(18, UL) | BIT(22, UL) |\
+                         BIT(23, UL) | BIT(28, UL) | BIT(29, UL))
+
+#define SCTLR_EL2_RES0  (BIT( 6, UL) | BIT( 7, UL) | BIT( 8, UL) |\
+                         BIT( 9, UL) | BIT(10, UL) | BIT(13, UL) |\
+                         BIT(14, UL) | BIT(15, UL) | BIT(17, UL) |\
+                         BIT(20, UL) | BIT(21, UL) | BIT(24, UL) |\
+                         BIT(26, UL) | BIT(27, UL) | BIT(30, UL) |\
+                         BIT(31, UL) | (0xffffffffULL << 32))
+
+/* Initial value for SCTLR_EL2 */
+#define SCTLR_EL2_SET   (SCTLR_EL2_RES1     | SCTLR_A64_ELx_SA  |\
+                         SCTLR_Axx_ELx_I)
+
+#define SCTLR_EL2_CLEAR (SCTLR_EL2_RES0     | SCTLR_Axx_ELx_M   |\
+                         SCTLR_Axx_ELx_A    | SCTLR_Axx_ELx_C   |\
+                         SCTLR_Axx_ELx_WXN  | SCTLR_Axx_ELx_EE)
+
+#if (SCTLR_EL2_SET ^ SCTLR_EL2_CLEAR) != 0xffffffffffffffffUL
+#error "Inconsistent SCTLR_EL2 set/clear bits"
+#endif
+
+#endif
 
 /* HCR Hyp Configuration Register */
 #define HCR_RW          (_AC(1,UL)<<31) /* Register Width, ARM64 only */
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH MM-PART2 RESEND v2 04/19] xen/arm: Rework HSCTLR_BASE

Posted by Stefano Stabellini 1 year, 10 months ago
On Tue, 14 May 2019, Julien Grall wrote:
> The current value of HSCTLR_BASE for Arm64 is pretty wrong. It would
> actually turn on SCTLR_EL2.nAA (bit 6) on hardware implementing
> ARMv8.4-LSE.
> 
> Furthermore, the documentation of what is cleared/set in SCTLR_EL2 is
> also not correct and looks like to be a verbatim copy from Arm32.
> 
> HSCTLR_BASE is replaced with a bunch of per-architecture new defines
> helping to understand better what is the initialie value for
> SCTLR_EL2/HSCTLR.
> 
> Note the defines *_CLEAR are only used to check the state of each bits
> are known.

So basically the only purpose of HSCTLR_CLEAR is to execute:

  #if (HSCTLR_SET ^ HSCTLR_CLEAR) != 0xffffffffU

Right? It is good to have the check.

Please add a one-line comment on top of HSCTLR_CLEAR -- "only used to
check that the state of each bits are known".


> Lastly, the documentation is dropped from arm{32,64}/head.S as it would
> be pretty easy to get out-of-sync with the definitions.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
>     Changes in v2:
>         - Use BIT(..., UL) instead of _BITUL
> ---
>  xen/arch/arm/arm32/head.S       | 12 +--------
>  xen/arch/arm/arm64/head.S       | 10 +-------
>  xen/include/asm-arm/processor.h | 54 ++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 55 insertions(+), 21 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 454d24537c..8a98607459 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -234,17 +234,7 @@ cpu_init_done:
>          ldr   r0, =(TCR_RES1|TCR_SH0_IS|TCR_ORGN0_WBWA|TCR_IRGN0_WBWA|TCR_T0SZ(0))
>          mcr   CP32(r0, HTCR)
>  
> -        /*
> -         * Set up the HSCTLR:
> -         * Exceptions in LE ARM,
> -         * Low-latency IRQs disabled,
> -         * Write-implies-XN disabled (for now),
> -         * D-cache disabled (for now),
> -         * I-cache enabled,
> -         * Alignment checking enabled,
> -         * MMU translation disabled (for now).
> -         */
> -        ldr   r0, =(HSCTLR_BASE|SCTLR_Axx_ELx_A)
> +        ldr   r0, =HSCTLR_SET
>          mcr   CP32(r0, HSCTLR)
>  
>          /*
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 8a6be3352e..4fe904c51d 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -363,15 +363,7 @@ skip_bss:
>  
>          msr   tcr_el2, x0
>  
> -        /* Set up the SCTLR_EL2:
> -         * Exceptions in LE ARM,
> -         * Low-latency IRQs disabled,
> -         * Write-implies-XN disabled (for now),
> -         * D-cache disabled (for now),
> -         * I-cache enabled,
> -         * Alignment checking disabled,
> -         * MMU translation disabled (for now). */
> -        ldr   x0, =(HSCTLR_BASE)
> +        ldr   x0, =SCTLR_EL2_SET
>          msr   SCTLR_EL2, x0
>  
>          /* Ensure that any exceptions encountered at EL2
> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
> index bbcba061ca..9afc3786c5 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -127,6 +127,9 @@
>  #define SCTLR_A32_ELx_TE    BIT(30, UL)
>  #define SCTLR_A32_ELx_FI    BIT(21, UL)
>  
> +/* Common bits for SCTLR_ELx for Arm64 */
> +#define SCTLR_A64_ELx_SA    BIT(3, UL)
> +
>  /* Common bits for SCTLR_ELx on all architectures */
>  #define SCTLR_Axx_ELx_EE    BIT(25, UL)
>  #define SCTLR_Axx_ELx_WXN   BIT(19, UL)
> @@ -135,7 +138,56 @@
>  #define SCTLR_Axx_ELx_A     BIT(1, UL)
>  #define SCTLR_Axx_ELx_M     BIT(0, UL)
>  
> -#define HSCTLR_BASE     _AC(0x30c51878,U)
> +#ifdef CONFIG_ARM_32
> +
> +#define HSCTLR_RES1     (BIT( 3, UL) | BIT( 4, UL) | BIT( 5, UL) |\
> +                         BIT( 6, UL) | BIT(11, UL) | BIT(16, UL) |\
> +                         BIT(18, UL) | BIT(22, UL) | BIT(23, UL) |\
> +                         BIT(28, UL) | BIT(29, UL))
> +
> +#define HSCTLR_RES0     (BIT(7, UL)  | BIT(8, UL)  | BIT(9, UL)  | BIT(10, UL) |\
> +                         BIT(13, UL) | BIT(14, UL) | BIT(15, UL) | BIT(17, UL) |\
> +                         BIT(20, UL) | BIT(24, UL) | BIT(26, UL) | BIT(27, UL) |\
> +                         BIT(31, UL))
> +
> +/* Initial value for HSCTLR */
> +#define HSCTLR_SET      (HSCTLR_RES1    | SCTLR_Axx_ELx_A   | SCTLR_Axx_ELx_I)

As far as my calculations go, it looks like the only difference is
SCTLR_Axx_ELx_A compared to 0x30c51878, right? Which is the alignment
check.


> +#define HSCTLR_CLEAR    (HSCTLR_RES0        | SCTLR_Axx_ELx_M   |\
> +                         SCTLR_Axx_ELx_C    | SCTLR_Axx_ELx_WXN |\
> +                         SCTLR_A32_ELx_FI   | SCTLR_Axx_ELx_EE  |\
> +                         SCTLR_A32_ELx_TE)
> +
> +#if (HSCTLR_SET ^ HSCTLR_CLEAR) != 0xffffffffU
> +#error "Inconsistent HSCTLR set/clear bits"
> +#endif
> +
> +#else
> +
> +#define SCTLR_EL2_RES1  (BIT( 4, UL) | BIT( 5, UL) | BIT(11, UL) |\
> +                         BIT(16, UL) | BIT(18, UL) | BIT(22, UL) |\
> +                         BIT(23, UL) | BIT(28, UL) | BIT(29, UL))
> +
> +#define SCTLR_EL2_RES0  (BIT( 6, UL) | BIT( 7, UL) | BIT( 8, UL) |\
> +                         BIT( 9, UL) | BIT(10, UL) | BIT(13, UL) |\
> +                         BIT(14, UL) | BIT(15, UL) | BIT(17, UL) |\
> +                         BIT(20, UL) | BIT(21, UL) | BIT(24, UL) |\
> +                         BIT(26, UL) | BIT(27, UL) | BIT(30, UL) |\
> +                         BIT(31, UL) | (0xffffffffULL << 32))
> +
> +/* Initial value for SCTLR_EL2 */
> +#define SCTLR_EL2_SET   (SCTLR_EL2_RES1     | SCTLR_A64_ELx_SA  |\
> +                         SCTLR_Axx_ELx_I)

Same here, you removed the reserved bits, and added the alignment check,
same as for aarch32. If I got it right, it would be nice to add a
statement like this to the commit message.


> +#define SCTLR_EL2_CLEAR (SCTLR_EL2_RES0     | SCTLR_Axx_ELx_M   |\
> +                         SCTLR_Axx_ELx_A    | SCTLR_Axx_ELx_C   |\
> +                         SCTLR_Axx_ELx_WXN  | SCTLR_Axx_ELx_EE)
> +
> +#if (SCTLR_EL2_SET ^ SCTLR_EL2_CLEAR) != 0xffffffffffffffffUL
> +#error "Inconsistent SCTLR_EL2 set/clear bits"
> +#endif
> +
> +#endif
>  
>  /* HCR Hyp Configuration Register */
>  #define HCR_RW          (_AC(1,UL)<<31) /* Register Width, ARM64 only */

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH MM-PART2 RESEND v2 04/19] xen/arm: Rework HSCTLR_BASE

Posted by Julien Grall 1 year, 10 months ago
Hi,

On 5/20/19 11:56 PM, Stefano Stabellini wrote:
> On Tue, 14 May 2019, Julien Grall wrote:
>> The current value of HSCTLR_BASE for Arm64 is pretty wrong. It would
>> actually turn on SCTLR_EL2.nAA (bit 6) on hardware implementing
>> ARMv8.4-LSE.
>>
>> Furthermore, the documentation of what is cleared/set in SCTLR_EL2 is
>> also not correct and looks like to be a verbatim copy from Arm32.
>>
>> HSCTLR_BASE is replaced with a bunch of per-architecture new defines
>> helping to understand better what is the initialie value for

s/initialie/initial/

>> SCTLR_EL2/HSCTLR.
>>
>> Note the defines *_CLEAR are only used to check the state of each bits
>> are known.
> 
> So basically the only purpose of HSCTLR_CLEAR is to execute:
> 
>    #if (HSCTLR_SET ^ HSCTLR_CLEAR) != 0xffffffffU
> 
> Right? It is good to have the check.
> 
> Please add a one-line comment on top of HSCTLR_CLEAR -- "only used to
> check that the state of each bits are known".

We don't commonly add a comment every time a define is used only one 
time. So what's the benefits here?

In all honesty, such wording in the commit message was probably over the 
top. I am thinking to replace the sentence with:

"Lastly, all the bits are now described as either set or cleared. This 
allows us to check at pre-processing time the consistency of the initial 
value."

> 
> 
>> Lastly, the documentation is dropped from arm{32,64}/head.S as it would
>> be pretty easy to get out-of-sync with the definitions.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>
>> ---
>>      Changes in v2:
>>          - Use BIT(..., UL) instead of _BITUL
>> ---
>>   xen/arch/arm/arm32/head.S       | 12 +--------
>>   xen/arch/arm/arm64/head.S       | 10 +-------
>>   xen/include/asm-arm/processor.h | 54 ++++++++++++++++++++++++++++++++++++++++-
>>   3 files changed, 55 insertions(+), 21 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
>> index 454d24537c..8a98607459 100644
>> --- a/xen/arch/arm/arm32/head.S
>> +++ b/xen/arch/arm/arm32/head.S
>> @@ -234,17 +234,7 @@ cpu_init_done:
>>           ldr   r0, =(TCR_RES1|TCR_SH0_IS|TCR_ORGN0_WBWA|TCR_IRGN0_WBWA|TCR_T0SZ(0))
>>           mcr   CP32(r0, HTCR)
>>   
>> -        /*
>> -         * Set up the HSCTLR:
>> -         * Exceptions in LE ARM,
>> -         * Low-latency IRQs disabled,
>> -         * Write-implies-XN disabled (for now),
>> -         * D-cache disabled (for now),
>> -         * I-cache enabled,
>> -         * Alignment checking enabled,
>> -         * MMU translation disabled (for now).
>> -         */
>> -        ldr   r0, =(HSCTLR_BASE|SCTLR_Axx_ELx_A)
>> +        ldr   r0, =HSCTLR_SET
>>           mcr   CP32(r0, HSCTLR)
>>   
>>           /*
>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>> index 8a6be3352e..4fe904c51d 100644
>> --- a/xen/arch/arm/arm64/head.S
>> +++ b/xen/arch/arm/arm64/head.S
>> @@ -363,15 +363,7 @@ skip_bss:
>>   
>>           msr   tcr_el2, x0
>>   
>> -        /* Set up the SCTLR_EL2:
>> -         * Exceptions in LE ARM,
>> -         * Low-latency IRQs disabled,
>> -         * Write-implies-XN disabled (for now),
>> -         * D-cache disabled (for now),
>> -         * I-cache enabled,
>> -         * Alignment checking disabled,
>> -         * MMU translation disabled (for now). */
>> -        ldr   x0, =(HSCTLR_BASE)
>> +        ldr   x0, =SCTLR_EL2_SET
>>           msr   SCTLR_EL2, x0
>>   
>>           /* Ensure that any exceptions encountered at EL2
>> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
>> index bbcba061ca..9afc3786c5 100644
>> --- a/xen/include/asm-arm/processor.h
>> +++ b/xen/include/asm-arm/processor.h
>> @@ -127,6 +127,9 @@
>>   #define SCTLR_A32_ELx_TE    BIT(30, UL)
>>   #define SCTLR_A32_ELx_FI    BIT(21, UL)
>>   
>> +/* Common bits for SCTLR_ELx for Arm64 */
>> +#define SCTLR_A64_ELx_SA    BIT(3, UL)
>> +
>>   /* Common bits for SCTLR_ELx on all architectures */
>>   #define SCTLR_Axx_ELx_EE    BIT(25, UL)
>>   #define SCTLR_Axx_ELx_WXN   BIT(19, UL)
>> @@ -135,7 +138,56 @@
>>   #define SCTLR_Axx_ELx_A     BIT(1, UL)
>>   #define SCTLR_Axx_ELx_M     BIT(0, UL)
>>   
>> -#define HSCTLR_BASE     _AC(0x30c51878,U)
>> +#ifdef CONFIG_ARM_32
>> +
>> +#define HSCTLR_RES1     (BIT( 3, UL) | BIT( 4, UL) | BIT( 5, UL) |\
>> +                         BIT( 6, UL) | BIT(11, UL) | BIT(16, UL) |\
>> +                         BIT(18, UL) | BIT(22, UL) | BIT(23, UL) |\
>> +                         BIT(28, UL) | BIT(29, UL))
>> +
>> +#define HSCTLR_RES0     (BIT(7, UL)  | BIT(8, UL)  | BIT(9, UL)  | BIT(10, UL) |\
>> +                         BIT(13, UL) | BIT(14, UL) | BIT(15, UL) | BIT(17, UL) |\
>> +                         BIT(20, UL) | BIT(24, UL) | BIT(26, UL) | BIT(27, UL) |\
>> +                         BIT(31, UL))
>> +
>> +/* Initial value for HSCTLR */
>> +#define HSCTLR_SET      (HSCTLR_RES1    | SCTLR_Axx_ELx_A   | SCTLR_Axx_ELx_I)
> 
> As far as my calculations go, it looks like the only difference is
> SCTLR_Axx_ELx_A compared to 0x30c51878, right? Which is the alignment
> check.

That's correct and match the initial value on Arm32 (see HSCTR_SET | 
SCTLR_Axx_ELx_A in the head.S).

> 
> 
>> +#define HSCTLR_CLEAR    (HSCTLR_RES0        | SCTLR_Axx_ELx_M   |\
>> +                         SCTLR_Axx_ELx_C    | SCTLR_Axx_ELx_WXN |\
>> +                         SCTLR_A32_ELx_FI   | SCTLR_Axx_ELx_EE  |\
>> +                         SCTLR_A32_ELx_TE)
>> +
>> +#if (HSCTLR_SET ^ HSCTLR_CLEAR) != 0xffffffffU
>> +#error "Inconsistent HSCTLR set/clear bits"
>> +#endif
>> +
>> +#else
>> +
>> +#define SCTLR_EL2_RES1  (BIT( 4, UL) | BIT( 5, UL) | BIT(11, UL) |\
>> +                         BIT(16, UL) | BIT(18, UL) | BIT(22, UL) |\
>> +                         BIT(23, UL) | BIT(28, UL) | BIT(29, UL))
>> +
>> +#define SCTLR_EL2_RES0  (BIT( 6, UL) | BIT( 7, UL) | BIT( 8, UL) |\
>> +                         BIT( 9, UL) | BIT(10, UL) | BIT(13, UL) |\
>> +                         BIT(14, UL) | BIT(15, UL) | BIT(17, UL) |\
>> +                         BIT(20, UL) | BIT(21, UL) | BIT(24, UL) |\
>> +                         BIT(26, UL) | BIT(27, UL) | BIT(30, UL) |\
>> +                         BIT(31, UL) | (0xffffffffULL << 32))
>> +
>> +/* Initial value for SCTLR_EL2 */
>> +#define SCTLR_EL2_SET   (SCTLR_EL2_RES1     | SCTLR_A64_ELx_SA  |\
>> +                         SCTLR_Axx_ELx_I)
> 
> Same here, you removed the reserved bits, and added the alignment check,
> same as for aarch32. If I got it right, it would be nice to add a
> statement like this to the commit message.

I don't see why "reserved bits" I dropped nor which alignment check I added.

It would be extremely useful if you provide more details in your 
review...  In this case, it would be the exact bits I dropped/added.

> 
> 
>> +#define SCTLR_EL2_CLEAR (SCTLR_EL2_RES0     | SCTLR_Axx_ELx_M   |\
>> +                         SCTLR_Axx_ELx_A    | SCTLR_Axx_ELx_C   |\
>> +                         SCTLR_Axx_ELx_WXN  | SCTLR_Axx_ELx_EE)
>> +
>> +#if (SCTLR_EL2_SET ^ SCTLR_EL2_CLEAR) != 0xffffffffffffffffUL
>> +#error "Inconsistent SCTLR_EL2 set/clear bits"
>> +#endif
>> +
>> +#endif
>>   
>>   /* HCR Hyp Configuration Register */
>>   #define HCR_RW          (_AC(1,UL)<<31) /* Register Width, ARM64 only */

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH MM-PART2 RESEND v2 04/19] xen/arm: Rework HSCTLR_BASE

Posted by Andrii Anisov 1 year, 10 months ago

On 21.05.19 13:09, Julien Grall wrote:
> Hi,
> 
> On 5/20/19 11:56 PM, Stefano Stabellini wrote:
>> On Tue, 14 May 2019, Julien Grall wrote:
>>> The current value of HSCTLR_BASE for Arm64 is pretty wrong. It would
>>> actually turn on SCTLR_EL2.nAA (bit 6) on hardware implementing
>>> ARMv8.4-LSE.
>>>
>>> Furthermore, the documentation of what is cleared/set in SCTLR_EL2 is
>>> also not correct and looks like to be a verbatim copy from Arm32.
>>>
>>> HSCTLR_BASE is replaced with a bunch of per-architecture new defines
>>> helping to understand better what is the initialie value for
> 
> s/initialie/initial/
> 
>>> Lastly, the documentation is dropped from arm{32,64}/head.S as it would
>>> be pretty easy to get out-of-sync with the definitions.
>>>
>>> Signed-off-by: Julien Grall <julien.grall@arm.com>

FWIW, with misprint fixed

Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH MM-PART2 RESEND v2 04/19] xen/arm: Rework HSCTLR_BASE

Posted by Julien Grall 1 year, 10 months ago
Ping, it would be good to know which bits I dropped...

On 21/05/2019 11:09, Julien Grall wrote:
> Hi,
> 
> On 5/20/19 11:56 PM, Stefano Stabellini wrote:
>> On Tue, 14 May 2019, Julien Grall wrote:
>>> The current value of HSCTLR_BASE for Arm64 is pretty wrong. It would
>>> actually turn on SCTLR_EL2.nAA (bit 6) on hardware implementing
>>> ARMv8.4-LSE.
>>>
>>> Furthermore, the documentation of what is cleared/set in SCTLR_EL2 is
>>> also not correct and looks like to be a verbatim copy from Arm32.
>>>
>>> HSCTLR_BASE is replaced with a bunch of per-architecture new defines
>>> helping to understand better what is the initialie value for
> 
> s/initialie/initial/
> 
>>> SCTLR_EL2/HSCTLR.
>>>
>>> Note the defines *_CLEAR are only used to check the state of each bits
>>> are known.
>>
>> So basically the only purpose of HSCTLR_CLEAR is to execute:
>>
>>    #if (HSCTLR_SET ^ HSCTLR_CLEAR) != 0xffffffffU
>>
>> Right? It is good to have the check.
>>
>> Please add a one-line comment on top of HSCTLR_CLEAR -- "only used to
>> check that the state of each bits are known".
> 
> We don't commonly add a comment every time a define is used only one time. So 
> what's the benefits here?
> 
> In all honesty, such wording in the commit message was probably over the top. I 
> am thinking to replace the sentence with:
> 
> "Lastly, all the bits are now described as either set or cleared. This allows us 
> to check at pre-processing time the consistency of the initial value."
> 
>>
>>
>>> Lastly, the documentation is dropped from arm{32,64}/head.S as it would
>>> be pretty easy to get out-of-sync with the definitions.
>>>
>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>
>>> ---
>>>      Changes in v2:
>>>          - Use BIT(..., UL) instead of _BITUL
>>> ---
>>>   xen/arch/arm/arm32/head.S       | 12 +--------
>>>   xen/arch/arm/arm64/head.S       | 10 +-------
>>>   xen/include/asm-arm/processor.h | 54 ++++++++++++++++++++++++++++++++++++++++-
>>>   3 files changed, 55 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
>>> index 454d24537c..8a98607459 100644
>>> --- a/xen/arch/arm/arm32/head.S
>>> +++ b/xen/arch/arm/arm32/head.S
>>> @@ -234,17 +234,7 @@ cpu_init_done:
>>>           ldr   r0, 
>>> =(TCR_RES1|TCR_SH0_IS|TCR_ORGN0_WBWA|TCR_IRGN0_WBWA|TCR_T0SZ(0))
>>>           mcr   CP32(r0, HTCR)
>>> -        /*
>>> -         * Set up the HSCTLR:
>>> -         * Exceptions in LE ARM,
>>> -         * Low-latency IRQs disabled,
>>> -         * Write-implies-XN disabled (for now),
>>> -         * D-cache disabled (for now),
>>> -         * I-cache enabled,
>>> -         * Alignment checking enabled,
>>> -         * MMU translation disabled (for now).
>>> -         */
>>> -        ldr   r0, =(HSCTLR_BASE|SCTLR_Axx_ELx_A)
>>> +        ldr   r0, =HSCTLR_SET
>>>           mcr   CP32(r0, HSCTLR)
>>>           /*
>>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>>> index 8a6be3352e..4fe904c51d 100644
>>> --- a/xen/arch/arm/arm64/head.S
>>> +++ b/xen/arch/arm/arm64/head.S
>>> @@ -363,15 +363,7 @@ skip_bss:
>>>           msr   tcr_el2, x0
>>> -        /* Set up the SCTLR_EL2:
>>> -         * Exceptions in LE ARM,
>>> -         * Low-latency IRQs disabled,
>>> -         * Write-implies-XN disabled (for now),
>>> -         * D-cache disabled (for now),
>>> -         * I-cache enabled,
>>> -         * Alignment checking disabled,
>>> -         * MMU translation disabled (for now). */
>>> -        ldr   x0, =(HSCTLR_BASE)
>>> +        ldr   x0, =SCTLR_EL2_SET
>>>           msr   SCTLR_EL2, x0
>>>           /* Ensure that any exceptions encountered at EL2
>>> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
>>> index bbcba061ca..9afc3786c5 100644
>>> --- a/xen/include/asm-arm/processor.h
>>> +++ b/xen/include/asm-arm/processor.h
>>> @@ -127,6 +127,9 @@
>>>   #define SCTLR_A32_ELx_TE    BIT(30, UL)
>>>   #define SCTLR_A32_ELx_FI    BIT(21, UL)
>>> +/* Common bits for SCTLR_ELx for Arm64 */
>>> +#define SCTLR_A64_ELx_SA    BIT(3, UL)
>>> +
>>>   /* Common bits for SCTLR_ELx on all architectures */
>>>   #define SCTLR_Axx_ELx_EE    BIT(25, UL)
>>>   #define SCTLR_Axx_ELx_WXN   BIT(19, UL)
>>> @@ -135,7 +138,56 @@
>>>   #define SCTLR_Axx_ELx_A     BIT(1, UL)
>>>   #define SCTLR_Axx_ELx_M     BIT(0, UL)
>>> -#define HSCTLR_BASE     _AC(0x30c51878,U)
>>> +#ifdef CONFIG_ARM_32
>>> +
>>> +#define HSCTLR_RES1     (BIT( 3, UL) | BIT( 4, UL) | BIT( 5, UL) |\
>>> +                         BIT( 6, UL) | BIT(11, UL) | BIT(16, UL) |\
>>> +                         BIT(18, UL) | BIT(22, UL) | BIT(23, UL) |\
>>> +                         BIT(28, UL) | BIT(29, UL))
>>> +
>>> +#define HSCTLR_RES0     (BIT(7, UL)  | BIT(8, UL)  | BIT(9, UL)  | BIT(10, 
>>> UL) |\
>>> +                         BIT(13, UL) | BIT(14, UL) | BIT(15, UL) | BIT(17, 
>>> UL) |\
>>> +                         BIT(20, UL) | BIT(24, UL) | BIT(26, UL) | BIT(27, 
>>> UL) |\
>>> +                         BIT(31, UL))
>>> +
>>> +/* Initial value for HSCTLR */
>>> +#define HSCTLR_SET      (HSCTLR_RES1    | SCTLR_Axx_ELx_A   | SCTLR_Axx_ELx_I)
>>
>> As far as my calculations go, it looks like the only difference is
>> SCTLR_Axx_ELx_A compared to 0x30c51878, right? Which is the alignment
>> check.
> 
> That's correct and match the initial value on Arm32 (see HSCTR_SET | 
> SCTLR_Axx_ELx_A in the head.S).
> 
>>
>>
>>> +#define HSCTLR_CLEAR    (HSCTLR_RES0        | SCTLR_Axx_ELx_M   |\
>>> +                         SCTLR_Axx_ELx_C    | SCTLR_Axx_ELx_WXN |\
>>> +                         SCTLR_A32_ELx_FI   | SCTLR_Axx_ELx_EE  |\
>>> +                         SCTLR_A32_ELx_TE)
>>> +
>>> +#if (HSCTLR_SET ^ HSCTLR_CLEAR) != 0xffffffffU
>>> +#error "Inconsistent HSCTLR set/clear bits"
>>> +#endif
>>> +
>>> +#else
>>> +
>>> +#define SCTLR_EL2_RES1  (BIT( 4, UL) | BIT( 5, UL) | BIT(11, UL) |\
>>> +                         BIT(16, UL) | BIT(18, UL) | BIT(22, UL) |\
>>> +                         BIT(23, UL) | BIT(28, UL) | BIT(29, UL))
>>> +
>>> +#define SCTLR_EL2_RES0  (BIT( 6, UL) | BIT( 7, UL) | BIT( 8, UL) |\
>>> +                         BIT( 9, UL) | BIT(10, UL) | BIT(13, UL) |\
>>> +                         BIT(14, UL) | BIT(15, UL) | BIT(17, UL) |\
>>> +                         BIT(20, UL) | BIT(21, UL) | BIT(24, UL) |\
>>> +                         BIT(26, UL) | BIT(27, UL) | BIT(30, UL) |\
>>> +                         BIT(31, UL) | (0xffffffffULL << 32))
>>> +
>>> +/* Initial value for SCTLR_EL2 */
>>> +#define SCTLR_EL2_SET   (SCTLR_EL2_RES1     | SCTLR_A64_ELx_SA  |\
>>> +                         SCTLR_Axx_ELx_I)
>>
>> Same here, you removed the reserved bits, and added the alignment check,
>> same as for aarch32. If I got it right, it would be nice to add a
>> statement like this to the commit message.
> 
> I don't see why "reserved bits" I dropped nor which alignment check I added.
> 
> It would be extremely useful if you provide more details in your review...  In 
> this case, it would be the exact bits I dropped/added.
> 
>>
>>
>>> +#define SCTLR_EL2_CLEAR (SCTLR_EL2_RES0     | SCTLR_Axx_ELx_M   |\
>>> +                         SCTLR_Axx_ELx_A    | SCTLR_Axx_ELx_C   |\
>>> +                         SCTLR_Axx_ELx_WXN  | SCTLR_Axx_ELx_EE)
>>> +
>>> +#if (SCTLR_EL2_SET ^ SCTLR_EL2_CLEAR) != 0xffffffffffffffffUL
>>> +#error "Inconsistent SCTLR_EL2 set/clear bits"
>>> +#endif
>>> +
>>> +#endif
>>>   /* HCR Hyp Configuration Register */
>>>   #define HCR_RW          (_AC(1,UL)<<31) /* Register Width, ARM64 only */
> 
> Cheers,
> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH MM-PART2 RESEND v2 04/19] xen/arm: Rework HSCTLR_BASE

Posted by Stefano Stabellini 1 year, 10 months ago
On Wed, 29 May 2019, Julien Grall wrote:
> Ping, it would be good to know which bits I dropped...
> 
> On 21/05/2019 11:09, Julien Grall wrote:
> > Hi,
> > 
> > On 5/20/19 11:56 PM, Stefano Stabellini wrote:
> > > On Tue, 14 May 2019, Julien Grall wrote:
> > > > The current value of HSCTLR_BASE for Arm64 is pretty wrong. It would
> > > > actually turn on SCTLR_EL2.nAA (bit 6) on hardware implementing
> > > > ARMv8.4-LSE.
> > > > 
> > > > Furthermore, the documentation of what is cleared/set in SCTLR_EL2 is
> > > > also not correct and looks like to be a verbatim copy from Arm32.
> > > > 
> > > > HSCTLR_BASE is replaced with a bunch of per-architecture new defines
> > > > helping to understand better what is the initialie value for
> > 
> > s/initialie/initial/
> > 
> > > > SCTLR_EL2/HSCTLR.
> > > > 
> > > > Note the defines *_CLEAR are only used to check the state of each bits
> > > > are known.
> > > 
> > > So basically the only purpose of HSCTLR_CLEAR is to execute:
> > > 
> > >    #if (HSCTLR_SET ^ HSCTLR_CLEAR) != 0xffffffffU
> > > 
> > > Right? It is good to have the check.
> > > 
> > > Please add a one-line comment on top of HSCTLR_CLEAR -- "only used to
> > > check that the state of each bits are known".
> > 
> > We don't commonly add a comment every time a define is used only one time.
> > So what's the benefits here?
> >
> > In all honesty, such wording in the commit message was probably over the
> > top. I am thinking to replace the sentence with:
> > 
> > "Lastly, all the bits are now described as either set or cleared. This
> > allows us to check at pre-processing time the consistency of the initial
> > value."

This is even clearer, but I understood that part of the commit message
well enough even before. I have no complaints there. My suggestion for
an in-code comment is because the purpose of HSCTLR_CLEAR is not
immediately obvious looking at the code only.  The commit message is
fine. I think that a one-liner in the code to say that HSCTLR_CLEAR is
"only used at pre-processing time" would be good to have and beneficial
for code readability.


> > > 
> > > 
> > > > Lastly, the documentation is dropped from arm{32,64}/head.S as it would
> > > > be pretty easy to get out-of-sync with the definitions.
> > > > 
> > > > Signed-off-by: Julien Grall <julien.grall@arm.com>
> > > > 
> > > > ---
> > > >      Changes in v2:
> > > >          - Use BIT(..., UL) instead of _BITUL
> > > > ---
> > > >   xen/arch/arm/arm32/head.S       | 12 +--------
> > > >   xen/arch/arm/arm64/head.S       | 10 +-------
> > > >   xen/include/asm-arm/processor.h | 54
> > > > ++++++++++++++++++++++++++++++++++++++++-
> > > >   3 files changed, 55 insertions(+), 21 deletions(-)
> > > > 
> > > > diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> > > > index 454d24537c..8a98607459 100644
> > > > --- a/xen/arch/arm/arm32/head.S
> > > > +++ b/xen/arch/arm/arm32/head.S
> > > > @@ -234,17 +234,7 @@ cpu_init_done:
> > > >           ldr   r0,
> > > > =(TCR_RES1|TCR_SH0_IS|TCR_ORGN0_WBWA|TCR_IRGN0_WBWA|TCR_T0SZ(0))
> > > >           mcr   CP32(r0, HTCR)
> > > > -        /*
> > > > -         * Set up the HSCTLR:
> > > > -         * Exceptions in LE ARM,
> > > > -         * Low-latency IRQs disabled,
> > > > -         * Write-implies-XN disabled (for now),
> > > > -         * D-cache disabled (for now),
> > > > -         * I-cache enabled,
> > > > -         * Alignment checking enabled,
> > > > -         * MMU translation disabled (for now).
> > > > -         */
> > > > -        ldr   r0, =(HSCTLR_BASE|SCTLR_Axx_ELx_A)
> > > > +        ldr   r0, =HSCTLR_SET
> > > >           mcr   CP32(r0, HSCTLR)
> > > >           /*
> > > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> > > > index 8a6be3352e..4fe904c51d 100644
> > > > --- a/xen/arch/arm/arm64/head.S
> > > > +++ b/xen/arch/arm/arm64/head.S
> > > > @@ -363,15 +363,7 @@ skip_bss:
> > > >           msr   tcr_el2, x0
> > > > -        /* Set up the SCTLR_EL2:
> > > > -         * Exceptions in LE ARM,
> > > > -         * Low-latency IRQs disabled,
> > > > -         * Write-implies-XN disabled (for now),
> > > > -         * D-cache disabled (for now),
> > > > -         * I-cache enabled,
> > > > -         * Alignment checking disabled,
> > > > -         * MMU translation disabled (for now). */
> > > > -        ldr   x0, =(HSCTLR_BASE)
> > > > +        ldr   x0, =SCTLR_EL2_SET
> > > >           msr   SCTLR_EL2, x0
> > > >           /* Ensure that any exceptions encountered at EL2
> > > > diff --git a/xen/include/asm-arm/processor.h
> > > > b/xen/include/asm-arm/processor.h
> > > > index bbcba061ca..9afc3786c5 100644
> > > > --- a/xen/include/asm-arm/processor.h
> > > > +++ b/xen/include/asm-arm/processor.h
> > > > @@ -127,6 +127,9 @@
> > > >   #define SCTLR_A32_ELx_TE    BIT(30, UL)
> > > >   #define SCTLR_A32_ELx_FI    BIT(21, UL)
> > > > +/* Common bits for SCTLR_ELx for Arm64 */
> > > > +#define SCTLR_A64_ELx_SA    BIT(3, UL)
> > > > +
> > > >   /* Common bits for SCTLR_ELx on all architectures */
> > > >   #define SCTLR_Axx_ELx_EE    BIT(25, UL)
> > > >   #define SCTLR_Axx_ELx_WXN   BIT(19, UL)
> > > > @@ -135,7 +138,56 @@
> > > >   #define SCTLR_Axx_ELx_A     BIT(1, UL)
> > > >   #define SCTLR_Axx_ELx_M     BIT(0, UL)
> > > > -#define HSCTLR_BASE     _AC(0x30c51878,U)
> > > > +#ifdef CONFIG_ARM_32
> > > > +
> > > > +#define HSCTLR_RES1     (BIT( 3, UL) | BIT( 4, UL) | BIT( 5, UL) |\
> > > > +                         BIT( 6, UL) | BIT(11, UL) | BIT(16, UL) |\
> > > > +                         BIT(18, UL) | BIT(22, UL) | BIT(23, UL) |\
> > > > +                         BIT(28, UL) | BIT(29, UL))
> > > > +
> > > > +#define HSCTLR_RES0     (BIT(7, UL)  | BIT(8, UL)  | BIT(9, UL)  |
> > > > BIT(10, UL) |\
> > > > +                         BIT(13, UL) | BIT(14, UL) | BIT(15, UL) |
> > > > BIT(17, UL) |\
> > > > +                         BIT(20, UL) | BIT(24, UL) | BIT(26, UL) |
> > > > BIT(27, UL) |\
> > > > +                         BIT(31, UL))
> > > > +
> > > > +/* Initial value for HSCTLR */
> > > > +#define HSCTLR_SET      (HSCTLR_RES1    | SCTLR_Axx_ELx_A   |
> > > > SCTLR_Axx_ELx_I)
> > > 
> > > As far as my calculations go, it looks like the only difference is
> > > SCTLR_Axx_ELx_A compared to 0x30c51878, right? Which is the alignment
> > > check.
> > 
> > That's correct and match the initial value on Arm32 (see HSCTR_SET |
> > SCTLR_Axx_ELx_A in the head.S).

OK


> > > 
> > > 
> > > > +#define HSCTLR_CLEAR    (HSCTLR_RES0        | SCTLR_Axx_ELx_M   |\
> > > > +                         SCTLR_Axx_ELx_C    | SCTLR_Axx_ELx_WXN |\
> > > > +                         SCTLR_A32_ELx_FI   | SCTLR_Axx_ELx_EE  |\
> > > > +                         SCTLR_A32_ELx_TE)
> > > > +
> > > > +#if (HSCTLR_SET ^ HSCTLR_CLEAR) != 0xffffffffU
> > > > +#error "Inconsistent HSCTLR set/clear bits"
> > > > +#endif
> > > > +
> > > > +#else
> > > > +
> > > > +#define SCTLR_EL2_RES1  (BIT( 4, UL) | BIT( 5, UL) | BIT(11, UL) |\
> > > > +                         BIT(16, UL) | BIT(18, UL) | BIT(22, UL) |\
> > > > +                         BIT(23, UL) | BIT(28, UL) | BIT(29, UL))
> > > > +
> > > > +#define SCTLR_EL2_RES0  (BIT( 6, UL) | BIT( 7, UL) | BIT( 8, UL) |\
> > > > +                         BIT( 9, UL) | BIT(10, UL) | BIT(13, UL) |\
> > > > +                         BIT(14, UL) | BIT(15, UL) | BIT(17, UL) |\
> > > > +                         BIT(20, UL) | BIT(21, UL) | BIT(24, UL) |\
> > > > +                         BIT(26, UL) | BIT(27, UL) | BIT(30, UL) |\
> > > > +                         BIT(31, UL) | (0xffffffffULL << 32))
> > > > +
> > > > +/* Initial value for SCTLR_EL2 */
> > > > +#define SCTLR_EL2_SET   (SCTLR_EL2_RES1     | SCTLR_A64_ELx_SA  |\
> > > > +                         SCTLR_Axx_ELx_I)
> > > 
> > > Same here, you removed the reserved bits, and added the alignment check,
> > > same as for aarch32. If I got it right, it would be nice to add a
> > > statement like this to the commit message.
> > 
> > I don't see why "reserved bits" I dropped nor which alignment check I added.
> > 
> > It would be extremely useful if you provide more details in your review... 
> > In this case, it would be the exact bits I dropped/added.

I looked at the full value of SCTLR_EL2_SET, it's 0x30c51838. I
copy/paste here the wcalc command for our own convenience:

wcalc -h '(1<<4)|(1<<5)|(1<<11)|(1<<16)|(1<<18)|(1<<22)|(1<<23)|(1<<28)|(1<<29)|(1<<3)|(1<<12)'

HSCTLR_BASE is 0x30c51878. The difference is bit 6 which is RES0. It
looks like I was wrong about the alignment check.


> > > 
> > > 
> > > > +#define SCTLR_EL2_CLEAR (SCTLR_EL2_RES0     | SCTLR_Axx_ELx_M   |\
> > > > +                         SCTLR_Axx_ELx_A    | SCTLR_Axx_ELx_C   |\
> > > > +                         SCTLR_Axx_ELx_WXN  | SCTLR_Axx_ELx_EE)
> > > > +
> > > > +#if (SCTLR_EL2_SET ^ SCTLR_EL2_CLEAR) != 0xffffffffffffffffUL
> > > > +#error "Inconsistent SCTLR_EL2 set/clear bits"
> > > > +#endif
> > > > +
> > > > +#endif
> > > >   /* HCR Hyp Configuration Register */
> > > >   #define HCR_RW          (_AC(1,UL)<<31) /* Register Width, ARM64 only
> > > > */_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH MM-PART2 RESEND v2 04/19] xen/arm: Rework HSCTLR_BASE

Posted by Julien Grall 1 year, 10 months ago
Hi Stefano,

On 6/4/19 12:12 AM, Stefano Stabellini wrote:
> On Wed, 29 May 2019, Julien Grall wrote:
>> Ping, it would be good to know which bits I dropped...
>>
>> On 21/05/2019 11:09, Julien Grall wrote:
>>> Hi,
>>>
>>> On 5/20/19 11:56 PM, Stefano Stabellini wrote:
>>>> On Tue, 14 May 2019, Julien Grall wrote:
>>>>> The current value of HSCTLR_BASE for Arm64 is pretty wrong. It would
>>>>> actually turn on SCTLR_EL2.nAA (bit 6) on hardware implementing
>>>>> ARMv8.4-LSE.
>>>>>
>>>>> Furthermore, the documentation of what is cleared/set in SCTLR_EL2 is
>>>>> also not correct and looks like to be a verbatim copy from Arm32.
>>>>>
>>>>> HSCTLR_BASE is replaced with a bunch of per-architecture new defines
>>>>> helping to understand better what is the initialie value for
>>>
>>> s/initialie/initial/
>>>
>>>>> SCTLR_EL2/HSCTLR.
>>>>>
>>>>> Note the defines *_CLEAR are only used to check the state of each bits
>>>>> are known.
>>>>
>>>> So basically the only purpose of HSCTLR_CLEAR is to execute:
>>>>
>>>>     #if (HSCTLR_SET ^ HSCTLR_CLEAR) != 0xffffffffU
>>>>
>>>> Right? It is good to have the check.
>>>>
>>>> Please add a one-line comment on top of HSCTLR_CLEAR -- "only used to
>>>> check that the state of each bits are known".
>>>
>>> We don't commonly add a comment every time a define is used only one time.
>>> So what's the benefits here?
>>>
>>> In all honesty, such wording in the commit message was probably over the
>>> top. I am thinking to replace the sentence with:
>>>
>>> "Lastly, all the bits are now described as either set or cleared. This
>>> allows us to check at pre-processing time the consistency of the initial
>>> value."
> 
> This is even clearer, but I understood that part of the commit message
> well enough even before. I have no complaints there. My suggestion for
> an in-code comment is because the purpose of HSCTLR_CLEAR is not
> immediately obvious looking at the code only.  The commit message is
> fine. I think that a one-liner in the code to say that HSCTLR_CLEAR is
> "only used at pre-processing time" would be good to have and beneficial
> for code readability.

It is quite an odd comment as a lot of defines are only used for 
pre-processing it (i.e CONFIG_ or even macro generating function)... It 
is going to rot quickly but I can add it if you think it improves the 
code...

>>>> Same here, you removed the reserved bits, and added the alignment check,
>>>> same as for aarch32. If I got it right, it would be nice to add a
>>>> statement like this to the commit message.
>>>
>>> I don't see why "reserved bits" I dropped nor which alignment check I added.
>>>
>>> It would be extremely useful if you provide more details in your review...
>>> In this case, it would be the exact bits I dropped/added.
> 
> I looked at the full value of SCTLR_EL2_SET, it's 0x30c51838. I
> copy/paste here the wcalc command for our own convenience:
> 
> wcalc -h '(1<<4)|(1<<5)|(1<<11)|(1<<16)|(1<<18)|(1<<22)|(1<<23)|(1<<28)|(1<<29)|(1<<3)|(1<<12)'
> 
> HSCTLR_BASE is 0x30c51878. The difference is bit 6 which is RES0. It
> looks like I was wrong about the alignment check.

This was mentioned in the commit message:

"The current value of HSCTLR_BASE for Arm64 is pretty wrong. It would
actually turn on SCTLR_EL2.nAA (bit 6) on hardware implementing
ARMv8.4-LSE."

> 
> 
>>>>
>>>>
>>>>> +#define SCTLR_EL2_CLEAR (SCTLR_EL2_RES0     | SCTLR_Axx_ELx_M   |\
>>>>> +                         SCTLR_Axx_ELx_A    | SCTLR_Axx_ELx_C   |\
>>>>> +                         SCTLR_Axx_ELx_WXN  | SCTLR_Axx_ELx_EE)
>>>>> +
>>>>> +#if (SCTLR_EL2_SET ^ SCTLR_EL2_CLEAR) != 0xffffffffffffffffUL
>>>>> +#error "Inconsistent SCTLR_EL2 set/clear bits"
>>>>> +#endif
>>>>> +
>>>>> +#endif
>>>>>    /* HCR Hyp Configuration Register */
>>>>>    #define HCR_RW          (_AC(1,UL)<<31) /* Register Width, ARM64 only
>>>>> */

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH MM-PART2 RESEND v2 04/19] xen/arm: Rework HSCTLR_BASE

Posted by Stefano Stabellini 1 year, 10 months ago
On Tue, 4 Jun 2019, Julien Grall wrote:
> Hi Stefano,
> 
> On 6/4/19 12:12 AM, Stefano Stabellini wrote:
> > On Wed, 29 May 2019, Julien Grall wrote:
> > > Ping, it would be good to know which bits I dropped...
> > > 
> > > On 21/05/2019 11:09, Julien Grall wrote:
> > > > Hi,
> > > > 
> > > > On 5/20/19 11:56 PM, Stefano Stabellini wrote:
> > > > > On Tue, 14 May 2019, Julien Grall wrote:
> > > > > > The current value of HSCTLR_BASE for Arm64 is pretty wrong. It would
> > > > > > actually turn on SCTLR_EL2.nAA (bit 6) on hardware implementing
> > > > > > ARMv8.4-LSE.
> > > > > > 
> > > > > > Furthermore, the documentation of what is cleared/set in SCTLR_EL2
> > > > > > is
> > > > > > also not correct and looks like to be a verbatim copy from Arm32.
> > > > > > 
> > > > > > HSCTLR_BASE is replaced with a bunch of per-architecture new defines
> > > > > > helping to understand better what is the initialie value for
> > > > 
> > > > s/initialie/initial/
> > > > 
> > > > > > SCTLR_EL2/HSCTLR.
> > > > > > 
> > > > > > Note the defines *_CLEAR are only used to check the state of each
> > > > > > bits
> > > > > > are known.
> > > > > 
> > > > > So basically the only purpose of HSCTLR_CLEAR is to execute:
> > > > > 
> > > > >     #if (HSCTLR_SET ^ HSCTLR_CLEAR) != 0xffffffffU
> > > > > 
> > > > > Right? It is good to have the check.
> > > > > 
> > > > > Please add a one-line comment on top of HSCTLR_CLEAR -- "only used to
> > > > > check that the state of each bits are known".
> > > > 
> > > > We don't commonly add a comment every time a define is used only one
> > > > time.
> > > > So what's the benefits here?
> > > > 
> > > > In all honesty, such wording in the commit message was probably over the
> > > > top. I am thinking to replace the sentence with:
> > > > 
> > > > "Lastly, all the bits are now described as either set or cleared. This
> > > > allows us to check at pre-processing time the consistency of the initial
> > > > value."
> > 
> > This is even clearer, but I understood that part of the commit message
> > well enough even before. I have no complaints there. My suggestion for
> > an in-code comment is because the purpose of HSCTLR_CLEAR is not
> > immediately obvious looking at the code only.  The commit message is
> > fine. I think that a one-liner in the code to say that HSCTLR_CLEAR is
> > "only used at pre-processing time" would be good to have and beneficial
> > for code readability.
> 
> It is quite an odd comment as a lot of defines are only used for
> pre-processing it (i.e CONFIG_ or even macro generating function)... It is
> going to rot quickly but I can add it if you think it improves the code...

Yes, but this macro in particular is in the middle of other similarly
named macros that are actually used at runtime. This is why I would like
the comment. However, this is code readibility, and as you know it is a
bit subjective.


> > > > > Same here, you removed the reserved bits, and added the alignment
> > > > > check,
> > > > > same as for aarch32. If I got it right, it would be nice to add a
> > > > > statement like this to the commit message.
> > > > 
> > > > I don't see why "reserved bits" I dropped nor which alignment check I
> > > > added.
> > > > 
> > > > It would be extremely useful if you provide more details in your
> > > > review...
> > > > In this case, it would be the exact bits I dropped/added.
> > 
> > I looked at the full value of SCTLR_EL2_SET, it's 0x30c51838. I
> > copy/paste here the wcalc command for our own convenience:
> > 
> > wcalc -h
> > '(1<<4)|(1<<5)|(1<<11)|(1<<16)|(1<<18)|(1<<22)|(1<<23)|(1<<28)|(1<<29)|(1<<3)|(1<<12)'
> > 
> > HSCTLR_BASE is 0x30c51878. The difference is bit 6 which is RES0. It
> > looks like I was wrong about the alignment check.
> 
> This was mentioned in the commit message:
> 
> "The current value of HSCTLR_BASE for Arm64 is pretty wrong. It would
> actually turn on SCTLR_EL2.nAA (bit 6) on hardware implementing
> ARMv8.4-LSE."

Good, it all checks out then.

 
> > > > > 
> > > > > 
> > > > > > +#define SCTLR_EL2_CLEAR (SCTLR_EL2_RES0     | SCTLR_Axx_ELx_M   |\
> > > > > > +                         SCTLR_Axx_ELx_A    | SCTLR_Axx_ELx_C   |\
> > > > > > +                         SCTLR_Axx_ELx_WXN  | SCTLR_Axx_ELx_EE)
> > > > > > +
> > > > > > +#if (SCTLR_EL2_SET ^ SCTLR_EL2_CLEAR) != 0xffffffffffffffffUL
> > > > > > +#error "Inconsistent SCTLR_EL2 set/clear bits"
> > > > > > +#endif
> > > > > > +
> > > > > > +#endif
> > > > > >    /* HCR Hyp Configuration Register */
> > > > > >    #define HCR_RW          (_AC(1,UL)<<31) /* Register Width, ARM64
> > > > > > only
> > > > > > */
> 
> Cheers,
> 
> -- 
> Julien Grall
> _______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel