[Qemu-devel] [PATCH v4 00/17] Convert VFIO-PCI to realize

[Qemu-devel] [PATCH v4 00/17] 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.

A side effect of this series is the cleanup of the error handling
in vfio-platform.

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

History:
v3 -> v4:
- added vfio: Pass an Error object to vfio_connect_container

v2 -> v3:
- Addressed Markus' comments :
  - don't use local error object if not necessary
  - fix some error handling: see individual patches
  - use error objects also on vfio platform realize path although
    hot-plug is not allowed, but that's cleaner I think.
  - bug fix in vfio platform

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 (17):
  vfio/pci: Use local error object in vfio_initfn
  vfio/pci: Pass an error object to vfio_populate_vga
  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_connect_container
  vfio: Pass an error object to vfio_get_group
  vfio: Pass an error object to vfio_get_device
  vfio/platform: Pass an error object to vfio_populate_device
  vfio/platform: fix a wrong returned value in vfio_populate_device
  vfio/platform: Pass an error object to vfio_base_device_init
  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              |  68 ++++++-----
 hw/vfio/pci-quirks.c          |  14 ++-
 hw/vfio/pci.c                 | 271 +++++++++++++++++++++++-------------------
 hw/vfio/pci.h                 |   5 +-
 hw/vfio/platform.c            |  65 +++++-----
 hw/vfio/trace-events          |   2 +-
 include/hw/vfio/vfio-common.h |   7 +-
 7 files changed, 244 insertions(+), 188 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH v4 01/17] 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 -> v3:
- use error_setg_errno on "no such host device" case
- on "no iommu_group found", use previously computed ret value

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

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index a5a620a..417bf7f 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_errno(&err, errno, "no such host device");
+        ret = -errno;
+        goto error;
     }
 
     vdev->vbasedev.name = g_strdup(basename(vdev->vbasedev.sysfsdev));
@@ -2520,40 +2521,43 @@ 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;
+        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 +2571,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 +2588,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 +2600,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 +2612,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 +2624,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 +2679,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 +2690,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 +2734,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 c17602e..b26b6cf 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

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

[Eric Auger]

Pass an error object to prepare for the same operation in
vfio_populate_device. Eventually this contributes to the migration
to VFIO-PCI realize.

We now report an error on vfio_get_region_info failure.

vfio_probe_igd_bar4_quirk is not involved in the migration to realize
and simply calls error_reportf_err.

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

---

v3: creation
---
 hw/vfio/pci-quirks.c |  4 +++-
 hw/vfio/pci.c        | 19 ++++++++++++-------
 hw/vfio/pci.h        |  2 +-
 3 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
index bec694c..806ea5d 100644
--- a/hw/vfio/pci-quirks.c
+++ b/hw/vfio/pci-quirks.c
@@ -1363,6 +1363,7 @@ static void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
     uint64_t *bdsm_size;
     uint32_t gmch;
     uint16_t cmd_orig, cmd;
+    Error *err = NULL;
 
     /*
      * This must be an Intel VGA device at address 00:02.0 for us to even
@@ -1464,7 +1465,8 @@ static void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
      * try to enable it.  Probably shouldn't be using legacy mode without VGA,
      * but also no point in us enabling VGA if disabled in hardware.
      */
-    if (!(gmch & 0x2) && !vdev->vga && vfio_populate_vga(vdev)) {
+    if (!(gmch & 0x2) && !vdev->vga && vfio_populate_vga(vdev, &err)) {
+        error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name);
         error_report("IGD device %s failed to enable VGA access, "
                      "legacy mode disabled", vdev->vbasedev.name);
         goto out;
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 417bf7f..db08dd3 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2134,7 +2134,7 @@ static VFIODeviceOps vfio_pci_ops = {
     .vfio_eoi = vfio_intx_eoi,
 };
 
-int vfio_populate_vga(VFIOPCIDevice *vdev)
+int vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp)
 {
     VFIODevice *vbasedev = &vdev->vbasedev;
     struct vfio_region_info *reg_info;
@@ -2142,15 +2142,18 @@ int vfio_populate_vga(VFIOPCIDevice *vdev)
 
     ret = vfio_get_region_info(vbasedev, VFIO_PCI_VGA_REGION_INDEX, &reg_info);
     if (ret) {
+        error_setg_errno(errp, -ret,
+                         "failed getting region info for VGA region index %d",
+                         VFIO_PCI_VGA_REGION_INDEX);
         return ret;
     }
 
     if (!(reg_info->flags & VFIO_REGION_INFO_FLAG_READ) ||
         !(reg_info->flags & VFIO_REGION_INFO_FLAG_WRITE) ||
         reg_info->size < 0xbffff + 1) {
-        error_report("vfio: Unexpected VGA info, flags 0x%lx, size 0x%lx",
-                     (unsigned long)reg_info->flags,
-                     (unsigned long)reg_info->size);
+        error_setg(errp, "unexpected VGA info, flags 0x%lx, size 0x%lx",
+                   (unsigned long)reg_info->flags,
+                   (unsigned long)reg_info->size);
         g_free(reg_info);
         return -EINVAL;
     }
@@ -2205,6 +2208,7 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
     struct vfio_region_info *reg_info;
     struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info) };
     int i, ret = -1;
+    Error *err = NULL;
 
     /* Sanity check device */
     if (!(vbasedev->flags & VFIO_DEVICE_FLAGS_PCI)) {
@@ -2259,10 +2263,11 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
     g_free(reg_info);
 
     if (vdev->features & VFIO_FEATURE_ENABLE_VGA) {
-        ret = vfio_populate_vga(vdev);
+        ret = vfio_populate_vga(vdev, &err);
         if (ret) {
-            error_report(
-                "vfio: Device does not support requested feature x-vga");
+            error_append_hint(&err, "device does not support "
+                              "requested feature x-vga");
+            error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name);
             goto error;
         }
     }
diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
index 7d482d9..87a62f9 100644
--- a/hw/vfio/pci.h
+++ b/hw/vfio/pci.h
@@ -161,7 +161,7 @@ void vfio_bar_quirk_exit(VFIOPCIDevice *vdev, int nr);
 void vfio_bar_quirk_finalize(VFIOPCIDevice *vdev, int nr);
 void vfio_setup_resetfn_quirk(VFIOPCIDevice *vdev);
 
-int vfio_populate_vga(VFIOPCIDevice *vdev);
+int vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp);
 
 int vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev,
                                struct vfio_region_info *info);
-- 
1.9.1

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

[Markus Armbruster]

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

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

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

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

[Qemu-devel] [PATCH v4 03/17] 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>

---
v2 -> v3:
- propagate the error from vfio_populate_vga

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 | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index db08dd3..8f825bd 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2202,28 +2202,27 @@ int vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp)
     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;
     struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info) };
     int i, ret = -1;
-    Error *err = NULL;
 
     /* Sanity check device */
     if (!(vbasedev->flags & VFIO_DEVICE_FLAGS_PCI)) {
-        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;
     }
 
@@ -2235,7 +2234,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;
         }
 
@@ -2245,7 +2244,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;
     }
 
@@ -2263,11 +2262,10 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
     g_free(reg_info);
 
     if (vdev->features & VFIO_FEATURE_ENABLE_VGA) {
-        ret = vfio_populate_vga(vdev, &err);
+        ret = vfio_populate_vga(vdev, errp);
         if (ret) {
-            error_append_hint(&err, "device does not support "
-                              "requested feature x-vga");
-            error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name);
+            error_append_hint(errp, "device does not support "
+                              "requested feature x-vga\n");
             goto error;
         }
     }
@@ -2282,7 +2280,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);
     }
@@ -2565,9 +2563,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

[Qemu-devel] [PATCH v4 04/17] 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 8f825bd..d1967a2 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;
         }
@@ -2657,9 +2660,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 v4 05/17] 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 down to vfio_intx_enable_kvm().

The three other callers, vfio_intx_enable_kvm(), 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 -> V3:
- reword commit message according to Markus' suggestion
- fix a regression introduced by considering vfio_intx_enable_kvm
  failure as an error instead of as a warning.

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

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index d1967a2..99e1938 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, WARN_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,16 @@ 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);
+    if (err) {
+        error_reportf_err(err, WARN_PREFIX, vdev->vbasedev.name);
+    }
 
     vdev->interrupt = VFIO_INT_INTx;
 
@@ -707,6 +715,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 +735,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 +1920,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)
@@ -2724,7 +2741,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

[Qemu-devel] [PATCH v4 06/17] 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>

---

v2 -> v3:
- set an error on vfio_msi_setup/PCI_CAP_FLAGS pread failure
- directly use errp in vfio_add_std_cap
- in vfio_add_capabilities, simply pass errp down to vfio_add_std_cap
---
 hw/vfio/pci.c | 59 +++++++++++++++++++++++++++++++----------------------------
 1 file changed, 31 insertions(+), 28 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 99e1938..7436248 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1180,7 +1180,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;
@@ -1189,6 +1189,7 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos)
 
     if (pread(vdev->vbasedev.fd, &ctrl, sizeof(ctrl),
               vdev->config_offset + pos + PCI_CAP_FLAGS) != sizeof(ctrl)) {
+        error_setg_errno(errp, errno, "failed reading PCI_CAP_FLAGS");
         return -errno;
     }
     ctrl = le16_to_cpu(ctrl);
@@ -1204,8 +1205,8 @@ 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_prepend(&err, "msi_init failed: ");
+        error_propagate(errp, err);
         return ret;
     }
     vdev->msi_cap_size = 0xa + (msi_maskbit ? 0xa : 0) + (msi_64bit ? 0x4 : 0);
@@ -1363,7 +1364,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;
 
@@ -1378,7 +1379,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;
     }
 
@@ -1563,7 +1564,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;
@@ -1575,8 +1577,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;
     }
 
@@ -1710,7 +1712,7 @@ 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;
@@ -1735,9 +1737,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, errp);
         if (ret) {
-            return ret;
+            goto out;
         }
     } else {
         /* Begin the rebuild, use QEMU emulated list bits */
@@ -1751,40 +1753,40 @@ 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, errp);
         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, errp);
         break;
     case PCI_CAP_ID_MSIX:
-        ret = vfio_msix_setup(vdev, pos);
+        ret = vfio_msix_setup(vdev, pos, errp);
         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, errp);
         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, errp);
         break;
     default:
-        ret = pci_add_capability(pdev, cap_id, pos, size);
+        ret = pci_add_capability2(pdev, cap_id, pos, size, errp);
         break;
     }
-
+out:
     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);
+        error_prepend(errp,
+                      "failed to add PCI capability 0x%x[0x%x]@0x%x: ",
+                      cap_id, size, pos);
         return ret;
     }
 
     return 0;
 }
 
-static int vfio_add_ext_cap(VFIOPCIDevice *vdev)
+static void vfio_add_ext_cap(VFIOPCIDevice *vdev)
 {
     PCIDevice *pdev = &vdev->pdev;
     uint32_t header;
@@ -1795,7 +1797,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;
     }
 
     /*
@@ -1860,10 +1862,10 @@ 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;
     int ret;
@@ -1873,12 +1875,13 @@ 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], 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)
@@ -2684,7 +2687,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

[Qemu-devel] [PATCH v4 07/17] 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.

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

diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
index 806ea5d..2cbda08 100644
--- a/hw/vfio/pci-quirks.c
+++ b/hw/vfio/pci-quirks.c
@@ -1056,7 +1056,7 @@ typedef struct VFIOIGDQuirk {
  * of the IGD device.
  */
 int vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev,
-                               struct vfio_region_info *info)
+                               struct vfio_region_info *info, Error **errp)
 {
     int ret;
 
@@ -1064,7 +1064,7 @@ 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;
@@ -1489,10 +1489,10 @@ static void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
     }
 
     /* Setup OpRegion access */
-    ret = vfio_pci_igd_opregion_init(vdev, opregion);
+    ret = vfio_pci_igd_opregion_init(vdev, opregion, &err);
     if (ret) {
-        error_report("IGD device %s failed to setup OpRegion, "
-                     "legacy mode disabled", vdev->vbasedev.name);
+        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 7436248..3757bc3 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2721,10 +2721,9 @@ static int vfio_initfn(PCIDevice *pdev)
             goto out_teardown;
         }
 
-        ret = vfio_pci_igd_opregion_init(vdev, opregion);
+        ret = vfio_pci_igd_opregion_init(vdev, opregion, &err);
         g_free(opregion);
         if (ret) {
-            error_setg_errno(&err, -ret, "IGD OpRegion initialization failed");
             goto out_teardown;
         }
     }
diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
index 87a62f9..198adac 100644
--- a/hw/vfio/pci.h
+++ b/hw/vfio/pci.h
@@ -164,6 +164,7 @@ void vfio_setup_resetfn_quirk(VFIOPCIDevice *vdev);
 int vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp);
 
 int vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev,
-                               struct vfio_region_info *info);
+                                struct vfio_region_info *info,
+                                Error **errp);
 
 #endif /* HW_VFIO_VFIO_PCI_H */
-- 
1.9.1

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

[Markus Armbruster]

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

> Pass an error object to prepare for migration to VFIO-PCI realize.
>
> In vfio_probe_igd_bar4_quirk, simply report the error.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  hw/vfio/pci-quirks.c | 10 +++++-----
>  hw/vfio/pci.c        |  3 +--
>  hw/vfio/pci.h        |  3 ++-
>  3 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
> index 806ea5d..2cbda08 100644
> --- a/hw/vfio/pci-quirks.c
> +++ b/hw/vfio/pci-quirks.c
> @@ -1056,7 +1056,7 @@ typedef struct VFIOIGDQuirk {
>   * of the IGD device.
>   */
>  int vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev,
> -                               struct vfio_region_info *info)
> +                               struct vfio_region_info *info, Error **errp)
>  {
>      int ret;
>  
> @@ -1064,7 +1064,7 @@ 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;
> @@ -1489,10 +1489,10 @@ static void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
>      }
>  
>      /* Setup OpRegion access */
> -    ret = vfio_pci_igd_opregion_init(vdev, opregion);
> +    ret = vfio_pci_igd_opregion_init(vdev, opregion, &err);
>      if (ret) {
> -        error_report("IGD device %s failed to setup OpRegion, "
> -                     "legacy mode disabled", vdev->vbasedev.name);
> +        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 7436248..3757bc3 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2721,10 +2721,9 @@ static int vfio_initfn(PCIDevice *pdev)
>              goto out_teardown;
>          }
>  
> -        ret = vfio_pci_igd_opregion_init(vdev, opregion);
> +        ret = vfio_pci_igd_opregion_init(vdev, opregion, &err);
>          g_free(opregion);
>          if (ret) {
> -            error_setg_errno(&err, -ret, "IGD OpRegion initialization failed");
>              goto out_teardown;
>          }
>      }
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index 87a62f9..198adac 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -164,6 +164,7 @@ void vfio_setup_resetfn_quirk(VFIOPCIDevice *vdev);
>  int vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp);
>  
>  int vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev,
> -                               struct vfio_region_info *info);
> +                                struct vfio_region_info *info,
> +                                Error **errp);
>  
>  #endif /* HW_VFIO_VFIO_PCI_H */

Indentation is off by one.  Could perhaps be touched up on commit.

[Qemu-devel] [PATCH v4 08/17] vfio: Pass an Error object to vfio_connect_container

[Eric Auger]

The error is currently simply reported in vfio_get_group. Don't
bother too much with the prefix which will be handled at upper level,
later on.

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

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 29188a1..3b18eb4 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);
@@ -900,7 +901,8 @@ static void vfio_put_address_space(VFIOAddressSpace *space)
     }
 }
 
-static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
+static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
+                                  Error **errp)
 {
     VFIOContainer *container;
     int ret, fd;
@@ -918,15 +920,15 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
 
     fd = qemu_open("/dev/vfio/vfio", O_RDWR);
     if (fd < 0) {
-        error_report("vfio: failed to open /dev/vfio/vfio: %m");
+        error_setg_errno(errp, errno, "failed to open /dev/vfio/vfio");
         ret = -errno;
         goto put_space_exit;
     }
 
     ret = ioctl(fd, VFIO_GET_API_VERSION);
     if (ret != VFIO_API_VERSION) {
-        error_report("vfio: supported vfio version: %d, "
-                     "reported version: %d", VFIO_API_VERSION, ret);
+        error_setg(errp, "supported vfio version: %d, "
+                   "reported version: %d", VFIO_API_VERSION, ret);
         ret = -EINVAL;
         goto close_fd_exit;
     }
@@ -941,7 +943,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
 
         ret = ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &fd);
         if (ret) {
-            error_report("vfio: failed to set group container: %m");
+            error_setg_errno(errp, errno, "failed to set group container");
             ret = -errno;
             goto free_container_exit;
         }
@@ -949,7 +951,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
         container->iommu_type = v2 ? VFIO_TYPE1v2_IOMMU : VFIO_TYPE1_IOMMU;
         ret = ioctl(fd, VFIO_SET_IOMMU, container->iommu_type);
         if (ret) {
-            error_report("vfio: failed to set iommu for container: %m");
+            error_setg_errno(errp, errno, "failed to set iommu for container");
             ret = -errno;
             goto free_container_exit;
         }
@@ -976,7 +978,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
 
         ret = ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &fd);
         if (ret) {
-            error_report("vfio: failed to set group container: %m");
+            error_setg_errno(errp, errno, "failed to set group container");
             ret = -errno;
             goto free_container_exit;
         }
@@ -984,7 +986,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
             v2 ? VFIO_SPAPR_TCE_v2_IOMMU : VFIO_SPAPR_TCE_IOMMU;
         ret = ioctl(fd, VFIO_SET_IOMMU, container->iommu_type);
         if (ret) {
-            error_report("vfio: failed to set iommu for container: %m");
+            error_setg_errno(errp, errno, "failed to set iommu for container");
             ret = -errno;
             goto free_container_exit;
         }
@@ -997,7 +999,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
         if (!v2) {
             ret = ioctl(fd, VFIO_IOMMU_ENABLE);
             if (ret) {
-                error_report("vfio: failed to enable container: %m");
+                error_setg_errno(errp, errno, "failed to enable container");
                 ret = -errno;
                 goto free_container_exit;
             }
@@ -1008,7 +1010,8 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
                                      &address_space_memory);
             if (container->error) {
                 memory_listener_unregister(&container->prereg_listener);
-                error_report("vfio: RAM memory listener initialization failed for container");
+                error_setg(errp,
+                    "RAM memory listener initialization failed for container");
                 goto free_container_exit;
             }
         }
@@ -1016,7 +1019,8 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
         info.argsz = sizeof(info);
         ret = ioctl(fd, VFIO_IOMMU_SPAPR_TCE_GET_INFO, &info);
         if (ret) {
-            error_report("vfio: VFIO_IOMMU_SPAPR_TCE_GET_INFO failed: %m");
+            error_setg_errno(errp, errno,
+                             "VFIO_IOMMU_SPAPR_TCE_GET_INFO failed");
             ret = -errno;
             if (v2) {
                 memory_listener_unregister(&container->prereg_listener);
@@ -1033,6 +1037,8 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
              */
             ret = vfio_spapr_remove_window(container, info.dma32_window_start);
             if (ret) {
+                error_setg_errno(errp, -ret,
+                                 "failed to remove existing window");
                 goto free_container_exit;
             }
         } else {
@@ -1043,7 +1049,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
                               0x1000);
         }
     } else {
-        error_report("vfio: No available IOMMU models");
+        error_setg(errp, "No available IOMMU models");
         ret = -EINVAL;
         goto free_container_exit;
     }
@@ -1054,7 +1060,8 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
 
     if (container->error) {
         ret = container->error;
-        error_report("vfio: memory listener initialization failed for container");
+        error_setg_errno(errp, -ret,
+                         "memory listener initialization failed for container");
         goto listener_release_exit;
     }
 
@@ -1120,6 +1127,7 @@ VFIOGroup *vfio_get_group(int groupid, AddressSpace *as)
     VFIOGroup *group;
     char path[32];
     struct vfio_group_status status = { .argsz = sizeof(status) };
+    Error *err = NULL;
 
     QLIST_FOREACH(group, &vfio_group_list, next) {
         if (group->groupid == groupid) {
@@ -1158,8 +1166,9 @@ VFIOGroup *vfio_get_group(int groupid, AddressSpace *as)
     group->groupid = groupid;
     QLIST_INIT(&group->device_list);
 
-    if (vfio_connect_container(group, as)) {
-        error_report("vfio: failed to setup container for group %d", groupid);
+    if (vfio_connect_container(group, as, &err)) {
+        error_reportf_err(err, "vfio: failed to setup container for group %d",
+                          groupid);
         goto close_fd_exit;
     }
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH v4 08/17] vfio: Pass an Error object to vfio_connect_container

[Markus Armbruster]

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

> The error is currently simply reported in vfio_get_group. Don't
> bother too much with the prefix which will be handled at upper level,
> later on.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  hw/vfio/common.c | 39 ++++++++++++++++++++++++---------------
>  1 file changed, 24 insertions(+), 15 deletions(-)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 29188a1..3b18eb4 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);
> @@ -900,7 +901,8 @@ static void vfio_put_address_space(VFIOAddressSpace *space)
>      }
>  }
>  
> -static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
> +static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
> +                                  Error **errp)
>  {
>      VFIOContainer *container;
>      int ret, fd;
> @@ -918,15 +920,15 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
>  
>      fd = qemu_open("/dev/vfio/vfio", O_RDWR);
>      if (fd < 0) {
> -        error_report("vfio: failed to open /dev/vfio/vfio: %m");
> +        error_setg_errno(errp, errno, "failed to open /dev/vfio/vfio");
>          ret = -errno;
>          goto put_space_exit;
>      }
>  
>      ret = ioctl(fd, VFIO_GET_API_VERSION);
>      if (ret != VFIO_API_VERSION) {
> -        error_report("vfio: supported vfio version: %d, "
> -                     "reported version: %d", VFIO_API_VERSION, ret);
> +        error_setg(errp, "supported vfio version: %d, "
> +                   "reported version: %d", VFIO_API_VERSION, ret);
>          ret = -EINVAL;
>          goto close_fd_exit;
>      }
> @@ -941,7 +943,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
>  
>          ret = ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &fd);
>          if (ret) {
> -            error_report("vfio: failed to set group container: %m");
> +            error_setg_errno(errp, errno, "failed to set group container");
>              ret = -errno;
>              goto free_container_exit;
>          }
> @@ -949,7 +951,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
>          container->iommu_type = v2 ? VFIO_TYPE1v2_IOMMU : VFIO_TYPE1_IOMMU;
>          ret = ioctl(fd, VFIO_SET_IOMMU, container->iommu_type);
>          if (ret) {
> -            error_report("vfio: failed to set iommu for container: %m");
> +            error_setg_errno(errp, errno, "failed to set iommu for container");
>              ret = -errno;
>              goto free_container_exit;
>          }
> @@ -976,7 +978,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
>  
>          ret = ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &fd);
>          if (ret) {
> -            error_report("vfio: failed to set group container: %m");
> +            error_setg_errno(errp, errno, "failed to set group container");
>              ret = -errno;
>              goto free_container_exit;
>          }
> @@ -984,7 +986,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
>              v2 ? VFIO_SPAPR_TCE_v2_IOMMU : VFIO_SPAPR_TCE_IOMMU;
>          ret = ioctl(fd, VFIO_SET_IOMMU, container->iommu_type);
>          if (ret) {
> -            error_report("vfio: failed to set iommu for container: %m");
> +            error_setg_errno(errp, errno, "failed to set iommu for container");
>              ret = -errno;
>              goto free_container_exit;
>          }
> @@ -997,7 +999,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
>          if (!v2) {
>              ret = ioctl(fd, VFIO_IOMMU_ENABLE);
>              if (ret) {
> -                error_report("vfio: failed to enable container: %m");
> +                error_setg_errno(errp, errno, "failed to enable container");
>                  ret = -errno;
>                  goto free_container_exit;
>              }
> @@ -1008,7 +1010,8 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
>                                       &address_space_memory);
>              if (container->error) {
>                  memory_listener_unregister(&container->prereg_listener);
> -                error_report("vfio: RAM memory listener initialization failed for container");
> +                error_setg(errp,
> +                    "RAM memory listener initialization failed for container");
>                  goto free_container_exit;

Preexisting: @ret not set here.  Intentional?

>              }
>          }
> @@ -1016,7 +1019,8 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
>          info.argsz = sizeof(info);
>          ret = ioctl(fd, VFIO_IOMMU_SPAPR_TCE_GET_INFO, &info);
>          if (ret) {
> -            error_report("vfio: VFIO_IOMMU_SPAPR_TCE_GET_INFO failed: %m");
> +            error_setg_errno(errp, errno,
> +                             "VFIO_IOMMU_SPAPR_TCE_GET_INFO failed");
>              ret = -errno;
>              if (v2) {
>                  memory_listener_unregister(&container->prereg_listener);
> @@ -1033,6 +1037,8 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
>               */
>              ret = vfio_spapr_remove_window(container, info.dma32_window_start);
>              if (ret) {
> +                error_setg_errno(errp, -ret,
> +                                 "failed to remove existing window");
>                  goto free_container_exit;

Error is no longer silent.  Worth mentioning in the commit message?

>              }
>          } else {
> @@ -1043,7 +1049,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
>                                0x1000);
>          }
>      } else {
> -        error_report("vfio: No available IOMMU models");
> +        error_setg(errp, "No available IOMMU models");
>          ret = -EINVAL;
>          goto free_container_exit;
>      }
> @@ -1054,7 +1060,8 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
>  
>      if (container->error) {
>          ret = container->error;
> -        error_report("vfio: memory listener initialization failed for container");
> +        error_setg_errno(errp, -ret,
> +                         "memory listener initialization failed for container");
>          goto listener_release_exit;
>      }
>  
> @@ -1120,6 +1127,7 @@ VFIOGroup *vfio_get_group(int groupid, AddressSpace *as)
>      VFIOGroup *group;
>      char path[32];
>      struct vfio_group_status status = { .argsz = sizeof(status) };
> +    Error *err = NULL;
>  
>      QLIST_FOREACH(group, &vfio_group_list, next) {
>          if (group->groupid == groupid) {
> @@ -1158,8 +1166,9 @@ VFIOGroup *vfio_get_group(int groupid, AddressSpace *as)
>      group->groupid = groupid;
>      QLIST_INIT(&group->device_list);
>  
> -    if (vfio_connect_container(group, as)) {
> -        error_report("vfio: failed to setup container for group %d", groupid);
> +    if (vfio_connect_container(group, as, &err)) {
> +        error_reportf_err(err, "vfio: failed to setup container for group %d",
> +                          groupid);
>          goto close_fd_exit;
>      }

Re: [Qemu-devel] [PATCH v4 08/17] vfio: Pass an Error object to vfio_connect_container

[Auger Eric]

Hi Markus,

On 04/10/2016 14:03, Markus Armbruster wrote:
> Eric Auger <eric.auger@redhat.com> writes:
> 
>> The error is currently simply reported in vfio_get_group. Don't
>> bother too much with the prefix which will be handled at upper level,
>> later on.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  hw/vfio/common.c | 39 ++++++++++++++++++++++++---------------
>>  1 file changed, 24 insertions(+), 15 deletions(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 29188a1..3b18eb4 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);
>> @@ -900,7 +901,8 @@ static void vfio_put_address_space(VFIOAddressSpace *space)
>>      }
>>  }
>>  
>> -static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
>> +static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>> +                                  Error **errp)
>>  {
>>      VFIOContainer *container;
>>      int ret, fd;
>> @@ -918,15 +920,15 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
>>  
>>      fd = qemu_open("/dev/vfio/vfio", O_RDWR);
>>      if (fd < 0) {
>> -        error_report("vfio: failed to open /dev/vfio/vfio: %m");
>> +        error_setg_errno(errp, errno, "failed to open /dev/vfio/vfio");
>>          ret = -errno;
>>          goto put_space_exit;
>>      }
>>  
>>      ret = ioctl(fd, VFIO_GET_API_VERSION);
>>      if (ret != VFIO_API_VERSION) {
>> -        error_report("vfio: supported vfio version: %d, "
>> -                     "reported version: %d", VFIO_API_VERSION, ret);
>> +        error_setg(errp, "supported vfio version: %d, "
>> +                   "reported version: %d", VFIO_API_VERSION, ret);
>>          ret = -EINVAL;
>>          goto close_fd_exit;
>>      }
>> @@ -941,7 +943,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
>>  
>>          ret = ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &fd);
>>          if (ret) {
>> -            error_report("vfio: failed to set group container: %m");
>> +            error_setg_errno(errp, errno, "failed to set group container");
>>              ret = -errno;
>>              goto free_container_exit;
>>          }
>> @@ -949,7 +951,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
>>          container->iommu_type = v2 ? VFIO_TYPE1v2_IOMMU : VFIO_TYPE1_IOMMU;
>>          ret = ioctl(fd, VFIO_SET_IOMMU, container->iommu_type);
>>          if (ret) {
>> -            error_report("vfio: failed to set iommu for container: %m");
>> +            error_setg_errno(errp, errno, "failed to set iommu for container");
>>              ret = -errno;
>>              goto free_container_exit;
>>          }
>> @@ -976,7 +978,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
>>  
>>          ret = ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &fd);
>>          if (ret) {
>> -            error_report("vfio: failed to set group container: %m");
>> +            error_setg_errno(errp, errno, "failed to set group container");
>>              ret = -errno;
>>              goto free_container_exit;
>>          }
>> @@ -984,7 +986,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
>>              v2 ? VFIO_SPAPR_TCE_v2_IOMMU : VFIO_SPAPR_TCE_IOMMU;
>>          ret = ioctl(fd, VFIO_SET_IOMMU, container->iommu_type);
>>          if (ret) {
>> -            error_report("vfio: failed to set iommu for container: %m");
>> +            error_setg_errno(errp, errno, "failed to set iommu for container");
>>              ret = -errno;
>>              goto free_container_exit;
>>          }
>> @@ -997,7 +999,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
>>          if (!v2) {
>>              ret = ioctl(fd, VFIO_IOMMU_ENABLE);
>>              if (ret) {
>> -                error_report("vfio: failed to enable container: %m");
>> +                error_setg_errno(errp, errno, "failed to enable container");
>>                  ret = -errno;
>>                  goto free_container_exit;
>>              }
>> @@ -1008,7 +1010,8 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
>>                                       &address_space_memory);
>>              if (container->error) {
>>                  memory_listener_unregister(&container->prereg_listener);
>> -                error_report("vfio: RAM memory listener initialization failed for container");
>> +                error_setg(errp,
>> +                    "RAM memory listener initialization failed for container");
>>                  goto free_container_exit;
> 
> Preexisting: @ret not set here.  Intentional?
It was not before either. I suspect it is a bug since the container is
teared down.

I set ret to container->error.
> 
>>              }
>>          }
>> @@ -1016,7 +1019,8 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
>>          info.argsz = sizeof(info);
>>          ret = ioctl(fd, VFIO_IOMMU_SPAPR_TCE_GET_INFO, &info);
>>          if (ret) {
>> -            error_report("vfio: VFIO_IOMMU_SPAPR_TCE_GET_INFO failed: %m");
>> +            error_setg_errno(errp, errno,
>> +                             "VFIO_IOMMU_SPAPR_TCE_GET_INFO failed");
>>              ret = -errno;
>>              if (v2) {
>>                  memory_listener_unregister(&container->prereg_listener);
>> @@ -1033,6 +1037,8 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
>>               */
>>              ret = vfio_spapr_remove_window(container, info.dma32_window_start);
>>              if (ret) {
>> +                error_setg_errno(errp, -ret,
>> +                                 "failed to remove existing window");
>>                  goto free_container_exit;
> 
> Error is no longer silent.  Worth mentioning in the commit message?
Sure

Thanks

Eric
> 
>>              }
>>          } else {
>> @@ -1043,7 +1049,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
>>                                0x1000);
>>          }
>>      } else {
>> -        error_report("vfio: No available IOMMU models");
>> +        error_setg(errp, "No available IOMMU models");
>>          ret = -EINVAL;
>>          goto free_container_exit;
>>      }
>> @@ -1054,7 +1060,8 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
>>  
>>      if (container->error) {
>>          ret = container->error;
>> -        error_report("vfio: memory listener initialization failed for container");
>> +        error_setg_errno(errp, -ret,
>> +                         "memory listener initialization failed for container");
>>          goto listener_release_exit;
>>      }
>>  
>> @@ -1120,6 +1127,7 @@ VFIOGroup *vfio_get_group(int groupid, AddressSpace *as)
>>      VFIOGroup *group;
>>      char path[32];
>>      struct vfio_group_status status = { .argsz = sizeof(status) };
>> +    Error *err = NULL;
>>  
>>      QLIST_FOREACH(group, &vfio_group_list, next) {
>>          if (group->groupid == groupid) {
>> @@ -1158,8 +1166,9 @@ VFIOGroup *vfio_get_group(int groupid, AddressSpace *as)
>>      group->groupid = groupid;
>>      QLIST_INIT(&group->device_list);
>>  
>> -    if (vfio_connect_container(group, as)) {
>> -        error_report("vfio: failed to setup container for group %d", groupid);
>> +    if (vfio_connect_container(group, as, &err)) {
>> +        error_reportf_err(err, "vfio: failed to setup container for group %d",
>> +                          groupid);
>>          goto close_fd_exit;
>>      }
> 

[Qemu-devel] [PATCH v4 09/17] vfio: Pass an error object to vfio_get_group

[Eric Auger]

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

For the time being let's just simply report the error in
vfio platform's vfio_base_device_init(). A subsequent patch will
duly propagate the error up to vfio_platform_realize.

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

---
v3 -> v4:
- vfio_connect_container now is passed an error object

v2 -> v3:
- In the commit message, add a comment about not propagating the
  vfio_get_group in vfio_base_device_init

v2: creation
---
 hw/vfio/common.c              | 24 ++++++++++++------------
 hw/vfio/pci.c                 |  3 +--
 hw/vfio/platform.c            | 11 ++++++++---
 include/hw/vfio/vfio-common.h |  2 +-
 4 files changed, 22 insertions(+), 18 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 3b18eb4..4749384 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1122,12 +1122,11 @@ 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];
     struct vfio_group_status status = { .argsz = sizeof(status) };
-    Error *err = NULL;
 
     QLIST_FOREACH(group, &vfio_group_list, next) {
         if (group->groupid == groupid) {
@@ -1135,8 +1134,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;
             }
         }
@@ -1147,28 +1146,29 @@ 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, "failed to open %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, "failed to get 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;
     }
 
     group->groupid = groupid;
     QLIST_INIT(&group->device_list);
 
-    if (vfio_connect_container(group, as, &err)) {
-        error_reportf_err(err, "vfio: failed to setup container for group %d",
-                          groupid);
+    if (vfio_connect_container(group, as, errp)) {
+        error_prepend(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 3757bc3..910c371 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2563,9 +2563,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 b26b6cf..286fa31 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

[Qemu-devel] [PATCH v4 10/17] vfio: Pass an error object to vfio_get_device

[Eric Auger]

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

In vfio platform vfio_base_device_init we currently just report the
error. Subsequent patches will propagate the error up to the realize
function.

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 4749384..1757853 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1210,23 +1210,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 910c371..40ff4a7 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2578,9 +2578,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 286fa31..c582de1 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 v4 11/17] vfio/platform: Pass an error object to vfio_populate_device

[Eric Auger]

Propagate the vfio_populate_device errors up to vfio_base_device_init.
The error object also is passed to vfio_init_intp. At the moment we
only report the error. Subsequent patches will propagate the error
up to the realize function.

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

diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
index 9014ea7..1a35da0 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -44,9 +44,10 @@ static inline bool vfio_irq_is_automasked(VFIOINTp *intp)
  * and add it into the list of IRQs
  * @vbasedev: the VFIO device handle
  * @info: irq info struct retrieved from VFIO driver
+ * @errp: error object
  */
 static VFIOINTp *vfio_init_intp(VFIODevice *vbasedev,
-                                struct vfio_irq_info info)
+                                struct vfio_irq_info info, Error **errp)
 {
     int ret;
     VFIOPlatformDevice *vdev =
@@ -69,7 +70,8 @@ static VFIOINTp *vfio_init_intp(VFIODevice *vbasedev,
     if (ret) {
         g_free(intp->interrupt);
         g_free(intp);
-        error_report("vfio: Error: trigger event_notifier_init failed ");
+        error_setg_errno(errp, -ret,
+                         "failed to initialize trigger eventd notifier");
         return NULL;
     }
     if (vfio_irq_is_automasked(intp)) {
@@ -80,7 +82,8 @@ static VFIOINTp *vfio_init_intp(VFIODevice *vbasedev,
             g_free(intp->interrupt);
             g_free(intp->unmask);
             g_free(intp);
-            error_report("vfio: Error: resamplefd event_notifier_init failed");
+            error_setg_errno(errp, -ret,
+                             "failed to initialize resample eventd notifier");
             return NULL;
         }
     }
@@ -456,9 +459,10 @@ static int vfio_platform_hot_reset_multi(VFIODevice *vbasedev)
  * vfio_populate_device - Allocate and populate MMIO region
  * and IRQ structs according to driver returned information
  * @vbasedev: the VFIO device handle
+ * @errp: error object
  *
  */
-static int vfio_populate_device(VFIODevice *vbasedev)
+static int vfio_populate_device(VFIODevice *vbasedev, Error **errp)
 {
     VFIOINTp *intp, *tmp;
     int i, ret = -1;
@@ -466,7 +470,7 @@ static int vfio_populate_device(VFIODevice *vbasedev)
         container_of(vbasedev, VFIOPlatformDevice, vbasedev);
 
     if (!(vbasedev->flags & VFIO_DEVICE_FLAGS_PLATFORM)) {
-        error_report("vfio: Um, this isn't a platform device");
+        error_setg(errp, "this isn't a platform device");
         return ret;
     }
 
@@ -480,7 +484,7 @@ static int vfio_populate_device(VFIODevice *vbasedev)
                                 vdev->regions[i], i, name);
         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 reg_error;
         }
     }
@@ -496,16 +500,14 @@ static int vfio_populate_device(VFIODevice *vbasedev)
         irq.index = i;
         ret = ioctl(vbasedev->fd, VFIO_DEVICE_GET_IRQ_INFO, &irq);
         if (ret) {
-            error_report("vfio: error getting device %s irq info",
-                         vbasedev->name);
+            error_setg_errno(errp, -ret, "failed to get device irq info");
             goto irq_err;
         } else {
             trace_vfio_platform_populate_interrupts(irq.index,
                                                     irq.count,
                                                     irq.flags);
-            intp = vfio_init_intp(vbasedev, irq);
+            intp = vfio_init_intp(vbasedev, irq, errp);
             if (!intp) {
-                error_report("vfio: Error installing IRQ %d up", i);
                 goto irq_err;
             }
         }
@@ -613,9 +615,8 @@ static int vfio_base_device_init(VFIODevice *vbasedev)
         goto error;
     }
 
-    ret = vfio_populate_device(vbasedev);
+    ret = vfio_populate_device(vbasedev, &err);
     if (ret) {
-        error_report("vfio: failed to populate device %s", vbasedev->name);
         vfio_put_group(group);
     }
 
-- 
1.9.1

[Qemu-devel] [PATCH v4 12/17] vfio/platform: fix a wrong returned value in vfio_populate_device

[Eric Auger]

In case the vfio_init_intp fails we currently do not return an
error value. This patch fixes the bug. The returned value is not
explicit but in practice the error object is the one used to
report the error to the end-user and the actual returned error
value is not used.

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

diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
index 1a35da0..484e31f 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -508,6 +508,7 @@ static int vfio_populate_device(VFIODevice *vbasedev, Error **errp)
                                                     irq.flags);
             intp = vfio_init_intp(vbasedev, irq, errp);
             if (!intp) {
+                ret = -1;
                 goto irq_err;
             }
         }
