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