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

[Palmer Dabbelt]

On Fri, 16 Jun 2023 13:12:14 PDT (-0700), bjorn at kernel.org wrote:
> Palmer Dabbelt <palmer at rivosinc.com> writes:
>
>> The V registers are clobbered by standard ABI functions, so userspace
>> probably doesn't have anything useful in them by the time we get to the
>> kernel.  So let's just document that they're clobbered by syscalls and
>> proactively clobber them.
>>
>> Signed-off-by: Palmer Dabbelt <palmer at rivosinc.com>
>> ---
>> IIRC we'd talked about doing this, but I didn't see anything in the
>> docs.  I figure it's better to just proactively clobber the registers on
>> syscalls, as that way userspace can't end up accidentally depending on
>> them.
>> ---
>>  Documentation/riscv/vector.rst | 5 +++++
>>  arch/riscv/kernel/traps.c      | 2 ++
>>  2 files changed, 7 insertions(+)
>>
>> diff --git a/Documentation/riscv/vector.rst b/Documentation/riscv/vector.rst
>> index 48f189d79e41..a4dfa954215b 100644
>> --- a/Documentation/riscv/vector.rst
>> +++ b/Documentation/riscv/vector.rst
>> @@ -130,3 +130,8 @@ processes in form of sysctl knob:
>>  
>>      Modifying the system default enablement status does not affect the enablement
>>      status of any existing process of thread that do not make an execve() call.
>> +
>> +3.  Vector Register State Across System Calls
>> +---------------------------------------------
>> +
>> +Vector registers are clobbered by system calls.
>> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
>> index 05ffdcd1424e..bb99a6379b37 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_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.

I think the issue with zeroing the registers in that it may be slow on 
some implementations, as it requires a bunch of V register writes and 
those could be multi-cycle.  I'd lean towards doing the zeroing now, as 
it'll make sure userspace respects the uABI and we don't have any HW to 
measure the performance on.  Maybe the zeroing will be enough to get HW 
to make that fast, if not we can always roll it back when HW starts 
showing up.

There's also some questions as to whether or not HW is going to bother 
respecting the intermediate states, as IIRC it's pretty common for HW to 
ignore them for the F/D extensions (at least the old SiFive cores do).  
I think there's just not a whole lot we can do there, HW that 
inaccurately tracks the metadata will just end up with more 
save/restore time.

> Björn
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv