[PATCH v5 0/6] Provide a fraemework for RISC-V ISA extensions

[PATCH v5 0/6] Provide a fraemework for RISC-V ISA extensions

[Atish Patra]

This series implements a generic framework to parse multi-letter ISA
extensions. This series is based on Tsukasa's v3 isa extension improvement
series[1]. I have fixed few bugs and improved comments from that series
(PATCH1-3). I have not used PATCH 4 from that series as we are not using
ISA extension versioning as of now. We can add that later if required.

PATCH 4 allows the probing of multi-letter extensions via a macro.
It continues to use the common isa extensions between all the harts.
Thus hetergenous hart systems will only see the common ISA extensions.

PATCH 6 improves the /proc/cpuinfo interface for the available ISA extensions
via /proc/cpuinfo.

Here is the example output of /proc/cpuinfo:
(with debug patches in Qemu and Linux kernel)

# cat /proc/cpuinfo 
processor	: 0
hart		: 0
isa		: rv64imafdch
isa-ext		: svpbmt svnapot svinval 
mmu		: sv48

processor	: 1
hart		: 1
isa		: rv64imafdch
isa-ext		: svpbmt svnapot svinval 
mmu		: sv48

processor	: 2
hart		: 2
isa		: rv64imafdch
isa-ext		: svpbmt svnapot svinval 
mmu		: sv48

processor	: 3
hart		: 3
isa		: rv64imafdch
isa-ext		: svpbmt svnapot svinval 
mmu		: sv48

Anybody adding support for any new multi-letter extensions should add an
entry to the riscv_isa_ext_id and the isa extension array. 
E.g. The patch[2] adds the support for various ISA extensions.

[1] https://lore.kernel.org/all/0f568515-a05e-8204-aae3-035975af3ee8@irq.a4lg.com/T/
[2] https://github.com/atishp04/linux/commit/e9e240c9a854dceb434ceb53bdbe82a657bee5f2 

Changes from v4->v5:
1. Improved the /proc/cpuinfo to include only valid & enabled extensions
2. Improved the multi-letter parsing by skipping the 'su' modes generated in
   Qemu as suggested by Tsukasa.

Changes from v3->v4:
1. Changed temporary variable for current hart isa to a bitmap
2. Added reviewed-by tags.
3. Improved comments

Changes from v2->v3:
1. Updated comments to mark clearly a fix required for Qemu only.
2. Fixed a bug where the 1st multi-letter extension can be present without _
3. Added Tested by tags. 

Changes from v1->v2:
1. Instead of adding a separate DT property use the riscv,isa property.
2. Based on Tsukasa's v3 isa extension improvement series.

Atish Patra (3):
RISC-V: Implement multi-letter ISA extension probing framework
RISC-V: Do no continue isa string parsing without correct XLEN
RISC-V: Improve /proc/cpuinfo output for ISA extensions

Tsukasa OI (3):
RISC-V: Correctly print supported extensions
RISC-V: Minimal parser for "riscv, isa" strings
RISC-V: Extract multi-letter extension names from "riscv, isa"

arch/riscv/include/asm/hwcap.h |  25 +++++++
arch/riscv/kernel/cpu.c        |  51 ++++++++++++-
arch/riscv/kernel/cpufeature.c | 130 +++++++++++++++++++++++++++------
3 files changed, 183 insertions(+), 23 deletions(-)

--
2.30.2

[PATCH v5 1/6] RISC-V: Correctly print supported extensions

[Atish Patra]

From: Tsukasa OI <research_trasio@irq.a4lg.com>

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.

Reviewed-by: Anup Patel <anup@brainfault.org>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com>
Signed-off-by: Atish Patra <atishp@rivosinc.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.30.2

[PATCH v5 2/6] RISC-V: Minimal parser for "riscv, isa" strings

[Atish Patra]

From: Tsukasa OI <research_trasio@irq.a4lg.com>

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

1. Multi-letter extensions
2. Version numbers

All ISA extensions ratified recently has multi-letter extensions
(except 'H'). 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.

[Improved commit text and fixed a bug around 's' in base extension]
Signed-off-by: Atish Patra <atishp@rivosinc.com>
[Fixed workaround for QEMU]
Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com>
Tested-by: Heiko Stuebner <heiko@sntech.de>
---
 arch/riscv/kernel/cpufeature.c | 72 ++++++++++++++++++++++++++++------
 1 file changed, 61 insertions(+), 11 deletions(-)

diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index dd3d57eb4eea..72c5f6ef56b5 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,72 @@ 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++;
+			const char *ext_end = isa;
+			bool ext_long = false, ext_err = false;
+
+			switch (*ext) {
+			case 's':
+				/**
+				 * Workaround for invalid single-letter 's' & 'u'(QEMU).
+				 * No need to set the bit in riscv_isa as 's' & 'u' are
+				 * not valid ISA extensions. It works until multi-letter
+				 * extension starting with "Su" appears.
+				 */
+				if (ext[-1] != '_' && ext[1] == 'u') {
+					++isa;
+					ext_err = true;
+					break;
+				}
+				fallthrough;
+			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;
+				break;
+			default:
+				if (unlikely(!islower(*ext))) {
+					ext_err = true;
+					break;
+				}
+				/* Find next extension */
+				if (!isdigit(*isa))
+					break;
+				/* Skip the minor version */
+				while (isdigit(*++isa))
+					;
+				if (*isa != 'p')
+					break;
+				if (!isdigit(*++isa)) {
+					--isa;
+					break;
+				}
+				/* Skip the major version */
+				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.30.2

[PATCH v5 3/6] RISC-V: Extract multi-letter extension names from "riscv, isa"

[Atish Patra]

From: Tsukasa OI <research_trasio@irq.a4lg.com>

Currently, there is no usage for version numbers in extensions as
any ratified non base ISA extension will always at v1.0.

Extract the extension names in place for future parsing.

Tested-by: Heiko Stuebner <heiko@sntech.de>
Reviewed-by: Anup Patel <anup@brainfault.org>
Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com>
[Improved commit text and comments]
Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 arch/riscv/kernel/cpufeature.c | 35 ++++++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 72c5f6ef56b5..b0df7eff47f7 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -124,8 +124,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;
+				/* Parse backwards */
+				ext_end = isa;
+				if (unlikely(ext_err))
+					break;
+				if (!isdigit(ext_end[-1]))
+					break;
+				/* Skip the minor version */
+				while (isdigit(*--ext_end))
+					;
+				if (ext_end[0] != 'p'
+				    || !isdigit(ext_end[-1])) {
+					/* Advance it to offset the pre-decrement */
+					++ext_end;
+					break;
+				}
+				/* Skip the major version */
+				while (isdigit(*--ext_end))
+					;
+				++ext_end;
 				break;
 			default:
 				if (unlikely(!islower(*ext))) {
@@ -151,14 +171,13 @@ 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)
+
+			if (unlikely(ext_err))
 				continue;
-			this_hwcap |= isa2hwcap[(unsigned char)(*ext)];
-			this_isa |= (1UL << (*ext - 'a'));
+			if (!ext_long) {
+				this_hwcap |= isa2hwcap[(unsigned char)(*ext)];
+				this_isa |= (1UL << (*ext - 'a'));
+			}
 		}
 
 		/*
-- 
2.30.2

[PATCH v5 4/6] RISC-V: Implement multi-letter ISA extension probing framework

[Atish Patra]

Multi-letter extensions can be probed using exising
riscv_isa_extension_available API now. It doesn't support versioning
right now as there is no use case for it.
Individual extension specific implementation will be added during
each extension support.

Tested-by: Heiko Stuebner <heiko@sntech.de>
Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 arch/riscv/include/asm/hwcap.h | 18 ++++++++++++++++++
 arch/riscv/kernel/cpufeature.c | 24 ++++++++++++++++++------
 2 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index 5ce50468aff1..170bd80da520 100644
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -34,7 +34,25 @@ extern unsigned long elf_hwcap;
 #define RISCV_ISA_EXT_s		('s' - 'a')
 #define RISCV_ISA_EXT_u		('u' - 'a')
 
+/*
+ * Increse this to higher value as kernel support more ISA extensions.
+ */
 #define RISCV_ISA_EXT_MAX	64
+#define RISCV_ISA_EXT_NAME_LEN_MAX 32
+
+/* The base ID for multi-letter ISA extensions */
+#define RISCV_ISA_EXT_BASE 26
+
+/*
+ * This enum represent the logical ID for each multi-letter RISC-V ISA extension.
+ * The logical ID should start from RISCV_ISA_EXT_BASE and must not exceed
+ * RISCV_ISA_EXT_MAX. 0-25 range is reserved for single letter
+ * extensions while all the multi-letter extensions should define the next
+ * available logical extension id.
+ */
+enum riscv_isa_ext_id {
+	RISCV_ISA_EXT_ID_MAX = RISCV_ISA_EXT_MAX,
+};
 
 unsigned long riscv_isa_extension_base(const unsigned long *isa_bitmap);
 
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index b0df7eff47f7..c6693873e95c 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -83,7 +83,7 @@ void __init riscv_fill_hwcap(void)
 
 	for_each_of_cpu_node(node) {
 		unsigned long this_hwcap = 0;
-		unsigned long this_isa = 0;
+		DECLARE_BITMAP(this_isa, RISCV_ISA_EXT_MAX);
 
 		if (riscv_of_processor_hartid(node) < 0)
 			continue;
@@ -100,6 +100,7 @@ void __init riscv_fill_hwcap(void)
 		if (!strncmp(isa, "rv64", 4))
 			isa += 4;
 #endif
+		bitmap_zero(this_isa, RISCV_ISA_EXT_MAX);
 		for (; *isa; ++isa) {
 			const char *ext = isa++;
 			const char *ext_end = isa;
@@ -172,12 +173,22 @@ 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)) {    \
+					set_bit(bit, this_isa);			\
+					pr_info("Found ISA extension %s", name);\
+				}						\
+			} while (false)						\
+
 			if (unlikely(ext_err))
 				continue;
 			if (!ext_long) {
 				this_hwcap |= isa2hwcap[(unsigned char)(*ext)];
-				this_isa |= (1UL << (*ext - 'a'));
+				set_bit(*ext - 'a', this_isa);
 			}
+#undef SET_ISA_EXT_MAP
 		}
 
 		/*
@@ -190,10 +201,11 @@ void __init riscv_fill_hwcap(void)
 		else
 			elf_hwcap = this_hwcap;
 
-		if (riscv_isa[0])
-			riscv_isa[0] &= this_isa;
+		if (bitmap_weight(riscv_isa, RISCV_ISA_EXT_MAX))
+			bitmap_and(riscv_isa, riscv_isa, this_isa, RISCV_ISA_EXT_MAX);
 		else
-			riscv_isa[0] = this_isa;
+			bitmap_copy(riscv_isa, this_isa, RISCV_ISA_EXT_MAX);
+
 	}
 
 	/* We don't support systems with F but without D, so mask those out
@@ -207,7 +219,7 @@ void __init riscv_fill_hwcap(void)
 	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);
+	pr_info("riscv: base ISA extensions %s\n", print_str);
 
 	memset(print_str, 0, sizeof(print_str));
 	for (i = 0, j = 0; i < NUM_ALPHA_EXTS; i++)
-- 
2.30.2

[PATCH v5 5/6] RISC-V: Do no continue isa string parsing without correct XLEN

[Atish Patra]

The isa string should begin with either rv64 or rv32. Otherwise, it is
an incorrect isa string. Currently, the string parsing continues even if
it doesnot begin with current XLEN.

Fix this by checking if it found "rv64" or "rv32" in the beginning.

Tested-by: Heiko Stuebner <heiko@sntech.de>
Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 arch/riscv/kernel/cpufeature.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index c6693873e95c..f3a4b0619aa0 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -84,6 +84,7 @@ void __init riscv_fill_hwcap(void)
 	for_each_of_cpu_node(node) {
 		unsigned long this_hwcap = 0;
 		DECLARE_BITMAP(this_isa, RISCV_ISA_EXT_MAX);
+		const char *temp;
 
 		if (riscv_of_processor_hartid(node) < 0)
 			continue;
@@ -93,6 +94,7 @@ void __init riscv_fill_hwcap(void)
 			continue;
 		}
 
+		temp = isa;
 #if IS_ENABLED(CONFIG_32BIT)
 		if (!strncmp(isa, "rv32", 4))
 			isa += 4;
@@ -100,6 +102,9 @@ void __init riscv_fill_hwcap(void)
 		if (!strncmp(isa, "rv64", 4))
 			isa += 4;
 #endif
+		/* The riscv,isa DT property must start with rv64 or rv32 */
+		if (temp == isa)
+			continue;
 		bitmap_zero(this_isa, RISCV_ISA_EXT_MAX);
 		for (; *isa; ++isa) {
 			const char *ext = isa++;
-- 
2.30.2

[PATCH v5 6/6] RISC-V: Improve /proc/cpuinfo output for ISA extensions

[Atish Patra]

Currently, the /proc/cpuinfo outputs the entire riscv,isa string which
is not ideal when we have multiple ISA extensions present in the ISA
string. Some of them may not be enabled in kernel as well.
Same goes for the single letter extensions as well which prints the
entire ISA string. Some of they may not be valid ISA extensions as
well (e.g 'su')

Parse only the valid & enabled ISA extension and print them.

Tested-by: Heiko Stuebner <heiko@sntech.de>
Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 arch/riscv/include/asm/hwcap.h |  7 +++++
 arch/riscv/kernel/cpu.c        | 51 ++++++++++++++++++++++++++++++++--
 2 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index 170bd80da520..691fc9c8099b 100644
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -54,6 +54,13 @@ enum riscv_isa_ext_id {
 	RISCV_ISA_EXT_ID_MAX = RISCV_ISA_EXT_MAX,
 };
 
+struct riscv_isa_ext_data {
+	/* Name of the extension displayed to userspace via /proc/cpuinfo */
+	char uprop[RISCV_ISA_EXT_NAME_LEN_MAX];
+	/* The logical ISA extension ID */
+	unsigned int isa_ext_id;
+};
+
 unsigned long riscv_isa_extension_base(const unsigned long *isa_bitmap);
 
 #define riscv_isa_extension_mask(ext) BIT_MASK(RISCV_ISA_EXT_##ext)
diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
index ad0a7e9f828b..031ad15a059f 100644
--- a/arch/riscv/kernel/cpu.c
+++ b/arch/riscv/kernel/cpu.c
@@ -6,6 +6,7 @@
 #include <linux/init.h>
 #include <linux/seq_file.h>
 #include <linux/of.h>
+#include <asm/hwcap.h>
 #include <asm/smp.h>
 #include <asm/pgtable.h>
 
@@ -63,12 +64,57 @@ int riscv_of_parent_hartid(struct device_node *node)
 }
 
 #ifdef CONFIG_PROC_FS
