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

[Julien Grall <julien@xxxxxxx>]

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

> On 07.06.2021 15:31, Julien Grall wrote:
> > Hi,
> >
> > 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 
> > > SPSR_FIQ.
> > > No need to modify the assembly code becuase the accesses were based on 64bit
> > s/becuase/because/
> 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.
> > > Increment XEN_DOMCTL_INTERFACE_VERSION.
> > >
> > > 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)
> > >            PSR_IRQ_MASK | PSR_DBG_MASK;
> > >        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,
> > >        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 
> > > *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.
> > >    #else
> > >        printk("PC:     %08"PRIx32, regs->pc);
> > >        if ( !guest_mode )
> > >            printk(" %pS", _p(regs->pc));
> > >        printk("\n");
> > > -#endif
> > >        printk("CPSR:   %08"PRIx32" MODE:%s\n", regs->cpsr,
> > >               mode_string(regs->cpsr));
> > > +#endif
> > [...]
> >
> > > diff --git a/xen/include/asm-arm/arm64/processor.h 
> > > b/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.
Cheers,

--
Julien Grall