Re: [Qemu-devel] [PATCH 1/2] Add param Error** to msi_init() & modify the callers

[Markus Armbruster <armbru@redhat.com>]

Cao jin <caoj.fnst@cn.fujitsu.com> 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 <caoj.fnst@cn.fujitsu.com>
> ---
>  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.