[BOOT-WRAPPER,v2,06/10] aarch32: Always enter kernel via exception return

Commit Message

Mark Rutland Aug. 12, 2024, 10:15 a.m. UTC

When the boot-wrapper is entered at Seculre PL1 it will enter the kernel
via an exception return, ERET, and when entered at Hyp it will branch to
the kernel directly. This is an artifact of the way the boot-wrapper was
originally written in assembly, and it would be preferable to always
enter the kernel via an exception return so that PSTATE is always
initialized to a known-good value.

Rework jump_kernel() to always enter the kernel via ERET, matching the
stype of the AArch64 version of jump_kernel()

Signed-off-by: Mark Rutland <[e-mail]>
Acked-by: Marc Zyngier <[e-mail]>
Cc: Akos Denke <[e-mail]>
Cc: Andre Przywara <[e-mail]>
Cc: Luca Fancellu <[e-mail]>
---
 arch/aarch32/boot.S | 48 +++++++++++++++++++++++----------------------
 1 file changed, 25 insertions(+), 23 deletions(-)

Comments

Andre Przywara Aug. 19, 2024, 5:22 p.m. UTC | #1

On Mon, 12 Aug 2024 11:15:51 +0100
Mark Rutland <[e-mail]> wrote:

Hi Mark,

> When the boot-wrapper is entered at Seculre PL1 it will enter the kernel

				      Secure

> via an exception return, ERET, and when entered at Hyp it will branch to
> the kernel directly. This is an artifact of the way the boot-wrapper was
> originally written in assembly, and it would be preferable to always
> enter the kernel via an exception return so that PSTATE is always
> initialized to a known-good value.
> 
> Rework jump_kernel() to always enter the kernel via ERET, matching the
> stype of the AArch64 version of jump_kernel()

  type

> 
> Signed-off-by: Mark Rutland <[e-mail]>
> Acked-by: Marc Zyngier <[e-mail]>
> Cc: Akos Denke <[e-mail]>
> Cc: Andre Przywara <[e-mail]>
> Cc: Luca Fancellu <[e-mail]>
> ---
>  arch/aarch32/boot.S | 48 +++++++++++++++++++++++----------------------
>  1 file changed, 25 insertions(+), 23 deletions(-)
> 
> diff --git a/arch/aarch32/boot.S b/arch/aarch32/boot.S
> index f21f89a..78d19a0 100644
> --- a/arch/aarch32/boot.S
> +++ b/arch/aarch32/boot.S
> @@ -76,10 +76,6 @@ reset_at_hyp:
>  
>  	bl	setup_stack
>  
> -	mov	r0, #1
> -	ldr	r1, =flag_no_el3
> -	str	r0, [r1]
> -
>  	bl	cpu_init_bootwrapper
>  
>  	bl	cpu_init_arch
> @@ -96,9 +92,10 @@ err_invalid_id:
>  	 * r1-r3, sp[0]: kernel arguments
>  	 */
>  ASM_FUNC(jump_kernel)
> -	sub	sp, #4				@ Ignore fourth argument

Can we maybe keep the comment, to avoid confusion? The comment above
explicitly mentions sp[0], but we never use it.

> -	push	{r0 - r3}
> -	mov	r5, sp
> +	mov	r4, r0
> +	mov	r5, r1
> +	mov	r6, r2
> +	mov	r7, r3
>  
>  	ldr	r0, =HSCTLR_KERNEL
>  	mcr	p15, 4, r0, c1, c0, 0		@ HSCTLR
> @@ -111,23 +108,28 @@ ASM_FUNC(jump_kernel)
>  	bl	find_logical_id
>  	bl	setup_stack
>  
> -	ldr	lr, [r5], #4
> -	ldm	r5, {r0 - r2}
> -
> -	ldr	r4, =flag_no_el3
> -	ldr	r4, [r4]
> -	cmp	r4, #1
> -	bxeq	lr				@ no EL3
> +	mov	r0, r5
> +	mov	r1, r6
> +	mov	r2, r7
> +	ldr	r3, =SPSR_KERNEL
>  
> -	ldr	r4, =SPSR_KERNEL
>  	/* Return in thumb2 mode when bit 0 of address is 1 */
> -	tst	lr, #1
> -	orrne	r4, #PSR_T
> +	tst	r4, #1
> +	orrne	r3, #PSR_T
> +
> +	mrs	r5, cpsr
> +	and	r5, #PSR_MODE_MASK
> +	cmp	r5, #PSR_MON
> +	beq	eret_at_mon
> +	cmp	r5, #PSR_HYP
> +	beq	eret_at_hyp
> +	b	.
>  
> -	msr	spsr_cxf, r4
> +eret_at_mon:
> +	mov	lr, r4
> +	msr	spsr_cxf, r3
>  	movs	pc, lr

