[Qemu-devel] [PULL 0/3] vfio updates 2019-06-13

[Qemu-devel] [PULL 0/3] vfio updates 2019-06-13

[Alex Williamson]

The following changes since commit 650a379d505bf558bcb41124bc6c951a76cbc113:

  Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20190613-1' into staging (2019-06-13 15:16:39 +0100)

are available in the Git repository at:

  git://github.com/awilliam/qemu-vfio.git tags/vfio-updates-20190613.0

for you to fetch changes up to 201a733145751aa691e7e3b9c0f263f0c92db0c5:

  vfio/common: Introduce vfio_set_irq_signaling helper (2019-06-13 09:57:37 -0600)

----------------------------------------------------------------
VFIO updates 2019-06-13

 - Hide resizable BAR capability to prevent false guest resizing
   (Alex Williamson)

 - Allow relocation to fix bogus MSI-X hardware (Alex Williamson)

 - Condense IRQ setup into a common helper (Eric Auger)

----------------------------------------------------------------
Alex Williamson (2):
      vfio/pci: Hide Resizable BAR capability
      vfio/pci: Allow MSI-X relocation to fixup bogus PBA

Eric Auger (1):
      vfio/common: Introduce vfio_set_irq_signaling helper

 hw/vfio/common.c              |  78 +++++++++++++++
 hw/vfio/pci.c                 | 220 ++++++++++--------------------------------
 hw/vfio/platform.c            |  68 +++++--------
 include/hw/vfio/vfio-common.h |   2 +
 4 files changed, 156 insertions(+), 212 deletions(-)

[Qemu-devel] [PULL 1/3] vfio/pci: Hide Resizable BAR capability

[Alex Williamson]

The resizable BAR capability is currently exposed read-only from the
kernel and we don't yet implement a protocol for virtualizing it to
the VM.  Exposing it to the guest read-only introduces poor behavior
as the guest has no reason to test that a control register write is
accepted by the hardware.  This can lead to cases where the guest OS
assumes the BAR has been resized, but it hasn't.  This has been
observed when assigning AMD Vega GPUs.

Note, this does not preclude future enablement of resizable BARs, but
it's currently incorrect to expose this capability as read-only, so
better to not expose it at all.

Reported-by: James Courtier-Dutton <james.dutton@gmail.com>
Tested-by: James Courtier-Dutton <james.dutton@gmail.com>
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 hw/vfio/pci.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 4a04f795162b..48f4e19a02cf 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2119,6 +2119,7 @@ static void vfio_add_ext_cap(VFIOPCIDevice *vdev)
         case 0: /* kernel masked capability */
         case PCI_EXT_CAP_ID_SRIOV: /* Read-only VF BARs confuse OVMF */
         case PCI_EXT_CAP_ID_ARI: /* XXX Needs next function virtualization */
+        case PCI_EXT_CAP_ID_REBAR: /* Can't expose read-only */
             trace_vfio_add_ext_cap_dropped(vdev->vbasedev.name, cap_id, next);
             break;
         default:

[Qemu-devel] [PULL 2/3] vfio/pci: Allow MSI-X relocation to fixup bogus PBA

[Alex Williamson]

The MSI-X relocation code can sometimes be used to work around bogus
MSI-X capabilities, but this test for whether the PBA is outside of
the specified BAR causes the device to error before we can apply a
relocation.  Let it proceed if we intend to relocate MSI-X anyway.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 hw/vfio/pci.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 48f4e19a02cf..6520c05dee92 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1533,7 +1533,7 @@ static void vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
         if (vdev->vendor_id == PCI_VENDOR_ID_CHELSIO &&
             (vdev->device_id & 0xff00) == 0x5800) {
             msix->pba_offset = 0x1000;
-        } else {
+        } else if (vdev->msix_relo == OFF_AUTOPCIBAR_OFF) {
             error_setg(errp, "hardware reports invalid configuration, "
                        "MSIX PBA outside of specified BAR");
             g_free(msix);

[Qemu-devel] [PULL 3/3] vfio/common: Introduce vfio_set_irq_signaling helper

[Alex Williamson]

From: Eric Auger <eric.auger@redhat.com>

The code used to assign an interrupt index/subindex to an
eventfd is duplicated many times. Let's introduce an helper that
allows to set/unset the signaling for an ACTION_TRIGGER,
ACTION_MASK or ACTION_UNMASK action.

In the error message, we now use errno in case of any
VFIO_DEVICE_SET_IRQS ioctl failure.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: Li Qiang <liq3ea@gmail.com>
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 hw/vfio/common.c              |   78 +++++++++++++++
 hw/vfio/pci.c                 |  217 ++++++++++-------------------------------
 hw/vfio/platform.c            |   68 ++++---------
 include/hw/vfio/vfio-common.h |    2 
 4 files changed, 154 insertions(+), 211 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 4374cc6176a2..a859298fdad9 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -95,6 +95,84 @@ void vfio_mask_single_irqindex(VFIODevice *vbasedev, int index)
     ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, &irq_set);
 }
 
+static inline const char *action_to_str(int action)
+{
+    switch (action) {
+    case VFIO_IRQ_SET_ACTION_MASK:
+        return "MASK";
+    case VFIO_IRQ_SET_ACTION_UNMASK:
+        return "UNMASK";
+    case VFIO_IRQ_SET_ACTION_TRIGGER:
+        return "TRIGGER";
+    default:
+        return "UNKNOWN ACTION";
+    }
+}
+
+static const char *index_to_str(VFIODevice *vbasedev, int index)
+{
+    if (vbasedev->type != VFIO_DEVICE_TYPE_PCI) {
+        return NULL;
+    }
+
+    switch (index) {
+    case VFIO_PCI_INTX_IRQ_INDEX:
+        return "INTX";
+    case VFIO_PCI_MSI_IRQ_INDEX:
+        return "MSI";
+    case VFIO_PCI_MSIX_IRQ_INDEX:
+        return "MSIX";
+    case VFIO_PCI_ERR_IRQ_INDEX:
+        return "ERR";
+    case VFIO_PCI_REQ_IRQ_INDEX:
+        return "REQ";
+    default:
+        return NULL;
+    }
+}
+
+int vfio_set_irq_signaling(VFIODevice *vbasedev, int index, int subindex,
+                           int action, int fd, Error **errp)
+{
+    struct vfio_irq_set *irq_set;
+    int argsz, ret = 0;
+    const char *name;
+    int32_t *pfd;
+
+    argsz = sizeof(*irq_set) + sizeof(*pfd);
+
+    irq_set = g_malloc0(argsz);
+    irq_set->argsz = argsz;
+    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD | action;
+    irq_set->index = index;
+    irq_set->start = subindex;
+    irq_set->count = 1;
+    pfd = (int32_t *)&irq_set->data;
+    *pfd = fd;
+
+    if (ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, irq_set)) {
+        ret = -errno;
+    }
+    g_free(irq_set);
+
+    if (!ret) {
+        return 0;
+    }
+
+    error_setg_errno(errp, -ret, "VFIO_DEVICE_SET_IRQS failure");
+
+    name = index_to_str(vbasedev, index);
+    if (name) {
+        error_prepend(errp, "%s-%d: ", name, subindex);
+    } else {
+        error_prepend(errp, "index %d-%d: ", index, subindex);
+    }
+    error_prepend(errp,
+                  "Failed to %s %s eventfd signaling for interrupt ",
+                  fd < 0 ? "tear down" : "set up", action_to_str(action));
+    return ret;
+}
+
 /*
  * IO Port/MMIO - Beware of the endians, VFIO is always little endian
  */
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 6520c05dee92..ce3fe96efe2c 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -114,9 +114,7 @@ static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp)
         .gsi = vdev->intx.route.irq,
         .flags = KVM_IRQFD_FLAG_RESAMPLE,
     };
-    struct vfio_irq_set *irq_set;
-    int ret, argsz;
-    int32_t *pfd;
+    Error *err = NULL;
 
     if (vdev->no_kvm_intx || !kvm_irqfds_enabled() ||
         vdev->intx.route.mode != PCI_INTX_ENABLED ||
@@ -144,22 +142,10 @@ static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp)
         goto fail_irqfd;
     }
 
-    argsz = sizeof(*irq_set) + sizeof(*pfd);
-
-    irq_set = g_malloc0(argsz);
-    irq_set->argsz = argsz;
-    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD | VFIO_IRQ_SET_ACTION_UNMASK;
-    irq_set->index = VFIO_PCI_INTX_IRQ_INDEX;
-    irq_set->start = 0;
-    irq_set->count = 1;
-    pfd = (int32_t *)&irq_set->data;
-
-    *pfd = irqfd.resamplefd;
-
-    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
-    g_free(irq_set);
-    if (ret) {
-        error_setg_errno(errp, -ret, "failed to setup INTx unmask fd");
+    if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_INTX_IRQ_INDEX, 0,
+                               VFIO_IRQ_SET_ACTION_UNMASK,
+                               irqfd.resamplefd, &err)) {
+        error_propagate(errp, err);
         goto fail_vfio;
     }
 
@@ -263,10 +249,10 @@ static void vfio_intx_update(PCIDevice *pdev)
 static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
 {
     uint8_t pin = vfio_pci_read_config(&vdev->pdev, PCI_INTERRUPT_PIN, 1);
-    int ret, argsz, retval = 0;
-    struct vfio_irq_set *irq_set;
-    int32_t *pfd;
     Error *err = NULL;
+    int32_t fd;
+    int ret;
+
 
     if (!pin) {
         return 0;
@@ -293,27 +279,15 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
         error_setg_errno(errp, -ret, "event_notifier_init failed");
         return ret;
     }
+    fd = event_notifier_get_fd(&vdev->intx.interrupt);
+    qemu_set_fd_handler(fd, vfio_intx_interrupt, NULL, vdev);
 
-    argsz = sizeof(*irq_set) + sizeof(*pfd);
-
-    irq_set = g_malloc0(argsz);
-    irq_set->argsz = argsz;
-    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD | VFIO_IRQ_SET_ACTION_TRIGGER;
-    irq_set->index = VFIO_PCI_INTX_IRQ_INDEX;
-    irq_set->start = 0;
-    irq_set->count = 1;
-    pfd = (int32_t *)&irq_set->data;
-
-    *pfd = event_notifier_get_fd(&vdev->intx.interrupt);
-    qemu_set_fd_handler(*pfd, vfio_intx_interrupt, NULL, vdev);
-
-    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
-    if (ret) {
-        error_setg_errno(errp, -ret, "failed to setup INTx fd");
-        qemu_set_fd_handler(*pfd, NULL, NULL, vdev);
+    if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_INTX_IRQ_INDEX, 0,
+                               VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
+        error_propagate(errp, err);
+        qemu_set_fd_handler(fd, NULL, NULL, vdev);
         event_notifier_cleanup(&vdev->intx.interrupt);
-        retval = -errno;
-        goto cleanup;
+        return -errno;
     }
 
     vfio_intx_enable_kvm(vdev, &err);
@@ -324,11 +298,7 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
     vdev->interrupt = VFIO_INT_INTx;
 
     trace_vfio_intx_enable(vdev->vbasedev.name);
-
-cleanup:
-    g_free(irq_set);
-
-    return retval;
+    return 0;
 }
 
 static void vfio_intx_disable(VFIOPCIDevice *vdev)
@@ -531,31 +501,19 @@ static int vfio_msix_vector_do_use(PCIDevice *pdev, unsigned int nr,
             error_report("vfio: failed to enable vectors, %d", ret);
         }
     } else {
-        int argsz;
-        struct vfio_irq_set *irq_set;
-        int32_t *pfd;
-
-        argsz = sizeof(*irq_set) + sizeof(*pfd);
-
-        irq_set = g_malloc0(argsz);
-        irq_set->argsz = argsz;
-        irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
-                         VFIO_IRQ_SET_ACTION_TRIGGER;
-        irq_set->index = VFIO_PCI_MSIX_IRQ_INDEX;
-        irq_set->start = nr;
-        irq_set->count = 1;
-        pfd = (int32_t *)&irq_set->data;
+        Error *err = NULL;
+        int32_t fd;
 
         if (vector->virq >= 0) {
-            *pfd = event_notifier_get_fd(&vector->kvm_interrupt);
+            fd = event_notifier_get_fd(&vector->kvm_interrupt);
         } else {
-            *pfd = event_notifier_get_fd(&vector->interrupt);
+            fd = event_notifier_get_fd(&vector->interrupt);
         }
 
-        ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
-        g_free(irq_set);
-        if (ret) {
-            error_report("vfio: failed to modify vector, %d", ret);
+        if (vfio_set_irq_signaling(&vdev->vbasedev,
+                                     VFIO_PCI_MSIX_IRQ_INDEX, nr,
+                                     VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
+            error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
         }
     }
 
@@ -592,26 +550,10 @@ static void vfio_msix_vector_release(PCIDevice *pdev, unsigned int nr)
      * be re-asserted on unmask.  Nothing to do if already using QEMU mode.
      */
     if (vector->virq >= 0) {
-        int argsz;
-        struct vfio_irq_set *irq_set;
-        int32_t *pfd;
-
-        argsz = sizeof(*irq_set) + sizeof(*pfd);
-
-        irq_set = g_malloc0(argsz);
-        irq_set->argsz = argsz;
-        irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
-                         VFIO_IRQ_SET_ACTION_TRIGGER;
-        irq_set->index = VFIO_PCI_MSIX_IRQ_INDEX;
-        irq_set->start = nr;
-        irq_set->count = 1;
-        pfd = (int32_t *)&irq_set->data;
+        int32_t fd = event_notifier_get_fd(&vector->interrupt);
 
-        *pfd = event_notifier_get_fd(&vector->interrupt);
-
-        ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
-
-        g_free(irq_set);
+        vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX, nr,
+                               VFIO_IRQ_SET_ACTION_TRIGGER, fd, NULL);
     }
 }
 
@@ -2638,10 +2580,8 @@ static void vfio_err_notifier_handler(void *opaque)
  */
 static void vfio_register_err_notifier(VFIOPCIDevice *vdev)
 {
-    int ret;
-    int argsz;
-    struct vfio_irq_set *irq_set;
-    int32_t *pfd;
+    Error *err = NULL;
+    int32_t fd;
 
     if (!vdev->pci_aer) {
         return;
@@ -2653,58 +2593,30 @@ static void vfio_register_err_notifier(VFIOPCIDevice *vdev)
         return;
     }
 
-    argsz = sizeof(*irq_set) + sizeof(*pfd);
+    fd = event_notifier_get_fd(&vdev->err_notifier);
+    qemu_set_fd_handler(fd, vfio_err_notifier_handler, NULL, vdev);
 
-    irq_set = g_malloc0(argsz);
-    irq_set->argsz = argsz;
-    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
-                     VFIO_IRQ_SET_ACTION_TRIGGER;
-    irq_set->index = VFIO_PCI_ERR_IRQ_INDEX;
-    irq_set->start = 0;
-    irq_set->count = 1;
-    pfd = (int32_t *)&irq_set->data;
-
-    *pfd = event_notifier_get_fd(&vdev->err_notifier);
-    qemu_set_fd_handler(*pfd, vfio_err_notifier_handler, NULL, vdev);
-
-    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
-    if (ret) {
-        error_report("vfio: Failed to set up error notification");
-        qemu_set_fd_handler(*pfd, NULL, NULL, vdev);
+    if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_ERR_IRQ_INDEX, 0,
+                               VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
+        error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
+        qemu_set_fd_handler(fd, NULL, NULL, vdev);
         event_notifier_cleanup(&vdev->err_notifier);
         vdev->pci_aer = false;
     }
-    g_free(irq_set);
 }
 
 static void vfio_unregister_err_notifier(VFIOPCIDevice *vdev)
 {
-    int argsz;
-    struct vfio_irq_set *irq_set;
-    int32_t *pfd;
-    int ret;
+    Error *err = NULL;
 
     if (!vdev->pci_aer) {
         return;
     }
 
-    argsz = sizeof(*irq_set) + sizeof(*pfd);
-
-    irq_set = g_malloc0(argsz);
-    irq_set->argsz = argsz;
-    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
-                     VFIO_IRQ_SET_ACTION_TRIGGER;
-    irq_set->index = VFIO_PCI_ERR_IRQ_INDEX;
-    irq_set->start = 0;
-    irq_set->count = 1;
-    pfd = (int32_t *)&irq_set->data;
-    *pfd = -1;
-
-    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
-    if (ret) {
-        error_report("vfio: Failed to de-assign error fd: %m");
+    if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_ERR_IRQ_INDEX, 0,
+                               VFIO_IRQ_SET_ACTION_TRIGGER, -1, &err)) {
+        error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
     }
-    g_free(irq_set);
     qemu_set_fd_handler(event_notifier_get_fd(&vdev->err_notifier),
                         NULL, NULL, vdev);
     event_notifier_cleanup(&vdev->err_notifier);
@@ -2729,9 +2641,8 @@ static void vfio_register_req_notifier(VFIOPCIDevice *vdev)
 {
     struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info),
                                       .index = VFIO_PCI_REQ_IRQ_INDEX };
-    int argsz;
-    struct vfio_irq_set *irq_set;
-    int32_t *pfd;
+    Error *err = NULL;
+    int32_t fd;
 
     if (!(vdev->features & VFIO_FEATURE_ENABLE_REQ)) {
         return;
@@ -2747,57 +2658,31 @@ static void vfio_register_req_notifier(VFIOPCIDevice *vdev)
         return;
     }
 
-    argsz = sizeof(*irq_set) + sizeof(*pfd);
-
-    irq_set = g_malloc0(argsz);
-    irq_set->argsz = argsz;
-    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
-                     VFIO_IRQ_SET_ACTION_TRIGGER;
-    irq_set->index = VFIO_PCI_REQ_IRQ_INDEX;
-    irq_set->start = 0;
-    irq_set->count = 1;
-    pfd = (int32_t *)&irq_set->data;
-
-    *pfd = event_notifier_get_fd(&vdev->req_notifier);
-    qemu_set_fd_handler(*pfd, vfio_req_notifier_handler, NULL, vdev);
+    fd = event_notifier_get_fd(&vdev->req_notifier);
+    qemu_set_fd_handler(fd, vfio_req_notifier_handler, NULL, vdev);
 
-    if (ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set)) {
-        error_report("vfio: Failed to set up device request notification");
-        qemu_set_fd_handler(*pfd, NULL, NULL, vdev);
+    if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_REQ_IRQ_INDEX, 0,
+                           VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
+        error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
+        qemu_set_fd_handler(fd, NULL, NULL, vdev);
         event_notifier_cleanup(&vdev->req_notifier);
     } else {
         vdev->req_enabled = true;
     }
