[Qemu-devel] [PATCH v9 00/11] Convert msix_init() to error

[Qemu-devel] [PATCH v9 00/11] Convert msix_init() to error

[Cao jin]

v9 changelog:
1. split previous patch 3 into two separate patches(3 & 4), per mst's review.

test:
1. make check ok.
2. detailed test on megasas/megasas-gen2, hcd-xhci, vmxnet3.
   megasas/megasas-gen2(M q35...bus=pcie.0): install a distro
   ./qemu-system-x86_64 --enable-kvm -m 1024
   -device megasas,id=scsi0,bus=pci.0
   -drive file=/xx/scsi-disk.img,if=none,id=drive-scsi0
   -device scsi-disk,bus=scsi0.0,channel=0,scsi-id=4,lun=0,drive=drive-scsi0,id=scsi0-4
   -cdrom /xx/Fedora-Server-DVD-x86_64-23.iso -boot once=d --monitor stdio

    hcd-xhci: fdisk, mkfs.ext4, write file to usbstick.img
    ./qemu-system-x86_64 -M q35 --enable-kvm -m 1024
    -drive if=none,id=usbstick,file=/xx/usbstick.img
    -device nec-usb-xhci,id=usb,p2=8,p3=8,bus=pcie.0
    -device usb-storage,bus=usb.0,drive=usbstick
    /xx/FedoraServer23-X86_64.img --monitor stdio

    vmxnet3: ping another destination belongs to host's network is ok; then
             migrate to another qemu instance on the same host is ok. After
             migration, ping can't work as before, out of the patchset's scope,
             it is the same issue as upstream.
    ./qemu-system-x86_64 -M q35 --enable-kvm -m 1024
    -netdev tap,id=mynet0 -device vmxnet3,netdev=mynet0
    /xx/FedoraServer23-X86_64.img --monitor stdio

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

Cao jin (11):
  msix: Follow CODING_STYLE
  hcd-xhci: check & correct param before using it
  pci: Convert msix_init() to Error and fix callers
  msix: check msix_init's return value
  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/block/nvme.c        |  2 +-
 hw/misc/ivshmem.c      |  8 +++---
 hw/net/e1000e.c        |  6 ++++-
 hw/net/rocker/rocker.c |  9 ++++++-
 hw/net/vmxnet3.c       | 46 +++++++++++++++------------------
 hw/pci/msi.c           |  9 ++++---
 hw/pci/msix.c          | 44 +++++++++++++++++++++++++++-----
 hw/scsi/megasas.c      | 49 ++++++++++++++++++++---------------
 hw/usb/hcd-xhci.c      | 69 ++++++++++++++++++++++++++++++--------------------
 hw/vfio/pci.c          |  8 ++++--
 hw/virtio/virtio-pci.c | 13 +++++-----
 include/hw/pci/msix.h  |  5 ++--
 12 files changed, 167 insertions(+), 101 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH v9 01/11] 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>
Acked-by: Marcel Apfelbaum <marcel@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 0ec1cb14fc60..0cee631ecc55 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 v9 02/11] 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: 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 | 37 ++++++++++++++++++-------------------
 1 file changed, 18 insertions(+), 19 deletions(-)

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

[Qemu-devel] [PATCH v9 03/11] pci: Convert msix_init() to Error and fix callers

[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. In order to make the API change as small as possible,
leave the return value check to later patch.

For some devices(like e1000e, vmxnet3, nvme) 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>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/block/nvme.c        |  2 +-
 hw/misc/ivshmem.c      |  8 ++++----
 hw/net/e1000e.c        |  2 +-
 hw/net/rocker/rocker.c |  4 +++-
 hw/net/vmxnet3.c       |  2 +-
 hw/pci/msix.c          | 36 +++++++++++++++++++++++++++++++-----
 hw/scsi/megasas.c      |  4 +++-
 hw/usb/hcd-xhci.c      |  4 ++--
 hw/vfio/pci.c          |  8 ++++++--
 hw/virtio/virtio-pci.c |  4 ++--
 include/hw/pci/msix.h  |  5 +++--
 11 files changed, 57 insertions(+), 22 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index d479fd22f573..ae91a18f1724 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -872,7 +872,7 @@ 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);
