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

Wed Jul 12 10:17:49 PDT 2023

On Wed, 12 Jul 2023 04:38:51 PDT (-0700), Conor Dooley wrote:
> Hey Palmer,
>
> On Tue, Jul 11, 2023 at 03:33:17PM -0700, Palmer Dabbelt wroteh:
>> 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 at rivosinc.com>
>
> Thinking a little harder than I did last night, I think the diff should
> actually look something like the below, if this is the approach that is
> going to be taken:
>
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index bdcf460ea53d..4b759cd2e3be 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -334,6 +334,26 @@ void __init riscv_fill_hwcap(void)
>  			set_bit(RISCV_ISA_EXT_ZIHPM, isainfo->isa);
>  		}
>
> +		/*
> +		 * "V" in ISA strings is a ambiguous in practice: it should
> +		 * mean just the standard V-1.0 but vendors aren't well
> +		 * behaved.  Only allow V in ISA strings that come from ACPI,
> +		 * as we've yet to build up enough history in ACPI land to stop
> +		 * trusting ISA strings.
> +		 *
> +		 * DT-based systems must provide the explicit V property, which
> +		 * is well defined.
> +		 */
> +		if (acpi_disabled) {
> +			if (of_property_match_string(node, "riscv,isa-extensions", "v") >= 0) {
> +				this_hwcap |= isa2hwcap[RISCV_ISA_EXT_v];
> +				set_bit(RISCV_ISA_EXT_v, isainfo->isa);
> +			} else {
> +				this_hwcap &= ~isa2hwcap[RISCV_ISA_EXT_v];
> +				clear_bit(RISCV_ISA_EXT_v, isainfo->isa);
> +			}
> +		}
> +
>  		/*
>  		 * All "okay" hart should have same isa. Set HWCAP based on
>  		 * common capabilities of every "okay" hart, in case they don't
>
>> This came up as part of some discussions about the T-Head vector
>> support.
>
> I guess what I brought up was the various T-Head devicetrees in the wild
> that contain things like "rv64imafdcv". The Sophgo sg204-whatever has
> that, and it has one of the "cleaner" downstream trees containing
> support for the T-Head vector stuff. Usually I would not care all that
> much about downstream trees, but it does mean that downstream U-Boots
> and OpenSBIs will likely use devicetrees that contain similar stuff & we
> do have to deal with being passed those.
>
>> 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?
>
> It looks super clunky to be honest, but the alternatives are not all
> that pretty either. Either we could disable detection from DT entirely
> until v6.6 when you hopefully take my series handling the new
> properties, but that puts the vector stuff into an odd spot, where no
> system could actually use it. Or, we could send my series as fixes for
> v6.5, but I think that is very much on the edge of what could be
> considered a fix...
>
>> 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.
>
> Right. Doing anything with the T-Head arch/impl IDs is really fragile,
> since we don't know if they'll continue to be zero in the future.
> I'd argue that it is less "dangerous" to disable mapping "v" -> vector
> when using riscv,isa on any T-Head systems w/ a zero archid/impid, than
> it is to turn on their v0.7.1 flavour under the same conditions, but
> that just amounts to a more niche version of what we have here.
> I still don't think that it is a good idea to do it in the errata
> handling code (at least how we have it structured now) since it could
> affect a cpu, like the c908, that does actually support the standard
> version of vector, if it still has impid/archid tied to zero.
>
> Disabling mapping "v" -> vector is a bit of a big hammer, since it hits
> behaving implementations too. I'd be inclined to limit the impact to
> just the known misbehaver & make the condition
>     if (acpi_disabled && riscv_cached_mvendorid(cpu) == THEAD_VENDOR_ID)
> or something along those lines.
> That way, it's only being disabled for T-Head systems that attempt to
> use "riscv,isa" to communicate support for vector, and T-Head systems
> that may pop up in the future that support the standard extension can
> use the new properties?

We were talking about this some in the patchwork meeting, I think the 
general conclusion is that there's really no good options.  It does seem 
reasonable to avoid penalizing vendors who follow the V spec, though, so 
unless anyone else has an opinion let's go with the "only ban V in ISA 
strings on T-Head machines" flavor.  Looks like T-Head will be shipping 
non-standard V for a while, so by that point the DT properties should be 
well established and we can just have them use those.

Do you want to re-spin the patch?  I can dig it out of the email if you 
want, but looks like you've got the diff already...

No rush on my end, just LMK -- it'll be too late for this week either 
way.

>
> Hope that makes sense,
> Conor.
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv