diff mbox

[v5,11/11] pci: Convert msi_init() to Error and fix callers to check it

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

Commit Message

Cao jin May 6, 2016, 4:20 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>
---
the affected device is modified in this way:
1. intel hd audio: move msi_init() above, save the strength to free the
   MemoryRegion when it fails.
2. ich9 ahci: move msi_init() above, save the strenth to free the resource
   allocated when calling achi_realize(). It doesn`t have msi property, so
   msi_init failure leads to fall back to INTx silently. Just free the error
   object
3. vmxnet3: move msi_init() above. Remove the unecessary vmxnet3_init_msi().
   It doesn`t have msi property, so msi_init() failure leads to fall back to
   INTx silently. Just free the error object.
4. ioh3420/xio3130_downstream/xio3130_upstream: they are pcie components, msi
   or msix is forced, catch error and report it right there.
5. pci_bridge_dev: msi_init`s failure is fatal, follow the behaviour.
6. megasas_scsi: move msi_init() above, save the strength to free the
   MemoryRegion when it fails.
7. mptsas: Move msi_init() above, save the strength to free the MemoryRegion
   when it fails.
8. pvscsi: it doesn`t have msi property, msi_init fail leads to fall back to
   INTx silently.
9. usb-xhci: move msi_init() above, save the strength to free the MemoryRegion
   when it fails.
10. vfio-pci: keep the previous behaviour, and just catch & report error.

 hw/audio/intel-hda.c               | 18 +++++++++++++----
 hw/ide/ich.c                       | 15 +++++++++-----
 hw/net/vmxnet3.c                   | 41 +++++++++++++++-----------------------
 hw/pci-bridge/ioh3420.c            |  4 +++-
 hw/pci-bridge/pci_bridge_dev.c     |  7 ++++---
 hw/pci-bridge/xio3130_downstream.c |  4 +++-
 hw/pci-bridge/xio3130_upstream.c   |  4 +++-
 hw/pci/msi.c                       |  9 +++++----
 hw/scsi/megasas.c                  | 18 +++++++++++++----
 hw/scsi/mptsas.c                   | 20 ++++++++++++++-----
 hw/scsi/vmw_pvscsi.c               |  6 +++++-
 hw/usb/hcd-xhci.c                  | 18 +++++++++++++----
 hw/vfio/pci.c                      |  6 ++++--
 include/hw/pci/msi.h               |  3 ++-
 14 files changed, 112 insertions(+), 61 deletions(-)

Comments

Marcel Apfelbaum May 15, 2016, 1:41 p.m. UTC | #1
On 05/06/2016 07:20 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>
> ---
> the affected device is modified in this way:
> 1. intel hd audio: move msi_init() above, save the strength to free the
>     MemoryRegion when it fails.
> 2. ich9 ahci: move msi_init() above, save the strenth to free the resource
>     allocated when calling achi_realize(). It doesn`t have msi property, so
>     msi_init failure leads to fall back to INTx silently. Just free the error
>     object
> 3. vmxnet3: move msi_init() above. Remove the unecessary vmxnet3_init_msi().
>     It doesn`t have msi property, so msi_init() failure leads to fall back to
>     INTx silently. Just free the error object.
> 4. ioh3420/xio3130_downstream/xio3130_upstream: they are pcie components, msi
>     or msix is forced, catch error and report it right there.
> 5. pci_bridge_dev: msi_init`s failure is fatal, follow the behaviour.
> 6. megasas_scsi: move msi_init() above, save the strength to free the
>     MemoryRegion when it fails.
> 7. mptsas: Move msi_init() above, save the strength to free the MemoryRegion
>     when it fails.
> 8. pvscsi: it doesn`t have msi property, msi_init fail leads to fall back to
>     INTx silently.
> 9. usb-xhci: move msi_init() above, save the strength to free the MemoryRegion
>     when it fails.
> 10. vfio-pci: keep the previous behaviour, and just catch & report error.
>
>   hw/audio/intel-hda.c               | 18 +++++++++++++----
>   hw/ide/ich.c                       | 15 +++++++++-----
>   hw/net/vmxnet3.c                   | 41 +++++++++++++++-----------------------
>   hw/pci-bridge/ioh3420.c            |  4 +++-
>   hw/pci-bridge/pci_bridge_dev.c     |  7 ++++---
>   hw/pci-bridge/xio3130_downstream.c |  4 +++-
>   hw/pci-bridge/xio3130_upstream.c   |  4 +++-
>   hw/pci/msi.c                       |  9 +++++----
>   hw/scsi/megasas.c                  | 18 +++++++++++++----
>   hw/scsi/mptsas.c                   | 20 ++++++++++++++-----
>   hw/scsi/vmw_pvscsi.c               |  6 +++++-
>   hw/usb/hcd-xhci.c                  | 18 +++++++++++++----
>   hw/vfio/pci.c                      |  6 ++++--
>   include/hw/pci/msi.h               |  3 ++-
>   14 files changed, 112 insertions(+), 61 deletions(-)
>
> diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
> index 61362e5..0a46358 100644
> --- a/hw/audio/intel-hda.c
> +++ b/hw/audio/intel-hda.c
> @@ -1131,6 +1131,7 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp)
>   {
>       IntelHDAState *d = INTEL_HDA(pci);
>       uint8_t *conf = d->pci.config;
> +    Error *err = NULL;
>
>       d->name = object_get_typename(OBJECT(d));
>
> @@ -1139,13 +1140,22 @@ 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) {
> +        msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, 1,
> +                 true, false, &err);
> +        if (err && d->msi == ON_OFF_AUTO_ON) {
> +            /* If user set msi=on, then device creation fail */
> +            error_propagate(errp, err);
> +            return;

The semantics now changed, old machines with msi=on on platforms with msi_nonbroken=false
will fail now, right? Is this acceptable or we need a compat way to kepp the semantics for old machines?

> +        } else if (err && d->msi == ON_OFF_AUTO_AUTO) {
> +            /* If user doesn`t set it on, switch to non-msi variant 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_AUTO ||
> -        d->msi == ON_OFF_AUTO_ON) {
> -        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..aec8262 100644
> --- a/hw/ide/ich.c
> +++ b/hw/ide/ich.c
> @@ -111,6 +111,16 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp)
>       int sata_cap_offset;
>       uint8_t *sata_cap;
>       d = ICH_AHCI(dev);
> +    Error *err = NULL;
> +
> +    /* 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, &err);
> +    if (err) {
> +        /* Fall back to INTx silently */
> +        error_free(err);
> +    }
>
>       ahci_realize(&d->ahci, DEVICE(dev), pci_get_address_space(dev), 6);
>
> @@ -142,11 +152,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..ab9a938 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -2189,27 +2189,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 +2250,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);
> +    Error *err = NULL;
>
>       VMW_CBPRN("Starting init...");
>
> @@ -2298,12 +2282,19 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
>       /* Interrupt pin A */
>       pci_dev->config[PCI_INTERRUPT_PIN] = 0x01;
>
> -    if (!vmxnet3_init_msix(s)) {
> -        VMW_WRPRN("Failed to initialize MSI-X, configuration is inconsistent.");
> +    msi_init(pci_dev, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS,
> +             VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK, &err);
> +    if (err) {
> +        /* Fall back to INTx silently */
> +        VMW_WRPRN("Failed to initialize MSI:  %s", error_get_pretty(err));
> +        error_free(err);
> +        s->msi_used = false;
> +    } else {
> +        s->msi_used = true;
>       }
>
> -    if (!vmxnet3_init_msi(s)) {
> -        VMW_WRPRN("Failed to initialize MSI, configuration is inconsistent.");
> +    if (!vmxnet3_init_msix(s)) {
> +        VMW_WRPRN("Failed to initialize MSI-X, configuration is inconsistent.");
>       }
>
>       vmxnet3_net_init(s);
> diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
> index b4a7806..d752e62 100644
> --- a/hw/pci-bridge/ioh3420.c
> +++ b/hw/pci-bridge/ioh3420.c
> @@ -97,6 +97,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 +110,9 @@ 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) {
> +        error_report_err(err);
>           goto err_bridge;
>       }

OK

>
> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
> index 9e31f0e..af71c98 100644
> --- a/hw/pci-bridge/pci_bridge_dev.c
> +++ b/hw/pci-bridge/pci_bridge_dev.c
> @@ -53,6 +53,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);
>
> @@ -74,11 +75,11 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>           goto slotid_error;
>       }
>
> -    if ((bridge_dev->msi == ON_OFF_AUTO_AUTO ||
> -                bridge_dev->msi == ON_OFF_AUTO_ON) &&
> +    if (bridge_dev->msi != ON_OFF_AUTO_OFF &&

So we made the msi property OnOffAuto, but we don't make a difference between ON and Auto?

>           msi_nonbroken) {
> -        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;
>           }
>       }
> diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
> index e6d653d..0982801 100644
> --- a/hw/pci-bridge/xio3130_downstream.c
> +++ b/hw/pci-bridge/xio3130_downstream.c
> @@ -60,14 +60,16 @@ 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) {
> +        error_report_err(err);
>           goto err_bridge;
>       }
>

OK


> diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c
> index d976844..1d2c597 100644
> --- a/hw/pci-bridge/xio3130_upstream.c
> +++ b/hw/pci-bridge/xio3130_upstream.c
> @@ -56,14 +56,16 @@ 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) {
> +        error_report_err(err);
>           goto err_bridge;
>       }
>

OK

> diff --git a/hw/pci/msi.c b/hw/pci/msi.c
> index dd9d957..662be56 100644
> --- a/hw/pci/msi.c
> +++ b/hw/pci/msi.c
> @@ -178,14 +178,13 @@ bool msi_enabled(const PCIDevice *dev)
>    * set @errp and return -errno on error.
>    *
>    * -ENOTSUP means lacking msi support for a msi-capable platform.
> - * -ENOSPC means running out of PCI config space, happens when @offset is 0,
> - *  which means a programming error.
>    * -EINVAL means capability overlap, happens when @offset is non-zero,
>    *  also means a programming error, except device assignment, which can check
>    *  if a real HW is broken.
>    */
>   int msi_init(struct PCIDevice *dev, uint8_t offset,
> -             unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask)
> +             unsigned int nr_vectors, bool msi64bit,
> +             bool msi_per_vector_mask, Error **errp)
>   {
>       unsigned int vectors_order;
>       uint16_t flags;
> @@ -193,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;
>       }
>
> @@ -216,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;
>       }

OK

> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index e71a28b..3585a6a 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -2330,6 +2330,7 @@ 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;
>
>       pci_conf = dev->config;
>
> @@ -2338,6 +2339,19 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
>       /* Interrupt pin 1 */
>       pci_conf[PCI_INTERRUPT_PIN] = 0x01;
>
> +    if (megasas_use_msi(s)) {
> +        msi_init(dev, 0x50, 1, true, false, &err);
> +        if (err && s->msi == ON_OFF_AUTO_ON) {
> +            /* If user set msi=on, then device creation fail */
> +            s->msi = ON_OFF_AUTO_OFF;
> +            error_propagate(errp, err);
> +            return;

The same question regarding the change of semantics.

> +        } else if (err && s->msi == ON_OFF_AUTO_AUTO) {
> +            /* If user doesn`t set it on, switch to non-msi variant 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 +2359,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) < 0) {
> -        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 afee576..aaee687 100644
> --- a/hw/scsi/mptsas.c
> +++ b/hw/scsi/mptsas.c
> @@ -1274,10 +1274,25 @@ static void mptsas_scsi_realize(PCIDevice *dev, Error **errp)
>   {
>       DeviceState *d = DEVICE(dev);
>       MPTSASState *s = MPT_SAS(dev);
> +    Error *err = NULL;
>
>       dev->config[PCI_LATENCY_TIMER] = 0;
>       dev->config[PCI_INTERRUPT_PIN] = 0x01;
>
> +    if (s->msi != ON_OFF_AUTO_OFF) {
> +        msi_init(dev, 0, 1, true, false, &err);
> +        if (err && s->msi == ON_OFF_AUTO_ON) {
> +            /* If user set msi=on, then device creation fail */
> +            error_propagate(errp, err);
> +            s->msi_in_use = false;


Same here

> +            return;
> +        } else if (err && s->msi == ON_OFF_AUTO_AUTO) {
> +            /* If user doesn`t set it on, switch to non-msi variant silently */
> +            error_free(err);
> +            s->msi_in_use = false;
> +        }
> +    }
> +
>       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,11 +1300,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_AUTO || s->msi == ON_OFF_AUTO_ON) &&
> -        msi_init(dev, 0, 1, true, false) >= 0) {
> -        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 4ce3581..2d38d6c 100644
> --- a/hw/scsi/vmw_pvscsi.c
> +++ b/hw/scsi/vmw_pvscsi.c
> @@ -1043,12 +1043,16 @@ 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_append_hint(&err, "MSI capability fail to init,"
> +                " will use INTx intead\n");
> +        error_report_err(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 6d70289..f17f242 100644
> --- a/hw/usb/hcd-xhci.c
> +++ b/hw/usb/hcd-xhci.c
> @@ -3578,6 +3578,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);
>
> @@ -3588,6 +3589,19 @@ 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);
> +        if (ret < 0 && xhci->msi == ON_OFF_AUTO_ON) {
> +            /* If user set msi=on, then device creation fail */
> +            error_propagate(errp, err);
> +            return;
> +        }
> +        else if (ret < 0 && xhci->msi == ON_OFF_AUTO_AUTO) {
> +            /* If user doesn`t set it on, switch to non-msi variant silently */
> +            error_free(err);
> +        }
> +    }
> +
>       if (xhci->numintrs > MAXINTRS) {
>           xhci->numintrs = MAXINTRS;
>       }
> @@ -3645,10 +3659,6 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
>           assert(ret >= 0);
>       }
>
> -    if (xhci->msi == ON_OFF_AUTO_ON ||
> -        xhci->msi == ON_OFF_AUTO_AUTO) {
> -        msi_init(dev, 0x70, xhci->numintrs, true, false);
> -    }
>       if (xhci->msix == ON_OFF_AUTO_ON ||
>           xhci->msix == ON_OFF_AUTO_AUTO) {
>           msix_init(dev, xhci->numintrs,
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index d091d8c..55ceb67 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -1171,6 +1171,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 +1185,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;
>       }

OK

>       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);
>


Thanks,
Marcel
Cao jin May 17, 2016, 10:08 a.m. UTC | #2
On 05/15/2016 09:41 PM, Marcel Apfelbaum wrote:

>
>>
>> diff --git a/hw/pci-bridge/pci_bridge_dev.c
>> b/hw/pci-bridge/pci_bridge_dev.c
>> index 9e31f0e..af71c98 100644
>> --- a/hw/pci-bridge/pci_bridge_dev.c
>> +++ b/hw/pci-bridge/pci_bridge_dev.c
>> @@ -53,6 +53,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);
>>
>> @@ -74,11 +75,11 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>>           goto slotid_error;
>>       }
>>
>> -    if ((bridge_dev->msi == ON_OFF_AUTO_AUTO ||
>> -                bridge_dev->msi == ON_OFF_AUTO_ON) &&
>> +    if (bridge_dev->msi != ON_OFF_AUTO_OFF &&
>
> So we made the msi property OnOffAuto, but we don't make a difference
> between ON and Auto?
>

That is why I am not quite sure about this device, msi has a relation 
with shpc.

 From its previous behaviour, it can be seen that it don`t treat 'on' as 
'auto'.(I am not sure why it is different with others)

Its previous behaviour treat msi_init`s failure as fatal, and lead to 
device creation fail. According to Markus`s comments(if I understand him 
right), this device has no non-msi variants. I think my patch follows 
the previous behaviour.

And also according to function comments:
/* MSI is not applicable without SHPC */
which means device either has both, or neither(if I understand it 
right), so that is why I don`t make a difference

>>           msi_nonbroken) {
>> -        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;
>>           }
>>       }
Marcel Apfelbaum May 23, 2016, 10:06 a.m. UTC | #3
On 05/17/2016 01:08 PM, Cao jin wrote:
>
>
> On 05/15/2016 09:41 PM, Marcel Apfelbaum wrote:
>
>>
>>>
>>> diff --git a/hw/pci-bridge/pci_bridge_dev.c
>>> b/hw/pci-bridge/pci_bridge_dev.c
>>> index 9e31f0e..af71c98 100644
>>> --- a/hw/pci-bridge/pci_bridge_dev.c
>>> +++ b/hw/pci-bridge/pci_bridge_dev.c
>>> @@ -53,6 +53,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);
>>>
>>> @@ -74,11 +75,11 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>>>           goto slotid_error;
>>>       }
>>>
>>> -    if ((bridge_dev->msi == ON_OFF_AUTO_AUTO ||
>>> -                bridge_dev->msi == ON_OFF_AUTO_ON) &&
>>> +    if (bridge_dev->msi != ON_OFF_AUTO_OFF &&
>>
>> So we made the msi property OnOffAuto, but we don't make a difference
>> between ON and Auto?
>>
>
> That is why I am not quite sure about this device, msi has a relation with shpc.
>
>  From its previous behaviour, it can be seen that it don`t treat 'on' as 'auto'.(I am not sure why it is different with others)
>
> Its previous behaviour treat msi_init`s failure as fatal, and lead to device creation fail. According to Markus`s comments(if I understand him right), this device has no non-msi variants.

Actually it has. The bridge needs msi for the SHPC controller, as you said.
If you look into shpc_interrupt_update function (hw/pci/shpc.c) you can see it can fall
back to legacy interrupts.

The only complication here is that msi is needed only if shpc is present.
So maybe having msi=on/off/auto is OK.

msi=auto => if shpc not present or msi broken results in msi = off, else msi = on
msi=on => fails if shpc present and msi broken
msi=off => use legacy interrupts if shpc is present


Basically the msi flag has no meaning if shpc not present.

Thanks,
Marcel

  I think my
> patch follows the previous behaviour.
>
> And also according to function comments:
> /* MSI is not applicable without SHPC */
> which means device either has both, or neither(if I understand it right), so that is why I don`t make a difference
>
>>>           msi_nonbroken) {
>>> -        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;
>>>           }
>>>       }
>
>
Cao jin May 23, 2016, 12:51 p.m. UTC | #4
On 05/23/2016 06:06 PM, Marcel Apfelbaum wrote:
> On 05/17/2016 01:08 PM, Cao jin wrote:
>>
>>
>> On 05/15/2016 09:41 PM, Marcel Apfelbaum wrote:
>>
>>
>> That is why I am not quite sure about this device, msi has a relation
>> with shpc.
>>
>>  From its previous behaviour, it can be seen that it don`t treat 'on'
>> as 'auto'.(I am not sure why it is different with others)
>>
>> Its previous behaviour treat msi_init`s failure as fatal, and lead to
>> device creation fail. According to Markus`s comments(if I understand
>> him right), this device has no non-msi variants.
>
> Actually it has. The bridge needs msi for the SHPC controller, as you said.
> If you look into shpc_interrupt_update function (hw/pci/shpc.c) you can
> see it can fall
> back to legacy interrupts.
>
> The only complication here is that msi is needed only if shpc is present.
> So maybe having msi=on/off/auto is OK.
>
> msi=auto => if shpc not present or msi broken results in msi = off, else
> msi = on
> msi=on => fails if shpc present and msi broken
> msi=off => use legacy interrupts if shpc is present
>
>
> Basically the msi flag has no meaning if shpc not present.
>

Thanks for the hint, v6 is on the way:)
diff mbox

Patch

diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
index 61362e5..0a46358 100644
--- a/hw/audio/intel-hda.c
+++ b/hw/audio/intel-hda.c
@@ -1131,6 +1131,7 @@  static void intel_hda_realize(PCIDevice *pci, Error **errp)
 {
     IntelHDAState *d = INTEL_HDA(pci);
     uint8_t *conf = d->pci.config;
+    Error *err = NULL;
 
     d->name = object_get_typename(OBJECT(d));
 
@@ -1139,13 +1140,22 @@  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) {
+        msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, 1,
+                 true, false, &err);
+        if (err && d->msi == ON_OFF_AUTO_ON) {
+            /* If user set msi=on, then device creation fail */
+            error_propagate(errp, err);
+            return;
+        } else if (err && d->msi == ON_OFF_AUTO_AUTO) {
+            /* If user doesn`t set it on, switch to non-msi variant 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_AUTO ||
-        d->msi == ON_OFF_AUTO_ON) {
-        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..aec8262 100644
--- a/hw/ide/ich.c
+++ b/hw/ide/ich.c
@@ -111,6 +111,16 @@  static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp)
     int sata_cap_offset;
     uint8_t *sata_cap;
     d = ICH_AHCI(dev);
+    Error *err = NULL;
+
+    /* 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, &err);
+    if (err) {
+        /* Fall back to INTx silently */
+        error_free(err);
+    }
 
     ahci_realize(&d->ahci, DEVICE(dev), pci_get_address_space(dev), 6);
 
@@ -142,11 +152,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..ab9a938 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -2189,27 +2189,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 +2250,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);
+    Error *err = NULL;
 
     VMW_CBPRN("Starting init...");
 
@@ -2298,12 +2282,19 @@  static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
     /* Interrupt pin A */
     pci_dev->config[PCI_INTERRUPT_PIN] = 0x01;
 
-    if (!vmxnet3_init_msix(s)) {
-        VMW_WRPRN("Failed to initialize MSI-X, configuration is inconsistent.");
+    msi_init(pci_dev, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS,
+             VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK, &err);
+    if (err) {
+        /* Fall back to INTx silently */
+        VMW_WRPRN("Failed to initialize MSI:  %s", error_get_pretty(err));
+        error_free(err);
+        s->msi_used = false;
+    } else {
+        s->msi_used = true;
     }
 
-    if (!vmxnet3_init_msi(s)) {
-        VMW_WRPRN("Failed to initialize MSI, configuration is inconsistent.");
+    if (!vmxnet3_init_msix(s)) {
+        VMW_WRPRN("Failed to initialize MSI-X, configuration is inconsistent.");
     }
 
     vmxnet3_net_init(s);
diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
index b4a7806..d752e62 100644
--- a/hw/pci-bridge/ioh3420.c
+++ b/hw/pci-bridge/ioh3420.c
@@ -97,6 +97,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 +110,9 @@  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) {
+        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 9e31f0e..af71c98 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -53,6 +53,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);
 
@@ -74,11 +75,11 @@  static int pci_bridge_dev_initfn(PCIDevice *dev)
         goto slotid_error;
     }
 
-    if ((bridge_dev->msi == ON_OFF_AUTO_AUTO ||
-                bridge_dev->msi == ON_OFF_AUTO_ON) &&
+    if (bridge_dev->msi != ON_OFF_AUTO_OFF &&
         msi_nonbroken) {
-        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;
         }
     }
diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
index e6d653d..0982801 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -60,14 +60,16 @@  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) {
+        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..1d2c597 100644
--- a/hw/pci-bridge/xio3130_upstream.c
+++ b/hw/pci-bridge/xio3130_upstream.c
@@ -56,14 +56,16 @@  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) {
+        error_report_err(err);
         goto err_bridge;
     }
 
diff --git a/hw/pci/msi.c b/hw/pci/msi.c
index dd9d957..662be56 100644
--- a/hw/pci/msi.c
+++ b/hw/pci/msi.c
@@ -178,14 +178,13 @@  bool msi_enabled(const PCIDevice *dev)
  * set @errp and return -errno on error.
  *
  * -ENOTSUP means lacking msi support for a msi-capable platform.