+    msix_init_exclusive_bar(&n->parent_obj, n->num_queues, 4, NULL);
 
     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 abeaf3da0800..70e71a597b9c 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 4994e1ca0062..ed04adce061c 100644
--- a/hw/net/e1000e.c
+++ b/hw/net/e1000e.c
@@ -292,7 +292,7 @@ e1000e_init_msix(E1000EState *s)
                         E1000E_MSIX_IDX, E1000E_MSIX_TABLE,
                         &s->msix,
                         E1000E_MSIX_IDX, E1000E_MSIX_PBA,
-                        0xA0);
+                        0xA0, NULL);
 
     if (res < 0) {
         trace_e1000e_msix_init_fail(res);
diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
index e9d215aa4df1..6e70fddee36b 100644
--- a/hw/net/rocker/rocker.c
+++ b/hw/net/rocker/rocker.c
@@ -1256,14 +1256,16 @@ 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);
     if (err) {
+        error_report_err(local_err);
         return err;
     }
 
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 92f6af9620f1..7b2971fe5902 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -2191,7 +2191,7 @@ 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);
 
     if (0 > res) {
         VMW_WRPRN("Failed to initialize MSI-X, error %d", res);
diff --git a/hw/pci/msix.c b/hw/pci/msix.c
index 0cee631ecc55..bd8cdd41d880 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,31 @@ 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 +271,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 +289,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 +332,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 +364,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 67fc1e7893f8..095dba8b36de 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -2366,9 +2366,11 @@ 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, 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);
     }
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 0ace273da472..fe64dd8525c7 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -3703,11 +3703,11 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
     }
 
     if (xhci->msix != ON_OFF_AUTO_OFF) {
-        /* TODO check for errors */
+        /* 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);
+                  0x90, NULL);
     }
 }
 
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index d7dbe0e3e04e..5df190510a05 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,12 +1440,15 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
                     vdev->bars[vdev->msix->table_bar].region.mem,
                     vdev->msix->table_bar, vdev->msix->table_offset,
                     vdev->bars[vdev->msix->pba_bar].region.mem,
-                    vdev->msix->pba_bar, vdev->msix->pba_offset, pos);
+                    vdev->msix->pba_bar, vdev->msix->pba_offset, pos,
+                    &err);
     if (ret < 0) {
         if (ret == -ENOTSUP) {
+            error_report_err(err);
             return 0;
         }
-        error_setg(errp, "msix_init failed");
+
+        error_propagate(errp, err);
         return ret;
     }
 
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 21c2b9dbfc97..4c2c4941d245 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1670,9 +1670,9 @@ 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, NULL);
         if (err) {
-            /* Notice when a system that supports MSIx can't initialize it.  */
+            /* 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);
diff --git a/include/hw/pci/msix.h b/include/hw/pci/msix.h
index 048a29dd2f65..1f27658d352f 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 v9 04/11] msix: check msix_init's return value

[Cao jin]

Doesn't do it for megasas & hcd-xhci, later patches will fix them.

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/net/e1000e.c        |  4 ++++
 hw/net/rocker/rocker.c |  5 +++++
 hw/net/vmxnet3.c       |  6 +++++-
 hw/virtio/virtio-pci.c | 13 +++++++------
 4 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
index ed04adce061c..74cbbef30366 100644
--- a/hw/net/e1000e.c
+++ b/hw/net/e1000e.c
@@ -294,6 +294,10 @@ e1000e_init_msix(E1000EState *s)
                         E1000E_MSIX_IDX, E1000E_MSIX_PBA,
                         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);
     } else {
diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
index 6e70fddee36b..e394fd61fe64 100644
--- a/hw/net/rocker/rocker.c
+++ b/hw/net/rocker/rocker.c
@@ -1264,6 +1264,11 @@ static int rocker_msix_init(Rocker *r)
                     &r->msix_bar,
                     ROCKER_PCI_MSIX_BAR_IDX, ROCKER_PCI_MSIX_PBA_OFFSET,
                     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 7b2971fe5902..a433cc017cb1 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -2193,8 +2193,12 @@ vmxnet3_init_msix(VMXNET3State *s)
                         VMXNET3_MSIX_BAR_IDX, VMXNET3_OFF_MSIX_PBA(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/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 4c2c4941d245..2417c78c477e 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1670,13 +1670,14 @@ 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, NULL);
+                                          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;
         }
     }
-- 
2.1.0

[Qemu-devel] [PATCH v9 05/11] 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>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/scsi/megasas.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 095dba8b36de..14d6e0c6d565 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -2359,18 +2359,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, 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 v9 06/11] 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 | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index fe64dd8525c7..aaca57cb5f1f 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -3636,12 +3636,14 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
     if (xhci->numintrs < 1) {
         xhci->numintrs = 1;
     }
+
     if (xhci->numslots > MAXSLOTS) {
         xhci->numslots = MAXSLOTS;
     }
     if (xhci->numslots < 1) {
         xhci->numslots = 1;
     }
+
     if (xhci_get_flag(xhci, XHCI_FLAG_ENABLE_STREAMS)) {
         xhci->max_pstreams_mask = 7; /* == 256 primary streams */
     } else {
@@ -3669,6 +3671,28 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
     xhci->mfwrap_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, xhci_mfwrap_timer, xhci);
 
     memory_region_init(&xhci->mem, OBJECT(xhci), "xhci", LEN_REGS);
