Re: [PATCH v4 7/9] riscv: vector: adjust minimum Vector requirement to ZVE32X

[Eric Biggers]

On Thu, Apr 18, 2024 at 07:26:00PM +0100, Conor Dooley wrote:
> That's great if it does require that the version of the vector extension
> must be standard. Higher quality spec than most if it does. But
> "supported" in the context of riscv_isa_extension_available() means that
> the hardware supports it (or set of harts), not that the currently
> running kernel does. The Kconfig deps that must be met for the code to be
> built at least mean the kernel is built with vector support, leaving only
> "the kernel was built with vector support and the hardware supports vector
> but for $reason the kernel refused to enable it".
> 
> I'm not sure if that final condition is actually possible with the system
> ending up in a broken state, however - I'm not sure that we ever do turn
> off access to the VPU at present (after we mark it usable), and if we do
> it doesn't get reflected in has_vector() so the kernel and userspace would
> both break, with what a crypto driver does probably being the least of
> your worries.
>
> > I am just concerned about how you're suggesting that non-standard extensions
> > might be pretending to be standard ones and individual users of kernel-mode
> > vector would need to work around that.
> 
> I am absolutely not suggesting that non-standard extensions should
> masquerade as standard ones, I don't know where you got that from. What
> I said was that a non-standard vector extension could reuse riscv_v_vlen
> (and should IMO for simplicity reasons), not that any of the APIs we have
> for checking extension availability would lie and say it was standard.
> riscv_v_vlen having a value greater than 128 is not one of those APIs ;)

It sounded like you were suggesting that a CPU could plausibly have a
pre-standard version of the vector extension but also have standard Zvkb.  I
don't think this makes sense, due to the dependency.

> > I think that neither has_vector() nor
> > 'if (riscv_isa_extension_available(NULL, ZVKB))' should return true if the CPU's
> > vector extension is non-standard.
> 
> riscv_isa_extension_available(NULL, ZVKB) only checks whether the extension
> was present in DT or ACPI for all harts. It doesn't check whether or not
> the required config option for vector has been set or anything related
> to dependencies. has_vector() at least checks that the vector core has
> been enabled (and uses the alternative-patched version of the check
> given it is used in some hotter paths). That's kinda moot for code
> that's only built if the vector core stuff is enabled as I said above
> though.
> 
> We could of course make riscv_isa_extension_available() check
> extension dependencies, but I'd rather leave dt validation to the dt
> tooling (apparently ACPI tables are never wrong...). Either would allow
> you to rely on the crypto extensions present only when the standard vector
> extensions unless someone's DT/ACPI stuff is shite, but then they keep the
> pieces IMO :)
> 
> Hope that makes sense?

If the RISC-V kernel ever disables V, then it should also disable everything
that depends on V.

This would be similar to how on x86, if the kernel decides to disable AVX to
mitigate the Gather Data Sampling vulnerability, it also disables AVX2, AVX512,
VAES, VPCLMULQDQ, etc.  See cpuid_deps[] in arch/x86/kernel/cpu/cpuid-deps.c.

Sometimes CPU features depend on other ones.  That's just the way things work.
Whenever possible that should be handled centrally, not pushed down to every
user both in-kernel and userspace.

- Eric