* Re: [PATCH v7 5/6] riscv: vector: optimize vstate operations
2026-11-11 18:43 ` [PATCH v7 5/6] riscv: vector: optimize vstate operations Andy Chiu
@ 2026-11-11 23:00 ` Conor Dooley
0 siblings, 0 replies; 2+ messages in thread
From: Conor Dooley @ 2026-11-11 23:00 UTC (permalink / raw)
To: Andy Chiu
Cc: Jonathan Corbet, Shuah Khan, Randy Dunlap, Paul Walmsley,
Palmer Dabbelt, Albert Ou, Alexandre Ghiti, linux-doc,
linux-riscv, andybnac, dfustini, greentime.hu,
Mike Rapoport (Microsoft), Arnd Bergmann, Karl Mehltretter,
Borislav Petkov (AMD), Dapeng Mi, Marco Elver, Ethan Nelson-Moore,
Jeff Layton, Jisheng Zhang, Conor Dooley, Yong-Xuan Wang,
Anup Patel, Aurelien Jarno, daichengrong, Troy Mitchell,
Guobin Zhang, Guodong Xu, Andrew Jones, Charlie Jenkins,
Atish Patra, Deepak Gupta, Hui Wang, Pincheng Wang, Zong Li,
Vivian Wang, Rui Qi, Yunhui Cui, Samuel Holland, Zishun Yi,
Sergey Matyukevich
[-- Attachment #1: Type: text/plain, Size: 14393 bytes --]
On Wed, Nov 11, 2026 at 12:43:27PM -0600, Andy Chiu wrote:
> Introduce the riscv_novstateopt to control the optimization of vector
> state operations in the kernel space.
>
> The optimization enables vector in kernel mode and bypasses vector
> register nulling for user space on the syscall fast path when the boot
> argument "riscv_novstateopt" is not set
>
> Enabling this optimization yields the following performance benefits:
> - Cut 80 cycles latency for getpid on Ascalon.
> - 1.1% ~ 4.09% request throughput improvment on nginx workload with web
> serving size under 4KB
>
> Signed-off-by: Andy Chiu <tchiu@tenstorrent.com>
> ---
> Changelog v6:
> - upgrade it to use alternative and change default to enable the
> optimization
Why? Argument for the default being like this needs to be in the commit
message.
> Changelog v5:
> - new patch since v5
> ---
> .../admin-guide/kernel-parameters.txt | 13 +++++++++++++
> arch/riscv/include/asm/alternative-macros.h | 6 ++++++
> arch/riscv/include/asm/alternative.h | 3 ---
> arch/riscv/include/asm/cpufeature-macros.h | 6 ++++--
> arch/riscv/include/asm/vector.h | 16 +++++++++++++++-
> arch/riscv/kernel/cpufeature.c | 18 ++++++++++++++++++
> arch/riscv/kernel/entry.S | 15 +++++++++++++--
> arch/riscv/kernel/process.c | 3 +++
> arch/riscv/kernel/vector.c | 17 +++++++++++++++--
> 9 files changed, 87 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 0b3c140babd9..e1146ee6ab1e 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -6722,6 +6722,19 @@ Kernel parameters
> fcfi Disable user forward CFI ABI to userspace even if the
> landing pad extension is available.
>
> + riscv_novstateopt [RISCV,EARLY]
> + Disable the vector state optimization. By default the
> + kernel keeps Vector enabled while running in kernel
> + space, rather than toggling sstatus.VS on every use of
> + the kernel-mode vector, and skips nulling the vector
> + registers for user space. Pass this option to restore the
> + conservative behavior, which catches illegal use of
> + Vector in kernel code. The optimization roughly cuts
> + 30 ns of getpid() latency on a Blackhole x280 and gives
> + a 4.09% request throughput improvement for an nginx
> + workload serving a 2KB web page on a four-core
> + Ascalon-S.
> +
> ro [KNL] Mount root device read-only on boot
>
> rodata= [KNL,EARLY]
> diff --git a/arch/riscv/include/asm/alternative-macros.h b/arch/riscv/include/asm/alternative-macros.h
> index 629d884578da..45d9ae1e0aa8 100644
> --- a/arch/riscv/include/asm/alternative-macros.h
> +++ b/arch/riscv/include/asm/alternative-macros.h
> @@ -2,6 +2,12 @@
> #ifndef __ASM_ALTERNATIVE_MACROS_H
> #define __ASM_ALTERNATIVE_MACROS_H
>
> +#include <linux/wordpart.h>
> +#define PATCH_ID_CPUFEATURE_ID(p) lower_16_bits(p)
> +#define PATCH_ID_CPUFEATURE_VALUE(p) upper_16_bits(p)
> +
> +#define RISCV_CPUFEAT_VSTATEOPT 0x1
This isn't a cpu feature at all though, so it should not be named like that.
> +
> #ifdef __ASSEMBLER__
>
> .macro ALT_ENTRY oldptr newptr vendor_id patch_id new_len
> diff --git a/arch/riscv/include/asm/alternative.h b/arch/riscv/include/asm/alternative.h
> index 6be7b2b6ade9..bc2ecaa9125a 100644
> --- a/arch/riscv/include/asm/alternative.h
> +++ b/arch/riscv/include/asm/alternative.h
> @@ -16,9 +16,6 @@
> #include <linux/stddef.h>
> #include <asm/hwcap.h>
>
> -#define PATCH_ID_CPUFEATURE_ID(p) lower_16_bits(p)
> -#define PATCH_ID_CPUFEATURE_VALUE(p) upper_16_bits(p)
> -
> #define RISCV_ALTERNATIVES_BOOT 0 /* alternatives applied during regular boot */
> #define RISCV_ALTERNATIVES_MODULE 1 /* alternatives applied during module-init */
> #define RISCV_ALTERNATIVES_EARLY_BOOT 2 /* alternatives applied before mmu start */
> diff --git a/arch/riscv/include/asm/cpufeature-macros.h b/arch/riscv/include/asm/cpufeature-macros.h
> index adaf9e3fb25c..7bd900899bb0 100644
> --- a/arch/riscv/include/asm/cpufeature-macros.h
> +++ b/arch/riscv/include/asm/cpufeature-macros.h
> @@ -45,14 +45,16 @@ static __always_inline bool __riscv_has_extension_unlikely(const unsigned long v
>
> static __always_inline bool riscv_has_extension_unlikely(const unsigned long ext)
> {
> - compiletime_assert(ext < RISCV_ISA_EXT_MAX, "ext must be < RISCV_ISA_EXT_MAX");
> + int realext = PATCH_ID_CPUFEATURE_ID(ext);
> + compiletime_assert(realext < RISCV_ISA_EXT_MAX, "ext must be < RISCV_ISA_EXT_MAX");
>
> return __riscv_has_extension_unlikely(STANDARD_EXT, ext);
> }
Please don't abuse riscv_has_extension_*() for things that are not
extensions. I don't mind if you do something that wraps
__riscv_has_extension_*() to make use of the underlying infrastructure
but I am against muddying the standard extension space with linux internal
stuff - particularly in this manner because this:
> +static __always_inline bool has_vstate_opt(void)
> +{
> + return riscv_has_extension_likely((RISCV_CPUFEAT_VSTATEOPT << 16) | RISCV_ISA_EXT_ZVE32X);
> +}
is incredibly confusing and error prone.
Recently enough I talked about something very very similar with Jisheng,
in the context of using an alternative for misaligned access, and I
said:
| ...I'd rather see __riscv_has_extension_*() get renamed
| to __riscv_has_cap*(), so that things like unaligned access speed get a
| different namespace to standard and vendor extensions. IOW you'd end up
| with
|
| static __always_inline bool riscv_has_extension_unlikely(const unsigned long ext)
| {
| compiletime_assert(ext < RISCV_ISA_EXT_MAX, "ext must be < RISCV_ISA_EXT_MAX");
|
| return __riscv_has_cap_unlikely(STANDARD_EXT, ext);
| }
|
| static __always_inline bool riscv_has_vendor_extension_unlikely(const unsigned long vendor,
| const unsigned long ext)
| {
| if (!IS_ENABLED(CONFIG_RISCV_ISA_VENDOR_EXT))
| return false;
|
| return __riscv_has_cap_unlikely(vendor, ext + RISCV_VENDOR_EXT_ALTERNATIVES_BASE);
| }
|
| static __always_inline bool riscv_has_cap_unlikely(const unsigned long cap)
| {
| return __riscv_has_cap_unlikely(LINUX_CAP, cap);
| }
|
| Currently standard extensions use 0 as the vendor, perhaps we could then
| use -1 (the value is an unsigned long) for non extension related caps?
What's here, with this realext business definitely looks like a hack.
Doing what I suggest above allows you to have a real key that you can
set, just like the vendor extensions do. You can call a function from
riscv_fill_hwcap() to populate the bitmap and modify
_apply_alternatives() to additionally patch this sort of thing.
Cheers,
Conor.
pw-bot: changes-requested
>
> static __always_inline bool riscv_has_extension_likely(const unsigned long ext)
> {
> - compiletime_assert(ext < RISCV_ISA_EXT_MAX, "ext must be < RISCV_ISA_EXT_MAX");
> + int realext = PATCH_ID_CPUFEATURE_ID(ext);
> + compiletime_assert(realext < RISCV_ISA_EXT_MAX, "ext must be < RISCV_ISA_EXT_MAX");
>
> return __riscv_has_extension_likely(STANDARD_EXT, ext);
> }
> diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h
> index e83f9e0046a9..745ac3ca67b7 100644
> --- a/arch/riscv/include/asm/vector.h
> +++ b/arch/riscv/include/asm/vector.h
> @@ -49,6 +49,7 @@
> _res; \
> })
>
> +extern bool riscv_v_vstate_opt;
> extern unsigned long riscv_v_vsize;
> int riscv_v_setup_vsize(void);
> bool insn_is_vector(u32 insn_buf);
> @@ -78,6 +79,11 @@ static __always_inline bool has_vector(void)
> return riscv_has_extension_unlikely(RISCV_ISA_EXT_ZVE32X);
> }
>
> +static __always_inline bool has_vstate_opt(void)
> +{
> + return riscv_has_extension_likely((RISCV_CPUFEAT_VSTATEOPT << 16) | RISCV_ISA_EXT_ZVE32X);
> +}
> +
> static __always_inline bool has_xtheadvector_no_alternatives(void)
> {
> if (IS_ENABLED(CONFIG_RISCV_ISA_XTHEADVECTOR))
> @@ -122,6 +128,9 @@ static inline bool riscv_v_vstate_query(struct pt_regs *regs)
>
> static __always_inline void riscv_v_enable(void)
> {
> + if (has_vstate_opt())
> + return;
> +
> if (has_xtheadvector())
> csr_set(CSR_SSTATUS, SR_VS_THEAD);
> else
> @@ -130,6 +139,9 @@ static __always_inline void riscv_v_enable(void)
>
> static __always_inline void riscv_v_disable(void)
> {
> + if (has_vstate_opt())
> + return;
> +
> if (has_xtheadvector())
> csr_clear(CSR_SSTATUS, SR_VS_THEAD);
> else
> @@ -336,7 +348,8 @@ static inline void riscv_v_vstate_set_restore(struct task_struct *task,
> static inline void riscv_v_vstate_discard(struct pt_regs *regs)
> {
> if (__riscv_v_vstate_check_gt(regs->status, INITIAL)) {
> - riscv_v_vstate_set_restore(current, regs);
> + if (!has_vstate_opt())
> + riscv_v_vstate_set_restore(current, regs);
> riscv_v_vstate_init(regs);
> }
> }
> @@ -422,6 +435,7 @@ struct pt_regs;
>
> static inline int riscv_v_setup_vsize(void) { return -EOPNOTSUPP; }
> static __always_inline bool has_vector(void) { return false; }
> +static __always_inline bool has_vstate_opt(void) { return false; }
> static __always_inline bool insn_is_vector(u32 insn_buf) { return false; }
> static __always_inline bool has_xtheadvector_no_alternatives(void) { return false; }
> static __always_inline bool has_xtheadvector(void) { return false; }
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 72bce616491f..041f6a35fba3 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -1294,6 +1294,19 @@ void riscv_clear_hypervisor_csr(void)
> csr_clear(CSR_HSTATUS, HSTATUS_HU);
> }
>
> +/*
> + * Leaving Vector enabled while in the kernel relies on the vstate alternatives
> + * being patched in. It can be turned off on the command line to catch illegal
> + * use of Vector in kernel code.
> + */
> +bool riscv_v_vstate_opt = true;
> +static int __init riscv_novstateopt_setup(char *__unused)
> +{
> + riscv_v_vstate_opt = false;
> + return 0;
> +}
> +early_param("riscv_novstateopt", riscv_novstateopt_setup);
> +
> /*
> * Alternative patch sites consider 48 bits when determining when to patch
> * the old instruction sequence with the new. These bits are broken into a
> @@ -1322,6 +1335,11 @@ static bool riscv_cpufeature_patch_check(u16 id, u16 value)
> * then the alternative cannot be applied.
> */
> return riscv_cboz_block_size <= (1U << value);
> + case RISCV_ISA_EXT_ZVE32X:
> + if (value == RISCV_CPUFEAT_VSTATEOPT)
> + return riscv_v_vstate_opt;
> +
> + return true;
> }
>
> return false;
> diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
> index 628b85e3915a..bb4787711f2c 100644
> --- a/arch/riscv/kernel/entry.S
> +++ b/arch/riscv/kernel/entry.S
> @@ -6,6 +6,7 @@
>
> #include <linux/init.h>
> #include <linux/linkage.h>
> +#include <linux/stringify.h>
>
> #include <asm/alternative-macros.h>
> #include <asm/asm.h>
> @@ -175,10 +176,13 @@ SYM_CODE_START(handle_exception)
> * Disable user-mode memory access as it should only be set in the
> * actual user copy routines.
> *
> - * Disable the FPU/Vector to detect illegal usage of floating point
> + * Disable the FPU to detect illegal usage of floating point
> * or vector in kernel space.
> */
> - li t0, SR_SUM | SR_FS_VS
> + li t0, SR_SUM | SR_FS
> + ALTERNATIVE(__stringify(ori t0, t0, SR_VS), "nop", 0,
> + (RISCV_CPUFEAT_VSTATEOPT << 16) | RISCV_ISA_EXT_ZVE32X,
> + CONFIG_RISCV_ISA_V)
> #ifdef CONFIG_64BIT
> li t1, SR_ELP
> or t0, t0, t1
> @@ -186,6 +190,13 @@ SYM_CODE_START(handle_exception)
>
> REG_L s0, TASK_TI_USER_SP(tp)
> csrrc s1, CSR_STATUS, t0
> + ALTERNATIVE("j .Lskip_v_enable", __stringify(andi t0, s1, SR_VS), 0,
> + (RISCV_CPUFEAT_VSTATEOPT << 16) | RISCV_ISA_EXT_ZVE32X,
> + CONFIG_RISCV_ISA_V)
> + bnez t0, .Lskip_v_enable
> + li a0, SR_VS_INITIAL
> + csrs CSR_STATUS, a0
> +.Lskip_v_enable:
> save_userssp s2, s1
> csrr s2, CSR_EPC
> csrr s3, CSR_TVAL
> diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c
> index 34c1d498d1ca..27146b3fa738 100644
> --- a/arch/riscv/kernel/process.c
> +++ b/arch/riscv/kernel/process.c
> @@ -258,6 +258,9 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args)
> /* Supervisor/Machine, irqs on: */
> childregs->status = SR_PP | SR_PIE;
>
> + if (has_vstate_opt())
> + riscv_v_vstate_init(childregs);
> +
> p->thread.s[0] = (unsigned long)args->fn;
> p->thread.s[1] = (unsigned long)args->fn_arg;
> p->thread.ra = (unsigned long)ret_from_fork_kernel_asm;
> diff --git a/arch/riscv/kernel/vector.c b/arch/riscv/kernel/vector.c
> index 6fd541f5d5cb..eec9deff8da7 100644
> --- a/arch/riscv/kernel/vector.c
> +++ b/arch/riscv/kernel/vector.c
> @@ -12,6 +12,7 @@
> #include <linux/prctl.h>
>
> #include <asm/thread_info.h>
> +#include <asm/cpufeature.h>
> #include <asm/processor.h>
> #include <asm/insn.h>
> #include <asm/vector.h>
> @@ -69,6 +70,16 @@ void riscv_v_ucontext_save(struct task_struct *tsk)
> int riscv_v_setup_vsize(void)
> {
> unsigned long this_vsize;
> + bool v_always_on = false;
> +
> + /*
> + * has_vstate_opt() cannot be used here if called from riscv_fill_hwcap(), before
> + * apply_boot_alternatives(),
> + */
> + if (__riscv_isa_extension_available(NULL, RISCV_ISA_EXT_ZVE32X) && riscv_v_vstate_opt) {
> + v_always_on = true;
> + csr_set(CSR_SSTATUS, SR_VS_INITIAL);
> + }
>
> /*
> * There are 32 vector registers with vlenb length.
> @@ -81,9 +92,11 @@ int riscv_v_setup_vsize(void)
> return 0;
> }
>
> - riscv_v_enable();
> + if (!v_always_on)
> + riscv_v_enable();
> this_vsize = csr_read(CSR_VLENB) * 32;
> - riscv_v_disable();
> + if (!v_always_on)
> + riscv_v_disable();
>
> if (!riscv_v_vsize) {
> riscv_v_vsize = this_vsize;
> --
> 2.43.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 2+ messages in thread