- * -ENOSPC means running out of PCI config space, happens when @offset is 0,
- *  which means a programming error.
  * -EINVAL means capability overlap, happens when @offset is non-zero,
  *  also means a programming error, except device assignment, which can check
  *  if a real HW is broken.
  */
 int msi_init(struct PCIDevice *dev, uint8_t offset,
-             unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask)
+             unsigned int nr_vectors, bool msi64bit,
+             bool msi_per_vector_mask, Error **errp)
 {
     unsigned int vectors_order;
     uint16_t flags;
@@ -193,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;
     }
 
@@ -216,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 e71a28b..3585a6a 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -2330,6 +2330,7 @@  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;
 
     pci_conf = dev->config;
 
@@ -2338,6 +2339,19 @@  static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
     /* Interrupt pin 1 */
     pci_conf[PCI_INTERRUPT_PIN] = 0x01;
 
+    if (megasas_use_msi(s)) {
+        msi_init(dev, 0x50, 1, true, false, &err);
+        if (err && s->msi == ON_OFF_AUTO_ON) {
+            /* If user set msi=on, then device creation fail */
+            s->msi = ON_OFF_AUTO_OFF;
+            error_propagate(errp, err);
+            return;
+        } else if (err && s->msi == ON_OFF_AUTO_AUTO) {
+            /* If user doesn`t set it on, switch to non-msi variant 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 +2359,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) < 0) {
-        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 afee576..aaee687 100644
--- a/hw/scsi/mptsas.c
+++ b/hw/scsi/mptsas.c
@@ -1274,10 +1274,25 @@  static void mptsas_scsi_realize(PCIDevice *dev, Error **errp)
 {
     DeviceState *d = DEVICE(dev);
     MPTSASState *s = MPT_SAS(dev);
+    Error *err = NULL;
 
     dev->config[PCI_LATENCY_TIMER] = 0;
     dev->config[PCI_INTERRUPT_PIN] = 0x01;
 
+    if (s->msi != ON_OFF_AUTO_OFF) {
+        msi_init(dev, 0, 1, true, false, &err);
+        if (err && s->msi == ON_OFF_AUTO_ON) {
+            /* If user set msi=on, then device creation fail */
+            error_propagate(errp, err);
+            s->msi_in_use = false;
+            return;
+        } else if (err && s->msi == ON_OFF_AUTO_AUTO) {
+            /* If user doesn`t set it on, switch to non-msi variant silently */
+            error_free(err);
+            s->msi_in_use = false;
+        }
+    }
+
     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,11 +1300,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_AUTO || s->msi == ON_OFF_AUTO_ON) &&
-        msi_init(dev, 0, 1, true, false) >= 0) {
-        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 4ce3581..2d38d6c 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -1043,12 +1043,16 @@  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_append_hint(&err, "MSI capability fail to init,"
+                " will use INTx intead\n");
+        error_report_err(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 6d70289..f17f242 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -3578,6 +3578,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);
 
@@ -3588,6 +3589,19 @@  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);
+        if (ret < 0 && xhci->msi == ON_OFF_AUTO_ON) {
+            /* If user set msi=on, then device creation fail */
+            error_propagate(errp, err);
+            return;
+        }
+        else if (ret < 0 && xhci->msi == ON_OFF_AUTO_AUTO) {
+            /* If user doesn`t set it on, switch to non-msi variant silently */
+            error_free(err);
+        }
+    }
+
     if (xhci->numintrs > MAXINTRS) {
         xhci->numintrs = MAXINTRS;
     }
@@ -3645,10 +3659,6 @@  static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
         assert(ret >= 0);
     }
 
-    if (xhci->msi == ON_OFF_AUTO_ON ||
-        xhci->msi == ON_OFF_AUTO_AUTO) {
-        msi_init(dev, 0x70, xhci->numintrs, true, false);
-    }
     if (xhci->msix == ON_OFF_AUTO_ON ||
         xhci->msix == ON_OFF_AUTO_AUTO) {
         msix_init(dev, xhci->numintrs,
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index d091d8c..55ceb67 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1171,6 +1171,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 +1185,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);