[Qemu-devel] [PATCH v3 0/8] Convert msix_init() to error

[Qemu-devel] [PATCH v3 0/8] Convert msix_init() to error

[Cao jin]

v3 changelog:
1. withdraw the patch "msix_init: assert programming error",
   set error in the main patch "Convert msix_init to Error..."
2. separate 3 new from main patch as per comments: patch 2,4,5
3. a new patch 8

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 (8):
  msix: Follow CODING_STYLE
  hcd-xhci: check & correct param before using it
  pci: Convert msix_init() to Error and fix callers to check it
  megasas: change behaviour of msix switch
  hcd-xhci: change behaviour of msix switch
  megasas: remove unnecessary megasas_use_msix()
  megasas: undo the overwrites of msi user configuration
  vmxnet3: remove unnecessary internal msix state flag

 hw/block/nvme.c        |  5 +++-
 hw/misc/ivshmem.c      |  8 +++---
 hw/net/e1000e.c        |  6 ++++-
 hw/net/rocker/rocker.c |  7 ++++-
 hw/net/vmxnet3.c       | 46 ++++++++++++++------------------
 hw/pci/msix.c          | 41 ++++++++++++++++++++++++-----
 hw/scsi/megasas.c      | 49 +++++++++++++++++++---------------
 hw/usb/hcd-xhci.c      | 71 ++++++++++++++++++++++++++++++--------------------
 hw/vfio/pci.c          |  7 +++--
 hw/virtio/virtio-pci.c | 11 ++++----
 include/hw/pci/msix.h  |  5 ++--
 11 files changed, 157 insertions(+), 99 deletions(-)

-- 
2.1.0

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

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

[Qemu-devel] [PATCH v3 2/8] hcd-xhci: check & correct param before using it

[Cao jin]

Param checking/correcting code of xchi->numintrs should be placed before
it is used.
Also move some resource-alloc code down, save the strenth to free them
on msi_init's failure.

CC: Gerd Hoffmann <kraxel@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Marcel Apfelbaum <marcel@redhat.com>
CC: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/usb/hcd-xhci.c | 39 +++++++++++++++++++--------------------
 1 file changed, 19 insertions(+), 20 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 188f954..95b1954 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -3594,25 +3594,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;
     }
@@ -3634,7 +3615,22 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
         xhci->max_pstreams_mask = 0;
     }
 
-    xhci->mfwrap_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, xhci_mfwrap_timer, xhci);
+    if (xhci->msi != ON_OFF_AUTO_OFF) {
+        ret = msi_init(dev, 0x70, xhci->numintrs, true, false, &err);
+        /* Any error other than -ENOTSUP(board's MSI support is broken)
+         * is a programming error */
+        assert(!ret || ret == -ENOTSUP);
+        if (ret && xhci->msi == ON_OFF_AUTO_ON) {
+            /* Can't satisfy user's explicit msi=on request, fail */
+            error_append_hint(&err, "You have to use msi=auto (default) or "
+                    "msi=off with this machine type.\n");
+            error_propagate(errp, err);
+            return;
+        }
+        assert(!err || xhci->msi == ON_OFF_AUTO_AUTO);
+        /* With msi=auto, we fall back to MSI off silently */
+        error_free(err);
+    }
 
     memory_region_init(&xhci->mem, OBJECT(xhci), "xhci", LEN_REGS);
     memory_region_init_io(&xhci->mem_cap, OBJECT(xhci), &xhci_cap_ops, xhci,
@@ -3664,6 +3660,9 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
                      PCI_BASE_ADDRESS_SPACE_MEMORY|PCI_BASE_ADDRESS_MEM_TYPE_64,
                      &xhci->mem);
 
+    usb_xhci_init(xhci);
+    xhci->mfwrap_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, xhci_mfwrap_timer, xhci);
+
     if (pci_bus_is_express(dev->bus) ||
         xhci_get_flag(xhci, XHCI_FLAG_FORCE_PCIE_ENDCAP)) {
         ret = pcie_endpoint_cap_init(dev, 0xa0);
-- 
2.1.0

Re: [Qemu-devel] [PATCH v3 2/8] hcd-xhci: check & correct param before using it

[Markus Armbruster]

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

> Param checking/correcting code of xchi->numintrs should be placed before
> it is used.
> Also move some resource-alloc code down, save the strenth to free them
> on msi_init's failure.
>
> CC: Gerd Hoffmann <kraxel@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Marcel Apfelbaum <marcel@redhat.com>
> CC: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/usb/hcd-xhci.c | 39 +++++++++++++++++++--------------------
>  1 file changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> index 188f954..95b1954 100644
> --- a/hw/usb/hcd-xhci.c
> +++ b/hw/usb/hcd-xhci.c
> @@ -3594,25 +3594,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);
> -    }
> -

Outside this patch's scope, but here goes anyway:

>      if (xhci->numintrs > MAXINTRS) {
>          xhci->numintrs = MAXINTRS;
>      }
       while (xhci->numintrs & (xhci->numintrs - 1)) {   /* ! power of 2 */
           xhci->numintrs++;
       }
       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 {
> @@ -3634,7 +3615,22 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
>          xhci->max_pstreams_mask = 0;
>      }

If the user specifies invalid intrs, slots or streams properties, his
configuration is silently corrected to a valid one.  I hate that.
Invalid configuration should be rejected cleanly.

By the way, calling the property "intrs" differs needlessly from virtio,
which calls it "vectors".

Anyway, nothing wrong with the patch here.

Before this patch, we can have msi_init() choke on invalid
xhci->numintrs before we reach the configuration correction code.
Your patch fixes it.

