[Qemu-devel] [PATCH v10 0/8] the reset of msix_init series

[Qemu-devel] [PATCH v10 0/8] the reset of msix_init series

[Cao jin]

v10 changelog:
1. drop the unliked patch, introduce a new patch 1 according to mst's comments.
2. base on the new patch, remove the following statements

        /* Any error other than -ENOTSUP(board's MSI support is broken)
         * is a programming error */
        assert(!ret || ret == -ENOTSUP);

   for the affected device: megasas, hcd-xhci. This is trivial changes,
   so I left the R-bs where it was.

Test:
1. Detailed test via command line as v9
2. make check hangs at: GTESTER check-qtest-x86_64. After ctrl-C, it says:

    make: *** [check-qtest-x86_64] Interrupt
    qemu-system-x86_64: Failed to read msg header. Read -1 instead of 12. Original request 11.
    qemu-system-x86_64: vhost VQ 0 ring restore failed: -1: Input/output error (5)
    qemu-system-x86_64: Failed to set msg fds.
    qemu-system-x86_64: vhost VQ 0 ring restore failed: -1: Invalid argument (22)

    qemu-system-x86_64: Failed to set msg fds.
    qemu-system-x86_64: vhost VQ 1 ring restore failed: -1: Invalid argument (22)

   Is it a regresstion or I missed something?

CC: Jason Wang <jasowang@redhat.com>
CC: Gerd Hoffmann <kraxel@redhat.com>
CC: Dmitry Fleytman <dmitry@daynix.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 (8):
  msix: Rename and create a wrapper
  megasas: change behaviour of msix switch
  hcd-xhci: change behaviour of msix switch
  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
  megasas: remove unnecessary megasas_use_msix()

 hw/net/vmxnet3.c      | 40 +++++++++++++++-------------------------
 hw/pci/msi.c          |  9 ++++++---
 hw/pci/msix.c         | 30 +++++++++++++++++++++---------
 hw/scsi/megasas.c     | 48 +++++++++++++++++++++++++-----------------------
 hw/usb/hcd-xhci.c     | 29 +++++++++++++++++++++--------
 hw/vfio/pci.c         | 12 ++++++------
 include/hw/pci/msix.h |  5 +++++
 7 files changed, 99 insertions(+), 74 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH v10 1/8] msix: Rename and create a wrapper

[Cao jin]

Rename msix_init to msix_validate_and_init, and use it from vfio which
might get a reasonable -EINVAL.  New a wrapper msix_init which assert the
programming error for debug purpose and use it from other devices.

CC: Alex Williamson <alex.williamson@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>
---
 hw/pci/msix.c         | 30 +++++++++++++++++++++---------
 hw/vfio/pci.c         | 12 ++++++------
 include/hw/pci/msix.h |  5 +++++
 3 files changed, 32 insertions(+), 15 deletions(-)

diff --git a/hw/pci/msix.c b/hw/pci/msix.c
index bb54e8b0ac37..2b7541ab2c8d 100644
--- a/hw/pci/msix.c
+++ b/hw/pci/msix.c
@@ -239,6 +239,19 @@ static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
     }
 }
 
