Received-SPF: pass (google.com: domain of linux-kernel+bounces-174219-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240412-zve-detection-v4-0-e0c45bb6b253@sifive.com>
 <20240412-zve-detection-v4-7-e0c45bb6b253@sifive.com> <20240418-brook-chili-4d3e61d1a55c@wendy>
 <20240418155256.GA2410@sol.localdomain> <20240418-ultimatum-yam-11de4b063b83@spud>
 <20240418173203.GA1081@sol.localdomain> <20240418173946.GB1081@sol.localdomain>
 <20240418-sterling-sanding-d59c3b0a2aaa@spud>
In-Reply-To: <20240418-sterling-sanding-d59c3b0a2aaa@spud>
From: Andy Chiu <andy.chiu@sifive.com>
Date: Thu, 9 May 2024 14:56:30 +0800
Message-ID: <CABgGipU74TA3KgCH4pPuRefbnYt3q6RKcQwfyspenisEtY6eqw@mail.gmail.com>
Subject: Re: [PATCH v4 7/9] riscv: vector: adjust minimum Vector requirement
 to ZVE32X
To: Conor Dooley <conor@kernel.org>
Cc: Eric Biggers <ebiggers@kernel.org>, Conor Dooley <conor.dooley@microchip.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>, 
	=?UTF-8?B?Q2zDqW1lbnQgTMOpZ2Vy?= <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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Conor,

On Fri, Apr 19, 2024 at 2:26=E2=80=AFAM Conor Dooley <conor@kernel.org> wro=
te:
>
> 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 c=
onfigurations.
> > > > > Some of it should work, but some of it won't be able to work.  (F=
or example, the
> > > > > SHA512 instructions require EEW=3D=3D64.)
> > > > >
> > > > > Also note that currently all the RISC-V vector crypto code only s=
upports riscv64
> > > > > (XLEN=3D64).  Similarly, that could be relaxed in the future if p=
eople 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 co=
ntext.  This
> > > > > differs from shash algorithms which can be invoked in any context=
.
> > > > >
> > > > > My understanding is that, like arm64, RISC-V always allows non-ne=
sted
> > > > > kernel-mode vector to be used in process and softirq context -- a=
nd in fact,
> > > > > this was intentionally done in order to support use cases like th=
is.  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 chec=
k
> > > > somewhere if has_vector() returns true in that driver though before
> > > > using vector instructions. Only checking vlen seems to me like rely=
ing on
> > > > an implementation detail and if we set vlen for the T-Head/0.7.1 ve=
ctor
> > > > it'd be fooled. That said, I don't think that any of the 0.7.1 vect=
or
> > > > systems actually support Zvkb, but I hope you get my drift.
> > >
> > > All the algorithms check for at least one of the vector crypto extens=
ions 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 exten=
sion.
>
> 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 vecto=
r
> 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 woul=
d
> 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 ar=
e 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 ca=
n
> 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 exten=
sions
> > 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 extensi=
on
> 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 vecto=
r
> extensions unless someone's DT/ACPI stuff is shite, but then they keep th=
e
> pieces IMO :)

Should we check if "v" presents for vector crypto extensions in
riscv_isa_extension_check()? We are not checking this for now. So a
kernel compiled with RISCV_ISA_V still has a problem if its isa-string
includes any of vector crypto ("zvbb, zvkg, etc") but not "v".

>
> Hope that makes sense?
> Conor.

Cheers,
Andy