| Message ID | 8612e69a10235e67fac8a55864e77a4ab8f771ac.1636362169.git.greentime.hu@sifive.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | riscv: Add vector ISA support | expand |
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>
Palmer Dabbelt <palmer@dabbelt.com> 於 2021年12月15日 週三 上午12:29寫道： > > 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? Thank you, Palmer. I'll update this and I also found another issue in secondary_start_sbi. We should also disable vector there. > > */ > > - 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. That's right. I'll update the code here to #ifdef CONFIG_VECTOR csrr t0, CSR_MISA li t1, COMPAT_HWCAP_ISA_V and t0, t0, t1 beqz t0, .Lreset_regs_done > > + 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. > /* * Clear vector registers and reset vcsr * VLMAX has a defined value, VLEN is a constant, * and this form of vsetvli is defined to set vl to VLMAX. */ li t1, SR_VS csrs CSR_STATUS, t1 csrs CSR_VCSR, x0 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 > > + /* 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>
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 */ - 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 + 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 + /* note that the caller must clear SR_VS */ +#endif /* CONFIG_VECTOR */ + .Lreset_regs_done: ret END(reset_regs)