From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41946) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1al0gT-0002jM-Bv for qemu-devel@nongnu.org; Tue, 29 Mar 2016 16:56:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1al0gP-000067-Pj for qemu-devel@nongnu.org; Tue, 29 Mar 2016 16:56:17 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46282) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1al0gP-00005w-Fk for qemu-devel@nongnu.org; Tue, 29 Mar 2016 16:56:13 -0400 References: <1459161888-32566-1-git-send-email-caoj.fnst@cn.fujitsu.com> From: Marcel Apfelbaum Message-ID: <56FAEBE6.2000609@redhat.com> Date: Tue, 29 Mar 2016 23:56:06 +0300 MIME-Version: 1.0 In-Reply-To: <1459161888-32566-1-git-send-email-caoj.fnst@cn.fujitsu.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3] Add param Error ** for msi_init() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cao jin , qemu-devel@nongnu.org Cc: mst@redhat.com, jasowang@redhat.com, armbru@redhat.com, alex.williamson@redhat.com, hare@suse.de, dmitry@daynix.com, pbonzini@redhat.com, jsnow@redhat.com, kraxel@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 > --- > 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