Re: [PATCH 4/5] KVM: arm64: Unmap vLPIs affected by changes to GSI routing information

[Sean Christopherson <seanjc@google.com>]

On Fri, May 23, 2025, Marc Zyngier wrote:
> On Fri, 23 May 2025 17:08:09 +0100,
> Oliver Upton <oliver.upton@linux.dev> wrote:
> > 
> > KVM's interrupt infrastructure is dodgy at best, allowing for some ugly
> > 'off label' usage of the various UAPIs. In one example, userspace can
> > change the routing entry of a particular "GSI" after configuring
> > irqbypass with KVM_IRQFD. KVM/arm64 is oblivious to this, and winds up
> > preserving the stale translation in cases where vLPIs are configured.
> > 
> > Honor userspace's intentions and tear down the vLPI mapping if affected
> > by a "GSI" routing change. Make no attempt to reconstruct vLPIs if the
> > new target is an MSI and just fall back to software injection.
> > 
> > Tested-by: Sweet Tea Dorminy <sweettea@google.com>
> > Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> > ---
> >  arch/arm64/kvm/arm.c | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> > 
> > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > index 1de49b48e35e..505d504b52b5 100644
> > --- a/arch/arm64/kvm/arm.c
> > +++ b/arch/arm64/kvm/arm.c
> > @@ -2790,6 +2790,7 @@ int kvm_arch_irq_bypass_add_producer(struct irq_bypass_consumer *cons,
> >  	return kvm_vgic_v4_set_forwarding(irqfd->kvm, prod->irq,
> >  					  &irqfd->irq_entry);
> >  }
> > +
> >  void kvm_arch_irq_bypass_del_producer(struct irq_bypass_consumer *cons,
> >  				      struct irq_bypass_producer *prod)
> >  {
> > @@ -2803,6 +2804,28 @@ void kvm_arch_irq_bypass_del_producer(struct irq_bypass_consumer *cons,
> >  	kvm_vgic_v4_unset_forwarding(irqfd->kvm, prod->irq);
> >  }
> >  
> > +bool kvm_arch_irqfd_route_changed(struct kvm_kernel_irq_routing_entry *old,
> > +				  struct kvm_kernel_irq_routing_entry *new)
> > +{
> > +	if (new->type != KVM_IRQ_ROUTING_MSI)
> > +		return true;
> > +
> > +	return memcmp(&old->msi, &new->msi, sizeof(new->msi));
> > +}
> > +
> > +int kvm_arch_update_irqfd_routing(struct kvm *kvm, unsigned int host_irq,
> > +				  uint32_t guest_irq, bool set)
> 
> If we're adding this, can we take out the trash and get rid of this
> 'set' parameter? Its only purpose is to be set to '1' and fed to the
> x86-specific stuff. How about this:

Can we hold off on any changes to the common APIs?  Unless they're urgent and
need to land in 6.16, I've got a massive series that I've been slowing working
on for ~7 months that fixes this wart, and several more, e.g. gets rid of
kvm_arch_irqfd_route_changed() entirely, and make the above ugliness into:

void kvm_arch_update_irqfd_routing(struct kvm_kernel_irqfd *irqfd,
				   struct kvm_kernel_irq_routing_entry *old,
				   struct kvm_kernel_irq_routing_entry *new)


I'm ~90% confident it'll land in 6.17:
https://lore.kernel.org/all/20250523010004.3240643-1-seanjc@google.com

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index df5b99ea1f181..2d69609c1ec00 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -13613,9 +13613,9 @@ void kvm_arch_irq_bypass_del_producer(struct irq_bypass_consumer *cons,
>  }
>  
>  int kvm_arch_update_irqfd_routing(struct kvm *kvm, unsigned int host_irq,
> -				   uint32_t guest_irq, bool set)
> +				   uint32_t guest_irq)
>  {
> -	return kvm_x86_call(pi_update_irte)(kvm, host_irq, guest_irq, set);
> +	return kvm_x86_call(pi_update_irte)(kvm, host_irq, guest_irq, true);
>  }
>  
>  bool kvm_arch_irqfd_route_changed(struct kvm_kernel_irq_routing_entry *old,
> diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
> index 11e5d1e3f12ea..2e60d0bf02e2f 100644
> --- a/virt/kvm/eventfd.c
> +++ b/virt/kvm/eventfd.c
> @@ -287,7 +287,7 @@ void __attribute__((weak)) kvm_arch_irq_bypass_start(
>  
>  int  __attribute__((weak)) kvm_arch_update_irqfd_routing(
>  				struct kvm *kvm, unsigned int host_irq,
> -				uint32_t guest_irq, bool set)
> +				uint32_t guest_irq)
>  {
>  	return 0;
>  }
> @@ -621,7 +621,7 @@ void kvm_irq_routing_update(struct kvm *kvm)
>  		    kvm_arch_irqfd_route_changed(&old, &irqfd->irq_entry)) {
>  			int ret = kvm_arch_update_irqfd_routing(
>  					irqfd->kvm, irqfd->producer->irq,
> -					irqfd->gsi, 1);
> +					irqfd->gsi);
>  			WARN_ON(ret);
>  		}
>  #endif
> 
> 
> Thanks,
> 
> 	M.
> 
> -- 
> Jazz isn't dead. It just smells funny.