| Message ID | 20210729152050.23635-5-mark.rutland@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Preparatory fixes and cleanup | expand |
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? > > - 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. 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. > 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? 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
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 >
On Fri, Jul 30, 2021 at 05:43:33PM +0100, Mark Rutland wrote: > 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. Upon reflection, I'm going to drop this patch from the series for now and rework it to make the above clearer and more robust... > 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 ... and to try to make this true as a first step. Thanks, Mark.
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 - 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 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 - 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
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(-)