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

[Markus Armbruster]

Cao jin <address@hidden> writes:

> 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>
>
> Signed-off-by: Cao jin <address@hidden>
> ---
>  hw/audio/intel-hda.c               | 25 +++++++++++++++++++++----
>  hw/ide/ich.c                       | 15 +++++++++------
>  hw/net/vmxnet3.c                   | 38 
> +++++++++++++-------------------------
>  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                  | 27 ++++++++++++++++++++++-----
>  hw/scsi/mptsas.c                   | 31 ++++++++++++++++++++++++-------
>  hw/scsi/vmw_pvscsi.c               |  4 +++-
>  hw/usb/hcd-xhci.c                  | 23 +++++++++++++++++++----
>  hw/vfio/pci.c                      |  7 +++++--
>  include/hw/pci/msi.h               |  3 ++-
>  14 files changed, 157 insertions(+), 65 deletions(-)
>
> diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
> index 40a8772..d588fd5 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,8 @@ static void intel_hda_realize(PCIDevice *pci, Error 
> **errp)
>  {
>      IntelHDAState *d = INTEL_HDA(pci);
>      uint8_t *conf = d->pci.config;
> +    Error *err = NULL;
> +    int32_t ret;

Make this plain int, because that's what msi_init() returns.

>  
>      d->name = object_get_typename(OBJECT(d));
>  
> @@ -1139,13 +1142,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);
[...]