Messages in this thread | | | Date | Tue, 14 Dec 2021 08:29:28 -0800 (PST) | Subject | Re: [PATCH v9 07/17] riscv: Reset vector register | From | Palmer Dabbelt <> |
| |
On Tue, 09 Nov 2021 01:48:19 PST (-0800), greentime.hu@sifive.com wrote: > From: Guo Ren <guoren@linux.alibaba.com> > > Reset vector registers at boot-time and disable vector instructions > execution for kernel mode. > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > Co-developed-by: Vincent Chen <vincent.chen@sifive.com> > Signed-off-by: Vincent Chen <vincent.chen@sifive.com> > Co-developed-by: Han-Kuan Chen <hankuan.chen@sifive.com> > Signed-off-by: Han-Kuan Chen <hankuan.chen@sifive.com> > Co-developed-by: Greentime Hu <greentime.hu@sifive.com> > Signed-off-by: Greentime Hu <greentime.hu@sifive.com> > --- > arch/riscv/kernel/entry.S | 6 +++--- > arch/riscv/kernel/head.S | 22 ++++++++++++++++++++-- > 2 files changed, 23 insertions(+), 5 deletions(-) > > diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S > index 98f502654edd..ad0fa80ada81 100644 > --- a/arch/riscv/kernel/entry.S > +++ b/arch/riscv/kernel/entry.S > @@ -77,10 +77,10 @@ _save_context: > * Disable user-mode memory access as it should only be set in the > * actual user copy routines. > * > - * Disable the FPU to detect illegal usage of floating point in kernel > - * space. > + * Disable the FPU/Vector to detect illegal usage of floating point > + * or vector in kernel space. > */ > - li t0, SR_SUM | SR_FS > + li t0, SR_SUM | SR_FS | SR_VS > > REG_L s0, TASK_TI_USER_SP(tp) > csrrc s1, CSR_STATUS, t0 > diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S > index 52c5ff9804c5..551afe1de85e 100644 > --- a/arch/riscv/kernel/head.S > +++ b/arch/riscv/kernel/head.S > @@ -242,10 +242,10 @@ pmp_done: > .option pop > > /* > - * Disable FPU to detect illegal usage of > + * Disable FPU & VECTOR to detect illegal usage of > * floating point in kernel space
Presumably that should be "floating point or vector", like the other one?
> */ > - li t0, SR_FS > + li t0, SR_FS | SR_VS > csrc CSR_STATUS, t0 > > #ifdef CONFIG_SMP > @@ -433,6 +433,24 @@ ENTRY(reset_regs) > csrw fcsr, 0 > /* note that the caller must clear SR_FS */ > #endif /* CONFIG_FPU */ > + > +#ifdef CONFIG_VECTOR > + csrr t0, CSR_MISA > + li t1, (COMPAT_HWCAP_ISA_V >> 16) > + slli t1, t1, 16
Why? Shouldn't the "li" pseudo handle generating that constant fine? It generates the expected lui for me.
> + and t0, t0, t1 > + beqz t0, .Lreset_regs_done > + > + li t1, SR_VS > + csrs CSR_STATUS, t1 > + vsetvli t1, x0, e8, m8 > + vmv.v.i v0, 0 > + vmv.v.i v8, 0 > + vmv.v.i v16, 0 > + vmv.v.i v24, 0
I don't see anything resetting vcsr here, which is explicitly required by ISA manual.
Otherwise this looks OK to me: I wasn't actually sure this was guaranteed to hit every bit in the vector register file, but IIUC it does -- VLMAX has a defined value, VLEN is a constant, and this form of vsetvli is defined to set vl to VLMAX. Probably worth a comment, though.
> + /* note that the caller must clear SR_VS */ > +#endif /* CONFIG_VECTOR */ > + > .Lreset_regs_done: > ret > END(reset_regs)
With those minor bits fixed,
Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com>
| |