[RFC PATCH v2 0/4] riscv: cpufeature: Improvements for extended feature handling

[RFC PATCH v2 0/4] riscv: cpufeature: Improvements for extended feature handling

[Tsukasa OI]

I think this patchset should be CCed to Mr. Patel.

RFC PATCH v1 (0/3 through 3/3):
http://lists.infradead.org/pipermail/linux-riscv/2021-November/010252.html
http://lists.infradead.org/pipermail/linux-riscv/2021-November/010249.html
http://lists.infradead.org/pipermail/linux-riscv/2021-November/010251.html
http://lists.infradead.org/pipermail/linux-riscv/2021-November/010250.html

See 0/3 (v1) for full background.

First of all, I must repeat that this patchset breaks RISC-V KVM.

Because single-letter extension "H" is not valid in the current ISA Manual
(see Volume 1, Chapter 27 "ISA Naming Conventions" for details), either ISA
Manual or RISC-V KVM (or both, most likely) must be changed to resolve
the conflict.

Current patchset is compliant to the ISA Manual and not compatible with
RISC-V KVM (that checks single-letter "H"-extension).  However, it is easy
to work around this issue.  By removing "case 'h':" line, this parser loses
compliance with the ISA Manual but gets compatibility with RISC-V KVM.

I created an Issue on GitHub so see the details here:
<https://github.com/riscv/riscv-isa-manual/issues/781>

I understand that this is the worst timing to report this kind of issue.
Still, situation is already a problem so I thought sooner is better.



Changed in v2: Patch 1 now uses a macro (NUM_ALPHA_EXTS), not magic number
    Thanks to Ben Dooks for valuable feedback!

Changed in v2: Patch 3 (v1) is split to 3, 4 (v2)
    It's possible that we only need extension names but not version
    numbers.  To separate ugly parts, I split original Patch 3 (v1) to two:
    1. Extract only extension names (Patch 3 (v2))
    2. Parse extension names / version numbers (Patch 4 (v2))

Changed in v2: `ext_err` has different meanings
    0: No error
    1: Invalid format (invalid character exists)
    2: Major version is out of range
    3: Minor version is out of range

New in v2: Backward parser for relaxed extension name rule
    Invalid vector subextension names (Zvl*, Zve*) are correctly parsed now
    (e.g. Zvl64b, Zve64d).  New backward parser finds version number from
    the end of an extension token (because multi-letter extension must be
    delimited by '_' if not end, we can search the delimiter first).
    Note that there's a proposal to relax requirements to extension names
    that would make Zvl* and Zve* valid:
    <https://github.com/riscv/riscv-isa-manual/pull/718>
    and RFC PATCH v2 is compatible with this propsed rule.
    Valid ISA string as per current ISA Manual is not affected by this
    (for "H"-extension, implementing such relaxed parser is impossible).

Fix in v2: Parser bug 1 in v1
    Following sequence is now parsed correctly
      (that bug occurred from the nature of greedy matching):
    1. Valid extension name
    2. Major version
    3. "P"-extension without any version number
    4. Next extension (or end of the string)

Fix in v2: Parser bug 2 in v1
    Full parser in v1 breaks too early when version number causes an
    arithmetic overflow or conflict with magic number (UINT_MAX, handled as
    unversioned), that would make the parser v1 misunderstand that non-
    existent "P" extension exists.
    That also caused full and minimal parsers don't produce the same
    tokenization result.

Those changes will change the parser behavior as follows:

Legend:
    [] : Valid token (either prefix or an extension + optional delimiter)
         note that "valid" does not necessarily mean "correct".
    <> : Invalid token (ext_err != 0)

"rv32imafzvl64b_zve64f" (backward parser [new in v2] involved)
    v1 : [rv32][i][m][a][f][zvl64][b_][zve64][f]
    v2 : [rv32][i][m][a][f][zvl64b_][zve64f] (intended)
"rv64b1pv" (parser bug 1 in v1 involved):
    v1 : [rv64]<b1p>[v]
    v2 : [rv64][b1][p][v] (correct)
"rv64i2p1" (parser bug 2 in v1 involved):
    v1 : [rv64][i2p1]
    v2 : [rv64][i2p1] (same; as long as no overflow in major version)
"rv64i4294967296p1" (now major version causes overflow):
    v1 : [rv64]<i4294967296>[p1]
    v2 : [rv64]<i4294967296p1> (correct)


Tsukasa OI (4):
  riscv: cpufeature: Correctly print supported extensions
  riscv: cpufeature: Minimal parser for "riscv,isa" strings
  riscv: cpufeature: Extract extension names from "riscv,isa"
  riscv: cpufeature: Full parser for "riscv,isa" strings

 arch/riscv/kernel/cpufeature.c | 119 +++++++++++++++++++++++++++++----
 1 file changed, 105 insertions(+), 14 deletions(-)

-- 
2.32.0


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

[RFC PATCH v2 1/4] riscv: cpufeature: Correctly print supported extensions

[Tsukasa OI]

This commit replaces BITS_PER_LONG with number of alphabet letters.

Current ISA pretty-printing code expects extension 'a' (bit 0) through
'z' (bit 25).  Although bit 26 and higher is not currently used (thus never
cause an issue in practice), it will be an annoying problem if we start to
use those in the future.

This commit disables printing high bits for now.

Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com>
---
 arch/riscv/kernel/cpufeature.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index d959d207a40d..dd3d57eb4eea 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -13,6 +13,8 @@
 #include <asm/smp.h>
 #include <asm/switch_to.h>
 
+#define NUM_ALPHA_EXTS ('z' - 'a' + 1)
+
 unsigned long elf_hwcap __read_mostly;
 
 /* Host ISA bitmap */
@@ -63,7 +65,7 @@ void __init riscv_fill_hwcap(void)
 {
 	struct device_node *node;
 	const char *isa;
-	char print_str[BITS_PER_LONG + 1];
+	char print_str[NUM_ALPHA_EXTS + 1];
 	size_t i, j, isa_len;
 	static unsigned long isa2hwcap[256] = {0};
 
@@ -133,13 +135,13 @@ void __init riscv_fill_hwcap(void)
 	}
 
 	memset(print_str, 0, sizeof(print_str));
-	for (i = 0, j = 0; i < BITS_PER_LONG; i++)
+	for (i = 0, j = 0; i < NUM_ALPHA_EXTS; i++)
 		if (riscv_isa[0] & BIT_MASK(i))
 			print_str[j++] = (char)('a' + i);
 	pr_info("riscv: ISA extensions %s\n", print_str);
 
 	memset(print_str, 0, sizeof(print_str));
-	for (i = 0, j = 0; i < BITS_PER_LONG; i++)
+	for (i = 0, j = 0; i < NUM_ALPHA_EXTS; i++)
 		if (elf_hwcap & BIT_MASK(i))
 			print_str[j++] = (char)('a' + i);
 	pr_info("riscv: ELF capabilities %s\n", print_str);
-- 
2.32.0


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

[RFC PATCH v2 2/4] riscv: cpufeature: Minimal parser for "riscv, isa" strings

[Tsukasa OI]

Current hart ISA ("riscv,isa") parser don't correctly parse:

1. Multi-letter extensions
2. Version numbers

If we don't have those in "riscv,isa", that's fine.  However, many of
standardized multi-letter extensions are being frozen and ratified.
The current "riscv,isa" parser that is easily confused by multi-letter
extensions and "p" in version numbers can be a huge problem for adding
new extensions through the device tree.

Leaving it would create incompatible hacks and would make "riscv,isa"
value unreliable.

This commit implements minimal parser for "riscv,isa" strings.  With this,
we can safely ignore multi-letter extensions and version numbers.

Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com>
---
 arch/riscv/kernel/cpufeature.c | 62 ++++++++++++++++++++++++++++------
 1 file changed, 51 insertions(+), 11 deletions(-)

diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index dd3d57eb4eea..93b436addd90 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -7,6 +7,7 @@
  */
 
 #include <linux/bitmap.h>
+#include <linux/ctype.h>
 #include <linux/of.h>
 #include <asm/processor.h>
 #include <asm/hwcap.h>
@@ -66,7 +67,7 @@ void __init riscv_fill_hwcap(void)
 	struct device_node *node;
 	const char *isa;
 	char print_str[NUM_ALPHA_EXTS + 1];
