Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 3 Jul 2023 12:44:48 +0200
From:   Andrew Jones <ajones@ventanamicro.com>
To:     Conor Dooley <conor.dooley@microchip.com>
Cc:     palmer@dabbelt.com, conor@kernel.org,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Albert Ou <aou@eecs.berkeley.edu>,
        Jonathan Corbet <corbet@lwn.net>,
        Heiko Stuebner <heiko.stuebner@vrull.eu>,
        Evan Green <evan@rivosinc.com>,
        Sunil V L <sunilvl@ventanamicro.com>,
        linux-doc@vger.kernel.org, linux-riscv@lists.infradead.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        Palmer Dabbelt <palmer@rivosinc.com>
Subject: Re: [PATCH v3 11/11] RISC-V: provide Kconfig & commandline options
 to control parsing "riscv,isa"
Message-ID: <20230703-57fe19b069fdacd4d9a82cf1@orel>
References: <20230703-repayment-vocalist-e4f3eeac2b2a@wendy>
 <20230703-greedy-dividable-251fa2b809ac@wendy>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230703-greedy-dividable-251fa2b809ac@wendy>
Precedence: bulk

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