Re: [PATCH v5 10/10] vfio: Extend vfio_set_migration_error() with Error* argument

[Cédric Le Goater]

On 5/13/24 16:26, Avihai Horon wrote:

On 06/05/2024 12:20, Cédric Le Goater wrote:
External email: Use caution opening links or attachments


vfio_set_migration_error() sets the 'return' error on the migration
stream if a migration is in progress. To improve error reporting, add
a new Error* argument to also set the Error object on the migration
stream, if a migration is progress.

Signed-off-by: Cédric Le Goater <c...@redhat.com>
---

  Changes in v5:

  - Rebased on 20c64c8a51a4 ("migration: migration_file_set_error")

  hw/vfio/common.c | 37 ++++++++++++++++++-------------------
  1 file changed, 18 insertions(+), 19 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 
c3d82a9d6e434e33f361e4b96157bf912d5c3a2f..4cf3e13a8439bd1b9a032e9d4e75df676eba457b
 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -147,10 +147,10 @@ bool vfio_viommu_preset(VFIODevice *vbasedev)
      return vbasedev->bcontainer->space->as != &address_space_memory;
  }

-static void vfio_set_migration_error(int err)
+static void vfio_set_migration_error(int ret, Error *err)
  {
      if (migration_is_setup_or_active()) {
-        migration_file_set_error(err, NULL);
+        migration_file_set_error(ret, err);
      }
  }

@@ -295,9 +295,10 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, 
IOMMUTLBEntry *iotlb)
                                  iova, iova + iotlb->addr_mask);

      if (iotlb->target_as != &address_space_memory) {
-        error_report("Wrong target AS \"%s\", only system memory is allowed",
-                     iotlb->target_as->name ? iotlb->target_as->name : "none");
-        vfio_set_migration_error(-EINVAL);
+        error_setg(&local_err,
+                   "Wrong target AS \"%s\", only system memory is allowed",
+                   iotlb->target_as->name ? iotlb->target_as->name : "none");
+        vfio_set_migration_error(-EINVAL, local_err);
          return;
      }

@@ -330,11 +331,12 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, 
IOMMUTLBEntry *iotlb)
          ret = vfio_container_dma_unmap(bcontainer, iova,
                                         iotlb->addr_mask + 1, iotlb);
          if (ret) {
-            error_report("vfio_container_dma_unmap(%p, 0x%"HWADDR_PRIx", "
-                         "0x%"HWADDR_PRIx") = %d (%s)",
-                         bcontainer, iova,
-                         iotlb->addr_mask + 1, ret, strerror(-ret));
-            vfio_set_migration_error(ret);
+            error_setg(&local_err,
+                       "vfio_container_dma_unmap(%p, 0x%"HWADDR_PRIx", "
+                       "0x%"HWADDR_PRIx") = %d (%s)",
+                       bcontainer, iova,
+                       iotlb->addr_mask + 1, ret, strerror(-ret));

Use error_setg_errno()?

sure.


+            vfio_set_migration_error(ret, local_err);

Now dma unmap errors (and also the error before it) are not reported if they 
happen not during migration.

This makes me think, maybe vfio_set_migration_error() is redundant and can be 
replaced by migration_file_set_error()?


yes. Good suggestion. I would like to get rid of vfio_set_migration_error(),
so that's a good start.

Thanks,

C.
 
Thanks.

          }
      }
  out:
@@ -1108,8 +1110,7 @@ static void vfio_listener_log_global_stop(MemoryListener 
*listener)
      if (ret) {
          error_prepend(&local_err,
                        "vfio: Could not stop dirty page tracking - ");
-        error_report_err(local_err);
-        vfio_set_migration_error(ret);
+        vfio_set_migration_error(ret, local_err);
      }
  }

@@ -1226,14 +1227,14 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier 
*n, IOMMUTLBEntry *iotlb)
      trace_vfio_iommu_map_dirty_notify(iova, iova + iotlb->addr_mask);

      if (iotlb->target_as != &address_space_memory) {
-        error_report("Wrong target AS \"%s\", only system memory is allowed",
-                     iotlb->target_as->name ? iotlb->target_as->name : "none");
+        error_setg(&local_err,
+                   "Wrong target AS \"%s\", only system memory is allowed",
+                   iotlb->target_as->name ? iotlb->target_as->name : "none");
          goto out;
      }

      rcu_read_lock();
      if (!vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL, &local_err)) 
{
-        error_report_err(local_err);
          goto out_lock;
      }

@@ -1244,7 +1245,6 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, 
IOMMUTLBEntry *iotlb)
                        "vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", "
                        "0x%"HWADDR_PRIx") failed - ", bcontainer, iova,
                        iotlb->addr_mask + 1);
-        error_report_err(local_err);
      }

  out_lock:
@@ -1252,7 +1252,7 @@ out_lock:

  out:
      if (ret) {
-        vfio_set_migration_error(ret);
+        vfio_set_migration_error(ret, local_err);
      }
  }

@@ -1372,8 +1372,7 @@ static void vfio_listener_log_sync(MemoryListener 
*listener,
      if (vfio_devices_all_dirty_tracking(bcontainer)) {
          ret = vfio_sync_dirty_bitmap(bcontainer, section, &local_err);
          if (ret) {
-            error_report_err(local_err);
-            vfio_set_migration_error(ret);
+            vfio_set_migration_error(ret, local_err);
          }
      }
  }
--
2.45.0