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

[Marcel Apfelbaum]

On 11/03/2016 06:06 AM, Cao jin wrote:
> msix_init() reports errors with error_report(), which is wrong when
> it's used in realize().  The same issue was fixed for msi_init() in
> commit 1108b2f.
>
> For some devices(like e1000e, vmxnet3) who won't fail because of
> msix_init's failure, suppress the error report by passing NULL error object.
>
> Bonus: add comment for msix_init.
>
> CC: Jiri Pirko <address@hidden>
> CC: Gerd Hoffmann <address@hidden>
> CC: Dmitry Fleytman <address@hidden>
> CC: Jason Wang <address@hidden>
> CC: Michael S. Tsirkin <address@hidden>
> CC: Hannes Reinecke <address@hidden>
> CC: Paolo Bonzini <address@hidden>
> CC: Alex Williamson <address@hidden>
> CC: Markus Armbruster <address@hidden>
> CC: Marcel Apfelbaum <address@hidden>
>
> Reviewed-by: Markus Armbruster <address@hidden>
> Signed-off-by: Cao jin <address@hidden>
> ---
>  hw/block/nvme.c        |  5 ++++-
>  hw/misc/ivshmem.c      |  8 ++++----
>  hw/net/e1000e.c        |  6 +++++-
>  hw/net/rocker/rocker.c |  7 ++++++-
>  hw/net/vmxnet3.c       |  8 ++++++--
>  hw/pci/msix.c          | 34 +++++++++++++++++++++++++++++-----
>  hw/scsi/megasas.c      |  5 ++++-
>  hw/usb/hcd-xhci.c      | 13 ++++++++-----
>  hw/vfio/pci.c          |  4 +++-
>  hw/virtio/virtio-pci.c | 11 +++++------
>  include/hw/pci/msix.h  |  5 +++--
>  11 files changed, 77 insertions(+), 29 deletions(-)

[...]

> 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?

> +        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
> index eb1dca5..05dc944 100644
> --- a/hw/usb/hcd-xhci.c
> +++ b/hw/usb/hcd-xhci.c
> @@ -3703,11 +3703,14 @@ static void usb_xhci_realize(struct PCIDevice *dev, 
> Error **errp)
>      }
>
>      if (xhci->msix != ON_OFF_AUTO_OFF) {
> -        /* TODO check for errors */
> -        msix_init(dev, xhci->numintrs,
> -                  &xhci->mem, 0, OFF_MSIX_TABLE,
> -                  &xhci->mem, 0, OFF_MSIX_PBA,
> -                  0x90);
> +        /* TODO check for errors, and should fail when msix=on */
> +        ret = msix_init(dev, xhci->numintrs,
> +                        &xhci->mem, 0, OFF_MSIX_TABLE,
> +                        &xhci->mem, 0, OFF_MSIX_PBA,
> +                        0x90, &err);
> +        if (ret) {
> +            error_report_err(err);
> +        }
>      }
>  }
>
> 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)

>      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.

>              proxy->nvectors = 0;
>          }
>      }
> diff --git a/include/hw/pci/msix.h b/include/hw/pci/msix.h
> index 048a29d..1f27658 100644
> --- a/include/hw/pci/msix.h
> +++ b/include/hw/pci/msix.h
> @@ -9,9 +9,10 @@ MSIMessage msix_get_message(PCIDevice *dev, unsigned int 
> vector);
>  int msix_init(PCIDevice *dev, unsigned short nentries,
>                MemoryRegion *table_bar, uint8_t table_bar_nr,
>                unsigned table_offset, MemoryRegion *pba_bar,
> -              uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos);
> +              uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos,
> +              Error **errp);
>  int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
> -                            uint8_t bar_nr);
> +                            uint8_t bar_nr, Error **errp);
>
>  void msix_write_config(PCIDevice *dev, uint32_t address, uint32_t val, int 
> len);


Other than a few questions, the patch looks good to me.

Thanks!
Marcel