Re: [PATCH hci-dev v4 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 1 potential issue(s) to consider:
- [Low] vfio/migration: hot-unplug in RUN_STATE_SHUTDOWN still closes a running device
--

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

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

This patch makes VFIO stop a device that is still RUNNING when the VM is in
RUN_STATE_SHUTDOWN. It does this from a new shutdown notifier when QEMU
quits and at the start of vfio_save_setup(). The goal is to avoid closing
or saving a device in the running state, which breaks NVIDIA vGPU VFs.

> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c

[ ... ]

> @@ -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);

[Severity: Low]
Can a device still be closed in the running state through the hot-unplug
path?

The stop logic is only reached from the shutdown notifier and from
vfio_save_setup().  The unplug path does not call it:

vfio_exitfn()
  vfio_migration_exit()
    vfio_migration_deinit()
      notifier_remove(&migration->shutdown);
      migration_remove_notifier(&migration->migration_state);
      qemu_del_vm_change_state_handler(migration->vm_state);
      unregister_savevm(...);

vfio_migration_stop_after_shutdown() is never called here.

With the VM in RUN_STATE_SHUTDOWN after a -no-shutdown guest shutdown, the
device is left RUNNING (by design, per the next patch).  A QMP device_del
can then unplug the device without guest cooperation, for example when a
PCIe slot is already powered off:

pcie_cap_slot_unplug_request_cb()
    /* slot is powered off -> unplug without round-trip to the guest */
    pcie_cap_slot_do_unplug(hotplug_pdev);

In that case the device would be closed while RUNNING, which is the
failure the commit message describes for NVIDIA vGPU.  I did not trace
this for every machine type.

The commit message says "From there QEMU can only reset the VM, quit, or
migrate or save it."  Is that accurate, given that device_del is also
possible on a stopped VM?

Should vfio_migration_deinit() also stop a device left running in
RUN_STATE_SHUTDOWN before the notifiers are removed?

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