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

[Guo Ren]

On Fri, Jul 14, 2023 at 1:43 AM Palmer Dabbelt <palmer at rivosinc.com> wrote:
>
> On Thu, 13 Jul 2023 10:33:56 PDT (-0700), guoren at kernel.org wrote:
> > On Thu, Jul 13, 2023 at 12:48 PM Jisheng Zhang <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
> >> 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 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.
> > 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...
Here is sales' link, the chips & EVB boards:
https://www.canaan-creative.com/product/k230

>
> >
> >> > +              * 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.
The c908 {arch,impl}id in k230 is non-zero, so it would help. Treat
all T-HEAD cores as 0.7.1 vector is not truth, only {arch,impl}id = 0
cores are vector 0.7.1.

And the all erratas of T-HEAD has the below code:
if (arch_id != 0 || impid != 0)
    return false;

So, we should do the same here.

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



-- 
Best Regards
 Guo Ren