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

[Conor Dooley]

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