Re: [PATCH v1 4/7] RISC-V: validate riscv,isa at boot, not during ISA string parsing

[Conor Dooley <conor.dooley@microchip.com>]

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

On Fri, May 05, 2023 at 09:40:22AM +0200, Andrew Jones wrote:
> On Thu, May 04, 2023 at 07:14:23PM +0100, Conor Dooley wrote:
> > From: Conor Dooley <conor.dooley@microchip.com>
> > 
> > Since riscv_fill_hwcap() now only iterates over possible cpus, the
> > basic validation of whether riscv,isa contains "rv<width>" can be moved
> > to riscv_early_of_processor_hartid().
> > 
> > Further, "ima" support is required by the kernel, so reject any CPU not
> > fitting the bill.
> >
> > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> > ---
> >  arch/riscv/kernel/cpu.c        |  8 +++++---
> >  arch/riscv/kernel/cpufeature.c | 12 ++++++------
> >  2 files changed, 11 insertions(+), 9 deletions(-)
> > 
> > diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> > index 7030a5004f8e..b0c3ec0f2f5b 100644
> > --- a/arch/riscv/kernel/cpu.c
> > +++ b/arch/riscv/kernel/cpu.c
> > @@ -63,10 +63,12 @@ int riscv_early_of_processor_hartid(struct device_node *node, unsigned long *har
> >  		pr_warn("CPU with hartid=%lu has no \"riscv,isa\" property\n", *hart);
> >  		return -ENODEV;
> >  	}
> > -	if (tolower(isa[0]) != 'r' || tolower(isa[1]) != 'v') {
> > -		pr_warn("CPU with hartid=%lu has an invalid ISA of \"%s\"\n", *hart, isa);
> > +
> > +	if (IS_ENABLED(CONFIG_32BIT) && strncasecmp(isa, "rv32ima", 7))
> > +		return -ENODEV;
> > +
> > +	if (IS_ENABLED(CONFIG_64BIT) && strncasecmp(isa, "rv64ima", 7))
> 
> 'ima' matches the DT binding pattern requirement and the order required
> by 27.11 "Subset Naming Convention".

Aye, perhaps I should have mentioned it in my commit message that those
particular orders are required by the dt-binding. If the ACPI series
lands before this one does, since I'll have to rebase anyway, I'll add a
mention of the ACPI spec's position.

> If the spec ever squeezes more single
> letter extensions into the front of the ISA string, then we can cross
> that bridge when we get to it.

Perhaps by then we have evolved beyond needing the ISA string!
(A boy can dream)

> >  		return -ENODEV;
> > -	}
> >  
> >  	return 0;
> >  }
> > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > index 3ae456413f79..a79c5c52a174 100644
> > --- a/arch/riscv/kernel/cpufeature.c
> > +++ b/arch/riscv/kernel/cpufeature.c
> > @@ -130,12 +130,12 @@ void __init riscv_fill_hwcap(void)
> >  			continue;
> >  		}
> >  
> > -		if (IS_ENABLED(CONFIG_32BIT) && strncasecmp(isa, "rv32", 4))
> > -			continue;
> > -
> > -		if (IS_ENABLED(CONFIG_64BIT) && strncasecmp(isa, "rv64", 4))
> > -			continue;
> > -
> > +		/*
> > +		 * For all possible cpus, we have already validated in
> > +		 * the boot process that they at least contain "rv" and
> > +		 * whichever of "32"/"64" this kernel supports, and so this
> > +		 * section can be skipped.
> > +		 */
> >  		isa += 4;
> 
> When we add RV128 support this will need a tweak, but that's for another
> day.

Yeah, adding 128-bit is so far off that it's not worth considering, but
easily handled in this particular location.

> Since all ISA strings must start with rvXX per the spec, then this
> works for ACPI too, which states the isa string in its ISA string table
> must conform to the unpriv spec.
> 
> >  
> >  		bitmap_zero(this_isa, RISCV_ISA_EXT_MAX);
> > -- 
> > 2.39.2
> > 
> 
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Thanks!

[-- 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