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

[Julien Grall <julien.grall@xxxxxxx>]

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