RE: [PATCH v3 7/9] vfio: add infrastructure to commit the deferred kvm routing

[Longpeng (Mike, Cloud Infrastructure Service Product Dept.)]

> -----Original Message-----
> From: Alex Williamson [mailto:alex.william...@redhat.com]
> Sent: Saturday, October 2, 2021 7:05 AM
> To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
> <longpe...@huawei.com>
> Cc: phi...@redhat.com; pbonz...@redhat.com; marcel.apfelb...@gmail.com;
> m...@redhat.com; qemu-devel@nongnu.org; Gonglei (Arei)
> <arei.gong...@huawei.com>; chenjiashang <chenjiash...@huawei.com>
> Subject: Re: [PATCH v3 7/9] vfio: add infrastructure to commit the deferred 
> kvm
> routing
> 
> On Tue, 21 Sep 2021 07:02:00 +0800
> "Longpeng(Mike)" <longpe...@huawei.com> wrote:
> 
> > 'defer_kvm_irq_routing' indicates whether we should defer to commit
> > the kvm routing.
> >
> > Signed-off-by: Longpeng(Mike) <longpe...@huawei.com>
> > ---
> >  hw/vfio/pci.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
> >  hw/vfio/pci.h |  1 +
> >  2 files changed, 43 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > index 8e97ca93cf..8fe238b11d 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -423,12 +423,24 @@ static void vfio_add_kvm_msi_virq(VFIOPCIDevice *vdev,
> VFIOMSIVector *vector,
> >          return;
> >      }
> >
> > -    virq = kvm_irqchip_add_msi_route(kvm_state, vector_n, &vdev->pdev);
> > +    virq = kvm_irqchip_add_deferred_msi_route(kvm_state, vector_n,
> &vdev->pdev);
> >      if (virq < 0) {
> >          event_notifier_cleanup(&vector->kvm_interrupt);
> >          return;
> >      }
> >
> > +    if (vdev->defer_kvm_irq_routing) {
> > +        /*
> > +         * Hold the allocated virq in vector->virq temporarily, will
> > +         * reset it to -1 when we fail to add the corresponding irqfd
> > +         * in vfio_commit_kvm_msi_virq().
> 
> s/when/if/
> 

OK, thanks.

> > +         */
> > +        vector->virq = virq;
> 
> Do we need to make this unique to the deferred case or could we use
> vector->virq directly and fill it with -1 on all error paths like we do
> on a failure in vfio_commit_kvm_msi_virq()?
> 

OK, I will use vector->irq directly, it looks neater.

> 
> > +        return;
> > +    }
> > +
> > +    kvm_irqchip_commit_routes(kvm_state);
> > +
> >      if (kvm_irqchip_add_irqfd_notifier_gsi(kvm_state,
> &vector->kvm_interrupt,
> >                                         NULL, virq) < 0) {
> >          kvm_irqchip_release_virq(kvm_state, virq);
> > @@ -567,6 +579,35 @@ static void vfio_msix_vector_release(PCIDevice *pdev,
> unsigned int nr)
> >      }
> >  }
> >
> > +/* TODO: invoked when enclabe msi/msix vectors */
> 
> "enclabe"?  Is this meant to be "enable"?
> 

Yes, it's a typo.

> > +static __attribute__((unused)) void vfio_commit_kvm_msi_virq(VFIOPCIDevice
> *vdev)
> 
> I'd move this function, if not this entire change, to patch 9 rather
> than adding these attributes for an unused function.  Thanks,
> 

OK. I think I should merge this patch into patch 9 entirely if we decide to
move this function.

> Alex
> 
> > +{
> > +    int i;
> > +    VFIOMSIVector *vector;
> > +
> > +    if (!vdev->defer_kvm_irq_routing || !vdev->nr_vectors) {
> > +        return;
> > +    }
> > +
> > +    kvm_irqchip_commit_routes(kvm_state);
> > +
> > +    for (i = 0; i < vdev->nr_vectors; i++) {
> > +        vector = &vdev->msi_vectors[i];
> > +
> > +        if (!vector->use || vector->virq < 0) {
> > +            continue;
> > +        }
> > +
> > +        if (kvm_irqchip_add_irqfd_notifier_gsi(kvm_state,
> > +                                               &vector->kvm_interrupt,
> > +                                               NULL, vector->virq) < 0) {
> > +            kvm_irqchip_release_virq(kvm_state, vector->virq);
> > +            event_notifier_cleanup(&vector->kvm_interrupt);
> > +            vector->virq = -1;
> > +        }
> > +    }
> > +}
> > +
> >  static void vfio_msix_enable(VFIOPCIDevice *vdev)
> >  {
> >      PCIDevice *pdev = &vdev->pdev;
> > diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> > index 64777516d1..d3c5177d37 100644
> > --- a/hw/vfio/pci.h
> > +++ b/hw/vfio/pci.h
> > @@ -171,6 +171,7 @@ struct VFIOPCIDevice {
> >      bool no_kvm_ioeventfd;
> >      bool no_vfio_ioeventfd;
> >      bool enable_ramfb;
> > +    bool defer_kvm_irq_routing;
> >      VFIODisplay *dpy;
> >      Notifier irqchip_change_notifier;
> >  };