Message-ID: <CALs-HstM64Hy_=XVz=0sWQt=8j1u+bq6RhthUuD3P0E4=HyvcA@mail.gmail.com>
Date: Wed, 1 May 2024 09:44:15 -0700
From: Evan Green <evan@...osinc.com>
To: Charlie Jenkins <charlie@...osinc.com>
Cc: Conor Dooley <conor@...nel.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, 
	Paul Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt <palmer@...belt.com>, 
	Albert Ou <aou@...s.berkeley.edu>, Guo Ren <guoren@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>, Chen-Yu Tsai <wens@...e.org>, 
	Jernej Skrabec <jernej.skrabec@...il.com>, Samuel Holland <samuel@...lland.org>, 
	Conor Dooley <conor.dooley@...rochip.com>, Clément Léger <cleger@...osinc.com>, 
	Jonathan Corbet <corbet@....net>, Shuah Khan <shuah@...nel.org>, linux-riscv@...ts.infradead.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Palmer Dabbelt <palmer@...osinc.com>, linux-arm-kernel@...ts.infradead.org, 
	linux-sunxi@...ts.linux.dev, linux-doc@...r.kernel.org, 
	linux-kselftest@...r.kernel.org
Subject: Re: [PATCH v4 05/16] riscv: Extend cpufeature.c to detect vendor extensions

On Fri, Apr 26, 2024 at 2:29 PM Charlie Jenkins <charlie@...osinc.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@...osinc.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).

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

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

> +               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 :).


> + */
> +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
>

Powered by blists - more mailing lists