Maybe that's just a wording confusion between "exception return
instruction" and "eret", but both the commit message and the label
promise an eret, and we have a "movs pc" here.
Reading "B9.1 General restrictions on system instructions" in the ARMv7 ARM
I don't immediately see why an eret wouldn't be possible here.

If there is a restriction I missed, I guess either a comment here or in
the commit message would be helpful.

> -
> -	.section .data
> -	.align 2
> -flag_no_el3:
> -	.long 0
> +eret_at_hyp:
> +	msr	elr_hyp, r4
> +	msr	spsr_cxf, r3

Shouldn't that be spsr_hyp?

Cheers,
Andre


> +	eret

Mark Rutland Aug. 20, 2024, 11:43 a.m. UTC | #2

On Mon, Aug 19, 2024 at 06:22:41PM +0100, Andre Przywara wrote:
> On Mon, 12 Aug 2024 11:15:51 +0100
> Mark Rutland <[e-mail]> wrote:
> 
> Hi Mark,
> 
> > When the boot-wrapper is entered at Seculre PL1 it will enter the kernel
> 
> 				      Secure
> 
> > via an exception return, ERET, and when entered at Hyp it will branch to

FWIW, that "ERET, " here was meant to go too (which I think addresses a
later concern below). I had taken the commit message wording from the
early AArch64 commit and adjusted it to suit AArch32, but clearly I had
done that in a rush and madea number of mistakes.

> > the kernel directly. This is an artifact of the way the boot-wrapper was
> > originally written in assembly, and it would be preferable to always
> > enter the kernel via an exception return so that PSTATE is always
> > initialized to a known-good value.
> > 
> > Rework jump_kernel() to always enter the kernel via ERET, matching the
> > stype of the AArch64 version of jump_kernel()
> 
>   type
> 
> > 
> > Signed-off-by: Mark Rutland <[e-mail]>
> > Acked-by: Marc Zyngier <[e-mail]>
> > Cc: Akos Denke <[e-mail]>
> > Cc: Andre Przywara <[e-mail]>
> > Cc: Luca Fancellu <[e-mail]>
> > ---
> >  arch/aarch32/boot.S | 48 +++++++++++++++++++++++----------------------
> >  1 file changed, 25 insertions(+), 23 deletions(-)
> > 
> > diff --git a/arch/aarch32/boot.S b/arch/aarch32/boot.S
> > index f21f89a..78d19a0 100644
> > --- a/arch/aarch32/boot.S
> > +++ b/arch/aarch32/boot.S
> > @@ -76,10 +76,6 @@ reset_at_hyp:
> >  
> >  	bl	setup_stack
> >  
> > -	mov	r0, #1
> > -	ldr	r1, =flag_no_el3
> > -	str	r0, [r1]
> > -
> >  	bl	cpu_init_bootwrapper
> >  
> >  	bl	cpu_init_arch
> > @@ -96,9 +92,10 @@ err_invalid_id:
> >  	 * r1-r3, sp[0]: kernel arguments
> >  	 */
> >  ASM_FUNC(jump_kernel)
> > -	sub	sp, #4				@ Ignore fourth argument
> 
> Can we maybe keep the comment, to avoid confusion? The comment above
> explicitly mentions sp[0], but we never use it.
> 
> > -	push	{r0 - r3}
> > -	mov	r5, sp
> > +	mov	r4, r0
> > +	mov	r5, r1
> > +	mov	r6, r2
> > +	mov	r7, r3
> >  
> >  	ldr	r0, =HSCTLR_KERNEL
> >  	mcr	p15, 4, r0, c1, c0, 0		@ HSCTLR
> > @@ -111,23 +108,28 @@ ASM_FUNC(jump_kernel)
> >  	bl	find_logical_id
> >  	bl	setup_stack
> >  
> > -	ldr	lr, [r5], #4
> > -	ldm	r5, {r0 - r2}
> > -
> > -	ldr	r4, =flag_no_el3
> > -	ldr	r4, [r4]
> > -	cmp	r4, #1
> > -	bxeq	lr				@ no EL3
> > +	mov	r0, r5
> > +	mov	r1, r6
> > +	mov	r2, r7
> > +	ldr	r3, =SPSR_KERNEL
> >  
> > -	ldr	r4, =SPSR_KERNEL
> >  	/* Return in thumb2 mode when bit 0 of address is 1 */
> > -	tst	lr, #1
> > -	orrne	r4, #PSR_T
> > +	tst	r4, #1
> > +	orrne	r3, #PSR_T
> > +
> > +	mrs	r5, cpsr
> > +	and	r5, #PSR_MODE_MASK
> > +	cmp	r5, #PSR_MON
> > +	beq	eret_at_mon
> > +	cmp	r5, #PSR_HYP
> > +	beq	eret_at_hyp
> > +	b	.
> >  
> > -	msr	spsr_cxf, r4
> > +eret_at_mon:
> > +	mov	lr, r4
> > +	msr	spsr_cxf, r3
> >  	movs	pc, lr
> 
> Maybe that's just a wording confusion between "exception return
> instruction" and "eret", but both the commit message and the label
> promise an eret, and we have a "movs pc" here.

