Re: [PATCH v3 08/17] riscv: Introduce vendor variants of extension helpers

On Sat, Apr 20, 2024 at 06:04:40PM -0700, Charlie Jenkins wrote:

Vendor extensions are maintained in per-vendor structs (separate from standard extensions which live in riscv_isa). Create vendor variants for the existing extension helpers to interface with the riscv_isa_vendor bitmaps.

There is a good amount of overlap between these functions, so the alternative checking code can be factored out.

Can you please split this out?

Signed-off-by: Charlie Jenkins charlie@rivosinc.com

arch/riscv/errata/sifive/errata.c | 3 ++ arch/riscv/errata/thead/errata.c | 3 ++ arch/riscv/include/asm/cpufeature.h | 86 +++++++++++++++++------------- arch/riscv/include/asm/vendor_extensions.h | 56 +++++++++++++++++++ arch/riscv/kernel/cpufeature.c | 20 ++++--- arch/riscv/kernel/vendor_extensions.c | 40 ++++++++++++++ 6 files changed, 164 insertions(+), 44 deletions(-)

diff --git a/arch/riscv/errata/sifive/errata.c b/arch/riscv/errata/sifive/errata.c index 3d9a32d791f7..b29b6e405ff2 100644 --- a/arch/riscv/errata/sifive/errata.c +++ b/arch/riscv/errata/sifive/errata.c @@ -12,6 +12,7 @@ #include <asm/alternative.h> #include <asm/vendorid_list.h> #include <asm/errata_list.h> +#include <asm/vendor_extensions.h> struct errata_info_t { char name[32]; @@ -99,6 +100,8 @@ void sifive_errata_patch_func(struct alt_entry *begin, struct alt_entry *end, for (alt = begin; alt < end; alt++) { if (alt->vendor_id != SIFIVE_VENDOR_ID) continue;

• if (alt->patch_id >= RISCV_VENDOR_EXT_ALTERNATIVES_BASE)
  

• 	continue;
  

  if (alt->patch_id >= ERRATA_SIFIVE_NUMBER) { WARN(1, "This errata id:%d is not in kernel errata list", alt->patch_id); continue;

diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c index b1c410bbc1ae..d8e78cc687bc 100644 --- a/arch/riscv/errata/thead/errata.c +++ b/arch/riscv/errata/thead/errata.c @@ -18,6 +18,7 @@ #include <asm/io.h> #include <asm/patch.h> #include <asm/vendorid_list.h> +#include <asm/vendor_extensions.h> static bool errata_probe_pbmt(unsigned int stage, unsigned long arch_id, unsigned long impid) @@ -163,6 +164,8 @@ void thead_errata_patch_func(struct alt_entry *begin, struct alt_entry *end, for (alt = begin; alt < end; alt++) { if (alt->vendor_id != THEAD_VENDOR_ID) continue;

• if (alt->patch_id >= RISCV_VENDOR_EXT_ALTERNATIVES_BASE)
  

• 	continue;
  

if (alt->patch_id >= ERRATA_THEAD_NUMBER)

This number is 2, how does the patching actually work for vendor stuff when the base is always greater than 2?

	continue;

diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h index db6a6d7d6b2e..83e1143db9ad 100644 --- a/arch/riscv/include/asm/cpufeature.h +++ b/arch/riscv/include/asm/cpufeature.h @@ -103,22 +103,13 @@ bool __riscv_isa_extension_available(const unsigned long *isa_bitmap, unsigned i __riscv_isa_extension_available(isa_bitmap, RISCV_ISA_EXT_##ext) static __always_inline bool -riscv_has_extension_likely(const unsigned long ext) +__riscv_has_extension_likely_alternatives(const unsigned long vendor, const unsigned long ext) {

• compiletime_assert(ext < RISCV_ISA_EXT_MAX,

• 	   "ext must be < RISCV_ISA_EXT_MAX");
  

• if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) {

• asm goto(
  

• ALTERNATIVE("j	%l[l_no]", "nop", 0, %[ext], 1)
  

• :
  

• : [ext] "i" (ext)
  

• :
  

• : l_no);
  

• } else {

• if (!__riscv_isa_extension_available(NULL, ext))
  

• 	goto l_no;
  

• }

• asm goto(ALTERNATIVE("j %l[l_no]", "nop", %[vendor], %[ext], 1)
• :
• : [vendor] "i" (vendor), [ext] "i" (ext)
• :
• : l_no);

return true; l_no: @@ -126,42 +117,65 @@ riscv_has_extension_likely(const unsigned long ext) } static __always_inline bool -riscv_has_extension_unlikely(const unsigned long ext) +__riscv_has_extension_unlikely_alternatives(const unsigned long vendor, const unsigned long ext)

ngl, I think you could drop the _alternatives from these - the likely/unlikely is only actually a thing because of the alternatives in the first place & just retain the __ as a differentiator. That'd help you with some of the long-line wrangling you've been doing below.

{

• compiletime_assert(ext < RISCV_ISA_EXT_MAX,

• 	   "ext must be < RISCV_ISA_EXT_MAX");
  

• if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) {

• asm goto(
  

• ALTERNATIVE("nop", "j	%l[l_yes]", 0, %[ext], 1)
  

• :
  

• : [ext] "i" (ext)
  

• :
  

• : l_yes);
  

• } else {

• if (__riscv_isa_extension_available(NULL, ext))
  

• 	goto l_yes;
  

• }

• asm goto(ALTERNATIVE("nop", "j %l[l_yes]", %[vendor], %[ext], 1)
• :
• : [vendor] "i" (vendor), [ext] "i" (ext)
• :
• : l_yes);

return false; l_yes: return true; } +static __always_inline bool +riscv_has_extension_likely(const unsigned long ext)

