From: Conor Dooley <conor@kernel.org>
To: Eric Biggers <ebiggers@kernel.org>
Cc: "Conor Dooley" <conor.dooley@microchip.com>,
"Andy Chiu" <andy.chiu@sifive.com>,
"Paul Walmsley" <paul.walmsley@sifive.com>,
"Palmer Dabbelt" <palmer@dabbelt.com>,
"Albert Ou" <aou@eecs.berkeley.edu>,
"Heiko Stuebner" <heiko@sntech.de>, "Guo Ren" <guoren@kernel.org>,
"Rob Herring" <robh@kernel.org>,
"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
"Jonathan Corbet" <corbet@lwn.net>,
"Evan Green" <evan@rivosinc.com>,
"Clément Léger" <cleger@rivosinc.com>,
"Shuah Khan" <shuah@kernel.org>,
linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org,
"Palmer Dabbelt" <palmer@rivosinc.com>,
"Vincent Chen" <vincent.chen@sifive.com>,
"Greentime Hu" <greentime.hu@sifive.com>,
devicetree@vger.kernel.org, linux-doc@vger.kernel.org,
linux-kselftest@vger.kernel.org,
"Joel Granados" <j.granados@samsung.com>,
"Jerry Shih" <jerry.shih@sifive.com>
Subject: Re: [PATCH v4 7/9] riscv: vector: adjust minimum Vector requirement to ZVE32X
Date: Thu, 18 Apr 2024 19:28:09 +0100 [thread overview]
Message-ID: <20240418-critter-pulp-4d68456fccf3@spud> (raw)
In-Reply-To: <20240418-sterling-sanding-d59c3b0a2aaa@spud>
[-- Attachment #1: Type: text/plain, Size: 6649 bytes --]
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
next prev parent reply other threads:[~2024-04-18 18:28 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-12 6:48 [PATCH v4 0/9] Support Zve32[xf] and Zve64[xfd] Vector subextensions Andy Chiu
2024-04-12 6:48 ` [PATCH v4 1/9] riscv: vector: add a comment when calling riscv_setup_vsize() Andy Chiu
2024-04-18 9:54 ` Conor Dooley
2024-04-12 6:48 ` [PATCH v4 2/9] riscv: smp: fail booting up smp if inconsistent vlen is detected Andy Chiu
2024-04-18 10:17 ` Conor Dooley
2024-04-19 6:09 ` [External] " yunhui cui
2024-04-24 20:01 ` Alexandre Ghiti
2024-05-08 8:21 ` Andy Chiu
2024-05-08 10:43 ` Alexandre Ghiti
2024-04-12 6:48 ` [PATCH v4 3/9] riscv: cpufeature: call match_isa_ext() for single-letter extensions Andy Chiu
2024-04-18 10:29 ` Conor Dooley
2024-04-12 6:49 ` [PATCH v4 4/9] riscv: cpufeature: add zve32[xf] and zve64[xfd] isa detection Andy Chiu
2024-04-18 10:19 ` Conor Dooley
2024-04-12 6:49 ` [PATCH v4 5/9] dt-bindings: riscv: add Zve32[xf] Zve64[xfd] ISA extension description Andy Chiu
2024-04-18 10:21 ` Conor Dooley
2024-04-12 6:49 ` [PATCH v4 6/9] riscv: hwprobe: add zve Vector subextensions into hwprobe interface Andy Chiu
2024-04-12 6:49 ` [PATCH v4 7/9] riscv: vector: adjust minimum Vector requirement to ZVE32X Andy Chiu
2024-04-18 11:02 ` Conor Dooley
2024-04-18 15:52 ` Eric Biggers
2024-04-18 16:53 ` Conor Dooley
2024-04-18 17:32 ` Eric Biggers
2024-04-18 17:39 ` Eric Biggers
2024-04-18 18:26 ` Conor Dooley
2024-04-18 18:28 ` Conor Dooley [this message]
2024-04-18 18:41 ` Eric Biggers
2024-04-18 20:00 ` Conor Dooley
2024-05-09 6:56 ` Andy Chiu
2024-05-09 7:48 ` Conor Dooley
2024-05-09 8:25 ` Conor Dooley
2024-05-09 22:22 ` Conor Dooley
2024-04-12 6:49 ` [PATCH v4 8/9] hwprobe: fix integer promotion in RISCV_HWPROBE_EXT macro Andy Chiu
2024-04-12 6:49 ` [PATCH v4 9/9] selftest: run vector prctl test for ZVE32X Andy Chiu
2024-04-25 23:00 ` [PATCH v4 0/9] Support Zve32[xf] and Zve64[xfd] Vector subextensions patchwork-bot+linux-riscv
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240418-critter-pulp-4d68456fccf3@spud \
--to=conor@kernel.org \
--cc=andy.chiu@sifive.com \
--cc=aou@eecs.berkeley.edu \
--cc=cleger@rivosinc.com \
--cc=conor.dooley@microchip.com \
--cc=corbet@lwn.net \
--cc=devicetree@vger.kernel.org \
--cc=ebiggers@kernel.org \
--cc=evan@rivosinc.com \
--cc=greentime.hu@sifive.com \
--cc=guoren@kernel.org \
--cc=heiko@sntech.de \
--cc=j.granados@samsung.com \
--cc=jerry.shih@sifive.com \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=linux-riscv@lists.infradead.org \
--cc=palmer@dabbelt.com \
--cc=palmer@rivosinc.com \
--cc=paul.walmsley@sifive.com \
--cc=robh@kernel.org \
--cc=shuah@kernel.org \
--cc=vincent.chen@sifive.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).