[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/2] Add param Error** to msi_init() & modify th
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 1/2] Add param Error** to msi_init() & modify the callers |
Date: |
Mon, 07 Dec 2015 10:59:30 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Cao jin <address@hidden> writes:
> msi_init() is a supporting function in PCI device initialization, in order to
> convert .init() to .realize(), it should be modified first. Also modify the
> callers
>
> Bonus: add more comment for msi_init().
Incomplete. See notes on impact inline.
> Signed-off-by: Cao jin <address@hidden>
> ---
> 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.
Re: [Qemu-devel] [PATCH 0/2] Preparation for PCI devices convert to realize(), Marcel Apfelbaum, 2015/12/07