[PATCH v1 0/7] ISA string parser cleanups++

[PATCH v1 0/7] ISA string parser cleanups++

[Conor Dooley]

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

This stuff goes on top of riscv/for-next plus this series from Yangyu
that made me go looking at the ISA string parser again:
https://lore.kernel.org/all/tencent_E6911C8D71F5624E432A1AFDF86804C3B509@qq.com/

With that out of the way, here are some cleanups for our riscv,isa
handling.
One of these patches is yoinked from Sunil's ACPI series & tweaked
slightly since this needs to apply independently of that, that runs the
isa string parsing loop only over _possible_ cpus.

Other than that, there are some bits that were discussed with Drew on
the "should we allow caps" threads that I have now created patches for:
- splitting of riscv_of_processor_hartid() into two distinct functions,
  one for use purely during early boot, prior to the establishment of
  the possible-cpus mask & another to fit the other current use-cases.
- this allows us to then completely skip some validation of the hartid
  in the parser.
- the biggest diff in the series is a rework of the comments in the
  parser, as I have mostly found the existing (sparse) ones to not be
  all that helpful whenever I have to go back and look at it.
- from writing the comments, I found a conditional doing a bit of a
  dance that I found counter-intuitive, so I've had a go at making that
  match what I would expect a little better.
- `i` implies `Zicsr` & `Zifence`, so add them as extensions and set
  them for the craic. Sure why not like. Of all the patches here, this
  is the one I can most take-or-leave.

Cheers,
Conor.

CC: Paul Walmsley <paul.walmsley@sifive.com>
CC: Palmer Dabbelt <palmer@dabbelt.com>
CC: Conor Dooley <conor.dooley@microchip.com>
CC: Andrew Jones <ajones@ventanamicro.com>
CC: Sunil V L <sunilvl@ventanamicro.com>
CC: Yangyu Chen <cyy@cyyself.name>
CC: linux-riscv@lists.infradead.org

Conor Dooley (6):
  RISC-V: simplify register width check in ISA string parsing
  RISC-V: split early & late of_node to hartid mapping
  RISC-V: validate riscv,isa at boot, not during ISA string parsing
  RISC-V: rework comments in ISA string parser
  RISC-V: remove decrement/increment dance in ISA string parser
  RISC-V: always report presence of Zicsr/Zifencei

Sunil V L (1):
  RISC-V: only iterate over possible CPUs in ISA string parser

 arch/riscv/include/asm/hwcap.h     |   2 +
 arch/riscv/include/asm/processor.h |   1 +
 arch/riscv/kernel/cpu.c            |  32 +++++++-
 arch/riscv/kernel/cpufeature.c     | 114 ++++++++++++++++++++++-------
 arch/riscv/kernel/smpboot.c        |   2 +-
 5 files changed, 118 insertions(+), 33 deletions(-)


base-commit: c2d3c8441e3ddbfe41fea9282ddc6ee372e154cd
prerequisite-patch-id: 50cc6c119a7f8f60b06829b2fafc90c9817f532c
prerequisite-patch-id: 4e2f66d8590db938d2e1a4e9bfaad58ee0ab3525
-- 
2.39.2


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

[PATCH v1 1/7] RISC-V: simplify register width check in ISA string parsing

[Conor Dooley]

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

Saving off the `isa` pointer to a temp variable, followed by checking if
it has been incremented is a bit of an odd pattern. Perhaps it was done
to avoid a funky looking if statement mixed with the ifdeffery.

Now that we use IS_ENABLED() here just return from the parser as soon as
we detect a mismatch between the string and the currently running
kernel.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 arch/riscv/kernel/cpufeature.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index cb32658180da..00df7a3a3931 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -115,7 +115,6 @@ void __init riscv_fill_hwcap(void)
 	for_each_of_cpu_node(node) {
 		unsigned long this_hwcap = 0;
 		DECLARE_BITMAP(this_isa, RISCV_ISA_EXT_MAX);
-		const char *temp;
 
 		rc = riscv_of_processor_hartid(node, &hartid);
 		if (rc < 0)
@@ -126,14 +125,14 @@ void __init riscv_fill_hwcap(void)
 			continue;
 		}
 
-		temp = isa;
-		if (IS_ENABLED(CONFIG_32BIT) && !strncasecmp(isa, "rv32", 4))
-			isa += 4;
-		else if (IS_ENABLED(CONFIG_64BIT) && !strncasecmp(isa, "rv64", 4))
-			isa += 4;
-		/* The riscv,isa DT property must start with rv64 or rv32 */
-		if (temp == isa)
+		if (IS_ENABLED(CONFIG_32BIT) && strncasecmp(isa, "rv32", 4))
 			continue;
