[PATCH v2 0/2] riscv: allow case-insensitive ISA string parsing

[PATCH v2 0/2] riscv: allow case-insensitive ISA string parsing

[Yangyu Chen]

This patchset allows case-insensitive ISA string parsing, which is
needed in the ACPI environment. As the RISC-V Hart Capabilities Table
(RHCT) description in UEFI Forum ECR[1] shows the format of the ISA
string is defined in the RISC-V unprivileged specification[2]. However,
the RISC-V unprivileged specification defines the ISA naming strings are
case-insensitive while the current ISA string parser in the kernel only
accepts lowercase letters. In this case, the kernel should allow
case-insensitive ISA string parsing. Moreover, this reason has been
discussed in Conor's patch[3]. And I have also checked the current ISA
string parsing in the recent ACPI support patch[4] will also call
`riscv_fill_hwcap` function as DT we use now.

The original motivation for my patch v1[5] is that some SoC generators
will provide generated DT with illegal ISA string in dt-binding such as
rocket-chip, which will even cause kernel panic in some cases as I
mentioned in v1[5]. Now, the rocket-chip has been fixed in PR #3333[6].
However, when using some specific version of rocket-chip with
illegal ISA string in DT, this patchset will also work for parsing
uppercase letters correctly in DT, thus will have better compatibility.

In summary, this patch not only works for case-insensitive ISA string
parsing to meet the requirements in ECR[1] but also can be a workaround
for some specific versions of rocket-chip.

[1] https://drive.google.com/file/d/1nP3nFiH4jkPMp6COOxP6123DCZKR-tia/view
[2] https://github.com/riscv/riscv-isa-manual/blob/main/src/naming.adoc
[3] https://lore.kernel.org/all/20230426-getting-tactile-e6cee2cdf870@spud/
[4] https://lore.kernel.org/linux-riscv/20230404182037.863533-14-sunilvl@ventanamicro.com/
[5] https://lore.kernel.org/all/tencent_1647475C9618C390BEC601BE2CC1206D0C07@qq.com/
[6] https://github.com/chipsalliance/rocket-chip/pull/3333

Changes since v1:
* Remove convert all isa string to lowercase letters in `print_isa`
* Remove warp parser pointer dereference with tolower in switch, use
    uppercase letter case instead in `riscv_fill_hwcap`
* Remove allow uppercase letters in dt-bindings
* Add Conor Dooley's patch which drops invalid comment about riscv,isa
    lower-case reasoning

Conor Dooley (1):
  dt-bindings: riscv: drop invalid comment about riscv,isa lower-case
    reasoning

Yangyu Chen (1):
  riscv: allow case-insensitive ISA string parsing

 .../devicetree/bindings/riscv/cpus.yaml       |  2 +-
 arch/riscv/kernel/cpu.c                       |  3 ++-
 arch/riscv/kernel/cpufeature.c                | 25 +++++++++----------
 3 files changed, 15 insertions(+), 15 deletions(-)

-- 
2.40.0

[PATCH v2 1/2] riscv: allow case-insensitive ISA string parsing

[Yangyu Chen]

According to RISC-V Hart Capabilities Table (RHCT) description in UEFI
Forum ECR, the format of the ISA string is defined in the RISC-V
unprivileged specification which is case-insensitive. However, the
current ISA string parser in the kernel does not support ISA strings
with uppercase letters.

This patch modifies the ISA string parser in the kernel to support
case-insensitive ISA string parsing.

Signed-off-by: Yangyu Chen <cyy@cyyself.name>
---
 arch/riscv/kernel/cpu.c        |  3 ++-
 arch/riscv/kernel/cpufeature.c | 25 ++++++++++++-------------
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
index 8400f0cc9704..52b92a267121 100644
--- a/arch/riscv/kernel/cpu.c
+++ b/arch/riscv/kernel/cpu.c
@@ -4,6 +4,7 @@
  */
 
 #include <linux/cpu.h>
+#include <linux/ctype.h>
 #include <linux/init.h>
 #include <linux/seq_file.h>
 #include <linux/of.h>
@@ -41,7 +42,7 @@ int riscv_of_processor_hartid(struct device_node *node, unsigned long *hart)
 		pr_warn("CPU with hartid=%lu has no \"riscv,isa\" property\n", *hart);
 		return -ENODEV;
 	}
