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

[Mark Rutland <mark.rutland@arm.com>]

On Mon, Aug 19, 2024 at 06:22:41PM +0100, Andre Przywara wrote:
> On Mon, 12 Aug 2024 11:15:51 +0100
> Mark Rutland <mark.rutland@arm.com> 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 <mark.rutland@arm.com>
> > Acked-by: Marc Zyngier <maz@kernel.org>
> > Cc: Akos Denke <akos.denke@arm.com>
> > Cc: Andre Przywara <andre.przywara@arm.com>
> > Cc: Luca Fancellu <luca.fancellu@arm.com>
> > ---
> >  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.