Re: [Qemu-devel] [PATCH] vfio-pci: Make host MSI-X enable track guest

[Alex Williamson]

On Thu, 2012-12-20 at 18:36 +0200, Michael S. Tsirkin wrote:
> On Thu, Dec 20, 2012 at 09:06:41AM -0700, Alex Williamson wrote:
> > Guests typically enable MSI-X with all of the vectors in the MSI-X
> > vector table masked.  Only when the vector is enabled does the vector
> > get unmasked, resulting in a vector_use callback.  These two points,
> > enable and unmask, correspond to pci_enable_msix() and request_irq()
> > for Linux guests.  Some drivers rely on VF/PF or PF/fw communication
> > channels that expect the physical state of the device to match the
> > guest visible state of the device.  They don't appreciate lazily
> > enabling MSI-X on the physical device.
> > 
> > To solve this, enable MSI-X with a single vector when the MSI-X
> > capability is enabled and immediate disable the vector.  This leaves
> > the physical device in exactly the same state between host and guest.
> > Furthermore, the brief gap where we enable vector 0, it fires into
> > userspace, not KVM, so the guest doesn't get spurious interrupts.
> > Ideally we could call VFIO_DEVICE_SET_IRQS with the right parameters
> > to enable MSI-X with zero vectors, but this will currently return an
> > error as the Linux MSI-X interfaces do not allow it.
> > 
> > Cc: address@hidden
> > Signed-off-by: Alex Williamson <address@hidden>
> 
> Do you need an interface for this?  Can you do low-level pci config
> access instead?  I imagine you would then enable MSIX and mask all
> vectors at the same time.
> 
> No?

I really don't like the idea of enabling MSI-X directly through config
space.  We're just asking for ownership conflicts doing that.  In fact,
vfio prevents MSI/X from being enabled through config space since those
are controlled through ioctl.  It also prevents access to the MSI-X
vector table since userspace has no business reading or modifying it.
Thanks,

Alex

> > ---
> >  hw/vfio_pci.c |   31 +++++++++++++++++++++++++++----
> >  1 file changed, 27 insertions(+), 4 deletions(-)
> > 
> > VFIO makes this a bit cleaner, so I think this is both the stable and
> > final fix here.
> > 
> > diff --git a/hw/vfio_pci.c b/hw/vfio_pci.c
> > index 7c27834..5178ccc 100644
> > --- a/hw/vfio_pci.c
> > +++ b/hw/vfio_pci.c
> > @@ -561,8 +561,9 @@ static int vfio_enable_vectors(VFIODevice *vdev, bool 
> > msix)
> >      return ret;
> >  }
> >  
> > -static int vfio_msix_vector_use(PCIDevice *pdev,
> > -                                unsigned int nr, MSIMessage msg)
> > +static int vfio_msix_vector_do_use(PCIDevice *pdev, unsigned int nr,
> > +                                   MSIMessage msg, bool try_kvm,
> > +                                   IOHandler *handler)
> >  {
> >      VFIODevice *vdev = DO_UPCAST(VFIODevice, pdev, pdev);
> >      VFIOMSIVector *vector;
> > @@ -586,7 +587,7 @@ static int vfio_msix_vector_use(PCIDevice *pdev,
> >       * Attempt to enable route through KVM irqchip,
> >       * default to userspace handling if unavailable.
> >       */
> > -    vector->virq = kvm_irqchip_add_msi_route(kvm_state, msg);
> > +    vector->virq = try_kvm ? kvm_irqchip_add_msi_route(kvm_state, msg) : 
> > -1;
> >      if (vector->virq < 0 ||
> >          kvm_irqchip_add_irqfd_notifier(kvm_state, &vector->interrupt,
> >                                         vector->virq) < 0) {
> > @@ -595,7 +596,7 @@ static int vfio_msix_vector_use(PCIDevice *pdev,
> >              vector->virq = -1;
> >          }
> >          qemu_set_fd_handler(event_notifier_get_fd(&vector->interrupt),
> > -                            vfio_msi_interrupt, NULL, vector);
> > +                            handler, NULL, vector);
> >      }
> >  
> >      /*
> > @@ -638,6 +639,12 @@ static int vfio_msix_vector_use(PCIDevice *pdev,
> >      return 0;
> >  }
> >  
> > +static int vfio_msix_vector_use(PCIDevice *pdev,
> > +                                unsigned int nr, MSIMessage msg)
> > +{
> > +    return vfio_msix_vector_do_use(pdev, nr, msg, true, 
> > vfio_msi_interrupt);
> > +}
> > +
> >  static void vfio_msix_vector_release(PCIDevice *pdev, unsigned int nr)
> >  {
> >      VFIODevice *vdev = DO_UPCAST(VFIODevice, pdev, pdev);
> > @@ -696,6 +703,22 @@ static void vfio_enable_msix(VFIODevice *vdev)
> >  
> >      vdev->interrupt = VFIO_INT_MSIX;
> >  
> > +    /*
> > +     * Some communication channels between VF & PF or PF & fw rely on the
> > +     * physical state of the device and expect that enabling MSI-X from the
> > +     * guest enables the same on the host.  When our guest is Linux, the
> > +     * guest driver call to pci_enable_msix() sets the enabling bit in the
> > +     * MSI-X capability, but leaves the vector table masked.  We therefore
> > +     * can't rely on a vector_use callback (from request_irq() in the 
> > guest)
> > +     * to switch the physical device into MSI-X mode because that may come 
> > a
> > +     * long time after pci_enable_msix().  This code enables vector 0 with
> > +     * triggering to userspace, then immediately release the vector, 
> > leaving
> > +     * the physical device with no vectors enabled, but MSI-X enabled, just
> > +     * like the guest view.
> > +     */
> > +    vfio_msix_vector_do_use(&vdev->pdev, 0, (MSIMessage) { 0, 0 }, false, 
> > NULL);
> > +    vfio_msix_vector_release(&vdev->pdev, 0);
> > +
> >      if (msix_set_vector_notifiers(&vdev->pdev, vfio_msix_vector_use,
> >                                    vfio_msix_vector_release)) {
> >          error_report("vfio: msix_set_vector_notifiers failed\n");