diff mbox

[v2,07/12] vfio: Pass an error object to vfio_get_group

Message ID 1474404352-28958-8-git-send-email-eric.auger@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Eric Auger Sept. 20, 2016, 8:45 p.m. UTC 
Pass an error object to prepare for migration to VFIO-PCI realize.

Signed-off-by: Eric Auger <eric.auger@redhat.com>

---

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(-)

Comments

Markus Armbruster Sept. 22, 2016, 5:01 p.m. UTC | #1 
Eric Auger <eric.auger@redhat.com> writes:

> Pass an error object to prepare for migration to VFIO-PCI realize.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>
> ---
>
> 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);
Eric Auger Sept. 30, 2016, 1:34 p.m. UTC | #2 
Hi Markus,

On 22/09/2016 19:01, Markus Armbruster wrote:
> Eric Auger <eric.auger@redhat.com> writes:
> 
>> Pass an error object to prepare for migration to VFIO-PCI realize.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
>>
>> 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);
>
diff mbox

Patch

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