+/* Just a wrapper to check the return value */
+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,
+              Error **errp)
+{
+    int ret = msix_validate_and_init(dev, nentries, table_bar, table_bar_nr,
+            table_offset, pba_bar, pba_bar_nr, pba_offset, cap_pos, errp);
+
+    assert(ret != -EINVAL);
+    return ret;
+}
 /*
  * Make PCI device @dev MSI-X capable
  * @nentries is the max number of MSI-X vectors that the device support.
@@ -259,11 +272,11 @@ static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
  * 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,
-              Error **errp)
+int msix_validate_and_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, Error **errp)
 {
     int cap;
     unsigned table_size, pba_size;
@@ -361,10 +374,9 @@ int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
     memory_region_init(&dev->msix_exclusive_bar, OBJECT(dev), name, bar_size);
     g_free(name);
 
-    ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr,
-                    0, &dev->msix_exclusive_bar,
-                    bar_nr, bar_pba_offset,
-                    0, errp);
+    ret = msix_validate_and_init(dev, nentries, &dev->msix_exclusive_bar,
+                                 bar_nr, 0, &dev->msix_exclusive_bar,
+                                 bar_nr, bar_pba_offset, 0, errp);
     if (ret) {
         return ret;
     }
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 332f41d6627f..06828b537a75 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1436,12 +1436,12 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
 
     vdev->msix->pending = g_malloc0(BITS_TO_LONGS(vdev->msix->entries) *
                                     sizeof(unsigned long));
-    ret = msix_init(&vdev->pdev, vdev->msix->entries,
-                    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,
-                    &err);
+    ret = msix_validate_and_init(&vdev->pdev, vdev->msix->entries,
+                             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,
+                             &err);
     if (ret < 0) {
         if (ret == -ENOTSUP) {
             error_report_err(err);
diff --git a/include/hw/pci/msix.h b/include/hw/pci/msix.h
index 1f27658d352f..815e59bc96f3 100644
--- a/include/hw/pci/msix.h
+++ b/include/hw/pci/msix.h
@@ -11,6 +11,11 @@ int msix_init(PCIDevice *dev, unsigned short nentries,
               unsigned table_offset, MemoryRegion *pba_bar,
               uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos,
               Error **errp);
+int msix_validate_and_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, Error **errp);
 int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
                             uint8_t bar_nr, Error **errp);
 
-- 
2.1.0

[Qemu-devel] [PATCH v10 2/8] 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>
Acked-by: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Hannes Reinecke <hare@suse.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/scsi/megasas.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index e3d59b7c83c7..eab480da354e 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -2359,18 +2359,28 @@ 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);
+        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, NULL)) {
-        /* TODO: check msix_init's error, and should fail on msix=on */
-        s->msix = ON_OFF_AUTO_OFF;
-    }
-
     if (pci_is_express(dev)) {
         pcie_endpoint_cap_init(dev, 0xa0);
     }
-- 
2.1.0

[Qemu-devel] [PATCH v10 3/8] 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>
Acked-by: Marcel Apfelbaum <marcel@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/usb/hcd-xhci.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 28dd2f2c9a97..4551a3758b3e 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -3554,12 +3554,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 {
@@ -3587,6 +3589,25 @@ 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);
+        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,
@@ -3619,14 +3640,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 */
-        msix_init(dev, xhci->numintrs,
-                  &xhci->mem, 0, OFF_MSIX_TABLE,
-                  &xhci->mem, 0, OFF_MSIX_PBA,
-                  0x90, NULL);
-    }
 }
 
 static void usb_xhci_exit(PCIDevice *dev)
-- 
2.1.0

[Qemu-devel] [PATCH v10 4/8] 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>
Acked-by: Marcel Apfelbaum <marcel@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 eab480da354e..ca98ae7cc329 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -2350,11 +2350,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 v10 5/8] 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>
Acked-by: Marcel Apfelbaum <marcel@redhat.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 e13a798b3b00..81ab131e4a47 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -2556,21 +2556,11 @@ static int 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 v10 6/8] 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>
Acked-by: Marcel Apfelbaum <marcel@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/net/vmxnet3.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 81ab131e4a47..a3ed77f04b7c 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,
@@ -2194,18 +2193,19 @@ vmxnet3_init_msix(VMXNET3State *s)
                         VMXNET3_MSIX_OFFSET(s), NULL);
 
     if (0 > res) {
-        VMW_WRPRN("Failed to initialize MSI-X, error %d", res);
-        s->msix_used = false;
+        VMW_WRPRN("Failed to initialize MSI-X, board's MSI support is broken");
+        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
@@ -2213,7 +2213,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 v10 7/8] 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 a87b2278a373..af3efbe525ce 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

