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.pieral...@arm.com>
> > Cc: Will Deacon <w...@kernel.org>
> > Cc: Catalin Marinas <catalin.mari...@arm.com>
> > Cc: Marc Zyngier <m...@kernel.org>
> > ---
> > 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
