Re: [PATCH v4 05/16] riscv: Extend cpufeature.c to detect vendor extensions

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