[PATCH V1 0/3] allow cpr-reboot for vfio

[PATCH V1 0/3] allow cpr-reboot for vfio

[Steve Sistare]

Allow cpr-reboot for vfio if the guest is in the suspended runstate.  The
guest drivers' suspend methods flush outstanding requests and re-initialize
the devices, and thus there is no device state to save and restore.  The
user is responsible for suspending the guest before initiating cpr, such as
by issuing guest-suspend-ram to the qemu guest agent.

This series depends on the series
  [PATCH V8 00/12] fix migration of suspended runstate

Steve Sistare (3):
  migration: check mode in notifiers
  migration: notifier error reporting
  vfio: allow cpr-reboot migration if suspended

 hw/net/virtio-net.c           |  4 ++++
 hw/vfio/common.c              |  2 +-
 hw/vfio/container.c           | 11 ++++++++++-
 hw/vfio/cpr.c                 | 42 ++++++++++++++++++++++++++++++++++++++++++
 hw/vfio/meson.build           |  1 +
 hw/vfio/migration.c           |  5 ++++-
 include/hw/vfio/vfio-common.h |  4 ++++
 include/migration/misc.h      |  3 ++-
 migration/migration.c         | 31 +++++++++++++++++++++++++++----
 migration/ram.c               |  9 +++++----
 net/vhost-vdpa.c              |  4 ++++
 ui/spice-core.c               |  2 +-
 12 files changed, 105 insertions(+), 13 deletions(-)
 create mode 100644 hw/vfio/cpr.c

-- 
1.8.3.1

[PATCH V1 1/3] migration: check mode in notifiers

[Steve Sistare]

The existing notifiers should only apply to normal mode.

No functional change.

Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
 hw/net/virtio-net.c      | 4 ++++
 hw/vfio/migration.c      | 3 +++
 include/migration/misc.h | 1 +
 migration/migration.c    | 5 +++++
 net/vhost-vdpa.c         | 4 ++++
 ui/spice-core.c          | 2 +-
 6 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 80c56f0..0fa083d 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3532,6 +3532,10 @@ static void virtio_net_handle_migration_primary(VirtIONet *n, MigrationState *s)
 static void virtio_net_migration_state_notifier(Notifier *notifier, void *data)
 {
     MigrationState *s = data;
+
+    if (migrate_mode_of(s) != MIG_MODE_NORMAL) {
+        return;
+    }
     VirtIONet *n = container_of(notifier, VirtIONet, migration_state);
     virtio_net_handle_migration_primary(n, s);
 }
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 28d422b..814132a 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -763,6 +763,9 @@ static void vfio_migration_state_notifier(Notifier *notifier, void *data)
                                             migration_state);
     VFIODevice *vbasedev = migration->vbasedev;
 
+    if (migrate_mode_of(s) != MIG_MODE_NORMAL) {
+        return;
+    }
     trace_vfio_migration_state_notifier(vbasedev->name,
                                         MigrationStatus_str(s->state));
 
diff --git a/include/migration/misc.h b/include/migration/misc.h
index 1bc8902..901d117 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -61,6 +61,7 @@ void migration_object_init(void);
 void migration_shutdown(void);
 bool migration_is_idle(void);
 bool migration_is_active(MigrationState *);
+MigMode migrate_mode_of(MigrationState *);
 void migration_add_notifier(Notifier *notify,
                             void (*func)(Notifier *notifier, void *data));
 void migration_remove_notifier(Notifier *notify);
diff --git a/migration/migration.c b/migration/migration.c
index 2cc7e2a..d5bfe70 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1559,6 +1559,11 @@ bool migration_is_active(MigrationState *s)
             s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE);
 }
 