-- 
1.9.1

Re: [Qemu-devel] [PATCH v4 12/17] vfio/platform: fix a wrong returned value in vfio_populate_device

[Markus Armbruster]

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

> In case the vfio_init_intp fails we currently do not return an
> error value. This patch fixes the bug. The returned value is not
> explicit but in practice the error object is the one used to
> report the error to the end-user and the actual returned error
> value is not used.

The function's contract permits this by neglecting to say anything about
the return value %-)

The callers don't actually care about the value.

> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  hw/vfio/platform.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
> index 1a35da0..484e31f 100644
> --- a/hw/vfio/platform.c
> +++ b/hw/vfio/platform.c
> @@ -508,6 +508,7 @@ static int vfio_populate_device(VFIODevice *vbasedev, Error **errp)
>                                                      irq.flags);
>              intp = vfio_init_intp(vbasedev, irq, errp);
>              if (!intp) {
> +                ret = -1;
>                  goto irq_err;
>              }
>          }

Re: [Qemu-devel] [PATCH v4 12/17] vfio/platform: fix a wrong returned value in vfio_populate_device

[Auger Eric]

Hi,

On 04/10/2016 14:48, Markus Armbruster wrote:
> Eric Auger <eric.auger@redhat.com> writes:
> 
>> In case the vfio_init_intp fails we currently do not return an
>> error value. This patch fixes the bug. The returned value is not
>> explicit but in practice the error object is the one used to
>> report the error to the end-user and the actual returned error
>> value is not used.
> 
> The function's contract permits this by neglecting to say anything about
> the return value %-)
Yes the error is severe enough to tear things down. I dared to keep it
as is since the error now is reported in the Error object.

Thanks

Eric
> 
> The callers don't actually care about the value.
> 
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  hw/vfio/platform.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
>> index 1a35da0..484e31f 100644
>> --- a/hw/vfio/platform.c
>> +++ b/hw/vfio/platform.c
>> @@ -508,6 +508,7 @@ static int vfio_populate_device(VFIODevice *vbasedev, Error **errp)
>>                                                      irq.flags);
>>              intp = vfio_init_intp(vbasedev, irq, errp);
>>              if (!intp) {
>> +                ret = -1;
>>                  goto irq_err;
>>              }
>>          }
> 

[Qemu-devel] [PATCH v4 13/17] vfio/platform: Pass an error object to vfio_base_device_init

[Eric Auger]

This patch propagates errors encountered during vfio_base_device_init
up to the realize function.

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

---

v3: creation
---
 hw/vfio/platform.c | 49 ++++++++++++++++++++++++++-----------------------
 1 file changed, 26 insertions(+), 23 deletions(-)

diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
index 484e31f..fc06b45 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -541,13 +541,14 @@ static VFIODeviceOps vfio_platform_ops = {
 /**
  * vfio_base_device_init - perform preliminary VFIO setup
  * @vbasedev: the VFIO device handle
+ * @errp: error object
  *
  * Implement the VFIO command sequence that allows to discover
  * assigned device resources: group extraction, device
  * fd retrieval, resource query.
  * Precondition: the device name must be initialized
  */
-static int vfio_base_device_init(VFIODevice *vbasedev)
+static int vfio_base_device_init(VFIODevice *vbasedev, Error **errp)
 {
     VFIOGroup *group;
     VFIODevice *vbasedev_iter;
@@ -555,7 +556,6 @@ 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 */
@@ -564,6 +564,7 @@ static int vfio_base_device_init(VFIODevice *vbasedev)
         vbasedev->name = g_strdup(basename(vbasedev->sysfsdev));
     } else {
         if (!vbasedev->name || strchr(vbasedev->name, '/')) {
+            error_setg(errp, "wrong host device name");
             return -EINVAL;
         }
 
@@ -572,8 +573,7 @@ static int vfio_base_device_init(VFIODevice *vbasedev)
     }
 
     if (stat(vbasedev->sysfsdev, &st) < 0) {
-        error_report("vfio: error: no such host device: %s",
-                     vbasedev->sysfsdev);
+        error_setg_errno(errp, errno, "no such host device");
         return -errno;
     }
 
@@ -582,49 +582,44 @@ static int vfio_base_device_init(VFIODevice *vbasedev)
     g_free(tmp);
 
     if (len < 0 || len >= sizeof(group_path)) {
-        error_report("vfio: error no iommu_group for device");
-        return len < 0 ? -errno : -ENAMETOOLONG;
+        ret = len < 0 ? -errno : -ENAMETOOLONG;
+        error_setg_errno(errp, -ret, "no iommu_group found");
+        return ret;
     }
 
     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);
+        error_setg_errno(errp, errno, "failed to read %s", group_path);
         return -errno;
     }
 
     trace_vfio_platform_base_device_init(vbasedev->name, groupid);
 
-    group = vfio_get_group(groupid, &address_space_memory, &err);
+    group = vfio_get_group(groupid, &address_space_memory, errp);
     if (!group) {
-        ret = -ENOENT;
-        goto error;
+        return -ENOENT;
     }
 
     QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
         if (strcmp(vbasedev_iter->name, vbasedev->name) == 0) {
-            error_report("vfio: error: device %s is already attached",
-                         vbasedev->name);
+            error_setg(errp, "device is already attached");
             vfio_put_group(group);
             return -EBUSY;
         }
     }
-    ret = vfio_get_device(group, vbasedev->name, vbasedev, &err);
+    ret = vfio_get_device(group, vbasedev->name, vbasedev, errp);
     if (ret) {
         vfio_put_group(group);
-        goto error;
+        return ret;
     }
 
-    ret = vfio_populate_device(vbasedev, &err);
+    ret = vfio_populate_device(vbasedev, errp);
     if (ret) {
         vfio_put_group(group);
     }
 
-error:
-    if (err) {
-        error_reportf_err(err, ERR_PREFIX, vbasedev->name);
-    }
     return ret;
 }
 
@@ -650,11 +645,9 @@ static void vfio_platform_realize(DeviceState *dev, Error **errp)
                                 vbasedev->sysfsdev : vbasedev->name,
                                 vdev->compat);
 
-    ret = vfio_base_device_init(vbasedev);
+    ret = vfio_base_device_init(vbasedev, errp);
     if (ret) {
-        error_setg(errp, "vfio: vfio_base_device_init failed for %s",
-                   vbasedev->name);
-        return;
+        goto out;
     }
 
     for (i = 0; i < vbasedev->num_regions; i++) {
@@ -664,6 +657,16 @@ static void vfio_platform_realize(DeviceState *dev, Error **errp)
         }
         sysbus_init_mmio(sbdev, vdev->regions[i]->mem);
     }
+out:
+    if (!ret) {
+        return;
+    }
+
+    if (vdev->vbasedev.name) {
+        error_prepend(errp, ERR_PREFIX, vdev->vbasedev.name);
+    } else {
+        error_prepend(errp, "vfio error: ");
+    }
 }
 
 static const VMStateDescription vfio_platform_vmstate = {
-- 
1.9.1

Re: [Qemu-devel] [PATCH v4 13/17] vfio/platform: Pass an error object to vfio_base_device_init

[Markus Armbruster]

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

> This patch propagates errors encountered during vfio_base_device_init
> up to the realize function.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>
> ---
>
> v3: creation
> ---
>  hw/vfio/platform.c | 49 ++++++++++++++++++++++++++-----------------------
>  1 file changed, 26 insertions(+), 23 deletions(-)
>
> diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
> index 484e31f..fc06b45 100644
> --- a/hw/vfio/platform.c
> +++ b/hw/vfio/platform.c
> @@ -541,13 +541,14 @@ static VFIODeviceOps vfio_platform_ops = {
>  /**
>   * vfio_base_device_init - perform preliminary VFIO setup
>   * @vbasedev: the VFIO device handle
> + * @errp: error object
>   *
>   * Implement the VFIO command sequence that allows to discover
>   * assigned device resources: group extraction, device
>   * fd retrieval, resource query.
>   * Precondition: the device name must be initialized
>   */
> -static int vfio_base_device_init(VFIODevice *vbasedev)
> +static int vfio_base_device_init(VFIODevice *vbasedev, Error **errp)
>  {
>      VFIOGroup *group;
>      VFIODevice *vbasedev_iter;
> @@ -555,7 +556,6 @@ 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 */
> @@ -564,6 +564,7 @@ static int vfio_base_device_init(VFIODevice *vbasedev)
>          vbasedev->name = g_strdup(basename(vbasedev->sysfsdev));
>      } else {
>          if (!vbasedev->name || strchr(vbasedev->name, '/')) {
> +            error_setg(errp, "wrong host device name");

Error is no longer silent.  Worth mentioning in the commit message?

>              return -EINVAL;
>          }
>  
> @@ -572,8 +573,7 @@ static int vfio_base_device_init(VFIODevice *vbasedev)
>      }
>  
>      if (stat(vbasedev->sysfsdev, &st) < 0) {
> -        error_report("vfio: error: no such host device: %s",
> -                     vbasedev->sysfsdev);
> +        error_setg_errno(errp, errno, "no such host device");

"no such host device" is correct and now redundant when errno is ENOENT,
and misleading when it isn't.  Suggest to rephrase the message.

>          return -errno;
>      }
>  
[...]

[Qemu-devel] [PATCH v4 14/17] 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>

---
v2 -> v3:
- use errp directly in all cases

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

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 40ff4a7..b316e13 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2513,13 +2513,12 @@ 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;
     VFIOGroup *group;
     char *tmp, group_path[PATH_MAX], *group_name;
-    Error *err = NULL;
     ssize_t len;
     struct stat st;
     int groupid;
@@ -2533,9 +2532,9 @@ static int vfio_initfn(PCIDevice *pdev)
     }
 
     if (stat(vdev->vbasedev.sysfsdev, &st) < 0) {
-        error_setg_errno(&err, errno, "no such host device");
-        ret = -errno;
-        goto error;
+        error_setg_errno(errp, errno, "no such host device");
+        error_prepend(errp, ERR_PREFIX, vdev->vbasedev.sysfsdev);
+        return;
     }
 
     vdev->vbasedev.name = g_strdup(basename(vdev->vbasedev.sysfsdev));
@@ -2547,8 +2546,8 @@ static int vfio_initfn(PCIDevice *pdev)
     g_free(tmp);
 
     if (len <= 0 || len >= sizeof(group_path)) {
-        ret = len < 0 ? -errno : -ENAMETOOLONG;
-        error_setg_errno(&err, -ret, "no iommu_group found");
+        error_setg_errno(errp, len < 0 ? errno : ENAMETOOLONG,
+                         "no iommu_group found");
         goto error;
     }
 
@@ -2556,36 +2555,33 @@ 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);
+    group = vfio_get_group(groupid, pci_device_iommu_address_space(pdev), errp);
     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;
         }
     }
 
-    ret = vfio_get_device(group, vdev->vbasedev.name, &vdev->vbasedev, &err);
+    ret = vfio_get_device(group, vdev->vbasedev.name, &vdev->vbasedev, errp);
     if (ret) {
         vfio_put_group(group);
         goto error;
     }
 
-    ret = vfio_populate_device(vdev, &err);
-    if (err) {
+    ret = vfio_populate_device(vdev, errp);
+    if (ret) {
         goto error;
     }
 
@@ -2595,7 +2591,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;
     }
 
