qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 2/6] pci: Convert msix_init() to Error and fix c


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH 2/6] pci: Convert msix_init() to Error and fix callers to check it
Date: Thu, 18 Aug 2016 09:55:41 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)

Cao jin <address@hidden> writes:

> msix_init() has the same issue with msi_init(), which reports errors
> via error_report(), that is not suitable when it's used in realize().

Suggest to point to commit 1108b2f.  Perhaps:

    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.

> Fix it by converting it to Error, also fix its callers to
> handle failure instead of ignoring it.
>
> 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>
> Signed-off-by: Cao jin <address@hidden>
> ---
>  hw/misc/ivshmem.c      |  1 +
>  hw/net/e1000e.c        |  2 +-
>  hw/net/rocker/rocker.c |  2 +-
>  hw/net/vmxnet3.c       | 42 +++++++++--------------------
>  hw/pci/msix.c          | 18 +++++++++----
>  hw/scsi/megasas.c      | 25 ++++++++++++++----
>  hw/usb/hcd-xhci.c      | 71 
> ++++++++++++++++++++++++++++++--------------------
>  hw/vfio/pci.c          |  7 +++--
>  hw/virtio/virtio-pci.c |  7 ++---
>  include/hw/pci/msix.h  |  3 ++-
>  10 files changed, 101 insertions(+), 77 deletions(-)
>
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index 40a2ebc..b8511ee 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -899,6 +899,7 @@ static void ivshmem_common_realize(PCIDevice *dev, Error 
> **errp)
>  
>          if (ivshmem_setup_interrupts(s) < 0) {
>              error_setg(errp, "failed to initialize interrupts");
> +            error_append_hint(errp, "MSI-X is not supported by interrupt 
> controller");
>              return;
>          }
>      }

How is this related to the stated purpose of the patch?

> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
> index d001c96..82a7be1 100644
> --- a/hw/net/e1000e.c
> +++ b/hw/net/e1000e.c
> @@ -295,7 +295,7 @@ e1000e_init_msix(E1000EState *s)
       int res = msix_init(PCI_DEVICE(s), E1000E_MSIX_VEC_NUM,
                           &s->msix,
>                          E1000E_MSIX_IDX, E1000E_MSIX_TABLE,
>                          &s->msix,
>                          E1000E_MSIX_IDX, E1000E_MSIX_PBA,
> -                        0xA0);
> +                        0xA0, NULL);

Further down, you convert msix_init() from error_report() to
error_setg().  It now relies on its callers to report errors.  However,
this caller doesn't.  Suppressing error reports like that may be
appropriate, since the function doesn't fail.  But such a change
shouldn't be hidden in a larger patch without at least a mention in the
commit message.

Here's how I'd skin this cat.  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.  The ones that are running within a
realize() method should propagate the error to the realize() method.
The ones that are still running within an init() method keep the
error_report_err().  e1000e_init_msix() may want to ignore the error.
Separaring the cleanups that way lets you explain each actual change
neatly in a commit message.

>  
>      if (res < 0) {
>          trace_e1000e_msix_init_fail(res);
> diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
> index 30f2ce4..769b1fd 100644
> --- a/hw/net/rocker/rocker.c
> +++ b/hw/net/rocker/rocker.c
> @@ -1262,7 +1262,7 @@ static int rocker_msix_init(Rocker *r)
       err = msix_init(dev, ROCKER_MSIX_VEC_COUNT(r->fp_ports),
                       &r->msix_bar,
>                      ROCKER_PCI_MSIX_BAR_IDX, ROCKER_PCI_MSIX_TABLE_OFFSET,
>                      &r->msix_bar,
>                      ROCKER_PCI_MSIX_BAR_IDX, ROCKER_PCI_MSIX_PBA_OFFSET,
> -                    0);
> +                    0, NULL);
>      if (err) {
>          return err;
>      }

This one runs in an init() method.  You're losing an error message here.

> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index bbf44ad..0ee80b7 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -2177,32 +2177,6 @@ vmxnet3_use_msix_vectors(VMXNET3State *s, int 
> num_vectors)
>      return true;
>  }
>  
> -static bool
> -vmxnet3_init_msix(VMXNET3State *s)
> -{
> -    PCIDevice *d = PCI_DEVICE(s);
> -    int res = msix_init(d, 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));
> -
> -    if (0 > res) {
> -        VMW_WRPRN("Failed to initialize MSI-X, error %d", res);
> -        s->msix_used = false;
> -    } else {
> -        if (!vmxnet3_use_msix_vectors(s, VMXNET3_MAX_INTRS)) {
> -            VMW_WRPRN("Failed to use MSI-X vectors, error %d", res);
> -            msix_uninit(d, &s->msix_bar, &s->msix_bar);
> -            s->msix_used = false;
> -        } else {
> -            s->msix_used = true;
> -        }
> -    }
> -    return s->msix_used;
> -}
> -
>  static void
>  vmxnet3_cleanup_msix(VMXNET3State *s)
>  {
> @@ -2311,9 +2285,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);
>  

This is similar to the e1000e case.

> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> index 384a29d..27fee0a 100644
> --- a/hw/pci/msix.c
> +++ b/hw/pci/msix.c
> @@ -21,6 +21,7 @@
>  #include "hw/pci/pci.h"
>  #include "hw/xen/xen.h"
>  #include "qemu/range.h"
> +#include "qapi/error.h"
>  
>  #define MSIX_CAP_LENGTH 12
>  
> @@ -242,7 +243,8 @@ static void msix_mask_all(struct PCIDevice *dev, unsigned 
> nentries)

   /* Initialize the MSI-X structures */
>  int msix_init(struct 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)

Turning the function comment into a contract similar to the one next to
msi_init() would be nice.

>  {
>      int cap;
>      unsigned table_size, pba_size;
> @@ -250,6 +252,7 @@ int msix_init(struct PCIDevice *dev, unsigned short 
> nentries,
>  
>      /* Nothing to do if MSI is not supported by interrupt controller */
>      if (!msi_nonbroken) {
> +        error_setg(errp, "MSI-X is not supported by interrupt controller");
>          return -ENOTSUP;
>      }
>  
> @@ -267,7 +270,8 @@ int msix_init(struct PCIDevice *dev, unsigned short 
> nentries,
>          assert(0);
>      }
>  
> -    cap = pci_add_capability(dev, PCI_CAP_ID_MSIX, cap_pos, MSIX_CAP_LENGTH);
> +    cap = pci_add_capability2(dev, PCI_CAP_ID_MSIX,
> +                              cap_pos, MSIX_CAP_LENGTH, errp);
>      if (cap < 0) {
>          return cap;
>      }
> @@ -336,7 +340,7 @@ int msix_init_exclusive_bar(PCIDevice *dev, unsigned 
> short nentries,
>      ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr,
>                      0, &dev->msix_exclusive_bar,
>                      bar_nr, bar_pba_offset,
> -                    0);
> +                    0, NULL);
>      if (ret) {
>          return ret;
>      }

This is a case where you have to propagate the error.  First step:
convert msix_exclusive_bar() to Error, too.

> @@ -445,8 +449,10 @@ void msix_notify(PCIDevice *dev, unsigned vector)
>  {
>      MSIMessage msg;
>  
> -    if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector])
> +    if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector]) {
>          return;
> +    }
> +
>      if (msix_is_masked(dev, vector)) {
>          msix_set_pending(dev, vector);
>          return;

Let's drop this hunk.

> @@ -481,8 +487,10 @@ void msix_reset(PCIDevice *dev)
>  /* Mark vector as used. */
>  int msix_vector_use(PCIDevice *dev, unsigned vector)
>  {
> -    if (vector >= dev->msix_entries_nr)
> +    if (vector >= dev->msix_entries_nr) {
>          return -EINVAL;
> +    }
> +
>      dev->msix_entry_used[vector]++;
>      return 0;
>  }

This one, too.

> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index e968302..61efb0f 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -2349,16 +2349,31 @@ static void megasas_scsi_realize(PCIDevice *dev, 
> Error **errp)
>  
>      memory_region_init_io(&s->mmio_io, OBJECT(s), &megasas_mmio_ops, s,
>                            "megasas-mmio", 0x4000);
> +    if (megasas_use_msix(s)) {
> +        ret = msix_init(dev, 15, &s->mmio_io, b->mmio_bar, 0x2000,
> +                        &s->mmio_io, b->mmio_bar, 0x3800, 0x68, &err);
> +        /* Any error other than -ENOTSUP(board's MSI support is broken)
> +         * is a programming error */
> +        assert(!ret || ret == -ENOTSUP);
> +        if (ret && s->msix == ON_OFF_AUTO_ON) {
> +            /* Can't satisfy user's explicit msix=on request, fail */
> +            error_append_hint(&err, "You have to use msix=auto (default) or "
> +                    "msix=off with this machine type.\n");
> +            object_unref(OBJECT(&s->mmio_io));
> +            error_propagate(errp, err);
> +            return;
> +        } else if (ret) {
> +            /* With msix=auto, we fall back to MSI off silently */
> +            s->msix = ON_OFF_AUTO_OFF;
> +            error_free(err);
> +        }
> +    }
> +
>      memory_region_init_io(&s->port_io, OBJECT(s), &megasas_port_ops, s,
>                            "megasas-io", 256);
>      memory_region_init_io(&s->queue_io, OBJECT(s), &megasas_queue_ops, s,
>                            "megasas-queue", 0x40000);
>  
> -    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->msix = ON_OFF_AUTO_OFF;
> -    }
>      if (pci_is_express(dev)) {
>          pcie_endpoint_cap_init(dev, 0xa0);
>      }

Here, you already propagate.

[Skipping the remainder of the patch for now...]



reply via email to

[Prev in Thread] Current Thread [Next in Thread]