+#define __RISCV_ISA_EXT_DATA(UPROP, EXTID) \
+	{							\
+		.uprop = #UPROP,				\
+		.isa_ext_id = EXTID,				\
+	}
+
+static struct riscv_isa_ext_data isa_ext_arr[] = {
+	__RISCV_ISA_EXT_DATA("", RISCV_ISA_EXT_MAX),
+};
+
+static void print_isa_ext(struct seq_file *f)
+{
+	struct riscv_isa_ext_data *edata;
+	int i = 0, arr_sz;
+
+	arr_sz = ARRAY_SIZE(isa_ext_arr) - 1;
+
+	/* No extension support available */
+	if (arr_sz <= 0)
+		return;
+
+	seq_puts(f, "isa-ext\t\t: ");
+	for (i = 0; i <= arr_sz; i++) {
+		edata = &isa_ext_arr[i];
+		if (!__riscv_isa_extension_available(NULL, edata->isa_ext_id))
+			continue;
+		seq_printf(f, "%s ", edata->uprop);
+	}
+	seq_puts(f, "\n");
+}
+
+/**
+ * These are the only valid base (single letter) ISA extensions as per the spec.
+ * It also specifies the canonical order in which it appears in the spec.
+ * Some of the extension may just be a place holder for now (B, K, P, J).
+ * This should be updated once corresponding extensions are ratified.
+ */
+static const char base_riscv_exts[13] = "imafdqcbkjpvh";
 
 static void print_isa(struct seq_file *f, const char *isa)
 {
-	/* Print the entire ISA as it is */
+	int i;
+
 	seq_puts(f, "isa\t\t: ");
-	seq_write(f, isa, strlen(isa));
+	/* Print the rv[64/32] part */
+	seq_write(f, isa, 4);
+	for (i = 0; i < sizeof(base_riscv_exts); i++) {
+		if (__riscv_isa_extension_available(NULL, base_riscv_exts[i] - 'a'))
+			/* Print only enabled the base ISA extensions */
+			seq_write(f, &base_riscv_exts[i], 1);
+	}
 	seq_puts(f, "\n");
 }
 
@@ -115,6 +161,7 @@ static int c_show(struct seq_file *m, void *v)
 	seq_printf(m, "hart\t\t: %lu\n", cpuid_to_hartid_map(cpu_id));
 	if (!of_property_read_string(node, "riscv,isa", &isa))
 		print_isa(m, isa);
+	print_isa_ext(m);
 	print_mmu(m);
 	if (!of_property_read_string(node, "compatible", &compat)
 	    && strcmp(compat, "riscv"))
-- 
2.30.2

Re: [PATCH v5 2/6] RISC-V: Minimal parser for "riscv, isa" strings

[Anup Patel]

On Wed, Feb 23, 2022 at 2:18 AM Atish Patra <atishp@rivosinc.com> wrote:
>
> From: Tsukasa OI <research_trasio@irq.a4lg.com>
>
> Current hart ISA ("riscv,isa") parser don't correctly parse:
>
> 1. Multi-letter extensions
> 2. Version numbers
>
> All ISA extensions ratified recently has multi-letter extensions
> (except 'H'). 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.
>
> [Improved commit text and fixed a bug around 's' in base extension]
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> [Fixed workaround for QEMU]
> Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com>
> Tested-by: Heiko Stuebner <heiko@sntech.de>

Looks good to me.

Reviewed-by: Anup Patel <anup@brainfault.org>

Regards,
Anup

