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

[Conor Dooley <conor@kernel.org>]

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

On Tue, Feb 11, 2025 at 11:16:45AM +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 extensions. From the
> > kernel's perfective, it's not required to differentiate between the
> > conditions for all the various vector subsets - it's the firmware's job
> > to not report impossible combinations. Instead, the kernel only has to
> > check that the correct config options are enabled and to enforce its
> > requirement of the d extension being present for FPU support.
> > 
> > Since vector will now be disabled proactively, there's no need to clear
> > the bit in elf_hwcap in riscv_fill_hwcap() any longer.
> > 
> > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> > ---
> >  arch/riscv/include/asm/cpufeature.h |  3 ++
> >  arch/riscv/kernel/cpufeature.c      | 57 +++++++++++++++++++----------
> >  2 files changed, 40 insertions(+), 20 deletions(-)
> > 
> > diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h
> > index 569140d6e639..5d9427ccbc7a 100644
> > --- a/arch/riscv/include/asm/cpufeature.h
> > +++ b/arch/riscv/include/asm/cpufeature.h
> > @@ -56,6 +56,9 @@ void __init riscv_user_isa_enable(void);
> >  #define __RISCV_ISA_EXT_BUNDLE(_name, _bundled_exts) \
> >  	_RISCV_ISA_EXT_DATA(_name, RISCV_ISA_EXT_INVALID, _bundled_exts, \
> >  			    ARRAY_SIZE(_bundled_exts), NULL)
> > +#define __RISCV_ISA_EXT_BUNDLE_VALIDATE(_name, _bundled_exts, _validate) \
> > +	_RISCV_ISA_EXT_DATA(_name, RISCV_ISA_EXT_INVALID, _bundled_exts, \
> > +			    ARRAY_SIZE(_bundled_exts), _validate)
> >  
> >  /* Used to declare extensions that are a superset of other extensions (Zvbb for instance) */
> >  #define __RISCV_ISA_EXT_SUPERSET(_name, _id, _sub_exts) \
> > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > index c6ba750536c3..40a24b08d905 100644
> > --- a/arch/riscv/kernel/cpufeature.c
> > +++ b/arch/riscv/kernel/cpufeature.c
> > @@ -109,6 +109,35 @@ static int riscv_ext_zicboz_validate(const struct riscv_isa_ext_data *data,
> >  	return 0;
> >  }
> >  
> > +static int riscv_ext_vector_x_validate(const struct riscv_isa_ext_data *data,
> > +				       const unsigned long *isa_bitmap)
> > +{
> > +	if (!IS_ENABLED(CONFIG_RISCV_ISA_V))
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> > +static int riscv_ext_vector_float_validate(const struct riscv_isa_ext_data *data,
> > +					   const unsigned long *isa_bitmap)
> > +{
> > +	if (!IS_ENABLED(CONFIG_RISCV_ISA_V))
> > +		return -EINVAL;
> > +
> > +	if (!IS_ENABLED(CONFIG_FPU))
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * The kernel doesn't support systems that don't implement both of
> > +	 * F and D, so if any of the vector extensions that do floating point
> > +	 * are to be usable, both floating point extensions need to be usable.
> > +	 */
> > +	if (!__riscv_isa_extension_available(isa_bitmap, RISCV_ISA_EXT_d))
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> 
> I think this should also be modified to be like this:
> 
>   if (__riscv_isa_extension_available(isa_bitmap, RISCV_ISA_EXT_d))
> 	  return 0;
> 
>   return -EPROBEDEFER;
> 
> That won't actually change the way it works since RISCV_ISA_EXT_d (and
> all single letter extensions) is always probed before the others though.

I don't think so, there's no point deferring since we know that the
extensions this is used for cannot become true afterwards. I'll add a
comment justifying it.

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