[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v6 11/11] pci: Convert msi_init() to Error and f

From: Cao jin
Subject: Re: [Qemu-devel] [PATCH v6 11/11] pci: Convert msi_init() to Error and fix callers to check it
Date: Fri, 3 Jun 2016 16:28:34 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0

Hi Markus,

Thanks very much for your thorough review for the whole series, really really impressed:)

On 06/01/2016 08:37 PM, Markus Armbruster wrote:
Cao jin <address@hidden> writes:

msi_init() reports errors with error_report(), which is wrong
when it's used in realize().

msix_init() has the same problem.  Perhaps you can tackle it later.

Sure, I will take care of it after this one has passed the review.

+            error_propagate(errp, err);
+            return;
+        } else if (err && d->msi == ON_OFF_AUTO_AUTO) {
+            /* If user doesn`t set it on, switch to non-msi variant silently */
+            error_free(err);
+        }

The conditional is superfluous.

We call msi_init() only if d->msi != ON_OFF_AUTO_OFF.  If it sets @err
and d->msi == ON_OFF_AUTO_ON, we don't get here.  Therefore, err implies
d->msi == ON_OFF_AUTO_AUTO, and the conditional can be simplified to

            } else if (err) {

But protecting error_free() like that is pointless, as it does nothing
when err is null.  Simplify further to

            assert(!err || d->msi == ON_OFF_AUTO_AUTO);
            /* With msi=auto, we fall back to MSI off silently */

The assertion is more for aiding the reader than for catching mistakes.

It take me a little while to understand the following tightened error checking:)

The error checking could be tightened as follows:

            ret = msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60,
                           1, true, false, &err);
            assert(!ret || ret == -ENOTSUP);

I guess you are assuming msi_init return 0 on success(all the following example code are), and actually it is the behaviour of msix_init, you mentioned the difference between them before. So I think it should be

        assert(ret < 0 || ret == -ENOTSUP);

And I think it is better to add a comments on it, for newbie understanding, like this:

/* -EINVAL means capability overlap, which is programming error for this device, so, assert it */

Is the comment ok?

And I will add a new patch in this series to make msi_init return 0 on success, and -error on failure, make it consistent with msix_init, so your example code will apply.

            if (ret && d->msi == ON_OFF_AUTO_ON) {
                /* Can't satisfy user's explicit msi=on request, fail */
                error_append_hint(&err, "You have to use msi=auto (default)"
                                  " or msi=off with this machine type.\n");
                error_propagate(errp, err);
            assert(!err || d->msi == ON_OFF_AUTO_AUTO);
            /* With msi=auto, we fall back to MSI off silently */

+    }

@@ -111,6 +111,16 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error 
      int sata_cap_offset;
      uint8_t *sata_cap;
      d = ICH_AHCI(dev);
+    Error *err = NULL;
+    /* 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, &err);
+    if (err) {
+        /* Fall back to INTx silently */
+        error_free(err);
+    }

Tighter error checking:

        ret = msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false, NULL);
        /* Fall back to INTx silently on -ENOTSUP */
        assert(!ret || ret == -ENOTSUP);

More concise, too.  No need for the include "qapi/error.h" then.

Learned, and /assert/ is for -EINVAL, so I will add a comment as I mentioned above for easy understanding, So will I do for all the following example code:)

+    if (!vmxnet3_init_msix(s)) {
+        VMW_WRPRN("Failed to initialize MSI-X, configuration is 

It doesn't replace it here, but that's appropriate, because it doesn't
touch MSI-X code, it only moves it.

will replace it when tackle msix_init

diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index fa0c50c..7f9131f 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -54,6 +54,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,12 +76,18 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
          goto slotid_error;

-    if ((bridge_dev->msi == ON_OFF_AUTO_AUTO ||
-                bridge_dev->msi == ON_OFF_AUTO_ON) &&
-        msi_nonbroken) {
-        err = msi_init(dev, 0, 1, true, true);
-        if (err < 0) {
+    if (bridge_dev->msi != ON_OFF_AUTO_OFF) {

Unnecessary churn, please use != ON_OFF_AUTO_OFF in PATCH 10.

+        /* it means SHPC exists */

Does it?  Why?

The comments says: /* MSI is not applicable without SHPC */. And also before the patch, we can see there are only following combination available:
    [shpc: on, msi:on] [shpc: on, msi:off] [shpc: off, msi: off]

But there is no:
    [shpc: off, msi: on]

So if msi != OFF, it implies shcp is on, right?

diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
index c2a387a..b040575 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -1044,12 +1044,16 @@ static void
  pvscsi_init_msi(PVSCSIState *s)
      int res;
+    Error *err = NULL;
      PCIDevice *d = PCI_DEVICE(s);

      res = msi_init(d, PVSCSI_MSI_OFFSET(s), PVSCSI_MSIX_NUM_VECTORS,
-                   PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK);
+                   PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK, &err);
      if (res < 0) {
+        error_append_hint(&err, "MSI capability fail to init,"
+                " will use INTx intead\n");
+        error_report_err(err);
          s->msi_used = false;
      } else {
          s->msi_used = true;

This is MSI device pattern #5: on whenever possible, else off, but
report an error when falling back to off.

Before your patch, this is pattern #2.  Why do you add error reporting
here?  You don't in other instances of pattern #2.

I dunno...maybe just flash into my mind randomly:-[ will get rid of it

Yours Sincerely,

Cao jin

reply via email to

[Prev in Thread] Current Thread [Next in Thread]