> ---
>  arch/riscv/kernel/cpufeature.c | 72 ++++++++++++++++++++++++++++------
>  1 file changed, 61 insertions(+), 11 deletions(-)
>
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index dd3d57eb4eea..72c5f6ef56b5 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,72 @@ 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++;
> +                       const char *ext_end = isa;
> +                       bool ext_long = false, ext_err = false;
> +
> +                       switch (*ext) {
> +                       case 's':
> +                               /**
> +                                * Workaround for invalid single-letter 's' & 'u'(QEMU).
> +                                * No need to set the bit in riscv_isa as 's' & 'u' are
> +                                * not valid ISA extensions. It works until multi-letter
> +                                * extension starting with "Su" appears.
> +                                */
> +                               if (ext[-1] != '_' && ext[1] == 'u') {
> +                                       ++isa;
> +                                       ext_err = true;
> +                                       break;
> +                               }
> +                               fallthrough;
> +                       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;
> +                               break;
> +                       default:
> +                               if (unlikely(!islower(*ext))) {
> +                                       ext_err = true;
> +                                       break;
> +                               }
> +                               /* Find next extension */
> +                               if (!isdigit(*isa))
> +                                       break;
> +                               /* Skip the minor version */
> +                               while (isdigit(*++isa))
> +                                       ;
> +                               if (*isa != 'p')
> +                                       break;
> +                               if (!isdigit(*++isa)) {
> +                                       --isa;
> +                                       break;
> +                               }
> +                               /* Skip the major version */
> +                               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.30.2
>

Re: [PATCH v5 3/6] RISC-V: Extract multi-letter extension names from "riscv, isa"

[Anup Patel]

On Wed, Feb 23, 2022 at 2:18 AM Atish Patra <atishp@rivosinc.com> wrote:
>
> From: Tsukasa OI <research_trasio@irq.a4lg.com>
>
> Currently, there is no usage for version numbers in extensions as
> any ratified non base ISA extension will always at v1.0.
>
> Extract the extension names in place for future parsing.
>
> Tested-by: Heiko Stuebner <heiko@sntech.de>
> Reviewed-by: Anup Patel <anup@brainfault.org>
> Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com>
> [Improved commit text and comments]
> Signed-off-by: Atish Patra <atishp@rivosinc.com>

Looks good to me.

Reviewed-by: Anup Patel <anup@brainfault.org>

Regards,
Anup

> ---
>  arch/riscv/kernel/cpufeature.c | 35 ++++++++++++++++++++++++++--------
>  1 file changed, 27 insertions(+), 8 deletions(-)
>
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 72c5f6ef56b5..b0df7eff47f7 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -124,8 +124,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;
> +                               /* Parse backwards */
> +                               ext_end = isa;
> +                               if (unlikely(ext_err))
> +                                       break;
> +                               if (!isdigit(ext_end[-1]))
> +                                       break;
> +                               /* Skip the minor version */
> +                               while (isdigit(*--ext_end))
> +                                       ;
> +                               if (ext_end[0] != 'p'
> +                                   || !isdigit(ext_end[-1])) {
> +                                       /* Advance it to offset the pre-decrement */
> +                                       ++ext_end;
> +                                       break;
> +                               }
> +                               /* Skip the major version */
> +                               while (isdigit(*--ext_end))
> +                                       ;
> +                               ++ext_end;
>                                 break;
>                         default:
>                                 if (unlikely(!islower(*ext))) {
> @@ -151,14 +171,13 @@ 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)
> +
> +                       if (unlikely(ext_err))
>                                 continue;
> -                       this_hwcap |= isa2hwcap[(unsigned char)(*ext)];
> -                       this_isa |= (1UL << (*ext - 'a'));
> +                       if (!ext_long) {
> +                               this_hwcap |= isa2hwcap[(unsigned char)(*ext)];
> +                               this_isa |= (1UL << (*ext - 'a'));
> +                       }
>                 }
>
>                 /*
> --
> 2.30.2
>

Re: [PATCH v5 4/6] RISC-V: Implement multi-letter ISA extension probing framework

[Anup Patel]

On Wed, Feb 23, 2022 at 2:18 AM Atish Patra <atishp@rivosinc.com> wrote:
>
> Multi-letter extensions can be probed using exising
> riscv_isa_extension_available API now. It doesn't support versioning
> right now as there is no use case for it.
> Individual extension specific implementation will be added during
> each extension support.
>
> Tested-by: Heiko Stuebner <heiko@sntech.de>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>

Looks good to me.

Reviewed-by: Anup Patel <anup@brainfault.org>

Regards,
Anup

> ---
>  arch/riscv/include/asm/hwcap.h | 18 ++++++++++++++++++
>  arch/riscv/kernel/cpufeature.c | 24 ++++++++++++++++++------
>  2 files changed, 36 insertions(+), 6 deletions(-)
>
> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> index 5ce50468aff1..170bd80da520 100644
> --- a/arch/riscv/include/asm/hwcap.h
> +++ b/arch/riscv/include/asm/hwcap.h
> @@ -34,7 +34,25 @@ extern unsigned long elf_hwcap;
>  #define RISCV_ISA_EXT_s                ('s' - 'a')
>  #define RISCV_ISA_EXT_u                ('u' - 'a')
>
> +/*
> + * Increse this to higher value as kernel support more ISA extensions.
> + */
>  #define RISCV_ISA_EXT_MAX      64
> +#define RISCV_ISA_EXT_NAME_LEN_MAX 32
> +
> +/* The base ID for multi-letter ISA extensions */
> +#define RISCV_ISA_EXT_BASE 26
> +
> +/*
> + * This enum represent the logical ID for each multi-letter RISC-V ISA extension.
> + * The logical ID should start from RISCV_ISA_EXT_BASE and must not exceed
> + * RISCV_ISA_EXT_MAX. 0-25 range is reserved for single letter
> + * extensions while all the multi-letter extensions should define the next
> + * available logical extension id.
> + */
> +enum riscv_isa_ext_id {
> +       RISCV_ISA_EXT_ID_MAX = RISCV_ISA_EXT_MAX,
> +};
>
>  unsigned long riscv_isa_extension_base(const unsigned long *isa_bitmap);
>
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index b0df7eff47f7..c6693873e95c 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -83,7 +83,7 @@ void __init riscv_fill_hwcap(void)
>
>         for_each_of_cpu_node(node) {
>                 unsigned long this_hwcap = 0;
> -               unsigned long this_isa = 0;
> +               DECLARE_BITMAP(this_isa, RISCV_ISA_EXT_MAX);
>
>                 if (riscv_of_processor_hartid(node) < 0)
>                         continue;
> @@ -100,6 +100,7 @@ void __init riscv_fill_hwcap(void)
>                 if (!strncmp(isa, "rv64", 4))
>                         isa += 4;
>  #endif
> +               bitmap_zero(this_isa, RISCV_ISA_EXT_MAX);
>                 for (; *isa; ++isa) {
>                         const char *ext = isa++;
>                         const char *ext_end = isa;
> @@ -172,12 +173,22 @@ 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)) {    \
> +                                       set_bit(bit, this_isa);                 \
> +                                       pr_info("Found ISA extension %s", name);\
> +                               }                                               \
> +                       } while (false)                                         \
> +
>                         if (unlikely(ext_err))
>                                 continue;
>                         if (!ext_long) {
>                                 this_hwcap |= isa2hwcap[(unsigned char)(*ext)];
> -                               this_isa |= (1UL << (*ext - 'a'));
> +                               set_bit(*ext - 'a', this_isa);
>                         }
> +#undef SET_ISA_EXT_MAP
>                 }
>
>                 /*
> @@ -190,10 +201,11 @@ void __init riscv_fill_hwcap(void)
>                 else
>                         elf_hwcap = this_hwcap;
>
> -               if (riscv_isa[0])
> -                       riscv_isa[0] &= this_isa;
> +               if (bitmap_weight(riscv_isa, RISCV_ISA_EXT_MAX))
> +                       bitmap_and(riscv_isa, riscv_isa, this_isa, RISCV_ISA_EXT_MAX);
>                 else
> -                       riscv_isa[0] = this_isa;
> +                       bitmap_copy(riscv_isa, this_isa, RISCV_ISA_EXT_MAX);
> +
>         }
>
>         /* We don't support systems with F but without D, so mask those out
> @@ -207,7 +219,7 @@ void __init riscv_fill_hwcap(void)
>         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);
> +       pr_info("riscv: base ISA extensions %s\n", print_str);
>
>         memset(print_str, 0, sizeof(print_str));
>         for (i = 0, j = 0; i < NUM_ALPHA_EXTS; i++)
> --
> 2.30.2
>

Re: [PATCH v5 5/6] RISC-V: Do no continue isa string parsing without correct XLEN

[Anup Patel]

On Wed, Feb 23, 2022 at 2:18 AM Atish Patra <atishp@rivosinc.com> wrote:
>
> The isa string should begin with either rv64 or rv32. Otherwise, it is
> an incorrect isa string. Currently, the string parsing continues even if
> it doesnot begin with current XLEN.
>
> Fix this by checking if it found "rv64" or "rv32" in the beginning.
>
> Tested-by: Heiko Stuebner <heiko@sntech.de>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>

Looks good to me.

Reviewed-by: Anup Patel <anup@brainfault.org>

Regards,
Anup

> ---
>  arch/riscv/kernel/cpufeature.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index c6693873e95c..f3a4b0619aa0 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -84,6 +84,7 @@ void __init riscv_fill_hwcap(void)
>         for_each_of_cpu_node(node) {
>                 unsigned long this_hwcap = 0;
>                 DECLARE_BITMAP(this_isa, RISCV_ISA_EXT_MAX);
> +               const char *temp;
>
>                 if (riscv_of_processor_hartid(node) < 0)
>                         continue;
> @@ -93,6 +94,7 @@ void __init riscv_fill_hwcap(void)
>                         continue;
>                 }
>
> +               temp = isa;
>  #if IS_ENABLED(CONFIG_32BIT)
>                 if (!strncmp(isa, "rv32", 4))
>                         isa += 4;
> @@ -100,6 +102,9 @@ void __init riscv_fill_hwcap(void)
>                 if (!strncmp(isa, "rv64", 4))
>                         isa += 4;
>  #endif
> +               /* The riscv,isa DT property must start with rv64 or rv32 */
> +               if (temp == isa)
> +                       continue;
>                 bitmap_zero(this_isa, RISCV_ISA_EXT_MAX);
>                 for (; *isa; ++isa) {
>                         const char *ext = isa++;
> --
> 2.30.2
>

Re: [PATCH v5 6/6] RISC-V: Improve /proc/cpuinfo output for ISA extensions

[Anup Patel]

On Wed, Feb 23, 2022 at 2:18 AM Atish Patra <atishp@rivosinc.com> wrote:
>
> Currently, the /proc/cpuinfo outputs the entire riscv,isa string which
> is not ideal when we have multiple ISA extensions present in the ISA
> string. Some of them may not be enabled in kernel as well.
> Same goes for the single letter extensions as well which prints the
> entire ISA string. Some of they may not be valid ISA extensions as
> well (e.g 'su')
>
> Parse only the valid & enabled ISA extension and print them.
>
> Tested-by: Heiko Stuebner <heiko@sntech.de>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>

Looks good to me.

Reviewed-by: Anup Patel <anup@brainfault.org>

Regards,
Anup

> ---
>  arch/riscv/include/asm/hwcap.h |  7 +++++
>  arch/riscv/kernel/cpu.c        | 51 ++++++++++++++++++++++++++++++++--
>  2 files changed, 56 insertions(+), 2 deletions(-)
>
> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> index 170bd80da520..691fc9c8099b 100644
> --- a/arch/riscv/include/asm/hwcap.h
> +++ b/arch/riscv/include/asm/hwcap.h
> @@ -54,6 +54,13 @@ enum riscv_isa_ext_id {
>         RISCV_ISA_EXT_ID_MAX = RISCV_ISA_EXT_MAX,
>  };
>
> +struct riscv_isa_ext_data {
> +       /* Name of the extension displayed to userspace via /proc/cpuinfo */
> +       char uprop[RISCV_ISA_EXT_NAME_LEN_MAX];
> +       /* The logical ISA extension ID */
> +       unsigned int isa_ext_id;
> +};
> +
>  unsigned long riscv_isa_extension_base(const unsigned long *isa_bitmap);
>
>  #define riscv_isa_extension_mask(ext) BIT_MASK(RISCV_ISA_EXT_##ext)
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index ad0a7e9f828b..031ad15a059f 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -6,6 +6,7 @@
>  #include <linux/init.h>
>  #include <linux/seq_file.h>
>  #include <linux/of.h>
> +#include <asm/hwcap.h>
>  #include <asm/smp.h>
>  #include <asm/pgtable.h>
>
> @@ -63,12 +64,57 @@ int riscv_of_parent_hartid(struct device_node *node)
>  }
>
>  #ifdef CONFIG_PROC_FS
> +#define __RISCV_ISA_EXT_DATA(UPROP, EXTID) \
> +       {                                                       \
> +               .uprop = #UPROP,                                \
> +               .isa_ext_id = EXTID,                            \
> +       }
> +
> +static struct riscv_isa_ext_data isa_ext_arr[] = {
> +       __RISCV_ISA_EXT_DATA("", RISCV_ISA_EXT_MAX),
> +};
> +
> +static void print_isa_ext(struct seq_file *f)
> +{
> +       struct riscv_isa_ext_data *edata;
> +       int i = 0, arr_sz;
> +
> +       arr_sz = ARRAY_SIZE(isa_ext_arr) - 1;
> +
> +       /* No extension support available */
> +       if (arr_sz <= 0)
> +               return;
> +
> +       seq_puts(f, "isa-ext\t\t: ");
> +       for (i = 0; i <= arr_sz; i++) {
> +               edata = &isa_ext_arr[i];
> +               if (!__riscv_isa_extension_available(NULL, edata->isa_ext_id))
> +                       continue;
> +               seq_printf(f, "%s ", edata->uprop);
> +       }
> +       seq_puts(f, "\n");
> +}
> +
> +/**
> + * These are the only valid base (single letter) ISA extensions as per the spec.
> + * It also specifies the canonical order in which it appears in the spec.
> + * Some of the extension may just be a place holder for now (B, K, P, J).
> + * This should be updated once corresponding extensions are ratified.
> + */
> +static const char base_riscv_exts[13] = "imafdqcbkjpvh";
>
>  static void print_isa(struct seq_file *f, const char *isa)
>  {
> -       /* Print the entire ISA as it is */
> +       int i;
> +
>         seq_puts(f, "isa\t\t: ");
> -       seq_write(f, isa, strlen(isa));
> +       /* Print the rv[64/32] part */
> +       seq_write(f, isa, 4);
> +       for (i = 0; i < sizeof(base_riscv_exts); i++) {
> +               if (__riscv_isa_extension_available(NULL, base_riscv_exts[i] - 'a'))
> +                       /* Print only enabled the base ISA extensions */
> +                       seq_write(f, &base_riscv_exts[i], 1);
> +       }
>         seq_puts(f, "\n");
>  }
>
> @@ -115,6 +161,7 @@ static int c_show(struct seq_file *m, void *v)
>         seq_printf(m, "hart\t\t: %lu\n", cpuid_to_hartid_map(cpu_id));
>         if (!of_property_read_string(node, "riscv,isa", &isa))
>                 print_isa(m, isa);
> +       print_isa_ext(m);
>         print_mmu(m);
>         if (!of_property_read_string(node, "compatible", &compat)
>             && strcmp(compat, "riscv"))
> --
> 2.30.2
>

