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

Suzuki K Poulose
2025-11-25T15:03:16Z

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
>