Can you format this so that its on one line & wrap the arguments if needs be?

+{

• compiletime_assert(ext < RISCV_ISA_EXT_MAX,

• 	   "ext must be < RISCV_ISA_EXT_MAX");
  

• if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE))

• return __riscv_has_extension_likely_alternatives(0, ext);
  

• else

I'm almost certain I said this before, but none of the else branches are needed here, there's a return in the if branch, so the remainder of the function becomes unconditionally executed.

• return __riscv_isa_extension_available(NULL, ext);
  

+}

+static __always_inline bool +riscv_has_extension_unlikely(const unsigned long ext) +{

• compiletime_assert(ext < RISCV_ISA_EXT_MAX,

• 	   "ext must be < RISCV_ISA_EXT_MAX");
  

• if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE))

• return __riscv_has_extension_unlikely_alternatives(0, ext);
  

• else

• return __riscv_isa_extension_available(NULL, ext);
  

+}

static __always_inline bool riscv_cpu_has_extension_likely(int cpu, const unsigned long ext) {

• if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_likely(ext))

• return true;
  

• compiletime_assert(ext < RISCV_ISA_EXT_MAX,

• 	   "ext must be < RISCV_ISA_EXT_MAX");
  

• return __riscv_isa_extension_available(hart_isa[cpu].isa, ext);

• if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) &&

•    __riscv_has_extension_likely_alternatives(0, ext))
  

0 is meaningless, please make this more understandable using a define of some sort.

• return true;
  

• else

• return __riscv_isa_extension_available(hart_isa[cpu].isa, ext);
  

} static __always_inline bool riscv_cpu_has_extension_unlikely(int cpu, const unsigned long ext) {

• if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_unlikely(ext))

• return true;
  

• compiletime_assert(ext < RISCV_ISA_EXT_MAX,

• 	   "ext must be < RISCV_ISA_EXT_MAX");
  

• return __riscv_isa_extension_available(hart_isa[cpu].isa, ext);

• if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) &&

•    __riscv_has_extension_unlikely_alternatives(0, ext))
  

• return true;
  

• else

• return __riscv_isa_extension_available(hart_isa[cpu].isa, ext);
  

} #endif diff --git a/arch/riscv/include/asm/vendor_extensions.h b/arch/riscv/include/asm/vendor_extensions.h index 0af1ddd0af70..3e676a96016e 100644 --- a/arch/riscv/include/asm/vendor_extensions.h +++ b/arch/riscv/include/asm/vendor_extensions.h @@ -23,4 +23,60 @@ extern const struct riscv_isa_vendor_ext_data_list *riscv_isa_vendor_ext_list[]; extern const size_t riscv_isa_vendor_ext_list_size; +/*

• • The alternatives need some way of distinguishing between vendor extensions
• • and errata. Incrementing all of the vendor extension keys so they are at
• • least 0x8000 accomplishes that.
• */

+#define RISCV_VENDOR_EXT_ALTERNATIVES_BASE 0x8000

+bool __riscv_isa_vendor_extension_available(int cpu, unsigned long vendor, unsigned int bit); +#define riscv_cpu_isa_vendor_extension_available(cpu, vendor, ext) \

• __riscv_isa_vendor_extension_available(cpu, vendor, RISCV_ISA_VENDOR_EXT_##ext)

+#define riscv_isa_vendor_extension_available(vendor, ext) \

• __riscv_isa_vendor_extension_available(-1, vendor, RISCV_ISA_VENDOR_EXT_##ext)

+static __always_inline bool +riscv_has_vendor_extension_likely(const unsigned long vendor, const unsigned long ext) +{

• if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE))

• return __riscv_has_extension_likely_alternatives(vendor,
  

• 						 ext + RISCV_VENDOR_EXT_ALTERNATIVES_BASE);
  

• else

• return __riscv_isa_vendor_extension_available(-1, vendor, ext);
  

+}

+static __always_inline bool +riscv_has_vendor_extension_unlikely(const unsigned long vendor, const unsigned long ext) +{

• if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE))

• return __riscv_has_extension_unlikely_alternatives(vendor,
  

• 						   ext + RISCV_VENDOR_EXT_ALTERNATIVES_BASE);
  

• else