+    if (xhci->msix != ON_OFF_AUTO_OFF) {
+        ret = msix_init(dev, xhci->numintrs,
+                        &xhci->mem, 0, OFF_MSIX_TABLE,
+                        &xhci->mem, 0, OFF_MSIX_PBA,
+                        0x90, &err);
+        /* Any error other than -ENOTSUP(board's MSI support is broken)
+         * is a programming error */
+        assert(!ret || ret == -ENOTSUP);
+        if (ret && xhci->msix == ON_OFF_AUTO_ON) {
+            /* Can't satisfy user's explicit msix=on request, fail */
+            error_append_hint(&err, "You have to use msix=auto (default) or "
+                    "msix=off with this machine type.\n");
+            /* No instance_finalize method, need to free the resource here */
+            object_unref(OBJECT(&xhci->mem));
+            error_propagate(errp, err);
+            return;
+        }
+        assert(!err || xhci->msix == ON_OFF_AUTO_AUTO);
+        /* With msix=auto, we fall back to MSI off silently */
+        error_free(err);
+    }
+
     memory_region_init_io(&xhci->mem_cap, OBJECT(xhci), &xhci_cap_ops, xhci,
                           "capabilities", LEN_CAP);
     memory_region_init_io(&xhci->mem_oper, OBJECT(xhci), &xhci_oper_ops, xhci,
@@ -3701,14 +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 */
-        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 v9 07/11] 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>
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 14d6e0c6d565..c208d520c4df 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 v9 08/11] 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 a433cc017cb1..45e125e92c8a 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 v9 09/11] 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 | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 45e125e92c8a..af39965c8cc2 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 v9 10/11] 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 v9 11/11] 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 c208d520c4df..73eab7844ee3 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;
@@ -2305,9 +2300,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);
         /* Any error other than -ENOTSUP(board's MSI support is broken)
@@ -2378,6 +2371,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,
@@ -2393,10 +2390,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 v9 04/11] msix: check msix_init's return value

[Cao jin]

forget to cc maintainers in this new patch

On 01/17/2017 02:18 PM, Cao jin wrote:
> Doesn't do it for megasas & hcd-xhci, later patches will fix them.
> 
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/net/e1000e.c        |  4 ++++
>  hw/net/rocker/rocker.c |  5 +++++
>  hw/net/vmxnet3.c       |  6 +++++-
>  hw/virtio/virtio-pci.c | 13 +++++++------
>  4 files changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
> index ed04adce061c..74cbbef30366 100644
> --- a/hw/net/e1000e.c
> +++ b/hw/net/e1000e.c
> @@ -294,6 +294,10 @@ e1000e_init_msix(E1000EState *s)
>                          E1000E_MSIX_IDX, E1000E_MSIX_PBA,
>                          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);
>      } else {
> diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
> index 6e70fddee36b..e394fd61fe64 100644
> --- a/hw/net/rocker/rocker.c
> +++ b/hw/net/rocker/rocker.c
> @@ -1264,6 +1264,11 @@ static int rocker_msix_init(Rocker *r)
>                      &r->msix_bar,
>                      ROCKER_PCI_MSIX_BAR_IDX, ROCKER_PCI_MSIX_PBA_OFFSET,
>                      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 7b2971fe5902..a433cc017cb1 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -2193,8 +2193,12 @@ vmxnet3_init_msix(VMXNET3State *s)
>                          VMXNET3_MSIX_BAR_IDX, VMXNET3_OFF_MSIX_PBA(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/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 4c2c4941d245..2417c78c477e 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1670,13 +1670,14 @@ 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, NULL);
> +                                          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;
>          }
>      }
> 

-- 
Sincerely,
Cao jin

Re: [Qemu-devel] [PATCH v9 04/11] msix: check msix_init's return value

[Michael S. Tsirkin]

On Tue, Jan 17, 2017 at 02:50:38PM +0800, Cao jin wrote:
> forget to cc maintainers in this new patch
> 
> On 01/17/2017 02:18 PM, Cao jin wrote:
> > Doesn't do it for megasas & hcd-xhci, later patches will fix them.
> > 
> > Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>

I don't like this one, frankly. That's a bunch of code duplication.
I suspect vfio is the only one who might reasonably get EINVAL here.
So how about e.g. msix_validate_and_init that doesn't assert and use that
from vfio, then switch msix_init to assert instead?

> > ---
> >  hw/net/e1000e.c        |  4 ++++
> >  hw/net/rocker/rocker.c |  5 +++++
> >  hw/net/vmxnet3.c       |  6 +++++-
> >  hw/virtio/virtio-pci.c | 13 +++++++------
> >  4 files changed, 21 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
> > index ed04adce061c..74cbbef30366 100644
> > --- a/hw/net/e1000e.c
> > +++ b/hw/net/e1000e.c
> > @@ -294,6 +294,10 @@ e1000e_init_msix(E1000EState *s)
> >                          E1000E_MSIX_IDX, E1000E_MSIX_PBA,
> >                          0xA0, NULL);
> >  
> > +    /* Any error other than -ENOTSUP(board's MSI support is broken)
> > +     * is a programming error. Fall back to INTx silently on -ENOTSUP */

