[PATCH v1 0/2] Support new RISC-V ISA extension properties

[PATCH v1 0/2] Support new RISC-V ISA extension properties

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

This would have just been a single patch (the second one), but as I
reported a while back there's a problem with extension detection when
the ISA string exceeds 32 characters:
https://lore.kernel.org/u-boot/20240221-daycare-reliably-8ec86f95fe71@spud/
The first patch here fixes what I see as a bit of a misuse of
cpu_get_desc() in supports_extension() as a preparatory patch for adding
the new properties. Or more accurately, new property, as U-Boot barely
makes use of extension detection as-is in s-mode and only one of the two
new properties is even needed.

Maybe it's not a misuse, but I left a comment under the --- line on that
patch about the before/after for the RISC-V cpu's cpu_get_desc() and
maybe my approach here would not be appreciated.

Cheers,
Conor.

CC: Rick Chen <rick@andestech.com>
CC: Leo <ycliang@andestech.com>
CC: Tom Rini <trini@konsulko.com>
CC: Conor Dooley <conor.dooley@microchip.com>
CC: Heinrich Schuchardt <xypron.glpk@gmx.de>
CC: u-boot@lists.denx.de (open list)

Conor Dooley (2):
  riscv: don't read riscv,isa in the riscv cpu's get_desc()
  riscv: support extension probing using riscv,isa-extensions

 arch/riscv/cpu/cpu.c    | 60 ++++++++++++++++++++++++++---------------
 drivers/cpu/riscv_cpu.c |  8 +++---
 2 files changed, 42 insertions(+), 26 deletions(-)

-- 
2.43.0

[PATCH v1 1/2] riscv: don't read riscv, isa in the riscv cpu's get_desc()

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

cpu_get_desc() for the RISC-V CPU currently reads "riscv,isa" to get
the description, but it is no longer a required property and cannot be
assummed to always be present, as the new "riscv,isa-extensions" and
"riscv,isa-base" properties may be present instead.

On RISC-V, cpu_get_desc() has two main uses - firstly providing an
informational name for the CPU for smbios or at boot with
DISPLAY_CPUINFO etc and secondly it forms the basis of ISA extension
detection in supports_extension() as it returns (a portion of) an ISA
string.

cpu_get_desc() returns a string, which aligned with "riscv,isa" but
the new property is a list of strings. Rather than add support for
the list of strings property, which would require creating an isa
string from "riscv,isa-extensions", modify the RISC-V CPU's
implementaion of cpu_get_desc() return the first compatible as the
cpu description instead. This may be fine for the informational cases,
but it would break extension dtection, given supports_extension()
expects cpu_get_desc() to return an ISA string.

Call dev_read_string() directly in supports_extension() to get the
contents of "riscv,isa" so that extension detection remains functional.
As a knock-on affect of this change, extension detection is no longer
broken for long ISA strings. Previously if the ISA string exceeded the
32 element array that supports_extension() passed to cpu_get_desc(),
it would return ENOSPC and no extensions would be detected.
This bug probably had no impact as U-Boot does not currently do anything
meaningful with the results of supports_extension() and most SoCs
supported by U-Boot don't have anywhere near that complex of an ISA
string. The QEMU virt machine's CPUs do however, so extension detection
doesn't work there.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
I'm not really sure if I am happy with this patch - people could
definitely have got use out of the cpu info printout of the ISA string
before this patch - they'd have seen something like
CPU:	rv64imafdc
at boot, but now they will see
CPU:	sifive,u74
If it really is desired, cpu_get_desc() could be made to assemble
an isa string out of "riscv,isa-extensions", but I think it's always
gonna be a bit flawed, since that string can run to almost arbitrary
length now - one I saw for a CPU last week was 320 characters long
and these things are only going to grow.
---
 arch/riscv/cpu/cpu.c    | 12 +++++++-----
 drivers/cpu/riscv_cpu.c |  8 ++++----
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c
index ecfefa1a02..99083e11df 100644
--- a/arch/riscv/cpu/cpu.c
+++ b/arch/riscv/cpu/cpu.c
@@ -39,7 +39,7 @@ static inline bool supports_extension(char ext)
 	return csr_read(CSR_MISA) & (1 << (ext - 'a'));
 #elif CONFIG_CPU
 	struct udevice *dev;
-	char desc[32];
+	const char *isa;
 	int i;
 
 	uclass_find_first_device(UCLASS_CPU, &dev);
@@ -47,12 +47,14 @@ static inline bool supports_extension(char ext)
 		debug("unable to find the RISC-V cpu device\n");
 		return false;
 	}
