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

On Fri, May 03, 2024 at 09:28:24AM -0700, Evan Green wrote:

On Thu, May 2, 2024 at 9:46 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 | 34 +++++++++ arch/riscv/include/asm/vendor_extensions/thead.h | 16 ++++ arch/riscv/kernel/Makefile | 2 + arch/riscv/kernel/cpufeature.c | 93 +++++++++++++++++++----- arch/riscv/kernel/vendor_extensions.c | 18 +++++ arch/riscv/kernel/vendor_extensions/Makefile | 3 + arch/riscv/kernel/vendor_extensions/thead.c | 18 +++++ 10 files changed, 203 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..85ac30496b0e --- /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 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.
  

•     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..bf4dac66e6e6 --- /dev/null +++ b/arch/riscv/include/asm/vendor_extensions.h @@ -0,0 +1,34 @@ +/* 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>

+/*

• • The extension keys of each vendor must be strictly less than this value.
• */

+#define RISCV_ISA_VENDOR_EXT_MAX 32

+struct riscv_isavendorinfo {

•   DECLARE_BITMAP(isa, RISCV_ISA_VENDOR_EXT_MAX);
  

+};

Nice, I think this was a good compromise: being honest with the compiler about the fixed array sizes, with the tradeoff that all vendors have to use the same ceiling for the number of bits. If one vendor raises this ceiling absurdly and starts creating huge amounts of waste we can revisit.

...

+struct riscv_isa_vendor_ext_data_list {

•   const size_t ext_data_count;
  

•   const struct riscv_isa_ext_data *ext_data;
  

•   struct riscv_isavendorinfo per_hart_isa_bitmap[NR_CPUS];
  

•   struct riscv_isavendorinfo all_harts_isa_bitmap;
  

+};

+extern 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..48421d1553ad --- /dev/null +++ b/arch/riscv/include/asm/vendor_extensions/thead.h @@ -0,0 +1,16 @@ +/* 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>

+/*

• • Extension keys must be strictly less than RISCV_ISA_VENDOR_EXT_MAX.
• */

+#define RISCV_ISA_VENDOR_EXT_XTHEADVECTOR 0

+extern 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 12c79db0b0bb..cc9ec393c8f6 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,21 @@ 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':
  

•                   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).
  

@@ -368,8 +366,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 +568,59 @@ 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++) {
  

•           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_isavendorinfo *isavendorinfo = &ext_list->per_hart_isa_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);
  

•           }
  

•   }
  

+}

+static void __init riscv_fill_vendor_ext_list(int cpu) +{

•   bool first = true;
  

•   if (!IS_ENABLED(CONFIG_RISCV_ISA_VENDOR_EXT))
  

•           return;
  

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

•           struct riscv_isa_vendor_ext_data_list *ext_list = riscv_isa_vendor_ext_list[i];
  

•           if (first) {
  

•                   bitmap_copy(ext_list->all_harts_isa_bitmap.isa,
  

•                               ext_list->per_hart_isa_bitmap[cpu].isa,
  

•                               RISCV_ISA_VENDOR_EXT_MAX);
  

•                   first = false;
  

I think this is still not quite right. This behaves properly when called on the first cpu (let's say 0), but then we call it again with cpu 1, first gets set to true, and we clobber the old work we did for cpu 0. If we knew that cpu 0 was always called first (this looks true since both calls are within a for_each_possible_cpu() loop), and that this was only called once at boot for cpu 0 (looks true, and riscv_fill_hwcap() is __init), then it could be bool first = cpu == 0.

Assuming that the first cpu is 0 should be safe, but to eliminate that assumption we can pass in a "first" parameter into this function that will only be true the first time this function is called. We also keep "first = false" in this function so the copy only happens on the first iteration of the first cpu.

- Charlie