vfio: check if host device supports INTx

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:

On Thu, Oct 23, 2014 at 08:26:51AM -0600, Alex Williamson wrote:
> 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:
>

This is great, works perfect and there is no need to do any qemu vfio changes
anymore. You can forget about this patch.

Thx,
Frank
 
> --- 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;
> 
>