Re: [PATCH v5 0/6] Provide a fraemework for RISC-V ISA extensions

[Palmer Dabbelt]

On Tue, 22 Feb 2022 12:48:05 PST (-0800), Atish Patra wrote:
> This series implements a generic framework to parse multi-letter ISA
> extensions. This series is based on Tsukasa's v3 isa extension improvement
> series[1]. I have fixed few bugs and improved comments from that series
> (PATCH1-3). I have not used PATCH 4 from that series as we are not using
> ISA extension versioning as of now. We can add that later if required.
>
> PATCH 4 allows the probing of multi-letter extensions via a macro.
> It continues to use the common isa extensions between all the harts.
> Thus hetergenous hart systems will only see the common ISA extensions.
>
> PATCH 6 improves the /proc/cpuinfo interface for the available ISA extensions
> via /proc/cpuinfo.
>
> Here is the example output of /proc/cpuinfo:
> (with debug patches in Qemu and Linux kernel)
>
> # cat /proc/cpuinfo
> processor	: 0
> hart		: 0
> isa		: rv64imafdch
> isa-ext		: svpbmt svnapot svinval

I know it might seem a bit pedantic, but I really don't want to 
introduce a new format for encoding ISA extensions -- doubly so if this 
is the only way we're giving this info to userspace, as then we're just 
asking folks to turn this into a defacto ABI.  Every time we try to do 
something that's sort of like an ISA string but not exactly what's in 
the spec we end up getting burned, and while I don't see a specific way 
that could happen here that's what's happened so many times before I 
just don't want to risk it.

