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

From: Marc Zyngier <maz@kernel.org>
To: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Ard Biesheuvel <ardb@kernel.org>,
	James Morse <james.morse@arm.com>,
	Oliver Upton <oliver.upton@linux.dev>,
	Zenghui Yu <yuzenghui@huawei.com>
Subject: Re: [PATCH v2 02/13] arm64: cpufeatures: Correctly handle signed values
Date: Wed, 22 Nov 2023 09:46:28 +0000	[thread overview]
Message-ID: <86leaqyvbf.wl-maz@kernel.org> (raw)
In-Reply-To: <2fb7398f-b865-44a7-8ee0-4a32ae40afa8@arm.com>

On Wed, 22 Nov 2023 09:29:50 +0000,
Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
> 
> On 20/11/2023 12:37, 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@kernel.org>
> > ---
> >   arch/arm64/include/asm/cpufeature.h |  1 +
> >   arch/arm64/kernel/cpufeature.c      | 66 +++++++++++++++++++++++++----
> >   2 files changed, 58 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..e52d2c2b757f 100644
> > --- a/arch/arm64/kernel/cpufeature.c
> > +++ b/arch/arm64/kernel/cpufeature.c
> > @@ -140,12 +140,43 @@ void dump_cpu_features(void)
> >   	pr_emerg("0x%*pb\n", ARM64_NCAPS, &system_cpucaps);
> >   }
> >   +#define __ARM64_EXPAND_RFV(reg, field, val)
> > reg##_##field##_##val
> 
> We have defined SYS_FIELD_VALUE to be the exact same thing in Patch 1
> and we later remove this and switch to using the same in Patch 8.
> Could we not do this straight away here ? i.e. use the SYS_FIELD_VALUE
> instead of adding this ?

Because I'm a moron and updated everything *except* my own patches.
Apologies for the noise, I'll fix that shortly.

> Rest looks good to me.

Thanks!

	M.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel