Re: [PATCH v7 5/6] riscv: vector: optimize vstate operations

[Conor Dooley <conor@kernel.org>]

[-- 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 --]