Re: [boot-wrapper PATCH 04/12] Remove `flag_no_el3`

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

On Fri, Jul 30, 2021 at 04:13:05PM +0100, Andre Przywara wrote:
> On Thu, 29 Jul 2021 16:20:42 +0100
> Mark Rutland <mark.rutland@arm.com> wrote:
> 
> Hi,
> 
> > We set `flag_no_el3` when not booted at EL3 / monitor mode, and
> > subsequently we use this to determine whether we need to drop exception
> > level before entering Linux. As this can be derived from CurrentEL or
> > CPSR, the flag itself is redundant, and we can defer the check until
> > we're about to enter Linux.
> > 
> > In future this will allow more logic to be converted into C, where it
> > will be easier to handle architectural variants.
> > 
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > ---
> >  arch/aarch32/boot.S | 14 +++-----------
> >  arch/aarch64/boot.S | 13 ++-----------
> >  2 files changed, 5 insertions(+), 22 deletions(-)
> > 
> > diff --git a/arch/aarch32/boot.S b/arch/aarch32/boot.S
> > index 2a85ad5..0bd1ca2 100644
> > --- a/arch/aarch32/boot.S
> > +++ b/arch/aarch32/boot.S
> > @@ -31,9 +31,6 @@ ENTRY(_start)
> >  	cmp	r0, #PSR_HYP
> >  	bne	_switch_monitor
> 
> Can't this become "beq start_no_el3" now?

I'm working to *remove* the el3/no_el3 labels, and handle the specific
exception levels as required, so I don't want to introduce that.

This says exactly what it does (i.e. switch to monitor mode), so I'd
rather leave it as-is.

> > -	mov	r0, #1
> > -	ldr	r1, =flag_no_el3
> > -	str	r0, [r1]
> >  	b	start_no_el3
> >  
> >  _switch_monitor:
> > @@ -89,9 +86,9 @@ ENTRY(jump_kernel)
> >  	ldr	lr, [r5], #4
> >  	ldm	r5, {r0 - r2}
> >  
> > -	ldr	r4, =flag_no_el3
> > -	ldr	r4, [r4]
> > -	cmp	r4, #1
> > +	mrs	r4, cpsr
> > +	and	r4, #PSR_MODE_MASK
> > +	cmp	r4, #PSR_MON
> 
> Is comparing explicitly against monitor mode the right thing? IIRC
> normally we come out of reset in secure SVC, and this *is* EL3 (the
> highest implemented exception level), from an ARMv8 perspective.

I agree it's not quite right, but the situation is more complicated:
It's more complicated than that. For details see:

* G1.4.1 "About the AArch32 PE modes"
* G1.9.1 "AArch32 state PE mode descriptions"
* G1.17 "Reset into AArch32 state" says:

The summary is:

* AArch32 doesn't necessarily reset into EL3. EL3 an EL2 are OPTIONAL.

* Supervisor mode can exist in EL3, Secure EL1, and Non-Secure EL1, and
  the PSR doesn't tell you which of the three you're in.

The boot-wrapper currently assumes we reset into EL3 or Non-Secure EL2,
and this is after the switch, where we should be in monitor mode
(otherwise PSCI cannot work, since we can't write to MVBAR). I'm not
changing that assumption. 

We should be able to rework that to *try* to switch to monitor mode, and
if that fails stick to S/NS EL1. I'm happy to tackle that as a follow
up, organising the logic so we can rely on:

* MON being EL3
* HYP being NS EL2
* SVC being S EL1 or NS EL1

> The old code did compare against HYP, which is probably what we want
> and is also one of the few modes we are sure of being not EL3.

That maches EL2 specifically (and I have left that as-is), but not about
EL3/EL1.

> 
> >  	bxeq	lr				@ no EL3
> >  
> >  	ldr	r4, =SPSR_KERNEL
> > @@ -113,8 +110,3 @@ boot_vectors:
> >  	b	.
> >  	b	.
> >  	b	.
> > -
> > -	.section .data
> > -	.align 2
> > -flag_no_el3:
> > -	.long 0
> > diff --git a/arch/aarch64/boot.S b/arch/aarch64/boot.S
> > index 37759ce..fae0188 100644
> > --- a/arch/aarch64/boot.S
> > +++ b/arch/aarch64/boot.S
> > @@ -28,10 +28,6 @@ _start:
> >  	cmp	x0, #CURRENTEL_EL3
> >  	b.eq	1f
> 
> Can't this become "b.ne start_no_el3" now?

As above, I'm working towards removing those labels, and having a single
boot path, so I'd prefer to leave that as-is for now.

Thanks,
Mark.

> 
> Cheers,
> Andre
> 
> >  
> > -	mov	w0, #1
> > -	ldr	x1, =flag_no_el3
> > -	str	w0, [x1]
> > -
> >  	b	start_no_el3
> >  
> >  1:	mov	x0, #0x30			// RES1
> > @@ -140,8 +136,8 @@ jump_kernel:
> >  	bl	find_logical_id
> >  	bl	setup_stack		// Reset stack pointer
> >  
> > -	ldr	w0, flag_no_el3
> > -	cmp	w0, #0			// Prepare Z flag
> > +	mrs	x0, CurrentEl
> > +	cmp	w0, #CURRENTEL_EL3	// Prepare Z flag
> >  
> >  	mov	x0, x20
> >  	mov	x1, x21
> > @@ -164,8 +160,3 @@ jump_kernel:
> >  	eret
> >  
> >  	.ltorg
> > -
> > -	.data
> > -	.align 3
> > -flag_no_el3:
> > -	.long 0
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel