diff mbox

[v7,12/15] pci: Convert msi_init() to Error and fix callers to check it

Message ID 1465200062-13824-13-git-send-email-caoj.fnst@cn.fujitsu.com (mailing list archive)
State New, archived
Headers show

Commit Message

Cao jin June 6, 2016, 8 a.m. UTC
msi_init() reports errors with error_report(), which is wrong
when it's used in realize().

Fix by converting it to Error.

Fix its callers to handle failure instead of ignoring it.

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

cc: Gerd Hoffmann <kraxel@redhat.com>
cc: John Snow <jsnow@redhat.com>
cc: Dmitry Fleytman <dmitry@daynix.com>
cc: Jason Wang <jasowang@redhat.com>
cc: Michael S. Tsirkin <mst@redhat.com>
cc: Hannes Reinecke <hare@suse.de>
cc: Paolo Bonzini <pbonzini@redhat.com>
cc: Alex Williamson <alex.williamson@redhat.com>
cc: Markus Armbruster <armbru@redhat.com>
cc: Marcel Apfelbaum <marcel@redhat.com>

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/audio/intel-hda.c               | 25 +++++++++++++++++++++----
 hw/ide/ich.c                       | 15 +++++++++------
 hw/net/vmxnet3.c                   | 38 +++++++++++++-------------------------
 hw/pci-bridge/ioh3420.c            |  6 +++++-
 hw/pci-bridge/pci_bridge_dev.c     | 20 ++++++++++++++++----
 hw/pci-bridge/xio3130_downstream.c |  6 +++++-
 hw/pci-bridge/xio3130_upstream.c   |  6 +++++-
 hw/pci/msi.c                       | 11 ++++++++---
 hw/scsi/megasas.c                  | 27 ++++++++++++++++++++++-----
 hw/scsi/mptsas.c                   | 31 ++++++++++++++++++++++++-------
 hw/scsi/vmw_pvscsi.c               |  4 +++-
 hw/usb/hcd-xhci.c                  | 23 +++++++++++++++++++----
 hw/vfio/pci.c                      |  7 +++++--
 include/hw/pci/msi.h               |  3 ++-
 14 files changed, 157 insertions(+), 65 deletions(-)

Comments

Hannes Reinecke June 6, 2016, 8:29 a.m. UTC | #1
On 06/06/2016 10:00 AM, Cao jin wrote:
> msi_init() reports errors with error_report(), which is wrong
> when it's used in realize().
> 
> Fix by converting it to Error.
> 
> Fix its callers to handle failure instead of ignoring it.
> 
> For those callers who don't handle the failure, it might happen:
> when user want msi on, but he doesn't get what he want because of
> msi_init fails silently.
> 
> cc: Gerd Hoffmann <kraxel@redhat.com>
> cc: John Snow <jsnow@redhat.com>
> cc: Dmitry Fleytman <dmitry@daynix.com>
> cc: Jason Wang <jasowang@redhat.com>
> cc: Michael S. Tsirkin <mst@redhat.com>
> cc: Hannes Reinecke <hare@suse.de>
> cc: Paolo Bonzini <pbonzini@redhat.com>
> cc: Alex Williamson <alex.williamson@redhat.com>
> cc: Markus Armbruster <armbru@redhat.com>
> cc: Marcel Apfelbaum <marcel@redhat.com>
> 
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/audio/intel-hda.c               | 25 +++++++++++++++++++++----
>  hw/ide/ich.c                       | 15 +++++++++------
>  hw/net/vmxnet3.c                   | 38 +++++++++++++-------------------------
>  hw/pci-bridge/ioh3420.c            |  6 +++++-
>  hw/pci-bridge/pci_bridge_dev.c     | 20 ++++++++++++++++----
>  hw/pci-bridge/xio3130_downstream.c |  6 +++++-
>  hw/pci-bridge/xio3130_upstream.c   |  6 +++++-
>  hw/pci/msi.c                       | 11 ++++++++---
>  hw/scsi/megasas.c                  | 27 ++++++++++++++++++++++-----
>  hw/scsi/mptsas.c                   | 31 ++++++++++++++++++++++++-------
>  hw/scsi/vmw_pvscsi.c               |  4 +++-
>  hw/usb/hcd-xhci.c                  | 23 +++++++++++++++++++----
>  hw/vfio/pci.c                      |  7 +++++--
>  include/hw/pci/msi.h               |  3 ++-
>  14 files changed, 157 insertions(+), 65 deletions(-)
> 
For the megasas parts:

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

Cheers,

Hannes
Cao jin June 6, 2016, 8:30 a.m. UTC | #2
On 06/06/2016 04:00 PM, Cao jin wrote:

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

>
> +    if (xhci->msi != ON_OFF_AUTO_OFF) {
> +        ret = msi_init(dev, 0x70, xhci->numintrs, true, false, &err);
> +        /* Any error other than -ENOTSUP(board's MSI support is broken)
> +         * is a programming error */
> +        assert(!ret || ret == -ENOTSUP);
> +        if (ret && xhci->msi == ON_OFF_AUTO_ON) {
> +            /* Can't satisfy user's explicit msi=on request, fail */
> +            error_append_hint(&err, "You have to use msi=auto (default) or "
> +                    "si=off with this machine type.\n");

sorry, I made a typo "si=off", maybe it could be fixed when be applied 
by maintainer
Markus Armbruster June 9, 2016, 3:53 p.m. UTC | #3
Cao jin <caoj.fnst@cn.fujitsu.com> writes:

> msi_init() reports errors with error_report(), which is wrong
> when it's used in realize().
>
> Fix by converting it to Error.
>
> Fix its callers to handle failure instead of ignoring it.
>
> For those callers who don't handle the failure, it might happen:
> when user want msi on, but he doesn't get what he want because of
> msi_init fails silently.
>
> cc: Gerd Hoffmann <kraxel@redhat.com>
> cc: John Snow <jsnow@redhat.com>
> cc: Dmitry Fleytman <dmitry@daynix.com>
> cc: Jason Wang <jasowang@redhat.com>
> cc: Michael S. Tsirkin <mst@redhat.com>
> cc: Hannes Reinecke <hare@suse.de>
> cc: Paolo Bonzini <pbonzini@redhat.com>
> cc: Alex Williamson <alex.williamson@redhat.com>
> cc: Markus Armbruster <armbru@redhat.com>
> cc: Marcel Apfelbaum <marcel@redhat.com>
>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/audio/intel-hda.c               | 25 +++++++++++++++++++++----
>  hw/ide/ich.c                       | 15 +++++++++------
>  hw/net/vmxnet3.c                   | 38 +++++++++++++-------------------------
>  hw/pci-bridge/ioh3420.c            |  6 +++++-
>  hw/pci-bridge/pci_bridge_dev.c     | 20 ++++++++++++++++----
>  hw/pci-bridge/xio3130_downstream.c |  6 +++++-
>  hw/pci-bridge/xio3130_upstream.c   |  6 +++++-
>  hw/pci/msi.c                       | 11 ++++++++---
>  hw/scsi/megasas.c                  | 27 ++++++++++++++++++++++-----
>  hw/scsi/mptsas.c                   | 31 ++++++++++++++++++++++++-------
>  hw/scsi/vmw_pvscsi.c               |  4 +++-
>  hw/usb/hcd-xhci.c                  | 23 +++++++++++++++++++----
>  hw/vfio/pci.c                      |  7 +++++--
>  include/hw/pci/msi.h               |  3 ++-
>  14 files changed, 157 insertions(+), 65 deletions(-)
>
> diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
> index 40a8772..d588fd5 100644
> --- a/hw/audio/intel-hda.c
> +++ b/hw/audio/intel-hda.c
> @@ -26,6 +26,7 @@
>  #include "intel-hda.h"
>  #include "intel-hda-defs.h"
>  #include "sysemu/dma.h"
> +#include "qapi/error.h"
>  
>  /* --------------------------------------------------------------------- */
>  /* hda bus                                                               */
> @@ -1131,6 +1132,8 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp)
>  {
>      IntelHDAState *d = INTEL_HDA(pci);
>      uint8_t *conf = d->pci.config;
> +    Error *err = NULL;
> +    int32_t ret;

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

>  
>      d->name = object_get_typename(OBJECT(d));
>  
> @@ -1139,13 +1142,27 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp)
>      /* HDCTL off 0x40 bit 0 selects signaling mode (1-HDA, 0 - Ac97) 18.1.19 */
>      conf[0x40] = 0x01;
>  
> +    if (d->msi != ON_OFF_AUTO_OFF) {
> +        ret = msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60,
> +                       1, true, false, &err);
> +        /* Any error other than -ENOTSUP(board's MSI support is broken)
> +         * is a programming error */
> +        assert(!ret || ret == -ENOTSUP);
> +        if (ret && d->msi == ON_OFF_AUTO_ON) {
> +            /* Can't satisfy user's explicit msi=on request, fail */
> +            error_append_hint(&err, "You have to use msi=auto (default) or "
> +                    "msi=off with this machine type.\n");
> +            error_propagate(errp, err);
> +            return;
> +        }
> +        assert(!err || d->msi == ON_OFF_AUTO_AUTO);
> +        /* With msi=auto, we fall back to MSI off silently */
> +        error_free(err);
> +    }
> +
>      memory_region_init_io(&d->mmio, OBJECT(d), &intel_hda_mmio_ops, d,
>                            "intel-hda", 0x4000);
>      pci_register_bar(&d->pci, 0, 0, &d->mmio);
> -    if (d->msi != ON_OFF_AUTO_OFF) {
> -         /* TODO check for errors */
> -        msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, 1, true, false);
> -    }
>  
>      hda_codec_bus_init(DEVICE(pci), &d->codecs, sizeof(d->codecs),
>                         intel_hda_response, intel_hda_xfer);
[...]
Markus Armbruster June 9, 2016, 3:54 p.m. UTC | #4
Cao jin <caoj.fnst@cn.fujitsu.com> writes:

> msi_init() reports errors with error_report(), which is wrong
> when it's used in realize().
>
> Fix by converting it to Error.
>
> Fix its callers to handle failure instead of ignoring it.
>
> For those callers who don't handle the failure, it might happen:
> when user want msi on, but he doesn't get what he want because of
> msi_init fails silently.
>
> cc: Gerd Hoffmann <kraxel@redhat.com>
> cc: John Snow <jsnow@redhat.com>
> cc: Dmitry Fleytman <dmitry@daynix.com>
> cc: Jason Wang <jasowang@redhat.com>
> cc: Michael S. Tsirkin <mst@redhat.com>
> cc: Hannes Reinecke <hare@suse.de>
> cc: Paolo Bonzini <pbonzini@redhat.com>
> cc: Alex Williamson <alex.williamson@redhat.com>
> cc: Markus Armbruster <armbru@redhat.com>
> cc: Marcel Apfelbaum <marcel@redhat.com>
>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
[...]
> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index 26119bf..5c57106 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -29,7 +29,7 @@
>  #include "hw/scsi/scsi.h"
>  #include "block/scsi.h"
>  #include "trace.h"
> -
> +#include "qapi/error.h"
>  #include "mfi.h"
>  
>  #define MEGASAS_VERSION_GEN1 "1.70"
> @@ -2330,6 +2330,8 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
>      MegasasBaseClass *b = MEGASAS_DEVICE_GET_CLASS(s);
>      uint8_t *pci_conf;
>      int i, bar_type;
> +    Error *err = NULL;
> +    int ret;
>  
>      pci_conf = dev->config;
>  
> @@ -2338,6 +2340,25 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
>      /* Interrupt pin 1 */
>      pci_conf[PCI_INTERRUPT_PIN] = 0x01;
>  
> +    if (megasas_use_msi(s)) {
> +        ret = msi_init(dev, 0x50, 1, true, false, &err);
> +        /* Any error other than -ENOTSUP(board's MSI support is broken)
> +         * is a programming error */
> +        assert(!ret || ret == -ENOTSUP);
> +        if (ret && s->msi == ON_OFF_AUTO_ON) {
> +            /* Can't satisfy user's explicit msi=on request, fail */
> +            error_append_hint(&err, "You have to use msi=auto (default) or "
> +                    "msi=off with this machine type.\n");
> +            error_propagate(errp, err);
> +            return;
> +        } else if (ret) {
> +            s->msi = ON_OFF_AUTO_OFF;
> +        }
> +
> +        /* With msi=auto, we fall back to MSI off silently */
> +        error_free(err);

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

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

Since you're not doing anything with err:

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

[...]
diff mbox

Patch

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