+MigMode migrate_mode_of(MigrationState *s)
+{
+    return s->parameters.mode;
+}
+
 int migrate_init(MigrationState *s, Error **errp)
 {
     int ret;
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index d0614d7..80acc4b 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -336,6 +336,10 @@ static void vdpa_net_migration_state_notifier(Notifier *notifier, void *data)
     VhostVDPAState *s = container_of(notifier, VhostVDPAState,
                                      migration_state);
 
+    if (migrate_mode_of(migration) != MIG_MODE_NORMAL) {
+        return;
+    }
+
     if (migration_in_setup(migration)) {
         vhost_vdpa_net_log_global_enable(s, true);
     } else if (migration_has_failed(migration)) {
diff --git a/ui/spice-core.c b/ui/spice-core.c
index db21db2..0a04eb0 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -572,7 +572,7 @@ static void migration_state_notifier(Notifier *notifier, void *data)
 {
     MigrationState *s = data;
 
-    if (!spice_have_target_host) {
+    if (!spice_have_target_host || migrate_mode_of(s) != MIG_MODE_NORMAL) {
         return;
     }
 
-- 
1.8.3.1

[PATCH V1 2/3] migration: notifier error reporting

[Steve Sistare]

After calling notifiers, check if an error has been reported via
migrate_set_error, and halt the migration.

None of the notifiers call migrate_set_error at this time, so no
functional change.

Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
 include/migration/misc.h |  2 +-
 migration/migration.c    | 26 ++++++++++++++++++++++----
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/include/migration/misc.h b/include/migration/misc.h
index 901d117..231d7e4 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -65,7 +65,7 @@ MigMode migrate_mode_of(MigrationState *);
 void migration_add_notifier(Notifier *notify,
                             void (*func)(Notifier *notifier, void *data));
 void migration_remove_notifier(Notifier *notify);
-void migration_call_notifiers(MigrationState *s);
+int migration_call_notifiers(MigrationState *s);
 bool migration_in_setup(MigrationState *);
 bool migration_has_finished(MigrationState *);
 bool migration_has_failed(MigrationState *);
diff --git a/migration/migration.c b/migration/migration.c
index d5bfe70..29a9a92 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1280,6 +1280,8 @@ void migrate_set_state(int *state, int old_state, int new_state)
 
 static void migrate_fd_cleanup(MigrationState *s)
 {
+    bool already_failed;
+
     qemu_bh_delete(s->cleanup_bh);
     s->cleanup_bh = NULL;
 
@@ -1327,11 +1329,20 @@ static void migrate_fd_cleanup(MigrationState *s)
                           MIGRATION_STATUS_CANCELLED);
     }
 
+    already_failed = migration_has_failed(s);
+    if (migration_call_notifiers(s)) {
+        if (!already_failed) {
+            migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
+            /* Notify again to recover from this late failure. */
+            migration_call_notifiers(s);
+        }
+    }
+
     if (s->error) {
         /* It is used on info migrate.  We can't free it */
         error_report_err(error_copy(s->error));
     }
-    migration_call_notifiers(s);
+
     block_cleanup_parameters();
     yank_unregister_instance(MIGRATION_YANK_INSTANCE);
 }
@@ -1450,9 +1461,10 @@ void migration_remove_notifier(Notifier *notify)
     }
 }
 
-void migration_call_notifiers(MigrationState *s)
+int migration_call_notifiers(MigrationState *s)
 {
     notifier_list_notify(&migration_state_notifiers, s);
+    return (s->error != NULL);
 }
 
 bool migration_in_setup(MigrationState *s)
@@ -2520,7 +2532,9 @@ static int postcopy_start(MigrationState *ms, Error **errp)
      * spice needs to trigger a transition now
      */
     ms->postcopy_after_devices = true;
-    migration_call_notifiers(ms);
+    if (migration_call_notifiers(ms)) {
+        goto fail;
+    }
 
     migration_downtime_end(ms);
 
@@ -3589,7 +3603,11 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
         rate_limit = migrate_max_bandwidth();
 
         /* Notify before starting migration thread */
-        migration_call_notifiers(s);
+        if (migration_call_notifiers(s)) {
+            migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
+            migrate_fd_cleanup(s);
+            return;
+        }
     }
 
     migration_rate_set(rate_limit);
-- 
1.8.3.1

[PATCH V1 3/3] vfio: allow cpr-reboot migration if suspended

[Steve Sistare]

Allow cpr-reboot for vfio if the guest is in the suspended runstate.  The
guest drivers' suspend methods flush outstanding requests and re-initialize
the devices, and thus there is no device state to save and restore.  The
user is responsible for suspending the guest before initiating cpr, such as
by issuing guest-suspend-ram to the qemu guest agent.

Relax the vfio blocker so it does not apply to cpr, and add a notifier that
verifies the guest is suspended.  Skip dirty page tracking, which is N/A for
cpr, to avoid ioctl errors.

Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
note: vfio_cpr_register_container is trivial in this patch and could be
squashed, but it is expanded in future patches for cpr-exec mode.
---
---
 hw/vfio/common.c              |  2 +-
 hw/vfio/container.c           | 11 ++++++++++-
 hw/vfio/cpr.c                 | 42 ++++++++++++++++++++++++++++++++++++++++++
 hw/vfio/meson.build           |  1 +
 hw/vfio/migration.c           |  2 +-
 include/hw/vfio/vfio-common.h |  4 ++++
 migration/ram.c               |  9 +++++----
 7 files changed, 64 insertions(+), 7 deletions(-)
 create mode 100644 hw/vfio/cpr.c

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index e70fdf5..833a528 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -128,7 +128,7 @@ int vfio_block_multiple_devices_migration(VFIODevice *vbasedev, Error **errp)
     error_setg(&multiple_devices_migration_blocker,
                "Multiple VFIO devices migration is supported only if all of "
                "them support P2P migration");
-    ret = migrate_add_blocker(&multiple_devices_migration_blocker, errp);
+    ret = migrate_add_blocker_normal(&multiple_devices_migration_blocker, errp);
 
     return ret;
 }
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index 2420100..ddae95e 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -558,10 +558,15 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
         goto free_container_exit;
     }
 