Wording-wise, "ERET" was spurious, and the commit message was inteneded
to say "via an exception return", with the "movs pc, lr" being the
exception return.

> Reading "B9.1 General restrictions on system instructions" in the ARMv7 ARM
> I don't immediately see why an eret wouldn't be possible here.
> 
> If there is a restriction I missed, I guess either a comment here or in
> the commit message would be helpful.

We can use ERET here; IIRC that was added in the ARMv7 virtualization
extensions, but the boot-wrapper requires that and really it's ARMv8+
anyway. I had opted to stick with "movs pc, lr" because it was a
(trivially) smaller change, and kept the cases distinct, but I'm happy
to use ERET.

However, beware that in AArch32 ERET is a bit odd: in Hyp mode takes the
return address from ELR_HYP, while in all other modes it takes it from
the LR (as only hyp has an ELR).

> > -
> > -	.section .data
> > -	.align 2
> > -flag_no_el3:
> > -	.long 0
> > +eret_at_hyp:
> > +	msr	elr_hyp, r4
> > +	msr	spsr_cxf, r3
> 
> Shouldn't that be spsr_hyp?

It can be, but doesn't need to be. This is the SPSR_<fields> encoding,
which writes to the SPSR for owned by the active mode, though it skips
bits<23:16>, which we probably should initialise.

If I change that all to:

| eret_at_mon:
| 	mov	lr, r4
| 	msr	spsr_mon, r3
| 	eret
| eret_at_hyp:
| 	msr     elr_hyp, r4
| 	msr     spsr_hyp, r3
|

... do you think that's clear enough, or do you think we need a comment
about the "LR" vs "ELR_HYP" distinction?

Mark.

Mark Rutland Aug. 20, 2024, 11:47 a.m. UTC | #3

On Mon, Aug 19, 2024 at 06:22:41PM +0100, Andre Przywara wrote:
> On Mon, 12 Aug 2024 11:15:51 +0100
> Mark Rutland <[e-mail]> wrote:
> > Rework jump_kernel() to always enter the kernel via ERET, matching the
> > stype of the AArch64 version of jump_kernel()
> 
>   type

That's actually meant to be "style", and "ERET" should be "an exception
return", i.e.

| Rework jump_kernel() to always enter the kernel via an exception
| return, matching the style of the AArch64 version of jump_kernel()

Mark.

Andre Przywara Aug. 20, 2024, 12:23 p.m. UTC | #4

On Tue, 20 Aug 2024 12:47:59 +0100
Mark Rutland <[e-mail]> wrote:

> On Mon, Aug 19, 2024 at 06:22:41PM +0100, Andre Przywara wrote:
> > On Mon, 12 Aug 2024 11:15:51 +0100
> > Mark Rutland <[e-mail]> wrote:  
> > > Rework jump_kernel() to always enter the kernel via ERET, matching the
> > > stype of the AArch64 version of jump_kernel()  
> > 
> >   type  
> 
> That's actually meant to be "style", and "ERET" should be "an exception
> return", i.e.

Ah, but three tries is not bad for a Wordle ;-)

Cheers,
Andre

> 
> | Rework jump_kernel() to always enter the kernel via an exception
> | return, matching the style of the AArch64 version of jump_kernel()
> 
> Mark.

Andre Przywara Aug. 20, 2024, 12:59 p.m. UTC | #5

On Tue, 20 Aug 2024 12:43:18 +0100
Mark Rutland <[e-mail]> wrote:

Hi,

