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

[Markus Armbruster <armbru@redhat.com>]

Cao jin <caoj.fnst@cn.fujitsu.com> writes:

> msi_init() reports errors with error_report(), which is wrong
> when it's used in realize().

msix_init() has the same problem.  Perhaps you can tackle it later.

> 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:

Please use ' (U+0027 APOSTROPHE) instead of ` (U+0060 GRAVE ACCENT) as
apostrophe: don't, not don`t.

> 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>
>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/audio/intel-hda.c               | 20 ++++++++++++++----
>  hw/ide/ich.c                       | 17 +++++++++------
>  hw/net/vmxnet3.c                   | 42 +++++++++++++++-----------------------
>  hw/pci-bridge/ioh3420.c            |  5 ++++-
>  hw/pci-bridge/pci_bridge_dev.c     | 17 ++++++++++-----
>  hw/pci-bridge/xio3130_downstream.c |  5 ++++-
>  hw/pci-bridge/xio3130_upstream.c   |  5 ++++-
>  hw/pci/msi.c                       |  8 ++++++--
>  hw/scsi/megasas.c                  | 22 +++++++++++++++-----
>  hw/scsi/mptsas.c                   | 26 +++++++++++++++++------
>  hw/scsi/vmw_pvscsi.c               |  6 +++++-
>  hw/usb/hcd-xhci.c                  | 21 +++++++++++++++----
>  hw/vfio/pci.c                      |  7 +++++--
>  include/hw/pci/msi.h               |  3 ++-
>  14 files changed, 140 insertions(+), 64 deletions(-)
>
> diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
> index 61362e5..9c7173c 100644
> --- a/hw/audio/intel-hda.c
> +++ b/hw/audio/intel-hda.c
> @@ -26,6 +26,7 @@
>  #include "intel-hda.h"
>  #include "intel-hda-defs.h"
>  #include "sysemu/dma.h"
> +#include "qapi/error.h"
>  
>  /* --------------------------------------------------------------------- */
>  /* hda bus                                                               */
> @@ -1131,6 +1132,7 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp)
>  {
>      IntelHDAState *d = INTEL_HDA(pci);
>      uint8_t *conf = d->pci.config;
> +    Error *err = NULL;
>  
>      d->name = object_get_typename(OBJECT(d));
>  
> @@ -1139,13 +1141,23 @@ 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) {
> +        msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, 1, true, false, &err);
> +        if (err && d->msi == ON_OFF_AUTO_ON) {
> +            /* If user set msi=on, then device creation fail */

Grammar nit: fails

Suggest: Can't satisfy user's explicit msi=on request, fail

> +            error_append_hint(&err, "msi=on fails to create device."
> +                    " Please use msi=auto or off and try again");

This produces something like

    qemu: -device intel-hda,msi=on: MSI is not supported by interrupt controller
    msi=on fails to create device. Please use msi=auto or off and try again

where the last line lacks punctuation and a newline.  Moreover, we
already told the user that it failed (first line), and telling him again
in different words isn't necessary.  Suggest:

    qemu: -device intel-hda,msi=on: MSI is not supported by interrupt controller
    You have to use msi=auto (default) or msi=off with this machine type.

> +            error_propagate(errp, err);
> +            return;
> +        } else if (err && d->msi == ON_OFF_AUTO_AUTO) {
> +            /* If user doesn`t set it on, switch to non-msi variant silently */
> +            error_free(err);
> +        }

The conditional is superfluous.

We call msi_init() only if d->msi != ON_OFF_AUTO_OFF.  If it sets @err
and d->msi == ON_OFF_AUTO_ON, we don't get here.  Therefore, err implies
d->msi == ON_OFF_AUTO_AUTO, and the conditional can be simplified to

           } else if (err) {
               error_free(err);
           }

But protecting error_free() like that is pointless, as it does nothing
when err is null.  Simplify further to

           }
           assert(!err || d->msi == ON_OFF_AUTO_AUTO);
           /* With msi=auto, we fall back to MSI off silently */
           error_free(err);

The assertion is more for aiding the reader than for catching mistakes.

The error checking could be tightened as follows:

           ret = msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60,
                          1, true, false, &err);
           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_AUTO ||
> -        d->msi == ON_OFF_AUTO_ON) {
> -        msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, 1, true, false);
> -    }

Code motion to avoid having to clean up d->mmio on error.  Good.

>  
>      hda_codec_bus_init(DEVICE(pci), &d->codecs, sizeof(d->codecs),
>                         intel_hda_response, intel_hda_xfer);

This is MSI device pattern #1: we have a property that lets the user
pick off, on or auto (on with silent fallback to off).  We'll see a few
more below.  The lack of consistency is disgusting, but cleaning it up
is out of scope for your series (assuming it's even possible now).

> diff --git a/hw/ide/ich.c b/hw/ide/ich.c
> index 0a13334..4b8198e 100644
> --- a/hw/ide/ich.c
> +++ b/hw/ide/ich.c
> @@ -68,7 +68,7 @@
>  #include <hw/isa/isa.h>
>  #include "sysemu/block-backend.h"
>  #include "sysemu/dma.h"
> -
> +#include "qapi/error.h"
>  #include <hw/ide/pci.h>
>  #include <hw/ide/ahci.h>
>  
> @@ -111,6 +111,16 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp)
>      int sata_cap_offset;
>      uint8_t *sata_cap;
>      d = ICH_AHCI(dev);
> +    Error *err = NULL;
> +
> +    /* 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, &err);
> +    if (err) {
> +        /* Fall back to INTx silently */
> +        error_free(err);
> +    }

Tighter error checking:

       ret = msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false, NULL);
       /* Fall back to INTx silently on -ENOTSUP */
       assert(!ret || ret == -ENOTSUP);

More concise, too.  No need for the include "qapi/error.h" then.

>  
>      ahci_realize(&d->ahci, DEVICE(dev), pci_get_address_space(dev), 6);
>  
> @@ -142,11 +152,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);
>  }
>  

This is MSI device pattern #2: on whenever possible, else off.

