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

[Markus Armbruster]

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