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

[Palmer Dabbelt]

On Wed, 21 Jun 2023 09:47:37 PDT (-0700), remi at remlab.net wrote:
> Le keskiviikkona 21. kesäkuuta 2023, 17.26.14 EEST Björn Töpel a écrit :
>> 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.
>> 
>> 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();
>
> Shouldn't this also set `vill` to 1 using `vsetvl`?

That seems reasonable to me.

> In fact, a faster alternative may yet be to *only* set an invalid vector 
> configuration. It's rather unlikely that user-space code would set a valid 
> configuration and use vectors without loading them first. If it ever does, then 
> it's so broken that the kernel probably doesn't need to care.

I think that's sufficient to force userspace to trap on a bad value?  
Most of the unsupported value writes in RISC-V are just WARL, but as far 
as I can tell the V spec requires vill handling.  Specifically

    Implementations must consider all bits of the vtype value to 
    determine if the configuration is supported. An unsupported value in 
    any location within the vtype value must result in vill being set.

which seems pretty concrete about this being required.  That's from the 
current draft of the V spec, the wording in 1.0 isn't quite as clear: it 
sort of allows for the WARL-type behavior, but that's probably splitting 
hairs.

That said, it provides a slightly different cost curve: we'd need to 
save/restore the V registers on non-syscall traps even when vill is set 
in userspace, as they've still got state in them (userspace could be in 
the middle of some probing routine, for example).

Also from Darius' fork of the thread: IIUC there's nothing saying 0 is 
initial, or that initial even needs to work.  So I think we're just 
splitting hairs here, as long as we clobber enough state that userspace 
doesn't accidentally depend on is fine with me.

> -- 
> 雷米‧德尼-库尔蒙
> http://www.remlab.net/