-	size_t i, j, isa_len;
+	int i, j;
 	static unsigned long isa2hwcap[256] = {0};
 
 	isa2hwcap['i'] = isa2hwcap['I'] = COMPAT_HWCAP_ISA_I;
@@ -92,23 +93,62 @@ void __init riscv_fill_hwcap(void)
 			continue;
 		}
 
-		i = 0;
-		isa_len = strlen(isa);
 #if IS_ENABLED(CONFIG_32BIT)
 		if (!strncmp(isa, "rv32", 4))
-			i += 4;
+			isa += 4;
 #elif IS_ENABLED(CONFIG_64BIT)
 		if (!strncmp(isa, "rv64", 4))
-			i += 4;
+			isa += 4;
 #endif
-		for (; i < isa_len; ++i) {
-			this_hwcap |= isa2hwcap[(unsigned char)(isa[i])];
+		for (; *isa; ++isa) {
+			const char *ext = isa++;
+			unsigned short ext_err = 0;
+			bool ext_long;
+
+			switch (*ext) {
+			case 'h':
+			case 's':
+			case 'x':
+			case 'z':
+				ext_long = true;
+				/* Multi-letter extension must be delimited */
+				for (; *isa && *isa != '_'; ++isa)
+					if (!islower(*isa) && !isdigit(*isa))
+						ext_err = 1;
+				/* ... but must be ignored. */
+				break;
+			default:
+				ext_long = false;
+				if (!islower(*ext)) {
+					ext_err = 1;
+					break;
+				}
+				/* Find next extension */
+				if (!isdigit(*isa))
+					break;
+				while (isdigit(*++isa))
+					;
+				if (*isa != 'p')
+					break;
+				if (!isdigit(*++isa)) {
+					--isa;
+					break;
+				}
+				while (isdigit(*++isa))
+					;
+				break;
+			}
+			if (*isa != '_')
+				--isa;
 			/*
-			 * TODO: X, Y and Z extension parsing for Host ISA
-			 * bitmap will be added in-future.
+			 * TODO: Full version-aware handling including
+			 * multi-letter extensions will be added in-future.
 			 */
-			if ('a' <= isa[i] && isa[i] < 'x')
-				this_isa |= (1UL << (isa[i] - 'a'));
+			if (!ext_long && !ext_err) {
+				this_hwcap |= isa2hwcap[(unsigned char)(*ext)];
+				if (!ext_long)
+					this_isa |= (1UL << (*ext - 'a'));
+			}
 		}
 
 		/*
-- 
2.32.0


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

[RFC PATCH v2 3/4] riscv: cpufeature: Extract extension names from "riscv, isa"

[Tsukasa OI]

It's possible that we only need extension names implemented but not
version numbers.  This commit doesn't parse version numbers but does
extract implemented extension names.

`ext_end-1` points at the last character of the extension name and
`ext_end-ext` is the length of the extension name.

Beware that the extension name is **not** null-terminated.

Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com>
---
 arch/riscv/kernel/cpufeature.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 93b436addd90..61bc326d15b0 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -102,6 +102,7 @@ void __init riscv_fill_hwcap(void)
 #endif
 		for (; *isa; ++isa) {
 			const char *ext = isa++;
+			const char *ext_end = isa;
 			unsigned short ext_err = 0;
 			bool ext_long;
 
@@ -115,7 +116,21 @@ void __init riscv_fill_hwcap(void)
 				for (; *isa && *isa != '_'; ++isa)
 					if (!islower(*isa) && !isdigit(*isa))
 						ext_err = 1;
-				/* ... but must be ignored. */
+				/* Find end of the extension name backwards */
+				ext_end = isa;
+				if (ext_err)
+					break;
+				if (!isdigit(ext_end[-1]))
+					break;
+				while (isdigit(*--ext_end))
+					;
+				if (ext_end[0] != 'p'
+				    || !isdigit(ext_end[-1])) {
+					++ext_end;
+					break;
+				}
+				while (isdigit(*--ext_end))
+					;
 				break;
 			default:
 				ext_long = false;
@@ -144,7 +159,7 @@ void __init riscv_fill_hwcap(void)
 			 * TODO: Full version-aware handling including
 			 * multi-letter extensions will be added in-future.
 			 */
-			if (!ext_long && !ext_err) {
+			if (!ext_err) {
 				this_hwcap |= isa2hwcap[(unsigned char)(*ext)];
 				if (!ext_long)
 					this_isa |= (1UL << (*ext - 'a'));
-- 
2.32.0


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

[RFC PATCH v2 4/4] riscv: cpufeature: Full parser for "riscv, isa" strings

[Tsukasa OI]

This commit implements full parser for "riscv,isa" strings.

We haven't determined how do we represent multi-letter and/or versioned
extensions in the ISA bitmap yet.  So, this commit handles only single-
letter extensions with no respect to version numbers (as before).

Nevertheless, it can be a foundation for our future work.

Note that major version of UINT_MAX represents non-versioned extension
(in many cases, they should be handled as 1 with some exceptions).

Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com>
---
 arch/riscv/kernel/cpufeature.c | 44 ++++++++++++++++++++++++++++++----
 1 file changed, 39 insertions(+), 5 deletions(-)

diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 61bc326d15b0..8c64f63a686c 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -62,6 +62,22 @@ bool __riscv_isa_extension_available(const unsigned long *isa_bitmap, int bit)
 }
 EXPORT_SYMBOL_GPL(__riscv_isa_extension_available);
 
+static inline int _decimal_part_to_uint(const char *s, unsigned int *res)
+{
+	unsigned int value = 0, d;
+
+	if (!isdigit(*s))
+		return -EINVAL;
+	do {
+		d = *s - '0';
+		if (value > (UINT_MAX - d) / 10)
+			return -ERANGE;
+		value = value * 10 + d;
+	} while (isdigit(*++s));
+	*res = value;
+	return 0;
+}
+
 void __init riscv_fill_hwcap(void)
 {
 	struct device_node *node;
@@ -103,8 +119,10 @@ void __init riscv_fill_hwcap(void)
 		for (; *isa; ++isa) {
 			const char *ext = isa++;
 			const char *ext_end = isa;
+			unsigned int ext_major = UINT_MAX; /* default */
+			unsigned int ext_minor = 0;
 			unsigned short ext_err = 0;
-			bool ext_long;
+			bool ext_long, ext_vpair;
 
 			switch (*ext) {
 			case 'h':
@@ -116,7 +134,7 @@ void __init riscv_fill_hwcap(void)
 				for (; *isa && *isa != '_'; ++isa)
 					if (!islower(*isa) && !isdigit(*isa))
 						ext_err = 1;
-				/* Find end of the extension name backwards */
+				/* Parse backwards */
 				ext_end = isa;
 				if (ext_err)
 					break;
@@ -124,13 +142,23 @@ void __init riscv_fill_hwcap(void)
 					break;
 				while (isdigit(*--ext_end))
 					;
-				if (ext_end[0] != 'p'
-				    || !isdigit(ext_end[-1])) {
+				ext_vpair = (ext_end[0] == 'p')
+					    && isdigit(ext_end[-1]);
+				if (_decimal_part_to_uint(ext_end + 1,
+							  &ext_major))
+					ext_err = ext_vpair ? 3 : 2;
+				if (!ext_vpair) {
 					++ext_end;
+					if (ext_major == UINT_MAX)
+						ext_err = 2;
 					break;
 				}
+				ext_minor = ext_major;
 				while (isdigit(*--ext_end))
 					;
+				if (_decimal_part_to_uint(++ext_end, &ext_major)
+				    || ext_major == UINT_MAX)
+					ext_err = 2;
 				break;
 			default:
 				ext_long = false;
@@ -138,9 +166,12 @@ void __init riscv_fill_hwcap(void)
 					ext_err = 1;
 					break;
 				}
-				/* Find next extension */
+				/* Parse forwards finding next extension */
 				if (!isdigit(*isa))
 					break;
+				_decimal_part_to_uint(isa, &ext_major);
+				if (ext_major == UINT_MAX)
+					ext_err = 2;
 				while (isdigit(*++isa))
 					;
 				if (*isa != 'p')
@@ -149,6 +180,9 @@ void __init riscv_fill_hwcap(void)
 					--isa;
 					break;
 				}
+				if (!ext_err &&
+				    _decimal_part_to_uint(isa, &ext_minor))
+					ext_err = 3;
 				while (isdigit(*++isa))
 					;
 				break;
-- 
2.32.0


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