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

[Cao jin <caoj.fnst@cn.fujitsu.com>]

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 <caoj.fnst@cn.fujitsu.com> 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?
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);
>                 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)
>>           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) {
>>           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

-- 
Yours Sincerely,

Cao jin