Cao jin <caoj.f...@cn.fujitsu.com> 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 <j...@resnulli.us>
> CC: Gerd Hoffmann <kra...@redhat.com>
> CC: Dmitry Fleytman <dmi...@daynix.com>
> CC: Jason Wang <jasow...@redhat.com>
> CC: Michael S. Tsirkin <m...@redhat.com>
> CC: Hannes Reinecke <h...@suse.de>
> CC: Paolo Bonzini <pbonz...@redhat.com>
> CC: Alex Williamson <alex.william...@redhat.com>
> CC: Markus Armbruster <arm...@redhat.com>
> CC: Marcel Apfelbaum <mar...@redhat.com>
> Signed-off-by: Cao jin <caoj.f...@cn.fujitsu.com>
> ---
> 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...]