> On Mon, Aug 19, 2024 at 06:22:41PM +0100, Andre Przywara wrote:
> > On Mon, 12 Aug 2024 11:15:51 +0100
> > Mark Rutland <[e-mail]> wrote:
> > 
> > Hi Mark,
> >   
> > > When the boot-wrapper is entered at Seculre PL1 it will enter the kernel  
> > 
> > 				      Secure
> >   
> > > via an exception return, ERET, and when entered at Hyp it will branch to  
> 
> FWIW, that "ERET, " here was meant to go too (which I think addresses a
> later concern below). I had taken the commit message wording from the
> early AArch64 commit and adjusted it to suit AArch32, but clearly I had
> done that in a rush and madea number of mistakes.

Yeah, no worries, I was just getting a bit confused, especially when I
learned more about the subtleties of exception returns on ARMv7 than I
ever wanted to know yesterday.

> > > the kernel directly. This is an artifact of the way the boot-wrapper was
> > > originally written in assembly, and it would be preferable to always
> > > enter the kernel via an exception return so that PSTATE is always
> > > initialized to a known-good value.
> > > 
> > > Rework jump_kernel() to always enter the kernel via ERET, matching the
> > > stype of the AArch64 version of jump_kernel()  
> > 
> >   type
> >   
> > > 
> > > Signed-off-by: Mark Rutland <[e-mail]>
> > > Acked-by: Marc Zyngier <[e-mail]>
> > > Cc: Akos Denke <[e-mail]>
> > > Cc: Andre Przywara <[e-mail]>
> > > Cc: Luca Fancellu <[e-mail]>
> > > ---
> > >  arch/aarch32/boot.S | 48 +++++++++++++++++++++++----------------------
> > >  1 file changed, 25 insertions(+), 23 deletions(-)
> > > 
> > > diff --git a/arch/aarch32/boot.S b/arch/aarch32/boot.S
> > > index f21f89a..78d19a0 100644
> > > --- a/arch/aarch32/boot.S
> > > +++ b/arch/aarch32/boot.S
> > > @@ -76,10 +76,6 @@ reset_at_hyp:
> > >  
> > >  	bl	setup_stack
> > >  
> > > -	mov	r0, #1
> > > -	ldr	r1, =flag_no_el3
> > > -	str	r0, [r1]
> > > -
> > >  	bl	cpu_init_bootwrapper
> > >  
> > >  	bl	cpu_init_arch
> > > @@ -96,9 +92,10 @@ err_invalid_id:
> > >  	 * r1-r3, sp[0]: kernel arguments
> > >  	 */
> > >  ASM_FUNC(jump_kernel)
> > > -	sub	sp, #4				@ Ignore fourth argument  
> > 
> > Can we maybe keep the comment, to avoid confusion? The comment above
> > explicitly mentions sp[0], but we never use it.
> >   
> > > -	push	{r0 - r3}
> > > -	mov	r5, sp
> > > +	mov	r4, r0
> > > +	mov	r5, r1
> > > +	mov	r6, r2
> > > +	mov	r7, r3
> > >  
> > >  	ldr	r0, =HSCTLR_KERNEL
> > >  	mcr	p15, 4, r0, c1, c0, 0		@ HSCTLR
> > > @@ -111,23 +108,28 @@ ASM_FUNC(jump_kernel)
> > >  	bl	find_logical_id
> > >  	bl	setup_stack
> > >  
> > > -	ldr	lr, [r5], #4
> > > -	ldm	r5, {r0 - r2}
> > > -
> > > -	ldr	r4, =flag_no_el3
> > > -	ldr	r4, [r4]
> > > -	cmp	r4, #1
> > > -	bxeq	lr				@ no EL3
> > > +	mov	r0, r5
> > > +	mov	r1, r6
> > > +	mov	r2, r7
> > > +	ldr	r3, =SPSR_KERNEL
> > >  
> > > -	ldr	r4, =SPSR_KERNEL
> > >  	/* Return in thumb2 mode when bit 0 of address is 1 */
> > > -	tst	lr, #1
> > > -	orrne	r4, #PSR_T
> > > +	tst	r4, #1
> > > +	orrne	r3, #PSR_T
> > > +
> > > +	mrs	r5, cpsr
> > > +	and	r5, #PSR_MODE_MASK
> > > +	cmp	r5, #PSR_MON
> > > +	beq	eret_at_mon
> > > +	cmp	r5, #PSR_HYP
> > > +	beq	eret_at_hyp
> > > +	b	.
> > >  
> > > -	msr	spsr_cxf, r4
> > > +eret_at_mon:
> > > +	mov	lr, r4
> > > +	msr	spsr_cxf, r3
> > >  	movs	pc, lr  
> > 
> > Maybe that's just a wording confusion between "exception return
> > instruction" and "eret", but both the commit message and the label
> > promise an eret, and we have a "movs pc" here.  
> 
> Wording-wise, "ERET" was spurious, and the commit message was inteneded
> to say "via an exception return", with the "movs pc, lr" being the
> exception return.

Ah thanks, that was one of the two possibilities I considered.

> > Reading "B9.1 General restrictions on system instructions" in the ARMv7 ARM
> > I don't immediately see why an eret wouldn't be possible here.
> > 
> > If there is a restriction I missed, I guess either a comment here or in
> > the commit message would be helpful.  
> 
> We can use ERET here; IIRC that was added in the ARMv7 virtualization
> extensions, but the boot-wrapper requires that and really it's ARMv8+

Is that so? I mean in all practicality we will indeed use the bootwrapper
on ARMv8 only these days, but I don't think we need to artificially limit
this. Also I consider the boot-wrapper one of the more reliable sources
for ARMv7 boot code, so not sure we should drop this aspect.
There is one ARMv7 compile time check, to avoid "sevl", so we have some
support, at least.

> anyway. I had opted to stick with "movs pc, lr" because it was a
> (trivially) smaller change, and kept the cases distinct, but I'm happy
> to use ERET.
> 
> However, beware that in AArch32 ERET is a bit odd: in Hyp mode takes the
> return address from ELR_HYP, while in all other modes it takes it from
> the LR (as only hyp has an ELR).

Yeah, I saw this yesterday, and am even more grateful for the ARMv8
exception model now ;-)

So I am fine with "movs pc, lr", if that's the more canonical way on
32-bit/ARMv7. On the other hand your revised sequence below looks
intriguingly simple ...

> 
> > > -
> > > -	.section .data
> > > -	.align 2
> > > -flag_no_el3:
> > > -	.long 0
> > > +eret_at_hyp:
> > > +	msr	elr_hyp, r4
> > > +	msr	spsr_cxf, r3  
> > 
> > Shouldn't that be spsr_hyp?  
> 
> It can be, but doesn't need to be. This is the SPSR_<fields> encoding,

So I didn't know about this until yesterday, and it's not easy to find,
since it seems not to be mentioned as such in the ARM ARM (at least not
"cxf"). binutils seems to disassemble this to SPSR_fxc, but I guess we
should indeed move to SPSR_fsxc (if we keep this at all).

> which writes to the SPSR for owned by the active mode, though it skips
> bits<23:16>, which we probably should initialise.
> 
> If I change that all to:
> 
> | eret_at_mon:
> | 	mov	lr, r4
> | 	msr	spsr_mon, r3
> | 	eret
> | eret_at_hyp:
> | 	msr     elr_hyp, r4
> | 	msr     spsr_hyp, r3
> |
> 
> ... do you think that's clear enough, or do you think we need a comment
> about the "LR" vs "ELR_HYP" distinction?

Oh, that certainly looks the clearest, but indeed a comment on LR vs. ELR
situation looks indicated.

Thanks,
Andre

> 
> Mark.

Mark Rutland Aug. 20, 2024, 1:36 p.m. UTC | #6

On Tue, Aug 20, 2024 at 01:59:44PM +0100, Andre Przywara wrote:
> On Tue, 20 Aug 2024 12:43:18 +0100
> Mark Rutland <[e-mail]> wrote:
> > On Mon, Aug 19, 2024 at 06:22:41PM +0100, Andre Przywara wrote:
> > > On Mon, 12 Aug 2024 11:15:51 +0100
> > > Mark Rutland <[e-mail]> wrote:

> > > > @@ -111,23 +108,28 @@ ASM_FUNC(jump_kernel)
> > > >  	bl	find_logical_id
> > > >  	bl	setup_stack
> > > >  
> > > > -	ldr	lr, [r5], #4
> > > > -	ldm	r5, {r0 - r2}
> > > > -
> > > > -	ldr	r4, =flag_no_el3
> > > > -	ldr	r4, [r4]
> > > > -	cmp	r4, #1
> > > > -	bxeq	lr				@ no EL3
> > > > +	mov	r0, r5
> > > > +	mov	r1, r6
> > > > +	mov	r2, r7
> > > > +	ldr	r3, =SPSR_KERNEL
> > > >  
> > > > -	ldr	r4, =SPSR_KERNEL
> > > >  	/* Return in thumb2 mode when bit 0 of address is 1 */
> > > > -	tst	lr, #1
> > > > -	orrne	r4, #PSR_T
> > > > +	tst	r4, #1
> > > > +	orrne	r3, #PSR_T
> > > > +
> > > > +	mrs	r5, cpsr
> > > > +	and	r5, #PSR_MODE_MASK
> > > > +	cmp	r5, #PSR_MON
> > > > +	beq	eret_at_mon
> > > > +	cmp	r5, #PSR_HYP
> > > > +	beq	eret_at_hyp
> > > > +	b	.
> > > >  
> > > > -	msr	spsr_cxf, r4
> > > > +eret_at_mon:
> > > > +	mov	lr, r4
> > > > +	msr	spsr_cxf, r3
> > > >  	movs	pc, lr  

> > > Reading "B9.1 General restrictions on system instructions" in the ARMv7 ARM
> > > I don't immediately see why an eret wouldn't be possible here.
> > > 
> > > If there is a restriction I missed, I guess either a comment here or in
> > > the commit message would be helpful.  
> > 
> > We can use ERET here; IIRC that was added in the ARMv7 virtualization
> > extensions, but the boot-wrapper requires that and really it's ARMv8+
> 
> Is that so? I mean in all practicality we will indeed use the bootwrapper
> on ARMv8 only these days, but I don't think we need to artificially limit
> this. Also I consider the boot-wrapper one of the more reliable sources
> for ARMv7 boot code, so not sure we should drop this aspect.
> There is one ARMv7 compile time check, to avoid "sevl", so we have some
> support, at least.

What I was trying to say here was "the minimum bound is ARMv7 +
virtualization extensions", which is already required by the
".arch_extension virt" directive that's been in this file since it was
introduced.

Practically speaking, I don't think that we should care about ARMv7
here, but if that happens to work, great!

> > anyway. I had opted to stick with "movs pc, lr" because it was a
> > (trivially) smaller change, and kept the cases distinct, but I'm happy
> > to use ERET.
> > 
> > However, beware that in AArch32 ERET is a bit odd: in Hyp mode takes the
> > return address from ELR_HYP, while in all other modes it takes it from
> > the LR (as only hyp has an ELR).
> 
> Yeah, I saw this yesterday, and am even more grateful for the ARMv8
> exception model now ;-)
> 
> So I am fine with "movs pc, lr", if that's the more canonical way on
> 32-bit/ARMv7. On the other hand your revised sequence below looks
> intriguingly simple ...
> 
> > 
> > > > -
> > > > -	.section .data
> > > > -	.align 2
> > > > -flag_no_el3:
> > > > -	.long 0
> > > > +eret_at_hyp:
> > > > +	msr	elr_hyp, r4
> > > > +	msr	spsr_cxf, r3  
> > > 
> > > Shouldn't that be spsr_hyp?  
> > 
> > It can be, but doesn't need to be. This is the SPSR_<fields> encoding,
> 
> So I didn't know about this until yesterday, and it's not easy to find,
> since it seems not to be mentioned as such in the ARM ARM (at least not
> "cxf"). binutils seems to disassemble this to SPSR_fxc, but I guess we
> should indeed move to SPSR_fsxc (if we keep this at all).
> 
> > which writes to the SPSR for owned by the active mode, though it skips
> > bits<23:16>, which we probably should initialise.
> > 
> > If I change that all to:
> > 
> > | eret_at_mon:
> > | 	mov	lr, r4
> > | 	msr	spsr_mon, r3
> > | 	eret
> > | eret_at_hyp:
> > | 	msr     elr_hyp, r4
> > | 	msr     spsr_hyp, r3
> > |
> > 
> > ... do you think that's clear enough, or do you think we need a comment
> > about the "LR" vs "ELR_HYP" distinction?
> 
> Oh, that certainly looks the clearest, but indeed a comment on LR vs. ELR
> situation looks indicated.

Considering the earlier comments I'm going to make this:

| eret_at_mon:
| 	mov	lr, r4
| 	msr	spsr_mon
| 	movs	pc, lr
| eret_at_hyp:
| 	msr	elr_hyp, r4
| 	msr	spsr_hyp, r3
| 	eret

Using 'spsr_mon' and 'spsr_hyp' means we initialize *all* of the SPSR
bits, so that's a bug fix in addition to being clearer.

Using 'movs pc, lr' for the 'eret_at_mon' case is the standard way to do
exception returns in AArch32 generally, and then that clearly doesnt't
depend on the virtualization extensions, so if we ever want to handle a
CPU without hyp in future all we'll need to do is mess with the SPSR
value.

I'm not going to bother with a comment given that's standard AArch32
behaviour.

Mark.

Andre Przywara Aug. 20, 2024, 1:50 p.m. UTC | #7

On Tue, 20 Aug 2024 14:36:37 +0100
Mark Rutland <[e-mail]> wrote:

Hi Mark,

> On Tue, Aug 20, 2024 at 01:59:44PM +0100, Andre Przywara wrote:
> > On Tue, 20 Aug 2024 12:43:18 +0100
> > Mark Rutland <[e-mail]> wrote:  
> > > On Mon, Aug 19, 2024 at 06:22:41PM +0100, Andre Przywara wrote:  
> > > > On Mon, 12 Aug 2024 11:15:51 +0100
> > > > Mark Rutland <[e-mail]> wrote:  
> 
> > > > > @@ -111,23 +108,28 @@ ASM_FUNC(jump_kernel)
> > > > >  	bl	find_logical_id
> > > > >  	bl	setup_stack
> > > > >  
> > > > > -	ldr	lr, [r5], #4
> > > > > -	ldm	r5, {r0 - r2}
> > > > > -
> > > > > -	ldr	r4, =flag_no_el3
> > > > > -	ldr	r4, [r4]
> > > > > -	cmp	r4, #1
> > > > > -	bxeq	lr				@ no EL3
> > > > > +	mov	r0, r5
> > > > > +	mov	r1, r6
> > > > > +	mov	r2, r7
> > > > > +	ldr	r3, =SPSR_KERNEL
> > > > >  
> > > > > -	ldr	r4, =SPSR_KERNEL
> > > > >  	/* Return in thumb2 mode when bit 0 of address is 1 */
> > > > > -	tst	lr, #1
> > > > > -	orrne	r4, #PSR_T
> > > > > +	tst	r4, #1
> > > > > +	orrne	r3, #PSR_T
> > > > > +
> > > > > +	mrs	r5, cpsr
> > > > > +	and	r5, #PSR_MODE_MASK
> > > > > +	cmp	r5, #PSR_MON
> > > > > +	beq	eret_at_mon
> > > > > +	cmp	r5, #PSR_HYP
> > > > > +	beq	eret_at_hyp
> > > > > +	b	.
> > > > >  
> > > > > -	msr	spsr_cxf, r4
> > > > > +eret_at_mon:
> > > > > +	mov	lr, r4
> > > > > +	msr	spsr_cxf, r3
> > > > >  	movs	pc, lr    
> 
> > > > Reading "B9.1 General restrictions on system instructions" in the ARMv7 ARM
> > > > I don't immediately see why an eret wouldn't be possible here.
> > > > 
> > > > If there is a restriction I missed, I guess either a comment here or in
> > > > the commit message would be helpful.    
> > > 
> > > We can use ERET here; IIRC that was added in the ARMv7 virtualization
> > > extensions, but the boot-wrapper requires that and really it's ARMv8+  
> > 
> > Is that so? I mean in all practicality we will indeed use the bootwrapper
> > on ARMv8 only these days, but I don't think we need to artificially limit
> > this. Also I consider the boot-wrapper one of the more reliable sources
> > for ARMv7 boot code, so not sure we should drop this aspect.
> > There is one ARMv7 compile time check, to avoid "sevl", so we have some
> > support, at least.  
> 
> What I was trying to say here was "the minimum bound is ARMv7 +
> virtualization extensions", which is already required by the
> ".arch_extension virt" directive that's been in this file since it was
> introduced.
> 
> Practically speaking, I don't think that we should care about ARMv7
> here, but if that happens to work, great!

Ah, no, I meant "armv7ve". Given that we either drop to HYP or stay in
HYP, I don't think supporting something before that makes much sense here
;-)

> > > anyway. I had opted to stick with "movs pc, lr" because it was a
> > > (trivially) smaller change, and kept the cases distinct, but I'm happy
> > > to use ERET.
> > > 
> > > However, beware that in AArch32 ERET is a bit odd: in Hyp mode takes the
> > > return address from ELR_HYP, while in all other modes it takes it from
> > > the LR (as only hyp has an ELR).  
> > 
> > Yeah, I saw this yesterday, and am even more grateful for the ARMv8
> > exception model now ;-)
> > 
> > So I am fine with "movs pc, lr", if that's the more canonical way on
> > 32-bit/ARMv7. On the other hand your revised sequence below looks
> > intriguingly simple ...
> >   
> > >   
> > > > > -
> > > > > -	.section .data
> > > > > -	.align 2
> > > > > -flag_no_el3:
> > > > > -	.long 0
> > > > > +eret_at_hyp:
> > > > > +	msr	elr_hyp, r4
> > > > > +	msr	spsr_cxf, r3    
> > > > 
> > > > Shouldn't that be spsr_hyp?    
> > > 
> > > It can be, but doesn't need to be. This is the SPSR_<fields> encoding,  
> > 
> > So I didn't know about this until yesterday, and it's not easy to find,
> > since it seems not to be mentioned as such in the ARM ARM (at least not
> > "cxf"). binutils seems to disassemble this to SPSR_fxc, but I guess we
> > should indeed move to SPSR_fsxc (if we keep this at all).
> >   
> > > which writes to the SPSR for owned by the active mode, though it skips
> > > bits<23:16>, which we probably should initialise.
> > > 
> > > If I change that all to:
> > > 
> > > | eret_at_mon:
> > > | 	mov	lr, r4
> > > | 	msr	spsr_mon, r3
> > > | 	eret
> > > | eret_at_hyp:
> > > | 	msr     elr_hyp, r4
> > > | 	msr     spsr_hyp, r3
> > > |
> > > 
> > > ... do you think that's clear enough, or do you think we need a comment
> > > about the "LR" vs "ELR_HYP" distinction?  
> > 
> > Oh, that certainly looks the clearest, but indeed a comment on LR vs. ELR
> > situation looks indicated.  
> 
> Considering the earlier comments I'm going to make this:
> 
> | eret_at_mon:
> | 	mov	lr, r4
> | 	msr	spsr_mon
> | 	movs	pc, lr
> | eret_at_hyp:
> | 	msr	elr_hyp, r4
> | 	msr	spsr_hyp, r3
> | 	eret
> 
> Using 'spsr_mon' and 'spsr_hyp' means we initialize *all* of the SPSR
> bits, so that's a bug fix in addition to being clearer.
> 
> Using 'movs pc, lr' for the 'eret_at_mon' case is the standard way to do
> exception returns in AArch32 generally, and then that clearly doesnt't
> depend on the virtualization extensions, so if we ever want to handle a
> CPU without hyp in future all we'll need to do is mess with the SPSR
> value.
> 
> I'm not going to bother with a comment given that's standard AArch32
> behaviour.

Many thanks, that looks absolutely fine to me and makes the most sense!

Cheers,
Andre.

Patch

diff --git a/arch/aarch32/boot.S b/arch/aarch32/boot.S
index f21f89a..78d19a0 100644
--- a/arch/aarch32/boot.S
+++ b/arch/aarch32/boot.S
@@ -76,10 +76,6 @@  reset_at_hyp:
 
 	bl	setup_stack
 
-	mov	r0, #1
-	ldr	r1, =flag_no_el3
-	str	r0, [r1]
-
 	bl	cpu_init_bootwrapper
 
 	bl	cpu_init_arch
@@ -96,9 +92,10 @@  err_invalid_id:
 	 * r1-r3, sp[0]: kernel arguments
 	 */
 ASM_FUNC(jump_kernel)
-	sub	sp, #4				@ Ignore fourth argument
-	push	{r0 - r3}
-	mov	r5, sp
+	mov	r4, r0
+	mov	r5, r1
+	mov	r6, r2
+	mov	r7, r3
 
 	ldr	r0, =HSCTLR_KERNEL
 	mcr	p15, 4, r0, c1, c0, 0		@ HSCTLR
@@ -111,23 +108,28 @@  ASM_FUNC(jump_kernel)
 	bl	find_logical_id
 	bl	setup_stack
 
-	ldr	lr, [r5], #4
-	ldm	r5, {r0 - r2}
-
-	ldr	r4, =flag_no_el3
-	ldr	r4, [r4]
-	cmp	r4, #1
-	bxeq	lr				@ no EL3
+	mov	r0, r5
+	mov	r1, r6
+	mov	r2, r7
+	ldr	r3, =SPSR_KERNEL
 
-	ldr	r4, =SPSR_KERNEL
 	/* Return in thumb2 mode when bit 0 of address is 1 */
-	tst	lr, #1
-	orrne	r4, #PSR_T
+	tst	r4, #1
+	orrne	r3, #PSR_T
+
+	mrs	r5, cpsr
+	and	r5, #PSR_MODE_MASK
+	cmp	r5, #PSR_MON
+	beq	eret_at_mon
+	cmp	r5, #PSR_HYP
+	beq	eret_at_hyp
+	b	.
 
-	msr	spsr_cxf, r4
+eret_at_mon:
+	mov	lr, r4
+	msr	spsr_cxf, r3
 	movs	pc, lr
-
-	.section .data
-	.align 2
-flag_no_el3:
-	.long 0
+eret_at_hyp:
+	msr	elr_hyp, r4
+	msr	spsr_cxf, r3
+	eret