[Qemu-devel] [PATCH v2 00/12] Convert VFIO-PCI to realize

[Qemu-devel] [PATCH v2 00/12] Convert VFIO-PCI to realize

[Eric Auger]

This series converts VFIO-PCI to realize. It also aims at improving
the error reporting in case of QMP hot-plug.

Before the series, a device_add failure would have reported:
{"error": {"class": "GenericError", "desc": "Device initialization
failed"}}.

Now the actual error cause is reported.

Git Information:
https://github.com/eauger/qemu/tree/v2.7.0_vfio_realize_v2

History:
v1 -> v2:
- Error object is cascaded downto vfio_intx_enable, vfio_add_capabilities
  vfio_pci_igd_opregion_init, vfio_get_group, vfio_get_device
- Actual migration to realize happens at a later stage following Markus'
  guidance
- special handling of the case where the end-user omits the host property
- error_setg_errno now uses positive errno
- more detailed commit message
- fix "cannot enable AER cannot" functional change

Eric Auger (12):
  vfio/pci: Use local error object in vfio_initfn
  vfio/pci: Pass an error object to vfio_populate_device
  vfio/pci: Pass an error object to vfio_msix_early_setup
  vfio/pci: Pass an error object to vfio_intx_enable
  vfio/pci: Pass an error object to vfio_add_capabilities
  vfio/pci: Pass an error object to vfio_pci_igd_opregion_init
  vfio: Pass an error object to vfio_get_group
  vfio: Pass an error object to vfio_get_device
  vfio/pci: Conversion to realize
  vfio/pci: Remove vfio_msix_early_setup returned value
  vfio/pci: Remove vfio_populate_device returned value
  vfio/pci: Handle host oversight

 hw/vfio/common.c              |  33 +++---
 hw/vfio/pci-quirks.c          |  19 ++-
 hw/vfio/pci.c                 | 268 +++++++++++++++++++++++-------------------
 hw/vfio/pci.h                 |   5 +-
 hw/vfio/platform.c            |  16 ++-
 hw/vfio/trace-events          |   2 +-
 include/hw/vfio/vfio-common.h |   7 +-
 7 files changed, 196 insertions(+), 154 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH v2 01/12] vfio/pci: Use local error object in vfio_initfn

[Eric Auger]

To prepare for migration to realize, let's use a local error
object in vfio_initfn. Also let's use the same error prefix for all
error messages.

On top of the 1-1 conversion, we start using a common error prefix for
all error messages. We also introduce a similar warning prefix which will
be used later on.

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

v2: creation
---
 hw/vfio/pci.c                 | 73 +++++++++++++++++++++++++------------------
 include/hw/vfio/vfio-common.h |  3 ++
 2 files changed, 46 insertions(+), 30 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index a5a620a..ca0293d 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2493,6 +2493,7 @@ static int vfio_initfn(PCIDevice *pdev)
     VFIODevice *vbasedev_iter;
     VFIOGroup *group;
     char *tmp, group_path[PATH_MAX], *group_name;
+    Error *err = NULL;
     ssize_t len;
     struct stat st;
     int groupid;
@@ -2506,9 +2507,9 @@ static int vfio_initfn(PCIDevice *pdev)
     }
 
     if (stat(vdev->vbasedev.sysfsdev, &st) < 0) {
-        error_report("vfio: error: no such host device: %s",
-                     vdev->vbasedev.sysfsdev);
-        return -errno;
+        error_setg(&err, "no such host device");
+        ret = -errno;
+        goto error;
     }
 
     vdev->vbasedev.name = g_strdup(basename(vdev->vbasedev.sysfsdev));
@@ -2520,40 +2521,44 @@ static int vfio_initfn(PCIDevice *pdev)
     g_free(tmp);
 
     if (len <= 0 || len >= sizeof(group_path)) {
-        error_report("vfio: error no iommu_group for device");
-        return len < 0 ? -errno : -ENAMETOOLONG;
+        error_setg_errno(&err, len < 0 ? errno : ENAMETOOLONG,
+                         "no iommu_group found");
+        ret = len < 0 ? -errno : -ENAMETOOLONG;
+        goto error;
     }
 
     group_path[len] = 0;
 
     group_name = basename(group_path);
     if (sscanf(group_name, "%d", &groupid) != 1) {
-        error_report("vfio: error reading %s: %m", group_path);
-        return -errno;
+        error_setg_errno(&err, errno, "failed to read %s", group_path);
+        ret = -errno;
+        goto error;
     }
 
     trace_vfio_initfn(vdev->vbasedev.name, groupid);
 
     group = vfio_get_group(groupid, pci_device_iommu_address_space(pdev));
     if (!group) {
-        error_report("vfio: failed to get group %d", groupid);
-        return -ENOENT;
+        error_setg(&err, "failed to get group %d", groupid);
+        ret = -ENOENT;
+        goto error;
     }
 
     QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
         if (strcmp(vbasedev_iter->name, vdev->vbasedev.name) == 0) {
-            error_report("vfio: error: device %s is already attached",
-                         vdev->vbasedev.name);
+            error_setg(&err, "device is already attached");
             vfio_put_group(group);
-            return -EBUSY;
+            ret = -EBUSY;
+            goto error;
         }
     }
 
     ret = vfio_get_device(group, vdev->vbasedev.name, &vdev->vbasedev);
     if (ret) {
-        error_report("vfio: failed to get device %s", vdev->vbasedev.name);
+        error_setg_errno(&err, -ret, "failed to get device");
         vfio_put_group(group);
-        return ret;
+        goto error;
     }
 
     ret = vfio_populate_device(vdev);
@@ -2567,8 +2572,8 @@ static int vfio_initfn(PCIDevice *pdev)
                 vdev->config_offset);
     if (ret < (int)MIN(pci_config_size(&vdev->pdev), vdev->config_size)) {
         ret = ret < 0 ? -errno : -EFAULT;
-        error_report("vfio: Failed to read device config space");
-        return ret;
+        error_setg_errno(&err, -ret, "failed to read device config space");
+        goto error;
     }
 
     /* vfio emulates a lot for us, but some bits need extra love */
@@ -2584,8 +2589,9 @@ static int vfio_initfn(PCIDevice *pdev)
      */
     if (vdev->vendor_id != PCI_ANY_ID) {
         if (vdev->vendor_id >= 0xffff) {
-            error_report("vfio: Invalid PCI vendor ID provided");
-            return -EINVAL;
+            error_setg(&err, "invalid PCI vendor ID provided");
+            ret = -EINVAL;
+            goto error;
         }
         vfio_add_emulated_word(vdev, PCI_VENDOR_ID, vdev->vendor_id, ~0);
         trace_vfio_pci_emulated_vendor_id(vdev->vbasedev.name, vdev->vendor_id);
@@ -2595,8 +2601,9 @@ static int vfio_initfn(PCIDevice *pdev)
 
     if (vdev->device_id != PCI_ANY_ID) {
         if (vdev->device_id > 0xffff) {
-            error_report("vfio: Invalid PCI device ID provided");
-            return -EINVAL;
+            error_setg(&err, "invalid PCI device ID provided");
+            ret = -EINVAL;
+            goto error;
         }
         vfio_add_emulated_word(vdev, PCI_DEVICE_ID, vdev->device_id, ~0);
         trace_vfio_pci_emulated_device_id(vdev->vbasedev.name, vdev->device_id);
@@ -2606,8 +2613,9 @@ static int vfio_initfn(PCIDevice *pdev)
 
     if (vdev->sub_vendor_id != PCI_ANY_ID) {
         if (vdev->sub_vendor_id > 0xffff) {
-            error_report("vfio: Invalid PCI subsystem vendor ID provided");
-            return -EINVAL;
+            error_setg(&err, "invalid PCI subsystem vendor ID provided");
+            ret = -EINVAL;
+            goto error;
         }
         vfio_add_emulated_word(vdev, PCI_SUBSYSTEM_VENDOR_ID,
                                vdev->sub_vendor_id, ~0);
@@ -2617,8 +2625,9 @@ static int vfio_initfn(PCIDevice *pdev)
 
     if (vdev->sub_device_id != PCI_ANY_ID) {
         if (vdev->sub_device_id > 0xffff) {
-            error_report("vfio: Invalid PCI subsystem device ID provided");
-            return -EINVAL;
+            error_setg(&err, "invalid PCI subsystem device ID provided");
+            ret = -EINVAL;
+            goto error;
         }
         vfio_add_emulated_word(vdev, PCI_SUBSYSTEM_ID, vdev->sub_device_id, ~0);
         trace_vfio_pci_emulated_sub_device_id(vdev->vbasedev.name,
@@ -2671,8 +2680,9 @@ static int vfio_initfn(PCIDevice *pdev)
         struct vfio_region_info *opregion;
 
         if (vdev->pdev.qdev.hotplugged) {
-            error_report("Cannot support IGD OpRegion feature on hotplugged "
-                         "device %s", vdev->vbasedev.name);
+            error_setg(&err,
+                       "cannot support IGD OpRegion feature on hotplugged "
+                       "device");
             ret = -EINVAL;
             goto out_teardown;
         }
@@ -2681,16 +2691,15 @@ static int vfio_initfn(PCIDevice *pdev)
                         VFIO_REGION_TYPE_PCI_VENDOR_TYPE | PCI_VENDOR_ID_INTEL,
                         VFIO_REGION_SUBTYPE_INTEL_IGD_OPREGION, &opregion);
         if (ret) {
-            error_report("Device %s does not support requested IGD OpRegion "
-                         "feature", vdev->vbasedev.name);
+            error_setg_errno(&err, -ret,
+                             "does not support requested IGD OpRegion feature");
             goto out_teardown;
         }
 
         ret = vfio_pci_igd_opregion_init(vdev, opregion);
         g_free(opregion);
         if (ret) {
-            error_report("Device %s IGD OpRegion initialization failed",
-                         vdev->vbasedev.name);
+            error_setg_errno(&err, -ret, "IGD OpRegion initialization failed");
             goto out_teardown;
         }
     }
@@ -2726,6 +2735,10 @@ out_teardown:
     pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
     vfio_teardown_msi(vdev);
     vfio_bars_exit(vdev);
+error:
+    if (err) {
+        error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name);
+    }
     return ret;
 }
 
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 94dfae3..fd19880 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -30,6 +30,9 @@
 #include <linux/vfio.h>
 #endif
 
+#define ERR_PREFIX "vfio error: %s: "
+#define WARN_PREFIX "vfio warning: %s: "
+
 /*#define DEBUG_VFIO*/
 #ifdef DEBUG_VFIO
 #define DPRINTF(fmt, ...) \
-- 
1.9.1

Re: [Qemu-devel] [PATCH v2 01/12] vfio/pci: Use local error object in vfio_initfn

[Markus Armbruster]

Eric Auger <eric.auger@redhat.com> writes:

> To prepare for migration to realize, let's use a local error
> object in vfio_initfn. Also let's use the same error prefix for all
> error messages.
>
> On top of the 1-1 conversion, we start using a common error prefix for
> all error messages. We also introduce a similar warning prefix which will
> be used later on.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>
> v2: creation
> ---
>  hw/vfio/pci.c                 | 73 +++++++++++++++++++++++++------------------
>  include/hw/vfio/vfio-common.h |  3 ++
>  2 files changed, 46 insertions(+), 30 deletions(-)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index a5a620a..ca0293d 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2493,6 +2493,7 @@ static int vfio_initfn(PCIDevice *pdev)
>      VFIODevice *vbasedev_iter;
>      VFIOGroup *group;
>      char *tmp, group_path[PATH_MAX], *group_name;
> +    Error *err = NULL;
>      ssize_t len;
>      struct stat st;
>      int groupid;
> @@ -2506,9 +2507,9 @@ static int vfio_initfn(PCIDevice *pdev)
>      }
>  
>      if (stat(vdev->vbasedev.sysfsdev, &st) < 0) {
> -        error_report("vfio: error: no such host device: %s",
> -                     vdev->vbasedev.sysfsdev);
> -        return -errno;
> +        error_setg(&err, "no such host device");

Any particular reason you don't use error_setg_errno() here?

> +        ret = -errno;
> +        goto error;
>      }
>  
>      vdev->vbasedev.name = g_strdup(basename(vdev->vbasedev.sysfsdev));
> @@ -2520,40 +2521,44 @@ static int vfio_initfn(PCIDevice *pdev)
>      g_free(tmp);
>  
>      if (len <= 0 || len >= sizeof(group_path)) {
> -        error_report("vfio: error no iommu_group for device");
> -        return len < 0 ? -errno : -ENAMETOOLONG;
> +        error_setg_errno(&err, len < 0 ? errno : ENAMETOOLONG,
> +                         "no iommu_group found");
> +        ret = len < 0 ? -errno : -ENAMETOOLONG;

Suggest

           ret = len < 0 ? -errno : -ENAMETOOLONG;
           error_setg_errno(&err, -ret, "no iommu_group found");

> +        goto error;
>      }
>  
>      group_path[len] = 0;
>  
>      group_name = basename(group_path);
>      if (sscanf(group_name, "%d", &groupid) != 1) {
> -        error_report("vfio: error reading %s: %m", group_path);
> -        return -errno;
> +        error_setg_errno(&err, errno, "failed to read %s", group_path);
> +        ret = -errno;
> +        goto error;
>      }
>  
>      trace_vfio_initfn(vdev->vbasedev.name, groupid);
>  
>      group = vfio_get_group(groupid, pci_device_iommu_address_space(pdev));
>      if (!group) {
> -        error_report("vfio: failed to get group %d", groupid);
> -        return -ENOENT;
> +        error_setg(&err, "failed to get group %d", groupid);
> +        ret = -ENOENT;
> +        goto error;
>      }
>  
>      QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
>          if (strcmp(vbasedev_iter->name, vdev->vbasedev.name) == 0) {
> -            error_report("vfio: error: device %s is already attached",
> -                         vdev->vbasedev.name);
> +            error_setg(&err, "device is already attached");
>              vfio_put_group(group);
> -            return -EBUSY;
> +            ret = -EBUSY;
> +            goto error;
>          }
>      }
>  
>      ret = vfio_get_device(group, vdev->vbasedev.name, &vdev->vbasedev);
>      if (ret) {
> -        error_report("vfio: failed to get device %s", vdev->vbasedev.name);
> +        error_setg_errno(&err, -ret, "failed to get device");
>          vfio_put_group(group);
> -        return ret;
> +        goto error;
>      }
>  
>      ret = vfio_populate_device(vdev);
> @@ -2567,8 +2572,8 @@ static int vfio_initfn(PCIDevice *pdev)
>                  vdev->config_offset);
>      if (ret < (int)MIN(pci_config_size(&vdev->pdev), vdev->config_size)) {
>          ret = ret < 0 ? -errno : -EFAULT;
> -        error_report("vfio: Failed to read device config space");
> -        return ret;
> +        error_setg_errno(&err, -ret, "failed to read device config space");
> +        goto error;
>      }
>  
>      /* vfio emulates a lot for us, but some bits need extra love */
> @@ -2584,8 +2589,9 @@ static int vfio_initfn(PCIDevice *pdev)
>       */
>      if (vdev->vendor_id != PCI_ANY_ID) {
>          if (vdev->vendor_id >= 0xffff) {
> -            error_report("vfio: Invalid PCI vendor ID provided");
> -            return -EINVAL;
> +            error_setg(&err, "invalid PCI vendor ID provided");
> +            ret = -EINVAL;
> +            goto error;
>          }
>          vfio_add_emulated_word(vdev, PCI_VENDOR_ID, vdev->vendor_id, ~0);
>          trace_vfio_pci_emulated_vendor_id(vdev->vbasedev.name, vdev->vendor_id);
> @@ -2595,8 +2601,9 @@ static int vfio_initfn(PCIDevice *pdev)
>  
>      if (vdev->device_id != PCI_ANY_ID) {
>          if (vdev->device_id > 0xffff) {
> -            error_report("vfio: Invalid PCI device ID provided");
> -            return -EINVAL;
> +            error_setg(&err, "invalid PCI device ID provided");
> +            ret = -EINVAL;
> +            goto error;
>          }
>          vfio_add_emulated_word(vdev, PCI_DEVICE_ID, vdev->device_id, ~0);
>          trace_vfio_pci_emulated_device_id(vdev->vbasedev.name, vdev->device_id);
> @@ -2606,8 +2613,9 @@ static int vfio_initfn(PCIDevice *pdev)
>  
>      if (vdev->sub_vendor_id != PCI_ANY_ID) {
>          if (vdev->sub_vendor_id > 0xffff) {
> -            error_report("vfio: Invalid PCI subsystem vendor ID provided");
> -            return -EINVAL;
> +            error_setg(&err, "invalid PCI subsystem vendor ID provided");
> +            ret = -EINVAL;
> +            goto error;
>          }
>          vfio_add_emulated_word(vdev, PCI_SUBSYSTEM_VENDOR_ID,
>                                 vdev->sub_vendor_id, ~0);
> @@ -2617,8 +2625,9 @@ static int vfio_initfn(PCIDevice *pdev)
>  
>      if (vdev->sub_device_id != PCI_ANY_ID) {
>          if (vdev->sub_device_id > 0xffff) {
> -            error_report("vfio: Invalid PCI subsystem device ID provided");
> -            return -EINVAL;
> +            error_setg(&err, "invalid PCI subsystem device ID provided");
> +            ret = -EINVAL;
> +            goto error;
>          }
>          vfio_add_emulated_word(vdev, PCI_SUBSYSTEM_ID, vdev->sub_device_id, ~0);
>          trace_vfio_pci_emulated_sub_device_id(vdev->vbasedev.name,
> @@ -2671,8 +2680,9 @@ static int vfio_initfn(PCIDevice *pdev)
>          struct vfio_region_info *opregion;
>  
>          if (vdev->pdev.qdev.hotplugged) {
> -            error_report("Cannot support IGD OpRegion feature on hotplugged "
> -                         "device %s", vdev->vbasedev.name);
> +            error_setg(&err,
> +                       "cannot support IGD OpRegion feature on hotplugged "
> +                       "device");
>              ret = -EINVAL;
>              goto out_teardown;
>          }
> @@ -2681,16 +2691,15 @@ static int vfio_initfn(PCIDevice *pdev)
>                          VFIO_REGION_TYPE_PCI_VENDOR_TYPE | PCI_VENDOR_ID_INTEL,
>                          VFIO_REGION_SUBTYPE_INTEL_IGD_OPREGION, &opregion);
>          if (ret) {
> -            error_report("Device %s does not support requested IGD OpRegion "
> -                         "feature", vdev->vbasedev.name);
> +            error_setg_errno(&err, -ret,
> +                             "does not support requested IGD OpRegion feature");
>              goto out_teardown;
>          }
>  
>          ret = vfio_pci_igd_opregion_init(vdev, opregion);
>          g_free(opregion);
>          if (ret) {
> -            error_report("Device %s IGD OpRegion initialization failed",
> -                         vdev->vbasedev.name);
> +            error_setg_errno(&err, -ret, "IGD OpRegion initialization failed");
>              goto out_teardown;
>          }
>      }
> @@ -2726,6 +2735,10 @@ out_teardown:
>      pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
>      vfio_teardown_msi(vdev);
>      vfio_bars_exit(vdev);
> +error:
> +    if (err) {
> +        error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name);
> +    }
>      return ret;
>  }

The value assigned to ret doesn't matter as long as it's negative, which
makes me wonder why we bother with different values here.  Peeking
ahead, I see you simplify this already as part of your conversion to
realize().  Okay, that works for me.

>  
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 94dfae3..fd19880 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -30,6 +30,9 @@
>  #include <linux/vfio.h>
>  #endif
>  
> +#define ERR_PREFIX "vfio error: %s: "
> +#define WARN_PREFIX "vfio warning: %s: "
> +
>  /*#define DEBUG_VFIO*/
>  #ifdef DEBUG_VFIO
>  #define DPRINTF(fmt, ...) \

We already discussed error message prefixes and errno strings.  I got my
taste, you got yours.  Since this is your code, yours wins :)

Re: [Qemu-devel] [PATCH v2 01/12] vfio/pci: Use local error object in vfio_initfn

[Auger Eric]

Hi Markus,

On 22/09/2016 17:42, Markus Armbruster wrote:
> Eric Auger <eric.auger@redhat.com> writes:
> 
>> To prepare for migration to realize, let's use a local error
>> object in vfio_initfn. Also let's use the same error prefix for all
>> error messages.
>>
>> On top of the 1-1 conversion, we start using a common error prefix for
>> all error messages. We also introduce a similar warning prefix which will
>> be used later on.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>
>> v2: creation
>> ---
>>  hw/vfio/pci.c                 | 73 +++++++++++++++++++++++++------------------
>>  include/hw/vfio/vfio-common.h |  3 ++
>>  2 files changed, 46 insertions(+), 30 deletions(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index a5a620a..ca0293d 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -2493,6 +2493,7 @@ static int vfio_initfn(PCIDevice *pdev)
>>      VFIODevice *vbasedev_iter;
>>      VFIOGroup *group;
>>      char *tmp, group_path[PATH_MAX], *group_name;
>> +    Error *err = NULL;
>>      ssize_t len;
>>      struct stat st;
>>      int groupid;
>> @@ -2506,9 +2507,9 @@ static int vfio_initfn(PCIDevice *pdev)
>>      }
>>  
>>      if (stat(vdev->vbasedev.sysfsdev, &st) < 0) {
>> -        error_report("vfio: error: no such host device: %s",
>> -                     vdev->vbasedev.sysfsdev);
>> -        return -errno;
>> +        error_setg(&err, "no such host device");
> 
> Any particular reason you don't use error_setg_errno() here?
I hesitated and eventually removed into thinking it was not bringing
much value. But not strong opinion here, I will restore it.
> 
>> +        ret = -errno;
>> +        goto error;
>>      }
>>  
>>      vdev->vbasedev.name = g_strdup(basename(vdev->vbasedev.sysfsdev));
>> @@ -2520,40 +2521,44 @@ static int vfio_initfn(PCIDevice *pdev)
>>      g_free(tmp);
>>  
>>      if (len <= 0 || len >= sizeof(group_path)) {
>> -        error_report("vfio: error no iommu_group for device");
>> -        return len < 0 ? -errno : -ENAMETOOLONG;
>> +        error_setg_errno(&err, len < 0 ? errno : ENAMETOOLONG,
>> +                         "no iommu_group found");
>> +        ret = len < 0 ? -errno : -ENAMETOOLONG;
> 
> Suggest
> 
>            ret = len < 0 ? -errno : -ENAMETOOLONG;
>            error_setg_errno(&err, -ret, "no iommu_group found");
Sure. Eventually ret will disappear so we will come back to
error_setg_errno(&err, len < 0 ? errno : ENAMETOOLONG,
>> +                         "no iommu_group found");
> 
>> +        goto error;
>>      }
>>  
>>      group_path[len] = 0;
>>  
>>      group_name = basename(group_path);
>>      if (sscanf(group_name, "%d", &groupid) != 1) {
>> -        error_report("vfio: error reading %s: %m", group_path);
>> -        return -errno;
>> +        error_setg_errno(&err, errno, "failed to read %s", group_path);
>> +        ret = -errno;
>> +        goto error;
>>      }
>>  
>>      trace_vfio_initfn(vdev->vbasedev.name, groupid);
>>  
>>      group = vfio_get_group(groupid, pci_device_iommu_address_space(pdev));
>>      if (!group) {
>> -        error_report("vfio: failed to get group %d", groupid);
>> -        return -ENOENT;
>> +        error_setg(&err, "failed to get group %d", groupid);
>> +        ret = -ENOENT;
>> +        goto error;
>>      }
>>  
>>      QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
>>          if (strcmp(vbasedev_iter->name, vdev->vbasedev.name) == 0) {
>> -            error_report("vfio: error: device %s is already attached",
>> -                         vdev->vbasedev.name);
>> +            error_setg(&err, "device is already attached");
>>              vfio_put_group(group);
>> -            return -EBUSY;
>> +            ret = -EBUSY;
>> +            goto error;
>>          }
>>      }
>>  
>>      ret = vfio_get_device(group, vdev->vbasedev.name, &vdev->vbasedev);
>>      if (ret) {
>> -        error_report("vfio: failed to get device %s", vdev->vbasedev.name);
>> +        error_setg_errno(&err, -ret, "failed to get device");
>>          vfio_put_group(group);
>> -        return ret;
>> +        goto error;
>>      }
>>  
>>      ret = vfio_populate_device(vdev);
>> @@ -2567,8 +2572,8 @@ static int vfio_initfn(PCIDevice *pdev)
>>                  vdev->config_offset);
>>      if (ret < (int)MIN(pci_config_size(&vdev->pdev), vdev->config_size)) {
>>          ret = ret < 0 ? -errno : -EFAULT;
>> -        error_report("vfio: Failed to read device config space");
>> -        return ret;
>> +        error_setg_errno(&err, -ret, "failed to read device config space");
>> +        goto error;
>>      }
>>  
>>      /* vfio emulates a lot for us, but some bits need extra love */
>> @@ -2584,8 +2589,9 @@ static int vfio_initfn(PCIDevice *pdev)
>>       */
>>      if (vdev->vendor_id != PCI_ANY_ID) {
>>          if (vdev->vendor_id >= 0xffff) {
>> -            error_report("vfio: Invalid PCI vendor ID provided");
>> -            return -EINVAL;
>> +            error_setg(&err, "invalid PCI vendor ID provided");
>> +            ret = -EINVAL;
>> +            goto error;
>>          }
>>          vfio_add_emulated_word(vdev, PCI_VENDOR_ID, vdev->vendor_id, ~0);
>>          trace_vfio_pci_emulated_vendor_id(vdev->vbasedev.name, vdev->vendor_id);
>> @@ -2595,8 +2601,9 @@ static int vfio_initfn(PCIDevice *pdev)
>>  
>>      if (vdev->device_id != PCI_ANY_ID) {
>>          if (vdev->device_id > 0xffff) {
>> -            error_report("vfio: Invalid PCI device ID provided");
>> -            return -EINVAL;
>> +            error_setg(&err, "invalid PCI device ID provided");
>> +            ret = -EINVAL;
>> +            goto error;
>>          }
>>          vfio_add_emulated_word(vdev, PCI_DEVICE_ID, vdev->device_id, ~0);
>>          trace_vfio_pci_emulated_device_id(vdev->vbasedev.name, vdev->device_id);
>> @@ -2606,8 +2613,9 @@ static int vfio_initfn(PCIDevice *pdev)
>>  
>>      if (vdev->sub_vendor_id != PCI_ANY_ID) {
>>          if (vdev->sub_vendor_id > 0xffff) {
>> -            error_report("vfio: Invalid PCI subsystem vendor ID provided");
>> -            return -EINVAL;
>> +            error_setg(&err, "invalid PCI subsystem vendor ID provided");
>> +            ret = -EINVAL;
>> +            goto error;
>>          }
>>          vfio_add_emulated_word(vdev, PCI_SUBSYSTEM_VENDOR_ID,
>>                                 vdev->sub_vendor_id, ~0);
>> @@ -2617,8 +2625,9 @@ static int vfio_initfn(PCIDevice *pdev)
>>  
>>      if (vdev->sub_device_id != PCI_ANY_ID) {
>>          if (vdev->sub_device_id > 0xffff) {
>> -            error_report("vfio: Invalid PCI subsystem device ID provided");
>> -            return -EINVAL;
>> +            error_setg(&err, "invalid PCI subsystem device ID provided");
>> +            ret = -EINVAL;
>> +            goto error;
>>          }
>>          vfio_add_emulated_word(vdev, PCI_SUBSYSTEM_ID, vdev->sub_device_id, ~0);
>>          trace_vfio_pci_emulated_sub_device_id(vdev->vbasedev.name,
>> @@ -2671,8 +2680,9 @@ static int vfio_initfn(PCIDevice *pdev)
>>          struct vfio_region_info *opregion;
>>  
>>          if (vdev->pdev.qdev.hotplugged) {
>> -            error_report("Cannot support IGD OpRegion feature on hotplugged "
>> -                         "device %s", vdev->vbasedev.name);
>> +            error_setg(&err,
>> +                       "cannot support IGD OpRegion feature on hotplugged "
>> +                       "device");
>>              ret = -EINVAL;
>>              goto out_teardown;
>>          }
>> @@ -2681,16 +2691,15 @@ static int vfio_initfn(PCIDevice *pdev)
>>                          VFIO_REGION_TYPE_PCI_VENDOR_TYPE | PCI_VENDOR_ID_INTEL,
>>                          VFIO_REGION_SUBTYPE_INTEL_IGD_OPREGION, &opregion);
>>          if (ret) {
>> -            error_report("Device %s does not support requested IGD OpRegion "
>> -                         "feature", vdev->vbasedev.name);
>> +            error_setg_errno(&err, -ret,
>> +                             "does not support requested IGD OpRegion feature");
>>              goto out_teardown;
>>          }
>>  
>>          ret = vfio_pci_igd_opregion_init(vdev, opregion);
>>          g_free(opregion);
>>          if (ret) {
>> -            error_report("Device %s IGD OpRegion initialization failed",
>> -                         vdev->vbasedev.name);
>> +            error_setg_errno(&err, -ret, "IGD OpRegion initialization failed");
>>              goto out_teardown;
>>          }
>>      }
>> @@ -2726,6 +2735,10 @@ out_teardown:
>>      pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
>>      vfio_teardown_msi(vdev);
>>      vfio_bars_exit(vdev);
>> +error:
>> +    if (err) {
>> +        error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name);
>> +    }
>>      return ret;
>>  }
> 
> The value assigned to ret doesn't matter as long as it's negative, which
> makes me wonder why we bother with different values here.  Peeking
> ahead, I see you simplify this already as part of your conversion to
> realize().  Okay, that works for me.
At that stage it was a 1-1 conversion. Indeed this eventually disappears.

> 
>>  
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>> index 94dfae3..fd19880 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -30,6 +30,9 @@
>>  #include <linux/vfio.h>
>>  #endif
>>  
>> +#define ERR_PREFIX "vfio error: %s: "
>> +#define WARN_PREFIX "vfio warning: %s: "
>> +
>>  /*#define DEBUG_VFIO*/
>>  #ifdef DEBUG_VFIO
>>  #define DPRINTF(fmt, ...) \
> 
> We already discussed error message prefixes and errno strings.  I got my
> taste, you got yours.  Since this is your code, yours wins :)
> 

Here is a sample output. Using qmp interactively you easily get to know
which device BDF produced the error. Also qemu prints the -device option
which lead to the error.

