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

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] vfio: comment claims nothing else leaves a running device in SHUTDOWN
- [Low] vfio/migration: RUNNING to STOP skips the P2P quiesce step for P2P-capable devices
--

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

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

This commit adds vfio_migration_stop_after_shutdown(). It moves a
still-RUNNING VFIO device to STOP when the VM is in RUN_STATE_SHUTDOWN.
It is called from a new shutdown notifier when QEMU quits, and from
vfio_save_setup(). The commit message says both calls are no-ops today.

> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index 7e7fb496bbecb..bfafc14e5b942 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c

[ ... ]

> @@ -415,6 +415,25 @@ 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. Nothing else leaves a running device in the SHUTDOWN run state,
> + * so this is a no-op for every other VM.
> + */

[Severity: Low]
Is the statement "Nothing else leaves a running device in the SHUTDOWN run
state" accurate?

vfio_migration_state_notifier() restores the device to RUNNING on
MIG_EVENT_PRECOPY_FAILED, without looking at the run state:

    if (e->type == MIG_EVENT_PRECOPY_FAILED) {
        ret = vfio_migration_set_state_or_reset(vbasedev,
                                                VFIO_DEVICE_STATE_RUNNING,
                                                &local_err);

Consider a -no-shutdown VM sitting in RUN_STATE_SHUTDOWN:

    vfio_save_setup()
      vfio_migration_stop_after_shutdown()   /* device -> STOP */
    migration fails or is cancelled
      vfio_migration_state_notifier(MIG_EVENT_PRECOPY_FAILED)
        vfio_migration_set_state_or_reset(..., VFIO_DEVICE_STATE_RUNNING)

The VM stays in SHUTDOWN and no VM state change is sent, so the device ends
up RUNNING in a stopped VM. This is a second way to get that state, besides
the guest shutdown itself.

The quit notifier and the next vfio_save_setup() stop the device again, so
nothing breaks functionally. Could the comment, and the commit message
wording that both paths are no-ops, be made more precise?

The same applies to a migration destination that ends up in SHUTDOWN, where
the device keeps its init-time RUNNING state.

> +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) {
> +        return 0;
> +    }
> +
> +    return vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP,
> +                                    VFIO_DEVICE_STATE_RUNNING, errp);
> +}

[Severity: Low]
Can this skip the P2P quiesce step for P2P-capable devices?

This moves a single device straight from RUNNING to STOP. Other VM-stop
paths go through vfio_vmstate_change_prepare(), installed in
vfio_migration_init() when VFIO_MIGRATION_P2P is set:

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

That function first moves all devices to RUNNING_P2P or PRE_COPY_P2P. Only
then does vfio_vmstate_change() move each device to STOP.

With the next patch, a guest shutdown no longer runs
vfio_vmstate_change_prepare() or vfio_vmstate_change(), because they return
early via vfio_vm_stopped_by_guest_shutdown(). The devices stay RUNNING.

The only remaining stops are this helper, called from the quit notifier and
from vfio_save_setup(). Each stops one device at a time, while other VFIO
devices may still be RUNNING and able to DMA to it.

This is speculative. It needs several P2P-capable devices and a guest that
left a device doing peer DMA after shutdown. Is the bypass intended to be
safe in that case? The commit message and comments don't mention P2P.

[ ... ]

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