[PATCH v6 0/4] util/vfio-helpers: Add support for multiple IRQs

[PATCH v6 0/4] 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.

For the NVMe use case, we only care about MSIX interrupts.
To not disrupt other users, introduce the qemu_vfio_pci_init_msix_irqs
function to initialize multiple MSIX IRQs and attach eventfd to
them.

Since RFC v5:
- addressed Fam review comment (return EINVAL)
- addressed Fam R-b tags
- no more RFC :)

Since RFC v4:
- addressed Alex review comment:
  check ioctl(VFIO_DEVICE_SET_IRQS) return value

Since RFC v3:
- addressed Alex and Stefan review comments

Since RFC v2:
- new patch to report vfio-helpers is not supported on AA64/POWER

(NVMe block driver series will follow).

Based-on: <20200908115322.325832-1-kwolf@redhat.com>
(Block layer pending pull request)

Philippe Mathieu-Daudé (4):
  util/vfio-helpers: Improve reporting unsupported IOMMU type
  util/vfio-helpers: Report error when IOMMU page size is not supported
  util/vfio-helpers: Introduce qemu_vfio_pci_init_msix_irqs()
  block/nvme: Use qemu_vfio_pci_init_msix_irqs() to initialize our IRQ

 include/qemu/vfio-helpers.h |  6 ++-
 block/nvme.c                |  9 +++-
 util/vfio-helpers.c         | 87 ++++++++++++++++++++++++++++++++++++-
 3 files changed, 97 insertions(+), 5 deletions(-)

-- 
2.26.2

[PATCH v6 1/4] util/vfio-helpers: Improve reporting unsupported IOMMU type

[Philippe Mathieu-Daudé]

Change the confuse "VFIO IOMMU check failed" error message by
the explicit "VFIO IOMMU Type1 is not supported" once.

Example on POWER:

 $ qemu-system-ppc64 -drive if=none,id=nvme0,file=nvme://0001:01:00.0/1,format=raw
 qemu-system-ppc64: -drive if=none,id=nvme0,file=nvme://0001:01:00.0/1,format=raw: VFIO IOMMU Type1 is not supported

Suggested-by: Alex Williamson <alex.williamson@redhat.com>
Reviewed-by: Fam Zheng <fam@euphon.net>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 util/vfio-helpers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
index 583bdfb36fc..55b4107ce69 100644
--- a/util/vfio-helpers.c
+++ b/util/vfio-helpers.c
@@ -261,7 +261,7 @@ static int qemu_vfio_init_pci(QEMUVFIOState *s, const char *device,
     }
 
     if (!ioctl(s->container, VFIO_CHECK_EXTENSION, VFIO_TYPE1_IOMMU)) {
-        error_setg_errno(errp, errno, "VFIO IOMMU check failed");
+        error_setg_errno(errp, errno, "VFIO IOMMU Type1 is not supported");
         ret = -EINVAL;
         goto fail_container;
     }
-- 
2.26.2

[PATCH v6 2/4] util/vfio-helpers: Report error when IOMMU page size is not supported

[Philippe Mathieu-Daudé]

This driver uses the host page size to align its memory regions,
but this size is not always compatible with the IOMMU. Add a
check if the size matches, and bails out with listing the sizes
the IOMMU supports.

Example on Aarch64:

 $ qemu-system-aarch64 -M virt -drive if=none,id=nvme0,file=nvme://0006:90:00.0/1,format=raw
 qemu-system-aarch64: -drive if=none,id=nvme0,file=nvme://0006:90:00.0/1,format=raw: Unsupported IOMMU page size: 4 KiB
 Available page size:
  64 KiB
  512 MiB

Suggested-by: Alex Williamson <alex.williamson@redhat.com>
Reviewed-by: Fam Zheng <fam@euphon.net>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 util/vfio-helpers.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
index 55b4107ce69..4cc5697dcb2 100644
--- a/util/vfio-helpers.c
+++ b/util/vfio-helpers.c
@@ -11,6 +11,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/cutils.h"
 #include <sys/ioctl.h>
 #include <linux/vfio.h>
 #include "qapi/error.h"
@@ -316,6 +317,25 @@ static int qemu_vfio_init_pci(QEMUVFIOState *s, const char *device,
         ret = -errno;
         goto fail;
     }
