linux-riscv.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2  0/2] Cleanup isa string access and print
@ 2019-10-09 22:00 Atish Patra
  2019-10-09 22:00 ` [PATCH v2 1/2] RISC-V: Remove unsupported isa string info print Atish Patra
  2019-10-09 22:00 ` [PATCH v2 2/2] RISC-V: Consolidate isa correctness check Atish Patra
  0 siblings, 2 replies; 8+ messages in thread
From: Atish Patra @ 2019-10-09 22:00 UTC (permalink / raw)
  To: linux-kernel
  Cc: Albert Ou, Richard Fontana, Greg Kroah-Hartman, Palmer Dabbelt,
	Johan Hovold, Atish Patra, Paul Walmsley, Anup Patel,
	linux-riscv, Enrico Weigelt, Thomas Gleixner, Allison Randal

This is a cleanup series addressing issues around isa string accesses
and prints. Patch 1 is actually a revised patch as a result of discussion
in the following thread.

http://lists.infradead.org/pipermail/linux-riscv/2019-September/006702.html

Patch 2 is an additional cleanup that tries to consolidate all isa
string related checks.

Changes from v1->v2
1. Used IS_ENABLED instead of #if defined
2. Adding additional warning statement incase of invalid isa string

Atish Patra (2):
RISC-V: Remove unsupported isa string info print
RISC-V: Consolidate isa correctness check

arch/riscv/include/asm/processor.h |  1 +
arch/riscv/kernel/cpu.c            | 86 ++++++++++++------------------
arch/riscv/kernel/cpufeature.c     |  4 +-
arch/riscv/kernel/smpboot.c        |  4 ++
4 files changed, 40 insertions(+), 55 deletions(-)

--
2.21.0


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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2  1/2] RISC-V: Remove unsupported isa string info print
  2019-10-09 22:00 [PATCH v2 0/2] Cleanup isa string access and print Atish Patra
@ 2019-10-09 22:00 ` Atish Patra
  2019-10-18  8:19   ` Paul Walmsley
  2019-10-09 22:00 ` [PATCH v2 2/2] RISC-V: Consolidate isa correctness check Atish Patra
  1 sibling, 1 reply; 8+ messages in thread
From: Atish Patra @ 2019-10-09 22:00 UTC (permalink / raw)
  To: linux-kernel
  Cc: Albert Ou, Richard Fontana, Anup Patel, Palmer Dabbelt,
	Johan Hovold, Atish Patra, Thomas Gleixner, Paul Walmsley,
	Greg Kroah-Hartman, linux-riscv, Enrico Weigelt,
	Christoph Hellwig, Allison Randal

/proc/cpuinfo should just print all the isa string as an information
instead of determining what is supported or not. ELF hwcap can be
used by the userspace to figure out that.

Simplify the isa string printing by removing the unsupported isa string
print and all related code.

The relevant discussion can be found at
http://lists.infradead.org/pipermail/linux-riscv/2019-September/006702.html

Signed-off-by: Atish Patra <atish.patra@wdc.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 arch/riscv/kernel/cpu.c | 45 +++--------------------------------------
 1 file changed, 3 insertions(+), 42 deletions(-)

diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
index 7da3c6a93abd..40a3c442ac5f 100644
--- a/arch/riscv/kernel/cpu.c
+++ b/arch/riscv/kernel/cpu.c
@@ -46,51 +46,12 @@ int riscv_of_processor_hartid(struct device_node *node)
 
 #ifdef CONFIG_PROC_FS
 
-static void print_isa(struct seq_file *f, const char *orig_isa)
+static void print_isa(struct seq_file *f, const char *isa)
 {
-	static const char *ext = "mafdcsu";
-	const char *isa = orig_isa;
-	const char *e;
-
-	/*
-	 * 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 defined(CONFIG_32BIT)
-	if (strncmp(isa, "rv32i", 5) != 0)
-		return;
-#elif defined(CONFIG_64BIT)
-	if (strncmp(isa, "rv64i", 5) != 0)
-		return;
-#endif
-
-	/* Print the base ISA, as we already know it's legal. */
+	/* Print the entire ISA as it is */
 	seq_puts(f, "isa\t\t: ");
-	seq_write(f, isa, 5);
-	isa += 5;
-
-	/*
-	 * Check the rest of the ISA string for valid extensions, printing those
-	 * we find.  RISC-V ISA strings define an order, so we only print the
-	 * extension bits when they're in order. Hide the supervisor (S)
-	 * extension from userspace as it's not accessible from there.
-	 */
-	for (e = ext; *e != '\0'; ++e) {
-		if (isa[0] == e[0]) {
-			if (isa[0] != 's')
-				seq_write(f, isa, 1);
-
-			isa++;
-		}
-	}
+	seq_write(f, isa, strlen(isa));
 	seq_puts(f, "\n");
-
-	/*
-	 * If we were given an unsupported ISA in the device tree then print
-	 * a bit of info describing what went wrong.
-	 */
-	if (isa[0] != '\0')
-		pr_info("unsupported ISA \"%s\" in device tree\n", orig_isa);
 }
 
 static void print_mmu(struct seq_file *f, const char *mmu_type)
-- 
2.21.0


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

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2  2/2] RISC-V: Consolidate isa correctness check
  2019-10-09 22:00 [PATCH v2 0/2] Cleanup isa string access and print Atish Patra
  2019-10-09 22:00 ` [PATCH v2 1/2] RISC-V: Remove unsupported isa string info print Atish Patra
