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

[Palmer Dabbelt]

On Wed, 21 Jun 2023 07:44:51 PDT (-0700), Darius Rad wrote:
> On Wed, Jun 21, 2023 at 04:26:14PM +0200, Björn Töpel wrote:
>> Palmer Dabbelt <palmer at rivosinc.com> writes:
>>
>> > On Mon, 19 Jun 2023 12:01:20 PDT (-0700), bjorn at kernel.org wrote:
>> >> Palmer Dabbelt <palmer at 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.
>
> Is it worth clobbering with all 1s, rather than zero, for consistency with
> other vector behavior (i.e., tail/mask agnostic) and for the reasons given
> in the vector spec for not doing so with zero?

Might be.  I guess the assumption was that vs==initial means all 0's, 
but unless I'm missing something there's no rules for what initial means 
in the spec.

>
>>
>> Signed-off-by: Björn Töpel <bjorn at 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 at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv