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

Andrew Jones
2022-10-06T13:31:23Z

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@kernel.org>
> ---
> 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@ventanamicro.com>