On Fri, Apr 26, 2024 at 02:29:17PM -0700, Charlie Jenkins wrote:

If vlenb is provided in the device tree, prefer that over reading the vlenb csr.

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

arch/riscv/include/asm/cpufeature.h | 2 ++ arch/riscv/kernel/cpufeature.c | 43 +++++++++++++++++++++++++++++++++++++ arch/riscv/kernel/vector.c | 12 ++++++++++- 3 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h index 347805446151..0c4f08577015 100644 --- a/arch/riscv/include/asm/cpufeature.h +++ b/arch/riscv/include/asm/cpufeature.h @@ -31,6 +31,8 @@ DECLARE_PER_CPU(struct riscv_cpuinfo, riscv_cpuinfo); /* Per-cpu ISA extensions. */ extern struct riscv_isainfo hart_isa[NR_CPUS]; +extern u32 riscv_vlenb_of;

void riscv_user_isa_enable(void); #if defined(CONFIG_RISCV_MISALIGNED) diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c index 3ed2359eae35..8158f34c3e36 100644 --- a/arch/riscv/kernel/cpufeature.c +++ b/arch/riscv/kernel/cpufeature.c @@ -35,6 +35,8 @@ static DECLARE_BITMAP(riscv_isa, RISCV_ISA_EXT_MAX) __read_mostly; /* Per-cpu ISA extensions. */ struct riscv_isainfo hart_isa[NR_CPUS]; +u32 riscv_vlenb_of;

/**

• riscv_isa_extension_base() - Get base extension word

@@ -648,6 +650,42 @@ static int __init riscv_isa_fallback_setup(char *__unused) early_param("riscv_isa_fallback", riscv_isa_fallback_setup); #endif +static int riscv_homogeneous_vlenb(void)

Without a verb, this function name is rather odd.

+{

• int cpu;
• u32 prev_vlenb = 0;
• u32 vlenb;
• for_each_possible_cpu(cpu) {

• struct device_node *cpu_node;
  

• cpu_node = of_cpu_device_node_get(cpu);
  

• if (!cpu_node) {
  

• 	pr_warn("Unable to find cpu node\n");
  

• 	continue;
  

Hmm, if we fail to find the cpu node, then shouldn't we be returning an error?

• }
  

• if (of_property_read_u32(cpu_node, "riscv,vlenb", &vlenb)) {
  

• 	of_node_put(cpu_node);
  

• 	if (prev_vlenb)
  

• 		return -1;
  

Can you return an errno here and below please?

• 	continue;
  

• }
  

• if (prev_vlenb && vlenb != prev_vlenb) {
  

• 	of_node_put(cpu_node);
  

• 	return -1;
  

• }
  

• prev_vlenb = vlenb;
  

• of_node_put(cpu_node);
  

• }
• riscv_vlenb_of = vlenb;
• return 0;

+}

void __init riscv_fill_hwcap(void) { char print_str[NUM_ALPHA_EXTS + 1]; @@ -671,6 +709,11 @@ void __init riscv_fill_hwcap(void) pr_info("Falling back to deprecated "riscv,isa"\n"); riscv_fill_hwcap_from_isa_string(isa2hwcap); }

• if (riscv_homogeneous_vlenb() < 0) {
  

• 	pr_warn("RISCV_ISA_V only supports one vlenb on SMP systems. Please ensure that the riscv,vlenb devicetree property is the same across all CPUs. Either all CPUs must have the riscv,vlenb property, or none. If no CPUs in the devicetree use riscv,vlenb then vlenb will be probed from the vlenb CSR. Disabling vector.\n");
  

Oh dear, that's a bit unwieldy... I think you could get away with a far more basic message - and you should be able to break this over lines, adjacent string literals should get concatenated. I'd probably say something like "unsupported heterogeneous vlen detected, vector extension disabled", however we should actually check that the vector extension has been detected on all CPUs and that kernel support for vector is enabled before emitting a warning for this.

Cheers, Conor.

• 	elf_hwcap &= ~COMPAT_HWCAP_ISA_V;
  

• }
  

  }

/* diff --git a/arch/riscv/kernel/vector.c b/arch/riscv/kernel/vector.c index 6727d1d3b8f2..e04586cdb7f0 100644 --- a/arch/riscv/kernel/vector.c +++ b/arch/riscv/kernel/vector.c @@ -33,7 +33,17 @@ int riscv_v_setup_vsize(void) { unsigned long this_vsize;

• /* There are 32 vector registers with vlenb length. */

• /*

• * There are 32 vector registers with vlenb length.
  

• *
  

• * If the riscv,vlenb property was provided by the firmware, use that
  

• * instead of probing the CSRs.
  

• */
  

• if (riscv_vlenb_of) {

• this_vsize = riscv_vlenb_of * 32;
  

• return 0;
  

• }
• riscv_v_enable(); this_vsize = csr_read(CSR_VLENB) * 32; riscv_v_disable();

-- 2.44.0

On Fri, Apr 26, 2024 at 02:29:19PM -0700, Charlie Jenkins wrote:

Separate vendor extensions out into one struct per vendor instead of adding vendor extensions onto riscv_isa_ext.

Add a hidden config RISCV_ISA_VENDOR_EXT to conditionally include this code.

The xtheadvector vendor extension is added using these changes.

This mostly looks good to me, thanks for the updates. There's one thing that I think is wrong, but I need to test and will get back to you on...

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

arch/riscv/Kconfig | 2 + arch/riscv/Kconfig.vendor | 19 ++++++ arch/riscv/include/asm/cpufeature.h | 18 ++++++ arch/riscv/include/asm/vendor_extensions.h | 26 ++++++++ arch/riscv/include/asm/vendor_extensions/thead.h | 19 ++++++ arch/riscv/kernel/Makefile | 2 + arch/riscv/kernel/cpufeature.c | 77 ++++++++++++++++++------ arch/riscv/kernel/vendor_extensions.c | 18 ++++++ arch/riscv/kernel/vendor_extensions/Makefile | 3 + arch/riscv/kernel/vendor_extensions/thead.c | 36 +++++++++++ 10 files changed, 200 insertions(+), 20 deletions(-)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig index be09c8836d56..fec86fba3acd 100644 --- a/arch/riscv/Kconfig +++ b/arch/riscv/Kconfig @@ -759,6 +759,8 @@ config RISCV_EFFICIENT_UNALIGNED_ACCESS endchoice +source "arch/riscv/Kconfig.vendor"

endmenu # "Platform type" menu "Kernel features" diff --git a/arch/riscv/Kconfig.vendor b/arch/riscv/Kconfig.vendor new file mode 100644 index 000000000000..4fc86810af1d --- /dev/null +++ b/arch/riscv/Kconfig.vendor @@ -0,0 +1,19 @@ +menu "Vendor extensions"

