Re: [Qemu-devel] [PATCH v2 3/5] pci: Convert msix_init() to Error and fix callers to check it

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

On 09/12/2016 09:47 PM, Markus Armbruster wrote:
> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>

>>   static void
>>   vmxnet3_cleanup_msix(VMXNET3State *s)
>>   {
>> @@ -2315,9 +2289,19 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
>>        * is a programming error. Fall back to INTx silently on -ENOTSUP */
>>       assert(!ret || ret == -ENOTSUP);
>>
>> -    if (!vmxnet3_init_msix(s)) {
>> -        VMW_WRPRN("Failed to initialize MSI-X, configuration is inconsistent.");
>> -    }
>> +    ret = msix_init(pci_dev, VMXNET3_MAX_INTRS,
>> +                    &s->msix_bar,
>> +                    VMXNET3_MSIX_BAR_IDX, VMXNET3_OFF_MSIX_TABLE,
>> +                    &s->msix_bar,
>> +                    VMXNET3_MSIX_BAR_IDX, VMXNET3_OFF_MSIX_PBA(s),
>> +                    VMXNET3_MSIX_OFFSET(s), NULL);
>> +    /* Any error other than -ENOTSUP(board's MSI support is broken)
>> +     * is a programming error. Fall back to INTx silently on -ENOTSUP */
>> +    assert(!ret || ret == -ENOTSUP);
>> +    s->msix_used = !ret;
>> +    /* VMXNET3_MAX_INTRS is passed, so it will never fail when mark vector.
>> +     * For simplicity, no need to check return value. */
>> +    vmxnet3_use_msix_vectors(s, VMXNET3_MAX_INTRS);
>>
>>       vmxnet3_net_init(s);
>
> Uh, this is more than just a conversion to Error.  Before, the code
> falls back to not using MSI-X on any error, with a warning.  After, it
> falls back on ENOTSUP only, silently, and crashes on any other error.
> Such a change needs to be documented in the commit message, or be in a
> separate patch.  I prefer separate patch.
>

Dmitry has option that we should check the return value of 
msix_vector_use and prefer to keep init function, so I will withdraw 
this modification.

>> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
>> index 188f954..4280c5d 100644
>> --- a/hw/usb/hcd-xhci.c
>> +++ b/hw/usb/hcd-xhci.c
>> @@ -3594,25 +3594,6 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
>>       dev->config[PCI_CACHE_LINE_SIZE] = 0x10;
>>       dev->config[0x60] = 0x30; /* release number */
>>
>> -    usb_xhci_init(xhci);
>> -
>> -    if (xhci->msi != ON_OFF_AUTO_OFF) {
>> -        ret = msi_init(dev, 0x70, xhci->numintrs, true, false, &err);
>> -        /* Any error other than -ENOTSUP(board's MSI support is broken)
>> -         * is a programming error */
>> -        assert(!ret || ret == -ENOTSUP);
>> -        if (ret && xhci->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 || xhci->msi == ON_OFF_AUTO_AUTO);
>> -        /* With msi=auto, we fall back to MSI off silently */
>> -        error_free(err);
>> -    }
>> -
>>       if (xhci->numintrs > MAXINTRS) {
>>           xhci->numintrs = MAXINTRS;
>>       }
>> @@ -3622,21 +3603,60 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
>>       if (xhci->numintrs < 1) {
>>           xhci->numintrs = 1;
>>       }
>> +
>>       if (xhci->numslots > MAXSLOTS) {
>>           xhci->numslots = MAXSLOTS;
>>       }
>>       if (xhci->numslots < 1) {
>>           xhci->numslots = 1;
>>       }
>> +
>>       if (xhci_get_flag(xhci, XHCI_FLAG_ENABLE_STREAMS)) {
>>           xhci->max_pstreams_mask = 7; /* == 256 primary streams */
>>       } else {
>>           xhci->max_pstreams_mask = 0;
>>       }
>>
>> -    xhci->mfwrap_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, xhci_mfwrap_timer, xhci);
>> +    if (xhci->msi != ON_OFF_AUTO_OFF) {
>> +        ret = msi_init(dev, 0x70, xhci->numintrs, true, false, &err);
>> +        /* Any error other than -ENOTSUP(board's MSI support is broken)
>> +         * is a programming error */
>> +        assert(!ret || ret == -ENOTSUP);
>> +        if (ret && xhci->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 || xhci->msi == ON_OFF_AUTO_AUTO);
>> +        /* With msi=auto, we fall back to MSI off silently */
>> +        error_free(err);
>> +    }
>
> Can you explain why you're moving this code?
>

Sorry I forget to mention this: msi_init() uses xhci->numintrs, but 
there is value checking/correcting on xhci->numintrs, it should be done 
before using.

-- 
Yours Sincerely,

Cao jin