Message-ID: <5a96d08e-0714-4925-a9c4-ea66549623d1@iscas.ac.cn>
Date: Sun, 10 Aug 2025 17:52:54 +0800
From: Vivian Wang <wangruikang@...as.ac.cn>
To: Drew Fustini <fustini@...nel.org>, 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 8/9/25 15:54, Drew Fustini wrote:
> 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)

Ahh... That's more complicated than I thought.

I have some rough ideas on fixing ptrace handling, and I personally
still vastly prefer VS=Initial, but given the complexity, I think it
makes sense to defer this work until later and just get this performance
fix in first. And also...

> 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.

I was initially (ha!) worried about the sysctl knob getting in, and
immediately going away after we implement VS=Initial (correctly), but if
you think the right thing is to have this knob always, that wouldn't be
a problem.

>> 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.
>
Yeah, I agree that this second part of the description is good.

>>>> +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.

Typo: eanble -> enable

Also, I think it kinda makes the false impression that the sysctl knob
is only available with this set to N? I think this should be structured
to say something like this being the default value, and also say that
userspace can override this behavior system-wide via the sysctl knob.

Otherwise mostly looks good to me, assuming that this matches the
intentions of the original design.

Thanks,
Vivian "dramforever" Wang

>   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