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

[Auger Eric]

Hi Markus,

On 22/09/2016 19:01, Markus Armbruster wrote:
> 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 :)
hopefully!
> 
>> 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()!
I added separate patches to properly handle error propagation in
vfio_base_device_init up to vfio_platform_realize since error reporting
is wrong in the vfio_platform device although realize is in place.

Thanks

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