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@xxxxxxx>
> > > ---
> > > 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
