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
