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

[Yangyu Chen]

Hi,

On 5/5/23 2:14 AM, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley at 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 at 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))
>  		return -ENODEV;
> -	}
>  
>  	return 0;
>  }

What about reducing the compare string to "rv32" and "rv64" only?

Linux kernel is able to run on RV32IA or RV64IA CPUs theoretically as
someone did before[1]. In this case, M extension is not required here.
Although it's an invalid DT for now, reducing the compare string will
make the porting easier. And M extension does not provide additional
ISA-level registers that need the kernel to care about during context
switch.

In my opinion, we should concern about if someday linux will support
RV32E or RV64E. It's not necessary to validate if "ima" presents here
for better forward compatibility.

[1] https://www.facebook.com/groups/riscv.tw/posts/1611033709104137/

> 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;
>  
>  		bitmap_zero(this_isa, RISCV_ISA_EXT_MAX);

Looks good to me.

Thanks,
Yangyu Chen