[PATCH v3 02/13] arm64: cpufeatures: Correctly handle signed values

[Marc Zyngier]

On Mon, 11 Dec 2023 12:24:16 +0000,
Will Deacon <will at kernel.org> wrote:
> 
> On Mon, Nov 27, 2023 at 11:45:48AM +0000, Marc Zyngier wrote:
> > Although we've had signed values for some features such as PMUv3
> > and FP, the code that handles the comparaison with some limit
> > has a couple of annoying issues:
> > 
> > - the min_field_value is always unsigned, meaning that we cannot
> >   easily compare it with a negative value
> > 
> > - it is not possible to have a range of values, let alone a range
> >   of negative values
> > 
> > Fix this by:
> > 
> > - adding an upper limit to the comparison, defaulting to all bits
> >   being set to the maximum positive value
> > 
> > - ensuring that the signess of the min and max values are taken into
> >   account
> > 
> > A ARM64_CPUID_FIELDS_NEG() macro is provided for signed features, but
> > nothing is using it yet.
> > 
> > Signed-off-by: Marc Zyngier <maz at kernel.org>
> > ---
> >  arch/arm64/include/asm/cpufeature.h |  1 +
> >  arch/arm64/kernel/cpufeature.c      | 65 +++++++++++++++++++++++++----
> >  2 files changed, 57 insertions(+), 9 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> > index f6d416fe49b0..5f3f62efebd5 100644
> > --- a/arch/arm64/include/asm/cpufeature.h
> > +++ b/arch/arm64/include/asm/cpufeature.h
> > @@ -363,6 +363,7 @@ struct arm64_cpu_capabilities {
> >  			u8 field_pos;
> >  			u8 field_width;
> >  			u8 min_field_value;
> > +			u8 max_field_value;
> >  			u8 hwcap_type;
> >  			bool sign;
> >  			unsigned long hwcap;
> > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> > index 646591c67e7a..bc8787f28ffd 100644
> > --- a/arch/arm64/kernel/cpufeature.c
> > +++ b/arch/arm64/kernel/cpufeature.c
> > @@ -140,12 +140,42 @@ void dump_cpu_features(void)
> >  	pr_emerg("0x%*pb\n", ARM64_NCAPS, &system_cpucaps);
> >  }
> >  
> > +#define __ARM64_MAX_POSITIVE(reg, field)				\
> > +		((reg##_##field##_SIGNED ?				\
> > +		  BIT(reg##_##field##_WIDTH - 1) :			\
> > +		  BIT(reg##_##field##_WIDTH)) - 1)
> > +
> > +#define __ARM64_MIN_NEGATIVE(reg, field)  BIT(reg##_##field##_WIDTH - 1)
> 
> I'm struggling to grok these two macros. For example, let's say I have a
> 4-bit signed field. In that case, the maximum positive value is 7 (0b0111)
> and the minimum negative value is -8 (0b1000), but the macros above appear
> to give 0b1000 for both.

Crap. Well spotted. The signed maximum needs to be further adjusted
like this:

#define __ARM64_MAX_POSITIVE(reg, field)				\
		((reg##_##field##_SIGNED ?				\
		  BIT(reg##_##field##_WIDTH - 1) - 1:			\
		  BIT(reg##_##field##_WIDTH)) - 1)

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.