| Message ID | 20231124163510.1835740-6-joey.gouly@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Permission Overlay Extension | expand |
On Fri, Nov 24, 2023 at 04:34:50PM +0000, Joey Gouly wrote: > +static void flush_poe(void) > +{ > + if (system_supports_poe()) > + write_sysreg_s(POR_EL0_INIT, SYS_POR_EL0); > +} Here we have no isb()... > +static void permission_overlay_switch(struct task_struct *next) > +{ > + if (system_supports_poe()) { > + current->thread.por_el0 = read_sysreg_s(SYS_POR_EL0); > + if (current->thread.por_el0 != next->thread.por_el0) { > + write_sysreg_s(next->thread.por_el0, SYS_POR_EL0); > + isb(); > + } > + } > +} ...but here we do, I'd expect them to be consistent. If the barrier is needed it'd probably be helpful to have a comment explaining why we need it before the ERET.
On Fri, Nov 24, 2023 at 04:34:50PM +0000, Joey Gouly wrote: > @@ -498,6 +508,17 @@ static void erratum_1418040_new_exec(void) > preempt_enable(); > } > > +static void permission_overlay_switch(struct task_struct *next) > +{ > + if (system_supports_poe()) { > + current->thread.por_el0 = read_sysreg_s(SYS_POR_EL0); > + if (current->thread.por_el0 != next->thread.por_el0) { > + write_sysreg_s(next->thread.por_el0, SYS_POR_EL0); > + isb(); > + } > + } Nitpick: use "if (!system_supports_poe()) return;" to avoid too much indentation. W.r.t. the isb(), I think we accumulated quite a lot on this path. It might be worth going through them and having one at the end, where possible.
On Sat, Nov 25, 2023 at 12:02:49PM +0000, Mark Brown wrote: > On Fri, Nov 24, 2023 at 04:34:50PM +0000, Joey Gouly wrote: > > > +static void flush_poe(void) > > +{ > > + if (system_supports_poe()) > > + write_sysreg_s(POR_EL0_INIT, SYS_POR_EL0); > > +} > > Here we have no isb()... My immediate thought was that we'd not care about the ISB here since we'll have an ERET before getting to EL0. However, we may have some LDTR/STTR populating the new process args page on exec which may, in theory, pick up a stale POR_EL0.
On Thu, Dec 07, 2023 at 01:55:31PM +0000, Catalin Marinas wrote: > On Sat, Nov 25, 2023 at 12:02:49PM +0000, Mark Brown wrote: > > On Fri, Nov 24, 2023 at 04:34:50PM +0000, Joey Gouly wrote: > > > +static void flush_poe(void) > > > +{ > > > + if (system_supports_poe()) > > > + write_sysreg_s(POR_EL0_INIT, SYS_POR_EL0); > > > +} > > Here we have no isb()... > My immediate thought was that we'd not care about the ISB here since > we'll have an ERET before getting to EL0. However, we may have some > LDTR/STTR populating the new process args page on exec which may, in > theory, pick up a stale POR_EL0. Yeah, it was a combination of the inconsistency and the lack of clarity over there being a path which could potentially use POR_EL0 before ERET. We at least probably need some comments with regard to the requirements here.
diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h index f6d416fe49b0..6870e4d46334 100644 --- a/arch/arm64/include/asm/cpufeature.h +++ b/arch/arm64/include/asm/cpufeature.h @@ -819,6 +819,12 @@ static inline bool system_supports_tlb_range(void) return alternative_has_cap_unlikely(ARM64_HAS_TLB_RANGE); } +static inline bool system_supports_poe(void) +{ + return IS_ENABLED(CONFIG_ARM64_POE) && + alternative_has_cap_unlikely(ARM64_HAS_S1POE); +} + int do_emulate_mrs(struct pt_regs *regs, u32 sys_reg, u32 rt); bool try_emulate_mrs(struct pt_regs *regs, u32 isn); diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h index e5bc54522e71..b3ad719c2d0c 100644 --- a/arch/arm64/include/asm/processor.h +++ b/arch/arm64/include/asm/processor.h @@ -179,6 +179,7 @@ struct thread_struct { u64 sctlr_user; u64 svcr; u64 tpidr2_el0; + u64 por_el0; }; static inline unsigned int thread_get_vl(struct thread_struct *thread, diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h index 9c2caf0efdc7..77a4797d0d54 100644 --- a/arch/arm64/include/asm/sysreg.h +++ b/arch/arm64/include/asm/sysreg.h @@ -1052,6 +1052,9 @@ #define POE_RXW UL(0x7) #define POE_MASK UL(0xf) +/* Initial value for Permission Overlay Extension for EL0 */ +#define POR_EL0_INIT POE_RXW + #define ARM64_FEATURE_FIELD_BITS 4 /* Defined for compatibility only, do not add new users. */ diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c index 7387b68c745b..fc899c12d759 100644 --- a/arch/arm64/kernel/process.c +++ b/arch/arm64/kernel/process.c @@ -271,12 +271,19 @@ static void flush_tagged_addr_state(void) clear_thread_flag(TIF_TAGGED_ADDR); } +static void flush_poe(void) +{ + if (system_supports_poe()) + write_sysreg_s(POR_EL0_INIT, SYS_POR_EL0); +} + void flush_thread(void) { fpsimd_flush_thread(); tls_thread_flush(); flush_ptrace_hw_breakpoint(current); flush_tagged_addr_state(); + flush_poe(); } void arch_release_task_struct(struct task_struct *tsk) @@ -374,6 +381,9 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args) if (system_supports_tpidr2()) p->thread.tpidr2_el0 = read_sysreg_s(SYS_TPIDR2_EL0); + if (system_supports_poe()) + p->thread.por_el0 = read_sysreg_s(SYS_POR_EL0); + if (stack_start) { if (is_compat_thread(task_thread_info(p))) childregs->compat_sp = stack_start; @@ -498,6 +508,17 @@ static void erratum_1418040_new_exec(void) preempt_enable(); } +static void permission_overlay_switch(struct task_struct *next) +{ + if (system_supports_poe()) { + current->thread.por_el0 = read_sysreg_s(SYS_POR_EL0); + if (current->thread.por_el0 != next->thread.por_el0) { + write_sysreg_s(next->thread.por_el0, SYS_POR_EL0); + isb(); + } + } +} + /* * __switch_to() checks current->thread.sctlr_user as an optimisation. Therefore * this function must be called with preemption disabled and the update to @@ -533,6 +554,7 @@ struct task_struct *__switch_to(struct task_struct *prev, ssbs_thread_switch(next); erratum_1418040_thread_switch(next); ptrauth_thread_switch_user(next); + permission_overlay_switch(next); /* * Complete any pending TLB or cache maintenance on this CPU in case
POR_EL0 is a register that can be modified by userspace directly, so it must be context switched. Signed-off-by: Joey Gouly <joey.gouly@arm.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will@kernel.org> --- arch/arm64/include/asm/cpufeature.h | 6 ++++++ arch/arm64/include/asm/processor.h | 1 + arch/arm64/include/asm/sysreg.h | 3 +++ arch/arm64/kernel/process.c | 22 ++++++++++++++++++++++ 4 files changed, 32 insertions(+)