+config RISCV_ISA_VENDOR_EXT

• bool

+menu "T-Head" +config RISCV_ISA_VENDOR_EXT_THEAD

• bool "T-Head vendor extension support"
• select RISCV_ISA_VENDOR_EXT
• default y
• help

•  Say N here if you want to disable all T-Head vendor extension
  

•  support. This will cause any T-Head vendor extensions that are
  

•  requested to be ignored.
  

What does "requested to be ignored" mean to a punter configuring a kernel? I'd expect this to be something like:

"Say N here to disable detection of and support for all T-Head vendor extensions. Without this option enabled, T-Head vendor extensions will not be detected at boot and their presence not reported to userspace."

In general, I'd expect something that needs some support in the kernel (like vector) to function to have a dedicated option, but the likes of their Zba variant could be detected and reported via hwprobe et al once RISCV_ISA_VENDOR_EXT_THEAD is enabled.

Cheers, Conor.

On Fri, Apr 26, 2024 at 02:29:19PM -0700, Charlie Jenkins wrote:

@@ -353,6 +336,10 @@ static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct risc bool ext_long = false, ext_err = false; switch (*ext) {

• case 'x':
  

• case 'X':
  

• 	pr_warn_once("Vendor extensions are ignored in riscv,isa. Use riscv,isa-extensions instead.");
  

• 	continue;
  

Yeah, so this is not right - you need to find the end of the extension before containing - for example if I had a system with rv64imafdcxconorkwe, you get something like: [ 0.000000] CPU with hartid=0 is not available [ 0.000000] Falling back to deprecated "riscv,isa" [ 0.000000] Vendor extensions are ignored in riscv,isa. Use riscv,isa-extensions instead. [ 0.000000] riscv: base ISA extensions acdefikmnorw [ 0.000000] riscv: ELF capabilities acdfim

kwe are all pretty safe letters, but suppose one was a v.. I think you can just yoink the code from the s/z case:

diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c index 20bc9ba6b7a2..4daedba7961f 100644 --- a/arch/riscv/kernel/cpufeature.c +++ b/arch/riscv/kernel/cpufeature.c @@ -338,8 +338,19 @@ static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct risc switch (*ext) { case 'x': case 'X': - pr_warn_once("Vendor extensions are ignored in riscv,isa. Use riscv,isa-extensions instead."); - continue; + if (acpi_disabled) + pr_warn_once("Vendor extensions are ignored in riscv,isa. Use riscv,isa-extensions instead."); + /* + * To skip an extension, we find its end. + * As multi-letter extensions must be split from other multi-letter + * extensions with an "_", the end of a multi-letter extension will + * either be the null character or the "_" at the start of the next + * multi-letter extension. + */ + for (; *isa && *isa != '_'; ++isa) + ; + ext_err = true; + break; case 's': /* * Workaround for invalid single-letter 's' & 'u' (QEMU).

You'll note that I made the dt property error dt only, this function gets called for ACPI too. I think the skip is pretty safe there though at the moment, we've not established any meanings yet for vendor stuff on ACPI. I think breaking is probably better than using continue - we get the _ skip from outside the switch statement out of that. And ye, I am lazy so I kept it as a for loop.

Cheers, Conor.

On Wed, May 01, 2024 at 09:44:15AM -0700, Evan Green wrote:

On Fri, Apr 26, 2024 at 2:29 PM Charlie Jenkins charlie@rivosinc.com wrote:

...

Separate vendor extensions out into one struct per vendor instead of adding vendor extensions onto riscv_isa_ext.

Add a hidden config RISCV_ISA_VENDOR_EXT to conditionally include this code.

The xtheadvector vendor extension is added using these changes.

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

arch/riscv/Kconfig | 2 + arch/riscv/Kconfig.vendor | 19 ++++++ arch/riscv/include/asm/cpufeature.h | 18 ++++++ arch/riscv/include/asm/vendor_extensions.h | 26 ++++++++ arch/riscv/include/asm/vendor_extensions/thead.h | 19 ++++++ arch/riscv/kernel/Makefile | 2 + arch/riscv/kernel/cpufeature.c | 77 ++++++++++++++++++------ arch/riscv/kernel/vendor_extensions.c | 18 ++++++ arch/riscv/kernel/vendor_extensions/Makefile | 3 + arch/riscv/kernel/vendor_extensions/thead.c | 36 +++++++++++ 10 files changed, 200 insertions(+), 20 deletions(-)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig index be09c8836d56..fec86fba3acd 100644 --- a/arch/riscv/Kconfig +++ b/arch/riscv/Kconfig @@ -759,6 +759,8 @@ config RISCV_EFFICIENT_UNALIGNED_ACCESS

endchoice

+source "arch/riscv/Kconfig.vendor"

endmenu # "Platform type"

menu "Kernel features" diff --git a/arch/riscv/Kconfig.vendor b/arch/riscv/Kconfig.vendor new file mode 100644 index 000000000000..4fc86810af1d --- /dev/null +++ b/arch/riscv/Kconfig.vendor @@ -0,0 +1,19 @@ +menu "Vendor extensions"

+config RISCV_ISA_VENDOR_EXT

•   bool
  

+menu "T-Head" +config RISCV_ISA_VENDOR_EXT_THEAD

•   bool "T-Head vendor extension support"
  

•   select RISCV_ISA_VENDOR_EXT
  

•   default y
  

•   help
  

•     Say N here if you want to disable all T-Head vendor extension
  

•     support. This will cause any T-Head vendor extensions that are
  

•     requested to be ignored.
  

•     If you don't know what to do here, say Y.
  

+endmenu

+endmenu diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h index 0c4f08577015..fedd479ccfd1 100644 --- a/arch/riscv/include/asm/cpufeature.h +++ b/arch/riscv/include/asm/cpufeature.h @@ -35,6 +35,24 @@ extern u32 riscv_vlenb_of;

void riscv_user_isa_enable(void);

+#define _RISCV_ISA_EXT_DATA(_name, _id, _subset_exts, _subset_exts_size) { \

•   .name = #_name,                                                         \
  

•   .property = #_name,                                                     \
  

•   .id = _id,                                                              \
  

•   .subset_ext_ids = _subset_exts,                                         \
  

•   .subset_ext_size = _subset_exts_size                                    \
  

+}

+#define __RISCV_ISA_EXT_DATA(_name, _id) _RISCV_ISA_EXT_DATA(_name, _id, NULL, 0)

+/* Used to declare pure "lasso" extension (Zk for instance) */ +#define __RISCV_ISA_EXT_BUNDLE(_name, _bundled_exts) \

•   _RISCV_ISA_EXT_DATA(_name, RISCV_ISA_EXT_INVALID, _bundled_exts, ARRAY_SIZE(_bundled_exts))
  

+/* Used to declare extensions that are a superset of other extensions (Zvbb for instance) */ +#define __RISCV_ISA_EXT_SUPERSET(_name, _id, _sub_exts) \

•   _RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts))
  