/* don't format
 * comments like this pls. */

/*
 * do it
 * like this pls
 */

> > +    assert(!res || res == -ENOTSUP);
> > +
> >      if (res < 0) {
> >          trace_e1000e_msix_init_fail(res);
> >      } else {
> > diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
> > index 6e70fddee36b..e394fd61fe64 100644
> > --- a/hw/net/rocker/rocker.c
> > +++ b/hw/net/rocker/rocker.c
> > @@ -1264,6 +1264,11 @@ static int rocker_msix_init(Rocker *r)
> >                      &r->msix_bar,
> >                      ROCKER_PCI_MSIX_BAR_IDX, ROCKER_PCI_MSIX_PBA_OFFSET,
> >                      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 7b2971fe5902..a433cc017cb1 100644
> > --- a/hw/net/vmxnet3.c
> > +++ b/hw/net/vmxnet3.c
> > @@ -2193,8 +2193,12 @@ vmxnet3_init_msix(VMXNET3State *s)
> >                          VMXNET3_MSIX_BAR_IDX, VMXNET3_OFF_MSIX_PBA(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/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index 4c2c4941d245..2417c78c477e 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -1670,13 +1670,14 @@ 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, NULL);
> > +                                          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;
> >          }
> >      }
> > 
> 
> -- 
> Sincerely,
> Cao jin
> 