+    ret = vfio_cpr_register_container(container, errp);
+    if (ret) {
+        goto free_container_exit;
+    }
+
     ret = vfio_ram_block_discard_disable(container, true);
     if (ret) {
         error_setg_errno(errp, -ret, "Cannot set discarding of RAM broken");
-        goto free_container_exit;
+        goto unregister_container_exit;
     }
 
     switch (container->iommu_type) {
@@ -638,6 +643,9 @@ listener_release_exit:
 enable_discards_exit:
     vfio_ram_block_discard_disable(container, false);
 
+unregister_container_exit:
+    vfio_cpr_unregister_container(container);
+
 free_container_exit:
     vfio_free_container(container);
 
@@ -689,6 +697,7 @@ static void vfio_disconnect_container(VFIOGroup *group)
         }
 
         trace_vfio_disconnect_container(container->fd);
+        vfio_cpr_unregister_container(container);
         close(container->fd);
         vfio_free_container(container);
 
diff --git a/hw/vfio/cpr.c b/hw/vfio/cpr.c
new file mode 100644
index 0000000..f0282ed
--- /dev/null
+++ b/hw/vfio/cpr.c
@@ -0,0 +1,42 @@
+/*
+ * Copyright (c) 2021-2023 Oracle and/or its affiliates.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/vfio/vfio-common.h"
+#include "migration/migration.h"
+#include "migration/misc.h"
+#include "qapi/error.h"
+#include "sysemu/runstate.h"
+
+static void vfio_cpr_reboot_notifier(Notifier *notifier, void *data)
+{
+    MigrationState *s = data;
+
+    if (migrate_mode_of(s) == MIG_MODE_CPR_REBOOT &&
+        !migration_has_failed(s) &&
+        !migration_has_finished(s) &&
+        !runstate_check(RUN_STATE_SUSPENDED)) {
+
+        Error *err = NULL;
+        error_setg(&err, "VFIO device only supports cpr-reboot for "
+                         "runstate suspended");
+        migrate_set_error(s, err);
+        error_free(err);
+    }
+}
+
+int vfio_cpr_register_container(VFIOContainer *container, Error **errp)
+{
+    migration_add_notifier(&container->cpr_reboot_notifier,
+                           vfio_cpr_reboot_notifier);
+    return 0;
+}
+
+void vfio_cpr_unregister_container(VFIOContainer *container)
+{
+    migration_remove_notifier(&container->cpr_reboot_notifier);
+}
diff --git a/hw/vfio/meson.build b/hw/vfio/meson.build
index 2a6912c..3e1f9ad 100644
--- a/hw/vfio/meson.build
+++ b/hw/vfio/meson.build
@@ -5,6 +5,7 @@ vfio_ss.add(files(
   'container.c',
   'spapr.c',
   'migration.c',
+  'cpr.c',
 ))
 vfio_ss.add(when: 'CONFIG_VFIO_PCI', if_true: files(
   'display.c',
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 814132a..86d8c9e 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -902,7 +902,7 @@ static int vfio_block_migration(VFIODevice *vbasedev, Error *err, Error **errp)
     vbasedev->migration_blocker = error_copy(err);
     error_free(err);
 
-    return migrate_add_blocker(&vbasedev->migration_blocker, errp);
+    return migrate_add_blocker_normal(&vbasedev->migration_blocker, errp);
 }
 
 /* ---------------------------------------------------------------------- */
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index a4a22ac..7da1602 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -85,6 +85,7 @@ typedef struct VFIOContainer {
     int fd; /* /dev/vfio/vfio, empowered by the attached groups */
     MemoryListener listener;
     MemoryListener prereg_listener;
+    Notifier cpr_reboot_notifier;
     unsigned iommu_type;
     Error *error;
     bool initialized;
@@ -254,6 +255,9 @@ void vfio_detach_device(VFIODevice *vbasedev);
 int vfio_kvm_device_add_fd(int fd, Error **errp);
 int vfio_kvm_device_del_fd(int fd, Error **errp);
 
+int vfio_cpr_register_container(VFIOContainer *container, Error **errp);
+void vfio_cpr_unregister_container(VFIOContainer *container);
+
 extern const MemoryRegionOps vfio_region_ops;
 typedef QLIST_HEAD(VFIOGroupList, VFIOGroup) VFIOGroupList;
 typedef QLIST_HEAD(VFIODeviceList, VFIODevice) VFIODeviceList;
diff --git a/migration/ram.c b/migration/ram.c
index 8c7886a..8604b97 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2393,8 +2393,8 @@ static void ram_save_cleanup(void *opaque)
     RAMState **rsp = opaque;
     RAMBlock *block;
 
-    /* We don't use dirty log with background snapshots */
-    if (!migrate_background_snapshot()) {
+    /* We don't use dirty log with background snapshots or cpr */
+    if (!migrate_background_snapshot() && migrate_mode() == MIG_MODE_NORMAL) {
         /* caller have hold iothread lock or is in a bh, so there is
          * no writing race against the migration bitmap
          */
@@ -2805,8 +2805,9 @@ static void ram_init_bitmaps(RAMState *rs)
 
     WITH_RCU_READ_LOCK_GUARD() {
         ram_list_init_bitmaps();
-        /* We don't use dirty log with background snapshots */
-        if (!migrate_background_snapshot()) {
+        /* We don't use dirty log with background snapshots or cpr */
+        if (!migrate_background_snapshot() &&
+            migrate_mode() == MIG_MODE_NORMAL) {
             memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION);
             migration_bitmap_sync_precopy(rs, false);
         }
-- 
1.8.3.1

Re: [PATCH V1 1/3] migration: check mode in notifiers

[Peter Xu]

On Wed, Dec 13, 2023 at 10:11:31AM -0800, Steve Sistare wrote:
> The existing notifiers should only apply to normal mode.
> 
> No functional change.

Instead of adding such check in every notifier, why not make CPR a separate
list of notifiers?  Just like the blocker lists.

Aside of this patch, I just started to look at this "notifier" code, I
really don't think we should pass in MigrationState* into the notifiers.
IIUC we only need the "state" as an enum.  Then with two separate
registers, the device code knows the migration mode.

What do you think?

-- 
Peter Xu

Re: [PATCH V1 2/3] migration: notifier error reporting

[Peter Xu]

On Wed, Dec 13, 2023 at 10:11:32AM -0800, Steve Sistare wrote:
> After calling notifiers, check if an error has been reported via
> migrate_set_error, and halt the migration.
> 
> None of the notifiers call migrate_set_error at this time, so no
> functional change.
> 
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>  include/migration/misc.h |  2 +-
>  migration/migration.c    | 26 ++++++++++++++++++++++----
>  2 files changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/include/migration/misc.h b/include/migration/misc.h
> index 901d117..231d7e4 100644
> --- a/include/migration/misc.h
> +++ b/include/migration/misc.h
> @@ -65,7 +65,7 @@ MigMode migrate_mode_of(MigrationState *);
>  void migration_add_notifier(Notifier *notify,
>                              void (*func)(Notifier *notifier, void *data));
>  void migration_remove_notifier(Notifier *notify);
> -void migration_call_notifiers(MigrationState *s);
> +int migration_call_notifiers(MigrationState *s);
>  bool migration_in_setup(MigrationState *);
>  bool migration_has_finished(MigrationState *);
>  bool migration_has_failed(MigrationState *);
> diff --git a/migration/migration.c b/migration/migration.c
> index d5bfe70..29a9a92 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1280,6 +1280,8 @@ void migrate_set_state(int *state, int old_state, int new_state)
>  
>  static void migrate_fd_cleanup(MigrationState *s)
>  {
> +    bool already_failed;
> +
>      qemu_bh_delete(s->cleanup_bh);
>      s->cleanup_bh = NULL;
>  
> @@ -1327,11 +1329,20 @@ static void migrate_fd_cleanup(MigrationState *s)
>                            MIGRATION_STATUS_CANCELLED);
>      }
>  
> +    already_failed = migration_has_failed(s);
> +    if (migration_call_notifiers(s)) {
> +        if (!already_failed) {
> +            migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
> +            /* Notify again to recover from this late failure. */
> +            migration_call_notifiers(s);
> +        }
> +    }
> +
>      if (s->error) {
>          /* It is used on info migrate.  We can't free it */
>          error_report_err(error_copy(s->error));
>      }
> -    migration_call_notifiers(s);
> +
>      block_cleanup_parameters();
>      yank_unregister_instance(MIGRATION_YANK_INSTANCE);
>  }
> @@ -1450,9 +1461,10 @@ void migration_remove_notifier(Notifier *notify)
>      }
>  }
>  
> -void migration_call_notifiers(MigrationState *s)
> +int migration_call_notifiers(MigrationState *s)
>  {
>      notifier_list_notify(&migration_state_notifiers, s);
> +    return (s->error != NULL);

Exporting more migration_*() functions is pretty ugly to me..

Would it be better to pass in "Error** errp" into each notifiers?  That may
need an open coded notifier_list_notify(), breaking the loop if "*errp".

And the notifier API currently only support one arg..  maybe we should
implement the notifiers ourselves, ideally passing in "(int state, Error
**errp)" instead of "(MigrationState *s)".

