[PATCH hci-dev v3 0/2] vfio/migration: don't touch the device on guest shutdown #VSTOR-153976

[PATCH hci-dev v3 0/2] vfio/migration: don't touch the device on guest shutdown #VSTOR-153976

[Andrey Drobyshev]

v2 -> v3:

 - new patch 1: stop the device when QEMU quits or migrates a VM that
   a guest shutdown left stopped. A VF closed while running, after the
   Linux guest driver unloaded, breaks the next VM that resumes on it;
 - the stop at the start of a save moved to patch 1 and now covers only
   the SHUTDOWN run state, so a suspended guest is left alone (Sashiko);
 - the guest shutdown check is one helper, used by both VM state
   handlers.

v2: http://lore.virtuozzo.com/qemu/20261108205050.37277-1-andrey.drobyshev@virtuozzo.com/

Andrey Drobyshev (2):
  vfio/migration: stop a device left running after a guest shutdown
    #VSTOR-153976
  vfio/migration: don't touch the device on guest shutdown #VSTOR-153976

 hw/vfio/migration.c               | 67 +++++++++++++++++++++++++++++++
 hw/vfio/vfio-migration-internal.h |  1 +
 include/system/runstate.h         |  1 +
 system/runstate.c                 |  5 +++
 4 files changed, 74 insertions(+)


base-commit: 5d129c4dd43d131fbcdd411ad30be8a9a641895e
--
2.47.1

[PATCH hci-dev v3 1/2]vfio/migration: stop a device left running after a guest shutdown #VSTOR-153976

[Andrey Drobyshev]

With -no-shutdown, a guest shutdown leaves the VM stopped. From there
QEMU can only reset the VM, quit, or migrate or save it. A quit or a
signal brings no VM state change, and neither does the switchover of a
migration from a stopped VM. So a device that is still running at that
point is closed or saved running.

Stop it first: in a shutdown notifier when QEMU quits, and at the start
of a save. Both are no-ops today, since the guest shutdown already stops
the device. The next patch leaves it running there.

This matters for NVIDIA vGPU (595.71.03): a VF closed while running,
after the Linux guest driver unloaded, breaks the next VM that resumes
on it. Its GPU stops answering.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 hw/vfio/migration.c               | 44 +++++++++++++++++++++++++++++++
 hw/vfio/vfio-migration-internal.h |  1 +
 include/system/runstate.h         |  1 +
 system/runstate.c                 |  5 ++++
 4 files changed, 51 insertions(+)

diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 7e7fb496bbe..bfafc14e5b9 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -415,6 +415,25 @@ static bool vfio_precopy_supported(VFIODevice *vbasedev)
     return migration->mig_flags & VFIO_MIGRATION_PRE_COPY;
 }
 
+/*
+ * Stop a device still running while the VM is stopped after a guest
+ * shutdown. Nothing else leaves a running device in the SHUTDOWN run state,
+ * so this is a no-op for every other VM.
+ */
+static int vfio_migration_stop_after_shutdown(VFIODevice *vbasedev,
+                                              Error **errp)
+{
+    VFIOMigration *migration = vbasedev->migration;
+
+    if (!runstate_check(RUN_STATE_SHUTDOWN) ||
+        migration->device_state != VFIO_DEVICE_STATE_RUNNING) {
+        return 0;
+    }
+
+    return vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP,
+                                    VFIO_DEVICE_STATE_RUNNING, errp);
+}
+
 /* ---------------------------------------------------------------------- */
 
 static int vfio_save_prepare(void *opaque, Error **errp)
@@ -473,6 +492,12 @@ static int vfio_save_setup(QEMUFile *f, void *opaque, Error **errp)
     migration->event_save_iterate_started = false;
     migration->event_precopy_empty_hit = false;
 
+    /* A stopped VM gets no VM state change at switchover */
+    ret = vfio_migration_stop_after_shutdown(vbasedev, errp);
+    if (ret) {
+        return ret;
+    }
+
     if (vfio_precopy_supported(vbasedev)) {
         switch (migration->device_state) {
         case VFIO_DEVICE_STATE_RUNNING:
@@ -959,6 +984,22 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
                               mig_state_to_str(new_state));
 }
 
