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

Marc Zyngier
2025-11-25T13:48:07Z

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.