+    if (!(iommu_info.flags & VFIO_IOMMU_INFO_PGSIZES)) {
+        error_setg(errp, "Failed to get IOMMU page size info");
+        ret = -EINVAL;
+        goto fail;
+    }
+    if (!extract64(iommu_info.iova_pgsizes,
+                   ctz64(qemu_real_host_page_size), 1)) {
+        g_autofree char *host_page_size = size_to_str(qemu_real_host_page_size);
+        error_setg(errp, "Unsupported IOMMU page size: %s", host_page_size);
+        error_append_hint(errp, "Available page size:\n");
+        for (int i = 0; i < 64; i++) {
+            if (extract64(iommu_info.iova_pgsizes, i, 1)) {
+                g_autofree char *iova_pgsizes = size_to_str(1UL << i);
+                error_append_hint(errp, " %s\n", iova_pgsizes);
+            }
+        }
+        ret = -EINVAL;
+        goto fail;
+    }
 
     s->device = ioctl(s->group, VFIO_GROUP_GET_DEVICE_FD, device);
 
-- 
2.26.2

[PATCH v6 3/4] util/vfio-helpers: Introduce qemu_vfio_pci_init_msix_irqs()

[Philippe Mathieu-Daudé]

qemu_vfio_pci_init_irq() allows us to initialize any type of IRQ,
but only one. Introduce qemu_vfio_pci_init_msix_irqs() which is
specific to MSIX IRQ type, and allow us to use multiple IRQs
(thus passing multiple eventfd notifiers).

Reviewed-by: Fam Zheng <fam@euphon.net>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/qemu/vfio-helpers.h |  6 +++-
 util/vfio-helpers.c         | 65 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 69 insertions(+), 2 deletions(-)

diff --git a/include/qemu/vfio-helpers.h b/include/qemu/vfio-helpers.h
index 1f057c2b9e4..092b7b243f9 100644
--- a/include/qemu/vfio-helpers.h
+++ b/include/qemu/vfio-helpers.h
@@ -1,11 +1,13 @@
 /*
  * QEMU VFIO helpers
  *
- * Copyright 2016 - 2018 Red Hat, Inc.
+ * Copyright 2016 - 2020 Red Hat, Inc.
  *
  * Authors:
  *   Fam Zheng <famz@redhat.com>
+ *   Philippe Mathieu-Daudé <philmd@redhat.com>
  *
+ * SPDX-License-Identifier: GPL-2.0-or-later
  * This work is licensed under the terms of the GNU GPL, version 2 or later.
  * See the COPYING file in the top-level directory.
  */
@@ -28,5 +30,7 @@ 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);
+int qemu_vfio_pci_init_msix_irqs(QEMUVFIOState *s, EventNotifier *e,
+                                 unsigned *irq_count, Error **errp);
 
 #endif
diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
index 4cc5697dcb2..3b1b81caf8b 100644
--- a/util/vfio-helpers.c
+++ b/util/vfio-helpers.c
@@ -1,11 +1,13 @@
 /*
  * VFIO utility
  *
- * Copyright 2016 - 2018 Red Hat, Inc.
+ * Copyright 2016 - 2020 Red Hat, Inc.
  *
  * Authors:
  *   Fam Zheng <famz@redhat.com>
+ *   Philippe Mathieu-Daudé <philmd@redhat.com>
  *
+ * SPDX-License-Identifier: GPL-2.0-or-later
  * This work is licensed under the terms of the GNU GPL, version 2 or later.
  * See the COPYING file in the top-level directory.
  */
@@ -216,6 +218,67 @@ int qemu_vfio_pci_init_irq(QEMUVFIOState *s, EventNotifier *e,
     return 0;
 }
 
