From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55111) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bCQRC-0004aR-E3 for qemu-devel@nongnu.org; Mon, 13 Jun 2016 07:53:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bCQR6-0007WC-Ug for qemu-devel@nongnu.org; Mon, 13 Jun 2016 07:53:49 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42533) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bCQR6-0007W4-Lo for qemu-devel@nongnu.org; Mon, 13 Jun 2016 07:53:44 -0400 References: <1465552478-5540-1-git-send-email-caoj.fnst@cn.fujitsu.com> <1465552478-5540-13-git-send-email-caoj.fnst@cn.fujitsu.com> <575E8814.8090505@redhat.com> <871t41xs7z.fsf@dusky.pond.sub.org> From: Marcel Apfelbaum Message-ID: <575E9EC2.3080105@redhat.com> Date: Mon, 13 Jun 2016 14:53:38 +0300 MIME-Version: 1.0 In-Reply-To: <871t41xs7z.fsf@dusky.pond.sub.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v8 12/17] pci: Convert msi_init() to Error and fix callers to check it List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Cao jin , qemu-devel@nongnu.org, "Michael S. Tsirkin" , Jason Wang , Alex Williamson , Hannes Reinecke , Dmitry Fleytman , Paolo Bonzini , John Snow , Gerd Hoffmann On 06/13/2016 02:07 PM, Markus Armbruster wrote: > Marcel Apfelbaum 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 >>> cc: John Snow >>> cc: Dmitry Fleytman >>> cc: Jason Wang >>> cc: Michael S. Tsirkin >>> cc: Hannes Reinecke >>> cc: Paolo Bonzini >>> cc: Alex Williamson >>> cc: Markus Armbruster >>> cc: Marcel Apfelbaum >>> >>> Reviewed-by: Markus Armbruster >>> Signed-off-by: Cao jin >>> --- >>> 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 >>> #include "sysemu/block-backend.h" >>> #include "sysemu/dma.h" >>> - >>> #include >>> #include >>> >>> @@ -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