+static void vfio_migration_shutdown_notifier(Notifier *notifier, void *data)
+{
+    VFIOMigration *migration = container_of(notifier, VFIOMigration,
+                                            shutdown);
+    Error *local_err = NULL;
+
+    /* QEMU is quitting: a device left running must be stopped first */
+    if (qemu_shutdown_action_is_pause()) {
+        return;
+    }
+
+    if (vfio_migration_stop_after_shutdown(migration->vbasedev, &local_err)) {
+        error_report_err(local_err);
+    }
+}
+
 static int vfio_migration_state_notifier(NotifierWithReturn *notifier,
                                          MigrationEvent *e, Error **errp)
 {
@@ -1083,6 +1124,8 @@ static int vfio_migration_init(VFIODevice *vbasedev)
         vbasedev->dev, vfio_vmstate_change, prepare_cb, NULL, vbasedev);
     migration_add_notifier(&migration->migration_state,
                            vfio_migration_state_notifier);
+    migration->shutdown.notify = vfio_migration_shutdown_notifier;
+    qemu_register_shutdown_notifier(&migration->shutdown);
 
     return 0;
 }
@@ -1150,6 +1193,7 @@ static void vfio_migration_deinit(VFIODevice *vbasedev)
 {
     VFIOMigration *migration = vbasedev->migration;
 
+    notifier_remove(&migration->shutdown);
     migration_remove_notifier(&migration->migration_state);
     qemu_del_vm_change_state_handler(migration->vm_state);
     unregister_savevm(VMSTATE_IF(vbasedev->dev), "vfio", vbasedev);
diff --git a/hw/vfio/vfio-migration-internal.h b/hw/vfio/vfio-migration-internal.h
index 814fbd9ebae..e81d854d3c4 100644
--- a/hw/vfio/vfio-migration-internal.h
+++ b/hw/vfio/vfio-migration-internal.h
@@ -40,6 +40,7 @@ typedef struct VFIOMigration {
     struct VFIODevice *vbasedev;
     VMChangeStateEntry *vm_state;
     NotifierWithReturn migration_state;
+    Notifier shutdown;
     uint32_t device_state;
     int data_fd;
     void *data_buffer;
diff --git a/include/system/runstate.h b/include/system/runstate.h
index 929379adae4..29a919348da 100644
--- a/include/system/runstate.h
+++ b/include/system/runstate.h
@@ -148,6 +148,7 @@ void qemu_system_vmstop_request_prepare(void);
 bool qemu_vmstop_requested(RunState *r);
 ShutdownCause qemu_shutdown_requested_get(void);
 bool qemu_force_shutdown_requested(void);
+bool qemu_shutdown_action_is_pause(void);
 ShutdownCause qemu_reset_requested_get(void);
 void qemu_system_killed(int signal, pid_t pid);
 void qemu_system_reset(ShutdownCause reason);
diff --git a/system/runstate.c b/system/runstate.c
index 6178b0091a3..1cdecde0fcf 100644
--- a/system/runstate.c
+++ b/system/runstate.c
@@ -433,6 +433,11 @@ bool qemu_force_shutdown_requested(void)
     return force_shutdown;
 }
 
+bool qemu_shutdown_action_is_pause(void)
+{
+    return shutdown_action == SHUTDOWN_ACTION_PAUSE;
+}
+
 ShutdownCause qemu_reset_requested_get(void)
 {
     return reset_requested;
-- 
2.47.1

Re: [PATCH hci-dev v3 1/2]vfio/migration: stop a device left running after a guest shutdown #VSTOR-153976

[no-reply]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] vfio: comment claims nothing else leaves a running device in SHUTDOWN
- [Low] vfio/migration: RUNNING to STOP skips the P2P quiesce step for P2P-capable devices
--

commit b98ac2b60638ee9b579c31103b9e4ae12b82d728
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

vfio/migration: stop a device left running after a guest shutdown

This commit adds vfio_migration_stop_after_shutdown(). It moves a
still-RUNNING VFIO device to STOP when the VM is in RUN_STATE_SHUTDOWN.
It is called from a new shutdown notifier when QEMU quits, and from
vfio_save_setup(). The commit message says both calls are no-ops today.

> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index 7e7fb496bbecb..bfafc14e5b942 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c

[ ... ]

> @@ -415,6 +415,25 @@ static bool vfio_precopy_supported(VFIODevice *vbasedev)
>      return migration->mig_flags & VFIO_MIGRATION_PRE_COPY;
>  }
>  
> +/*
> + * Stop a device still running while the VM is stopped after a guest
> + * shutdown. Nothing else leaves a running device in the SHUTDOWN run state,
> + * so this is a no-op for every other VM.
> + */

