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

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

[Tsukasa OI]

This is my v3 patchset about RISC-V device tree-based feature handling.

Background sections are mostly unmodified from RFC PATCH v1.  I repost
them so that this mail contains all important information and background.


Index
------

-   History
-   Background: New Extensions
-   Background: How Current ISA Handling is Broken?
-   Patches
-   Changes from RFC PATCH v2 -> v3


History
--------

RFC PATCH v1:
http://lists.infradead.org/pipermail/linux-riscv/2021-November/010252.html
RFC PATCH v2:
http://lists.infradead.org/pipermail/linux-riscv/2021-November/010350.html
RFC PATCH v3:
You are here.


Background: New Extensions
---------------------------

In this year, many of standard multi-letter extensions are being frozen,
reviewed and ratified.

They include:

-   Svpbmt (Page-Based Memory Types) 1.0
-   Zfinx / Zdinx / Zhinx (FP in Integer Registers) 1.0.0-rc
-   Zba / Zbb / Zbc / Zbs (Bit Manipulation) 1.0.0
-   Zkn / Zks / Zkr (Scalar Crypto) 1.0.0-rc6
-   Zve* / Zvl* (Vector Subextensions) 1.0

many of which introduce new instructions, new CSR (registers/bits)
and/or new features.

Also, it's highly plausible that distinguishing incompatible major versions
(version 2 and later) will be needed in the future.

In the device tree, we have "riscv,isa" string representing implemented ISA
and extensions supported by a hart.  Parsing this enables feature
detection. (Current kernel parses this string to detect presence of FPU.)

However, current ISA handling code does not allow handling those multi-
letter extensions and version numbers for multiple reasons.

(a) We haven't defined how do we encode those in ISA bitmap and
(b) current code simply does not support those

My series of patches are intended to resolve (b) and pave a path to easily
detect features noted by versioned and/or multi-letter extensions
(Extended Features) after (a) is resolved.


Background: How Current ISA Handling is Broken?
------------------------------------------------

Current arch/riscv/kernel/cpufeature.c (as of
commit 8bb7eca972ad ("Linux 5.15")) has two issues:

(a) It handles "riscv,isa" as prefix + array of single-letter extensions
(b) ignores every other characters (including version numbers)

For instance, ISA variant "rv32i2p1" means:

-   RISC-V, 32-bit
-   Base ISA "I" version 2.1

but current code parses this string (on 32-bit kernel) as:

-   RISC-V, 32-bit (which matches the kernel)
-   Base ISA "I"
-   Packed-SIMD extension "P"

This is clearly broken ...but it isn't very bad considering proposed "P"
draft is not yet frozen.  With non-versioned "rv64imazifencei_zdinx"
however (on 64-bit kernel), things can get really worse:

(intended meaning)
-   RISC-V, 64-bit
-   Base Instruction Set "I"
-   Single-letter Extensions "M", "A"
-   Multi-letter Extensions "Zifencei" and "Zdinx"

(parsed as)
-   RISC-V, 64-bit
-   Base Instruction **Sets** "I" and "E"
-   Extensions "M" and "A"
-   Compressed Instruction Extension "C"
-   Floating Point Extensions "F" and "D"
-   Reserved Extension "N" (removed user-level interrupts)
-   Supervisor Extension here? "S"

Incorrect detection of "F" and "D" is really bad ("C" is no good either).

Yes, I admit that "Zdinx" (double FP in GPR) is here to demonstrate the
worst kind of the problem intentionally.  But the problem stays.

Even if the extension does not need kernel support (e.g. extensions provide
new instructions but only general purpose registers are involved), wrong
host ISA detection can easily confuse the kernel and will cause problems
in the near future.  That's why I submit this series of patches now (not
during/after kernel support for multiple Extended Features is discussed).


Patches
--------

Patch 1/4: Change BITS_PER_LONG to number of alphabet letters
    Current ISA pretty-printing code assumes only bit 0-25 ('a'-'z') are
    used (it uses 'a' + BIT_OFFSET to print supported extensions).
    Although current parser does not set bit 26 and higher, it will be an
    annoying problem if we start to use those high feature bits.

