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

[Conor Dooley]

On Thu, Apr 18, 2024 at 10:39:46AM -0700, Eric Biggers wrote:
> On Thu, Apr 18, 2024 at 10:32:03AM -0700, Eric Biggers wrote:
> > On Thu, Apr 18, 2024 at 05:53:55PM +0100, Conor Dooley wrote:
> > > > If it would be useful to do so, we should be able to enable some of the code
> > > > with a smaller VLEN and/or EEW once it has been tested in those configurations.
> > > > Some of it should work, but some of it won't be able to work.  (For example, the
> > > > SHA512 instructions require EEW==64.)
> > > > 
> > > > Also note that currently all the RISC-V vector crypto code only supports riscv64
> > > > (XLEN=64).  Similarly, that could be relaxed in the future if people really need
> > > > the vector crypto acceleration on 32-bit CPUs...  But similarly, the code would
> > > > need to be revised and tested in that configuration.
> > > > 
> > > > > Eric/Jerry (although read the previous paragraph too):
> > > > > I noticed that the sha256 glue code calls crypto_simd_usable(), and in
> > > > > turn may_use_simd() before kernel_vector_begin(). The chacha20 glue code
> > > > > does not call either, which seems to violate the edict in
> > > > > kernel_vector_begin()'s kerneldoc:
> > > > > "Must not be called unless may_use_simd() returns true."
> > > > 
> > > > skcipher algorithms can only be invoked in process and softirq context.  This
> > > > differs from shash algorithms which can be invoked in any context.
> > > > 
> > > > My understanding is that, like arm64, RISC-V always allows non-nested
> > > > kernel-mode vector to be used in process and softirq context -- and in fact,
> > > > this was intentionally done in order to support use cases like this.  So that's
> > > > why the RISC-V skcipher algorithms don't check for may_use_simd() before calling
> > > > kernel_vector_begin().
> > > 
> > > I see, thanks for explaining that. I think you should probably check
> > > somewhere if has_vector() returns true in that driver though before
> > > using vector instructions. Only checking vlen seems to me like relying on
> > > an implementation detail and if we set vlen for the T-Head/0.7.1 vector
> > > it'd be fooled. That said, I don't think that any of the 0.7.1 vector
> > > systems actually support Zvkb, but I hope you get my drift.
> > 
> > All the algorithms check for at least one of the vector crypto extensions being
> > supported, for example Zvkb.  'if (riscv_isa_extension_available(NULL, ZVKB))'
> > should return whether the ratified version of Zvkb is supported, and likewise
> > for the other vector crypto extensions.  The ratified version of the vector
> > crypto extensions depends on the ratified version of the vector extension.

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.

> > So
> > there should be no issue.  If there is, the RISC-V core architecture code needs
> > to be fixed to not declare that extensions are supported when they are actually
> > incompatible non-standard versions of those extensions.  Incompatible
> > non-standard extensions should be represented as separate extensions.
> > 
> 
> It probably makes sense to check has_vector() to exclude Zve* for now, though.

I think you might actually be better served at present, given the code can
only be built if the core vector code is, by using
riscv_isa_extension_available(NULL, v). That way you know for sure that
you're getting the ratified extension and nothing else.

Prior to this conversation I thought that has_vector() should return true
if there's a standard compliant vector unit available - given all users
Andy added only need Zve32x.

> 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 ;)

> 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?
Conor.

Attachment: signature.asc
Description: PGP signature