Re: [Qemu-devel] [PATCH v2 07/12] vfio: Pass an error object to vfio_get_group

[Markus Armbruster]

Eric Auger <address@hidden> writes:

> Pass an error object to prepare for migration to VFIO-PCI realize.
>
> Signed-off-by: Eric Auger <address@hidden>
>
> ---
>
> v2: creation
> ---
>  hw/vfio/common.c              | 20 +++++++++++---------
>  hw/vfio/pci.c                 |  3 +--
>  hw/vfio/platform.c            | 11 ++++++++---
>  include/hw/vfio/vfio-common.h |  2 +-
>  4 files changed, 21 insertions(+), 15 deletions(-)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index b313e7c..ef9e4cd 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -34,6 +34,7 @@
>  #include "qemu/range.h"
>  #include "sysemu/kvm.h"
>  #include "trace.h"
> +#include "qapi/error.h"
>  
>  struct vfio_group_head vfio_group_list =
>      QLIST_HEAD_INITIALIZER(vfio_group_list);
> @@ -1115,7 +1116,7 @@ static void vfio_disconnect_container(VFIOGroup *group)
>      }
>  }
>  
> -VFIOGroup *vfio_get_group(int groupid, AddressSpace *as)
> +VFIOGroup *vfio_get_group(int groupid, AddressSpace *as, Error **errp)
>  {
>      VFIOGroup *group;
>      char path[32];
> @@ -1127,8 +1128,8 @@ VFIOGroup *vfio_get_group(int groupid, AddressSpace *as)
>              if (group->container->space->as == as) {
>                  return group;
>              } else {
> -                error_report("vfio: group %d used in multiple address 
> spaces",
> -                             group->groupid);
> +                error_setg(errp, "group %d used in multiple address spaces",
> +                           group->groupid);
>                  return NULL;
>              }
>          }
> @@ -1139,19 +1140,20 @@ VFIOGroup *vfio_get_group(int groupid, AddressSpace 
> *as)
>      snprintf(path, sizeof(path), "/dev/vfio/%d", groupid);
>      group->fd = qemu_open(path, O_RDWR);
>      if (group->fd < 0) {
> -        error_report("vfio: error opening %s: %m", path);
> +        error_setg_errno(errp, errno, "error opening %s", path);
>          goto free_group_exit;
>      }
>  
>      if (ioctl(group->fd, VFIO_GROUP_GET_STATUS, &status)) {
> -        error_report("vfio: error getting group status: %m");
> +        error_setg_errno(errp, errno, "error getting group %d status", 
> groupid);
>          goto close_fd_exit;
>      }
>  
>      if (!(status.flags & VFIO_GROUP_FLAGS_VIABLE)) {
> -        error_report("vfio: error, group %d is not viable, please ensure "
> -                     "all devices within the iommu_group are bound to their "
> -                     "vfio bus driver.", groupid);
> +        error_setg(errp, "group %d is not viable", groupid);
> +        error_append_hint(errp,
> +                          "Please ensure all devices within the iommu_group "
> +                          "are bound to their vfio bus driver.\n");
>          goto close_fd_exit;
>      }
>  
> @@ -1159,7 +1161,7 @@ VFIOGroup *vfio_get_group(int groupid, AddressSpace *as)
>      QLIST_INIT(&group->device_list);
>  
>      if (vfio_connect_container(group, as)) {
> -        error_report("vfio: failed to setup container for group %d", 
> groupid);
> +        error_setg(errp, "failed to setup container for group %d", groupid);
>          goto close_fd_exit;
>      }
>  
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index f9a4fe7..1173d4a 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2562,9 +2562,8 @@ static int vfio_initfn(PCIDevice *pdev)
>  
>      trace_vfio_initfn(vdev->vbasedev.name, groupid);
>  
> -    group = vfio_get_group(groupid, pci_device_iommu_address_space(pdev));
> +    group = vfio_get_group(groupid, pci_device_iommu_address_space(pdev), 
> &err);
>      if (!group) {
> -        error_setg(&err, "failed to get group %d", groupid);
>          ret = -ENOENT;
>          goto error;
>      }

By now you can see the conversion running step by step like clockwork :)

> diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
> index a559e7b..7bf525b 100644
> --- a/hw/vfio/platform.c
> +++ b/hw/vfio/platform.c
> @@ -552,6 +552,7 @@ static int vfio_base_device_init(VFIODevice *vbasedev)
>      ssize_t len;
>      struct stat st;
>      int groupid;
> +    Error *err = NULL;
>      int ret;
>  
>      /* @sysfsdev takes precedence over @host */
> @@ -592,10 +593,10 @@ static int vfio_base_device_init(VFIODevice *vbasedev)
>  
>      trace_vfio_platform_base_device_init(vbasedev->name, groupid);
>  
> -    group = vfio_get_group(groupid, &address_space_memory);
> +    group = vfio_get_group(groupid, &address_space_memory, &err);
>      if (!group) {
> -        error_report("vfio: failed to get group %d", groupid);
> -        return -ENOENT;
> +        ret = -ENOENT;
> +        goto error;
>      }
>  
>      QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
> @@ -619,6 +620,10 @@ static int vfio_base_device_init(VFIODevice *vbasedev)
>          vfio_put_group(group);
>      }
>  
> +error:
> +    if (err) {
> +        error_reportf_err(err, ERR_PREFIX, vbasedev->name);
> +    }
>      return ret;
>  }

Hmm, vfio_base_device_init() is called by realize method
vfio_platform_realize().  It should therefore error_propagate() instead
of error_reportf_err()!

>  
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index fd19880..4fb6fc3 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -155,7 +155,7 @@ void vfio_region_mmaps_set_enabled(VFIORegion *region, 
> bool enabled);
>  void vfio_region_exit(VFIORegion *region);
>  void vfio_region_finalize(VFIORegion *region);
>  void vfio_reset_handler(void *opaque);
> -VFIOGroup *vfio_get_group(int groupid, AddressSpace *as);
> +VFIOGroup *vfio_get_group(int groupid, AddressSpace *as, Error **errp);
>  void vfio_put_group(VFIOGroup *group);
>  int vfio_get_device(VFIOGroup *group, const char *name,
>                      VFIODevice *vbasedev);