| Message ID | 20220506205605.359830-20-nikos.nikoleris@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | EFI and ACPI support for arm64 | expand |
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? > + > + // Align sp; this is necessary due to way we store cpu0's thread_info /* */ comment style > mov x29, sp > + and x29, x29, #THREAD_MASK > + mov x30, sp > + mov sp, x29 > + str x30, [sp, #-32]! > + > + mov x29, sp > > stp x0, x1, [sp, #16] > + > 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 > + ldr x30, [sp] 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 > ret > -- > 2.25.1 >
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 >> + >> + // 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.) >> + 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 | | x1 | | x0 | | pad | sp -> | old_sp | >> 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). > > 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. Thanks, Nikos >> ret >> -- >> 2.25.1 >>
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
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]! + + // Align sp; this is necessary due to way we store cpu0's thread_info mov x29, sp + and x29, x29, #THREAD_MASK + mov x30, sp + mov sp, x29 + str x30, [sp, #-32]! + + mov x29, sp stp x0, x1, [sp, #16] + 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 + ldr x30, [sp] + mov sp, x30 + +0: ldp x29, x30, [sp], #16 ret
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(-)