| Message ID | 1471444747-6277-3-git-send-email-caoj.fnst@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
Cao jin <caoj.fnst@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 <jiri@resnulli.us> > CC: Gerd Hoffmann <kraxel@redhat.com> > CC: Dmitry Fleytman <dmitry@daynix.com> > CC: Jason Wang <jasowang@redhat.com> > CC: Michael S. Tsirkin <mst@redhat.com> > CC: Hannes Reinecke <hare@suse.de> > CC: Paolo Bonzini <pbonzini@redhat.com> > CC: Alex Williamson <alex.williamson@redhat.com> > CC: Markus Armbruster <armbru@redhat.com> > CC: Marcel Apfelbaum <marcel@redhat.com> > Signed-off-by: Cao jin <caoj.fnst@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...]
On 08/18/2016 03:55 PM, Markus Armbruster wrote: > Cao jin <caoj.fnst@cn.fujitsu.com> writes: > >> >> 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? > Because I don't propagate error via msix_init_exclusive_bar(), so add this to show the detail error message to user.(Please also see my comment on msix_init_exclusive_bar(), then come back to this comment) >> 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. > For this device, I was planning 1)make this patch as small as possible for reviewer's convenience sake(so suppressed error report here), then 2) drop this function as another patch, and then 3) convert this device to .realize(), error report or setting error could be placed at one of last two steps. For other devices, the plan is basically the same. > 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. Indeed... planned to propagate or set error object when convert the device to realize because the only failure is -ENOTSUP >> @@ -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. I was thinking: all devices call msix_init_exclusive_bar() will only see -ENOTSUP on failure, so, to make this patch short and simple, we could set error or report error in the caller(or caller's caller, that is what I have done in ivshmem_common_realize() at the top of this patch). > >> @@ -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. > Sorry, I forget to make the coding style changes into a separated one.
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; } } 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) E1000E_MSIX_IDX, E1000E_MSIX_TABLE, &s->msix, E1000E_MSIX_IDX, E1000E_MSIX_PBA, - 0xA0); + 0xA0, NULL); 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) 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; } 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); 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) 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) { 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; } @@ -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; @@ -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; } 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); } diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c index 188f954..4280c5d 100644 --- a/hw/usb/hcd-xhci.c +++ b/hw/usb/hcd-xhci.c @@ -3594,25 +3594,6 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp) dev->config[PCI_CACHE_LINE_SIZE] = 0x10; dev->config[0x60] = 0x30; /* release number */ - usb_xhci_init(xhci); - - if (xhci->msi != ON_OFF_AUTO_OFF) { - ret = msi_init(dev, 0x70, xhci->numintrs, true, false, &err); - /* Any error other than -ENOTSUP(board's MSI support is broken) - * is a programming error */ - assert(!ret || ret == -ENOTSUP); - if (ret && xhci->msi == ON_OFF_AUTO_ON) { - /* Can't satisfy user's explicit msi=on request, fail */ - error_append_hint(&err, "You have to use msi=auto (default) or " - "msi=off with this machine type.\n"); - error_propagate(errp, err); - return; - } - assert(!err || xhci->msi == ON_OFF_AUTO_AUTO); - /* With msi=auto, we fall back to MSI off silently */ - error_free(err); - } - if (xhci->numintrs > MAXINTRS) { xhci->numintrs = MAXINTRS; } @@ -3622,21 +3603,60 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp) if (xhci->numintrs < 1) { xhci->numintrs = 1; } + if (xhci->numslots > MAXSLOTS) { xhci->numslots = MAXSLOTS; } if (xhci->numslots < 1) { xhci->numslots = 1; } + if (xhci_get_flag(xhci, XHCI_FLAG_ENABLE_STREAMS)) { xhci->max_pstreams_mask = 7; /* == 256 primary streams */ } else { xhci->max_pstreams_mask = 0; } - xhci->mfwrap_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, xhci_mfwrap_timer, xhci); + if (xhci->msi != ON_OFF_AUTO_OFF) { + ret = msi_init(dev, 0x70, xhci->numintrs, true, false, &err); + /* Any error other than -ENOTSUP(board's MSI support is broken) + * is a programming error */ + assert(!ret || ret == -ENOTSUP); + if (ret && xhci->msi == ON_OFF_AUTO_ON) { + /* Can't satisfy user's explicit msi=on request, fail */ + error_append_hint(&err, "You have to use msi=auto (default) or " + "msi=off with this machine type.\n"); + error_propagate(errp, err); + return; + } + assert(!err || xhci->msi == ON_OFF_AUTO_AUTO); + /* With msi=auto, we fall back to MSI off silently */ + error_free(err); + } memory_region_init(&xhci->mem, OBJECT(xhci), "xhci", LEN_REGS); + if (xhci->msix != ON_OFF_AUTO_OFF) { + ret = msix_init(dev, xhci->numintrs, + &xhci->mem, 0, OFF_MSIX_TABLE, + &xhci->mem, 0, OFF_MSIX_PBA, + 0x90, &err); + /* Any error other than -ENOTSUP(board's MSI support is broken) + * is a programming error */ + assert(!ret || ret == -ENOTSUP); + if (ret && xhci->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 " + "msic=off with this machine type.\n"); + /* No instance_finalize method, need to free the resource here */ + object_unref(OBJECT(&xhci->mem)); + error_propagate(errp, err); + return; + } + assert(!err || xhci->msix == ON_OFF_AUTO_AUTO); + /* With msix=auto, we fall back to MSI off silently */ + error_free(err); + } + memory_region_init_io(&xhci->mem_cap, OBJECT(xhci), &xhci_cap_ops, xhci, "capabilities", LEN_CAP); memory_region_init_io(&xhci->mem_oper, OBJECT(xhci), &xhci_oper_ops, xhci, @@ -3664,19 +3684,14 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp) PCI_BASE_ADDRESS_SPACE_MEMORY|PCI_BASE_ADDRESS_MEM_TYPE_64, &xhci->mem); + usb_xhci_init(xhci); + xhci->mfwrap_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, xhci_mfwrap_timer, xhci); + if (pci_bus_is_express(dev->bus) || xhci_get_flag(xhci, XHCI_FLAG_FORCE_PCIE_ENDCAP)) { ret = pcie_endpoint_cap_init(dev, 0xa0); assert(ret >= 0); } - - 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); - } } static void usb_xhci_exit(PCIDevice *dev) diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index 7bfa17c..87f4e11 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -1349,6 +1349,7 @@ static int vfio_msix_early_setup(VFIOPCIDevice *vdev) static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos) { int ret; + Error *err = NULL; vdev->msix->pending = g_malloc0(BITS_TO_LONGS(vdev->msix->entries) * sizeof(unsigned long)); @@ -1356,12 +1357,14 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos) 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); if (ret < 0) { if (ret == -ENOTSUP) { return 0; } - error_report("vfio: msix_init failed"); + error_prepend(&err, "vfio: msix_init failed: "); + error_report_err(err); return ret; } diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index 755f921..fae0bf1 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -1659,11 +1659,8 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp) int err = msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, proxy->msix_bar); 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("unable to init msix: " + "MSI-X is not supported by interrupt controller"); proxy->nvectors = 0; } } diff --git a/include/hw/pci/msix.h b/include/hw/pci/msix.h index 048a29d..0b0e31b 100644 --- a/include/hw/pci/msix.h +++ b/include/hw/pci/msix.h @@ -9,7 +9,8 @@ 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);
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(). Fix it by converting it to Error, also fix its callers to handle failure instead of ignoring it. Cc: Jiri Pirko <jiri@resnulli.us> CC: Gerd Hoffmann <kraxel@redhat.com> CC: Dmitry Fleytman <dmitry@daynix.com> CC: Jason Wang <jasowang@redhat.com> CC: Michael S. Tsirkin <mst@redhat.com> CC: Hannes Reinecke <hare@suse.de> CC: Paolo Bonzini <pbonzini@redhat.com> CC: Alex Williamson <alex.williamson@redhat.com> CC: Markus Armbruster <armbru@redhat.com> CC: Marcel Apfelbaum <marcel@redhat.com> Signed-off-by: Cao jin <caoj.fnst@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(-)