[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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] KVM: arm64: vgic: stale pending_latch when vgic_target_oracle() drops LPI
- [Medium] KVM: arm64: vgic: unretired LPI pending_latch in vgic_prune_ap_list() race
--

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

KVM: arm64: vgic: Don't forward LPIs to a redistributor with LPIs disabled

This commit updates vgic_target_oracle() to return NULL for LPIs targeting a
redistributor with GICR_CTLR.EnableLPIs cleared. It also reorders
vgic_mmio_write_v3r_ctlr() to pause the target vCPU before clearing
GICR_CTLR.EnableLPIs so that vgic_flush_pending_lpis() can retire pending
LPIs before vgic_prune_ap_list() removes them from the ap_list.

Link: https://lore.kernel.org/r/asf87i2RDh-WksL-@kernel.org

> 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,28 +293,31 @@ static void vgic_mmio_write_v3r_ctlr(struct kvm_vcpu *vcpu,
>  		return;
>  
>  	if (!(val & GICR_CTLR_ENABLE_LPIS)) {
> -		/*
> -		 * Don't disable if RWP is set, as there already an
> -		 * ongoing disable. Funky guest...
> -		 */
> -		ctlr = atomic_cmpxchg_acquire(&vgic_cpu->ctlr,
> -					      GICR_CTLR_ENABLE_LPIS,
> -					      GICR_CTLR_RWP);
> -		if (ctlr != GICR_CTLR_ENABLE_LPIS)
> -			return;
> -
>  		/*
>  		 * 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);
> -		vgic_flush_pending_lpis(vcpu);
> -		vgic_its_invalidate_all_caches(vcpu->kvm);
> -		atomic_set_release(&vgic_cpu->ctlr, 0);
> +
> +		/*
> +		 * Don't disable if RWP is set, as there already an
> +		 * ongoing disable. Funky guest...
> +		 */
> +		ctlr = atomic_cmpxchg_acquire(&vgic_cpu->ctlr,
> +					      GICR_CTLR_ENABLE_LPIS,
> +					      GICR_CTLR_RWP);
> +		if (ctlr == GICR_CTLR_ENABLE_LPIS) {
> +			vgic_flush_pending_lpis(vcpu);

[Severity: Medium]
Is kvm_pause_vcpu() sufficient to prevent vgic_prune_ap_list() from seeing
!vgic_lpis_enabled(vcpu) and dropping pending LPIs off an ap_list without
clearing irq->pending_latch before vgic_flush_pending_lpis() retires them?

First, kvm_pause_vcpu() sets KVM_REQ_SLEEP and calls
kvm_make_vcpus_request_mask(), which only waits for vcpu->mode to become
OUTSIDE_GUEST_MODE. In arch/arm64/kvm/arm.c:check_vcpu_requests(), the vCPU
already runs in OUTSIDE_GUEST_MODE:

    if (kvm_check_request(KVM_REQ_SLEEP, vcpu))
        kvm_vcpu_sleep(vcpu);
    ...
    if (kvm_check_request(KVM_REQ_VGIC_PROCESS_UPDATE, vcpu))
        kvm_vgic_process_async_update(vcpu);

If KVM_REQ_SLEEP is set right after the vCPU passes the KVM_REQ_SLEEP check
(or if kvm_vcpu_sleep() wakes early due to a pending signal),
kvm_pause_vcpu() returns while the vCPU continues into
kvm_vgic_process_async_update() -> vgic_prune_ap_list() concurrently with
atomic_cmpxchg_acquire() and vgic_flush_pending_lpis().

Second, if a pending LPI queued on vcpuA (irq->vcpu == vcpuA) was retargeted
to vcpuB (irq->target_vcpu = vcpuB, for example via MOVI or MAPC in
update_affinity()) before vcpuA exited to prune its ap_list, and vcpuB then
disables LPIs, vgic_mmio_write_v3r_ctlr() only pauses and flushes vcpuB. When
vcpuA exits and runs vgic_prune_ap_list(), vgic_target_oracle(irq) observes
!vgic_lpis_enabled(vcpuB) and returns NULL:

    target_vcpu = vgic_target_oracle(irq);

    if (!target_vcpu) {
        list_del(&irq->ap_list);
        irq->vcpu = NULL;
        raw_spin_unlock(&irq->irq_lock);

This removes irq from vcpuA's ap_list while leaving irq->pending_latch set to
true, which can later cause a stale pending bit to be saved by
vgic_v3_save_pending_tables() or delivered spuriously if LPIs are re-enabled
with GICR_PENDBASER_PTZ set followed by INV/INVALL.

> 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;
> +
> +		if (unlikely(vcpu && irq_is_lpi(vcpu->kvm, irq->intid) && !vgic_lpis_enabled(vcpu)))
>  			return NULL;

[Severity: Medium]
When an MSI injection races with clearing GICR_CTLR.EnableLPIs in
vgic_mmio_write_v3r_ctlr(), can returning NULL here leave irq->pending_latch
stuck at true and leave a live entry in its->translation_cache?

In vgic_mmio_write_v3r_ctlr(), atomic_cmpxchg_acquire() sets vgic_cpu->ctlr
to GICR_CTLR_RWP before calling vgic_flush_pending_lpis() and
vgic_its_invalidate_all_caches(). If vgic_its_inject_msi() hits the cache via
vgic_its_inject_cached_translation() in that window:

    raw_spin_lock_irqsave(&irq->irq_lock, flags);
    irq->pending_latch = true;
    vgic_queue_irq_unlock(kvm, irq, flags);

vgic_target_oracle() sees !vgic_lpis_enabled(vcpu) and returns NULL, so
vgic_queue_irq_unlock() does not add irq to ap_list_head and does not clear
irq->pending_latch. Because vgic_flush_pending_lpis() only walks
ap_list_head, irq->pending_latch remains true while irq->vcpu is NULL.

Similarly, in arch/arm64/kvm/vgic/vgic-its.c:vgic_its_resolve_lpi():

    if (!vgic_lpis_enabled(vcpu))
        return -EBUSY;

    vgic_its_cache_translation(kvm, its, devid, eventid, ite->irq);

Because vgic_its_resolve_lpi() is not serialized against
vgic_mmio_write_v3r_ctlr(), it can pass the vgic_lpis_enabled(vcpu) check
just before ctlr transitions to GICR_CTLR_RWP, insert the translation into
its->translation_cache after vgic_its_invalidate_all_caches() has already
finished, and set irq->pending_latch = true in vgic_its_trigger_msi().

Wouldn't this leave a stale irq->pending_latch that gets written to guest
memory on migration by vgic_v3_save_pending_tables(), forwarded by
kvm_vgic_v4_set_forwarding(), or delivered after re-enabling LPIs with
GICR_PENDBASER_PTZ set and issuing INV/INVALL?

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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] KVM: arm64: vgic-v3: unconsumed pending table bit when LPIs are enabled
- [High] KVM: arm64: vgic-v3: TOCTOU race on vgic_lpis_enabled() drops pending LPI
--

commit 1318a5e945ab06141897d5ffe8f4348f6a0ecf31
Author: Fuad Tabba <fuad.tabba@linux.dev>

KVM: arm64: vgic-v3: Don't drop a pending LPI mapped with LPIs disabled

This commit updates vgic_v3_lpi_sync_pending_status() so that it only clears
the pending bit in the guest redistributor's pending table when LPIs are
enabled on the target vCPU. This ensures that when LPIs are subsequently
enabled and its_sync_lpi_pending_table() reloads pending state from guest
memory, a pending LPI mapped while LPIs were disabled does not lose its
pending status.

> 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)) {

[Severity: High]
When vgic_lpis_enabled(vcpu) is false, vgic_v3_lpi_sync_pending_status() leaves
the pending bit set in the guest's pending table for its_sync_lpi_pending_table()
to consume later.

However, when the guest enables LPIs on the redistributor via
vgic_mmio_write_v3r_ctlr() -> vgic_enable_lpis() -> its_sync_lpi_pending_table(),
its_sync_lpi_pending_table() reads the pending bit into irq->pending_latch
without clearing the bit in guest memory:

arch/arm64/kvm/vgic/vgic-its.c:its_sync_lpi_pending_table() {
	...
	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);
	...
}

Could leaving that bit set in guest RAM cause spurious LPI delivery in the
following cases?

1. After the guest acknowledges and deactivates the LPI, only irq->pending_latch
   is cleared in software while the bit in guest RAM remains 1. If the guest
   later discards and remaps the LPI while LPIs are enabled, vgic_add_lpi() ->
   vgic_v3_lpi_sync_pending_status() will read the stale bit from guest RAM and
   inject a spurious interrupt (or if the guest disables and re-enables LPIs,
   its_sync_lpi_pending_table() will re-inject the already-consumed interrupt).

2. If the guest issues an ITS CLEAR command (vgic_its_cmd_handle_clear())
   before enabling LPIs, CLEAR only clears irq->pending_latch, and enabling
   LPIs via its_sync_lpi_pending_table() will resurrect the cleared interrupt
   from the bit left in guest RAM.

[Severity: High]
Can a race between vgic_v3_lpi_sync_pending_status() on one vCPU and
vgic_mmio_write_v3r_ctlr() enabling LPIs on another vCPU cause the pending LPI
to still be dropped?

In vgic_v3_lpi_sync_pending_status(), vgic_queue_irq_unlock() calls
vgic_target_oracle(irq) while holding irq->irq_lock:

arch/arm64/kvm/vgic/vgic.c:vgic_target_oracle() {
	...
	if (unlikely(vcpu && irq_is_lpi(vcpu->kvm, irq->intid) && !vgic_lpis_enabled(vcpu)))
		return NULL;
	...
}

If vgic_lpis_enabled(vcpu) is false at that point, vgic_target_oracle() returns
NULL and vgic_queue_irq_unlock() unlocks irq->irq_lock without queueing the LPI
onto the target vCPU's ap_list.

If another vCPU concurrently writes GICR_CTLR_ENABLE_LPIS in
vgic_mmio_write_v3r_ctlr() right after vgic_queue_irq_unlock() checks
vgic_lpis_enabled(vcpu):

CPU 0 (vgic_v3_lpi_sync_pending_status)     CPU 1 (vgic_mmio_write_v3r_ctlr)
---------------------------------------     --------------------------------
vgic_queue_irq_unlock()
  vgic_target_oracle()
    !vgic_lpis_enabled(vcpu) -> returns NULL
  unlocks irq->irq_lock (not queued)
                                            atomic_cmpxchg_acquire(&vgic_cpu->ctlr,
                                                0, GICR_CTLR_ENABLE_LPIS);
if (status && vgic_lpis_enabled(vcpu))
  /* now true! */
  val &= ~(1 << bit_nr);
  vgic_write_guest_lock(kvm, ptr, &val, 1);
                                            vgic_enable_lpis(vcpu)
                                              its_sync_lpi_pending_table(vcpu)
                                                /* reads 0 from guest table */
                                                irq->pending_latch = 0;

Wouldn't CPU 0 observe vgic_lpis_enabled(vcpu) as true on the second check and
clear the bit in the guest pending table before its_sync_lpi_pending_table()
reads it, causing its_sync_lpi_pending_table() to overwrite irq->pending_latch
with 0 and lose the pending interrupt?

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

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

[Fuad Tabba]

On Mon, 26 Oct 2026 10:13:14 +0000, sashiko-bot@kernel.org wrote:
[...]
> > +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> > @@ -293,28 +293,31 @@ static void vgic_mmio_write_v3r_ctlr(struct kvm_vcpu *vcpu,
[...]
> > +                if (ctlr == GICR_CTLR_ENABLE_LPIS) {
> > +                        vgic_flush_pending_lpis(vcpu);
>
> [Severity: Medium]
> Is kvm_pause_vcpu() sufficient to prevent vgic_prune_ap_list() from seeing
> !vgic_lpis_enabled(vcpu) and dropping pending LPIs off an ap_list without
> clearing irq->pending_latch before vgic_flush_pending_lpis() retires them?
[...]

No. A prune on a KVM_REQ_VGIC_PROCESS_UPDATE request, or on the vCPU
the LPI was retargeted away from, can drop it with its latch set. The
guest still doesn't take it with EnableLPIs clear. Taking it after a
re-enable with PTZ set is a pre-existing bug, which I'll fix in its
own patch, not in this series.

> > +++ b/arch/arm64/kvm/vgic/vgic.c
> > @@ -273,14 +273,18 @@ struct kvm_vcpu *vgic_target_oracle(struct vgic_irq *irq)
[...]
> > +                if (unlikely(vcpu && irq_is_lpi(vcpu->kvm, irq->intid) && !vgic_lpis_enabled(vcpu)))
> >                          return NULL;
>
> [Severity: Medium]
> When an MSI injection races with clearing GICR_CTLR.EnableLPIs in
> vgic_mmio_write_v3r_ctlr(), can returning NULL here leave irq->pending_latch
> stuck at true and leave a live entry in its->translation_cache?
[...]

The same latch, with the same limit and the same separate fix.

Cheers,
/fuad

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

[Fuad Tabba]

On Mon, 26 Oct 2026 10:33:42 +0000, sashiko-bot@kernel.org wrote:
[...]
> > +++ 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)) {
>
> [Severity: High]
> When vgic_lpis_enabled(vcpu) is false, vgic_v3_lpi_sync_pending_status() leaves
> the pending bit set in the guest's pending table for its_sync_lpi_pending_table()
> to consume later.
[...]
> Could leaving that bit set in guest RAM cause spurious LPI delivery in the
> following cases?
>
> 1. After the guest acknowledges and deactivates the LPI, only irq->pending_latch
>    is cleared in software while the bit in guest RAM remains 1. If the guest
>    later discards and remaps the LPI while LPIs are enabled, vgic_add_lpi() ->
>    vgic_v3_lpi_sync_pending_status() will read the stale bit from guest RAM and
>    inject a spurious interrupt (or if the guest disables and re-enables LPIs,
>    its_sync_lpi_pending_table() will re-inject the already-consumed interrupt).

That's pre-existing: its_sync_lpi_pending_table() has never cleared
the bits it reads, for any LPI pending at the enable. This patch adds
LPIs mapped with LPIs disabled to that set. I'll look at clearing them
separately, outside this series.

> 2. If the guest issues an ITS CLEAR command (vgic_its_cmd_handle_clear())
>    before enabling LPIs, CLEAR only clears irq->pending_latch, and enabling
>    LPIs via its_sync_lpi_pending_table() will resurrect the cleared interrupt
>    from the bit left in guest RAM.

A redistributor with EnableLPIs clear ignores commands involving its
LPIs, so hardware also takes this LPI at the enable.

> [Severity: High]
> Can a race between vgic_v3_lpi_sync_pending_status() on one vCPU and
> vgic_mmio_write_v3r_ctlr() enabling LPIs on another vCPU cause the pending LPI
> to still be dropped?
[...]

Predates this series. Without it, the map clears the bit
unconditionally, so a later enable with PTZ clear drops the LPI,
racing or not.

Cheers,
/fuad

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

[Oliver Upton]

On Mon, Oct 26, 2026 at 09:28:00AM +0000, Fuad Tabba wrote:
> 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

Reviewed-by: Oliver Upton <oupton@kernel.org>

>  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
> 
>