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

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

[Andrey Drobyshev]

v4 -> v5, after Sashiko's review:

 - patch 1: a device in RUNNING_P2P is stopped in any run state, not
   only in SHUTDOWN. A reset of the stopped VM leaves the device there
   and the VM in PRELAUNCH, and a migration started from there failed;
 - patch 2: the message says that a device without P2P is left running
   at the guest shutdown, not only P2P-quiesced.

v4: http://lore.virtuozzo.com/qemu/20261109165435.175715-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 stop 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 v5 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. The P2P round of the VM stop has already run, so the devices
are stopped one by one. A reset of the stopped VM leaves the device in
RUNNING_P2P. Nothing else does, so a device in that state is stopped in
any run state. Today only a failed migration of such a VM leaves a
device running there. The next patch does it at every guest shutdown.

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               | 46 +++++++++++++++++++++++++++++++
 hw/vfio/vfio-migration-internal.h |  1 +
 include/system/runstate.h         |  1 +
 system/runstate.c                 |  5 ++++
 4 files changed, 53 insertions(+)

diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 7e7fb496bbe..c391422d601 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -415,6 +415,27 @@ static bool vfio_precopy_supported(VFIODevice *vbasedev)
     return migration->mig_flags & VFIO_MIGRATION_PRE_COPY;
 }
 