Re: [Qemu-devel] [PATCH v9 04/11] msix: check msix_init's return value

[Cao jin]

On 01/18/2017 12:01 AM, Michael S. Tsirkin wrote:
> On Tue, Jan 17, 2017 at 02:50:38PM +0800, Cao jin wrote:
>> forget to cc maintainers in this new patch
>>
>> On 01/17/2017 02:18 PM, Cao jin wrote:
>>> Doesn't do it for megasas & hcd-xhci, later patches will fix them.
>>>
>>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> 
> I don't like this one, frankly. That's a bunch of code duplication.

Yes, code duplication, seems inevitable if move the asserts into a
separate patch.

> I suspect vfio is the only one who might reasonably get EINVAL here.
> So how about e.g. msix_validate_and_init that doesn't assert and use that
> from vfio, then switch msix_init to assert instead?
> 

Not sure if I get your idea. Do you mean: do param check via assert in
msix_init(), so that no need check its returned error outside, and
introduce new api msix_validate_and_init(same content as msix_init,
except param check) dedicated to vfio?

If I understand you right, the way we do param check for msi_init[*] &
msix_init will be inconsistent.

[*] patch: msi_init: convert assert to return -errno

-- 
Sincerely,
Cao jin

Re: [Qemu-devel] [PATCH v9 07/11] megasas: undo the overwrites of msi user configuration

[Hannes Reinecke]

On 01/17/2017 07:18 AM, Cao jin wrote:
> 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>
> 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 14d6e0c6d565..c208d520c4df 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,
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH v9 03/11] pci: Convert msix_init() to Error and fix callers

[Hannes Reinecke]

On 01/17/2017 07:18 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. In order to make the API change as small as possible,
> leave the return value check to later patch.
> 
> For some devices(like e1000e, vmxnet3, nvme) 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>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/block/nvme.c        |  2 +-
>  hw/misc/ivshmem.c      |  8 ++++----
>  hw/net/e1000e.c        |  2 +-
>  hw/net/rocker/rocker.c |  4 +++-
>  hw/net/vmxnet3.c       |  2 +-
>  hw/pci/msix.c          | 36 +++++++++++++++++++++++++++++++-----
>  hw/scsi/megasas.c      |  4 +++-
>  hw/usb/hcd-xhci.c      |  4 ++--
>  hw/vfio/pci.c          |  8 ++++++--
>  hw/virtio/virtio-pci.c |  4 ++--
>  include/hw/pci/msix.h  |  5 +++--
>  11 files changed, 57 insertions(+), 22 deletions(-)
> 
For megasas: Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH v9 05/11] megasas: change behaviour of msix switch

[Hannes Reinecke]

On 01/17/2017 07:18 AM, Cao jin wrote:
> 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>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/scsi/megasas.c | 27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH v9 04/11] msix: check msix_init's return value

[Michael S. Tsirkin]

On Wed, Jan 18, 2017 at 02:29:19PM +0800, Cao jin wrote:
> 
> 
> On 01/18/2017 12:01 AM, Michael S. Tsirkin wrote:
> > On Tue, Jan 17, 2017 at 02:50:38PM +0800, Cao jin wrote:
> >> forget to cc maintainers in this new patch
> >>
> >> On 01/17/2017 02:18 PM, Cao jin wrote:
> >>> Doesn't do it for megasas & hcd-xhci, later patches will fix them.
> >>>
> >>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> > 
> > I don't like this one, frankly. That's a bunch of code duplication.
> 
> Yes, code duplication, seems inevitable if move the asserts into a
> separate patch.
> 
> > I suspect vfio is the only one who might reasonably get EINVAL here.
> > So how about e.g. msix_validate_and_init that doesn't assert and use that
> > from vfio, then switch msix_init to assert instead?
> > 
> 
> Not sure if I get your idea. Do you mean: do param check via assert in
> msix_init(), so that no need check its returned error outside, and
> introduce new api msix_validate_and_init(same content as msix_init,
> except param check) dedicated to vfio?

