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

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

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