[v5,00/17] riscv: Support vendor extensions and xtheadvector

This patch series ended up much larger than expected, please bear with
me! The goal here is to support vendor extensions, starting at probing
the device tree and ending with reporting to userspace.

The main design objective was to allow vendors to operate independently
of each other. This has been achieved by delegating vendor extensions to
a their own files and then accumulating the extensions in
arch/riscv/kernel/vendor_extensions.c.

Each vendor will have their own list of extensions they support.

There is a new hwprobe key RISCV_HWPROBE_KEY_VENDOR_EXT_THEAD_0 that is
used to request which thead vendor extensions are supported on the
current platform. This allows future vendors to allocate hwprobe keys
for their vendor.

On to the xtheadvector specific code. xtheadvector is a custom extension
that is based upon riscv vector version 0.7.1 [1]. All of the vector
routines have been modified to support this alternative vector version
based upon whether xtheadvector was determined to be supported at boot.
I have tested this with an Allwinner Nezha board. I ran into issues
booting the board on 6.9-rc1 so I applied these patches to 6.8. There
are a couple of minor merge conflicts that do arrise when doing that, so
please let me know if you have been able to boot this board with a 6.9
kernel. I used SkiffOS [2] to manage building the image, but upgraded
the U-Boot version to Samuel Holland's more up-to-date version [3] and
changed out the device tree used by U-Boot with the device trees that
are present in upstream linux and this series. Thank you Samuel for all
of the work you did to make this task possible.

To test the integration, I used the riscv vector kselftests. I modified
the test cases to be able to more easily extend them, and then added a
xtheadvector target that works by calling hwprobe and swapping out the
vector asm if needed.

[1] https://github.com/T-head-Semi/thead-extension-spec/blob/95358cb2cca9489361c61d335e03d3134b14133f/xtheadvector.adoc
[2] https://github.com/skiffos/SkiffOS/tree/master/configs/allwinner/nezha
[3] https://github.com/smaeul/u-boot/commit/2e89b706f5c956a70c989cd31665f1429e9a0b48

Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
---
Changes in v5:
- Make all vendors have the same size bitmap
- Extract vendor hwprobe code into helper macro
- Fix bug related to the handling of vendor extensions in the parsing of
  the isa string (Conor)
- Fix bug with the vendor bitmap being incorrectly populated (Evan)
- Add vendor extensions to /proc/cpuinfo
- Link to v4: https://lore.kernel.org/r/20240426-dev-charlie-support_thead_vector_6_9-v4-0-5cf53b5bc492@rivosinc.com

Changes in v4:
- Disable vector immediately if vlenb from the device tree is not
  homogeneous
- Hide vendor extension code behind a hidden config that vendor
  extensions select to eliminate the code when kernel is compiled
  without vendor extensions
- Clear up naming conventions and introduce some defines to make the
  vendor extension code clearer
- Link to v3: https://lore.kernel.org/r/20240420-dev-charlie-support_thead_vector_6_9-v3-0-67cff4271d1d@rivosinc.com

Changes in v3:
- Allow any hardware to support any vendor extension, rather than
  restricting the vendor extensions to the same vendor as the hardware
- Introduce config options to enable/disable a vendor's extensions
- Link to v2: https://lore.kernel.org/r/20240415-dev-charlie-support_thead_vector_6_9-v2-0-c7d68c603268@rivosinc.com

Changes in v2:
- Added commit hash to xtheadvector
- Simplified riscv,isa vector removal fix to not mess with the DT
  riscv,vendorid
- Moved riscv,vendorid parsing into a different patch and cache the
  value to be used by alternative patching
- Reduce riscv,vendorid missing severity to "info"
- Separate vendor extension list to vendor files
- xtheadvector no longer puts v in the elf_hwcap
- Only patch vendor extension if all harts are associated with the same
  vendor. This is the best chance the kernel has for working properly if
  there are multiple vendors.