@@ -2612,8 +2608,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);
@@ -2624,8 +2619,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);
@@ -2636,8 +2630,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,
@@ -2648,8 +2641,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);
@@ -2678,14 +2670,14 @@ static int vfio_initfn(PCIDevice *pdev)
 
     vfio_pci_size_rom(vdev);
 
-    ret = vfio_msix_early_setup(vdev, &err);
-    if (err) {
+    ret = vfio_msix_early_setup(vdev, errp);
+    if (ret) {
         goto error;
     }
 
     vfio_bars_setup(vdev);
 
-    ret = vfio_add_capabilities(vdev, &err);
+    ret = vfio_add_capabilities(vdev, errp);
     if (ret) {
         goto out_teardown;
     }
@@ -2703,10 +2695,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;
         }
 
@@ -2714,12 +2705,12 @@ 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;
         }
 
-        ret = vfio_pci_igd_opregion_init(vdev, opregion, &err);
+        ret = vfio_pci_igd_opregion_init(vdev, opregion, errp);
         g_free(opregion);
         if (ret) {
             goto out_teardown;
@@ -2741,7 +2732,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, &err);
+        ret = vfio_intx_enable(vdev, errp);
         if (ret) {
             goto out_teardown;
         }
@@ -2751,17 +2742,14 @@ 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);
-    }
-    return ret;
+    error_prepend(errp, ERR_PREFIX, vdev->vbasedev.name);
 }
 
 static void vfio_instance_finalize(Object *obj)
@@ -2890,7 +2878,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 v4 14/17] 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>
>
> ---
> v2 -> v3:
> - use errp directly in all cases
>
> v1 -> v2:
> - correct error_setg_errno with positive error values
> ---
>  hw/vfio/pci.c        | 68 ++++++++++++++++++++++------------------------------
>  hw/vfio/trace-events |  2 +-
>  2 files changed, 29 insertions(+), 41 deletions(-)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 40ff4a7..b316e13 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2513,13 +2513,12 @@ 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;
>      VFIOGroup *group;
>      char *tmp, group_path[PATH_MAX], *group_name;
> -    Error *err = NULL;
>      ssize_t len;
>      struct stat st;
>      int groupid;
> @@ -2533,9 +2532,9 @@ static int vfio_initfn(PCIDevice *pdev)
>      }
>  
>      if (stat(vdev->vbasedev.sysfsdev, &st) < 0) {
> -        error_setg_errno(&err, errno, "no such host device");
> -        ret = -errno;
> -        goto error;
> +        error_setg_errno(errp, errno, "no such host device");
> +        error_prepend(errp, ERR_PREFIX, vdev->vbasedev.sysfsdev);
> +        return;
>      }
>  
>      vdev->vbasedev.name = g_strdup(basename(vdev->vbasedev.sysfsdev));
> @@ -2547,8 +2546,8 @@ static int vfio_initfn(PCIDevice *pdev)
>      g_free(tmp);
>  
>      if (len <= 0 || len >= sizeof(group_path)) {
> -        ret = len < 0 ? -errno : -ENAMETOOLONG;
> -        error_setg_errno(&err, -ret, "no iommu_group found");
> +        error_setg_errno(errp, len < 0 ? errno : ENAMETOOLONG,
> +                         "no iommu_group found");
>          goto error;
>      }
>  
> @@ -2556,36 +2555,33 @@ 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);
> +    group = vfio_get_group(groupid, pci_device_iommu_address_space(pdev), errp);
>      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;
>          }
>      }
>  
> -    ret = vfio_get_device(group, vdev->vbasedev.name, &vdev->vbasedev, &err);
> +    ret = vfio_get_device(group, vdev->vbasedev.name, &vdev->vbasedev, errp);
>      if (ret) {
>          vfio_put_group(group);
>          goto error;
>      }
>  
> -    ret = vfio_populate_device(vdev, &err);
> -    if (err) {
> +    ret = vfio_populate_device(vdev, errp);
> +    if (ret) {

The if (err) comes from PATCH 03.  You could reduce churn by checking
ret from the start.  More of the same below.  Your choice.

>          goto error;
>      }
>  
[...]

Re: [Qemu-devel] [PATCH v4 14/17] vfio/pci: Conversion to realize

[Auger Eric]

Hi Markus

On 04/10/2016 14:58, 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>
>>
>> ---
>> v2 -> v3:
>> - use errp directly in all cases
>>
>> v1 -> v2:
>> - correct error_setg_errno with positive error values
>> ---
>>  hw/vfio/pci.c        | 68 ++++++++++++++++++++++------------------------------
>>  hw/vfio/trace-events |  2 +-
>>  2 files changed, 29 insertions(+), 41 deletions(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index 40ff4a7..b316e13 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -2513,13 +2513,12 @@ 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;
>>      VFIOGroup *group;
>>      char *tmp, group_path[PATH_MAX], *group_name;
>> -    Error *err = NULL;
>>      ssize_t len;
>>      struct stat st;
>>      int groupid;
>> @@ -2533,9 +2532,9 @@ static int vfio_initfn(PCIDevice *pdev)
>>      }
>>  
>>      if (stat(vdev->vbasedev.sysfsdev, &st) < 0) {
>> -        error_setg_errno(&err, errno, "no such host device");
>> -        ret = -errno;
>> -        goto error;
>> +        error_setg_errno(errp, errno, "no such host device");
>> +        error_prepend(errp, ERR_PREFIX, vdev->vbasedev.sysfsdev);
>> +        return;
>>      }
>>  
>>      vdev->vbasedev.name = g_strdup(basename(vdev->vbasedev.sysfsdev));
>> @@ -2547,8 +2546,8 @@ static int vfio_initfn(PCIDevice *pdev)
>>      g_free(tmp);
>>  
>>      if (len <= 0 || len >= sizeof(group_path)) {
>> -        ret = len < 0 ? -errno : -ENAMETOOLONG;
>> -        error_setg_errno(&err, -ret, "no iommu_group found");
>> +        error_setg_errno(errp, len < 0 ? errno : ENAMETOOLONG,
>> +                         "no iommu_group found");
>>          goto error;
>>      }
>>  
>> @@ -2556,36 +2555,33 @@ 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);
>> +    group = vfio_get_group(groupid, pci_device_iommu_address_space(pdev), errp);
>>      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;
>>          }
>>      }
>>  
>> -    ret = vfio_get_device(group, vdev->vbasedev.name, &vdev->vbasedev, &err);
>> +    ret = vfio_get_device(group, vdev->vbasedev.name, &vdev->vbasedev, errp);
>>      if (ret) {
>>          vfio_put_group(group);
>>          goto error;
>>      }
>>  
>> -    ret = vfio_populate_device(vdev, &err);
>> -    if (err) {
>> +    ret = vfio_populate_device(vdev, errp);
>> +    if (ret) {
> 
> The if (err) comes from PATCH 03.  You could reduce churn by checking
> ret from the start.  More of the same below.  Your choice.

I removed this spurious flip for vfio_populate_device and
vfio_msix_early_setup

Thanks

Eric
> 
>>          goto error;
>>      }
>>  
> [...]
> 

[Qemu-devel] [PATCH v4 15/17] 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>

---

Logically we could do that job for all the functions now getting an
Error object passed as a parameter to avoid duplicate information
between the error content and the returned value. This requires to use
a local error object in vfio_realize. So I am not sure this is worth
the candle.
---
 hw/vfio/pci.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index b316e13..cea4d48 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1290,7 +1290,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;
@@ -1300,25 +1300,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);
@@ -1351,7 +1351,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;
         }
     }
 
@@ -1360,8 +1360,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)
@@ -2519,6 +2517,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
     VFIODevice *vbasedev_iter;
     VFIOGroup *group;
     char *tmp, group_path[PATH_MAX], *group_name;
+    Error *err = NULL;
     ssize_t len;
     struct stat st;
     int groupid;
@@ -2670,8 +2669,9 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
 
     vfio_pci_size_rom(vdev);
 
-    ret = vfio_msix_early_setup(vdev, errp);
-    if (ret) {
+    vfio_msix_early_setup(vdev, &err);
+    if (err) {
+        error_propagate(errp, err);
         goto error;
     }
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH v4 15/17] vfio/pci: Remove vfio_msix_early_setup returned value

[Markus Armbruster]

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

> 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>
>
> ---
>
> Logically we could do that job for all the functions now getting an
> Error object passed as a parameter to avoid duplicate information
> between the error content and the returned value. This requires to use
> a local error object in vfio_realize. So I am not sure this is worth
> the candle.

Matter of taste, yours is fine.

