diff mbox series

[Xen-devel,04/20] xen/arm: Rework HSCTLR_BASE

Message ID 20190422164937.21350-5-julien.grall@arm.com
State New
Headers show
Series xen/arm: Clean-up & fixes in boot/mm code | expand

Commit Message

Julien Grall April 22, 2019, 4:49 p.m. UTC
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>
---
 xen/arch/arm/arm32/head.S       | 12 +---------
 xen/arch/arm/arm64/head.S       | 10 +-------
 xen/include/asm-arm/processor.h | 52 ++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 53 insertions(+), 21 deletions(-)

Comments

Andrii Anisov May 3, 2019, 3:56 p.m. UTC | #1
Hello Julien,

On 22.04.19 19:49, 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.
> 
> 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>
> ---
>   xen/arch/arm/arm32/head.S       | 12 +---------
>   xen/arch/arm/arm64/head.S       | 10 +-------
>   xen/include/asm-arm/processor.h | 52 ++++++++++++++++++++++++++++++++++++++++-
>   3 files changed, 53 insertions(+), 21 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 5ef7e5a2f3..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_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 1a143fb6a3..6dac500e40 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -122,6 +122,9 @@
>   #define SCTLR_A32_ELx_TE    _BITUL(30)
>   #define SCTLR_A32_ELx_FI    _BITUL(21)
>   
> +/* Common bits for SCTLR_ELx for Arm64 */
> +#define SCTLR_A64_ELx_SA    _BITUL(3)
> +
>   /* Common bits for SCTLR_ELx on all architectures */
>   #define SCTLR_Axx_ELx_EE    _BITUL(25)
>   #define SCTLR_Axx_ELx_WXN   _BITUL(19)
> @@ -130,7 +133,54 @@
>   #define SCTLR_Axx_ELx_A     _BITUL(1)
>   #define SCTLR_Axx_ELx_M     _BITUL(0)
>   
> -#define HSCTLR_BASE     _AC(0x30c51878,U)
> +#ifdef CONFIG_ARM_32
> +
> +#define HSCTLR_RES1     (_BITUL(3)  | _BITUL(4)  | _BITUL(5)  | _BITUL(6)  |\
> +                         _BITUL(11) | _BITUL(16) | _BITUL(18) | _BITUL(22) |\
> +                         _BITUL(23) | _BITUL(28) | _BITUL(29))
> +
> +#define HSCTLR_RES0     (_BITUL(7)  | _BITUL(8)  | _BITUL(9)  | _BITUL(10) |\
> +                         _BITUL(13) | _BITUL(14) | _BITUL(15) | _BITUL(17) |\
> +                         _BITUL(20) | _BITUL(24) | _BITUL(26) | _BITUL(27) |\
> +                         _BITUL(31))
> +
> +/* 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  (_BITUL(4)  | _BITUL(5)  | _BITUL(11) | _BITUL(16) |\
> +                         _BITUL(18) | _BITUL(22) | _BITUL(23) | _BITUL(28) |\
> +                         _BITUL(29))
> +
> +#define SCTLR_EL2_RES0  (_BITUL(6)  | _BITUL(7)  | _BITUL(8)  | _BITUL(9)  |\
> +                         _BITUL(10) | _BITUL(13) | _BITUL(14) | _BITUL(15) |\
> +                         _BITUL(17) | _BITUL(20) | _BITUL(21) | _BITUL(24) |\
> +                         _BITUL(26) | _BITUL(27) | _BITUL(30) | _BITUL(31) |\
> +                         (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 */
> 

Resolution of the dispute with Jan about [PATCH 01/20] is required first.
Julien Grall May 3, 2019, 4:10 p.m. UTC | #2
Hi,

On 03/05/2019 16:56, Andrii Anisov wrote:
> On 22.04.19 19:49, Julien Grall wrote:
>>   /* HCR Hyp Configuration Register */
>>   #define HCR_RW          (_AC(1,UL)<<31) /* Register Width, ARM64 only */
>>
> 
> Resolution of the dispute with Jan about [PATCH 01/20] is required first.

I don't understand what is your "second". Does it mean you are happy with the 
idea of the patch but we should agree on the naming first?

Cheers,
Andrii Anisov May 3, 2019, 4:17 p.m. UTC | #3
On 03.05.19 19:10, Julien Grall wrote:
> I don't understand what is your "second". Does it mean you are happy with the idea of the patch but we should agree on the naming first?

Yes, I'm ok with the change.
Actually I like the part with HSCTLR_CLEAR as a compilation time guard.
But naming should be agreed first.
diff mbox series

Patch

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 5ef7e5a2f3..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_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 1a143fb6a3..6dac500e40 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -122,6 +122,9 @@ 
 #define SCTLR_A32_ELx_TE    _BITUL(30)
 #define SCTLR_A32_ELx_FI    _BITUL(21)
 
+/* Common bits for SCTLR_ELx for Arm64 */
+#define SCTLR_A64_ELx_SA    _BITUL(3)
+
 /* Common bits for SCTLR_ELx on all architectures */
 #define SCTLR_Axx_ELx_EE    _BITUL(25)
 #define SCTLR_Axx_ELx_WXN   _BITUL(19)
@@ -130,7 +133,54 @@ 
 #define SCTLR_Axx_ELx_A     _BITUL(1)
 #define SCTLR_Axx_ELx_M     _BITUL(0)
 
-#define HSCTLR_BASE     _AC(0x30c51878,U)
+#ifdef CONFIG_ARM_32
+
+#define HSCTLR_RES1     (_BITUL(3)  | _BITUL(4)  | _BITUL(5)  | _BITUL(6)  |\
+                         _BITUL(11) | _BITUL(16) | _BITUL(18) | _BITUL(22) |\
+                         _BITUL(23) | _BITUL(28) | _BITUL(29))
+
+#define HSCTLR_RES0     (_BITUL(7)  | _BITUL(8)  | _BITUL(9)  | _BITUL(10) |\
+                         _BITUL(13) | _BITUL(14) | _BITUL(15) | _BITUL(17) |\
+                         _BITUL(20) | _BITUL(24) | _BITUL(26) | _BITUL(27) |\
+                         _BITUL(31))
+
+/* 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  (_BITUL(4)  | _BITUL(5)  | _BITUL(11) | _BITUL(16) |\
+                         _BITUL(18) | _BITUL(22) | _BITUL(23) | _BITUL(28) |\
+                         _BITUL(29))
+
+#define SCTLR_EL2_RES0  (_BITUL(6)  | _BITUL(7)  | _BITUL(8)  | _BITUL(9)  |\
+                         _BITUL(10) | _BITUL(13) | _BITUL(14) | _BITUL(15) |\
+                         _BITUL(17) | _BITUL(20) | _BITUL(21) | _BITUL(24) |\
+                         _BITUL(26) | _BITUL(27) | _BITUL(30) | _BITUL(31) |\
+                         (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 */