[PATCH v1 0/9] RISC-V: Probe DT extension support using riscv,isa-extensions & riscv,isa-base

[PATCH v1 0/9] RISC-V: Probe DT extension support using riscv,isa-extensions & riscv,isa-base

[Conor Dooley]

Hey,

Based on my latest iteration of deprecating riscv,isa [1], here's an
implementation of the new properties for Linux. The first few patches,
up to "RISC-V: split riscv_fill_hwcap() in 3", are all prep work that
further tames some of the extension related code, on top of my already
applied series that cleans up the ISA string parser.
Perhaps "RISC-V: shunt isa_ext_arr to cpufeature.c" is a bit gratuitous,
but I figured a bit of coalescing of extension related data structures
would be a good idea. I certainly would not be against putting this
stuff in hwcap.h instead.

Note that riscv,isa will still be used in the absence of the new
properties:
	if (!acpi_disabled) {
		riscv_fill_hwcap_from_isa_string(isa2hwcap);
	} else {
		int ret = riscv_fill_hwcap_new(isa2hwcap);
		if (ret) {
			pr_info("Falling back to deprecated \"riscv,isa\"\n");
			riscv_fill_hwcap_from_isa_string(isa2hwcap);
		}
	}

Also, I could not come up with a good name for the new function,
suggestions welcome on that front for sure.

Cheers,
Conor.

As a side note, I tried some macro fiddling to remove the need for a
\#define for each extension that ends up conflicting a bunch between
different people, but didn't come up with anything I was happy with that
worked. I'll keep tinkering with that.

[1] https://lore.kernel.org/all/20230626-unmarked-atom-70b4d624a386@wendy/

CC: Rob Herring <robh+dt@kernel.org>
CC: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
CC: Paul Walmsley <paul.walmsley@sifive.com>
CC: Palmer Dabbelt <palmer@dabbelt.com>
CC: Albert Ou <aou@eecs.berkeley.edu>
CC: Andrew Jones <ajones@ventanamicro.com>
CC: Heiko Stuebner <heiko.stuebner@vrull.eu>
CC: Evan Green <evan@rivosinc.com>
CC: Sunil V L <sunilvl@ventanamicro.com>
CC: linux-riscv@lists.infradead.org
CC: devicetree@vger.kernel.org
CC: linux-kernel@vger.kernel.org

Conor Dooley (8):
  RISC-V: drop a needless check in print_isa_ext()
  RISC-V: shunt isa_ext_arr to cpufeature.c
  RISC-V: repurpose riscv_isa_ext array in riscv_fill_hwcap()
  RISC-V: add missing single letter extension definitions
  RISC-V: add single letter extensions to riscv_isa_ext
  RISC-V: split riscv_fill_hwcap() in 3
  RISC-V: enable extension detection from new properties
  RISC-V: try new extension properties in of_early_processor_hartid()

Heiko Stuebner (1):
  RISC-V: don't parse dt/acpi isa string to get rv32/rv64

 arch/riscv/include/asm/hwcap.h |  16 +-
 arch/riscv/kernel/cpu.c        | 149 +++-------
 arch/riscv/kernel/cpufeature.c | 508 +++++++++++++++++++++------------
 3 files changed, 379 insertions(+), 294 deletions(-)

-- 
2.40.1

[PATCH v1 1/9] RISC-V: don't parse dt/acpi isa string to get rv32/rv64

[Conor Dooley]

From: Heiko Stuebner <heiko.stuebner@vrull.eu>

When filling hwcap the kernel already expects the isa string to start with
rv32 if CONFIG_32BIT and rv64 if CONFIG_64BIT.

So when recreating the runtime isa-string we can also just go the other way
to get the correct starting point for it.

Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
Co-developed-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 arch/riscv/kernel/cpu.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
index a2fc952318e9..742bb42e7e86 100644
--- a/arch/riscv/kernel/cpu.c
+++ b/arch/riscv/kernel/cpu.c
@@ -253,13 +253,16 @@ static void print_isa_ext(struct seq_file *f)
  */
 static const char base_riscv_exts[13] = "imafdqcbkjpvh";
 
-static void print_isa(struct seq_file *f, const char *isa)
+static void print_isa(struct seq_file *f)
 {
 	int i;
 
 	seq_puts(f, "isa\t\t: ");
-	/* Print the rv[64/32] part */
-	seq_write(f, isa, 4);
+	if (IS_ENABLED(CONFIG_32BIT))
+		seq_write(f, "rv32", 4);
+	else
+		seq_write(f, "rv64", 4);
+
 	for (i = 0; i < sizeof(base_riscv_exts); i++) {
 		if (__riscv_isa_extension_available(NULL, base_riscv_exts[i] - 'a'))
 			/* Print only enabled the base ISA extensions */
@@ -316,15 +319,14 @@ static int c_show(struct seq_file *m, void *v)
 	unsigned long cpu_id = (unsigned long)v - 1;
 	struct riscv_cpuinfo *ci = per_cpu_ptr(&riscv_cpuinfo, cpu_id);
 	struct device_node *node;
-	const char *compat, *isa;
+	const char *compat;
 
 	seq_printf(m, "processor\t: %lu\n", cpu_id);
 	seq_printf(m, "hart\t\t: %lu\n", cpuid_to_hartid_map(cpu_id));
+	print_isa(m);
 
 	if (acpi_disabled) {
 		node = of_get_cpu_node(cpu_id, NULL);
-		if (!of_property_read_string(node, "riscv,isa", &isa))
-			print_isa(m, isa);
 
 		print_mmu(m);
 		if (!of_property_read_string(node, "compatible", &compat) &&
@@ -333,8 +335,6 @@ static int c_show(struct seq_file *m, void *v)
 
 		of_node_put(node);
 	} else {
-		if (!acpi_get_riscv_isa(NULL, cpu_id, &isa))
-			print_isa(m, isa);
 
 		print_mmu(m);
 	}
-- 
2.40.1

[PATCH v1 2/9] RISC-V: drop a needless check in print_isa_ext()

[Conor Dooley]

isa_ext_arr cannot be empty, as some of the extensions within it are
always built into the kernel.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 arch/riscv/kernel/cpu.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
index 742bb42e7e86..01f7e5c62997 100644
--- a/arch/riscv/kernel/cpu.c
+++ b/arch/riscv/kernel/cpu.c
@@ -233,10 +233,6 @@ static void print_isa_ext(struct seq_file *f)
 
 	arr_sz = ARRAY_SIZE(isa_ext_arr) - 1;
 
-	/* No extension support available */
-	if (arr_sz <= 0)
-		return;
-
 	for (i = 0; i <= arr_sz; i++) {
 		edata = &isa_ext_arr[i];
 		if (!__riscv_isa_extension_available(NULL, edata->isa_ext_id))
-- 
2.40.1

[PATCH v1 3/9] RISC-V: shunt isa_ext_arr to cpufeature.c

[Conor Dooley]

To facilitate using one struct to define extensions, rather than having
several, shunt isa_ext_arr to cpufeature.c, where it will be used for
probing extension presence also.
As that scope of the array as widened, prefix it with riscv & drop the
type from the variable name.

Since the new array is const, print_isa() needs a wee bit of cleanup to
avoid complaints about losing the const qualifier.

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

diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index f041bfa7f6a0..7a57e6109aef 100644
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -76,6 +76,9 @@ struct riscv_isa_ext_data {
 	unsigned int isa_ext_id;
 };
 
+extern const struct riscv_isa_ext_data riscv_isa_ext[];
+extern const size_t riscv_isa_ext_count;
+
 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 01f7e5c62997..61fb92e7d524 100644
--- a/arch/riscv/kernel/cpu.c
+++ b/arch/riscv/kernel/cpu.c
@@ -160,81 +160,10 @@ arch_initcall(riscv_cpuinfo_init);
 
 #ifdef CONFIG_PROC_FS
 
-#define __RISCV_ISA_EXT_DATA(UPROP, EXTID) \
-	{							\
-		.uprop = #UPROP,				\
-		.isa_ext_id = EXTID,				\
-	}
-
-/*
- * The canonical order of ISA extension names in the ISA string is defined in
- * chapter 27 of the unprivileged specification.
- *
- * Ordinarily, for in-kernel data structures, this order is unimportant but
- * isa_ext_arr defines the order of the ISA string in /proc/cpuinfo.
- *
- * The specification uses vague wording, such as should, when it comes to
- * ordering, so for our purposes the following rules apply:
- *
- * 1. All multi-letter extensions must be separated from other extensions by an
- *    underscore.
- *
- * 2. Additional standard extensions (starting with 'Z') must be sorted after
- *    single-letter extensions and before any higher-privileged extensions.
-
- * 3. The first letter following the 'Z' conventionally indicates the most
- *    closely related alphabetical extension category, IMAFDQLCBKJTPVH.
- *    If multiple 'Z' extensions are named, they must be ordered first by
- *    category, then alphabetically within a category.
- *
- * 3. Standard supervisor-level extensions (starting with 'S') must be listed
- *    after standard unprivileged extensions.  If multiple supervisor-level
- *    extensions are listed, they must be ordered alphabetically.
- *
- * 4. Standard machine-level extensions (starting with 'Zxm') must be listed
- *    after any lower-privileged, standard extensions.  If multiple
- *    machine-level extensions are listed, they must be ordered
- *    alphabetically.
- *
- * 5. Non-standard extensions (starting with 'X') must be listed after all
- *    standard extensions. If multiple non-standard extensions are listed, they
- *    must be ordered alphabetically.
- *
- * An example string following the order is:
- *    rv64imadc_zifoo_zigoo_zafoo_sbar_scar_zxmbaz_xqux_xrux
- *
- * 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(zicbom, RISCV_ISA_EXT_ZICBOM),
-	__RISCV_ISA_EXT_DATA(zicboz, RISCV_ISA_EXT_ZICBOZ),
-	__RISCV_ISA_EXT_DATA(zicntr, RISCV_ISA_EXT_ZICNTR),
-	__RISCV_ISA_EXT_DATA(zicsr, RISCV_ISA_EXT_ZICSR),
-	__RISCV_ISA_EXT_DATA(zifencei, RISCV_ISA_EXT_ZIFENCEI),
-	__RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
-	__RISCV_ISA_EXT_DATA(zihpm, RISCV_ISA_EXT_ZIHPM),
-	__RISCV_ISA_EXT_DATA(zba, RISCV_ISA_EXT_ZBA),
-	__RISCV_ISA_EXT_DATA(zbb, RISCV_ISA_EXT_ZBB),
-	__RISCV_ISA_EXT_DATA(zbs, RISCV_ISA_EXT_ZBS),
-	__RISCV_ISA_EXT_DATA(smaia, RISCV_ISA_EXT_SMAIA),
-	__RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
-	__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
-	__RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),
-	__RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL),
-	__RISCV_ISA_EXT_DATA(svnapot, RISCV_ISA_EXT_SVNAPOT),
-	__RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT),
-	__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;
-
-	for (i = 0; i <= arr_sz; i++) {
-		edata = &isa_ext_arr[i];
+	for (int i = 0; i < riscv_isa_ext_count; i++) {
+		const struct riscv_isa_ext_data *edata = &riscv_isa_ext[i];
 		if (!__riscv_isa_extension_available(NULL, edata->isa_ext_id))
 			continue;
 		seq_printf(f, "_%s", edata->uprop);
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index bdcf460ea53d..f0ae310006de 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -99,6 +99,74 @@ static bool riscv_isa_extension_check(int id)
 	return true;
 }
 
+#define __RISCV_ISA_EXT_DATA(UPROP, EXTID) \
+	{							\
+		.uprop = #UPROP,				\
+		.isa_ext_id = EXTID,				\
+	}
+
+/*
+ * The canonical order of ISA extension names in the ISA string is defined in
+ * chapter 27 of the unprivileged specification.
+ *
+ * Ordinarily, for in-kernel data structures, this order is unimportant but
+ * isa_ext_arr defines the order of the ISA string in /proc/cpuinfo.
+ *
+ * The specification uses vague wording, such as should, when it comes to
+ * ordering, so for our purposes the following rules apply:
+ *
+ * 1. All multi-letter extensions must be separated from other extensions by an
+ *    underscore.
+ *
+ * 2. Additional standard extensions (starting with 'Z') must be sorted after
+ *    single-letter extensions and before any higher-privileged extensions.
+ *
+ * 3. The first letter following the 'Z' conventionally indicates the most
+ *    closely related alphabetical extension category, IMAFDQLCBKJTPVH.
+ *    If multiple 'Z' extensions are named, they must be ordered first by
+ *    category, then alphabetically within a category.
+ *
+ * 3. Standard supervisor-level extensions (starting with 'S') must be listed
+ *    after standard unprivileged extensions.  If multiple supervisor-level
+ *    extensions are listed, they must be ordered alphabetically.
+ *
+ * 4. Standard machine-level extensions (starting with 'Zxm') must be listed
+ *    after any lower-privileged, standard extensions.  If multiple
+ *    machine-level extensions are listed, they must be ordered
+ *    alphabetically.
+ *
+ * 5. Non-standard extensions (starting with 'X') must be listed after all
+ *    standard extensions. If multiple non-standard extensions are listed, they
+ *    must be ordered alphabetically.
+ *
+ * An example string following the order is:
+ *    rv64imadc_zifoo_zigoo_zafoo_sbar_scar_zxmbaz_xqux_xrux
+ *
+ * New entries to this struct should follow the ordering rules described above.
+ */
+const struct riscv_isa_ext_data riscv_isa_ext[] = {
+	__RISCV_ISA_EXT_DATA(zicbom, RISCV_ISA_EXT_ZICBOM),
+	__RISCV_ISA_EXT_DATA(zicboz, RISCV_ISA_EXT_ZICBOZ),
+	__RISCV_ISA_EXT_DATA(zicntr, RISCV_ISA_EXT_ZICNTR),
+	__RISCV_ISA_EXT_DATA(zicsr, RISCV_ISA_EXT_ZICSR),
+	__RISCV_ISA_EXT_DATA(zifencei, RISCV_ISA_EXT_ZIFENCEI),
+	__RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
+	__RISCV_ISA_EXT_DATA(zihpm, RISCV_ISA_EXT_ZIHPM),
+	__RISCV_ISA_EXT_DATA(zba, RISCV_ISA_EXT_ZBA),
+	__RISCV_ISA_EXT_DATA(zbb, RISCV_ISA_EXT_ZBB),
+	__RISCV_ISA_EXT_DATA(zbs, RISCV_ISA_EXT_ZBS),
+	__RISCV_ISA_EXT_DATA(smaia, RISCV_ISA_EXT_SMAIA),
+	__RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
+	__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
+	__RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),
+	__RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL),
+	__RISCV_ISA_EXT_DATA(svnapot, RISCV_ISA_EXT_SVNAPOT),
+	__RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT),
+	__RISCV_ISA_EXT_DATA("", RISCV_ISA_EXT_MAX),
+};
+
+const size_t riscv_isa_ext_count = ARRAY_SIZE(riscv_isa_ext);
+
 void __init riscv_fill_hwcap(void)
 {
 	struct device_node *node;
-- 
2.40.1

[PATCH v1 4/9] RISC-V: repurpose riscv_isa_ext array in riscv_fill_hwcap()

[Conor Dooley]

In riscv_fill_hwcap() riscv_isa_ext array can be looped over, rather
than duplicating the list of extensions with individual
SET_ISA_EXT_MAP() usage. While at it, drop the statement-of-the-obvious
comments from the struct, rename uprop to something more suitable for
its new use & constify the members.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 arch/riscv/include/asm/hwcap.h |  6 ++----
 arch/riscv/kernel/cpu.c        |  5 +++--
 arch/riscv/kernel/cpufeature.c | 26 +++++++-------------------
 3 files changed, 12 insertions(+), 25 deletions(-)

diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index 7a57e6109aef..36f46dfd2b87 100644
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -70,10 +70,8 @@
 unsigned long riscv_get_elf_hwcap(void);
 
 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;
+	const unsigned int id;
+	const char *name;
 };
 
 extern const struct riscv_isa_ext_data riscv_isa_ext[];
diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
index 61fb92e7d524..beb8b16bbf87 100644
--- a/arch/riscv/kernel/cpu.c
+++ b/arch/riscv/kernel/cpu.c
@@ -164,9 +164,10 @@ static void print_isa_ext(struct seq_file *f)
 {
 	for (int i = 0; i < riscv_isa_ext_count; i++) {
 		const struct riscv_isa_ext_data *edata = &riscv_isa_ext[i];
-		if (!__riscv_isa_extension_available(NULL, edata->isa_ext_id))
+		if (!__riscv_isa_extension_available(NULL, edata->id))
 			continue;
-		seq_printf(f, "_%s", edata->uprop);
+
+		seq_printf(f, "_%s", edata->name);
 	}
 }
 
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index f0ae310006de..b5e23506c4f0 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -99,11 +99,10 @@ static bool riscv_isa_extension_check(int id)
 	return true;
 }
 
-#define __RISCV_ISA_EXT_DATA(UPROP, EXTID) \
-	{							\
-		.uprop = #UPROP,				\
-		.isa_ext_id = EXTID,				\
-	}
+#define __RISCV_ISA_EXT_DATA(_name, _id) {	\
+	.name = #_name,				\
+	.id = _id,				\
+}
 
 /*
  * The canonical order of ISA extension names in the ISA string is defined in
@@ -367,20 +366,9 @@ void __init riscv_fill_hwcap(void)
 					set_bit(nr, isainfo->isa);
 				}
 			} else {
-				/* sorted alphabetically */
-				SET_ISA_EXT_MAP("smaia", RISCV_ISA_EXT_SMAIA);
-				SET_ISA_EXT_MAP("ssaia", RISCV_ISA_EXT_SSAIA);
-				SET_ISA_EXT_MAP("sscofpmf", RISCV_ISA_EXT_SSCOFPMF);
-				SET_ISA_EXT_MAP("sstc", RISCV_ISA_EXT_SSTC);
-				SET_ISA_EXT_MAP("svinval", RISCV_ISA_EXT_SVINVAL);
-				SET_ISA_EXT_MAP("svnapot", RISCV_ISA_EXT_SVNAPOT);
-				SET_ISA_EXT_MAP("svpbmt", RISCV_ISA_EXT_SVPBMT);
-				SET_ISA_EXT_MAP("zba", RISCV_ISA_EXT_ZBA);
-				SET_ISA_EXT_MAP("zbb", RISCV_ISA_EXT_ZBB);
-				SET_ISA_EXT_MAP("zbs", RISCV_ISA_EXT_ZBS);
-				SET_ISA_EXT_MAP("zicbom", RISCV_ISA_EXT_ZICBOM);
-				SET_ISA_EXT_MAP("zicboz", RISCV_ISA_EXT_ZICBOZ);
-				SET_ISA_EXT_MAP("zihintpause", RISCV_ISA_EXT_ZIHINTPAUSE);
+				for (int i = 0; i < riscv_isa_ext_count; i++)
+					SET_ISA_EXT_MAP(riscv_isa_ext[i].name,
+							riscv_isa_ext[i].id);
 			}
 #undef SET_ISA_EXT_MAP
 		}
-- 
2.40.1

[PATCH v1 5/9] RISC-V: add missing single letter extension definitions

[Conor Dooley]

To facilitate adding single letter extensions to riscv_isa_ext, add
definitions for the extensions present in base_riscv_exts that do not
already have them.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 arch/riscv/include/asm/hwcap.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index 36f46dfd2b87..a35bee219dd7 100644
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -14,12 +14,17 @@
 #include <uapi/asm/hwcap.h>
 
 #define RISCV_ISA_EXT_a		('a' - 'a')
+#define RISCV_ISA_EXT_b		('b' - 'a')
 #define RISCV_ISA_EXT_c		('c' - 'a')
 #define RISCV_ISA_EXT_d		('d' - 'a')
 #define RISCV_ISA_EXT_f		('f' - 'a')
 #define RISCV_ISA_EXT_h		('h' - 'a')
 #define RISCV_ISA_EXT_i		('i' - 'a')
+#define RISCV_ISA_EXT_j		('j' - 'a')
+#define RISCV_ISA_EXT_k		('k' - 'a')
 #define RISCV_ISA_EXT_m		('m' - 'a')
+#define RISCV_ISA_EXT_p		('p' - 'a')
+#define RISCV_ISA_EXT_q		('q' - 'a')
 #define RISCV_ISA_EXT_s		('s' - 'a')
 #define RISCV_ISA_EXT_u		('u' - 'a')
 #define RISCV_ISA_EXT_v		('v' - 'a')
-- 
2.40.1

[PATCH v1 6/9] RISC-V: add single letter extensions to riscv_isa_ext

[Conor Dooley]

So that riscv_fill_hwcap() can use riscv_isa_ext to probe for single
letter extensions, add them to it. riscv_isa_ext_data grows a new
member, signifying whether an extension is multi-letter & thus requiring
special handling.
As a result, what gets spat out in /proc/cpuinfo will become borked, as
single letter extensions will be printed as part of the base extensions
and while printing from riscv_isa_arr. Take the opportunity to unify the
printing of the isa string, using the new member of riscv_isa_ext_data
in the process.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 arch/riscv/include/asm/hwcap.h |  1 +
 arch/riscv/kernel/cpu.c        | 36 ++++++----------------
 arch/riscv/kernel/cpufeature.c | 56 +++++++++++++++++++++-------------
 3 files changed, 46 insertions(+), 47 deletions(-)

diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index a35bee219dd7..6ad896dc4342 100644
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -77,6 +77,7 @@ unsigned long riscv_get_elf_hwcap(void);
 struct riscv_isa_ext_data {
 	const unsigned int id;
 	const char *name;
+	const bool multi_letter;
 };
 
 extern const struct riscv_isa_ext_data riscv_isa_ext[];
diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
index beb8b16bbf87..046d9d3dac16 100644
--- a/arch/riscv/kernel/cpu.c
+++ b/arch/riscv/kernel/cpu.c
@@ -160,41 +160,25 @@ arch_initcall(riscv_cpuinfo_init);
 
 #ifdef CONFIG_PROC_FS
 
-static void print_isa_ext(struct seq_file *f)
-{
-	for (int i = 0; i < riscv_isa_ext_count; i++) {
-		const struct riscv_isa_ext_data *edata = &riscv_isa_ext[i];
-		if (!__riscv_isa_extension_available(NULL, edata->id))
-			continue;
-
-		seq_printf(f, "_%s", edata->name);
-	}
-}
-
-/*
- * These are the only valid base (single letter) ISA extensions as per the spec.
- * It also specifies the canonical order in which it appears in the spec.
- * Some of the extension may just be a place holder for now (B, K, P, J).
- * This should be updated once corresponding extensions are ratified.
- */
-static const char base_riscv_exts[13] = "imafdqcbkjpvh";
-
 static void print_isa(struct seq_file *f)
 {
-	int i;
-
 	seq_puts(f, "isa\t\t: ");
+
 	if (IS_ENABLED(CONFIG_32BIT))
 		seq_write(f, "rv32", 4);
 	else
 		seq_write(f, "rv64", 4);
 
-	for (i = 0; i < sizeof(base_riscv_exts); i++) {
-		if (__riscv_isa_extension_available(NULL, base_riscv_exts[i] - 'a'))
-			/* Print only enabled the base ISA extensions */
-			seq_write(f, &base_riscv_exts[i], 1);
+	for (int i = 0; i < riscv_isa_ext_count; i++) {
+		if (!__riscv_isa_extension_available(NULL, riscv_isa_ext[i].id))
+			continue;
+
+		if (riscv_isa_ext[i].multi_letter)
+			seq_puts(f, "_");
+
+		seq_printf(f, "%s", riscv_isa_ext[i].name);
 	}
-	print_isa_ext(f);
+
 	seq_puts(f, "\n");
 }
 
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index b5e23506c4f0..5405d8a58537 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -99,9 +99,10 @@ static bool riscv_isa_extension_check(int id)
 	return true;
 }
 
-#define __RISCV_ISA_EXT_DATA(_name, _id) {	\
-	.name = #_name,				\
-	.id = _id,				\
+#define __RISCV_ISA_EXT_DATA(_name, _id, _multi) {	\
+	.name = #_name,					\
+	.id = _id,					\
+	.multi_letter = _multi,				\
 }
 
 /*
@@ -144,24 +145,37 @@ static bool riscv_isa_extension_check(int id)
  * New entries to this struct should follow the ordering rules described above.
  */
 const struct riscv_isa_ext_data riscv_isa_ext[] = {
-	__RISCV_ISA_EXT_DATA(zicbom, RISCV_ISA_EXT_ZICBOM),
-	__RISCV_ISA_EXT_DATA(zicboz, RISCV_ISA_EXT_ZICBOZ),
-	__RISCV_ISA_EXT_DATA(zicntr, RISCV_ISA_EXT_ZICNTR),
-	__RISCV_ISA_EXT_DATA(zicsr, RISCV_ISA_EXT_ZICSR),
-	__RISCV_ISA_EXT_DATA(zifencei, RISCV_ISA_EXT_ZIFENCEI),
-	__RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
-	__RISCV_ISA_EXT_DATA(zihpm, RISCV_ISA_EXT_ZIHPM),
-	__RISCV_ISA_EXT_DATA(zba, RISCV_ISA_EXT_ZBA),
-	__RISCV_ISA_EXT_DATA(zbb, RISCV_ISA_EXT_ZBB),
-	__RISCV_ISA_EXT_DATA(zbs, RISCV_ISA_EXT_ZBS),
-	__RISCV_ISA_EXT_DATA(smaia, RISCV_ISA_EXT_SMAIA),
-	__RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
-	__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
-	__RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),
-	__RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL),
-	__RISCV_ISA_EXT_DATA(svnapot, RISCV_ISA_EXT_SVNAPOT),
-	__RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT),
-	__RISCV_ISA_EXT_DATA("", RISCV_ISA_EXT_MAX),
+	__RISCV_ISA_EXT_DATA(i, RISCV_ISA_EXT_i, false),
+	__RISCV_ISA_EXT_DATA(m, RISCV_ISA_EXT_m, false),
+	__RISCV_ISA_EXT_DATA(a, RISCV_ISA_EXT_a, false),
+	__RISCV_ISA_EXT_DATA(f, RISCV_ISA_EXT_f, false),
+	__RISCV_ISA_EXT_DATA(d, RISCV_ISA_EXT_d, false),
+	__RISCV_ISA_EXT_DATA(q, RISCV_ISA_EXT_q, false),
+	__RISCV_ISA_EXT_DATA(c, RISCV_ISA_EXT_c, false),
+	__RISCV_ISA_EXT_DATA(b, RISCV_ISA_EXT_b, false),
+	__RISCV_ISA_EXT_DATA(k, RISCV_ISA_EXT_k, false),
+	__RISCV_ISA_EXT_DATA(j, RISCV_ISA_EXT_j, false),
+	__RISCV_ISA_EXT_DATA(p, RISCV_ISA_EXT_p, false),
+	__RISCV_ISA_EXT_DATA(v, RISCV_ISA_EXT_v, false),
+	__RISCV_ISA_EXT_DATA(h, RISCV_ISA_EXT_h, false),
+	__RISCV_ISA_EXT_DATA(zicbom, RISCV_ISA_EXT_ZICBOM, true),
+	__RISCV_ISA_EXT_DATA(zicboz, RISCV_ISA_EXT_ZICBOZ, true),
+	__RISCV_ISA_EXT_DATA(zicntr, RISCV_ISA_EXT_ZICNTR, true),
+	__RISCV_ISA_EXT_DATA(zicsr, RISCV_ISA_EXT_ZICSR, true),
+	__RISCV_ISA_EXT_DATA(zifencei, RISCV_ISA_EXT_ZIFENCEI, true),
+	__RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE, true),
+	__RISCV_ISA_EXT_DATA(zihpm, RISCV_ISA_EXT_ZIHPM, true),
+	__RISCV_ISA_EXT_DATA(zba, RISCV_ISA_EXT_ZBA, true),
+	__RISCV_ISA_EXT_DATA(zbb, RISCV_ISA_EXT_ZBB, true),
+	__RISCV_ISA_EXT_DATA(zbs, RISCV_ISA_EXT_ZBS, true),
+	__RISCV_ISA_EXT_DATA(smaia, RISCV_ISA_EXT_SMAIA, true),
+	__RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA, true),
+	__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF, true),
+	__RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC, true),
+	__RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL, true),
+	__RISCV_ISA_EXT_DATA(svnapot, RISCV_ISA_EXT_SVNAPOT, true),
+	__RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT, true),
+	__RISCV_ISA_EXT_DATA("", RISCV_ISA_EXT_MAX, true),
 };
 
 const size_t riscv_isa_ext_count = ARRAY_SIZE(riscv_isa_ext);
-- 
2.40.1

[PATCH v1 7/9] RISC-V: split riscv_fill_hwcap() in 3

[Conor Dooley]

Before adding more complexity to it, split riscv_fill_hwcap() into 3
distinct sections:
- riscv_fill_hwcap() still is the top level function, into which the
  additional complexity will be added.
- riscv_fill_hwcap_from_isa_string() handles getting the information
  from the riscv,isa/ACPI equivalent across harts & the various quirks
  there
- riscv_parse_isa_string() does what it says on the tin.

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

diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 5405d8a58537..366477ba1eea 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -180,29 +180,172 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
 
 const size_t riscv_isa_ext_count = ARRAY_SIZE(riscv_isa_ext);
 
