diff mbox series

[kvm-unit-tests,1/8] arm/arm64: Reorganize cstart assembler

Message ID 20210407185918.371983-2-drjones@redhat.com (mailing list archive)
State New
Headers show
Series arm/arm64: Prepare for target-efi | expand

Commit Message

Andrew Jones April 7, 2021, 6:59 p.m. UTC
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(-)

Comments

Nikos Nikoleris April 9, 2021, 5:18 p.m. UTC | #1
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
>
Andrew Jones April 9, 2021, 5:28 p.m. UTC | #2
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
Alexandru Elisei April 13, 2021, 4:34 p.m. UTC | #3
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
Andrew Jones April 14, 2021, 8:59 a.m. UTC | #4
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
Alexandru Elisei April 14, 2021, 3:15 p.m. UTC | #5
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
Andrew Jones April 15, 2021, 1:03 p.m. UTC | #6
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 mbox series

Patch

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