Patch 2/4: Minimal "riscv,isa" Parser
    This patch implements minimal "riscv,isa" parser, which is designed to
    ignore multi-letter extensions and version numbers.  With this patch,
    you can safely ignore mutli-letter extensions and version numbers.

Patch 3/4: Full "riscv,isa" Parser (Part 1: Extension Names)
    This patch enables to extract multi-letter extension names. You can
    match any extension names to test some feature.
    Note that I implemented a backward parser to handle extension names
    with one or more digits such as "Zvl256b" (which is not valid per
    current ISA Manual not relaxing the rule is being discussed).
    cf. <https://github.com/riscv/riscv-isa-manual/pull/718>

Patch 4/4: Full "riscv,isa" Parser (Part 2: Version Numbers)
    In addition to Patch 3, this patch enables you to extract version
    information of an extension.  If version number handling is not needed,
    this patch may be omitted
    (I consider that Patch 1-3 satisfy most usecases).


Changes from RFC PATCH v2 -> v3
--------------------------------

-   'H'-extension is now parsed as a single-letter extension

    Because current RISC-V ISA Manual and software implementations seemed
    inconsistent, I decided to comply with the ISA Manual in
    RFC PATCH v1/v2 and reported an "issue" to riscv-isa-manual.
    cf. <https://github.com/riscv/riscv-isa-manual/issues/781>

    Greg Favor commented that 'H'-extension is in fact a single-letter
    extension and corresponding sections in the ISA Manual will be
    changed in a few month.

    Now, I'm confident enough to consider that 'H' is a single-letter
    extension and should not be handled as a prefix of multi-letter
    extensions "H*".  If ISA change didn't happen, we can discuss later.

    RISC-V KVM is now compatible with this patchset.

