| Message ID | 20200925160722.27155-4-mark.rutland@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: remove set_fs() and friends | expand |
Hi Mark, On 25/09/2020 17:07, Mark Rutland wrote: > As with SCTLR_ELx and other control registers, some PSTATE bits are > UNKNOWN out-of-reset, and we may not be able to rely on hardware of > firmware (hardware or firmware) > to initialize them to our liking prior to entry to the kernel, > e.g. in the primary/secondary boot paths and return from idle/suspend. > > It would be more robust (and easier to reason about) if we consistently > initialized PSTATE to a default value, as we do with control registers. > This will ensure that the kernel is not adversely affected by bits it is > not aware of, e.g. when support for a feature such as PAN/UAO is > disabled. > > This patch ensures that PSTATE is consistently initialized at boot time > via an ERET. This is not intended to relax the existing requirements > (e.g. DAIF bits must still be set prior to entering the kernel). For > features detected dynamically (which may require system-wide support), > it is still necessary to subsequently modify PSTATE. > diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S > index 9bb07be45249a..b8ee9a62c9b61 100644 > --- a/arch/arm64/kernel/head.S > +++ b/arch/arm64/kernel/head.S > @@ -494,17 +494,22 @@ EXPORT_SYMBOL(kimage_vaddr) > * booted in EL1 or EL2 respectively. > */ > SYM_FUNC_START(init_kernel_el) > - msr SPsel, #1 // We want to use SP_EL{1,2} > mrs x0, CurrentEL > cmp x0, #CurrentEL_EL2 > - b.eq 1f > + b.eq init_el2 > + > +SYM_INNER_LABEL(init_el1, SYM_L_LOCAL) > mov_q x0, INIT_SCTLR_EL1_MMU_OFF > msr sctlr_el1, x0 > - mov w0, #BOOT_CPU_MODE_EL1 // This cpu booted in EL1 > isb Could you add a comment covering why this ISB isn't rolled into the ERET? I'm guessing its in case SCTLR_EL1 previously had v8.5s EOS 'Exception Exit is Context Synchronizing' clear. > - ret > + mov_q x0, INIT_PSTATE_EL1 > + msr spsr_el1, x0 > + msr elr_el1, lr > + mov w0, #BOOT_CPU_MODE_EL1 > + eret Thanks, James
On Wed, Sep 30, 2020 at 05:09:47PM +0100, James Morse wrote: > Hi Mark, > > On 25/09/2020 17:07, Mark Rutland wrote: > > As with SCTLR_ELx and other control registers, some PSTATE bits are > > UNKNOWN out-of-reset, and we may not be able to rely on hardware of > > firmware > > (hardware or firmware) Whoops; fixed! [...] > > diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S > > index 9bb07be45249a..b8ee9a62c9b61 100644 > > --- a/arch/arm64/kernel/head.S > > +++ b/arch/arm64/kernel/head.S > > @@ -494,17 +494,22 @@ EXPORT_SYMBOL(kimage_vaddr) > > * booted in EL1 or EL2 respectively. > > */ > > SYM_FUNC_START(init_kernel_el) > > - msr SPsel, #1 // We want to use SP_EL{1,2} > > mrs x0, CurrentEL > > cmp x0, #CurrentEL_EL2 > > - b.eq 1f > > + b.eq init_el2 > > + > > +SYM_INNER_LABEL(init_el1, SYM_L_LOCAL) > > mov_q x0, INIT_SCTLR_EL1_MMU_OFF > > msr sctlr_el1, x0 > > - mov w0, #BOOT_CPU_MODE_EL1 // This cpu booted in EL1 > > > isb > > Could you add a comment covering why this ISB isn't rolled into the ERET? > I'm guessing its in case SCTLR_EL1 previously had v8.5s EOS 'Exception Exit is Context > Synchronizing' clear. Yup, and I had a vague fear that we may gain new bits in future along similar lines. I'll add a note to the top of init_kernel_el, since it applies to all of the ERET cases. I'll also add a (pessimistic) ISB to the install_el2_stub return path to make this clear/consistent. Thanks, Mark.
diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h index 966ed30ed5f7b..366075f228eb3 100644 --- a/arch/arm64/include/asm/ptrace.h +++ b/arch/arm64/include/asm/ptrace.h @@ -16,6 +16,11 @@ #define CurrentEL_EL1 (1 << 2) #define CurrentEL_EL2 (2 << 2) +#define INIT_PSTATE_EL1 \ + (PSR_D_BIT | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT | PSR_MODE_EL1h) +#define INIT_PSTATE_EL2 \ + (PSR_D_BIT | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT | PSR_MODE_EL2h) + /* * PMR values used to mask/unmask interrupts. * diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S index 9bb07be45249a..b8ee9a62c9b61 100644 --- a/arch/arm64/kernel/head.S +++ b/arch/arm64/kernel/head.S @@ -494,17 +494,22 @@ EXPORT_SYMBOL(kimage_vaddr) * booted in EL1 or EL2 respectively. */ SYM_FUNC_START(init_kernel_el) - msr SPsel, #1 // We want to use SP_EL{1,2} mrs x0, CurrentEL cmp x0, #CurrentEL_EL2 - b.eq 1f + b.eq init_el2 + +SYM_INNER_LABEL(init_el1, SYM_L_LOCAL) mov_q x0, INIT_SCTLR_EL1_MMU_OFF msr sctlr_el1, x0 - mov w0, #BOOT_CPU_MODE_EL1 // This cpu booted in EL1 isb - ret + mov_q x0, INIT_PSTATE_EL1 + msr spsr_el1, x0 + msr elr_el1, lr + mov w0, #BOOT_CPU_MODE_EL1 + eret -1: mov_q x0, INIT_SCTLR_EL2_MMU_OFF +SYM_INNER_LABEL(init_el2, SYM_L_LOCAL) + mov_q x0, INIT_SCTLR_EL2_MMU_OFF msr sctlr_el2, x0 #ifdef CONFIG_ARM64_VHE @@ -613,9 +618,12 @@ set_hcr: cbz x2, install_el2_stub - mov w0, #BOOT_CPU_MODE_EL2 // This CPU booted in EL2 isb - ret + mov_q x0, INIT_PSTATE_EL2 + msr spsr_el2, x0 + msr elr_el2, lr + mov w0, #BOOT_CPU_MODE_EL2 + eret SYM_INNER_LABEL(install_el2_stub, SYM_L_LOCAL) /* @@ -648,11 +656,10 @@ SYM_INNER_LABEL(install_el2_stub, SYM_L_LOCAL) msr vbar_el2, x0 /* spsr */ - mov x0, #(PSR_F_BIT | PSR_I_BIT | PSR_A_BIT | PSR_D_BIT |\ - PSR_MODE_EL1h) + mov x0, #INIT_PSTATE_EL1 msr spsr_el2, x0 msr elr_el2, lr - mov w0, #BOOT_CPU_MODE_EL2 // This CPU booted in EL2 + mov w0, #BOOT_CPU_MODE_EL2 eret SYM_FUNC_END(init_kernel_el)
As with SCTLR_ELx and other control registers, some PSTATE bits are UNKNOWN out-of-reset, and we may not be able to rely on hardware of firmware to initialize them to our liking prior to entry to the kernel, e.g. in the primary/secondary boot paths and return from idle/suspend. It would be more robust (and easier to reason about) if we consistently initialized PSTATE to a default value, as we do with control registers. This will ensure that the kernel is not adversely affected by bits it is not aware of, e.g. when support for a feature such as PAN/UAO is disabled. This patch ensures that PSTATE is consistently initialized at boot time via an ERET. This is not intended to relax the existing requirements (e.g. DAIF bits must still be set prior to entering the kernel). For features detected dynamically (which may require system-wide support), it is still necessary to subsequently modify PSTATE. Signed-off-by: Mark Rutland <mark.rutland@arm.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Christoph Hellwig <hch@lst.de> Cc: James Morse <james.morse@arm.com> Cc: Will Deacon <will@kernel.org> --- arch/arm64/include/asm/ptrace.h | 5 +++++ arch/arm64/kernel/head.S | 27 +++++++++++++++++---------- 2 files changed, 22 insertions(+), 10 deletions(-)