[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 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


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

[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 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


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

[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 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


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

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

[Tsukasa OI]

Hi Atish,

Your patches for new RISC-V extension framework seems good.  I was busy
working on GNU Binutils and (though I'm not completely convinced) it's
okay with "isa-ext" approach of Anup.

And thanks for using my parser for new ISA extension framework.

Still, I will raise an issue with your "improvement" and propose new
version of PATCH 1-3/6 (as replies to this e-mail).

(I also corrected indent on PATCH 3/6 though this is not functional).

Thanks,
Tsukasa

On 2022/02/11 6:40, Atish Patra 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.
> 
> 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;

To address an issue reported in
<https://lore.kernel.org/all/202202111616.z7nZoKYj-lkp@intel.com/T/>,
we'd be better to initialize ext_long with 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;

I think you added this for QEMU bug workaround (I will describe leter)
but this will raise an issue.

QEMU generates invalid ISA string on "riscv,isa" such like:
"rv64imafdcsuh".  QEMU handles 'S' as a single-letter extension and can
easily confuse my parser (handles this as 'Suh' non-existent extension).

Atish tried to address this issue by handling \2 of "([^_])(S)"
as a single-letter 'S' extension.

It's okay for QEMU-generated ones and full ISA strings like
"rv64imafdc_svinval_svnapot".  However, it causes problems on
"rv64imafdcsvinval_svnapot" (no underscore before "svinval").  This is
still a valid ISA string (with existing extensions) but confuses
Atish's parser.

The best solution is to fix **ONLY** QEMU and not make this exception.

If we need QEMU workaround now, how about checking \2 of "([^_])(SU)"
as single-letter 'S' and 'U' extensions?

Here's the background for my new workaround:

1.  Linux requires U-mode.
2.  QEMU always generates invalid ISA string containing "SU"
    for RISC-V with MSU modes.
3.  There's no standard extensions starting with 'Su'
    (there's even no discussions/drafts about them).

Following is related dumps from QEMU and Spike with my test Busybox image
(with some notes):

QEMU + RV64GC (with hypervisor enabled) w/o Atish's workaround
=-===========================================================================-=
processor	: 0
hart		: 0
isa		: rv64imafdcsuh
isa-ext		: 
mmu		: sv48

riscv,isa (DTS) : rv64imafdcsuh   ("suh" is handled as an extension)
dmesg:
[    0.000000] riscv: ISA extensions acdfim  (hypervisor not detected!)
[    0.000000] riscv: ELF capabilities acdfim
=-===========================================================================-=

QEMU + RV64GC (with hypervisor enabled) w/ Atish's workaround
=-===========================================================================-=
processor	: 0
hart		: 0
isa		: rv64imafdcsuh
isa-ext		: 
mmu		: sv48

riscv,isa (DTS) : rv64imafdcsuh   ("suh" is handled as 'S', 'U' and 'H')
dmesg:
[    0.000000] riscv: ISA extensions acdfhimsu (extra 'S' and 'U' are harmless)
[    0.000000] riscv: ELF capabilities acdfim
=-===========================================================================-=

QEMU + RV64GC (with hypervisor enabled) w/ Tsukasa's new workaround
=-===========================================================================-=
processor	: 0
hart		: 0
isa		: rv64imafdcsuh
isa-ext		: 
mmu		: sv48

riscv,isa (DTS) : rv64imafdcsuh   ("suh" is handled as 'S', 'U' and 'H')
dmesg:
[    0.000000] riscv: ISA extensions acdfhimsu (same as Atish's workaround)
[    0.000000] riscv: ELF capabilities acdfim
=-===========================================================================-=

Spike + rv64imafdc_svinval_svnapot w/ Atish's workaround
=-===========================================================================-=
processor	: 0
hart		: 0
isa		: rv64imafdc
isa-ext		: svinval svnapot 
mmu		: sv48

riscv,isa (DTS) : rv64imafdc_svinval_svnapot
dmesg:
[    0.000000] Found ISA extension svinval
[    0.000000] Found ISA extension svnapot
[    0.000000] riscv: ISA extensions acdfim
[    0.000000] riscv: ELF capabilities acdfim
=-===========================================================================-=

Spike + rv64imafdcsvinval_svnapot w/ Atish's workaround
=-===========================================================================-=
processor	: 0
hart		: 0
isa		: rv64imafdcsvinval
isa-ext		: svnapot 
mmu		: sv48

riscv,isa (DTS) : rv64imafdcsvinval_svnapot
dmesg:
[    0.000000] Found ISA extension svnapot        (no 'Svinval' is found!)
[    0.000000] riscv: ISA extensions acdfilmnsv   ('V' et al. should not be here!)
[    0.000000] riscv: ELF capabilities acdfim
=-===========================================================================-=

Spike + rv64imafdcsvinval_svnapot w/ Tsukasa's new workaround
=-===========================================================================-=
processor	: 0
hart		: 0
isa		: rv64imafdcsvinval
isa-ext		: svinval svnapot 
mmu		: sv48

riscv,isa (DTS) : rv64imafdcsvinval_svnapot
dmesg:
[    0.000000] Found ISA extension svinval
[    0.000000] Found ISA extension svnapot
[    0.000000] riscv: ISA extensions acdfim
[    0.000000] riscv: ELF capabilities acdfim
=-===========================================================================-=


> +				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;

A line above is removed on my new patches.

> +				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'));
>  		}
>  
>  		/*

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

[PATCH v3 1/3] RISC-V: Correctly print supported extensions

[Tsukasa OI]

This commit replaces BITS_PER_LONG with number of alphabet letters.

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

This commit disables printing high bits for now.

Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com>
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);

base-commit: 7aed1489bdf879da403e8ee9d6d77b0be26bb84d
-- 
2.32.0


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

Re: [PATCH v3 1/3] RISC-V: Correctly print supported extensions

[Heiko Stübner]

Am Samstag, 12. Februar 2022, 07:29:59 CET schrieb Tsukasa OI:
> This commit replaces BITS_PER_LONG with number of alphabet letters.
> 
> Current ISA pretty-printing code expects extension 'a' (bit 0) through
> 'z' (bit 25).  Although bit 26 and higher is not currently used (thus never
> cause an issue in practice), it will be an annoying problem if we start to
> use those in the future.
> 
> This commit disables printing high bits for now.
> 
> Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>

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

> ---
>  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);
> 
> base-commit: 7aed1489bdf879da403e8ee9d6d77b0be26bb84d
> 





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

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

[Tsukasa OI]

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

1. Multi-letter extensions
2. Version numbers

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>
[Fixed workaround for QEMU]
Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com>
---
 arch/riscv/kernel/cpufeature.c | 66 ++++++++++++++++++++++++++++------
 1 file changed, 55 insertions(+), 11 deletions(-)

diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index dd3d57eb4eea..9d5448542226 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,66 @@ 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':
+			case 'x':
+			case 'z':
+				/**
+				 * Workaround for invalid single-letter 's' (QEMU).
+				 * It works until multi-letter extension starting
+				 * with "Su" appears.
+				 */
+				if (*ext == 's' && ext[-1] != '_' && ext[1] == 'u')
+					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:
+				if (unlikely(!islower(*ext))) {
+					ext_err = true;
+					break;
+				}
+				/* Find next extension */
+				if (!isdigit(*isa))
+					break;
+				while (isdigit(*++isa))
+					;
+				if (*isa != 'p')
+					break;
+				if (!isdigit(*++isa)) {
+					--isa;
+					break;
+				}
+				while (isdigit(*++isa))
+					;
+				break;
+			}
+			if (*isa != '_')
+				--isa;
 			/*
-			 * TODO: X, Y and Z extension parsing for Host ISA
-			 * bitmap will be added in-future.
+			 * TODO: Full version-aware handling including
+			 * multi-letter extensions will be added in-future.
 			 */
-			if ('a' <= isa[i] && isa[i] < 'x')
-				this_isa |= (1UL << (isa[i] - 'a'));
+			if (ext_err || ext_long)
+				continue;
+			this_hwcap |= isa2hwcap[(unsigned char)(*ext)];
+			this_isa |= (1UL << (*ext - 'a'));
 		}
 
 		/*
-- 
2.32.0


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

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

[Heiko Stübner]

Am Samstag, 12. Februar 2022, 07:30:00 CET schrieb Tsukasa OI:
> 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>
> [Fixed workaround for QEMU]
> Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com>

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



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

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

[Tsukasa OI]

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 | 38 ++++++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 9 deletions(-)

diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 9d5448542226..cd9eb34f8d11 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -119,9 +119,28 @@ void __init riscv_fill_hwcap(void)
 				ext_long = true;
 				/* Multi-letter extension must be delimited */
 				for (; *isa && *isa != '_'; ++isa)
-					if (!islower(*isa) && !isdigit(*isa))
+					if (unlikely(!islower(*isa)
+						     && !isdigit(*isa)))
 						ext_err = true;
-				/* ... but must be ignored. */
+				/* 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))) {
@@ -131,6 +150,7 @@ void __init riscv_fill_hwcap(void)
 				/* Find next extension */
 				if (!isdigit(*isa))
 					break;
+				/* Skip the minor version */
 				while (isdigit(*++isa))
 					;
 				if (*isa != 'p')
@@ -139,20 +159,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.32.0


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

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

[Heiko Stübner]

Am Samstag, 12. Februar 2022, 07:30:01 CET schrieb Tsukasa OI:
> 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>

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



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

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

[Atish Patra]

On Fri, Feb 11, 2022 at 10:25 PM Tsukasa OI
<research_trasio@irq.a4lg.com> wrote:
>
> Hi Atish,
>
> Your patches for new RISC-V extension framework seems good.  I was busy
> working on GNU Binutils and (though I'm not completely convinced) it's
> okay with "isa-ext" approach of Anup.
>
> And thanks for using my parser for new ISA extension framework.
>
> Still, I will raise an issue with your "improvement" and propose new
> version of PATCH 1-3/6 (as replies to this e-mail).
>
> (I also corrected indent on PATCH 3/6 though this is not functional).
>
> Thanks,
> Tsukasa
>
> On 2022/02/11 6:40, Atish Patra 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.
> >
> > 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;
>
> To address an issue reported in
> <https://lore.kernel.org/all/202202111616.z7nZoKYj-lkp@intel.com/T/>,
> we'd be better to initialize ext_long with 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;
>
> I think you added this for QEMU bug workaround (I will describe leter)
> but this will raise an issue.
>
> QEMU generates invalid ISA string on "riscv,isa" such like:
> "rv64imafdcsuh".  QEMU handles 'S' as a single-letter extension and can
> easily confuse my parser (handles this as 'Suh' non-existent extension).
>
> Atish tried to address this issue by handling \2 of "([^_])(S)"
> as a single-letter 'S' extension.
>
> It's okay for QEMU-generated ones and full ISA strings like
> "rv64imafdc_svinval_svnapot".  However, it causes problems on
> "rv64imafdcsvinval_svnapot" (no underscore before "svinval").  This is
> still a valid ISA string (with existing extensions) but confuses
> Atish's parser.
>
> The best solution is to fix **ONLY** QEMU and not make this exception.
>

"S" is a bit ambiguous as misa defines the "S" bit as Supervisor
implemented but it's not a valid ISA extension.

Probably, we should fix the isa-string generation in Qemu. But we
should have a workaround for backward compatibility
as well so that users can run hypervisors on older qemu.

> If we need QEMU workaround now, how about checking \2 of "([^_])(SU)"
> as single-letter 'S' and 'U' extensions?
>

Sounds good to me. Can I fold in your changes and send a v3 so that it
is easier to merge ?

> Here's the background for my new workaround:
>
> 1.  Linux requires U-mode.
> 2.  QEMU always generates invalid ISA string containing "SU"
>     for RISC-V with MSU modes.
> 3.  There's no standard extensions starting with 'Su'
>     (there's even no discussions/drafts about them).
>
> Following is related dumps from QEMU and Spike with my test Busybox image
> (with some notes):
>
> QEMU + RV64GC (with hypervisor enabled) w/o Atish's workaround
> =-===========================================================================-=
> processor       : 0
> hart            : 0
> isa             : rv64imafdcsuh
> isa-ext         :
> mmu             : sv48
>
> riscv,isa (DTS) : rv64imafdcsuh   ("suh" is handled as an extension)
> dmesg:
> [    0.000000] riscv: ISA extensions acdfim  (hypervisor not detected!)
> [    0.000000] riscv: ELF capabilities acdfim
> =-===========================================================================-=
>
> QEMU + RV64GC (with hypervisor enabled) w/ Atish's workaround
> =-===========================================================================-=
> processor       : 0
> hart            : 0
> isa             : rv64imafdcsuh
> isa-ext         :
> mmu             : sv48
>
> riscv,isa (DTS) : rv64imafdcsuh   ("suh" is handled as 'S', 'U' and 'H')
> dmesg:
> [    0.000000] riscv: ISA extensions acdfhimsu (extra 'S' and 'U' are harmless)
> [    0.000000] riscv: ELF capabilities acdfim
> =-===========================================================================-=
>
> QEMU + RV64GC (with hypervisor enabled) w/ Tsukasa's new workaround
> =-===========================================================================-=
> processor       : 0
> hart            : 0
> isa             : rv64imafdcsuh
> isa-ext         :
> mmu             : sv48
>
> riscv,isa (DTS) : rv64imafdcsuh   ("suh" is handled as 'S', 'U' and 'H')
> dmesg:
> [    0.000000] riscv: ISA extensions acdfhimsu (same as Atish's workaround)
> [    0.000000] riscv: ELF capabilities acdfim
> =-===========================================================================-=
>
> Spike + rv64imafdc_svinval_svnapot w/ Atish's workaround
> =-===========================================================================-=
> processor       : 0
> hart            : 0
> isa             : rv64imafdc
> isa-ext         : svinval svnapot
> mmu             : sv48
>
> riscv,isa (DTS) : rv64imafdc_svinval_svnapot
> dmesg:
> [    0.000000] Found ISA extension svinval
> [    0.000000] Found ISA extension svnapot
> [    0.000000] riscv: ISA extensions acdfim
> [    0.000000] riscv: ELF capabilities acdfim
> =-===========================================================================-=
>
> Spike + rv64imafdcsvinval_svnapot w/ Atish's workaround
> =-===========================================================================-=
> processor       : 0
> hart            : 0
> isa             : rv64imafdcsvinval
> isa-ext         : svnapot
> mmu             : sv48
>
> riscv,isa (DTS) : rv64imafdcsvinval_svnapot
> dmesg:
> [    0.000000] Found ISA extension svnapot        (no 'Svinval' is found!)
> [    0.000000] riscv: ISA extensions acdfilmnsv   ('V' et al. should not be here!)
> [    0.000000] riscv: ELF capabilities acdfim
> =-===========================================================================-=
>
> Spike + rv64imafdcsvinval_svnapot w/ Tsukasa's new workaround
> =-===========================================================================-=
> processor       : 0
> hart            : 0
> isa             : rv64imafdcsvinval
> isa-ext         : svinval svnapot
> mmu             : sv48
>
> riscv,isa (DTS) : rv64imafdcsvinval_svnapot
> dmesg:
> [    0.000000] Found ISA extension svinval
> [    0.000000] Found ISA extension svnapot
> [    0.000000] riscv: ISA extensions acdfim
> [    0.000000] riscv: ELF capabilities acdfim
> =-===========================================================================-=
>
>
> > +                             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;
>
> A line above is removed on my new patches.
>
> > +                             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'));
> >               }
> >
> >               /*



-- 
Regards,
Atish

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

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

[Tsukasa OI]

On 2022/02/15 5:07, Atish Patra wrote:
> On Fri, Feb 11, 2022 at 10:25 PM Tsukasa OI
> <research_trasio@irq.a4lg.com> wrote:
>>
>> Hi Atish,
>>
>> Your patches for new RISC-V extension framework seems good.  I was busy
>> working on GNU Binutils and (though I'm not completely convinced) it's
>> okay with "isa-ext" approach of Anup.
>>
>> And thanks for using my parser for new ISA extension framework.
>>
>> Still, I will raise an issue with your "improvement" and propose new
>> version of PATCH 1-3/6 (as replies to this e-mail).
>>
>> (I also corrected indent on PATCH 3/6 though this is not functional).
>>
>> Thanks,
>> Tsukasa
>>
>> On 2022/02/11 6:40, Atish Patra 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.
>>>
>>> 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;
>>
>> To address an issue reported in
>> <https://lore.kernel.org/all/202202111616.z7nZoKYj-lkp@intel.com/T/>,
>> we'd be better to initialize ext_long with 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;
>>
>> I think you added this for QEMU bug workaround (I will describe leter)
>> but this will raise an issue.
>>
>> QEMU generates invalid ISA string on "riscv,isa" such like:
>> "rv64imafdcsuh".  QEMU handles 'S' as a single-letter extension and can
>> easily confuse my parser (handles this as 'Suh' non-existent extension).
>>
>> Atish tried to address this issue by handling \2 of "([^_])(S)"
>> as a single-letter 'S' extension.
>>
>> It's okay for QEMU-generated ones and full ISA strings like
>> "rv64imafdc_svinval_svnapot".  However, it causes problems on
>> "rv64imafdcsvinval_svnapot" (no underscore before "svinval").  This is
>> still a valid ISA string (with existing extensions) but confuses
>> Atish's parser.
>>
>> The best solution is to fix **ONLY** QEMU and not make this exception.
>>
> 
> "S" is a bit ambiguous as misa defines the "S" bit as Supervisor
> implemented but it's not a valid ISA extension.
> 
> Probably, we should fix the isa-string generation in Qemu. But we
> should have a workaround for backward compatibility
> as well so that users can run hypervisors on older qemu.

Okay.

I sent a patch to qemu-riscv and (before sending the patch), I
surprised by the fact that I'm the third person to try to fix this bug.

<https://lists.nongnu.org/archive/html/qemu-riscv/2019-08/msg00165.html>
<https://lists.nongnu.org/archive/html/qemu-riscv/2021-04/msg00248.html>
<https://lists.nongnu.org/archive/html/qemu-riscv/2022-02/msg00098.html>


> 
>> If we need QEMU workaround now, how about checking \2 of "([^_])(SU)"
>> as single-letter 'S' and 'U' extensions?
>>
> 
> Sounds good to me. Can I fold in your changes and send a v3 so that it
> is easier to merge ?

Of course!  That's exactly what I meant.


> 
>> Here's the background for my new workaround:
>>
>> 1.  Linux requires U-mode.
>> 2.  QEMU always generates invalid ISA string containing "SU"
>>     for RISC-V with MSU modes.
>> 3.  There's no standard extensions starting with 'Su'
>>     (there's even no discussions/drafts about them).
>>
>> Following is related dumps from QEMU and Spike with my test Busybox image
>> (with some notes):
>>
>> QEMU + RV64GC (with hypervisor enabled) w/o Atish's workaround
>> =-===========================================================================-=
>> processor       : 0
>> hart            : 0
>> isa             : rv64imafdcsuh
>> isa-ext         :
>> mmu             : sv48
>>
>> riscv,isa (DTS) : rv64imafdcsuh   ("suh" is handled as an extension)
>> dmesg:
>> [    0.000000] riscv: ISA extensions acdfim  (hypervisor not detected!)
>> [    0.000000] riscv: ELF capabilities acdfim
>> =-===========================================================================-=
>>
>> QEMU + RV64GC (with hypervisor enabled) w/ Atish's workaround
>> =-===========================================================================-=
>> processor       : 0
>> hart            : 0
>> isa             : rv64imafdcsuh
>> isa-ext         :
>> mmu             : sv48
>>
>> riscv,isa (DTS) : rv64imafdcsuh   ("suh" is handled as 'S', 'U' and 'H')
>> dmesg:
>> [    0.000000] riscv: ISA extensions acdfhimsu (extra 'S' and 'U' are harmless)
>> [    0.000000] riscv: ELF capabilities acdfim
>> =-===========================================================================-=
>>
>> QEMU + RV64GC (with hypervisor enabled) w/ Tsukasa's new workaround
>> =-===========================================================================-=
>> processor       : 0
>> hart            : 0
>> isa             : rv64imafdcsuh
>> isa-ext         :
>> mmu             : sv48
>>
>> riscv,isa (DTS) : rv64imafdcsuh   ("suh" is handled as 'S', 'U' and 'H')
>> dmesg:
>> [    0.000000] riscv: ISA extensions acdfhimsu (same as Atish's workaround)
>> [    0.000000] riscv: ELF capabilities acdfim
>> =-===========================================================================-=
>>
>> Spike + rv64imafdc_svinval_svnapot w/ Atish's workaround
>> =-===========================================================================-=
>> processor       : 0
>> hart            : 0
>> isa             : rv64imafdc
>> isa-ext         : svinval svnapot
>> mmu             : sv48
>>
>> riscv,isa (DTS) : rv64imafdc_svinval_svnapot
>> dmesg:
>> [    0.000000] Found ISA extension svinval
>> [    0.000000] Found ISA extension svnapot
>> [    0.000000] riscv: ISA extensions acdfim
>> [    0.000000] riscv: ELF capabilities acdfim
>> =-===========================================================================-=
>>
>> Spike + rv64imafdcsvinval_svnapot w/ Atish's workaround
>> =-===========================================================================-=
>> processor       : 0
>> hart            : 0
>> isa             : rv64imafdcsvinval
>> isa-ext         : svnapot
>> mmu             : sv48
>>
>> riscv,isa (DTS) : rv64imafdcsvinval_svnapot
>> dmesg:
>> [    0.000000] Found ISA extension svnapot        (no 'Svinval' is found!)
>> [    0.000000] riscv: ISA extensions acdfilmnsv   ('V' et al. should not be here!)
>> [    0.000000] riscv: ELF capabilities acdfim
>> =-===========================================================================-=
>>
>> Spike + rv64imafdcsvinval_svnapot w/ Tsukasa's new workaround
>> =-===========================================================================-=
>> processor       : 0
>> hart            : 0
>> isa             : rv64imafdcsvinval
>> isa-ext         : svinval svnapot
>> mmu             : sv48
>>
>> riscv,isa (DTS) : rv64imafdcsvinval_svnapot
>> dmesg:
>> [    0.000000] Found ISA extension svinval
>> [    0.000000] Found ISA extension svnapot
>> [    0.000000] riscv: ISA extensions acdfim
>> [    0.000000] riscv: ELF capabilities acdfim
>> =-===========================================================================-=
>>
>>
>>> +                             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;
>>
>> A line above is removed on my new patches.
>>
>>> +                             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'));
>>>               }
>>>
>>>               /*
> 
> 
> 

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

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

[Atish Patra]

On Mon, Feb 14, 2022 at 7:27 PM Tsukasa OI <research_trasio@irq.a4lg.com> wrote:
>
> On 2022/02/15 5:07, Atish Patra wrote:
> > On Fri, Feb 11, 2022 at 10:25 PM Tsukasa OI
> > <research_trasio@irq.a4lg.com> wrote:
> >>
> >> Hi Atish,
> >>
> >> Your patches for new RISC-V extension framework seems good.  I was busy
> >> working on GNU Binutils and (though I'm not completely convinced) it's
> >> okay with "isa-ext" approach of Anup.
> >>
> >> And thanks for using my parser for new ISA extension framework.
> >>
> >> Still, I will raise an issue with your "improvement" and propose new
> >> version of PATCH 1-3/6 (as replies to this e-mail).
> >>
> >> (I also corrected indent on PATCH 3/6 though this is not functional).
> >>
> >> Thanks,
> >> Tsukasa
> >>
> >> On 2022/02/11 6:40, Atish Patra 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.
> >>>
> >>> 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;
> >>
> >> To address an issue reported in
> >> <https://lore.kernel.org/all/202202111616.z7nZoKYj-lkp@intel.com/T/>,
> >> we'd be better to initialize ext_long with 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;
> >>
> >> I think you added this for QEMU bug workaround (I will describe leter)
> >> but this will raise an issue.
> >>
> >> QEMU generates invalid ISA string on "riscv,isa" such like:
> >> "rv64imafdcsuh".  QEMU handles 'S' as a single-letter extension and can
> >> easily confuse my parser (handles this as 'Suh' non-existent extension).
> >>
> >> Atish tried to address this issue by handling \2 of "([^_])(S)"
> >> as a single-letter 'S' extension.
> >>
> >> It's okay for QEMU-generated ones and full ISA strings like
> >> "rv64imafdc_svinval_svnapot".  However, it causes problems on
> >> "rv64imafdcsvinval_svnapot" (no underscore before "svinval").  This is
> >> still a valid ISA string (with existing extensions) but confuses
> >> Atish's parser.
> >>
> >> The best solution is to fix **ONLY** QEMU and not make this exception.
> >>
> >
> > "S" is a bit ambiguous as misa defines the "S" bit as Supervisor
> > implemented but it's not a valid ISA extension.
> >
> > Probably, we should fix the isa-string generation in Qemu. But we
> > should have a workaround for backward compatibility
> > as well so that users can run hypervisors on older qemu.
>
> Okay.
>
> I sent a patch to qemu-riscv and (before sending the patch), I
> surprised by the fact that I'm the third person to try to fix this bug.
>
> <https://lists.nongnu.org/archive/html/qemu-riscv/2019-08/msg00165.html>
> <https://lists.nongnu.org/archive/html/qemu-riscv/2021-04/msg00248.html>
> <https://lists.nongnu.org/archive/html/qemu-riscv/2022-02/msg00098.html>
>

Wow. At least I did not become the fourth person ;)
I was about to send the patch to qemu mailing list when I saw this email.

A side note, I usually send qemu patches to both qemu-riscv & qemu-devel
for a wider review audience. I noticed that you sent the patch only to
qemu-riscv.

>
> >
> >> If we need QEMU workaround now, how about checking \2 of "([^_])(SU)"
> >> as single-letter 'S' and 'U' extensions?
> >>
> >
> > Sounds good to me. Can I fold in your changes and send a v3 so that it
> > is easier to merge ?
>
> Of course!  That's exactly what I meant.
>
>
> >
> >> Here's the background for my new workaround:
> >>
> >> 1.  Linux requires U-mode.
> >> 2.  QEMU always generates invalid ISA string containing "SU"
> >>     for RISC-V with MSU modes.
> >> 3.  There's no standard extensions starting with 'Su'
> >>     (there's even no discussions/drafts about them).
> >>
> >> Following is related dumps from QEMU and Spike with my test Busybox image
> >> (with some notes):
> >>
> >> QEMU + RV64GC (with hypervisor enabled) w/o Atish's workaround
> >> =-===========================================================================-=
> >> processor       : 0
> >> hart            : 0
> >> isa             : rv64imafdcsuh
> >> isa-ext         :
> >> mmu             : sv48
> >>
> >> riscv,isa (DTS) : rv64imafdcsuh   ("suh" is handled as an extension)
> >> dmesg:
> >> [    0.000000] riscv: ISA extensions acdfim  (hypervisor not detected!)
> >> [    0.000000] riscv: ELF capabilities acdfim
> >> =-===========================================================================-=
> >>
> >> QEMU + RV64GC (with hypervisor enabled) w/ Atish's workaround
> >> =-===========================================================================-=
> >> processor       : 0
> >> hart            : 0
> >> isa             : rv64imafdcsuh
> >> isa-ext         :
> >> mmu             : sv48
> >>
> >> riscv,isa (DTS) : rv64imafdcsuh   ("suh" is handled as 'S', 'U' and 'H')
> >> dmesg:
> >> [    0.000000] riscv: ISA extensions acdfhimsu (extra 'S' and 'U' are harmless)
> >> [    0.000000] riscv: ELF capabilities acdfim
> >> =-===========================================================================-=
> >>
> >> QEMU + RV64GC (with hypervisor enabled) w/ Tsukasa's new workaround
> >> =-===========================================================================-=
> >> processor       : 0
> >> hart            : 0
> >> isa             : rv64imafdcsuh
> >> isa-ext         :
> >> mmu             : sv48
> >>
> >> riscv,isa (DTS) : rv64imafdcsuh   ("suh" is handled as 'S', 'U' and 'H')
> >> dmesg:
> >> [    0.000000] riscv: ISA extensions acdfhimsu (same as Atish's workaround)
> >> [    0.000000] riscv: ELF capabilities acdfim
> >> =-===========================================================================-=
> >>
> >> Spike + rv64imafdc_svinval_svnapot w/ Atish's workaround
> >> =-===========================================================================-=
> >> processor       : 0
> >> hart            : 0
> >> isa             : rv64imafdc
> >> isa-ext         : svinval svnapot
> >> mmu             : sv48
> >>
> >> riscv,isa (DTS) : rv64imafdc_svinval_svnapot
> >> dmesg:
> >> [    0.000000] Found ISA extension svinval
> >> [    0.000000] Found ISA extension svnapot
> >> [    0.000000] riscv: ISA extensions acdfim
> >> [    0.000000] riscv: ELF capabilities acdfim
> >> =-===========================================================================-=
> >>
> >> Spike + rv64imafdcsvinval_svnapot w/ Atish's workaround
> >> =-===========================================================================-=
> >> processor       : 0
> >> hart            : 0
> >> isa             : rv64imafdcsvinval
> >> isa-ext         : svnapot
> >> mmu             : sv48
> >>
> >> riscv,isa (DTS) : rv64imafdcsvinval_svnapot
> >> dmesg:
> >> [    0.000000] Found ISA extension svnapot        (no 'Svinval' is found!)
> >> [    0.000000] riscv: ISA extensions acdfilmnsv   ('V' et al. should not be here!)
> >> [    0.000000] riscv: ELF capabilities acdfim
> >> =-===========================================================================-=
> >>
> >> Spike + rv64imafdcsvinval_svnapot w/ Tsukasa's new workaround
> >> =-===========================================================================-=
> >> processor       : 0
> >> hart            : 0
> >> isa             : rv64imafdcsvinval
> >> isa-ext         : svinval svnapot
> >> mmu             : sv48
> >>
> >> riscv,isa (DTS) : rv64imafdcsvinval_svnapot
> >> dmesg:
> >> [    0.000000] Found ISA extension svinval
> >> [    0.000000] Found ISA extension svnapot
> >> [    0.000000] riscv: ISA extensions acdfim
> >> [    0.000000] riscv: ELF capabilities acdfim
> >> =-===========================================================================-=
> >>
> >>
> >>> +                             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;
> >>
> >> A line above is removed on my new patches.
> >>
> >>> +                             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'));
> >>>               }
> >>>
> >>>               /*
> >
> >
> >



-- 
Regards,
Atish

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

[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 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


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

[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 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


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

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
> 





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

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

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

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





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

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

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

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

[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
> > > > >
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> >
> >
> >
> >
> 
> 
> 





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

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

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

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

[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
> > > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > >
> > > > >
> > > > >
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> >
> >
> >
> >

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

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




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

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

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





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

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



>
>
>
>

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



>
>
>
>

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

[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 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


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

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

[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."

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

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

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

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

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

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



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

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>

[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

[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


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

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>



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

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>