(QEMU) device_add driver=vfio-pci host=0000:01:10.0 bus=head.2 addr=0x14
{"error": {"class": "GenericError", "desc": "vfio error: 0000:01:10.0:
error opening /dev/vfio/6: No such file or directory"}}

qemu-system-aarch64: -device vfio-pci,host=0000:01:10.0: vfio error:
0000:01:10.0: error opening /dev/vfio/6: No such file or directory

However in case of more complex usage (qmp scripts?), might be helpful
to get the BDF directly in the error output?

Thanks

Eric

[Qemu-devel] [PATCH v2 02/12] vfio/pci: Pass an error object to vfio_populate_device

[Eric Auger]

Pass an error object to prepare for migration to VFIO-PCI realize.
The returned value will be removed later on.

The case where error recovery cannot be enabled is not converted into
an error object but directly reported through error_report, as before.
Populating an error instead would cause the future realize function to
fail, which is not wanted.

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

---

v1 -> v2:
- this patch now comes before the actual realize migration
- do not handle the error recovery issue as an error
---
 hw/vfio/pci.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index ca0293d..9fe970a 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2199,7 +2199,7 @@ int vfio_populate_vga(VFIOPCIDevice *vdev)
     return 0;
 }
 
-static int vfio_populate_device(VFIOPCIDevice *vdev)
+static int vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
 {
     VFIODevice *vbasedev = &vdev->vbasedev;
     struct vfio_region_info *reg_info;
@@ -2208,18 +2208,18 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
 
     /* Sanity check device */
     if (!(vbasedev->flags & VFIO_DEVICE_FLAGS_PCI)) {
-        error_report("vfio: Um, this isn't a PCI device");
+        error_setg(errp, "this isn't a PCI device");
         goto error;
     }
 
     if (vbasedev->num_regions < VFIO_PCI_CONFIG_REGION_INDEX + 1) {
-        error_report("vfio: unexpected number of io regions %u",
-                     vbasedev->num_regions);
+        error_setg(errp, "unexpected number of io regions %u",
+                   vbasedev->num_regions);
         goto error;
     }
 
     if (vbasedev->num_irqs < VFIO_PCI_MSIX_IRQ_INDEX + 1) {
-        error_report("vfio: unexpected number of irqs %u", vbasedev->num_irqs);
+        error_setg(errp, "unexpected number of irqs %u", vbasedev->num_irqs);
         goto error;
     }
 
@@ -2231,7 +2231,7 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
         g_free(name);
 
         if (ret) {
-            error_report("vfio: Error getting region %d info: %m", i);
+            error_setg_errno(errp, -ret, "failed to get region %d info", i);
             goto error;
         }
 
@@ -2241,7 +2241,7 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
     ret = vfio_get_region_info(vbasedev,
                                VFIO_PCI_CONFIG_REGION_INDEX, &reg_info);
     if (ret) {
-        error_report("vfio: Error getting config info: %m");
+        error_setg_errno(errp, -ret, "failed to get config info");
         goto error;
     }
 
@@ -2261,8 +2261,8 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
     if (vdev->features & VFIO_FEATURE_ENABLE_VGA) {
         ret = vfio_populate_vga(vdev);
         if (ret) {
-            error_report(
-                "vfio: Device does not support requested feature x-vga");
+            error_setg_errno(errp, -ret,
+                "device does not support requested feature x-vga");
             goto error;
         }
     }
@@ -2277,7 +2277,7 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
     } else if (irq_info.count == 1) {
         vdev->pci_aer = true;
     } else {
-        error_report("vfio: %s "
+        error_report(WARN_PREFIX
                      "Could not enable error recovery for the device",
                      vbasedev->name);
     }
@@ -2561,9 +2561,9 @@ static int vfio_initfn(PCIDevice *pdev)
         goto error;
     }
 
-    ret = vfio_populate_device(vdev);
-    if (ret) {
-        return ret;
+    ret = vfio_populate_device(vdev, &err);
+    if (err) {
+        goto error;
     }
 
     /* Get a copy of config space */
-- 
1.9.1

Re: [Qemu-devel] [PATCH v2 02/12] vfio/pci: Pass an error object to vfio_populate_device

[Markus Armbruster]

Eric Auger <eric.auger@redhat.com> writes:

> Pass an error object to prepare for migration to VFIO-PCI realize.
> The returned value will be removed later on.
>
> The case where error recovery cannot be enabled is not converted into
> an error object but directly reported through error_report, as before.
> Populating an error instead would cause the future realize function to
> fail, which is not wanted.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>
> ---
>
> v1 -> v2:
> - this patch now comes before the actual realize migration
> - do not handle the error recovery issue as an error
> ---
>  hw/vfio/pci.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index ca0293d..9fe970a 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2199,7 +2199,7 @@ int vfio_populate_vga(VFIOPCIDevice *vdev)
>      return 0;
>  }
>  
> -static int vfio_populate_device(VFIOPCIDevice *vdev)
> +static int vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
>  {
>      VFIODevice *vbasedev = &vdev->vbasedev;
>      struct vfio_region_info *reg_info;
> @@ -2208,18 +2208,18 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
>  
>      /* Sanity check device */
>      if (!(vbasedev->flags & VFIO_DEVICE_FLAGS_PCI)) {
> -        error_report("vfio: Um, this isn't a PCI device");
> +        error_setg(errp, "this isn't a PCI device");
>          goto error;
>      }
>  
>      if (vbasedev->num_regions < VFIO_PCI_CONFIG_REGION_INDEX + 1) {
> -        error_report("vfio: unexpected number of io regions %u",
> -                     vbasedev->num_regions);
> +        error_setg(errp, "unexpected number of io regions %u",
> +                   vbasedev->num_regions);
>          goto error;
>      }
>  
>      if (vbasedev->num_irqs < VFIO_PCI_MSIX_IRQ_INDEX + 1) {
> -        error_report("vfio: unexpected number of irqs %u", vbasedev->num_irqs);
> +        error_setg(errp, "unexpected number of irqs %u", vbasedev->num_irqs);
>          goto error;
>      }
>  
> @@ -2231,7 +2231,7 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
>          g_free(name);
>  
>          if (ret) {
> -            error_report("vfio: Error getting region %d info: %m", i);
> +            error_setg_errno(errp, -ret, "failed to get region %d info", i);
>              goto error;
>          }
>  
> @@ -2241,7 +2241,7 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
>      ret = vfio_get_region_info(vbasedev,
>                                 VFIO_PCI_CONFIG_REGION_INDEX, &reg_info);
>      if (ret) {
> -        error_report("vfio: Error getting config info: %m");
> +        error_setg_errno(errp, -ret, "failed to get config info");
>          goto error;
>      }
>  
> @@ -2261,8 +2261,8 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
>      if (vdev->features & VFIO_FEATURE_ENABLE_VGA) {
>          ret = vfio_populate_vga(vdev);
>          if (ret) {
> -            error_report(
> -                "vfio: Device does not support requested feature x-vga");
> +            error_setg_errno(errp, -ret,
> +                "device does not support requested feature x-vga");
>              goto error;
>          }
>      }

What about the error_report() in vfio_populate_vga()?  Should the
vfio_populate_vga() take an Error * argument?

> @@ -2277,7 +2277,7 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
>      } else if (irq_info.count == 1) {
>          vdev->pci_aer = true;
>      } else {
> -        error_report("vfio: %s "
> +        error_report(WARN_PREFIX
>                       "Could not enable error recovery for the device",
>                       vbasedev->name);
>      }
> @@ -2561,9 +2561,9 @@ static int vfio_initfn(PCIDevice *pdev)
>          goto error;
>      }
>  
> -    ret = vfio_populate_device(vdev);
> -    if (ret) {
> -        return ret;
> +    ret = vfio_populate_device(vdev, &err);
> +    if (err) {
> +        goto error;
>      }
>  
>      /* Get a copy of config space */

Re: [Qemu-devel] [PATCH v2 02/12] vfio/pci: Pass an error object to vfio_populate_device

[Auger Eric]

Markus,

On 22/09/2016 17:49, Markus Armbruster wrote:
> Eric Auger <eric.auger@redhat.com> writes:
> 
>> Pass an error object to prepare for migration to VFIO-PCI realize.
>> The returned value will be removed later on.
>>
>> The case where error recovery cannot be enabled is not converted into
>> an error object but directly reported through error_report, as before.
>> Populating an error instead would cause the future realize function to
>> fail, which is not wanted.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
>>
>> v1 -> v2:
>> - this patch now comes before the actual realize migration
>> - do not handle the error recovery issue as an error
>> ---
>>  hw/vfio/pci.c | 26 +++++++++++++-------------
>>  1 file changed, 13 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index ca0293d..9fe970a 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -2199,7 +2199,7 @@ int vfio_populate_vga(VFIOPCIDevice *vdev)
>>      return 0;
>>  }
>>  
>> -static int vfio_populate_device(VFIOPCIDevice *vdev)
>> +static int vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
>>  {
>>      VFIODevice *vbasedev = &vdev->vbasedev;
>>      struct vfio_region_info *reg_info;
>> @@ -2208,18 +2208,18 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
>>  
>>      /* Sanity check device */
>>      if (!(vbasedev->flags & VFIO_DEVICE_FLAGS_PCI)) {
>> -        error_report("vfio: Um, this isn't a PCI device");
>> +        error_setg(errp, "this isn't a PCI device");
>>          goto error;
>>      }
>>  
>>      if (vbasedev->num_regions < VFIO_PCI_CONFIG_REGION_INDEX + 1) {
>> -        error_report("vfio: unexpected number of io regions %u",
>> -                     vbasedev->num_regions);
>> +        error_setg(errp, "unexpected number of io regions %u",
>> +                   vbasedev->num_regions);
>>          goto error;
>>      }
>>  
>>      if (vbasedev->num_irqs < VFIO_PCI_MSIX_IRQ_INDEX + 1) {
>> -        error_report("vfio: unexpected number of irqs %u", vbasedev->num_irqs);
>> +        error_setg(errp, "unexpected number of irqs %u", vbasedev->num_irqs);
>>          goto error;
>>      }
>>  
>> @@ -2231,7 +2231,7 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
>>          g_free(name);
>>  
>>          if (ret) {
>> -            error_report("vfio: Error getting region %d info: %m", i);
>> +            error_setg_errno(errp, -ret, "failed to get region %d info", i);
>>              goto error;
>>          }
>>  
>> @@ -2241,7 +2241,7 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
>>      ret = vfio_get_region_info(vbasedev,
>>                                 VFIO_PCI_CONFIG_REGION_INDEX, &reg_info);
>>      if (ret) {
>> -        error_report("vfio: Error getting config info: %m");
>> +        error_setg_errno(errp, -ret, "failed to get config info");
>>          goto error;
>>      }
>>  
>> @@ -2261,8 +2261,8 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
>>      if (vdev->features & VFIO_FEATURE_ENABLE_VGA) {
>>          ret = vfio_populate_vga(vdev);
>>          if (ret) {
>> -            error_report(
>> -                "vfio: Device does not support requested feature x-vga");
>> +            error_setg_errno(errp, -ret,
>> +                "device does not support requested feature x-vga");
>>              goto error;
>>          }
>>      }
> 
> What about the error_report() in vfio_populate_vga()?  Should the
> vfio_populate_vga() take an Error * argument?
Sure

Thanks

Eric
> 
>> @@ -2277,7 +2277,7 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
>>      } else if (irq_info.count == 1) {
>>          vdev->pci_aer = true;
>>      } else {
>> -        error_report("vfio: %s "
>> +        error_report(WARN_PREFIX
>>                       "Could not enable error recovery for the device",
>>                       vbasedev->name);
>>      }
>> @@ -2561,9 +2561,9 @@ static int vfio_initfn(PCIDevice *pdev)
>>          goto error;
>>      }
>>  
>> -    ret = vfio_populate_device(vdev);
>> -    if (ret) {
>> -        return ret;
>> +    ret = vfio_populate_device(vdev, &err);
>> +    if (err) {
>> +        goto error;
>>      }
>>  
>>      /* Get a copy of config space */
> 

[Qemu-devel] [PATCH v2 03/12] vfio/pci: Pass an error object to vfio_msix_early_setup

[Eric Auger]

Pass an error object to prepare for migration to VFIO-PCI realize.
The returned value will be removed later on.

We now format an error in case of reading failure for
- the MSIX flags
- the MSIX table,
- the MSIX PBA.

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

---

v1 -> v2:
- this patch now comes before the actual realize migration
- mention in the commit message we are no more silent on above errors
---
 hw/vfio/pci.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 9fe970a..b35925a 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1277,7 +1277,7 @@ static void vfio_pci_fixup_msix_region(VFIOPCIDevice *vdev)
  * need to first look for where the MSI-X table lives.  So we
  * unfortunately split MSI-X setup across two functions.
  */
-static int vfio_msix_early_setup(VFIOPCIDevice *vdev)
+static int vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
 {
     uint8_t pos;
     uint16_t ctrl;
@@ -1292,16 +1292,19 @@ static int vfio_msix_early_setup(VFIOPCIDevice *vdev)
 
     if (pread(fd, &ctrl, sizeof(ctrl),
               vdev->config_offset + pos + PCI_MSIX_FLAGS) != sizeof(ctrl)) {
+        error_setg_errno(errp, errno, "failed to read PCI MSIX FLAGS");
         return -errno;
     }
 
     if (pread(fd, &table, sizeof(table),
               vdev->config_offset + pos + PCI_MSIX_TABLE) != sizeof(table)) {
+        error_setg_errno(errp, errno, "failed to read PCI MSIX TABLE");
         return -errno;
     }
 
     if (pread(fd, &pba, sizeof(pba),
               vdev->config_offset + pos + PCI_MSIX_PBA) != sizeof(pba)) {
+        error_setg_errno(errp, errno, "failed to read PCI MSIX PBA");
         return -errno;
     }
 
@@ -1332,8 +1335,8 @@ static int vfio_msix_early_setup(VFIOPCIDevice *vdev)
             (vdev->device_id & 0xff00) == 0x5800) {
             msix->pba_offset = 0x1000;
         } else {
-            error_report("vfio: Hardware reports invalid configuration, "
-                         "MSIX PBA outside of specified BAR");
+            error_setg(errp, "hardware reports invalid configuration, "
+                       "MSIX PBA outside of specified BAR");
             g_free(msix);
             return -EINVAL;
         }
@@ -2655,9 +2658,9 @@ static int vfio_initfn(PCIDevice *pdev)
 
     vfio_pci_size_rom(vdev);
 
-    ret = vfio_msix_early_setup(vdev);
-    if (ret) {
-        return ret;
+    ret = vfio_msix_early_setup(vdev, &err);
+    if (err) {
+        goto error;
     }
 
     vfio_bars_setup(vdev);
-- 
1.9.1

[Qemu-devel] [PATCH v2 04/12] vfio/pci: Pass an error object to vfio_intx_enable

[Eric Auger]

Pass an error object to prepare for migration to VFIO-PCI realize.

The error object is propagated downto vfio_intx_enable_kvm

vfio_intx_update which calls vfio_intx_enable_kvm and
vfio_msi_disable_common/vfio_pci_post_reset which calls vfio_intx_enable
do not propagate the error and simply call error_reportf_err with the
ERR_PREFIX formatting.

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

---

v2: creation
---
 hw/vfio/pci.c | 39 +++++++++++++++++++++++++++------------
 1 file changed, 27 insertions(+), 12 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index b35925a..f67eec4 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -100,7 +100,7 @@ static void vfio_intx_eoi(VFIODevice *vbasedev)
     vfio_unmask_single_irqindex(vbasedev, VFIO_PCI_INTX_IRQ_INDEX);
 }
 
-static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev)
+static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp)
 {
 #ifdef CONFIG_KVM
     struct kvm_irqfd irqfd = {
@@ -126,7 +126,7 @@ static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev)
 
     /* Get an eventfd for resample/unmask */
     if (event_notifier_init(&vdev->intx.unmask, 0)) {
-        error_report("vfio: Error: event_notifier_init failed eoi");
+        error_setg(errp, "event_notifier_init failed eoi");
         goto fail;
     }
 
@@ -134,7 +134,7 @@ static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev)
     irqfd.resamplefd = event_notifier_get_fd(&vdev->intx.unmask);
 
     if (kvm_vm_ioctl(kvm_state, KVM_IRQFD, &irqfd)) {
-        error_report("vfio: Error: Failed to setup resample irqfd: %m");
+        error_setg_errno(errp, errno, "failed to setup resample irqfd");
         goto fail_irqfd;
     }
 
@@ -153,7 +153,7 @@ static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev)
     ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
     g_free(irq_set);
     if (ret) {
-        error_report("vfio: Error: Failed to setup INTx unmask fd: %m");
+        error_setg_errno(errp, -ret, "failed to setup INTx unmask fd");
         goto fail_vfio;
     }
 
@@ -222,6 +222,7 @@ static void vfio_intx_update(PCIDevice *pdev)
 {
     VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
     PCIINTxRoute route;
+    Error *err = NULL;
 
     if (vdev->interrupt != VFIO_INT_INTx) {
         return;
@@ -244,18 +245,22 @@ static void vfio_intx_update(PCIDevice *pdev)
         return;
     }
 
-    vfio_intx_enable_kvm(vdev);
+    vfio_intx_enable_kvm(vdev, &err);
+    if (err) {
+        error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name);
+    }
 
     /* Re-enable the interrupt in cased we missed an EOI */
     vfio_intx_eoi(&vdev->vbasedev);
 }
 
-static int vfio_intx_enable(VFIOPCIDevice *vdev)
+static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
 {
     uint8_t pin = vfio_pci_read_config(&vdev->pdev, PCI_INTERRUPT_PIN, 1);
     int ret, argsz;
     struct vfio_irq_set *irq_set;
     int32_t *pfd;
+    Error *err = NULL;
 
     if (!pin) {
         return 0;
@@ -279,7 +284,7 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev)
 
     ret = event_notifier_init(&vdev->intx.interrupt, 0);
     if (ret) {
-        error_report("vfio: Error: event_notifier_init failed");
+        error_setg_errno(errp, -ret, "event_notifier_init failed");
         return ret;
     }
 
@@ -299,13 +304,14 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev)
     ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
     g_free(irq_set);
     if (ret) {
-        error_report("vfio: Error: Failed to setup INTx fd: %m");
+        error_setg_errno(errp, -ret, "failed to setup INTx fd");
         qemu_set_fd_handler(*pfd, NULL, NULL, vdev);
         event_notifier_cleanup(&vdev->intx.interrupt);
         return -errno;
     }
 
-    vfio_intx_enable_kvm(vdev);
+    vfio_intx_enable_kvm(vdev, &err);
+    error_propagate(errp, err);
 
     vdev->interrupt = VFIO_INT_INTx;
 
@@ -707,6 +713,7 @@ retry:
 
 static void vfio_msi_disable_common(VFIOPCIDevice *vdev)
 {
+    Error *err = NULL;
     int i;
 
     for (i = 0; i < vdev->nr_vectors; i++) {
@@ -726,7 +733,10 @@ static void vfio_msi_disable_common(VFIOPCIDevice *vdev)
     vdev->nr_vectors = 0;
     vdev->interrupt = VFIO_INT_NONE;
 
-    vfio_intx_enable(vdev);
+    vfio_intx_enable(vdev, &err);
+    if (err) {
+        error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name);
+    }
 }
 
 static void vfio_msix_disable(VFIOPCIDevice *vdev)
@@ -1908,7 +1918,12 @@ static void vfio_pci_pre_reset(VFIOPCIDevice *vdev)
 
 static void vfio_pci_post_reset(VFIOPCIDevice *vdev)
 {
-    vfio_intx_enable(vdev);
+    Error *err = NULL;
+
+    vfio_intx_enable(vdev, &err);
+    if (err) {
+        error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name);
+    }
 }
 
 static bool vfio_pci_host_match(PCIHostDeviceAddress *addr, const char *name)
@@ -2722,7 +2737,7 @@ static int vfio_initfn(PCIDevice *pdev)
         vdev->intx.mmap_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,
                                                   vfio_intx_mmap_enable, vdev);
         pci_device_set_intx_routing_notifier(&vdev->pdev, vfio_intx_update);
-        ret = vfio_intx_enable(vdev);
+        ret = vfio_intx_enable(vdev, &err);
         if (ret) {
             goto out_teardown;
         }
