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