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

[Andy Chiu]

On Fri, Jun 23, 2023 at 12:38 AM Björn Töpel <bjorn at kernel.org> wrote:
>
> 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?

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.

In your diff case, V-regs may be restored back to the previously-saved
state if the syscall caused a context switch.

I have not had a chance to test it yet because we are having a
vacation in Taiwan, and I have some other stuff to keep me busy :)
Please correct me if my thinking was wrong and I forgot some important
idea again...

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

Thanks,
Andy