Re: [kvm-unit-tests PATCH v2 19/23] arm64: Use code from the gnu-efi when booting with EFI

[Ricardo Koller <ricarkol@google.com>]

Ni Nikos,

On Mon, Jun 27, 2022 at 06:10:20PM +0100, Nikos Nikoleris wrote:
> Hi Ricardo,
> 
> Thanks for this, let me go through the idea I had. Please let me know if I
> am missing something.
> 
> On 21/06/2022 23:32, Ricardo Koller wrote:
> > On Fri, May 06, 2022 at 09:56:01PM +0100, Nikos Nikoleris wrote:
> > > arm/efi/crt0-efi-aarch64.S defines the header and the handover
> > > sequence from EFI to a efi_main. This change includes the whole file
> > > in arm/cstart64.S when we compile with EFI support.
> > > 
> > > In addition, we change the handover code in arm/efi/crt0-efi-aarch64.S
> > > to align the stack pointer. This alignment is necessary because we
> > > make assumptions about cpu0's stack alignment and most importantly we
> > > place its thread_info at the bottom of this stack.
> > > 
> > > Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
> > > ---
> > >   arm/cstart64.S             |  6 ++++++
> > >   arm/efi/crt0-efi-aarch64.S | 17 +++++++++++++++--
> > >   2 files changed, 21 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arm/cstart64.S b/arm/cstart64.S
> > > index 55b41ea..08cf02f 100644
> > > --- a/arm/cstart64.S
> > > +++ b/arm/cstart64.S
> > > @@ -15,6 +15,10 @@
> > >   #include <asm/thread_info.h>
> > >   #include <asm/sysreg.h>
> > > +#ifdef CONFIG_EFI
> > > +#include "efi/crt0-efi-aarch64.S"
> > > +#else
> > > +
> > >   .macro zero_range, tmp1, tmp2
> > >   9998:	cmp	\tmp1, \tmp2
> > >   	b.eq	9997f
> > > @@ -107,6 +111,8 @@ start:
> > >   	bl	exit
> > >   	b	halt
> > > +#endif
> > > +
> > >   .text
> > >   /*
> > > diff --git a/arm/efi/crt0-efi-aarch64.S b/arm/efi/crt0-efi-aarch64.S
> > > index d50e78d..11a062d 100644
> > > --- a/arm/efi/crt0-efi-aarch64.S
> > > +++ b/arm/efi/crt0-efi-aarch64.S
> > > @@ -111,10 +111,19 @@ section_table:
> > >   	.align		12
> > >   _start:
> > > -	stp		x29, x30, [sp, #-32]!
> > > +	stp		x29, x30, [sp, #-16]!
> > 
> > Is this and the "ldp x29, x30, [sp], #16" change below needed?
> > why is #-32 not good?
> > 
> 
> The stack is full-descending. Here we make space for x29 and x30 in the
> stack (16bytes) and save the two registers
> 

Got it, was thinking mainly in terms of optimizing for less changes.
But I see your point; plus it makes the restoring simpler.

> > > +
> > > +	// Align sp; this is necessary due to way we store cpu0's thread_info
> > 
> > /* */ comment style
> > 
> 
> ack
> 
> > >   	mov		x29, sp
> > > +	and		x29, x29, #THREAD_MASK
> > > +	mov		x30, sp
> > > +	mov		sp, x29
> > > +	str		x30, [sp, #-32]!
> > > +
> 
> Here we're making space in the stack for the old sp (x30), x0 and x1 but we
> have to also ensure that the sp is aligned (32bytes). The we store x30.
> 
> (As a side note, I could also change this to
> 
> +	str		x30, [sp, #-16]!
> 
> and change the next stp to do pre-incrementing mode. This might make things
> simpler.)

Definitely, it would look like two regular pushes.

> 
> > > +	mov             x29, sp
> > >   	stp		x0, x1, [sp, #16]
> > > +
> 
> Here, we use the space we made before to store x0 and x1.
> 
> I think, the stack now should look like:
> 
>        |   ...  |
>        |   x30  |
>        |   x29  |

possibly some more extra space due to #THREAD_MASK

>        |   x1   |
>        |   x0   |
>        |   pad  |
> sp ->  | old_sp |

Thanks for this!

> 
> 
> > >   	mov		x2, x0
> > >   	mov		x3, x1
> > >   	adr		x0, ImageBase
> > > @@ -126,5 +135,9 @@ _start:
> > >   	ldp		x0, x1, [sp, #16]
> > >   	bl		efi_main
> > > -0:	ldp		x29, x30, [sp], #32
> > > +	// Restore sp
> > 
> > /* */ comment style
> 
> ack
> 
> > 
> > > +	ldr		x30, [sp]
> 
> I think this should have been:
> 
> +	ldr		x30, [sp], #32
> 
> Restore x30 from the current sp and free up space in the stack (all
> 32bytes).
> 

Now I get it. That's not needed actually, as you are restoring sp from
old_sp below to this address:

          |   ...  |
          |   x30  |
sp ->     |   x29  |

(old_sp was the sp after pushing x29,x30)

> > 
> > I'm not able to understand this. Is this ldr restoring the value pushed
> > with "str x30, [sp, #-32]!" above? in that case, shouldn't this be at
> > [sp - 32]? But, given that this code is unreachable when efi_main is
> > called, do you even need to restore the sp?
> > 
> > > +	mov             sp, x30
> > > +
> > > +0:	ldp		x29, x30, [sp], #16
> 
> Then, this restores x29 and x30 and frees up the the corresponding space in
> the stack.
> 
> 
> I am not sure we shouldn't get to this point and I wanted to properly save
> and restore the register state. I haven't really found what's the right/best
> way to exit from an EFI app and I wanted to allow for graceful return from
> this point. But I am happy to change all this.

Yes, let's keep it just in case.

> 
> Thanks,
> 
> Nikos
> 
> > >   	ret
> > > -- 
> > > 2.25.1
> > > 

Thanks!
Ricardo