>  static void pci_ich9_uninit(PCIDevice *dev)
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 7a38e47..9cff21c 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -32,6 +32,7 @@
>  #include "vmware_utils.h"
>  #include "vmxnet_tx_pkt.h"
>  #include "vmxnet_rx_pkt.h"
> +#include "qapi/error.h"
>  
>  #define PCI_DEVICE_ID_VMWARE_VMXNET3_REVISION 0x1
>  #define VMXNET3_MSIX_BAR_SIZE 0x2000
> @@ -2189,27 +2190,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)
>  {
> @@ -2271,10 +2251,15 @@ static uint8_t *vmxnet3_device_serial_num(VMXNET3State *s)
>      return dsnp;
>  }
>  
> +
> +#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);
> +    Error *err = NULL;
>  
>      VMW_CBPRN("Starting init...");
>  
> @@ -2298,12 +2283,19 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
>      /* Interrupt pin A */
>      pci_dev->config[PCI_INTERRUPT_PIN] = 0x01;
>  
> -    if (!vmxnet3_init_msix(s)) {
> -        VMW_WRPRN("Failed to initialize MSI-X, configuration is inconsistent.");
> +    msi_init(pci_dev, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS,
> +             VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK, &err);
> +    if (err) {
> +        /* Fall back to INTx silently */
> +        VMW_WRPRN("Failed to initialize MSI:  %s", error_get_pretty(err));
> +        error_free(err);
> +        s->msi_used = false;
> +    } else {
> +        s->msi_used = true;
>      }

Tighter error checking:

       ret = msi_init(..., NULL);
       /* Fall back to INTx silently on -ENOTSUP */
       assert(!ret || ret == -ENOTSUP);
       s->msi_used = !ret;

Can't see why we'd want to preserve the VMW_WRPRN() here.  If it's
interesting to know that this device falls back to MSI off, it's
interesting to know for other devices as well, and that means the debug
should go into msi_init().

But if you'd prefer to keep it here:

       ret = msi_init(..., &err);
       /* Fall back to INTx silently on -ENOTSUP */
       assert(!ret || ret == -ENOTSUP);
       if (ret) {
           VMW_WRPRN("Failed to initialize MSI:  %s", error_get_pretty(err));
           error_free(err);
       }
       s->msi_used = !ret;

Your choice.

>  
> -    if (!vmxnet3_init_msi(s)) {
> -        VMW_WRPRN("Failed to initialize MSI, configuration is inconsistent.");

The "configuration is inconsistent" part is useless.  Your patch
replaces it.  Good.

> +    if (!vmxnet3_init_msix(s)) {
> +        VMW_WRPRN("Failed to initialize MSI-X, configuration is inconsistent.");

It doesn't replace it here, but that's appropriate, because it doesn't
touch MSI-X code, it only moves it.

>      }
>  
>      vmxnet3_net_init(s);

Pattern #2.

> diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
> index b4a7806..c08cca3 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,9 @@ 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) {

Tighter error checking:

           assert(rc == -ENOTSUP);

> +        error_report_err(err);
>          goto err_bridge;
>      }
>  

This is MSI device pattern #3: on whenever possible, else error.

> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
> index fa0c50c..7f9131f 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,18 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>          goto slotid_error;
>      }
>  
> -    if ((bridge_dev->msi == ON_OFF_AUTO_AUTO ||
> -                bridge_dev->msi == ON_OFF_AUTO_ON) &&
> -        msi_nonbroken) {
> -        err = msi_init(dev, 0, 1, true, true);
> -        if (err < 0) {
> +    if (bridge_dev->msi != ON_OFF_AUTO_OFF) {

Unnecessary churn, please use != ON_OFF_AUTO_OFF in PATCH 10.

> +        /* it means SHPC exists */

Does it?  Why?

> +        err = msi_init(dev, 0, 1, true, true, &local_err);
> +        if (err < 0 && bridge_dev->msi == ON_OFF_AUTO_ON) {
> +            /* If user set msi=on, then device creation fail */
> +            error_append_hint(&local_err, "msi=on fails to create device."
> +                    " Please use msi=auto or off and try again");
> +            error_report_err(local_err);
>              goto msi_error;
> +        } else if (err < 0 && bridge_dev->msi == ON_OFF_AUTO_AUTO) {
> +            /* If user doesn`t set it on, switch to non-msi variant silently */
> +            error_free(local_err);
>          }

Comments on intel_hda_realize() apply.

>      }
>  

Pattern #1.

> diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
> index e6d653d..5f45fec 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,16 @@ 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) {
> +        error_report_err(err);
>          goto err_bridge;

Comment on ioh3420_initfn() applies.

>      }
>  

Pattern #2.

> diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c
> index d976844..3602bce 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,16 @@ 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) {
> +        error_report_err(err);
>          goto err_bridge;

Comment on ioh3420_initfn() applies.

>      }
>  

Pattern #2.

> diff --git a/hw/pci/msi.c b/hw/pci/msi.c
> index 97f35c0..ed0b38e 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)
> @@ -183,7 +184,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;
> @@ -191,6 +193,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;
>      }
>  
> @@ -214,7 +217,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 e71a28b..32ea65a 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"
> @@ -2330,6 +2330,7 @@ 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;
>  
>      pci_conf = dev->config;
>  
> @@ -2338,6 +2339,21 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
>      /* Interrupt pin 1 */
>      pci_conf[PCI_INTERRUPT_PIN] = 0x01;
>  
> +    if (megasas_use_msi(s)) {
> +        msi_init(dev, 0x50, 1, true, false, &err);
> +        if (err && s->msi == ON_OFF_AUTO_ON) {
> +            /* If user set msi=on, then device creation fail */
> +            s->msi = ON_OFF_AUTO_OFF;

This assignment...

> +            error_append_hint(&err, "msi=on fails to create device."
> +                    " Please use msi=auto or off and try again");
> +            error_propagate(errp, err);
> +            return;
> +        } else if (err && s->msi == ON_OFF_AUTO_AUTO) {
> +            /* If user doesn`t set it on, switch to non-msi variant silently */

... belongs here.

> +            error_free(err);
> +        }

Comments on intel_hda_realize() apply.

> +    }
> +
>      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,
> @@ -2345,10 +2361,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)) {

This is MSI device pattern #4: we have a property that lets the user
pick off, on or auto (on with silent fallback to off).  We overwrite
auto to off in fallback.

This pattern is problematic.  Say we create the device with msi=auto,
and realize changes it to msi=off.  If we unrealize, then realize again,
we won't use MSI even when we could.  We currenrly don't re-realize
device that way, and possible never will, but realize messing up device
configuration is unclean all the same.  Not this patch's fault, and not
for this series to clean up.

> diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
> index afee576..c013a07 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"
>  
> @@ -1274,10 +1274,29 @@ static void mptsas_scsi_realize(PCIDevice *dev, Error **errp)
>  {
>      DeviceState *d = DEVICE(dev);
>      MPTSASState *s = MPT_SAS(dev);
> +    Error *err = NULL;
>  
>      dev->config[PCI_LATENCY_TIMER] = 0;
>      dev->config[PCI_INTERRUPT_PIN] = 0x01;
>  
> +    if (s->msi != ON_OFF_AUTO_OFF) {
> +        msi_init(dev, 0, 1, true, false, &err);
> +        if (err && s->msi == ON_OFF_AUTO_ON) {
> +            /* If user set msi=on, then device creation fail */
> +            error_append_hint(&err, "msi=on fails to create device."
> +                    " Please use msi=auto or off and try again");
> +            error_propagate(errp, err);
> +            s->msi_in_use = false;
> +            return;
> +        } else if (err && s->msi == ON_OFF_AUTO_AUTO) {
> +            /* If user doesn`t set it on, switch to non-msi variant silently */
> +            error_free(err);
> +            s->msi_in_use = false;
> +        } else if (!err) {
> +            s->msi_in_use = true;
> +        }
> +    }
> +

Comments on intel_hda_realize() apply.

>      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,
> @@ -1285,11 +1304,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_AUTO || s->msi == ON_OFF_AUTO_ON) &&
> -        msi_init(dev, 0, 1, true, false) >= 0) {
> -        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);