Ideally with that MigrationState* shouldn't be visible outside migration/.

Thanks,

>  }
>  
>  bool migration_in_setup(MigrationState *s)
> @@ -2520,7 +2532,9 @@ static int postcopy_start(MigrationState *ms, Error **errp)
>       * spice needs to trigger a transition now
>       */
>      ms->postcopy_after_devices = true;
> -    migration_call_notifiers(ms);
> +    if (migration_call_notifiers(ms)) {
> +        goto fail;
> +    }
>  
>      migration_downtime_end(ms);
>  
> @@ -3589,7 +3603,11 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
>          rate_limit = migrate_max_bandwidth();
>  
>          /* Notify before starting migration thread */
> -        migration_call_notifiers(s);
> +        if (migration_call_notifiers(s)) {
> +            migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
> +            migrate_fd_cleanup(s);
> +            return;
> +        }
>      }
>  
>      migration_rate_set(rate_limit);
> -- 
> 1.8.3.1
> 

-- 
Peter Xu

Re: [PATCH V1 1/3] migration: check mode in notifiers

[Steven Sistare]

On 1/10/2024 2:09 AM, Peter Xu wrote:
> On Wed, Dec 13, 2023 at 10:11:31AM -0800, Steve Sistare wrote:
>> The existing notifiers should only apply to normal mode.
>>
>> No functional change.
> 
> Instead of adding such check in every notifier, why not make CPR a separate
> list of notifiers?  Just like the blocker lists.

