Re: [PATCH hci-dev v4 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: 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