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@xxxxxxxxxxxx>
> > > Signed-off-by: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
> > > ---
> > >  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