[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v7 03/10] pci: Convert msix_init() to Error and
|
From: |
Michael S. Tsirkin |
|
Subject: |
Re: [Qemu-devel] [PATCH v7 03/10] pci: Convert msix_init() to Error and fix callers to check it |
|
Date: |
Tue, 10 Jan 2017 21:21:31 +0200 |
On Tue, Jan 10, 2017 at 06:58:39PM +0100, Paolo Bonzini wrote:
>
>
> On 10/01/2017 18:54, Michael S. Tsirkin wrote:
> > On Mon, Nov 14, 2016 at 03:25:33PM +0800, 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>
> >> Reviewed-by: Hannes Reinecke <address@hidden>
> >> Signed-off-by: Cao jin <address@hidden>
> >
> > I'd rather add a new API. Once that is merged you can make device
> > changes avoiding a flag day. Merge this through individual trees. At the
> > end of the day we can drop the old API when it's no longer needed.
>
> I think that's the worst. We don't need another partial transition and
> this series is all but big. The main issue is that it was handled badly
> in the past, so people tend not to trust it.
Exactly.
> If anything, if there are independent patches in the series that can be
> merged through USB or SCSI trees, before this one, we can do that.
>
> Paolo
I wish but pci core changes seem to be patch 3.
> >> ---
> >> 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 | 8 ++++++--
> >> hw/virtio/virtio-pci.c | 11 +++++------
> >> include/hw/pci/msix.h | 5 +++--
> >> 11 files changed, 80 insertions(+), 30 deletions(-)
> >>
> >> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> >> index d479fd2..2d703c8 100644
> >> --- a/hw/block/nvme.c
> >> +++ b/hw/block/nvme.c
> >> @@ -831,6 +831,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;
> >> @@ -872,7 +873,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));
> >
> > Why don't you suppress this one too?
> >
> >> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> >> index 230e51b..ca6f804 100644
> >> --- a/hw/misc/ivshmem.c
> >> +++ b/hw/misc/ivshmem.c
> >> @@ -749,13 +749,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_fe_set_handlers(&s->server_chr, ivshmem_can_receive,
> >> ivshmem_read, NULL, s, NULL, true);
> >>
> >> - 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 4994e1c..74cbbef 100644
> >> --- a/hw/net/e1000e.c
> >> +++ b/hw/net/e1000e.c
> >> @@ -292,7 +292,11 @@ e1000e_init_msix(E1000EState *s)
> >> E1000E_MSIX_IDX, E1000E_MSIX_TABLE,
> >> &s->msix,
> >> E1000E_MSIX_IDX, E1000E_MSIX_PBA,
> >> - 0xA0);
> >> + 0xA0, NULL);
> >> +
> >> + /* Any error other than -ENOTSUP(board's MSI support is broken)
> >> + * is a programming error. Fall back to INTx silently on -ENOTSUP */
> >> + assert(!res || res == -ENOTSUP);
> >>
> >> if (res < 0) {
> >> trace_e1000e_msix_init_fail(res);
> >> diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
> >> index e9d215a..8f829f2 100644
> >> --- a/hw/net/rocker/rocker.c
> >> +++ b/hw/net/rocker/rocker.c
> >> @@ -1256,14 +1256,19 @@ 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);
> >> + /* Any error other than -ENOTSUP(board's MSI support is broken)
> >> + * is a programming error. */
> >> + assert(!err || err == -ENOTSUP);
> >> if (err) {
> >> + error_report_err(local_err);
> >> return err;
> >> }
> >>
> >> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> >> index 92f6af9..a433cc0 100644
> >> --- a/hw/net/vmxnet3.c
> >> +++ b/hw/net/vmxnet3.c
> >> @@ -2191,10 +2191,14 @@ vmxnet3_init_msix(VMXNET3State *s)
> >> VMXNET3_MSIX_BAR_IDX, VMXNET3_OFF_MSIX_TABLE,
> >> &s->msix_bar,
> >> VMXNET3_MSIX_BAR_IDX, VMXNET3_OFF_MSIX_PBA(s),
> >> - VMXNET3_MSIX_OFFSET(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 on -ENOTSUP */
> >> + assert(!res || res == -ENOTSUP);
> >>
> >> if (0 > res) {
> >> - VMW_WRPRN("Failed to initialize MSI-X, error %d", res);
> >> + VMW_WRPRN("Failed to initialize MSI-X, board's MSI support is
> >> broken");
> >> s->msix_used = false;
> >> } else {
> >> if (!vmxnet3_use_msix_vectors(s, VMXNET3_MAX_INTRS)) {
> >> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> >> index 0cee631..d03016f 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,10 +269,12 @@ 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;
> >> }
> >>
> >> if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1) {
> >> + error_setg(errp, "The number of MSI-X vectors is invalid");
> >> return -EINVAL;
> >> }
> >>
> >> @@ -266,10 +287,13 @@ int msix_init(struct PCIDevice *dev, unsigned short
> >> nentries,
> >> table_offset + table_size > memory_region_size(table_bar) ||
> >> pba_offset + pba_size > memory_region_size(pba_bar) ||
> >> (table_offset | pba_offset) & PCI_MSIX_FLAGS_BIRMASK) {
> >> + error_setg(errp, "table & pba overlap, or they don't fit in BARs,"
> >> + " or don't align");
> >> return -EINVAL;
> >> }
> >>
> >> - 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;
> >> }
> >> @@ -306,7 +330,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;
> >> @@ -338,7 +362,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 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 */
> >> + 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 0ace273..153b006 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..0bcfaad 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 *local_err = NULL;
> >>
> >> vdev->msix->pending = g_malloc0(BITS_TO_LONGS(vdev->msix->entries) *
> >> sizeof(unsigned long));
> >> @@ -1439,12 +1440,15 @@ 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,
> >> + &local_err);
> >> if (ret < 0) {
> >> if (ret == -ENOTSUP) {
> >> + error_report_err(local_err);
> >> return 0;
> >> }
> >> - error_setg(errp, "msix_init failed");
> >> +
> >> + error_propagate(errp, local_err);
> >> return ret;
> >> }
> >>
> >> 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);
> >> 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);
> >>
> >> --
> >> 2.1.0
> >>
> >>