[RFC PATCH v2 0/7] util/vfio-helpers: Add support for multiple IRQs

[RFC PATCH v2 0/7] util/vfio-helpers: Add support for multiple IRQs

[Philippe Mathieu-Daudé]

This series intends to setup the VFIO helper to allow
binding notifiers on different IRQs. Only MSIX IRQ type
considered so far.

Stefan suggested me to publish earlier to discuss.
If not too bad feedbacks I'll add some documentation in
"qemu/vfio-helpers.h" before reposting.

(NVMe block driver series will follow).

Based-on: <20200812185014.18267-1-philmd@redhat.com>
"block/nvme: Various cleanups required to use multiple queues"
https://www.mail-archive.com/qemu-devel@nongnu.org/msg729395.html
Supersedes: <20200811172845.16698-1-philmd@redhat.com>

Philippe Mathieu-Daudé (7):
  util/vfio-helpers: Store eventfd using int32_t type
  util/vfio-helpers: Move IRQ 'type' from pci_init_irq() to open_pci()
  util/vfio-helpers: Introduce 'irq_count' variable
  util/vfio-helpers: Check the device allow up to 'irq_count' IRQs
  util/vfio-helpers: Support multiple eventfd
  util/vfio-helpers: Allow to set EventNotifier to particular IRQ
  util/vfio-helpers: Allow opening device requesting for multiple IRQs

 include/qemu/vfio-helpers.h |  5 +++--
 block/nvme.c                |  9 +++++---
 util/vfio-helpers.c         | 42 ++++++++++++++++++++++++++++++-------
 util/trace-events           |  1 +
 4 files changed, 45 insertions(+), 12 deletions(-)

-- 
2.21.3

[RFC PATCH v2 1/7] util/vfio-helpers: Store eventfd using int32_t type

[Philippe Mathieu-Daudé]

Per the documentation in linux-headers/linux/vfio.h:

 VFIO_DEVICE_SET_IRQS

 * DATA_EVENTFD binds the specified ACTION to the provided __s32 eventfd.

Replace the 'int' by an 'int32_t' to match the documentation.

Fixes: 418026ca43 ("util: Introduce vfio helpers")
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 util/vfio-helpers.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
index e399e330e2..9cb9b553a5 100644
--- a/util/vfio-helpers.c
+++ b/util/vfio-helpers.c
@@ -193,7 +193,7 @@ int qemu_vfio_pci_init_irq(QEMUVFIOState *s, EventNotifier *e,
         return -EINVAL;
     }
 
-    irq_set_size = sizeof(*irq_set) + sizeof(int);
+    irq_set_size = sizeof(*irq_set) + sizeof(int32_t);
     irq_set = g_malloc0(irq_set_size);
 
     /* Get to a known IRQ state */
@@ -205,7 +205,7 @@ int qemu_vfio_pci_init_irq(QEMUVFIOState *s, EventNotifier *e,
         .count = 1,
     };
 
-    *(int *)&irq_set->data = event_notifier_get_fd(e);
+    *(int32_t *)&irq_set->data = event_notifier_get_fd(e);
     r = ioctl(s->device, VFIO_DEVICE_SET_IRQS, irq_set);
     g_free(irq_set);
     if (r) {
-- 
2.21.3

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

[Philippe Mathieu-Daudé]

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

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;

[RFC PATCH v2 3/7] util/vfio-helpers: Introduce 'irq_count' variable

[Philippe Mathieu-Daudé]

Currently this helper is restricted to a single VFIO (MSIX) IRQ.
As we will slowly make it support multiple IRQs, introduce the
'irq_count' variable which contains the total number of IRQs we
initialized the device with.

Set the variable in qemu_vfio_init_pci().
Hardcode the current single IRQ used.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 util/vfio-helpers.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
index f1196e43dc..bad60076f3 100644
--- a/util/vfio-helpers.c
+++ b/util/vfio-helpers.c
@@ -44,6 +44,7 @@ struct QEMUVFIOState {
     QemuMutex lock;
 
     int irq_type; /* vfio index */
+    size_t irq_count; /* vfio subindex (vector) */
 
     /* These fields are protected by BQL */
     int container;
@@ -195,7 +196,7 @@ int qemu_vfio_pci_init_irq(QEMUVFIOState *s, EventNotifier *e,
         return -EINVAL;
     }
 
-    irq_set_size = sizeof(*irq_set) + sizeof(int32_t);
+    irq_set_size = sizeof(*irq_set) + s->irq_count * sizeof(int32_t);
     irq_set = g_malloc0(irq_set_size);
 
     /* Get to a known IRQ state */
@@ -204,7 +205,7 @@ int qemu_vfio_pci_init_irq(QEMUVFIOState *s, EventNotifier *e,
         .flags = VFIO_IRQ_SET_DATA_EVENTFD | VFIO_IRQ_SET_ACTION_TRIGGER,
         .index = irq_info.index,
         .start = 0,
-        .count = 1,
+        .count = s->irq_count,
     };
 
     *(int32_t *)&irq_set->data = event_notifier_get_fd(e);
@@ -239,7 +240,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,
+                              int irq_type, unsigned irq_count,
                               Error **errp)
 {
     int ret;
@@ -335,6 +336,7 @@ static int qemu_vfio_init_pci(QEMUVFIOState *s, const char *device,
         goto fail;
     }
     s->irq_type = irq_type;
+    s->irq_count = irq_count;
 
     if (device_info.num_regions < VFIO_PCI_CONFIG_REGION_INDEX) {
         error_setg(errp, "Invalid device regions");
@@ -433,7 +435,7 @@ QEMUVFIOState *qemu_vfio_open_pci(const char *device, int irq_type,
     int r;
     QEMUVFIOState *s = g_new0(QEMUVFIOState, 1);
 
-    r = qemu_vfio_init_pci(s, device, irq_type, errp);
+    r = qemu_vfio_init_pci(s, device, irq_type, 1, errp);
     if (r) {
         g_free(s);
         return NULL;
-- 
2.21.3

[RFC PATCH v2 4/7] util/vfio-helpers: Check the device allow up to 'irq_count' IRQs

[Philippe Mathieu-Daudé]

As we want to use more than one single IRQ, add a check that
the device accept our request to use multiple IRQs.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 util/vfio-helpers.c | 6 ++++++
 util/trace-events   | 1 +
 2 files changed, 7 insertions(+)

diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
index bad60076f3..b81d4c70c2 100644
--- a/util/vfio-helpers.c
+++ b/util/vfio-helpers.c
@@ -335,6 +335,12 @@ static int qemu_vfio_init_pci(QEMUVFIOState *s, const char *device,
         ret = -errno;
         goto fail;
     }
+    trace_qemu_vfio_init_pci(device_info.num_irqs);
+    if (device_info.num_irqs < irq_count) {
+        error_setg(errp, "Invalid device IRQ count");
+        ret = -EINVAL;
+        goto fail;
+    }
     s->irq_type = irq_type;
     s->irq_count = irq_count;
 
diff --git a/util/trace-events b/util/trace-events
index 0ce42822eb..2e85555be3 100644
--- a/util/trace-events
+++ b/util/trace-events
@@ -83,3 +83,4 @@ qemu_vfio_new_mapping(void *s, void *host, size_t size, int index, uint64_t iova
 qemu_vfio_do_mapping(void *s, void *host, size_t size, uint64_t iova) "s %p host %p size %zu iova 0x%"PRIx64
 qemu_vfio_dma_map(void *s, void *host, size_t size, bool temporary, uint64_t *iova) "s %p host %p size %zu temporary %d iova %p"
 qemu_vfio_dma_unmap(void *s, void *host) "s %p host %p"
+qemu_vfio_init_pci(uint32_t count) "device interrupt count: %"PRIu32
-- 
2.21.3

Re: [RFC PATCH v2 4/7] util/vfio-helpers: Check the device allow up to 'irq_count' IRQs

[Alex Williamson]

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

> As we want to use more than one single IRQ, add a check that
> the device accept our request to use multiple IRQs.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  util/vfio-helpers.c | 6 ++++++
>  util/trace-events   | 1 +
>  2 files changed, 7 insertions(+)
> 
> diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
> index bad60076f3..b81d4c70c2 100644
> --- a/util/vfio-helpers.c
> +++ b/util/vfio-helpers.c
> @@ -335,6 +335,12 @@ static int qemu_vfio_init_pci(QEMUVFIOState *s, const char *device,
>          ret = -errno;
>          goto fail;
>      }
> +    trace_qemu_vfio_init_pci(device_info.num_irqs);
> +    if (device_info.num_irqs < irq_count) {
> +        error_setg(errp, "Invalid device IRQ count");
> +        ret = -EINVAL;
> +        goto fail;
> +    }

This is confusing the number of IRQ indexes (ie. IRQ types -
INTx/MSI/MSIx plus virtual interrupts like error reporting and device
request) with the number of sub-indexes available for a given type
again.  You actually need to look at VFIO_DEVICE_GET_IRQ_INFO for the
specified irq_type to see if it supports irq_count sub-indexes.

Maybe think of interrupts as a 2-dimensional array, we have:

INDEX   \  SUBINDEX
         \ 0 1 2 3 4 ... N
==========================
INTx  [0]| 
MSI   [1]|
MSI-X [2]|
...   [M]|

VFIO_DEVICE_GET_INFO only tells us essentially the last INDEX that the
device supports.  In order to learn about the number of SUBINDEXes, or
vectors, if any, that each INDEX provides, we need to look at
VFIO_DEVICE_GET_IRQ_INFO.  When we're wanting to probe support for some
number of concurrent device interrupt vectors, we need to look at the
vfio_irq_info.count value for the desired index, ie. the extent of the
entries in the row associated with our column index type.  Thanks,

Alex

>      s->irq_type = irq_type;
>      s->irq_count = irq_count;
>  
> diff --git a/util/trace-events b/util/trace-events
> index 0ce42822eb..2e85555be3 100644
> --- a/util/trace-events
> +++ b/util/trace-events
> @@ -83,3 +83,4 @@ qemu_vfio_new_mapping(void *s, void *host, size_t size, int index, uint64_t iova
>  qemu_vfio_do_mapping(void *s, void *host, size_t size, uint64_t iova) "s %p host %p size %zu iova 0x%"PRIx64
>  qemu_vfio_dma_map(void *s, void *host, size_t size, bool temporary, uint64_t *iova) "s %p host %p size %zu temporary %d iova %p"
>  qemu_vfio_dma_unmap(void *s, void *host) "s %p host %p"
> +qemu_vfio_init_pci(uint32_t count) "device interrupt count: %"PRIu32

[RFC PATCH v2 5/7] util/vfio-helpers: Support multiple eventfd

[Philippe Mathieu-Daudé]

When using multiple IRQs, we'll assign an eventfd to each IRQ.
Be ready by holding an array of eventfd file descriptors in the
instance state, so when we assign new IRQs we will still use the
previous eventfds for the already assigned IRQs.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 util/vfio-helpers.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
index b81d4c70c2..5781e4f066 100644
--- a/util/vfio-helpers.c
+++ b/util/vfio-helpers.c
@@ -45,6 +45,7 @@ struct QEMUVFIOState {
 
     int irq_type; /* vfio index */
     size_t irq_count; /* vfio subindex (vector) */
+    int32_t *eventfd;
 
     /* These fields are protected by BQL */
     int container;
@@ -195,6 +196,7 @@ int qemu_vfio_pci_init_irq(QEMUVFIOState *s, EventNotifier *e,
         error_setg(errp, "Device interrupt doesn't support eventfd");
         return -EINVAL;
     }
+    s->eventfd[0] = event_notifier_get_fd(e);
 
     irq_set_size = sizeof(*irq_set) + s->irq_count * sizeof(int32_t);
     irq_set = g_malloc0(irq_set_size);
@@ -207,8 +209,8 @@ int qemu_vfio_pci_init_irq(QEMUVFIOState *s, EventNotifier *e,
         .start = 0,
         .count = s->irq_count,
     };
+    memcpy(&irq_set->data, &s->eventfd, s->irq_count * sizeof(int32_t));
 
-    *(int32_t *)&irq_set->data = event_notifier_get_fd(e);
     r = ioctl(s->device, VFIO_DEVICE_SET_IRQS, irq_set);
     g_free(irq_set);
     if (r) {
@@ -343,6 +345,10 @@ static int qemu_vfio_init_pci(QEMUVFIOState *s, const char *device,
     }
     s->irq_type = irq_type;
     s->irq_count = irq_count;
+    s->eventfd = g_new(int32_t, irq_count);
+    for (i = 0; i < irq_count; i++) {
+        s->eventfd[i] = -1;
+    }
 
     if (device_info.num_regions < VFIO_PCI_CONFIG_REGION_INDEX) {
         error_setg(errp, "Invalid device regions");
@@ -379,6 +385,7 @@ static int qemu_vfio_init_pci(QEMUVFIOState *s, const char *device,
     }
     return 0;
 fail:
+    g_free(s->eventfd);
     close(s->group);
 fail_container:
     close(s->container);
@@ -730,6 +737,7 @@ void qemu_vfio_close(QEMUVFIOState *s)
     }
     ram_block_notifier_remove(&s->ram_notifier);
     qemu_vfio_reset(s);
+    g_free(s->eventfd);
     close(s->device);
     close(s->group);
     close(s->container);
-- 
2.21.3

[RFC PATCH v2 6/7] util/vfio-helpers: Allow to set EventNotifier to particular IRQ

[Philippe Mathieu-Daudé]

Let qemu_vfio_pci_init_irq() take an 'index' argument, so we can
set the EventNotifier to a specific IRQ.
Add a safety check. Since our helper is limited to one single IRQ
we are safe.

Our only user is the NVMe block driver, update it (also safe because
it only uses the first IRQ).

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/qemu/vfio-helpers.h |  2 +-
 block/nvme.c                |  2 +-
 util/vfio-helpers.c         | 11 +++++++++--
 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/include/qemu/vfio-helpers.h b/include/qemu/vfio-helpers.h
index 728f40922b..5c2d8ee5b3 100644
--- a/include/qemu/vfio-helpers.h
+++ b/include/qemu/vfio-helpers.h
@@ -28,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,
-                           Error **errp);
+                           int irq_index, Error **errp);
 
 #endif
diff --git a/block/nvme.c b/block/nvme.c
index 21b0770c02..a5ef571492 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -785,7 +785,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
         }
     }
 
-    ret = qemu_vfio_pci_init_irq(s->vfio, &s->irq_notifier, errp);
+    ret = qemu_vfio_pci_init_irq(s->vfio, &s->irq_notifier, INDEX_ADMIN, errp);
     if (ret) {
         goto out;
     }
diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
index 5781e4f066..7a934d1a1b 100644
--- a/util/vfio-helpers.c
+++ b/util/vfio-helpers.c
@@ -180,13 +180,20 @@ 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,
-                           Error **errp)
+                           int irq_index, 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) };
 
+    if (irq_index >= s->irq_count) {
+        error_setg(errp,
+                   "Illegal interrupt %d (device initialized for %zu in total)",
+                   irq_index, s->irq_count);
+        return -EINVAL;
+    }
+
     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");
@@ -196,7 +203,7 @@ int qemu_vfio_pci_init_irq(QEMUVFIOState *s, EventNotifier *e,
         error_setg(errp, "Device interrupt doesn't support eventfd");
         return -EINVAL;
     }
-    s->eventfd[0] = event_notifier_get_fd(e);
+    s->eventfd[irq_index] = event_notifier_get_fd(e);
 
     irq_set_size = sizeof(*irq_set) + s->irq_count * sizeof(int32_t);
     irq_set = g_malloc0(irq_set_size);
-- 
2.21.3

Re: [RFC PATCH v2 6/7] util/vfio-helpers: Allow to set EventNotifier to particular IRQ

[Alex Williamson]

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

> Let qemu_vfio_pci_init_irq() take an 'index' argument, so we can
> set the EventNotifier to a specific IRQ.
> Add a safety check. Since our helper is limited to one single IRQ
> we are safe.
> 
> Our only user is the NVMe block driver, update it (also safe because
> it only uses the first IRQ).
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/qemu/vfio-helpers.h |  2 +-
>  block/nvme.c                |  2 +-
>  util/vfio-helpers.c         | 11 +++++++++--
>  3 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/include/qemu/vfio-helpers.h b/include/qemu/vfio-helpers.h
> index 728f40922b..5c2d8ee5b3 100644
> --- a/include/qemu/vfio-helpers.h
> +++ b/include/qemu/vfio-helpers.h
> @@ -28,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,
> -                           Error **errp);
> +                           int irq_index, Error **errp);
>  
>  #endif
> diff --git a/block/nvme.c b/block/nvme.c
> index 21b0770c02..a5ef571492 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -785,7 +785,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
>          }
>      }
>  
> -    ret = qemu_vfio_pci_init_irq(s->vfio, &s->irq_notifier, errp);
> +    ret = qemu_vfio_pci_init_irq(s->vfio, &s->irq_notifier, INDEX_ADMIN, errp);
>      if (ret) {
>          goto out;
>      }
> diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
> index 5781e4f066..7a934d1a1b 100644
> --- a/util/vfio-helpers.c
> +++ b/util/vfio-helpers.c
> @@ -180,13 +180,20 @@ 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,
> -                           Error **errp)
> +                           int irq_index, 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) };
>  
> +    if (irq_index >= s->irq_count) {
> +        error_setg(errp,
> +                   "Illegal interrupt %d (device initialized for %zu in total)",
> +                   irq_index, s->irq_count);
> +        return -EINVAL;
> +    }
> +
>      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");
> @@ -196,7 +203,7 @@ int qemu_vfio_pci_init_irq(QEMUVFIOState *s, EventNotifier *e,
>          error_setg(errp, "Device interrupt doesn't support eventfd");
>          return -EINVAL;
>      }
> -    s->eventfd[0] = event_notifier_get_fd(e);
> +    s->eventfd[irq_index] = event_notifier_get_fd(e);

This can't work.  For each fd in the array provided the kernel is going
to try to get that fd and configure it as an eventfd.  For each call
until we set all eventfd index {0..irq_count}, this SET_IRQS ioctl will
fail.  I would probably make that pre-configure function I referred to
earlier and create a single spurious interrupt eventfd and configure
all of the vectors to signal that one eventfd.  You could then have
this per vector callback swap the eventfd with the caller provided one
for the given vector.

NB, I don't know if you're going to run into trouble with this scheme
with the fact that devices can behave differently based on the number
of vectors they have enabled.  You're creating an interface for a
driver, so presumably that driver knows, for example, that as soon as
vector N is enabled, signaling for event foo moves from vector 0 to
vector N.  Thanks,

Alex

>  
>      irq_set_size = sizeof(*irq_set) + s->irq_count * sizeof(int32_t);
>      irq_set = g_malloc0(irq_set_size);

[RFC PATCH v2 7/7] util/vfio-helpers: Allow opening device requesting for multiple IRQs

[Philippe Mathieu-Daudé]

Now that our helper is ready for handling multiple IRQs, let
qemu_vfio_open_pci() take an 'irq_count' argument.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/qemu/vfio-helpers.h | 2 +-
 block/nvme.c                | 5 ++++-
 util/vfio-helpers.c         | 4 ++--
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/include/qemu/vfio-helpers.h b/include/qemu/vfio-helpers.h
index 5c2d8ee5b3..4773b116df 100644
--- a/include/qemu/vfio-helpers.h
+++ b/include/qemu/vfio-helpers.h
@@ -16,7 +16,7 @@
 typedef struct QEMUVFIOState QEMUVFIOState;
 
 QEMUVFIOState *qemu_vfio_open_pci(const char *device, int irq_type,
-                                  Error **errp);
+                                  unsigned irq_count, 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);
diff --git a/block/nvme.c b/block/nvme.c
index a5ef571492..2d7aac3903 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -106,6 +106,9 @@ QEMU_BUILD_BUG_ON(offsetof(NVMeRegs, doorbells) != 0x1000);
 #define INDEX_ADMIN     0
 #define INDEX_IO(n)     (1 + n)
 
