Re: [Qemu-devel] [PATCH v4 5/5] Add param Error ** for msi_init()

[Markus Armbruster]

Cao jin <address@hidden> 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 <address@hidden>
> ---
>  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.