+/**
+ * Initialize device MSIX IRQs and register event notifiers.
+ * @irq_count: pointer to number of MSIX IRQs to initialize
+ * @notifier: Array of @irq_count notifiers (each corresponding to a MSIX IRQ)
+
+ * If the number of IRQs requested exceeds the available on the device,
+ * store the number of available IRQs in @irq_count and return -EOVERFLOW.
+ */
+int qemu_vfio_pci_init_msix_irqs(QEMUVFIOState *s, EventNotifier *notifier,
+                                 unsigned *irq_count, Error **errp)
+{
+    int r;
+    size_t irq_set_size;
+    struct vfio_irq_set *irq_set;
+    struct vfio_irq_info irq_info = {
+        .argsz = sizeof(irq_info),
+        .index = VFIO_PCI_MSIX_IRQ_INDEX
+    };
+
+    if (ioctl(s->device, VFIO_DEVICE_GET_IRQ_INFO, &irq_info)) {
+        error_setg_errno(errp, errno, "Failed to get device interrupt info");
+        return -errno;
+    }
+    if (irq_info.count < *irq_count) {
+        error_setg(errp, "Not enough device interrupts available");
+        *irq_count = irq_info.count;
+        return -EOVERFLOW;
+    }
+    if (!(irq_info.flags & VFIO_IRQ_INFO_EVENTFD)) {
+        error_setg(errp, "Device interrupt doesn't support eventfd");
+        return -EINVAL;
+    }
+
+    irq_set_size = sizeof(*irq_set) + *irq_count * sizeof(int32_t);
+    irq_set = g_malloc0(irq_set_size);
+
+    /* Get to a known IRQ state */
+    *irq_set = (struct vfio_irq_set) {
+        .argsz = irq_set_size,
+        .flags = VFIO_IRQ_SET_DATA_EVENTFD | VFIO_IRQ_SET_ACTION_TRIGGER,
+        .index = irq_info.index,
+        .start = 0,
+        .count = *irq_count,
+    };
+
+    for (unsigned i = 0; i < *irq_count; i++) {
+        ((int32_t *)&irq_set->data)[i] = event_notifier_get_fd(&notifier[i]);
+    }
+    r = ioctl(s->device, VFIO_DEVICE_SET_IRQS, irq_set);
+    g_free(irq_set);
+    if (r <= 0) {
+        error_setg_errno(errp, errno, "Failed to setup device interrupts");
+        return -errno;
+    } else if (r < *irq_count) {
+        error_setg(errp, "Not enough device interrupts available");
+        *irq_count = r;
+        return -EOVERFLOW;
+    }
+    return 0;
+}
+
 static int qemu_vfio_pci_read_config(QEMUVFIOState *s, void *buf,
                                      int size, int ofs)
 {
-- 
2.26.2

[PATCH v6 4/4] block/nvme: Use qemu_vfio_pci_init_msix_irqs() to initialize our IRQ

[Philippe Mathieu-Daudé]

Instead of initializing one MSIX IRQ with the generic
qemu_vfio_pci_init_irq() function, use the MSIX specific one which
will allow us to use multiple IRQs. For now we provide an array of
a single IRQ.

Reviewed-by: Fam Zheng <fam@euphon.net>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index f4f27b6da7d..e6dac027f72 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -694,6 +694,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
     uint64_t timeout_ms;
     uint64_t deadline, now;
     Error *local_err = NULL;
+    unsigned irq_count = MSIX_IRQ_COUNT;
 
     qemu_co_mutex_init(&s->dma_map_lock);
     qemu_co_queue_init(&s->dma_flush_queue);
@@ -779,9 +780,13 @@ 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_msix_irqs(s->vfio, s->irq_notifier,
+                                       &irq_count, errp);
     if (ret) {
+        if (ret == -EOVERFLOW) {
+            error_append_hint(errp, "%u IRQs requested but only %u available\n",
+                              MSIX_IRQ_COUNT, irq_count);
+        }
         goto out;
     }
     aio_set_event_notifier(bdrv_get_aio_context(bs),
-- 
2.26.2

Re: [PATCH v6 3/4] util/vfio-helpers: Introduce qemu_vfio_pci_init_msix_irqs()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2899 bytes --]

On Wed, Sep 09, 2020 at 04:23:53PM +0200, Philippe Mathieu-Daudé wrote:
> +/**
> + * Initialize device MSIX IRQs and register event notifiers.
> + * @irq_count: pointer to number of MSIX IRQs to initialize
> + * @notifier: Array of @irq_count notifiers (each corresponding to a MSIX IRQ)
> +
> + * If the number of IRQs requested exceeds the available on the device,
> + * store the number of available IRQs in @irq_count and return -EOVERFLOW.
> + */
> +int qemu_vfio_pci_init_msix_irqs(QEMUVFIOState *s, EventNotifier *notifier,
> +                                 unsigned *irq_count, Error **errp)
> +{
> +    int r;
> +    size_t irq_set_size;
> +    struct vfio_irq_set *irq_set;
> +    struct vfio_irq_info irq_info = {
> +        .argsz = sizeof(irq_info),
> +        .index = VFIO_PCI_MSIX_IRQ_INDEX
> +    };
> +
> +    if (ioctl(s->device, VFIO_DEVICE_GET_IRQ_INFO, &irq_info)) {
> +        error_setg_errno(errp, errno, "Failed to get device interrupt info");
> +        return -errno;
> +    }
> +    if (irq_info.count < *irq_count) {
> +        error_setg(errp, "Not enough device interrupts available");
> +        *irq_count = irq_info.count;
> +        return -EOVERFLOW;
> +    }
> +    if (!(irq_info.flags & VFIO_IRQ_INFO_EVENTFD)) {
> +        error_setg(errp, "Device interrupt doesn't support eventfd");
> +        return -EINVAL;
> +    }
> +
> +    irq_set_size = sizeof(*irq_set) + *irq_count * sizeof(int32_t);
> +    irq_set = g_malloc0(irq_set_size);
> +
> +    /* Get to a known IRQ state */
> +    *irq_set = (struct vfio_irq_set) {
> +        .argsz = irq_set_size,
> +        .flags = VFIO_IRQ_SET_DATA_EVENTFD | VFIO_IRQ_SET_ACTION_TRIGGER,
> +        .index = irq_info.index,
> +        .start = 0,
> +        .count = *irq_count,
> +    };
> +
> +    for (unsigned i = 0; i < *irq_count; i++) {
> +        ((int32_t *)&irq_set->data)[i] = event_notifier_get_fd(&notifier[i]);
> +    }
> +    r = ioctl(s->device, VFIO_DEVICE_SET_IRQS, irq_set);
> +    g_free(irq_set);
> +    if (r <= 0) {
> +        error_setg_errno(errp, errno, "Failed to setup device interrupts");
> +        return -errno;
> +    } else if (r < *irq_count) {
> +        error_setg(errp, "Not enough device interrupts available");
> +        *irq_count = r;
> +        return -EOVERFLOW;
> +    }

EOVERFLOW can occur in two cases: VFIO_DEVICE_GET_IRQ_INFO and
VFIO_DEVICE_SET_IRQS.

If it happens in the second case the notifier[] array has been
registered successfully.

The caller has no way of distinguishing the two cases. Therefore the
caller doesn't know if the eventfds will be used by the kernel after
EOVERFLOW.

If the second case can ever happen then this function should probably
call VFIO_DEVICE_SET_IRQS again with VFIO_IRQ_SET_DATA_NONE to
unregister the eventfds before returning EOVERFLOW.

STefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v6 1/4] util/vfio-helpers: Improve reporting unsupported IOMMU type

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 743 bytes --]

On Wed, Sep 09, 2020 at 04:23:51PM +0200, Philippe Mathieu-Daudé wrote:
> Change the confuse "VFIO IOMMU check failed" error message by
> the explicit "VFIO IOMMU Type1 is not supported" once.
> 
> Example on POWER:
> 
>  $ qemu-system-ppc64 -drive if=none,id=nvme0,file=nvme://0001:01:00.0/1,format=raw
>  qemu-system-ppc64: -drive if=none,id=nvme0,file=nvme://0001:01:00.0/1,format=raw: VFIO IOMMU Type1 is not supported
> 
> Suggested-by: Alex Williamson <alex.williamson@redhat.com>
> Reviewed-by: Fam Zheng <fam@euphon.net>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  util/vfio-helpers.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v6 2/4] util/vfio-helpers: Report error when IOMMU page size is not supported

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 913 bytes --]

On Wed, Sep 09, 2020 at 04:23:52PM +0200, Philippe Mathieu-Daudé wrote:
> This driver uses the host page size to align its memory regions,
> but this size is not always compatible with the IOMMU. Add a
> check if the size matches, and bails out with listing the sizes
> the IOMMU supports.
> 
> Example on Aarch64:
> 
>  $ qemu-system-aarch64 -M virt -drive if=none,id=nvme0,file=nvme://0006:90:00.0/1,format=raw
>  qemu-system-aarch64: -drive if=none,id=nvme0,file=nvme://0006:90:00.0/1,format=raw: Unsupported IOMMU page size: 4 KiB
>  Available page size:
>   64 KiB
>   512 MiB
> 
> Suggested-by: Alex Williamson <alex.williamson@redhat.com>
> Reviewed-by: Fam Zheng <fam@euphon.net>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  util/vfio-helpers.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v6 4/4] block/nvme: Use qemu_vfio_pci_init_msix_irqs() to initialize our IRQ

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 539 bytes --]

On Wed, Sep 09, 2020 at 04:23:54PM +0200, Philippe Mathieu-Daudé wrote:
> Instead of initializing one MSIX IRQ with the generic
> qemu_vfio_pci_init_irq() function, use the MSIX specific one which
> will allow us to use multiple IRQs. For now we provide an array of
> a single IRQ.
> 
> Reviewed-by: Fam Zheng <fam@euphon.net>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v6 3/4] util/vfio-helpers: Introduce qemu_vfio_pci_init_msix_irqs()

