[PATCH 00/14] migration: Improve error reporting

[PATCH 00/14] migration: Improve error reporting

[Cédric Le Goater]

Hello,

The motivation behind these changes is to improve error reporting to
the upper management layer (libvirt) with a more detailed error, this
to let it decide, depending on the reported error, whether to try
migration again later. It would be useful in cases where migration
fails due to lack of HW resources on the host. For instance, some
adapters can only initiate a limited number of simultaneous dirty
tracking requests and this imposes a limit on the the number of VMs
that can be migrated simultaneously.

We are not quite ready for such a mechanism but what we can do first is
to cleanup the error reporting ​in the early save_setup sequence. This
is what the following changes propose, by adding an Error argument to
various handlers and propagating it to the core migration subsystem.

The last patches try to address a related issue found on VMs with MLX5
VF assigned devices. These are one of those adapters with the HW
limitation described above. If dirty tracking setup fails and
return-path is in use, the return-path thread does not terminate,
leaving the source and destination VMs waiting for an event to occur.

The last patch is still an RFC because the correct fix is not obvious
and implies reworking the QEMUFile software construct, built on top of
the QEMU I/O channel.
 
Thanks,

C.

[1] https://lore.kernel.org/qemu-devel/20240201184853.890471-1-clg@redhat.com/

Cédric Le Goater (14):
  migration: Add Error** argument to .save_setup() handler
  migration: Add Error** argument to .load_setup() handler
  memory: Add Error** argument to .log_global*() handlers
  migration: Modify ram_init_bitmaps() to report dirty tracking errors
  vfio: Add Error** argument to .set_dirty_page_tracking() handler
  vfio: Add Error** argument to vfio_devices_dma_logging_start()
  vfio: Add Error** argument to vfio_devices_dma_logging_stop()
  vfio: Use new Error** argument in vfio_save_setup()
  vfio: Add Error** argument to .vfio_save_config() handler
  vfio: Also trace event failures in vfio_save_complete_precopy()
  vfio: Extend vfio_set_migration_error() with Error* argument
  migration: Report error when shutdown fails
  migration: Use migrate_has_error() in close_return_path_on_source()
  migration: Fix return-path thread exit

 include/exec/memory.h                 | 12 ++--
 include/hw/vfio/vfio-common.h         |  2 +-
 include/hw/vfio/vfio-container-base.h |  4 +-
 include/migration/register.h          |  4 +-
 hw/i386/xen/xen-hvm.c                 |  8 +--
 hw/ppc/spapr.c                        |  2 +-
 hw/s390x/s390-stattrib.c              |  2 +-
 hw/vfio/common.c                      | 96 ++++++++++++++++-----------
 hw/vfio/container-base.c              |  4 +-
 hw/vfio/container.c                   |  6 +-
 hw/vfio/migration.c                   | 87 +++++++++++++++---------
 hw/vfio/pci.c                         |  5 +-
 hw/virtio/vhost.c                     |  4 +-
 migration/block-dirty-bitmap.c        |  2 +-
 migration/block.c                     |  2 +-
 migration/dirtyrate.c                 | 24 +++++--
 migration/migration.c                 | 16 ++---
 migration/qemu-file.c                 |  5 +-
 migration/ram.c                       | 40 ++++++++---
 migration/savevm.c                    | 14 ++--
 system/memory.c                       | 37 +++++++----
 21 files changed, 236 insertions(+), 140 deletions(-)

-- 
2.43.0

[PATCH 01/14] migration: Add Error** argument to .save_setup() handler

[Cédric Le Goater]

The purpose is to record a potential error in the migration stream if
qemu_savevm_state_setup() fails. Most of the current .save_setup()
handlers can be modified to use the Error argument instead of managing
their own and calling locally error_report(). The following patches
will introduce such changes for VFIO first.

Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 include/migration/register.h   | 2 +-
 hw/ppc/spapr.c                 | 2 +-
 hw/s390x/s390-stattrib.c       | 2 +-
 hw/vfio/migration.c            | 2 +-
 migration/block-dirty-bitmap.c | 2 +-
 migration/block.c              | 2 +-
 migration/ram.c                | 2 +-
 migration/savevm.c             | 4 ++--
 8 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/include/migration/register.h b/include/migration/register.h
index 9ab1f79512c605f0c88a45b560c57486fa054441..831600a00eae4efd0464b60925d65de4d9dbcff8 100644
--- a/include/migration/register.h
+++ b/include/migration/register.h
@@ -25,7 +25,7 @@ typedef struct SaveVMHandlers {
      * used to perform early checks.
      */
     int (*save_prepare)(void *opaque, Error **errp);
-    int (*save_setup)(QEMUFile *f, void *opaque);
+    int (*save_setup)(QEMUFile *f, void *opaque, Error **errp);
     void (*save_cleanup)(void *opaque);
     int (*save_live_complete_postcopy)(QEMUFile *f, void *opaque);
     int (*save_live_complete_precopy)(QEMUFile *f, void *opaque);
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 0d72d286d80f0435122593555f79fae4d90acf81..a1b0aa02582ad2d68a13476c1859b18143da7bb8 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2142,7 +2142,7 @@ static const VMStateDescription vmstate_spapr = {
     }
 };
 
-static int htab_save_setup(QEMUFile *f, void *opaque)
+static int htab_save_setup(QEMUFile *f, void *opaque, Error **errp)
 {
     SpaprMachineState *spapr = opaque;
 
diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c
index c483b62a9b5f71772639fc180bdad15ecb6711cb..c934df424a555d83d2198f5ddfc0cbe0ea98e9ec 100644
--- a/hw/s390x/s390-stattrib.c
+++ b/hw/s390x/s390-stattrib.c
@@ -166,7 +166,7 @@ static int cmma_load(QEMUFile *f, void *opaque, int version_id)
     return ret;
 }
 
-static int cmma_save_setup(QEMUFile *f, void *opaque)
+static int cmma_save_setup(QEMUFile *f, void *opaque, Error **errp)
 {
     S390StAttribState *sas = S390_STATTRIB(opaque);
     S390StAttribClass *sac = S390_STATTRIB_GET_CLASS(sas);
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 70e6b1a709f9b67e4c9eb41033d76347275cac42..8bcb4bc73cd5ba5338e3ffa4d907d0e6bfbb9485 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -378,7 +378,7 @@ static int vfio_save_prepare(void *opaque, Error **errp)
     return 0;
 }
 
-static int vfio_save_setup(QEMUFile *f, void *opaque)
+static int vfio_save_setup(QEMUFile *f, void *opaque, Error **errp)
 {
     VFIODevice *vbasedev = opaque;
     VFIOMigration *migration = vbasedev->migration;
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 2708abf3d762de774ed294d3fdb8e56690d2974c..16f84e6c57c2403a8c2d6319f4e7b6360dade28c 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -1213,7 +1213,7 @@ fail:
     return ret;
 }
 
-static int dirty_bitmap_save_setup(QEMUFile *f, void *opaque)
+static int dirty_bitmap_save_setup(QEMUFile *f, void *opaque, Error **errp)
 {
     DBMSaveState *s = &((DBMState *)opaque)->save;
     SaveBitmapState *dbms = NULL;
diff --git a/migration/block.c b/migration/block.c
index 8c6ebafacc1ffe930d1d4f19d968817b14852c69..df15319ceab66201b043f15eac1b0a7d6522b60c 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -708,7 +708,7 @@ static void block_migration_cleanup(void *opaque)
     blk_mig_unlock();
 }
 
-static int block_save_setup(QEMUFile *f, void *opaque)
+static int block_save_setup(QEMUFile *f, void *opaque, Error **errp)
 {
     int ret;
 
diff --git a/migration/ram.c b/migration/ram.c
index d5b7cd5ac2f31aabf4a248b966153401c48912cf..136c237f4079f68d4e578cf1c72eec2efc815bc8 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2931,7 +2931,7 @@ void qemu_guest_free_page_hint(void *addr, size_t len)
  * @f: QEMUFile where to send the data
  * @opaque: RAMState pointer
  */
-static int ram_save_setup(QEMUFile *f, void *opaque)
+static int ram_save_setup(QEMUFile *f, void *opaque, Error **errp)
 {
     RAMState **rsp = opaque;
     RAMBlock *block;
diff --git a/migration/savevm.c b/migration/savevm.c
index d612c8a9020b204d5d078d5df85f0e6449c27645..f2ae799bad13e631bccf733a34c3a8fd22e8dd48 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1342,10 +1342,10 @@ void qemu_savevm_state_setup(QEMUFile *f)
         }
         save_section_header(f, se, QEMU_VM_SECTION_START);
 
-        ret = se->ops->save_setup(f, se->opaque);
+        ret = se->ops->save_setup(f, se->opaque, &local_err);
         save_section_footer(f, se);
         if (ret < 0) {
-            qemu_file_set_error(f, ret);
+            qemu_file_set_error_obj(f, ret, local_err);
             break;
         }
     }
-- 
2.43.0

[PATCH 02/14] migration: Add Error** argument to .load_setup() handler

[Cédric Le Goater]

This will be useful to report errors at a higher level, mostly in VFIO
today.

Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 include/migration/register.h |  2 +-
 hw/vfio/migration.c          |  2 +-
 migration/ram.c              |  2 +-
 migration/savevm.c           | 10 ++++++----
 4 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/include/migration/register.h b/include/migration/register.h
index 831600a00eae4efd0464b60925d65de4d9dbcff8..e6bc226c98b27c1fb0f9e2b56d8aff491aa14d65 100644
--- a/include/migration/register.h
+++ b/include/migration/register.h
@@ -72,7 +72,7 @@ typedef struct SaveVMHandlers {
     void (*state_pending_exact)(void *opaque, uint64_t *must_precopy,
                                 uint64_t *can_postcopy);
     LoadStateHandler *load_state;
-    int (*load_setup)(QEMUFile *f, void *opaque);
+    int (*load_setup)(QEMUFile *f, void *opaque, Error **errp);
     int (*load_cleanup)(void *opaque);
     /* Called when postcopy migration wants to resume from failure */
     int (*resume_prepare)(MigrationState *s, void *opaque);
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 8bcb4bc73cd5ba5338e3ffa4d907d0e6bfbb9485..2dfbe671f6f45aa530c7341177bb532d8292cecd 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -580,7 +580,7 @@ static void vfio_save_state(QEMUFile *f, void *opaque)
     }
 }
 
-static int vfio_load_setup(QEMUFile *f, void *opaque)
+static int vfio_load_setup(QEMUFile *f, void *opaque, Error **errp)
 {
     VFIODevice *vbasedev = opaque;
 
diff --git a/migration/ram.c b/migration/ram.c
index 136c237f4079f68d4e578cf1c72eec2efc815bc8..8dac9bac2fe8b8c19e102c771a7ef6e976252906 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3498,7 +3498,7 @@ void colo_release_ram_cache(void)
  * @f: QEMUFile where to receive the data
  * @opaque: RAMState pointer
  */
-static int ram_load_setup(QEMUFile *f, void *opaque)
+static int ram_load_setup(QEMUFile *f, void *opaque, Error **errp)
 {
     xbzrle_load_setup();
     ramblock_recv_map_init();
diff --git a/migration/savevm.c b/migration/savevm.c
index f2ae799bad13e631bccf733a34c3a8fd22e8dd48..990f4249a26d28117ee365d8b20fc5bbca0d43d6 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2737,7 +2737,7 @@ static void qemu_loadvm_state_switchover_ack_needed(MigrationIncomingState *mis)
     trace_loadvm_state_switchover_ack_needed(mis->switchover_ack_pending_num);
 }
 
-static int qemu_loadvm_state_setup(QEMUFile *f)
+static int qemu_loadvm_state_setup(QEMUFile *f, Error **errp)
 {
     SaveStateEntry *se;
     int ret;
@@ -2753,10 +2753,11 @@ static int qemu_loadvm_state_setup(QEMUFile *f)
             }
         }
 
-        ret = se->ops->load_setup(f, se->opaque);
+        ret = se->ops->load_setup(f, se->opaque, errp);
         if (ret < 0) {
+            error_prepend(errp, "Load state of device %s failed: ",
+                          se->idstr);
             qemu_file_set_error(f, ret);
-            error_report("Load state of device %s failed", se->idstr);
             return ret;
         }
     }
@@ -2937,7 +2938,8 @@ int qemu_loadvm_state(QEMUFile *f)
         return ret;
     }
 
-    if (qemu_loadvm_state_setup(f) != 0) {
+    if (qemu_loadvm_state_setup(f, &local_err) != 0) {
+        error_report_err(local_err);
         return -EINVAL;
     }
 
-- 
2.43.0

[PATCH 03/14] memory: Add Error** argument to .log_global*() handlers

[Cédric Le Goater]

Modify memory_global_dirty_log_start() and memory_global_dirty_log_stop()
to also take an Error** parameter and report the error in the callers.
Aside from error reporting, there should be no functional changes.

Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Paul Durrant <paul@xen.org>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: David Hildenbrand <david@redhat.com>
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 include/exec/memory.h | 12 ++++++++----
 hw/i386/xen/xen-hvm.c |  8 ++++----
 hw/vfio/common.c      |  6 ++++--
 hw/virtio/vhost.c     |  4 ++--
 migration/dirtyrate.c | 24 ++++++++++++++++++++----
 migration/ram.c       | 27 +++++++++++++++++++++++----
 system/memory.c       | 37 +++++++++++++++++++++++++------------
 7 files changed, 86 insertions(+), 32 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 177be23db709d8bab9cebfe6acbae57611073327..b348070dc8f17b3505196d3a92d8cfb2171b640f 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -998,8 +998,9 @@ struct MemoryListener {
      * active at that time.
      *
      * @listener: The #MemoryListener.
+     * @errp: pointer to Error*, to store an error if it happens.
      */
-    void (*log_global_start)(MemoryListener *listener);
+    void (*log_global_start)(MemoryListener *listener, Error **errp);
 
     /**
      * @log_global_stop:
@@ -1009,8 +1010,9 @@ struct MemoryListener {
      * the address space.
      *
      * @listener: The #MemoryListener.
+     * @errp: pointer to Error*, to store an error if it happens.
      */
-    void (*log_global_stop)(MemoryListener *listener);
+    void (*log_global_stop)(MemoryListener *listener, Error **errp);
 
     /**
      * @log_global_after_sync:
@@ -2567,15 +2569,17 @@ void memory_listener_unregister(MemoryListener *listener);
  * memory_global_dirty_log_start: begin dirty logging for all regions
  *
  * @flags: purpose of starting dirty log, migration or dirty rate
+ * @errp: pointer to Error*, to store an error if it happens.
  */
-void memory_global_dirty_log_start(unsigned int flags);
+void memory_global_dirty_log_start(unsigned int flags, Error **errp);
 
 /**
  * memory_global_dirty_log_stop: end dirty logging for all regions
  *
  * @flags: purpose of stopping dirty log, migration or dirty rate
+ * @errp: pointer to Error*, to store an error if it happens.
  */
-void memory_global_dirty_log_stop(unsigned int flags);
+void memory_global_dirty_log_stop(unsigned int flags, Error **errp);
 
 void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled);
 
diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index f42621e6742552035122ea58092c91c3458338ff..d9c80416343b71311389563c7bdaa748829ada29 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -446,14 +446,14 @@ static void xen_log_sync(MemoryListener *listener, MemoryRegionSection *section)
                           int128_get64(section->size));
 }
 
-static void xen_log_global_start(MemoryListener *listener)
+static void xen_log_global_start(MemoryListener *listener, Error **errp)
 {
     if (xen_enabled()) {
         xen_in_migration = true;
     }
 }
 
-static void xen_log_global_stop(MemoryListener *listener)
+static void xen_log_global_stop(MemoryListener *listener, Error **errp)
 {
     xen_in_migration = false;
 }
@@ -653,9 +653,9 @@ void xen_hvm_modified_memory(ram_addr_t start, ram_addr_t length)
 void qmp_xen_set_global_dirty_log(bool enable, Error **errp)
 {
     if (enable) {
-        memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION);
+        memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, errp);
     } else {
-        memory_global_dirty_log_stop(GLOBAL_DIRTY_MIGRATION);
+        memory_global_dirty_log_stop(GLOBAL_DIRTY_MIGRATION, errp);
     }
 }
 
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 059bfdc07a85e2eb908df828c1f42104d683e911..45af5c675584e1931dfba3b4f78469cc4c00014e 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1075,7 +1075,8 @@ out:
     return ret;
 }
 
-static void vfio_listener_log_global_start(MemoryListener *listener)
+static void vfio_listener_log_global_start(MemoryListener *listener,
+                                           Error **errp)
 {
     VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
                                                  listener);
@@ -1094,7 +1095,8 @@ static void vfio_listener_log_global_start(MemoryListener *listener)
     }
 }
 
-static void vfio_listener_log_global_stop(MemoryListener *listener)
+static void vfio_listener_log_global_stop(MemoryListener *listener,
+                                          Error **errp)
 {
     VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
                                                  listener);
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 2c9ac794680ea9b65eba6cc22e70cf141e90aa73..970f5951cc0b2113f91a3c640e27add5752b2944 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1044,7 +1044,7 @@ check_dev_state:
     return r;
 }
 
-static void vhost_log_global_start(MemoryListener *listener)
+static void vhost_log_global_start(MemoryListener *listener, Error **errp)
 {
     int r;
 
@@ -1054,7 +1054,7 @@ static void vhost_log_global_start(MemoryListener *listener)
     }
 }
 
-static void vhost_log_global_stop(MemoryListener *listener)
+static void vhost_log_global_stop(MemoryListener *listener, Error **errp)
 {
     int r;
 
diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 1d2e85746fb7b10eb7f149976970f9a92125af8a..443acab7a7efbd6e9c94883363e1a827a3538292 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -90,13 +90,19 @@ static int64_t do_calculate_dirtyrate(DirtyPageRecord dirty_pages,
 
 void global_dirty_log_change(unsigned int flag, bool start)
 {
+    Error *local_err = NULL;
+
     bql_lock();
     if (start) {
-        memory_global_dirty_log_start(flag);
+        memory_global_dirty_log_start(flag, &local_err);
     } else {
-        memory_global_dirty_log_stop(flag);
+        memory_global_dirty_log_stop(flag, &local_err);
     }
     bql_unlock();
+
+    if (local_err) {
+        error_report_err(local_err);
+    }
 }
 
 /*
@@ -106,12 +112,18 @@ void global_dirty_log_change(unsigned int flag, bool start)
  */
 static void global_dirty_log_sync(unsigned int flag, bool one_shot)
 {
+    Error *local_err = NULL;
+
     bql_lock();
     memory_global_dirty_log_sync(false);
     if (one_shot) {
-        memory_global_dirty_log_stop(flag);
+        memory_global_dirty_log_stop(flag, &local_err);
     }
     bql_unlock();
+
+    if (local_err) {
+        error_report_err(local_err);
+    }
 }
 
 static DirtyPageRecord *vcpu_dirty_stat_alloc(VcpuStat *stat)
@@ -608,9 +620,13 @@ static void calculate_dirtyrate_dirty_bitmap(struct DirtyRateConfig config)
 {
     int64_t start_time;
     DirtyPageRecord dirty_pages;
+    Error *local_err = NULL;
 
     bql_lock();
-    memory_global_dirty_log_start(GLOBAL_DIRTY_DIRTY_RATE);
+    memory_global_dirty_log_start(GLOBAL_DIRTY_DIRTY_RATE, &local_err);
+    if (local_err) {
+        error_report_err(local_err);
+    }
 
     /*
      * 1'round of log sync may return all 1 bits with
diff --git a/migration/ram.c b/migration/ram.c
index 8dac9bac2fe8b8c19e102c771a7ef6e976252906..d86626bb1c704b2d3497b323a702ca6ca8939a79 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2391,6 +2391,7 @@ static void ram_save_cleanup(void *opaque)
 {
     RAMState **rsp = opaque;
     RAMBlock *block;
+    Error *local_err = NULL;
 
     /* We don't use dirty log with background snapshots */
     if (!migrate_background_snapshot()) {
@@ -2403,7 +2404,10 @@ static void ram_save_cleanup(void *opaque)
              * memory_global_dirty_log_stop will assert that
              * memory_global_dirty_log_start/stop used in pairs
              */
-            memory_global_dirty_log_stop(GLOBAL_DIRTY_MIGRATION);
+            memory_global_dirty_log_stop(GLOBAL_DIRTY_MIGRATION, &local_err);
+            if (local_err) {
+                error_report_err(local_err);
+            }
         }
     }
 
@@ -2800,13 +2804,18 @@ static void migration_bitmap_clear_discarded_pages(RAMState *rs)
 
 static void ram_init_bitmaps(RAMState *rs)
 {
+    Error *local_err = NULL;
+
     qemu_mutex_lock_ramlist();
 
     WITH_RCU_READ_LOCK_GUARD() {
         ram_list_init_bitmaps();
         /* We don't use dirty log with background snapshots */
         if (!migrate_background_snapshot()) {
-            memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION);
+            memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, &local_err);
+            if (local_err) {
+                error_report_err(local_err);
+            }
             migration_bitmap_sync_precopy(rs, false);
         }
     }
@@ -3450,6 +3459,8 @@ int colo_init_ram_cache(void)
 void colo_incoming_start_dirty_log(void)
 {
     RAMBlock *block = NULL;
+    Error *local_err = NULL;
+
     /* For memory_global_dirty_log_start below. */
     bql_lock();
     qemu_mutex_lock_ramlist();
@@ -3461,7 +3472,10 @@ void colo_incoming_start_dirty_log(void)
             /* Discard this dirty bitmap record */
             bitmap_zero(block->bmap, block->max_length >> TARGET_PAGE_BITS);
         }
-        memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION);
+        memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, &local_err);
+        if (local_err) {
+            error_report_err(local_err);
+        }
     }
     ram_state->migration_dirty_pages = 0;
     qemu_mutex_unlock_ramlist();
@@ -3472,8 +3486,13 @@ void colo_incoming_start_dirty_log(void)
 void colo_release_ram_cache(void)
 {
     RAMBlock *block;
+    Error *local_err = NULL;
+
+    memory_global_dirty_log_stop(GLOBAL_DIRTY_MIGRATION, &local_err);
+    if (local_err) {
+        error_report_err(local_err);
+    }
 
-    memory_global_dirty_log_stop(GLOBAL_DIRTY_MIGRATION);
     RAMBLOCK_FOREACH_NOT_IGNORED(block) {
         g_free(block->bmap);
         block->bmap = NULL;
diff --git a/system/memory.c b/system/memory.c
index a229a79988fce2aa3cb77e3a130db4c694e8cd49..2fb9ef56e7302af120f6287e1beda7a181c9a349 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -2912,18 +2912,22 @@ void memory_global_after_dirty_log_sync(void)
  */
 static unsigned int postponed_stop_flags;
 static VMChangeStateEntry *vmstate_change;
-static void memory_global_dirty_log_stop_postponed_run(void);
+static void memory_global_dirty_log_stop_postponed_run(Error **errp);
 
-void memory_global_dirty_log_start(unsigned int flags)
+void memory_global_dirty_log_start(unsigned int flags, Error **errp)
 {
     unsigned int old_flags;
+    Error *local_err = NULL;
 
     assert(flags && !(flags & (~GLOBAL_DIRTY_MASK)));
 
     if (vmstate_change) {
         /* If there is postponed stop(), operate on it first */
         postponed_stop_flags &= ~flags;
-        memory_global_dirty_log_stop_postponed_run();
+        memory_global_dirty_log_stop_postponed_run(&local_err);
+        if (local_err) {
+            error_report_err(local_err);
+        }
     }
 
     flags &= ~global_dirty_tracking;
@@ -2936,14 +2940,14 @@ void memory_global_dirty_log_start(unsigned int flags)
     trace_global_dirty_changed(global_dirty_tracking);
 
     if (!old_flags) {
-        MEMORY_LISTENER_CALL_GLOBAL(log_global_start, Forward);
+        MEMORY_LISTENER_CALL_GLOBAL(log_global_start, Forward, errp);
         memory_region_transaction_begin();
         memory_region_update_pending = true;
         memory_region_transaction_commit();
     }
 }
 
-static void memory_global_dirty_log_do_stop(unsigned int flags)
+static void memory_global_dirty_log_do_stop(unsigned int flags, Error **errp)
 {
     assert(flags && !(flags & (~GLOBAL_DIRTY_MASK)));
     assert((global_dirty_tracking & flags) == flags);
@@ -2955,7 +2959,7 @@ static void memory_global_dirty_log_do_stop(unsigned int flags)
         memory_region_transaction_begin();
         memory_region_update_pending = true;
         memory_region_transaction_commit();
-        MEMORY_LISTENER_CALL_GLOBAL(log_global_stop, Reverse);
+        MEMORY_LISTENER_CALL_GLOBAL(log_global_stop, Reverse, errp);
     }
 }
 
@@ -2963,14 +2967,14 @@ static void memory_global_dirty_log_do_stop(unsigned int flags)
  * Execute the postponed dirty log stop operations if there is, then reset
  * everything (including the flags and the vmstate change hook).
  */
-static void memory_global_dirty_log_stop_postponed_run(void)
+static void memory_global_dirty_log_stop_postponed_run(Error **errp)
 {
     /* This must be called with the vmstate handler registered */
     assert(vmstate_change);
 
     /* Note: postponed_stop_flags can be cleared in log start routine */
     if (postponed_stop_flags) {
-        memory_global_dirty_log_do_stop(postponed_stop_flags);
+        memory_global_dirty_log_do_stop(postponed_stop_flags, errp);
         postponed_stop_flags = 0;
     }
 
@@ -2981,12 +2985,17 @@ static void memory_global_dirty_log_stop_postponed_run(void)
 static void memory_vm_change_state_handler(void *opaque, bool running,
                                            RunState state)
 {
+    Error *local_err = NULL;
+
     if (running) {
-        memory_global_dirty_log_stop_postponed_run();
+        memory_global_dirty_log_stop_postponed_run(&local_err);
+        if (local_err) {
+            error_report_err(local_err);
+        }
     }
 }
 
-void memory_global_dirty_log_stop(unsigned int flags)
+void memory_global_dirty_log_stop(unsigned int flags, Error **errp)
 {
     if (!runstate_is_running()) {
         /* Postpone the dirty log stop, e.g., to when VM starts again */
@@ -3001,7 +3010,7 @@ void memory_global_dirty_log_stop(unsigned int flags)
         return;
     }
 
-    memory_global_dirty_log_do_stop(flags);
+    memory_global_dirty_log_do_stop(flags, errp);
 }
 
 static void listener_add_address_space(MemoryListener *listener,
@@ -3009,13 +3018,17 @@ static void listener_add_address_space(MemoryListener *listener,
 {
     FlatView *view;
     FlatRange *fr;
+    Error *local_err = NULL;
 
     if (listener->begin) {
         listener->begin(listener);
     }
     if (global_dirty_tracking) {
         if (listener->log_global_start) {
-            listener->log_global_start(listener);
+            listener->log_global_start(listener, &local_err);
+            if (local_err) {
+                error_report_err(local_err);
+            }
         }
     }
 
-- 
2.43.0

[PATCH 04/14] migration: Modify ram_init_bitmaps() to report dirty tracking errors

[Cédric Le Goater]

The .save_setup() handler has now an Error** argument that we can use
to propagate errors reported by the .log_global_start() handler. Do
that for the RAM. qemu_savevm_state_setup() will store the error under
the migration stream for later detection in the migration sequence.

Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 migration/ram.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index d86626bb1c704b2d3497b323a702ca6ca8939a79..b87245466bb46937fd0358d0c66432bcc6280018 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2802,19 +2802,17 @@ static void migration_bitmap_clear_discarded_pages(RAMState *rs)
     }
 }
 
-static void ram_init_bitmaps(RAMState *rs)
+static void ram_init_bitmaps(RAMState *rs, Error **errp)
 {
-    Error *local_err = NULL;
-
     qemu_mutex_lock_ramlist();
 
     WITH_RCU_READ_LOCK_GUARD() {
         ram_list_init_bitmaps();
         /* We don't use dirty log with background snapshots */
         if (!migrate_background_snapshot()) {
-            memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, &local_err);
-            if (local_err) {
-                error_report_err(local_err);
+            memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, errp);
+            if (*errp) {
+                break;
             }
             migration_bitmap_sync_precopy(rs, false);
         }
@@ -2828,7 +2826,7 @@ static void ram_init_bitmaps(RAMState *rs)
     migration_bitmap_clear_discarded_pages(rs);
 }
 
-static int ram_init_all(RAMState **rsp)
+static int ram_init_all(RAMState **rsp, Error **errp)
 {
     if (ram_state_init(rsp)) {
         return -1;
@@ -2839,7 +2837,10 @@ static int ram_init_all(RAMState **rsp)
         return -1;
     }
 
-    ram_init_bitmaps(*rsp);
+    ram_init_bitmaps(*rsp, errp);
+    if (*errp) {
+        return -1;
+    }
 
     return 0;
 }
@@ -2952,7 +2953,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque, Error **errp)
 
     /* migration has already setup the bitmap, reuse it. */
     if (!migration_in_colo_state()) {
-        if (ram_init_all(rsp) != 0) {
+        if (ram_init_all(rsp, errp) != 0) {
             compress_threads_save_cleanup();
             return -1;
         }
-- 
2.43.0

[PATCH 05/14] vfio: Add Error** argument to .set_dirty_page_tracking() handler

[Cédric Le Goater]

We will use the Error object to improve error reporting in the
.log_global*() handlers of VFIO.

Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 include/hw/vfio/vfio-container-base.h | 4 ++--
 hw/vfio/common.c                      | 4 ++--
 hw/vfio/container-base.c              | 4 ++--
 hw/vfio/container.c                   | 6 +++---
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h
index b2813b0c117985425c842d91f011bb895955d738..f22fcb5a214be2717b42815371346401bb7fce51 100644
--- a/include/hw/vfio/vfio-container-base.h
+++ b/include/hw/vfio/vfio-container-base.h
@@ -81,7 +81,7 @@ int vfio_container_add_section_window(VFIOContainerBase *bcontainer,
 void vfio_container_del_section_window(VFIOContainerBase *bcontainer,
                                        MemoryRegionSection *section);
 int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
-                                           bool start);
+                                           bool start, Error **errp);
 int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
                                       VFIOBitmap *vbmap,
                                       hwaddr iova, hwaddr size);
@@ -122,7 +122,7 @@ struct VFIOIOMMUClass {
     void (*detach_device)(VFIODevice *vbasedev);
     /* migration feature */
     int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer,
-                                   bool start);
+                                   bool start, Error **errp);
     int (*query_dirty_bitmap)(const VFIOContainerBase *bcontainer,
                               VFIOBitmap *vbmap,
                               hwaddr iova, hwaddr size);
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 45af5c675584e1931dfba3b4f78469cc4c00014e..03f2059d903eca335b02f633b07cd35ef3dd6237 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1085,7 +1085,7 @@ static void vfio_listener_log_global_start(MemoryListener *listener,
     if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
         ret = vfio_devices_dma_logging_start(bcontainer);
     } else {
-        ret = vfio_container_set_dirty_page_tracking(bcontainer, true);
+        ret = vfio_container_set_dirty_page_tracking(bcontainer, true, NULL);
     }
 
     if (ret) {
@@ -1105,7 +1105,7 @@ static void vfio_listener_log_global_stop(MemoryListener *listener,
     if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
         vfio_devices_dma_logging_stop(bcontainer);
     } else {
-        ret = vfio_container_set_dirty_page_tracking(bcontainer, false);
+        ret = vfio_container_set_dirty_page_tracking(bcontainer, false, NULL);
     }
 
     if (ret) {
diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
index 913ae49077c4f09b7b27517c1231cfbe4befb7fb..7c0764121d24b02b6c4e66e368d7dff78a6d65aa 100644
--- a/hw/vfio/container-base.c
+++ b/hw/vfio/container-base.c
@@ -53,14 +53,14 @@ void vfio_container_del_section_window(VFIOContainerBase *bcontainer,
 }
 
 int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
-                                           bool start)
+                                           bool start, Error **errp)
 {
     if (!bcontainer->dirty_pages_supported) {
         return 0;
     }
 
     g_assert(bcontainer->ops->set_dirty_page_tracking);
-    return bcontainer->ops->set_dirty_page_tracking(bcontainer, start);
+    return bcontainer->ops->set_dirty_page_tracking(bcontainer, start, errp);
 }
 
 int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index bd25b9fbad2e717e63c2ab0e331186e5f63cef49..f772ac79b9c413c86d7e60f6dc4e6699852d5aac 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -210,7 +210,7 @@ static int vfio_legacy_dma_map(const VFIOContainerBase *bcontainer, hwaddr iova,
 
 static int
 vfio_legacy_set_dirty_page_tracking(const VFIOContainerBase *bcontainer,
-                                    bool start)
+                                    bool start, Error **errp)
 {
     const VFIOContainer *container = container_of(bcontainer, VFIOContainer,
                                                   bcontainer);
@@ -228,8 +228,8 @@ vfio_legacy_set_dirty_page_tracking(const VFIOContainerBase *bcontainer,
     ret = ioctl(container->fd, VFIO_IOMMU_DIRTY_PAGES, &dirty);
     if (ret) {
         ret = -errno;
-        error_report("Failed to set dirty tracking flag 0x%x errno: %d",
-                     dirty.flags, errno);
+        error_setg(errp, "Failed to set dirty tracking flag 0x%x errno: %d",
+                   dirty.flags, errno);
     }
 
     return ret;
-- 
2.43.0

[PATCH 06/14] vfio: Add Error** argument to vfio_devices_dma_logging_start()

[Cédric Le Goater]

This allows to update the Error argument of the VFIO log_global_start()
handler. Errors detected when device level logging is started will be
propagated up to qemu_savevm_state_setup() when the ram save_setup()
handler is executed.

The vfio_set_migration_error() call becomes redudant. Remove it.

Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 hw/vfio/common.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 03f2059d903eca335b02f633b07cd35ef3dd6237..a5d53e67efaa921e89ad918390a22506c7b1ed66 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1036,7 +1036,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;
@@ -1058,8 +1059,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(errp, "%s: Failed to start DMA logging, err %d (%s)",
+                       vbasedev->name, ret, strerror(errno));
             goto out;
         }
         vbasedev->dirty_tracking = true;
@@ -1083,15 +1084,13 @@ static void vfio_listener_log_global_start(MemoryListener *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 - ");
     }
 }
 
@@ -1105,13 +1104,11 @@ static void vfio_listener_log_global_stop(MemoryListener *listener,
     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, errp);
     }
 
     if (ret) {
-        error_report("vfio: Could not stop dirty page tracking, err: %d (%s)",
-                     ret, strerror(-ret));
-        vfio_set_migration_error(ret);
+        error_prepend(errp, "vfio: Could not stop dirty page tracking - ");
     }
 }
 
-- 
2.43.0

[PATCH 07/14] vfio: Add Error** argument to vfio_devices_dma_logging_stop()

[Cédric Le Goater]

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

Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 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 a5d53e67efaa921e89ad918390a22506c7b1ed66..82173b039c47150f5edd05d329192c5b9c8a9a0f 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(errp, "%s: Failed to stop DMA logging, err %d (%s)",
+                           vbasedev->name, -errno, strerror(errno));
+            }
         }
         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);
@@ -1102,7 +1111,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, errp);
     } else {
         ret = vfio_container_set_dirty_page_tracking(bcontainer, false, errp);
     }
-- 
2.43.0

[PATCH 08/14] vfio: Use new Error** argument in vfio_save_setup()

[Cédric Le Goater]

Add an Error** argument to vfio_migration_set_state() and adjust
callers, including vfio_save_setup(). The error will be propagated up
to qemu_savevm_state_setup() where the save_setup() handler is
executed.

Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 hw/vfio/migration.c | 62 +++++++++++++++++++++++++++++----------------
 1 file changed, 40 insertions(+), 22 deletions(-)

diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 2dfbe671f6f45aa530c7341177bb532d8292cecd..2e0a79967cc97f44d9be5575c3cfe18c9f349dab 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -84,7 +84,8 @@ static const char *mig_state_to_str(enum vfio_device_mig_state state)
 
 static int vfio_migration_set_state(VFIODevice *vbasedev,
                                     enum vfio_device_mig_state new_state,
-                                    enum vfio_device_mig_state recover_state)
+                                    enum vfio_device_mig_state recover_state,
+                                    Error **errp)
 {
     VFIOMigration *migration = vbasedev->migration;
     uint64_t buf[DIV_ROUND_UP(sizeof(struct vfio_device_feature) +
@@ -104,15 +105,15 @@ static int vfio_migration_set_state(VFIODevice *vbasedev,
         ret = -errno;
 
         if (recover_state == VFIO_DEVICE_STATE_ERROR) {
-            error_report("%s: Failed setting device state to %s, err: %s. "
-                         "Recover state is ERROR. Resetting device",
-                         vbasedev->name, mig_state_to_str(new_state),
-                         strerror(errno));
+            error_setg(errp, "%s: Failed setting device state to %s, err: %s. "
+                       "Recover state is ERROR. Resetting device",
+                       vbasedev->name, mig_state_to_str(new_state),
+                       strerror(errno));
 
             goto reset_device;
         }
 
-        error_report(
+        error_setg(errp,
             "%s: Failed setting device state to %s, err: %s. Setting device in recover state %s",
                      vbasedev->name, mig_state_to_str(new_state),
                      strerror(errno), mig_state_to_str(recover_state));
@@ -120,7 +121,7 @@ static int vfio_migration_set_state(VFIODevice *vbasedev,
         mig_state->device_state = recover_state;
         if (ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature)) {
             ret = -errno;
-            error_report(
+            error_setg(errp,
                 "%s: Failed setting device in recover state, err: %s. Resetting device",
                          vbasedev->name, strerror(errno));
 
@@ -139,7 +140,7 @@ static int vfio_migration_set_state(VFIODevice *vbasedev,
              * This can happen if the device is asynchronously reset and
              * terminates a data transfer.
              */
-            error_report("%s: data_fd out of sync", vbasedev->name);
+            error_setg(errp, "%s: data_fd out of sync", vbasedev->name);
             close(mig_state->data_fd);
 
             return -EBADF;
@@ -170,10 +171,11 @@ reset_device:
  */
 static int
 vfio_migration_set_state_or_reset(VFIODevice *vbasedev,
-                                  enum vfio_device_mig_state new_state)
+                                  enum vfio_device_mig_state new_state,
+                                  Error **errp)
 {
     return vfio_migration_set_state(vbasedev, new_state,
-                                    VFIO_DEVICE_STATE_ERROR);
+                                    VFIO_DEVICE_STATE_ERROR, errp);
 }
 
 static int vfio_load_buffer(QEMUFile *f, VFIODevice *vbasedev,
@@ -391,8 +393,8 @@ static int vfio_save_setup(QEMUFile *f, void *opaque, Error **errp)
                                       stop_copy_size);
     migration->data_buffer = g_try_malloc0(migration->data_buffer_size);
     if (!migration->data_buffer) {
-        error_report("%s: Failed to allocate migration data buffer",
-                     vbasedev->name);
+        error_setg(errp, "%s: Failed to allocate migration data buffer",
+                   vbasedev->name);
         return -ENOMEM;
     }
 
@@ -402,7 +404,7 @@ static int vfio_save_setup(QEMUFile *f, void *opaque, Error **errp)
         switch (migration->device_state) {
         case VFIO_DEVICE_STATE_RUNNING:
             ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_PRE_COPY,
-                                           VFIO_DEVICE_STATE_RUNNING);
+                                           VFIO_DEVICE_STATE_RUNNING, errp);
             if (ret) {
                 return ret;
             }
@@ -429,13 +431,18 @@ static void vfio_save_cleanup(void *opaque)
 {
     VFIODevice *vbasedev = opaque;
     VFIOMigration *migration = vbasedev->migration;
+    Error *local_err = NULL;
 
     /*
      * Changing device state from STOP_COPY to STOP can take time. Do it here,
      * after migration has completed, so it won't increase downtime.
      */
     if (migration->device_state == VFIO_DEVICE_STATE_STOP_COPY) {
-        vfio_migration_set_state_or_reset(vbasedev, VFIO_DEVICE_STATE_STOP);
+        vfio_migration_set_state_or_reset(vbasedev, VFIO_DEVICE_STATE_STOP,
+                                          &local_err);
+        if (local_err) {
+            error_report_err(local_err);
+        }
     }
 
     g_free(migration->data_buffer);
@@ -541,11 +548,13 @@ static int vfio_save_complete_precopy(QEMUFile *f, void *opaque)
     VFIODevice *vbasedev = opaque;
     ssize_t data_size;
     int ret;
+    Error *local_err = NULL;
 
     /* We reach here with device state STOP or STOP_COPY only */
     ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP_COPY,
-                                   VFIO_DEVICE_STATE_STOP);
-    if (ret) {
+                                   VFIO_DEVICE_STATE_STOP, &local_err);
+    if (local_err) {
+        error_report_err(local_err);
         return ret;
     }
 
@@ -585,7 +594,7 @@ static int vfio_load_setup(QEMUFile *f, void *opaque, Error **errp)
     VFIODevice *vbasedev = opaque;
 
     return vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_RESUMING,
-                                   vbasedev->migration->device_state);
+                                    vbasedev->migration->device_state, errp);
 }
 
 static int vfio_load_cleanup(void *opaque)
@@ -701,20 +710,22 @@ static void vfio_vmstate_change_prepare(void *opaque, bool running,
     VFIODevice *vbasedev = opaque;
     VFIOMigration *migration = vbasedev->migration;
     enum vfio_device_mig_state new_state;
+    Error *local_err = NULL;
     int ret;
 
     new_state = migration->device_state == VFIO_DEVICE_STATE_PRE_COPY ?
                     VFIO_DEVICE_STATE_PRE_COPY_P2P :
                     VFIO_DEVICE_STATE_RUNNING_P2P;
 
-    ret = vfio_migration_set_state_or_reset(vbasedev, new_state);
+    ret = vfio_migration_set_state_or_reset(vbasedev, new_state, &local_err);
     if (ret) {
         /*
          * Migration should be aborted in this case, but vm_state_notify()
          * currently does not support reporting failures.
          */
         if (migrate_get_current()->to_dst_file) {
-            qemu_file_set_error(migrate_get_current()->to_dst_file, ret);
+            qemu_file_set_error_obj(migrate_get_current()->to_dst_file, ret,
+                                    local_err);
         }
     }
 
@@ -727,6 +738,7 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
 {
     VFIODevice *vbasedev = opaque;
     enum vfio_device_mig_state new_state;
+    Error *local_err = NULL;
     int ret;
 
     if (running) {
@@ -739,14 +751,15 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
                 VFIO_DEVICE_STATE_STOP;
     }
 
-    ret = vfio_migration_set_state_or_reset(vbasedev, new_state);
+    ret = vfio_migration_set_state_or_reset(vbasedev, new_state, &local_err);
     if (ret) {
         /*
          * Migration should be aborted in this case, but vm_state_notify()
          * currently does not support reporting failures.
          */
         if (migrate_get_current()->to_dst_file) {
-            qemu_file_set_error(migrate_get_current()->to_dst_file, ret);
+            qemu_file_set_error_obj(migrate_get_current()->to_dst_file, ret,
+                                    local_err);
         }
     }
 
@@ -760,6 +773,7 @@ static void vfio_migration_state_notifier(Notifier *notifier, void *data)
     VFIOMigration *migration = container_of(notifier, VFIOMigration,
                                             migration_state);
     VFIODevice *vbasedev = migration->vbasedev;
+    Error *local_err = NULL;
 
     trace_vfio_migration_state_notifier(vbasedev->name,
                                         MigrationStatus_str(s->state));
@@ -768,7 +782,11 @@ static void vfio_migration_state_notifier(Notifier *notifier, void *data)
     case MIGRATION_STATUS_CANCELLING:
     case MIGRATION_STATUS_CANCELLED:
     case MIGRATION_STATUS_FAILED:
-        vfio_migration_set_state_or_reset(vbasedev, VFIO_DEVICE_STATE_RUNNING);
+        vfio_migration_set_state_or_reset(vbasedev, VFIO_DEVICE_STATE_RUNNING,
+                                          &local_err);
+        if (local_err) {
+            error_report_err(local_err);
+        }
     }
 }
 
-- 
2.43.0

[PATCH 09/14] vfio: Add Error** argument to .vfio_save_config() handler

[Cédric Le Goater]

Use vmstate_save_state_with_err() to improve error reporting in the
callers.

Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 include/hw/vfio/vfio-common.h |  2 +-
 hw/vfio/migration.c           | 18 ++++++++++++------
 hw/vfio/pci.c                 |  5 +++--
 3 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 9b7ef7d02b5a0ad5266bcc4d06cd6874178978e4..710e0d6a880b97848af6ddc2e7968a01054fa122 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -133,7 +133,7 @@ struct VFIODeviceOps {
     int (*vfio_hot_reset_multi)(VFIODevice *vdev);
     void (*vfio_eoi)(VFIODevice *vdev);
     Object *(*vfio_get_object)(VFIODevice *vdev);
-    void (*vfio_save_config)(VFIODevice *vdev, QEMUFile *f);
+    int (*vfio_save_config)(VFIODevice *vdev, QEMUFile *f, Error **errp);
     int (*vfio_load_config)(VFIODevice *vdev, QEMUFile *f);
 };
 
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 2e0a79967cc97f44d9be5575c3cfe18c9f349dab..fb264c1ef57bbbde4306901e5449e0dfbd0ce3b7 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -190,14 +190,19 @@ static int vfio_load_buffer(QEMUFile *f, VFIODevice *vbasedev,
     return ret;
 }
 
-static int vfio_save_device_config_state(QEMUFile *f, void *opaque)
+static int vfio_save_device_config_state(QEMUFile *f, void *opaque,
+                                         Error **errp)
 {
     VFIODevice *vbasedev = opaque;
+    int ret = 0;
 
     qemu_put_be64(f, VFIO_MIG_FLAG_DEV_CONFIG_STATE);
 
     if (vbasedev->ops && vbasedev->ops->vfio_save_config) {
-        vbasedev->ops->vfio_save_config(vbasedev, f);
+        ret = vbasedev->ops->vfio_save_config(vbasedev, f, errp);
+        if (ret) {
+            return ret;
+        }
     }
 
     qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
@@ -579,13 +584,14 @@ static int vfio_save_complete_precopy(QEMUFile *f, void *opaque)
 static void vfio_save_state(QEMUFile *f, void *opaque)
 {
     VFIODevice *vbasedev = opaque;
+    Error *local_err = NULL;
     int ret;
 
-    ret = vfio_save_device_config_state(f, opaque);
+    ret = vfio_save_device_config_state(f, opaque, &local_err);
     if (ret) {
-        error_report("%s: Failed to save device config space",
-                     vbasedev->name);
-        qemu_file_set_error(f, ret);
+        error_prepend(&local_err, "%s: Failed to save device config space",
+                      vbasedev->name);
+        qemu_file_set_error_obj(f, ret, local_err);
     }
 }
 
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 4fa387f0430d62ca2ba1b5ae5b7037f8f06b33f9..99d86e1d40ef25133fc76ad6e58294b07bd20843 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2585,11 +2585,12 @@ const VMStateDescription vmstate_vfio_pci_config = {
     }
 };
 
-static void vfio_pci_save_config(VFIODevice *vbasedev, QEMUFile *f)
+static int vfio_pci_save_config(VFIODevice *vbasedev, QEMUFile *f, Error **errp)
 {
     VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
 
-    vmstate_save_state(f, &vmstate_vfio_pci_config, vdev, NULL);
+    return vmstate_save_state_with_err(f, &vmstate_vfio_pci_config, vdev, NULL,
+                                       errp);
 }
 
 static int vfio_pci_load_config(VFIODevice *vbasedev, QEMUFile *f)
-- 
2.43.0

[PATCH 10/14] vfio: Also trace event failures in vfio_save_complete_precopy()

[Cédric Le Goater]

vfio_save_complete_precopy() currently returns before doing the trace
event. Change that.

Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 hw/vfio/migration.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index fb264c1ef57bbbde4306901e5449e0dfbd0ce3b7..cc5b74f9563eca25d3c7285f106ed06f1eb2f519 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -572,9 +572,6 @@ static int vfio_save_complete_precopy(QEMUFile *f, void *opaque)
 
     qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
     ret = qemu_file_get_error(f);
-    if (ret) {
-        return ret;
-    }
 
     trace_vfio_save_complete_precopy(vbasedev->name, ret);
 
-- 
2.43.0

[PATCH 11/14] vfio: Extend vfio_set_migration_error() with Error* argument

[Cédric Le Goater]

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.

Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 hw/vfio/common.c | 50 +++++++++++++++++++++++++++++-------------------
 1 file changed, 30 insertions(+), 20 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 82173b039c47150f5edd05d329192c5b9c8a9a0f..afe8b6bd294fd5904f394a5db48aae3fd718b14e 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -148,16 +148,18 @@ 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)
 {
     MigrationState *ms = migrate_get_current();
 
     if (migration_is_setup_or_active(ms->state)) {
         WITH_QEMU_LOCK_GUARD(&ms->qemu_file_lock) {
             if (ms->to_dst_file) {
-                qemu_file_set_error(ms->to_dst_file, err);
+                qemu_file_set_error_obj(ms->to_dst_file, ret, err);
             }
         }
+    } else {
+        error_report_err(err);
     }
 }
 
@@ -296,15 +298,17 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
     VFIOContainerBase *bcontainer = giommu->bcontainer;
     hwaddr iova = iotlb->iova + giommu->iommu_offset;
     void *vaddr;
+    Error *local_err = NULL;
     int ret;
 
     trace_vfio_iommu_map_notify(iotlb->perm == IOMMU_NONE ? "UNMAP" : "MAP",
                                 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;
     }
 
@@ -336,11 +340,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));
+            vfio_set_migration_error(ret, local_err);
         }
     }
 out:
@@ -1224,13 +1229,15 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
     VFIOContainerBase *bcontainer = giommu->bcontainer;
     hwaddr iova = iotlb->iova + giommu->iommu_offset;
     ram_addr_t translated_addr;
+    Error *local_err = NULL;
     int ret = -EINVAL;
 
     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;
     }
 
@@ -1239,17 +1246,18 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
         ret = vfio_get_dirty_bitmap(bcontainer, iova, iotlb->addr_mask + 1,
                                     translated_addr);
         if (ret) {
-            error_report("vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", "
-                         "0x%"HWADDR_PRIx") = %d (%s)",
-                         bcontainer, iova, iotlb->addr_mask + 1, ret,
-                         strerror(-ret));
+            error_setg(&local_err,
+                       "vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", "
+                       "0x%"HWADDR_PRIx") = %d (%s)",
+                       bcontainer, iova, iotlb->addr_mask + 1, ret,
+                       strerror(-ret));
         }
     }
     rcu_read_unlock();
 
 out:
     if (ret) {
-        vfio_set_migration_error(ret);
+        vfio_set_migration_error(ret, local_err);
     }
 }
 
@@ -1345,6 +1353,7 @@ static void vfio_listener_log_sync(MemoryListener *listener,
 {
     VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
                                                  listener);
+    Error *local_err = NULL;
     int ret;
 
     if (vfio_listener_skipped_section(section)) {
@@ -1354,9 +1363,10 @@ static void vfio_listener_log_sync(MemoryListener *listener,
     if (vfio_devices_all_dirty_tracking(bcontainer)) {
         ret = vfio_sync_dirty_bitmap(bcontainer, section);
         if (ret) {
-            error_report("vfio: Failed to sync dirty bitmap, err: %d (%s)", ret,
-                         strerror(-ret));
-            vfio_set_migration_error(ret);
+            error_setg(&local_err,
+                       "vfio: Failed to sync dirty bitmap, err: %d (%s)", ret,
+                       strerror(-ret));
+            vfio_set_migration_error(ret, local_err);
         }
     }
 }
-- 
2.43.0

[PATCH 12/14] migration: Report error when shutdown fails

[Cédric Le Goater]

This will help detect issues regarding I/O channels usage.

Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 migration/qemu-file.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 94231ff2955c80b3d0fab11a40510d34c334a826..b69e0c62e2fcf21d346a3687df7eebee23791fdc 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -62,6 +62,8 @@ struct QEMUFile {
  */
 int qemu_file_shutdown(QEMUFile *f)
 {
+    Error *err = NULL;
+
     /*
      * We must set qemufile error before the real shutdown(), otherwise
      * there can be a race window where we thought IO all went though
@@ -90,7 +92,8 @@ int qemu_file_shutdown(QEMUFile *f)
         return -ENOSYS;
     }
 
-    if (qio_channel_shutdown(f->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL) < 0) {
+    if (qio_channel_shutdown(f->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, &err) < 0) {
+        error_report_err(err);
         return -EIO;
     }
 
-- 
2.43.0

[PATCH 13/14] migration: Use migrate_has_error() in close_return_path_on_source()

[Cédric Le Goater]

close_return_path_on_source() retrieves the migration error from the
the QEMUFile '->to_dst_file' to know if a shutdown is required. This
shutdown is required to exit the return-path thread. However, in
migrate_fd_cleanup(), '->to_dst_file' is cleaned up before calling
close_return_path_on_source() and the shutdown is never performed,
leaving the source and destination waiting for an event to occur.

Avoid relying on '->to_dst_file' and use migrate_has_error() instead.

Suggested-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 migration/migration.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index d5f705ceef4c925589aa49335969672c0d761fa2..5f55af3d7624750ca416c4177781241b3e291e5d 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2372,8 +2372,7 @@ static bool close_return_path_on_source(MigrationState *ms)
      * cause it to unblock if it's stuck waiting for the destination.
      */
     WITH_QEMU_LOCK_GUARD(&ms->qemu_file_lock) {
-        if (ms->to_dst_file && ms->rp_state.from_dst_file &&
-            qemu_file_get_error(ms->to_dst_file)) {
+        if (migrate_has_error(ms) && ms->rp_state.from_dst_file) {
             qemu_file_shutdown(ms->rp_state.from_dst_file);
         }
     }
-- 
2.43.0

[RFC PATCH 14/14] migration: Fix return-path thread exit

[Cédric Le Goater]

In case of error, close_return_path_on_source() can perform a shutdown
to exit the return-path thread.  However, in migrate_fd_cleanup(),
'to_dst_file' is closed before calling close_return_path_on_source()
and the shutdown fails, leaving the source and destination waiting for
an event to occur.

Close the file after calling close_return_path_on_source() so that the
shutdown succeeds and the return-path thread exits.

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

 This is an RFC because the correct fix implies reworking the QEMUFile
 construct, built on top of the QEMU I/O channel.

 migration/migration.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 5f55af3d7624750ca416c4177781241b3e291e5d..de329f2c553288935d824748286e79e535929b8b 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1313,6 +1313,8 @@ void migrate_set_state(int *state, int old_state, int new_state)
 
 static void migrate_fd_cleanup(MigrationState *s)
 {
+    QEMUFile *tmp = NULL;
+
     g_free(s->hostname);
     s->hostname = NULL;
     json_writer_free(s->vmdesc);
@@ -1321,8 +1323,6 @@ static void migrate_fd_cleanup(MigrationState *s)
     qemu_savevm_state_cleanup();
 
     if (s->to_dst_file) {
-        QEMUFile *tmp;
-
         trace_migrate_fd_cleanup();
         bql_unlock();
         if (s->migration_thread_running) {
@@ -1341,15 +1341,14 @@ static void migrate_fd_cleanup(MigrationState *s)
          * critical section won't block for long.
          */
         migration_ioc_unregister_yank_from_file(tmp);
-        qemu_fclose(tmp);
     }
 
-    /*
-     * We already cleaned up to_dst_file, so errors from the return
-     * path might be due to that, ignore them.
-     */
     close_return_path_on_source(s);
 
+    if (tmp) {
+        qemu_fclose(tmp);
+    }
+
     assert(!migration_is_active(s));
 
     if (s->state == MIGRATION_STATUS_CANCELLING) {
-- 
2.43.0

Re: [PATCH 01/14] migration: Add Error** argument to .save_setup() handler

[Philippe Mathieu-Daudé]

On 7/2/24 14:33, Cédric Le Goater wrote:
> The purpose is to record a potential error in the migration stream if
> qemu_savevm_state_setup() fails. Most of the current .save_setup()
> handlers can be modified to use the Error argument instead of managing
> their own and calling locally error_report(). The following patches
> will introduce such changes for VFIO first.
> 
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>   include/migration/register.h   | 2 +-
>   hw/ppc/spapr.c                 | 2 +-
>   hw/s390x/s390-stattrib.c       | 2 +-
>   hw/vfio/migration.c            | 2 +-
>   migration/block-dirty-bitmap.c | 2 +-
>   migration/block.c              | 2 +-
>   migration/ram.c                | 2 +-
>   migration/savevm.c             | 4 ++--
>   8 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/include/migration/register.h b/include/migration/register.h
> index 9ab1f79512c605f0c88a45b560c57486fa054441..831600a00eae4efd0464b60925d65de4d9dbcff8 100644
> --- a/include/migration/register.h
> +++ b/include/migration/register.h
> @@ -25,7 +25,7 @@ typedef struct SaveVMHandlers {
>        * used to perform early checks.
>        */
>       int (*save_prepare)(void *opaque, Error **errp);
> -    int (*save_setup)(QEMUFile *f, void *opaque);
> +    int (*save_setup)(QEMUFile *f, void *opaque, Error **errp);

Since you change this, do you mind adding a docstring
describing this prototype?

Otherwise,
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 02/14] migration: Add Error** argument to .load_setup() handler

[Philippe Mathieu-Daudé]

On 7/2/24 14:33, Cédric Le Goater wrote:
> This will be useful to report errors at a higher level, mostly in VFIO
> today.
> 
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>   include/migration/register.h |  2 +-
>   hw/vfio/migration.c          |  2 +-
>   migration/ram.c              |  2 +-
>   migration/savevm.c           | 10 ++++++----
>   4 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/include/migration/register.h b/include/migration/register.h
> index 831600a00eae4efd0464b60925d65de4d9dbcff8..e6bc226c98b27c1fb0f9e2b56d8aff491aa14d65 100644
> --- a/include/migration/register.h
> +++ b/include/migration/register.h
> @@ -72,7 +72,7 @@ typedef struct SaveVMHandlers {
>       void (*state_pending_exact)(void *opaque, uint64_t *must_precopy,
>                                   uint64_t *can_postcopy);
>       LoadStateHandler *load_state;
> -    int (*load_setup)(QEMUFile *f, void *opaque);
> +    int (*load_setup)(QEMUFile *f, void *opaque, Error **errp);

Please document this prototype. Otherwise:

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 04/14] migration: Modify ram_init_bitmaps() to report dirty tracking errors

[Philippe Mathieu-Daudé]

On 7/2/24 14:33, Cédric Le Goater wrote:
> The .save_setup() handler has now an Error** argument that we can use
> to propagate errors reported by the .log_global_start() handler. Do
> that for the RAM. qemu_savevm_state_setup() will store the error under
> the migration stream for later detection in the migration sequence.
> 
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>   migration/ram.c | 19 ++++++++++---------
>   1 file changed, 10 insertions(+), 9 deletions(-)


> -static void ram_init_bitmaps(RAMState *rs)
> +static void ram_init_bitmaps(RAMState *rs, Error **errp)

Please return a boolean.

>   {
> -    Error *local_err = NULL;
> -
>       qemu_mutex_lock_ramlist();
>   
>       WITH_RCU_READ_LOCK_GUARD() {
>           ram_list_init_bitmaps();
>           /* We don't use dirty log with background snapshots */
>           if (!migrate_background_snapshot()) {
> -            memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, &local_err);
> -            if (local_err) {
> -                error_report_err(local_err);
> +            memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, errp);
> +            if (*errp) {
> +                break;
>               }
>               migration_bitmap_sync_precopy(rs, false);
>           }
> @@ -2828,7 +2826,7 @@ static void ram_init_bitmaps(RAMState *rs)
>       migration_bitmap_clear_discarded_pages(rs);
>   }

Re: [PATCH 05/14] vfio: Add Error** argument to .set_dirty_page_tracking() handler

[Philippe Mathieu-Daudé]

On 7/2/24 14:33, Cédric Le Goater wrote:
> We will use the Error object to improve error reporting in the
> .log_global*() handlers of VFIO.
> 
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>   include/hw/vfio/vfio-container-base.h | 4 ++--
>   hw/vfio/common.c                      | 4 ++--
>   hw/vfio/container-base.c              | 4 ++--
>   hw/vfio/container.c                   | 6 +++---
>   4 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h
> index b2813b0c117985425c842d91f011bb895955d738..f22fcb5a214be2717b42815371346401bb7fce51 100644
> --- a/include/hw/vfio/vfio-container-base.h
> +++ b/include/hw/vfio/vfio-container-base.h
> @@ -81,7 +81,7 @@ int vfio_container_add_section_window(VFIOContainerBase *bcontainer,
>   void vfio_container_del_section_window(VFIOContainerBase *bcontainer,
>                                          MemoryRegionSection *section);
>   int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
> -                                           bool start);
> +                                           bool start, Error **errp);

Since here, please document modified prototypes, otherwise:

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

>   int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
>                                         VFIOBitmap *vbmap,
>                                         hwaddr iova, hwaddr size);
> @@ -122,7 +122,7 @@ struct VFIOIOMMUClass {
>       void (*detach_device)(VFIODevice *vbasedev);
>       /* migration feature */
>       int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer,
> -                                   bool start);
> +                                   bool start, Error **errp);
>       int (*query_dirty_bitmap)(const VFIOContainerBase *bcontainer,
>                                 VFIOBitmap *vbmap,
>                                 hwaddr iova, hwaddr size);

Re: [PATCH 06/14] vfio: Add Error** argument to vfio_devices_dma_logging_start()

[Philippe Mathieu-Daudé]

On 7/2/24 14:33, Cédric Le Goater wrote:
> This allows to update the Error argument of the VFIO log_global_start()
> handler. Errors detected when device level logging is started will be
> propagated up to qemu_savevm_state_setup() when the ram save_setup()
> handler is executed.
> 
> The vfio_set_migration_error() call becomes redudant. Remove it.

Typo "redundant".

> 
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>   hw/vfio/common.c | 21 +++++++++------------
>   1 file changed, 9 insertions(+), 12 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 07/14] vfio: Add Error** argument to vfio_devices_dma_logging_stop()

[Philippe Mathieu-Daudé]

On 7/2/24 14:33, Cédric Le Goater wrote:
> This improves error reporting in the log_global_stop() VFIO handler.
> 
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>   hw/vfio/common.c | 19 ++++++++++++++-----
>   1 file changed, 14 insertions(+), 5 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 08/14] vfio: Use new Error** argument in vfio_save_setup()

[Philippe Mathieu-Daudé]

On 7/2/24 14:33, Cédric Le Goater wrote:
> Add an Error** argument to vfio_migration_set_state() and adjust
> callers, including vfio_save_setup(). The error will be propagated up
> to qemu_savevm_state_setup() where the save_setup() handler is
> executed.
> 
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>   hw/vfio/migration.c | 62 +++++++++++++++++++++++++++++----------------
>   1 file changed, 40 insertions(+), 22 deletions(-)


> @@ -429,13 +431,18 @@ static void vfio_save_cleanup(void *opaque)
>   {
>       VFIODevice *vbasedev = opaque;
>       VFIOMigration *migration = vbasedev->migration;
> +    Error *local_err = NULL;
>   
>       /*
>        * Changing device state from STOP_COPY to STOP can take time. Do it here,
>        * after migration has completed, so it won't increase downtime.
>        */
>       if (migration->device_state == VFIO_DEVICE_STATE_STOP_COPY) {
> -        vfio_migration_set_state_or_reset(vbasedev, VFIO_DEVICE_STATE_STOP);
> +        vfio_migration_set_state_or_reset(vbasedev, VFIO_DEVICE_STATE_STOP,
> +                                          &local_err);
> +        if (local_err) {

Please check callee return value instead.

> +            error_report_err(local_err);
> +        }
>       }
>   
>       g_free(migration->data_buffer);
> @@ -541,11 +548,13 @@ static int vfio_save_complete_precopy(QEMUFile *f, void *opaque)
>       VFIODevice *vbasedev = opaque;
>       ssize_t data_size;
>       int ret;
> +    Error *local_err = NULL;
>   
>       /* We reach here with device state STOP or STOP_COPY only */
>       ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP_COPY,
> -                                   VFIO_DEVICE_STATE_STOP);
> -    if (ret) {
> +                                   VFIO_DEVICE_STATE_STOP, &local_err);
> +    if (local_err) {

Ditto.

> +        error_report_err(local_err);
>           return ret;
>       }


> @@ -760,6 +773,7 @@ static void vfio_migration_state_notifier(Notifier *notifier, void *data)
>       VFIOMigration *migration = container_of(notifier, VFIOMigration,
>                                               migration_state);
>       VFIODevice *vbasedev = migration->vbasedev;
> +    Error *local_err = NULL;
>   
>       trace_vfio_migration_state_notifier(vbasedev->name,
>                                           MigrationStatus_str(s->state));
> @@ -768,7 +782,11 @@ static void vfio_migration_state_notifier(Notifier *notifier, void *data)
>       case MIGRATION_STATUS_CANCELLING:
>       case MIGRATION_STATUS_CANCELLED:
>       case MIGRATION_STATUS_FAILED:
> -        vfio_migration_set_state_or_reset(vbasedev, VFIO_DEVICE_STATE_RUNNING);
> +        vfio_migration_set_state_or_reset(vbasedev, VFIO_DEVICE_STATE_RUNNING,
> +                                          &local_err);
> +        if (local_err) {

Ditto.

> +            error_report_err(local_err);
> +        }
>       }
>   }
>   

Re: [PATCH 09/14] vfio: Add Error** argument to .vfio_save_config() handler

[Philippe Mathieu-Daudé]

On 7/2/24 14:33, Cédric Le Goater wrote:
> Use vmstate_save_state_with_err() to improve error reporting in the
> callers.
> 
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>   include/hw/vfio/vfio-common.h |  2 +-
>   hw/vfio/migration.c           | 18 ++++++++++++------
>   hw/vfio/pci.c                 |  5 +++--
>   3 files changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 9b7ef7d02b5a0ad5266bcc4d06cd6874178978e4..710e0d6a880b97848af6ddc2e7968a01054fa122 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -133,7 +133,7 @@ struct VFIODeviceOps {
>       int (*vfio_hot_reset_multi)(VFIODevice *vdev);
>       void (*vfio_eoi)(VFIODevice *vdev);
>       Object *(*vfio_get_object)(VFIODevice *vdev);
> -    void (*vfio_save_config)(VFIODevice *vdev, QEMUFile *f);
> +    int (*vfio_save_config)(VFIODevice *vdev, QEMUFile *f, Error **errp);

Worth a one-line docstring?

Otherwise,
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

>       int (*vfio_load_config)(VFIODevice *vdev, QEMUFile *f);
>   };

Re: [PATCH 11/14] vfio: Extend vfio_set_migration_error() with Error* argument

[Philippe Mathieu-Daudé]

On 7/2/24 14:33, Cédric Le Goater wrote:
> 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.
> 
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>   hw/vfio/common.c | 50 +++++++++++++++++++++++++++++-------------------
>   1 file changed, 30 insertions(+), 20 deletions(-)


> -static void vfio_set_migration_error(int err)
> +static void vfio_set_migration_error(int ret, Error *err)

Maybe rename vfio_report_migration_error() for clarity?

>   {
>       MigrationState *ms = migrate_get_current();
>   
>       if (migration_is_setup_or_active(ms->state)) {
>           WITH_QEMU_LOCK_GUARD(&ms->qemu_file_lock) {
>               if (ms->to_dst_file) {
> -                qemu_file_set_error(ms->to_dst_file, err);
> +                qemu_file_set_error_obj(ms->to_dst_file, ret, err);
>               }
>           }
> +    } else {
> +        error_report_err(err);
>       }
>   }

Re: [PATCH 12/14] migration: Report error when shutdown fails

[Philippe Mathieu-Daudé]

On 7/2/24 14:33, Cédric Le Goater wrote:
> This will help detect issues regarding I/O channels usage.

English isn't my native language but I'd expect "detecting" here.

> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>   migration/qemu-file.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 01/14] migration: Add Error** argument to .save_setup() handler

[Peter Xu]

On Wed, Feb 07, 2024 at 02:33:34PM +0100, Cédric Le Goater wrote:
> diff --git a/migration/ram.c b/migration/ram.c
> index d5b7cd5ac2f31aabf4a248b966153401c48912cf..136c237f4079f68d4e578cf1c72eec2efc815bc8 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2931,7 +2931,7 @@ void qemu_guest_free_page_hint(void *addr, size_t len)
>   * @f: QEMUFile where to send the data
>   * @opaque: RAMState pointer

Document may need a touch-up.

>   */
> -static int ram_save_setup(QEMUFile *f, void *opaque)
> +static int ram_save_setup(QEMUFile *f, void *opaque, Error **errp)
>  {
>      RAMState **rsp = opaque;
>      RAMBlock *block;

Besides:

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH 02/14] migration: Add Error** argument to .load_setup() handler

[Peter Xu]

On Wed, Feb 07, 2024 at 02:33:35PM +0100, Cédric Le Goater wrote:
> diff --git a/migration/ram.c b/migration/ram.c
> index 136c237f4079f68d4e578cf1c72eec2efc815bc8..8dac9bac2fe8b8c19e102c771a7ef6e976252906 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -3498,7 +3498,7 @@ void colo_release_ram_cache(void)
>   * @f: QEMUFile where to receive the data
>   * @opaque: RAMState pointer

Another one may need touch up..

>   */
> -static int ram_load_setup(QEMUFile *f, void *opaque)
> +static int ram_load_setup(QEMUFile *f, void *opaque, Error **errp)
>  {
>      xbzrle_load_setup();
>      ramblock_recv_map_init();
> diff --git a/migration/savevm.c b/migration/savevm.c
> index f2ae799bad13e631bccf733a34c3a8fd22e8dd48..990f4249a26d28117ee365d8b20fc5bbca0d43d6 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2737,7 +2737,7 @@ static void qemu_loadvm_state_switchover_ack_needed(MigrationIncomingState *mis)
>      trace_loadvm_state_switchover_ack_needed(mis->switchover_ack_pending_num);
>  }
>  
> -static int qemu_loadvm_state_setup(QEMUFile *f)
> +static int qemu_loadvm_state_setup(QEMUFile *f, Error **errp)
>  {
>      SaveStateEntry *se;
>      int ret;
> @@ -2753,10 +2753,11 @@ static int qemu_loadvm_state_setup(QEMUFile *f)
>              }
>          }
>  
> -        ret = se->ops->load_setup(f, se->opaque);
> +        ret = se->ops->load_setup(f, se->opaque, errp);
>          if (ret < 0) {
> +            error_prepend(errp, "Load state of device %s failed: ",
> +                          se->idstr);
>              qemu_file_set_error(f, ret);

Do we also want to switch to _set_error_obj()?  Or even use
migrate_set_error() (the latter may apply to previous patch too if it
works)?

> -            error_report("Load state of device %s failed", se->idstr);
>              return ret;
>          }
>      }
> @@ -2937,7 +2938,8 @@ int qemu_loadvm_state(QEMUFile *f)
>          return ret;
>      }
>  
> -    if (qemu_loadvm_state_setup(f) != 0) {
> +    if (qemu_loadvm_state_setup(f, &local_err) != 0) {
> +        error_report_err(local_err);
>          return -EINVAL;
>      }
>  
> -- 
> 2.43.0
> 
> 

-- 
Peter Xu

Re: [PATCH 03/14] memory: Add Error** argument to .log_global*() handlers

[Peter Xu]

On Wed, Feb 07, 2024 at 02:33:36PM +0100, Cédric Le Goater wrote:
> @@ -2936,14 +2940,14 @@ void memory_global_dirty_log_start(unsigned int flags)
>      trace_global_dirty_changed(global_dirty_tracking);
>  
>      if (!old_flags) {
> -        MEMORY_LISTENER_CALL_GLOBAL(log_global_start, Forward);
> +        MEMORY_LISTENER_CALL_GLOBAL(log_global_start, Forward, errp);
>          memory_region_transaction_begin();
>          memory_region_update_pending = true;
>          memory_region_transaction_commit();
>      }
>  }
>  
> -static void memory_global_dirty_log_do_stop(unsigned int flags)
> +static void memory_global_dirty_log_do_stop(unsigned int flags, Error **errp)
>  {
>      assert(flags && !(flags & (~GLOBAL_DIRTY_MASK)));
>      assert((global_dirty_tracking & flags) == flags);
> @@ -2955,7 +2959,7 @@ static void memory_global_dirty_log_do_stop(unsigned int flags)
>          memory_region_transaction_begin();
>          memory_region_update_pending = true;
>          memory_region_transaction_commit();
> -        MEMORY_LISTENER_CALL_GLOBAL(log_global_stop, Reverse);
> +        MEMORY_LISTENER_CALL_GLOBAL(log_global_stop, Reverse, errp);
>      }
>  }

I'm a little bit surprised to see that MEMORY_LISTENER_CALL_GLOBAL()
already allows >2 args, with the ability to conditionally pass over errp
with such oneliner change; even if all callers were only using 2 args
before this patch..

Acked-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH 12/14] migration: Report error when shutdown fails

[Peter Xu]

On Wed, Feb 07, 2024 at 02:33:45PM +0100, Cédric Le Goater wrote:
> This will help detect issues regarding I/O channels usage.
> 
> Signed-off-by: Cédric Le Goater <clg@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH 13/14] migration: Use migrate_has_error() in close_return_path_on_source()

[Peter Xu]

On Wed, Feb 07, 2024 at 02:33:46PM +0100, Cédric Le Goater wrote:
> close_return_path_on_source() retrieves the migration error from the
> the QEMUFile '->to_dst_file' to know if a shutdown is required. This
> shutdown is required to exit the return-path thread. However, in
> migrate_fd_cleanup(), '->to_dst_file' is cleaned up before calling
> close_return_path_on_source() and the shutdown is never performed,
> leaving the source and destination waiting for an event to occur.
> 
> Avoid relying on '->to_dst_file' and use migrate_has_error() instead.
> 
> Suggested-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [RFC PATCH 14/14] migration: Fix return-path thread exit

[Peter Xu]

On Wed, Feb 07, 2024 at 02:33:47PM +0100, Cédric Le Goater wrote:
> In case of error, close_return_path_on_source() can perform a shutdown
> to exit the return-path thread.  However, in migrate_fd_cleanup(),
> 'to_dst_file' is closed before calling close_return_path_on_source()
> and the shutdown fails, leaving the source and destination waiting for
> an event to occur.
> 
> Close the file after calling close_return_path_on_source() so that the
> shutdown succeeds and the return-path thread exits.
> 
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
> 
>  This is an RFC because the correct fix implies reworking the QEMUFile
>  construct, built on top of the QEMU I/O channel.
> 
>  migration/migration.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 5f55af3d7624750ca416c4177781241b3e291e5d..de329f2c553288935d824748286e79e535929b8b 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1313,6 +1313,8 @@ void migrate_set_state(int *state, int old_state, int new_state)
>  
>  static void migrate_fd_cleanup(MigrationState *s)
>  {
> +    QEMUFile *tmp = NULL;
> +
>      g_free(s->hostname);
>      s->hostname = NULL;
>      json_writer_free(s->vmdesc);
> @@ -1321,8 +1323,6 @@ static void migrate_fd_cleanup(MigrationState *s)
>      qemu_savevm_state_cleanup();
>  
>      if (s->to_dst_file) {
> -        QEMUFile *tmp;
> -
>          trace_migrate_fd_cleanup();
>          bql_unlock();
>          if (s->migration_thread_running) {
> @@ -1341,15 +1341,14 @@ static void migrate_fd_cleanup(MigrationState *s)
>           * critical section won't block for long.
>           */
>          migration_ioc_unregister_yank_from_file(tmp);
> -        qemu_fclose(tmp);
>      }
>  
> -    /*
> -     * We already cleaned up to_dst_file, so errors from the return
> -     * path might be due to that, ignore them.
> -     */
>      close_return_path_on_source(s);
>  
> +    if (tmp) {
> +        qemu_fclose(tmp);
> +    }
> +
>      assert(!migration_is_active(s));
>  
>      if (s->state == MIGRATION_STATUS_CANCELLING) {

I think this is okay to me for a short term plan.  I'll see how others
think, also add Dan into the loop.

If so, would you please add a rich comment explaining why tmp needs to be
closed later?  Especially, explicit comment on the ordering requirement
would be helpful: IMHO here it's an order that qemu_fclose() must happen
after close_return_path_on_source().  So when others work on this code we
don't easily break it without noticing.

Also please feel free to post separately on migration patches if you'd like
us to merge the patches when repost.

Thanks,

-- 
Peter Xu

Re: [PATCH 13/14] migration: Use migrate_has_error() in close_return_path_on_source()

[Fabiano Rosas]

Cédric Le Goater <clg@redhat.com> writes:

> close_return_path_on_source() retrieves the migration error from the
> the QEMUFile '->to_dst_file' to know if a shutdown is required. This
> shutdown is required to exit the return-path thread. However, in
> migrate_fd_cleanup(), '->to_dst_file' is cleaned up before calling
> close_return_path_on_source() and the shutdown is never performed,
> leaving the source and destination waiting for an event to occur.
>
> Avoid relying on '->to_dst_file' and use migrate_has_error() instead.
>
> Suggested-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>  migration/migration.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index d5f705ceef4c925589aa49335969672c0d761fa2..5f55af3d7624750ca416c4177781241b3e291e5d 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2372,8 +2372,7 @@ static bool close_return_path_on_source(MigrationState *ms)
>       * cause it to unblock if it's stuck waiting for the destination.
>       */
>      WITH_QEMU_LOCK_GUARD(&ms->qemu_file_lock) {
> -        if (ms->to_dst_file && ms->rp_state.from_dst_file &&
> -            qemu_file_get_error(ms->to_dst_file)) {
> +        if (migrate_has_error(ms) && ms->rp_state.from_dst_file) {
>              qemu_file_shutdown(ms->rp_state.from_dst_file);
>          }
>      }

Hm, maybe Peter can help defend this, but this assumes that every
function that takes an 'f' and sets the file error also sets
migrate_set_error(). I'm not sure we have determined that, have we?

Re: [PATCH 01/14] migration: Add Error** argument to .save_setup() handler

[Cédric Le Goater]

On 2/7/24 21:11, Philippe Mathieu-Daudé wrote:
> On 7/2/24 14:33, Cédric Le Goater wrote:
>> The purpose is to record a potential error in the migration stream if
>> qemu_savevm_state_setup() fails. Most of the current .save_setup()
>> handlers can be modified to use the Error argument instead of managing
>> their own and calling locally error_report(). The following patches
>> will introduce such changes for VFIO first.
>>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>   include/migration/register.h   | 2 +-
>>   hw/ppc/spapr.c                 | 2 +-
>>   hw/s390x/s390-stattrib.c       | 2 +-
>>   hw/vfio/migration.c            | 2 +-
>>   migration/block-dirty-bitmap.c | 2 +-
>>   migration/block.c              | 2 +-
>>   migration/ram.c                | 2 +-
>>   migration/savevm.c             | 4 ++--
>>   8 files changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/migration/register.h b/include/migration/register.h
>> index 9ab1f79512c605f0c88a45b560c57486fa054441..831600a00eae4efd0464b60925d65de4d9dbcff8 100644
>> --- a/include/migration/register.h
>> +++ b/include/migration/register.h
>> @@ -25,7 +25,7 @@ typedef struct SaveVMHandlers {
>>        * used to perform early checks.
>>        */
>>       int (*save_prepare)(void *opaque, Error **errp);
>> -    int (*save_setup)(QEMUFile *f, void *opaque);
>> +    int (*save_setup)(QEMUFile *f, void *opaque, Error **errp);
> 
> Since you change this, do you mind adding a docstring
> describing this prototype?

I can send an initial patch adding the documentation tags and then
resend the same patch with the updates people will provide. I don't
have the knowledge to cover all of the SaveVMHandlers struct on my
own.

Thanks,

C.


> Otherwise,
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 

Re: [RFC PATCH 14/14] migration: Fix return-path thread exit

[Fabiano Rosas]

Cédric Le Goater <clg@redhat.com> writes:

> In case of error, close_return_path_on_source() can perform a shutdown
> to exit the return-path thread.  However, in migrate_fd_cleanup(),
> 'to_dst_file' is closed before calling close_return_path_on_source()
> and the shutdown fails, leaving the source and destination waiting for
> an event to occur.

Hi, Cédric

Are you sure this is not caused by patch 13? That 'if (ms->to_dst_file'
was there to avoid this sort of thing happening.

Is there some reordering possibility that I'm not spotting in the code
below? I think the data dependency on to_dst_file shouldn't allow it.

migrate_fd_cleanup:
        qemu_mutex_lock(&s->qemu_file_lock);
        tmp = s->to_dst_file;
        s->to_dst_file = NULL;
        qemu_mutex_unlock(&s->qemu_file_lock);
        ...
        qemu_fclose(tmp);

close_return_path_on_source:
    WITH_QEMU_LOCK_GUARD(&ms->qemu_file_lock) {
        if (ms->to_dst_file && ms->rp_state.from_dst_file &&
            qemu_file_get_error(ms->to_dst_file)) {
            qemu_file_shutdown(ms->rp_state.from_dst_file);
        }
    }

I'm thinking maybe the culprit is the close_return_path_on_source() at
migration_completion(). It might be possible for it to race with the
migrate_fd_cleanup_bh from migration_iteration_finish().

If that's the case, then I think that one possible fix would be to hold
the BQL at migration_completion() so the BH doesn't get dispatched until
we properly close the return path.

>
> Close the file after calling close_return_path_on_source() so that the
> shutdown succeeds and the return-path thread exits.
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>
>  This is an RFC because the correct fix implies reworking the QEMUFile
>  construct, built on top of the QEMU I/O channel.
>
>  migration/migration.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 5f55af3d7624750ca416c4177781241b3e291e5d..de329f2c553288935d824748286e79e535929b8b 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1313,6 +1313,8 @@ void migrate_set_state(int *state, int old_state, int new_state)
>  
>  static void migrate_fd_cleanup(MigrationState *s)
>  {
> +    QEMUFile *tmp = NULL;
> +
>      g_free(s->hostname);
>      s->hostname = NULL;
>      json_writer_free(s->vmdesc);
> @@ -1321,8 +1323,6 @@ static void migrate_fd_cleanup(MigrationState *s)
>      qemu_savevm_state_cleanup();
>  
>      if (s->to_dst_file) {
> -        QEMUFile *tmp;
> -
>          trace_migrate_fd_cleanup();
>          bql_unlock();
>          if (s->migration_thread_running) {
> @@ -1341,15 +1341,14 @@ static void migrate_fd_cleanup(MigrationState *s)
>           * critical section won't block for long.
>           */
>          migration_ioc_unregister_yank_from_file(tmp);
> -        qemu_fclose(tmp);
>      }
>  
> -    /*
> -     * We already cleaned up to_dst_file, so errors from the return
> -     * path might be due to that, ignore them.
> -     */
>      close_return_path_on_source(s);
>  
> +    if (tmp) {
> +        qemu_fclose(tmp);
> +    }
> +
>      assert(!migration_is_active(s));
>  
>      if (s->state == MIGRATION_STATUS_CANCELLING) {

Re: [PATCH 13/14] migration: Use migrate_has_error() in close_return_path_on_source()

[Cédric Le Goater]

On 2/8/24 14:07, Fabiano Rosas wrote:
> Cédric Le Goater <clg@redhat.com> writes:
> 
>> close_return_path_on_source() retrieves the migration error from the
>> the QEMUFile '->to_dst_file' to know if a shutdown is required. This
>> shutdown is required to exit the return-path thread. However, in
>> migrate_fd_cleanup(), '->to_dst_file' is cleaned up before calling
>> close_return_path_on_source() and the shutdown is never performed,
>> leaving the source and destination waiting for an event to occur.
>>
>> Avoid relying on '->to_dst_file' and use migrate_has_error() instead.
>>
>> Suggested-by: Peter Xu <peterx@redhat.com>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>   migration/migration.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index d5f705ceef4c925589aa49335969672c0d761fa2..5f55af3d7624750ca416c4177781241b3e291e5d 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -2372,8 +2372,7 @@ static bool close_return_path_on_source(MigrationState *ms)
>>        * cause it to unblock if it's stuck waiting for the destination.
>>        */
>>       WITH_QEMU_LOCK_GUARD(&ms->qemu_file_lock) {
>> -        if (ms->to_dst_file && ms->rp_state.from_dst_file &&
>> -            qemu_file_get_error(ms->to_dst_file)) {
>> +        if (migrate_has_error(ms) && ms->rp_state.from_dst_file) {
>>               qemu_file_shutdown(ms->rp_state.from_dst_file);
>>           }
>>       }
> 
> Hm, maybe Peter can help defend this, but this assumes that every
> function that takes an 'f' and sets the file error also sets
> migrate_set_error(). I'm not sure we have determined that, have we?

How could we check all the code path ? I agree it is difficult when
looking at the code :/

Thanks,

C.

Re: [PATCH 13/14] migration: Use migrate_has_error() in close_return_path_on_source()

[Fabiano Rosas]

Cédric Le Goater <clg@redhat.com> writes:

> On 2/8/24 14:07, Fabiano Rosas wrote:
>> Cédric Le Goater <clg@redhat.com> writes:
>> 
>>> close_return_path_on_source() retrieves the migration error from the
>>> the QEMUFile '->to_dst_file' to know if a shutdown is required. This
>>> shutdown is required to exit the return-path thread. However, in
>>> migrate_fd_cleanup(), '->to_dst_file' is cleaned up before calling
>>> close_return_path_on_source() and the shutdown is never performed,
>>> leaving the source and destination waiting for an event to occur.
>>>
>>> Avoid relying on '->to_dst_file' and use migrate_has_error() instead.
>>>
>>> Suggested-by: Peter Xu <peterx@redhat.com>
>>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>>> ---
>>>   migration/migration.c | 3 +--
>>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/migration/migration.c b/migration/migration.c
>>> index d5f705ceef4c925589aa49335969672c0d761fa2..5f55af3d7624750ca416c4177781241b3e291e5d 100644
>>> --- a/migration/migration.c
>>> +++ b/migration/migration.c
>>> @@ -2372,8 +2372,7 @@ static bool close_return_path_on_source(MigrationState *ms)
>>>        * cause it to unblock if it's stuck waiting for the destination.
>>>        */
>>>       WITH_QEMU_LOCK_GUARD(&ms->qemu_file_lock) {
>>> -        if (ms->to_dst_file && ms->rp_state.from_dst_file &&
>>> -            qemu_file_get_error(ms->to_dst_file)) {
>>> +        if (migrate_has_error(ms) && ms->rp_state.from_dst_file) {
>>>               qemu_file_shutdown(ms->rp_state.from_dst_file);
>>>           }
>>>       }
>> 
>> Hm, maybe Peter can help defend this, but this assumes that every
>> function that takes an 'f' and sets the file error also sets
>> migrate_set_error(). I'm not sure we have determined that, have we?
>
> How could we check all the code path ? I agree it is difficult when
> looking at the code :/

It would help if the thing wasn't called 'f' for the most part of the
code to begin with.

Whenever there's a file error at to_dst_file there's the chance that the
rp_state.from_dst_file got stuck. So we cannot ignore the file error.

Would it work if we checked it earlier during cleanup as you did
previously and then set the migration error?

Re: [PATCH 03/14] memory: Add Error** argument to .log_global*() handlers

[Philippe Mathieu-Daudé]

Hi Cédric,

On 7/2/24 14:33, Cédric Le Goater wrote:
> Modify memory_global_dirty_log_start() and memory_global_dirty_log_stop()
> to also take an Error** parameter and report the error in the callers.
> Aside from error reporting, there should be no functional changes.
> 
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Paul Durrant <paul@xen.org>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: David Hildenbrand <david@redhat.com>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>   include/exec/memory.h | 12 ++++++++----
>   hw/i386/xen/xen-hvm.c |  8 ++++----
>   hw/vfio/common.c      |  6 ++++--
>   hw/virtio/vhost.c     |  4 ++--
>   migration/dirtyrate.c | 24 ++++++++++++++++++++----
>   migration/ram.c       | 27 +++++++++++++++++++++++----
>   system/memory.c       | 37 +++++++++++++++++++++++++------------
>   7 files changed, 86 insertions(+), 32 deletions(-)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 177be23db709d8bab9cebfe6acbae57611073327..b348070dc8f17b3505196d3a92d8cfb2171b640f 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -998,8 +998,9 @@ struct MemoryListener {
>        * active at that time.
>        *
>        * @listener: The #MemoryListener.
> +     * @errp: pointer to Error*, to store an error if it happens.
>        */
> -    void (*log_global_start)(MemoryListener *listener);
> +    void (*log_global_start)(MemoryListener *listener, Error **errp);

As documented in error.h, functions taking an Error** handle
as last argument should return a boolean indicating failure...

(multiple occurrences)

> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index 1d2e85746fb7b10eb7f149976970f9a92125af8a..443acab7a7efbd6e9c94883363e1a827a3538292 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -90,13 +90,19 @@ static int64_t do_calculate_dirtyrate(DirtyPageRecord dirty_pages,
>   
>   void global_dirty_log_change(unsigned int flag, bool start)
>   {
> +    Error *local_err = NULL;
> +
>       bql_lock();
>       if (start) {
> -        memory_global_dirty_log_start(flag);
> +        memory_global_dirty_log_start(flag, &local_err);
>       } else {
> -        memory_global_dirty_log_stop(flag);
> +        memory_global_dirty_log_stop(flag, &local_err);
>       }
>       bql_unlock();
> +
> +    if (local_err) {

... that way we don't have to check the pointer.

> +        error_report_err(local_err);
> +    }
>   }

Re: [PATCH 02/14] migration: Add Error** argument to .load_setup() handler

[Cédric Le Goater]

On 2/8/24 05:30, Peter Xu wrote:
> On Wed, Feb 07, 2024 at 02:33:35PM +0100, Cédric Le Goater wrote:
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 136c237f4079f68d4e578cf1c72eec2efc815bc8..8dac9bac2fe8b8c19e102c771a7ef6e976252906 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -3498,7 +3498,7 @@ void colo_release_ram_cache(void)
>>    * @f: QEMUFile where to receive the data
>>    * @opaque: RAMState pointer
> 
> Another one may need touch up..
> 
>>    */
>> -static int ram_load_setup(QEMUFile *f, void *opaque)
>> +static int ram_load_setup(QEMUFile *f, void *opaque, Error **errp)
>>   {
>>       xbzrle_load_setup();
>>       ramblock_recv_map_init();
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index f2ae799bad13e631bccf733a34c3a8fd22e8dd48..990f4249a26d28117ee365d8b20fc5bbca0d43d6 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -2737,7 +2737,7 @@ static void qemu_loadvm_state_switchover_ack_needed(MigrationIncomingState *mis)
>>       trace_loadvm_state_switchover_ack_needed(mis->switchover_ack_pending_num);
>>   }
>>   
>> -static int qemu_loadvm_state_setup(QEMUFile *f)
>> +static int qemu_loadvm_state_setup(QEMUFile *f, Error **errp)
>>   {
>>       SaveStateEntry *se;
>>       int ret;
>> @@ -2753,10 +2753,11 @@ static int qemu_loadvm_state_setup(QEMUFile *f)
>>               }
>>           }
>>   
>> -        ret = se->ops->load_setup(f, se->opaque);
>> +        ret = se->ops->load_setup(f, se->opaque, errp);
>>           if (ret < 0) {
>> +            error_prepend(errp, "Load state of device %s failed: ",
>> +                          se->idstr);
>>               qemu_file_set_error(f, ret);
> 
> Do we also want to switch to _set_error_obj()? 

yes. possible.

> Or even use migrate_set_error() 

It seems so and may be even remove it completely.

What we could do first is add an Errp ** argument to qemu_loadvm_state()
which would improve qmp_xen_load_devices_state() and load_snapshot().
It is less obvious for process_incoming_migration_co().

> (the latter may apply to previous patch too if it works)?

It seems safe to use migrate_set_error for both migration_thread() and
bg_migration_thread() because migration_detect_error() is called after
calling qemu_savevm_state_setup().

However, qemu_savevm_state() relies only on qemu_file_get_error() and
there would be a problem there I think.

Thanks,

C.


> 
>> -            error_report("Load state of device %s failed", se->idstr);
>>               return ret;
>>           }
>>       }
>> @@ -2937,7 +2938,8 @@ int qemu_loadvm_state(QEMUFile *f)
>>           return ret;
>>       }
>>   
>> -    if (qemu_loadvm_state_setup(f) != 0) {
>> +    if (qemu_loadvm_state_setup(f, &local_err) != 0) {
>> +        error_report_err(local_err);
>>           return -EINVAL;
>>       }
>>   
>> -- 
>> 2.43.0
>>
>>
> 

Re: [PATCH 03/14] memory: Add Error** argument to .log_global*() handlers

[Cédric Le Goater]

On 2/8/24 06:48, Peter Xu wrote:
> On Wed, Feb 07, 2024 at 02:33:36PM +0100, Cédric Le Goater wrote:
>> @@ -2936,14 +2940,14 @@ void memory_global_dirty_log_start(unsigned int flags)
>>       trace_global_dirty_changed(global_dirty_tracking);
>>   
>>       if (!old_flags) {
>> -        MEMORY_LISTENER_CALL_GLOBAL(log_global_start, Forward);
>> +        MEMORY_LISTENER_CALL_GLOBAL(log_global_start, Forward, errp);
>>           memory_region_transaction_begin();
>>           memory_region_update_pending = true;
>>           memory_region_transaction_commit();
>>       }
>>   }
>>   
>> -static void memory_global_dirty_log_do_stop(unsigned int flags)
>> +static void memory_global_dirty_log_do_stop(unsigned int flags, Error **errp)
>>   {
>>       assert(flags && !(flags & (~GLOBAL_DIRTY_MASK)));
>>       assert((global_dirty_tracking & flags) == flags);
>> @@ -2955,7 +2959,7 @@ static void memory_global_dirty_log_do_stop(unsigned int flags)
>>           memory_region_transaction_begin();
>>           memory_region_update_pending = true;
>>           memory_region_transaction_commit();
>> -        MEMORY_LISTENER_CALL_GLOBAL(log_global_stop, Reverse);
>> +        MEMORY_LISTENER_CALL_GLOBAL(log_global_stop, Reverse, errp);
>>       }
>>   }
> 
> I'm a little bit surprised to see that MEMORY_LISTENER_CALL_GLOBAL()
> already allows >2 args, with the ability to conditionally pass over errp
> with such oneliner change; even if all callers were only using 2 args
> before this patch..
yes. The proposal takes the easy path.

Should we change all memory listener global handlers :

   begin
   commit
   log_global_after_sync
   log_global_start
   log_global_stop

to take an extra Error **errp argument ?

I think we should distinguish begin + commit handlers from the log_global_*
with a new macro. In which case, we could also change the handler to return
a bool and fail at the first error in MEMORY_LISTENER_CALL_GLOBAL(...).

Thanks,

C.

Re: [PATCH 01/14] migration: Add Error** argument to .save_setup() handler

[Avihai Horon]

Hi, Cedric

On 07/02/2024 15:33, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> The purpose is to record a potential error in the migration stream if
> qemu_savevm_state_setup() fails. Most of the current .save_setup()
> handlers can be modified to use the Error argument instead of managing
> their own and calling locally error_report(). The following patches
> will introduce such changes for VFIO first.
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>   include/migration/register.h   | 2 +-
>   hw/ppc/spapr.c                 | 2 +-
>   hw/s390x/s390-stattrib.c       | 2 +-
>   hw/vfio/migration.c            | 2 +-
>   migration/block-dirty-bitmap.c | 2 +-
>   migration/block.c              | 2 +-
>   migration/ram.c                | 2 +-
>   migration/savevm.c             | 4 ++--
>   8 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/include/migration/register.h b/include/migration/register.h
> index 9ab1f79512c605f0c88a45b560c57486fa054441..831600a00eae4efd0464b60925d65de4d9dbcff8 100644
> --- a/include/migration/register.h
> +++ b/include/migration/register.h
> @@ -25,7 +25,7 @@ typedef struct SaveVMHandlers {
>        * used to perform early checks.
>        */
>       int (*save_prepare)(void *opaque, Error **errp);
> -    int (*save_setup)(QEMUFile *f, void *opaque);
> +    int (*save_setup)(QEMUFile *f, void *opaque, Error **errp);
>       void (*save_cleanup)(void *opaque);
>       int (*save_live_complete_postcopy)(QEMUFile *f, void *opaque);
>       int (*save_live_complete_precopy)(QEMUFile *f, void *opaque);
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 0d72d286d80f0435122593555f79fae4d90acf81..a1b0aa02582ad2d68a13476c1859b18143da7bb8 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2142,7 +2142,7 @@ static const VMStateDescription vmstate_spapr = {
>       }
>   };
>
> -static int htab_save_setup(QEMUFile *f, void *opaque)
> +static int htab_save_setup(QEMUFile *f, void *opaque, Error **errp)
>   {
>       SpaprMachineState *spapr = opaque;
>
> diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c
> index c483b62a9b5f71772639fc180bdad15ecb6711cb..c934df424a555d83d2198f5ddfc0cbe0ea98e9ec 100644
> --- a/hw/s390x/s390-stattrib.c
> +++ b/hw/s390x/s390-stattrib.c
> @@ -166,7 +166,7 @@ static int cmma_load(QEMUFile *f, void *opaque, int version_id)
>       return ret;
>   }
>
> -static int cmma_save_setup(QEMUFile *f, void *opaque)
> +static int cmma_save_setup(QEMUFile *f, void *opaque, Error **errp)
>   {
>       S390StAttribState *sas = S390_STATTRIB(opaque);
>       S390StAttribClass *sac = S390_STATTRIB_GET_CLASS(sas);
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index 70e6b1a709f9b67e4c9eb41033d76347275cac42..8bcb4bc73cd5ba5338e3ffa4d907d0e6bfbb9485 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -378,7 +378,7 @@ static int vfio_save_prepare(void *opaque, Error **errp)
>       return 0;
>   }
>
> -static int vfio_save_setup(QEMUFile *f, void *opaque)
> +static int vfio_save_setup(QEMUFile *f, void *opaque, Error **errp)
>   {
>       VFIODevice *vbasedev = opaque;
>       VFIOMigration *migration = vbasedev->migration;
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index 2708abf3d762de774ed294d3fdb8e56690d2974c..16f84e6c57c2403a8c2d6319f4e7b6360dade28c 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -1213,7 +1213,7 @@ fail:
>       return ret;
>   }
>
> -static int dirty_bitmap_save_setup(QEMUFile *f, void *opaque)
> +static int dirty_bitmap_save_setup(QEMUFile *f, void *opaque, Error **errp)
>   {
>       DBMSaveState *s = &((DBMState *)opaque)->save;
>       SaveBitmapState *dbms = NULL;
> diff --git a/migration/block.c b/migration/block.c
> index 8c6ebafacc1ffe930d1d4f19d968817b14852c69..df15319ceab66201b043f15eac1b0a7d6522b60c 100644
> --- a/migration/block.c
> +++ b/migration/block.c
> @@ -708,7 +708,7 @@ static void block_migration_cleanup(void *opaque)
>       blk_mig_unlock();
>   }
>
> -static int block_save_setup(QEMUFile *f, void *opaque)
> +static int block_save_setup(QEMUFile *f, void *opaque, Error **errp)
>   {
>       int ret;
>
> diff --git a/migration/ram.c b/migration/ram.c
> index d5b7cd5ac2f31aabf4a248b966153401c48912cf..136c237f4079f68d4e578cf1c72eec2efc815bc8 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2931,7 +2931,7 @@ void qemu_guest_free_page_hint(void *addr, size_t len)
>    * @f: QEMUFile where to send the data
>    * @opaque: RAMState pointer
>    */
> -static int ram_save_setup(QEMUFile *f, void *opaque)
> +static int ram_save_setup(QEMUFile *f, void *opaque, Error **errp)
>   {
>       RAMState **rsp = opaque;
>       RAMBlock *block;
> diff --git a/migration/savevm.c b/migration/savevm.c
> index d612c8a9020b204d5d078d5df85f0e6449c27645..f2ae799bad13e631bccf733a34c3a8fd22e8dd48 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1342,10 +1342,10 @@ void qemu_savevm_state_setup(QEMUFile *f)
>           }
>           save_section_header(f, se, QEMU_VM_SECTION_START);
>
> -        ret = se->ops->save_setup(f, se->opaque);
> +        ret = se->ops->save_setup(f, se->opaque, &local_err);
>           save_section_footer(f, se);
>           if (ret < 0) {
> -            qemu_file_set_error(f, ret);
> +            qemu_file_set_error_obj(f, ret, local_err);

Should we set local_err = NULL? Because it is re-used a few lines after 
this, by precopy_notify().

BTW, I think that if we add Error** parameter to functions we must make 
sure all their error flows set errp as well.
According to Error API:
* - On success, the function should not touch *errp.  On failure, it
*   should set a new error, e.g. with error_setg(errp, ...), or
*   propagate an existing one, e.g. with error_propagate(errp, ...).

For example, a caller that handles errors by printing them with 
error_report_err() would crash when trying to access NULL error object 
(if some error path didn't set errp).
If you agree, we should check it throughout the series.

Thanks.

>               break;
>           }
>       }
> --
> 2.43.0
>
>

Re: [PATCH 03/14] memory: Add Error** argument to .log_global*() handlers

[Avihai Horon]

Hi Cedric,

On 09/02/2024 12:14, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> On 2/8/24 06:48, Peter Xu wrote:
>> On Wed, Feb 07, 2024 at 02:33:36PM +0100, Cédric Le Goater wrote:
>>> @@ -2936,14 +2940,14 @@ void memory_global_dirty_log_start(unsigned 
>>> int flags)
>>>       trace_global_dirty_changed(global_dirty_tracking);
>>>
>>>       if (!old_flags) {
>>> -        MEMORY_LISTENER_CALL_GLOBAL(log_global_start, Forward);
>>> +        MEMORY_LISTENER_CALL_GLOBAL(log_global_start, Forward, errp);
>>>           memory_region_transaction_begin();
>>>           memory_region_update_pending = true;
>>>           memory_region_transaction_commit();
>>>       }
>>>   }
>>>
>>> -static void memory_global_dirty_log_do_stop(unsigned int flags)
>>> +static void memory_global_dirty_log_do_stop(unsigned int flags, 
>>> Error **errp)
>>>   {
>>>       assert(flags && !(flags & (~GLOBAL_DIRTY_MASK)));
>>>       assert((global_dirty_tracking & flags) == flags);
>>> @@ -2955,7 +2959,7 @@ static void 
>>> memory_global_dirty_log_do_stop(unsigned int flags)
>>>           memory_region_transaction_begin();
>>>           memory_region_update_pending = true;
>>>           memory_region_transaction_commit();
>>> -        MEMORY_LISTENER_CALL_GLOBAL(log_global_stop, Reverse);
>>> +        MEMORY_LISTENER_CALL_GLOBAL(log_global_stop, Reverse, errp);
>>>       }
>>>   }
>>
>> I'm a little bit surprised to see that MEMORY_LISTENER_CALL_GLOBAL()
>> already allows >2 args, with the ability to conditionally pass over errp
>> with such oneliner change; even if all callers were only using 2 args
>> before this patch..
> yes. The proposal takes the easy path.
>
> Should we change all memory listener global handlers :
>
>   begin
>   commit
>   log_global_after_sync
>   log_global_start
>   log_global_stop
>
> to take an extra Error **errp argument ?
>
> I think we should distinguish begin + commit handlers from the 
> log_global_*
> with a new macro. In which case, we could also change the handler to 
> return
> a bool and fail at the first error in MEMORY_LISTENER_CALL_GLOBAL(...).

I think we must fail at first error in any case. Otherwise, if two 
handlers error and call error_setg() with errp, the second handler will 
assert IIUC.

Re: [PATCH 04/14] migration: Modify ram_init_bitmaps() to report dirty tracking errors

[Avihai Horon]

Hi Cedric,

On 07/02/2024 15:33, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> The .save_setup() handler has now an Error** argument that we can use
> to propagate errors reported by the .log_global_start() handler. Do
> that for the RAM. qemu_savevm_state_setup() will store the error under
> the migration stream for later detection in the migration sequence.
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>   migration/ram.c | 19 ++++++++++---------
>   1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/migration/ram.c b/migration/ram.c
> index d86626bb1c704b2d3497b323a702ca6ca8939a79..b87245466bb46937fd0358d0c66432bcc6280018 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2802,19 +2802,17 @@ static void migration_bitmap_clear_discarded_pages(RAMState *rs)
>       }
>   }
>
> -static void ram_init_bitmaps(RAMState *rs)
> +static void ram_init_bitmaps(RAMState *rs, Error **errp)
>   {
> -    Error *local_err = NULL;
> -
>       qemu_mutex_lock_ramlist();
>
>       WITH_RCU_READ_LOCK_GUARD() {
>           ram_list_init_bitmaps();
>           /* We don't use dirty log with background snapshots */
>           if (!migrate_background_snapshot()) {
> -            memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, &local_err);
> -            if (local_err) {
> -                error_report_err(local_err);
> +            memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, errp);
> +            if (*errp) {

I think we should use ERRP_GUARD() or a local error here and also below 
at ram_init_bitmaps() (or return bool like Philippe suggested).

Thanks.

> +                break;
>               }
>               migration_bitmap_sync_precopy(rs, false);
>           }
> @@ -2828,7 +2826,7 @@ static void ram_init_bitmaps(RAMState *rs)
>       migration_bitmap_clear_discarded_pages(rs);
>   }
>
> -static int ram_init_all(RAMState **rsp)
> +static int ram_init_all(RAMState **rsp, Error **errp)
>   {
>       if (ram_state_init(rsp)) {
>           return -1;
> @@ -2839,7 +2837,10 @@ static int ram_init_all(RAMState **rsp)
>           return -1;
>       }
>
> -    ram_init_bitmaps(*rsp);
> +    ram_init_bitmaps(*rsp, errp);
> +    if (*errp) {
> +        return -1;
> +    }
>
>       return 0;
>   }
> @@ -2952,7 +2953,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque, Error **errp)
>
>       /* migration has already setup the bitmap, reuse it. */
>       if (!migration_in_colo_state()) {
> -        if (ram_init_all(rsp) != 0) {
> +        if (ram_init_all(rsp, errp) != 0) {
>               compress_threads_save_cleanup();
>               return -1;
>           }
> --
> 2.43.0
>
>

Re: [PATCH 08/14] vfio: Use new Error** argument in vfio_save_setup()

[Avihai Horon]

Hi Cedric,

On 07/02/2024 15:33, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> Add an Error** argument to vfio_migration_set_state() and adjust
> callers, including vfio_save_setup(). The error will be propagated up
> to qemu_savevm_state_setup() where the save_setup() handler is
> executed.
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>   hw/vfio/migration.c | 62 +++++++++++++++++++++++++++++----------------
>   1 file changed, 40 insertions(+), 22 deletions(-)
>
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index 2dfbe671f6f45aa530c7341177bb532d8292cecd..2e0a79967cc97f44d9be5575c3cfe18c9f349dab 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -84,7 +84,8 @@ static const char *mig_state_to_str(enum vfio_device_mig_state state)
>
>   static int vfio_migration_set_state(VFIODevice *vbasedev,
>                                       enum vfio_device_mig_state new_state,
> -                                    enum vfio_device_mig_state recover_state)
> +                                    enum vfio_device_mig_state recover_state,
> +                                    Error **errp)
>   {
>       VFIOMigration *migration = vbasedev->migration;
>       uint64_t buf[DIV_ROUND_UP(sizeof(struct vfio_device_feature) +
> @@ -104,15 +105,15 @@ static int vfio_migration_set_state(VFIODevice *vbasedev,
>           ret = -errno;
>
>           if (recover_state == VFIO_DEVICE_STATE_ERROR) {
> -            error_report("%s: Failed setting device state to %s, err: %s. "
> -                         "Recover state is ERROR. Resetting device",
> -                         vbasedev->name, mig_state_to_str(new_state),
> -                         strerror(errno));
> +            error_setg(errp, "%s: Failed setting device state to %s, err: %s. "
> +                       "Recover state is ERROR. Resetting device",
> +                       vbasedev->name, mig_state_to_str(new_state),
> +                       strerror(errno));
>
>               goto reset_device;
>           }
>
> -        error_report(
> +        error_setg(errp,
>               "%s: Failed setting device state to %s, err: %s. Setting device in recover state %s",
>                        vbasedev->name, mig_state_to_str(new_state),
>                        strerror(errno), mig_state_to_str(recover_state));
> @@ -120,7 +121,7 @@ static int vfio_migration_set_state(VFIODevice *vbasedev,
>           mig_state->device_state = recover_state;
>           if (ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature)) {
>               ret = -errno;
> -            error_report(
> +            error_setg(errp,
>                   "%s: Failed setting device in recover state, err: %s. Resetting device",
>                            vbasedev->name, strerror(errno));

I think here we will assert because errp is already set.

Adding an error_append() API would be useful here I guess.
Otherwise, we need to move the first error_setg() below, to before we 
return from a successful recover state change, and construct the error 
message differently (e.g., provide a full error message for the recover 
state fail case containing also the first error).

Do you have other ideas?

Thanks.

>
> @@ -139,7 +140,7 @@ static int vfio_migration_set_state(VFIODevice *vbasedev,
>                * This can happen if the device is asynchronously reset and
>                * terminates a data transfer.
>                */
> -            error_report("%s: data_fd out of sync", vbasedev->name);
> +            error_setg(errp, "%s: data_fd out of sync", vbasedev->name);
>               close(mig_state->data_fd);
>
>               return -EBADF;
> @@ -170,10 +171,11 @@ reset_device:
>    */
>   static int
>   vfio_migration_set_state_or_reset(VFIODevice *vbasedev,
> -                                  enum vfio_device_mig_state new_state)
> +                                  enum vfio_device_mig_state new_state,
> +                                  Error **errp)
>   {
>       return vfio_migration_set_state(vbasedev, new_state,
> -                                    VFIO_DEVICE_STATE_ERROR);
> +                                    VFIO_DEVICE_STATE_ERROR, errp);
>   }
>
>   static int vfio_load_buffer(QEMUFile *f, VFIODevice *vbasedev,
> @@ -391,8 +393,8 @@ static int vfio_save_setup(QEMUFile *f, void *opaque, Error **errp)
>                                         stop_copy_size);
>       migration->data_buffer = g_try_malloc0(migration->data_buffer_size);
>       if (!migration->data_buffer) {
> -        error_report("%s: Failed to allocate migration data buffer",
> -                     vbasedev->name);
> +        error_setg(errp, "%s: Failed to allocate migration data buffer",
> +                   vbasedev->name);
>           return -ENOMEM;
>       }
>
> @@ -402,7 +404,7 @@ static int vfio_save_setup(QEMUFile *f, void *opaque, Error **errp)
>           switch (migration->device_state) {
>           case VFIO_DEVICE_STATE_RUNNING:
>               ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_PRE_COPY,
> -                                           VFIO_DEVICE_STATE_RUNNING);
> +                                           VFIO_DEVICE_STATE_RUNNING, errp);
>               if (ret) {
>                   return ret;
>               }
> @@ -429,13 +431,18 @@ static void vfio_save_cleanup(void *opaque)
>   {
>       VFIODevice *vbasedev = opaque;
>       VFIOMigration *migration = vbasedev->migration;
> +    Error *local_err = NULL;
>
>       /*
>        * Changing device state from STOP_COPY to STOP can take time. Do it here,
>        * after migration has completed, so it won't increase downtime.
>        */
>       if (migration->device_state == VFIO_DEVICE_STATE_STOP_COPY) {
> -        vfio_migration_set_state_or_reset(vbasedev, VFIO_DEVICE_STATE_STOP);
> +        vfio_migration_set_state_or_reset(vbasedev, VFIO_DEVICE_STATE_STOP,
> +                                          &local_err);
> +        if (local_err) {
> +            error_report_err(local_err);
> +        }
>       }
>
>       g_free(migration->data_buffer);
> @@ -541,11 +548,13 @@ static int vfio_save_complete_precopy(QEMUFile *f, void *opaque)
>       VFIODevice *vbasedev = opaque;
>       ssize_t data_size;
>       int ret;
> +    Error *local_err = NULL;
>
>       /* We reach here with device state STOP or STOP_COPY only */
>       ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP_COPY,
> -                                   VFIO_DEVICE_STATE_STOP);
> -    if (ret) {
> +                                   VFIO_DEVICE_STATE_STOP, &local_err);
> +    if (local_err) {
> +        error_report_err(local_err);
>           return ret;
>       }
>
> @@ -585,7 +594,7 @@ static int vfio_load_setup(QEMUFile *f, void *opaque, Error **errp)
>       VFIODevice *vbasedev = opaque;
>
>       return vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_RESUMING,
> -                                   vbasedev->migration->device_state);
> +                                    vbasedev->migration->device_state, errp);
>   }
>
>   static int vfio_load_cleanup(void *opaque)
> @@ -701,20 +710,22 @@ static void vfio_vmstate_change_prepare(void *opaque, bool running,
>       VFIODevice *vbasedev = opaque;
>       VFIOMigration *migration = vbasedev->migration;
>       enum vfio_device_mig_state new_state;
> +    Error *local_err = NULL;
>       int ret;
>
>       new_state = migration->device_state == VFIO_DEVICE_STATE_PRE_COPY ?
>                       VFIO_DEVICE_STATE_PRE_COPY_P2P :
>                       VFIO_DEVICE_STATE_RUNNING_P2P;
>
> -    ret = vfio_migration_set_state_or_reset(vbasedev, new_state);
> +    ret = vfio_migration_set_state_or_reset(vbasedev, new_state, &local_err);
>       if (ret) {
>           /*
>            * Migration should be aborted in this case, but vm_state_notify()
>            * currently does not support reporting failures.
>            */
>           if (migrate_get_current()->to_dst_file) {
> -            qemu_file_set_error(migrate_get_current()->to_dst_file, ret);
> +            qemu_file_set_error_obj(migrate_get_current()->to_dst_file, ret,
> +                                    local_err);
>           }
>       }
>
> @@ -727,6 +738,7 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
>   {
>       VFIODevice *vbasedev = opaque;
>       enum vfio_device_mig_state new_state;
> +    Error *local_err = NULL;
>       int ret;
>
>       if (running) {
> @@ -739,14 +751,15 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
>                   VFIO_DEVICE_STATE_STOP;
>       }
>
> -    ret = vfio_migration_set_state_or_reset(vbasedev, new_state);
> +    ret = vfio_migration_set_state_or_reset(vbasedev, new_state, &local_err);
>       if (ret) {
>           /*
>            * Migration should be aborted in this case, but vm_state_notify()
>            * currently does not support reporting failures.
>            */
>           if (migrate_get_current()->to_dst_file) {
> -            qemu_file_set_error(migrate_get_current()->to_dst_file, ret);
> +            qemu_file_set_error_obj(migrate_get_current()->to_dst_file, ret,
> +                                    local_err);
>           }
>       }
>
> @@ -760,6 +773,7 @@ static void vfio_migration_state_notifier(Notifier *notifier, void *data)
>       VFIOMigration *migration = container_of(notifier, VFIOMigration,
>                                               migration_state);
>       VFIODevice *vbasedev = migration->vbasedev;
> +    Error *local_err = NULL;
>
>       trace_vfio_migration_state_notifier(vbasedev->name,
>                                           MigrationStatus_str(s->state));
> @@ -768,7 +782,11 @@ static void vfio_migration_state_notifier(Notifier *notifier, void *data)
>       case MIGRATION_STATUS_CANCELLING:
>       case MIGRATION_STATUS_CANCELLED:
>       case MIGRATION_STATUS_FAILED:
> -        vfio_migration_set_state_or_reset(vbasedev, VFIO_DEVICE_STATE_RUNNING);
> +        vfio_migration_set_state_or_reset(vbasedev, VFIO_DEVICE_STATE_RUNNING,
> +                                          &local_err);
> +        if (local_err) {
> +            error_report_err(local_err);
> +        }
>       }
>   }
>
> --
> 2.43.0
>
>

Re: [PATCH 09/14] vfio: Add Error** argument to .vfio_save_config() handler

[Avihai Horon]

On 07/02/2024 15:33, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> Use vmstate_save_state_with_err() to improve error reporting in the
> callers.
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>   include/hw/vfio/vfio-common.h |  2 +-
>   hw/vfio/migration.c           | 18 ++++++++++++------
>   hw/vfio/pci.c                 |  5 +++--
>   3 files changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 9b7ef7d02b5a0ad5266bcc4d06cd6874178978e4..710e0d6a880b97848af6ddc2e7968a01054fa122 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -133,7 +133,7 @@ struct VFIODeviceOps {
>       int (*vfio_hot_reset_multi)(VFIODevice *vdev);
>       void (*vfio_eoi)(VFIODevice *vdev);
>       Object *(*vfio_get_object)(VFIODevice *vdev);
> -    void (*vfio_save_config)(VFIODevice *vdev, QEMUFile *f);
> +    int (*vfio_save_config)(VFIODevice *vdev, QEMUFile *f, Error **errp);
>       int (*vfio_load_config)(VFIODevice *vdev, QEMUFile *f);
>   };
>
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index 2e0a79967cc97f44d9be5575c3cfe18c9f349dab..fb264c1ef57bbbde4306901e5449e0dfbd0ce3b7 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -190,14 +190,19 @@ static int vfio_load_buffer(QEMUFile *f, VFIODevice *vbasedev,
>       return ret;
>   }
>
> -static int vfio_save_device_config_state(QEMUFile *f, void *opaque)
> +static int vfio_save_device_config_state(QEMUFile *f, void *opaque,
> +                                         Error **errp)
>   {
>       VFIODevice *vbasedev = opaque;
> +    int ret = 0;

Nit: 0 assignment is redundant.

Thanks.

>
>       qemu_put_be64(f, VFIO_MIG_FLAG_DEV_CONFIG_STATE);
>
>       if (vbasedev->ops && vbasedev->ops->vfio_save_config) {
> -        vbasedev->ops->vfio_save_config(vbasedev, f);
> +        ret = vbasedev->ops->vfio_save_config(vbasedev, f, errp);
> +        if (ret) {
> +            return ret;
> +        }
>       }
>
>       qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
> @@ -579,13 +584,14 @@ static int vfio_save_complete_precopy(QEMUFile *f, void *opaque)
>   static void vfio_save_state(QEMUFile *f, void *opaque)
>   {
>       VFIODevice *vbasedev = opaque;
> +    Error *local_err = NULL;
>       int ret;
>
> -    ret = vfio_save_device_config_state(f, opaque);
> +    ret = vfio_save_device_config_state(f, opaque, &local_err);
>       if (ret) {
> -        error_report("%s: Failed to save device config space",
> -                     vbasedev->name);
> -        qemu_file_set_error(f, ret);
> +        error_prepend(&local_err, "%s: Failed to save device config space",
> +                      vbasedev->name);
> +        qemu_file_set_error_obj(f, ret, local_err);
>       }
>   }
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 4fa387f0430d62ca2ba1b5ae5b7037f8f06b33f9..99d86e1d40ef25133fc76ad6e58294b07bd20843 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2585,11 +2585,12 @@ const VMStateDescription vmstate_vfio_pci_config = {
>       }
>   };
>
> -static void vfio_pci_save_config(VFIODevice *vbasedev, QEMUFile *f)
> +static int vfio_pci_save_config(VFIODevice *vbasedev, QEMUFile *f, Error **errp)
>   {
>       VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
>
> -    vmstate_save_state(f, &vmstate_vfio_pci_config, vdev, NULL);
> +    return vmstate_save_state_with_err(f, &vmstate_vfio_pci_config, vdev, NULL,
> +                                       errp);
>   }
>
>   static int vfio_pci_load_config(VFIODevice *vbasedev, QEMUFile *f)
> --
> 2.43.0
>
>

Re: [PATCH 11/14] vfio: Extend vfio_set_migration_error() with Error* argument

[Avihai Horon]

Hi Cedric,

On 07/02/2024 15:33, 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.
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>   hw/vfio/common.c | 50 +++++++++++++++++++++++++++++-------------------
>   1 file changed, 30 insertions(+), 20 deletions(-)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 82173b039c47150f5edd05d329192c5b9c8a9a0f..afe8b6bd294fd5904f394a5db48aae3fd718b14e 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -148,16 +148,18 @@ 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)
>   {
>       MigrationState *ms = migrate_get_current();
>
>       if (migration_is_setup_or_active(ms->state)) {
>           WITH_QEMU_LOCK_GUARD(&ms->qemu_file_lock) {
>               if (ms->to_dst_file) {
> -                qemu_file_set_error(ms->to_dst_file, err);
> +                qemu_file_set_error_obj(ms->to_dst_file, ret, err);
>               }
>           }
> +    } else {
> +        error_report_err(err);
>       }
>   }
>
> @@ -296,15 +298,17 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>       VFIOContainerBase *bcontainer = giommu->bcontainer;
>       hwaddr iova = iotlb->iova + giommu->iommu_offset;
>       void *vaddr;
> +    Error *local_err = NULL;
>       int ret;
>
>       trace_vfio_iommu_map_notify(iotlb->perm == IOMMU_NONE ? "UNMAP" : "MAP",
>                                   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;
>       }
>
> @@ -336,11 +340,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));
> +            vfio_set_migration_error(ret, local_err);
>           }
>       }
>   out:
> @@ -1224,13 +1229,15 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>       VFIOContainerBase *bcontainer = giommu->bcontainer;
>       hwaddr iova = iotlb->iova + giommu->iommu_offset;
>       ram_addr_t translated_addr;
> +    Error *local_err = NULL;
>       int ret = -EINVAL;
>
>       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;
>       }
>
> @@ -1239,17 +1246,18 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>           ret = vfio_get_dirty_bitmap(bcontainer, iova, iotlb->addr_mask + 1,
>                                       translated_addr);

If vfio_get_xlat_addr() above (it's not shown here) returns false, we 
will pass a NULL local_err to vfio_set_migration_error() and it may 
de-reference NULL ptr in error_report_err().

Should we refactor vfio_get_xlat_addr() to get errp, or add an else 
branch below, setting -EINVAL (and removing the default -EINVAL from the 
top of the function)?

Thanks.

>           if (ret) {
> -            error_report("vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", "
> -                         "0x%"HWADDR_PRIx") = %d (%s)",
> -                         bcontainer, iova, iotlb->addr_mask + 1, ret,
> -                         strerror(-ret));
> +            error_setg(&local_err,
> +                       "vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", "
> +                       "0x%"HWADDR_PRIx") = %d (%s)",
> +                       bcontainer, iova, iotlb->addr_mask + 1, ret,
> +                       strerror(-ret));
>           }
>       }
>       rcu_read_unlock();
>
>   out:
>       if (ret) {
> -        vfio_set_migration_error(ret);
> +        vfio_set_migration_error(ret, local_err);
>       }
>   }
>
> @@ -1345,6 +1353,7 @@ static void vfio_listener_log_sync(MemoryListener *listener,
>   {
>       VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
>                                                    listener);
> +    Error *local_err = NULL;
>       int ret;
>
>       if (vfio_listener_skipped_section(section)) {
> @@ -1354,9 +1363,10 @@ static void vfio_listener_log_sync(MemoryListener *listener,
>       if (vfio_devices_all_dirty_tracking(bcontainer)) {
>           ret = vfio_sync_dirty_bitmap(bcontainer, section);
>           if (ret) {
> -            error_report("vfio: Failed to sync dirty bitmap, err: %d (%s)", ret,
> -                         strerror(-ret));
> -            vfio_set_migration_error(ret);
> +            error_setg(&local_err,
> +                       "vfio: Failed to sync dirty bitmap, err: %d (%s)", ret,
> +                       strerror(-ret));
> +            vfio_set_migration_error(ret, local_err);
>           }
>       }
>   }
> --
> 2.43.0
>
>

Re: [PATCH 13/14] migration: Use migrate_has_error() in close_return_path_on_source()

[Cédric Le Goater]

On 2/8/24 14:57, Fabiano Rosas wrote:
> Cédric Le Goater <clg@redhat.com> writes:
> 
>> On 2/8/24 14:07, Fabiano Rosas wrote:
>>> Cédric Le Goater <clg@redhat.com> writes:
>>>
>>>> close_return_path_on_source() retrieves the migration error from the
>>>> the QEMUFile '->to_dst_file' to know if a shutdown is required. This
>>>> shutdown is required to exit the return-path thread. However, in
>>>> migrate_fd_cleanup(), '->to_dst_file' is cleaned up before calling
>>>> close_return_path_on_source() and the shutdown is never performed,
>>>> leaving the source and destination waiting for an event to occur.
>>>>
>>>> Avoid relying on '->to_dst_file' and use migrate_has_error() instead.
>>>>
>>>> Suggested-by: Peter Xu <peterx@redhat.com>
>>>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>>>> ---
>>>>    migration/migration.c | 3 +--
>>>>    1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/migration/migration.c b/migration/migration.c
>>>> index d5f705ceef4c925589aa49335969672c0d761fa2..5f55af3d7624750ca416c4177781241b3e291e5d 100644
>>>> --- a/migration/migration.c
>>>> +++ b/migration/migration.c
>>>> @@ -2372,8 +2372,7 @@ static bool close_return_path_on_source(MigrationState *ms)
>>>>         * cause it to unblock if it's stuck waiting for the destination.
>>>>         */
>>>>        WITH_QEMU_LOCK_GUARD(&ms->qemu_file_lock) {
>>>> -        if (ms->to_dst_file && ms->rp_state.from_dst_file &&
>>>> -            qemu_file_get_error(ms->to_dst_file)) {
>>>> +        if (migrate_has_error(ms) && ms->rp_state.from_dst_file) {
>>>>                qemu_file_shutdown(ms->rp_state.from_dst_file);
>>>>            }
>>>>        }
>>>
>>> Hm, maybe Peter can help defend this, but this assumes that every
>>> function that takes an 'f' and sets the file error also sets
>>> migrate_set_error(). I'm not sure we have determined that, have we?
>>
>> How could we check all the code path ? I agree it is difficult when
>> looking at the code :/
> 
> It would help if the thing wasn't called 'f' for the most part of the
> code to begin with.
> 
> Whenever there's a file error at to_dst_file there's the chance that the
> rp_state.from_dst_file got stuck. So we cannot ignore the file error.
> 
> Would it work if we checked it earlier during cleanup as you did
> previously and then set the migration error?

Do you mean doing something similar to what is done in
source_return_path_thread() ?

         if (qemu_file_get_error(s->to_dst_file)) {
             qemu_file_get_error_obj(s->to_dst_file, &err);
     	    if (err) {
         	migrate_set_error(ms, err);
         	error_free(err);
	...

Yes. That would be safer I think.


Nevertheless, I am struggling to understand how qemu_file_set_error()
and migrate_set_error() fit together. I was expecting some kind of
synchronization  routine but there isn't it seems. Are they completely
orthogonal ? when should we use these routines and when not ?

My initial goal was to modify some of the memory handlers (log_global*)
and migration handlers to propagate errors at the QMP level and them
report to the management layer. This is growing in something bigger
and currently, I don't find a good approach to the problem.

The last two patches of this series try to fix the return-path thread
termination. Let's keep that for after.

Thanks,

C.

Re: [PATCH 01/14] migration: Add Error** argument to .save_setup() handler

[Cédric Le Goater]

On 2/12/24 09:36, Avihai Horon wrote:
> Hi, Cedric
> 
> On 07/02/2024 15:33, Cédric Le Goater wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> The purpose is to record a potential error in the migration stream if
>> qemu_savevm_state_setup() fails. Most of the current .save_setup()
>> handlers can be modified to use the Error argument instead of managing
>> their own and calling locally error_report(). The following patches
>> will introduce such changes for VFIO first.
>>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>   include/migration/register.h   | 2 +-
>>   hw/ppc/spapr.c                 | 2 +-
>>   hw/s390x/s390-stattrib.c       | 2 +-
>>   hw/vfio/migration.c            | 2 +-
>>   migration/block-dirty-bitmap.c | 2 +-
>>   migration/block.c              | 2 +-
>>   migration/ram.c                | 2 +-
>>   migration/savevm.c             | 4 ++--
>>   8 files changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/migration/register.h b/include/migration/register.h
>> index 9ab1f79512c605f0c88a45b560c57486fa054441..831600a00eae4efd0464b60925d65de4d9dbcff8 100644
>> --- a/include/migration/register.h
>> +++ b/include/migration/register.h
>> @@ -25,7 +25,7 @@ typedef struct SaveVMHandlers {
>>        * used to perform early checks.
>>        */
>>       int (*save_prepare)(void *opaque, Error **errp);
>> -    int (*save_setup)(QEMUFile *f, void *opaque);
>> +    int (*save_setup)(QEMUFile *f, void *opaque, Error **errp);
>>       void (*save_cleanup)(void *opaque);
>>       int (*save_live_complete_postcopy)(QEMUFile *f, void *opaque);
>>       int (*save_live_complete_precopy)(QEMUFile *f, void *opaque);
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 0d72d286d80f0435122593555f79fae4d90acf81..a1b0aa02582ad2d68a13476c1859b18143da7bb8 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -2142,7 +2142,7 @@ static const VMStateDescription vmstate_spapr = {
>>       }
>>   };
>>
>> -static int htab_save_setup(QEMUFile *f, void *opaque)
>> +static int htab_save_setup(QEMUFile *f, void *opaque, Error **errp)
>>   {
>>       SpaprMachineState *spapr = opaque;
>>
>> diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c
>> index c483b62a9b5f71772639fc180bdad15ecb6711cb..c934df424a555d83d2198f5ddfc0cbe0ea98e9ec 100644
>> --- a/hw/s390x/s390-stattrib.c
>> +++ b/hw/s390x/s390-stattrib.c
>> @@ -166,7 +166,7 @@ static int cmma_load(QEMUFile *f, void *opaque, int version_id)
>>       return ret;
>>   }
>>
>> -static int cmma_save_setup(QEMUFile *f, void *opaque)
>> +static int cmma_save_setup(QEMUFile *f, void *opaque, Error **errp)
>>   {
>>       S390StAttribState *sas = S390_STATTRIB(opaque);
>>       S390StAttribClass *sac = S390_STATTRIB_GET_CLASS(sas);
>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>> index 70e6b1a709f9b67e4c9eb41033d76347275cac42..8bcb4bc73cd5ba5338e3ffa4d907d0e6bfbb9485 100644
>> --- a/hw/vfio/migration.c
>> +++ b/hw/vfio/migration.c
>> @@ -378,7 +378,7 @@ static int vfio_save_prepare(void *opaque, Error **errp)
>>       return 0;
>>   }
>>
>> -static int vfio_save_setup(QEMUFile *f, void *opaque)
>> +static int vfio_save_setup(QEMUFile *f, void *opaque, Error **errp)
>>   {
>>       VFIODevice *vbasedev = opaque;
>>       VFIOMigration *migration = vbasedev->migration;
>> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
>> index 2708abf3d762de774ed294d3fdb8e56690d2974c..16f84e6c57c2403a8c2d6319f4e7b6360dade28c 100644
>> --- a/migration/block-dirty-bitmap.c
>> +++ b/migration/block-dirty-bitmap.c
>> @@ -1213,7 +1213,7 @@ fail:
>>       return ret;
>>   }
>>
>> -static int dirty_bitmap_save_setup(QEMUFile *f, void *opaque)
>> +static int dirty_bitmap_save_setup(QEMUFile *f, void *opaque, Error **errp)
>>   {
>>       DBMSaveState *s = &((DBMState *)opaque)->save;
>>       SaveBitmapState *dbms = NULL;
>> diff --git a/migration/block.c b/migration/block.c
>> index 8c6ebafacc1ffe930d1d4f19d968817b14852c69..df15319ceab66201b043f15eac1b0a7d6522b60c 100644
>> --- a/migration/block.c
>> +++ b/migration/block.c
>> @@ -708,7 +708,7 @@ static void block_migration_cleanup(void *opaque)
>>       blk_mig_unlock();
>>   }
>>
>> -static int block_save_setup(QEMUFile *f, void *opaque)
>> +static int block_save_setup(QEMUFile *f, void *opaque, Error **errp)
>>   {
>>       int ret;
>>
>> diff --git a/migration/ram.c b/migration/ram.c
>> index d5b7cd5ac2f31aabf4a248b966153401c48912cf..136c237f4079f68d4e578cf1c72eec2efc815bc8 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -2931,7 +2931,7 @@ void qemu_guest_free_page_hint(void *addr, size_t len)
>>    * @f: QEMUFile where to send the data
>>    * @opaque: RAMState pointer
>>    */
>> -static int ram_save_setup(QEMUFile *f, void *opaque)
>> +static int ram_save_setup(QEMUFile *f, void *opaque, Error **errp)
>>   {
>>       RAMState **rsp = opaque;
>>       RAMBlock *block;
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index d612c8a9020b204d5d078d5df85f0e6449c27645..f2ae799bad13e631bccf733a34c3a8fd22e8dd48 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -1342,10 +1342,10 @@ void qemu_savevm_state_setup(QEMUFile *f)
>>           }
>>           save_section_header(f, se, QEMU_VM_SECTION_START);
>>
>> -        ret = se->ops->save_setup(f, se->opaque);
>> +        ret = se->ops->save_setup(f, se->opaque, &local_err);
>>           save_section_footer(f, se);
>>           if (ret < 0) {
>> -            qemu_file_set_error(f, ret);
>> +            qemu_file_set_error_obj(f, ret, local_err);
> 
> Should we set local_err = NULL? 

possibly, yes.

> Because it is re-used a few lines after this, by precopy_notify().

I wonder why is precopy_notify(PRECOPY_NOTIFY_SETUP) even called when
there was an error in one of the save_setup() handlers. It probably
shouldn't and qemu_savevm_state_setup() should return at the first
error in the loop. This is something that could have been overlooked
by commit bd2270608fa0 "migration/ram.c: add a notifier chain for
precopy" because qemu_savevm_state_setup() does not have a return
value. Probably because the callers rely on qemu_file_get_error()
to know if something wrong happened.

Also, the only user of PRECOPY_NOTIFY_SETUP is virtio-balloon and
nothing is done. PrecopyNotifyData has an errp attribute which is
unused.

> 
> BTW, I think that if we add Error** parameter to functions we must make sure all their error flows set errp as well.
> According to Error API:
> * - On success, the function should not touch *errp.  On failure, it
> *   should set a new error, e.g. with error_setg(errp, ...), or
> *   propagate an existing one, e.g. with error_propagate(errp, ...).
> 
> For example, a caller that handles errors by printing them with error_report_err() would crash when trying to access NULL error object (if some error path didn't set errp).

One of the underlying goal is to avoid and remove all error_report_err()
calls to propagate the error up the call stack.

> If you agree, we should check it throughout the series.

I do agree and this is a can of worms ! I haven't quite found my way
around yet.

Thanks for your inputs,

C.

Re: [RFC PATCH 14/14] migration: Fix return-path thread exit

[Cédric Le Goater]

Hello Fabiano

On 2/8/24 14:29, Fabiano Rosas wrote:
> Cédric Le Goater <clg@redhat.com> writes:
> 
>> In case of error, close_return_path_on_source() can perform a shutdown
>> to exit the return-path thread.  However, in migrate_fd_cleanup(),
>> 'to_dst_file' is closed before calling close_return_path_on_source()
>> and the shutdown fails, leaving the source and destination waiting for
>> an event to occur.
> 
> Hi, Cédric
> 
> Are you sure this is not caused by patch 13? 

It happens with upstream QEMU without any patch.

When vfio_listener_log_global_start() fails, it sets an error on the
QEMUFile. To reproduce without a VFIO device, you can inject an error
when dirty tracking is started. Something like below,

     @@ -2817,6 +2817,8 @@ static void ram_init_bitmaps(RAMState *r
           * containing all 1s to exclude any discarded pages from migration.
           */
          migration_bitmap_clear_discarded_pages(rs);
     +
     +    qemu_file_set_error(migrate_get_current()->to_dst_file, -EAGAIN);
      }
      
      static int ram_init_all(RAMState **rsp)

Activate return-path and migrate.

> That 'if (ms->to_dst_file'
> was there to avoid this sort of thing happening.
> 
> Is there some reordering possibility that I'm not spotting in the code
> below? I think the data dependency on to_dst_file shouldn't allow it.
> 
> migrate_fd_cleanup:
>          qemu_mutex_lock(&s->qemu_file_lock);
>          tmp = s->to_dst_file;
>          s->to_dst_file = NULL;
>          qemu_mutex_unlock(&s->qemu_file_lock);
>          ...
>          qemu_fclose(tmp);
> 
> close_return_path_on_source:
>      WITH_QEMU_LOCK_GUARD(&ms->qemu_file_lock) {
>          if (ms->to_dst_file && ms->rp_state.from_dst_file &&
>              qemu_file_get_error(ms->to_dst_file)) {
>              qemu_file_shutdown(ms->rp_state.from_dst_file);
>          }
>      }

close_return_path_on_source() is called by migrate_fd_cleanup() in
the same thread. So, when we reach the locking section ms->to_dst_file
is already NULL and qemu_fclose() has been closed :/

May be I misunderstood. Please try to reproduce with the little hack
above.

Thanks,

C.

> I'm thinking maybe the culprit is the close_return_path_on_source() at
> migration_completion(). It might be possible for it to race with the
> migrate_fd_cleanup_bh from migration_iteration_finish().
> 
> If that's the case, then I think that one possible fix would be to hold
> the BQL at migration_completion() so the BH doesn't get dispatched until
> we properly close the return path.
> 
>>
>> Close the file after calling close_return_path_on_source() so that the
>> shutdown succeeds and the return-path thread exits.
>>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>
>>   This is an RFC because the correct fix implies reworking the QEMUFile
>>   construct, built on top of the QEMU I/O channel.
>>
>>   migration/migration.c | 13 ++++++-------
>>   1 file changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 5f55af3d7624750ca416c4177781241b3e291e5d..de329f2c553288935d824748286e79e535929b8b 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -1313,6 +1313,8 @@ void migrate_set_state(int *state, int old_state, int new_state)
>>   
>>   static void migrate_fd_cleanup(MigrationState *s)
>>   {
>> +    QEMUFile *tmp = NULL;
>> +
>>       g_free(s->hostname);
>>       s->hostname = NULL;
>>       json_writer_free(s->vmdesc);
>> @@ -1321,8 +1323,6 @@ static void migrate_fd_cleanup(MigrationState *s)
>>       qemu_savevm_state_cleanup();
>>   
>>       if (s->to_dst_file) {
>> -        QEMUFile *tmp;
>> -
>>           trace_migrate_fd_cleanup();
>>           bql_unlock();
>>           if (s->migration_thread_running) {
>> @@ -1341,15 +1341,14 @@ static void migrate_fd_cleanup(MigrationState *s)
>>            * critical section won't block for long.
>>            */
>>           migration_ioc_unregister_yank_from_file(tmp);
>> -        qemu_fclose(tmp);
>>       }
>>   
>> -    /*
>> -     * We already cleaned up to_dst_file, so errors from the return
>> -     * path might be due to that, ignore them.
>> -     */
>>       close_return_path_on_source(s);
>>   
>> +    if (tmp) {
>> +        qemu_fclose(tmp);
>> +    }
>> +
>>       assert(!migration_is_active(s));
>>   
>>       if (s->state == MIGRATION_STATUS_CANCELLING) {
> 

Re: [PATCH 01/14] migration: Add Error** argument to .save_setup() handler

[Avihai Horon]

On 12/02/2024 16:49, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> On 2/12/24 09:36, Avihai Horon wrote:
>> Hi, Cedric
>>
>> On 07/02/2024 15:33, Cédric Le Goater wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> The purpose is to record a potential error in the migration stream if
>>> qemu_savevm_state_setup() fails. Most of the current .save_setup()
>>> handlers can be modified to use the Error argument instead of managing
>>> their own and calling locally error_report(). The following patches
>>> will introduce such changes for VFIO first.
>>>
>>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>>> ---
>>>   include/migration/register.h   | 2 +-
>>>   hw/ppc/spapr.c                 | 2 +-
>>>   hw/s390x/s390-stattrib.c       | 2 +-
>>>   hw/vfio/migration.c            | 2 +-
>>>   migration/block-dirty-bitmap.c | 2 +-
>>>   migration/block.c              | 2 +-
>>>   migration/ram.c                | 2 +-
>>>   migration/savevm.c             | 4 ++--
>>>   8 files changed, 9 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/include/migration/register.h 
>>> b/include/migration/register.h
>>> index 
>>> 9ab1f79512c605f0c88a45b560c57486fa054441..831600a00eae4efd0464b60925d65de4d9dbcff8 
>>> 100644
>>> --- a/include/migration/register.h
>>> +++ b/include/migration/register.h
>>> @@ -25,7 +25,7 @@ typedef struct SaveVMHandlers {
>>>        * used to perform early checks.
>>>        */
>>>       int (*save_prepare)(void *opaque, Error **errp);
>>> -    int (*save_setup)(QEMUFile *f, void *opaque);
>>> +    int (*save_setup)(QEMUFile *f, void *opaque, Error **errp);
>>>       void (*save_cleanup)(void *opaque);
>>>       int (*save_live_complete_postcopy)(QEMUFile *f, void *opaque);
>>>       int (*save_live_complete_precopy)(QEMUFile *f, void *opaque);
>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>> index 
>>> 0d72d286d80f0435122593555f79fae4d90acf81..a1b0aa02582ad2d68a13476c1859b18143da7bb8 
>>> 100644
>>> --- a/hw/ppc/spapr.c
>>> +++ b/hw/ppc/spapr.c
>>> @@ -2142,7 +2142,7 @@ static const VMStateDescription vmstate_spapr = {
>>>       }
>>>   };
>>>
>>> -static int htab_save_setup(QEMUFile *f, void *opaque)
>>> +static int htab_save_setup(QEMUFile *f, void *opaque, Error **errp)
>>>   {
>>>       SpaprMachineState *spapr = opaque;
>>>
>>> diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c
>>> index 
>>> c483b62a9b5f71772639fc180bdad15ecb6711cb..c934df424a555d83d2198f5ddfc0cbe0ea98e9ec 
>>> 100644
>>> --- a/hw/s390x/s390-stattrib.c
>>> +++ b/hw/s390x/s390-stattrib.c
>>> @@ -166,7 +166,7 @@ static int cmma_load(QEMUFile *f, void *opaque, 
>>> int version_id)
>>>       return ret;
>>>   }
>>>
>>> -static int cmma_save_setup(QEMUFile *f, void *opaque)
>>> +static int cmma_save_setup(QEMUFile *f, void *opaque, Error **errp)
>>>   {
>>>       S390StAttribState *sas = S390_STATTRIB(opaque);
>>>       S390StAttribClass *sac = S390_STATTRIB_GET_CLASS(sas);
>>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>>> index 
>>> 70e6b1a709f9b67e4c9eb41033d76347275cac42..8bcb4bc73cd5ba5338e3ffa4d907d0e6bfbb9485 
>>> 100644
>>> --- a/hw/vfio/migration.c
>>> +++ b/hw/vfio/migration.c
>>> @@ -378,7 +378,7 @@ static int vfio_save_prepare(void *opaque, Error 
>>> **errp)
>>>       return 0;
>>>   }
>>>
>>> -static int vfio_save_setup(QEMUFile *f, void *opaque)
>>> +static int vfio_save_setup(QEMUFile *f, void *opaque, Error **errp)
>>>   {
>>>       VFIODevice *vbasedev = opaque;
>>>       VFIOMigration *migration = vbasedev->migration;
>>> diff --git a/migration/block-dirty-bitmap.c 
>>> b/migration/block-dirty-bitmap.c
>>> index 
>>> 2708abf3d762de774ed294d3fdb8e56690d2974c..16f84e6c57c2403a8c2d6319f4e7b6360dade28c 
>>> 100644
>>> --- a/migration/block-dirty-bitmap.c
>>> +++ b/migration/block-dirty-bitmap.c
>>> @@ -1213,7 +1213,7 @@ fail:
>>>       return ret;
>>>   }
>>>
>>> -static int dirty_bitmap_save_setup(QEMUFile *f, void *opaque)
>>> +static int dirty_bitmap_save_setup(QEMUFile *f, void *opaque, Error 
>>> **errp)
>>>   {
>>>       DBMSaveState *s = &((DBMState *)opaque)->save;
>>>       SaveBitmapState *dbms = NULL;
>>> diff --git a/migration/block.c b/migration/block.c
>>> index 
>>> 8c6ebafacc1ffe930d1d4f19d968817b14852c69..df15319ceab66201b043f15eac1b0a7d6522b60c 
>>> 100644
>>> --- a/migration/block.c
>>> +++ b/migration/block.c
>>> @@ -708,7 +708,7 @@ static void block_migration_cleanup(void *opaque)
>>>       blk_mig_unlock();
>>>   }
>>>
>>> -static int block_save_setup(QEMUFile *f, void *opaque)
>>> +static int block_save_setup(QEMUFile *f, void *opaque, Error **errp)
>>>   {
>>>       int ret;
>>>
>>> diff --git a/migration/ram.c b/migration/ram.c
>>> index 
>>> d5b7cd5ac2f31aabf4a248b966153401c48912cf..136c237f4079f68d4e578cf1c72eec2efc815bc8 
>>> 100644
>>> --- a/migration/ram.c
>>> +++ b/migration/ram.c
>>> @@ -2931,7 +2931,7 @@ void qemu_guest_free_page_hint(void *addr, 
>>> size_t len)
>>>    * @f: QEMUFile where to send the data
>>>    * @opaque: RAMState pointer
>>>    */
>>> -static int ram_save_setup(QEMUFile *f, void *opaque)
>>> +static int ram_save_setup(QEMUFile *f, void *opaque, Error **errp)
>>>   {
>>>       RAMState **rsp = opaque;
>>>       RAMBlock *block;
>>> diff --git a/migration/savevm.c b/migration/savevm.c
>>> index 
>>> d612c8a9020b204d5d078d5df85f0e6449c27645..f2ae799bad13e631bccf733a34c3a8fd22e8dd48 
>>> 100644
>>> --- a/migration/savevm.c
>>> +++ b/migration/savevm.c
>>> @@ -1342,10 +1342,10 @@ void qemu_savevm_state_setup(QEMUFile *f)
>>>           }
>>>           save_section_header(f, se, QEMU_VM_SECTION_START);
>>>
>>> -        ret = se->ops->save_setup(f, se->opaque);
>>> +        ret = se->ops->save_setup(f, se->opaque, &local_err);
>>>           save_section_footer(f, se);
>>>           if (ret < 0) {
>>> -            qemu_file_set_error(f, ret);
>>> +            qemu_file_set_error_obj(f, ret, local_err);
>>
>> Should we set local_err = NULL?
>
> possibly, yes.
>
>> Because it is re-used a few lines after this, by precopy_notify().
>
> I wonder why is precopy_notify(PRECOPY_NOTIFY_SETUP) even called when
> there was an error in one of the save_setup() handlers. It probably
> shouldn't and qemu_savevm_state_setup() should return at the first
> error in the loop. This is something that could have been overlooked
> by commit bd2270608fa0 "migration/ram.c: add a notifier chain for
> precopy" because qemu_savevm_state_setup() does not have a return
> value. Probably because the callers rely on qemu_file_get_error()
> to know if something wrong happened.

Yes, I guess here we could return early and skip precopy_notify().

>
> Also, the only user of PRECOPY_NOTIFY_SETUP is virtio-balloon and
> nothing is done. PrecopyNotifyData has an errp attribute which is
> unused.

You are right, with current code there won't be any problem, but new 
code that will make use of the errp can be problematic.

>
>>
>> BTW, I think that if we add Error** parameter to functions we must 
>> make sure all their error flows set errp as well.
>> According to Error API:
>> * - On success, the function should not touch *errp.  On failure, it
>> *   should set a new error, e.g. with error_setg(errp, ...), or
>> *   propagate an existing one, e.g. with error_propagate(errp, ...).
>>
>> For example, a caller that handles errors by printing them with 
>> error_report_err() would crash when trying to access NULL error 
>> object (if some error path didn't set errp).
>
> One of the underlying goal is to avoid and remove all error_report_err()
> calls to propagate the error up the call stack.

Yes, I just gave an example of how this could go wrong.
I think the general point here is that a caller that provides a valid 
Error** errp assumes he will get an Error object from the callee in case 
of an error, and the caller can operate on it as he wants.

>
>> If you agree, we should check it throughout the series.
>
> I do agree and this is a can of worms !

Indeed.

> I haven't quite found my way
> around yet.

Briefly looking, it seems like .save_setup() / .load_setup() shouldn't 
be too hard.
However, memory stuff seem to be more involved.

Re: [RFC PATCH 14/14] migration: Fix return-path thread exit

[Cédric Le Goater]

Hello Peter

On 2/8/24 06:57, Peter Xu wrote:
> On Wed, Feb 07, 2024 at 02:33:47PM +0100, Cédric Le Goater wrote:
>> In case of error, close_return_path_on_source() can perform a shutdown
>> to exit the return-path thread.  However, in migrate_fd_cleanup(),
>> 'to_dst_file' is closed before calling close_return_path_on_source()
>> and the shutdown fails, leaving the source and destination waiting for
>> an event to occur.
>>
>> Close the file after calling close_return_path_on_source() so that the
>> shutdown succeeds and the return-path thread exits.
>>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>
>>   This is an RFC because the correct fix implies reworking the QEMUFile
>>   construct, built on top of the QEMU I/O channel.
>>
>>   migration/migration.c | 13 ++++++-------
>>   1 file changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 5f55af3d7624750ca416c4177781241b3e291e5d..de329f2c553288935d824748286e79e535929b8b 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -1313,6 +1313,8 @@ void migrate_set_state(int *state, int old_state, int new_state)
>>   
>>   static void migrate_fd_cleanup(MigrationState *s)
>>   {
>> +    QEMUFile *tmp = NULL;
>> +
>>       g_free(s->hostname);
>>       s->hostname = NULL;
>>       json_writer_free(s->vmdesc);
>> @@ -1321,8 +1323,6 @@ static void migrate_fd_cleanup(MigrationState *s)
>>       qemu_savevm_state_cleanup();
>>   
>>       if (s->to_dst_file) {
>> -        QEMUFile *tmp;
>> -
>>           trace_migrate_fd_cleanup();
>>           bql_unlock();
>>           if (s->migration_thread_running) {
>> @@ -1341,15 +1341,14 @@ static void migrate_fd_cleanup(MigrationState *s)
>>            * critical section won't block for long.
>>            */
>>           migration_ioc_unregister_yank_from_file(tmp);
>> -        qemu_fclose(tmp);
>>       }
>>   
>> -    /*
>> -     * We already cleaned up to_dst_file, so errors from the return
>> -     * path might be due to that, ignore them.
>> -     */
>>       close_return_path_on_source(s);
>>   
>> +    if (tmp) {
>> +        qemu_fclose(tmp);
>> +    }
>> +
>>       assert(!migration_is_active(s));
>>   
>>       if (s->state == MIGRATION_STATUS_CANCELLING) {
> 
> I think this is okay to me for a short term plan.  I'll see how others
> think, also add Dan into the loop.
> 
> If so, would you please add a rich comment explaining why tmp needs to be
> closed later?  Especially, explicit comment on the ordering requirement
> would be helpful: IMHO here it's an order that qemu_fclose() must happen
> after close_return_path_on_source().  So when others work on this code we
> don't easily break it without noticing.

Sure. I will when we have clarified with Fabiano what is the best
approach.

> Also please feel free to post separately on migration patches if you'd like
> us to merge the patches when repost.

This series is a collection of multiple (related) changes :

* extra Error** parameter to save_setup() migration handlers.
   This change has consequences on the various callers which are not
   fully analyzed.
* similar changes for memory logging handlers. These looks more self
   contained and I will see if I can send then separately.
* return-path thread termination

and then, in background we have open questions regarding :

* the QEMUfile implementation and its QIOChannel usage for migration
   streams
* qemu_file_set_error* vs. migrate_set_error. It is confusing, at least
   for me. Do we have some documentation on best practices ?

Thanks,

C.

Re: [PATCH 03/14] memory: Add Error** argument to .log_global*() handlers

[Cédric Le Goater]

On 2/12/24 09:43, Avihai Horon wrote:
> Hi Cedric,
> 
> On 09/02/2024 12:14, Cédric Le Goater wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On 2/8/24 06:48, Peter Xu wrote:
>>> On Wed, Feb 07, 2024 at 02:33:36PM +0100, Cédric Le Goater wrote:
>>>> @@ -2936,14 +2940,14 @@ void memory_global_dirty_log_start(unsigned int flags)
>>>>       trace_global_dirty_changed(global_dirty_tracking);
>>>>
>>>>       if (!old_flags) {
>>>> -        MEMORY_LISTENER_CALL_GLOBAL(log_global_start, Forward);
>>>> +        MEMORY_LISTENER_CALL_GLOBAL(log_global_start, Forward, errp);
>>>>           memory_region_transaction_begin();
>>>>           memory_region_update_pending = true;
>>>>           memory_region_transaction_commit();
>>>>       }
>>>>   }
>>>>
>>>> -static void memory_global_dirty_log_do_stop(unsigned int flags)
>>>> +static void memory_global_dirty_log_do_stop(unsigned int flags, Error **errp)
>>>>   {
>>>>       assert(flags && !(flags & (~GLOBAL_DIRTY_MASK)));
>>>>       assert((global_dirty_tracking & flags) == flags);
>>>> @@ -2955,7 +2959,7 @@ static void memory_global_dirty_log_do_stop(unsigned int flags)
>>>>           memory_region_transaction_begin();
>>>>           memory_region_update_pending = true;
>>>>           memory_region_transaction_commit();
>>>> -        MEMORY_LISTENER_CALL_GLOBAL(log_global_stop, Reverse);
>>>> +        MEMORY_LISTENER_CALL_GLOBAL(log_global_stop, Reverse, errp);
>>>>       }
>>>>   }
>>>
>>> I'm a little bit surprised to see that MEMORY_LISTENER_CALL_GLOBAL()
>>> already allows >2 args, with the ability to conditionally pass over errp
>>> with such oneliner change; even if all callers were only using 2 args
>>> before this patch..
>> yes. The proposal takes the easy path.
>>
>> Should we change all memory listener global handlers :
>>
>>   begin
>>   commit
>>   log_global_after_sync
>>   log_global_start
>>   log_global_stop
>>
>> to take an extra Error **errp argument ?
>>
>> I think we should distinguish begin + commit handlers from the log_global_*
>> with a new macro. In which case, we could also change the handler to return
>> a bool and fail at the first error in MEMORY_LISTENER_CALL_GLOBAL(...).
> 
> I think we must fail at first error in any case. Otherwise, if two handlers error and call error_setg() with errp, the second handler will assert IIUC.

Good point. I will respin with a new MEMORY_LISTENER_CALL_GLOBAL_ERR macro
exiting the loop at first error.

Thanks,

C.

Re: [PATCH 04/14] migration: Modify ram_init_bitmaps() to report dirty tracking errors

[Cédric Le Goater]

On 2/12/24 09:51, Avihai Horon wrote:
> Hi Cedric,
> 
> On 07/02/2024 15:33, Cédric Le Goater wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> The .save_setup() handler has now an Error** argument that we can use
>> to propagate errors reported by the .log_global_start() handler. Do
>> that for the RAM. qemu_savevm_state_setup() will store the error under
>> the migration stream for later detection in the migration sequence.
>>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>   migration/ram.c | 19 ++++++++++---------
>>   1 file changed, 10 insertions(+), 9 deletions(-)
>>
>> diff --git a/migration/ram.c b/migration/ram.c
>> index d86626bb1c704b2d3497b323a702ca6ca8939a79..b87245466bb46937fd0358d0c66432bcc6280018 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -2802,19 +2802,17 @@ static void migration_bitmap_clear_discarded_pages(RAMState *rs)
>>       }
>>   }
>>
>> -static void ram_init_bitmaps(RAMState *rs)
>> +static void ram_init_bitmaps(RAMState *rs, Error **errp)
>>   {
>> -    Error *local_err = NULL;
>> -
>>       qemu_mutex_lock_ramlist();
>>
>>       WITH_RCU_READ_LOCK_GUARD() {
>>           ram_list_init_bitmaps();
>>           /* We don't use dirty log with background snapshots */
>>           if (!migrate_background_snapshot()) {
>> -            memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, &local_err);
>> -            if (local_err) {
>> -                error_report_err(local_err);
>> +            memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, errp);
>> +            if (*errp) {
> 
> I think we should use ERRP_GUARD() or a local error here and also below at ram_init_bitmaps() (or return bool like Philippe suggested).

yes. I will rework that part.

Thanks,

C.


> 
> Thanks.
> 
>> +                break;
>>               }
>>               migration_bitmap_sync_precopy(rs, false);
>>           }
>> @@ -2828,7 +2826,7 @@ static void ram_init_bitmaps(RAMState *rs)
>>       migration_bitmap_clear_discarded_pages(rs);
>>   }
>>
>> -static int ram_init_all(RAMState **rsp)
>> +static int ram_init_all(RAMState **rsp, Error **errp)
>>   {
>>       if (ram_state_init(rsp)) {
>>           return -1;
>> @@ -2839,7 +2837,10 @@ static int ram_init_all(RAMState **rsp)
>>           return -1;
>>       }
>>
>> -    ram_init_bitmaps(*rsp);
>> +    ram_init_bitmaps(*rsp, errp);
>> +    if (*errp) {
>> +        return -1;
>> +    }
>>
>>       return 0;
>>   }
>> @@ -2952,7 +2953,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque, Error **errp)
>>
>>       /* migration has already setup the bitmap, reuse it. */
>>       if (!migration_in_colo_state()) {
>> -        if (ram_init_all(rsp) != 0) {
>> +        if (ram_init_all(rsp, errp) != 0) {
>>               compress_threads_save_cleanup();
>>               return -1;
>>           }
>> -- 
>> 2.43.0
>>
>>
> 

Re: [PATCH 08/14] vfio: Use new Error** argument in vfio_save_setup()

[Cédric Le Goater]

On 2/12/24 10:17, Avihai Horon wrote:
> Hi Cedric,
> 
> On 07/02/2024 15:33, Cédric Le Goater wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> Add an Error** argument to vfio_migration_set_state() and adjust
>> callers, including vfio_save_setup(). The error will be propagated up
>> to qemu_savevm_state_setup() where the save_setup() handler is
>> executed.
>>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>   hw/vfio/migration.c | 62 +++++++++++++++++++++++++++++----------------
>>   1 file changed, 40 insertions(+), 22 deletions(-)
>>
>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>> index 2dfbe671f6f45aa530c7341177bb532d8292cecd..2e0a79967cc97f44d9be5575c3cfe18c9f349dab 100644
>> --- a/hw/vfio/migration.c
>> +++ b/hw/vfio/migration.c
>> @@ -84,7 +84,8 @@ static const char *mig_state_to_str(enum vfio_device_mig_state state)
>>
>>   static int vfio_migration_set_state(VFIODevice *vbasedev,
>>                                       enum vfio_device_mig_state new_state,
>> -                                    enum vfio_device_mig_state recover_state)
>> +                                    enum vfio_device_mig_state recover_state,
>> +                                    Error **errp)
>>   {
>>       VFIOMigration *migration = vbasedev->migration;
>>       uint64_t buf[DIV_ROUND_UP(sizeof(struct vfio_device_feature) +
>> @@ -104,15 +105,15 @@ static int vfio_migration_set_state(VFIODevice *vbasedev,
>>           ret = -errno;
>>
>>           if (recover_state == VFIO_DEVICE_STATE_ERROR) {
>> -            error_report("%s: Failed setting device state to %s, err: %s. "
>> -                         "Recover state is ERROR. Resetting device",
>> -                         vbasedev->name, mig_state_to_str(new_state),
>> -                         strerror(errno));
>> +            error_setg(errp, "%s: Failed setting device state to %s, err: %s. "
>> +                       "Recover state is ERROR. Resetting device",
>> +                       vbasedev->name, mig_state_to_str(new_state),
>> +                       strerror(errno));
>>
>>               goto reset_device;
>>           }
>>
>> -        error_report(
>> +        error_setg(errp,
>>               "%s: Failed setting device state to %s, err: %s. Setting device in recover state %s",
>>                        vbasedev->name, mig_state_to_str(new_state),
>>                        strerror(errno), mig_state_to_str(recover_state));
>> @@ -120,7 +121,7 @@ static int vfio_migration_set_state(VFIODevice *vbasedev,
>>           mig_state->device_state = recover_state;
>>           if (ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature)) {
>>               ret = -errno;
>> -            error_report(
>> +            error_setg(errp,
>>                   "%s: Failed setting device in recover state, err: %s. Resetting device",
>>                            vbasedev->name, strerror(errno));
> 
> I think here we will assert because errp is already set.
> 
> Adding an error_append() API would be useful here I guess.

yes.

> Otherwise, we need to move the first error_setg() below, to before we return from a successful recover state change, and construct the error message differently (e.g., provide a full error message for the recover state fail case containing also the first error).
> 
> Do you have other ideas?

Errors for :

     if (ioctl(vbasedev->fd, VFIO_DEVICE_RESET)) {

should be treated as the others with and error_append() and not
hw_error(). This needs a rework before any new changes.

I also wonder why we have twice :

     migration->device_state = recover_state;

It looks redundant. The ioctl VFIO_DEVICE_FEATURE should leave the
state unmodified.

Thanks,

C.

Re: [PATCH 08/14] vfio: Use new Error** argument in vfio_save_setup()

[Avihai Horon]

On 12/02/2024 19:54, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> On 2/12/24 10:17, Avihai Horon wrote:
>> Hi Cedric,
>>
>> On 07/02/2024 15:33, Cédric Le Goater wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> Add an Error** argument to vfio_migration_set_state() and adjust
>>> callers, including vfio_save_setup(). The error will be propagated up
>>> to qemu_savevm_state_setup() where the save_setup() handler is
>>> executed.
>>>
>>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>>> ---
>>>   hw/vfio/migration.c | 62 
>>> +++++++++++++++++++++++++++++----------------
>>>   1 file changed, 40 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>>> index 
>>> 2dfbe671f6f45aa530c7341177bb532d8292cecd..2e0a79967cc97f44d9be5575c3cfe18c9f349dab 
>>> 100644
>>> --- a/hw/vfio/migration.c
>>> +++ b/hw/vfio/migration.c
>>> @@ -84,7 +84,8 @@ static const char *mig_state_to_str(enum 
>>> vfio_device_mig_state state)
>>>
>>>   static int vfio_migration_set_state(VFIODevice *vbasedev,
>>>                                       enum vfio_device_mig_state 
>>> new_state,
>>> -                                    enum vfio_device_mig_state 
>>> recover_state)
>>> +                                    enum vfio_device_mig_state 
>>> recover_state,
>>> +                                    Error **errp)
>>>   {
>>>       VFIOMigration *migration = vbasedev->migration;
>>>       uint64_t buf[DIV_ROUND_UP(sizeof(struct vfio_device_feature) +
>>> @@ -104,15 +105,15 @@ static int vfio_migration_set_state(VFIODevice 
>>> *vbasedev,
>>>           ret = -errno;
>>>
>>>           if (recover_state == VFIO_DEVICE_STATE_ERROR) {
>>> -            error_report("%s: Failed setting device state to %s, 
>>> err: %s. "
>>> -                         "Recover state is ERROR. Resetting device",
>>> -                         vbasedev->name, mig_state_to_str(new_state),
>>> -                         strerror(errno));
>>> +            error_setg(errp, "%s: Failed setting device state to 
>>> %s, err: %s. "
>>> +                       "Recover state is ERROR. Resetting device",
>>> +                       vbasedev->name, mig_state_to_str(new_state),
>>> +                       strerror(errno));
>>>
>>>               goto reset_device;
>>>           }
>>>
>>> -        error_report(
>>> +        error_setg(errp,
>>>               "%s: Failed setting device state to %s, err: %s. 
>>> Setting device in recover state %s",
>>>                        vbasedev->name, mig_state_to_str(new_state),
>>>                        strerror(errno), 
>>> mig_state_to_str(recover_state));
>>> @@ -120,7 +121,7 @@ static int vfio_migration_set_state(VFIODevice 
>>> *vbasedev,
>>>           mig_state->device_state = recover_state;
>>>           if (ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature)) {
>>>               ret = -errno;
>>> -            error_report(
>>> +            error_setg(errp,
>>>                   "%s: Failed setting device in recover state, err: 
>>> %s. Resetting device",
>>>                            vbasedev->name, strerror(errno));
>>
>> I think here we will assert because errp is already set.
>>
>> Adding an error_append() API would be useful here I guess.
>
> yes.
>
>> Otherwise, we need to move the first error_setg() below, to before we 
>> return from a successful recover state change, and construct the 
>> error message differently (e.g., provide a full error message for the 
>> recover state fail case containing also the first error).
>>
>> Do you have other ideas?
>
> Errors for :
>
>     if (ioctl(vbasedev->fd, VFIO_DEVICE_RESET)) {
>
> should be treated as the others with and error_append() and not
> hw_error(). This needs a rework before any new changes.
>
> I also wonder why we have twice :
>
>     migration->device_state = recover_state;

Not sure where you see we have it twice.

One time we set "mig_state->device_state = recover_state;" for the ioctl.
And another time we set "migration->device_state = recover_state;" in 
case of successful ioctl, to update the device state in migration struct.

>
> It looks redundant. The ioctl VFIO_DEVICE_FEATURE should leave the
> state unmodified.
>
> Thanks,
>
> C.
>
>

Re: [PATCH 13/14] migration: Use migrate_has_error() in close_return_path_on_source()

[Fabiano Rosas]

Cédric Le Goater <clg@redhat.com> writes:

> On 2/8/24 14:57, Fabiano Rosas wrote:
>> Cédric Le Goater <clg@redhat.com> writes:
>> 
>>> On 2/8/24 14:07, Fabiano Rosas wrote:
>>>> Cédric Le Goater <clg@redhat.com> writes:
>>>>
>>>>> close_return_path_on_source() retrieves the migration error from the
>>>>> the QEMUFile '->to_dst_file' to know if a shutdown is required. This
>>>>> shutdown is required to exit the return-path thread. However, in
>>>>> migrate_fd_cleanup(), '->to_dst_file' is cleaned up before calling
>>>>> close_return_path_on_source() and the shutdown is never performed,
>>>>> leaving the source and destination waiting for an event to occur.
>>>>>
>>>>> Avoid relying on '->to_dst_file' and use migrate_has_error() instead.
>>>>>
>>>>> Suggested-by: Peter Xu <peterx@redhat.com>
>>>>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>>>>> ---
>>>>>    migration/migration.c | 3 +--
>>>>>    1 file changed, 1 insertion(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/migration/migration.c b/migration/migration.c
>>>>> index d5f705ceef4c925589aa49335969672c0d761fa2..5f55af3d7624750ca416c4177781241b3e291e5d 100644
>>>>> --- a/migration/migration.c
>>>>> +++ b/migration/migration.c
>>>>> @@ -2372,8 +2372,7 @@ static bool close_return_path_on_source(MigrationState *ms)
>>>>>         * cause it to unblock if it's stuck waiting for the destination.
>>>>>         */
>>>>>        WITH_QEMU_LOCK_GUARD(&ms->qemu_file_lock) {
>>>>> -        if (ms->to_dst_file && ms->rp_state.from_dst_file &&
>>>>> -            qemu_file_get_error(ms->to_dst_file)) {
>>>>> +        if (migrate_has_error(ms) && ms->rp_state.from_dst_file) {
>>>>>                qemu_file_shutdown(ms->rp_state.from_dst_file);
>>>>>            }
>>>>>        }
>>>>
>>>> Hm, maybe Peter can help defend this, but this assumes that every
>>>> function that takes an 'f' and sets the file error also sets
>>>> migrate_set_error(). I'm not sure we have determined that, have we?
>>>
>>> How could we check all the code path ? I agree it is difficult when
>>> looking at the code :/
>> 
>> It would help if the thing wasn't called 'f' for the most part of the
>> code to begin with.
>> 
>> Whenever there's a file error at to_dst_file there's the chance that the
>> rp_state.from_dst_file got stuck. So we cannot ignore the file error.
>> 
>> Would it work if we checked it earlier during cleanup as you did
>> previously and then set the migration error?
>
> Do you mean doing something similar to what is done in
> source_return_path_thread() ?
>
>          if (qemu_file_get_error(s->to_dst_file)) {
>              qemu_file_get_error_obj(s->to_dst_file, &err);
>      	    if (err) {
>          	migrate_set_error(ms, err);
>          	error_free(err);
> 	...
>
> Yes. That would be safer I think.

Yes, something like that.

I wish we could make that return path cleanup more deterministic, but
currently it's just: "if something hangs, call shutdown()". We don't
have a way to detect a hang, we just look at the file error and hope it
works.

A crucial aspect here is that calling qemu_file_shutdown() itself sets
the file error. So there's not even a guarantee that an error is
actually an error.

>
>
> Nevertheless, I am struggling to understand how qemu_file_set_error()
> and migrate_set_error() fit together. I was expecting some kind of
> synchronization  routine but there isn't it seems. Are they completely
> orthogonal ? when should we use these routines and when not ?

We're trying to phase out the QEMUFile usage altogether. One thing that
is getting in the way is this dependency on the qemu_file_*_error
functions.

While we're not there yet, a good pattern is to find a
qemu_file_set|get_error() pair and replace it with
migrate_set|has_error(). Unfortunately the return path does not fit in
this, because we don't have a matching qemu_file_set_error, it could be
anywhere. As I said above, we're using that error as a heuristic for: "a
recvmsg() might be hanging".

>
> My initial goal was to modify some of the memory handlers (log_global*)
> and migration handlers to propagate errors at the QMP level and them
> report to the management layer. This is growing in something bigger
> and currently, I don't find a good approach to the problem.
>
> The last two patches of this series try to fix the return-path thread
> termination. Let's keep that for after.

I'll try to figure that out. I see you provided a reproducer.

>
> Thanks,
>
> C.

Re: [RFC PATCH 14/14] migration: Fix return-path thread exit

[Fabiano Rosas]

Cédric Le Goater <clg@redhat.com> writes:

> Hello Fabiano
>
> On 2/8/24 14:29, Fabiano Rosas wrote:
>> Cédric Le Goater <clg@redhat.com> writes:
>> 
>>> In case of error, close_return_path_on_source() can perform a shutdown
>>> to exit the return-path thread.  However, in migrate_fd_cleanup(),
>>> 'to_dst_file' is closed before calling close_return_path_on_source()
>>> and the shutdown fails, leaving the source and destination waiting for
>>> an event to occur.
>> 
>> Hi, Cédric
>> 
>> Are you sure this is not caused by patch 13? 
>
> It happens with upstream QEMU without any patch.

I might have taken that "shutdown fails" in the commit message too
literaly. Anyway, I have a proposed solution:

-->8--
From 729aa7b5b7f130f756d41649fdd0862bd2e90430 Mon Sep 17 00:00:00 2001
From: Fabiano Rosas <farosas@suse.de>
Date: Wed, 14 Feb 2024 16:45:43 -0300
Subject: [PATCH] migration: Join the return path thread before releasing
 to_dst_file
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The return path thread might hang at a blocking system call. Before
joining the thread we might need to issue a shutdown() on the socket
file descriptor to release it. To determine whether the shutdown() is
necessary we look at the QEMUFile error.

Make sure we only clean up the QEMUFile after the return path has been
waited for.

This fixes a hang when qemu_savevm_state_setup() produced an error
that was detected by migration_detect_error(). That skips
migration_completion() so close_return_path_on_source() would get
stuck waiting for the RP thread to terminate.

At migrate_fd_cleanup() I'm keeping the relative order of joining the
migration thread and the return path just in case.

Reported-by: Cédric Le Goater <clg@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/migration.c | 36 ++++++++++++++++--------------------
 1 file changed, 16 insertions(+), 20 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index ab21de2cad..f0b70e8a9d 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1326,17 +1326,19 @@ static void migrate_fd_cleanup(MigrationState *s)
 
     qemu_savevm_state_cleanup();
 
+    bql_unlock();
+    if (s->migration_thread_running) {
+        qemu_thread_join(&s->thread);
+        s->migration_thread_running = false;
+    }
+    bql_lock();
+
+    close_return_path_on_source(s);
+
     if (s->to_dst_file) {
         QEMUFile *tmp;
 
         trace_migrate_fd_cleanup();
-        bql_unlock();
-        if (s->migration_thread_running) {
-            qemu_thread_join(&s->thread);
-            s->migration_thread_running = false;
-        }
-        bql_lock();
-
         multifd_send_shutdown();
         qemu_mutex_lock(&s->qemu_file_lock);
         tmp = s->to_dst_file;
@@ -1350,12 +1352,6 @@ static void migrate_fd_cleanup(MigrationState *s)
         qemu_fclose(tmp);
     }
 
-    /*
-     * We already cleaned up to_dst_file, so errors from the return
-     * path might be due to that, ignore them.
-     */
-    close_return_path_on_source(s);
-
     assert(!migration_is_active(s));
 
     if (s->state == MIGRATION_STATUS_CANCELLING) {
@@ -2874,6 +2870,13 @@ static MigThrError postcopy_pause(MigrationState *s)
     while (true) {
         QEMUFile *file;
 
+        /*
+         * We're already pausing, so ignore any errors on the return
+         * path and just wait for the thread to finish. It will be
+         * re-created when we resume.
+         */
+        close_return_path_on_source(s);
+
         /*
          * Current channel is possibly broken. Release it.  Note that this is
          * guaranteed even without lock because to_dst_file should only be
@@ -2893,13 +2896,6 @@ static MigThrError postcopy_pause(MigrationState *s)
         qemu_file_shutdown(file);
         qemu_fclose(file);
 
-        /*
-         * We're already pausing, so ignore any errors on the return
-         * path and just wait for the thread to finish. It will be
-         * re-created when we resume.
-         */
-        close_return_path_on_source(s);
-
         migrate_set_state(&s->state, s->state,
                           MIGRATION_STATUS_POSTCOPY_PAUSED);
 
-- 
2.35.3

Re: [PATCH 11/14] vfio: Extend vfio_set_migration_error() with Error* argument

[Cédric Le Goater]

Hello Avihai,

On 2/12/24 10:35, Avihai Horon wrote:
> Hi Cedric,
> 
> On 07/02/2024 15:33, 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.
>>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>   hw/vfio/common.c | 50 +++++++++++++++++++++++++++++-------------------
>>   1 file changed, 30 insertions(+), 20 deletions(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 82173b039c47150f5edd05d329192c5b9c8a9a0f..afe8b6bd294fd5904f394a5db48aae3fd718b14e 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -148,16 +148,18 @@ 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)
>>   {
>>       MigrationState *ms = migrate_get_current();
>>
>>       if (migration_is_setup_or_active(ms->state)) {
>>           WITH_QEMU_LOCK_GUARD(&ms->qemu_file_lock) {
>>               if (ms->to_dst_file) {
>> -                qemu_file_set_error(ms->to_dst_file, err);
>> +                qemu_file_set_error_obj(ms->to_dst_file, ret, err);
>>               }
>>           }
>> +    } else {
>> +        error_report_err(err);
>>       }
>>   }
>>
>> @@ -296,15 +298,17 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>>       VFIOContainerBase *bcontainer = giommu->bcontainer;
>>       hwaddr iova = iotlb->iova + giommu->iommu_offset;
>>       void *vaddr;
>> +    Error *local_err = NULL;
>>       int ret;
>>
>>       trace_vfio_iommu_map_notify(iotlb->perm == IOMMU_NONE ? "UNMAP" : "MAP",
>>                                   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;
>>       }
>>
>> @@ -336,11 +340,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));
>> +            vfio_set_migration_error(ret, local_err);
>>           }
>>       }
>>   out:
>> @@ -1224,13 +1229,15 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>>       VFIOContainerBase *bcontainer = giommu->bcontainer;
>>       hwaddr iova = iotlb->iova + giommu->iommu_offset;
>>       ram_addr_t translated_addr;
>> +    Error *local_err = NULL;
>>       int ret = -EINVAL;
>>
>>       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;
>>       }
>>
>> @@ -1239,17 +1246,18 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>>           ret = vfio_get_dirty_bitmap(bcontainer, iova, iotlb->addr_mask + 1,
>>                                       translated_addr);
> 
> If vfio_get_xlat_addr() above (it's not shown here) returns false, we will pass a NULL local_err to vfio_set_migration_error() and it may de-reference NULL ptr in error_report_err().

Ah yes. Thanks for spotting this.

> 
> Should we refactor vfio_get_xlat_addr() to get errp, 

I think we should add an Error** parameter to vfio_get_xlat_addr() and
memory_get_xlat_addr(). It shouldn't be too much change and would be
cleaner.

Thanks,

C.


> or add an else branch below, setting -EINVAL (and removing the default -EINVAL from the top of the function)?
> 
> Thanks.
> 
>>           if (ret) {
>> -            error_report("vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", "
>> -                         "0x%"HWADDR_PRIx") = %d (%s)",
>> -                         bcontainer, iova, iotlb->addr_mask + 1, ret,
>> -                         strerror(-ret));
>> +            error_setg(&local_err,
>> +                       "vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", "
>> +                       "0x%"HWADDR_PRIx") = %d (%s)",
>> +                       bcontainer, iova, iotlb->addr_mask + 1, ret,
>> +                       strerror(-ret));
>>           }
>>       }
>>       rcu_read_unlock();
>>
>>   out:
>>       if (ret) {
>> -        vfio_set_migration_error(ret);
>> +        vfio_set_migration_error(ret, local_err);
>>       }
>>   }
>>
>> @@ -1345,6 +1353,7 @@ static void vfio_listener_log_sync(MemoryListener *listener,
>>   {
>>       VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
>>                                                    listener);
>> +    Error *local_err = NULL;
>>       int ret;
>>
>>       if (vfio_listener_skipped_section(section)) {
>> @@ -1354,9 +1363,10 @@ static void vfio_listener_log_sync(MemoryListener *listener,
>>       if (vfio_devices_all_dirty_tracking(bcontainer)) {
>>           ret = vfio_sync_dirty_bitmap(bcontainer, section);
>>           if (ret) {
>> -            error_report("vfio: Failed to sync dirty bitmap, err: %d (%s)", ret,
>> -                         strerror(-ret));
>> -            vfio_set_migration_error(ret);
>> +            error_setg(&local_err,
>> +                       "vfio: Failed to sync dirty bitmap, err: %d (%s)", ret,
>> +                       strerror(-ret));
>> +            vfio_set_migration_error(ret, local_err);
>>           }
>>       }
>>   }
>> -- 
>> 2.43.0
>>
>>
> 

Re: [RFC PATCH 14/14] migration: Fix return-path thread exit

[Cédric Le Goater]

Hello Fabiano

On 2/14/24 21:35, Fabiano Rosas wrote:
> Cédric Le Goater <clg@redhat.com> writes:
> 
>> Hello Fabiano
>>
>> On 2/8/24 14:29, Fabiano Rosas wrote:
>>> Cédric Le Goater <clg@redhat.com> writes:
>>>
>>>> In case of error, close_return_path_on_source() can perform a shutdown
>>>> to exit the return-path thread.  However, in migrate_fd_cleanup(),
>>>> 'to_dst_file' is closed before calling close_return_path_on_source()
>>>> and the shutdown fails, leaving the source and destination waiting for
>>>> an event to occur.
>>>
>>> Hi, Cédric
>>>
>>> Are you sure this is not caused by patch 13?
>>
>> It happens with upstream QEMU without any patch.
> 
> I might have taken that "shutdown fails" in the commit message too
> literaly. Anyway, I have a proposed solution:
> 
> -->8--
>  From 729aa7b5b7f130f756d41649fdd0862bd2e90430 Mon Sep 17 00:00:00 2001
> From: Fabiano Rosas <farosas@suse.de>
> Date: Wed, 14 Feb 2024 16:45:43 -0300
> Subject: [PATCH] migration: Join the return path thread before releasing
>   to_dst_file
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> The return path thread might hang at a blocking system call. Before
> joining the thread we might need to issue a shutdown() on the socket
> file descriptor to release it. To determine whether the shutdown() is
> necessary we look at the QEMUFile error.
> 
> Make sure we only clean up the QEMUFile after the return path has been
> waited for.

Yes. That's the important part.

> This fixes a hang when qemu_savevm_state_setup() produced an error
> that was detected by migration_detect_error(). That skips
> migration_completion() so close_return_path_on_source() would get
> stuck waiting for the RP thread to terminate.
> 
> At migrate_fd_cleanup() I'm keeping the relative order of joining the
> migration thread and the return path just in case.

That doesn't look necessary. What was the reason to join the migration
thread only when s->to_dst_file is valid ?


> Reported-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

LGTM, it fixes the hang when an error is detected, the migration is
aborted and the VM resumes execution. FWIW,

Tested-by: Cédric Le Goater <clg@redhat.com>

It requires more thorough testing though.

Thanks,

C.




> ---
>   migration/migration.c | 36 ++++++++++++++++--------------------
>   1 file changed, 16 insertions(+), 20 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index ab21de2cad..f0b70e8a9d 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1326,17 +1326,19 @@ static void migrate_fd_cleanup(MigrationState *s)
>   
>       qemu_savevm_state_cleanup();
>   
> +    bql_unlock();
> +    if (s->migration_thread_running) {
> +        qemu_thread_join(&s->thread);
> +        s->migration_thread_running = false;
> +    }
> +    bql_lock();
> +
> +    close_return_path_on_source(s);
> +
>       if (s->to_dst_file) {
>           QEMUFile *tmp;
>   
>           trace_migrate_fd_cleanup();
> -        bql_unlock();
> -        if (s->migration_thread_running) {
> -            qemu_thread_join(&s->thread);
> -            s->migration_thread_running = false;
> -        }
> -        bql_lock();
> -
>           multifd_send_shutdown();
>           qemu_mutex_lock(&s->qemu_file_lock);
>           tmp = s->to_dst_file;
> @@ -1350,12 +1352,6 @@ static void migrate_fd_cleanup(MigrationState *s)
>           qemu_fclose(tmp);
>       }
>   
> -    /*
> -     * We already cleaned up to_dst_file, so errors from the return
> -     * path might be due to that, ignore them.
> -     */
> -    close_return_path_on_source(s);
> -
>       assert(!migration_is_active(s));
>   
>       if (s->state == MIGRATION_STATUS_CANCELLING) {
> @@ -2874,6 +2870,13 @@ static MigThrError postcopy_pause(MigrationState *s)
>       while (true) {
>           QEMUFile *file;
>   
> +        /*
> +         * We're already pausing, so ignore any errors on the return
> +         * path and just wait for the thread to finish. It will be
> +         * re-created when we resume.
> +         */
> +        close_return_path_on_source(s);
> +
>           /*
>            * Current channel is possibly broken. Release it.  Note that this is
>            * guaranteed even without lock because to_dst_file should only be
> @@ -2893,13 +2896,6 @@ static MigThrError postcopy_pause(MigrationState *s)
>           qemu_file_shutdown(file);
>           qemu_fclose(file);
>   
> -        /*
> -         * We're already pausing, so ignore any errors on the return
> -         * path and just wait for the thread to finish. It will be
> -         * re-created when we resume.
> -         */
> -        close_return_path_on_source(s);
> -
>           migrate_set_state(&s->state, s->state,
>                             MIGRATION_STATUS_POSTCOPY_PAUSED);
>   

Re: [PATCH 13/14] migration: Use migrate_has_error() in close_return_path_on_source()

[Cédric Le Goater]

On 2/14/24 17:00, Fabiano Rosas wrote:
> Cédric Le Goater <clg@redhat.com> writes:
> 
>> On 2/8/24 14:57, Fabiano Rosas wrote:
>>> Cédric Le Goater <clg@redhat.com> writes:
>>>
>>>> On 2/8/24 14:07, Fabiano Rosas wrote:
>>>>> Cédric Le Goater <clg@redhat.com> writes:
>>>>>
>>>>>> close_return_path_on_source() retrieves the migration error from the
>>>>>> the QEMUFile '->to_dst_file' to know if a shutdown is required. This
>>>>>> shutdown is required to exit the return-path thread. However, in
>>>>>> migrate_fd_cleanup(), '->to_dst_file' is cleaned up before calling
>>>>>> close_return_path_on_source() and the shutdown is never performed,
>>>>>> leaving the source and destination waiting for an event to occur.
>>>>>>
>>>>>> Avoid relying on '->to_dst_file' and use migrate_has_error() instead.
>>>>>>
>>>>>> Suggested-by: Peter Xu <peterx@redhat.com>
>>>>>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>>>>>> ---
>>>>>>     migration/migration.c | 3 +--
>>>>>>     1 file changed, 1 insertion(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/migration/migration.c b/migration/migration.c
>>>>>> index d5f705ceef4c925589aa49335969672c0d761fa2..5f55af3d7624750ca416c4177781241b3e291e5d 100644
>>>>>> --- a/migration/migration.c
>>>>>> +++ b/migration/migration.c
>>>>>> @@ -2372,8 +2372,7 @@ static bool close_return_path_on_source(MigrationState *ms)
>>>>>>          * cause it to unblock if it's stuck waiting for the destination.
>>>>>>          */
>>>>>>         WITH_QEMU_LOCK_GUARD(&ms->qemu_file_lock) {
>>>>>> -        if (ms->to_dst_file && ms->rp_state.from_dst_file &&
>>>>>> -            qemu_file_get_error(ms->to_dst_file)) {
>>>>>> +        if (migrate_has_error(ms) && ms->rp_state.from_dst_file) {
>>>>>>                 qemu_file_shutdown(ms->rp_state.from_dst_file);
>>>>>>             }
>>>>>>         }
>>>>>
>>>>> Hm, maybe Peter can help defend this, but this assumes that every
>>>>> function that takes an 'f' and sets the file error also sets
>>>>> migrate_set_error(). I'm not sure we have determined that, have we?
>>>>
>>>> How could we check all the code path ? I agree it is difficult when
>>>> looking at the code :/
>>>
>>> It would help if the thing wasn't called 'f' for the most part of the
>>> code to begin with.
>>>
>>> Whenever there's a file error at to_dst_file there's the chance that the
>>> rp_state.from_dst_file got stuck. So we cannot ignore the file error.
>>>
>>> Would it work if we checked it earlier during cleanup as you did
>>> previously and then set the migration error?
>>
>> Do you mean doing something similar to what is done in
>> source_return_path_thread() ?
>>
>>           if (qemu_file_get_error(s->to_dst_file)) {
>>               qemu_file_get_error_obj(s->to_dst_file, &err);
>>       	    if (err) {
>>           	migrate_set_error(ms, err);
>>           	error_free(err);
>> 	...
>>
>> Yes. That would be safer I think.
> 
> Yes, something like that.
> 
> I wish we could make that return path cleanup more deterministic, but
> currently it's just: "if something hangs, call shutdown()". We don't
> have a way to detect a hang, we just look at the file error and hope it
> works.
> 
> A crucial aspect here is that calling qemu_file_shutdown() itself sets
> the file error. So there's not even a guarantee that an error is
> actually an error.
> 
>>
>>
>> Nevertheless, I am struggling to understand how qemu_file_set_error()
>> and migrate_set_error() fit together. I was expecting some kind of
>> synchronization  routine but there isn't it seems. Are they completely
>> orthogonal ? when should we use these routines and when not ?
> 
> We're trying to phase out the QEMUFile usage altogether. One thing that
> is getting in the way is this dependency on the qemu_file_*_error
> functions.

OK. the other changes, which add an Error** argument to various handlers,
reduce the use of qemu_file_*_error routines in VFIO.

> While we're not there yet, a good pattern is to find a
> qemu_file_set|get_error() pair and replace it with
> migrate_set|has_error(). 

OK. I will keep that in mind for the other changes.

Thanks,

C.



> Unfortunately the return path does not fit in
> this, because we don't have a matching qemu_file_set_error, it could be
> anywhere. As I said above, we're using that error as a heuristic for: "a
> recvmsg() might be hanging".
> 
>>
>> My initial goal was to modify some of the memory handlers (log_global*)
>> and migration handlers to propagate errors at the QMP level and them
>> report to the management layer. This is growing in something bigger
>> and currently, I don't find a good approach to the problem.
>>
>> The last two patches of this series try to fix the return-path thread
>> termination. Let's keep that for after.
> 
> I'll try to figure that out. I see you provided a reproducer.
> 
>>
>> Thanks,
>>
>> C.
> 

Re: [RFC PATCH 14/14] migration: Fix return-path thread exit

[Fabiano Rosas]

Cédric Le Goater <clg@redhat.com> writes:

> Hello Fabiano
>
> On 2/14/24 21:35, Fabiano Rosas wrote:
>> Cédric Le Goater <clg@redhat.com> writes:
>> 
>>> Hello Fabiano
>>>
>>> On 2/8/24 14:29, Fabiano Rosas wrote:
>>>> Cédric Le Goater <clg@redhat.com> writes:
>>>>
>>>>> In case of error, close_return_path_on_source() can perform a shutdown
>>>>> to exit the return-path thread.  However, in migrate_fd_cleanup(),
>>>>> 'to_dst_file' is closed before calling close_return_path_on_source()
>>>>> and the shutdown fails, leaving the source and destination waiting for
>>>>> an event to occur.
>>>>
>>>> Hi, Cédric
>>>>
>>>> Are you sure this is not caused by patch 13?
>>>
>>> It happens with upstream QEMU without any patch.
>> 
>> I might have taken that "shutdown fails" in the commit message too
>> literaly. Anyway, I have a proposed solution:
>> 
>> -->8--
>>  From 729aa7b5b7f130f756d41649fdd0862bd2e90430 Mon Sep 17 00:00:00 2001
>> From: Fabiano Rosas <farosas@suse.de>
>> Date: Wed, 14 Feb 2024 16:45:43 -0300
>> Subject: [PATCH] migration: Join the return path thread before releasing
>>   to_dst_file
>> MIME-Version: 1.0
>> Content-Type: text/plain; charset=UTF-8
>> Content-Transfer-Encoding: 8bit
>> 
>> The return path thread might hang at a blocking system call. Before
>> joining the thread we might need to issue a shutdown() on the socket
>> file descriptor to release it. To determine whether the shutdown() is
>> necessary we look at the QEMUFile error.
>> 
>> Make sure we only clean up the QEMUFile after the return path has been
>> waited for.
>
> Yes. That's the important part.
>
>> This fixes a hang when qemu_savevm_state_setup() produced an error
>> that was detected by migration_detect_error(). That skips
>> migration_completion() so close_return_path_on_source() would get
>> stuck waiting for the RP thread to terminate.
>> 
>> At migrate_fd_cleanup() I'm keeping the relative order of joining the
>> migration thread and the return path just in case.
>
> That doesn't look necessary.

Indeed. But I don't trust the migration code, it's full of undocumented
dependencies like that.

> What was the reason to join the migration thread only when
> s->to_dst_file is valid ?

I didn't find any explicit reason looking through the history. It seems
we used to rely on to_dst_file before migration_thread_running was
introduced.

I wouldn't mind keeping that 'if' there.

Let's see what Peter thinks about it.

Re: [PATCH 13/14] migration: Use migrate_has_error() in close_return_path_on_source()

[Peter Xu]

On Thu, Feb 08, 2024 at 10:07:44AM -0300, Fabiano Rosas wrote:
> > diff --git a/migration/migration.c b/migration/migration.c
> > index d5f705ceef4c925589aa49335969672c0d761fa2..5f55af3d7624750ca416c4177781241b3e291e5d 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -2372,8 +2372,7 @@ static bool close_return_path_on_source(MigrationState *ms)
> >       * cause it to unblock if it's stuck waiting for the destination.
> >       */
> >      WITH_QEMU_LOCK_GUARD(&ms->qemu_file_lock) {
> > -        if (ms->to_dst_file && ms->rp_state.from_dst_file &&
> > -            qemu_file_get_error(ms->to_dst_file)) {
> > +        if (migrate_has_error(ms) && ms->rp_state.from_dst_file) {
> >              qemu_file_shutdown(ms->rp_state.from_dst_file);
> >          }
> >      }
> 
> Hm, maybe Peter can help defend this, but this assumes that every
> function that takes an 'f' and sets the file error also sets
> migrate_set_error(). I'm not sure we have determined that, have we?

[apologies on getting back to this thread late.. I saw there's yet another
 proposal in the other email, will look at that soon]

I think that should be set, or otherwise we lose an error?  After all
s->error is the only thing we report, if there is a qemufile error that is
not reported into s->error it can be lost then.

On src QEMU we have both migration thread and return path thread.  For
migration thread the file error should always be collected by
migration_detect_error() by the qemu_file_get_error_obj_any() (it also
looks after postcopy_qemufile_src).  For return path thread it's always
collected when the loop quits.

Would migrate_has_error() be safer than qemu_file_get_error() in some
cases?  I'm considering when there is an error outside of qemufile itself,
that's the case where qemu_file_get_error(ms->to_dst_file) can return
false, however we may still need a kick to the from_dst_file?

-- 
Peter Xu

Re: [RFC PATCH 14/14] migration: Fix return-path thread exit

[Peter Xu]

On Mon, Feb 12, 2024 at 05:04:28PM +0100, Cédric Le Goater wrote:
> and then, in background we have open questions regarding :
> 
> * the QEMUfile implementation and its QIOChannel usage for migration
>   streams
> * qemu_file_set_error* vs. migrate_set_error. It is confusing, at least
>   for me. Do we have some documentation on best practices ?

Right it is confusing..  It can all boil down to the acient qemufile api
that Fabiano also mentioned in the other reply.  IMHO ideally iochannel
errors should be reported through the stack (rather than kept within the
object) from the channel's API and stored with migrate_set_error() if
necessary, and the channel itself may not need to maintain its own errors.
Right now it's needed because many qemufile APIs do not return errors.

Thanks,

-- 
Peter Xu

Re: [RFC PATCH 14/14] migration: Fix return-path thread exit

[Peter Xu]

On Fri, Feb 16, 2024 at 02:35:26PM -0300, Fabiano Rosas wrote:
> Cédric Le Goater <clg@redhat.com> writes:
> 
> > Hello Fabiano
> >
> > On 2/14/24 21:35, Fabiano Rosas wrote:
> >> Cédric Le Goater <clg@redhat.com> writes:
> >> 
> >>> Hello Fabiano
> >>>
> >>> On 2/8/24 14:29, Fabiano Rosas wrote:
> >>>> Cédric Le Goater <clg@redhat.com> writes:
> >>>>
> >>>>> In case of error, close_return_path_on_source() can perform a shutdown
> >>>>> to exit the return-path thread.  However, in migrate_fd_cleanup(),
> >>>>> 'to_dst_file' is closed before calling close_return_path_on_source()
> >>>>> and the shutdown fails, leaving the source and destination waiting for
> >>>>> an event to occur.
> >>>>
> >>>> Hi, Cédric
> >>>>
> >>>> Are you sure this is not caused by patch 13?
> >>>
> >>> It happens with upstream QEMU without any patch.
> >> 
> >> I might have taken that "shutdown fails" in the commit message too
> >> literaly. Anyway, I have a proposed solution:
> >> 
> >> -->8--
> >>  From 729aa7b5b7f130f756d41649fdd0862bd2e90430 Mon Sep 17 00:00:00 2001
> >> From: Fabiano Rosas <farosas@suse.de>
> >> Date: Wed, 14 Feb 2024 16:45:43 -0300
> >> Subject: [PATCH] migration: Join the return path thread before releasing
> >>   to_dst_file
> >> MIME-Version: 1.0
> >> Content-Type: text/plain; charset=UTF-8
> >> Content-Transfer-Encoding: 8bit
> >> 
> >> The return path thread might hang at a blocking system call. Before
> >> joining the thread we might need to issue a shutdown() on the socket
> >> file descriptor to release it. To determine whether the shutdown() is
> >> necessary we look at the QEMUFile error.
> >> 
> >> Make sure we only clean up the QEMUFile after the return path has been
> >> waited for.
> >
> > Yes. That's the important part.
> >
> >> This fixes a hang when qemu_savevm_state_setup() produced an error
> >> that was detected by migration_detect_error(). That skips
> >> migration_completion() so close_return_path_on_source() would get
> >> stuck waiting for the RP thread to terminate.
> >> 
> >> At migrate_fd_cleanup() I'm keeping the relative order of joining the
> >> migration thread and the return path just in case.
> >
> > That doesn't look necessary.
> 
> Indeed. But I don't trust the migration code, it's full of undocumented
> dependencies like that.
> 
> > What was the reason to join the migration thread only when
> > s->to_dst_file is valid ?
> 
> I didn't find any explicit reason looking through the history. It seems
> we used to rely on to_dst_file before migration_thread_running was
> introduced.
> 
> I wouldn't mind keeping that 'if' there.
> 
> Let's see what Peter thinks about it.

Frankly I don't have a strong opinion on current patch 14 or the new
proposal, but it seems we reached a consensus.

Fabiano, would you repost with a formal patch, with the proper tags?

One thing I am still not sure is whether we should still have patch 13
altogether? Please see my other reply on whether it's possible to have
migrate_get_error() == true but qemu_file_get_error() == false.  In
postcopy_pause(), currently we constantly shutdown() so the join() should
always work:

        qemu_file_shutdown(file);
        qemu_fclose(file);

        /*
         * We're already pausing, so ignore any errors on the return
         * path and just wait for the thread to finish. It will be
         * re-created when we resume.
         */
        close_return_path_on_source(s);

If move close_return_path_on_source() upper, qemu_file_shutdown() may not
be needed? And I think we need to make sure close_return_path_on_source()
will always properly kick the other thread.

Thanks,

-- 
Peter Xu

Re: [RFC PATCH 14/14] migration: Fix return-path thread exit

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Fri, Feb 16, 2024 at 02:35:26PM -0300, Fabiano Rosas wrote:
>> Cédric Le Goater <clg@redhat.com> writes:
>> 
>> > Hello Fabiano
>> >
>> > On 2/14/24 21:35, Fabiano Rosas wrote:
>> >> Cédric Le Goater <clg@redhat.com> writes:
>> >> 
>> >>> Hello Fabiano
>> >>>
>> >>> On 2/8/24 14:29, Fabiano Rosas wrote:
>> >>>> Cédric Le Goater <clg@redhat.com> writes:
>> >>>>
>> >>>>> In case of error, close_return_path_on_source() can perform a shutdown
>> >>>>> to exit the return-path thread.  However, in migrate_fd_cleanup(),
>> >>>>> 'to_dst_file' is closed before calling close_return_path_on_source()
>> >>>>> and the shutdown fails, leaving the source and destination waiting for
>> >>>>> an event to occur.
>> >>>>
>> >>>> Hi, Cédric
>> >>>>
>> >>>> Are you sure this is not caused by patch 13?
>> >>>
>> >>> It happens with upstream QEMU without any patch.
>> >> 
>> >> I might have taken that "shutdown fails" in the commit message too
>> >> literaly. Anyway, I have a proposed solution:
>> >> 
>> >> -->8--
>> >>  From 729aa7b5b7f130f756d41649fdd0862bd2e90430 Mon Sep 17 00:00:00 2001
>> >> From: Fabiano Rosas <farosas@suse.de>
>> >> Date: Wed, 14 Feb 2024 16:45:43 -0300
>> >> Subject: [PATCH] migration: Join the return path thread before releasing
>> >>   to_dst_file
>> >> MIME-Version: 1.0
>> >> Content-Type: text/plain; charset=UTF-8
>> >> Content-Transfer-Encoding: 8bit
>> >> 
>> >> The return path thread might hang at a blocking system call. Before
>> >> joining the thread we might need to issue a shutdown() on the socket
>> >> file descriptor to release it. To determine whether the shutdown() is
>> >> necessary we look at the QEMUFile error.
>> >> 
>> >> Make sure we only clean up the QEMUFile after the return path has been
>> >> waited for.
>> >
>> > Yes. That's the important part.
>> >
>> >> This fixes a hang when qemu_savevm_state_setup() produced an error
>> >> that was detected by migration_detect_error(). That skips
>> >> migration_completion() so close_return_path_on_source() would get
>> >> stuck waiting for the RP thread to terminate.
>> >> 
>> >> At migrate_fd_cleanup() I'm keeping the relative order of joining the
>> >> migration thread and the return path just in case.
>> >
>> > That doesn't look necessary.
>> 
>> Indeed. But I don't trust the migration code, it's full of undocumented
>> dependencies like that.
>> 
>> > What was the reason to join the migration thread only when
>> > s->to_dst_file is valid ?
>> 
>> I didn't find any explicit reason looking through the history. It seems
>> we used to rely on to_dst_file before migration_thread_running was
>> introduced.
>> 
>> I wouldn't mind keeping that 'if' there.
>> 
>> Let's see what Peter thinks about it.
>
> Frankly I don't have a strong opinion on current patch 14 or the new
> proposal, but it seems we reached a consensus.
>
> Fabiano, would you repost with a formal patch, with the proper tags?

Yes, I'll post it soon.

>
> One thing I am still not sure is whether we should still have patch 13
> altogether? Please see my other reply on whether it's possible to have
> migrate_get_error() == true but qemu_file_get_error() == false.

I'll include it then.

> In
> postcopy_pause(), currently we constantly shutdown() so the join() should
> always work:
>
>         qemu_file_shutdown(file);
>         qemu_fclose(file);
>
>         /*
>          * We're already pausing, so ignore any errors on the return
>          * path and just wait for the thread to finish. It will be
>          * re-created when we resume.
>          */
>         close_return_path_on_source(s);
>
> If move close_return_path_on_source() upper, qemu_file_shutdown() may not
> be needed? And I think we need to make sure close_return_path_on_source()
> will always properly kick the other thread.

Re: [RFC PATCH 14/14] migration: Fix return-path thread exit

[Cédric Le Goater]

On 2/23/24 15:05, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
>> On Fri, Feb 16, 2024 at 02:35:26PM -0300, Fabiano Rosas wrote:
>>> Cédric Le Goater <clg@redhat.com> writes:
>>>
>>>> Hello Fabiano
>>>>
>>>> On 2/14/24 21:35, Fabiano Rosas wrote:
>>>>> Cédric Le Goater <clg@redhat.com> writes:
>>>>>
>>>>>> Hello Fabiano
>>>>>>
>>>>>> On 2/8/24 14:29, Fabiano Rosas wrote:
>>>>>>> Cédric Le Goater <clg@redhat.com> writes:
>>>>>>>
>>>>>>>> In case of error, close_return_path_on_source() can perform a shutdown
>>>>>>>> to exit the return-path thread.  However, in migrate_fd_cleanup(),
>>>>>>>> 'to_dst_file' is closed before calling close_return_path_on_source()
>>>>>>>> and the shutdown fails, leaving the source and destination waiting for
>>>>>>>> an event to occur.
>>>>>>>
>>>>>>> Hi, Cédric
>>>>>>>
>>>>>>> Are you sure this is not caused by patch 13?
>>>>>>
>>>>>> It happens with upstream QEMU without any patch.
>>>>>
>>>>> I might have taken that "shutdown fails" in the commit message too
>>>>> literaly. Anyway, I have a proposed solution:
>>>>>
>>>>> -->8--
>>>>>   From 729aa7b5b7f130f756d41649fdd0862bd2e90430 Mon Sep 17 00:00:00 2001
>>>>> From: Fabiano Rosas <farosas@suse.de>
>>>>> Date: Wed, 14 Feb 2024 16:45:43 -0300
>>>>> Subject: [PATCH] migration: Join the return path thread before releasing
>>>>>    to_dst_file
>>>>> MIME-Version: 1.0
>>>>> Content-Type: text/plain; charset=UTF-8
>>>>> Content-Transfer-Encoding: 8bit
>>>>>
>>>>> The return path thread might hang at a blocking system call. Before
>>>>> joining the thread we might need to issue a shutdown() on the socket
>>>>> file descriptor to release it. To determine whether the shutdown() is
>>>>> necessary we look at the QEMUFile error.
>>>>>
>>>>> Make sure we only clean up the QEMUFile after the return path has been
>>>>> waited for.
>>>>
>>>> Yes. That's the important part.
>>>>
>>>>> This fixes a hang when qemu_savevm_state_setup() produced an error
>>>>> that was detected by migration_detect_error(). That skips
>>>>> migration_completion() so close_return_path_on_source() would get
>>>>> stuck waiting for the RP thread to terminate.
>>>>>
>>>>> At migrate_fd_cleanup() I'm keeping the relative order of joining the
>>>>> migration thread and the return path just in case.
>>>>
>>>> That doesn't look necessary.
>>>
>>> Indeed. But I don't trust the migration code, it's full of undocumented
>>> dependencies like that.
>>>
>>>> What was the reason to join the migration thread only when
>>>> s->to_dst_file is valid ?
>>>
>>> I didn't find any explicit reason looking through the history. It seems
>>> we used to rely on to_dst_file before migration_thread_running was
>>> introduced.
>>>
>>> I wouldn't mind keeping that 'if' there.
>>>
>>> Let's see what Peter thinks about it.
>>
>> Frankly I don't have a strong opinion on current patch 14 or the new
>> proposal, but it seems we reached a consensus.
>>
>> Fabiano, would you repost with a formal patch, with the proper tags?
> 
> Yes, I'll post it soon.
> 
>>
>> One thing I am still not sure is whether we should still have patch 13
>> altogether? Please see my other reply on whether it's possible to have
>> migrate_get_error() == true but qemu_file_get_error() == false.
> 
> I'll include it then.

Thanks for taking over.

I have included :

  [PATCH] migration: Join the return path thread before releasing to_dst_file

in my series and dropped 13-14. I hope to send a follow up on :

   https://lore.kernel.org/qemu-devel/20240207133347.1115903-1-clg@redhat.com/

before we reach soft freeze. It's growing quite a lot.

C.