-
-    g_free(irq_set);
 }
 
 static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
 {
-    int argsz;
-    struct vfio_irq_set *irq_set;
-    int32_t *pfd;
+    Error *err = NULL;
 
     if (!vdev->req_enabled) {
         return;
     }
 
-    argsz = sizeof(*irq_set) + sizeof(*pfd);
-
-    irq_set = g_malloc0(argsz);
-    irq_set->argsz = argsz;
-    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
-                     VFIO_IRQ_SET_ACTION_TRIGGER;
-    irq_set->index = VFIO_PCI_REQ_IRQ_INDEX;
-    irq_set->start = 0;
-    irq_set->count = 1;
-    pfd = (int32_t *)&irq_set->data;
-    *pfd = -1;
-
-    if (ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set)) {
-        error_report("vfio: Failed to de-assign device request fd: %m");
+    if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_REQ_IRQ_INDEX, 0,
+                               VFIO_IRQ_SET_ACTION_TRIGGER, -1, &err)) {
+        error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
     }
-    g_free(irq_set);
     qemu_set_fd_handler(event_notifier_get_fd(&vdev->req_notifier),
                         NULL, NULL, vdev);
     event_notifier_cleanup(&vdev->req_notifier);
diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
index ad2972595574..622e609fb425 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -107,26 +107,19 @@ static int vfio_set_trigger_eventfd(VFIOINTp *intp,
                                     eventfd_user_side_handler_t handler)
 {
     VFIODevice *vbasedev = &intp->vdev->vbasedev;
-    struct vfio_irq_set *irq_set;
-    int argsz, ret;
-    int32_t *pfd;
-
-    argsz = sizeof(*irq_set) + sizeof(*pfd);
-    irq_set = g_malloc0(argsz);
-    irq_set->argsz = argsz;
-    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD | VFIO_IRQ_SET_ACTION_TRIGGER;
-    irq_set->index = intp->pin;
-    irq_set->start = 0;
-    irq_set->count = 1;
-    pfd = (int32_t *)&irq_set->data;
-    *pfd = event_notifier_get_fd(intp->interrupt);
-    qemu_set_fd_handler(*pfd, (IOHandler *)handler, NULL, intp);
-    ret = ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, irq_set);
-    if (ret < 0) {
-        error_report("vfio: Failed to set trigger eventfd: %m");
-        qemu_set_fd_handler(*pfd, NULL, NULL, NULL);
-    }
-    g_free(irq_set);
+    int32_t fd = event_notifier_get_fd(intp->interrupt);
+    Error *err = NULL;
+    int ret;
+
+    qemu_set_fd_handler(fd, (IOHandler *)handler, NULL, intp);
+
+    ret = vfio_set_irq_signaling(vbasedev, intp->pin, 0,
+                                 VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err);
+    if (ret) {
+        error_reportf_err(err, VFIO_MSG_PREFIX, vbasedev->name);
+        qemu_set_fd_handler(fd, NULL, NULL, NULL);
+    }
+
     return ret;
 }
 
@@ -331,7 +324,6 @@ static void vfio_platform_eoi(VFIODevice *vbasedev)
 
 static void vfio_start_eventfd_injection(SysBusDevice *sbdev, qemu_irq irq)
 {
-    int ret;
     VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
     VFIOINTp *intp;
 
@@ -342,10 +334,7 @@ static void vfio_start_eventfd_injection(SysBusDevice *sbdev, qemu_irq irq)
     }
     assert(intp);
 
-    ret = vfio_set_trigger_eventfd(intp, vfio_intp_interrupt);
-    if (ret) {
-        error_report("vfio: failed to start eventfd signaling for IRQ %d: %m",
-                     intp->pin);
+    if (vfio_set_trigger_eventfd(intp, vfio_intp_interrupt)) {
         abort();
     }
 }
@@ -362,25 +351,16 @@ static void vfio_start_eventfd_injection(SysBusDevice *sbdev, qemu_irq irq)
  */
 static int vfio_set_resample_eventfd(VFIOINTp *intp)
 {
+    int32_t fd = event_notifier_get_fd(intp->unmask);
     VFIODevice *vbasedev = &intp->vdev->vbasedev;
-    struct vfio_irq_set *irq_set;
-    int argsz, ret;
-    int32_t *pfd;
-
-    argsz = sizeof(*irq_set) + sizeof(*pfd);
-    irq_set = g_malloc0(argsz);
-    irq_set->argsz = argsz;
-    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD | VFIO_IRQ_SET_ACTION_UNMASK;
-    irq_set->index = intp->pin;
-    irq_set->start = 0;
-    irq_set->count = 1;
-    pfd = (int32_t *)&irq_set->data;
-    *pfd = event_notifier_get_fd(intp->unmask);
-    qemu_set_fd_handler(*pfd, NULL, NULL, NULL);
-    ret = ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, irq_set);
-    g_free(irq_set);
-    if (ret < 0) {
-        error_report("vfio: Failed to set resample eventfd: %m");
+    Error *err = NULL;
+    int ret;
+
+    qemu_set_fd_handler(fd, NULL, NULL, NULL);
+    ret = vfio_set_irq_signaling(vbasedev, intp->pin, 0,
+                                 VFIO_IRQ_SET_ACTION_UNMASK, fd, &err);
+    if (ret) {
+        error_reportf_err(err, VFIO_MSG_PREFIX, vbasedev->name);
     }
     return ret;
 }
