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 <address@hidden>
> 
> 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 <address@hidden>
> ---
>  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);