Something like this.

> If I understand you right, the way we do param check for msi_init[*] &
> msix_init will be inconsistent.

Right, we should consolidate these for msi too.


> [*] patch: msi_init: convert assert to return -errno
> 
> -- 
> Sincerely,
> Cao jin
> 

Re: [Qemu-devel] [PATCH v9 04/11] msix: check msix_init's return value

[Cao jin]

On 01/18/2017 11:21 PM, Michael S. Tsirkin wrote:
> On Wed, Jan 18, 2017 at 02:29:19PM +0800, Cao jin wrote:
>>
>>
>> On 01/18/2017 12:01 AM, Michael S. Tsirkin wrote:
>>> On Tue, Jan 17, 2017 at 02:50:38PM +0800, Cao jin wrote:
>>>> forget to cc maintainers in this new patch
>>>>
>>>> On 01/17/2017 02:18 PM, Cao jin wrote:
>>>>> Doesn't do it for megasas & hcd-xhci, later patches will fix them.
>>>>>
>>>>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>>>
>>> I don't like this one, frankly. That's a bunch of code duplication.
>>
>> Yes, code duplication, seems inevitable if move the asserts into a
>> separate patch.
>>
>>> I suspect vfio is the only one who might reasonably get EINVAL here.
>>> So how about e.g. msix_validate_and_init that doesn't assert and use that
>>> from vfio, then switch msix_init to assert instead?
>>>
>>
>> Not sure if I get your idea. Do you mean: do param check via assert in
>> msix_init(), so that no need check its returned error outside, and
>> introduce new api msix_validate_and_init(same content as msix_init,
>> except param check) dedicated to vfio?
> 
> Something like this.
> 
>> If I understand you right, the way we do param check for msi_init[*] &
>> msix_init will be inconsistent.
> 
> Right, we should consolidate these for msi too.
> 
> 

I got confused: for msi_init, convert assert to return -errno is a
choice from a long discussion:
http://lists.nongnu.org/archive/html/qemu-devel/2016-09/msg08215.html

then now we will revert again? And IIRC, I did use assert in msix_init
to do sanity test, and revert as suggest. And this is the way we have
done for msi_init: assert the return error outside.  And if it need to
be modified as your suggestion, I see lots of place need to be taken
care, does that worth the trouble?

I see there is a simpler way helping us: drop this one from the
patchset, at least there is no regression, just a few devices doesn't
assert the return error while other(megasas, hcd-xhci) does.  What would
you say?
-- 
Sincerely,
Cao jin

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

[Michael S. Tsirkin]

On Tue, Jan 17, 2017 at 02:18:45PM +0800, Cao jin wrote:
> v9 changelog:
> 1. split previous patch 3 into two separate patches(3 & 4), per mst's review.

So I merged 1-3, making progress here.
I really don't like forcing all callers to check and assert on specific
return status, IMO most callers want an assert on any error so that
should be the default, and special cases (probably just vfio) will use a
version that does not assert.


