[PATCH V2 00/11] allow cpr-reboot for vfio

[PATCH V2 00/11] 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.

Most of the patches in this series enhance migration notifiers so they can
return an error status and message.  The last two patches register a notifier
for vfio that returns an error if the guest is not suspended.

Steve Sistare (11):
  notify: pass error to notifier with return
  migration: remove error from notifier data
  migration: convert to NotifierWithReturn
  migration: remove migration_in_postcopy parameter
  migration: MigrationEvent for notifiers
  migration: MigrationNotifyFunc
  migration: per-mode notifiers
  migration: refactor migrate_fd_connect failures
  migration: notifier error checking
  vfio: register container for cpr
  vfio: allow cpr-reboot migration if suspended

 hw/net/virtio-net.c            |  14 ++---
 hw/vfio/common.c               |   2 +-
 hw/vfio/container.c            |  11 +++-
 hw/vfio/cpr.c                  |  39 ++++++++++++++
 hw/vfio/meson.build            |   1 +
 hw/vfio/migration.c            |  13 +++--
 hw/virtio/vhost-user.c         |  10 ++--
 hw/virtio/virtio-balloon.c     |   3 +-
 include/hw/vfio/vfio-common.h  |   6 ++-
 include/hw/virtio/virtio-net.h |   2 +-
 include/migration/misc.h       |  21 +++++---
 include/qemu/notify.h          |   7 ++-
 migration/migration.c          | 117 +++++++++++++++++++++++++++--------------
 migration/postcopy-ram.c       |   3 +-
 migration/postcopy-ram.h       |   1 -
 migration/ram.c                |  12 ++---
 net/vhost-vdpa.c               |  15 +++---
 ui/spice-core.c                |  19 +++----
 util/notify.c                  |   5 +-
 19 files changed, 206 insertions(+), 95 deletions(-)
 create mode 100644 hw/vfio/cpr.c

-- 
1.8.3.1

[PATCH V2 01/11] notify: pass error to notifier with return

[Steve Sistare]

Pass an error object as the third parameter to "notifier with return"
notifiers, so clients no longer need to bundle an error object in the
opaque data.  The new parameter is used in a later patch.

Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
 hw/virtio/vhost-user.c     | 2 +-
 hw/virtio/virtio-balloon.c | 3 ++-
 include/qemu/notify.h      | 7 +++++--
 migration/postcopy-ram.c   | 2 +-
 migration/ram.c            | 2 +-
 util/notify.c              | 5 +++--
 6 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index f214df8..f502345 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -2084,7 +2084,7 @@ static int vhost_user_postcopy_end(struct vhost_dev *dev, Error **errp)
 }
 
 static int vhost_user_postcopy_notifier(NotifierWithReturn *notifier,
-                                        void *opaque)
+                                        void *opaque, Error **errp)
 {
     struct PostcopyNotifyData *pnd = opaque;
     struct vhost_user *u = container_of(notifier, struct vhost_user,
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 486fe3d..89f853f 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -633,7 +633,8 @@ static void virtio_balloon_free_page_done(VirtIOBalloon *s)
 }
 
 static int
-virtio_balloon_free_page_hint_notify(NotifierWithReturn *n, void *data)
+virtio_balloon_free_page_hint_notify(NotifierWithReturn *n, void *data,
+                                     Error **errp)
 {
     VirtIOBalloon *dev = container_of(n, VirtIOBalloon, free_page_hint_notify);
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
diff --git a/include/qemu/notify.h b/include/qemu/notify.h
index bcfa70f..9a85631 100644
--- a/include/qemu/notify.h
+++ b/include/qemu/notify.h
@@ -45,12 +45,15 @@ bool notifier_list_empty(NotifierList *list);
 /* Same as Notifier but allows .notify() to return errors */
 typedef struct NotifierWithReturn NotifierWithReturn;
 
+typedef int (*NotifierWithReturnFunc)(NotifierWithReturn *notifier, void *data,
+                                      Error **errp);
+
 struct NotifierWithReturn {
     /**
      * Return 0 on success (next notifier will be invoked), otherwise
      * notifier_with_return_list_notify() will stop and return the value.
      */
-    int (*notify)(NotifierWithReturn *notifier, void *data);
+    NotifierWithReturnFunc notify;
     QLIST_ENTRY(NotifierWithReturn) node;
 };
 
@@ -69,6 +72,6 @@ void notifier_with_return_list_add(NotifierWithReturnList *list,
 void notifier_with_return_remove(NotifierWithReturn *notifier);
 
 int notifier_with_return_list_notify(NotifierWithReturnList *list,
-                                     void *data);
+                                     void *data, Error **errp);
 
 #endif
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index 5408e02..4438ac7 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -80,7 +80,7 @@ int postcopy_notify(enum PostcopyNotifyReason reason, Error **errp)
     pnd.errp = errp;
 
     return notifier_with_return_list_notify(&postcopy_notifier_list,
-                                            &pnd);
+                                            &pnd, errp);
 }
 
 /*
diff --git a/migration/ram.c b/migration/ram.c
index 890f31c..3fceba4 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -428,7 +428,7 @@ int precopy_notify(PrecopyNotifyReason reason, Error **errp)
     pnd.reason = reason;
     pnd.errp = errp;
 
-    return notifier_with_return_list_notify(&precopy_notifier_list, &pnd);
+    return notifier_with_return_list_notify(&precopy_notifier_list, &pnd, errp);
 }
 
 uint64_t ram_bytes_remaining(void)
diff --git a/util/notify.c b/util/notify.c
index 76bab21..c6e158f 100644
--- a/util/notify.c
+++ b/util/notify.c
@@ -61,13 +61,14 @@ void notifier_with_return_remove(NotifierWithReturn *notifier)
     QLIST_REMOVE(notifier, node);
 }
 
-int notifier_with_return_list_notify(NotifierWithReturnList *list, void *data)
+int notifier_with_return_list_notify(NotifierWithReturnList *list, void *data,
+                                     Error **errp)
 {
     NotifierWithReturn *notifier, *next;
     int ret = 0;
 
     QLIST_FOREACH_SAFE(notifier, &list->notifiers, node, next) {
-        ret = notifier->notify(notifier, data);
+        ret = notifier->notify(notifier, data, errp);
         if (ret != 0) {
             break;
         }
-- 
1.8.3.1

Re: [PATCH V2 01/11] notify: pass error to notifier with return

[Peter Xu]

On Fri, Jan 12, 2024 at 07:05:00AM -0800, Steve Sistare wrote:
> Pass an error object as the third parameter to "notifier with return"
> notifiers, so clients no longer need to bundle an error object in the
> opaque data.  The new parameter is used in a later patch.
> 
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>

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

-- 
Peter Xu

[PATCH V2 02/11] migration: remove error from notifier data

[Steve Sistare]

Remove the error object from opaque data passed to notifiers.
Use the new error parameter passed to the notifier instead.

Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
 hw/virtio/vhost-user.c   | 8 ++++----
 include/migration/misc.h | 1 -
 migration/postcopy-ram.c | 1 -
 migration/postcopy-ram.h | 1 -
 migration/ram.c          | 1 -
 5 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index f502345..a1eea85 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -2096,20 +2096,20 @@ static int vhost_user_postcopy_notifier(NotifierWithReturn *notifier,
         if (!virtio_has_feature(dev->protocol_features,
                                 VHOST_USER_PROTOCOL_F_PAGEFAULT)) {
             /* TODO: Get the device name into this error somehow */
-            error_setg(pnd->errp,
+            error_setg(errp,
                        "vhost-user backend not capable of postcopy");
             return -ENOENT;
         }
         break;
 
     case POSTCOPY_NOTIFY_INBOUND_ADVISE:
-        return vhost_user_postcopy_advise(dev, pnd->errp);
+        return vhost_user_postcopy_advise(dev, errp);
 
     case POSTCOPY_NOTIFY_INBOUND_LISTEN:
-        return vhost_user_postcopy_listen(dev, pnd->errp);
+        return vhost_user_postcopy_listen(dev, errp);
 
     case POSTCOPY_NOTIFY_INBOUND_END:
-        return vhost_user_postcopy_end(dev, pnd->errp);
+        return vhost_user_postcopy_end(dev, errp);
 
     default:
         /* We ignore notifications we don't know */
diff --git a/include/migration/misc.h b/include/migration/misc.h
index 1bc8902..5e65c18 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -31,7 +31,6 @@ typedef enum PrecopyNotifyReason {
 
 typedef struct PrecopyNotifyData {
     enum PrecopyNotifyReason reason;
-    Error **errp;
 } PrecopyNotifyData;
 
 void precopy_infrastructure_init(void);
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index 4438ac7..59b80dc 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -77,7 +77,6 @@ int postcopy_notify(enum PostcopyNotifyReason reason, Error **errp)
 {
     struct PostcopyNotifyData pnd;
     pnd.reason = reason;
-    pnd.errp = errp;
 
     return notifier_with_return_list_notify(&postcopy_notifier_list,
                                             &pnd, errp);
diff --git a/migration/postcopy-ram.h b/migration/postcopy-ram.h
index 442ab89..ecae941 100644
--- a/migration/postcopy-ram.h
+++ b/migration/postcopy-ram.h
@@ -128,7 +128,6 @@ enum PostcopyNotifyReason {
 
 struct PostcopyNotifyData {
     enum PostcopyNotifyReason reason;
-    Error **errp;
 };
 
 void postcopy_add_notifier(NotifierWithReturn *nn);
diff --git a/migration/ram.c b/migration/ram.c
index 3fceba4..1923366 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -426,7 +426,6 @@ int precopy_notify(PrecopyNotifyReason reason, Error **errp)
 {
     PrecopyNotifyData pnd;
     pnd.reason = reason;
-    pnd.errp = errp;
 
     return notifier_with_return_list_notify(&precopy_notifier_list, &pnd, errp);
 }
-- 
1.8.3.1

Re: [PATCH V2 02/11] migration: remove error from notifier data

[Peter Xu]

On Fri, Jan 12, 2024 at 07:05:01AM -0800, Steve Sistare wrote:
> Remove the error object from opaque data passed to notifiers.
> Use the new error parameter passed to the notifier instead.
> 
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>

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

-- 
Peter Xu

[PATCH V2 03/11] migration: convert to NotifierWithReturn

[Steve Sistare]

Change all migration notifiers to type NotifierWithReturn, so notifiers
can return an error status in a future patch.  For now, pass NULL for the
notifier error parameter, and do not check the return value.

Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
 hw/net/virtio-net.c            |  4 +++-
 hw/vfio/migration.c            |  4 +++-
 include/hw/vfio/vfio-common.h  |  2 +-
 include/hw/virtio/virtio-net.h |  2 +-
 include/migration/misc.h       |  6 +++---
 migration/migration.c          | 16 ++++++++--------
 net/vhost-vdpa.c               |  6 ++++--
 ui/spice-core.c                |  8 +++++---
 8 files changed, 28 insertions(+), 20 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 7a2846f..9570353 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3529,11 +3529,13 @@ static void virtio_net_handle_migration_primary(VirtIONet *n, MigrationState *s)
     }
 }
 
-static void virtio_net_migration_state_notifier(Notifier *notifier, void *data)
+static int virtio_net_migration_state_notifier(NotifierWithReturn *notifier,
+                                               void *data, Error **errp)
 {
     MigrationState *s = data;
     VirtIONet *n = container_of(notifier, VirtIONet, migration_state);
     virtio_net_handle_migration_primary(n, s);
+    return 0;
 }
 
 static bool failover_hide_primary_device(DeviceListener *listener,
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 70e6b1a..6b6acc4 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -754,7 +754,8 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
                               mig_state_to_str(new_state));
 }
 