#if defined(CONFIG_RISCV_MISALIGNED) bool check_unaligned_access_emulated_all_cpus(void); void unaligned_emulation_finish(void); diff --git a/arch/riscv/include/asm/vendor_extensions.h b/arch/riscv/include/asm/vendor_extensions.h new file mode 100644 index 000000000000..0af1ddd0af70 --- /dev/null +++ b/arch/riscv/include/asm/vendor_extensions.h @@ -0,0 +1,26 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/*

• • Copyright 2024 Rivos, Inc
• */

+#ifndef _ASM_VENDOR_EXTENSIONS_H +#define _ASM_VENDOR_EXTENSIONS_H

+#include <asm/cpufeature.h>

+#include <linux/array_size.h> +#include <linux/types.h>

+struct riscv_isa_vendor_ext_data_list {

•   const struct riscv_isa_ext_data *ext_data;
  

•   struct riscv_isainfo *per_hart_vendor_bitmap;
  

•   unsigned long *vendor_bitmap;
  

It took a lot of digging for me to understand this was the set of vendor extensions supported on all harts. Can we add that to the name, maybe something like isa_bitmap_all_harts? (I wonder if we could drop the vendor part of the name since we already know we're in a vendor_ext_data_list structure).

Sure, I figured it was implied since the other bitmap says "per_hart", but I can see how it could be confusing.

...

•   const size_t ext_data_count;
  

•   const size_t bitmap_size;
  

+};

+extern const struct riscv_isa_vendor_ext_data_list *riscv_isa_vendor_ext_list[];

+extern const size_t riscv_isa_vendor_ext_list_size;

+#endif /* _ASM_VENDOR_EXTENSIONS_H */ diff --git a/arch/riscv/include/asm/vendor_extensions/thead.h b/arch/riscv/include/asm/vendor_extensions/thead.h new file mode 100644 index 000000000000..92eec729888d --- /dev/null +++ b/arch/riscv/include/asm/vendor_extensions/thead.h @@ -0,0 +1,19 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _ASM_RISCV_VENDOR_EXTENSIONS_THEAD_H +#define _ASM_RISCV_VENDOR_EXTENSIONS_THEAD_H

+#include <asm/vendor_extensions.h>

+#include <linux/types.h>

+#define RISCV_ISA_VENDOR_EXT_XTHEADVECTOR 0

+/*

• • Extension keys should be strictly less than max.
• • It is safe to increment this when necessary.
• */

+#define RISCV_ISA_VENDOR_EXT_MAX_THEAD 32

+extern const struct riscv_isa_vendor_ext_data_list riscv_isa_vendor_ext_list_thead;

+#endif diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile index 81d94a8ee10f..53361c50fb46 100644 --- a/arch/riscv/kernel/Makefile +++ b/arch/riscv/kernel/Makefile @@ -58,6 +58,8 @@ obj-y += riscv_ksyms.o obj-y += stacktrace.o obj-y += cacheinfo.o obj-y += patch.o +obj-y += vendor_extensions.o +obj-y += vendor_extensions/ obj-y += probes/ obj-y += tests/ obj-$(CONFIG_MMU) += vdso.o vdso/ diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c index 8158f34c3e36..c073494519eb 100644 --- a/arch/riscv/kernel/cpufeature.c +++ b/arch/riscv/kernel/cpufeature.c @@ -24,6 +24,7 @@ #include <asm/processor.h> #include <asm/sbi.h> #include <asm/vector.h> +#include <asm/vendor_extensions.h>

#define NUM_ALPHA_EXTS ('z' - 'a' + 1)

@@ -102,24 +103,6 @@ static bool riscv_isa_extension_check(int id) return true; }

-#define _RISCV_ISA_EXT_DATA(_name, _id, _subset_exts, _subset_exts_size) { \

•   .name = #_name,                                                         \
  

•   .property = #_name,                                                     \
  

•   .id = _id,                                                              \
  

•   .subset_ext_ids = _subset_exts,                                         \
  

•   .subset_ext_size = _subset_exts_size                                    \
  

-}

-#define __RISCV_ISA_EXT_DATA(_name, _id) _RISCV_ISA_EXT_DATA(_name, _id, NULL, 0)

-/* Used to declare pure "lasso" extension (Zk for instance) */ -#define __RISCV_ISA_EXT_BUNDLE(_name, _bundled_exts) \

•   _RISCV_ISA_EXT_DATA(_name, RISCV_ISA_EXT_INVALID, _bundled_exts, ARRAY_SIZE(_bundled_exts))
  

-/* Used to declare extensions that are a superset of other extensions (Zvbb for instance) */ -#define __RISCV_ISA_EXT_SUPERSET(_name, _id, _sub_exts) \

•   _RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts))
  

static const unsigned int riscv_zk_bundled_exts[] = { RISCV_ISA_EXT_ZBKB, RISCV_ISA_EXT_ZBKC, @@ -353,6 +336,10 @@ static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct risc bool ext_long = false, ext_err = false;

            switch (*ext) {

•           case 'x':
  

•           case 'X':
  

•                   pr_warn_once("Vendor extensions are ignored in riscv,isa. Use riscv,isa-extensions instead.");
  

•                   continue;
            case 's':
                    /*
                     * Workaround for invalid single-letter 's' & 'u' (QEMU).
  

@@ -368,8 +355,6 @@ static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct risc } fallthrough; case 'S':

•           case 'x':
  

•           case 'X':
            case 'z':
            case 'Z':
                    /*
  

@@ -572,6 +557,54 @@ static void __init riscv_fill_hwcap_from_isa_string(unsigned long *isa2hwcap) acpi_put_table((struct acpi_table_header *)rhct); }

+static void __init riscv_fill_cpu_vendor_ext(struct device_node *cpu_node, int cpu) +{

•   if (!IS_ENABLED(CONFIG_RISCV_ISA_VENDOR_EXT))
  

•           return;
  

•   for (int i = 0; i < riscv_isa_vendor_ext_list_size; i++) {
  

•           const struct riscv_isa_vendor_ext_data_list *ext_list = riscv_isa_vendor_ext_list[i];
  

•           for (int j = 0; j < ext_list->ext_data_count; j++) {
  

•                   const struct riscv_isa_ext_data ext = ext_list->ext_data[j];
  

•                   struct riscv_isainfo *isavendorinfo = &ext_list->per_hart_vendor_bitmap[cpu];
  

•                   if (of_property_match_string(cpu_node, "riscv,isa-extensions",
  

•                                                ext.property) < 0)
  

•                           continue;
  

•                   /*
  

•                    * Assume that subset extensions are all members of the
  

•                    * same vendor.
  

•                    */
  

•                   if (ext.subset_ext_size)
  

•                           for (int k = 0; k < ext.subset_ext_size; k++)
  

•                                   set_bit(ext.subset_ext_ids[k], isavendorinfo->isa);
  

•                   set_bit(ext.id, isavendorinfo->isa);
  

•           }
  

This loop seems super similar to the regular one (in riscv_fill_hwcap_from_ext_list() in the random, possibly old, kernel I have open). Could we refactor these together into a common helper? The other loop has an extra stanza for riscv_isa_extension_check(), so we'd have to add an extra condition there, but otherwise it looks pretty compatible?

I actually did have this refactored into a single function in a previous version but broke it back up since I felt there just wasn't enough overlap. The one for standard extensions is:

for (int i = 0; i < riscv_isa_ext_count; i++) { const struct riscv_isa_ext_data *ext = &riscv_isa_ext[i];

if (of_property_match_string(cpu_node, "riscv,isa-extensions", ext->property) < 0) continue;

if (ext->subset_ext_size) { for (int j = 0; j < ext->subset_ext_size; j++) { if (riscv_isa_extension_check(ext->subset_ext_ids[i])) set_bit(ext->subset_ext_ids[j], isainfo->isa); } }

if (riscv_isa_extension_check(ext->id)) { set_bit(ext->id, isainfo->isa);

/* Only single letter extensions get set in hwcap */ if (strnlen(riscv_isa_ext[i].name, 2) == 1) this_hwcap |= isa2hwcap[riscv_isa_ext[i].id]; } }

The motivating reason why I didn't combine them was the additional `struct riscv_isa_vendor_ext_data_list *` data type for the vendor version which contains ext and isainfo. This can probably be combined in a straight-forward way though.

...

•   }
  

+}

+static void __init riscv_fill_vendor_ext_list(int cpu) +{

•   if (!IS_ENABLED(CONFIG_RISCV_ISA_VENDOR_EXT))
  

•           return;
  

•   for (int i = 0; i < riscv_isa_vendor_ext_list_size; i++) {
  

•           const struct riscv_isa_vendor_ext_data_list *ext_list = riscv_isa_vendor_ext_list[i];
  

•           if (bitmap_empty(ext_list->vendor_bitmap, ext_list->bitmap_size))
  

•                   bitmap_copy(ext_list->vendor_bitmap,
  

•                               ext_list->per_hart_vendor_bitmap[cpu].isa,
  

•                               ext_list->bitmap_size);
  

Could you get into trouble here if the set of vendor extensions reduces to zero, and then becomes non-zero? To illustrate, consider these masks: cpu 0: 0x0000C000 cpu 1: 0x00000003 <<< vendor_bitmap ANDs out to 0 cpu 2: 0x00000010 <<< oops, we end up copying this into vendor_bitmap

Huh that's a good point. The standard extensions have that same bug too?

if (bitmap_empty(riscv_isa, RISCV_ISA_EXT_MAX)) bitmap_copy(riscv_isa, isainfo->isa, RISCV_ISA_EXT_MAX); else bitmap_and(riscv_isa, riscv_isa, isainfo->isa, RISCV_ISA_EXT_MAX);

...

•           else
  

•                   bitmap_and(ext_list->vendor_bitmap, ext_list->vendor_bitmap,
  

•                              ext_list->per_hart_vendor_bitmap[cpu].isa,
  

•                              ext_list->bitmap_size);
  

•   }
  

+}

static int __init riscv_fill_hwcap_from_ext_list(unsigned long *isa2hwcap) { unsigned int cpu; @@ -615,6 +648,8 @@ static int __init riscv_fill_hwcap_from_ext_list(unsigned long *isa2hwcap) } }

•           riscv_fill_cpu_vendor_ext(cpu_node, cpu);
  

•           of_node_put(cpu_node);
  
            /*
  

@@ -630,6 +665,8 @@ static int __init riscv_fill_hwcap_from_ext_list(unsigned long *isa2hwcap) bitmap_copy(riscv_isa, isainfo->isa, RISCV_ISA_EXT_MAX); else bitmap_and(riscv_isa, riscv_isa, isainfo->isa, RISCV_ISA_EXT_MAX);

•           riscv_fill_vendor_ext_list(cpu);
    }
  
    if (bitmap_empty(riscv_isa, RISCV_ISA_EXT_MAX))
  

diff --git a/arch/riscv/kernel/vendor_extensions.c b/arch/riscv/kernel/vendor_extensions.c new file mode 100644 index 000000000000..f76cb3013c2d --- /dev/null +++ b/arch/riscv/kernel/vendor_extensions.c @@ -0,0 +1,18 @@ +// SPDX-License-Identifier: GPL-2.0-only +/*

• • Copyright 2024 Rivos, Inc
• */

+#include <asm/vendor_extensions.h> +#include <asm/vendor_extensions/thead.h>

+#include <linux/array_size.h> +#include <linux/types.h>

+const struct riscv_isa_vendor_ext_data_list *riscv_isa_vendor_ext_list[] = { +#ifdef CONFIG_RISCV_ISA_VENDOR_EXT_THEAD

•   &riscv_isa_vendor_ext_list_thead,
  

+#endif +};

+const size_t riscv_isa_vendor_ext_list_size = ARRAY_SIZE(riscv_isa_vendor_ext_list); diff --git a/arch/riscv/kernel/vendor_extensions/Makefile b/arch/riscv/kernel/vendor_extensions/Makefile new file mode 100644 index 000000000000..3383066baaab --- /dev/null +++ b/arch/riscv/kernel/vendor_extensions/Makefile @@ -0,0 +1,3 @@ +# SPDX-License-Identifier: GPL-2.0-only

+obj-$(CONFIG_RISCV_ISA_VENDOR_EXT_THEAD) += thead.o diff --git a/arch/riscv/kernel/vendor_extensions/thead.c b/arch/riscv/kernel/vendor_extensions/thead.c new file mode 100644 index 000000000000..edb20b928c0c --- /dev/null +++ b/arch/riscv/kernel/vendor_extensions/thead.c @@ -0,0 +1,36 @@ +// SPDX-License-Identifier: GPL-2.0-only

+#include <asm/cpufeature.h> +#include <asm/vendor_extensions.h> +#include <asm/vendor_extensions/thead.h>

+#include <linux/array_size.h> +#include <linux/types.h>

+/* All T-Head vendor extensions supported in Linux */ +const struct riscv_isa_ext_data riscv_isa_vendor_ext_thead[] = {

•   __RISCV_ISA_EXT_DATA(xtheadvector, RISCV_ISA_VENDOR_EXT_XTHEADVECTOR),
  

+};

+/*

• • The first member of this struct must be a bitmap named isa so it can be
• • compatible with riscv_isainfo even though the sizes of the bitmaps may be
• • different.

This is kinda yucky, as you're casting a bitmap of a different size into a struct riscv_isainfo *, which has a known size. I don't necessarily have a fabulous suggestion to fix though. The best I can come up with is refactor struct riscv_isainfo to be: struct riscv_isainfo { int count; unsigned long isa[0]; };

then declare a standard one (for hart_isa, which is statically allocated): struct riscv_std_isainfo { int count; DECLARE_BITMAP(isa, RISCV_ISA_EXT_MAX); }

and a thead one struct riscv_thead_isainfo { int count; DECLARE_BITMAP(isa, RISCV_ISA_VENDOR_EXT_MAX_THEAD); }

But there's still a cast in there, as you'd cast the specialized structs to struct riscv_isainfo *. But at least the size is in there to be enforced at runtime, rather than a compile-time check that's wrong. So I'll just leave this half baked thought here, and maybe you can think of a cleaner way, or ignore it :).

Yes perhaps this is a better way of doing it.

- Charlie

...

• */

+struct riscv_isavendorinfo_thead {

•   DECLARE_BITMAP(isa, RISCV_ISA_VENDOR_EXT_MAX_THEAD);
  

+};

+/* Hart specific T-Head vendor extension support */ +static struct riscv_isavendorinfo_thead hart_vendorinfo_thead[NR_CPUS];

+/* Set of T-Head vendor extensions supported on all harts */ +DECLARE_BITMAP(vendorinfo_thead, RISCV_ISA_VENDOR_EXT_MAX_THEAD);

+const struct riscv_isa_vendor_ext_data_list riscv_isa_vendor_ext_list_thead = {

•   .ext_data = riscv_isa_vendor_ext_thead,
  

•   .per_hart_vendor_bitmap = (struct riscv_isainfo *)hart_vendorinfo_thead,
  

•   .vendor_bitmap = vendorinfo_thead,
  

•   .ext_data_count = ARRAY_SIZE(riscv_isa_vendor_ext_thead),
  

•   .bitmap_size = RISCV_ISA_VENDOR_EXT_MAX_THEAD
  

+};

-- 2.44.0

On Wed, May 1, 2024 at 10:51 AM Charlie Jenkins charlie@rivosinc.com wrote:

On Wed, May 01, 2024 at 09:44:15AM -0700, Evan Green wrote:

...

On Fri, Apr 26, 2024 at 2:29 PM Charlie Jenkins charlie@rivosinc.com wrote:

...

Separate vendor extensions out into one struct per vendor instead of adding vendor extensions onto riscv_isa_ext.

Add a hidden config RISCV_ISA_VENDOR_EXT to conditionally include this code.

The xtheadvector vendor extension is added using these changes.

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

arch/riscv/Kconfig | 2 + arch/riscv/Kconfig.vendor | 19 ++++++ arch/riscv/include/asm/cpufeature.h | 18 ++++++ arch/riscv/include/asm/vendor_extensions.h | 26 ++++++++ arch/riscv/include/asm/vendor_extensions/thead.h | 19 ++++++ arch/riscv/kernel/Makefile | 2 + arch/riscv/kernel/cpufeature.c | 77 ++++++++++++++++++------ arch/riscv/kernel/vendor_extensions.c | 18 ++++++ arch/riscv/kernel/vendor_extensions/Makefile | 3 + arch/riscv/kernel/vendor_extensions/thead.c | 36 +++++++++++ 10 files changed, 200 insertions(+), 20 deletions(-)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig index be09c8836d56..fec86fba3acd 100644 --- a/arch/riscv/Kconfig +++ b/arch/riscv/Kconfig @@ -759,6 +759,8 @@ config RISCV_EFFICIENT_UNALIGNED_ACCESS

endchoice

+source "arch/riscv/Kconfig.vendor"

endmenu # "Platform type"

menu "Kernel features" diff --git a/arch/riscv/Kconfig.vendor b/arch/riscv/Kconfig.vendor new file mode 100644 index 000000000000..4fc86810af1d --- /dev/null +++ b/arch/riscv/Kconfig.vendor @@ -0,0 +1,19 @@ +menu "Vendor extensions"

+config RISCV_ISA_VENDOR_EXT

•   bool
  

+menu "T-Head" +config RISCV_ISA_VENDOR_EXT_THEAD

•   bool "T-Head vendor extension support"
  

•   select RISCV_ISA_VENDOR_EXT
  

•   default y
  

•   help
  

•     Say N here if you want to disable all T-Head vendor extension
  

•     support. This will cause any T-Head vendor extensions that are
  

•     requested to be ignored.
  

•     If you don't know what to do here, say Y.
  

+endmenu

+endmenu diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h index 0c4f08577015..fedd479ccfd1 100644 --- a/arch/riscv/include/asm/cpufeature.h +++ b/arch/riscv/include/asm/cpufeature.h @@ -35,6 +35,24 @@ extern u32 riscv_vlenb_of;

void riscv_user_isa_enable(void);

+#define _RISCV_ISA_EXT_DATA(_name, _id, _subset_exts, _subset_exts_size) { \

•   .name = #_name,                                                         \
  

•   .property = #_name,                                                     \
  

•   .id = _id,                                                              \
  

•   .subset_ext_ids = _subset_exts,                                         \
  

•   .subset_ext_size = _subset_exts_size                                    \
  

+}

+#define __RISCV_ISA_EXT_DATA(_name, _id) _RISCV_ISA_EXT_DATA(_name, _id, NULL, 0)

+/* Used to declare pure "lasso" extension (Zk for instance) */ +#define __RISCV_ISA_EXT_BUNDLE(_name, _bundled_exts) \

•   _RISCV_ISA_EXT_DATA(_name, RISCV_ISA_EXT_INVALID, _bundled_exts, ARRAY_SIZE(_bundled_exts))
  

+/* Used to declare extensions that are a superset of other extensions (Zvbb for instance) */ +#define __RISCV_ISA_EXT_SUPERSET(_name, _id, _sub_exts) \

•   _RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts))
  

#if defined(CONFIG_RISCV_MISALIGNED) bool check_unaligned_access_emulated_all_cpus(void); void unaligned_emulation_finish(void); diff --git a/arch/riscv/include/asm/vendor_extensions.h b/arch/riscv/include/asm/vendor_extensions.h new file mode 100644 index 000000000000..0af1ddd0af70 --- /dev/null +++ b/arch/riscv/include/asm/vendor_extensions.h @@ -0,0 +1,26 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/*

• • Copyright 2024 Rivos, Inc
• */

+#ifndef _ASM_VENDOR_EXTENSIONS_H +#define _ASM_VENDOR_EXTENSIONS_H

+#include <asm/cpufeature.h>

+#include <linux/array_size.h> +#include <linux/types.h>

+struct riscv_isa_vendor_ext_data_list {

•   const struct riscv_isa_ext_data *ext_data;
  

•   struct riscv_isainfo *per_hart_vendor_bitmap;
  

•   unsigned long *vendor_bitmap;
  

It took a lot of digging for me to understand this was the set of vendor extensions supported on all harts. Can we add that to the name, maybe something like isa_bitmap_all_harts? (I wonder if we could drop the vendor part of the name since we already know we're in a vendor_ext_data_list structure).

Sure, I figured it was implied since the other bitmap says "per_hart", but I can see how it could be confusing.

...

...

•   const size_t ext_data_count;
  

•   const size_t bitmap_size;
  

+};

+extern const struct riscv_isa_vendor_ext_data_list *riscv_isa_vendor_ext_list[];

+extern const size_t riscv_isa_vendor_ext_list_size;

+#endif /* _ASM_VENDOR_EXTENSIONS_H */ diff --git a/arch/riscv/include/asm/vendor_extensions/thead.h b/arch/riscv/include/asm/vendor_extensions/thead.h new file mode 100644 index 000000000000..92eec729888d --- /dev/null +++ b/arch/riscv/include/asm/vendor_extensions/thead.h @@ -0,0 +1,19 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _ASM_RISCV_VENDOR_EXTENSIONS_THEAD_H +#define _ASM_RISCV_VENDOR_EXTENSIONS_THEAD_H

+#include <asm/vendor_extensions.h>

+#include <linux/types.h>

+#define RISCV_ISA_VENDOR_EXT_XTHEADVECTOR 0

+/*

• • Extension keys should be strictly less than max.
• • It is safe to increment this when necessary.
• */

+#define RISCV_ISA_VENDOR_EXT_MAX_THEAD 32

+extern const struct riscv_isa_vendor_ext_data_list riscv_isa_vendor_ext_list_thead;

+#endif diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile index 81d94a8ee10f..53361c50fb46 100644 --- a/arch/riscv/kernel/Makefile +++ b/arch/riscv/kernel/Makefile @@ -58,6 +58,8 @@ obj-y += riscv_ksyms.o obj-y += stacktrace.o obj-y += cacheinfo.o obj-y += patch.o +obj-y += vendor_extensions.o +obj-y += vendor_extensions/ obj-y += probes/ obj-y += tests/ obj-$(CONFIG_MMU) += vdso.o vdso/ diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c index 8158f34c3e36..c073494519eb 100644 --- a/arch/riscv/kernel/cpufeature.c +++ b/arch/riscv/kernel/cpufeature.c @@ -24,6 +24,7 @@ #include <asm/processor.h> #include <asm/sbi.h> #include <asm/vector.h> +#include <asm/vendor_extensions.h>

#define NUM_ALPHA_EXTS ('z' - 'a' + 1)

@@ -102,24 +103,6 @@ static bool riscv_isa_extension_check(int id) return true; }

-#define _RISCV_ISA_EXT_DATA(_name, _id, _subset_exts, _subset_exts_size) { \

•   .name = #_name,                                                         \
  

•   .property = #_name,                                                     \
  

•   .id = _id,                                                              \
  

•   .subset_ext_ids = _subset_exts,                                         \
  

•   .subset_ext_size = _subset_exts_size                                    \
  

-}

-#define __RISCV_ISA_EXT_DATA(_name, _id) _RISCV_ISA_EXT_DATA(_name, _id, NULL, 0)

-/* Used to declare pure "lasso" extension (Zk for instance) */ -#define __RISCV_ISA_EXT_BUNDLE(_name, _bundled_exts) \

•   _RISCV_ISA_EXT_DATA(_name, RISCV_ISA_EXT_INVALID, _bundled_exts, ARRAY_SIZE(_bundled_exts))
  

-/* Used to declare extensions that are a superset of other extensions (Zvbb for instance) */ -#define __RISCV_ISA_EXT_SUPERSET(_name, _id, _sub_exts) \

•   _RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts))
  

static const unsigned int riscv_zk_bundled_exts[] = { RISCV_ISA_EXT_ZBKB, RISCV_ISA_EXT_ZBKC, @@ -353,6 +336,10 @@ static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct risc bool ext_long = false, ext_err = false;

            switch (*ext) {

•           case 'x':
  

•           case 'X':
  

•                   pr_warn_once("Vendor extensions are ignored in riscv,isa. Use riscv,isa-extensions instead.");
  

•                   continue;
            case 's':
                    /*
                     * Workaround for invalid single-letter 's' & 'u' (QEMU).
  

@@ -368,8 +355,6 @@ static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct risc } fallthrough; case 'S':

•           case 'x':
  

•           case 'X':
            case 'z':
            case 'Z':
                    /*
  

@@ -572,6 +557,54 @@ static void __init riscv_fill_hwcap_from_isa_string(unsigned long *isa2hwcap) acpi_put_table((struct acpi_table_header *)rhct); }

+static void __init riscv_fill_cpu_vendor_ext(struct device_node *cpu_node, int cpu) +{

•   if (!IS_ENABLED(CONFIG_RISCV_ISA_VENDOR_EXT))
  

•           return;
  

•   for (int i = 0; i < riscv_isa_vendor_ext_list_size; i++) {
  

•           const struct riscv_isa_vendor_ext_data_list *ext_list = riscv_isa_vendor_ext_list[i];
  

•           for (int j = 0; j < ext_list->ext_data_count; j++) {
  

•                   const struct riscv_isa_ext_data ext = ext_list->ext_data[j];
  

•                   struct riscv_isainfo *isavendorinfo = &ext_list->per_hart_vendor_bitmap[cpu];
  

•                   if (of_property_match_string(cpu_node, "riscv,isa-extensions",
  

•                                                ext.property) < 0)
  

•                           continue;
  

•                   /*
  

•                    * Assume that subset extensions are all members of the
  

•                    * same vendor.
  

•                    */
  

•                   if (ext.subset_ext_size)
  

•                           for (int k = 0; k < ext.subset_ext_size; k++)
  

•                                   set_bit(ext.subset_ext_ids[k], isavendorinfo->isa);
  

•                   set_bit(ext.id, isavendorinfo->isa);
  

•           }
  

This loop seems super similar to the regular one (in riscv_fill_hwcap_from_ext_list() in the random, possibly old, kernel I have open). Could we refactor these together into a common helper? The other loop has an extra stanza for riscv_isa_extension_check(), so we'd have to add an extra condition there, but otherwise it looks pretty compatible?

I actually did have this refactored into a single function in a previous version but broke it back up since I felt there just wasn't enough overlap. The one for standard extensions is:

    for (int i = 0; i < riscv_isa_ext_count; i++) {
            const struct riscv_isa_ext_data *ext = &riscv_isa_ext[i];

            if (of_property_match_string(cpu_node, "riscv,isa-extensions",
                                         ext->property) < 0)
                    continue;

            if (ext->subset_ext_size) {
                    for (int j = 0; j < ext->subset_ext_size; j++) {
                            if (riscv_isa_extension_check(ext->subset_ext_ids[i]))
                                    set_bit(ext->subset_ext_ids[j], isainfo->isa);
                    }
            }

            if (riscv_isa_extension_check(ext->id)) {
                    set_bit(ext->id, isainfo->isa);

                    /* Only single letter extensions get set in hwcap */
                    if (strnlen(riscv_isa_ext[i].name, 2) == 1)
                            this_hwcap |= isa2hwcap[riscv_isa_ext[i].id];
            }
    }

The motivating reason why I didn't combine them was the additional `struct riscv_isa_vendor_ext_data_list *` data type for the vendor version which contains ext and isainfo. This can probably be combined in a straight-forward way though.

I see what you mean. There might be a way to reconfigure the structs to make this work better, but yeah, those slight differences make it hard to extract a common bit.

...

...

•   }
  

