Marcel Apfelbaum <[email protected]> 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 <[email protected]> >> cc: John Snow <[email protected]> >> cc: Dmitry Fleytman <[email protected]> >> cc: Jason Wang <[email protected]> >> cc: Michael S. Tsirkin <[email protected]> >> cc: Hannes Reinecke <[email protected]> >> cc: Paolo Bonzini <[email protected]> >> cc: Alex Williamson <[email protected]> >> cc: Markus Armbruster <[email protected]> >> cc: Marcel Apfelbaum <[email protected]> >> >> Reviewed-by: Markus Armbruster <[email protected]> >> Signed-off-by: Cao jin <[email protected]> >> --- >> 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.
