Subject: Re: [PATCH] KVM: arm64: Skip interrupts in LRs during EOIcount replay

From: Marc Zyngier <[email-masked]>
Date: Sat, 07 Mar 2026 16:33:08 +0000

Hi Valentine,

Thanks for this.

On Sat, 07 Mar 2026 11:59:50 +0000,
Valentine Burley <[email-masked]> wrote:
> 
> Commit 05984ba67eb6 ("KVM: arm64: Invert ap_list sorting to push active
> interrupts out") allowed active interrupts to be evicted from LRs to
> make room for pending ones.
> 
> When an evicted interrupt is deactivated by the guest, the GIC
> increments EOIcount. KVM replays this by finding an active interrupt
> in the ap_list to deactivate. However, the replay logic may pick an
> interrupt that is currently residing in an LR, leading to a spurious
> deactivation and leaving the actually finished interrupt active.

Well, the interrupt is not in an LR anymore. It has been sync'd
("folded") back into the vgic_irq structure, and the lr copy is now
stale.

But the observation is key: if in EOImode==0, and if we don't trap
ICC_EOIRx_EL1 with ICH_HCR_EL2.TC, we can race against the maintenance
interrupt that signals a non-zero EOIcount, leaving the guest a chance
to Ack more interrupts. Since we don't have the interrupt number, we
pick the first active interrupt in the AP list, with prejudice. Boo.

I can't reproduce it locally, but in a crap integration, where the GIC
is clocked at a few dozen MHz, this is far more likely to happen. I
should dig that Lazor out of the bin and put it back in the test rig.

In retrospect, it is obvious. I just couldn't see it until then. Many
thanks for going the extra mile and pointing out the core issue.

[skip v2 stuff]

> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
> index 1d6dd1b545bd..00d9bc39bffb 100644
> --- a/arch/arm64/kvm/vgic/vgic-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-v3.c
> @@ -179,6 +179,25 @@ void vgic_v3_fold_lr_state(struct kvm_vcpu *vcpu)
>  			      irq->active))
>  				continue;
>  
> +			bool was_in_lr = false;
> +
> +			for (int i = 0; i < cpuif->used_lrs; i++) {
> +				u32 intid;
> +
> +				if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3)
> +					intid = cpuif->vgic_lr[i] & ICH_LR_VIRTUAL_ID_MASK;
> +				else
> +					intid = cpuif->vgic_lr[i] & GICH_LR_VIRTUALID;
> +
> +				if (intid == irq->intid) {
> +					was_in_lr = true;
> +					break;
> +				}
> +			}
> +
> +			if (was_in_lr)
> +				continue;
> +

I'm afraid we can't afford this sort of quadratic behaviour, and I
really don't want to go back rummaging in what was in the LRs -- this
data is notionally stale. But we can rely on some properties of the AP
list, of which the interrupts in the LRs are guaranteed to be a strict
prefix.

We just need to keep track of what point we have reached in the AP
list at flush time, and use that as the starting point for the
EOIcount search. It's a tiny bit of extra state, but I'd rather have
that than what you are suggesting here.

Could you please give the hack below a go on your setup? It seems to
work for me, but given that I never observed the issue the first
place...

Thanks again,

	M.

diff --git a/arch/arm64/kvm/vgic/vgic-v2.c b/arch/arm64/kvm/vgic/vgic-v2.c
index 585491fbda807..e5b49274a6683 100644
--- a/arch/arm64/kvm/vgic/vgic-v2.c
+++ b/arch/arm64/kvm/vgic/vgic-v2.c
@@ -115,7 +115,7 @@ void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu)
 	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
 	struct vgic_v2_cpu_if *cpuif = &vgic_cpu->vgic_v2;
 	u32 eoicount = FIELD_GET(GICH_HCR_EOICOUNT, cpuif->vgic_hcr);
-	struct vgic_irq *irq;
+	struct vgic_irq *irq = vgic_cpu->last_lr_irq;
 
 	DEBUG_SPINLOCK_BUG_ON(!irqs_disabled());
 
