[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3 10/10] arm64: Change type of hsr, cpsr, spsr_el1 to uint64_t

On 01/07/2021 09:19, Michal Orzel wrote:
Hi Julien,

Hi Michal,

On 07.06.2021 15:31, Julien Grall wrote:

On 05/05/2021 08:43, Michal Orzel wrote:
AArch64 registers are 64bit whereas AArch32 registers
are 32bit or 64bit. MSR/MRS are expecting 64bit values thus
we should get rid of helpers READ/WRITE_SYSREG32
in favour of using READ/WRITE_SYSREG.
We should also use register_t type when reading sysregs
which can correspond to uint64_t or uint32_t.
Even though many AArch64 registers have upper 32bit reserved
it does not mean that they can't be widen in the future.

Modify type of hsr, cpsr, spsr_el1 to uint64_t.
Previously we relied on the padding after SPSR_EL1.
As we removed the padding, modify the union to be 64bit so we don't corrupt 
No need to modify the assembly code becuase the accesses were based on 64bit


Will do.
registers as there was a 32bit padding after SPSR_EL1.

Remove 32bit padding in cpu_user_regs before spsr_fiq
as it is no longer needed due to upper union being 64bit now.
Add 64bit padding in cpu_user_regs before spsr_el1
because offset of spsr_el1 must be a multiple of 8.

Change type of cpsr to uint64_t in the public outside interface
"public/arch-arm.h" to allow ABI compatibility between 32bit and 64bit.

Change type of cpsr to uint64_t in the public outside interface
"public/vm_event.h" to allow ABI compatibility between 32bit and 64bit.

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index e7384381cc..c8f9773566 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -546,7 +546,7 @@ void inject_undef64_exception(struct cpu_user_regs *regs, 
int instr_len)
       regs->pc = handler;
   -    WRITE_SYSREG32(esr.bits, ESR_EL1);
+    WRITE_SYSREG(esr.bits, ESR_EL1);
     /* Inject an abort exception into a 64 bit guest */
@@ -580,7 +580,7 @@ static void inject_abt64_exception(struct cpu_user_regs 
       regs->pc = handler;
         WRITE_SYSREG(addr, FAR_EL1);
-    WRITE_SYSREG32(esr.bits, ESR_EL1);
+    WRITE_SYSREG(esr.bits, ESR_EL1);
     static void inject_dabt64_exception(struct cpu_user_regs *regs,
@@ -717,7 +717,7 @@ struct reg_ctxt {
       uint64_t vttbr_el2;
   -static const char *mode_string(uint32_t cpsr)
+static const char *mode_string(register_t cpsr)
       uint32_t mode;
       static const char *mode_strings[] = {
@@ -756,14 +756,16 @@ static void show_registers_32(const struct cpu_user_regs 
   #ifdef CONFIG_ARM_64
       BUG_ON( ! (regs->cpsr & PSR_MODE_BIT) );
       printk("PC:     %08"PRIx32"\n", regs->pc32);
+    printk("CPSR:   %016"PRIx64" MODE:%s\n", regs->cpsr,
+           mode_string(regs->cpsr));
Why do you now need to duplicate this line? Can't we use PRIregister you did 
everywhere else a register is printed?

Indeed we do not need. It was needed at the beginning but I do not remember 
why. I will fix it.
       printk("PC:     %08"PRIx32, regs->pc);
       if ( !guest_mode )
           printk(" %pS", _p(regs->pc));
       printk("CPSR:   %08"PRIx32" MODE:%s\n", regs->cpsr,


diff --git a/xen/include/asm-arm/arm64/processor.h 
index 81dfc5e615..0e86079cbb 100644
--- a/xen/include/asm-arm/arm64/processor.h
+++ b/xen/include/asm-arm/arm64/processor.h
@@ -63,18 +63,19 @@ struct cpu_user_regs
         /* Return address and mode */
       __DECL_REG(pc,           pc32);             /* ELR_EL2 */
-    uint32_t cpsr;                              /* SPSR_EL2 */
-    uint32_t hsr;                               /* ESR_EL2 */
+    uint64_t cpsr;                              /* SPSR_EL2 */
+    uint64_t hsr;                               /* ESR_EL2 */
+    /* Offset of spsr_el1 must be a multiple of 8 */

I am guessing you are saying it should be 8-byte aligned, right? If so, the 
field before is a 64-bit value, therefore the offset should already be a 
multiple of 8. Did I miss anything?

Well, instead of 8 I should write 16.
We have 2 bug_on checks:
1) BUILD_BUG_ON((sizeof (struct cpu_user_regs)) & 0xf);
2) BUILD_BUG_ON((offsetof(struct cpu_user_regs, spsr_el1)) & 0xf);

Total size of cpu_user_regs without padding is 344 which does not pass the 
first check.
With the padding it is 352 and it passes the first check.

The offset without padding is 280 which does not pass the second check.
The offset with padding is 288 which passes the second check.

Ah yes, I forgot that the stack should always be 16-byte aligned.

So I should write:
"/* Offset of spsr_el1 must be a multiple of 16 bytes */"
I would suggest the following comment:

/* The kernel frame should be 16-byte aligned. */

So this would make clear that the 16-byte requirement is for the frame itself.


Julien Grall



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.