[PATCH 0/2] KVM: arm64: vgic: LPI fixes for redistributors with LPIs disabled

[PATCH 0/2] KVM: arm64: vgic: LPI fixes for redistributors with LPIs disabled

[Fuad Tabba]

Hi folks,

Two fixes on top of Marc's cross-CPU vcpu pausing series [1], for LPIs
whose redistributor has LPIs disabled. Without them, the guest can take
such an LPI with EnableLPIs clear, or lose one mapped before it enables
them.

The first is the vgic_target_oracle() change Oliver suggested on v2 of
that series [2]. It stops an LPI from being queued to a redistributor
with EnableLPIs clear once the disable path has retired the vCPU's AP
list. With that check, the path has to pause the vCPU before clearing
EnableLPIs, so that the retire still finds its LPIs.

With the second, an LPI mapped while its redistributor has LPIs
disabled keeps its pending bit when they're enabled. The second relies
on the first to keep such an LPI off the AP list until the enable.

Reproducers for the INVALL case in the first and the map in the second
fail in every run without the patch and pass with it.

Based on kvm-arm64/cross-vcpu-sync as merged in kvmarm/fixes, which the
first builds on (e99ff76f4a02b; also applies to kvmarm/fixes
309e1d2215255).

Cheers,
/fuad

[1] https://lore.kernel.org/all/20261015100539.1011760-1-maz@kernel.org/
[2] https://lore.kernel.org/all/asf87i2RDh-WksL-@kernel.org/

Fuad Tabba (2):
  KVM: arm64: vgic: Don't forward LPIs to a redistributor with LPIs
    disabled
  KVM: arm64: vgic-v3: Don't drop a pending LPI mapped with LPIs
    disabled

 arch/arm64/kvm/vgic/vgic-mmio-v3.c | 31 ++++++++++++++++--------------
 arch/arm64/kvm/vgic/vgic-v3.c      |  3 ++-
 arch/arm64/kvm/vgic/vgic.c         | 12 ++++++++----
 3 files changed, 27 insertions(+), 19 deletions(-)


base-commit: e99ff76f4a02be8542e2c3ecc8843d7339a0200d
-- 
2.47.3

[PATCH 1/2] KVM: arm64: vgic: Don't forward LPIs to a redistributor with LPIs disabled

[Fuad Tabba]

Clearing GICR_CTLR.EnableLPIs retires the LPIs on the target vCPU's
ap_list, then invalidates the ITS translation caches. However, an MSI
that hits the translation cache can still queue an LPI to that vCPU
after the retire, and so can an INVALL that enables an LPI already
pending. vgic_target_oracle() only checks that the distributor is
enabled, so the LPI goes on the ap_list and the vCPU takes it with
EnableLPIs clear.

Make the oracle also return NULL for an LPI whose redistributor has LPIs
disabled. vgic_queue_irq_unlock() rechecks it under the ap_list lock,
which the retire takes after clearing EnableLPIs, so either the retire
finds the LPI or the queueing reads EnableLPIs clear. The disable path
pauses the vCPU after clearing EnableLPIs, and with this change the
prune on that vCPU's exit would take its pending LPIs off the ap_list
before the retire walks it. Pause the vCPU before clearing EnableLPIs,
so that the retire still clears their pending state.

Fixes: 0e4e82f154e38 ("KVM: arm64: vgic-its: Enable ITS emulation as a virtual MSI controller")
Suggested-by: Oliver Upton <oupton@kernel.org>
Link: https://lore.kernel.org/r/asf87i2RDh-WksL-@kernel.org
Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev>
---
 arch/arm64/kvm/vgic/vgic-mmio-v3.c | 31 ++++++++++++++++--------------
 arch/arm64/kvm/vgic/vgic.c         | 12 ++++++++----
 2 files changed, 25 insertions(+), 18 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