-	if (isa[0] != 'r' || isa[1] != 'v') {
+	if (tolower(isa[0]) != 'r' || tolower(isa[1]) != 'v') {
 		pr_warn("CPU with hartid=%lu has an invalid ISA of \"%s\"\n", *hart, isa);
 		return -ENODEV;
 	}
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 59d58ee0f68d..d1991c12e546 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -119,13 +119,10 @@ void __init riscv_fill_hwcap(void)
 		}
 
 		temp = isa;
-#if IS_ENABLED(CONFIG_32BIT)
-		if (!strncmp(isa, "rv32", 4))
+		if (IS_ENABLED(CONFIG_32BIT) && !strncasecmp(isa, "rv32", 4))
 			isa += 4;
-#elif IS_ENABLED(CONFIG_64BIT)
-		if (!strncmp(isa, "rv64", 4))
+		else if (IS_ENABLED(CONFIG_64BIT) && !strncasecmp(isa, "rv64", 4))
 			isa += 4;
-#endif
 		/* The riscv,isa DT property must start with rv64 or rv32 */
 		if (temp == isa)
 			continue;
@@ -136,6 +133,7 @@ void __init riscv_fill_hwcap(void)
 			bool ext_long = false, ext_err = false;
 
 			switch (*ext) {
+			case 'S':
 			case 's':
 				/**
 				 * Workaround for invalid single-letter 's' & 'u'(QEMU).
@@ -143,19 +141,20 @@ void __init riscv_fill_hwcap(void)
 				 * not valid ISA extensions. It works until multi-letter
 				 * extension starting with "Su" appears.
 				 */
-				if (ext[-1] != '_' && ext[1] == 'u') {
+				if (ext[-1] != '_' && tolower(ext[1]) == 'u') {
 					++isa;
 					ext_err = true;
 					break;
 				}
 				fallthrough;
+			case 'X':
 			case 'x':
+			case 'Z':
 			case 'z':
 				ext_long = true;
 				/* Multi-letter extension must be delimited */
 				for (; *isa && *isa != '_'; ++isa)
-					if (unlikely(!islower(*isa)
-						     && !isdigit(*isa)))
+					if (unlikely(!isalnum(*isa)))
 						ext_err = true;
 				/* Parse backwards */
 				ext_end = isa;
@@ -166,7 +165,7 @@ void __init riscv_fill_hwcap(void)
 				/* Skip the minor version */
 				while (isdigit(*--ext_end))
 					;
-				if (ext_end[0] != 'p'
+				if (tolower(ext_end[0]) != 'p'
 				    || !isdigit(ext_end[-1])) {
 					/* Advance it to offset the pre-decrement */
 					++ext_end;
@@ -178,7 +177,7 @@ void __init riscv_fill_hwcap(void)
 				++ext_end;
 				break;
 			default:
-				if (unlikely(!islower(*ext))) {
+				if (unlikely(!isalpha(*ext))) {
 					ext_err = true;
 					break;
 				}
@@ -188,7 +187,7 @@ void __init riscv_fill_hwcap(void)
 				/* Skip the minor version */
 				while (isdigit(*++isa))
 					;
-				if (*isa != 'p')
+				if (tolower(*isa) != 'p')
 					break;
 				if (!isdigit(*++isa)) {
 					--isa;
@@ -205,7 +204,7 @@ void __init riscv_fill_hwcap(void)
 #define SET_ISA_EXT_MAP(name, bit)						\
 			do {							\
 				if ((ext_end - ext == sizeof(name) - 1) &&	\
-				     !memcmp(ext, name, sizeof(name) - 1) &&	\
+				     !strncasecmp(ext, name, sizeof(name) - 1) &&	\
 				     riscv_isa_extension_check(bit))		\
 					set_bit(bit, this_isa);			\
 			} while (false)						\
@@ -213,7 +212,7 @@ void __init riscv_fill_hwcap(void)
 			if (unlikely(ext_err))
 				continue;
 			if (!ext_long) {
-				int nr = *ext - 'a';
+				int nr = tolower(*ext) - 'a';
 
 				if (riscv_isa_extension_check(nr)) {
 					this_hwcap |= isa2hwcap[nr];
-- 
2.40.0

[PATCH v2 2/2] dt-bindings: riscv: drop invalid comment about riscv,isa lower-case reasoning

[Yangyu Chen]

From: Conor Dooley <conor.dooley@microchip.com>

"Ease of parsing" may have been the initial argument for keeping this
string in lower-case, but parsers may have been written that expect
lower-case only.
For example, the one in released kernels currently does not behave
correctly for multi-letter extensions that begin with a capital letter.
Allowing upper-case here brings about no benefit but would break
compatibility between new devicetrees and older kernels.

Drop the comment to avoid confusing people.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 Documentation/devicetree/bindings/riscv/cpus.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/riscv/cpus.yaml b/Documentation/devicetree/bindings/riscv/cpus.yaml
index 001931d526ec..1ee97621d0c7 100644
--- a/Documentation/devicetree/bindings/riscv/cpus.yaml
+++ b/Documentation/devicetree/bindings/riscv/cpus.yaml
@@ -81,7 +81,7 @@ properties:
 
       While the isa strings in ISA specification are case
       insensitive, letters in the riscv,isa string must be all
-      lowercase to simplify parsing.
+      lowercase.
     $ref: "/schemas/types.yaml#/definitions/string"
     pattern: ^rv(?:64|32)imaf?d?q?c?b?k?j?p?v?h?(?:[hsxz](?:[a-z])+)?(?:_[hsxz](?:[a-z])+)*$
 
-- 
2.40.0

Re: [PATCH v2 0/2] riscv: allow case-insensitive ISA string parsing

[Yangyu Chen]

Sorry for the cover letter being detached with patches. My mailing
service provider will override the Message-IDs which caused this 
trouble. I will send the cover letter first to get the correct Message-ID
before sending the rest of the patches the next time.

The cover letter is at https://lore.kernel.org/linux-riscv/tencent_8492B68063042E768C758871A3171FBD2006@qq.com/
The patches is at https://lore.kernel.org/linux-riscv/tencent_85F69423082E524C478844E31D5F8920A506@qq.com/

Re: [PATCH v2 2/2] dt-bindings: riscv: drop invalid comment about riscv,isa lower-case reasoning

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1753 bytes --]

On Fri, Apr 28, 2023 at 10:16:01PM +0800, Yangyu Chen wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> "Ease of parsing" may have been the initial argument for keeping this
> string in lower-case, but parsers may have been written that expect
> lower-case only.
> For example, the one in released kernels currently does not behave
> correctly for multi-letter extensions that begin with a capital letter.
> Allowing upper-case here brings about no benefit but would break
> compatibility between new devicetrees and older kernels.
> 
> Drop the comment to avoid confusing people.
> 
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>

You missed an Acked-by from Rob here:
https://lore.kernel.org/all/168261158214.3107331.4410018416833510357.robh@kernel.org/

Also, when you are submitting a patch authored by another person, you
need to append your Signed-off-by to the patch ;)

Cheers,
Conor.


> ---
>  Documentation/devicetree/bindings/riscv/cpus.yaml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/riscv/cpus.yaml b/Documentation/devicetree/bindings/riscv/cpus.yaml
> index 001931d526ec..1ee97621d0c7 100644
> --- a/Documentation/devicetree/bindings/riscv/cpus.yaml
> +++ b/Documentation/devicetree/bindings/riscv/cpus.yaml
> @@ -81,7 +81,7 @@ properties:
>  
>        While the isa strings in ISA specification are case
>        insensitive, letters in the riscv,isa string must be all
> -      lowercase to simplify parsing.
> +      lowercase.
>      $ref: "/schemas/types.yaml#/definitions/string"
>      pattern: ^rv(?:64|32)imaf?d?q?c?b?k?j?p?v?h?(?:[hsxz](?:[a-z])+)?(?:_[hsxz](?:[a-z])+)*$
>  
> -- 
> 2.40.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 0/2] riscv: allow case-insensitive ISA string parsing

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 668 bytes --]

On Sat, Apr 29, 2023 at 12:26:00AM +0800, Yangyu Chen wrote:
> Sorry for the cover letter being detached with patches. My mailing
> service provider will override the Message-IDs which caused this 
> trouble. I will send the cover letter first to get the correct Message-ID
> before sending the rest of the patches the next time.

At least it doesn't overwrite the in-reply-to headers also, so the
patches are at least threaded.

> The cover letter is at https://lore.kernel.org/linux-riscv/tencent_8492B68063042E768C758871A3171FBD2006@qq.com/
> The patches is at https://lore.kernel.org/linux-riscv/tencent_85F69423082E524C478844E31D5F8920A506@qq.com/
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 1/2] riscv: allow case-insensitive ISA string parsing

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1957 bytes --]

Hey Yangyu,

On Fri, Apr 28, 2023 at 10:16:00PM +0800, Yangyu Chen wrote:
> According to RISC-V Hart Capabilities Table (RHCT) description in UEFI
> Forum ECR, the format of the ISA string is defined in the RISC-V
> unprivileged specification which is case-insensitive. However, the
> current ISA string parser in the kernel does not support ISA strings
> with uppercase letters.
> 
> This patch modifies the ISA string parser in the kernel to support
> case-insensitive ISA string parsing.

@Palmer, @Sunil
Just to note, we probably should get this applied *before* we enable ACPI,
right?

> 
> Signed-off-by: Yangyu Chen <cyy@cyyself.name>
> ---
>  arch/riscv/kernel/cpu.c        |  3 ++-
>  arch/riscv/kernel/cpufeature.c | 25 ++++++++++++-------------
>  2 files changed, 14 insertions(+), 14 deletions(-)

> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 59d58ee0f68d..d1991c12e546 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -119,13 +119,10 @@ void __init riscv_fill_hwcap(void)
>  		}
>  
>  		temp = isa;
> -#if IS_ENABLED(CONFIG_32BIT)
> -		if (!strncmp(isa, "rv32", 4))
> +		if (IS_ENABLED(CONFIG_32BIT) && !strncasecmp(isa, "rv32", 4))
>  			isa += 4;
> -#elif IS_ENABLED(CONFIG_64BIT)
> -		if (!strncmp(isa, "rv64", 4))
> +		else if (IS_ENABLED(CONFIG_64BIT) && !strncasecmp(isa, "rv64", 4))
>  			isa += 4;
> -#endif
>  		/* The riscv,isa DT property must start with rv64 or rv32 */
>  		if (temp == isa)
>  			continue;
> @@ -136,6 +133,7 @@ void __init riscv_fill_hwcap(void)
>  			bool ext_long = false, ext_err = false;
>  
>  			switch (*ext) {
> +			case 'S':

Capital S should never be emitted by QEMU, so there's no need to have
this use the workaround, right? IOW, move it between s & X.

Otherwise, this looks good to me:
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 1/2] riscv: allow case-insensitive ISA string parsing

[Andrew Jones]

On Fri, Apr 28, 2023 at 10:16:00PM +0800, Yangyu Chen wrote:
> According to RISC-V Hart Capabilities Table (RHCT) description in UEFI
> Forum ECR, the format of the ISA string is defined in the RISC-V
> unprivileged specification which is case-insensitive. However, the
> current ISA string parser in the kernel does not support ISA strings
> with uppercase letters.
> 
> This patch modifies the ISA string parser in the kernel to support
> case-insensitive ISA string parsing.
> 
> Signed-off-by: Yangyu Chen <cyy@cyyself.name>
> ---
>  arch/riscv/kernel/cpu.c        |  3 ++-
>  arch/riscv/kernel/cpufeature.c | 25 ++++++++++++-------------
>  2 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index 8400f0cc9704..52b92a267121 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -4,6 +4,7 @@
>   */
>  
>  #include <linux/cpu.h>
> +#include <linux/ctype.h>
>  #include <linux/init.h>
>  #include <linux/seq_file.h>
>  #include <linux/of.h>
> @@ -41,7 +42,7 @@ int riscv_of_processor_hartid(struct device_node *node, unsigned long *hart)
>  		pr_warn("CPU with hartid=%lu has no \"riscv,isa\" property\n", *hart);
>  		return -ENODEV;
>  	}
> -	if (isa[0] != 'r' || isa[1] != 'v') {
> +	if (tolower(isa[0]) != 'r' || tolower(isa[1]) != 'v') {
>  		pr_warn("CPU with hartid=%lu has an invalid ISA of \"%s\"\n", *hart, isa);
>  		return -ENODEV;
>  	}
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 59d58ee0f68d..d1991c12e546 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -119,13 +119,10 @@ void __init riscv_fill_hwcap(void)
>  		}
>  
>  		temp = isa;
> -#if IS_ENABLED(CONFIG_32BIT)
> -		if (!strncmp(isa, "rv32", 4))
> +		if (IS_ENABLED(CONFIG_32BIT) && !strncasecmp(isa, "rv32", 4))
>  			isa += 4;
> -#elif IS_ENABLED(CONFIG_64BIT)
> -		if (!strncmp(isa, "rv64", 4))
> +		else if (IS_ENABLED(CONFIG_64BIT) && !strncasecmp(isa, "rv64", 4))
>  			isa += 4;
> -#endif
>  		/* The riscv,isa DT property must start with rv64 or rv32 */
>  		if (temp == isa)
>  			continue;
> @@ -136,6 +133,7 @@ void __init riscv_fill_hwcap(void)
>  			bool ext_long = false, ext_err = false;
>  
>  			switch (*ext) {
> +			case 'S':
>  			case 's':
>  				/**
>  				 * Workaround for invalid single-letter 's' & 'u'(QEMU).
> @@ -143,19 +141,20 @@ void __init riscv_fill_hwcap(void)
>  				 * not valid ISA extensions. It works until multi-letter
>  				 * extension starting with "Su" appears.
>  				 */
> -				if (ext[-1] != '_' && ext[1] == 'u') {
> +				if (ext[-1] != '_' && tolower(ext[1]) == 'u') {
>  					++isa;
>  					ext_err = true;
>  					break;
>  				}
>  				fallthrough;
> +			case 'X':
>  			case 'x':
> +			case 'Z':
>  			case 'z':
>  				ext_long = true;
>  				/* Multi-letter extension must be delimited */
>  				for (; *isa && *isa != '_'; ++isa)
> -					if (unlikely(!islower(*isa)
> -						     && !isdigit(*isa)))
> +					if (unlikely(!isalnum(*isa)))
>  						ext_err = true;
>  				/* Parse backwards */
>  				ext_end = isa;
> @@ -166,7 +165,7 @@ void __init riscv_fill_hwcap(void)
>  				/* Skip the minor version */
>  				while (isdigit(*--ext_end))
>  					;
> -				if (ext_end[0] != 'p'
> +				if (tolower(ext_end[0]) != 'p'
>  				    || !isdigit(ext_end[-1])) {
>  					/* Advance it to offset the pre-decrement */
>  					++ext_end;
> @@ -178,7 +177,7 @@ void __init riscv_fill_hwcap(void)
>  				++ext_end;
>  				break;
>  			default:
> -				if (unlikely(!islower(*ext))) {
> +				if (unlikely(!isalpha(*ext))) {
>  					ext_err = true;
>  					break;
>  				}
> @@ -188,7 +187,7 @@ void __init riscv_fill_hwcap(void)
>  				/* Skip the minor version */
>  				while (isdigit(*++isa))
>  					;
> -				if (*isa != 'p')
> +				if (tolower(*isa) != 'p')
>  					break;
>  				if (!isdigit(*++isa)) {
>  					--isa;
> @@ -205,7 +204,7 @@ void __init riscv_fill_hwcap(void)
>  #define SET_ISA_EXT_MAP(name, bit)						\
>  			do {							\
>  				if ((ext_end - ext == sizeof(name) - 1) &&	\
> -				     !memcmp(ext, name, sizeof(name) - 1) &&	\
> +				     !strncasecmp(ext, name, sizeof(name) - 1) &&	\
>  				     riscv_isa_extension_check(bit))		\
>  					set_bit(bit, this_isa);			\
>  			} while (false)						\
> @@ -213,7 +212,7 @@ void __init riscv_fill_hwcap(void)
>  			if (unlikely(ext_err))
>  				continue;
>  			if (!ext_long) {
> -				int nr = *ext - 'a';
> +				int nr = tolower(*ext) - 'a';
>  
>  				if (riscv_isa_extension_check(nr)) {
>  					this_hwcap |= isa2hwcap[nr];
> -- 
> 2.40.0
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Thanks,
drew

Re: [PATCH v2 1/2] riscv: allow case-insensitive ISA string parsing

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 696 bytes --]

On Fri, Apr 28, 2023 at 10:16:00PM +0800, Yangyu Chen wrote:
> @@ -205,7 +204,7 @@ void __init riscv_fill_hwcap(void)
>  #define SET_ISA_EXT_MAP(name, bit)						\
>  			do {							\
>  				if ((ext_end - ext == sizeof(name) - 1) &&	\
> -				     !memcmp(ext, name, sizeof(name) - 1) &&	\
> +				     !strncasecmp(ext, name, sizeof(name) - 1) &&	\
>  				     riscv_isa_extension_check(bit))		\
>  					set_bit(bit, this_isa);			\
>  			} while (false)						\

I was doing some poking in this area the other day & noticed that the \s
being misaligned isn't just the diff's fault and they are now misaligned
in the code.
If you are re-submitting, then I think you could align them.

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]