@@ -123,7 +123,7 @@ void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu)
 		vgic_v2_fold_lr(vcpu, cpuif->vgic_lr[lr]);
 
 	/* See the GICv3 equivalent for the EOIcount handling rationale */
-	list_for_each_entry(irq, &vgic_cpu->ap_list_head, ap_list) {
+	list_for_each_entry_continue(irq, &vgic_cpu->ap_list_head, ap_list) {
 		u32 lr;
 
 		if (!eoicount) {
diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
index 386ddf69a9c51..457fb4cd3fc63 100644
--- a/arch/arm64/kvm/vgic/vgic-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-v3.c
@@ -148,7 +148,7 @@ void vgic_v3_fold_lr_state(struct kvm_vcpu *vcpu)
 	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
 	struct vgic_v3_cpu_if *cpuif = &vgic_cpu->vgic_v3;
 	u32 eoicount = FIELD_GET(ICH_HCR_EL2_EOIcount, cpuif->vgic_hcr);
-	struct vgic_irq *irq;
+	struct vgic_irq *irq = vgic_cpu->last_lr_irq;
 
 	DEBUG_SPINLOCK_BUG_ON(!irqs_disabled());
 
@@ -158,12 +158,12 @@ void vgic_v3_fold_lr_state(struct kvm_vcpu *vcpu)
 	/*
 	 * EOIMode=0: use EOIcount to emulate deactivation. We are
 	 * guaranteed to deactivate in reverse order of the activation, so
-	 * just pick one active interrupt after the other in the ap_list,
-	 * and replay the deactivation as if the CPU was doing it. We also
-	 * rely on priority drop to have taken place, and the list to be
-	 * sorted by priority.
+	 * just pick one active interrupt after the other in the tail part
+	 * of the ap_list, past the LRs, and replay the deactivation as if
+	 * the CPU was doing it. We also rely on priority drop to have taken
+	 * place, and the list to be sorted by priority.
 	 */
-	list_for_each_entry(irq, &vgic_cpu->ap_list_head, ap_list) {
+	list_for_each_entry_continue(irq, &vgic_cpu->ap_list_head, ap_list) {
 		u64 lr;
 
 		/*
diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c
index 430aa98888fda..4a641a12d026b 100644
--- a/arch/arm64/kvm/vgic/vgic.c
+++ b/arch/arm64/kvm/vgic/vgic.c
@@ -814,6 +814,9 @@ static void vgic_prune_ap_list(struct kvm_vcpu *vcpu)
 
 static inline void vgic_fold_lr_state(struct kvm_vcpu *vcpu)
 {
+	if (!vcpu->arch.vgic_cpu.last_lr_irq)
+		return;
+
 	if (kvm_vgic_global_state.type == VGIC_V2)
 		vgic_v2_fold_lr_state(vcpu);
 	else
@@ -960,10 +963,13 @@ static void vgic_flush_lr_state(struct kvm_vcpu *vcpu)
 	if (irqs_outside_lrs(&als))
 		vgic_sort_ap_list(vcpu);
 
+	vgic_cpu->last_lr_irq = NULL;
+
 	list_for_each_entry(irq, &vgic_cpu->ap_list_head, ap_list) {
 		scoped_guard(raw_spinlock,  &irq->irq_lock) {
 			if (likely(vgic_target_oracle(irq) == vcpu)) {
 				vgic_populate_lr(vcpu, irq, count++);
+				vgic_cpu->last_lr_irq = irq;
 			}
 		}
 
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index f2eafc65bbf4c..61040a14cb388 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -359,6 +359,9 @@ struct vgic_cpu {
 	 */
 	struct list_head ap_list_head;
 
+	/* Last vgic_irq part of the AP list recorded in an LR */
+	struct vgic_irq *last_lr_irq;
+
 	/*
 	 * Members below are used with GICv3 emulation only and represent
 	 * parts of the redistributor.

-- 
Jazz isn't dead. It just smells funny.