@ 2019-10-09 22:00 ` Atish Patra
  2019-10-18  8:43   ` Paul Walmsley
  1 sibling, 1 reply; 8+ messages in thread
From: Atish Patra @ 2019-10-09 22:00 UTC (permalink / raw)
  To: linux-kernel
  Cc: Albert Ou, Richard Fontana, Greg Kroah-Hartman, Palmer Dabbelt,
	Johan Hovold, Atish Patra, Paul Walmsley, Anup Patel,
	linux-riscv, Enrico Weigelt, Thomas Gleixner, Allison Randal

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


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

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2  1/2] RISC-V: Remove unsupported isa string info print
  2019-10-09 22:00 ` [PATCH v2 1/2] RISC-V: Remove unsupported isa string info print Atish Patra
@ 2019-10-18  8:19   ` Paul Walmsley
  0 siblings, 0 replies; 8+ messages in thread
From: Paul Walmsley @ 2019-10-18  8:19 UTC (permalink / raw)
  To: Atish Patra
  Cc: Albert Ou, Anup Patel, Palmer Dabbelt, linux-kernel,
	Johan Hovold, Richard Fontana, Thomas Gleixner,
	Greg Kroah-Hartman, linux-riscv, Enrico Weigelt,
	Christoph Hellwig, Allison Randal

On Wed, 9 Oct 2019, Atish Patra wrote:

> /proc/cpuinfo should just print all the isa string as an information
> instead of determining what is supported or not. ELF hwcap can be
> used by the userspace to figure out that.
> 
> Simplify the isa string printing by removing the unsupported isa string
> print and all related code.
> 
> The relevant discussion can be found at
> http://lists.infradead.org/pipermail/linux-riscv/2019-September/006702.html
> 
> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Thanks, queued for v5.5-rc1.


- Paul

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2  2/2] RISC-V: Consolidate isa correctness check
  2019-10-09 22:00 ` [PATCH v2 2/2] RISC-V: Consolidate isa correctness check Atish Patra
@ 2019-10-18  8:43   ` Paul Walmsley
  2019-10-18 17:53     ` Atish Patra
  0 siblings, 1 reply; 8+ messages in thread
From: Paul Walmsley @ 2019-10-18  8:43 UTC (permalink / raw)
  To: Atish Patra
  Cc: Albert Ou, Anup Patel, Palmer Dabbelt, linux-kernel,
	Johan Hovold, Richard Fontana, Greg Kroah-Hartman, linux-riscv,
	Enrico Weigelt, Thomas Gleixner, Allison Randal

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

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2  2/2] RISC-V: Consolidate isa correctness check
  2019-10-18  8:43   ` Paul Walmsley
@ 2019-10-18 17:53     ` Atish Patra
  2019-10-18 18:25       ` Paul Walmsley
  0 siblings, 1 reply; 8+ messages in thread
From: Atish Patra @ 2019-10-18 17:53 UTC (permalink / raw)
  To: paul.walmsley
  Cc: aou, gregkh, palmer, johan, linux-kernel, rfontana, anup,
	linux-riscv, info, tglx, allison

On Fri, 2019-10-18 at 01:43 -0700, Paul Walmsley wrote:
> 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?
> 

I had to add the check in riscv_fill_hwcap() because that function is
iterating over all cpu nodes to set the hwcap. Thus, some of the harts
that are not available due to incorrect isa string can affect hwcap.