>  
> -    xhci->mfwrap_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, xhci_mfwrap_timer, xhci);
> +    if (xhci->msi != ON_OFF_AUTO_OFF) {
> +        ret = msi_init(dev, 0x70, xhci->numintrs, true, false, &err);
> +        /* Any error other than -ENOTSUP(board's MSI support is broken)
> +         * is a programming error */
> +        assert(!ret || ret == -ENOTSUP);
> +        if (ret && xhci->msi == ON_OFF_AUTO_ON) {
> +            /* Can't satisfy user's explicit msi=on request, fail */
> +            error_append_hint(&err, "You have to use msi=auto (default) or "
> +                    "msi=off with this machine type.\n");
> +            error_propagate(errp, err);
> +            return;
> +        }
> +        assert(!err || xhci->msi == ON_OFF_AUTO_AUTO);
> +        /* With msi=auto, we fall back to MSI off silently */
> +        error_free(err);
> +    }
>  
>      memory_region_init(&xhci->mem, OBJECT(xhci), "xhci", LEN_REGS);
>      memory_region_init_io(&xhci->mem_cap, OBJECT(xhci), &xhci_cap_ops, xhci,
> @@ -3664,6 +3660,9 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
>                       PCI_BASE_ADDRESS_SPACE_MEMORY|PCI_BASE_ADDRESS_MEM_TYPE_64,
>                       &xhci->mem);
>  
> +    usb_xhci_init(xhci);
> +    xhci->mfwrap_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, xhci_mfwrap_timer, xhci);
> +

Before your patch, resources allocated by usb_xhci_init() were leaked on
msi_init() failure.

Your patch also delays it and timer_new_ns() until after we can't fail
anymore.  That's a good idea unless their work is actually used earlier.
It isn't as far as I can see.

