[PATCH v2 00/10] RISC-V: Probe DT extension support using riscv,isa-extensions & riscv,isa-base

[PATCH v2 00/10] 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. Note that riscv,isa will still be used in the
absence of the new properties. Palmer suggested adding a Kconfig option
to turn off the fallback for DT, which I have gone and done. It's locked
behind the NONPORTABLE option for good reason.

In v2, I've also come up with a more reasonable name for the new
function I added & fixed up various comments from Drew and Evan.

Cheers,
Conor.

[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 (9):
  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()
  RISC-V: provide a Kconfig option to disable parsing "riscv,isa"

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

 arch/riscv/Kconfig             |  16 ++
 arch/riscv/include/asm/hwcap.h |  16 +-
 arch/riscv/kernel/cpu.c        | 158 +++-------
 arch/riscv/kernel/cpufeature.c | 507 +++++++++++++++++++++------------
 4 files changed, 398 insertions(+), 299 deletions(-)

-- 
2.40.1

[PATCH v2 01/10] 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>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
Co-developed-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
Changes in v2:
- Delete the whole else & pull print_mmu() above it, since that's common
  code now
---
 arch/riscv/kernel/cpu.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
index a2fc952318e9..2fb5e8e1df52 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,27 +319,21 @@ 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);
+		print_mmu(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) &&
 		    strcmp(compat, "riscv"))
 			seq_printf(m, "uarch\t\t: %s\n", compat);
 
 		of_node_put(node);
-	} else {
-		if (!acpi_get_riscv_isa(NULL, cpu_id, &isa))
-			print_isa(m, isa);
-
-		print_mmu(m);
 	}
 
 	seq_printf(m, "mvendorid\t: 0x%lx\n", ci->mvendorid);
-- 
2.40.1

Re: [PATCH v2 01/10] RISC-V: don't parse dt/acpi isa string to get rv32/rv64

[Evan Green]

On Thu, Jun 29, 2023 at 1:29 AM Conor Dooley <conor.dooley@microchip.com> 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>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> Co-developed-by: Conor Dooley <conor.dooley@microchip.com>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
> Changes in v2:
> - Delete the whole else & pull print_mmu() above it, since that's common
>   code now
> ---
>  arch/riscv/kernel/cpu.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
>
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index a2fc952318e9..2fb5e8e1df52 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,27 +319,21 @@ 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);
> +               print_mmu(m);

Did the indent get wonky here or am I just seeing it wrong because gmail?
Otherwise:

Reviewed-by: Evan Green <evan@rivosinc.com>

>
>         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) &&
>                     strcmp(compat, "riscv"))
>                         seq_printf(m, "uarch\t\t: %s\n", compat);
>
>                 of_node_put(node);
> -       } else {
> -               if (!acpi_get_riscv_isa(NULL, cpu_id, &isa))
> -                       print_isa(m, isa);
> -
> -               print_mmu(m);
>         }
>
>         seq_printf(m, "mvendorid\t: 0x%lx\n", ci->mvendorid);
> --
> 2.40.1
>

Re: [PATCH v2 01/10] RISC-V: don't parse dt/acpi isa string to get rv32/rv64

[Conor Dooley]

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

On Thu, Jun 29, 2023 at 04:10:48PM -0700, Evan Green wrote:

> > +               print_mmu(m);
> 
> Did the indent get wonky here or am I just seeing it wrong because gmail?

Nope, you're right. Thanks!

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

[PATCH v2 02/10] 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. When this code was first added, back in
commit a9b202606c69 ("RISC-V: Improve /proc/cpuinfo output for ISA
extensions"), the array was empty and needed a dummy item & thus there
could be no extensions present. When the first multi-letter ones did
get added, it was Sscofpmf - which didn't have a Kconfig symbol to
disable it.

Remove this check, as it has been redundant since Sscofpmf was added.

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
Changes in v2:
- Reword commit message to explain why this can be dropped
---
 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 2fb5e8e1df52..ddd7634e4c1d 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 v2 03/10] 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.

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
Changes in v2:
- Drop the empty element from the end of the array, it was adding a bug
  anyway as I was not decrementing the result of ARRAY_SIZE() by one.
  Likely I meant to drop it originally and forgot, as dropping the
  decrement was intentional.
---
 arch/riscv/include/asm/hwcap.h |  3 ++
 arch/riscv/kernel/cpu.c        | 75 +---------------------------------
 arch/riscv/kernel/cpufeature.c | 67 ++++++++++++++++++++++++++++++
 3 files changed, 72 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 ddd7634e4c1d..269a32ceb595 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..fb476153fffc 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -99,6 +99,73 @@ 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),
+};
+
+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

Re: [PATCH v2 03/10] RISC-V: shunt isa_ext_arr to cpufeature.c

[Evan Green]

On Thu, Jun 29, 2023 at 1:30 AM Conor Dooley <conor.dooley@microchip.com> 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.
>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>

Reviewed-by: Evan Green <evan@rivosinc.com>

Re: [PATCH v2 03/10] RISC-V: shunt isa_ext_arr to cpufeature.c

[Andrew Jones]

On Thu, Jun 29, 2023 at 09:28:49AM +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.
> 
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
> Changes in v2:
> - Drop the empty element from the end of the array, it was adding a bug
>   anyway as I was not decrementing the result of ARRAY_SIZE() by one.
>   Likely I meant to drop it originally and forgot, as dropping the
>   decrement was intentional.

I don't think v1 introduced a bug, because you changed the condition to
'<' from '<=' to account for not subtracting one. Both the original code
and v1 were doing a useless __riscv_isa_extension_available() check of
RISCV_ISA_EXT_MAX, though, which this version removes.

Thanks,
drew


> ---
>  arch/riscv/include/asm/hwcap.h |  3 ++
>  arch/riscv/kernel/cpu.c        | 75 +---------------------------------
>  arch/riscv/kernel/cpufeature.c | 67 ++++++++++++++++++++++++++++++
>  3 files changed, 72 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 ddd7634e4c1d..269a32ceb595 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..fb476153fffc 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -99,6 +99,73 @@ 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),
> +};
> +
> +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 v2 04/10] 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.

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
Changes in v2:
- Delete the now unused definition
---
 arch/riscv/include/asm/hwcap.h |  7 ++-----
 arch/riscv/kernel/cpu.c        |  5 +++--
 arch/riscv/kernel/cpufeature.c | 26 +++++++-------------------
 3 files changed, 12 insertions(+), 26 deletions(-)

diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index 7a57e6109aef..2460ac2fc7ed 100644
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -55,7 +55,6 @@
 #define RISCV_ISA_EXT_ZIHPM		42
 
 #define RISCV_ISA_EXT_MAX		64
-#define RISCV_ISA_EXT_NAME_LEN_MAX	32
 
 #ifdef CONFIG_RISCV_M_MODE
 #define RISCV_ISA_EXT_SxAIA		RISCV_ISA_EXT_SMAIA
@@ -70,10 +69,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 269a32ceb595..c89abf8ef6de 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 fb476153fffc..6d8cd45af723 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
@@ -366,20 +365,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 v2 05/10] 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.

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
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 2460ac2fc7ed..a20e4ade1b53 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 v2 06/10] 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.
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.

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
Changes in v2:
- Drop the multi_letter member, in exchange for calling strnlen() in two
  places.
---
 arch/riscv/kernel/cpu.c        | 37 ++++++++++------------------------
 arch/riscv/kernel/cpufeature.c | 13 ++++++++++++
 2 files changed, 24 insertions(+), 26 deletions(-)

diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
index c89abf8ef6de..d0dfd88221df 100644
--- a/arch/riscv/kernel/cpu.c
+++ b/arch/riscv/kernel/cpu.c
@@ -160,41 +160,26 @@ 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;
+
+		/* Only multi-letter extensions are split by underscores */
+		if (strnlen(riscv_isa_ext[i].name, 2) != 1)
+			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 6d8cd45af723..bf7e8e8852f0 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -144,6 +144,19 @@ 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(i, RISCV_ISA_EXT_i),
+	__RISCV_ISA_EXT_DATA(m, RISCV_ISA_EXT_m),
+	__RISCV_ISA_EXT_DATA(a, RISCV_ISA_EXT_a),
+	__RISCV_ISA_EXT_DATA(f, RISCV_ISA_EXT_f),
+	__RISCV_ISA_EXT_DATA(d, RISCV_ISA_EXT_d),
+	__RISCV_ISA_EXT_DATA(q, RISCV_ISA_EXT_q),
+	__RISCV_ISA_EXT_DATA(c, RISCV_ISA_EXT_c),
+	__RISCV_ISA_EXT_DATA(b, RISCV_ISA_EXT_b),
+	__RISCV_ISA_EXT_DATA(k, RISCV_ISA_EXT_k),
+	__RISCV_ISA_EXT_DATA(j, RISCV_ISA_EXT_j),
+	__RISCV_ISA_EXT_DATA(p, RISCV_ISA_EXT_p),
+	__RISCV_ISA_EXT_DATA(v, RISCV_ISA_EXT_v),
+	__RISCV_ISA_EXT_DATA(h, RISCV_ISA_EXT_h),
 	__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),
-- 
2.40.1

Re: [PATCH v2 06/10] RISC-V: add single letter extensions to riscv_isa_ext

[Evan Green]

On Thu, Jun 29, 2023 at 1:30 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.
> 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.
>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>

Reviewed-by: Evan Green <evan@rivosinc.com>

[PATCH v2 07/10] 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.

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
Changes in v2:
- Drop unused variables
---
 arch/riscv/kernel/cpufeature.c | 345 +++++++++++++++++----------------
 1 file changed, 177 insertions(+), 168 deletions(-)

diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index bf7e8e8852f0..41aedeaecb61 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -178,29 +178,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))
@@ -232,158 +375,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
@@ -420,6 +412,23 @@ void __init riscv_fill_hwcap(void)
 
 	if (!acpi_disabled && rhct)
 		acpi_put_table((struct acpi_table_header *)rhct);
+}
+
+void __init riscv_fill_hwcap(void)
+{
+	char print_str[NUM_ALPHA_EXTS + 1];
+	int i, j;
+	unsigned long isa2hwcap[26] = {0};
+
+	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 v2 08/10] 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 for all users, however this may not be the case for all
extensions, based on how the dt-binding is written.
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.

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
Changes in v2:
- Pick a more suitable function name than fill_hwcap_new()
- Actually use the property member to read from the DT
---
 arch/riscv/include/asm/hwcap.h |  1 +
 arch/riscv/kernel/cpufeature.c | 76 ++++++++++++++++++++++++++++++++--
 2 files changed, 73 insertions(+), 4 deletions(-)

diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index a20e4ade1b53..e3cda14a486b 100644
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -76,6 +76,7 @@ unsigned long riscv_get_elf_hwcap(void);
 struct riscv_isa_ext_data {
 	const unsigned int id;
 	const char *name;
+	const char *property;
 };
 
 extern const struct riscv_isa_ext_data riscv_isa_ext[];
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 41aedeaecb61..2c4503fa984f 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) {	\
 	.name = #_name,				\
+	.property = #_name,			\
 	.id = _id,				\
 }
 
@@ -414,11 +415,67 @@ 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_from_ext_list(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].property) < 0)
+				continue;
+
+			if (!riscv_isa_extension_check(riscv_isa_ext[i].id))
+				continue;
+
+			/* Only single letter extensions get set in hwcap */
+			if (strnlen(riscv_isa_ext[i].name, 2) == 1)
+				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)
 {
 	char print_str[NUM_ALPHA_EXTS + 1];
-	int i, j;
 	unsigned long isa2hwcap[26] = {0};
+	int i, j;
 
 	isa2hwcap['i' - 'a'] = COMPAT_HWCAP_ISA_I;
 	isa2hwcap['m' - 'a'] = COMPAT_HWCAP_ISA_M;
@@ -428,10 +485,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_from_ext_list(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 v2 09/10] 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".

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
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 d0dfd88221df..9a4f4a23afcd 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

[PATCH v2 10/10] RISC-V: provide a Kconfig option to disable parsing "riscv,isa"

[Conor Dooley]

As it says on the tin, provide a Kconfig option to disabling parsing the
"riscv,isa" devicetree property. Hide the option behind NONPORTABLE so
that only those willing to keep the pieces enable it, and make sure the
default kernel contains the fallback code.

Suggested-by: Palmer Dabbelt <palmer@rivosinc.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 arch/riscv/Kconfig             | 16 ++++++++++++++++
 arch/riscv/kernel/cpu.c        |  3 +++
 arch/riscv/kernel/cpufeature.c |  2 +-
 3 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 1d39efe2b940..0e1909ac5947 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -291,6 +291,22 @@ config NONPORTABLE
 
 	  If unsure, say N.
 
+config NO_RISCV_ISA_FALLBACK
+	bool "Permit falling back to parsing riscv,isa for extension support"
+	depends on NONPORTABLE
+	help
+	  Parsing the "riscv,isa" devicetree property has been deprecated and
+	  replaced by a list of explicitly defined strings. For compatibility
+	  with existing platforms, the kernel will fall back to parsing the
+	  "riscv,isa" property if the replacements are not found.
+
+	  Selecting Y here will result in a kernel without this fallback, and
+	  will not work on platforms where the devicetree does not contain the
+	  replacement 	  properties of "riscv,isa-base" and
+	  "riscv,isa-extensions". Please see the dt-binding, located at
+	  Documentation/devicetree/bindings/riscv/extensions.yaml for details
+	  on the replacement properties.
+
 choice
 	prompt "Base ISA"
 	default ARCH_RV64I
diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
index 9a4f4a23afcd..86a1d98b8b3b 100644
--- a/arch/riscv/kernel/cpu.c
+++ b/arch/riscv/kernel/cpu.c
@@ -81,6 +81,9 @@ int riscv_early_of_processor_hartid(struct device_node *node, unsigned long *har
 	return 0;
 
 old_interface:
+	if (IS_ENABLED(CONFIG_NO_RISCV_ISA_FALLBACK))
+		return -ENODEV;
+
 	if (of_property_read_string(node, "riscv,isa", &isa)) {
 		pr_warn("CPU with hartid=%lu has no \"riscv,isa-base\" or \"riscv,isa\" property\n",
 			*hart);
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 2c4503fa984f..f6fb18d2af84 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -490,7 +490,7 @@ void __init riscv_fill_hwcap(void)
 	} else {
 		int ret = riscv_fill_hwcap_from_ext_list(isa2hwcap);
 
-		if (ret) {
+		if (ret && !IS_ENABLED(CONFIG_NO_RISCV_ISA_FALLBACK)) {
 			pr_info("Falling back to deprecated \"riscv,isa\"\n");
 			riscv_fill_hwcap_from_isa_string(isa2hwcap);
 		}
-- 
2.40.1

Re: [PATCH v2 10/10] RISC-V: provide a Kconfig option to disable parsing "riscv,isa"

[Andrew Jones]

On Thu, Jun 29, 2023 at 09:28:56AM +0100, Conor Dooley wrote:
> As it says on the tin, provide a Kconfig option to disabling parsing the
> "riscv,isa" devicetree property. Hide the option behind NONPORTABLE so
> that only those willing to keep the pieces enable it, and make sure the
> default kernel contains the fallback code.
> 
> Suggested-by: Palmer Dabbelt <palmer@rivosinc.com>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  arch/riscv/Kconfig             | 16 ++++++++++++++++
>  arch/riscv/kernel/cpu.c        |  3 +++
>  arch/riscv/kernel/cpufeature.c |  2 +-
>  3 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 1d39efe2b940..0e1909ac5947 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -291,6 +291,22 @@ config NONPORTABLE
>  
>  	  If unsure, say N.
>  
> +config NO_RISCV_ISA_FALLBACK
> +	bool "Permit falling back to parsing riscv,isa for extension support"
> +	depends on NONPORTABLE
> +	help
> +	  Parsing the "riscv,isa" devicetree property has been deprecated and
> +	  replaced by a list of explicitly defined strings. For compatibility
> +	  with existing platforms, the kernel will fall back to parsing the
> +	  "riscv,isa" property if the replacements are not found.
> +
> +	  Selecting Y here will result in a kernel without this fallback, and
> +	  will not work on platforms where the devicetree does not contain the
> +	  replacement 	  properties of "riscv,isa-base" and
                     ^ spacing issue

> +	  "riscv,isa-extensions". Please see the dt-binding, located at
> +	  Documentation/devicetree/bindings/riscv/extensions.yaml for details
> +	  on the replacement properties.
> +
>  choice
>  	prompt "Base ISA"
>  	default ARCH_RV64I
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index 9a4f4a23afcd..86a1d98b8b3b 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -81,6 +81,9 @@ int riscv_early_of_processor_hartid(struct device_node *node, unsigned long *har
>  	return 0;
>  
>  old_interface:
> +	if (IS_ENABLED(CONFIG_NO_RISCV_ISA_FALLBACK))
> +		return -ENODEV;
> +
>  	if (of_property_read_string(node, "riscv,isa", &isa)) {
>  		pr_warn("CPU with hartid=%lu has no \"riscv,isa-base\" or \"riscv,isa\" property\n",
>  			*hart);
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 2c4503fa984f..f6fb18d2af84 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -490,7 +490,7 @@ void __init riscv_fill_hwcap(void)
>  	} else {
>  		int ret = riscv_fill_hwcap_from_ext_list(isa2hwcap);
>  
> -		if (ret) {
> +		if (ret && !IS_ENABLED(CONFIG_NO_RISCV_ISA_FALLBACK)) {
>  			pr_info("Falling back to deprecated \"riscv,isa\"\n");
>  			riscv_fill_hwcap_from_isa_string(isa2hwcap);
>  		}
> -- 
> 2.40.1
>

Should we also have a kernel command line option, 'isa_fallback', where
without this config the command line option is not necessary to fallback,
but, with this config, no fallback will be done unless 'isa_fallback' is
provided?

Thanks,
drew

Re: [PATCH v2 10/10] RISC-V: provide a Kconfig option to disable parsing "riscv,isa"

[Conor Dooley]

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

On Thu, Jun 29, 2023 at 11:31:33AM +0200, Andrew Jones wrote:
> On Thu, Jun 29, 2023 at 09:28:56AM +0100, Conor Dooley wrote:
> > As it says on the tin, provide a Kconfig option to disabling parsing the
> > "riscv,isa" devicetree property. Hide the option behind NONPORTABLE so
> > that only those willing to keep the pieces enable it, and make sure the
> > default kernel contains the fallback code.
> > 
> > Suggested-by: Palmer Dabbelt <palmer@rivosinc.com>
> > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> > ---
> >  arch/riscv/Kconfig             | 16 ++++++++++++++++
> >  arch/riscv/kernel/cpu.c        |  3 +++
> >  arch/riscv/kernel/cpufeature.c |  2 +-
> >  3 files changed, 20 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index 1d39efe2b940..0e1909ac5947 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -291,6 +291,22 @@ config NONPORTABLE
> >  
> >  	  If unsure, say N.
> >  
> > +config NO_RISCV_ISA_FALLBACK
> > +	bool "Permit falling back to parsing riscv,isa for extension support"
> > +	depends on NONPORTABLE
> > +	help
> > +	  Parsing the "riscv,isa" devicetree property has been deprecated and
> > +	  replaced by a list of explicitly defined strings. For compatibility
> > +	  with existing platforms, the kernel will fall back to parsing the
> > +	  "riscv,isa" property if the replacements are not found.
> > +
> > +	  Selecting Y here will result in a kernel without this fallback, and
> > +	  will not work on platforms where the devicetree does not contain the
> > +	  replacement 	  properties of "riscv,isa-base" and
>                      ^ spacing issue

Huh, weird. Given the tab followed by spaces, it must have snuck in
during reflow of the text after some rewording.
Wonder how I missed it, given that...

> Should we also have a kernel command line option, 'isa_fallback', where
> without this config the command line option is not necessary to fallback,
> but, with this config, no fallback will be done unless 'isa_fallback' is
> provided?

I don't know, maybe I have the wrong end of the stick but it feels a bit
premature for something that may never not be hidden behind NONPORTABLE?
Perhaps that could be left for a point in time where the default value
of the symbol changes, or the dependency on NONPORTABLE is removed?

Cheers,
Conor.

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

Re: [PATCH v2 10/10] RISC-V: provide a Kconfig option to disable parsing "riscv,isa"

[Andrew Jones]

On Thu, Jun 29, 2023 at 12:39:51PM +0100, Conor Dooley wrote:
> On Thu, Jun 29, 2023 at 11:31:33AM +0200, Andrew Jones wrote:
> > On Thu, Jun 29, 2023 at 09:28:56AM +0100, Conor Dooley wrote:
> > > As it says on the tin, provide a Kconfig option to disabling parsing the
> > > "riscv,isa" devicetree property. Hide the option behind NONPORTABLE so
> > > that only those willing to keep the pieces enable it, and make sure the
> > > default kernel contains the fallback code.
> > > 
> > > Suggested-by: Palmer Dabbelt <palmer@rivosinc.com>
> > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> > > ---
> > >  arch/riscv/Kconfig             | 16 ++++++++++++++++
> > >  arch/riscv/kernel/cpu.c        |  3 +++
> > >  arch/riscv/kernel/cpufeature.c |  2 +-
> > >  3 files changed, 20 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > > index 1d39efe2b940..0e1909ac5947 100644
> > > --- a/arch/riscv/Kconfig
> > > +++ b/arch/riscv/Kconfig
> > > @@ -291,6 +291,22 @@ config NONPORTABLE
> > >  
> > >  	  If unsure, say N.
> > >  
> > > +config NO_RISCV_ISA_FALLBACK
> > > +	bool "Permit falling back to parsing riscv,isa for extension support"
> > > +	depends on NONPORTABLE
> > > +	help
> > > +	  Parsing the "riscv,isa" devicetree property has been deprecated and
> > > +	  replaced by a list of explicitly defined strings. For compatibility
> > > +	  with existing platforms, the kernel will fall back to parsing the
> > > +	  "riscv,isa" property if the replacements are not found.
> > > +
> > > +	  Selecting Y here will result in a kernel without this fallback, and
> > > +	  will not work on platforms where the devicetree does not contain the
> > > +	  replacement 	  properties of "riscv,isa-base" and
> >                      ^ spacing issue
> 
> Huh, weird. Given the tab followed by spaces, it must have snuck in
> during reflow of the text after some rewording.
> Wonder how I missed it, given that...
> 
> > Should we also have a kernel command line option, 'isa_fallback', where
> > without this config the command line option is not necessary to fallback,
> > but, with this config, no fallback will be done unless 'isa_fallback' is
> > provided?
> 
> I don't know, maybe I have the wrong end of the stick but it feels a bit
> premature for something that may never not be hidden behind NONPORTABLE?
> Perhaps that could be left for a point in time where the default value
> of the symbol changes, or the dependency on NONPORTABLE is removed?
> 

With the command line option, we could consider dropping NONPORTABLE (but
still default off this config). What I'm thinking is that if we want to
encourage the adoption of the new format, then there should be a bit of
pain when it's not used, but not enough pain to risk rebellion. So,

 * defconfig builds will silently/painlessly fallback

 * builds that want to encourage adoption enable this config and will
   fail to boot when they don't get what they want and don't have the
   command line option

 * users still working through the growing pains can manage when
   the boot fails, and when it doesn't, with the command line

Thanks,
drew

Re: [PATCH v2 10/10] RISC-V: provide a Kconfig option to disable parsing "riscv,isa"

[Conor Dooley]

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

On Thu, Jun 29, 2023 at 03:53:08PM +0200, Andrew Jones wrote:
> On Thu, Jun 29, 2023 at 12:39:51PM +0100, Conor Dooley wrote:
> > On Thu, Jun 29, 2023 at 11:31:33AM +0200, Andrew Jones wrote:
> > > On Thu, Jun 29, 2023 at 09:28:56AM +0100, Conor Dooley wrote:
> > > > As it says on the tin, provide a Kconfig option to disabling parsing the
> > > > "riscv,isa" devicetree property. Hide the option behind NONPORTABLE so
> > > > that only those willing to keep the pieces enable it, and make sure the
> > > > default kernel contains the fallback code.
> > > > 
> > > > Suggested-by: Palmer Dabbelt <palmer@rivosinc.com>
> > > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> > > > ---
> > > >  arch/riscv/Kconfig             | 16 ++++++++++++++++
> > > >  arch/riscv/kernel/cpu.c        |  3 +++
> > > >  arch/riscv/kernel/cpufeature.c |  2 +-
> > > >  3 files changed, 20 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > > > index 1d39efe2b940..0e1909ac5947 100644
> > > > --- a/arch/riscv/Kconfig
> > > > +++ b/arch/riscv/Kconfig
> > > > @@ -291,6 +291,22 @@ config NONPORTABLE
> > > >  
> > > >  	  If unsure, say N.
> > > >  
> > > > +config NO_RISCV_ISA_FALLBACK
> > > > +	bool "Permit falling back to parsing riscv,isa for extension support"
> > > > +	depends on NONPORTABLE
> > > > +	help
> > > > +	  Parsing the "riscv,isa" devicetree property has been deprecated and
> > > > +	  replaced by a list of explicitly defined strings. For compatibility
> > > > +	  with existing platforms, the kernel will fall back to parsing the
> > > > +	  "riscv,isa" property if the replacements are not found.
> > > > +
> > > > +	  Selecting Y here will result in a kernel without this fallback, and
> > > > +	  will not work on platforms where the devicetree does not contain the
> > > > +	  replacement 	  properties of "riscv,isa-base" and
> > >                      ^ spacing issue
> > 
> > Huh, weird. Given the tab followed by spaces, it must have snuck in
> > during reflow of the text after some rewording.
> > Wonder how I missed it, given that...
> > 
> > > Should we also have a kernel command line option, 'isa_fallback', where
> > > without this config the command line option is not necessary to fallback,
> > > but, with this config, no fallback will be done unless 'isa_fallback' is
> > > provided?
> > 
> > I don't know, maybe I have the wrong end of the stick but it feels a bit
> > premature for something that may never not be hidden behind NONPORTABLE?
> > Perhaps that could be left for a point in time where the default value
> > of the symbol changes, or the dependency on NONPORTABLE is removed?
> > 
> 
> With the command line option, we could consider dropping NONPORTABLE (but
> still default off this config). What I'm thinking is that if we want to
> encourage the adoption of the new format, then there should be a bit of
> pain when it's not used, but not enough pain to risk rebellion. So,
> 
>  * defconfig builds will silently/painlessly fallback
> 
>  * builds that want to encourage adoption enable this config and will
>    fail to boot when they don't get what they want and don't have the
>    command line option
> 
>  * users still working through the growing pains can manage when
>    the boot fails, and when it doesn't, with the command line

So, something like the following squashed in? I inverted the config
option, seemed more natural that way.

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index d910fba25f2c..6c0d0bc06048 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -5437,6 +5437,13 @@
 			[KNL] Disable ring 3 MONITOR/MWAIT feature on supported
 			CPUs.
 
+	riscv_isa_fallback [RISCV]
+			Fall back to detecting extension support using the
+			"riscv,isa" property on devicetree systems when the
+			replacement properties are not found, on kernels where
+			RISCV_ISA_FALLBACK is not enabled. See the Kconfig entry
+			for RISCV_ISA_FALLBACK.
+
 	ro		[KNL] Mount root device read-only on boot
 
 	rodata=		[KNL]
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 0370713ad965..a9a473b67182 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -291,22 +291,6 @@ config NONPORTABLE
 
 	  If unsure, say N.
 
-config NO_RISCV_ISA_FALLBACK
-	bool "Permit falling back to parsing riscv,isa for extension support"
-	depends on NONPORTABLE
-	help
-	  Parsing the "riscv,isa" devicetree property has been deprecated and
-	  replaced by a list of explicitly defined strings. For compatibility
-	  with existing platforms, the kernel will fall back to parsing the
-	  "riscv,isa" property if the replacements are not found.
-
-	  Selecting Y here will result in a kernel without this fallback, and
-	  will not work on platforms where the devicetree does not contain the
-	  replacement properties of "riscv,isa-base" and "riscv,isa-extensions".
-	  Please see the dt-binding, located at
-	  Documentation/devicetree/bindings/riscv/extensions.yaml for details
-	  on the replacement properties.
-
 choice
 	prompt "Base ISA"
 	default ARCH_RV64I
@@ -857,6 +841,24 @@ config XIP_PHYS_ADDR
 	  be linked for and stored to.  This address is dependent on your
 	  own flash usage.
 
+config RISCV_ISA_FALLBACK
+	bool "Permit falling back to parsing riscv,isa for extension support by default"
+	default y
+	help
+	  Parsing the "riscv,isa" devicetree property has been deprecated and
+	  replaced by a list of explicitly defined strings. For compatibility
+	  with existing platforms, the kernel will fall back to parsing the
+	  "riscv,isa" property if the replacements are not found.
+
+	  Selecting N here will result in a kernel that does not use the
+	  fallback, unless the commandline "riscv_isa_fallback" parameter is
+	  present.
+
+	  Please see the dt-binding, located at
+	  Documentation/devicetree/bindings/riscv/extensions.yaml for details
+	  on the replacement properties of "riscv,isa-base" and
+	  "riscv,isa-extensions".
+
 endmenu # "Boot options"
 
 config BUILTIN_DTB
diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index e3cda14a486b..52fa607a1691 100644
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -81,6 +81,7 @@ struct riscv_isa_ext_data {
 
 extern const struct riscv_isa_ext_data riscv_isa_ext[];
 extern const size_t riscv_isa_ext_count;
+extern bool riscv_isa_fallback_cmdline;
 
 unsigned long riscv_isa_extension_base(const unsigned long *isa_bitmap);
 
diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
index 86a1d98b8b3b..1e4cbdedc7fc 100644
--- a/arch/riscv/kernel/cpu.c
+++ b/arch/riscv/kernel/cpu.c
@@ -81,7 +81,7 @@ int riscv_early_of_processor_hartid(struct device_node *node, unsigned long *har
 	return 0;
 
 old_interface:
-	if (IS_ENABLED(CONFIG_NO_RISCV_ISA_FALLBACK))
+	if (!IS_ENABLED(CONFIG_RISCV_ISA_FALLBACK) && !riscv_isa_fallback_cmdline)
 		return -ENODEV;
 
 	if (of_property_read_string(node, "riscv,isa", &isa)) {
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index f6fb18d2af84..5d848b7c1dde 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -471,6 +471,14 @@ static int __init riscv_fill_hwcap_from_ext_list(unsigned long *isa2hwcap)
 	return 0;
 }
 
+bool __initdata riscv_isa_fallback_cmdline = false;
+static int __init riscv_isa_fallback_setup(char *__unused)
+{
+	riscv_isa_fallback_cmdline = true;
+	return 1;
+}
+early_param("riscv_isa_fallback", riscv_isa_fallback_setup);
+
 void __init riscv_fill_hwcap(void)
 {
 	char print_str[NUM_ALPHA_EXTS + 1];
@@ -490,7 +498,7 @@ void __init riscv_fill_hwcap(void)
 	} else {
 		int ret = riscv_fill_hwcap_from_ext_list(isa2hwcap);
 
-		if (ret && !IS_ENABLED(CONFIG_NO_RISCV_ISA_FALLBACK)) {
+		if (ret && (IS_ENABLED(CONFIG_RISCV_ISA_FALLBACK) || riscv_isa_fallback_cmdline)) {
 			pr_info("Falling back to deprecated \"riscv,isa\"\n");
 			riscv_fill_hwcap_from_isa_string(isa2hwcap);
 		}


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

Re: [PATCH v2 10/10] RISC-V: provide a Kconfig option to disable parsing "riscv,isa"

[Palmer Dabbelt]

On Thu, 29 Jun 2023 13:20:55 PDT (-0700), Conor Dooley wrote:
> On Thu, Jun 29, 2023 at 03:53:08PM +0200, Andrew Jones wrote:
>> On Thu, Jun 29, 2023 at 12:39:51PM +0100, Conor Dooley wrote:
>> > On Thu, Jun 29, 2023 at 11:31:33AM +0200, Andrew Jones wrote:
>> > > On Thu, Jun 29, 2023 at 09:28:56AM +0100, Conor Dooley wrote:
>> > > > As it says on the tin, provide a Kconfig option to disabling parsing the
>> > > > "riscv,isa" devicetree property. Hide the option behind NONPORTABLE so
>> > > > that only those willing to keep the pieces enable it, and make sure the
>> > > > default kernel contains the fallback code.
>> > > > 
>> > > > Suggested-by: Palmer Dabbelt <palmer@rivosinc.com>
>> > > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
>> > > > ---
>> > > >  arch/riscv/Kconfig             | 16 ++++++++++++++++
>> > > >  arch/riscv/kernel/cpu.c        |  3 +++
>> > > >  arch/riscv/kernel/cpufeature.c |  2 +-
>> > > >  3 files changed, 20 insertions(+), 1 deletion(-)
>> > > > 
>> > > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>> > > > index 1d39efe2b940..0e1909ac5947 100644
>> > > > --- a/arch/riscv/Kconfig
>> > > > +++ b/arch/riscv/Kconfig
>> > > > @@ -291,6 +291,22 @@ config NONPORTABLE
>> > > >  
>> > > >  	  If unsure, say N.
>> > > >  
>> > > > +config NO_RISCV_ISA_FALLBACK
>> > > > +	bool "Permit falling back to parsing riscv,isa for extension support"
>> > > > +	depends on NONPORTABLE
>> > > > +	help
>> > > > +	  Parsing the "riscv,isa" devicetree property has been deprecated and
>> > > > +	  replaced by a list of explicitly defined strings. For compatibility
>> > > > +	  with existing platforms, the kernel will fall back to parsing the
>> > > > +	  "riscv,isa" property if the replacements are not found.
>> > > > +
>> > > > +	  Selecting Y here will result in a kernel without this fallback, and
>> > > > +	  will not work on platforms where the devicetree does not contain the
>> > > > +	  replacement 	  properties of "riscv,isa-base" and
>> > >                      ^ spacing issue
>> > 
>> > Huh, weird. Given the tab followed by spaces, it must have snuck in
>> > during reflow of the text after some rewording.
>> > Wonder how I missed it, given that...

I sometimes end up with these when reflowing in vim, the Kconfig help 
text indent confuses things (though many things confuse whatever vim 
reflowing I'm using, so I should probably find something better).

>> > > Should we also have a kernel command line option, 'isa_fallback', where
>> > > without this config the command line option is not necessary to fallback,
>> > > but, with this config, no fallback will be done unless 'isa_fallback' is
>> > > provided?
>> > 
>> > I don't know, maybe I have the wrong end of the stick but it feels a bit
>> > premature for something that may never not be hidden behind NONPORTABLE?
>> > Perhaps that could be left for a point in time where the default value
>> > of the symbol changes, or the dependency on NONPORTABLE is removed?
>> > 

I agree it might not be worth the work, but looks like that's done...

>> With the command line option, we could consider dropping NONPORTABLE (but
>> still default off this config). What I'm thinking is that if we want to
>> encourage the adoption of the new format, then there should be a bit of
>> pain when it's not used, but not enough pain to risk rebellion. So,
>> 
>>  * defconfig builds will silently/painlessly fallback
>> 
>>  * builds that want to encourage adoption enable this config and will
>>    fail to boot when they don't get what they want and don't have the
>>    command line option
>> 
>>  * users still working through the growing pains can manage when
>>    the boot fails, and when it doesn't, with the command line
>
> So, something like the following squashed in? I inverted the config
> option, seemed more natural that way.
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index d910fba25f2c..6c0d0bc06048 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -5437,6 +5437,13 @@
>  			[KNL] Disable ring 3 MONITOR/MWAIT feature on supported
>  			CPUs.
>  
> +	riscv_isa_fallback [RISCV]
> +			Fall back to detecting extension support using the
> +			"riscv,isa" property on devicetree systems when the
> +			replacement properties are not found, on kernels where
> +			RISCV_ISA_FALLBACK is not enabled. See the Kconfig entry
> +			for RISCV_ISA_FALLBACK.
> +
>  	ro		[KNL] Mount root device read-only on boot
>  
>  	rodata=		[KNL]
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 0370713ad965..a9a473b67182 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -291,22 +291,6 @@ config NONPORTABLE
>  
>  	  If unsure, say N.
>  
> -config NO_RISCV_ISA_FALLBACK
> -	bool "Permit falling back to parsing riscv,isa for extension support"
> -	depends on NONPORTABLE
> -	help
> -	  Parsing the "riscv,isa" devicetree property has been deprecated and
> -	  replaced by a list of explicitly defined strings. For compatibility
> -	  with existing platforms, the kernel will fall back to parsing the
> -	  "riscv,isa" property if the replacements are not found.
> -
> -	  Selecting Y here will result in a kernel without this fallback, and
> -	  will not work on platforms where the devicetree does not contain the
> -	  replacement properties of "riscv,isa-base" and "riscv,isa-extensions".
> -	  Please see the dt-binding, located at
> -	  Documentation/devicetree/bindings/riscv/extensions.yaml for details
> -	  on the replacement properties.
> -
>  choice
>  	prompt "Base ISA"
>  	default ARCH_RV64I
> @@ -857,6 +841,24 @@ config XIP_PHYS_ADDR
>  	  be linked for and stored to.  This address is dependent on your
>  	  own flash usage.
>  
> +config RISCV_ISA_FALLBACK
> +	bool "Permit falling back to parsing riscv,isa for extension support by default"
> +	default y

I think the only risk here is that randconfig will result in systems 
that don't boot, but we're already stuck with no meaningful base 
requirements.  Fixing that is a way bigger hurdle than this, so IMO it's 
fine.

> +	help
> +	  Parsing the "riscv,isa" devicetree property has been deprecated and
> +	  replaced by a list of explicitly defined strings. For compatibility
> +	  with existing platforms, the kernel will fall back to parsing the
> +	  "riscv,isa" property if the replacements are not found.
> +
> +	  Selecting N here will result in a kernel that does not use the
> +	  fallback, unless the commandline "riscv_isa_fallback" parameter is
> +	  present.
> +
> +	  Please see the dt-binding, located at
> +	  Documentation/devicetree/bindings/riscv/extensions.yaml for details
> +	  on the replacement properties of "riscv,isa-base" and
> +	  "riscv,isa-extensions".
> +
>  endmenu # "Boot options"
>  
>  config BUILTIN_DTB
> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> index e3cda14a486b..52fa607a1691 100644
> --- a/arch/riscv/include/asm/hwcap.h
> +++ b/arch/riscv/include/asm/hwcap.h
> @@ -81,6 +81,7 @@ struct riscv_isa_ext_data {
>  
>  extern const struct riscv_isa_ext_data riscv_isa_ext[];
>  extern const size_t riscv_isa_ext_count;
> +extern bool riscv_isa_fallback_cmdline;
>
>  unsigned long riscv_isa_extension_base(const unsigned long *isa_bitmap);
>  
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index 86a1d98b8b3b..1e4cbdedc7fc 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -81,7 +81,7 @@ int riscv_early_of_processor_hartid(struct device_node *node, unsigned long *har
>  	return 0;
>  
>  old_interface:
> -	if (IS_ENABLED(CONFIG_NO_RISCV_ISA_FALLBACK))
> +	if (!IS_ENABLED(CONFIG_RISCV_ISA_FALLBACK) && !riscv_isa_fallback_cmdline)
>  		return -ENODEV;
>  
>  	if (of_property_read_string(node, "riscv,isa", &isa)) {
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index f6fb18d2af84..5d848b7c1dde 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -471,6 +471,14 @@ static int __init riscv_fill_hwcap_from_ext_list(unsigned long *isa2hwcap)
>  	return 0;
>  }
>  
> +bool __initdata riscv_isa_fallback_cmdline = false;
> +static int __init riscv_isa_fallback_setup(char *__unused)

Maybe it's better to support =true and =false here?  Not sure it 
matters, we're already down a rabbit hole ;)

> +{
> +	riscv_isa_fallback_cmdline = true;
> +	return 1;
> +}
> +early_param("riscv_isa_fallback", riscv_isa_fallback_setup);

IMO it's cleaner to just set the default based on the Kconfig, that way 
there's only one thing to check.  We're already duplicating the "check 
both Kconfig and cmdline" logic twice, there's probably going to be 
others eventually.

> +
>  void __init riscv_fill_hwcap(void)
>  {
>  	char print_str[NUM_ALPHA_EXTS + 1];
> @@ -490,7 +498,7 @@ void __init riscv_fill_hwcap(void)
>  	} else {
>  		int ret = riscv_fill_hwcap_from_ext_list(isa2hwcap);
>  
> -		if (ret && !IS_ENABLED(CONFIG_NO_RISCV_ISA_FALLBACK)) {
> +		if (ret && (IS_ENABLED(CONFIG_RISCV_ISA_FALLBACK) || riscv_isa_fallback_cmdline)) {
>  			pr_info("Falling back to deprecated \"riscv,isa\"\n");
>  			riscv_fill_hwcap_from_isa_string(isa2hwcap);
>  		}

I haven't read the whole patch set, but at least having a nice error for 
why probing didn't happen seems like the right way to go here.  IIUC 
this is just silent when ISA string fallbacks are disabled and the new 
properties aren't there.

Sorry in advance if it's just somewhere outside the diff, though.

Re: [PATCH v2 10/10] RISC-V: provide a Kconfig option to disable parsing "riscv,isa"

[Conor Dooley]

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

On Thu, Jun 29, 2023 at 02:16:49PM -0700, Palmer Dabbelt wrote:
> On Thu, 29 Jun 2023 13:20:55 PDT (-0700), Conor Dooley wrote:
> > On Thu, Jun 29, 2023 at 03:53:08PM +0200, Andrew Jones wrote:
> > > On Thu, Jun 29, 2023 at 12:39:51PM +0100, Conor Dooley wrote:
> > > > On Thu, Jun 29, 2023 at 11:31:33AM +0200, Andrew Jones wrote:
> > > > > On Thu, Jun 29, 2023 at 09:28:56AM +0100, Conor Dooley wrote:
> > > > > > As it says on the tin, provide a Kconfig option to disabling parsing the
> > > > > > "riscv,isa" devicetree property. Hide the option behind NONPORTABLE so
> > > > > > that only those willing to keep the pieces enable it, and make sure the
> > > > > > default kernel contains the fallback code.
> > > > > > > > > Suggested-by: Palmer Dabbelt <palmer@rivosinc.com>
> > > > > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> > > > > > ---
> > > > > >  arch/riscv/Kconfig             | 16 ++++++++++++++++
> > > > > >  arch/riscv/kernel/cpu.c        |  3 +++
> > > > > >  arch/riscv/kernel/cpufeature.c |  2 +-
> > > > > >  3 files changed, 20 insertions(+), 1 deletion(-)
> > > > > > > > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > > > > > index 1d39efe2b940..0e1909ac5947 100644
> > > > > > --- a/arch/riscv/Kconfig
> > > > > > +++ b/arch/riscv/Kconfig
> > > > > > @@ -291,6 +291,22 @@ config NONPORTABLE
> > > > > >  > > >  	  If unsure, say N.
> > > > > >  > > > +config NO_RISCV_ISA_FALLBACK
> > > > > > +	bool "Permit falling back to parsing riscv,isa for extension support"
> > > > > > +	depends on NONPORTABLE
> > > > > > +	help
> > > > > > +	  Parsing the "riscv,isa" devicetree property has been deprecated and
> > > > > > +	  replaced by a list of explicitly defined strings. For compatibility
> > > > > > +	  with existing platforms, the kernel will fall back to parsing the
> > > > > > +	  "riscv,isa" property if the replacements are not found.
> > > > > > +
> > > > > > +	  Selecting Y here will result in a kernel without this fallback, and
> > > > > > +	  will not work on platforms where the devicetree does not contain the
> > > > > > +	  replacement 	  properties of "riscv,isa-base" and
> > > > >                      ^ spacing issue
> > > > > Huh, weird. Given the tab followed by spaces, it must have snuck
> > > in
> > > > during reflow of the text after some rewording.
> > > > Wonder how I missed it, given that...
> 
> I sometimes end up with these when reflowing in vim, the Kconfig help text
> indent confuses things (though many things confuse whatever vim reflowing
> I'm using, so I should probably find something better).

I did it by hand, I have no excuses.

> > > > > Should we also have a kernel command line option, 'isa_fallback', where
> > > > > without this config the command line option is not necessary to fallback,
> > > > > but, with this config, no fallback will be done unless 'isa_fallback' is
> > > > > provided?
> > > > > I don't know, maybe I have the wrong end of the stick but it
> > > feels a bit
> > > > premature for something that may never not be hidden behind NONPORTABLE?
> > > > Perhaps that could be left for a point in time where the default value
> > > > of the symbol changes, or the dependency on NONPORTABLE is removed?
> > > >
> 
> I agree it might not be worth the work, but looks like that's done...

Well, whether I did it or not doesn't mean it has to be included in the
series. The work wasn't all that much to be honest.

> > > With the command line option, we could consider dropping NONPORTABLE (but
> > > still default off this config). What I'm thinking is that if we want to
> > > encourage the adoption of the new format, then there should be a bit of
> > > pain when it's not used, but not enough pain to risk rebellion. So,
> > > 
> > >  * defconfig builds will silently/painlessly fallback
> > > 
> > >  * builds that want to encourage adoption enable this config and will
> > >    fail to boot when they don't get what they want and don't have the
> > >    command line option
> > > 
> > >  * users still working through the growing pains can manage when
> > >    the boot fails, and when it doesn't, with the command line
> > 
> > So, something like the following squashed in? I inverted the config
> > option, seemed more natural that way.
> > 
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > index d910fba25f2c..6c0d0bc06048 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -5437,6 +5437,13 @@
> >  			[KNL] Disable ring 3 MONITOR/MWAIT feature on supported
> >  			CPUs.
> > +	riscv_isa_fallback [RISCV]
> > +			Fall back to detecting extension support using the
> > +			"riscv,isa" property on devicetree systems when the
> > +			replacement properties are not found, on kernels where
> > +			RISCV_ISA_FALLBACK is not enabled. See the Kconfig entry
> > +			for RISCV_ISA_FALLBACK.
> > +
> >  	ro		[KNL] Mount root device read-only on boot
> >  	rodata=		[KNL]
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index 0370713ad965..a9a473b67182 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -291,22 +291,6 @@ config NONPORTABLE
> >  	  If unsure, say N.
> > -config NO_RISCV_ISA_FALLBACK
> > -	bool "Permit falling back to parsing riscv,isa for extension support"
> > -	depends on NONPORTABLE
> > -	help
> > -	  Parsing the "riscv,isa" devicetree property has been deprecated and
> > -	  replaced by a list of explicitly defined strings. For compatibility
> > -	  with existing platforms, the kernel will fall back to parsing the
> > -	  "riscv,isa" property if the replacements are not found.
> > -
> > -	  Selecting Y here will result in a kernel without this fallback, and
> > -	  will not work on platforms where the devicetree does not contain the
> > -	  replacement properties of "riscv,isa-base" and "riscv,isa-extensions".
> > -	  Please see the dt-binding, located at
> > -	  Documentation/devicetree/bindings/riscv/extensions.yaml for details
> > -	  on the replacement properties.
> > -
> >  choice
> >  	prompt "Base ISA"
> >  	default ARCH_RV64I
> > @@ -857,6 +841,24 @@ config XIP_PHYS_ADDR
> >  	  be linked for and stored to.  This address is dependent on your
> >  	  own flash usage.
> > +config RISCV_ISA_FALLBACK
> > +	bool "Permit falling back to parsing riscv,isa for extension support by default"
> > +	default y
> 
> I think the only risk here is that randconfig will result in systems that
> don't boot, but we're already stuck with no meaningful base requirements.
> Fixing that is a way bigger hurdle than this, so IMO it's fine.

Maybe I'm way off here, but I didn't think that we had to be worried
about randconfigs not booting, as it was a testing tool?

> > +	help
> > +	  Parsing the "riscv,isa" devicetree property has been deprecated and
> > +	  replaced by a list of explicitly defined strings. For compatibility
> > +	  with existing platforms, the kernel will fall back to parsing the
> > +	  "riscv,isa" property if the replacements are not found.
> > +
> > +	  Selecting N here will result in a kernel that does not use the
> > +	  fallback, unless the commandline "riscv_isa_fallback" parameter is
> > +	  present.
> > +
> > +	  Please see the dt-binding, located at
> > +	  Documentation/devicetree/bindings/riscv/extensions.yaml for details
> > +	  on the replacement properties of "riscv,isa-base" and
> > +	  "riscv,isa-extensions".
> > +
> >  endmenu # "Boot options"
> >  config BUILTIN_DTB
> > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> > index e3cda14a486b..52fa607a1691 100644
> > --- a/arch/riscv/include/asm/hwcap.h
> > +++ b/arch/riscv/include/asm/hwcap.h
> > @@ -81,6 +81,7 @@ struct riscv_isa_ext_data {
> >  extern const struct riscv_isa_ext_data riscv_isa_ext[];
> >  extern const size_t riscv_isa_ext_count;
> > +extern bool riscv_isa_fallback_cmdline;
> > 
> >  unsigned long riscv_isa_extension_base(const unsigned long *isa_bitmap);
> > diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> > index 86a1d98b8b3b..1e4cbdedc7fc 100644
> > --- a/arch/riscv/kernel/cpu.c
> > +++ b/arch/riscv/kernel/cpu.c
> > @@ -81,7 +81,7 @@ int riscv_early_of_processor_hartid(struct device_node *node, unsigned long *har
> >  	return 0;
> >  old_interface:
> > -	if (IS_ENABLED(CONFIG_NO_RISCV_ISA_FALLBACK))
> > +	if (!IS_ENABLED(CONFIG_RISCV_ISA_FALLBACK) && !riscv_isa_fallback_cmdline)
> >  		return -ENODEV;
> >  	if (of_property_read_string(node, "riscv,isa", &isa)) {
> > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > index f6fb18d2af84..5d848b7c1dde 100644
> > --- a/arch/riscv/kernel/cpufeature.c
> > +++ b/arch/riscv/kernel/cpufeature.c
> > @@ -471,6 +471,14 @@ static int __init riscv_fill_hwcap_from_ext_list(unsigned long *isa2hwcap)
> >  	return 0;
> >  }
> > +bool __initdata riscv_isa_fallback_cmdline = false;
> > +static int __init riscv_isa_fallback_setup(char *__unused)
> 
> Maybe it's better to support =true and =false here?  Not sure it matters,
> we're already down a rabbit hole ;)

Dunno, not implemented a cmdline param before. Seemed "cleaner" to check
for presence, don't really care so I'll adapt to w/e.

> > +{
> > +	riscv_isa_fallback_cmdline = true;
> > +	return 1;
> > +}
> > +early_param("riscv_isa_fallback", riscv_isa_fallback_setup);
> 
> IMO it's cleaner to just set the default based on the Kconfig, that way
> there's only one thing to check.  We're already duplicating the "check both
> Kconfig and cmdline" logic twice, there's probably going to be others
> eventually.

Sure.

> > +
> >  void __init riscv_fill_hwcap(void)
> >  {
> >  	char print_str[NUM_ALPHA_EXTS + 1];
> > @@ -490,7 +498,7 @@ void __init riscv_fill_hwcap(void)
> >  	} else {
> >  		int ret = riscv_fill_hwcap_from_ext_list(isa2hwcap);
> > -		if (ret && !IS_ENABLED(CONFIG_NO_RISCV_ISA_FALLBACK)) {
> > +		if (ret && (IS_ENABLED(CONFIG_RISCV_ISA_FALLBACK) || riscv_isa_fallback_cmdline)) {
> >  			pr_info("Falling back to deprecated \"riscv,isa\"\n");
> >  			riscv_fill_hwcap_from_isa_string(isa2hwcap);
> >  		}
> 
> I haven't read the whole patch set, but at least having a nice error for why
> probing didn't happen seems like the right way to go here.  IIUC this is
> just silent when ISA string fallbacks are disabled and the new properties
> aren't there.
> 

> Sorry in advance if it's just somewhere outside the diff, though.

It sorta is. You'll actually not even reach this code if probing fails.
Because riscv_early_of_processor_hartid(), which is called super early
on in of_parse_and_init_cpus(), checks whether there are any valid
harts (including checking for sufficient extension support to run a
kernel) we will hit a BUG() during setup_smp() & blow up really early:

[    0.000000] Linux version 6.4.0-rc1-00096-ge0097d2c62d5-dirty (conor@spud) (ClangBuiltLinux clang version 15.0.7 (/stuff/brsdk/llvm/clang 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a), ClangBuiltLinux LLD 15.0.7) #1 SMP PREEMPT @666
[    0.000000] Machine model: Microchip PolarFire-SoC Icicle Kit
[    0.000000] SBI specification v0.3 detected
[    0.000000] SBI implementation ID=0x1 Version=0x10000
[    0.000000] SBI TIME extension detected
[    0.000000] SBI IPI extension detected
[    0.000000] SBI RFENCE extension detected
[    0.000000] SBI SRST extension detected
[    0.000000] earlycon: ns16550a0 at MMIO32 0x0000000020100000 (options '115200n8')
[    0.000000] printk: bootconsole [ns16550a0] enabled
[    0.000000] printk: debug: skip boot console de-registration.
[    0.000000] efi: UEFI not found.
[    0.000000] OF: reserved mem: 0x00000000bfc00000..0x00000000bfffffff (4096 KiB) nomap non-reusable region@BFC00000
[    0.000000] Zone ranges:
[    0.000000]   DMA32    [mem 0x0000000080000000-0x00000000ffffffff]
[    0.000000]   Normal   [mem 0x0000000100000000-0x000000107fffffff]
[    0.000000] Movable zone start for each node
[    0.000000] Early memory node ranges
[    0.000000]   node   0: [mem 0x0000000080000000-0x00000000bfbfffff]
[    0.000000]   node   0: [mem 0x00000000bfc00000-0x00000000bfffffff]
[    0.000000]   node   0: [mem 0x0000001040000000-0x000000107fffffff]
[    0.000000] Initmem setup node 0 [mem 0x0000000080000000-0x000000107fffffff]
[    0.000000] SBI HSM extension detected
[    0.000000] CPU with hartid=0 is not available
[    0.000000] ------------[ cut here ]------------
[    0.000000] kernel BUG at arch/riscv/kernel/smpboot.c:174!
[    0.000000] Kernel BUG [#1]
[    0.000000] Modules linked in:
[    0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 6.4.0-rc1-00096-ge0097d2c62d5-dirty #1
[    0.000000] Hardware name: Microchip PolarFire-SoC Icicle Kit (DT)
[    0.000000] epc : of_parse_and_init_cpus+0x16c/0x16e
[    0.000000]  ra : of_parse_and_init_cpus+0x9a/0x16e
[    0.000000] epc : ffffffff80c04e0a ra : ffffffff80c04d38 sp : ffffffff81603e20
[    0.000000]  gp : ffffffff8182d658 tp : ffffffff81613f80 t0 : 000000000000006e
[    0.000000]  t1 : 0000000000000064 t2 : 0000000000000000 s0 : ffffffff81603e80
[    0.000000]  s1 : 0000000000000000 a0 : 0000000000000000 a1 : 0000000000000000
[    0.000000]  a2 : 0000000000000000 a3 : 0000000000000000 a4 : 0000000000000000
[    0.000000]  a5 : 0000000000001fff a6 : 0000000000001fff a7 : ffffffff816148b0
[    0.000000]  s2 : 0000000000000001 s3 : ffffffff81492a4c s4 : ffffffff81a4b090
[    0.000000]  s5 : ffffffff81506030 s6 : 0000000000000040 s7 : 0000000000000000
[    0.000000]  s8 : 00000000bfb6f046 s9 : 0000000000000001 s10: 0000000000000000
[    0.000000]  s11: 00000000bf389700 t3 : 0000000000000000 t4 : 0000000000000000
[    0.000000]  t5 : ffffffff824dd188 t6 : ffffffff824dd187
[    0.000000] status: 0000000200000100 badaddr: 0000000000000000 cause: 0000000000000003
[    0.000000] [<ffffffff80c04e0a>] of_parse_and_init_cpus+0x16c/0x16e
[    0.000000] [<ffffffff80c04c96>] setup_smp+0x1e/0x26
[    0.000000] [<ffffffff80c03ffe>] setup_arch+0x6e/0xb2
[    0.000000] [<ffffffff80c00384>] start_kernel+0x72/0x400
[    0.000000] Code: 80e7 4a00 a603 0009 b795 1097 ffe5 80e7 92c0 9002 (9002) 715d 
[    0.000000] ---[ end trace 0000000000000000 ]---
[    0.000000] Kernel panic - not syncing: Fatal exception in interrupt



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

Re: [PATCH v2 10/10] RISC-V: provide a Kconfig option to disable parsing "riscv,isa"

[Palmer Dabbelt]

On Thu, 29 Jun 2023 14:44:18 PDT (-0700), Conor Dooley wrote:
> On Thu, Jun 29, 2023 at 02:16:49PM -0700, Palmer Dabbelt wrote:
>> On Thu, 29 Jun 2023 13:20:55 PDT (-0700), Conor Dooley wrote:
>> > On Thu, Jun 29, 2023 at 03:53:08PM +0200, Andrew Jones wrote:
>> > > On Thu, Jun 29, 2023 at 12:39:51PM +0100, Conor Dooley wrote:
>> > > > On Thu, Jun 29, 2023 at 11:31:33AM +0200, Andrew Jones wrote:
>> > > > > On Thu, Jun 29, 2023 at 09:28:56AM +0100, Conor Dooley wrote:
>> > > > > > As it says on the tin, provide a Kconfig option to disabling parsing the
>> > > > > > "riscv,isa" devicetree property. Hide the option behind NONPORTABLE so
>> > > > > > that only those willing to keep the pieces enable it, and make sure the
>> > > > > > default kernel contains the fallback code.
>> > > > > > > > > Suggested-by: Palmer Dabbelt <palmer@rivosinc.com>
>> > > > > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
>> > > > > > ---
>> > > > > >  arch/riscv/Kconfig             | 16 ++++++++++++++++
>> > > > > >  arch/riscv/kernel/cpu.c        |  3 +++
>> > > > > >  arch/riscv/kernel/cpufeature.c |  2 +-
>> > > > > >  3 files changed, 20 insertions(+), 1 deletion(-)
>> > > > > > > > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>> > > > > > index 1d39efe2b940..0e1909ac5947 100644
>> > > > > > --- a/arch/riscv/Kconfig
>> > > > > > +++ b/arch/riscv/Kconfig
>> > > > > > @@ -291,6 +291,22 @@ config NONPORTABLE
>> > > > > >  > > >  	  If unsure, say N.
>> > > > > >  > > > +config NO_RISCV_ISA_FALLBACK
>> > > > > > +	bool "Permit falling back to parsing riscv,isa for extension support"
>> > > > > > +	depends on NONPORTABLE
>> > > > > > +	help
>> > > > > > +	  Parsing the "riscv,isa" devicetree property has been deprecated and
>> > > > > > +	  replaced by a list of explicitly defined strings. For compatibility
>> > > > > > +	  with existing platforms, the kernel will fall back to parsing the
>> > > > > > +	  "riscv,isa" property if the replacements are not found.
>> > > > > > +
>> > > > > > +	  Selecting Y here will result in a kernel without this fallback, and
>> > > > > > +	  will not work on platforms where the devicetree does not contain the
>> > > > > > +	  replacement 	  properties of "riscv,isa-base" and
>> > > > >                      ^ spacing issue
>> > > > > Huh, weird. Given the tab followed by spaces, it must have snuck
>> > > in
>> > > > during reflow of the text after some rewording.
>> > > > Wonder how I missed it, given that...
>> 
>> I sometimes end up with these when reflowing in vim, the Kconfig help text
>> indent confuses things (though many things confuse whatever vim reflowing
>> I'm using, so I should probably find something better).
>
> I did it by hand, I have no excuses.
>
>> > > > > Should we also have a kernel command line option, 'isa_fallback', where
>> > > > > without this config the command line option is not necessary to fallback,
>> > > > > but, with this config, no fallback will be done unless 'isa_fallback' is
>> > > > > provided?
>> > > > > I don't know, maybe I have the wrong end of the stick but it
>> > > feels a bit
>> > > > premature for something that may never not be hidden behind NONPORTABLE?
>> > > > Perhaps that could be left for a point in time where the default value
>> > > > of the symbol changes, or the dependency on NONPORTABLE is removed?
>> > > >
>> 
>> I agree it might not be worth the work, but looks like that's done...
>
> Well, whether I did it or not doesn't mean it has to be included in the
> series. The work wasn't all that much to be honest.
>
>> > > With the command line option, we could consider dropping NONPORTABLE (but
>> > > still default off this config). What I'm thinking is that if we want to
>> > > encourage the adoption of the new format, then there should be a bit of
>> > > pain when it's not used, but not enough pain to risk rebellion. So,
>> > > 
>> > >  * defconfig builds will silently/painlessly fallback
>> > > 
>> > >  * builds that want to encourage adoption enable this config and will
>> > >    fail to boot when they don't get what they want and don't have the
>> > >    command line option
>> > > 
>> > >  * users still working through the growing pains can manage when
>> > >    the boot fails, and when it doesn't, with the command line
>> > 
>> > So, something like the following squashed in? I inverted the config
>> > option, seemed more natural that way.
>> > 
>> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> > index d910fba25f2c..6c0d0bc06048 100644
>> > --- a/Documentation/admin-guide/kernel-parameters.txt
>> > +++ b/Documentation/admin-guide/kernel-parameters.txt
>> > @@ -5437,6 +5437,13 @@
>> >  			[KNL] Disable ring 3 MONITOR/MWAIT feature on supported
>> >  			CPUs.
>> > +	riscv_isa_fallback [RISCV]
>> > +			Fall back to detecting extension support using the
>> > +			"riscv,isa" property on devicetree systems when the
>> > +			replacement properties are not found, on kernels where
>> > +			RISCV_ISA_FALLBACK is not enabled. See the Kconfig entry
>> > +			for RISCV_ISA_FALLBACK.
>> > +
>> >  	ro		[KNL] Mount root device read-only on boot
>> >  	rodata=		[KNL]
>> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>> > index 0370713ad965..a9a473b67182 100644
>> > --- a/arch/riscv/Kconfig
>> > +++ b/arch/riscv/Kconfig
>> > @@ -291,22 +291,6 @@ config NONPORTABLE
>> >  	  If unsure, say N.
>> > -config NO_RISCV_ISA_FALLBACK
>> > -	bool "Permit falling back to parsing riscv,isa for extension support"
>> > -	depends on NONPORTABLE
>> > -	help
>> > -	  Parsing the "riscv,isa" devicetree property has been deprecated and
>> > -	  replaced by a list of explicitly defined strings. For compatibility
>> > -	  with existing platforms, the kernel will fall back to parsing the
>> > -	  "riscv,isa" property if the replacements are not found.
>> > -
>> > -	  Selecting Y here will result in a kernel without this fallback, and
>> > -	  will not work on platforms where the devicetree does not contain the
>> > -	  replacement properties of "riscv,isa-base" and "riscv,isa-extensions".
>> > -	  Please see the dt-binding, located at
>> > -	  Documentation/devicetree/bindings/riscv/extensions.yaml for details
>> > -	  on the replacement properties.
>> > -
>> >  choice
>> >  	prompt "Base ISA"
>> >  	default ARCH_RV64I
>> > @@ -857,6 +841,24 @@ config XIP_PHYS_ADDR
>> >  	  be linked for and stored to.  This address is dependent on your
>> >  	  own flash usage.
>> > +config RISCV_ISA_FALLBACK
>> > +	bool "Permit falling back to parsing riscv,isa for extension support by default"
>> > +	default y
>> 
>> I think the only risk here is that randconfig will result in systems that
>> don't boot, but we're already stuck with no meaningful base requirements.
>> Fixing that is a way bigger hurdle than this, so IMO it's fine.
>
> Maybe I'm way off here, but I didn't think that we had to be worried
> about randconfigs not booting, as it was a testing tool?

IIRC at least some ports had randconfigs that booted, there was just 
some standard set of drivers always built in.  We're a long way from 
that, though, so whatever.

>> > +	help
>> > +	  Parsing the "riscv,isa" devicetree property has been deprecated and
>> > +	  replaced by a list of explicitly defined strings. For compatibility
>> > +	  with existing platforms, the kernel will fall back to parsing the
>> > +	  "riscv,isa" property if the replacements are not found.
>> > +
>> > +	  Selecting N here will result in a kernel that does not use the
>> > +	  fallback, unless the commandline "riscv_isa_fallback" parameter is
>> > +	  present.
>> > +
>> > +	  Please see the dt-binding, located at
>> > +	  Documentation/devicetree/bindings/riscv/extensions.yaml for details
>> > +	  on the replacement properties of "riscv,isa-base" and
>> > +	  "riscv,isa-extensions".
>> > +
>> >  endmenu # "Boot options"
>> >  config BUILTIN_DTB
>> > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
>> > index e3cda14a486b..52fa607a1691 100644
>> > --- a/arch/riscv/include/asm/hwcap.h
>> > +++ b/arch/riscv/include/asm/hwcap.h
>> > @@ -81,6 +81,7 @@ struct riscv_isa_ext_data {
>> >  extern const struct riscv_isa_ext_data riscv_isa_ext[];
>> >  extern const size_t riscv_isa_ext_count;
>> > +extern bool riscv_isa_fallback_cmdline;
>> > 
>> >  unsigned long riscv_isa_extension_base(const unsigned long *isa_bitmap);
>> > diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
>> > index 86a1d98b8b3b..1e4cbdedc7fc 100644
>> > --- a/arch/riscv/kernel/cpu.c
>> > +++ b/arch/riscv/kernel/cpu.c
>> > @@ -81,7 +81,7 @@ int riscv_early_of_processor_hartid(struct device_node *node, unsigned long *har
>> >  	return 0;
>> >  old_interface:
>> > -	if (IS_ENABLED(CONFIG_NO_RISCV_ISA_FALLBACK))
>> > +	if (!IS_ENABLED(CONFIG_RISCV_ISA_FALLBACK) && !riscv_isa_fallback_cmdline)
>> >  		return -ENODEV;
>> >  	if (of_property_read_string(node, "riscv,isa", &isa)) {
>> > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
>> > index f6fb18d2af84..5d848b7c1dde 100644
>> > --- a/arch/riscv/kernel/cpufeature.c
>> > +++ b/arch/riscv/kernel/cpufeature.c
>> > @@ -471,6 +471,14 @@ static int __init riscv_fill_hwcap_from_ext_list(unsigned long *isa2hwcap)
>> >  	return 0;
>> >  }
>> > +bool __initdata riscv_isa_fallback_cmdline = false;
>> > +static int __init riscv_isa_fallback_setup(char *__unused)
>> 
>> Maybe it's better to support =true and =false here?  Not sure it matters,
>> we're already down a rabbit hole ;)
>
> Dunno, not implemented a cmdline param before. Seemed "cleaner" to check
> for presence, don't really care so I'll adapt to w/e.
>
>> > +{
>> > +	riscv_isa_fallback_cmdline = true;
>> > +	return 1;
>> > +}
>> > +early_param("riscv_isa_fallback", riscv_isa_fallback_setup);
>> 
>> IMO it's cleaner to just set the default based on the Kconfig, that way
>> there's only one thing to check.  We're already duplicating the "check both
>> Kconfig and cmdline" logic twice, there's probably going to be others
>> eventually.
>
> Sure.
>
>> > +
>> >  void __init riscv_fill_hwcap(void)
>> >  {
>> >  	char print_str[NUM_ALPHA_EXTS + 1];
>> > @@ -490,7 +498,7 @@ void __init riscv_fill_hwcap(void)
>> >  	} else {
>> >  		int ret = riscv_fill_hwcap_from_ext_list(isa2hwcap);
>> > -		if (ret && !IS_ENABLED(CONFIG_NO_RISCV_ISA_FALLBACK)) {
>> > +		if (ret && (IS_ENABLED(CONFIG_RISCV_ISA_FALLBACK) || riscv_isa_fallback_cmdline)) {
>> >  			pr_info("Falling back to deprecated \"riscv,isa\"\n");
>> >  			riscv_fill_hwcap_from_isa_string(isa2hwcap);
>> >  		}
>> 
>> I haven't read the whole patch set, but at least having a nice error for why
>> probing didn't happen seems like the right way to go here.  IIUC this is
>> just silent when ISA string fallbacks are disabled and the new properties
>> aren't there.
>> 
>
>> Sorry in advance if it's just somewhere outside the diff, though.
>
> It sorta is. You'll actually not even reach this code if probing fails.
> Because riscv_early_of_processor_hartid(), which is called super early
> on in of_parse_and_init_cpus(), checks whether there are any valid
> harts (including checking for sufficient extension support to run a
> kernel) we will hit a BUG() during setup_smp() & blow up really early:
>
> [    0.000000] Linux version 6.4.0-rc1-00096-ge0097d2c62d5-dirty (conor@spud) (ClangBuiltLinux clang version 15.0.7 (/stuff/brsdk/llvm/clang 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a), ClangBuiltLinux LLD 15.0.7) #1 SMP PREEMPT @666
> [    0.000000] Machine model: Microchip PolarFire-SoC Icicle Kit
> [    0.000000] SBI specification v0.3 detected
> [    0.000000] SBI implementation ID=0x1 Version=0x10000
> [    0.000000] SBI TIME extension detected
> [    0.000000] SBI IPI extension detected
> [    0.000000] SBI RFENCE extension detected
> [    0.000000] SBI SRST extension detected
> [    0.000000] earlycon: ns16550a0 at MMIO32 0x0000000020100000 (options '115200n8')
> [    0.000000] printk: bootconsole [ns16550a0] enabled
> [    0.000000] printk: debug: skip boot console de-registration.
> [    0.000000] efi: UEFI not found.
> [    0.000000] OF: reserved mem: 0x00000000bfc00000..0x00000000bfffffff (4096 KiB) nomap non-reusable region@BFC00000
> [    0.000000] Zone ranges:
> [    0.000000]   DMA32    [mem 0x0000000080000000-0x00000000ffffffff]
> [    0.000000]   Normal   [mem 0x0000000100000000-0x000000107fffffff]
> [    0.000000] Movable zone start for each node
> [    0.000000] Early memory node ranges
> [    0.000000]   node   0: [mem 0x0000000080000000-0x00000000bfbfffff]
> [    0.000000]   node   0: [mem 0x00000000bfc00000-0x00000000bfffffff]
> [    0.000000]   node   0: [mem 0x0000001040000000-0x000000107fffffff]
> [    0.000000] Initmem setup node 0 [mem 0x0000000080000000-0x000000107fffffff]
> [    0.000000] SBI HSM extension detected
> [    0.000000] CPU with hartid=0 is not available
> [    0.000000] ------------[ cut here ]------------
> [    0.000000] kernel BUG at arch/riscv/kernel/smpboot.c:174!
> [    0.000000] Kernel BUG [#1]
> [    0.000000] Modules linked in:
> [    0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 6.4.0-rc1-00096-ge0097d2c62d5-dirty #1
> [    0.000000] Hardware name: Microchip PolarFire-SoC Icicle Kit (DT)
> [    0.000000] epc : of_parse_and_init_cpus+0x16c/0x16e
> [    0.000000]  ra : of_parse_and_init_cpus+0x9a/0x16e
> [    0.000000] epc : ffffffff80c04e0a ra : ffffffff80c04d38 sp : ffffffff81603e20
> [    0.000000]  gp : ffffffff8182d658 tp : ffffffff81613f80 t0 : 000000000000006e
> [    0.000000]  t1 : 0000000000000064 t2 : 0000000000000000 s0 : ffffffff81603e80
> [    0.000000]  s1 : 0000000000000000 a0 : 0000000000000000 a1 : 0000000000000000
> [    0.000000]  a2 : 0000000000000000 a3 : 0000000000000000 a4 : 0000000000000000
> [    0.000000]  a5 : 0000000000001fff a6 : 0000000000001fff a7 : ffffffff816148b0
> [    0.000000]  s2 : 0000000000000001 s3 : ffffffff81492a4c s4 : ffffffff81a4b090
> [    0.000000]  s5 : ffffffff81506030 s6 : 0000000000000040 s7 : 0000000000000000
> [    0.000000]  s8 : 00000000bfb6f046 s9 : 0000000000000001 s10: 0000000000000000
> [    0.000000]  s11: 00000000bf389700 t3 : 0000000000000000 t4 : 0000000000000000
> [    0.000000]  t5 : ffffffff824dd188 t6 : ffffffff824dd187
> [    0.000000] status: 0000000200000100 badaddr: 0000000000000000 cause: 0000000000000003
> [    0.000000] [<ffffffff80c04e0a>] of_parse_and_init_cpus+0x16c/0x16e
> [    0.000000] [<ffffffff80c04c96>] setup_smp+0x1e/0x26
> [    0.000000] [<ffffffff80c03ffe>] setup_arch+0x6e/0xb2
> [    0.000000] [<ffffffff80c00384>] start_kernel+0x72/0x400
> [    0.000000] Code: 80e7 4a00 a603 0009 b795 1097 ffe5 80e7 92c0 9002 (9002) 715d 
> [    0.000000] ---[ end trace 0000000000000000 ]---
> [    0.000000] Kernel panic - not syncing: Fatal exception in interrupt

Thanks, I guess we can add a warning independently of these changes.

https://lore.kernel.org/r/20230629223502.1924-1-palmer@rivosinc.com/

Re: [PATCH v2 10/10] RISC-V: provide a Kconfig option to disable parsing "riscv,isa"

[Andrew Jones]

On Thu, Jun 29, 2023 at 10:44:18PM +0100, Conor Dooley wrote:
> On Thu, Jun 29, 2023 at 02:16:49PM -0700, Palmer Dabbelt wrote:
> > On Thu, 29 Jun 2023 13:20:55 PDT (-0700), Conor Dooley wrote:
...
> > > +bool __initdata riscv_isa_fallback_cmdline = false;
> > > +static int __init riscv_isa_fallback_setup(char *__unused)
> > 
> > Maybe it's better to support =true and =false here?  Not sure it matters,
> > we're already down a rabbit hole ;)
> 
> Dunno, not implemented a cmdline param before. Seemed "cleaner" to check
> for presence, don't really care so I'll adapt to w/e.
>

I don't have a strong preference here, but to throw in more food for
thought, I see this DT-v1 vs. DT-v2 choice to be a bit analogous to the
DT vs. ACPI choice. The 'acpi' command line parameter, for RISC-V, can
be 'off', 'on', and 'force', where

  off -- disable ACPI if default was on
  on -- enable ACPI but allow fallback to DT
  force -- enable ACPI if default was off

So, if the default of the isa fallback command line option will depend on
Kconfig, then we may also want a 'force'.

Thanks,
drew

Re: [PATCH v2 10/10] RISC-V: provide a Kconfig option to disable parsing "riscv,isa"

[Conor Dooley]

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

On Fri, Jun 30, 2023 at 09:46:48AM +0200, Andrew Jones wrote:
> On Thu, Jun 29, 2023 at 10:44:18PM +0100, Conor Dooley wrote:
> > On Thu, Jun 29, 2023 at 02:16:49PM -0700, Palmer Dabbelt wrote:
> > > On Thu, 29 Jun 2023 13:20:55 PDT (-0700), Conor Dooley wrote:
> ...
> > > > +bool __initdata riscv_isa_fallback_cmdline = false;
> > > > +static int __init riscv_isa_fallback_setup(char *__unused)
> > > 
> > > Maybe it's better to support =true and =false here?  Not sure it matters,
> > > we're already down a rabbit hole ;)
> > 
> > Dunno, not implemented a cmdline param before. Seemed "cleaner" to check
> > for presence, don't really care so I'll adapt to w/e.
> >
> 
> I don't have a strong preference here, but to throw in more food for
> thought, I see this DT-v1 vs. DT-v2 choice to be a bit analogous to the
> DT vs. ACPI choice. The 'acpi' command line parameter, for RISC-V, can
> be 'off', 'on', and 'force', where
> 
>   off -- disable ACPI if default was on
>   on -- enable ACPI but allow fallback to DT
>   force -- enable ACPI if default was off
> 
> So, if the default of the isa fallback command line option will depend on
> Kconfig, then we may also want a 'force'.

I'm not sure that I understand what "force" would give us.
There's 4 cases:
- CONFIG_RISCV_ISA_FALLBACK is enabled, cmdline option is present:
  cmdline option is ignored, fallback is taken if needed.
  crash if neither are present.

- CONFIG_RISCV_ISA_FALLBACK is enabled, cmdline option is not present:
  cmdline option is ignored, fallback is taken if needed.
  crash if neither are present.

- CONFIG_RISCV_ISA_FALLBACK is disabled, cmdline option is present:
  cmdline option takes priority, fallback is taken if needed.
  crash if neither are present.

- CONFIG_RISCV_ISA_FALLBACK is disabled, cmdline option is not present:
  fallback is never taken
  crash if new properties aren't present.

I don't really see the value in having an equivalent to acpi=off,
because the order of precedence is, to use your naming, "DT-v2" falling
back to "DT-v1" & the default value concerns the use of the fallback.
For ACPI, it is the other way around & the option controls the use of
"DT-v2"'s analogue. Trying to slot in that logic:

- CONFIG_RISCV_ISA_FALLBACK is enabled, cmdline option "=on":
  cmdline option is ignored, fallback is taken if needed.
  crash if neither are present.

- CONFIG_RISCV_ISA_FALLBACK is enabled, cmdline option "=off":
  cmdline option is prioritised, fallback is taken if needed.
  crash if new properties aren't present.

- CONFIG_RISCV_ISA_FALLBACK is disabled, cmdline option "=on":
  cmdline option is prioritised, fallback is taken if needed.
  crash if neither are present.

- CONFIG_RISCV_ISA_FALLBACK is disabled, cmdline option "=off":
  fallback is never taken
  crash if new properties aren't present.

I think I prefer the behaviour of what I currently have & I don't really
get where the "force" option is supposed to fit in either?

Cheers,
Conor.

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

Re: [PATCH v2 10/10] RISC-V: provide a Kconfig option to disable parsing "riscv,isa"

[Andrew Jones]

On Fri, Jun 30, 2023 at 02:19:46PM +0100, Conor Dooley wrote:
> On Fri, Jun 30, 2023 at 09:46:48AM +0200, Andrew Jones wrote:
> > On Thu, Jun 29, 2023 at 10:44:18PM +0100, Conor Dooley wrote:
> > > On Thu, Jun 29, 2023 at 02:16:49PM -0700, Palmer Dabbelt wrote:
> > > > On Thu, 29 Jun 2023 13:20:55 PDT (-0700), Conor Dooley wrote:
> > ...
> > > > > +bool __initdata riscv_isa_fallback_cmdline = false;
> > > > > +static int __init riscv_isa_fallback_setup(char *__unused)
> > > > 
> > > > Maybe it's better to support =true and =false here?  Not sure it matters,
> > > > we're already down a rabbit hole ;)
> > > 
> > > Dunno, not implemented a cmdline param before. Seemed "cleaner" to check
> > > for presence, don't really care so I'll adapt to w/e.
> > >
> > 
> > I don't have a strong preference here, but to throw in more food for
> > thought, I see this DT-v1 vs. DT-v2 choice to be a bit analogous to the
> > DT vs. ACPI choice. The 'acpi' command line parameter, for RISC-V, can
> > be 'off', 'on', and 'force', where
> > 
> >   off -- disable ACPI if default was on
> >   on -- enable ACPI but allow fallback to DT
> >   force -- enable ACPI if default was off
> > 
> > So, if the default of the isa fallback command line option will depend on
> > Kconfig, then we may also want a 'force'.
> 
> I'm not sure that I understand what "force" would give us.
> There's 4 cases:
> - CONFIG_RISCV_ISA_FALLBACK is enabled, cmdline option is present:
>   cmdline option is ignored, fallback is taken if needed.
>   crash if neither are present.
> 
> - CONFIG_RISCV_ISA_FALLBACK is enabled, cmdline option is not present:
>   cmdline option is ignored, fallback is taken if needed.
>   crash if neither are present.
> 
> - CONFIG_RISCV_ISA_FALLBACK is disabled, cmdline option is present:
>   cmdline option takes priority, fallback is taken if needed.
>   crash if neither are present.
> 
> - CONFIG_RISCV_ISA_FALLBACK is disabled, cmdline option is not present:
>   fallback is never taken
>   crash if new properties aren't present.
> 
> I don't really see the value in having an equivalent to acpi=off,
> because the order of precedence is, to use your naming, "DT-v2" falling
> back to "DT-v1" & the default value concerns the use of the fallback.
> For ACPI, it is the other way around & the option controls the use of
> "DT-v2"'s analogue. Trying to slot in that logic:
> 
> - CONFIG_RISCV_ISA_FALLBACK is enabled, cmdline option "=on":
>   cmdline option is ignored, fallback is taken if needed.
>   crash if neither are present.
> 
> - CONFIG_RISCV_ISA_FALLBACK is enabled, cmdline option "=off":
>   cmdline option is prioritised, fallback is taken if needed.
>   crash if new properties aren't present.
> 
> - CONFIG_RISCV_ISA_FALLBACK is disabled, cmdline option "=on":
>   cmdline option is prioritised, fallback is taken if needed.
>   crash if neither are present.
> 
> - CONFIG_RISCV_ISA_FALLBACK is disabled, cmdline option "=off":
>   fallback is never taken
>   crash if new properties aren't present.
> 
> I think I prefer the behaviour of what I currently have & I don't really
> get where the "force" option is supposed to fit in either?
>

WFM

Thanks,
drew