Sure.   I proposed minimal changes in this current series, but extending the 
api to take migration mode would be nicer.

> Aside of this patch, I just started to look at this "notifier" code, I
> really don't think we should pass in MigrationState* into the notifiers.
> IIUC we only need the "state" as an enum.  Then with two separate
> registers, the device code knows the migration mode.
> 
> What do you think?

If we pass state, the notifier must either compare to enum values such as
MIGRATION_STATUS_COMPLETED instead of calling migration_has_finished(s), or
we must define new accessors such as migration_state_is_finished(state).

IMO passing MigrationState is the best approach.
MigrationState is an incomplete type in most notifiers, and the client can
pass it to a limited set of accessors to get more information -- exactly what 
we want to hide migration internals.  However, we could further limit the
allowed accessors, eg move these to a new file "include/migration/notifier.h".

----------------------------------------
#include "qemu/notify.h"
void migration_add_notifier(Notifier *notify,
                            void (*func)(Notifier *notifier, void *data));
void migration_remove_notifier(Notifier *notify);
bool migration_is_active(MigrationState *);
bool migration_in_setup(MigrationState *);
bool migration_has_finished(MigrationState *);
bool migration_has_failed(MigrationState *);
-----------------------------------------------

- Steve

Re: [PATCH V1 2/3] migration: notifier error reporting

[Steven Sistare]

On 1/10/2024 2:18 AM, Peter Xu wrote:
> On Wed, Dec 13, 2023 at 10:11:32AM -0800, Steve Sistare wrote:
>> After calling notifiers, check if an error has been reported via
>> migrate_set_error, and halt the migration.
>>
>> None of the notifiers call migrate_set_error at this time, so no
>> functional change.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>>  include/migration/misc.h |  2 +-
>>  migration/migration.c    | 26 ++++++++++++++++++++++----
>>  2 files changed, 23 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/migration/misc.h b/include/migration/misc.h
>> index 901d117..231d7e4 100644
>> --- a/include/migration/misc.h
>> +++ b/include/migration/misc.h
>> @@ -65,7 +65,7 @@ MigMode migrate_mode_of(MigrationState *);
>>  void migration_add_notifier(Notifier *notify,
>>                              void (*func)(Notifier *notifier, void *data));
>>  void migration_remove_notifier(Notifier *notify);
>> -void migration_call_notifiers(MigrationState *s);
>> +int migration_call_notifiers(MigrationState *s);
>>  bool migration_in_setup(MigrationState *);
>>  bool migration_has_finished(MigrationState *);
>>  bool migration_has_failed(MigrationState *);
>> diff --git a/migration/migration.c b/migration/migration.c
>> index d5bfe70..29a9a92 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -1280,6 +1280,8 @@ void migrate_set_state(int *state, int old_state, int new_state)
>>  
>>  static void migrate_fd_cleanup(MigrationState *s)
>>  {
>> +    bool already_failed;
>> +
>>      qemu_bh_delete(s->cleanup_bh);
>>      s->cleanup_bh = NULL;
>>  
>> @@ -1327,11 +1329,20 @@ static void migrate_fd_cleanup(MigrationState *s)
>>                            MIGRATION_STATUS_CANCELLED);
>>      }
>>  
>> +    already_failed = migration_has_failed(s);
>> +    if (migration_call_notifiers(s)) {
>> +        if (!already_failed) {
>> +            migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
>> +            /* Notify again to recover from this late failure. */
>> +            migration_call_notifiers(s);
>> +        }
>> +    }
>> +
>>      if (s->error) {
>>          /* It is used on info migrate.  We can't free it */
>>          error_report_err(error_copy(s->error));
>>      }
>> -    migration_call_notifiers(s);
>> +
>>      block_cleanup_parameters();
>>      yank_unregister_instance(MIGRATION_YANK_INSTANCE);
>>  }
>> @@ -1450,9 +1461,10 @@ void migration_remove_notifier(Notifier *notify)
>>      }
>>  }
>>  
>> -void migration_call_notifiers(MigrationState *s)
>> +int migration_call_notifiers(MigrationState *s)
>>  {
>>      notifier_list_notify(&migration_state_notifiers, s);
>> +    return (s->error != NULL);
> 
> Exporting more migration_*() functions is pretty ugly to me..

I assume you mean migrate_set_error(), which is currently only called from
migration/*.c code.

Instead, we could define a new function migrate_set_notifier_error(), defined
in the new file migration/notifier.h, so we clearly limit the migration 
functions which can be called from notifiers.  (Its implementation just calls
migrate_set_error)

> Would it be better to pass in "Error** errp" into each notifiers?  That may
> need an open coded notifier_list_notify(), breaking the loop if "*errp".
> 
> And the notifier API currently only support one arg..  maybe we should
> implement the notifiers ourselves, ideally passing in "(int state, Error
> **errp)" instead of "(MigrationState *s)".
> 
> Ideally with that MigrationState* shouldn't be visible outside migration/.

I will regret saying this because of the amount of (mechanical) code change involved,
but the cleanest solution is:

* Pass errp to: 
  notifier_with_return_list_notify(NotifierWithReturnList *list, void *data, Error *errp)
* Pass errp to the NotifierWithReturn notifier:
  int (*notify)(NotifierWithReturn *notifier, void *data, Error **errp);
* Delete the errp member from struct PostcopyNotifyData and pass errp to the notifier function
  Ditto for PrecopyNotifyData.
* Convert all migration notifiers to NotifierWithReturn

- Steve

>>  }
>>  
>>  bool migration_in_setup(MigrationState *s)
>> @@ -2520,7 +2532,9 @@ static int postcopy_start(MigrationState *ms, Error **errp)
>>       * spice needs to trigger a transition now
>>       */
>>      ms->postcopy_after_devices = true;
>> -    migration_call_notifiers(ms);
>> +    if (migration_call_notifiers(ms)) {
>> +        goto fail;
>> +    }
>>  
>>      migration_downtime_end(ms);
>>  
>> @@ -3589,7 +3603,11 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
>>          rate_limit = migrate_max_bandwidth();
>>  
>>          /* Notify before starting migration thread */
>> -        migration_call_notifiers(s);
>> +        if (migration_call_notifiers(s)) {
>> +            migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
>> +            migrate_fd_cleanup(s);
>> +            return;
>> +        }
>>      }
>>  
>>      migration_rate_set(rate_limit);
>> -- 
>> 1.8.3.1
>>
> 

