[Qemu-devel] [PATCH RFC v2 0/2] MSI/MSIX: fix to catch and report errors

[Qemu-devel] [PATCH RFC v2 0/2] MSI/MSIX: fix to catch and report errors

[Cao jin]

msi_init() & msix_init() are supporting functions for PCI devices. catch the
errors they produced and report.

V2 changelog:
1. Modify as per Markus`s review
  a. Try to cleanup on function fail, as possible as I can.
  b. For .init() function, use error_report_err() and return non-zero value.
  c. For .realize(), propagate the error.
  d. Special case: TYPE_ICH9_AHCI, it is a on-board device initialized with
     machine init, so don`t bother to cleanup on failure, as process will exit
     anyway.

Cao jin (2):
  Add param Error** to msi_init() & modify the callers
  Add param Error** to msix_init() & modify the callers

 hw/audio/intel-hda.c               | 10 ++++-
 hw/block/nvme.c                    | 32 +++++++++-----
 hw/ide/ich.c                       |  2 +-
 hw/misc/ivshmem.c                  |  7 ++-
 hw/net/rocker/rocker.c             | 10 +++--
 hw/net/vmxnet3.c                   | 39 +++++++++++------
 hw/pci-bridge/ioh3420.c            |  7 ++-
 hw/pci-bridge/pci_bridge_dev.c     |  8 +++-
 hw/pci-bridge/xio3130_downstream.c |  8 +++-
 hw/pci-bridge/xio3130_upstream.c   |  8 +++-
 hw/pci/msi.c                       | 18 ++++++--
 hw/pci/msix.c                      | 20 ++++++---
 hw/scsi/megasas.c                  | 35 +++++++++++----
 hw/scsi/vmw_pvscsi.c               | 13 ++++--
 hw/usb/hcd-xhci.c                  | 88 +++++++++++++++++++++-----------------
 hw/vfio/pci.c                      | 28 ++++++------
 hw/virtio/virtio-bus.c             |  3 ++
 hw/virtio/virtio-pci.c             | 64 +++++++++++++--------------
 include/hw/pci/msi.h               |  4 +-
 include/hw/pci/msix.h              |  5 ++-
 20 files changed, 264 insertions(+), 145 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH RFC v2 1/2] Add param Error** to msi_init() & modify the callers

[Cao jin]

msi_init() is a supporting function in PCI device initialization,
in order to convert .init() to .realize(), it should be modified first.
Also modify the callers

Bonus: add more comment for msi_init().
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/audio/intel-hda.c               | 10 ++++-
 hw/ide/ich.c                       |  2 +-
 hw/net/vmxnet3.c                   | 13 +++---
 hw/pci-bridge/ioh3420.c            |  7 +++-
 hw/pci-bridge/pci_bridge_dev.c     |  8 +++-
 hw/pci-bridge/xio3130_downstream.c |  8 +++-
 hw/pci-bridge/xio3130_upstream.c   |  8 +++-
 hw/pci/msi.c                       | 18 +++++++--
 hw/scsi/megasas.c                  | 15 +++++--
 hw/scsi/vmw_pvscsi.c               | 13 ++++--
 hw/usb/hcd-xhci.c                  | 81 +++++++++++++++++++++-----------------
 hw/vfio/pci.c                      | 20 +++++-----
 include/hw/pci/msi.h               |  4 +-
 13 files changed, 135 insertions(+), 72 deletions(-)

diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
index 433463e..0d770131 100644
--- a/hw/audio/intel-hda.c
+++ b/hw/audio/intel-hda.c
@@ -1130,6 +1130,7 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp)
 {
     IntelHDAState *d = INTEL_HDA(pci);
     uint8_t *conf = d->pci.config;
+    int ret;
 
     d->name = object_get_typename(OBJECT(d));
 
@@ -1142,11 +1143,18 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp)
                           "intel-hda", 0x4000);
     pci_register_bar(&d->pci, 0, 0, &d->mmio);
     if (d->msi) {
-        msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, 1, true, false);
+        ret = msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, 1, true,
+                false, errp);
+        if (ret < 0) {
+            goto cleanup_on_msi_fail;
+        }
     }
 
     hda_codec_bus_init(DEVICE(pci), &d->codecs, sizeof(d->codecs),
                        intel_hda_response, intel_hda_xfer);
+
+cleanup_on_msi_fail:
+    object_unref(OBJECT(&d->mmio));
 }
 
 static void intel_hda_exit(PCIDevice *pci)
diff --git a/hw/ide/ich.c b/hw/ide/ich.c
index 16925fa..94b1809 100644
--- a/hw/ide/ich.c
+++ b/hw/ide/ich.c
@@ -145,7 +145,7 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp)
     /* 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);
+    msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false, errp);
 }
 
 static void pci_ich9_uninit(PCIDevice *dev)
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 37373e5..c373e77 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -2135,13 +2135,13 @@ vmxnet3_cleanup_msix(VMXNET3State *s)
 #define VMXNET3_PER_VECTOR_MASK   (false)
 
 static bool
-vmxnet3_init_msi(VMXNET3State *s)
+vmxnet3_init_msi(VMXNET3State *s, Error **errp)
 {
     PCIDevice *d = PCI_DEVICE(s);
     int res;
 
     res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MAX_NMSIX_INTRS,
-                   VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK);
+                   VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK, errp);
     if (0 > res) {
         VMW_WRPRN("Failed to initialize MSI, error %d", res);
         s->msi_used = false;
@@ -2204,6 +2204,11 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
 
     VMW_CBPRN("Starting init...");
 
+    if (!vmxnet3_init_msi(s, errp)) {
+        VMW_WRPRN("Failed to initialize MSI, configuration is inconsistent.");
+        return;
+    }
+
     memory_region_init_io(&s->bar0, OBJECT(s), &b0_ops, s,
                           "vmxnet3-b0", VMXNET3_PT_REG_SIZE);
     pci_register_bar(pci_dev, VMXNET3_BAR0_IDX,
@@ -2228,10 +2233,6 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
         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);
 
     register_savevm(dev, "vmxnet3-msix", -1, 1,
diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
index eead195..5df9e83 100644
--- a/hw/pci-bridge/ioh3420.c
+++ b/hw/pci-bridge/ioh3420.c
@@ -96,6 +96,7 @@ static int ioh3420_initfn(PCIDevice *d)
     PCIEPort *p = PCIE_PORT(d);
     PCIESlot *s = PCIE_SLOT(d);
     int rc;
+    Error *local_err = NULL;
 
     pci_bridge_initfn(d, TYPE_PCIE_BUS);
     pcie_port_init_reg(d);
@@ -105,12 +106,16 @@ 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);
+                  IOH_EP_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT,
+                  &local_err);
     if (rc < 0) {
+        error_report_err(local_err);
         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;
diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index bc3e1b7..bafaeeb 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -51,6 +51,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);
 
@@ -66,17 +67,21 @@ 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_supported) {
-        err = msi_init(dev, 0, 1, true, true);
+        err = msi_init(dev, 0, 1, true, true, &local_err);
         if (err < 0) {
+            error_report_err(local_err);
             goto msi_error;
         }
     }
+
     if (shpc_present(dev)) {
         /* TODO: spec recommends using 64 bit prefetcheable BAR.
          * Check whether that works well. */
@@ -84,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 b4dd25f..3fc7455 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -59,21 +59,26 @@ static int xio3130_downstream_initfn(PCIDevice *d)
     PCIEPort *p = PCIE_PORT(d);
     PCIESlot *s = PCIE_SLOT(d);
     int rc;
+    Error *local_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,
+                  &local_err);
     if (rc < 0) {
+        error_report_err(local_err);
         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) {
@@ -103,6 +108,7 @@ err_msi:
     msi_uninit(d);
 err_bridge:
     pci_bridge_exitfn(d);
+
     return rc;
 }
 
diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c
index 434c8fd..882271c 100644
--- a/hw/pci-bridge/xio3130_upstream.c
+++ b/hw/pci-bridge/xio3130_upstream.c
@@ -55,26 +55,32 @@ static int xio3130_upstream_initfn(PCIDevice *d)
 {
     PCIEPort *p = PCIE_PORT(d);
     int rc;
+    Error *local_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,
+                  &local_err);
     if (rc < 0) {
+        error_report_err(local_err);
         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) {
         goto err_msi;
     }
+
     pcie_cap_flr_init(d);
     pcie_cap_deverr_init(d);
     rc = pcie_aer_init(d, XIO3130_AER_OFFSET);
diff --git a/hw/pci/msi.c b/hw/pci/msi.c
index c1dd531..ad6ed09 100644
--- a/hw/pci/msi.c
+++ b/hw/pci/msi.c
@@ -150,15 +150,23 @@ bool msi_enabled(const PCIDevice *dev)
          PCI_MSI_FLAGS_ENABLE);
 }
 
-int msi_init(struct PCIDevice *dev, uint8_t offset,
-             unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask)
+/*
+ * @nr_vectors: Multiple Message Capable field of Message Control register
+ * @msi64bit: support 64-bit message address or not
+ * @msi_per_vector_mask: support per-vector masking or not
+ *
+ * return: MSI capability offset in config space
+ */
+int msi_init(struct PCIDevice *dev, uint8_t offset, unsigned int nr_vectors,
+             bool msi64bit, bool msi_per_vector_mask, Error **errp)
 {
     unsigned int vectors_order;
-    uint16_t flags;
+    uint16_t flags; /* Message Control register value */
     uint8_t cap_size;
     int config_offset;
 
     if (!msi_supported) {
+        error_setg(errp, "MSI is not supported by interrupt controller");
         return -ENOTSUP;
     }
 
@@ -182,7 +190,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;
     }
@@ -205,6 +214,7 @@ 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;
 }
 
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index d7dc667..ba25b3a 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -2339,6 +2339,17 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
     /* Interrupt pin 1 */
     pci_conf[PCI_INTERRUPT_PIN] = 0x01;
 
+    if (megasas_use_msi(s)) {
+        int ret;
+
+        ret = msi_init(dev, 0x50, 1, true, false, errp);
+        if (ret > 0) {
+            s->flags &= ~MEGASAS_MASK_USE_MSI;
+        } else {
+            return;
+        }
+    }
+
     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,
@@ -2346,10 +2357,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->flags &= ~MEGASAS_MASK_USE_MSI;
-    }
     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 9c71f31..073b956 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -1014,13 +1014,13 @@ pvscsi_io_read(void *opaque, hwaddr addr, unsigned size)
 
 
 static bool
-pvscsi_init_msi(PVSCSIState *s)
+pvscsi_init_msi(PVSCSIState *s, Error **errp)
 {
     int res;
     PCIDevice *d = PCI_DEVICE(s);
 
     res = msi_init(d, PVSCSI_MSI_OFFSET, PVSCSI_MSIX_NUM_VECTORS,
-                   PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK);
+                   PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK, errp);
     if (res < 0) {
         trace_pvscsi_init_msi_fail(res);
         s->msi_used = false;
@@ -1066,6 +1066,7 @@ static int
 pvscsi_init(PCIDevice *pci_dev)
 {
     PVSCSIState *s = PVSCSI(pci_dev);
+    Error *local_err = NULL;
 
     trace_pvscsi_state("init");
 
@@ -1079,12 +1080,16 @@ pvscsi_init(PCIDevice *pci_dev)
     /* Interrupt pin A */
     pci_config_set_interrupt_pin(pci_dev->config, 1);
 
+    pvscsi_init_msi(s, &local_err);
+    if (local_err) {
+        error_report_err(local_err);
+        return -1;
+    }
+
     memory_region_init_io(&s->io_space, OBJECT(s), &pvscsi_ops, s,
                           "pvscsi-io", PVSCSI_MEM_SPACE_SIZE);
     pci_register_bar(pci_dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->io_space);
 
-    pvscsi_init_msi(s);
-
     s->completion_worker = qemu_bh_new(pvscsi_process_completion_queue, s);
     if (!s->completion_worker) {
         pvscsi_cleanup_msi(s);
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 268ab36..7cd5f6c 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -3572,6 +3572,43 @@ static void usb_xhci_init(XHCIState *xhci)
     }
 }
 
+static void usb_xhci_exit(PCIDevice *dev)
+{
+    int i;
+    XHCIState *xhci = XHCI(dev);
+
+    trace_usb_xhci_exit();
+
+    for (i = 0; i < xhci->numslots; i++) {
+        xhci_disable_slot(xhci, i + 1);
+    }
+
+    if (xhci->mfwrap_timer) {
+        timer_del(xhci->mfwrap_timer);
+        timer_free(xhci->mfwrap_timer);
+        xhci->mfwrap_timer = NULL;
+    }
+
+    memory_region_del_subregion(&xhci->mem, &xhci->mem_cap);
+    memory_region_del_subregion(&xhci->mem, &xhci->mem_oper);
+    memory_region_del_subregion(&xhci->mem, &xhci->mem_runtime);
+    memory_region_del_subregion(&xhci->mem, &xhci->mem_doorbell);
+
+    for (i = 0; i < xhci->numports; i++) {
+        XHCIPort *port = &xhci->ports[i];
+        memory_region_del_subregion(&xhci->mem, &port->mem);
+    }
+
+    /* destroy msix memory region */
+    if (dev->msix_table && dev->msix_pba
+        && dev->msix_entry_used) {
+        memory_region_del_subregion(&xhci->mem, &dev->msix_table_mmio);
+        memory_region_del_subregion(&xhci->mem, &dev->msix_pba_mmio);
+    }
+
+    usb_bus_release(&xhci->bus);
+}
+
 static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
 {
     int i, ret;
@@ -3643,7 +3680,10 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
     }
 
     if (xhci_get_flag(xhci, XHCI_FLAG_USE_MSI)) {
-        msi_init(dev, 0x70, xhci->numintrs, true, false);
+        ret = msi_init(dev, 0x70, xhci->numintrs, true, false, errp);
+        if (ret < 0) {
+            goto cleanup_on_msi_fail;
+        }
     }
     if (xhci_get_flag(xhci, XHCI_FLAG_USE_MSI_X)) {
         msix_init(dev, xhci->numintrs,
@@ -3651,43 +3691,10 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
                   &xhci->mem, 0, OFF_MSIX_PBA,
                   0x90);
     }
-}
-
-static void usb_xhci_exit(PCIDevice *dev)
-{
-    int i;
-    XHCIState *xhci = XHCI(dev);
-
-    trace_usb_xhci_exit();
-
-    for (i = 0; i < xhci->numslots; i++) {
-        xhci_disable_slot(xhci, i + 1);
-    }
-
-    if (xhci->mfwrap_timer) {
-        timer_del(xhci->mfwrap_timer);
-        timer_free(xhci->mfwrap_timer);
-        xhci->mfwrap_timer = NULL;
-    }
 
-    memory_region_del_subregion(&xhci->mem, &xhci->mem_cap);
-    memory_region_del_subregion(&xhci->mem, &xhci->mem_oper);
-    memory_region_del_subregion(&xhci->mem, &xhci->mem_runtime);
-    memory_region_del_subregion(&xhci->mem, &xhci->mem_doorbell);
-
-    for (i = 0; i < xhci->numports; i++) {
-        XHCIPort *port = &xhci->ports[i];
-        memory_region_del_subregion(&xhci->mem, &port->mem);
-    }
-
-    /* destroy msix memory region */
-    if (dev->msix_table && dev->msix_pba
-        && dev->msix_entry_used) {
-        memory_region_del_subregion(&xhci->mem, &dev->msix_table_mmio);
-        memory_region_del_subregion(&xhci->mem, &dev->msix_pba_mmio);
-    }
-
-    usb_bus_release(&xhci->bus);
+cleanup_on_msi_fail:
+    usb_xhci_exit(dev);
+    object_unref(OBJECT(&xhci->mem));
 }
 
 static int usb_xhci_post_load(void *opaque, int version_id)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 1fb868c..633642e 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1139,7 +1139,7 @@ static void vfio_disable_interrupts(VFIOPCIDevice *vdev)
     }
 }
 
-static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos)
+static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
 {
     uint16_t ctrl;
     bool msi_64bit, msi_maskbit;
@@ -1147,6 +1147,7 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos)
 
     if (pread(vdev->vbasedev.fd, &ctrl, sizeof(ctrl),
               vdev->config_offset + pos + PCI_CAP_FLAGS) != sizeof(ctrl)) {
+        error_setg(errp, "Read error!");
         return -errno;
     }
     ctrl = le16_to_cpu(ctrl);
@@ -1157,12 +1158,11 @@ 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, errp);
     if (ret < 0) {
         if (ret == -ENOTSUP) {
             return 0;
         }
-        error_report("vfio: msi_init failed");
         return ret;
     }
     vdev->msi_cap_size = 0xa + (msi_maskbit ? 0xa : 0) + (msi_64bit ? 0x4 : 0);
@@ -1654,7 +1654,7 @@ static void vfio_check_af_flr(VFIOPCIDevice *vdev, uint8_t pos)
     }
 }
 
-static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
+static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos, Error **errp)
 {
     PCIDevice *pdev = &vdev->pdev;
     uint8_t cap_id, next, size;
@@ -1679,7 +1679,7 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
      * will be changed as we unwind the stack.
      */
     if (next) {
-        ret = vfio_add_std_cap(vdev, next);
+        ret = vfio_add_std_cap(vdev, next, errp);
         if (ret) {
             return ret;
         }
@@ -1695,7 +1695,7 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
 
     switch (cap_id) {
     case PCI_CAP_ID_MSI:
-        ret = vfio_msi_setup(vdev, pos);
+        ret = vfio_msi_setup(vdev, pos, errp);
         break;
     case PCI_CAP_ID_EXP:
         vfio_check_pcie_flr(vdev, pos);
@@ -1729,7 +1729,7 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
     return 0;
 }
 
-static int vfio_add_capabilities(VFIOPCIDevice *vdev)
+static int vfio_add_capabilities(VFIOPCIDevice *vdev, Error **errp)
 {
     PCIDevice *pdev = &vdev->pdev;
 
@@ -1738,7 +1738,7 @@ static int vfio_add_capabilities(VFIOPCIDevice *vdev)
         return 0; /* Nothing to add */
     }
 
-    return vfio_add_std_cap(vdev, pdev->config[PCI_CAPABILITY_LIST]);
+    return vfio_add_std_cap(vdev, pdev->config[PCI_CAPABILITY_LIST], errp);
 }
 
 static void vfio_pci_pre_reset(VFIOPCIDevice *vdev)
@@ -2349,6 +2349,7 @@ static int vfio_initfn(PCIDevice *pdev)
     struct stat st;
     int groupid;
     int ret;
+    Error *local_err = NULL;
 
     /* Check that the host device exists */
     snprintf(path, sizeof(path),
@@ -2507,8 +2508,9 @@ static int vfio_initfn(PCIDevice *pdev)
 
     vfio_map_bars(vdev);
 
-    ret = vfio_add_capabilities(vdev);
+    ret = vfio_add_capabilities(vdev, &local_err);
     if (ret) {
+        error_report_err(local_err);
         goto out_teardown;
     }
 
diff --git a/include/hw/pci/msi.h b/include/hw/pci/msi.h
index 50e452b..da1dc1a 100644
--- a/include/hw/pci/msi.h
+++ b/include/hw/pci/msi.h
@@ -34,8 +34,8 @@ extern bool msi_supported;
 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);
+int msi_init(struct PCIDevice *dev, uint8_t offset, 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 RFC v2 1/2] Add param Error** to msi_init() & modify the callers

[Markus Armbruster]

This got lost over the Christmas break, sorry.

Cc'ing Marcel for additional PCI expertise.

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

> msi_init() is a supporting function in PCI device initialization,
> in order to convert .init() to .realize(), it should be modified first.

"Supporting function" doesn't imply "should use Error to report
errors".  HACKING explains:

    Use the simplest suitable method to communicate success / failure to
    callers.  Stick to common methods: non-negative on success / -1 on
    error, non-negative / -errno, non-null / null, or Error objects.

    Example: when a function returns a non-null pointer on success, and it
    can fail only in one way (as far as the caller is concerned), returning
    null on failure is just fine, and certainly simpler and a lot easier on
    the eyes than propagating an Error object through an Error ** parameter.

    Example: when a function's callers need to report details on failure
    only the function really knows, use Error **, and set suitable errors.

    Do not report an error to the user when you're also returning an error
    for somebody else to handle.  Leave the reporting to the place that
    consumes the error returned.

As we'll see in your patch of msi.c, msi_init() can fail in multiple
ways, and uses -errno to comunicate them.  That can be okay even in
realize().  It also reports to the user.  That's what makes it
unsuitable for realize().

> Also modify the callers

Actually, you're *fixing* callers!  But the bugs aren't 100% clear, yet,
see below for details.  Once we know what the bugs are, we'll want to
reword the commit message to describe the bugs and their impact.

I recommend to skip ahead to msi.c, then come back to the device models.

> Bonus: add more comment for msi_init().
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/audio/intel-hda.c               | 10 ++++-
>  hw/ide/ich.c                       |  2 +-
>  hw/net/vmxnet3.c                   | 13 +++---
>  hw/pci-bridge/ioh3420.c            |  7 +++-
>  hw/pci-bridge/pci_bridge_dev.c     |  8 +++-
>  hw/pci-bridge/xio3130_downstream.c |  8 +++-
>  hw/pci-bridge/xio3130_upstream.c   |  8 +++-
>  hw/pci/msi.c                       | 18 +++++++--
>  hw/scsi/megasas.c                  | 15 +++++--
>  hw/scsi/vmw_pvscsi.c               | 13 ++++--
>  hw/usb/hcd-xhci.c                  | 81 +++++++++++++++++++++-----------------
>  hw/vfio/pci.c                      | 20 +++++-----
>  include/hw/pci/msi.h               |  4 +-
>  13 files changed, 135 insertions(+), 72 deletions(-)
>
> diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
> index 433463e..0d770131 100644
> --- a/hw/audio/intel-hda.c
> +++ b/hw/audio/intel-hda.c
> @@ -1130,6 +1130,7 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp)
>  {
>      IntelHDAState *d = INTEL_HDA(pci);
>      uint8_t *conf = d->pci.config;
> +    int ret;
>  
>      d->name = object_get_typename(OBJECT(d));
>  
> @@ -1142,11 +1143,18 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp)
>                            "intel-hda", 0x4000);
>      pci_register_bar(&d->pci, 0, 0, &d->mmio);
>      if (d->msi) {
> -        msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, 1, true, false);
> +        ret = msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, 1, true,
> +                false, errp);
> +        if (ret < 0) {
> +            goto cleanup_on_msi_fail;
> +        }
>      }
>  
>      hda_codec_bus_init(DEVICE(pci), &d->codecs, sizeof(d->codecs),
>                         intel_hda_response, intel_hda_xfer);
> +
> +cleanup_on_msi_fail:
> +    object_unref(OBJECT(&d->mmio));
>  }
>  

This is a bug fix.  Two bugs, actually.

Bug#1: we ignore msi_init() failure.  If it fails because the board
doesn't support MSI, the failure is ignored silently.  If it fails
because the PCI config space offset is already occupied, we report the
error to the user, but still ignore it.  The latter feels like a
programming error to me.

Your patch fixes realize() to fail when msi_init() fails.

Bug#2: we report errors with error_report_err() instead through errp.
This is because we use pci_add_capability() instead of
pci_add_capability2().

I don't have the time to review the other devices you patch.  Both bugs
should be easy to spot in the patch: if the value of msi_init() is
ignored before the patch, the device got bug#1, and if the patched
device uses realize(), it got bug#2.

The patch could be split up into parts that fix just one thing.  Not
sure that's worth the bother.

>  static void intel_hda_exit(PCIDevice *pci)
> diff --git a/hw/ide/ich.c b/hw/ide/ich.c
> index 16925fa..94b1809 100644
> --- a/hw/ide/ich.c
> +++ b/hw/ide/ich.c
> @@ -145,7 +145,7 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp)
>      /* 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);
> +    msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false, errp);
>  }
>  
>  static void pci_ich9_uninit(PCIDevice *dev)
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 37373e5..c373e77 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -2135,13 +2135,13 @@ vmxnet3_cleanup_msix(VMXNET3State *s)
>  #define VMXNET3_PER_VECTOR_MASK   (false)
>  
>  static bool
> -vmxnet3_init_msi(VMXNET3State *s)
> +vmxnet3_init_msi(VMXNET3State *s, Error **errp)
>  {
>      PCIDevice *d = PCI_DEVICE(s);
>      int res;
>  
>      res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MAX_NMSIX_INTRS,
> -                   VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK);
> +                   VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK, errp);
>      if (0 > res) {
>          VMW_WRPRN("Failed to initialize MSI, error %d", res);
>          s->msi_used = false;
> @@ -2204,6 +2204,11 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
>  
>      VMW_CBPRN("Starting init...");
>  
> +    if (!vmxnet3_init_msi(s, errp)) {
> +        VMW_WRPRN("Failed to initialize MSI, configuration is inconsistent.");
> +        return;
> +    }
> +
>      memory_region_init_io(&s->bar0, OBJECT(s), &b0_ops, s,
>                            "vmxnet3-b0", VMXNET3_PT_REG_SIZE);
>      pci_register_bar(pci_dev, VMXNET3_BAR0_IDX,
> @@ -2228,10 +2233,6 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
>          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);
>  
>      register_savevm(dev, "vmxnet3-msix", -1, 1,
> diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
> index eead195..5df9e83 100644
> --- a/hw/pci-bridge/ioh3420.c
> +++ b/hw/pci-bridge/ioh3420.c
> @@ -96,6 +96,7 @@ static int ioh3420_initfn(PCIDevice *d)
>      PCIEPort *p = PCIE_PORT(d);
>      PCIESlot *s = PCIE_SLOT(d);
>      int rc;
> +    Error *local_err = NULL;
>  
>      pci_bridge_initfn(d, TYPE_PCIE_BUS);
>      pcie_port_init_reg(d);
> @@ -105,12 +106,16 @@ 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);
> +                  IOH_EP_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT,
> +                  &local_err);
>      if (rc < 0) {
> +        error_report_err(local_err);
>          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;
> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
> index bc3e1b7..bafaeeb 100644
> --- a/hw/pci-bridge/pci_bridge_dev.c
> +++ b/hw/pci-bridge/pci_bridge_dev.c
> @@ -51,6 +51,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);
>  
> @@ -66,17 +67,21 @@ 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_supported) {
> -        err = msi_init(dev, 0, 1, true, true);
> +        err = msi_init(dev, 0, 1, true, true, &local_err);
>          if (err < 0) {
> +            error_report_err(local_err);
>              goto msi_error;
>          }
>      }
> +
>      if (shpc_present(dev)) {
>          /* TODO: spec recommends using 64 bit prefetcheable BAR.
>           * Check whether that works well. */
> @@ -84,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 b4dd25f..3fc7455 100644
> --- a/hw/pci-bridge/xio3130_downstream.c
> +++ b/hw/pci-bridge/xio3130_downstream.c
> @@ -59,21 +59,26 @@ static int xio3130_downstream_initfn(PCIDevice *d)
>      PCIEPort *p = PCIE_PORT(d);
>      PCIESlot *s = PCIE_SLOT(d);
>      int rc;
> +    Error *local_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,
> +                  &local_err);
>      if (rc < 0) {
> +        error_report_err(local_err);
>          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) {
> @@ -103,6 +108,7 @@ err_msi:
>      msi_uninit(d);
>  err_bridge:
>      pci_bridge_exitfn(d);
> +
>      return rc;
>  }
>  
> diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c
> index 434c8fd..882271c 100644
> --- a/hw/pci-bridge/xio3130_upstream.c
> +++ b/hw/pci-bridge/xio3130_upstream.c
> @@ -55,26 +55,32 @@ static int xio3130_upstream_initfn(PCIDevice *d)
>  {
>      PCIEPort *p = PCIE_PORT(d);
>      int rc;
> +    Error *local_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,
> +                  &local_err);
>      if (rc < 0) {
> +        error_report_err(local_err);
>          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) {
>          goto err_msi;
>      }
> +
>      pcie_cap_flr_init(d);
>      pcie_cap_deverr_init(d);
>      rc = pcie_aer_init(d, XIO3130_AER_OFFSET);
> diff --git a/hw/pci/msi.c b/hw/pci/msi.c
> index c1dd531..ad6ed09 100644
> --- a/hw/pci/msi.c
> +++ b/hw/pci/msi.c
> @@ -150,15 +150,23 @@ bool msi_enabled(const PCIDevice *dev)
>           PCI_MSI_FLAGS_ENABLE);
>  }
>  
> -int msi_init(struct PCIDevice *dev, uint8_t offset,
> -             unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask)
> +/*
> + * @nr_vectors: Multiple Message Capable field of Message Control register
> + * @msi64bit: support 64-bit message address or not
> + * @msi_per_vector_mask: support per-vector masking or not

Missing: @offset, @errp.

git-grep @errp shows the various ways we document this parameter
elsewhere.  We suck at consistency.  Pick one you like.

> + *
> + * return: MSI capability offset in config space

"... on success, -errno on error" plus an explanation of what the error
codes mean.  If nobody uses the error codes, we could simply return -1
and call it a day.

> + */

The comment follows GTK-Doc conventions, but not quite.

GTK-Doc comments are designed for easy extraction of reference
documentation.  We don't do that.  We use them in a few places anyway,
such as object.h.  The majority of the code doesn't use them.  Their use
is up to the maintainer.  I detest them, and keep them out of the
subsystems I maintain.  PCI maintainer's call.

Prior discussion:
http://lists.nongnu.org/archive/html/qemu-devel/2015-05/msg03368.html

Here's how I'd write this one:

/*
 * 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.
 * @errp is for returning errors.
 * Return the offset of capability MSI in config space on success,
 * set @errp and return -errno on error.
 * FIXME explain the error codes, or dumb down return value to -1
 */

Except I'm not sure the function should fail in the first place!  See
below.

> +int msi_init(struct PCIDevice *dev, uint8_t offset, unsigned int nr_vectors,
> +             bool msi64bit, bool msi_per_vector_mask, Error **errp)
>  {
>      unsigned int vectors_order;
> -    uint16_t flags;
> +    uint16_t flags; /* Message Control register value */
>      uint8_t cap_size;
>      int config_offset;
>  
>      if (!msi_supported) {
> +        error_setg(errp, "MSI is not supported by interrupt controller");
>          return -ENOTSUP;

First failure mode: board does not support MSI (-ENOTSUP).

Question to the PCI guys: why is this even an error?  A device with
capability MSI should work just fine in such a board.

>      }
>  
> @@ -182,7 +190,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);

pci_add_capability() is a wrapper around pci_add_capability2() that
additionally reports errors with error_report_err().  This is what makes
it unsuitable for realize().

>      if (config_offset < 0) {
>          return config_offset;

Inherits failing modes from pci_add_capability2().  Two: out of PCI
config space (-ENOSPC), and capability overlap (-EINVAL).

Question for the PCI guys: how can these happen?  When they happen, is
it a programming error?

>      }
> @@ -205,6 +214,7 @@ 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;
>  }
>  
> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index d7dc667..ba25b3a 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -2339,6 +2339,17 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
>      /* Interrupt pin 1 */
>      pci_conf[PCI_INTERRUPT_PIN] = 0x01;
>  
> +    if (megasas_use_msi(s)) {
> +        int ret;
> +
> +        ret = msi_init(dev, 0x50, 1, true, false, errp);
> +        if (ret > 0) {
> +            s->flags &= ~MEGASAS_MASK_USE_MSI;
> +        } else {
> +            return;
> +        }
> +    }
> +
>      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,
> @@ -2346,10 +2357,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->flags &= ~MEGASAS_MASK_USE_MSI;
> -    }
>      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 9c71f31..073b956 100644
> --- a/hw/scsi/vmw_pvscsi.c
> +++ b/hw/scsi/vmw_pvscsi.c
> @@ -1014,13 +1014,13 @@ pvscsi_io_read(void *opaque, hwaddr addr, unsigned size)
>  
>  
>  static bool
> -pvscsi_init_msi(PVSCSIState *s)
> +pvscsi_init_msi(PVSCSIState *s, Error **errp)
>  {
>      int res;
>      PCIDevice *d = PCI_DEVICE(s);
>  
>      res = msi_init(d, PVSCSI_MSI_OFFSET, PVSCSI_MSIX_NUM_VECTORS,
> -                   PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK);
> +                   PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK, errp);
>      if (res < 0) {
>          trace_pvscsi_init_msi_fail(res);
>          s->msi_used = false;
> @@ -1066,6 +1066,7 @@ static int
>  pvscsi_init(PCIDevice *pci_dev)
>  {
>      PVSCSIState *s = PVSCSI(pci_dev);
> +    Error *local_err = NULL;
>  
>      trace_pvscsi_state("init");
>  
> @@ -1079,12 +1080,16 @@ pvscsi_init(PCIDevice *pci_dev)
>      /* Interrupt pin A */
>      pci_config_set_interrupt_pin(pci_dev->config, 1);
>  
> +    pvscsi_init_msi(s, &local_err);
> +    if (local_err) {
> +        error_report_err(local_err);
> +        return -1;
> +    }
> +
>      memory_region_init_io(&s->io_space, OBJECT(s), &pvscsi_ops, s,
>                            "pvscsi-io", PVSCSI_MEM_SPACE_SIZE);
>      pci_register_bar(pci_dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->io_space);
>  
> -    pvscsi_init_msi(s);
> -
>      s->completion_worker = qemu_bh_new(pvscsi_process_completion_queue, s);
>      if (!s->completion_worker) {
>          pvscsi_cleanup_msi(s);
> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> index 268ab36..7cd5f6c 100644
> --- a/hw/usb/hcd-xhci.c
> +++ b/hw/usb/hcd-xhci.c
> @@ -3572,6 +3572,43 @@ static void usb_xhci_init(XHCIState *xhci)
>      }
>  }
>  
> +static void usb_xhci_exit(PCIDevice *dev)
> +{
> +    int i;
> +    XHCIState *xhci = XHCI(dev);
> +
> +    trace_usb_xhci_exit();
> +
> +    for (i = 0; i < xhci->numslots; i++) {
> +        xhci_disable_slot(xhci, i + 1);
> +    }
> +
> +    if (xhci->mfwrap_timer) {
> +        timer_del(xhci->mfwrap_timer);
> +        timer_free(xhci->mfwrap_timer);
> +        xhci->mfwrap_timer = NULL;
> +    }
> +
> +    memory_region_del_subregion(&xhci->mem, &xhci->mem_cap);
> +    memory_region_del_subregion(&xhci->mem, &xhci->mem_oper);
> +    memory_region_del_subregion(&xhci->mem, &xhci->mem_runtime);
> +    memory_region_del_subregion(&xhci->mem, &xhci->mem_doorbell);
> +
> +    for (i = 0; i < xhci->numports; i++) {
> +        XHCIPort *port = &xhci->ports[i];
> +        memory_region_del_subregion(&xhci->mem, &port->mem);
> +    }
> +
> +    /* destroy msix memory region */
> +    if (dev->msix_table && dev->msix_pba
> +        && dev->msix_entry_used) {
> +        memory_region_del_subregion(&xhci->mem, &dev->msix_table_mmio);
> +        memory_region_del_subregion(&xhci->mem, &dev->msix_pba_mmio);
> +    }
> +
> +    usb_bus_release(&xhci->bus);
> +}
> +
>  static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
>  {
>      int i, ret;
> @@ -3643,7 +3680,10 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
>      }
>  
>      if (xhci_get_flag(xhci, XHCI_FLAG_USE_MSI)) {
> -        msi_init(dev, 0x70, xhci->numintrs, true, false);
> +        ret = msi_init(dev, 0x70, xhci->numintrs, true, false, errp);
> +        if (ret < 0) {
> +            goto cleanup_on_msi_fail;
> +        }
>      }
>      if (xhci_get_flag(xhci, XHCI_FLAG_USE_MSI_X)) {
>          msix_init(dev, xhci->numintrs,
> @@ -3651,43 +3691,10 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
>                    &xhci->mem, 0, OFF_MSIX_PBA,
>                    0x90);
>      }
> -}
> -
> -static void usb_xhci_exit(PCIDevice *dev)
> -{
> -    int i;
> -    XHCIState *xhci = XHCI(dev);
> -
> -    trace_usb_xhci_exit();
> -
> -    for (i = 0; i < xhci->numslots; i++) {
> -        xhci_disable_slot(xhci, i + 1);
> -    }
> -
> -    if (xhci->mfwrap_timer) {
> -        timer_del(xhci->mfwrap_timer);
> -        timer_free(xhci->mfwrap_timer);
> -        xhci->mfwrap_timer = NULL;
> -    }
>  
> -    memory_region_del_subregion(&xhci->mem, &xhci->mem_cap);
> -    memory_region_del_subregion(&xhci->mem, &xhci->mem_oper);
> -    memory_region_del_subregion(&xhci->mem, &xhci->mem_runtime);
> -    memory_region_del_subregion(&xhci->mem, &xhci->mem_doorbell);
> -
> -    for (i = 0; i < xhci->numports; i++) {
> -        XHCIPort *port = &xhci->ports[i];
> -        memory_region_del_subregion(&xhci->mem, &port->mem);
> -    }
> -
> -    /* destroy msix memory region */
> -    if (dev->msix_table && dev->msix_pba
> -        && dev->msix_entry_used) {
> -        memory_region_del_subregion(&xhci->mem, &dev->msix_table_mmio);
> -        memory_region_del_subregion(&xhci->mem, &dev->msix_pba_mmio);
> -    }
> -
> -    usb_bus_release(&xhci->bus);
> +cleanup_on_msi_fail:
> +    usb_xhci_exit(dev);
> +    object_unref(OBJECT(&xhci->mem));
>  }
>  
>  static int usb_xhci_post_load(void *opaque, int version_id)
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 1fb868c..633642e 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -1139,7 +1139,7 @@ static void vfio_disable_interrupts(VFIOPCIDevice *vdev)
>      }
>  }
>  
> -static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos)
> +static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
>  {
>      uint16_t ctrl;
>      bool msi_64bit, msi_maskbit;
> @@ -1147,6 +1147,7 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos)
>  
>      if (pread(vdev->vbasedev.fd, &ctrl, sizeof(ctrl),
>                vdev->config_offset + pos + PCI_CAP_FLAGS) != sizeof(ctrl)) {
> +        error_setg(errp, "Read error!");
>          return -errno;
>      }
>      ctrl = le16_to_cpu(ctrl);
> @@ -1157,12 +1158,11 @@ 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, errp);
>      if (ret < 0) {
>          if (ret == -ENOTSUP) {
>              return 0;
>          }
> -        error_report("vfio: msi_init failed");
>          return ret;
>      }
>      vdev->msi_cap_size = 0xa + (msi_maskbit ? 0xa : 0) + (msi_64bit ? 0x4 : 0);
> @@ -1654,7 +1654,7 @@ static void vfio_check_af_flr(VFIOPCIDevice *vdev, uint8_t pos)
>      }
>  }
>  
> -static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
> +static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos, Error **errp)
>  {
>      PCIDevice *pdev = &vdev->pdev;
>      uint8_t cap_id, next, size;
> @@ -1679,7 +1679,7 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
>       * will be changed as we unwind the stack.
>       */
>      if (next) {
> -        ret = vfio_add_std_cap(vdev, next);
> +        ret = vfio_add_std_cap(vdev, next, errp);
>          if (ret) {
>              return ret;
>          }
> @@ -1695,7 +1695,7 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
>  
>      switch (cap_id) {
>      case PCI_CAP_ID_MSI:
> -        ret = vfio_msi_setup(vdev, pos);
> +        ret = vfio_msi_setup(vdev, pos, errp);
>          break;
>      case PCI_CAP_ID_EXP:
>          vfio_check_pcie_flr(vdev, pos);
> @@ -1729,7 +1729,7 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
>      return 0;
>  }
>  
> -static int vfio_add_capabilities(VFIOPCIDevice *vdev)
> +static int vfio_add_capabilities(VFIOPCIDevice *vdev, Error **errp)
>  {
>      PCIDevice *pdev = &vdev->pdev;
>  
> @@ -1738,7 +1738,7 @@ static int vfio_add_capabilities(VFIOPCIDevice *vdev)
>          return 0; /* Nothing to add */
>      }
>  
> -    return vfio_add_std_cap(vdev, pdev->config[PCI_CAPABILITY_LIST]);
> +    return vfio_add_std_cap(vdev, pdev->config[PCI_CAPABILITY_LIST], errp);
>  }
>  
>  static void vfio_pci_pre_reset(VFIOPCIDevice *vdev)
> @@ -2349,6 +2349,7 @@ static int vfio_initfn(PCIDevice *pdev)
>      struct stat st;
>      int groupid;
>      int ret;
> +    Error *local_err = NULL;
>  
>      /* Check that the host device exists */
>      snprintf(path, sizeof(path),
> @@ -2507,8 +2508,9 @@ static int vfio_initfn(PCIDevice *pdev)
>  
>      vfio_map_bars(vdev);
>  
> -    ret = vfio_add_capabilities(vdev);
> +    ret = vfio_add_capabilities(vdev, &local_err);
>      if (ret) {
> +        error_report_err(local_err);
>          goto out_teardown;
>      }
>  
> diff --git a/include/hw/pci/msi.h b/include/hw/pci/msi.h
> index 50e452b..da1dc1a 100644
> --- a/include/hw/pci/msi.h
> +++ b/include/hw/pci/msi.h
> @@ -34,8 +34,8 @@ extern bool msi_supported;
>  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);
> +int msi_init(struct PCIDevice *dev, uint8_t offset, 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);

Re: [Qemu-devel] [PATCH RFC v2 1/2] Add param Error** to msi_init() & modify the callers

[Cao jin]

hi, Markus
Thanks for still remembering this patch, and quite a lot response:)
I will give a appropriate response after I read & understand them 
all.(so, not cc other guys here)

On 03/02/2016 05:13 PM, Markus Armbruster wrote:
> This got lost over the Christmas break, sorry.
>
> Cc'ing Marcel for additional PCI expertise.
>
> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>
>> msi_init() is a supporting function in PCI device initialization,
>> in order to convert .init() to .realize(), it should be modified first.
>
> "Supporting function" doesn't imply "should use Error to report
> errors".  HACKING explains:
>
>      Use the simplest suitable method to communicate success / failure to
>      callers.  Stick to common methods: non-negative on success / -1 on
>      error, non-negative / -errno, non-null / null, or Error objects.
>
>      Example: when a function returns a non-null pointer on success, and it
>      can fail only in one way (as far as the caller is concerned), returning
>      null on failure is just fine, and certainly simpler and a lot easier on
>      the eyes than propagating an Error object through an Error ** parameter.
>
>      Example: when a function's callers need to report details on failure
>      only the function really knows, use Error **, and set suitable errors.
>
>      Do not report an error to the user when you're also returning an error
>      for somebody else to handle.  Leave the reporting to the place that
>      consumes the error returned.
>
> As we'll see in your patch of msi.c, msi_init() can fail in multiple
> ways, and uses -errno to comunicate them.  That can be okay even in
> realize().  It also reports to the user.  That's what makes it
> unsuitable for realize().
>
>> Also modify the callers
>
> Actually, you're *fixing* callers!  But the bugs aren't 100% clear, yet,
> see below for details.  Once we know what the bugs are, we'll want to
> reword the commit message to describe the bugs and their impact.
>
> I recommend to skip ahead to msi.c, then come back to the device models.
>
>> Bonus: add more comment for msi_init().
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>>   hw/audio/intel-hda.c               | 10 ++++-
>>   hw/ide/ich.c                       |  2 +-
>>   hw/net/vmxnet3.c                   | 13 +++---
>>   hw/pci-bridge/ioh3420.c            |  7 +++-
>>   hw/pci-bridge/pci_bridge_dev.c     |  8 +++-
>>   hw/pci-bridge/xio3130_downstream.c |  8 +++-
>>   hw/pci-bridge/xio3130_upstream.c   |  8 +++-
>>   hw/pci/msi.c                       | 18 +++++++--
>>   hw/scsi/megasas.c                  | 15 +++++--
>>   hw/scsi/vmw_pvscsi.c               | 13 ++++--
>>   hw/usb/hcd-xhci.c                  | 81 +++++++++++++++++++++-----------------
>>   hw/vfio/pci.c                      | 20 +++++-----
>>   include/hw/pci/msi.h               |  4 +-
>>   13 files changed, 135 insertions(+), 72 deletions(-)
>>
>> diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
>> index 433463e..0d770131 100644
>> --- a/hw/audio/intel-hda.c
>> +++ b/hw/audio/intel-hda.c
>> @@ -1130,6 +1130,7 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp)
>>   {
>>       IntelHDAState *d = INTEL_HDA(pci);
>>       uint8_t *conf = d->pci.config;
>> +    int ret;
>>
>>       d->name = object_get_typename(OBJECT(d));
>>
>> @@ -1142,11 +1143,18 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp)
>>                             "intel-hda", 0x4000);
>>       pci_register_bar(&d->pci, 0, 0, &d->mmio);
>>       if (d->msi) {
>> -        msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, 1, true, false);
>> +        ret = msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, 1, true,
>> +                false, errp);
>> +        if (ret < 0) {
>> +            goto cleanup_on_msi_fail;
>> +        }
>>       }
>>
>>       hda_codec_bus_init(DEVICE(pci), &d->codecs, sizeof(d->codecs),
>>                          intel_hda_response, intel_hda_xfer);
>> +
>> +cleanup_on_msi_fail:
>> +    object_unref(OBJECT(&d->mmio));
>>   }
>>
>
> This is a bug fix.  Two bugs, actually.
>
> Bug#1: we ignore msi_init() failure.  If it fails because the board
> doesn't support MSI, the failure is ignored silently.  If it fails
> because the PCI config space offset is already occupied, we report the
> error to the user, but still ignore it.  The latter feels like a
> programming error to me.
>
> Your patch fixes realize() to fail when msi_init() fails.
>
> Bug#2: we report errors with error_report_err() instead through errp.
> This is because we use pci_add_capability() instead of
> pci_add_capability2().
>
> I don't have the time to review the other devices you patch.  Both bugs
> should be easy to spot in the patch: if the value of msi_init() is
> ignored before the patch, the device got bug#1, and if the patched
> device uses realize(), it got bug#2.
>
> The patch could be split up into parts that fix just one thing.  Not
> sure that's worth the bother.
>
>>   static void intel_hda_exit(PCIDevice *pci)
>> diff --git a/hw/ide/ich.c b/hw/ide/ich.c
>> index 16925fa..94b1809 100644
>> --- a/hw/ide/ich.c
>> +++ b/hw/ide/ich.c
>> @@ -145,7 +145,7 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp)
>>       /* 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);
>> +    msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false, errp);
>>   }
>>
>>   static void pci_ich9_uninit(PCIDevice *dev)
>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
>> index 37373e5..c373e77 100644
>> --- a/hw/net/vmxnet3.c
>> +++ b/hw/net/vmxnet3.c
>> @@ -2135,13 +2135,13 @@ vmxnet3_cleanup_msix(VMXNET3State *s)
>>   #define VMXNET3_PER_VECTOR_MASK   (false)
>>
>>   static bool
>> -vmxnet3_init_msi(VMXNET3State *s)
>> +vmxnet3_init_msi(VMXNET3State *s, Error **errp)
>>   {
>>       PCIDevice *d = PCI_DEVICE(s);
>>       int res;
>>
>>       res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MAX_NMSIX_INTRS,
>> -                   VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK);
>> +                   VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK, errp);
>>       if (0 > res) {
>>           VMW_WRPRN("Failed to initialize MSI, error %d", res);
>>           s->msi_used = false;
>> @@ -2204,6 +2204,11 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
>>
>>       VMW_CBPRN("Starting init...");
>>
>> +    if (!vmxnet3_init_msi(s, errp)) {
>> +        VMW_WRPRN("Failed to initialize MSI, configuration is inconsistent.");
>> +        return;
>> +    }
>> +
>>       memory_region_init_io(&s->bar0, OBJECT(s), &b0_ops, s,
>>                             "vmxnet3-b0", VMXNET3_PT_REG_SIZE);
>>       pci_register_bar(pci_dev, VMXNET3_BAR0_IDX,
>> @@ -2228,10 +2233,6 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
>>           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);
>>
>>       register_savevm(dev, "vmxnet3-msix", -1, 1,
>> diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
>> index eead195..5df9e83 100644
>> --- a/hw/pci-bridge/ioh3420.c
>> +++ b/hw/pci-bridge/ioh3420.c
>> @@ -96,6 +96,7 @@ static int ioh3420_initfn(PCIDevice *d)
>>       PCIEPort *p = PCIE_PORT(d);
>>       PCIESlot *s = PCIE_SLOT(d);
>>       int rc;
>> +    Error *local_err = NULL;
>>
>>       pci_bridge_initfn(d, TYPE_PCIE_BUS);
>>       pcie_port_init_reg(d);
>> @@ -105,12 +106,16 @@ 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);
>> +                  IOH_EP_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT,
>> +                  &local_err);
>>       if (rc < 0) {
>> +        error_report_err(local_err);
>>           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;
>> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
>> index bc3e1b7..bafaeeb 100644
>> --- a/hw/pci-bridge/pci_bridge_dev.c
>> +++ b/hw/pci-bridge/pci_bridge_dev.c
>> @@ -51,6 +51,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);
>>
>> @@ -66,17 +67,21 @@ 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_supported) {
>> -        err = msi_init(dev, 0, 1, true, true);
>> +        err = msi_init(dev, 0, 1, true, true, &local_err);
>>           if (err < 0) {
>> +            error_report_err(local_err);
>>               goto msi_error;
>>           }
>>       }
>> +
>>       if (shpc_present(dev)) {
>>           /* TODO: spec recommends using 64 bit prefetcheable BAR.
>>            * Check whether that works well. */
>> @@ -84,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 b4dd25f..3fc7455 100644
>> --- a/hw/pci-bridge/xio3130_downstream.c
>> +++ b/hw/pci-bridge/xio3130_downstream.c
>> @@ -59,21 +59,26 @@ static int xio3130_downstream_initfn(PCIDevice *d)
>>       PCIEPort *p = PCIE_PORT(d);
>>       PCIESlot *s = PCIE_SLOT(d);
>>       int rc;
>> +    Error *local_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,
>> +                  &local_err);
>>       if (rc < 0) {
>> +        error_report_err(local_err);
>>           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) {
>> @@ -103,6 +108,7 @@ err_msi:
>>       msi_uninit(d);
>>   err_bridge:
>>       pci_bridge_exitfn(d);
>> +
>>       return rc;
>>   }
>>
>> diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c
>> index 434c8fd..882271c 100644
>> --- a/hw/pci-bridge/xio3130_upstream.c
>> +++ b/hw/pci-bridge/xio3130_upstream.c
>> @@ -55,26 +55,32 @@ static int xio3130_upstream_initfn(PCIDevice *d)
>>   {
>>       PCIEPort *p = PCIE_PORT(d);
>>       int rc;
>> +    Error *local_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,
>> +                  &local_err);
>>       if (rc < 0) {
>> +        error_report_err(local_err);
>>           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) {
>>           goto err_msi;
>>       }
>> +
>>       pcie_cap_flr_init(d);
>>       pcie_cap_deverr_init(d);
>>       rc = pcie_aer_init(d, XIO3130_AER_OFFSET);
>> diff --git a/hw/pci/msi.c b/hw/pci/msi.c
>> index c1dd531..ad6ed09 100644
>> --- a/hw/pci/msi.c
>> +++ b/hw/pci/msi.c
>> @@ -150,15 +150,23 @@ bool msi_enabled(const PCIDevice *dev)
>>            PCI_MSI_FLAGS_ENABLE);
>>   }
>>
>> -int msi_init(struct PCIDevice *dev, uint8_t offset,
>> -             unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask)
>> +/*
>> + * @nr_vectors: Multiple Message Capable field of Message Control register
>> + * @msi64bit: support 64-bit message address or not
>> + * @msi_per_vector_mask: support per-vector masking or not
>
> Missing: @offset, @errp.
>
> git-grep @errp shows the various ways we document this parameter
> elsewhere.  We suck at consistency.  Pick one you like.
>
>> + *
>> + * return: MSI capability offset in config space
>
> "... on success, -errno on error" plus an explanation of what the error
> codes mean.  If nobody uses the error codes, we could simply return -1
> and call it a day.
>
>> + */
>
> The comment follows GTK-Doc conventions, but not quite.
>
> GTK-Doc comments are designed for easy extraction of reference
> documentation.  We don't do that.  We use them in a few places anyway,
> such as object.h.  The majority of the code doesn't use them.  Their use
> is up to the maintainer.  I detest them, and keep them out of the
> subsystems I maintain.  PCI maintainer's call.
>
> Prior discussion:
> http://lists.nongnu.org/archive/html/qemu-devel/2015-05/msg03368.html
>
> Here's how I'd write this one:
>
> /*
>   * 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.
>   * @errp is for returning errors.
>   * Return the offset of capability MSI in config space on success,
>   * set @errp and return -errno on error.
>   * FIXME explain the error codes, or dumb down return value to -1
>   */
>
> Except I'm not sure the function should fail in the first place!  See
> below.
>
>> +int msi_init(struct PCIDevice *dev, uint8_t offset, unsigned int nr_vectors,
>> +             bool msi64bit, bool msi_per_vector_mask, Error **errp)
>>   {
>>       unsigned int vectors_order;
>> -    uint16_t flags;
>> +    uint16_t flags; /* Message Control register value */
>>       uint8_t cap_size;
>>       int config_offset;
>>
>>       if (!msi_supported) {
>> +        error_setg(errp, "MSI is not supported by interrupt controller");
>>           return -ENOTSUP;
>
> First failure mode: board does not support MSI (-ENOTSUP).
>
> Question to the PCI guys: why is this even an error?  A device with
> capability MSI should work just fine in such a board.
>
>>       }
>>
>> @@ -182,7 +190,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);
>
> pci_add_capability() is a wrapper around pci_add_capability2() that
> additionally reports errors with error_report_err().  This is what makes
> it unsuitable for realize().
>
>>       if (config_offset < 0) {
>>           return config_offset;
>
> Inherits failing modes from pci_add_capability2().  Two: out of PCI
> config space (-ENOSPC), and capability overlap (-EINVAL).
>
> Question for the PCI guys: how can these happen?  When they happen, is
> it a programming error?
>
>>       }
>> @@ -205,6 +214,7 @@ 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;
>>   }
>>
>> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
>> index d7dc667..ba25b3a 100644
>> --- a/hw/scsi/megasas.c
>> +++ b/hw/scsi/megasas.c
>> @@ -2339,6 +2339,17 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
>>       /* Interrupt pin 1 */
>>       pci_conf[PCI_INTERRUPT_PIN] = 0x01;
>>
>> +    if (megasas_use_msi(s)) {
>> +        int ret;
>> +
>> +        ret = msi_init(dev, 0x50, 1, true, false, errp);
>> +        if (ret > 0) {
>> +            s->flags &= ~MEGASAS_MASK_USE_MSI;
>> +        } else {
>> +            return;
>> +        }
>> +    }
>> +
>>       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,
>> @@ -2346,10 +2357,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->flags &= ~MEGASAS_MASK_USE_MSI;
>> -    }
>>       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 9c71f31..073b956 100644
>> --- a/hw/scsi/vmw_pvscsi.c
>> +++ b/hw/scsi/vmw_pvscsi.c
>> @@ -1014,13 +1014,13 @@ pvscsi_io_read(void *opaque, hwaddr addr, unsigned size)
>>
>>
>>   static bool
>> -pvscsi_init_msi(PVSCSIState *s)
>> +pvscsi_init_msi(PVSCSIState *s, Error **errp)
>>   {
>>       int res;
>>       PCIDevice *d = PCI_DEVICE(s);
>>
>>       res = msi_init(d, PVSCSI_MSI_OFFSET, PVSCSI_MSIX_NUM_VECTORS,
>> -                   PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK);
>> +                   PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK, errp);
>>       if (res < 0) {
>>           trace_pvscsi_init_msi_fail(res);
>>           s->msi_used = false;
>> @@ -1066,6 +1066,7 @@ static int
>>   pvscsi_init(PCIDevice *pci_dev)
>>   {
>>       PVSCSIState *s = PVSCSI(pci_dev);
>> +    Error *local_err = NULL;
>>
>>       trace_pvscsi_state("init");
>>
>> @@ -1079,12 +1080,16 @@ pvscsi_init(PCIDevice *pci_dev)
>>       /* Interrupt pin A */
>>       pci_config_set_interrupt_pin(pci_dev->config, 1);
>>
>> +    pvscsi_init_msi(s, &local_err);
>> +    if (local_err) {
>> +        error_report_err(local_err);
>> +        return -1;
>> +    }
>> +
>>       memory_region_init_io(&s->io_space, OBJECT(s), &pvscsi_ops, s,
>>                             "pvscsi-io", PVSCSI_MEM_SPACE_SIZE);
>>       pci_register_bar(pci_dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->io_space);
>>
>> -    pvscsi_init_msi(s);
>> -
>>       s->completion_worker = qemu_bh_new(pvscsi_process_completion_queue, s);
>>       if (!s->completion_worker) {
>>           pvscsi_cleanup_msi(s);
>> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
>> index 268ab36..7cd5f6c 100644
>> --- a/hw/usb/hcd-xhci.c
>> +++ b/hw/usb/hcd-xhci.c
>> @@ -3572,6 +3572,43 @@ static void usb_xhci_init(XHCIState *xhci)
>>       }
>>   }
>>
>> +static void usb_xhci_exit(PCIDevice *dev)
>> +{
>> +    int i;
>> +    XHCIState *xhci = XHCI(dev);
>> +
>> +    trace_usb_xhci_exit();
>> +
>> +    for (i = 0; i < xhci->numslots; i++) {
>> +        xhci_disable_slot(xhci, i + 1);
>> +    }
>> +
>> +    if (xhci->mfwrap_timer) {
>> +        timer_del(xhci->mfwrap_timer);
>> +        timer_free(xhci->mfwrap_timer);
>> +        xhci->mfwrap_timer = NULL;
>> +    }
>> +
>> +    memory_region_del_subregion(&xhci->mem, &xhci->mem_cap);
>> +    memory_region_del_subregion(&xhci->mem, &xhci->mem_oper);
>> +    memory_region_del_subregion(&xhci->mem, &xhci->mem_runtime);
>> +    memory_region_del_subregion(&xhci->mem, &xhci->mem_doorbell);
>> +
>> +    for (i = 0; i < xhci->numports; i++) {
>> +        XHCIPort *port = &xhci->ports[i];
>> +        memory_region_del_subregion(&xhci->mem, &port->mem);
>> +    }
>> +
>> +    /* destroy msix memory region */
>> +    if (dev->msix_table && dev->msix_pba
>> +        && dev->msix_entry_used) {
>> +        memory_region_del_subregion(&xhci->mem, &dev->msix_table_mmio);
>> +        memory_region_del_subregion(&xhci->mem, &dev->msix_pba_mmio);
>> +    }
>> +
>> +    usb_bus_release(&xhci->bus);
>> +}
>> +
>>   static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
>>   {
>>       int i, ret;
>> @@ -3643,7 +3680,10 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
>>       }
>>
>>       if (xhci_get_flag(xhci, XHCI_FLAG_USE_MSI)) {
>> -        msi_init(dev, 0x70, xhci->numintrs, true, false);
>> +        ret = msi_init(dev, 0x70, xhci->numintrs, true, false, errp);
>> +        if (ret < 0) {
>> +            goto cleanup_on_msi_fail;
>> +        }
>>       }
>>       if (xhci_get_flag(xhci, XHCI_FLAG_USE_MSI_X)) {
>>           msix_init(dev, xhci->numintrs,
>> @@ -3651,43 +3691,10 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
>>                     &xhci->mem, 0, OFF_MSIX_PBA,
>>                     0x90);
>>       }
>> -}
>> -
>> -static void usb_xhci_exit(PCIDevice *dev)
>> -{
>> -    int i;
>> -    XHCIState *xhci = XHCI(dev);
>> -
>> -    trace_usb_xhci_exit();
>> -
>> -    for (i = 0; i < xhci->numslots; i++) {
>> -        xhci_disable_slot(xhci, i + 1);
>> -    }
>> -
>> -    if (xhci->mfwrap_timer) {
>> -        timer_del(xhci->mfwrap_timer);
>> -        timer_free(xhci->mfwrap_timer);
>> -        xhci->mfwrap_timer = NULL;
>> -    }
>>
>> -    memory_region_del_subregion(&xhci->mem, &xhci->mem_cap);
>> -    memory_region_del_subregion(&xhci->mem, &xhci->mem_oper);
>> -    memory_region_del_subregion(&xhci->mem, &xhci->mem_runtime);
>> -    memory_region_del_subregion(&xhci->mem, &xhci->mem_doorbell);
>> -
>> -    for (i = 0; i < xhci->numports; i++) {
>> -        XHCIPort *port = &xhci->ports[i];
>> -        memory_region_del_subregion(&xhci->mem, &port->mem);
>> -    }
>> -
>> -    /* destroy msix memory region */
>> -    if (dev->msix_table && dev->msix_pba
>> -        && dev->msix_entry_used) {
>> -        memory_region_del_subregion(&xhci->mem, &dev->msix_table_mmio);
>> -        memory_region_del_subregion(&xhci->mem, &dev->msix_pba_mmio);
>> -    }
>> -
>> -    usb_bus_release(&xhci->bus);
>> +cleanup_on_msi_fail:
>> +    usb_xhci_exit(dev);
>> +    object_unref(OBJECT(&xhci->mem));
>>   }
>>
>>   static int usb_xhci_post_load(void *opaque, int version_id)
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index 1fb868c..633642e 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -1139,7 +1139,7 @@ static void vfio_disable_interrupts(VFIOPCIDevice *vdev)
>>       }
>>   }
>>
>> -static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos)
>> +static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
>>   {
>>       uint16_t ctrl;
>>       bool msi_64bit, msi_maskbit;
>> @@ -1147,6 +1147,7 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos)
>>
>>       if (pread(vdev->vbasedev.fd, &ctrl, sizeof(ctrl),
>>                 vdev->config_offset + pos + PCI_CAP_FLAGS) != sizeof(ctrl)) {
>> +        error_setg(errp, "Read error!");
>>           return -errno;
>>       }
>>       ctrl = le16_to_cpu(ctrl);
>> @@ -1157,12 +1158,11 @@ 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, errp);
>>       if (ret < 0) {
>>           if (ret == -ENOTSUP) {
>>               return 0;
>>           }
>> -        error_report("vfio: msi_init failed");
>>           return ret;
>>       }
>>       vdev->msi_cap_size = 0xa + (msi_maskbit ? 0xa : 0) + (msi_64bit ? 0x4 : 0);
>> @@ -1654,7 +1654,7 @@ static void vfio_check_af_flr(VFIOPCIDevice *vdev, uint8_t pos)
>>       }
>>   }
>>
>> -static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
>> +static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos, Error **errp)
>>   {
>>       PCIDevice *pdev = &vdev->pdev;
>>       uint8_t cap_id, next, size;
>> @@ -1679,7 +1679,7 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
>>        * will be changed as we unwind the stack.
>>        */
>>       if (next) {
>> -        ret = vfio_add_std_cap(vdev, next);
>> +        ret = vfio_add_std_cap(vdev, next, errp);
>>           if (ret) {
>>               return ret;
>>           }
>> @@ -1695,7 +1695,7 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
>>
>>       switch (cap_id) {
>>       case PCI_CAP_ID_MSI:
>> -        ret = vfio_msi_setup(vdev, pos);
>> +        ret = vfio_msi_setup(vdev, pos, errp);
>>           break;
>>       case PCI_CAP_ID_EXP:
>>           vfio_check_pcie_flr(vdev, pos);
>> @@ -1729,7 +1729,7 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
>>       return 0;
>>   }
>>
>> -static int vfio_add_capabilities(VFIOPCIDevice *vdev)
>> +static int vfio_add_capabilities(VFIOPCIDevice *vdev, Error **errp)
>>   {
>>       PCIDevice *pdev = &vdev->pdev;
>>
>> @@ -1738,7 +1738,7 @@ static int vfio_add_capabilities(VFIOPCIDevice *vdev)
>>           return 0; /* Nothing to add */
>>       }
>>
>> -    return vfio_add_std_cap(vdev, pdev->config[PCI_CAPABILITY_LIST]);
>> +    return vfio_add_std_cap(vdev, pdev->config[PCI_CAPABILITY_LIST], errp);
>>   }
>>
>>   static void vfio_pci_pre_reset(VFIOPCIDevice *vdev)
>> @@ -2349,6 +2349,7 @@ static int vfio_initfn(PCIDevice *pdev)
>>       struct stat st;
>>       int groupid;
>>       int ret;
>> +    Error *local_err = NULL;
>>
>>       /* Check that the host device exists */
>>       snprintf(path, sizeof(path),
>> @@ -2507,8 +2508,9 @@ static int vfio_initfn(PCIDevice *pdev)
>>
>>       vfio_map_bars(vdev);
>>
>> -    ret = vfio_add_capabilities(vdev);
>> +    ret = vfio_add_capabilities(vdev, &local_err);
>>       if (ret) {
>> +        error_report_err(local_err);
>>           goto out_teardown;
>>       }
>>
>> diff --git a/include/hw/pci/msi.h b/include/hw/pci/msi.h
>> index 50e452b..da1dc1a 100644
>> --- a/include/hw/pci/msi.h
>> +++ b/include/hw/pci/msi.h
>> @@ -34,8 +34,8 @@ extern bool msi_supported;
>>   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);
>> +int msi_init(struct PCIDevice *dev, uint8_t offset, 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);
>
>
> .
>

-- 
Yours Sincerely,

Cao jin

Re: [Qemu-devel] [PATCH RFC v2 1/2] Add param Error** to msi_init() & modify the callers

[Marcel Apfelbaum]

On 03/02/2016 11:13 AM, Markus Armbruster wrote:
> This got lost over the Christmas break, sorry.
>
> Cc'ing Marcel for additional PCI expertise.
>
> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>
>> msi_init() is a supporting function in PCI device initialization,
>> in order to convert .init() to .realize(), it should be modified first.
>
> "Supporting function" doesn't imply "should use Error to report
> errors".  HACKING explains:
>
>      Use the simplest suitable method to communicate success / failure to
>      callers.  Stick to common methods: non-negative on success / -1 on
>      error, non-negative / -errno, non-null / null, or Error objects.
>
>      Example: when a function returns a non-null pointer on success, and it
>      can fail only in one way (as far as the caller is concerned), returning
>      null on failure is just fine, and certainly simpler and a lot easier on
>      the eyes than propagating an Error object through an Error ** parameter.
>
>      Example: when a function's callers need to report details on failure
>      only the function really knows, use Error **, and set suitable errors.
>
>      Do not report an error to the user when you're also returning an error
>      for somebody else to handle.  Leave the reporting to the place that
>      consumes the error returned.
>
> As we'll see in your patch of msi.c, msi_init() can fail in multiple
> ways, and uses -errno to comunicate them.  That can be okay even in
> realize().  It also reports to the user.  That's what makes it
> unsuitable for realize().
>
>> Also modify the callers
>
> Actually, you're *fixing* callers!  But the bugs aren't 100% clear, yet,
> see below for details.  Once we know what the bugs are, we'll want to
> reword the commit message to describe the bugs and their impact.
>
> I recommend to skip ahead to msi.c, then come back to the device models.
>
>> Bonus: add more comment for msi_init().
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>>   hw/audio/intel-hda.c               | 10 ++++-
>>   hw/ide/ich.c                       |  2 +-
>>   hw/net/vmxnet3.c                   | 13 +++---
>>   hw/pci-bridge/ioh3420.c            |  7 +++-
>>   hw/pci-bridge/pci_bridge_dev.c     |  8 +++-
>>   hw/pci-bridge/xio3130_downstream.c |  8 +++-
>>   hw/pci-bridge/xio3130_upstream.c   |  8 +++-
>>   hw/pci/msi.c                       | 18 +++++++--
>>   hw/scsi/megasas.c                  | 15 +++++--
>>   hw/scsi/vmw_pvscsi.c               | 13 ++++--
>>   hw/usb/hcd-xhci.c                  | 81 +++++++++++++++++++++-----------------
>>   hw/vfio/pci.c                      | 20 +++++-----
>>   include/hw/pci/msi.h               |  4 +-
>>   13 files changed, 135 insertions(+), 72 deletions(-)
>>

[...]

> Except I'm not sure the function should fail in the first place!  See
> below.
>
>> +int msi_init(struct PCIDevice *dev, uint8_t offset, unsigned int nr_vectors,
>> +             bool msi64bit, bool msi_per_vector_mask, Error **errp)
>>   {
>>       unsigned int vectors_order;
>> -    uint16_t flags;
>> +    uint16_t flags; /* Message Control register value */
>>       uint8_t cap_size;
>>       int config_offset;
>>
>>       if (!msi_supported) {
>> +        error_setg(errp, "MSI is not supported by interrupt controller");
>>           return -ENOTSUP;
>
> First failure mode: board does not support MSI (-ENOTSUP).
>
> Question to the PCI guys: why is this even an error?  A device with
> capability MSI should work just fine in such a board.

Hi Markus,

Adding Jan Kiszka, maybe he can help.

That's a fair question. Is there any history for this decision?
The board not supporting MSI has nothing to do with the capability being there.
The HW should not change because the board doe not support it.

The capability should be present but not active.

>
>>       }
>>
>> @@ -182,7 +190,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);
>
> pci_add_capability() is a wrapper around pci_add_capability2() that
> additionally reports errors with error_report_err().  This is what makes
> it unsuitable for realize().
>
>>       if (config_offset < 0) {
>>           return config_offset;
>
> Inherits failing modes from pci_add_capability2().  Two: out of PCI
> config space (-ENOSPC), and capability overlap (-EINVAL).
>
> Question for the PCI guys: how can these happen?  When they happen, is
> it a programming error?

out of PCI config space: a device emulation error, not enough room
for all its capabilities - it seems to be a programming error.

capability overlap: is for device assignment. This checks for a real HW
that is broke. - not a programming error.

>
>>       }
>> @@ -205,6 +214,7 @@ 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;
>>   }
>>


