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

Palmer Dabbelt
2023-07-13T17:43:49Z

On Thu, 13 Jul 2023 10:33:56 PDT (-0700), guoren@kernel.org wrote:
> On Thu, Jul 13, 2023 at 12:48 PM Jisheng Zhang <jszhang@kernel.org> wrote:
>>
>> On Wed, Jul 12, 2023 at 06:48:02PM +0100, Conor Dooley wrote:
>> > From: Palmer Dabbelt <palmer@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
>> 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.
>>
>> 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@rivosinc.com>
>> > Co-developed-by: Conor Dooley <conor.dooley@microchip.com>
>> > Signed-off-by: Conor Dooley <conor.dooley@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.
> T-HEAD's vector 1.0 SoCs is in circulation. Kendryte K230 is the
> shipped SoC chip, which vendor id = THEAD_VENDOR_ID and with vector
> 1.0.

Do you have a pointer to where they're availiable? Kendryte's website
doesn't list a K230 yet: https://www.canaan.io/product/kendryteai## .
They'd be the first V-1.0 implementation in the wild, I bet a lot of
people would want one to play with...

>
>> > + * 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 you insist on doing this, please:
>
> if (acpi_disabled && riscv_cached_mvendorid(cpu) == THEAD_VENDOR_ID &&
> riscv_cached_marchid(cpu) == 0 && riscv_cached_mimpid(cpu) == 0) {

From above it sounds like that's OK for the C920. Does the C908 set a
non-zero m{arch,impl}id? If not, then this won't help anything.

Also: does the K230 work with upstream kernels yet? I don't see any DT
stuff for it, so aside from the unlikely event it's quirk-free then it'd
need DTs changed by upstream review and thus would be forced to use the
new properties anyway.

>> > + 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@lists.infradead.org
>> > http://lists.infradead.org/mailman/listinfo/linux-riscv
>
>
>
> --
> Best Regards
> Guo Ren