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

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

[Andrey Drobyshev]

v3 -> v4, after Sashiko's review:

 - patch 2: the guest shutdown skips only the final STOP. The P2P round
   of the VM stop still runs, and a device in the middle of a migration
   is stopped as before;
 - patch 1: the deferred stop also takes a device in RUNNING_P2P, and a
   failed migration is named as the other way to leave a device running;
 - patch 2: renamed to "don't stop the device on guest shutdown"; the
   message no longer claims the guest released the device.

v3: http://lore.virtuozzo.com/qemu/20261109154346.153153-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               | 68 +++++++++++++++++++++++++++++++
 hw/vfio/vfio-migration-internal.h |  1 +
 include/system/runstate.h         |  1 +
 system/runstate.c                 |  5 +++
 4 files changed, 75 insertions(+)


base-commit: 5d129c4dd43d131fbcdd411ad30be8a9a641895e
--
2.47.1

[PATCH hci-dev v4 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. 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               | 47 +++++++++++++++++++++++++++++++
 hw/vfio/vfio-migration-internal.h |  1 +
 include/system/runstate.h         |  1 +
 system/runstate.c                 |  5 ++++
 4 files changed, 54 insertions(+)

diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 7e7fb496bbe..064da62ddc2 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -415,6 +415,28 @@ 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, before it is closed or saved. Such a device comes from the
+ * guest shutdown itself, or from a failed migration of the VM stopped that
+ * way. The P2P round of the VM stop has already run, so the devices are
+ * stopped one by one. 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 &&
+         migration->device_state != VFIO_DEVICE_STATE_RUNNING_P2P)) {
+        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 +495,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 +987,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 +1127,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 +1196,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 v4 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 1 potential issue(s) to consider:
- [Low] vfio/migration: hot-unplug in RUN_STATE_SHUTDOWN still closes a running device
--

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

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

This patch makes VFIO stop a device that is still RUNNING when the VM is in
RUN_STATE_SHUTDOWN. It does this from a new shutdown notifier when QEMU
quits and at the start of vfio_save_setup(). The goal is to avoid closing
or saving a device in the running state, which breaks NVIDIA vGPU VFs.

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

[ ... ]

> @@ -1150,6 +1196,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);

[Severity: Low]
Can a device still be closed in the running state through the hot-unplug
path?

The stop logic is only reached from the shutdown notifier and from
vfio_save_setup().  The unplug path does not call it:

vfio_exitfn()
  vfio_migration_exit()
    vfio_migration_deinit()
      notifier_remove(&migration->shutdown);
      migration_remove_notifier(&migration->migration_state);
      qemu_del_vm_change_state_handler(migration->vm_state);
      unregister_savevm(...);

vfio_migration_stop_after_shutdown() is never called here.

With the VM in RUN_STATE_SHUTDOWN after a -no-shutdown guest shutdown, the
device is left RUNNING (by design, per the next patch).  A QMP device_del
can then unplug the device without guest cooperation, for example when a
PCIe slot is already powered off:

pcie_cap_slot_unplug_request_cb()
    /* slot is powered off -> unplug without round-trip to the guest */
    pcie_cap_slot_do_unplug(hotplug_pdev);

In that case the device would be closed while RUNNING, which is the
failure the commit message describes for NVIDIA vGPU.  I did not trace
this for every machine type.

The commit message says "From there QEMU can only reset the VM, quit, or
migrate or save it."  Is that accurate, given that device_del is also
possible on a stopped VM?

Should vfio_migration_deinit() also stop a device left running in
RUN_STATE_SHUTDOWN before the notifiers are removed?

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

[PATCH hci-dev v4 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
the device only stops its peer to peer DMA. 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 064da62ddc2..34aaa35a2cf 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 still running while the VM is stopped after a guest
  * shutdown, before it is closed or saved. Such a device comes from the
@@ -964,6 +980,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 v4 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 3 potential issue(s) to consider:
- [Medium] vfio/migration: stale RUNNING_P2P after system_reset breaks precopy save in PRELAUNCH
- [Medium] vfio/migration: vfio_migration_deinit() has no stop for a device left running after guest shutdown
- [Low] vfio: shutdown-stop skip leaves non-P2P devices fully RUNNING, unlike the commit message says
--

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

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

This patch skips the VFIO device STOP transition in vfio_vmstate_change()
when the VM stops with RUN_STATE_SHUTDOWN and the shutdown action is
"pause" (-no-shutdown). The aim is to stop NVIDIA vGPU from treating the
STOP as the start of a migration during a libvirt soft reboot. A device in
pre-copy is still stopped as before.

> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index 064da62ddc26..34aaa35a2cf5 100644
> --- 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: Low]
This isn't a bug, but the comment and the commit message do not match the
code for devices without VFIO_MIGRATION_P2P. The commit message says "The
VM stop still runs its P2P round, so the device only stops its peer to peer
DMA."

vfio_migration_init() registers the prepare callback only for P2P-capable
devices:

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

For a device without P2P, the early return in vfio_vmstate_change() leaves
the device fully in VFIO_DEVICE_STATE_RUNNING while the vCPUs are stopped.
Could the commit message and comment say that such devices are left
completely running until reset, quit or migration?

[ ... ]

> @@ -964,6 +980,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;
> +    }
> +

[Severity: Medium]
Can migration->device_state go stale here after system_reset? With
-no-shutdown, the guest shuts down and vm_stop(RUN_STATE_SHUTDOWN) runs.
vfio_vmstate_change_prepare() moves the device to RUNNING_P2P, and this
early return means it is never moved to STOP, so the cached device_state
stays RUNNING_P2P.

system_reset then resets the device in the kernel. In main_loop_should_exit()
the runstate becomes PRELAUNCH:

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

Nothing in hw/vfio resyncs migration->device_state on reset. The
compensating stop is gated on the SHUTDOWN runstate, so it does nothing in
PRELAUNCH:

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

If a migration or savevm is then started from PRELAUNCH without a cont,
vfio_save_setup() calls vfio_migration_stop_after_shutdown(), which is a
no-op. For a device with pre-copy support, the switch on
migration->device_state accepts only RUNNING and STOP:

    default:
        error_setg(errp, "%s: Invalid device state %d", vbasedev->name,
                   migration->device_state);
        return -EINVAL;

RUNNING_P2P reaches the default case, so migration setup fails with
-EINVAL. Before this patch the device was in STOP at this point.

The soft reboot order (reset, then cont) works, since cont sets RUNNING.
Does the commit message claim that "each of those resets or stops it" hold
for reset followed by migration without cont? The cached state is also out
of sync with the kernel state, which is RUNNING after VFIO_DEVICE_RESET.

[Severity: Medium]
Can a device be closed while still in VFIO_DEVICE_STATE_RUNNING on a removal
path? After this patch the device stays RUNNING after a guest shutdown with
pause. The stop-after-shutdown helper has only two callers, vfio_save_setup()
and vfio_migration_shutdown_notifier(), and the notifier returns early when
the shutdown action is pause:

    /* QEMU is quitting: a device left running must be stopped first */
    if (qemu_shutdown_action_is_pause()) {
        return;
    }

vfio_migration_deinit() does not stop the device:

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

So an unrealize or unplug in a SHUTDOWN VM would go through
pci_qdev_unrealize()->vfio_exitfn()->vfio_migration_exit()->
vfio_migration_deinit() and later vfio_device_detach() with the device
RUNNING. The preceding patch says a VF closed while running breaks the next
VM on NVIDIA vGPU 595.71.03.

Unplug without guest cooperation looks possible, for example
pcie_cap_slot_unplug_request_cb() when the slot is already powered off.
Is a removal in RUN_STATE_SHUTDOWN reachable, and if so, should the removal
path also stop the device? The commit message lists only reset, quit and
migration as follow-up paths.

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

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