| Message ID | 1471944454-13895-4-git-send-email-caoj.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Cao jin <caoj.fnst@cn.fujitsu.com> writes: > 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 > msi_init's failure, suppress the error report by passing NULL error object. > > 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/block/nvme.c | 5 +++- > hw/misc/ivshmem.c | 8 +++--- > hw/net/e1000e.c | 2 +- > hw/net/rocker/rocker.c | 4 ++- > hw/net/vmxnet3.c | 42 +++++++++-------------------- > hw/pci/msix.c | 31 ++++++++++++++++++---- > hw/scsi/megasas.c | 26 ++++++++++++++---- > hw/usb/hcd-xhci.c | 71 ++++++++++++++++++++++++++++++-------------------- > hw/vfio/pci.c | 7 +++-- > hw/virtio/virtio-pci.c | 8 ++---- > include/hw/pci/msix.h | 5 ++-- > 11 files changed, 125 insertions(+), 84 deletions(-) > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c > index cef3bb4..ae84dc7 100644 > --- a/hw/block/nvme.c > +++ b/hw/block/nvme.c > @@ -829,6 +829,7 @@ static int nvme_init(PCIDevice *pci_dev) > { > NvmeCtrl *n = NVME(pci_dev); > NvmeIdCtrl *id = &n->id_ctrl; > + Error *err = NULL; > > int i; > int64_t bs_size; > @@ -870,7 +871,9 @@ static int nvme_init(PCIDevice *pci_dev) > pci_register_bar(&n->parent_obj, 0, > PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_64, > &n->iomem); > - msix_init_exclusive_bar(&n->parent_obj, n->num_queues, 4); > + if (msix_init_exclusive_bar(&n->parent_obj, n->num_queues, 4, &err)) { > + error_report_err(err); > + } > > id->vid = cpu_to_le16(pci_get_word(pci_conf + PCI_VENDOR_ID)); > id->ssvid = cpu_to_le16(pci_get_word(pci_conf + PCI_SUBSYSTEM_VENDOR_ID)); > diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c > index 40a2ebc..a1060ec 100644 > --- a/hw/misc/ivshmem.c > +++ b/hw/misc/ivshmem.c > @@ -750,13 +750,13 @@ static void ivshmem_reset(DeviceState *d) > } > } > > -static int ivshmem_setup_interrupts(IVShmemState *s) > +static int ivshmem_setup_interrupts(IVShmemState *s, Error **errp) > { > /* allocate QEMU callback data for receiving interrupts */ > s->msi_vectors = g_malloc0(s->vectors * sizeof(MSIVector)); > > if (ivshmem_has_feature(s, IVSHMEM_MSI)) { > - if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1)) { > + if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1, errp)) { > return -1; > } > > @@ -897,8 +897,8 @@ static void ivshmem_common_realize(PCIDevice *dev, Error **errp) > qemu_chr_add_handlers(s->server_chr, ivshmem_can_receive, > ivshmem_read, NULL, s); > > - if (ivshmem_setup_interrupts(s) < 0) { > - error_setg(errp, "failed to initialize interrupts"); > + if (ivshmem_setup_interrupts(s, errp) < 0) { > + error_prepend(errp, "Failed to initialize interrupts: "); > return; > } > } > diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c > index bad43f4..72aad21 100644 > --- a/hw/net/e1000e.c > +++ b/hw/net/e1000e.c > @@ -292,7 +292,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..e421ebb 100644 > --- a/hw/net/rocker/rocker.c > +++ b/hw/net/rocker/rocker.c > @@ -1256,14 +1256,16 @@ static int rocker_msix_init(Rocker *r) > { > PCIDevice *dev = PCI_DEVICE(r); > int err; > + Error *local_err = NULL; > > 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, &local_err); > if (err) { > + error_report_err(local_err); > return err; > } > > diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c > index 90f6943..4824f8d 100644 > --- a/hw/net/vmxnet3.c > +++ b/hw/net/vmxnet3.c > @@ -2181,32 +2181,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) > { > @@ -2315,9 +2289,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); Uh, this is more than just a conversion to Error. Before, the code falls back to not using MSI-X on any error, with a warning. After, it falls back on ENOTSUP only, silently, and crashes on any other error. Such a change needs to be documented in the commit message, or be in a separate patch. I prefer separate patch. > > diff --git a/hw/pci/msix.c b/hw/pci/msix.c > index 0aadc0c..568c051 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 > > @@ -238,11 +239,29 @@ static void msix_mask_all(struct PCIDevice *dev, unsigned nentries) > } > } > > -/* Initialize the MSI-X structures */ > +/* Make PCI device @dev MSI-X capable > + * @nentries is the max number of MSI-X vectors that the device support. > + * @table_bar is the MemoryRegion that MSI-X table structure resides. > + * @table_bar_nr is number of base address register corresponding to @table_bar. > + * @table_offset indicates the offset that the MSI-X table structure starts with > + * in @table_bar. > + * @pba_bar is the MemoryRegion that the Pending Bit Array structure resides. > + * @pba_bar_nr is number of base address register corresponding to @pba_bar. > + * @pba_offset indicates the offset that the Pending Bit Array structure > + * starts with in @pba_bar. > + * Non-zero @cap_pos puts capability MSI-X at that offset in PCI config space. > + * @errp is for returning errors. > + * > + * Return 0 on success; set @errp and return -errno on error. > + * -ENOTSUP means lacking msi support for a msi-capable platform. > + * -EINVAL means capability overlap, happens when @cap_pos is non-zero, > + * also means a programming error, except device assignment, which can check > + * if a real HW is broken.*/ > 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 +269,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 +287,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; > } > @@ -304,7 +325,7 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries, > } > > int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries, > - uint8_t bar_nr) > + uint8_t bar_nr, Error **errp) > { > int ret; > char *name; > @@ -336,7 +357,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, errp); > if (ret) { > return ret; > } > diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c > index e968302..6d45025 100644 > --- a/hw/scsi/megasas.c > +++ b/hw/scsi/megasas.c > @@ -2349,16 +2349,32 @@ 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"); > + /* No instance_finalize method, need to free the resource here */ > + 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; > - } Before your patch, msix=on behaves just like msix=auto. Afterwards, msix=on fails when MSI-X can't be enabled. That's a good change, but it needs to be documented in the commit message, or be in a separate patch. I prefer separate patch. > 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); > + } Can you explain why you're moving this code? > > 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); > - } You're resolving the TODO. Good, but it needs to be documented in the commit message, or be in a separate patch. > } > > 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..2e6b9bc 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -1657,13 +1657,9 @@ 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); > + proxy->msix_bar, errp); > 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); > 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);
On 09/12/2016 09:47 PM, Markus Armbruster wrote: > Cao jin <caoj.fnst@cn.fujitsu.com> writes: > >> static void >> vmxnet3_cleanup_msix(VMXNET3State *s) >> { >> @@ -2315,9 +2289,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); > > Uh, this is more than just a conversion to Error. Before, the code > falls back to not using MSI-X on any error, with a warning. After, it > falls back on ENOTSUP only, silently, and crashes on any other error. > Such a change needs to be documented in the commit message, or be in a > separate patch. I prefer separate patch. > Dmitry has option that we should check the return value of msix_vector_use and prefer to keep init function, so I will withdraw this modification. >> 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); >> + } > > Can you explain why you're moving this code? > Sorry I forget to mention this: msi_init() uses xhci->numintrs, but there is value checking/correcting on xhci->numintrs, it should be done before using.
Cao jin <caoj.fnst@cn.fujitsu.com> writes: > On 09/12/2016 09:47 PM, Markus Armbruster wrote: >> Cao jin <caoj.fnst@cn.fujitsu.com> writes: [...] >>> 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); >>> + } >> >> Can you explain why you're moving this code? >> > > Sorry I forget to mention this: msi_init() uses xhci->numintrs, but > there is value checking/correcting on xhci->numintrs, it should be > done before using. If you do the move in a separate patch before this one, you can explain it in its commit message. Easier to review that way.
diff --git a/hw/block/nvme.c b/hw/block/nvme.c index cef3bb4..ae84dc7 100644 --- a/hw/block/nvme.c +++ b/hw/block/nvme.c @@ -829,6 +829,7 @@ static int nvme_init(PCIDevice *pci_dev) { NvmeCtrl *n = NVME(pci_dev); NvmeIdCtrl *id = &n->id_ctrl; + Error *err = NULL; int i; int64_t bs_size; @@ -870,7 +871,9 @@ static int nvme_init(PCIDevice *pci_dev) pci_register_bar(&n->parent_obj, 0, PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_64, &n->iomem); - msix_init_exclusive_bar(&n->parent_obj, n->num_queues, 4); + if (msix_init_exclusive_bar(&n->parent_obj, n->num_queues, 4, &err)) { + error_report_err(err); + } id->vid = cpu_to_le16(pci_get_word(pci_conf + PCI_VENDOR_ID)); id->ssvid = cpu_to_le16(pci_get_word(pci_conf + PCI_SUBSYSTEM_VENDOR_ID)); diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c index 40a2ebc..a1060ec 100644 --- a/hw/misc/ivshmem.c +++ b/hw/misc/ivshmem.c @@ -750,13 +750,13 @@ static void ivshmem_reset(DeviceState *d) } } -static int ivshmem_setup_interrupts(IVShmemState *s) +static int ivshmem_setup_interrupts(IVShmemState *s, Error **errp) { /* allocate QEMU callback data for receiving interrupts */ s->msi_vectors = g_malloc0(s->vectors * sizeof(MSIVector)); if (ivshmem_has_feature(s, IVSHMEM_MSI)) { - if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1)) { + if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1, errp)) { return -1; } @@ -897,8 +897,8 @@ static void ivshmem_common_realize(PCIDevice *dev, Error **errp) qemu_chr_add_handlers(s->server_chr, ivshmem_can_receive, ivshmem_read, NULL, s); - if (ivshmem_setup_interrupts(s) < 0) { - error_setg(errp, "failed to initialize interrupts"); + if (ivshmem_setup_interrupts(s, errp) < 0) { + error_prepend(errp, "Failed to initialize interrupts: "); return; } } diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c index bad43f4..72aad21 100644 --- a/hw/net/e1000e.c +++ b/hw/net/e1000e.c @@ -292,7 +292,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..e421ebb 100644 --- a/hw/net/rocker/rocker.c +++ b/hw/net/rocker/rocker.c @@ -1256,14 +1256,16 @@ static int rocker_msix_init(Rocker *r) { PCIDevice *dev = PCI_DEVICE(r); int err; + Error *local_err = NULL; 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, &local_err); if (err) { + error_report_err(local_err); return err; } diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c index 90f6943..4824f8d 100644 --- a/hw/net/vmxnet3.c +++ b/hw/net/vmxnet3.c @@ -2181,32 +2181,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) { @@ -2315,9 +2289,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 0aadc0c..568c051 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 @@ -238,11 +239,29 @@ static void msix_mask_all(struct PCIDevice *dev, unsigned nentries) } } -/* Initialize the MSI-X structures */ +/* Make PCI device @dev MSI-X capable + * @nentries is the max number of MSI-X vectors that the device support. + * @table_bar is the MemoryRegion that MSI-X table structure resides. + * @table_bar_nr is number of base address register corresponding to @table_bar. + * @table_offset indicates the offset that the MSI-X table structure starts with + * in @table_bar. + * @pba_bar is the MemoryRegion that the Pending Bit Array structure resides. + * @pba_bar_nr is number of base address register corresponding to @pba_bar. + * @pba_offset indicates the offset that the Pending Bit Array structure + * starts with in @pba_bar. + * Non-zero @cap_pos puts capability MSI-X at that offset in PCI config space. + * @errp is for returning errors. + * + * Return 0 on success; set @errp and return -errno on error. + * -ENOTSUP means lacking msi support for a msi-capable platform. + * -EINVAL means capability overlap, happens when @cap_pos is non-zero, + * also means a programming error, except device assignment, which can check + * if a real HW is broken.*/ 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 +269,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 +287,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; } @@ -304,7 +325,7 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries, } int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries, - uint8_t bar_nr) + uint8_t bar_nr, Error **errp) { int ret; char *name; @@ -336,7 +357,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, errp); if (ret) { return ret; } diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c index e968302..6d45025 100644 --- a/hw/scsi/megasas.c +++ b/hw/scsi/megasas.c @@ -2349,16 +2349,32 @@ 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"); + /* No instance_finalize method, need to free the resource here */ + 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..2e6b9bc 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -1657,13 +1657,9 @@ 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); + proxy->msix_bar, errp); 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); 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);
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 msi_init's failure, suppress the error report by passing NULL error object. 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/block/nvme.c | 5 +++- hw/misc/ivshmem.c | 8 +++--- hw/net/e1000e.c | 2 +- hw/net/rocker/rocker.c | 4 ++- hw/net/vmxnet3.c | 42 +++++++++-------------------- hw/pci/msix.c | 31 ++++++++++++++++++---- hw/scsi/megasas.c | 26 ++++++++++++++---- hw/usb/hcd-xhci.c | 71 ++++++++++++++++++++++++++++++-------------------- hw/vfio/pci.c | 7 +++-- hw/virtio/virtio-pci.c | 8 ++---- include/hw/pci/msix.h | 5 ++-- 11 files changed, 125 insertions(+), 84 deletions(-)