diff mbox series

[v9,16/17] riscv: Fix an illegal instruction exception when accessing vlenb without enable vector first

Message ID 3c0297d8335e4cac54a4397c880092c1c983e04e.1636362169.git.greentime.hu@sifive.com (mailing list archive)
State New, archived
Headers show
Series riscv: Add vector ISA support | expand

Commit Message

Greentime Hu Nov. 9, 2021, 9:48 a.m. UTC 
It triggered an illegal instruction exception when accessing vlenb CSR
without enable vector first. To fix this issue, we should enable vector
before using it and disable vector after using it.

Co-developed-by: Vincent Chen <vincent.chen@sifive.com>
Signed-off-by: Vincent Chen <vincent.chen@sifive.com>
Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
---
 arch/riscv/include/asm/vector.h        | 2 ++
 arch/riscv/kernel/cpufeature.c         | 2 ++
 arch/riscv/kernel/kernel_mode_vector.c | 6 ++++--
 3 files changed, 8 insertions(+), 2 deletions(-)

Comments

Ben Dooks Nov. 10, 2021, 10:38 a.m. UTC | #1 
On 09/11/2021 09:48, Greentime Hu wrote:
> It triggered an illegal instruction exception when accessing vlenb CSR
> without enable vector first. To fix this issue, we should enable vector
> before using it and disable vector after using it.
> 
> Co-developed-by: Vincent Chen <vincent.chen@sifive.com>
> Signed-off-by: Vincent Chen <vincent.chen@sifive.com>
> Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
> ---
>   arch/riscv/include/asm/vector.h        | 2 ++
>   arch/riscv/kernel/cpufeature.c         | 2 ++
>   arch/riscv/kernel/kernel_mode_vector.c | 6 ++++--
>   3 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h
> index 5d7f14453f68..ca063c8f47f2 100644
> --- a/arch/riscv/include/asm/vector.h
> +++ b/arch/riscv/include/asm/vector.h
> @@ -8,6 +8,8 @@
>   
>   #include <linux/types.h>
>   
> +void rvv_enable(void);
> +void rvv_disable(void);
>   void kernel_rvv_begin(void);
>   void kernel_rvv_end(void);
>   
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 8e7557980faf..0139ec20adce 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -159,7 +159,9 @@ void __init riscv_fill_hwcap(void)
>   	if (elf_hwcap & COMPAT_HWCAP_ISA_V) {
>   		static_branch_enable(&cpu_hwcap_vector);
>   		/* There are 32 vector registers with vlenb length. */
> +		rvv_enable();
>   		riscv_vsize = csr_read(CSR_VLENB) * 32;
> +		rvv_disable();
>   	}
>   #endif

Would it be better to enable this here, and then restore the original
state instead of calling rvv_disable? If it was enabled then maybe
something else is going to rely on that state?

Maybe something like:

		prev = rvv_enable()
		riscv_vsize = csr_read(CSR_VLENB) * 32;
		rvv_restore(prev);


>   }
> diff --git a/arch/riscv/kernel/kernel_mode_vector.c b/arch/riscv/kernel/kernel_mode_vector.c
> index 8d2e53ea25c1..1ecb6ec5c56d 100644
> --- a/arch/riscv/kernel/kernel_mode_vector.c
> +++ b/arch/riscv/kernel/kernel_mode_vector.c
> @@ -71,15 +71,17 @@ static void put_cpu_vector_context(void)
>   	preempt_enable();
>   }
>   
> -static void rvv_enable(void)
> +void rvv_enable(void)
>   {
>   	csr_set(CSR_STATUS, SR_VS);
>   }
> +EXPORT_SYMBOL(rvv_enable);
>   
> -static void rvv_disable(void)
> +void rvv_disable(void)
>   {
>   	csr_clear(CSR_STATUS, SR_VS);
>   }
> +EXPORT_SYMBOL(rvv_disable);
>   
>   /*
>    * kernel_rvv_begin(): obtain the CPU vector registers for use by the calling
>
Greentime Hu Nov. 16, 2021, 1:14 p.m. UTC | #2 
Ben Dooks <ben.dooks@codethink.co.uk> 於 2021年11月10日 週三 下午6:38寫道：
>
> On 09/11/2021 09:48, Greentime Hu wrote:
> > It triggered an illegal instruction exception when accessing vlenb CSR
> > without enable vector first. To fix this issue, we should enable vector
> > before using it and disable vector after using it.
> >
> > Co-developed-by: Vincent Chen <vincent.chen@sifive.com>
> > Signed-off-by: Vincent Chen <vincent.chen@sifive.com>
> > Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
> > ---
> >   arch/riscv/include/asm/vector.h        | 2 ++
> >   arch/riscv/kernel/cpufeature.c         | 2 ++
> >   arch/riscv/kernel/kernel_mode_vector.c | 6 ++++--
> >   3 files changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h
> > index 5d7f14453f68..ca063c8f47f2 100644
> > --- a/arch/riscv/include/asm/vector.h
> > +++ b/arch/riscv/include/asm/vector.h
> > @@ -8,6 +8,8 @@
> >
> >   #include <linux/types.h>
> >
> > +void rvv_enable(void);
> > +void rvv_disable(void);
> >   void kernel_rvv_begin(void);
> >   void kernel_rvv_end(void);
> >
> > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > index 8e7557980faf..0139ec20adce 100644
> > --- a/arch/riscv/kernel/cpufeature.c
> > +++ b/arch/riscv/kernel/cpufeature.c
> > @@ -159,7 +159,9 @@ void __init riscv_fill_hwcap(void)
> >       if (elf_hwcap & COMPAT_HWCAP_ISA_V) {
> >               static_branch_enable(&cpu_hwcap_vector);
> >               /* There are 32 vector registers with vlenb length. */
> > +             rvv_enable();
> >               riscv_vsize = csr_read(CSR_VLENB) * 32;
> > +             rvv_disable();
> >       }
> >   #endif
>
> Would it be better to enable this here, and then restore the original
> state instead of calling rvv_disable? If it was enabled then maybe
> something else is going to rely on that state?
>
> Maybe something like:
>
>                 prev = rvv_enable()
>                 riscv_vsize = csr_read(CSR_VLENB) * 32;
>                 rvv_restore(prev);
>
>
Hi Ben,

