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

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

[Cao jin]

v5 changelog:
1. Address to all comments of Markus
2. Add the given R-b to each commit message.

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           | 11 +++++---
 hw/pci/msix.c          | 42 ++++++++++++++++++++++++-----
 hw/scsi/megasas.c      | 49 +++++++++++++++++++---------------
 hw/usb/hcd-xhci.c      | 71 ++++++++++++++++++++++++++++++--------------------
 hw/vfio/pci.c          |  4 ++-
 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 v5 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 v5 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>

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 | 39 +++++++++++++++++++--------------------
 1 file changed, 19 insertions(+), 20 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 4acf0c6..eb1dca5 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,7 +3648,22 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
         xhci->max_pstreams_mask = 0;
     }
 
-    xhci->mfwrap_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, xhci_mfwrap_timer, 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);
+    }
 
     memory_region_init(&xhci->mem, OBJECT(xhci), "xhci", LEN_REGS);
     memory_region_init_io(&xhci->mem_cap, OBJECT(xhci), &xhci_cap_ops, xhci,
@@ -3697,6 +3693,9 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
                      PCI_BASE_ADDRESS_SPACE_MEMORY|PCI_BASE_ADDRESS_MEM_TYPE_64,
                      &xhci->mem);
 
+    usb_xhci_init(xhci);
+    xhci->mfwrap_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, xhci_mfwrap_timer, xhci);
+
     if (pci_bus_is_express(dev->bus) ||
         xhci_get_flag(xhci, XHCI_FLAG_FORCE_PCIE_ENDCAP)) {
         ret = pcie_endpoint_cap_init(dev, 0xa0);
-- 
2.1.0

[Qemu-devel] [PATCH v5 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>
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          |  4 +++-
 hw/virtio/virtio-pci.c | 11 +++++------
 include/hw/pci/msix.h  |  5 +++--
 11 files changed, 77 insertions(+), 29 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 eb1dca5..05dc944 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..6fbd30f 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 *err = NULL;
 
     vdev->msix->pending = g_malloc0(BITS_TO_LONGS(vdev->msix->entries) *
                                     sizeof(unsigned long));
@@ -1439,7 +1440,8 @@ 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,
+                    &err);
     if (ret < 0) {
         if (ret == -ENOTSUP) {
             return 0;
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 v5 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>
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 v5 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 05dc944..4767045 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 {
@@ -3666,6 +3668,28 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
     }
 
     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 v5 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>
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 v5 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>
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 v5 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 v5 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 v5 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>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/pci/msi.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/hw/pci/msi.c b/hw/pci/msi.c
index a87b227..443682b 100644
--- a/hw/pci/msi.c
+++ b/hw/pci/msi.c
@@ -201,9 +201,14 @@ 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 ((nr_vectors == 0) ||
+        (nr_vectors > PCI_MSI_VECTORS_MAX) ||
+        (nr_vectors & (nr_vectors - 1))) {
+        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 v5 00/10] Convert msix_init() to error

[Cao jin]

sorry I forget to cc maintainers in cover letter.

On 11/03/2016 12:06 PM, Cao jin wrote:
> v5 changelog:
> 1. Address to all comments of Markus
> 2. Add the given R-b to each commit message.
>
> 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           | 11 +++++---
>   hw/pci/msix.c          | 42 ++++++++++++++++++++++++-----
>   hw/scsi/megasas.c      | 49 +++++++++++++++++++---------------
>   hw/usb/hcd-xhci.c      | 71 ++++++++++++++++++++++++++++++--------------------
>   hw/vfio/pci.c          |  4 ++-
>   hw/virtio/virtio-pci.c | 11 ++++----
>   include/hw/pci/msix.h  |  5 ++--
>   12 files changed, 164 insertions(+), 101 deletions(-)
>

-- 
Yours Sincerely,

Cao jin

[Qemu-devel] [PATCH v5 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>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/pci/msi.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Really sorry, I forget to commit what I amend...

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 v5 10/10] msi_init: convert assert to return -errno

[Cao jin]

Please ignore this one, I forget to commit the amendment...
Already send the right one.

Cao jin

On 11/03/2016 12:06 PM, Cao jin wrote:
> 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>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>   hw/pci/msi.c | 11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/hw/pci/msi.c b/hw/pci/msi.c
> index a87b227..443682b 100644
> --- a/hw/pci/msi.c
> +++ b/hw/pci/msi.c
> @@ -201,9 +201,14 @@ 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 ((nr_vectors == 0) ||
> +        (nr_vectors > PCI_MSI_VECTORS_MAX) ||
> +        (nr_vectors & (nr_vectors - 1))) {
> +        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);
>
>

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

[Marcel Apfelbaum]

On 11/03/2016 06:06 AM, 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>
> 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          |  4 +++-
>  hw/virtio/virtio-pci.c | 11 +++++------
>  include/hw/pci/msix.h  |  5 +++--
>  11 files changed, 77 insertions(+), 29 deletions(-)
>

[...]

> 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 */

Why this "TODO", can't we do something similar to other changes already done in this patch?

> +        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 eb1dca5..05dc944 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..6fbd30f 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 *err = NULL;
>
>      vdev->msix->pending = g_malloc0(BITS_TO_LONGS(vdev->msix->entries) *
>                                      sizeof(unsigned long));
> @@ -1439,7 +1440,8 @@ 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,
> +                    &err);

Do we pass the err pointer to msix_init, but we don't do anything with it?

Also since we do have an *errp in the function already, I suggest
renaming err -> local_err or something. (only if the series needs a re-spin)

>      if (ret < 0) {
>          if (ret == -ENOTSUP) {
>              return 0;
> 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);

Why do we report the error here and we don't let the propagation
mechanism do its thing? We can prep-end the current  message, I think.

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


Other than a few questions, the patch looks good to me.

Thanks!
Marcel

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

[Marcel Apfelbaum]

On 11/03/2016 08:10 AM, Cao jin wrote:
> According to the disscussion:
> http://lists.nongnu.org/archive/html/qemu-devel/2016-09/msg08215.html
>

Hi,

Intense discussion, but I learned a lot from it.

> 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>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/pci/msi.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> Really sorry, I forget to commit what I amend...
>
> 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);
>
>


Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>


Thanks,
Marcel

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

[Cao jin]

On 11/03/2016 07:38 PM, Marcel Apfelbaum wrote:
> On 11/03/2016 06:06 AM, Cao jin wrote:

>>
>
> [...]
>
>> 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 */
>
> Why this "TODO", can't we do something similar to other changes already
> done in this patch?
>

The 1st version of this series handles the error in this patch. look at 
the comments:

http://lists.nongnu.org/archive/html/qemu-devel/2016-08/msg03192.html

*First convert msix_init() without changing behavior, by having every 
caller of msix_init() immediately pass the error received to 
error_report_err().  Then clean up the callers one after the other.*

So later, this patch looks like what it is now. I feel it also make this 
patch thinner, easier to review.

>> +        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

>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index d7dbe0e..6fbd30f 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 *err = NULL;
>>
>>      vdev->msix->pending = g_malloc0(BITS_TO_LONGS(vdev->msix->entries) *
>>                                      sizeof(unsigned long));
>> @@ -1439,7 +1440,8 @@ 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,
>> +                    &err);
>
> Do we pass the err pointer to msix_init, but we don't do anything with it?
>
> Also since we do have an *errp in the function already, I suggest
> renaming err -> local_err or something. (only if the series needs a
> re-spin)
>

yes, it maybe need a re-spin, thanks

>>      if (ret < 0) {
>>          if (ret == -ENOTSUP) {
>>              return 0;
>> 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);
>
> Why do we report the error here and we don't let the propagation
> mechanism do its thing? We can prep-end the current  message, I think.
>

The original behaviour won't fail on msix init failure, so, report & 
free the Error keep the behaviour same as before,  propagation will 
results in failing to create virtio device.

>
>
> Other than a few questions, the patch looks good to me.
>
> Thanks!
> Marcel
>
-- 
Yours Sincerely,

Cao jin

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

[Marcel Apfelbaum]

On 11/04/2016 05:01 AM, Cao jin wrote:
>
>
> On 11/03/2016 07:38 PM, Marcel Apfelbaum wrote:
>> On 11/03/2016 06:06 AM, Cao jin wrote:
>
>>>
>>
>> [...]
>>
>>> 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 */
>>
>> Why this "TODO", can't we do something similar to other changes already
>> done in this patch?
>>
>
> The 1st version of this series handles the error in this patch. look at the comments:
>
> http://lists.nongnu.org/archive/html/qemu-devel/2016-08/msg03192.html
>
> *First convert msix_init() without changing behavior, by having every caller of msix_init() immediately pass the error received to error_report_err().  Then clean up the callers one after the other.*
>
> So later, this patch looks like what it is now. I feel it also make this patch thinner, easier to review.
>

I thought it can be solved in the same way as in the other places,
however I am OK with it.




>>> +        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
>
>>>
>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>> index d7dbe0e..6fbd30f 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 *err = NULL;
>>>
>>>      vdev->msix->pending = g_malloc0(BITS_TO_LONGS(vdev->msix->entries) *
>>>                                      sizeof(unsigned long));
>>> @@ -1439,7 +1440,8 @@ 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,
>>> +                    &err);
>>
>> Do we pass the err pointer to msix_init, but we don't do anything with it?
>>
>> Also since we do have an *errp in the function already, I suggest
>> renaming err -> local_err or something. (only if the series needs a
>> re-spin)
>>
>
> yes, it maybe need a re-spin, thanks
>
>>>      if (ret < 0) {
>>>          if (ret == -ENOTSUP) {
>>>              return 0;
>>> 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);
>>
>> Why do we report the error here and we don't let the propagation
>> mechanism do its thing? We can prep-end the current  message, I think.
>>
>
> The original behaviour won't fail on msix init failure, so, report & free the Error keep the behaviour same as before,  propagation will results in failing to create virtio device.
>

Got it

Thanks,
Marcel

>>
>>
>> Other than a few questions, the patch looks good to me.
>>
>> Thanks!
>> Marcel
>>