Re: [PATCH] RISC-V: Clobber V registers on syscalls

["Björn Töpel" <bjorn@kernel.org>]

Palmer Dabbelt <palmer@rivosinc.com> writes:

> On Mon, 19 Jun 2023 12:01:20 PDT (-0700), bjorn@kernel.org wrote:
>> Palmer Dabbelt <palmer@rivosinc.com> writes:
>>
>> [...]
>>
>>>>> +		riscv_v_vstate_off(regs);
>>>>> +
>>>>
>>>> Not off, right? Isn't it __riscv_v_vstate_clean() that you'd like to
>>>> call? Something like:
>>>>
>>>> static void vstate_discard(struct pt_regs *regs)
>>>> {
>>>>        if ((regs->status & SR_VS) == SR_VS_DIRTY)
>>>>                __riscv_v_vstate_clean(regs);
>>>> }
>>>>
>>>> Complemented by a !V config variant.
>>>
>>> I think it's just a question of what we're trying to do here: clean 
>>> avoids the kernel V state save, but unless the kernel decides to use V 
>>> during the syscall the register contents will still be usable by 
>>> userspace.  Maybe that's fine and we can just rely on the ISA spec, 
>>> though?  I sent another patch to just document it in Linux, even if it's 
>>> in the ISA spec it seems worth having in the kernel as well.
>>>
>>> That said, I think the right thing to do here might be to zero the V 
>>> register state and set it to initial: that way we can prevent userspace 
>>> from accidentally relying on the state save, but we can also avoid the 
>>> trap that would come from turning it off.  That lets us give the 
>>> hardware a nice clean indication when the V state isn't in use, which 
>>> will hopefully help us avoid the save/restore performance issues that 
>>> other ports have hit.
>>
>> FWIW, I think that's a much better idea than turning V off. I also like
>> that it'll preventing userland to rely on pre-ecall state.
>
> OK, anyone else opposed?
>
> We're kind of in the weeds on performance, I think we'd need HW to know 
> for sure if either is an issue.  Seems best to just play it safe WRT the 
> uABI for now, we can always deal with any performance issues if the 
> exist.

Here's the patch you mentioned at the PW synchup; I've kept the Subject
and such if you wan't to apply it. LMK if you'd like a proper one.

--

Subject: [PATCH] riscv: Discard vector state on syscalls
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The RISC-V vector specification states:
  Executing a system call causes all caller-saved vector registers
  (v0-v31, vl, vtype) and vstart to become unspecified.

The vector status is set to Initial, and the vector state is
explicitly zeroed. That way we can prevent userspace from accidentally
relying on the stated save.

Signed-off-by: Björn Töpel <bjorn@rivosinc.com>
---
arch/riscv/include/asm/vector.h | 24 ++++++++++++++++++++++++
 arch/riscv/kernel/traps.c       |  2 ++
 2 files changed, 26 insertions(+)

diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h
index 04c0b07bf6cd..b3020d064f42 100644
--- a/arch/riscv/include/asm/vector.h
+++ b/arch/riscv/include/asm/vector.h
@@ -163,6 +163,29 @@ static inline void __switch_to_vector(struct task_struct *prev,
 void riscv_v_vstate_ctrl_init(struct task_struct *tsk);
 bool riscv_v_vstate_ctrl_user_allowed(void);
 
+static inline void riscv_v_vstate_discard(struct pt_regs *regs)
+{
+	unsigned long vl;
+
+	if (!riscv_v_vstate_query(regs))
+		return;
+
+	riscv_v_vstate_on(regs);
+
+	riscv_v_enable();
+	asm volatile (
+		".option push\n\t"
+		".option arch, +v\n\t"
+		"vsetvli	%0, x0, e8, m8, ta, ma\n\t"
+		"vmv.v.i	v0, 0\n\t"
+		"vmv.v.i	v8, 0\n\t"
+		"vmv.v.i	v16, 0\n\t"
+		"vmv.v.i	v24, 0\n\t"
+		".option pop\n\t"
+		: "=&r" (vl) : : "memory");
+	riscv_v_disable();
+}
+
 #else /* ! CONFIG_RISCV_ISA_V  */
 
 struct pt_regs;
@@ -178,6 +201,7 @@ static inline bool riscv_v_vstate_ctrl_user_allowed(void) { return false; }
 #define __switch_to_vector(__prev, __next)	do {} while (0)
 #define riscv_v_vstate_off(regs)		do {} while (0)
 #define riscv_v_vstate_on(regs)			do {} while (0)
+#define riscv_v_vstate_discard(regs)		do {} while (0)
 
 #endif /* CONFIG_RISCV_ISA_V */
 
diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
index 05ffdcd1424e..00c68b57ff88 100644
--- a/arch/riscv/kernel/traps.c
+++ b/arch/riscv/kernel/traps.c
@@ -295,6 +295,8 @@ asmlinkage __visible __trap_section void do_trap_ecall_u(struct pt_regs *regs)
 		regs->epc += 4;
 		regs->orig_a0 = regs->a0;
 
+		riscv_v_vstate_discard(regs);
+
 		syscall = syscall_enter_from_user_mode(regs, syscall);
 
 		if (syscall < NR_syscalls)

base-commit: abd6152d6046ddc4be1040b6206bee2e025e8a79
-- 
2.39.2

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv