[PATCH 0/3] RISC-V: Support querying vendor extensions

[PATCH 0/3] RISC-V: Support querying vendor extensions

[Charlie Jenkins]

Introduce extensible method of querying vendor extensions. Keys above
1UL<<63 passed into the riscv_hwprobe syscall are reserved for vendor
extensions. The appropriate vendor is resolved using the discovered
mvendorid. Vendor specific code is then entered which determines how to
respond to the input hwprobe key.

The T-Head 0.7.1 vector extension is used to complete this vendor
extension framework. If vector support is compiled in and the cpu is
T-Head c906, determined with (marchid == 0 && mimpid == 0), then the
value of the hwprobe pair is set to 1 (defined as
THEAD_ISA_EXT0_V0_7_1).

Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
---
Charlie Jenkins (3):
      RISC-V: Framework for vendor extensions
      RISC-V: Add T-Head 0.7.1 vector extension to hwprobe
      RISC-V: Include documentation for hwprobe vendor extensions

 Documentation/riscv/hwprobe.rst                 | 17 +++++++
 arch/riscv/Kbuild                               |  1 +
 arch/riscv/Kconfig                              |  1 +
 arch/riscv/Kconfig.vendor                       | 14 ++++++
 arch/riscv/include/asm/extensions.h             | 16 +++++++
 arch/riscv/include/asm/hwprobe.h                |  1 +
 arch/riscv/kernel/sys_riscv.c                   | 60 +++++++++++++++++++++++--
 arch/riscv/vendor_extensions/Makefile           |  5 +++
 arch/riscv/vendor_extensions/thead/Makefile     |  8 ++++
 arch/riscv/vendor_extensions/thead/extensions.c | 24 ++++++++++
 10 files changed, 144 insertions(+), 3 deletions(-)
---
base-commit: 53cdf865f90ba922a854c65ed05b519f9d728424
change-id: 20230627-thead_vendor_extensions-0d320a311fcb
-- 
- Charlie

[PATCH 1/3] RISC-V: Framework for vendor extensions

[Charlie Jenkins]

Create Kconfig files, Makefiles, and functions to enable vendors to
provide information via the riscv_hwprobe syscall about which vendor
extensions are available.

Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
---
 arch/riscv/Kbuild                     |  1 +
 arch/riscv/Kconfig                    |  1 +
 arch/riscv/Kconfig.vendor             |  3 +++
 arch/riscv/include/asm/hwprobe.h      |  1 +
 arch/riscv/kernel/sys_riscv.c         | 40 ++++++++++++++++++++++++++++++++---
 arch/riscv/vendor_extensions/Makefile |  3 +++
 6 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/arch/riscv/Kbuild b/arch/riscv/Kbuild
index afa83e307a2e..bea38010d9db 100644
--- a/arch/riscv/Kbuild
+++ b/arch/riscv/Kbuild
@@ -3,6 +3,7 @@
 obj-y += kernel/ mm/ net/
 obj-$(CONFIG_BUILTIN_DTB) += boot/dts/
 obj-y += errata/
+obj-y += vendor_extensions/
 obj-$(CONFIG_KVM) += kvm/
 
 obj-$(CONFIG_ARCH_HAS_KEXEC_PURGATORY) += purgatory/
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index c1505c7729ec..19404ede0ee3 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -276,6 +276,7 @@ config AS_HAS_OPTION_ARCH
 
 source "arch/riscv/Kconfig.socs"
 source "arch/riscv/Kconfig.errata"
+source "arch/riscv/Kconfig.vendor"
 
 menu "Platform type"
 
diff --git a/arch/riscv/Kconfig.vendor b/arch/riscv/Kconfig.vendor
new file mode 100644
index 000000000000..213ac3e6fed5
--- /dev/null
+++ b/arch/riscv/Kconfig.vendor
@@ -0,0 +1,3 @@
+menu "Vendor extensions selection"
+
+endmenu # "Vendor extensions selection"
diff --git a/arch/riscv/include/asm/hwprobe.h b/arch/riscv/include/asm/hwprobe.h
index 78936f4ff513..fadb38b83243 100644
--- a/arch/riscv/include/asm/hwprobe.h
+++ b/arch/riscv/include/asm/hwprobe.h
@@ -9,5 +9,6 @@
 #include <uapi/asm/hwprobe.h>
 
 #define RISCV_HWPROBE_MAX_KEY 5
+#define RISCV_HWPROBE_VENDOR_EXTENSION_SPACE (UL(1)<<63)
 
 #endif
diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c
index 26ef5526bfb4..2351a5f7b8b1 100644
--- a/arch/riscv/kernel/sys_riscv.c
+++ b/arch/riscv/kernel/sys_riscv.c
@@ -188,9 +188,35 @@ static u64 hwprobe_misaligned(const struct cpumask *cpus)
 	return perf;
 }
 
+static int hwprobe_vendor(__u64 mvendorid, struct riscv_hwprobe *pair,
+			 const struct cpumask *cpus)
+{
+	switch (mvendorid) {
+	default:
+		return -1;
+	}
+
+	return 0;
+}
+
 static void hwprobe_one_pair(struct riscv_hwprobe *pair,
 			     const struct cpumask *cpus)
 {
+	int err;
+
+	if (((unsigned long) pair->key) >= RISCV_HWPROBE_VENDOR_EXTENSION_SPACE) {
+		struct riscv_hwprobe mvendorid = {
+			.key = RISCV_HWPROBE_KEY_MVENDORID,
+			.value = 0
+		};
+
+		hwprobe_arch_id(&mvendorid, cpus);
+		if (mvendorid.value != -1ULL)
+			err = hwprobe_vendor(mvendorid.value, pair, cpus);
+		else
+			err = -1;
+	}
+
 	switch (pair->key) {
 	case RISCV_HWPROBE_KEY_MVENDORID:
 	case RISCV_HWPROBE_KEY_MARCHID:
@@ -217,13 +243,21 @@ static void hwprobe_one_pair(struct riscv_hwprobe *pair,
 
 	/*
 	 * For forward compatibility, unknown keys don't fail the whole
-	 * call, but get their element key set to -1 and value set to 0
-	 * indicating they're unrecognized.
+	 * call, instead an error is raised to indicate the element key
+	 * is unrecognized.
 	 */
 	default:
+		err = -1;
+		break;
+	}
+
+	/*
+	 * Setting the element key to -1 and value to 0 indicates that
+	 * hwprobe was unable to find the requested key.
+	 */
+	if (err != 0) {
 		pair->key = -1;
 		pair->value = 0;
-		break;
 	}
 }
 
diff --git a/arch/riscv/vendor_extensions/Makefile b/arch/riscv/vendor_extensions/Makefile
new file mode 100644
index 000000000000..e815895e9372
--- /dev/null
+++ b/arch/riscv/vendor_extensions/Makefile
@@ -0,0 +1,3 @@
+ifdef CONFIG_RELOCATABLE
+KBUILD_CFLAGS += -fno-pie
+endif

-- 
2.41.0

[PATCH 2/3] RISC-V: Add T-Head 0.7.1 vector extension to hwprobe

[Charlie Jenkins]

Using vendor extensions in hwprobe, add the ability to query if the
0.7.1 vector extension is available. It is determined to be available
only if the kernel is compiled with vector support, and the user is
using the c906.

Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
---
 arch/riscv/Kconfig.vendor                       | 11 +++++++++++
 arch/riscv/include/asm/extensions.h             | 16 ++++++++++++++++
 arch/riscv/kernel/sys_riscv.c                   | 20 ++++++++++++++++++++
 arch/riscv/vendor_extensions/Makefile           |  2 ++
 arch/riscv/vendor_extensions/thead/Makefile     |  8 ++++++++
 arch/riscv/vendor_extensions/thead/extensions.c | 24 ++++++++++++++++++++++++
 6 files changed, 81 insertions(+)

diff --git a/arch/riscv/Kconfig.vendor b/arch/riscv/Kconfig.vendor
index 213ac3e6fed5..b8b9d15153eb 100644
--- a/arch/riscv/Kconfig.vendor
+++ b/arch/riscv/Kconfig.vendor
@@ -1,3 +1,14 @@
 menu "Vendor extensions selection"
 
+config VENDOR_EXTENSIONS_THEAD
+	bool "T-HEAD vendor extensions"
+	depends on RISCV_ALTERNATIVE
+	default n
+	help
+	  All T-HEAD vendor extensions Kconfig depend on this Kconfig. Disabling
+	  this Kconfig will disable all T-HEAD vendor extensions. Please say "Y"
+	  here if your platform uses T-HEAD vendor extensions.
+
+	  Otherwise, please say "N" here to avoid unnecessary overhead.
+
 endmenu # "Vendor extensions selection"
diff --git a/arch/riscv/include/asm/extensions.h b/arch/riscv/include/asm/extensions.h
new file mode 100644
index 000000000000..27ce294a3d65
--- /dev/null
+++ b/arch/riscv/include/asm/extensions.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2023 by Rivos Inc.
+ */
+#ifndef __ASM_EXTENSIONS_H
+#define __ASM_EXTENSIONS_H
+
+#include <asm/hwprobe.h>
+#include <linux/cpumask.h>
+
+#define THEAD_ISA_EXT0 (RISCV_HWPROBE_VENDOR_EXTENSION_SPACE)
+#define THEAD_ISA_EXT0_V0_7_1 (1 << 0)
+
+int hwprobe_thead(__u64 marchid, __u64 mimpid, struct riscv_hwprobe *pair,
+		  const struct cpumask *cpus);
+#endif
diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c
index 2351a5f7b8b1..58b12eaeaf46 100644
--- a/arch/riscv/kernel/sys_riscv.c
+++ b/arch/riscv/kernel/sys_riscv.c
@@ -13,6 +13,7 @@
 #include <asm/vector.h>
 #include <asm/switch_to.h>
 #include <asm/uaccess.h>
+#include <asm/extensions.h>
 #include <asm/unistd.h>
 #include <asm-generic/mman-common.h>
 #include <vdso/vsyscall.h>
@@ -192,6 +193,25 @@ static int hwprobe_vendor(__u64 mvendorid, struct riscv_hwprobe *pair,
 			 const struct cpumask *cpus)
 {
 	switch (mvendorid) {
+#ifdef CONFIG_VENDOR_EXTENSIONS_THEAD
+	case THEAD_VENDOR_ID:
+		struct riscv_hwprobe marchid = {
+			.key = RISCV_HWPROBE_KEY_MARCHID,
+			.value = 0
+		};
+		struct riscv_hwprobe mimpid = {
+			.key = RISCV_HWPROBE_KEY_MIMPID,
+			.value = 0
+		};
+
+		hwprobe_arch_id(&marchid, cpus);
+		hwprobe_arch_id(&mimpid, cpus);
+		if (marchid.value != -1ULL && mimpid.value != -1ULL)
+			hwprobe_thead(marchid.value, mimpid.value, pair, cpus);
+		else
+			return -1;
+		break;
+#endif
 	default:
 		return -1;
 	}
diff --git a/arch/riscv/vendor_extensions/Makefile b/arch/riscv/vendor_extensions/Makefile
index e815895e9372..38c3e80469fd 100644
--- a/arch/riscv/vendor_extensions/Makefile
+++ b/arch/riscv/vendor_extensions/Makefile
@@ -1,3 +1,5 @@
 ifdef CONFIG_RELOCATABLE
 KBUILD_CFLAGS += -fno-pie
 endif
+
+obj-$(CONFIG_VENDOR_EXTENSIONS_THEAD) += thead/
diff --git a/arch/riscv/vendor_extensions/thead/Makefile b/arch/riscv/vendor_extensions/thead/Makefile
new file mode 100644
index 000000000000..7cf43c629b66
--- /dev/null
+++ b/arch/riscv/vendor_extensions/thead/Makefile
@@ -0,0 +1,8 @@
+ifdef CONFIG_FTRACE
+CFLAGS_REMOVE_extensions.o = $(CC_FLAGS_FTRACE)
+endif
+ifdef CONFIG_KASAN
+KASAN_SANITIZE_extensions.o := n
+endif
+
+obj-y += extensions.o
diff --git a/arch/riscv/vendor_extensions/thead/extensions.c b/arch/riscv/vendor_extensions/thead/extensions.c
new file mode 100644
index 000000000000..a177501bc99c
--- /dev/null
+++ b/arch/riscv/vendor_extensions/thead/extensions.c
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2023 by Rivos Inc.
+ */
+
+#include <asm/extensions.h>
+
+int hwprobe_thead(__u64 marchid, __u64 mimpid, struct riscv_hwprobe *pair,
+		  const struct cpumask *cpus)
+{
+	pair->value = 0;
+	switch (pair->key) {
+	case THEAD_ISA_EXT0:
+#ifdef CONFIG_RISCV_ISA_V
+		if (marchid == 0 && mimpid == 0)
+			pair->value |= THEAD_ISA_EXT0_V0_7_1;
+#endif
+		break;
+	default:
+		return -1;
+	}
+
+	return 0;
+}

-- 
2.41.0

[PATCH 3/3] RISC-V: Include documentation for hwprobe vendor extensions

[Charlie Jenkins]

Document available vendor extensions.

Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
---
 Documentation/riscv/hwprobe.rst | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/Documentation/riscv/hwprobe.rst b/Documentation/riscv/hwprobe.rst
index 19165ebd82ba..167fd3e25632 100644
--- a/Documentation/riscv/hwprobe.rst
+++ b/Documentation/riscv/hwprobe.rst
@@ -97,3 +97,20 @@ The following keys are defined:
 
   * :c:macro:`RISCV_HWPROBE_MISALIGNED_UNSUPPORTED`: Misaligned accesses are
     not supported at all and will generate a misaligned address fault.
+
+RISC-V Hardware Probing Interface Vendor Extensions
+---------------------------------------------------
+
+All vendor extensions live at and beyond
+:c:macro:`RISCV_HWPROBE_VENDOR_EXTENSION_SPACE`. Each vendor can specify vendor
+extensions at any value above or equal to
+:c:macro:`RISCV_HWPROBE_VENDOR_EXTENSION_SPACE` without worrying about
+conflicting with values from other vendors. Only extensions from the vendor of
+the cpus passed into riscv_hwprobe will be matched.
+
+T-HEAD
+~~~~~~
+
+* :c:macro:`THEAD_ISA_EXT0`: Contains all of the EXT0 extensions
+
+  * :c:macro:`THEAD_ISA_EXT0_V0_7_1`: Vector extension V0.7.1 is supported

-- 
2.41.0

Re: [PATCH 1/3] RISC-V: Framework for vendor extensions

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 5484 bytes --]

Hey Charlie,

On Wed, Jul 05, 2023 at 08:30:17PM -0700, Charlie Jenkins wrote:
> Create Kconfig files, Makefiles, and functions to enable vendors to
> provide information via the riscv_hwprobe syscall about which vendor
> extensions are available.

This is all apparently from reading the diff, you don't need to tell us
what files you have created etc. Please just stick with explaining the
rationale for your changes (especially anything that might make someone
reading it go "huh").

> 
> Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> ---
>  arch/riscv/Kbuild                     |  1 +
>  arch/riscv/Kconfig                    |  1 +
>  arch/riscv/Kconfig.vendor             |  3 +++
>  arch/riscv/include/asm/hwprobe.h      |  1 +
>  arch/riscv/kernel/sys_riscv.c         | 40 ++++++++++++++++++++++++++++++++---
>  arch/riscv/vendor_extensions/Makefile |  3 +++
>  6 files changed, 46 insertions(+), 3 deletions(-)

> diff --git a/arch/riscv/Kbuild b/arch/riscv/Kbuild
> index afa83e307a2e..bea38010d9db 100644
> --- a/arch/riscv/Kbuild
> +++ b/arch/riscv/Kbuild
> @@ -3,6 +3,7 @@
>  obj-y += kernel/ mm/ net/
>  obj-$(CONFIG_BUILTIN_DTB) += boot/dts/
>  obj-y += errata/
> +obj-y += vendor_extensions/
>  obj-$(CONFIG_KVM) += kvm/
>  
>  obj-$(CONFIG_ARCH_HAS_KEXEC_PURGATORY) += purgatory/
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index c1505c7729ec..19404ede0ee3 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -276,6 +276,7 @@ config AS_HAS_OPTION_ARCH
>  
>  source "arch/riscv/Kconfig.socs"
>  source "arch/riscv/Kconfig.errata"
> +source "arch/riscv/Kconfig.vendor"
>  
>  menu "Platform type"
>  
> diff --git a/arch/riscv/Kconfig.vendor b/arch/riscv/Kconfig.vendor
> new file mode 100644
> index 000000000000..213ac3e6fed5
> --- /dev/null
> +++ b/arch/riscv/Kconfig.vendor
> @@ -0,0 +1,3 @@
> +menu "Vendor extensions selection"
> +
> +endmenu # "Vendor extensions selection"

These files don't do anything, don't add them until you need to.

> diff --git a/arch/riscv/include/asm/hwprobe.h b/arch/riscv/include/asm/hwprobe.h
> index 78936f4ff513..fadb38b83243 100644
> --- a/arch/riscv/include/asm/hwprobe.h
> +++ b/arch/riscv/include/asm/hwprobe.h
> @@ -9,5 +9,6 @@
>  #include <uapi/asm/hwprobe.h>
>  
>  #define RISCV_HWPROBE_MAX_KEY 5
> +#define RISCV_HWPROBE_VENDOR_EXTENSION_SPACE (UL(1)<<63)

Should this not be BIT_ULL(63)? Although I am not sure that we can
actually do this, more on that front later.

>  
>  #endif
> diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c
> index 26ef5526bfb4..2351a5f7b8b1 100644
> --- a/arch/riscv/kernel/sys_riscv.c
> +++ b/arch/riscv/kernel/sys_riscv.c
> @@ -188,9 +188,35 @@ static u64 hwprobe_misaligned(const struct cpumask *cpus)
>  	return perf;
>  }
>  
> +static int hwprobe_vendor(__u64 mvendorid, struct riscv_hwprobe *pair,
> +			 const struct cpumask *cpus)
> +{
> +	switch (mvendorid) {
> +	default:
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
>  static void hwprobe_one_pair(struct riscv_hwprobe *pair,
>  			     const struct cpumask *cpus)
>  {
> +	int err;
> +
> +	if (((unsigned long) pair->key) >= RISCV_HWPROBE_VENDOR_EXTENSION_SPACE) {

Hopefully Bjorn or someone that actually knows a thing or two about uapi
stuff can chime in here, but I think what you are doing here (where the
vendor space sets the MSB) really muddies the api. These keys are defined
as signed 64 bit numbers & -1 is the value set when a key is not valid.
I'd much rather we kept the negative space off-limits, and used the 62nd
bit instead, avoiding using negative numbers for valid keys.

> +		struct riscv_hwprobe mvendorid = {
> +			.key = RISCV_HWPROBE_KEY_MVENDORID,
> +			.value = 0
> +		};
> +
> +		hwprobe_arch_id(&mvendorid, cpus);

I think this needs a comment explaining why you do this hwprobe call, 
> +		if (mvendorid.value != -1ULL)
> +			err = hwprobe_vendor(mvendorid.value, pair, cpus);
> +		else
> +			err = -1;
> +	}

I don't really understand the control flow here. Why are you continuing
on to the switch statement, if you have either a) already ran
hwprobe_vendor() or b) noticed that mvendorid.value is not valid?

>  	switch (pair->key) {
>  	case RISCV_HWPROBE_KEY_MVENDORID:
>  	case RISCV_HWPROBE_KEY_MARCHID:
> @@ -217,13 +243,21 @@ static void hwprobe_one_pair(struct riscv_hwprobe *pair,
>  
>  	/*
>  	 * For forward compatibility, unknown keys don't fail the whole
> -	 * call, but get their element key set to -1 and value set to 0
> -	 * indicating they're unrecognized.
> +	 * call, instead an error is raised to indicate the element key
> +	 * is unrecognized.
>  	 */
>  	default:
> +		err = -1;
> +		break;
> +	}
> +
> +	/*
> +	 * Setting the element key to -1 and value to 0 indicates that
> +	 * hwprobe was unable to find the requested key.
> +	 */
> +	if (err != 0) {
>  		pair->key = -1;
>  		pair->value = 0;
> -		break;
>  	}
>  }
>  
> diff --git a/arch/riscv/vendor_extensions/Makefile b/arch/riscv/vendor_extensions/Makefile
> new file mode 100644
> index 000000000000..e815895e9372
> --- /dev/null
> +++ b/arch/riscv/vendor_extensions/Makefile
> @@ -0,0 +1,3 @@
> +ifdef CONFIG_RELOCATABLE
> +KBUILD_CFLAGS += -fno-pie
> +endif

There are no files in this directory, why do you need to do a dance
about relocatable kernels?

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/3] RISC-V: Add T-Head 0.7.1 vector extension to hwprobe

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 7578 bytes --]

