[BOOT-WRAPPER v2 06/10] aarch32: Always enter kernel via exception return
Mark Rutland
mark.rutland at arm.com
Tue Aug 20 06:36:37 PDT 2024
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!
> > 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.
Mark.
More information about the linux-arm-kernel
mailing list