Re: [PATCH 4/8] riscv: cpufeature: extend riscv_cpufeature_patch_func to all ISA extensions

[Andrew Jones]

On Thu, Oct 06, 2022 at 03:08:14PM +0800, Jisheng Zhang wrote:
> make the riscv_cpufeature_patch_func() scan all ISA extensions rather
> than limited feature macros.
> 
> Signed-off-by: Jisheng Zhang <jszhang@xxxxxxxxxx>
> ---
>  arch/riscv/include/asm/errata_list.h |  9 ++--
>  arch/riscv/kernel/cpufeature.c       | 61 +++-------------------------
>  2 files changed, 9 insertions(+), 61 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/errata_list.h b/arch/riscv/include/asm/errata_list.h
> index 19a771085781..722525f4fc96 100644
> --- a/arch/riscv/include/asm/errata_list.h
> +++ b/arch/riscv/include/asm/errata_list.h
> @@ -6,6 +6,7 @@
>  #define ASM_ERRATA_LIST_H
>  
>  #include <asm/alternative.h>
> +#include <asm/hwcap.h>
>  #include <asm/vendorid_list.h>
>  
>  #ifdef CONFIG_ERRATA_SIFIVE
> @@ -20,10 +21,6 @@
>  #define	ERRATA_THEAD_NUMBER 2
>  #endif
>  
> -#define	CPUFEATURE_SVPBMT 0
> -#define	CPUFEATURE_ZICBOM 1
> -#define	CPUFEATURE_NUMBER 2
> -
>  #ifdef __ASSEMBLY__
>  
>  #define ALT_INSN_FAULT(x)						\
> @@ -53,7 +50,7 @@ asm(ALTERNATIVE("sfence.vma %0", "sfence.vma", SIFIVE_VENDOR_ID,	\
>  #define ALT_SVPBMT(_val, prot)						\
>  asm(ALTERNATIVE_2("li %0, 0\t\nnop",					\
>  		  "li %0, %1\t\nslli %0,%0,%3", 0,			\
> -			CPUFEATURE_SVPBMT, CONFIG_RISCV_ISA_SVPBMT,	\
> +			RISCV_ISA_EXT_SVPBMT, CONFIG_RISCV_ISA_SVPBMT,	\
>  		  "li %0, %2\t\nslli %0,%0,%4", THEAD_VENDOR_ID,	\
>  			ERRATA_THEAD_PBMT, CONFIG_ERRATA_THEAD_PBMT)	\
>  		: "=r"(_val)						\
> @@ -127,7 +124,7 @@ asm volatile(ALTERNATIVE_2(						\
>  	"add a0, a0, %0\n\t"						\
>  	"2:\n\t"							\
>  	"bltu a0, %2, 3b\n\t"						\
> -	"nop", 0, CPUFEATURE_ZICBOM, CONFIG_RISCV_ISA_ZICBOM,		\
> +	"nop", 0, RISCV_ISA_EXT_ZICBOM, CONFIG_RISCV_ISA_ZICBOM,	\
>  	"mv a0, %1\n\t"							\
>  	"j 2f\n\t"							\
>  	"3:\n\t"							\
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index afa54635c180..2b1f18f97253 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -251,61 +251,11 @@ void __init riscv_fill_hwcap(void)
>  }
>  
>  #ifdef CONFIG_RISCV_ALTERNATIVE
> -static bool __init_or_module cpufeature_probe_svpbmt(unsigned int stage)
> -{
> -#ifdef CONFIG_RISCV_ISA_SVPBMT
> -	switch (stage) {
> -	case RISCV_ALTERNATIVES_EARLY_BOOT:
> -		return false;
> -	default:
> -		return riscv_isa_extension_available(NULL, SVPBMT);
> -	}
> -#endif
> -
> -	return false;
> -}
> -
> -static bool __init_or_module cpufeature_probe_zicbom(unsigned int stage)
> -{
> -#ifdef CONFIG_RISCV_ISA_ZICBOM
> -	switch (stage) {
> -	case RISCV_ALTERNATIVES_EARLY_BOOT:
> -		return false;
> -	default:
> -		return riscv_isa_extension_available(NULL, ZICBOM);
> -	}
> -#endif
> -
> -	return false;
> -}
> -
> -/*
> - * Probe presence of individual extensions.
> - *
> - * This code may also be executed before kernel relocation, so we cannot use
> - * addresses generated by the address-of operator as they won't be valid in
> - * this context.
> - */
> -static u32 __init_or_module cpufeature_probe(unsigned int stage)
> -{
> -	u32 cpu_req_feature = 0;
> -
> -	if (cpufeature_probe_svpbmt(stage))
> -		cpu_req_feature |= (1U << CPUFEATURE_SVPBMT);
> -
> -	if (cpufeature_probe_zicbom(stage))
> -		cpu_req_feature |= (1U << CPUFEATURE_ZICBOM);
> -
> -	return cpu_req_feature;
> -}
> -
>  void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin,
>  						  struct alt_entry *end,
>  						  unsigned int stage)
>  {
> -	u32 cpu_req_feature = cpufeature_probe(stage);
>  	struct alt_entry *alt;
> -	u32 tmp;
>  
>  	if (stage == RISCV_ALTERNATIVES_EARLY_BOOT)
>  		return;
> @@ -313,15 +263,16 @@ void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin,
>  	for (alt = begin; alt < end; alt++) {
>  		if (alt->vendor_id != 0)
>  			continue;
> -		if (alt->errata_id >= CPUFEATURE_NUMBER) {
> -			WARN(1, "This feature id:%d is not in kernel cpufeature list",
> +		if (alt->errata_id >= RISCV_ISA_EXT_ID_MAX) {

Ok, so RISCV_ISA_EXT_ID_MAX is used here now, but we could use
RISCV_ISA_EXT_MAX directly instead.

> +			WARN(1, "This extension id:%d is not in ISA extension list",
                                                               ^ the
>  				alt->errata_id);
>  			continue;
>  		}
>  
> -		tmp = (1U << alt->errata_id);
> -		if (cpu_req_feature & tmp)
> -			patch_text_nosync(alt->old_ptr, alt->alt_ptr, alt->alt_len);
> +		if (!test_bit(alt->errata_id, riscv_isa))

Maybe avoid using riscv_isa directly in case we want to move this function
out of cpufeature.c some day?

  if (!__riscv_isa_extension_available(NULL, alt->errata_id))


> +			continue;
> +
> +		patch_text_nosync(alt->old_ptr, alt->alt_ptr, alt->alt_len);
>  	}
>  }
>  #endif
> -- 
> 2.37.2
>

Otherwise,

Reviewed-by: Andrew Jones <ajones@xxxxxxxxxxxxxxxx>