Re: [PATCH hci-dev] vfio/migration: leave the device alone when the guest shuts down #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: SHUTDOWN guard leaves device RUNNING when migration starts from SHUTDOWN
- [Medium] vfio/migration: RUN_STATE_SHUTDOWN guard also skips device STOP at process exit
- [Low] vfio/migration: duplicate RUN_STATE_SHUTDOWN guards in vmstate handlers
--

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

vfio/migration: leave the device alone when the guest shuts down #VSTOR-153976

This patch makes vfio_vmstate_change_prepare() and vfio_vmstate_change()
return early when the VM stops with RUN_STATE_SHUTDOWN. The device state is
then left unchanged on guest shutdown with -no-shutdown. The aim is to avoid
the vGPU manager treating the STOP as the start of a migration.

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

[ ... ]

> @@ -904,6 +904,11 @@ static void vfio_vmstate_change_prepare(void *opaque, bool running,
>          return;
>      }
>  
> +    /* The guest has shut down, the device state stays as it is */
> +    if (!running && state == RUN_STATE_SHUTDOWN) {
> +        return;
> +    }
> +

[Severity: Low]
This isn't a bug, but the tree this was reviewed against already has an
identical guard in vfio_vmstate_change_prepare() just above this one, so
the added copy is unreachable:

    /* The guest has shut down, the device state stays as it is */
    if (!running && state == RUN_STATE_SHUTDOWN) {
        return;
    }

Could the patch be respun so that only one guard remains?

Because the guard is already present before this patch, the commit message
claim "With this patch applied, soft reboot + subsequent live migration pass
successfully" cannot be attributed to this change.

[Severity: Medium]
Does this guard also match the process-exit path?  qemu_cleanup() calls
vm_shutdown(), which calls do_vm_stop(RUN_STATE_SHUTDOWN, false).  If the
VM is live, vm_state_notify(0, RUN_STATE_SHUTDOWN) reaches this handler
with running=false:

qemu_cleanup()
  vm_shutdown()
    do_vm_stop(RUN_STATE_SHUTDOWN, false)
      vm_state_notify(0, RUN_STATE_SHUTDOWN)
        vfio_vmstate_change_prepare()

The guard keys only on the run state, so the transition to RUNNING_P2P or
PRE_COPY_P2P is skipped when QEMU quits from a live VM.  The commit message
justifies the skip with "the reset resets the device too", but no reset
follows on the quit path.

vfio_migration_exit() and vfio_migration_deinit() only remove the notifiers
and the vm-change handler, and do not stop or reset the device.

Can the guard distinguish a guest-initiated shutdown with -no-shutdown from
the vm_shutdown() use of the same run state?

> @@ -950,6 +955,15 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
>          return;
>      }
>  
> +    /*
> +     * The guest has shut down and QEMU keeps the VM stopped. It can only
> +     * be reset or quit now, so leave the device state as it is: the reset
> +     * resets the device too.
> +     */
> +    if (!running && state == RUN_STATE_SHUTDOWN) {
> +        return;
> +    }
> +

[Severity: Low]
This isn't a bug, but the same guard and its five-line comment are
duplicated in vfio_vmstate_change(), where an identical copy already exists
earlier in the function.  The second copy can never execute.

[Severity: Medium]
Can migration or savevm that starts from RUN_STATE_SHUTDOWN now leave the
device running?  With this guard, the device stays RUNNING or PRE_COPY
after guest shutdown.  The migration path then looks like:

migration_stop_vm(s, RUN_STATE_FINISH_MIGRATE)
  vm_stop_force_state()
    /* runstate_is_live(SHUTDOWN) is false */
    runstate_set(state);

Since SHUTDOWN is not a live state, vm_stop_force_state() only sets the
run state and sends no vm_state_notify().  The runstate table allows
SHUTDOWN -> FINISH_MIGRATE, so the vfio notifiers never run.

vfio_save_complete_precopy() then does:

    /* We reach here with device state STOP or STOP_COPY only */
    ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP_COPY,
                                   VFIO_DEVICE_STATE_STOP, &local_err);

The comment is no longer true, and STOP as the recover state is also wrong
if the device was RUNNING or PRE_COPY.

Could a device that was never stopped dirty pages after the final RAM sync?

The savevm path goes through vm_stop() and do_vm_stop(), which also notify
only from live states.  I did not trace that path to its end.

Before this patch the device was already in STOP in this scenario.  Could
the guard be restricted so that migration and savevm from SHUTDOWN still
stop the device?

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