-	if (!cpu_get_desc(dev, desc, sizeof(desc))) {
+
+	isa = dev_read_string(dev, "riscv,isa");
+	if (isa) {
 		/*
 		 * skip the first 4 characters (rv32|rv64)
 		 */
-		for (i = 4; i < sizeof(desc); i++) {
-			switch (desc[i]) {
+		for (i = 4; i < sizeof(isa); i++) {
+			switch (isa[i]) {
 			case 's':
 			case 'x':
 			case 'z':
@@ -64,7 +66,7 @@ static inline bool supports_extension(char ext)
 				 */
 				return false;
 			default:
-				if (desc[i] == ext)
+				if (isa[i] == ext)
 					return true;
 			}
 		}
diff --git a/drivers/cpu/riscv_cpu.c b/drivers/cpu/riscv_cpu.c
index 5d1026b37d..9b1950efe0 100644
--- a/drivers/cpu/riscv_cpu.c
+++ b/drivers/cpu/riscv_cpu.c
@@ -21,13 +21,13 @@ DECLARE_GLOBAL_DATA_PTR;
 
 static int riscv_cpu_get_desc(const struct udevice *dev, char *buf, int size)
 {
-	const char *isa;
+	const char *cpu;
 
-	isa = dev_read_string(dev, "riscv,isa");
-	if (size < (strlen(isa) + 1))
+	cpu = dev_read_string(dev, "compatible");
+	if (size < (strlen(cpu) + 1))
 		return -ENOSPC;
 
-	strcpy(buf, isa);
+	strcpy(buf, cpu);
 
 	return 0;
 }
-- 
2.43.0

[PATCH v1 2/2] riscv: support extension probing using riscv, isa-extensions

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

A new property has been added, with an extensive rationale at [1], that
can be used in place of "riscv,isa" to indicate what extensions are
supported by a given platform that is a list of strings rather than a
single string. There are some differences between the new property,
"riscv,isa-extensions" and the incumbent "riscv,isa" - chief among them
for the sake of parsing being the list of strings, as opposed to a
string. Another advantage is strictly defined meanings for each string
in a dt-binding, rather than deriving meaning from RVI standards. This
will likely to some divergence over time, but U-Boot's current use of
extension detection is very limited - there are just four callsites of
supports_extension() in mainline U-Boot.

These checks are limited to two checks for FPU support and two checks
for "s" and "u". "s" and "u" are not supported by the new property, but
they were also not permitted in "riscv,isa". These checks are only
meaningful (or run) in M-Mode, in which case supports_extension() does
not parse the devicetree anyway.

Add support for the new property in U-Boot, prioritising it, before
falling back to the, now deprecated, "riscv,isa" property if it is not
present.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
I moved the kernel devicetrees to use the new properties, I'd do the
same here, but I'd rather just move things to use dt-rebasing instead,
where possible.
---
 arch/riscv/cpu/cpu.c | 56 +++++++++++++++++++++++++++-----------------
 1 file changed, 35 insertions(+), 21 deletions(-)

diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c
index 99083e11df..affe70081b 100644
--- a/arch/riscv/cpu/cpu.c
+++ b/arch/riscv/cpu/cpu.c
@@ -38,9 +38,10 @@ static inline bool supports_extension(char ext)
 #if CONFIG_IS_ENABLED(RISCV_MMODE)
 	return csr_read(CSR_MISA) & (1 << (ext - 'a'));
 #elif CONFIG_CPU
+	char sext[2] = {ext};
 	struct udevice *dev;
 	const char *isa;
-	int i;
+	int ret, i;
 
 	uclass_find_first_device(UCLASS_CPU, &dev);
 	if (!dev) {
@@ -48,27 +49,40 @@ static inline bool supports_extension(char ext)
 		return false;
 	}
 
+	ret = dev_read_stringlist_search(dev, "riscv,isa-extensions", sext);
+	if (ret >= 0)
+		return true;
+
+	/*
+	 * Only if the property is not found (ENODATA) is the fallback to
+	 * riscv,isa used, otherwise the extension is not present in this
+	 * CPU.
+	 */
+	if (ret != -ENODATA)
+		return false;
+
 	isa = dev_read_string(dev, "riscv,isa");
-	if (isa) {
-		/*
-		 * skip the first 4 characters (rv32|rv64)
-		 */
-		for (i = 4; i < sizeof(isa); i++) {
-			switch (isa[i]) {
-			case 's':
-			case 'x':
-			case 'z':
-			case '_':
-			case '\0':
-				/*
-				 * Any of these characters mean the single
-				 * letter extensions have all been consumed.
-				 */
-				return false;
-			default:
-				if (isa[i] == ext)
-					return true;
-			}
+	if (!isa)
+		return false;
+
+	/*
+	 * Skip the first 4 characters (rv32|rv64).
+	 */
+	for (i = 4; i < sizeof(isa); i++) {
+		switch (isa[i]) {
+		case 's':
+		case 'x':
+		case 'z':
+		case '_':
+		case '\0':
+			/*
+			 * Any of these characters mean the single
+			 * letter extensions have all been consumed.
+			 */
+			return false;
+		default:
+			if (isa[i] == ext)
+				return true;
 		}
 	}
 
-- 
2.43.0

Re: [PATCH v1 1/2] riscv: don't read riscv, isa in the riscv cpu's get_desc()

[Leo Liang]

On Mon, Mar 18, 2024 at 03:16:02PM +0000, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> cpu_get_desc() for the RISC-V CPU currently reads "riscv,isa" to get
> the description, but it is no longer a required property and cannot be
> assummed to always be present, as the new "riscv,isa-extensions" and
> "riscv,isa-base" properties may be present instead.
> 
> On RISC-V, cpu_get_desc() has two main uses - firstly providing an
> informational name for the CPU for smbios or at boot with
> DISPLAY_CPUINFO etc and secondly it forms the basis of ISA extension
> detection in supports_extension() as it returns (a portion of) an ISA
> string.
> 
> cpu_get_desc() returns a string, which aligned with "riscv,isa" but
> the new property is a list of strings. Rather than add support for
> the list of strings property, which would require creating an isa
> string from "riscv,isa-extensions", modify the RISC-V CPU's
> implementaion of cpu_get_desc() return the first compatible as the
> cpu description instead. This may be fine for the informational cases,
> but it would break extension dtection, given supports_extension()
> expects cpu_get_desc() to return an ISA string.
> 
> Call dev_read_string() directly in supports_extension() to get the
> contents of "riscv,isa" so that extension detection remains functional.
> As a knock-on affect of this change, extension detection is no longer
> broken for long ISA strings. Previously if the ISA string exceeded the
> 32 element array that supports_extension() passed to cpu_get_desc(),
> it would return ENOSPC and no extensions would be detected.
> This bug probably had no impact as U-Boot does not currently do anything
> meaningful with the results of supports_extension() and most SoCs
> supported by U-Boot don't have anywhere near that complex of an ISA
> string. The QEMU virt machine's CPUs do however, so extension detection
> doesn't work there.
> 
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
> I'm not really sure if I am happy with this patch - people could
> definitely have got use out of the cpu info printout of the ISA string
> before this patch - they'd have seen something like
> CPU:	rv64imafdc
> at boot, but now they will see
> CPU:	sifive,u74
> If it really is desired, cpu_get_desc() could be made to assemble
> an isa string out of "riscv,isa-extensions", but I think it's always
> gonna be a bit flawed, since that string can run to almost arbitrary
> length now - one I saw for a CPU last week was 320 characters long
> and these things are only going to grow.
> ---
>  arch/riscv/cpu/cpu.c    | 12 +++++++-----
>  drivers/cpu/riscv_cpu.c |  8 ++++----
>  2 files changed, 11 insertions(+), 9 deletions(-)

LGTM!

reviewed-by: Leo Yu-Chi Liang <ycliang@andestech.com>

Re: [PATCH v1 2/2] riscv: support extension probing using riscv, isa-extensions

[Leo Liang]

On Mon, Mar 18, 2024 at 03:16:03PM +0000, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> A new property has been added, with an extensive rationale at [1], that
> can be used in place of "riscv,isa" to indicate what extensions are
> supported by a given platform that is a list of strings rather than a
> single string. There are some differences between the new property,
> "riscv,isa-extensions" and the incumbent "riscv,isa" - chief among them
> for the sake of parsing being the list of strings, as opposed to a
> string. Another advantage is strictly defined meanings for each string
> in a dt-binding, rather than deriving meaning from RVI standards. This
> will likely to some divergence over time, but U-Boot's current use of
> extension detection is very limited - there are just four callsites of
> supports_extension() in mainline U-Boot.
> 
> These checks are limited to two checks for FPU support and two checks
> for "s" and "u". "s" and "u" are not supported by the new property, but
> they were also not permitted in "riscv,isa". These checks are only
> meaningful (or run) in M-Mode, in which case supports_extension() does
> not parse the devicetree anyway.
> 
> Add support for the new property in U-Boot, prioritising it, before
> falling back to the, now deprecated, "riscv,isa" property if it is not
> present.
> 
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
> I moved the kernel devicetrees to use the new properties, I'd do the
> same here, but I'd rather just move things to use dt-rebasing instead,
> where possible.
> ---
>  arch/riscv/cpu/cpu.c | 56 +++++++++++++++++++++++++++-----------------
>  1 file changed, 35 insertions(+), 21 deletions(-)

Reviewed-by: Leo Yu-Chi Liang <ycliang@andestech.com>