[Severity: Low]
Is the statement "Nothing else leaves a running device in the SHUTDOWN run
state" accurate?

vfio_migration_state_notifier() restores the device to RUNNING on
MIG_EVENT_PRECOPY_FAILED, without looking at the run state:

    if (e->type == MIG_EVENT_PRECOPY_FAILED) {
        ret = vfio_migration_set_state_or_reset(vbasedev,
                                                VFIO_DEVICE_STATE_RUNNING,
                                                &local_err);

Consider a -no-shutdown VM sitting in RUN_STATE_SHUTDOWN:

    vfio_save_setup()
      vfio_migration_stop_after_shutdown()   /* device -> STOP */
    migration fails or is cancelled
      vfio_migration_state_notifier(MIG_EVENT_PRECOPY_FAILED)
        vfio_migration_set_state_or_reset(..., VFIO_DEVICE_STATE_RUNNING)

The VM stays in SHUTDOWN and no VM state change is sent, so the device ends
up RUNNING in a stopped VM. This is a second way to get that state, besides
the guest shutdown itself.

The quit notifier and the next vfio_save_setup() stop the device again, so
nothing breaks functionally. Could the comment, and the commit message
wording that both paths are no-ops, be made more precise?

The same applies to a migration destination that ends up in SHUTDOWN, where
the device keeps its init-time RUNNING state.

> +static int vfio_migration_stop_after_shutdown(VFIODevice *vbasedev,
> +                                              Error **errp)
> +{
> +    VFIOMigration *migration = vbasedev->migration;
> +
> +    if (!runstate_check(RUN_STATE_SHUTDOWN) ||
> +        migration->device_state != VFIO_DEVICE_STATE_RUNNING) {
> +        return 0;
> +    }
> +
> +    return vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP,
> +                                    VFIO_DEVICE_STATE_RUNNING, errp);
> +}

[Severity: Low]
Can this skip the P2P quiesce step for P2P-capable devices?

This moves a single device straight from RUNNING to STOP. Other VM-stop
paths go through vfio_vmstate_change_prepare(), installed in
vfio_migration_init() when VFIO_MIGRATION_P2P is set:

    prepare_cb = migration->mig_flags & VFIO_MIGRATION_P2P ?
                     vfio_vmstate_change_prepare :
                     NULL;

That function first moves all devices to RUNNING_P2P or PRE_COPY_P2P. Only
then does vfio_vmstate_change() move each device to STOP.

With the next patch, a guest shutdown no longer runs
vfio_vmstate_change_prepare() or vfio_vmstate_change(), because they return
early via vfio_vm_stopped_by_guest_shutdown(). The devices stay RUNNING.

The only remaining stops are this helper, called from the quit notifier and
from vfio_save_setup(). Each stops one device at a time, while other VFIO
devices may still be RUNNING and able to DMA to it.

This is speculative. It needs several P2P-capable devices and a guest that
left a device doing peer DMA after shutdown. Is the bypass intended to be
safe in that case? The commit message and comments don't mention P2P.

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261109154346.153153-1-andrey.drobyshev@virtuozzo.com?part=1

[PATCH hci-dev v3 2/2]vfio/migration: don't touch the device on guest shutdown #VSTOR-153976

[Andrey Drobyshev]

When the guest shuts down and QEMU runs with -no-shutdown, the VM stays
stopped and can only be reset or quit. A libvirt soft reboot is exactly
that: guest shutdown, system_reset, cont. Today vfio_vmstate_change()
treats this stop like any other and moves the device to STOP, then cont
brings it back to RUNNING.

NVIDIA's vGPU manager (595.71.03) takes that STOP for the start of a
migration, finds no guest driver, breaks its dirty page tracking and
never repairs it when the driver loads again. Every live migration of
the VM after a soft reboot then fails: "Error saving page in pipelined
mode".

