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

[Cao jin]

On 09/12/2016 09:47 PM, Markus Armbruster wrote:
> Cao jin <address@hidden> 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