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

[Charlie Jenkins <charlie@rivosinc.com>]

On Fri, May 03, 2024 at 10:13:33AM -0700, Evan Green wrote:
> On Fri, May 3, 2024 at 10:08 AM Charlie Jenkins <charlie@rivosinc.com> wrote:
> >
> > 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.
> 
> Yeah, though then you still have to maintain that the function passing
> true as the first parameter really is the first invocation.
> 
> static bool first = true would also fix it, maybe more reliably. Are
> static locals allowed in the kernel?

That's even better! I did a search and saw many uses of static locals,
so let's go with that solution. I also tested it locally with a DT with
two CPUs that have two different vendor extensions from the same vendor
and verified that the all_hart_isa_bitmap did not contain either of the
vendor extensions.  I don't think it's possible to have an automated
test case like that in the kernel.

- Charlie

> 
> >
> > - Charlie
> >