Re: [Qemu-devel] [PATCH v3] Add param Error ** for msi_init()

[Marcel Apfelbaum <marcel@redhat.com>]

On 03/28/2016 01:44 PM, Cao jin wrote:
> Add param Error **errp, and change pci_add_capability() to
> pci_add_capability2(), because pci_add_capability() report error, and
> msi_init() is widely used in realize(), so it is not suitable for realize().
>

Hi,
The patch is looking good in my opinion, a few comments below:

> Also fix all the callers who should deal with the msi_init() failure but
> actually not. The affected devices are as following:
>
>    1. intel hd audio: move msi_init() above, save the strength to free
> the MemoryRegion when it fails.
>    2. usb-xhci: move msi_init() above, save the strength to free the
> MemoryRegion when it fails.
>    3. ich9 ahci: it is a on-board device created during machine
> initialization, when it fail, qemu will exit, so, no need to free
> resource manually.
>    4. megasas_scsi: move msi_init() above, save the strength to free
> the MemoryRegion when it fails . Also fix a bug: when user enable
> msi, and msi_init success(return positive offset value), the code
> disable msi in the flags. But it seems this bug doesn`t do anything
> bad.
>    5. mptsas: when msi_init() fail, it will use INTx. So, catch the
> error & report it right there, don`t propagate it. Move msi_init()
> above,  save the strength to free the MemoryRegion when it fails.
>    6. pci_bridge_dev/ioh3420/xio3130_downstream/xio3130_upstream: catch
> error & report it right there.
>    7. vmxnet3: move msi_init() above, save the strength to free the
> MemoryRegion when it fails. Remove the unecessary vmxnet3_init_msi().
> When msi_init() fail, it will use INTx, so msi_init()`s failure should not
> break the realize. Just catch & report msi_init()`s failure message.
>    8. pvscsi: when msi_init fail, it will use INTx. so msi_init failure
> should not break the realize. Report the error when msi_init fail.
> Nobody use the return value of pvscsi_init_msi(), so change its type
> to void.
>    9. vfio-pci: it ignores the config space corruption error, so,
> catch & report it right there.

I suggest to move 1-9 points to after --- so it will not be part of the commit
message, only reference for the reviewers.

>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
> The patch has been compiled. No further test.
>
>   hw/audio/intel-hda.c               | 11 +++++++---
>   hw/ide/ich.c                       |  2 +-
>   hw/net/vmxnet3.c                   | 41 ++++++++++++++------------------------
>   hw/pci-bridge/ioh3420.c            |  6 +++++-
>   hw/pci-bridge/pci_bridge_dev.c     |  8 +++++++-
>   hw/pci-bridge/xio3130_downstream.c |  7 ++++++-
>   hw/pci-bridge/xio3130_upstream.c   |  7 ++++++-
>   hw/pci/msi.c                       | 23 +++++++++++++++++++--
>   hw/scsi/megasas.c                  | 12 +++++++----
>   hw/scsi/mptsas.c                   | 17 +++++++++++-----
>   hw/scsi/vmw_pvscsi.c               | 10 ++++++----
>   hw/usb/hcd-xhci.c                  | 10 +++++++---
>   hw/vfio/pci.c                      |  6 ++++--
>   include/hw/pci/msi.h               |  3 ++-
>   14 files changed, 108 insertions(+), 55 deletions(-)
>
> diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
> index d372d4a..c3856cc 100644
> --- a/hw/audio/intel-hda.c
> +++ b/hw/audio/intel-hda.c
> @@ -1139,12 +1139,17 @@ 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) {
> +        msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, 1,
> +                true, false, errp);
> +        if (*errp) {
> +            return;
> +        }
> +    }
> +
>       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) {
> -        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..db4fdb5 100644
> --- a/hw/ide/ich.c
> +++ b/hw/ide/ich.c
> @@ -146,7 +146,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 093a71e..f3614f2 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -348,7 +348,7 @@ typedef struct {
>   /* Interrupt management */
>
>   /*
> - *This function returns sign whether interrupt line is in asserted state
> + * This function returns sign whether interrupt line is in asserted state
>    * This depends on the type of interrupt used. For INTX interrupt line will
>    * be asserted until explicit deassertion, for MSI(X) interrupt line will
>    * be deasserted automatically due to notification semantics of the MSI(X)
> @@ -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,6 +2250,10 @@ 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);
> @@ -2278,6 +2261,16 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
>
>       VMW_CBPRN("Starting init...");
>
> +    msi_init(pci_dev, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS,
> +             VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK, errp);
> +    if (*errp) {
> +        error_report_err(*errp);
> +        *errp = NULL;

You suppress prev debug message: VMW_WRPRN("Failed to initialize MSI, error %d", res),
but you replace with one of yourself. If the vmxnet maintainers have nothing against,
I suppose is OK.

Then, you don't propagate the error because you don't want realize
to fail, so you report it and NULL it. I suppose we should have
a "warning only" error type so the reporting party can decide to issue a warning
or to fail, but is out of the scope of this patch, of course.

 > -        s->msi_used = false;

> +        s->msi_used = false;
> +    } else {
> +        s->msi_used = true;
> +    }
> +
>       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,
> @@ -2302,10 +2295,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.");

Here again you remove an existent debug warning, maybe you should keep it.

> -    }
> -
>       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 0937fa3..159a10c 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);
> @@ -106,12 +107,15 @@ 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, &err);
>       if (rc < 0) {
> +        error_report_err(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 862a236..07c7bf8 100644
> --- a/hw/pci-bridge/pci_bridge_dev.c
> +++ b/hw/pci-bridge/pci_bridge_dev.c
> @@ -52,6 +52,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;

You use both local_err and err for local error names. It doesn't really matter
the name, but please be consistent. I personally like local_error but is up to you, of course.

>
>       pci_bridge_initfn(dev, TYPE_PCI_BUS);
>
> @@ -67,17 +68,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;
>       }
> +

You have several new lines (before and after this comment) that have nothing
to do with the patch. I suggest putting them into a trivial separate patch.

>       if ((bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_MSI_REQ)) &&
>           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;
>           }
>       }
> +
>       if (shpc_present(dev)) {
>           /* TODO: spec recommends using 64 bit prefetcheable BAR.
>            * Check whether that works well. */
> @@ -85,6 +90,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>                            PCI_BASE_ADDRESS_MEM_TYPE_64, &bridge_dev->bar);
>       }
>       return 0;
> +
>   msi_error:
>       slotid_cap_cleanup(dev);
>   slotid_error:
> diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
> index cf1ee63..7428923 100644
> --- a/hw/pci-bridge/xio3130_downstream.c
> +++ b/hw/pci-bridge/xio3130_downstream.c
> @@ -60,26 +60,31 @@ 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;
>       }
> +
>       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) {
>           goto err_msi;
>       }
> +
>       pcie_cap_flr_init(d);
>       pcie_cap_deverr_init(d);
>       pcie_cap_slot_init(d, s->slot);
> diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c
> index 164ef58..a7f7987 100644
> --- a/hw/pci-bridge/xio3130_upstream.c
> +++ b/hw/pci-bridge/xio3130_upstream.c
> @@ -56,26 +56,31 @@ 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;
>       }
> +
>       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, PCI_ERR_SIZEOF);
> diff --git a/hw/pci/msi.c b/hw/pci/msi.c
> index e0e64c2..b1c8f8f 100644
> --- a/hw/pci/msi.c
> +++ b/hw/pci/msi.c
> @@ -165,15 +165,32 @@ bool msi_enabled(const PCIDevice *dev)
>            PCI_MSI_FLAGS_ENABLE);
>   }
>
> +/*
> + * Make PCI device @dev MSI-capable.
> + * Non-zero @offset puts capability MSI at that offset in PCI config
> + * space.
> + * @nr_vectors is the number of MSI vectors (1, 2, 4, 8, 16 or 32).
> + * If @msi64bit, make the device capable of sending a 64-bit message
> + * address.
> + * If @msi_per_vector_mask, make the device support per-vector masking.
> + * @errp is for returning errors.
> + * Return the offset of capability MSI in config space on success,
> + * set @errp and return -errno on error.
> + * -ENOTSUP means lacking msi support for a msi-capable platform.
> + */
>   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;
>       uint8_t cap_size;
>       int config_offset;
> +    Error *err = NULL;
>
>       if (!msi_nonbroken) {
> +        error_setg(&err, "MSI is not supported by interrupt controller");
> +        error_propagate(errp, err);

I suppose error_setg(errp... is enough and you don't have to use also error_propagate.

>           return -ENOTSUP;
>       }
>
> @@ -197,8 +214,10 @@ 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, &err);
>       if (config_offset < 0) {
> +        error_propagate(errp, err);
>           return config_offset;
>       }
>
> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index a63a581..0aaf3af 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -2340,6 +2340,14 @@ 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, errp);
> +        if (*errp) {
> +            s->flags &= ~MEGASAS_MASK_USE_MSI;
> +            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,
> @@ -2347,10 +2355,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)) {

This was a bug. msi_init returns non-zero value on both failure and success.
Your code fixes the bug by checking the errp. I think you should
fix the bug in a separate patch and only then apply this patch.

> -        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/mptsas.c b/hw/scsi/mptsas.c
> index 499c146..27350b7 100644
> --- a/hw/scsi/mptsas.c
> +++ b/hw/scsi/mptsas.c
> @@ -1274,10 +1274,22 @@ static void mptsas_scsi_init(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_available) {
> +        if (msi_init(dev, 0, 1, true, false, &err) >= 0) {
> +            s->msi_in_use = true;
> +        }
> +        else {
> +            error_append_hint(&err, "MSI init fail, will use INTx instead");

The hint should end with a new line: "\n".

> +            error_report_err(err);

You should not report the error if the fucntion has an **errp parameter.

> +        }
> +    }
> +
> +
>       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 +1297,6 @@ static void mptsas_scsi_init(PCIDevice *dev, Error **errp)
>       memory_region_init_io(&s->diag_io, OBJECT(s), &mptsas_diag_ops, s,
>                             "mptsas-diag", 0x10000);
>
> -    if (s->msi_available &&
> -        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 9abc086..88fb611 100644
> --- a/hw/scsi/vmw_pvscsi.c
> +++ b/hw/scsi/vmw_pvscsi.c
> @@ -1039,22 +1039,24 @@ pvscsi_io_read(void *opaque, hwaddr addr, unsigned size)
>   }
>
>
> -static bool
> +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");

The same for this hint, should end with "\n"

> +        error_report_err(err);
>           s->msi_used = false;
>       } else {
>           s->msi_used = true;
>       }
> -
> -    return s->msi_used;

Refactoring the function to return void instead of bool
is out of the scope of this patch. You can take this out to a simple
patch.

>   }
>
>   static void
> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> index bcde8a2..f132a57 100644
> --- a/hw/usb/hcd-xhci.c
> +++ b/hw/usb/hcd-xhci.c
> @@ -3588,6 +3588,13 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
>
>       usb_xhci_init(xhci);
>
> +    if (xhci_get_flag(xhci, XHCI_FLAG_USE_MSI)) {
> +        ret = msi_init(dev, 0x70, xhci->numintrs, true, false, errp);
> +        if (ret < 0) {
> +            return;
> +        }
> +    }
> +
>       if (xhci->numintrs > MAXINTRS) {
>           xhci->numintrs = MAXINTRS;
>       }
> @@ -3645,9 +3652,6 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
>           assert(ret >= 0);
>       }
>
> -    if (xhci_get_flag(xhci, XHCI_FLAG_USE_MSI)) {
> -        msi_init(dev, 0x70, xhci->numintrs, true, false);
> -    }
>       if (xhci_get_flag(xhci, XHCI_FLAG_USE_MSI_X)) {
>           msix_init(dev, xhci->numintrs,
>                     &xhci->mem, 0, OFF_MSIX_TABLE,
> 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);
>

Thanks,
Marcel