Message-ID: <9D2DE979F179BFC6+aKUlbEZa2vvgmGdQ@LT-Guozexi>
Date: Wed, 20 Aug 2025 09:31:24 +0800
From: Troy Mitchell <troy.mitchell@...ux.spacemit.com>
To: Drew Fustini <fustini@...nel.org>, Palmer Dabbelt <palmer@...belt.com>,
	Paul Walmsley <paul.walmsley@...ive.com>,
	Alexandre Ghiti <alex@...ti.fr>
Cc: Samuel Holland <samuel.holland@...ive.com>,
	Björn Töpel <bjorn@...osinc.com>,
	Andy Chiu <andybnac@...il.com>,
	Conor Dooley <conor.dooley@...rochip.com>,
	Darius Rad <darius@...espec.com>,
	Vivian Wang <wangruikang@...as.ac.cn>,
	Florian Weimer <fweimer@...hat.com>,
	linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
	Drew Fustini <dfustini@...storrent.com>,
	Troy Mitchell <troy.mitchell@...ux.spacemit.com>
Subject: Re: [PATCH v3] riscv: Add sysctl to control discard of vstate on
 syscall entry

On Tue, Aug 19, 2025 at 02:40:21AM -0700, Drew Fustini wrote:
> From: Drew Fustini <dfustini@...storrent.com>
> 
> Vector registers are always clobbered in the syscall entry path to
> enforce the documented ABI that vector state is not preserved across
> syscalls. However, this operation can be slow on some RISC-V cores.
> To mitigate this performance impact, add a sysctl knob to control
> whether vector state is discarded in the syscall entry path:
> 
> /proc/sys/abi/riscv_v_vstate_discard
> 
> Valid values are:
> 
> 0: Vector state is not intentionally clobbered when entering a syscall
> 1: Vector state is always clobbered when entering a syscall
> 
> The initial state is controlled by CONFIG_RISCV_ISA_V_VSTATE_DISCARD.
> 
> Fixes: 9657e9b7d253 ("riscv: Discard vector state on syscalls")
> Signed-off-by: Drew Fustini <dfustini@...storrent.com>
> ---
> Changes in v3:
>  - Reword the Kconfig description to clarify that the sysctl can still
>    be changed during runtime regardless of the initial value chosen
>  - Improve the description of vstate clobbering and the sysctl in
>    section 3 of vector.rst
>  - v2: https://lore.kernel.org/linux-riscv/20250806-riscv_v_vstate_discard-v2-1-6bfd61b2c23b@kernel.org/
> 
> Changes in v2:
>  - Reword the description of the abi.riscv_v_vstate_discard sysctl to
>    clarify that option '0' does not preserve the vector state - it just
>    means that vector state will not always be clobbered in the syscall
>    path.
>  - Add clarification suggested by Palmer in v1 to the "Vector Register
>    State Across System Calls" documentation section.
>  - v1: https://lore.kernel.org/linux-riscv/20250719033912.1313955-1-fustini@kernel.org/
> 
> Test results:
> I've tested the impact of riscv_v_vstate_discard() on the SiFive X280
> cores [1] in the Tenstorrent Blackhole SoC [2]. The results from the
> Blackhole P100 [3] card show that discarding the vector registers
> increases null syscall latency by 25%.
> 
> The null syscall program [4] executes vsetvli and then calls getppid()
> in a loop. The average duration of getppid() is 198 ns when registers
> are clobbered in riscv_v_vstate_discard(). The average duration drops
> to 149 ns when riscv_v_vstate_discard() skips clobbering the registers
> because riscv_v_vstate_discard is set to 0.
> 
> $ sudo sysctl abi.riscv_v_vstate_discard=1
> abi.riscv_v_vstate_discard = 1
> 
> $ ./null_syscall --vsetvli
> vsetvli complete
>  iterations: 1000000000
>    duration: 198 seconds
> avg latency: 198.73 ns
> 
> $ sudo sysctl abi.riscv_v_vstate_discard=0
> abi.riscv_v_vstate_discard = 0
> 
> $ ./null_syscall --vsetvli
> vsetvli complete
>  iterations: 1000000000
>    duration: 149 seconds
> avg latency: 149.89 ns
> 
> I'm testing on the tt-blackhole-v6.16-rc1_vstate_discard [5] branch that
> has 13 patches, including this one, on top of v6.16-rc1. Most are simple
> yaml patches for dt bindings along with dts files and a bespoke network
> driver. I don't think the other patches are relevant to this discussion.
> 
> This patch applies clean on its own mainline and riscv/for-next.
> 
> [1] https://www.sifive.com/cores/intelligence-x200-series
> [2] https://tenstorrent.com/en/hardware/blackhole
> [3] https://github.com/tenstorrent/tt-bh-linux
> [4] https://gist.github.com/tt-fustini/ab9b217756912ce75522b3cce11d0d58
> [5] https://github.com/tenstorrent/linux/tree/tt-blackhole-v6.16-rc1_vstate_discard
> 
> Signed-off-by: Drew Fustini <fustini@...nel.org>
> ---
[...]

> diff --git a/arch/riscv/kernel/vector.c b/arch/riscv/kernel/vector.c
> index 184f780c932d443d81eecac7a6fb8070ee7a5824..7a4c209ad337efd7a3995cfc7cf1700c03e55b40 100644
> --- a/arch/riscv/kernel/vector.c
> +++ b/arch/riscv/kernel/vector.c
> @@ -26,6 +26,7 @@ static struct kmem_cache *riscv_v_user_cachep;
>  static struct kmem_cache *riscv_v_kernel_cachep;
>  #endif
>  
> +bool riscv_v_vstate_discard_ctl = IS_ENABLED(CONFIG_RISCV_ISA_V_VSTATE_DISCARD);
>  unsigned long riscv_v_vsize __read_mostly;
>  EXPORT_SYMBOL_GPL(riscv_v_vsize);
>  
> @@ -307,11 +308,24 @@ static const struct ctl_table riscv_v_default_vstate_table[] = {
>  	},
>  };
>  
> +static const struct ctl_table riscv_v_vstate_discard_table[] = {
> +	{
> +		.procname       = "riscv_v_vstate_discard",
> +		.data           = &riscv_v_vstate_discard_ctl,
> +		.maxlen         = sizeof(riscv_v_vstate_discard_ctl),
> +		.mode           = 0644,
> +		.proc_handler   = proc_dobool,
> +	},
> +};
> +
>  static int __init riscv_v_sysctl_init(void)
>  {
> -	if (has_vector() || has_xtheadvector())
> +	if (has_vector() || has_xtheadvector()) {
Is this pair of curly braces strictly necessary?
for potential extensibility?

Acked-by: Troy Mitchell <troy.mitchell@...ux.spacemit.com>

Best regards,
Troy
>  		if (!register_sysctl("abi", riscv_v_default_vstate_table))
>  			return -EINVAL;
> +		if (!register_sysctl("abi", riscv_v_vstate_discard_table))
> +			return -EINVAL;
> +	}
>  	return 0;
>  }
>  
> 
> ---
> base-commit: 3ac864c2d9bb8608ee236e89bf561811613abfce
> change-id: 20250818-riscv_v_vstate_discard-e89b3181e0ac
> 
> Best regards,
> -- 
> Drew Fustini <fustini@...nel.org>
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Powered by blists - more mailing lists