+
+		if (IS_ENABLED(CONFIG_64BIT) && strncasecmp(isa, "rv64", 4))
+			continue;
+
+		isa += 4;
+
 		bitmap_zero(this_isa, RISCV_ISA_EXT_MAX);
 		for (; *isa; ++isa) {
 			const char *ext = isa++;
-- 
2.39.2


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

[PATCH v1 2/7] RISC-V: only iterate over possible CPUs in ISA string parser

[Conor Dooley]

From: Sunil V L <sunilvl@ventanamicro.com>

During boot we call riscv_of_processor_hartid() for each hart that we
add to the possible cpus list. Repeating the call again here is not
required, if we iterate over the list of possible CPUs, rather than the
list of all CPUs.

The call to of_property_read_string() for "riscv,isa" cannot fail
either, as it has previously succeeded in riscv_of_processor_hartid(),
but leaving in the error checking makes the operation of the loop more
obvious & provides leeway for future refactoring of
riscv_of_processor_hartid().

Partially ripped from Sunil's ACPI support series, with the logic
inverted to continue early on failure.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
Co-developed-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 arch/riscv/kernel/cpufeature.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 00df7a3a3931..3ae456413f79 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -12,6 +12,7 @@
 #include <linux/memory.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <asm/alternative.h>
 #include <asm/cacheflush.h>
 #include <asm/cpufeature.h>
@@ -99,7 +100,7 @@ void __init riscv_fill_hwcap(void)
 	char print_str[NUM_ALPHA_EXTS + 1];
 	int i, j, rc;
 	unsigned long isa2hwcap[26] = {0};
-	unsigned long hartid;
+	unsigned int cpu;
 
 	isa2hwcap['i' - 'a'] = COMPAT_HWCAP_ISA_I;
 	isa2hwcap['m' - 'a'] = COMPAT_HWCAP_ISA_M;
@@ -112,15 +113,19 @@ void __init riscv_fill_hwcap(void)
 
 	bitmap_zero(riscv_isa, RISCV_ISA_EXT_MAX);
 
-	for_each_of_cpu_node(node) {
+	for_each_possible_cpu(cpu) {
 		unsigned long this_hwcap = 0;
 		DECLARE_BITMAP(this_isa, RISCV_ISA_EXT_MAX);
 
-		rc = riscv_of_processor_hartid(node, &hartid);
-		if (rc < 0)
+		node = of_cpu_device_node_get(cpu);
+		if (!node) {
+			pr_warn("Unable to find cpu node\n");
 			continue;
+		}
 
-		if (of_property_read_string(node, "riscv,isa", &isa)) {
+		rc = of_property_read_string(node, "riscv,isa", &isa);
+		of_node_put(node);
+		if (rc) {
 			pr_warn("Unable to find \"riscv,isa\" devicetree entry\n");
 			continue;
 		}
-- 
2.39.2


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

[PATCH v1 3/7] RISC-V: split early & late of_node to hartid mapping

[Conor Dooley]

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

Some back and forth with Drew [1] about riscv_fill_hwcap() resulted in
the realisation that it is not very useful to parse the DT & perform
validation of riscv,isa every time we would like to get the id for a
hart.

Although it is no longer called in riscv_fill_hwcap(),
riscv_of_processor_hartid() is called in several other places.
Notably in setup_smp() it forms part of the logic for filling the mask
of possible CPUs. Since a possible CPU must have passed this basic
validation of riscv,isa, a repeat validation is not required.

Rename riscv_of_processor_id() to riscv_early_of_processor_id(),
which will be called from setup_smp() & introduce a new
riscv_of_processor_id() which makes use of the pre-populated mask of
possible cpus.

Link: https://lore.kernel.org/linux-riscv/xvdswl3iyikwvamny7ikrxo2ncuixshtg3f6uucjahpe3xpc5c@ud4cz4fkg5dj/ [1]
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 arch/riscv/include/asm/processor.h |  1 +
 arch/riscv/kernel/cpu.c            | 22 +++++++++++++++++++++-
 arch/riscv/kernel/smpboot.c        |  2 +-
 3 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
index 94a0590c6971..3479f9fca4b0 100644
--- a/arch/riscv/include/asm/processor.h
+++ b/arch/riscv/include/asm/processor.h
@@ -75,6 +75,7 @@ static inline void wait_for_interrupt(void)
 
 struct device_node;
 int riscv_of_processor_hartid(struct device_node *node, unsigned long *hartid);
+int riscv_early_of_processor_hartid(struct device_node *node, unsigned long *hartid);
 int riscv_of_parent_hartid(struct device_node *node, unsigned long *hartid);
 
 extern void riscv_fill_hwcap(void);
diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
index f4dadbfecd04..7030a5004f8e 100644
--- a/arch/riscv/kernel/cpu.c
+++ b/arch/riscv/kernel/cpu.c
@@ -20,6 +20,26 @@
  * isn't an enabled and valid RISC-V hart node.
  */
 int riscv_of_processor_hartid(struct device_node *node, unsigned long *hart)
+{
+	int cpu;
+
+	*hart = (unsigned long)of_get_cpu_hwid(node, 0);
+	if (*hart == ~0UL) {
+		pr_warn("Found CPU without hart ID\n");
+		return -ENODEV;
+	}
+
+	cpu = riscv_hartid_to_cpuid(*hart);
+	if (cpu < 0)
+		return cpu;
+
+	if (!cpu_possible(cpu))
+		return -ENODEV;
+
+	return 0;
+}
+
+int riscv_early_of_processor_hartid(struct device_node *node, unsigned long *hart)
 {
 	const char *isa;
 
@@ -28,7 +48,7 @@ int riscv_of_processor_hartid(struct device_node *node, unsigned long *hart)
 		return -ENODEV;
 	}
 
-	*hart = (unsigned long) of_get_cpu_hwid(node, 0);
+	*hart = (unsigned long)of_get_cpu_hwid(node, 0);
 	if (*hart == ~0UL) {
 		pr_warn("Found CPU without hart ID\n");
 		return -ENODEV;
diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
index 445a4efee267..626238200010 100644
--- a/arch/riscv/kernel/smpboot.c
+++ b/arch/riscv/kernel/smpboot.c
@@ -81,7 +81,7 @@ void __init setup_smp(void)
 	cpu_set_ops(0);
 
 	for_each_of_cpu_node(dn) {
-		rc = riscv_of_processor_hartid(dn, &hart);
+		rc = riscv_early_of_processor_hartid(dn, &hart);
 		if (rc < 0)
 			continue;
 
-- 
2.39.2


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

[PATCH v1 4/7] RISC-V: validate riscv,isa at boot, not during ISA string parsing

[Conor Dooley]

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

Since riscv_fill_hwcap() now only iterates over possible cpus, the
basic validation of whether riscv,isa contains "rv<width>" can be moved
to riscv_early_of_processor_hartid().

Further, "ima" support is required by the kernel, so reject any CPU not
fitting the bill.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 arch/riscv/kernel/cpu.c        |  8 +++++---
 arch/riscv/kernel/cpufeature.c | 12 ++++++------
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
index 7030a5004f8e..b0c3ec0f2f5b 100644
--- a/arch/riscv/kernel/cpu.c
+++ b/arch/riscv/kernel/cpu.c
@@ -63,10 +63,12 @@ int riscv_early_of_processor_hartid(struct device_node *node, unsigned long *har
 		pr_warn("CPU with hartid=%lu has no \"riscv,isa\" property\n", *hart);
 		return -ENODEV;
 	}
-	if (tolower(isa[0]) != 'r' || tolower(isa[1]) != 'v') {
-		pr_warn("CPU with hartid=%lu has an invalid ISA of \"%s\"\n", *hart, isa);
+
+	if (IS_ENABLED(CONFIG_32BIT) && strncasecmp(isa, "rv32ima", 7))
+		return -ENODEV;
+
+	if (IS_ENABLED(CONFIG_64BIT) && strncasecmp(isa, "rv64ima", 7))
 		return -ENODEV;
-	}
 
 	return 0;
 }
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 3ae456413f79..a79c5c52a174 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -130,12 +130,12 @@ void __init riscv_fill_hwcap(void)
 			continue;
 		}
 
-		if (IS_ENABLED(CONFIG_32BIT) && strncasecmp(isa, "rv32", 4))
-			continue;
-
-		if (IS_ENABLED(CONFIG_64BIT) && strncasecmp(isa, "rv64", 4))
-			continue;
-
+		/*
+		 * For all possible cpus, we have already validated in
+		 * the boot process that they at least contain "rv" and
+		 * whichever of "32"/"64" this kernel supports, and so this
+		 * section can be skipped.
+		 */
 		isa += 4;
 
 		bitmap_zero(this_isa, RISCV_ISA_EXT_MAX);
-- 
2.39.2


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

[PATCH v1 5/7] RISC-V: rework comments in ISA string parser

[Conor Dooley]

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

I have found these comments to not be at all helpful whenever I look at
the parser. Further, the comments in the default case (single letter
parser) are not quite right either.
Group the comments into a larger one at the start of each case, that
attempts to explain things at a higher level.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 arch/riscv/kernel/cpufeature.c | 71 ++++++++++++++++++++++++++++------
 1 file changed, 60 insertions(+), 11 deletions(-)

diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index a79c5c52a174..2fc72f092057 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -146,7 +146,7 @@ void __init riscv_fill_hwcap(void)
 
 			switch (*ext) {
 			case 's':
-				/**
+				/*
 				 * Workaround for invalid single-letter 's' & 'u'(QEMU).
 				 * No need to set the bit in riscv_isa as 's' & 'u' are
 				 * not valid ISA extensions. It works until multi-letter
@@ -163,53 +163,102 @@ void __init riscv_fill_hwcap(void)
 			case 'X':
 			case 'z':
 			case 'Z':
+				/*
+				 * Before attempting to parse the extension itself, we find its end.
+				 * As multi-letter extensions must be split from other multi-letter
+				 * extensions with an "_", the end of a multi-letter extension will
+				 * either be the null character as of_property_read_string() returns
+				 * null-terminated strings, or the "_" at the start of the next
+				 * multi-letter extension.
+				 *
+				 * Next, as the extensions version is currently ignored, we
+				 * eliminate that portion. This is done by parsing backwards from
+				 * the end of the extension, removing any numbers. This may be a
+				 * major or minor number however, so the process is repeated if a
+				 * minor number was found.
+				 *
+				 * ext_end is intended to represent the first character *after* the
+				 * name portion of an extension, but will be decremented to the last
+				 * character itself while eliminating the extensions version number.
+				 * A simple re-increment solves this problem.
+				 */
 				ext_long = true;
-				/* Multi-letter extension must be delimited */
 				for (; *isa && *isa != '_'; ++isa)
 					if (unlikely(!isalnum(*isa)))
 						ext_err = true;
-				/* Parse backwards */
+
 				ext_end = isa;
 				if (unlikely(ext_err))
 					break;
+
 				if (!isdigit(ext_end[-1]))
 					break;
-				/* Skip the minor version */
+
 				while (isdigit(*--ext_end))
 					;
-				if (tolower(ext_end[0]) != 'p'
-				    || !isdigit(ext_end[-1])) {
-					/* Advance it to offset the pre-decrement */
+
+				if (tolower(ext_end[0]) != 'p' || !isdigit(ext_end[-1])) {
 					++ext_end;
 					break;
 				}
-				/* Skip the major version */
+
 				while (isdigit(*--ext_end))
 					;
+
 				++ext_end;
 				break;
 			default:
+				/*
+				 * Things are a little easier for single-letter extensions, as they
+				 * are parsed forwards.
+				 *
+				 * After checking that our starting position is valid, we need to
+				 * ensure that, when isa was incremented at the start of the loop,
+				 * that it arrived at the start of the next extension.
+				 *
+				 * If we are already on a non-digit, there is nothing to do. Either
+				 * we have a multi-letter extension's _, or the start of an
+				 * extension.
+				 *
+				 * Otherwise we have found the current extension's major version
+				 * number. Parse past it, and a subsequent p/minor version number
+				 * if present. The `p` extension must not appear immediately after
+				 * a number, so there is no fear of missing it.
+				 *
+				 */
 				if (unlikely(!isalpha(*ext))) {
 					ext_err = true;
 					break;
 				}
-				/* Find next extension */
+
 				if (!isdigit(*isa))
 					break;
-				/* Skip the minor version */
+
 				while (isdigit(*++isa))
 					;
+
 				if (tolower(*isa) != 'p')
 					break;
+
 				if (!isdigit(*++isa)) {
 					--isa;
 					break;
 				}
-				/* Skip the major version */
+
 				while (isdigit(*++isa))
 					;
+
 				break;
 			}
+
+			/*
+			 * The parser expects that at the start of an iteration isa points to the
+			 * character before the start of the next extension. This will not be the
+			 * case if we have just parsed a single-letter extension and the next
+			 * extension is not a multi-letter extension prefixed with an "_". It is
+			 * also not the case at the end of the string, where it will point to the
+			 * terminating null character.
+			 */
 			if (*isa != '_')
 				--isa;
 
-- 
2.39.2


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

[PATCH v1 6/7] RISC-V: remove decrement/increment dance in ISA string parser

[Conor Dooley]

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

While expanding on the comments in the ISA string parsing code, I
noticed that the conditional decrement of `isa` at the end of the loop
was a bit odd.
The parsing code expects that at the start of the for loop, `isa` will
point to the first character of the next unparsed extension.
However, depending on what the next extension is, this may not be true.
Unless the next extension is a multi-letter extension preceded by an
underscore, `isa` will either point to the string's null-terminator or
to the first character of the next extension, once the switch statement
has been evaluated.
Obviously incrementing `isa` at the end of the loop could cause it to
increment past the null terminator or miss a single letter extension, so
`isa` is conditionally decremented, just so that the loop can increment
it again.

It's easier to understand the code if, instead of this decrement +
increment dance, we instead use a while loop & rely on the handling of
individual extension types to leave `isa` pointing to the first
character of the next extension.
As already mentioned, this won't be the case where the following
extension is multi-letter & preceded by an underscore. To handle that,
invert the check and increment rather than decrement.
Hopefully this eliminates a "huh?!?" moment the next time somebody tries
to understand this code.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 arch/riscv/kernel/cpufeature.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 2fc72f092057..b425658bbf08 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -139,7 +139,7 @@ void __init riscv_fill_hwcap(void)
 		isa += 4;
 
 		bitmap_zero(this_isa, RISCV_ISA_EXT_MAX);
-		for (; *isa; ++isa) {
+		while (*isa) {
 			const char *ext = isa++;
 			const char *ext_end = isa;
 			bool ext_long = false, ext_err = false;
@@ -253,14 +253,12 @@ void __init riscv_fill_hwcap(void)
 
 			/*
 			 * The parser expects that at the start of an iteration isa points to the
-			 * character before the start of the next extension. This will not be the
-			 * case if we have just parsed a single-letter extension and the next
-			 * extension is not a multi-letter extension prefixed with an "_". It is
-			 * also not the case at the end of the string, where it will point to the
-			 * terminating null character.
+			 * first character of the next extension. As we stop parsing an extension
+			 * on meeting a non-alphanumeric character, an extra increment is needed
+			 * where the succeeding extension is a multi-letter prefixed with an "_".
 			 */
-			if (*isa != '_')
-				--isa;
+			if (*isa == '_')
+				++isa;
 
 #define SET_ISA_EXT_MAP(name, bit)							\
 			do {								\
-- 
2.39.2


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

[PATCH v1 7/7] RISC-V: always report presence of Zicsr/Zifencei

[Conor Dooley]

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

Zicsr/Zifencei were part of i when the port was written and are required
by the kernel. There's not much that userspace can do with this extra
information, but there is no harm in reporting an ISA string that closer
resembles the current versions of the ISA specifications either.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 arch/riscv/include/asm/hwcap.h | 2 ++
 arch/riscv/kernel/cpu.c        | 2 ++
 arch/riscv/kernel/cpufeature.c | 7 +++++++
 3 files changed, 11 insertions(+)

diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index 9af793970855..aa61031f7923 100644
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -44,6 +44,8 @@
 #define RISCV_ISA_EXT_ZIHINTPAUSE	32
 #define RISCV_ISA_EXT_SVNAPOT		33
 #define RISCV_ISA_EXT_ZICBOZ		34
+#define RISCV_ISA_EXT_ZICSR		35
+#define RISCV_ISA_EXT_ZIFENCEI		36
 
 #define RISCV_ISA_EXT_MAX		64
 #define RISCV_ISA_EXT_NAME_LEN_MAX	32
diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
index b0c3ec0f2f5b..0d5d580dca61 100644
--- a/arch/riscv/kernel/cpu.c
+++ b/arch/riscv/kernel/cpu.c
@@ -204,8 +204,10 @@ arch_initcall(riscv_cpuinfo_init);
  * New entries to this struct should follow the ordering rules described above.
  */
 static struct riscv_isa_ext_data isa_ext_arr[] = {
+	__RISCV_ISA_EXT_DATA(zicsr, RISCV_ISA_EXT_ZICSR),
 	__RISCV_ISA_EXT_DATA(zicbom, RISCV_ISA_EXT_ZICBOM),
 	__RISCV_ISA_EXT_DATA(zicboz, RISCV_ISA_EXT_ZICBOZ),
+	__RISCV_ISA_EXT_DATA(zifencei, RISCV_ISA_EXT_ZIFENCEI),
 	__RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
 	__RISCV_ISA_EXT_DATA(zbb, RISCV_ISA_EXT_ZBB),
 	__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index b425658bbf08..92f0e7b78eef 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -292,6 +292,13 @@ void __init riscv_fill_hwcap(void)
 #undef SET_ISA_EXT_MAP
 		}
 
+		/*
+		 * Linux requires Zicsr & Zifencei, so we may as well always
+		 * set them.
+		 */
+		set_bit(RISCV_ISA_EXT_ZIFENCEI, this_isa);
+		set_bit(RISCV_ISA_EXT_ZICSR, this_isa);
+
 		/*
 		 * All "okay" hart should have same isa. Set HWCAP based on
 		 * common capabilities of every "okay" hart, in case they don't
-- 
2.39.2


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

Re: [PATCH v1 7/7] RISC-V: always report presence of Zicsr/Zifencei

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 763 bytes --]

On Thu, May 04, 2023 at 07:14:26PM +0100, Conor Dooley wrote:

> @@ -204,8 +204,10 @@ arch_initcall(riscv_cpuinfo_init);
>   * New entries to this struct should follow the ordering rules described above.
>   */
>  static struct riscv_isa_ext_data isa_ext_arr[] = {
> +	__RISCV_ISA_EXT_DATA(zicsr, RISCV_ISA_EXT_ZICSR),

Heh, well that is not the right order, per the rules I wrote in the
comment above. Whoops...

>  	__RISCV_ISA_EXT_DATA(zicbom, RISCV_ISA_EXT_ZICBOM),
>  	__RISCV_ISA_EXT_DATA(zicboz, RISCV_ISA_EXT_ZICBOZ),
> +	__RISCV_ISA_EXT_DATA(zifencei, RISCV_ISA_EXT_ZIFENCEI),
>  	__RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
>  	__RISCV_ISA_EXT_DATA(zbb, RISCV_ISA_EXT_ZBB),
>  	__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

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

Re: [PATCH v1 1/7] RISC-V: simplify register width check in ISA string parsing

[Andrew Jones]

On Thu, May 04, 2023 at 07:14:20PM +0100, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> Saving off the `isa` pointer to a temp variable, followed by checking if
> it has been incremented is a bit of an odd pattern. Perhaps it was done
> to avoid a funky looking if statement mixed with the ifdeffery.
> 
> Now that we use IS_ENABLED() here just return from the parser as soon as
> we detect a mismatch between the string and the currently running
> kernel.
> 
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  arch/riscv/kernel/cpufeature.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

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

Re: [PATCH v1 2/7] RISC-V: only iterate over possible CPUs in ISA string parser

[Andrew Jones]

On Thu, May 04, 2023 at 07:14:21PM +0100, Conor Dooley wrote:
> From: Sunil V L <sunilvl@ventanamicro.com>
> 
> During boot we call riscv_of_processor_hartid() for each hart that we
> add to the possible cpus list. Repeating the call again here is not
> required, if we iterate over the list of possible CPUs, rather than the
> list of all CPUs.
> 
> The call to of_property_read_string() for "riscv,isa" cannot fail
> either, as it has previously succeeded in riscv_of_processor_hartid(),
> but leaving in the error checking makes the operation of the loop more
> obvious & provides leeway for future refactoring of
> riscv_of_processor_hartid().
> 
> Partially ripped from Sunil's ACPI support series, with the logic
> inverted to continue early on failure.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> Co-developed-by: Conor Dooley <conor.dooley@microchip.com>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  arch/riscv/kernel/cpufeature.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

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

Re: [PATCH v1 4/7] RISC-V: validate riscv,isa at boot, not during ISA string parsing

[Andrew Jones]

On Thu, May 04, 2023 at 07:14:23PM +0100, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> Since riscv_fill_hwcap() now only iterates over possible cpus, the
> basic validation of whether riscv,isa contains "rv<width>" can be moved
> to riscv_early_of_processor_hartid().
> 
> Further, "ima" support is required by the kernel, so reject any CPU not
> fitting the bill.
>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  arch/riscv/kernel/cpu.c        |  8 +++++---
>  arch/riscv/kernel/cpufeature.c | 12 ++++++------
>  2 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index 7030a5004f8e..b0c3ec0f2f5b 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -63,10 +63,12 @@ int riscv_early_of_processor_hartid(struct device_node *node, unsigned long *har
>  		pr_warn("CPU with hartid=%lu has no \"riscv,isa\" property\n", *hart);
>  		return -ENODEV;
>  	}
> -	if (tolower(isa[0]) != 'r' || tolower(isa[1]) != 'v') {
> -		pr_warn("CPU with hartid=%lu has an invalid ISA of \"%s\"\n", *hart, isa);
> +
> +	if (IS_ENABLED(CONFIG_32BIT) && strncasecmp(isa, "rv32ima", 7))
> +		return -ENODEV;
> +
> +	if (IS_ENABLED(CONFIG_64BIT) && strncasecmp(isa, "rv64ima", 7))

'ima' matches the DT binding pattern requirement and the order required
by 27.11 "Subset Naming Convention". If the spec ever squeezes more single
letter extensions into the front of the ISA string, then we can cross
that bridge when we get to it.

>  		return -ENODEV;
> -	}
>  
>  	return 0;
>  }
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 3ae456413f79..a79c5c52a174 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -130,12 +130,12 @@ void __init riscv_fill_hwcap(void)
>  			continue;
>  		}
>  
> -		if (IS_ENABLED(CONFIG_32BIT) && strncasecmp(isa, "rv32", 4))
> -			continue;
> -
> -		if (IS_ENABLED(CONFIG_64BIT) && strncasecmp(isa, "rv64", 4))
> -			continue;
> -
> +		/*
> +		 * For all possible cpus, we have already validated in
> +		 * the boot process that they at least contain "rv" and
> +		 * whichever of "32"/"64" this kernel supports, and so this
> +		 * section can be skipped.
> +		 */
>  		isa += 4;

When we add RV128 support this will need a tweak, but that's for another
day. Since all ISA strings must start with rvXX per the spec, then this
works for ACPI too, which states the isa string in its ISA string table
must conform to the unpriv spec.

>  
>  		bitmap_zero(this_isa, RISCV_ISA_EXT_MAX);
> -- 
> 2.39.2
> 

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Thanks,
drew

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

Re: [PATCH v1 4/7] RISC-V: validate riscv,isa at boot, not during ISA string parsing

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 3453 bytes --]

On Fri, May 05, 2023 at 09:40:22AM +0200, Andrew Jones wrote:
> On Thu, May 04, 2023 at 07:14:23PM +0100, Conor Dooley wrote:
> > From: Conor Dooley <conor.dooley@microchip.com>
> > 
> > Since riscv_fill_hwcap() now only iterates over possible cpus, the
> > basic validation of whether riscv,isa contains "rv<width>" can be moved
> > to riscv_early_of_processor_hartid().
> > 
> > Further, "ima" support is required by the kernel, so reject any CPU not
> > fitting the bill.
> >
> > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> > ---
> >  arch/riscv/kernel/cpu.c        |  8 +++++---
> >  arch/riscv/kernel/cpufeature.c | 12 ++++++------
> >  2 files changed, 11 insertions(+), 9 deletions(-)
> > 
> > diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> > index 7030a5004f8e..b0c3ec0f2f5b 100644
> > --- a/arch/riscv/kernel/cpu.c
> > +++ b/arch/riscv/kernel/cpu.c
> > @@ -63,10 +63,12 @@ int riscv_early_of_processor_hartid(struct device_node *node, unsigned long *har
> >  		pr_warn("CPU with hartid=%lu has no \"riscv,isa\" property\n", *hart);
> >  		return -ENODEV;
> >  	}
> > -	if (tolower(isa[0]) != 'r' || tolower(isa[1]) != 'v') {
> > -		pr_warn("CPU with hartid=%lu has an invalid ISA of \"%s\"\n", *hart, isa);
> > +
> > +	if (IS_ENABLED(CONFIG_32BIT) && strncasecmp(isa, "rv32ima", 7))
> > +		return -ENODEV;
> > +
> > +	if (IS_ENABLED(CONFIG_64BIT) && strncasecmp(isa, "rv64ima", 7))
> 
> 'ima' matches the DT binding pattern requirement and the order required
> by 27.11 "Subset Naming Convention".

Aye, perhaps I should have mentioned it in my commit message that those
particular orders are required by the dt-binding. If the ACPI series
lands before this one does, since I'll have to rebase anyway, I'll add a
mention of the ACPI spec's position.

> If the spec ever squeezes more single
> letter extensions into the front of the ISA string, then we can cross
> that bridge when we get to it.

Perhaps by then we have evolved beyond needing the ISA string!
(A boy can dream)

> >  		return -ENODEV;
> > -	}
> >  
> >  	return 0;
> >  }
> > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > index 3ae456413f79..a79c5c52a174 100644
> > --- a/arch/riscv/kernel/cpufeature.c
> > +++ b/arch/riscv/kernel/cpufeature.c
> > @@ -130,12 +130,12 @@ void __init riscv_fill_hwcap(void)
> >  			continue;
> >  		}
> >  
> > -		if (IS_ENABLED(CONFIG_32BIT) && strncasecmp(isa, "rv32", 4))
> > -			continue;
> > -
> > -		if (IS_ENABLED(CONFIG_64BIT) && strncasecmp(isa, "rv64", 4))
> > -			continue;
> > -
> > +		/*
> > +		 * For all possible cpus, we have already validated in
> > +		 * the boot process that they at least contain "rv" and
> > +		 * whichever of "32"/"64" this kernel supports, and so this
> > +		 * section can be skipped.
> > +		 */
> >  		isa += 4;
> 
> When we add RV128 support this will need a tweak, but that's for another
> day.

Yeah, adding 128-bit is so far off that it's not worth considering, but
easily handled in this particular location.

> Since all ISA strings must start with rvXX per the spec, then this
> works for ACPI too, which states the isa string in its ISA string table
> must conform to the unpriv spec.
> 
> >  
> >  		bitmap_zero(this_isa, RISCV_ISA_EXT_MAX);
> > -- 
> > 2.39.2
> > 
> 
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Thanks!

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

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

Re: [PATCH v1 5/7] RISC-V: rework comments in ISA string parser

[Andrew Jones]

On Thu, May 04, 2023 at 07:14:24PM +0100, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> I have found these comments to not be at all helpful whenever I look at
> the parser. Further, the comments in the default case (single letter
> parser) are not quite right either.
> Group the comments into a larger one at the start of each case, that
> attempts to explain things at a higher level.
> 
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  arch/riscv/kernel/cpufeature.c | 71 ++++++++++++++++++++++++++++------
>  1 file changed, 60 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index a79c5c52a174..2fc72f092057 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -146,7 +146,7 @@ void __init riscv_fill_hwcap(void)
>  
>  			switch (*ext) {
>  			case 's':
> -				/**
> +				/*
>  				 * Workaround for invalid single-letter 's' & 'u'(QEMU).
>  				 * No need to set the bit in riscv_isa as 's' & 'u' are
>  				 * not valid ISA extensions. It works until multi-letter
> @@ -163,53 +163,102 @@ void __init riscv_fill_hwcap(void)
>  			case 'X':
>  			case 'z':
>  			case 'Z':
> +				/*
> +				 * Before attempting to parse the extension itself, we find its end.
> +				 * As multi-letter extensions must be split from other multi-letter
> +				 * extensions with an "_", the end of a multi-letter extension will
> +				 * either be the null character as of_property_read_string() returns
> +				 * null-terminated strings,

The ACPI table also requires the ISA string be null-terminated. I'd maybe
drop the reference to the DT function and just state the string will be
null-terminated to avoid any ACPI concern.

> or the "_" at the start of the next
> +				 * multi-letter extension.
> +				 *
> +				 * Next, as the extensions version is currently ignored, we
> +				 * eliminate that portion. This is done by parsing backwards from
> +				 * the end of the extension, removing any numbers. This may be a
> +				 * major or minor number however, so the process is repeated if a
> +				 * minor number was found.
> +				 *
> +				 * ext_end is intended to represent the first character *after* the
> +				 * name portion of an extension, but will be decremented to the last
> +				 * character itself while eliminating the extensions version number.
> +				 * A simple re-increment solves this problem.
> +				 */
>  				ext_long = true;
> -				/* Multi-letter extension must be delimited */
>  				for (; *isa && *isa != '_'; ++isa)
>  					if (unlikely(!isalnum(*isa)))
>  						ext_err = true;
> -				/* Parse backwards */
> +
>  				ext_end = isa;
>  				if (unlikely(ext_err))
>  					break;
> +
>  				if (!isdigit(ext_end[-1]))
>  					break;
> -				/* Skip the minor version */
> +
>  				while (isdigit(*--ext_end))
>  					;
> -				if (tolower(ext_end[0]) != 'p'
> -				    || !isdigit(ext_end[-1])) {
> -					/* Advance it to offset the pre-decrement */
> +
> +				if (tolower(ext_end[0]) != 'p' || !isdigit(ext_end[-1])) {
>  					++ext_end;
>  					break;
>  				}
> -				/* Skip the major version */
> +
>  				while (isdigit(*--ext_end))
>  					;
> +
>  				++ext_end;
>  				break;
>  			default:
> +				/*
> +				 * Things are a little easier for single-letter extensions, as they
> +				 * are parsed forwards.
> +				 *
> +				 * After checking that our starting position is valid, we need to
> +				 * ensure that, when isa was incremented at the start of the loop,
> +				 * that it arrived at the start of the next extension.
> +				 *
> +				 * If we are already on a non-digit, there is nothing to do. Either
> +				 * we have a multi-letter extension's _, or the start of an
> +				 * extension.
> +				 *
> +				 * Otherwise we have found the current extension's major version
> +				 * number. Parse past it, and a subsequent p/minor version number
> +				 * if present. The `p` extension must not appear immediately after
> +				 * a number, so there is no fear of missing it.
> +				 *
> +				 */
>  				if (unlikely(!isalpha(*ext))) {
>  					ext_err = true;
>  					break;
>  				}
> -				/* Find next extension */
> +
>  				if (!isdigit(*isa))
>  					break;
> -				/* Skip the minor version */
> +
>  				while (isdigit(*++isa))
>  					;
> +
>  				if (tolower(*isa) != 'p')
>  					break;
> +
>  				if (!isdigit(*++isa)) {
>  					--isa;
>  					break;
>  				}
> -				/* Skip the major version */
> +
>  				while (isdigit(*++isa))
>  					;
> +
>  				break;
>  			}
> +
> +			/*
> +			 * The parser expects that at the start of an iteration isa points to the
> +			 * character before the start of the next extension. This will not be the
> +			 * case if we have just parsed a single-letter extension and the next
> +			 * extension is not a multi-letter extension prefixed with an "_". It is
> +			 * also not the case at the end of the string, where it will point to the
> +			 * terminating null character.
> +			 */
>  			if (*isa != '_')
>  				--isa;
>  
> -- 
> 2.39.2
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Thanks,
drew

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

Re: [PATCH v1 6/7] RISC-V: remove decrement/increment dance in ISA string parser

[Andrew Jones]

On Thu, May 04, 2023 at 07:14:25PM +0100, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> While expanding on the comments in the ISA string parsing code, I
> noticed that the conditional decrement of `isa` at the end of the loop
> was a bit odd.
> The parsing code expects that at the start of the for loop, `isa` will
> point to the first character of the next unparsed extension.
> However, depending on what the next extension is, this may not be true.
> Unless the next extension is a multi-letter extension preceded by an
> underscore, `isa` will either point to the string's null-terminator or
> to the first character of the next extension, once the switch statement
> has been evaluated.
> Obviously incrementing `isa` at the end of the loop could cause it to
> increment past the null terminator or miss a single letter extension, so
> `isa` is conditionally decremented, just so that the loop can increment
> it again.
> 
> It's easier to understand the code if, instead of this decrement +
> increment dance, we instead use a while loop & rely on the handling of
> individual extension types to leave `isa` pointing to the first
> character of the next extension.
> As already mentioned, this won't be the case where the following
> extension is multi-letter & preceded by an underscore. To handle that,
> invert the check and increment rather than decrement.
> Hopefully this eliminates a "huh?!?" moment the next time somebody tries
> to understand this code.
> 
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  arch/riscv/kernel/cpufeature.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

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

Re: [PATCH v1 7/7] RISC-V: always report presence of Zicsr/Zifencei

[Andrew Jones]

On Thu, May 04, 2023 at 07:14:26PM +0100, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> Zicsr/Zifencei were part of i when the port was written and are required
> by the kernel. There's not much that userspace can do with this extra
> information, but there is no harm in reporting an ISA string that closer
> resembles the current versions of the ISA specifications either.
> 
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  arch/riscv/include/asm/hwcap.h | 2 ++
>  arch/riscv/kernel/cpu.c        | 2 ++
>  arch/riscv/kernel/cpufeature.c | 7 +++++++
>  3 files changed, 11 insertions(+)
> 
> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> index 9af793970855..aa61031f7923 100644
> --- a/arch/riscv/include/asm/hwcap.h
> +++ b/arch/riscv/include/asm/hwcap.h
> @@ -44,6 +44,8 @@
>  #define RISCV_ISA_EXT_ZIHINTPAUSE	32
>  #define RISCV_ISA_EXT_SVNAPOT		33
>  #define RISCV_ISA_EXT_ZICBOZ		34
> +#define RISCV_ISA_EXT_ZICSR		35
> +#define RISCV_ISA_EXT_ZIFENCEI		36
>  
>  #define RISCV_ISA_EXT_MAX		64
>  #define RISCV_ISA_EXT_NAME_LEN_MAX	32
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index b0c3ec0f2f5b..0d5d580dca61 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -204,8 +204,10 @@ arch_initcall(riscv_cpuinfo_init);
>   * New entries to this struct should follow the ordering rules described above.
>   */
>  static struct riscv_isa_ext_data isa_ext_arr[] = {
> +	__RISCV_ISA_EXT_DATA(zicsr, RISCV_ISA_EXT_ZICSR),
>  	__RISCV_ISA_EXT_DATA(zicbom, RISCV_ISA_EXT_ZICBOM),
>  	__RISCV_ISA_EXT_DATA(zicboz, RISCV_ISA_EXT_ZICBOZ),
> +	__RISCV_ISA_EXT_DATA(zifencei, RISCV_ISA_EXT_ZIFENCEI),
>  	__RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
>  	__RISCV_ISA_EXT_DATA(zbb, RISCV_ISA_EXT_ZBB),
>  	__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index b425658bbf08..92f0e7b78eef 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -292,6 +292,13 @@ void __init riscv_fill_hwcap(void)
>  #undef SET_ISA_EXT_MAP
>  		}
>  
> +		/*
> +		 * Linux requires Zicsr & Zifencei, so we may as well always
> +		 * set them.
> +		 */
> +		set_bit(RISCV_ISA_EXT_ZIFENCEI, this_isa);
> +		set_bit(RISCV_ISA_EXT_ZICSR, this_isa);
> +
>  		/*
>  		 * All "okay" hart should have same isa. Set HWCAP based on
>  		 * common capabilities of every "okay" hart, in case they don't
> -- 
> 2.39.2
>

Besides the ordering you pointed out,

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

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

Re: [PATCH v1 4/7] RISC-V: validate riscv,isa at boot, not during ISA string parsing

[Yangyu Chen]

Hi,

On 5/5/23 2:14 AM, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
>
> Since riscv_fill_hwcap() now only iterates over possible cpus, the
> basic validation of whether riscv,isa contains "rv<width>" can be moved
> to riscv_early_of_processor_hartid().
>
> Further, "ima" support is required by the kernel, so reject any CPU not
> fitting the bill.
>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  arch/riscv/kernel/cpu.c        |  8 +++++---
>  arch/riscv/kernel/cpufeature.c | 12 ++++++------
>  2 files changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index 7030a5004f8e..b0c3ec0f2f5b 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -63,10 +63,12 @@ int riscv_early_of_processor_hartid(struct device_node *node, unsigned long *har
>  		pr_warn("CPU with hartid=%lu has no \"riscv,isa\" property\n", *hart);
>  		return -ENODEV;
>  	}
> -	if (tolower(isa[0]) != 'r' || tolower(isa[1]) != 'v') {
> -		pr_warn("CPU with hartid=%lu has an invalid ISA of \"%s\"\n", *hart, isa);
> +
> +	if (IS_ENABLED(CONFIG_32BIT) && strncasecmp(isa, "rv32ima", 7))
> +		return -ENODEV;
> +
> +	if (IS_ENABLED(CONFIG_64BIT) && strncasecmp(isa, "rv64ima", 7))
>  		return -ENODEV;
> -	}
>  
>  	return 0;
>  }

What about reducing the compare string to "rv32" and "rv64" only?

Linux kernel is able to run on RV32IA or RV64IA CPUs theoretically as
someone did before[1]. In this case, M extension is not required here.
Although it's an invalid DT for now, reducing the compare string will
make the porting easier. And M extension does not provide additional
ISA-level registers that need the kernel to care about during context
switch.

In my opinion, we should concern about if someday linux will support
RV32E or RV64E. It's not necessary to validate if "ima" presents here
for better forward compatibility.

[1] https://www.facebook.com/groups/riscv.tw/posts/1611033709104137/

> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 3ae456413f79..a79c5c52a174 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -130,12 +130,12 @@ void __init riscv_fill_hwcap(void)
>  			continue;
>  		}
>  
> -		if (IS_ENABLED(CONFIG_32BIT) && strncasecmp(isa, "rv32", 4))
> -			continue;
> -
> -		if (IS_ENABLED(CONFIG_64BIT) && strncasecmp(isa, "rv64", 4))
> -			continue;
> -
> +		/*
> +		 * For all possible cpus, we have already validated in
> +		 * the boot process that they at least contain "rv" and
> +		 * whichever of "32"/"64" this kernel supports, and so this
> +		 * section can be skipped.
> +		 */
>  		isa += 4;
>  
>  		bitmap_zero(this_isa, RISCV_ISA_EXT_MAX);

Looks good to me.

Thanks,
Yangyu Chen


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

Re: [PATCH v1 4/7] RISC-V: validate riscv,isa at boot, not during ISA string parsing

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 2612 bytes --]

On Fri, May 05, 2023 at 08:40:22PM +0800, Yangyu Chen wrote:
> On 5/5/23 2:14 AM, Conor Dooley wrote:
> > From: Conor Dooley <conor.dooley@microchip.com>
> >
> > Since riscv_fill_hwcap() now only iterates over possible cpus, the
> > basic validation of whether riscv,isa contains "rv<width>" can be moved
> > to riscv_early_of_processor_hartid().
> >
> > Further, "ima" support is required by the kernel, so reject any CPU not
> > fitting the bill.
> >
> > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> > ---
> >  arch/riscv/kernel/cpu.c        |  8 +++++---
> >  arch/riscv/kernel/cpufeature.c | 12 ++++++------
> >  2 files changed, 11 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> > index 7030a5004f8e..b0c3ec0f2f5b 100644
> > --- a/arch/riscv/kernel/cpu.c
> > +++ b/arch/riscv/kernel/cpu.c
> > @@ -63,10 +63,12 @@ int riscv_early_of_processor_hartid(struct device_node *node, unsigned long *har
> >  		pr_warn("CPU with hartid=%lu has no \"riscv,isa\" property\n", *hart);
> >  		return -ENODEV;
> >  	}
> > -	if (tolower(isa[0]) != 'r' || tolower(isa[1]) != 'v') {
> > -		pr_warn("CPU with hartid=%lu has an invalid ISA of \"%s\"\n", *hart, isa);
> > +
> > +	if (IS_ENABLED(CONFIG_32BIT) && strncasecmp(isa, "rv32ima", 7))
> > +		return -ENODEV;
> > +
> > +	if (IS_ENABLED(CONFIG_64BIT) && strncasecmp(isa, "rv64ima", 7))
> >  		return -ENODEV;
> > -	}
> >  
> >  	return 0;
> >  }
> 
> What about reducing the compare string to "rv32" and "rv64" only?

I explicitly wanted the "ima" to rule out harts that might've been
left enabled but we cannot run on.

> Linux kernel is able to run on RV32IA or RV64IA CPUs theoretically as
> someone did before[1]. In this case, M extension is not required here.
> Although it's an invalid DT for now, reducing the compare string will
> make the porting easier. And M extension does not provide additional
> ISA-level registers that need the kernel to care about during context
> switch.
> 
> In my opinion, we should concern about if someday linux will support
> RV32E or RV64E. It's not necessary to validate if "ima" presents here
> for better forward compatibility.
> 
> [1] https://www.facebook.com/groups/riscv.tw/posts/1611033709104137/

I had a look, and as expected, this requires a makefile change to remove
the M extension from -march.
If that change ever lands in mainline the check here could be modified
too, but otherwise I don't think we need to be concerned about what some
out of tree code is doing.

Cheers,
Conor.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

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