[BOOT-WRAPPER v2 06/10] aarch32: Always enter kernel via exception return
Andre Przywara
andre.przywara at arm.com
Tue Aug 20 05:59:44 PDT 2024
On Tue, 20 Aug 2024 12:43:18 +0100
Mark Rutland <mark.rutland at arm.com> 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 <mark.rutland at 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.
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 <mark.rutland at arm.com>
> > > Acked-by: Marc Zyngier <maz at kernel.org>
> > > Cc: Akos Denke <akos.denke at arm.com>
> > > Cc: Andre Przywara <andre.przywara at arm.com>
> > > Cc: Luca Fancellu <luca.fancellu at 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.
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.
More information about the linux-arm-kernel
mailing list