Thanks,
Marcel


[...]

Re: [Qemu-devel] [PATCH RFC v2 1/2] Add param Error** to msi_init() & modify the callers

[Michael S. Tsirkin]

On Thu, Mar 03, 2016 at 12:12:27PM +0200, Marcel Apfelbaum wrote:
> >>+int msi_init(struct PCIDevice *dev, uint8_t offset, unsigned int nr_vectors,
> >>+             bool msi64bit, bool msi_per_vector_mask, Error **errp)
> >>  {
> >>      unsigned int vectors_order;
> >>-    uint16_t flags;
> >>+    uint16_t flags; /* Message Control register value */
> >>      uint8_t cap_size;
> >>      int config_offset;
> >>
> >>      if (!msi_supported) {
> >>+        error_setg(errp, "MSI is not supported by interrupt controller");
> >>          return -ENOTSUP;
> >
> >First failure mode: board does not support MSI (-ENOTSUP).
> >
> >Question to the PCI guys: why is this even an error?  A device with
> >capability MSI should work just fine in such a board.
> 
> Hi Markus,
> 
> Adding Jan Kiszka, maybe he can help.
> 
> That's a fair question. Is there any history for this decision?
> The board not supporting MSI has nothing to do with the capability being there.
> The HW should not change because the board doe not support it.
> 
> The capability should be present but not active.

Digging in git log will tell you why we have the msi_supported flag:

commit 02eb84d0ec97f183ac23ee939403a139e8849b1d ("qemu/pci: MSI-X support functions")

	This is a safety measure to avoid breaking platforms which should support
	MSI-X but currently lack this in the interrupt controller emulation.

in other words, on some platforms we must hide msi support from devices
because otherwise guests will try to use it, and our emulation is
incomplete.

And the conclusion from that is that for msi_init to fail silently is
at the moment the right thing to do.

The only other reason for it to fail is pci config space corruption,
this probably never happens in practice.

-- 
MST

Re: [Qemu-devel] [PATCH RFC v2 1/2] Add param Error** to msi_init() & modify the callers

[Marcel Apfelbaum]

On 03/03/2016 12:45 PM, Michael S. Tsirkin wrote:
> On Thu, Mar 03, 2016 at 12:12:27PM +0200, Marcel Apfelbaum wrote:
>>>> +int msi_init(struct PCIDevice *dev, uint8_t offset, unsigned int nr_vectors,
>>>> +             bool msi64bit, bool msi_per_vector_mask, Error **errp)
>>>>   {
>>>>       unsigned int vectors_order;
>>>> -    uint16_t flags;
>>>> +    uint16_t flags; /* Message Control register value */
>>>>       uint8_t cap_size;
>>>>       int config_offset;
>>>>
>>>>       if (!msi_supported) {
>>>> +        error_setg(errp, "MSI is not supported by interrupt controller");
>>>>           return -ENOTSUP;
>>>
>>> First failure mode: board does not support MSI (-ENOTSUP).
>>>
>>> Question to the PCI guys: why is this even an error?  A device with
>>> capability MSI should work just fine in such a board.
>>
>> Hi Markus,
>>
>> Adding Jan Kiszka, maybe he can help.
>>
>> That's a fair question. Is there any history for this decision?
>> The board not supporting MSI has nothing to do with the capability being there.
>> The HW should not change because the board doe not support it.
>>
>> The capability should be present but not active.
>
> Digging in git log will tell you why we have the msi_supported flag:
>
> commit 02eb84d0ec97f183ac23ee939403a139e8849b1d ("qemu/pci: MSI-X support functions")
>
> 	This is a safety measure to avoid breaking platforms which should support
> 	MSI-X but currently lack this in the interrupt controller emulation.
>
> in other words, on some platforms we must hide msi support from devices
> because otherwise guests will try to use it, and our emulation is
> incomplete.


OK, thanks. So the flag should be "msi_broken" or "msi_present_but_not_implemented" and not
"msi_supported" that leads (at least me) to the assumption that some platform *does not support msi*
rather than it supports it, but we don't emulate it.


>
> And the conclusion from that is that for msi_init to fail silently is
> at the moment the right thing to do.

But this is not the only thing we do, we are modifying the PCI devices. We could fail starting the VM
if a device supporting MSI is added on a platform with broken msi, but this will prevent us to use
assigned devices. Emulated devices should be created with a specific "msi=off" flag.

Thanks,
Marcel

>
> The only other reason for it to fail is pci config space corruption,
> this probably never happens in practice.
>

Re: [Qemu-devel] [PATCH RFC v2 1/2] Add param Error** to msi_init() & modify the callers

[Michael S. Tsirkin]

On Thu, Mar 03, 2016 at 01:19:09PM +0200, Marcel Apfelbaum wrote:
> On 03/03/2016 12:45 PM, Michael S. Tsirkin wrote:
> >On Thu, Mar 03, 2016 at 12:12:27PM +0200, Marcel Apfelbaum wrote:
> >>>>+int msi_init(struct PCIDevice *dev, uint8_t offset, unsigned int nr_vectors,
> >>>>+             bool msi64bit, bool msi_per_vector_mask, Error **errp)
> >>>>  {
> >>>>      unsigned int vectors_order;
> >>>>-    uint16_t flags;
> >>>>+    uint16_t flags; /* Message Control register value */
> >>>>      uint8_t cap_size;
> >>>>      int config_offset;
> >>>>
> >>>>      if (!msi_supported) {
> >>>>+        error_setg(errp, "MSI is not supported by interrupt controller");
> >>>>          return -ENOTSUP;
> >>>
> >>>First failure mode: board does not support MSI (-ENOTSUP).
> >>>
> >>>Question to the PCI guys: why is this even an error?  A device with
> >>>capability MSI should work just fine in such a board.
> >>
> >>Hi Markus,
> >>
> >>Adding Jan Kiszka, maybe he can help.
> >>
> >>That's a fair question. Is there any history for this decision?
> >>The board not supporting MSI has nothing to do with the capability being there.
> >>The HW should not change because the board doe not support it.
> >>
> >>The capability should be present but not active.
> >
> >Digging in git log will tell you why we have the msi_supported flag:
> >
> >commit 02eb84d0ec97f183ac23ee939403a139e8849b1d ("qemu/pci: MSI-X support functions")
> >
> >	This is a safety measure to avoid breaking platforms which should support
> >	MSI-X but currently lack this in the interrupt controller emulation.
> >
> >in other words, on some platforms we must hide msi support from devices
> >because otherwise guests will try to use it, and our emulation is
> >incomplete.
> 
> 
> OK, thanks. So the flag should be "msi_broken" or "msi_present_but_not_implemented" and not
> "msi_supported" that leads (at least me) to the assumption that some platform *does not support msi*
> rather than it supports it, but we don't emulate it.
> 
> 
> >
> >And the conclusion from that is that for msi_init to fail silently is
> >at the moment the right thing to do.
> 
> But this is not the only thing we do, we are modifying the PCI devices. We could fail starting the VM
> if a device supporting MSI is added on a platform with broken msi, but this will prevent us to use
> assigned devices. Emulated devices should be created with a specific "msi=off" flag.
> 
> Thanks,
> Marcel

That will just break a bunch of valid configurations, for no real
benefit to users.

> >
> >The only other reason for it to fail is pci config space corruption,
> >this probably never happens in practice.
> >

Re: [Qemu-devel] [PATCH RFC v2 1/2] Add param Error** to msi_init() & modify the callers

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Thu, Mar 03, 2016 at 01:19:09PM +0200, Marcel Apfelbaum wrote:
>> On 03/03/2016 12:45 PM, Michael S. Tsirkin wrote:
>> >On Thu, Mar 03, 2016 at 12:12:27PM +0200, Marcel Apfelbaum wrote:
>> >>>>+int msi_init(struct PCIDevice *dev, uint8_t offset, unsigned int nr_vectors,
>> >>>>+             bool msi64bit, bool msi_per_vector_mask, Error **errp)
>> >>>>  {
>> >>>>      unsigned int vectors_order;
>> >>>>-    uint16_t flags;
>> >>>>+    uint16_t flags; /* Message Control register value */
>> >>>>      uint8_t cap_size;
>> >>>>      int config_offset;
>> >>>>
>> >>>>      if (!msi_supported) {
>> >>>>+        error_setg(errp, "MSI is not supported by interrupt controller");
>> >>>>          return -ENOTSUP;
>> >>>
>> >>>First failure mode: board does not support MSI (-ENOTSUP).
>> >>>
>> >>>Question to the PCI guys: why is this even an error?  A device with
>> >>>capability MSI should work just fine in such a board.
>> >>
>> >>Hi Markus,
>> >>
>> >>Adding Jan Kiszka, maybe he can help.
>> >>
>> >>That's a fair question. Is there any history for this decision?
>> >>The board not supporting MSI has nothing to do with the capability being there.
>> >>The HW should not change because the board doe not support it.
>> >>
>> >>The capability should be present but not active.
>> >
>> >Digging in git log will tell you why we have the msi_supported flag:
>> >
>> >commit 02eb84d0ec97f183ac23ee939403a139e8849b1d ("qemu/pci: MSI-X support functions")
>> >
>> >	This is a safety measure to avoid breaking platforms which should support
>> >	MSI-X but currently lack this in the interrupt controller emulation.
>> >
>> >in other words, on some platforms we must hide msi support from devices
>> >because otherwise guests will try to use it, and our emulation is
>> >incomplete.
>> 
>> 
>> OK, thanks. So the flag should be "msi_broken" or "msi_present_but_not_implemented" and not
>> "msi_supported" that leads (at least me) to the assumption that some platform *does not support msi*
>> rather than it supports it, but we don't emulate it.

I agree the name is badly misleading for this role.

Now let me see how this contraption actually works.  msi_supported is
global, initialized to false, and becomes globally true when

1. certain MSI-capable interrupt controllers realize: "apic",
  "kvm-apic" if kvm_has_gsi_routing(), "xen-apic", "arm-gicv2m",
  "openpic" models 1 and 2, "kvm-openpic" models 1 and 2

2. "s390-pcihost" class-initializes

3. machine "spapr-machine" initializes

Issues:

* "Global is problematic.  What if a board has more than one interrupt
  controller?  What if one of them sets msi_supported, but the other one
  is of the kind Michael described, i.e. guests know it has MSI, but our
  emulation doesn't actually work?

* "Initialize to false" is problematic.  We don't clear msi_supported
  when we have a broken interrupt controler, we set it when we have a
  working one.  The consequence is that boards with non-MSI interrupt
  controllers are treated just like boards with broken interrupt
  controllers.

  Here's  how msi_supported is documented:

    /* Flag for interrupt controller to declare MSI/MSI-X support */
    bool msi_supported;

  This is matches how the code works.  However, it contradicts the
  commit message Michael quoted.  The most plausible explanation is that
  the commit is flawed.

* Class-initialize (2.) looks wrong to me.  msi_supported becomes true
  when QOM type "s390-pcihost" is created, regardless of whether
  instances get created, let alone used.

* I'm not sure about 3., but the spapr guys can worry about that.

>> >And the conclusion from that is that for msi_init to fail silently is
>> >at the moment the right thing to do.
>> 
>> But this is not the only thing we do, we are modifying the PCI devices. We could fail starting the VM
>> if a device supporting MSI is added on a platform with broken msi, but this will prevent us to use
>> assigned devices. Emulated devices should be created with a specific "msi=off" flag.
>> 
>> Thanks,
>> Marcel
>
> That will just break a bunch of valid configurations, for no real
> benefit to users.

I disagree, strongly.

If I ask for msi=on, then QEMU should either give it to me or fail, not
silently "correct" my configuration.

Of course, if we have msi_supported = false for everybody and its dog
whether it's really needed or not, "or fail" will indeed reject "a bunch
of valid configurations".  We "compensate" by silently messing with the
user's configuration.  That's shoddy workmanship, sorry.

We should instead have msi_supported = false only when it's actually
needed, and thus reject exactly the configurations that won't work.

>> >
>> >The only other reason for it to fail is pci config space corruption,
>> >this probably never happens in practice.
>> >

Re: [Qemu-devel] [PATCH RFC v2 1/2] Add param Error** to msi_init() & modify the callers

[Michael S. Tsirkin]

On Thu, Mar 03, 2016 at 04:03:16PM +0100, Markus Armbruster wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Thu, Mar 03, 2016 at 01:19:09PM +0200, Marcel Apfelbaum wrote:
> >> On 03/03/2016 12:45 PM, Michael S. Tsirkin wrote:
> >> >On Thu, Mar 03, 2016 at 12:12:27PM +0200, Marcel Apfelbaum wrote:
> >> >>>>+int msi_init(struct PCIDevice *dev, uint8_t offset, unsigned int nr_vectors,
> >> >>>>+             bool msi64bit, bool msi_per_vector_mask, Error **errp)
> >> >>>>  {
> >> >>>>      unsigned int vectors_order;
> >> >>>>-    uint16_t flags;
> >> >>>>+    uint16_t flags; /* Message Control register value */
> >> >>>>      uint8_t cap_size;
> >> >>>>      int config_offset;
> >> >>>>
> >> >>>>      if (!msi_supported) {
> >> >>>>+        error_setg(errp, "MSI is not supported by interrupt controller");
> >> >>>>          return -ENOTSUP;
> >> >>>
> >> >>>First failure mode: board does not support MSI (-ENOTSUP).
> >> >>>
> >> >>>Question to the PCI guys: why is this even an error?  A device with
> >> >>>capability MSI should work just fine in such a board.
> >> >>
> >> >>Hi Markus,
> >> >>
> >> >>Adding Jan Kiszka, maybe he can help.
> >> >>
> >> >>That's a fair question. Is there any history for this decision?
> >> >>The board not supporting MSI has nothing to do with the capability being there.
> >> >>The HW should not change because the board doe not support it.
> >> >>
> >> >>The capability should be present but not active.
> >> >
> >> >Digging in git log will tell you why we have the msi_supported flag:
> >> >
> >> >commit 02eb84d0ec97f183ac23ee939403a139e8849b1d ("qemu/pci: MSI-X support functions")
> >> >
> >> >	This is a safety measure to avoid breaking platforms which should support
> >> >	MSI-X but currently lack this in the interrupt controller emulation.
> >> >
> >> >in other words, on some platforms we must hide msi support from devices
> >> >because otherwise guests will try to use it, and our emulation is
> >> >incomplete.
> >> 
> >> 
> >> OK, thanks. So the flag should be "msi_broken" or "msi_present_but_not_implemented" and not
> >> "msi_supported" that leads (at least me) to the assumption that some platform *does not support msi*
> >> rather than it supports it, but we don't emulate it.
> 
> I agree the name is badly misleading for this role.
> 
> Now let me see how this contraption actually works.  msi_supported is
> global, initialized to false, and becomes globally true when
> 
> 1. certain MSI-capable interrupt controllers realize: "apic",
>   "kvm-apic" if kvm_has_gsi_routing(), "xen-apic", "arm-gicv2m",
>   "openpic" models 1 and 2, "kvm-openpic" models 1 and 2
> 
> 2. "s390-pcihost" class-initializes
> 
> 3. machine "spapr-machine" initializes
> 
> Issues:
> 
> * "Global is problematic.  What if a board has more than one interrupt
>   controller?  What if one of them sets msi_supported, but the other one
>   is of the kind Michael described, i.e. guests know it has MSI, but our
>   emulation doesn't actually work?
> 
> * "Initialize to false" is problematic.  We don't clear msi_supported
>   when we have a broken interrupt controler, we set it when we have a
>   working one.  The consequence is that boards with non-MSI interrupt
>   controllers are treated just like boards with broken interrupt
>   controllers.
> 
>   Here's  how msi_supported is documented:
> 
>     /* Flag for interrupt controller to declare MSI/MSI-X support */
>     bool msi_supported;
> 
>   This is matches how the code works.  However, it contradicts the
>   commit message Michael quoted.  The most plausible explanation is that
>   the commit is flawed.
> 
> * Class-initialize (2.) looks wrong to me.  msi_supported becomes true
>   when QOM type "s390-pcihost" is created, regardless of whether
>   instances get created, let alone used.
> 
> * I'm not sure about 3., but the spapr guys can worry about that.
> 
> >> >And the conclusion from that is that for msi_init to fail silently is
> >> >at the moment the right thing to do.
> >> 
> >> But this is not the only thing we do, we are modifying the PCI devices. We could fail starting the VM
> >> if a device supporting MSI is added on a platform with broken msi, but this will prevent us to use
> >> assigned devices. Emulated devices should be created with a specific "msi=off" flag.
> >> 
> >> Thanks,
> >> Marcel
> >
> > That will just break a bunch of valid configurations, for no real
> > benefit to users.
> 
> I disagree, strongly.
> 
> If I ask for msi=on, then QEMU should either give it to me or fail, not
> silently "correct" my configuration.
> 
> Of course, if we have msi_supported = false for everybody and its dog
> whether it's really needed or not, "or fail" will indeed reject "a bunch
> of valid configurations".  We "compensate" by silently messing with the
> user's configuration.  That's shoddy workmanship, sorry.
> 
> We should instead have msi_supported = false only when it's actually
> needed, and thus reject exactly the configurations that won't work.

Most people don't care one way or the other.  We started without msi.
We later added msi for some boards only.  Asking everyone to add msi=off
for all other boards at that point would have been silly.

> >> >
> >> >The only other reason for it to fail is pci config space corruption,
> >> >this probably never happens in practice.
> >> >

Re: [Qemu-devel] [PATCH RFC v2 1/2] Add param Error** to msi_init() & modify the callers

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Thu, Mar 03, 2016 at 04:03:16PM +0100, Markus Armbruster wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> 
>> > On Thu, Mar 03, 2016 at 01:19:09PM +0200, Marcel Apfelbaum wrote:
>> >> On 03/03/2016 12:45 PM, Michael S. Tsirkin wrote:
>> >> >On Thu, Mar 03, 2016 at 12:12:27PM +0200, Marcel Apfelbaum wrote:
>> >> >>>>+int msi_init(struct PCIDevice *dev, uint8_t offset, unsigned int nr_vectors,
>> >> >>>>+             bool msi64bit, bool msi_per_vector_mask, Error **errp)
>> >> >>>>  {
>> >> >>>>      unsigned int vectors_order;
>> >> >>>>-    uint16_t flags;
>> >> >>>>+    uint16_t flags; /* Message Control register value */
>> >> >>>>      uint8_t cap_size;
>> >> >>>>      int config_offset;
>> >> >>>>
>> >> >>>>      if (!msi_supported) {
>> >> >>>>+        error_setg(errp, "MSI is not supported by interrupt controller");
>> >> >>>>          return -ENOTSUP;
>> >> >>>
>> >> >>>First failure mode: board does not support MSI (-ENOTSUP).
>> >> >>>
>> >> >>>Question to the PCI guys: why is this even an error?  A device with
>> >> >>>capability MSI should work just fine in such a board.
>> >> >>
>> >> >>Hi Markus,
>> >> >>
>> >> >>Adding Jan Kiszka, maybe he can help.
>> >> >>
>> >> >>That's a fair question. Is there any history for this decision?
>> >> >>The board not supporting MSI has nothing to do with the capability being there.
>> >> >>The HW should not change because the board doe not support it.
>> >> >>
>> >> >>The capability should be present but not active.
>> >> >
>> >> >Digging in git log will tell you why we have the msi_supported flag:
>> >> >
>> >> >commit 02eb84d0ec97f183ac23ee939403a139e8849b1d ("qemu/pci: MSI-X support functions")
>> >> >
>> >> >	This is a safety measure to avoid breaking platforms which should support
>> >> >	MSI-X but currently lack this in the interrupt controller emulation.
>> >> >
>> >> >in other words, on some platforms we must hide msi support from devices
>> >> >because otherwise guests will try to use it, and our emulation is
>> >> >incomplete.
>> >> 
>> >> 
>> >> OK, thanks. So the flag should be "msi_broken" or "msi_present_but_not_implemented" and not
>> >> "msi_supported" that leads (at least me) to the assumption that some platform *does not support msi*
>> >> rather than it supports it, but we don't emulate it.
>> 
>> I agree the name is badly misleading for this role.
>> 
>> Now let me see how this contraption actually works.  msi_supported is
>> global, initialized to false, and becomes globally true when
>> 
>> 1. certain MSI-capable interrupt controllers realize: "apic",
>>   "kvm-apic" if kvm_has_gsi_routing(), "xen-apic", "arm-gicv2m",
>>   "openpic" models 1 and 2, "kvm-openpic" models 1 and 2
>> 
>> 2. "s390-pcihost" class-initializes
>> 
>> 3. machine "spapr-machine" initializes
>> 
>> Issues:
>> 
>> * "Global is problematic.  What if a board has more than one interrupt
>>   controller?  What if one of them sets msi_supported, but the other one
>>   is of the kind Michael described, i.e. guests know it has MSI, but our
>>   emulation doesn't actually work?
>> 
>> * "Initialize to false" is problematic.  We don't clear msi_supported
>>   when we have a broken interrupt controler, we set it when we have a
>>   working one.  The consequence is that boards with non-MSI interrupt
>>   controllers are treated just like boards with broken interrupt
>>   controllers.
>> 
>>   Here's  how msi_supported is documented:
>> 
>>     /* Flag for interrupt controller to declare MSI/MSI-X support */
>>     bool msi_supported;
>> 
>>   This is matches how the code works.  However, it contradicts the
>>   commit message Michael quoted.  The most plausible explanation is that
>>   the commit is flawed.
>> 
>> * Class-initialize (2.) looks wrong to me.  msi_supported becomes true
>>   when QOM type "s390-pcihost" is created, regardless of whether
>>   instances get created, let alone used.
>> 
>> * I'm not sure about 3., but the spapr guys can worry about that.
>> 
>> >> >And the conclusion from that is that for msi_init to fail silently is
>> >> >at the moment the right thing to do.
>> >> 
>> >> But this is not the only thing we do, we are modifying the PCI devices. We could fail starting the VM
>> >> if a device supporting MSI is added on a platform with broken msi, but this will prevent us to use
>> >> assigned devices. Emulated devices should be created with a specific "msi=off" flag.
>> >> 
>> >> Thanks,
>> >> Marcel
>> >
>> > That will just break a bunch of valid configurations, for no real
>> > benefit to users.
>> 
>> I disagree, strongly.
>> 
>> If I ask for msi=on, then QEMU should either give it to me or fail, not
>> silently "correct" my configuration.
>> 
>> Of course, if we have msi_supported = false for everybody and its dog
>> whether it's really needed or not, "or fail" will indeed reject "a bunch
>> of valid configurations".  We "compensate" by silently messing with the
>> user's configuration.  That's shoddy workmanship, sorry.
>> 
>> We should instead have msi_supported = false only when it's actually
>> needed, and thus reject exactly the configurations that won't work.
>
> Most people don't care one way or the other.  We started without msi.
> We later added msi for some boards only.  Asking everyone to add msi=off
> for all other boards at that point would have been silly.

I'm afraid you're missing my point entirely.

Yes, we started without MSI.  We then added MSI support to some devices
and to some boards.  Exactly the same history as for physical hardware.

Plugging an MSI-capable device into an MSI-incapable board works just
fine, both for physical and for virtual hardware.  What doesn't work is
plugging an MSI-capable device into an MSI-capable board with *broken*
MSI support.

As a convenience feature, we summarily and silently remove a device's
MSI capability when we detect such a broken board.  At least that's what
the commit message you quoted claims.

In reality, we remove it not just for broken boards, but even for
MSI-incapable boards.

I take issue with "summarily and silently", and "even for MSI-incapable
boards".

When multiple variants of a device exist, and the user didn't ask for a
specific one, then picking the one that works best with the board is
just fine.

It's absolutely not fine when the user did ask for a specific one.  When
I ask for msi=on, I mean it.  If it can't work with this board, tell me.
But silently ignoring my orders is a bug.

It's fine to have emulations of MSI-capable boards where MSI doesn't yet
work.  Even if that means we have to reject MSI-capable devices.

It's absolutely not fine to reject them for MSI-incapable boards, where
they'd work just fine.

Furthermore, I doubt the wisdom of creating virtual devices that don't
exist physically just to have something that works in our broken boards.
If the physical device exists in MSI-capable and MSI-incapable variants,
emulating both is fine.  But if it only ever existed MSI-capable, having
the PCI core(!) drop the MSI capability is a questionable idea.  I
suspect that the need for this dubious hack would be much smaller if we
didn't foolishly treat every MSI-incapable board as broken MSI-capable
board.

If you conclude that cleaning up this carelessly made mess is not worth
the bother now, then at least explain the mess in the code, please.
It's not obvious, and figuring out what's going on and why it is the way
it is has taken me several hours, and Marcel's help.

[...]

Re: [Qemu-devel] [PATCH RFC v2 1/2] Add param Error** to msi_init() & modify the callers

[Michael S. Tsirkin]

On Fri, Mar 04, 2016 at 09:42:02AM +0100, Markus Armbruster wrote:
> Plugging an MSI-capable device into an MSI-incapable board works just
> fine, both for physical and for virtual hardware.  What doesn't work is
> plugging an MSI-capable device into an MSI-capable board with *broken*
> MSI support.
> 
> As a convenience feature, we summarily and silently remove a device's
> MSI capability when we detect such a broken board.  At least that's what
> the commit message you quoted claims.

And this makes sense, right?

> In reality, we remove it not just for broken boards, but even for
> MSI-incapable boards.
> 
> I take issue with "summarily and silently", and "even for MSI-incapable
> boards".
> 
> When multiple variants of a device exist, and the user didn't ask for a
> specific one, then picking the one that works best with the board is
> just fine.
> 
> It's absolutely not fine when the user did ask for a specific one.  When
> I ask for msi=on, I mean it.  If it can't work with this board, tell me.
> But silently ignoring my orders is a bug.

I agree. msi is not the only case either.  We really need - for any boolean
flag - a way to figure out whether it was set by user.
With that in place we could fix it.

However, almost no one uses the msi/msi-x flag - we introduced
them only for one reason - for backwards compatibility.
The fact that each time we need a compatibility flag
we also expose a new user interface is very unfortunate.
IMO it was a design mistake made a long time ago and it won't
be easy to fix now.

And for the above reason I personally do not intend to
spend time designing a specific hack just for the msi
property.

> It's fine to have emulations of MSI-capable boards where MSI doesn't yet
> work.  Even if that means we have to reject MSI-capable devices.

I don't know what does reject mean here. Removing msi capability?
In that case I agree.

> It's absolutely not fine to reject them for MSI-incapable boards, where
> they'd work just fine.

I think that as long as users did not ask for msi explicitly,
and board is msi incapable, it does not matter much whether
device has msi capability or not - guest will not try
to use it anyway.

> Furthermore, I doubt the wisdom of creating virtual devices that don't
> exist physically just to have something that works in our broken boards.
> If the physical device exists in MSI-capable and MSI-incapable variants,
> emulating both is fine.  But if it only ever existed MSI-capable, having
> the PCI core(!) drop the MSI capability is a questionable idea.  I
> suspect that the need for this dubious hack would be much smaller if we
> didn't foolishly treat every MSI-incapable board as broken MSI-capable
> board.
> 
> If you conclude that cleaning up this carelessly made mess is not worth
> the bother now, then at least explain the mess in the code, please.
> It's not obvious, and figuring out what's going on and why it is the way
> it is has taken me several hours, and Marcel's help.

I think it's worth cleaning up, or at least documenting.
Fixing it will take much more than the patch proposed here,
and we can not start with this patch as it will cause
regressions.
Adding a comment won't be too much work.
How about the below?

-->

msi_supported -> msi_nonbroken

Rename controller flag to make it clearer what it means.
Add some documentation as well.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---

diff --git a/include/hw/pci/msi.h b/include/hw/pci/msi.h
index 50e452b..8124908 100644
--- a/include/hw/pci/msi.h
+++ b/include/hw/pci/msi.h
@@ -29,7 +29,7 @@ struct MSIMessage {
     uint32_t data;
 };
 
-extern bool msi_supported;
+extern bool msi_nonbroken;
 
 void msi_set_message(PCIDevice *dev, MSIMessage msg);
 MSIMessage msi_get_message(PCIDevice *dev, unsigned int vector);
diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
index 694d398..3c7c8fa 100644
--- a/hw/i386/kvm/apic.c
+++ b/hw/i386/kvm/apic.c
@@ -186,7 +186,7 @@ static void kvm_apic_realize(DeviceState *dev, Error **errp)
                           APIC_SPACE_SIZE);
 
     if (kvm_has_gsi_routing()) {
-        msi_supported = true;
+        msi_nonbroken = true;
     }
 }
 
diff --git a/hw/i386/xen/xen_apic.c b/hw/i386/xen/xen_apic.c
index 2b8d709..21d68ee 100644
--- a/hw/i386/xen/xen_apic.c
+++ b/hw/i386/xen/xen_apic.c
@@ -44,7 +44,7 @@ static void xen_apic_realize(DeviceState *dev, Error **errp)
     s->vapic_control = 0;
     memory_region_init_io(&s->io_memory, OBJECT(s), &xen_apic_io_ops, s,
                           "xen-apic-msi", APIC_SPACE_SIZE);
-    msi_supported = true;
+    msi_nonbroken = true;
 }
 
 static void xen_apic_set_base(APICCommonState *s, uint64_t val)
diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index a299462..28c2ea5 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -874,7 +874,7 @@ static void apic_realize(DeviceState *dev, Error **errp)
     s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, apic_timer, s);
     local_apics[s->idx] = s;
 
-    msi_supported = true;
+    msi_nonbroken = true;
 }
 
 static void apic_class_init(ObjectClass *klass, void *data)
diff --git a/hw/intc/arm_gicv2m.c b/hw/intc/arm_gicv2m.c
index 70c0b97..ebd368b 100644
--- a/hw/intc/arm_gicv2m.c
+++ b/hw/intc/arm_gicv2m.c
@@ -148,7 +148,7 @@ static void gicv2m_realize(DeviceState *dev, Error **errp)
         sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->spi[i]);
     }
 
-    msi_supported = true;
+    msi_nonbroken = true;
     kvm_gsi_direct_mapping = true;
     kvm_msi_via_irqfd_allowed = kvm_irqfds_enabled();
 }
diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
index 903888c..7685250 100644
--- a/hw/intc/openpic.c
+++ b/hw/intc/openpic.c
@@ -1375,7 +1375,7 @@ static void fsl_common_init(OpenPICState *opp)
 
     opp->irq_msi = 224;
 
-    msi_supported = true;
+    msi_nonbroken = true;
     for (i = 0; i < opp->fsl->max_ext; i++) {
         opp->src[i].level = false;
     }
diff --git a/hw/intc/openpic_kvm.c b/hw/intc/openpic_kvm.c
index 4dcdb61..778af4a 100644
--- a/hw/intc/openpic_kvm.c
+++ b/hw/intc/openpic_kvm.c
@@ -239,7 +239,7 @@ static void kvm_openpic_realize(DeviceState *dev, Error **errp)
     memory_listener_register(&opp->mem_listener, &address_space_memory);
 
     /* indicate pic capabilities */
-    msi_supported = true;
+    msi_nonbroken = true;
     kvm_kernel_irqchip = true;
     kvm_async_interrupts_allowed = true;
 
diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index 100bb5e..862a2366 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -72,7 +72,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
         goto slotid_error;
     }
     if ((bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_MSI_REQ)) &&
-        msi_supported) {
+        msi_nonbroken) {
         err = msi_init(dev, 0, 1, true, true);
         if (err < 0) {
             goto msi_error;
diff --git a/hw/pci/msi.c b/hw/pci/msi.c
index 85f21b8..e0e64c2 100644
--- a/hw/pci/msi.c
+++ b/hw/pci/msi.c
@@ -34,8 +34,21 @@
 
 #define PCI_MSI_VECTORS_MAX     32
 
-/* Flag for interrupt controller to declare MSI/MSI-X support */
-bool msi_supported;
+/*
+ * Flag for interrupt controllers to declare broken MSI/MSI-X support.
+ * values: false - broken; true - non-broken.
+ *
+ * Setting this flag to false will remove MSI/MSI-X capability from all devices.
+ *
+ * It is preferrable for controllers to set this to true (non-broken) even if
+ * they do not actually support MSI/MSI-X: guests normally probe the controller
+ * type and do not attempt to enable MSI/MSI-X with interrupt controllers not
+ * supporting such, so removing the capability is not required, and
+ * it seems cleaner to have a given device look the same for all boards.
+ *
+ * TODO: some existing controllers violate the above rule. Identify and fix them.
+ */
+bool msi_nonbroken;
 
 /* If we get rid of cap allocator, we won't need this. */
 static inline uint8_t msi_cap_sizeof(uint16_t flags)
@@ -160,7 +173,7 @@ int msi_init(struct PCIDevice *dev, uint8_t offset,
     uint8_t cap_size;
     int config_offset;
 
-    if (!msi_supported) {
+    if (!msi_nonbroken) {
         return -ENOTSUP;
     }
 
diff --git a/hw/pci/msix.c b/hw/pci/msix.c
index 537fdba..b75f0e9 100644
--- a/hw/pci/msix.c
+++ b/hw/pci/msix.c
@@ -249,7 +249,7 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
     uint8_t *config;
 
     /* Nothing to do if MSI is not supported by interrupt controller */
-    if (!msi_supported) {
+    if (!msi_nonbroken) {
         return -ENOTSUP;
     }
 
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index e9d4abf..c4fb206 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -439,7 +439,7 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
     _FDT((fdt_property_cell(fdt, "rtas-event-scan-rate",
                             RTAS_EVENT_SCAN_RATE)));
 
-    if (msi_supported) {
+    if (msi_nonbroken) {
         _FDT((fdt_property(fdt, "ibm,change-msix-capable", NULL, 0)));
     }
 
@@ -1743,7 +1743,7 @@ static void ppc_spapr_init(MachineState *machine)
     bool kernel_le = false;
     char *filename;
 
-    msi_supported = true;
+    msi_nonbroken = true;
 
     QLIST_INIT(&spapr->phbs);
 
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index e8edad3..3fc7895 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1790,7 +1790,7 @@ void spapr_pci_rtas_init(void)
                         rtas_ibm_read_pci_config);
     spapr_rtas_register(RTAS_IBM_WRITE_PCI_CONFIG, "ibm,write-pci-config",
                         rtas_ibm_write_pci_config);
-    if (msi_supported) {
+    if (msi_nonbroken) {
         spapr_rtas_register(RTAS_IBM_QUERY_INTERRUPT_SOURCE_NUMBER,
                             "ibm,query-interrupt-source-number",
                             rtas_ibm_query_interrupt_source_number);
diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index dba0202..f5f679f 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -597,7 +597,7 @@ static void s390_pcihost_class_init(ObjectClass *klass, void *data)
     k->init = s390_pcihost_init;
     hc->plug = s390_pcihost_hot_plug;
     hc->unplug = s390_pcihost_hot_unplug;
-    msi_supported = true;
+    msi_nonbroken = true;
 }
 
 static const TypeInfo s390_pcihost_info = {
-- 
MST

Re: [Qemu-devel] [PATCH RFC v2 1/2] Add param Error** to msi_init() & modify the callers

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Fri, Mar 04, 2016 at 09:42:02AM +0100, Markus Armbruster wrote:
>> Plugging an MSI-capable device into an MSI-incapable board works just
>> fine, both for physical and for virtual hardware.  What doesn't work is
>> plugging an MSI-capable device into an MSI-capable board with *broken*
>> MSI support.
>> 
>> As a convenience feature, we summarily and silently remove a device's
>> MSI capability when we detect such a broken board.  At least that's what
>> the commit message you quoted claims.
>
> And this makes sense, right?

Yes, except for the part where we ignore the user's explicit orders,
and, to a lesser degree, for the part where we create variants of
physical devices that don't exist physically.

>> In reality, we remove it not just for broken boards, but even for
>> MSI-incapable boards.
>> 
>> I take issue with "summarily and silently", and "even for MSI-incapable
>> boards".
>> 
>> When multiple variants of a device exist, and the user didn't ask for a
>> specific one, then picking the one that works best with the board is
>> just fine.
>> 
>> It's absolutely not fine when the user did ask for a specific one.  When
>> I ask for msi=on, I mean it.  If it can't work with this board, tell me.
>> But silently ignoring my orders is a bug.
>
> I agree. msi is not the only case either.  We really need - for any boolean
> flag - a way to figure out whether it was set by user.
> With that in place we could fix it.

QMP provides that directly as "optional bool", but qdev properties do
"optional" diffently, and when you see the default value, you don't know
whether it comes from the user or not.

Another solution is an on/off/auto type.  We got it already in the QAPI
schema, as OnOffAuto, and my recent "[PATCH 32/38] qdev: New
DEFINE_PROP_ON_OFF_AUTO" makes it available as qdev property.  With
default set to auto, we should be set.

> However, almost no one uses the msi/msi-x flag - we introduced
> them only for one reason - for backwards compatibility.
> The fact that each time we need a compatibility flag
> we also expose a new user interface is very unfortunate.
> IMO it was a design mistake made a long time ago and it won't
> be easy to fix now.

I agree there's no easy fix, but we can try to find a non-easy one.

For backward compatibility, we need to configure some device models for
certain machine types.  We use the only object configuration mechanism
we have: properties.  The properties we use for compatibility are all
exposed to the user.

We could easily provide a flag to mark properties private, and only
accept non-private properties at external interfaces.  This should help
stopping growth of the problem, but it's not an easy fix for reducing
it, because making a property private retroactively is problematic.  We
could have a flag to mark them deprecated instead, warn on use, remove
them from documentation, and perhaps drop them a couple of releases
later.

> And for the above reason I personally do not intend to
> spend time designing a specific hack just for the msi
> property.
>
>> It's fine to have emulations of MSI-capable boards where MSI doesn't yet
>> work.  Even if that means we have to reject MSI-capable devices.
>
> I don't know what does reject mean here. Removing msi capability?
> In that case I agree.

By "reject" I mean "reject the user's order to plug in an MSI-capable
device."

>> It's absolutely not fine to reject them for MSI-incapable boards, where
>> they'd work just fine.
>
> I think that as long as users did not ask for msi explicitly,
> and board is msi incapable, it does not matter much whether
> device has msi capability or not - guest will not try
> to use it anyway.

If the device comes in MSI-capable and MSI-incapable variants, and the
user didn't specify a variant, then picking one that fits the board is
fine.

If the device does not come in variants (and many physical devices
don't), then rejecting it just because it's MSI-capable and the board
isn't is not fine.

To fix, we'd have to limit what is now !msi_supported to the boards that
are nominally MSI-capable, except our emulation doesn't work, i.e. do
exactly what the commit message promised.

The PCI core encourages creating both variants, and most (but not all)
device models do, but:

>> Furthermore, I doubt the wisdom of creating virtual devices that don't
>> exist physically just to have something that works in our broken boards.
>> If the physical device exists in MSI-capable and MSI-incapable variants,
>> emulating both is fine.  But if it only ever existed MSI-capable, having
>> the PCI core(!) drop the MSI capability is a questionable idea.  I
>> suspect that the need for this dubious hack would be much smaller if we
>> didn't foolishly treat every MSI-incapable board as broken MSI-capable
>> board.
>> 
>> If you conclude that cleaning up this carelessly made mess is not worth
>> the bother now, then at least explain the mess in the code, please.
>> It's not obvious, and figuring out what's going on and why it is the way
>> it is has taken me several hours, and Marcel's help.
>
> I think it's worth cleaning up, or at least documenting.
> Fixing it will take much more than the patch proposed here,
> and we can not start with this patch as it will cause
> regressions.
> Adding a comment won't be too much work.
> How about the below?
>
> -->
>
> msi_supported -> msi_nonbroken
>
> Rename controller flag to make it clearer what it means.
> Add some documentation as well.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> ---
>
> diff --git a/include/hw/pci/msi.h b/include/hw/pci/msi.h
> index 50e452b..8124908 100644
> --- a/include/hw/pci/msi.h
> +++ b/include/hw/pci/msi.h
> @@ -29,7 +29,7 @@ struct MSIMessage {
>      uint32_t data;
>  };
>  
> -extern bool msi_supported;
> +extern bool msi_nonbroken;
>  
>  void msi_set_message(PCIDevice *dev, MSIMessage msg);
>  MSIMessage msi_get_message(PCIDevice *dev, unsigned int vector);
> diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
> index 694d398..3c7c8fa 100644
> --- a/hw/i386/kvm/apic.c
> +++ b/hw/i386/kvm/apic.c
> @@ -186,7 +186,7 @@ static void kvm_apic_realize(DeviceState *dev, Error **errp)
>                            APIC_SPACE_SIZE);
>  
>      if (kvm_has_gsi_routing()) {
> -        msi_supported = true;
> +        msi_nonbroken = true;
>      }
>  }
>  
> diff --git a/hw/i386/xen/xen_apic.c b/hw/i386/xen/xen_apic.c
> index 2b8d709..21d68ee 100644
> --- a/hw/i386/xen/xen_apic.c
> +++ b/hw/i386/xen/xen_apic.c
> @@ -44,7 +44,7 @@ static void xen_apic_realize(DeviceState *dev, Error **errp)
>      s->vapic_control = 0;
>      memory_region_init_io(&s->io_memory, OBJECT(s), &xen_apic_io_ops, s,
>                            "xen-apic-msi", APIC_SPACE_SIZE);
> -    msi_supported = true;
> +    msi_nonbroken = true;
>  }
>  
>  static void xen_apic_set_base(APICCommonState *s, uint64_t val)
> diff --git a/hw/intc/apic.c b/hw/intc/apic.c
> index a299462..28c2ea5 100644
> --- a/hw/intc/apic.c
> +++ b/hw/intc/apic.c
> @@ -874,7 +874,7 @@ static void apic_realize(DeviceState *dev, Error **errp)
>      s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, apic_timer, s);
>      local_apics[s->idx] = s;
>  
> -    msi_supported = true;
> +    msi_nonbroken = true;
>  }
>  
>  static void apic_class_init(ObjectClass *klass, void *data)
> diff --git a/hw/intc/arm_gicv2m.c b/hw/intc/arm_gicv2m.c
> index 70c0b97..ebd368b 100644
> --- a/hw/intc/arm_gicv2m.c
> +++ b/hw/intc/arm_gicv2m.c
> @@ -148,7 +148,7 @@ static void gicv2m_realize(DeviceState *dev, Error **errp)
>          sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->spi[i]);
>      }
>  
> -    msi_supported = true;
> +    msi_nonbroken = true;
>      kvm_gsi_direct_mapping = true;
>      kvm_msi_via_irqfd_allowed = kvm_irqfds_enabled();
>  }
> diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
> index 903888c..7685250 100644
> --- a/hw/intc/openpic.c
> +++ b/hw/intc/openpic.c
> @@ -1375,7 +1375,7 @@ static void fsl_common_init(OpenPICState *opp)
>  
>      opp->irq_msi = 224;
>  
> -    msi_supported = true;
> +    msi_nonbroken = true;
>      for (i = 0; i < opp->fsl->max_ext; i++) {
>          opp->src[i].level = false;
>      }
> diff --git a/hw/intc/openpic_kvm.c b/hw/intc/openpic_kvm.c
> index 4dcdb61..778af4a 100644
> --- a/hw/intc/openpic_kvm.c
> +++ b/hw/intc/openpic_kvm.c
> @@ -239,7 +239,7 @@ static void kvm_openpic_realize(DeviceState *dev, Error **errp)
>      memory_listener_register(&opp->mem_listener, &address_space_memory);
>  
>      /* indicate pic capabilities */
> -    msi_supported = true;
> +    msi_nonbroken = true;
>      kvm_kernel_irqchip = true;
>      kvm_async_interrupts_allowed = true;
>  
> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
> index 100bb5e..862a2366 100644
> --- a/hw/pci-bridge/pci_bridge_dev.c
> +++ b/hw/pci-bridge/pci_bridge_dev.c
> @@ -72,7 +72,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>          goto slotid_error;
>      }
>      if ((bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_MSI_REQ)) &&
> -        msi_supported) {
> +        msi_nonbroken) {
>          err = msi_init(dev, 0, 1, true, true);
>          if (err < 0) {
>              goto msi_error;
> diff --git a/hw/pci/msi.c b/hw/pci/msi.c
> index 85f21b8..e0e64c2 100644
> --- a/hw/pci/msi.c
> +++ b/hw/pci/msi.c
> @@ -34,8 +34,21 @@
>  
>  #define PCI_MSI_VECTORS_MAX     32
>  
> -/* Flag for interrupt controller to declare MSI/MSI-X support */
> -bool msi_supported;
> +/*
> + * Flag for interrupt controllers to declare broken MSI/MSI-X support.
> + * values: false - broken; true - non-broken.
> + *
> + * Setting this flag to false will remove MSI/MSI-X capability from all devices.
> + *
> + * It is preferrable for controllers to set this to true (non-broken) even if
> + * they do not actually support MSI/MSI-X: guests normally probe the controller
> + * type and do not attempt to enable MSI/MSI-X with interrupt controllers not
> + * supporting such, so removing the capability is not required, and
> + * it seems cleaner to have a given device look the same for all boards.
> + *
> + * TODO: some existing controllers violate the above rule. Identify and fix them.
> + */
> +bool msi_nonbroken;

Good description.  I'd use FIXME rather than TODO, but that's detail.

>  
>  /* If we get rid of cap allocator, we won't need this. */
>  static inline uint8_t msi_cap_sizeof(uint16_t flags)
> @@ -160,7 +173,7 @@ int msi_init(struct PCIDevice *dev, uint8_t offset,
>      uint8_t cap_size;
>      int config_offset;
>  
> -    if (!msi_supported) {
> +    if (!msi_nonbroken) {
>          return -ENOTSUP;
>      }
>  
> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> index 537fdba..b75f0e9 100644
> --- a/hw/pci/msix.c
> +++ b/hw/pci/msix.c
> @@ -249,7 +249,7 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
>      uint8_t *config;
>  
>      /* Nothing to do if MSI is not supported by interrupt controller */
> -    if (!msi_supported) {
> +    if (!msi_nonbroken) {
>          return -ENOTSUP;
>      }
>  
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index e9d4abf..c4fb206 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -439,7 +439,7 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
>      _FDT((fdt_property_cell(fdt, "rtas-event-scan-rate",
>                              RTAS_EVENT_SCAN_RATE)));
>  
> -    if (msi_supported) {
> +    if (msi_nonbroken) {
>          _FDT((fdt_property(fdt, "ibm,change-msix-capable", NULL, 0)));
>      }
>  
> @@ -1743,7 +1743,7 @@ static void ppc_spapr_init(MachineState *machine)
>      bool kernel_le = false;
>      char *filename;
>  
> -    msi_supported = true;
> +    msi_nonbroken = true;
>  
>      QLIST_INIT(&spapr->phbs);
>  
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index e8edad3..3fc7895 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1790,7 +1790,7 @@ void spapr_pci_rtas_init(void)
>                          rtas_ibm_read_pci_config);
>      spapr_rtas_register(RTAS_IBM_WRITE_PCI_CONFIG, "ibm,write-pci-config",
>                          rtas_ibm_write_pci_config);
> -    if (msi_supported) {
> +    if (msi_nonbroken) {
>          spapr_rtas_register(RTAS_IBM_QUERY_INTERRUPT_SOURCE_NUMBER,
>                              "ibm,query-interrupt-source-number",
>                              rtas_ibm_query_interrupt_source_number);
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index dba0202..f5f679f 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -597,7 +597,7 @@ static void s390_pcihost_class_init(ObjectClass *klass, void *data)
>      k->init = s390_pcihost_init;
>      hc->plug = s390_pcihost_hot_plug;
>      hc->unplug = s390_pcihost_hot_unplug;
> -    msi_supported = true;
> +    msi_nonbroken = true;
>  }
>  
>  static const TypeInfo s390_pcihost_info = {

Much appreciated!

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

Re: [Qemu-devel] [PATCH RFC v2 1/2] Add param Error** to msi_init() & modify the callers

[Michael S. Tsirkin]

On Fri, Mar 04, 2016 at 01:57:05PM +0100, Markus Armbruster wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Fri, Mar 04, 2016 at 09:42:02AM +0100, Markus Armbruster wrote:
> >> Plugging an MSI-capable device into an MSI-incapable board works just
> >> fine, both for physical and for virtual hardware.  What doesn't work is
> >> plugging an MSI-capable device into an MSI-capable board with *broken*
> >> MSI support.
> >> 
> >> As a convenience feature, we summarily and silently remove a device's
> >> MSI capability when we detect such a broken board.  At least that's what
> >> the commit message you quoted claims.
> >
> > And this makes sense, right?
> 
> Yes, except for the part where we ignore the user's explicit orders,
> and, to a lesser degree, for the part where we create variants of
> physical devices that don't exist physically.
> 
> >> In reality, we remove it not just for broken boards, but even for
> >> MSI-incapable boards.
> >> 
> >> I take issue with "summarily and silently", and "even for MSI-incapable
> >> boards".
> >> 
> >> When multiple variants of a device exist, and the user didn't ask for a
> >> specific one, then picking the one that works best with the board is
> >> just fine.
> >> 
> >> It's absolutely not fine when the user did ask for a specific one.  When
> >> I ask for msi=on, I mean it.  If it can't work with this board, tell me.
> >> But silently ignoring my orders is a bug.
> >
> > I agree. msi is not the only case either.  We really need - for any boolean
> > flag - a way to figure out whether it was set by user.
> > With that in place we could fix it.
> 
> QMP provides that directly as "optional bool", but qdev properties do
> "optional" diffently, and when you see the default value, you don't know
> whether it comes from the user or not.
> 
> Another solution is an on/off/auto type.  We got it already in the QAPI
> schema, as OnOffAuto, and my recent "[PATCH 32/38] qdev: New
> DEFINE_PROP_ON_OFF_AUTO" makes it available as qdev property.  With
> default set to auto, we should be set.

Should we somehow change all on/off properties to on/off/auto?

> > However, almost no one uses the msi/msi-x flag - we introduced
> > them only for one reason - for backwards compatibility.
> > The fact that each time we need a compatibility flag
> > we also expose a new user interface is very unfortunate.
> > IMO it was a design mistake made a long time ago and it won't
> > be easy to fix now.
> 
> I agree there's no easy fix, but we can try to find a non-easy one.
> 
> For backward compatibility, we need to configure some device models for
> certain machine types.  We use the only object configuration mechanism
> we have: properties.  The properties we use for compatibility are all
> exposed to the user.
> 
> We could easily provide a flag to mark properties private, and only
> accept non-private properties at external interfaces.  This should help
> stopping growth of the problem, but it's not an easy fix for reducing
> it, because making a property private retroactively is problematic.  We
> could have a flag to mark them deprecated instead, warn on use, remove
> them from documentation, and perhaps drop them a couple of releases
> later.

Sounds good.

> > And for the above reason I personally do not intend to
> > spend time designing a specific hack just for the msi
> > property.
> >
> >> It's fine to have emulations of MSI-capable boards where MSI doesn't yet
> >> work.  Even if that means we have to reject MSI-capable devices.
> >
> > I don't know what does reject mean here. Removing msi capability?
> > In that case I agree.
> 
> By "reject" I mean "reject the user's order to plug in an MSI-capable
> device."

I don't believe anyone tweaks these properties in practice.

However, I have to wonder. Assume that you have supplied
a device with 10 properties. QEMU can not support them.
At your suggesion, device is rejected. How does user
know which property to tweak? Try all values for them all?


> >> It's absolutely not fine to reject them for MSI-incapable boards, where
> >> they'd work just fine.
> >
> > I think that as long as users did not ask for msi explicitly,
> > and board is msi incapable, it does not matter much whether
> > device has msi capability or not - guest will not try
> > to use it anyway.
> 
> If the device comes in MSI-capable and MSI-incapable variants, and the
> user didn't specify a variant, then picking one that fits the board is
> fine.
> 
> If the device does not come in variants (and many physical devices
> don't), then rejecting it just because it's MSI-capable and the board
> isn't is not fine.
> 
> To fix, we'd have to limit what is now !msi_supported to the boards that
> are nominally MSI-capable, except our emulation doesn't work, i.e. do
> exactly what the commit message promised.

I rather think it's academic. MSI is a performance optimization, and is
optional for guests anyway.  It's hard to see when may users absolutely
require it.

> The PCI core encourages creating both variants, and most (but not all)
> device models do, but:
> 
> >> Furthermore, I doubt the wisdom of creating virtual devices that don't
> >> exist physically just to have something that works in our broken boards.
> >> If the physical device exists in MSI-capable and MSI-incapable variants,
> >> emulating both is fine.  But if it only ever existed MSI-capable, having
> >> the PCI core(!) drop the MSI capability is a questionable idea.  I
> >> suspect that the need for this dubious hack would be much smaller if we
> >> didn't foolishly treat every MSI-incapable board as broken MSI-capable
> >> board.
> >> 
> >> If you conclude that cleaning up this carelessly made mess is not worth
> >> the bother now, then at least explain the mess in the code, please.
> >> It's not obvious, and figuring out what's going on and why it is the way
> >> it is has taken me several hours, and Marcel's help.
> >
> > I think it's worth cleaning up, or at least documenting.
> > Fixing it will take much more than the patch proposed here,
> > and we can not start with this patch as it will cause
> > regressions.
> > Adding a comment won't be too much work.
> > How about the below?
> >
> > -->
> >
> > msi_supported -> msi_nonbroken
> >
> > Rename controller flag to make it clearer what it means.
> > Add some documentation as well.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > ---
> >
> > diff --git a/include/hw/pci/msi.h b/include/hw/pci/msi.h
> > index 50e452b..8124908 100644
> > --- a/include/hw/pci/msi.h
> > +++ b/include/hw/pci/msi.h
> > @@ -29,7 +29,7 @@ struct MSIMessage {
> >      uint32_t data;
> >  };
> >  
> > -extern bool msi_supported;
> > +extern bool msi_nonbroken;
> >  
> >  void msi_set_message(PCIDevice *dev, MSIMessage msg);
> >  MSIMessage msi_get_message(PCIDevice *dev, unsigned int vector);
> > diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
> > index 694d398..3c7c8fa 100644
> > --- a/hw/i386/kvm/apic.c
> > +++ b/hw/i386/kvm/apic.c
> > @@ -186,7 +186,7 @@ static void kvm_apic_realize(DeviceState *dev, Error **errp)
> >                            APIC_SPACE_SIZE);
> >  
> >      if (kvm_has_gsi_routing()) {
> > -        msi_supported = true;
> > +        msi_nonbroken = true;
> >      }
> >  }
> >  
> > diff --git a/hw/i386/xen/xen_apic.c b/hw/i386/xen/xen_apic.c
> > index 2b8d709..21d68ee 100644
> > --- a/hw/i386/xen/xen_apic.c
> > +++ b/hw/i386/xen/xen_apic.c
> > @@ -44,7 +44,7 @@ static void xen_apic_realize(DeviceState *dev, Error **errp)
> >      s->vapic_control = 0;
> >      memory_region_init_io(&s->io_memory, OBJECT(s), &xen_apic_io_ops, s,
> >                            "xen-apic-msi", APIC_SPACE_SIZE);
> > -    msi_supported = true;
> > +    msi_nonbroken = true;
> >  }
> >  
> >  static void xen_apic_set_base(APICCommonState *s, uint64_t val)
> > diff --git a/hw/intc/apic.c b/hw/intc/apic.c
> > index a299462..28c2ea5 100644
> > --- a/hw/intc/apic.c
> > +++ b/hw/intc/apic.c
> > @@ -874,7 +874,7 @@ static void apic_realize(DeviceState *dev, Error **errp)
> >      s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, apic_timer, s);
> >      local_apics[s->idx] = s;
> >  
> > -    msi_supported = true;
> > +    msi_nonbroken = true;
> >  }
> >  
> >  static void apic_class_init(ObjectClass *klass, void *data)
> > diff --git a/hw/intc/arm_gicv2m.c b/hw/intc/arm_gicv2m.c
> > index 70c0b97..ebd368b 100644
> > --- a/hw/intc/arm_gicv2m.c
> > +++ b/hw/intc/arm_gicv2m.c
> > @@ -148,7 +148,7 @@ static void gicv2m_realize(DeviceState *dev, Error **errp)
> >          sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->spi[i]);
> >      }
> >  
> > -    msi_supported = true;
> > +    msi_nonbroken = true;
> >      kvm_gsi_direct_mapping = true;
> >      kvm_msi_via_irqfd_allowed = kvm_irqfds_enabled();
> >  }
> > diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
> > index 903888c..7685250 100644
> > --- a/hw/intc/openpic.c
> > +++ b/hw/intc/openpic.c
> > @@ -1375,7 +1375,7 @@ static void fsl_common_init(OpenPICState *opp)
> >  
> >      opp->irq_msi = 224;
> >  
> > -    msi_supported = true;
> > +    msi_nonbroken = true;
> >      for (i = 0; i < opp->fsl->max_ext; i++) {
> >          opp->src[i].level = false;
> >      }
> > diff --git a/hw/intc/openpic_kvm.c b/hw/intc/openpic_kvm.c
> > index 4dcdb61..778af4a 100644
> > --- a/hw/intc/openpic_kvm.c
> > +++ b/hw/intc/openpic_kvm.c
> > @@ -239,7 +239,7 @@ static void kvm_openpic_realize(DeviceState *dev, Error **errp)
> >      memory_listener_register(&opp->mem_listener, &address_space_memory);
> >  
> >      /* indicate pic capabilities */
> > -    msi_supported = true;
> > +    msi_nonbroken = true;
> >      kvm_kernel_irqchip = true;
> >      kvm_async_interrupts_allowed = true;
> >  
> > diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
> > index 100bb5e..862a2366 100644
> > --- a/hw/pci-bridge/pci_bridge_dev.c
> > +++ b/hw/pci-bridge/pci_bridge_dev.c
> > @@ -72,7 +72,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
> >          goto slotid_error;
> >      }
> >      if ((bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_MSI_REQ)) &&
> > -        msi_supported) {
> > +        msi_nonbroken) {
> >          err = msi_init(dev, 0, 1, true, true);
> >          if (err < 0) {
> >              goto msi_error;
> > diff --git a/hw/pci/msi.c b/hw/pci/msi.c
> > index 85f21b8..e0e64c2 100644
> > --- a/hw/pci/msi.c
> > +++ b/hw/pci/msi.c
> > @@ -34,8 +34,21 @@
> >  
> >  #define PCI_MSI_VECTORS_MAX     32
> >  
> > -/* Flag for interrupt controller to declare MSI/MSI-X support */
> > -bool msi_supported;
> > +/*
> > + * Flag for interrupt controllers to declare broken MSI/MSI-X support.
> > + * values: false - broken; true - non-broken.
> > + *
> > + * Setting this flag to false will remove MSI/MSI-X capability from all devices.
> > + *
> > + * It is preferrable for controllers to set this to true (non-broken) even if
> > + * they do not actually support MSI/MSI-X: guests normally probe the controller
> > + * type and do not attempt to enable MSI/MSI-X with interrupt controllers not
> > + * supporting such, so removing the capability is not required, and
> > + * it seems cleaner to have a given device look the same for all boards.
> > + *
> > + * TODO: some existing controllers violate the above rule. Identify and fix them.
> > + */
> > +bool msi_nonbroken;
> 
> Good description.  I'd use FIXME rather than TODO, but that's detail.
> 
> >  
> >  /* If we get rid of cap allocator, we won't need this. */
> >  static inline uint8_t msi_cap_sizeof(uint16_t flags)
> > @@ -160,7 +173,7 @@ int msi_init(struct PCIDevice *dev, uint8_t offset,
> >      uint8_t cap_size;
> >      int config_offset;
> >  
> > -    if (!msi_supported) {
> > +    if (!msi_nonbroken) {
> >          return -ENOTSUP;
> >      }
> >  
> > diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> > index 537fdba..b75f0e9 100644
> > --- a/hw/pci/msix.c
> > +++ b/hw/pci/msix.c
> > @@ -249,7 +249,7 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
> >      uint8_t *config;
> >  
> >      /* Nothing to do if MSI is not supported by interrupt controller */
> > -    if (!msi_supported) {
> > +    if (!msi_nonbroken) {
> >          return -ENOTSUP;
> >      }
> >  
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index e9d4abf..c4fb206 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -439,7 +439,7 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
> >      _FDT((fdt_property_cell(fdt, "rtas-event-scan-rate",
> >                              RTAS_EVENT_SCAN_RATE)));
> >  
> > -    if (msi_supported) {
> > +    if (msi_nonbroken) {
> >          _FDT((fdt_property(fdt, "ibm,change-msix-capable", NULL, 0)));
> >      }
> >  
> > @@ -1743,7 +1743,7 @@ static void ppc_spapr_init(MachineState *machine)
> >      bool kernel_le = false;
> >      char *filename;
> >  
> > -    msi_supported = true;
> > +    msi_nonbroken = true;
> >  
> >      QLIST_INIT(&spapr->phbs);
> >  
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index e8edad3..3fc7895 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -1790,7 +1790,7 @@ void spapr_pci_rtas_init(void)
> >                          rtas_ibm_read_pci_config);
> >      spapr_rtas_register(RTAS_IBM_WRITE_PCI_CONFIG, "ibm,write-pci-config",
> >                          rtas_ibm_write_pci_config);
> > -    if (msi_supported) {
> > +    if (msi_nonbroken) {
> >          spapr_rtas_register(RTAS_IBM_QUERY_INTERRUPT_SOURCE_NUMBER,
> >                              "ibm,query-interrupt-source-number",
> >                              rtas_ibm_query_interrupt_source_number);
> > diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> > index dba0202..f5f679f 100644
> > --- a/hw/s390x/s390-pci-bus.c
> > +++ b/hw/s390x/s390-pci-bus.c
> > @@ -597,7 +597,7 @@ static void s390_pcihost_class_init(ObjectClass *klass, void *data)
> >      k->init = s390_pcihost_init;
> >      hc->plug = s390_pcihost_hot_plug;
> >      hc->unplug = s390_pcihost_hot_unplug;
> > -    msi_supported = true;
> > +    msi_nonbroken = true;
> >  }
> >  
> >  static const TypeInfo s390_pcihost_info = {
> 
> Much appreciated!
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH RFC v2 1/2] Add param Error** to msi_init() & modify the callers

[Markus Armbruster]

Marcel Apfelbaum <marcel@redhat.com> writes:

> On 03/02/2016 11:13 AM, Markus Armbruster wrote:
>> This got lost over the Christmas break, sorry.
>>
>> Cc'ing Marcel for additional PCI expertise.
>>
>> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>>
>>> msi_init() is a supporting function in PCI device initialization,
>>> in order to convert .init() to .realize(), it should be modified first.
>>
>> "Supporting function" doesn't imply "should use Error to report
>> errors".  HACKING explains:
>>
>>      Use the simplest suitable method to communicate success / failure to
>>      callers.  Stick to common methods: non-negative on success / -1 on
>>      error, non-negative / -errno, non-null / null, or Error objects.
>>
>>      Example: when a function returns a non-null pointer on success, and it
>>      can fail only in one way (as far as the caller is concerned), returning
>>      null on failure is just fine, and certainly simpler and a lot easier on
>>      the eyes than propagating an Error object through an Error ** parameter.
>>
>>      Example: when a function's callers need to report details on failure
>>      only the function really knows, use Error **, and set suitable errors.
>>
>>      Do not report an error to the user when you're also returning an error
>>      for somebody else to handle.  Leave the reporting to the place that
>>      consumes the error returned.
>>
>> As we'll see in your patch of msi.c, msi_init() can fail in multiple
>> ways, and uses -errno to comunicate them.  That can be okay even in
>> realize().  It also reports to the user.  That's what makes it
>> unsuitable for realize().
>>
>>> Also modify the callers
>>
>> Actually, you're *fixing* callers!  But the bugs aren't 100% clear, yet,
>> see below for details.  Once we know what the bugs are, we'll want to
>> reword the commit message to describe the bugs and their impact.
>>
>> I recommend to skip ahead to msi.c, then come back to the device models.
>>
>>> Bonus: add more comment for msi_init().
>>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>>> ---
>>>   hw/audio/intel-hda.c               | 10 ++++-
>>>   hw/ide/ich.c                       |  2 +-
>>>   hw/net/vmxnet3.c                   | 13 +++---
>>>   hw/pci-bridge/ioh3420.c            |  7 +++-
>>>   hw/pci-bridge/pci_bridge_dev.c     |  8 +++-
>>>   hw/pci-bridge/xio3130_downstream.c |  8 +++-
>>>   hw/pci-bridge/xio3130_upstream.c   |  8 +++-
>>>   hw/pci/msi.c                       | 18 +++++++--
>>>   hw/scsi/megasas.c                  | 15 +++++--
>>>   hw/scsi/vmw_pvscsi.c               | 13 ++++--
>>>   hw/usb/hcd-xhci.c                  | 81 +++++++++++++++++++++-----------------
>>>   hw/vfio/pci.c                      | 20 +++++-----
>>>   include/hw/pci/msi.h               |  4 +-
>>>   13 files changed, 135 insertions(+), 72 deletions(-)
>>>
>
> [...]
>
>> Except I'm not sure the function should fail in the first place!  See
>> below.
>>
>>> +int msi_init(struct PCIDevice *dev, uint8_t offset, unsigned int nr_vectors,
>>> +             bool msi64bit, bool msi_per_vector_mask, Error **errp)
>>>   {
>>>       unsigned int vectors_order;
>>> -    uint16_t flags;
>>> +    uint16_t flags; /* Message Control register value */
>>>       uint8_t cap_size;
>>>       int config_offset;
>>>
>>>       if (!msi_supported) {
>>> +        error_setg(errp, "MSI is not supported by interrupt controller");
>>>           return -ENOTSUP;
>>
>> First failure mode: board does not support MSI (-ENOTSUP).
>>
>> Question to the PCI guys: why is this even an error?  A device with
>> capability MSI should work just fine in such a board.
>
> Hi Markus,
>
> Adding Jan Kiszka, maybe he can help.
>
> That's a fair question. Is there any history for this decision?
> The board not supporting MSI has nothing to do with the capability being there.
> The HW should not change because the board doe not support it.
>
> The capability should be present but not active.
>
>>
>>>       }
>>>
>>> @@ -182,7 +190,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);
>>
>> pci_add_capability() is a wrapper around pci_add_capability2() that
>> additionally reports errors with error_report_err().  This is what makes
>> it unsuitable for realize().
>>
>>>       if (config_offset < 0) {
>>>           return config_offset;
>>
>> Inherits failing modes from pci_add_capability2().  Two: out of PCI
>> config space (-ENOSPC), and capability overlap (-EINVAL).
>>
>> Question for the PCI guys: how can these happen?  When they happen, is
>> it a programming error?
>
> out of PCI config space: a device emulation error, not enough room
> for all its capabilities - it seems to be a programming error.

Programming error should be an assertion failure, not falling to a
variant of the device the user didn't order and that might not even
exist in the real world.

> capability overlap: is for device assignment. This checks for a real HW
> that is broke. - not a programming error.

Okay.

Thanks!

[...]

Re: [Qemu-devel] [PATCH RFC v2 1/2] Add param Error** to msi_init() & modify the callers

[Cao jin]

On 03/02/2016 05:13 PM, Markus Armbruster wrote:
> This got lost over the Christmas break, sorry.
>
> Cc'ing Marcel for additional PCI expertise.
>
> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>
>> msi_init() is a supporting function in PCI device initialization,
>> in order to convert .init() to .realize(), it should be modified first.
>
> "Supporting function" doesn't imply "should use Error to report
> errors".  HACKING explains:
>
>      Use the simplest suitable method to communicate success / failure to
>      callers.  Stick to common methods: non-negative on success / -1 on
>      error, non-negative / -errno, non-null / null, or Error objects.
>
>      Example: when a function returns a non-null pointer on success, and it
>      can fail only in one way (as far as the caller is concerned), returning
>      null on failure is just fine, and certainly simpler and a lot easier on
>      the eyes than propagating an Error object through an Error ** parameter.
>
>      Example: when a function's callers need to report details on failure
>      only the function really knows, use Error **, and set suitable errors.
>
>      Do not report an error to the user when you're also returning an error
>      for somebody else to handle.  Leave the reporting to the place that
>      consumes the error returned.
>

Really appreciate your review, I just finished reading all the comments 
and discussion.

Seems pci_add_capability2()(commit cd9aa33e introduced) doesn`t follow 
the new error reporting rule(report error while also return error).

So I am thinking, could we revert commit cd9aa33e, let 
pci_add_capability() return error code and assert when out of pci space, 
and let caller(only assigned device, others could ignore the error) 
handle the error code(new a error object, propagate it)

Hope to hear PCI Maintainer`s advice(So I don`t cc other in this round)

-- 
Yours Sincerely,

Cao jin

Re: [Qemu-devel] [PATCH RFC v2 1/2] Add param Error** to msi_init() & modify the callers

[Markus Armbruster]

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

> On 03/02/2016 05:13 PM, Markus Armbruster wrote:
>> This got lost over the Christmas break, sorry.
>>
>> Cc'ing Marcel for additional PCI expertise.
>>
>> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>>
>>> msi_init() is a supporting function in PCI device initialization,
>>> in order to convert .init() to .realize(), it should be modified first.
>>
>> "Supporting function" doesn't imply "should use Error to report
>> errors".  HACKING explains:
>>
>>      Use the simplest suitable method to communicate success / failure to
>>      callers.  Stick to common methods: non-negative on success / -1 on
>>      error, non-negative / -errno, non-null / null, or Error objects.
>>
>>      Example: when a function returns a non-null pointer on success, and it
>>      can fail only in one way (as far as the caller is concerned), returning
>>      null on failure is just fine, and certainly simpler and a lot easier on
>>      the eyes than propagating an Error object through an Error ** parameter.
>>
>>      Example: when a function's callers need to report details on failure
>>      only the function really knows, use Error **, and set suitable errors.
>>
>>      Do not report an error to the user when you're also returning an error
>>      for somebody else to handle.  Leave the reporting to the place that
>>      consumes the error returned.
>>
>
> Really appreciate your review, I just finished reading all the
> comments and discussion.
>
> Seems pci_add_capability2()(commit cd9aa33e introduced) doesn`t follow
> the new error reporting rule(report error while also return error).

Misunderstanding?

"Report an error to the user" means error_report() and such.
error_setg() doesn't report to the user, it returns an error object to
the caller.

> So I am thinking, could we revert commit cd9aa33e, let
> pci_add_capability() return error code and assert when out of pci
> space, and let caller(only assigned device, others could ignore the
> error) handle the error code(new a error object, propagate it)
>
> Hope to hear PCI Maintainer`s advice(So I don`t cc other in this round)

Re: [Qemu-devel] [PATCH RFC v2 1/2] Add param Error** to msi_init() & modify the callers

[Cao jin]

On 03/23/2016 04:12 PM, Markus Armbruster wrote:
> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>

>>
>> Really appreciate your review, I just finished reading all the
>> comments and discussion.
>>
>> Seems pci_add_capability2()(commit cd9aa33e introduced) doesn`t follow
>> the new error reporting rule(report error while also return error).
>
> Misunderstanding?
>
> "Report an error to the user" means error_report() and such.
> error_setg() doesn't report to the user, it returns an error object to
> the caller.
>

ah...thanks for correcting me.

>> So I am thinking, could we revert commit cd9aa33e, let
>> pci_add_capability() return error code and assert when out of pci
>> space, and let caller(only assigned device, others could ignore the
>> error) handle the error code(new a error object, propagate it)
>>
>> Hope to hear PCI Maintainer`s advice(So I don`t cc other in this round)
>
-- 
Yours Sincerely,

Cao jin

[Qemu-devel] [PATCH v2 RFC 2/2] Add param Error** to msix_init() & modify the callers

[Cao jin]

msix_init() is a supporting function in PCI device initialization,
in order to convert .init() to .realize(), it should be modified first.
Also modify the callers.

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/block/nvme.c        | 32 +++++++++++++++++--------
 hw/misc/ivshmem.c      |  7 +++---
 hw/net/rocker/rocker.c | 10 +++++---
 hw/net/vmxnet3.c       | 26 ++++++++++++++------
 hw/pci/msix.c          | 20 ++++++++++++----
 hw/scsi/megasas.c      | 20 ++++++++++++----
 hw/usb/hcd-xhci.c      |  7 ++++--
 hw/vfio/pci.c          |  8 +++----
 hw/virtio/virtio-bus.c |  3 +++
 hw/virtio/virtio-pci.c | 64 +++++++++++++++++++++++++-------------------------
 include/hw/pci/msix.h  |  5 ++--
 11 files changed, 129 insertions(+), 73 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 169e4fa..21b6479 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -778,14 +778,27 @@ static const MemoryRegionOps nvme_mmio_ops = {
     },
 };
 
+static void nvme_exit(PCIDevice *pci_dev)
+{
+    NvmeCtrl *n = NVME(pci_dev);
+
+    nvme_clear_ctrl(n);
+    g_free(n->namespaces);
+    g_free(n->cq);
+    g_free(n->sq);
+    msix_uninit_exclusive_bar(pci_dev);
+}
+
 static int nvme_init(PCIDevice *pci_dev)
 {
     NvmeCtrl *n = NVME(pci_dev);
     NvmeIdCtrl *id = &n->id_ctrl;
+    Error *local_err = NULL;
 
     int i;
     int64_t bs_size;
     uint8_t *pci_conf;
+    int ret;
 
     if (!n->conf.blk) {
         return -1;
@@ -822,7 +835,11 @@ static int nvme_init(PCIDevice *pci_dev)
     pci_register_bar(&n->parent_obj, 0,
         PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_64,
         &n->iomem);
-    msix_init_exclusive_bar(&n->parent_obj, n->num_queues, 4);
+    ret = msix_init_exclusive_bar(&n->parent_obj, n->num_queues, 4, &local_err);
+    if (ret) {
+        error_report_err(local_err);
+        goto cleanup_on_msix_fail;
+    }
 
     id->vid = cpu_to_le16(pci_get_word(pci_conf + PCI_VENDOR_ID));
     id->ssvid = cpu_to_le16(pci_get_word(pci_conf + PCI_SUBSYSTEM_VENDOR_ID));
@@ -872,17 +889,12 @@ static int nvme_init(PCIDevice *pci_dev)
                 id_ns->lbaf[NVME_ID_NS_FLBAS_INDEX(ns->id_ns.flbas)].ds);
     }
     return 0;
-}
 
-static void nvme_exit(PCIDevice *pci_dev)
-{
-    NvmeCtrl *n = NVME(pci_dev);
+cleanup_on_msix_fail:
+    object_unref(OBJECT(&n->iomem));
+    nvme_exit(pci_dev);
 
-    nvme_clear_ctrl(n);
-    g_free(n->namespaces);
-    g_free(n->cq);
-    g_free(n->sq);
-    msix_uninit_exclusive_bar(pci_dev);
+    return ret;
 }
 
 static Property nvme_props[] = {
diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index f73f0c2..cfb210f 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -771,9 +771,9 @@ static void ivshmem_reset(DeviceState *d)
     ivshmem_use_msix(s);
 }
 
-static int ivshmem_setup_msi(IVShmemState * s)
+static int ivshmem_setup_msi(IVShmemState *s, Error **errp)
 {
-    if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1)) {
+    if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1, errp)) {
         return -1;
     }
 
@@ -950,8 +950,7 @@ static void pci_ivshmem_realize(PCIDevice *dev, Error **errp)
                         s->server_chr->filename);
 
         if (ivshmem_has_feature(s, IVSHMEM_MSI) &&
-            ivshmem_setup_msi(s)) {
-            error_setg(errp, "msix initialization failed");
+            ivshmem_setup_msi(s, errp)) {
             return;
         }
 
diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
index c57f1a6..64e4c2a 100644
--- a/hw/net/rocker/rocker.c
+++ b/hw/net/rocker/rocker.c
@@ -1244,7 +1244,7 @@ rollback:
     return err;
 }
 
-static int rocker_msix_init(Rocker *r)
+static int rocker_msix_init(Rocker *r, Error **errp)
 {
     PCIDevice *dev = PCI_DEVICE(r);
     int err;
@@ -1254,7 +1254,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, errp);
     if (err) {
         return err;
     }
@@ -1286,6 +1286,7 @@ static int pci_rocker_init(PCIDevice *dev)
     const MACAddr dflt = { .a = { 0x52, 0x54, 0x00, 0x12, 0x35, 0x01 } };
     static int sw_index;
     int i, err = 0;
+    Error *local_err = NULL;
 
     /* allocate worlds */
 
@@ -1314,8 +1315,9 @@ static int pci_rocker_init(PCIDevice *dev)
 
     /* MSI-X init */
 
-    err = rocker_msix_init(r);
+    err = rocker_msix_init(r, &local_err);
     if (err) {
+        error_report_err(local_err);
         goto err_msix_init;
     }
 
@@ -1431,6 +1433,8 @@ err_duplicate:
 err_msix_init:
     object_unparent(OBJECT(&r->msix_bar));
     object_unparent(OBJECT(&r->mmio));
+    object_unref(OBJECT(&r->msix_bar));
+    object_unref(OBJECT(&r->mmio));
 err_world_alloc:
     for (i = 0; i < ROCKER_WORLD_TYPE_MAX; i++) {
         if (r->worlds[i]) {
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index c373e77..b40d70c 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -2078,37 +2078,42 @@ vmxnet3_unuse_msix_vectors(VMXNET3State *s, int num_vectors)
 }
 
 static bool
-vmxnet3_use_msix_vectors(VMXNET3State *s, int num_vectors)
+vmxnet3_use_msix_vectors(VMXNET3State *s, int num_vectors, Error **errp)
 {
     PCIDevice *d = PCI_DEVICE(s);
     int i;
+
     for (i = 0; i < num_vectors; i++) {
         int res = msix_vector_use(d, i);
         if (0 > res) {
             VMW_WRPRN("Failed to use MSI-X vector %d, error %d", i, res);
+            error_setg(errp, "Failed to use MSI-X vector %d, error %d",
+                    i, res);
             vmxnet3_unuse_msix_vectors(s, i);
             return false;
         }
     }
+
     return true;
 }
 
 static bool
-vmxnet3_init_msix(VMXNET3State *s)
+vmxnet3_init_msix(VMXNET3State *s, Error **errp)
 {
     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,
-                        0);
+                        0, errp);
 
     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)) {
+        if (!vmxnet3_use_msix_vectors(s, VMXNET3_MAX_INTRS, errp)) {
             VMW_WRPRN("Failed to use MSI-X vectors, error %d", res);
             msix_uninit(d, &s->msix_bar, &s->msix_bar);
             s->msix_used = false;
@@ -2223,20 +2228,25 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
                        VMXNET3_MSIX_BAR_SIZE);
     pci_register_bar(pci_dev, VMXNET3_MSIX_BAR_IDX,
                      PCI_BASE_ADDRESS_SPACE_MEMORY, &s->msix_bar);
-
     vmxnet3_reset_interrupt_states(s);
 
     /* Interrupt pin A */
     pci_dev->config[PCI_INTERRUPT_PIN] = 0x01;
 
-    if (!vmxnet3_init_msix(s)) {
+    if (!vmxnet3_init_msix(s, errp)) {
         VMW_WRPRN("Failed to initialize MSI-X, configuration is inconsistent.");
+        goto cleanup_on_msix_fail;
     }
 
     vmxnet3_net_init(s);
 
     register_savevm(dev, "vmxnet3-msix", -1, 1,
                     vmxnet3_msix_save, vmxnet3_msix_load, s);
+
+cleanup_on_msix_fail:
+    object_unref(OBJECT(&s->bar0));
+    object_unref(OBJECT(&s->bar1));
+    object_unref(OBJECT(&s->msix_bar));
 }
 
 static void vmxnet3_instance_init(Object *obj)
@@ -2453,13 +2463,15 @@ static int vmxnet3_post_load(void *opaque, int version_id)
 {
     VMXNET3State *s = opaque;
     PCIDevice *d = PCI_DEVICE(s);
+    Error *local_err = NULL;
 
     vmxnet_tx_pkt_init(&s->tx_pkt, s->max_tx_frags, s->peer_has_vhdr);
     vmxnet_rx_pkt_init(&s->rx_pkt, s->peer_has_vhdr);
 
     if (s->msix_used) {
-        if  (!vmxnet3_use_msix_vectors(s, VMXNET3_MAX_INTRS)) {
+        if  (!vmxnet3_use_msix_vectors(s, VMXNET3_MAX_INTRS, &local_err)) {
             VMW_WRPRN("Failed to re-use MSI-X vectors");
+            error_report_err(local_err);
             msix_uninit(d, &s->msix_bar, &s->msix_bar);
             s->msix_used = false;
             return -1;
diff --git a/hw/pci/msix.c b/hw/pci/msix.c
index 64c93d8..44abd58 100644
--- a/hw/pci/msix.c
+++ b/hw/pci/msix.c
@@ -233,7 +233,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;
@@ -241,10 +242,13 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
 
     /* Nothing to do if MSI is not supported by interrupt controller */
     if (!msi_supported) {
+        error_setg(errp, "MSI-X is not supported by interrupt controller");
         return -ENOTSUP;
     }
 
     if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1) {
+        error_setg(errp, "Vector number %d invalid, should be 1 ~ %d",
+                nentries, PCI_MSIX_FLAGS_QSIZE + 1);
         return -EINVAL;
     }
 
@@ -257,10 +261,12 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
         table_offset + table_size > memory_region_size(table_bar) ||
         pba_offset + pba_size > memory_region_size(pba_bar) ||
         (table_offset | pba_offset) & PCI_MSIX_FLAGS_BIRMASK) {
+        error_setg(errp, "Arguments invalid. Please check them carefully");
         return -EINVAL;
     }
 
-    cap = pci_add_capability(dev, PCI_CAP_ID_MSIX, cap_pos, MSIX_CAP_LENGTH);
+    cap = pci_add_capability2(dev, PCI_CAP_ID_MSIX, cap_pos,
+                              MSIX_CAP_LENGTH, errp);
     if (cap < 0) {
         return cap;
     }
@@ -297,7 +303,7 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
 }
 
 int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
-                            uint8_t bar_nr)
+                            uint8_t bar_nr, Error **errp)
 {
     int ret;
     char *name;
@@ -329,15 +335,19 @@ int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
     ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr,
                     0, &dev->msix_exclusive_bar,
                     bar_nr, bar_pba_offset,
-                    0);
+                    0, errp);
     if (ret) {
-        return ret;
+        goto cleanup_on_fail;
     }
 
     pci_register_bar(dev, bar_nr, PCI_BASE_ADDRESS_SPACE_MEMORY,
                      &dev->msix_exclusive_bar);
 
     return 0;
+
+cleanup_on_fail:
+    object_unref(OBJECT(&dev->msix_exclusive_bar));
+    return ret;
 }
 
 static void msix_free_irq_entries(PCIDevice *dev)
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index ba25b3a..465fadd 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -2357,11 +2357,18 @@ 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_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;
+    if (megasas_use_msix(s)) {
+        int ret;
+
+        ret = msix_init(dev, 15, &s->mmio_io, b->mmio_bar, 0x2000,
+                  &s->mmio_io, b->mmio_bar, 0x3800, 0x68, errp);
+        if (ret > 0) {
+            s->flags &= ~MEGASAS_MASK_USE_MSIX;
+        } else {
+            goto cleanup_on_msix_fail;
+        }
     }
+
     if (pci_is_express(dev)) {
         pcie_endpoint_cap_init(dev, 0xa0);
     }
@@ -2419,6 +2426,11 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
     if (!d->hotplugged) {
         scsi_bus_legacy_handle_cmdline(&s->bus, errp);
     }
+
+cleanup_on_msix_fail:
+    object_unref(OBJECT(&s->mmio_io));
+    object_unref(OBJECT(&s->port_io));
+    object_unref(OBJECT(&s->queue_io));
 }
 
 static Property megasas_properties_gen1[] = {
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 7cd5f6c..c7d2ea7 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -3686,10 +3686,13 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
         }
     }
     if (xhci_get_flag(xhci, XHCI_FLAG_USE_MSI_X)) {
-        msix_init(dev, xhci->numintrs,
+        ret = msix_init(dev, xhci->numintrs,
                   &xhci->mem, 0, OFF_MSIX_TABLE,
                   &xhci->mem, 0, OFF_MSIX_PBA,
-                  0x90);
+                  0x90, errp);
+        if (ret > 0) {
+            return;
+        }
     }
 
 cleanup_on_msi_fail:
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 633642e..fe88d57 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1247,7 +1247,7 @@ static int vfio_msix_early_setup(VFIOPCIDevice *vdev)
     return 0;
 }
 
-static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos)
+static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
 {
     int ret;
 
@@ -1255,12 +1255,12 @@ 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,
+                    errp);
     if (ret < 0) {
         if (ret == -ENOTSUP) {
             return 0;
         }
-        error_report("vfio: msix_init failed");
         return ret;
     }
 
@@ -1702,7 +1702,7 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos, Error **errp)
         ret = vfio_setup_pcie_cap(vdev, pos, size);
         break;
     case PCI_CAP_ID_MSIX:
-        ret = vfio_msix_setup(vdev, pos);
+        ret = vfio_msix_setup(vdev, pos, errp);
         break;
     case PCI_CAP_ID_PM:
         vfio_check_pm_reset(vdev, pos);
diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index 81c7cdd..d36edac 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -50,6 +50,9 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp)
 
     if (klass->device_plugged != NULL) {
         klass->device_plugged(qbus->parent, errp);
+        if (*errp) {
+            return;
+        }
     }
 
     /* Get the features of the plugged device. */
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 94667e6..f50f77f 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1619,6 +1619,25 @@ static void virtio_pci_modern_io_region_unmap(VirtIOPCIProxy *proxy,
                                 &region->mr);
 }
 
+static void virtio_pci_device_unplugged(DeviceState *d)
+{
+    VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
+    bool modern = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN);
+    bool modern_pio = proxy->flags & VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY;
+
+    virtio_pci_stop_ioeventfd(proxy);
+
+    if (modern) {
+        virtio_pci_modern_mem_region_unmap(proxy, &proxy->common);
+        virtio_pci_modern_mem_region_unmap(proxy, &proxy->isr);
+        virtio_pci_modern_mem_region_unmap(proxy, &proxy->device);
+        virtio_pci_modern_mem_region_unmap(proxy, &proxy->notify);
+        if (modern_pio) {
+            virtio_pci_modern_io_region_unmap(proxy, &proxy->notify_pio);
+        }
+    }
+}
+
 /* This is called by virtio-bus just after the device is plugged. */
 static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
 {
@@ -1651,6 +1670,19 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
     }
     config[PCI_INTERRUPT_PIN] = 1;
 
+    if (proxy->nvectors) {
+        int err = msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors,
+                                          proxy->msix_bar, errp);
+        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);
+            }
+            proxy->nvectors = 0;
+            return;
+        }
+    }
 
     if (modern) {
         struct virtio_pci_cap cap = {
@@ -1705,19 +1737,6 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
         pci_set_long(cfg_mask->pci_cfg_data, ~0x0);
     }
 
-    if (proxy->nvectors) {
-        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);
-            }
-            proxy->nvectors = 0;
-        }
-    }
-
     proxy->pci_dev.config_write = virtio_write_config;
     proxy->pci_dev.config_read = virtio_read_config;
 
@@ -1741,25 +1760,6 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
     virtio_add_feature(&vdev->host_features, VIRTIO_F_BAD_FEATURE);
 }
 
-static void virtio_pci_device_unplugged(DeviceState *d)
-{
-    VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
-    bool modern = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN);
-    bool modern_pio = proxy->flags & VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY;
-
-    virtio_pci_stop_ioeventfd(proxy);
-
-    if (modern) {
-        virtio_pci_modern_mem_region_unmap(proxy, &proxy->common);
-        virtio_pci_modern_mem_region_unmap(proxy, &proxy->isr);
-        virtio_pci_modern_mem_region_unmap(proxy, &proxy->device);
-        virtio_pci_modern_mem_region_unmap(proxy, &proxy->notify);
-        if (modern_pio) {
-            virtio_pci_modern_io_region_unmap(proxy, &proxy->notify_pio);
-        }
-    }
-}
-
 static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
 {
     VirtIOPCIProxy *proxy = VIRTIO_PCI(pci_dev);
diff --git a/include/hw/pci/msix.h b/include/hw/pci/msix.h
index 72e5f93..7f2b8cc 100644
--- a/include/hw/pci/msix.h
+++ b/include/hw/pci/msix.h
@@ -9,9 +9,10 @@ MSIMessage msix_get_message(PCIDevice *dev, unsigned int vector);
 int msix_init(PCIDevice *dev, unsigned short nentries,
               MemoryRegion *table_bar, uint8_t table_bar_nr,
               unsigned table_offset, MemoryRegion *pba_bar,
-              uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos);
+              uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos,
+              Error **errp);
 int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
-                            uint8_t bar_nr);
+                            uint8_t bar_nr, Error **errp);
 
 void msix_write_config(PCIDevice *dev, uint32_t address, uint32_t val, int len);
 
-- 
2.1.0

Re: [Qemu-devel] [PATCH RFC v2 0/2] MSI/MSIX: fix to catch and report errors

[Cao jin]

Ping.

About cleanup:
1. When realize()/init() fail, PCI device will free the config space 
memory, so the necessary cleanup I can find until now is: MemoryRegion 
object. Maybe I missed something to cleanup, hope for comments.

2. certain instance: move the msi/msxi init func to the beginning of the 
realize()/init() func, to avoid cleanup work.

@Markus: hope I didn`t miss anything you mentioned in last review;)

On 12/15/2015 06:43 PM, Cao jin wrote:
> msi_init() & msix_init() are supporting functions for PCI devices. catch the
> errors they produced and report.
>
> V2 changelog:
> 1. Modify as per Markus`s review
>    a. Try to cleanup on function fail, as possible as I can.
>    b. For .init() function, use error_report_err() and return non-zero value.
>    c. For .realize(), propagate the error.
>    d. Special case: TYPE_ICH9_AHCI, it is a on-board device initialized with
>       machine init, so don`t bother to cleanup on failure, as process will exit
>       anyway.
>
> Cao jin (2):
>    Add param Error** to msi_init() & modify the callers
>    Add param Error** to msix_init() & modify the callers
>
>   hw/audio/intel-hda.c               | 10 ++++-
>   hw/block/nvme.c                    | 32 +++++++++-----
>   hw/ide/ich.c                       |  2 +-
>   hw/misc/ivshmem.c                  |  7 ++-
>   hw/net/rocker/rocker.c             | 10 +++--
>   hw/net/vmxnet3.c                   | 39 +++++++++++------
>   hw/pci-bridge/ioh3420.c            |  7 ++-
>   hw/pci-bridge/pci_bridge_dev.c     |  8 +++-
>   hw/pci-bridge/xio3130_downstream.c |  8 +++-
>   hw/pci-bridge/xio3130_upstream.c   |  8 +++-
>   hw/pci/msi.c                       | 18 ++++++--
>   hw/pci/msix.c                      | 20 ++++++---
>   hw/scsi/megasas.c                  | 35 +++++++++++----
>   hw/scsi/vmw_pvscsi.c               | 13 ++++--
>   hw/usb/hcd-xhci.c                  | 88 +++++++++++++++++++++-----------------
>   hw/vfio/pci.c                      | 28 ++++++------
>   hw/virtio/virtio-bus.c             |  3 ++
>   hw/virtio/virtio-pci.c             | 64 +++++++++++++--------------
>   include/hw/pci/msi.h               |  4 +-
>   include/hw/pci/msix.h              |  5 ++-
>   20 files changed, 264 insertions(+), 145 deletions(-)
>

-- 
Yours Sincerely,

Cao Jin