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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> > > --- > > 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 > >