Re: [PATCH v4 18/25] vfio: Add Error** argument to vfio_devices_dma_logging_stop()

[Cédric Le Goater]

On 3/7/24 09:53, Eric Auger wrote:

On 3/6/24 14:34, Cédric Le Goater wrote:

This improves error reporting in the log_global_stop() VFIO handler.

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

  Changes in v4:

  - Dropped log_global_stop() and log_global_sync() changes

Changes in v3:

  - Use error_setg_errno() in vfio_devices_dma_logging_stop()

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

  1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 
d6790557da2f2890398fa03dbbef18129cd2c1bb..5b2e6a179cdd5f8ca5be84b7097661e96b391456
 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -938,12 +938,14 @@ static void vfio_dirty_tracking_init(VFIOContainerBase 
*bcontainer,
      memory_listener_unregister(&dirty.listener);
  }

-static void vfio_devices_dma_logging_stop(VFIOContainerBase *bcontainer)

+static int vfio_devices_dma_logging_stop(VFIOContainerBase *bcontainer,
+                                          Error **errp)
  {
      uint64_t buf[DIV_ROUND_UP(sizeof(struct vfio_device_feature),
                                sizeof(uint64_t))] = {};
      struct vfio_device_feature *feature = (struct vfio_device_feature *)buf;
      VFIODevice *vbasedev;
+    int ret = 0;

feature->argsz = sizeof(buf);

      feature->flags = VFIO_DEVICE_FEATURE_SET |
@@ -955,11 +957,17 @@ static void 
vfio_devices_dma_logging_stop(VFIOContainerBase *bcontainer)
          }

if (ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature)) {

-            warn_report("%s: Failed to stop DMA logging, err %d (%s)",
-                        vbasedev->name, -errno, strerror(errno));
+            /* Keep first error */
+            if (!ret) {
+                ret = -errno;
+                error_setg_errno(errp, errno, "%s: Failed to stop DMA logging",
+                                 vbasedev->name);

maybe you can keep the previous warn_report in case errp is NULL
(rollback) or for subsequent failures?

Hmm, I wonder if we should keep this patch. It made sense when
vfio_listener_log_global_stop() had an Error ** parameter. We
dropped it in v4 for the sake of simplicity, so we might as
well keep the previous behavior and simply warn the user when
dirty tracking fails to stop. I will check for v5.

Thanks,

C.

Eric

+            }
          }
          vbasedev->dirty_tracking = false;
      }
+
+    return ret;
  }

static struct vfio_device_feature *

@@ -1068,7 +1076,8 @@ static int 
vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer,

out:

      if (ret) {
-        vfio_devices_dma_logging_stop(bcontainer);
+        /* Ignore the potential errors when doing rollback */
+        vfio_devices_dma_logging_stop(bcontainer, NULL);
      }

vfio_device_feature_dma_logging_start_destroy(feature);

@@ -1104,7 +1113,7 @@ static void vfio_listener_log_global_stop(MemoryListener 
*listener)
      int ret = 0;

if (vfio_devices_all_device_dirty_tracking(bcontainer)) {

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