* [Qemu-devel] [PATCH v3] Add param Error ** for msi_init()
@ 2016-03-28 10:44 Cao jin
2016-03-29 20:56 ` Marcel Apfelbaum
0 siblings, 1 reply; 5+ messages in thread
From: Cao jin @ 2016-03-28 10:44 UTC (permalink / raw)
To: qemu-devel
Cc: mst, jasowang, armbru, marcel, alex.williamson, hare, dmitry,
pbonzini, jsnow, kraxel
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().
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.
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;
+ 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.");
- }
-
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;
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;
}
+
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);
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)) {
- 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");
+ error_report_err(err);
+ }
+ }
+
+
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");
+ error_report_err(err);
s->msi_used = false;
} else {
s->msi_used = true;
}
-
- return s->msi_used;
}
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);
--
2.1.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v3] Add param Error ** for msi_init()
2016-03-28 10:44 [Qemu-devel] [PATCH v3] Add param Error ** for msi_init() Cao jin
@ 2016-03-29 20:56 ` Marcel Apfelbaum
2016-03-30 4:10 ` Cao jin
0 siblings, 1 reply; 5+ messages in thread
From: Marcel Apfelbaum @ 2016-03-29 20:56 UTC (permalink / raw)
To: Cao jin, qemu-devel
Cc: mst, jasowang, armbru, alex.williamson, hare, dmitry, pbonzini,
jsnow, kraxel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v3] Add param Error ** for msi_init()
2016-03-29 20:56 ` Marcel Apfelbaum
@ 2016-03-30 4:10 ` Cao jin
2016-03-30 9:00 ` Marcel Apfelbaum
0 siblings, 1 reply; 5+ messages in thread
From: Cao jin @ 2016-03-30 4:10 UTC (permalink / raw)
To: Marcel Apfelbaum, qemu-devel
Cc: mst, jasowang, armbru, alex.williamson, hare, dmitry, pbonzini,
jsnow, kraxel
Hi Marcel,
Thanks for your quick review for this big fat patch:)
please see my comments inline.
On 03/30/2016 04:56 AM, Marcel Apfelbaum wrote:
> On 03/28/2016 01:44 PM, Cao jin wrote:
>>
>> -#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.
>
Yes, I should add more hint message. I don`t quite understand about:
/have a "warning only" error type so the reporting party can decide to
issue a warning or to fail/
Do you mean still using VMW_WRPRN or using error_append_hint? It seems
VMW_WRPRN should only work in debug time? and if user should see this
error message, should use error_report_err ?
>> - 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);
>>
>> 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.
>
Yes, I prefer consistent way, but here, you see there is a "int err", so
I thought two different variants using same name maybe not so good for
reading code? what would you choose?(I like local_err at first because
it is easy to understand for newbie, but when I get familiar with error
handling, I turn to like err, just because it is shorter:p)
>>
>> 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.
>
see what I was told before:
http://lists.nongnu.org/archive/html/qemu-trivial/2015-10/msg00116.html
http://lists.nongnu.org/archive/html/qemu-trivial/2015-10/msg00123.html
Actually I am ok with both sides. I just stop sending coding style patch
since then:)
I don`t know, coding style & indentation patch really matters or is just
a personal taste thing?
>> +++ 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.
>
it will use INTx even if msi_init fail, so it should not break realize.
But I think user should know it if something wrong happened there，so I
use a local *err* to report error message, while don`t touch **errp. Is
there any better way?
>
For all the other comments, will fix them in next version.
--
Yours Sincerely,
Cao jin
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v3] Add param Error ** for msi_init()
2016-03-30 4:10 ` Cao jin
@ 2016-03-30 9:00 ` Marcel Apfelbaum
2016-03-31 8:09 ` Cao jin
0 siblings, 1 reply; 5+ messages in thread
From: Marcel Apfelbaum @ 2016-03-30 9:00 UTC (permalink / raw)
To: Cao jin, qemu-devel
Cc: mst, jasowang, Markus Armbruster, alex.williamson, hare, dmitry,
pbonzini, jsnow, kraxel
On 03/30/2016 07:10 AM, Cao jin wrote:
> Hi Marcel,
> Thanks for your quick review for this big fat patch:)
> please see my comments inline.
>
> On 03/30/2016 04:56 AM, Marcel Apfelbaum wrote:
>> On 03/28/2016 01:44 PM, Cao jin wrote:
>
>>>
>>> -#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.
>>
Hi,
>
> Yes, I should add more hint message. I don`t quite understand about:
>
> /have a "warning only" error type so the reporting party can decide to issue a warning or to fail/
>
> Do you mean still using VMW_WRPRN or using error_append_hint? It seems VMW_WRPRN should only work in debug time? and if user should see this error message, should use error_report_err ?
No, it is not related to VMW_WRPRN. On this subject, those are debugging warnings
and we should keep them the same as before.
About the "warning only" error type: if an error is returned, the caller
assumes that the initialization failed and it will return with failure. That means
that you cannot return a non-null error if you want the process to finish OK.
This is why you had to:
- report the error (even if this function should not be a reporter because it receives an Error parameter)
- empty the error: so why use error at all, right?
My point is if the caller had a way to check what kind of error this is
and act accordingly, it would be nicer. But again, this is out of the scope of this patch, only some thoughts.
>
>>> - 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);
>>>
>
>>> 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.
>>
>
> Yes, I prefer consistent way, but here, you see there is a "int err", so I thought two different variants using same name maybe not so good for reading code? what would you choose?(I like local_err
> at first because it is easy to understand for newbie, but when I get familiar with error handling, I turn to like err, just because it is shorter:p)
Indeed is a little confusing, this is why I prefer "local_error" because it is used widely
and became a familiar pattern.
>
>>>
>>> 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.
>>
>
> see what I was told before:
> http://lists.nongnu.org/archive/html/qemu-trivial/2015-10/msg00116.html
> http://lists.nongnu.org/archive/html/qemu-trivial/2015-10/msg00123.html
>
> Actually I am ok with both sides. I just stop sending coding style patch since then:)
Oh, I hate when it happens to me, tho different opinions, how can you deal with that, right ? :)
>
> I don`t know, coding style & indentation patch really matters or is just a personal taste thing?
Coding style and indentation *are important* IMHO.
For this case, what I would do is put the new lines and comments in a separate patch,\
but send it with *the same* series, not through trivial list, saying something like:
"fixed some code style problems while resolving the ... problem".
>
>>> +++ 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.
>>
>
> it will use INTx even if msi_init fail, so it should not break realize. But I think user should know it if something wrong happened there，so I use a local *err* to report error message, while don`t
> touch **errp. Is there any better way?
Same problem as discussed before, Markus do you have any idea how to solve this elegantly?
CC: Markus Armbruster <armbru@redhat.com>
Thanks,
Marcel
>
>>
>
> For all the other comments, will fix them in next version.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v3] Add param Error ** for msi_init()
2016-03-30 9:00 ` Marcel Apfelbaum
@ 2016-03-31 8:09 ` Cao jin
0 siblings, 0 replies; 5+ messages in thread
From: Cao jin @ 2016-03-31 8:09 UTC (permalink / raw)
To: Marcel Apfelbaum, qemu-devel
Cc: mst, jasowang, Markus Armbruster, alex.williamson, hare, dmitry,
pbonzini, jsnow, kraxel
On 03/30/2016 05:00 PM, Marcel Apfelbaum wrote:
> On 03/30/2016 07:10 AM, Cao jin wrote:
>
> Hi,
>
>>
>> Yes, I should add more hint message. I don`t quite understand about:
>>
>> /have a "warning only" error type so the reporting party can decide to
>> issue a warning or to fail/
>>
>> Do you mean still using VMW_WRPRN or using error_append_hint? It seems
>> VMW_WRPRN should only work in debug time? and if user should see this
>> error message, should use error_report_err ?
>
> No, it is not related to VMW_WRPRN. On this subject, those are debugging
> warnings
> and we should keep them the same as before.
>
ok
> About the "warning only" error type: if an error is returned, the caller
> assumes that the initialization failed and it will return with failure.
> That means
> that you cannot return a non-null error if you want the process to
> finish OK.
>
> This is why you had to:
> - report the error (even if this function should not be a reporter
> because it receives an Error parameter)
> - empty the error: so why use error at all, right?
>
> My point is if the caller had a way to check what kind of error this is
> and act accordingly, it would be nicer. But again, this is out of the
> scope of this patch, only some thoughts.
>
I see, and agree.
>>
>>>
>>
>> see what I was told before:
>> http://lists.nongnu.org/archive/html/qemu-trivial/2015-10/msg00116.html
>> http://lists.nongnu.org/archive/html/qemu-trivial/2015-10/msg00123.html
>>
>> Actually I am ok with both sides. I just stop sending coding style
>> patch since then:)
>
> Oh, I hate when it happens to me, tho different opinions, how can you
> deal with that, right ? :)
>
>>
>> I don`t know, coding style & indentation patch really matters or is
>> just a personal taste thing?
>
> Coding style and indentation *are important* IMHO.
>
Totally, absolutely agree
> For this case, what I would do is put the new lines and comments in a
> separate patch,\
> but send it with *the same* series, not through trivial list, saying
> something like:
> "fixed some code style problems while resolving the ... problem".
>
OK
--
Yours Sincerely,
Cao jin
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-03-31 8:07 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-03-28 10:44 [Qemu-devel] [PATCH v3] Add param Error ** for msi_init() Cao jin
2016-03-29 20:56 ` Marcel Apfelbaum
2016-03-30 4:10 ` Cao jin
2016-03-30 9:00 ` Marcel Apfelbaum
2016-03-31 8:09 ` Cao jin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).