+}

+static void __init riscv_fill_vendor_ext_list(int cpu) +{

•   if (!IS_ENABLED(CONFIG_RISCV_ISA_VENDOR_EXT))
  

•           return;
  

•   for (int i = 0; i < riscv_isa_vendor_ext_list_size; i++) {
  

•           const struct riscv_isa_vendor_ext_data_list *ext_list = riscv_isa_vendor_ext_list[i];
  

•           if (bitmap_empty(ext_list->vendor_bitmap, ext_list->bitmap_size))
  

•                   bitmap_copy(ext_list->vendor_bitmap,
  

•                               ext_list->per_hart_vendor_bitmap[cpu].isa,
  

•                               ext_list->bitmap_size);
  

Could you get into trouble here if the set of vendor extensions reduces to zero, and then becomes non-zero? To illustrate, consider these masks: cpu 0: 0x0000C000 cpu 1: 0x00000003 <<< vendor_bitmap ANDs out to 0 cpu 2: 0x00000010 <<< oops, we end up copying this into vendor_bitmap

Huh that's a good point. The standard extensions have that same bug too?

    if (bitmap_empty(riscv_isa, RISCV_ISA_EXT_MAX))
            bitmap_copy(riscv_isa, isainfo->isa, RISCV_ISA_EXT_MAX);
    else
            bitmap_and(riscv_isa, riscv_isa, isainfo->isa, RISCV_ISA_EXT_MAX);

Ah crap you're right. What clown introduced that code? Oh, me. I'm not aware of anything heterogenous yet, so hopefully we can just quietly fix it.

...

...

•           else
  

•                   bitmap_and(ext_list->vendor_bitmap, ext_list->vendor_bitmap,
  

•                              ext_list->per_hart_vendor_bitmap[cpu].isa,
  

•                              ext_list->bitmap_size);
  

•   }
  

+}

static int __init riscv_fill_hwcap_from_ext_list(unsigned long *isa2hwcap) { unsigned int cpu; @@ -615,6 +648,8 @@ static int __init riscv_fill_hwcap_from_ext_list(unsigned long *isa2hwcap) } }

•           riscv_fill_cpu_vendor_ext(cpu_node, cpu);
  

•           of_node_put(cpu_node);
  
            /*
  

@@ -630,6 +665,8 @@ static int __init riscv_fill_hwcap_from_ext_list(unsigned long *isa2hwcap) bitmap_copy(riscv_isa, isainfo->isa, RISCV_ISA_EXT_MAX); else bitmap_and(riscv_isa, riscv_isa, isainfo->isa, RISCV_ISA_EXT_MAX);

•           riscv_fill_vendor_ext_list(cpu);
    }
  
    if (bitmap_empty(riscv_isa, RISCV_ISA_EXT_MAX))
  

diff --git a/arch/riscv/kernel/vendor_extensions.c b/arch/riscv/kernel/vendor_extensions.c new file mode 100644 index 000000000000..f76cb3013c2d --- /dev/null +++ b/arch/riscv/kernel/vendor_extensions.c @@ -0,0 +1,18 @@ +// SPDX-License-Identifier: GPL-2.0-only +/*

• • Copyright 2024 Rivos, Inc
• */

+#include <asm/vendor_extensions.h> +#include <asm/vendor_extensions/thead.h>

+#include <linux/array_size.h> +#include <linux/types.h>