> test:
> 1. make check ok.
> 2. detailed test on megasas/megasas-gen2, hcd-xhci, vmxnet3.
>    megasas/megasas-gen2(M q35...bus=pcie.0): install a distro
>    ./qemu-system-x86_64 --enable-kvm -m 1024
>    -device megasas,id=scsi0,bus=pci.0
>    -drive file=/xx/scsi-disk.img,if=none,id=drive-scsi0
>    -device scsi-disk,bus=scsi0.0,channel=0,scsi-id=4,lun=0,drive=drive-scsi0,id=scsi0-4
>    -cdrom /xx/Fedora-Server-DVD-x86_64-23.iso -boot once=d --monitor stdio
> 
>     hcd-xhci: fdisk, mkfs.ext4, write file to usbstick.img
>     ./qemu-system-x86_64 -M q35 --enable-kvm -m 1024
>     -drive if=none,id=usbstick,file=/xx/usbstick.img
>     -device nec-usb-xhci,id=usb,p2=8,p3=8,bus=pcie.0
>     -device usb-storage,bus=usb.0,drive=usbstick
>     /xx/FedoraServer23-X86_64.img --monitor stdio
> 
>     vmxnet3: ping another destination belongs to host's network is ok; then
>              migrate to another qemu instance on the same host is ok. After
>              migration, ping can't work as before, out of the patchset's scope,
>              it is the same issue as upstream.
>     ./qemu-system-x86_64 -M q35 --enable-kvm -m 1024
>     -netdev tap,id=mynet0 -device vmxnet3,netdev=mynet0
>     /xx/FedoraServer23-X86_64.img --monitor stdio
> 
> CC: Jiri Pirko <jiri@resnulli.us>
> CC: Gerd Hoffmann <kraxel@redhat.com>
> CC: Dmitry Fleytman <dmitry@daynix.com>
> CC: Jason Wang <jasowang@redhat.com>
> CC: Michael S. Tsirkin <mst@redhat.com>
> CC: Hannes Reinecke <hare@suse.de>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Alex Williamson <alex.williamson@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Marcel Apfelbaum <marcel@redhat.com>
> 
> Cao jin (11):
>   msix: Follow CODING_STYLE
>   hcd-xhci: check & correct param before using it
>   pci: Convert msix_init() to Error and fix callers
>   msix: check msix_init's return value
>   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/block/nvme.c        |  2 +-
>  hw/misc/ivshmem.c      |  8 +++---
>  hw/net/e1000e.c        |  6 ++++-
>  hw/net/rocker/rocker.c |  9 ++++++-
>  hw/net/vmxnet3.c       | 46 +++++++++++++++------------------
>  hw/pci/msi.c           |  9 ++++---
>  hw/pci/msix.c          | 44 +++++++++++++++++++++++++++-----
>  hw/scsi/megasas.c      | 49 ++++++++++++++++++++---------------
>  hw/usb/hcd-xhci.c      | 69 ++++++++++++++++++++++++++++++--------------------
>  hw/vfio/pci.c          |  8 ++++--
>  hw/virtio/virtio-pci.c | 13 +++++-----
>  include/hw/pci/msix.h  |  5 ++--
>  12 files changed, 167 insertions(+), 101 deletions(-)
> 
> -- 
> 2.1.0
> 
> 

Re: [Qemu-devel] [PATCH v9 04/11] msix: check msix_init's return value

[Paolo Bonzini]

On 17/01/2017 17:01, Michael S. Tsirkin wrote:
>>> Doesn't do it for megasas & hcd-xhci, later patches will fix them.
>>>
>>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> I don't like this one, frankly. That's a bunch of code duplication.
> I suspect vfio is the only one who might reasonably get EINVAL here.
> So how about e.g. msix_validate_and_init that doesn't assert and use that
> from vfio, then switch msix_init to assert instead?

The names we use normally would be msix_init and msix_init_nofail.
Would still require a change through the whole tree, but it's more
consistent at least.

Paolo

Re: [Qemu-devel] [PATCH v9 04/11] msix: check msix_init's return value

[Michael S. Tsirkin]

On Tue, Jan 24, 2017 at 07:18:14PM +0100, Paolo Bonzini wrote:
> 
> 
> On 17/01/2017 17:01, Michael S. Tsirkin wrote:
> >>> Doesn't do it for megasas & hcd-xhci, later patches will fix them.
> >>>
> >>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> > I don't like this one, frankly. That's a bunch of code duplication.
> > I suspect vfio is the only one who might reasonably get EINVAL here.
> > So how about e.g. msix_validate_and_init that doesn't assert and use that
> > from vfio, then switch msix_init to assert instead?
> 
> The names we use normally would be msix_init and msix_init_nofail.
> Would still require a change through the whole tree, but it's more
> consistent at least.
> 
> Paolo

This area has seen too much noise already but OK I guess.
Also, msix_init_exclusive_bar probably should assert
internally, no need for two versions.

-- 
MST