Re: [PATCH 1/3] RISC-V: Improve use of isa2hwcap[]

From: Conor Dooley <conor.dooley@microchip.com>
To: Andrew Jones <ajones@ventanamicro.com>
Cc: Conor Dooley <conor@kernel.org>,
	<linux-riscv@lists.infradead.org>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	Albert Ou <aou@eecs.berkeley.edu>,
	Heiko Stuebner <heiko@sntech.de>,
	Anup Patel <apatel@ventanamicro.com>,
	Atish Patra <atishp@rivosinc.com>
Subject: Re: [PATCH 1/3] RISC-V: Improve use of isa2hwcap[]
Date: Mon, 24 Oct 2022 08:16:53 +0100	[thread overview]
Message-ID: <Y1Y75dyTeqVAIzNn@wendy> (raw)
In-Reply-To: <20221024064811.rtu4nsia23yuga7w@kamzik>

On Mon, Oct 24, 2022 at 08:48:11AM +0200, Andrew Jones wrote:
> On Sun, Oct 23, 2022 at 08:28:20PM +0100, Conor Dooley wrote:
> > On Fri, Oct 21, 2022 at 12:59:03PM +0200, Andrew Jones wrote:
> > > Improve isa2hwcap[] by removing it from static storage, as
> > > riscv_fill_hwcap() is only called once, and by reducing its size
> > > from 256 bytes to 26. The latter improvement is possible because
> > > isa2hwcap[] will never be indexed with capital letters and we can
> > > precompute the offsets from 'a'.
> > 
> > Hey Drew, couple questions for you - mostly due to naivety I think..
> > 
> > How do we know that isa2hwcap will never interact with capital letters?
> > It pulls the isa string from dt and the no-capitals enforcement comes
> > from there since one with capitals is invalid? I didn't dig particularly
> > deeply into the code, but is there a risk that we regress some user that
> > has a dt with capitals in the isa string? Or is that a "your dt was wrong
> > and you're out-of-tree so that's your problem" situation?
> 
> Our ISA string parser here in riscv_fill_hwcap() ignores non-capital
> letters, see arch/riscv/kernel/cpufeature.c lines 141, 165 and 196.

Ah, I missed the one on L165, that's a bit silly... Apologies!

> (Maybe we should be noisier about that in case there are DTs out there
> with capitals which aren't realizing they're being ignored.)

Possibly, but it's not this patch that has introduced the behaviour.
Checking very briefly though, looks like it was 2a31c54be097 ("RISC-V:
Minimal parser for "riscv, isa" strings") that introduced the checks
and that was not really all that long ago..

> 
> > 
> > Secondly, in the UAPI header, the COMPAT_HWCAP_ISA_FOO defines are
> > computed as I - A rather than i - a. Should those be changed too for the
> > sake of consistently using the lowercase everywhere, or do you think
> > that doesn't really matter?
> 
> I think I'd leave it since it doesn't change the math and I'm reluctant
> to churn UAPI.

Yeah, figured you'd be of that opinion.
Anyways, only "issue" I had was my own confusion so
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Thanks...

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