Re: [PATCH v4 5/8] RISC-V: cpufeature: Put vendor_id to work

From: Conor Dooley <conor@kernel.org>
To: Andrew Jones <ajones@ventanamicro.com>
Cc: linux-riscv@lists.infradead.org, kvm-riscv@lists.infradead.org,
	devicetree@vger.kernel.org,
	'Anup Patel ' <apatel@ventanamicro.com>,
	'Palmer Dabbelt ' <palmer@dabbelt.com>,
	'Paul Walmsley ' <paul.walmsley@sifive.com>,
	'Krzysztof Kozlowski ' <krzysztof.kozlowski+dt@linaro.org>,
	'Atish Patra ' <atishp@rivosinc.com>,
	'Heiko Stuebner ' <heiko@sntech.de>,
	'Jisheng Zhang ' <jszhang@kernel.org>,
	'Rob Herring ' <robh@kernel.org>,
	'Albert Ou ' <aou@eecs.berkeley.edu>,
	'Conor Dooley ' <conor.dooley@microchip.com>
Subject: Re: [PATCH v4 5/8] RISC-V: cpufeature: Put vendor_id to work
Date: Fri, 10 Feb 2023 20:29:15 +0000	[thread overview]
Message-ID: <Y+apG/nVSLJY8nsN@spud> (raw)
In-Reply-To: <20230210075818.chh5myc2tmcvjplc@orel>

[-- Attachment #1.1: Type: text/plain, Size: 2466 bytes --]

On Fri, Feb 10, 2023 at 08:58:18AM +0100, Andrew Jones wrote:
> On Thu, Feb 09, 2023 at 07:04:59PM +0000, Conor Dooley wrote:
> > On Thu, Feb 09, 2023 at 04:26:25PM +0100, Andrew Jones wrote:

> > > +static bool riscv_cpufeature_application_check(u32 feature, u16 data)
> > > +{
> > > +	return data == 0;
> > > +}
> > > +
> > >  void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin,
> > >  						  struct alt_entry *end,
> > >  						  unsigned int stage)
> > > @@ -289,8 +294,6 @@ void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin,
> > >  		return;
> > >  
> > >  	for (alt = begin; alt < end; alt++) {
> > > -		if (alt->vendor_id != 0)
> > > -			continue;
> > 
> > Can you remind me what makes this "safe"?
> > My understanding was that a vendor_id of zero was safe, as zero is
> > reserved in JEDEC.
> > What is stopping someone stuffing this with a given value and
> > colliding with a real vendor's errata?
> > 
> > 	for (alt = begin; alt < end; alt++) {
> > 		if (alt->vendor_id != A_VENDOR_ID)
> > 			continue;
> > 		if (alt->errata_id >= ERRATA_A_NUMBER)
> > 			continue;
> > 
> > 		tmp = (1U << alt->errata_id);
> > 		if (cpu_req_errata & tmp) {
> > 			oldptr = ALT_OLD_PTR(alt);
> > 			altptr = ALT_ALT_PTR(alt);
> > 
> > 			/* On vm-alternatives, the mmu isn't running yet */
> > 			if (stage == RISCV_ALTERNATIVES_EARLY_BOOT)
> > 				memcpy((void *)__pa_symbol(oldptr),
> > 				       (void *)__pa_symbol(altptr),
> > 				       alt->alt_len);
> > 			else
> > 				patch_text_nosync(oldptr, altptr, alt->alt_len);
> > 		}
> > 	}
> > 
> > I've probably just missing something, my brain swapped out alternatives
> > the other week. Hopefully whatever I missed isn't embarrassingly obvious!
> 
> You're right. I was assuming the errata_id space for errata didn't overlap
> with the errata_id space for cpufeatures. It doesn't, atm, but by luck,
> not design. I could try to ensure that somehow, but probably the better
> approach would be to use the upper bits of errata_id for the application
> data and to leave vendor_id alone. Thanks for catching my oversight!

Sounds like a better idea at least.
I suppose the ideal would be to add another arg and not abuse things,
but maybe that's one for the future, idk.
Is this somewhere that an assertion should be used to make sure that
we don't grow the list of extensions such that the regions collide?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv