Re: [PATCH v3 2/6] RISC-V: add vector crypto extension validation checks

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3398 bytes --]

On Tue, Feb 11, 2025 at 09:45:44AM +0100, Clément Léger wrote:
> 
> 
> On 05/02/2025 17:05, Conor Dooley wrote:
> > From: Conor Dooley <conor.dooley@microchip.com>
> > 
> > Using Clement's new validation callbacks, support checking that
> > dependencies have been satisfied for the vector crpyto extensions.
> > Currently riscv_isa_extension_available(<vector crypto>) will return
> > true on systems that support the extensions but vector itself has been
> > disabled by the kernel, adding validation callbacks will prevent such a
> > scenario from occuring and make the behaviour of the extension detection
> > functions more consistent with user expectations - it's not expected to
> > have to check for vector AND the specific crypto extension.
> > 
> > The 1.0.0 Vector crypto spec states:
> > 	The Zvknhb and Zvbc Vector Crypto Extensions --and accordingly
> > 	the composite extensions Zvkn and Zvks-- require a Zve64x base,
> > 	or application ("V") base Vector Extension. All of the other
> > 	Vector Crypto Extensions can be built on any embedded (Zve*) or
> > 	application ("V") base Vector Extension.
> > and this could be used as the basis for checking that the correct base
> > for individual crypto extensions, but that's not really the kernel's job
> > in my opinion and it is sufficient to leave that sort of precision to
> > the dt-bindings. The kernel only needs to make sure that vector, in some
> > form, is available.
> > 
> > Since vector will now be disabled proactively, there's no need to clear
> > the bit in elf_hwcap in riscv_fill_hwcap() any longer.
> > 
> > Link: https://github.com/riscv/riscv-crypto/releases/tag/v1.0.0
> > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> > ---
> >  arch/riscv/kernel/cpufeature.c | 49 +++++++++++++++++++++++-----------
> >  1 file changed, 33 insertions(+), 16 deletions(-)
> > 
> > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > index 40a24b08d905..1c148ecea612 100644
> > --- a/arch/riscv/kernel/cpufeature.c
> > +++ b/arch/riscv/kernel/cpufeature.c
> > @@ -138,6 +138,23 @@ static int riscv_ext_vector_float_validate(const struct riscv_isa_ext_data *data
> >  	return 0;
> >  }
> >  
> > +static int riscv_ext_vector_crypto_validate(const struct riscv_isa_ext_data *data,
> > +					    const unsigned long *isa_bitmap)
> > +{
> > +	if (!IS_ENABLED(CONFIG_RISCV_ISA_V))
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * It isn't the kernel's job to check that the binding is correct, so
> > +	 * it should be enough to check that any of the vector extensions are
> > +	 * enabled, which in-turn means that vector is usable in this kernel
> > +	 */
> > +	if (!__riscv_isa_extension_available(isa_bitmap, RISCV_ISA_EXT_ZVE32X))
> > +		return -EINVAL;
> 
> After a second thought, I think it should be this:
> 
> if (__riscv_isa_extension_available(isa_bitmap, RISCV_ISA_EXT_ZVE32X))
> 	return 0;
> 
> return -EPROBEDEFER;
> 
> Extensions can be enabled later (but can not be "reverted") so check for
> the extension to be present (in which case it's ok), or wait for it to
> be (potentially) enabled.

Ah, of course it is operating on the /resolved/ isa, not the source one.
Makes me thing the parameter of all the validate callbacks should be
"resolved_isa_bitmap" instead of "isa_bitmap" to make things clearer?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]