Re: [Qemu-devel] [PATCH v8 12/17] pci: Convert msi_init() to Error and fix callers to check it

[Marcel Apfelbaum <marcel@redhat.com>]

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