Re: [PATCH -next v13 07/19] riscv: Introduce riscv_vsize to record size of Vector context

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3591 bytes --]

Hey again Andy!

On Wed, Jan 25, 2023 at 02:20:44PM +0000, Andy Chiu wrote:
> From: Greentime Hu <greentime.hu@sifive.com>
> 
> This patch is used to detect the size of CPU vector registers and use
> riscv_vsize to save the size of all the vector registers. It assumes all
> harts has the same capabilities in a SMP system.
> 
> [guoren@linux.alibaba.com: add has_vector checking]
> Co-developed-by: Guo Ren <guoren@linux.alibaba.com>
> 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>
> Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
> Signed-off-by: Andy Chiu <andy.chiu@sifive.com>
> ---
>  arch/riscv/include/asm/vector.h |  3 +++
>  arch/riscv/kernel/cpufeature.c  | 12 +++++++++++-
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h
> index 0fda0faf5277..16cb4a1c1230 100644
> --- a/arch/riscv/include/asm/vector.h
> +++ b/arch/riscv/include/asm/vector.h
> @@ -13,6 +13,8 @@
>  #include <asm/hwcap.h>
>  #include <asm/csr.h>
>  
> +extern unsigned long riscv_vsize;

Hmm, naming this with a riscv_v_ prefix too would be good I think.

> +
>  static __always_inline bool has_vector(void)
>  {
>  	return static_branch_likely(&riscv_isa_ext_keys[RISCV_ISA_EXT_KEY_VECTOR]);
> @@ -31,6 +33,7 @@ static __always_inline void rvv_disable(void)
>  #else /* ! CONFIG_RISCV_ISA_V  */
>  
>  static __always_inline bool has_vector(void) { return false; }
> +#define riscv_vsize (0)
>  
>  #endif /* CONFIG_RISCV_ISA_V */
>  
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index c433899542ff..3aaae4e0b963 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -21,6 +21,7 @@
>  #include <asm/processor.h>
>  #include <asm/smp.h>
>  #include <asm/switch_to.h>
> +#include <asm/vector.h>
>  
>  #define NUM_ALPHA_EXTS ('z' - 'a' + 1)
>  
> @@ -31,6 +32,10 @@ static DECLARE_BITMAP(riscv_isa, RISCV_ISA_EXT_MAX) __read_mostly;
>  
>  DEFINE_STATIC_KEY_ARRAY_FALSE(riscv_isa_ext_keys, RISCV_ISA_EXT_KEY_MAX);
>  EXPORT_SYMBOL(riscv_isa_ext_keys);
> +#ifdef CONFIG_RISCV_ISA_V
> +unsigned long riscv_vsize __read_mostly;
> +EXPORT_SYMBOL_GPL(riscv_vsize);
> +#endif

I really don't think that this should be in here at all. IMO, this
should be moved out to vector.c or something along those lines and...

>  
>  /**
>   * riscv_isa_extension_base() - Get base extension word
> @@ -258,7 +263,12 @@ void __init riscv_fill_hwcap(void)
>  	}
>  
>  	if (elf_hwcap & COMPAT_HWCAP_ISA_V) {
> -#ifndef CONFIG_RISCV_ISA_V
> +#ifdef CONFIG_RISCV_ISA_V
> +		/* There are 32 vector registers with vlenb length. */
> +		rvv_enable();
> +		riscv_vsize = csr_read(CSR_VLENB) * 32;
> +		rvv_disable();
> +#else

...so should all of this code, especially the csr_read(). vector.c
would then provide the actual implementation, and vector.h would provide
an implementation with an empty function body.
The code here could the, following my previous suggestion of
IS_ENABLED() look along the lines of:
	if (elf_hwcap & COMPAT_HWCAP_ISA_V) {
		if (IS_ENABLED(CONFIG_RISCV_ISA_V))
			riscv_v_setup_vsize();
		else
			elf_hwcap &= ~COMPAT_HWCAP_ISA_V;
	}

Having the csr_read() in particular looks rather out of place to me in
this file. Better off keeping the vector stuff together & having
unneeded ifdefs is my pet peeve ;)

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]