-void __init riscv_fill_hwcap(void)
+static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct riscv_isainfo *isainfo,
+					  unsigned long *isa2hwcap, const char *isa)
+{
+	/*
+	 * 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;
+
+	while (*isa) {
+		const char *ext = isa++;
+		const char *ext_end = isa;
+		bool ext_long = false, ext_err = false;
+
+		switch (*ext) {
+		case 's':
+			/*
+			 * Workaround for invalid single-letter 's' & 'u'(QEMU).
+			 * No need to set the bit in riscv_isa as 's' & 'u' are
+			 * not valid ISA extensions. It works until multi-letter
+			 * extension starting with "Su" appears.
+			 */
+			if (ext[-1] != '_' && ext[1] == 'u') {
+				++isa;
+				ext_err = true;
+				break;
+			}
+			fallthrough;
+		case 'S':
+		case 'x':
+		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 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;
+			for (; *isa && *isa != '_'; ++isa)
+				if (unlikely(!isalnum(*isa)))
+					ext_err = true;
+
+			ext_end = isa;
+			if (unlikely(ext_err))
+				break;
+
+			if (!isdigit(ext_end[-1]))
+				break;
+
+			while (isdigit(*--ext_end))
+				;
+
+			if (tolower(ext_end[0]) != 'p' || !isdigit(ext_end[-1])) {
+				++ext_end;
+				break;
+			}
+
+			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;
+			}
+
+			if (!isdigit(*isa))
+				break;
+
+			while (isdigit(*++isa))
+				;
+
+			if (tolower(*isa) != 'p')
+				break;
+
+			if (!isdigit(*++isa)) {
+				--isa;
+				break;
+			}
+
+			while (isdigit(*++isa))
+				;
+
+			break;
+		}
+
+		/*
+		 * The parser expects that at the start of an iteration isa points to the
+		 * 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;
+
+#define SET_ISA_EXT_MAP(name, bit)						\
+		do {								\
+			if ((ext_end - ext == sizeof(name) - 1) &&		\
+			     !strncasecmp(ext, name, sizeof(name) - 1) &&	\
+			     riscv_isa_extension_check(bit))			\
+				set_bit(bit, isainfo->isa);			\
+		} while (false)							\
+
+		if (unlikely(ext_err))
+			continue;
+		if (!ext_long) {
+			int nr = tolower(*ext) - 'a';
+
+			if (riscv_isa_extension_check(nr)) {
+				*this_hwcap |= isa2hwcap[nr];
+				set_bit(nr, isainfo->isa);
+			}
+		} else {
+			for (int i = 0; i < riscv_isa_ext_count; i++)
+				SET_ISA_EXT_MAP(riscv_isa_ext[i].name,
+						riscv_isa_ext[i].id);
+		}
+#undef SET_ISA_EXT_MAP
+	}
+}
+
+static void __init riscv_fill_hwcap_from_isa_string(unsigned long *isa2hwcap)
 {
 	struct device_node *node;
 	const char *isa;
-	char print_str[NUM_ALPHA_EXTS + 1];
-	int i, j, rc;
-	unsigned long isa2hwcap[26] = {0};
+	int rc;
 	struct acpi_table_header *rhct;
 	acpi_status status;
 	unsigned int cpu;
 
-	isa2hwcap['i' - 'a'] = COMPAT_HWCAP_ISA_I;
-	isa2hwcap['m' - 'a'] = COMPAT_HWCAP_ISA_M;
-	isa2hwcap['a' - 'a'] = COMPAT_HWCAP_ISA_A;
-	isa2hwcap['f' - 'a'] = COMPAT_HWCAP_ISA_F;
-	isa2hwcap['d' - 'a'] = COMPAT_HWCAP_ISA_D;
-	isa2hwcap['c' - 'a'] = COMPAT_HWCAP_ISA_C;
-	isa2hwcap['v' - 'a'] = COMPAT_HWCAP_ISA_V;
-
-	elf_hwcap = 0;
-
-	bitmap_zero(riscv_isa, RISCV_ISA_EXT_MAX);
-
 	if (!acpi_disabled) {
 		status = acpi_get_table(ACPI_SIG_RHCT, 0, &rhct);
 		if (ACPI_FAILURE(status))
@@ -234,158 +377,7 @@ void __init riscv_fill_hwcap(void)
 			}
 		}
 
-		/*
-		 * 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;
-
-		while (*isa) {
-			const char *ext = isa++;
-			const char *ext_end = isa;
-			bool ext_long = false, ext_err = false;
-
-			switch (*ext) {
-			case 's':
-				/*
-				 * Workaround for invalid single-letter 's' & 'u'(QEMU).
-				 * No need to set the bit in riscv_isa as 's' & 'u' are
-				 * not valid ISA extensions. It works until multi-letter
-				 * extension starting with "Su" appears.
-				 */
-				if (ext[-1] != '_' && ext[1] == 'u') {
-					++isa;
-					ext_err = true;
-					break;
-				}
-				fallthrough;
-			case 'S':
-			case 'x':
-			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 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;
-				for (; *isa && *isa != '_'; ++isa)
-					if (unlikely(!isalnum(*isa)))
-						ext_err = true;
-
-				ext_end = isa;
-				if (unlikely(ext_err))
-					break;
-
-				if (!isdigit(ext_end[-1]))
-					break;
-
-				while (isdigit(*--ext_end))
-					;
-
-				if (tolower(ext_end[0]) != 'p' || !isdigit(ext_end[-1])) {
-					++ext_end;
-					break;
-				}
-
-				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;
-				}
-
-				if (!isdigit(*isa))
-					break;
-
-				while (isdigit(*++isa))
-					;
-
-				if (tolower(*isa) != 'p')
-					break;
-
-				if (!isdigit(*++isa)) {
-					--isa;
-					break;
-				}
-
-				while (isdigit(*++isa))
-					;
-
-				break;
-			}
-
-			/*
-			 * The parser expects that at the start of an iteration isa points to the
-			 * 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;
-
-#define SET_ISA_EXT_MAP(name, bit)							\
-			do {								\
-				if ((ext_end - ext == sizeof(name) - 1) &&		\
-				     !strncasecmp(ext, name, sizeof(name) - 1) &&	\
-				     riscv_isa_extension_check(bit))			\
-					set_bit(bit, isainfo->isa);			\
-			} while (false)							\
-
-			if (unlikely(ext_err))
-				continue;
-			if (!ext_long) {
-				int nr = tolower(*ext) - 'a';
-
-				if (riscv_isa_extension_check(nr)) {
-					this_hwcap |= isa2hwcap[nr];
-					set_bit(nr, isainfo->isa);
-				}
-			} else {
-				for (int i = 0; i < riscv_isa_ext_count; i++)
-					SET_ISA_EXT_MAP(riscv_isa_ext[i].name,
-							riscv_isa_ext[i].id);
-			}
-#undef SET_ISA_EXT_MAP
-		}
+		riscv_parse_isa_string(&this_hwcap, isainfo, isa2hwcap, isa);
 
 		/*
 		 * Linux requires the following extensions, so we may as well
@@ -422,6 +414,28 @@ void __init riscv_fill_hwcap(void)
 
 	if (!acpi_disabled && rhct)
 		acpi_put_table((struct acpi_table_header *)rhct);
+}
+
+void __init riscv_fill_hwcap(void)
+{
+	struct device_node *node;
+	const char *isa;
+	char print_str[NUM_ALPHA_EXTS + 1];
+	int i, j, rc;
+	unsigned long isa2hwcap[26] = {0};
+	struct acpi_table_header *rhct;
+	acpi_status status;
+	unsigned int cpu;
+
+	isa2hwcap['i' - 'a'] = COMPAT_HWCAP_ISA_I;
+	isa2hwcap['m' - 'a'] = COMPAT_HWCAP_ISA_M;
+	isa2hwcap['a' - 'a'] = COMPAT_HWCAP_ISA_A;
+	isa2hwcap['f' - 'a'] = COMPAT_HWCAP_ISA_F;
+	isa2hwcap['d' - 'a'] = COMPAT_HWCAP_ISA_D;
+	isa2hwcap['c' - 'a'] = COMPAT_HWCAP_ISA_C;
+	isa2hwcap['v' - 'a'] = COMPAT_HWCAP_ISA_V;
+
+	riscv_fill_hwcap_from_isa_string(isa2hwcap);
 
 	/* We don't support systems with F but without D, so mask those out
 	 * here. */
-- 
2.40.1

[PATCH v1 8/9] RISC-V: enable extension detection from new properties

[Conor Dooley]

Add support for parsing the new riscv,isa-extensions property in
riscv_fill_hwcap(), by means of a new "property" member of the
riscv_isa_ext_data struct. For now, this shadows the name of the
extension, however this may not be the case for all extensions.
For the sake of backwards compatibility, fall back to the old scheme
if the new properties are not detected. For now, just inform, rather
than warn, when that happens.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
Naming things is hard, didn't know what to call the new function...
---
 arch/riscv/include/asm/hwcap.h |  1 +
 arch/riscv/kernel/cpufeature.c | 80 ++++++++++++++++++++++++++++++----
 2 files changed, 72 insertions(+), 9 deletions(-)

diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index 6ad896dc4342..e7f235868aa2 100644
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -77,6 +77,7 @@ unsigned long riscv_get_elf_hwcap(void);
 struct riscv_isa_ext_data {
 	const unsigned int id;
 	const char *name;
+	const char *property;
 	const bool multi_letter;
 };
 
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 366477ba1eea..72eb757ad871 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -101,6 +101,7 @@ static bool riscv_isa_extension_check(int id)
 
 #define __RISCV_ISA_EXT_DATA(_name, _id, _multi) {	\
 	.name = #_name,					\
+	.property = #_name,				\
 	.id = _id,					\
 	.multi_letter = _multi,				\
 }
@@ -416,16 +417,66 @@ static void __init riscv_fill_hwcap_from_isa_string(unsigned long *isa2hwcap)
 		acpi_put_table((struct acpi_table_header *)rhct);
 }
 
+static int __init riscv_fill_hwcap_new(unsigned long *isa2hwcap)
+{
+	unsigned int cpu;
+
+	for_each_possible_cpu(cpu) {
+		unsigned long this_hwcap = 0;
+		struct device_node *cpu_node;
+		DECLARE_BITMAP(this_isa, RISCV_ISA_EXT_MAX);
+
+		cpu_node = of_cpu_device_node_get(cpu);
+		if (!cpu_node) {
+			pr_warn("Unable to find cpu node\n");
+			continue;
+		}
+
+		if (!of_property_present(cpu_node, "riscv,isa-extensions"))
+			continue;
+
+		for (int i = 0; i < riscv_isa_ext_count; i++) {
+			if (of_property_match_string(cpu_node, "riscv,isa-extensions",
+						     riscv_isa_ext[i].name) < 0)
+				continue;
+
+			if (!riscv_isa_extension_check(riscv_isa_ext[i].id))
+				continue;
+
+			if (!riscv_isa_ext[i].multi_letter)
+				this_hwcap |= isa2hwcap[riscv_isa_ext[i].id];
+
+			set_bit(riscv_isa_ext[i].id, this_isa);
+		}
+
+		of_node_put(cpu_node);
+
+		/*
+		 * All "okay" harts should have same isa. Set HWCAP based on
+		 * common capabilities of every "okay" hart, in case they don't.
+		 */
+		if (elf_hwcap)
+			elf_hwcap &= this_hwcap;
+		else
+			elf_hwcap = this_hwcap;
+
+		if (bitmap_empty(riscv_isa, RISCV_ISA_EXT_MAX))
+			bitmap_copy(riscv_isa, this_isa, RISCV_ISA_EXT_MAX);
+		else
+			bitmap_and(riscv_isa, riscv_isa, this_isa, RISCV_ISA_EXT_MAX);
+	}
+
+	if (bitmap_empty(riscv_isa, RISCV_ISA_EXT_MAX))
+		return -ENOENT;
+
+	return 0;
+}
+
 void __init riscv_fill_hwcap(void)
 {
-	struct device_node *node;
-	const char *isa;
 	char print_str[NUM_ALPHA_EXTS + 1];
-	int i, j, rc;
 	unsigned long isa2hwcap[26] = {0};
-	struct acpi_table_header *rhct;
-	acpi_status status;
-	unsigned int cpu;
+	int i, j;
 
 	isa2hwcap['i' - 'a'] = COMPAT_HWCAP_ISA_I;
 	isa2hwcap['m' - 'a'] = COMPAT_HWCAP_ISA_M;
@@ -435,10 +486,21 @@ void __init riscv_fill_hwcap(void)
 	isa2hwcap['c' - 'a'] = COMPAT_HWCAP_ISA_C;
 	isa2hwcap['v' - 'a'] = COMPAT_HWCAP_ISA_V;
 
-	riscv_fill_hwcap_from_isa_string(isa2hwcap);
+	if (!acpi_disabled) {
+		riscv_fill_hwcap_from_isa_string(isa2hwcap);
+	} else {
+		int ret = riscv_fill_hwcap_new(isa2hwcap);
 
-	/* We don't support systems with F but without D, so mask those out
-	 * here. */
+		if (ret) {
+			pr_info("Falling back to deprecated \"riscv,isa\"\n");
+			riscv_fill_hwcap_from_isa_string(isa2hwcap);
+		}
+	}
+
+	/*
+	 * We don't support systems with F but without D, so mask those out
+	 * here.
+	 */
 	if ((elf_hwcap & COMPAT_HWCAP_ISA_F) && !(elf_hwcap & COMPAT_HWCAP_ISA_D)) {
 		pr_info("This kernel does not support systems with F but not D\n");
 		elf_hwcap &= ~COMPAT_HWCAP_ISA_F;
-- 
2.40.1

[PATCH v1 9/9] RISC-V: try new extension properties in of_early_processor_hartid()

[Conor Dooley]

To fully deprecate the kernel's use of "riscv,isa",
of_early_processor_hartid() needs to first try using the new properties,
before falling back to "riscv,isa".

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 arch/riscv/kernel/cpu.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
index 046d9d3dac16..332574f27c95 100644
--- a/arch/riscv/kernel/cpu.c
+++ b/arch/riscv/kernel/cpu.c
@@ -61,8 +61,29 @@ int riscv_early_of_processor_hartid(struct device_node *node, unsigned long *har
 		return -ENODEV;
 	}
 
+	if (of_property_read_string(node, "riscv,isa-base", &isa))
+		goto old_interface;
+
+	if (IS_ENABLED(CONFIG_32BIT) && strncasecmp(isa, "rv32i", 5))
+		return -ENODEV;
+
+	if (IS_ENABLED(CONFIG_64BIT) && strncasecmp(isa, "rv64i", 5))
+		return -ENODEV;
+
+	if (!of_property_present(node, "riscv,isa-extensions"))
+		return -ENODEV;
+
+	if (of_property_match_string(node, "riscv,isa-extensions", "i") < 0 ||
+	    of_property_match_string(node, "riscv,isa-extensions", "m") < 0 ||
+	    of_property_match_string(node, "riscv,isa-extensions", "a") < 0)
+		return -ENODEV;
+
+	return 0;
+
+old_interface:
 	if (of_property_read_string(node, "riscv,isa", &isa)) {
-		pr_warn("CPU with hartid=%lu has no \"riscv,isa\" property\n", *hart);
+		pr_warn("CPU with hartid=%lu has no \"riscv,isa-base\" or \"riscv,isa\" property\n",
+			*hart);
 		return -ENODEV;
 	}
 
-- 
2.40.1

Re: [PATCH v1 1/9] RISC-V: don't parse dt/acpi isa string to get rv32/rv64

[Andrew Jones]

On Mon, Jun 26, 2023 at 12:19:39PM +0100, Conor Dooley wrote:
> From: Heiko Stuebner <heiko.stuebner@vrull.eu>
> 
> When filling hwcap the kernel already expects the isa string to start with
> rv32 if CONFIG_32BIT and rv64 if CONFIG_64BIT.
> 
> So when recreating the runtime isa-string we can also just go the other way
> to get the correct starting point for it.
> 
> Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
> Co-developed-by: Conor Dooley <conor.dooley@microchip.com>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  arch/riscv/kernel/cpu.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index a2fc952318e9..742bb42e7e86 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -253,13 +253,16 @@ static void print_isa_ext(struct seq_file *f)
>   */
>  static const char base_riscv_exts[13] = "imafdqcbkjpvh";
>  
> -static void print_isa(struct seq_file *f, const char *isa)
> +static void print_isa(struct seq_file *f)
>  {
>  	int i;
>  
>  	seq_puts(f, "isa\t\t: ");
> -	/* Print the rv[64/32] part */
> -	seq_write(f, isa, 4);
> +	if (IS_ENABLED(CONFIG_32BIT))
> +		seq_write(f, "rv32", 4);
> +	else
> +		seq_write(f, "rv64", 4);
> +
>  	for (i = 0; i < sizeof(base_riscv_exts); i++) {
>  		if (__riscv_isa_extension_available(NULL, base_riscv_exts[i] - 'a'))
>  			/* Print only enabled the base ISA extensions */
> @@ -316,15 +319,14 @@ static int c_show(struct seq_file *m, void *v)
>  	unsigned long cpu_id = (unsigned long)v - 1;
>  	struct riscv_cpuinfo *ci = per_cpu_ptr(&riscv_cpuinfo, cpu_id);
>  	struct device_node *node;
> -	const char *compat, *isa;
> +	const char *compat;
>  
>  	seq_printf(m, "processor\t: %lu\n", cpu_id);
>  	seq_printf(m, "hart\t\t: %lu\n", cpuid_to_hartid_map(cpu_id));
> +	print_isa(m);
>  
>  	if (acpi_disabled) {
>  		node = of_get_cpu_node(cpu_id, NULL);
> -		if (!of_property_read_string(node, "riscv,isa", &isa))
> -			print_isa(m, isa);
>  
>  		print_mmu(m);
>  		if (!of_property_read_string(node, "compatible", &compat) &&
> @@ -333,8 +335,6 @@ static int c_show(struct seq_file *m, void *v)
>  
>  		of_node_put(node);
>  	} else {
> -		if (!acpi_get_riscv_isa(NULL, cpu_id, &isa))
> -			print_isa(m, isa);
>  

Extra blank line here to remove. Actually the whole 'else' can be removed
because the print_mmu() call can be brought up above the
'if (acpi_disabled)'

>  		print_mmu(m);
>  	}
> -- 
> 2.40.1
>

Otherwise,

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

Thanks,
drew

Re: [PATCH v1 2/9] RISC-V: drop a needless check in print_isa_ext()

[Andrew Jones]

On Mon, Jun 26, 2023 at 12:19:40PM +0100, Conor Dooley wrote:
> isa_ext_arr cannot be empty, as some of the extensions within it are
> always built into the kernel.

This is only true since commit 07edc32779e3 ("RISC-V: always report
presence of extensions formerly part of the base ISA"), right? If
so, it might be nice to call that commit out in this commit message.

> 
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  arch/riscv/kernel/cpu.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index 742bb42e7e86..01f7e5c62997 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -233,10 +233,6 @@ static void print_isa_ext(struct seq_file *f)
>  
>  	arr_sz = ARRAY_SIZE(isa_ext_arr) - 1;
>  
> -	/* No extension support available */
> -	if (arr_sz <= 0)
> -		return;
> -
>  	for (i = 0; i <= arr_sz; i++) {
>  		edata = &isa_ext_arr[i];
>  		if (!__riscv_isa_extension_available(NULL, edata->isa_ext_id))
> -- 
> 2.40.1
>

Otherwise,

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

Thanks,
drew

Re: [PATCH v1 3/9] RISC-V: shunt isa_ext_arr to cpufeature.c

[Andrew Jones]

On Mon, Jun 26, 2023 at 12:19:41PM +0100, Conor Dooley wrote:
> To facilitate using one struct to define extensions, rather than having
> several, shunt isa_ext_arr to cpufeature.c, where it will be used for
> probing extension presence also.
> As that scope of the array as widened, prefix it with riscv & drop the
> type from the variable name.
> 
> Since the new array is const, print_isa() needs a wee bit of cleanup to
> avoid complaints about losing the const qualifier.
> 
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  arch/riscv/include/asm/hwcap.h |  3 ++
>  arch/riscv/kernel/cpu.c        | 75 +---------------------------------
>  arch/riscv/kernel/cpufeature.c | 68 ++++++++++++++++++++++++++++++
>  3 files changed, 73 insertions(+), 73 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> index f041bfa7f6a0..7a57e6109aef 100644
> --- a/arch/riscv/include/asm/hwcap.h
> +++ b/arch/riscv/include/asm/hwcap.h
> @@ -76,6 +76,9 @@ struct riscv_isa_ext_data {
>  	unsigned int isa_ext_id;
>  };
>  
> +extern const struct riscv_isa_ext_data riscv_isa_ext[];
> +extern const size_t riscv_isa_ext_count;
> +
>  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 01f7e5c62997..61fb92e7d524 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -160,81 +160,10 @@ arch_initcall(riscv_cpuinfo_init);
>  
>  #ifdef CONFIG_PROC_FS
>  
> -#define __RISCV_ISA_EXT_DATA(UPROP, EXTID) \
> -	{							\
> -		.uprop = #UPROP,				\
> -		.isa_ext_id = EXTID,				\
> -	}
> -
> -/*
> - * The canonical order of ISA extension names in the ISA string is defined in
> - * chapter 27 of the unprivileged specification.
> - *
> - * Ordinarily, for in-kernel data structures, this order is unimportant but
> - * isa_ext_arr defines the order of the ISA string in /proc/cpuinfo.
> - *
> - * The specification uses vague wording, such as should, when it comes to
> - * ordering, so for our purposes the following rules apply:
> - *
> - * 1. All multi-letter extensions must be separated from other extensions by an
> - *    underscore.
> - *
> - * 2. Additional standard extensions (starting with 'Z') must be sorted after
> - *    single-letter extensions and before any higher-privileged extensions.
> -
> - * 3. The first letter following the 'Z' conventionally indicates the most
> - *    closely related alphabetical extension category, IMAFDQLCBKJTPVH.
> - *    If multiple 'Z' extensions are named, they must be ordered first by
> - *    category, then alphabetically within a category.
> - *
> - * 3. Standard supervisor-level extensions (starting with 'S') must be listed
> - *    after standard unprivileged extensions.  If multiple supervisor-level
> - *    extensions are listed, they must be ordered alphabetically.
> - *
> - * 4. Standard machine-level extensions (starting with 'Zxm') must be listed
> - *    after any lower-privileged, standard extensions.  If multiple
> - *    machine-level extensions are listed, they must be ordered
> - *    alphabetically.
> - *
> - * 5. Non-standard extensions (starting with 'X') must be listed after all
> - *    standard extensions. If multiple non-standard extensions are listed, they
> - *    must be ordered alphabetically.
> - *
> - * An example string following the order is:
> - *    rv64imadc_zifoo_zigoo_zafoo_sbar_scar_zxmbaz_xqux_xrux
> - *
> - * 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(zicbom, RISCV_ISA_EXT_ZICBOM),
> -	__RISCV_ISA_EXT_DATA(zicboz, RISCV_ISA_EXT_ZICBOZ),
> -	__RISCV_ISA_EXT_DATA(zicntr, RISCV_ISA_EXT_ZICNTR),
> -	__RISCV_ISA_EXT_DATA(zicsr, RISCV_ISA_EXT_ZICSR),
> -	__RISCV_ISA_EXT_DATA(zifencei, RISCV_ISA_EXT_ZIFENCEI),
> -	__RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
> -	__RISCV_ISA_EXT_DATA(zihpm, RISCV_ISA_EXT_ZIHPM),
> -	__RISCV_ISA_EXT_DATA(zba, RISCV_ISA_EXT_ZBA),
> -	__RISCV_ISA_EXT_DATA(zbb, RISCV_ISA_EXT_ZBB),
> -	__RISCV_ISA_EXT_DATA(zbs, RISCV_ISA_EXT_ZBS),
> -	__RISCV_ISA_EXT_DATA(smaia, RISCV_ISA_EXT_SMAIA),
> -	__RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
> -	__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
> -	__RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),
> -	__RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL),
> -	__RISCV_ISA_EXT_DATA(svnapot, RISCV_ISA_EXT_SVNAPOT),
> -	__RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT),
> -	__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;
> -
> -	for (i = 0; i <= arr_sz; i++) {
> -		edata = &isa_ext_arr[i];
> +	for (int i = 0; i < riscv_isa_ext_count; i++) {
> +		const struct riscv_isa_ext_data *edata = &riscv_isa_ext[i];
>  		if (!__riscv_isa_extension_available(NULL, edata->isa_ext_id))
>  			continue;
>  		seq_printf(f, "_%s", edata->uprop);
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index bdcf460ea53d..f0ae310006de 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -99,6 +99,74 @@ static bool riscv_isa_extension_check(int id)
>  	return true;
>  }
>  
> +#define __RISCV_ISA_EXT_DATA(UPROP, EXTID) \
> +	{							\
> +		.uprop = #UPROP,				\
> +		.isa_ext_id = EXTID,				\
> +	}
> +
> +/*
> + * The canonical order of ISA extension names in the ISA string is defined in
> + * chapter 27 of the unprivileged specification.
> + *
> + * Ordinarily, for in-kernel data structures, this order is unimportant but
> + * isa_ext_arr defines the order of the ISA string in /proc/cpuinfo.
> + *
> + * The specification uses vague wording, such as should, when it comes to
> + * ordering, so for our purposes the following rules apply:
> + *
> + * 1. All multi-letter extensions must be separated from other extensions by an
> + *    underscore.
> + *
> + * 2. Additional standard extensions (starting with 'Z') must be sorted after
> + *    single-letter extensions and before any higher-privileged extensions.
> + *
> + * 3. The first letter following the 'Z' conventionally indicates the most
> + *    closely related alphabetical extension category, IMAFDQLCBKJTPVH.
> + *    If multiple 'Z' extensions are named, they must be ordered first by
> + *    category, then alphabetically within a category.
> + *
> + * 3. Standard supervisor-level extensions (starting with 'S') must be listed
> + *    after standard unprivileged extensions.  If multiple supervisor-level
> + *    extensions are listed, they must be ordered alphabetically.
> + *
> + * 4. Standard machine-level extensions (starting with 'Zxm') must be listed
> + *    after any lower-privileged, standard extensions.  If multiple
> + *    machine-level extensions are listed, they must be ordered
> + *    alphabetically.
> + *
> + * 5. Non-standard extensions (starting with 'X') must be listed after all
> + *    standard extensions. If multiple non-standard extensions are listed, they
> + *    must be ordered alphabetically.
> + *
> + * An example string following the order is:
> + *    rv64imadc_zifoo_zigoo_zafoo_sbar_scar_zxmbaz_xqux_xrux
> + *
> + * New entries to this struct should follow the ordering rules described above.
> + */
> +const struct riscv_isa_ext_data riscv_isa_ext[] = {
> +	__RISCV_ISA_EXT_DATA(zicbom, RISCV_ISA_EXT_ZICBOM),
> +	__RISCV_ISA_EXT_DATA(zicboz, RISCV_ISA_EXT_ZICBOZ),
> +	__RISCV_ISA_EXT_DATA(zicntr, RISCV_ISA_EXT_ZICNTR),
> +	__RISCV_ISA_EXT_DATA(zicsr, RISCV_ISA_EXT_ZICSR),
> +	__RISCV_ISA_EXT_DATA(zifencei, RISCV_ISA_EXT_ZIFENCEI),
> +	__RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
> +	__RISCV_ISA_EXT_DATA(zihpm, RISCV_ISA_EXT_ZIHPM),
> +	__RISCV_ISA_EXT_DATA(zba, RISCV_ISA_EXT_ZBA),
> +	__RISCV_ISA_EXT_DATA(zbb, RISCV_ISA_EXT_ZBB),
> +	__RISCV_ISA_EXT_DATA(zbs, RISCV_ISA_EXT_ZBS),
> +	__RISCV_ISA_EXT_DATA(smaia, RISCV_ISA_EXT_SMAIA),
> +	__RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
> +	__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
> +	__RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),
> +	__RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL),
> +	__RISCV_ISA_EXT_DATA(svnapot, RISCV_ISA_EXT_SVNAPOT),
> +	__RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT),
> +	__RISCV_ISA_EXT_DATA("", RISCV_ISA_EXT_MAX),

I think we can either drop this null entry or drop the count variable
below. My preference would be to drop the count variable, and always
loop to the null.

> +};
> +
> +const size_t riscv_isa_ext_count = ARRAY_SIZE(riscv_isa_ext);
> +
>  void __init riscv_fill_hwcap(void)
>  {
>  	struct device_node *node;
> -- 
> 2.40.1
>

Otherwise,

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

Thanks,
drew

Re: [PATCH v1 4/9] RISC-V: repurpose riscv_isa_ext array in riscv_fill_hwcap()

[Andrew Jones]

On Mon, Jun 26, 2023 at 12:19:42PM +0100, Conor Dooley wrote:
> In riscv_fill_hwcap() riscv_isa_ext array can be looped over, rather
> than duplicating the list of extensions with individual
> SET_ISA_EXT_MAP() usage. While at it, drop the statement-of-the-obvious
> comments from the struct, rename uprop to something more suitable for
> its new use & constify the members.
> 
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  arch/riscv/include/asm/hwcap.h |  6 ++----
>  arch/riscv/kernel/cpu.c        |  5 +++--
>  arch/riscv/kernel/cpufeature.c | 26 +++++++-------------------
>  3 files changed, 12 insertions(+), 25 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> index 7a57e6109aef..36f46dfd2b87 100644
> --- a/arch/riscv/include/asm/hwcap.h
> +++ b/arch/riscv/include/asm/hwcap.h
> @@ -70,10 +70,8 @@
>  unsigned long riscv_get_elf_hwcap(void);
>  
>  struct riscv_isa_ext_data {
> -	/* Name of the extension displayed to userspace via /proc/cpuinfo */
> -	char uprop[RISCV_ISA_EXT_NAME_LEN_MAX];

The RISCV_ISA_EXT_NAME_LEN_MAX define can now also be deleted.

> -	/* The logical ISA extension ID */
> -	unsigned int isa_ext_id;
> +	const unsigned int id;
> +	const char *name;
>  };
>  
>  extern const struct riscv_isa_ext_data riscv_isa_ext[];
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index 61fb92e7d524..beb8b16bbf87 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -164,9 +164,10 @@ static void print_isa_ext(struct seq_file *f)
>  {
>  	for (int i = 0; i < riscv_isa_ext_count; i++) {
>  		const struct riscv_isa_ext_data *edata = &riscv_isa_ext[i];
> -		if (!__riscv_isa_extension_available(NULL, edata->isa_ext_id))
> +		if (!__riscv_isa_extension_available(NULL, edata->id))
>  			continue;
> -		seq_printf(f, "_%s", edata->uprop);
> +
> +		seq_printf(f, "_%s", edata->name);
>  	}
>  }
>  
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index f0ae310006de..b5e23506c4f0 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -99,11 +99,10 @@ static bool riscv_isa_extension_check(int id)
>  	return true;
>  }
>  
> -#define __RISCV_ISA_EXT_DATA(UPROP, EXTID) \
> -	{							\
> -		.uprop = #UPROP,				\
> -		.isa_ext_id = EXTID,				\
> -	}
> +#define __RISCV_ISA_EXT_DATA(_name, _id) {	\
> +	.name = #_name,				\
> +	.id = _id,				\
> +}
>  
>  /*
>   * The canonical order of ISA extension names in the ISA string is defined in
> @@ -367,20 +366,9 @@ void __init riscv_fill_hwcap(void)
>  					set_bit(nr, isainfo->isa);
>  				}
>  			} else {
> -				/* sorted alphabetically */
> -				SET_ISA_EXT_MAP("smaia", RISCV_ISA_EXT_SMAIA);
> -				SET_ISA_EXT_MAP("ssaia", RISCV_ISA_EXT_SSAIA);
> -				SET_ISA_EXT_MAP("sscofpmf", RISCV_ISA_EXT_SSCOFPMF);
> -				SET_ISA_EXT_MAP("sstc", RISCV_ISA_EXT_SSTC);
> -				SET_ISA_EXT_MAP("svinval", RISCV_ISA_EXT_SVINVAL);
> -				SET_ISA_EXT_MAP("svnapot", RISCV_ISA_EXT_SVNAPOT);
> -				SET_ISA_EXT_MAP("svpbmt", RISCV_ISA_EXT_SVPBMT);
> -				SET_ISA_EXT_MAP("zba", RISCV_ISA_EXT_ZBA);
> -				SET_ISA_EXT_MAP("zbb", RISCV_ISA_EXT_ZBB);
> -				SET_ISA_EXT_MAP("zbs", RISCV_ISA_EXT_ZBS);
> -				SET_ISA_EXT_MAP("zicbom", RISCV_ISA_EXT_ZICBOM);
> -				SET_ISA_EXT_MAP("zicboz", RISCV_ISA_EXT_ZICBOZ);
> -				SET_ISA_EXT_MAP("zihintpause", RISCV_ISA_EXT_ZIHINTPAUSE);
> +				for (int i = 0; i < riscv_isa_ext_count; i++)
> +					SET_ISA_EXT_MAP(riscv_isa_ext[i].name,
> +							riscv_isa_ext[i].id);

Three cheers for removing one list that needed to be maintained!

>  			}
>  #undef SET_ISA_EXT_MAP
>  		}
> -- 
> 2.40.1
>

Other than also dropping the define,

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

Thanks,
drew

Re: [PATCH v1 5/9] RISC-V: add missing single letter extension definitions

[Andrew Jones]

On Mon, Jun 26, 2023 at 12:19:43PM +0100, Conor Dooley wrote:
> To facilitate adding single letter extensions to riscv_isa_ext, add
> definitions for the extensions present in base_riscv_exts that do not
> already have them.
> 
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  arch/riscv/include/asm/hwcap.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> index 36f46dfd2b87..a35bee219dd7 100644
> --- a/arch/riscv/include/asm/hwcap.h
> +++ b/arch/riscv/include/asm/hwcap.h
> @@ -14,12 +14,17 @@
>  #include <uapi/asm/hwcap.h>
>  
>  #define RISCV_ISA_EXT_a		('a' - 'a')
> +#define RISCV_ISA_EXT_b		('b' - 'a')
>  #define RISCV_ISA_EXT_c		('c' - 'a')
>  #define RISCV_ISA_EXT_d		('d' - 'a')
>  #define RISCV_ISA_EXT_f		('f' - 'a')
>  #define RISCV_ISA_EXT_h		('h' - 'a')
>  #define RISCV_ISA_EXT_i		('i' - 'a')
> +#define RISCV_ISA_EXT_j		('j' - 'a')
> +#define RISCV_ISA_EXT_k		('k' - 'a')
>  #define RISCV_ISA_EXT_m		('m' - 'a')
> +#define RISCV_ISA_EXT_p		('p' - 'a')
> +#define RISCV_ISA_EXT_q		('q' - 'a')
>  #define RISCV_ISA_EXT_s		('s' - 'a')
>  #define RISCV_ISA_EXT_u		('u' - 'a')
>  #define RISCV_ISA_EXT_v		('v' - 'a')
> -- 
> 2.40.1
>

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

Re: [PATCH v1 6/9] RISC-V: add single letter extensions to riscv_isa_ext

[Andrew Jones]

On Mon, Jun 26, 2023 at 12:19:44PM +0100, Conor Dooley wrote:
> So that riscv_fill_hwcap() can use riscv_isa_ext to probe for single
> letter extensions, add them to it. riscv_isa_ext_data grows a new
> member, signifying whether an extension is multi-letter & thus requiring
> special handling.
> As a result, what gets spat out in /proc/cpuinfo will become borked, as
> single letter extensions will be printed as part of the base extensions
> and while printing from riscv_isa_arr. Take the opportunity to unify the
> printing of the isa string, using the new member of riscv_isa_ext_data
> in the process.
> 
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  arch/riscv/include/asm/hwcap.h |  1 +
>  arch/riscv/kernel/cpu.c        | 36 ++++++----------------
>  arch/riscv/kernel/cpufeature.c | 56 +++++++++++++++++++++-------------
>  3 files changed, 46 insertions(+), 47 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> index a35bee219dd7..6ad896dc4342 100644
> --- a/arch/riscv/include/asm/hwcap.h
> +++ b/arch/riscv/include/asm/hwcap.h
> @@ -77,6 +77,7 @@ unsigned long riscv_get_elf_hwcap(void);
>  struct riscv_isa_ext_data {
>  	const unsigned int id;
>  	const char *name;
> +	const bool multi_letter;
>  };
>  
>  extern const struct riscv_isa_ext_data riscv_isa_ext[];
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index beb8b16bbf87..046d9d3dac16 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -160,41 +160,25 @@ arch_initcall(riscv_cpuinfo_init);
>  
>  #ifdef CONFIG_PROC_FS
>  
> -static void print_isa_ext(struct seq_file *f)
> -{
> -	for (int i = 0; i < riscv_isa_ext_count; i++) {
> -		const struct riscv_isa_ext_data *edata = &riscv_isa_ext[i];
> -		if (!__riscv_isa_extension_available(NULL, edata->id))
> -			continue;
> -
> -		seq_printf(f, "_%s", edata->name);
> -	}
> -}
> -
> -/*
> - * These are the only valid base (single letter) ISA extensions as per the spec.
> - * It also specifies the canonical order in which it appears in the spec.
> - * Some of the extension may just be a place holder for now (B, K, P, J).
> - * This should be updated once corresponding extensions are ratified.
> - */
> -static const char base_riscv_exts[13] = "imafdqcbkjpvh";
> -
>  static void print_isa(struct seq_file *f)
>  {
> -	int i;
> -
>  	seq_puts(f, "isa\t\t: ");
> +
>  	if (IS_ENABLED(CONFIG_32BIT))
>  		seq_write(f, "rv32", 4);
>  	else
>  		seq_write(f, "rv64", 4);
>  
> -	for (i = 0; i < sizeof(base_riscv_exts); i++) {
> -		if (__riscv_isa_extension_available(NULL, base_riscv_exts[i] - 'a'))
> -			/* Print only enabled the base ISA extensions */
> -			seq_write(f, &base_riscv_exts[i], 1);
> +	for (int i = 0; i < riscv_isa_ext_count; i++) {
> +		if (!__riscv_isa_extension_available(NULL, riscv_isa_ext[i].id))
> +			continue;
> +
> +		if (riscv_isa_ext[i].multi_letter)
> +			seq_puts(f, "_");
> +
> +		seq_printf(f, "%s", riscv_isa_ext[i].name);
>  	}
> -	print_isa_ext(f);
> +
>  	seq_puts(f, "\n");
>  }
>  
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index b5e23506c4f0..5405d8a58537 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -99,9 +99,10 @@ static bool riscv_isa_extension_check(int id)
>  	return true;
>  }
>  
> -#define __RISCV_ISA_EXT_DATA(_name, _id) {	\
> -	.name = #_name,				\
> -	.id = _id,				\
> +#define __RISCV_ISA_EXT_DATA(_name, _id, _multi) {	\
> +	.name = #_name,					\
> +	.id = _id,					\
> +	.multi_letter = _multi,				\
>  }
>  
>  /*
> @@ -144,24 +145,37 @@ static bool riscv_isa_extension_check(int id)
>   * New entries to this struct should follow the ordering rules described above.
>   */
>  const struct riscv_isa_ext_data riscv_isa_ext[] = {
> -	__RISCV_ISA_EXT_DATA(zicbom, RISCV_ISA_EXT_ZICBOM),
> -	__RISCV_ISA_EXT_DATA(zicboz, RISCV_ISA_EXT_ZICBOZ),
> -	__RISCV_ISA_EXT_DATA(zicntr, RISCV_ISA_EXT_ZICNTR),
> -	__RISCV_ISA_EXT_DATA(zicsr, RISCV_ISA_EXT_ZICSR),
> -	__RISCV_ISA_EXT_DATA(zifencei, RISCV_ISA_EXT_ZIFENCEI),
> -	__RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
> -	__RISCV_ISA_EXT_DATA(zihpm, RISCV_ISA_EXT_ZIHPM),
> -	__RISCV_ISA_EXT_DATA(zba, RISCV_ISA_EXT_ZBA),
> -	__RISCV_ISA_EXT_DATA(zbb, RISCV_ISA_EXT_ZBB),
> -	__RISCV_ISA_EXT_DATA(zbs, RISCV_ISA_EXT_ZBS),
> -	__RISCV_ISA_EXT_DATA(smaia, RISCV_ISA_EXT_SMAIA),
> -	__RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
> -	__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
> -	__RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),
> -	__RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL),
> -	__RISCV_ISA_EXT_DATA(svnapot, RISCV_ISA_EXT_SVNAPOT),
> -	__RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT),
> -	__RISCV_ISA_EXT_DATA("", RISCV_ISA_EXT_MAX),
> +	__RISCV_ISA_EXT_DATA(i, RISCV_ISA_EXT_i, false),
> +	__RISCV_ISA_EXT_DATA(m, RISCV_ISA_EXT_m, false),
> +	__RISCV_ISA_EXT_DATA(a, RISCV_ISA_EXT_a, false),
> +	__RISCV_ISA_EXT_DATA(f, RISCV_ISA_EXT_f, false),
> +	__RISCV_ISA_EXT_DATA(d, RISCV_ISA_EXT_d, false),
> +	__RISCV_ISA_EXT_DATA(q, RISCV_ISA_EXT_q, false),
> +	__RISCV_ISA_EXT_DATA(c, RISCV_ISA_EXT_c, false),
> +	__RISCV_ISA_EXT_DATA(b, RISCV_ISA_EXT_b, false),
> +	__RISCV_ISA_EXT_DATA(k, RISCV_ISA_EXT_k, false),
> +	__RISCV_ISA_EXT_DATA(j, RISCV_ISA_EXT_j, false),
> +	__RISCV_ISA_EXT_DATA(p, RISCV_ISA_EXT_p, false),
> +	__RISCV_ISA_EXT_DATA(v, RISCV_ISA_EXT_v, false),
> +	__RISCV_ISA_EXT_DATA(h, RISCV_ISA_EXT_h, false),
> +	__RISCV_ISA_EXT_DATA(zicbom, RISCV_ISA_EXT_ZICBOM, true),
> +	__RISCV_ISA_EXT_DATA(zicboz, RISCV_ISA_EXT_ZICBOZ, true),
> +	__RISCV_ISA_EXT_DATA(zicntr, RISCV_ISA_EXT_ZICNTR, true),
> +	__RISCV_ISA_EXT_DATA(zicsr, RISCV_ISA_EXT_ZICSR, true),
> +	__RISCV_ISA_EXT_DATA(zifencei, RISCV_ISA_EXT_ZIFENCEI, true),
> +	__RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE, true),
> +	__RISCV_ISA_EXT_DATA(zihpm, RISCV_ISA_EXT_ZIHPM, true),
> +	__RISCV_ISA_EXT_DATA(zba, RISCV_ISA_EXT_ZBA, true),
> +	__RISCV_ISA_EXT_DATA(zbb, RISCV_ISA_EXT_ZBB, true),
> +	__RISCV_ISA_EXT_DATA(zbs, RISCV_ISA_EXT_ZBS, true),
> +	__RISCV_ISA_EXT_DATA(smaia, RISCV_ISA_EXT_SMAIA, true),
> +	__RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA, true),
> +	__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF, true),
> +	__RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC, true),
> +	__RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL, true),
> +	__RISCV_ISA_EXT_DATA(svnapot, RISCV_ISA_EXT_SVNAPOT, true),
> +	__RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT, true),
> +	__RISCV_ISA_EXT_DATA("", RISCV_ISA_EXT_MAX, true),
>  };
>  
>  const size_t riscv_isa_ext_count = ARRAY_SIZE(riscv_isa_ext);
> -- 
> 2.40.1
>

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

Re: [PATCH v1 3/9] RISC-V: shunt isa_ext_arr to cpufeature.c

[Andrew Jones]

On Mon, Jun 26, 2023 at 05:29:04PM +0200, Andrew Jones wrote:
> On Mon, Jun 26, 2023 at 12:19:41PM +0100, Conor Dooley wrote:
...
> > +const struct riscv_isa_ext_data riscv_isa_ext[] = {
> > +	__RISCV_ISA_EXT_DATA(zicbom, RISCV_ISA_EXT_ZICBOM),
> > +	__RISCV_ISA_EXT_DATA(zicboz, RISCV_ISA_EXT_ZICBOZ),
> > +	__RISCV_ISA_EXT_DATA(zicntr, RISCV_ISA_EXT_ZICNTR),
> > +	__RISCV_ISA_EXT_DATA(zicsr, RISCV_ISA_EXT_ZICSR),
> > +	__RISCV_ISA_EXT_DATA(zifencei, RISCV_ISA_EXT_ZIFENCEI),
> > +	__RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
> > +	__RISCV_ISA_EXT_DATA(zihpm, RISCV_ISA_EXT_ZIHPM),
> > +	__RISCV_ISA_EXT_DATA(zba, RISCV_ISA_EXT_ZBA),
> > +	__RISCV_ISA_EXT_DATA(zbb, RISCV_ISA_EXT_ZBB),
> > +	__RISCV_ISA_EXT_DATA(zbs, RISCV_ISA_EXT_ZBS),
> > +	__RISCV_ISA_EXT_DATA(smaia, RISCV_ISA_EXT_SMAIA),
> > +	__RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
> > +	__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
> > +	__RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),
> > +	__RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL),
> > +	__RISCV_ISA_EXT_DATA(svnapot, RISCV_ISA_EXT_SVNAPOT),
> > +	__RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT),
> > +	__RISCV_ISA_EXT_DATA("", RISCV_ISA_EXT_MAX),
> 
> I think we can either drop this null entry or drop the count variable
> below. My preference would be to drop the count variable, and always
> loop to the null.