Re: [PATCH V1 1/3] migration: check mode in notifiers

[Peter Xu]

On Wed, Jan 10, 2024 at 01:08:01PM -0500, Steven Sistare wrote:
> On 1/10/2024 2:09 AM, Peter Xu wrote:
> > On Wed, Dec 13, 2023 at 10:11:31AM -0800, Steve Sistare wrote:
> >> The existing notifiers should only apply to normal mode.
> >>
> >> No functional change.
> > 
> > Instead of adding such check in every notifier, why not make CPR a separate
> > list of notifiers?  Just like the blocker lists.
> 
> Sure.   I proposed minimal changes in this current series, but extending the 
> api to take migration mode would be nicer.
> 
> > Aside of this patch, I just started to look at this "notifier" code, I
> > really don't think we should pass in MigrationState* into the notifiers.
> > IIUC we only need the "state" as an enum.  Then with two separate
> > registers, the device code knows the migration mode.
> > 
> > What do you think?
> 
> If we pass state, the notifier must either compare to enum values such as
> MIGRATION_STATUS_COMPLETED instead of calling migration_has_finished(s), or
> we must define new accessors such as migration_state_is_finished(state).
> 
> IMO passing MigrationState is the best approach.
> MigrationState is an incomplete type in most notifiers, and the client can
> pass it to a limited set of accessors to get more information -- exactly what 
> we want to hide migration internals.  However, we could further limit the
> allowed accessors, eg move these to a new file "include/migration/notifier.h".
> 
> ----------------------------------------
> #include "qemu/notify.h"
> void migration_add_notifier(Notifier *notify,
>                             void (*func)(Notifier *notifier, void *data));
> void migration_remove_notifier(Notifier *notify);
> bool migration_is_active(MigrationState *);
> bool migration_in_setup(MigrationState *);
> bool migration_has_finished(MigrationState *);
> bool migration_has_failed(MigrationState *);
> -----------------------------------------------

Yes this also sounds good.  Thanks,

-- 
Peter Xu

Re: [PATCH V1 2/3] migration: notifier error reporting

[Peter Xu]

