Re: [PATCH 1/2] arm64: cpufeature: Add GIC CPUIF v4.1 detection

[Lorenzo Pieralisi]

On Thu, Nov 12, 2020 at 11:20:08AM +0000, Marc Zyngier wrote:
> On 2020-11-11 16:28, Lorenzo Pieralisi wrote:
> > GIC v4.1 introduced changes to the GIC CPU interface; systems that
> > integrate CPUs that do not support GIC v4.1 features (as reported in
> > the
> > ID_AA64PFR0_EL1.GIC bitfield) and a GIC v4.1 controller must disable in
> > software virtual SGIs support since the CPUIF and GIC controller
> > version
> > mismatch results in CONSTRAINED UNPREDICTABLE behaviour at
> > architectural
> > level.
> > 
> > Add a cpufeature and related capability to detect GIC v4.1 CPUIF
> > features so that the GIC driver can probe it to detect GIC CPUIF
> > hardware configuration and take action accordingly.
> > 
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
> > Cc: Will Deacon <will@xxxxxxxxxx>
> > Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
> > Cc: Marc Zyngier <maz@xxxxxxxxxx>
> > ---
> >  arch/arm64/include/asm/cpucaps.h |  3 ++-
> >  arch/arm64/kernel/cpufeature.c   | 10 ++++++++++
> >  2 files changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/include/asm/cpucaps.h
> > b/arch/arm64/include/asm/cpucaps.h
> > index 42868dbd29fd..35ef0319f422 100644
> > --- a/arch/arm64/include/asm/cpucaps.h
> > +++ b/arch/arm64/include/asm/cpucaps.h
> > @@ -65,7 +65,8 @@
> >  #define ARM64_HAS_ARMv8_4_TTL			55
> >  #define ARM64_HAS_TLB_RANGE			56
> >  #define ARM64_MTE				57
> > +#define ARM64_HAS_GIC_CPUIF_VSGI		58
> > 
> > -#define ARM64_NCAPS				58
> > +#define ARM64_NCAPS				59
> > 
> >  #endif /* __ASM_CPUCAPS_H */
> > diff --git a/arch/arm64/kernel/cpufeature.c
> > b/arch/arm64/kernel/cpufeature.c
> > index dcc165b3fc04..9eabbaddfe5e 100644
> > --- a/arch/arm64/kernel/cpufeature.c
> > +++ b/arch/arm64/kernel/cpufeature.c
> > @@ -2136,6 +2136,16 @@ static const struct arm64_cpu_capabilities
> > arm64_features[] = {
> >  		.cpu_enable = cpu_enable_mte,
> >  	},
> >  #endif /* CONFIG_ARM64_MTE */
> > +	{
> > +		.desc = "GIC CPUIF virtual SGI",
> 
> nit: that's not really what this feature is. It only means that the
> sysreg interface complies to v4.1. Which on its own is totally rubbish,
> because the sysreg don't change behaviour between 3.0/4.0 and 4.1.

True.

> > +		.capability = ARM64_HAS_GIC_CPUIF_VSGI,
> > +		.type = ARM64_CPUCAP_BOOT_CPU_FEATURE,
> > +		.matches = has_cpuid_feature,
> > +		.sys_reg = SYS_ID_AA64PFR0_EL1,
> > +		.field_pos = ID_AA64PFR0_GIC_SHIFT,
> > +		.sign = FTR_UNSIGNED,
> > +		.min_field_value = 3,
> > +	},
> 
> Do we really need a new cap for that? Or can we rely on simply looking
> at the sanitised feature set? I'm not overly keen on advertising a
> feature
> at CPU boot time if we discover later on that we cannot use it because
> all
> we have in a non-4.1 GIC.

Yes I thought about that, posted exactly to get this feedback.

You have a point. It is also wrong to force secondaries to die if
they mismatch with the boot CPU without knowing what *actual* GIC
controller we have in the system.

> Another thing is that we currently assume that *all* CPUs will be the
> same
> at the point where we setup the GIC (we only have a single CPU booted at
> that
> point).

Yes - we would taint (?) if that's not the case right ? So using a
sanitised feature looks sane to me, certainly saner than using a new
cap and I think that's all we can and should do.

Thanks,
Lorenzo