Re: [PATCH v4 06/49] KVM: arm64: GICv3: Detect and work around the lack of ICV_DIR_EL1 trapping

[Marc Zyngier <maz@kernel.org>]

On Tue, 25 Nov 2025 11:26:10 +0000,
Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
> 
> On 20/11/2025 17:24, Marc Zyngier wrote:
> > A long time ago, an unsuspecting architect forgot to add a trap
> > bit for ICV_DIR_EL1 in ICH_HCR_EL2. Which was unfortunate, but
> > what's a bit of spec between friends? Thankfully, this was fixed
> > in a later revision, and ARM "deprecates" the lack of trapping
> > ability.
> > 
> > Unfortuantely, a few (billion) CPUs went out with that defect,
> > anything ARMv8.0 from ARM, give or take. And on these CPUs,
> > you can't trap DIR on its own, full stop.
> > 
> > As the next best thing, we can trap everything in the common group,
> > which is a tad expensive, but hey ho, that's what you get. You can
> > otherwise recycle the HW in the neaby bin.
> > 
> > Tested-by: Fuad Tabba <tabba@google.com>
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >   arch/arm64/include/asm/virt.h  |  7 ++++-
> >   arch/arm64/kernel/cpufeature.c | 52 ++++++++++++++++++++++++++++++++++
> >   arch/arm64/kernel/hyp-stub.S   |  5 ++++
> >   arch/arm64/kvm/vgic/vgic-v3.c  |  3 ++
> >   arch/arm64/tools/cpucaps       |  1 +
> >   5 files changed, 67 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/include/asm/virt.h b/arch/arm64/include/asm/virt.h
> > index aa280f356b96a..8eb63d3294974 100644
> > --- a/arch/arm64/include/asm/virt.h
> > +++ b/arch/arm64/include/asm/virt.h
> > @@ -40,8 +40,13 @@
> >    */
> >   #define HVC_FINALISE_EL2	3
> >   +/*
> > + * HVC_GET_ICH_VTR_EL2 - Retrieve the ICH_VTR_EL2 value
> > + */
> > +#define HVC_GET_ICH_VTR_EL2	4
> > +
> >   /* Max number of HYP stub hypercalls */
> > -#define HVC_STUB_HCALL_NR 4
> > +#define HVC_STUB_HCALL_NR 5
> >     /* Error returned when an invalid stub number is passed into x0
> > */
> >   #define HVC_STUB_ERR	0xbadca11
> > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> > index 5ed401ff79e3e..5de51cb1b8fe2 100644
> > --- a/arch/arm64/kernel/cpufeature.c
> > +++ b/arch/arm64/kernel/cpufeature.c
> > @@ -2303,6 +2303,49 @@ static bool has_gic_prio_relaxed_sync(const struct arm64_cpu_capabilities *entry
> >   }
> >   #endif
> >   +static bool can_trap_icv_dir_el1(const struct
> > arm64_cpu_capabilities *entry,
> > +				 int scope)
> > +{
> > +	static const struct midr_range has_vgic_v3[] = {
> > +		MIDR_ALL_VERSIONS(MIDR_APPLE_M1_ICESTORM),
> > +		MIDR_ALL_VERSIONS(MIDR_APPLE_M1_FIRESTORM),
> > +		MIDR_ALL_VERSIONS(MIDR_APPLE_M1_ICESTORM_PRO),
> > +		MIDR_ALL_VERSIONS(MIDR_APPLE_M1_FIRESTORM_PRO),
> > +		MIDR_ALL_VERSIONS(MIDR_APPLE_M1_ICESTORM_MAX),
> > +		MIDR_ALL_VERSIONS(MIDR_APPLE_M1_FIRESTORM_MAX),
> > +		MIDR_ALL_VERSIONS(MIDR_APPLE_M2_BLIZZARD),
> > +		MIDR_ALL_VERSIONS(MIDR_APPLE_M2_AVALANCHE),
> > +		MIDR_ALL_VERSIONS(MIDR_APPLE_M2_BLIZZARD_PRO),
> > +		MIDR_ALL_VERSIONS(MIDR_APPLE_M2_AVALANCHE_PRO),
> > +		MIDR_ALL_VERSIONS(MIDR_APPLE_M2_BLIZZARD_MAX),
> > +		MIDR_ALL_VERSIONS(MIDR_APPLE_M2_AVALANCHE_MAX),
> > +		{},
> > +	};
> > +	struct arm_smccc_res res = {};
> > +
> > +	BUILD_BUG_ON(ARM64_HAS_ICH_HCR_EL2_TDIR <= ARM64_HAS_GICV3_CPUIF);
> > +	BUILD_BUG_ON(ARM64_HAS_ICH_HCR_EL2_TDIR <= ARM64_HAS_GICV5_LEGACY);
> > +	if (!cpus_have_cap(ARM64_HAS_GICV3_CPUIF) &&
> > +	    !is_midr_in_range_list(has_vgic_v3))
> > +		return false;
> > +
> > +	if (!is_hyp_mode_available())
> > +		return false;
> > +
> > +	if (cpus_have_cap(ARM64_HAS_GICV5_LEGACY))
> > +		return true;
> > +
> > +	if (is_kernel_in_hyp_mode())
> > +		res.a1 = read_sysreg_s(SYS_ICH_VTR_EL2);
> > +	else
> > +		arm_smccc_1_1_hvc(HVC_GET_ICH_VTR_EL2, &res);
> 
> We are reading the register on the current CPU and this capability,
> being a SYSTEM_FEATURE, relies on the "probing CPU". If there CPUs
> with differing values (which I don't think is practical, but hey,
> never say..). This is would better be a
> ARM64_CPUCAP_EARLY_LOCAL_CPU_FEATURE, which would run through all
> boot CPUs and would set the capability when it matches.

While I agree that SYSTEM_FEATURE is most probably the wrong thing, I
can't help but notice that

- ARM64_HAS_GICV3_CPUIF,
- ARM64_HAS_GIC_PRIO_MASKING
- ARM64_HAS_GIC_PRIO_RELAXED_SYNC

are all ARM64_CPUCAP_STRICT_BOOT_CPU_FEATURE.

On the other ARM64_HAS_GICV5_LEGACY is ARM64_CPUCAP_EARLY_LOCAL_CPU_FEATURE.

Given that ARM64_HAS_ICH_HCR_EL2_TDIR is dependent on both
ARM64_HAS_GICV3_CPUIF and ARM64_HAS_GICV5_LEGACY, shouldn't these two
(and their dependencies) be aligned to have the same behaviour?

Thanks,

	M.

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