Re: [PATCH] RISC-V: Don't trust V from the riscv,isa DT property

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 4405 bytes --]

Hey Palmer,

On Tue, Jul 11, 2023 at 03:33:17PM -0700, Palmer Dabbelt wrote:
> The last merge window contained both V support and the deprecation of
> the riscv,isa DT property, with the V implementation reading riscv,isa
> to determine the presence of the V extension.  At the time that was the
> only way to do it, but there's a lot of ambiguity around V in ISA
> strings.
> 
> Rather than trying to sort that out, let's just not introduce the
> ambiguity in the first place and retroactively make the deprecation
> apply to V.  This all happened in the same merge window anyway, so this
> way we don't end up introducing a new ambiguous interface we need to
> maintain compatibility with forever (despite it having been deprecated
> in the same release).
> 
> ACPI still trusts ISA strings, so we'll leave that alone.
> 
> Fixes: dc6667a4e7e3 ("riscv: Extending cpufeature.c to detect V-extension")
> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
> ---
> This came up as part of some discussions about the T-Head vector
> support.  I haven't actually tested this, but Conor and I were talking
> about options and it was easier to just implement it than describe it.
> It's kind of clunky to only parse that one property, but we're already
> in a grey area WRT having the DT bindings that we don't parse so maybe
> that's not so bad?
> 
> The other option would be to turn off V when we detect we're on a T-Head
> system as part of the errata handling.  That's similar to what we do for
> misaligned accesses, but that's a hack that we're getting rid of.  It'd
> be less of a hack for V, but given that we've found T-Head systems
> aliasing the arch/impl IDs already we might be digging ourselves a
> bigger hole here.
> ---
>  arch/riscv/kernel/cpufeature.c | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index bdcf460ea53d..8e970f55285e 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -116,7 +116,19 @@ void __init riscv_fill_hwcap(void)
>  	isa2hwcap['f' - 'a'] = COMPAT_HWCAP_ISA_F;
>  	isa2hwcap['d' - 'a'] = COMPAT_HWCAP_ISA_D;
>  	isa2hwcap['c' - 'a'] = COMPAT_HWCAP_ISA_C;
> -	isa2hwcap['v' - 'a'] = COMPAT_HWCAP_ISA_V;
> +
> +	/*
> +	 * "V" in ISA strings is a ambiguous in practice: it should
> +	 * mean just the standard V-1.0 but vendors aren't well
> +	 * behaved.  So only allow V in ISA strings that come from ACPI, as
> +	 * we've yet to build up enough histroy in ACPI land to stop trusting
> +	 * ISA strings.
> +	 *
> +	 * DT-based systems must provide the explicit V property, which is well
> +	 * defined.  That is parsed below.
> +	 */
> +	if (!acpi_disabled)
> +		isa2hwcap['v' - 'a'] = COMPAT_HWCAP_ISA_V;

I'm insufficiently awake at this point to review this properly & will
reply again tomorrow, but I don't think this is sufficient - won't you
still end up setting the V bit in isainfo::isa?
	if (!ext_long) {
		int nr = tolower(*ext) - 'a';

		if (riscv_isa_extension_check(nr)) {
			this_hwcap |= isa2hwcap[nr];
			set_bit(nr, isainfo->isa);
		}
	}
cpufeature.c @ L294

>  
>  	elf_hwcap = 0;
>  
> @@ -131,6 +143,8 @@ void __init riscv_fill_hwcap(void)
>  	for_each_possible_cpu(cpu) {
>  		struct riscv_isainfo *isainfo = &hart_isa[cpu];
>  		unsigned long this_hwcap = 0;
> +		struct property *p;
> +		const char *ext;
>  
>  		if (acpi_disabled) {
>  			node = of_cpu_device_node_get(cpu);
> @@ -334,6 +348,15 @@ void __init riscv_fill_hwcap(void)
>  			set_bit(RISCV_ISA_EXT_ZIHPM, isainfo->isa);
>  		}
>  
> +		/*
> +		 * Check just the V property on DT-based systems, as we don't
> +		 * trust the ISA string in DT land.
> +		 */
> +		if (acpi_disabled)
> +			of_property_for_each_string(node, "riscv,isa-extensions", p, ext)
> +				if (strcmp(ext, "v") == 0)
> +					this_hwcap |= COMPAT_HWCAP_ISA_V;

I think this should be:
	if (acpi_disabled)
		if (of_property_match_string(node, "riscv,isa-extensions", "v") >= 0)
			this_hwcap |= COMPAT_HWCAP_ISA_V;

(or compressed into a single if, depending on w/e you like..)

> +
>  		/*
>  		 * All "okay" hart should have same isa. Set HWCAP based on
>  		 * common capabilities of every "okay" hart, in case they don't
> -- 
> 2.40.1
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv