[Qemu-devel] [PATCH v7 00/15] Add param Error ** for msi_init()

[Qemu-devel] [PATCH v7 00/15] Add param Error ** for msi_init()

[Cao jin]

v7 changelog:
1. 4 new patches, patch 11, 13, 14, 15. previous patch 11 numbered 12 now.
2. patch 2: remove comment of "errp", add it in patch 12
3. patch 4: fix commit message as sugguestion
4. replace all "msi == ON_OFF_AUTO_ON || msi == ON_OFF_AUTO_AUTO" to
   "msi != ON_OFF_AUTO_OFF", before patch 12
5. patch 7: correct a error in intel_hda_update_irq(), and change the
   variant type from int to bool
6. patch 13: fix the issue that code would overwrite user configuration.
   patch 14&15 actually almost the same with 13, remove unnecessary flag.
7. patch 12 use tightened error checking as suggestions, but not sure
   megasas/mptsas is modified as sugguestion perfectly, RFC.

cc: Gerd Hoffmann <kraxel@redhat.com>
cc: John Snow <jsnow@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 (15):
  pci core: assert ENOSPC when add capability
  fix some coding style problems
  change pvscsi_init_msi() type to void
  megasas: Fix check for msi_init() failure
  mptsas: change .realize function name
  usb xhci: change msi/msix property type
  intel-hda: change msi property type
  mptsas: change msi property type
  megasas: change msi/msix property type
  pci bridge dev: change msi property type
  msi_init: change return value to 0 on success
  pci: Convert msi_init() to Error and fix callers to check it
  megasas: remove unnecessary megasas_use_msi()
  mptsas: remove unnecessary internal msi state flag
  vmw_pvscsi: remove unnecessary internal msi state flag

 hw/audio/intel-hda.c               | 30 ++++++++++++++++----
 hw/ide/ich.c                       | 15 ++++++----
 hw/net/vmxnet3.c                   | 40 +++++++++-----------------
 hw/pci-bridge/ioh3420.c            | 13 +++++++--
 hw/pci-bridge/pci_bridge_dev.c     | 35 +++++++++++++++++------
 hw/pci-bridge/xio3130_downstream.c | 12 ++++++--
 hw/pci-bridge/xio3130_upstream.c   |  9 +++++-
 hw/pci/msi.c                       | 27 ++++++++++++++++--
 hw/pci/pci.c                       |  6 ++--
 hw/scsi/megasas.c                  | 58 ++++++++++++++++++++------------------
 hw/scsi/mptsas.c                   | 40 +++++++++++++++++---------
 hw/scsi/mptsas.h                   |  5 ++--
 hw/scsi/vmw_pvscsi.c               | 22 +++++----------
 hw/usb/hcd-xhci.c                  | 35 +++++++++++++++++------
 hw/vfio/pci.c                      |  7 +++--
 include/hw/pci/msi.h               |  3 +-
 16 files changed, 226 insertions(+), 131 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH v7 01/15] pci core: assert ENOSPC when add capability

[Cao jin]

ENOSPC is programming error, assert it for debugging.

cc: Michael S. Tsirkin <mst@redhat.com>
cc: Marcel Apfelbaum <marcel@redhat.com>
cc: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/pci/pci.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 06d3117..26f387c 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2152,10 +2152,8 @@ int pci_add_capability2(PCIDevice *pdev, uint8_t cap_id,
 
     if (!offset) {
         offset = pci_find_space(pdev, size);
-        if (!offset) {
-            error_setg(errp, "out of PCI config space");
-            return -ENOSPC;
-        }
+        /* out of PCI config space is programming error */
+        assert(offset);
     } else {
         /* Verify that capabilities don't overlap.  Note: device assignment
          * depends on this check to verify that the device is not broken.
-- 
2.1.0

[Qemu-devel] [PATCH v7 02/15] fix some coding style problems

[Cao jin]

It has:
1. More newlines make the code block well separated.
2. Add more comments for msi_init.
3. Fix a indentation in vmxnet3.c.
4. ioh3420 & xio3130_downstream: put PCI Express capability init function
   together, make it more readable.

cc: Michael S. Tsirkin <mst@redhat.com>
cc: Markus Armbruster <armbru@redhat.com>
cc: Marcel Apfelbaum <marcel@redhat.com>
cc: Dmitry Fleytman <dmitry@daynix.com>
cc: Jason Wang <jasowang@redhat.com>

Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/net/vmxnet3.c                   |  2 +-
 hw/pci-bridge/ioh3420.c            |  7 ++++++-
 hw/pci-bridge/pci_bridge_dev.c     |  4 ++++
 hw/pci-bridge/xio3130_downstream.c |  6 +++++-
 hw/pci-bridge/xio3130_upstream.c   |  3 +++
 hw/pci/msi.c                       | 16 ++++++++++++++++
 6 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 093a71e..7a38e47 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -348,7 +348,7 @@ typedef struct {
 /* Interrupt management */
 
 /*
- *This function returns sign whether interrupt line is in asserted state
+ * This function returns sign whether interrupt line is in asserted state
  * This depends on the type of interrupt used. For INTX interrupt line will
  * be asserted until explicit deassertion, for MSI(X) interrupt line will
  * be deasserted automatically due to notification semantics of the MSI(X)
diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
index 0937fa3..b4a7806 100644
--- a/hw/pci-bridge/ioh3420.c
+++ b/hw/pci-bridge/ioh3420.c
@@ -106,12 +106,14 @@ static int ioh3420_initfn(PCIDevice *d)
     if (rc < 0) {
         goto err_bridge;
     }
+
     rc = msi_init(d, IOH_EP_MSI_OFFSET, IOH_EP_MSI_NR_VECTOR,
                   IOH_EP_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_64BIT,
                   IOH_EP_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT);
     if (rc < 0) {
         goto err_bridge;
     }
+
     rc = pcie_cap_init(d, IOH_EP_EXP_OFFSET, PCI_EXP_TYPE_ROOT_PORT, p->port);
     if (rc < 0) {
         goto err_msi;
@@ -120,18 +122,21 @@ static int ioh3420_initfn(PCIDevice *d)
     pcie_cap_arifwd_init(d);
     pcie_cap_deverr_init(d);
     pcie_cap_slot_init(d, s->slot);
+    pcie_cap_root_init(d);
+
     pcie_chassis_create(s->chassis);
     rc = pcie_chassis_add_slot(s);
     if (rc < 0) {
         goto err_pcie_cap;
     }
-    pcie_cap_root_init(d);
+
     rc = pcie_aer_init(d, IOH_EP_AER_OFFSET, PCI_ERR_SIZEOF);
     if (rc < 0) {
         goto err;
     }
     pcie_aer_root_init(d);
     ioh3420_aer_vector_update(d);
+
     return 0;
 
 err:
diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index 7b582e9..41ca47b 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -68,10 +68,12 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
         /* MSI is not applicable without SHPC */
         bridge_dev->flags &= ~(1 << PCI_BRIDGE_DEV_F_MSI_REQ);
     }
+
     err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0);
     if (err) {
         goto slotid_error;
     }
+
     if ((bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_MSI_REQ)) &&
         msi_nonbroken) {
         err = msi_init(dev, 0, 1, true, true);
@@ -79,6 +81,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
             goto msi_error;
         }
     }
+
     if (shpc_present(dev)) {
         /* TODO: spec recommends using 64 bit prefetcheable BAR.
          * Check whether that works well. */
@@ -86,6 +89,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
                          PCI_BASE_ADDRESS_MEM_TYPE_64, &bridge_dev->bar);
     }
     return 0;
+
 msi_error:
     slotid_cap_cleanup(dev);
 slotid_error:
diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
index cf1ee63..e6d653d 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -70,11 +70,13 @@ static int xio3130_downstream_initfn(PCIDevice *d)
     if (rc < 0) {
         goto err_bridge;
     }
+
     rc = pci_bridge_ssvid_init(d, XIO3130_SSVID_OFFSET,
                                XIO3130_SSVID_SVID, XIO3130_SSVID_SSID);
     if (rc < 0) {
         goto err_bridge;
     }
+
     rc = pcie_cap_init(d, XIO3130_EXP_OFFSET, PCI_EXP_TYPE_DOWNSTREAM,
                        p->port);
     if (rc < 0) {
@@ -83,12 +85,14 @@ static int xio3130_downstream_initfn(PCIDevice *d)
     pcie_cap_flr_init(d);
     pcie_cap_deverr_init(d);
     pcie_cap_slot_init(d, s->slot);
+    pcie_cap_arifwd_init(d);
+
     pcie_chassis_create(s->chassis);
     rc = pcie_chassis_add_slot(s);
     if (rc < 0) {
         goto err_pcie_cap;
     }
-    pcie_cap_arifwd_init(d);
+
     rc = pcie_aer_init(d, XIO3130_AER_OFFSET, PCI_ERR_SIZEOF);
     if (rc < 0) {
         goto err;
diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c
index 164ef58..d976844 100644
--- a/hw/pci-bridge/xio3130_upstream.c
+++ b/hw/pci-bridge/xio3130_upstream.c
@@ -66,11 +66,13 @@ static int xio3130_upstream_initfn(PCIDevice *d)
     if (rc < 0) {
         goto err_bridge;
     }
+
     rc = pci_bridge_ssvid_init(d, XIO3130_SSVID_OFFSET,
                                XIO3130_SSVID_SVID, XIO3130_SSVID_SSID);
     if (rc < 0) {
         goto err_bridge;
     }
+
     rc = pcie_cap_init(d, XIO3130_EXP_OFFSET, PCI_EXP_TYPE_UPSTREAM,
                        p->port);
     if (rc < 0) {
@@ -78,6 +80,7 @@ static int xio3130_upstream_initfn(PCIDevice *d)
     }
     pcie_cap_flr_init(d);
     pcie_cap_deverr_init(d);
+
     rc = pcie_aer_init(d, XIO3130_AER_OFFSET, PCI_ERR_SIZEOF);
     if (rc < 0) {
         goto err;
diff --git a/hw/pci/msi.c b/hw/pci/msi.c
index e0e64c2..aa6cf49 100644
--- a/hw/pci/msi.c
+++ b/hw/pci/msi.c
@@ -165,6 +165,22 @@ bool msi_enabled(const PCIDevice *dev)
          PCI_MSI_FLAGS_ENABLE);
 }
 
+/*
+ * Make PCI device @dev MSI-capable.
+ * Non-zero @offset puts capability MSI at that offset in PCI config
+ * space.
+ * @nr_vectors is the number of MSI vectors (1, 2, 4, 8, 16 or 32).
+ * If @msi64bit, make the device capable of sending a 64-bit message
+ * address.
+ * If @msi_per_vector_mask, make the device support per-vector masking.
+ * Return the offset of capability MSI in config space on success,
+ * return -errno on error.
+ *
+ * -ENOTSUP means lacking msi support for a msi-capable platform.
+ * -EINVAL means capability overlap, happens when @offset is non-zero,
+ *  also means a programming error, except device assignment, which can check
+ *  if a real HW is broken.
+ */
 int msi_init(struct PCIDevice *dev, uint8_t offset,
              unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask)
 {
-- 
2.1.0

[Qemu-devel] [PATCH v7 03/15] change pvscsi_init_msi() type to void

[Cao jin]

Nobody use its return value, so change the type to void.

cc: Paolo Bonzini <pbonzini@redhat.com>
cc: Markus Armbruster <armbru@redhat.com>
cc: Marcel Apfelbaum <marcel@redhat.com>

Acked-by: Dmitry Fleytman <dmitry@daynix.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/scsi/vmw_pvscsi.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
index e690b4e..c2a387a 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -1040,7 +1040,7 @@ pvscsi_io_read(void *opaque, hwaddr addr, unsigned size)
 }
 
 
-static bool
+static void
 pvscsi_init_msi(PVSCSIState *s)
 {
     int res;
@@ -1054,8 +1054,6 @@ pvscsi_init_msi(PVSCSIState *s)
     } else {
         s->msi_used = true;
     }
-
-    return s->msi_used;
 }
 
 static void
-- 
2.1.0

[Qemu-devel] [PATCH v7 04/15] megasas: Fix check for msi_init() failure

[Cao jin]

msi_init returns non-zero value on both failure and success.

cc: Paolo Bonzini <pbonzini@redhat.com>
cc: Marcel Apfelbaum <marcel@redhat.com>
cc: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/scsi/megasas.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index a63a581..56fb645 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -2348,7 +2348,7 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
                           "megasas-queue", 0x40000);
 
     if (megasas_use_msi(s) &&
-        msi_init(dev, 0x50, 1, true, false)) {
+        msi_init(dev, 0x50, 1, true, false) < 0) {
         s->flags &= ~MEGASAS_MASK_USE_MSI;
     }
     if (megasas_use_msix(s) &&
-- 
2.1.0

[Qemu-devel] [PATCH v7 05/15] mptsas: change .realize function name

[Cao jin]

All the other devices` .realize function name are xxx_realize, except this one.

cc: Paolo Bonzini <pbonzini@redhat.com>
cc: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/scsi/mptsas.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
index 499c146..1c18c84 100644
--- a/hw/scsi/mptsas.c
+++ b/hw/scsi/mptsas.c
@@ -1270,7 +1270,7 @@ static const struct SCSIBusInfo mptsas_scsi_info = {
     .load_request = mptsas_load_request,
 };
 
-static void mptsas_scsi_init(PCIDevice *dev, Error **errp)
+static void mptsas_scsi_realize(PCIDevice *dev, Error **errp)
 {
     DeviceState *d = DEVICE(dev);
     MPTSASState *s = MPT_SAS(dev);
@@ -1413,7 +1413,7 @@ static void mptsas1068_class_init(ObjectClass *oc, void *data)
     DeviceClass *dc = DEVICE_CLASS(oc);
     PCIDeviceClass *pc = PCI_DEVICE_CLASS(oc);
 
-    pc->realize = mptsas_scsi_init;
+    pc->realize = mptsas_scsi_realize;
     pc->exit = mptsas_scsi_uninit;
     pc->romfile = 0;
     pc->vendor_id = PCI_VENDOR_ID_LSI_LOGIC;
-- 
2.1.0

[Qemu-devel] [PATCH v7 06/15] usb xhci: change msi/msix property type

[Cao jin]

>From bit to enum OnOffAuto

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

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 43ba615..0a5510f 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -461,6 +461,8 @@ struct XHCIState {
     uint32_t numslots;
     uint32_t flags;
     uint32_t max_pstreams_mask;
+    OnOffAuto msi;
+    OnOffAuto msix;
 
     /* Operational Registers */
     uint32_t usbcmd;
@@ -498,9 +500,7 @@ typedef struct XHCIEvRingSeg {
 } XHCIEvRingSeg;
 
 enum xhci_flags {
-    XHCI_FLAG_USE_MSI = 1,
-    XHCI_FLAG_USE_MSI_X,
-    XHCI_FLAG_SS_FIRST,
+    XHCI_FLAG_SS_FIRST = 1,
     XHCI_FLAG_FORCE_PCIE_ENDCAP,
     XHCI_FLAG_ENABLE_STREAMS,
 };
@@ -3648,10 +3648,12 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
         assert(ret >= 0);
     }
 
-    if (xhci_get_flag(xhci, XHCI_FLAG_USE_MSI)) {
+    if (xhci->msi != ON_OFF_AUTO_OFF) {
+        /* TODO check for errors */
         msi_init(dev, 0x70, xhci->numintrs, true, false);
     }
-    if (xhci_get_flag(xhci, XHCI_FLAG_USE_MSI_X)) {
+    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,
@@ -3872,8 +3874,8 @@ static const VMStateDescription vmstate_xhci = {
 };
 
 static Property xhci_properties[] = {
-    DEFINE_PROP_BIT("msi",      XHCIState, flags, XHCI_FLAG_USE_MSI, true),
-    DEFINE_PROP_BIT("msix",     XHCIState, flags, XHCI_FLAG_USE_MSI_X, true),
+    DEFINE_PROP_ON_OFF_AUTO("msi", XHCIState, msi, ON_OFF_AUTO_AUTO),
+    DEFINE_PROP_ON_OFF_AUTO("msix", XHCIState, msix, ON_OFF_AUTO_AUTO),
     DEFINE_PROP_BIT("superspeed-ports-first",
                     XHCIState, flags, XHCI_FLAG_SS_FIRST, true),
     DEFINE_PROP_BIT("force-pcie-endcap", XHCIState, flags,
-- 
2.1.0

[Qemu-devel] [PATCH v7 07/15] intel-hda: change msi property type

[Cao jin]

>From uint32 to enum OnOffAuto. msi_enabled() should be enough to check device
msi state, and change local variant type from int to bool.

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/audio/intel-hda.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
index d372d4a..40a8772 100644
--- a/hw/audio/intel-hda.c
+++ b/hw/audio/intel-hda.c
@@ -187,7 +187,7 @@ struct IntelHDAState {
 
     /* properties */
     uint32_t debug;
-    uint32_t msi;
+    OnOffAuto msi;
     bool old_msi_addr;
 };
 
@@ -255,7 +255,7 @@ static void intel_hda_update_int_sts(IntelHDAState *d)
 
 static void intel_hda_update_irq(IntelHDAState *d)
 {
-    int msi = d->msi && msi_enabled(&d->pci);
+    bool msi = msi_enabled(&d->pci);
     int level;
 
     intel_hda_update_int_sts(d);
@@ -1142,7 +1142,8 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp)
     memory_region_init_io(&d->mmio, OBJECT(d), &intel_hda_mmio_ops, d,
                           "intel-hda", 0x4000);
     pci_register_bar(&d->pci, 0, 0, &d->mmio);
-    if (d->msi) {
+    if (d->msi != ON_OFF_AUTO_OFF) {
+         /* TODO check for errors */
         msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, 1, true, false);
     }
 
@@ -1234,7 +1235,7 @@ static const VMStateDescription vmstate_intel_hda = {
 
 static Property intel_hda_properties[] = {
     DEFINE_PROP_UINT32("debug", IntelHDAState, debug, 0),
-    DEFINE_PROP_UINT32("msi", IntelHDAState, msi, 1),
+    DEFINE_PROP_ON_OFF_AUTO("msi", IntelHDAState, msi, ON_OFF_AUTO_AUTO),
     DEFINE_PROP_BOOL("old_msi_addr", IntelHDAState, old_msi_addr, false),
     DEFINE_PROP_END_OF_LIST(),
 };
-- 
2.1.0

[Qemu-devel] [PATCH v7 08/15] mptsas: change msi property type

[Cao jin]

>From uint32 to enum OnOffAuto, and give it a shorter name.

cc: Paolo Bonzini <pbonzini@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/scsi/mptsas.c | 5 +++--
 hw/scsi/mptsas.h | 3 ++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
index 1c18c84..4ff4d06 100644
--- a/hw/scsi/mptsas.c
+++ b/hw/scsi/mptsas.c
@@ -1285,8 +1285,9 @@ static void mptsas_scsi_realize(PCIDevice *dev, Error **errp)
     memory_region_init_io(&s->diag_io, OBJECT(s), &mptsas_diag_ops, s,
                           "mptsas-diag", 0x10000);
 
-    if (s->msi_available &&
+    if (s->msi != ON_OFF_AUTO_OFF &&
         msi_init(dev, 0, 1, true, false) >= 0) {
+        /* TODO check for errors */
         s->msi_in_use = true;
     }
 
@@ -1404,7 +1405,7 @@ static const VMStateDescription vmstate_mptsas = {
 static Property mptsas_properties[] = {
     DEFINE_PROP_UINT64("sas_address", MPTSASState, sas_addr, 0),
     /* TODO: test MSI support under Windows */
-    DEFINE_PROP_BIT("msi", MPTSASState, msi_available, 0, true),
+    DEFINE_PROP_ON_OFF_AUTO("msi", MPTSASState, msi, ON_OFF_AUTO_AUTO),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/scsi/mptsas.h b/hw/scsi/mptsas.h
index 595f81f..0436a33 100644
--- a/hw/scsi/mptsas.h
+++ b/hw/scsi/mptsas.h
@@ -27,7 +27,8 @@ struct MPTSASState {
     MemoryRegion diag_io;
     QEMUBH *request_bh;
 
-    uint32_t msi_available;
+    /* properties */
+    OnOffAuto msi;
     uint64_t sas_addr;
 
     bool msi_in_use;
-- 
2.1.0

[Qemu-devel] [PATCH v7 09/15] megasas: change msi/msix property type

[Cao jin]

>From bit to enum OnOffAuto.

cc: Hannes Reinecke <hare@suse.de>
cc: Paolo Bonzini <pbonzini@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/scsi/megasas.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 56fb645..635be13 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -48,11 +48,7 @@
 
 #define MEGASAS_FLAG_USE_JBOD      0
 #define MEGASAS_MASK_USE_JBOD      (1 << MEGASAS_FLAG_USE_JBOD)
-#define MEGASAS_FLAG_USE_MSI       1
-#define MEGASAS_MASK_USE_MSI       (1 << MEGASAS_FLAG_USE_MSI)
-#define MEGASAS_FLAG_USE_MSIX      2
-#define MEGASAS_MASK_USE_MSIX      (1 << MEGASAS_FLAG_USE_MSIX)
-#define MEGASAS_FLAG_USE_QUEUE64   3
+#define MEGASAS_FLAG_USE_QUEUE64   1
 #define MEGASAS_MASK_USE_QUEUE64   (1 << MEGASAS_FLAG_USE_QUEUE64)
 
 static const char *mfi_frame_desc[] = {
@@ -96,6 +92,8 @@ typedef struct MegasasState {
     int busy;
     int diag;
     int adp_reset;
+    OnOffAuto msi;
+    OnOffAuto msix;
 
     MegasasCmd *event_cmd;
     int event_locale;
@@ -159,12 +157,12 @@ static bool megasas_use_queue64(MegasasState *s)
 
 static bool megasas_use_msi(MegasasState *s)
 {
-    return s->flags & MEGASAS_MASK_USE_MSI;
+    return s->msi != ON_OFF_AUTO_OFF;
 }
 
 static bool megasas_use_msix(MegasasState *s)
 {
-    return s->flags & MEGASAS_MASK_USE_MSIX;
+    return s->msix != ON_OFF_AUTO_OFF;
 }
 
 static bool megasas_is_jbod(MegasasState *s)
@@ -2349,12 +2347,12 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
 
     if (megasas_use_msi(s) &&
         msi_init(dev, 0x50, 1, true, false) < 0) {
-        s->flags &= ~MEGASAS_MASK_USE_MSI;
+        s->msi = ON_OFF_AUTO_OFF;
     }
     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->flags &= ~MEGASAS_MASK_USE_MSIX;
+        s->msix = ON_OFF_AUTO_OFF;
     }
     if (pci_is_express(dev)) {
         pcie_endpoint_cap_init(dev, 0xa0);
@@ -2422,10 +2420,8 @@ static Property megasas_properties_gen1[] = {
                        MEGASAS_DEFAULT_FRAMES),
     DEFINE_PROP_STRING("hba_serial", MegasasState, hba_serial),
     DEFINE_PROP_UINT64("sas_address", MegasasState, sas_addr, 0),
-    DEFINE_PROP_BIT("use_msi", MegasasState, flags,
-                    MEGASAS_FLAG_USE_MSI, false),
-    DEFINE_PROP_BIT("use_msix", MegasasState, flags,
-                    MEGASAS_FLAG_USE_MSIX, false),
+    DEFINE_PROP_ON_OFF_AUTO("msi", MegasasState, msi, ON_OFF_AUTO_AUTO),
+    DEFINE_PROP_ON_OFF_AUTO("msix", MegasasState, msix, ON_OFF_AUTO_AUTO),
     DEFINE_PROP_BIT("use_jbod", MegasasState, flags,
                     MEGASAS_FLAG_USE_JBOD, false),
     DEFINE_PROP_END_OF_LIST(),
@@ -2438,10 +2434,8 @@ static Property megasas_properties_gen2[] = {
                        MEGASAS_GEN2_DEFAULT_FRAMES),
     DEFINE_PROP_STRING("hba_serial", MegasasState, hba_serial),
     DEFINE_PROP_UINT64("sas_address", MegasasState, sas_addr, 0),
-    DEFINE_PROP_BIT("use_msi", MegasasState, flags,
-                    MEGASAS_FLAG_USE_MSI, true),
-    DEFINE_PROP_BIT("use_msix", MegasasState, flags,
-                    MEGASAS_FLAG_USE_MSIX, true),
+    DEFINE_PROP_ON_OFF_AUTO("msi", MegasasState, msi, ON_OFF_AUTO_AUTO),
+    DEFINE_PROP_ON_OFF_AUTO("msix", MegasasState, msix, ON_OFF_AUTO_AUTO),
     DEFINE_PROP_BIT("use_jbod", MegasasState, flags,
                     MEGASAS_FLAG_USE_JBOD, false),
     DEFINE_PROP_END_OF_LIST(),
-- 
2.1.0

Re: [Qemu-devel] [PATCH v7 09/15] megasas: change msi/msix property type

[Hannes Reinecke]

On 06/06/2016 10:00 AM, Cao jin wrote:
> From bit to enum OnOffAuto.
> 
> cc: Hannes Reinecke <hare@suse.de>
> cc: Paolo Bonzini <pbonzini@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/scsi/megasas.c | 28 +++++++++++-----------------
>  1 file changed, 11 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index 56fb645..635be13 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -48,11 +48,7 @@
>  
>  #define MEGASAS_FLAG_USE_JBOD      0
>  #define MEGASAS_MASK_USE_JBOD      (1 << MEGASAS_FLAG_USE_JBOD)
> -#define MEGASAS_FLAG_USE_MSI       1
> -#define MEGASAS_MASK_USE_MSI       (1 << MEGASAS_FLAG_USE_MSI)
> -#define MEGASAS_FLAG_USE_MSIX      2
> -#define MEGASAS_MASK_USE_MSIX      (1 << MEGASAS_FLAG_USE_MSIX)
> -#define MEGASAS_FLAG_USE_QUEUE64   3
> +#define MEGASAS_FLAG_USE_QUEUE64   1
>  #define MEGASAS_MASK_USE_QUEUE64   (1 << MEGASAS_FLAG_USE_QUEUE64)
>  
>  static const char *mfi_frame_desc[] = {
> @@ -96,6 +92,8 @@ typedef struct MegasasState {
>      int busy;
>      int diag;
>      int adp_reset;
> +    OnOffAuto msi;
> +    OnOffAuto msix;
>  
>      MegasasCmd *event_cmd;
>      int event_locale;
> @@ -159,12 +157,12 @@ static bool megasas_use_queue64(MegasasState *s)
>  
>  static bool megasas_use_msi(MegasasState *s)
>  {
> -    return s->flags & MEGASAS_MASK_USE_MSI;
> +    return s->msi != ON_OFF_AUTO_OFF;
>  }
>  
>  static bool megasas_use_msix(MegasasState *s)
>  {
> -    return s->flags & MEGASAS_MASK_USE_MSIX;
> +    return s->msix != ON_OFF_AUTO_OFF;
>  }
>  
>  static bool megasas_is_jbod(MegasasState *s)
> @@ -2349,12 +2347,12 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
>  
>      if (megasas_use_msi(s) &&
>          msi_init(dev, 0x50, 1, true, false) < 0) {
> -        s->flags &= ~MEGASAS_MASK_USE_MSI;
> +        s->msi = ON_OFF_AUTO_OFF;
>      }
>      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->flags &= ~MEGASAS_MASK_USE_MSIX;
> +        s->msix = ON_OFF_AUTO_OFF;
>      }
>      if (pci_is_express(dev)) {
>          pcie_endpoint_cap_init(dev, 0xa0);
> @@ -2422,10 +2420,8 @@ static Property megasas_properties_gen1[] = {
>                         MEGASAS_DEFAULT_FRAMES),
>      DEFINE_PROP_STRING("hba_serial", MegasasState, hba_serial),
>      DEFINE_PROP_UINT64("sas_address", MegasasState, sas_addr, 0),
> -    DEFINE_PROP_BIT("use_msi", MegasasState, flags,
> -                    MEGASAS_FLAG_USE_MSI, false),
> -    DEFINE_PROP_BIT("use_msix", MegasasState, flags,
> -                    MEGASAS_FLAG_USE_MSIX, false),
> +    DEFINE_PROP_ON_OFF_AUTO("msi", MegasasState, msi, ON_OFF_AUTO_AUTO),
> +    DEFINE_PROP_ON_OFF_AUTO("msix", MegasasState, msix, ON_OFF_AUTO_AUTO),
>      DEFINE_PROP_BIT("use_jbod", MegasasState, flags,
>                      MEGASAS_FLAG_USE_JBOD, false),
>      DEFINE_PROP_END_OF_LIST(),
> @@ -2438,10 +2434,8 @@ static Property megasas_properties_gen2[] = {
>                         MEGASAS_GEN2_DEFAULT_FRAMES),
>      DEFINE_PROP_STRING("hba_serial", MegasasState, hba_serial),
>      DEFINE_PROP_UINT64("sas_address", MegasasState, sas_addr, 0),
> -    DEFINE_PROP_BIT("use_msi", MegasasState, flags,
> -                    MEGASAS_FLAG_USE_MSI, true),
> -    DEFINE_PROP_BIT("use_msix", MegasasState, flags,
> -                    MEGASAS_FLAG_USE_MSIX, true),
> +    DEFINE_PROP_ON_OFF_AUTO("msi", MegasasState, msi, ON_OFF_AUTO_AUTO),
> +    DEFINE_PROP_ON_OFF_AUTO("msix", MegasasState, msix, ON_OFF_AUTO_AUTO),
>      DEFINE_PROP_BIT("use_jbod", MegasasState, flags,
>                      MEGASAS_FLAG_USE_JBOD, false),
>      DEFINE_PROP_END_OF_LIST(),
> 
What is the default value of 'ON_OFF_AUTO_AUTO'?
Originally we've disabled MSI-X for gen1, and enabled it for gen2.
Is this behaviour carried over?

Cheers,

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

Re: [Qemu-devel] [PATCH v7 09/15] megasas: change msi/msix property type

[Cao jin]

On 06/06/2016 04:27 PM, Hannes Reinecke wrote:
> On 06/06/2016 10:00 AM, Cao jin wrote:

>> @@ -2422,10 +2420,8 @@ static Property megasas_properties_gen1[] = {
>>                          MEGASAS_DEFAULT_FRAMES),
>>       DEFINE_PROP_STRING("hba_serial", MegasasState, hba_serial),
>>       DEFINE_PROP_UINT64("sas_address", MegasasState, sas_addr, 0),
>> -    DEFINE_PROP_BIT("use_msi", MegasasState, flags,
>> -                    MEGASAS_FLAG_USE_MSI, false),
>> -    DEFINE_PROP_BIT("use_msix", MegasasState, flags,
>> -                    MEGASAS_FLAG_USE_MSIX, false),
>> +    DEFINE_PROP_ON_OFF_AUTO("msi", MegasasState, msi, ON_OFF_AUTO_AUTO),
>> +    DEFINE_PROP_ON_OFF_AUTO("msix", MegasasState, msix, ON_OFF_AUTO_AUTO),
>>       DEFINE_PROP_BIT("use_jbod", MegasasState, flags,
>>                       MEGASAS_FLAG_USE_JBOD, false),
>>       DEFINE_PROP_END_OF_LIST(),
>> @@ -2438,10 +2434,8 @@ static Property megasas_properties_gen2[] = {
>>                          MEGASAS_GEN2_DEFAULT_FRAMES),
>>       DEFINE_PROP_STRING("hba_serial", MegasasState, hba_serial),
>>       DEFINE_PROP_UINT64("sas_address", MegasasState, sas_addr, 0),
>> -    DEFINE_PROP_BIT("use_msi", MegasasState, flags,
>> -                    MEGASAS_FLAG_USE_MSI, true),
>> -    DEFINE_PROP_BIT("use_msix", MegasasState, flags,
>> -                    MEGASAS_FLAG_USE_MSIX, true),
>> +    DEFINE_PROP_ON_OFF_AUTO("msi", MegasasState, msi, ON_OFF_AUTO_AUTO),
>> +    DEFINE_PROP_ON_OFF_AUTO("msix", MegasasState, msix, ON_OFF_AUTO_AUTO),
>>       DEFINE_PROP_BIT("use_jbod", MegasasState, flags,
>>                       MEGASAS_FLAG_USE_JBOD, false),
>>       DEFINE_PROP_END_OF_LIST(),
>>
> What is the default value of 'ON_OFF_AUTO_AUTO'?
> Originally we've disabled MSI-X for gen1, and enabled it for gen2.
> Is this behaviour carried over?
>

default value of auto is on. Seems the behaviour is not carried over. If 
it must be carried over, gen1 will be the only one exception in all 
devices, which seems little weird. is any special reason for gen1 to be 
msi/msix-incapable by default?

> Cheers,
>
> Hannes
>

-- 
Yours Sincerely,

Cao jin

[Qemu-devel] [PATCH v7 10/15] pci bridge dev: change msi property type

[Cao jin]

>From bit to enum OnOffAuto.

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/pci-bridge/pci_bridge_dev.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index 41ca47b..0fbecc4 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -42,9 +42,10 @@ struct PCIBridgeDev {
 
     MemoryRegion bar;
     uint8_t chassis_nr;
-#define PCI_BRIDGE_DEV_F_MSI_REQ 0
-#define PCI_BRIDGE_DEV_F_SHPC_REQ 1
+#define PCI_BRIDGE_DEV_F_SHPC_REQ 0
     uint32_t flags;
+
+    OnOffAuto msi;
 };
 typedef struct PCIBridgeDev PCIBridgeDev;
 
@@ -66,7 +67,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
         }
     } else {
         /* MSI is not applicable without SHPC */
-        bridge_dev->flags &= ~(1 << PCI_BRIDGE_DEV_F_MSI_REQ);
+        bridge_dev->msi = ON_OFF_AUTO_OFF;
     }
 
     err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0);
@@ -74,7 +75,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
         goto slotid_error;
     }
 
-    if ((bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_MSI_REQ)) &&
+    if (bridge_dev->msi != ON_OFF_AUTO_OFF &&
         msi_nonbroken) {
         err = msi_init(dev, 0, 1, true, true);
         if (err < 0) {
@@ -147,8 +148,8 @@ static Property pci_bridge_dev_properties[] = {
                     /* Note: 0 is not a legal chassis number. */
     DEFINE_PROP_UINT8(PCI_BRIDGE_DEV_PROP_CHASSIS_NR, PCIBridgeDev, chassis_nr,
                       0),
-    DEFINE_PROP_BIT(PCI_BRIDGE_DEV_PROP_MSI, PCIBridgeDev, flags,
-                    PCI_BRIDGE_DEV_F_MSI_REQ, true),
+    DEFINE_PROP_ON_OFF_AUTO(PCI_BRIDGE_DEV_PROP_MSI, PCIBridgeDev, msi,
+                            ON_OFF_AUTO_AUTO),
     DEFINE_PROP_BIT(PCI_BRIDGE_DEV_PROP_SHPC, PCIBridgeDev, flags,
                     PCI_BRIDGE_DEV_F_SHPC_REQ, true),
     DEFINE_PROP_END_OF_LIST(),
-- 
2.1.0

[Qemu-devel] [PATCH v7 11/15] msi_init: change return value to 0 on success

[Cao jin]

No caller use its return value as msi capability offset,  in order
to make its return behaviour consistent with msix_init().

cc: Michael S. Tsirkin <mst@redhat.com>
cc: Paolo Bonzini <pbonzini@redhat.com>
cc: Hannes Reinecke <hare@suse.de>
cc: Markus Armbruster <armbru@redhat.com>
cc: Marcel Apfelbaum <marcel@redhat.com>

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/pci/msi.c      | 6 +++---
 hw/scsi/megasas.c | 2 +-
 hw/scsi/mptsas.c  | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/pci/msi.c b/hw/pci/msi.c
index aa6cf49..70464bf 100644
--- a/hw/pci/msi.c
+++ b/hw/pci/msi.c
@@ -173,8 +173,7 @@ bool msi_enabled(const PCIDevice *dev)
  * If @msi64bit, make the device capable of sending a 64-bit message
  * address.
  * If @msi_per_vector_mask, make the device support per-vector masking.
- * Return the offset of capability MSI in config space on success,
- * return -errno on error.
+ * Return 0 on success, return -errno on error.
  *
  * -ENOTSUP means lacking msi support for a msi-capable platform.
  * -EINVAL means capability overlap, happens when @offset is non-zero,
@@ -236,7 +235,8 @@ int msi_init(struct PCIDevice *dev, uint8_t offset,
         pci_set_long(dev->wmask + msi_mask_off(dev, msi64bit),
                      0xffffffff >> (PCI_MSI_VECTORS_MAX - nr_vectors));
     }
-    return config_offset;
+
+    return 0;
 }
 
 void msi_uninit(struct PCIDevice *dev)
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 635be13..26119bf 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -2346,7 +2346,7 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
                           "megasas-queue", 0x40000);
 
     if (megasas_use_msi(s) &&
-        msi_init(dev, 0x50, 1, true, false) < 0) {
+        msi_init(dev, 0x50, 1, true, false)) {
         s->msi = ON_OFF_AUTO_OFF;
     }
     if (megasas_use_msix(s) &&
diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
index 4ff4d06..d4773e2 100644
--- a/hw/scsi/mptsas.c
+++ b/hw/scsi/mptsas.c
@@ -1286,7 +1286,7 @@ static void mptsas_scsi_realize(PCIDevice *dev, Error **errp)
                           "mptsas-diag", 0x10000);
 
     if (s->msi != ON_OFF_AUTO_OFF &&
-        msi_init(dev, 0, 1, true, false) >= 0) {
+        msi_init(dev, 0, 1, true, false) > 0) {
         /* TODO check for errors */
         s->msi_in_use = true;
     }
-- 
2.1.0

Re: [Qemu-devel] [PATCH v7 11/15] msi_init: change return value to 0 on success

[Hannes Reinecke]

On 06/06/2016 10:00 AM, Cao jin wrote:
> No caller use its return value as msi capability offset,  in order
> to make its return behaviour consistent with msix_init().
> 
> cc: Michael S. Tsirkin <mst@redhat.com>
> cc: Paolo Bonzini <pbonzini@redhat.com>
> cc: Hannes Reinecke <hare@suse.de>
> cc: Markus Armbruster <armbru@redhat.com>
> cc: Marcel Apfelbaum <marcel@redhat.com>
> 
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/pci/msi.c      | 6 +++---
>  hw/scsi/megasas.c | 2 +-
>  hw/scsi/mptsas.c  | 2 +-
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/pci/msi.c b/hw/pci/msi.c
> index aa6cf49..70464bf 100644
> --- a/hw/pci/msi.c
> +++ b/hw/pci/msi.c
> @@ -173,8 +173,7 @@ bool msi_enabled(const PCIDevice *dev)
>   * If @msi64bit, make the device capable of sending a 64-bit message
>   * address.
>   * If @msi_per_vector_mask, make the device support per-vector masking.
> - * Return the offset of capability MSI in config space on success,
> - * return -errno on error.
> + * Return 0 on success, return -errno on error.
>   *
>   * -ENOTSUP means lacking msi support for a msi-capable platform.
>   * -EINVAL means capability overlap, happens when @offset is non-zero,
> @@ -236,7 +235,8 @@ int msi_init(struct PCIDevice *dev, uint8_t offset,
>          pci_set_long(dev->wmask + msi_mask_off(dev, msi64bit),
>                       0xffffffff >> (PCI_MSI_VECTORS_MAX - nr_vectors));
>      }
> -    return config_offset;
> +
> +    return 0;
>  }
>  
>  void msi_uninit(struct PCIDevice *dev)
> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index 635be13..26119bf 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -2346,7 +2346,7 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
>                            "megasas-queue", 0x40000);
>  
>      if (megasas_use_msi(s) &&
> -        msi_init(dev, 0x50, 1, true, false) < 0) {
> +        msi_init(dev, 0x50, 1, true, false)) {
>          s->msi = ON_OFF_AUTO_OFF;
>      }
>      if (megasas_use_msix(s) &&
> diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
> index 4ff4d06..d4773e2 100644
> --- a/hw/scsi/mptsas.c
> +++ b/hw/scsi/mptsas.c
> @@ -1286,7 +1286,7 @@ static void mptsas_scsi_realize(PCIDevice *dev, Error **errp)
>                            "mptsas-diag", 0x10000);
>  
>      if (s->msi != ON_OFF_AUTO_OFF &&
> -        msi_init(dev, 0, 1, true, false) >= 0) {
> +        msi_init(dev, 0, 1, true, false) > 0) {
>          /* TODO check for errors */
>          s->msi_in_use = true;
>      }
> 
Acked-by: Hannes Reinecke <hare@suse.com>

Cheers,

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

Re: [Qemu-devel] [PATCH v7 11/15] msi_init: change return value to 0 on success

[Cao jin]

On 06/06/2016 04:28 PM, Hannes Reinecke wrote:
> On 06/06/2016 10:00 AM, Cao jin wrote:

>> diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
>> index 4ff4d06..d4773e2 100644
>> --- a/hw/scsi/mptsas.c
>> +++ b/hw/scsi/mptsas.c
>> @@ -1286,7 +1286,7 @@ static void mptsas_scsi_realize(PCIDevice *dev, Error **errp)
>>                             "mptsas-diag", 0x10000);
>>
>>       if (s->msi != ON_OFF_AUTO_OFF &&
>> -        msi_init(dev, 0, 1, true, false) >= 0) {
>> +        msi_init(dev, 0, 1, true, false) > 0) {

I am so sorry I made the mistake...it should be msi_init() == 0

>>           /* TODO check for errors */
>>           s->msi_in_use = true;
>>       }

-- 
Yours Sincerely,

Cao jin

Re: [Qemu-devel] [PATCH v7 11/15] msi_init: change return value to 0 on success

[Markus Armbruster]

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

> No caller use its return value as msi capability offset,  in order
> to make its return behaviour consistent with msix_init().
>
> cc: Michael S. Tsirkin <mst@redhat.com>
> cc: Paolo Bonzini <pbonzini@redhat.com>
> cc: Hannes Reinecke <hare@suse.de>
> cc: Markus Armbruster <armbru@redhat.com>
> cc: Marcel Apfelbaum <marcel@redhat.com>
>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/pci/msi.c      | 6 +++---
>  hw/scsi/megasas.c | 2 +-
>  hw/scsi/mptsas.c  | 2 +-
>  3 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/hw/pci/msi.c b/hw/pci/msi.c
> index aa6cf49..70464bf 100644
> --- a/hw/pci/msi.c
> +++ b/hw/pci/msi.c
> @@ -173,8 +173,7 @@ bool msi_enabled(const PCIDevice *dev)
>   * If @msi64bit, make the device capable of sending a 64-bit message
>   * address.
>   * If @msi_per_vector_mask, make the device support per-vector masking.
> - * Return the offset of capability MSI in config space on success,
> - * return -errno on error.
> + * Return 0 on success, return -errno on error.
>   *
>   * -ENOTSUP means lacking msi support for a msi-capable platform.
>   * -EINVAL means capability overlap, happens when @offset is non-zero,
> @@ -236,7 +235,8 @@ int msi_init(struct PCIDevice *dev, uint8_t offset,
>          pci_set_long(dev->wmask + msi_mask_off(dev, msi64bit),
>                       0xffffffff >> (PCI_MSI_VECTORS_MAX - nr_vectors));
>      }
> -    return config_offset;
> +
> +    return 0;
>  }
>  
>  void msi_uninit(struct PCIDevice *dev)
> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index 635be13..26119bf 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -2346,7 +2346,7 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
>                            "megasas-queue", 0x40000);
>  
>      if (megasas_use_msi(s) &&
> -        msi_init(dev, 0x50, 1, true, false) < 0) {
> +        msi_init(dev, 0x50, 1, true, false)) {

I'd leave this alone.

>          s->msi = ON_OFF_AUTO_OFF;
>      }
>      if (megasas_use_msix(s) &&
> diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
> index 4ff4d06..d4773e2 100644
> --- a/hw/scsi/mptsas.c
> +++ b/hw/scsi/mptsas.c
> @@ -1286,7 +1286,7 @@ static void mptsas_scsi_realize(PCIDevice *dev, Error **errp)
>                            "mptsas-diag", 0x10000);
>  
>      if (s->msi != ON_OFF_AUTO_OFF &&
> -        msi_init(dev, 0, 1, true, false) >= 0) {
> +        msi_init(dev, 0, 1, true, false) > 0) {
>          /* TODO check for errors */
>          s->msi_in_use = true;
>      }

This one, too.

[Qemu-devel] [PATCH v7 12/15] pci: Convert msi_init() to Error and fix callers to check it

[Cao jin]

msi_init() reports errors with error_report(), which is wrong
when it's used in realize().

Fix by converting it to Error.

Fix its callers to handle failure instead of ignoring it.

For those callers who don't handle the failure, it might happen:
when user want msi on, but he doesn't get what he want because of
msi_init fails silently.

cc: Gerd Hoffmann <kraxel@redhat.com>
cc: John Snow <jsnow@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/audio/intel-hda.c               | 25 +++++++++++++++++++++----
 hw/ide/ich.c                       | 15 +++++++++------
 hw/net/vmxnet3.c                   | 38 +++++++++++++-------------------------
 hw/pci-bridge/ioh3420.c            |  6 +++++-
 hw/pci-bridge/pci_bridge_dev.c     | 20 ++++++++++++++++----
 hw/pci-bridge/xio3130_downstream.c |  6 +++++-
 hw/pci-bridge/xio3130_upstream.c   |  6 +++++-
 hw/pci/msi.c                       | 11 ++++++++---
 hw/scsi/megasas.c                  | 27 ++++++++++++++++++++++-----
 hw/scsi/mptsas.c                   | 31 ++++++++++++++++++++++++-------
 hw/scsi/vmw_pvscsi.c               |  4 +++-
 hw/usb/hcd-xhci.c                  | 23 +++++++++++++++++++----
 hw/vfio/pci.c                      |  7 +++++--
 include/hw/pci/msi.h               |  3 ++-
 14 files changed, 157 insertions(+), 65 deletions(-)

diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
index 40a8772..d588fd5 100644
--- a/hw/audio/intel-hda.c
+++ b/hw/audio/intel-hda.c
@@ -26,6 +26,7 @@
 #include "intel-hda.h"
 #include "intel-hda-defs.h"
 #include "sysemu/dma.h"
+#include "qapi/error.h"
 
 /* --------------------------------------------------------------------- */
 /* hda bus                                                               */
@@ -1131,6 +1132,8 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp)
 {
     IntelHDAState *d = INTEL_HDA(pci);
     uint8_t *conf = d->pci.config;
+    Error *err = NULL;
+    int32_t ret;
 
     d->name = object_get_typename(OBJECT(d));
 
@@ -1139,13 +1142,27 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp)
     /* HDCTL off 0x40 bit 0 selects signaling mode (1-HDA, 0 - Ac97) 18.1.19 */
     conf[0x40] = 0x01;
 
+    if (d->msi != ON_OFF_AUTO_OFF) {
+        ret = msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60,
+                       1, true, false, &err);
+        /* Any error other than -ENOTSUP(board's MSI support is broken)
+         * is a programming error */
+        assert(!ret || ret == -ENOTSUP);
+        if (ret && d->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 || d->msi == ON_OFF_AUTO_AUTO);
+        /* With msi=auto, we fall back to MSI off silently */
+        error_free(err);
+    }
+
     memory_region_init_io(&d->mmio, OBJECT(d), &intel_hda_mmio_ops, d,
                           "intel-hda", 0x4000);
     pci_register_bar(&d->pci, 0, 0, &d->mmio);
-    if (d->msi != ON_OFF_AUTO_OFF) {
-         /* TODO check for errors */
-        msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, 1, true, false);
-    }
 
     hda_codec_bus_init(DEVICE(pci), &d->codecs, sizeof(d->codecs),
                        intel_hda_response, intel_hda_xfer);
diff --git a/hw/ide/ich.c b/hw/ide/ich.c
index 0a13334..084bef8 100644
--- a/hw/ide/ich.c
+++ b/hw/ide/ich.c
@@ -68,7 +68,6 @@
 #include <hw/isa/isa.h>
 #include "sysemu/block-backend.h"
 #include "sysemu/dma.h"
-
 #include <hw/ide/pci.h>
 #include <hw/ide/ahci.h>
 
@@ -111,6 +110,15 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp)
     int sata_cap_offset;
     uint8_t *sata_cap;
     d = ICH_AHCI(dev);
+    int ret;
+
+    /* Although the AHCI 1.3 specification states that the first capability
+     * should be PMCAP, the Intel ICH9 data sheet specifies that the ICH9
+     * AHCI device puts the MSI capability first, pointing to 0x80. */
+    ret = msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false, 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);
 
     ahci_realize(&d->ahci, DEVICE(dev), pci_get_address_space(dev), 6);
 
@@ -142,11 +150,6 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp)
     pci_set_long(sata_cap + SATA_CAP_BAR,
                  (ICH9_IDP_BAR + 0x4) | (ICH9_IDP_INDEX_LOG2 << 4));
     d->ahci.idp_offset = ICH9_IDP_INDEX;
-
-    /* Although the AHCI 1.3 specification states that the first capability
-     * should be PMCAP, the Intel ICH9 data sheet specifies that the ICH9
-     * AHCI device puts the MSI capability first, pointing to 0x80. */
-    msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false);
 }
 
 static void pci_ich9_uninit(PCIDevice *dev)
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 7a38e47..a331615 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -32,6 +32,7 @@
 #include "vmware_utils.h"
 #include "vmxnet_tx_pkt.h"
 #include "vmxnet_rx_pkt.h"
+#include "qapi/error.h"
 
 #define PCI_DEVICE_ID_VMWARE_VMXNET3_REVISION 0x1
 #define VMXNET3_MSIX_BAR_SIZE 0x2000
@@ -2189,27 +2190,6 @@ vmxnet3_cleanup_msix(VMXNET3State *s)
     }
 }
 
-#define VMXNET3_USE_64BIT         (true)
-#define VMXNET3_PER_VECTOR_MASK   (false)
-
-static bool
-vmxnet3_init_msi(VMXNET3State *s)
-{
-    PCIDevice *d = PCI_DEVICE(s);
-    int res;
-
-    res = msi_init(d, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS,
-                   VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK);
-    if (0 > res) {
-        VMW_WRPRN("Failed to initialize MSI, error %d", res);
-        s->msi_used = false;
-    } else {
-        s->msi_used = true;
-    }
-
-    return s->msi_used;
-}
-
 static void
 vmxnet3_cleanup_msi(VMXNET3State *s)
 {
@@ -2271,10 +2251,15 @@ static uint8_t *vmxnet3_device_serial_num(VMXNET3State *s)
     return dsnp;
 }
 
+
+#define VMXNET3_USE_64BIT         (true)
+#define VMXNET3_PER_VECTOR_MASK   (false)
+
 static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
 {
     DeviceState *dev = DEVICE(pci_dev);
     VMXNET3State *s = VMXNET3(pci_dev);
+    int ret;
 
     VMW_CBPRN("Starting init...");
 
@@ -2298,14 +2283,17 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
     /* Interrupt pin A */
     pci_dev->config[PCI_INTERRUPT_PIN] = 0x01;
 
+    ret = msi_init(pci_dev, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS,
+                   VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK, 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->msi_used = !ret;
+
     if (!vmxnet3_init_msix(s)) {
         VMW_WRPRN("Failed to initialize MSI-X, configuration is inconsistent.");
     }
 
-    if (!vmxnet3_init_msi(s)) {
-        VMW_WRPRN("Failed to initialize MSI, configuration is inconsistent.");
-    }
-
     vmxnet3_net_init(s);
 
     if (pci_is_express(pci_dev)) {
diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
index b4a7806..93c6f0b 100644
--- a/hw/pci-bridge/ioh3420.c
+++ b/hw/pci-bridge/ioh3420.c
@@ -25,6 +25,7 @@
 #include "hw/pci/msi.h"
 #include "hw/pci/pcie.h"
 #include "ioh3420.h"
+#include "qapi/error.h"
 
 #define PCI_DEVICE_ID_IOH_EPORT         0x3420  /* D0:F0 express mode */
 #define PCI_DEVICE_ID_IOH_REV           0x2
@@ -97,6 +98,7 @@ static int ioh3420_initfn(PCIDevice *d)
     PCIEPort *p = PCIE_PORT(d);
     PCIESlot *s = PCIE_SLOT(d);
     int rc;
+    Error *err = NULL;
 
     pci_bridge_initfn(d, TYPE_PCIE_BUS);
     pcie_port_init_reg(d);
@@ -109,8 +111,10 @@ static int ioh3420_initfn(PCIDevice *d)
 
     rc = msi_init(d, IOH_EP_MSI_OFFSET, IOH_EP_MSI_NR_VECTOR,
                   IOH_EP_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_64BIT,
-                  IOH_EP_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT);
+                  IOH_EP_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT, &err);
     if (rc < 0) {
+        assert(rc == -ENOTSUP);
+        error_report_err(err);
         goto err_bridge;
     }
 
diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index 0fbecc4..c4d2c0b 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -54,6 +54,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
     PCIBridge *br = PCI_BRIDGE(dev);
     PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
     int err;
+    Error *local_err = NULL;
 
     pci_bridge_initfn(dev, TYPE_PCI_BUS);
 
@@ -75,12 +76,23 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
         goto slotid_error;
     }
 
-    if (bridge_dev->msi != ON_OFF_AUTO_OFF &&
-        msi_nonbroken) {
-        err = msi_init(dev, 0, 1, true, true);
-        if (err < 0) {
+    if (bridge_dev->msi != ON_OFF_AUTO_OFF) {
+        /* it means SHPC exists, because SHPC is required for MSI */
+
+        err = msi_init(dev, 0, 1, true, true, &local_err);
+        /* Any error other than -ENOTSUP(board's MSI support is broken)
+         * is a programming error */
+        assert(!err || err == -ENOTSUP);
+        if (err && bridge_dev->msi == ON_OFF_AUTO_ON) {
+            /* Can't satisfy user's explicit msi=on request, fail */
+            error_append_hint(&local_err, "You have to use msi=auto (default) "
+                    "or msi=off with this machine type.\n");
+            error_report_err(local_err);
             goto msi_error;
         }
+        assert(!local_err || bridge_dev->msi == ON_OFF_AUTO_AUTO);
+        /* With msi=auto, we fall back to MSI off silently */
+        error_free(local_err);
     }
 
     if (shpc_present(dev)) {
diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
index e6d653d..f6149a3 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -24,6 +24,7 @@
 #include "hw/pci/msi.h"
 #include "hw/pci/pcie.h"
 #include "xio3130_downstream.h"
+#include "qapi/error.h"
 
 #define PCI_DEVICE_ID_TI_XIO3130D       0x8233  /* downstream port */
 #define XIO3130_REVISION                0x1
@@ -60,14 +61,17 @@ static int xio3130_downstream_initfn(PCIDevice *d)
     PCIEPort *p = PCIE_PORT(d);
     PCIESlot *s = PCIE_SLOT(d);
     int rc;
+    Error *err = NULL;
 
     pci_bridge_initfn(d, TYPE_PCIE_BUS);
     pcie_port_init_reg(d);
 
     rc = msi_init(d, XIO3130_MSI_OFFSET, XIO3130_MSI_NR_VECTOR,
                   XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_64BIT,
-                  XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT);
+                  XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT, &err);
     if (rc < 0) {
+        assert(rc == -ENOTSUP);
+        error_report_err(err);
         goto err_bridge;
     }
 
diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c
index d976844..487edac 100644
--- a/hw/pci-bridge/xio3130_upstream.c
+++ b/hw/pci-bridge/xio3130_upstream.c
@@ -24,6 +24,7 @@
 #include "hw/pci/msi.h"
 #include "hw/pci/pcie.h"
 #include "xio3130_upstream.h"
+#include "qapi/error.h"
 
 #define PCI_DEVICE_ID_TI_XIO3130U       0x8232  /* upstream port */
 #define XIO3130_REVISION                0x2
@@ -56,14 +57,17 @@ static int xio3130_upstream_initfn(PCIDevice *d)
 {
     PCIEPort *p = PCIE_PORT(d);
     int rc;
+    Error *err = NULL;
 
     pci_bridge_initfn(d, TYPE_PCIE_BUS);
     pcie_port_init_reg(d);
 
     rc = msi_init(d, XIO3130_MSI_OFFSET, XIO3130_MSI_NR_VECTOR,
                   XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_64BIT,
-                  XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT);
+                  XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT, &err);
     if (rc < 0) {
+        assert(rc == -ENOTSUP);
+        error_report_err(err);
         goto err_bridge;
     }
 
diff --git a/hw/pci/msi.c b/hw/pci/msi.c
index 70464bf..51a3169 100644
--- a/hw/pci/msi.c
+++ b/hw/pci/msi.c
@@ -22,6 +22,7 @@
 #include "hw/pci/msi.h"
 #include "hw/xen/xen.h"
 #include "qemu/range.h"
+#include "qapi/error.h"
 
 /* PCI_MSI_ADDRESS_LO */
 #define PCI_MSI_ADDRESS_LO_MASK         (~0x3)
@@ -173,7 +174,8 @@ bool msi_enabled(const PCIDevice *dev)
  * If @msi64bit, make the device capable of sending a 64-bit message
  * address.
  * If @msi_per_vector_mask, make the device support per-vector masking.
- * Return 0 on success, return -errno on error.
+ * @errp is for returning errors.
+ * Return 0 on success; set @errp and return -errno on error.
  *
  * -ENOTSUP means lacking msi support for a msi-capable platform.
  * -EINVAL means capability overlap, happens when @offset is non-zero,
@@ -181,7 +183,8 @@ bool msi_enabled(const PCIDevice *dev)
  *  if a real HW is broken.
  */
 int msi_init(struct PCIDevice *dev, uint8_t offset,
-             unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask)
+             unsigned int nr_vectors, bool msi64bit,
+             bool msi_per_vector_mask, Error **errp)
 {
     unsigned int vectors_order;
     uint16_t flags;
@@ -189,6 +192,7 @@ int msi_init(struct PCIDevice *dev, uint8_t offset,
     int config_offset;
 
     if (!msi_nonbroken) {
+        error_setg(errp, "MSI is not supported by interrupt controller");
         return -ENOTSUP;
     }
 
@@ -212,7 +216,8 @@ int msi_init(struct PCIDevice *dev, uint8_t offset,
     }
 
     cap_size = msi_cap_sizeof(flags);
-    config_offset = pci_add_capability(dev, PCI_CAP_ID_MSI, offset, cap_size);
+    config_offset = pci_add_capability2(dev, PCI_CAP_ID_MSI, offset,
+                                        cap_size, errp);
     if (config_offset < 0) {
         return config_offset;
     }
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 26119bf..5c57106 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -29,7 +29,7 @@
 #include "hw/scsi/scsi.h"
 #include "block/scsi.h"
 #include "trace.h"
-
+#include "qapi/error.h"
 #include "mfi.h"
 
 #define MEGASAS_VERSION_GEN1 "1.70"
@@ -2330,6 +2330,8 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
     MegasasBaseClass *b = MEGASAS_DEVICE_GET_CLASS(s);
     uint8_t *pci_conf;
     int i, bar_type;
+    Error *err = NULL;
+    int ret;
 
     pci_conf = dev->config;
 
@@ -2338,6 +2340,25 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
     /* Interrupt pin 1 */
     pci_conf[PCI_INTERRUPT_PIN] = 0x01;
 
+    if (megasas_use_msi(s)) {
+        ret = msi_init(dev, 0x50, 1, true, false, &err);
+        /* Any error other than -ENOTSUP(board's MSI support is broken)
+         * is a programming error */
+        assert(!ret || ret == -ENOTSUP);
+        if (ret && s->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;
+        } else if (ret) {
+            s->msi = ON_OFF_AUTO_OFF;
+        }
+
+        /* 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,
                           "megasas-mmio", 0x4000);
     memory_region_init_io(&s->port_io, OBJECT(s), &megasas_port_ops, s,
@@ -2345,10 +2366,6 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
     memory_region_init_io(&s->queue_io, OBJECT(s), &megasas_queue_ops, s,
                           "megasas-queue", 0x40000);
 
-    if (megasas_use_msi(s) &&
-        msi_init(dev, 0x50, 1, true, false)) {
-        s->msi = ON_OFF_AUTO_OFF;
-    }
     if (megasas_use_msix(s) &&
         msix_init(dev, 15, &s->mmio_io, b->mmio_bar, 0x2000,
                   &s->mmio_io, b->mmio_bar, 0x3800, 0x68)) {
diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
index d4773e2..5bde3cd 100644
--- a/hw/scsi/mptsas.c
+++ b/hw/scsi/mptsas.c
@@ -32,7 +32,7 @@
 #include "hw/scsi/scsi.h"
 #include "block/scsi.h"
 #include "trace.h"
-
+#include "qapi/error.h"
 #include "mptsas.h"
 #include "mpi.h"
 
@@ -1274,10 +1274,33 @@ static void mptsas_scsi_realize(PCIDevice *dev, Error **errp)
 {
     DeviceState *d = DEVICE(dev);
     MPTSASState *s = MPT_SAS(dev);
+    Error *err = NULL;
+    int ret;
 
     dev->config[PCI_LATENCY_TIMER] = 0;
     dev->config[PCI_INTERRUPT_PIN] = 0x01;
 
+    if (s->msi != ON_OFF_AUTO_OFF) {
+        ret = msi_init(dev, 0, 1, true, false, &err);
+        /* Any error other than -ENOTSUP(board's MSI support is broken)
+         * is a programming error */
+        assert(!ret || ret == -ENOTSUP);
+        if (ret && s->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);
+            s->msi_in_use = false;
+            return;
+        } else if (ret) {
+            /* With msi=auto, we fall back to MSI off silently */
+            error_free(err);
+            s->msi_in_use = false;
+        } else {
+            s->msi_in_use = true;
+        }
+    }
+
     memory_region_init_io(&s->mmio_io, OBJECT(s), &mptsas_mmio_ops, s,
                           "mptsas-mmio", 0x4000);
     memory_region_init_io(&s->port_io, OBJECT(s), &mptsas_port_ops, s,
@@ -1285,12 +1308,6 @@ static void mptsas_scsi_realize(PCIDevice *dev, Error **errp)
     memory_region_init_io(&s->diag_io, OBJECT(s), &mptsas_diag_ops, s,
                           "mptsas-diag", 0x10000);
 
-    if (s->msi != ON_OFF_AUTO_OFF &&
-        msi_init(dev, 0, 1, true, false) > 0) {
-        /* TODO check for errors */
-        s->msi_in_use = true;
-    }
-
     pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &s->port_io);
     pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY |
                                  PCI_BASE_ADDRESS_MEM_TYPE_32, &s->mmio_io);
diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
index c2a387a..c384442 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -1044,12 +1044,14 @@ static void
 pvscsi_init_msi(PVSCSIState *s)
 {
     int res;
+    Error *err = NULL;
     PCIDevice *d = PCI_DEVICE(s);
 
     res = msi_init(d, PVSCSI_MSI_OFFSET(s), PVSCSI_MSIX_NUM_VECTORS,
-                   PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK);
+                   PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK, &err);
     if (res < 0) {
         trace_pvscsi_init_msi_fail(res);
+        error_free(err);
         s->msi_used = false;
     } else {
         s->msi_used = true;
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 0a5510f..f76a15f 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -26,6 +26,7 @@
 #include "hw/pci/msi.h"
 #include "hw/pci/msix.h"
 #include "trace.h"
+#include "qapi/error.h"
 
 //#define DEBUG_XHCI
 //#define DEBUG_DATA
@@ -3581,6 +3582,7 @@ static void usb_xhci_init(XHCIState *xhci)
 static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
 {
     int i, ret;
+    Error *err = NULL;
 
     XHCIState *xhci = XHCI(dev);
 
@@ -3591,6 +3593,23 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
 
     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 "
+                    "si=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;
     }
@@ -3648,10 +3667,6 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
         assert(ret >= 0);
     }
 
-    if (xhci->msi != ON_OFF_AUTO_OFF) {
-        /* TODO check for errors */
-        msi_init(dev, 0x70, xhci->numintrs, true, false);
-    }
     if (xhci->msix != ON_OFF_AUTO_OFF) {
         /* TODO check for errors */
         msix_init(dev, xhci->numintrs,
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index d091d8c..67d93f7 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -32,6 +32,7 @@
 #include "sysemu/sysemu.h"
 #include "pci.h"
 #include "trace.h"
+#include "qapi/error.h"
 
 #define MSIX_CAP_LENGTH 12
 
@@ -1171,6 +1172,7 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos)
     uint16_t ctrl;
     bool msi_64bit, msi_maskbit;
     int ret, entries;
+    Error *err = NULL;
 
     if (pread(vdev->vbasedev.fd, &ctrl, sizeof(ctrl),
               vdev->config_offset + pos + PCI_CAP_FLAGS) != sizeof(ctrl)) {
@@ -1184,12 +1186,13 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos)
 
     trace_vfio_msi_setup(vdev->vbasedev.name, pos);
 
-    ret = msi_init(&vdev->pdev, pos, entries, msi_64bit, msi_maskbit);
+    ret = msi_init(&vdev->pdev, pos, entries, msi_64bit, msi_maskbit, &err);
     if (ret < 0) {
         if (ret == -ENOTSUP) {
             return 0;
         }
-        error_report("vfio: msi_init failed");
+        error_prepend(&err, "vfio: msi_init failed: ");
+        error_report_err(err);
         return ret;
     }
     vdev->msi_cap_size = 0xa + (msi_maskbit ? 0xa : 0) + (msi_64bit ? 0x4 : 0);
diff --git a/include/hw/pci/msi.h b/include/hw/pci/msi.h
index 8124908..4837bcf 100644
--- a/include/hw/pci/msi.h
+++ b/include/hw/pci/msi.h
@@ -35,7 +35,8 @@ void msi_set_message(PCIDevice *dev, MSIMessage msg);
 MSIMessage msi_get_message(PCIDevice *dev, unsigned int vector);
 bool msi_enabled(const PCIDevice *dev);
 int msi_init(struct PCIDevice *dev, uint8_t offset,
-             unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask);
+             unsigned int nr_vectors, bool msi64bit,
+             bool msi_per_vector_mask, Error **errp);
 void msi_uninit(struct PCIDevice *dev);
 void msi_reset(PCIDevice *dev);
 void msi_notify(PCIDevice *dev, unsigned int vector);
-- 
2.1.0

Re: [Qemu-devel] [PATCH v7 12/15] pci: Convert msi_init() to Error and fix callers to check it

[Hannes Reinecke]

On 06/06/2016 10:00 AM, Cao jin wrote:
> msi_init() reports errors with error_report(), which is wrong
> when it's used in realize().
> 
> Fix by converting it to Error.
> 
> Fix its callers to handle failure instead of ignoring it.
> 
> For those callers who don't handle the failure, it might happen:
> when user want msi on, but he doesn't get what he want because of
> msi_init fails silently.
> 
> cc: Gerd Hoffmann <kraxel@redhat.com>
> cc: John Snow <jsnow@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/audio/intel-hda.c               | 25 +++++++++++++++++++++----
>  hw/ide/ich.c                       | 15 +++++++++------
>  hw/net/vmxnet3.c                   | 38 +++++++++++++-------------------------
>  hw/pci-bridge/ioh3420.c            |  6 +++++-
>  hw/pci-bridge/pci_bridge_dev.c     | 20 ++++++++++++++++----
>  hw/pci-bridge/xio3130_downstream.c |  6 +++++-
>  hw/pci-bridge/xio3130_upstream.c   |  6 +++++-
>  hw/pci/msi.c                       | 11 ++++++++---
>  hw/scsi/megasas.c                  | 27 ++++++++++++++++++++++-----
>  hw/scsi/mptsas.c                   | 31 ++++++++++++++++++++++++-------
>  hw/scsi/vmw_pvscsi.c               |  4 +++-
>  hw/usb/hcd-xhci.c                  | 23 +++++++++++++++++++----
>  hw/vfio/pci.c                      |  7 +++++--
>  include/hw/pci/msi.h               |  3 ++-
>  14 files changed, 157 insertions(+), 65 deletions(-)
> 
For the megasas parts:

Acked-by: Hannes Reinecke <hare@suse.com>

Cheers,

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

Re: [Qemu-devel] [PATCH v7 12/15] pci: Convert msi_init() to Error and fix callers to check it

[Cao jin]

On 06/06/2016 04:00 PM, Cao jin wrote:

> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c

>
> +    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 "
> +                    "si=off with this machine type.\n");

sorry, I made a typo "si=off", maybe it could be fixed when be applied 
by maintainer

-- 
Yours Sincerely,

Cao jin

Re: [Qemu-devel] [PATCH v7 12/15] pci: Convert msi_init() to Error and fix callers to check it

[Markus Armbruster]

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

> msi_init() reports errors with error_report(), which is wrong
> when it's used in realize().
>
> Fix by converting it to Error.
>
> Fix its callers to handle failure instead of ignoring it.
>
> For those callers who don't handle the failure, it might happen:
> when user want msi on, but he doesn't get what he want because of
> msi_init fails silently.
>
> cc: Gerd Hoffmann <kraxel@redhat.com>
> cc: John Snow <jsnow@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/audio/intel-hda.c               | 25 +++++++++++++++++++++----
>  hw/ide/ich.c                       | 15 +++++++++------
>  hw/net/vmxnet3.c                   | 38 +++++++++++++-------------------------
>  hw/pci-bridge/ioh3420.c            |  6 +++++-
>  hw/pci-bridge/pci_bridge_dev.c     | 20 ++++++++++++++++----
>  hw/pci-bridge/xio3130_downstream.c |  6 +++++-
>  hw/pci-bridge/xio3130_upstream.c   |  6 +++++-
>  hw/pci/msi.c                       | 11 ++++++++---
>  hw/scsi/megasas.c                  | 27 ++++++++++++++++++++++-----
>  hw/scsi/mptsas.c                   | 31 ++++++++++++++++++++++++-------
>  hw/scsi/vmw_pvscsi.c               |  4 +++-
>  hw/usb/hcd-xhci.c                  | 23 +++++++++++++++++++----
>  hw/vfio/pci.c                      |  7 +++++--
>  include/hw/pci/msi.h               |  3 ++-
>  14 files changed, 157 insertions(+), 65 deletions(-)
>
> diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
> index 40a8772..d588fd5 100644
> --- a/hw/audio/intel-hda.c
> +++ b/hw/audio/intel-hda.c
> @@ -26,6 +26,7 @@
>  #include "intel-hda.h"
>  #include "intel-hda-defs.h"
>  #include "sysemu/dma.h"
> +#include "qapi/error.h"
>  
>  /* --------------------------------------------------------------------- */
>  /* hda bus                                                               */
> @@ -1131,6 +1132,8 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp)
>  {
>      IntelHDAState *d = INTEL_HDA(pci);
>      uint8_t *conf = d->pci.config;
> +    Error *err = NULL;
> +    int32_t ret;

Make this plain int, because that's what msi_init() returns.

>  
>      d->name = object_get_typename(OBJECT(d));
>  
> @@ -1139,13 +1142,27 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp)
>      /* HDCTL off 0x40 bit 0 selects signaling mode (1-HDA, 0 - Ac97) 18.1.19 */
>      conf[0x40] = 0x01;
>  
> +    if (d->msi != ON_OFF_AUTO_OFF) {
> +        ret = msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60,
> +                       1, true, false, &err);
> +        /* Any error other than -ENOTSUP(board's MSI support is broken)
> +         * is a programming error */
> +        assert(!ret || ret == -ENOTSUP);
> +        if (ret && d->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 || d->msi == ON_OFF_AUTO_AUTO);
> +        /* With msi=auto, we fall back to MSI off silently */
> +        error_free(err);
> +    }
> +
>      memory_region_init_io(&d->mmio, OBJECT(d), &intel_hda_mmio_ops, d,
>                            "intel-hda", 0x4000);
>      pci_register_bar(&d->pci, 0, 0, &d->mmio);
> -    if (d->msi != ON_OFF_AUTO_OFF) {
> -         /* TODO check for errors */
> -        msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, 1, true, false);
> -    }
>  
>      hda_codec_bus_init(DEVICE(pci), &d->codecs, sizeof(d->codecs),
>                         intel_hda_response, intel_hda_xfer);
[...]

Re: [Qemu-devel] [PATCH v7 12/15] pci: Convert msi_init() to Error and fix callers to check it

[Markus Armbruster]

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

> msi_init() reports errors with error_report(), which is wrong
> when it's used in realize().
>
> Fix by converting it to Error.
>
> Fix its callers to handle failure instead of ignoring it.
>
> For those callers who don't handle the failure, it might happen:
> when user want msi on, but he doesn't get what he want because of
> msi_init fails silently.
>
> cc: Gerd Hoffmann <kraxel@redhat.com>
> cc: John Snow <jsnow@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/scsi/megasas.c b/hw/scsi/megasas.c
> index 26119bf..5c57106 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -29,7 +29,7 @@
>  #include "hw/scsi/scsi.h"
>  #include "block/scsi.h"
>  #include "trace.h"
> -
> +#include "qapi/error.h"
>  #include "mfi.h"
>  
>  #define MEGASAS_VERSION_GEN1 "1.70"
> @@ -2330,6 +2330,8 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
>      MegasasBaseClass *b = MEGASAS_DEVICE_GET_CLASS(s);
>      uint8_t *pci_conf;
>      int i, bar_type;
> +    Error *err = NULL;
> +    int ret;
>  
>      pci_conf = dev->config;
>  
> @@ -2338,6 +2340,25 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
>      /* Interrupt pin 1 */
>      pci_conf[PCI_INTERRUPT_PIN] = 0x01;
>  
> +    if (megasas_use_msi(s)) {
> +        ret = msi_init(dev, 0x50, 1, true, false, &err);
> +        /* Any error other than -ENOTSUP(board's MSI support is broken)
> +         * is a programming error */
> +        assert(!ret || ret == -ENOTSUP);
> +        if (ret && s->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;
> +        } else if (ret) {
> +            s->msi = ON_OFF_AUTO_OFF;
> +        }
> +
> +        /* With msi=auto, we fall back to MSI off silently */
> +        error_free(err);

Let's move this into the "else if (ret)" conditional, where the comment
serves to explain the s->msi = ON_OFF_AUTO_OFF.

> +    }
> +
>      memory_region_init_io(&s->mmio_io, OBJECT(s), &megasas_mmio_ops, s,
>                            "megasas-mmio", 0x4000);
>      memory_region_init_io(&s->port_io, OBJECT(s), &megasas_port_ops, s,
> @@ -2345,10 +2366,6 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
>      memory_region_init_io(&s->queue_io, OBJECT(s), &megasas_queue_ops, s,
>                            "megasas-queue", 0x40000);
>  
> -    if (megasas_use_msi(s) &&
> -        msi_init(dev, 0x50, 1, true, false)) {
> -        s->msi = ON_OFF_AUTO_OFF;
> -    }
>      if (megasas_use_msix(s) &&
>          msix_init(dev, 15, &s->mmio_io, b->mmio_bar, 0x2000,
>                    &s->mmio_io, b->mmio_bar, 0x3800, 0x68)) {
[...]
> diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
> index c2a387a..c384442 100644
> --- a/hw/scsi/vmw_pvscsi.c
> +++ b/hw/scsi/vmw_pvscsi.c
> @@ -1044,12 +1044,14 @@ static void
>  pvscsi_init_msi(PVSCSIState *s)
>  {
>      int res;
> +    Error *err = NULL;
>      PCIDevice *d = PCI_DEVICE(s);
>  
>      res = msi_init(d, PVSCSI_MSI_OFFSET(s), PVSCSI_MSIX_NUM_VECTORS,
> -                   PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK);
> +                   PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK, &err);
>      if (res < 0) {
>          trace_pvscsi_init_msi_fail(res);
> +        error_free(err);
>          s->msi_used = false;
>      } else {
>          s->msi_used = true;

Since you're not doing anything with err:

   pvscsi_init_msi(PVSCSIState *s)
   {
       int res;
       PCIDevice *d = PCI_DEVICE(s);
   
       res = msi_init(d, PVSCSI_MSI_OFFSET(s), PVSCSI_MSIX_NUM_VECTORS,
  -                   PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK);
  +                   PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK, NULL);
       if (res < 0) {
           trace_pvscsi_init_msi_fail(res);
           s->msi_used = false;
       } else {
           s->msi_used = true;

[...]

[Qemu-devel] [PATCH v7 13/15] megasas: remove unnecessary megasas_use_msi()

[Cao jin]

megasas overwrite user configuration when msi_init fail to reflect internal msi
state, which is unsuitable. megasa_use_msi() is unnecessary, we can call
msi_uninit() when unrealize, even no bother to call msi_enabled() first.

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 | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 5c57106..84d7885 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_msi(MegasasState *s)
-{
-    return s->msi != ON_OFF_AUTO_OFF;
-}
-
 static bool megasas_use_msix(MegasasState *s)
 {
     return s->msix != ON_OFF_AUTO_OFF;
@@ -2307,9 +2302,7 @@ static void megasas_scsi_uninit(PCIDevice *d)
     if (megasas_use_msix(s)) {
         msix_uninit(d, &s->mmio_io, &s->mmio_io);
     }
-    if (megasas_use_msi(s)) {
-        msi_uninit(d);
-    }
+    msi_uninit(d);
 }
 
 static const struct SCSIBusInfo megasas_scsi_info = {
@@ -2340,7 +2333,7 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
     /* Interrupt pin 1 */
     pci_conf[PCI_INTERRUPT_PIN] = 0x01;
 
-    if (megasas_use_msi(s)) {
+    if (s->msi != ON_OFF_AUTO_OFF) {
         ret = msi_init(dev, 0x50, 1, true, false, &err);
         /* Any error other than -ENOTSUP(board's MSI support is broken)
          * is a programming error */
@@ -2351,10 +2344,8 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
                     "msi=off with this machine type.\n");
             error_propagate(errp, err);
             return;
-        } else if (ret) {
-            s->msi = ON_OFF_AUTO_OFF;
         }
-
+        assert(!err || s->msi == ON_OFF_AUTO_AUTO);
         /* With msi=auto, we fall back to MSI off silently */
         error_free(err);
     }
-- 
2.1.0

Re: [Qemu-devel] [PATCH v7 13/15] megasas: remove unnecessary megasas_use_msi()

[Hannes Reinecke]

On 06/06/2016 10:01 AM, Cao jin wrote:
> megasas overwrite user configuration when msi_init fail to reflect internal msi
> state, which is unsuitable. megasa_use_msi() is unnecessary, we can call
> msi_uninit() when unrealize, even no bother to call msi_enabled() first.
> 
> 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 | 15 +++------------
>  1 file changed, 3 insertions(+), 12 deletions(-)
> 

Acked-by: Hannes Reinecke <hare@suse.com>

Cheers,

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

[Qemu-devel] [PATCH v7 14/15] mptsas: remove unnecessary internal msi state flag

[Cao jin]

internal flag msi_in_use in unnecessary, msi_uninit() could be called
directly, and msi_enabled() is enough to check device msi state.

cc: Markus Armbruster <armbru@redhat.com>
cc: Marcel Apfelbaum <marcel@redhat.com>
cc: Paolo Bonzini <pbonzini@redhat.com>
cc: Michael S. Tsirkin <mst@redhat.com>

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/scsi/mptsas.c | 18 ++++++------------
 hw/scsi/mptsas.h |  2 --
 2 files changed, 6 insertions(+), 14 deletions(-)

diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
index 5bde3cd..249c976 100644
--- a/hw/scsi/mptsas.c
+++ b/hw/scsi/mptsas.c
@@ -63,7 +63,7 @@ static void mptsas_update_interrupt(MPTSASState *s)
     PCIDevice *pci = (PCIDevice *) s;
     uint32_t state = s->intr_status & ~(s->intr_mask | MPI_HIS_IOP_DOORBELL_STATUS);
 
-    if (s->msi_in_use && msi_enabled(pci)) {
+    if (msi_enabled(pci)) {
         if (state) {
             trace_mptsas_irq_msi(s);
             msi_notify(pci, 0);
@@ -1290,15 +1290,12 @@ static void mptsas_scsi_realize(PCIDevice *dev, Error **errp)
             error_append_hint(&err, "You have to use msi=auto (default) or "
                     "msi=off with this machine type.\n");
             error_propagate(errp, err);
-            s->msi_in_use = false;
             return;
-        } else if (ret) {
-            /* With msi=auto, we fall back to MSI off silently */
-            error_free(err);
-            s->msi_in_use = false;
-        } else {
-            s->msi_in_use = true;
         }
+        assert(!err || s->msi == 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), &mptsas_mmio_ops, s,
@@ -1338,9 +1335,7 @@ static void mptsas_scsi_uninit(PCIDevice *dev)
     MPTSASState *s = MPT_SAS(dev);
 
     qemu_bh_delete(s->request_bh);
-    if (s->msi_in_use) {
-        msi_uninit(dev);
-    }
+    msi_uninit(dev);
 }
 
 static void mptsas_reset(DeviceState *dev)
@@ -1376,7 +1371,6 @@ static const VMStateDescription vmstate_mptsas = {
     .post_load = mptsas_post_load,
     .fields      = (VMStateField[]) {
         VMSTATE_PCI_DEVICE(dev, MPTSASState),
-        VMSTATE_BOOL(msi_in_use, MPTSASState),
 
         VMSTATE_UINT32(state, MPTSASState),
         VMSTATE_UINT8(who_init, MPTSASState),
diff --git a/hw/scsi/mptsas.h b/hw/scsi/mptsas.h
index 0436a33..da014a3 100644
--- a/hw/scsi/mptsas.h
+++ b/hw/scsi/mptsas.h
@@ -31,8 +31,6 @@ struct MPTSASState {
     OnOffAuto msi;
     uint64_t sas_addr;
 
-    bool msi_in_use;
-
     /* Doorbell register */
     uint32_t state;
     uint8_t who_init;
-- 
2.1.0

[Qemu-devel] [PATCH v7 15/15] vmw_pvscsi: remove unnecessary internal msi state flag

[Cao jin]

Internal flag msi_used is uncesessary, msi_uninit() could be called
directly, msi_enabled() is enough to check device msi state.

cc: Paolo Bonzini <pbonzini@redhat.com>
cc: Dmitry Fleytman <dmitry@daynix.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/vmw_pvscsi.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
index c384442..9838fa5 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -121,8 +121,6 @@ typedef struct {
     uint8_t msg_ring_info_valid;         /* Whether message ring initialized */
     uint8_t use_msg;                     /* Whether to use message ring      */
 
-    uint8_t msi_used;    /* Whether MSI support was installed successfully   */
-
     PVSCSIRingInfo rings;                /* Data transfer rings manager      */
     uint32_t resetting;                  /* Reset in progress                */
 
@@ -351,7 +349,7 @@ pvscsi_update_irq_status(PVSCSIState *s)
     trace_pvscsi_update_irq_level(should_raise, s->reg_interrupt_enabled,
                                   s->reg_interrupt_status);
 
-    if (s->msi_used && msi_enabled(d)) {
+    if (msi_enabled(d)) {
         if (should_raise) {
             trace_pvscsi_update_irq_msi();
             msi_notify(d, PVSCSI_VECTOR_COMPLETION);
@@ -1049,12 +1047,9 @@ pvscsi_init_msi(PVSCSIState *s)
 
     res = msi_init(d, PVSCSI_MSI_OFFSET(s), PVSCSI_MSIX_NUM_VECTORS,
                    PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK, &err);
-    if (res < 0) {
+    if (res) {
         trace_pvscsi_init_msi_fail(res);
         error_free(err);
-        s->msi_used = false;
-    } else {
-        s->msi_used = true;
     }
 }
 
@@ -1063,9 +1058,7 @@ pvscsi_cleanup_msi(PVSCSIState *s)
 {
     PCIDevice *d = PCI_DEVICE(s);
 
-    if (s->msi_used) {
-        msi_uninit(d);
-    }
+    msi_uninit(d);
 }
 
 static const MemoryRegionOps pvscsi_ops = {
@@ -1208,7 +1201,6 @@ static const VMStateDescription vmstate_pvscsi = {
         VMSTATE_STRUCT_TEST(parent_obj, PVSCSIState,
                             pvscsi_vmstate_test_pci_device, 0,
                             vmstate_pci_device, PCIDevice),
-        VMSTATE_UINT8(msi_used, PVSCSIState),
         VMSTATE_UINT32(resetting, PVSCSIState),
         VMSTATE_UINT64(reg_interrupt_status, PVSCSIState),
         VMSTATE_UINT64(reg_interrupt_enabled, PVSCSIState),
-- 
2.1.0

Re: [Qemu-devel] [PATCH v7 15/15] vmw_pvscsi: remove unnecessary internal msi state flag

[Markus Armbruster]

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

> Internal flag msi_used is uncesessary, msi_uninit() could be called
> directly, msi_enabled() is enough to check device msi state.
>
> cc: Paolo Bonzini <pbonzini@redhat.com>
> cc: Dmitry Fleytman <dmitry@daynix.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/vmw_pvscsi.c | 14 +++-----------
>  1 file changed, 3 insertions(+), 11 deletions(-)
>
> diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
> index c384442..9838fa5 100644
> --- a/hw/scsi/vmw_pvscsi.c
> +++ b/hw/scsi/vmw_pvscsi.c
> @@ -121,8 +121,6 @@ typedef struct {
>      uint8_t msg_ring_info_valid;         /* Whether message ring initialized */
>      uint8_t use_msg;                     /* Whether to use message ring      */
>  
> -    uint8_t msi_used;    /* Whether MSI support was installed successfully   */
> -
>      PVSCSIRingInfo rings;                /* Data transfer rings manager      */
>      uint32_t resetting;                  /* Reset in progress                */
>  
> @@ -351,7 +349,7 @@ pvscsi_update_irq_status(PVSCSIState *s)
>      trace_pvscsi_update_irq_level(should_raise, s->reg_interrupt_enabled,
>                                    s->reg_interrupt_status);
>  
> -    if (s->msi_used && msi_enabled(d)) {
> +    if (msi_enabled(d)) {
>          if (should_raise) {
>              trace_pvscsi_update_irq_msi();
>              msi_notify(d, PVSCSI_VECTOR_COMPLETION);
> @@ -1049,12 +1047,9 @@ pvscsi_init_msi(PVSCSIState *s)
>  
>      res = msi_init(d, PVSCSI_MSI_OFFSET(s), PVSCSI_MSIX_NUM_VECTORS,
>                     PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK, &err);
> -    if (res < 0) {
> +    if (res) {

I'd leave this alone.

>          trace_pvscsi_init_msi_fail(res);
>          error_free(err);
> -        s->msi_used = false;
> -    } else {
> -        s->msi_used = true;
>      }
>  }
>  
> @@ -1063,9 +1058,7 @@ pvscsi_cleanup_msi(PVSCSIState *s)
>  {
>      PCIDevice *d = PCI_DEVICE(s);
>  
> -    if (s->msi_used) {
> -        msi_uninit(d);
> -    }
> +    msi_uninit(d);
>  }
>  
>  static const MemoryRegionOps pvscsi_ops = {
> @@ -1208,7 +1201,6 @@ static const VMStateDescription vmstate_pvscsi = {
>          VMSTATE_STRUCT_TEST(parent_obj, PVSCSIState,
>                              pvscsi_vmstate_test_pci_device, 0,
>                              vmstate_pci_device, PCIDevice),
> -        VMSTATE_UINT8(msi_used, PVSCSIState),
>          VMSTATE_UINT32(resetting, PVSCSIState),
>          VMSTATE_UINT64(reg_interrupt_status, PVSCSIState),
>          VMSTATE_UINT64(reg_interrupt_enabled, PVSCSIState),

Re: [Qemu-devel] [PATCH v7 00/15] Add param Error ** for msi_init()

[Cao jin]

ping~

On 06/06/2016 04:00 PM, Cao jin wrote:
> v7 changelog:
> 1. 4 new patches, patch 11, 13, 14, 15. previous patch 11 numbered 12 now.
> 2. patch 2: remove comment of "errp", add it in patch 12
> 3. patch 4: fix commit message as sugguestion
> 4. replace all "msi == ON_OFF_AUTO_ON || msi == ON_OFF_AUTO_AUTO" to
>     "msi != ON_OFF_AUTO_OFF", before patch 12
> 5. patch 7: correct a error in intel_hda_update_irq(), and change the
>     variant type from int to bool
> 6. patch 13: fix the issue that code would overwrite user configuration.
>     patch 14&15 actually almost the same with 13, remove unnecessary flag.
> 7. patch 12 use tightened error checking as suggestions, but not sure
>     megasas/mptsas is modified as sugguestion perfectly, RFC.
>
> cc: Gerd Hoffmann <kraxel@redhat.com>
> cc: John Snow <jsnow@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 (15):
>    pci core: assert ENOSPC when add capability
>    fix some coding style problems
>    change pvscsi_init_msi() type to void
>    megasas: Fix check for msi_init() failure
>    mptsas: change .realize function name
>    usb xhci: change msi/msix property type
>    intel-hda: change msi property type
>    mptsas: change msi property type
>    megasas: change msi/msix property type
>    pci bridge dev: change msi property type
>    msi_init: change return value to 0 on success
>    pci: Convert msi_init() to Error and fix callers to check it
>    megasas: remove unnecessary megasas_use_msi()
>    mptsas: remove unnecessary internal msi state flag
>    vmw_pvscsi: remove unnecessary internal msi state flag
>
>   hw/audio/intel-hda.c               | 30 ++++++++++++++++----
>   hw/ide/ich.c                       | 15 ++++++----
>   hw/net/vmxnet3.c                   | 40 +++++++++-----------------
>   hw/pci-bridge/ioh3420.c            | 13 +++++++--
>   hw/pci-bridge/pci_bridge_dev.c     | 35 +++++++++++++++++------
>   hw/pci-bridge/xio3130_downstream.c | 12 ++++++--
>   hw/pci-bridge/xio3130_upstream.c   |  9 +++++-
>   hw/pci/msi.c                       | 27 ++++++++++++++++--
>   hw/pci/pci.c                       |  6 ++--
>   hw/scsi/megasas.c                  | 58 ++++++++++++++++++++------------------
>   hw/scsi/mptsas.c                   | 40 +++++++++++++++++---------
>   hw/scsi/mptsas.h                   |  5 ++--
>   hw/scsi/vmw_pvscsi.c               | 22 +++++----------
>   hw/usb/hcd-xhci.c                  | 35 +++++++++++++++++------
>   hw/vfio/pci.c                      |  7 +++--
>   include/hw/pci/msi.h               |  3 +-
>   16 files changed, 226 insertions(+), 131 deletions(-)
>

-- 
Yours Sincerely,

Cao jin

Re: [Qemu-devel] [PATCH v7 00/15] Add param Error ** for msi_init()

[Markus Armbruster]

I had only minor stylistic remarks this time.  However, the series needs
a rebase already.  If the conflict resolution is trivial, you may add my

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

Re: [Qemu-devel] [PATCH v7 00/15] Add param Error ** for msi_init()

[Cao jin]

On 06/09/2016 11:58 PM, Markus Armbruster wrote:
> I had only minor stylistic remarks this time.  However, the series needs
> a rebase already.  If the conflict resolution is trivial, you may add my
>

Yes, it is trivial. just two file name changed in commit 605d52e62:

-#include "vmxnet_tx_pkt.h"
-#include "vmxnet_rx_pkt.h"
+#include "net_tx_pkt.h"
+#include "net_rx_pkt.h"

But there is a new device "e1000e" added, so, there is additional part 
in patch 12, maybe you want to double check it. But I will add your R-b 
first:)

Thanks very much!

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


-- 
Yours Sincerely,

Cao jin