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

Commit Message

Palmer Dabbelt July 11, 2023, 10:33 p.m. UTC

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 <[e-mail]>
---
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(-)

Comments

Conor Dooley July 11, 2023, 11:04 p.m. UTC | #1

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 <[e-mail]>
> ---
> 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
>

Conor Dooley July 12, 2023, 11:38 a.m. UTC | #2

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 <[e-mail]>

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.

Palmer Dabbelt July 12, 2023, 5:17 p.m. UTC | #3

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 <[e-mail]>
>
> 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
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Conor Dooley July 12, 2023, 5:22 p.m. UTC | #4

On Wed, Jul 12, 2023 at 10:17:49AM -0700, Palmer Dabbelt wrote:

> 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.

I wrote it at work, I'll be digging it out of my email too ;)
I'll send it now before I get lost in my DT review queue.

Palmer Dabbelt July 12, 2023, 5:24 p.m. UTC | #5

On Wed, 12 Jul 2023 10:22:09 PDT (-0700), Conor Dooley wrote:
> On Wed, Jul 12, 2023 at 10:17:49AM -0700, Palmer Dabbelt wrote:
>
>> 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.
>
> I wrote it at work, I'll be digging it out of my email too ;)
> I'll send it now before I get lost in my DT review queue.

Thanks, I'll forget about it for now ;)

Patch

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;
 
 	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;
+
 		/*
 		 * All "okay" hart should have same isa. Set HWCAP based on
 		 * common capabilities of every "okay" hart, in case they don't