[Qemu-devel] [PATCH v10 8/8] 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 ca98ae7cc329..49f38002448e 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;
@@ -2306,9 +2301,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);
 }
 
@@ -2358,7 +2351,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);
         if (ret && s->msix == ON_OFF_AUTO_ON) {
@@ -2375,6 +2368,10 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
         error_free(err);
     }
 
+    if (s->msix != ON_OFF_AUTO_OFF) {
+        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,
@@ -2390,10 +2387,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

Re: [Qemu-devel] [PATCH v10 0/8] the reset of msix_init series

[Cao jin]

Michael,
Is this series ok for 2.9?

-- 
Sincerely,
Cao jin

On 02/25/2017 04:26 PM, Cao jin wrote:
> v10 changelog:
> 1. drop the unliked patch, introduce a new patch 1 according to mst's comments.
> 2. base on the new patch, remove the following statements
> 
>         /* Any error other than -ENOTSUP(board's MSI support is broken)
>          * is a programming error */
>         assert(!ret || ret == -ENOTSUP);
> 
>    for the affected device: megasas, hcd-xhci. This is trivial changes,
>    so I left the R-bs where it was.
> 
> Test:
> 1. Detailed test via command line as v9
> 2. make check hangs at: GTESTER check-qtest-x86_64. After ctrl-C, it says:
> 
>     make: *** [check-qtest-x86_64] Interrupt
>     qemu-system-x86_64: Failed to read msg header. Read -1 instead of 12. Original request 11.
>     qemu-system-x86_64: vhost VQ 0 ring restore failed: -1: Input/output error (5)
>     qemu-system-x86_64: Failed to set msg fds.
>     qemu-system-x86_64: vhost VQ 0 ring restore failed: -1: Invalid argument (22)
> 
>     qemu-system-x86_64: Failed to set msg fds.
>     qemu-system-x86_64: vhost VQ 1 ring restore failed: -1: Invalid argument (22)
> 
>    Is it a regresstion or I missed something?
> 
> CC: Jason Wang <jasowang@redhat.com>
> CC: Gerd Hoffmann <kraxel@redhat.com>
> CC: Dmitry Fleytman <dmitry@daynix.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 (8):
>   msix: Rename and create a wrapper
>   megasas: change behaviour of msix switch
>   hcd-xhci: change behaviour of msix switch
>   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
>   megasas: remove unnecessary megasas_use_msix()
> 
>  hw/net/vmxnet3.c      | 40 +++++++++++++++-------------------------
>  hw/pci/msi.c          |  9 ++++++---
>  hw/pci/msix.c         | 30 +++++++++++++++++++++---------
>  hw/scsi/megasas.c     | 48 +++++++++++++++++++++++++-----------------------
>  hw/usb/hcd-xhci.c     | 29 +++++++++++++++++++++--------
>  hw/vfio/pci.c         | 12 ++++++------
>  include/hw/pci/msix.h |  5 +++++
>  7 files changed, 99 insertions(+), 74 deletions(-)
> 

Re: [Qemu-devel] [PATCH v10 0/8] the reset of msix_init series

[Michael S. Tsirkin]

On Mon, Mar 06, 2017 at 04:10:42PM +0800, Cao jin wrote:
> Michael,
> Is this series ok for 2.9?
> 
> -- 
> Sincerely,
> Cao jin

I'll consider it for 2.10.  Sorry it's taking a long time, the benefit
here is rather small so I don't see a reason to hurry.

> On 02/25/2017 04:26 PM, Cao jin wrote:
> > v10 changelog:
> > 1. drop the unliked patch, introduce a new patch 1 according to mst's comments.
> > 2. base on the new patch, remove the following statements
> > 
> >         /* Any error other than -ENOTSUP(board's MSI support is broken)
> >          * is a programming error */
> >         assert(!ret || ret == -ENOTSUP);
> > 
> >    for the affected device: megasas, hcd-xhci. This is trivial changes,
> >    so I left the R-bs where it was.
> > 
> > Test:
> > 1. Detailed test via command line as v9
> > 2. make check hangs at: GTESTER check-qtest-x86_64. After ctrl-C, it says:
> > 
> >     make: *** [check-qtest-x86_64] Interrupt
> >     qemu-system-x86_64: Failed to read msg header. Read -1 instead of 12. Original request 11.
> >     qemu-system-x86_64: vhost VQ 0 ring restore failed: -1: Input/output error (5)
> >     qemu-system-x86_64: Failed to set msg fds.
> >     qemu-system-x86_64: vhost VQ 0 ring restore failed: -1: Invalid argument (22)
> > 
> >     qemu-system-x86_64: Failed to set msg fds.
> >     qemu-system-x86_64: vhost VQ 1 ring restore failed: -1: Invalid argument (22)
> > 
> >    Is it a regresstion or I missed something?
> > 
> > CC: Jason Wang <jasowang@redhat.com>
> > CC: Gerd Hoffmann <kraxel@redhat.com>
> > CC: Dmitry Fleytman <dmitry@daynix.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 (8):
> >   msix: Rename and create a wrapper
> >   megasas: change behaviour of msix switch
> >   hcd-xhci: change behaviour of msix switch
> >   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
> >   megasas: remove unnecessary megasas_use_msix()
> > 
> >  hw/net/vmxnet3.c      | 40 +++++++++++++++-------------------------
> >  hw/pci/msi.c          |  9 ++++++---
> >  hw/pci/msix.c         | 30 +++++++++++++++++++++---------
> >  hw/scsi/megasas.c     | 48 +++++++++++++++++++++++++-----------------------
> >  hw/usb/hcd-xhci.c     | 29 +++++++++++++++++++++--------
> >  hw/vfio/pci.c         | 12 ++++++------
> >  include/hw/pci/msix.h |  5 +++++
> >  7 files changed, 99 insertions(+), 74 deletions(-)
> > 
> 
> 
> 

Re: [Qemu-devel] [PATCH v10 1/8] msix: Rename and create a wrapper

[Markus Armbruster]

Uh, two weeks since your posted this already.  I apologize for taking so
long to review.

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

> Rename msix_init to msix_validate_and_init, and use it from vfio which
> might get a reasonable -EINVAL.  New a wrapper msix_init which assert the
> programming error for debug purpose and use it from other devices.
>
> CC: Alex Williamson <alex.williamson@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>
> ---
>  hw/pci/msix.c         | 30 +++++++++++++++++++++---------
>  hw/vfio/pci.c         | 12 ++++++------
>  include/hw/pci/msix.h |  5 +++++
>  3 files changed, 32 insertions(+), 15 deletions(-)
>
> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> index bb54e8b0ac37..2b7541ab2c8d 100644
> --- a/hw/pci/msix.c
> +++ b/hw/pci/msix.c
> @@ -239,6 +239,19 @@ static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
>      }
>  }
>  
> +/* Just a wrapper to check the return value */
> +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,
> +              Error **errp)
> +{
> +    int ret = msix_validate_and_init(dev, nentries, table_bar, table_bar_nr,
> +            table_offset, pba_bar, pba_bar_nr, pba_offset, cap_pos, errp);
> +
> +    assert(ret != -EINVAL);
> +    return ret;
> +}
>  /*
>   * Make PCI device @dev MSI-X capable
>   * @nentries is the max number of MSI-X vectors that the device support.
> @@ -259,11 +272,11 @@ static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
>   * 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,
> -              Error **errp)
> +int msix_validate_and_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, Error **errp)
>  {
>      int cap;
>      unsigned table_size, pba_size;
> @@ -361,10 +374,9 @@ int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
>      memory_region_init(&dev->msix_exclusive_bar, OBJECT(dev), name, bar_size);
>      g_free(name);
>  
> -    ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr,
> -                    0, &dev->msix_exclusive_bar,
> -                    bar_nr, bar_pba_offset,
> -                    0, errp);
> +    ret = msix_validate_and_init(dev, nentries, &dev->msix_exclusive_bar,
> +                                 bar_nr, 0, &dev->msix_exclusive_bar,
> +                                 bar_nr, bar_pba_offset, 0, errp);
>      if (ret) {
>          return ret;
>      }

This change assumes that for the callers of msix_exclusive_bar(),
-EINVAL (capability overlap) is not a programming error.  Is that true?

> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 332f41d6627f..06828b537a75 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -1436,12 +1436,12 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
>  
>      vdev->msix->pending = g_malloc0(BITS_TO_LONGS(vdev->msix->entries) *
>                                      sizeof(unsigned long));
> -    ret = msix_init(&vdev->pdev, vdev->msix->entries,
> -                    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,
> -                    &err);
> +    ret = msix_validate_and_init(&vdev->pdev, vdev->msix->entries,
> +                             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,
> +                             &err);
>      if (ret < 0) {
>          if (ret == -ENOTSUP) {
>              error_report_err(err);
> diff --git a/include/hw/pci/msix.h b/include/hw/pci/msix.h
> index 1f27658d352f..815e59bc96f3 100644
> --- a/include/hw/pci/msix.h
> +++ b/include/hw/pci/msix.h
> @@ -11,6 +11,11 @@ int msix_init(PCIDevice *dev, unsigned short nentries,
>                unsigned table_offset, MemoryRegion *pba_bar,
>                uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos,
>                Error **errp);
> +int msix_validate_and_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, Error **errp);
>  int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
>                              uint8_t bar_nr, Error **errp);

Re: [Qemu-devel] [PATCH v10 1/8] msix: Rename and create a wrapper

[Cao jin]

On 03/07/2017 03:44 PM, Markus Armbruster wrote:
> Uh, two weeks since your posted this already.  I apologize for taking so
> long to review.
> 
> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
> 
>> Rename msix_init to msix_validate_and_init, and use it from vfio which
>> might get a reasonable -EINVAL.  New a wrapper msix_init which assert the
>> programming error for debug purpose and use it from other devices.
>>
>> CC: Alex Williamson <alex.williamson@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>
>> ---
>>  hw/pci/msix.c         | 30 +++++++++++++++++++++---------
>>  hw/vfio/pci.c         | 12 ++++++------
>>  include/hw/pci/msix.h |  5 +++++
>>  3 files changed, 32 insertions(+), 15 deletions(-)
>>
>> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
>> index bb54e8b0ac37..2b7541ab2c8d 100644
>> --- a/hw/pci/msix.c
>> +++ b/hw/pci/msix.c
>> @@ -239,6 +239,19 @@ static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
>>      }
>>  }
>>  
>> +/* Just a wrapper to check the return value */
>> +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,
>> +              Error **errp)
>> +{
>> +    int ret = msix_validate_and_init(dev, nentries, table_bar, table_bar_nr,
>> +            table_offset, pba_bar, pba_bar_nr, pba_offset, cap_pos, errp);
>> +
>> +    assert(ret != -EINVAL);
>> +    return ret;
>> +}
>>  /*
>>   * Make PCI device @dev MSI-X capable
>>   * @nentries is the max number of MSI-X vectors that the device support.
>> @@ -259,11 +272,11 @@ static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
>>   * 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,
>> -              Error **errp)
>> +int msix_validate_and_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, Error **errp)
>>  {
>>      int cap;
>>      unsigned table_size, pba_size;
>> @@ -361,10 +374,9 @@ int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
>>      memory_region_init(&dev->msix_exclusive_bar, OBJECT(dev), name, bar_size);
>>      g_free(name);
>>  
>> -    ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr,
>> -                    0, &dev->msix_exclusive_bar,
>> -                    bar_nr, bar_pba_offset,
>> -                    0, errp);
>> +    ret = msix_validate_and_init(dev, nentries, &dev->msix_exclusive_bar,
>> +                                 bar_nr, 0, &dev->msix_exclusive_bar,
>> +                                 bar_nr, bar_pba_offset, 0, errp);
>>      if (ret) {
>>          return ret;
>>      }
> 
> This change assumes that for the callers of msix_exclusive_bar(),
> -EINVAL (capability overlap) is not a programming error.  Is that true?
> 

Oh...it looks as you said. Will revert this part and send a new one
in-reply-to this one.

-- 
Sincerely,
Cao jin

[Qemu-devel] [PATCH v10] msix: rename and create a wrapper

[Cao jin]

Rename msix_init to msix_validate_and_init which doesn't assert;
New a wrapper msix_init to assert programming error.

CC: Alex Williamson <alex.williamson@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>
---
revert the modification in msix_init_exclusive_bar()

 hw/pci/msix.c         | 23 ++++++++++++++++++-----
 hw/vfio/pci.c         | 12 ++++++------
 include/hw/pci/msix.h |  5 +++++
 3 files changed, 29 insertions(+), 11 deletions(-)

diff --git a/hw/pci/msix.c b/hw/pci/msix.c
index bb54e8b0ac37..02697de32dfc 100644
--- a/hw/pci/msix.c
+++ b/hw/pci/msix.c
@@ -239,6 +239,19 @@ static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
     }
 }
 
+/* Just a wrapper to check the return value */
+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,
+              Error **errp)
+{
+    int ret = msix_validate_and_init(dev, nentries, table_bar, table_bar_nr,
+            table_offset, pba_bar, pba_bar_nr, pba_offset, cap_pos, errp);
+
+    assert(ret != -EINVAL);
+    return ret;
+}
 /*
  * Make PCI device @dev MSI-X capable
  * @nentries is the max number of MSI-X vectors that the device support.
@@ -259,11 +272,11 @@ static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
  * 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,
-              Error **errp)
+int msix_validate_and_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, Error **errp)
 {
     int cap;
     unsigned table_size, pba_size;
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 332f41d6627f..06828b537a75 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1436,12 +1436,12 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
 
     vdev->msix->pending = g_malloc0(BITS_TO_LONGS(vdev->msix->entries) *
                                     sizeof(unsigned long));
-    ret = msix_init(&vdev->pdev, vdev->msix->entries,
-                    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,
-                    &err);
+    ret = msix_validate_and_init(&vdev->pdev, vdev->msix->entries,
+                             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,
+                             &err);
     if (ret < 0) {
         if (ret == -ENOTSUP) {
             error_report_err(err);
diff --git a/include/hw/pci/msix.h b/include/hw/pci/msix.h
index 1f27658d352f..815e59bc96f3 100644
--- a/include/hw/pci/msix.h
+++ b/include/hw/pci/msix.h
@@ -11,6 +11,11 @@ int msix_init(PCIDevice *dev, unsigned short nentries,
               unsigned table_offset, MemoryRegion *pba_bar,
               uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos,
               Error **errp);
+int msix_validate_and_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, Error **errp);
 int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
                             uint8_t bar_nr, Error **errp);
 
-- 
2.1.0

Re: [Qemu-devel] [PATCH v10] msix: rename and create a wrapper

[Markus Armbruster]

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

> Rename msix_init to msix_validate_and_init which doesn't assert;
> New a wrapper msix_init to assert programming error.
>
> CC: Alex Williamson <alex.williamson@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>

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

Re: [Qemu-devel] [PATCH v10 0/8] the reset of msix_init series

[Cao jin]

Hi Michael,

  I rebased this patchset against upstream, find no conflicts.
  Hope it is time to merge it.

On 03/06/2017 04:10 PM, Cao jin wrote:
> Michael,
> Is this series ok for 2.9?
> 

-- 
Sincerely,
Cao jin