Re: [PATCH v6 2/9] vfio: Add Error** argument to vfio_devices_dma_logging_start()

[Cédric Le Goater]

On 5/15/24 08:53, Eric Auger wrote:

Hi Cédric,
On 5/14/24 17:31, Cédric Le Goater wrote:

This allows to update the Error argument of the VFIO log_global_start()
handler. Errors for container based logging will also be propagated to
qemu_savevm_state_setup() when the ram save_setup() handler is executed.

nit: also now collect & print errors from
vfio_container_set_dirty_page_tracking()

OK. To avoid resending, I amended the commit log with :

"Also, errors from vfio_container_set_dirty_page_tracking() are now
collected and reported."


Thanks,

C.

The vfio_set_migration_error() call becomes redundant in
vfio_listener_log_global_start(). Remove it.

Reviewed-by: Philippe Mathieu-Daudé <phi...@linaro.org>
Reviewed-by: Avihai Horon <avih...@nvidia.com>
Signed-off-by: Cédric Le Goater <c...@redhat.com>
---

  Changes in v6:

  - Commit log improvements (Avihai)

Changes in v5:

  - Used error_setg_errno() in vfio_devices_dma_logging_start()

hw/vfio/common.c | 26 +++++++++++++++-----------

  1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 
485e53916491f1164d29e739fb7106c0c77df737..b5102f54a6474a50c6366e8fbce23812d55e384e
 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1027,7 +1027,8 @@ static void vfio_device_feature_dma_logging_start_destroy(
      g_free(feature);
  }

-static int vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer)

+static int vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer,
+                                          Error **errp)
  {
      struct vfio_device_feature *feature;
      VFIODirtyRanges ranges;
@@ -1038,6 +1039,7 @@ static int 
vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer)
      feature = vfio_device_feature_dma_logging_start_create(bcontainer,
                                                             &ranges);
      if (!feature) {
+        error_setg_errno(errp, errno, "Failed to prepare DMA logging");
          return -errno;
      }

@@ -1049,8 +1051,8 @@ static int vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer)

          ret = ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature);
          if (ret) {
              ret = -errno;
-            error_report("%s: Failed to start DMA logging, err %d (%s)",
-                         vbasedev->name, ret, strerror(errno));
+            error_setg_errno(errp, errno, "%s: Failed to start DMA logging",
+                             vbasedev->name);
              goto out;
          }
          vbasedev->dirty_tracking = true;
@@ -1069,20 +1071,19 @@ out:
  static bool vfio_listener_log_global_start(MemoryListener *listener,
                                             Error **errp)
  {
+    ERRP_GUARD();
      VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
                                                   listener);
      int ret;

if (vfio_devices_all_device_dirty_tracking(bcontainer)) {

-        ret = vfio_devices_dma_logging_start(bcontainer);
+        ret = vfio_devices_dma_logging_start(bcontainer, errp);
      } else {
-        ret = vfio_container_set_dirty_page_tracking(bcontainer, true, NULL);
+        ret = vfio_container_set_dirty_page_tracking(bcontainer, true, errp);
      }

if (ret) {

-        error_report("vfio: Could not start dirty page tracking, err: %d (%s)",
-                     ret, strerror(-ret));
-        vfio_set_migration_error(ret);
+        error_prepend(errp, "vfio: Could not start dirty page tracking - ");
      }
      return !ret;
  }
@@ -1091,17 +1092,20 @@ static void 
vfio_listener_log_global_stop(MemoryListener *listener)
  {
      VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
                                                   listener);
+    Error *local_err = NULL;
      int ret = 0;

if (vfio_devices_all_device_dirty_tracking(bcontainer)) {

          vfio_devices_dma_logging_stop(bcontainer);
      } else {
-        ret = vfio_container_set_dirty_page_tracking(bcontainer, false, NULL);
+        ret = vfio_container_set_dirty_page_tracking(bcontainer, false,
+                                                     &local_err);
      }

if (ret) {

-        error_report("vfio: Could not stop dirty page tracking, err: %d (%s)",
-                     ret, strerror(-ret));
+        error_prepend(&local_err,
+                      "vfio: Could not stop dirty page tracking - ");
+        error_report_err(local_err);
          vfio_set_migration_error(ret);
      }
  }

Reviewed-by: Eric Auger <eric.au...@redhat.com>

Eric