-static void vfio_migration_state_notifier(Notifier *notifier, void *data)
+static int vfio_migration_state_notifier(NotifierWithReturn *notifier,
+                                         void *data, Error **errp)
 {
     MigrationState *s = data;
     VFIOMigration *migration = container_of(notifier, VFIOMigration,
@@ -770,6 +771,7 @@ static void vfio_migration_state_notifier(Notifier *notifier, void *data)
     case MIGRATION_STATUS_FAILED:
         vfio_migration_set_state_or_reset(vbasedev, VFIO_DEVICE_STATE_RUNNING);
     }
+    return 0;
 }
 
 static void vfio_migration_free(VFIODevice *vbasedev)
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 9b7ef7d..4a6c262 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -62,7 +62,7 @@ typedef struct VFIORegion {
 typedef struct VFIOMigration {
     struct VFIODevice *vbasedev;
     VMChangeStateEntry *vm_state;
-    Notifier migration_state;
+    NotifierWithReturn migration_state;
     uint32_t device_state;
     int data_fd;
     void *data_buffer;
diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
index 55977f0..eaee8f4 100644
--- a/include/hw/virtio/virtio-net.h
+++ b/include/hw/virtio/virtio-net.h
@@ -221,7 +221,7 @@ struct VirtIONet {
     DeviceListener primary_listener;
     QDict *primary_opts;
     bool primary_opts_from_json;
-    Notifier migration_state;
+    NotifierWithReturn migration_state;
     VirtioNetRssData rss_data;
     struct NetRxPkt *rx_pkt;
     struct EBPFRSSContext ebpf_rss;
diff --git a/include/migration/misc.h b/include/migration/misc.h
index 5e65c18..b62e351 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -60,9 +60,9 @@ void migration_object_init(void);
 void migration_shutdown(void);
 bool migration_is_idle(void);
 bool migration_is_active(MigrationState *);
-void migration_add_notifier(Notifier *notify,
-                            void (*func)(Notifier *notifier, void *data));
-void migration_remove_notifier(Notifier *notify);
+void migration_add_notifier(NotifierWithReturn *notify,
+                            NotifierWithReturnFunc func);
+void migration_remove_notifier(NotifierWithReturn *notify);
 void migration_call_notifiers(MigrationState *s);
 bool migration_in_setup(MigrationState *);
 bool migration_has_finished(MigrationState *);
diff --git a/migration/migration.c b/migration/migration.c
index 98c5c3e..fa662a5 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -68,8 +68,8 @@
 #include "sysemu/dirtylimit.h"
 #include "qemu/sockets.h"
 
-static NotifierList migration_state_notifiers =
-    NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
+static NotifierWithReturnList migration_state_notifiers =
+    NOTIFIER_WITH_RETURN_LIST_INITIALIZER(migration_state_notifiers);
 
 /* Messages sent on the return path from destination to source */
 enum mig_rp_message_type {
@@ -1424,24 +1424,24 @@ static void migrate_fd_cancel(MigrationState *s)
     }
 }
 
-void migration_add_notifier(Notifier *notify,
-                            void (*func)(Notifier *notifier, void *data))
+void migration_add_notifier(NotifierWithReturn *notify,
+                            NotifierWithReturnFunc func)
 {
     notify->notify = func;
-    notifier_list_add(&migration_state_notifiers, notify);
+    notifier_with_return_list_add(&migration_state_notifiers, notify);
 }
 
-void migration_remove_notifier(Notifier *notify)
+void migration_remove_notifier(NotifierWithReturn *notify)
 {
     if (notify->notify) {
-        notifier_remove(notify);
+        notifier_with_return_remove(notify);
         notify->notify = NULL;
     }
 }
 
 void migration_call_notifiers(MigrationState *s)
 {
-    notifier_list_notify(&migration_state_notifiers, s);
+    notifier_with_return_list_notify(&migration_state_notifiers, s, 0);
 }
 
 bool migration_in_setup(MigrationState *s)
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 3726ee5..1c00519 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -34,7 +34,7 @@
 typedef struct VhostVDPAState {
     NetClientState nc;
     struct vhost_vdpa vhost_vdpa;
-    Notifier migration_state;
+    NotifierWithReturn migration_state;
     VHostNetState *vhost_net;
 
     /* Control commands shadow buffers */
@@ -322,7 +322,8 @@ static void vhost_vdpa_net_log_global_enable(VhostVDPAState *s, bool enable)
     }
 }
 
-static void vdpa_net_migration_state_notifier(Notifier *notifier, void *data)
+static int vdpa_net_migration_state_notifier(NotifierWithReturn *notifier,
+                                             void *data, Error **errp)
 {
     MigrationState *migration = data;
     VhostVDPAState *s = container_of(notifier, VhostVDPAState,
@@ -333,6 +334,7 @@ static void vdpa_net_migration_state_notifier(Notifier *notifier, void *data)
     } else if (migration_has_failed(migration)) {
         vhost_vdpa_net_log_global_enable(s, false);
     }
+    return 0;
 }
 
 static void vhost_vdpa_net_data_start_first(VhostVDPAState *s)
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 37b277f..b3cd229 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -42,7 +42,7 @@
 /* core bits */
 
 static SpiceServer *spice_server;
-static Notifier migration_state;
+static NotifierWithReturn migration_state;
 static const char *auth = "spice";
 static char *auth_passwd;
 static time_t auth_expires = TIME_MAX;
@@ -568,12 +568,13 @@ static SpiceInfo *qmp_query_spice_real(Error **errp)
     return info;
 }
 
-static void migration_state_notifier(Notifier *notifier, void *data)
+static int migration_state_notifier(NotifierWithReturn *notifier,
+                                    void *data, Error **errp)
 {
     MigrationState *s = data;
 
     if (!spice_have_target_host) {
-        return;
+        return 0;
     }
 
     if (migration_in_setup(s)) {
@@ -586,6 +587,7 @@ static void migration_state_notifier(Notifier *notifier, void *data)
         spice_server_migrate_end(spice_server, false);
         spice_have_target_host = false;
     }
+    return 0;
 }
 
 int qemu_spice_migrate_info(const char *hostname, int port, int tls_port,
-- 
1.8.3.1

Re: [PATCH V2 03/11] migration: convert to NotifierWithReturn

[Peter Xu]

On Fri, Jan 12, 2024 at 07:05:02AM -0800, Steve Sistare wrote:
> Change all migration notifiers to type NotifierWithReturn, so notifiers
> can return an error status in a future patch.  For now, pass NULL for the
> notifier error parameter, and do not check the return value.
> 
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>  hw/net/virtio-net.c            |  4 +++-
>  hw/vfio/migration.c            |  4 +++-
>  include/hw/vfio/vfio-common.h  |  2 +-
>  include/hw/virtio/virtio-net.h |  2 +-
>  include/migration/misc.h       |  6 +++---
>  migration/migration.c          | 16 ++++++++--------
>  net/vhost-vdpa.c               |  6 ++++--
>  ui/spice-core.c                |  8 +++++---
>  8 files changed, 28 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 7a2846f..9570353 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -3529,11 +3529,13 @@ static void virtio_net_handle_migration_primary(VirtIONet *n, MigrationState *s)
>      }
>  }
>  
> -static void virtio_net_migration_state_notifier(Notifier *notifier, void *data)
> +static int virtio_net_migration_state_notifier(NotifierWithReturn *notifier,
> +                                               void *data, Error **errp)
>  {
>      MigrationState *s = data;
>      VirtIONet *n = container_of(notifier, VirtIONet, migration_state);
>      virtio_net_handle_migration_primary(n, s);
> +    return 0;
>  }

I should have mentioned this earlier.. we have a trend recently to modify
retval to boolean when Error** existed, e.g.:

https://lore.kernel.org/all/20231017202633.296756-5-peterx@redhat.com/

Let's start using boolean too here?  Previous patches may need touch-ups
too for this.

Other than that it all looks good here.  Thanks,

-- 
Peter Xu

Re: [PATCH V2 03/11] migration: convert to NotifierWithReturn

[Steven Sistare]

On 1/15/2024 1:44 AM, Peter Xu wrote:
> On Fri, Jan 12, 2024 at 07:05:02AM -0800, Steve Sistare wrote:
>> Change all migration notifiers to type NotifierWithReturn, so notifiers
>> can return an error status in a future patch.  For now, pass NULL for the
>> notifier error parameter, and do not check the return value.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>>  hw/net/virtio-net.c            |  4 +++-
>>  hw/vfio/migration.c            |  4 +++-
>>  include/hw/vfio/vfio-common.h  |  2 +-
>>  include/hw/virtio/virtio-net.h |  2 +-
>>  include/migration/misc.h       |  6 +++---
>>  migration/migration.c          | 16 ++++++++--------
>>  net/vhost-vdpa.c               |  6 ++++--
>>  ui/spice-core.c                |  8 +++++---
>>  8 files changed, 28 insertions(+), 20 deletions(-)
>>
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index 7a2846f..9570353 100644
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -3529,11 +3529,13 @@ static void virtio_net_handle_migration_primary(VirtIONet *n, MigrationState *s)
>>      }
>>  }
>>  
>> -static void virtio_net_migration_state_notifier(Notifier *notifier, void *data)
>> +static int virtio_net_migration_state_notifier(NotifierWithReturn *notifier,
>> +                                               void *data, Error **errp)
>>  {
>>      MigrationState *s = data;
>>      VirtIONet *n = container_of(notifier, VirtIONet, migration_state);
>>      virtio_net_handle_migration_primary(n, s);
>> +    return 0;
>>  }
> 
> I should have mentioned this earlier.. we have a trend recently to modify
> retval to boolean when Error** existed, e.g.:
> 
> https://lore.kernel.org/all/20231017202633.296756-5-peterx@redhat.com/
> 
> Let's start using boolean too here?  Previous patches may need touch-ups
> too for this.
> 
> Other than that it all looks good here.  Thanks,

Boolean makes sense when there is only one way to recover from failure.
However, when the notifier may fail in different ways, and recovery differs
for each, then the function should return an int errno.  NotifierWithReturn
could have future uses that need multiple return values and multiple recovery 
paths above the notifier_with_return_list_notify level, so IMO the function 
should continue to return int for generality.

- Steve

Re: [PATCH V2 03/11] migration: convert to NotifierWithReturn

[Peter Xu]

On Tue, Jan 16, 2024 at 03:35:53PM -0500, Steven Sistare wrote:
> On 1/15/2024 1:44 AM, Peter Xu wrote:
> > On Fri, Jan 12, 2024 at 07:05:02AM -0800, Steve Sistare wrote:
> >> Change all migration notifiers to type NotifierWithReturn, so notifiers
> >> can return an error status in a future patch.  For now, pass NULL for the
> >> notifier error parameter, and do not check the return value.
> >>
> >> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> >> ---
> >>  hw/net/virtio-net.c            |  4 +++-
> >>  hw/vfio/migration.c            |  4 +++-
> >>  include/hw/vfio/vfio-common.h  |  2 +-
> >>  include/hw/virtio/virtio-net.h |  2 +-
> >>  include/migration/misc.h       |  6 +++---
> >>  migration/migration.c          | 16 ++++++++--------
> >>  net/vhost-vdpa.c               |  6 ++++--
> >>  ui/spice-core.c                |  8 +++++---
> >>  8 files changed, 28 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >> index 7a2846f..9570353 100644
> >> --- a/hw/net/virtio-net.c
> >> +++ b/hw/net/virtio-net.c
> >> @@ -3529,11 +3529,13 @@ static void virtio_net_handle_migration_primary(VirtIONet *n, MigrationState *s)
> >>      }
> >>  }
> >>  
> >> -static void virtio_net_migration_state_notifier(Notifier *notifier, void *data)
> >> +static int virtio_net_migration_state_notifier(NotifierWithReturn *notifier,
> >> +                                               void *data, Error **errp)
> >>  {
> >>      MigrationState *s = data;
> >>      VirtIONet *n = container_of(notifier, VirtIONet, migration_state);
> >>      virtio_net_handle_migration_primary(n, s);
> >> +    return 0;
> >>  }
> > 
> > I should have mentioned this earlier.. we have a trend recently to modify
> > retval to boolean when Error** existed, e.g.:
> > 
> > https://lore.kernel.org/all/20231017202633.296756-5-peterx@redhat.com/
> > 
> > Let's start using boolean too here?  Previous patches may need touch-ups
> > too for this.
> > 
> > Other than that it all looks good here.  Thanks,
> 
> Boolean makes sense when there is only one way to recover from failure.
> However, when the notifier may fail in different ways, and recovery differs
> for each, then the function should return an int errno.  NotifierWithReturn
> could have future uses that need multiple return values and multiple recovery 
> paths above the notifier_with_return_list_notify level, so IMO the function 
> should continue to return int for generality.

Ah ok.  Please then add a comment either in the commit message or code for
that.  Thanks.

-- 
Peter Xu

[PATCH V2 04/11] migration: remove migration_in_postcopy parameter

[Steve Sistare]

Delete the explicit MigrationState parameter from migration_in_postcopy,
so we can eliminate MigrationState from notifiers in a later patch.
No functional change.

Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
 include/migration/misc.h |  2 +-
 migration/migration.c    | 27 ++++++++++++++++-----------
 ui/spice-core.c          |  2 +-
 3 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/include/migration/misc.h b/include/migration/misc.h
index b62e351..dcc98bb 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -68,7 +68,7 @@ bool migration_in_setup(MigrationState *);
 bool migration_has_finished(MigrationState *);
 bool migration_has_failed(MigrationState *);
 /* ...and after the device transmission */
-bool migration_in_postcopy_after_devices(MigrationState *);
+bool migration_in_postcopy_after_devices(void);
 /* True if incoming migration entered POSTCOPY_INCOMING_DISCARD */
 bool migration_in_incoming_postcopy(void);
 /* True if incoming migration entered POSTCOPY_INCOMING_ADVISE */
diff --git a/migration/migration.c b/migration/migration.c
index fa662a5..ad3acd1 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1460,18 +1460,22 @@ bool migration_has_failed(MigrationState *s)
             s->state == MIGRATION_STATUS_FAILED);
 }
 
+static bool migration_state_in_postcopy(MigrationState *s)
+{
+    switch (s->state) {
+    case MIGRATION_STATUS_POSTCOPY_ACTIVE:
+    case MIGRATION_STATUS_POSTCOPY_PAUSED:
+    case MIGRATION_STATUS_POSTCOPY_RECOVER:
+        return true;
+    default:
+        return false;
+    }
+}
+
 bool migration_in_postcopy(void)
 {
     MigrationState *s = migrate_get_current();
-
-    switch (s->state) {
-    case MIGRATION_STATUS_POSTCOPY_ACTIVE:
-    case MIGRATION_STATUS_POSTCOPY_PAUSED:
-    case MIGRATION_STATUS_POSTCOPY_RECOVER:
-        return true;
-    default:
-        return false;
-    }
+    return migration_state_in_postcopy(s);
 }
 
 bool migration_postcopy_is_alive(int state)
@@ -1485,9 +1489,10 @@ bool migration_postcopy_is_alive(int state)
     }
 }
 
-bool migration_in_postcopy_after_devices(MigrationState *s)
+bool migration_in_postcopy_after_devices(void)
 {
-    return migration_in_postcopy() && s->postcopy_after_devices;
+    MigrationState *s = migrate_get_current();
+    return migration_state_in_postcopy(s) && s->postcopy_after_devices;
 }
 
 bool migration_in_incoming_postcopy(void)
