| Message ID | 20240605205658.184399-3-samuel.holland@sifive.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | riscv: Per-thread envcfg CSR support | expand |
On Wed, Jun 05, 2024 at 01:56:46PM -0700, Samuel Holland wrote: >Some bits in the [ms]envcfg CSR, such as the CFI state and pointer >masking mode, need to be controlled on a per-thread basis. Support this >by keeping a copy of the CSR value in struct thread_struct and writing >it during context switches. It is safe to discard the old CSR value >during the context switch because the CSR is modified only by software, >so the CSR will remain in sync with the copy in thread_struct. > >Use ALTERNATIVE directly instead of riscv_has_extension_unlikely() to >minimize branchiness in the context switching code. > >Since thread_struct is copied during fork(), setting the value for the >init task sets the default value for all other threads. > >Signed-off-by: Samuel Holland <samuel.holland@sifive.com> >--- > > arch/riscv/include/asm/processor.h | 1 + > arch/riscv/include/asm/switch_to.h | 8 ++++++++ > arch/riscv/kernel/cpufeature.c | 2 +- > 3 files changed, 10 insertions(+), 1 deletion(-) > >diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h >index 68c3432dc6ea..0838922bd1c8 100644 >--- a/arch/riscv/include/asm/processor.h >+++ b/arch/riscv/include/asm/processor.h >@@ -118,6 +118,7 @@ struct thread_struct { > unsigned long s[12]; /* s[0]: frame pointer */ > struct __riscv_d_ext_state fstate; > unsigned long bad_cause; >+ unsigned long envcfg; > u32 riscv_v_flags; > u32 vstate_ctrl; > struct __riscv_v_ext_state vstate; >diff --git a/arch/riscv/include/asm/switch_to.h b/arch/riscv/include/asm/switch_to.h >index 7594df37cc9f..9685cd85e57c 100644 >--- a/arch/riscv/include/asm/switch_to.h >+++ b/arch/riscv/include/asm/switch_to.h >@@ -70,6 +70,13 @@ static __always_inline bool has_fpu(void) { return false; } > #define __switch_to_fpu(__prev, __next) do { } while (0) > #endif > >+static inline void __switch_to_envcfg(struct task_struct *next) >+{ >+ asm volatile (ALTERNATIVE("nop", "csrw " __stringify(CSR_ENVCFG) ", %0", >+ 0, RISCV_ISA_EXT_XLINUXENVCFG, 1) >+ :: "r" (next->thread.envcfg) : "memory"); >+} >+ > extern struct task_struct *__switch_to(struct task_struct *, > struct task_struct *); > >@@ -103,6 +110,7 @@ do { \ > __switch_to_vector(__prev, __next); \ > if (switch_to_should_flush_icache(__next)) \ > local_flush_icache_all(); \ >+ __switch_to_envcfg(__next); \ > ((last) = __switch_to(__prev, __next)); \ > } while (0) Suggestion: Probably make this patch 1 > >diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c >index 2879e26dbcd8..1153b96346ae 100644 >--- a/arch/riscv/kernel/cpufeature.c >+++ b/arch/riscv/kernel/cpufeature.c >@@ -728,7 +728,7 @@ unsigned long riscv_get_elf_hwcap(void) > void riscv_user_isa_enable(void) > { > if (riscv_has_extension_unlikely(RISCV_ISA_EXT_ZICBOZ)) >- csr_set(CSR_ENVCFG, ENVCFG_CBZE); >+ current->thread.envcfg |= ENVCFG_CBZE; Suggestion: Squash this with current patch 1 and call it patch 2. > else if (any_cpu_has_zicboz) > pr_warn_once("Zicboz disabled as it is unavailable on some harts\n"); > } >-- >2.44.1 >
Hi Deepak, On 2024-06-07 4:59 PM, Deepak Gupta wrote: > On Wed, Jun 05, 2024 at 01:56:46PM -0700, Samuel Holland wrote: >> Some bits in the [ms]envcfg CSR, such as the CFI state and pointer >> masking mode, need to be controlled on a per-thread basis. Support this >> by keeping a copy of the CSR value in struct thread_struct and writing >> it during context switches. It is safe to discard the old CSR value >> during the context switch because the CSR is modified only by software, >> so the CSR will remain in sync with the copy in thread_struct. >> >> Use ALTERNATIVE directly instead of riscv_has_extension_unlikely() to >> minimize branchiness in the context switching code. >> >> Since thread_struct is copied during fork(), setting the value for the >> init task sets the default value for all other threads. >> >> Signed-off-by: Samuel Holland <samuel.holland@sifive.com> >> --- >> >> arch/riscv/include/asm/processor.h | 1 + >> arch/riscv/include/asm/switch_to.h | 8 ++++++++ >> arch/riscv/kernel/cpufeature.c | 2 +- >> 3 files changed, 10 insertions(+), 1 deletion(-) >> >> diff --git a/arch/riscv/include/asm/processor.h >> b/arch/riscv/include/asm/processor.h >> index 68c3432dc6ea..0838922bd1c8 100644 >> --- a/arch/riscv/include/asm/processor.h >> +++ b/arch/riscv/include/asm/processor.h >> @@ -118,6 +118,7 @@ struct thread_struct { >> unsigned long s[12]; /* s[0]: frame pointer */ >> struct __riscv_d_ext_state fstate; >> unsigned long bad_cause; >> + unsigned long envcfg; >> u32 riscv_v_flags; >> u32 vstate_ctrl; >> struct __riscv_v_ext_state vstate; >> diff --git a/arch/riscv/include/asm/switch_to.h >> b/arch/riscv/include/asm/switch_to.h >> index 7594df37cc9f..9685cd85e57c 100644 >> --- a/arch/riscv/include/asm/switch_to.h >> +++ b/arch/riscv/include/asm/switch_to.h >> @@ -70,6 +70,13 @@ static __always_inline bool has_fpu(void) { return false; } >> #define __switch_to_fpu(__prev, __next) do { } while (0) >> #endif >> >> +static inline void __switch_to_envcfg(struct task_struct *next) >> +{ >> + asm volatile (ALTERNATIVE("nop", "csrw " __stringify(CSR_ENVCFG) ", %0", >> + 0, RISCV_ISA_EXT_XLINUXENVCFG, 1) >> + :: "r" (next->thread.envcfg) : "memory"); >> +} >> + >> extern struct task_struct *__switch_to(struct task_struct *, >> struct task_struct *); >> >> @@ -103,6 +110,7 @@ do { \ >> __switch_to_vector(__prev, __next); \ >> if (switch_to_should_flush_icache(__next)) \ >> local_flush_icache_all(); \ >> + __switch_to_envcfg(__next); \ >> ((last) = __switch_to(__prev, __next)); \ >> } while (0) > > Suggestion: > Probably make this patch 1 > >> >> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c >> index 2879e26dbcd8..1153b96346ae 100644 >> --- a/arch/riscv/kernel/cpufeature.c >> +++ b/arch/riscv/kernel/cpufeature.c >> @@ -728,7 +728,7 @@ unsigned long riscv_get_elf_hwcap(void) >> void riscv_user_isa_enable(void) >> { >> if (riscv_has_extension_unlikely(RISCV_ISA_EXT_ZICBOZ)) >> - csr_set(CSR_ENVCFG, ENVCFG_CBZE); >> + current->thread.envcfg |= ENVCFG_CBZE; > > Suggestion: > Squash this with current patch 1 and call it patch 2. This reorganization doesn't work. If I add __switch_to_envcfg() first without this change, then the CSR would get zeroed out during the first context switch, so userspace cbo.zero would be broken after the first patch. Regards, Samuel >> else if (any_cpu_has_zicboz) >> pr_warn_once("Zicboz disabled as it is unavailable on some harts\n"); >> } >> -- >> 2.44.1 >>
diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h index 68c3432dc6ea..0838922bd1c8 100644 --- a/arch/riscv/include/asm/processor.h +++ b/arch/riscv/include/asm/processor.h @@ -118,6 +118,7 @@ struct thread_struct { unsigned long s[12]; /* s[0]: frame pointer */ struct __riscv_d_ext_state fstate; unsigned long bad_cause; + unsigned long envcfg; u32 riscv_v_flags; u32 vstate_ctrl; struct __riscv_v_ext_state vstate; diff --git a/arch/riscv/include/asm/switch_to.h b/arch/riscv/include/asm/switch_to.h index 7594df37cc9f..9685cd85e57c 100644 --- a/arch/riscv/include/asm/switch_to.h +++ b/arch/riscv/include/asm/switch_to.h @@ -70,6 +70,13 @@ static __always_inline bool has_fpu(void) { return false; } #define __switch_to_fpu(__prev, __next) do { } while (0) #endif +static inline void __switch_to_envcfg(struct task_struct *next) +{ + asm volatile (ALTERNATIVE("nop", "csrw " __stringify(CSR_ENVCFG) ", %0", + 0, RISCV_ISA_EXT_XLINUXENVCFG, 1) + :: "r" (next->thread.envcfg) : "memory"); +} + extern struct task_struct *__switch_to(struct task_struct *, struct task_struct *); @@ -103,6 +110,7 @@ do { \ __switch_to_vector(__prev, __next); \ if (switch_to_should_flush_icache(__next)) \ local_flush_icache_all(); \ + __switch_to_envcfg(__next); \ ((last) = __switch_to(__prev, __next)); \ } while (0) diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c index 2879e26dbcd8..1153b96346ae 100644 --- a/arch/riscv/kernel/cpufeature.c +++ b/arch/riscv/kernel/cpufeature.c @@ -728,7 +728,7 @@ unsigned long riscv_get_elf_hwcap(void) void riscv_user_isa_enable(void) { if (riscv_has_extension_unlikely(RISCV_ISA_EXT_ZICBOZ)) - csr_set(CSR_ENVCFG, ENVCFG_CBZE); + current->thread.envcfg |= ENVCFG_CBZE; else if (any_cpu_has_zicboz) pr_warn_once("Zicboz disabled as it is unavailable on some harts\n"); }
Some bits in the [ms]envcfg CSR, such as the CFI state and pointer masking mode, need to be controlled on a per-thread basis. Support this by keeping a copy of the CSR value in struct thread_struct and writing it during context switches. It is safe to discard the old CSR value during the context switch because the CSR is modified only by software, so the CSR will remain in sync with the copy in thread_struct. Use ALTERNATIVE directly instead of riscv_has_extension_unlikely() to minimize branchiness in the context switching code. Since thread_struct is copied during fork(), setting the value for the init task sets the default value for all other threads. Signed-off-by: Samuel Holland <samuel.holland@sifive.com> --- arch/riscv/include/asm/processor.h | 1 + arch/riscv/include/asm/switch_to.h | 8 ++++++++ arch/riscv/kernel/cpufeature.c | 2 +- 3 files changed, 10 insertions(+), 1 deletion(-)