+/*
+ * Stop a device that a guest shutdown left running, before it is closed or
+ * saved. Only that leaves a device in RUNNING_P2P, or in RUNNING while the
+ * VM is in SHUTDOWN. The P2P round of the VM stop has already run, so the
+ * devices are stopped one by one.
+ */
+static int vfio_migration_stop_after_shutdown(VFIODevice *vbasedev,
+                                              Error **errp)
+{
+    VFIOMigration *migration = vbasedev->migration;
+
+    if (migration->device_state != VFIO_DEVICE_STATE_RUNNING_P2P &&
+        !(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 +494,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 +986,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 +1126,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 +1195,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 v5 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:
- [Medium] vfio/migration: stop helper misses RUNNING device left in a PAUSED VM
- [Low] vfio: stop_after_shutdown() stops RUNNING devices one by one without a P2P round
--

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

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

This patch adds vfio_migration_stop_after_shutdown(). It stops a VFIO
device that is still in RUNNING_P2P, or in RUNNING while the VM is in
SHUTDOWN. The helper is called from a new shutdown notifier and from
vfio_save_setup().

> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c

[ ... ]

> +/*
> + * Stop a device that a guest shutdown left running, before it is closed or
> + * saved. Only that leaves a device in RUNNING_P2P, or in RUNNING while the
> + * VM is in SHUTDOWN. The P2P round of the VM stop has already run, so the
> + * devices are stopped one by one.
> + */
> +static int vfio_migration_stop_after_shutdown(VFIODevice *vbasedev,
> +                                              Error **errp)
> +{
> +    VFIOMigration *migration = vbasedev->migration;
> +
> +    if (migration->device_state != VFIO_DEVICE_STATE_RUNNING_P2P &&
> +        !(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: Medium]
Can a RUNNING device be left behind in a PAUSED or PRELAUNCH VM here?
The helper stops a RUNNING device only when the run state is exactly
RUN_STATE_SHUTDOWN.

One path that seems to reach this state:

    VM is PAUSED, VFIO device is in STOP
    migration_stop_vm()
      vm_stop_force_state(RUN_STATE_FINISH_MIGRATE)
        (VM not live: runstate_set() only, no vmstate-change
         notification, device stays in STOP)
    migration fails or is cancelled
    migration_cleanup()
      migration_call_notifiers(MIG_EVENT_PRECOPY_FAILED)
        vfio_migration_state_notifier()
          vfio_migration_set_state_or_reset(RUNNING)
    migration_iteration_finish()
      if (runstate_is_live(s->vm_old_state)) {
          ...
      } else {
          if (runstate_check(RUN_STATE_FINISH_MIGRATE)) {
              runstate_set(s->vm_old_state);
          }
      }

For a non-live old state, vm_start() is not called. The device ends up
RUNNING in a PAUSED VM.

On a later quit, vfio_migration_shutdown_notifier() calls
vfio_migration_stop_after_shutdown(). The device state is RUNNING, not
RUNNING_P2P, and the run state is PAUSED, not SHUTDOWN. So the helper
returns 0 and the device is closed running.

A retried migration reaches vfio_save_setup(), which calls the same
helper and also returns 0.

The commit message says "Today only a failed migration of such a VM leaves
a device running there", and the helper comment says "or in RUNNING while
the VM is in SHUTDOWN". Both seem to understate this case, because the
same failed-migration path also leaves a RUNNING device in PAUSED and
PRELAUNCH VMs.

The follow-up patch "vfio/migration: don't stop the device on guest
shutdown" only adds an early return in vfio_vmstate_change(). It doesn't
change the PRECOPY_FAILED handler or the run-state check in this helper.

Should the helper cover this case, or should the PRECOPY_FAILED handler
avoid setting RUNNING when the VM is not running?

[Severity: Low]
Is the comment "The P2P round of the VM stop has already run, so the
devices are stopped one by one" accurate for the RUNNING branch?

A device in plain RUNNING in RUN_STATE_SHUTDOWN did not go through
vfio_vmstate_change_prepare(). It is sent straight to STOP with no
all-device RUNNING_P2P round first.

The path to that state is a failed migration of a -no-shutdown VM:

    vfio_migration_state_notifier()
      /* MIG_EVENT_PRECOPY_FAILED */
      vfio_migration_set_state_or_reset(RUNNING)

This moves every device to plain RUNNING while the VM stays in SHUTDOWN.

A later quit or a new save then reaches
vfio_migration_stop_after_shutdown() for each device in turn. With two or
more P2P-capable devices, one device can go to STOP while a peer is still
RUNNING and able to DMA to it.

I haven't shown that this causes a concrete failure. The comment is only
correct for devices found in RUNNING_P2P. The follow-up patch makes
RUNNING_P2P the common case, but it leaves the RUNNING branch and this
comment unchanged.

[ ... ]

> +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);
> +    }
> +}

[Severity: Low]
Each device is stopped individually by its own notifier, with no
coordination between devices, and a failure is only logged with
error_report_err().

Could a device that fails to stop here still be closed running?

[ ... ]

Please let me know if I have misread any of these paths.

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

[PATCH hci-dev v5 2/2] vfio/migration: don't stop 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".

Skip the STOP in this case. The VM stop still runs its P2P round, so a
device that supports P2P only stops its peer to peer DMA; a device
without P2P is left running. The vCPUs are stopped and nothing more is
asked of the device until the reset, quit or migration that must
follow, and each of those resets or stops it: the reset by itself, the
two others with the previous patch. A device in the middle of a
migration is stopped as before, the migration needs it stopped.

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.

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

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

diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index c391422d601..0341cbeeba2 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -415,6 +415,22 @@ 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). Only
+ * a reset, a quit or a migration can follow. The device is not stopped
+ * here, it only stops its P2P DMA like on any VM stop: the reset resets it,
+ * the two other paths stop it with vfio_migration_stop_after_shutdown().
+ * A device in the middle of a migration is stopped as before.
+ *
+ * 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 that a guest shutdown left running, before it is closed or
  * saved. Only that leaves a device in RUNNING_P2P, or in RUNNING while the
@@ -963,6 +979,11 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
         return;
     }
 
+    if (vfio_vm_stopped_by_guest_shutdown(running, state) &&
+        !vfio_device_state_is_precopy(vbasedev)) {
+        return;
+    }
+
     if (running) {
         new_state = VFIO_DEVICE_STATE_RUNNING;
     } else {
-- 
2.47.1

Re: [PATCH hci-dev v5 2/2] vfio/migration: don't stop 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: non-P2P device left RUNNING after shutdown+reset escapes stop on quit/migrate
- [Medium] vfio: shutdown during non-precopy migration leaves device RUNNING at switchover
--

Commit 9684fec7cb9ba3f7a56f75e07435bf932dc36779
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

vfio/migration: don't stop the device on guest shutdown #VSTOR-153976

When the guest shuts down with -no-shutdown, vfio_vmstate_change() no
longer moves the device to STOP. A device with P2P support only stops its
P2P DMA, and a device without P2P is left RUNNING. The device is expected
to be stopped later by a reset, or by vfio_migration_stop_after_shutdown()
on quit or migration.

> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c

[ ... ]

> +/*
> + * The guest has shut down and QEMU keeps the VM stopped (-no-shutdown). Only
> + * a reset, a quit or a migration can follow. The device is not stopped
> + * here, it only stops its P2P DMA like on any VM stop: the reset resets it,
> + * the two other paths stop it with vfio_migration_stop_after_shutdown().
> + * A device in the middle of a migration is stopped as before.
> + *

[Severity: Medium]
Is the claim "A device in the middle of a migration is stopped as before"
accurate for a device that does not support PRE_COPY?

Such a device stays in RUNNING after vfio_save_setup(), and
vfio_device_state_is_precopy() is false for it. Consider this sequence:

    vm_stop(RUN_STATE_SHUTDOWN)
      vfio_vmstate_change()
        vfio_vm_stopped_by_guest_shutdown() && !vfio_device_state_is_precopy()
          return;

The migration thread later calls migration_stop_vm(FINISH_MIGRATE), which
calls vm_stop_force_state(). The run state is already SHUTDOWN, which is
not live, so this path only runs runstate_set() and bdrv_drain_all(). No
vm_state_notify() is sent.

vfio_migration_stop_after_shutdown() is called only from vfio_save_setup(),
which has already run, and from the shutdown notifier. So can anything stop
the device before the final RAM sync?

vfio_save_complete_precopy() and
vfio_multifd_save_complete_precopy_thread() both say "We reach here with
device state STOP or STOP_COPY only". They would then request STOP_COPY
from a RUNNING device, and the device may have been DMAing during the final
RAM sync.

> +static bool vfio_vm_stopped_by_guest_shutdown(bool running, RunState state)
> +{
> +    return !running && state == RUN_STATE_SHUTDOWN &&
> +           qemu_shutdown_action_is_pause();
> +}

[ ... ]

> @@ -963,6 +979,11 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
>      }
>  
> +    if (vfio_vm_stopped_by_guest_shutdown(running, state) &&
> +        !vfio_device_state_is_precopy(vbasedev)) {
> +        return;
> +    }
> +

[Severity: Medium]
Can a non-P2P device be left RUNNING here, so that it escapes the stop on
quit or migrate after a system_reset?

With the pause action, this early return leaves migration->device_state
as VFIO_DEVICE_STATE_RUNNING. Then system_reset runs qemu_system_reset()
and vfio_pci_reset(), which only issues VFIO_DEVICE_RESET, a hot reset or
a PM reset. It does not update migration->device_state.

main_loop_should_exit() then does:

    if (!runstate_check(RUN_STATE_RUNNING) &&
        !runstate_check(RUN_STATE_INMIGRATE) &&
        !runstate_check(RUN_STATE_FINISH_MIGRATE)) {
        runstate_set(RUN_STATE_PRELAUNCH);
    }

The run state is now PRELAUNCH. vfio_migration_stop_after_shutdown() has
this check:

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

With a cached RUNNING state and run state PRELAUNCH, this returns 0 and
does nothing.

On quit, vfio_migration_shutdown_notifier() calls the helper, which is a
no-op, and vm_shutdown() sends no VM state change because PRELAUNCH is not
live. Can the device then be closed while still RUNNING?

On migration or savevm from PRELAUNCH, vfio_save_setup() calls the same
helper, which is also a no-op. Can the device then be saved from RUNNING,
contrary to the assumption in vfio_save_complete_precopy()?

Before this patch the shutdown stop left the device in STOP, so this gap
is new. The commit message says "the reset by itself" covers it, but
vfio_pci_reset() does not stop the device, and it can fall through
silently when no FLR, hot reset or PM reset works.

libvirt's soft reboot sends cont right after the reset, so that flow is
fine. A manual reset followed by quit or migrate would hit this.

>      if (running) {
>          new_state = VFIO_DEVICE_STATE_RUNNING;
>      } else {

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