diff --git a/ui/spice-core.c b/ui/spice-core.c
index b3cd229..e43a93f 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -580,7 +580,7 @@ static int migration_state_notifier(NotifierWithReturn *notifier,
     if (migration_in_setup(s)) {
         spice_server_migrate_start(spice_server);
     } else if (migration_has_finished(s) ||
-               migration_in_postcopy_after_devices(s)) {
+               migration_in_postcopy_after_devices()) {
         spice_server_migrate_end(spice_server, true);
         spice_have_target_host = false;
     } else if (migration_has_failed(s)) {
-- 
1.8.3.1

Re: [PATCH V2 04/11] migration: remove migration_in_postcopy parameter

[Peter Xu]

On Fri, Jan 12, 2024 at 07:05:03AM -0800, Steve Sistare wrote:
>  bool migration_in_incoming_postcopy(void)
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index b3cd229..e43a93f 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -580,7 +580,7 @@ static int migration_state_notifier(NotifierWithReturn *notifier,
>      if (migration_in_setup(s)) {
>          spice_server_migrate_start(spice_server);
>      } else if (migration_has_finished(s) ||
> -               migration_in_postcopy_after_devices(s)) {
> +               migration_in_postcopy_after_devices()) {

This can be a reply also to your other email: my previous suggestion of
using PRECOPY_DONE should apply here, where we can convert this chunk into:

  } else if (event == MIG_EVENT_PRECOPY_DONE) {...}

Because PRECOPY_DONE should also cover the notification from
postcopy_start(), then we can drop migration_in_postcopy_after_devices()
completely, I think.

-- 
Peter Xu

Re: [PATCH V2 04/11] migration: remove migration_in_postcopy parameter

[Steven Sistare]

On 1/15/2024 1:48 AM, Peter Xu wrote:
> On Fri, Jan 12, 2024 at 07:05:03AM -0800, Steve Sistare wrote:
>>  bool migration_in_incoming_postcopy(void)
>> diff --git a/ui/spice-core.c b/ui/spice-core.c
>> index b3cd229..e43a93f 100644
>> --- a/ui/spice-core.c
>> +++ b/ui/spice-core.c
>> @@ -580,7 +580,7 @@ static int migration_state_notifier(NotifierWithReturn *notifier,
>>      if (migration_in_setup(s)) {
>>          spice_server_migrate_start(spice_server);
>>      } else if (migration_has_finished(s) ||
>> -               migration_in_postcopy_after_devices(s)) {
>> +               migration_in_postcopy_after_devices()) {
> 
> This can be a reply also to your other email: my previous suggestion of
> using PRECOPY_DONE should apply here, where we can convert this chunk into:
> 
>   } else if (event == MIG_EVENT_PRECOPY_DONE) {...}
> 
> Because PRECOPY_DONE should also cover the notification from
> postcopy_start(), then we can drop migration_in_postcopy_after_devices()
> completely, I think.

Yes, that works.  I will define and use MIG_EVENT_PRECOPY_DONE and friends in 
"MigrationEvent for notifiers".

- Steve

[PATCH V2 05/11] migration: MigrationEvent for notifiers

[Steve Sistare]

Passing MigrationState to notifiers is unsound because they could access
unstable migration state internals or even modify the state.  Instead, pass
the minimal info needed in a new MigrationEvent struct, which could be
extended in the future if needed.

Suggested-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
 hw/net/virtio-net.c      | 12 +++++++-----
 hw/vfio/migration.c      |  8 +++++---
 include/migration/misc.h |  5 +++++
 migration/migration.c    |  5 ++++-
 net/vhost-vdpa.c         |  7 ++++---
 ui/spice-core.c          |  9 +++++----
 6 files changed, 30 insertions(+), 16 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 9570353..71e1133 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3499,7 +3499,7 @@ out:
     return !err;
 }
 
-static void virtio_net_handle_migration_primary(VirtIONet *n, MigrationState *s)
+static void virtio_net_handle_migration_primary(VirtIONet *n, MigrationEvent *e)
 {
     bool should_be_hidden;
     Error *err = NULL;
@@ -3511,7 +3511,7 @@ static void virtio_net_handle_migration_primary(VirtIONet *n, MigrationState *s)
 
     should_be_hidden = qatomic_read(&n->failover_primary_hidden);
 
-    if (migration_in_setup(s) && !should_be_hidden) {
+    if (e->state == MIGRATION_STATUS_SETUP && !should_be_hidden) {
         if (failover_unplug_primary(n, dev)) {
             vmstate_unregister(VMSTATE_IF(dev), qdev_get_vmsd(dev), dev);
             qapi_event_send_unplug_primary(dev->id);
@@ -3519,7 +3519,8 @@ static void virtio_net_handle_migration_primary(VirtIONet *n, MigrationState *s)
         } else {
             warn_report("couldn't unplug primary device");
         }
-    } else if (migration_has_failed(s)) {
+    } else if (e->state == MIGRATION_STATUS_FAILED ||
+               e->state == MIGRATION_STATUS_CANCELLED) {
         /* We already unplugged the device let's plug it back */
         if (!failover_replug_primary(n, dev, &err)) {
             if (err) {
@@ -3532,9 +3533,10 @@ static void virtio_net_handle_migration_primary(VirtIONet *n, MigrationState *s)
 static int virtio_net_migration_state_notifier(NotifierWithReturn *notifier,
                                                void *data, Error **errp)
 {
-    MigrationState *s = data;
+    MigrationEvent *e = data;
+
     VirtIONet *n = container_of(notifier, VirtIONet, migration_state);
-    virtio_net_handle_migration_primary(n, s);
+    virtio_net_handle_migration_primary(n, e);
     return 0;
 }
 
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 6b6acc4..746ec08 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -757,19 +757,21 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
 static int vfio_migration_state_notifier(NotifierWithReturn *notifier,
                                          void *data, Error **errp)
 {
-    MigrationState *s = data;
+    MigrationEvent *e = data;
     VFIOMigration *migration = container_of(notifier, VFIOMigration,
                                             migration_state);
     VFIODevice *vbasedev = migration->vbasedev;
 
     trace_vfio_migration_state_notifier(vbasedev->name,
-                                        MigrationStatus_str(s->state));
+                                        MigrationStatus_str(e->state));
 
-    switch (s->state) {
+    switch (e->state) {
     case MIGRATION_STATUS_CANCELLING:
     case MIGRATION_STATUS_CANCELLED:
     case MIGRATION_STATUS_FAILED:
         vfio_migration_set_state_or_reset(vbasedev, VFIO_DEVICE_STATE_RUNNING);
+    default:
+        break;
     }
     return 0;
 }
diff --git a/include/migration/misc.h b/include/migration/misc.h
index dcc98bb..0b4ce0f 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -60,6 +60,11 @@ void migration_object_init(void);
 void migration_shutdown(void);
 bool migration_is_idle(void);
 bool migration_is_active(MigrationState *);
+
+typedef struct MigrationEvent {
+    MigrationStatus state;
+} MigrationEvent;
+
 void migration_add_notifier(NotifierWithReturn *notify,
                             NotifierWithReturnFunc func);
 void migration_remove_notifier(NotifierWithReturn *notify);
diff --git a/migration/migration.c b/migration/migration.c
index ad3acd1..4c5180c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1441,7 +1441,10 @@ void migration_remove_notifier(NotifierWithReturn *notify)
 
 void migration_call_notifiers(MigrationState *s)
 {
-    notifier_with_return_list_notify(&migration_state_notifiers, s, 0);
+    MigrationEvent e;
+
+    e.state = s->state;
+    notifier_with_return_list_notify(&migration_state_notifiers, &e, 0);
 }
 
 bool migration_in_setup(MigrationState *s)
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 1c00519..f96ac75 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -325,13 +325,14 @@ static void vhost_vdpa_net_log_global_enable(VhostVDPAState *s, bool enable)
 static int vdpa_net_migration_state_notifier(NotifierWithReturn *notifier,
                                              void *data, Error **errp)
 {
-    MigrationState *migration = data;
+    MigrationEvent *e = data;
     VhostVDPAState *s = container_of(notifier, VhostVDPAState,
                                      migration_state);
 
-    if (migration_in_setup(migration)) {
+    if (e->state == MIGRATION_STATUS_SETUP) {
         vhost_vdpa_net_log_global_enable(s, true);
-    } else if (migration_has_failed(migration)) {
+    } else if (e->state == MIGRATION_STATUS_FAILED ||
+               e->state == MIGRATION_STATUS_CANCELLED) {
         vhost_vdpa_net_log_global_enable(s, false);
     }
     return 0;
diff --git a/ui/spice-core.c b/ui/spice-core.c
index e43a93f..74aa3bc 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -571,19 +571,20 @@ static SpiceInfo *qmp_query_spice_real(Error **errp)
 static int migration_state_notifier(NotifierWithReturn *notifier,
                                     void *data, Error **errp)
 {
-    MigrationState *s = data;
+    MigrationEvent *e = data;
 
     if (!spice_have_target_host) {
         return 0;
     }
 
-    if (migration_in_setup(s)) {
+    if (e->state == MIGRATION_STATUS_SETUP) {
         spice_server_migrate_start(spice_server);
-    } else if (migration_has_finished(s) ||
+    } else if (e->state == MIGRATION_STATUS_COMPLETED ||
                migration_in_postcopy_after_devices()) {
         spice_server_migrate_end(spice_server, true);
         spice_have_target_host = false;
-    } else if (migration_has_failed(s)) {
+    } else if (e->state == MIGRATION_STATUS_FAILED ||
+               e->state == MIGRATION_STATUS_CANCELLED) {
         spice_server_migrate_end(spice_server, false);
         spice_have_target_host = false;
     }
-- 
1.8.3.1

Re: [PATCH V2 05/11] migration: MigrationEvent for notifiers

[Steven Sistare]

On 1/12/2024 10:05 AM, Steve Sistare wrote:
> Passing MigrationState to notifiers is unsound because they could access
> unstable migration state internals or even modify the state.  Instead, pass
> the minimal info needed in a new MigrationEvent struct, which could be
> extended in the future if needed.
> 
> Suggested-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>  [...]
> diff --git a/include/migration/misc.h b/include/migration/misc.h
> index dcc98bb..0b4ce0f 100644
> --- a/include/migration/misc.h
> +++ b/include/migration/misc.h
> @@ -60,6 +60,11 @@ void migration_object_init(void);
>  void migration_shutdown(void);
>  bool migration_is_idle(void);
>  bool migration_is_active(MigrationState *);
> +
> +typedef struct MigrationEvent {
> +    MigrationStatus state;
> +} MigrationEvent;

Hi Peter, I chose to pass MigrationStatus rather than define a new enum and map
MigrationStatus to it.  IMO a new enum adds little value, yet it is more code and
another layer of abstraction for coders to grok.  

- Steve

[PATCH V2 06/11] migration: MigrationNotifyFunc

[Steve Sistare]

Define MigrationNotifyFunc to improve type safety and simplify migration
notifiers.

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

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 71e1133..f7f2c3b 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3531,10 +3531,8 @@ static void virtio_net_handle_migration_primary(VirtIONet *n, MigrationEvent *e)
 }
 
 static int virtio_net_migration_state_notifier(NotifierWithReturn *notifier,
-                                               void *data, Error **errp)
+                                               MigrationEvent *e, Error **errp)
 {
-    MigrationEvent *e = data;
-
     VirtIONet *n = container_of(notifier, VirtIONet, migration_state);
     virtio_net_handle_migration_primary(n, e);
     return 0;
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 746ec08..534fddf 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -755,9 +755,8 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
 }
 
 static int vfio_migration_state_notifier(NotifierWithReturn *notifier,
-                                         void *data, Error **errp)
+                                         MigrationEvent *e, Error **errp)
 {
-    MigrationEvent *e = data;
     VFIOMigration *migration = container_of(notifier, VFIOMigration,
                                             migration_state);
     VFIODevice *vbasedev = migration->vbasedev;
diff --git a/include/migration/misc.h b/include/migration/misc.h
index 0b4ce0f..8eeb9d5 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -65,8 +65,11 @@ typedef struct MigrationEvent {
     MigrationStatus state;
 } MigrationEvent;
 
+typedef int (*MigrationNotifyFunc)(NotifierWithReturn *notify,
+                                  MigrationEvent *e, Error **errp);
+
 void migration_add_notifier(NotifierWithReturn *notify,
-                            NotifierWithReturnFunc func);
+                            MigrationNotifyFunc func);
 void migration_remove_notifier(NotifierWithReturn *notify);
 void migration_call_notifiers(MigrationState *s);
 bool migration_in_setup(MigrationState *);
diff --git a/migration/migration.c b/migration/migration.c
index 4c5180c..33288bc 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1425,9 +1425,9 @@ static void migrate_fd_cancel(MigrationState *s)
 }
 
 void migration_add_notifier(NotifierWithReturn *notify,
-                            NotifierWithReturnFunc func)
+                            MigrationNotifyFunc func)
 {
-    notify->notify = func;
+    notify->notify = (NotifierWithReturnFunc)func;
     notifier_with_return_list_add(&migration_state_notifiers, notify);
 }
 
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index f96ac75..bffa666 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -323,11 +323,9 @@ static void vhost_vdpa_net_log_global_enable(VhostVDPAState *s, bool enable)
 }
 
 static int vdpa_net_migration_state_notifier(NotifierWithReturn *notifier,
-                                             void *data, Error **errp)
+                                             MigrationEvent *e, Error **errp)
 {
-    MigrationEvent *e = data;
-    VhostVDPAState *s = container_of(notifier, VhostVDPAState,
-                                     migration_state);
+    VhostVDPAState *s = container_of(notifier, VhostVDPAState, migration_state);
 
     if (e->state == MIGRATION_STATUS_SETUP) {
         vhost_vdpa_net_log_global_enable(s, true);
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 74aa3bc..1fdcd1a 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -569,10 +569,8 @@ static SpiceInfo *qmp_query_spice_real(Error **errp)
 }
 
 static int migration_state_notifier(NotifierWithReturn *notifier,
-                                    void *data, Error **errp)
+                                    MigrationEvent *e, Error **errp)
 {
-    MigrationEvent *e = data;
-
     if (!spice_have_target_host) {
         return 0;
     }
-- 
1.8.3.1

[PATCH V2 07/11] migration: per-mode notifiers

[Steve Sistare]

Keep a separate list of migration notifiers for each migration mode.

Suggested-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
 include/migration/misc.h |  2 ++
 migration/migration.c    | 22 +++++++++++++++++-----
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/include/migration/misc.h b/include/migration/misc.h
index 8eeb9d5..e79694f 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -70,6 +70,8 @@ typedef int (*MigrationNotifyFunc)(NotifierWithReturn *notify,
 
 void migration_add_notifier(NotifierWithReturn *notify,
                             MigrationNotifyFunc func);
+void migration_add_notifier_mode(NotifierWithReturn *notify,
+                                 MigrationNotifyFunc func, MigMode mode);
 void migration_remove_notifier(NotifierWithReturn *notify);
 void migration_call_notifiers(MigrationState *s);
 bool migration_in_setup(MigrationState *);
diff --git a/migration/migration.c b/migration/migration.c
index 33288bc..e9914aa 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -68,8 +68,13 @@
 #include "sysemu/dirtylimit.h"
 #include "qemu/sockets.h"
 
-static NotifierWithReturnList migration_state_notifiers =
-    NOTIFIER_WITH_RETURN_LIST_INITIALIZER(migration_state_notifiers);
+#define NOTIFIER_ELEM_INIT(array, elem)    \
+    [elem] = NOTIFIER_WITH_RETURN_LIST_INITIALIZER((array)[elem])
+
+static NotifierWithReturnList migration_state_notifiers[] = {
+    NOTIFIER_ELEM_INIT(migration_state_notifiers, MIG_MODE_NORMAL),
+    NOTIFIER_ELEM_INIT(migration_state_notifiers, MIG_MODE_CPR_REBOOT),
+};
 
 /* Messages sent on the return path from destination to source */
 enum mig_rp_message_type {
@@ -1424,11 +1429,17 @@ static void migrate_fd_cancel(MigrationState *s)
     }
 }
 
+void migration_add_notifier_mode(NotifierWithReturn *notify,
+                                 MigrationNotifyFunc func, MigMode mode)
+{
+    notify->notify = (NotifierWithReturnFunc)func;
+    notifier_with_return_list_add(&migration_state_notifiers[mode], notify);
+}
+
 void migration_add_notifier(NotifierWithReturn *notify,
                             MigrationNotifyFunc func)
 {
-    notify->notify = (NotifierWithReturnFunc)func;
-    notifier_with_return_list_add(&migration_state_notifiers, notify);
+    migration_add_notifier_mode(notify, func, MIG_MODE_NORMAL);
 }
 
 void migration_remove_notifier(NotifierWithReturn *notify)
@@ -1441,10 +1452,11 @@ void migration_remove_notifier(NotifierWithReturn *notify)
 
 void migration_call_notifiers(MigrationState *s)
 {
+    MigMode mode = s->parameters.mode;
     MigrationEvent e;
 
     e.state = s->state;
-    notifier_with_return_list_notify(&migration_state_notifiers, &e, 0);
+    notifier_with_return_list_notify(&migration_state_notifiers[mode], &e, 0);
 }
 
 bool migration_in_setup(MigrationState *s)
-- 
1.8.3.1

[PATCH V2 08/11] migration: refactor migrate_fd_connect failures

[Steve Sistare]

Move common code for the error path in migrate_fd_connect to a shared
fail label.  No functional change.

Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
 migration/migration.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index e9914aa..c828ba7 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3549,6 +3549,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
     Error *local_err = NULL;
     uint64_t rate_limit;
     bool resume = s->state == MIGRATION_STATUS_POSTCOPY_PAUSED;
+    int expect_state = s->state;
 
     /*
      * If there's a previous error, free it and prepare for another one.
@@ -3603,11 +3604,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
     if (migrate_postcopy_ram() || migrate_return_path()) {
         if (open_return_path_on_source(s)) {
             error_setg(&local_err, "Unable to open return-path for postcopy");
-            migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
-            migrate_set_error(s, local_err);
-            error_report_err(local_err);
-            migrate_fd_cleanup(s);
-            return;
+            goto fail;
         }
     }
 
@@ -3629,12 +3626,8 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
     }
 
     if (multifd_save_setup(&local_err) != 0) {
-        migrate_set_error(s, local_err);
-        error_report_err(local_err);
-        migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
-                          MIGRATION_STATUS_FAILED);
-        migrate_fd_cleanup(s);
-        return;
+        expect_state = MIGRATION_STATUS_SETUP;
+        goto fail;
     }
 
     if (migrate_background_snapshot()) {
@@ -3645,6 +3638,13 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
                 migration_thread, s, QEMU_THREAD_JOINABLE);
     }
     s->migration_thread_running = true;
+    return;
+
+fail:
+    migrate_set_error(s, local_err);
+    migrate_set_state(&s->state, expect_state, MIGRATION_STATUS_FAILED);
+    error_report_err(local_err);
+    migrate_fd_cleanup(s);
 }
 
 static void migration_class_init(ObjectClass *klass, void *data)
-- 
1.8.3.1

Re: [PATCH V2 08/11] migration: refactor migrate_fd_connect failures

[Peter Xu]

On Fri, Jan 12, 2024 at 07:05:07AM -0800, Steve Sistare wrote:
> Move common code for the error path in migrate_fd_connect to a shared
> fail label.  No functional change.
> 
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>

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

One nitpick below:

> ---
>  migration/migration.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index e9914aa..c828ba7 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -3549,6 +3549,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
>      Error *local_err = NULL;
>      uint64_t rate_limit;
>      bool resume = s->state == MIGRATION_STATUS_POSTCOPY_PAUSED;
> +    int expect_state = s->state;

IMHO we can drop this.

>  
>      /*
>       * If there's a previous error, free it and prepare for another one.
> @@ -3603,11 +3604,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
>      if (migrate_postcopy_ram() || migrate_return_path()) {
>          if (open_return_path_on_source(s)) {
>              error_setg(&local_err, "Unable to open return-path for postcopy");
> -            migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
> -            migrate_set_error(s, local_err);
> -            error_report_err(local_err);
> -            migrate_fd_cleanup(s);
> -            return;
> +            goto fail;
>          }
>      }
>  
> @@ -3629,12 +3626,8 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
>      }
>  
>      if (multifd_save_setup(&local_err) != 0) {
> -        migrate_set_error(s, local_err);
> -        error_report_err(local_err);
> -        migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
> -                          MIGRATION_STATUS_FAILED);
> -        migrate_fd_cleanup(s);
> -        return;
> +        expect_state = MIGRATION_STATUS_SETUP;

Then drop this.

> +        goto fail;
>      }
>  
>      if (migrate_background_snapshot()) {
> @@ -3645,6 +3638,13 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
>                  migration_thread, s, QEMU_THREAD_JOINABLE);
>      }
>      s->migration_thread_running = true;
> +    return;
> +
> +fail:
> +    migrate_set_error(s, local_err);
> +    migrate_set_state(&s->state, expect_state, MIGRATION_STATUS_FAILED);

Then constantly use s->state.

> +    error_report_err(local_err);
> +    migrate_fd_cleanup(s);
>  }
>  
>  static void migration_class_init(ObjectClass *klass, void *data)
> -- 
> 1.8.3.1
> 

-- 
Peter Xu

Re: [PATCH V2 08/11] migration: refactor migrate_fd_connect failures

[Steven Sistare]

On 1/15/2024 2:37 AM, Peter Xu wrote:
> On Fri, Jan 12, 2024 at 07:05:07AM -0800, Steve Sistare wrote:
>> Move common code for the error path in migrate_fd_connect to a shared
>> fail label.  No functional change.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> 
> Reviewed-by: Peter Xu <peterx@redhat.com>
> 
> One nitpick below:
> 
>> ---
>>  migration/migration.c | 22 +++++++++++-----------
>>  1 file changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index e9914aa..c828ba7 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -3549,6 +3549,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
>>      Error *local_err = NULL;
>>      uint64_t rate_limit;
>>      bool resume = s->state == MIGRATION_STATUS_POSTCOPY_PAUSED;
>> +    int expect_state = s->state;
> 
> IMHO we can drop this.
> 
>>  
>>      /*
>>       * If there's a previous error, free it and prepare for another one.
>> @@ -3603,11 +3604,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
>>      if (migrate_postcopy_ram() || migrate_return_path()) {
>>          if (open_return_path_on_source(s)) {
>>              error_setg(&local_err, "Unable to open return-path for postcopy");
>> -            migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
>> -            migrate_set_error(s, local_err);
>> -            error_report_err(local_err);
>> -            migrate_fd_cleanup(s);
>> -            return;
>> +            goto fail;
>>          }
>>      }
>>  
>> @@ -3629,12 +3626,8 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
>>      }
>>  
>>      if (multifd_save_setup(&local_err) != 0) {
>> -        migrate_set_error(s, local_err);
>> -        error_report_err(local_err);
>> -        migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
>> -                          MIGRATION_STATUS_FAILED);
>> -        migrate_fd_cleanup(s);
>> -        return;
>> +        expect_state = MIGRATION_STATUS_SETUP;
> 
> Then drop this.
> 
>> +        goto fail;
>>      }
>>  
>>      if (migrate_background_snapshot()) {
>> @@ -3645,6 +3638,13 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
>>                  migration_thread, s, QEMU_THREAD_JOINABLE);
>>      }
>>      s->migration_thread_running = true;
>> +    return;
>> +
>> +fail:
>> +    migrate_set_error(s, local_err);
>> +    migrate_set_state(&s->state, expect_state, MIGRATION_STATUS_FAILED);
> 
> Then constantly use s->state.

Yes, if you are OK with it, I also prefer to simplify here.

- Steve

>> +    error_report_err(local_err);
>> +    migrate_fd_cleanup(s);
>>  }
>>  
>>  static void migration_class_init(ObjectClass *klass, void *data)
>> -- 
>> 1.8.3.1
>>
> 

[PATCH V2 09/11] migration: notifier error checking

[Steve Sistare]

Check the status returned by migration notifiers and report errors.
If notifiers fail, call the notifiers again so they can clean up.
None of the notifiers return an error status at this time.

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

diff --git a/include/migration/misc.h b/include/migration/misc.h
index e79694f..782bdec 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -73,7 +73,7 @@ void migration_add_notifier(NotifierWithReturn *notify,
 void migration_add_notifier_mode(NotifierWithReturn *notify,
                                  MigrationNotifyFunc func, MigMode mode);
 void migration_remove_notifier(NotifierWithReturn *notify);
-void migration_call_notifiers(MigrationState *s);
+int migration_call_notifiers(MigrationState *s, Error **errp);
 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 c828ba7..86a5a94 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1274,6 +1274,8 @@ void migrate_set_state(int *state, int old_state, int new_state)
 
 static void migrate_fd_cleanup(MigrationState *s)
 {
+    Error *local_err = NULL;
+
     qemu_bh_delete(s->cleanup_bh);
     s->cleanup_bh = NULL;
 
@@ -1321,11 +1323,23 @@ static void migrate_fd_cleanup(MigrationState *s)
                           MIGRATION_STATUS_CANCELLED);
     }
 
+    if (!migration_has_failed(s) &&
+        migration_call_notifiers(s, &local_err)) {
+
+        migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
+        migrate_set_error(s, local_err);
+        error_free(local_err);
+    }
+
     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);
+
+    if (migration_has_failed(s)) {
+        migration_call_notifiers(s, NULL);
+    }
+
     block_cleanup_parameters();
     yank_unregister_instance(MIGRATION_YANK_INSTANCE);
 }
@@ -1450,13 +1464,14 @@ void migration_remove_notifier(NotifierWithReturn *notify)
     }
 }
 
-void migration_call_notifiers(MigrationState *s)
+int migration_call_notifiers(MigrationState *s, Error **errp)
 {
     MigMode mode = s->parameters.mode;
     MigrationEvent e;
 
     e.state = s->state;
-    notifier_with_return_list_notify(&migration_state_notifiers[mode], &e, 0);
+    return notifier_with_return_list_notify(&migration_state_notifiers[mode],
+                                            &e, errp);
 }
 
 bool migration_in_setup(MigrationState *s)
@@ -2520,7 +2535,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, errp)) {
+        goto fail;
+    }
 
     migration_downtime_end(ms);
 
@@ -2540,11 +2557,10 @@ static int postcopy_start(MigrationState *ms, Error **errp)
 
     ret = qemu_file_get_error(ms->to_dst_file);
     if (ret) {
-        error_setg(errp, "postcopy_start: Migration stream errored");
-        migrate_set_state(&ms->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
-                              MIGRATION_STATUS_FAILED);
+        error_setg_errno(errp, -ret, "postcopy_start: Migration stream error");
+        bql_lock();
+        goto fail;
     }
-
     trace_postcopy_preempt_enabled(migrate_postcopy_preempt());
 
     return ret;
@@ -2565,6 +2581,7 @@ fail:
             error_report_err(local_err);
         }
     }
+    migration_call_notifiers(ms, NULL);         /* Notify about failure */
     bql_unlock();
     return -1;
 }
@@ -3590,7 +3607,9 @@ 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, &local_err)) {
+            goto fail;
+        }
     }
 
     migration_rate_set(rate_limit);
-- 
1.8.3.1

[PATCH V2 10/11] vfio: register container for cpr

[Steve Sistare]

Define entry points to perform per-container cpr-specific initialization
and teardown.

Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
 hw/vfio/container.c           | 11 ++++++++++-
 hw/vfio/cpr.c                 | 19 +++++++++++++++++++
 hw/vfio/meson.build           |  1 +
 include/hw/vfio/vfio-common.h |  3 +++
 4 files changed, 33 insertions(+), 1 deletion(-)
 create mode 100644 hw/vfio/cpr.c

diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index bd25b9f..33a64bc 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -621,10 +621,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;
     }
 
     assert(bcontainer->ops->setup);
@@ -667,6 +672,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:
     g_free(container);
 
@@ -710,6 +718,7 @@ static void vfio_disconnect_container(VFIOGroup *group)
         vfio_container_destroy(bcontainer);
 
         trace_vfio_disconnect_container(container->fd);
+        vfio_cpr_unregister_container(container);
         close(container->fd);
         g_free(container);
 
diff --git a/hw/vfio/cpr.c b/hw/vfio/cpr.c
new file mode 100644
index 0000000..bbd1c7a
--- /dev/null
+++ b/hw/vfio/cpr.c
@@ -0,0 +1,19 @@
+/*
+ * Copyright (c) 2021-2024 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 "qapi/error.h"
+
+int vfio_cpr_register_container(VFIOContainer *container, Error **errp)
+{
+    return 0;
+}
+
+void vfio_cpr_unregister_container(VFIOContainer *container)
+{
+}
diff --git a/hw/vfio/meson.build b/hw/vfio/meson.build
index bb98493..bba776f 100644
--- a/hw/vfio/meson.build
+++ b/hw/vfio/meson.build
@@ -5,6 +5,7 @@ vfio_ss.add(files(
   'container-base.c',
   'container.c',
   'migration.c',
+  'cpr.c',
 ))
 vfio_ss.add(when: 'CONFIG_PSERIES', if_true: files('spapr.c'))
 vfio_ss.add(when: 'CONFIG_IOMMUFD', if_true: files(
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 4a6c262..1add5b7 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -205,6 +205,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;
-- 
1.8.3.1

[PATCH V2 11/11] 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>
---
 hw/vfio/common.c              |  2 +-
 hw/vfio/cpr.c                 | 20 ++++++++++++++++++++
 hw/vfio/migration.c           |  2 +-
 include/hw/vfio/vfio-common.h |  1 +
 migration/ram.c               |  9 +++++----
 5 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 0b3352f..09af934 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/cpr.c b/hw/vfio/cpr.c
index bbd1c7a..9f4b1fe 100644
--- a/hw/vfio/cpr.c
+++ b/hw/vfio/cpr.c
@@ -7,13 +7,33 @@
 
 #include "qemu/osdep.h"
 #include "hw/vfio/vfio-common.h"
+#include "migration/misc.h"
 #include "qapi/error.h"
+#include "sysemu/runstate.h"
+
+static int vfio_cpr_reboot_notifier(NotifierWithReturn *notifier,
+                                    MigrationEvent *e, Error **errp)
+{
+    if (e->state == MIGRATION_STATUS_SETUP &&
+        !runstate_check(RUN_STATE_SUSPENDED)) {
+
+        error_setg(errp,
+            "VFIO device only supports cpr-reboot for runstate suspended");
+
+        return -1;
+    }
+    return 0;
+}
 
 int vfio_cpr_register_container(VFIOContainer *container, Error **errp)
 {
+    migration_add_notifier_mode(&container->cpr_reboot_notifier,
+                                vfio_cpr_reboot_notifier,
+                                MIG_MODE_CPR_REBOOT);
     return 0;
 }
 
 void vfio_cpr_unregister_container(VFIOContainer *container)
 {
+    migration_remove_notifier(&container->cpr_reboot_notifier);
 }
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 534fddf..488905d 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -895,7 +895,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 1add5b7..7a46e24 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -78,6 +78,7 @@ struct VFIOGroup;
 typedef struct VFIOContainer {
     VFIOContainerBase bcontainer;
     int fd; /* /dev/vfio/vfio, empowered by the attached groups */
+    NotifierWithReturn cpr_reboot_notifier;
     unsigned iommu_type;
     QLIST_HEAD(, VFIOGroup) group_list;
 } VFIOContainer;
