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

[Suzuki K Poulose]

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.

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.

Does that help ?

Suzuki

> 
> Thanks,
> 
> 	M.
>