Do not touch the device state in this case. There is nothing to
quiesce, and the reset that must follow resets the device anyway. The
case is told apart by the shutdown action: only "pause" (-no-shutdown)
keeps the VM alive. A quit of a running VM, a signal and a plain guest
shutdown use "poweroff" and still stop the device as before. A quit or
a migration of the stopped VM stops the device first, see the previous
patch.

With this patch applied, soft reboot + subsequent live migration pass
successfully.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 hw/vfio/migration.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index bfafc14e5b9..454c4e3f553 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -415,6 +415,21 @@ static bool vfio_precopy_supported(VFIODevice *vbasedev)
     return migration->mig_flags & VFIO_MIGRATION_PRE_COPY;
 }
 
+/*
+ * The guest has shut down and QEMU keeps the VM stopped (-no-shutdown). The
+ * guest has released the device, and only a reset, a quit or a migration can
+ * follow. The device is left running here: the reset resets it, and the two
+ * other paths stop it with vfio_migration_stop_after_shutdown().
+ *
+ * The same run state with the "poweroff" action is a quit of a running VM,
+ * which stops the device as usual.
+ */
+static bool vfio_vm_stopped_by_guest_shutdown(bool running, RunState state)
+{
+    return !running && state == RUN_STATE_SHUTDOWN &&
+           qemu_shutdown_action_is_pause();
+}
+
 /*
  * Stop a device still running while the VM is stopped after a guest
  * shutdown. Nothing else leaves a running device in the SHUTDOWN run state,
@@ -924,6 +939,10 @@ static void vfio_vmstate_change_prepare(void *opaque, bool running,
         return;
     }
 
+    if (vfio_vm_stopped_by_guest_shutdown(running, state)) {
+        return;
+    }
+
     new_state = migration->device_state == VFIO_DEVICE_STATE_PRE_COPY ?
                     VFIO_DEVICE_STATE_PRE_COPY_P2P :
                     VFIO_DEVICE_STATE_RUNNING_P2P;
@@ -961,6 +980,10 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
         return;
     }
 
+    if (vfio_vm_stopped_by_guest_shutdown(running, state)) {
+        return;
+    }
+
     if (running) {
         new_state = VFIO_DEVICE_STATE_RUNNING;
     } else {
-- 
2.47.1

Re: [PATCH hci-dev v3 2/2]vfio/migration: don't touch the device on guest shutdown #VSTOR-153976

[no-reply]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] vfio/migration: shutdown skip leaves PRE_COPY device unstopped on pause
- [Medium] vfio/migration: skip-stop predicate matches every pause-action SHUTDOWN, not only guest shutdowns
--

commit 3d78bf00afa301cabb7efe37569c2de747a04bdc
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

vfio/migration: don't touch the device on guest shutdown

This patch makes vfio_vmstate_change_prepare() and vfio_vmstate_change()
leave the VFIO device untouched when the VM stops in RUN_STATE_SHUTDOWN
and the shutdown action is "pause" (-no-shutdown). It avoids moving the
device to STOP on a libvirt soft reboot, which confuses the NVIDIA vGPU
manager. A quit or a migration of the stopped VM is expected to stop the
device through vfio_migration_stop_after_shutdown().

> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index bfafc14e5b94..454c4e3f553a 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c

[ ... ]

> +/*
> + * The guest has shut down and QEMU keeps the VM stopped (-no-shutdown). The
> + * guest has released the device, and only a reset, a quit or a migration can
> + * follow. The device is left running here: the reset resets it, and the two
> + * other paths stop it with vfio_migration_stop_after_shutdown().
> + *
> + * The same run state with the "poweroff" action is a quit of a running VM,
> + * which stops the device as usual.
> + */
> +static bool vfio_vm_stopped_by_guest_shutdown(bool running, RunState state)
> +{
> +    return !running && state == RUN_STATE_SHUTDOWN &&
> +           qemu_shutdown_action_is_pause();
> +}

[Severity: Medium]
Does this predicate cover only guest-initiated shutdowns?

It tests only !running, RUN_STATE_SHUTDOWN and
qemu_shutdown_action_is_pause().  It does not look at the ShutdownCause.