Eh, never mind, the entry isn't null, it's "". Why do we have that entry
though? I guess it can be dropped?

Thanks,
drew

Re: [PATCH v1 1/9] RISC-V: don't parse dt/acpi isa string to get rv32/rv64

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1182 bytes --]

On Mon, Jun 26, 2023 at 05:14:15PM +0200, Andrew Jones wrote:
> On Mon, Jun 26, 2023 at 12:19:39PM +0100, Conor Dooley wrote:
> > From: Heiko Stuebner <heiko.stuebner@vrull.eu>
> > @@ -333,8 +335,6 @@ static int c_show(struct seq_file *m, void *v)
> >  
> >  		of_node_put(node);
> >  	} else {
> > -		if (!acpi_get_riscv_isa(NULL, cpu_id, &isa))
> > -			print_isa(m, isa);
> >  
> 
> Extra blank line here to remove. Actually the whole 'else' can be removed
> because the print_mmu() call can be brought up above the
> 'if (acpi_disabled)'

Can it be? I intentionally did not make that change - wasn't sure
whether re-ordering the fields in there was permissible.

One of the few things I know does parsing of /proc/cpuinfo is:
https://github.com/google/cpu_features/blob/main/src/impl_riscv_linux.c
and that doesn't seem to care about the mmu, but does rely on
vendor/uarch ordering.

Makes me wonder, does ACPI break things by leaving out uarch/vendor
fields, if there is something that expects them to exist? We should
not intentionally break stuff in /proc/cpuinfo, but can't say I feel any
sympathy for naively parsing it.

> >  		print_mmu(m);


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

Re: [PATCH v1 3/9] RISC-V: shunt isa_ext_arr to cpufeature.c

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2057 bytes --]

On Mon, Jun 26, 2023 at 05:44:18PM +0200, Andrew Jones wrote:
> On Mon, Jun 26, 2023 at 05:29:04PM +0200, Andrew Jones wrote:
> > On Mon, Jun 26, 2023 at 12:19:41PM +0100, Conor Dooley wrote:
> ...
> > > +const struct riscv_isa_ext_data riscv_isa_ext[] = {
> > > +	__RISCV_ISA_EXT_DATA(zicbom, RISCV_ISA_EXT_ZICBOM),
> > > +	__RISCV_ISA_EXT_DATA(zicboz, RISCV_ISA_EXT_ZICBOZ),
> > > +	__RISCV_ISA_EXT_DATA(zicntr, RISCV_ISA_EXT_ZICNTR),
> > > +	__RISCV_ISA_EXT_DATA(zicsr, RISCV_ISA_EXT_ZICSR),
> > > +	__RISCV_ISA_EXT_DATA(zifencei, RISCV_ISA_EXT_ZIFENCEI),
> > > +	__RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
> > > +	__RISCV_ISA_EXT_DATA(zihpm, RISCV_ISA_EXT_ZIHPM),
> > > +	__RISCV_ISA_EXT_DATA(zba, RISCV_ISA_EXT_ZBA),
> > > +	__RISCV_ISA_EXT_DATA(zbb, RISCV_ISA_EXT_ZBB),
> > > +	__RISCV_ISA_EXT_DATA(zbs, RISCV_ISA_EXT_ZBS),
> > > +	__RISCV_ISA_EXT_DATA(smaia, RISCV_ISA_EXT_SMAIA),
> > > +	__RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
> > > +	__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
> > > +	__RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),
> > > +	__RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL),
> > > +	__RISCV_ISA_EXT_DATA(svnapot, RISCV_ISA_EXT_SVNAPOT),
> > > +	__RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT),
> > > +	__RISCV_ISA_EXT_DATA("", RISCV_ISA_EXT_MAX),
> > 
> > I think we can either drop this null entry or drop the count variable
> > below. My preference would be to drop the count variable, and always
> > loop to the null.
> 
> Eh, never mind, the entry isn't null, it's "". Why do we have that entry
> though? I guess it can be dropped?

It may just be cargo culting? The commit that added this, a9b202606c69
("RISC-V: Improve /proc/cpuinfo output for ISA extensions") [1], added
an empty array other than this element & perhaps it was just not removed
when real entries were added? Symptom of adding a feature without an
actual user (multiletter extension) perhaps?

1 - https://lore.kernel.org/all/20220314203845.832648-1-atishp@rivosinc.com/

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

Re: [PATCH v1 1/9] RISC-V: don't parse dt/acpi isa string to get rv32/rv64

[Andrew Jones]

On Mon, Jun 26, 2023 at 04:51:29PM +0100, Conor Dooley wrote:
> On Mon, Jun 26, 2023 at 05:14:15PM +0200, Andrew Jones wrote:
> > On Mon, Jun 26, 2023 at 12:19:39PM +0100, Conor Dooley wrote:
> > > From: Heiko Stuebner <heiko.stuebner@vrull.eu>
> > > @@ -333,8 +335,6 @@ static int c_show(struct seq_file *m, void *v)
> > >  
> > >  		of_node_put(node);
> > >  	} else {
> > > -		if (!acpi_get_riscv_isa(NULL, cpu_id, &isa))
> > > -			print_isa(m, isa);
> > >  
> > 
> > Extra blank line here to remove. Actually the whole 'else' can be removed
> > because the print_mmu() call can be brought up above the
> > 'if (acpi_disabled)'
> 
> Can it be? I intentionally did not make that change - wasn't sure
> whether re-ordering the fields in there was permissible.

I agree we shouldn't change the order, but moving print_mmu() up won't,
afaict.

> 
> One of the few things I know does parsing of /proc/cpuinfo is:
> https://github.com/google/cpu_features/blob/main/src/impl_riscv_linux.c
> and that doesn't seem to care about the mmu, but does rely on
> vendor/uarch ordering.
> 
> Makes me wonder, does ACPI break things by leaving out uarch/vendor
> fields, if there is something that expects them to exist? We should
> not intentionally break stuff in /proc/cpuinfo, but can't say I feel any
> sympathy for naively parsing it.

Yes, it would be nice for ACPI to be consistent. I'm not sure what can be
done about that.

Thanks,
drew

> 
> > >  		print_mmu(m);
> 

Re: [PATCH v1 2/9] RISC-V: drop a needless check in print_isa_ext()

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1941 bytes --]

On Mon, Jun 26, 2023 at 05:19:08PM +0200, Andrew Jones wrote:
> On Mon, Jun 26, 2023 at 12:19:40PM +0100, Conor Dooley wrote:
> > isa_ext_arr cannot be empty, as some of the extensions within it are
> > always built into the kernel.
> 
> This is only true since commit 07edc32779e3 ("RISC-V: always report
> presence of extensions formerly part of the base ISA"), right? If
> so, it might be nice to call that commit out in this commit message.

Per my last mail, where I commented on the origins of some of this code,
there were no multi-letter extensions when this code was first added.
When the first multi-letter ones did get added, it was Sscofpmf - that
doesn't have a Kconfig symbol to disable it, so I think this has been
redundant for a long time.

Apart from the ones I recently added, there's a fair few others that
are not gated & should always be present.
It's probably not clear from the comment, but this check is for whether
the kernel supports extensions, not whether the system it is running on
does. I guess I should expand on that in my commit message.

Thanks,
Conor.

> > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> > ---
> >  arch/riscv/kernel/cpu.c | 4 ----
> >  1 file changed, 4 deletions(-)
> > 
> > diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> > index 742bb42e7e86..01f7e5c62997 100644
> > --- a/arch/riscv/kernel/cpu.c
> > +++ b/arch/riscv/kernel/cpu.c
> > @@ -233,10 +233,6 @@ static void print_isa_ext(struct seq_file *f)
> >  
> >  	arr_sz = ARRAY_SIZE(isa_ext_arr) - 1;
> >  
> > -	/* No extension support available */
> > -	if (arr_sz <= 0)
> > -		return;
> > -
> >  	for (i = 0; i <= arr_sz; i++) {
> >  		edata = &isa_ext_arr[i];
> >  		if (!__riscv_isa_extension_available(NULL, edata->isa_ext_id))
> > -- 
> > 2.40.1
> >
> 
> Otherwise,
> 
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> 
> Thanks,
> drew

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

Re: [PATCH v1 1/9] RISC-V: don't parse dt/acpi isa string to get rv32/rv64

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1992 bytes --]

On Mon, Jun 26, 2023 at 06:05:40PM +0200, Andrew Jones wrote:
> On Mon, Jun 26, 2023 at 04:51:29PM +0100, Conor Dooley wrote:
> > On Mon, Jun 26, 2023 at 05:14:15PM +0200, Andrew Jones wrote:
> > > On Mon, Jun 26, 2023 at 12:19:39PM +0100, Conor Dooley wrote:
> > > > From: Heiko Stuebner <heiko.stuebner@vrull.eu>
> > > > @@ -333,8 +335,6 @@ static int c_show(struct seq_file *m, void *v)
> > > >  
> > > >  		of_node_put(node);
> > > >  	} else {
> > > > -		if (!acpi_get_riscv_isa(NULL, cpu_id, &isa))
> > > > -			print_isa(m, isa);
> > > >  
> > > 
> > > Extra blank line here to remove. Actually the whole 'else' can be removed
> > > because the print_mmu() call can be brought up above the
> > > 'if (acpi_disabled)'
> > 
> > Can it be? I intentionally did not make that change - wasn't sure
> > whether re-ordering the fields in there was permissible.
> 
> I agree we shouldn't change the order, but moving print_mmu() up won't,
> afaict.

D'oh, I'm an eejit. Sure, I'll do that for v2. Thanks!

> > One of the few things I know does parsing of /proc/cpuinfo is:
> > https://github.com/google/cpu_features/blob/main/src/impl_riscv_linux.c
> > and that doesn't seem to care about the mmu, but does rely on
> > vendor/uarch ordering.
> > 
> > Makes me wonder, does ACPI break things by leaving out uarch/vendor
> > fields, if there is something that expects them to exist? We should
> > not intentionally break stuff in /proc/cpuinfo, but can't say I feel any
> > sympathy for naively parsing it.
> 
> Yes, it would be nice for ACPI to be consistent. I'm not sure what can be
> done about that.

Print "unknown", until there's a way of passing the info?
Speaking of being an eejit, adding new fields to the file would probably
break some really naive parsers & quite frankly that sort of thing can
keep the pieces IMO. Ditto if adding more extensions breaks someone that
expects _zicbom_zicboz that breaks when _zicbop is slid into the middle.

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

Re: [PATCH v1 7/9] RISC-V: split riscv_fill_hwcap() in 3

[Andrew Jones]

On Mon, Jun 26, 2023 at 12:19:45PM +0100, Conor Dooley wrote:
> Before adding more complexity to it, split riscv_fill_hwcap() into 3
> distinct sections:
> - riscv_fill_hwcap() still is the top level function, into which the
>   additional complexity will be added.
> - riscv_fill_hwcap_from_isa_string() handles getting the information
>   from the riscv,isa/ACPI equivalent across harts & the various quirks
>   there
> - riscv_parse_isa_string() does what it says on the tin.
> 
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  arch/riscv/kernel/cpufeature.c | 350 +++++++++++++++++----------------
>  1 file changed, 182 insertions(+), 168 deletions(-)
> 
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 5405d8a58537..366477ba1eea 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -180,29 +180,172 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
>  
>  const size_t riscv_isa_ext_count = ARRAY_SIZE(riscv_isa_ext);
>  
> -void __init riscv_fill_hwcap(void)
> +static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct riscv_isainfo *isainfo,
> +					  unsigned long *isa2hwcap, const char *isa)
> +{
> +	/*
> +	 * 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;
> +
> +	while (*isa) {
> +		const char *ext = isa++;
> +		const char *ext_end = isa;
> +		bool ext_long = false, ext_err = false;
> +
> +		switch (*ext) {
> +		case 's':
> +			/*
> +			 * Workaround for invalid single-letter 's' & 'u'(QEMU).
> +			 * No need to set the bit in riscv_isa as 's' & 'u' are
> +			 * not valid ISA extensions. It works until multi-letter
> +			 * extension starting with "Su" appears.
> +			 */
> +			if (ext[-1] != '_' && ext[1] == 'u') {
> +				++isa;
> +				ext_err = true;
> +				break;
> +			}
> +			fallthrough;
> +		case 'S':
> +		case 'x':
> +		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 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;
> +			for (; *isa && *isa != '_'; ++isa)
> +				if (unlikely(!isalnum(*isa)))
> +					ext_err = true;
> +
> +			ext_end = isa;
> +			if (unlikely(ext_err))
> +				break;
> +
> +			if (!isdigit(ext_end[-1]))
> +				break;
> +
> +			while (isdigit(*--ext_end))
> +				;
> +
> +			if (tolower(ext_end[0]) != 'p' || !isdigit(ext_end[-1])) {
> +				++ext_end;
> +				break;
> +			}
> +
> +			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;
> +			}
> +
> +			if (!isdigit(*isa))
> +				break;
> +
> +			while (isdigit(*++isa))
> +				;
> +
> +			if (tolower(*isa) != 'p')
> +				break;
> +
> +			if (!isdigit(*++isa)) {
> +				--isa;
> +				break;
> +			}
> +
> +			while (isdigit(*++isa))
> +				;
> +
> +			break;
> +		}
> +
> +		/*
> +		 * The parser expects that at the start of an iteration isa points to the
> +		 * 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;
> +
> +#define SET_ISA_EXT_MAP(name, bit)						\
> +		do {								\
> +			if ((ext_end - ext == sizeof(name) - 1) &&		\
> +			     !strncasecmp(ext, name, sizeof(name) - 1) &&	\
> +			     riscv_isa_extension_check(bit))			\
> +				set_bit(bit, isainfo->isa);			\
> +		} while (false)							\
> +
> +		if (unlikely(ext_err))
> +			continue;
> +		if (!ext_long) {
> +			int nr = tolower(*ext) - 'a';
> +
> +			if (riscv_isa_extension_check(nr)) {
> +				*this_hwcap |= isa2hwcap[nr];
> +				set_bit(nr, isainfo->isa);
> +			}
> +		} else {
> +			for (int i = 0; i < riscv_isa_ext_count; i++)
> +				SET_ISA_EXT_MAP(riscv_isa_ext[i].name,
> +						riscv_isa_ext[i].id);
> +		}
> +#undef SET_ISA_EXT_MAP
> +	}
> +}
> +
> +static void __init riscv_fill_hwcap_from_isa_string(unsigned long *isa2hwcap)
>  {
>  	struct device_node *node;
>  	const char *isa;
> -	char print_str[NUM_ALPHA_EXTS + 1];
> -	int i, j, rc;
> -	unsigned long isa2hwcap[26] = {0};
> +	int rc;
>  	struct acpi_table_header *rhct;
>  	acpi_status status;
>  	unsigned int cpu;
>  
> -	isa2hwcap['i' - 'a'] = COMPAT_HWCAP_ISA_I;
> -	isa2hwcap['m' - 'a'] = COMPAT_HWCAP_ISA_M;
> -	isa2hwcap['a' - 'a'] = COMPAT_HWCAP_ISA_A;
> -	isa2hwcap['f' - 'a'] = COMPAT_HWCAP_ISA_F;
> -	isa2hwcap['d' - 'a'] = COMPAT_HWCAP_ISA_D;
> -	isa2hwcap['c' - 'a'] = COMPAT_HWCAP_ISA_C;
> -	isa2hwcap['v' - 'a'] = COMPAT_HWCAP_ISA_V;
> -
> -	elf_hwcap = 0;
> -
> -	bitmap_zero(riscv_isa, RISCV_ISA_EXT_MAX);
> -
>  	if (!acpi_disabled) {
>  		status = acpi_get_table(ACPI_SIG_RHCT, 0, &rhct);
>  		if (ACPI_FAILURE(status))
> @@ -234,158 +377,7 @@ void __init riscv_fill_hwcap(void)
>  			}
>  		}
>  
> -		/*
> -		 * 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;
> -
> -		while (*isa) {
> -			const char *ext = isa++;
> -			const char *ext_end = isa;
> -			bool ext_long = false, ext_err = false;
> -
> -			switch (*ext) {
> -			case 's':
> -				/*
> -				 * Workaround for invalid single-letter 's' & 'u'(QEMU).
> -				 * No need to set the bit in riscv_isa as 's' & 'u' are
> -				 * not valid ISA extensions. It works until multi-letter
> -				 * extension starting with "Su" appears.
> -				 */
> -				if (ext[-1] != '_' && ext[1] == 'u') {
> -					++isa;
> -					ext_err = true;
> -					break;
> -				}
> -				fallthrough;
> -			case 'S':
> -			case 'x':
> -			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 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;
> -				for (; *isa && *isa != '_'; ++isa)
> -					if (unlikely(!isalnum(*isa)))
> -						ext_err = true;
> -
> -				ext_end = isa;
> -				if (unlikely(ext_err))
> -					break;
> -
> -				if (!isdigit(ext_end[-1]))
> -					break;
> -
> -				while (isdigit(*--ext_end))
> -					;
> -
> -				if (tolower(ext_end[0]) != 'p' || !isdigit(ext_end[-1])) {
> -					++ext_end;
> -					break;
> -				}
> -
> -				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;
> -				}
> -
> -				if (!isdigit(*isa))
> -					break;
> -
> -				while (isdigit(*++isa))
> -					;
> -
> -				if (tolower(*isa) != 'p')
> -					break;
> -
> -				if (!isdigit(*++isa)) {
> -					--isa;
> -					break;
> -				}
> -
> -				while (isdigit(*++isa))
> -					;
> -
> -				break;
> -			}
> -
> -			/*
> -			 * The parser expects that at the start of an iteration isa points to the
> -			 * 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;
> -
> -#define SET_ISA_EXT_MAP(name, bit)							\
> -			do {								\
> -				if ((ext_end - ext == sizeof(name) - 1) &&		\
> -				     !strncasecmp(ext, name, sizeof(name) - 1) &&	\
> -				     riscv_isa_extension_check(bit))			\
> -					set_bit(bit, isainfo->isa);			\
> -			} while (false)							\
> -
> -			if (unlikely(ext_err))
> -				continue;
> -			if (!ext_long) {
> -				int nr = tolower(*ext) - 'a';
> -
> -				if (riscv_isa_extension_check(nr)) {
> -					this_hwcap |= isa2hwcap[nr];
> -					set_bit(nr, isainfo->isa);
> -				}
> -			} else {
> -				for (int i = 0; i < riscv_isa_ext_count; i++)
> -					SET_ISA_EXT_MAP(riscv_isa_ext[i].name,
> -							riscv_isa_ext[i].id);
> -			}
> -#undef SET_ISA_EXT_MAP
> -		}
> +		riscv_parse_isa_string(&this_hwcap, isainfo, isa2hwcap, isa);
>  
>  		/*
>  		 * Linux requires the following extensions, so we may as well
> @@ -422,6 +414,28 @@ void __init riscv_fill_hwcap(void)
>  
>  	if (!acpi_disabled && rhct)
>  		acpi_put_table((struct acpi_table_header *)rhct);
> +}
> +
> +void __init riscv_fill_hwcap(void)
> +{
> +	struct device_node *node;
> +	const char *isa;
> +	char print_str[NUM_ALPHA_EXTS + 1];
> +	int i, j, rc;
> +	unsigned long isa2hwcap[26] = {0};
> +	struct acpi_table_header *rhct;
> +	acpi_status status;
> +	unsigned int cpu;

I see all these unused variables get removed in the next patch, but they
should get removed here, lest they trigger some warnings and bots come
after you!

> +
> +	isa2hwcap['i' - 'a'] = COMPAT_HWCAP_ISA_I;
> +	isa2hwcap['m' - 'a'] = COMPAT_HWCAP_ISA_M;
> +	isa2hwcap['a' - 'a'] = COMPAT_HWCAP_ISA_A;
> +	isa2hwcap['f' - 'a'] = COMPAT_HWCAP_ISA_F;
> +	isa2hwcap['d' - 'a'] = COMPAT_HWCAP_ISA_D;
> +	isa2hwcap['c' - 'a'] = COMPAT_HWCAP_ISA_C;
> +	isa2hwcap['v' - 'a'] = COMPAT_HWCAP_ISA_V;
> +
> +	riscv_fill_hwcap_from_isa_string(isa2hwcap);
>  
>  	/* We don't support systems with F but without D, so mask those out
>  	 * here. */
> -- 
> 2.40.1
>

Otherwise,

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

Thanks,
drew

Re: [PATCH v1 8/9] RISC-V: enable extension detection from new properties

[Andrew Jones]

On Mon, Jun 26, 2023 at 12:19:46PM +0100, Conor Dooley wrote:
> Add support for parsing the new riscv,isa-extensions property in
> riscv_fill_hwcap(), by means of a new "property" member of the
> riscv_isa_ext_data struct. For now, this shadows the name of the
> extension, however this may not be the case for all extensions.
> For the sake of backwards compatibility, fall back to the old scheme
> if the new properties are not detected. For now, just inform, rather
> than warn, when that happens.
> 
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
> Naming things is hard, didn't know what to call the new function...

riscv_fill_hwcap_from_isa_extensions() ?

_new() won't age well.

> ---
>  arch/riscv/include/asm/hwcap.h |  1 +
>  arch/riscv/kernel/cpufeature.c | 80 ++++++++++++++++++++++++++++++----
>  2 files changed, 72 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> index 6ad896dc4342..e7f235868aa2 100644
> --- a/arch/riscv/include/asm/hwcap.h
> +++ b/arch/riscv/include/asm/hwcap.h
> @@ -77,6 +77,7 @@ unsigned long riscv_get_elf_hwcap(void);
>  struct riscv_isa_ext_data {
>  	const unsigned int id;
>  	const char *name;
> +	const char *property;
>  	const bool multi_letter;
>  };
>  
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 366477ba1eea..72eb757ad871 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -101,6 +101,7 @@ static bool riscv_isa_extension_check(int id)
>  
>  #define __RISCV_ISA_EXT_DATA(_name, _id, _multi) {	\
>  	.name = #_name,					\
> +	.property = #_name,				\
>  	.id = _id,					\
>  	.multi_letter = _multi,				\
>  }
> @@ -416,16 +417,66 @@ static void __init riscv_fill_hwcap_from_isa_string(unsigned long *isa2hwcap)
>  		acpi_put_table((struct acpi_table_header *)rhct);
>  }
>  
> +static int __init riscv_fill_hwcap_new(unsigned long *isa2hwcap)
> +{
> +	unsigned int cpu;
> +
> +	for_each_possible_cpu(cpu) {
> +		unsigned long this_hwcap = 0;
> +		struct device_node *cpu_node;
> +		DECLARE_BITMAP(this_isa, RISCV_ISA_EXT_MAX);
> +
> +		cpu_node = of_cpu_device_node_get(cpu);
> +		if (!cpu_node) {
> +			pr_warn("Unable to find cpu node\n");
> +			continue;
> +		}
> +
> +		if (!of_property_present(cpu_node, "riscv,isa-extensions"))
> +			continue;
> +
> +		for (int i = 0; i < riscv_isa_ext_count; i++) {
> +			if (of_property_match_string(cpu_node, "riscv,isa-extensions",
> +						     riscv_isa_ext[i].name) < 0)
> +				continue;
> +
> +			if (!riscv_isa_extension_check(riscv_isa_ext[i].id))
> +				continue;
> +
> +			if (!riscv_isa_ext[i].multi_letter)
> +				this_hwcap |= isa2hwcap[riscv_isa_ext[i].id];
> +
> +			set_bit(riscv_isa_ext[i].id, this_isa);
> +		}
> +
> +		of_node_put(cpu_node);
> +
> +		/*
> +		 * All "okay" harts should have same isa. Set HWCAP based on
> +		 * common capabilities of every "okay" hart, in case they don't.
> +		 */
> +		if (elf_hwcap)
> +			elf_hwcap &= this_hwcap;
> +		else
> +			elf_hwcap = this_hwcap;
> +
> +		if (bitmap_empty(riscv_isa, RISCV_ISA_EXT_MAX))
> +			bitmap_copy(riscv_isa, this_isa, RISCV_ISA_EXT_MAX);
> +		else
> +			bitmap_and(riscv_isa, riscv_isa, this_isa, RISCV_ISA_EXT_MAX);
> +	}
> +
> +	if (bitmap_empty(riscv_isa, RISCV_ISA_EXT_MAX))
> +		return -ENOENT;
> +
> +	return 0;
> +}
> +
>  void __init riscv_fill_hwcap(void)
>  {
> -	struct device_node *node;
> -	const char *isa;
>  	char print_str[NUM_ALPHA_EXTS + 1];
> -	int i, j, rc;
>  	unsigned long isa2hwcap[26] = {0};
> -	struct acpi_table_header *rhct;
> -	acpi_status status;
> -	unsigned int cpu;
> +	int i, j;
>  
>  	isa2hwcap['i' - 'a'] = COMPAT_HWCAP_ISA_I;
>  	isa2hwcap['m' - 'a'] = COMPAT_HWCAP_ISA_M;
> @@ -435,10 +486,21 @@ void __init riscv_fill_hwcap(void)
>  	isa2hwcap['c' - 'a'] = COMPAT_HWCAP_ISA_C;
>  	isa2hwcap['v' - 'a'] = COMPAT_HWCAP_ISA_V;
>  
> -	riscv_fill_hwcap_from_isa_string(isa2hwcap);
> +	if (!acpi_disabled) {
> +		riscv_fill_hwcap_from_isa_string(isa2hwcap);
> +	} else {
> +		int ret = riscv_fill_hwcap_new(isa2hwcap);
>  
> -	/* We don't support systems with F but without D, so mask those out
> -	 * here. */
> +		if (ret) {
> +			pr_info("Falling back to deprecated \"riscv,isa\"\n");
> +			riscv_fill_hwcap_from_isa_string(isa2hwcap);
> +		}
> +	}
> +
> +	/*
> +	 * We don't support systems with F but without D, so mask those out
> +	 * here.
> +	 */
>  	if ((elf_hwcap & COMPAT_HWCAP_ISA_F) && !(elf_hwcap & COMPAT_HWCAP_ISA_D)) {
>  		pr_info("This kernel does not support systems with F but not D\n");
>  		elf_hwcap &= ~COMPAT_HWCAP_ISA_F;
> -- 
> 2.40.1
>

Other than the name and moving the removal of the unused declarations to
the previous patch,

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

Thanks,
drew

Re: [PATCH v1 9/9] RISC-V: try new extension properties in of_early_processor_hartid()

[Andrew Jones]

On Mon, Jun 26, 2023 at 12:19:47PM +0100, Conor Dooley wrote:
> To fully deprecate the kernel's use of "riscv,isa",
> of_early_processor_hartid() needs to first try using the new properties,
> before falling back to "riscv,isa".
> 
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  arch/riscv/kernel/cpu.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index 046d9d3dac16..332574f27c95 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -61,8 +61,29 @@ int riscv_early_of_processor_hartid(struct device_node *node, unsigned long *har
>  		return -ENODEV;
>  	}
>  
> +	if (of_property_read_string(node, "riscv,isa-base", &isa))
> +		goto old_interface;
> +
> +	if (IS_ENABLED(CONFIG_32BIT) && strncasecmp(isa, "rv32i", 5))
> +		return -ENODEV;
> +
> +	if (IS_ENABLED(CONFIG_64BIT) && strncasecmp(isa, "rv64i", 5))
> +		return -ENODEV;
> +
> +	if (!of_property_present(node, "riscv,isa-extensions"))
> +		return -ENODEV;
> +
> +	if (of_property_match_string(node, "riscv,isa-extensions", "i") < 0 ||
> +	    of_property_match_string(node, "riscv,isa-extensions", "m") < 0 ||
> +	    of_property_match_string(node, "riscv,isa-extensions", "a") < 0)
> +		return -ENODEV;
> +
> +	return 0;
> +
> +old_interface:
>  	if (of_property_read_string(node, "riscv,isa", &isa)) {
> -		pr_warn("CPU with hartid=%lu has no \"riscv,isa\" property\n", *hart);
> +		pr_warn("CPU with hartid=%lu has no \"riscv,isa-base\" or \"riscv,isa\" property\n",
> +			*hart);
>  		return -ENODEV;
>  	}
>  
> -- 
> 2.40.1
>

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

Re: [PATCH v1 2/9] RISC-V: drop a needless check in print_isa_ext()

[Andrew Jones]

On Mon, Jun 26, 2023 at 05:08:28PM +0100, Conor Dooley wrote:
> On Mon, Jun 26, 2023 at 05:19:08PM +0200, Andrew Jones wrote:
> > On Mon, Jun 26, 2023 at 12:19:40PM +0100, Conor Dooley wrote:
> > > isa_ext_arr cannot be empty, as some of the extensions within it are
> > > always built into the kernel.
> > 
> > This is only true since commit 07edc32779e3 ("RISC-V: always report
> > presence of extensions formerly part of the base ISA"), right? If
> > so, it might be nice to call that commit out in this commit message.
> 
> Per my last mail, where I commented on the origins of some of this code,
> there were no multi-letter extensions when this code was first added.
> When the first multi-letter ones did get added, it was Sscofpmf - that
> doesn't have a Kconfig symbol to disable it, so I think this has been
> redundant for a long time.
> 
> Apart from the ones I recently added, there's a fair few others that
> are not gated & should always be present.
> It's probably not clear from the comment, but this check is for whether
> the kernel supports extensions, not whether the system it is running on
> does. I guess I should expand on that in my commit message.

That part I understood, but I was thinking it'd be nice to call out when
the first extension was added which cannot be disabled by a config to
provide extra evidence that it's safe to remove the check.

Thanks,
drew

Re: [PATCH v1 1/9] RISC-V: don't parse dt/acpi isa string to get rv32/rv64

[Sunil V L]

On Mon, Jun 26, 2023 at 05:16:09PM +0100, Conor Dooley wrote:
> On Mon, Jun 26, 2023 at 06:05:40PM +0200, Andrew Jones wrote:
> > On Mon, Jun 26, 2023 at 04:51:29PM +0100, Conor Dooley wrote:
> > > On Mon, Jun 26, 2023 at 05:14:15PM +0200, Andrew Jones wrote:
> > > > On Mon, Jun 26, 2023 at 12:19:39PM +0100, Conor Dooley wrote:
> > > > > From: Heiko Stuebner <heiko.stuebner@vrull.eu>
> > > > > @@ -333,8 +335,6 @@ static int c_show(struct seq_file *m, void *v)
> > > > >  
> > > > >  		of_node_put(node);
> > > > >  	} else {
> > > > > -		if (!acpi_get_riscv_isa(NULL, cpu_id, &isa))
> > > > > -			print_isa(m, isa);
> > > > >  
> > > > 
> > > > Extra blank line here to remove. Actually the whole 'else' can be removed
> > > > because the print_mmu() call can be brought up above the
> > > > 'if (acpi_disabled)'
> > > 
> > > Can it be? I intentionally did not make that change - wasn't sure
> > > whether re-ordering the fields in there was permissible.
> > 
> > I agree we shouldn't change the order, but moving print_mmu() up won't,
> > afaict.
> 
> D'oh, I'm an eejit. Sure, I'll do that for v2. Thanks!
> 
> > > One of the few things I know does parsing of /proc/cpuinfo is:
> > > https://github.com/google/cpu_features/blob/main/src/impl_riscv_linux.c
> > > and that doesn't seem to care about the mmu, but does rely on
> > > vendor/uarch ordering.
> > > 
> > > Makes me wonder, does ACPI break things by leaving out uarch/vendor
> > > fields, if there is something that expects them to exist? We should
> > > not intentionally break stuff in /proc/cpuinfo, but can't say I feel any
> > > sympathy for naively parsing it.
> > 
> > Yes, it would be nice for ACPI to be consistent. I'm not sure what can be
> > done about that.
> 
> Print "unknown", until there's a way of passing the info?
> Speaking of being an eejit, adding new fields to the file would probably
> break some really naive parsers & quite frankly that sort of thing can
> keep the pieces IMO. Ditto if adding more extensions breaks someone that
> expects _zicbom_zicboz that breaks when _zicbop is slid into the middle.

Hi Conor,

Instead of unknown, could you print "risc-v" or "riscv"? There is nothing
equivalent to compatible property in ACPI. Using mvendorid,
marchid and mimpid, people can determine the exact processor
<manufacturer>,<model>.

Thanks!
Sunil

Re: [PATCH v1 1/9] RISC-V: don't parse dt/acpi isa string to get rv32/rv64

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1680 bytes --]

On Tue, Jun 27, 2023 at 01:32:23PM +0530, Sunil V L wrote:
> On Mon, Jun 26, 2023 at 05:16:09PM +0100, Conor Dooley wrote:
> > On Mon, Jun 26, 2023 at 06:05:40PM +0200, Andrew Jones wrote:
> > > On Mon, Jun 26, 2023 at 04:51:29PM +0100, Conor Dooley wrote:
> > > > On Mon, Jun 26, 2023 at 05:14:15PM +0200, Andrew Jones wrote:
> > > > > On Mon, Jun 26, 2023 at 12:19:39PM +0100, Conor Dooley wrote:

> > > > One of the few things I know does parsing of /proc/cpuinfo is:
> > > > https://github.com/google/cpu_features/blob/main/src/impl_riscv_linux.c
> > > > and that doesn't seem to care about the mmu, but does rely on
> > > > vendor/uarch ordering.
> > > > 
> > > > Makes me wonder, does ACPI break things by leaving out uarch/vendor
> > > > fields, if there is something that expects them to exist? We should
> > > > not intentionally break stuff in /proc/cpuinfo, but can't say I feel any
> > > > sympathy for naively parsing it.
> > > 
> > > Yes, it would be nice for ACPI to be consistent. I'm not sure what can be
> > > done about that.
> > 
> > Print "unknown", until there's a way of passing the info?
> > Speaking of being an eejit, adding new fields to the file would probably
> > break some really naive parsers & quite frankly that sort of thing can
> > keep the pieces IMO. Ditto if adding more extensions breaks someone that
> > expects _zicbom_zicboz that breaks when _zicbop is slid into the middle.

> Instead of unknown, could you print "risc-v" or "riscv"?

I don't really see how that is better. "riscv" is not the uarch or
vendor. If we don't know, we should either say we don't know or omit the
field IMO.

Cheers,
Conor.

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

Re: [PATCH v1 1/9] RISC-V: don't parse dt/acpi isa string to get rv32/rv64

[Sunil V L]

On Tue, Jun 27, 2023 at 09:51:05AM +0100, Conor Dooley wrote:
> On Tue, Jun 27, 2023 at 01:32:23PM +0530, Sunil V L wrote:
> > On Mon, Jun 26, 2023 at 05:16:09PM +0100, Conor Dooley wrote:
> > > On Mon, Jun 26, 2023 at 06:05:40PM +0200, Andrew Jones wrote:
> > > > On Mon, Jun 26, 2023 at 04:51:29PM +0100, Conor Dooley wrote:
> > > > > On Mon, Jun 26, 2023 at 05:14:15PM +0200, Andrew Jones wrote:
> > > > > > On Mon, Jun 26, 2023 at 12:19:39PM +0100, Conor Dooley wrote:
> 
> > > > > One of the few things I know does parsing of /proc/cpuinfo is:
> > > > > https://github.com/google/cpu_features/blob/main/src/impl_riscv_linux.c
> > > > > and that doesn't seem to care about the mmu, but does rely on
> > > > > vendor/uarch ordering.
> > > > > 
> > > > > Makes me wonder, does ACPI break things by leaving out uarch/vendor
> > > > > fields, if there is something that expects them to exist? We should
> > > > > not intentionally break stuff in /proc/cpuinfo, but can't say I feel any
> > > > > sympathy for naively parsing it.
> > > > 
> > > > Yes, it would be nice for ACPI to be consistent. I'm not sure what can be
> > > > done about that.
> > > 
> > > Print "unknown", until there's a way of passing the info?
> > > Speaking of being an eejit, adding new fields to the file would probably
> > > break some really naive parsers & quite frankly that sort of thing can
> > > keep the pieces IMO. Ditto if adding more extensions breaks someone that
> > > expects _zicbom_zicboz that breaks when _zicbop is slid into the middle.
> 
> > Instead of unknown, could you print "risc-v" or "riscv"?
> 
> I don't really see how that is better. "riscv" is not the uarch or
> vendor. If we don't know, we should either say we don't know or omit the
> field IMO.
> 
Okay. Makes sense. In that case, I prefer to skip.
uarch is anyway printed using other ways. Even in DT, this is not
printed for generic cpus having compatible string riscv. So, consumers
should expect it not being printed.

Thanks,
Sunil

Re: [PATCH v1 7/9] RISC-V: split riscv_fill_hwcap() in 3

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 670 bytes --]

On Mon, Jun 26, 2023 at 06:17:51PM +0200, Andrew Jones wrote:
> > +void __init riscv_fill_hwcap(void)
> > +{
> > +	struct device_node *node;
> > +	const char *isa;
> > +	char print_str[NUM_ALPHA_EXTS + 1];
> > +	int i, j, rc;
> > +	unsigned long isa2hwcap[26] = {0};
> > +	struct acpi_table_header *rhct;
> > +	acpi_status status;
> > +	unsigned int cpu;
> 
> I see all these unused variables get removed in the next patch, but they
> should get removed here, lest they trigger some warnings and bots come
> after you!

Funnily enough, I'd pushed this out for LKP and it never complained
about the unused variables - but my own stuff on patchwork did.

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

Re: [PATCH v1 6/9] RISC-V: add single letter extensions to riscv_isa_ext

[Evan Green]

On Mon, Jun 26, 2023 at 4:21 AM Conor Dooley <conor.dooley@microchip.com> wrote:
>
> So that riscv_fill_hwcap() can use riscv_isa_ext to probe for single
> letter extensions, add them to it. riscv_isa_ext_data grows a new
> member, signifying whether an extension is multi-letter & thus requiring
> special handling.
> As a result, what gets spat out in /proc/cpuinfo will become borked, as
> single letter extensions will be printed as part of the base extensions
> and while printing from riscv_isa_arr. Take the opportunity to unify the
> printing of the isa string, using the new member of riscv_isa_ext_data
> in the process.
>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  arch/riscv/include/asm/hwcap.h |  1 +
>  arch/riscv/kernel/cpu.c        | 36 ++++++----------------
>  arch/riscv/kernel/cpufeature.c | 56 +++++++++++++++++++++-------------
>  3 files changed, 46 insertions(+), 47 deletions(-)
>
> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> index a35bee219dd7..6ad896dc4342 100644
> --- a/arch/riscv/include/asm/hwcap.h
> +++ b/arch/riscv/include/asm/hwcap.h
> @@ -77,6 +77,7 @@ unsigned long riscv_get_elf_hwcap(void);
>  struct riscv_isa_ext_data {
>         const unsigned int id;
>         const char *name;
> +       const bool multi_letter;

Instead of defining a new member, could we just infer this by making a
macro like #define MULTI_LETTER(name) (name[0] && name[1])?

Re: [PATCH v1 6/9] RISC-V: add single letter extensions to riscv_isa_ext

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1733 bytes --]

On Wed, Jun 28, 2023 at 10:33:20AM -0700, Evan Green wrote:
> On Mon, Jun 26, 2023 at 4:21 AM Conor Dooley <conor.dooley@microchip.com> wrote:
> >
> > So that riscv_fill_hwcap() can use riscv_isa_ext to probe for single
> > letter extensions, add them to it. riscv_isa_ext_data grows a new
> > member, signifying whether an extension is multi-letter & thus requiring
> > special handling.
> > As a result, what gets spat out in /proc/cpuinfo will become borked, as
> > single letter extensions will be printed as part of the base extensions
> > and while printing from riscv_isa_arr. Take the opportunity to unify the
> > printing of the isa string, using the new member of riscv_isa_ext_data
> > in the process.
> >
> > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> > ---
> >  arch/riscv/include/asm/hwcap.h |  1 +
> >  arch/riscv/kernel/cpu.c        | 36 ++++++----------------
> >  arch/riscv/kernel/cpufeature.c | 56 +++++++++++++++++++++-------------
> >  3 files changed, 46 insertions(+), 47 deletions(-)
> >
> > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> > index a35bee219dd7..6ad896dc4342 100644
> > --- a/arch/riscv/include/asm/hwcap.h
> > +++ b/arch/riscv/include/asm/hwcap.h
> > @@ -77,6 +77,7 @@ unsigned long riscv_get_elf_hwcap(void);
> >  struct riscv_isa_ext_data {
> >         const unsigned int id;
> >         const char *name;
> > +       const bool multi_letter;
> 
> Instead of defining a new member, could we just infer this by making a
> macro like #define MULTI_LETTER(name) (name[0] && name[1])?

Or don't even try to be clever like this and just call strnlen on the
name & check if it is 1? It's only used in 2 places.

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

Re: [PATCH v1 6/9] RISC-V: add single letter extensions to riscv_isa_ext

[Evan Green]

On Wed, Jun 28, 2023 at 10:43 AM Conor Dooley <conor@kernel.org> wrote:
>
> On Wed, Jun 28, 2023 at 10:33:20AM -0700, Evan Green wrote:
> > On Mon, Jun 26, 2023 at 4:21 AM Conor Dooley <conor.dooley@microchip.com> wrote:
> > >
> > > So that riscv_fill_hwcap() can use riscv_isa_ext to probe for single
> > > letter extensions, add them to it. riscv_isa_ext_data grows a new
> > > member, signifying whether an extension is multi-letter & thus requiring
> > > special handling.
> > > As a result, what gets spat out in /proc/cpuinfo will become borked, as
> > > single letter extensions will be printed as part of the base extensions
> > > and while printing from riscv_isa_arr. Take the opportunity to unify the
> > > printing of the isa string, using the new member of riscv_isa_ext_data
> > > in the process.
> > >
> > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> > > ---
> > >  arch/riscv/include/asm/hwcap.h |  1 +
> > >  arch/riscv/kernel/cpu.c        | 36 ++++++----------------
> > >  arch/riscv/kernel/cpufeature.c | 56 +++++++++++++++++++++-------------
> > >  3 files changed, 46 insertions(+), 47 deletions(-)
> > >
> > > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> > > index a35bee219dd7..6ad896dc4342 100644
> > > --- a/arch/riscv/include/asm/hwcap.h
> > > +++ b/arch/riscv/include/asm/hwcap.h
> > > @@ -77,6 +77,7 @@ unsigned long riscv_get_elf_hwcap(void);
> > >  struct riscv_isa_ext_data {
> > >         const unsigned int id;
> > >         const char *name;
> > > +       const bool multi_letter;
> >
> > Instead of defining a new member, could we just infer this by making a
> > macro like #define MULTI_LETTER(name) (name[0] && name[1])?
>
> Or don't even try to be clever like this and just call strnlen on the
> name & check if it is 1? It's only used in 2 places.

Sounds good to me!