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

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

[Andrey Drobyshev]

v5 -> v6, after Sashiko's review:

 - patch 2: the device is stopped as before when a migration is running,
   not only when it is in a precopy state. A device without precopy
   support kept running after a guest shutdown during a migration;
 - patch 1: a device that fails to stop is reset, as on a VM stop, so it
   is never closed or saved running.

v5: http://lore.virtuozzo.com/qemu/20261109190342.200293-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 v6 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               | 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..9c0ba2bde5c 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 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 devices are stopped one by one: a device in
+ * RUNNING_P2P has been through the P2P round of the VM stop, a device in
+ * RUNNING was set there by a failed migration, which has no such round.
+ */
+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_or_reset(vbasedev, VFIO_DEVICE_STATE_STOP,
+                                             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 v6 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 3 potential issue(s) to consider:
- [Medium] vfio/migration: cached device_state is stale after reset of a stopped VM
- [Low] vfio/migration: commit message and comment describe RUNNING_P2P state that only exists after patch 2
- [Low] vfio: stop_after_shutdown() stops RUNNING devices to STOP with no P2P quiesce round
--

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

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

This commit adds a helper that stops a VFIO device left in RUNNING_P2P, or
in RUNNING while the VM is in SHUTDOWN. The helper is called from a new
shutdown notifier and at the start of vfio_save_setup(), so a device is
not closed or saved while running after a -no-shutdown guest shutdown.

> 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 devices are stopped one by one: a device in
> + * RUNNING_P2P has been through the P2P round of the VM stop, a device in
> + * RUNNING was set there by a failed migration, which has no such round.
> + */
> +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;
> +    }

[Severity: Medium]
Can a device still be closed or saved running after a reset of the stopped
VM? The stop decision uses only the cached migration->device_state.

At this commit, a VM stop with RUN_STATE_SHUTDOWN runs
vfio_vmstate_change(), which moves the device to STOP and caches STOP.

A later reset goes through qemu_system_reset() -> qemu_devices_reset() ->
vfio_pci_reset() -> ioctl(VFIO_DEVICE_RESET). This path does not go through
vm_state_notify() and does not update migration->device_state.

By contrast, the reset_device path in vfio_migration_set_state() does
refresh the cache:

    if (ioctl(vbasedev->fd, VFIO_DEVICE_RESET)) {
        hw_error(...);
    }

    vfio_migration_set_device_state(vbasedev, VFIO_DEVICE_STATE_RUNNING);

If the reset returns the device to RUNNING in the kernel, the cache would
still say STOP. vfio_migration_stop_after_shutdown() would then match
neither RUNNING_P2P nor (SHUTDOWN and RUNNING), since the run state is
PRELAUNCH, and return 0. The device would then be closed or saved running,
which is what the patch aims to prevent.

The kernel-side behaviour of VFIO_DEVICE_RESET on a migration-capable
device was not verified here.

[Severity: Low]
Is the commit message accurate for this patch on its own? It says:

    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.

The code comment says "Only that leaves a device in RUNNING_P2P".

At this commit, vm_state_notify(false) runs vfio_vmstate_change_prepare(),
which sets RUNNING_P2P, and then vfio_vmstate_change(), which sets STOP in
the same pass:

    new_state =
        (vfio_device_state_is_precopy(vbasedev) &&
         (state == RUN_STATE_FINISH_MIGRATE || state == RUN_STATE_PAUSED)) ?
            VFIO_DEVICE_STATE_STOP_COPY :
            VFIO_DEVICE_STATE_STOP;

A reset therefore does not find the device in RUNNING_P2P here. The
RUNNING_P2P arm of the new helper is unreachable until the follow-up patch
adds an early return in vfio_vmstate_change() for the guest-shutdown case.

Also, "Only that leaves a device in RUNNING_P2P" does not seem strictly
true, since a failed vfio_vmstate_change() on VM start can leave
RUNNING_P2P with the VM running.

Could the commit message and comment be reworded, or this patch be ordered
after the one that makes the RUNNING_P2P case reachable?

> +
> +    return vfio_migration_set_state_or_reset(vbasedev, VFIO_DEVICE_STATE_STOP,
> +                                             errp);
> +}

[Severity: Low]
Is it safe to move a device in RUNNING directly to STOP here? This bypasses
the RUNNING -> RUNNING_P2P step that vfio_vmstate_change_prepare() runs for
all devices before any device is stopped.

The helper is called per device, from vfio_migration_shutdown_notifier()
and from each device's vfio_save_setup(), so other devices may still be
RUNNING when one is stopped.

For example, with -no-shutdown, a migration of a SHUTDOWN VM fails.
vfio_migration_state_notifier() then sets every device to RUNNING with no
P2P round. A following quit or a new save moves one RUNNING device straight
to STOP while its peers are still RUNNING, so a running peer could issue
P2P DMA to a stopped device.

