Cao jin <caoj.f...@cn.fujitsu.com> writes:
> Add param Error **errp, and change pci_add_capability() to
> pci_add_capability2(), because pci_add_capability() report error, and
> msi_init() is widely used in realize(), so it is not suitable for realize()
Suggest:
pci: Convert msi_init() to Error and fix callers to check it
msi_init() reports errors with error_report(), which is wrong when
it's used in realize().
Fix by converting it to Error.
But see the discussion of the msi_init() failure modes below; commit
message may need further work for that.
Same issue in msix_init(). Please fix that as well, if it's not too
much trouble.
> Also fix all the callers who should deal with the msi_init() failure
> but actually not.
Grammar: "but actually don't" (you need a verb).
You neglect to explain the bug's impact. Something like
Fix its callers to handle failure instead of ignoring it.
[Description on what goes wrong because of that goes here]
>
> Signed-off-by: Cao jin <caoj.f...@cn.fujitsu.com>
> ---
> hw/audio/intel-hda.c | 11 +++++++---
> hw/ide/ich.c | 2 +-
> hw/net/vmxnet3.c | 41
> +++++++++++++++-----------------------
> hw/pci-bridge/ioh3420.c | 4 +++-
> hw/pci-bridge/pci_bridge_dev.c | 4 +++-
> hw/pci-bridge/xio3130_downstream.c | 4 +++-
> hw/pci-bridge/xio3130_upstream.c | 4 +++-
> hw/pci/msi.c | 9 +++++++--
> hw/scsi/megasas.c | 12 +++++++----
> hw/scsi/mptsas.c | 15 +++++++++-----
> hw/scsi/vmw_pvscsi.c | 6 +++++-
> hw/usb/hcd-xhci.c | 10 +++++++---
> hw/vfio/pci.c | 6 ++++--
> include/hw/pci/msi.h | 3 ++-
> 14 files changed, 80 insertions(+), 51 deletions(-)
>
> diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
> index d372d4a..c3856cc 100644
> --- a/hw/audio/intel-hda.c
> +++ b/hw/audio/intel-hda.c
> @@ -1139,12 +1139,17 @@ static void intel_hda_realize(PCIDevice *pci, Error
> **errp)
> /* HDCTL off 0x40 bit 0 selects signaling mode (1-HDA, 0 - Ac97) 18.1.19
> */
> conf[0x40] = 0x01;
>
> + if (d->msi) {
> + msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, 1,
> + true, false, errp);
> + if (*errp) {
Crash bug if errp is null. I guess it's never null here right now, but
let's not rely on that for robustness, and to avoid setting a bad
example. Bad examples multiply like rabbits.
The big comment in error.h explains how to receive an error correctly:
* Receive an error and pass it on to the caller:
* Error *err = NULL;
* foo(arg, &err);
* if (err) {
* handle the error...
* error_propagate(errp, err);
* }
* where Error **errp is a parameter, by convention the last one.
This is what you should do here.
*
* Do *not* "optimize" this to
* foo(arg, errp);
* if (*errp) { // WRONG!
* handle the error...
* }
* because errp may be NULL!
This is what your patch does.
*
* But when all you do with the error is pass it on, please use
* foo(arg, errp);
* for readability.
*
> + return;
> + }
> + }
> +
> memory_region_init_io(&d->mmio, OBJECT(d), &intel_hda_mmio_ops, d,
> "intel-hda", 0x4000);
> pci_register_bar(&d->pci, 0, 0, &d->mmio);
> - if (d->msi) {
> - msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, 1, true, false);
> - }
>
> hda_codec_bus_init(DEVICE(pci), &d->codecs, sizeof(d->codecs),
> intel_hda_response, intel_hda_xfer);
> diff --git a/hw/ide/ich.c b/hw/ide/ich.c
> index 0a13334..db4fdb5 100644
> --- a/hw/ide/ich.c
> +++ b/hw/ide/ich.c
> @@ -146,7 +146,7 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error
> **errp)
> /* Although the AHCI 1.3 specification states that the first capability
> * should be PMCAP, the Intel ICH9 data sheet specifies that the ICH9
> * AHCI device puts the MSI capability first, pointing to 0x80. */
> - msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false);
> + msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false, errp);
Sure there's nothing to undo on error? Instead of undoing, you may want
to move msi_init() before the stuff that needs undoing.
> }
>
> static void pci_ich9_uninit(PCIDevice *dev)
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 7a38e47..d8dbb0b 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -2189,27 +2189,6 @@ vmxnet3_cleanup_msix(VMXNET3State *s)
> }
> }
>
> -#define VMXNET3_USE_64BIT (true)
> -#define VMXNET3_PER_VECTOR_MASK (false)
> -
> -static bool
> -vmxnet3_init_msi(VMXNET3State *s)
> -{
> - PCIDevice *d = PCI_DEVICE(s);
> - int res;
> -
> - res = msi_init(d, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS,
> - VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK);
> - if (0 > res) {
> - VMW_WRPRN("Failed to initialize MSI, error %d", res);
> - s->msi_used = false;
> - } else {
> - s->msi_used = true;
> - }
> -
> - return s->msi_used;
> -}
> -
> static void
> vmxnet3_cleanup_msi(VMXNET3State *s)
> {
> @@ -2271,13 +2250,29 @@ static uint8_t
> *vmxnet3_device_serial_num(VMXNET3State *s)
> return dsnp;
> }
>
> +
> +#define VMXNET3_USE_64BIT (true)
> +#define VMXNET3_PER_VECTOR_MASK (false)
> +
> static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
> {
> DeviceState *dev = DEVICE(pci_dev);
> VMXNET3State *s = VMXNET3(pci_dev);
> + int ret;
>
> VMW_CBPRN("Starting init...");
>
> + ret = msi_init(pci_dev, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS,
> + VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK, errp);
> + if (ret < 0) {
> + error_free_or_abort(errp);
Aborts when errp is null.
> + VMW_WRPRN("Failed to initialize MSI, error = %d."
> + " Configuration is inconsistent.", ret);
For friendlier debug messages, you could do
ret = msi_init(pci_dev, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS,
VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK, &err);
if (ret < 0) {
VMW_WRPRN("Failed to initialize MSI: %s", error_get_pretty(err);
error_free(err);
However, begs the question why we let realize succeed after msi_init()
failure for this device, but not for others. See discussion of
msi_init() failure modes below.
> + s->msi_used = false;
> + } else {
> + s->msi_used = true;
> + }
> +
> memory_region_init_io(&s->bar0, OBJECT(s), &b0_ops, s,
> "vmxnet3-b0", VMXNET3_PT_REG_SIZE);
> pci_register_bar(pci_dev, VMXNET3_BAR0_IDX,
> @@ -2302,10 +2297,6 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev,
> Error **errp)
> VMW_WRPRN("Failed to initialize MSI-X, configuration is
> inconsistent.");
> }
>
> - if (!vmxnet3_init_msi(s)) {
> - VMW_WRPRN("Failed to initialize MSI, configuration is
> inconsistent.");
> - }
> -
> vmxnet3_net_init(s);
>
> if (pci_is_express(pci_dev)) {
> diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
> index b4a7806..d752e62 100644
> --- a/hw/pci-bridge/ioh3420.c
> +++ b/hw/pci-bridge/ioh3420.c
> @@ -97,6 +97,7 @@ static int ioh3420_initfn(PCIDevice *d)
> PCIEPort *p = PCIE_PORT(d);
> PCIESlot *s = PCIE_SLOT(d);
> int rc;
> + Error *err = NULL;
>
> pci_bridge_initfn(d, TYPE_PCIE_BUS);
> pcie_port_init_reg(d);
> @@ -109,8 +110,9 @@ static int ioh3420_initfn(PCIDevice *d)
>
> rc = msi_init(d, IOH_EP_MSI_OFFSET, IOH_EP_MSI_NR_VECTOR,
> IOH_EP_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_64BIT,
> - IOH_EP_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT);
> + IOH_EP_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT, &err);
> if (rc < 0) {
> + error_report_err(err);
> goto err_bridge;
> }
>
> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
> index 32f4daa..07c7bf8 100644
> --- a/hw/pci-bridge/pci_bridge_dev.c
> +++ b/hw/pci-bridge/pci_bridge_dev.c
> @@ -52,6 +52,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
> PCIBridge *br = PCI_BRIDGE(dev);
> PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
> int err;
> + Error *local_err = NULL;
>
> pci_bridge_initfn(dev, TYPE_PCI_BUS);
>
> @@ -75,8 +76,9 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>
> if ((bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_MSI_REQ)) &&
> msi_nonbroken) {
> - err = msi_init(dev, 0, 1, true, true);
> + err = msi_init(dev, 0, 1, true, true, &local_err);
> if (err < 0) {
> + error_report_err(local_err);
> goto msi_error;
> }
> }
> diff --git a/hw/pci-bridge/xio3130_downstream.c
> b/hw/pci-bridge/xio3130_downstream.c
> index e6d653d..0982801 100644
> --- a/hw/pci-bridge/xio3130_downstream.c
> +++ b/hw/pci-bridge/xio3130_downstream.c
> @@ -60,14 +60,16 @@ static int xio3130_downstream_initfn(PCIDevice *d)
> PCIEPort *p = PCIE_PORT(d);
> PCIESlot *s = PCIE_SLOT(d);
> int rc;
> + Error *err = NULL;
>
> pci_bridge_initfn(d, TYPE_PCIE_BUS);
> pcie_port_init_reg(d);
>
> rc = msi_init(d, XIO3130_MSI_OFFSET, XIO3130_MSI_NR_VECTOR,
> XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_64BIT,
> - XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT);
> + XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT, &err);
> if (rc < 0) {
> + error_report_err(err);
> goto err_bridge;
> }
>
> diff --git a/hw/pci-bridge/xio3130_upstream.c
> b/hw/pci-bridge/xio3130_upstream.c
> index d976844..1d2c597 100644
> --- a/hw/pci-bridge/xio3130_upstream.c
> +++ b/hw/pci-bridge/xio3130_upstream.c
> @@ -56,14 +56,16 @@ static int xio3130_upstream_initfn(PCIDevice *d)
> {
> PCIEPort *p = PCIE_PORT(d);
> int rc;
> + Error *err = NULL;
>
> pci_bridge_initfn(d, TYPE_PCIE_BUS);
> pcie_port_init_reg(d);
>
> rc = msi_init(d, XIO3130_MSI_OFFSET, XIO3130_MSI_NR_VECTOR,
> XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_64BIT,
> - XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT);
> + XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT, &err);
> if (rc < 0) {
> + error_report_err(err);
> goto err_bridge;
> }
>
> diff --git a/hw/pci/msi.c b/hw/pci/msi.c
> index e2a701b..bf7a3b9 100644
> --- a/hw/pci/msi.c
> +++ b/hw/pci/msi.c
> @@ -179,14 +179,17 @@ bool msi_enabled(const PCIDevice *dev)
> * -ENOTSUP means lacking msi support for a msi-capable platform.
> */
> int msi_init(struct PCIDevice *dev, uint8_t offset,
> - unsigned int nr_vectors, bool msi64bit, bool
> msi_per_vector_mask)
> + unsigned int nr_vectors, bool msi64bit,
> + bool msi_per_vector_mask, Error **errp)
> {
> unsigned int vectors_order;
> uint16_t flags;
> uint8_t cap_size;
> int config_offset;
> + Error *err = NULL;
>
> if (!msi_nonbroken) {
> + error_setg(errp, "MSI is not supported by interrupt controller");
> return -ENOTSUP;
> }
>
> @@ -210,8 +213,10 @@ int msi_init(struct PCIDevice *dev, uint8_t offset,
> }
>
> cap_size = msi_cap_sizeof(flags);
> - config_offset = pci_add_capability(dev, PCI_CAP_ID_MSI, offset,
> cap_size);
> + config_offset = pci_add_capability2(dev, PCI_CAP_ID_MSI, offset,
> + cap_size, &err);
> if (config_offset < 0) {
> + error_propagate(errp, err);
> return config_offset;
> }
>
msi_init() has three failure modes:
* -ENOTSUP
Board's MSI emulation is not known to work: !msi_nonbroken.
This is not necessarily an error.
It is when the device model requires MSI.
It isnt' when a non-MSI variant of the device model exists. Then
caller should silently switch to the non-MSI variant[*].
* -ENOSPC
Out of PCI config space. Can happen only when offset == 0. I believe
this is a programming error, and therefore should be an assertion
failure. But changing pci_add_capability2() that way is outside this
patch's scope, and up to the PCI maintainers.
* -EINVAL
PCI capabilities overlap. Can happen only when offset != 0. Also a
programming error, except when assigning a physical device. There,
it's a broken physical device.
So, for devices with a non-MSI variant, realize() should use msi_init()
like this:
ret = msi_init(..., &err);
if (ret == -ENOTSUP) {
error_free(err);
[switch off MSI]
} else if (ret < 0) {
error_propagate(errp, err);
[handle error]
}
Your patch lacks the special -ENOTSUP case.
init() should error_report_err() + return -1 instead of
error_propagate(), of course.
[handle error] needs to take care to revert previous side effects.
For devices that require MSI, it's either
msi_init(..., &err);
if (err) {
error_propagate(errp, err);
[handle error]
}
or
if (msi_init(..., errp)) {
[handle error]
}
I don't have the time to review the rest of the patch now, but I hope
this is enough for a productive v5.
[...]
[*] Inappropriate when the user ordered msi=on, but that's outside this
patch's scope.
