[16/19] riscv: hwprobe: Add vendor extension probing

Message ID	20240411-dev-charlie-support_thead_vector_6_9-v1-16-4af9815ec746@rivosinc.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org> From: Charlie Jenkins <charlie@rivosinc.com> Date: Thu, 11 Apr 2024 21:11:22 -0700 Subject: [PATCH 16/19] riscv: hwprobe: Add vendor extension probing MIME-Version: 1.0 Message-Id: <20240411-dev-charlie-support_thead_vector_6_9-v1-16-4af9815ec746@rivosinc.com> References: <20240411-dev-charlie-support_thead_vector_6_9-v1-0-4af9815ec746@rivosinc.com> In-Reply-To: <20240411-dev-charlie-support_thead_vector_6_9-v1-0-4af9815ec746@rivosinc.com> To: Conor Dooley <conor@kernel.org>, Rob Herring <robh@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Paul Walmsley <paul.walmsley@sifive.com>, Palmer Dabbelt <palmer@dabbelt.com>, Albert Ou <aou@eecs.berkeley.edu>, Guo Ren <guoren@kernel.org>, Conor Dooley <conor+dt@kernel.org>, Chen-Yu Tsai <wens@csie.org>, Jernej Skrabec <jernej.skrabec@gmail.com>, Samuel Holland <samuel@sholland.org>, Conor Dooley <conor.dooley@microchip.com>, Evan Green <evan@rivosinc.com>, =?utf-8?b?Q2zDqW1lbnQgTMOpZ2Vy?= <cleger@rivosinc.com>, Jonathan Corbet <corbet@lwn.net>, Shuah Khan <shuah@kernel.org> Cc: linux-riscv@lists.infradead.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Palmer Dabbelt <palmer@rivosinc.com>, linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev, linux-doc@vger.kernel.org, linux-kselftest@vger.kernel.org, Charlie Jenkins <charlie@rivosinc.com> Precedence: list Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" <linux-arm-kernel-bounces@lists.infradead.org> Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org
Series	riscv: Support vendor extensions and xtheadvector \| expand [00/19] riscv: Support vendor extensions and xtheadvector [01/19] dt-bindings: riscv: Add vendorid and archid [02/19] riscv: cpufeature: Fix thead vector hwcap removal [03/19] dt-bindings: riscv: Add xtheadvector ISA extension description [04/19] riscv: dts: allwinner: Add xtheadvector to the D1/D1s devicetree [05/19] riscv: Fix extension subset checking [06/19] riscv: Extend cpufeature.c to detect vendor extensions [07/19] riscv: Optimize riscv_cpu_isa_extension_(un)likely() [08/19] riscv: Introduce vendor variants of extension helpers [09/19] riscv: uaccess: Add alternative for xtheadvector uaccess [10/19] RISC-V: define the elements of the VCSR vector CSR [11/19] riscv: csr: Add CSR encodings for VCSR_VXRM/VCSR_VXSAT [12/19] riscv: Create xtheadvector file [13/19] riscv: vector: Support xtheadvector save/restore [14/19] riscv: hwprobe: Disambiguate vector and xtheadvector in hwprobe [15/19] riscv: hwcap: Add v to hwcap if xtheadvector enabled [16/19] riscv: hwprobe: Add vendor extension probing [17/19] riscv: hwprobe: Document vendor extensions and xtheadvector extension [18/19] selftests: riscv: Fix vector tests [19/19] selftests: riscv: Support xtheadvector in vector tests

Charlie Jenkins April 12, 2024, 4:11 a.m. UTC

Add a new hwprobe key "RISCV_HWPROBE_KEY_VENDOR_EXT_0" which allows
userspace to probe for the new RISCV_ISA_VENDOR_EXT_XTHEADVECTOR vendor
extension.

Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
---
 arch/riscv/include/asm/hwprobe.h      |  4 +--
 arch/riscv/include/uapi/asm/hwprobe.h | 10 +++++-
 arch/riscv/kernel/sys_hwprobe.c       | 59 +++++++++++++++++++++++++++++++++--
 3 files changed, 68 insertions(+), 5 deletions(-)

Conor Dooley April 12, 2024, 11:39 a.m. UTC | #1

On Thu, Apr 11, 2024 at 09:11:22PM -0700, Charlie Jenkins wrote:

> +static void hwprobe_isa_vendor_ext0(struct riscv_hwprobe *pair,
> +				    const struct cpumask *cpus)
> +{
> +	int cpu;
> +	u64 missing = 0;
> +
> +	pair->value = 0;
> +
> +	struct riscv_hwprobe mvendorid = {
> +		.key = RISCV_HWPROBE_KEY_MVENDORID,
> +		.value = 0
> +	};
> +
> +	hwprobe_arch_id(&mvendorid, cpus);
> +
> +	/* Set value to zero if CPUs in the set do not have the same vendor. */
> +	if (mvendorid.value == -1ULL)
> +		return;
> +
> +	/*
> +	 * Loop through and record vendor extensions that 1) anyone has, and
> +	 * 2) anyone doesn't have.
> +	 */
> +	for_each_cpu(cpu, cpus) {
> +		struct riscv_isainfo *isavendorinfo = &hart_isa_vendor[cpu];
> +
> +#define VENDOR_EXT_KEY(ext)								\
> +	do {										\
> +		if (__riscv_isa_vendor_extension_available(isavendorinfo->isa,		\
> +							 RISCV_ISA_VENDOR_EXT_##ext))	\
> +			pair->value |= RISCV_HWPROBE_VENDOR_EXT_##ext;			\
> +		else									\
> +			missing |= RISCV_HWPROBE_VENDOR_EXT_##ext;			\
> +	} while (false)
> +
> +	/*
> +	 * Only use VENDOR_EXT_KEY() for extensions which can be exposed to userspace,
> +	 * regardless of the kernel's configuration, as no other checks, besides
> +	 * presence in the hart_vendor_isa bitmap, are made.
> +	 */
> +	VENDOR_EXT_KEY(XTHEADVECTOR);

Reading the comment here, I don't think you can do this. All vector
support in userspace is continent on kernel configuration options.

> +
> +#undef VENDOR_EXT_KEY
> +	}
> +
> +	/* Now turn off reporting features if any CPU is missing it. */
> +	pair->value &= ~missing;
> +}

Evan Green April 12, 2024, 5:05 p.m. UTC | #2

On Thu, Apr 11, 2024 at 9:12 PM Charlie Jenkins <charlie@rivosinc.com> wrote:
>
> Add a new hwprobe key "RISCV_HWPROBE_KEY_VENDOR_EXT_0" which allows
> userspace to probe for the new RISCV_ISA_VENDOR_EXT_XTHEADVECTOR vendor
> extension.
>
> Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> ---
>  arch/riscv/include/asm/hwprobe.h      |  4 +--
>  arch/riscv/include/uapi/asm/hwprobe.h | 10 +++++-
>  arch/riscv/kernel/sys_hwprobe.c       | 59 +++++++++++++++++++++++++++++++++--
>  3 files changed, 68 insertions(+), 5 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/uapi/asm/hwprobe.h b/arch/riscv/include/uapi/asm/hwprobe.h
> index 9f2a8e3ff204..6614d3adfc75 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,14 @@ 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
> +/*
> + * It is not possible for one CPU to have multiple vendor ids, so each vendor
> + * has its own vendor extension "namespace". The keys for each vendor starts
> + * at zero.
> + */
> +#define RISCV_HWPROBE_KEY_VENDOR_EXT_0 7
> + /* T-Head */
> +#define                RISCV_HWPROBE_VENDOR_EXT_XTHEADVECTOR   (1 << 0)
>  /* Increase RISCV_HWPROBE_MAX_KEY when adding items. */
>
>  /* Flags */
> diff --git a/arch/riscv/kernel/sys_hwprobe.c b/arch/riscv/kernel/sys_hwprobe.c
> index e0a42c851511..365ce7380443 100644
> --- a/arch/riscv/kernel/sys_hwprobe.c
> +++ b/arch/riscv/kernel/sys_hwprobe.c
> @@ -69,7 +69,8 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
>         if (riscv_isa_extension_available(NULL, c))
>                 pair->value |= RISCV_HWPROBE_IMA_C;
>
> -       if (has_vector() && !riscv_has_vendor_extension_unlikely(RISCV_ISA_VENDOR_EXT_XTHEADVECTOR))
> +       if (has_vector() &&
> +           !__riscv_isa_vendor_extension_available(NULL, RISCV_ISA_VENDOR_EXT_XTHEADVECTOR))
>                 pair->value |= RISCV_HWPROBE_IMA_V;
>
>         /*
> @@ -112,7 +113,8 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
>                 EXT_KEY(ZACAS);
>                 EXT_KEY(ZICOND);
>
> -               if (has_vector() && !riscv_has_vendor_extension_unlikely(RISCV_ISA_VENDOR_EXT_XTHEADVECTOR)) {
> +               if (has_vector() &&
> +                   !riscv_has_vendor_extension_unlikely(RISCV_ISA_VENDOR_EXT_XTHEADVECTOR)) {
>                         EXT_KEY(ZVBB);
>                         EXT_KEY(ZVBC);
>                         EXT_KEY(ZVKB);
> @@ -139,6 +141,55 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
>         pair->value &= ~missing;
>  }
>
> +static void hwprobe_isa_vendor_ext0(struct riscv_hwprobe *pair,
> +                                   const struct cpumask *cpus)
> +{
> +       int cpu;
> +       u64 missing = 0;
> +
> +       pair->value = 0;
> +
> +       struct riscv_hwprobe mvendorid = {
> +               .key = RISCV_HWPROBE_KEY_MVENDORID,
> +               .value = 0
> +       };
> +
> +       hwprobe_arch_id(&mvendorid, cpus);
> +
> +       /* Set value to zero if CPUs in the set do not have the same vendor. */
> +       if (mvendorid.value == -1ULL)
> +               return;
> +
> +       /*
> +        * Loop through and record vendor extensions that 1) anyone has, and
> +        * 2) anyone doesn't have.
> +        */
> +       for_each_cpu(cpu, cpus) {
> +               struct riscv_isainfo *isavendorinfo = &hart_isa_vendor[cpu];
> +
> +#define VENDOR_EXT_KEY(ext)                                                            \
> +       do {                                                                            \
> +               if (__riscv_isa_vendor_extension_available(isavendorinfo->isa,          \
> +                                                        RISCV_ISA_VENDOR_EXT_##ext))   \
> +                       pair->value |= RISCV_HWPROBE_VENDOR_EXT_##ext;                  \
> +               else                                                                    \
> +                       missing |= RISCV_HWPROBE_VENDOR_EXT_##ext;                      \
> +       } while (false)
> +
> +       /*
> +        * Only use VENDOR_EXT_KEY() for extensions which can be exposed to userspace,
> +        * regardless of the kernel's configuration, as no other checks, besides
> +        * presence in the hart_vendor_isa bitmap, are made.
> +        */
> +       VENDOR_EXT_KEY(XTHEADVECTOR);
> +
> +#undef VENDOR_EXT_KEY

Hey Charlie,
Thanks for writing this up! At the very least I think the
THEAD-specific stuff should probably end up in its own file, otherwise
it'll get chaotic with vendors clamoring to add stuff right here.
What do you think about this approach:
 * We leave RISCV_HWPROBE_MAX_KEY as the max key for the "generic
world", eg 6-ish
 * We define that any key above 0x8000000000000000 is in the vendor
space, so the meaning of the keys depends first on the mvendorid
value.
 * In the kernel code, each new vendor adds on to a global struct,
which might look something like:
struct hwprobe_vendor_space vendor_space[] = {
        {
                .mvendorid = VENDOR_THEAD,
                .max_hwprobe_key = THEAD_MAX_HWPROBE_KEY, // currently
1 or 0x8000000000000001 with what you've got.
                .hwprobe_fn = thead_hwprobe
        },
        ...
};

 * A hwprobe_thead.c implements thead_hwprobe(), and is called
whenever the generic hwprobe encounters a key >=0x8000000000000000.
 * Generic code for setting up the VDSO can then still call the
vendor-specific hwprobe_fn() repeatedly with an "all CPUs" mask from
the base to max_hwprobe_key and set up the cached tables in userspace.
 * Since the VDSO data has limited space we may have to cap the number
of vendor keys we cache to be lower than max_hwprobe_key. Since the
data itself is not exposed to usermode we can raise this cap later if
needed.


-Evan

> +       }
> +
> +       /* Now turn off reporting features if any CPU is missing it. */
> +       pair->value &= ~missing;
> +}
> +
>  static bool hwprobe_ext0_has(const struct cpumask *cpus, unsigned long ext)
>  {
>         struct riscv_hwprobe pair;
> @@ -216,6 +267,10 @@ static void hwprobe_one_pair(struct riscv_hwprobe *pair,
>                         pair->value = riscv_cboz_block_size;
>                 break;
>
> +       case RISCV_HWPROBE_KEY_VENDOR_EXT_0:
> +               hwprobe_isa_vendor_ext0(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
>
> --
> 2.44.0
>

Charlie Jenkins April 12, 2024, 6:16 p.m. UTC | #3

On Fri, Apr 12, 2024 at 10:05:21AM -0700, Evan Green wrote:
> On Thu, Apr 11, 2024 at 9:12 PM Charlie Jenkins <charlie@rivosinc.com> wrote:
> >
> > Add a new hwprobe key "RISCV_HWPROBE_KEY_VENDOR_EXT_0" which allows
> > userspace to probe for the new RISCV_ISA_VENDOR_EXT_XTHEADVECTOR vendor
> > extension.
> >
> > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> > ---
> >  arch/riscv/include/asm/hwprobe.h      |  4 +--
> >  arch/riscv/include/uapi/asm/hwprobe.h | 10 +++++-
> >  arch/riscv/kernel/sys_hwprobe.c       | 59 +++++++++++++++++++++++++++++++++--
> >  3 files changed, 68 insertions(+), 5 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/uapi/asm/hwprobe.h b/arch/riscv/include/uapi/asm/hwprobe.h
> > index 9f2a8e3ff204..6614d3adfc75 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,14 @@ 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
> > +/*
> > + * It is not possible for one CPU to have multiple vendor ids, so each vendor
> > + * has its own vendor extension "namespace". The keys for each vendor starts
> > + * at zero.
> > + */
> > +#define RISCV_HWPROBE_KEY_VENDOR_EXT_0 7
> > + /* T-Head */
> > +#define                RISCV_HWPROBE_VENDOR_EXT_XTHEADVECTOR   (1 << 0)
> >  /* Increase RISCV_HWPROBE_MAX_KEY when adding items. */
> >
> >  /* Flags */
> > diff --git a/arch/riscv/kernel/sys_hwprobe.c b/arch/riscv/kernel/sys_hwprobe.c
> > index e0a42c851511..365ce7380443 100644
> > --- a/arch/riscv/kernel/sys_hwprobe.c
> > +++ b/arch/riscv/kernel/sys_hwprobe.c
> > @@ -69,7 +69,8 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
> >         if (riscv_isa_extension_available(NULL, c))
> >                 pair->value |= RISCV_HWPROBE_IMA_C;
> >
> > -       if (has_vector() && !riscv_has_vendor_extension_unlikely(RISCV_ISA_VENDOR_EXT_XTHEADVECTOR))
> > +       if (has_vector() &&
> > +           !__riscv_isa_vendor_extension_available(NULL, RISCV_ISA_VENDOR_EXT_XTHEADVECTOR))
> >                 pair->value |= RISCV_HWPROBE_IMA_V;
> >
> >         /*
> > @@ -112,7 +113,8 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
> >                 EXT_KEY(ZACAS);
> >                 EXT_KEY(ZICOND);
> >
> > -               if (has_vector() && !riscv_has_vendor_extension_unlikely(RISCV_ISA_VENDOR_EXT_XTHEADVECTOR)) {
> > +               if (has_vector() &&
> > +                   !riscv_has_vendor_extension_unlikely(RISCV_ISA_VENDOR_EXT_XTHEADVECTOR)) {
> >                         EXT_KEY(ZVBB);
> >                         EXT_KEY(ZVBC);
> >                         EXT_KEY(ZVKB);
> > @@ -139,6 +141,55 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
> >         pair->value &= ~missing;
> >  }
> >
> > +static void hwprobe_isa_vendor_ext0(struct riscv_hwprobe *pair,
> > +                                   const struct cpumask *cpus)
> > +{
> > +       int cpu;
> > +       u64 missing = 0;
> > +
> > +       pair->value = 0;
> > +
> > +       struct riscv_hwprobe mvendorid = {
> > +               .key = RISCV_HWPROBE_KEY_MVENDORID,
> > +               .value = 0
> > +       };
> > +
> > +       hwprobe_arch_id(&mvendorid, cpus);
> > +
> > +       /* Set value to zero if CPUs in the set do not have the same vendor. */
> > +       if (mvendorid.value == -1ULL)
> > +               return;
> > +
> > +       /*
> > +        * Loop through and record vendor extensions that 1) anyone has, and
> > +        * 2) anyone doesn't have.
> > +        */
> > +       for_each_cpu(cpu, cpus) {
> > +               struct riscv_isainfo *isavendorinfo = &hart_isa_vendor[cpu];
> > +
> > +#define VENDOR_EXT_KEY(ext)                                                            \
> > +       do {                                                                            \
> > +               if (__riscv_isa_vendor_extension_available(isavendorinfo->isa,          \
> > +                                                        RISCV_ISA_VENDOR_EXT_##ext))   \
> > +                       pair->value |= RISCV_HWPROBE_VENDOR_EXT_##ext;                  \
> > +               else                                                                    \
> > +                       missing |= RISCV_HWPROBE_VENDOR_EXT_##ext;                      \
> > +       } while (false)
> > +
> > +       /*
> > +        * Only use VENDOR_EXT_KEY() for extensions which can be exposed to userspace,
> > +        * regardless of the kernel's configuration, as no other checks, besides
> > +        * presence in the hart_vendor_isa bitmap, are made.
> > +        */
> > +       VENDOR_EXT_KEY(XTHEADVECTOR);
> > +
> > +#undef VENDOR_EXT_KEY
> 
> Hey Charlie,
> Thanks for writing this up! At the very least I think the
> THEAD-specific stuff should probably end up in its own file, otherwise
> it'll get chaotic with vendors clamoring to add stuff right here.

Great idea!

> What do you think about this approach:
>  * We leave RISCV_HWPROBE_MAX_KEY as the max key for the "generic
> world", eg 6-ish
>  * We define that any key above 0x8000000000000000 is in the vendor
> space, so the meaning of the keys depends first on the mvendorid
> value.
>  * In the kernel code, each new vendor adds on to a global struct,
> which might look something like:
> struct hwprobe_vendor_space vendor_space[] = {
>         {
>                 .mvendorid = VENDOR_THEAD,
>                 .max_hwprobe_key = THEAD_MAX_HWPROBE_KEY, // currently
> 1 or 0x8000000000000001 with what you've got.
>                 .hwprobe_fn = thead_hwprobe
>         },
>         ...
> };
> 
>  * A hwprobe_thead.c implements thead_hwprobe(), and is called
> whenever the generic hwprobe encounters a key >=0x8000000000000000.
>  * Generic code for setting up the VDSO can then still call the
> vendor-specific hwprobe_fn() repeatedly with an "all CPUs" mask from
> the base to max_hwprobe_key and set up the cached tables in userspace.
>  * Since the VDSO data has limited space we may have to cap the number
> of vendor keys we cache to be lower than max_hwprobe_key. Since the
> data itself is not exposed to usermode we can raise this cap later if
> needed.

I know vendor extensions are kind of the "wild west" of riscv, but in
spite of that I want to design a consistent API. The issue I had with
having this "vendor space" for exposing vendor extensions was that this
is something that is inherently the same for all vendors. I see a vendor
space like this more applicable for something like
"RISCV_HWPROBE_KEY_ZICBOZ_BLOCK_SIZE" where a vendor has a specific
value they would like to expose. I do agree that having a vendor space
is a good design choice, but I am not convinced that vendor extensions
are the proper use-case.

By having RISCV_HWPROBE_KEY_VENDOR_EXT_0 we can expose the vendor
extensions in the same way that standard extensions are exposed, with a
bitmask representing each extension. If these are instead in the vendor
space, each vendor would probably be inclined to introduce a key like
RISCV_HWPROBE_KEY_THEAD_EXT_0 that returns a bitmask of all of the thead
vendor extensions. This duplicated effort is what I am trying to avoid.
The alternative would be that vendors have a separate key for each
vendor extension they would like to expose, but that is strictly less
efficient than the existing bitmask probing.

Do you think that having the vendor space is appropriate for vendor
extensions given my concerns?

- Charlie

> 
> 
> -Evan
> 
> > +       }
> > +
> > +       /* Now turn off reporting features if any CPU is missing it. */
> > +       pair->value &= ~missing;
> > +}
> > +
> >  static bool hwprobe_ext0_has(const struct cpumask *cpus, unsigned long ext)
> >  {
> >         struct riscv_hwprobe pair;
> > @@ -216,6 +267,10 @@ static void hwprobe_one_pair(struct riscv_hwprobe *pair,
> >                         pair->value = riscv_cboz_block_size;
> >                 break;
> >
> > +       case RISCV_HWPROBE_KEY_VENDOR_EXT_0:
> > +               hwprobe_isa_vendor_ext0(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
> >
> > --
> > 2.44.0
> >

Evan Green April 12, 2024, 7:07 p.m. UTC | #4

On Fri, Apr 12, 2024 at 11:17 AM Charlie Jenkins <charlie@rivosinc.com> wrote:
>
> On Fri, Apr 12, 2024 at 10:05:21AM -0700, Evan Green wrote:
> > On Thu, Apr 11, 2024 at 9:12 PM Charlie Jenkins <charlie@rivosinc.com> wrote:
> > >
> > > Add a new hwprobe key "RISCV_HWPROBE_KEY_VENDOR_EXT_0" which allows
> > > userspace to probe for the new RISCV_ISA_VENDOR_EXT_XTHEADVECTOR vendor
> > > extension.
> > >
> > > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> > > ---
> > >  arch/riscv/include/asm/hwprobe.h      |  4 +--
> > >  arch/riscv/include/uapi/asm/hwprobe.h | 10 +++++-
> > >  arch/riscv/kernel/sys_hwprobe.c       | 59 +++++++++++++++++++++++++++++++++--
> > >  3 files changed, 68 insertions(+), 5 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/uapi/asm/hwprobe.h b/arch/riscv/include/uapi/asm/hwprobe.h
> > > index 9f2a8e3ff204..6614d3adfc75 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,14 @@ 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
> > > +/*
> > > + * It is not possible for one CPU to have multiple vendor ids, so each vendor
> > > + * has its own vendor extension "namespace". The keys for each vendor starts
> > > + * at zero.
> > > + */
> > > +#define RISCV_HWPROBE_KEY_VENDOR_EXT_0 7
> > > + /* T-Head */
> > > +#define                RISCV_HWPROBE_VENDOR_EXT_XTHEADVECTOR   (1 << 0)
> > >  /* Increase RISCV_HWPROBE_MAX_KEY when adding items. */
> > >
> > >  /* Flags */
> > > diff --git a/arch/riscv/kernel/sys_hwprobe.c b/arch/riscv/kernel/sys_hwprobe.c
> > > index e0a42c851511..365ce7380443 100644
> > > --- a/arch/riscv/kernel/sys_hwprobe.c
> > > +++ b/arch/riscv/kernel/sys_hwprobe.c
> > > @@ -69,7 +69,8 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
> > >         if (riscv_isa_extension_available(NULL, c))
> > >                 pair->value |= RISCV_HWPROBE_IMA_C;
> > >
> > > -       if (has_vector() && !riscv_has_vendor_extension_unlikely(RISCV_ISA_VENDOR_EXT_XTHEADVECTOR))
> > > +       if (has_vector() &&
> > > +           !__riscv_isa_vendor_extension_available(NULL, RISCV_ISA_VENDOR_EXT_XTHEADVECTOR))
> > >                 pair->value |= RISCV_HWPROBE_IMA_V;
> > >
> > >         /*
> > > @@ -112,7 +113,8 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
> > >                 EXT_KEY(ZACAS);
> > >                 EXT_KEY(ZICOND);
> > >
> > > -               if (has_vector() && !riscv_has_vendor_extension_unlikely(RISCV_ISA_VENDOR_EXT_XTHEADVECTOR)) {
> > > +               if (has_vector() &&
> > > +                   !riscv_has_vendor_extension_unlikely(RISCV_ISA_VENDOR_EXT_XTHEADVECTOR)) {
> > >                         EXT_KEY(ZVBB);
> > >                         EXT_KEY(ZVBC);
> > >                         EXT_KEY(ZVKB);
> > > @@ -139,6 +141,55 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
> > >         pair->value &= ~missing;
> > >  }
> > >
> > > +static void hwprobe_isa_vendor_ext0(struct riscv_hwprobe *pair,
> > > +                                   const struct cpumask *cpus)
> > > +{
> > > +       int cpu;
> > > +       u64 missing = 0;
> > > +
> > > +       pair->value = 0;
> > > +
> > > +       struct riscv_hwprobe mvendorid = {
> > > +               .key = RISCV_HWPROBE_KEY_MVENDORID,
> > > +               .value = 0
> > > +       };
> > > +
> > > +       hwprobe_arch_id(&mvendorid, cpus);
> > > +
> > > +       /* Set value to zero if CPUs in the set do not have the same vendor. */
> > > +       if (mvendorid.value == -1ULL)
> > > +               return;
> > > +
> > > +       /*
> > > +        * Loop through and record vendor extensions that 1) anyone has, and
> > > +        * 2) anyone doesn't have.
> > > +        */
> > > +       for_each_cpu(cpu, cpus) {
> > > +               struct riscv_isainfo *isavendorinfo = &hart_isa_vendor[cpu];
> > > +
> > > +#define VENDOR_EXT_KEY(ext)                                                            \
> > > +       do {                                                                            \
> > > +               if (__riscv_isa_vendor_extension_available(isavendorinfo->isa,          \
> > > +                                                        RISCV_ISA_VENDOR_EXT_##ext))   \
> > > +                       pair->value |= RISCV_HWPROBE_VENDOR_EXT_##ext;                  \
> > > +               else                                                                    \
> > > +                       missing |= RISCV_HWPROBE_VENDOR_EXT_##ext;                      \
> > > +       } while (false)
> > > +
> > > +       /*
> > > +        * Only use VENDOR_EXT_KEY() for extensions which can be exposed to userspace,
> > > +        * regardless of the kernel's configuration, as no other checks, besides
> > > +        * presence in the hart_vendor_isa bitmap, are made.
> > > +        */
> > > +       VENDOR_EXT_KEY(XTHEADVECTOR);
> > > +
> > > +#undef VENDOR_EXT_KEY
> >
> > Hey Charlie,
> > Thanks for writing this up! At the very least I think the
> > THEAD-specific stuff should probably end up in its own file, otherwise
> > it'll get chaotic with vendors clamoring to add stuff right here.
>
> Great idea!
>
> > What do you think about this approach:
> >  * We leave RISCV_HWPROBE_MAX_KEY as the max key for the "generic
> > world", eg 6-ish
> >  * We define that any key above 0x8000000000000000 is in the vendor
> > space, so the meaning of the keys depends first on the mvendorid
> > value.
> >  * In the kernel code, each new vendor adds on to a global struct,
> > which might look something like:
> > struct hwprobe_vendor_space vendor_space[] = {
> >         {
> >                 .mvendorid = VENDOR_THEAD,
> >                 .max_hwprobe_key = THEAD_MAX_HWPROBE_KEY, // currently
> > 1 or 0x8000000000000001 with what you've got.
> >                 .hwprobe_fn = thead_hwprobe
> >         },
> >         ...
> > };
> >
> >  * A hwprobe_thead.c implements thead_hwprobe(), and is called
> > whenever the generic hwprobe encounters a key >=0x8000000000000000.
> >  * Generic code for setting up the VDSO can then still call the
> > vendor-specific hwprobe_fn() repeatedly with an "all CPUs" mask from
> > the base to max_hwprobe_key and set up the cached tables in userspace.
> >  * Since the VDSO data has limited space we may have to cap the number
> > of vendor keys we cache to be lower than max_hwprobe_key. Since the
> > data itself is not exposed to usermode we can raise this cap later if
> > needed.
>
> I know vendor extensions are kind of the "wild west" of riscv, but in
> spite of that I want to design a consistent API. The issue I had with
> having this "vendor space" for exposing vendor extensions was that this
> is something that is inherently the same for all vendors. I see a vendor
> space like this more applicable for something like
> "RISCV_HWPROBE_KEY_ZICBOZ_BLOCK_SIZE" where a vendor has a specific
> value they would like to expose. I do agree that having a vendor space
> is a good design choice, but I am not convinced that vendor extensions
> are the proper use-case.
>
> By having RISCV_HWPROBE_KEY_VENDOR_EXT_0 we can expose the vendor
> extensions in the same way that standard extensions are exposed, with a
> bitmask representing each extension. If these are instead in the vendor
> space, each vendor would probably be inclined to introduce a key like
> RISCV_HWPROBE_KEY_THEAD_EXT_0 that returns a bitmask of all of the thead
> vendor extensions. This duplicated effort is what I am trying to avoid.
> The alternative would be that vendors have a separate key for each
> vendor extension they would like to expose, but that is strictly less
> efficient than the existing bitmask probing.
>
> Do you think that having the vendor space is appropriate for vendor
> extensions given my concerns?

I do see what you're going for. It's tidy for a bitmask to just let
anyone allocate the next bit, but leaves you with the same problem
when a vendor decides they want to expose an enum, or decides they
want to expose a bazillion things. I think a generalized version of
the approach you've written would be: simply let vendors allocate keys
from the same global space we're already using. My worry was that it
would turn into an expansive suburban sprawl of mostly dead bits, or
in the case of vendor-specific keys, full of "if (mvendor_id() !=
MINE) return 0;". My hope with the vendored keyspace is it would keep
the sprawl from polluting the general array of (hopefully valuable)
info with stuff that's likely to become less relevant as time passes.
It also lowers the bar a bit to make it easier for vendors to expose
bits, as they don't consume global space for everyone for all of time,
just themselves.

So yes, personally I'm still in the camp of siloing the vendor stuff
off to its own area.
-Evan

Charlie Jenkins April 12, 2024, 8:20 p.m. UTC | #5

On Fri, Apr 12, 2024 at 12:07:46PM -0700, Evan Green wrote:
> On Fri, Apr 12, 2024 at 11:17 AM Charlie Jenkins <charlie@rivosinc.com> wrote:
> >
> > On Fri, Apr 12, 2024 at 10:05:21AM -0700, Evan Green wrote:
> > > On Thu, Apr 11, 2024 at 9:12 PM Charlie Jenkins <charlie@rivosinc.com> wrote:
> > > >
> > > > Add a new hwprobe key "RISCV_HWPROBE_KEY_VENDOR_EXT_0" which allows
> > > > userspace to probe for the new RISCV_ISA_VENDOR_EXT_XTHEADVECTOR vendor
> > > > extension.
> > > >
> > > > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> > > > ---
> > > >  arch/riscv/include/asm/hwprobe.h      |  4 +--
> > > >  arch/riscv/include/uapi/asm/hwprobe.h | 10 +++++-
> > > >  arch/riscv/kernel/sys_hwprobe.c       | 59 +++++++++++++++++++++++++++++++++--
> > > >  3 files changed, 68 insertions(+), 5 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/uapi/asm/hwprobe.h b/arch/riscv/include/uapi/asm/hwprobe.h
> > > > index 9f2a8e3ff204..6614d3adfc75 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,14 @@ 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
> > > > +/*
> > > > + * It is not possible for one CPU to have multiple vendor ids, so each vendor
> > > > + * has its own vendor extension "namespace". The keys for each vendor starts
> > > > + * at zero.
> > > > + */
> > > > +#define RISCV_HWPROBE_KEY_VENDOR_EXT_0 7
> > > > + /* T-Head */
> > > > +#define                RISCV_HWPROBE_VENDOR_EXT_XTHEADVECTOR   (1 << 0)
> > > >  /* Increase RISCV_HWPROBE_MAX_KEY when adding items. */
> > > >
> > > >  /* Flags */
> > > > diff --git a/arch/riscv/kernel/sys_hwprobe.c b/arch/riscv/kernel/sys_hwprobe.c
> > > > index e0a42c851511..365ce7380443 100644
> > > > --- a/arch/riscv/kernel/sys_hwprobe.c
> > > > +++ b/arch/riscv/kernel/sys_hwprobe.c
> > > > @@ -69,7 +69,8 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
> > > >         if (riscv_isa_extension_available(NULL, c))
> > > >                 pair->value |= RISCV_HWPROBE_IMA_C;
> > > >
> > > > -       if (has_vector() && !riscv_has_vendor_extension_unlikely(RISCV_ISA_VENDOR_EXT_XTHEADVECTOR))
> > > > +       if (has_vector() &&
> > > > +           !__riscv_isa_vendor_extension_available(NULL, RISCV_ISA_VENDOR_EXT_XTHEADVECTOR))
> > > >                 pair->value |= RISCV_HWPROBE_IMA_V;
> > > >
> > > >         /*
> > > > @@ -112,7 +113,8 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
> > > >                 EXT_KEY(ZACAS);
> > > >                 EXT_KEY(ZICOND);
> > > >
> > > > -               if (has_vector() && !riscv_has_vendor_extension_unlikely(RISCV_ISA_VENDOR_EXT_XTHEADVECTOR)) {
> > > > +               if (has_vector() &&
> > > > +                   !riscv_has_vendor_extension_unlikely(RISCV_ISA_VENDOR_EXT_XTHEADVECTOR)) {
> > > >                         EXT_KEY(ZVBB);
> > > >                         EXT_KEY(ZVBC);
> > > >                         EXT_KEY(ZVKB);
> > > > @@ -139,6 +141,55 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
> > > >         pair->value &= ~missing;
> > > >  }
> > > >
> > > > +static void hwprobe_isa_vendor_ext0(struct riscv_hwprobe *pair,
> > > > +                                   const struct cpumask *cpus)
> > > > +{
> > > > +       int cpu;
> > > > +       u64 missing = 0;
> > > > +
> > > > +       pair->value = 0;
> > > > +
> > > > +       struct riscv_hwprobe mvendorid = {
> > > > +               .key = RISCV_HWPROBE_KEY_MVENDORID,
> > > > +               .value = 0
> > > > +       };
> > > > +
> > > > +       hwprobe_arch_id(&mvendorid, cpus);
> > > > +
> > > > +       /* Set value to zero if CPUs in the set do not have the same vendor. */
> > > > +       if (mvendorid.value == -1ULL)
> > > > +               return;
> > > > +
> > > > +       /*
> > > > +        * Loop through and record vendor extensions that 1) anyone has, and
> > > > +        * 2) anyone doesn't have.
> > > > +        */
> > > > +       for_each_cpu(cpu, cpus) {
> > > > +               struct riscv_isainfo *isavendorinfo = &hart_isa_vendor[cpu];
> > > > +
> > > > +#define VENDOR_EXT_KEY(ext)                                                            \
> > > > +       do {                                                                            \
> > > > +               if (__riscv_isa_vendor_extension_available(isavendorinfo->isa,          \
> > > > +                                                        RISCV_ISA_VENDOR_EXT_##ext))   \
> > > > +                       pair->value |= RISCV_HWPROBE_VENDOR_EXT_##ext;                  \
> > > > +               else                                                                    \
> > > > +                       missing |= RISCV_HWPROBE_VENDOR_EXT_##ext;                      \
> > > > +       } while (false)
> > > > +
> > > > +       /*
> > > > +        * Only use VENDOR_EXT_KEY() for extensions which can be exposed to userspace,
> > > > +        * regardless of the kernel's configuration, as no other checks, besides
> > > > +        * presence in the hart_vendor_isa bitmap, are made.
> > > > +        */
> > > > +       VENDOR_EXT_KEY(XTHEADVECTOR);
> > > > +
> > > > +#undef VENDOR_EXT_KEY
> > >
> > > Hey Charlie,
> > > Thanks for writing this up! At the very least I think the
> > > THEAD-specific stuff should probably end up in its own file, otherwise
> > > it'll get chaotic with vendors clamoring to add stuff right here.
> >
> > Great idea!
> >
> > > What do you think about this approach:
> > >  * We leave RISCV_HWPROBE_MAX_KEY as the max key for the "generic
> > > world", eg 6-ish
> > >  * We define that any key above 0x8000000000000000 is in the vendor
> > > space, so the meaning of the keys depends first on the mvendorid
> > > value.
> > >  * In the kernel code, each new vendor adds on to a global struct,
> > > which might look something like:
> > > struct hwprobe_vendor_space vendor_space[] = {
> > >         {
> > >                 .mvendorid = VENDOR_THEAD,
> > >                 .max_hwprobe_key = THEAD_MAX_HWPROBE_KEY, // currently
> > > 1 or 0x8000000000000001 with what you've got.
> > >                 .hwprobe_fn = thead_hwprobe
> > >         },
> > >         ...
> > > };
> > >
> > >  * A hwprobe_thead.c implements thead_hwprobe(), and is called
> > > whenever the generic hwprobe encounters a key >=0x8000000000000000.
> > >  * Generic code for setting up the VDSO can then still call the
> > > vendor-specific hwprobe_fn() repeatedly with an "all CPUs" mask from
> > > the base to max_hwprobe_key and set up the cached tables in userspace.
> > >  * Since the VDSO data has limited space we may have to cap the number
> > > of vendor keys we cache to be lower than max_hwprobe_key. Since the
> > > data itself is not exposed to usermode we can raise this cap later if
> > > needed.
> >
> > I know vendor extensions are kind of the "wild west" of riscv, but in
> > spite of that I want to design a consistent API. The issue I had with
> > having this "vendor space" for exposing vendor extensions was that this
> > is something that is inherently the same for all vendors. I see a vendor
> > space like this more applicable for something like
> > "RISCV_HWPROBE_KEY_ZICBOZ_BLOCK_SIZE" where a vendor has a specific
> > value they would like to expose. I do agree that having a vendor space
> > is a good design choice, but I am not convinced that vendor extensions
> > are the proper use-case.
> >
> > By having RISCV_HWPROBE_KEY_VENDOR_EXT_0 we can expose the vendor
> > extensions in the same way that standard extensions are exposed, with a
> > bitmask representing each extension. If these are instead in the vendor
> > space, each vendor would probably be inclined to introduce a key like
> > RISCV_HWPROBE_KEY_THEAD_EXT_0 that returns a bitmask of all of the thead
> > vendor extensions. This duplicated effort is what I am trying to avoid.
> > The alternative would be that vendors have a separate key for each
> > vendor extension they would like to expose, but that is strictly less
> > efficient than the existing bitmask probing.
> >
> > Do you think that having the vendor space is appropriate for vendor
> > extensions given my concerns?
> 
> I do see what you're going for. It's tidy for a bitmask to just let
> anyone allocate the next bit, but leaves you with the same problem
> when a vendor decides they want to expose an enum, or decides they
> want to expose a bazillion things. I think a generalized version of

This patch is strictly to expose if a vendor extension is supported,
how does exposing enums factor in here?

> the approach you've written would be: simply let vendors allocate keys
> from the same global space we're already using. My worry was that it

I am missing how my proposal suggests allowing vendors to allocate keys
in a global space.

> would turn into an expansive suburban sprawl of mostly dead bits, or
> in the case of vendor-specific keys, full of "if (mvendor_id() !=
> MINE) return 0;". My hope with the vendored keyspace is it would keep

An application will always need to check vendorid before calling hwprobe
with a vendor-specific feature? If that hwprobe support is a key above
1<<63, then the application will need to pass that vendor-specific key
and interpret the vendor-specific value. If that hwprobe support is what
I have proposed here, then the user calls the standardized vendor
extension hwprobe endpoint and then needs to interpret the result based
on the vendor of the cpumask. In both cases they need to check the
vendorid of the cpumask. In the test case I added I failed to check the
vendorid but I should have had that.

> the sprawl from polluting the general array of (hopefully valuable)
> info with stuff that's likely to become less relevant as time passes.
> It also lowers the bar a bit to make it easier for vendors to expose
> bits, as they don't consume global space for everyone for all of time,
> just themselves.

The vendor keys are tied directly to the vendor. So as it grows we would
have something like:

#define RISCV_HWPROBE_KEY_VENDOR_EXT_0	7
/* T-Head */
#define		RISCV_HWPROBE_VENDOR_EXT_XTHEADVECTOR	(1 << 0)
#define		RISCV_HWPROBE_VENDOR_EXT_XTHEAD2	(2 << 0)
#define		RISCV_HWPROBE_VENDOR_EXT_XTHEAD3	(3 << 0)
/* Vendor 2 */
#define		RISCV_HWPROBE_VENDOR_EXT_XVENDOR1	(1 << 0)
#define		RISCV_HWPROBE_VENDOR_EXT_XVENDOR2	(2 << 0)
/* Vendor 3 */
...

The keys overlap between vendors. To determine which extension a vendor
supports, hwprobe gets data from hart_isa_vendor[cpu]. If the vendor is
vendor 2, it is not possible for a vendor extension from vendor 3 to end
up in there. Only the extensions from that vendor can be supported by
that vendor's hardware. 

> 
> So yes, personally I'm still in the camp of siloing the vendor stuff
> off to its own area.

I don't quite see how what I have proposed doesn't "silo" the extensions
that pertain to each vendor since the keys are specific to each vendor.

- Charlie

> -Evan

Evan Green April 12, 2024, 9:43 p.m. UTC | #6

On Fri, Apr 12, 2024 at 1:20 PM Charlie Jenkins <charlie@rivosinc.com> wrote:
>
> On Fri, Apr 12, 2024 at 12:07:46PM -0700, Evan Green wrote:
> > On Fri, Apr 12, 2024 at 11:17 AM Charlie Jenkins <charlie@rivosinc.com> wrote:
> > >
> > > On Fri, Apr 12, 2024 at 10:05:21AM -0700, Evan Green wrote:
> > > > On Thu, Apr 11, 2024 at 9:12 PM Charlie Jenkins <charlie@rivosinc.com> wrote:
> > > > >
> > > > > Add a new hwprobe key "RISCV_HWPROBE_KEY_VENDOR_EXT_0" which allows
> > > > > userspace to probe for the new RISCV_ISA_VENDOR_EXT_XTHEADVECTOR vendor
> > > > > extension.
> > > > >
> > > > > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> > > > > ---
> > > > >  arch/riscv/include/asm/hwprobe.h      |  4 +--
> > > > >  arch/riscv/include/uapi/asm/hwprobe.h | 10 +++++-
> > > > >  arch/riscv/kernel/sys_hwprobe.c       | 59 +++++++++++++++++++++++++++++++++--
> > > > >  3 files changed, 68 insertions(+), 5 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/uapi/asm/hwprobe.h b/arch/riscv/include/uapi/asm/hwprobe.h
> > > > > index 9f2a8e3ff204..6614d3adfc75 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,14 @@ 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
> > > > > +/*
> > > > > + * It is not possible for one CPU to have multiple vendor ids, so each vendor
> > > > > + * has its own vendor extension "namespace". The keys for each vendor starts
> > > > > + * at zero.
> > > > > + */
> > > > > +#define RISCV_HWPROBE_KEY_VENDOR_EXT_0 7
> > > > > + /* T-Head */
> > > > > +#define                RISCV_HWPROBE_VENDOR_EXT_XTHEADVECTOR   (1 << 0)
> > > > >  /* Increase RISCV_HWPROBE_MAX_KEY when adding items. */
> > > > >
> > > > >  /* Flags */
> > > > > diff --git a/arch/riscv/kernel/sys_hwprobe.c b/arch/riscv/kernel/sys_hwprobe.c
> > > > > index e0a42c851511..365ce7380443 100644
> > > > > --- a/arch/riscv/kernel/sys_hwprobe.c
> > > > > +++ b/arch/riscv/kernel/sys_hwprobe.c
> > > > > @@ -69,7 +69,8 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
> > > > >         if (riscv_isa_extension_available(NULL, c))
> > > > >                 pair->value |= RISCV_HWPROBE_IMA_C;
> > > > >
> > > > > -       if (has_vector() && !riscv_has_vendor_extension_unlikely(RISCV_ISA_VENDOR_EXT_XTHEADVECTOR))
> > > > > +       if (has_vector() &&
> > > > > +           !__riscv_isa_vendor_extension_available(NULL, RISCV_ISA_VENDOR_EXT_XTHEADVECTOR))
> > > > >                 pair->value |= RISCV_HWPROBE_IMA_V;
> > > > >
> > > > >         /*
> > > > > @@ -112,7 +113,8 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
> > > > >                 EXT_KEY(ZACAS);
> > > > >                 EXT_KEY(ZICOND);
> > > > >
> > > > > -               if (has_vector() && !riscv_has_vendor_extension_unlikely(RISCV_ISA_VENDOR_EXT_XTHEADVECTOR)) {
> > > > > +               if (has_vector() &&
> > > > > +                   !riscv_has_vendor_extension_unlikely(RISCV_ISA_VENDOR_EXT_XTHEADVECTOR)) {
> > > > >                         EXT_KEY(ZVBB);
> > > > >                         EXT_KEY(ZVBC);
> > > > >                         EXT_KEY(ZVKB);
> > > > > @@ -139,6 +141,55 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
> > > > >         pair->value &= ~missing;
> > > > >  }
> > > > >
> > > > > +static void hwprobe_isa_vendor_ext0(struct riscv_hwprobe *pair,
> > > > > +                                   const struct cpumask *cpus)
> > > > > +{
> > > > > +       int cpu;
> > > > > +       u64 missing = 0;
> > > > > +
> > > > > +       pair->value = 0;
> > > > > +
> > > > > +       struct riscv_hwprobe mvendorid = {
> > > > > +               .key = RISCV_HWPROBE_KEY_MVENDORID,
> > > > > +               .value = 0
> > > > > +       };
> > > > > +
> > > > > +       hwprobe_arch_id(&mvendorid, cpus);
> > > > > +
> > > > > +       /* Set value to zero if CPUs in the set do not have the same vendor. */
> > > > > +       if (mvendorid.value == -1ULL)
> > > > > +               return;
> > > > > +
> > > > > +       /*
> > > > > +        * Loop through and record vendor extensions that 1) anyone has, and
> > > > > +        * 2) anyone doesn't have.
> > > > > +        */
> > > > > +       for_each_cpu(cpu, cpus) {
> > > > > +               struct riscv_isainfo *isavendorinfo = &hart_isa_vendor[cpu];
> > > > > +
> > > > > +#define VENDOR_EXT_KEY(ext)                                                            \
> > > > > +       do {                                                                            \
> > > > > +               if (__riscv_isa_vendor_extension_available(isavendorinfo->isa,          \
> > > > > +                                                        RISCV_ISA_VENDOR_EXT_##ext))   \
> > > > > +                       pair->value |= RISCV_HWPROBE_VENDOR_EXT_##ext;                  \
> > > > > +               else                                                                    \
> > > > > +                       missing |= RISCV_HWPROBE_VENDOR_EXT_##ext;                      \
> > > > > +       } while (false)
> > > > > +
> > > > > +       /*
> > > > > +        * Only use VENDOR_EXT_KEY() for extensions which can be exposed to userspace,
> > > > > +        * regardless of the kernel's configuration, as no other checks, besides
> > > > > +        * presence in the hart_vendor_isa bitmap, are made.
> > > > > +        */
> > > > > +       VENDOR_EXT_KEY(XTHEADVECTOR);
> > > > > +
> > > > > +#undef VENDOR_EXT_KEY
> > > >
> > > > Hey Charlie,
> > > > Thanks for writing this up! At the very least I think the
> > > > THEAD-specific stuff should probably end up in its own file, otherwise
> > > > it'll get chaotic with vendors clamoring to add stuff right here.
> > >
> > > Great idea!
> > >
> > > > What do you think about this approach:
> > > >  * We leave RISCV_HWPROBE_MAX_KEY as the max key for the "generic
> > > > world", eg 6-ish
> > > >  * We define that any key above 0x8000000000000000 is in the vendor
> > > > space, so the meaning of the keys depends first on the mvendorid
> > > > value.
> > > >  * In the kernel code, each new vendor adds on to a global struct,
> > > > which might look something like:
> > > > struct hwprobe_vendor_space vendor_space[] = {
> > > >         {
> > > >                 .mvendorid = VENDOR_THEAD,
> > > >                 .max_hwprobe_key = THEAD_MAX_HWPROBE_KEY, // currently
> > > > 1 or 0x8000000000000001 with what you've got.
> > > >                 .hwprobe_fn = thead_hwprobe
> > > >         },
> > > >         ...
> > > > };
> > > >
> > > >  * A hwprobe_thead.c implements thead_hwprobe(), and is called
> > > > whenever the generic hwprobe encounters a key >=0x8000000000000000.
> > > >  * Generic code for setting up the VDSO can then still call the
> > > > vendor-specific hwprobe_fn() repeatedly with an "all CPUs" mask from
> > > > the base to max_hwprobe_key and set up the cached tables in userspace.
> > > >  * Since the VDSO data has limited space we may have to cap the number
> > > > of vendor keys we cache to be lower than max_hwprobe_key. Since the
> > > > data itself is not exposed to usermode we can raise this cap later if
> > > > needed.
> > >
> > > I know vendor extensions are kind of the "wild west" of riscv, but in
> > > spite of that I want to design a consistent API. The issue I had with
> > > having this "vendor space" for exposing vendor extensions was that this
> > > is something that is inherently the same for all vendors. I see a vendor
> > > space like this more applicable for something like
> > > "RISCV_HWPROBE_KEY_ZICBOZ_BLOCK_SIZE" where a vendor has a specific
> > > value they would like to expose. I do agree that having a vendor space
> > > is a good design choice, but I am not convinced that vendor extensions
> > > are the proper use-case.
> > >
> > > By having RISCV_HWPROBE_KEY_VENDOR_EXT_0 we can expose the vendor
> > > extensions in the same way that standard extensions are exposed, with a
> > > bitmask representing each extension. If these are instead in the vendor
> > > space, each vendor would probably be inclined to introduce a key like
> > > RISCV_HWPROBE_KEY_THEAD_EXT_0 that returns a bitmask of all of the thead
> > > vendor extensions. This duplicated effort is what I am trying to avoid.
> > > The alternative would be that vendors have a separate key for each
> > > vendor extension they would like to expose, but that is strictly less
> > > efficient than the existing bitmask probing.
> > >
> > > Do you think that having the vendor space is appropriate for vendor
> > > extensions given my concerns?
> >
> > I do see what you're going for. It's tidy for a bitmask to just let
> > anyone allocate the next bit, but leaves you with the same problem
> > when a vendor decides they want to expose an enum, or decides they
> > want to expose a bazillion things. I think a generalized version of
>
> This patch is strictly to expose if a vendor extension is supported,
> how does exposing enums factor in here?
>
> > the approach you've written would be: simply let vendors allocate keys
> > from the same global space we're already using. My worry was that it
>
> I am missing how my proposal suggests allowing vendors to allocate keys
> in a global space.
>
> > would turn into an expansive suburban sprawl of mostly dead bits, or
> > in the case of vendor-specific keys, full of "if (mvendor_id() !=
> > MINE) return 0;". My hope with the vendored keyspace is it would keep
>
> An application will always need to check vendorid before calling hwprobe
> with a vendor-specific feature? If that hwprobe support is a key above
> 1<<63, then the application will need to pass that vendor-specific key
> and interpret the vendor-specific value. If that hwprobe support is what
> I have proposed here, then the user calls the standardized vendor
> extension hwprobe endpoint and then needs to interpret the result based
> on the vendor of the cpumask. In both cases they need to check the
> vendorid of the cpumask. In the test case I added I failed to check the
> vendorid but I should have had that.
>
> > the sprawl from polluting the general array of (hopefully valuable)
> > info with stuff that's likely to become less relevant as time passes.
> > It also lowers the bar a bit to make it easier for vendors to expose
> > bits, as they don't consume global space for everyone for all of time,
> > just themselves.
>
> The vendor keys are tied directly to the vendor. So as it grows we would
> have something like:
>
> #define RISCV_HWPROBE_KEY_VENDOR_EXT_0  7
> /* T-Head */
> #define         RISCV_HWPROBE_VENDOR_EXT_XTHEADVECTOR   (1 << 0)
> #define         RISCV_HWPROBE_VENDOR_EXT_XTHEAD2        (2 << 0)
> #define         RISCV_HWPROBE_VENDOR_EXT_XTHEAD3        (3 << 0)
> /* Vendor 2 */
> #define         RISCV_HWPROBE_VENDOR_EXT_XVENDOR1       (1 << 0)
> #define         RISCV_HWPROBE_VENDOR_EXT_XVENDOR2       (2 << 0)
> /* Vendor 3 */
> ...
>
> The keys overlap between vendors. To determine which extension a vendor
> supports, hwprobe gets data from hart_isa_vendor[cpu]. If the vendor is
> vendor 2, it is not possible for a vendor extension from vendor 3 to end
> up in there. Only the extensions from that vendor can be supported by
> that vendor's hardware.

Gotcha. You're right I had misinterpreted this, thinking XTHEADVECTOR
was a valid bit regardless of mvendorid, and that other vendors would
have to choose new bits for their features and always return 0 for
XTHEADVECTOR. With your explanation, it seems like you're allocating
keys (in no particular order) whose meaning will change based on
mvendorid.

I guess I'm still not convinced that saving each vendor from having to
add a VENDOR_EXT key in their keyspace is worth the sacrifice of
spraying the vendor-specific keys across the generic keyspace. Are
there advantages to having a single key whose category is similar but
whose bits are entirely vendor-defined? Maybe if I were userspace and
my feature could be satisfied equivalently by XTHEADVECTOR or
XRIVOSOTHERTHING, then I could do one hwprobe call instead of two? But
I don't think the vendors are going to be consistent enough for that
equivalency to ever prove useful. The advantages in my head of the
separate vendor keyspace are:
 * Keeps the kernel code simple: if key >= (1 >> 63)
vendor_config->do_hwprobe(), rather than having all these little calls
in each specific switch case for vendor_config->do_vendor_ext0(),
vendor_config->do_vendor_ext1(), etc.
 * It extends easily into passing other forms of vendor hwprobe info
later, rather than solving only the case of risc-v extensions now, and
then having to do this all again for each additional category of
vendor data.
 * Similarly, it discourages future vendors from trying to squint and
find a way to make a vaguely generic sounding category for their own
hwprobe key which will ultimately only ever be filled in by them
anyway.

-Evan

Charlie Jenkins April 12, 2024, 10:21 p.m. UTC | #7

On Fri, Apr 12, 2024 at 02:43:01PM -0700, Evan Green wrote:
> On Fri, Apr 12, 2024 at 1:20 PM Charlie Jenkins <charlie@rivosinc.com> wrote:
> >
> > On Fri, Apr 12, 2024 at 12:07:46PM -0700, Evan Green wrote:
> > > On Fri, Apr 12, 2024 at 11:17 AM Charlie Jenkins <charlie@rivosinc.com> wrote:
> > > >
> > > > On Fri, Apr 12, 2024 at 10:05:21AM -0700, Evan Green wrote:
> > > > > On Thu, Apr 11, 2024 at 9:12 PM Charlie Jenkins <charlie@rivosinc.com> wrote:
> > > > > >
> > > > > > Add a new hwprobe key "RISCV_HWPROBE_KEY_VENDOR_EXT_0" which allows
> > > > > > userspace to probe for the new RISCV_ISA_VENDOR_EXT_XTHEADVECTOR vendor
> > > > > > extension.
> > > > > >
> > > > > > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> > > > > > ---
> > > > > >  arch/riscv/include/asm/hwprobe.h      |  4 +--
> > > > > >  arch/riscv/include/uapi/asm/hwprobe.h | 10 +++++-
> > > > > >  arch/riscv/kernel/sys_hwprobe.c       | 59 +++++++++++++++++++++++++++++++++--
> > > > > >  3 files changed, 68 insertions(+), 5 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/uapi/asm/hwprobe.h b/arch/riscv/include/uapi/asm/hwprobe.h
> > > > > > index 9f2a8e3ff204..6614d3adfc75 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,14 @@ 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
> > > > > > +/*
> > > > > > + * It is not possible for one CPU to have multiple vendor ids, so each vendor
> > > > > > + * has its own vendor extension "namespace". The keys for each vendor starts
> > > > > > + * at zero.
> > > > > > + */
> > > > > > +#define RISCV_HWPROBE_KEY_VENDOR_EXT_0 7
> > > > > > + /* T-Head */
> > > > > > +#define                RISCV_HWPROBE_VENDOR_EXT_XTHEADVECTOR   (1 << 0)
> > > > > >  /* Increase RISCV_HWPROBE_MAX_KEY when adding items. */
> > > > > >
> > > > > >  /* Flags */
> > > > > > diff --git a/arch/riscv/kernel/sys_hwprobe.c b/arch/riscv/kernel/sys_hwprobe.c
> > > > > > index e0a42c851511..365ce7380443 100644
> > > > > > --- a/arch/riscv/kernel/sys_hwprobe.c
> > > > > > +++ b/arch/riscv/kernel/sys_hwprobe.c
> > > > > > @@ -69,7 +69,8 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
> > > > > >         if (riscv_isa_extension_available(NULL, c))
> > > > > >                 pair->value |= RISCV_HWPROBE_IMA_C;
> > > > > >
> > > > > > -       if (has_vector() && !riscv_has_vendor_extension_unlikely(RISCV_ISA_VENDOR_EXT_XTHEADVECTOR))
> > > > > > +       if (has_vector() &&
> > > > > > +           !__riscv_isa_vendor_extension_available(NULL, RISCV_ISA_VENDOR_EXT_XTHEADVECTOR))
> > > > > >                 pair->value |= RISCV_HWPROBE_IMA_V;
> > > > > >
> > > > > >         /*
> > > > > > @@ -112,7 +113,8 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
> > > > > >                 EXT_KEY(ZACAS);
> > > > > >                 EXT_KEY(ZICOND);
> > > > > >
> > > > > > -               if (has_vector() && !riscv_has_vendor_extension_unlikely(RISCV_ISA_VENDOR_EXT_XTHEADVECTOR)) {
> > > > > > +               if (has_vector() &&
> > > > > > +                   !riscv_has_vendor_extension_unlikely(RISCV_ISA_VENDOR_EXT_XTHEADVECTOR)) {
> > > > > >                         EXT_KEY(ZVBB);
> > > > > >                         EXT_KEY(ZVBC);
> > > > > >                         EXT_KEY(ZVKB);
> > > > > > @@ -139,6 +141,55 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
> > > > > >         pair->value &= ~missing;
> > > > > >  }
> > > > > >
> > > > > > +static void hwprobe_isa_vendor_ext0(struct riscv_hwprobe *pair,
> > > > > > +                                   const struct cpumask *cpus)
> > > > > > +{
> > > > > > +       int cpu;
> > > > > > +       u64 missing = 0;
> > > > > > +
> > > > > > +       pair->value = 0;
> > > > > > +
> > > > > > +       struct riscv_hwprobe mvendorid = {
> > > > > > +               .key = RISCV_HWPROBE_KEY_MVENDORID,
> > > > > > +               .value = 0
> > > > > > +       };
> > > > > > +
> > > > > > +       hwprobe_arch_id(&mvendorid, cpus);
> > > > > > +
> > > > > > +       /* Set value to zero if CPUs in the set do not have the same vendor. */
> > > > > > +       if (mvendorid.value == -1ULL)
> > > > > > +               return;
> > > > > > +
> > > > > > +       /*
> > > > > > +        * Loop through and record vendor extensions that 1) anyone has, and
> > > > > > +        * 2) anyone doesn't have.
> > > > > > +        */
> > > > > > +       for_each_cpu(cpu, cpus) {
> > > > > > +               struct riscv_isainfo *isavendorinfo = &hart_isa_vendor[cpu];
> > > > > > +
> > > > > > +#define VENDOR_EXT_KEY(ext)                                                            \
> > > > > > +       do {                                                                            \
> > > > > > +               if (__riscv_isa_vendor_extension_available(isavendorinfo->isa,          \
> > > > > > +                                                        RISCV_ISA_VENDOR_EXT_##ext))   \
> > > > > > +                       pair->value |= RISCV_HWPROBE_VENDOR_EXT_##ext;                  \
> > > > > > +               else                                                                    \
> > > > > > +                       missing |= RISCV_HWPROBE_VENDOR_EXT_##ext;                      \
> > > > > > +       } while (false)
> > > > > > +
> > > > > > +       /*
> > > > > > +        * Only use VENDOR_EXT_KEY() for extensions which can be exposed to userspace,
> > > > > > +        * regardless of the kernel's configuration, as no other checks, besides
> > > > > > +        * presence in the hart_vendor_isa bitmap, are made.
> > > > > > +        */
> > > > > > +       VENDOR_EXT_KEY(XTHEADVECTOR);
> > > > > > +
> > > > > > +#undef VENDOR_EXT_KEY
> > > > >
> > > > > Hey Charlie,
> > > > > Thanks for writing this up! At the very least I think the
> > > > > THEAD-specific stuff should probably end up in its own file, otherwise
> > > > > it'll get chaotic with vendors clamoring to add stuff right here.
> > > >
> > > > Great idea!
> > > >
> > > > > What do you think about this approach:
> > > > >  * We leave RISCV_HWPROBE_MAX_KEY as the max key for the "generic
> > > > > world", eg 6-ish
> > > > >  * We define that any key above 0x8000000000000000 is in the vendor
> > > > > space, so the meaning of the keys depends first on the mvendorid
> > > > > value.
> > > > >  * In the kernel code, each new vendor adds on to a global struct,
> > > > > which might look something like:
> > > > > struct hwprobe_vendor_space vendor_space[] = {
> > > > >         {
> > > > >                 .mvendorid = VENDOR_THEAD,
> > > > >                 .max_hwprobe_key = THEAD_MAX_HWPROBE_KEY, // currently
> > > > > 1 or 0x8000000000000001 with what you've got.
> > > > >                 .hwprobe_fn = thead_hwprobe
> > > > >         },
> > > > >         ...
> > > > > };
> > > > >
> > > > >  * A hwprobe_thead.c implements thead_hwprobe(), and is called
> > > > > whenever the generic hwprobe encounters a key >=0x8000000000000000.
> > > > >  * Generic code for setting up the VDSO can then still call the
> > > > > vendor-specific hwprobe_fn() repeatedly with an "all CPUs" mask from
> > > > > the base to max_hwprobe_key and set up the cached tables in userspace.
> > > > >  * Since the VDSO data has limited space we may have to cap the number
> > > > > of vendor keys we cache to be lower than max_hwprobe_key. Since the
> > > > > data itself is not exposed to usermode we can raise this cap later if
> > > > > needed.
> > > >
> > > > I know vendor extensions are kind of the "wild west" of riscv, but in
> > > > spite of that I want to design a consistent API. The issue I had with
> > > > having this "vendor space" for exposing vendor extensions was that this
> > > > is something that is inherently the same for all vendors. I see a vendor
> > > > space like this more applicable for something like
> > > > "RISCV_HWPROBE_KEY_ZICBOZ_BLOCK_SIZE" where a vendor has a specific
> > > > value they would like to expose. I do agree that having a vendor space
> > > > is a good design choice, but I am not convinced that vendor extensions
> > > > are the proper use-case.
> > > >
> > > > By having RISCV_HWPROBE_KEY_VENDOR_EXT_0 we can expose the vendor
> > > > extensions in the same way that standard extensions are exposed, with a
> > > > bitmask representing each extension. If these are instead in the vendor
> > > > space, each vendor would probably be inclined to introduce a key like
> > > > RISCV_HWPROBE_KEY_THEAD_EXT_0 that returns a bitmask of all of the thead
> > > > vendor extensions. This duplicated effort is what I am trying to avoid.
> > > > The alternative would be that vendors have a separate key for each
> > > > vendor extension they would like to expose, but that is strictly less
> > > > efficient than the existing bitmask probing.
> > > >
> > > > Do you think that having the vendor space is appropriate for vendor
> > > > extensions given my concerns?
> > >
> > > I do see what you're going for. It's tidy for a bitmask to just let
> > > anyone allocate the next bit, but leaves you with the same problem
> > > when a vendor decides they want to expose an enum, or decides they
> > > want to expose a bazillion things. I think a generalized version of
> >
> > This patch is strictly to expose if a vendor extension is supported,
> > how does exposing enums factor in here?
> >
> > > the approach you've written would be: simply let vendors allocate keys
> > > from the same global space we're already using. My worry was that it
> >
> > I am missing how my proposal suggests allowing vendors to allocate keys
> > in a global space.
> >
> > > would turn into an expansive suburban sprawl of mostly dead bits, or
> > > in the case of vendor-specific keys, full of "if (mvendor_id() !=
> > > MINE) return 0;". My hope with the vendored keyspace is it would keep
> >
> > An application will always need to check vendorid before calling hwprobe
> > with a vendor-specific feature? If that hwprobe support is a key above
> > 1<<63, then the application will need to pass that vendor-specific key
> > and interpret the vendor-specific value. If that hwprobe support is what
> > I have proposed here, then the user calls the standardized vendor
> > extension hwprobe endpoint and then needs to interpret the result based
> > on the vendor of the cpumask. In both cases they need to check the
> > vendorid of the cpumask. In the test case I added I failed to check the
> > vendorid but I should have had that.
> >
> > > the sprawl from polluting the general array of (hopefully valuable)
> > > info with stuff that's likely to become less relevant as time passes.
> > > It also lowers the bar a bit to make it easier for vendors to expose
> > > bits, as they don't consume global space for everyone for all of time,
> > > just themselves.
> >
> > The vendor keys are tied directly to the vendor. So as it grows we would
> > have something like:
> >
> > #define RISCV_HWPROBE_KEY_VENDOR_EXT_0  7
> > /* T-Head */
> > #define         RISCV_HWPROBE_VENDOR_EXT_XTHEADVECTOR   (1 << 0)
> > #define         RISCV_HWPROBE_VENDOR_EXT_XTHEAD2        (2 << 0)
> > #define         RISCV_HWPROBE_VENDOR_EXT_XTHEAD3        (3 << 0)
> > /* Vendor 2 */
> > #define         RISCV_HWPROBE_VENDOR_EXT_XVENDOR1       (1 << 0)
> > #define         RISCV_HWPROBE_VENDOR_EXT_XVENDOR2       (2 << 0)
> > /* Vendor 3 */
> > ...
> >
> > The keys overlap between vendors. To determine which extension a vendor
> > supports, hwprobe gets data from hart_isa_vendor[cpu]. If the vendor is
> > vendor 2, it is not possible for a vendor extension from vendor 3 to end
> > up in there. Only the extensions from that vendor can be supported by
> > that vendor's hardware.
> 
> Gotcha. You're right I had misinterpreted this, thinking XTHEADVECTOR
> was a valid bit regardless of mvendorid, and that other vendors would
> have to choose new bits for their features and always return 0 for
> XTHEADVECTOR. With your explanation, it seems like you're allocating
> keys (in no particular order) whose meaning will change based on
> mvendorid.
> 
> I guess I'm still not convinced that saving each vendor from having to
> add a VENDOR_EXT key in their keyspace is worth the sacrifice of
> spraying the vendor-specific keys across the generic keyspace. Are
> there advantages to having a single key whose category is similar but
> whose bits are entirely vendor-defined? Maybe if I were userspace and
> my feature could be satisfied equivalently by XTHEADVECTOR or
> XRIVOSOTHERTHING, then I could do one hwprobe call instead of two? But
> I don't think the vendors are going to be consistent enough for that
> equivalency to ever prove useful. The advantages in my head of the
> separate vendor keyspace are:
>  * Keeps the kernel code simple: if key >= (1 >> 63)
> vendor_config->do_hwprobe(), rather than having all these little calls
> in each specific switch case for vendor_config->do_vendor_ext0(),
> vendor_config->do_vendor_ext1(), etc.

The consistency between vendors is guaranteed in this scheme. They just
add the extension to hwprobe_isa_vendor_ext0. The following code is the
critical code from the kernel:

	for_each_cpu(cpu, cpus) {
		struct riscv_isainfo *isavendorinfo = &hart_isa_vendor[cpu];

#define VENDOR_EXT_KEY(ext)								\
	do {										\
		if (__riscv_isa_vendor_extension_available(isavendorinfo->isa,		\
							 RISCV_ISA_VENDOR_EXT_##ext))	\
			pair->value |= RISCV_HWPROBE_VENDOR_EXT_##ext;			\
		else									\
			missing |= RISCV_HWPROBE_VENDOR_EXT_##ext;			\
	} while (false)

	/*
	 * Only use VENDOR_EXT_KEY() for extensions which can be exposed to userspace,
	 * regardless of the kernel's configuration, as no other checks, besides
	 * presence in the hart_vendor_isa bitmap, are made.
	 */
	VENDOR_EXT_KEY(XTHEADVECTOR);

#undef VENDOR_EXT_KEY
	}

	/* Now turn off reporting features if any CPU is missing it. */
	pair->value &= ~missing;

The only thing a vendor will have to do is add an entry below
VENDOR_EXT_KEY(XTHEADVECTOR) with their extension name (of course
populating a value for the key as well). This existing code will then
check if the extension is compatible with the hardware and appropriate
populate the bitmask. All vendors get this functionality for "free"
without needing to write the boilerplate code to expose vendor
extensions through hwprobe.

Now that I write this out I do see that I overlooked that this code
needs to check the vendorid to ensure that the given extension is
actually associated with the vendorid. This would make this more
complicated but still seems like a low barrier to entry for a new
vendor, as well as a standard API for getting all vendor extensions that
are available on the platform regardless of which platform is being
used.

>  * It extends easily into passing other forms of vendor hwprobe info
> later, rather than solving only the case of risc-v extensions now, and
> then having to do this all again for each additional category of
> vendor data.

This is a great point. I do agree that a different solution will be
necessary for arbitrary vendor data and I am all for making something
future compatible. At the same time I don't want to get trapped into
something that is suboptimal for the sake of doing less work later.
There is no chance of any compatibility once we leave the realm of
riscv extensions, so once a vendor needs something exported I would be
happy to write the code to support that. 

>  * Similarly, it discourages future vendors from trying to squint and
> find a way to make a vaguely generic sounding category for their own
> hwprobe key which will ultimately only ever be filled in by them
> anyway.

What do you mean by this? There are no "categories" here, the vendor
just writes out their extension VENDOR_EXT_KEY(XVENDOREXTENSION) and it
gets shuttled to userspace on the hwprobe vendor call.

- Charlie

> 
> -Evan

Evan Green April 12, 2024, 10:50 p.m. UTC | #8

On Fri, Apr 12, 2024 at 3:21 PM Charlie Jenkins <charlie@rivosinc.com> wrote:
>
> On Fri, Apr 12, 2024 at 02:43:01PM -0700, Evan Green wrote:
> > On Fri, Apr 12, 2024 at 1:20 PM Charlie Jenkins <charlie@rivosinc.com> wrote:
> > >
> > > On Fri, Apr 12, 2024 at 12:07:46PM -0700, Evan Green wrote:
> > > > On Fri, Apr 12, 2024 at 11:17 AM Charlie Jenkins <charlie@rivosinc.com> wrote:
> > > > >
> > > > > On Fri, Apr 12, 2024 at 10:05:21AM -0700, Evan Green wrote:
> > > > > > On Thu, Apr 11, 2024 at 9:12 PM Charlie Jenkins <charlie@rivosinc.com> wrote:
> > > > > > >
> > > > > > > Add a new hwprobe key "RISCV_HWPROBE_KEY_VENDOR_EXT_0" which allows
> > > > > > > userspace to probe for the new RISCV_ISA_VENDOR_EXT_XTHEADVECTOR vendor
> > > > > > > extension.
> > > > > > >
> > > > > > > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> > > > > > > ---
> > > > > > >  arch/riscv/include/asm/hwprobe.h      |  4 +--
> > > > > > >  arch/riscv/include/uapi/asm/hwprobe.h | 10 +++++-
> > > > > > >  arch/riscv/kernel/sys_hwprobe.c       | 59 +++++++++++++++++++++++++++++++++--
> > > > > > >  3 files changed, 68 insertions(+), 5 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/uapi/asm/hwprobe.h b/arch/riscv/include/uapi/asm/hwprobe.h
> > > > > > > index 9f2a8e3ff204..6614d3adfc75 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,14 @@ 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
> > > > > > > +/*
> > > > > > > + * It is not possible for one CPU to have multiple vendor ids, so each vendor
> > > > > > > + * has its own vendor extension "namespace". The keys for each vendor starts
> > > > > > > + * at zero.
> > > > > > > + */
> > > > > > > +#define RISCV_HWPROBE_KEY_VENDOR_EXT_0 7
> > > > > > > + /* T-Head */
> > > > > > > +#define                RISCV_HWPROBE_VENDOR_EXT_XTHEADVECTOR   (1 << 0)
> > > > > > >  /* Increase RISCV_HWPROBE_MAX_KEY when adding items. */
> > > > > > >
> > > > > > >  /* Flags */
> > > > > > > diff --git a/arch/riscv/kernel/sys_hwprobe.c b/arch/riscv/kernel/sys_hwprobe.c
> > > > > > > index e0a42c851511..365ce7380443 100644
> > > > > > > --- a/arch/riscv/kernel/sys_hwprobe.c
> > > > > > > +++ b/arch/riscv/kernel/sys_hwprobe.c
> > > > > > > @@ -69,7 +69,8 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
> > > > > > >         if (riscv_isa_extension_available(NULL, c))
> > > > > > >                 pair->value |= RISCV_HWPROBE_IMA_C;
> > > > > > >
> > > > > > > -       if (has_vector() && !riscv_has_vendor_extension_unlikely(RISCV_ISA_VENDOR_EXT_XTHEADVECTOR))
> > > > > > > +       if (has_vector() &&
> > > > > > > +           !__riscv_isa_vendor_extension_available(NULL, RISCV_ISA_VENDOR_EXT_XTHEADVECTOR))
> > > > > > >                 pair->value |= RISCV_HWPROBE_IMA_V;
> > > > > > >
> > > > > > >         /*
> > > > > > > @@ -112,7 +113,8 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
> > > > > > >                 EXT_KEY(ZACAS);
> > > > > > >                 EXT_KEY(ZICOND);
> > > > > > >
> > > > > > > -               if (has_vector() && !riscv_has_vendor_extension_unlikely(RISCV_ISA_VENDOR_EXT_XTHEADVECTOR)) {
> > > > > > > +               if (has_vector() &&
> > > > > > > +                   !riscv_has_vendor_extension_unlikely(RISCV_ISA_VENDOR_EXT_XTHEADVECTOR)) {
> > > > > > >                         EXT_KEY(ZVBB);
> > > > > > >                         EXT_KEY(ZVBC);
> > > > > > >                         EXT_KEY(ZVKB);
> > > > > > > @@ -139,6 +141,55 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
> > > > > > >         pair->value &= ~missing;
> > > > > > >  }
> > > > > > >
> > > > > > > +static void hwprobe_isa_vendor_ext0(struct riscv_hwprobe *pair,
> > > > > > > +                                   const struct cpumask *cpus)
> > > > > > > +{
> > > > > > > +       int cpu;
> > > > > > > +       u64 missing = 0;
> > > > > > > +
> > > > > > > +       pair->value = 0;
> > > > > > > +
> > > > > > > +       struct riscv_hwprobe mvendorid = {
> > > > > > > +               .key = RISCV_HWPROBE_KEY_MVENDORID,
> > > > > > > +               .value = 0
> > > > > > > +       };
> > > > > > > +
> > > > > > > +       hwprobe_arch_id(&mvendorid, cpus);
> > > > > > > +
> > > > > > > +       /* Set value to zero if CPUs in the set do not have the same vendor. */
> > > > > > > +       if (mvendorid.value == -1ULL)
> > > > > > > +               return;
> > > > > > > +
> > > > > > > +       /*
> > > > > > > +        * Loop through and record vendor extensions that 1) anyone has, and
> > > > > > > +        * 2) anyone doesn't have.
> > > > > > > +        */
> > > > > > > +       for_each_cpu(cpu, cpus) {
> > > > > > > +               struct riscv_isainfo *isavendorinfo = &hart_isa_vendor[cpu];
> > > > > > > +
> > > > > > > +#define VENDOR_EXT_KEY(ext)                                                            \
> > > > > > > +       do {                                                                            \
> > > > > > > +               if (__riscv_isa_vendor_extension_available(isavendorinfo->isa,          \
> > > > > > > +                                                        RISCV_ISA_VENDOR_EXT_##ext))   \
> > > > > > > +                       pair->value |= RISCV_HWPROBE_VENDOR_EXT_##ext;                  \
> > > > > > > +               else                                                                    \
> > > > > > > +                       missing |= RISCV_HWPROBE_VENDOR_EXT_##ext;                      \
> > > > > > > +       } while (false)
> > > > > > > +
> > > > > > > +       /*
> > > > > > > +        * Only use VENDOR_EXT_KEY() for extensions which can be exposed to userspace,
> > > > > > > +        * regardless of the kernel's configuration, as no other checks, besides
> > > > > > > +        * presence in the hart_vendor_isa bitmap, are made.
> > > > > > > +        */
> > > > > > > +       VENDOR_EXT_KEY(XTHEADVECTOR);
> > > > > > > +
> > > > > > > +#undef VENDOR_EXT_KEY
> > > > > >
> > > > > > Hey Charlie,
> > > > > > Thanks for writing this up! At the very least I think the
> > > > > > THEAD-specific stuff should probably end up in its own file, otherwise
> > > > > > it'll get chaotic with vendors clamoring to add stuff right here.
> > > > >
> > > > > Great idea!
> > > > >
> > > > > > What do you think about this approach:
> > > > > >  * We leave RISCV_HWPROBE_MAX_KEY as the max key for the "generic
> > > > > > world", eg 6-ish
> > > > > >  * We define that any key above 0x8000000000000000 is in the vendor
> > > > > > space, so the meaning of the keys depends first on the mvendorid
> > > > > > value.
> > > > > >  * In the kernel code, each new vendor adds on to a global struct,
> > > > > > which might look something like:
> > > > > > struct hwprobe_vendor_space vendor_space[] = {
> > > > > >         {
> > > > > >                 .mvendorid = VENDOR_THEAD,
> > > > > >                 .max_hwprobe_key = THEAD_MAX_HWPROBE_KEY, // currently
> > > > > > 1 or 0x8000000000000001 with what you've got.
> > > > > >                 .hwprobe_fn = thead_hwprobe
> > > > > >         },
> > > > > >         ...
> > > > > > };
> > > > > >
> > > > > >  * A hwprobe_thead.c implements thead_hwprobe(), and is called
> > > > > > whenever the generic hwprobe encounters a key >=0x8000000000000000.
> > > > > >  * Generic code for setting up the VDSO can then still call the
> > > > > > vendor-specific hwprobe_fn() repeatedly with an "all CPUs" mask from
> > > > > > the base to max_hwprobe_key and set up the cached tables in userspace.
> > > > > >  * Since the VDSO data has limited space we may have to cap the number
> > > > > > of vendor keys we cache to be lower than max_hwprobe_key. Since the
> > > > > > data itself is not exposed to usermode we can raise this cap later if
> > > > > > needed.
> > > > >
> > > > > I know vendor extensions are kind of the "wild west" of riscv, but in
> > > > > spite of that I want to design a consistent API. The issue I had with
> > > > > having this "vendor space" for exposing vendor extensions was that this
> > > > > is something that is inherently the same for all vendors. I see a vendor
> > > > > space like this more applicable for something like
> > > > > "RISCV_HWPROBE_KEY_ZICBOZ_BLOCK_SIZE" where a vendor has a specific
> > > > > value they would like to expose. I do agree that having a vendor space
> > > > > is a good design choice, but I am not convinced that vendor extensions
> > > > > are the proper use-case.
> > > > >
> > > > > By having RISCV_HWPROBE_KEY_VENDOR_EXT_0 we can expose the vendor
> > > > > extensions in the same way that standard extensions are exposed, with a
> > > > > bitmask representing each extension. If these are instead in the vendor
> > > > > space, each vendor would probably be inclined to introduce a key like
> > > > > RISCV_HWPROBE_KEY_THEAD_EXT_0 that returns a bitmask of all of the thead
> > > > > vendor extensions. This duplicated effort is what I am trying to avoid.
> > > > > The alternative would be that vendors have a separate key for each
> > > > > vendor extension they would like to expose, but that is strictly less
> > > > > efficient than the existing bitmask probing.
> > > > >
> > > > > Do you think that having the vendor space is appropriate for vendor
> > > > > extensions given my concerns?
> > > >
> > > > I do see what you're going for. It's tidy for a bitmask to just let
> > > > anyone allocate the next bit, but leaves you with the same problem
> > > > when a vendor decides they want to expose an enum, or decides they
> > > > want to expose a bazillion things. I think a generalized version of
> > >
> > > This patch is strictly to expose if a vendor extension is supported,
> > > how does exposing enums factor in here?
> > >
> > > > the approach you've written would be: simply let vendors allocate keys
> > > > from the same global space we're already using. My worry was that it
> > >
> > > I am missing how my proposal suggests allowing vendors to allocate keys
> > > in a global space.
> > >
> > > > would turn into an expansive suburban sprawl of mostly dead bits, or
> > > > in the case of vendor-specific keys, full of "if (mvendor_id() !=
> > > > MINE) return 0;". My hope with the vendored keyspace is it would keep
> > >
> > > An application will always need to check vendorid before calling hwprobe
> > > with a vendor-specific feature? If that hwprobe support is a key above
> > > 1<<63, then the application will need to pass that vendor-specific key
> > > and interpret the vendor-specific value. If that hwprobe support is what
> > > I have proposed here, then the user calls the standardized vendor
> > > extension hwprobe endpoint and then needs to interpret the result based
> > > on the vendor of the cpumask. In both cases they need to check the
> > > vendorid of the cpumask. In the test case I added I failed to check the
> > > vendorid but I should have had that.
> > >
> > > > the sprawl from polluting the general array of (hopefully valuable)
> > > > info with stuff that's likely to become less relevant as time passes.
> > > > It also lowers the bar a bit to make it easier for vendors to expose
> > > > bits, as they don't consume global space for everyone for all of time,
> > > > just themselves.
> > >
> > > The vendor keys are tied directly to the vendor. So as it grows we would
> > > have something like:
> > >
> > > #define RISCV_HWPROBE_KEY_VENDOR_EXT_0  7
> > > /* T-Head */
> > > #define         RISCV_HWPROBE_VENDOR_EXT_XTHEADVECTOR   (1 << 0)
> > > #define         RISCV_HWPROBE_VENDOR_EXT_XTHEAD2        (2 << 0)
> > > #define         RISCV_HWPROBE_VENDOR_EXT_XTHEAD3        (3 << 0)
> > > /* Vendor 2 */
> > > #define         RISCV_HWPROBE_VENDOR_EXT_XVENDOR1       (1 << 0)
> > > #define         RISCV_HWPROBE_VENDOR_EXT_XVENDOR2       (2 << 0)
> > > /* Vendor 3 */
> > > ...
> > >
> > > The keys overlap between vendors. To determine which extension a vendor
> > > supports, hwprobe gets data from hart_isa_vendor[cpu]. If the vendor is
> > > vendor 2, it is not possible for a vendor extension from vendor 3 to end
> > > up in there. Only the extensions from that vendor can be supported by
> > > that vendor's hardware.
> >
> > Gotcha. You're right I had misinterpreted this, thinking XTHEADVECTOR
> > was a valid bit regardless of mvendorid, and that other vendors would
> > have to choose new bits for their features and always return 0 for
> > XTHEADVECTOR. With your explanation, it seems like you're allocating
> > keys (in no particular order) whose meaning will change based on
> > mvendorid.
> >
> > I guess I'm still not convinced that saving each vendor from having to
> > add a VENDOR_EXT key in their keyspace is worth the sacrifice of
> > spraying the vendor-specific keys across the generic keyspace. Are
> > there advantages to having a single key whose category is similar but
> > whose bits are entirely vendor-defined? Maybe if I were userspace and
> > my feature could be satisfied equivalently by XTHEADVECTOR or
> > XRIVOSOTHERTHING, then I could do one hwprobe call instead of two? But
> > I don't think the vendors are going to be consistent enough for that
> > equivalency to ever prove useful. The advantages in my head of the
> > separate vendor keyspace are:
> >  * Keeps the kernel code simple: if key >= (1 >> 63)
> > vendor_config->do_hwprobe(), rather than having all these little calls
> > in each specific switch case for vendor_config->do_vendor_ext0(),
> > vendor_config->do_vendor_ext1(), etc.
>
> The consistency between vendors is guaranteed in this scheme. They just
> add the extension to hwprobe_isa_vendor_ext0. The following code is the
> critical code from the kernel:
>
>         for_each_cpu(cpu, cpus) {
>                 struct riscv_isainfo *isavendorinfo = &hart_isa_vendor[cpu];
>
> #define VENDOR_EXT_KEY(ext)                                                             \
>         do {                                                                            \
>                 if (__riscv_isa_vendor_extension_available(isavendorinfo->isa,          \
>                                                          RISCV_ISA_VENDOR_EXT_##ext))   \
>                         pair->value |= RISCV_HWPROBE_VENDOR_EXT_##ext;                  \
>                 else                                                                    \
>                         missing |= RISCV_HWPROBE_VENDOR_EXT_##ext;                      \
>         } while (false)
>
>         /*
>          * Only use VENDOR_EXT_KEY() for extensions which can be exposed to userspace,
>          * regardless of the kernel's configuration, as no other checks, besides
>          * presence in the hart_vendor_isa bitmap, are made.
>          */
>         VENDOR_EXT_KEY(XTHEADVECTOR);
>
> #undef VENDOR_EXT_KEY
>         }
>
>         /* Now turn off reporting features if any CPU is missing it. */
>         pair->value &= ~missing;
>
> The only thing a vendor will have to do is add an entry below
> VENDOR_EXT_KEY(XTHEADVECTOR) with their extension name (of course
> populating a value for the key as well). This existing code will then
> check if the extension is compatible with the hardware and appropriate
> populate the bitmask. All vendors get this functionality for "free"
> without needing to write the boilerplate code to expose vendor
> extensions through hwprobe.
>
> Now that I write this out I do see that I overlooked that this code
> needs to check the vendorid to ensure that the given extension is
> actually associated with the vendorid. This would make this more
> complicated but still seems like a low barrier to entry for a new
> vendor, as well as a standard API for getting all vendor extensions that
> are available on the platform regardless of which platform is being
> used.
>

Maybe I'll reserve judgment until I see the next spin, since we need
both the "conditionalize on mvendorid" part, and to move the vendor
stuff into a thead-specific file as discussed earlier. I'll be trying
to picture how this looks 10 years from now, when a bunch of vendors
have added dozens of extensions, and 75% of them are at that point
defunct baggage.

> >  * It extends easily into passing other forms of vendor hwprobe info
> > later, rather than solving only the case of risc-v extensions now, and
> > then having to do this all again for each additional category of
> > vendor data.
>
> This is a great point. I do agree that a different solution will be
> necessary for arbitrary vendor data and I am all for making something
> future compatible. At the same time I don't want to get trapped into
> something that is suboptimal for the sake of doing less work later.
> There is no chance of any compatibility once we leave the realm of
> riscv extensions, so once a vendor needs something exported I would be
> happy to write the code to support that.
>
> >  * Similarly, it discourages future vendors from trying to squint and
> > find a way to make a vaguely generic sounding category for their own
> > hwprobe key which will ultimately only ever be filled in by them
> > anyway.
>
> What do you mean by this? There are no "categories" here, the vendor
> just writes out their extension VENDOR_EXT_KEY(XVENDOREXTENSION) and it
> gets shuttled to userspace on the hwprobe vendor call.

The category in this case is RISC-V extensions, since you've defined a
key whose contents are vendor-specific, but whose bits must all fit
the category of being a risc-v vendor extension.

To frame it in another light, one equivalent version from an ABI
perspective would be to say ok, let's put this key up into the 1<<63
range, but carve out a "common key" range where all vendors implement
the same key definitions, like this VENDOR_EXT_0 key. Is that useful,
or is it unnecessary structure? I think I'm of the opinion it's
unnecessary structure, but I'm still open to being convinced.
-Evan

>
> - Charlie
>
> >
> > -Evan

Charlie Jenkins April 12, 2024, 11:12 p.m. UTC | #9

On Fri, Apr 12, 2024 at 03:50:05PM -0700, Evan Green wrote:
> On Fri, Apr 12, 2024 at 3:21 PM Charlie Jenkins <charlie@rivosinc.com> wrote:
> >
> > On Fri, Apr 12, 2024 at 02:43:01PM -0700, Evan Green wrote:
> > > On Fri, Apr 12, 2024 at 1:20 PM Charlie Jenkins <charlie@rivosinc.com> wrote:
> > > >
> > > > On Fri, Apr 12, 2024 at 12:07:46PM -0700, Evan Green wrote:
> > > > > On Fri, Apr 12, 2024 at 11:17 AM Charlie Jenkins <charlie@rivosinc.com> wrote:
> > > > > >
> > > > > > On Fri, Apr 12, 2024 at 10:05:21AM -0700, Evan Green wrote:
> > > > > > > On Thu, Apr 11, 2024 at 9:12 PM Charlie Jenkins <charlie@rivosinc.com> wrote:
> > > > > > > >
> > > > > > > > Add a new hwprobe key "RISCV_HWPROBE_KEY_VENDOR_EXT_0" which allows
> > > > > > > > userspace to probe for the new RISCV_ISA_VENDOR_EXT_XTHEADVECTOR vendor
> > > > > > > > extension.
> > > > > > > >
> > > > > > > > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> > > > > > > > ---
> > > > > > > >  arch/riscv/include/asm/hwprobe.h      |  4 +--
> > > > > > > >  arch/riscv/include/uapi/asm/hwprobe.h | 10 +++++-
> > > > > > > >  arch/riscv/kernel/sys_hwprobe.c       | 59 +++++++++++++++++++++++++++++++++--
> > > > > > > >  3 files changed, 68 insertions(+), 5 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/uapi/asm/hwprobe.h b/arch/riscv/include/uapi/asm/hwprobe.h
> > > > > > > > index 9f2a8e3ff204..6614d3adfc75 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,14 @@ 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
> > > > > > > > +/*
> > > > > > > > + * It is not possible for one CPU to have multiple vendor ids, so each vendor
> > > > > > > > + * has its own vendor extension "namespace". The keys for each vendor starts
> > > > > > > > + * at zero.
> > > > > > > > + */
> > > > > > > > +#define RISCV_HWPROBE_KEY_VENDOR_EXT_0 7
> > > > > > > > + /* T-Head */
> > > > > > > > +#define                RISCV_HWPROBE_VENDOR_EXT_XTHEADVECTOR   (1 << 0)
> > > > > > > >  /* Increase RISCV_HWPROBE_MAX_KEY when adding items. */
> > > > > > > >
> > > > > > > >  /* Flags */
> > > > > > > > diff --git a/arch/riscv/kernel/sys_hwprobe.c b/arch/riscv/kernel/sys_hwprobe.c
> > > > > > > > index e0a42c851511..365ce7380443 100644
> > > > > > > > --- a/arch/riscv/kernel/sys_hwprobe.c
> > > > > > > > +++ b/arch/riscv/kernel/sys_hwprobe.c
> > > > > > > > @@ -69,7 +69,8 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
> > > > > > > >         if (riscv_isa_extension_available(NULL, c))
> > > > > > > >                 pair->value |= RISCV_HWPROBE_IMA_C;
> > > > > > > >
> > > > > > > > -       if (has_vector() && !riscv_has_vendor_extension_unlikely(RISCV_ISA_VENDOR_EXT_XTHEADVECTOR))
> > > > > > > > +       if (has_vector() &&
> > > > > > > > +           !__riscv_isa_vendor_extension_available(NULL, RISCV_ISA_VENDOR_EXT_XTHEADVECTOR))
> > > > > > > >                 pair->value |= RISCV_HWPROBE_IMA_V;
> > > > > > > >
> > > > > > > >         /*
> > > > > > > > @@ -112,7 +113,8 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
> > > > > > > >                 EXT_KEY(ZACAS);
> > > > > > > >                 EXT_KEY(ZICOND);
> > > > > > > >
> > > > > > > > -               if (has_vector() && !riscv_has_vendor_extension_unlikely(RISCV_ISA_VENDOR_EXT_XTHEADVECTOR)) {
> > > > > > > > +               if (has_vector() &&
> > > > > > > > +                   !riscv_has_vendor_extension_unlikely(RISCV_ISA_VENDOR_EXT_XTHEADVECTOR)) {
> > > > > > > >                         EXT_KEY(ZVBB);
> > > > > > > >                         EXT_KEY(ZVBC);
> > > > > > > >                         EXT_KEY(ZVKB);
> > > > > > > > @@ -139,6 +141,55 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
> > > > > > > >         pair->value &= ~missing;
> > > > > > > >  }
> > > > > > > >
> > > > > > > > +static void hwprobe_isa_vendor_ext0(struct riscv_hwprobe *pair,
> > > > > > > > +                                   const struct cpumask *cpus)
> > > > > > > > +{
> > > > > > > > +       int cpu;
> > > > > > > > +       u64 missing = 0;
> > > > > > > > +
> > > > > > > > +       pair->value = 0;
> > > > > > > > +
> > > > > > > > +       struct riscv_hwprobe mvendorid = {
> > > > > > > > +               .key = RISCV_HWPROBE_KEY_MVENDORID,
> > > > > > > > +               .value = 0
> > > > > > > > +       };
> > > > > > > > +
> > > > > > > > +       hwprobe_arch_id(&mvendorid, cpus);
> > > > > > > > +
> > > > > > > > +       /* Set value to zero if CPUs in the set do not have the same vendor. */
> > > > > > > > +       if (mvendorid.value == -1ULL)
> > > > > > > > +               return;
> > > > > > > > +
> > > > > > > > +       /*
> > > > > > > > +        * Loop through and record vendor extensions that 1) anyone has, and
> > > > > > > > +        * 2) anyone doesn't have.
> > > > > > > > +        */
> > > > > > > > +       for_each_cpu(cpu, cpus) {
> > > > > > > > +               struct riscv_isainfo *isavendorinfo = &hart_isa_vendor[cpu];
> > > > > > > > +
> > > > > > > > +#define VENDOR_EXT_KEY(ext)                                                            \
> > > > > > > > +       do {                                                                            \
> > > > > > > > +               if (__riscv_isa_vendor_extension_available(isavendorinfo->isa,          \
> > > > > > > > +                                                        RISCV_ISA_VENDOR_EXT_##ext))   \
> > > > > > > > +                       pair->value |= RISCV_HWPROBE_VENDOR_EXT_##ext;                  \
> > > > > > > > +               else                                                                    \
> > > > > > > > +                       missing |= RISCV_HWPROBE_VENDOR_EXT_##ext;                      \
> > > > > > > > +       } while (false)
> > > > > > > > +
> > > > > > > > +       /*
> > > > > > > > +        * Only use VENDOR_EXT_KEY() for extensions which can be exposed to userspace,
> > > > > > > > +        * regardless of the kernel's configuration, as no other checks, besides
> > > > > > > > +        * presence in the hart_vendor_isa bitmap, are made.
> > > > > > > > +        */
> > > > > > > > +       VENDOR_EXT_KEY(XTHEADVECTOR);
> > > > > > > > +
> > > > > > > > +#undef VENDOR_EXT_KEY
> > > > > > >
> > > > > > > Hey Charlie,
> > > > > > > Thanks for writing this up! At the very least I think the
> > > > > > > THEAD-specific stuff should probably end up in its own file, otherwise
> > > > > > > it'll get chaotic with vendors clamoring to add stuff right here.
> > > > > >
> > > > > > Great idea!
> > > > > >
> > > > > > > What do you think about this approach:
> > > > > > >  * We leave RISCV_HWPROBE_MAX_KEY as the max key for the "generic
> > > > > > > world", eg 6-ish
> > > > > > >  * We define that any key above 0x8000000000000000 is in the vendor
> > > > > > > space, so the meaning of the keys depends first on the mvendorid
> > > > > > > value.
> > > > > > >  * In the kernel code, each new vendor adds on to a global struct,
> > > > > > > which might look something like:
> > > > > > > struct hwprobe_vendor_space vendor_space[] = {
> > > > > > >         {
> > > > > > >                 .mvendorid = VENDOR_THEAD,
> > > > > > >                 .max_hwprobe_key = THEAD_MAX_HWPROBE_KEY, // currently
> > > > > > > 1 or 0x8000000000000001 with what you've got.
> > > > > > >                 .hwprobe_fn = thead_hwprobe
> > > > > > >         },
> > > > > > >         ...
> > > > > > > };
> > > > > > >
> > > > > > >  * A hwprobe_thead.c implements thead_hwprobe(), and is called
> > > > > > > whenever the generic hwprobe encounters a key >=0x8000000000000000.
> > > > > > >  * Generic code for setting up the VDSO can then still call the
> > > > > > > vendor-specific hwprobe_fn() repeatedly with an "all CPUs" mask from
> > > > > > > the base to max_hwprobe_key and set up the cached tables in userspace.
> > > > > > >  * Since the VDSO data has limited space we may have to cap the number
> > > > > > > of vendor keys we cache to be lower than max_hwprobe_key. Since the
> > > > > > > data itself is not exposed to usermode we can raise this cap later if
> > > > > > > needed.
> > > > > >
> > > > > > I know vendor extensions are kind of the "wild west" of riscv, but in
> > > > > > spite of that I want to design a consistent API. The issue I had with
> > > > > > having this "vendor space" for exposing vendor extensions was that this
> > > > > > is something that is inherently the same for all vendors. I see a vendor
> > > > > > space like this more applicable for something like
> > > > > > "RISCV_HWPROBE_KEY_ZICBOZ_BLOCK_SIZE" where a vendor has a specific
> > > > > > value they would like to expose. I do agree that having a vendor space
> > > > > > is a good design choice, but I am not convinced that vendor extensions
> > > > > > are the proper use-case.
> > > > > >
> > > > > > By having RISCV_HWPROBE_KEY_VENDOR_EXT_0 we can expose the vendor
> > > > > > extensions in the same way that standard extensions are exposed, with a
> > > > > > bitmask representing each extension. If these are instead in the vendor
> > > > > > space, each vendor would probably be inclined to introduce a key like
> > > > > > RISCV_HWPROBE_KEY_THEAD_EXT_0 that returns a bitmask of all of the thead
> > > > > > vendor extensions. This duplicated effort is what I am trying to avoid.
> > > > > > The alternative would be that vendors have a separate key for each
> > > > > > vendor extension they would like to expose, but that is strictly less
> > > > > > efficient than the existing bitmask probing.
> > > > > >
> > > > > > Do you think that having the vendor space is appropriate for vendor
> > > > > > extensions given my concerns?
> > > > >
> > > > > I do see what you're going for. It's tidy for a bitmask to just let
> > > > > anyone allocate the next bit, but leaves you with the same problem
> > > > > when a vendor decides they want to expose an enum, or decides they
> > > > > want to expose a bazillion things. I think a generalized version of
> > > >
> > > > This patch is strictly to expose if a vendor extension is supported,
> > > > how does exposing enums factor in here?
> > > >
> > > > > the approach you've written would be: simply let vendors allocate keys
> > > > > from the same global space we're already using. My worry was that it
> > > >
> > > > I am missing how my proposal suggests allowing vendors to allocate keys
> > > > in a global space.
> > > >
> > > > > would turn into an expansive suburban sprawl of mostly dead bits, or
> > > > > in the case of vendor-specific keys, full of "if (mvendor_id() !=
> > > > > MINE) return 0;". My hope with the vendored keyspace is it would keep
> > > >
> > > > An application will always need to check vendorid before calling hwprobe
> > > > with a vendor-specific feature? If that hwprobe support is a key above
> > > > 1<<63, then the application will need to pass that vendor-specific key
> > > > and interpret the vendor-specific value. If that hwprobe support is what
> > > > I have proposed here, then the user calls the standardized vendor
> > > > extension hwprobe endpoint and then needs to interpret the result based
> > > > on the vendor of the cpumask. In both cases they need to check the
> > > > vendorid of the cpumask. In the test case I added I failed to check the
> > > > vendorid but I should have had that.
> > > >
> > > > > the sprawl from polluting the general array of (hopefully valuable)
> > > > > info with stuff that's likely to become less relevant as time passes.
> > > > > It also lowers the bar a bit to make it easier for vendors to expose
> > > > > bits, as they don't consume global space for everyone for all of time,
> > > > > just themselves.
> > > >
> > > > The vendor keys are tied directly to the vendor. So as it grows we would
> > > > have something like:
> > > >
> > > > #define RISCV_HWPROBE_KEY_VENDOR_EXT_0  7
> > > > /* T-Head */
> > > > #define         RISCV_HWPROBE_VENDOR_EXT_XTHEADVECTOR   (1 << 0)
> > > > #define         RISCV_HWPROBE_VENDOR_EXT_XTHEAD2        (2 << 0)
> > > > #define         RISCV_HWPROBE_VENDOR_EXT_XTHEAD3        (3 << 0)
> > > > /* Vendor 2 */
> > > > #define         RISCV_HWPROBE_VENDOR_EXT_XVENDOR1       (1 << 0)
> > > > #define         RISCV_HWPROBE_VENDOR_EXT_XVENDOR2       (2 << 0)
> > > > /* Vendor 3 */
> > > > ...
> > > >
> > > > The keys overlap between vendors. To determine which extension a vendor
> > > > supports, hwprobe gets data from hart_isa_vendor[cpu]. If the vendor is
> > > > vendor 2, it is not possible for a vendor extension from vendor 3 to end
> > > > up in there. Only the extensions from that vendor can be supported by
> > > > that vendor's hardware.
> > >
> > > Gotcha. You're right I had misinterpreted this, thinking XTHEADVECTOR
> > > was a valid bit regardless of mvendorid, and that other vendors would
> > > have to choose new bits for their features and always return 0 for
> > > XTHEADVECTOR. With your explanation, it seems like you're allocating
> > > keys (in no particular order) whose meaning will change based on
> > > mvendorid.
> > >
> > > I guess I'm still not convinced that saving each vendor from having to
> > > add a VENDOR_EXT key in their keyspace is worth the sacrifice of
> > > spraying the vendor-specific keys across the generic keyspace. Are
> > > there advantages to having a single key whose category is similar but
> > > whose bits are entirely vendor-defined? Maybe if I were userspace and
> > > my feature could be satisfied equivalently by XTHEADVECTOR or
> > > XRIVOSOTHERTHING, then I could do one hwprobe call instead of two? But
> > > I don't think the vendors are going to be consistent enough for that
> > > equivalency to ever prove useful. The advantages in my head of the
> > > separate vendor keyspace are:
> > >  * Keeps the kernel code simple: if key >= (1 >> 63)
> > > vendor_config->do_hwprobe(), rather than having all these little calls
> > > in each specific switch case for vendor_config->do_vendor_ext0(),
> > > vendor_config->do_vendor_ext1(), etc.
> >
> > The consistency between vendors is guaranteed in this scheme. They just
> > add the extension to hwprobe_isa_vendor_ext0. The following code is the
> > critical code from the kernel:
> >
> >         for_each_cpu(cpu, cpus) {
> >                 struct riscv_isainfo *isavendorinfo = &hart_isa_vendor[cpu];
> >
> > #define VENDOR_EXT_KEY(ext)                                                             \
> >         do {                                                                            \
> >                 if (__riscv_isa_vendor_extension_available(isavendorinfo->isa,          \
> >                                                          RISCV_ISA_VENDOR_EXT_##ext))   \
> >                         pair->value |= RISCV_HWPROBE_VENDOR_EXT_##ext;                  \
> >                 else                                                                    \
> >                         missing |= RISCV_HWPROBE_VENDOR_EXT_##ext;                      \
> >         } while (false)
> >
> >         /*
> >          * Only use VENDOR_EXT_KEY() for extensions which can be exposed to userspace,
> >          * regardless of the kernel's configuration, as no other checks, besides
> >          * presence in the hart_vendor_isa bitmap, are made.
> >          */
> >         VENDOR_EXT_KEY(XTHEADVECTOR);
> >
> > #undef VENDOR_EXT_KEY
> >         }
> >
> >         /* Now turn off reporting features if any CPU is missing it. */
> >         pair->value &= ~missing;
> >
> > The only thing a vendor will have to do is add an entry below
> > VENDOR_EXT_KEY(XTHEADVECTOR) with their extension name (of course
> > populating a value for the key as well). This existing code will then
> > check if the extension is compatible with the hardware and appropriate
> > populate the bitmask. All vendors get this functionality for "free"
> > without needing to write the boilerplate code to expose vendor
> > extensions through hwprobe.
> >
> > Now that I write this out I do see that I overlooked that this code
> > needs to check the vendorid to ensure that the given extension is
> > actually associated with the vendorid. This would make this more
> > complicated but still seems like a low barrier to entry for a new
> > vendor, as well as a standard API for getting all vendor extensions that
> > are available on the platform regardless of which platform is being
> > used.
> >
> 
> Maybe I'll reserve judgment until I see the next spin, since we need
> both the "conditionalize on mvendorid" part, and to move the vendor
> stuff into a thead-specific file as discussed earlier. I'll be trying
> to picture how this looks 10 years from now, when a bunch of vendors
> have added dozens of extensions, and 75% of them are at that point
> defunct baggage.

Okay I will make some changes here and then we can continue this
conversation :)

> 
> > >  * It extends easily into passing other forms of vendor hwprobe info
> > > later, rather than solving only the case of risc-v extensions now, and
> > > then having to do this all again for each additional category of
> > > vendor data.
> >
> > This is a great point. I do agree that a different solution will be
> > necessary for arbitrary vendor data and I am all for making something
> > future compatible. At the same time I don't want to get trapped into
> > something that is suboptimal for the sake of doing less work later.
> > There is no chance of any compatibility once we leave the realm of
> > riscv extensions, so once a vendor needs something exported I would be
> > happy to write the code to support that.
> >
> > >  * Similarly, it discourages future vendors from trying to squint and
> > > find a way to make a vaguely generic sounding category for their own
> > > hwprobe key which will ultimately only ever be filled in by them
> > > anyway.
> >
> > What do you mean by this? There are no "categories" here, the vendor
> > just writes out their extension VENDOR_EXT_KEY(XVENDOREXTENSION) and it
> > gets shuttled to userspace on the hwprobe vendor call.
> 
> The category in this case is RISC-V extensions, since you've defined a
> key whose contents are vendor-specific, but whose bits must all fit
> the category of being a risc-v vendor extension.
> 
> To frame it in another light, one equivalent version from an ABI
> perspective would be to say ok, let's put this key up into the 1<<63
> range, but carve out a "common key" range where all vendors implement
> the same key definitions, like this VENDOR_EXT_0 key. Is that useful,
> or is it unnecessary structure? I think I'm of the opinion it's
> unnecessary structure, but I'm still open to being convinced.

That makes sense, thank you for clarifying, I appreciate that
perspective. I am coming from the direction that I want to share as much
as possible between vendors to minimize both kernel and userspace code.
In that sense, it is unnecessary. It would be fine to have each vendor
define their own way of probing which vendor extensions are available.
My inclination is that would lead to more verbosity in the
kernel and userspace, but I too am open to be convinced.

- Charlie

> -Evan
> 
> >
> > - Charlie
> >
> > >
> > > -Evan

[16/19] riscv: hwprobe: Add vendor extension probing

Commit Message

Comments

Patch