Received-SPF: pass (google.com: domain of linux-kernel+bounces-150429-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 18 Apr 2024 08:52:56 -0700
From: Eric Biggers <ebiggers@kernel.org>
To: Conor Dooley <conor.dooley@microchip.com>
Cc: 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>, Conor Dooley <conor@kernel.org>,
	Rob Herring <robh@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Jonathan Corbet <corbet@lwn.net>, Evan Green <evan@rivosinc.com>,
	=?iso-8859-1?Q?Cl=E9ment_L=E9ger?= <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
Message-ID: <20240418155256.GA2410@sol.localdomain>
References: <20240412-zve-detection-v4-0-e0c45bb6b253@sifive.com>
 <20240412-zve-detection-v4-7-e0c45bb6b253@sifive.com>
 <20240418-brook-chili-4d3e61d1a55c@wendy>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240418-brook-chili-4d3e61d1a55c@wendy>

Hi Conor,

On Thu, Apr 18, 2024 at 12:02:10PM +0100, Conor Dooley wrote:
> +CC Eric, Jerry
> 
> On Fri, Apr 12, 2024 at 02:49:03PM +0800, Andy Chiu wrote:
> > Make has_vector take one argument. This argument represents the minimum
> > Vector subextension that the following Vector actions assume.
> > 
> > Also, change riscv_v_first_use_handler(), and boot code that calls
> > riscv_v_setup_vsize() to accept the minimum Vector sub-extension,
> > ZVE32X.
> > 
> > Most kernel/user interfaces requires minimum of ZVE32X. Thus, programs
> > compiled and run with ZVE32X should be supported by the kernel on most
> > aspects. This includes context-switch, signal, ptrace, prctl, and
> > hwprobe.
> > 
> > One exception is that ELF_HWCAP returns 'V' only if full V is supported
> > on the platform. This means that the system without a full V must not
> > rely on ELF_HWCAP to tell whether it is allowable to execute Vector
> > without first invoking a prctl() check.
> > 
> > Signed-off-by: Andy Chiu <andy.chiu@sifive.com>
> > Acked-by: Joel Granados <j.granados@samsung.com>
> 
> I'm not sure that I like this patch to be honest. As far as I can tell,
> every user here of has_vector(ext) is ZVE32X, so why bother actually
> having an argument?
> 
> Could we just document that has_vector() is just a tyre kick of "is
> there a vector unit and are we allowed to use it", and anything
> requiring more than the bare-minimum (so zve32x?)must explicitly check
> for that form of vector using riscv_has_extension_[un]likely()?
> 
> Finally, the in-kernel crypto stuff or other things that use
> can_use_simd() to check for vector support - do they all function correctly
> with all of the vector flavours? I don't understand the vector
> extensions well enough to evaluate that - I know that they do check for
> the individual extensions like Zvkb during probe but don't have anything
> for the vector version (at least in the chacha20 and sha256 glue code).
> If they don't, then we need to make sure those drivers do not probe with
> the cut-down variants.

As far as I know, none of the RISC-V vector crypto code has been tested with
Zve* yet.  Currently it always checks for VLEN >= 128, which should exclude most
Zve* implementations.

Currently it doesn't check for EEW >= 64, even though it sometimes assumes that.
It looks like a check for EEW >= 64 needs to be added in order to exclude Zve32x
and Zve32f implementations that don't support EEW == 64.

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().

Has that changed?  If so, why?

Some architectures like x86 do provide no-SIMD fallbacks for all skcipher
algorithms, but it's very annoying to do.  We were hoping to avoid that in
RISC-V.

- Eric