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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 12 Nov 2020 14:55:14 +0000
From:   Lorenzo Pieralisi <lorenzo.pieralisi@....com>
To:     Marc Zyngier <maz@...nel.org>
Cc:     linux-kernel@...r.kernel.org, Will Deacon <will@...nel.org>,
        Catalin Marinas <catalin.marinas@....com>,
        LAKML <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 1/2] arm64: cpufeature: Add GIC CPUIF v4.1 detection

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@....com>
> > Cc: Will Deacon <will@...nel.org>
> > Cc: Catalin Marinas <catalin.marinas@....com>
> > Cc: Marc Zyngier <maz@...nel.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