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

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

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
  

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.
  

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
  

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);
 		}
  

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.
  

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
  

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/
  

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
  

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.
  

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