From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42790) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a5sa1-0003ED-5z for qemu-devel@nongnu.org; Mon, 07 Dec 2015 04:59:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a5sZx-0003Az-V2 for qemu-devel@nongnu.org; Mon, 07 Dec 2015 04:59:37 -0500 Received: from mx1.redhat.com ([209.132.183.28]:51123) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a5sZx-0003Aq-LF for qemu-devel@nongnu.org; Mon, 07 Dec 2015 04:59:33 -0500 From: Markus Armbruster References: <1449475725-16453-1-git-send-email-caoj.fnst@cn.fujitsu.com> <1449475725-16453-2-git-send-email-caoj.fnst@cn.fujitsu.com> Date: Mon, 07 Dec 2015 10:59:30 +0100 In-Reply-To: <1449475725-16453-2-git-send-email-caoj.fnst@cn.fujitsu.com> (Cao jin's message of "Mon, 7 Dec 2015 16:08:44 +0800") Message-ID: <87a8pmziil.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH 1/2] Add param Error** to msi_init() & modify the callers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cao jin Cc: jiri@resnulli.us, mst@redhat.com, jasowang@redhat.com, qemu-devel@nongnu.org, alex.williamson@redhat.com, sfeldma@gmail.com, hare@suse.de, dmitry@daynix.com, pbonzini@redhat.com, jsnow@redhat.com, emu-block@nongnu.org, kraxel@redhat.com Cao jin 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 > --- > 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.