From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37600) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a6IEg-0001Cb-5Z for qemu-devel@nongnu.org; Tue, 08 Dec 2015 08:23:20 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a6IEe-0004D0-00 for qemu-devel@nongnu.org; Tue, 08 Dec 2015 08:23:18 -0500 References: <1449475725-16453-1-git-send-email-caoj.fnst@cn.fujitsu.com> <1449475725-16453-2-git-send-email-caoj.fnst@cn.fujitsu.com> <87a8pmziil.fsf@blackfin.pond.sub.org> From: Cao jin Message-ID: <5666D9EF.1090803@cn.fujitsu.com> Date: Tue, 8 Dec 2015 21:23:59 +0800 MIME-Version: 1.0 In-Reply-To: <87a8pmziil.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit 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: Markus Armbruster Cc: jiri@resnulli.us, qemu-block@nongnu.org, 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, kraxel@redhat.com 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 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. > 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