In main_loop_should_exit() the pause action leads to a single call:

    if (shutdown_action == SHUTDOWN_ACTION_PAUSE) {
        vm_stop(RUN_STATE_SHUTDOWN);

This is reached for every shutdown cause.  That includes
SHUTDOWN_CAUSE_HOST_UI, SHUTDOWN_CAUSE_HOST_ERROR and
SHUTDOWN_CAUSE_GUEST_PANIC requested directly by hw/misc/virt_ctrl.c.  In
those cases the guest driver may not have run its shutdown sequence.

The device is then left RUNNING, not P2P-quiesced and not STOP, while the
vCPUs are stopped.  The claims "The guest has released the device" and
"There is nothing to quiesce" are not established for these causes.

The predicate also reads the global shutdown_action, which qmp_quit() and
qmp_set_action() change at runtime:

    shutdown_action = SHUTDOWN_ACTION_POWEROFF;
    qemu_system_shutdown_request(SHUTDOWN_CAUSE_HOST_QMP_QUIT);

Could the stop-time decision and a later decision then disagree?  Would
shutdown_caused_by_guest(cause), already used in qemu_system_shutdown(), or
a flag recorded at stop time be a more precise discriminator?

[ ... ]

> @@ -924,6 +939,10 @@ static void vfio_vmstate_change_prepare(void *opaque, bool running,
>          return;
>      }
>  
> +    if (vfio_vm_stopped_by_guest_shutdown(running, state)) {
> +        return;
> +    }
> +

[Severity: Medium]
Can this early return leave a device that is mid-precopy running?

The predicate does not look at migration->device_state.  Take a precopy
migration that is active, with vfio_save_setup() having put the device in
PRE_COPY, and then a guest shutdown with -no-shutdown.  Before this patch
the device went:

    PRE_COPY -> PRE_COPY_P2P -> STOP

Now both vfio_vmstate_change_prepare() and vfio_vmstate_change() return
early, and the device stays in PRE_COPY while the VM is stopped.

At switchover, migration_stop_vm() calls
vm_stop_force_state(RUN_STATE_FINISH_MIGRATE).  The runstate is already
stopped, so that only calls runstate_set() and sends no vm_state_notify.
Then vfio_save_complete_precopy() requests STOP_COPY with a recover state
of STOP:

    /* We reach here with device state STOP or STOP_COPY only */
    ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP_COPY,
                                   VFIO_DEVICE_STATE_STOP, &local_err);

Does that comment still hold, given that the device can arrive in PRE_COPY?
Is STOP the right recover state in that case?

On quit, vfio_migration_shutdown_notifier() calls
vfio_migration_stop_after_shutdown(), which has this check:

    if (!runstate_check(RUN_STATE_SHUTDOWN) ||
        migration->device_state != VFIO_DEVICE_STATE_RUNNING) {
        return 0;
    }

That returns 0 for PRE_COPY, PRE_COPY_P2P and RUNNING_P2P.  The device is
then closed in PRE_COPY, or moved to RUNNING by the
MIG_EVENT_PRECOPY_FAILED handler.  vfio_save_setup() already ran before the
shutdown, so its call to vfio_migration_stop_after_shutdown() does not help.

The comment on vfio_vm_stopped_by_guest_shutdown() says "the two other
paths stop it with vfio_migration_stop_after_shutdown()", and the helper's
own comment says "Nothing else leaves a running device in the SHUTDOWN run
state".  Do those statements hold for a migration that was already in
precopy?

Would it be better to skip only when device_state is
VFIO_DEVICE_STATE_RUNNING, or to extend vfio_migration_stop_after_shutdown()
to cover PRE_COPY, PRE_COPY_P2P and RUNNING_P2P?

[ ... ]

> @@ -961,6 +980,10 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
>          return;
>      }
>  
> +    if (vfio_vm_stopped_by_guest_shutdown(running, state)) {
> +        return;
> +    }
> +

[Severity: Medium]
The same early return in vfio_vmstate_change() skips the STOP transition
for a device in PRE_COPY, as described above, and for the non-guest
shutdown causes listed in the first comment.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261109154346.153153-1-andrey.drobyshev@virtuozzo.com?part=2