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

[Conor Dooley]

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