diff --git a/migration/ram.c b/migration/ram.c
index 1923366..44ad324 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2392,8 +2392,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 BQL or is in a bh, so there is
          * no writing race against the migration bitmap
          */
@@ -2804,8 +2804,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 V2 11/11] vfio: allow cpr-reboot migration if suspended

[Peter Xu]

On Fri, Jan 12, 2024 at 07:05:10AM -0800, Steve Sistare wrote:
> 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>
> ---
>  hw/vfio/common.c              |  2 +-
>  hw/vfio/cpr.c                 | 20 ++++++++++++++++++++
>  hw/vfio/migration.c           |  2 +-
>  include/hw/vfio/vfio-common.h |  1 +
>  migration/ram.c               |  9 +++++----
>  5 files changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 0b3352f..09af934 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/cpr.c b/hw/vfio/cpr.c
> index bbd1c7a..9f4b1fe 100644
> --- a/hw/vfio/cpr.c
> +++ b/hw/vfio/cpr.c
> @@ -7,13 +7,33 @@
>  
>  #include "qemu/osdep.h"
>  #include "hw/vfio/vfio-common.h"
> +#include "migration/misc.h"
>  #include "qapi/error.h"
> +#include "sysemu/runstate.h"
> +
> +static int vfio_cpr_reboot_notifier(NotifierWithReturn *notifier,
> +                                    MigrationEvent *e, Error **errp)
> +{
> +    if (e->state == MIGRATION_STATUS_SETUP &&
> +        !runstate_check(RUN_STATE_SUSPENDED)) {
> +
> +        error_setg(errp,
> +            "VFIO device only supports cpr-reboot for runstate suspended");
> +
> +        return -1;
> +    }

What happens if the guest is suspended during SETUP, but then quickly waked
up before CPR migration completes?

> +    return 0;
> +}
>  
>  int vfio_cpr_register_container(VFIOContainer *container, Error **errp)
>  {
> +    migration_add_notifier_mode(&container->cpr_reboot_notifier,
> +                                vfio_cpr_reboot_notifier,
> +                                MIG_MODE_CPR_REBOOT);
>      return 0;
>  }
>  
>  void vfio_cpr_unregister_container(VFIOContainer *container)
>  {
> +    migration_remove_notifier(&container->cpr_reboot_notifier);
>  }
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index 534fddf..488905d 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -895,7 +895,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 1add5b7..7a46e24 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -78,6 +78,7 @@ struct VFIOGroup;
>  typedef struct VFIOContainer {
>      VFIOContainerBase bcontainer;
>      int fd; /* /dev/vfio/vfio, empowered by the attached groups */
> +    NotifierWithReturn cpr_reboot_notifier;
>      unsigned iommu_type;
>      QLIST_HEAD(, VFIOGroup) group_list;
>  } VFIOContainer;
> diff --git a/migration/ram.c b/migration/ram.c
> index 1923366..44ad324 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2392,8 +2392,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) {

Same question here, on what happens if the user resumes the VM before
migration completes?  IIUC shared-ram is not required, then it means if
that happens the cpr migration image can contain corrupted data, and that
may be a problem.

Background snapshot is special in that it relies on totally different
tracking facilities (userfault, rather than KVM_GET_DIRTY_LOG), so it
disabled dirty tracking completely.  I assume not the case for cpr.

If cpr is not going to support that use case, IIUC it should fail that
system wakeup properly.  Or perhaps when CPR mode QEMU should instead
reject a wakeup?

>          /* caller have hold BQL or is in a bh, so there is
>           * no writing race against the migration bitmap
>           */
> @@ -2804,8 +2804,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
> 

-- 
Peter Xu

Re: [PATCH V2 11/11] vfio: allow cpr-reboot migration if suspended

[Steven Sistare]

On 1/15/2024 2:33 AM, Peter Xu wrote:
> On Fri, Jan 12, 2024 at 07:05:10AM -0800, Steve Sistare wrote:
>> 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>
>> ---
>>  hw/vfio/common.c              |  2 +-
>>  hw/vfio/cpr.c                 | 20 ++++++++++++++++++++
>>  hw/vfio/migration.c           |  2 +-
>>  include/hw/vfio/vfio-common.h |  1 +
>>  migration/ram.c               |  9 +++++----
>>  5 files changed, 28 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 0b3352f..09af934 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/cpr.c b/hw/vfio/cpr.c
>> index bbd1c7a..9f4b1fe 100644
>> --- a/hw/vfio/cpr.c
>> +++ b/hw/vfio/cpr.c
>> @@ -7,13 +7,33 @@
>>  
>>  #include "qemu/osdep.h"
>>  #include "hw/vfio/vfio-common.h"
>> +#include "migration/misc.h"
>>  #include "qapi/error.h"
>> +#include "sysemu/runstate.h"
>> +
>> +static int vfio_cpr_reboot_notifier(NotifierWithReturn *notifier,
>> +                                    MigrationEvent *e, Error **errp)
>> +{
>> +    if (e->state == MIGRATION_STATUS_SETUP &&
>> +        !runstate_check(RUN_STATE_SUSPENDED)) {
>> +
>> +        error_setg(errp,
>> +            "VFIO device only supports cpr-reboot for runstate suspended");
>> +
>> +        return -1;
>> +    }
> 
> What happens if the guest is suspended during SETUP, but then quickly waked
> up before CPR migration completes?

That is a management layer bug -- we told them the VM must be suspended.
However, I will reject a wakeup request if migration is active and mode is cpr.

>> +    return 0;
>> +}
>>  
>>  int vfio_cpr_register_container(VFIOContainer *container, Error **errp)
>>  {
>> +    migration_add_notifier_mode(&container->cpr_reboot_notifier,
>> +                                vfio_cpr_reboot_notifier,
>> +                                MIG_MODE_CPR_REBOOT);
>>      return 0;
>>  }
>>  
>>  void vfio_cpr_unregister_container(VFIOContainer *container)
>>  {
>> +    migration_remove_notifier(&container->cpr_reboot_notifier);
>>  }
>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>> index 534fddf..488905d 100644
>> --- a/hw/vfio/migration.c
>> +++ b/hw/vfio/migration.c
>> @@ -895,7 +895,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 1add5b7..7a46e24 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -78,6 +78,7 @@ struct VFIOGroup;
>>  typedef struct VFIOContainer {
>>      VFIOContainerBase bcontainer;
>>      int fd; /* /dev/vfio/vfio, empowered by the attached groups */
>> +    NotifierWithReturn cpr_reboot_notifier;
>>      unsigned iommu_type;
>>      QLIST_HEAD(, VFIOGroup) group_list;
>>  } VFIOContainer;
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 1923366..44ad324 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -2392,8 +2392,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) {
> 
> Same question here, on what happens if the user resumes the VM before
> migration completes?  IIUC shared-ram is not required, then it means if
> that happens the cpr migration image can contain corrupted data, and that
> may be a problem.
> 
> Background snapshot is special in that it relies on totally different
> tracking facilities (userfault, rather than KVM_GET_DIRTY_LOG), so it
> disabled dirty tracking completely.  I assume not the case for cpr.
> 
> If cpr is not going to support that use case, IIUC it should fail that
> system wakeup properly.  Or perhaps when CPR mode QEMU should instead
> reject a wakeup?

Good catch, this hunk would break the non-vfio case where the guest can be
running when migration is initiated.  I should narrow the logic to check for
that:

    if (!migrate_background_snapshot() &&
        (migrate_mode() == MIG_MODE_NORMAL || runstate_is_running()) {
        ... use dirty logging ...

That plus rejecting a wakeup request should be sufficient.

- Steve

>>          /* caller have hold BQL or is in a bh, so there is
>>           * no writing race against the migration bitmap
>>           */
>> @@ -2804,8 +2804,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 V2 11/11] vfio: allow cpr-reboot migration if suspended

[Steven Sistare]

On 1/16/2024 3:37 PM, Steven Sistare wrote:
> On 1/15/2024 2:33 AM, Peter Xu wrote:
>> On Fri, Jan 12, 2024 at 07:05:10AM -0800, Steve Sistare wrote:
>>> 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>
>>> ---
>>>  hw/vfio/common.c              |  2 +-
>>>  hw/vfio/cpr.c                 | 20 ++++++++++++++++++++
>>>  hw/vfio/migration.c           |  2 +-
>>>  include/hw/vfio/vfio-common.h |  1 +
>>>  migration/ram.c               |  9 +++++----
>>>  5 files changed, 28 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>>> index 0b3352f..09af934 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/cpr.c b/hw/vfio/cpr.c
>>> index bbd1c7a..9f4b1fe 100644
>>> --- a/hw/vfio/cpr.c
>>> +++ b/hw/vfio/cpr.c
>>> @@ -7,13 +7,33 @@
>>>  
>>>  #include "qemu/osdep.h"
>>>  #include "hw/vfio/vfio-common.h"
>>> +#include "migration/misc.h"
>>>  #include "qapi/error.h"
>>> +#include "sysemu/runstate.h"
>>> +
>>> +static int vfio_cpr_reboot_notifier(NotifierWithReturn *notifier,
>>> +                                    MigrationEvent *e, Error **errp)
>>> +{
>>> +    if (e->state == MIGRATION_STATUS_SETUP &&
>>> +        !runstate_check(RUN_STATE_SUSPENDED)) {
>>> +
>>> +        error_setg(errp,
>>> +            "VFIO device only supports cpr-reboot for runstate suspended");
>>> +
>>> +        return -1;
>>> +    }
>>
>> What happens if the guest is suspended during SETUP, but then quickly waked
>> up before CPR migration completes?
> 
> That is a management layer bug -- we told them the VM must be suspended.
> However, I will reject a wakeup request if migration is active and mode is cpr.
> 
>>> +    return 0;
>>> +}
>>>  
>>>  int vfio_cpr_register_container(VFIOContainer *container, Error **errp)
>>>  {
>>> +    migration_add_notifier_mode(&container->cpr_reboot_notifier,
>>> +                                vfio_cpr_reboot_notifier,
>>> +                                MIG_MODE_CPR_REBOOT);
>>>      return 0;
>>>  }
>>>  
>>>  void vfio_cpr_unregister_container(VFIOContainer *container)
>>>  {
>>> +    migration_remove_notifier(&container->cpr_reboot_notifier);
>>>  }
>>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>>> index 534fddf..488905d 100644
>>> --- a/hw/vfio/migration.c
>>> +++ b/hw/vfio/migration.c
>>> @@ -895,7 +895,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 1add5b7..7a46e24 100644
>>> --- a/include/hw/vfio/vfio-common.h
>>> +++ b/include/hw/vfio/vfio-common.h
>>> @@ -78,6 +78,7 @@ struct VFIOGroup;
>>>  typedef struct VFIOContainer {
>>>      VFIOContainerBase bcontainer;
>>>      int fd; /* /dev/vfio/vfio, empowered by the attached groups */
>>> +    NotifierWithReturn cpr_reboot_notifier;
>>>      unsigned iommu_type;
>>>      QLIST_HEAD(, VFIOGroup) group_list;
>>>  } VFIOContainer;
>>> diff --git a/migration/ram.c b/migration/ram.c
>>> index 1923366..44ad324 100644
>>> --- a/migration/ram.c
>>> +++ b/migration/ram.c
>>> @@ -2392,8 +2392,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) {
>>
>> Same question here, on what happens if the user resumes the VM before
>> migration completes?  IIUC shared-ram is not required, then it means if
>> that happens the cpr migration image can contain corrupted data, and that
>> may be a problem.
>>
>> Background snapshot is special in that it relies on totally different
>> tracking facilities (userfault, rather than KVM_GET_DIRTY_LOG), so it
>> disabled dirty tracking completely.  I assume not the case for cpr.
>>
>> If cpr is not going to support that use case, IIUC it should fail that
>> system wakeup properly.  Or perhaps when CPR mode QEMU should instead
>> reject a wakeup?
> 
> Good catch, this hunk would break the non-vfio case where the guest can be
> running when migration is initiated.  I should narrow the logic to check for
> that:
> 
>     if (!migrate_background_snapshot() &&
>         (migrate_mode() == MIG_MODE_NORMAL || runstate_is_running()) {

Correction: I need to specifically check for the suspended state (RUNSTATE_SUSPENDED
or vm_was_suspended), since I will not reject "cont from stopped" requests.

- Steve

>         ... use dirty logging ...
> 
> That plus rejecting a wakeup request should be sufficient.
> 
>>>          /* caller have hold BQL or is in a bh, so there is
>>>           * no writing race against the migration bitmap
>>>           */
>>> @@ -2804,8 +2804,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 V2 11/11] vfio: allow cpr-reboot migration if suspended

[Peter Xu]

On Tue, Jan 16, 2024 at 03:37:33PM -0500, Steven Sistare wrote:
> On 1/15/2024 2:33 AM, Peter Xu wrote:
> > On Fri, Jan 12, 2024 at 07:05:10AM -0800, Steve Sistare wrote:
> >> 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>
> >> ---
> >>  hw/vfio/common.c              |  2 +-
> >>  hw/vfio/cpr.c                 | 20 ++++++++++++++++++++
> >>  hw/vfio/migration.c           |  2 +-
> >>  include/hw/vfio/vfio-common.h |  1 +
> >>  migration/ram.c               |  9 +++++----
> >>  5 files changed, 28 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> >> index 0b3352f..09af934 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/cpr.c b/hw/vfio/cpr.c
> >> index bbd1c7a..9f4b1fe 100644
> >> --- a/hw/vfio/cpr.c
> >> +++ b/hw/vfio/cpr.c
> >> @@ -7,13 +7,33 @@
> >>  
> >>  #include "qemu/osdep.h"
> >>  #include "hw/vfio/vfio-common.h"
> >> +#include "migration/misc.h"
> >>  #include "qapi/error.h"
> >> +#include "sysemu/runstate.h"
> >> +
> >> +static int vfio_cpr_reboot_notifier(NotifierWithReturn *notifier,
> >> +                                    MigrationEvent *e, Error **errp)
> >> +{
> >> +    if (e->state == MIGRATION_STATUS_SETUP &&
> >> +        !runstate_check(RUN_STATE_SUSPENDED)) {
> >> +
> >> +        error_setg(errp,
> >> +            "VFIO device only supports cpr-reboot for runstate suspended");
> >> +
> >> +        return -1;
> >> +    }
> > 
> > What happens if the guest is suspended during SETUP, but then quickly waked
> > up before CPR migration completes?
> 
> That is a management layer bug -- we told them the VM must be suspended.
> However, I will reject a wakeup request if migration is active and mode is cpr.

Yes it needs to be enforced if it is required by cpr-reboot.  QEMU
hopefully should never rely on mgmt app behavior.

> 
> >> +    return 0;
> >> +}
> >>  
> >>  int vfio_cpr_register_container(VFIOContainer *container, Error **errp)
> >>  {
> >> +    migration_add_notifier_mode(&container->cpr_reboot_notifier,
> >> +                                vfio_cpr_reboot_notifier,
> >> +                                MIG_MODE_CPR_REBOOT);
> >>      return 0;
> >>  }
> >>  
> >>  void vfio_cpr_unregister_container(VFIOContainer *container)
> >>  {
> >> +    migration_remove_notifier(&container->cpr_reboot_notifier);
> >>  }
> >> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> >> index 534fddf..488905d 100644
> >> --- a/hw/vfio/migration.c
> >> +++ b/hw/vfio/migration.c
> >> @@ -895,7 +895,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 1add5b7..7a46e24 100644
> >> --- a/include/hw/vfio/vfio-common.h
> >> +++ b/include/hw/vfio/vfio-common.h
> >> @@ -78,6 +78,7 @@ struct VFIOGroup;
> >>  typedef struct VFIOContainer {
> >>      VFIOContainerBase bcontainer;
> >>      int fd; /* /dev/vfio/vfio, empowered by the attached groups */
> >> +    NotifierWithReturn cpr_reboot_notifier;
> >>      unsigned iommu_type;
> >>      QLIST_HEAD(, VFIOGroup) group_list;
> >>  } VFIOContainer;
> >> diff --git a/migration/ram.c b/migration/ram.c
> >> index 1923366..44ad324 100644
> >> --- a/migration/ram.c
> >> +++ b/migration/ram.c
> >> @@ -2392,8 +2392,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) {
> > 
> > Same question here, on what happens if the user resumes the VM before
> > migration completes?  IIUC shared-ram is not required, then it means if
> > that happens the cpr migration image can contain corrupted data, and that
> > may be a problem.
> > 
> > Background snapshot is special in that it relies on totally different
> > tracking facilities (userfault, rather than KVM_GET_DIRTY_LOG), so it
> > disabled dirty tracking completely.  I assume not the case for cpr.
> > 
> > If cpr is not going to support that use case, IIUC it should fail that
> > system wakeup properly.  Or perhaps when CPR mode QEMU should instead
> > reject a wakeup?
> 
> Good catch, this hunk would break the non-vfio case where the guest can be
> running when migration is initiated.  I should narrow the logic to check for
> that:
> 
>     if (!migrate_background_snapshot() &&
>         (migrate_mode() == MIG_MODE_NORMAL || runstate_is_running()) {
>         ... use dirty logging ...
> 
> That plus rejecting a wakeup request should be sufficient.

Sounds good.

Please also consider adding "VM suspended for the whole cpr-reboot
migration process" into the cpr-reboot mode qapi/ documentation as a
hard requirement.  I don't see that mentioned at all, then people will be
surprised to see a wakeup failure otherwise.

Frankly, every time I re-read the cpr-reboot description I get confused on
what is implied and enforced in this mode.  Can we provide an update?

# @cpr-reboot: The migrate command saves state to a file, allowing one to
#              quit qemu, reboot to an updated kernel, and restart an updated
#              version of qemu.  The caller must specify a migration URI
#              that writes to and reads from a file.  Unlike normal mode,
#              the use of certain local storage options does not block the
#              migration, but the caller must not modify guest block devices
#              between the quit and restart.  To avoid saving guest RAM to the
#              file, the memory backend must be shared, and the @x-ignore-shared
#              migration capability must be set.  Guest RAM must be non-volatile
#              across reboot, such as by backing it with a dax device, but this
#              is not enforced.  The restarted qemu arguments must match those
#              used to initially start qemu, plus the -incoming option.
#              (since 8.2)

For example, is shared-mem enfornced?  AFAIU it's not required for now, at
least I don't remember any code to stop cpr-reboot migration if there's
private memories.

What about the image location restriction on non-volatile?  Is it required?
IIUC "guest ram must be non-volatile across reboot" asks more than what we
need to document for cpr-reboot.  For example, can someone use cpr-reboot
to upgrade the userspace virt stack, but doesn't want to upgrade the
kernel?  Then I assume dax is not required, since anyone can use a
memory-based file (like shmem, hugetlbfs, or even guest_memfd in the future
of security contexts)?

I think it's okay to keep such descriptions, but the document (so far only
exists in qapi/migration.json) should clearly describe what must be done,
separated from what is suggested.  The current version is still unclear,
IMHO, and it'll be good to amend it when adding the hard requirement on
suspended mode.  AFAIU the suspended mode helps:

  (1) vfio devices all stateless (vfio use case specific)

  (2) no dirty tracking required (vfio + non-vfio use case, e.g., merely no
      restriction on what block type the VM uses)

Also, since we already have cpr-reboot mode merged, we need a solid
document file describing this feature.  Maybe "what is suggested" should be
put into that document instead, keeping the qapi document the essential
documents.  Above explanations can also be good material to be mentioned
there (if my understanding of why suspended is a hard requirement is
correct..).

For that, feel free to pull the lastest qemu master branch, then you can
create a file under docs/devel/migration/, possibly, cpr-reboot.rst, and
link that into the features.rst file.  Currently we lack of docs for
migration features, but we do have plan to enrich it to some degree soon:

  https://www.qemu.org/docs/master/devel/migration/features.html

So it would be good to have cpr-reboot since the 1st day the whole feature
is there.  The doc can be posted separately if you prefer, so I can merge
the doc series even sooner (one patch to update qapi/, one to add that file
to docs/, perhaps).  You can also maintain missing items as TODO in the
same .rst file, depending on how much work is still leftover, then remove
entries alongside of your patches merged, which I mostly suggested the same
to fixed-ram.

Thanks,

-- 
Peter Xu

Re: [PATCH V2 11/11] vfio: allow cpr-reboot migration if suspended

[Steven Sistare]

On 1/17/2024 2:12 AM, Peter Xu wrote:
> On Tue, Jan 16, 2024 at 03:37:33PM -0500, Steven Sistare wrote:
>> On 1/15/2024 2:33 AM, Peter Xu wrote:
>>> On Fri, Jan 12, 2024 at 07:05:10AM -0800, Steve Sistare wrote:
>>>> 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>
>>>> ---
>>>>  hw/vfio/common.c              |  2 +-
>>>>  hw/vfio/cpr.c                 | 20 ++++++++++++++++++++
>>>>  hw/vfio/migration.c           |  2 +-
>>>>  include/hw/vfio/vfio-common.h |  1 +
>>>>  migration/ram.c               |  9 +++++----
>>>>  5 files changed, 28 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>>>> index 0b3352f..09af934 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/cpr.c b/hw/vfio/cpr.c
>>>> index bbd1c7a..9f4b1fe 100644
>>>> --- a/hw/vfio/cpr.c
>>>> +++ b/hw/vfio/cpr.c
>>>> @@ -7,13 +7,33 @@
>>>>  
>>>>  #include "qemu/osdep.h"
>>>>  #include "hw/vfio/vfio-common.h"
>>>> +#include "migration/misc.h"
>>>>  #include "qapi/error.h"
>>>> +#include "sysemu/runstate.h"
>>>> +
>>>> +static int vfio_cpr_reboot_notifier(NotifierWithReturn *notifier,
>>>> +                                    MigrationEvent *e, Error **errp)
>>>> +{
>>>> +    if (e->state == MIGRATION_STATUS_SETUP &&
>>>> +        !runstate_check(RUN_STATE_SUSPENDED)) {
>>>> +
>>>> +        error_setg(errp,
>>>> +            "VFIO device only supports cpr-reboot for runstate suspended");
>>>> +
>>>> +        return -1;
>>>> +    }
>>>
>>> What happens if the guest is suspended during SETUP, but then quickly waked
>>> up before CPR migration completes?
>>
>> That is a management layer bug -- we told them the VM must be suspended.
>> However, I will reject a wakeup request if migration is active and mode is cpr.
> 
> Yes it needs to be enforced if it is required by cpr-reboot.  QEMU
> hopefully should never rely on mgmt app behavior.
> 
>>
>>>> +    return 0;
>>>> +}
>>>>  
>>>>  int vfio_cpr_register_container(VFIOContainer *container, Error **errp)
>>>>  {
>>>> +    migration_add_notifier_mode(&container->cpr_reboot_notifier,
>>>> +                                vfio_cpr_reboot_notifier,
>>>> +                                MIG_MODE_CPR_REBOOT);
>>>>      return 0;
>>>>  }
>>>>  
>>>>  void vfio_cpr_unregister_container(VFIOContainer *container)
>>>>  {
>>>> +    migration_remove_notifier(&container->cpr_reboot_notifier);
>>>>  }
>>>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>>>> index 534fddf..488905d 100644
>>>> --- a/hw/vfio/migration.c
>>>> +++ b/hw/vfio/migration.c
>>>> @@ -895,7 +895,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 1add5b7..7a46e24 100644
>>>> --- a/include/hw/vfio/vfio-common.h
>>>> +++ b/include/hw/vfio/vfio-common.h
>>>> @@ -78,6 +78,7 @@ struct VFIOGroup;
>>>>  typedef struct VFIOContainer {
>>>>      VFIOContainerBase bcontainer;
>>>>      int fd; /* /dev/vfio/vfio, empowered by the attached groups */
>>>> +    NotifierWithReturn cpr_reboot_notifier;
>>>>      unsigned iommu_type;
>>>>      QLIST_HEAD(, VFIOGroup) group_list;
>>>>  } VFIOContainer;
>>>> diff --git a/migration/ram.c b/migration/ram.c
>>>> index 1923366..44ad324 100644
>>>> --- a/migration/ram.c
>>>> +++ b/migration/ram.c
>>>> @@ -2392,8 +2392,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) {
>>>
>>> Same question here, on what happens if the user resumes the VM before
>>> migration completes?  IIUC shared-ram is not required, then it means if
>>> that happens the cpr migration image can contain corrupted data, and that
>>> may be a problem.
>>>
>>> Background snapshot is special in that it relies on totally different
>>> tracking facilities (userfault, rather than KVM_GET_DIRTY_LOG), so it
>>> disabled dirty tracking completely.  I assume not the case for cpr.
>>>
>>> If cpr is not going to support that use case, IIUC it should fail that
>>> system wakeup properly.  Or perhaps when CPR mode QEMU should instead
>>> reject a wakeup?
>>
>> Good catch, this hunk would break the non-vfio case where the guest can be
>> running when migration is initiated.  I should narrow the logic to check for
>> that:
>>
>>     if (!migrate_background_snapshot() &&
>>         (migrate_mode() == MIG_MODE_NORMAL || runstate_is_running()) {
>>         ... use dirty logging ...
>>
>> That plus rejecting a wakeup request should be sufficient.
> 
> Sounds good.
> 
> Please also consider adding "VM suspended for the whole cpr-reboot
> migration process" into the cpr-reboot mode qapi/ documentation as a
> hard requirement.  I don't see that mentioned at all, then people will be
> surprised to see a wakeup failure otherwise.

I will add something, but suspending is not always required:
  * If the VM has vfio, then the user must suspend it before cpr-reboot
  * If the VM does not have vfio, then the user does not need to suspend it.  
  * If the VM is suspended when cpr-reboot begins (regardless of whether it
    has vfio or not), then wakeup is blocked until cpr-reboot finishes

> Frankly, every time I re-read the cpr-reboot description I get confused on
> what is implied and enforced in this mode.  Can we provide an update?
> 
> # @cpr-reboot: The migrate command saves state to a file, allowing one to
> #              quit qemu, reboot to an updated kernel, and restart an updated
> #              version of qemu.  The caller must specify a migration URI
> #              that writes to and reads from a file.  Unlike normal mode,
> #              the use of certain local storage options does not block the
> #              migration, but the caller must not modify guest block devices
> #              between the quit and restart.  To avoid saving guest RAM to the
> #              file, the memory backend must be shared, and the @x-ignore-shared
> #              migration capability must be set.  Guest RAM must be non-volatile
> #              across reboot, such as by backing it with a dax device, but this
> #              is not enforced.  The restarted qemu arguments must match those
> #              used to initially start qemu, plus the -incoming option.
> #              (since 8.2)
> 
> For example, is shared-mem enfornced?  AFAIU it's not required for now, at
> least I don't remember any code to stop cpr-reboot migration if there's
> private memories.

shared-mem is not enforced or always required.
*If* you want "To avoid saving guest RAM to the file", then
"the memory backend must be shared".

If you are willing to incur the cost of saving RAM to the file, then shared-mem is 
not required.

> What about the image location restriction on non-volatile?  Is it required?
> IIUC "guest ram must be non-volatile across reboot" asks more than what we
> need to document for cpr-reboot.  For example, can someone use cpr-reboot
> to upgrade the userspace virt stack, but doesn't want to upgrade the
> kernel?  Then I assume dax is not required, since anyone can use a
> memory-based file (like shmem, hugetlbfs, or even guest_memfd in the future
> of security contexts)?

"Guest RAM must be non-volatile *across reboot*".
If you are not rebooting, it can be volatile.

> I think it's okay to keep such descriptions, but the document (so far only
> exists in qapi/migration.json) should clearly describe what must be done,

I believe I did so, perhaps too tersely, but I will expand it.

> separated from what is suggested.  The current version is still unclear,
> IMHO, and it'll be good to amend it when adding the hard requirement on
> suspended mode.  AFAIU the suspended mode helps:
> 
>   (1) vfio devices all stateless (vfio use case specific)
> 
>   (2) no dirty tracking required (vfio + non-vfio use case, e.g., merely no
>       restriction on what block type the VM uses)
> 
> Also, since we already have cpr-reboot mode merged, we need a solid
> document file describing this feature.  Maybe "what is suggested" should be
> put into that document instead, keeping the qapi document the essential
> documents.  Above explanations can also be good material to be mentioned
> there (if my understanding of why suspended is a hard requirement is
> correct..).
> 
> For that, feel free to pull the lastest qemu master branch, then you can
> create a file under docs/devel/migration/, possibly, cpr-reboot.rst, and
> link that into the features.rst file.  Currently we lack of docs for
> migration features, but we do have plan to enrich it to some degree soon:
> 
>   https://www.qemu.org/docs/master/devel/migration/features.html
> 
> So it would be good to have cpr-reboot since the 1st day the whole feature
> is there.  The doc can be posted separately if you prefer, so I can merge
> the doc series even sooner (one patch to update qapi/, one to add that file
> to docs/, perhaps).  You can also maintain missing items as TODO in the
> same .rst file, depending on how much work is still leftover, then remove
> entries alongside of your patches merged, which I mostly suggested the same
> to fixed-ram.

Sure, I will work on a doc.

- Steve

Re: [PATCH V2 11/11] vfio: allow cpr-reboot migration if suspended

[Peter Xu]

On Wed, Jan 17, 2024 at 04:30:48PM -0500, Steven Sistare wrote:
> On 1/17/2024 2:12 AM, Peter Xu wrote:
> > On Tue, Jan 16, 2024 at 03:37:33PM -0500, Steven Sistare wrote:
> >> On 1/15/2024 2:33 AM, Peter Xu wrote:
> >>> On Fri, Jan 12, 2024 at 07:05:10AM -0800, Steve Sistare wrote:
> >>>> 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>
> >>>> ---
> >>>>  hw/vfio/common.c              |  2 +-
> >>>>  hw/vfio/cpr.c                 | 20 ++++++++++++++++++++
> >>>>  hw/vfio/migration.c           |  2 +-
> >>>>  include/hw/vfio/vfio-common.h |  1 +
> >>>>  migration/ram.c               |  9 +++++----
> >>>>  5 files changed, 28 insertions(+), 6 deletions(-)
> >>>>
> >>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> >>>> index 0b3352f..09af934 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/cpr.c b/hw/vfio/cpr.c
> >>>> index bbd1c7a..9f4b1fe 100644
> >>>> --- a/hw/vfio/cpr.c
> >>>> +++ b/hw/vfio/cpr.c
> >>>> @@ -7,13 +7,33 @@
> >>>>  
> >>>>  #include "qemu/osdep.h"
> >>>>  #include "hw/vfio/vfio-common.h"
> >>>> +#include "migration/misc.h"
> >>>>  #include "qapi/error.h"
> >>>> +#include "sysemu/runstate.h"
> >>>> +
> >>>> +static int vfio_cpr_reboot_notifier(NotifierWithReturn *notifier,
> >>>> +                                    MigrationEvent *e, Error **errp)
> >>>> +{
> >>>> +    if (e->state == MIGRATION_STATUS_SETUP &&
> >>>> +        !runstate_check(RUN_STATE_SUSPENDED)) {
> >>>> +
> >>>> +        error_setg(errp,
> >>>> +            "VFIO device only supports cpr-reboot for runstate suspended");
> >>>> +
> >>>> +        return -1;
> >>>> +    }
> >>>
> >>> What happens if the guest is suspended during SETUP, but then quickly waked
> >>> up before CPR migration completes?
> >>
> >> That is a management layer bug -- we told them the VM must be suspended.
> >> However, I will reject a wakeup request if migration is active and mode is cpr.
> > 
> > Yes it needs to be enforced if it is required by cpr-reboot.  QEMU
> > hopefully should never rely on mgmt app behavior.
> > 
> >>
> >>>> +    return 0;
> >>>> +}
> >>>>  
> >>>>  int vfio_cpr_register_container(VFIOContainer *container, Error **errp)
> >>>>  {
> >>>> +    migration_add_notifier_mode(&container->cpr_reboot_notifier,
> >>>> +                                vfio_cpr_reboot_notifier,
> >>>> +                                MIG_MODE_CPR_REBOOT);
> >>>>      return 0;
> >>>>  }
> >>>>  
> >>>>  void vfio_cpr_unregister_container(VFIOContainer *container)
> >>>>  {
> >>>> +    migration_remove_notifier(&container->cpr_reboot_notifier);
> >>>>  }
> >>>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> >>>> index 534fddf..488905d 100644
> >>>> --- a/hw/vfio/migration.c
> >>>> +++ b/hw/vfio/migration.c
> >>>> @@ -895,7 +895,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 1add5b7..7a46e24 100644
> >>>> --- a/include/hw/vfio/vfio-common.h
> >>>> +++ b/include/hw/vfio/vfio-common.h
> >>>> @@ -78,6 +78,7 @@ struct VFIOGroup;
> >>>>  typedef struct VFIOContainer {
> >>>>      VFIOContainerBase bcontainer;
> >>>>      int fd; /* /dev/vfio/vfio, empowered by the attached groups */
> >>>> +    NotifierWithReturn cpr_reboot_notifier;
> >>>>      unsigned iommu_type;
> >>>>      QLIST_HEAD(, VFIOGroup) group_list;
> >>>>  } VFIOContainer;
> >>>> diff --git a/migration/ram.c b/migration/ram.c
> >>>> index 1923366..44ad324 100644
> >>>> --- a/migration/ram.c
> >>>> +++ b/migration/ram.c
> >>>> @@ -2392,8 +2392,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) {
> >>>
> >>> Same question here, on what happens if the user resumes the VM before
> >>> migration completes?  IIUC shared-ram is not required, then it means if
> >>> that happens the cpr migration image can contain corrupted data, and that
> >>> may be a problem.
> >>>
> >>> Background snapshot is special in that it relies on totally different
> >>> tracking facilities (userfault, rather than KVM_GET_DIRTY_LOG), so it
> >>> disabled dirty tracking completely.  I assume not the case for cpr.
> >>>
> >>> If cpr is not going to support that use case, IIUC it should fail that
> >>> system wakeup properly.  Or perhaps when CPR mode QEMU should instead
> >>> reject a wakeup?
> >>
> >> Good catch, this hunk would break the non-vfio case where the guest can be
> >> running when migration is initiated.  I should narrow the logic to check for
> >> that:
> >>
> >>     if (!migrate_background_snapshot() &&
> >>         (migrate_mode() == MIG_MODE_NORMAL || runstate_is_running()) {
> >>         ... use dirty logging ...
> >>
> >> That plus rejecting a wakeup request should be sufficient.
> > 
> > Sounds good.
> > 
> > Please also consider adding "VM suspended for the whole cpr-reboot
> > migration process" into the cpr-reboot mode qapi/ documentation as a
> > hard requirement.  I don't see that mentioned at all, then people will be
> > surprised to see a wakeup failure otherwise.
> 
> I will add something, but suspending is not always required:
>   * If the VM has vfio, then the user must suspend it before cpr-reboot
>   * If the VM does not have vfio, then the user does not need to suspend it.  

But you plan to unconditionally disable dirty tracking for cpr-reboot, am I
right?  If that's unconditional, we need to make it enforced in both codes
and document.

Dirty tracking is still essential if the VM is not suspended, or can be
waked up during migration, and when shared-mem-only is not enforced.

If you see this is why I had such feeling that the interfacing for
cpr-reboot is not clear (even if the major use case is), even to us
developers and reviewers, and then clear documentations will help.  That's
an issue at least to me.  We seem to have mixed up a few things.

For example, if we can have two hints (or capability/etc., just an idea to
show how we could have split those):

  - MIG_HINT_SUSPENDED: when set this flag, suspension is required during a
    live migration.  This will be required for VFIO live upgrades.  If this
    is set, start migration will check "suspended" state and fail
    otherwise.  Meanwhile if this is set system-wakeup is forbidden during
    migration.

    PS: this is pretty tricky because AFAICT we're relying on guest suspend
    behavior to make sure VFIO is stateless.. can a guest OS not obey that?
    Can it go into suspension with VFIO state kept?  I don't know.  But
    this is another story.  It just smells tricky because QEMU migration
    now relies on that guest behavior to be correct.

  - MIG_HINT_RAM_SHARED_PERSIST: when set this flag, writable memory must
    be shared and persisted even after QEMU quits.  We need this hint
    because that's a sign-off from the admin that we can ignore RAM during
    the migration.  This can already imply e.g. x-ignore-shared.

  - MIG_HINT_MIGRATE_TO_FILE: when this flag set, it guarantees VM migrates
    to the file and it'll be stopped later.  All the cpr-reboot blocker
    changes should only be relevant to this hint, e.g. VM that contains
    "qcow" block drive should be allowed for migration if this flag is set,
    even if it may not support a generic live migration for other reasons
    (which I actually am not clear..).  Also, when this flag is set, QEMU
    can stop the VM before migration starts, because we know tracking dirty
    is meaningless and the VM is destined to be stored in the file later.

Then it's clearer to know whether dirty tracking is needed:

  if (MIG_HINT_SUSPENDED || MIG_HINT_RAM_SHARED_PERSIST ||
      MIG_HINT_MIGRATE_TO_FILE || background_snspashot())
    // dirty tracking not needed
  else
    // dirty tracking needed

Also, if with above it can also help fixed-ram migration.  For example,
MIG_HINT_MIGRATE_TO_FILE is exactly what we used to discuss on the
"auto-pause", which is mentioned in the fixed-ram series:

https://lore.kernel.org/all/20231023203608.26370-1-farosas@suse.de/

Note that I'm not saying cpr-reboot should do like this, because I haven't
yet 100% think all through with the idea.  But I want to show the idea that
how such complicated feature can be split into multiple, with pretty clear
interface and even reuse-able outside this project (e.g. in fixed-ram
migration; currently fixed-ram migration always enable dirty-tracking
because it lacks that hint even if the admin knows it will destroy the VM
after migration).

>   * If the VM is suspended when cpr-reboot begins (regardless of whether it
>     has vfio or not), then wakeup is blocked until cpr-reboot finishes
> 
> > Frankly, every time I re-read the cpr-reboot description I get confused on
> > what is implied and enforced in this mode.  Can we provide an update?
> > 
> > # @cpr-reboot: The migrate command saves state to a file, allowing one to
> > #              quit qemu, reboot to an updated kernel, and restart an updated
> > #              version of qemu.  The caller must specify a migration URI
> > #              that writes to and reads from a file.  Unlike normal mode,
> > #              the use of certain local storage options does not block the
> > #              migration, but the caller must not modify guest block devices
> > #              between the quit and restart.  To avoid saving guest RAM to the
> > #              file, the memory backend must be shared, and the @x-ignore-shared
> > #              migration capability must be set.  Guest RAM must be non-volatile
> > #              across reboot, such as by backing it with a dax device, but this
> > #              is not enforced.  The restarted qemu arguments must match those
> > #              used to initially start qemu, plus the -incoming option.
> > #              (since 8.2)
> > 
> > For example, is shared-mem enfornced?  AFAIU it's not required for now, at
> > least I don't remember any code to stop cpr-reboot migration if there's
> > private memories.
> 
> shared-mem is not enforced or always required.
> *If* you want "To avoid saving guest RAM to the file", then
> "the memory backend must be shared".
> 
> If you are willing to incur the cost of saving RAM to the file, then shared-mem is 
> not required.

This can be one entry of the "Suggestions on cpr-reboot mode".

> 
> > What about the image location restriction on non-volatile?  Is it required?
> > IIUC "guest ram must be non-volatile across reboot" asks more than what we
> > need to document for cpr-reboot.  For example, can someone use cpr-reboot
> > to upgrade the userspace virt stack, but doesn't want to upgrade the
> > kernel?  Then I assume dax is not required, since anyone can use a
> > memory-based file (like shmem, hugetlbfs, or even guest_memfd in the future
> > of security contexts)?
> 
> "Guest RAM must be non-volatile *across reboot*".
> If you are not rebooting, it can be volatile.

Yet another entry of "Suggestions".

> 
> > I think it's okay to keep such descriptions, but the document (so far only
> > exists in qapi/migration.json) should clearly describe what must be done,
> 
> I believe I did so, perhaps too tersely, but I will expand it.

Maybe the current issue is it contains too much information, while many of
them can confuse the reader.

IMHO it'll be good to provide a summary paragraph on explaining what
cpr-reboot mode is, after reading this people should know whether they
should enable it.  Then for a richer document we can reference the docs/
link in qapi document if we want, or at least put details into separate
paragraphs.

> 
> > separated from what is suggested.  The current version is still unclear,
> > IMHO, and it'll be good to amend it when adding the hard requirement on
> > suspended mode.  AFAIU the suspended mode helps:
> > 
> >   (1) vfio devices all stateless (vfio use case specific)
> > 
> >   (2) no dirty tracking required (vfio + non-vfio use case, e.g., merely no
> >       restriction on what block type the VM uses)
> > 
> > Also, since we already have cpr-reboot mode merged, we need a solid
> > document file describing this feature.  Maybe "what is suggested" should be
> > put into that document instead, keeping the qapi document the essential
> > documents.  Above explanations can also be good material to be mentioned
> > there (if my understanding of why suspended is a hard requirement is
> > correct..).
> > 
> > For that, feel free to pull the lastest qemu master branch, then you can
> > create a file under docs/devel/migration/, possibly, cpr-reboot.rst, and
> > link that into the features.rst file.  Currently we lack of docs for
> > migration features, but we do have plan to enrich it to some degree soon:
> > 
> >   https://www.qemu.org/docs/master/devel/migration/features.html
> > 
> > So it would be good to have cpr-reboot since the 1st day the whole feature
> > is there.  The doc can be posted separately if you prefer, so I can merge
> > the doc series even sooner (one patch to update qapi/, one to add that file
> > to docs/, perhaps).  You can also maintain missing items as TODO in the
> > same .rst file, depending on how much work is still leftover, then remove
> > entries alongside of your patches merged, which I mostly suggested the same
> > to fixed-ram.
> 
> Sure, I will work on a doc.

Thanks a lot.

-- 
Peter Xu

Re: [PATCH V2 00/11] allow cpr-reboot for vfio

[Alex Williamson]

On Fri, 12 Jan 2024 07:04:59 -0800
Steve Sistare <steven.sistare@oracle.com> wrote:

> 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.
> 
> Most of the patches in this series enhance migration notifiers so they can
> return an error status and message.  The last two patches register a notifier
> for vfio that returns an error if the guest is not suspended.

Hi Steve,

This appears to only support legacy container mode and not cdev/iommufd
mode.  Shouldn't this support both?  Thanks,

Alex

> Steve Sistare (11):
>   notify: pass error to notifier with return
>   migration: remove error from notifier data
>   migration: convert to NotifierWithReturn
>   migration: remove migration_in_postcopy parameter
>   migration: MigrationEvent for notifiers
>   migration: MigrationNotifyFunc
>   migration: per-mode notifiers
>   migration: refactor migrate_fd_connect failures
>   migration: notifier error checking
>   vfio: register container for cpr
>   vfio: allow cpr-reboot migration if suspended
> 
>  hw/net/virtio-net.c            |  14 ++---
>  hw/vfio/common.c               |   2 +-
>  hw/vfio/container.c            |  11 +++-
>  hw/vfio/cpr.c                  |  39 ++++++++++++++
>  hw/vfio/meson.build            |   1 +
>  hw/vfio/migration.c            |  13 +++--
>  hw/virtio/vhost-user.c         |  10 ++--
>  hw/virtio/virtio-balloon.c     |   3 +-
>  include/hw/vfio/vfio-common.h  |   6 ++-
>  include/hw/virtio/virtio-net.h |   2 +-
>  include/migration/misc.h       |  21 +++++---
>  include/qemu/notify.h          |   7 ++-
>  migration/migration.c          | 117 +++++++++++++++++++++++++++--------------
>  migration/postcopy-ram.c       |   3 +-
>  migration/postcopy-ram.h       |   1 -
>  migration/ram.c                |  12 ++---
>  net/vhost-vdpa.c               |  15 +++---
>  ui/spice-core.c                |  19 +++----
>  util/notify.c                  |   5 +-
>  19 files changed, 206 insertions(+), 95 deletions(-)
>  create mode 100644 hw/vfio/cpr.c
> 

Re: [PATCH V2 00/11] allow cpr-reboot for vfio

[Steven Sistare]

On 1/12/2024 4:38 PM, Alex Williamson wrote:
> On Fri, 12 Jan 2024 07:04:59 -0800
> Steve Sistare <steven.sistare@oracle.com> wrote:
> 
>> 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.
>>
>> Most of the patches in this series enhance migration notifiers so they can
>> return an error status and message.  The last two patches register a notifier
>> for vfio that returns an error if the guest is not suspended.
> 
> Hi Steve,
> 
> This appears to only support legacy container mode and not cdev/iommufd
> mode.  Shouldn't this support both?  Thanks,

My bad, thanks! I'll move cpr_reboot_notifier to the base container, and also call
cpr register/unregister from iommufd_cdev_attach/iommufd_cdev_detach.

- Steve

>> Steve Sistare (11):
>>   notify: pass error to notifier with return
>>   migration: remove error from notifier data
>>   migration: convert to NotifierWithReturn
>>   migration: remove migration_in_postcopy parameter
>>   migration: MigrationEvent for notifiers
>>   migration: MigrationNotifyFunc
>>   migration: per-mode notifiers
>>   migration: refactor migrate_fd_connect failures
>>   migration: notifier error checking
>>   vfio: register container for cpr
>>   vfio: allow cpr-reboot migration if suspended
>>
>>  hw/net/virtio-net.c            |  14 ++---
>>  hw/vfio/common.c               |   2 +-
>>  hw/vfio/container.c            |  11 +++-
>>  hw/vfio/cpr.c                  |  39 ++++++++++++++
>>  hw/vfio/meson.build            |   1 +
>>  hw/vfio/migration.c            |  13 +++--
>>  hw/virtio/vhost-user.c         |  10 ++--
>>  hw/virtio/virtio-balloon.c     |   3 +-
>>  include/hw/vfio/vfio-common.h  |   6 ++-
>>  include/hw/virtio/virtio-net.h |   2 +-
>>  include/migration/misc.h       |  21 +++++---
>>  include/qemu/notify.h          |   7 ++-
>>  migration/migration.c          | 117 +++++++++++++++++++++++++++--------------
>>  migration/postcopy-ram.c       |   3 +-
>>  migration/postcopy-ram.h       |   1 -
>>  migration/ram.c                |  12 ++---
>>  net/vhost-vdpa.c               |  15 +++---
>>  ui/spice-core.c                |  19 +++----
>>  util/notify.c                  |   5 +-
>>  19 files changed, 206 insertions(+), 95 deletions(-)
>>  create mode 100644 hw/vfio/cpr.c
>>
> 

Re: [PATCH V2 00/11] allow cpr-reboot for vfio

[David Hildenbrand]

On 12.01.24 16:04, Steve Sistare wrote:
> 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.

Can you briefly explain what cpr-reboot is, or do you have a pointer to 
some more details?

What is is good for, why would you use it, how does it work?

I *suspect* that you want to live-migrate a VM with vfio devices 
attached, whereby you don't want to migrate device state.

-- 
Cheers,

David / dhildenb

Re: [PATCH V2 00/11] allow cpr-reboot for vfio

[David Hildenbrand]

On 15.01.24 11:48, David Hildenbrand wrote:
> On 12.01.24 16:04, Steve Sistare wrote:
>> 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.
> 
> Can you briefly explain what cpr-reboot is, or do you have a pointer to
> some more details?
> 
> What is is good for, why would you use it, how does it work?
> 
> I *suspect* that you want to live-migrate a VM with vfio devices
> attached, whereby you don't want to migrate device state.
> 

Ah, found it:

+# @cpr-reboot: The migrate command saves state to a file, allowing one to
+#              quit qemu, reboot to an updated kernel, and restart an updated
+#              version of qemu.  The caller must specify a migration URI
+#              that writes to and reads from a file.  Unlike normal mode,
+#              the use of certain local storage options does not block the
+#              migration, but the caller must not modify guest block devices
+#              between the quit and restart.  To avoid saving guest RAM to the
+#              file, the memory backend must be shared, and the @x-ignore-shared
+#              migration capability must be set.  Guest RAM must be non-volatile
+#              across reboot, such as by backing it with a dax device, but this
+#              is not enforced.  The restarted qemu arguments must match those
+#              used to initially start qemu, plus the -incoming option.
+#              (since 8.2)

I'll note that the use of "reboot" is extremely confusing in this context.
You *might* want to reboot the hypervisor, but that's actually completely
independent from the QEMU implementation.

-- 
Cheers,

David / dhildenb