KVM: arm64: vgic: Check the interrupt is still ours before migrating it

vgic_prune_ap_list() drops both ap_list_lock and irq_lock while migrating an interrupt to another vCPU. After reacquiring the locks it only checks that the affinity is unchanged (target_vcpu == vgic_target_oracle(irq)) before moving the interrupt, which assumes that an interrupt whose affinity is preserved is still queued on this vCPU's ap_list.

That assumption no longer holds if the interrupt is taken off the ap_list while the locks are dropped. vgic_flush_pending_lpis() removes the interrupt from the list and sets irq->vcpu to NULL, but leaves enabled/pending/target_vcpu untouched. As the interrupt is still enabled and pending, vgic_target_oracle() returns the same target_vcpu, so the affinity check passes and list_del() is run a second time on an entry that has already been removed.

Also check that the interrupt is still assigned to this vCPU (irq->vcpu == vcpu) before moving it.

Fixes 0919e84c0fc1 ("KVM: arm/arm64: vgic-new: Add IRQ sync/flush framework")

Signed-off-by Hyunwoo Kim <[email protected]>

---
 arch/arm64/kvm/vgic/vgic.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

arch/arm64/kvm/vgic/vgic.c +6 -5

index 1e9fe8764584..18b280de9a29 100644
@@ -818,15 +818,16 @@ static void vgic_prune_ap_list(struct kvm_vcpu *vcpu)
 		raw_spin_lock(&irq->irq_lock);
 
 		/*
-		 * If the affinity has been preserved, move the
-		 * interrupt around. Otherwise, it means things have
-		 * changed while the interrupt was unlocked, and we
-		 * need to replay this.
+		 * If the interrupt is still ours and its affinity has
+		 * been preserved, move it around. Otherwise, it means
+		 * things have changed while the interrupt was unlocked
+		 * (it may even have been taken off the list with its
+		 * affinity left untouched), and we need to replay this.
 		 *
 		 * In all cases, we cannot trust the list not to have
 		 * changed, so we restart from the beginning.
 		 */
-		if (target_vcpu == vgic_target_oracle(irq)) {
+		if (irq->vcpu == vcpu && target_vcpu == vgic_target_oracle(irq)) {
 			struct vgic_cpu *new_cpu = &target_vcpu->arch.vgic_cpu;
 
 			list_del(&irq->ap_list);

Signature · 1 line

2.43.0

Quoted 14 lines · Hyunwoo Kim

vgic_prune_ap_list() drops both ap_list_lock and irq_lock while migrating an interrupt to another vCPU. After reacquiring the locks it only checks that the affinity is unchanged (target_vcpu == vgic_target_oracle(irq)) before moving the interrupt, which assumes that an interrupt whose affinity is preserved is still queued on this vCPU's ap_list.

That assumption no longer holds if the interrupt is taken off the ap_list while the locks are dropped. vgic_flush_pending_lpis() removes the interrupt from the list and sets irq->vcpu to NULL, but leaves enabled/pending/target_vcpu untouched. As the interrupt is still enabled and pending, vgic_target_oracle() returns the same target_vcpu, so the affinity check passes and list_del() is run a second time on an entry that has already been removed.

Quoted 5 lines

Also check that the interrupt is still assigned to this vCPU (irq->vcpu == vcpu) before moving it.

Fixes 0919e84c0fc1 ("KVM: arm/arm64: vgic-new: Add IRQ sync/flush framework")

Signed-off-by Hyunwoo Kim <[email protected]>

Looking at this and the other VGIC patch you sent (which should've been a combined series), are you trying to deal with a vCPU writing to another vCPU's redistributor? I.e. vCPU B setting GICR_CTLR.EnableLPIs=0 behind the back of vCPU A?

That is extremely relevant information as the off-the-cuff reaction is that no race exists. But since the GIC architecture is awesome and allows for this sort of insanity, it obviously does....

Anyway, for LPIs resident on a particular RD, there's zero expectation that the pending state is preserved when EnableLPIs=0. So I'd rather vgic_flush_pending_lpis() just invalidate the pending state.

Beyond that, I see two other fixes for lifetime issues around the vgic_irq in the middle of migration. I'd like to see explicit RCU protection around the release && reacquire of the ap_list_lock rather than depending on the precondition that IRQs are disabled.

Quoted 27 lines · Oliver Upton

Quoted 14 lines · Hyunwoo Kim

vgic_prune_ap_list() drops both ap_list_lock and irq_lock while migrating an interrupt to another vCPU. After reacquiring the locks it only checks that the affinity is unchanged (target_vcpu == vgic_target_oracle(irq)) before moving the interrupt, which assumes that an interrupt whose affinity is preserved is still queued on this vCPU's ap_list.

That assumption no longer holds if the interrupt is taken off the ap_list while the locks are dropped. vgic_flush_pending_lpis() removes the interrupt from the list and sets irq->vcpu to NULL, but leaves enabled/pending/target_vcpu untouched. As the interrupt is still enabled and pending, vgic_target_oracle() returns the same target_vcpu, so the affinity check passes and list_del() is run a second time on an entry that has already been removed.

Quoted 5 lines

Also check that the interrupt is still assigned to this vCPU (irq->vcpu == vcpu) before moving it.

Fixes 0919e84c0fc1 ("KVM: arm/arm64: vgic-new: Add IRQ sync/flush framework")

Signed-off-by Hyunwoo Kim <[email protected]>

Looking at this and the other VGIC patch you sent (which should've been a combined series), are you trying to deal with a vCPU writing to another vCPU's redistributor? I.e. vCPU B setting GICR_CTLR.EnableLPIs=0 behind the back of vCPU A?

Quoted 7 lines

That is extremely relevant information as the off-the-cuff reaction is that no race exists. But since the GIC architecture is awesome and allows for this sort of insanity, it obviously does....

Anyway, for LPIs resident on a particular RD, there's zero expectation that the pending state is preserved when EnableLPIs=0. So I'd rather vgic_flush_pending_lpis() just invalidate the pending state.

Just clearing the pending state introduces a potential problem as we now have an interrupt that is neither active nor pending on the AP list. It is not impossible to solve (we now have similar behaviours with SPI deactivation from another vcpu), but that requires posting a KVM_REQ_VGIC_PROCESS_UPDATE to the target vcpu.

Quoted 4 lines

Beyond that, I see two other fixes for lifetime issues around the vgic_irq in the middle of migration. I'd like to see explicit RCU protection around the release && reacquire of the ap_list_lock rather than depending on the precondition that IRQs are disabled.

I'm not sure I follow. Are you suggesting turning the AP list into an RCU protected list?

Quoted 37 lines · Marc Zyngier

Quoted 27 lines · Oliver Upton

Quoted 14 lines · Hyunwoo Kim

vgic_prune_ap_list() drops both ap_list_lock and irq_lock while migrating an interrupt to another vCPU. After reacquiring the locks it only checks that the affinity is unchanged (target_vcpu == vgic_target_oracle(irq)) before moving the interrupt, which assumes that an interrupt whose affinity is preserved is still queued on this vCPU's ap_list.

That assumption no longer holds if the interrupt is taken off the ap_list while the locks are dropped. vgic_flush_pending_lpis() removes the interrupt from the list and sets irq->vcpu to NULL, but leaves enabled/pending/target_vcpu untouched. As the interrupt is still enabled and pending, vgic_target_oracle() returns the same target_vcpu, so the affinity check passes and list_del() is run a second time on an entry that has already been removed.

Quoted 5 lines

Also check that the interrupt is still assigned to this vCPU (irq->vcpu == vcpu) before moving it.

Fixes 0919e84c0fc1 ("KVM: arm/arm64: vgic-new: Add IRQ sync/flush framework")

Signed-off-by Hyunwoo Kim <[email protected]>

Looking at this and the other VGIC patch you sent (which should've been a combined series), are you trying to deal with a vCPU writing to another vCPU's redistributor? I.e. vCPU B setting GICR_CTLR.EnableLPIs=0 behind the back of vCPU A?

Quoted 7 lines

That is extremely relevant information as the off-the-cuff reaction is that no race exists. But since the GIC architecture is awesome and allows for this sort of insanity, it obviously does....

Anyway, for LPIs resident on a particular RD, there's zero expectation that the pending state is preserved when EnableLPIs=0. So I'd rather vgic_flush_pending_lpis() just invalidate the pending state.

Quoted 5 lines

Just clearing the pending state introduces a potential problem as we now have an interrupt that is neither active nor pending on the AP list. It is not impossible to solve (we now have similar behaviours with SPI deactivation from another vcpu), but that requires posting a KVM_REQ_VGIC_PROCESS_UPDATE to the target vcpu.

Right, I was suggesting that in addition to deleting the LPI from the AP list we actually invalidate the pending state so that someone sitting on a pointer to a to-be-freed LPI sees vgic_target_oracle() returning NULL

Quoted 7 lines

Quoted 4 lines

Beyond that, I see two other fixes for lifetime issues around the vgic_irq in the middle of migration. I'd like to see explicit RCU protection around the release && reacquire of the ap_list_lock rather than depending on the precondition that IRQs are disabled.

I'm not sure I follow. Are you suggesting turning the AP list into an RCU protected list?

No, sorry, I should expand a little.

We store a reference on the vgic_irq struct in the AP list, which is stable so long as the ap_list_lock is held. It should be possible for the refcount to drop to 0 between releasing the ap_list_lock and reacquiring it.

So either vgic_prune_ap_list() takes an additional reference on the vgic_irq before dropping the ap_list_lock or rely on RCU to protect vgic_irq structs observed with a non-zero refcount.

Quoted 59 lines · Oliver Upton

Quoted 37 lines · Marc Zyngier

Quoted 27 lines · Oliver Upton

Quoted 14 lines · Hyunwoo Kim

vgic_prune_ap_list() drops both ap_list_lock and irq_lock while migrating an interrupt to another vCPU. After reacquiring the locks it only checks that the affinity is unchanged (target_vcpu == vgic_target_oracle(irq)) before moving the interrupt, which assumes that an interrupt whose affinity is preserved is still queued on this vCPU's ap_list.

That assumption no longer holds if the interrupt is taken off the ap_list while the locks are dropped. vgic_flush_pending_lpis() removes the interrupt from the list and sets irq->vcpu to NULL, but leaves enabled/pending/target_vcpu untouched. As the interrupt is still enabled and pending, vgic_target_oracle() returns the same target_vcpu, so the affinity check passes and list_del() is run a second time on an entry that has already been removed.

Quoted 5 lines

Also check that the interrupt is still assigned to this vCPU (irq->vcpu == vcpu) before moving it.

Fixes 0919e84c0fc1 ("KVM: arm/arm64: vgic-new: Add IRQ sync/flush framework")

Signed-off-by Hyunwoo Kim <[email protected]>

Looking at this and the other VGIC patch you sent (which should've been a combined series), are you trying to deal with a vCPU writing to another vCPU's redistributor? I.e. vCPU B setting GICR_CTLR.EnableLPIs=0 behind the back of vCPU A?

Quoted 7 lines

That is extremely relevant information as the off-the-cuff reaction is that no race exists. But since the GIC architecture is awesome and allows for this sort of insanity, it obviously does....

Anyway, for LPIs resident on a particular RD, there's zero expectation that the pending state is preserved when EnableLPIs=0. So I'd rather vgic_flush_pending_lpis() just invalidate the pending state.

Quoted 5 lines

Just clearing the pending state introduces a potential problem as we now have an interrupt that is neither active nor pending on the AP list. It is not impossible to solve (we now have similar behaviours with SPI deactivation from another vcpu), but that requires posting a KVM_REQ_VGIC_PROCESS_UPDATE to the target vcpu.

Right, I was suggesting that in addition to deleting the LPI from the AP list we actually invalidate the pending state so that someone sitting on a pointer to a to-be-freed LPI sees vgic_target_oracle() returning NULL

Quoted 7 lines

Quoted 4 lines

Beyond that, I see two other fixes for lifetime issues around the vgic_irq in the middle of migration. I'd like to see explicit RCU protection around the release && reacquire of the ap_list_lock rather than depending on the precondition that IRQs are disabled.

I'm not sure I follow. Are you suggesting turning the AP list into an RCU protected list?

No, sorry, I should expand a little.

Quoted 8 lines

We store a reference on the vgic_irq struct in the AP list, which is stable so long as the ap_list_lock is held. It should be possible for the refcount to drop to 0 between releasing the ap_list_lock and reacquiring it.

So either vgic_prune_ap_list() takes an additional reference on the vgic_irq before dropping the ap_list_lock or rely on RCU to protect vgic_irq structs observed with a non-zero refcount.

What are your thoughts on this approach?

Best regards,
Hyunwoo Kim

---

arch/arm64/kvm/vgic/vgic-init.c +1 -1

index 933983bb2005..7fb871c3ccd8 100644
@@ -523,7 +523,7 @@ static void __kvm_vgic_vcpu_destroy(struct kvm_vcpu *vcpu)
 	 * Retire all pending LPIs on this vcpu anyway as we're
 	 * going to destroy it.
 	 */
-	vgic_flush_pending_lpis(vcpu);
+	vgic_flush_pending_lpis(vcpu, true);
 
 	INIT_LIST_HEAD(&vgic_cpu->ap_list_head);
 	kfree(vgic_cpu->private_irqs);

arch/arm64/kvm/vgic/vgic-mmio-v3.c +1 -1

index 5913a20d8301..f85d63f17af0 100644
@@ -303,7 +303,7 @@ static void vgic_mmio_write_v3r_ctlr(struct kvm_vcpu *vcpu,
 		if (ctlr != GICR_CTLR_ENABLE_LPIS)
 			return;
 
-		vgic_flush_pending_lpis(vcpu);
+		vgic_flush_pending_lpis(vcpu, false);
 		vgic_its_invalidate_all_caches(vcpu->kvm);
 		atomic_set_release(&vgic_cpu->ctlr, 0);
 	} else {

arch/arm64/kvm/vgic/vgic.c +1 -1

index 1e9fe8764584..09629a38fc0a 100644
@@ -192,7 +192,7 @@ static void vgic_release_deleted_lpis(struct kvm *kvm)
 	xa_unlock_irqrestore(&dist->lpi_xa, flags);
 }
 
-void vgic_flush_pending_lpis(struct kvm_vcpu *vcpu)
+void vgic_flush_pending_lpis(struct kvm_vcpu *vcpu, bool destroy)
 {
 	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
 	struct vgic_irq *irq, *tmp;

arch/arm64/kvm/vgic/vgic.c +5 -0

 			list_del(&irq->ap_list);
 			irq->vcpu = NULL;
 			raw_spin_unlock(&irq->irq_lock);

@@ -797,6 +804,9 @@ static void vgic_prune_ap_list(struct kvm_vcpu *vcpu)
 
 		/* This interrupt looks like it has to be migrated. */
 
+		/* Keep the interrupt alive while the locks are dropped. */
+		vgic_get_irq_ref(irq);
+
 		raw_spin_unlock(&irq->irq_lock);
 		raw_spin_unlock(&vgic_cpu->ap_list_lock);
 

@@ -839,6 +849,8 @@ static void vgic_prune_ap_list(struct kvm_vcpu *vcpu)
 		raw_spin_unlock(&vcpuB->arch.vgic_cpu.ap_list_lock);
 		raw_spin_unlock(&vcpuA->arch.vgic_cpu.ap_list_lock);
 
+		deleted_lpis |= vgic_put_irq_norelease(vcpu->kvm, irq);
+
 		if (target_vcpu_needs_kick) {
 			kvm_make_request(KVM_REQ_IRQ_PENDING, target_vcpu);
 			kvm_vcpu_kick(target_vcpu);

arch/arm64/kvm/vgic/vgic.h +0 -0

index 9d941241c8a2..c1ac24ede899 100644

Quoted 118 lines · Hyunwoo Kim

Quoted 59 lines · Oliver Upton

Quoted 37 lines · Marc Zyngier

Quoted 27 lines · Oliver Upton

Quoted 14 lines · Hyunwoo Kim

vgic_prune_ap_list() drops both ap_list_lock and irq_lock while migrating an interrupt to another vCPU. After reacquiring the locks it only checks that the affinity is unchanged (target_vcpu == vgic_target_oracle(irq)) before moving the interrupt, which assumes that an interrupt whose affinity is preserved is still queued on this vCPU's ap_list.

That assumption no longer holds if the interrupt is taken off the ap_list while the locks are dropped. vgic_flush_pending_lpis() removes the interrupt from the list and sets irq->vcpu to NULL, but leaves enabled/pending/target_vcpu untouched. As the interrupt is still enabled and pending, vgic_target_oracle() returns the same target_vcpu, so the affinity check passes and list_del() is run a second time on an entry that has already been removed.

Quoted 5 lines

Also check that the interrupt is still assigned to this vCPU (irq->vcpu == vcpu) before moving it.

Fixes 0919e84c0fc1 ("KVM: arm/arm64: vgic-new: Add IRQ sync/flush framework")

Signed-off-by Hyunwoo Kim <[email protected]>

Looking at this and the other VGIC patch you sent (which should've been a combined series), are you trying to deal with a vCPU writing to another vCPU's redistributor? I.e. vCPU B setting GICR_CTLR.EnableLPIs=0 behind the back of vCPU A?

Quoted 7 lines

That is extremely relevant information as the off-the-cuff reaction is that no race exists. But since the GIC architecture is awesome and allows for this sort of insanity, it obviously does....

Anyway, for LPIs resident on a particular RD, there's zero expectation that the pending state is preserved when EnableLPIs=0. So I'd rather vgic_flush_pending_lpis() just invalidate the pending state.

Quoted 5 lines

Just clearing the pending state introduces a potential problem as we now have an interrupt that is neither active nor pending on the AP list. It is not impossible to solve (we now have similar behaviours with SPI deactivation from another vcpu), but that requires posting a KVM_REQ_VGIC_PROCESS_UPDATE to the target vcpu.

Right, I was suggesting that in addition to deleting the LPI from the AP list we actually invalidate the pending state so that someone sitting on a pointer to a to-be-freed LPI sees vgic_target_oracle() returning NULL

Quoted 7 lines

Quoted 4 lines

Beyond that, I see two other fixes for lifetime issues around the vgic_irq in the middle of migration. I'd like to see explicit RCU protection around the release && reacquire of the ap_list_lock rather than depending on the precondition that IRQs are disabled.

I'm not sure I follow. Are you suggesting turning the AP list into an RCU protected list?

No, sorry, I should expand a little.

Quoted 8 lines

We store a reference on the vgic_irq struct in the AP list, which is stable so long as the ap_list_lock is held. It should be possible for the refcount to drop to 0 between releasing the ap_list_lock and reacquiring it.

So either vgic_prune_ap_list() takes an additional reference on the vgic_irq before dropping the ap_list_lock or rely on RCU to protect vgic_irq structs observed with a non-zero refcount.

What are your thoughts on this approach?

Best regards,
Hyunwoo Kim

---

arch/arm64/kvm/vgic/vgic-init.c +1 -1

index 933983bb2005..7fb871c3ccd8 100644
@@ -523,7 +523,7 @@ static void __kvm_vgic_vcpu_destroy(struct kvm_vcpu *vcpu)
 	 * Retire all pending LPIs on this vcpu anyway as we're
 	 * going to destroy it.
 	 */
-	vgic_flush_pending_lpis(vcpu);
+	vgic_flush_pending_lpis(vcpu, true);
 
 	INIT_LIST_HEAD(&vgic_cpu->ap_list_head);
 	kfree(vgic_cpu->private_irqs);

arch/arm64/kvm/vgic/vgic-mmio-v3.c +1 -1

index 5913a20d8301..f85d63f17af0 100644
@@ -303,7 +303,7 @@ static void vgic_mmio_write_v3r_ctlr(struct kvm_vcpu *vcpu,
 		if (ctlr != GICR_CTLR_ENABLE_LPIS)
 			return;
 
-		vgic_flush_pending_lpis(vcpu);
+		vgic_flush_pending_lpis(vcpu, false);
 		vgic_its_invalidate_all_caches(vcpu->kvm);
 		atomic_set_release(&vgic_cpu->ctlr, 0);
 	} else {

arch/arm64/kvm/vgic/vgic.c +1 -1

index 1e9fe8764584..09629a38fc0a 100644
@@ -192,7 +192,7 @@ static void vgic_release_deleted_lpis(struct kvm *kvm)
 	xa_unlock_irqrestore(&dist->lpi_xa, flags);
 }
 
-void vgic_flush_pending_lpis(struct kvm_vcpu *vcpu)
+void vgic_flush_pending_lpis(struct kvm_vcpu *vcpu, bool destroy)
 {
 	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
 	struct vgic_irq *irq, *tmp;

arch/arm64/kvm/vgic/vgic.c void vgic_flush_pending_lpis(struct kvm_vcpu *vcpu) · line 211

Quoted 8 lines

arch/arm64/kvm/vgic/vgic.c +5 -0

@@ -204,6 +204,13 @@ void vgic_flush_pending_lpis(struct kvm_vcpu *vcpu)
 	list_for_each_entry_safe(irq, tmp, &vgic_cpu->ap_list_head, ap_list) {
 		if (irq_is_lpi(vcpu->kvm, irq->intid)) {
 			raw_spin_lock(&irq->irq_lock);
+			/* Leave interrupts pending a migration for prune. */
+			if (!destroy && irq->vcpu != vgic_target_oracle(irq)) {
+				raw_spin_unlock(&irq->irq_lock);
+				continue;
+			}

It's rather unclear to me what the semantics of this are.

If vcpu-a decides to nuke the LPIs of vcpu-b and the LPI had in the meantime been migrated to vcpu-c, but obviously not observed by vcpu-c yet as the LPI is still on vcpu-b's AP-list, then I don't see the point in keeping this state.

Am I missing something obvious?

arch/arm64/kvm/vgic/vgic.c void vgic_flush_pending_lpis(struct kvm_vcpu *vcpu) · line 213

Quoted 3 lines

arch/arm64/kvm/vgic/vgic.c +2 -0

+			/* Pending state is not preserved across EnableLPIs=0. */
+			irq->pending_latch = false;

That part I agree with.

Quoted 28 lines

arch/arm64/kvm/vgic/vgic.c +5 -0

 			list_del(&irq->ap_list);
 			irq->vcpu = NULL;
 			raw_spin_unlock(&irq->irq_lock);

@@ -797,6 +804,9 @@ static void vgic_prune_ap_list(struct kvm_vcpu *vcpu)
 
 		/* This interrupt looks like it has to be migrated. */
 
+		/* Keep the interrupt alive while the locks are dropped. */
+		vgic_get_irq_ref(irq);
+
 		raw_spin_unlock(&irq->irq_lock);
 		raw_spin_unlock(&vgic_cpu->ap_list_lock);
 

@@ -839,6 +849,8 @@ static void vgic_prune_ap_list(struct kvm_vcpu *vcpu)
 		raw_spin_unlock(&vcpuB->arch.vgic_cpu.ap_list_lock);
 		raw_spin_unlock(&vcpuA->arch.vgic_cpu.ap_list_lock);
 
+		deleted_lpis |= vgic_put_irq_norelease(vcpu->kvm, irq);
+
 		if (target_vcpu_needs_kick) {
 			kvm_make_request(KVM_REQ_IRQ_PENDING, target_vcpu);
 			kvm_vcpu_kick(target_vcpu);

arch/arm64/kvm/vgic/vgic.h +0 -0

index 9d941241c8a2..c1ac24ede899 100644

arch/arm64/kvm/vgic/vgic.h void vgic_v3_put(struct kvm_vcpu *vcpu); · line 347

Quoted 8 lines

arch/arm64/kvm/vgic/vgic.h +1 -1

@@ -341,7 +341,7 @@ void vgic_v3_put(struct kvm_vcpu *vcpu);
 bool vgic_has_its(struct kvm *kvm);
 int kvm_vgic_register_its_device(void);
 void vgic_enable_lpis(struct kvm_vcpu *vcpu);
-void vgic_flush_pending_lpis(struct kvm_vcpu *vcpu);
+void vgic_flush_pending_lpis(struct kvm_vcpu *vcpu, bool destroy);
 int vgic_its_inject_msi(struct kvm *kvm, struct kvm_msi *msi);
 int vgic_v3_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr);
 int vgic_v3_dist_uaccess(struct kvm_vcpu *vcpu, bool is_write,

I reckon this would work just as well with just the pending state being removed in vgic_flush_pending_lpis(), and the reference holding hack in gvgic_prune_ap_list().

Thanks,

	M.

Signature · 1 line

Without deviation from the norm, progress is not possible.

Quoted 8 lines · Marc Zyngier

It's rather unclear to me what the semantics of this are.

If vcpu-a decides to nuke the LPIs of vcpu-b and the LPI had in the meantime been migrated to vcpu-c, but obviously not observed by vcpu-c yet as the LPI is still on vcpu-b's AP-list, then I don't see the point in keeping this state.

Am I missing something obvious?

I looked a bit more into Oliver's review, the one suggesting that pending be cleared only for resident LPIs while the ones being migrated are left in place.

What the leave preserves is the pending edge of a single LPI whose target is already vcpu-c but which is still on vcpu-b's ap_list. This edge is always lost when we just clear it, but for a device that fires again a later INT reaches vcpu-c through the oracle, so it is mostly harmless.

The
exception is a software LPI that never fires again(irq->hw == false):
that edge is then lost with no way to recover it, because its_sync_lpi_pending_table only re-syncs the LPIs whose target_vcpu matches, and the disable path does no pending writeback. I am not entirely sure about this part, though.

Since this does not look like the common case, if it does not need to be covered I will send v2 keeping only the pending clear and the ref hold in vgic_prune_ap_list(). What do you think?

Quoted 11 lines · Hyunwoo Kim

Quoted 8 lines · Marc Zyngier

It's rather unclear to me what the semantics of this are.

If vcpu-a decides to nuke the LPIs of vcpu-b and the LPI had in the meantime been migrated to vcpu-c, but obviously not observed by vcpu-c yet as the LPI is still on vcpu-b's AP-list, then I don't see the point in keeping this state.

Am I missing something obvious?

Quoted 8 lines

I looked a bit more into Oliver's review, the one suggesting that pending be cleared only for resident LPIs while the ones being migrated are left in place.

What the leave preserves is the pending edge of a single LPI whose target is already vcpu-c but which is still on vcpu-b's ap_list. This edge is always lost when we just clear it, but for a device that fires again a later INT reaches vcpu-c through the oracle, so it is mostly harmless.

Not completely harmless. When the guest writes EnableLPIs==0, it accepts the lost of any pending bit that could be stored. These won't be regenerated, unless the device signals a new event that maps to the same LPIs. But again, this is the guest's own decision, and I don't see a reason to prevent it from shooting itself in the foot.

Quoted 6 lines

The
exception is a software LPI that never fires again(irq->hw == false):
that edge is then lost with no way to recover it, because its_sync_lpi_pending_table only re-syncs the LPIs whose target_vcpu matches, and the disable path does no pending writeback. I am not entirely sure about this part, though.

I don't think this is a problem, as the architecture doesn't guarantee the state of the pending table after turning EnableLPIs off. There's even a note recommending to move the interrupts to another RD before doing that.

Quoted 4 lines

Since this does not look like the common case, if it does not need to be covered I will send v2 keeping only the pending clear and the ref hold in vgic_prune_ap_list(). What do you think?

So that it is entirely unambiguous, my suggestion is to have this:

arch/arm64/kvm/vgic/vgic.c +8 -1

index 5a4768d8cd4f3..70a161383e5a6 100644
@@ -203,6 +203,7 @@ void vgic_flush_pending_lpis(struct kvm_vcpu *vcpu)
 	list_for_each_entry_safe(irq, tmp, &vgic_cpu->ap_list_head, ap_list) {
 		if (irq_is_lpi(vcpu->kvm, irq->intid)) {
 			raw_spin_lock(&irq->irq_lock);
+			irq->pending_latch = false;
 			list_del(&irq->ap_list);
 			irq->vcpu = NULL;
 			raw_spin_unlock(&irq->irq_lock);

@@ -792,7 +793,11 @@ static void vgic_prune_ap_list(struct kvm_vcpu *vcpu)
 			continue;
 		}
 
-		/* This interrupt looks like it has to be migrated. */
+		/*
+		 * This interrupt looks like it has to be migrated,
+		 * make sure it is kept alive while locks are dropped.
+		 */
+		vgic_get_irq_ref(irq);
 
 		raw_spin_unlock(&irq->irq_lock);
 		raw_spin_unlock(&vgic_cpu->ap_list_lock);

@@ -836,6 +841,8 @@ static void vgic_prune_ap_list(struct kvm_vcpu *vcpu)
 		raw_spin_unlock(&vcpuB->arch.vgic_cpu.ap_list_lock);
 		raw_spin_unlock(&vcpuA->arch.vgic_cpu.ap_list_lock);
 
+		deleted_lpis |= vgic_put_irq_norelease(vcpu->kvm, irq);
+
 		if (target_vcpu_needs_kick) {
 			kvm_make_request(KVM_REQ_IRQ_PENDING, target_vcpu);
 			kvm_vcpu_kick(target_vcpu);

Could you please give it a go with whatever reproducer you have?

vgic_prune_ap_list() drops both ap_list_lock and irq_lock while migrating an interrupt to another vCPU. After reacquiring the locks it only checks that the affinity is unchanged (target_vcpu == vgic_target_oracle(irq)) before moving the interrupt, which assumes that an interrupt whose affinity is preserved is still queued on this vCPU's ap_list.

That assumption no longer holds if the interrupt is taken off the ap_list while the locks are dropped. vgic_flush_pending_lpis() removes the interrupt from the list and sets irq->vcpu to NULL, but leaves enabled/pending/target_vcpu untouched. As the interrupt is still enabled and pending, vgic_target_oracle() returns the same target_vcpu, so the affinity check passes and list_del() is run a second time on an entry that has already been removed.

[...]