Re: [PATCH v3 02/62] KVM: arm64: WARN if unmapping vLPI fails

Oliver Upton
2025-06-20T18:49:22Z

On Fri, Jun 20, 2025 at 10:22:32AM -0700, Sean Christopherson wrote:
> On Fri, Jun 13, 2025, Oliver Upton wrote:
> > On Thu, Jun 12, 2025 at 07:34:35AM -0700, Sean Christopherson wrote:
> > > On Thu, Jun 12, 2025, Marc Zyngier wrote:
> > > > But not having an VLPI mapping for an interrupt at the point where we're
> > > > tearing down the forwarding is pretty benign. IRQs *still* go where they
> > > > should, and we don't lose anything.
> >
> > The VM may not actually be getting torn down, though. The series of
> > fixes [*] we took for 6.16 addressed games that VMMs might be playing on
> > irqbypass for a live VM.
> >
> > [*] https://lore.kernel.org/kvmarm/20250523194722.4066715-1-oliver.upton@linux.dev/
> >
> > > All of those failure scenario seem like warnable offences when KVM thinks it has
> > > configured the IRQ to be forwarded to a vCPU.
> >
> > I tend to agree here, especially considering how horribly fragile GICv4
> > has been in some systems. I know of a couple implementations where ITS
> > command failures and/or unmapped MSIs are fatal for the entire machine.
> > Debugging them has been a genuine pain in the ass.
> >
> > WARN'ing when state tracking for vLPIs is out of whack would've made it
> > a little easier.
>
> Marc, does this look and read better?
>
> I'd really, really like to get this sorted out asap, as it's the only thing
> blocking the series, and I want to get the series into linux-next early next
> week, before I go OOO for ~10 days.

Can you just send it out as a standalone patch? It's only tangientally
related to the truckload of x86 stuff that I'd rather not pull in the
event of conflict resolution.

Thanks,
Oliver

> --
> From: Sean Christopherson <seanjc@google.com>
> Date: Thu, 12 Jun 2025 16:51:47 -0700
> Subject: [PATCH] KVM: arm64: WARN if unmapping a vLPI fails in any path
>
> When unmapping a vLPI, WARN if nullifying vCPU affinity fails, not just if
> failure occurs when freeing an ITE. If undoing vCPU affinity fails, then
> odds are very good that vLPI state tracking has has gotten out of whack,
> i.e. that KVM and the GIC disagree on the state of an IRQ/vLPI. At best,
> inconsistent state means there is a lurking bug/flaw somewhere. At worst,
> the inconsistency could eventually be fatal to the host, e.g. if an ITS
> command fails because KVM's view of things doesn't match reality/hardware.
>
> Note, only the call from kvm_arch_irq_bypass_del_producer() by way of
> kvm_vgic_v4_unset_forwarding() doesn't already WARN. Common KVM's
> kvm_irq_routing_update() WARNs if kvm_arch_update_irqfd_routing() fails.
> For that path, if its_unmap_vlpi() fails in kvm_vgic_v4_unset_forwarding(),
> the only possible causes are that the GIC doesn't have a v4 ITS (from
> its_irq_set_vcpu_affinity()):
>
> /* Need a v4 ITS */
> if (!is_v4(its_dev->its))
> return -EINVAL;
>
> guard(raw_spinlock)(&its_dev->event_map.vlpi_lock);
>
> /* Unmap request? */
> if (!info)
> return its_vlpi_unmap(d);
>
> or that KVM has gotten out of sync with the GIC/ITS (from its_vlpi_unmap()):
>
> if (!its_dev->event_map.vm || !irqd_is_forwarded_to_vcpu(d))
> return -EINVAL;
>
> All of the above failure scenarios are warnable offences, as they should
> never occur absent a kernel/KVM bug.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> arch/arm64/kvm/vgic/vgic-its.c | 2 +-
> arch/arm64/kvm/vgic/vgic-v4.c | 4 ++--
> drivers/irqchip/irq-gic-v4.c | 4 ++--
> include/linux/irqchip/arm-gic-v4.h | 2 +-
> 4 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> index 534049c7c94b..98630dae910d 100644
> --- a/arch/arm64/kvm/vgic/vgic-its.c
> +++ b/arch/arm64/kvm/vgic/vgic-its.c
> @@ -758,7 +758,7 @@ static void its_free_ite(struct kvm *kvm, struct its_ite *ite)
> if (irq) {
> scoped_guard(raw_spinlock_irqsave, &irq->irq_lock) {
> if (irq->hw)
> - WARN_ON(its_unmap_vlpi(ite->irq->host_irq));
> + its_unmap_vlpi(ite->irq->host_irq);
>
> irq->hw = false;
> }
> diff --git a/arch/arm64/kvm/vgic/vgic-v4.c b/arch/arm64/kvm/vgic/vgic-v4.c
> index 193946108192..911170d4a9c8 100644
> --- a/arch/arm64/kvm/vgic/vgic-v4.c
> +++ b/arch/arm64/kvm/vgic/vgic-v4.c
> @@ -545,10 +545,10 @@ int kvm_vgic_v4_unset_forwarding(struct kvm *kvm, int host_irq)
> if (irq->hw) {
> atomic_dec(&irq->target_vcpu->arch.vgic_cpu.vgic_v3.its_vpe.vlpi_count);
> irq->hw = false;
> - ret = its_unmap_vlpi(host_irq);
> + its_unmap_vlpi(host_irq);
> }
>
> raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
> vgic_put_irq(kvm, irq);
> - return ret;
> + return 0;
> }
> diff --git a/drivers/irqchip/irq-gic-v4.c b/drivers/irqchip/irq-gic-v4.c
> index 58c28895f8c4..8455b4a5fbb0 100644
> --- a/drivers/irqchip/irq-gic-v4.c
> +++ b/drivers/irqchip/irq-gic-v4.c
> @@ -342,10 +342,10 @@ int its_get_vlpi(int irq, struct its_vlpi_map *map)
> return irq_set_vcpu_affinity(irq, &info);
> }
>
> -int its_unmap_vlpi(int irq)
> +void its_unmap_vlpi(int irq)
> {
> irq_clear_status_flags(irq, IRQ_DISABLE_UNLAZY);
> - return irq_set_vcpu_affinity(irq, NULL);
> + WARN_ON_ONCE(irq_set_vcpu_affinity(irq, NULL));
> }
>
> int its_prop_update_vlpi(int irq, u8 config, bool inv)
> diff --git a/include/linux/irqchip/arm-gic-v4.h b/include/linux/irqchip/arm-gic-v4.h
> index 7f1f11a5e4e4..0b0887099fd7 100644
> --- a/include/linux/irqchip/arm-gic-v4.h
> +++ b/include/linux/irqchip/arm-gic-v4.h
> @@ -146,7 +146,7 @@ int its_commit_vpe(struct its_vpe *vpe);
> int its_invall_vpe(struct its_vpe *vpe);
> int its_map_vlpi(int irq, struct its_vlpi_map *map);
> int its_get_vlpi(int irq, struct its_vlpi_map *map);
> -int its_unmap_vlpi(int irq);
> +void its_unmap_vlpi(int irq);
> int its_prop_update_vlpi(int irq, u8 config, bool inv);
> int its_prop_update_vsgi(int irq, u8 priority, bool group);
>
>
> base-commit: 4fc39a165c70a49991b7cc29be3a19eddcd9e5b9
> --
>