>      if (pci_bus_is_express(dev->bus) ||
>          xhci_get_flag(xhci, XHCI_FLAG_FORCE_PCIE_ENDCAP)) {
>          ret = pcie_endpoint_cap_init(dev, 0xa0);

I can take this through my tree if Gerd provides at least his Acked-by,
preferably Reviewed-by.  Gerd, if you'd rather take it through yours,
let me know.  I'll take this series into my tree then, wait for the
patch to make its way through yours, and rebase.

Re: [Qemu-devel] [PATCH v3 2/8] hcd-xhci: check & correct param before using it

[Gerd Hoffmann]

  Hi,

> I can take this through my tree if Gerd provides at least his Acked-by,
> preferably Reviewed-by.  Gerd, if you'd rather take it through yours,
> let me know.  I'll take this series into my tree then, wait for the
> patch to make its way through yours, and rebase.

I'm fine with you merging the patch.  Splitting the series only makes
things more complicated for no good reason.

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

cheers,
  Gerd

[Qemu-devel] [PATCH v3 3/8] pci: Convert msix_init() to Error and fix callers to check it

[Cao jin]

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

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

Bonus: add comment for msix_init.

CC: Jiri Pirko <jiri@resnulli.us>
CC: Gerd Hoffmann <kraxel@redhat.com>
CC: Dmitry Fleytman <dmitry@daynix.com>
CC: Jason Wang <jasowang@redhat.com>
CC: Michael S. Tsirkin <mst@redhat.com>
CC: Hannes Reinecke <hare@suse.de>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Alex Williamson <alex.williamson@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Marcel Apfelbaum <marcel@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/block/nvme.c        |  5 ++++-
 hw/misc/ivshmem.c      |  8 ++++----
 hw/net/e1000e.c        |  6 +++++-
 hw/net/rocker/rocker.c |  7 ++++++-
 hw/net/vmxnet3.c       |  8 ++++++--
 hw/pci/msix.c          | 33 ++++++++++++++++++++++++++++-----
 hw/scsi/megasas.c      |  5 ++++-
 hw/usb/hcd-xhci.c      | 13 ++++++++-----
 hw/vfio/pci.c          |  7 +++++--
 hw/virtio/virtio-pci.c | 11 +++++------
 include/hw/pci/msix.h  |  5 +++--
 11 files changed, 78 insertions(+), 30 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index cef3bb4..ae84dc7 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -829,6 +829,7 @@ static int nvme_init(PCIDevice *pci_dev)
 {
     NvmeCtrl *n = NVME(pci_dev);
     NvmeIdCtrl *id = &n->id_ctrl;
+    Error *err = NULL;
 
     int i;
     int64_t bs_size;
@@ -870,7 +871,9 @@ static int nvme_init(PCIDevice *pci_dev)
     pci_register_bar(&n->parent_obj, 0,
         PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_64,
         &n->iomem);
-    msix_init_exclusive_bar(&n->parent_obj, n->num_queues, 4);
+    if (msix_init_exclusive_bar(&n->parent_obj, n->num_queues, 4, &err)) {
+        error_report_err(err);
+    }
 
     id->vid = cpu_to_le16(pci_get_word(pci_conf + PCI_VENDOR_ID));
     id->ssvid = cpu_to_le16(pci_get_word(pci_conf + PCI_SUBSYSTEM_VENDOR_ID));
diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index 40a2ebc..a1060ec 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -750,13 +750,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_add_handlers(s->server_chr, ivshmem_can_receive,
                               ivshmem_read, NULL, s);
 
-        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 bad43f4..942ec8a 100644
--- a/hw/net/e1000e.c
+++ b/hw/net/e1000e.c
@@ -292,7 +292,11 @@ e1000e_init_msix(E1000EState *s)
                         E1000E_MSIX_IDX, E1000E_MSIX_TABLE,
                         &s->msix,
                         E1000E_MSIX_IDX, E1000E_MSIX_PBA,
-                        0xA0);
+                        0xA0, NULL);
+
+    /* Any error other than -ENOTSUP(board's MSI support is broken)
+     * is a programming error. Fall back to INTx silently on -ENOTSUP */
+    assert(!res || res == -ENOTSUP);
 
     if (res < 0) {
         trace_e1000e_msix_init_fail(res);
diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
index 30f2ce4..34aa298 100644
--- a/hw/net/rocker/rocker.c
+++ b/hw/net/rocker/rocker.c
@@ -1256,14 +1256,19 @@ static int rocker_msix_init(Rocker *r)
 {
     PCIDevice *dev = PCI_DEVICE(r);
     int err;
+    Error *local_err = NULL;
 
     err = msix_init(dev, ROCKER_MSIX_VEC_COUNT(r->fp_ports),
                     &r->msix_bar,
                     ROCKER_PCI_MSIX_BAR_IDX, ROCKER_PCI_MSIX_TABLE_OFFSET,
                     &r->msix_bar,
                     ROCKER_PCI_MSIX_BAR_IDX, ROCKER_PCI_MSIX_PBA_OFFSET,
-                    0);
+                    0, &local_err);
+    /* Any error other than -ENOTSUP(board's MSI support is broken)
+     * is a programming error. */
+    assert(!err || err == -ENOTSUP);
     if (err) {
+        error_report_err(local_err);
         return err;
     }
 
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 90f6943..7d44af1 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -2190,10 +2190,14 @@ vmxnet3_init_msix(VMXNET3State *s)
                         VMXNET3_MSIX_BAR_IDX, VMXNET3_OFF_MSIX_TABLE,
                         &s->msix_bar,
                         VMXNET3_MSIX_BAR_IDX, VMXNET3_OFF_MSIX_PBA(s),
-                        VMXNET3_MSIX_OFFSET(s));
+                        VMXNET3_MSIX_OFFSET(s), NULL);
+
+    /* Any error other than -ENOTSUP(board's MSI support is broken)
+     * is a programming error. Fall back to INTx on -ENOTSUP */
+    assert(!res || res == -ENOTSUP);
 
     if (0 > res) {
-        VMW_WRPRN("Failed to initialize MSI-X, error %d", res);
+        VMW_WRPRN("Failed to initialize MSI-X, board's MSI support is broken");
         s->msix_used = false;
     } else {
         if (!vmxnet3_use_msix_vectors(s, VMXNET3_MAX_INTRS)) {
diff --git a/hw/pci/msix.c b/hw/pci/msix.c
index 0cee631..d6b38fe 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,28 @@ 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.
+ *
+ * Return 0 on success; 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 +268,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 +286,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 +329,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 +361,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 e968302..9db4bac 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -2356,9 +2356,12 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
 
     if (megasas_use_msix(s) &&
         msix_init(dev, 15, &s->mmio_io, b->mmio_bar, 0x2000,
-                  &s->mmio_io, b->mmio_bar, 0x3800, 0x68)) {
+                  &s->mmio_io, b->mmio_bar, 0x3800, 0x68, &err)) {
+        /*TODO: check msix_init's error, and should fail on msix=on */
+        error_report_err(err);
         s->msix = ON_OFF_AUTO_OFF;
     }
+
     if (pci_is_express(dev)) {
         pcie_endpoint_cap_init(dev, 0xa0);
     }
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 95b1954..59f0239 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -3670,11 +3670,14 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
     }
 
     if (xhci->msix != ON_OFF_AUTO_OFF) {
-        /* TODO check for errors */
-        msix_init(dev, xhci->numintrs,
-                  &xhci->mem, 0, OFF_MSIX_TABLE,
-                  &xhci->mem, 0, OFF_MSIX_PBA,
-                  0x90);
+        /* TODO check for errors, and should fail when msix=on */
+        ret = msix_init(dev, xhci->numintrs,
+                        &xhci->mem, 0, OFF_MSIX_TABLE,
+                        &xhci->mem, 0, OFF_MSIX_PBA,
+                        0x90, &err);
+        if (ret) {
+            error_report_err(err);
+        }
     }
 }
 
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 7bfa17c..87f4e11 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1349,6 +1349,7 @@ static int vfio_msix_early_setup(VFIOPCIDevice *vdev)
 static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos)
 {
     int ret;
+    Error *err = NULL;
 
     vdev->msix->pending = g_malloc0(BITS_TO_LONGS(vdev->msix->entries) *
                                     sizeof(unsigned long));
@@ -1356,12 +1357,14 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos)
                     vdev->bars[vdev->msix->table_bar].region.mem,
                     vdev->msix->table_bar, vdev->msix->table_offset,
                     vdev->bars[vdev->msix->pba_bar].region.mem,
-                    vdev->msix->pba_bar, vdev->msix->pba_offset, pos);
+                    vdev->msix->pba_bar, vdev->msix->pba_offset, pos,
+                    &err);
     if (ret < 0) {
         if (ret == -ENOTSUP) {
             return 0;
         }
-        error_report("vfio: msix_init failed");
+        error_prepend(&err, "vfio: msix_init failed: ");
+        error_report_err(err);
         return ret;
     }
 
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 755f921..fff4fcd 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1657,13 +1657,12 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
 
     if (proxy->nvectors) {
         int err = msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors,
-                                          proxy->msix_bar);
+                                          proxy->msix_bar, errp);
+        /* Any error other than -ENOTSUP(board's MSI support is broken)
+         * is a programming error. */
+        assert(!err || err == -ENOTSUP);
         if (err) {
-            /* Notice when a system that supports MSIx can't initialize it.  */
-            if (err != -ENOTSUP) {
-                error_report("unable to init msix vectors to %" PRIu32,
-                             proxy->nvectors);
-            }
+            error_report_err(*errp);
             proxy->nvectors = 0;
         }
     }
diff --git a/include/hw/pci/msix.h b/include/hw/pci/msix.h
index 048a29d..1f27658 100644
--- a/include/hw/pci/msix.h
+++ b/include/hw/pci/msix.h
@@ -9,9 +9,10 @@ MSIMessage msix_get_message(PCIDevice *dev, unsigned int vector);
 int msix_init(PCIDevice *dev, unsigned short nentries,
               MemoryRegion *table_bar, uint8_t table_bar_nr,
               unsigned table_offset, MemoryRegion *pba_bar,
-              uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos);
+              uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos,
+              Error **errp);
 int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
-                            uint8_t bar_nr);
+                            uint8_t bar_nr, Error **errp);
 
 void msix_write_config(PCIDevice *dev, uint32_t address, uint32_t val, int len);
 
-- 
2.1.0

Re: [Qemu-devel] [PATCH v3 3/8] pci: Convert msix_init() to Error and fix callers to check it

[Markus Armbruster]

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

> msix_init() reports errors with error_report(), which is wrong when
> it's used in realize().  The same issue was fixed for msi_init() in
> commit 1108b2f.
>
> For some devices(like e1000e, vmxnet3) who won't fail because of
> msix_init's failure, suppress the error report by passing NULL error object.
>
> Bonus: add comment for msix_init.
>
> CC: Jiri Pirko <jiri@resnulli.us>
> CC: Gerd Hoffmann <kraxel@redhat.com>
> CC: Dmitry Fleytman <dmitry@daynix.com>
> CC: Jason Wang <jasowang@redhat.com>
> CC: Michael S. Tsirkin <mst@redhat.com>
> CC: Hannes Reinecke <hare@suse.de>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Alex Williamson <alex.williamson@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Marcel Apfelbaum <marcel@redhat.com>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
[...]
> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> index 0cee631..d6b38fe 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,28 @@ 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.
> + *
> + * Return 0 on success; return -errno on error:

Previous version had:

  + * @errp is for returning errors.
  + *
  + * Return 0 on success; set @errp and return -errno on error.

Intentional change?  I like the old one better.

> + * -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;
[...]
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 7bfa17c..87f4e11 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -1349,6 +1349,7 @@ static int vfio_msix_early_setup(VFIOPCIDevice *vdev)
>  static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos)
>  {
>      int ret;
> +    Error *err = NULL;
>  
>      vdev->msix->pending = g_malloc0(BITS_TO_LONGS(vdev->msix->entries) *
>                                      sizeof(unsigned long));
> @@ -1356,12 +1357,14 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos)
>                      vdev->bars[vdev->msix->table_bar].region.mem,
>                      vdev->msix->table_bar, vdev->msix->table_offset,
>                      vdev->bars[vdev->msix->pba_bar].region.mem,
> -                    vdev->msix->pba_bar, vdev->msix->pba_offset, pos);
> +                    vdev->msix->pba_bar, vdev->msix->pba_offset, pos,
> +                    &err);
>      if (ret < 0) {
>          if (ret == -ENOTSUP) {
>              return 0;
>          }
> -        error_report("vfio: msix_init failed");
> +        error_prepend(&err, "vfio: msix_init failed: ");
> +        error_report_err(err);
>          return ret;
>      }
>  

Might conflict with Eric Auger's "Convert VFIO-PCI to realize" series,
but resolving that shouldn't be hard.

[...]

The conversion looks good to me now.

Re: [Qemu-devel] [PATCH v3 3/8] pci: Convert msix_init() to Error and fix callers to check it

[Cao jin]

On 09/29/2016 10:17 PM, Markus Armbruster wrote:
> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>

>>
>> -/* 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.
>> + *
>> + * Return 0 on success; return -errno on error:
>
> Previous version had:
>
>    + * @errp is for returning errors.
>    + *
>    + * Return 0 on success; set @errp and return -errno on error.
>
> Intentional change?  I like the old one better.
>

Oh...it was lost by me. I was planning move these comments into a 
separate patch, but later feel that it is not worth the trouble, so I 
undo the movement, it is lost during the process.

>
> Might conflict with Eric Auger's "Convert VFIO-PCI to realize" series,
> but resolving that shouldn't be hard.
>
> [...]
>
> The conversion looks good to me now.
>
>
> .
>

-- 
Yours Sincerely,

Cao jin

Re: [Qemu-devel] [PATCH v3 3/8] pci: Convert msix_init() to Error and fix callers to check it

[Markus Armbruster]

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

> On 09/29/2016 10:17 PM, Markus Armbruster wrote:
>> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>>
>
>>>
>>> -/* 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.
>>> + *
>>> + * Return 0 on success; return -errno on error:
>>
>> Previous version had:
>>
>>    + * @errp is for returning errors.
>>    + *
>>    + * Return 0 on success; set @errp and return -errno on error.
>>
>> Intentional change?  I like the old one better.
>>
>
> Oh...it was lost by me. I was planning move these comments into a
> separate patch, but later feel that it is not worth the trouble, so I
> undo the movement, it is lost during the process.

Let's restore it then.

>> Might conflict with Eric Auger's "Convert VFIO-PCI to realize" series,
>> but resolving that shouldn't be hard.
>>
>> [...]
>>
>> The conversion looks good to me now.

Re: [Qemu-devel] [PATCH v3 3/8] pci: Convert msix_init() to Error and fix callers to check it

[Cao jin]

On 09/30/2016 03:01 PM, Markus Armbruster wrote:
> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>
>> On 09/29/2016 10:17 PM, Markus Armbruster wrote:

>>>
>>> Intentional change?  I like the old one better.
>>>
>>
>> Oh...it was lost by me. I was planning move these comments into a
>> separate patch, but later feel that it is not worth the trouble, so I
>> undo the movement, it is lost during the process.
>
> Let's restore it then.
>

sure, I will send new version to fix all these comments, after get 
confirmation from Dmitry about the last patch: vmxnet3

-- 
Yours Sincerely,

Cao jin

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

diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 9db4bac..d801dd0 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -2349,19 +2349,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 || d->msix == ON_OFF_AUTO_AUTO);
+        /* With msix=auto, we fall back to MSI off silently */
+        error_free(err);
+    }
+
     memory_region_init_io(&s->port_io, OBJECT(s), &megasas_port_ops, s,
                           "megasas-io", 256);
     memory_region_init_io(&s->queue_io, OBJECT(s), &megasas_queue_ops, s,
                           "megasas-queue", 0x40000);
 
