Message-ID: <aJb+wifbIAsit+me@x1>
Date: Sat, 9 Aug 2025 00:54:42 -0700
From: Drew Fustini <fustini@...nel.org>
To: Vivian Wang <wangruikang@...as.ac.cn>, Darius Rad <darius@...espec.com>
Cc: Paul Walmsley <paul.walmsley@...ive.com>,
	Palmer Dabbelt <palmer@...belt.com>,
	Alexandre Ghiti <alex@...ti.fr>,
	Samuel Holland <samuel.holland@...ive.com>,
	Björn Töpel <bjorn@...osinc.com>,
	Andy Chiu <andybnac@...il.com>,
	Conor Dooley <conor.dooley@...rochip.com>,
	linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
	Drew Fustini <dfustini@...storrent.com>
Subject: Re: [PATCH v2] riscv: Add sysctl to control discard of vstate during
 syscall

On Sat, Aug 09, 2025 at 11:58:24AM +0800, Vivian Wang wrote:
> My previous comment on v1 on prefering clobbering with VS = Initial
> handling aside...

I found that in the discard vector state patch discussion 2 years ago
that Andy and Bjorn discussed how Initial could cause a problem [1]:

  It's not a racy, but you're correct that setting the state to Initial,
  will cause issues. When get/set_regs is called, the tracee will be
  stopped, and a schedule() has been done.

In the v3 series, Bjorn notes [2]:

  Set state to Dirty after discard, for proper ptrace() handling (Andy)

Also, I would like the ability to have the ability to switch off
__riscv_v_vstate_discard() and not loose any cycles to it, so I think
this sysctl is a good fit for that.

> 
> On 8/8/25 20:36, Darius Rad wrote:
> > On Wed, Aug 06, 2025 at 07:03:28AM -0700, Drew Fustini wrote:
> > [...]
> >> diff --git a/Documentation/arch/riscv/vector.rst b/Documentation/arch/riscv/vector.rst
> >> index 3987f5f76a9deb0824e53a72df4c3bf90ac2bee1..b702c00351617165a4d8897c7df68eadcd2d562e 100644
> >> --- a/Documentation/arch/riscv/vector.rst
> >> +++ b/Documentation/arch/riscv/vector.rst
> >> @@ -134,7 +134,25 @@ processes in form of sysctl knob:
> >>  3.  Vector Register State Across System Calls
> >>  ---------------------------------------------
> >>  
> >> -As indicated by version 1.0 of the V extension [1], vector registers are
> >> -clobbered by system calls.
> >> +Linux adopts the syscall ABI proposed by version 1.0 of the V extension [1],
> >> +where vector registers are clobbered by system calls. Specifically:
> >> +
> >> +    Executing a system call causes all caller-saved vector registers
> >> +    (v0-v31, vl, vtype) and vstart to become unspecied.
> >> +
> > Perhaps:
> >
> > Clobbering the vector registers may prevent leaking information to user
> 
> No... Not clobbering does not "leak" anything. If you find that it leaks
> information, please report - that's a bug.

Thanks Darius and Vivian for your comments. I think it is a good idea
for me to write about the possible advantages of mandatory clobbering on
syscall entry. However, I am also uncertain how clobbering on syscall
entry helps prevent leaking information.

> > space and aid in debugging, but can significantly increase system call
> > latency for some implementations.  [...]

I think that is a good idea for me to call out that this is can be
useful for debugging and testing.

> >
> >> +However, clobbering the vector registers can significantly increase system call
> >> +latency for some implementations. To mitigate this performance impact, a sysctl
> >> +knob is provided that controls whether vector state is always discarded in the
> >> +syscall path:
> >> +
> >> +* /proc/sys/abi/riscv_v_vstate_discard
> >> +
> >> +    Valid values are:
> >> +
> >> +    * 0: Vector state is not always clobbered in all syscalls
> >> +    * 1: Mandatory clobbering of vector state in all syscalls
> >> +
> >> +    Reading this file returns the current discard behavior. The initial state is
> >> +    controlled by CONFIG_RISCV_ISA_V_VSTATE_DISCARD.
> >>  
> >>  1: https://github.com/riscv/riscv-v-spec/blob/master/calling-convention.adoc
> >> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> >> index 36061f4732b7496a9c68a9a10f9959849dc2a95c..7bb8a8513135cbc105bd94d273012486a886f724 100644
> >> --- a/arch/riscv/Kconfig
> >> +++ b/arch/riscv/Kconfig
> >> @@ -656,6 +656,16 @@ config RISCV_ISA_V_DEFAULT_ENABLE
> >>  
> >>  	  If you don't know what to do here, say Y.
> >>  
> >> +config RISCV_ISA_V_VSTATE_DISCARD
> >> +	bool "Enable Vector state discard by default"
> >> +	depends on RISCV_ISA_V
> >> +	default n
> >> +	help
> > Perhaps add the following paragraph:
> >
> > Discarding vector state is more robust, but has negative performance
> > implications in certain implementations.
> 
> "Robust" is too vague... I don't think this word is helpful for anyone
> trying to understand what this does.

I agree that I should add more description to the Kconfig option as I
think what I wrote assumes too much prior knowledge of the code. Maybe
something like this:

  Discarding vector state on syscall entry can help identify userpace
  programs that are mistakenly relying on vector state being preserved
  across syscalls. This can be useful for debugging and test suites.
  However, this behavior can negatively impact performance on some
  RISC-V implementations.

  Say Y here if you want mandatory clobbering of vector state before
  entering all syscalls. If you select N, then userspace can still
  eanble it via the abi.riscv_v_vstate_discard sysctl knob.

  If you don't know what to do here, then select N.

Thanks,
Drew


[1] https://lore.kernel.org/linux-riscv/87r0pug6hb.fsf@all.your.base.are.belong.to.us/
[2] https://lore.kernel.org/linux-riscv/20230629062730.985184-1-bjorn@kernel.org/

Powered by blists - more mailing lists