Re: [Qemu-devel] [PATCH] vfio: check if host device supports INTx

[Alex Williamson <alex.williamson@redhat.com>]

On Thu, 2014-10-23 at 10:21 +0200, Frank Blaschka wrote:
> On Wed, Oct 22, 2014 at 11:17:11AM -0600, Alex Williamson wrote:
> > On Wed, 2014-10-22 at 17:13 +0200, Frank Blaschka wrote:
> > > From: Frank Blaschka <frank.blaschka@de.ibm.com>
> > > 
> > > Let the kernel announce if INTx is available. Yes, there are platforms
> > > (e.g. s390) which do not support INTx.
> > > 
> > > Signed-off-by: Frank Blaschka <frank.blaschka@de.ibm.com>
> > > ---
> > >  hw/misc/vfio.c | 18 +++++++++++++++++-
> > >  1 file changed, 17 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
> > > index d66f3d2..3e9600b 100644
> > > --- a/hw/misc/vfio.c
> > > +++ b/hw/misc/vfio.c
> > > @@ -109,6 +109,7 @@ typedef struct VFIOVGA {
> > >  } VFIOVGA;
> > >  
> > >  typedef struct VFIOINTx {
> > > +    bool available; /* intx available */
> > >      bool pending; /* interrupt pending */
> > >      bool kvm_accel; /* set when QEMU bypass through KVM enabled */
> > >      uint8_t pin; /* which pin to pull for qemu_set_irq */
> > > @@ -554,7 +555,7 @@ static int vfio_enable_intx(VFIODevice *vdev)
> > >      struct vfio_irq_set *irq_set;
> > >      int32_t *pfd;
> > >  
> > > -    if (!pin) {
> > > +    if (!pin || !vdev->intx.available) {
> > >          return 0;
> > >      }
> > >  
> > > @@ -4032,6 +4033,21 @@ static int vfio_get_device(VFIOGroup *group, const char *name, VFIODevice *vdev)
> > >                       vdev->host.function);
> > >      }
> > >  
> > > +    irq_info.index = VFIO_PCI_INTX_IRQ_INDEX;
> > > +    ret = ioctl(vdev->fd, VFIO_DEVICE_GET_IRQ_INFO, &irq_info);
> > > +    if (ret) {
> > > +        /*
> > > +         * This can fail for an old kernel or legacy PCI dev
> > > +         * we assume intx is available
> > > +         */
> > 
> > Is this true?  It's unfortunately from an ABI stability standpoint that
> > we weren't calling this, but the ioctl should have always been there and
> > worked.  I'd rather error out here than add a fallback if this is just
> 
> ok
> 
> > paranoia.  Note that SR-IOV VFs will also report count=0 but their IRQ
> > pin register will read 0.  We do also have the option to virtualize the
> > IRQ pin register for s390 devices which would make them look the same as
> 
> sounds interesting, can you elaborate a little bit more on this?
> At what point can we hook in and modify the pci device config space?

Untested, but I think it would be something like this:

--- a/drivers/vfio/pci/vfio_pci_config.c
+++ b/drivers/vfio/pci/vfio_pci_config.c
@@ -609,6 +609,10 @@ static int __init init_pci_cap_basic_perm(struct perm_bits 
 
        /* Sometimes used by sw, just virtualize */
        p_setb(perm, PCI_INTERRUPT_LINE, (u8)ALL_VIRT, (u8)ALL_WRITE);
+
+       /* Virtualize interrupt pin to allow hiding INTx */
+       p_setb(perm, PCI_INTERRUPT_PIN, (u8)ALL_VIRT, (u8)NO_WRITE);
+
        return 0;
 }
 
@@ -1445,6 +1449,9 @@ int vfio_config_init(struct vfio_pci_device *vdev)
                *(__le16 *)&vconfig[PCI_DEVICE_ID] = cpu_to_le16(pdev->device);
        }
 
+       if (!IS_ENABLED(CONFIG_VFIO_PCI_INTX))
+               vconfig[PCI_INTERRUPT_PIN] = 0;
+
        ret = vfio_cap_init(vdev);
        if (ret)
                goto out;