-- 
1.9.1

Re: [Qemu-devel] [PATCH v2 04/12] vfio/pci: Pass an error object to vfio_intx_enable

[Markus Armbruster]

Eric Auger <eric.auger@redhat.com> writes:

> Pass an error object to prepare for migration to VFIO-PCI realize.
>
> The error object is propagated downto vfio_intx_enable_kvm

down to vfio_intx_enable_kvm().

(feel free to omit the () I automatically add after a function name)

> vfio_intx_update which calls vfio_intx_enable_kvm and
> vfio_msi_disable_common/vfio_pci_post_reset which calls vfio_intx_enable

Suggest: The three other callers vfio_intx_update,
vfio_msi_disable_common() and vfio_pci_post_reset()

> do not propagate the error and simply call error_reportf_err with the
> ERR_PREFIX formatting.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>
> ---
>
> v2: creation
> ---
>  hw/vfio/pci.c | 39 +++++++++++++++++++++++++++------------
>  1 file changed, 27 insertions(+), 12 deletions(-)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index b35925a..f67eec4 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -100,7 +100,7 @@ static void vfio_intx_eoi(VFIODevice *vbasedev)
>      vfio_unmask_single_irqindex(vbasedev, VFIO_PCI_INTX_IRQ_INDEX);
>  }
>  
> -static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev)
> +static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp)
>  {
>  #ifdef CONFIG_KVM
>      struct kvm_irqfd irqfd = {
> @@ -126,7 +126,7 @@ static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev)
>  
>      /* Get an eventfd for resample/unmask */
>      if (event_notifier_init(&vdev->intx.unmask, 0)) {
> -        error_report("vfio: Error: event_notifier_init failed eoi");
> +        error_setg(errp, "event_notifier_init failed eoi");
>          goto fail;
>      }
>  
> @@ -134,7 +134,7 @@ static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev)
>      irqfd.resamplefd = event_notifier_get_fd(&vdev->intx.unmask);
>  
>      if (kvm_vm_ioctl(kvm_state, KVM_IRQFD, &irqfd)) {
> -        error_report("vfio: Error: Failed to setup resample irqfd: %m");
> +        error_setg_errno(errp, errno, "failed to setup resample irqfd");
>          goto fail_irqfd;
>      }
>  
> @@ -153,7 +153,7 @@ static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev)
>      ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
>      g_free(irq_set);
>      if (ret) {
> -        error_report("vfio: Error: Failed to setup INTx unmask fd: %m");
> +        error_setg_errno(errp, -ret, "failed to setup INTx unmask fd");
>          goto fail_vfio;
>      }
>  
> @@ -222,6 +222,7 @@ static void vfio_intx_update(PCIDevice *pdev)
>  {
>      VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
>      PCIINTxRoute route;
> +    Error *err = NULL;
>  
>      if (vdev->interrupt != VFIO_INT_INTx) {
>          return;
> @@ -244,18 +245,22 @@ static void vfio_intx_update(PCIDevice *pdev)
>          return;
>      }
>  
> -    vfio_intx_enable_kvm(vdev);
> +    vfio_intx_enable_kvm(vdev, &err);
> +    if (err) {
> +        error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name);
> +    }
>  
>      /* Re-enable the interrupt in cased we missed an EOI */
>      vfio_intx_eoi(&vdev->vbasedev);
>  }

Nate that this function now permits callers to detect failure.

>  
> -static int vfio_intx_enable(VFIOPCIDevice *vdev)
> +static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
>  {
>      uint8_t pin = vfio_pci_read_config(&vdev->pdev, PCI_INTERRUPT_PIN, 1);
>      int ret, argsz;
>      struct vfio_irq_set *irq_set;
>      int32_t *pfd;
> +    Error *err = NULL;
>  
>      if (!pin) {
>          return 0;
> @@ -279,7 +284,7 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev)
>  
>      ret = event_notifier_init(&vdev->intx.interrupt, 0);
>      if (ret) {
> -        error_report("vfio: Error: event_notifier_init failed");
> +        error_setg_errno(errp, -ret, "event_notifier_init failed");
>          return ret;
>      }
>  
> @@ -299,13 +304,14 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev)
>      ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
>      g_free(irq_set);
>      if (ret) {
> -        error_report("vfio: Error: Failed to setup INTx fd: %m");
> +        error_setg_errno(errp, -ret, "failed to setup INTx fd");
>          qemu_set_fd_handler(*pfd, NULL, NULL, vdev);
>          event_notifier_cleanup(&vdev->intx.interrupt);
>          return -errno;
>      }
>  
> -    vfio_intx_enable_kvm(vdev);
> +    vfio_intx_enable_kvm(vdev, &err);
> +    error_propagate(errp, err);

This wasn't an error before.  Bug fix or regression?

Since you don't examine err, you should simply

       vfio_intx_enable_kvm(errp)

>      vdev->interrupt = VFIO_INT_INTx;
>  
> @@ -707,6 +713,7 @@ retry:
>  
>  static void vfio_msi_disable_common(VFIOPCIDevice *vdev)
>  {
> +    Error *err = NULL;
>      int i;
>  
>      for (i = 0; i < vdev->nr_vectors; i++) {
> @@ -726,7 +733,10 @@ static void vfio_msi_disable_common(VFIOPCIDevice *vdev)
>      vdev->nr_vectors = 0;
>      vdev->interrupt = VFIO_INT_NONE;
>  
> -    vfio_intx_enable(vdev);
> +    vfio_intx_enable(vdev, &err);
> +    if (err) {
> +        error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name);
> +    }
>  }
>  
>  static void vfio_msix_disable(VFIOPCIDevice *vdev)
> @@ -1908,7 +1918,12 @@ static void vfio_pci_pre_reset(VFIOPCIDevice *vdev)
>  
>  static void vfio_pci_post_reset(VFIOPCIDevice *vdev)
>  {
> -    vfio_intx_enable(vdev);
> +    Error *err = NULL;
> +
> +    vfio_intx_enable(vdev, &err);
> +    if (err) {
> +        error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name);
> +    }
>  }
>  
>  static bool vfio_pci_host_match(PCIHostDeviceAddress *addr, const char *name)
> @@ -2722,7 +2737,7 @@ static int vfio_initfn(PCIDevice *pdev)
>          vdev->intx.mmap_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,
>                                                    vfio_intx_mmap_enable, vdev);
>          pci_device_set_intx_routing_notifier(&vdev->pdev, vfio_intx_update);
> -        ret = vfio_intx_enable(vdev);
> +        ret = vfio_intx_enable(vdev, &err);
>          if (ret) {
>              goto out_teardown;
>          }

Re: [Qemu-devel] [PATCH v2 04/12] vfio/pci: Pass an error object to vfio_intx_enable

[Auger Eric]

Markus,

On 22/09/2016 18:27, Markus Armbruster wrote:
> Eric Auger <eric.auger@redhat.com> writes:
> 
>> Pass an error object to prepare for migration to VFIO-PCI realize.
>>
>> The error object is propagated downto vfio_intx_enable_kvm
> 
> down to vfio_intx_enable_kvm().
> 
> (feel free to omit the () I automatically add after a function name)
> 
>> vfio_intx_update which calls vfio_intx_enable_kvm and
>> vfio_msi_disable_common/vfio_pci_post_reset which calls vfio_intx_enable
I took into account the rewording suggestions.
> 
> Suggest: The three other callers vfio_intx_update,
> vfio_msi_disable_common() and vfio_pci_post_reset()
> 
>> do not propagate the error and simply call error_reportf_err with the
>> ERR_PREFIX formatting.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
>>
>> v2: creation
>> ---
>>  hw/vfio/pci.c | 39 +++++++++++++++++++++++++++------------
>>  1 file changed, 27 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index b35925a..f67eec4 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -100,7 +100,7 @@ static void vfio_intx_eoi(VFIODevice *vbasedev)
>>      vfio_unmask_single_irqindex(vbasedev, VFIO_PCI_INTX_IRQ_INDEX);
>>  }
>>  
>> -static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev)
>> +static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp)
>>  {
>>  #ifdef CONFIG_KVM
>>      struct kvm_irqfd irqfd = {
>> @@ -126,7 +126,7 @@ static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev)
>>  
>>      /* Get an eventfd for resample/unmask */
>>      if (event_notifier_init(&vdev->intx.unmask, 0)) {
>> -        error_report("vfio: Error: event_notifier_init failed eoi");
>> +        error_setg(errp, "event_notifier_init failed eoi");
>>          goto fail;
>>      }
>>  
>> @@ -134,7 +134,7 @@ static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev)
>>      irqfd.resamplefd = event_notifier_get_fd(&vdev->intx.unmask);
>>  
>>      if (kvm_vm_ioctl(kvm_state, KVM_IRQFD, &irqfd)) {
>> -        error_report("vfio: Error: Failed to setup resample irqfd: %m");
>> +        error_setg_errno(errp, errno, "failed to setup resample irqfd");
>>          goto fail_irqfd;
>>      }
>>  
>> @@ -153,7 +153,7 @@ static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev)
>>      ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
>>      g_free(irq_set);
>>      if (ret) {
>> -        error_report("vfio: Error: Failed to setup INTx unmask fd: %m");
>> +        error_setg_errno(errp, -ret, "failed to setup INTx unmask fd");
>>          goto fail_vfio;
>>      }
>>  
>> @@ -222,6 +222,7 @@ static void vfio_intx_update(PCIDevice *pdev)
>>  {
>>      VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
>>      PCIINTxRoute route;
>> +    Error *err = NULL;
>>  
>>      if (vdev->interrupt != VFIO_INT_INTx) {
>>          return;
>> @@ -244,18 +245,22 @@ static void vfio_intx_update(PCIDevice *pdev)
>>          return;
>>      }
>>  
>> -    vfio_intx_enable_kvm(vdev);
>> +    vfio_intx_enable_kvm(vdev, &err);
>> +    if (err) {
>> +        error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name);
>> +    }
>>  
>>      /* Re-enable the interrupt in cased we missed an EOI */
>>      vfio_intx_eoi(&vdev->vbasedev);
>>  }
> 
> Nate that this function now permits callers to detect failure.
There was error_report before in vfio_intx_enable_kvm but indeed testing
vfio_intx_enable_kvm()'s *errp now makes possible to detect issues.
> 
>>  
>> -static int vfio_intx_enable(VFIOPCIDevice *vdev)
>> +static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
>>  {
>>      uint8_t pin = vfio_pci_read_config(&vdev->pdev, PCI_INTERRUPT_PIN, 1);
>>      int ret, argsz;
>>      struct vfio_irq_set *irq_set;
>>      int32_t *pfd;
>> +    Error *err = NULL;
>>  
>>      if (!pin) {
>>          return 0;
>> @@ -279,7 +284,7 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev)
>>  
>>      ret = event_notifier_init(&vdev->intx.interrupt, 0);
>>      if (ret) {
>> -        error_report("vfio: Error: event_notifier_init failed");
>> +        error_setg_errno(errp, -ret, "event_notifier_init failed");
>>          return ret;
>>      }
>>  
>> @@ -299,13 +304,14 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev)
>>      ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
>>      g_free(irq_set);
>>      if (ret) {
>> -        error_report("vfio: Error: Failed to setup INTx fd: %m");
>> +        error_setg_errno(errp, -ret, "failed to setup INTx fd");
>>          qemu_set_fd_handler(*pfd, NULL, NULL, vdev);
>>          event_notifier_cleanup(&vdev->intx.interrupt);
>>          return -errno;
>>      }
>>  
>> -    vfio_intx_enable_kvm(vdev);
>> +    vfio_intx_enable_kvm(vdev, &err);
>> +    error_propagate(errp, err);
> 
> This wasn't an error before.  Bug fix or regression?
Thanks for catching this. It is a regression since in case KVM setup
fails we revert to userspace handled eventfds.

I replaced that by a warning instead.

Thanks!

Eric
> 
> Since you don't examine err, you should simply
> 
>        vfio_intx_enable_kvm(errp)
> 
>>      vdev->interrupt = VFIO_INT_INTx;
>>  
>> @@ -707,6 +713,7 @@ retry:
>>  
>>  static void vfio_msi_disable_common(VFIOPCIDevice *vdev)
>>  {
>> +    Error *err = NULL;
>>      int i;
>>  
>>      for (i = 0; i < vdev->nr_vectors; i++) {
>> @@ -726,7 +733,10 @@ static void vfio_msi_disable_common(VFIOPCIDevice *vdev)
>>      vdev->nr_vectors = 0;
>>      vdev->interrupt = VFIO_INT_NONE;
>>  
>> -    vfio_intx_enable(vdev);
>> +    vfio_intx_enable(vdev, &err);
>> +    if (err) {
>> +        error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name);
>> +    }
>>  }
>>  
>>  static void vfio_msix_disable(VFIOPCIDevice *vdev)
>> @@ -1908,7 +1918,12 @@ static void vfio_pci_pre_reset(VFIOPCIDevice *vdev)
>>  
>>  static void vfio_pci_post_reset(VFIOPCIDevice *vdev)
>>  {
>> -    vfio_intx_enable(vdev);
>> +    Error *err = NULL;
>> +
>> +    vfio_intx_enable(vdev, &err);
>> +    if (err) {
>> +        error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name);
>> +    }
>>  }
>>  
>>  static bool vfio_pci_host_match(PCIHostDeviceAddress *addr, const char *name)
>> @@ -2722,7 +2737,7 @@ static int vfio_initfn(PCIDevice *pdev)
>>          vdev->intx.mmap_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,
>>                                                    vfio_intx_mmap_enable, vdev);
>>          pci_device_set_intx_routing_notifier(&vdev->pdev, vfio_intx_update);
>> -        ret = vfio_intx_enable(vdev);
>> +        ret = vfio_intx_enable(vdev, &err);
>>          if (ret) {
>>              goto out_teardown;
>>          }
> 

[Qemu-devel] [PATCH v2 05/12] vfio/pci: Pass an error object to vfio_add_capabilities

[Eric Auger]

Pass an error object to prepare for migration to VFIO-PCI realize.
The error is cascaded downto vfio_add_std_cap and then vfio_msi(x)_setup,
vfio_setup_pcie_cap.

vfio_add_ext_cap does not return anything else than 0 so let's transform
it into a void function.

Also use pci_add_capability2 which takes an error object.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 hw/vfio/pci.c | 66 ++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 36 insertions(+), 30 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index f67eec4..07a44f5 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1178,7 +1178,7 @@ static void vfio_disable_interrupts(VFIOPCIDevice *vdev)
     }
 }
 
-static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos)
+static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
 {
     uint16_t ctrl;
     bool msi_64bit, msi_maskbit;
@@ -1202,8 +1202,7 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos)
         if (ret == -ENOTSUP) {
             return 0;
         }
-        error_prepend(&err, "vfio: msi_init failed: ");
-        error_report_err(err);
+        error_propagate(errp, err);
         return ret;
     }
     vdev->msi_cap_size = 0xa + (msi_maskbit ? 0xa : 0) + (msi_64bit ? 0x4 : 0);
@@ -1361,7 +1360,7 @@ static int vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
     return 0;
 }
 
-static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos)
+static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
 {
     int ret;
 
@@ -1376,7 +1375,7 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos)
         if (ret == -ENOTSUP) {
             return 0;
         }
-        error_report("vfio: msix_init failed");
+        error_setg(errp, "msix_init failed");
         return ret;
     }
 
@@ -1561,7 +1560,8 @@ static void vfio_add_emulated_long(VFIOPCIDevice *vdev, int pos,
     vfio_set_long_bits(vdev->emulated_config_bits + pos, mask, mask);
 }
 
-static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size)
+static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size,
+                               Error **errp)
 {
     uint16_t flags;
     uint8_t type;
@@ -1573,8 +1573,8 @@ static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size)
         type != PCI_EXP_TYPE_LEG_END &&
         type != PCI_EXP_TYPE_RC_END) {
 
-        error_report("vfio: Assignment of PCIe type 0x%x "
-                     "devices is not currently supported", type);
+        error_setg(errp, "assignment of PCIe type 0x%x "
+                   "devices is not currently supported", type);
         return -EINVAL;
     }
 
@@ -1708,10 +1708,11 @@ static void vfio_check_af_flr(VFIOPCIDevice *vdev, uint8_t pos)
     }
 }
 
