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

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

[Cao jin]

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 (6):
  msix_init: assert programming error
  pci: Convert msix_init() to Error and fix callers to check it
  e1000e: fix for migration compatibility
  e1000e: drop unnecessary funtions
  megasas: remove unnecessary megasas_use_msix()
  megasas: undo the overwrites of user configuration

 hw/misc/ivshmem.c      |  1 +
 hw/net/e1000e.c        | 54 ++++++++++++++------------------------
 hw/net/rocker/rocker.c |  2 +-
 hw/net/vmxnet3.c       | 42 +++++++++--------------------
 hw/pci/msix.c          | 24 ++++++++++-------
 hw/scsi/megasas.c      | 48 +++++++++++++++++++---------------
 hw/usb/hcd-xhci.c      | 71 ++++++++++++++++++++++++++++++--------------------
 hw/vfio/pci.c          |  7 +++--
 hw/virtio/virtio-pci.c |  7 ++---
 include/hw/pci/msix.h  |  3 ++-
 10 files changed, 129 insertions(+), 130 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH 1/6] msix_init: assert programming error

[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 | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/pci/msix.c b/hw/pci/msix.c
index 0ec1cb1..384a29d 100644
--- a/hw/pci/msix.c
+++ b/hw/pci/msix.c
@@ -253,9 +253,7 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
         return -ENOTSUP;
     }
 
-    if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1) {
-        return -EINVAL;
-    }
+    assert(nentries >= 1 && nentries <= PCI_MSIX_FLAGS_QSIZE + 1);
 
     table_size = nentries * PCI_MSIX_ENTRY_SIZE;
     pba_size = QEMU_ALIGN_UP(nentries, 64) / 8;
@@ -266,7 +264,7 @@ 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) {
-        return -EINVAL;
+        assert(0);
     }
 
     cap = pci_add_capability(dev, PCI_CAP_ID_MSIX, cap_pos, MSIX_CAP_LENGTH);
-- 
2.1.0

Re: [Qemu-devel] [PATCH 1/6] msix_init: assert programming error

[Markus Armbruster]

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

> 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 | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> index 0ec1cb1..384a29d 100644
> --- a/hw/pci/msix.c
> +++ b/hw/pci/msix.c
> @@ -253,9 +253,7 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
>          return -ENOTSUP;
>      }
>  
> -    if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1) {
> -        return -EINVAL;
> -    }
> +    assert(nentries >= 1 && nentries <= PCI_MSIX_FLAGS_QSIZE + 1);
>  
>      table_size = nentries * PCI_MSIX_ENTRY_SIZE;
>      pba_size = QEMU_ALIGN_UP(nentries, 64) / 8;
> @@ -266,7 +264,7 @@ 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) {
> -        return -EINVAL;
> +        assert(0);
>      }
>  
>      cap = pci_add_capability(dev, PCI_CAP_ID_MSIX, cap_pos, MSIX_CAP_LENGTH);

Please explain in the commit message why these are programming errors.
If you're unsure how, give it a try in a reply to this message, and
we'll hammer out something together.

Re: [Qemu-devel] [PATCH 1/6] msix_init: assert programming error

[Cao jin]

On 08/18/2016 03:14 PM, Markus Armbruster wrote:
> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>

>
> Please explain in the commit message why these are programming errors.
> If you're unsure how, give it a try in a reply to this message, and
> we'll hammer out something together.
>

Ok, how about this:

The input parameters is used for creating the msix capable device, so 
they must obey the PCI spec, or else, it should be programming error.

-- 
Yours Sincerely,

Cao jin

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

[Cao jin]

msix_init() has the same issue with msi_init(), which reports errors
via error_report(), that is not suitable when it's used in realize().

Fix it by converting it to Error, also fix its callers to
handle failure instead of ignoring it.

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/misc/ivshmem.c      |  1 +
 hw/net/e1000e.c        |  2 +-
 hw/net/rocker/rocker.c |  2 +-
 hw/net/vmxnet3.c       | 42 +++++++++--------------------
 hw/pci/msix.c          | 18 +++++++++----
 hw/scsi/megasas.c      | 25 ++++++++++++++----
 hw/usb/hcd-xhci.c      | 71 ++++++++++++++++++++++++++++++--------------------
 hw/vfio/pci.c          |  7 +++--
 hw/virtio/virtio-pci.c |  7 ++---
 include/hw/pci/msix.h  |  3 ++-
 10 files changed, 101 insertions(+), 77 deletions(-)

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index 40a2ebc..b8511ee 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -899,6 +899,7 @@ static void ivshmem_common_realize(PCIDevice *dev, Error **errp)
 
         if (ivshmem_setup_interrupts(s) < 0) {
             error_setg(errp, "failed to initialize interrupts");
+            error_append_hint(errp, "MSI-X is not supported by interrupt controller");
             return;
         }
     }
diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
index d001c96..82a7be1 100644
--- a/hw/net/e1000e.c
+++ b/hw/net/e1000e.c
@@ -295,7 +295,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 30f2ce4..769b1fd 100644
--- a/hw/net/rocker/rocker.c
+++ b/hw/net/rocker/rocker.c
@@ -1262,7 +1262,7 @@ static int rocker_msix_init(Rocker *r)
                     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, NULL);
     if (err) {
         return err;
     }
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index bbf44ad..0ee80b7 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -2177,32 +2177,6 @@ vmxnet3_use_msix_vectors(VMXNET3State *s, int num_vectors)
     return true;
 }
 
-static bool
-vmxnet3_init_msix(VMXNET3State *s)
-{
-    PCIDevice *d = PCI_DEVICE(s);
-    int res = msix_init(d, VMXNET3_MAX_INTRS,
-                        &s->msix_bar,
-                        VMXNET3_MSIX_BAR_IDX, VMXNET3_OFF_MSIX_TABLE,
-                        &s->msix_bar,
-                        VMXNET3_MSIX_BAR_IDX, VMXNET3_OFF_MSIX_PBA(s),
-                        VMXNET3_MSIX_OFFSET(s));
-
-    if (0 > res) {
-        VMW_WRPRN("Failed to initialize MSI-X, error %d", res);
-        s->msix_used = 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;
-        } else {
-            s->msix_used = true;
-        }
-    }
-    return s->msix_used;
-}
-
 static void
 vmxnet3_cleanup_msix(VMXNET3State *s)
 {
@@ -2311,9 +2285,19 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
      * is a programming error. Fall back to INTx silently on -ENOTSUP */
     assert(!ret || ret == -ENOTSUP);
 
-    if (!vmxnet3_init_msix(s)) {
-        VMW_WRPRN("Failed to initialize MSI-X, configuration is inconsistent.");
-    }
+    ret = msix_init(pci_dev, VMXNET3_MAX_INTRS,
+                    &s->msix_bar,
+                    VMXNET3_MSIX_BAR_IDX, VMXNET3_OFF_MSIX_TABLE,
+                    &s->msix_bar,
+                    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 silently on -ENOTSUP */
+    assert(!ret || ret == -ENOTSUP);
+    s->msix_used = !ret;
+    /* VMXNET3_MAX_INTRS is passed, so it will never fail when mark vector.
+     * For simplicity, no need to check return value. */
+    vmxnet3_use_msix_vectors(s, VMXNET3_MAX_INTRS);
 
     vmxnet3_net_init(s);
 
diff --git a/hw/pci/msix.c b/hw/pci/msix.c
index 384a29d..27fee0a 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
 
@@ -242,7 +243,8 @@ static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
 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,6 +252,7 @@ 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;
     }
 
@@ -267,7 +270,8 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
         assert(0);
     }
 
-    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;
     }
@@ -336,7 +340,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, NULL);
     if (ret) {
         return ret;
     }
@@ -445,8 +449,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;
@@ -481,8 +487,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;
 }
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index e968302..61efb0f 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -2349,16 +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");
+            object_unref(OBJECT(&s->mmio_io));
+            error_propagate(errp, err);
+            return;
+        } else if (ret) {
+            /* With msix=auto, we fall back to MSI off silently */
+            s->msix = ON_OFF_AUTO_OFF;
+            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)) {
-        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 188f954..4280c5d 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;
     }
@@ -3622,21 +3603,60 @@ 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 {
         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);
+    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,
@@ -3664,19 +3684,14 @@ 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);
         assert(ret >= 0);
     }
-
-    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);
-    }
 }
 
 static void usb_xhci_exit(PCIDevice *dev)
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..fae0bf1 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1659,11 +1659,8 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
         int err = msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors,
                                           proxy->msix_bar);
         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("unable to init msix: "
+                         "MSI-X is not supported by interrupt controller");
             proxy->nvectors = 0;
         }
     }
diff --git a/include/hw/pci/msix.h b/include/hw/pci/msix.h
index 048a29d..0b0e31b 100644
--- a/include/hw/pci/msix.h
+++ b/include/hw/pci/msix.h
@@ -9,7 +9,8 @@ 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);
 
-- 
2.1.0

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

[Markus Armbruster]

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

> msix_init() has the same issue with msi_init(), which reports errors
> via error_report(), that is not suitable when it's used in realize().

Suggest to point to commit 1108b2f.  Perhaps:

    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.

