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

[Clément Léger]

On 11/02/2025 13:34, Conor Dooley wrote:
> 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@xxxxxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxxxxx>
>>> ---
>>>  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?

Yeah that would be helpful I guess.

Clément