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

[Björn Töpel]

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

> On Thu, Jun 22, 2023 at 5:40 AM Björn Töpel <bjorn at kernel.org> wrote:
>>
>> Andy Chiu <andy.chiu at sifive.com> writes:
>>
>> >> +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);
>> >
>> > Do we need this riscv_v_vstate_on()?  If it is not on we'd return
>> > early in the previous "if" statement, right?
>>
>> riscv_v_vstate_on() just set the state to Initial, right? Or do you mean
>> that riscv_v_vstate_query() is too much, and we should only check if the
>> state is dirty?
>>
>> >> +
>> >> +       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();
>> >
>> > Maybe consider cleaning the vstate (status.vs) here. As such we don't
>> > have to save V during context switch.
>>
>> It's late, and I'm slower than usual. The regs are cleared, and the
>> state is Initial. No save on context switch, but restore, right?
>
> Yes, it's my bad, you are right. I sometime messed around the "real"
> status.VS with the one in the userspace context :P
>
>>
>> > Or, maybe we could set vstate as off during syscall and discard V-reg
>> > + restore status.VS when returning back to userspace?
>>
>> Hmm, interesting. We need to track the status.VS to restore somewhere...
>
> Maybe something like this?
>
> diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h
> index 04c0b07bf6cd..79de9ca83391 100644
> --- a/arch/riscv/include/asm/vector.h
> +++ b/arch/riscv/include/asm/vector.h
> @@ -43,6 +43,11 @@ static inline void riscv_v_vstate_on(struct pt_regs *regs)
>  	regs->status = (regs->status & ~SR_VS) | SR_VS_INITIAL;
>  }
>  
> +static inline void riscv_v_vstate_dirty(struct pt_regs *regs)
> +{
> +	regs->status = (regs->status & ~SR_VS) | SR_VS_DIRTY;
> +}
> +
>  static inline bool riscv_v_vstate_query(struct pt_regs *regs)
>  {
>  	return (regs->status & SR_VS) != 0;
> @@ -163,6 +168,24 @@ 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;
> +
> +	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,8 @@ 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_dirty(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 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?

This flow does avoid some context switch costs, but I wonder if this is
some that can be added later, when we can more reliable measure the
overhead. Premature optimization, and all that. ;-)


Björn