diff mbox series

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

Message ID 20230504-sultry-frostlike-9dbf19333725@spud (mailing list archive)
State Superseded
Delegated to: Palmer Dabbelt
Headers show
Series ISA string parser cleanups++ | expand

Checks

Context Check Description
conchuod/tree_selection fail Failed to apply to next/pending-fixes or riscv/for-next

Commit Message

Conor Dooley May 4, 2023, 6:14 p.m. UTC 
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(-)

Comments

Andrew Jones May 5, 2023, 7:40 a.m. UTC | #1 
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". 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.

>  		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. 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,
drew
Conor Dooley May 5, 2023, 7:51 a.m. UTC | #2 
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!
Yangyu Chen May 5, 2023, 12:40 p.m. UTC | #3 
Hi,

On 5/5/23 2:14 AM, 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))
>  		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
Conor Dooley May 5, 2023, 1:04 p.m. UTC | #4 
On Fri, May 05, 2023 at 08:40:22PM +0800, Yangyu Chen wrote:
> On 5/5/23 2:14 AM, 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))
> >  		return -ENODEV;
> > -	}
> >  
> >  	return 0;
> >  }
> 
> What about reducing the compare string to "rv32" and "rv64" only?

I explicitly wanted the "ima" to rule out harts that might've been
left enabled but we cannot run on.

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

I had a look, and as expected, this requires a makefile change to remove
the M extension from -march.
If that change ever lands in mainline the check here could be modified
too, but otherwise I don't think we need to be concerned about what some
out of tree code is doing.

Cheers,
Conor.
diff mbox series

Patch

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;
 }
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);