[PATCH v4 06/49] KVM: arm64: GICv3: Detect and work around the lack of ICV_DIR_EL1 trapping
Marc Zyngier
maz at kernel.org
Tue Nov 25 07:01:18 PST 2025
On Tue, 25 Nov 2025 14:14:25 +0000,
Suzuki K Poulose <suzuki.poulose at arm.com> wrote:
>
> On 25/11/2025 13:48, Marc Zyngier wrote:
> > On Tue, 25 Nov 2025 11:26:10 +0000,
> > Suzuki K Poulose <suzuki.poulose at 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 at google.com>
> >>> Signed-off-by: Marc Zyngier <maz at 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.
>
> Which means, if GICV3_CPUIF is set any booting CPU must have them.
>
> >
> > 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?
>
> Yes, but it also depends on how you are detecting the feature.
>
> BOOT_CPU_FEATURES are finalized with BOOT CPU and thusanything that
> boots up later must have it. So it is fine, as long
> as a SYSTEM cap depends on it and can do its own additional checks
> in a system wide safe manner.
I had a quick go at it, and things are a bit murky. As it turns out,
the GICv3 stuff is STRICT_BOOT_CPU_FEATURE because we need the GIC
super early, before secondary CPUs can be brought up.
However, virtualisation-specific features only get used way down the
line, so both ICH_HCR_EL2_TDIR and GICV5_LEGACY can be delayed until
all CPUs have booted.
> But in your case, we a have SYSTEM cap, which only performs the check
> on the "running CPU" and not considering a system wide safe value for
> ICH_VTR_EL2_TDS. In order to do this, we need to switch to something
> like the GICV5_LEGACY cap, which chooses to perform the check on all
> booting CPUs (as we don't keep the system wide safe value for the
> bit it is looking for).
>
> e.g., your cap may be incorrectly SET, when the BOOT cpu (which
> mostly runs the SYSTEM scope), but another CPU didn't have the
> ICH_VTR_EL2_TDS.
Yup, that's the conclusion I also came to.
> Does that help ?
It does. I came up with the fix below, which I'll spin as a patch shortly.
Thanks,
M.
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index ff2b05f7226a9..c840a93b9ef95 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -2326,14 +2326,14 @@ static bool can_trap_icv_dir_el1(const struct arm64_cpu_capabilities *entry,
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) &&
+ if (!this_cpu_has_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))
+ if (this_cpu_has_cap(ARM64_HAS_GICV5_LEGACY))
return true;
if (is_kernel_in_hyp_mode())
@@ -2864,7 +2864,7 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
*/
.desc = "ICV_DIR_EL1 trapping",
.capability = ARM64_HAS_ICH_HCR_EL2_TDIR,
- .type = ARM64_CPUCAP_SYSTEM_FEATURE,
+ .type = ARM64_CPUCAP_EARLY_LOCAL_CPU_FEATURE,
.matches = can_trap_icv_dir_el1,
},
#ifdef CONFIG_ARM64_E0PD
--
Without deviation from the norm, progress is not possible.
More information about the linux-arm-kernel
mailing list