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

[Drew Fustini]

On Mon, Jun 19, 2023 at 12:05:43PM -0700, Palmer Dabbelt wrote:
> 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.

I've tested the impact of riscv_v_vstate_discard() on the SiFive X280
cores [1] in the Tenstorrent Blackhole SoC [2]. The results from the
Blackhole P100 [3] card show that discarding the vector registers
increases null syscall latency by 28%.

The null syscall program [4] executes the vsetvli vector instruction and
then calls getppid() in a loop for 1 million iterations. The average
duration of the syscall is 201 ns with a branch based on v6.16-rc1 [5].
This is with the current upstream behavior where do_trap_ecall_u() calls
riscv_v_vstate_discard().

I then created a new branch [6] which disables riscv_v_vstate_discard().
The average duration of the syscall drops to 143 ns.

Would some sort of tunable be acceptable to allow the user to opt out
of the v state discard? Maybe a kernel cmdline argument?

Thanks,
Drew

[1] https://www.sifive.com/document-file/x280-datasheet
[2] https://tenstorrent.com/en/hardware/blackhole
[3] https://github.com/tenstorrent/tt-bh-linux
[4] https://gist.github.com/tt-fustini/fa793a35c34f07059d8a7427e1cd8e84
[5] https://github.com/tenstorrent/linux/tree/tt-blackhole-v6.16-rc1
[6] https://github.com/tenstorrent/linux/tree/tt-blackhole-v6.16-rc1_no_vstate_discard