Re: [Qemu-devel] [PATCH v6 11/11] pci: Convert msi_init() to Error and fix callers to check it

[Markus Armbruster]

Cao jin <address@hidden> writes:

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

Thank you :)

> 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) {
>>                 error_free(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 */
>>             error_free(err);
>>
>> 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);
>
> Right?

You're right in that my assertion is wrong: msi_init() returns an offset
on success (>= 0).  It should be

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

In English: if msi_init() fails (ret < 0), any failure other than
-ENOTSUP is a programming error.

> 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?

If you feel a comment is needed, perhaps: Any error other than -ENOTSUP
(board's MSI support is broken) is a programming error.

> 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.

Only possible if none of its users needs the offset.

Making the two consistent would be nice.

>>             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);
>>                 return;
>>             }
>>             assert(!err || d->msi == ON_OFF_AUTO_AUTO);
>>             /* With msi=auto, we fall back to MSI off silently */
>>             error_free(err);
>>
>>> +    }
>>> +
>
>>>
>>> @@ -111,6 +111,16 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, 
>>> Error **errp)
>>>       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 
>>> inconsistent.");
>>
>> 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)
          if (bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_SHPC_REQ)) {
              dev->config[PCI_INTERRUPT_PIN] = 0x1;
              memory_region_init(&bridge_dev->bar, OBJECT(dev), "shpc-bar",
                                 shpc_bar_size(dev));
              err = shpc_init(dev, &br->sec_bus, &bridge_dev->bar, 0);
              if (err) {
                  goto shpc_error;
              }
          } else {
              /* MSI is not applicable without SHPC */
--->          bridge_dev->msi = ON_OFF_AUTO_OFF;

SHPC is required for MSI.

SHPC is controlled by property "shpc", default on.

Switching MSI off for shpc=off,msi=auto is fine.  But shpc=off,msi=on
should be an error.

This device messes with its configuration, like megasas does.  I
explained there why that's problematic, but that fixing it is not this
patch's business.

          }

          err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0);
          if (err) {
>>>           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?

It does because of the spot I marked ---> above.

Is a comment necessary here?

> 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?

Before the patch:

    shpc  msi | msi'  SHPC  MSI
     off  off |  off    no   no
     off   on |  off    no   no
      on  off |  off   yes   no
      on   on |   on   yes  yes*

where
    shpc is the value of property "shpc"
    msi is the value of property "msi" before realize
    msi' is the value after realize,
    SHPC is whether the device has the SHPC structure
    MSI is whether it has the MSI capability
    yes* means yes unless !msi_nonbroken

After the patch:

    shpc  msi | msi'  SHPC  MSI
     off  off |  off    no   no   unchanged
     off   on |  off    no   no   unchanged, but should be an error
     off auto |  off    no   no   new, good
      on  off |  off   yes   no   unchanged
      on   on |   on   yes  yes   !msi_nonbroken is now an error, good
      on auto | auto   yes  yes*  new, good

I can't help to wonder whether we really need property "shpc".  Is the
combination "have SHPC but not MSI" useful?  Do we have to maintain
backward compatibility?  You may not be the right person to answer these
questions.  That's okay, your patch can simply preserve the status quo.

>>> 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) {
>>>           trace_pvscsi_init_msi_fail(res);
>>> +        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

Thanks!