[PATCH 0/3] KVM: arm64: vgic: Drop stale LPI pending state

[PATCH 0/3] KVM: arm64: vgic: Drop stale LPI pending state

[Fuad Tabba]

Hi folks,

The GIC just keeps on giving. Three follow-ups to my LPI fixes for
redistributors with LPIs disabled [1], to fix pre-existing bugs Sashiko
found.

The first drops the pending state of every LPI targeting a
redistributor when the guest disables its LPIs, not just of those on
the AP list, so a re-enable with GICR_PENDBASER.PTZ set doesn't bring
any of them back. Linux never sets PTZ.

The second stops the pending-table save from writing to the table of a
redistributor while its LPIs are disabled.

The third stops KVM reading the pending table when mapping an LPI, so
that mapping a discarded LPI's INTID again doesn't make it pending a
second time. A Linux guest can hit that after its VMM pauses and
resumes it. It also drops the copy that patch 2 of [1] takes, which is
why it comes after the second. An LPI mapped after its redistributor
enabled LPIs no longer takes a bit the guest wrote into the table
before the enable. Keeping that needs a per-redistributor record of
the pending state of unmapped INTIDs. I can add it on top if you think
it's worth it.

A pending LPI that MOVI moves onto a redistributor with LPIs disabled,
or that a device signals there through a translation cached before the
move, can still be taken once after a PTZ-set enable. The architecture
lets KVM drop it, but that's pending state moved or raised onto a
redistributor with LPIs disabled, not state the guest gave up. I've
left it alone, but can fix it on top if you think it's worth it.

Reproducers for all three fail on kvmarm/fixes and pass with the
series. I'll fold them into vgic_lpi_stress separately.

Based on kvmarm/fixes (181a701fc14d3), which has [1].

Cheers,
/fuad

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

Fuad Tabba (3):
  KVM: arm64: vgic: Drop the pending state of all LPIs when disabling
    LPIs
  KVM: arm64: vgic-v3: Don't save pending tables with LPIs disabled
  KVM: arm64: vgic-its: Don't read the pending table when mapping an LPI

 .../virt/kvm/devices/arm-vgic-v3.rst          |  1 +
 arch/arm64/kvm/vgic/vgic-its.c                | 33 ++++++++----
 arch/arm64/kvm/vgic/vgic-mmio-v3.c            | 13 ++---
 arch/arm64/kvm/vgic/vgic-v3.c                 | 50 ++-----------------
 arch/arm64/kvm/vgic/vgic.c                    | 16 +++++-
 arch/arm64/kvm/vgic/vgic.h                    |  1 -
 6 files changed, 49 insertions(+), 65 deletions(-)


base-commit: 181a701fc14d3d468c603b5ecfb9f12115c5fb72
-- 
2.47.3

[PATCH 1/3] KVM: arm64: vgic: Drop the pending state of all LPIs when disabling LPIs

[Fuad Tabba]

When the guest clears GICR_CTLR.EnableLPIs, vgic_flush_pending_lpis()
retires the LPIs on the vCPU's AP list. However, an LPI disabled in the
configuration table can be pending without being on that list. An LPI
moved to the vCPU by MOVI can also still be in another vCPU's LRs,
which pausing the vCPU doesn't fold back. These LPIs stay pending, and
the guest takes them even after re-enabling LPIs with
GICR_PENDBASER.PTZ set.

Halt the guest rather than the vCPU, so that no vCPU can fold such an
LPI back from its LRs after the flush. Have the flush clear the pending
state of every LPI targeting the vCPU. A forwarded vLPI's pending state
is in its vPE's pending table, which the flush doesn't clear.

Invalidate the translation cache before the flush. Only an MSI racing
with the disable can then make an LPI pending, or cache its translation
for later ones.

Fixes: 0e4e82f154e38 ("KVM: arm64: vgic-its: Enable ITS emulation as a virtual MSI controller")
Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev>
---
 arch/arm64/kvm/vgic/vgic-mmio-v3.c | 13 +++++++------
 arch/arm64/kvm/vgic/vgic.c         | 16 +++++++++++++++-
 2 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
index 02292f8e0b431..93024879ed3b3 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
@@ -298,14 +298,15 @@ static void vgic_mmio_write_v3r_ctlr(struct kvm_vcpu *vcpu,
 		/*
 		 * 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
+		 * sure that every 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.
+		 * Halt them before the LPIs read as disabled, so that none can
+		 * fold an LPI back from its LRs after vgic_flush_pending_lpis()
+		 * has cleared it.
 		 *
 		 * If it hurts, don't do it.
 		 */
-		kvm_pause_vcpu(vcpu);
+		kvm_arm_halt_guest(vcpu->kvm);
 
 		/*
 		 * Don't disable if RWP is set, as there already an
@@ -315,12 +316,12 @@ static void vgic_mmio_write_v3r_ctlr(struct kvm_vcpu *vcpu,
 					      GICR_CTLR_ENABLE_LPIS,
 					      GICR_CTLR_RWP);
 		if (ctlr == GICR_CTLR_ENABLE_LPIS) {
-			vgic_flush_pending_lpis(vcpu);
 			vgic_its_invalidate_all_caches(vcpu->kvm);
+			vgic_flush_pending_lpis(vcpu);
 			atomic_set_release(&vgic_cpu->ctlr, 0);
 		}
 
-		kvm_resume_vcpu(vcpu);
+		kvm_arm_resume_guest(vcpu->kvm);
 	} else {
 		ctlr = atomic_cmpxchg_acquire(&vgic_cpu->ctlr, 0,
 					      GICR_CTLR_ENABLE_LPIS);
diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c
index fe09ada322625..a99570538f104 100644
--- a/arch/arm64/kvm/vgic/vgic.c
+++ b/arch/arm64/kvm/vgic/vgic.c
@@ -207,9 +207,10 @@ static void vgic_release_deleted_lpis(struct kvm *kvm)
 void vgic_flush_pending_lpis(struct kvm_vcpu *vcpu)
 {
 	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
+	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
 	struct vgic_irq *irq, *tmp;
+	unsigned long intid, flags;
 	bool deleted = false;
-	unsigned long flags;
 
 	raw_spin_lock_irqsave(&vgic_cpu->ap_list_lock, flags);
 
@@ -228,6 +229,19 @@ void vgic_flush_pending_lpis(struct kvm_vcpu *vcpu)
 
 	if (deleted)
 		vgic_release_deleted_lpis(vcpu->kvm);
+
+	/* Disabled LPIs and LPIs MOVI'd here can be pending off this AP list. */
+	xa_for_each(&dist->lpi_xa, intid, irq) {
+		irq = vgic_get_irq(vcpu->kvm, intid);
+		if (!irq)
+			continue;
+
+		raw_spin_lock_irqsave(&irq->irq_lock, flags);
+		if (irq->target_vcpu == vcpu)
+			irq->pending_latch = false;
+		raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
+		vgic_put_irq(vcpu->kvm, irq);
+	}
 }
 
 void vgic_irq_set_phys_pending(struct vgic_irq *irq, bool pending)
-- 
2.47.3

[PATCH 2/3] KVM: arm64: vgic-v3: Don't save pending tables with LPIs disabled

[Fuad Tabba]

KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES writes each LPI's pending state
into its redistributor's pending table, clearing the bit of an LPI that
isn't pending. However, once a redistributor has LPIs disabled and RWP
reads 0, "there are no further accesses by the GIC to the LPI Pending
table" (IHI 0069H.b, 5.1.2) until enabling LPIs reads it. A bit the
guest sets there is cleared by the next save, which QEMU runs whenever
it stops the VM, and the guest never takes the LPI.

Skip the LPIs of a redistributor with LPIs disabled, and document it.
On GICv4.1 this drops the state of a forwarded vLPI there, which the
disable is allowed to lose: "There is no guarantee that clearing
GICR_CTLR.EnableLPIs causes the LPI Pending table to be updated in
memory" (5.1.2).

Fixes: 280771252c1ba ("KVM: arm64: vgic-v3: KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES")
Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev>
---
 Documentation/virt/kvm/devices/arm-vgic-v3.rst | 1 +
 arch/arm64/kvm/vgic/vgic-v3.c                  | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/virt/kvm/devices/arm-vgic-v3.rst b/Documentation/virt/kvm/devices/arm-vgic-v3.rst
index 5395ee66fc324..c878d3f728858 100644
--- a/Documentation/virt/kvm/devices/arm-vgic-v3.rst
+++ b/Documentation/virt/kvm/devices/arm-vgic-v3.rst
@@ -308,6 +308,7 @@ Groups:
       save all LPI pending bits into guest RAM pending tables.
 
       The first kB of the pending table is not altered by this operation.
+      Neither is the pending table of a redistributor with LPIs disabled.
 
   Errors:
 
diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
index 65b77fb35be37..9bbf11ddba92e 100644
--- a/arch/arm64/kvm/vgic/vgic-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-v3.c
@@ -625,7 +625,9 @@ int vgic_v3_save_pending_tables(struct kvm *kvm)
 			continue;
 
 		vcpu = irq->target_vcpu;
-		if (!vcpu)
+
+		/* While LPIs are disabled, the pending table is the guest's. */
+		if (!vcpu || !vgic_lpis_enabled(vcpu))
 			goto put_irq;
 
 		pendbase = GICR_PENDBASER_ADDRESS(vcpu->arch.vgic_cpu.pendbaser);
-- 
2.47.3

[PATCH 3/3] KVM: arm64: vgic-its: Don't read the pending table when mapping an LPI

[Fuad Tabba]

Mapping an LPI makes it pending if its bit in the target redistributor's
pending table is set. While the redistributor has LPIs enabled, KVM
doesn't keep that bit current. Saving the pending tables, as QEMU does
whenever it stops the VM, sets the bit of each pending LPI, and the bit
stays set once the guest has taken the LPI. Discarding the LPI and
mapping its INTID again makes it pending a second time.

MAPTI (IHI 0069H.b, 5.3.12) doesn't read the table, so stop reading it
when an LPI is created. Read it once the ITS tables are restored
instead, for each redistributor with LPIs enabled, as enabling LPIs
does (5.1.2). The documented restore order restores the redistributors
first.

The copy that commit b4c9f30194a4 ("KVM: arm64: vgic-v3: Don't drop a
pending LPI mapped with LPIs disabled") takes while LPIs are disabled
goes too, now that the save leaves that bit alone.

An LPI mapped after its redistributor enabled LPIs no longer picks up a
bit the table held at the enable. A GIC makes the LPI pending at the
enable, mapped or not (5.1.2). KVM has no pending state for an unmapped
INTID, and a bit the guest wrote is indistinguishable from one a save
left behind. Linux enables LPIs with a zeroed table.

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-its.c | 33 ++++++++++++++++--------
 arch/arm64/kvm/vgic/vgic-v3.c  | 46 ----------------------------------
 arch/arm64/kvm/vgic/vgic.h     |  1 -
 3 files changed, 23 insertions(+), 57 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
index 5ce485485a117..ede205e05dc04 100644
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -142,7 +142,7 @@ static struct vgic_irq *vgic_add_lpi(struct kvm *kvm, u32 intid,
 	 * However we only have those structs for mapped IRQs, so we read in
 	 * the respective config data from memory here upon mapping the LPI.
 	 *
-	 * Should any of these fail, behave as if we couldn't create the LPI
+	 * Should that fail, behave as if we couldn't create the LPI
 	 * by dropping the refcount and returning the error.
 	 */
 	ret = update_lpi_config(kvm, irq, NULL, false);
@@ -151,12 +151,6 @@ static struct vgic_irq *vgic_add_lpi(struct kvm *kvm, u32 intid,
 		return ERR_PTR(ret);
 	}
 
-	ret = vgic_v3_lpi_sync_pending_status(kvm, irq);
-	if (ret) {
-		vgic_put_irq(kvm, irq);
-		return ERR_PTR(ret);
-	}
-
 	return irq;
 }
 
@@ -2710,6 +2704,9 @@ static int vgic_its_save_tables_v0(struct vgic_its *its)
  */
 static int vgic_its_restore_tables_v0(struct vgic_its *its)
 {
+	struct kvm *kvm = its->dev->kvm;
+	struct kvm_vcpu *vcpu;
+	unsigned long i;
 	int ret;
 
 	ret = vgic_its_restore_collection_table(its);
@@ -2717,9 +2714,25 @@ static int vgic_its_restore_tables_v0(struct vgic_its *its)
 		return ret;
 
 	ret = vgic_its_restore_device_tables(its);
-	if (ret)
-		vgic_its_free_collection_list(its->dev->kvm, its);
-	return ret;
+	if (ret) {
+		vgic_its_free_collection_list(kvm, its);
+		return ret;
+	}
+
+	/* Enabling LPIs read the pending tables before these LPIs existed. */
+	kvm_for_each_vcpu(i, vcpu, kvm) {
+		if (!vgic_lpis_enabled(vcpu))
+			continue;
+
+		ret = its_sync_lpi_pending_table(vcpu);
+		if (ret) {
+			vgic_its_free_device_list(kvm, its);
+			vgic_its_free_collection_list(kvm, its);
+			return ret;
+		}
+	}
+
+	return 0;
 }
 
 static void vgic_its_commit_v0(struct vgic_its *its)
diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
index 9bbf11ddba92e..35e45479dfa42 100644
--- a/arch/arm64/kvm/vgic/vgic-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-v3.c
@@ -517,52 +517,6 @@ void vcpu_set_ich_hcr(struct kvm_vcpu *vcpu)
 				      ICH_HCR_EL2_TC);
 }
 
-int vgic_v3_lpi_sync_pending_status(struct kvm *kvm, struct vgic_irq *irq)
-{
-	struct kvm_vcpu *vcpu;
-	int byte_offset, bit_nr;
-	gpa_t pendbase, ptr;
-	bool status;
-	u8 val;
-	int ret;
-	unsigned long flags;
-
-retry:
-	vcpu = irq->target_vcpu;
-	if (!vcpu)
-		return 0;
-
-	pendbase = GICR_PENDBASER_ADDRESS(vcpu->arch.vgic_cpu.pendbaser);
-
-	byte_offset = irq->intid / BITS_PER_BYTE;
-	bit_nr = irq->intid % BITS_PER_BYTE;
-	ptr = pendbase + byte_offset;
-
-	ret = kvm_read_guest_lock(kvm, ptr, &val, 1);
-	if (ret)
-		return ret;
-
-	status = val & (1 << bit_nr);
-
-	raw_spin_lock_irqsave(&irq->irq_lock, flags);
-	if (irq->target_vcpu != vcpu) {
-		raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
-		goto retry;
-	}
-	irq->pending_latch = status;
-	vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
-
-	/* 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);
-		if (ret)
-			return ret;
-	}
-	return 0;
-}
-
 /*
  * The deactivation of the doorbell interrupt will trigger the
  * unmapping of the associated vPE.
diff --git a/arch/arm64/kvm/vgic/vgic.h b/arch/arm64/kvm/vgic/vgic.h
index 71c481ddd98d6..c4407f69f3613 100644
--- a/arch/arm64/kvm/vgic/vgic.h
+++ b/arch/arm64/kvm/vgic/vgic.h
@@ -347,7 +347,6 @@ void vgic_v3_reset(struct kvm_vcpu *vcpu);
 void vgic_v3_enable_cpuif_traps(void);
 int vgic_v3_probe(const struct gic_kvm_info *info);
 int vgic_v3_map_resources(struct kvm *kvm);
-int vgic_v3_lpi_sync_pending_status(struct kvm *kvm, struct vgic_irq *irq);
 int vgic_v3_save_pending_tables(struct kvm *kvm);
 int vgic_v3_set_redist_base(struct kvm *kvm, u32 index, u64 addr, u32 count);
 int vgic_register_redist_iodev(struct kvm_vcpu *vcpu);
-- 
2.47.3

Re: [PATCH 3/3] KVM: arm64: vgic-its: Don't read the pending table when mapping an LPI

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] KVM: arm64: vgic-its: its_sync_lpi_pending_table() reads unrelated LPIs
- [Medium] KVM: arm64: vgic-its: uncleaned queued LPIs in vgic_its_restore_tables_v0()
--

commit 5977878e13acf988213f1898fd84a41cd479ed52
Author: Fuad Tabba <fuad.tabba@linux.dev>

KVM: arm64: vgic-its: Don't read the pending table when mapping an LPI

This commit stops reading the redistributor LPI pending table when mapping an
LPI in vgic_add_lpi() and removes vgic_v3_lpi_sync_pending_status(). Instead,
it synchronizes the pending tables for all vCPUs with LPIs enabled at the end
of vgic_its_restore_tables_v0() via its_sync_lpi_pending_table().

> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> index 5ce485485a117..ede205e05dc04 100644
> --- a/arch/arm64/kvm/vgic/vgic-its.c
> +++ b/arch/arm64/kvm/vgic/vgic-its.c

[ ... ]

> @@ -2710,6 +2704,9 @@ static int vgic_its_save_tables_v0(struct vgic_its *its)
>   */
>  static int vgic_its_restore_tables_v0(struct vgic_its *its)
>  {
> +	struct kvm *kvm = its->dev->kvm;
> +	struct kvm_vcpu *vcpu;
> +	unsigned long i;
>  	int ret;
>  
>  	ret = vgic_its_restore_collection_table(its);
> @@ -2717,9 +2714,25 @@ static int vgic_its_restore_tables_v0(struct vgic_its *its)
>  		return ret;
>  
>  	ret = vgic_its_restore_device_tables(its);
> -	if (ret)
> -		vgic_its_free_collection_list(its->dev->kvm, its);
> -	return ret;
> +	if (ret) {
> +		vgic_its_free_collection_list(kvm, its);
> +		return ret;
> +	}
> +
> +	/* Enabling LPIs read the pending tables before these LPIs existed. */
> +	kvm_for_each_vcpu(i, vcpu, kvm) {
> +		if (!vgic_lpis_enabled(vcpu))
> +			continue;
> +
> +		ret = its_sync_lpi_pending_table(vcpu);

[Severity: Medium]
Can calling its_sync_lpi_pending_table() here cause KVM_DEV_ARM_ITS_RESTORE_TABLES
to fail with -EFAULT when a vCPU with LPIs enabled has a pending table that
does not cover LPI INTIDs targeting other vCPUs or unmapped collections?

Previously, vgic_add_lpi() called vgic_v3_lpi_sync_pending_status(), which
checked irq->target_vcpu (returning 0 if NULL) and only read the pending table
of the redistributor that the LPI actually targeted.

In contrast, its_sync_lpi_pending_table() iterates over every LPI in
dist->lpi_xa and reads from the current vCPU's pendbase before checking
whether the LPI targets that vCPU:

arch/arm64/kvm/vgic/vgic-its.c:its_sync_lpi_pending_table() {
	xa_for_each(&dist->lpi_xa, intid, irq) {
		...
		if (byte_offset != last_byte_offset) {
			ret = kvm_read_guest_lock(vcpu->kvm,
						  pendbase + byte_offset,
						  &pendmask, 1);
			if (ret)
				return ret;

			last_byte_offset = byte_offset;
		}

		irq = vgic_get_irq(vcpu->kvm, intid);
		if (!irq)
			continue;

		raw_spin_lock_irqsave(&irq->irq_lock, flags);
		if (irq->target_vcpu == vcpu)
			irq->pending_latch = pendmask & (1U << bit_nr);
		vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
		vgic_put_irq(vcpu->kvm, irq);
	}
}

If a guest enables LPIs on a vCPU that has no LPIs targeting it (or only
lower-INTID LPIs) with an unmapped or smaller pending table in GICR_PENDBASER,
while LPIs in dist->lpi_xa target other vCPUs or unmapped collections,
its_sync_lpi_pending_table() will read that vCPU's pending table at the byte
offsets of those unrelated LPIs and return -EFAULT. That also performs
O(nr_vcpus * nr_lpis) guest memory reads and vgic_queue_irq_unlock() calls
across all vCPUs during restore.

> +		if (ret) {
> +			vgic_its_free_device_list(kvm, its);
> +			vgic_its_free_collection_list(kvm, its);
> +			return ret;
> +		}

[Severity: Medium]
If its_sync_lpi_pending_table() fails partway through syncing the vCPUs'
pending tables, does this error path leave already-queued LPIs referenced in
dist->lpi_xa and on vcpu->arch.vgic_cpu.ap_list_head?

During the documented restore sequence, the distributor and redistributors are
restored before KVM_DEV_ARM_ITS_RESTORE_TABLES is invoked. When
its_sync_lpi_pending_table() sees a pending bit set for an enabled LPI, it
sets irq->pending_latch and calls vgic_queue_irq_unlock(), which takes an
additional reference via vgic_get_irq_ref() and adds irq->ap_list to
vcpu->arch.vgic_cpu.ap_list_head.

If a later kvm_read_guest_lock() call fails (on a subsequent byte offset or a
subsequent vCPU), vgic_its_free_device_list() frees the ITEs via
its_free_ite():

arch/arm64/kvm/vgic/vgic-its.c:its_free_ite() {
	if (irq) {
		scoped_guard(raw_spinlock_irqsave, &irq->irq_lock) {
			if (irq->hw)
				its_unmap_vlpi(ite->irq->host_irq);

			irq->hw = false;
			irq->mapped = false;
		}

		vgic_put_irq(kvm, ite->irq);
	}
}

Because its_free_ite() only drops the single ITE reference without clearing
irq->pending_latch or flushing ap_list_head, any LPI that was already queued
retains its ap_list reference and stays in dist->lpi_xa and on ap_list_head
with irq->mapped = false. If the vCPU runs, vgic_target_oracle() does not
check irq->mapped and can inject the unmapped LPI, or if
KVM_DEV_ARM_ITS_RESTORE_TABLES is retried, vgic_add_lpi() will find the
existing vgic_irq in dist->lpi_xa and return it early without calling
update_lpi_config().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105110050.1244525-1-fuad.tabba@linux.dev?part=3

Re: [PATCH 3/3] KVM: arm64: vgic-its: Don't read the pending table when mapping an LPI

[Fuad Tabba]

On Thu, 05 Nov 2026 11:23:34 +0000, sashiko-bot@kernel.org wrote:
[...]
> > diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
[...]
> > @@ -2717,9 +2714,25 @@ static int vgic_its_restore_tables_v0(struct vgic_its *its)
[...]
> > +        /* Enabling LPIs read the pending tables before these LPIs existed. */
> > +        kvm_for_each_vcpu(i, vcpu, kvm) {
> > +                if (!vgic_lpis_enabled(vcpu))
> > +                        continue;
> > +
> > +                ret = its_sync_lpi_pending_table(vcpu);
>
> [Severity: Medium]
> Can calling its_sync_lpi_pending_table() here cause KVM_DEV_ARM_ITS_RESTORE_TABLES
> to fail with -EFAULT when a vCPU with LPIs enabled has a pending table that
> does not cover LPI INTIDs targeting other vCPUs or unmapped collections?

Yes. Unlike the map-time read, this reads each LPI-enabled
redistributor's table at the bits of LPIs targeting others. Will fix
in v2 by skipping those LPIs.

[...]
> > +                if (ret) {
> > +                        vgic_its_free_device_list(kvm, its);
> > +                        vgic_its_free_collection_list(kvm, its);
> > +                        return ret;
> > +                }
>
> [Severity: Medium]
> If its_sync_lpi_pending_table() fails partway through syncing the vCPUs'
> pending tables, does this error path leave already-queued LPIs referenced in
> dist->lpi_xa and on vcpu->arch.vgic_cpu.ap_list_head?

Pre-existing. On kvmarm/fixes, vgic_add_lpi() already queues a pending
LPI as the restore maps it, so a later device-table failure leaves the
same state. The early return in vgic_add_lpi() on a retried restore is
a separate bug.

Cheers,
/fuad

Re: [PATCH 0/3] KVM: arm64: vgic: Drop stale LPI pending state

[Marc Zyngier]

On Thu, 05 Nov 2026 11:00:47 +0000,
Fuad Tabba <fuad.tabba@linux.dev> wrote:
> 
> Hi folks,
> 
> The GIC just keeps on giving. Three follow-ups to my LPI fixes for
> redistributors with LPIs disabled [1], to fix pre-existing bugs Sashiko
> found.

Can you *please* come up with a full series rather than this constant
"here's another set of patches" that address the same thing, one
pothole at a time?

I'm now really tempted to drop all the patches I have queued on this
so far, and wait for the "one true series".

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 0/3] KVM: arm64: vgic: Drop stale LPI pending state

[Fuad Tabba]

On Thu, 5 Nov 2026 at 15:51, Marc Zyngier <maz@kernel.org> wrote:
>
> On Thu, 05 Nov 2026 11:00:47 +0000,
> Fuad Tabba <fuad.tabba@linux.dev> wrote:
> >
> > Hi folks,
> >
> > The GIC just keeps on giving. Three follow-ups to my LPI fixes for
> > redistributors with LPIs disabled [1], to fix pre-existing bugs Sashiko
> > found.
>
> Can you *please* come up with a full series rather than this constant
> "here's another set of patches" that address the same thing, one
> pothole at a time?
>
> I'm now really tempted to drop all the patches I have queued on this
> so far, and wait for the "one true series".

Sorry about that. Every time I think I'm done with it, Sashiko pokes more holes!

I'll sit on this a bit more, make sure all the gaps are closed,
Sashiko it locally, before I respin.

Cheers,
/fuad

>
>         M.
>
> --
> Without deviation from the norm, progress is not possible.