Re: [PATCH -next v19 02/24] riscv: Extending cpufeature.c to detect V-extension

From: Palmer Dabbelt <palmer@dabbelt.com>
To: andy.chiu@sifive.com
Cc: linux-riscv@lists.infradead.org, anup@brainfault.org,
	atishp@atishpatra.org, kvm-riscv@lists.infradead.org,
	kvm@vger.kernel.org, Vineet Gupta <vineetg@rivosinc.com>,
	greentime.hu@sifive.com, guoren@linux.alibaba.com,
	ren_guo@c-sky.com, andy.chiu@sifive.com,
	Paul Walmsley <paul.walmsley@sifive.com>,
	aou@eecs.berkeley.edu, ajones@ventanamicro.com,
	Conor Dooley <conor.dooley@microchip.com>,
	heiko.stuebner@vrull.eu, apatel@ventanamicro.com,
	jszhang@kernel.org, guoren@kernel.org, vincent.chen@sifive.com
Subject: Re: [PATCH -next v19 02/24] riscv: Extending cpufeature.c to detect V-extension
Date: Thu, 11 May 2023 15:56:35 -0700 (PDT)	[thread overview]
Message-ID: <mhng-5a4ccb60-d078-436a-a25f-b444e27b9115@palmer-ri-x1c9a> (raw)
In-Reply-To: <20230509103033.11285-3-andy.chiu@sifive.com>

On Tue, 09 May 2023 03:30:11 PDT (-0700), andy.chiu@sifive.com wrote:
> From: Guo Ren <ren_guo@c-sky.com>
>
> Add V-extension into riscv_isa_ext_keys array and detect it with isa
> string parsing.
>
> Signed-off-by: Guo Ren <ren_guo@c-sky.com>
> Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
> Suggested-by: Vineet Gupta <vineetg@rivosinc.com>
> Co-developed-by: Andy Chiu <andy.chiu@sifive.com>
> Signed-off-by: Andy Chiu <andy.chiu@sifive.com>
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> Reviewed-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
> Tested-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
> ---
>  arch/riscv/include/asm/hwcap.h      |  1 +
>  arch/riscv/include/asm/vector.h     | 26 ++++++++++++++++++++++++++
>  arch/riscv/include/uapi/asm/hwcap.h |  1 +
>  arch/riscv/kernel/cpufeature.c      | 11 +++++++++++
>  4 files changed, 39 insertions(+)
>  create mode 100644 arch/riscv/include/asm/vector.h
>
> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> index e0c40a4c63d5..574385930ba7 100644
> --- a/arch/riscv/include/asm/hwcap.h
> +++ b/arch/riscv/include/asm/hwcap.h
> @@ -22,6 +22,7 @@
>  #define RISCV_ISA_EXT_m		('m' - 'a')
>  #define RISCV_ISA_EXT_s		('s' - 'a')
>  #define RISCV_ISA_EXT_u		('u' - 'a')
> +#define RISCV_ISA_EXT_v		('v' - 'a')
>
>  /*
>   * These macros represent the logical IDs of each multi-letter RISC-V ISA
> diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h
> new file mode 100644
> index 000000000000..427a3b51df72
> --- /dev/null
> +++ b/arch/riscv/include/asm/vector.h
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2020 SiFive
> + */
> +
> +#ifndef __ASM_RISCV_VECTOR_H
> +#define __ASM_RISCV_VECTOR_H
> +
> +#include <linux/types.h>
> +
> +#ifdef CONFIG_RISCV_ISA_V
> +
> +#include <asm/hwcap.h>
> +
> +static __always_inline bool has_vector(void)
> +{
> +	return riscv_has_extension_likely(RISCV_ISA_EXT_v);

Nothing publicly availiable has V yet, so it's not likely.

> +}
> +
> +#else /* ! CONFIG_RISCV_ISA_V  */
> +
> +static __always_inline bool has_vector(void) { return false; }
> +
> +#endif /* CONFIG_RISCV_ISA_V */
> +
> +#endif /* ! __ASM_RISCV_VECTOR_H */
> diff --git a/arch/riscv/include/uapi/asm/hwcap.h b/arch/riscv/include/uapi/asm/hwcap.h
> index 46dc3f5ee99f..c52bb7bbbabe 100644
> --- a/arch/riscv/include/uapi/asm/hwcap.h
> +++ b/arch/riscv/include/uapi/asm/hwcap.h
> @@ -21,5 +21,6 @@
>  #define COMPAT_HWCAP_ISA_F	(1 << ('F' - 'A'))
>  #define COMPAT_HWCAP_ISA_D	(1 << ('D' - 'A'))
>  #define COMPAT_HWCAP_ISA_C	(1 << ('C' - 'A'))
> +#define COMPAT_HWCAP_ISA_V	(1 << ('V' - 'A'))
>
>  #endif /* _UAPI_ASM_RISCV_HWCAP_H */
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index b1d6b7e4b829..7aaf92fff64e 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -107,6 +107,7 @@ void __init riscv_fill_hwcap(void)
>  	isa2hwcap['f' - 'a'] = COMPAT_HWCAP_ISA_F;
>  	isa2hwcap['d' - 'a'] = COMPAT_HWCAP_ISA_D;
>  	isa2hwcap['c' - 'a'] = COMPAT_HWCAP_ISA_C;
> +	isa2hwcap['v' - 'a'] = COMPAT_HWCAP_ISA_V;

IMO it's OK to provide V in hwcap, as there is a "V" extension defined 
(unlike "B", for example).

>
>  	elf_hwcap = 0;
>
> @@ -267,6 +268,16 @@ void __init riscv_fill_hwcap(void)
>  		elf_hwcap &= ~COMPAT_HWCAP_ISA_F;
>  	}
>
> +	if (elf_hwcap & COMPAT_HWCAP_ISA_V) {
> +		/*
> +		 * ISA string in device tree might have 'v' flag, but
> +		 * CONFIG_RISCV_ISA_V is disabled in kernel.
> +		 * Clear V flag in elf_hwcap if CONFIG_RISCV_ISA_V is disabled.
> +		 */
> +		if (!IS_ENABLED(CONFIG_RISCV_ISA_V))
> +			elf_hwcap &= ~COMPAT_HWCAP_ISA_V;
> +	}
> +
>  	memset(print_str, 0, sizeof(print_str));
>  	for (i = 0, j = 0; i < NUM_ALPHA_EXTS; i++)
>  		if (riscv_isa[0] & BIT_MASK(i))

Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com>