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

[PATCH v2 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		: rv64imafdcsu
isa-ext		: sstc,sscofpmf
mmu		: sv48

processor	: 1
hart		: 1
isa		: rv64imafdcsu
isa-ext		: sstc,sscofpmf
mmu		: sv48

processor	: 2
hart		: 2
isa		: rv64imafdcsu
isa-ext		: sstc,sscofpmf
mmu		: sv48

processor	: 3
hart		: 3
isa		: rv64imafdcsu
isa-ext		: sstc,sscofpmf
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 sstc extension.

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


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        |  44 ++++++++++-
arch/riscv/kernel/cpufeature.c | 132 ++++++++++++++++++++++++++++-----
3 files changed, 180 insertions(+), 21 deletions(-)

--
2.30.2

[PATCH v2 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.

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

Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com>
[Improved commit text and fixed a bug around 's' in base extension]
Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 arch/riscv/kernel/cpufeature.c | 67 ++++++++++++++++++++++++++++------
 1 file changed, 56 insertions(+), 11 deletions(-)

diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index dd3d57eb4eea..e19ae4391a9b 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,67 @@ 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':
+				/**
+				 * 's' is a special case because:
+				 * It can be present in base extension for supervisor
+				 * Multi-letter extensions can start with 's' as well for
+				 * Supervisor extensions (i.e. sstc, sscofpmf, svinval)
+				 */
+				if (*ext == 's' && ext[-1] != '_')
+					break;
+				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.30.2

[PATCH v2 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.

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 | 41 +++++++++++++++++++++++++---------
 1 file changed, 31 insertions(+), 10 deletions(-)

diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index e19ae4391a9b..e9e3b0693d16 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) {
@@ -119,19 +120,39 @@ 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. */
+				 /* 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:
 				ext_long = false;
-				if (!islower(*ext)) {
+				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')
@@ -140,20 +161,20 @@ void __init riscv_fill_hwcap(void)
 					--isa;
 					break;
 				}
+				/* Skip the major version */
 				while (isdigit(*++isa))
 					;
 				break;
 			}
 			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 v2 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.

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 arch/riscv/include/asm/hwcap.h | 18 ++++++++++++++++++
 arch/riscv/kernel/cpufeature.c | 27 ++++++++++++++++++++++++---
 2 files changed, 42 insertions(+), 3 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 e9e3b0693d16..469b9739faf7 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;
+		uint64_t this_isa = 0;
 
 		if (riscv_of_processor_hartid(node) < 0)
 			continue;
@@ -169,12 +169,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)) {    \
+					this_isa |= (1UL << bit);		\
+					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'));
 			}
+#undef SET_ISA_EXT_MAP
 		}
 
 		/*
@@ -187,10 +197,21 @@ void __init riscv_fill_hwcap(void)
 		else
 			elf_hwcap = this_hwcap;
 
-		if (riscv_isa[0])
+		if (riscv_isa[0]) {
+#if IS_ENABLED(CONFIG_32BIT)
+			riscv_isa[0] &= this_isa & 0xFFFFFFFF;
+			riscv_isa[1] &= this_isa >> 32;
+#else
 			riscv_isa[0] &= this_isa;
-		else
+#endif
+		} else {
+#if IS_ENABLED(CONFIG_32BIT)
+			riscv_isa[0] = this_isa & 0xFFFFFFFF;
+			riscv_isa[1] = this_isa >> 32;
+#else
 			riscv_isa[0] = this_isa;
+#endif
+		}
 	}
 
 	/* We don't support systems with F but without D, so mask those out
-- 
2.30.2

[PATCH v2 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.

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 469b9739faf7..cca579bae8a0 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;
 		uint64_t this_isa = 0;
+		char *temp;
 
 		if (riscv_of_processor_hartid(node) < 0)
 			continue;
@@ -93,6 +94,7 @@ void __init riscv_fill_hwcap(void)
 			continue;
 		}
 
+		temp = (char *)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;
 		for (; *isa; ++isa) {
 			const char *ext = isa++;
 			const char *ext_end = isa;
-- 
2.30.2

[PATCH v2 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.

Parse only the enabled ISA extension and print them in a separate row.

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 arch/riscv/include/asm/hwcap.h |  7 ++++++
 arch/riscv/kernel/cpu.c        | 44 ++++++++++++++++++++++++++++++++--
 2 files changed, 49 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..ced7e5be8641 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,50 @@ 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");
+}
 
 static void print_isa(struct seq_file *f, const char *isa)
 {
-	/* Print the entire ISA as it is */
+	char *ext_start;
+	int isa_len = strlen(isa);
+	int base_isa_len = isa_len;
+
+	ext_start = strnchr(isa, isa_len, '_');
+	if (ext_start)
+		base_isa_len = isa_len - strlen(ext_start);
+
+	/* Print only the base ISA as it is */
 	seq_puts(f, "isa\t\t: ");
-	seq_write(f, isa, strlen(isa));
+	seq_write(f, isa, base_isa_len);
 	seq_puts(f, "\n");
 }
 
@@ -115,6 +154,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 v2 5/6] RISC-V: Do no continue isa string parsing without correct XLEN

[Andreas Schwab]

On Feb 10 2022, Atish Patra wrote:

> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 469b9739faf7..cca579bae8a0 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;
>  		uint64_t this_isa = 0;
> +		char *temp;
>  
>  		if (riscv_of_processor_hartid(node) < 0)
>  			continue;
> @@ -93,6 +94,7 @@ void __init riscv_fill_hwcap(void)
>  			continue;
>  		}
>  
> +		temp = (char *)isa;

There should be no need for this cast.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

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

[Geert Uytterhoeven]

Hi Andreas,

On Thu, Feb 10, 2022 at 11:00 PM Andreas Schwab <schwab@linux-m68k.org> wrote:
> On Feb 10 2022, Atish Patra wrote:
> > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > index 469b9739faf7..cca579bae8a0 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;
> >               uint64_t this_isa = 0;
> > +             char *temp;
> >
> >               if (riscv_of_processor_hartid(node) < 0)
> >                       continue;
> > @@ -93,6 +94,7 @@ void __init riscv_fill_hwcap(void)
> >                       continue;
> >               }
> >
> > +             temp = (char *)isa;
>
> There should be no need for this cast.

Indeed, but only if "temp" is changed to "const char *".

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Heiko Stübner]

Am Donnerstag, 10. Februar 2022, 22:40:16 CET schrieb 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.
> 
> Signed-off-by: Atish Patra <atishp@rivosinc.com>

Tested-by: Heiko Stuebner <heiko@sntech.de>


By the way, does a similar parsing exist for opensbi as well?
Things like svpbmt as well as zicbom have CSR bits controlling how
these functions should behave (enabling them, etc), so I guess
opensbi also needs to parse the extensions from the ISA string?


Heiko

