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 14:14:25 +0000,
Suzuki K Poulose <suzuki.poulose@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@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.
> 
> 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.