[Qemu-devel] [PATCH v7 00/10] Convert msix_init() to error

[Qemu-devel] [PATCH v7 00/10] Convert msix_init() to error

[Cao jin]

v7 changelog:
1. fix the segfaut bug in patch 2. So drop the all the R-b of it,
   please take a look, there is detailed description in the patch.
2. add the R-b from Hannes Reinecke

Test:
1. make check: pass
2. After applied all the patch, command line test for all the
   affected devices, just make sure device realize process is ok,
   no crash, but no further use of device.

CC: Jiri Pirko <jiri@resnulli.us>
CC: Gerd Hoffmann <kraxel@redhat.com>
CC: Dmitry Fleytman <dmitry@daynix.com>
CC: Jason Wang <jasowang@redhat.com>
CC: Michael S. Tsirkin <mst@redhat.com>
CC: Hannes Reinecke <hare@suse.de>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Alex Williamson <alex.williamson@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Marcel Apfelbaum <marcel@redhat.com>

Cao jin (10):
  msix: Follow CODING_STYLE
  hcd-xhci: check & correct param before using it
  pci: Convert msix_init() to Error and fix callers to check it
  megasas: change behaviour of msix switch
  hcd-xhci: change behaviour of msix switch
  megasas: remove unnecessary megasas_use_msix()
  megasas: undo the overwrites of msi user configuration
  vmxnet3: fix reference leak issue
  vmxnet3: remove unnecessary internal msix flag
  msi_init: convert assert to return -errno

 hw/block/nvme.c        |  5 +++-
 hw/misc/ivshmem.c      |  8 +++---
 hw/net/e1000e.c        |  6 ++++-
 hw/net/rocker/rocker.c |  7 ++++-
 hw/net/vmxnet3.c       | 46 +++++++++++++++------------------
 hw/pci/msi.c           |  9 ++++---
 hw/pci/msix.c          | 42 +++++++++++++++++++++++++-----
 hw/scsi/megasas.c      | 49 ++++++++++++++++++++---------------
 hw/usb/hcd-xhci.c      | 69 ++++++++++++++++++++++++++++++--------------------
 hw/vfio/pci.c          |  8 ++++--
 hw/virtio/virtio-pci.c | 11 ++++----
 include/hw/pci/msix.h  |  5 ++--
 12 files changed, 164 insertions(+), 101 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH v7 01/10] msix: Follow CODING_STYLE

[Cao jin]

CC: Markus Armbruster <armbru@redhat.com>
CC: Marcel Apfelbaum <marcel@redhat.com>
CC: Michael S. Tsirkin <mst@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/pci/msix.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/pci/msix.c b/hw/pci/msix.c
index 0ec1cb1..0cee631 100644
--- a/hw/pci/msix.c
+++ b/hw/pci/msix.c
@@ -447,8 +447,10 @@ void msix_notify(PCIDevice *dev, unsigned vector)
 {
     MSIMessage msg;
 
-    if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector])
+    if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector]) {
         return;
+    }
+
     if (msix_is_masked(dev, vector)) {
         msix_set_pending(dev, vector);
         return;
@@ -483,8 +485,10 @@ void msix_reset(PCIDevice *dev)
 /* Mark vector as used. */
 int msix_vector_use(PCIDevice *dev, unsigned vector)
 {
-    if (vector >= dev->msix_entries_nr)
+    if (vector >= dev->msix_entries_nr) {
         return -EINVAL;
+    }
+
     dev->msix_entry_used[vector]++;
     return 0;
 }
-- 
2.1.0

[Qemu-devel] [PATCH v7 02/10] hcd-xhci: check & correct param before using it

[Cao jin]

usb_xhci_realize() corrects invalid values of property "intrs"
automatically, but the uncorrected value is passed to msi_init(),
which chokes on invalid values.  Delay that until after the
correction.

Resources allocated by usb_xhci_init() are leaked when msi_init()
fails.  Fix by calling it after msi_init().

CC: Gerd Hoffmann <kraxel@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Marcel Apfelbaum <marcel@redhat.com>
CC: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
In previous rounds, usb_xhci_init() is moved too far from its original place,
which results the segfault(XHCIState->numports is initialized in this func),
now move it adjacent to msi_init code hunk.

 hw/usb/hcd-xhci.c | 37 ++++++++++++++++++-------------------
 1 file changed, 18 insertions(+), 19 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 4acf0c6..0ace273 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -3627,25 +3627,6 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
     dev->config[PCI_CACHE_LINE_SIZE] = 0x10;
     dev->config[0x60] = 0x30; /* release number */
 
-    usb_xhci_init(xhci);
-
-    if (xhci->msi != ON_OFF_AUTO_OFF) {
-        ret = msi_init(dev, 0x70, xhci->numintrs, true, false, &err);
-        /* Any error other than -ENOTSUP(board's MSI support is broken)
-         * is a programming error */
-        assert(!ret || ret == -ENOTSUP);
-        if (ret && xhci->msi == ON_OFF_AUTO_ON) {
-            /* Can't satisfy user's explicit msi=on request, fail */
-            error_append_hint(&err, "You have to use msi=auto (default) or "
-                    "msi=off with this machine type.\n");
-            error_propagate(errp, err);
-            return;
-        }
-        assert(!err || xhci->msi == ON_OFF_AUTO_AUTO);
-        /* With msi=auto, we fall back to MSI off silently */
-        error_free(err);
-    }
-
     if (xhci->numintrs > MAXINTRS) {
         xhci->numintrs = MAXINTRS;
     }
@@ -3667,6 +3648,24 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
         xhci->max_pstreams_mask = 0;
     }
 
+    if (xhci->msi != ON_OFF_AUTO_OFF) {
+        ret = msi_init(dev, 0x70, xhci->numintrs, true, false, &err);
+        /* Any error other than -ENOTSUP(board's MSI support is broken)
+         * is a programming error */
+        assert(!ret || ret == -ENOTSUP);
+        if (ret && xhci->msi == ON_OFF_AUTO_ON) {
+            /* Can't satisfy user's explicit msi=on request, fail */
+            error_append_hint(&err, "You have to use msi=auto (default) or "
+                    "msi=off with this machine type.\n");
+            error_propagate(errp, err);
+            return;
+        }
+        assert(!err || xhci->msi == ON_OFF_AUTO_AUTO);
+        /* With msi=auto, we fall back to MSI off silently */
+        error_free(err);
+    }
+
+    usb_xhci_init(xhci);
     xhci->mfwrap_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, xhci_mfwrap_timer, xhci);
 
     memory_region_init(&xhci->mem, OBJECT(xhci), "xhci", LEN_REGS);
-- 
2.1.0

[Qemu-devel] [PATCH v7 03/10] pci: Convert msix_init() to Error and fix callers to check it

[Cao jin]

msix_init() reports errors with error_report(), which is wrong when
it's used in realize().  The same issue was fixed for msi_init() in
commit 1108b2f.

For some devices(like e1000e, vmxnet3) who won't fail because of
msix_init's failure, suppress the error report by passing NULL error object.

Bonus: add comment for msix_init.

CC: Jiri Pirko <jiri@resnulli.us>
CC: Gerd Hoffmann <kraxel@redhat.com>
CC: Dmitry Fleytman <dmitry@daynix.com>
CC: Jason Wang <jasowang@redhat.com>
CC: Michael S. Tsirkin <mst@redhat.com>
CC: Hannes Reinecke <hare@suse.de>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Alex Williamson <alex.williamson@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Marcel Apfelbaum <marcel@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Hannes Reinecke <hare@suse.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/block/nvme.c        |  5 ++++-
 hw/misc/ivshmem.c      |  8 ++++----
 hw/net/e1000e.c        |  6 +++++-
 hw/net/rocker/rocker.c |  7 ++++++-
 hw/net/vmxnet3.c       |  8 ++++++--
 hw/pci/msix.c          | 34 +++++++++++++++++++++++++++++-----
 hw/scsi/megasas.c      |  5 ++++-
 hw/usb/hcd-xhci.c      | 13 ++++++++-----
 hw/vfio/pci.c          |  8 ++++++--
 hw/virtio/virtio-pci.c | 11 +++++------
 include/hw/pci/msix.h  |  5 +++--
 11 files changed, 80 insertions(+), 30 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index d479fd2..2d703c8 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -831,6 +831,7 @@ static int nvme_init(PCIDevice *pci_dev)
 {
     NvmeCtrl *n = NVME(pci_dev);
     NvmeIdCtrl *id = &n->id_ctrl;
+    Error *err = NULL;
 
     int i;
     int64_t bs_size;
@@ -872,7 +873,9 @@ static int nvme_init(PCIDevice *pci_dev)
     pci_register_bar(&n->parent_obj, 0,
         PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_64,
         &n->iomem);
-    msix_init_exclusive_bar(&n->parent_obj, n->num_queues, 4);
+    if (msix_init_exclusive_bar(&n->parent_obj, n->num_queues, 4, &err)) {
+        error_report_err(err);
+    }
 
     id->vid = cpu_to_le16(pci_get_word(pci_conf + PCI_VENDOR_ID));
     id->ssvid = cpu_to_le16(pci_get_word(pci_conf + PCI_SUBSYSTEM_VENDOR_ID));
diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index 230e51b..ca6f804 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -749,13 +749,13 @@ static void ivshmem_reset(DeviceState *d)
     }
 }
 
-static int ivshmem_setup_interrupts(IVShmemState *s)
+static int ivshmem_setup_interrupts(IVShmemState *s, Error **errp)
 {
     /* allocate QEMU callback data for receiving interrupts */
     s->msi_vectors = g_malloc0(s->vectors * sizeof(MSIVector));
 
     if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
-        if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1)) {
+        if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1, errp)) {
             return -1;
         }
 
@@ -897,8 +897,8 @@ static void ivshmem_common_realize(PCIDevice *dev, Error **errp)
         qemu_chr_fe_set_handlers(&s->server_chr, ivshmem_can_receive,
                                  ivshmem_read, NULL, s, NULL, true);
 
-        if (ivshmem_setup_interrupts(s) < 0) {
-            error_setg(errp, "failed to initialize interrupts");
+        if (ivshmem_setup_interrupts(s, errp) < 0) {
+            error_prepend(errp, "Failed to initialize interrupts: ");
             return;
         }
     }
diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
index 4994e1c..74cbbef 100644
--- a/hw/net/e1000e.c
+++ b/hw/net/e1000e.c
@@ -292,7 +292,11 @@ e1000e_init_msix(E1000EState *s)
                         E1000E_MSIX_IDX, E1000E_MSIX_TABLE,
                         &s->msix,
                         E1000E_MSIX_IDX, E1000E_MSIX_PBA,
-                        0xA0);
+                        0xA0, NULL);
+
+    /* Any error other than -ENOTSUP(board's MSI support is broken)
+     * is a programming error. Fall back to INTx silently on -ENOTSUP */
+    assert(!res || res == -ENOTSUP);
 
     if (res < 0) {
         trace_e1000e_msix_init_fail(res);
diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
index e9d215a..8f829f2 100644
--- a/hw/net/rocker/rocker.c
+++ b/hw/net/rocker/rocker.c
@@ -1256,14 +1256,19 @@ static int rocker_msix_init(Rocker *r)
 {
     PCIDevice *dev = PCI_DEVICE(r);
     int err;
+    Error *local_err = NULL;
 
     err = msix_init(dev, ROCKER_MSIX_VEC_COUNT(r->fp_ports),
                     &r->msix_bar,
                     ROCKER_PCI_MSIX_BAR_IDX, ROCKER_PCI_MSIX_TABLE_OFFSET,
                     &r->msix_bar,
                     ROCKER_PCI_MSIX_BAR_IDX, ROCKER_PCI_MSIX_PBA_OFFSET,
-                    0);
+                    0, &local_err);
+    /* Any error other than -ENOTSUP(board's MSI support is broken)
+     * is a programming error. */
+    assert(!err || err == -ENOTSUP);
     if (err) {
+        error_report_err(local_err);
         return err;
     }
 
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 92f6af9..a433cc0 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -2191,10 +2191,14 @@ vmxnet3_init_msix(VMXNET3State *s)
                         VMXNET3_MSIX_BAR_IDX, VMXNET3_OFF_MSIX_TABLE,
                         &s->msix_bar,
                         VMXNET3_MSIX_BAR_IDX, VMXNET3_OFF_MSIX_PBA(s),
-                        VMXNET3_MSIX_OFFSET(s));
+                        VMXNET3_MSIX_OFFSET(s), NULL);
+
+    /* Any error other than -ENOTSUP(board's MSI support is broken)
+     * is a programming error. Fall back to INTx on -ENOTSUP */
+    assert(!res || res == -ENOTSUP);
 
     if (0 > res) {
-        VMW_WRPRN("Failed to initialize MSI-X, error %d", res);
+        VMW_WRPRN("Failed to initialize MSI-X, board's MSI support is broken");
         s->msix_used = false;
     } else {
         if (!vmxnet3_use_msix_vectors(s, VMXNET3_MAX_INTRS)) {
diff --git a/hw/pci/msix.c b/hw/pci/msix.c
index 0cee631..d03016f 100644
--- a/hw/pci/msix.c
+++ b/hw/pci/msix.c
@@ -21,6 +21,7 @@
 #include "hw/pci/pci.h"
 #include "hw/xen/xen.h"
 #include "qemu/range.h"
+#include "qapi/error.h"
 
 #define MSIX_CAP_LENGTH 12
 
@@ -238,11 +239,29 @@ static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
     }
 }
 
-/* Initialize the MSI-X structures */
+/* Make PCI device @dev MSI-X capable
+ * @nentries is the max number of MSI-X vectors that the device support.
+ * @table_bar is the MemoryRegion that MSI-X table structure resides.
+ * @table_bar_nr is number of base address register corresponding to @table_bar.
+ * @table_offset indicates the offset that the MSI-X table structure starts with
+ * in @table_bar.
+ * @pba_bar is the MemoryRegion that the Pending Bit Array structure resides.
+ * @pba_bar_nr is number of base address register corresponding to @pba_bar.
+ * @pba_offset indicates the offset that the Pending Bit Array structure
+ * starts with in @pba_bar.
+ * Non-zero @cap_pos puts capability MSI-X at that offset in PCI config space.
+ * @errp is for returning errors.
+ *
+ * Return 0 on success; set @errp and return -errno on error:
+ * -ENOTSUP means lacking msi support for a msi-capable platform.
+ * -EINVAL means capability overlap, happens when @cap_pos is non-zero,
+ * also means a programming error, except device assignment, which can check
+ * if a real HW is broken.*/
 int msix_init(struct PCIDevice *dev, unsigned short nentries,
               MemoryRegion *table_bar, uint8_t table_bar_nr,
               unsigned table_offset, MemoryRegion *pba_bar,
-              uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos)
+              uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos,
+              Error **errp)
 {
     int cap;
     unsigned table_size, pba_size;
@@ -250,10 +269,12 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
 
     /* Nothing to do if MSI is not supported by interrupt controller */
     if (!msi_nonbroken) {
+        error_setg(errp, "MSI-X is not supported by interrupt controller");
         return -ENOTSUP;
     }
 
     if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1) {
+        error_setg(errp, "The number of MSI-X vectors is invalid");
         return -EINVAL;
     }
 
@@ -266,10 +287,13 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
         table_offset + table_size > memory_region_size(table_bar) ||
         pba_offset + pba_size > memory_region_size(pba_bar) ||
         (table_offset | pba_offset) & PCI_MSIX_FLAGS_BIRMASK) {
+        error_setg(errp, "table & pba overlap, or they don't fit in BARs,"
+                   " or don't align");
         return -EINVAL;
     }
 
-    cap = pci_add_capability(dev, PCI_CAP_ID_MSIX, cap_pos, MSIX_CAP_LENGTH);
+    cap = pci_add_capability2(dev, PCI_CAP_ID_MSIX,
+                              cap_pos, MSIX_CAP_LENGTH, errp);
     if (cap < 0) {
         return cap;
     }
@@ -306,7 +330,7 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
 }
 
 int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
-                            uint8_t bar_nr)
+                            uint8_t bar_nr, Error **errp)
 {
     int ret;
     char *name;
@@ -338,7 +362,7 @@ int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
     ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr,
                     0, &dev->msix_exclusive_bar,
                     bar_nr, bar_pba_offset,
-                    0);
+                    0, errp);
     if (ret) {
         return ret;
     }
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 52a4123..fada834 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -2360,9 +2360,12 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
 
     if (megasas_use_msix(s) &&
         msix_init(dev, 15, &s->mmio_io, b->mmio_bar, 0x2000,
-                  &s->mmio_io, b->mmio_bar, 0x3800, 0x68)) {
+                  &s->mmio_io, b->mmio_bar, 0x3800, 0x68, &err)) {
+        /*TODO: check msix_init's error, and should fail on msix=on */
+        error_report_err(err);
         s->msix = ON_OFF_AUTO_OFF;
     }
+
     if (pci_is_express(dev)) {
         pcie_endpoint_cap_init(dev, 0xa0);
     }
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 0ace273..153b006 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -3703,11 +3703,14 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
     }
 
     if (xhci->msix != ON_OFF_AUTO_OFF) {
-        /* TODO check for errors */
-        msix_init(dev, xhci->numintrs,
-                  &xhci->mem, 0, OFF_MSIX_TABLE,
-                  &xhci->mem, 0, OFF_MSIX_PBA,
-                  0x90);
+        /* TODO check for errors, and should fail when msix=on */
+        ret = msix_init(dev, xhci->numintrs,
+                        &xhci->mem, 0, OFF_MSIX_TABLE,
+                        &xhci->mem, 0, OFF_MSIX_PBA,
+                        0x90, &err);
+        if (ret) {
+            error_report_err(err);
+        }
     }
 }
 
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index d7dbe0e..0bcfaad 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1432,6 +1432,7 @@ static void vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
 static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
 {
     int ret;
+    Error *local_err = NULL;
 
     vdev->msix->pending = g_malloc0(BITS_TO_LONGS(vdev->msix->entries) *
                                     sizeof(unsigned long));
@@ -1439,12 +1440,15 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
                     vdev->bars[vdev->msix->table_bar].region.mem,
                     vdev->msix->table_bar, vdev->msix->table_offset,
                     vdev->bars[vdev->msix->pba_bar].region.mem,
-                    vdev->msix->pba_bar, vdev->msix->pba_offset, pos);
+                    vdev->msix->pba_bar, vdev->msix->pba_offset, pos,
+                    &local_err);
     if (ret < 0) {
         if (ret == -ENOTSUP) {
+            error_report_err(local_err);
             return 0;
         }
-        error_setg(errp, "msix_init failed");
+
+        error_propagate(errp, local_err);
         return ret;
     }
 
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 06831de..5acce38 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1693,13 +1693,12 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
 
     if (proxy->nvectors) {
         int err = msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors,
-                                          proxy->msix_bar_idx);
+                                          proxy->msix_bar_idx, errp);
+        /* Any error other than -ENOTSUP(board's MSI support is broken)
+         * is a programming error. */
+        assert(!err || err == -ENOTSUP);
         if (err) {
-            /* Notice when a system that supports MSIx can't initialize it.  */
-            if (err != -ENOTSUP) {
-                error_report("unable to init msix vectors to %" PRIu32,
-                             proxy->nvectors);
-            }
+            error_report_err(*errp);
             proxy->nvectors = 0;
         }
     }
diff --git a/include/hw/pci/msix.h b/include/hw/pci/msix.h
index 048a29d..1f27658 100644
--- a/include/hw/pci/msix.h
+++ b/include/hw/pci/msix.h
@@ -9,9 +9,10 @@ MSIMessage msix_get_message(PCIDevice *dev, unsigned int vector);
 int msix_init(PCIDevice *dev, unsigned short nentries,
               MemoryRegion *table_bar, uint8_t table_bar_nr,
               unsigned table_offset, MemoryRegion *pba_bar,
-              uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos);
+              uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos,
+              Error **errp);
 int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
-                            uint8_t bar_nr);
+                            uint8_t bar_nr, Error **errp);
 
 void msix_write_config(PCIDevice *dev, uint32_t address, uint32_t val, int len);
 
-- 
2.1.0

[Qemu-devel] [PATCH v7 04/10] megasas: change behaviour of msix switch

[Cao jin]

Resolve the TODO, msix=auto means msix on; if user specify msix=on,
then device creation fail on msix_init failure.
Also undo the overwrites of user configuration of msix.

CC: Michael S. Tsirkin <mst@redhat.com>
CC: Hannes Reinecke <hare@suse.de>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Marcel Apfelbaum <marcel@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Hannes Reinecke <hare@suse.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/scsi/megasas.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index fada834..6cef9a3 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -2353,19 +2353,31 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
 
     memory_region_init_io(&s->mmio_io, OBJECT(s), &megasas_mmio_ops, s,
                           "megasas-mmio", 0x4000);
+    if (megasas_use_msix(s)) {
+        ret = msix_init(dev, 15, &s->mmio_io, b->mmio_bar, 0x2000,
+                        &s->mmio_io, b->mmio_bar, 0x3800, 0x68, &err);
+        /* Any error other than -ENOTSUP(board's MSI support is broken)
+         * is a programming error */
+        assert(!ret || ret == -ENOTSUP);
+        if (ret && s->msix == ON_OFF_AUTO_ON) {
+            /* Can't satisfy user's explicit msix=on request, fail */
+            error_append_hint(&err, "You have to use msix=auto (default) or "
+                    "msix=off with this machine type.\n");
+            /* No instance_finalize method, need to free the resource here */
+            object_unref(OBJECT(&s->mmio_io));
+            error_propagate(errp, err);
+            return;
+        }
+        assert(!err || s->msix == ON_OFF_AUTO_AUTO);
+        /* With msix=auto, we fall back to MSI off silently */
+        error_free(err);
+    }
+
     memory_region_init_io(&s->port_io, OBJECT(s), &megasas_port_ops, s,
                           "megasas-io", 256);
     memory_region_init_io(&s->queue_io, OBJECT(s), &megasas_queue_ops, s,
                           "megasas-queue", 0x40000);
 
-    if (megasas_use_msix(s) &&
-        msix_init(dev, 15, &s->mmio_io, b->mmio_bar, 0x2000,
-                  &s->mmio_io, b->mmio_bar, 0x3800, 0x68, &err)) {
-        /*TODO: check msix_init's error, and should fail on msix=on */
-        error_report_err(err);
-        s->msix = ON_OFF_AUTO_OFF;
-    }
-
     if (pci_is_express(dev)) {
         pcie_endpoint_cap_init(dev, 0xa0);
     }
-- 
2.1.0

[Qemu-devel] [PATCH v7 05/10] hcd-xhci: change behaviour of msix switch

[Cao jin]

Resolve the TODO, msix=auto means msix on; if user specify msix=on,
then device creation fail on msix_init failure.

CC: Gerd Hoffmann <kraxel@redhat.com>
CC: Michael S. Tsirkin <mst@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Marcel Apfelbaum <marcel@redhat.com>

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/usb/hcd-xhci.c | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 153b006..aaca57c 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -3636,12 +3636,14 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
     if (xhci->numintrs < 1) {
         xhci->numintrs = 1;
     }
+
     if (xhci->numslots > MAXSLOTS) {
         xhci->numslots = MAXSLOTS;
     }
     if (xhci->numslots < 1) {
         xhci->numslots = 1;
     }
+
     if (xhci_get_flag(xhci, XHCI_FLAG_ENABLE_STREAMS)) {
         xhci->max_pstreams_mask = 7; /* == 256 primary streams */
     } else {
@@ -3669,6 +3671,28 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
     xhci->mfwrap_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, xhci_mfwrap_timer, xhci);
 
     memory_region_init(&xhci->mem, OBJECT(xhci), "xhci", LEN_REGS);
+    if (xhci->msix != ON_OFF_AUTO_OFF) {
+        ret = msix_init(dev, xhci->numintrs,
+                        &xhci->mem, 0, OFF_MSIX_TABLE,
+                        &xhci->mem, 0, OFF_MSIX_PBA,
+                        0x90, &err);
+        /* Any error other than -ENOTSUP(board's MSI support is broken)
+         * is a programming error */
+        assert(!ret || ret == -ENOTSUP);
+        if (ret && xhci->msix == ON_OFF_AUTO_ON) {
+            /* Can't satisfy user's explicit msix=on request, fail */
+            error_append_hint(&err, "You have to use msix=auto (default) or "
+                    "msix=off with this machine type.\n");
+            /* No instance_finalize method, need to free the resource here */
+            object_unref(OBJECT(&xhci->mem));
+            error_propagate(errp, err);
+            return;
+        }
+        assert(!err || xhci->msix == ON_OFF_AUTO_AUTO);
+        /* With msix=auto, we fall back to MSI off silently */
+        error_free(err);
+    }
+
     memory_region_init_io(&xhci->mem_cap, OBJECT(xhci), &xhci_cap_ops, xhci,
                           "capabilities", LEN_CAP);
     memory_region_init_io(&xhci->mem_oper, OBJECT(xhci), &xhci_oper_ops, xhci,
@@ -3701,17 +3725,6 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
         ret = pcie_endpoint_cap_init(dev, 0xa0);
         assert(ret >= 0);
     }
-
-    if (xhci->msix != ON_OFF_AUTO_OFF) {
-        /* TODO check for errors, and should fail when msix=on */
-        ret = msix_init(dev, xhci->numintrs,
-                        &xhci->mem, 0, OFF_MSIX_TABLE,
-                        &xhci->mem, 0, OFF_MSIX_PBA,
-                        0x90, &err);
-        if (ret) {
-            error_report_err(err);
-        }
-    }
 }
 
 static void usb_xhci_exit(PCIDevice *dev)
-- 
2.1.0

[Qemu-devel] [PATCH v7 06/10] megasas: remove unnecessary megasas_use_msix()

[Cao jin]

Also move certain hunk above, to place msix init related code together.

CC: Hannes Reinecke <hare@suse.de>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Marcel Apfelbaum <marcel@redhat.com>
CC: Michael S. Tsirkin <mst@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Hannes Reinecke <hare@suse.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/scsi/megasas.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 6cef9a3..ba79e7a 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -155,11 +155,6 @@ static bool megasas_use_queue64(MegasasState *s)
     return s->flags & MEGASAS_MASK_USE_QUEUE64;
 }
 
-static bool megasas_use_msix(MegasasState *s)
-{
-    return s->msix != ON_OFF_AUTO_OFF;
-}
-
 static bool megasas_is_jbod(MegasasState *s)
 {
     return s->flags & MEGASAS_MASK_USE_JBOD;
@@ -2299,9 +2294,7 @@ static void megasas_scsi_uninit(PCIDevice *d)
 {
     MegasasState *s = MEGASAS(d);
 
-    if (megasas_use_msix(s)) {
-        msix_uninit(d, &s->mmio_io, &s->mmio_io);
-    }
+    msix_uninit(d, &s->mmio_io, &s->mmio_io);
     msi_uninit(d);
 }
 
@@ -2353,7 +2346,7 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
 
     memory_region_init_io(&s->mmio_io, OBJECT(s), &megasas_mmio_ops, s,
                           "megasas-mmio", 0x4000);
-    if (megasas_use_msix(s)) {
+    if (s->msix != ON_OFF_AUTO_OFF) {
         ret = msix_init(dev, 15, &s->mmio_io, b->mmio_bar, 0x2000,
                         &s->mmio_io, b->mmio_bar, 0x3800, 0x68, &err);
         /* Any error other than -ENOTSUP(board's MSI support is broken)
@@ -2373,6 +2366,10 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
         error_free(err);
     }
 
+    if (msix_enabled(dev)) {
+        msix_vector_use(dev, 0);
+    }
+
     memory_region_init_io(&s->port_io, OBJECT(s), &megasas_port_ops, s,
                           "megasas-io", 256);
     memory_region_init_io(&s->queue_io, OBJECT(s), &megasas_queue_ops, s,
@@ -2388,10 +2385,6 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
     pci_register_bar(dev, b->mmio_bar, bar_type, &s->mmio_io);
     pci_register_bar(dev, 3, bar_type, &s->queue_io);
 
-    if (megasas_use_msix(s)) {
-        msix_vector_use(dev, 0);
-    }
-
     s->fw_state = MFI_FWSTATE_READY;
     if (!s->sas_addr) {
         s->sas_addr = ((NAA_LOCALLY_ASSIGNED_ID << 24) |
-- 
2.1.0

[Qemu-devel] [PATCH v7 07/10] megasas: undo the overwrites of msi user configuration

[Cao jin]

Commit afea4e14 seems forgetting to undo the overwrites, which is
unsuitable.

CC: Hannes Reinecke <hare@suse.de>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Marcel Apfelbaum <marcel@redhat.com>
CC: Michael S. Tsirkin <mst@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Hannes Reinecke <hare@suse.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/scsi/megasas.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index ba79e7a..bbef9e9 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -2337,11 +2337,10 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
                     "msi=off with this machine type.\n");
             error_propagate(errp, err);
             return;
-        } else if (ret) {
-            /* With msi=auto, we fall back to MSI off silently */
-            s->msi = ON_OFF_AUTO_OFF;
-            error_free(err);
         }
+        assert(!err || s->msix == ON_OFF_AUTO_AUTO);
+        /* With msi=auto, we fall back to MSI off silently */
+        error_free(err);
     }
 
     memory_region_init_io(&s->mmio_io, OBJECT(s), &megasas_mmio_ops, s,
-- 
2.1.0

[Qemu-devel] [PATCH v7 08/10] vmxnet3: fix reference leak issue

[Cao jin]

On migration target, msix_vector_use() will be called in vmxnet3_post_load()
in second time, without a matching second call to msi_vector_unuse(),
which results in vector reference leak.

CC: Dmitry Fleytman <dmitry@daynix.com>
CC: Jason Wang <jasowang@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Michael S. Tsirkin <mst@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Dmitry Fleytman <dmitry@daynix.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/net/vmxnet3.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index a433cc0..45e125e 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -2552,21 +2552,11 @@ static void vmxnet3_put_rxq_descr(QEMUFile *f, void *pv, size_t size)
 static int vmxnet3_post_load(void *opaque, int version_id)
 {
     VMXNET3State *s = opaque;
-    PCIDevice *d = PCI_DEVICE(s);
 
     net_tx_pkt_init(&s->tx_pkt, PCI_DEVICE(s),
                     s->max_tx_frags, s->peer_has_vhdr);
     net_rx_pkt_init(&s->rx_pkt, s->peer_has_vhdr);
 
-    if (s->msix_used) {
-        if  (!vmxnet3_use_msix_vectors(s, VMXNET3_MAX_INTRS)) {
-            VMW_WRPRN("Failed to re-use MSI-X vectors");
-            msix_uninit(d, &s->msix_bar, &s->msix_bar);
-            s->msix_used = false;
-            return -1;
-        }
-    }
-
     vmxnet3_validate_queues(s);
     vmxnet3_validate_interrupts(s);
 
-- 
2.1.0

[Qemu-devel] [PATCH v7 09/10] vmxnet3: remove unnecessary internal msix flag

[Cao jin]

Internal flag msix_used is unnecessary, it has the same effect as
msix_enabled().

The corresponding msi flag is already dropped in commit 1070048e.

CC: Dmitry Fleytman <dmitry@daynix.com>
CC: Jason Wang <jasowang@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Michael S. Tsirkin <mst@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Dmitry Fleytman <dmitry@daynix.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/net/vmxnet3.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 45e125e..af39965 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -281,8 +281,6 @@ typedef struct {
         Vmxnet3RxqDescr rxq_descr[VMXNET3_DEVICE_MAX_RX_QUEUES];
         Vmxnet3TxqDescr txq_descr[VMXNET3_DEVICE_MAX_TX_QUEUES];
 
-        /* Whether MSI-X support was installed successfully */
-        bool msix_used;
         hwaddr drv_shmem;
         hwaddr temp_shared_guest_driver_memory;
 
@@ -359,7 +357,7 @@ static bool _vmxnet3_assert_interrupt_line(VMXNET3State *s, uint32_t int_idx)
 {
     PCIDevice *d = PCI_DEVICE(s);
 
-    if (s->msix_used && msix_enabled(d)) {
+    if (msix_enabled(d)) {
         VMW_IRPRN("Sending MSI-X notification for vector %u", int_idx);
         msix_notify(d, int_idx);
         return false;
@@ -383,7 +381,7 @@ static void _vmxnet3_deassert_interrupt_line(VMXNET3State *s, int lidx)
      * This function should never be called for MSI(X) interrupts
      * because deassertion never required for message interrupts
      */
-    assert(!s->msix_used || !msix_enabled(d));
+    assert(!msix_enabled(d));
     /*
      * This function should never be called for MSI(X) interrupts
      * because deassertion never required for message interrupts
@@ -421,7 +419,7 @@ static void vmxnet3_trigger_interrupt(VMXNET3State *s, int lidx)
     s->interrupt_states[lidx].is_pending = true;
     vmxnet3_update_interrupt_line_state(s, lidx);
 
-    if (s->msix_used && msix_enabled(d) && s->auto_int_masking) {
+    if (msix_enabled(d) && s->auto_int_masking) {
         goto do_automask;
     }
 
@@ -1428,7 +1426,7 @@ static void vmxnet3_update_features(VMXNET3State *s)
 
 static bool vmxnet3_verify_intx(VMXNET3State *s, int intx)
 {
-    return s->msix_used || msi_enabled(PCI_DEVICE(s))
+    return msix_enabled(PCI_DEVICE(s)) || msi_enabled(PCI_DEVICE(s))
         || intx == pci_get_byte(s->parent_obj.config + PCI_INTERRUPT_PIN) - 1;
 }
 
@@ -1445,18 +1443,18 @@ static void vmxnet3_validate_interrupts(VMXNET3State *s)
     int i;
 
     VMW_CFPRN("Verifying event interrupt index (%d)", s->event_int_idx);
-    vmxnet3_validate_interrupt_idx(s->msix_used, s->event_int_idx);
+    vmxnet3_validate_interrupt_idx(msix_enabled(PCI_DEVICE(s)), s->event_int_idx);
 
     for (i = 0; i < s->txq_num; i++) {
         int idx = s->txq_descr[i].intr_idx;
         VMW_CFPRN("Verifying TX queue %d interrupt index (%d)", i, idx);
-        vmxnet3_validate_interrupt_idx(s->msix_used, idx);
+        vmxnet3_validate_interrupt_idx(msix_enabled(PCI_DEVICE(s)), idx);
     }
 
     for (i = 0; i < s->rxq_num; i++) {
         int idx = s->rxq_descr[i].intr_idx;
         VMW_CFPRN("Verifying RX queue %d interrupt index (%d)", i, idx);
-        vmxnet3_validate_interrupt_idx(s->msix_used, idx);
+        vmxnet3_validate_interrupt_idx(msix_enabled(PCI_DEVICE(s)), idx);
     }
 }
 
@@ -2185,6 +2183,7 @@ vmxnet3_use_msix_vectors(VMXNET3State *s, int num_vectors)
 static bool
 vmxnet3_init_msix(VMXNET3State *s)
 {
+    bool msix;
     PCIDevice *d = PCI_DEVICE(s);
     int res = msix_init(d, VMXNET3_MAX_INTRS,
                         &s->msix_bar,
@@ -2199,17 +2198,18 @@ vmxnet3_init_msix(VMXNET3State *s)
 
     if (0 > res) {
         VMW_WRPRN("Failed to initialize MSI-X, board's MSI support is broken");
-        s->msix_used = false;
+        msix = false;
     } else {
         if (!vmxnet3_use_msix_vectors(s, VMXNET3_MAX_INTRS)) {
             VMW_WRPRN("Failed to use MSI-X vectors, error %d", res);
             msix_uninit(d, &s->msix_bar, &s->msix_bar);
-            s->msix_used = false;
+            msix = false;
         } else {
-            s->msix_used = true;
+            msix = true;
         }
     }
-    return s->msix_used;
+
+    return msix;
 }
 
 static void
@@ -2217,7 +2217,7 @@ vmxnet3_cleanup_msix(VMXNET3State *s)
 {
     PCIDevice *d = PCI_DEVICE(s);
 
-    if (s->msix_used) {
+    if (msix_enabled(d)) {
         vmxnet3_unuse_msix_vectors(s, VMXNET3_MAX_INTRS);
         msix_uninit(d, &s->msix_bar, &s->msix_bar);
     }
-- 
2.1.0

[Qemu-devel] [PATCH v7 10/10] msi_init: convert assert to return -errno

[Cao jin]

According to the disscussion:
http://lists.nongnu.org/archive/html/qemu-devel/2016-09/msg08215.html

Let leaf function returns reasonable -errno, let caller decide how to
handle the return value.

Suggested-by: Markus Armbruster <armbru@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Michael S. Tsirkin <mst@redhat.com>
CC: Marcel Apfelbaum <marcel@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/pci/msi.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/pci/msi.c b/hw/pci/msi.c
index a87b227..af3efbe 100644
--- a/hw/pci/msi.c
+++ b/hw/pci/msi.c
@@ -201,9 +201,12 @@ int msi_init(struct PCIDevice *dev, uint8_t offset,
                    " 64bit %d mask %d\n",
                    offset, nr_vectors, msi64bit, msi_per_vector_mask);
 
-    assert(!(nr_vectors & (nr_vectors - 1)));   /* power of 2 */
-    assert(nr_vectors > 0);
-    assert(nr_vectors <= PCI_MSI_VECTORS_MAX);
+    /* vector sanity test: should in range 1 - 32, should be power of 2 */
+    if (!is_power_of_2(nr_vectors) || nr_vectors > PCI_MSI_VECTORS_MAX) {
+        error_setg(errp, "Invalid vector number: %d", nr_vectors);
+        return -EINVAL;
+    }
+
     /* the nr of MSI vectors is up to 32 */
     vectors_order = ctz32(nr_vectors);
 
-- 
2.1.0

Re: [Qemu-devel] [PATCH v7 00/10] Convert msix_init() to error

[Michael S. Tsirkin]

On Mon, Nov 14, 2016 at 03:25:30PM +0800, Cao jin wrote:
> v7 changelog:
> 1. fix the segfaut bug in patch 2. So drop the all the R-b of it,
>    please take a look, there is detailed description in the patch.
> 2. add the R-b from Hannes Reinecke

Pls remember to ping after release.

> Test:
> 1. make check: pass
> 2. After applied all the patch, command line test for all the
>    affected devices, just make sure device realize process is ok,
>    no crash, but no further use of device.
> CC: Jiri Pirko <jiri@resnulli.us>
> CC: Gerd Hoffmann <kraxel@redhat.com>
> CC: Dmitry Fleytman <dmitry@daynix.com>
> CC: Jason Wang <jasowang@redhat.com>
> CC: Michael S. Tsirkin <mst@redhat.com>
> CC: Hannes Reinecke <hare@suse.de>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Alex Williamson <alex.williamson@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Marcel Apfelbaum <marcel@redhat.com>
> 
> Cao jin (10):
>   msix: Follow CODING_STYLE
>   hcd-xhci: check & correct param before using it
>   pci: Convert msix_init() to Error and fix callers to check it
>   megasas: change behaviour of msix switch
>   hcd-xhci: change behaviour of msix switch
>   megasas: remove unnecessary megasas_use_msix()
>   megasas: undo the overwrites of msi user configuration
>   vmxnet3: fix reference leak issue
>   vmxnet3: remove unnecessary internal msix flag
>   msi_init: convert assert to return -errno
> 
>  hw/block/nvme.c        |  5 +++-
>  hw/misc/ivshmem.c      |  8 +++---
>  hw/net/e1000e.c        |  6 ++++-
>  hw/net/rocker/rocker.c |  7 ++++-
>  hw/net/vmxnet3.c       | 46 +++++++++++++++------------------
>  hw/pci/msi.c           |  9 ++++---
>  hw/pci/msix.c          | 42 +++++++++++++++++++++++++-----
>  hw/scsi/megasas.c      | 49 ++++++++++++++++++++---------------
>  hw/usb/hcd-xhci.c      | 69 ++++++++++++++++++++++++++++++--------------------
>  hw/vfio/pci.c          |  8 ++++--
>  hw/virtio/virtio-pci.c | 11 ++++----
>  include/hw/pci/msix.h  |  5 ++--
>  12 files changed, 164 insertions(+), 101 deletions(-)
> 
> -- 
> 2.1.0
> 
> 

Re: [Qemu-devel] [PATCH v7 02/10] hcd-xhci: check & correct param before using it

[Markus Armbruster]

Cao jin <caoj.fnst@cn.fujitsu.com> writes:

> usb_xhci_realize() corrects invalid values of property "intrs"
> automatically, but the uncorrected value is passed to msi_init(),
> which chokes on invalid values.  Delay that until after the
> correction.
>
> Resources allocated by usb_xhci_init() are leaked when msi_init()
> fails.  Fix by calling it after msi_init().
>
> CC: Gerd Hoffmann <kraxel@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Marcel Apfelbaum <marcel@redhat.com>
> CC: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
> In previous rounds, usb_xhci_init() is moved too far from its original place,
> which results the segfault(XHCIState->numports is initialized in this func),
> now move it adjacent to msi_init code hunk.

Thanks for explaining what you changed.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v7 00/10] Convert msix_init() to error

[Cao jin]

ping

On 11/14/2016 03:25 PM, Cao jin wrote:
> v7 changelog:
> 1. fix the segfaut bug in patch 2. So drop the all the R-b of it,
>    please take a look, there is detailed description in the patch.
> 2. add the R-b from Hannes Reinecke
> 
> Test:
> 1. make check: pass
> 2. After applied all the patch, command line test for all the
>    affected devices, just make sure device realize process is ok,
>    no crash, but no further use of device.
> 
> CC: Jiri Pirko <jiri@resnulli.us>
> CC: Gerd Hoffmann <kraxel@redhat.com>
> CC: Dmitry Fleytman <dmitry@daynix.com>
> CC: Jason Wang <jasowang@redhat.com>
> CC: Michael S. Tsirkin <mst@redhat.com>
> CC: Hannes Reinecke <hare@suse.de>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Alex Williamson <alex.williamson@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Marcel Apfelbaum <marcel@redhat.com>
> 
> Cao jin (10):
>   msix: Follow CODING_STYLE
>   hcd-xhci: check & correct param before using it
>   pci: Convert msix_init() to Error and fix callers to check it
>   megasas: change behaviour of msix switch
>   hcd-xhci: change behaviour of msix switch
>   megasas: remove unnecessary megasas_use_msix()
>   megasas: undo the overwrites of msi user configuration
>   vmxnet3: fix reference leak issue
>   vmxnet3: remove unnecessary internal msix flag
>   msi_init: convert assert to return -errno
> 
>  hw/block/nvme.c        |  5 +++-
>  hw/misc/ivshmem.c      |  8 +++---
>  hw/net/e1000e.c        |  6 ++++-
>  hw/net/rocker/rocker.c |  7 ++++-
>  hw/net/vmxnet3.c       | 46 +++++++++++++++------------------
>  hw/pci/msi.c           |  9 ++++---
>  hw/pci/msix.c          | 42 +++++++++++++++++++++++++-----
>  hw/scsi/megasas.c      | 49 ++++++++++++++++++++---------------
>  hw/usb/hcd-xhci.c      | 69 ++++++++++++++++++++++++++++++--------------------
>  hw/vfio/pci.c          |  8 ++++--
>  hw/virtio/virtio-pci.c | 11 ++++----
>  include/hw/pci/msix.h  |  5 ++--
>  12 files changed, 164 insertions(+), 101 deletions(-)
> 

-- 
Sincerely,
Cao jin

Re: [Qemu-devel] [PATCH v7 00/10] Convert msix_init() to error

[Marcel Apfelbaum]

On 12/21/2016 08:16 AM, Cao jin wrote:
> ping
>

Misses the
   Acked-by: Marcel Apfelbaum <marcel@redhat.com>
from V6, if it helps.

Thanks,
Marcel


> On 11/14/2016 03:25 PM, Cao jin wrote:
>> v7 changelog:
>> 1. fix the segfaut bug in patch 2. So drop the all the R-b of it,
>>    please take a look, there is detailed description in the patch.
>> 2. add the R-b from Hannes Reinecke
>>
>> Test:
>> 1. make check: pass
>> 2. After applied all the patch, command line test for all the
>>    affected devices, just make sure device realize process is ok,
>>    no crash, but no further use of device.
>>
>> CC: Jiri Pirko <jiri@resnulli.us>
>> CC: Gerd Hoffmann <kraxel@redhat.com>
>> CC: Dmitry Fleytman <dmitry@daynix.com>
>> CC: Jason Wang <jasowang@redhat.com>
>> CC: Michael S. Tsirkin <mst@redhat.com>
>> CC: Hannes Reinecke <hare@suse.de>
>> CC: Paolo Bonzini <pbonzini@redhat.com>
>> CC: Alex Williamson <alex.williamson@redhat.com>
>> CC: Markus Armbruster <armbru@redhat.com>
>> CC: Marcel Apfelbaum <marcel@redhat.com>
>>
>> Cao jin (10):
>>   msix: Follow CODING_STYLE
>>   hcd-xhci: check & correct param before using it
>>   pci: Convert msix_init() to Error and fix callers to check it
>>   megasas: change behaviour of msix switch
>>   hcd-xhci: change behaviour of msix switch
>>   megasas: remove unnecessary megasas_use_msix()
>>   megasas: undo the overwrites of msi user configuration
>>   vmxnet3: fix reference leak issue
>>   vmxnet3: remove unnecessary internal msix flag
>>   msi_init: convert assert to return -errno
>>
>>  hw/block/nvme.c        |  5 +++-
>>  hw/misc/ivshmem.c      |  8 +++---
>>  hw/net/e1000e.c        |  6 ++++-
>>  hw/net/rocker/rocker.c |  7 ++++-
>>  hw/net/vmxnet3.c       | 46 +++++++++++++++------------------
>>  hw/pci/msi.c           |  9 ++++---
>>  hw/pci/msix.c          | 42 +++++++++++++++++++++++++-----
>>  hw/scsi/megasas.c      | 49 ++++++++++++++++++++---------------
>>  hw/usb/hcd-xhci.c      | 69 ++++++++++++++++++++++++++++++--------------------
>>  hw/vfio/pci.c          |  8 ++++--
>>  hw/virtio/virtio-pci.c | 11 ++++----
>>  include/hw/pci/msix.h  |  5 ++--
>>  12 files changed, 164 insertions(+), 101 deletions(-)
>>
>

Re: [Qemu-devel] [PATCH v7 00/10] Convert msix_init() to error

[Michael S. Tsirkin]

On Mon, Nov 14, 2016 at 03:25:30PM +0800, Cao jin wrote:
> v7 changelog:
> 1. fix the segfaut bug in patch 2. So drop the all the R-b of it,
>    please take a look, there is detailed description in the patch.
> 2. add the R-b from Hannes Reinecke
> 
> Test:
> 1. make check: pass
> 2. After applied all the patch, command line test for all the
>    affected devices, just make sure device realize process is ok,
>    no crash, but no further use of device.

Consider the megasas device for example, don't you
need to test that the change actually does what
it's intended to do?


> CC: Jiri Pirko <jiri@resnulli.us>
> CC: Gerd Hoffmann <kraxel@redhat.com>
> CC: Dmitry Fleytman <dmitry@daynix.com>
> CC: Jason Wang <jasowang@redhat.com>
> CC: Michael S. Tsirkin <mst@redhat.com>
> CC: Hannes Reinecke <hare@suse.de>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Alex Williamson <alex.williamson@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Marcel Apfelbaum <marcel@redhat.com>
> 
> Cao jin (10):
>   msix: Follow CODING_STYLE
>   hcd-xhci: check & correct param before using it
>   pci: Convert msix_init() to Error and fix callers to check it
>   megasas: change behaviour of msix switch
>   hcd-xhci: change behaviour of msix switch
>   megasas: remove unnecessary megasas_use_msix()
>   megasas: undo the overwrites of msi user configuration
>   vmxnet3: fix reference leak issue
>   vmxnet3: remove unnecessary internal msix flag
>   msi_init: convert assert to return -errno
> 
>  hw/block/nvme.c        |  5 +++-
>  hw/misc/ivshmem.c      |  8 +++---
>  hw/net/e1000e.c        |  6 ++++-
>  hw/net/rocker/rocker.c |  7 ++++-
>  hw/net/vmxnet3.c       | 46 +++++++++++++++------------------
>  hw/pci/msi.c           |  9 ++++---
>  hw/pci/msix.c          | 42 +++++++++++++++++++++++++-----
>  hw/scsi/megasas.c      | 49 ++++++++++++++++++++---------------
>  hw/usb/hcd-xhci.c      | 69 ++++++++++++++++++++++++++++++--------------------
>  hw/vfio/pci.c          |  8 ++++--
>  hw/virtio/virtio-pci.c | 11 ++++----
>  include/hw/pci/msix.h  |  5 ++--
>  12 files changed, 164 insertions(+), 101 deletions(-)
> 
> -- 
> 2.1.0
> 
> 

Re: [Qemu-devel] [PATCH v7 00/10] Convert msix_init() to error

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Mon, Nov 14, 2016 at 03:25:30PM +0800, Cao jin wrote:
>> v7 changelog:
>> 1. fix the segfaut bug in patch 2. So drop the all the R-b of it,
>>    please take a look, there is detailed description in the patch.
>> 2. add the R-b from Hannes Reinecke
>> 
>> Test:
>> 1. make check: pass
>> 2. After applied all the patch, command line test for all the
>>    affected devices, just make sure device realize process is ok,
>>    no crash, but no further use of device.
>
> Consider the megasas device for example, don't you
> need to test that the change actually does what
> it's intended to do?

For better or worse, that's a higher bar than we commonly require for
refactorings.

[...]

Re: [Qemu-devel] [PATCH v7 00/10] Convert msix_init() to error

[Michael S. Tsirkin]

On Tue, Jan 10, 2017 at 11:06:08AM +0100, Markus Armbruster wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Mon, Nov 14, 2016 at 03:25:30PM +0800, Cao jin wrote:
> >> v7 changelog:
> >> 1. fix the segfaut bug in patch 2. So drop the all the R-b of it,
> >>    please take a look, there is detailed description in the patch.
> >> 2. add the R-b from Hannes Reinecke
> >> 
> >> Test:
> >> 1. make check: pass
> >> 2. After applied all the patch, command line test for all the
> >>    affected devices, just make sure device realize process is ok,
> >>    no crash, but no further use of device.
> >
> > Consider the megasas device for example, don't you
> > need to test that the change actually does what
> > it's intended to do?
> 
> For better or worse, that's a higher bar than we commonly require for
> refactorings.
> 
> [...]

Well the patch says that it's addressing a TODO. If no one can
be bothered to test the functionality, maybe we shouldn't bother
with the change.

Generally this patchset is at v7.  It brings a very limited benefit to
the project.  It better be perfect otherwise I don't see why bother.

-- 
MST

Re: [Qemu-devel] [PATCH v7 00/10] Convert msix_init() to error

[Paolo Bonzini]

On 10/01/2017 15:38, Michael S. Tsirkin wrote:
> On Tue, Jan 10, 2017 at 11:06:08AM +0100, Markus Armbruster wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>>
>>> On Mon, Nov 14, 2016 at 03:25:30PM +0800, Cao jin wrote:
>>>> v7 changelog:
>>>> 1. fix the segfaut bug in patch 2. So drop the all the R-b of it,
>>>>    please take a look, there is detailed description in the patch.
>>>> 2. add the R-b from Hannes Reinecke
>>>>
>>>> Test:
>>>> 1. make check: pass
>>>> 2. After applied all the patch, command line test for all the
>>>>    affected devices, just make sure device realize process is ok,
>>>>    no crash, but no further use of device.
>>>
>>> Consider the megasas device for example, don't you
>>> need to test that the change actually does what
>>> it's intended to do?
>>
>> For better or worse, that's a higher bar than we commonly require for
>> refactorings.
>>
>> [...]
> 
> Well the patch says that it's addressing a TODO. If no one can
> be bothered to test the functionality, maybe we shouldn't bother
> with the change.
> 
> Generally this patchset is at v7.  It brings a very limited benefit to
> the project.  It better be perfect otherwise I don't see why bother.

I agree.  The changes to the device model are non-trivial and you should
make a decent effort at coverage of non-trivial changes.

Paolo

Re: [Qemu-devel] [PATCH v7 00/10] Convert msix_init() to error

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Tue, Jan 10, 2017 at 11:06:08AM +0100, Markus Armbruster wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> 
>> > On Mon, Nov 14, 2016 at 03:25:30PM +0800, Cao jin wrote:
>> >> v7 changelog:
>> >> 1. fix the segfaut bug in patch 2. So drop the all the R-b of it,
>> >>    please take a look, there is detailed description in the patch.
>> >> 2. add the R-b from Hannes Reinecke
>> >> 
>> >> Test:
>> >> 1. make check: pass
>> >> 2. After applied all the patch, command line test for all the
>> >>    affected devices, just make sure device realize process is ok,
>> >>    no crash, but no further use of device.
>> >
>> > Consider the megasas device for example, don't you
>> > need to test that the change actually does what
>> > it's intended to do?
>> 
>> For better or worse, that's a higher bar than we commonly require for
>> refactorings.
>> 
>> [...]
>
> Well the patch says that it's addressing a TODO. If no one can
> be bothered to test the functionality, maybe we shouldn't bother
> with the change.
>
> Generally this patchset is at v7.  It brings a very limited benefit to
> the project.  It better be perfect otherwise I don't see why bother.

We obviously disagree on the benefit.  Before this series, error
reporting is *broken* for QMP.  Fixing that is definitely not "why
bother" material.

I certainly don't object to tightening our testing habits.  I was merely
pointing out that you're doing that.

Testing patches that modernize interfaces often isn't easy for the
person doing the work.  Fortunately, megasas has a maintainer: Hannes.
Who gave his R-by.  For me, that would suffice, but you may see things
differently.  Would a Tested-by from him satisfy you?  If not, what
would satisfy you?

Re: [Qemu-devel] [PATCH v7 00/10] Convert msix_init() to error

[Paolo Bonzini]

On 10/01/2017 17:19, Markus Armbruster wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> Generally this patchset is at v7.  It brings a very limited benefit to
>> the project.  It better be perfect otherwise I don't see why bother.
> 
> We obviously disagree on the benefit.  Before this series, error
> reporting is *broken* for QMP.  Fixing that is definitely not "why
> bother" material.
> 
> I certainly don't object to tightening our testing habits.  I was merely
> pointing out that you're doing that.
> 
> Testing patches that modernize interfaces often isn't easy for the
> person doing the work.  Fortunately, megasas has a maintainer: Hannes.
> Who gave his R-by.  For me, that would suffice, but you may see things
> differently.  Would a Tested-by from him satisfy you?  If not, what
> would satisfy you?

As a co-maintainer (via hw/scsi/" of megasas) I think that the only
satisfactory testing, for merging hw/scsi patches *through a tree other
than mine*, is "this installs Linux for a distro of the submitter's
choice", and "the commit message explains the differences clearly with
sample invocations of QEMU".

Installing Linux seems like a higher bar than usual, but it only takes a
few minutes to start such an install.  I'll even accept "it formats the
disk fine".

Sure, I don't even apply it to all of my patches.  But I'm the sole
responsible for my screwups and the community knows that I'll be there
to fix them so they accept it.  A random patch submitter is going to
waste time of multiple maintainers if they don't do proper testing, and
I think it's fair given that some previous submitted versions didn't
even compile.

Paolo

Re: [Qemu-devel] [PATCH v7 03/10] pci: Convert msix_init() to Error and fix callers to check it

[Michael S. Tsirkin]

On Mon, Nov 14, 2016 at 03:25:33PM +0800, Cao jin wrote:
> msix_init() reports errors with error_report(), which is wrong when
> it's used in realize().  The same issue was fixed for msi_init() in
> commit 1108b2f.
> 
> For some devices(like e1000e, vmxnet3) who won't fail because of
> msix_init's failure, suppress the error report by passing NULL error object.
> 
> Bonus: add comment for msix_init.
> 
> CC: Jiri Pirko <jiri@resnulli.us>
> CC: Gerd Hoffmann <kraxel@redhat.com>
> CC: Dmitry Fleytman <dmitry@daynix.com>
> CC: Jason Wang <jasowang@redhat.com>
> CC: Michael S. Tsirkin <mst@redhat.com>
> CC: Hannes Reinecke <hare@suse.de>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Alex Williamson <alex.williamson@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Marcel Apfelbaum <marcel@redhat.com>
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Hannes Reinecke <hare@suse.com>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>

I'd rather add a new API. Once that is merged you can make device
changes avoiding a flag day. Merge this through individual trees. At the
end of the day we can drop the old API when it's no longer needed.

> ---
>  hw/block/nvme.c        |  5 ++++-
>  hw/misc/ivshmem.c      |  8 ++++----
>  hw/net/e1000e.c        |  6 +++++-
>  hw/net/rocker/rocker.c |  7 ++++++-
>  hw/net/vmxnet3.c       |  8 ++++++--
>  hw/pci/msix.c          | 34 +++++++++++++++++++++++++++++-----
>  hw/scsi/megasas.c      |  5 ++++-
>  hw/usb/hcd-xhci.c      | 13 ++++++++-----
>  hw/vfio/pci.c          |  8 ++++++--
>  hw/virtio/virtio-pci.c | 11 +++++------
>  include/hw/pci/msix.h  |  5 +++--
>  11 files changed, 80 insertions(+), 30 deletions(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index d479fd2..2d703c8 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -831,6 +831,7 @@ static int nvme_init(PCIDevice *pci_dev)
>  {
>      NvmeCtrl *n = NVME(pci_dev);
>      NvmeIdCtrl *id = &n->id_ctrl;
> +    Error *err = NULL;
>  
>      int i;
>      int64_t bs_size;
> @@ -872,7 +873,9 @@ static int nvme_init(PCIDevice *pci_dev)
>      pci_register_bar(&n->parent_obj, 0,
>          PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_64,
>          &n->iomem);
> -    msix_init_exclusive_bar(&n->parent_obj, n->num_queues, 4);
> +    if (msix_init_exclusive_bar(&n->parent_obj, n->num_queues, 4, &err)) {
> +        error_report_err(err);
> +    }
>  
>      id->vid = cpu_to_le16(pci_get_word(pci_conf + PCI_VENDOR_ID));
>      id->ssvid = cpu_to_le16(pci_get_word(pci_conf + PCI_SUBSYSTEM_VENDOR_ID));

Why don't you suppress this one too?

> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index 230e51b..ca6f804 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -749,13 +749,13 @@ static void ivshmem_reset(DeviceState *d)
>      }
>  }
>  
> -static int ivshmem_setup_interrupts(IVShmemState *s)
> +static int ivshmem_setup_interrupts(IVShmemState *s, Error **errp)
>  {
>      /* allocate QEMU callback data for receiving interrupts */
>      s->msi_vectors = g_malloc0(s->vectors * sizeof(MSIVector));
>  
>      if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
> -        if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1)) {
> +        if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1, errp)) {
>              return -1;
>          }
>  
> @@ -897,8 +897,8 @@ static void ivshmem_common_realize(PCIDevice *dev, Error **errp)
>          qemu_chr_fe_set_handlers(&s->server_chr, ivshmem_can_receive,
>                                   ivshmem_read, NULL, s, NULL, true);
>  
> -        if (ivshmem_setup_interrupts(s) < 0) {
> -            error_setg(errp, "failed to initialize interrupts");
> +        if (ivshmem_setup_interrupts(s, errp) < 0) {
> +            error_prepend(errp, "Failed to initialize interrupts: ");
>              return;
>          }
>      }
> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
> index 4994e1c..74cbbef 100644
> --- a/hw/net/e1000e.c
> +++ b/hw/net/e1000e.c
> @@ -292,7 +292,11 @@ e1000e_init_msix(E1000EState *s)
>                          E1000E_MSIX_IDX, E1000E_MSIX_TABLE,
>                          &s->msix,
>                          E1000E_MSIX_IDX, E1000E_MSIX_PBA,
> -                        0xA0);
> +                        0xA0, NULL);
> +
> +    /* Any error other than -ENOTSUP(board's MSI support is broken)
> +     * is a programming error. Fall back to INTx silently on -ENOTSUP */
> +    assert(!res || res == -ENOTSUP);
>  
>      if (res < 0) {
>          trace_e1000e_msix_init_fail(res);
> diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
> index e9d215a..8f829f2 100644
> --- a/hw/net/rocker/rocker.c
> +++ b/hw/net/rocker/rocker.c
> @@ -1256,14 +1256,19 @@ static int rocker_msix_init(Rocker *r)
>  {
>      PCIDevice *dev = PCI_DEVICE(r);
>      int err;
> +    Error *local_err = NULL;
>  
>      err = msix_init(dev, ROCKER_MSIX_VEC_COUNT(r->fp_ports),
>                      &r->msix_bar,
>                      ROCKER_PCI_MSIX_BAR_IDX, ROCKER_PCI_MSIX_TABLE_OFFSET,
>                      &r->msix_bar,
>                      ROCKER_PCI_MSIX_BAR_IDX, ROCKER_PCI_MSIX_PBA_OFFSET,
> -                    0);
> +                    0, &local_err);
> +    /* Any error other than -ENOTSUP(board's MSI support is broken)
> +     * is a programming error. */
> +    assert(!err || err == -ENOTSUP);
>      if (err) {
> +        error_report_err(local_err);
>          return err;
>      }
>  
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 92f6af9..a433cc0 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -2191,10 +2191,14 @@ vmxnet3_init_msix(VMXNET3State *s)
>                          VMXNET3_MSIX_BAR_IDX, VMXNET3_OFF_MSIX_TABLE,
>                          &s->msix_bar,
>                          VMXNET3_MSIX_BAR_IDX, VMXNET3_OFF_MSIX_PBA(s),
> -                        VMXNET3_MSIX_OFFSET(s));
> +                        VMXNET3_MSIX_OFFSET(s), NULL);
> +
> +    /* Any error other than -ENOTSUP(board's MSI support is broken)
> +     * is a programming error. Fall back to INTx on -ENOTSUP */
> +    assert(!res || res == -ENOTSUP);
>  
>      if (0 > res) {
> -        VMW_WRPRN("Failed to initialize MSI-X, error %d", res);
> +        VMW_WRPRN("Failed to initialize MSI-X, board's MSI support is broken");
>          s->msix_used = false;
>      } else {
>          if (!vmxnet3_use_msix_vectors(s, VMXNET3_MAX_INTRS)) {
> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> index 0cee631..d03016f 100644
> --- a/hw/pci/msix.c
> +++ b/hw/pci/msix.c
> @@ -21,6 +21,7 @@
>  #include "hw/pci/pci.h"
>  #include "hw/xen/xen.h"
>  #include "qemu/range.h"
> +#include "qapi/error.h"
>  
>  #define MSIX_CAP_LENGTH 12
>  
> @@ -238,11 +239,29 @@ static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
>      }
>  }
>  
> -/* Initialize the MSI-X structures */
> +/* Make PCI device @dev MSI-X capable
> + * @nentries is the max number of MSI-X vectors that the device support.
> + * @table_bar is the MemoryRegion that MSI-X table structure resides.
> + * @table_bar_nr is number of base address register corresponding to @table_bar.
> + * @table_offset indicates the offset that the MSI-X table structure starts with
> + * in @table_bar.
> + * @pba_bar is the MemoryRegion that the Pending Bit Array structure resides.
> + * @pba_bar_nr is number of base address register corresponding to @pba_bar.
> + * @pba_offset indicates the offset that the Pending Bit Array structure
> + * starts with in @pba_bar.
> + * Non-zero @cap_pos puts capability MSI-X at that offset in PCI config space.
> + * @errp is for returning errors.
> + *
> + * Return 0 on success; set @errp and return -errno on error:
> + * -ENOTSUP means lacking msi support for a msi-capable platform.
> + * -EINVAL means capability overlap, happens when @cap_pos is non-zero,
> + * also means a programming error, except device assignment, which can check
> + * if a real HW is broken.*/
>  int msix_init(struct PCIDevice *dev, unsigned short nentries,
>                MemoryRegion *table_bar, uint8_t table_bar_nr,
>                unsigned table_offset, MemoryRegion *pba_bar,
> -              uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos)
> +              uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos,
> +              Error **errp)
>  {
>      int cap;
>      unsigned table_size, pba_size;
> @@ -250,10 +269,12 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
>  
>      /* Nothing to do if MSI is not supported by interrupt controller */
>      if (!msi_nonbroken) {
> +        error_setg(errp, "MSI-X is not supported by interrupt controller");
>          return -ENOTSUP;
>      }
>  
>      if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1) {
> +        error_setg(errp, "The number of MSI-X vectors is invalid");
>          return -EINVAL;
>      }
>  
> @@ -266,10 +287,13 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
>          table_offset + table_size > memory_region_size(table_bar) ||
>          pba_offset + pba_size > memory_region_size(pba_bar) ||
>          (table_offset | pba_offset) & PCI_MSIX_FLAGS_BIRMASK) {
> +        error_setg(errp, "table & pba overlap, or they don't fit in BARs,"
> +                   " or don't align");
>          return -EINVAL;
>      }
>  
> -    cap = pci_add_capability(dev, PCI_CAP_ID_MSIX, cap_pos, MSIX_CAP_LENGTH);
> +    cap = pci_add_capability2(dev, PCI_CAP_ID_MSIX,
> +                              cap_pos, MSIX_CAP_LENGTH, errp);
>      if (cap < 0) {
>          return cap;
>      }
> @@ -306,7 +330,7 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
>  }
>  
>  int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
> -                            uint8_t bar_nr)
> +                            uint8_t bar_nr, Error **errp)
>  {
>      int ret;
>      char *name;
> @@ -338,7 +362,7 @@ int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
>      ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr,
>                      0, &dev->msix_exclusive_bar,
>                      bar_nr, bar_pba_offset,
> -                    0);
> +                    0, errp);
>      if (ret) {
>          return ret;
>      }
> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index 52a4123..fada834 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -2360,9 +2360,12 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
>  
>      if (megasas_use_msix(s) &&
>          msix_init(dev, 15, &s->mmio_io, b->mmio_bar, 0x2000,
> -                  &s->mmio_io, b->mmio_bar, 0x3800, 0x68)) {
> +                  &s->mmio_io, b->mmio_bar, 0x3800, 0x68, &err)) {
> +        /*TODO: check msix_init's error, and should fail on msix=on */
> +        error_report_err(err);
>          s->msix = ON_OFF_AUTO_OFF;
>      }
> +
>      if (pci_is_express(dev)) {
>          pcie_endpoint_cap_init(dev, 0xa0);
>      }
> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> index 0ace273..153b006 100644
> --- a/hw/usb/hcd-xhci.c
> +++ b/hw/usb/hcd-xhci.c
> @@ -3703,11 +3703,14 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
>      }
>  
>      if (xhci->msix != ON_OFF_AUTO_OFF) {
> -        /* TODO check for errors */
> -        msix_init(dev, xhci->numintrs,
> -                  &xhci->mem, 0, OFF_MSIX_TABLE,
> -                  &xhci->mem, 0, OFF_MSIX_PBA,
> -                  0x90);
> +        /* TODO check for errors, and should fail when msix=on */
> +        ret = msix_init(dev, xhci->numintrs,
> +                        &xhci->mem, 0, OFF_MSIX_TABLE,
> +                        &xhci->mem, 0, OFF_MSIX_PBA,
> +                        0x90, &err);
> +        if (ret) {
> +            error_report_err(err);
> +        }
>      }
>  }
>  
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index d7dbe0e..0bcfaad 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -1432,6 +1432,7 @@ static void vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
>  static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
>  {
>      int ret;
> +    Error *local_err = NULL;
>  
>      vdev->msix->pending = g_malloc0(BITS_TO_LONGS(vdev->msix->entries) *
>                                      sizeof(unsigned long));
> @@ -1439,12 +1440,15 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
>                      vdev->bars[vdev->msix->table_bar].region.mem,
>                      vdev->msix->table_bar, vdev->msix->table_offset,
>                      vdev->bars[vdev->msix->pba_bar].region.mem,
> -                    vdev->msix->pba_bar, vdev->msix->pba_offset, pos);
> +                    vdev->msix->pba_bar, vdev->msix->pba_offset, pos,
> +                    &local_err);
>      if (ret < 0) {
>          if (ret == -ENOTSUP) {
> +            error_report_err(local_err);
>              return 0;
>          }
> -        error_setg(errp, "msix_init failed");
> +
> +        error_propagate(errp, local_err);
>          return ret;
>      }
>  
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 06831de..5acce38 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1693,13 +1693,12 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
>  
>      if (proxy->nvectors) {
>          int err = msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors,
> -                                          proxy->msix_bar_idx);
> +                                          proxy->msix_bar_idx, errp);
> +        /* Any error other than -ENOTSUP(board's MSI support is broken)
> +         * is a programming error. */
> +        assert(!err || err == -ENOTSUP);
>          if (err) {
> -            /* Notice when a system that supports MSIx can't initialize it.  */
> -            if (err != -ENOTSUP) {
> -                error_report("unable to init msix vectors to %" PRIu32,
> -                             proxy->nvectors);
> -            }
> +            error_report_err(*errp);
>              proxy->nvectors = 0;
>          }
>      }
> diff --git a/include/hw/pci/msix.h b/include/hw/pci/msix.h
> index 048a29d..1f27658 100644
> --- a/include/hw/pci/msix.h
> +++ b/include/hw/pci/msix.h
> @@ -9,9 +9,10 @@ MSIMessage msix_get_message(PCIDevice *dev, unsigned int vector);
>  int msix_init(PCIDevice *dev, unsigned short nentries,
>                MemoryRegion *table_bar, uint8_t table_bar_nr,
>                unsigned table_offset, MemoryRegion *pba_bar,
> -              uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos);
> +              uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos,
> +              Error **errp);
>  int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
> -                            uint8_t bar_nr);
> +                            uint8_t bar_nr, Error **errp);
>  
>  void msix_write_config(PCIDevice *dev, uint32_t address, uint32_t val, int len);
>  
> -- 
> 2.1.0
> 
> 

Re: [Qemu-devel] [PATCH v7 03/10] pci: Convert msix_init() to Error and fix callers to check it

[Paolo Bonzini]

On 10/01/2017 18:54, Michael S. Tsirkin wrote:
> On Mon, Nov 14, 2016 at 03:25:33PM +0800, Cao jin wrote:
>> msix_init() reports errors with error_report(), which is wrong when
>> it's used in realize().  The same issue was fixed for msi_init() in
>> commit 1108b2f.
>>
>> For some devices(like e1000e, vmxnet3) who won't fail because of
>> msix_init's failure, suppress the error report by passing NULL error object.
>>
>> Bonus: add comment for msix_init.
>>
>> CC: Jiri Pirko <jiri@resnulli.us>
>> CC: Gerd Hoffmann <kraxel@redhat.com>
>> CC: Dmitry Fleytman <dmitry@daynix.com>
>> CC: Jason Wang <jasowang@redhat.com>
>> CC: Michael S. Tsirkin <mst@redhat.com>
>> CC: Hannes Reinecke <hare@suse.de>
>> CC: Paolo Bonzini <pbonzini@redhat.com>
>> CC: Alex Williamson <alex.williamson@redhat.com>
>> CC: Markus Armbruster <armbru@redhat.com>
>> CC: Marcel Apfelbaum <marcel@redhat.com>
>>
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>> Reviewed-by: Hannes Reinecke <hare@suse.com>
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> 
> I'd rather add a new API. Once that is merged you can make device
> changes avoiding a flag day. Merge this through individual trees. At the
> end of the day we can drop the old API when it's no longer needed.

I think that's the worst.  We don't need another partial transition and
this series is all but big.  The main issue is that it was handled badly
in the past, so people tend not to trust it.

If anything, if there are independent patches in the series that can be
merged through USB or SCSI trees, before this one, we can do that.

Paolo
>> ---
>>  hw/block/nvme.c        |  5 ++++-
>>  hw/misc/ivshmem.c      |  8 ++++----
>>  hw/net/e1000e.c        |  6 +++++-
>>  hw/net/rocker/rocker.c |  7 ++++++-
>>  hw/net/vmxnet3.c       |  8 ++++++--
>>  hw/pci/msix.c          | 34 +++++++++++++++++++++++++++++-----
>>  hw/scsi/megasas.c      |  5 ++++-
>>  hw/usb/hcd-xhci.c      | 13 ++++++++-----
>>  hw/vfio/pci.c          |  8 ++++++--
>>  hw/virtio/virtio-pci.c | 11 +++++------
>>  include/hw/pci/msix.h  |  5 +++--
>>  11 files changed, 80 insertions(+), 30 deletions(-)
>>
>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>> index d479fd2..2d703c8 100644
>> --- a/hw/block/nvme.c
>> +++ b/hw/block/nvme.c
>> @@ -831,6 +831,7 @@ static int nvme_init(PCIDevice *pci_dev)
>>  {
>>      NvmeCtrl *n = NVME(pci_dev);
>>      NvmeIdCtrl *id = &n->id_ctrl;
>> +    Error *err = NULL;
>>  
>>      int i;
>>      int64_t bs_size;
>> @@ -872,7 +873,9 @@ static int nvme_init(PCIDevice *pci_dev)
>>      pci_register_bar(&n->parent_obj, 0,
>>          PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_64,
>>          &n->iomem);
>> -    msix_init_exclusive_bar(&n->parent_obj, n->num_queues, 4);
>> +    if (msix_init_exclusive_bar(&n->parent_obj, n->num_queues, 4, &err)) {
>> +        error_report_err(err);
>> +    }
>>  
>>      id->vid = cpu_to_le16(pci_get_word(pci_conf + PCI_VENDOR_ID));
>>      id->ssvid = cpu_to_le16(pci_get_word(pci_conf + PCI_SUBSYSTEM_VENDOR_ID));
> 
> Why don't you suppress this one too?
> 
>> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
>> index 230e51b..ca6f804 100644
>> --- a/hw/misc/ivshmem.c
>> +++ b/hw/misc/ivshmem.c
>> @@ -749,13 +749,13 @@ static void ivshmem_reset(DeviceState *d)
>>      }
>>  }
>>  
>> -static int ivshmem_setup_interrupts(IVShmemState *s)
>> +static int ivshmem_setup_interrupts(IVShmemState *s, Error **errp)
>>  {
>>      /* allocate QEMU callback data for receiving interrupts */
>>      s->msi_vectors = g_malloc0(s->vectors * sizeof(MSIVector));
>>  
>>      if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
>> -        if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1)) {
>> +        if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1, errp)) {
>>              return -1;
>>          }
>>  
>> @@ -897,8 +897,8 @@ static void ivshmem_common_realize(PCIDevice *dev, Error **errp)
>>          qemu_chr_fe_set_handlers(&s->server_chr, ivshmem_can_receive,
>>                                   ivshmem_read, NULL, s, NULL, true);
>>  
>> -        if (ivshmem_setup_interrupts(s) < 0) {
>> -            error_setg(errp, "failed to initialize interrupts");
>> +        if (ivshmem_setup_interrupts(s, errp) < 0) {
>> +            error_prepend(errp, "Failed to initialize interrupts: ");
>>              return;
>>          }
>>      }
>> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
>> index 4994e1c..74cbbef 100644
>> --- a/hw/net/e1000e.c
>> +++ b/hw/net/e1000e.c
>> @@ -292,7 +292,11 @@ e1000e_init_msix(E1000EState *s)
>>                          E1000E_MSIX_IDX, E1000E_MSIX_TABLE,
>>                          &s->msix,
>>                          E1000E_MSIX_IDX, E1000E_MSIX_PBA,
>> -                        0xA0);
>> +                        0xA0, NULL);
>> +
>> +    /* Any error other than -ENOTSUP(board's MSI support is broken)
>> +     * is a programming error. Fall back to INTx silently on -ENOTSUP */
>> +    assert(!res || res == -ENOTSUP);
>>  
>>      if (res < 0) {
>>          trace_e1000e_msix_init_fail(res);
>> diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
>> index e9d215a..8f829f2 100644
>> --- a/hw/net/rocker/rocker.c
>> +++ b/hw/net/rocker/rocker.c
>> @@ -1256,14 +1256,19 @@ static int rocker_msix_init(Rocker *r)
>>  {
>>      PCIDevice *dev = PCI_DEVICE(r);
>>      int err;
>> +    Error *local_err = NULL;
>>  
>>      err = msix_init(dev, ROCKER_MSIX_VEC_COUNT(r->fp_ports),
>>                      &r->msix_bar,
>>                      ROCKER_PCI_MSIX_BAR_IDX, ROCKER_PCI_MSIX_TABLE_OFFSET,
>>                      &r->msix_bar,
>>                      ROCKER_PCI_MSIX_BAR_IDX, ROCKER_PCI_MSIX_PBA_OFFSET,
>> -                    0);
>> +                    0, &local_err);
>> +    /* Any error other than -ENOTSUP(board's MSI support is broken)
>> +     * is a programming error. */
>> +    assert(!err || err == -ENOTSUP);
>>      if (err) {
>> +        error_report_err(local_err);
>>          return err;
>>      }
>>  
>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
>> index 92f6af9..a433cc0 100644
>> --- a/hw/net/vmxnet3.c
>> +++ b/hw/net/vmxnet3.c
>> @@ -2191,10 +2191,14 @@ vmxnet3_init_msix(VMXNET3State *s)
>>                          VMXNET3_MSIX_BAR_IDX, VMXNET3_OFF_MSIX_TABLE,
>>                          &s->msix_bar,
>>                          VMXNET3_MSIX_BAR_IDX, VMXNET3_OFF_MSIX_PBA(s),
>> -                        VMXNET3_MSIX_OFFSET(s));
>> +                        VMXNET3_MSIX_OFFSET(s), NULL);
>> +
>> +    /* Any error other than -ENOTSUP(board's MSI support is broken)
>> +     * is a programming error. Fall back to INTx on -ENOTSUP */
>> +    assert(!res || res == -ENOTSUP);
>>  
>>      if (0 > res) {
>> -        VMW_WRPRN("Failed to initialize MSI-X, error %d", res);
>> +        VMW_WRPRN("Failed to initialize MSI-X, board's MSI support is broken");
>>          s->msix_used = false;
>>      } else {
>>          if (!vmxnet3_use_msix_vectors(s, VMXNET3_MAX_INTRS)) {
>> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
>> index 0cee631..d03016f 100644
>> --- a/hw/pci/msix.c
>> +++ b/hw/pci/msix.c
>> @@ -21,6 +21,7 @@
>>  #include "hw/pci/pci.h"
>>  #include "hw/xen/xen.h"
>>  #include "qemu/range.h"
>> +#include "qapi/error.h"
>>  
>>  #define MSIX_CAP_LENGTH 12
>>  
>> @@ -238,11 +239,29 @@ static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
>>      }
>>  }
>>  
>> -/* Initialize the MSI-X structures */
>> +/* Make PCI device @dev MSI-X capable
>> + * @nentries is the max number of MSI-X vectors that the device support.
>> + * @table_bar is the MemoryRegion that MSI-X table structure resides.
>> + * @table_bar_nr is number of base address register corresponding to @table_bar.
>> + * @table_offset indicates the offset that the MSI-X table structure starts with
>> + * in @table_bar.
>> + * @pba_bar is the MemoryRegion that the Pending Bit Array structure resides.
>> + * @pba_bar_nr is number of base address register corresponding to @pba_bar.
>> + * @pba_offset indicates the offset that the Pending Bit Array structure
>> + * starts with in @pba_bar.
>> + * Non-zero @cap_pos puts capability MSI-X at that offset in PCI config space.
>> + * @errp is for returning errors.
>> + *
>> + * Return 0 on success; set @errp and return -errno on error:
>> + * -ENOTSUP means lacking msi support for a msi-capable platform.
>> + * -EINVAL means capability overlap, happens when @cap_pos is non-zero,
>> + * also means a programming error, except device assignment, which can check
>> + * if a real HW is broken.*/
>>  int msix_init(struct PCIDevice *dev, unsigned short nentries,
>>                MemoryRegion *table_bar, uint8_t table_bar_nr,
>>                unsigned table_offset, MemoryRegion *pba_bar,
>> -              uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos)
>> +              uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos,
>> +              Error **errp)
>>  {
>>      int cap;
>>      unsigned table_size, pba_size;
>> @@ -250,10 +269,12 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
>>  
>>      /* Nothing to do if MSI is not supported by interrupt controller */
>>      if (!msi_nonbroken) {
>> +        error_setg(errp, "MSI-X is not supported by interrupt controller");
>>          return -ENOTSUP;
>>      }
>>  
>>      if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1) {
>> +        error_setg(errp, "The number of MSI-X vectors is invalid");
>>          return -EINVAL;
>>      }
>>  
>> @@ -266,10 +287,13 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
>>          table_offset + table_size > memory_region_size(table_bar) ||
>>          pba_offset + pba_size > memory_region_size(pba_bar) ||
>>          (table_offset | pba_offset) & PCI_MSIX_FLAGS_BIRMASK) {
>> +        error_setg(errp, "table & pba overlap, or they don't fit in BARs,"
>> +                   " or don't align");
>>          return -EINVAL;
>>      }
>>  
>> -    cap = pci_add_capability(dev, PCI_CAP_ID_MSIX, cap_pos, MSIX_CAP_LENGTH);
>> +    cap = pci_add_capability2(dev, PCI_CAP_ID_MSIX,
>> +                              cap_pos, MSIX_CAP_LENGTH, errp);
>>      if (cap < 0) {
>>          return cap;
>>      }
>> @@ -306,7 +330,7 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
>>  }
>>  
>>  int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
>> -                            uint8_t bar_nr)
>> +                            uint8_t bar_nr, Error **errp)
>>  {
>>      int ret;
>>      char *name;
>> @@ -338,7 +362,7 @@ int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
>>      ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr,
>>                      0, &dev->msix_exclusive_bar,
>>                      bar_nr, bar_pba_offset,
>> -                    0);
>> +                    0, errp);
>>      if (ret) {
>>          return ret;
>>      }
>> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
>> index 52a4123..fada834 100644
>> --- a/hw/scsi/megasas.c
>> +++ b/hw/scsi/megasas.c
>> @@ -2360,9 +2360,12 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
>>  
>>      if (megasas_use_msix(s) &&
>>          msix_init(dev, 15, &s->mmio_io, b->mmio_bar, 0x2000,
>> -                  &s->mmio_io, b->mmio_bar, 0x3800, 0x68)) {
>> +                  &s->mmio_io, b->mmio_bar, 0x3800, 0x68, &err)) {
>> +        /*TODO: check msix_init's error, and should fail on msix=on */
>> +        error_report_err(err);
>>          s->msix = ON_OFF_AUTO_OFF;
>>      }
>> +
>>      if (pci_is_express(dev)) {
>>          pcie_endpoint_cap_init(dev, 0xa0);
>>      }
>> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
>> index 0ace273..153b006 100644
>> --- a/hw/usb/hcd-xhci.c
>> +++ b/hw/usb/hcd-xhci.c
>> @@ -3703,11 +3703,14 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
>>      }
>>  
>>      if (xhci->msix != ON_OFF_AUTO_OFF) {
>> -        /* TODO check for errors */
>> -        msix_init(dev, xhci->numintrs,
>> -                  &xhci->mem, 0, OFF_MSIX_TABLE,
>> -                  &xhci->mem, 0, OFF_MSIX_PBA,
>> -                  0x90);
>> +        /* TODO check for errors, and should fail when msix=on */
>> +        ret = msix_init(dev, xhci->numintrs,
>> +                        &xhci->mem, 0, OFF_MSIX_TABLE,
>> +                        &xhci->mem, 0, OFF_MSIX_PBA,
>> +                        0x90, &err);
>> +        if (ret) {
>> +            error_report_err(err);
>> +        }
>>      }
>>  }
>>  
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index d7dbe0e..0bcfaad 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -1432,6 +1432,7 @@ static void vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
>>  static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
>>  {
>>      int ret;
>> +    Error *local_err = NULL;
>>  
>>      vdev->msix->pending = g_malloc0(BITS_TO_LONGS(vdev->msix->entries) *
>>                                      sizeof(unsigned long));
>> @@ -1439,12 +1440,15 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
>>                      vdev->bars[vdev->msix->table_bar].region.mem,
>>                      vdev->msix->table_bar, vdev->msix->table_offset,
>>                      vdev->bars[vdev->msix->pba_bar].region.mem,
>> -                    vdev->msix->pba_bar, vdev->msix->pba_offset, pos);
>> +                    vdev->msix->pba_bar, vdev->msix->pba_offset, pos,
>> +                    &local_err);
>>      if (ret < 0) {
>>          if (ret == -ENOTSUP) {
>> +            error_report_err(local_err);
>>              return 0;
>>          }
>> -        error_setg(errp, "msix_init failed");
>> +
>> +        error_propagate(errp, local_err);
>>          return ret;
>>      }
>>  
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index 06831de..5acce38 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -1693,13 +1693,12 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
>>  
>>      if (proxy->nvectors) {
>>          int err = msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors,
>> -                                          proxy->msix_bar_idx);
>> +                                          proxy->msix_bar_idx, errp);
>> +        /* Any error other than -ENOTSUP(board's MSI support is broken)
>> +         * is a programming error. */
>> +        assert(!err || err == -ENOTSUP);
>>          if (err) {
>> -            /* Notice when a system that supports MSIx can't initialize it.  */
>> -            if (err != -ENOTSUP) {
>> -                error_report("unable to init msix vectors to %" PRIu32,
>> -                             proxy->nvectors);
>> -            }
>> +            error_report_err(*errp);
>>              proxy->nvectors = 0;
>>          }
>>      }
>> diff --git a/include/hw/pci/msix.h b/include/hw/pci/msix.h
>> index 048a29d..1f27658 100644
>> --- a/include/hw/pci/msix.h
>> +++ b/include/hw/pci/msix.h
>> @@ -9,9 +9,10 @@ MSIMessage msix_get_message(PCIDevice *dev, unsigned int vector);
>>  int msix_init(PCIDevice *dev, unsigned short nentries,
>>                MemoryRegion *table_bar, uint8_t table_bar_nr,
>>                unsigned table_offset, MemoryRegion *pba_bar,
>> -              uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos);
>> +              uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos,
>> +              Error **errp);
>>  int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
>> -                            uint8_t bar_nr);
>> +                            uint8_t bar_nr, Error **errp);
>>  
>>  void msix_write_config(PCIDevice *dev, uint32_t address, uint32_t val, int len);
>>  
>> -- 
>> 2.1.0
>>
>>

Re: [Qemu-devel] [PATCH v7 03/10] pci: Convert msix_init() to Error and fix callers to check it

[Michael S. Tsirkin]

On Tue, Jan 10, 2017 at 06:58:39PM +0100, Paolo Bonzini wrote:
> 
> 
> On 10/01/2017 18:54, Michael S. Tsirkin wrote:
> > On Mon, Nov 14, 2016 at 03:25:33PM +0800, Cao jin wrote:
> >> msix_init() reports errors with error_report(), which is wrong when
> >> it's used in realize().  The same issue was fixed for msi_init() in
> >> commit 1108b2f.
> >>
> >> For some devices(like e1000e, vmxnet3) who won't fail because of
> >> msix_init's failure, suppress the error report by passing NULL error object.
> >>
> >> Bonus: add comment for msix_init.
> >>
> >> CC: Jiri Pirko <jiri@resnulli.us>
> >> CC: Gerd Hoffmann <kraxel@redhat.com>
> >> CC: Dmitry Fleytman <dmitry@daynix.com>
> >> CC: Jason Wang <jasowang@redhat.com>
> >> CC: Michael S. Tsirkin <mst@redhat.com>
> >> CC: Hannes Reinecke <hare@suse.de>
> >> CC: Paolo Bonzini <pbonzini@redhat.com>
> >> CC: Alex Williamson <alex.williamson@redhat.com>
> >> CC: Markus Armbruster <armbru@redhat.com>
> >> CC: Marcel Apfelbaum <marcel@redhat.com>
> >>
> >> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> >> Reviewed-by: Hannes Reinecke <hare@suse.com>
> >> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> > 
> > I'd rather add a new API. Once that is merged you can make device
> > changes avoiding a flag day. Merge this through individual trees. At the
> > end of the day we can drop the old API when it's no longer needed.
> 
> I think that's the worst.  We don't need another partial transition and
> this series is all but big.  The main issue is that it was handled badly
> in the past, so people tend not to trust it.

Exactly.

> If anything, if there are independent patches in the series that can be
> merged through USB or SCSI trees, before this one, we can do that.
> 
> Paolo

I wish but pci core changes seem to be patch 3.

> >> ---
> >>  hw/block/nvme.c        |  5 ++++-
> >>  hw/misc/ivshmem.c      |  8 ++++----
> >>  hw/net/e1000e.c        |  6 +++++-
> >>  hw/net/rocker/rocker.c |  7 ++++++-
> >>  hw/net/vmxnet3.c       |  8 ++++++--
> >>  hw/pci/msix.c          | 34 +++++++++++++++++++++++++++++-----
> >>  hw/scsi/megasas.c      |  5 ++++-
> >>  hw/usb/hcd-xhci.c      | 13 ++++++++-----
> >>  hw/vfio/pci.c          |  8 ++++++--
> >>  hw/virtio/virtio-pci.c | 11 +++++------
> >>  include/hw/pci/msix.h  |  5 +++--
> >>  11 files changed, 80 insertions(+), 30 deletions(-)
> >>
> >> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> >> index d479fd2..2d703c8 100644
> >> --- a/hw/block/nvme.c
> >> +++ b/hw/block/nvme.c
> >> @@ -831,6 +831,7 @@ static int nvme_init(PCIDevice *pci_dev)
> >>  {
> >>      NvmeCtrl *n = NVME(pci_dev);
> >>      NvmeIdCtrl *id = &n->id_ctrl;
> >> +    Error *err = NULL;
> >>  
> >>      int i;
> >>      int64_t bs_size;
> >> @@ -872,7 +873,9 @@ static int nvme_init(PCIDevice *pci_dev)
> >>      pci_register_bar(&n->parent_obj, 0,
> >>          PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_64,
> >>          &n->iomem);
> >> -    msix_init_exclusive_bar(&n->parent_obj, n->num_queues, 4);
> >> +    if (msix_init_exclusive_bar(&n->parent_obj, n->num_queues, 4, &err)) {
> >> +        error_report_err(err);
> >> +    }
> >>  
> >>      id->vid = cpu_to_le16(pci_get_word(pci_conf + PCI_VENDOR_ID));
> >>      id->ssvid = cpu_to_le16(pci_get_word(pci_conf + PCI_SUBSYSTEM_VENDOR_ID));
> > 
> > Why don't you suppress this one too?
> > 
> >> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> >> index 230e51b..ca6f804 100644
> >> --- a/hw/misc/ivshmem.c
> >> +++ b/hw/misc/ivshmem.c
> >> @@ -749,13 +749,13 @@ static void ivshmem_reset(DeviceState *d)
> >>      }
> >>  }
> >>  
> >> -static int ivshmem_setup_interrupts(IVShmemState *s)
> >> +static int ivshmem_setup_interrupts(IVShmemState *s, Error **errp)
> >>  {
> >>      /* allocate QEMU callback data for receiving interrupts */
> >>      s->msi_vectors = g_malloc0(s->vectors * sizeof(MSIVector));
> >>  
> >>      if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
> >> -        if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1)) {
> >> +        if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1, errp)) {
> >>              return -1;
> >>          }
> >>  
> >> @@ -897,8 +897,8 @@ static void ivshmem_common_realize(PCIDevice *dev, Error **errp)
> >>          qemu_chr_fe_set_handlers(&s->server_chr, ivshmem_can_receive,
> >>                                   ivshmem_read, NULL, s, NULL, true);
> >>  
> >> -        if (ivshmem_setup_interrupts(s) < 0) {
> >> -            error_setg(errp, "failed to initialize interrupts");
> >> +        if (ivshmem_setup_interrupts(s, errp) < 0) {
> >> +            error_prepend(errp, "Failed to initialize interrupts: ");
> >>              return;
> >>          }
> >>      }
> >> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
> >> index 4994e1c..74cbbef 100644
> >> --- a/hw/net/e1000e.c
> >> +++ b/hw/net/e1000e.c
> >> @@ -292,7 +292,11 @@ e1000e_init_msix(E1000EState *s)
> >>                          E1000E_MSIX_IDX, E1000E_MSIX_TABLE,
> >>                          &s->msix,
> >>                          E1000E_MSIX_IDX, E1000E_MSIX_PBA,
> >> -                        0xA0);
> >> +                        0xA0, NULL);
> >> +
> >> +    /* Any error other than -ENOTSUP(board's MSI support is broken)
> >> +     * is a programming error. Fall back to INTx silently on -ENOTSUP */
> >> +    assert(!res || res == -ENOTSUP);
> >>  
> >>      if (res < 0) {
> >>          trace_e1000e_msix_init_fail(res);
> >> diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
> >> index e9d215a..8f829f2 100644
> >> --- a/hw/net/rocker/rocker.c
> >> +++ b/hw/net/rocker/rocker.c
> >> @@ -1256,14 +1256,19 @@ static int rocker_msix_init(Rocker *r)
> >>  {
> >>      PCIDevice *dev = PCI_DEVICE(r);
> >>      int err;
> >> +    Error *local_err = NULL;
> >>  
> >>      err = msix_init(dev, ROCKER_MSIX_VEC_COUNT(r->fp_ports),
> >>                      &r->msix_bar,
> >>                      ROCKER_PCI_MSIX_BAR_IDX, ROCKER_PCI_MSIX_TABLE_OFFSET,
> >>                      &r->msix_bar,
> >>                      ROCKER_PCI_MSIX_BAR_IDX, ROCKER_PCI_MSIX_PBA_OFFSET,
> >> -                    0);
> >> +                    0, &local_err);
> >> +    /* Any error other than -ENOTSUP(board's MSI support is broken)
> >> +     * is a programming error. */
> >> +    assert(!err || err == -ENOTSUP);
> >>      if (err) {
> >> +        error_report_err(local_err);
> >>          return err;
> >>      }
> >>  
> >> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> >> index 92f6af9..a433cc0 100644
> >> --- a/hw/net/vmxnet3.c
> >> +++ b/hw/net/vmxnet3.c
> >> @@ -2191,10 +2191,14 @@ vmxnet3_init_msix(VMXNET3State *s)
> >>                          VMXNET3_MSIX_BAR_IDX, VMXNET3_OFF_MSIX_TABLE,
> >>                          &s->msix_bar,
> >>                          VMXNET3_MSIX_BAR_IDX, VMXNET3_OFF_MSIX_PBA(s),
> >> -                        VMXNET3_MSIX_OFFSET(s));
> >> +                        VMXNET3_MSIX_OFFSET(s), NULL);
> >> +
> >> +    /* Any error other than -ENOTSUP(board's MSI support is broken)
> >> +     * is a programming error. Fall back to INTx on -ENOTSUP */
> >> +    assert(!res || res == -ENOTSUP);
> >>  
> >>      if (0 > res) {
> >> -        VMW_WRPRN("Failed to initialize MSI-X, error %d", res);
> >> +        VMW_WRPRN("Failed to initialize MSI-X, board's MSI support is broken");
> >>          s->msix_used = false;
> >>      } else {
> >>          if (!vmxnet3_use_msix_vectors(s, VMXNET3_MAX_INTRS)) {
> >> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> >> index 0cee631..d03016f 100644
> >> --- a/hw/pci/msix.c
> >> +++ b/hw/pci/msix.c
> >> @@ -21,6 +21,7 @@
> >>  #include "hw/pci/pci.h"
> >>  #include "hw/xen/xen.h"
> >>  #include "qemu/range.h"
> >> +#include "qapi/error.h"
> >>  
> >>  #define MSIX_CAP_LENGTH 12
> >>  
> >> @@ -238,11 +239,29 @@ static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
> >>      }
> >>  }
> >>  
> >> -/* Initialize the MSI-X structures */
> >> +/* Make PCI device @dev MSI-X capable
> >> + * @nentries is the max number of MSI-X vectors that the device support.
> >> + * @table_bar is the MemoryRegion that MSI-X table structure resides.
> >> + * @table_bar_nr is number of base address register corresponding to @table_bar.
> >> + * @table_offset indicates the offset that the MSI-X table structure starts with
> >> + * in @table_bar.
> >> + * @pba_bar is the MemoryRegion that the Pending Bit Array structure resides.
> >> + * @pba_bar_nr is number of base address register corresponding to @pba_bar.
> >> + * @pba_offset indicates the offset that the Pending Bit Array structure
> >> + * starts with in @pba_bar.
> >> + * Non-zero @cap_pos puts capability MSI-X at that offset in PCI config space.
> >> + * @errp is for returning errors.
> >> + *
> >> + * Return 0 on success; set @errp and return -errno on error:
> >> + * -ENOTSUP means lacking msi support for a msi-capable platform.
> >> + * -EINVAL means capability overlap, happens when @cap_pos is non-zero,
> >> + * also means a programming error, except device assignment, which can check
> >> + * if a real HW is broken.*/
> >>  int msix_init(struct PCIDevice *dev, unsigned short nentries,
> >>                MemoryRegion *table_bar, uint8_t table_bar_nr,
> >>                unsigned table_offset, MemoryRegion *pba_bar,
> >> -              uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos)
> >> +              uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos,
> >> +              Error **errp)
> >>  {
> >>      int cap;
> >>      unsigned table_size, pba_size;
> >> @@ -250,10 +269,12 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
> >>  
> >>      /* Nothing to do if MSI is not supported by interrupt controller */
> >>      if (!msi_nonbroken) {
> >> +        error_setg(errp, "MSI-X is not supported by interrupt controller");
> >>          return -ENOTSUP;
> >>      }
> >>  
> >>      if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1) {
> >> +        error_setg(errp, "The number of MSI-X vectors is invalid");
> >>          return -EINVAL;
> >>      }
> >>  
> >> @@ -266,10 +287,13 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
> >>          table_offset + table_size > memory_region_size(table_bar) ||
> >>          pba_offset + pba_size > memory_region_size(pba_bar) ||
> >>          (table_offset | pba_offset) & PCI_MSIX_FLAGS_BIRMASK) {
> >> +        error_setg(errp, "table & pba overlap, or they don't fit in BARs,"
> >> +                   " or don't align");
> >>          return -EINVAL;
> >>      }
> >>  
> >> -    cap = pci_add_capability(dev, PCI_CAP_ID_MSIX, cap_pos, MSIX_CAP_LENGTH);
> >> +    cap = pci_add_capability2(dev, PCI_CAP_ID_MSIX,
> >> +                              cap_pos, MSIX_CAP_LENGTH, errp);
> >>      if (cap < 0) {
> >>          return cap;
> >>      }
> >> @@ -306,7 +330,7 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
> >>  }
> >>  
> >>  int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
> >> -                            uint8_t bar_nr)
> >> +                            uint8_t bar_nr, Error **errp)
> >>  {
> >>      int ret;
> >>      char *name;
> >> @@ -338,7 +362,7 @@ int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
> >>      ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr,
> >>                      0, &dev->msix_exclusive_bar,
> >>                      bar_nr, bar_pba_offset,
> >> -                    0);
> >> +                    0, errp);
> >>      if (ret) {
> >>          return ret;
> >>      }
> >> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> >> index 52a4123..fada834 100644
> >> --- a/hw/scsi/megasas.c
> >> +++ b/hw/scsi/megasas.c
> >> @@ -2360,9 +2360,12 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
> >>  
> >>      if (megasas_use_msix(s) &&
> >>          msix_init(dev, 15, &s->mmio_io, b->mmio_bar, 0x2000,
> >> -                  &s->mmio_io, b->mmio_bar, 0x3800, 0x68)) {
> >> +                  &s->mmio_io, b->mmio_bar, 0x3800, 0x68, &err)) {
> >> +        /*TODO: check msix_init's error, and should fail on msix=on */
> >> +        error_report_err(err);
> >>          s->msix = ON_OFF_AUTO_OFF;
> >>      }
> >> +
> >>      if (pci_is_express(dev)) {
> >>          pcie_endpoint_cap_init(dev, 0xa0);
> >>      }
> >> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> >> index 0ace273..153b006 100644
> >> --- a/hw/usb/hcd-xhci.c
> >> +++ b/hw/usb/hcd-xhci.c
> >> @@ -3703,11 +3703,14 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
> >>      }
> >>  
> >>      if (xhci->msix != ON_OFF_AUTO_OFF) {
> >> -        /* TODO check for errors */
> >> -        msix_init(dev, xhci->numintrs,
> >> -                  &xhci->mem, 0, OFF_MSIX_TABLE,
> >> -                  &xhci->mem, 0, OFF_MSIX_PBA,
> >> -                  0x90);
> >> +        /* TODO check for errors, and should fail when msix=on */
> >> +        ret = msix_init(dev, xhci->numintrs,
> >> +                        &xhci->mem, 0, OFF_MSIX_TABLE,
> >> +                        &xhci->mem, 0, OFF_MSIX_PBA,
> >> +                        0x90, &err);
> >> +        if (ret) {
> >> +            error_report_err(err);
> >> +        }
> >>      }
> >>  }
> >>  
> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> >> index d7dbe0e..0bcfaad 100644
> >> --- a/hw/vfio/pci.c
> >> +++ b/hw/vfio/pci.c
> >> @@ -1432,6 +1432,7 @@ static void vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
> >>  static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
> >>  {
> >>      int ret;
> >> +    Error *local_err = NULL;
> >>  
> >>      vdev->msix->pending = g_malloc0(BITS_TO_LONGS(vdev->msix->entries) *
> >>                                      sizeof(unsigned long));
> >> @@ -1439,12 +1440,15 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
> >>                      vdev->bars[vdev->msix->table_bar].region.mem,
> >>                      vdev->msix->table_bar, vdev->msix->table_offset,
> >>                      vdev->bars[vdev->msix->pba_bar].region.mem,
> >> -                    vdev->msix->pba_bar, vdev->msix->pba_offset, pos);
> >> +                    vdev->msix->pba_bar, vdev->msix->pba_offset, pos,
> >> +                    &local_err);
> >>      if (ret < 0) {
> >>          if (ret == -ENOTSUP) {
> >> +            error_report_err(local_err);
> >>              return 0;
> >>          }
> >> -        error_setg(errp, "msix_init failed");
> >> +
> >> +        error_propagate(errp, local_err);
> >>          return ret;
> >>      }
> >>  
> >> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> >> index 06831de..5acce38 100644
> >> --- a/hw/virtio/virtio-pci.c
> >> +++ b/hw/virtio/virtio-pci.c
> >> @@ -1693,13 +1693,12 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
> >>  
> >>      if (proxy->nvectors) {
> >>          int err = msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors,
> >> -                                          proxy->msix_bar_idx);
> >> +                                          proxy->msix_bar_idx, errp);
> >> +        /* Any error other than -ENOTSUP(board's MSI support is broken)
> >> +         * is a programming error. */
> >> +        assert(!err || err == -ENOTSUP);
> >>          if (err) {
> >> -            /* Notice when a system that supports MSIx can't initialize it.  */
> >> -            if (err != -ENOTSUP) {
> >> -                error_report("unable to init msix vectors to %" PRIu32,
> >> -                             proxy->nvectors);
> >> -            }
> >> +            error_report_err(*errp);
> >>              proxy->nvectors = 0;
> >>          }
> >>      }
> >> diff --git a/include/hw/pci/msix.h b/include/hw/pci/msix.h
> >> index 048a29d..1f27658 100644
> >> --- a/include/hw/pci/msix.h
> >> +++ b/include/hw/pci/msix.h
> >> @@ -9,9 +9,10 @@ MSIMessage msix_get_message(PCIDevice *dev, unsigned int vector);
> >>  int msix_init(PCIDevice *dev, unsigned short nentries,
> >>                MemoryRegion *table_bar, uint8_t table_bar_nr,
> >>                unsigned table_offset, MemoryRegion *pba_bar,
> >> -              uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos);
> >> +              uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos,
> >> +              Error **errp);
> >>  int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
> >> -                            uint8_t bar_nr);
> >> +                            uint8_t bar_nr, Error **errp);
> >>  
> >>  void msix_write_config(PCIDevice *dev, uint32_t address, uint32_t val, int len);
> >>  
> >> -- 
> >> 2.1.0
> >>
> >>

Re: [Qemu-devel] [PATCH v7 03/10] pci: Convert msix_init() to Error and fix callers to check it

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 10/01/2017 18:54, Michael S. Tsirkin wrote:
>> On Mon, Nov 14, 2016 at 03:25:33PM +0800, Cao jin wrote:
>>> msix_init() reports errors with error_report(), which is wrong when
>>> it's used in realize().  The same issue was fixed for msi_init() in
>>> commit 1108b2f.
>>>
>>> For some devices(like e1000e, vmxnet3) who won't fail because of
>>> msix_init's failure, suppress the error report by passing NULL error object.
>>>
>>> Bonus: add comment for msix_init.
>>>
>>> CC: Jiri Pirko <jiri@resnulli.us>
>>> CC: Gerd Hoffmann <kraxel@redhat.com>
>>> CC: Dmitry Fleytman <dmitry@daynix.com>
>>> CC: Jason Wang <jasowang@redhat.com>
>>> CC: Michael S. Tsirkin <mst@redhat.com>
>>> CC: Hannes Reinecke <hare@suse.de>
>>> CC: Paolo Bonzini <pbonzini@redhat.com>
>>> CC: Alex Williamson <alex.williamson@redhat.com>
>>> CC: Markus Armbruster <armbru@redhat.com>
>>> CC: Marcel Apfelbaum <marcel@redhat.com>
>>>
>>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>>> Reviewed-by: Hannes Reinecke <hare@suse.com>
>>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> 
>> I'd rather add a new API. Once that is merged you can make device
>> changes avoiding a flag day. Merge this through individual trees. At the
>> end of the day we can drop the old API when it's no longer needed.
>
> I think that's the worst.  We don't need another partial transition and

Seconded.

> this series is all but big.  The main issue is that it was handled badly
> in the past, so people tend not to trust it.
>
> If anything, if there are independent patches in the series that can be
> merged through USB or SCSI trees, before this one, we can do that.

I guess some of the later patches are follow-up cleanups that could be
merged separately.  Might require reordering, then re-review.  But would
this really be worth the trouble?  Merging through another tree is no
pixie dust for quality.  If we can get the review and testing we need
only by merging every shard through the exact right tree, we're doing it
wrong.

Yes, we absolutely want a maintainer to review patches, and yes, that's
not trivial for work spanning subsystems.  But we got the R-bys alright.

Elsewhere in this thread, you ask for specific testing in addition to a
maintainer's R-by.  I think that's fair.

Re: [Qemu-devel] [PATCH v7 00/10] Convert msix_init() to error

[Cao jin]

On 01/10/2017 05:45 AM, Michael S. Tsirkin wrote:
> On Mon, Nov 14, 2016 at 03:25:30PM +0800, Cao jin wrote:
>> v7 changelog:
>> 1. fix the segfaut bug in patch 2. So drop the all the R-b of it,
>>    please take a look, there is detailed description in the patch.
>> 2. add the R-b from Hannes Reinecke
>>
>> Test:
>> 1. make check: pass
>> 2. After applied all the patch, command line test for all the
>>    affected devices, just make sure device realize process is ok,
>>    no crash, but no further use of device.
> 
> Consider the megasas device for example, don't you
> need to test that the change actually does what
> it's intended to do?
> 

Thanks very much for your reminding, it does has a problem in patch
"megasas: remove unnecessary megasas_use_msix()", it is only one screw
megasas. Sorry for that.

Install linux distro to test megasas as following:

./qemu-system-x86_64 --enable-kvm -m 1024 -device
megasas,id=scsi0,bus=pci.0 -drive
file=/xx/xx/scsi-disk.img,if=none,id=drive-scsi0 -device
scsi-disk,bus=scsi0.0,channel=0,scsi-id=4,lun=0,drive=drive-scsi0,id=scsi0-4
-cdrom /xx/vm/Fedora-Server-DVD-x86_64-23.iso [-boot once=d] --monitor stdio

Sincerely,
Cao jin

Re: [Qemu-devel] [PATCH v7 03/10] pci: Convert msix_init() to Error and fix callers to check it

[Michael S. Tsirkin]

On Mon, Nov 14, 2016 at 03:25:33PM +0800, Cao jin wrote:
> msix_init() reports errors with error_report(), which is wrong when
> it's used in realize().  The same issue was fixed for msi_init() in
> commit 1108b2f.

Is there some way to trigger this? Markus says "QMP is broken"
so surely one can make it fail somehow?

> 
> For some devices(like e1000e, vmxnet3) who won't fail because of
> msix_init's failure, suppress the error report by passing NULL error object.
> 
> Bonus: add comment for msix_init.
> 
> CC: Jiri Pirko <jiri@resnulli.us>
> CC: Gerd Hoffmann <kraxel@redhat.com>
> CC: Dmitry Fleytman <dmitry@daynix.com>
> CC: Jason Wang <jasowang@redhat.com>
> CC: Michael S. Tsirkin <mst@redhat.com>
> CC: Hannes Reinecke <hare@suse.de>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Alex Williamson <alex.williamson@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Marcel Apfelbaum <marcel@redhat.com>
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Hannes Reinecke <hare@suse.com>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>

So how about making just this API change as small as possible
and I'll merge. Enhancements can then go through other trees.


> ---
>  hw/block/nvme.c        |  5 ++++-
>  hw/misc/ivshmem.c      |  8 ++++----
>  hw/net/e1000e.c        |  6 +++++-
>  hw/net/rocker/rocker.c |  7 ++++++-
>  hw/net/vmxnet3.c       |  8 ++++++--
>  hw/pci/msix.c          | 34 +++++++++++++++++++++++++++++-----
>  hw/scsi/megasas.c      |  5 ++++-
>  hw/usb/hcd-xhci.c      | 13 ++++++++-----
>  hw/vfio/pci.c          |  8 ++++++--
>  hw/virtio/virtio-pci.c | 11 +++++------
>  include/hw/pci/msix.h  |  5 +++--
>  11 files changed, 80 insertions(+), 30 deletions(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index d479fd2..2d703c8 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -831,6 +831,7 @@ static int nvme_init(PCIDevice *pci_dev)
>  {
>      NvmeCtrl *n = NVME(pci_dev);
>      NvmeIdCtrl *id = &n->id_ctrl;
> +    Error *err = NULL;
>  
>      int i;
>      int64_t bs_size;
> @@ -872,7 +873,9 @@ static int nvme_init(PCIDevice *pci_dev)
>      pci_register_bar(&n->parent_obj, 0,
>          PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_64,
>          &n->iomem);
> -    msix_init_exclusive_bar(&n->parent_obj, n->num_queues, 4);
> +    if (msix_init_exclusive_bar(&n->parent_obj, n->num_queues, 4, &err)) {
> +        error_report_err(err);
> +    }
>  
>      id->vid = cpu_to_le16(pci_get_word(pci_conf + PCI_VENDOR_ID));
>      id->ssvid = cpu_to_le16(pci_get_word(pci_conf + PCI_SUBSYSTEM_VENDOR_ID));


This previously ignored errors, so should probably pass in NULL.

> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index 230e51b..ca6f804 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -749,13 +749,13 @@ static void ivshmem_reset(DeviceState *d)
>      }
>  }
>  
> -static int ivshmem_setup_interrupts(IVShmemState *s)
> +static int ivshmem_setup_interrupts(IVShmemState *s, Error **errp)
>  {
>      /* allocate QEMU callback data for receiving interrupts */
>      s->msi_vectors = g_malloc0(s->vectors * sizeof(MSIVector));
>  
>      if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
> -        if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1)) {
> +        if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1, errp)) {
>              return -1;
>          }
>  
> @@ -897,8 +897,8 @@ static void ivshmem_common_realize(PCIDevice *dev, Error **errp)
>          qemu_chr_fe_set_handlers(&s->server_chr, ivshmem_can_receive,
>                                   ivshmem_read, NULL, s, NULL, true);
>  
> -        if (ivshmem_setup_interrupts(s) < 0) {
> -            error_setg(errp, "failed to initialize interrupts");
> +        if (ivshmem_setup_interrupts(s, errp) < 0) {
> +            error_prepend(errp, "Failed to initialize interrupts: ");
>              return;
>          }
>      }
> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
> index 4994e1c..74cbbef 100644
> --- a/hw/net/e1000e.c
> +++ b/hw/net/e1000e.c
> @@ -292,7 +292,11 @@ e1000e_init_msix(E1000EState *s)
>                          E1000E_MSIX_IDX, E1000E_MSIX_TABLE,
>                          &s->msix,
>                          E1000E_MSIX_IDX, E1000E_MSIX_PBA,
> -                        0xA0);
> +                        0xA0, NULL);
> +
> +    /* Any error other than -ENOTSUP(board's MSI support is broken)
> +     * is a programming error. Fall back to INTx silently on -ENOTSUP */
> +    assert(!res || res == -ENOTSUP);
>  
>      if (res < 0) {
>          trace_e1000e_msix_init_fail(res);

I'd rather move these asserts to a separate patch as noted below.

> diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
> index e9d215a..8f829f2 100644
> --- a/hw/net/rocker/rocker.c
> +++ b/hw/net/rocker/rocker.c
> @@ -1256,14 +1256,19 @@ static int rocker_msix_init(Rocker *r)
>  {
>      PCIDevice *dev = PCI_DEVICE(r);
>      int err;
> +    Error *local_err = NULL;
>  
>      err = msix_init(dev, ROCKER_MSIX_VEC_COUNT(r->fp_ports),
>                      &r->msix_bar,
>                      ROCKER_PCI_MSIX_BAR_IDX, ROCKER_PCI_MSIX_TABLE_OFFSET,
>                      &r->msix_bar,
>                      ROCKER_PCI_MSIX_BAR_IDX, ROCKER_PCI_MSIX_PBA_OFFSET,
> -                    0);
> +                    0, &local_err);
> +    /* Any error other than -ENOTSUP(board's MSI support is broken)
> +     * is a programming error. */
> +    assert(!err || err == -ENOTSUP);
>      if (err) {
> +        error_report_err(local_err);
>          return err;
>      }
>  

I'd rather move these asserts to a separate patch as noted below.

> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 92f6af9..a433cc0 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -2191,10 +2191,14 @@ vmxnet3_init_msix(VMXNET3State *s)
>                          VMXNET3_MSIX_BAR_IDX, VMXNET3_OFF_MSIX_TABLE,
>                          &s->msix_bar,
>                          VMXNET3_MSIX_BAR_IDX, VMXNET3_OFF_MSIX_PBA(s),
> -                        VMXNET3_MSIX_OFFSET(s));
> +                        VMXNET3_MSIX_OFFSET(s), NULL);
> +
> +    /* Any error other than -ENOTSUP(board's MSI support is broken)
> +     * is a programming error. Fall back to INTx on -ENOTSUP */
> +    assert(!res || res == -ENOTSUP);
>  
>      if (0 > res) {
> -        VMW_WRPRN("Failed to initialize MSI-X, error %d", res);
> +        VMW_WRPRN("Failed to initialize MSI-X, board's MSI support is broken");
>          s->msix_used = false;
>      } else {
>          if (!vmxnet3_use_msix_vectors(s, VMXNET3_MAX_INTRS)) {

I'd rather move these changes to a separate patch.

> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> index 0cee631..d03016f 100644
> --- a/hw/pci/msix.c
> +++ b/hw/pci/msix.c
> @@ -21,6 +21,7 @@
>  #include "hw/pci/pci.h"
>  #include "hw/xen/xen.h"
>  #include "qemu/range.h"
> +#include "qapi/error.h"
>  
>  #define MSIX_CAP_LENGTH 12
>  
> @@ -238,11 +239,29 @@ static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
>      }
>  }
>  
> -/* Initialize the MSI-X structures */
> +/* Make PCI device @dev MSI-X capable
> + * @nentries is the max number of MSI-X vectors that the device support.
> + * @table_bar is the MemoryRegion that MSI-X table structure resides.
> + * @table_bar_nr is number of base address register corresponding to @table_bar.
> + * @table_offset indicates the offset that the MSI-X table structure starts with
> + * in @table_bar.
> + * @pba_bar is the MemoryRegion that the Pending Bit Array structure resides.
> + * @pba_bar_nr is number of base address register corresponding to @pba_bar.
> + * @pba_offset indicates the offset that the Pending Bit Array structure
> + * starts with in @pba_bar.
> + * Non-zero @cap_pos puts capability MSI-X at that offset in PCI config space.
> + * @errp is for returning errors.
> + *
> + * Return 0 on success; set @errp and return -errno on error:
> + * -ENOTSUP means lacking msi support for a msi-capable platform.
> + * -EINVAL means capability overlap, happens when @cap_pos is non-zero,
> + * also means a programming error, except device assignment, which can check
> + * if a real HW is broken.*/

/*
 * Multi-line comments
 * should look
 * like this
 */

/* Not
 * like this */


>  int msix_init(struct PCIDevice *dev, unsigned short nentries,
>                MemoryRegion *table_bar, uint8_t table_bar_nr,
>                unsigned table_offset, MemoryRegion *pba_bar,
> -              uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos)
> +              uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos,
> +              Error **errp)
>  {
>      int cap;
>      unsigned table_size, pba_size;
> @@ -250,10 +269,12 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
>  
>      /* Nothing to do if MSI is not supported by interrupt controller */
>      if (!msi_nonbroken) {
> +        error_setg(errp, "MSI-X is not supported by interrupt controller");
>          return -ENOTSUP;
>      }
>  
>      if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1) {
> +        error_setg(errp, "The number of MSI-X vectors is invalid");
>          return -EINVAL;
>      }
>  
> @@ -266,10 +287,13 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
>          table_offset + table_size > memory_region_size(table_bar) ||
>          pba_offset + pba_size > memory_region_size(pba_bar) ||
>          (table_offset | pba_offset) & PCI_MSIX_FLAGS_BIRMASK) {
> +        error_setg(errp, "table & pba overlap, or they don't fit in BARs,"
> +                   " or don't align");
>          return -EINVAL;
>      }
>  
> -    cap = pci_add_capability(dev, PCI_CAP_ID_MSIX, cap_pos, MSIX_CAP_LENGTH);
> +    cap = pci_add_capability2(dev, PCI_CAP_ID_MSIX,
> +                              cap_pos, MSIX_CAP_LENGTH, errp);
>      if (cap < 0) {
>          return cap;
>      }
> @@ -306,7 +330,7 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
>  }
>  
>  int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
> -                            uint8_t bar_nr)
> +                            uint8_t bar_nr, Error **errp)
>  {
>      int ret;
>      char *name;
> @@ -338,7 +362,7 @@ int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
>      ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr,
>                      0, &dev->msix_exclusive_bar,
>                      bar_nr, bar_pba_offset,
> -                    0);
> +                    0, errp);
>      if (ret) {
>          return ret;
>      }
> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index 52a4123..fada834 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -2360,9 +2360,12 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
>  
>      if (megasas_use_msix(s) &&
>          msix_init(dev, 15, &s->mmio_io, b->mmio_bar, 0x2000,
> -                  &s->mmio_io, b->mmio_bar, 0x3800, 0x68)) {
> +                  &s->mmio_io, b->mmio_bar, 0x3800, 0x68, &err)) {
> +        /*TODO: check msix_init's error, and should fail on msix=on */

Missing space after *.

> +        error_report_err(err);
>          s->msix = ON_OFF_AUTO_OFF;
>      }
> +
>      if (pci_is_express(dev)) {
>          pcie_endpoint_cap_init(dev, 0xa0);
>      }

This disabled msix silently so should probably keep doing so for now.
Then you can keep the existing TODO.

> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> index 0ace273..153b006 100644
> --- a/hw/usb/hcd-xhci.c
> +++ b/hw/usb/hcd-xhci.c
> @@ -3703,11 +3703,14 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
>      }
>  
>      if (xhci->msix != ON_OFF_AUTO_OFF) {
> -        /* TODO check for errors */
> -        msix_init(dev, xhci->numintrs,
> -                  &xhci->mem, 0, OFF_MSIX_TABLE,
> -                  &xhci->mem, 0, OFF_MSIX_PBA,
> -                  0x90);
> +        /* TODO check for errors, and should fail when msix=on */
> +        ret = msix_init(dev, xhci->numintrs,
> +                        &xhci->mem, 0, OFF_MSIX_TABLE,
> +                        &xhci->mem, 0, OFF_MSIX_PBA,
> +                        0x90, &err);
> +        if (ret) {
> +            error_report_err(err);
> +        }
>      }
>  }
>  

Please just pass in NULL in this patch. Do enhancements
in a separate one to be merged by someone who cares.


> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index d7dbe0e..0bcfaad 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -1432,6 +1432,7 @@ static void vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
>  static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
>  {
>      int ret;
> +    Error *local_err = NULL;
>  
>      vdev->msix->pending = g_malloc0(BITS_TO_LONGS(vdev->msix->entries) *
>                                      sizeof(unsigned long));
> @@ -1439,12 +1440,15 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
>                      vdev->bars[vdev->msix->table_bar].region.mem,
>                      vdev->msix->table_bar, vdev->msix->table_offset,
>                      vdev->bars[vdev->msix->pba_bar].region.mem,
> -                    vdev->msix->pba_bar, vdev->msix->pba_offset, pos);
> +                    vdev->msix->pba_bar, vdev->msix->pba_offset, pos,
> +                    &local_err);
>      if (ret < 0) {
>          if (ret == -ENOTSUP) {
> +            error_report_err(local_err);
>              return 0;
>          }
> -        error_setg(errp, "msix_init failed");
> +
> +        error_propagate(errp, local_err);
>          return ret;
>      }
>  
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 06831de..5acce38 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1693,13 +1693,12 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
>  
>      if (proxy->nvectors) {
>          int err = msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors,
> -                                          proxy->msix_bar_idx);
> +                                          proxy->msix_bar_idx, errp);
> +        /* Any error other than -ENOTSUP(board's MSI support is broken)
> +         * is a programming error. */
> +        assert(!err || err == -ENOTSUP);

This is spread in too many places. Almost everyone except vfio
wants this, so simple msix_init should include this assert,
and vfio should have a special _check variant that
does not. You can make these asserts in a separate patch
though. This one is already too big.

>          if (err) {
> -            /* Notice when a system that supports MSIx can't initialize it.  */
> -            if (err != -ENOTSUP) {
> -                error_report("unable to init msix vectors to %" PRIu32,
> -                             proxy->nvectors);
> -            }
> +            error_report_err(*errp);
>              proxy->nvectors = 0;
>          }
>      }
> diff --git a/include/hw/pci/msix.h b/include/hw/pci/msix.h
> index 048a29d..1f27658 100644
> --- a/include/hw/pci/msix.h
> +++ b/include/hw/pci/msix.h
> @@ -9,9 +9,10 @@ MSIMessage msix_get_message(PCIDevice *dev, unsigned int vector);
>  int msix_init(PCIDevice *dev, unsigned short nentries,
>                MemoryRegion *table_bar, uint8_t table_bar_nr,
>                unsigned table_offset, MemoryRegion *pba_bar,
> -              uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos);
> +              uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos,
> +              Error **errp);
>  int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
> -                            uint8_t bar_nr);
> +                            uint8_t bar_nr, Error **errp);
>  
>  void msix_write_config(PCIDevice *dev, uint32_t address, uint32_t val, int len);
>  
> -- 
> 2.1.0
> 
> 

Re: [Qemu-devel] [PATCH v7 03/10] pci: Convert msix_init() to Error and fix callers to check it

[Michael S. Tsirkin]

On Wed, Jan 11, 2017 at 10:55:24AM +0100, Markus Armbruster wrote:
> I guess some of the later patches are follow-up cleanups that could be
> merged separately.  Might require reordering, then re-review.  But would
> this really be worth the trouble?  Merging through another tree is no
> pixie dust for quality.  If we can get the review and testing we need
> only by merging every shard through the exact right tree, we're doing it
> wrong.

Well I'm just looking for a way to merge something minimal so I can stop
spending time on this.

-- 
MST