On Thu, Apr 18, 2024 at 07:26:00PM +0100, Conor Dooley wrote: > 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. Poor choice of wording here - I meant, of course, the "main" vector extension, rather than the Zve* variants. > 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