+/* This driver shares a single MSIX IRQ for the admin and I/O queues */
+#define MSIX_IRQ_COUNT  1
+
 struct BDRVNVMeState {
     AioContext *aio_context;
     QEMUVFIOState *vfio;
@@ -712,7 +715,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
     }
 
     s->vfio = qemu_vfio_open_pci(device, VFIO_PCI_MSIX_IRQ_INDEX,
-                                 errp);
+                                 MSIX_IRQ_COUNT, errp);
     if (!s->vfio) {
         ret = -EINVAL;
         goto out;
diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
index 7a934d1a1b..36fafef0d3 100644
--- a/util/vfio-helpers.c
+++ b/util/vfio-helpers.c
@@ -450,12 +450,12 @@ 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, int irq_type,
-                                  Error **errp)
+                                  unsigned irq_count, Error **errp)
 {
     int r;
     QEMUVFIOState *s = g_new0(QEMUVFIOState, 1);
 
-    r = qemu_vfio_init_pci(s, device, irq_type, 1, errp);
+    r = qemu_vfio_init_pci(s, device, irq_type, irq_count, errp);
     if (r) {
         g_free(s);
         return NULL;
-- 
2.21.3

Re: [RFC PATCH v2 7/7] util/vfio-helpers: Allow opening device requesting for multiple IRQs

[Alex Williamson]

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

> Now that our helper is ready for handling multiple IRQs, let
> qemu_vfio_open_pci() take an 'irq_count' argument.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---

As with patch 2/ tying IRQ setup with the opening of a device seems
wrong.  Get the device open, then create an interface to configure the
interrupt.  Thanks,

Alex


>  include/qemu/vfio-helpers.h | 2 +-
>  block/nvme.c                | 5 ++++-
>  util/vfio-helpers.c         | 4 ++--
>  3 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/include/qemu/vfio-helpers.h b/include/qemu/vfio-helpers.h
> index 5c2d8ee5b3..4773b116df 100644
> --- a/include/qemu/vfio-helpers.h
> +++ b/include/qemu/vfio-helpers.h
> @@ -16,7 +16,7 @@
>  typedef struct QEMUVFIOState QEMUVFIOState;
>  
>  QEMUVFIOState *qemu_vfio_open_pci(const char *device, int irq_type,
> -                                  Error **errp);
> +                                  unsigned irq_count, 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);
> diff --git a/block/nvme.c b/block/nvme.c
> index a5ef571492..2d7aac3903 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -106,6 +106,9 @@ QEMU_BUILD_BUG_ON(offsetof(NVMeRegs, doorbells) != 0x1000);
>  #define INDEX_ADMIN     0
>  #define INDEX_IO(n)     (1 + n)
>  
> +/* This driver shares a single MSIX IRQ for the admin and I/O queues */
> +#define MSIX_IRQ_COUNT  1
> +
>  struct BDRVNVMeState {
>      AioContext *aio_context;
>      QEMUVFIOState *vfio;
> @@ -712,7 +715,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
>      }
>  
>      s->vfio = qemu_vfio_open_pci(device, VFIO_PCI_MSIX_IRQ_INDEX,
> -                                 errp);
> +                                 MSIX_IRQ_COUNT, errp);
>      if (!s->vfio) {
>          ret = -EINVAL;
>          goto out;
> diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
> index 7a934d1a1b..36fafef0d3 100644
> --- a/util/vfio-helpers.c
> +++ b/util/vfio-helpers.c
> @@ -450,12 +450,12 @@ 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, int irq_type,
> -                                  Error **errp)
> +                                  unsigned irq_count, Error **errp)
>  {
>      int r;
>      QEMUVFIOState *s = g_new0(QEMUVFIOState, 1);
>  
> -    r = qemu_vfio_init_pci(s, device, irq_type, 1, errp);
> +    r = qemu_vfio_init_pci(s, device, irq_type, irq_count, errp);
>      if (r) {
>          g_free(s);
>          return NULL;