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

[Björn Töpel]

Andy Chiu <andy.chiu at sifive.com> writes:

>> > diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
>> > index 24d309c6ab8d..e36b69c9b07f 100644
>> > --- a/arch/riscv/kernel/traps.c
>> > +++ b/arch/riscv/kernel/traps.c
>> > @@ -291,10 +291,14 @@ asmlinkage __visible __trap_section void do_trap_ecall_u(struct pt_regs *regs)
>> >  {
>> >       if (user_mode(regs)) {
>> >               ulong syscall = regs->a7;
>> > +             bool v_is_on;
>> >
>> >               regs->epc += 4;
>> >               regs->orig_a0 = regs->a0;
>> >
>> > +             v_is_on = riscv_v_vstate_query(regs);
>> > +             riscv_v_vstate_off(regs);
>> > +
>> >               syscall = syscall_enter_from_user_mode(regs, syscall);
>> >
>> >               if (syscall < NR_syscalls)
>> > @@ -303,6 +307,10 @@ asmlinkage __visible __trap_section void do_trap_ecall_u(struct pt_regs *regs)
>> >                       regs->a0 = -ENOSYS;
>> >
>> >               syscall_exit_to_user_mode(regs);
>> > +             if (v_is_on) {
>> > +                     riscv_v_vstate_discard(regs);
>> > +                     riscv_v_vstate_dirty(regs);
>>
>> Ah! Neat! Why dirty, instead of just keeping the "set to Initial" from
>> my diff?
>
> Both work, I think. But here if we set it to "on" after discarding
> V-regs, then take a context switch before executing any V instructions
> in user space (does not change future vstate to dirty). Then we will
> leak V-regs previously set into its vstate.datap after switching back,
> because we only save V context if vstate is dirty. So, I think setting
> vstate to dirty is a safer option.

Ah, yes, good point. An alternative variant is this:

---
diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h
index 04c0b07bf6cd..32b6115a54a5 100644
--- a/arch/riscv/include/asm/vector.h
+++ b/arch/riscv/include/asm/vector.h
@@ -139,15 +139,51 @@ static inline void riscv_v_vstate_save(struct task_struct *task,
 	}
 }
 
+static inline void __riscv_v_vstate_discard(void)
+{
+	unsigned long vl, vtype_inval = 1UL << (BITS_PER_LONG - 1);
+
+	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"
+		"vsetvl		%0, x0, %1\n\t"
+		".option pop\n\t"
+		: "=&r" (vl) : "r" (vtype_inval) : "memory");
+	riscv_v_disable();
+}
+
+static inline void riscv_v_vstate_discard(struct pt_regs *regs)
+{
+	if (!riscv_v_vstate_query(regs))
+		return;
+
+	__riscv_v_vstate_discard();
+	riscv_v_vstate_on(regs);
+}
+
 static inline void riscv_v_vstate_restore(struct task_struct *task,
 					  struct pt_regs *regs)
 {
-	if ((regs->status & SR_VS) != SR_VS_OFF) {
+	unsigned long status = regs->status & SR_VS;
+
+	WARN_ON(status == SR_VS_DIRTY);
+
+	if (status == SR_VS_CLEAN) {
 		struct __riscv_v_ext_state *vstate = &task->thread.vstate;
 
 		__riscv_v_vstate_restore(vstate, vstate->datap);
 		__riscv_v_vstate_clean(regs);
+		return;
 	}
+
+	if (status == SR_VS_INITIAL)
+		__riscv_v_vstate_discard();
 }
 
 static inline void __switch_to_vector(struct task_struct *prev,
@@ -178,6 +214,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 5158961ea977..5ff63a784a6d 100644
--- a/arch/riscv/kernel/traps.c
+++ b/arch/riscv/kernel/traps.c
@@ -296,6 +296,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)

---


Here, we simply discard the regs if the state is Initial. Thoughts?


Björn