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

[no-reply@sashiko.virtuozzo.com]

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