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

[Julien Grall <julien.grall@xxxxxxx>]

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@xxxxxxx>
> >
> > ---
> >      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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel