[PATCH 0/2] KVM: arm64: vgic-its: LPI pending-state fixes for DISCARD and CLEAR

[PATCH 0/2] KVM: arm64: vgic-its: LPI pending-state fixes for DISCARD and CLEAR

[Fuad Tabba]

Hi folks,

Two follow-ups to my DISCARD fix [1], for LPI pending state that KVM
keeps after the guest has removed it. Hopefully in time for the next
fixes pull.

The first closes a window in that fix in which the guest can still take
the LPI it discarded.

The second does for CLEAR what that fix did for DISCARD. Without it,
the guest can take an LPI that it has cleared, and a racing CLEAR can
trip the WARN_ON() in vgic_v3_compute_lr().

In my reproducer, the guest takes the LPI after the CLEAR in every run
without the second, and doesn't in any run with it.

Based on kvm-arm64/cross-vcpu-sync as merged in kvmarm/fixes, which both
patches build on (e99ff76f4a02b; also applies to kvmarm/fixes
79f3724ca8913).

Cheers,
/fuad

[1] https://lore.kernel.org/all/20261013081646.933312-3-fuad.tabba@linux.dev/

Fuad Tabba (2):
  KVM: arm64: vgic-its: Clear an unqueued LPI before dropping its lock
  KVM: arm64: vgic-its: Remove the pending state of a cleared LPI

 arch/arm64/kvm/vgic/vgic-its.c | 65 ++++++++++++++++++----------------
 1 file changed, 35 insertions(+), 30 deletions(-)


base-commit: e99ff76f4a02be8542e2c3ecc8843d7339a0200d
-- 
2.47.3

[PATCH 1/2] KVM: arm64: vgic-its: Clear an unqueued LPI before dropping its lock

[Fuad Tabba]

DISCARD reads which vCPU's AP list holds the LPI, drops the irq lock to
pause that vCPU, and takes the lock again to clear the pending state.
When no list holds the LPI there's nothing to pause, yet the lock is
still dropped. In between, vgic_queue_irq_unlock() can list the LPI (for
an MSI that had already made it pending, or a GICR_INVLPIR write
enabling it while latched) and its vCPU can load it into a list
register. The guest then takes an LPI it has discarded.

Clear an LPI that no list holds before dropping the lock.

Fixes: e99ff76f4a02b ("KVM: arm64: vgic-its: Remove the pending state of a discarded LPI")
Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev>
---
 arch/arm64/kvm/vgic/vgic-its.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
index 37059d1338406..42dc71495ad98 100644
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -842,22 +842,24 @@ static int vgic_its_cmd_handle_discard(struct kvm *kvm, struct vgic_its *its,
 		 * Only an LPI on a vCPU's AP list can be in its list registers,
 		 * which the exit path folds back, and that exit moves the LPI
 		 * to its target vCPU if the target has changed: pause both.
+		 * Clear an LPI on no AP list before dropping the lock, so that
+		 * nothing can put it in a list register first.
 		 */
 		scoped_guard(raw_spinlock_irqsave, &irq->irq_lock) {
 			vcpu = irq->vcpu;
 			target = irq->target_vcpu;
+			if (!vcpu)
+				irq->pending_latch = false;
 		}
 
 		if (vcpu) {
 			if (target != vcpu)
 				kvm_pause_vcpu(target);
 			kvm_pause_vcpu(vcpu);
-		}
 
-		scoped_guard(raw_spinlock_irqsave, &irq->irq_lock)
-			irq->pending_latch = false;
+			scoped_guard(raw_spinlock_irqsave, &irq->irq_lock)
+				irq->pending_latch = false;
 
-		if (vcpu) {
 			kvm_resume_vcpu(vcpu);
 			if (target != vcpu)
 				kvm_resume_vcpu(target);
-- 
2.47.3

[PATCH 2/2] KVM: arm64: vgic-its: Remove the pending state of a cleared LPI

[Fuad Tabba]

CLEAR removes the pending state of an LPI (IHI 0069H.b 5.3.3). Since
commit e99ff76f4a02b ("KVM: arm64: vgic-its: Remove the pending state of
a discarded LPI"), DISCARD does that under the irq lock, after pausing
the vCPUs whose list registers can hold the LPI. CLEAR does neither. An
LPI pending in a running vCPU's list register is folded back on exit, so
the guest takes it after the CLEAR. pending_latch also shares a word
with on_lr, so a racing CLEAR can restore on_lr after the exit path has
cleared it under the lock, and trip the WARN_ON() in
vgic_v3_compute_lr() when the LPI is next queued.

Move DISCARD's pause and clear into a helper and use it for CLEAR too,
after checking, as DISCARD does, that the event's collection is mapped.
CLEAR on an unmapped collection is a command error. Without the check,
the helper could be handed an LPI that has no target vCPU to pause.

Fixes: df9f58fbea9bc ("KVM: arm64: vgic-its: Implement ITS command queue command handlers")
Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev>
---
 arch/arm64/kvm/vgic/vgic-its.c | 67 ++++++++++++++++++----------------
 1 file changed, 35 insertions(+), 32 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
index 42dc71495ad98..f9e9b0a71eb5a 100644
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -817,6 +817,38 @@ static u64 its_cmd_mask_field(u64 *its_cmd, int word, int shift, int size)
 #define its_cmd_get_target_addr(cmd)	its_cmd_mask_field(cmd, 2, 16, 32)
 #define its_cmd_get_validbit(cmd)	its_cmd_mask_field(cmd, 2, 63,  1)
 
+/*
+ * Only an LPI on a vCPU's AP list can be in its list registers, which the
+ * exit path folds back, and that exit moves the LPI to its target vCPU if
+ * the target has changed: pause both. Clear an LPI on no AP list before
+ * dropping the lock, so that nothing can put it in a list register first.
+ * Must be called with the its_lock mutex held.
+ */
+static void vgic_its_clear_pending(struct vgic_irq *irq)
+{
+	struct kvm_vcpu *vcpu, *target;
+
+	scoped_guard(raw_spinlock_irqsave, &irq->irq_lock) {
+		vcpu = irq->vcpu;
+		target = irq->target_vcpu;
+		if (!vcpu)
+			irq->pending_latch = false;
+	}
+
+	if (vcpu) {
+		if (target != vcpu)
+			kvm_pause_vcpu(target);
+		kvm_pause_vcpu(vcpu);
+
+		scoped_guard(raw_spinlock_irqsave, &irq->irq_lock)
+			irq->pending_latch = false;
+
+		kvm_resume_vcpu(vcpu);
+		if (target != vcpu)
+			kvm_resume_vcpu(target);
+	}
+}
+
 /*
  * The DISCARD command frees an Interrupt Translation Table Entry (ITTE).
  * Must be called with the its_lock mutex held.
@@ -833,38 +865,9 @@ static int vgic_its_cmd_handle_discard(struct kvm *kvm, struct vgic_its *its,
 		struct its_device *device = find_its_device(its, device_id);
 		int ite_esz = vgic_its_get_abi(its)->ite_esz;
 		gpa_t gpa = device->itt_addr + ite->event_id * ite_esz;
-		struct vgic_irq *irq = ite->irq;
-		struct kvm_vcpu *vcpu, *target;
 
 		vgic_its_invalidate_cache(its);
-
-		/*
-		 * Only an LPI on a vCPU's AP list can be in its list registers,
-		 * which the exit path folds back, and that exit moves the LPI
-		 * to its target vCPU if the target has changed: pause both.
-		 * Clear an LPI on no AP list before dropping the lock, so that
-		 * nothing can put it in a list register first.
-		 */
-		scoped_guard(raw_spinlock_irqsave, &irq->irq_lock) {
-			vcpu = irq->vcpu;
-			target = irq->target_vcpu;
-			if (!vcpu)
-				irq->pending_latch = false;
-		}
-
-		if (vcpu) {
-			if (target != vcpu)
-				kvm_pause_vcpu(target);
-			kvm_pause_vcpu(vcpu);
-
-			scoped_guard(raw_spinlock_irqsave, &irq->irq_lock)
-				irq->pending_latch = false;
-
-			kvm_resume_vcpu(vcpu);
-			if (target != vcpu)
-				kvm_resume_vcpu(target);
-		}
-
+		vgic_its_clear_pending(ite->irq);
 		its_free_ite(kvm, ite);
 
 		return vgic_its_write_entry_lock(its, gpa, 0ULL, ite);
@@ -1309,10 +1312,10 @@ static int vgic_its_cmd_handle_clear(struct kvm *kvm, struct vgic_its *its,
 
 
 	ite = find_ite(its, device_id, event_id);
-	if (!ite)
+	if (!ite || !its_is_collection_mapped(ite->collection))
 		return E_ITS_CLEAR_UNMAPPED_INTERRUPT;
 
-	ite->irq->pending_latch = false;
+	vgic_its_clear_pending(ite->irq);
 
 	if (ite->irq->hw)
 		return irq_set_irqchip_state(ite->irq->host_irq,
-- 
2.47.3