- Split hwprobe vendor keys out into vendor file
- Add attribution for Heiko's patches
- Link to v1: https://lore.kernel.org/r/20240411-dev-charlie-support_thead_vector_6_9-v1-0-4af9815ec746@rivosinc.com

---
Charlie Jenkins (15):
      dt-bindings: riscv: Add xtheadvector ISA extension description
      riscv: vector: Use vlenb from DT
      riscv: dts: allwinner: Add xtheadvector to the D1/D1s devicetree
      riscv: Extend cpufeature.c to detect vendor extensions
      riscv: Add vendor extensions to /proc/cpuinfo
      riscv: Introduce vendor variants of extension helpers
      riscv: cpufeature: Extract common elements from extension checking
      riscv: Convert xandespmu to use the vendor extension framework
      riscv: csr: Add CSR encodings for VCSR_VXRM/VCSR_VXSAT
      riscv: Add xtheadvector instruction definitions
      riscv: vector: Support xtheadvector save/restore
      riscv: hwprobe: Add thead vendor extension probing
      riscv: hwprobe: Document thead vendor extensions and xtheadvector extension
      selftests: riscv: Fix vector tests
      selftests: riscv: Support xtheadvector in vector tests

Conor Dooley (1):
      dt-bindings: riscv: cpus: add a vlen register length property

Heiko Stuebner (1):
      RISC-V: define the elements of the VCSR vector CSR

 Documentation/arch/riscv/hwprobe.rst               |  10 +
 Documentation/devicetree/bindings/riscv/cpus.yaml  |   6 +
 .../devicetree/bindings/riscv/extensions.yaml      |  10 +
 arch/riscv/Kconfig                                 |   2 +
 arch/riscv/Kconfig.vendor                          |  44 +++
 arch/riscv/boot/dts/allwinner/sun20i-d1s.dtsi      |   3 +-
 arch/riscv/errata/andes/errata.c                   |   2 +
 arch/riscv/errata/sifive/errata.c                  |   3 +
 arch/riscv/errata/thead/errata.c                   |   3 +
 arch/riscv/include/asm/cpufeature.h                |  98 ++++---
 arch/riscv/include/asm/csr.h                       |  13 +
 arch/riscv/include/asm/hwcap.h                     |   1 -
 arch/riscv/include/asm/hwprobe.h                   |   4 +-
 arch/riscv/include/asm/switch_to.h                 |   2 +-
 arch/riscv/include/asm/vector.h                    | 247 +++++++++++++----
 arch/riscv/include/asm/vendor_extensions.h         | 103 +++++++
 arch/riscv/include/asm/vendor_extensions/andes.h   |  19 ++
 arch/riscv/include/asm/vendor_extensions/thead.h   |  42 +++
 .../include/asm/vendor_extensions/thead_hwprobe.h  |  18 ++
 .../include/asm/vendor_extensions/vendor_hwprobe.h |  34 +++
 arch/riscv/include/uapi/asm/hwprobe.h              |   3 +-
 arch/riscv/include/uapi/asm/vendor/thead.h         |   3 +
 arch/riscv/kernel/Makefile                         |   2 +
 arch/riscv/kernel/cpu.c                            |  35 ++-
 arch/riscv/kernel/cpufeature.c                     | 171 +++++++++---
 arch/riscv/kernel/kernel_mode_vector.c             |   8 +-
 arch/riscv/kernel/process.c                        |   4 +-
 arch/riscv/kernel/signal.c                         |   6 +-
 arch/riscv/kernel/sys_hwprobe.c                    |   5 +
 arch/riscv/kernel/vector.c                         |  25 +-
 arch/riscv/kernel/vendor_extensions.c              |  66 +++++
 arch/riscv/kernel/vendor_extensions/Makefile       |   5 +
 arch/riscv/kernel/vendor_extensions/andes.c        |  18 ++
 arch/riscv/kernel/vendor_extensions/thead.c        |  18 ++
 .../riscv/kernel/vendor_extensions/thead_hwprobe.c |  19 ++
 drivers/perf/riscv_pmu_sbi.c                       |   9 +-
 tools/testing/selftests/riscv/vector/.gitignore    |   3 +-
 tools/testing/selftests/riscv/vector/Makefile      |  17 +-
 .../selftests/riscv/vector/v_exec_initval_nolibc.c |  93 +++++++
 tools/testing/selftests/riscv/vector/v_helpers.c   |  67 +++++
 tools/testing/selftests/riscv/vector/v_helpers.h   |   7 +
 tools/testing/selftests/riscv/vector/v_initval.c   |  22 ++
 .../selftests/riscv/vector/v_initval_nolibc.c      |  68 -----
 .../selftests/riscv/vector/vstate_exec_nolibc.c    |  20 +-
 .../testing/selftests/riscv/vector/vstate_prctl.c  | 295 ++++++++++++---------
 45 files changed, 1312 insertions(+), 341 deletions(-)