-static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
+static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos, Error **errp)
 {
     PCIDevice *pdev = &vdev->pdev;
     uint8_t cap_id, next, size;
+    Error *err = NULL;
     int ret;
 
     cap_id = pdev->config[pos];
@@ -1733,9 +1734,9 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
      * will be changed as we unwind the stack.
      */
     if (next) {
-        ret = vfio_add_std_cap(vdev, next);
+        ret = vfio_add_std_cap(vdev, next, &err);
         if (ret) {
-            return ret;
+            goto out;
         }
     } else {
         /* Begin the rebuild, use QEMU emulated list bits */
@@ -1749,40 +1750,42 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
 
     switch (cap_id) {
     case PCI_CAP_ID_MSI:
-        ret = vfio_msi_setup(vdev, pos);
+        ret = vfio_msi_setup(vdev, pos, &err);
         break;
     case PCI_CAP_ID_EXP:
         vfio_check_pcie_flr(vdev, pos);
-        ret = vfio_setup_pcie_cap(vdev, pos, size);
+        ret = vfio_setup_pcie_cap(vdev, pos, size, &err);
         break;
     case PCI_CAP_ID_MSIX:
-        ret = vfio_msix_setup(vdev, pos);
+        ret = vfio_msix_setup(vdev, pos, &err);
         break;
     case PCI_CAP_ID_PM:
         vfio_check_pm_reset(vdev, pos);
         vdev->pm_cap = pos;
-        ret = pci_add_capability(pdev, cap_id, pos, size);
+        ret = pci_add_capability2(pdev, cap_id, pos, size, &err);
         break;
     case PCI_CAP_ID_AF:
         vfio_check_af_flr(vdev, pos);
-        ret = pci_add_capability(pdev, cap_id, pos, size);
+        ret = pci_add_capability2(pdev, cap_id, pos, size, &err);
         break;
     default:
-        ret = pci_add_capability(pdev, cap_id, pos, size);
+        ret = pci_add_capability2(pdev, cap_id, pos, size, &err);
         break;
     }
 
-    if (ret < 0) {
-        error_report("vfio: %s Error adding PCI capability "
-                     "0x%x[0x%x]@0x%x: %d", vdev->vbasedev.name,
-                     cap_id, size, pos, ret);
-        return ret;
+out:
+    error_propagate(errp, err);
+    if (*errp) {
+        error_prepend(errp,
+                      "failed to add PCI capability 0x%x[0x%x]@0x%x: ",
+                      cap_id, size, pos);
     }
 
-    return 0;
+    return ret < 0 ? ret : 0;
+
 }
 
-static int vfio_add_ext_cap(VFIOPCIDevice *vdev)
+static void vfio_add_ext_cap(VFIOPCIDevice *vdev)
 {
     PCIDevice *pdev = &vdev->pdev;
     uint32_t header;
@@ -1793,7 +1796,7 @@ static int vfio_add_ext_cap(VFIOPCIDevice *vdev)
     /* Only add extended caps if we have them and the guest can see them */
     if (!pci_is_express(pdev) || !pci_bus_is_express(pdev->bus) ||
         !pci_get_long(pdev->config + PCI_CONFIG_SPACE_SIZE)) {
-        return 0;
+        return;
     }
 
     /*
@@ -1858,12 +1861,13 @@ static int vfio_add_ext_cap(VFIOPCIDevice *vdev)
     }
 
     g_free(config);
-    return 0;
+    return;
 }
 
-static int vfio_add_capabilities(VFIOPCIDevice *vdev)
+static int vfio_add_capabilities(VFIOPCIDevice *vdev, Error **errp)
 {
     PCIDevice *pdev = &vdev->pdev;
+    Error *err = NULL;
     int ret;
 
     if (!(pdev->config[PCI_STATUS] & PCI_STATUS_CAP_LIST) ||
@@ -1871,12 +1875,14 @@ static int vfio_add_capabilities(VFIOPCIDevice *vdev)
         return 0; /* Nothing to add */
     }
 
-    ret = vfio_add_std_cap(vdev, pdev->config[PCI_CAPABILITY_LIST]);
+    ret = vfio_add_std_cap(vdev, pdev->config[PCI_CAPABILITY_LIST], &err);
     if (ret) {
+        error_propagate(errp, err);
         return ret;
     }
 
-    return vfio_add_ext_cap(vdev);
+    vfio_add_ext_cap(vdev);
+    return 0;
 }
 
 static void vfio_pci_pre_reset(VFIOPCIDevice *vdev)
@@ -2680,7 +2686,7 @@ static int vfio_initfn(PCIDevice *pdev)
 
     vfio_bars_setup(vdev);
 
-    ret = vfio_add_capabilities(vdev);
+    ret = vfio_add_capabilities(vdev, &err);
     if (ret) {
         goto out_teardown;
     }
-- 
1.9.1

Re: [Qemu-devel] [PATCH v2 05/12] vfio/pci: Pass an error object to vfio_add_capabilities

[Markus Armbruster]

Eric Auger <eric.auger@redhat.com> writes:

> Pass an error object to prepare for migration to VFIO-PCI realize.
> The error is cascaded downto vfio_add_std_cap and then vfio_msi(x)_setup,
> vfio_setup_pcie_cap.
>
> vfio_add_ext_cap does not return anything else than 0 so let's transform
> it into a void function.
>
> Also use pci_add_capability2 which takes an error object.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  hw/vfio/pci.c | 66 ++++++++++++++++++++++++++++++++---------------------------
>  1 file changed, 36 insertions(+), 30 deletions(-)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index f67eec4..07a44f5 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -1178,7 +1178,7 @@ static void vfio_disable_interrupts(VFIOPCIDevice *vdev)
>      }
>  }
>  
> -static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos)
> +static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
>  {
>      uint16_t ctrl;
>      bool msi_64bit, msi_maskbit;
       int ret, entries;
       Error *err = NULL;

       if (pread(vdev->vbasedev.fd, &ctrl, sizeof(ctrl),
                 vdev->config_offset + pos + PCI_CAP_FLAGS) != sizeof(ctrl)) {
           return -errno;
       }

Fails without setting error here.

> @@ -1202,8 +1202,7 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos)
>          if (ret == -ENOTSUP) {
>              return 0;
>          }
> -        error_prepend(&err, "vfio: msi_init failed: ");
> -        error_report_err(err);
> +        error_propagate(errp, err);
>          return ret;
>      }
>      vdev->msi_cap_size = 0xa + (msi_maskbit ? 0xa : 0) + (msi_64bit ? 0x4 : 0);
> @@ -1361,7 +1360,7 @@ static int vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
>      return 0;
>  }
>  
> -static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos)
> +static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
>  {
>      int ret;
>  
> @@ -1376,7 +1375,7 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos)
>          if (ret == -ENOTSUP) {
>              return 0;
>          }
> -        error_report("vfio: msix_init failed");
> +        error_setg(errp, "msix_init failed");
>          return ret;
>      }
>  
> @@ -1561,7 +1560,8 @@ static void vfio_add_emulated_long(VFIOPCIDevice *vdev, int pos,
>      vfio_set_long_bits(vdev->emulated_config_bits + pos, mask, mask);
>  }
>  
> -static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size)
> +static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size,
> +                               Error **errp)
>  {
>      uint16_t flags;
>      uint8_t type;
> @@ -1573,8 +1573,8 @@ static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size)
>          type != PCI_EXP_TYPE_LEG_END &&
>          type != PCI_EXP_TYPE_RC_END) {
>  
> -        error_report("vfio: Assignment of PCIe type 0x%x "
> -                     "devices is not currently supported", type);
> +        error_setg(errp, "assignment of PCIe type 0x%x "
> +                   "devices is not currently supported", type);
>          return -EINVAL;
>      }
>  
> @@ -1708,10 +1708,11 @@ static void vfio_check_af_flr(VFIOPCIDevice *vdev, uint8_t pos)
>      }
>  }
>  
> -static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
> +static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos, Error **errp)
>  {
>      PCIDevice *pdev = &vdev->pdev;
>      uint8_t cap_id, next, size;
> +    Error *err = NULL;
>      int ret;
>  
>      cap_id = pdev->config[pos];
> @@ -1733,9 +1734,9 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
>       * will be changed as we unwind the stack.
>       */
>      if (next) {
> -        ret = vfio_add_std_cap(vdev, next);
> +        ret = vfio_add_std_cap(vdev, next, &err);
>          if (ret) {
> -            return ret;
> +            goto out;
>          }
>      } else {
>          /* Begin the rebuild, use QEMU emulated list bits */
> @@ -1749,40 +1750,42 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
>  
>      switch (cap_id) {
>      case PCI_CAP_ID_MSI:
> -        ret = vfio_msi_setup(vdev, pos);
> +        ret = vfio_msi_setup(vdev, pos, &err);
>          break;
>      case PCI_CAP_ID_EXP:
>          vfio_check_pcie_flr(vdev, pos);
> -        ret = vfio_setup_pcie_cap(vdev, pos, size);
> +        ret = vfio_setup_pcie_cap(vdev, pos, size, &err);
>          break;
>      case PCI_CAP_ID_MSIX:
> -        ret = vfio_msix_setup(vdev, pos);
> +        ret = vfio_msix_setup(vdev, pos, &err);
>          break;
>      case PCI_CAP_ID_PM:
>          vfio_check_pm_reset(vdev, pos);
>          vdev->pm_cap = pos;
> -        ret = pci_add_capability(pdev, cap_id, pos, size);
> +        ret = pci_add_capability2(pdev, cap_id, pos, size, &err);
>          break;
>      case PCI_CAP_ID_AF:
>          vfio_check_af_flr(vdev, pos);
> -        ret = pci_add_capability(pdev, cap_id, pos, size);
> +        ret = pci_add_capability2(pdev, cap_id, pos, size, &err);
>          break;
>      default:
> -        ret = pci_add_capability(pdev, cap_id, pos, size);
> +        ret = pci_add_capability2(pdev, cap_id, pos, size, &err);
>          break;
>      }
>  
> -    if (ret < 0) {
> -        error_report("vfio: %s Error adding PCI capability "
> -                     "0x%x[0x%x]@0x%x: %d", vdev->vbasedev.name,
> -                     cap_id, size, pos, ret);
> -        return ret;
> +out:
> +    error_propagate(errp, err);
> +    if (*errp) {
> +        error_prepend(errp,
> +                      "failed to add PCI capability 0x%x[0x%x]@0x%x: ",
> +                      cap_id, size, pos);
>      }

Less verbose, and therefore recommended by the big comment in error.h is
using errp instead of &err, then

   out:
       if (ret < 0) {
           error_prepend(errp,
                         "failed to add PCI capability 0x%x[0x%x]@0x%x: ",
                         cap_id, size, pos);
       }

Adding the error_propagate() now anyway can make sense when you get rid
of ret later in the series.

>  
> -    return 0;
> +    return ret < 0 ? ret : 0;
> +

Spurious blank line.

>  }
>  
> -static int vfio_add_ext_cap(VFIOPCIDevice *vdev)
> +static void vfio_add_ext_cap(VFIOPCIDevice *vdev)
>  {
>      PCIDevice *pdev = &vdev->pdev;
>      uint32_t header;
> @@ -1793,7 +1796,7 @@ static int vfio_add_ext_cap(VFIOPCIDevice *vdev)
>      /* Only add extended caps if we have them and the guest can see them */
>      if (!pci_is_express(pdev) || !pci_bus_is_express(pdev->bus) ||
>          !pci_get_long(pdev->config + PCI_CONFIG_SPACE_SIZE)) {
> -        return 0;
> +        return;
>      }
>  
>      /*
> @@ -1858,12 +1861,13 @@ static int vfio_add_ext_cap(VFIOPCIDevice *vdev)
>      }
>  
>      g_free(config);
> -    return 0;
> +    return;
>  }
>  
> -static int vfio_add_capabilities(VFIOPCIDevice *vdev)
> +static int vfio_add_capabilities(VFIOPCIDevice *vdev, Error **errp)
>  {
>      PCIDevice *pdev = &vdev->pdev;
> +    Error *err = NULL;
>      int ret;
>  
>      if (!(pdev->config[PCI_STATUS] & PCI_STATUS_CAP_LIST) ||
> @@ -1871,12 +1875,14 @@ static int vfio_add_capabilities(VFIOPCIDevice *vdev)
>          return 0; /* Nothing to add */
>      }
>  
> -    ret = vfio_add_std_cap(vdev, pdev->config[PCI_CAPABILITY_LIST]);
> +    ret = vfio_add_std_cap(vdev, pdev->config[PCI_CAPABILITY_LIST], &err);
>      if (ret) {
> +        error_propagate(errp, err);
>          return ret;
>      }

Likewise, as long as you have (and intend to keep) ret, do

       ret = vfio_add_std_cap(vdev, pdev->config[PCI_CAPABILITY_LIST], errp);
       if (ret) {
           return ret;
       }

>  
> -    return vfio_add_ext_cap(vdev);
> +    vfio_add_ext_cap(vdev);
> +    return 0;
>  }
>  
>  static void vfio_pci_pre_reset(VFIOPCIDevice *vdev)
> @@ -2680,7 +2686,7 @@ static int vfio_initfn(PCIDevice *pdev)
>  
>      vfio_bars_setup(vdev);
>  
> -    ret = vfio_add_capabilities(vdev);
> +    ret = vfio_add_capabilities(vdev, &err);
>      if (ret) {
>          goto out_teardown;
>      }

Re: [Qemu-devel] [PATCH v2 05/12] vfio/pci: Pass an error object to vfio_add_capabilities

[Auger Eric]

Hi Markus,

On 22/09/2016 18:52, Markus Armbruster wrote:
> Eric Auger <eric.auger@redhat.com> writes:
> 
>> Pass an error object to prepare for migration to VFIO-PCI realize.
>> The error is cascaded downto vfio_add_std_cap and then vfio_msi(x)_setup,
>> vfio_setup_pcie_cap.
>>
>> vfio_add_ext_cap does not return anything else than 0 so let's transform
>> it into a void function.
>>
>> Also use pci_add_capability2 which takes an error object.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  hw/vfio/pci.c | 66 ++++++++++++++++++++++++++++++++---------------------------
>>  1 file changed, 36 insertions(+), 30 deletions(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index f67eec4..07a44f5 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -1178,7 +1178,7 @@ static void vfio_disable_interrupts(VFIOPCIDevice *vdev)
>>      }
>>  }
>>  
>> -static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos)
>> +static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
>>  {
>>      uint16_t ctrl;
>>      bool msi_64bit, msi_maskbit;
>        int ret, entries;
>        Error *err = NULL;
> 
>        if (pread(vdev->vbasedev.fd, &ctrl, sizeof(ctrl),
>                  vdev->config_offset + pos + PCI_CAP_FLAGS) != sizeof(ctrl)) {
>            return -errno;
>        }
> 
> Fails without setting error here.
Sure
> 
>> @@ -1202,8 +1202,7 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos)
>>          if (ret == -ENOTSUP) {
>>              return 0;
>>          }
>> -        error_prepend(&err, "vfio: msi_init failed: ");
>> -        error_report_err(err);
>> +        error_propagate(errp, err);
>>          return ret;
>>      }
>>      vdev->msi_cap_size = 0xa + (msi_maskbit ? 0xa : 0) + (msi_64bit ? 0x4 : 0);
>> @@ -1361,7 +1360,7 @@ static int vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
>>      return 0;
>>  }
>>  
>> -static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos)
>> +static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
>>  {
>>      int ret;
>>  
>> @@ -1376,7 +1375,7 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos)
>>          if (ret == -ENOTSUP) {
>>              return 0;
>>          }
>> -        error_report("vfio: msix_init failed");
>> +        error_setg(errp, "msix_init failed");
>>          return ret;
>>      }
>>  
>> @@ -1561,7 +1560,8 @@ static void vfio_add_emulated_long(VFIOPCIDevice *vdev, int pos,
>>      vfio_set_long_bits(vdev->emulated_config_bits + pos, mask, mask);
>>  }
>>  
>> -static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size)
>> +static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size,
>> +                               Error **errp)
>>  {
>>      uint16_t flags;
>>      uint8_t type;
>> @@ -1573,8 +1573,8 @@ static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size)
>>          type != PCI_EXP_TYPE_LEG_END &&
>>          type != PCI_EXP_TYPE_RC_END) {
>>  
>> -        error_report("vfio: Assignment of PCIe type 0x%x "
>> -                     "devices is not currently supported", type);
>> +        error_setg(errp, "assignment of PCIe type 0x%x "
>> +                   "devices is not currently supported", type);
>>          return -EINVAL;
>>      }
>>  
>> @@ -1708,10 +1708,11 @@ static void vfio_check_af_flr(VFIOPCIDevice *vdev, uint8_t pos)
>>      }
>>  }
>>  
>> -static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
>> +static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos, Error **errp)
>>  {
>>      PCIDevice *pdev = &vdev->pdev;
>>      uint8_t cap_id, next, size;
>> +    Error *err = NULL;
>>      int ret;
>>  
>>      cap_id = pdev->config[pos];
>> @@ -1733,9 +1734,9 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
>>       * will be changed as we unwind the stack.
>>       */
>>      if (next) {
>> -        ret = vfio_add_std_cap(vdev, next);
>> +        ret = vfio_add_std_cap(vdev, next, &err);
>>          if (ret) {
>> -            return ret;
>> +            goto out;
>>          }
>>      } else {
>>          /* Begin the rebuild, use QEMU emulated list bits */
>> @@ -1749,40 +1750,42 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
>>  
>>      switch (cap_id) {
>>      case PCI_CAP_ID_MSI:
>> -        ret = vfio_msi_setup(vdev, pos);
>> +        ret = vfio_msi_setup(vdev, pos, &err);
>>          break;
>>      case PCI_CAP_ID_EXP:
>>          vfio_check_pcie_flr(vdev, pos);
>> -        ret = vfio_setup_pcie_cap(vdev, pos, size);
>> +        ret = vfio_setup_pcie_cap(vdev, pos, size, &err);
>>          break;
>>      case PCI_CAP_ID_MSIX:
>> -        ret = vfio_msix_setup(vdev, pos);
>> +        ret = vfio_msix_setup(vdev, pos, &err);
>>          break;
>>      case PCI_CAP_ID_PM:
>>          vfio_check_pm_reset(vdev, pos);
>>          vdev->pm_cap = pos;
>> -        ret = pci_add_capability(pdev, cap_id, pos, size);
>> +        ret = pci_add_capability2(pdev, cap_id, pos, size, &err);
>>          break;
>>      case PCI_CAP_ID_AF:
>>          vfio_check_af_flr(vdev, pos);
>> -        ret = pci_add_capability(pdev, cap_id, pos, size);
>> +        ret = pci_add_capability2(pdev, cap_id, pos, size, &err);
>>          break;
>>      default:
>> -        ret = pci_add_capability(pdev, cap_id, pos, size);
>> +        ret = pci_add_capability2(pdev, cap_id, pos, size, &err);
>>          break;
>>      }
>>  
>> -    if (ret < 0) {
>> -        error_report("vfio: %s Error adding PCI capability "
>> -                     "0x%x[0x%x]@0x%x: %d", vdev->vbasedev.name,
>> -                     cap_id, size, pos, ret);
>> -        return ret;
>> +out:
>> +    error_propagate(errp, err);
>> +    if (*errp) {
>> +        error_prepend(errp,
>> +                      "failed to add PCI capability 0x%x[0x%x]@0x%x: ",
>> +                      cap_id, size, pos);
>>      }
> 
> Less verbose, and therefore recommended by the big comment in error.h is
> using errp instead of &err, then
OK thanks for reminding me of this guideline.
> 
>    out:
>        if (ret < 0) {
>            error_prepend(errp,
>                          "failed to add PCI capability 0x%x[0x%x]@0x%x: ",
>                          cap_id, size, pos);
>        }
> 
> Adding the error_propagate() now anyway can make sense when you get rid
> of ret later in the series.
Yes. So I dared to keep it as is and added a comment in the commit message.

> 
>>  
>> -    return 0;
>> +    return ret < 0 ? ret : 0;
>> +
> 
> Spurious blank line.
OK
> 
>>  }
>>  
>> -static int vfio_add_ext_cap(VFIOPCIDevice *vdev)
>> +static void vfio_add_ext_cap(VFIOPCIDevice *vdev)
>>  {
>>      PCIDevice *pdev = &vdev->pdev;
>>      uint32_t header;
>> @@ -1793,7 +1796,7 @@ static int vfio_add_ext_cap(VFIOPCIDevice *vdev)
>>      /* Only add extended caps if we have them and the guest can see them */
>>      if (!pci_is_express(pdev) || !pci_bus_is_express(pdev->bus) ||
>>          !pci_get_long(pdev->config + PCI_CONFIG_SPACE_SIZE)) {
>> -        return 0;
>> +        return;
>>      }
>>  
>>      /*
>> @@ -1858,12 +1861,13 @@ static int vfio_add_ext_cap(VFIOPCIDevice *vdev)
>>      }
>>  
>>      g_free(config);
>> -    return 0;
>> +    return;
>>  }
>>  
>> -static int vfio_add_capabilities(VFIOPCIDevice *vdev)
>> +static int vfio_add_capabilities(VFIOPCIDevice *vdev, Error **errp)
>>  {
>>      PCIDevice *pdev = &vdev->pdev;
>> +    Error *err = NULL;
>>      int ret;
>>  
>>      if (!(pdev->config[PCI_STATUS] & PCI_STATUS_CAP_LIST) ||
>> @@ -1871,12 +1875,14 @@ static int vfio_add_capabilities(VFIOPCIDevice *vdev)
>>          return 0; /* Nothing to add */
>>      }
>>  
>> -    ret = vfio_add_std_cap(vdev, pdev->config[PCI_CAPABILITY_LIST]);
>> +    ret = vfio_add_std_cap(vdev, pdev->config[PCI_CAPABILITY_LIST], &err);
>>      if (ret) {
>> +        error_propagate(errp, err);
>>          return ret;
>>      }
> 
> Likewise, as long as you have (and intend to keep) ret, do
> 
>        ret = vfio_add_std_cap(vdev, pdev->config[PCI_CAPABILITY_LIST], errp);
>        if (ret) {
>            return ret;
>        }
Agreed

Thanks

Eric
> 
>>  
>> -    return vfio_add_ext_cap(vdev);
>> +    vfio_add_ext_cap(vdev);
>> +    return 0;
>>  }
>>  
>>  static void vfio_pci_pre_reset(VFIOPCIDevice *vdev)
>> @@ -2680,7 +2686,7 @@ static int vfio_initfn(PCIDevice *pdev)
>>  
>>      vfio_bars_setup(vdev);
>>  
>> -    ret = vfio_add_capabilities(vdev);
>> +    ret = vfio_add_capabilities(vdev, &err);
>>      if (ret) {
>>          goto out_teardown;
>>      }
> 

[Qemu-devel] [PATCH v2 06/12] vfio/pci: Pass an error object to vfio_pci_igd_opregion_init

[Eric Auger]

Pass an error object to prepare for migration to VFIO-PCI realize.

In vfio_probe_igd_bar4_quirk, simply report the error without
propagating.

Transform vfio_pci_igd_opregion_init into a void function. The
-EINVAL returned value only is used in vfio_initfn and during
migration to realize, this will not be used anymore.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 hw/vfio/pci-quirks.c | 19 +++++++++----------
 hw/vfio/pci.c        |  6 +++---
 hw/vfio/pci.h        |  5 +++--
 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
index bec694c..57921de 100644
--- a/hw/vfio/pci-quirks.c
+++ b/hw/vfio/pci-quirks.c
@@ -1055,8 +1055,8 @@ typedef struct VFIOIGDQuirk {
  * the table and to write the base address of that memory to the ASLS register
  * of the IGD device.
  */
-int vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev,
-                               struct vfio_region_info *info)
+void vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev,
+                               struct vfio_region_info *info, Error **errp)
 {
     int ret;
 
@@ -1064,10 +1064,10 @@ int vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev,
     ret = pread(vdev->vbasedev.fd, vdev->igd_opregion,
                 info->size, info->offset);
     if (ret != info->size) {
-        error_report("vfio: Error reading IGD OpRegion");
+        error_setg(errp, "failed to read IGD OpRegion");
         g_free(vdev->igd_opregion);
         vdev->igd_opregion = NULL;
-        return -EINVAL;
+        return;
     }
 
     /*
@@ -1091,8 +1091,6 @@ int vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev,
     pci_set_long(vdev->pdev.config + IGD_ASLS, 0);
     pci_set_long(vdev->pdev.wmask + IGD_ASLS, ~0);
     pci_set_long(vdev->emulated_config_bits + IGD_ASLS, ~0);
-
-    return 0;
 }
 
 /*
@@ -1363,6 +1361,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
@@ -1487,10 +1486,10 @@ static void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
     }
 
     /* Setup OpRegion access */
-    ret = vfio_pci_igd_opregion_init(vdev, opregion);
-    if (ret) {
-        error_report("IGD device %s failed to setup OpRegion, "
-                     "legacy mode disabled", vdev->vbasedev.name);
+    vfio_pci_igd_opregion_init(vdev, opregion, &err);
+    if (err) {
+        error_append_hint(&err, "IGD legacy mode disabled\n");
+        error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name);
         goto out;
     }
 
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 07a44f5..f9a4fe7 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2720,10 +2720,10 @@ static int vfio_initfn(PCIDevice *pdev)
             goto out_teardown;
         }
 