We used to recommend returing void instead of an error code when the
function sets and error.  More parsimonious in theory, more boiler-plate
in practice, so we accept either now.  Perhaps we should even recommend
returning an error code, but such a recommendation needs to come with
patches converting existing code to it.

> ---
>  hw/vfio/pci.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index b316e13..cea4d48 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -1290,7 +1290,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;
> @@ -1300,25 +1300,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);
> @@ -1351,7 +1351,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;
>          }
>      }
>  
> @@ -1360,8 +1360,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)
> @@ -2519,6 +2517,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>      VFIODevice *vbasedev_iter;
>      VFIOGroup *group;
>      char *tmp, group_path[PATH_MAX], *group_name;
> +    Error *err = NULL;
>      ssize_t len;
>      struct stat st;
>      int groupid;
> @@ -2670,8 +2669,9 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>  
>      vfio_pci_size_rom(vdev);
>  
> -    ret = vfio_msix_early_setup(vdev, errp);
> -    if (ret) {
> +    vfio_msix_early_setup(vdev, &err);
> +    if (err) {
> +        error_propagate(errp, err);
>          goto error;
>      }

PATCH 04 checks err, PATCH 13 flips to ret, and this one flips back.
Have you considered dropping both flips?  Your choice.

Re: [Qemu-devel] [PATCH v4 15/17] vfio/pci: Remove vfio_msix_early_setup returned value

[Auger Eric]

Hi Markus,

On 04/10/2016 15:05, Markus Armbruster wrote:
> Eric Auger <eric.auger@redhat.com> writes:
> 
>> 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>
>>
>> ---
>>
>> Logically we could do that job for all the functions now getting an
>> Error object passed as a parameter to avoid duplicate information
>> between the error content and the returned value. This requires to use
>> a local error object in vfio_realize. So I am not sure this is worth
>> the candle.
> 
> Matter of taste, yours is fine.
> 
> We used to recommend returing void instead of an error code when the
> function sets and error.  More parsimonious in theory, more boiler-plate
> in practice, so we accept either now.  Perhaps we should even recommend
> returning an error code, but such a recommendation needs to come with
> patches converting existing code to it.

The risk is that if a programmer returns an error value without setting
the errp he will get a sigsev on subsequent error_prepend().
> 
>> ---
>>  hw/vfio/pci.c | 20 ++++++++++----------
>>  1 file changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index b316e13..cea4d48 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -1290,7 +1290,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;
>> @@ -1300,25 +1300,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);
>> @@ -1351,7 +1351,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;
>>          }
>>      }
>>  
>> @@ -1360,8 +1360,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)
>> @@ -2519,6 +2517,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>>      VFIODevice *vbasedev_iter;
>>      VFIOGroup *group;
>>      char *tmp, group_path[PATH_MAX], *group_name;
>> +    Error *err = NULL;
>>      ssize_t len;
>>      struct stat st;
>>      int groupid;
>> @@ -2670,8 +2669,9 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>>  
>>      vfio_pci_size_rom(vdev);
>>  
>> -    ret = vfio_msix_early_setup(vdev, errp);
>> -    if (ret) {
>> +    vfio_msix_early_setup(vdev, &err);
>> +    if (err) {
>> +        error_propagate(errp, err);
>>          goto error;
>>      }
> 
> PATCH 04 checks err, PATCH 13 flips to ret, and this one flips back.
> Have you considered dropping both flips?  Your choice.
I removed one flip at least

Thanks

Eric
> 

Re: [Qemu-devel] [PATCH v4 15/17] vfio/pci: Remove vfio_msix_early_setup returned value

[Markus Armbruster]

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

> Hi Markus,
>
> On 04/10/2016 15:05, Markus Armbruster wrote:
>> Eric Auger <eric.auger@redhat.com> writes:
>> 
>>> 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>
>>>
>>> ---
>>>
>>> Logically we could do that job for all the functions now getting an
>>> Error object passed as a parameter to avoid duplicate information
>>> between the error content and the returned value. This requires to use
>>> a local error object in vfio_realize. So I am not sure this is worth
>>> the candle.
>> 
>> Matter of taste, yours is fine.
>> 
>> We used to recommend returing void instead of an error code when the
>> function sets and error.  More parsimonious in theory, more boiler-plate
>> in practice, so we accept either now.  Perhaps we should even recommend
>> returning an error code, but such a recommendation needs to come with
>> patches converting existing code to it.
>
> The risk is that if a programmer returns an error value without setting
> the errp he will get a sigsev on subsequent error_prepend().

Yes, the function contract becomes more complex, giving the programmer
another way to screw up.  Besides crashing, callers might also detect
success as failure, failure as success, and leak error objects.

I don't particularly like setting such traps, but:

1. the risk already exists for functions returning a distinct value on
failure, e.g. null on failure and non-null on success, and

2. I've gotten really tired of the extra error-checking boiler-plate
necessary for functions returning void.

[...]

[Qemu-devel] [PATCH v4 16/17] 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 | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index cea4d48..eb08d01 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2223,7 +2223,7 @@ int vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp)
     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;
@@ -2233,18 +2233,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++) {
@@ -2256,7 +2256,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);
@@ -2266,7 +2266,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,
@@ -2287,7 +2287,7 @@ static int vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
         if (ret) {
             error_append_hint(errp, "device does not support "
                               "requested feature x-vga\n");
-            goto error;
+            return;
         }
     }
 
@@ -2297,7 +2297,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 {
@@ -2305,9 +2304,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)
@@ -2579,8 +2575,9 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
         goto error;
     }
 
-    ret = vfio_populate_device(vdev, errp);
-    if (ret) {
+    vfio_populate_device(vdev, &err);
+    if (err) {
+        error_propagate(errp, err);
         goto error;
     }
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH v4 16/17] vfio/pci: Remove vfio_populate_device returned value

[Markus Armbruster]

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

> 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 | 23 ++++++++++-------------
>  1 file changed, 10 insertions(+), 13 deletions(-)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index cea4d48..eb08d01 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2223,7 +2223,7 @@ int vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp)
>      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;
> @@ -2233,18 +2233,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++) {
> @@ -2256,7 +2256,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);
> @@ -2266,7 +2266,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,
> @@ -2287,7 +2287,7 @@ static int vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
>          if (ret) {
>              error_append_hint(errp, "device does not support "
>                                "requested feature x-vga\n");
> -            goto error;
> +            return;
>          }
>      }
>  
> @@ -2297,7 +2297,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 {
> @@ -2305,9 +2304,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)
> @@ -2579,8 +2575,9 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>          goto error;
>      }
>  
> -    ret = vfio_populate_device(vdev, errp);
> -    if (ret) {
> +    vfio_populate_device(vdev, &err);
> +    if (err) {
> +        error_propagate(errp, err);
>          goto error;
>      }

PATCH 03 checks err, PATCH 14 flips to ret, and this one flips back.
Have you considered dropping both flips?  Your choice.

[Qemu-devel] [PATCH v4 17/17] 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 eb08d01..e339790 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2520,6 +2520,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,
@@ -2828,6 +2835,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 v4 00/17] 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.
>
> A side effect of this series is the cleanup of the error handling
> in vfio-platform.

Only a few really minor issues left, so series
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v4 00/17] Convert VFIO-PCI to realize

[Alex Williamson]

On Tue, 04 Oct 2016 15:20:12 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> 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.
> >
> > A side effect of this series is the cleanup of the error handling
> > in vfio-platform.  
> 
> Only a few really minor issues left, so series
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Eric, thanks for doing this conversion and thanks to Markus for the
thorough reviews.  One tiny nit I see is patch 06/17 does this:

> @@ -1189,6 +1189,7 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos)
>  
>      if (pread(vdev->vbasedev.fd, &ctrl, sizeof(ctrl),
>                vdev->config_offset + pos + PCI_CAP_FLAGS) != sizeof(ctrl)) {
> +        error_setg_errno(errp, errno, "failed reading PCI_CAP_FLAGS");
>          return -errno;
>      }

And that new error string might be more useful if it were "failed
reading MSI PCI_CAP_FLAGS".  Unless some sort of MSI hint gets added
somewhere that I'm missing.

Markus noted a few things that could be fixed on commit and several
others that I don't want to decide for you, so I'll look forward to a
v5 to wrap this up.  Thanks!

Alex

Re: [Qemu-devel] [PATCH v4 00/17] Convert VFIO-PCI to realize

[Auger Eric]

Hi Markus,

On 04/10/2016 15:20, Markus Armbruster wrote:
> 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.
>>
>> A side effect of this series is the cleanup of the error handling
>> in vfio-platform.
> 
> Only a few really minor issues left, so series
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>

Thank you very much for your comprehensive review. Starting v5 respin
taking into account your your last comments + Alex's one!

Best Regards

Eric