Re: [Qemu-devel] [PATCH v4 02/17] vfio/pci: Pass an error object to vfio_populate_vga

[Markus Armbruster]

Eric Auger <address@hidden> writes:

> Pass an error object to prepare for the same operation in
> vfio_populate_device. Eventually this contributes to the migration
> to VFIO-PCI realize.
>
> We now report an error on vfio_get_region_info failure.
>
> vfio_probe_igd_bar4_quirk is not involved in the migration to realize
> and simply calls error_reportf_err.
>
> Signed-off-by: Eric Auger <address@hidden>
>
> ---
>
> v3: creation
> ---
>  hw/vfio/pci-quirks.c |  4 +++-
>  hw/vfio/pci.c        | 19 ++++++++++++-------
>  hw/vfio/pci.h        |  2 +-
>  3 files changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
> index bec694c..806ea5d 100644
> --- a/hw/vfio/pci-quirks.c
> +++ b/hw/vfio/pci-quirks.c
> @@ -1363,6 +1363,7 @@ static void vfio_probe_igd_bar4_quirk(VFIOPCIDevice 
> *vdev, int nr)
>      uint64_t *bdsm_size;
>      uint32_t gmch;
>      uint16_t cmd_orig, cmd;
> +    Error *err = NULL;
>  
>      /*
>       * This must be an Intel VGA device at address 00:02.0 for us to even
> @@ -1464,7 +1465,8 @@ static void vfio_probe_igd_bar4_quirk(VFIOPCIDevice 
> *vdev, int nr)
>       * try to enable it.  Probably shouldn't be using legacy mode without 
> VGA,
>       * but also no point in us enabling VGA if disabled in hardware.
>       */
> -    if (!(gmch & 0x2) && !vdev->vga && vfio_populate_vga(vdev)) {
> +    if (!(gmch & 0x2) && !vdev->vga && vfio_populate_vga(vdev, &err)) {
> +        error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name);
>          error_report("IGD device %s failed to enable VGA access, "
>                       "legacy mode disabled", vdev->vbasedev.name);
>          goto out;
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 417bf7f..db08dd3 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2134,7 +2134,7 @@ static VFIODeviceOps vfio_pci_ops = {
>      .vfio_eoi = vfio_intx_eoi,
>  };
>  
> -int vfio_populate_vga(VFIOPCIDevice *vdev)
> +int vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp)
>  {
>      VFIODevice *vbasedev = &vdev->vbasedev;
>      struct vfio_region_info *reg_info;
> @@ -2142,15 +2142,18 @@ int vfio_populate_vga(VFIOPCIDevice *vdev)
>  
>      ret = vfio_get_region_info(vbasedev, VFIO_PCI_VGA_REGION_INDEX, 
> &reg_info);
>      if (ret) {
> +        error_setg_errno(errp, -ret,
> +                         "failed getting region info for VGA region index 
> %d",
> +                         VFIO_PCI_VGA_REGION_INDEX);
>          return ret;
>      }
>  
>      if (!(reg_info->flags & VFIO_REGION_INFO_FLAG_READ) ||
>          !(reg_info->flags & VFIO_REGION_INFO_FLAG_WRITE) ||
>          reg_info->size < 0xbffff + 1) {
> -        error_report("vfio: Unexpected VGA info, flags 0x%lx, size 0x%lx",
> -                     (unsigned long)reg_info->flags,
> -                     (unsigned long)reg_info->size);
> +        error_setg(errp, "unexpected VGA info, flags 0x%lx, size 0x%lx",
> +                   (unsigned long)reg_info->flags,
> +                   (unsigned long)reg_info->size);
>          g_free(reg_info);
>          return -EINVAL;
>      }
> @@ -2205,6 +2208,7 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
>      struct vfio_region_info *reg_info;
>      struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info) };
>      int i, ret = -1;
> +    Error *err = NULL;
>  
>      /* Sanity check device */
>      if (!(vbasedev->flags & VFIO_DEVICE_FLAGS_PCI)) {
> @@ -2259,10 +2263,11 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
>      g_free(reg_info);
>  
>      if (vdev->features & VFIO_FEATURE_ENABLE_VGA) {
> -        ret = vfio_populate_vga(vdev);
> +        ret = vfio_populate_vga(vdev, &err);
>          if (ret) {
> -            error_report(
> -                "vfio: Device does not support requested feature x-vga");
> +            error_append_hint(&err, "device does not support "
> +                              "requested feature x-vga");

The hint lacks a newline.  You fix this in PATCH 03.  Could perhaps be
touched up on commit.

> +            error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name);
>              goto error;
>          }
>      }
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index 7d482d9..87a62f9 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -161,7 +161,7 @@ void vfio_bar_quirk_exit(VFIOPCIDevice *vdev, int nr);
>  void vfio_bar_quirk_finalize(VFIOPCIDevice *vdev, int nr);
>  void vfio_setup_resetfn_quirk(VFIOPCIDevice *vdev);
>  
> -int vfio_populate_vga(VFIOPCIDevice *vdev);
> +int vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp);
>  
>  int vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev,
>                                 struct vfio_region_info *info);