Thank you for reviewing this. The rvv won't be enabled here because we
disable it in head.S at beginning and this stage is still doing some
initialization work for rvv to set riscv_vsize, so we can assume that
kernel mode vector should not be allowed to use vector yet.

> >   }
> > diff --git a/arch/riscv/kernel/kernel_mode_vector.c b/arch/riscv/kernel/kernel_mode_vector.c
> > index 8d2e53ea25c1..1ecb6ec5c56d 100644
> > --- a/arch/riscv/kernel/kernel_mode_vector.c
> > +++ b/arch/riscv/kernel/kernel_mode_vector.c
> > @@ -71,15 +71,17 @@ static void put_cpu_vector_context(void)
> >       preempt_enable();
> >   }
> >
> > -static void rvv_enable(void)
> > +void rvv_enable(void)
> >   {
> >       csr_set(CSR_STATUS, SR_VS);
> >   }
> > +EXPORT_SYMBOL(rvv_enable);
> >
> > -static void rvv_disable(void)
> > +void rvv_disable(void)
> >   {
> >       csr_clear(CSR_STATUS, SR_VS);
> >   }
> > +EXPORT_SYMBOL(rvv_disable);
> >
> >   /*
> >    * kernel_rvv_begin(): obtain the CPU vector registers for use by the calling
> >
>
>
> --
> Ben Dooks                               http://www.codethink.co.uk/
> Senior Engineer                         Codethink - Providing Genius
>
> https://www.codethink.co.uk/privacy.html
diff mbox series

Patch

diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h
index 5d7f14453f68..ca063c8f47f2 100644
--- a/arch/riscv/include/asm/vector.h
+++ b/arch/riscv/include/asm/vector.h
@@ -8,6 +8,8 @@ 
 
 #include <linux/types.h>
 
+void rvv_enable(void);
+void rvv_disable(void);
 void kernel_rvv_begin(void);
 void kernel_rvv_end(void);
 
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 8e7557980faf..0139ec20adce 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -159,7 +159,9 @@  void __init riscv_fill_hwcap(void)
 	if (elf_hwcap & COMPAT_HWCAP_ISA_V) {
 		static_branch_enable(&cpu_hwcap_vector);
 		/* There are 32 vector registers with vlenb length. */
+		rvv_enable();
 		riscv_vsize = csr_read(CSR_VLENB) * 32;
+		rvv_disable();
 	}
 #endif
 }
diff --git a/arch/riscv/kernel/kernel_mode_vector.c b/arch/riscv/kernel/kernel_mode_vector.c
index 8d2e53ea25c1..1ecb6ec5c56d 100644
--- a/arch/riscv/kernel/kernel_mode_vector.c
+++ b/arch/riscv/kernel/kernel_mode_vector.c
@@ -71,15 +71,17 @@  static void put_cpu_vector_context(void)
 	preempt_enable();
 }
 
-static void rvv_enable(void)
+void rvv_enable(void)
 {
 	csr_set(CSR_STATUS, SR_VS);
 }
+EXPORT_SYMBOL(rvv_enable);
 
-static void rvv_disable(void)
+void rvv_disable(void)
 {
 	csr_clear(CSR_STATUS, SR_VS);
 }
+EXPORT_SYMBOL(rvv_disable);
 
 /*
  * kernel_rvv_begin(): obtain the CPU vector registers for use by the calling