* [Qemu-devel] [PATCH 0/2] Preparation for PCI devices convert to realize()
@ 2015-12-07 8:08 Cao jin
2015-12-07 8:08 ` [Qemu-devel] [PATCH 1/2] Add param Error** to msi_init() & modify the callers Cao jin
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Cao jin @ 2015-12-07 8:08 UTC (permalink / raw)
To: qemu-devel
Cc: jiri, mst, jasowang, alex.williamson, sfeldma, hare, dmitry,
pbonzini, jsnow, emu-block, kraxel
There are many PCI devices still using .init() as its initialization function,
I am planning to do the "convert to realize()" work, and PCI bridge devices are
chosen first. The supporting functions should be modified first.msi_init() &
msix_init() are supporting functions for PCI devices.
Because this patchset is much different from the previous one, so, didn`t
add "V2" in the subject
Cao jin (2):
Add param Error** to msi_init() & modify the callers
Add param Error** to msix_init() & modify the callers
hw/audio/intel-hda.c | 7 ++++++-
hw/ide/ich.c | 2 +-
hw/net/rocker/rocker.c | 3 ++-
hw/net/vmxnet3.c | 7 +++++--
hw/pci-bridge/ioh3420.c | 6 +++++-
hw/pci-bridge/pci_bridge_dev.c | 6 +++++-
hw/pci-bridge/xio3130_downstream.c | 7 ++++++-
hw/pci-bridge/xio3130_upstream.c | 7 ++++++-
hw/pci/msi.c | 17 +++++++++++++----
hw/pci/msix.c | 13 ++++++++++---
hw/scsi/megasas.c | 28 +++++++++++++++++++++-------
hw/scsi/vmw_pvscsi.c | 3 ++-
hw/usb/hcd-xhci.c | 7 +++++--
hw/vfio/pci.c | 7 +++++--
include/hw/pci/msi.h | 4 ++--
include/hw/pci/msix.h | 3 ++-
16 files changed, 96 insertions(+), 31 deletions(-)
--
2.1.0
^ permalink raw reply [flat|nested] 9+ messages in thread* [Qemu-devel] [PATCH 1/2] Add param Error** to msi_init() & modify the callers 2015-12-07 8:08 [Qemu-devel] [PATCH 0/2] Preparation for PCI devices convert to realize() Cao jin @ 2015-12-07 8:08 ` Cao jin 2015-12-07 9:59 ` Markus Armbruster 2015-12-07 8:08 ` [Qemu-devel] [PATCH 2/2] Add param Error** to msix_init() " Cao jin 2015-12-07 13:42 ` [Qemu-devel] [PATCH 0/2] Preparation for PCI devices convert to realize() Marcel Apfelbaum 2 siblings, 1 reply; 9+ messages in thread From: Cao jin @ 2015-12-07 8:08 UTC (permalink / raw) To: qemu-devel Cc: jiri, mst, jasowang, alex.williamson, sfeldma, hare, dmitry, pbonzini, jsnow, emu-block, kraxel msi_init() is a supporting function in PCI device initialization, in order to convert .init() to .realize(), it should be modified first. Also modify the callers Bonus: add more comment for msi_init(). Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> --- hw/audio/intel-hda.c | 7 ++++++- hw/ide/ich.c | 2 +- hw/net/vmxnet3.c | 3 ++- hw/pci-bridge/ioh3420.c | 6 +++++- hw/pci-bridge/pci_bridge_dev.c | 6 +++++- hw/pci-bridge/xio3130_downstream.c | 7 ++++++- hw/pci-bridge/xio3130_upstream.c | 7 ++++++- hw/pci/msi.c | 17 +++++++++++++---- hw/scsi/megasas.c | 12 +++++++++--- hw/scsi/vmw_pvscsi.c | 3 ++- hw/usb/hcd-xhci.c | 5 ++++- hw/vfio/pci.c | 3 ++- include/hw/pci/msi.h | 4 ++-- 13 files changed, 63 insertions(+), 19 deletions(-) diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c index 433463e..9d733da 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,7 +1143,11 @@ 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) { + return; + } } hda_codec_bus_init(DEVICE(pci), &d->codecs, sizeof(d->codecs), 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 5e3a233..4269141 100644 --- a/hw/net/vmxnet3.c +++ b/hw/net/vmxnet3.c @@ -2140,9 +2140,10 @@ vmxnet3_init_msi(VMXNET3State *s) { PCIDevice *d = PCI_DEVICE(s); int res; + Error *local_err = NULL; 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, &local_err); if (0 > res) { VMW_WRPRN("Failed to initialize MSI, error %d", res); s->msi_used = false; diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c index eead195..05b004a 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,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, + &local_err); if (rc < 0) { 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..13e8583 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,13 +67,15 @@ 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) { goto msi_error; } @@ -84,6 +87,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..8e91034 100644 --- a/hw/pci-bridge/xio3130_downstream.c +++ b/hw/pci-bridge/xio3130_downstream.c @@ -59,21 +59,25 @@ 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) { 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 +107,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..bae49f6 100644 --- a/hw/pci-bridge/xio3130_upstream.c +++ b/hw/pci-bridge/xio3130_upstream.c @@ -55,26 +55,31 @@ 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) { 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..961f114 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 + * + * return: MSI capability offset in config space + */ +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; } @@ -182,7 +190,7 @@ int msi_init(struct PCIDevice *dev, uint8_t offset, } cap_size = msi_cap_sizeof(flags); - config_offset = pci_add_capability(dev, PCI_CAP_ID_MSI, offset, cap_size); + config_offset = pci_add_capability2(dev, PCI_CAP_ID_MSI, offset, cap_size, errp); if (config_offset < 0) { return config_offset; } @@ -205,6 +213,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..4759fb5 100644 --- a/hw/scsi/megasas.c +++ b/hw/scsi/megasas.c @@ -2346,9 +2346,15 @@ 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_msi(s)) { + int ret; + + ret = msi_init(dev, 0x50, 1, true, false, errp); + if(ret > 0) { + s->flags &= ~MEGASAS_MASK_USE_MSI; + } else { + return; + } } if (megasas_use_msix(s) && msix_init(dev, 15, &s->mmio_io, b->mmio_bar, 0x2000, diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c index 9c71f31..b2ff293 100644 --- a/hw/scsi/vmw_pvscsi.c +++ b/hw/scsi/vmw_pvscsi.c @@ -1018,9 +1018,10 @@ pvscsi_init_msi(PVSCSIState *s) { int res; PCIDevice *d = PCI_DEVICE(s); + Error *local_err = NULL; 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, &local_err); if (res < 0) { trace_pvscsi_init_msi_fail(res); s->msi_used = false; diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c index 268ab36..b452c52 100644 --- a/hw/usb/hcd-xhci.c +++ b/hw/usb/hcd-xhci.c @@ -3643,7 +3643,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) { + return; + } } if (xhci_get_flag(xhci, XHCI_FLAG_USE_MSI_X)) { msix_init(dev, xhci->numintrs, diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index 1fb868c..8559950 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -1144,6 +1144,7 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos) uint16_t ctrl; bool msi_64bit, msi_maskbit; int ret, entries; + Error *local_err = NULL; if (pread(vdev->vbasedev.fd, &ctrl, sizeof(ctrl), vdev->config_offset + pos + PCI_CAP_FLAGS) != sizeof(ctrl)) { @@ -1157,7 +1158,7 @@ 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, &local_err); if (ret < 0) { if (ret == -ENOTSUP) { return 0; 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); -- 2.1.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] Add param Error** to msi_init() & modify the callers 2015-12-07 8:08 ` [Qemu-devel] [PATCH 1/2] Add param Error** to msi_init() & modify the callers Cao jin @ 2015-12-07 9:59 ` Markus Armbruster 2015-12-08 13:23 ` Cao jin 0 siblings, 1 reply; 9+ messages in thread From: Markus Armbruster @ 2015-12-07 9:59 UTC (permalink / raw) To: Cao jin Cc: jiri, mst, jasowang, qemu-devel, alex.williamson, sfeldma, hare, dmitry, pbonzini, jsnow, emu-block, kraxel Cao jin <caoj.fnst@cn.fujitsu.com> writes: > msi_init() is a supporting function in PCI device initialization, in order to > convert .init() to .realize(), it should be modified first. Also modify the > callers > > Bonus: add more comment for msi_init(). Incomplete. See notes on impact inline. > Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> > --- > hw/audio/intel-hda.c | 7 ++++++- > hw/ide/ich.c | 2 +- > hw/net/vmxnet3.c | 3 ++- > hw/pci-bridge/ioh3420.c | 6 +++++- > hw/pci-bridge/pci_bridge_dev.c | 6 +++++- > hw/pci-bridge/xio3130_downstream.c | 7 ++++++- > hw/pci-bridge/xio3130_upstream.c | 7 ++++++- > hw/pci/msi.c | 17 +++++++++++++---- > hw/scsi/megasas.c | 12 +++++++++--- > hw/scsi/vmw_pvscsi.c | 3 ++- > hw/usb/hcd-xhci.c | 5 ++++- > hw/vfio/pci.c | 3 ++- > include/hw/pci/msi.h | 4 ++-- > 13 files changed, 63 insertions(+), 19 deletions(-) > > diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c > index 433463e..9d733da 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,7 +1143,11 @@ 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) { Please use scripts/checkpatch.pl to check your patches. It's occasionally wrong, so use your judgement. > + return; This returns with the device in a half-realized state. Do we have to undo prior side effects to put it back into unrealized state? See also ioh3420_initfn() below. Before: msi_init() failure is ignored. After: it makes device realization fail. To assess impact, we need to understand how msi_init() can fail. > + } > } > > hda_codec_bus_init(DEVICE(pci), &d->codecs, sizeof(d->codecs), > 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); Do we have to put the device back into unrealized state on failure? > } > > static void pci_ich9_uninit(PCIDevice *dev) > diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c > index 5e3a233..4269141 100644 > --- a/hw/net/vmxnet3.c > +++ b/hw/net/vmxnet3.c > @@ -2140,9 +2140,10 @@ vmxnet3_init_msi(VMXNET3State *s) > { > PCIDevice *d = PCI_DEVICE(s); > int res; > + Error *local_err = NULL; > > 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, &local_err); > if (0 > res) { > VMW_WRPRN("Failed to initialize MSI, error %d", res); > s->msi_used = false; The error is neither propagated nor handled, and the error object leaks. Since this function can't handle it, it needs to propagate it. Requires adding an Error ** parameter. > diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c > index eead195..05b004a 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,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, > + &local_err); > if (rc < 0) { > goto err_bridge; The error is neither propagated nor handled, and the error object leaks. Since this is an init method, it should report errors with error_report and return -1. The latter is there, but the former is missing. You need to error_report_err(local_err). > } > + > rc = pcie_cap_init(d, IOH_EP_EXP_OFFSET, PCI_EXP_TYPE_ROOT_PORT, p->port); Aside, not for this patch: this function (and many more) will have to be converted to Error as well before we can convert to realize. > if (rc < 0) { > goto err_msi; Further down: err: pcie_chassis_del_slot(s); err_pcie_cap: pcie_cap_exit(d); err_msi: msi_uninit(d); err_bridge: pci_bridge_exitfn(d); return rc; } Note that we carefully undo side effects on failure. > diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c > index bc3e1b7..13e8583 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,13 +67,15 @@ 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) { > goto msi_error; The error is neither propagated nor handled, and the error object leaks. Again, you need error_report_err(local_err). > } > @@ -84,6 +87,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..8e91034 100644 > --- a/hw/pci-bridge/xio3130_downstream.c > +++ b/hw/pci-bridge/xio3130_downstream.c > @@ -59,21 +59,25 @@ 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) { > goto err_bridge; Likewise. > } > + > 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 +107,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..bae49f6 100644 > --- a/hw/pci-bridge/xio3130_upstream.c > +++ b/hw/pci-bridge/xio3130_upstream.c > @@ -55,26 +55,31 @@ 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) { > goto err_bridge; Likewise. > } > + > 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..961f114 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 > + * > + * return: MSI capability offset in config space > + */ > +int msi_init(struct PCIDevice *dev, uint8_t offset, unsigned int nr_vectors, > + bool msi64bit, bool msi_per_vector_mask, Error **errp) Let's see how this function can fail. > { > 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; Attempt to use MSI when !msi_supported. Before: silently return -ENOTSUP. After: additionally pass a suitable error. > } > > @@ -182,7 +190,7 @@ int msi_init(struct PCIDevice *dev, uint8_t offset, > } > > cap_size = msi_cap_sizeof(flags); > - config_offset = pci_add_capability(dev, PCI_CAP_ID_MSI, offset, cap_size); > + config_offset = pci_add_capability2(dev, PCI_CAP_ID_MSI, offset, cap_size, errp); > if (config_offset < 0) { > return config_offset; Can't set PCI_CAP_ID_MSI. Before: report with error_report_err() and return -errno. After: pass a suitable error and return -errno. > } > @@ -205,6 +213,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; > } To assess the patch's impact, you need to compare before / after for both failure modes and each caller. I suspect the result will promote your patch from "prepare for realize" to "fix to catch and report errors". > > diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c > index d7dc667..4759fb5 100644 > --- a/hw/scsi/megasas.c > +++ b/hw/scsi/megasas.c > @@ -2346,9 +2346,15 @@ 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_msi(s)) { > + int ret; > + > + ret = msi_init(dev, 0x50, 1, true, false, errp); > + if(ret > 0) { > + s->flags &= ~MEGASAS_MASK_USE_MSI; > + } else { > + return; Do we have to undo side effects? > + } > } > if (megasas_use_msix(s) && > msix_init(dev, 15, &s->mmio_io, b->mmio_bar, 0x2000, > diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c > index 9c71f31..b2ff293 100644 > --- a/hw/scsi/vmw_pvscsi.c > +++ b/hw/scsi/vmw_pvscsi.c > @@ -1018,9 +1018,10 @@ pvscsi_init_msi(PVSCSIState *s) > { > int res; > PCIDevice *d = PCI_DEVICE(s); > + Error *local_err = NULL; > > 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, &local_err); > if (res < 0) { > trace_pvscsi_init_msi_fail(res); > s->msi_used = false; Once again, error_report_err(local_err) needed. > diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c > index 268ab36..b452c52 100644 > --- a/hw/usb/hcd-xhci.c > +++ b/hw/usb/hcd-xhci.c > @@ -3643,7 +3643,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) { > + return; Do we have to undo side effects? > + } > } > if (xhci_get_flag(xhci, XHCI_FLAG_USE_MSI_X)) { > msix_init(dev, xhci->numintrs, > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index 1fb868c..8559950 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -1144,6 +1144,7 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos) > uint16_t ctrl; > bool msi_64bit, msi_maskbit; > int ret, entries; > + Error *local_err = NULL; > > if (pread(vdev->vbasedev.fd, &ctrl, sizeof(ctrl), > vdev->config_offset + pos + PCI_CAP_FLAGS) != sizeof(ctrl)) { > @@ -1157,7 +1158,7 @@ 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, &local_err); > if (ret < 0) { > if (ret == -ENOTSUP) { > return 0; error_report_err(local_err) now, but for conversion to realize with require conversion to Error. > 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); I guess your PATCH 2 has similar issues; I'll skip reviewing it. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] Add param Error** to msi_init() & modify the callers 2015-12-07 9:59 ` Markus Armbruster @ 2015-12-08 13:23 ` Cao jin 2015-12-08 15:01 ` Markus Armbruster 0 siblings, 1 reply; 9+ messages in thread From: Cao jin @ 2015-12-08 13:23 UTC (permalink / raw) To: Markus Armbruster Cc: jiri, qemu-block, mst, jasowang, qemu-devel, alex.williamson, sfeldma, hare, dmitry, pbonzini, jsnow, kraxel Hi Markus, I have to say, you really did a amazing review for this "trivial "patch, thanks a lot & really appreciate it:) On 12/07/2015 05:59 PM, Markus Armbruster wrote: > Cao jin <caoj.fnst@cn.fujitsu.com> writes: > >> msi_init() is a supporting function in PCI device initialization, in order to >> convert .init() to .realize(), it should be modified first. Also modify the >> callers >> >> Bonus: add more comment for msi_init(). > > Incomplete. See notes on impact inline. > >> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> >> --- >> hw/audio/intel-hda.c | 7 ++++++- >> hw/ide/ich.c | 2 +- >> hw/net/vmxnet3.c | 3 ++- >> hw/pci-bridge/ioh3420.c | 6 +++++- >> hw/pci-bridge/pci_bridge_dev.c | 6 +++++- >> hw/pci-bridge/xio3130_downstream.c | 7 ++++++- >> hw/pci-bridge/xio3130_upstream.c | 7 ++++++- >> hw/pci/msi.c | 17 +++++++++++++---- >> hw/scsi/megasas.c | 12 +++++++++--- >> hw/scsi/vmw_pvscsi.c | 3 ++- >> hw/usb/hcd-xhci.c | 5 ++++- >> hw/vfio/pci.c | 3 ++- >> include/hw/pci/msi.h | 4 ++-- >> 13 files changed, 63 insertions(+), 19 deletions(-) >> >> diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c >> index 433463e..9d733da 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,7 +1143,11 @@ 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) { > > Please use scripts/checkpatch.pl to check your patches. It's > occasionally wrong, so use your judgement. > Thanks for the tips, seems I got dizzy looking because many trivial place need to be modified... >> + return; > > This returns with the device in a half-realized state. Do we have to > undo prior side effects to put it back into unrealized state? See also > ioh3420_initfn() below. > > Before: msi_init() failure is ignored. After: it makes device > realization fail. To assess impact, we need to understand how > msi_init() can fail. > It seems I missed the reality: devices are default to be hot-pluggable & most devices are hot-pluggable:-[ Because when cold plugged, process will exit on device-init failing, so, the half-realized state doesn`t matter in this condition. Will rework it later. >> + } >> } >> >> hda_codec_bus_init(DEVICE(pci), &d->codecs, sizeof(d->codecs), >> 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); > > Do we have to put the device back into unrealized state on failure? > >> } >> >> static void pci_ich9_uninit(PCIDevice *dev) >> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c >> index 5e3a233..4269141 100644 >> --- a/hw/net/vmxnet3.c >> +++ b/hw/net/vmxnet3.c >> @@ -2140,9 +2140,10 @@ vmxnet3_init_msi(VMXNET3State *s) >> { >> PCIDevice *d = PCI_DEVICE(s); >> int res; >> + Error *local_err = NULL; >> >> 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, &local_err); >> if (0 > res) { >> VMW_WRPRN("Failed to initialize MSI, error %d", res); >> s->msi_used = false; > > The error is neither propagated nor handled, and the error object leaks. > > Since this function can't handle it, it needs to propagate it. Requires > adding an Error ** parameter. > [*]Actually, here is my consideration: a device-realize function(take the following ioh3420 for example) will call many supporting functions like msi_init(), so I am planning, every supporting function goes into a patch first, then every "device convert to realize()" goes into a patch, otherwise, it may will be a big patch for the reviewer. That`s why I didn`t add Error ** param, and propagate it, and plan to do it in "convert to realize()" patch. But for now, I think this patch should at least be successfully compiled & won`t impact the existed things. Yes, it seems may have leaks when error happens, but will be fixed when the "convert to realize()" patch comes out. I am not sure whether the plan is ok, So, How do you think? >> diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c >> index eead195..05b004a 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,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, >> + &local_err); >> if (rc < 0) { >> goto err_bridge; > > The error is neither propagated nor handled, and the error object leaks. > > Since this is an init method, it should report errors with error_report > and return -1. The latter is there, but the former is missing. You > need to error_report_err(local_err). > Refer previous comment[*]: Was planning propagate it in "convert to realize()" patch later. >> } >> + >> rc = pcie_cap_init(d, IOH_EP_EXP_OFFSET, PCI_EXP_TYPE_ROOT_PORT, p->port); > > Aside, not for this patch: this function (and many more) will have to be > converted to Error as well before we can convert to realize. > Yes, I am planning every supporting function goes into a patch. Because msi & msix are too similar, so I put them together and send out first... >> if (rc < 0) { >> goto err_msi; > > Further down: > > err: > pcie_chassis_del_slot(s); > err_pcie_cap: > pcie_cap_exit(d); > err_msi: > msi_uninit(d); > err_bridge: > pci_bridge_exitfn(d); > return rc; > } > > Note that we carefully undo side effects on failure. > >> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c >> index bc3e1b7..13e8583 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,13 +67,15 @@ 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) { >> goto msi_error; > > The error is neither propagated nor handled, and the error object leaks. > > Again, you need error_report_err(local_err). > Refer the previous comment[*]: was planning propagate it in "convert to realize()" patch later. >> } >> @@ -84,6 +87,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..8e91034 100644 >> --- a/hw/pci-bridge/xio3130_downstream.c >> +++ b/hw/pci-bridge/xio3130_downstream.c >> @@ -59,21 +59,25 @@ 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) { >> goto err_bridge; > > Likewise. > >> } >> + >> 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 +107,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..bae49f6 100644 >> --- a/hw/pci-bridge/xio3130_upstream.c >> +++ b/hw/pci-bridge/xio3130_upstream.c >> @@ -55,26 +55,31 @@ 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) { >> goto err_bridge; > > Likewise. > >> } >> + >> 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..961f114 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 >> + * >> + * return: MSI capability offset in config space >> + */ >> +int msi_init(struct PCIDevice *dev, uint8_t offset, unsigned int nr_vectors, >> + bool msi64bit, bool msi_per_vector_mask, Error **errp) > > Let's see how this function can fail. > >> { >> 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; > > Attempt to use MSI when !msi_supported. > > Before: silently return -ENOTSUP. > > After: additionally pass a suitable error. > >> } >> >> @@ -182,7 +190,7 @@ int msi_init(struct PCIDevice *dev, uint8_t offset, >> } >> >> cap_size = msi_cap_sizeof(flags); >> - config_offset = pci_add_capability(dev, PCI_CAP_ID_MSI, offset, cap_size); >> + config_offset = pci_add_capability2(dev, PCI_CAP_ID_MSI, offset, cap_size, errp); >> if (config_offset < 0) { >> return config_offset; > > Can't set PCI_CAP_ID_MSI. > > Before: report with error_report_err() and return -errno. > > After: pass a suitable error and return -errno. > >> } >> @@ -205,6 +213,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; >> } > > To assess the patch's impact, you need to compare before / after for > both failure modes and each caller. I suspect the result will promote > your patch from "prepare for realize" to "fix to catch and report > errors". > Thanks a lot for the direction:) but I still have a question: if I start off by per-device, then will modify every supporting function, and common supporting function will impact other device, so will need to convert other device together, and this will result in a huge patch contains converting of many devices and supporting functions, what do you think of it? >> >> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c >> index d7dc667..4759fb5 100644 >> --- a/hw/scsi/megasas.c >> +++ b/hw/scsi/megasas.c >> @@ -2346,9 +2346,15 @@ 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_msi(s)) { >> + int ret; >> + >> + ret = msi_init(dev, 0x50, 1, true, false, errp); >> + if(ret > 0) { >> + s->flags &= ~MEGASAS_MASK_USE_MSI; >> + } else { >> + return; > > Do we have to undo side effects? will assess it. > >> + } >> } >> if (megasas_use_msix(s) && >> msix_init(dev, 15, &s->mmio_io, b->mmio_bar, 0x2000, >> diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c >> index 9c71f31..b2ff293 100644 >> --- a/hw/scsi/vmw_pvscsi.c >> +++ b/hw/scsi/vmw_pvscsi.c >> @@ -1018,9 +1018,10 @@ pvscsi_init_msi(PVSCSIState *s) >> { >> int res; >> PCIDevice *d = PCI_DEVICE(s); >> + Error *local_err = NULL; >> >> 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, &local_err); >> if (res < 0) { >> trace_pvscsi_init_msi_fail(res); >> s->msi_used = false; > > Once again, error_report_err(local_err) needed. > >> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c >> index 268ab36..b452c52 100644 >> --- a/hw/usb/hcd-xhci.c >> +++ b/hw/usb/hcd-xhci.c >> @@ -3643,7 +3643,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) { >> + return; > > Do we have to undo side effects? > will assess it. >> + } >> } >> if (xhci_get_flag(xhci, XHCI_FLAG_USE_MSI_X)) { >> msix_init(dev, xhci->numintrs, >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >> index 1fb868c..8559950 100644 >> --- a/hw/vfio/pci.c >> +++ b/hw/vfio/pci.c >> @@ -1144,6 +1144,7 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos) >> uint16_t ctrl; >> bool msi_64bit, msi_maskbit; >> int ret, entries; >> + Error *local_err = NULL; >> >> if (pread(vdev->vbasedev.fd, &ctrl, sizeof(ctrl), >> vdev->config_offset + pos + PCI_CAP_FLAGS) != sizeof(ctrl)) { >> @@ -1157,7 +1158,7 @@ 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, &local_err); >> if (ret < 0) { >> if (ret == -ENOTSUP) { >> return 0; > > error_report_err(local_err) now, but for conversion to realize with > require conversion to Error. > Refer previous comment[*]. Was plannning to add Error ** param to every function in the call chain when convert the vfio device to realize(). >> 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); > > I guess your PATCH 2 has similar issues; I'll skip reviewing it. > > > . > -- Yours Sincerely, Cao Jin ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] Add param Error** to msi_init() & modify the callers 2015-12-08 13:23 ` Cao jin @ 2015-12-08 15:01 ` Markus Armbruster 2015-12-09 8:54 ` Cao jin 0 siblings, 1 reply; 9+ messages in thread From: Markus Armbruster @ 2015-12-08 15:01 UTC (permalink / raw) To: Cao jin Cc: jiri, qemu-block, mst, jasowang, qemu-devel, alex.williamson, sfeldma, hare, dmitry, pbonzini, jsnow, kraxel Cao jin <caoj.fnst@cn.fujitsu.com> writes: > Hi Markus, > I have to say, you really did a amazing review for this "trivial > "patch, thanks a lot & really appreciate it:) Thanks! I'm afraid the problem you picked isn't trivial, but I hope it's still simple enough to be a useful exercise to get you going with the code. > On 12/07/2015 05:59 PM, Markus Armbruster wrote: >> Cao jin <caoj.fnst@cn.fujitsu.com> writes: >> >>> msi_init() is a supporting function in PCI device initialization, in order to >>> convert .init() to .realize(), it should be modified first. Also modify the >>> callers >>> >>> Bonus: add more comment for msi_init(). >> >> Incomplete. See notes on impact inline. >> >>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> >>> --- >>> hw/audio/intel-hda.c | 7 ++++++- >>> hw/ide/ich.c | 2 +- >>> hw/net/vmxnet3.c | 3 ++- >>> hw/pci-bridge/ioh3420.c | 6 +++++- >>> hw/pci-bridge/pci_bridge_dev.c | 6 +++++- >>> hw/pci-bridge/xio3130_downstream.c | 7 ++++++- >>> hw/pci-bridge/xio3130_upstream.c | 7 ++++++- >>> hw/pci/msi.c | 17 +++++++++++++---- >>> hw/scsi/megasas.c | 12 +++++++++--- >>> hw/scsi/vmw_pvscsi.c | 3 ++- >>> hw/usb/hcd-xhci.c | 5 ++++- >>> hw/vfio/pci.c | 3 ++- >>> include/hw/pci/msi.h | 4 ++-- >>> 13 files changed, 63 insertions(+), 19 deletions(-) >>> >>> diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c >>> index 433463e..9d733da 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,7 +1143,11 @@ 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) { >> >> Please use scripts/checkpatch.pl to check your patches. It's >> occasionally wrong, so use your judgement. >> > > Thanks for the tips, seems I got dizzy looking because many trivial > place need to be modified... > >>> + return; >> >> This returns with the device in a half-realized state. Do we have to >> undo prior side effects to put it back into unrealized state? See also >> ioh3420_initfn() below. >> >> Before: msi_init() failure is ignored. After: it makes device >> realization fail. To assess impact, we need to understand how >> msi_init() can fail. >> > > It seems I missed the reality: devices are default to be hot-pluggable > & most devices are hot-pluggable:-[ Because when cold plugged, process > will exit on device-init failing, so, the half-realized state doesn`t > matter in this condition. > Will rework it later. In theory, realize() should always fail cleanly. In practice, unclean realize() failure doesn't matter when it's fatal anyway. Some devices are only used where it's always fatal. See also "Error handling in realize() methods" I just sent to the list; I hope we can come up with some guidance on when shortcuts in realize() methods are tolerable. >>> + } >>> } >>> >>> hda_codec_bus_init(DEVICE(pci), &d->codecs, sizeof(d->codecs), >>> 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); >> >> Do we have to put the device back into unrealized state on failure? >> >>> } >>> >>> static void pci_ich9_uninit(PCIDevice *dev) >>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c >>> index 5e3a233..4269141 100644 >>> --- a/hw/net/vmxnet3.c >>> +++ b/hw/net/vmxnet3.c >>> @@ -2140,9 +2140,10 @@ vmxnet3_init_msi(VMXNET3State *s) >>> { >>> PCIDevice *d = PCI_DEVICE(s); >>> int res; >>> + Error *local_err = NULL; >>> >>> 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, &local_err); >>> if (0 > res) { >>> VMW_WRPRN("Failed to initialize MSI, error %d", res); >>> s->msi_used = false; >> >> The error is neither propagated nor handled, and the error object leaks. >> >> Since this function can't handle it, it needs to propagate it. Requires >> adding an Error ** parameter. >> > > [*]Actually, here is my consideration: a device-realize function(take > the following ioh3420 for example) will call many supporting functions > like msi_init(), so I am planning, every supporting function goes into > a patch first, then every "device convert to realize()" goes into a > patch, otherwise, it may will be a big patch for the reviewer. That`s > why I didn`t add Error ** param, and propagate it, and plan to do it > in "convert to realize()" patch. But for now, I think this patch > should at least be successfully compiled & won`t impact the existed > things. > > Yes, it seems may have leaks when error happens, but will be fixed > when the "convert to realize()" patch comes out. > > I am not sure whether the plan is ok, So, How do you think? If you don't want to propagate the error further in this patch, you need to free it: if (0 > res) { VMW_WRPRN("Failed to initialize MSI, error %d", res); error_free(local_err); s->msi_used = false; While there, you could improve the error message by printing error_get_pretty(local_err)) instead of res, but I wouldn't bother, since we'll have to replace it with error_propagate() anyway. >>> diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c >>> index eead195..05b004a 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,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, >>> + &local_err); >>> if (rc < 0) { >>> goto err_bridge; >> >> The error is neither propagated nor handled, and the error object leaks. >> >> Since this is an init method, it should report errors with error_report >> and return -1. The latter is there, but the former is missing. You >> need to error_report_err(local_err). >> > > Refer previous comment[*]: Was planning propagate it in "convert to > realize()" patch later. Again, if you don't want to propagate the error further in this patch, you need to free it. Actually, you have to report and free it; see msi_init() below for why. The obvious way to do that: if (rc < 0) { error_report_err(local_err); goto err_bridge; } By the way, I use err instead of local_err. Matter of taste. >>> } >>> + >>> rc = pcie_cap_init(d, IOH_EP_EXP_OFFSET, >>> PCI_EXP_TYPE_ROOT_PORT, p->port); >> >> Aside, not for this patch: this function (and many more) will have to be >> converted to Error as well before we can convert to realize. >> > > Yes, I am planning every supporting function goes into a > patch. Because msi & msix are too similar, so I put them together and > send out first... > >>> if (rc < 0) { >>> goto err_msi; >> >> Further down: >> >> err: >> pcie_chassis_del_slot(s); >> err_pcie_cap: >> pcie_cap_exit(d); >> err_msi: >> msi_uninit(d); >> err_bridge: >> pci_bridge_exitfn(d); >> return rc; >> } >> >> Note that we carefully undo side effects on failure. >> >>> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c >>> index bc3e1b7..13e8583 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,13 +67,15 @@ 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) { >>> goto msi_error; >> >> The error is neither propagated nor handled, and the error object leaks. >> >> Again, you need error_report_err(local_err). >> > > Refer the previous comment[*]: was planning propagate it in "convert > to realize()" patch later. > >>> } >>> @@ -84,6 +87,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..8e91034 100644 >>> --- a/hw/pci-bridge/xio3130_downstream.c >>> +++ b/hw/pci-bridge/xio3130_downstream.c >>> @@ -59,21 +59,25 @@ 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) { >>> goto err_bridge; >> >> Likewise. >> >>> } >>> + >>> 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 +107,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..bae49f6 100644 >>> --- a/hw/pci-bridge/xio3130_upstream.c >>> +++ b/hw/pci-bridge/xio3130_upstream.c >>> @@ -55,26 +55,31 @@ 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) { >>> goto err_bridge; >> >> Likewise. >> >>> } >>> + >>> 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..961f114 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 >>> + * >>> + * return: MSI capability offset in config space >>> + */ >>> +int msi_init(struct PCIDevice *dev, uint8_t offset, unsigned int nr_vectors, >>> + bool msi64bit, bool msi_per_vector_mask, Error **errp) >> >> Let's see how this function can fail. >> >>> { >>> 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; >> >> Attempt to use MSI when !msi_supported. >> >> Before: silently return -ENOTSUP. >> >> After: additionally pass a suitable error. >> >>> } >>> >>> @@ -182,7 +190,7 @@ int msi_init(struct PCIDevice *dev, uint8_t offset, >>> } >>> >>> cap_size = msi_cap_sizeof(flags); >>> - config_offset = pci_add_capability(dev, PCI_CAP_ID_MSI, offset, cap_size); >>> + config_offset = pci_add_capability2(dev, PCI_CAP_ID_MSI, offset, cap_size, errp); >>> if (config_offset < 0) { >>> return config_offset; >> >> Can't set PCI_CAP_ID_MSI. >> >> Before: report with error_report_err() and return -errno. >> >> After: pass a suitable error and return -errno. >> >>> } >>> @@ -205,6 +213,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; >>> } >> >> To assess the patch's impact, you need to compare before / after for >> both failure modes and each caller. I suspect the result will promote >> your patch from "prepare for realize" to "fix to catch and report >> errors". Let's do that for ioh3420_initfn(). Before: * when !msi_supported, ioh3420_initfn() fails silently * when pci_add_capability() fails, we report with error_report_err(), and ioh3420_initfn() fails After (your patch): * when !msi_supported, ioh3420_initfn() fails silently * when pci_add_capability2() fails, ioh3420_initfn() fails silently, regression After (with ioh3420_initfn() calling error_report_err(), as per my review): * when !msi_supported, ioh3420_initfn() reports with error_report_err() and fails, bug fix (mention in commit message) * when pci_add_capability2() fails, reports with error_report_err() and fails Do that for every caller of msi_init(), then summarize the patch's impact change in the commit message. > Thanks a lot for the direction:) but I still have a question: if I > start off by per-device, then will modify every supporting function, > and common supporting function will impact other device, so will need > to convert other device together, and this will result in a huge patch > contains converting of many devices and supporting functions, what do > you think of it? A huge patch would be much harder to review. Limiting this patch to just msi_init() is sensible. But the patch mustn't break things. Not even if you plan to fix the breakage in later patches. [...] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] Add param Error** to msi_init() & modify the callers 2015-12-08 15:01 ` Markus Armbruster @ 2015-12-09 8:54 ` Cao jin 0 siblings, 0 replies; 9+ messages in thread From: Cao jin @ 2015-12-09 8:54 UTC (permalink / raw) To: Markus Armbruster Cc: jiri, qemu-block, mst, jasowang, qemu-devel, alex.williamson, sfeldma, hare, dmitry, pbonzini, jsnow, kraxel On 12/08/2015 11:01 PM, Markus Armbruster wrote: > Cao jin <caoj.fnst@cn.fujitsu.com> writes: > >> Hi Markus, >> I have to say, you really did a amazing review for this "trivial >> "patch, thanks a lot & really appreciate it:) > > Thanks! I'm afraid the problem you picked isn't trivial, but I hope > it's still simple enough to be a useful exercise to get you going with > the code. > [...] > > In theory, realize() should always fail cleanly. In practice, unclean > realize() failure doesn't matter when it's fatal anyway. Some devices > are only used where it's always fatal. See also "Error handling in > realize() methods" I just sent to the list; I hope we can come up with > some guidance on when shortcuts in realize() methods are tolerable. > Read it, really great background introduction to me:) [...] >> >> [*]Actually, here is my consideration: a device-realize function(take >> the following ioh3420 for example) will call many supporting functions >> like msi_init(), so I am planning, every supporting function goes into >> a patch first, then every "device convert to realize()" goes into a >> patch, otherwise, it may will be a big patch for the reviewer. That`s >> why I didn`t add Error ** param, and propagate it, and plan to do it >> in "convert to realize()" patch. But for now, I think this patch >> should at least be successfully compiled & won`t impact the existed >> things. >> >> Yes, it seems may have leaks when error happens, but will be fixed >> when the "convert to realize()" patch comes out. >> >> I am not sure whether the plan is ok, So, How do you think? > > If you don't want to propagate the error further in this patch, you need > to free it: > > if (0 > res) { > VMW_WRPRN("Failed to initialize MSI, error %d", res); > error_free(local_err); > s->msi_used = false; > > While there, you could improve the error message by printing > error_get_pretty(local_err)) instead of res, but I wouldn't bother, > since we'll have to replace it with error_propagate() anyway. > Agree. will fix it later. [...] >> >> Refer previous comment[*]: Was planning propagate it in "convert to >> realize()" patch later. > > Again, if you don't want to propagate the error further in this patch, > you need to free it. Actually, you have to report and free it; see > msi_init() below for why. The obvious way to do that: > > if (rc < 0) { > error_report_err(local_err); > goto err_bridge; > } > > By the way, I use err instead of local_err. Matter of taste. > Agree. will fix it later. [...] >>> >>> To assess the patch's impact, you need to compare before / after for >>> both failure modes and each caller. I suspect the result will promote >>> your patch from "prepare for realize" to "fix to catch and report >>> errors". > > Let's do that for ioh3420_initfn(). > > Before: > * when !msi_supported, ioh3420_initfn() fails silently > * when pci_add_capability() fails, we report with error_report_err(), > and ioh3420_initfn() fails > > After (your patch): > * when !msi_supported, ioh3420_initfn() fails silently > * when pci_add_capability2() fails, ioh3420_initfn() fails silently, > regression > > After (with ioh3420_initfn() calling error_report_err(), as per my > review): > * when !msi_supported, ioh3420_initfn() reports with error_report_err() > and fails, bug fix (mention in commit message) > * when pci_add_capability2() fails, reports with error_report_err() and > fails > > Do that for every caller of msi_init(), then summarize the patch's > impact change in the commit message. > great example for me, I Will try to do it. >> Thanks a lot for the direction:) but I still have a question: if I >> start off by per-device, then will modify every supporting function, >> and common supporting function will impact other device, so will need >> to convert other device together, and this will result in a huge patch >> contains converting of many devices and supporting functions, what do >> you think of it? > > A huge patch would be much harder to review. Limiting this patch to > just msi_init() is sensible. But the patch mustn't break things. Not > even if you plan to fix the breakage in later patches. > Agree. will try to undo the prior side effects and handle the error(like via error_report_err()) temporary[1] in next version. [1]all error will be propagated ultimately, for hot-pluggable device. > [...] > > > . > -- Yours Sincerely, Cao Jin ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH 2/2] Add param Error** to msix_init() & modify the callers 2015-12-07 8:08 [Qemu-devel] [PATCH 0/2] Preparation for PCI devices convert to realize() Cao jin 2015-12-07 8:08 ` [Qemu-devel] [PATCH 1/2] Add param Error** to msi_init() & modify the callers Cao jin @ 2015-12-07 8:08 ` Cao jin 2015-12-07 13:42 ` [Qemu-devel] [PATCH 0/2] Preparation for PCI devices convert to realize() Marcel Apfelbaum 2 siblings, 0 replies; 9+ messages in thread From: Cao jin @ 2015-12-07 8:08 UTC (permalink / raw) To: qemu-devel Cc: jiri, mst, jasowang, alex.williamson, sfeldma, hare, dmitry, pbonzini, jsnow, emu-block, kraxel msix_init() is a supporting function in PCI device initialization, in order to convert .init() to .realize(), it should be modified first. Also modify the callers. Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> --- hw/net/rocker/rocker.c | 3 ++- hw/net/vmxnet3.c | 4 +++- hw/pci/msix.c | 13 ++++++++++--- hw/scsi/megasas.c | 16 ++++++++++++---- hw/usb/hcd-xhci.c | 2 +- hw/vfio/pci.c | 4 +++- include/hw/pci/msix.h | 3 ++- 7 files changed, 33 insertions(+), 12 deletions(-) diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c index c57f1a6..9faa117 100644 --- a/hw/net/rocker/rocker.c +++ b/hw/net/rocker/rocker.c @@ -1248,13 +1248,14 @@ static int rocker_msix_init(Rocker *r) { PCIDevice *dev = PCI_DEVICE(r); int err; + Error *local_err = NULL; err = msix_init(dev, ROCKER_MSIX_VEC_COUNT(r->fp_ports), &r->msix_bar, ROCKER_PCI_MSIX_BAR_IDX, ROCKER_PCI_MSIX_TABLE_OFFSET, &r->msix_bar, ROCKER_PCI_MSIX_BAR_IDX, ROCKER_PCI_MSIX_PBA_OFFSET, - 0); + 0, &local_err); if (err) { return err; } diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c index 4269141..fad783e 100644 --- a/hw/net/vmxnet3.c +++ b/hw/net/vmxnet3.c @@ -2098,12 +2098,14 @@ static bool vmxnet3_init_msix(VMXNET3State *s) { PCIDevice *d = PCI_DEVICE(s); + Error *local_err = NULL; + int res = msix_init(d, VMXNET3_MAX_INTRS, &s->msix_bar, VMXNET3_MSIX_BAR_IDX, VMXNET3_OFF_MSIX_TABLE, &s->msix_bar, VMXNET3_MSIX_BAR_IDX, VMXNET3_OFF_MSIX_PBA, - 0); + 0, &local_err); if (0 > res) { VMW_WRPRN("Failed to initialize MSI-X, error %d", res); diff --git a/hw/pci/msix.c b/hw/pci/msix.c index 64c93d8..28c14d1 100644 --- a/hw/pci/msix.c +++ b/hw/pci/msix.c @@ -233,7 +233,8 @@ static void msix_mask_all(struct PCIDevice *dev, unsigned nentries) int msix_init(struct PCIDevice *dev, unsigned short nentries, MemoryRegion *table_bar, uint8_t table_bar_nr, unsigned table_offset, MemoryRegion *pba_bar, - uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos) + uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos, + Error **errp) { int cap; unsigned table_size, pba_size; @@ -241,10 +242,13 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries, /* Nothing to do if MSI is not supported by interrupt controller */ if (!msi_supported) { + error_setg(errp, "MSI-X is not supported by interrupt controller"); return -ENOTSUP; } if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1) { + error_setg(errp, "Vector number %d invalid, should be 1 ~ %d", + nentries, PCI_MSIX_FLAGS_QSIZE + 1); return -EINVAL; } @@ -257,10 +261,12 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries, table_offset + table_size > memory_region_size(table_bar) || pba_offset + pba_size > memory_region_size(pba_bar) || (table_offset | pba_offset) & PCI_MSIX_FLAGS_BIRMASK) { + error_setg(errp, "Arguments invalid. please check them carefully"); return -EINVAL; } - cap = pci_add_capability(dev, PCI_CAP_ID_MSIX, cap_pos, MSIX_CAP_LENGTH); + cap = pci_add_capability2(dev, PCI_CAP_ID_MSIX, cap_pos, + MSIX_CAP_LENGTH, errp); if (cap < 0) { return cap; } @@ -304,6 +310,7 @@ int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries, uint32_t bar_size = 4096; uint32_t bar_pba_offset = bar_size / 2; uint32_t bar_pba_size = (nentries / 8 + 1) * 8; + Error *local_err = NULL; /* * Migration compatibility dictates that this remains a 4k @@ -329,7 +336,7 @@ int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries, ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr, 0, &dev->msix_exclusive_bar, bar_nr, bar_pba_offset, - 0); + 0, &local_err); if (ret) { return ret; } diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c index 4759fb5..ef52a85 100644 --- a/hw/scsi/megasas.c +++ b/hw/scsi/megasas.c @@ -2356,11 +2356,19 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp) return; } } - if (megasas_use_msix(s) && - msix_init(dev, 15, &s->mmio_io, b->mmio_bar, 0x2000, - &s->mmio_io, b->mmio_bar, 0x3800, 0x68)) { - s->flags &= ~MEGASAS_MASK_USE_MSIX; + + if (megasas_use_msix(s)) { + int ret; + + ret = msix_init(dev, 15, &s->mmio_io, b->mmio_bar, 0x2000, + &s->mmio_io, b->mmio_bar, 0x3800, 0x68, errp); + if (ret > 0) { + s->flags &= ~MEGASAS_MASK_USE_MSIX; + } else { + return; + } } + if (pci_is_express(dev)) { pcie_endpoint_cap_init(dev, 0xa0); } diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c index b452c52..26e81d5 100644 --- a/hw/usb/hcd-xhci.c +++ b/hw/usb/hcd-xhci.c @@ -3652,7 +3652,7 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp) msix_init(dev, xhci->numintrs, &xhci->mem, 0, OFF_MSIX_TABLE, &xhci->mem, 0, OFF_MSIX_PBA, - 0x90); + 0x90, errp); } } diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index 8559950..6b2bf5c 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -1251,12 +1251,14 @@ static int vfio_msix_early_setup(VFIOPCIDevice *vdev) static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos) { int ret; + Error *local_err = NULL; ret = msix_init(&vdev->pdev, vdev->msix->entries, &vdev->bars[vdev->msix->table_bar].region.mem, vdev->msix->table_bar, vdev->msix->table_offset, &vdev->bars[vdev->msix->pba_bar].region.mem, - vdev->msix->pba_bar, vdev->msix->pba_offset, pos); + vdev->msix->pba_bar, vdev->msix->pba_offset, pos, + &local_err); if (ret < 0) { if (ret == -ENOTSUP) { return 0; diff --git a/include/hw/pci/msix.h b/include/hw/pci/msix.h index 72e5f93..6535a6c 100644 --- a/include/hw/pci/msix.h +++ b/include/hw/pci/msix.h @@ -9,7 +9,8 @@ MSIMessage msix_get_message(PCIDevice *dev, unsigned int vector); int msix_init(PCIDevice *dev, unsigned short nentries, MemoryRegion *table_bar, uint8_t table_bar_nr, unsigned table_offset, MemoryRegion *pba_bar, - uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos); + uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos, + Error **errp); int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries, uint8_t bar_nr); -- 2.1.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] Preparation for PCI devices convert to realize() 2015-12-07 8:08 [Qemu-devel] [PATCH 0/2] Preparation for PCI devices convert to realize() Cao jin 2015-12-07 8:08 ` [Qemu-devel] [PATCH 1/2] Add param Error** to msi_init() & modify the callers Cao jin 2015-12-07 8:08 ` [Qemu-devel] [PATCH 2/2] Add param Error** to msix_init() " Cao jin @ 2015-12-07 13:42 ` Marcel Apfelbaum 2015-12-08 1:27 ` Cao jin 2 siblings, 1 reply; 9+ messages in thread From: Marcel Apfelbaum @ 2015-12-07 13:42 UTC (permalink / raw) To: Cao jin, qemu-devel Cc: jiri, mst, jasowang, alex.williamson, sfeldma, hare, dmitry, pbonzini, jsnow, emu-block, kraxel On 12/07/2015 10:08 AM, Cao jin wrote: > There are many PCI devices still using .init() as its initialization function, > I am planning to do the "convert to realize()" work, and PCI bridge devices are > chosen first. The supporting functions should be modified first.msi_init() & > msix_init() are supporting functions for PCI devices. > > Because this patchset is much different from the previous one, so, didn`t > add "V2" in the subject Hi, Even if the patches are different is worth mentioning V2, otherwise the maintainer would not know which to take. Thanks, Marcel > > Cao jin (2): > Add param Error** to msi_init() & modify the callers > Add param Error** to msix_init() & modify the callers > > hw/audio/intel-hda.c | 7 ++++++- > hw/ide/ich.c | 2 +- > hw/net/rocker/rocker.c | 3 ++- > hw/net/vmxnet3.c | 7 +++++-- > hw/pci-bridge/ioh3420.c | 6 +++++- > hw/pci-bridge/pci_bridge_dev.c | 6 +++++- > hw/pci-bridge/xio3130_downstream.c | 7 ++++++- > hw/pci-bridge/xio3130_upstream.c | 7 ++++++- > hw/pci/msi.c | 17 +++++++++++++---- > hw/pci/msix.c | 13 ++++++++++--- > hw/scsi/megasas.c | 28 +++++++++++++++++++++------- > hw/scsi/vmw_pvscsi.c | 3 ++- > hw/usb/hcd-xhci.c | 7 +++++-- > hw/vfio/pci.c | 7 +++++-- > include/hw/pci/msi.h | 4 ++-- > include/hw/pci/msix.h | 3 ++- > 16 files changed, 96 insertions(+), 31 deletions(-) > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] Preparation for PCI devices convert to realize() 2015-12-07 13:42 ` [Qemu-devel] [PATCH 0/2] Preparation for PCI devices convert to realize() Marcel Apfelbaum @ 2015-12-08 1:27 ` Cao jin 0 siblings, 0 replies; 9+ messages in thread From: Cao jin @ 2015-12-08 1:27 UTC (permalink / raw) To: marcel, qemu-devel Cc: jiri, qemu-block, mst, jasowang, alex.williamson, sfeldma, hare, dmitry, pbonzini, jsnow, kraxel Hi, Marcel On 12/07/2015 09:42 PM, Marcel Apfelbaum wrote: > On 12/07/2015 10:08 AM, Cao jin wrote: >> There are many PCI devices still using .init() as its initialization >> function, >> I am planning to do the "convert to realize()" work, and PCI bridge >> devices are >> chosen first. The supporting functions should be modified >> first.msi_init() & >> msix_init() are supporting functions for PCI devices. >> >> Because this patchset is much different from the previous one, so, didn`t >> add "V2" in the subject > > Hi, > > Even if the patches are different is worth mentioning V2, otherwise > the maintainer would not know which to take. > I see. Thanks for your suggestion:) > Thanks, > Marcel > -- Yours Sincerely, Cao Jin ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-12-09 9:09 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-12-07 8:08 [Qemu-devel] [PATCH 0/2] Preparation for PCI devices convert to realize() Cao jin 2015-12-07 8:08 ` [Qemu-devel] [PATCH 1/2] Add param Error** to msi_init() & modify the callers Cao jin 2015-12-07 9:59 ` Markus Armbruster 2015-12-08 13:23 ` Cao jin 2015-12-08 15:01 ` Markus Armbruster 2015-12-09 8:54 ` Cao jin 2015-12-07 8:08 ` [Qemu-devel] [PATCH 2/2] Add param Error** to msix_init() " Cao jin 2015-12-07 13:42 ` [Qemu-devel] [PATCH 0/2] Preparation for PCI devices convert to realize() Marcel Apfelbaum 2015-12-08 1:27 ` Cao jin
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.