[Philippe Mathieu-Daudé]

Hi Stefan, Alex.

On 9/10/20 12:44 PM, Stefan Hajnoczi wrote:
> On Wed, Sep 09, 2020 at 04:23:53PM +0200, Philippe Mathieu-Daudé wrote:
>> +/**
>> + * Initialize device MSIX IRQs and register event notifiers.
>> + * @irq_count: pointer to number of MSIX IRQs to initialize
>> + * @notifier: Array of @irq_count notifiers (each corresponding to a MSIX IRQ)
>> +
>> + * If the number of IRQs requested exceeds the available on the device,
>> + * store the number of available IRQs in @irq_count and return -EOVERFLOW.
>> + */
>> +int qemu_vfio_pci_init_msix_irqs(QEMUVFIOState *s, EventNotifier *notifier,
>> +                                 unsigned *irq_count, Error **errp)
>> +{
>> +    int r;
>> +    size_t irq_set_size;
>> +    struct vfio_irq_set *irq_set;
>> +    struct vfio_irq_info irq_info = {
>> +        .argsz = sizeof(irq_info),
>> +        .index = VFIO_PCI_MSIX_IRQ_INDEX
>> +    };
>> +
>> +    if (ioctl(s->device, VFIO_DEVICE_GET_IRQ_INFO, &irq_info)) {
>> +        error_setg_errno(errp, errno, "Failed to get device interrupt info");
>> +        return -errno;
>> +    }
>> +    if (irq_info.count < *irq_count) {
>> +        error_setg(errp, "Not enough device interrupts available");
>> +        *irq_count = irq_info.count;
>> +        return -EOVERFLOW;
>> +    }
>> +    if (!(irq_info.flags & VFIO_IRQ_INFO_EVENTFD)) {
>> +        error_setg(errp, "Device interrupt doesn't support eventfd");
>> +        return -EINVAL;
>> +    }
>> +
>> +    irq_set_size = sizeof(*irq_set) + *irq_count * sizeof(int32_t);
>> +    irq_set = g_malloc0(irq_set_size);
>> +
>> +    /* Get to a known IRQ state */
>> +    *irq_set = (struct vfio_irq_set) {
>> +        .argsz = irq_set_size,
>> +        .flags = VFIO_IRQ_SET_DATA_EVENTFD | VFIO_IRQ_SET_ACTION_TRIGGER,
>> +        .index = irq_info.index,
>> +        .start = 0,
>> +        .count = *irq_count,
>> +    };
>> +
>> +    for (unsigned i = 0; i < *irq_count; i++) {
>> +        ((int32_t *)&irq_set->data)[i] = event_notifier_get_fd(&notifier[i]);
>> +    }
>> +    r = ioctl(s->device, VFIO_DEVICE_SET_IRQS, irq_set);
>> +    g_free(irq_set);
>> +    if (r <= 0) {
>> +        error_setg_errno(errp, errno, "Failed to setup device interrupts");
>> +        return -errno;
>> +    } else if (r < *irq_count) {
>> +        error_setg(errp, "Not enough device interrupts available");
>> +        *irq_count = r;
>> +        return -EOVERFLOW;
>> +    }
> 
> EOVERFLOW can occur in two cases: VFIO_DEVICE_GET_IRQ_INFO and
> VFIO_DEVICE_SET_IRQS.

Yes.

> 
> If it happens in the second case the notifier[] array has been
> registered successfully.

No, I don't think so:

vfio_pci_set_msi_trigger() register the notifier only if
vfio_msi_enable() succeeded (returned 0). If vfio_msi_enable()
failed it returns the number of vectors available but do
not register the notifiers.

Alex, do you confirm?

> 
> The caller has no way of distinguishing the two cases. Therefore the
> caller doesn't know if the eventfds will be used by the kernel after
> EOVERFLOW.
> 
> If the second case can ever happen then this function should probably
> call VFIO_DEVICE_SET_IRQS again with VFIO_IRQ_SET_DATA_NONE to
> unregister the eventfds before returning EOVERFLOW.
> 
> STefan
> 

Re: [PATCH v6 3/4] util/vfio-helpers: Introduce qemu_vfio_pci_init_msix_irqs()

[Alex Williamson]

On Thu, 10 Sep 2020 17:29:25 +0200
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> Hi Stefan, Alex.
> 
> On 9/10/20 12:44 PM, Stefan Hajnoczi wrote:
> > On Wed, Sep 09, 2020 at 04:23:53PM +0200, Philippe Mathieu-Daudé wrote:  
> >> +/**
> >> + * Initialize device MSIX IRQs and register event notifiers.
> >> + * @irq_count: pointer to number of MSIX IRQs to initialize
> >> + * @notifier: Array of @irq_count notifiers (each corresponding to a MSIX IRQ)
> >> +
> >> + * If the number of IRQs requested exceeds the available on the device,
> >> + * store the number of available IRQs in @irq_count and return -EOVERFLOW.
> >> + */
> >> +int qemu_vfio_pci_init_msix_irqs(QEMUVFIOState *s, EventNotifier *notifier,
> >> +                                 unsigned *irq_count, Error **errp)
> >> +{
> >> +    int r;
> >> +    size_t irq_set_size;
> >> +    struct vfio_irq_set *irq_set;
> >> +    struct vfio_irq_info irq_info = {
> >> +        .argsz = sizeof(irq_info),
> >> +        .index = VFIO_PCI_MSIX_IRQ_INDEX
> >> +    };
> >> +
> >> +    if (ioctl(s->device, VFIO_DEVICE_GET_IRQ_INFO, &irq_info)) {
> >> +        error_setg_errno(errp, errno, "Failed to get device interrupt info");
> >> +        return -errno;
> >> +    }
> >> +    if (irq_info.count < *irq_count) {
> >> +        error_setg(errp, "Not enough device interrupts available");
> >> +        *irq_count = irq_info.count;
> >> +        return -EOVERFLOW;
> >> +    }
> >> +    if (!(irq_info.flags & VFIO_IRQ_INFO_EVENTFD)) {
> >> +        error_setg(errp, "Device interrupt doesn't support eventfd");
> >> +        return -EINVAL;
> >> +    }
> >> +
> >> +    irq_set_size = sizeof(*irq_set) + *irq_count * sizeof(int32_t);
> >> +    irq_set = g_malloc0(irq_set_size);
> >> +
> >> +    /* Get to a known IRQ state */
> >> +    *irq_set = (struct vfio_irq_set) {
> >> +        .argsz = irq_set_size,
> >> +        .flags = VFIO_IRQ_SET_DATA_EVENTFD | VFIO_IRQ_SET_ACTION_TRIGGER,
> >> +        .index = irq_info.index,
> >> +        .start = 0,
> >> +        .count = *irq_count,
> >> +    };
> >> +
> >> +    for (unsigned i = 0; i < *irq_count; i++) {
> >> +        ((int32_t *)&irq_set->data)[i] = event_notifier_get_fd(&notifier[i]);
> >> +    }
> >> +    r = ioctl(s->device, VFIO_DEVICE_SET_IRQS, irq_set);
> >> +    g_free(irq_set);
> >> +    if (r <= 0) {
> >> +        error_setg_errno(errp, errno, "Failed to setup device interrupts");
> >> +        return -errno;
> >> +    } else if (r < *irq_count) {
> >> +        error_setg(errp, "Not enough device interrupts available");
> >> +        *irq_count = r;
> >> +        return -EOVERFLOW;
> >> +    }  
> > 
> > EOVERFLOW can occur in two cases: VFIO_DEVICE_GET_IRQ_INFO and
> > VFIO_DEVICE_SET_IRQS.  
> 
> Yes.
> 
> > 
> > If it happens in the second case the notifier[] array has been
> > registered successfully.  
> 
> No, I don't think so:
> 
> vfio_pci_set_msi_trigger() register the notifier only if
> vfio_msi_enable() succeeded (returned 0). If vfio_msi_enable()
> failed it returns the number of vectors available but do
> not register the notifiers.
> 
> Alex, do you confirm?

Yes, if we can't setup what the user requested we don't setup anything.
However, I think we return zero on success, which seems to fall into
your error condition.  Has this been tested?  Thanks,

Alex

> > The caller has no way of distinguishing the two cases. Therefore the
> > caller doesn't know if the eventfds will be used by the kernel after
> > EOVERFLOW.
> > 
> > If the second case can ever happen then this function should probably
> > call VFIO_DEVICE_SET_IRQS again with VFIO_IRQ_SET_DATA_NONE to
> > unregister the eventfds before returning EOVERFLOW.
> > 
> > STefan
> >   
> 

Re: [PATCH v6 3/4] util/vfio-helpers: Introduce qemu_vfio_pci_init_msix_irqs()

[Philippe Mathieu-Daudé]

On 9/10/20 6:29 PM, Alex Williamson wrote:
> On Thu, 10 Sep 2020 17:29:25 +0200
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> 
>> Hi Stefan, Alex.
>>
>> On 9/10/20 12:44 PM, Stefan Hajnoczi wrote:
>>> On Wed, Sep 09, 2020 at 04:23:53PM +0200, Philippe Mathieu-Daudé wrote:  
>>>> +/**
>>>> + * Initialize device MSIX IRQs and register event notifiers.
>>>> + * @irq_count: pointer to number of MSIX IRQs to initialize
>>>> + * @notifier: Array of @irq_count notifiers (each corresponding to a MSIX IRQ)
>>>> +
>>>> + * If the number of IRQs requested exceeds the available on the device,
>>>> + * store the number of available IRQs in @irq_count and return -EOVERFLOW.
>>>> + */
>>>> +int qemu_vfio_pci_init_msix_irqs(QEMUVFIOState *s, EventNotifier *notifier,
>>>> +                                 unsigned *irq_count, Error **errp)
>>>> +{
>>>> +    int r;
>>>> +    size_t irq_set_size;
>>>> +    struct vfio_irq_set *irq_set;
>>>> +    struct vfio_irq_info irq_info = {
>>>> +        .argsz = sizeof(irq_info),
>>>> +        .index = VFIO_PCI_MSIX_IRQ_INDEX
>>>> +    };
>>>> +
>>>> +    if (ioctl(s->device, VFIO_DEVICE_GET_IRQ_INFO, &irq_info)) {
>>>> +        error_setg_errno(errp, errno, "Failed to get device interrupt info");
>>>> +        return -errno;
>>>> +    }
>>>> +    if (irq_info.count < *irq_count) {
>>>> +        error_setg(errp, "Not enough device interrupts available");
>>>> +        *irq_count = irq_info.count;
>>>> +        return -EOVERFLOW;
>>>> +    }
>>>> +    if (!(irq_info.flags & VFIO_IRQ_INFO_EVENTFD)) {
>>>> +        error_setg(errp, "Device interrupt doesn't support eventfd");
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    irq_set_size = sizeof(*irq_set) + *irq_count * sizeof(int32_t);
>>>> +    irq_set = g_malloc0(irq_set_size);
>>>> +
>>>> +    /* Get to a known IRQ state */
>>>> +    *irq_set = (struct vfio_irq_set) {
>>>> +        .argsz = irq_set_size,
>>>> +        .flags = VFIO_IRQ_SET_DATA_EVENTFD | VFIO_IRQ_SET_ACTION_TRIGGER,
>>>> +        .index = irq_info.index,
>>>> +        .start = 0,
>>>> +        .count = *irq_count,
>>>> +    };
>>>> +
>>>> +    for (unsigned i = 0; i < *irq_count; i++) {
>>>> +        ((int32_t *)&irq_set->data)[i] = event_notifier_get_fd(&notifier[i]);
>>>> +    }
>>>> +    r = ioctl(s->device, VFIO_DEVICE_SET_IRQS, irq_set);
>>>> +    g_free(irq_set);
>>>> +    if (r <= 0) {
>>>> +        error_setg_errno(errp, errno, "Failed to setup device interrupts");
>>>> +        return -errno;
>>>> +    } else if (r < *irq_count) {
>>>> +        error_setg(errp, "Not enough device interrupts available");
>>>> +        *irq_count = r;
>>>> +        return -EOVERFLOW;
>>>> +    }  
>>>
>>> EOVERFLOW can occur in two cases: VFIO_DEVICE_GET_IRQ_INFO and
>>> VFIO_DEVICE_SET_IRQS.  
>>
>> Yes.
>>
>>>
>>> If it happens in the second case the notifier[] array has been
>>> registered successfully.  
>>
>> No, I don't think so:
>>
>> vfio_pci_set_msi_trigger() register the notifier only if
>> vfio_msi_enable() succeeded (returned 0). If vfio_msi_enable()
>> failed it returns the number of vectors available but do
>> not register the notifiers.
>>
>> Alex, do you confirm?
> 
> Yes, if we can't setup what the user requested we don't setup anything.
> However, I think we return zero on success, which seems to fall into
> your error condition.  Has this been tested?  Thanks,

Not v6 as I didn't have the testing setup handy, and thought
v5 -> v6 change was trivial enough :S

Good news: my next task is to add a test :)

> 
> Alex
> 
>>> The caller has no way of distinguishing the two cases. Therefore the
>>> caller doesn't know if the eventfds will be used by the kernel after
>>> EOVERFLOW.
>>>
>>> If the second case can ever happen then this function should probably
>>> call VFIO_DEVICE_SET_IRQS again with VFIO_IRQ_SET_DATA_NONE to
>>> unregister the eventfds before returning EOVERFLOW.
>>>
>>> STefan
>>>   
>>
> 

Re: [PATCH v6 3/4] util/vfio-helpers: Introduce qemu_vfio_pci_init_msix_irqs()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3185 bytes --]

On Thu, Sep 10, 2020 at 05:29:25PM +0200, Philippe Mathieu-Daudé wrote:
> Hi Stefan, Alex.
> 
> On 9/10/20 12:44 PM, Stefan Hajnoczi wrote:
> > On Wed, Sep 09, 2020 at 04:23:53PM +0200, Philippe Mathieu-Daudé wrote:
> >> +/**
> >> + * Initialize device MSIX IRQs and register event notifiers.
> >> + * @irq_count: pointer to number of MSIX IRQs to initialize
> >> + * @notifier: Array of @irq_count notifiers (each corresponding to a MSIX IRQ)
> >> +
> >> + * If the number of IRQs requested exceeds the available on the device,
> >> + * store the number of available IRQs in @irq_count and return -EOVERFLOW.
> >> + */
> >> +int qemu_vfio_pci_init_msix_irqs(QEMUVFIOState *s, EventNotifier *notifier,
> >> +                                 unsigned *irq_count, Error **errp)
> >> +{
> >> +    int r;
> >> +    size_t irq_set_size;
> >> +    struct vfio_irq_set *irq_set;
> >> +    struct vfio_irq_info irq_info = {
> >> +        .argsz = sizeof(irq_info),
> >> +        .index = VFIO_PCI_MSIX_IRQ_INDEX
> >> +    };
> >> +
> >> +    if (ioctl(s->device, VFIO_DEVICE_GET_IRQ_INFO, &irq_info)) {
> >> +        error_setg_errno(errp, errno, "Failed to get device interrupt info");
> >> +        return -errno;
> >> +    }
> >> +    if (irq_info.count < *irq_count) {
> >> +        error_setg(errp, "Not enough device interrupts available");
> >> +        *irq_count = irq_info.count;
> >> +        return -EOVERFLOW;
> >> +    }
> >> +    if (!(irq_info.flags & VFIO_IRQ_INFO_EVENTFD)) {
> >> +        error_setg(errp, "Device interrupt doesn't support eventfd");
> >> +        return -EINVAL;
> >> +    }
> >> +
> >> +    irq_set_size = sizeof(*irq_set) + *irq_count * sizeof(int32_t);
> >> +    irq_set = g_malloc0(irq_set_size);
> >> +
> >> +    /* Get to a known IRQ state */
> >> +    *irq_set = (struct vfio_irq_set) {
> >> +        .argsz = irq_set_size,
> >> +        .flags = VFIO_IRQ_SET_DATA_EVENTFD | VFIO_IRQ_SET_ACTION_TRIGGER,
> >> +        .index = irq_info.index,
> >> +        .start = 0,
> >> +        .count = *irq_count,
> >> +    };
> >> +
> >> +    for (unsigned i = 0; i < *irq_count; i++) {
> >> +        ((int32_t *)&irq_set->data)[i] = event_notifier_get_fd(&notifier[i]);
> >> +    }
> >> +    r = ioctl(s->device, VFIO_DEVICE_SET_IRQS, irq_set);
> >> +    g_free(irq_set);
> >> +    if (r <= 0) {
> >> +        error_setg_errno(errp, errno, "Failed to setup device interrupts");
> >> +        return -errno;
> >> +    } else if (r < *irq_count) {
> >> +        error_setg(errp, "Not enough device interrupts available");
> >> +        *irq_count = r;
> >> +        return -EOVERFLOW;
> >> +    }
> > 
> > EOVERFLOW can occur in two cases: VFIO_DEVICE_GET_IRQ_INFO and
> > VFIO_DEVICE_SET_IRQS.
> 
> Yes.
> 
> > 
> > If it happens in the second case the notifier[] array has been
> > registered successfully.
> 
> No, I don't think so:
> 
> vfio_pci_set_msi_trigger() register the notifier only if
> vfio_msi_enable() succeeded (returned 0). If vfio_msi_enable()
> failed it returns the number of vectors available but do
> not register the notifiers.

Good, thanks!

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]