-   Changed parser error handling again

    It seemed that need to handling version numbers is rare and possible
    error number handling seemed a bit clumsy.  So, I simplified it
    (usually, we don't have to know *which* version number is invalid).
    Patch 4 is optional but I request to merge this too due to small
    overhead of version number handling and possible usecases involving
    custom and/or experimental extensions.

    `ext_err`:
        It indicates whether a valid extension token is parsed.
        If `false`, the extension token is valid.
    `ext_err_ver` (Patch 4 only):
        It indicates whether version number of the extension token is valid
        and correctly parsed by the implementation.
        If `false`, version number `ext_{major,minor}` are valid.
        Note that those version numbers have default values.
        `ext_major`:
            `UINT_MAX` means the token does not contain any version
            information.  Usually, implied major version is `1`
            (with some exceptions).
        `ext_minor`:
            `0` means either:
                (a) the token does not contain minor version number or
                (b) given minor version number is `0`.

-   `MATCH_EXT` macro is added

    I added `MATCH_EXT` macro to Patch 3.  This macro enables you to test
    whether certain extension name (in lowercase) is matched.

    For instance, `MATCH_EXT("zba")` will match extension "Zba".

    I didn't included an example for this but did in my GitHub branch
    (on the top of Patch 4).
    <https://github.com/a4lg/linux/tree/riscv-cpufeature.v3>

-   Added `unlikely` to error path




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 | 120 ++++++++++++++++++++++++++++-----
 1 file changed, 104 insertions(+), 16 deletions(-)


base-commit: 8bb7eca972ad531c9b149c0a51ab43a417385813
-- 
2.32.0


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

[RFC PATCH v3 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 v3 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.

Important Note:
  This commit does handle "H" as a single-letter extension.  This
  handling is not compliant with current version of RISC-V ISA Manual
  but will going to be in a few months.

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

diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index dd3d57eb4eea..d4ff704db53b 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,59 @@ 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++;
+			bool ext_long, ext_err = false;
+
+			switch (*ext) {
+			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 = true;
+				/* ... but must be ignored. */
+				break;
+			default:
+				ext_long = false;
+				if (!islower(*ext)) {
+					ext_err = true;
+					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_err || ext_long)
+				continue;
+			this_hwcap |= isa2hwcap[(unsigned char)(*ext)];
+			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 v3 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.

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

Use `MATCH_EXT` macro to match extension name (the argument is
lower-case constant string).

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

diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index d4ff704db53b..f52e15488a70 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;
 			bool ext_long, ext_err = false;
 
 			switch (*ext) {
@@ -111,13 +112,28 @@ void __init riscv_fill_hwcap(void)
 				ext_long = true;
 				/* Multi-letter extension must be delimited */
 				for (; *isa && *isa != '_'; ++isa)
-					if (!islower(*isa) && !isdigit(*isa))
+					if (unlikely(!islower(*isa)
+						     && !isdigit(*isa)))
 						ext_err = true;
-				/* ... but must be ignored. */
+				/* Find end of the extension name backwards */
+				ext_end = isa;
+				if (unlikely(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;
-				if (!islower(*ext)) {
+				if (unlikely(!islower(*ext))) {
 					ext_err = true;
 					break;
 				}
@@ -138,14 +154,15 @@ void __init riscv_fill_hwcap(void)
 			}
 			if (*isa != '_')
 				--isa;
-			/*
-			 * TODO: Full version-aware handling including
-			 * multi-letter extensions will be added in-future.
-			 */
-			if (ext_err || ext_long)
+
+#define MATCH_EXT(name)  (ext_end - ext == sizeof(name) - 1 \
+			  && !memcmp(ext, name, sizeof(name) - 1))
+			if (unlikely(ext_err))
 				continue;
 			this_hwcap |= isa2hwcap[(unsigned char)(*ext)];
-			this_isa |= (1UL << (*ext - 'a'));
+			if (!ext_long)
+				this_isa |= (1UL << (*ext - 'a'));
+#undef MATCH_EXT
 		}
 
 		/*
-- 
2.32.0


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

[RFC PATCH v3 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 | 42 ++++++++++++++++++++++++++++++----
 1 file changed, 37 insertions(+), 5 deletions(-)

diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index f52e15488a70..a462ceb959d9 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,7 +119,10 @@ void __init riscv_fill_hwcap(void)
 		for (; *isa; ++isa) {
 			const char *ext = isa++;
 			const char *ext_end = isa;
-			bool ext_long, ext_err = false;
+			unsigned int ext_major = UINT_MAX; /* default */
+			unsigned int ext_minor = 0;
+			bool ext_long, ext_vpair,
+			     ext_err = false, ext_err_ver = false;
 
 			switch (*ext) {
 			case 's':
@@ -115,7 +134,7 @@ void __init riscv_fill_hwcap(void)
 					if (unlikely(!islower(*isa)
 						     && !isdigit(*isa)))
 						ext_err = true;
-				/* Find end of the extension name backwards */
+				/* Parse backwards */
 				ext_end = isa;
 				if (unlikely(ext_err))
 					break;
@@ -123,13 +142,21 @@ 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_ver = true;
+				if (!ext_vpair) {
 					++ext_end;
 					break;
 				}
+				ext_minor = ext_major;
 				while (isdigit(*--ext_end))
 					;
+				if (_decimal_part_to_uint(++ext_end, &ext_major)
+				    || ext_major == UINT_MAX)
+					ext_err_ver = true;
 				break;
 			default:
 				ext_long = false;
@@ -137,9 +164,12 @@ void __init riscv_fill_hwcap(void)
 					ext_err = true;
 					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_ver = true;
 				while (isdigit(*++isa))
 					;
 				if (*isa != 'p')
@@ -148,6 +178,8 @@ void __init riscv_fill_hwcap(void)
 					--isa;
 					break;
 				}
+				if (_decimal_part_to_uint(isa, &ext_minor))
+					ext_err_ver = true;
 				while (isdigit(*++isa))
 					;
 				break;
-- 
2.32.0


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