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.gr...@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