@@ -436,8 +416,6 @@ static void vfio_start_irqfd_injection(SysBusDevice *sbdev, qemu_irq irq)
     return;
 fail_vfio:
     kvm_irqchip_remove_irqfd_notifier(kvm_state, intp->interrupt, irq);
-    error_report("vfio: failed to start eventfd signaling for IRQ %d: %m",
-                 intp->pin);
     abort();
 fail_irqfd:
     vfio_start_eventfd_injection(sbdev, irq);
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index e05bc09794ab..a88b69b6750e 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -166,6 +166,8 @@ void vfio_put_base_device(VFIODevice *vbasedev);
 void vfio_disable_irqindex(VFIODevice *vbasedev, int index);
 void vfio_unmask_single_irqindex(VFIODevice *vbasedev, int index);
 void vfio_mask_single_irqindex(VFIODevice *vbasedev, int index);
+int vfio_set_irq_signaling(VFIODevice *vbasedev, int index, int subindex,
+                           int action, int fd, Error **errp);
 void vfio_region_write(void *opaque, hwaddr addr,
                            uint64_t data, unsigned size);
 uint64_t vfio_region_read(void *opaque,

Re: [Qemu-devel] [PULL 3/3] vfio/common: Introduce vfio_set_irq_signaling helper

[Peter Maydell]

On Thu, 13 Jun 2019 at 22:51, Alex Williamson
<alex.williamson@redhat.com> wrote:
>
> From: Eric Auger <eric.auger@redhat.com>
>
> The code used to assign an interrupt index/subindex to an
> eventfd is duplicated many times. Let's introduce an helper that
> allows to set/unset the signaling for an ACTION_TRIGGER,
> ACTION_MASK or ACTION_UNMASK action.
>
> In the error message, we now use errno in case of any
> VFIO_DEVICE_SET_IRQS ioctl failure.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> Reviewed-by: Li Qiang <liq3ea@gmail.com>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>

Hi; coverity reports (CID 1402196) a possible unchecked return value
in this code:


> @@ -592,26 +550,10 @@ static void vfio_msix_vector_release(PCIDevice *pdev, unsigned int nr)
>       * be re-asserted on unmask.  Nothing to do if already using QEMU mode.
>       */
>      if (vector->virq >= 0) {
> -        int argsz;
> -        struct vfio_irq_set *irq_set;
> -        int32_t *pfd;
> -
> -        argsz = sizeof(*irq_set) + sizeof(*pfd);
> -
> -        irq_set = g_malloc0(argsz);
> -        irq_set->argsz = argsz;
> -        irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
> -                         VFIO_IRQ_SET_ACTION_TRIGGER;
> -        irq_set->index = VFIO_PCI_MSIX_IRQ_INDEX;
> -        irq_set->start = nr;
> -        irq_set->count = 1;
> -        pfd = (int32_t *)&irq_set->data;
> +        int32_t fd = event_notifier_get_fd(&vector->interrupt);
>
> -        *pfd = event_notifier_get_fd(&vector->interrupt);
> -
> -        ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
> -
> -        g_free(irq_set);
> +        vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX, nr,
> +                               VFIO_IRQ_SET_ACTION_TRIGGER, fd, NULL);

In vfp_msix_vector_release() we call vfio_set_irq_signaling(),
but we don't check the returned error value, whereas in the other
7 places we call the function we do check. Is there some missing
error handling here ?

thanks
-- PMM

Re: [Qemu-devel] [PULL 3/3] vfio/common: Introduce vfio_set_irq_signaling helper

[Auger Eric]

Hi Peter,

On 7/2/19 12:37 PM, Peter Maydell wrote:
> On Thu, 13 Jun 2019 at 22:51, Alex Williamson
> <alex.williamson@redhat.com> wrote:
>>
>> From: Eric Auger <eric.auger@redhat.com>
>>
>> The code used to assign an interrupt index/subindex to an
>> eventfd is duplicated many times. Let's introduce an helper that
>> allows to set/unset the signaling for an ACTION_TRIGGER,
>> ACTION_MASK or ACTION_UNMASK action.
>>
>> In the error message, we now use errno in case of any
>> VFIO_DEVICE_SET_IRQS ioctl failure.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>> Reviewed-by: Li Qiang <liq3ea@gmail.com>
>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> 
> Hi; coverity reports (CID 1402196) a possible unchecked return value
> in this code:
> 
> 
>> @@ -592,26 +550,10 @@ static void vfio_msix_vector_release(PCIDevice *pdev, unsigned int nr)
>>       * be re-asserted on unmask.  Nothing to do if already using QEMU mode.
>>       */
>>      if (vector->virq >= 0) {
>> -        int argsz;
>> -        struct vfio_irq_set *irq_set;
>> -        int32_t *pfd;
>> -
>> -        argsz = sizeof(*irq_set) + sizeof(*pfd);
>> -
>> -        irq_set = g_malloc0(argsz);
>> -        irq_set->argsz = argsz;
>> -        irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
>> -                         VFIO_IRQ_SET_ACTION_TRIGGER;
>> -        irq_set->index = VFIO_PCI_MSIX_IRQ_INDEX;
>> -        irq_set->start = nr;
>> -        irq_set->count = 1;
>> -        pfd = (int32_t *)&irq_set->data;
>> +        int32_t fd = event_notifier_get_fd(&vector->interrupt);
>>
>> -        *pfd = event_notifier_get_fd(&vector->interrupt);
>> -
>> -        ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
>> -
>> -        g_free(irq_set);
>> +        vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX, nr,
>> +                               VFIO_IRQ_SET_ACTION_TRIGGER, fd, NULL);
> 
> In vfp_msix_vector_release() we call vfio_set_irq_signaling(),
> but we don't check the returned error value, whereas in the other
> 7 places we call the function we do check. Is there some missing
> error handling here ?

the difference with the other calls is that we pass a NULL errp here so
we don't need to consume a potential error. Before the introduction of
vfio_set_irq_signaling we had an ioctl call whose returned value was not
tested either. So I think it properly translates what was done before.
It seems we are willingly not producing any error message in that case.
Alex, can you confirm?

Thanks

Eric
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PULL 3/3] vfio/common: Introduce vfio_set_irq_signaling helper

[Alex Williamson]

On Tue, 2 Jul 2019 14:32:26 +0200
Auger Eric <eric.auger@redhat.com> wrote:

> Hi Peter,
> 
> On 7/2/19 12:37 PM, Peter Maydell wrote:
> > On Thu, 13 Jun 2019 at 22:51, Alex Williamson
> > <alex.williamson@redhat.com> wrote:  
> >>
> >> From: Eric Auger <eric.auger@redhat.com>
> >>
> >> The code used to assign an interrupt index/subindex to an
> >> eventfd is duplicated many times. Let's introduce an helper that
> >> allows to set/unset the signaling for an ACTION_TRIGGER,
> >> ACTION_MASK or ACTION_UNMASK action.
> >>
> >> In the error message, we now use errno in case of any
> >> VFIO_DEVICE_SET_IRQS ioctl failure.
> >>
> >> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> >> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> >> Reviewed-by: Li Qiang <liq3ea@gmail.com>
> >> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>  
> > 
> > Hi; coverity reports (CID 1402196) a possible unchecked return value
> > in this code:
> > 
> >   
> >> @@ -592,26 +550,10 @@ static void vfio_msix_vector_release(PCIDevice *pdev, unsigned int nr)
> >>       * be re-asserted on unmask.  Nothing to do if already using QEMU mode.
> >>       */
> >>      if (vector->virq >= 0) {
> >> -        int argsz;
> >> -        struct vfio_irq_set *irq_set;
> >> -        int32_t *pfd;
> >> -
> >> -        argsz = sizeof(*irq_set) + sizeof(*pfd);
> >> -
> >> -        irq_set = g_malloc0(argsz);
> >> -        irq_set->argsz = argsz;
> >> -        irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
> >> -                         VFIO_IRQ_SET_ACTION_TRIGGER;
> >> -        irq_set->index = VFIO_PCI_MSIX_IRQ_INDEX;
> >> -        irq_set->start = nr;
> >> -        irq_set->count = 1;
> >> -        pfd = (int32_t *)&irq_set->data;
> >> +        int32_t fd = event_notifier_get_fd(&vector->interrupt);
> >>
> >> -        *pfd = event_notifier_get_fd(&vector->interrupt);
> >> -
> >> -        ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
> >> -
> >> -        g_free(irq_set);
> >> +        vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX, nr,
> >> +                               VFIO_IRQ_SET_ACTION_TRIGGER, fd, NULL);  
> > 
> > In vfp_msix_vector_release() we call vfio_set_irq_signaling(),
> > but we don't check the returned error value, whereas in the other
> > 7 places we call the function we do check. Is there some missing
> > error handling here ?  
> 
> the difference with the other calls is that we pass a NULL errp here so
> we don't need to consume a potential error. Before the introduction of
> vfio_set_irq_signaling we had an ioctl call whose returned value was not
> tested either. So I think it properly translates what was done before.
> It seems we are willingly not producing any error message in that case.
> Alex, can you confirm?

When we're emulating writes to the MSI-X vector table we have no
failure path up to the guest.  Real hardware cannot fail to enable a
vector that's available in hardware, thus we can either log the issue,
ignore the issue, or fault.  I guess Coverity is simply noting that
other cases are tested while this is not, therefore we should either
explicitly ignore the return value with a cast to void or take this as
an opportunity to log the fault, which might be useful in debugging a
device that isn't working properly.  Thanks,

Alex

Re: [Qemu-devel] [PULL 3/3] vfio/common: Introduce vfio_set_irq_signaling helper

[Peter Maydell]

On Tue, 2 Jul 2019 at 16:56, Alex Williamson <alex.williamson@redhat.com> wrote:
> When we're emulating writes to the MSI-X vector table we have no
> failure path up to the guest.  Real hardware cannot fail to enable a
> vector that's available in hardware, thus we can either log the issue,
> ignore the issue, or fault.  I guess Coverity is simply noting that
> other cases are tested while this is not, therefore we should either
> explicitly ignore the return value with a cast to void or take this as
> an opportunity to log the fault, which might be useful in debugging a
> device that isn't working properly.  Thanks,

Yeah, Coverity's check here is purely a heuristic ("did we seem
to check returns from this function in other places?") so it's
wrong sometimes. If you want me to mark this as a false positive
in the coverity UI I can do that.

thanks
-- PMM

Re: [Qemu-devel] [PULL 3/3] vfio/common: Introduce vfio_set_irq_signaling helper

[Alex Williamson]

On Tue, 2 Jul 2019 16:58:02 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On Tue, 2 Jul 2019 at 16:56, Alex Williamson <alex.williamson@redhat.com> wrote:
> > When we're emulating writes to the MSI-X vector table we have no
> > failure path up to the guest.  Real hardware cannot fail to enable a
> > vector that's available in hardware, thus we can either log the issue,
> > ignore the issue, or fault.  I guess Coverity is simply noting that
> > other cases are tested while this is not, therefore we should either
> > explicitly ignore the return value with a cast to void or take this as
> > an opportunity to log the fault, which might be useful in debugging a
> > device that isn't working properly.  Thanks,  
> 
> Yeah, Coverity's check here is purely a heuristic ("did we seem
> to check returns from this function in other places?") so it's
> wrong sometimes. If you want me to mark this as a false positive
> in the coverity UI I can do that.

TBH, it seems like a good nag to log it properly.  Eric, do you mind
posting a fix to do that?  Thanks,

Alex

Re: [Qemu-devel] [PULL 3/3] vfio/common: Introduce vfio_set_irq_signaling helper

[Auger Eric]

Hi,
On 7/2/19 6:30 PM, Alex Williamson wrote:
> On Tue, 2 Jul 2019 16:58:02 +0100
> Peter Maydell <peter.maydell@linaro.org> wrote:
> 
>> On Tue, 2 Jul 2019 at 16:56, Alex Williamson <alex.williamson@redhat.com> wrote:
>>> When we're emulating writes to the MSI-X vector table we have no
>>> failure path up to the guest.  Real hardware cannot fail to enable a
>>> vector that's available in hardware, thus we can either log the issue,
>>> ignore the issue, or fault.  I guess Coverity is simply noting that
>>> other cases are tested while this is not, therefore we should either
>>> explicitly ignore the return value with a cast to void or take this as
>>> an opportunity to log the fault, which might be useful in debugging a
>>> device that isn't working properly.  Thanks,  
>>
>> Yeah, Coverity's check here is purely a heuristic ("did we seem
>> to check returns from this function in other places?") so it's
>> wrong sometimes. If you want me to mark this as a false positive
>> in the coverity UI I can do that.
> 
> TBH, it seems like a good nag to log it properly.  Eric, do you mind
> posting a fix to do that?  Thanks,
Sure I will send a fix.

Thanks

Eric
> 
> Alex
> 

Re: [Qemu-devel] [PULL 0/3] vfio updates 2019-06-13

[Peter Maydell]

On Thu, 13 Jun 2019 at 22:41, Alex Williamson
<alex.williamson@redhat.com> wrote:
>
> The following changes since commit 650a379d505bf558bcb41124bc6c951a76cbc113:
>
>   Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20190613-1' into staging (2019-06-13 15:16:39 +0100)
>
> are available in the Git repository at:
>
>   git://github.com/awilliam/qemu-vfio.git tags/vfio-updates-20190613.0
>
> for you to fetch changes up to 201a733145751aa691e7e3b9c0f263f0c92db0c5:
>
>   vfio/common: Introduce vfio_set_irq_signaling helper (2019-06-13 09:57:37 -0600)
>
> ----------------------------------------------------------------
> VFIO updates 2019-06-13
>
>  - Hide resizable BAR capability to prevent false guest resizing
>    (Alex Williamson)
>
>  - Allow relocation to fix bogus MSI-X hardware (Alex Williamson)
>
>  - Condense IRQ setup into a common helper (Eric Auger)


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/4.1
for any user-visible changes.

-- PMM