Hey,

On Wed, Jul 05, 2023 at 08:30:18PM -0700, Charlie Jenkins wrote:
> Using vendor extensions in hwprobe, add the ability to query if the
> 0.7.1 vector extension is available. It is determined to be available
> only if the kernel is compiled with vector support,

> and the user is
> using the c906.

Heh, unfortunately your patch doesn't apply this limitation.

I'm not really sure where this series is intended to sit in relation to
Heiko's series that adds support for the actual T-Head vector stuff:
https://lore.kernel.org/linux-riscv/20230622231305.631331-1-heiko@sntech.de/

Is this intended to complement that? If so, these patches don't really
seem to integrate with it (and have some of the same flaws unfortunately
as that series does).

Firstly, to my knowledge, all T-Head cpu cores report 0 for impid &
archid. Stefan pointed out:
	C906 supports t-head/0.7.1 vectors as a configuration option.  The C906 in
	the D1 and BL808 has vectors, the recently announced CV1800B has one C906
	with vectors and one without, and I vaguely remember seeing a chip with only
	a non-vector C906.
	
	C908 (announced, no manual yet) claims V 1.0 support.  Presumably it will
	not support 0.7.1.
	
	C910 (exists on evaluation boards) lacks vector support.
	
	C920 (TH1520, SG2042, etc) has 0.7.1 support, at least superficially
	compatible with C906-with-vectors.  Hopefully we can share errata.
	
	This probably needs to be handled as an orthogonal "xtheadv" or "v0p7p1"
	extension in whatever replaces riscv,isa.

This makes an approach that does anything w/ their vector implementation
not discernible based on the m*id CSRs. IMO, the only way to make this
stuff work properly is to detect based on a DT or ACPI property whether
this stuff is supported on a given core.

Since the approach just cannot work, I don't have any detailed comments
to make, just a few small ones below.

> Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> ---
>  arch/riscv/Kconfig.vendor                       | 11 +++++++++++
>  arch/riscv/include/asm/extensions.h             | 16 ++++++++++++++++
>  arch/riscv/kernel/sys_riscv.c                   | 20 ++++++++++++++++++++
>  arch/riscv/vendor_extensions/Makefile           |  2 ++
>  arch/riscv/vendor_extensions/thead/Makefile     |  8 ++++++++
>  arch/riscv/vendor_extensions/thead/extensions.c | 24 ++++++++++++++++++++++++
>  6 files changed, 81 insertions(+)
> 
> diff --git a/arch/riscv/Kconfig.vendor b/arch/riscv/Kconfig.vendor
> index 213ac3e6fed5..b8b9d15153eb 100644
> --- a/arch/riscv/Kconfig.vendor
> +++ b/arch/riscv/Kconfig.vendor
> @@ -1,3 +1,14 @@
>  menu "Vendor extensions selection"
>  
> +config VENDOR_EXTENSIONS_THEAD
> +	bool "T-HEAD vendor extensions"

> +	depends on RISCV_ALTERNATIVE

Why do you need this?

> +	default n
> +	help
> +	  All T-HEAD vendor extensions Kconfig depend on this Kconfig. Disabling
> +	  this Kconfig will disable all T-HEAD vendor extensions. Please say "Y"
> +	  here if your platform uses T-HEAD vendor extensions.

I don't really like this Kconfig entry. We should just use the one in
Kconfig.errata that enables the actual vector stuff.

> +
> +	  Otherwise, please say "N" here to avoid unnecessary overhead.
> +
>  endmenu # "Vendor extensions selection"
> diff --git a/arch/riscv/include/asm/extensions.h b/arch/riscv/include/asm/extensions.h
> new file mode 100644
> index 000000000000..27ce294a3d65
> --- /dev/null
> +++ b/arch/riscv/include/asm/extensions.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2023 by Rivos Inc.
> + */
> +#ifndef __ASM_EXTENSIONS_H
> +#define __ASM_EXTENSIONS_H
> +
> +#include <asm/hwprobe.h>
> +#include <linux/cpumask.h>
> +
> +#define THEAD_ISA_EXT0 (RISCV_HWPROBE_VENDOR_EXTENSION_SPACE)
> +#define THEAD_ISA_EXT0_V0_7_1 (1 << 0)
> +
> +int hwprobe_thead(__u64 marchid, __u64 mimpid, struct riscv_hwprobe *pair,
> +		  const struct cpumask *cpus);
> +#endif
> diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c
> index 2351a5f7b8b1..58b12eaeaf46 100644
> --- a/arch/riscv/kernel/sys_riscv.c
> +++ b/arch/riscv/kernel/sys_riscv.c
> @@ -13,6 +13,7 @@
>  #include <asm/vector.h>
>  #include <asm/switch_to.h>
>  #include <asm/uaccess.h>
> +#include <asm/extensions.h>
>  #include <asm/unistd.h>
>  #include <asm-generic/mman-common.h>
>  #include <vdso/vsyscall.h>
> @@ -192,6 +193,25 @@ static int hwprobe_vendor(__u64 mvendorid, struct riscv_hwprobe *pair,
>  			 const struct cpumask *cpus)
>  {
>  	switch (mvendorid) {
> +#ifdef CONFIG_VENDOR_EXTENSIONS_THEAD

Please use IS_ENABLED() in code where possible, so that we get compile
time coverage of the code it disables. IMO, this kinda overcomplicates
the switch anyway, and it should be as simple as:
case THEAD_VENDOR_ID:
	return hwprobe_thead(pair, cpus);

and deal with the specific stuff (like impid etc) inside that function.

> +	case THEAD_VENDOR_ID:
> +		struct riscv_hwprobe marchid = {
> +			.key = RISCV_HWPROBE_KEY_MARCHID,
> +			.value = 0
> +		};
> +		struct riscv_hwprobe mimpid = {
> +			.key = RISCV_HWPROBE_KEY_MIMPID,
> +			.value = 0
> +		};
> +
> +		hwprobe_arch_id(&marchid, cpus);
> +		hwprobe_arch_id(&mimpid, cpus);
> +		if (marchid.value != -1ULL && mimpid.value != -1ULL)
> +			hwprobe_thead(marchid.value, mimpid.value, pair, cpus);
> +		else
> +			return -1;
> +		break;
> +#endif
>  	default:
>  		return -1;
>  	}
> diff --git a/arch/riscv/vendor_extensions/Makefile b/arch/riscv/vendor_extensions/Makefile
> index e815895e9372..38c3e80469fd 100644
> --- a/arch/riscv/vendor_extensions/Makefile
> +++ b/arch/riscv/vendor_extensions/Makefile
> @@ -1,3 +1,5 @@
>  ifdef CONFIG_RELOCATABLE
>  KBUILD_CFLAGS += -fno-pie
>  endif

Again, why do you need this, or...

> +
> +obj-$(CONFIG_VENDOR_EXTENSIONS_THEAD) += thead/
> diff --git a/arch/riscv/vendor_extensions/thead/Makefile b/arch/riscv/vendor_extensions/thead/Makefile
> new file mode 100644
> index 000000000000..7cf43c629b66
> --- /dev/null
> +++ b/arch/riscv/vendor_extensions/thead/Makefile
> @@ -0,0 +1,8 @@
> +ifdef CONFIG_FTRACE
> +CFLAGS_REMOVE_extensions.o = $(CC_FLAGS_FTRACE)
> +endif
> +ifdef CONFIG_KASAN
> +KASAN_SANITIZE_extensions.o := n
> +endif

...any of this? Not saying you don't, but I think it should be explained.

> +
> +obj-y += extensions.o
> diff --git a/arch/riscv/vendor_extensions/thead/extensions.c b/arch/riscv/vendor_extensions/thead/extensions.c
> new file mode 100644
> index 000000000000..a177501bc99c
> --- /dev/null
> +++ b/arch/riscv/vendor_extensions/thead/extensions.c
> @@ -0,0 +1,24 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2023 by Rivos Inc.
> + */
> +
> +#include <asm/extensions.h>
> +
> +int hwprobe_thead(__u64 marchid, __u64 mimpid, struct riscv_hwprobe *pair,
> +		  const struct cpumask *cpus)
> +{
> +	pair->value = 0;
> +	switch (pair->key) {
> +	case THEAD_ISA_EXT0:
> +#ifdef CONFIG_RISCV_ISA_V

As pointed out by Remi, this doesn't work either.
You should not claim this is supported, just because V is, you also need
the support for their vector "flavour" from Heiko's series.

Plus, it should be IS_ENABLED() too.

Cheers,
Conor.

> +		if (marchid == 0 && mimpid == 0)
> +			pair->value |= THEAD_ISA_EXT0_V0_7_1;
> +#endif
> +		break;
> +	default:
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> 
> -- 
> 2.41.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/3] RISC-V: Framework for vendor extensions

[Charlie Jenkins]

On Thu, Jul 06, 2023 at 06:15:57PM +0100, Conor Dooley wrote:
> Hey Charlie,
> 
> On Wed, Jul 05, 2023 at 08:30:17PM -0700, Charlie Jenkins wrote:
> > Create Kconfig files, Makefiles, and functions to enable vendors to
> > provide information via the riscv_hwprobe syscall about which vendor
> > extensions are available.
> 
> This is all apparently from reading the diff, you don't need to tell us
> what files you have created etc. Please just stick with explaining the
> rationale for your changes (especially anything that might make someone
> reading it go "huh").
> 
> > 
> > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> > ---
> >  arch/riscv/Kbuild                     |  1 +
> >  arch/riscv/Kconfig                    |  1 +
> >  arch/riscv/Kconfig.vendor             |  3 +++
> >  arch/riscv/include/asm/hwprobe.h      |  1 +
> >  arch/riscv/kernel/sys_riscv.c         | 40 ++++++++++++++++++++++++++++++++---
> >  arch/riscv/vendor_extensions/Makefile |  3 +++
> >  6 files changed, 46 insertions(+), 3 deletions(-)
> 
> > diff --git a/arch/riscv/Kbuild b/arch/riscv/Kbuild
> > index afa83e307a2e..bea38010d9db 100644
> > --- a/arch/riscv/Kbuild
> > +++ b/arch/riscv/Kbuild
> > @@ -3,6 +3,7 @@
> >  obj-y += kernel/ mm/ net/
> >  obj-$(CONFIG_BUILTIN_DTB) += boot/dts/
> >  obj-y += errata/
> > +obj-y += vendor_extensions/
> >  obj-$(CONFIG_KVM) += kvm/
> >  
> >  obj-$(CONFIG_ARCH_HAS_KEXEC_PURGATORY) += purgatory/
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index c1505c7729ec..19404ede0ee3 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -276,6 +276,7 @@ config AS_HAS_OPTION_ARCH
> >  
> >  source "arch/riscv/Kconfig.socs"
> >  source "arch/riscv/Kconfig.errata"
> > +source "arch/riscv/Kconfig.vendor"
> >  
> >  menu "Platform type"
> >  
> > diff --git a/arch/riscv/Kconfig.vendor b/arch/riscv/Kconfig.vendor
> > new file mode 100644
> > index 000000000000..213ac3e6fed5
> > --- /dev/null
> > +++ b/arch/riscv/Kconfig.vendor
> > @@ -0,0 +1,3 @@
> > +menu "Vendor extensions selection"
> > +
> > +endmenu # "Vendor extensions selection"
> 
> These files don't do anything, don't add them until you need to.
I wasn't sure if it was more clear to split up the vendor extension
framework from the T-Head specific calls since the main goal of this
series is to propose a vendor extension framework. I can merge this with
the following patch.
> 
> > diff --git a/arch/riscv/include/asm/hwprobe.h b/arch/riscv/include/asm/hwprobe.h
> > index 78936f4ff513..fadb38b83243 100644
> > --- a/arch/riscv/include/asm/hwprobe.h
> > +++ b/arch/riscv/include/asm/hwprobe.h
> > @@ -9,5 +9,6 @@
> >  #include <uapi/asm/hwprobe.h>
> >  
> >  #define RISCV_HWPROBE_MAX_KEY 5
> > +#define RISCV_HWPROBE_VENDOR_EXTENSION_SPACE (UL(1)<<63)
> 
> Should this not be BIT_ULL(63)? Although I am not sure that we can
> actually do this, more on that front later.
> 
> >  
> >  #endif
> > diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c
> > index 26ef5526bfb4..2351a5f7b8b1 100644
> > --- a/arch/riscv/kernel/sys_riscv.c
> > +++ b/arch/riscv/kernel/sys_riscv.c
> > @@ -188,9 +188,35 @@ static u64 hwprobe_misaligned(const struct cpumask *cpus)
> >  	return perf;
> >  }
> >  
> > +static int hwprobe_vendor(__u64 mvendorid, struct riscv_hwprobe *pair,
> > +			 const struct cpumask *cpus)
> > +{
> > +	switch (mvendorid) {
> > +	default:
> > +		return -1;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static void hwprobe_one_pair(struct riscv_hwprobe *pair,
> >  			     const struct cpumask *cpus)
> >  {
> > +	int err;
> > +
> > +	if (((unsigned long) pair->key) >= RISCV_HWPROBE_VENDOR_EXTENSION_SPACE) {
> 
> Hopefully Bjorn or someone that actually knows a thing or two about uapi
> stuff can chime in here, but I think what you are doing here (where the
> vendor space sets the MSB) really muddies the api. These keys are defined
> as signed 64 bit numbers & -1 is the value set when a key is not valid.
> I'd much rather we kept the negative space off-limits, and used the 62nd
> bit instead, avoiding using negative numbers for valid keys.
> 
Yeah that makes sense, I can change this up.
> > +		struct riscv_hwprobe mvendorid = {
> > +			.key = RISCV_HWPROBE_KEY_MVENDORID,
> > +			.value = 0
> > +		};
> > +
> > +		hwprobe_arch_id(&mvendorid, cpus);
> 
> I think this needs a comment explaining why you do this hwprobe call, 
> > +		if (mvendorid.value != -1ULL)
> > +			err = hwprobe_vendor(mvendorid.value, pair, cpus);
> > +		else
> > +			err = -1;
> > +	}
> 
> I don't really understand the control flow here. Why are you continuing
> on to the switch statement, if you have either a) already ran
> hwprobe_vendor() or b) noticed that mvendorid.value is not valid?
> 
The purpose of this was to consolidate the error handling to a single
spot at the bottom of the file. It would fall through the switch
statement and set the values appropriately. I guess it does seem a bit
awkward.
> >  	switch (pair->key) {
> >  	case RISCV_HWPROBE_KEY_MVENDORID:
> >  	case RISCV_HWPROBE_KEY_MARCHID:
> > @@ -217,13 +243,21 @@ static void hwprobe_one_pair(struct riscv_hwprobe *pair,
> >  
> >  	/*
> >  	 * For forward compatibility, unknown keys don't fail the whole
> > -	 * call, but get their element key set to -1 and value set to 0
> > -	 * indicating they're unrecognized.
> > +	 * call, instead an error is raised to indicate the element key
> > +	 * is unrecognized.
> >  	 */
> >  	default:
> > +		err = -1;
> > +		break;
> > +	}
> > +
> > +	/*
> > +	 * Setting the element key to -1 and value to 0 indicates that
> > +	 * hwprobe was unable to find the requested key.
> > +	 */
> > +	if (err != 0) {
> >  		pair->key = -1;
> >  		pair->value = 0;
> > -		break;
> >  	}
> >  }
> >  
> > diff --git a/arch/riscv/vendor_extensions/Makefile b/arch/riscv/vendor_extensions/Makefile
> > new file mode 100644
> > index 000000000000..e815895e9372
> > --- /dev/null
> > +++ b/arch/riscv/vendor_extensions/Makefile
> > @@ -0,0 +1,3 @@
> > +ifdef CONFIG_RELOCATABLE
> > +KBUILD_CFLAGS += -fno-pie
> > +endif
> 
> There are no files in this directory, why do you need to do a dance
> about relocatable kernels?
> 
This is framework for the following patch in the series. I saw these
lines in the errata Makefile so I created this Makefile to set up the
following patch in the series. I can merge this patch with the following
patch to make this series more clear.
> Cheers,
> Conor.

Re: [PATCH 2/3] RISC-V: Add T-Head 0.7.1 vector extension to hwprobe

[Charlie Jenkins]

On Thu, Jul 06, 2023 at 06:38:17PM +0100, Conor Dooley wrote:
> Hey,
> 
> On Wed, Jul 05, 2023 at 08:30:18PM -0700, Charlie Jenkins wrote:
> > Using vendor extensions in hwprobe, add the ability to query if the
> > 0.7.1 vector extension is available. It is determined to be available
> > only if the kernel is compiled with vector support,
> 
> > and the user is
> > using the c906.
> 
> Heh, unfortunately your patch doesn't apply this limitation.
> 
> I'm not really sure where this series is intended to sit in relation to
> Heiko's series that adds support for the actual T-Head vector stuff:
> https://lore.kernel.org/linux-riscv/20230622231305.631331-1-heiko@sntech.de/
> 
> Is this intended to complement that? If so, these patches don't really
> seem to integrate with it (and have some of the same flaws unfortunately
> as that series does).
> 
> Firstly, to my knowledge, all T-Head cpu cores report 0 for impid &
> archid. Stefan pointed out:
> 	C906 supports t-head/0.7.1 vectors as a configuration option.  The C906 in
> 	the D1 and BL808 has vectors, the recently announced CV1800B has one C906
> 	with vectors and one without, and I vaguely remember seeing a chip with only
> 	a non-vector C906.
> 	
> 	C908 (announced, no manual yet) claims V 1.0 support.  Presumably it will
> 	not support 0.7.1.
> 	
> 	C910 (exists on evaluation boards) lacks vector support.
> 	
> 	C920 (TH1520, SG2042, etc) has 0.7.1 support, at least superficially
> 	compatible with C906-with-vectors.  Hopefully we can share errata.
> 	
> 	This probably needs to be handled as an orthogonal "xtheadv" or "v0p7p1"
> 	extension in whatever replaces riscv,isa.
> 
> This makes an approach that does anything w/ their vector implementation
> not discernible based on the m*id CSRs. IMO, the only way to make this
> stuff work properly is to detect based on a DT or ACPI property whether
> this stuff is supported on a given core.
> 
> Since the approach just cannot work, I don't have any detailed comments
> to make, just a few small ones below.
>

It would be beneficial to use Heiko's vector support patch. I can look
into using that. The main purpose of this patch is to propose a method
of vendor extension support and since T-Head has hardware I have used 
their hardware as an example of how to implement vendor extensions. 
That is the reason for the kind of awkward patch segmentation.

> > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> > ---
> >  arch/riscv/Kconfig.vendor                       | 11 +++++++++++
> >  arch/riscv/include/asm/extensions.h             | 16 ++++++++++++++++
> >  arch/riscv/kernel/sys_riscv.c                   | 20 ++++++++++++++++++++
> >  arch/riscv/vendor_extensions/Makefile           |  2 ++
> >  arch/riscv/vendor_extensions/thead/Makefile     |  8 ++++++++
> >  arch/riscv/vendor_extensions/thead/extensions.c | 24 ++++++++++++++++++++++++
> >  6 files changed, 81 insertions(+)
> > 
> > diff --git a/arch/riscv/Kconfig.vendor b/arch/riscv/Kconfig.vendor
> > index 213ac3e6fed5..b8b9d15153eb 100644
> > --- a/arch/riscv/Kconfig.vendor
> > +++ b/arch/riscv/Kconfig.vendor
> > @@ -1,3 +1,14 @@
> >  menu "Vendor extensions selection"
> >  
> > +config VENDOR_EXTENSIONS_THEAD
> > +	bool "T-HEAD vendor extensions"
> 
> > +	depends on RISCV_ALTERNATIVE
> 
> Why do you need this?
> 
Thanks for pointing that out, I meant to remove that.
> > +	default n
> > +	help
> > +	  All T-HEAD vendor extensions Kconfig depend on this Kconfig. Disabling
> > +	  this Kconfig will disable all T-HEAD vendor extensions. Please say "Y"
> > +	  here if your platform uses T-HEAD vendor extensions.
> 
> I don't really like this Kconfig entry. We should just use the one in
> Kconfig.errata that enables the actual vector stuff.
> 
The purpose of this is to support more than just the T-Head vector
extension. The vector extension is just the vendor extension I selected
to support first. The purpose of this file is for all vendors to have
their own Kconfig entries to enable the vector extension which I didn't
feel that it properly fit into the errata.
> > +
> > +	  Otherwise, please say "N" here to avoid unnecessary overhead.
> > +
> >  endmenu # "Vendor extensions selection"
> > diff --git a/arch/riscv/include/asm/extensions.h b/arch/riscv/include/asm/extensions.h
> > new file mode 100644
> > index 000000000000..27ce294a3d65
> > --- /dev/null
> > +++ b/arch/riscv/include/asm/extensions.h
> > @@ -0,0 +1,16 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Copyright (C) 2023 by Rivos Inc.
> > + */
> > +#ifndef __ASM_EXTENSIONS_H
> > +#define __ASM_EXTENSIONS_H
> > +
> > +#include <asm/hwprobe.h>
> > +#include <linux/cpumask.h>
> > +
> > +#define THEAD_ISA_EXT0 (RISCV_HWPROBE_VENDOR_EXTENSION_SPACE)
> > +#define THEAD_ISA_EXT0_V0_7_1 (1 << 0)
> > +
> > +int hwprobe_thead(__u64 marchid, __u64 mimpid, struct riscv_hwprobe *pair,
> > +		  const struct cpumask *cpus);
> > +#endif
> > diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c
> > index 2351a5f7b8b1..58b12eaeaf46 100644
> > --- a/arch/riscv/kernel/sys_riscv.c
> > +++ b/arch/riscv/kernel/sys_riscv.c
> > @@ -13,6 +13,7 @@
> >  #include <asm/vector.h>
> >  #include <asm/switch_to.h>
> >  #include <asm/uaccess.h>
> > +#include <asm/extensions.h>
> >  #include <asm/unistd.h>
> >  #include <asm-generic/mman-common.h>
> >  #include <vdso/vsyscall.h>
> > @@ -192,6 +193,25 @@ static int hwprobe_vendor(__u64 mvendorid, struct riscv_hwprobe *pair,
> >  			 const struct cpumask *cpus)
> >  {
> >  	switch (mvendorid) {
> > +#ifdef CONFIG_VENDOR_EXTENSIONS_THEAD
> 
> Please use IS_ENABLED() in code where possible, so that we get compile
> time coverage of the code it disables. IMO, this kinda overcomplicates
> the switch anyway, and it should be as simple as:
> case THEAD_VENDOR_ID:
> 	return hwprobe_thead(pair, cpus);
> 
> and deal with the specific stuff (like impid etc) inside that function.
> 
> > +	case THEAD_VENDOR_ID:
> > +		struct riscv_hwprobe marchid = {
> > +			.key = RISCV_HWPROBE_KEY_MARCHID,
> > +			.value = 0
> > +		};
> > +		struct riscv_hwprobe mimpid = {
> > +			.key = RISCV_HWPROBE_KEY_MIMPID,
> > +			.value = 0
> > +		};
> > +
> > +		hwprobe_arch_id(&marchid, cpus);
> > +		hwprobe_arch_id(&mimpid, cpus);
> > +		if (marchid.value != -1ULL && mimpid.value != -1ULL)
> > +			hwprobe_thead(marchid.value, mimpid.value, pair, cpus);
> > +		else
> > +			return -1;
> > +		break;
> > +#endif
> >  	default:
> >  		return -1;
> >  	}
> > diff --git a/arch/riscv/vendor_extensions/Makefile b/arch/riscv/vendor_extensions/Makefile
> > index e815895e9372..38c3e80469fd 100644
> > --- a/arch/riscv/vendor_extensions/Makefile
> > +++ b/arch/riscv/vendor_extensions/Makefile
> > @@ -1,3 +1,5 @@
> >  ifdef CONFIG_RELOCATABLE
> >  KBUILD_CFLAGS += -fno-pie
> >  endif
> 
> Again, why do you need this, or...
This file is properly filled out in the next patch in the series but I
wanted to break it up. I saw this in the errata Makefiles so I assumed
it would be needed here.
> > +
> > +obj-$(CONFIG_VENDOR_EXTENSIONS_THEAD) += thead/
> > diff --git a/arch/riscv/vendor_extensions/thead/Makefile b/arch/riscv/vendor_extensions/thead/Makefile
> > new file mode 100644
> > index 000000000000..7cf43c629b66
> > --- /dev/null
> > +++ b/arch/riscv/vendor_extensions/thead/Makefile
> > @@ -0,0 +1,8 @@
> > +ifdef CONFIG_FTRACE
> > +CFLAGS_REMOVE_extensions.o = $(CC_FLAGS_FTRACE)
> > +endif
> > +ifdef CONFIG_KASAN
> > +KASAN_SANITIZE_extensions.o := n
> > +endif
> 
> ...any of this? Not saying you don't, but I think it should be explained.
> 
Same reasoning as above, I can remove it if it's not needed.
> > +
> > +obj-y += extensions.o
> > diff --git a/arch/riscv/vendor_extensions/thead/extensions.c b/arch/riscv/vendor_extensions/thead/extensions.c
> > new file mode 100644
> > index 000000000000..a177501bc99c
> > --- /dev/null
> > +++ b/arch/riscv/vendor_extensions/thead/extensions.c
> > @@ -0,0 +1,24 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (C) 2023 by Rivos Inc.
> > + */
> > +
> > +#include <asm/extensions.h>
> > +
> > +int hwprobe_thead(__u64 marchid, __u64 mimpid, struct riscv_hwprobe *pair,
> > +		  const struct cpumask *cpus)
> > +{
> > +	pair->value = 0;
> > +	switch (pair->key) {
> > +	case THEAD_ISA_EXT0:
> > +#ifdef CONFIG_RISCV_ISA_V
> 
> As pointed out by Remi, this doesn't work either.
> You should not claim this is supported, just because V is, you also need
> the support for their vector "flavour" from Heiko's series.
> 
> Plus, it should be IS_ENABLED() too.
> 
> Cheers,
> Conor.
> 
The thought process was that it should only matter if they care about
V. However since they are different versions of V I could see it being
better to not depend on the same config. 
> > +		if (marchid == 0 && mimpid == 0)
> > +			pair->value |= THEAD_ISA_EXT0_V0_7_1;
> > +#endif
> > +		break;
> > +	default:
> > +		return -1;
> > +	}
> > +
> > +	return 0;
> > +}
> > 
> > -- 
> > 2.41.0
> > 

Re: [PATCH 1/3] RISC-V: Framework for vendor extensions

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 5510 bytes --]

On Thu, Jul 06, 2023 at 12:51:03PM -0700, Charlie Jenkins wrote:
> On Thu, Jul 06, 2023 at 06:15:57PM +0100, Conor Dooley wrote:
> > On Wed, Jul 05, 2023 at 08:30:17PM -0700, Charlie Jenkins wrote:
> > > diff --git a/arch/riscv/Kbuild b/arch/riscv/Kbuild
> > > index afa83e307a2e..bea38010d9db 100644
> > > --- a/arch/riscv/Kbuild
> > > +++ b/arch/riscv/Kbuild
> > > @@ -3,6 +3,7 @@
> > >  obj-y += kernel/ mm/ net/
> > >  obj-$(CONFIG_BUILTIN_DTB) += boot/dts/
> > >  obj-y += errata/
> > > +obj-y += vendor_extensions/
> > >  obj-$(CONFIG_KVM) += kvm/
> > >  
> > >  obj-$(CONFIG_ARCH_HAS_KEXEC_PURGATORY) += purgatory/
> > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > > index c1505c7729ec..19404ede0ee3 100644
> > > --- a/arch/riscv/Kconfig
> > > +++ b/arch/riscv/Kconfig
> > > @@ -276,6 +276,7 @@ config AS_HAS_OPTION_ARCH
> > >  
> > >  source "arch/riscv/Kconfig.socs"
> > >  source "arch/riscv/Kconfig.errata"
> > > +source "arch/riscv/Kconfig.vendor"
> > >  
> > >  menu "Platform type"
> > >  
> > > diff --git a/arch/riscv/Kconfig.vendor b/arch/riscv/Kconfig.vendor
> > > new file mode 100644
> > > index 000000000000..213ac3e6fed5
> > > --- /dev/null
> > > +++ b/arch/riscv/Kconfig.vendor
> > > @@ -0,0 +1,3 @@
> > > +menu "Vendor extensions selection"
> > > +
> > > +endmenu # "Vendor extensions selection"
> > 
> > These files don't do anything, don't add them until you need to.
> 
> I wasn't sure if it was more clear to split up the vendor extension
> framework from the T-Head specific calls since the main goal of this
> series is to propose a vendor extension framework. I can merge this with
> the following patch.

Yeah, don't add files that you are not using, until the patch in which
you need them. Say we had to revert your 2/3 patch - we'd be left with
dead files in the tree.

> > >  static void hwprobe_one_pair(struct riscv_hwprobe *pair,
> > >  			     const struct cpumask *cpus)
> > >  {
> > > +	int err;
> > > +
> > > +	if (((unsigned long) pair->key) >= RISCV_HWPROBE_VENDOR_EXTENSION_SPACE) {
> > 
> > Hopefully Bjorn or someone that actually knows a thing or two about uapi
> > stuff can chime in here, but I think what you are doing here (where the
> > vendor space sets the MSB) really muddies the api. These keys are defined
> > as signed 64 bit numbers & -1 is the value set when a key is not valid.
> > I'd much rather we kept the negative space off-limits, and used the 62nd
> > bit instead, avoiding using negative numbers for valid keys.
> > 
> Yeah that makes sense, I can change this up.
> > > +		struct riscv_hwprobe mvendorid = {
> > > +			.key = RISCV_HWPROBE_KEY_MVENDORID,
> > > +			.value = 0
> > > +		};
> > > +
> > > +		hwprobe_arch_id(&mvendorid, cpus);
> > 
> > I think this needs a comment explaining why you do this hwprobe call, 
> > > +		if (mvendorid.value != -1ULL)
> > > +			err = hwprobe_vendor(mvendorid.value, pair, cpus);
> > > +		else
> > > +			err = -1;
> > > +	}
> > 
> > I don't really understand the control flow here. Why are you continuing
> > on to the switch statement, if you have either a) already ran
> > hwprobe_vendor() or b) noticed that mvendorid.value is not valid?
> > 
> The purpose of this was to consolidate the error handling to a single
> spot at the bottom of the file. It would fall through the switch
> statement and set the values appropriately. I guess it does seem a bit
> awkward.

Use a goto? It seems to do exactly what you want here.
You could also define err as -1 to begin with, and drop the else branch.

The other limitation of this stuff, while I think of it, is that you
preclude more than one vendor implementing an extension.

> > >  	switch (pair->key) {
> > >  	case RISCV_HWPROBE_KEY_MVENDORID:
> > >  	case RISCV_HWPROBE_KEY_MARCHID:
> > > @@ -217,13 +243,21 @@ static void hwprobe_one_pair(struct riscv_hwprobe *pair,
> > >  
> > >  	/*
> > >  	 * For forward compatibility, unknown keys don't fail the whole
> > > -	 * call, but get their element key set to -1 and value set to 0
> > > -	 * indicating they're unrecognized.
> > > +	 * call, instead an error is raised to indicate the element key
> > > +	 * is unrecognized.
> > >  	 */
> > >  	default:
> > > +		err = -1;
> > > +		break;
> > > +	}
> > > +
> > > +	/*
> > > +	 * Setting the element key to -1 and value to 0 indicates that
> > > +	 * hwprobe was unable to find the requested key.
> > > +	 */
> > > +	if (err != 0) {
> > >  		pair->key = -1;
> > >  		pair->value = 0;
> > > -		break;
> > >  	}
> > >  }
> > >  
> > > diff --git a/arch/riscv/vendor_extensions/Makefile b/arch/riscv/vendor_extensions/Makefile
> > > new file mode 100644
> > > index 000000000000..e815895e9372
> > > --- /dev/null
> > > +++ b/arch/riscv/vendor_extensions/Makefile
> > > @@ -0,0 +1,3 @@
> > > +ifdef CONFIG_RELOCATABLE
> > > +KBUILD_CFLAGS += -fno-pie
> > > +endif
> > 
> > There are no files in this directory, why do you need to do a dance
> > about relocatable kernels?
> > 
> This is framework for the following patch in the series. I saw these
> lines in the errata Makefile so I created this Makefile to set up the
> following patch in the series. I can merge this patch with the following
> patch to make this series more clear.

The errata code gets called super early on, so it needs it. What you
have here does not. We want to remove it from the errata Makefile
anyway.

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/3] RISC-V: Add T-Head 0.7.1 vector extension to hwprobe

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 8939 bytes --]

Hey Charlie,

On Thu, Jul 06, 2023 at 01:00:52PM -0700, Charlie Jenkins wrote:
> On Thu, Jul 06, 2023 at 06:38:17PM +0100, Conor Dooley wrote:
> > On Wed, Jul 05, 2023 at 08:30:18PM -0700, Charlie Jenkins wrote:
> > > Using vendor extensions in hwprobe, add the ability to query if the
> > > 0.7.1 vector extension is available. It is determined to be available
> > > only if the kernel is compiled with vector support,
> > 
> > > and the user is
> > > using the c906.
> > 
> > Heh, unfortunately your patch doesn't apply this limitation.
> > 
> > I'm not really sure where this series is intended to sit in relation to
> > Heiko's series that adds support for the actual T-Head vector stuff:
> > https://lore.kernel.org/linux-riscv/20230622231305.631331-1-heiko@sntech.de/
> > 
> > Is this intended to complement that? If so, these patches don't really
> > seem to integrate with it (and have some of the same flaws unfortunately
> > as that series does).
> > 
> > Firstly, to my knowledge, all T-Head cpu cores report 0 for impid &
> > archid. Stefan pointed out:
> > 	C906 supports t-head/0.7.1 vectors as a configuration option.  The C906 in
> > 	the D1 and BL808 has vectors, the recently announced CV1800B has one C906
> > 	with vectors and one without, and I vaguely remember seeing a chip with only
> > 	a non-vector C906.
> > 	
> > 	C908 (announced, no manual yet) claims V 1.0 support.  Presumably it will
> > 	not support 0.7.1.
> > 	
> > 	C910 (exists on evaluation boards) lacks vector support.
> > 	
> > 	C920 (TH1520, SG2042, etc) has 0.7.1 support, at least superficially
> > 	compatible with C906-with-vectors.  Hopefully we can share errata.
> > 	
> > 	This probably needs to be handled as an orthogonal "xtheadv" or "v0p7p1"
> > 	extension in whatever replaces riscv,isa.
> > 
> > This makes an approach that does anything w/ their vector implementation
> > not discernible based on the m*id CSRs. IMO, the only way to make this
> > stuff work properly is to detect based on a DT or ACPI property whether
> > this stuff is supported on a given core.
> > 
> > Since the approach just cannot work, I don't have any detailed comments
> > to make, just a few small ones below.
> >
> 
> It would be beneficial to use Heiko's vector support patch. I can look
> into using that. The main purpose of this patch is to propose a method
> of vendor extension support and since T-Head has hardware I have used 
> their hardware as an example of how to implement vendor extensions. 
> That is the reason for the kind of awkward patch segmentation.

Just to be clear, you *need* to do something on top of Heiko's patches,
but with an adaptation for how you actually get the information as to
whether the device supports the extension. It makes no sense to tell
userspace that this "flavour" of V is present, if using it is going to
be problematic, as the kernel doesn't actually support it.

As I have pointed out above, while you might have a D1 with a c906 that
does have vector, other T-Head cores that respond identically to
mvendorid/marchid/mimpid may not.

For anything you do, please do it on top of my series adding a new
mechanism for parsing this information:
https://lore.kernel.org/all/20230703-repayment-vocalist-e4f3eeac2b2a@wendy/

If you have not already, you should coordinate with Heiko about what to
do here, before taking over the series he submitted.

> > > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> > > ---
> > >  arch/riscv/Kconfig.vendor                       | 11 +++++++++++
> > >  arch/riscv/include/asm/extensions.h             | 16 ++++++++++++++++
> > >  arch/riscv/kernel/sys_riscv.c                   | 20 ++++++++++++++++++++
> > >  arch/riscv/vendor_extensions/Makefile           |  2 ++
> > >  arch/riscv/vendor_extensions/thead/Makefile     |  8 ++++++++
> > >  arch/riscv/vendor_extensions/thead/extensions.c | 24 ++++++++++++++++++++++++
> > >  6 files changed, 81 insertions(+)
> > > 
> > > diff --git a/arch/riscv/Kconfig.vendor b/arch/riscv/Kconfig.vendor
> > > index 213ac3e6fed5..b8b9d15153eb 100644
> > > --- a/arch/riscv/Kconfig.vendor
> > > +++ b/arch/riscv/Kconfig.vendor
> > > @@ -1,3 +1,14 @@
> > >  menu "Vendor extensions selection"
> > >  
> > > +config VENDOR_EXTENSIONS_THEAD
> > > +	bool "T-HEAD vendor extensions"
> > 
> > > +	depends on RISCV_ALTERNATIVE
> > 
> > Why do you need this?
> > 
> Thanks for pointing that out, I meant to remove that.
> > > +	default n
> > > +	help
> > > +	  All T-HEAD vendor extensions Kconfig depend on this Kconfig. Disabling
> > > +	  this Kconfig will disable all T-HEAD vendor extensions. Please say "Y"
> > > +	  here if your platform uses T-HEAD vendor extensions.
> > 
> > I don't really like this Kconfig entry. We should just use the one in
> > Kconfig.errata that enables the actual vector stuff.
> > 
> The purpose of this is to support more than just the T-Head vector
> extension. The vector extension is just the vendor extension I selected
> to support first. The purpose of this file is for all vendors to have
> their own Kconfig entries to enable the vector extension which I didn't
> feel that it properly fit into the errata.

