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

[Conor Dooley]

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@xxxxxxxxx/

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@xxxxxxxxxxxx>
> ---
>  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: signature.asc
Description: PGP signature