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

[Suzuki K Poulose <suzuki.poulose@arm.com>]

On 25/11/2025 15:01, Marc Zyngier wrote:
> 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,

Thanks, the below looks good to me.


Cheers
Suzuki

> 
> 	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
>