| Message ID | 1465552478-5540-13-git-send-email-caoj.fnst@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
On 06/10/2016 12:54 PM, 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> > > Reviewed-by: Markus Armbruster <armbru@redhat.com> > Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> > --- > hw/audio/intel-hda.c | 24 ++++++++++++++++++++---- > hw/ide/ich.c | 15 +++++++++------ > hw/net/e1000e.c | 8 ++------ > hw/net/vmxnet3.c | 37 ++++++++++++------------------------- > hw/pci-bridge/ioh3420.c | 6 +++++- > hw/pci-bridge/pci_bridge_dev.c | 20 ++++++++++++++++---- > hw/pci-bridge/xio3130_downstream.c | 6 +++++- > hw/pci-bridge/xio3130_upstream.c | 6 +++++- > hw/pci/msi.c | 11 ++++++++--- > hw/scsi/megasas.c | 26 +++++++++++++++++++++----- > hw/scsi/mptsas.c | 31 ++++++++++++++++++++++++------- > hw/scsi/vmw_pvscsi.c | 2 +- > hw/usb/hcd-xhci.c | 23 +++++++++++++++++++---- > hw/vfio/pci.c | 7 +++++-- > include/hw/pci/msi.h | 3 ++- > 15 files changed, 154 insertions(+), 71 deletions(-) > > diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c > index 6b4dda0..82101f8 100644 > --- a/hw/audio/intel-hda.c > +++ b/hw/audio/intel-hda.c > @@ -1135,6 +1135,8 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp) > { > IntelHDAState *d = INTEL_HDA(pci); > uint8_t *conf = d->pci.config; > + Error *err = NULL; > + int ret; > > d->name = object_get_typename(OBJECT(d)); > > @@ -1143,13 +1145,27 @@ 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) { > + ret = msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, > + 1, true, false, &err); > + /* Any error other than -ENOTSUP(board's MSI support is broken) > + * is a programming error */ > + assert(!ret || ret == -ENOTSUP); > + if (ret && d->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 || d->msi == ON_OFF_AUTO_AUTO); > + /* With msi=auto, we fall back to MSI off 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_OFF) { > - /* TODO check for errors */ > - 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..084bef8 100644 > --- a/hw/ide/ich.c > +++ b/hw/ide/ich.c > @@ -68,7 +68,6 @@ > #include <hw/isa/isa.h> > #include "sysemu/block-backend.h" > #include "sysemu/dma.h" > - > #include <hw/ide/pci.h> > #include <hw/ide/ahci.h> > > @@ -111,6 +110,15 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp) > int sata_cap_offset; > uint8_t *sata_cap; > d = ICH_AHCI(dev); > + int ret; > + > + /* 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. */ > + ret = msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false, 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); > > ahci_realize(&d->ahci, DEVICE(dev), pci_get_address_space(dev), 6); > > @@ -142,11 +150,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/e1000e.c b/hw/net/e1000e.c > index 692283f..a06d184 100644 > --- a/hw/net/e1000e.c > +++ b/hw/net/e1000e.c > @@ -268,13 +268,9 @@ e1000e_init_msi(E1000EState *s) > { > int res; > > - res = msi_init(PCI_DEVICE(s), > - 0xD0, /* MSI capability offset */ > - 1, /* MAC MSI interrupts */ > - true, /* 64-bit message addresses supported */ > - false); /* Per vector mask supported */ > + res = msi_init(PCI_DEVICE(s), 0xD0, 1, true, false, NULL); > Why did you chose to remove author's comments? > - if (res > 0) { > + if (!res) { > s->intr_state |= E1000E_USE_MSI; > } else { > trace_e1000e_msi_init_fail(res); > diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c > index d978976..63f8904 100644 > --- a/hw/net/vmxnet3.c > +++ b/hw/net/vmxnet3.c > @@ -2197,27 +2197,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) > { > @@ -2279,10 +2258,15 @@ static uint64_t vmxnet3_device_serial_num(VMXNET3State *s) > return dsn_payload; > } > > + > +#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); > + int ret; > > VMW_CBPRN("Starting init..."); > > @@ -2306,14 +2290,17 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp) > /* Interrupt pin A */ > pci_dev->config[PCI_INTERRUPT_PIN] = 0x01; > > + ret = msi_init(pci_dev, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS, > + VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK, 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->msi_used = !ret; > + > if (!vmxnet3_init_msix(s)) { > VMW_WRPRN("Failed to initialize MSI-X, configuration is inconsistent."); > } > > - if (!vmxnet3_init_msi(s)) { > - VMW_WRPRN("Failed to initialize MSI, configuration is inconsistent."); > - } > - > vmxnet3_net_init(s); > > if (pci_is_express(pci_dev)) { > diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c > index b4a7806..93c6f0b 100644 > --- a/hw/pci-bridge/ioh3420.c > +++ b/hw/pci-bridge/ioh3420.c > @@ -25,6 +25,7 @@ > #include "hw/pci/msi.h" > #include "hw/pci/pcie.h" > #include "ioh3420.h" > +#include "qapi/error.h" > > #define PCI_DEVICE_ID_IOH_EPORT 0x3420 /* D0:F0 express mode */ > #define PCI_DEVICE_ID_IOH_REV 0x2 > @@ -97,6 +98,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 +111,10 @@ 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) { > + assert(rc == -ENOTSUP); > + 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 0fbecc4..c4d2c0b 100644 > --- a/hw/pci-bridge/pci_bridge_dev.c > +++ b/hw/pci-bridge/pci_bridge_dev.c > @@ -54,6 +54,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); > > @@ -75,12 +76,23 @@ static int pci_bridge_dev_initfn(PCIDevice *dev) > goto slotid_error; > } > > - if (bridge_dev->msi != ON_OFF_AUTO_OFF && > - msi_nonbroken) { > - err = msi_init(dev, 0, 1, true, true); > - if (err < 0) { > + if (bridge_dev->msi != ON_OFF_AUTO_OFF) { > + /* it means SHPC exists, because SHPC is required for MSI */ Is the other way around, MSI is needed by SHPC (but not compulsory) > + > + err = msi_init(dev, 0, 1, true, true, &local_err); > + /* Any error other than -ENOTSUP(board's MSI support is broken) > + * is a programming error */ > + assert(!err || err == -ENOTSUP); > + if (err && bridge_dev->msi == ON_OFF_AUTO_ON) { > + /* Can't satisfy user's explicit msi=on request, fail */ > + error_append_hint(&local_err, "You have to use msi=auto (default) " > + "or msi=off with this machine type.\n"); > + error_report_err(local_err); > goto msi_error; > } > + assert(!local_err || bridge_dev->msi == ON_OFF_AUTO_AUTO); > + /* With msi=auto, we fall back to MSI off silently */ > + error_free(local_err); > } > > if (shpc_present(dev)) { > diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c > index e6d653d..f6149a3 100644 > --- a/hw/pci-bridge/xio3130_downstream.c > +++ b/hw/pci-bridge/xio3130_downstream.c > @@ -24,6 +24,7 @@ > #include "hw/pci/msi.h" > #include "hw/pci/pcie.h" > #include "xio3130_downstream.h" > +#include "qapi/error.h" > > #define PCI_DEVICE_ID_TI_XIO3130D 0x8233 /* downstream port */ > #define XIO3130_REVISION 0x1 > @@ -60,14 +61,17 @@ 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) { > + assert(rc == -ENOTSUP); > + 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..487edac 100644 > --- a/hw/pci-bridge/xio3130_upstream.c > +++ b/hw/pci-bridge/xio3130_upstream.c > @@ -24,6 +24,7 @@ > #include "hw/pci/msi.h" > #include "hw/pci/pcie.h" > #include "xio3130_upstream.h" > +#include "qapi/error.h" > > #define PCI_DEVICE_ID_TI_XIO3130U 0x8232 /* upstream port */ > #define XIO3130_REVISION 0x2 > @@ -56,14 +57,17 @@ 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) { > + assert(rc == -ENOTSUP); > + error_report_err(err); Hi Jin, Markus It looks a little weird to me to check for negative error code and then assert for a specific error as the only "valid error". Maybe we should assert inside msi_init to leave the callers "clean"? I am well aware a lot of time was spent for this series, but I just spotted it and I want to be sure we are doing it right. Thanks, Marcel > goto err_bridge; > } > > diff --git a/hw/pci/msi.c b/hw/pci/msi.c > index ed79225..a87b227 100644 > --- a/hw/pci/msi.c > +++ b/hw/pci/msi.c > @@ -22,6 +22,7 @@ > #include "hw/pci/msi.h" > #include "hw/xen/xen.h" > #include "qemu/range.h" > +#include "qapi/error.h" > > /* PCI_MSI_ADDRESS_LO */ > #define PCI_MSI_ADDRESS_LO_MASK (~0x3) > @@ -173,7 +174,8 @@ bool msi_enabled(const PCIDevice *dev) > * If @msi64bit, make the device capable of sending a 64-bit message > * address. > * If @msi_per_vector_mask, make the device support per-vector masking. > - * Return 0 on success, return -errno on error. > + * @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 @offset is non-zero, > @@ -181,7 +183,8 @@ bool msi_enabled(const PCIDevice *dev) > * 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; > @@ -189,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; > } > > @@ -212,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 2ede192..345783d 100644 > --- a/hw/scsi/megasas.c > +++ b/hw/scsi/megasas.c > @@ -29,7 +29,7 @@ > #include "hw/scsi/scsi.h" > #include "block/scsi.h" > #include "trace.h" > - > +#include "qapi/error.h" > #include "mfi.h" > > #define MEGASAS_VERSION_GEN1 "1.70" > @@ -2333,6 +2333,8 @@ 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; > + int ret; > > pci_conf = dev->config; > > @@ -2341,6 +2343,24 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp) > /* Interrupt pin 1 */ > pci_conf[PCI_INTERRUPT_PIN] = 0x01; > > + if (megasas_use_msi(s)) { > + ret = msi_init(dev, 0x50, 1, true, false, &err); > + /* Any error other than -ENOTSUP(board's MSI support is broken) > + * is a programming error */ > + assert(!ret || ret == -ENOTSUP); > + if (ret && s->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; > + } else if (ret) { > + /* With msi=auto, we fall back to MSI off silently */ > + s->msi = ON_OFF_AUTO_OFF; > + 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, > @@ -2348,10 +2368,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 dfbc0c4..698be42 100644 > --- a/hw/scsi/mptsas.c > +++ b/hw/scsi/mptsas.c > @@ -32,7 +32,7 @@ > #include "hw/scsi/scsi.h" > #include "block/scsi.h" > #include "trace.h" > - > +#include "qapi/error.h" > #include "mptsas.h" > #include "mpi.h" > > @@ -1273,10 +1273,33 @@ static void mptsas_scsi_realize(PCIDevice *dev, Error **errp) > { > DeviceState *d = DEVICE(dev); > MPTSASState *s = MPT_SAS(dev); > + Error *err = NULL; > + int ret; > > dev->config[PCI_LATENCY_TIMER] = 0; > dev->config[PCI_INTERRUPT_PIN] = 0x01; > > + if (s->msi != ON_OFF_AUTO_OFF) { > + ret = msi_init(dev, 0, 1, true, false, &err); > + /* Any error other than -ENOTSUP(board's MSI support is broken) > + * is a programming error */ > + assert(!ret || ret == -ENOTSUP); > + if (ret && s->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); > + s->msi_in_use = false; > + return; > + } else if (ret) { > + /* With msi=auto, we fall back to MSI off silently */ > + error_free(err); > + s->msi_in_use = false; > + } else { > + s->msi_in_use = true; > + } > + } > + > 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, > @@ -1284,12 +1307,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_OFF && > - msi_init(dev, 0, 1, true, false) >= 0) { > - /* TODO check for errors */ > - 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 e035fce..ecd6077 100644 > --- a/hw/scsi/vmw_pvscsi.c > +++ b/hw/scsi/vmw_pvscsi.c > @@ -1063,7 +1063,7 @@ pvscsi_init_msi(PVSCSIState *s) > 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, NULL); > if (res < 0) { > trace_pvscsi_init_msi_fail(res); > s->msi_used = false; > diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c > index 0a5510f..1a3377f 100644 > --- a/hw/usb/hcd-xhci.c > +++ b/hw/usb/hcd-xhci.c > @@ -26,6 +26,7 @@ > #include "hw/pci/msi.h" > #include "hw/pci/msix.h" > #include "trace.h" > +#include "qapi/error.h" > > //#define DEBUG_XHCI > //#define DEBUG_DATA > @@ -3581,6 +3582,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); > > @@ -3591,6 +3593,23 @@ 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); > + /* 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; > } > @@ -3648,10 +3667,6 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp) > assert(ret >= 0); > } > > - if (xhci->msi != ON_OFF_AUTO_OFF) { > - /* TODO check for errors */ > - msi_init(dev, 0x70, xhci->numintrs, true, false); > - } > if (xhci->msix != ON_OFF_AUTO_OFF) { > /* TODO check for errors */ > msix_init(dev, xhci->numintrs, > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index deab0c6..dfbf8ba 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -32,6 +32,7 @@ > #include "sysemu/sysemu.h" > #include "pci.h" > #include "trace.h" > +#include "qapi/error.h" > > #define MSIX_CAP_LENGTH 12 > > @@ -1171,6 +1172,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 +1186,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); >
Marcel Apfelbaum <marcel@redhat.com> writes: > On 06/10/2016 12:54 PM, 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> >> >> Reviewed-by: Markus Armbruster <armbru@redhat.com> >> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> >> --- >> hw/audio/intel-hda.c | 24 ++++++++++++++++++++---- >> hw/ide/ich.c | 15 +++++++++------ >> hw/net/e1000e.c | 8 ++------ >> hw/net/vmxnet3.c | 37 ++++++++++++------------------------- >> hw/pci-bridge/ioh3420.c | 6 +++++- >> hw/pci-bridge/pci_bridge_dev.c | 20 ++++++++++++++++---- >> hw/pci-bridge/xio3130_downstream.c | 6 +++++- >> hw/pci-bridge/xio3130_upstream.c | 6 +++++- >> hw/pci/msi.c | 11 ++++++++--- >> hw/scsi/megasas.c | 26 +++++++++++++++++++++----- >> hw/scsi/mptsas.c | 31 ++++++++++++++++++++++++------- >> hw/scsi/vmw_pvscsi.c | 2 +- >> hw/usb/hcd-xhci.c | 23 +++++++++++++++++++---- >> hw/vfio/pci.c | 7 +++++-- >> include/hw/pci/msi.h | 3 ++- >> 15 files changed, 154 insertions(+), 71 deletions(-) >> >> diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c >> index 6b4dda0..82101f8 100644 >> --- a/hw/audio/intel-hda.c >> +++ b/hw/audio/intel-hda.c >> @@ -1135,6 +1135,8 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp) >> { >> IntelHDAState *d = INTEL_HDA(pci); >> uint8_t *conf = d->pci.config; >> + Error *err = NULL; >> + int ret; >> >> d->name = object_get_typename(OBJECT(d)); >> >> @@ -1143,13 +1145,27 @@ 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) { >> + ret = msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, >> + 1, true, false, &err); >> + /* Any error other than -ENOTSUP(board's MSI support is broken) >> + * is a programming error */ >> + assert(!ret || ret == -ENOTSUP); >> + if (ret && d->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 || d->msi == ON_OFF_AUTO_AUTO); >> + /* With msi=auto, we fall back to MSI off 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_OFF) { >> - /* TODO check for errors */ >> - 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..084bef8 100644 >> --- a/hw/ide/ich.c >> +++ b/hw/ide/ich.c >> @@ -68,7 +68,6 @@ >> #include <hw/isa/isa.h> >> #include "sysemu/block-backend.h" >> #include "sysemu/dma.h" >> - >> #include <hw/ide/pci.h> >> #include <hw/ide/ahci.h> >> >> @@ -111,6 +110,15 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp) >> int sata_cap_offset; >> uint8_t *sata_cap; >> d = ICH_AHCI(dev); >> + int ret; >> + >> + /* 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. */ >> + ret = msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false, 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); >> >> ahci_realize(&d->ahci, DEVICE(dev), pci_get_address_space(dev), 6); >> >> @@ -142,11 +150,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/e1000e.c b/hw/net/e1000e.c >> index 692283f..a06d184 100644 >> --- a/hw/net/e1000e.c >> +++ b/hw/net/e1000e.c >> @@ -268,13 +268,9 @@ e1000e_init_msi(E1000EState *s) >> { >> int res; >> >> - res = msi_init(PCI_DEVICE(s), >> - 0xD0, /* MSI capability offset */ >> - 1, /* MAC MSI interrupts */ >> - true, /* 64-bit message addresses supported */ >> - false); /* Per vector mask supported */ >> + res = msi_init(PCI_DEVICE(s), 0xD0, 1, true, false, NULL); >> > > Why did you chose to remove author's comments? > > >> - if (res > 0) { >> + if (!res) { >> s->intr_state |= E1000E_USE_MSI; >> } else { >> trace_e1000e_msi_init_fail(res); >> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c >> index d978976..63f8904 100644 >> --- a/hw/net/vmxnet3.c >> +++ b/hw/net/vmxnet3.c >> @@ -2197,27 +2197,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) >> { >> @@ -2279,10 +2258,15 @@ static uint64_t vmxnet3_device_serial_num(VMXNET3State *s) >> return dsn_payload; >> } >> >> + >> +#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); >> + int ret; >> >> VMW_CBPRN("Starting init..."); >> >> @@ -2306,14 +2290,17 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp) >> /* Interrupt pin A */ >> pci_dev->config[PCI_INTERRUPT_PIN] = 0x01; >> >> + ret = msi_init(pci_dev, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS, >> + VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK, 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->msi_used = !ret; >> + >> if (!vmxnet3_init_msix(s)) { >> VMW_WRPRN("Failed to initialize MSI-X, configuration is inconsistent."); >> } >> >> - if (!vmxnet3_init_msi(s)) { >> - VMW_WRPRN("Failed to initialize MSI, configuration is inconsistent."); >> - } >> - >> vmxnet3_net_init(s); >> >> if (pci_is_express(pci_dev)) { >> diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c >> index b4a7806..93c6f0b 100644 >> --- a/hw/pci-bridge/ioh3420.c >> +++ b/hw/pci-bridge/ioh3420.c >> @@ -25,6 +25,7 @@ >> #include "hw/pci/msi.h" >> #include "hw/pci/pcie.h" >> #include "ioh3420.h" >> +#include "qapi/error.h" >> >> #define PCI_DEVICE_ID_IOH_EPORT 0x3420 /* D0:F0 express mode */ >> #define PCI_DEVICE_ID_IOH_REV 0x2 >> @@ -97,6 +98,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 +111,10 @@ 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) { >> + assert(rc == -ENOTSUP); >> + 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 0fbecc4..c4d2c0b 100644 >> --- a/hw/pci-bridge/pci_bridge_dev.c >> +++ b/hw/pci-bridge/pci_bridge_dev.c >> @@ -54,6 +54,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); >> >> @@ -75,12 +76,23 @@ static int pci_bridge_dev_initfn(PCIDevice *dev) >> goto slotid_error; >> } >> >> - if (bridge_dev->msi != ON_OFF_AUTO_OFF && >> - msi_nonbroken) { >> - err = msi_init(dev, 0, 1, true, true); >> - if (err < 0) { >> + if (bridge_dev->msi != ON_OFF_AUTO_OFF) { >> + /* it means SHPC exists, because SHPC is required for MSI */ > > Is the other way around, MSI is needed by SHPC (but not compulsory) > >> + >> + err = msi_init(dev, 0, 1, true, true, &local_err); >> + /* Any error other than -ENOTSUP(board's MSI support is broken) >> + * is a programming error */ >> + assert(!err || err == -ENOTSUP); >> + if (err && bridge_dev->msi == ON_OFF_AUTO_ON) { >> + /* Can't satisfy user's explicit msi=on request, fail */ >> + error_append_hint(&local_err, "You have to use msi=auto (default) " >> + "or msi=off with this machine type.\n"); >> + error_report_err(local_err); >> goto msi_error; >> } >> + assert(!local_err || bridge_dev->msi == ON_OFF_AUTO_AUTO); >> + /* With msi=auto, we fall back to MSI off silently */ >> + error_free(local_err); >> } >> >> if (shpc_present(dev)) { >> diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c >> index e6d653d..f6149a3 100644 >> --- a/hw/pci-bridge/xio3130_downstream.c >> +++ b/hw/pci-bridge/xio3130_downstream.c >> @@ -24,6 +24,7 @@ >> #include "hw/pci/msi.h" >> #include "hw/pci/pcie.h" >> #include "xio3130_downstream.h" >> +#include "qapi/error.h" >> >> #define PCI_DEVICE_ID_TI_XIO3130D 0x8233 /* downstream port */ >> #define XIO3130_REVISION 0x1 >> @@ -60,14 +61,17 @@ 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) { >> + assert(rc == -ENOTSUP); >> + 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..487edac 100644 >> --- a/hw/pci-bridge/xio3130_upstream.c >> +++ b/hw/pci-bridge/xio3130_upstream.c >> @@ -24,6 +24,7 @@ >> #include "hw/pci/msi.h" >> #include "hw/pci/pcie.h" >> #include "xio3130_upstream.h" >> +#include "qapi/error.h" >> >> #define PCI_DEVICE_ID_TI_XIO3130U 0x8232 /* upstream port */ >> #define XIO3130_REVISION 0x2 >> @@ -56,14 +57,17 @@ 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) { >> + assert(rc == -ENOTSUP); >> + error_report_err(err); > > Hi Jin, Markus > > It looks a little weird to me to check for negative error code > and then assert for a specific error as the only "valid error". > Maybe we should assert inside msi_init to leave the callers "clean"? > > I am well aware a lot of time was spent for this series, but I just > spotted it and I want to be sure we are doing it right. Only the callers know how to handle these errors. Let me explain using the function comment: * -ENOTSUP means lacking msi support for a msi-capable platform. Our board emulation is broken. The device decides whether this is an error (say because the user requested msi=on), or not. If it isn't, devices generally fall back to a non-MSI variant. * -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. For virtual devices, this is a programming error. Such callers should therefore abort. But for device assignment, it's a physical device with messed up capabilities --- not a programming error, aborting would be inappropriate. See vfio_msi_setup() for an example.
On 06/13/2016 06:16 PM, Marcel Apfelbaum wrote: > On 06/10/2016 12:54 PM, Cao jin wrote: >> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c >> index 692283f..a06d184 100644 >> --- a/hw/net/e1000e.c >> +++ b/hw/net/e1000e.c >> @@ -268,13 +268,9 @@ e1000e_init_msi(E1000EState *s) >> { >> int res; >> >> - res = msi_init(PCI_DEVICE(s), >> - 0xD0, /* MSI capability offset */ >> - 1, /* MAC MSI interrupts */ >> - true, /* 64-bit message addresses supported */ >> - false); /* Per vector mask supported */ >> + res = msi_init(PCI_DEVICE(s), 0xD0, 1, true, false, NULL); >> > > Why did you chose to remove author's comments? > Because msi_init() has its function comments now, which is the same is the author`s comments, I guess author add these comments because function has no comment before, remove comments also is to save screen space:) some macros of some devices is also unnecessary I think, because we have comments of msi_init(). >> + >> +#define VMXNET3_USE_64BIT (true) >> +#define VMXNET3_PER_VECTOR_MASK (false) >> + like these macros, but it does't take too much space as above one I feel, so I didn't touch them. >> @@ -56,14 +57,17 @@ 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) { >> + assert(rc == -ENOTSUP); >> + error_report_err(err); > > Hi Jin, Markus > > It looks a little weird to me to check for negative error code > and then assert for a specific error as the only "valid error". > Maybe we should assert inside msi_init to leave the callers "clean"? > Hi Marcel, I think it is because: except assigned device(vfio), the emulated pci devices won`t have config space overlapped(-EINVAL), unless programming error. If implemented as you said, I guess there need a judgement (if..else..) to check if current device is assigned in msi_init(), or else assert the error > I am well aware a lot of time was spent for this series, but I just > spotted it and I want to be sure we are doing it right. > > Thanks, > Marcel >
On 06/13/2016 02:07 PM, Markus Armbruster wrote: > Marcel Apfelbaum <marcel@redhat.com> writes: > >> On 06/10/2016 12:54 PM, 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> >>> >>> Reviewed-by: Markus Armbruster <armbru@redhat.com> >>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> >>> --- >>> hw/audio/intel-hda.c | 24 ++++++++++++++++++++---- >>> hw/ide/ich.c | 15 +++++++++------ >>> hw/net/e1000e.c | 8 ++------ >>> hw/net/vmxnet3.c | 37 ++++++++++++------------------------- >>> hw/pci-bridge/ioh3420.c | 6 +++++- >>> hw/pci-bridge/pci_bridge_dev.c | 20 ++++++++++++++++---- >>> hw/pci-bridge/xio3130_downstream.c | 6 +++++- >>> hw/pci-bridge/xio3130_upstream.c | 6 +++++- >>> hw/pci/msi.c | 11 ++++++++--- >>> hw/scsi/megasas.c | 26 +++++++++++++++++++++----- >>> hw/scsi/mptsas.c | 31 ++++++++++++++++++++++++------- >>> hw/scsi/vmw_pvscsi.c | 2 +- >>> hw/usb/hcd-xhci.c | 23 +++++++++++++++++++---- >>> hw/vfio/pci.c | 7 +++++-- >>> include/hw/pci/msi.h | 3 ++- >>> 15 files changed, 154 insertions(+), 71 deletions(-) >>> >>> diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c >>> index 6b4dda0..82101f8 100644 >>> --- a/hw/audio/intel-hda.c >>> +++ b/hw/audio/intel-hda.c >>> @@ -1135,6 +1135,8 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp) >>> { >>> IntelHDAState *d = INTEL_HDA(pci); >>> uint8_t *conf = d->pci.config; >>> + Error *err = NULL; >>> + int ret; >>> >>> d->name = object_get_typename(OBJECT(d)); >>> >>> @@ -1143,13 +1145,27 @@ 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) { >>> + ret = msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, >>> + 1, true, false, &err); >>> + /* Any error other than -ENOTSUP(board's MSI support is broken) >>> + * is a programming error */ >>> + assert(!ret || ret == -ENOTSUP); >>> + if (ret && d->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 || d->msi == ON_OFF_AUTO_AUTO); >>> + /* With msi=auto, we fall back to MSI off 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_OFF) { >>> - /* TODO check for errors */ >>> - 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..084bef8 100644 >>> --- a/hw/ide/ich.c >>> +++ b/hw/ide/ich.c >>> @@ -68,7 +68,6 @@ >>> #include <hw/isa/isa.h> >>> #include "sysemu/block-backend.h" >>> #include "sysemu/dma.h" >>> - >>> #include <hw/ide/pci.h> >>> #include <hw/ide/ahci.h> >>> >>> @@ -111,6 +110,15 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp) >>> int sata_cap_offset; >>> uint8_t *sata_cap; >>> d = ICH_AHCI(dev); >>> + int ret; >>> + >>> + /* 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. */ >>> + ret = msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false, 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); >>> >>> ahci_realize(&d->ahci, DEVICE(dev), pci_get_address_space(dev), 6); >>> >>> @@ -142,11 +150,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/e1000e.c b/hw/net/e1000e.c >>> index 692283f..a06d184 100644 >>> --- a/hw/net/e1000e.c >>> +++ b/hw/net/e1000e.c >>> @@ -268,13 +268,9 @@ e1000e_init_msi(E1000EState *s) >>> { >>> int res; >>> >>> - res = msi_init(PCI_DEVICE(s), >>> - 0xD0, /* MSI capability offset */ >>> - 1, /* MAC MSI interrupts */ >>> - true, /* 64-bit message addresses supported */ >>> - false); /* Per vector mask supported */ >>> + res = msi_init(PCI_DEVICE(s), 0xD0, 1, true, false, NULL); >>> >> >> Why did you chose to remove author's comments? >> >> >>> - if (res > 0) { >>> + if (!res) { >>> s->intr_state |= E1000E_USE_MSI; >>> } else { >>> trace_e1000e_msi_init_fail(res); >>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c >>> index d978976..63f8904 100644 >>> --- a/hw/net/vmxnet3.c >>> +++ b/hw/net/vmxnet3.c >>> @@ -2197,27 +2197,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) >>> { >>> @@ -2279,10 +2258,15 @@ static uint64_t vmxnet3_device_serial_num(VMXNET3State *s) >>> return dsn_payload; >>> } >>> >>> + >>> +#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); >>> + int ret; >>> >>> VMW_CBPRN("Starting init..."); >>> >>> @@ -2306,14 +2290,17 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp) >>> /* Interrupt pin A */ >>> pci_dev->config[PCI_INTERRUPT_PIN] = 0x01; >>> >>> + ret = msi_init(pci_dev, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS, >>> + VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK, 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->msi_used = !ret; >>> + >>> if (!vmxnet3_init_msix(s)) { >>> VMW_WRPRN("Failed to initialize MSI-X, configuration is inconsistent."); >>> } >>> >>> - if (!vmxnet3_init_msi(s)) { >>> - VMW_WRPRN("Failed to initialize MSI, configuration is inconsistent."); >>> - } >>> - >>> vmxnet3_net_init(s); >>> >>> if (pci_is_express(pci_dev)) { >>> diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c >>> index b4a7806..93c6f0b 100644 >>> --- a/hw/pci-bridge/ioh3420.c >>> +++ b/hw/pci-bridge/ioh3420.c >>> @@ -25,6 +25,7 @@ >>> #include "hw/pci/msi.h" >>> #include "hw/pci/pcie.h" >>> #include "ioh3420.h" >>> +#include "qapi/error.h" >>> >>> #define PCI_DEVICE_ID_IOH_EPORT 0x3420 /* D0:F0 express mode */ >>> #define PCI_DEVICE_ID_IOH_REV 0x2 >>> @@ -97,6 +98,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 +111,10 @@ 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) { >>> + assert(rc == -ENOTSUP); >>> + 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 0fbecc4..c4d2c0b 100644 >>> --- a/hw/pci-bridge/pci_bridge_dev.c >>> +++ b/hw/pci-bridge/pci_bridge_dev.c >>> @@ -54,6 +54,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); >>> >>> @@ -75,12 +76,23 @@ static int pci_bridge_dev_initfn(PCIDevice *dev) >>> goto slotid_error; >>> } >>> >>> - if (bridge_dev->msi != ON_OFF_AUTO_OFF && >>> - msi_nonbroken) { >>> - err = msi_init(dev, 0, 1, true, true); >>> - if (err < 0) { >>> + if (bridge_dev->msi != ON_OFF_AUTO_OFF) { >>> + /* it means SHPC exists, because SHPC is required for MSI */ >> >> Is the other way around, MSI is needed by SHPC (but not compulsory) >> >>> + >>> + err = msi_init(dev, 0, 1, true, true, &local_err); >>> + /* Any error other than -ENOTSUP(board's MSI support is broken) >>> + * is a programming error */ >>> + assert(!err || err == -ENOTSUP); >>> + if (err && bridge_dev->msi == ON_OFF_AUTO_ON) { >>> + /* Can't satisfy user's explicit msi=on request, fail */ >>> + error_append_hint(&local_err, "You have to use msi=auto (default) " >>> + "or msi=off with this machine type.\n"); >>> + error_report_err(local_err); >>> goto msi_error; >>> } >>> + assert(!local_err || bridge_dev->msi == ON_OFF_AUTO_AUTO); >>> + /* With msi=auto, we fall back to MSI off silently */ >>> + error_free(local_err); >>> } >>> >>> if (shpc_present(dev)) { >>> diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c >>> index e6d653d..f6149a3 100644 >>> --- a/hw/pci-bridge/xio3130_downstream.c >>> +++ b/hw/pci-bridge/xio3130_downstream.c >>> @@ -24,6 +24,7 @@ >>> #include "hw/pci/msi.h" >>> #include "hw/pci/pcie.h" >>> #include "xio3130_downstream.h" >>> +#include "qapi/error.h" >>> >>> #define PCI_DEVICE_ID_TI_XIO3130D 0x8233 /* downstream port */ >>> #define XIO3130_REVISION 0x1 >>> @@ -60,14 +61,17 @@ 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) { >>> + assert(rc == -ENOTSUP); >>> + 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..487edac 100644 >>> --- a/hw/pci-bridge/xio3130_upstream.c >>> +++ b/hw/pci-bridge/xio3130_upstream.c >>> @@ -24,6 +24,7 @@ >>> #include "hw/pci/msi.h" >>> #include "hw/pci/pcie.h" >>> #include "xio3130_upstream.h" >>> +#include "qapi/error.h" >>> >>> #define PCI_DEVICE_ID_TI_XIO3130U 0x8232 /* upstream port */ >>> #define XIO3130_REVISION 0x2 >>> @@ -56,14 +57,17 @@ 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) { >>> + assert(rc == -ENOTSUP); >>> + error_report_err(err); >> >> Hi Jin, Markus >> >> It looks a little weird to me to check for negative error code >> and then assert for a specific error as the only "valid error". >> Maybe we should assert inside msi_init to leave the callers "clean"? >> >> I am well aware a lot of time was spent for this series, but I just >> spotted it and I want to be sure we are doing it right. > > Only the callers know how to handle these errors. Let me explain using > the function comment: > > * -ENOTSUP means lacking msi support for a msi-capable platform. > > Our board emulation is broken. The device decides whether this is an > error (say because the user requested msi=on), or not. If it isn't, > devices generally fall back to a non-MSI variant. > > * -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. > > For virtual devices, this is a programming error. Such callers should > therefore abort. But for device assignment, it's a physical device with > messed up capabilities --- not a programming error, aborting would be > inappropriate. See vfio_msi_setup() for an example. > I missed the vfio scenario. Now I got it. Thanks! Marcel
On 06/13/2016 02:09 PM, Cao jin wrote: > > > On 06/13/2016 06:16 PM, Marcel Apfelbaum wrote: >> On 06/10/2016 12:54 PM, Cao jin wrote: > >>> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c >>> index 692283f..a06d184 100644 >>> --- a/hw/net/e1000e.c >>> +++ b/hw/net/e1000e.c >>> @@ -268,13 +268,9 @@ e1000e_init_msi(E1000EState *s) >>> { >>> int res; >>> >>> - res = msi_init(PCI_DEVICE(s), >>> - 0xD0, /* MSI capability offset */ >>> - 1, /* MAC MSI interrupts */ >>> - true, /* 64-bit message addresses supported */ >>> - false); /* Per vector mask supported */ >>> + res = msi_init(PCI_DEVICE(s), 0xD0, 1, true, false, NULL); >>> >> >> Why did you chose to remove author's comments? >> > > Because msi_init() has its function comments now, which is the same is the author`s comments, I guess author add these comments because function has no comment before, remove comments also is to save > screen space:) > > some macros of some devices is also unnecessary I think, because we have comments of msi_init(). > Right. >>> + >>> +#define VMXNET3_USE_64BIT (true) >>> +#define VMXNET3_PER_VECTOR_MASK (false) >>> + > > like these macros, but it does't take too much space as above one I feel, so I didn't touch them. > >>> @@ -56,14 +57,17 @@ 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) { >>> + assert(rc == -ENOTSUP); >>> + error_report_err(err); >> >> Hi Jin, Markus >> >> It looks a little weird to me to check for negative error code >> and then assert for a specific error as the only "valid error". >> Maybe we should assert inside msi_init to leave the callers "clean"? >> > Hi Marcel, > > I think it is because: except assigned device(vfio), the emulated pci devices won`t have config space overlapped(-EINVAL), unless programming error. > Understood, thanks for the explanation. For the PCI part: Reviewed-by: Marcel Apfelbaum <marcel@redhat.com> Thanks, Marcel > If implemented as you said, I guess there need a judgement (if..else..) to check if current device is assigned in msi_init(), or else assert the error > >> I am well aware a lot of time was spent for this series, but I just >> spotted it and I want to be sure we are doing it right. >> >> Thanks, >> Marcel >> >
diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c index 6b4dda0..82101f8 100644 --- a/hw/audio/intel-hda.c +++ b/hw/audio/intel-hda.c @@ -1135,6 +1135,8 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp) { IntelHDAState *d = INTEL_HDA(pci); uint8_t *conf = d->pci.config; + Error *err = NULL; + int ret; d->name = object_get_typename(OBJECT(d)); @@ -1143,13 +1145,27 @@ 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) { + ret = msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, + 1, true, false, &err); + /* Any error other than -ENOTSUP(board's MSI support is broken) + * is a programming error */ + assert(!ret || ret == -ENOTSUP); + if (ret && d->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 || d->msi == ON_OFF_AUTO_AUTO); + /* With msi=auto, we fall back to MSI off 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_OFF) { - /* TODO check for errors */ - 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..084bef8 100644 --- a/hw/ide/ich.c +++ b/hw/ide/ich.c @@ -68,7 +68,6 @@ #include <hw/isa/isa.h> #include "sysemu/block-backend.h" #include "sysemu/dma.h" - #include <hw/ide/pci.h> #include <hw/ide/ahci.h> @@ -111,6 +110,15 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp) int sata_cap_offset; uint8_t *sata_cap; d = ICH_AHCI(dev); + int ret; + + /* 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. */ + ret = msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false, 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); ahci_realize(&d->ahci, DEVICE(dev), pci_get_address_space(dev), 6); @@ -142,11 +150,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/e1000e.c b/hw/net/e1000e.c index 692283f..a06d184 100644 --- a/hw/net/e1000e.c +++ b/hw/net/e1000e.c @@ -268,13 +268,9 @@ e1000e_init_msi(E1000EState *s) { int res; - res = msi_init(PCI_DEVICE(s), - 0xD0, /* MSI capability offset */ - 1, /* MAC MSI interrupts */ - true, /* 64-bit message addresses supported */ - false); /* Per vector mask supported */ + res = msi_init(PCI_DEVICE(s), 0xD0, 1, true, false, NULL); - if (res > 0) { + if (!res) { s->intr_state |= E1000E_USE_MSI; } else { trace_e1000e_msi_init_fail(res); diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c index d978976..63f8904 100644 --- a/hw/net/vmxnet3.c +++ b/hw/net/vmxnet3.c @@ -2197,27 +2197,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) { @@ -2279,10 +2258,15 @@ static uint64_t vmxnet3_device_serial_num(VMXNET3State *s) return dsn_payload; } + +#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); + int ret; VMW_CBPRN("Starting init..."); @@ -2306,14 +2290,17 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp) /* Interrupt pin A */ pci_dev->config[PCI_INTERRUPT_PIN] = 0x01; + ret = msi_init(pci_dev, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS, + VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK, 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->msi_used = !ret; + if (!vmxnet3_init_msix(s)) { VMW_WRPRN("Failed to initialize MSI-X, configuration is inconsistent."); } - if (!vmxnet3_init_msi(s)) { - VMW_WRPRN("Failed to initialize MSI, configuration is inconsistent."); - } - vmxnet3_net_init(s); if (pci_is_express(pci_dev)) { diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c index b4a7806..93c6f0b 100644 --- a/hw/pci-bridge/ioh3420.c +++ b/hw/pci-bridge/ioh3420.c @@ -25,6 +25,7 @@ #include "hw/pci/msi.h" #include "hw/pci/pcie.h" #include "ioh3420.h" +#include "qapi/error.h" #define PCI_DEVICE_ID_IOH_EPORT 0x3420 /* D0:F0 express mode */ #define PCI_DEVICE_ID_IOH_REV 0x2 @@ -97,6 +98,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 +111,10 @@ 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) { + assert(rc == -ENOTSUP); + 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 0fbecc4..c4d2c0b 100644 --- a/hw/pci-bridge/pci_bridge_dev.c +++ b/hw/pci-bridge/pci_bridge_dev.c @@ -54,6 +54,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); @@ -75,12 +76,23 @@ static int pci_bridge_dev_initfn(PCIDevice *dev) goto slotid_error; } - if (bridge_dev->msi != ON_OFF_AUTO_OFF && - msi_nonbroken) { - err = msi_init(dev, 0, 1, true, true); - if (err < 0) { + if (bridge_dev->msi != ON_OFF_AUTO_OFF) { + /* it means SHPC exists, because SHPC is required for MSI */ + + err = msi_init(dev, 0, 1, true, true, &local_err); + /* Any error other than -ENOTSUP(board's MSI support is broken) + * is a programming error */ + assert(!err || err == -ENOTSUP); + if (err && bridge_dev->msi == ON_OFF_AUTO_ON) { + /* Can't satisfy user's explicit msi=on request, fail */ + error_append_hint(&local_err, "You have to use msi=auto (default) " + "or msi=off with this machine type.\n"); + error_report_err(local_err); goto msi_error; } + assert(!local_err || bridge_dev->msi == ON_OFF_AUTO_AUTO); + /* With msi=auto, we fall back to MSI off silently */ + error_free(local_err); } if (shpc_present(dev)) { diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c index e6d653d..f6149a3 100644 --- a/hw/pci-bridge/xio3130_downstream.c +++ b/hw/pci-bridge/xio3130_downstream.c @@ -24,6 +24,7 @@ #include "hw/pci/msi.h" #include "hw/pci/pcie.h" #include "xio3130_downstream.h" +#include "qapi/error.h" #define PCI_DEVICE_ID_TI_XIO3130D 0x8233 /* downstream port */ #define XIO3130_REVISION 0x1 @@ -60,14 +61,17 @@ 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) { + assert(rc == -ENOTSUP); + 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..487edac 100644 --- a/hw/pci-bridge/xio3130_upstream.c +++ b/hw/pci-bridge/xio3130_upstream.c @@ -24,6 +24,7 @@ #include "hw/pci/msi.h" #include "hw/pci/pcie.h" #include "xio3130_upstream.h" +#include "qapi/error.h" #define PCI_DEVICE_ID_TI_XIO3130U 0x8232 /* upstream port */ #define XIO3130_REVISION 0x2 @@ -56,14 +57,17 @@ 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) { + assert(rc == -ENOTSUP); + error_report_err(err); goto err_bridge; } diff --git a/hw/pci/msi.c b/hw/pci/msi.c index ed79225..a87b227 100644 --- a/hw/pci/msi.c +++ b/hw/pci/msi.c @@ -22,6 +22,7 @@ #include "hw/pci/msi.h" #include "hw/xen/xen.h" #include "qemu/range.h" +#include "qapi/error.h" /* PCI_MSI_ADDRESS_LO */ #define PCI_MSI_ADDRESS_LO_MASK (~0x3) @@ -173,7 +174,8 @@ bool msi_enabled(const PCIDevice *dev) * If @msi64bit, make the device capable of sending a 64-bit message * address. * If @msi_per_vector_mask, make the device support per-vector masking. - * Return 0 on success, return -errno on error. + * @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 @offset is non-zero, @@ -181,7 +183,8 @@ bool msi_enabled(const PCIDevice *dev) * 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; @@ -189,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; } @@ -212,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 2ede192..345783d 100644 --- a/hw/scsi/megasas.c +++ b/hw/scsi/megasas.c @@ -29,7 +29,7 @@ #include "hw/scsi/scsi.h" #include "block/scsi.h" #include "trace.h" - +#include "qapi/error.h" #include "mfi.h" #define MEGASAS_VERSION_GEN1 "1.70" @@ -2333,6 +2333,8 @@ 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; + int ret; pci_conf = dev->config; @@ -2341,6 +2343,24 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp) /* Interrupt pin 1 */ pci_conf[PCI_INTERRUPT_PIN] = 0x01; + if (megasas_use_msi(s)) { + ret = msi_init(dev, 0x50, 1, true, false, &err); + /* Any error other than -ENOTSUP(board's MSI support is broken) + * is a programming error */ + assert(!ret || ret == -ENOTSUP); + if (ret && s->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; + } else if (ret) { + /* With msi=auto, we fall back to MSI off silently */ + s->msi = ON_OFF_AUTO_OFF; + 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, @@ -2348,10 +2368,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 dfbc0c4..698be42 100644 --- a/hw/scsi/mptsas.c +++ b/hw/scsi/mptsas.c @@ -32,7 +32,7 @@ #include "hw/scsi/scsi.h" #include "block/scsi.h" #include "trace.h" - +#include "qapi/error.h" #include "mptsas.h" #include "mpi.h" @@ -1273,10 +1273,33 @@ static void mptsas_scsi_realize(PCIDevice *dev, Error **errp) { DeviceState *d = DEVICE(dev); MPTSASState *s = MPT_SAS(dev); + Error *err = NULL; + int ret; dev->config[PCI_LATENCY_TIMER] = 0; dev->config[PCI_INTERRUPT_PIN] = 0x01; + if (s->msi != ON_OFF_AUTO_OFF) { + ret = msi_init(dev, 0, 1, true, false, &err); + /* Any error other than -ENOTSUP(board's MSI support is broken) + * is a programming error */ + assert(!ret || ret == -ENOTSUP); + if (ret && s->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); + s->msi_in_use = false; + return; + } else if (ret) { + /* With msi=auto, we fall back to MSI off silently */ + error_free(err); + s->msi_in_use = false; + } else { + s->msi_in_use = true; + } + } + 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, @@ -1284,12 +1307,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_OFF && - msi_init(dev, 0, 1, true, false) >= 0) { - /* TODO check for errors */ - 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 e035fce..ecd6077 100644 --- a/hw/scsi/vmw_pvscsi.c +++ b/hw/scsi/vmw_pvscsi.c @@ -1063,7 +1063,7 @@ pvscsi_init_msi(PVSCSIState *s) 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, NULL); if (res < 0) { trace_pvscsi_init_msi_fail(res); s->msi_used = false; diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c index 0a5510f..1a3377f 100644 --- a/hw/usb/hcd-xhci.c +++ b/hw/usb/hcd-xhci.c @@ -26,6 +26,7 @@ #include "hw/pci/msi.h" #include "hw/pci/msix.h" #include "trace.h" +#include "qapi/error.h" //#define DEBUG_XHCI //#define DEBUG_DATA @@ -3581,6 +3582,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); @@ -3591,6 +3593,23 @@ 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); + /* 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; } @@ -3648,10 +3667,6 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp) assert(ret >= 0); } - if (xhci->msi != ON_OFF_AUTO_OFF) { - /* TODO check for errors */ - msi_init(dev, 0x70, xhci->numintrs, true, false); - } if (xhci->msix != ON_OFF_AUTO_OFF) { /* TODO check for errors */ msix_init(dev, xhci->numintrs, diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index deab0c6..dfbf8ba 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -32,6 +32,7 @@ #include "sysemu/sysemu.h" #include "pci.h" #include "trace.h" +#include "qapi/error.h" #define MSIX_CAP_LENGTH 12 @@ -1171,6 +1172,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 +1186,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);