index f5bd29fbe3a90..9117cd85aa593 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
@@ -293,6 +293,18 @@ static void vgic_mmio_write_v3r_ctlr(struct kvm_vcpu *vcpu,
 		return;
 
 	if (!(val & GICR_CTLR_ENABLE_LPIS)) {
+		/*
+		 * Yes, disabling LPIs is painful, since it can be done from
+		 * a *remote* vcpu! So let's not take any chance, and make
+		 * sure that the target vCPU has written their LRs back to the
+		 * irq structures, and release any reference they would have.
+		 * Pause it before its LPIs read as disabled, so that the exit's
+		 * prune leaves them for vgic_flush_pending_lpis() to retire.
+		 *
+		 * If it hurts, don't do it.
+		 */
+		kvm_pause_vcpu(vcpu);
+
 		/*
 		 * Don't disable if RWP is set, as there already an
 		 * ongoing disable. Funky guest...
@@ -300,21 +312,12 @@ static void vgic_mmio_write_v3r_ctlr(struct kvm_vcpu *vcpu,
 		ctlr = atomic_cmpxchg_acquire(&vgic_cpu->ctlr,
 					      GICR_CTLR_ENABLE_LPIS,
 					      GICR_CTLR_RWP);
-		if (ctlr != GICR_CTLR_ENABLE_LPIS)
-			return;
+		if (ctlr == GICR_CTLR_ENABLE_LPIS) {
+			vgic_flush_pending_lpis(vcpu);
+			vgic_its_invalidate_all_caches(vcpu->kvm);
+			atomic_set_release(&vgic_cpu->ctlr, 0);
+		}
 
-		/*
-		 * Yes, disabling LPIs is painful, since it can be done from
-		 * a *remote* vcpu! So let's not take any chance, and make
-		 * sure that the target vCPU has written their LRs back to the
-		 * irq structures, and release any reference they would have.
-		 *
-		 * If it hurts, don't do it.
-		 */
-		kvm_pause_vcpu(vcpu);
-		vgic_flush_pending_lpis(vcpu);
-		vgic_its_invalidate_all_caches(vcpu->kvm);
-		atomic_set_release(&vgic_cpu->ctlr, 0);
 		kvm_resume_vcpu(vcpu);
 	} else {
 		ctlr = atomic_cmpxchg_acquire(&vgic_cpu->ctlr, 0,
diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c
index 7061b4ab206c9..e1a6a5aa61a8e 100644
--- a/arch/arm64/kvm/vgic/vgic.c
+++ b/arch/arm64/kvm/vgic/vgic.c
@@ -273,14 +273,18 @@ struct kvm_vcpu *vgic_target_oracle(struct vgic_irq *irq)
 	 * If the IRQ is not active but enabled and pending, we should direct
 	 * it to its configured target VCPU.
 	 * If the distributor is disabled, pending interrupts shouldn't be
-	 * forwarded.
+	 * forwarded, nor should LPIs whose redistributor has them disabled.
 	 */
 	if (irq->enabled && irq_is_pending(irq)) {
-		if (unlikely(irq->target_vcpu &&
-			     !irq->target_vcpu->kvm->arch.vgic.enabled))
+		struct kvm_vcpu *vcpu = irq->target_vcpu;
+
+		if (unlikely(vcpu && !vcpu->kvm->arch.vgic.enabled))
 			return NULL;
 
-		return irq->target_vcpu;
+		if (unlikely(vcpu && irq_is_lpi(vcpu->kvm, irq->intid) && !vgic_lpis_enabled(vcpu)))
+			return NULL;
+
+		return vcpu;
 	}
 
 	/* If neither active nor pending and enabled, then this IRQ should not
-- 
2.47.3

[PATCH 2/2] KVM: arm64: vgic-v3: Don't drop a pending LPI mapped with LPIs disabled

[Fuad Tabba]

When an LPI is mapped, or restored with the ITS tables,
vgic_v3_lpi_sync_pending_status() moves its bit in the target
redistributor's pending table into the LPI's pending latch. Enabling
LPIs on that redistributor resets the latch of every LPI that targets it
from the table (its_sync_lpi_pending_table()), which no longer has the
bit. So an LPI mapped while its redistributor has LPIs disabled loses
its pending state, and the guest never takes it.

While the redistributor has LPIs disabled, copy the bit into the latch
rather than move it, so that enabling LPIs reads it again from the
table. It's still copied into the latch, since saving the pending tables
would otherwise write the empty latch over the bit.

Fixes: 06bd5359549d7 ("KVM: arm64: vgic-its: Read config and pending bit in add_lpi()")
Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev>
---
 arch/arm64/kvm/vgic/vgic-v3.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
index 346bacb3198f2..65b77fb35be37 100644
--- a/arch/arm64/kvm/vgic/vgic-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-v3.c
@@ -552,7 +552,8 @@ int vgic_v3_lpi_sync_pending_status(struct kvm *kvm, struct vgic_irq *irq)
 	irq->pending_latch = status;
 	vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
 
-	if (status) {
+	/* Enabling LPIs resets the latch from the table: leave the bit for it */
+	if (status && vgic_lpis_enabled(vcpu)) {
 		/* clear consumed data */
 		val &= ~(1 << bit_nr);
 		ret = vgic_write_guest_lock(kvm, ptr, &val, 1);
-- 
2.47.3