Re: [RFC PATCH v2 2/7] util/vfio-helpers: Move IRQ 'type' from pci_init_irq() to open_pci()

[Alex Williamson]

On Thu, 13 Aug 2020 19:29:52 +0200
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> Once opened, we will used the same IRQ type for all our event
> notifiers, so pass the argument when we open the PCI device,
> store the IRQ type in the driver state, and directly use the
> value saved in the state each time we call qemu_vfio_pci_init_irq.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---

This feels quite a bit strange to me, a PCI device can operate in one
of several interrupt modes, or without interrupts at all.  Why would we
force a user of this interface to define the interrupt type they'll use
in advance and then not even verify if the device supports that type?
A driver might want to fall back to a different interrupt type if the
one they want is not supported.  If we want to abstract this from the
driver then we should at least have an interface separate from the
initial open function that tells us to preconfigure some specified
number of vectors.  We could then have a preference policy that would
attempt to use MSI-X, followed by MSI, followed by INTx (assuming
request is for a single vector), based on what the device supports.
Then a driver could fallback to fewer interrupts if the device does not
support, or the host system cannot provide, the desired number of
interrupts.  Thanks,

Alex


>  include/qemu/vfio-helpers.h |  5 +++--
>  block/nvme.c                |  6 +++---
>  util/vfio-helpers.c         | 13 +++++++++----
>  3 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/include/qemu/vfio-helpers.h b/include/qemu/vfio-helpers.h
> index 1f057c2b9e..728f40922b 100644
> --- a/include/qemu/vfio-helpers.h
> +++ b/include/qemu/vfio-helpers.h
> @@ -15,7 +15,8 @@
>  
>  typedef struct QEMUVFIOState QEMUVFIOState;
>  
> -QEMUVFIOState *qemu_vfio_open_pci(const char *device, Error **errp);
> +QEMUVFIOState *qemu_vfio_open_pci(const char *device, int irq_type,
> +                                  Error **errp);
>  void qemu_vfio_close(QEMUVFIOState *s);
>  int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
>                        bool temporary, uint64_t *iova_list);
> @@ -27,6 +28,6 @@ void *qemu_vfio_pci_map_bar(QEMUVFIOState *s, int index,
>  void qemu_vfio_pci_unmap_bar(QEMUVFIOState *s, int index, void *bar,
>                               uint64_t offset, uint64_t size);
>  int qemu_vfio_pci_init_irq(QEMUVFIOState *s, EventNotifier *e,
> -                           int irq_type, Error **errp);
> +                           Error **errp);
>  
>  #endif
> diff --git a/block/nvme.c b/block/nvme.c
> index a61e86a83e..21b0770c02 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -711,7 +711,8 @@ static int nvme_init(BlockDriverState *bs, const char 
> *device, int namespace,
>          return ret;
>      }
>  
> -    s->vfio = qemu_vfio_open_pci(device, errp);
> +    s->vfio = qemu_vfio_open_pci(device, VFIO_PCI_MSIX_IRQ_INDEX,
> +                                 errp);
>      if (!s->vfio) {
>          ret = -EINVAL;
>          goto out;
> @@ -784,8 +785,7 @@ static int nvme_init(BlockDriverState *bs, const char 
> *device, int namespace,
>          }
>      }
>  
> -    ret = qemu_vfio_pci_init_irq(s->vfio, &s->irq_notifier,
> -                                 VFIO_PCI_MSIX_IRQ_INDEX, errp);
> +    ret = qemu_vfio_pci_init_irq(s->vfio, &s->irq_notifier, errp);
>      if (ret) {
>          goto out;
>      }
> diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
> index 9cb9b553a5..f1196e43dc 100644
> --- a/util/vfio-helpers.c
> +++ b/util/vfio-helpers.c
> @@ -43,6 +43,8 @@ typedef struct {
>  struct QEMUVFIOState {
>      QemuMutex lock;
>  
> +    int irq_type; /* vfio index */
> +
>      /* These fields are protected by BQL */
>      int container;
>      int group;
> @@ -176,14 +178,14 @@ void qemu_vfio_pci_unmap_bar(QEMUVFIOState *s, int 
> index, void *bar,
>   * Initialize device IRQ with @irq_type and and register an event notifier.
>   */
>  int qemu_vfio_pci_init_irq(QEMUVFIOState *s, EventNotifier *e,
> -                           int irq_type, Error **errp)
> +                           Error **errp)
>  {
>      int r;
>      struct vfio_irq_set *irq_set;
>      size_t irq_set_size;
>      struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info) };
>  
> -    irq_info.index = irq_type;
> +    irq_info.index = s->irq_type;
>      if (ioctl(s->device, VFIO_DEVICE_GET_IRQ_INFO, &irq_info)) {
>          error_setg_errno(errp, errno, "Failed to get device interrupt info");
>          return -errno;
> @@ -237,6 +239,7 @@ static int qemu_vfio_pci_write_config(QEMUVFIOState *s, 
> void *buf, int size, int
>  }
>  
>  static int qemu_vfio_init_pci(QEMUVFIOState *s, const char *device,
> +                              int irq_type,
>                                Error **errp)
>  {
>      int ret;
> @@ -331,6 +334,7 @@ static int qemu_vfio_init_pci(QEMUVFIOState *s, const 
> char *device,
>          ret = -errno;
>          goto fail;
>      }
> +    s->irq_type = irq_type;
>  
>      if (device_info.num_regions < VFIO_PCI_CONFIG_REGION_INDEX) {
>          error_setg(errp, "Invalid device regions");
> @@ -423,12 +427,13 @@ static void qemu_vfio_open_common(QEMUVFIOState *s)
>  /**
>   * Open a PCI device, e.g. "0000:00:01.0".
>   */
> -QEMUVFIOState *qemu_vfio_open_pci(const char *device, Error **errp)
> +QEMUVFIOState *qemu_vfio_open_pci(const char *device, int irq_type,
> +                                  Error **errp)
>  {
>      int r;
>      QEMUVFIOState *s = g_new0(QEMUVFIOState, 1);
>  
> -    r = qemu_vfio_init_pci(s, device, errp);
> +    r = qemu_vfio_init_pci(s, device, irq_type, errp);
>      if (r) {
>          g_free(s);
>          return NULL;