Re: [PATCH] xen/arm64: Zero the top 32 bits of gp registers on entry...

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 07/12/2021 09:55, Jan Beulich wrote:
> > > > --- a/xen/arch/arm/arm64/entry.S
> > > > +++ b/xen/arch/arm/arm64/entry.S
> > > > @@ -109,8 +109,16 @@
> > > >     * If 0, we rely on the on x0/x1 to have been saved at the correct
> > > >     * position on the stack before.
> > > >     */
> > > > -        .macro  entry, hyp, compat, save_x0_x1=1
> > > > +        .macro  entry, hyp, compat=0, save_x0_x1=1
> > > >            sub     sp, sp, #(UREGS_SPSR_el1 - UREGS_LR) /* CPSR, PC, SP, LR */
> > > > +
> > > > +        /* Zero the upper 32 bits of the registers when switching to AArch32 */
> > > > +        .if \compat == 1      /* AArch32 mode */
> > > > +        .irp 
> > > > nr,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29
> > > > +        mov w\nr, w\nr
> > > > +        .endr
> > > > +        .endif
> > > So Jan mentioned, the x0/x1 may have already been saved. So you may need to 
> > > fetch them from the stack and then clobber the top 32-bit.
> > So I would do the following:
> > -fetch x0/x1 from the stack
> > -clobber them
> > -store them again on the stack
> >
> > /*
> >   * Zero the upper 32 bits of the gp registers when switching
> >   * from AArch32.
> >   */
> > .if \compat == 1      /* AArch32 mode */
> >
> > /* x0/x1 have already been saved so fetch them to zero top 32 bits */
> > .if \save_x0_x1 == 0
> > ldp     x0, x1, [sp], #(UREGS_kernel_sizeof - UREGS_X0)
> > .endif
> >
> > .irp 
> > nr,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29
> > mov w\nr, w\nr
> > .endr
> >
> > .if \save_x0_x1 == 0
> > stp     x0, x1, [sp, #-(UREGS_kernel_sizeof - UREGS_X0)]
> > .endif
> >
> > .endif
> Wouldn't it be more efficient to store 32 bits of zero each into the
> high halves of the respective stack slots? Afaict same code size, but
> less memory / cache traffic.
It would indeed be more efficient.

> Plus it would avoid the latent issue of
> a user of the macro actually expecting the two registers to retain
> their values across the macro invocation.
While this is not explicitely written, a caller cannot expect the 
registers to be preserved by this macro.
> I'm also puzzled by the two different memory addressing forms, but I
> can easily see that I may be lacking enough Arm knowledge there.
I agree this is quite puzzling. The first one would update 'sp' after 
loading the register but I don't quite understand why it is necessary.
Assuming the this is happening before the instruction 'sub sp, sp, ...', 
then we should only need to load/store from sp with an offset (i.e. the 
second form).
Cheers,

--
Julien Grall