• return __riscv_isa_vendor_extension_available(-1, vendor, ext);
  

+}

+static __always_inline bool riscv_cpu_has_vendor_extension_likely(const unsigned long vendor,

• 						  int cpu, const unsigned long ext)
  

+{

• if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) &&

•    __riscv_has_extension_likely_alternatives(vendor,
  

• 				      ext + RISCV_VENDOR_EXT_ALTERNATIVES_BASE))
  

• return true;
  

• else

• return __riscv_isa_vendor_extension_available(cpu, vendor, ext);
  

+}

+static __always_inline bool riscv_cpu_has_vendor_extension_unlikely(const unsigned long vendor,

• 						    int cpu,
  

• 						    const unsigned long ext)
  

+{

• if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) &&

•    __riscv_has_extension_unlikely_alternatives(vendor,
  

• 					ext + RISCV_VENDOR_EXT_ALTERNATIVES_BASE))
  

• return true;
  

• else

• return __riscv_isa_vendor_extension_available(cpu, vendor, ext);
  

+}

#endif /* _ASM_VENDOR_EXTENSIONS_H */ diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c index c9f36822e337..17371887abcc 100644 --- a/arch/riscv/kernel/cpufeature.c +++ b/arch/riscv/kernel/cpufeature.c @@ -833,25 +833,29 @@ void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin, { struct alt_entry *alt; void *oldptr, *altptr;

• u16 id, value;

• u16 id, value, vendor;

if (stage == RISCV_ALTERNATIVES_EARLY_BOOT) return; for (alt = begin; alt < end; alt++) {

• if (alt->vendor_id != 0)
  

• 	continue;
  

• id = PATCH_ID_CPUFEATURE_ID(alt->patch_id);

• vendor = PATCH_ID_CPUFEATURE_ID(alt->vendor_id);
  

• if (id >= RISCV_ISA_EXT_MAX) {
  

• if (id < RISCV_ISA_EXT_MAX) {
  

I think any reliance on the standard ext max requires a comment explaining what the interaction is between it and the vendor stuff.

• 	if (alt->vendor_id != 0)
  

• 		continue;
  

If this happens, it's a bug, should we be continuing silently?

Cheers, Conor.

• 	if (!__riscv_isa_extension_available(NULL, id))
  

• 		continue;
  

• } else if (id >= RISCV_VENDOR_EXT_ALTERNATIVES_BASE) {
  

• 	if (!__riscv_isa_vendor_extension_available(-1, vendor, id))
  

• 		continue;
  

• } else {
  WARN(1, "This extension id:%d is not in ISA extension list", id);
  continue;
  

  }

• if (!__riscv_isa_extension_available(NULL, id))
  

• 	continue;
  

• value = PATCH_ID_CPUFEATURE_VALUE(alt->patch_id); if (!riscv_cpufeature_patch_check(id, value)) continue;

diff --git a/arch/riscv/kernel/vendor_extensions.c b/arch/riscv/kernel/vendor_extensions.c index f76cb3013c2d..eced93eec5a6 100644 --- a/arch/riscv/kernel/vendor_extensions.c +++ b/arch/riscv/kernel/vendor_extensions.c @@ -3,6 +3,7 @@

• Copyright 2024 Rivos, Inc

*/ +#include <asm/vendorid_list.h> #include <asm/vendor_extensions.h> #include <asm/vendor_extensions/thead.h> @@ -16,3 +17,42 @@ const struct riscv_isa_vendor_ext_data_list *riscv_isa_vendor_ext_list[] = { }; const size_t riscv_isa_vendor_ext_list_size = ARRAY_SIZE(riscv_isa_vendor_ext_list);

+/**

• • __riscv_isa_vendor_extension_available() - Check whether given vendor
• • extension is available or not.
• • @cpu: check if extension is available on this cpu
• • @vendor: vendor that the extension is a member of
• • @bit: bit position of the desired extension
• • Return: true or false
• • NOTE: When cpu is -1, will check if extension is available on all cpus
• */

+bool __riscv_isa_vendor_extension_available(int cpu, unsigned long vendor, unsigned int bit) +{

• unsigned long *bmap;
• struct riscv_isainfo *cpu_bmap;
• size_t bmap_size;
• switch (vendor) {

+#ifdef CONFIG_RISCV_ISA_VENDOR_EXT_THEAD

• case THEAD_VENDOR_ID:

• bmap = riscv_isa_vendor_ext_list_thead.vendor_bitmap;
  

• cpu_bmap = riscv_isa_vendor_ext_list_thead.per_hart_vendor_bitmap;
  

• bmap_size = riscv_isa_vendor_ext_list_thead.bitmap_size;
  

• break;
  

+#endif

• default:

• return false;
  

• }
• if (cpu != -1)

• bmap = cpu_bmap[cpu].isa;
  

• if (bit >= bmap_size)

• return false;
  

• return test_bit(bit, bmap) ? true : false;

+} +EXPORT_SYMBOL_GPL(__riscv_isa_vendor_extension_available);

-- 2.44.0