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

  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(+)
  

Le torstaina 6. heinäkuuta 2023, 6.30.18 EEST Charlie Jenkins a écrit :
> 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;

I'm not sure I follow the code, but exposing an extension to userspace that 
the kernel does not handle is a bad idea. AFAIK, the initialisation and 
context switching code must be in place first.

And I don't suppose that this can *simply* piggy-back on the existing RVV 
1.0.0 support, because user-space  assumes 1.0.0-compliant behaviour if the 
existing vector flag is set in hwprobe().

Indeed, I recall somebody else posted a recent patchset ostensibly with the 
same goal that was a lot more involved than this.

> +#endif
> +		break;
> +	default:
> +		return -1;
> +	}
> +
> +	return 0;
> +}
  

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
>
  

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

On Thu, Jul 06, 2023 at 07:32:36PM +0300, Rémi Denis-Courmont wrote:
> Le torstaina 6. heinäkuuta 2023, 6.30.18 EEST Charlie Jenkins a écrit :
> > 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;
> 
> I'm not sure I follow the code, but exposing an extension to userspace that 
> the kernel does not handle is a bad idea. AFAIK, the initialisation and 
> context switching code must be in place first.
> 
> And I don't suppose that this can *simply* piggy-back on the existing RVV 
> 1.0.0 support, because user-space  assumes 1.0.0-compliant behaviour if the 
> existing vector flag is set in hwprobe().
> 
> Indeed, I recall somebody else posted a recent patchset ostensibly with the 
> same goal that was a lot more involved than this.
> 
The main goal of this patch was to get vendor extensions tied into
hwprobe. I selected this vector extension to start with, but it looks
like it is more complicated than I had anticipated.
> > +#endif
> > +		break;
> > +	default:
> > +		return -1;
> > +	}
> > +
> > +	return 0;
> > +}
> 
> 
> -- 
> 雷米‧德尼-库尔蒙
> http://www.remlab.net/
> 
> 
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
  

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