Hopefully there will be no other vendors that decide to implement
v0.7.1! We probably do need to re-do how our menus look, although I
question whether adding yet another file (we have Kconfig.socs...) is
the right thing to do.


> > > diff --git a/arch/riscv/vendor_extensions/Makefile b/arch/riscv/vendor_extensions/Makefile
> > > index e815895e9372..38c3e80469fd 100644
> > > --- a/arch/riscv/vendor_extensions/Makefile
> > > +++ b/arch/riscv/vendor_extensions/Makefile
> > > @@ -1,3 +1,5 @@
> > >  ifdef CONFIG_RELOCATABLE
> > >  KBUILD_CFLAGS += -fno-pie
> > >  endif
> > 
> > Again, why do you need this, or...

> This file is properly filled out in the next patch in the series but I
> wanted to break it up. I saw this in the errata Makefiles so I assumed
> it would be needed here.

Ye, you shouldn't...

> > > +
> > > +obj-$(CONFIG_VENDOR_EXTENSIONS_THEAD) += thead/
> > > diff --git a/arch/riscv/vendor_extensions/thead/Makefile b/arch/riscv/vendor_extensions/thead/Makefile
> > > new file mode 100644
> > > index 000000000000..7cf43c629b66
> > > --- /dev/null
> > > +++ b/arch/riscv/vendor_extensions/thead/Makefile
> > > @@ -0,0 +1,8 @@
> > > +ifdef CONFIG_FTRACE
> > > +CFLAGS_REMOVE_extensions.o = $(CC_FLAGS_FTRACE)
> > > +endif
> > > +ifdef CONFIG_KASAN
> > > +KASAN_SANITIZE_extensions.o := n
> > > +endif
> > 
> > ...any of this? Not saying you don't, but I think it should be explained.
> > 
> Same reasoning as above, I can remove it if it's not needed.

