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