[bootwrapper PATCH v2 06/13] aarch64: initialize SCTLR_ELx for the boot-wrapper
Mark Rutland
mark.rutland at arm.com
Mon Jan 17 04:15:57 PST 2022
On Fri, Jan 14, 2022 at 06:12:47PM +0000, Andre Przywara wrote:
> On Fri, 14 Jan 2022 10:56:46 +0000
> Mark Rutland <mark.rutland at arm.com> wrote:
>
> Hi Mark,
Hi Andre,
> > The SCTLR_ELx registers contain fields which are UNKNOWN or
> > IMPLEMENTATION DEFINED out of reset. This includes SCTLR_ELx.EE, which
> > defines the endianness of memory accesses (e.g. reads from literal
> > pools). Due to this, portions of boot-wrapper code are not guaranteed
> > to work correctly.
> >
> > Rework the startup code to explicitly initialize SCTLR_ELx for the
> > exception level the boot-wrapper was entered at. When entered at EL2
> > it's necessary to first initialise HCR_EL2.E2H as this affects the RESx
> > behaviour of bits in SCTLR_EL2, and also aliases SCTLR_EL1 to SCTLR_EL2,
> > which would break the initialization performed in jump_kernel.
> >
> > As we plan to eventually support the highest implemented EL being any of
> > EL3/EL2/EL1, code is added to handle all of these exception levels, even
> > though we do not currently support starting at EL1.
> >
> > We'll initialize other registers in subsequent patches.
>
> So the idea of initialising each EL and the respective code below looks
> good to me, however I have some questions about the SCTLR reset values
> below:
>
> >
> > Signed-off-by: Mark Rutland <mark.rutland at arm.com>
> > ---
> > arch/aarch64/boot.S | 74 +++++++++++++++++++++++++++-------
> > arch/aarch64/include/asm/cpu.h | 27 ++++++++++++-
> > 2 files changed, 85 insertions(+), 16 deletions(-)
> >
> > diff --git a/arch/aarch64/boot.S b/arch/aarch64/boot.S
> > index 900b9f8..45a0367 100644
> > --- a/arch/aarch64/boot.S
> > +++ b/arch/aarch64/boot.S
> > @@ -26,26 +26,26 @@
> > * PSCI is not supported when entered in this exception level.
> > */
> > ASM_FUNC(_start)
> > - cpuid x0, x1
> > - bl find_logical_id
> > - cmp x0, #MPIDR_INVALID
> > - beq err_invalid_id
> > - bl setup_stack
> > -
> > - /*
> > - * EL3 initialisation
> > - */
> > mrs x0, CurrentEL
> > cmp x0, #CURRENTEL_EL3
> > - b.eq 1f
> > + b.eq reset_at_el3
> > + cmp x0, #CURRENTEL_EL2
> > + b.eq reset_at_el2
> > + cmp x0, #CURRENTEL_EL1
> > + b.eq reset_at_el1
> >
> > - mov w0, #1
> > - ldr x1, =flag_no_el3
> > - str w0, [x1]
> > + /* Booting at EL0 is not supported */
> > + b .
> >
> > - b start_no_el3
> > + /*
> > + * EL3 initialisation
> > + */
> > +reset_at_el3:
> > + mov_64 x0, SCTLR_EL3_RESET
> > + msr sctlr_el3, x0
> > + isb
> >
> > -1: mov x0, #0x30 // RES1
> > + mov x0, #0x30 // RES1
> > orr x0, x0, #(1 << 0) // Non-secure EL1
> > orr x0, x0, #(1 << 8) // HVC enable
> >
> > @@ -135,10 +135,54 @@ ASM_FUNC(_start)
> > ldr x0, =COUNTER_FREQ
> > msr cntfrq_el0, x0
> >
> > + cpuid x0, x1
> > + bl find_logical_id
> > + cmp x0, #MPIDR_INVALID
> > + b.eq err_invalid_id
> > + bl setup_stack
> > +
> > bl gic_secure_init
> >
> > b start_el3
> >
> > + /*
> > + * EL2 initialization
> > + */
> > +reset_at_el2:
> > + // Ensure E2H is not in use
> > + mov_64 x0, HCR_EL2_RESET
> > + msr hcr_el2, x0
> > + isb
> > +
> > + mov_64 x0, SCTLR_EL2_RESET
> > + msr sctlr_el2, x0
> > + isb
> > +
> > + b reset_no_el3
> > +
> > + /*
> > + * EL1 initialization
> > + */
> > +reset_at_el1:
> > + mov_64 x0, SCTLR_EL1_RESET
> > + msr sctlr_el1, x0
> > + isb
> > +
> > + b reset_no_el3
> > +
> > +reset_no_el3:
> > + cpuid x0, x1
> > + bl find_logical_id
> > + cmp x0, #MPIDR_INVALID
> > + b.eq err_invalid_id
> > + bl setup_stack
> > +
> > + mov w0, #1
> > + ldr x1, =flag_no_el3
> > + str w0, [x1]
> > +
> > + b start_no_el3
> > +
> > err_invalid_id:
> > b .
> >
> > diff --git a/arch/aarch64/include/asm/cpu.h b/arch/aarch64/include/asm/cpu.h
> > index 1053414..1e9141a 100644
> > --- a/arch/aarch64/include/asm/cpu.h
> > +++ b/arch/aarch64/include/asm/cpu.h
> > @@ -14,6 +14,32 @@
> > #define MPIDR_ID_BITS 0xff00ffffff
> >
> > #define CURRENTEL_EL3 (3 << 2)
> > +#define CURRENTEL_EL2 (2 << 2)
> > +#define CURRENTEL_EL1 (1 << 2)
> > +
> > +/*
> > + * RES1 bit definitions definitions as of ARM DDI 0487G.b
> > + *
> > + * These includes bits which are RES1 in some configurations.
> > + */
> > +#define SCTLR_EL3_RES1 (BIT(29) | BIT(28) | BIT(23) | BIT(22) | \
> > + BIT(18) | BIT(16) | BIT(11) | BIT(5) | BIT(4))
> > +
> > +#define SCTLR_EL2_RES1 (BIT(29) | BIT(28) | BIT(23) | BIT(22) | \
> > + BIT(18) | BIT(16) | BIT(11) | BIT(5) | BIT(4))
>
> I compared all bits against the ARM ARM and the kernel version for EL2,
> that looks correct to me.
>
> > +
> > +#define SCTLR_EL1_RES1 (BIT(29) | BIT(28) | BIT(23) | BIT(22) | \
> > + BIT(11) | BIT(8) | BIT(7) | BIT(4))
>
> - The kernel sets TSCXT(bit[20]), and the ARM ARM says that the value
> should be RES1 if FEAT_CSV2_* is not implemented. Should we copy this?
Yes, we should. I'll go and fold that in.
> - The kernel clears ITD(bit[7]), and the ARM ARM says it's *Otherwise* RES1
> (no AArch32 in EL0). I feel like we should not disable IT instructions in
> EL0 needlessly?
Per the ARM ARM the bit resets to an UNKNOWN value, and so per our usual
policy the kernel must initialize that before it can depend upon it, and
IIUC you say the kernel already does so.
So it shouldn't matter what the boot-wrapper does, and for consitency, I
think the boot-wrapper should set this to 0b1.
> - I also feel like we should set CP15BEN(bit[5]), for similar reasons.
I agree.
> Granted those bits affect only EL0 execution, which we don't care about in
> the boot-wrapper, but I was wondering if we should change those anyway? At
> least bit 20?
I'll set all three of those bits. as above.
> > +#define HCR_EL2_RES1 (BIT(1))
>
> Should we set RW(bit[31]), just to be safe? Not sure this is explicitly
> mentioned somewhere, but is the boot flow when we are entered in EL2 to
> stay in EL2 and launch the kernel in there as well?
I don't think we need to currently. HCR_EL2.RW is UNKNOWN out-of-reset,
is RAO/WI rather than RES1, and only affects lower ELs, so per usual
policy we leave this to the kernel to initialize. Of the configurations
this could affect:
* BW @ EL3 + {psci,spin}
Kernel entered at EL2 on all PEs.
* BW @ EL2 + spin-table
Kernel entered at EL2 on all PEs.
* BW @ EL2 + PSCI
Kernel entered at EL2 on boot PE.
BROKEN due to PSCI conduit anyhow.
I'd like to clean up the EL + boot-method combinations, but this
reqiures some more work which I had punted out of this series for now.
For now, I'd prefer to leave this as-is. I would like to fix this as
part of a subsequent boot-method cleanup, configuring HCR_EL2.RW as part
of the kernel handover logic later in the boot flow.
>
> > +
> > +/*
> > + * Initial register values required for the boot-wrapper to run out-of-reset.
> > + */
> > +#define SCTLR_EL3_RESET SCTLR_EL3_RES1
> > +#define SCTLR_EL2_RESET SCTLR_EL2_RES1
> > +#define SCTLR_EL1_RESET SCTLR_EL1_RES1
> > +#define HCR_EL2_RESET HCR_EL2_RES1
> >
> > #define ID_AA64PFR0_EL1_GIC BITS(27, 24)
> >
> > @@ -43,7 +69,6 @@
> > #define ZCR_EL3_LEN_MASK 0x1ff
> >
> > #define SCTLR_EL1_CP15BEN (1 << 5)
> > -#define SCTLR_EL1_RES1 (3 << 28 | 3 << 22 | 1 << 11)
> >
> > #ifdef KERNEL_32
> > /* 32-bit kernel decompressor uses CP15 barriers */
> > #define SCTLR_EL1_KERNEL (SCTLR_EL1_RES1 | SCTLR_EL1_CP15BEN)
>
> So I wonder if this actually works? The ARMv7 version of SCTLR
> differs in some bits from both the ARMv8 AArch32 version and more
> importantly the AArch64 version.
Hmm. SCTLR_EL1 is architecturally mapped to SCTLR, and for some reason I
thought that applied field-wise, but IIUC you're saying that applies
bit-wise, right?
The ARM ARM is delightfully unclear about this, which is reather
unfortunate. Per the glossary in ARM DDI 0487G.b:
| Where this manual describes a register as being architecturally mapped
| to another register, this indicates that, in an implementation that
| supports both of the registers, the two registers access the same
| state.
That being the case, we'd need to do likewise for all the other
registers accessible at kernel ELs, right?
> I had troubles the other day running the
> arm32 Linux kernel decompressor with some ARMv8 SCTLR_EL1 reset value. The
> decompressor code does only read-modify-write of SCTLR (probably to
> cover multiple architecture revisions), so some bits might stay wrong. In
> particular I think having bits 28 and 29 set caused problems.
> By looking at the ARMv7 ARM and with experimentation I came up
> with 0x00c00878 as a safe and working value.
Do we have any comments/documentation saying what the 32-bit kernel
actually needs/wants here?
> Shall we have a separate reset value for 32bit?
Currently, the 32-bit side of the boot-wrapper uses:
(3 << 22 | 1 << 11 | 1 << 5 | 3 << 4)
... noting that `1 << 5` and `3 << 4` overlap..
Which is:
* bit[22]: RES1
* bit[11]: RES1
* bit[5]: CP15BEN
* bit[4]: LSMAOE
Per the above, it sounds like you think that's wrong too?
Whatever we do, the 32-bit/64-bit boot-wrapper should program something
functionally equivalent when booting a 32-bit kernel.
Thanks,
Mark.
More information about the linux-arm-kernel
mailing list