The "one by one" comment does not address this. The next patch only adds a
skip in vfio_vmstate_change() and leaves this RUNNING branch unchanged.

Would moving all RUNNING devices to RUNNING_P2P first, as
vfio_vmstate_change_prepare() does, be needed before stopping any of them?

[ ... ]

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

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

[Andrey Drobyshev]

On 11/9/26 22:55, no-reply@sashiko.virtuozzo.com wrote:
> [Severity: Medium]
> Can a device still be closed or saved running after a reset of the stopped
> VM? The stop decision uses only the cached migration->device_state.
>
> At this commit, a VM stop with RUN_STATE_SHUTDOWN runs
> vfio_vmstate_change(), which moves the device to STOP and caches STOP.
>
> A later reset goes through qemu_system_reset() -> qemu_devices_reset() ->
> vfio_pci_reset() -> ioctl(VFIO_DEVICE_RESET). This path does not go through
> vm_state_notify() and does not update migration->device_state.

This is how QEMU behaves today, without this series: the cache says STOP
while the kernel has reset the device. The patch does not change that
sequence. A device that has just been reset is idle, it has no guest
driver and no DMA, so closing or saving it is the same as closing or
saving a freshly started device. The patch is about a device the guest
shutdown left running, not about the cache after a reset.

Updating the cache on reset is not safe either: the NVIDIA vGPU driver
keeps its migration state across a reset, so a paused VM that is reset
and continued would end up with the device in STOP while the guest runs.

> [Severity: Low]
> Is the commit message accurate for this patch on its own? It says:
>
>     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.
> [...]
> A reset therefore does not find the device in RUNNING_P2P here. The
> RUNNING_P2P arm of the new helper is unreachable until the follow-up patch
> adds an early return in vfio_vmstate_change() for the guest-shutdown case.

That is intended. This patch has to come first, so that no commit in the
series can close a device running. On its own it is a no-op, and the
message says so: "The next patch does it at every guest shutdown."

> Also, "Only that leaves a device in RUNNING_P2P" does not seem strictly
> true, since a failed vfio_vmstate_change() on VM start can leave
> RUNNING_P2P with the VM running.

It cannot. vfio_vmstate_change() uses vfio_migration_set_state_or_reset(),
which resets the device when the transition fails and caches RUNNING.
RUNNING_P2P is never left behind by a failure.

> [Severity: Low]
> Is it safe to move a device in RUNNING directly to STOP here? This bypasses
> the RUNNING -> RUNNING_P2P step that vfio_vmstate_change_prepare() runs for
> all devices before any device is stopped.
> [...]
> For example, with -no-shutdown, a migration of a SHUTDOWN VM fails.
> vfio_migration_state_notifier() then sets every device to RUNNING with no
> P2P round. A following quit or a new save moves one RUNNING device straight
> to STOP while its peers are still RUNNING, so a running peer could issue
> P2P DMA to a stopped device.

The failure handler itself moves the devices to RUNNING one by one, with
no P2P round, for any stopped VM, today. The VM has been stopped the
whole time, so the guest cannot have started any new DMA. Stopping the
devices one by one afterwards adds nothing to what that path already
does. The comment states exactly this.

[PATCH hci-dev v6 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 9c0ba2bde5c..536fc8b7fbc 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
@@ -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) &&
+        !migration_is_running()) {
+        return;
+    }
+
     if (running) {
         new_state = VFIO_DEVICE_STATE_RUNNING;
     } else {
-- 
2.47.1

Re: [PATCH hci-dev v6 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: device left RUNNING at hot-unplug after guest shutdown
- [Low] vfio/migration: shutdown-skip keyed on run state and global action, not the cause
- [Low] vfio: P2P round still runs on guest-shutdown stop, so the skip is partial
--

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

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

This patch makes vfio_vmstate_change() skip the move to VFIO_DEVICE_STATE_STOP
when the VM stops in RUN_STATE_SHUTDOWN and the shutdown action is "pause"
(-no-shutdown), unless a migration is running. The aim is to stop the NVIDIA
vGPU manager from treating a soft reboot as the start of a migration. The
device is left to be reset or stopped by the later reset, quit or migration
paths.

> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index 9c0ba2bde5c6..536fc8b7fbce 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.
> + *
> + * 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: Low]
The predicate tests only !running, state == RUN_STATE_SHUTDOWN and
qemu_shutdown_action_is_pause(). It does not look at what caused the stop.

In main_loop_should_exit(), any request returned by qemu_shutdown_requested()
reaches vm_stop(RUN_STATE_SHUTDOWN) when the action is pause:

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

The spapr watchdog expiry also calls
qemu_system_vmstop_request(RUN_STATE_SHUTDOWN).

Under -no-shutdown, these host-originated stops would also skip the device
STOP, even though the guest driver is not necessarily gone. The skip also
applies to every migration-capable VFIO device, not only the NVIDIA vGPU
case.

Is the "guest shutdown" wording in the function name, the comment and the
commit message broader than what this check actually matches?

A non-P2P device then stays in RUNNING, and a P2P device only reaches
RUNNING_P2P, while the VM is reported as stopped. Its DMA and interrupts stay
live until a later 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) &&
> +        !migration_is_running()) {
> +        return;
> +    }
> +

[Severity: Medium]
This early return leaves the device in RUNNING or RUNNING_P2P while the VM
sits in RUN_STATE_SHUTDOWN. The commit message and the new comment name only
reset, quit and migration as the follow-up paths that stop the device.

What about device removal? The hot-unplug teardown is:

    vfio_exitfn() -> vfio_migration_exit() -> vfio_migration_deinit()

vfio_migration_deinit() only does the following:

    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);
    vfio_migration_free(vbasedev);

It sets no device state and does not call vfio_migration_stop_after_shutdown().
That helper is called only from vfio_save_setup() and
vfio_migration_shutdown_notifier(). vfio_device_detach(),
vfio_legacy_detach_device() and iommufd_cdev_detach() also issue no stop or
reset.

Before this patch the device was always in STOP by the time it was
unrealized. Can the device now be closed while still in RUNNING or
RUNNING_P2P if it is unplugged after a guest shutdown, with no reset, quit or
save in between?

I could not confirm which configurations can complete an unplug in that
state, so this may be a narrow window.

[Severity: Low]
This skip covers only the final STOP. vfio_vmstate_change_prepare() is not
gated by the new check:

    new_state = migration->device_state == VFIO_DEVICE_STATE_PRE_COPY ?
                    VFIO_DEVICE_STATE_PRE_COPY_P2P :
                    VFIO_DEVICE_STATE_RUNNING_P2P;

vfio_migration_init() registers it for every device with
VFIO_MIGRATION_P2P:

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

So a P2P-capable device still goes RUNNING to RUNNING_P2P on the guest
shutdown stop, before vfio_vmstate_change() returns early.

RUNNING_P2P is a migration-flow state. Could a vendor manager that treats
migration-flow transitions as the start of a migration still be confused by
it?

The commit message says the P2P round is deliberate, but it does not say
whether the NVIDIA vGPU manager tolerates RUNNING_P2P. It also does not say
whether the testing was done on a device with P2P support.

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

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

[Andrey Drobyshev]

On 11/9/26 22:57, no-reply@sashiko.virtuozzo.com wrote:
> [Severity: Low]
> The predicate tests only !running, state == RUN_STATE_SHUTDOWN and
> qemu_shutdown_action_is_pause(). It does not look at what caused the stop.
> [...]
> Under -no-shutdown, these host-originated stops would also skip the device
> STOP, even though the guest driver is not necessarily gone.

Yes, and that is fine. Every stop with the "pause" action ends the same
way: the VM can only be reset, quit or migrated, and each of those
resets or stops the device. Until then the vCPUs are stopped and nothing
new is asked of the device. Whether the guest driver is still loaded
makes no difference to that, so the cause is not needed.

> [Severity: Medium]
> This early return leaves the device in RUNNING or RUNNING_P2P while the VM
> sits in RUN_STATE_SHUTDOWN. The commit message and the new comment name only
> reset, quit and migration as the follow-up paths that stop the device.
>
> What about device removal?
> [...]
> Before this patch the device was always in STOP by the time it was
> unrealized. Can the device now be closed while still in RUNNING or
> RUNNING_P2P if it is unplugged after a guest shutdown, with no reset, quit or
> save in between?

A hot-unplug needs the guest to eject the device, through the attention
button on PCIe or the ACPI notification on legacy PCI. The guest is
halted, so the request stays pending until the reset and cont, and then
the device is handled as on any running VM. The one shortcut that skips
the guest, a slot the guest had already powered off, means the guest had
already released that device. And that shortcut closes a running device
on any running VM today, that is how every VFIO hot-unplug works. So
nothing new is reachable here.

> [Severity: Low]
> This skip covers only the final STOP. vfio_vmstate_change_prepare() is not
> gated by the new check:
> [...]
> RUNNING_P2P is a migration-flow state. Could a vendor manager that treats
> migration-flow transitions as the start of a migration still be confused by
> it?
>
> The commit message says the P2P round is deliberate, but it does not say
> whether the NVIDIA vGPU manager tolerates RUNNING_P2P. It also does not say
> whether the testing was done on a device with P2P support.

It does. The NVIDIA vGPU VF driver takes RUNNING to RUNNING_P2P and back
without telling its manager, that transition is a no-op in its state
table. All testing was done on that device, which supports P2P, so the
P2P round ran at every guest shutdown: 16 soft reboots followed by a
live migration, and four runs of the vGPU test suite with 29 migrations
each, all passed. I will say so in the commit message.