Message-ID: <mhng-d1fcf69b-9925-42ec-aa8f-eb3a25eaafe1@palmer-ri-x1c9a>
Date:   Thu, 29 Jun 2023 15:47:35 -0700 (PDT)
From:   Palmer Dabbelt <palmer@...osinc.com>
To:     Conor Dooley <conor@...nel.org>
CC:     ajones@...tanamicro.com, Conor Dooley <conor.dooley@...rochip.com>,
        robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
        Paul Walmsley <paul.walmsley@...ive.com>,
        aou@...s.berkeley.edu, heiko.stuebner@...ll.eu,
        Evan Green <evan@...osinc.com>, sunilvl@...tanamicro.com,
        linux-riscv@...ts.infradead.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject:     Re: [PATCH v2 10/10] RISC-V: provide a Kconfig option to disable parsing "riscv,isa"

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@...osinc.com>
>> > > > > > Signed-off-by: Conor Dooley <conor.dooley@...rochip.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@...d) (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@...00000
> [    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/

Powered by blists - more mailing lists