> Fix it by converting it to Error, also fix its callers to
> handle failure instead of ignoring it.
>
> 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/misc/ivshmem.c      |  1 +
>  hw/net/e1000e.c        |  2 +-
>  hw/net/rocker/rocker.c |  2 +-
>  hw/net/vmxnet3.c       | 42 +++++++++--------------------
>  hw/pci/msix.c          | 18 +++++++++----
>  hw/scsi/megasas.c      | 25 ++++++++++++++----
>  hw/usb/hcd-xhci.c      | 71 ++++++++++++++++++++++++++++++--------------------
>  hw/vfio/pci.c          |  7 +++--
>  hw/virtio/virtio-pci.c |  7 ++---
>  include/hw/pci/msix.h  |  3 ++-
>  10 files changed, 101 insertions(+), 77 deletions(-)
>
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index 40a2ebc..b8511ee 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -899,6 +899,7 @@ static void ivshmem_common_realize(PCIDevice *dev, Error **errp)
>  
>          if (ivshmem_setup_interrupts(s) < 0) {
>              error_setg(errp, "failed to initialize interrupts");
> +            error_append_hint(errp, "MSI-X is not supported by interrupt controller");
>              return;
>          }
>      }

How is this related to the stated purpose of the patch?

> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
> index d001c96..82a7be1 100644
> --- a/hw/net/e1000e.c
> +++ b/hw/net/e1000e.c
> @@ -295,7 +295,7 @@ e1000e_init_msix(E1000EState *s)
       int res = msix_init(PCI_DEVICE(s), E1000E_MSIX_VEC_NUM,
                           &s->msix,
>                          E1000E_MSIX_IDX, E1000E_MSIX_TABLE,
>                          &s->msix,
>                          E1000E_MSIX_IDX, E1000E_MSIX_PBA,
> -                        0xA0);
> +                        0xA0, NULL);

Further down, you convert msix_init() from error_report() to
error_setg().  It now relies on its callers to report errors.  However,
this caller doesn't.  Suppressing error reports like that may be
appropriate, since the function doesn't fail.  But such a change
shouldn't be hidden in a larger patch without at least a mention in the
commit message.

Here's how I'd skin this cat.  First convert msix_init() without
changing behavior, by having every caller of msix_init() immediately
pass the error received to error_report_err().  Then clean up the
callers one after the other.  The ones that are running within a
realize() method should propagate the error to the realize() method.
The ones that are still running within an init() method keep the
error_report_err().  e1000e_init_msix() may want to ignore the error.
Separaring the cleanups that way lets you explain each actual change
neatly in a commit message.

>  
>      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..769b1fd 100644
> --- a/hw/net/rocker/rocker.c
> +++ b/hw/net/rocker/rocker.c
> @@ -1262,7 +1262,7 @@ static int rocker_msix_init(Rocker *r)
       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, NULL);
>      if (err) {
>          return err;
>      }

This one runs in an init() method.  You're losing an error message here.

> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index bbf44ad..0ee80b7 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -2177,32 +2177,6 @@ vmxnet3_use_msix_vectors(VMXNET3State *s, int num_vectors)
>      return true;
>  }
>  
> -static bool
> -vmxnet3_init_msix(VMXNET3State *s)
> -{
> -    PCIDevice *d = PCI_DEVICE(s);
> -    int res = msix_init(d, VMXNET3_MAX_INTRS,
> -                        &s->msix_bar,
> -                        VMXNET3_MSIX_BAR_IDX, VMXNET3_OFF_MSIX_TABLE,
> -                        &s->msix_bar,
> -                        VMXNET3_MSIX_BAR_IDX, VMXNET3_OFF_MSIX_PBA(s),
> -                        VMXNET3_MSIX_OFFSET(s));
> -
> -    if (0 > res) {
> -        VMW_WRPRN("Failed to initialize MSI-X, error %d", res);
> -        s->msix_used = 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;
> -        } else {
> -            s->msix_used = true;
> -        }
> -    }
> -    return s->msix_used;
> -}
> -
>  static void
>  vmxnet3_cleanup_msix(VMXNET3State *s)
>  {
> @@ -2311,9 +2285,19 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
>       * is a programming error. Fall back to INTx silently on -ENOTSUP */
>      assert(!ret || ret == -ENOTSUP);
>  
> -    if (!vmxnet3_init_msix(s)) {
> -        VMW_WRPRN("Failed to initialize MSI-X, configuration is inconsistent.");
> -    }
> +    ret = msix_init(pci_dev, VMXNET3_MAX_INTRS,
> +                    &s->msix_bar,
> +                    VMXNET3_MSIX_BAR_IDX, VMXNET3_OFF_MSIX_TABLE,
> +                    &s->msix_bar,
> +                    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 silently on -ENOTSUP */
> +    assert(!ret || ret == -ENOTSUP);
> +    s->msix_used = !ret;
> +    /* VMXNET3_MAX_INTRS is passed, so it will never fail when mark vector.
> +     * For simplicity, no need to check return value. */
> +    vmxnet3_use_msix_vectors(s, VMXNET3_MAX_INTRS);
>  
>      vmxnet3_net_init(s);
>  

This is similar to the e1000e case.

> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> index 384a29d..27fee0a 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
>  
> @@ -242,7 +243,8 @@ static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)

   /* Initialize the MSI-X structures */
>  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)

Turning the function comment into a contract similar to the one next to
msi_init() would be nice.

