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

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

[Frank Blaschka]

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
+         */
+        vdev->intx.available = true;
+        ret = 0;
+    } else if (irq_info.count == 0) {
+        vdev->intx.available = false;
+    } else {
+        vdev->intx.available = true;
+    }
+
 error:
     if (ret) {
         QLIST_REMOVE(vdev, next);
-- 
1.8.5.5

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

[Alex Williamson]

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
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
an SR-IOV VF from an INTx perspective to the user.  That may be the more
consistent option so that the VFIO IRQ INFO aligns with config space.
Thanks,

Alex

> +        vdev->intx.available = true;
> +        ret = 0;
> +    } else if (irq_info.count == 0) {
> +        vdev->intx.available = false;
> +    } else {
> +        vdev->intx.available = true;
> +    }
> +
>  error:
>      if (ret) {
>          QLIST_REMOVE(vdev, next);

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

[Frank Blaschka]

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?

> an SR-IOV VF from an INTx perspective to the user.  That may be the more
> consistent option so that the VFIO IRQ INFO aligns with config space.
> Thanks,
> 
> Alex
> 
> > +        vdev->intx.available = true;
> > +        ret = 0;
> > +    } else if (irq_info.count == 0) {
> > +        vdev->intx.available = false;
> > +    } else {
> > +        vdev->intx.available = true;
> > +    }
> > +
> >  error:
> >      if (ret) {
> >          QLIST_REMOVE(vdev, next);
> 
> 
> 
> 

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

[Alex Williamson]

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;

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

[Frank Blaschka]

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;
> 
>