-    if (megasas_use_msix(s) &&
-        msix_init(dev, 15, &s->mmio_io, b->mmio_bar, 0x2000,
-                  &s->mmio_io, b->mmio_bar, 0x3800, 0x68, &err)) {
-        /*TODO: check msix_init's error, and should fail on msix=on */
-        error_report_err(err);
-        s->msix = ON_OFF_AUTO_OFF;
-    }
-
     if (pci_is_express(dev)) {
         pcie_endpoint_cap_init(dev, 0xa0);
     }
-- 
2.1.0

[Qemu-devel] [PATCH v3 5/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>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/usb/hcd-xhci.c | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 59f0239..4280c5d 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -3603,12 +3603,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 {
@@ -3633,6 +3635,28 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
     }
 
     memory_region_init(&xhci->mem, OBJECT(xhci), "xhci", LEN_REGS);
+    if (xhci->msix != ON_OFF_AUTO_OFF) {
+        ret = msix_init(dev, xhci->numintrs,
+                        &xhci->mem, 0, OFF_MSIX_TABLE,
+                        &xhci->mem, 0, OFF_MSIX_PBA,
+                        0x90, &err);
+        /* Any error other than -ENOTSUP(board's MSI support is broken)
+         * is a programming error */
+        assert(!ret || ret == -ENOTSUP);
+        if (ret && xhci->msix == ON_OFF_AUTO_ON) {
+            /* Can't satisfy user's explicit msix=on request, fail */
+            error_append_hint(&err, "You have to use msix=auto (default) or "
+                    "msic=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,
@@ -3668,17 +3692,6 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
         ret = pcie_endpoint_cap_init(dev, 0xa0);
         assert(ret >= 0);
     }
-
-    if (xhci->msix != ON_OFF_AUTO_OFF) {
-        /* TODO check for errors, and should fail when msix=on */
-        ret = msix_init(dev, xhci->numintrs,
-                        &xhci->mem, 0, OFF_MSIX_TABLE,
-                        &xhci->mem, 0, OFF_MSIX_PBA,
-                        0x90, &err);
-        if (ret) {
-            error_report_err(err);
-        }
-    }
 }
 
 static void usb_xhci_exit(PCIDevice *dev)
-- 
2.1.0

Re: [Qemu-devel] [PATCH v3 5/8] hcd-xhci: change behaviour of msix switch

[Markus Armbruster]

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

> 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>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/usb/hcd-xhci.c | 35 ++++++++++++++++++++++++-----------
>  1 file changed, 24 insertions(+), 11 deletions(-)
>
> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> index 59f0239..4280c5d 100644
> --- a/hw/usb/hcd-xhci.c
> +++ b/hw/usb/hcd-xhci.c
> @@ -3603,12 +3603,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 {
> @@ -3633,6 +3635,28 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
>      }
>  
>      memory_region_init(&xhci->mem, OBJECT(xhci), "xhci", LEN_REGS);
> +    if (xhci->msix != ON_OFF_AUTO_OFF) {
> +        ret = msix_init(dev, xhci->numintrs,
> +                        &xhci->mem, 0, OFF_MSIX_TABLE,
> +                        &xhci->mem, 0, OFF_MSIX_PBA,
> +                        0x90, &err);
> +        /* Any error other than -ENOTSUP(board's MSI support is broken)
> +         * is a programming error */
> +        assert(!ret || ret == -ENOTSUP);
> +        if (ret && xhci->msix == ON_OFF_AUTO_ON) {
> +            /* Can't satisfy user's explicit msix=on request, fail */
> +            error_append_hint(&err, "You have to use msix=auto (default) or "
> +                    "msic=off with this machine type.\n");

s/msic/msix/

Can hopefully be touched up on commit.

> +            /* 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,
> @@ -3668,17 +3692,6 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
>          ret = pcie_endpoint_cap_init(dev, 0xa0);
>          assert(ret >= 0);
>      }
> -
> -    if (xhci->msix != ON_OFF_AUTO_OFF) {
> -        /* TODO check for errors, and should fail when msix=on */
> -        ret = msix_init(dev, xhci->numintrs,
> -                        &xhci->mem, 0, OFF_MSIX_TABLE,
> -                        &xhci->mem, 0, OFF_MSIX_PBA,
> -                        0x90, &err);
> -        if (ret) {
> -            error_report_err(err);
> -        }
> -    }
>  }
>  
>  static void usb_xhci_exit(PCIDevice *dev)

Re: [Qemu-devel] [PATCH v3 5/8] hcd-xhci: change behaviour of msix switch

[Gerd Hoffmann]

On Do, 2016-09-29 at 16:32 +0200, Markus Armbruster wrote:
> > Resolve the TODO, msix=auto means msix on; if user specify msix=on,
> > then device creation fail on msix_init failure.

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

[Qemu-devel] [PATCH v3 6/8] megasas: remove unnecessary megasas_use_msix()

[Cao jin]

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>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/scsi/megasas.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index d801dd0..90cd873 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;
@@ -2295,9 +2290,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);
 }
 
@@ -2349,7 +2342,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)
@@ -2364,11 +2357,15 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
             error_propagate(errp, err);
             return;
         }
-        assert(!err || d->msix == ON_OFF_AUTO_AUTO);
+        assert(!err || s->msix == ON_OFF_AUTO_AUTO);
         /* With msix=auto, we fall back to MSI off silently */
         error_free(err);
     }
 
+    if (msix_enabled(dev)) {
+        msix_vector_use(dev, 0);
+    }
+
     memory_region_init_io(&s->port_io, OBJECT(s), &megasas_port_ops, s,
                           "megasas-io", 256);
     memory_region_init_io(&s->queue_io, OBJECT(s), &megasas_queue_ops, s,
@@ -2384,10 +2381,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 v3 6/8] megasas: remove unnecessary megasas_use_msix()

[Markus Armbruster]

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

> 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>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/scsi/megasas.c | 21 +++++++--------------
>  1 file changed, 7 insertions(+), 14 deletions(-)
>
> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index d801dd0..90cd873 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;
> @@ -2295,9 +2290,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);
>  }
>  
> @@ -2349,7 +2342,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)
> @@ -2364,11 +2357,15 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
>              error_propagate(errp, err);
>              return;
>          }
> -        assert(!err || d->msix == ON_OFF_AUTO_AUTO);
> +        assert(!err || s->msix == ON_OFF_AUTO_AUTO);

You add this line in PATCH 4.  Could it use s->msix from the start?

>          /* With msix=auto, we fall back to MSI off silently */
>          error_free(err);
>      }
>  
> +    if (msix_enabled(dev)) {
> +        msix_vector_use(dev, 0);
> +    }
> +
>      memory_region_init_io(&s->port_io, OBJECT(s), &megasas_port_ops, s,
>                            "megasas-io", 256);
>      memory_region_init_io(&s->queue_io, OBJECT(s), &megasas_queue_ops, s,
> @@ -2384,10 +2381,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) |

Can you explain why you move the code?

Re: [Qemu-devel] [PATCH v3 6/8] megasas: remove unnecessary megasas_use_msix()

[Cao jin]

On 09/29/2016 10:35 PM, Markus Armbruster wrote:
> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>

>>
>> @@ -2349,7 +2342,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)
>> @@ -2364,11 +2357,15 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
>>               error_propagate(errp, err);
>>               return;
>>           }
>> -        assert(!err || d->msix == ON_OFF_AUTO_AUTO);
>> +        assert(!err || s->msix == ON_OFF_AUTO_AUTO);
>
> You add this line in PATCH 4.  Could it use s->msix from the start?
>

It seems a copy&paste error...for the mistake.

>>           /* With msix=auto, we fall back to MSI off silently */
>>           error_free(err);
>>       }
>>
>> +    if (msix_enabled(dev)) {
>> +        msix_vector_use(dev, 0);
>> +    }
>> +
>>       memory_region_init_io(&s->port_io, OBJECT(s), &megasas_port_ops, s,
>>                             "megasas-io", 256);
>>       memory_region_init_io(&s->queue_io, OBJECT(s), &megasas_queue_ops, s,
>> @@ -2384,10 +2381,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) |
>
> Can you explain why you move the code?
>

Oh...just place the msix init related code together, for logical, 
nothing special.

-- 
Yours Sincerely,

Cao jin

Re: [Qemu-devel] [PATCH v3 6/8] megasas: remove unnecessary megasas_use_msix()

[Markus Armbruster]

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

> On 09/29/2016 10:35 PM, Markus Armbruster wrote:
>> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>>
>
>>>
>>> @@ -2349,7 +2342,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)
>>> @@ -2364,11 +2357,15 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
>>>               error_propagate(errp, err);
>>>               return;
>>>           }
>>> -        assert(!err || d->msix == ON_OFF_AUTO_AUTO);
>>> +        assert(!err || s->msix == ON_OFF_AUTO_AUTO);
>>
>> You add this line in PATCH 4.  Could it use s->msix from the start?
>>
>
> It seems a copy&paste error...for the mistake.

Easy enough to fix :)

>>>           /* With msix=auto, we fall back to MSI off silently */
>>>           error_free(err);
>>>       }
>>>
>>> +    if (msix_enabled(dev)) {
>>> +        msix_vector_use(dev, 0);
>>> +    }
>>> +
>>>       memory_region_init_io(&s->port_io, OBJECT(s), &megasas_port_ops, s,
>>>                             "megasas-io", 256);
>>>       memory_region_init_io(&s->queue_io, OBJECT(s), &megasas_queue_ops, s,
>>> @@ -2384,10 +2381,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) |
>>
>> Can you explain why you move the code?
>>
>
> Oh...just place the msix init related code together, for logical,
> nothing special.

Okay with me, but please mention it in the commit message.

[Qemu-devel] [PATCH v3 7/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>
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 90cd873..ff314a2 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -2333,11 +2333,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 v3 8/8] vmxnet3: remove unnecessary internal msix state flag

[Cao jin]

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

CC: Dmitry Fleytman <dmitry@daynix.com>
CC: Jason Wang <jasowang@redhat.com>
CC: Michael S. Tsirkin <mst@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/net/vmxnet3.c | 38 ++++++++++++++------------------------
 1 file changed, 14 insertions(+), 24 deletions(-)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 7d44af1..1decb53 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;
     }
 