-        ret = vfio_pci_igd_opregion_init(vdev, opregion);
+        vfio_pci_igd_opregion_init(vdev, opregion, &err);
         g_free(opregion);
-        if (ret) {
-            error_setg_errno(&err, -ret, "IGD OpRegion initialization failed");
+        if (err) {
+            ret = -EINVAL;
             goto out_teardown;
         }
     }
diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
index 7d482d9..5336aa4 100644
--- a/hw/vfio/pci.h
+++ b/hw/vfio/pci.h
@@ -163,7 +163,8 @@ void vfio_setup_resetfn_quirk(VFIOPCIDevice *vdev);
 
 int vfio_populate_vga(VFIOPCIDevice *vdev);
 
-int vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev,
-                               struct vfio_region_info *info);
+void vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev,
+                                struct vfio_region_info *info,
+                                Error **errp);
 
 #endif /* HW_VFIO_VFIO_PCI_H */
-- 
1.9.1

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

[Eric Auger]

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

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

[Markus Armbruster]

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

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

[Qemu-devel] [PATCH v2 08/12] vfio: Pass an error object to vfio_get_device

[Eric Auger]

Pass an error object to prepare for migration to VFIO-PCI realize.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 hw/vfio/common.c              | 13 +++++++------
 hw/vfio/pci.c                 |  3 +--
 hw/vfio/platform.c            |  5 ++---
 include/hw/vfio/vfio-common.h |  2 +-
 4 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index ef9e4cd..603e915 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1203,23 +1203,24 @@ void vfio_put_group(VFIOGroup *group)
 }
 
 int vfio_get_device(VFIOGroup *group, const char *name,
-                       VFIODevice *vbasedev)
+                    VFIODevice *vbasedev, Error **errp)
 {
     struct vfio_device_info dev_info = { .argsz = sizeof(dev_info) };
     int ret, fd;
 
     fd = ioctl(group->fd, VFIO_GROUP_GET_DEVICE_FD, name);
     if (fd < 0) {
-        error_report("vfio: error getting device %s from group %d: %m",
-                     name, group->groupid);
-        error_printf("Verify all devices in group %d are bound to vfio-<bus> "
-                     "or pci-stub and not already in use\n", group->groupid);
+        error_setg_errno(errp, errno, "error getting device from group %d",
+                         group->groupid);
+        error_append_hint(errp,
+                      "Verify all devices in group %d are bound to vfio-<bus> "
+                      "or pci-stub and not already in use\n", group->groupid);
         return fd;
     }
 
     ret = ioctl(fd, VFIO_DEVICE_GET_INFO, &dev_info);
     if (ret) {
-        error_report("vfio: error getting device info: %m");
+        error_setg_errno(errp, errno, "error getting device info");
         close(fd);
         return ret;
     }
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 1173d4a..3d43718 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2577,9 +2577,8 @@ static int vfio_initfn(PCIDevice *pdev)
         }
     }
 
-    ret = vfio_get_device(group, vdev->vbasedev.name, &vdev->vbasedev);
+    ret = vfio_get_device(group, vdev->vbasedev.name, &vdev->vbasedev, &err);
     if (ret) {
-        error_setg_errno(&err, -ret, "failed to get device");
         vfio_put_group(group);
         goto error;
     }
diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
index 7bf525b..9014ea7 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -607,11 +607,10 @@ static int vfio_base_device_init(VFIODevice *vbasedev)
             return -EBUSY;
         }
     }
-    ret = vfio_get_device(group, vbasedev->name, vbasedev);
+    ret = vfio_get_device(group, vbasedev->name, vbasedev, &err);
     if (ret) {
-        error_report("vfio: failed to get device %s", vbasedev->name);
         vfio_put_group(group);
-        return ret;
+        goto error;
     }
 
     ret = vfio_populate_device(vbasedev);
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 4fb6fc3..7790e70 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -158,7 +158,7 @@ void vfio_reset_handler(void *opaque);
 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);
+                    VFIODevice *vbasedev, Error **errp);
 
 extern const MemoryRegionOps vfio_region_ops;
 extern QLIST_HEAD(vfio_group_head, VFIOGroup) vfio_group_list;
-- 
1.9.1

[Qemu-devel] [PATCH v2 09/12] vfio/pci: Conversion to realize

[Eric Auger]

This patch converts VFIO PCI to realize function.

Also original initfn errors now are propagated using QEMU
error objects. All errors are formatted with the same pattern:
"vfio: %s: the error description"

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

---

v1 -> v2:
- correct error_setg_errno with positive error values
---
 hw/vfio/pci.c        | 52 ++++++++++++++++++++++------------------------------
 hw/vfio/trace-events |  2 +-
 2 files changed, 23 insertions(+), 31 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 3d43718..857d89f 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2511,7 +2511,7 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
     vdev->req_enabled = false;
 }
 
-static int vfio_initfn(PCIDevice *pdev)
+static void vfio_realize(PCIDevice *pdev, Error **errp)
 {
     VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
     VFIODevice *vbasedev_iter;
@@ -2531,9 +2531,9 @@ static int vfio_initfn(PCIDevice *pdev)
     }
 
     if (stat(vdev->vbasedev.sysfsdev, &st) < 0) {
-        error_setg(&err, "no such host device");
-        ret = -errno;
-        goto error;
+        error_setg(errp, "no such host device");
+        error_prepend(errp, ERR_PREFIX, vdev->vbasedev.sysfsdev);
+        return;
     }
 
     vdev->vbasedev.name = g_strdup(basename(vdev->vbasedev.sysfsdev));
@@ -2545,9 +2545,8 @@ static int vfio_initfn(PCIDevice *pdev)
     g_free(tmp);
 
     if (len <= 0 || len >= sizeof(group_path)) {
-        error_setg_errno(&err, len < 0 ? errno : ENAMETOOLONG,
+        error_setg_errno(errp, len < 0 ? errno : ENAMETOOLONG,
                          "no iommu_group found");
-        ret = len < 0 ? -errno : -ENAMETOOLONG;
         goto error;
     }
 
@@ -2555,24 +2554,21 @@ static int vfio_initfn(PCIDevice *pdev)
 
     group_name = basename(group_path);
     if (sscanf(group_name, "%d", &groupid) != 1) {
-        error_setg_errno(&err, errno, "failed to read %s", group_path);
-        ret = -errno;
+        error_setg_errno(errp, errno, "failed to read %s", group_path);
         goto error;
     }
 
-    trace_vfio_initfn(vdev->vbasedev.name, groupid);
+    trace_vfio_realize(vdev->vbasedev.name, groupid);
 
     group = vfio_get_group(groupid, pci_device_iommu_address_space(pdev), &err);
     if (!group) {
-        ret = -ENOENT;
         goto error;
     }
 
     QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
         if (strcmp(vbasedev_iter->name, vdev->vbasedev.name) == 0) {
-            error_setg(&err, "device is already attached");
+            error_setg(errp, "device is already attached");
             vfio_put_group(group);
-            ret = -EBUSY;
             goto error;
         }
     }
@@ -2594,7 +2590,7 @@ static int vfio_initfn(PCIDevice *pdev)
                 vdev->config_offset);
     if (ret < (int)MIN(pci_config_size(&vdev->pdev), vdev->config_size)) {
         ret = ret < 0 ? -errno : -EFAULT;
-        error_setg_errno(&err, -ret, "failed to read device config space");
+        error_setg_errno(errp, -ret, "failed to read device config space");
         goto error;
     }
 