On Wed, Jan 10, 2024 at 01:08:41PM -0500, Steven Sistare wrote:
> On 1/10/2024 2:18 AM, Peter Xu wrote:
> > On Wed, Dec 13, 2023 at 10:11:32AM -0800, Steve Sistare wrote:
> >> After calling notifiers, check if an error has been reported via
> >> migrate_set_error, and halt the migration.
> >>
> >> None of the notifiers call migrate_set_error at this time, so no
> >> functional change.
> >>
> >> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> >> ---
> >>  include/migration/misc.h |  2 +-
> >>  migration/migration.c    | 26 ++++++++++++++++++++++----
> >>  2 files changed, 23 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/include/migration/misc.h b/include/migration/misc.h
> >> index 901d117..231d7e4 100644
> >> --- a/include/migration/misc.h
> >> +++ b/include/migration/misc.h
> >> @@ -65,7 +65,7 @@ MigMode migrate_mode_of(MigrationState *);
> >>  void migration_add_notifier(Notifier *notify,
> >>                              void (*func)(Notifier *notifier, void *data));
> >>  void migration_remove_notifier(Notifier *notify);
> >> -void migration_call_notifiers(MigrationState *s);
> >> +int migration_call_notifiers(MigrationState *s);
> >>  bool migration_in_setup(MigrationState *);
> >>  bool migration_has_finished(MigrationState *);
> >>  bool migration_has_failed(MigrationState *);
> >> diff --git a/migration/migration.c b/migration/migration.c
> >> index d5bfe70..29a9a92 100644
> >> --- a/migration/migration.c
> >> +++ b/migration/migration.c
> >> @@ -1280,6 +1280,8 @@ void migrate_set_state(int *state, int old_state, int new_state)
> >>  
> >>  static void migrate_fd_cleanup(MigrationState *s)
> >>  {
> >> +    bool already_failed;
> >> +
> >>      qemu_bh_delete(s->cleanup_bh);
> >>      s->cleanup_bh = NULL;
> >>  
> >> @@ -1327,11 +1329,20 @@ static void migrate_fd_cleanup(MigrationState *s)
> >>                            MIGRATION_STATUS_CANCELLED);
> >>      }
> >>  
> >> +    already_failed = migration_has_failed(s);
> >> +    if (migration_call_notifiers(s)) {
> >> +        if (!already_failed) {
> >> +            migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
> >> +            /* Notify again to recover from this late failure. */
> >> +            migration_call_notifiers(s);
> >> +        }
> >> +    }
> >> +
> >>      if (s->error) {
> >>          /* It is used on info migrate.  We can't free it */
> >>          error_report_err(error_copy(s->error));
> >>      }
> >> -    migration_call_notifiers(s);
> >> +
> >>      block_cleanup_parameters();
> >>      yank_unregister_instance(MIGRATION_YANK_INSTANCE);
> >>  }
> >> @@ -1450,9 +1461,10 @@ void migration_remove_notifier(Notifier *notify)
> >>      }
> >>  }
> >>  
> >> -void migration_call_notifiers(MigrationState *s)
> >> +int migration_call_notifiers(MigrationState *s)
> >>  {
> >>      notifier_list_notify(&migration_state_notifiers, s);
> >> +    return (s->error != NULL);
> > 
> > Exporting more migration_*() functions is pretty ugly to me..
> 
> I assume you mean migrate_set_error(), which is currently only called from
> migration/*.c code.
> 
> Instead, we could define a new function migrate_set_notifier_error(), defined
> in the new file migration/notifier.h, so we clearly limit the migration 
> functions which can be called from notifiers.  (Its implementation just calls
> migrate_set_error)

Fundementally this allows another .c to change one more field of
MigrationState (which is ->error) and I still want to avoid it.

I just replied in the other thread, but now with all these in mind I think
I still prefer not passing in MigrationState* at all.  It's already kind of
abused due to migrate_get_current(), and IMHO it's healthier to limit its
usage to minimum to cover the core of migration states for migration/ use
only.

Shrinking or even stop exporting migrate_get_current() is another more
challenging task, but now what we can do is stop enlarging the direct use
of MigrationState*.

> 
> > Would it be better to pass in "Error** errp" into each notifiers?  That may
> > need an open coded notifier_list_notify(), breaking the loop if "*errp".
> > 
> > And the notifier API currently only support one arg..  maybe we should
> > implement the notifiers ourselves, ideally passing in "(int state, Error
> > **errp)" instead of "(MigrationState *s)".
> > 
> > Ideally with that MigrationState* shouldn't be visible outside migration/.
> 
> I will regret saying this because of the amount of (mechanical) code change involved,
> but the cleanest solution is:

:)

>
> * Pass errp to: 
>   notifier_with_return_list_notify(NotifierWithReturnList *list, void *data, Error *errp)
> * Pass errp to the NotifierWithReturn notifier:
>   int (*notify)(NotifierWithReturn *notifier, void *data, Error **errp);
> * Delete the errp member from struct PostcopyNotifyData and pass errp to the notifier function
>   Ditto for PrecopyNotifyData.
> * Convert all migration notifiers to NotifierWithReturn

Would you mind changing MigrationState* into an event just like postcopy?
We don't need to use migration_has_failed() etc., afaict three events
should be enough for the existing four users, exactly like what postcopy
does:

  - MIG_EVENT_PRECOPY_SETUP
  - MIG_EVENT_PRECOPY_DONE
  - MIG_EVENT_PRECOPY_FAILED

Merging postcopy will be indeed the cleanest.  I'm okay if you want to
leave that for later, but if you'd do that together I'd appreciate that.

Thanks,

-- 
Peter Xu

Re: [PATCH V1 2/3] migration: notifier error reporting

[Steven Sistare]

On 1/10/2024 9:16 PM, Peter Xu wrote:
> On Wed, Jan 10, 2024 at 01:08:41PM -0500, Steven Sistare wrote:
>> On 1/10/2024 2:18 AM, Peter Xu wrote:
>>> On Wed, Dec 13, 2023 at 10:11:32AM -0800, Steve Sistare wrote:
>>>> After calling notifiers, check if an error has been reported via
>>>> migrate_set_error, and halt the migration.
>>>>
>>>> None of the notifiers call migrate_set_error at this time, so no
>>>> functional change.
>>>>
>>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>>> ---
>>>>  include/migration/misc.h |  2 +-
>>>>  migration/migration.c    | 26 ++++++++++++++++++++++----
>>>>  2 files changed, 23 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/include/migration/misc.h b/include/migration/misc.h
>>>> index 901d117..231d7e4 100644
>>>> --- a/include/migration/misc.h
>>>> +++ b/include/migration/misc.h
>>>> @@ -65,7 +65,7 @@ MigMode migrate_mode_of(MigrationState *);
>>>>  void migration_add_notifier(Notifier *notify,
>>>>                              void (*func)(Notifier *notifier, void *data));
>>>>  void migration_remove_notifier(Notifier *notify);
>>>> -void migration_call_notifiers(MigrationState *s);
>>>> +int migration_call_notifiers(MigrationState *s);
>>>>  bool migration_in_setup(MigrationState *);
>>>>  bool migration_has_finished(MigrationState *);
>>>>  bool migration_has_failed(MigrationState *);
>>>> diff --git a/migration/migration.c b/migration/migration.c
>>>> index d5bfe70..29a9a92 100644
>>>> --- a/migration/migration.c
>>>> +++ b/migration/migration.c
>>>> @@ -1280,6 +1280,8 @@ void migrate_set_state(int *state, int old_state, int new_state)
>>>>  
>>>>  static void migrate_fd_cleanup(MigrationState *s)
>>>>  {
>>>> +    bool already_failed;
>>>> +
>>>>      qemu_bh_delete(s->cleanup_bh);
>>>>      s->cleanup_bh = NULL;
>>>>  
>>>> @@ -1327,11 +1329,20 @@ static void migrate_fd_cleanup(MigrationState *s)
>>>>                            MIGRATION_STATUS_CANCELLED);
>>>>      }
>>>>  
>>>> +    already_failed = migration_has_failed(s);
>>>> +    if (migration_call_notifiers(s)) {
>>>> +        if (!already_failed) {
>>>> +            migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
>>>> +            /* Notify again to recover from this late failure. */
>>>> +            migration_call_notifiers(s);
>>>> +        }
>>>> +    }
>>>> +
>>>>      if (s->error) {
>>>>          /* It is used on info migrate.  We can't free it */
>>>>          error_report_err(error_copy(s->error));
>>>>      }
>>>> -    migration_call_notifiers(s);
>>>> +
>>>>      block_cleanup_parameters();
>>>>      yank_unregister_instance(MIGRATION_YANK_INSTANCE);
>>>>  }
>>>> @@ -1450,9 +1461,10 @@ void migration_remove_notifier(Notifier *notify)
>>>>      }
>>>>  }
>>>>  
>>>> -void migration_call_notifiers(MigrationState *s)
>>>> +int migration_call_notifiers(MigrationState *s)
>>>>  {
>>>>      notifier_list_notify(&migration_state_notifiers, s);
>>>> +    return (s->error != NULL);
>>>
>>> Exporting more migration_*() functions is pretty ugly to me..
>>
>> I assume you mean migrate_set_error(), which is currently only called from
>> migration/*.c code.
>>
>> Instead, we could define a new function migrate_set_notifier_error(), defined
>> in the new file migration/notifier.h, so we clearly limit the migration 
>> functions which can be called from notifiers.  (Its implementation just calls
>> migrate_set_error)
> 
> Fundementally this allows another .c to change one more field of
> MigrationState (which is ->error) and I still want to avoid it.
> 
> I just replied in the other thread, but now with all these in mind I think
> I still prefer not passing in MigrationState* at all.  It's already kind of
> abused due to migrate_get_current(), and IMHO it's healthier to limit its
> usage to minimum to cover the core of migration states for migration/ use
> only.
> 
> Shrinking or even stop exporting migrate_get_current() is another more
> challenging task, but now what we can do is stop enlarging the direct use
> of MigrationState*.
> 
>>
>>> Would it be better to pass in "Error** errp" into each notifiers?  That may
>>> need an open coded notifier_list_notify(), breaking the loop if "*errp".
>>>
>>> And the notifier API currently only support one arg..  maybe we should
>>> implement the notifiers ourselves, ideally passing in "(int state, Error
>>> **errp)" instead of "(MigrationState *s)".
>>>
>>> Ideally with that MigrationState* shouldn't be visible outside migration/.
>>
>> I will regret saying this because of the amount of (mechanical) code change involved,
>> but the cleanest solution is:
> 
> :)
> 
>>
>> * Pass errp to: 
>>   notifier_with_return_list_notify(NotifierWithReturnList *list, void *data, Error *errp)
>> * Pass errp to the NotifierWithReturn notifier:
>>   int (*notify)(NotifierWithReturn *notifier, void *data, Error **errp);
>> * Delete the errp member from struct PostcopyNotifyData and pass errp to the notifier function
>>   Ditto for PrecopyNotifyData.
>> * Convert all migration notifiers to NotifierWithReturn
> 
> Would you mind changing MigrationState* into an event just like postcopy?
> We don't need to use migration_has_failed() etc., afaict three events
> should be enough for the existing four users, exactly like what postcopy
> does:
> 
>   - MIG_EVENT_PRECOPY_SETUP
>   - MIG_EVENT_PRECOPY_DONE
>   - MIG_EVENT_PRECOPY_FAILED

Will do.

> Merging postcopy will be indeed the cleanest.  I'm okay if you want to
> leave that for later, but if you'd do that together I'd appreciate that.

Will do.

- Steve