@@ -1427,7 +1425,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;
 }
 
@@ -1444,18 +1442,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);
     }
 }
 
@@ -2184,6 +2182,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,
@@ -2198,17 +2197,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
@@ -2216,7 +2216,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);
     }
@@ -2551,21 +2551,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

Re: [Qemu-devel] [PATCH v3 8/8] vmxnet3: remove unnecessary internal msix state flag

[Markus Armbruster]

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

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

Repeating the rationale for the change wouldn't hurt:

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

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

Can hopefully be touched up on commit.

>
> CC: Dmitry Fleytman <dmitry@daynix.com>
> CC: Jason Wang <jasowang@redhat.com>
> CC: Michael S. Tsirkin <mst@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/net/vmxnet3.c | 38 ++++++++++++++------------------------
>  1 file changed, 14 insertions(+), 24 deletions(-)
>
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 7d44af1..1decb53 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;
>      }
>  
> @@ -1427,7 +1425,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;
>  }
>  
> @@ -1444,18 +1442,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);
>      }
>  }
>  
> @@ -2184,6 +2182,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,
> @@ -2198,17 +2197,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
> @@ -2216,7 +2216,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);
>      }
> @@ -2551,21 +2551,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);

This hunk isn't obvious.  Can you explain the change?

Re: [Qemu-devel] [PATCH v3 8/8] vmxnet3: remove unnecessary internal msix state flag

[Cao jin]

On 09/29/2016 10:42 PM, Markus Armbruster wrote:
> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>

>>   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);
>
> This hunk isn't obvious.  Can you explain the change?
>

flag msix_used is used in VMStateDescription.post_Load().

1st, I think msix's code here is not necessary, because in destination, 
device has been realized before incoming migration, So I don't know why 
re-use MSI-X vectors here. Dmitry, could help to explain?

2nd, this patch is going to remove this flag, so I removed the hunk.

-- 
Yours Sincerely,

Cao jin

Re: [Qemu-devel] [PATCH v3 8/8] vmxnet3: remove unnecessary internal msix state flag

[Markus Armbruster]

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

> On 09/29/2016 10:42 PM, Markus Armbruster wrote:
>> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>>
>
>>>   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);
>>
>> This hunk isn't obvious.  Can you explain the change?
>>
>
> flag msix_used is used in VMStateDescription.post_Load().
>
> 1st, I think msix's code here is not necessary, because in
> destination, device has been realized before incoming migration, So I
> don't know why re-use MSI-X vectors here. Dmitry, could help to
> explain?
>
> 2nd, this patch is going to remove this flag, so I removed the hunk.

We need to find out whether the call of vmxnet3_use_msix_vectors() is
necessary.  I suspect it's not only not necessary, but actively wrong.

If that's true, removing becomes a bug fix that should be a separate
patch.

If it's only unnecessary, the removal may stay in this patch, but it
needs to be explained.  Separate patch might be easier to explain.  Your
choice.

If it correct and necessary, then this patch needs to be changed not to
drop it.  Instead, replace s->msix_used by msix_enabled(d) like you do
elsewhere.

Dmitry, can you help us find out?

Re: [Qemu-devel] [PATCH v3 8/8] vmxnet3: remove unnecessary internal msix state flag

[Dmitry Fleytman]

> On 30 Sep 2016, at 15:08 PM, Markus Armbruster <armbru@redhat.com> wrote:
> 
> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
> 
>> On 09/29/2016 10:42 PM, Markus Armbruster wrote:
>>> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>>> 
>> 
>>>>  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);
>>> 
>>> This hunk isn't obvious.  Can you explain the change?
>>> 
>> 
>> flag msix_used is used in VMStateDescription.post_Load().
>> 
>> 1st, I think msix's code here is not necessary, because in
>> destination, device has been realized before incoming migration, So I
>> don't know why re-use MSI-X vectors here. Dmitry, could help to
>> explain?
>> 
>> 2nd, this patch is going to remove this flag, so I removed the hunk.
> 
> We need to find out whether the call of vmxnet3_use_msix_vectors() is
> necessary.  I suspect it's not only not necessary, but actively wrong.
> 
> If that's true, removing becomes a bug fix that should be a separate
> patch.
> 
> If it's only unnecessary, the removal may stay in this patch, but it
> needs to be explained.  Separate patch might be easier to explain.  Your
> choice.
> 
> If it correct and necessary, then this patch needs to be changed not to
> drop it.  Instead, replace s->msix_used by msix_enabled(d) like you do
> elsewhere.
> 
> Dmitry, can you help us find out?

Hello,

Yes, from what I see, this call is wrong and leads to
reference leaks on device unload at migration target.
It should be removed.

Best regards,
Dmitry

Re: [Qemu-devel] [PATCH v3 8/8] vmxnet3: remove unnecessary internal msix state flag

[Dr. David Alan Gilbert]