---
base-commit: 4cece764965020c22cff7665b18a012006359095
change-id: 20240411-dev-charlie-support_thead_vector_6_9-1591fc2a431d

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.

On Thu, May 2, 2024 at 9:47 PM Charlie Jenkins <charlie@rivosinc.com> wrote:
>
> Add a new hwprobe key "RISCV_HWPROBE_KEY_VENDOR_EXT_THEAD_0" which
> allows userspace to probe for the new RISCV_ISA_VENDOR_EXT_XTHEADVECTOR
> vendor extension.
>
> This new key will allow userspace code to probe for which thead vendor
> extensions are supported. This API is modeled to be consistent with
> RISCV_HWPROBE_KEY_IMA_EXT_0. The bitmask returned will have each bit
> corresponding to a supported thead vendor extension of the cpumask set.
> Just like RISCV_HWPROBE_KEY_IMA_EXT_0, this allows a userspace program
> to determine all of the supported thead vendor extensions in one call.
>
> Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>

Reviewed-by: Evan Green <evan@rivosinc.com>

> ---
>  arch/riscv/include/asm/hwprobe.h                   |  4 +--
>  .../include/asm/vendor_extensions/thead_hwprobe.h  | 18 ++++++++++++
>  .../include/asm/vendor_extensions/vendor_hwprobe.h | 34 ++++++++++++++++++++++
>  arch/riscv/include/uapi/asm/hwprobe.h              |  3 +-
>  arch/riscv/include/uapi/asm/vendor/thead.h         |  3 ++
>  arch/riscv/kernel/sys_hwprobe.c                    |  5 ++++
>  arch/riscv/kernel/vendor_extensions/Makefile       |  1 +
>  .../riscv/kernel/vendor_extensions/thead_hwprobe.c | 19 ++++++++++++
>  8 files changed, 84 insertions(+), 3 deletions(-)
>
> diff --git a/arch/riscv/include/asm/hwprobe.h b/arch/riscv/include/asm/hwprobe.h
> index 630507dff5ea..e68496b4f8de 100644
> --- a/arch/riscv/include/asm/hwprobe.h
> +++ b/arch/riscv/include/asm/hwprobe.h
> @@ -1,6 +1,6 @@
>  /* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>  /*
> - * Copyright 2023 Rivos, Inc
> + * Copyright 2023-2024 Rivos, Inc
>   */
>
>  #ifndef _ASM_HWPROBE_H
> @@ -8,7 +8,7 @@
>
>  #include <uapi/asm/hwprobe.h>
>
> -#define RISCV_HWPROBE_MAX_KEY 6
> +#define RISCV_HWPROBE_MAX_KEY 7
>
>  static inline bool riscv_hwprobe_key_is_valid(__s64 key)
>  {
> diff --git a/arch/riscv/include/asm/vendor_extensions/thead_hwprobe.h b/arch/riscv/include/asm/vendor_extensions/thead_hwprobe.h
> new file mode 100644
> index 000000000000..925fef39a2c0
> --- /dev/null
> +++ b/arch/riscv/include/asm/vendor_extensions/thead_hwprobe.h
> @@ -0,0 +1,18 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_RISCV_VENDOR_EXTENSIONS_THEAD_HWPROBE_H
> +#define _ASM_RISCV_VENDOR_EXTENSIONS_THEAD_HWPROBE_H
> +
> +#include <linux/cpumask.h>
> +
> +#include <uapi/asm/hwprobe.h>
> +
> +#ifdef CONFIG_RISCV_ISA_VENDOR_EXT_THEAD
> +void hwprobe_isa_vendor_ext_thead_0(struct riscv_hwprobe *pair, const struct cpumask *cpus);
> +#else
> +static inline void hwprobe_isa_vendor_ext_thead_0(struct riscv_hwprobe *pair, const struct cpumask *cpus)
> +{
> +       pair->value = 0;
> +}
> +#endif
> +
> +#endif
> diff --git a/arch/riscv/include/asm/vendor_extensions/vendor_hwprobe.h b/arch/riscv/include/asm/vendor_extensions/vendor_hwprobe.h
> new file mode 100644
> index 000000000000..2a29f1a5cae3
> --- /dev/null
> +++ b/arch/riscv/include/asm/vendor_extensions/vendor_hwprobe.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright 2024 Rivos, Inc
> + */
> +
> +#ifndef _ASM_RISCV_SYS_HWPROBE_H
> +#define _ASM_RISCV_SYS_HWPROBE_H
> +
> +#include <asm/cpufeature.h>
> +
> +#define EXT_KEY(ext)                                                                   \
> +       do {                                                                            \
> +               if (__riscv_isa_extension_available(isainfo->isa, RISCV_ISA_VENDOR_EXT_##ext)) \
> +                       pair->value |= RISCV_HWPROBE_VENDOR_EXT_##ext;                  \
> +               else                                                                    \
> +                       missing |= RISCV_HWPROBE_VENDOR_EXT_##ext;                      \
> +       } while (false)
> +
> +/*
> + * Loop through and record extensions that 1) anyone has, and 2) anyone
> + * doesn't have.
> + */
> +#define VENDOR_EXTENSION_SUPPORTED(pair, cpus, per_hart_thead_bitmap, ...)                     \
> +       do {                                                                                    \
> +               int cpu;                                                                        \
> +               u64 missing;                                                                    \
> +               for_each_cpu(cpu, (cpus)) {                                                     \
> +                       struct riscv_isavendorinfo *isainfo = &(per_hart_thead_bitmap)[cpu];    \
> +                       __VA_ARGS__                                                             \
> +               }                                                                               \
> +               (pair)->value &= ~missing;                                                      \
> +       } while (false)                                                                         \
> +
> +#endif /* _ASM_RISCV_SYS_HWPROBE_H */
> diff --git a/arch/riscv/include/uapi/asm/hwprobe.h b/arch/riscv/include/uapi/asm/hwprobe.h
> index 9f2a8e3ff204..21e96a63f9ea 100644
> --- a/arch/riscv/include/uapi/asm/hwprobe.h
> +++ b/arch/riscv/include/uapi/asm/hwprobe.h
> @@ -1,6 +1,6 @@
>  /* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>  /*
> - * Copyright 2023 Rivos, Inc
> + * Copyright 2023-2024 Rivos, Inc
>   */
>
>  #ifndef _UAPI_ASM_HWPROBE_H
> @@ -67,6 +67,7 @@ struct riscv_hwprobe {
>  #define                RISCV_HWPROBE_MISALIGNED_UNSUPPORTED    (4 << 0)
>  #define                RISCV_HWPROBE_MISALIGNED_MASK           (7 << 0)
>  #define RISCV_HWPROBE_KEY_ZICBOZ_BLOCK_SIZE    6
> +#define RISCV_HWPROBE_KEY_VENDOR_EXT_THEAD_0   7
>  /* Increase RISCV_HWPROBE_MAX_KEY when adding items. */
>
>  /* Flags */
> diff --git a/arch/riscv/include/uapi/asm/vendor/thead.h b/arch/riscv/include/uapi/asm/vendor/thead.h
> new file mode 100644
> index 000000000000..43790ebe5faf
> --- /dev/null
> +++ b/arch/riscv/include/uapi/asm/vendor/thead.h
> @@ -0,0 +1,3 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +
> +#define                RISCV_HWPROBE_VENDOR_EXT_XTHEADVECTOR   (1 << 0)
> diff --git a/arch/riscv/kernel/sys_hwprobe.c b/arch/riscv/kernel/sys_hwprobe.c
> index 8cae41a502dd..aeb70afe230b 100644
> --- a/arch/riscv/kernel/sys_hwprobe.c
> +++ b/arch/riscv/kernel/sys_hwprobe.c
> @@ -13,6 +13,7 @@
>  #include <asm/uaccess.h>
>  #include <asm/unistd.h>
>  #include <asm/vector.h>
> +#include <asm/vendor_extensions/thead_hwprobe.h>
>  #include <vdso/vsyscall.h>
>
>
> @@ -216,6 +217,10 @@ static void hwprobe_one_pair(struct riscv_hwprobe *pair,
>                         pair->value = riscv_cboz_block_size;
>                 break;
>
> +       case RISCV_HWPROBE_KEY_VENDOR_EXT_THEAD_0:
> +               hwprobe_isa_vendor_ext_thead_0(pair, cpus);
> +               break;
> +
>         /*
>          * For forward compatibility, unknown keys don't fail the whole
>          * call, but get their element key set to -1 and value set to 0
> diff --git a/arch/riscv/kernel/vendor_extensions/Makefile b/arch/riscv/kernel/vendor_extensions/Makefile
> index 8f1c5a4dc38f..f511fd269e8a 100644
> --- a/arch/riscv/kernel/vendor_extensions/Makefile
> +++ b/arch/riscv/kernel/vendor_extensions/Makefile
> @@ -1,4 +1,5 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>
>  obj-$(CONFIG_RISCV_ISA_VENDOR_EXT_THEAD)       += thead.o
> +obj-$(CONFIG_RISCV_ISA_VENDOR_EXT_THEAD)       += thead_hwprobe.o
>  obj-$(CONFIG_RISCV_ISA_VENDOR_EXT_ANDES)       += andes.o
> diff --git a/arch/riscv/kernel/vendor_extensions/thead_hwprobe.c b/arch/riscv/kernel/vendor_extensions/thead_hwprobe.c
> new file mode 100644
> index 000000000000..53f65942f7e8
> --- /dev/null
> +++ b/arch/riscv/kernel/vendor_extensions/thead_hwprobe.c
> @@ -0,0 +1,19 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <asm/vendor_extensions/thead.h>
> +#include <asm/vendor_extensions/thead_hwprobe.h>
> +#include <asm/vendor_extensions/vendor_hwprobe.h>
> +
> +#include <linux/cpumask.h>
> +#include <linux/types.h>
> +
> +#include <uapi/asm/hwprobe.h>
> +#include <uapi/asm/vendor/thead.h>
> +
> +void hwprobe_isa_vendor_ext_thead_0(struct riscv_hwprobe *pair, const struct cpumask *cpus)
> +{
> +       VENDOR_EXTENSION_SUPPORTED(pair, cpus,
> +                                  riscv_isa_vendor_ext_list_thead.per_hart_isa_bitmap, {
> +               EXT_KEY(XTHEADVECTOR);
> +       });
> +}

Very tidy!


>
> --
> 2.44.0
>

On Thu, May 2, 2024 at 9:47 PM Charlie Jenkins <charlie@rivosinc.com> wrote:
>
> Document support for thead vendor extensions using the key
> RISCV_HWPROBE_KEY_VENDOR_EXT_THEAD_0 and xtheadvector extension using
> the key RISCV_HWPROBE_VENDOR_EXT_XTHEADVECTOR.
>
> Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>

Reviewed-by: Evan Green <evan@rivosinc.com>

On Thu, May 02, 2024 at 09:46:38PM -0700, Charlie Jenkins wrote:
> If vlenb is provided in the device tree, prefer that over reading the
> vlenb csr.
> 
> Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> ---
>  arch/riscv/include/asm/cpufeature.h |  2 ++
>  arch/riscv/kernel/cpufeature.c      | 43 +++++++++++++++++++++++++++++++++++++
>  arch/riscv/kernel/vector.c          | 12 ++++++++++-
>  3 files changed, 56 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h
> index 347805446151..0c4f08577015 100644
> --- a/arch/riscv/include/asm/cpufeature.h
> +++ b/arch/riscv/include/asm/cpufeature.h
> @@ -31,6 +31,8 @@ DECLARE_PER_CPU(struct riscv_cpuinfo, riscv_cpuinfo);
>  /* Per-cpu ISA extensions. */
>  extern struct riscv_isainfo hart_isa[NR_CPUS];
>  
> +extern u32 riscv_vlenb_of;
> +
>  void riscv_user_isa_enable(void);
>  
>  #if defined(CONFIG_RISCV_MISALIGNED)
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 3ed2359eae35..12c79db0b0bb 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -35,6 +35,8 @@ static DECLARE_BITMAP(riscv_isa, RISCV_ISA_EXT_MAX) __read_mostly;
>  /* Per-cpu ISA extensions. */
>  struct riscv_isainfo hart_isa[NR_CPUS];
>  
> +u32 riscv_vlenb_of;
> +
>  /**
>   * riscv_isa_extension_base() - Get base extension word
>   *
> @@ -648,6 +650,42 @@ static int __init riscv_isa_fallback_setup(char *__unused)
>  early_param("riscv_isa_fallback", riscv_isa_fallback_setup);
>  #endif
>  
> +static int has_riscv_homogeneous_vlenb(void)
> +{
> +	int cpu;
> +	u32 prev_vlenb = 0;
> +	u32 vlenb;
> +
> +	for_each_possible_cpu(cpu) {
> +		struct device_node *cpu_node;
> +
> +		cpu_node = of_cpu_device_node_get(cpu);
> +		if (!cpu_node) {
> +			pr_warn("Unable to find cpu node\n");
> +			return -ENOENT;
> +		}
> +
> +		if (of_property_read_u32(cpu_node, "riscv,vlenb", &vlenb)) {
> +			of_node_put(cpu_node);
> +
> +			if (prev_vlenb)
> +				return -ENOENT;
> +			continue;
> +		}
> +
> +		if (prev_vlenb && vlenb != prev_vlenb) {
> +			of_node_put(cpu_node);
> +			return -ENOENT;
> +		}
> +
> +		prev_vlenb = vlenb;
> +		of_node_put(cpu_node);
> +	}
> +
> +	riscv_vlenb_of = vlenb;
> +	return 0;
> +}
> +
>  void __init riscv_fill_hwcap(void)
>  {
>  	char print_str[NUM_ALPHA_EXTS + 1];
> @@ -671,6 +709,11 @@ void __init riscv_fill_hwcap(void)
>  			pr_info("Falling back to deprecated \"riscv,isa\"\n");
>  			riscv_fill_hwcap_from_isa_string(isa2hwcap);
>  		}
> +
> +		if (elf_hwcap & COMPAT_HWCAP_ISA_V && has_riscv_homogeneous_vlenb() < 0) {

I still think this isn't quite right, as it will emit a warning when
RISCV_ISA_V is disabled. The simplest thing to do probably is just
add an `if (IS_ENABLED(CONFIG_RISCV_ISA_V) return 0` shortcut the to
function? It'll get disabled a few lines later so I think a zero is
safe.

> +			pr_warn("Unsupported heterogeneous vlen detected, vector extension disabled.\n");
> +			elf_hwcap &= ~COMPAT_HWCAP_ISA_V;
> +		}
>  	}
>  
>  	/*
> diff --git a/arch/riscv/kernel/vector.c b/arch/riscv/kernel/vector.c
> index 6727d1d3b8f2..e04586cdb7f0 100644
> --- a/arch/riscv/kernel/vector.c
> +++ b/arch/riscv/kernel/vector.c
> @@ -33,7 +33,17 @@ int riscv_v_setup_vsize(void)
>  {
>  	unsigned long this_vsize;
>  
> -	/* There are 32 vector registers with vlenb length. */
> +	/*
> +	 * There are 32 vector registers with vlenb length.
> +	 *
> +	 * If the riscv,vlenb property was provided by the firmware, use that
> +	 * instead of probing the CSRs.
> +	 */
> +	if (riscv_vlenb_of) {
> +		this_vsize = riscv_vlenb_of * 32;
> +		return 0;
> +	}
> +
>  	riscv_v_enable();
>  	this_vsize = csr_read(CSR_VLENB) * 32;
>  	riscv_v_disable();
> 
> -- 
> 2.44.0
>

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

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?

>
> - Charlie
>

On Fri, May 03, 2024 at 05:59:33PM +0100, Conor Dooley wrote:
> On Thu, May 02, 2024 at 09:46:38PM -0700, Charlie Jenkins wrote:
> > If vlenb is provided in the device tree, prefer that over reading the
> > vlenb csr.
> > 
> > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> > ---
> >  arch/riscv/include/asm/cpufeature.h |  2 ++
> >  arch/riscv/kernel/cpufeature.c      | 43 +++++++++++++++++++++++++++++++++++++
> >  arch/riscv/kernel/vector.c          | 12 ++++++++++-
> >  3 files changed, 56 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h
> > index 347805446151..0c4f08577015 100644
> > --- a/arch/riscv/include/asm/cpufeature.h
> > +++ b/arch/riscv/include/asm/cpufeature.h
> > @@ -31,6 +31,8 @@ DECLARE_PER_CPU(struct riscv_cpuinfo, riscv_cpuinfo);
> >  /* Per-cpu ISA extensions. */
> >  extern struct riscv_isainfo hart_isa[NR_CPUS];
> >  
> > +extern u32 riscv_vlenb_of;
> > +
> >  void riscv_user_isa_enable(void);
> >  
> >  #if defined(CONFIG_RISCV_MISALIGNED)
> > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > index 3ed2359eae35..12c79db0b0bb 100644
> > --- a/arch/riscv/kernel/cpufeature.c
> > +++ b/arch/riscv/kernel/cpufeature.c
> > @@ -35,6 +35,8 @@ static DECLARE_BITMAP(riscv_isa, RISCV_ISA_EXT_MAX) __read_mostly;
> >  /* Per-cpu ISA extensions. */
> >  struct riscv_isainfo hart_isa[NR_CPUS];
> >  
> > +u32 riscv_vlenb_of;
> > +
> >  /**
> >   * riscv_isa_extension_base() - Get base extension word
> >   *
> > @@ -648,6 +650,42 @@ static int __init riscv_isa_fallback_setup(char *__unused)
> >  early_param("riscv_isa_fallback", riscv_isa_fallback_setup);
> >  #endif
> >  
> > +static int has_riscv_homogeneous_vlenb(void)
> > +{
> > +	int cpu;
> > +	u32 prev_vlenb = 0;
> > +	u32 vlenb;
> > +
> > +	for_each_possible_cpu(cpu) {
> > +		struct device_node *cpu_node;
> > +
> > +		cpu_node = of_cpu_device_node_get(cpu);
> > +		if (!cpu_node) {
> > +			pr_warn("Unable to find cpu node\n");
> > +			return -ENOENT;
> > +		}
> > +
> > +		if (of_property_read_u32(cpu_node, "riscv,vlenb", &vlenb)) {
> > +			of_node_put(cpu_node);
> > +
> > +			if (prev_vlenb)
> > +				return -ENOENT;
> > +			continue;
> > +		}
> > +
> > +		if (prev_vlenb && vlenb != prev_vlenb) {
> > +			of_node_put(cpu_node);
> > +			return -ENOENT;
> > +		}
> > +
> > +		prev_vlenb = vlenb;
> > +		of_node_put(cpu_node);
> > +	}
> > +
> > +	riscv_vlenb_of = vlenb;
> > +	return 0;
> > +}
> > +
> >  void __init riscv_fill_hwcap(void)
> >  {
> >  	char print_str[NUM_ALPHA_EXTS + 1];
> > @@ -671,6 +709,11 @@ void __init riscv_fill_hwcap(void)
> >  			pr_info("Falling back to deprecated \"riscv,isa\"\n");
> >  			riscv_fill_hwcap_from_isa_string(isa2hwcap);
> >  		}
> > +
> > +		if (elf_hwcap & COMPAT_HWCAP_ISA_V && has_riscv_homogeneous_vlenb() < 0) {
> 
> I still think this isn't quite right, as it will emit a warning when
> RISCV_ISA_V is disabled. The simplest thing to do probably is just
> add an `if (IS_ENABLED(CONFIG_RISCV_ISA_V) return 0` shortcut the to
> function? It'll get disabled a few lines later so I think a zero is
> safe.

That seems like a good idea. It is weird to throw a warning about this
even when they have V disabled in the kernel. The DT is improperly
formatted since it has heterogeneous vlenb entries and has V enabled,
but since the user disabled V in the kernel skipping the warning is
reasonable.

- Charlie

> 
> > +			pr_warn("Unsupported heterogeneous vlen detected, vector extension disabled.\n");
> > +			elf_hwcap &= ~COMPAT_HWCAP_ISA_V;
> > +		}
> >  	}
> >  
> >  	/*
> > diff --git a/arch/riscv/kernel/vector.c b/arch/riscv/kernel/vector.c
> > index 6727d1d3b8f2..e04586cdb7f0 100644
> > --- a/arch/riscv/kernel/vector.c
> > +++ b/arch/riscv/kernel/vector.c
> > @@ -33,7 +33,17 @@ int riscv_v_setup_vsize(void)
> >  {
> >  	unsigned long this_vsize;
> >  
> > -	/* There are 32 vector registers with vlenb length. */
> > +	/*
> > +	 * There are 32 vector registers with vlenb length.
> > +	 *
> > +	 * If the riscv,vlenb property was provided by the firmware, use that
> > +	 * instead of probing the CSRs.
> > +	 */
> > +	if (riscv_vlenb_of) {
> > +		this_vsize = riscv_vlenb_of * 32;
> > +		return 0;
> > +	}
> > +
> >  	riscv_v_enable();
> >  	this_vsize = csr_read(CSR_VLENB) * 32;
> >  	riscv_v_disable();
> > 
> > -- 
> > 2.44.0
> >

On Fri, May 03, 2024 at 10:15:16AM -0700, Charlie Jenkins wrote:
> The DT is improperly
> formatted since it has heterogeneous vlenb entries and has V enabled,
> but since the user disabled V in the kernel skipping the warning is
> reasonable.

I wouldn't go as far as "improperly formatted", as if the harts really
do have differing vector lengths, it's correctly formatted. It's just
not something we support in Linux.

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

On Fri, May 03, 2024 at 06:26:58PM +0100, Conor Dooley wrote:
> On Fri, May 03, 2024 at 10:15:16AM -0700, Charlie Jenkins wrote:
> > The DT is improperly
> > formatted since it has heterogeneous vlenb entries and has V enabled,
> > but since the user disabled V in the kernel skipping the warning is
> > reasonable.
> 
> I wouldn't go as far as "improperly formatted", as if the harts really
> do have differing vector lengths, it's correctly formatted. It's just
> not something we support in Linux.

Fair enough, not supported is a better term here.

- Charlie