I've gone ahead and removed some of this information (isa-ext, and all 
the single-letter extensions we can't properly turn on yet) from 
/proc/cpuinfo, modifying the last patch to do so.  It's at 
palmer/riscv-isa, LMK if that's OK.

I'm not opposed to doing something here, I just really don't want to 
rush into it as we've already got enough complexity around the various 
flavors of ISA strings.  I don't see a big pressing need to provide this 
information to userspace, but having the rest of this sorted out is 
great (and there's some dependencies on this) so I'd prefer to just 
stick to what we know is good.

> mmu		: sv48
>
> processor	: 1
> hart		: 1
> isa		: rv64imafdch
> isa-ext		: svpbmt svnapot svinval
> mmu		: sv48
>
> processor	: 2
> hart		: 2
> isa		: rv64imafdch
> isa-ext		: svpbmt svnapot svinval
> mmu		: sv48
>
> processor	: 3
> hart		: 3
> isa		: rv64imafdch
> isa-ext		: svpbmt svnapot svinval
> mmu		: sv48
>
> Anybody adding support for any new multi-letter extensions should add an
> entry to the riscv_isa_ext_id and the isa extension array.
> E.g. The patch[2] adds the support for various ISA extensions.
>
> [1] https://lore.kernel.org/all/0f568515-a05e-8204-aae3-035975af3ee8@irq.a4lg.com/T/
> [2] https://github.com/atishp04/linux/commit/e9e240c9a854dceb434ceb53bdbe82a657bee5f2
>
> Changes from v4->v5:
> 1. Improved the /proc/cpuinfo to include only valid & enabled extensions
> 2. Improved the multi-letter parsing by skipping the 'su' modes generated in
>    Qemu as suggested by Tsukasa.
>
> Changes from v3->v4:
> 1. Changed temporary variable for current hart isa to a bitmap
> 2. Added reviewed-by tags.
> 3. Improved comments
>
> Changes from v2->v3:
> 1. Updated comments to mark clearly a fix required for Qemu only.
> 2. Fixed a bug where the 1st multi-letter extension can be present without _
> 3. Added Tested by tags.
>
> Changes from v1->v2:
> 1. Instead of adding a separate DT property use the riscv,isa property.
> 2. Based on Tsukasa's v3 isa extension improvement series.
>
> Atish Patra (3):
> RISC-V: Implement multi-letter ISA extension probing framework
> RISC-V: Do no continue isa string parsing without correct XLEN
> RISC-V: Improve /proc/cpuinfo output for ISA extensions
>
> Tsukasa OI (3):
> RISC-V: Correctly print supported extensions
> RISC-V: Minimal parser for "riscv, isa" strings
> RISC-V: Extract multi-letter extension names from "riscv, isa"
>
> arch/riscv/include/asm/hwcap.h |  25 +++++++
> arch/riscv/kernel/cpu.c        |  51 ++++++++++++-
> arch/riscv/kernel/cpufeature.c | 130 +++++++++++++++++++++++++++------
> 3 files changed, 183 insertions(+), 23 deletions(-)

Re: [PATCH v5 0/6] Provide a fraemework for RISC-V ISA extensions

[Atish Kumar Patra]

On Thu, Mar 10, 2022 at 3:50 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
> On Tue, 22 Feb 2022 12:48:05 PST (-0800), Atish Patra wrote:
> > This series implements a generic framework to parse multi-letter ISA
> > extensions. This series is based on Tsukasa's v3 isa extension improvement
> > series[1]. I have fixed few bugs and improved comments from that series
> > (PATCH1-3). I have not used PATCH 4 from that series as we are not using
> > ISA extension versioning as of now. We can add that later if required.
> >
> > PATCH 4 allows the probing of multi-letter extensions via a macro.
> > It continues to use the common isa extensions between all the harts.
> > Thus hetergenous hart systems will only see the common ISA extensions.
> >
> > PATCH 6 improves the /proc/cpuinfo interface for the available ISA extensions
> > via /proc/cpuinfo.
> >
> > Here is the example output of /proc/cpuinfo:
> > (with debug patches in Qemu and Linux kernel)
> >
> > # cat /proc/cpuinfo
> > processor     : 0
> > hart          : 0
> > isa           : rv64imafdch
> > isa-ext               : svpbmt svnapot svinval
>
> I know it might seem a bit pedantic, but I really don't want to
> introduce a new format for encoding ISA extensions -- doubly so if this
> is the only way we're giving this info to userspace, as then we're just
> asking folks to turn this into a defacto ABI.  Every time we try to do
> something that's sort of like an ISA string but not exactly what's in
> the spec we end up getting burned, and while I don't see a specific way

I agree that this is an ABI change/improvement which is impossible to
modify later.
However, this is a Linux specific ABI. Do you think the RISC-V spec
will ever say anything about how /proc/cpuinfo is shown to the user ?

and we do have similar precedence in other arch

/proc/cpuinfo output in  x86:
flags : fpu vme .... arch_capabilities
vmx flags : vnmi preemption_timer ..tsc_scaling

/proc/cpuinfo output in  arm64:

Features  : fp asimd evtstrm aes pmull sha1 sha2 crc32


> that could happen here that's what's happened so many times before I
> just don't want to risk it.
>
> I've gone ahead and removed some of this information (isa-ext, and all
> the single-letter extensions we can't properly turn on yet) from
> /proc/cpuinfo, modifying the last patch to do so.  It's at
> palmer/riscv-isa, LMK if that's OK.
>

Few changes required on your tree:

riscv_isa_ext_data definition is no longer required

and this should be to show hypervisor extension:
+static const char *base_riscv_exts = "imafdch";

> I'm not opposed to doing something here, I just really don't want to
> rush into it as we've already got enough complexity around the various
> flavors of ISA strings.  I don't see a big pressing need to provide this
> information to userspace, but having the rest of this sorted out is
> great (and there's some dependencies on this) so I'd prefer to just
> stick to what we know is good.
>

"isa-ext" row is kind of helpful to inform the developer to verify
that a specific extension is available
without looking at extension specific dmesg logs. It proved to be
useful while developing sstc/sscofpmf.

Is it better if we just dump the entire ISA string as before so that
we know which extensions are available at least ?

> > mmu           : sv48
> >
> > processor     : 1
> > hart          : 1
> > isa           : rv64imafdch
> > isa-ext               : svpbmt svnapot svinval
> > mmu           : sv48
> >
> > processor     : 2
> > hart          : 2
> > isa           : rv64imafdch
> > isa-ext               : svpbmt svnapot svinval
> > mmu           : sv48
> >
> > processor     : 3
> > hart          : 3
> > isa           : rv64imafdch
> > isa-ext               : svpbmt svnapot svinval
> > mmu           : sv48
> >
> > Anybody adding support for any new multi-letter extensions should add an
> > entry to the riscv_isa_ext_id and the isa extension array.
> > E.g. The patch[2] adds the support for various ISA extensions.
> >
> > [1] https://lore.kernel.org/all/0f568515-a05e-8204-aae3-035975af3ee8@irq.a4lg.com/T/
> > [2] https://github.com/atishp04/linux/commit/e9e240c9a854dceb434ceb53bdbe82a657bee5f2
> >
> > Changes from v4->v5:
> > 1. Improved the /proc/cpuinfo to include only valid & enabled extensions
> > 2. Improved the multi-letter parsing by skipping the 'su' modes generated in
> >    Qemu as suggested by Tsukasa.
> >
> > Changes from v3->v4:
> > 1. Changed temporary variable for current hart isa to a bitmap
> > 2. Added reviewed-by tags.
> > 3. Improved comments
> >
> > Changes from v2->v3:
> > 1. Updated comments to mark clearly a fix required for Qemu only.
> > 2. Fixed a bug where the 1st multi-letter extension can be present without _
> > 3. Added Tested by tags.
> >
> > Changes from v1->v2:
> > 1. Instead of adding a separate DT property use the riscv,isa property.
> > 2. Based on Tsukasa's v3 isa extension improvement series.
> >
> > Atish Patra (3):
> > RISC-V: Implement multi-letter ISA extension probing framework
> > RISC-V: Do no continue isa string parsing without correct XLEN
> > RISC-V: Improve /proc/cpuinfo output for ISA extensions
> >
> > Tsukasa OI (3):
> > RISC-V: Correctly print supported extensions
> > RISC-V: Minimal parser for "riscv, isa" strings
> > RISC-V: Extract multi-letter extension names from "riscv, isa"
> >
> > arch/riscv/include/asm/hwcap.h |  25 +++++++
> > arch/riscv/kernel/cpu.c        |  51 ++++++++++++-
> > arch/riscv/kernel/cpufeature.c | 130 +++++++++++++++++++++++++++------
> > 3 files changed, 183 insertions(+), 23 deletions(-)

Re: [PATCH v5 0/6] Provide a fraemework for RISC-V ISA extensions

[Nick Kossifidis]

Στις 2022-03-11 02:21, Atish Kumar Patra έγραψε:
> On Thu, Mar 10, 2022 at 3:50 PM Palmer Dabbelt <palmer@dabbelt.com> 
> wrote:
>> 
>> On Tue, 22 Feb 2022 12:48:05 PST (-0800), Atish Patra wrote:
>> > This series implements a generic framework to parse multi-letter ISA
>> > extensions. This series is based on Tsukasa's v3 isa extension improvement
>> > series[1]. I have fixed few bugs and improved comments from that series
>> > (PATCH1-3). I have not used PATCH 4 from that series as we are not using
>> > ISA extension versioning as of now. We can add that later if required.
>> >
>> > PATCH 4 allows the probing of multi-letter extensions via a macro.
>> > It continues to use the common isa extensions between all the harts.
>> > Thus hetergenous hart systems will only see the common ISA extensions.
>> >
>> > PATCH 6 improves the /proc/cpuinfo interface for the available ISA extensions
>> > via /proc/cpuinfo.
>> >
>> > Here is the example output of /proc/cpuinfo:
>> > (with debug patches in Qemu and Linux kernel)
>> >
>> > # cat /proc/cpuinfo
>> > processor     : 0
>> > hart          : 0
>> > isa           : rv64imafdch
>> > isa-ext               : svpbmt svnapot svinval
>> 
>> I know it might seem a bit pedantic, but I really don't want to
>> introduce a new format for encoding ISA extensions -- doubly so if 
>> this
>> is the only way we're giving this info to userspace, as then we're 
>> just
>> asking folks to turn this into a defacto ABI.  Every time we try to do
>> something that's sort of like an ISA string but not exactly what's in
>> the spec we end up getting burned, and while I don't see a specific 
>> way
> 
> I agree that this is an ABI change/improvement which is impossible to
> modify later.
> However, this is a Linux specific ABI. Do you think the RISC-V spec
> will ever say anything about how /proc/cpuinfo is shown to the user ?
> 

Actually there was a discussion on chairs at some point on how isa 
extensions will be represented as a single string. If I recall correctly 
they wanted a way to compare features between implementations so this 
was something the user should be able to read as well. I'm ccing Philipp 
from the Software HC in case he has more details on this.

I also believe we need to discuss this a bit further, also I thought we 
agreed that having everything as a single string (riscv-isa) on the 
device tree doesn't scale, there were some other suggestions regarding 
for example mmu extensions being declared inside an mmu sub-node etc. 
This patch series will not only make it hard to change /proc/cpuinfo 
output in the future, but also establishes a device-tree binding for all 
isa extensions through the riscv-isa string that we also won't be able 
to modify later on.

Regards,
Nick

Re: [PATCH v5 0/6] Provide a fraemework for RISC-V ISA extensions

[Anup Patel]

On Fri, Mar 11, 2022 at 6:13 PM Nick Kossifidis <mick@ics.forth.gr> wrote:
>
> Στις 2022-03-11 02:21, Atish Kumar Patra έγραψε:
> > On Thu, Mar 10, 2022 at 3:50 PM Palmer Dabbelt <palmer@dabbelt.com>
> > wrote:
> >>
> >> On Tue, 22 Feb 2022 12:48:05 PST (-0800), Atish Patra wrote:
> >> > This series implements a generic framework to parse multi-letter ISA
> >> > extensions. This series is based on Tsukasa's v3 isa extension improvement
> >> > series[1]. I have fixed few bugs and improved comments from that series
> >> > (PATCH1-3). I have not used PATCH 4 from that series as we are not using
> >> > ISA extension versioning as of now. We can add that later if required.
> >> >
> >> > PATCH 4 allows the probing of multi-letter extensions via a macro.
> >> > It continues to use the common isa extensions between all the harts.
> >> > Thus hetergenous hart systems will only see the common ISA extensions.
> >> >
> >> > PATCH 6 improves the /proc/cpuinfo interface for the available ISA extensions
> >> > via /proc/cpuinfo.
> >> >
> >> > Here is the example output of /proc/cpuinfo:
> >> > (with debug patches in Qemu and Linux kernel)
> >> >
> >> > # cat /proc/cpuinfo
> >> > processor     : 0
> >> > hart          : 0
> >> > isa           : rv64imafdch
> >> > isa-ext               : svpbmt svnapot svinval
> >>
> >> I know it might seem a bit pedantic, but I really don't want to
> >> introduce a new format for encoding ISA extensions -- doubly so if
> >> this
> >> is the only way we're giving this info to userspace, as then we're
> >> just
> >> asking folks to turn this into a defacto ABI.  Every time we try to do
> >> something that's sort of like an ISA string but not exactly what's in
> >> the spec we end up getting burned, and while I don't see a specific
> >> way
> >
> > I agree that this is an ABI change/improvement which is impossible to
> > modify later.
> > However, this is a Linux specific ABI. Do you think the RISC-V spec
> > will ever say anything about how /proc/cpuinfo is shown to the user ?
> >
>
> Actually there was a discussion on chairs at some point on how isa
> extensions will be represented as a single string. If I recall correctly
> they wanted a way to compare features between implementations so this
> was something the user should be able to read as well. I'm ccing Philipp
> from the Software HC in case he has more details on this.
>
> I also believe we need to discuss this a bit further, also I thought we
> agreed that having everything as a single string (riscv-isa) on the
> device tree doesn't scale, there were some other suggestions regarding
> for example mmu extensions being declared inside an mmu sub-node etc.
> This patch series will not only make it hard to change /proc/cpuinfo
> output in the future, but also establishes a device-tree binding for all
> isa extensions through the riscv-isa string that we also won't be able
> to modify later on.

Initially we can just follow the ISA string approach because this
is what RISC-V unpric spec defines.

Specifying ISA extensions via DT or ACPI, can be incrementally
done in future.

We have a lot of patches (Svpbmt, Sstc, Scofpmf, Zcboxyz, etc)
blocked because we don't have a way to detect multi-letter
extensions in Linux.

Regards,
Anup

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