Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 22 Jun 2023 13:55:54 -0400
From:   Darius Rad <darius@bluespec.com>
To:     Palmer Dabbelt <palmer@dabbelt.com>
Cc:     bjorn@kernel.org, Paul Walmsley <paul.walmsley@sifive.com>,
        aou@eecs.berkeley.edu, linux-riscv@lists.infradead.org,
        Bjorn Topel <bjorn@rivosinc.com>, linux-kernel@vger.kernel.org,
        linux@rivosinc.com, remi@remlab.net, andy.chiu@sifive.com
Subject: Re: [PATCH] riscv: Discard vector state on syscalls
Message-ID: <ZJSLKrB/4xaYB75d@bruce.bluespec.com>
Mail-Followup-To: Palmer Dabbelt <palmer@dabbelt.com>, bjorn@kernel.org,
        Paul Walmsley <paul.walmsley@sifive.com>, aou@eecs.berkeley.edu,
        linux-riscv@lists.infradead.org, Bjorn Topel <bjorn@rivosinc.com>,
        linux-kernel@vger.kernel.org, linux@rivosinc.com, remi@remlab.net,
        andy.chiu@sifive.com
References: <20230622173613.30722-1-bjorn@kernel.org>
 <mhng-5f8ae912-3b56-4eda-bdfa-1de31a7c93e0@palmer-ri-x1c9a>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <mhng-5f8ae912-3b56-4eda-bdfa-1de31a7c93e0@palmer-ri-x1c9a>
Precedence: bulk

On Thu, Jun 22, 2023 at 10:46:31AM -0700, Palmer Dabbelt wrote:
> On Thu, 22 Jun 2023 10:36:13 PDT (-0700), bjorn@kernel.org wrote:
> > From: Björn Töpel <bjorn@rivosinc.com>
> > 
> > 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 registers are cleared, vill is set (invalid), and the
> > vector status is set to Initial.
> > 
> > That way we can prevent userspace from accidentally relying on the
> > stated save.
> > 
> > Rémi pointed out [1] that clearing the registers might be superfluous,
> > and setting vill is sufficient.
> > 
> > Link: https://lore.kernel.org/linux-riscv/12784326.9UPPK3MAeB@basile.remlab.net/ # [1]
> > Suggested-by: Palmer Dabbelt <palmer@rivosinc.com>
> > Suggested-by: Rémi Denis-Courmont <remi@remlab.net>
> > Signed-off-by: Björn Töpel <bjorn@rivosinc.com>
> > ---
> > 
> > I figured I'd sent out a proper patch. I like Andy's optimization
> > patch, but TBH I think we should do that as a follow up.
> > 
> > As Rémi pointed out, the clearing might be opted out, but I left it in
> > here.
> 
> I think we're going to end up with a bunch of uarch-specific stuff here, but
> for now having the heavy hammer seems safest.  There's no V hardware yet so
> we can't really tell how anything performs, at least this way we're
> definately not leaking anything.
> 
> So I'm OK with this.  I'd also be fine with clearing to all-1s, I think it's
> kind of splitting hairs at this point: the 1s are nice because they're what
> the rest of V does, but setting vill should make everything trap anyway so
> maybe it doesn't matter -- and it's not clear if 1 or 0 will allow initial,
> so who knows.
> 
> Darius: I'm cool swapping over to the 1s if you feel strongly about it.
> Bjorn says Sweeden is on vacation, so just LMK and I'll re-spin it with the
> 1s.

I think all 1s would be preferred, but I don't think it's particularly
critical (splitting hairs, like you said), so I'll let you make the call.  

> 
> Regardless I'd like to pick up something that blows away V state for this
> merge window, as it'll make sure the uABI is quite strictly enforced.
> 
> > Björn
> > 
> > ---
> >  arch/riscv/include/asm/vector.h | 25 +++++++++++++++++++++++++
> >  arch/riscv/kernel/traps.c       |  2 ++
> >  2 files changed, 27 insertions(+)
> > 
> > diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h
> > index 04c0b07bf6cd..692ce55e4a69 100644
> > --- a/arch/riscv/include/asm/vector.h
> > +++ b/arch/riscv/include/asm/vector.h
> > @@ -163,6 +163,30 @@ 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, vtype_inval = 1UL << (BITS_PER_LONG - 1);
> > +
> > +	if (!riscv_v_vstate_query(regs))
> > +		return;
> > +
> > +	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"
> > +		"vsetvl		%0, x0, %1\n\t"
> > +		".option pop\n\t"
> > +		: "=&r" (vl) : "r" (vtype_inval) : "memory");
> > +	riscv_v_disable();
> > +
> > +	riscv_v_vstate_on(regs);
> > +}
> > +
> >  #else /* ! CONFIG_RISCV_ISA_V  */
> > 
> >  struct pt_regs;
> > @@ -178,6 +202,7 @@ 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_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 05ffdcd1424e..00c68b57ff88 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_discard(regs);
> > +
> >  		syscall = syscall_enter_from_user_mode(regs, syscall);
> > 
> >  		if (syscall < NR_syscalls)
> > 
> > base-commit: 4681dacadeefa5ca6017e00736adc1d7dc963c6a