@@ -2611,8 +2607,7 @@ static int vfio_initfn(PCIDevice *pdev)
      */
     if (vdev->vendor_id != PCI_ANY_ID) {
         if (vdev->vendor_id >= 0xffff) {
-            error_setg(&err, "invalid PCI vendor ID provided");
-            ret = -EINVAL;
+            error_setg(errp, "invalid PCI vendor ID provided");
             goto error;
         }
         vfio_add_emulated_word(vdev, PCI_VENDOR_ID, vdev->vendor_id, ~0);
@@ -2623,8 +2618,7 @@ static int vfio_initfn(PCIDevice *pdev)
 
     if (vdev->device_id != PCI_ANY_ID) {
         if (vdev->device_id > 0xffff) {
-            error_setg(&err, "invalid PCI device ID provided");
-            ret = -EINVAL;
+            error_setg(errp, "invalid PCI device ID provided");
             goto error;
         }
         vfio_add_emulated_word(vdev, PCI_DEVICE_ID, vdev->device_id, ~0);
@@ -2635,8 +2629,7 @@ static int vfio_initfn(PCIDevice *pdev)
 
     if (vdev->sub_vendor_id != PCI_ANY_ID) {
         if (vdev->sub_vendor_id > 0xffff) {
-            error_setg(&err, "invalid PCI subsystem vendor ID provided");
-            ret = -EINVAL;
+            error_setg(errp, "invalid PCI subsystem vendor ID provided");
             goto error;
         }
         vfio_add_emulated_word(vdev, PCI_SUBSYSTEM_VENDOR_ID,
@@ -2647,8 +2640,7 @@ static int vfio_initfn(PCIDevice *pdev)
 
     if (vdev->sub_device_id != PCI_ANY_ID) {
         if (vdev->sub_device_id > 0xffff) {
-            error_setg(&err, "invalid PCI subsystem device ID provided");
-            ret = -EINVAL;
+            error_setg(errp, "invalid PCI subsystem device ID provided");
             goto error;
         }
         vfio_add_emulated_word(vdev, PCI_SUBSYSTEM_ID, vdev->sub_device_id, ~0);
@@ -2702,10 +2694,9 @@ static int vfio_initfn(PCIDevice *pdev)
         struct vfio_region_info *opregion;
 
         if (vdev->pdev.qdev.hotplugged) {
-            error_setg(&err,
+            error_setg(errp,
                        "cannot support IGD OpRegion feature on hotplugged "
                        "device");
-            ret = -EINVAL;
             goto out_teardown;
         }
 
@@ -2713,7 +2704,7 @@ static int vfio_initfn(PCIDevice *pdev)
                         VFIO_REGION_TYPE_PCI_VENDOR_TYPE | PCI_VENDOR_ID_INTEL,
                         VFIO_REGION_SUBTYPE_INTEL_IGD_OPREGION, &opregion);
         if (ret) {
-            error_setg_errno(&err, -ret,
+            error_setg_errno(errp, -ret,
                              "does not support requested IGD OpRegion feature");
             goto out_teardown;
         }
@@ -2721,7 +2712,6 @@ static int vfio_initfn(PCIDevice *pdev)
         vfio_pci_igd_opregion_init(vdev, opregion, &err);
         g_free(opregion);
         if (err) {
-            ret = -EINVAL;
             goto out_teardown;
         }
     }
@@ -2743,6 +2733,7 @@ static int vfio_initfn(PCIDevice *pdev)
         pci_device_set_intx_routing_notifier(&vdev->pdev, vfio_intx_update);
         ret = vfio_intx_enable(vdev, &err);
         if (ret) {
+            error_setg_errno(errp, -ret, "failed to enable intx");
             goto out_teardown;
         }
     }
@@ -2751,17 +2742,18 @@ static int vfio_initfn(PCIDevice *pdev)
     vfio_register_req_notifier(vdev);
     vfio_setup_resetfn_quirk(vdev);
 
-    return 0;
+    return;
 
 out_teardown:
     pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
     vfio_teardown_msi(vdev);
     vfio_bars_exit(vdev);
 error:
-    if (err) {
-        error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name);
+    error_propagate(errp, err);
+
+    if (*errp) {
+        error_prepend(errp, ERR_PREFIX, vdev->vbasedev.name);
     }
-    return ret;
 }
 
 static void vfio_instance_finalize(Object *obj)
@@ -2890,7 +2882,7 @@ static void vfio_pci_dev_class_init(ObjectClass *klass, void *data)
     dc->vmsd = &vfio_pci_vmstate;
     dc->desc = "VFIO-based PCI device assignment";
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
-    pdc->init = vfio_initfn;
+    pdc->realize = vfio_realize;
     pdc->exit = vfio_exitfn;
     pdc->config_read = vfio_pci_read_config;
     pdc->config_write = vfio_pci_write_config;
diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
index da13322..ef81609 100644
--- a/hw/vfio/trace-events
+++ b/hw/vfio/trace-events
@@ -36,7 +36,7 @@ vfio_pci_hot_reset_dep_devices(int domain, int bus, int slot, int function, int
 vfio_pci_hot_reset_result(const char *name, const char *result) "%s hot reset: %s"
 vfio_populate_device_config(const char *name, unsigned long size, unsigned long offset, unsigned long flags) "Device %s config:\n  size: 0x%lx, offset: 0x%lx, flags: 0x%lx"
 vfio_populate_device_get_irq_info_failure(void) "VFIO_DEVICE_GET_IRQ_INFO failure: %m"
-vfio_initfn(const char *name, int group_id) " (%s) group %d"
+vfio_realize(const char *name, int group_id) " (%s) group %d"
 vfio_add_ext_cap_dropped(const char *name, uint16_t cap, uint16_t offset) "%s %x@%x"
 vfio_pci_reset(const char *name) " (%s)"
 vfio_pci_reset_flr(const char *name) "%s FLR/VFIO_DEVICE_RESET"
-- 
1.9.1

Re: [Qemu-devel] [PATCH v2 09/12] vfio/pci: Conversion to realize

[Markus Armbruster]

Eric Auger <eric.auger@redhat.com> writes:

> This patch converts VFIO PCI to realize function.
>
> Also original initfn errors now are propagated using QEMU
> error objects. All errors are formatted with the same pattern:
> "vfio: %s: the error description"
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>
> ---
>
> v1 -> v2:
> - correct error_setg_errno with positive error values
> ---
>  hw/vfio/pci.c        | 52 ++++++++++++++++++++++------------------------------
>  hw/vfio/trace-events |  2 +-
>  2 files changed, 23 insertions(+), 31 deletions(-)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 3d43718..857d89f 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2511,7 +2511,7 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
>      vdev->req_enabled = false;
>  }
>  
> -static int vfio_initfn(PCIDevice *pdev)
> +static void vfio_realize(PCIDevice *pdev, Error **errp)
>  {
>      VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
>      VFIODevice *vbasedev_iter;
> @@ -2531,9 +2531,9 @@ static int vfio_initfn(PCIDevice *pdev)
>      }
>  
>      if (stat(vdev->vbasedev.sysfsdev, &st) < 0) {
> -        error_setg(&err, "no such host device");
> -        ret = -errno;
> -        goto error;
> +        error_setg(errp, "no such host device");
> +        error_prepend(errp, ERR_PREFIX, vdev->vbasedev.sysfsdev);
> +        return;

Could do

           error_setg(errp, "no such host device");
           goto error;

Your choice.

>      }
>  
>      vdev->vbasedev.name = g_strdup(basename(vdev->vbasedev.sysfsdev));
> @@ -2545,9 +2545,8 @@ static int vfio_initfn(PCIDevice *pdev)
>      g_free(tmp);
>  
>      if (len <= 0 || len >= sizeof(group_path)) {
> -        error_setg_errno(&err, len < 0 ? errno : ENAMETOOLONG,
> +        error_setg_errno(errp, len < 0 ? errno : ENAMETOOLONG,
>                           "no iommu_group found");
> -        ret = len < 0 ? -errno : -ENAMETOOLONG;
>          goto error;
>      }
>  

Due to this change and others below, ret is no longer valid at the error
label (fine, it's not used), and the error is set either via err or via
errp.  Works, because error_propagate(errp, err) does the right thing
then: nothing when err is null.  However, changing the goto to jump
behind the error_propagate when using errp rather than err might be
clearer.

Continuing to set the error via err also works.  Less churn.

Your choice.

> @@ -2555,24 +2554,21 @@ static int vfio_initfn(PCIDevice *pdev)
>  
>      group_name = basename(group_path);
>      if (sscanf(group_name, "%d", &groupid) != 1) {
> -        error_setg_errno(&err, errno, "failed to read %s", group_path);
> -        ret = -errno;
> +        error_setg_errno(errp, errno, "failed to read %s", group_path);
>          goto error;
>      }
>  
> -    trace_vfio_initfn(vdev->vbasedev.name, groupid);
> +    trace_vfio_realize(vdev->vbasedev.name, groupid);
>  
>      group = vfio_get_group(groupid, pci_device_iommu_address_space(pdev), &err);
>      if (!group) {
> -        ret = -ENOENT;
>          goto error;
>      }

Since you don't examine the error, you could use errp rather than &err.
Perhaps not worth the churn unless you can get rid of err entirely.

>  
>      QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
>          if (strcmp(vbasedev_iter->name, vdev->vbasedev.name) == 0) {
> -            error_setg(&err, "device is already attached");
> +            error_setg(errp, "device is already attached");
>              vfio_put_group(group);
> -            ret = -EBUSY;
>              goto error;
>          }
>      }
> @@ -2594,7 +2590,7 @@ static int vfio_initfn(PCIDevice *pdev)
>                  vdev->config_offset);
>      if (ret < (int)MIN(pci_config_size(&vdev->pdev), vdev->config_size)) {
>          ret = ret < 0 ? -errno : -EFAULT;
> -        error_setg_errno(&err, -ret, "failed to read device config space");
> +        error_setg_errno(errp, -ret, "failed to read device config space");
>          goto error;
>      }
>  
> @@ -2611,8 +2607,7 @@ static int vfio_initfn(PCIDevice *pdev)
>       */
>      if (vdev->vendor_id != PCI_ANY_ID) {
>          if (vdev->vendor_id >= 0xffff) {
> -            error_setg(&err, "invalid PCI vendor ID provided");
> -            ret = -EINVAL;
> +            error_setg(errp, "invalid PCI vendor ID provided");
>              goto error;
>          }
>          vfio_add_emulated_word(vdev, PCI_VENDOR_ID, vdev->vendor_id, ~0);
> @@ -2623,8 +2618,7 @@ static int vfio_initfn(PCIDevice *pdev)
>  
>      if (vdev->device_id != PCI_ANY_ID) {
>          if (vdev->device_id > 0xffff) {
> -            error_setg(&err, "invalid PCI device ID provided");
> -            ret = -EINVAL;
> +            error_setg(errp, "invalid PCI device ID provided");
>              goto error;
>          }
>          vfio_add_emulated_word(vdev, PCI_DEVICE_ID, vdev->device_id, ~0);
> @@ -2635,8 +2629,7 @@ static int vfio_initfn(PCIDevice *pdev)
>  
>      if (vdev->sub_vendor_id != PCI_ANY_ID) {
>          if (vdev->sub_vendor_id > 0xffff) {
> -            error_setg(&err, "invalid PCI subsystem vendor ID provided");
> -            ret = -EINVAL;
> +            error_setg(errp, "invalid PCI subsystem vendor ID provided");
>              goto error;
>          }
>          vfio_add_emulated_word(vdev, PCI_SUBSYSTEM_VENDOR_ID,
> @@ -2647,8 +2640,7 @@ static int vfio_initfn(PCIDevice *pdev)
>  
>      if (vdev->sub_device_id != PCI_ANY_ID) {
>          if (vdev->sub_device_id > 0xffff) {
> -            error_setg(&err, "invalid PCI subsystem device ID provided");
> -            ret = -EINVAL;
> +            error_setg(errp, "invalid PCI subsystem device ID provided");
>              goto error;
>          }
>          vfio_add_emulated_word(vdev, PCI_SUBSYSTEM_ID, vdev->sub_device_id, ~0);
> @@ -2702,10 +2694,9 @@ static int vfio_initfn(PCIDevice *pdev)
>          struct vfio_region_info *opregion;
>  
>          if (vdev->pdev.qdev.hotplugged) {
> -            error_setg(&err,
> +            error_setg(errp,
>                         "cannot support IGD OpRegion feature on hotplugged "
>                         "device");
> -            ret = -EINVAL;
>              goto out_teardown;
>          }
>  
> @@ -2713,7 +2704,7 @@ static int vfio_initfn(PCIDevice *pdev)
>                          VFIO_REGION_TYPE_PCI_VENDOR_TYPE | PCI_VENDOR_ID_INTEL,
>                          VFIO_REGION_SUBTYPE_INTEL_IGD_OPREGION, &opregion);
>          if (ret) {
> -            error_setg_errno(&err, -ret,
> +            error_setg_errno(errp, -ret,
>                               "does not support requested IGD OpRegion feature");
>              goto out_teardown;
>          }
> @@ -2721,7 +2712,6 @@ static int vfio_initfn(PCIDevice *pdev)
>          vfio_pci_igd_opregion_init(vdev, opregion, &err);
>          g_free(opregion);
>          if (err) {
> -            ret = -EINVAL;
>              goto out_teardown;
>          }
>      }
> @@ -2743,6 +2733,7 @@ static int vfio_initfn(PCIDevice *pdev)
>          pci_device_set_intx_routing_notifier(&vdev->pdev, vfio_intx_update);
>          ret = vfio_intx_enable(vdev, &err);
>          if (ret) {
> +            error_setg_errno(errp, -ret, "failed to enable intx");

If vfio_intx_enable() sets an error on any failure (as it should), then
the later error_propagate(errp, err) will simply free err.  Adding the
error_setg_errno() here overrides the specific error set by
vfio_intx_enable() with a generic one.  Looks wrong to me.

If it doesn't, then the error_propagate() will work around that
misbehavior.  You should fix vfio_intx_enable() then.

>              goto out_teardown;
>          }
>      }
> @@ -2751,17 +2742,18 @@ static int vfio_initfn(PCIDevice *pdev)
>      vfio_register_req_notifier(vdev);
>      vfio_setup_resetfn_quirk(vdev);
>  
> -    return 0;
> +    return;
>  
>  out_teardown:
>      pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
>      vfio_teardown_msi(vdev);
>      vfio_bars_exit(vdev);
>  error:
> -    if (err) {
> -        error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name);
> +    error_propagate(errp, err);
> +
> +    if (*errp) {
> +        error_prepend(errp, ERR_PREFIX, vdev->vbasedev.name);
>      }

Dereferencing errp is unclean, because callers may pass null to ignore
errors.  The qdev core doesn't.  However, error_prepend() does the right
thing when passed null, namely nothing.  Simply call it unconditionally.

> -    return ret;
>  }
>  
>  static void vfio_instance_finalize(Object *obj)
> @@ -2890,7 +2882,7 @@ static void vfio_pci_dev_class_init(ObjectClass *klass, void *data)
>      dc->vmsd = &vfio_pci_vmstate;
>      dc->desc = "VFIO-based PCI device assignment";
>      set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> -    pdc->init = vfio_initfn;
> +    pdc->realize = vfio_realize;
>      pdc->exit = vfio_exitfn;
>      pdc->config_read = vfio_pci_read_config;
>      pdc->config_write = vfio_pci_write_config;
> diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
> index da13322..ef81609 100644
> --- a/hw/vfio/trace-events
> +++ b/hw/vfio/trace-events
> @@ -36,7 +36,7 @@ vfio_pci_hot_reset_dep_devices(int domain, int bus, int slot, int function, int
>  vfio_pci_hot_reset_result(const char *name, const char *result) "%s hot reset: %s"
>  vfio_populate_device_config(const char *name, unsigned long size, unsigned long offset, unsigned long flags) "Device %s config:\n  size: 0x%lx, offset: 0x%lx, flags: 0x%lx"
>  vfio_populate_device_get_irq_info_failure(void) "VFIO_DEVICE_GET_IRQ_INFO failure: %m"
> -vfio_initfn(const char *name, int group_id) " (%s) group %d"
> +vfio_realize(const char *name, int group_id) " (%s) group %d"
>  vfio_add_ext_cap_dropped(const char *name, uint16_t cap, uint16_t offset) "%s %x@%x"
>  vfio_pci_reset(const char *name) " (%s)"
>  vfio_pci_reset_flr(const char *name) "%s FLR/VFIO_DEVICE_RESET"

Re: [Qemu-devel] [PATCH v2 09/12] vfio/pci: Conversion to realize

[Auger Eric]

Hi,

On 22/09/2016 19:24, Markus Armbruster wrote:
> Eric Auger <eric.auger@redhat.com> writes:
> 
>> This patch converts VFIO PCI to realize function.
>>
>> Also original initfn errors now are propagated using QEMU
>> error objects. All errors are formatted with the same pattern:
>> "vfio: %s: the error description"
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
>>
>> v1 -> v2:
>> - correct error_setg_errno with positive error values
>> ---
>>  hw/vfio/pci.c        | 52 ++++++++++++++++++++++------------------------------
>>  hw/vfio/trace-events |  2 +-
>>  2 files changed, 23 insertions(+), 31 deletions(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index 3d43718..857d89f 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -2511,7 +2511,7 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
>>      vdev->req_enabled = false;
>>  }
>>  
>> -static int vfio_initfn(PCIDevice *pdev)
>> +static void vfio_realize(PCIDevice *pdev, Error **errp)
>>  {
>>      VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
>>      VFIODevice *vbasedev_iter;
>> @@ -2531,9 +2531,9 @@ static int vfio_initfn(PCIDevice *pdev)
>>      }
>>  
>>      if (stat(vdev->vbasedev.sysfsdev, &st) < 0) {
>> -        error_setg(&err, "no such host device");
>> -        ret = -errno;
>> -        goto error;
>> +        error_setg(errp, "no such host device");
>> +        error_prepend(errp, ERR_PREFIX, vdev->vbasedev.sysfsdev);
>> +        return;
> 
> Could do
> 
>            error_setg(errp, "no such host device");
>            goto error;
> 
> Your choice.
At that stage the vdev->vbasedev.name is not populated yet.
> 
>>      }
>>  
>>      vdev->vbasedev.name = g_strdup(basename(vdev->vbasedev.sysfsdev));
>> @@ -2545,9 +2545,8 @@ static int vfio_initfn(PCIDevice *pdev)
>>      g_free(tmp);
>>  
>>      if (len <= 0 || len >= sizeof(group_path)) {
>> -        error_setg_errno(&err, len < 0 ? errno : ENAMETOOLONG,
>> +        error_setg_errno(errp, len < 0 ? errno : ENAMETOOLONG,
>>                           "no iommu_group found");
>> -        ret = len < 0 ? -errno : -ENAMETOOLONG;
>>          goto error;
>>      }
>>  
> 
> Due to this change and others below, ret is no longer valid at the error
> label (fine, it's not used), and the error is set either via err or via
> errp.  Works, because error_propagate(errp, err) does the right thing
> then: nothing when err is null.  However, changing the goto to jump
> behind the error_propagate when using errp rather than err might be
> clearer.
Let's use a local error when I can't do otherwise then.
> 
> Continuing to set the error via err also works.  Less churn.
> 
> Your choice.
> 
>> @@ -2555,24 +2554,21 @@ static int vfio_initfn(PCIDevice *pdev)
>>  
>>      group_name = basename(group_path);
>>      if (sscanf(group_name, "%d", &groupid) != 1) {
>> -        error_setg_errno(&err, errno, "failed to read %s", group_path);
>> -        ret = -errno;
>> +        error_setg_errno(errp, errno, "failed to read %s", group_path);
>>          goto error;
>>      }
>>  
>> -    trace_vfio_initfn(vdev->vbasedev.name, groupid);
>> +    trace_vfio_realize(vdev->vbasedev.name, groupid);
>>  
>>      group = vfio_get_group(groupid, pci_device_iommu_address_space(pdev), &err);
>>      if (!group) {
>> -        ret = -ENOENT;
>>          goto error;
>>      }
> 
> Since you don't examine the error, you could use errp rather than &err.
> Perhaps not worth the churn unless you can get rid of err entirely.
Not possible to completely get rid of local err due to
vfio_pci_igd_opregion_init - since can't dereference errp  -but I can do
a local error_propagate.
> 
>>  
>>      QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
>>          if (strcmp(vbasedev_iter->name, vdev->vbasedev.name) == 0) {
>> -            error_setg(&err, "device is already attached");
>> +            error_setg(errp, "device is already attached");
>>              vfio_put_group(group);
>> -            ret = -EBUSY;
>>              goto error;
>>          }
>>      }
>> @@ -2594,7 +2590,7 @@ static int vfio_initfn(PCIDevice *pdev)
>>                  vdev->config_offset);
>>      if (ret < (int)MIN(pci_config_size(&vdev->pdev), vdev->config_size)) {
>>          ret = ret < 0 ? -errno : -EFAULT;
>> -        error_setg_errno(&err, -ret, "failed to read device config space");
>> +        error_setg_errno(errp, -ret, "failed to read device config space");
>>          goto error;
>>      }
>>  
>> @@ -2611,8 +2607,7 @@ static int vfio_initfn(PCIDevice *pdev)
>>       */
>>      if (vdev->vendor_id != PCI_ANY_ID) {
>>          if (vdev->vendor_id >= 0xffff) {
>> -            error_setg(&err, "invalid PCI vendor ID provided");
>> -            ret = -EINVAL;
>> +            error_setg(errp, "invalid PCI vendor ID provided");
>>              goto error;
>>          }
>>          vfio_add_emulated_word(vdev, PCI_VENDOR_ID, vdev->vendor_id, ~0);
>> @@ -2623,8 +2618,7 @@ static int vfio_initfn(PCIDevice *pdev)
>>  
>>      if (vdev->device_id != PCI_ANY_ID) {
>>          if (vdev->device_id > 0xffff) {
>> -            error_setg(&err, "invalid PCI device ID provided");
>> -            ret = -EINVAL;
>> +            error_setg(errp, "invalid PCI device ID provided");
>>              goto error;
>>          }
>>          vfio_add_emulated_word(vdev, PCI_DEVICE_ID, vdev->device_id, ~0);
>> @@ -2635,8 +2629,7 @@ static int vfio_initfn(PCIDevice *pdev)
>>  
>>      if (vdev->sub_vendor_id != PCI_ANY_ID) {
>>          if (vdev->sub_vendor_id > 0xffff) {
>> -            error_setg(&err, "invalid PCI subsystem vendor ID provided");
>> -            ret = -EINVAL;
>> +            error_setg(errp, "invalid PCI subsystem vendor ID provided");
>>              goto error;
>>          }
>>          vfio_add_emulated_word(vdev, PCI_SUBSYSTEM_VENDOR_ID,
>> @@ -2647,8 +2640,7 @@ static int vfio_initfn(PCIDevice *pdev)
>>  
>>      if (vdev->sub_device_id != PCI_ANY_ID) {
>>          if (vdev->sub_device_id > 0xffff) {
>> -            error_setg(&err, "invalid PCI subsystem device ID provided");
>> -            ret = -EINVAL;
>> +            error_setg(errp, "invalid PCI subsystem device ID provided");
>>              goto error;
>>          }
>>          vfio_add_emulated_word(vdev, PCI_SUBSYSTEM_ID, vdev->sub_device_id, ~0);
>> @@ -2702,10 +2694,9 @@ static int vfio_initfn(PCIDevice *pdev)
>>          struct vfio_region_info *opregion;
>>  
>>          if (vdev->pdev.qdev.hotplugged) {
>> -            error_setg(&err,
>> +            error_setg(errp,
>>                         "cannot support IGD OpRegion feature on hotplugged "
>>                         "device");
>> -            ret = -EINVAL;
>>              goto out_teardown;
>>          }
>>  
>> @@ -2713,7 +2704,7 @@ static int vfio_initfn(PCIDevice *pdev)
>>                          VFIO_REGION_TYPE_PCI_VENDOR_TYPE | PCI_VENDOR_ID_INTEL,
>>                          VFIO_REGION_SUBTYPE_INTEL_IGD_OPREGION, &opregion);
>>          if (ret) {
>> -            error_setg_errno(&err, -ret,
>> +            error_setg_errno(errp, -ret,
>>                               "does not support requested IGD OpRegion feature");
>>              goto out_teardown;
>>          }
>> @@ -2721,7 +2712,6 @@ static int vfio_initfn(PCIDevice *pdev)
>>          vfio_pci_igd_opregion_init(vdev, opregion, &err);
>>          g_free(opregion);
>>          if (err) {
>> -            ret = -EINVAL;
>>              goto out_teardown;
>>          }
>>      }
>> @@ -2743,6 +2733,7 @@ static int vfio_initfn(PCIDevice *pdev)
>>          pci_device_set_intx_routing_notifier(&vdev->pdev, vfio_intx_update);
>>          ret = vfio_intx_enable(vdev, &err);
>>          if (ret) {
>> +            error_setg_errno(errp, -ret, "failed to enable intx");
> 
> If vfio_intx_enable() sets an error on any failure (as it should), then
> the later error_propagate(errp, err) will simply free err.  Adding the
> error_setg_errno() here overrides the specific error set by
> vfio_intx_enable() with a generic one.  Looks wrong to me.
Yes this is definitively wrong!
> 
> If it doesn't, then the error_propagate() will work around that
> misbehavior.  You should fix vfio_intx_enable() then.
> 
>>              goto out_teardown;
>>          }
>>      }
>> @@ -2751,17 +2742,18 @@ static int vfio_initfn(PCIDevice *pdev)
>>      vfio_register_req_notifier(vdev);
>>      vfio_setup_resetfn_quirk(vdev);
>>  
>> -    return 0;
>> +    return;
>>  
>>  out_teardown:
>>      pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
>>      vfio_teardown_msi(vdev);
>>      vfio_bars_exit(vdev);
>>  error:
>> -    if (err) {
>> -        error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name);
>> +    error_propagate(errp, err);
>> +
>> +    if (*errp) {
>> +        error_prepend(errp, ERR_PREFIX, vdev->vbasedev.name);
>>      }
> 
> Dereferencing errp is unclean, because callers may pass null to ignore
> errors.  The qdev core doesn't.  However, error_prepend() does the right
> thing when passed null, namely nothing.  Simply call it unconditionally.
understood.

Thank you for your time!

Best Regards

Eric
> 
>> -    return ret;
>>  }
>>  
>>  static void vfio_instance_finalize(Object *obj)
>> @@ -2890,7 +2882,7 @@ static void vfio_pci_dev_class_init(ObjectClass *klass, void *data)
>>      dc->vmsd = &vfio_pci_vmstate;
>>      dc->desc = "VFIO-based PCI device assignment";
>>      set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>> -    pdc->init = vfio_initfn;
>> +    pdc->realize = vfio_realize;
>>      pdc->exit = vfio_exitfn;
>>      pdc->config_read = vfio_pci_read_config;
>>      pdc->config_write = vfio_pci_write_config;
>> diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
>> index da13322..ef81609 100644
>> --- a/hw/vfio/trace-events
>> +++ b/hw/vfio/trace-events
>> @@ -36,7 +36,7 @@ vfio_pci_hot_reset_dep_devices(int domain, int bus, int slot, int function, int
>>  vfio_pci_hot_reset_result(const char *name, const char *result) "%s hot reset: %s"
>>  vfio_populate_device_config(const char *name, unsigned long size, unsigned long offset, unsigned long flags) "Device %s config:\n  size: 0x%lx, offset: 0x%lx, flags: 0x%lx"
>>  vfio_populate_device_get_irq_info_failure(void) "VFIO_DEVICE_GET_IRQ_INFO failure: %m"
>> -vfio_initfn(const char *name, int group_id) " (%s) group %d"
>> +vfio_realize(const char *name, int group_id) " (%s) group %d"
>>  vfio_add_ext_cap_dropped(const char *name, uint16_t cap, uint16_t offset) "%s %x@%x"
>>  vfio_pci_reset(const char *name) " (%s)"
>>  vfio_pci_reset_flr(const char *name) "%s FLR/VFIO_DEVICE_RESET"
> 

[Qemu-devel] [PATCH v2 10/12] vfio/pci: Remove vfio_msix_early_setup returned value

[Eric Auger]

The returned value is not used anymore by the caller, vfio_realize
since the error now is stored in the error object. So let's remove it.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 hw/vfio/pci.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 857d89f..f8be00d 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1286,7 +1286,7 @@ static void vfio_pci_fixup_msix_region(VFIOPCIDevice *vdev)
  * need to first look for where the MSI-X table lives.  So we
  * unfortunately split MSI-X setup across two functions.
  */
-static int vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
+static void vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
 {
     uint8_t pos;
     uint16_t ctrl;
@@ -1296,25 +1296,25 @@ static int vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
 
     pos = pci_find_capability(&vdev->pdev, PCI_CAP_ID_MSIX);
     if (!pos) {
-        return 0;
+        return;
     }
 
     if (pread(fd, &ctrl, sizeof(ctrl),
               vdev->config_offset + pos + PCI_MSIX_FLAGS) != sizeof(ctrl)) {
         error_setg_errno(errp, errno, "failed to read PCI MSIX FLAGS");
-        return -errno;
+        return;
     }
 
     if (pread(fd, &table, sizeof(table),
               vdev->config_offset + pos + PCI_MSIX_TABLE) != sizeof(table)) {
         error_setg_errno(errp, errno, "failed to read PCI MSIX TABLE");
-        return -errno;
+        return;
     }
 
     if (pread(fd, &pba, sizeof(pba),
               vdev->config_offset + pos + PCI_MSIX_PBA) != sizeof(pba)) {
         error_setg_errno(errp, errno, "failed to read PCI MSIX PBA");
-        return -errno;
+        return;
     }
 
     ctrl = le16_to_cpu(ctrl);
@@ -1347,7 +1347,7 @@ static int vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
             error_setg(errp, "hardware reports invalid configuration, "
                        "MSIX PBA outside of specified BAR");
             g_free(msix);
-            return -EINVAL;
+            return;
         }
     }
 
@@ -1356,8 +1356,6 @@ static int vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
     vdev->msix = msix;
 
     vfio_pci_fixup_msix_region(vdev);
-
-    return 0;
 }
 
 static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
@@ -2669,7 +2667,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
 
     vfio_pci_size_rom(vdev);
 
-    ret = vfio_msix_early_setup(vdev, &err);
+    vfio_msix_early_setup(vdev, &err);
     if (err) {
         goto error;
     }
-- 
1.9.1

[Qemu-devel] [PATCH v2 11/12] vfio/pci: Remove vfio_populate_device returned value

[Eric Auger]

The returned value (either -errno or -1) is not used anymore by the caller,
vfio_realize, since the error now is stored in the error object. So let's
remove it.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 hw/vfio/pci.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index f8be00d..f5ff14b 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2221,7 +2221,7 @@ int vfio_populate_vga(VFIOPCIDevice *vdev)
     return 0;
 }
 
-static int vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
+static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
 {
     VFIODevice *vbasedev = &vdev->vbasedev;
     struct vfio_region_info *reg_info;
@@ -2231,18 +2231,18 @@ static int vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
     /* Sanity check device */
     if (!(vbasedev->flags & VFIO_DEVICE_FLAGS_PCI)) {
         error_setg(errp, "this isn't a PCI device");
-        goto error;
+        return;
     }
 
     if (vbasedev->num_regions < VFIO_PCI_CONFIG_REGION_INDEX + 1) {
         error_setg(errp, "unexpected number of io regions %u",
                    vbasedev->num_regions);
-        goto error;
+        return;
     }
 
     if (vbasedev->num_irqs < VFIO_PCI_MSIX_IRQ_INDEX + 1) {
         error_setg(errp, "unexpected number of irqs %u", vbasedev->num_irqs);
-        goto error;
+        return;
     }
 
     for (i = VFIO_PCI_BAR0_REGION_INDEX; i < VFIO_PCI_ROM_REGION_INDEX; i++) {
@@ -2254,7 +2254,7 @@ static int vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
 
         if (ret) {
             error_setg_errno(errp, -ret, "failed to get region %d info", i);
-            goto error;
+            return;
         }
 
         QLIST_INIT(&vdev->bars[i].quirks);
@@ -2264,7 +2264,7 @@ static int vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
                                VFIO_PCI_CONFIG_REGION_INDEX, &reg_info);
     if (ret) {
         error_setg_errno(errp, -ret, "failed to get config info");
-        goto error;
+        return;
     }
 
     trace_vfio_populate_device_config(vdev->vbasedev.name,
@@ -2285,7 +2285,7 @@ static int vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
         if (ret) {
             error_setg_errno(errp, -ret,
                 "device does not support requested feature x-vga");
-            goto error;
+            return;
         }
     }
 
@@ -2295,7 +2295,6 @@ static int vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
     if (ret) {
         /* This can fail for an old kernel or legacy PCI dev */
         trace_vfio_populate_device_get_irq_info_failure();
-        ret = 0;
     } else if (irq_info.count == 1) {
         vdev->pci_aer = true;
     } else {
@@ -2303,9 +2302,6 @@ static int vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
                      "Could not enable error recovery for the device",
                      vbasedev->name);
     }
-
-error:
-    return ret;
 }
 
 static void vfio_put_device(VFIOPCIDevice *vdev)
@@ -2577,7 +2573,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
         goto error;
     }
 
-    ret = vfio_populate_device(vdev, &err);
+    vfio_populate_device(vdev, &err);
     if (err) {
         goto error;
     }
-- 
1.9.1

[Qemu-devel] [PATCH v2 12/12] vfio/pci: Handle host oversight

[Eric Auger]

In case the end-user calls qemu with -vfio-pci option without passing
either sysfsdev or host property value, the device is interpreted as
0000:00:00.0. Let's create a specific error message to guide the end-user.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 hw/vfio/pci.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index f5ff14b..6a961bf 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2518,6 +2518,13 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
     int i, ret;
 
     if (!vdev->vbasedev.sysfsdev) {
+        if (!(~vdev->host.domain || ~vdev->host.bus ||
+              ~vdev->host.slot || ~vdev->host.function)) {
+            error_setg(errp, "No provided host device");
+            error_append_hint(errp, "Use -vfio-pci,host=DDDD:BB:DD.F "
+                              "or -vfio-pci,sysfsdev=PATH_TO_DEVICE\n");
+            return;
+        }
         vdev->vbasedev.sysfsdev =
             g_strdup_printf("/sys/bus/pci/devices/%04x:%02x:%02x.%01x",
                             vdev->host.domain, vdev->host.bus,
@@ -2829,6 +2836,10 @@ static void vfio_instance_init(Object *obj)
     device_add_bootindex_property(obj, &vdev->bootindex,
                                   "bootindex", NULL,
                                   &pci_dev->qdev, NULL);
+    vdev->host.domain = ~0U;
+    vdev->host.bus = ~0U;
+    vdev->host.slot = ~0U;
+    vdev->host.function = ~0U;
 }
 
 static Property vfio_pci_dev_properties[] = {
-- 
1.9.1

Re: [Qemu-devel] [PATCH v2 00/12] Convert VFIO-PCI to realize

[Markus Armbruster]

Eric Auger <eric.auger@redhat.com> writes:

> This series converts VFIO-PCI to realize. It also aims at improving
> the error reporting in case of QMP hot-plug.
>
> Before the series, a device_add failure would have reported:
> {"error": {"class": "GenericError", "desc": "Device initialization
> failed"}}.
>
> Now the actual error cause is reported.

Looks pretty nice now, but I think I found enough nits to warrant a
respin.