We can check cpu_possible_mask to figure out the harts with invalid isa
strings but that will perform poorly as RISC-V have more harts in
future.


> 
> - 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
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2  2/2] RISC-V: Consolidate isa correctness check
  2019-10-18 17:53     ` Atish Patra
@ 2019-10-18 18:25       ` Paul Walmsley
  2019-11-04 23:06         ` Atish Patra
  0 siblings, 1 reply; 8+ messages in thread
From: Paul Walmsley @ 2019-10-18 18:25 UTC (permalink / raw)
  To: Atish Patra
  Cc: aou, gregkh, palmer, johan, linux-kernel, rfontana, anup,
	linux-riscv, info, tglx, allison

On Fri, 18 Oct 2019, Atish Patra wrote:

> On Fri, 2019-10-18 at 01:43 -0700, Paul Walmsley wrote:
> > 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?
> > 
> 
> I had to add the check in riscv_fill_hwcap() because that function is
> iterating over all cpu nodes to set the hwcap. Thus, some of the harts
> that are not available due to incorrect isa string can affect hwcap.
> 
> We can check cpu_possible_mask to figure out the harts with invalid isa
> strings but that will perform poorly as RISC-V have more harts in
> future.

How about just calling riscv_read_check_isa() once for all harts and 
leaving riscv_fill_hwcap() the way it was?  You'll probably need to hoist 
the earlier call out of setup_smp(), so it still is called when 
!CONFIG_SMP.


- Paul

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2  2/2] RISC-V: Consolidate isa correctness check
  2019-10-18 18:25       ` Paul Walmsley
@ 2019-11-04 23:06         ` Atish Patra
  0 siblings, 0 replies; 8+ messages in thread
From: Atish Patra @ 2019-11-04 23:06 UTC (permalink / raw)
  To: paul.walmsley
  Cc: aou, gregkh, palmer, johan, linux-kernel, rfontana, anup,
	linux-riscv, info, tglx, allison

On Fri, 2019-10-18 at 11:25 -0700, Paul Walmsley wrote:
> On Fri, 18 Oct 2019, Atish Patra wrote:
> 
> > On Fri, 2019-10-18 at 01:43 -0700, Paul Walmsley wrote:
> > > 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?
> > > 
> > 
> > I had to add the check in riscv_fill_hwcap() because that function
> > is
> > iterating over all cpu nodes to set the hwcap. Thus, some of the
> > harts
> > that are not available due to incorrect isa string can affect
> > hwcap.
> > 
> > We can check cpu_possible_mask to figure out the harts with invalid
> > isa
> > strings but that will perform poorly as RISC-V have more harts in
> > future.
> 
> How about just calling riscv_read_check_isa() once for all harts and 
> leaving riscv_fill_hwcap() the way it was? 
>  You'll probably need to hoist 
> the earlier call out of setup_smp(), so it still is called when 
> !CONFIG_SMP.

Currently, it doesn't let boot any cpu with incorrect isa string for
smp usecase. We still need to preserve that usecase. I think
setup_smp() use is unavoidable.

If the boot cpu has incorrect isa info for !CONFIG_SMP, I guess we
should halt the boot with BUG_ON. This is a separate
riscv_read_check_isa call with boot hart device node.

This is what we can do:

Maintain a global cpumask of harts with invalid isa strings which would
be set during early bootup (before setup_smp). This cpumask will be
used in setup_smp() and riscv_fill_hwcap() to avoid using harts with
invalid isa. This will make sure that there is only single invocaiton
of riscv_read_check_isa(). In most cases, this cpumask will be empty
and penalty of cpumask check won't matter.

Is that what you had in mind or any other approach to address all 3
usecases ? 

> 
> - Paul

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2019-11-04 23:07 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-09 22:00 [PATCH v2 0/2] Cleanup isa string access and print Atish Patra
2019-10-09 22:00 ` [PATCH v2 1/2] RISC-V: Remove unsupported isa string info print Atish Patra
2019-10-18  8:19   ` Paul Walmsley
2019-10-09 22:00 ` [PATCH v2 2/2] RISC-V: Consolidate isa correctness check Atish Patra
2019-10-18  8:43   ` Paul Walmsley
2019-10-18 17:53     ` Atish Patra
2019-10-18 18:25       ` Paul Walmsley
2019-11-04 23:06         ` Atish Patra

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).