linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/2] riscv: allow case-insensitive ISA string parsing
@ 2023-05-01 16:10 Yangyu Chen
  2023-05-01 16:17 ` [PATCH v3 1/2] " Yangyu Chen
                   ` (3 more replies)
  0 siblings, 4 replies; 5+ messages in thread
From: Yangyu Chen @ 2023-05-01 16:10 UTC (permalink / raw)
  To: Conor Dooley, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Rob Herring, Krzysztof Kozlowski
  Cc: linux-riscv, linux-kernel, Andrew Jones, Wende Tan, Soha Jin,
	Hongren Zheng, 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 v2:
* Fixed misaligned '\' in `riscv_fill_hwcap`
* Move case 'S' after 's' to make the workaround only works for QEMU 
    in `riscv_fill_hwcap`

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                | 35 +++++++++----------
 3 files changed, 20 insertions(+), 20 deletions(-)

-- 
2.40.1


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

* [PATCH v3 1/2] riscv: allow case-insensitive ISA string parsing
  2023-05-01 16:10 [PATCH v3 0/2] riscv: allow case-insensitive ISA string parsing Yangyu Chen
@ 2023-05-01 16:17 ` Yangyu Chen
  2023-05-01 16:17 ` [PATCH v3 2/2] dt-bindings: riscv: drop invalid comment about riscv,isa lower-case reasoning Yangyu Chen
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 5+ messages in thread
From: Yangyu Chen @ 2023-05-01 16:17 UTC (permalink / raw)
  To: Conor Dooley, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Rob Herring, Krzysztof Kozlowski
  Cc: linux-riscv, linux-kernel, Andrew Jones, Wende Tan, Soha Jin,
	Hongren Zheng, Yangyu Chen, Conor Dooley

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.

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Yangyu Chen <cyy@cyyself.name>
---
 arch/riscv/kernel/cpu.c        |  3 ++-
 arch/riscv/kernel/cpufeature.c | 35 +++++++++++++++++-----------------
 2 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
index 3df38052dcbd..f4dadbfecd04 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>
@@ -42,7 +43,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 52585e088873..af2b468642a4 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -126,13 +126,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;
@@ -156,13 +153,15 @@ void __init riscv_fill_hwcap(void)
 					break;
 				}
 				fallthrough;
+			case 'S':
 			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;
@@ -173,7 +172,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;
@@ -185,7 +184,7 @@ void __init riscv_fill_hwcap(void)
 				++ext_end;
 				break;
 			default:
