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

[Andre Przywara]

On Tue, 20 Aug 2024 14:36:37 +0100
Mark Rutland <mark.rutland at arm.com> 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 <mark.rutland at arm.com> 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 <mark.rutland at arm.com> 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.