Pattern #1.

> diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
> index c2a387a..b040575 100644
> --- a/hw/scsi/vmw_pvscsi.c
> +++ b/hw/scsi/vmw_pvscsi.c
> @@ -1044,12 +1044,16 @@ static void
>  pvscsi_init_msi(PVSCSIState *s)
>  {
>      int res;
> +    Error *err = NULL;
>      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, &err);
>      if (res < 0) {
>          trace_pvscsi_init_msi_fail(res);
> +        error_append_hint(&err, "MSI capability fail to init,"
> +                " will use INTx intead\n");
> +        error_report_err(err);
>          s->msi_used = false;
>      } else {
>          s->msi_used = true;

This is MSI device pattern #5: on whenever possible, else off, but
report an error when falling back to off.

Before your patch, this is pattern #2.  Why do you add error reporting
here?  You don't in other instances of pattern #2.

> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> index bbe5cca..d137ee2 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,21 @@ 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);
> +        if (ret < 0 && xhci->msi == ON_OFF_AUTO_ON) {
> +            /* If user set msi=on, then device creation fail */
> +            error_append_hint(&err, "msi=on fails to create device."
> +                    " Please use msi=auto or off and try again");
> +            error_propagate(errp, err);
> +            return;
> +        }
> +        else if (ret < 0 && xhci->msi == ON_OFF_AUTO_AUTO) {

No line break between } and else.  Please use scripts/checkpatch.pl.

> +            /* If user doesn`t set it on, switch to non-msi variant silently */
> +            error_free(err);
> +        }

Comments on intel_hda_realize() apply.

> +    }
> +
>      if (xhci->numintrs > MAXINTRS) {
>          xhci->numintrs = MAXINTRS;
>      }
> @@ -3648,10 +3665,6 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
>          assert(ret >= 0);
>      }
>  
> -    if (xhci->msi == ON_OFF_AUTO_ON ||
> -        xhci->msi == ON_OFF_AUTO_AUTO) {
> -        msi_init(dev, 0x70, xhci->numintrs, true, false);
> -    }
>      if (xhci->msix == ON_OFF_AUTO_ON ||
>          xhci->msix == ON_OFF_AUTO_AUTO) {
>          msix_init(dev, xhci->numintrs,

Pattern #1.

> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index d091d8c..67d93f7 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);

This is not an assertion, because bad host hardware can trigger it.
Okay.

>          return ret;
>      }
>      vdev->msi_cap_size = 0xa + (msi_maskbit ? 0xa : 0) + (msi_64bit ? 0x4 : 0);

Otherwise pattern #2.

> 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);