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

[Conor Dooley]

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?

Hope that makes sense,
Conor.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-riscv/attachments/20230712/9333e18c/attachment.sig>