* Dmitry Fleytman (dmitry@daynix.com) wrote:
> 
> > On 30 Sep 2016, at 15:08 PM, Markus Armbruster <armbru@redhat.com> wrote:
> > 
> > Cao jin <caoj.fnst@cn.fujitsu.com> writes:
> > 
> >> On 09/29/2016 10:42 PM, Markus Armbruster wrote:
> >>> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
> >>> 
> >> 
> >>>>  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);
> >>> 
> >>> This hunk isn't obvious.  Can you explain the change?
> >>> 
> >> 
> >> flag msix_used is used in VMStateDescription.post_Load().
> >> 
> >> 1st, I think msix's code here is not necessary, because in
> >> destination, device has been realized before incoming migration, So I
> >> don't know why re-use MSI-X vectors here. Dmitry, could help to
> >> explain?
> >> 
> >> 2nd, this patch is going to remove this flag, so I removed the hunk.
> > 
> > We need to find out whether the call of vmxnet3_use_msix_vectors() is
> > necessary.  I suspect it's not only not necessary, but actively wrong.
> > 
> > If that's true, removing becomes a bug fix that should be a separate
> > patch.
> > 
> > If it's only unnecessary, the removal may stay in this patch, but it
> > needs to be explained.  Separate patch might be easier to explain.  Your
> > choice.
> > 
> > If it correct and necessary, then this patch needs to be changed not to
> > drop it.  Instead, replace s->msix_used by msix_enabled(d) like you do
> > elsewhere.
> > 
> > Dmitry, can you help us find out?
> 
> Hello,
> 
> Yes, from what I see, this call is wrong and leads to
> reference leaks on device unload at migration target.
> It should be removed.

Talking of oddities in vmxnet3's msix load/save,
vmxnet3 has the honour of being the only device that
has both a register_savevm (which registers vmxnet3-msix)
and also has a ->vmsd entry (dc->vmsd = &vmstate_vmxnet3)

What's the history behind that? Is there some ordering requirement
etc about the order the two get loaded/saved?

Dave

> Best regards,
> Dmitry
> 
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 8/8] vmxnet3: remove unnecessary internal msix state flag

[Dmitry Fleytman]

> On 6 Oct 2016, at 17:43, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> 
> * Dmitry Fleytman (dmitry@daynix.com) wrote:
>> 
>>> On 30 Sep 2016, at 15:08 PM, Markus Armbruster <armbru@redhat.com> wrote:
>>> 
>>> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>>> 
>>>>> On 09/29/2016 10:42 PM, Markus Armbruster wrote:
>>>>> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>>>>> 
>>>> 
>>>>>> 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);
>>>>> 
>>>>> This hunk isn't obvious.  Can you explain the change?
>>>>> 
>>>> 
>>>> flag msix_used is used in VMStateDescription.post_Load().
>>>> 
>>>> 1st, I think msix's code here is not necessary, because in
>>>> destination, device has been realized before incoming migration, So I
>>>> don't know why re-use MSI-X vectors here. Dmitry, could help to
>>>> explain?
>>>> 
>>>> 2nd, this patch is going to remove this flag, so I removed the hunk.
>>> 
>>> We need to find out whether the call of vmxnet3_use_msix_vectors() is
>>> necessary.  I suspect it's not only not necessary, but actively wrong.
>>> 
>>> If that's true, removing becomes a bug fix that should be a separate
>>> patch.
>>> 
>>> If it's only unnecessary, the removal may stay in this patch, but it
>>> needs to be explained.  Separate patch might be easier to explain.  Your
>>> choice.
>>> 
>>> If it correct and necessary, then this patch needs to be changed not to
>>> drop it.  Instead, replace s->msix_used by msix_enabled(d) like you do
>>> elsewhere.
>>> 
>>> Dmitry, can you help us find out?
>> 
>> Hello,
>> 
>> Yes, from what I see, this call is wrong and leads to
>> reference leaks on device unload at migration target.
>> It should be removed.
> 
> Talking of oddities in vmxnet3's msix load/save,
> vmxnet3 has the honour of being the only device that
> has both a register_savevm (which registers vmxnet3-msix)
> and also has a ->vmsd entry (dc->vmsd = &vmstate_vmxnet3)
> 
> What's the history behind that? Is there some ordering requirement
> etc about the order the two get loaded/saved?
> 
> Dave

Hi Dave,

There is no specific history behind that. Vmxnet3 code was written long time ago and this part is just a legacy code that was not cleaned up as QEMU code base evolved.

~Dmitry

> 
>> Best regards,
>> Dmitry
>> 
>> 
>> 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 8/8] vmxnet3: remove unnecessary internal msix state flag

[Cao jin]

On 10/06/2016 05:39 PM, Dmitry Fleytman wrote:
>

>
> Hello,
>
> Yes, from what I see, this call is wrong and leads to
> reference leaks on device unload at migration target.
> It should be removed.
>
> Best regards,
> Dmitry
>

Hi,Dmitry

     From what I saw, current code will call msix_vector_use twice on 
migration target, which is wrong. I am not quite follow you on 
"reference leak", are you saying the same thing as me? if not, could you 
help to explain more?

-- 
Yours Sincerely,

Cao jin

Re: [Qemu-devel] [PATCH v3 8/8] vmxnet3: remove unnecessary internal msix state flag

[Dmitry Fleytman]

> On 11 Oct 2016, at 6:35, Cao jin <caoj.fnst@cn.fujitsu.com> wrote:
> 
> 
> 
>> On 10/06/2016 05:39 PM, Dmitry Fleytman wrote:
>> 
> 
>> 
>> Hello,
>> 
>> Yes, from what I see, this call is wrong and leads to
>> reference leaks on device unload at migration target.
>> It should be removed.
>> 
>> Best regards,
>> Dmitry
>> 
> 
> Hi,Dmitry
> 
>    From what I saw, current code will call msix_vector_use twice on migration target, which is wrong. I am not quite follow you on "reference leak", are you saying the same thing as me? if not, could you help to explain more?

Hello,

Yes, I meant exactly this :)

msix_vector_use() increments vector reference counter each time it is called,
so device holds two references to used vectors after migration.

The second reference will be leaked because there is no second call to msix_vector_unuse() on unload.

Best Regards,
Dmitry

> 
> -- 
> Yours Sincerely,
> 
> Cao jin
> 
>