Re: [PATCH v3 11/11] RISC-V: provide Kconfig & commandline options to control parsing "riscv,isa"

Andrew Jones
2023-07-03T10:44:57Z

On Mon, Jul 03, 2023 at 11:28:03AM +0100, Conor Dooley wrote:
> As it says on the tin, provide Kconfig option to control parsing the
> "riscv,isa" devicetree property. If either option is used, the kernel
> will fall back to parsing "riscv,isa", where "riscv,isa-base" and
> "riscv,isa-extensions" are not present.
> The Kconfig options are set up so that the default kernel configuration
> will enable the fallback path, without needing the commandline option.
>
> Suggested-by: Andrew Jones <ajones@ventanamicro.com>
> Suggested-by: Palmer Dabbelt <palmer@rivosinc.com>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
> Changes in v3:
> - Invert the Kconfig entry. It's now default y & not hidden by
> NONPORTABLE, but its entablement will now activate the fallback
> - Add a commandline option to enable the fallback on kernels that do not
> enable it in Kconfig, as Drew suggested
> - Default the global var to the Kconfig option & override it with the
> commandline one, rather than have checks for IS_ENABLED() and for the
> commandline option in riscv_fill_hwcap() &
> riscv_early_of_processor_hartid()
> ---
> .../admin-guide/kernel-parameters.txt | 7 +++++++
> arch/riscv/Kconfig | 18 ++++++++++++++++++
> arch/riscv/include/asm/hwcap.h | 1 +
> arch/riscv/kernel/cpu.c | 6 ++++++
> arch/riscv/kernel/cpufeature.c | 14 +++++++++++++-
> 5 files changed, 45 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index d910fba25f2c..1bd435f60055 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]
> + When CONFIG_RISCV_ISA_FALLBACK is not enabled, permit
> + falling back to detecting extension support by parsing
> + "riscv,isa" property on devicetree systems when the
> + replacement properties are not found. 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 1d39efe2b940..a9a473b67182 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -841,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

nit: It's probably just me, but 'of' twists my brain a bit here. I
think

...the replacement properties, "riscv,isa-base" and "riscv,isa-extensions".

works better (for me).

> + "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..b7b58258f6c7 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;
>
> 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 28d5af21f544..1acf3679600d 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -87,6 +87,12 @@ int riscv_early_of_processor_hartid(struct device_node *node, unsigned long *har
> return 0;
>
> old_interface:
> + if (!riscv_isa_fallback) {
> + pr_warn("CPU with hartid=%lu is invalid: this kernel does not parse \"riscv,isa\"",
> + *hart);
> + 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..5945dfc5f806 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -471,6 +471,18 @@ static int __init riscv_fill_hwcap_from_ext_list(unsigned long *isa2hwcap)
> return 0;
> }
>
> +#ifdef CONFIG_RISCV_ISA_FALLBACK
> +bool __initdata riscv_isa_fallback = true;
> +#else
> +bool __initdata riscv_isa_fallback;
> +static int __init riscv_isa_fallback_setup(char *__unused)
> +{
> + riscv_isa_fallback = true;
> + return 1;
> +}
> +early_param("riscv_isa_fallback", riscv_isa_fallback_setup);
> +#endif
> +
> void __init riscv_fill_hwcap(void)
> {
> char print_str[NUM_ALPHA_EXTS + 1];
> @@ -490,7 +502,7 @@ void __init riscv_fill_hwcap(void)
> } else {
> int ret = riscv_fill_hwcap_from_ext_list(isa2hwcap);
>
> - if (ret) {
> + if (ret && riscv_isa_fallback) {
> pr_info("Falling back to deprecated \"riscv,isa\"\n");
> riscv_fill_hwcap_from_isa_string(isa2hwcap);
> }
> --
> 2.40.1
>

Otherwise,

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

Thanks,
drew