-				if (unlikely(!islower(*ext))) {
+				if (unlikely(!isalpha(*ext))) {
 					ext_err = true;
 					break;
 				}
@@ -195,7 +194,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;
@@ -209,18 +208,18 @@ void __init riscv_fill_hwcap(void)
 			if (*isa != '_')
 				--isa;
 
-#define SET_ISA_EXT_MAP(name, bit)						\
-			do {							\
-				if ((ext_end - ext == sizeof(name) - 1) &&	\
-				     !memcmp(ext, name, sizeof(name) - 1) &&	\
-				     riscv_isa_extension_check(bit))		\
-					set_bit(bit, this_isa);			\
-			} while (false)						\
+#define SET_ISA_EXT_MAP(name, bit)							\
+			do {								\
+				if ((ext_end - ext == sizeof(name) - 1) &&		\
+				     !strncasecmp(ext, name, sizeof(name) - 1) &&	\
+				     riscv_isa_extension_check(bit))			\
+					set_bit(bit, this_isa);				\
+			} while (false)							\
 
 			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.1


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

* [PATCH v3 2/2] dt-bindings: riscv: drop invalid comment about riscv,isa lower-case reasoning
  2023-05-01 16:10 [PATCH v3 0/2] riscv: allow case-insensitive ISA string parsing Yangyu Chen
  2023-05-01 16:17 ` [PATCH v3 1/2] " Yangyu Chen
@ 2023-05-01 16:17 ` Yangyu Chen
  2023-06-06 23:49 ` [PATCH v3 0/2] riscv: allow case-insensitive ISA string parsing Palmer Dabbelt
  2023-06-07  0:00 ` patchwork-bot+linux-riscv
  3 siblings, 0 replies; 5+ messages in thread
From: Yangyu Chen @ 2023-05-01 16:17 UTC (permalink / raw)
  To: Conor Dooley, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Rob Herring, Krzysztof Kozlowski
  Cc: linux-riscv, linux-kernel, Andrew Jones, Wende Tan, Soha Jin,
	Hongren Zheng, Conor Dooley, Rob Herring, 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>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Yangyu Chen <cyy@cyyself.name>
---
 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 25d6e8dbffb8..7dd792e7bb45 100644
--- a/Documentation/devicetree/bindings/riscv/cpus.yaml
+++ b/Documentation/devicetree/bindings/riscv/cpus.yaml
@@ -88,7 +88,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.1


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

* Re: [PATCH v3 0/2] riscv: allow case-insensitive ISA string parsing
  2023-05-01 16:10 [PATCH v3 0/2] riscv: allow case-insensitive ISA string parsing Yangyu Chen
  2023-05-01 16:17 ` [PATCH v3 1/2] " Yangyu Chen
  2023-05-01 16:17 ` [PATCH v3 2/2] dt-bindings: riscv: drop invalid comment about riscv,isa lower-case reasoning Yangyu Chen
@ 2023-06-06 23:49 ` Palmer Dabbelt
  2023-06-07  0:00 ` patchwork-bot+linux-riscv
  3 siblings, 0 replies; 5+ messages in thread
From: Palmer Dabbelt @ 2023-06-06 23:49 UTC (permalink / raw)
  To: Conor Dooley, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Rob Herring, Krzysztof Kozlowski, Yangyu Chen
  Cc: linux-riscv, linux-kernel, Andrew Jones, Wende Tan, Soha Jin,
	Hongren Zheng


On Tue, 02 May 2023 00:10:19 +0800, Yangyu Chen wrote:
> 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.
> 
> [...]

Applied, thanks!

[1/2] riscv: allow case-insensitive ISA string parsing
      https://git.kernel.org/palmer/c/255b34d799dd
[2/2] dt-bindings: riscv: drop invalid comment about riscv,isa lower-case reasoning
      https://git.kernel.org/palmer/c/9e320d7ca46a

Best regards,
-- 
Palmer Dabbelt <palmer@rivosinc.com>


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

* Re: [PATCH v3 0/2] riscv: allow case-insensitive ISA string parsing
  2023-05-01 16:10 [PATCH v3 0/2] riscv: allow case-insensitive ISA string parsing Yangyu Chen
                   ` (2 preceding siblings ...)
  2023-06-06 23:49 ` [PATCH v3 0/2] riscv: allow case-insensitive ISA string parsing Palmer Dabbelt
@ 2023-06-07  0:00 ` patchwork-bot+linux-riscv
  3 siblings, 0 replies; 5+ messages in thread
From: patchwork-bot+linux-riscv @ 2023-06-07  0:00 UTC (permalink / raw)
  To: Yangyu Chen
  Cc: linux-riscv, conor, paul.walmsley, palmer, aou, robh+dt,
	krzysztof.kozlowski+dt, linux-kernel, ajones, twd2.me, soha, i

Hello:

This series was applied to riscv/linux.git (for-next)
by Palmer Dabbelt <palmer@rivosinc.com>:

On Tue,  2 May 2023 00:10:19 +0800 you wrote:
> 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.
> 
> [...]

Here is the summary with links:
  - [v3,1/2] riscv: allow case-insensitive ISA string parsing
    https://git.kernel.org/riscv/c/255b34d799dd
  - [v3,2/2] dt-bindings: riscv: drop invalid comment about riscv,isa lower-case reasoning
    https://git.kernel.org/riscv/c/9e320d7ca46a

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



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

end of thread, other threads:[~2023-06-13 17:50 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-01 16:10 [PATCH v3 0/2] riscv: allow case-insensitive ISA string parsing Yangyu Chen
2023-05-01 16:17 ` [PATCH v3 1/2] " Yangyu Chen
2023-05-01 16:17 ` [PATCH v3 2/2] dt-bindings: riscv: drop invalid comment about riscv,isa lower-case reasoning Yangyu Chen
2023-06-06 23:49 ` [PATCH v3 0/2] riscv: allow case-insensitive ISA string parsing Palmer Dabbelt
2023-06-07  0:00 ` patchwork-bot+linux-riscv

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