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>
[...]
> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index 26119bf..5c57106 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,8 @@ 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;
> +    int ret;
>  
>      pci_conf = dev->config;
>  
> @@ -2338,6 +2340,25 @@ static void megasas_scsi_realize(PCIDevice *dev, Error 
> **errp)
>      /* Interrupt pin 1 */
>      pci_conf[PCI_INTERRUPT_PIN] = 0x01;
>  
> +    if (megasas_use_msi(s)) {
> +        ret = msi_init(dev, 0x50, 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 && s->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;
> +        } else if (ret) {
> +            s->msi = ON_OFF_AUTO_OFF;
> +        }
> +
> +        /* With msi=auto, we fall back to MSI off silently */
> +        error_free(err);

Let's move this into the "else if (ret)" conditional, where the comment
serves to explain the s->msi = ON_OFF_AUTO_OFF.

> +    }
> +
>      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 +2366,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)) {
> -        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)) {
[...]
> diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
> index c2a387a..c384442 100644
> --- a/hw/scsi/vmw_pvscsi.c
> +++ b/hw/scsi/vmw_pvscsi.c
> @@ -1044,12 +1044,14 @@ 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_free(err);
>          s->msi_used = false;
>      } else {
>          s->msi_used = true;

Since you're not doing anything with err:

   pvscsi_init_msi(PVSCSIState *s)
   {
       int res;
       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, NULL);
       if (res < 0) {
           trace_pvscsi_init_msi_fail(res);
           s->msi_used = false;
       } else {
           s->msi_used = true;

[...]