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

[Marc Zyngier <maz@kernel.org>]

On Fri, 23 May 2025 18:48:02 +0100,
Sean Christopherson <seanjc@google.com> wrote:
> 
> 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

Sure, if you want to preserve this fossil for another round, I don't
really care. But if you are planning your grand plan for 6.17, you
could as well rebase on top of theses patches which are aimed at -rc1
(yes, they are bug fixes), and it would then be perfectly acceptable
to have this cleanup early.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.