+const struct riscv_isa_vendor_ext_data_list *riscv_isa_vendor_ext_list[] = { +#ifdef CONFIG_RISCV_ISA_VENDOR_EXT_THEAD

•   &riscv_isa_vendor_ext_list_thead,
  

+#endif +};

+const size_t riscv_isa_vendor_ext_list_size = ARRAY_SIZE(riscv_isa_vendor_ext_list); diff --git a/arch/riscv/kernel/vendor_extensions/Makefile b/arch/riscv/kernel/vendor_extensions/Makefile new file mode 100644 index 000000000000..3383066baaab --- /dev/null +++ b/arch/riscv/kernel/vendor_extensions/Makefile @@ -0,0 +1,3 @@ +# SPDX-License-Identifier: GPL-2.0-only

+obj-$(CONFIG_RISCV_ISA_VENDOR_EXT_THEAD) += thead.o diff --git a/arch/riscv/kernel/vendor_extensions/thead.c b/arch/riscv/kernel/vendor_extensions/thead.c new file mode 100644 index 000000000000..edb20b928c0c --- /dev/null +++ b/arch/riscv/kernel/vendor_extensions/thead.c @@ -0,0 +1,36 @@ +// SPDX-License-Identifier: GPL-2.0-only

+#include <asm/cpufeature.h> +#include <asm/vendor_extensions.h> +#include <asm/vendor_extensions/thead.h>

+#include <linux/array_size.h> +#include <linux/types.h>

+/* All T-Head vendor extensions supported in Linux */ +const struct riscv_isa_ext_data riscv_isa_vendor_ext_thead[] = {

•   __RISCV_ISA_EXT_DATA(xtheadvector, RISCV_ISA_VENDOR_EXT_XTHEADVECTOR),
  

+};

+/*

• • The first member of this struct must be a bitmap named isa so it can be
• • compatible with riscv_isainfo even though the sizes of the bitmaps may be
• • different.

This is kinda yucky, as you're casting a bitmap of a different size into a struct riscv_isainfo *, which has a known size. I don't necessarily have a fabulous suggestion to fix though. The best I can come up with is refactor struct riscv_isainfo to be: struct riscv_isainfo { int count; unsigned long isa[0]; };

then declare a standard one (for hart_isa, which is statically allocated): struct riscv_std_isainfo { int count; DECLARE_BITMAP(isa, RISCV_ISA_EXT_MAX); }

and a thead one struct riscv_thead_isainfo { int count; DECLARE_BITMAP(isa, RISCV_ISA_VENDOR_EXT_MAX_THEAD); }

But there's still a cast in there, as you'd cast the specialized structs to struct riscv_isainfo *. But at least the size is in there to be enforced at runtime, rather than a compile-time check that's wrong. So I'll just leave this half baked thought here, and maybe you can think of a cleaner way, or ignore it :).

Yes perhaps this is a better way of doing it.

• Charlie

...

...

• */

+struct riscv_isavendorinfo_thead {

•   DECLARE_BITMAP(isa, RISCV_ISA_VENDOR_EXT_MAX_THEAD);
  

+};

+/* Hart specific T-Head vendor extension support */ +static struct riscv_isavendorinfo_thead hart_vendorinfo_thead[NR_CPUS];

+/* Set of T-Head vendor extensions supported on all harts */ +DECLARE_BITMAP(vendorinfo_thead, RISCV_ISA_VENDOR_EXT_MAX_THEAD);

+const struct riscv_isa_vendor_ext_data_list riscv_isa_vendor_ext_list_thead = {

•   .ext_data = riscv_isa_vendor_ext_thead,
  

•   .per_hart_vendor_bitmap = (struct riscv_isainfo *)hart_vendorinfo_thead,
  

•   .vendor_bitmap = vendorinfo_thead,
  

•   .ext_data_count = ARRAY_SIZE(riscv_isa_vendor_ext_thead),
  

•   .bitmap_size = RISCV_ISA_VENDOR_EXT_MAX_THEAD
  

+};

-- 2.44.0

On Wed, May 01, 2024 at 12:29:56PM +0100, Conor Dooley wrote:

On Fri, Apr 26, 2024 at 02:29:20PM -0700, Charlie Jenkins wrote:

...

index c073494519eb..dd7e8e0c0af1 100644 --- a/arch/riscv/kernel/cpufeature.c +++ b/arch/riscv/kernel/cpufeature.c @@ -844,25 +844,41 @@ 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) {
  

• /*
  

•  * Any alternative with a patch_id that is less than
  

•  * RISCV_ISA_EXT_MAX is interpreted as a standard extension.
  

•  *
  

•  * Any alternative with patch_id that is greater than or equal
  

•  * to RISCV_VENDOR_EXT_ALTERNATIVES_BASE is interpreted as a
  

•  * vendor extension.
  

I think this stuff is all fine, since we can always re-jig things in the future if needs be.

...

•  */
  

• if (id < RISCV_ISA_EXT_MAX) {
  

• 	/*
  

• 	 * This patch should be treated as errata so skip
  

• 	 * processing here.
  

• 	 */
  

• 	if (alt->vendor_id != 0)
  

• 		continue;
  

• 	if (!__riscv_isa_extension_available(NULL, id))
  

• 		continue;
  

• } else if (id >= RISCV_VENDOR_EXT_ALTERNATIVES_BASE) {
  

• 	if (!__riscv_isa_vendor_extension_available(VENDOR_EXT_ALL_CPUS, 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);

I wonder if we care to implement a non __ prefixed version of this, like the standard stuff? The only __ version users of the standard one are in kvm and core arch code, the "external" users all use the non-prefixed version.

In vendor_extensions.h there is:

#define riscv_isa_vendor_extension_available(vendor, ext) \ __riscv_isa_vendor_extension_available(VENDOR_EXT_ALL_CPUS, vendor, \ RISCV_ISA_VENDOR_EXT_##ext)

In any case, Reviewed-by: Conor Dooley conor.dooley@microchip.com

Thanks!

- Charlie

Cheers, Conor.

On Fri, Apr 26, 2024 at 02:29:21PM -0700, Charlie Jenkins wrote:

The __riscv_has_extension_likely() and __riscv_has_extension_unlikely() functions from the vendor_extensions.h can be used to simplify the standard extension checking code as well. Migrate those functions to cpufeature.h and reorganize the code in the file to use the functions.

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

arch/riscv/include/asm/cpufeature.h | 78 +++++++++++++++++------------- arch/riscv/include/asm/vendor_extensions.h | 28 ----------- 2 files changed, 44 insertions(+), 62 deletions(-)

diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h index fedd479ccfd1..17896ec9ec11 100644 --- a/arch/riscv/include/asm/cpufeature.h +++ b/arch/riscv/include/asm/cpufeature.h @@ -98,59 +98,66 @@ extern bool riscv_isa_fallback; unsigned long riscv_isa_extension_base(const unsigned long *isa_bitmap); +#define EXT_ALL_VENDORS 0

It's not really "all vendors", it's standard. Otherwise, this seems all grand to me, Reviewed-by: Conor Dooley conor.dooley@microchip.com

Cheers, Conor.

On Wed, May 01, 2024 at 12:48:13PM -0700, Charlie Jenkins wrote:

On Wed, May 01, 2024 at 12:37:14PM +0100, Conor Dooley wrote:

...

On Fri, Apr 26, 2024 at 02:29:21PM -0700, Charlie Jenkins wrote:

...

The __riscv_has_extension_likely() and __riscv_has_extension_unlikely() functions from the vendor_extensions.h can be used to simplify the standard extension checking code as well. Migrate those functions to cpufeature.h and reorganize the code in the file to use the functions.

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

arch/riscv/include/asm/cpufeature.h | 78 +++++++++++++++++------------- arch/riscv/include/asm/vendor_extensions.h | 28 ----------- 2 files changed, 44 insertions(+), 62 deletions(-)

diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h index fedd479ccfd1..17896ec9ec11 100644 --- a/arch/riscv/include/asm/cpufeature.h +++ b/arch/riscv/include/asm/cpufeature.h @@ -98,59 +98,66 @@ extern bool riscv_isa_fallback; unsigned long riscv_isa_extension_base(const unsigned long *isa_bitmap); +#define EXT_ALL_VENDORS 0

It's not really "all vendors", it's standard. Otherwise, this seems all

This hooks up into the alternatives:

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

Yeah, I know what you're using it for, I just find the naming odd.

Where the "vendor" argument is supposed to be 0 if the alternative is applicable to all vendors. Is there a better way to convey this?

s/EXT_ALL_VENDORS/STANDARD_EXT/?