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

[Oliver Upton <oliver.upton@linux.dev>]

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