KVM: arm64: vgic: LPI fixes for redistributors with LPIs disabled

arch/arm64/kvm/vgic/vgic-mmio-v3.c static void vgic_mmio_write_v3r_ctlr(struct kvm_vcpu *vcpu, · line 316

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

arch/arm64/kvm/vgic/vgic.c struct kvm_vcpu *vgic_target_oracle(struct vgic_irq *irq) · line 285

[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/[email protected]?part=1

arch/arm64/kvm/vgic/vgic-v3.c int vgic_v3_lpi_sync_pending_status(struct kvm *kvm, struct vgic_irq *irq) · line 556

[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/[email protected]?part=2