Ditto.

> > > +
> > > +obj-y += extensions.o
> > > diff --git a/arch/riscv/vendor_extensions/thead/extensions.c b/arch/riscv/vendor_extensions/thead/extensions.c
> > > new file mode 100644
> > > index 000000000000..a177501bc99c
> > > --- /dev/null
> > > +++ b/arch/riscv/vendor_extensions/thead/extensions.c
> > > @@ -0,0 +1,24 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Copyright (C) 2023 by Rivos Inc.
> > > + */
> > > +
> > > +#include <asm/extensions.h>
> > > +
> > > +int hwprobe_thead(__u64 marchid, __u64 mimpid, struct riscv_hwprobe *pair,
> > > +		  const struct cpumask *cpus)
> > > +{
> > > +	pair->value = 0;
> > > +	switch (pair->key) {
> > > +	case THEAD_ISA_EXT0:
> > > +#ifdef CONFIG_RISCV_ISA_V
> > 
> > As pointed out by Remi, this doesn't work either.
> > You should not claim this is supported, just because V is, you also need
> > the support for their vector "flavour" from Heiko's series.

> The thought process was that it should only matter if they care about
> V. However since they are different versions of V I could see it being
> better to not depend on the same config. 

Yeah, you unfortunately cannot use that kconfig symbol for this purpose,
they are incompatible after all. You probably need to use some interface
like riscv_isa_extension_available() that supports the vendor flavour of
this stuff. I've given no more thought to how that should look though
than the time it took to type this out. I'd be down to collaborate on
something in that neck of the woods, once things settle down after -rc1
& I've written a patch for dealing with extensions that have multiple
subsets in the extension detection code.

Perhaps Palmer or someone at Rivos could give you a rundown of the
vector incompatibility stuff on a platform with a shorter response
time than email ;)

Cheers,
Conor.

> > > +		if (marchid == 0 && mimpid == 0)
> > > +			pair->value |= THEAD_ISA_EXT0_V0_7_1;
> > > +#endif
> > > +		break;
> > > +	default:
> > > +		return -1;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > 
> > > -- 
> > > 2.41.0
> > > 
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]