Re: [PATCH v2 2/2] RISC-V: Consolidate isa correctness check

[Paul Walmsley <paul.walmsley@sifive.com>]

On Wed, 9 Oct 2019, Atish Patra wrote:

> Currently, isa string is read and checked for correctness at multiple
> places.
> 
> Consolidate them into one function and use it only during early bootup.
> In case of a incorrect isa string, the cpu shouldn't boot at all.
> 
> Signed-off-by: Atish Patra <atish.patra@wdc.com>

Looks like riscv_read_check_isa() is called twice for each hart.  Is there 
any way to call it only once per hart?


- Paul

> ---
>  arch/riscv/include/asm/processor.h |  1 +
>  arch/riscv/kernel/cpu.c            | 41 ++++++++++++++++++++++--------
>  arch/riscv/kernel/cpufeature.c     |  4 +--
>  arch/riscv/kernel/smpboot.c        |  4 +++
>  4 files changed, 37 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
> index f539149d04c2..189bf98f9a3f 100644
> --- a/arch/riscv/include/asm/processor.h
> +++ b/arch/riscv/include/asm/processor.h
> @@ -74,6 +74,7 @@ static inline void wait_for_interrupt(void)
>  }
>  
>  struct device_node;
> +int riscv_read_check_isa(struct device_node *node, const char **isa);
>  int riscv_of_processor_hartid(struct device_node *node);
>  
>  extern void riscv_fill_hwcap(void);
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index 40a3c442ac5f..6bd4c7176bf6 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -8,13 +8,43 @@
>  #include <linux/of.h>
>  #include <asm/smp.h>
>  
> +int riscv_read_check_isa(struct device_node *node, const char **isa)
> +{
> +	u32 hart;
> +
> +	if (of_property_read_u32(node, "reg", &hart)) {
> +		pr_warn("Found CPU without hart ID\n");
> +		return -ENODEV;
> +	}
> +
> +	if (of_property_read_string(node, "riscv,isa", isa)) {
> +		pr_warn("CPU with hartid=%d has no \"riscv,isa\" property\n",
> +			hart);
> +		return -ENODEV;
> +	}
> +	/*
> +	 * Linux doesn't support rv32e or rv128i, and we only support booting
> +	 * kernels on harts with the same ISA that the kernel is compiled for.
> +	 */
> +	if (IS_ENABLED(CONFIG_32BIT) && (strncmp(*isa, "rv32i", 5) != 0)) {
> +		pr_warn("hartid=%d has an invalid ISA \"%s\" for 32bit config\n",
> +			hart, *isa);
> +		return -ENODEV;
> +	} else if (IS_ENABLED(CONFIG_64BIT) &&
> +		  (strncmp(*isa, "rv64i", 5) != 0)) {
> +		pr_warn("hartid=%d has an invalid ISA \"%s\" for 64bit config\n",
> +			hart, *isa);
> +		return -ENODEV;
> +	}
> +	return 0;
> +}
> +
>  /*
>   * Returns the hart ID of the given device tree node, or -ENODEV if the node
>   * isn't an enabled and valid RISC-V hart node.
>   */
>  int riscv_of_processor_hartid(struct device_node *node)
>  {
> -	const char *isa;
>  	u32 hart;
>  
>  	if (!of_device_is_compatible(node, "riscv")) {
> @@ -32,15 +62,6 @@ int riscv_of_processor_hartid(struct device_node *node)
>  		return -ENODEV;
>  	}
>  
> -	if (of_property_read_string(node, "riscv,isa", &isa)) {
> -		pr_warn("CPU with hartid=%d has no \"riscv,isa\" property\n", hart);
> -		return -ENODEV;
> -	}
> -	if (isa[0] != 'r' || isa[1] != 'v') {
> -		pr_warn("CPU with hartid=%d has an invalid ISA of \"%s\"\n", hart, isa);
> -		return -ENODEV;
> -	}
> -
>  	return hart;
>  }
>  
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index b1ade9a49347..eaad5aa07403 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -38,10 +38,8 @@ void riscv_fill_hwcap(void)
>  		if (riscv_of_processor_hartid(node) < 0)
>  			continue;
>  
> -		if (of_property_read_string(node, "riscv,isa", &isa)) {
> -			pr_warn("Unable to find \"riscv,isa\" devicetree entry\n");
> +		if (riscv_read_check_isa(node, &isa) < 0)
>  			continue;
> -		}
>  
>  		for (i = 0; i < strlen(isa); ++i)
>  			this_hwcap |= isa2hwcap[(unsigned char)(isa[i])];
> diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
> index 18ae6da5115e..15ee71297abf 100644
> --- a/arch/riscv/kernel/smpboot.c
> +++ b/arch/riscv/kernel/smpboot.c
> @@ -60,12 +60,16 @@ void __init setup_smp(void)
>  	int hart;
>  	bool found_boot_cpu = false;
>  	int cpuid = 1;
> +	const char *isa;
>  
>  	for_each_of_cpu_node(dn) {
>  		hart = riscv_of_processor_hartid(dn);
>  		if (hart < 0)
>  			continue;
>  
> +		if (riscv_read_check_isa(dn, &isa) < 0)
> +			continue;
> +
>  		if (hart == cpuid_to_hartid_map(0)) {
>  			BUG_ON(found_boot_cpu);
>  			found_boot_cpu = 1;
> -- 
> 2.21.0
> 
> 


- Paul