[PATCH v2] RISC-V: Don't trust V from the riscv,isa DT property on T-Head CPUs

[Palmer Dabbelt]

On Thu, 13 Jul 2023 09:36:49 PDT (-0700), jszhang at kernel.org wrote:
> On Wed, Jul 12, 2023 at 06:48:02PM +0100, Conor Dooley wrote:
>> From: Palmer Dabbelt <palmer at rivosinc.com>
>>
>> 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.  In particular, there is a lot of firmware in the wild that
>> uses "v" in the riscv,isa DT property to communicate support for the
>> 0.7.1 version of the Vector specification implemented by T-Head CPU
>> cores.
>
> Add Guo
>
> Hi Conor, Palmer,
>
> FWICT, new T-HEAD's riscv cores such as C908 support standard RVV-1.0,
> this patch looks like a big hammer for T-HEAD. I do understand why

Ya, it's a big hammer.  There's no extant systems with the C908, though, 
and given that the C906 and C910 alias marchid/mimplid it's kind of hard 
to trust any of those values for T-Head systems.  We could check for the 
0s and hope T-Head starts setting something else, but I'm not sure 
that's a net win (we've also got the C920 in the Sophgo chip, which IIUC 
is V-0.7.1 too).

> this patch is provided, but can we mitigate the situation by carefully
> review the DTs? Per my understanding, dts is also part of linux kernel.

That would break compatibility with existing firmware.  It's certainly 
something that has happened before, but we try to avoid it where 
possible.

In this case new T-Head systems that have standard V would just need to 
provide the new DT properties instead of the ISA string property.  We've 
deprecated the ISA string property already so that's what new systems 
should be doing anyway, and given that this only applies to new systems 
it doesn't seem like a big ask.

> Thanks
>
>>
>> Rather than forcing use of the newly added interface that has strict
>> meanings for extensions to detect the presence of vector support, as
>> that would penalise those who have behaved, only ignore v in riscv,isa
>> on CPUs that report T-Head's vendor ID.
>>
>> Fixes: dc6667a4e7e3 ("riscv: Extending cpufeature.c to detect V-extension")
>> Signed-off-by: Palmer Dabbelt <palmer at rivosinc.com>
>> Co-developed-by: Conor Dooley <conor.dooley at microchip.com>
>> Signed-off-by: Conor Dooley <conor.dooley at microchip.com>
>> ---
>> Changes in v2:
>> - Use my version of the patch that touches hwcap and isainfo uniformly
>> - Don't penalise those who behaved
>> ---
>>  arch/riscv/kernel/cpufeature.c | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
>> index bdcf460ea53d..05362715e1b7 100644
>> --- a/arch/riscv/kernel/cpufeature.c
>> +++ b/arch/riscv/kernel/cpufeature.c
>> @@ -21,6 +21,7 @@
>>  #include <asm/hwcap.h>
>>  #include <asm/patch.h>
>>  #include <asm/processor.h>
>> +#include <asm/sbi.h>
>>  #include <asm/vector.h>
>>
>>  #define NUM_ALPHA_EXTS ('z' - 'a' + 1)
>> @@ -334,6 +335,27 @@ void __init riscv_fill_hwcap(void)
>>  			set_bit(RISCV_ISA_EXT_ZIHPM, isainfo->isa);
>>  		}
>>
>> +		/*
>> +		 * "V" in ISA strings is ambiguous in practice: it should mean
>> +		 * just the standard V-1.0 but vendors aren't well behaved.
>> +		 * Many vendors with T-Head CPU cores which implement the 0.7.1
>> +		 * version of the vector specification put "v" into their DTs
>> +		 * and no T-Head CPU cores with the standard version of vector
>> +		 * are in circulation yet.
>> +		 * Platforms with T-Head CPU cores that support the standard
>> +		 * version of vector must provide the explicit V property,
>> +		 * which is well defined.
>> +		 */
>> +		if (acpi_disabled && riscv_cached_mvendorid(cpu) == THEAD_VENDOR_ID) {
>> +			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
>> --
>> 2.39.2
>>
>>
>> _______________________________________________
>> linux-riscv mailing list
>> linux-riscv at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv