[PATCH v2] RISC-V: Don't trust V from the riscv,isa DT property on T-Head CPUs
Conor Dooley
conor at kernel.org
Thu Jul 13 10:04:22 PDT 2023
Jumping on top of Palmer's reply cos I had already started replying...
On Thu, Jul 13, 2023 at 09:56:34AM -0700, Palmer Dabbelt wrote:
> 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).
(In reply to Jisheng mostly)
It is most definitely a big hammer. And yes, we did talk about the c908
& its standard implementation of vector before submitting this. Unless
Guo can confirm that the c908 (and later CPU cores) will start setting
mimpid & mvendorid, I don't really see what the alternatives are? *
Whacking in a list of DT compatibles to blacklist? That doesn't seem
like something that would scale.
Open to ideas on that front for sure, smaller hammers are always better!
@Palmer, from what I am told, the c920 does put zeros in those CSRs,
so we are okay on that front.
* If they do do something other than 0s, the errata handling will need
an update anyway, so the big hammer could be revised in tandem...
> > 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.
(Mostly in reply to Jisheng again)
Sure, some devicetrees are part of the kernel, but not all are - they may
be passed up from U-Boot or OpenSBI etc & contain "v" in riscv,isa.
The intent of this patch (and Palmer's v1) is to make sure such systems
do not tell the kernel they support the standard version of v, when they
do not do so.
> 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.
Aye, AFAIK there are no extant systems with a c908 outside of vendors?
At least, from what searching I did I could not find a way to acquire
one... If you read the patch, you will see that there is in fact a way
out being provided & it is not a "no T-Head can never have vector"
situation - just slightly earlier use of the new properties in the
kernel than we perhaps intended.
Thanks,
Conor.
> > > 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
-------------- 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/20230713/13b356c7/attachment.sig>
More information about the linux-riscv
mailing list