Re: [Qemu-devel] [PATCH v5 03/10] pci: Convert msix_init() to Error and fix callers to check it

[Cao jin]

On 11/03/2016 07:38 PM, Marcel Apfelbaum wrote:
> On 11/03/2016 06:06 AM, Cao jin wrote:

> >
>
> [...]
>
> > diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> > index 52a4123..fada834 100644
> > --- a/hw/scsi/megasas.c
> > +++ b/hw/scsi/megasas.c
> > @@ -2360,9 +2360,12 @@ static void megasas_scsi_realize(PCIDevice
> > *dev, Error **errp)
> >
> >      if (megasas_use_msix(s) &&
> >          msix_init(dev, 15, &s->mmio_io, b->mmio_bar, 0x2000,
> > -                  &s->mmio_io, b->mmio_bar, 0x3800, 0x68)) {
> > +                  &s->mmio_io, b->mmio_bar, 0x3800, 0x68, &err)) {
> > +        /*TODO: check msix_init's error, and should fail on msix=on */
>
> Why this "TODO", can't we do something similar to other changes already
> done in this patch?

The 1st version of this series handles the error in this patch. look at 
the comments:

http://lists.nongnu.org/archive/html/qemu-devel/2016-08/msg03192.html

*First convert msix_init() without changing behavior, by having every 
caller of msix_init() immediately pass the error received to 
error_report_err().  Then clean up the callers one after the other.*

So later, this patch looks like what it is now. I feel it also make this 
patch thinner, easier to review.

> > +        error_report_err(err);
> >          s->msix = ON_OFF_AUTO_OFF;
> >      }
> > +
> >      if (pci_is_express(dev)) {
> >          pcie_endpoint_cap_init(dev, 0xa0);
> >      }
> > diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c

> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > index d7dbe0e..6fbd30f 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -1432,6 +1432,7 @@ static void vfio_msix_early_setup(VFIOPCIDevice
> > *vdev, Error **errp)
> >  static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
> >  {
> >      int ret;
> > +    Error *err = NULL;
> >
> >      vdev->msix->pending = g_malloc0(BITS_TO_LONGS(vdev->msix->entries) *
> >                                      sizeof(unsigned long));
> > @@ -1439,7 +1440,8 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev,
> > int pos, Error **errp)
> >                      vdev->bars[vdev->msix->table_bar].region.mem,
> >                      vdev->msix->table_bar, vdev->msix->table_offset,
> >                      vdev->bars[vdev->msix->pba_bar].region.mem,
> > -                    vdev->msix->pba_bar, vdev->msix->pba_offset, pos);
> > +                    vdev->msix->pba_bar, vdev->msix->pba_offset, pos,
> > +                    &err);
>
> Do we pass the err pointer to msix_init, but we don't do anything with it?
>
> Also since we do have an *errp in the function already, I suggest
> renaming err -> local_err or something. (only if the series needs a
> re-spin)

yes, it maybe need a re-spin, thanks

> >      if (ret < 0) {
> >          if (ret == -ENOTSUP) {
> >              return 0;
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index 06831de..5acce38 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -1693,13 +1693,12 @@ static void
> > virtio_pci_device_plugged(DeviceState *d, Error **errp)
> >
> >      if (proxy->nvectors) {
> >          int err = msix_init_exclusive_bar(&proxy->pci_dev,
> > proxy->nvectors,
> > -                                          proxy->msix_bar_idx);
> > +                                          proxy->msix_bar_idx, errp);
> > +        /* Any error other than -ENOTSUP(board's MSI support is broken)
> > +         * is a programming error. */
> > +        assert(!err || err == -ENOTSUP);
> >          if (err) {
> > -            /* Notice when a system that supports MSIx can't
> > initialize it.  */
> > -            if (err != -ENOTSUP) {
> > -                error_report("unable to init msix vectors to %" PRIu32,
> > -                             proxy->nvectors);
> > -            }
> > +            error_report_err(*errp);
>
> Why do we report the error here and we don't let the propagation
> mechanism do its thing? We can prep-end the current  message, I think.

The original behaviour won't fail on msix init failure, so, report & 
free the Error keep the behaviour same as before,  propagation will 
results in failing to create virtio device.

> Other than a few questions, the patch looks good to me.
>
> Thanks!
> Marcel
--
Yours Sincerely,

Cao jin