>  {
>      int cap;
>      unsigned table_size, pba_size;
> @@ -250,6 +252,7 @@ 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;
>      }
>  
> @@ -267,7 +270,8 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
>          assert(0);
>      }
>  
> -    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;
>      }
> @@ -336,7 +340,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, NULL);
>      if (ret) {
>          return ret;
>      }

This is a case where you have to propagate the error.  First step:
convert msix_exclusive_bar() to Error, too.

> @@ -445,8 +449,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;

Let's drop this hunk.

> @@ -481,8 +487,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;
>  }

This one, too.

> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index e968302..61efb0f 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -2349,16 +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");
> +            object_unref(OBJECT(&s->mmio_io));
> +            error_propagate(errp, err);
> +            return;
> +        } else if (ret) {
> +            /* With msix=auto, we fall back to MSI off silently */
> +            s->msix = ON_OFF_AUTO_OFF;
> +            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)) {
> -        s->msix = ON_OFF_AUTO_OFF;
> -    }
>      if (pci_is_express(dev)) {
>          pcie_endpoint_cap_init(dev, 0xa0);
>      }

Here, you already propagate.

[Skipping the remainder of the patch for now...]

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

[Cao jin]

On 08/18/2016 03:55 PM, Markus Armbruster wrote:
> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>

>>
>> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
>> index 40a2ebc..b8511ee 100644
>> --- a/hw/misc/ivshmem.c
>> +++ b/hw/misc/ivshmem.c
>> @@ -899,6 +899,7 @@ static void ivshmem_common_realize(PCIDevice *dev, Error **errp)
>>
>>           if (ivshmem_setup_interrupts(s) < 0) {
>>               error_setg(errp, "failed to initialize interrupts");
>> +            error_append_hint(errp, "MSI-X is not supported by interrupt controller");
>>               return;
>>           }
>>       }
>
> How is this related to the stated purpose of the patch?
>

Because I don't propagate error via msix_init_exclusive_bar(), so add 
this to show the detail error message to user.(Please also see my 
comment on msix_init_exclusive_bar(), then come back to this comment)

>> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
>> index d001c96..82a7be1 100644
>> --- a/hw/net/e1000e.c
>> +++ b/hw/net/e1000e.c
>> @@ -295,7 +295,7 @@ e1000e_init_msix(E1000EState *s)
>         int res = msix_init(PCI_DEVICE(s), E1000E_MSIX_VEC_NUM,
>                             &s->msix,
>>                           E1000E_MSIX_IDX, E1000E_MSIX_TABLE,
>>                           &s->msix,
>>                           E1000E_MSIX_IDX, E1000E_MSIX_PBA,
>> -                        0xA0);
>> +                        0xA0, NULL);
>
> Further down, you convert msix_init() from error_report() to
> error_setg().  It now relies on its callers to report errors.  However,
> this caller doesn't.  Suppressing error reports like that may be
> appropriate, since the function doesn't fail.  But such a change
> shouldn't be hidden in a larger patch without at least a mention in the
> commit message.
>

For this device, I was planning 1)make this patch as small as possible 
for reviewer's convenience sake(so suppressed error report here), then 
2) drop this function as another patch, and then 3) convert this device 
to .realize(), error report or setting error could be placed at one of 
last two steps. For other devices, the plan is basically the same.

> Here's how I'd skin this cat.  First convert msix_init() without
> changing behavior, by having every caller of msix_init() immediately
> pass the error received to error_report_err().  Then clean up the
> callers one after the other.  The ones that are running within a
> realize() method should propagate the error to the realize() method.
> The ones that are still running within an init() method keep the
> error_report_err().  e1000e_init_msix() may want to ignore the error.
> Separaring the cleanups that way lets you explain each actual change
> neatly in a commit message.
>
>>
>>       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..769b1fd 100644
>> --- a/hw/net/rocker/rocker.c
>> +++ b/hw/net/rocker/rocker.c
>> @@ -1262,7 +1262,7 @@ static int rocker_msix_init(Rocker *r)
>         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, NULL);
>>       if (err) {
>>           return err;
>>       }
>
> This one runs in an init() method.  You're losing an error message here.

Indeed... planned to propagate or set error object when convert the 
device to realize because the only failure is -ENOTSUP

>> @@ -336,7 +340,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, NULL);
>>       if (ret) {
>>           return ret;
>>       }
>
> This is a case where you have to propagate the error.  First step:
> convert msix_exclusive_bar() to Error, too.

I was thinking: all devices call msix_init_exclusive_bar() will only see 
-ENOTSUP on failure, so, to make this patch short and simple, we could 
set error or report error in the caller(or caller's caller, that is what 
I have done in ivshmem_common_realize() at the top of this patch).

>
>> @@ -445,8 +449,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;
>
> Let's drop this hunk.
>

Sorry, I forget to make the coding style changes into a separated one.

-- 
Yours Sincerely,

Cao jin

[Qemu-devel] [PATCH 3/6] e1000e: fix for migration compatibility

[Cao jin]

commit 66bf7d58 removed internal msi state flag E1000E_USE_MSI, but it
is used by intr_state which exists in vmstate. Restore it for migration
to older QEMU versions

CC: Dmitry Fleytman <dmitry@daynix.com>
CC: Jason Wang <jasowang@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/net/e1000e.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
index 82a7be1..ba37fe9 100644
--- a/hw/net/e1000e.c
+++ b/hw/net/e1000e.c
@@ -89,7 +89,8 @@ typedef struct E1000EState {
 #define E1000E_MSIX_TABLE   (0x0000)
 #define E1000E_MSIX_PBA     (0x2000)
 
-#define E1000E_USE_MSIX    BIT(0)
+#define E1000E_USE_MSI     BIT(0)
+#define E1000E_USE_MSIX    BIT(1)
 
 static uint64_t
 e1000e_mmio_read(void *opaque, hwaddr addr, unsigned size)
@@ -470,6 +471,8 @@ static void e1000e_pci_realize(PCIDevice *pci_dev, Error **errp)
     ret = msi_init(PCI_DEVICE(s), 0xD0, 1, true, false, NULL);
     if (ret) {
         trace_e1000e_msi_init_fail(ret);
+    } else {
+        s->intr_state |= E1000E_USE_MSI;
     }
 
     if (e1000e_add_pm_capability(pci_dev, e1000e_pmrb_offset,
-- 
2.1.0

Re: [Qemu-devel] [PATCH 3/6] e1000e: fix for migration compatibility

[Dmitry Fleytman]

Acked-by: Dmitry Fleytman <dmitry@daynix.com>

> On 17 Aug 2016, at 17:39, Cao jin <caoj.fnst@cn.fujitsu.com> wrote:
> 
> commit 66bf7d58 removed internal msi state flag E1000E_USE_MSI, but it
> is used by intr_state which exists in vmstate. Restore it for migration
> to older QEMU versions
> 
> CC: Dmitry Fleytman <dmitry@daynix.com>
> CC: Jason Wang <jasowang@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/net/e1000e.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
> index 82a7be1..ba37fe9 100644
> --- a/hw/net/e1000e.c
> +++ b/hw/net/e1000e.c
> @@ -89,7 +89,8 @@ typedef struct E1000EState {
> #define E1000E_MSIX_TABLE   (0x0000)
> #define E1000E_MSIX_PBA     (0x2000)
> 
> -#define E1000E_USE_MSIX    BIT(0)
> +#define E1000E_USE_MSI     BIT(0)
> +#define E1000E_USE_MSIX    BIT(1)
> 
> static uint64_t
> e1000e_mmio_read(void *opaque, hwaddr addr, unsigned size)
> @@ -470,6 +471,8 @@ static void e1000e_pci_realize(PCIDevice *pci_dev, Error **errp)
>     ret = msi_init(PCI_DEVICE(s), 0xD0, 1, true, false, NULL);
>     if (ret) {
>         trace_e1000e_msi_init_fail(ret);
> +    } else {
> +        s->intr_state |= E1000E_USE_MSI;
>     }
> 
>     if (e1000e_add_pm_capability(pci_dev, e1000e_pmrb_offset,
> -- 
> 2.1.0
> 
> 
> 

Re: [Qemu-devel] [PATCH 3/6] e1000e: fix for migration compatibility

[Paolo Bonzini]

On 17/08/2016 16:39, Cao jin wrote:
> commit 66bf7d58 removed internal msi state flag E1000E_USE_MSI, but it
> is used by intr_state which exists in vmstate. Restore it for migration
> to older QEMU versions
> 
> CC: Dmitry Fleytman <dmitry@daynix.com>
> CC: Jason Wang <jasowang@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>

Not necessary.  No released version of QEMU had e1000e and lacked commit
66bf7d58.

Paolo

>  hw/net/e1000e.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
> index 82a7be1..ba37fe9 100644
> --- a/hw/net/e1000e.c
> +++ b/hw/net/e1000e.c
> @@ -89,7 +89,8 @@ typedef struct E1000EState {
>  #define E1000E_MSIX_TABLE   (0x0000)
>  #define E1000E_MSIX_PBA     (0x2000)
>  
> -#define E1000E_USE_MSIX    BIT(0)
> +#define E1000E_USE_MSI     BIT(0)
> +#define E1000E_USE_MSIX    BIT(1)
>  
>  static uint64_t
>  e1000e_mmio_read(void *opaque, hwaddr addr, unsigned size)
> @@ -470,6 +471,8 @@ static void e1000e_pci_realize(PCIDevice *pci_dev, Error **errp)
>      ret = msi_init(PCI_DEVICE(s), 0xD0, 1, true, false, NULL);
>      if (ret) {
>          trace_e1000e_msi_init_fail(ret);
> +    } else {
> +        s->intr_state |= E1000E_USE_MSI;
>      }
>  
>      if (e1000e_add_pm_capability(pci_dev, e1000e_pmrb_offset,
> 

Re: [Qemu-devel] [PATCH 3/6] e1000e: fix for migration compatibility

[Cao jin]

On 08/18/2016 06:47 PM, Paolo Bonzini wrote:
>
>
> On 17/08/2016 16:39, Cao jin wrote:
>> commit 66bf7d58 removed internal msi state flag E1000E_USE_MSI, but it
>> is used by intr_state which exists in vmstate. Restore it for migration
>> to older QEMU versions
>>
>> CC: Dmitry Fleytman <dmitry@daynix.com>
>> CC: Jason Wang <jasowang@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>
>
> Not necessary.  No released version of QEMU had e1000e and lacked commit
> 66bf7d58.
>
> Paolo
>

Ok, then I will make this patch as separated one and send it out asap, 
so maybe it goes in 2.7

-- 
Yours Sincerely,

Cao jin

Re: [Qemu-devel] [PATCH 3/6] e1000e: fix for migration compatibility

[Paolo Bonzini]

On 18/08/2016 15:11, Cao jin wrote:
> 
> 
> On 08/18/2016 06:47 PM, Paolo Bonzini wrote:
>>
>>
>> On 17/08/2016 16:39, Cao jin wrote:
>>> commit 66bf7d58 removed internal msi state flag E1000E_USE_MSI, but it
>>> is used by intr_state which exists in vmstate. Restore it for migration
>>> to older QEMU versions
>>>
>>> CC: Dmitry Fleytman <dmitry@daynix.com>
>>> CC: Jason Wang <jasowang@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>
>>
>> Not necessary.  No released version of QEMU had e1000e and lacked commit
>> 66bf7d58.
>>
>> Paolo
>>
> 
> Ok, then I will make this patch as separated one and send it out asap,
> so maybe it goes in 2.7

It's not necessary at all; why would it be useful in 2.7?

Paolo

Re: [Qemu-devel] [PATCH 3/6] e1000e: fix for migration compatibility

[Cao jin]

On 08/18/2016 09:04 PM, Paolo Bonzini wrote:
>
>
> On 18/08/2016 15:11, Cao jin wrote:
>>
>>
>> On 08/18/2016 06:47 PM, Paolo Bonzini wrote:
>>>
>>>
>>> On 17/08/2016 16:39, Cao jin wrote:
>>>> commit 66bf7d58 removed internal msi state flag E1000E_USE_MSI, but it
>>>> is used by intr_state which exists in vmstate. Restore it for migration
>>>> to older QEMU versions
>>>>
>>>> CC: Dmitry Fleytman <dmitry@daynix.com>
>>>> CC: Jason Wang <jasowang@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>
>>>
>>> Not necessary.  No released version of QEMU had e1000e and lacked commit
>>> 66bf7d58.
>>>
>>> Paolo
>>>
>>
>> Ok, then I will make this patch as separated one and send it out asap,
>> so maybe it goes in 2.7
>
> It's not necessary at all; why would it be useful in 2.7?
>
> Paolo
>

commit 66bf7d58I already removed E1000E_USE_MSI, so I think maybe I can 
send a patch to remove E1000E_USE_MSIX & intr_state, so there will be no 
migration compatibility issue

Re: [Qemu-devel] [PATCH 3/6] e1000e: fix for migration compatibility

[Paolo Bonzini]

On 18/08/2016 15:25, Cao jin wrote:
> 
> 
> On 08/18/2016 09:04 PM, Paolo Bonzini wrote:
>>
>>
>> On 18/08/2016 15:11, Cao jin wrote:
>>>
>>>
>>> On 08/18/2016 06:47 PM, Paolo Bonzini wrote:
>>>>
>>>>
>>>> On 17/08/2016 16:39, Cao jin wrote:
>>>>> commit 66bf7d58 removed internal msi state flag E1000E_USE_MSI, but it
>>>>> is used by intr_state which exists in vmstate. Restore it for
>>>>> migration
>>>>> to older QEMU versions
>>>>>
>>>>> CC: Dmitry Fleytman <dmitry@daynix.com>
>>>>> CC: Jason Wang <jasowang@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>
>>>>
>>>> Not necessary.  No released version of QEMU had e1000e and lacked
>>>> commit
>>>> 66bf7d58.
>>>>
>>>> Paolo
>>>>
>>>
>>> Ok, then I will make this patch as separated one and send it out asap,
>>> so maybe it goes in 2.7
>>
>> It's not necessary at all; why would it be useful in 2.7?
>>
>> Paolo
>>
> 
> commit 66bf7d58I already removed E1000E_USE_MSI, so I think maybe I can
> send a patch to remove E1000E_USE_MSIX & intr_state, so there will be no
> migration compatibility issue

Please do, worst case it won't be accepted.

Paolo

[Qemu-devel] [PATCH 4/6] e1000e: drop unnecessary funtions

[Cao jin]

Internal helper function: e1000e_init_msix(), e1000e_cleanup_msix()
is unnecessary, remove them all. MSI-X state flag is used by intr_state
which exists in vmstate, keep it for migration compatibility.

CC: Dmitry Fleytman <dmitry@daynix.com>
CC: Jason Wang <jasowang@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/net/e1000e.c | 49 ++++++++++++++++---------------------------------
 1 file changed, 16 insertions(+), 33 deletions(-)

diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
index ba37fe9..4b4eb46 100644
--- a/hw/net/e1000e.c
+++ b/hw/net/e1000e.c
@@ -288,37 +288,6 @@ e1000e_use_msix_vectors(E1000EState *s, int num_vectors)
 }
 
 static void
-e1000e_init_msix(E1000EState *s)
-{
-    PCIDevice *d = PCI_DEVICE(s);
-    int res = msix_init(PCI_DEVICE(s), E1000E_MSIX_VEC_NUM,
-                        &s->msix,
-                        E1000E_MSIX_IDX, E1000E_MSIX_TABLE,
-                        &s->msix,
-                        E1000E_MSIX_IDX, E1000E_MSIX_PBA,
-                        0xA0, NULL);
-
-    if (res < 0) {
-        trace_e1000e_msix_init_fail(res);
-    } else {
-        if (!e1000e_use_msix_vectors(s, E1000E_MSIX_VEC_NUM)) {
-            msix_uninit(d, &s->msix, &s->msix);
-        } else {
-            s->intr_state |= E1000E_USE_MSIX;
-        }
-    }
-}
-
-static void
-e1000e_cleanup_msix(E1000EState *s)
-{
-    if (s->intr_state & E1000E_USE_MSIX) {
-        e1000e_unuse_msix_vectors(s, E1000E_MSIX_VEC_NUM);
-        msix_uninit(PCI_DEVICE(s), &s->msix, &s->msix);
-    }
-}
-
-static void
 e1000e_init_net_peer(E1000EState *s, PCIDevice *pci_dev, uint8_t *macaddr)
 {
     DeviceState *dev = DEVICE(pci_dev);
@@ -462,7 +431,20 @@ static void e1000e_pci_realize(PCIDevice *pci_dev, Error **errp)
     qemu_macaddr_default_if_unset(&s->conf.macaddr);
     macaddr = s->conf.macaddr.a;
 
-    e1000e_init_msix(s);
+    ret = msix_init(pci_dev, E1000E_MSIX_VEC_NUM,
+                    &s->msix,
+                    E1000E_MSIX_IDX, E1000E_MSIX_TABLE,
+                    &s->msix,
+                    E1000E_MSIX_IDX, E1000E_MSIX_PBA,
+                    0xA0, NULL);
+
+    if (ret) {
+        trace_e1000e_msix_init_fail(ret);
+    } else {
+        /* Won't fail, for simplicity, no need to check return value */
+        e1000e_use_msix_vectors(s, E1000E_MSIX_VEC_NUM);
+        s->intr_state |= E1000E_USE_MSIX;
+    }
 
     if (pcie_endpoint_cap_v1_init(pci_dev, e1000e_pcie_offset) < 0) {
         hw_error("Failed to initialize PCIe capability");
@@ -511,7 +493,8 @@ static void e1000e_pci_uninit(PCIDevice *pci_dev)
 
     qemu_del_nic(s->nic);
 
-    e1000e_cleanup_msix(s);
+    e1000e_unuse_msix_vectors(s, E1000E_MSIX_VEC_NUM);
+    msix_uninit(pci_dev, &s->msix, &s->msix);
     msi_uninit(pci_dev);
 }
 
-- 
2.1.0

Re: [Qemu-devel] [PATCH 4/6] e1000e: drop unnecessary funtions

[Dmitry Fleytman]

> On 17 Aug 2016, at 17:39, Cao jin <caoj.fnst@cn.fujitsu.com> wrote:
> 
> Internal helper function: e1000e_init_msix(), e1000e_cleanup_msix()
> is unnecessary, remove them all.

Is there any reason to drop these functions?
They exist to improve code readability and modularisation.

> MSI-X state flag is used by intr_state
> which exists in vmstate, keep it for migration compatibility.
> 
> CC: Dmitry Fleytman <dmitry@daynix.com>
> CC: Jason Wang <jasowang@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/net/e1000e.c | 49 ++++++++++++++++---------------------------------
> 1 file changed, 16 insertions(+), 33 deletions(-)
> 
> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
> index ba37fe9..4b4eb46 100644
> --- a/hw/net/e1000e.c
> +++ b/hw/net/e1000e.c
> @@ -288,37 +288,6 @@ e1000e_use_msix_vectors(E1000EState *s, int num_vectors)
> }
> 
> static void
> -e1000e_init_msix(E1000EState *s)
> -{
> -    PCIDevice *d = PCI_DEVICE(s);
> -    int res = msix_init(PCI_DEVICE(s), E1000E_MSIX_VEC_NUM,
> -                        &s->msix,
> -                        E1000E_MSIX_IDX, E1000E_MSIX_TABLE,
> -                        &s->msix,
> -                        E1000E_MSIX_IDX, E1000E_MSIX_PBA,
> -                        0xA0, NULL);
> -
> -    if (res < 0) {
> -        trace_e1000e_msix_init_fail(res);
> -    } else {
> -        if (!e1000e_use_msix_vectors(s, E1000E_MSIX_VEC_NUM)) {
> -            msix_uninit(d, &s->msix, &s->msix);
> -        } else {
> -            s->intr_state |= E1000E_USE_MSIX;
> -        }
> -    }
> -}
> -
> -static void
> -e1000e_cleanup_msix(E1000EState *s)
> -{
> -    if (s->intr_state & E1000E_USE_MSIX) {
> -        e1000e_unuse_msix_vectors(s, E1000E_MSIX_VEC_NUM);
> -        msix_uninit(PCI_DEVICE(s), &s->msix, &s->msix);
> -    }
> -}
> -
> -static void
> e1000e_init_net_peer(E1000EState *s, PCIDevice *pci_dev, uint8_t *macaddr)
> {
>     DeviceState *dev = DEVICE(pci_dev);
> @@ -462,7 +431,20 @@ static void e1000e_pci_realize(PCIDevice *pci_dev, Error **errp)
>     qemu_macaddr_default_if_unset(&s->conf.macaddr);
>     macaddr = s->conf.macaddr.a;
> 
> -    e1000e_init_msix(s);
> +    ret = msix_init(pci_dev, E1000E_MSIX_VEC_NUM,
> +                    &s->msix,
> +                    E1000E_MSIX_IDX, E1000E_MSIX_TABLE,
> +                    &s->msix,
> +                    E1000E_MSIX_IDX, E1000E_MSIX_PBA,
> +                    0xA0, NULL);
> +
> +    if (ret) {
> +        trace_e1000e_msix_init_fail(ret);
> +    } else {
> +        /* Won't fail, for simplicity, no need to check return value */
> +        e1000e_use_msix_vectors(s, E1000E_MSIX_VEC_NUM);
> +        s->intr_state |= E1000E_USE_MSIX;
> +    }
> 
>     if (pcie_endpoint_cap_v1_init(pci_dev, e1000e_pcie_offset) < 0) {
>         hw_error("Failed to initialize PCIe capability");
> @@ -511,7 +493,8 @@ static void e1000e_pci_uninit(PCIDevice *pci_dev)
> 
>     qemu_del_nic(s->nic);
> 
> -    e1000e_cleanup_msix(s);
> +    e1000e_unuse_msix_vectors(s, E1000E_MSIX_VEC_NUM);
> +    msix_uninit(pci_dev, &s->msix, &s->msix);
>     msi_uninit(pci_dev);
> }
> 
> -- 
> 2.1.0
> 
> 
> 

Re: [Qemu-devel] [PATCH 4/6] e1000e: drop unnecessary funtions

[Cao jin]

On 08/18/2016 01:23 PM, Dmitry Fleytman wrote:
>
>
>> On 17 Aug 2016, at 17:39, Cao jin <caoj.fnst@cn.fujitsu.com> wrote:
>>
>> Internal helper function: e1000e_init_msix(), e1000e_cleanup_msix()
>> is unnecessary, remove them all.
>
> Is there any reason to drop these functions?
> They exist to improve code readability and modularisation.
>

previous commit 66bf7d58d removed the corresponding function of msi, so,
keep the consistency is the reason

-- 
Yours Sincerely,

Cao jin

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

[Cao jin]

megasas overwrites user configuration when msix_init() fail,
to indicate internal msi state, which is unsuitable.
And megasa_use_msix() is unnecessary, so remove it.

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 | 26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)

diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 61efb0f..bde408d 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)
@@ -2362,11 +2355,14 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
             object_unref(OBJECT(&s->mmio_io));
             error_propagate(errp, err);
             return;
-        } else if (ret) {
-            /* With msix=auto, we fall back to MSI off silently */
-            s->msix = ON_OFF_AUTO_OFF;
-            error_free(err);
         }
+        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,
@@ -2384,10 +2380,6 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
     pci_register_bar(dev, b->mmio_bar, bar_type, &s->mmio_io);
     pci_register_bar(dev, 3, bar_type, &s->queue_io);
 
-    if (megasas_use_msix(s)) {
-        msix_vector_use(dev, 0);
-    }
-
     s->fw_state = MFI_FWSTATE_READY;
     if (!s->sas_addr) {
         s->sas_addr = ((NAA_LOCALLY_ASSIGNED_ID << 24) |
-- 
2.1.0

[Qemu-devel] [PATCH 6/6] megasas: undo the overwrites of 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 bde408d..6f73217 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