| Message ID | 20210407185918.371983-2-drjones@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm/arm64: Prepare for target-efi | expand |
On 07/04/2021 19:59, Andrew Jones wrote: > Move secondary_entry helper functions out of .init and into .text, > since secondary_entry isn't run at "init" time. > > Signed-off-by: Andrew Jones <drjones@redhat.com> > --- > arm/cstart.S | 62 +++++++++++++++++++++++++++----------------------- > arm/cstart64.S | 22 +++++++++++------- > 2 files changed, 48 insertions(+), 36 deletions(-) > > diff --git a/arm/cstart.S b/arm/cstart.S > index d88a98362940..653ab1e8a141 100644 > --- a/arm/cstart.S > +++ b/arm/cstart.S > @@ -96,32 +96,7 @@ start: > bl exit > b halt > > - > -.macro set_mode_stack mode, stack > - add \stack, #S_FRAME_SIZE > - msr cpsr_c, #(\mode | PSR_I_BIT | PSR_F_BIT) > - isb > - mov sp, \stack > -.endm > - > -exceptions_init: > - mrc p15, 0, r2, c1, c0, 0 @ read SCTLR > - bic r2, #CR_V @ SCTLR.V := 0 > - mcr p15, 0, r2, c1, c0, 0 @ write SCTLR > - ldr r2, =vector_table > - mcr p15, 0, r2, c12, c0, 0 @ write VBAR > - > - mrs r2, cpsr > - > - /* first frame reserved for svc mode */ > - set_mode_stack UND_MODE, r0 > - set_mode_stack ABT_MODE, r0 > - set_mode_stack IRQ_MODE, r0 > - set_mode_stack FIQ_MODE, r0 > - > - msr cpsr_cxsf, r2 @ back to svc mode > - isb > - mov pc, lr > +.text > > enable_vfp: > /* Enable full access to CP10 and CP11: */ > @@ -133,8 +108,6 @@ enable_vfp: > vmsr fpexc, r0 > mov pc, lr > > -.text > - > .global get_mmu_off > get_mmu_off: > ldr r0, =auxinfo > @@ -235,6 +208,39 @@ asm_mmu_disable: > > mov pc, lr > > +/* > + * Vectors > + */ > + > +.macro set_mode_stack mode, stack > + add \stack, #S_FRAME_SIZE > + msr cpsr_c, #(\mode | PSR_I_BIT | PSR_F_BIT) > + isb > + mov sp, \stack > +.endm > + > +exceptions_init: > + mrc p15, 0, r2, c1, c0, 0 @ read SCTLR > + bic r2, #CR_V @ SCTLR.V := 0 > + mcr p15, 0, r2, c1, c0, 0 @ write SCTLR > + ldr r2, =vector_table > + mcr p15, 0, r2, c12, c0, 0 @ write VBAR > + > + mrs r2, cpsr > + > + /* > + * Input r0 is the stack top, which is the exception stacks base Minor, feel free to ignore - wouldn't it be better to put this comment at the start of this routine to inform the caller? I am not sure about the practical implications of having an .init section but in any case, moving secondary_entry helper functions to .text seems sensible. Reviewed-by Nikos Nikoleris <nikos.nikoleris@arm.com> Thanks, Nikos > + * The first frame is reserved for svc mode > + */ > + set_mode_stack UND_MODE, r0 > + set_mode_stack ABT_MODE, r0 > + set_mode_stack IRQ_MODE, r0 > + set_mode_stack FIQ_MODE, r0 > + > + msr cpsr_cxsf, r2 @ back to svc mode > + isb > + mov pc, lr > + > /* > * Vector stubs > * Simplified version of the Linux kernel implementation > diff --git a/arm/cstart64.S b/arm/cstart64.S > index 0a85338bcdae..d39cf4dfb99c 100644 > --- a/arm/cstart64.S > +++ b/arm/cstart64.S > @@ -89,10 +89,12 @@ start: > msr cpacr_el1, x4 > > /* set up exception handling */ > + mov x4, x0 // x0 is the addr of the dtb > bl exceptions_init > > /* complete setup */ > - bl setup // x0 is the addr of the dtb > + mov x0, x4 // restore the addr of the dtb > + bl setup > bl get_mmu_off > cbnz x0, 1f > bl setup_vm > @@ -109,13 +111,6 @@ start: > bl exit > b halt > > -exceptions_init: > - adrp x4, vector_table > - add x4, x4, :lo12:vector_table > - msr vbar_el1, x4 > - isb > - ret > - > .text > > .globl get_mmu_off > @@ -251,6 +246,17 @@ asm_mmu_disable: > > /* > * Vectors > + */ > + > +exceptions_init: > + adrp x0, vector_table > + add x0, x0, :lo12:vector_table > + msr vbar_el1, x0 > + isb > + ret > + > +/* > + * Vector stubs > * Adapted from arch/arm64/kernel/entry.S > */ > .macro vector_stub, name, vec >
On Fri, Apr 09, 2021 at 06:18:05PM +0100, Nikos Nikoleris wrote: > On 07/04/2021 19:59, Andrew Jones wrote: > > +exceptions_init: > > + mrc p15, 0, r2, c1, c0, 0 @ read SCTLR > > + bic r2, #CR_V @ SCTLR.V := 0 > > + mcr p15, 0, r2, c1, c0, 0 @ write SCTLR > > + ldr r2, =vector_table > > + mcr p15, 0, r2, c12, c0, 0 @ write VBAR > > + > > + mrs r2, cpsr > > + > > + /* > > + * Input r0 is the stack top, which is the exception stacks base > > Minor, feel free to ignore - wouldn't it be better to put this comment at > the start of this routine to inform the caller? Yes, that's a good suggestion. Will do for v2. > > I am not sure about the practical implications of having an .init section > but in any case, moving secondary_entry helper functions to .text seems > sensible. > > Reviewed-by Nikos Nikoleris <nikos.nikoleris@arm.com> Thanks, drew
Hi Drew, On 4/7/21 7:59 PM, Andrew Jones wrote: > Move secondary_entry helper functions out of .init and into .text, > since secondary_entry isn't run at "init" time. The tests aren't loaded using the loader, so as far as I can tell the reason for having an .init section is to make sure the code from the start label is put at offset 0 in the test binary. As long as the start label is kept at the beginning of the .init section, and the loader script places the section first, I don't see any issues with this change. The only hypothetical problem that I can think of is that the code from .init calls code from .text, and if the text section grows very large we might end up with a PC offset larger than what can be encoded in the BL instruction. That's unlikely to happen (the offset is 16MB for arm and 64MB for arm64), and the .init code already calls other functions (like setup) which are in .text, so we would have this problem regardless of this change. And the compiler will emit an error if that happens. > > Signed-off-by: Andrew Jones <drjones@redhat.com> > --- > arm/cstart.S | 62 +++++++++++++++++++++++++++----------------------- > arm/cstart64.S | 22 +++++++++++------- > 2 files changed, 48 insertions(+), 36 deletions(-) > > diff --git a/arm/cstart.S b/arm/cstart.S > index d88a98362940..653ab1e8a141 100644 > --- a/arm/cstart.S > +++ b/arm/cstart.S > @@ -96,32 +96,7 @@ start: > bl exit > b halt > > - > -.macro set_mode_stack mode, stack > - add \stack, #S_FRAME_SIZE > - msr cpsr_c, #(\mode | PSR_I_BIT | PSR_F_BIT) > - isb > - mov sp, \stack > -.endm > - > -exceptions_init: > - mrc p15, 0, r2, c1, c0, 0 @ read SCTLR > - bic r2, #CR_V @ SCTLR.V := 0 > - mcr p15, 0, r2, c1, c0, 0 @ write SCTLR > - ldr r2, =vector_table > - mcr p15, 0, r2, c12, c0, 0 @ write VBAR > - > - mrs r2, cpsr > - > - /* first frame reserved for svc mode */ > - set_mode_stack UND_MODE, r0 > - set_mode_stack ABT_MODE, r0 > - set_mode_stack IRQ_MODE, r0 > - set_mode_stack FIQ_MODE, r0 > - > - msr cpsr_cxsf, r2 @ back to svc mode > - isb > - mov pc, lr > +.text Hm... now we've moved enable_vfp from .init to .text, and enable_vfp *is* called from .init code, which doesn't fully match up with the commit message. Is the actual reason for this change that the linker script for EFI will discard the .init section? Maybe it's worth mentioning that in the commit message, because it will explain this change better. Or is it to align arm with arm64, where only start is in the .init section? > > enable_vfp: > /* Enable full access to CP10 and CP11: */ > @@ -133,8 +108,6 @@ enable_vfp: > vmsr fpexc, r0 > mov pc, lr > > -.text > - > .global get_mmu_off > get_mmu_off: > ldr r0, =auxinfo > @@ -235,6 +208,39 @@ asm_mmu_disable: > > mov pc, lr > > +/* > + * Vectors > + */ > + > +.macro set_mode_stack mode, stack > + add \stack, #S_FRAME_SIZE > + msr cpsr_c, #(\mode | PSR_I_BIT | PSR_F_BIT) > + isb > + mov sp, \stack > +.endm > + > +exceptions_init: > + mrc p15, 0, r2, c1, c0, 0 @ read SCTLR > + bic r2, #CR_V @ SCTLR.V := 0 > + mcr p15, 0, r2, c1, c0, 0 @ write SCTLR > + ldr r2, =vector_table > + mcr p15, 0, r2, c12, c0, 0 @ write VBAR > + > + mrs r2, cpsr > + > + /* > + * Input r0 is the stack top, which is the exception stacks base > + * The first frame is reserved for svc mode > + */ > + set_mode_stack UND_MODE, r0 > + set_mode_stack ABT_MODE, r0 > + set_mode_stack IRQ_MODE, r0 > + set_mode_stack FIQ_MODE, r0 > + > + msr cpsr_cxsf, r2 @ back to svc mode > + isb > + mov pc, lr > + > /* > * Vector stubs > * Simplified version of the Linux kernel implementation > diff --git a/arm/cstart64.S b/arm/cstart64.S > index 0a85338bcdae..d39cf4dfb99c 100644 > --- a/arm/cstart64.S > +++ b/arm/cstart64.S > @@ -89,10 +89,12 @@ start: > msr cpacr_el1, x4 > > /* set up exception handling */ > + mov x4, x0 // x0 is the addr of the dtb I suppose changing exceptions_init to use x0 as a scratch register instead of x4 makes some sense if you look at it from the perspective of it being called from secondary_entry, where all the functions use x0 as a scratch register. But it's still called from start, where using x4 as a scratch register is preferred because of the kernel boot protocol (x0-x3 are reserved). Is there an actual bug that this is supposed to fix (I looked for it and couldn't figure it out) or is it just a cosmetic change? Thanks, Alex > bl exceptions_init > > /* complete setup */ > - bl setup // x0 is the addr of the dtb > + mov x0, x4 // restore the addr of the dtb > + bl setup > bl get_mmu_off > cbnz x0, 1f > bl setup_vm > @@ -109,13 +111,6 @@ start: > bl exit > b halt > > -exceptions_init: > - adrp x4, vector_table > - add x4, x4, :lo12:vector_table > - msr vbar_el1, x4 > - isb > - ret > - > .text > > .globl get_mmu_off > @@ -251,6 +246,17 @@ asm_mmu_disable: > > /* > * Vectors > + */ > + > +exceptions_init: > + adrp x0, vector_table > + add x0, x0, :lo12:vector_table > + msr vbar_el1, x0 > + isb > + ret > + > +/* > + * Vector stubs > * Adapted from arch/arm64/kernel/entry.S > */ > .macro vector_stub, name, vec
On Tue, Apr 13, 2021 at 05:34:24PM +0100, Alexandru Elisei wrote: > Hi Drew, > > On 4/7/21 7:59 PM, Andrew Jones wrote: > > Move secondary_entry helper functions out of .init and into .text, > > since secondary_entry isn't run at "init" time. > > The tests aren't loaded using the loader, so as far as I can tell the reason for > having an .init section is to make sure the code from the start label is put at > offset 0 in the test binary. As long as the start label is kept at the beginning > of the .init section, and the loader script places the section first, I don't see > any issues with this change. > > The only hypothetical problem that I can think of is that the code from .init > calls code from .text, and if the text section grows very large we might end up > with a PC offset larger than what can be encoded in the BL instruction. That's > unlikely to happen (the offset is 16MB for arm and 64MB for arm64), and the .init > code already calls other functions (like setup) which are in .text, so we would > have this problem regardless of this change. And the compiler will emit an error > if that happens. > > > > > Signed-off-by: Andrew Jones <drjones@redhat.com> > > --- > > arm/cstart.S | 62 +++++++++++++++++++++++++++----------------------- > > arm/cstart64.S | 22 +++++++++++------- > > 2 files changed, 48 insertions(+), 36 deletions(-) > > > > diff --git a/arm/cstart.S b/arm/cstart.S > > index d88a98362940..653ab1e8a141 100644 > > --- a/arm/cstart.S > > +++ b/arm/cstart.S > > @@ -96,32 +96,7 @@ start: > > bl exit > > b halt > > > > - > > -.macro set_mode_stack mode, stack > > - add \stack, #S_FRAME_SIZE > > - msr cpsr_c, #(\mode | PSR_I_BIT | PSR_F_BIT) > > - isb > > - mov sp, \stack > > -.endm > > - > > -exceptions_init: > > - mrc p15, 0, r2, c1, c0, 0 @ read SCTLR > > - bic r2, #CR_V @ SCTLR.V := 0 > > - mcr p15, 0, r2, c1, c0, 0 @ write SCTLR > > - ldr r2, =vector_table > > - mcr p15, 0, r2, c12, c0, 0 @ write VBAR > > - > > - mrs r2, cpsr > > - > > - /* first frame reserved for svc mode */ > > - set_mode_stack UND_MODE, r0 > > - set_mode_stack ABT_MODE, r0 > > - set_mode_stack IRQ_MODE, r0 > > - set_mode_stack FIQ_MODE, r0 > > - > > - msr cpsr_cxsf, r2 @ back to svc mode > > - isb > > - mov pc, lr > > +.text > > Hm... now we've moved enable_vfp from .init to .text, and enable_vfp *is* called > from .init code, which doesn't fully match up with the commit message. Is the > actual reason for this change that the linker script for EFI will discard the > .init section? Maybe it's worth mentioning that in the commit message, because it > will explain this change better. Right, the .init section may not exist when linking with other linker scripts. I'll make the commit message more clear. > Or is it to align arm with arm64, where only > start is in the .init section? > > > > > enable_vfp: > > /* Enable full access to CP10 and CP11: */ > > @@ -133,8 +108,6 @@ enable_vfp: > > vmsr fpexc, r0 > > mov pc, lr > > > > -.text > > - > > .global get_mmu_off > > get_mmu_off: > > ldr r0, =auxinfo > > @@ -235,6 +208,39 @@ asm_mmu_disable: > > > > mov pc, lr > > > > +/* > > + * Vectors > > + */ > > + > > +.macro set_mode_stack mode, stack > > + add \stack, #S_FRAME_SIZE > > + msr cpsr_c, #(\mode | PSR_I_BIT | PSR_F_BIT) > > + isb > > + mov sp, \stack > > +.endm > > + > > +exceptions_init: > > + mrc p15, 0, r2, c1, c0, 0 @ read SCTLR > > + bic r2, #CR_V @ SCTLR.V := 0 > > + mcr p15, 0, r2, c1, c0, 0 @ write SCTLR > > + ldr r2, =vector_table > > + mcr p15, 0, r2, c12, c0, 0 @ write VBAR > > + > > + mrs r2, cpsr > > + > > + /* > > + * Input r0 is the stack top, which is the exception stacks base > > + * The first frame is reserved for svc mode > > + */ > > + set_mode_stack UND_MODE, r0 > > + set_mode_stack ABT_MODE, r0 > > + set_mode_stack IRQ_MODE, r0 > > + set_mode_stack FIQ_MODE, r0 > > + > > + msr cpsr_cxsf, r2 @ back to svc mode > > + isb > > + mov pc, lr > > + > > /* > > * Vector stubs > > * Simplified version of the Linux kernel implementation > > diff --git a/arm/cstart64.S b/arm/cstart64.S > > index 0a85338bcdae..d39cf4dfb99c 100644 > > --- a/arm/cstart64.S > > +++ b/arm/cstart64.S > > @@ -89,10 +89,12 @@ start: > > msr cpacr_el1, x4 > > > > /* set up exception handling */ > > + mov x4, x0 // x0 is the addr of the dtb > > I suppose changing exceptions_init to use x0 as a scratch register instead of x4 > makes some sense if you look at it from the perspective of it being called from > secondary_entry, where all the functions use x0 as a scratch register. But it's > still called from start, where using x4 as a scratch register is preferred because > of the kernel boot protocol (x0-x3 are reserved). > > Is there an actual bug that this is supposed to fix (I looked for it and couldn't > figure it out) or is it just a cosmetic change? Now that exceptions_init isn't a private function of start (actually it hasn't been in a long time, considering secondary_entry calls it) I would like it to better conform to calling conventions. I guess I should have used x19 here instead of x4 to be 100% correct. Or, would you rather I just continue using x4 in exceptions_init in order to avoid the save/restore? Thanks, drew
Hi Drew, On 4/14/21 9:59 AM, Andrew Jones wrote: > On Tue, Apr 13, 2021 at 05:34:24PM +0100, Alexandru Elisei wrote: >> Hi Drew, >> >> On 4/7/21 7:59 PM, Andrew Jones wrote: >>> Move secondary_entry helper functions out of .init and into .text, >>> since secondary_entry isn't run at "init" time. >> The tests aren't loaded using the loader, so as far as I can tell the reason for >> having an .init section is to make sure the code from the start label is put at >> offset 0 in the test binary. As long as the start label is kept at the beginning >> of the .init section, and the loader script places the section first, I don't see >> any issues with this change. >> >> The only hypothetical problem that I can think of is that the code from .init >> calls code from .text, and if the text section grows very large we might end up >> with a PC offset larger than what can be encoded in the BL instruction. That's >> unlikely to happen (the offset is 16MB for arm and 64MB for arm64), and the .init >> code already calls other functions (like setup) which are in .text, so we would >> have this problem regardless of this change. And the compiler will emit an error >> if that happens. >> >>> Signed-off-by: Andrew Jones <drjones@redhat.com> >>> --- >>> arm/cstart.S | 62 +++++++++++++++++++++++++++----------------------- >>> arm/cstart64.S | 22 +++++++++++------- >>> 2 files changed, 48 insertions(+), 36 deletions(-) >>> >>> diff --git a/arm/cstart.S b/arm/cstart.S >>> index d88a98362940..653ab1e8a141 100644 >>> --- a/arm/cstart.S >>> +++ b/arm/cstart.S >>> @@ -96,32 +96,7 @@ start: >>> bl exit >>> b halt >>> >>> - >>> -.macro set_mode_stack mode, stack >>> - add \stack, #S_FRAME_SIZE >>> - msr cpsr_c, #(\mode | PSR_I_BIT | PSR_F_BIT) >>> - isb >>> - mov sp, \stack >>> -.endm >>> - >>> -exceptions_init: >>> - mrc p15, 0, r2, c1, c0, 0 @ read SCTLR >>> - bic r2, #CR_V @ SCTLR.V := 0 >>> - mcr p15, 0, r2, c1, c0, 0 @ write SCTLR >>> - ldr r2, =vector_table >>> - mcr p15, 0, r2, c12, c0, 0 @ write VBAR >>> - >>> - mrs r2, cpsr >>> - >>> - /* first frame reserved for svc mode */ >>> - set_mode_stack UND_MODE, r0 >>> - set_mode_stack ABT_MODE, r0 >>> - set_mode_stack IRQ_MODE, r0 >>> - set_mode_stack FIQ_MODE, r0 >>> - >>> - msr cpsr_cxsf, r2 @ back to svc mode >>> - isb >>> - mov pc, lr >>> +.text >> Hm... now we've moved enable_vfp from .init to .text, and enable_vfp *is* called >> from .init code, which doesn't fully match up with the commit message. Is the >> actual reason for this change that the linker script for EFI will discard the >> .init section? Maybe it's worth mentioning that in the commit message, because it >> will explain this change better. > Right, the .init section may not exist when linking with other linker > scripts. I'll make the commit message more clear. > >> Or is it to align arm with arm64, where only >> start is in the .init section? >> >>> >>> enable_vfp: >>> /* Enable full access to CP10 and CP11: */ >>> @@ -133,8 +108,6 @@ enable_vfp: >>> vmsr fpexc, r0 >>> mov pc, lr >>> >>> -.text >>> - >>> .global get_mmu_off >>> get_mmu_off: >>> ldr r0, =auxinfo >>> @@ -235,6 +208,39 @@ asm_mmu_disable: >>> >>> mov pc, lr >>> >>> +/* >>> + * Vectors >>> + */ >>> + >>> +.macro set_mode_stack mode, stack >>> + add \stack, #S_FRAME_SIZE >>> + msr cpsr_c, #(\mode | PSR_I_BIT | PSR_F_BIT) >>> + isb >>> + mov sp, \stack >>> +.endm >>> + >>> +exceptions_init: >>> + mrc p15, 0, r2, c1, c0, 0 @ read SCTLR >>> + bic r2, #CR_V @ SCTLR.V := 0 >>> + mcr p15, 0, r2, c1, c0, 0 @ write SCTLR >>> + ldr r2, =vector_table >>> + mcr p15, 0, r2, c12, c0, 0 @ write VBAR >>> + >>> + mrs r2, cpsr >>> + >>> + /* >>> + * Input r0 is the stack top, which is the exception stacks base >>> + * The first frame is reserved for svc mode >>> + */ >>> + set_mode_stack UND_MODE, r0 >>> + set_mode_stack ABT_MODE, r0 >>> + set_mode_stack IRQ_MODE, r0 >>> + set_mode_stack FIQ_MODE, r0 >>> + >>> + msr cpsr_cxsf, r2 @ back to svc mode >>> + isb >>> + mov pc, lr >>> + >>> /* >>> * Vector stubs >>> * Simplified version of the Linux kernel implementation >>> diff --git a/arm/cstart64.S b/arm/cstart64.S >>> index 0a85338bcdae..d39cf4dfb99c 100644 >>> --- a/arm/cstart64.S >>> +++ b/arm/cstart64.S >>> @@ -89,10 +89,12 @@ start: >>> msr cpacr_el1, x4 >>> >>> /* set up exception handling */ >>> + mov x4, x0 // x0 is the addr of the dtb >> I suppose changing exceptions_init to use x0 as a scratch register instead of x4 >> makes some sense if you look at it from the perspective of it being called from >> secondary_entry, where all the functions use x0 as a scratch register. But it's >> still called from start, where using x4 as a scratch register is preferred because >> of the kernel boot protocol (x0-x3 are reserved). >> >> Is there an actual bug that this is supposed to fix (I looked for it and couldn't >> figure it out) or is it just a cosmetic change? > Now that exceptions_init isn't a private function of start (actually it > hasn't been in a long time, considering secondary_entry calls it) I would > like it to better conform to calling conventions. I guess I should have > used x19 here instead of x4 to be 100% correct. Or, would you rather I > just continue using x4 in exceptions_init in order to avoid the > save/restore? To be honest, for this patch, I think it would be best to leave exceptions_init unchanged: - We switch to using x0 like the rest of the code from secondary_entry, but because of that we need to save and restore the DTB address from x0 in start, so I don't think we've gained anything. - It makes the diff larger. - It runs the risk of introducing regressions (like all changes). Maybe this can be left for a separate patch that changes code called from C to follow aapcs64. Thanks, Alex
On Wed, Apr 14, 2021 at 04:15:10PM +0100, Alexandru Elisei wrote: > Hi Drew, > > On 4/14/21 9:59 AM, Andrew Jones wrote: > > On Tue, Apr 13, 2021 at 05:34:24PM +0100, Alexandru Elisei wrote: > >> Hi Drew, > >> > >> On 4/7/21 7:59 PM, Andrew Jones wrote: > >>> Move secondary_entry helper functions out of .init and into .text, > >>> since secondary_entry isn't run at "init" time. > >> The tests aren't loaded using the loader, so as far as I can tell the reason for > >> having an .init section is to make sure the code from the start label is put at > >> offset 0 in the test binary. As long as the start label is kept at the beginning > >> of the .init section, and the loader script places the section first, I don't see > >> any issues with this change. > >> > >> The only hypothetical problem that I can think of is that the code from .init > >> calls code from .text, and if the text section grows very large we might end up > >> with a PC offset larger than what can be encoded in the BL instruction. That's > >> unlikely to happen (the offset is 16MB for arm and 64MB for arm64), and the .init > >> code already calls other functions (like setup) which are in .text, so we would > >> have this problem regardless of this change. And the compiler will emit an error > >> if that happens. > >> > >>> Signed-off-by: Andrew Jones <drjones@redhat.com> > >>> --- > >>> arm/cstart.S | 62 +++++++++++++++++++++++++++----------------------- > >>> arm/cstart64.S | 22 +++++++++++------- > >>> 2 files changed, 48 insertions(+), 36 deletions(-) > >>> > >>> diff --git a/arm/cstart.S b/arm/cstart.S > >>> index d88a98362940..653ab1e8a141 100644 > >>> --- a/arm/cstart.S > >>> +++ b/arm/cstart.S > >>> @@ -96,32 +96,7 @@ start: > >>> bl exit > >>> b halt > >>> > >>> - > >>> -.macro set_mode_stack mode, stack > >>> - add \stack, #S_FRAME_SIZE > >>> - msr cpsr_c, #(\mode | PSR_I_BIT | PSR_F_BIT) > >>> - isb > >>> - mov sp, \stack > >>> -.endm > >>> - > >>> -exceptions_init: > >>> - mrc p15, 0, r2, c1, c0, 0 @ read SCTLR > >>> - bic r2, #CR_V @ SCTLR.V := 0 > >>> - mcr p15, 0, r2, c1, c0, 0 @ write SCTLR > >>> - ldr r2, =vector_table > >>> - mcr p15, 0, r2, c12, c0, 0 @ write VBAR > >>> - > >>> - mrs r2, cpsr > >>> - > >>> - /* first frame reserved for svc mode */ > >>> - set_mode_stack UND_MODE, r0 > >>> - set_mode_stack ABT_MODE, r0 > >>> - set_mode_stack IRQ_MODE, r0 > >>> - set_mode_stack FIQ_MODE, r0 > >>> - > >>> - msr cpsr_cxsf, r2 @ back to svc mode > >>> - isb > >>> - mov pc, lr > >>> +.text > >> Hm... now we've moved enable_vfp from .init to .text, and enable_vfp *is* called > >> from .init code, which doesn't fully match up with the commit message. Is the > >> actual reason for this change that the linker script for EFI will discard the > >> .init section? Maybe it's worth mentioning that in the commit message, because it > >> will explain this change better. > > Right, the .init section may not exist when linking with other linker > > scripts. I'll make the commit message more clear. > > > >> Or is it to align arm with arm64, where only > >> start is in the .init section? > >> > >>> > >>> enable_vfp: > >>> /* Enable full access to CP10 and CP11: */ > >>> @@ -133,8 +108,6 @@ enable_vfp: > >>> vmsr fpexc, r0 > >>> mov pc, lr > >>> > >>> -.text > >>> - > >>> .global get_mmu_off > >>> get_mmu_off: > >>> ldr r0, =auxinfo > >>> @@ -235,6 +208,39 @@ asm_mmu_disable: > >>> > >>> mov pc, lr > >>> > >>> +/* > >>> + * Vectors > >>> + */ > >>> + > >>> +.macro set_mode_stack mode, stack > >>> + add \stack, #S_FRAME_SIZE > >>> + msr cpsr_c, #(\mode | PSR_I_BIT | PSR_F_BIT) > >>> + isb > >>> + mov sp, \stack > >>> +.endm > >>> + > >>> +exceptions_init: > >>> + mrc p15, 0, r2, c1, c0, 0 @ read SCTLR > >>> + bic r2, #CR_V @ SCTLR.V := 0 > >>> + mcr p15, 0, r2, c1, c0, 0 @ write SCTLR > >>> + ldr r2, =vector_table > >>> + mcr p15, 0, r2, c12, c0, 0 @ write VBAR > >>> + > >>> + mrs r2, cpsr > >>> + > >>> + /* > >>> + * Input r0 is the stack top, which is the exception stacks base > >>> + * The first frame is reserved for svc mode > >>> + */ > >>> + set_mode_stack UND_MODE, r0 > >>> + set_mode_stack ABT_MODE, r0 > >>> + set_mode_stack IRQ_MODE, r0 > >>> + set_mode_stack FIQ_MODE, r0 > >>> + > >>> + msr cpsr_cxsf, r2 @ back to svc mode > >>> + isb > >>> + mov pc, lr > >>> + > >>> /* > >>> * Vector stubs > >>> * Simplified version of the Linux kernel implementation > >>> diff --git a/arm/cstart64.S b/arm/cstart64.S > >>> index 0a85338bcdae..d39cf4dfb99c 100644 > >>> --- a/arm/cstart64.S > >>> +++ b/arm/cstart64.S > >>> @@ -89,10 +89,12 @@ start: > >>> msr cpacr_el1, x4 > >>> > >>> /* set up exception handling */ > >>> + mov x4, x0 // x0 is the addr of the dtb > >> I suppose changing exceptions_init to use x0 as a scratch register instead of x4 > >> makes some sense if you look at it from the perspective of it being called from > >> secondary_entry, where all the functions use x0 as a scratch register. But it's > >> still called from start, where using x4 as a scratch register is preferred because > >> of the kernel boot protocol (x0-x3 are reserved). > >> > >> Is there an actual bug that this is supposed to fix (I looked for it and couldn't > >> figure it out) or is it just a cosmetic change? > > Now that exceptions_init isn't a private function of start (actually it > > hasn't been in a long time, considering secondary_entry calls it) I would > > like it to better conform to calling conventions. I guess I should have > > used x19 here instead of x4 to be 100% correct. Or, would you rather I > > just continue using x4 in exceptions_init in order to avoid the > > save/restore? > > To be honest, for this patch, I think it would be best to leave exceptions_init > unchanged: > > - We switch to using x0 like the rest of the code from secondary_entry, but > because of that we need to save and restore the DTB address from x0 in start, so I > don't think we've gained anything. > - It makes the diff larger. > - It runs the risk of introducing regressions (like all changes). > > Maybe this can be left for a separate patch that changes code called from C to > follow aapcs64. > OK, I'll switch it back to x4 and add a comment. Thanks, drew
diff --git a/arm/cstart.S b/arm/cstart.S index d88a98362940..653ab1e8a141 100644 --- a/arm/cstart.S +++ b/arm/cstart.S @@ -96,32 +96,7 @@ start: bl exit b halt - -.macro set_mode_stack mode, stack - add \stack, #S_FRAME_SIZE - msr cpsr_c, #(\mode | PSR_I_BIT | PSR_F_BIT) - isb - mov sp, \stack -.endm - -exceptions_init: - mrc p15, 0, r2, c1, c0, 0 @ read SCTLR - bic r2, #CR_V @ SCTLR.V := 0 - mcr p15, 0, r2, c1, c0, 0 @ write SCTLR - ldr r2, =vector_table - mcr p15, 0, r2, c12, c0, 0 @ write VBAR - - mrs r2, cpsr - - /* first frame reserved for svc mode */ - set_mode_stack UND_MODE, r0 - set_mode_stack ABT_MODE, r0 - set_mode_stack IRQ_MODE, r0 - set_mode_stack FIQ_MODE, r0 - - msr cpsr_cxsf, r2 @ back to svc mode - isb - mov pc, lr +.text enable_vfp: /* Enable full access to CP10 and CP11: */ @@ -133,8 +108,6 @@ enable_vfp: vmsr fpexc, r0 mov pc, lr -.text - .global get_mmu_off get_mmu_off: ldr r0, =auxinfo @@ -235,6 +208,39 @@ asm_mmu_disable: mov pc, lr +/* + * Vectors + */ + +.macro set_mode_stack mode, stack + add \stack, #S_FRAME_SIZE + msr cpsr_c, #(\mode | PSR_I_BIT | PSR_F_BIT) + isb + mov sp, \stack +.endm + +exceptions_init: + mrc p15, 0, r2, c1, c0, 0 @ read SCTLR + bic r2, #CR_V @ SCTLR.V := 0 + mcr p15, 0, r2, c1, c0, 0 @ write SCTLR + ldr r2, =vector_table + mcr p15, 0, r2, c12, c0, 0 @ write VBAR + + mrs r2, cpsr + + /* + * Input r0 is the stack top, which is the exception stacks base + * The first frame is reserved for svc mode + */ + set_mode_stack UND_MODE, r0 + set_mode_stack ABT_MODE, r0 + set_mode_stack IRQ_MODE, r0 + set_mode_stack FIQ_MODE, r0 + + msr cpsr_cxsf, r2 @ back to svc mode + isb + mov pc, lr + /* * Vector stubs * Simplified version of the Linux kernel implementation diff --git a/arm/cstart64.S b/arm/cstart64.S index 0a85338bcdae..d39cf4dfb99c 100644 --- a/arm/cstart64.S +++ b/arm/cstart64.S @@ -89,10 +89,12 @@ start: msr cpacr_el1, x4 /* set up exception handling */ + mov x4, x0 // x0 is the addr of the dtb bl exceptions_init /* complete setup */ - bl setup // x0 is the addr of the dtb + mov x0, x4 // restore the addr of the dtb + bl setup bl get_mmu_off cbnz x0, 1f bl setup_vm @@ -109,13 +111,6 @@ start: bl exit b halt -exceptions_init: - adrp x4, vector_table - add x4, x4, :lo12:vector_table - msr vbar_el1, x4 - isb - ret - .text .globl get_mmu_off @@ -251,6 +246,17 @@ asm_mmu_disable: /* * Vectors + */ + +exceptions_init: + adrp x0, vector_table + add x0, x0, :lo12:vector_table + msr vbar_el1, x0 + isb + ret + +/* + * Vector stubs * Adapted from arch/arm64/kernel/entry.S */ .macro vector_stub, name, vec
Move secondary_entry helper functions out of .init and into .text, since secondary_entry isn't run at "init" time. Signed-off-by: Andrew Jones <drjones@redhat.com> --- arm/cstart.S | 62 +++++++++++++++++++++++++++----------------------- arm/cstart64.S | 22 +++++++++++------- 2 files changed, 48 insertions(+), 36 deletions(-)