> ---
>  arch/riscv/include/asm/hwcap.h | 18 ++++++++++++++++++
>  arch/riscv/kernel/cpufeature.c | 27 ++++++++++++++++++++++++---
>  2 files changed, 42 insertions(+), 3 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 e9e3b0693d16..469b9739faf7 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;
> +		uint64_t this_isa = 0;
>  
>  		if (riscv_of_processor_hartid(node) < 0)
>  			continue;
> @@ -169,12 +169,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)) {    \
> +					this_isa |= (1UL << bit);		\
> +					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'));
>  			}
> +#undef SET_ISA_EXT_MAP
>  		}
>  
>  		/*
> @@ -187,10 +197,21 @@ void __init riscv_fill_hwcap(void)
>  		else
>  			elf_hwcap = this_hwcap;
>  
> -		if (riscv_isa[0])
> +		if (riscv_isa[0]) {
> +#if IS_ENABLED(CONFIG_32BIT)
> +			riscv_isa[0] &= this_isa & 0xFFFFFFFF;
> +			riscv_isa[1] &= this_isa >> 32;
> +#else
>  			riscv_isa[0] &= this_isa;
> -		else
> +#endif
> +		} else {
> +#if IS_ENABLED(CONFIG_32BIT)
> +			riscv_isa[0] = this_isa & 0xFFFFFFFF;
> +			riscv_isa[1] = this_isa >> 32;
> +#else
>  			riscv_isa[0] = this_isa;
> +#endif
> +		}
>  	}
>  
>  	/* We don't support systems with F but without D, so mask those out
> 

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

[Heiko Stübner]

Am Donnerstag, 10. Februar 2022, 22:40:17 CET schrieb 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.
> 
> Signed-off-by: Atish Patra <atishp@rivosinc.com>

Tested-by: Heiko Stuebner <heiko@sntech.de>

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

[Heiko Stübner]

Am Donnerstag, 10. Februar 2022, 22:40:18 CET schrieb 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.
> 
> Parse only the enabled ISA extension and print them in a separate row.
> 
> Signed-off-by: Atish Patra <atishp@rivosinc.com>

Using this for my svpbmt series, it shows the extension nicely

Tested-by: Heiko Stuebner <heiko@sntech.de>

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

[Atish Patra]

On Mon, Feb 14, 2022 at 12:06 PM Heiko Stübner <heiko@sntech.de> wrote:
>
> Am Donnerstag, 10. Februar 2022, 22:40:16 CET schrieb 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.
> >
> > Signed-off-by: Atish Patra <atishp@rivosinc.com>
>
> Tested-by: Heiko Stuebner <heiko@sntech.de>
>
>
> By the way, does a similar parsing exist for opensbi as well?
> Things like svpbmt as well as zicbom have CSR bits controlling how
> these functions should behave (enabling them, etc), so I guess
> opensbi also needs to parse the extensions from the ISA string?
>
>

No. Currently, OpenSBI relies on the CSR read/write & trap method to
identify the extensions [1].

https://github.com/riscv-software-src/opensbi/blob/master/lib/sbi/sbi_hart.c#L404

In the future, zicbom can be detected in the same manner. However,
svpbmt is a bit tricky as it doesn't
define any new CSR. Do you think OpenSBI needs to detect svpbmt for any reason ?

> Heiko
>
> > ---
> >  arch/riscv/include/asm/hwcap.h | 18 ++++++++++++++++++
> >  arch/riscv/kernel/cpufeature.c | 27 ++++++++++++++++++++++++---
> >  2 files changed, 42 insertions(+), 3 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 e9e3b0693d16..469b9739faf7 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;
> > +             uint64_t this_isa = 0;
> >
> >               if (riscv_of_processor_hartid(node) < 0)
> >                       continue;
> > @@ -169,12 +169,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)) {    \
> > +                                     this_isa |= (1UL << bit);               \
> > +                                     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'));
> >                       }
> > +#undef SET_ISA_EXT_MAP
> >               }
> >
> >               /*
> > @@ -187,10 +197,21 @@ void __init riscv_fill_hwcap(void)
> >               else
> >                       elf_hwcap = this_hwcap;
> >
> > -             if (riscv_isa[0])
> > +             if (riscv_isa[0]) {
> > +#if IS_ENABLED(CONFIG_32BIT)
> > +                     riscv_isa[0] &= this_isa & 0xFFFFFFFF;
> > +                     riscv_isa[1] &= this_isa >> 32;
> > +#else
> >                       riscv_isa[0] &= this_isa;
> > -             else
> > +#endif
> > +             } else {
> > +#if IS_ENABLED(CONFIG_32BIT)
> > +                     riscv_isa[0] = this_isa & 0xFFFFFFFF;
> > +                     riscv_isa[1] = this_isa >> 32;
> > +#else
> >                       riscv_isa[0] = this_isa;
> > +#endif
> > +             }
> >       }
> >
> >       /* We don't support systems with F but without D, so mask those out
> >
>
>
>
>


-- 
Regards,
Atish

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

[Atish Patra]

On Fri, Feb 11, 2022 at 4:52 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Andreas,
>
> On Thu, Feb 10, 2022 at 11:00 PM Andreas Schwab <schwab@linux-m68k.org> wrote:
> > On Feb 10 2022, Atish Patra wrote:
> > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > > index 469b9739faf7..cca579bae8a0 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;
> > >               uint64_t this_isa = 0;
> > > +             char *temp;
> > >
> > >               if (riscv_of_processor_hartid(node) < 0)
> > >                       continue;
> > > @@ -93,6 +94,7 @@ void __init riscv_fill_hwcap(void)
> > >                       continue;
> > >               }
> > >
> > > +             temp = (char *)isa;
> >
> > There should be no need for this cast.
>
> Indeed, but only if "temp" is changed to "const char *".
>

Yes. Fixed it in v3. Thanks.

> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds



-- 
Regards,
Atish

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

[Heiko Stübner]

Am Montag, 14. Februar 2022, 21:14:13 CET schrieb Atish Patra:
> On Mon, Feb 14, 2022 at 12:06 PM Heiko Stübner <heiko@sntech.de> wrote:
> >
> > Am Donnerstag, 10. Februar 2022, 22:40:16 CET schrieb 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.
> > >
> > > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> >
> > Tested-by: Heiko Stuebner <heiko@sntech.de>
> >
> >
> > By the way, does a similar parsing exist for opensbi as well?
> > Things like svpbmt as well as zicbom have CSR bits controlling how
> > these functions should behave (enabling them, etc), so I guess
> > opensbi also needs to parse the extensions from the ISA string?
> >
> >
> 
> No. Currently, OpenSBI relies on the CSR read/write & trap method to
> identify the extensions [1].
> 
> https://github.com/riscv-software-src/opensbi/blob/master/lib/sbi/sbi_hart.c#L404

I guess my question is more, who is supposed to set CBIE, CBCFE bits in the
ENVCFG CSR. I.e. at it's default settings CMO instructions will cause
illegal instructions until the level above does allow them.

When the kernel wants to call a cache-invalidate, from my reading menvcfg
needs to be modified accordingly - which would fall in SBI's court?


> In the future, zicbom can be detected in the same manner. However,
> svpbmt is a bit tricky as it doesn't
> define any new CSR. Do you think OpenSBI needs to detect svpbmt for any reason ?

There is the PBMTE bit in MENVCFG, which I found while looking through the
zicbom-parts, which is supposed to "control wheter svpbmt is available for
use". So I guess the question is the same as above :-)


Heiko


> > Heiko
> >
> > > ---
> > >  arch/riscv/include/asm/hwcap.h | 18 ++++++++++++++++++
> > >  arch/riscv/kernel/cpufeature.c | 27 ++++++++++++++++++++++++---
> > >  2 files changed, 42 insertions(+), 3 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 e9e3b0693d16..469b9739faf7 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;
> > > +             uint64_t this_isa = 0;
> > >
> > >               if (riscv_of_processor_hartid(node) < 0)
> > >                       continue;
> > > @@ -169,12 +169,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)) {    \
> > > +                                     this_isa |= (1UL << bit);               \
> > > +                                     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'));
> > >                       }
> > > +#undef SET_ISA_EXT_MAP
> > >               }
> > >
> > >               /*
> > > @@ -187,10 +197,21 @@ void __init riscv_fill_hwcap(void)
> > >               else
> > >                       elf_hwcap = this_hwcap;
> > >
> > > -             if (riscv_isa[0])
> > > +             if (riscv_isa[0]) {
> > > +#if IS_ENABLED(CONFIG_32BIT)
> > > +                     riscv_isa[0] &= this_isa & 0xFFFFFFFF;
> > > +                     riscv_isa[1] &= this_isa >> 32;
> > > +#else
> > >                       riscv_isa[0] &= this_isa;
> > > -             else
> > > +#endif
> > > +             } else {
> > > +#if IS_ENABLED(CONFIG_32BIT)
> > > +                     riscv_isa[0] = this_isa & 0xFFFFFFFF;
> > > +                     riscv_isa[1] = this_isa >> 32;
> > > +#else
> > >                       riscv_isa[0] = this_isa;
> > > +#endif
> > > +             }
> > >       }
> > >
> > >       /* We don't support systems with F but without D, so mask those out
> > >
> >
> >
> >
> >
> 
> 
> 

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

[Atish Patra]

On Mon, Feb 14, 2022 at 12:24 PM Heiko Stübner <heiko@sntech.de> wrote:
>
> Am Montag, 14. Februar 2022, 21:14:13 CET schrieb Atish Patra:
> > On Mon, Feb 14, 2022 at 12:06 PM Heiko Stübner <heiko@sntech.de> wrote:
> > >
> > > Am Donnerstag, 10. Februar 2022, 22:40:16 CET schrieb 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.
> > > >
> > > > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > >
> > > Tested-by: Heiko Stuebner <heiko@sntech.de>
> > >
> > >
> > > By the way, does a similar parsing exist for opensbi as well?
> > > Things like svpbmt as well as zicbom have CSR bits controlling how
> > > these functions should behave (enabling them, etc), so I guess
> > > opensbi also needs to parse the extensions from the ISA string?
> > >
> > >
> >
> > No. Currently, OpenSBI relies on the CSR read/write & trap method to
> > identify the extensions [1].
> >
> > https://github.com/riscv-software-src/opensbi/blob/master/lib/sbi/sbi_hart.c#L404
>
> I guess my question is more, who is supposed to set CBIE, CBCFE bits in the
> ENVCFG CSR. I.e. at it's default settings CMO instructions will cause
> illegal instructions until the level above does allow them.
>
> When the kernel wants to call a cache-invalidate, from my reading menvcfg
> needs to be modified accordingly - which would fall in SBI's court?
>

I think so. I had the same question for the SSTC extension as well.
This is what I currently do:

1. Detect menvcfg first, detect stimecmp
2. Enable SSTC feature only if both are available
3. Set the STCE bit in menvcfg if SSTC is available

Here is the patch
https://github.com/atishp04/opensbi/commit/e6b185821e8302bffdceb4633b413252e0de4889

>
> > In the future, zicbom can be detected in the same manner. However,
> > svpbmt is a bit tricky as it doesn't
> > define any new CSR. Do you think OpenSBI needs to detect svpbmt for any reason ?
>
> There is the PBMTE bit in MENVCFG, which I found while looking through the
> zicbom-parts, which is supposed to "control wheter svpbmt is available for
> use". So I guess the question is the same as above :-)
>

PBMTE bit in MENVCFG says if PBMTE bit is available or not. OpenSBI
needs other way to
detect if PBMTE is available.

That's why, I think MENVCFG should be set correctly by the hardware
upon reset. What do you think
about that ? I couldn't find anything related to the reset state for menvcfg.

>
> Heiko
>
>
> > > Heiko
> > >
> > > > ---
> > > >  arch/riscv/include/asm/hwcap.h | 18 ++++++++++++++++++
> > > >  arch/riscv/kernel/cpufeature.c | 27 ++++++++++++++++++++++++---
> > > >  2 files changed, 42 insertions(+), 3 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 e9e3b0693d16..469b9739faf7 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;
> > > > +             uint64_t this_isa = 0;
> > > >
> > > >               if (riscv_of_processor_hartid(node) < 0)
> > > >                       continue;
> > > > @@ -169,12 +169,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)) {    \
> > > > +                                     this_isa |= (1UL << bit);               \
> > > > +                                     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'));
> > > >                       }
> > > > +#undef SET_ISA_EXT_MAP
> > > >               }
> > > >
> > > >               /*
> > > > @@ -187,10 +197,21 @@ void __init riscv_fill_hwcap(void)
> > > >               else
> > > >                       elf_hwcap = this_hwcap;
> > > >
> > > > -             if (riscv_isa[0])
> > > > +             if (riscv_isa[0]) {
> > > > +#if IS_ENABLED(CONFIG_32BIT)
> > > > +                     riscv_isa[0] &= this_isa & 0xFFFFFFFF;
> > > > +                     riscv_isa[1] &= this_isa >> 32;
> > > > +#else
> > > >                       riscv_isa[0] &= this_isa;
> > > > -             else
> > > > +#endif
> > > > +             } else {
> > > > +#if IS_ENABLED(CONFIG_32BIT)
> > > > +                     riscv_isa[0] = this_isa & 0xFFFFFFFF;
> > > > +                     riscv_isa[1] = this_isa >> 32;
> > > > +#else
> > > >                       riscv_isa[0] = this_isa;
> > > > +#endif
> > > > +             }
> > > >       }
> > > >
> > > >       /* We don't support systems with F but without D, so mask those out
> > > >
> > >
> > >
> > >
> > >
> >
> >
> >
>
>
>
>


-- 
Regards,
Atish

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

[Heiko Stübner]

Am Montag, 14. Februar 2022, 21:42:32 CET schrieb Atish Patra:
> On Mon, Feb 14, 2022 at 12:24 PM Heiko Stübner <heiko@sntech.de> wrote:
> >
> > Am Montag, 14. Februar 2022, 21:14:13 CET schrieb Atish Patra:
> > > On Mon, Feb 14, 2022 at 12:06 PM Heiko Stübner <heiko@sntech.de> wrote:
> > > >
> > > > Am Donnerstag, 10. Februar 2022, 22:40:16 CET schrieb 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.
> > > > >
> > > > > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > > >
> > > > Tested-by: Heiko Stuebner <heiko@sntech.de>
> > > >
> > > >
> > > > By the way, does a similar parsing exist for opensbi as well?
> > > > Things like svpbmt as well as zicbom have CSR bits controlling how
> > > > these functions should behave (enabling them, etc), so I guess
> > > > opensbi also needs to parse the extensions from the ISA string?
> > > >
> > > >
> > >
> > > No. Currently, OpenSBI relies on the CSR read/write & trap method to
> > > identify the extensions [1].
> > >
> > > https://github.com/riscv-software-src/opensbi/blob/master/lib/sbi/sbi_hart.c#L404
> >
> > I guess my question is more, who is supposed to set CBIE, CBCFE bits in the
> > ENVCFG CSR. I.e. at it's default settings CMO instructions will cause
> > illegal instructions until the level above does allow them.
> >
> > When the kernel wants to call a cache-invalidate, from my reading menvcfg
> > needs to be modified accordingly - which would fall in SBI's court?
> >
> 
> I think so. I had the same question for the SSTC extension as well.
> This is what I currently do:
> 
> 1. Detect menvcfg first, detect stimecmp
> 2. Enable SSTC feature only if both are available
> 3. Set the STCE bit in menvcfg if SSTC is available
> 
> Here is the patch
> https://github.com/atishp04/opensbi/commit/e6b185821e8302bffdceb4633b413252e0de4889

Hmm, the CBO fields are defined as WARL (write any, read legal),
so I guess some sort of trap won't work here.

The priv-spec only points to the cmo-spec for these bits and the cmo-spec
does not specifiy what the value should be when cmo is not present.


> > > In the future, zicbom can be detected in the same manner. However,
> > > svpbmt is a bit tricky as it doesn't
> > > define any new CSR. Do you think OpenSBI needs to detect svpbmt for any reason ?
> >
> > There is the PBMTE bit in MENVCFG, which I found while looking through the
> > zicbom-parts, which is supposed to "control wheter svpbmt is available for
> > use". So I guess the question is the same as above :-)
> >
> 
> PBMTE bit in MENVCFG says if PBMTE bit is available or not. OpenSBI
> needs other way to
> detect if PBMTE is available.
> 
> That's why, I think MENVCFG should be set correctly by the hardware
> upon reset. What do you think
> about that ? I couldn't find anything related to the reset state for menvcfg.

me neither. Both the priv-spec as well as the cmobase spec do not
specifiy any reset-values it seems.

So I guess in the Qemu case, Qemu needs to set that bit when
its svpbmt extension is enabled?


Heiko


> > > > > ---
> > > > >  arch/riscv/include/asm/hwcap.h | 18 ++++++++++++++++++
> > > > >  arch/riscv/kernel/cpufeature.c | 27 ++++++++++++++++++++++++---
> > > > >  2 files changed, 42 insertions(+), 3 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 e9e3b0693d16..469b9739faf7 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;
> > > > > +             uint64_t this_isa = 0;
> > > > >
> > > > >               if (riscv_of_processor_hartid(node) < 0)
> > > > >                       continue;
> > > > > @@ -169,12 +169,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)) {    \
> > > > > +                                     this_isa |= (1UL << bit);               \
> > > > > +                                     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'));
> > > > >                       }
> > > > > +#undef SET_ISA_EXT_MAP
> > > > >               }
> > > > >
> > > > >               /*
> > > > > @@ -187,10 +197,21 @@ void __init riscv_fill_hwcap(void)
> > > > >               else
> > > > >                       elf_hwcap = this_hwcap;
> > > > >
> > > > > -             if (riscv_isa[0])
> > > > > +             if (riscv_isa[0]) {
> > > > > +#if IS_ENABLED(CONFIG_32BIT)
> > > > > +                     riscv_isa[0] &= this_isa & 0xFFFFFFFF;
> > > > > +                     riscv_isa[1] &= this_isa >> 32;
> > > > > +#else
> > > > >                       riscv_isa[0] &= this_isa;
> > > > > -             else
> > > > > +#endif
> > > > > +             } else {
> > > > > +#if IS_ENABLED(CONFIG_32BIT)
> > > > > +                     riscv_isa[0] = this_isa & 0xFFFFFFFF;
> > > > > +                     riscv_isa[1] = this_isa >> 32;
> > > > > +#else
> > > > >                       riscv_isa[0] = this_isa;
> > > > > +#endif
> > > > > +             }
> > > > >       }
> > > > >
> > > > >       /* We don't support systems with F but without D, so mask those out
> > > > >
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> >
> >
> >
> >
> 
> 
> 

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

[Atish Kumar Patra]

On Mon, Feb 14, 2022 at 2:22 PM Heiko Stübner <heiko@sntech.de> wrote:
>
> Am Montag, 14. Februar 2022, 21:42:32 CET schrieb Atish Patra:
> > On Mon, Feb 14, 2022 at 12:24 PM Heiko Stübner <heiko@sntech.de> wrote:
> > >
> > > Am Montag, 14. Februar 2022, 21:14:13 CET schrieb Atish Patra:
> > > > On Mon, Feb 14, 2022 at 12:06 PM Heiko Stübner <heiko@sntech.de> wrote:
> > > > >
> > > > > Am Donnerstag, 10. Februar 2022, 22:40:16 CET schrieb 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.
> > > > > >
> > > > > > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > > > >
> > > > > Tested-by: Heiko Stuebner <heiko@sntech.de>
> > > > >
> > > > >
> > > > > By the way, does a similar parsing exist for opensbi as well?
> > > > > Things like svpbmt as well as zicbom have CSR bits controlling how
> > > > > these functions should behave (enabling them, etc), so I guess
> > > > > opensbi also needs to parse the extensions from the ISA string?
> > > > >
> > > > >
> > > >
> > > > No. Currently, OpenSBI relies on the CSR read/write & trap method to
> > > > identify the extensions [1].
> > > >
> > > > https://github.com/riscv-software-src/opensbi/blob/master/lib/sbi/sbi_hart.c#L404
> > >
> > > I guess my question is more, who is supposed to set CBIE, CBCFE bits in the
> > > ENVCFG CSR. I.e. at it's default settings CMO instructions will cause
> > > illegal instructions until the level above does allow them.
> > >
> > > When the kernel wants to call a cache-invalidate, from my reading menvcfg
> > > needs to be modified accordingly - which would fall in SBI's court?
> > >
> >
> > I think so. I had the same question for the SSTC extension as well.
> > This is what I currently do:
> >
> > 1. Detect menvcfg first, detect stimecmp
> > 2. Enable SSTC feature only if both are available
> > 3. Set the STCE bit in menvcfg if SSTC is available
> >
> > Here is the patch
> > https://github.com/atishp04/opensbi/commit/e6b185821e8302bffdceb4633b413252e0de4889
>
> Hmm, the CBO fields are defined as WARL (write any, read legal),
> so I guess some sort of trap won't work here.
>

Correct. Traps for extensions that introduce new CSRs.
I was suggesting setting the corresponding bits in MENVCFG and reading
it again to check if it sticks.

> The priv-spec only points to the cmo-spec for these bits and the cmo-spec
> does not specifiy what the value should be when cmo is not present.
>
>
> > > > In the future, zicbom can be detected in the same manner. However,
> > > > svpbmt is a bit tricky as it doesn't
> > > > define any new CSR. Do you think OpenSBI needs to detect svpbmt for any reason ?
> > >
> > > There is the PBMTE bit in MENVCFG, which I found while looking through the
> > > zicbom-parts, which is supposed to "control wheter svpbmt is available for
> > > use". So I guess the question is the same as above :-)
> > >
> >
> > PBMTE bit in MENVCFG says if PBMTE bit is available or not. OpenSBI
> > needs other way to
> > detect if PBMTE is available.
> >
> > That's why, I think MENVCFG should be set correctly by the hardware
> > upon reset. What do you think
> > about that ? I couldn't find anything related to the reset state for menvcfg.
>
> me neither. Both the priv-spec as well as the cmobase spec do not
> specifiy any reset-values it seems.
>
I have raised an issue in the ISA spec.
https://github.com/riscv/riscv-isa-manual/issues/820

> So I guess in the Qemu case, Qemu needs to set that bit when
> its svpbmt extension is enabled?
>

We can do that if the priv spec is modified to allow that.

>
> Heiko
>
>
> > > > > > ---
> > > > > >  arch/riscv/include/asm/hwcap.h | 18 ++++++++++++++++++
> > > > > >  arch/riscv/kernel/cpufeature.c | 27 ++++++++++++++++++++++++---
> > > > > >  2 files changed, 42 insertions(+), 3 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 e9e3b0693d16..469b9739faf7 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;
> > > > > > +             uint64_t this_isa = 0;
> > > > > >
> > > > > >               if (riscv_of_processor_hartid(node) < 0)
> > > > > >                       continue;
> > > > > > @@ -169,12 +169,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)) {    \
> > > > > > +                                     this_isa |= (1UL << bit);               \
> > > > > > +                                     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'));
> > > > > >                       }
> > > > > > +#undef SET_ISA_EXT_MAP
> > > > > >               }
> > > > > >
> > > > > >               /*
> > > > > > @@ -187,10 +197,21 @@ void __init riscv_fill_hwcap(void)
> > > > > >               else
> > > > > >                       elf_hwcap = this_hwcap;
> > > > > >
> > > > > > -             if (riscv_isa[0])
> > > > > > +             if (riscv_isa[0]) {
> > > > > > +#if IS_ENABLED(CONFIG_32BIT)
> > > > > > +                     riscv_isa[0] &= this_isa & 0xFFFFFFFF;
> > > > > > +                     riscv_isa[1] &= this_isa >> 32;
> > > > > > +#else
> > > > > >                       riscv_isa[0] &= this_isa;
> > > > > > -             else
> > > > > > +#endif
> > > > > > +             } else {
> > > > > > +#if IS_ENABLED(CONFIG_32BIT)
> > > > > > +                     riscv_isa[0] = this_isa & 0xFFFFFFFF;
> > > > > > +                     riscv_isa[1] = this_isa >> 32;
> > > > > > +#else
> > > > > >                       riscv_isa[0] = this_isa;
> > > > > > +#endif
> > > > > > +             }
> > > > > >       }
> > > > > >
> > > > > >       /* We don't support systems with F but without D, so mask those out
> > > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> > >
> >
> >
> >
>
>
>
>

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

[Atish Kumar Patra]

On Mon, Feb 14, 2022 at 3:22 PM Atish Kumar Patra <atishp@rivosinc.com> wrote:
>
> On Mon, Feb 14, 2022 at 2:22 PM Heiko Stübner <heiko@sntech.de> wrote:
> >
> > Am Montag, 14. Februar 2022, 21:42:32 CET schrieb Atish Patra:
> > > On Mon, Feb 14, 2022 at 12:24 PM Heiko Stübner <heiko@sntech.de> wrote:
> > > >
> > > > Am Montag, 14. Februar 2022, 21:14:13 CET schrieb Atish Patra:
> > > > > On Mon, Feb 14, 2022 at 12:06 PM Heiko Stübner <heiko@sntech.de> wrote:
> > > > > >
> > > > > > Am Donnerstag, 10. Februar 2022, 22:40:16 CET schrieb 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.
> > > > > > >
> > > > > > > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > > > > >
> > > > > > Tested-by: Heiko Stuebner <heiko@sntech.de>
> > > > > >
> > > > > >
> > > > > > By the way, does a similar parsing exist for opensbi as well?
> > > > > > Things like svpbmt as well as zicbom have CSR bits controlling how
> > > > > > these functions should behave (enabling them, etc), so I guess
> > > > > > opensbi also needs to parse the extensions from the ISA string?
> > > > > >
> > > > > >
> > > > >
> > > > > No. Currently, OpenSBI relies on the CSR read/write & trap method to
> > > > > identify the extensions [1].
> > > > >
> > > > > https://github.com/riscv-software-src/opensbi/blob/master/lib/sbi/sbi_hart.c#L404
> > > >
> > > > I guess my question is more, who is supposed to set CBIE, CBCFE bits in the
> > > > ENVCFG CSR. I.e. at it's default settings CMO instructions will cause
> > > > illegal instructions until the level above does allow them.
> > > >
> > > > When the kernel wants to call a cache-invalidate, from my reading menvcfg
> > > > needs to be modified accordingly - which would fall in SBI's court?
> > > >
> > >
> > > I think so. I had the same question for the SSTC extension as well.
> > > This is what I currently do:
> > >
> > > 1. Detect menvcfg first, detect stimecmp
> > > 2. Enable SSTC feature only if both are available
> > > 3. Set the STCE bit in menvcfg if SSTC is available
> > >
> > > Here is the patch
> > > https://github.com/atishp04/opensbi/commit/e6b185821e8302bffdceb4633b413252e0de4889
> >
> > Hmm, the CBO fields are defined as WARL (write any, read legal),
> > so I guess some sort of trap won't work here.
> >
>
> Correct. Traps for extensions that introduce new CSRs.
> I was suggesting setting the corresponding bits in MENVCFG and reading
> it again to check if it sticks.
>
> > The priv-spec only points to the cmo-spec for these bits and the cmo-spec
> > does not specifiy what the value should be when cmo is not present.
> >
> >
> > > > > In the future, zicbom can be detected in the same manner. However,
> > > > > svpbmt is a bit tricky as it doesn't
> > > > > define any new CSR. Do you think OpenSBI needs to detect svpbmt for any reason ?
> > > >
> > > > There is the PBMTE bit in MENVCFG, which I found while looking through the
> > > > zicbom-parts, which is supposed to "control wheter svpbmt is available for
> > > > use". So I guess the question is the same as above :-)
> > > >
> > >
> > > PBMTE bit in MENVCFG says if PBMTE bit is available or not. OpenSBI
> > > needs other way to
> > > detect if PBMTE is available.
> > >
> > > That's why, I think MENVCFG should be set correctly by the hardware
> > > upon reset. What do you think
> > > about that ? I couldn't find anything related to the reset state for menvcfg.
> >
> > me neither. Both the priv-spec as well as the cmobase spec do not
> > specifiy any reset-values it seems.
> >
> I have raised an issue in the ISA spec.
> https://github.com/riscv/riscv-isa-manual/issues/820
>
> > So I guess in the Qemu case, Qemu needs to set that bit when
> > its svpbmt extension is enabled?
> >
>
> We can do that if the priv spec is modified to allow that.
>

As per Greg's response, hardware is not expected to do that.
So we have to dynamically detect the extensions in OpenSBI and write to menvcfg.

I am not sure what needs to be done for CBIE bits as it both flush(01)
or invalidate(11) are valid values

> >
> > Heiko
> >
> >
> > > > > > > ---
> > > > > > >  arch/riscv/include/asm/hwcap.h | 18 ++++++++++++++++++
> > > > > > >  arch/riscv/kernel/cpufeature.c | 27 ++++++++++++++++++++++++---
> > > > > > >  2 files changed, 42 insertions(+), 3 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 e9e3b0693d16..469b9739faf7 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;
> > > > > > > +             uint64_t this_isa = 0;
> > > > > > >
> > > > > > >               if (riscv_of_processor_hartid(node) < 0)
> > > > > > >                       continue;
> > > > > > > @@ -169,12 +169,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)) {    \
> > > > > > > +                                     this_isa |= (1UL << bit);               \
> > > > > > > +                                     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'));
> > > > > > >                       }
> > > > > > > +#undef SET_ISA_EXT_MAP
> > > > > > >               }
> > > > > > >
> > > > > > >               /*
> > > > > > > @@ -187,10 +197,21 @@ void __init riscv_fill_hwcap(void)
> > > > > > >               else
> > > > > > >                       elf_hwcap = this_hwcap;
> > > > > > >
> > > > > > > -             if (riscv_isa[0])
> > > > > > > +             if (riscv_isa[0]) {
> > > > > > > +#if IS_ENABLED(CONFIG_32BIT)
> > > > > > > +                     riscv_isa[0] &= this_isa & 0xFFFFFFFF;
> > > > > > > +                     riscv_isa[1] &= this_isa >> 32;
> > > > > > > +#else
> > > > > > >                       riscv_isa[0] &= this_isa;
> > > > > > > -             else
> > > > > > > +#endif
> > > > > > > +             } else {
> > > > > > > +#if IS_ENABLED(CONFIG_32BIT)
> > > > > > > +                     riscv_isa[0] = this_isa & 0xFFFFFFFF;
> > > > > > > +                     riscv_isa[1] = this_isa >> 32;
> > > > > > > +#else
> > > > > > >                       riscv_isa[0] = this_isa;
> > > > > > > +#endif
> > > > > > > +             }
> > > > > > >       }
> > > > > > >
> > > > > > >       /* We don't support systems with F but without D, so mask those out
> > > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > >
> > > > >
> > > > >
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> >
> >
> >
> >

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

[Heiko Stübner]

Am Dienstag, 15. Februar 2022, 10:12:53 CET schrieb Atish Kumar Patra:
> On Mon, Feb 14, 2022 at 3:22 PM Atish Kumar Patra <atishp@rivosinc.com> wrote:
> >
> > On Mon, Feb 14, 2022 at 2:22 PM Heiko Stübner <heiko@sntech.de> wrote:
> > >
> > > Am Montag, 14. Februar 2022, 21:42:32 CET schrieb Atish Patra:
> > > > On Mon, Feb 14, 2022 at 12:24 PM Heiko Stübner <heiko@sntech.de> wrote:
> > > > >
> > > > > Am Montag, 14. Februar 2022, 21:14:13 CET schrieb Atish Patra:
> > > > > > On Mon, Feb 14, 2022 at 12:06 PM Heiko Stübner <heiko@sntech.de> wrote:
> > > > > > >
> > > > > > > Am Donnerstag, 10. Februar 2022, 22:40:16 CET schrieb 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.
> > > > > > > >
> > > > > > > > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > > > > > >
> > > > > > > Tested-by: Heiko Stuebner <heiko@sntech.de>
> > > > > > >
> > > > > > >
> > > > > > > By the way, does a similar parsing exist for opensbi as well?
> > > > > > > Things like svpbmt as well as zicbom have CSR bits controlling how
> > > > > > > these functions should behave (enabling them, etc), so I guess
> > > > > > > opensbi also needs to parse the extensions from the ISA string?
> > > > > > >
> > > > > > >
> > > > > >
> > > > > > No. Currently, OpenSBI relies on the CSR read/write & trap method to
> > > > > > identify the extensions [1].
> > > > > >
> > > > > > https://github.com/riscv-software-src/opensbi/blob/master/lib/sbi/sbi_hart.c#L404
> > > > >
> > > > > I guess my question is more, who is supposed to set CBIE, CBCFE bits in the
> > > > > ENVCFG CSR. I.e. at it's default settings CMO instructions will cause
> > > > > illegal instructions until the level above does allow them.
> > > > >
> > > > > When the kernel wants to call a cache-invalidate, from my reading menvcfg
> > > > > needs to be modified accordingly - which would fall in SBI's court?
> > > > >
> > > >
> > > > I think so. I had the same question for the SSTC extension as well.
> > > > This is what I currently do:
> > > >
> > > > 1. Detect menvcfg first, detect stimecmp
> > > > 2. Enable SSTC feature only if both are available
> > > > 3. Set the STCE bit in menvcfg if SSTC is available
> > > >
> > > > Here is the patch
> > > > https://github.com/atishp04/opensbi/commit/e6b185821e8302bffdceb4633b413252e0de4889
> > >
> > > Hmm, the CBO fields are defined as WARL (write any, read legal),
> > > so I guess some sort of trap won't work here.
> > >
> >
> > Correct. Traps for extensions that introduce new CSRs.
> > I was suggesting setting the corresponding bits in MENVCFG and reading
> > it again to check if it sticks.
> >
> > > The priv-spec only points to the cmo-spec for these bits and the cmo-spec
> > > does not specifiy what the value should be when cmo is not present.
> > >
> > >
> > > > > > In the future, zicbom can be detected in the same manner. However,
> > > > > > svpbmt is a bit tricky as it doesn't
> > > > > > define any new CSR. Do you think OpenSBI needs to detect svpbmt for any reason ?
> > > > >
> > > > > There is the PBMTE bit in MENVCFG, which I found while looking through the
> > > > > zicbom-parts, which is supposed to "control wheter svpbmt is available for
> > > > > use". So I guess the question is the same as above :-)
> > > > >
> > > >
> > > > PBMTE bit in MENVCFG says if PBMTE bit is available or not. OpenSBI
> > > > needs other way to
> > > > detect if PBMTE is available.
> > > >
> > > > That's why, I think MENVCFG should be set correctly by the hardware
> > > > upon reset. What do you think
> > > > about that ? I couldn't find anything related to the reset state for menvcfg.
> > >
> > > me neither. Both the priv-spec as well as the cmobase spec do not
> > > specifiy any reset-values it seems.
> > >
> > I have raised an issue in the ISA spec.
> > https://github.com/riscv/riscv-isa-manual/issues/820
> >
> > > So I guess in the Qemu case, Qemu needs to set that bit when
> > > its svpbmt extension is enabled?
> > >
> >
> > We can do that if the priv spec is modified to allow that.
> >
> 
> As per Greg's response, hardware is not expected to do that.
> So we have to dynamically detect the extensions in OpenSBI and write to menvcfg.
> 
> I am not sure what needs to be done for CBIE bits as it both flush(01)
> or invalidate(11) are valid values

From looking at the security remark in the cmo-spec, I guess flush would be
the appropriate thing to do?

"Until a modified cache block has updated memory, a CBO.INVAL instruction may expose stale data values
in memory if the CSRs are programmed to perform an invalidate operation. This behavior may result in a
security hole if lower privileged level software performs an invalidate operation and accesses sensitive
information in memory."

But also do we actually _want_ to enable cmo always ... Greg was talking
about backwards compatiblity in his response as well.

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

[Heiko Stübner]

Am Dienstag, 15. Februar 2022, 10:48:16 CET schrieb Heiko Stübner:
> Am Dienstag, 15. Februar 2022, 10:12:53 CET schrieb Atish Kumar Patra:
> > On Mon, Feb 14, 2022 at 3:22 PM Atish Kumar Patra <atishp@rivosinc.com> wrote:
> > >
> > > On Mon, Feb 14, 2022 at 2:22 PM Heiko Stübner <heiko@sntech.de> wrote:
> > > >
> > > > Am Montag, 14. Februar 2022, 21:42:32 CET schrieb Atish Patra:
> > > > > On Mon, Feb 14, 2022 at 12:24 PM Heiko Stübner <heiko@sntech.de> wrote:
> > > > > >
> > > > > > Am Montag, 14. Februar 2022, 21:14:13 CET schrieb Atish Patra:
> > > > > > > On Mon, Feb 14, 2022 at 12:06 PM Heiko Stübner <heiko@sntech.de> wrote:
> > > > > > > >
> > > > > > > > Am Donnerstag, 10. Februar 2022, 22:40:16 CET schrieb 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.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > > > > > > >
> > > > > > > > Tested-by: Heiko Stuebner <heiko@sntech.de>
> > > > > > > >
> > > > > > > >
> > > > > > > > By the way, does a similar parsing exist for opensbi as well?
> > > > > > > > Things like svpbmt as well as zicbom have CSR bits controlling how
> > > > > > > > these functions should behave (enabling them, etc), so I guess
> > > > > > > > opensbi also needs to parse the extensions from the ISA string?
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > > > No. Currently, OpenSBI relies on the CSR read/write & trap method to
> > > > > > > identify the extensions [1].
> > > > > > >
> > > > > > > https://github.com/riscv-software-src/opensbi/blob/master/lib/sbi/sbi_hart.c#L404
> > > > > >
> > > > > > I guess my question is more, who is supposed to set CBIE, CBCFE bits in the
> > > > > > ENVCFG CSR. I.e. at it's default settings CMO instructions will cause
> > > > > > illegal instructions until the level above does allow them.
> > > > > >
> > > > > > When the kernel wants to call a cache-invalidate, from my reading menvcfg
> > > > > > needs to be modified accordingly - which would fall in SBI's court?
> > > > > >
> > > > >
> > > > > I think so. I had the same question for the SSTC extension as well.
> > > > > This is what I currently do:
> > > > >
> > > > > 1. Detect menvcfg first, detect stimecmp
> > > > > 2. Enable SSTC feature only if both are available
> > > > > 3. Set the STCE bit in menvcfg if SSTC is available
> > > > >
> > > > > Here is the patch
> > > > > https://github.com/atishp04/opensbi/commit/e6b185821e8302bffdceb4633b413252e0de4889
> > > >
> > > > Hmm, the CBO fields are defined as WARL (write any, read legal),
> > > > so I guess some sort of trap won't work here.
> > > >
> > >
> > > Correct. Traps for extensions that introduce new CSRs.
> > > I was suggesting setting the corresponding bits in MENVCFG and reading
> > > it again to check if it sticks.
> > >
> > > > The priv-spec only points to the cmo-spec for these bits and the cmo-spec
> > > > does not specifiy what the value should be when cmo is not present.
> > > >
> > > >
> > > > > > > In the future, zicbom can be detected in the same manner. However,
> > > > > > > svpbmt is a bit tricky as it doesn't
> > > > > > > define any new CSR. Do you think OpenSBI needs to detect svpbmt for any reason ?
> > > > > >
> > > > > > There is the PBMTE bit in MENVCFG, which I found while looking through the
> > > > > > zicbom-parts, which is supposed to "control wheter svpbmt is available for
> > > > > > use". So I guess the question is the same as above :-)
> > > > > >
> > > > >
> > > > > PBMTE bit in MENVCFG says if PBMTE bit is available or not. OpenSBI
> > > > > needs other way to
> > > > > detect if PBMTE is available.
> > > > >
> > > > > That's why, I think MENVCFG should be set correctly by the hardware
> > > > > upon reset. What do you think
> > > > > about that ? I couldn't find anything related to the reset state for menvcfg.
> > > >
> > > > me neither. Both the priv-spec as well as the cmobase spec do not
> > > > specifiy any reset-values it seems.
> > > >
> > > I have raised an issue in the ISA spec.
> > > https://github.com/riscv/riscv-isa-manual/issues/820
> > >
> > > > So I guess in the Qemu case, Qemu needs to set that bit when
> > > > its svpbmt extension is enabled?
> > > >
> > >
> > > We can do that if the priv spec is modified to allow that.
> > >
> > 
> > As per Greg's response, hardware is not expected to do that.
> > So we have to dynamically detect the extensions in OpenSBI and write to menvcfg.

Doesn't SBI also get the devicetree and could therefore parse
the ISA string for extensions? Might be less volatile and would
have both Kernel and SBI do the same thing for detection.


> > I am not sure what needs to be done for CBIE bits as it both flush(01)
> > or invalidate(11) are valid values
> 
> From looking at the security remark in the cmo-spec, I guess flush would be
> the appropriate thing to do?
> 
> "Until a modified cache block has updated memory, a CBO.INVAL instruction may expose stale data values
> in memory if the CSRs are programmed to perform an invalidate operation. This behavior may result in a
> security hole if lower privileged level software performs an invalidate operation and accesses sensitive
> information in memory."
> 
> But also do we actually _want_ to enable cmo always ... Greg was talking
> about backwards compatiblity in his response as well.
> 

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

[Atish Kumar Patra]

On Tue, Feb 15, 2022 at 1:50 AM Heiko Stübner <heiko@sntech.de> wrote:
>
> Am Dienstag, 15. Februar 2022, 10:48:16 CET schrieb Heiko Stübner:
> > Am Dienstag, 15. Februar 2022, 10:12:53 CET schrieb Atish Kumar Patra:
> > > On Mon, Feb 14, 2022 at 3:22 PM Atish Kumar Patra <atishp@rivosinc.com> wrote:
> > > >
> > > > On Mon, Feb 14, 2022 at 2:22 PM Heiko Stübner <heiko@sntech.de> wrote:
> > > > >
> > > > > Am Montag, 14. Februar 2022, 21:42:32 CET schrieb Atish Patra:
> > > > > > On Mon, Feb 14, 2022 at 12:24 PM Heiko Stübner <heiko@sntech.de> wrote:
> > > > > > >
> > > > > > > Am Montag, 14. Februar 2022, 21:14:13 CET schrieb Atish Patra:
> > > > > > > > On Mon, Feb 14, 2022 at 12:06 PM Heiko Stübner <heiko@sntech.de> wrote:
> > > > > > > > >
> > > > > > > > > Am Donnerstag, 10. Februar 2022, 22:40:16 CET schrieb 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.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > > > > > > > >
> > > > > > > > > Tested-by: Heiko Stuebner <heiko@sntech.de>
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > By the way, does a similar parsing exist for opensbi as well?
> > > > > > > > > Things like svpbmt as well as zicbom have CSR bits controlling how
> > > > > > > > > these functions should behave (enabling them, etc), so I guess
> > > > > > > > > opensbi also needs to parse the extensions from the ISA string?
> > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > > > No. Currently, OpenSBI relies on the CSR read/write & trap method to
> > > > > > > > identify the extensions [1].
> > > > > > > >
> > > > > > > > https://github.com/riscv-software-src/opensbi/blob/master/lib/sbi/sbi_hart.c#L404
> > > > > > >
> > > > > > > I guess my question is more, who is supposed to set CBIE, CBCFE bits in the
> > > > > > > ENVCFG CSR. I.e. at it's default settings CMO instructions will cause
> > > > > > > illegal instructions until the level above does allow them.
> > > > > > >
> > > > > > > When the kernel wants to call a cache-invalidate, from my reading menvcfg
> > > > > > > needs to be modified accordingly - which would fall in SBI's court?
> > > > > > >
> > > > > >
> > > > > > I think so. I had the same question for the SSTC extension as well.
> > > > > > This is what I currently do:
> > > > > >
> > > > > > 1. Detect menvcfg first, detect stimecmp
> > > > > > 2. Enable SSTC feature only if both are available
> > > > > > 3. Set the STCE bit in menvcfg if SSTC is available
> > > > > >
> > > > > > Here is the patch
> > > > > > https://github.com/atishp04/opensbi/commit/e6b185821e8302bffdceb4633b413252e0de4889
> > > > >
> > > > > Hmm, the CBO fields are defined as WARL (write any, read legal),
> > > > > so I guess some sort of trap won't work here.
> > > > >
> > > >
> > > > Correct. Traps for extensions that introduce new CSRs.
> > > > I was suggesting setting the corresponding bits in MENVCFG and reading
> > > > it again to check if it sticks.
> > > >
> > > > > The priv-spec only points to the cmo-spec for these bits and the cmo-spec
> > > > > does not specifiy what the value should be when cmo is not present.
> > > > >
> > > > >
> > > > > > > > In the future, zicbom can be detected in the same manner. However,
> > > > > > > > svpbmt is a bit tricky as it doesn't
> > > > > > > > define any new CSR. Do you think OpenSBI needs to detect svpbmt for any reason ?
> > > > > > >
> > > > > > > There is the PBMTE bit in MENVCFG, which I found while looking through the
> > > > > > > zicbom-parts, which is supposed to "control wheter svpbmt is available for
> > > > > > > use". So I guess the question is the same as above :-)
> > > > > > >
> > > > > >
> > > > > > PBMTE bit in MENVCFG says if PBMTE bit is available or not. OpenSBI
> > > > > > needs other way to
> > > > > > detect if PBMTE is available.
> > > > > >
> > > > > > That's why, I think MENVCFG should be set correctly by the hardware
> > > > > > upon reset. What do you think
> > > > > > about that ? I couldn't find anything related to the reset state for menvcfg.
> > > > >
> > > > > me neither. Both the priv-spec as well as the cmobase spec do not
> > > > > specifiy any reset-values it seems.
> > > > >
> > > > I have raised an issue in the ISA spec.
> > > > https://github.com/riscv/riscv-isa-manual/issues/820
> > > >
> > > > > So I guess in the Qemu case, Qemu needs to set that bit when
> > > > > its svpbmt extension is enabled?
> > > > >
> > > >
> > > > We can do that if the priv spec is modified to allow that.
> > > >
> > >
> > > As per Greg's response, hardware is not expected to do that.
> > > So we have to dynamically detect the extensions in OpenSBI and write to menvcfg.
>
> Doesn't SBI also get the devicetree and could therefore parse
> the ISA string for extensions? Might be less volatile and would
> have both Kernel and SBI do the same thing for detection.
>

It does. But the later stage boot loader can replace the DT as well. A
incorrect DT passed to OpenSBI may
lead to crash the system but that's probably okay because the
"riscv,isa" properties shouldn't be incorrect at the first place.
We can set the hart features based on DT parsing as well (similar to
Linux kernel)

I suggested the earlier method because that infra already exists in
OpenSBI and will
continue to exist because not all hart features are ISA extensions.

So we can leverage that or add dt parsing as well. I am fine with
either approach.

>
> > > I am not sure what needs to be done for CBIE bits as it both flush(01)
> > > or invalidate(11) are valid values
> >
> > From looking at the security remark in the cmo-spec, I guess flush would be
> > the appropriate thing to do?
> >

Looks like that. But how does a supervisor/usermode use the invalidate
functionality ?

> > "Until a modified cache block has updated memory, a CBO.INVAL instruction may expose stale data values
> > in memory if the CSRs are programmed to perform an invalidate operation. This behavior may result in a
> > security hole if lower privileged level software performs an invalidate operation and accesses sensitive
> > information in memory."
> >
> > But also do we actually _want_ to enable cmo always ... Greg was talking
> > about backwards compatiblity in his response as well.
> >



>
>
>
>