qemu-devel
[Top][All Lists]
Advanced
[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: Cao jin
Subject: Re: [Qemu-devel] [PATCH 1/2] Add param Error** to msi_init() & modify the callers
Date: Tue, 8 Dec 2015 21:23:59 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 
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 <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.
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
reply via email to
[Prev in Thread] Current Thread [Next in Thread]