| Message ID | 1462508442-9407-12-git-send-email-caoj.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/06/2016 07:20 AM, Cao jin wrote: > msi_init() reports errors with error_report(), which is wrong > when it's used in realize(). > > Fix by converting it to Error. > > Fix its callers to handle failure instead of ignoring it. > > For those callers who don`t handle the failure, it might happen: > when user want msi on, but he doesn`t get what he want because of > msi_init fails silently. > > cc: Gerd Hoffmann <kraxel@redhat.com> > cc: John Snow <jsnow@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> > --- > the affected device is modified in this way: > 1. intel hd audio: move msi_init() above, save the strength to free the > MemoryRegion when it fails. > 2. ich9 ahci: move msi_init() above, save the strenth to free the resource > allocated when calling achi_realize(). It doesn`t have msi property, so > msi_init failure leads to fall back to INTx silently. Just free the error > object > 3. vmxnet3: move msi_init() above. Remove the unecessary vmxnet3_init_msi(). > It doesn`t have msi property, so msi_init() failure leads to fall back to > INTx silently. Just free the error object. > 4. ioh3420/xio3130_downstream/xio3130_upstream: they are pcie components, msi > or msix is forced, catch error and report it right there. > 5. pci_bridge_dev: msi_init`s failure is fatal, follow the behaviour. > 6. megasas_scsi: move msi_init() above, save the strength to free the > MemoryRegion when it fails. > 7. mptsas: Move msi_init() above, save the strength to free the MemoryRegion > when it fails. > 8. pvscsi: it doesn`t have msi property, msi_init fail leads to fall back to > INTx silently. > 9. usb-xhci: move msi_init() above, save the strength to free the MemoryRegion > when it fails. > 10. vfio-pci: keep the previous behaviour, and just catch & report error. > > hw/audio/intel-hda.c | 18 +++++++++++++---- > hw/ide/ich.c | 15 +++++++++----- > hw/net/vmxnet3.c | 41 +++++++++++++++----------------------- > hw/pci-bridge/ioh3420.c | 4 +++- > hw/pci-bridge/pci_bridge_dev.c | 7 ++++--- > hw/pci-bridge/xio3130_downstream.c | 4 +++- > hw/pci-bridge/xio3130_upstream.c | 4 +++- > hw/pci/msi.c | 9 +++++---- > hw/scsi/megasas.c | 18 +++++++++++++---- > hw/scsi/mptsas.c | 20 ++++++++++++++----- > hw/scsi/vmw_pvscsi.c | 6 +++++- > hw/usb/hcd-xhci.c | 18 +++++++++++++---- > hw/vfio/pci.c | 6 ++++-- > include/hw/pci/msi.h | 3 ++- > 14 files changed, 112 insertions(+), 61 deletions(-) > > diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c > index 61362e5..0a46358 100644 > --- a/hw/audio/intel-hda.c > +++ b/hw/audio/intel-hda.c > @@ -1131,6 +1131,7 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp) > { > IntelHDAState *d = INTEL_HDA(pci); > uint8_t *conf = d->pci.config; > + Error *err = NULL; > > d->name = object_get_typename(OBJECT(d)); > > @@ -1139,13 +1140,22 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp) > /* HDCTL off 0x40 bit 0 selects signaling mode (1-HDA, 0 - Ac97) 18.1.19 */ > conf[0x40] = 0x01; > > + if (d->msi != ON_OFF_AUTO_OFF) { > + msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, 1, > + true, false, &err); > + if (err && d->msi == ON_OFF_AUTO_ON) { > + /* If user set msi=on, then device creation fail */ > + error_propagate(errp, err); > + return; The semantics now changed, old machines with msi=on on platforms with msi_nonbroken=false will fail now, right? Is this acceptable or we need a compat way to kepp the semantics for old machines? > + } else if (err && d->msi == ON_OFF_AUTO_AUTO) { > + /* If user doesn`t set it on, switch to non-msi variant silently */ > + error_free(err); > + } > + } > + > memory_region_init_io(&d->mmio, OBJECT(d), &intel_hda_mmio_ops, d, > "intel-hda", 0x4000); > pci_register_bar(&d->pci, 0, 0, &d->mmio); > - if (d->msi == ON_OFF_AUTO_AUTO || > - d->msi == ON_OFF_AUTO_ON) { > - msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, 1, true, false); > - } > > hda_codec_bus_init(DEVICE(pci), &d->codecs, sizeof(d->codecs), > intel_hda_response, intel_hda_xfer); > diff --git a/hw/ide/ich.c b/hw/ide/ich.c > index 0a13334..aec8262 100644 > --- a/hw/ide/ich.c > +++ b/hw/ide/ich.c > @@ -111,6 +111,16 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp) > int sata_cap_offset; > uint8_t *sata_cap; > d = ICH_AHCI(dev); > + Error *err = NULL; > + > + /* Although the AHCI 1.3 specification states that the first capability > + * should be PMCAP, the Intel ICH9 data sheet specifies that the ICH9 > + * AHCI device puts the MSI capability first, pointing to 0x80. */ > + msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false, &err); > + if (err) { > + /* Fall back to INTx silently */ > + error_free(err); > + } > > ahci_realize(&d->ahci, DEVICE(dev), pci_get_address_space(dev), 6); > > @@ -142,11 +152,6 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp) > pci_set_long(sata_cap + SATA_CAP_BAR, > (ICH9_IDP_BAR + 0x4) | (ICH9_IDP_INDEX_LOG2 << 4)); > d->ahci.idp_offset = ICH9_IDP_INDEX; > - > - /* Although the AHCI 1.3 specification states that the first capability > - * should be PMCAP, the Intel ICH9 data sheet specifies that the ICH9 > - * AHCI device puts the MSI capability first, pointing to 0x80. */ > - msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false); > } > > static void pci_ich9_uninit(PCIDevice *dev) > diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c > index 7a38e47..ab9a938 100644 > --- a/hw/net/vmxnet3.c > +++ b/hw/net/vmxnet3.c > @@ -2189,27 +2189,6 @@ vmxnet3_cleanup_msix(VMXNET3State *s) > } > } > > -#define VMXNET3_USE_64BIT (true) > -#define VMXNET3_PER_VECTOR_MASK (false) > - > -static bool > -vmxnet3_init_msi(VMXNET3State *s) > -{ > - PCIDevice *d = PCI_DEVICE(s); > - int res; > - > - res = msi_init(d, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS, > - VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK); > - if (0 > res) { > - VMW_WRPRN("Failed to initialize MSI, error %d", res); > - s->msi_used = false; > - } else { > - s->msi_used = true; > - } > - > - return s->msi_used; > -} > - > static void > vmxnet3_cleanup_msi(VMXNET3State *s) > { > @@ -2271,10 +2250,15 @@ static uint8_t *vmxnet3_device_serial_num(VMXNET3State *s) > return dsnp; > } > > + > +#define VMXNET3_USE_64BIT (true) > +#define VMXNET3_PER_VECTOR_MASK (false) > + > static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp) > { > DeviceState *dev = DEVICE(pci_dev); > VMXNET3State *s = VMXNET3(pci_dev); > + Error *err = NULL; > > VMW_CBPRN("Starting init..."); > > @@ -2298,12 +2282,19 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp) > /* Interrupt pin A */ > pci_dev->config[PCI_INTERRUPT_PIN] = 0x01; > > - if (!vmxnet3_init_msix(s)) { > - VMW_WRPRN("Failed to initialize MSI-X, configuration is inconsistent."); > + msi_init(pci_dev, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS, > + VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK, &err); > + if (err) { > + /* Fall back to INTx silently */ > + VMW_WRPRN("Failed to initialize MSI: %s", error_get_pretty(err)); > + error_free(err); > + s->msi_used = false; > + } else { > + s->msi_used = true; > } > > - if (!vmxnet3_init_msi(s)) { > - VMW_WRPRN("Failed to initialize MSI, configuration is inconsistent."); > + if (!vmxnet3_init_msix(s)) { > + VMW_WRPRN("Failed to initialize MSI-X, configuration is inconsistent."); > } > > vmxnet3_net_init(s); > diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c > index b4a7806..d752e62 100644 > --- a/hw/pci-bridge/ioh3420.c > +++ b/hw/pci-bridge/ioh3420.c > @@ -97,6 +97,7 @@ static int ioh3420_initfn(PCIDevice *d) > PCIEPort *p = PCIE_PORT(d); > PCIESlot *s = PCIE_SLOT(d); > int rc; > + Error *err = NULL; > > pci_bridge_initfn(d, TYPE_PCIE_BUS); > pcie_port_init_reg(d); > @@ -109,8 +110,9 @@ static int ioh3420_initfn(PCIDevice *d) > > rc = msi_init(d, IOH_EP_MSI_OFFSET, IOH_EP_MSI_NR_VECTOR, > IOH_EP_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_64BIT, > - IOH_EP_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT); > + IOH_EP_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT, &err); > if (rc < 0) { > + error_report_err(err); > goto err_bridge; > } OK > > diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c > index 9e31f0e..af71c98 100644 > --- a/hw/pci-bridge/pci_bridge_dev.c > +++ b/hw/pci-bridge/pci_bridge_dev.c > @@ -53,6 +53,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev) > PCIBridge *br = PCI_BRIDGE(dev); > PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev); > int err; > + Error *local_err = NULL; > > pci_bridge_initfn(dev, TYPE_PCI_BUS); > > @@ -74,11 +75,11 @@ static int pci_bridge_dev_initfn(PCIDevice *dev) > goto slotid_error; > } > > - if ((bridge_dev->msi == ON_OFF_AUTO_AUTO || > - bridge_dev->msi == ON_OFF_AUTO_ON) && > + if (bridge_dev->msi != ON_OFF_AUTO_OFF && So we made the msi property OnOffAuto, but we don't make a difference between ON and Auto? > msi_nonbroken) { > - err = msi_init(dev, 0, 1, true, true); > + err = msi_init(dev, 0, 1, true, true, &local_err); > if (err < 0) { > + error_report_err(local_err); > goto msi_error; > } > } > diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c > index e6d653d..0982801 100644 > --- a/hw/pci-bridge/xio3130_downstream.c > +++ b/hw/pci-bridge/xio3130_downstream.c > @@ -60,14 +60,16 @@ static int xio3130_downstream_initfn(PCIDevice *d) > PCIEPort *p = PCIE_PORT(d); > PCIESlot *s = PCIE_SLOT(d); > int rc; > + Error *err = NULL; > > pci_bridge_initfn(d, TYPE_PCIE_BUS); > pcie_port_init_reg(d); > > rc = msi_init(d, XIO3130_MSI_OFFSET, XIO3130_MSI_NR_VECTOR, > XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_64BIT, > - XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT); > + XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT, &err); > if (rc < 0) { > + error_report_err(err); > goto err_bridge; > } > OK > diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c > index d976844..1d2c597 100644 > --- a/hw/pci-bridge/xio3130_upstream.c > +++ b/hw/pci-bridge/xio3130_upstream.c > @@ -56,14 +56,16 @@ static int xio3130_upstream_initfn(PCIDevice *d) > { > PCIEPort *p = PCIE_PORT(d); > int rc; > + Error *err = NULL; > > pci_bridge_initfn(d, TYPE_PCIE_BUS); > pcie_port_init_reg(d); > > rc = msi_init(d, XIO3130_MSI_OFFSET, XIO3130_MSI_NR_VECTOR, > XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_64BIT, > - XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT); > + XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT, &err); > if (rc < 0) { > + error_report_err(err); > goto err_bridge; > } > OK > diff --git a/hw/pci/msi.c b/hw/pci/msi.c > index dd9d957..662be56 100644 > --- a/hw/pci/msi.c > +++ b/hw/pci/msi.c > @@ -178,14 +178,13 @@ bool msi_enabled(const PCIDevice *dev) > * set @errp and return -errno on error. > * > * -ENOTSUP means lacking msi support for a msi-capable platform. > - * -ENOSPC means running out of PCI config space, happens when @offset is 0, > - * which means a programming error. > * -EINVAL means capability overlap, happens when @offset is non-zero, > * also means a programming error, except device assignment, which can check > * if a real HW is broken. > */ > int msi_init(struct PCIDevice *dev, uint8_t offset, > - unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask) > + unsigned int nr_vectors, bool msi64bit, > + bool msi_per_vector_mask, Error **errp) > { > unsigned int vectors_order; > uint16_t flags; > @@ -193,6 +192,7 @@ int msi_init(struct PCIDevice *dev, uint8_t offset, > int config_offset; > > if (!msi_nonbroken) { > + error_setg(errp, "MSI is not supported by interrupt controller"); > return -ENOTSUP; > } > > @@ -216,7 +216,8 @@ int msi_init(struct PCIDevice *dev, uint8_t offset, > } > > cap_size = msi_cap_sizeof(flags); > - config_offset = pci_add_capability(dev, PCI_CAP_ID_MSI, offset, cap_size); > + config_offset = pci_add_capability2(dev, PCI_CAP_ID_MSI, offset, > + cap_size, errp); > if (config_offset < 0) { > return config_offset; > } OK > diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c > index e71a28b..3585a6a 100644 > --- a/hw/scsi/megasas.c > +++ b/hw/scsi/megasas.c > @@ -2330,6 +2330,7 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp) > MegasasBaseClass *b = MEGASAS_DEVICE_GET_CLASS(s); > uint8_t *pci_conf; > int i, bar_type; > + Error *err = NULL; > > pci_conf = dev->config; > > @@ -2338,6 +2339,19 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp) > /* Interrupt pin 1 */ > pci_conf[PCI_INTERRUPT_PIN] = 0x01; > > + if (megasas_use_msi(s)) { > + msi_init(dev, 0x50, 1, true, false, &err); > + if (err && s->msi == ON_OFF_AUTO_ON) { > + /* If user set msi=on, then device creation fail */ > + s->msi = ON_OFF_AUTO_OFF; > + error_propagate(errp, err); > + return; The same question regarding the change of semantics. > + } else if (err && s->msi == ON_OFF_AUTO_AUTO) { > + /* If user doesn`t set it on, switch to non-msi variant silently */ > + error_free(err); > + } > + } > + > memory_region_init_io(&s->mmio_io, OBJECT(s), &megasas_mmio_ops, s, > "megasas-mmio", 0x4000); > memory_region_init_io(&s->port_io, OBJECT(s), &megasas_port_ops, s, > @@ -2345,10 +2359,6 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp) > memory_region_init_io(&s->queue_io, OBJECT(s), &megasas_queue_ops, s, > "megasas-queue", 0x40000); > > - if (megasas_use_msi(s) && > - msi_init(dev, 0x50, 1, true, false) < 0) { > - s->msi = ON_OFF_AUTO_OFF; > - } > if (megasas_use_msix(s) && > msix_init(dev, 15, &s->mmio_io, b->mmio_bar, 0x2000, > &s->mmio_io, b->mmio_bar, 0x3800, 0x68)) { > diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c > index afee576..aaee687 100644 > --- a/hw/scsi/mptsas.c > +++ b/hw/scsi/mptsas.c > @@ -1274,10 +1274,25 @@ static void mptsas_scsi_realize(PCIDevice *dev, Error **errp) > { > DeviceState *d = DEVICE(dev); > MPTSASState *s = MPT_SAS(dev); > + Error *err = NULL; > > dev->config[PCI_LATENCY_TIMER] = 0; > dev->config[PCI_INTERRUPT_PIN] = 0x01; > > + if (s->msi != ON_OFF_AUTO_OFF) { > + msi_init(dev, 0, 1, true, false, &err); > + if (err && s->msi == ON_OFF_AUTO_ON) { > + /* If user set msi=on, then device creation fail */ > + error_propagate(errp, err); > + s->msi_in_use = false; Same here > + return; > + } else if (err && s->msi == ON_OFF_AUTO_AUTO) { > + /* If user doesn`t set it on, switch to non-msi variant silently */ > + error_free(err); > + s->msi_in_use = false; > + } > + } > + > memory_region_init_io(&s->mmio_io, OBJECT(s), &mptsas_mmio_ops, s, > "mptsas-mmio", 0x4000); > memory_region_init_io(&s->port_io, OBJECT(s), &mptsas_port_ops, s, > @@ -1285,11 +1300,6 @@ static void mptsas_scsi_realize(PCIDevice *dev, Error **errp) > memory_region_init_io(&s->diag_io, OBJECT(s), &mptsas_diag_ops, s, > "mptsas-diag", 0x10000); > > - if ((s->msi == ON_OFF_AUTO_AUTO || s->msi == ON_OFF_AUTO_ON) && > - msi_init(dev, 0, 1, true, false) >= 0) { > - s->msi_in_use = true; > - } > - > pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &s->port_io); > pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY | > PCI_BASE_ADDRESS_MEM_TYPE_32, &s->mmio_io); > diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c > index 4ce3581..2d38d6c 100644 > --- a/hw/scsi/vmw_pvscsi.c > +++ b/hw/scsi/vmw_pvscsi.c > @@ -1043,12 +1043,16 @@ static void > pvscsi_init_msi(PVSCSIState *s) > { > int res; > + Error *err = NULL; > PCIDevice *d = PCI_DEVICE(s); > > res = msi_init(d, PVSCSI_MSI_OFFSET(s), PVSCSI_MSIX_NUM_VECTORS, > - PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK); > + PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK, &err); > if (res < 0) { > trace_pvscsi_init_msi_fail(res); > + error_append_hint(&err, "MSI capability fail to init," > + " will use INTx intead\n"); > + error_report_err(err); > s->msi_used = false; > } else { > s->msi_used = true; > diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c > index 6d70289..f17f242 100644 > --- a/hw/usb/hcd-xhci.c > +++ b/hw/usb/hcd-xhci.c > @@ -3578,6 +3578,7 @@ static void usb_xhci_init(XHCIState *xhci) > static void usb_xhci_realize(struct PCIDevice *dev, Error **errp) > { > int i, ret; > + Error *err = NULL; > > XHCIState *xhci = XHCI(dev); > > @@ -3588,6 +3589,19 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp) > > usb_xhci_init(xhci); > > + if (xhci->msi != ON_OFF_AUTO_OFF) { > + ret = msi_init(dev, 0x70, xhci->numintrs, true, false, &err); > + if (ret < 0 && xhci->msi == ON_OFF_AUTO_ON) { > + /* If user set msi=on, then device creation fail */ > + error_propagate(errp, err); > + return; > + } > + else if (ret < 0 && xhci->msi == ON_OFF_AUTO_AUTO) { > + /* If user doesn`t set it on, switch to non-msi variant silently */ > + error_free(err); > + } > + } > + > if (xhci->numintrs > MAXINTRS) { > xhci->numintrs = MAXINTRS; > } > @@ -3645,10 +3659,6 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp) > assert(ret >= 0); > } > > - if (xhci->msi == ON_OFF_AUTO_ON || > - xhci->msi == ON_OFF_AUTO_AUTO) { > - msi_init(dev, 0x70, xhci->numintrs, true, false); > - } > if (xhci->msix == ON_OFF_AUTO_ON || > xhci->msix == ON_OFF_AUTO_AUTO) { > msix_init(dev, xhci->numintrs, > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index d091d8c..55ceb67 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -1171,6 +1171,7 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos) > uint16_t ctrl; > bool msi_64bit, msi_maskbit; > int ret, entries; > + Error *err = NULL; > > if (pread(vdev->vbasedev.fd, &ctrl, sizeof(ctrl), > vdev->config_offset + pos + PCI_CAP_FLAGS) != sizeof(ctrl)) { > @@ -1184,12 +1185,13 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos) > > trace_vfio_msi_setup(vdev->vbasedev.name, pos); > > - ret = msi_init(&vdev->pdev, pos, entries, msi_64bit, msi_maskbit); > + ret = msi_init(&vdev->pdev, pos, entries, msi_64bit, msi_maskbit, &err); > if (ret < 0) { > if (ret == -ENOTSUP) { > return 0; > } > - error_report("vfio: msi_init failed"); > + error_prepend(&err, "vfio: msi_init failed: "); > + error_report_err(err); > return ret; > } OK > vdev->msi_cap_size = 0xa + (msi_maskbit ? 0xa : 0) + (msi_64bit ? 0x4 : 0); > diff --git a/include/hw/pci/msi.h b/include/hw/pci/msi.h > index 8124908..4837bcf 100644 > --- a/include/hw/pci/msi.h > +++ b/include/hw/pci/msi.h > @@ -35,7 +35,8 @@ void msi_set_message(PCIDevice *dev, MSIMessage msg); > MSIMessage msi_get_message(PCIDevice *dev, unsigned int vector); > bool msi_enabled(const PCIDevice *dev); > int msi_init(struct PCIDevice *dev, uint8_t offset, > - unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask); > + unsigned int nr_vectors, bool msi64bit, > + bool msi_per_vector_mask, Error **errp); > void msi_uninit(struct PCIDevice *dev); > void msi_reset(PCIDevice *dev); > void msi_notify(PCIDevice *dev, unsigned int vector); > Thanks, Marcel
On 05/15/2016 09:41 PM, Marcel Apfelbaum wrote: > >> >> diff --git a/hw/pci-bridge/pci_bridge_dev.c >> b/hw/pci-bridge/pci_bridge_dev.c >> index 9e31f0e..af71c98 100644 >> --- a/hw/pci-bridge/pci_bridge_dev.c >> +++ b/hw/pci-bridge/pci_bridge_dev.c >> @@ -53,6 +53,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev) >> PCIBridge *br = PCI_BRIDGE(dev); >> PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev); >> int err; >> + Error *local_err = NULL; >> >> pci_bridge_initfn(dev, TYPE_PCI_BUS); >> >> @@ -74,11 +75,11 @@ static int pci_bridge_dev_initfn(PCIDevice *dev) >> goto slotid_error; >> } >> >> - if ((bridge_dev->msi == ON_OFF_AUTO_AUTO || >> - bridge_dev->msi == ON_OFF_AUTO_ON) && >> + if (bridge_dev->msi != ON_OFF_AUTO_OFF && > > So we made the msi property OnOffAuto, but we don't make a difference > between ON and Auto? > That is why I am not quite sure about this device, msi has a relation with shpc. From its previous behaviour, it can be seen that it don`t treat 'on' as 'auto'.(I am not sure why it is different with others) Its previous behaviour treat msi_init`s failure as fatal, and lead to device creation fail. According to Markus`s comments(if I understand him right), this device has no non-msi variants. I think my patch follows the previous behaviour. And also according to function comments: /* MSI is not applicable without SHPC */ which means device either has both, or neither(if I understand it right), so that is why I don`t make a difference >> msi_nonbroken) { >> - err = msi_init(dev, 0, 1, true, true); >> + err = msi_init(dev, 0, 1, true, true, &local_err); >> if (err < 0) { >> + error_report_err(local_err); >> goto msi_error; >> } >> }
On 05/17/2016 01:08 PM, Cao jin wrote: > > > On 05/15/2016 09:41 PM, Marcel Apfelbaum wrote: > >> >>> >>> diff --git a/hw/pci-bridge/pci_bridge_dev.c >>> b/hw/pci-bridge/pci_bridge_dev.c >>> index 9e31f0e..af71c98 100644 >>> --- a/hw/pci-bridge/pci_bridge_dev.c >>> +++ b/hw/pci-bridge/pci_bridge_dev.c >>> @@ -53,6 +53,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev) >>> PCIBridge *br = PCI_BRIDGE(dev); >>> PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev); >>> int err; >>> + Error *local_err = NULL; >>> >>> pci_bridge_initfn(dev, TYPE_PCI_BUS); >>> >>> @@ -74,11 +75,11 @@ static int pci_bridge_dev_initfn(PCIDevice *dev) >>> goto slotid_error; >>> } >>> >>> - if ((bridge_dev->msi == ON_OFF_AUTO_AUTO || >>> - bridge_dev->msi == ON_OFF_AUTO_ON) && >>> + if (bridge_dev->msi != ON_OFF_AUTO_OFF && >> >> So we made the msi property OnOffAuto, but we don't make a difference >> between ON and Auto? >> > > That is why I am not quite sure about this device, msi has a relation with shpc. > > From its previous behaviour, it can be seen that it don`t treat 'on' as 'auto'.(I am not sure why it is different with others) > > Its previous behaviour treat msi_init`s failure as fatal, and lead to device creation fail. According to Markus`s comments(if I understand him right), this device has no non-msi variants. Actually it has. The bridge needs msi for the SHPC controller, as you said. If you look into shpc_interrupt_update function (hw/pci/shpc.c) you can see it can fall back to legacy interrupts. The only complication here is that msi is needed only if shpc is present. So maybe having msi=on/off/auto is OK. msi=auto => if shpc not present or msi broken results in msi = off, else msi = on msi=on => fails if shpc present and msi broken msi=off => use legacy interrupts if shpc is present Basically the msi flag has no meaning if shpc not present. Thanks, Marcel I think my > patch follows the previous behaviour. > > And also according to function comments: > /* MSI is not applicable without SHPC */ > which means device either has both, or neither(if I understand it right), so that is why I don`t make a difference > >>> msi_nonbroken) { >>> - err = msi_init(dev, 0, 1, true, true); >>> + err = msi_init(dev, 0, 1, true, true, &local_err); >>> if (err < 0) { >>> + error_report_err(local_err); >>> goto msi_error; >>> } >>> } > >
On 05/23/2016 06:06 PM, Marcel Apfelbaum wrote: > On 05/17/2016 01:08 PM, Cao jin wrote: >> >> >> On 05/15/2016 09:41 PM, Marcel Apfelbaum wrote: >> >> >> That is why I am not quite sure about this device, msi has a relation >> with shpc. >> >> From its previous behaviour, it can be seen that it don`t treat 'on' >> as 'auto'.(I am not sure why it is different with others) >> >> Its previous behaviour treat msi_init`s failure as fatal, and lead to >> device creation fail. According to Markus`s comments(if I understand >> him right), this device has no non-msi variants. > > Actually it has. The bridge needs msi for the SHPC controller, as you said. > If you look into shpc_interrupt_update function (hw/pci/shpc.c) you can > see it can fall > back to legacy interrupts. > > The only complication here is that msi is needed only if shpc is present. > So maybe having msi=on/off/auto is OK. > > msi=auto => if shpc not present or msi broken results in msi = off, else > msi = on > msi=on => fails if shpc present and msi broken > msi=off => use legacy interrupts if shpc is present > > > Basically the msi flag has no meaning if shpc not present. > Thanks for the hint, v6 is on the way:)
diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c index 61362e5..0a46358 100644 --- a/hw/audio/intel-hda.c +++ b/hw/audio/intel-hda.c @@ -1131,6 +1131,7 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp) { IntelHDAState *d = INTEL_HDA(pci); uint8_t *conf = d->pci.config; + Error *err = NULL; d->name = object_get_typename(OBJECT(d)); @@ -1139,13 +1140,22 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp) /* HDCTL off 0x40 bit 0 selects signaling mode (1-HDA, 0 - Ac97) 18.1.19 */ conf[0x40] = 0x01; + if (d->msi != ON_OFF_AUTO_OFF) { + msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, 1, + true, false, &err); + if (err && d->msi == ON_OFF_AUTO_ON) { + /* If user set msi=on, then device creation fail */ + error_propagate(errp, err); + return; + } else if (err && d->msi == ON_OFF_AUTO_AUTO) { + /* If user doesn`t set it on, switch to non-msi variant silently */ + error_free(err); + } + } + memory_region_init_io(&d->mmio, OBJECT(d), &intel_hda_mmio_ops, d, "intel-hda", 0x4000); pci_register_bar(&d->pci, 0, 0, &d->mmio); - if (d->msi == ON_OFF_AUTO_AUTO || - d->msi == ON_OFF_AUTO_ON) { - msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, 1, true, false); - } hda_codec_bus_init(DEVICE(pci), &d->codecs, sizeof(d->codecs), intel_hda_response, intel_hda_xfer); diff --git a/hw/ide/ich.c b/hw/ide/ich.c index 0a13334..aec8262 100644 --- a/hw/ide/ich.c +++ b/hw/ide/ich.c @@ -111,6 +111,16 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp) int sata_cap_offset; uint8_t *sata_cap; d = ICH_AHCI(dev); + Error *err = NULL; + + /* Although the AHCI 1.3 specification states that the first capability + * should be PMCAP, the Intel ICH9 data sheet specifies that the ICH9 + * AHCI device puts the MSI capability first, pointing to 0x80. */ + msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false, &err); + if (err) { + /* Fall back to INTx silently */ + error_free(err); + } ahci_realize(&d->ahci, DEVICE(dev), pci_get_address_space(dev), 6); @@ -142,11 +152,6 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp) pci_set_long(sata_cap + SATA_CAP_BAR, (ICH9_IDP_BAR + 0x4) | (ICH9_IDP_INDEX_LOG2 << 4)); d->ahci.idp_offset = ICH9_IDP_INDEX; - - /* Although the AHCI 1.3 specification states that the first capability - * should be PMCAP, the Intel ICH9 data sheet specifies that the ICH9 - * AHCI device puts the MSI capability first, pointing to 0x80. */ - msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false); } static void pci_ich9_uninit(PCIDevice *dev) diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c index 7a38e47..ab9a938 100644 --- a/hw/net/vmxnet3.c +++ b/hw/net/vmxnet3.c @@ -2189,27 +2189,6 @@ vmxnet3_cleanup_msix(VMXNET3State *s) } } -#define VMXNET3_USE_64BIT (true) -#define VMXNET3_PER_VECTOR_MASK (false) - -static bool -vmxnet3_init_msi(VMXNET3State *s) -{ - PCIDevice *d = PCI_DEVICE(s); - int res; - - res = msi_init(d, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS, - VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK); - if (0 > res) { - VMW_WRPRN("Failed to initialize MSI, error %d", res); - s->msi_used = false; - } else { - s->msi_used = true; - } - - return s->msi_used; -} - static void vmxnet3_cleanup_msi(VMXNET3State *s) { @@ -2271,10 +2250,15 @@ static uint8_t *vmxnet3_device_serial_num(VMXNET3State *s) return dsnp; } + +#define VMXNET3_USE_64BIT (true) +#define VMXNET3_PER_VECTOR_MASK (false) + static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp) { DeviceState *dev = DEVICE(pci_dev); VMXNET3State *s = VMXNET3(pci_dev); + Error *err = NULL; VMW_CBPRN("Starting init..."); @@ -2298,12 +2282,19 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp) /* Interrupt pin A */ pci_dev->config[PCI_INTERRUPT_PIN] = 0x01; - if (!vmxnet3_init_msix(s)) { - VMW_WRPRN("Failed to initialize MSI-X, configuration is inconsistent."); + msi_init(pci_dev, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS, + VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK, &err); + if (err) { + /* Fall back to INTx silently */ + VMW_WRPRN("Failed to initialize MSI: %s", error_get_pretty(err)); + error_free(err); + s->msi_used = false; + } else { + s->msi_used = true; } - if (!vmxnet3_init_msi(s)) { - VMW_WRPRN("Failed to initialize MSI, configuration is inconsistent."); + if (!vmxnet3_init_msix(s)) { + VMW_WRPRN("Failed to initialize MSI-X, configuration is inconsistent."); } vmxnet3_net_init(s); diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c index b4a7806..d752e62 100644 --- a/hw/pci-bridge/ioh3420.c +++ b/hw/pci-bridge/ioh3420.c @@ -97,6 +97,7 @@ static int ioh3420_initfn(PCIDevice *d) PCIEPort *p = PCIE_PORT(d); PCIESlot *s = PCIE_SLOT(d); int rc; + Error *err = NULL; pci_bridge_initfn(d, TYPE_PCIE_BUS); pcie_port_init_reg(d); @@ -109,8 +110,9 @@ static int ioh3420_initfn(PCIDevice *d) rc = msi_init(d, IOH_EP_MSI_OFFSET, IOH_EP_MSI_NR_VECTOR, IOH_EP_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_64BIT, - IOH_EP_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT); + IOH_EP_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT, &err); if (rc < 0) { + error_report_err(err); goto err_bridge; } diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c index 9e31f0e..af71c98 100644 --- a/hw/pci-bridge/pci_bridge_dev.c +++ b/hw/pci-bridge/pci_bridge_dev.c @@ -53,6 +53,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev) PCIBridge *br = PCI_BRIDGE(dev); PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev); int err; + Error *local_err = NULL; pci_bridge_initfn(dev, TYPE_PCI_BUS); @@ -74,11 +75,11 @@ static int pci_bridge_dev_initfn(PCIDevice *dev) goto slotid_error; } - if ((bridge_dev->msi == ON_OFF_AUTO_AUTO || - bridge_dev->msi == ON_OFF_AUTO_ON) && + if (bridge_dev->msi != ON_OFF_AUTO_OFF && msi_nonbroken) { - err = msi_init(dev, 0, 1, true, true); + err = msi_init(dev, 0, 1, true, true, &local_err); if (err < 0) { + error_report_err(local_err); goto msi_error; } } diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c index e6d653d..0982801 100644 --- a/hw/pci-bridge/xio3130_downstream.c +++ b/hw/pci-bridge/xio3130_downstream.c @@ -60,14 +60,16 @@ static int xio3130_downstream_initfn(PCIDevice *d) PCIEPort *p = PCIE_PORT(d); PCIESlot *s = PCIE_SLOT(d); int rc; + Error *err = NULL; pci_bridge_initfn(d, TYPE_PCIE_BUS); pcie_port_init_reg(d); rc = msi_init(d, XIO3130_MSI_OFFSET, XIO3130_MSI_NR_VECTOR, XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_64BIT, - XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT); + XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT, &err); if (rc < 0) { + error_report_err(err); goto err_bridge; } diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c index d976844..1d2c597 100644 --- a/hw/pci-bridge/xio3130_upstream.c +++ b/hw/pci-bridge/xio3130_upstream.c @@ -56,14 +56,16 @@ static int xio3130_upstream_initfn(PCIDevice *d) { PCIEPort *p = PCIE_PORT(d); int rc; + Error *err = NULL; pci_bridge_initfn(d, TYPE_PCIE_BUS); pcie_port_init_reg(d); rc = msi_init(d, XIO3130_MSI_OFFSET, XIO3130_MSI_NR_VECTOR, XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_64BIT, - XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT); + XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT, &err); if (rc < 0) { + error_report_err(err); goto err_bridge; } diff --git a/hw/pci/msi.c b/hw/pci/msi.c index dd9d957..662be56 100644 --- a/hw/pci/msi.c +++ b/hw/pci/msi.c @@ -178,14 +178,13 @@ bool msi_enabled(const PCIDevice *dev) * set @errp and return -errno on error. * * -ENOTSUP means lacking msi support for a msi-capable platform. - * -ENOSPC means running out of PCI config space, happens when @offset is 0, - * which means a programming error. * -EINVAL means capability overlap, happens when @offset is non-zero, * also means a programming error, except device assignment, which can check * if a real HW is broken. */ int msi_init(struct PCIDevice *dev, uint8_t offset, - unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask) + unsigned int nr_vectors, bool msi64bit, + bool msi_per_vector_mask, Error **errp) { unsigned int vectors_order; uint16_t flags; @@ -193,6 +192,7 @@ int msi_init(struct PCIDevice *dev, uint8_t offset, int config_offset; if (!msi_nonbroken) { + error_setg(errp, "MSI is not supported by interrupt controller"); return -ENOTSUP; } @@ -216,7 +216,8 @@ int msi_init(struct PCIDevice *dev, uint8_t offset, } cap_size = msi_cap_sizeof(flags); - config_offset = pci_add_capability(dev, PCI_CAP_ID_MSI, offset, cap_size); + config_offset = pci_add_capability2(dev, PCI_CAP_ID_MSI, offset, + cap_size, errp); if (config_offset < 0) { return config_offset; } diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c index e71a28b..3585a6a 100644 --- a/hw/scsi/megasas.c +++ b/hw/scsi/megasas.c @@ -2330,6 +2330,7 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp) MegasasBaseClass *b = MEGASAS_DEVICE_GET_CLASS(s); uint8_t *pci_conf; int i, bar_type; + Error *err = NULL; pci_conf = dev->config; @@ -2338,6 +2339,19 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp) /* Interrupt pin 1 */ pci_conf[PCI_INTERRUPT_PIN] = 0x01; + if (megasas_use_msi(s)) { + msi_init(dev, 0x50, 1, true, false, &err); + if (err && s->msi == ON_OFF_AUTO_ON) { + /* If user set msi=on, then device creation fail */ + s->msi = ON_OFF_AUTO_OFF; + error_propagate(errp, err); + return; + } else if (err && s->msi == ON_OFF_AUTO_AUTO) { + /* If user doesn`t set it on, switch to non-msi variant silently */ + error_free(err); + } + } + memory_region_init_io(&s->mmio_io, OBJECT(s), &megasas_mmio_ops, s, "megasas-mmio", 0x4000); memory_region_init_io(&s->port_io, OBJECT(s), &megasas_port_ops, s, @@ -2345,10 +2359,6 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp) memory_region_init_io(&s->queue_io, OBJECT(s), &megasas_queue_ops, s, "megasas-queue", 0x40000); - if (megasas_use_msi(s) && - msi_init(dev, 0x50, 1, true, false) < 0) { - s->msi = ON_OFF_AUTO_OFF; - } if (megasas_use_msix(s) && msix_init(dev, 15, &s->mmio_io, b->mmio_bar, 0x2000, &s->mmio_io, b->mmio_bar, 0x3800, 0x68)) { diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c index afee576..aaee687 100644 --- a/hw/scsi/mptsas.c +++ b/hw/scsi/mptsas.c @@ -1274,10 +1274,25 @@ static void mptsas_scsi_realize(PCIDevice *dev, Error **errp) { DeviceState *d = DEVICE(dev); MPTSASState *s = MPT_SAS(dev); + Error *err = NULL; dev->config[PCI_LATENCY_TIMER] = 0; dev->config[PCI_INTERRUPT_PIN] = 0x01; + if (s->msi != ON_OFF_AUTO_OFF) { + msi_init(dev, 0, 1, true, false, &err); + if (err && s->msi == ON_OFF_AUTO_ON) { + /* If user set msi=on, then device creation fail */ + error_propagate(errp, err); + s->msi_in_use = false; + return; + } else if (err && s->msi == ON_OFF_AUTO_AUTO) { + /* If user doesn`t set it on, switch to non-msi variant silently */ + error_free(err); + s->msi_in_use = false; + } + } + memory_region_init_io(&s->mmio_io, OBJECT(s), &mptsas_mmio_ops, s, "mptsas-mmio", 0x4000); memory_region_init_io(&s->port_io, OBJECT(s), &mptsas_port_ops, s, @@ -1285,11 +1300,6 @@ static void mptsas_scsi_realize(PCIDevice *dev, Error **errp) memory_region_init_io(&s->diag_io, OBJECT(s), &mptsas_diag_ops, s, "mptsas-diag", 0x10000); - if ((s->msi == ON_OFF_AUTO_AUTO || s->msi == ON_OFF_AUTO_ON) && - msi_init(dev, 0, 1, true, false) >= 0) { - s->msi_in_use = true; - } - pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &s->port_io); pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32, &s->mmio_io); diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c index 4ce3581..2d38d6c 100644 --- a/hw/scsi/vmw_pvscsi.c +++ b/hw/scsi/vmw_pvscsi.c @@ -1043,12 +1043,16 @@ static void pvscsi_init_msi(PVSCSIState *s) { int res; + Error *err = NULL; PCIDevice *d = PCI_DEVICE(s); res = msi_init(d, PVSCSI_MSI_OFFSET(s), PVSCSI_MSIX_NUM_VECTORS, - PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK); + PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK, &err); if (res < 0) { trace_pvscsi_init_msi_fail(res); + error_append_hint(&err, "MSI capability fail to init," + " will use INTx intead\n"); + error_report_err(err); s->msi_used = false; } else { s->msi_used = true; diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c index 6d70289..f17f242 100644 --- a/hw/usb/hcd-xhci.c +++ b/hw/usb/hcd-xhci.c @@ -3578,6 +3578,7 @@ static void usb_xhci_init(XHCIState *xhci) static void usb_xhci_realize(struct PCIDevice *dev, Error **errp) { int i, ret; + Error *err = NULL; XHCIState *xhci = XHCI(dev); @@ -3588,6 +3589,19 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp) usb_xhci_init(xhci); + if (xhci->msi != ON_OFF_AUTO_OFF) { + ret = msi_init(dev, 0x70, xhci->numintrs, true, false, &err); + if (ret < 0 && xhci->msi == ON_OFF_AUTO_ON) { + /* If user set msi=on, then device creation fail */ + error_propagate(errp, err); + return; + } + else if (ret < 0 && xhci->msi == ON_OFF_AUTO_AUTO) { + /* If user doesn`t set it on, switch to non-msi variant silently */ + error_free(err); + } + } + if (xhci->numintrs > MAXINTRS) { xhci->numintrs = MAXINTRS; } @@ -3645,10 +3659,6 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp) assert(ret >= 0); } - if (xhci->msi == ON_OFF_AUTO_ON || - xhci->msi == ON_OFF_AUTO_AUTO) { - msi_init(dev, 0x70, xhci->numintrs, true, false); - } if (xhci->msix == ON_OFF_AUTO_ON || xhci->msix == ON_OFF_AUTO_AUTO) { msix_init(dev, xhci->numintrs, diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index d091d8c..55ceb67 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -1171,6 +1171,7 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos) uint16_t ctrl; bool msi_64bit, msi_maskbit; int ret, entries; + Error *err = NULL; if (pread(vdev->vbasedev.fd, &ctrl, sizeof(ctrl), vdev->config_offset + pos + PCI_CAP_FLAGS) != sizeof(ctrl)) { @@ -1184,12 +1185,13 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos) trace_vfio_msi_setup(vdev->vbasedev.name, pos); - ret = msi_init(&vdev->pdev, pos, entries, msi_64bit, msi_maskbit); + ret = msi_init(&vdev->pdev, pos, entries, msi_64bit, msi_maskbit, &err); if (ret < 0) { if (ret == -ENOTSUP) { return 0; } - error_report("vfio: msi_init failed"); + error_prepend(&err, "vfio: msi_init failed: "); + error_report_err(err); return ret; } vdev->msi_cap_size = 0xa + (msi_maskbit ? 0xa : 0) + (msi_64bit ? 0x4 : 0); diff --git a/include/hw/pci/msi.h b/include/hw/pci/msi.h index 8124908..4837bcf 100644 --- a/include/hw/pci/msi.h +++ b/include/hw/pci/msi.h @@ -35,7 +35,8 @@ void msi_set_message(PCIDevice *dev, MSIMessage msg); MSIMessage msi_get_message(PCIDevice *dev, unsigned int vector); bool msi_enabled(const PCIDevice *dev); int msi_init(struct PCIDevice *dev, uint8_t offset, - unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask); + unsigned int nr_vectors, bool msi64bit, + bool msi_per_vector_mask, Error **errp); void msi_uninit(struct PCIDevice *dev); void msi_reset(PCIDevice *dev); void msi_notify(PCIDevice *dev, unsigned int vector);
msi_init() reports errors with error_report(), which is wrong when it's used in realize(). Fix by converting it to Error. Fix its callers to handle failure instead of ignoring it. For those callers who don`t handle the failure, it might happen: when user want msi on, but he doesn`t get what he want because of msi_init fails silently. cc: Gerd Hoffmann <kraxel@redhat.com> cc: John Snow <jsnow@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> --- the affected device is modified in this way: 1. intel hd audio: move msi_init() above, save the strength to free the MemoryRegion when it fails. 2. ich9 ahci: move msi_init() above, save the strenth to free the resource allocated when calling achi_realize(). It doesn`t have msi property, so msi_init failure leads to fall back to INTx silently. Just free the error object 3. vmxnet3: move msi_init() above. Remove the unecessary vmxnet3_init_msi(). It doesn`t have msi property, so msi_init() failure leads to fall back to INTx silently. Just free the error object. 4. ioh3420/xio3130_downstream/xio3130_upstream: they are pcie components, msi or msix is forced, catch error and report it right there. 5. pci_bridge_dev: msi_init`s failure is fatal, follow the behaviour. 6. megasas_scsi: move msi_init() above, save the strength to free the MemoryRegion when it fails. 7. mptsas: Move msi_init() above, save the strength to free the MemoryRegion when it fails. 8. pvscsi: it doesn`t have msi property, msi_init fail leads to fall back to INTx silently. 9. usb-xhci: move msi_init() above, save the strength to free the MemoryRegion when it fails. 10. vfio-pci: keep the previous behaviour, and just catch & report error. hw/audio/intel-hda.c | 18 +++++++++++++---- hw/ide/ich.c | 15 +++++++++----- hw/net/vmxnet3.c | 41 +++++++++++++++----------------------- hw/pci-bridge/ioh3420.c | 4 +++- hw/pci-bridge/pci_bridge_dev.c | 7 ++++--- hw/pci-bridge/xio3130_downstream.c | 4 +++- hw/pci-bridge/xio3130_upstream.c | 4 +++- hw/pci/msi.c | 9 +++++---- hw/scsi/megasas.c | 18 +++++++++++++---- hw/scsi/mptsas.c | 20 ++++++++++++++----- hw/scsi/vmw_pvscsi.c | 6 +++++- hw/usb/hcd-xhci.c | 18 +++++++++++++---- hw/vfio/pci.c | 6 ++++-- include/hw/pci/msi.h | 3 ++- 14 files changed, 112 insertions(+), 61 deletions(-)