diff mbox series

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

Message ID 20230705-thead_vendor_extensions-v1-1-ad6915349c4d@rivosinc.com (mailing list archive)
State Changes Requested
Headers show
Series RISC-V: Support querying vendor extensions | expand

Checks

Context Check Description
conchuod/cover_letter success Series has a cover letter
conchuod/tree_selection success Guessed tree name to be for-next at HEAD 6259f3443c6a
conchuod/fixes_present success Fixes tag not required for -next series
conchuod/maintainers_pattern success MAINTAINERS pattern errors before the patch: 4 and now 4
conchuod/verify_signedoff success Signed-off-by tag matches author and committer
conchuod/kdoc success Errors and warnings before: 0 this patch: 0
conchuod/build_rv64_clang_allmodconfig success Errors and warnings before: 234 this patch: 234
conchuod/module_param success Was 0 now: 0
conchuod/build_rv64_gcc_allmodconfig success Errors and warnings before: 881 this patch: 881
conchuod/build_rv32_defconfig success Build OK
conchuod/dtb_warn_rv64 success Errors and warnings before: 3 this patch: 3
conchuod/header_inline success No static functions without inline keyword in header files
conchuod/checkpatch warning CHECK: Alignment should match open parenthesis CHECK: No space is necessary after a cast CHECK: spaces preferred around that '<<' (ctx:VxV) WARNING: It's generally not useful to have the filename in the file WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
conchuod/build_rv64_nommu_k210_defconfig success Build OK
conchuod/verify_fixes success No Fixes tag
conchuod/build_rv64_nommu_virt_defconfig success Build OK

Commit Message

Charlie Jenkins July 6, 2023, 3:30 a.m. UTC 
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(-)

Comments

Rémi Denis-Courmont July 6, 2023, 4:29 p.m. UTC | #1 
Le torstaina 6. heinäkuuta 2023, 6.30.17 EEST Charlie Jenkins a écrit :
> 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)

Isn't this UB on 32-bit RISC-V ?

> 
>  #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
Conor Dooley July 6, 2023, 5:15 p.m. UTC | #2 
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.
Charlie Jenkins July 6, 2023, 7:44 p.m. UTC | #3 
On Thu, Jul 06, 2023 at 07:29:37PM +0300, Rémi Denis-Courmont wrote:
> Le torstaina 6. heinäkuuta 2023, 6.30.17 EEST Charlie Jenkins a écrit :
> > 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)
> 
> Isn't this UB on 32-bit RISC-V ?
Hmm, yeah it would be, but the hwprobe syscall is based on a 64 bit key
using the __s64 type. There could be an alternative vendor space at the
top of 32-bits for those machines.
> 
> > 
> >  #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
> 
> 
> -- 
> レミ・デニ-クールモン
> http://www.remlab.net/
> 
> 
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Charlie Jenkins July 6, 2023, 7:51 p.m. UTC | #4 
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.
Rémi Denis-Courmont July 6, 2023, 8:04 p.m. UTC | #5 
Le torstaina 6. heinäkuuta 2023, 22.44.07 EEST Charlie Jenkins a écrit :
> On Thu, Jul 06, 2023 at 07:29:37PM +0300, Rémi Denis-Courmont wrote:
> > Le torstaina 6. heinäkuuta 2023, 6.30.17 EEST Charlie Jenkins a écrit :
> > > 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)
> > 
> > Isn't this UB on 32-bit RISC-V ?
> 
> Hmm, yeah it would be, but the hwprobe syscall is based on a 64 bit key
> using the __s64 type. There could be an alternative vendor space at the
> top of 32-bits for those machines.

Key values inside the struct are 64-bit even on RV32, but your key definition 
here is 32-bit. That can't work.

Plus this needs to be defined under uapi/ as the rest of the hwprobe constants.

And then, even on RV64, 1UL << 63 is not a valid signed constant. Kernel code 
might tolerate it, but that will most likely cause warnings or errors in the 
potentially stricter compilation environments of user-space code.

> > >  #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
Conor Dooley July 6, 2023, 8:43 p.m. UTC | #6 
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.
diff mbox series

Patch

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