Re: [PATCH QEMU HCI-8.1 v3 21/28] vfio/migration: don't migrate device state during CPR

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] vfio/migration: stale cached device_state when CPR starts from a paused VM
- [Medium] vfio/migration: CPR skips VFIO state using local-only checks, breaking old-source to new-target streams
- [Medium] vfio/migration: cached device_state not synced after CPR when guest was paused
--

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

vfio/migration: don't migrate device state during CPR

This patch skips VFIO device state save and load while a CPR migration
(cpr-transfer or cpr-exec) is in progress. It gates the save, load and
VM stop/start handlers on cpr_is_in_progress(). It also resets
multifd_transfer in vfio_multifd_cleanup().

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

[ ... ]

> @@ -674,6 +674,16 @@ static void vfio_save_state(QEMUFile *f, void *opaque)
>      Error *local_err = NULL;
>      int ret;
>  
> +    /*
> +     * .is_active doesn't cover the non-iterable stage, so the section is
> +     * still sent: leave it empty, or new QEMU would load the config into
> +     * the preserved device and re-program its MSI-X.
> +     */
> +    if (cpr_is_in_progress()) {
> +        qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
> +        return;
> +    }
> +

[Severity: High]
Can the new QEMU end up with a stale cached device_state when CPR starts
from an already paused VM?

If the user stops the VM before CPR, vfio_vmstate_change(running=false)
runs while cpr_is_in_progress() is false. It moves the kernel device to
STOP and caches STOP in migration->device_state.

Once CPR starts, vfio_save_state() emits only END_OF_STATE, so no
device_state reaches the target. The new QEMU then runs
vfio_migration_init(), which does:

    migration->device_state = VFIO_DEVICE_STATE_RUNNING;

It never queries the kernel. The preserved device is still in STOP.

The target stays paused, because process_incoming_migration_bh() calls
runstate_set(RUN_STATE_PAUSED) and then migration_incoming_state_destroy().
That clears the incoming mode, so cpr_is_in_progress() becomes false.

A later 'cont' then reaches vfio_vmstate_change(running=true), which
requests RUNNING. In vfio_migration_set_state() this hits:

    if (new_state == migration->device_state) {
        return 0;
    }

No ioctl is issued, and the kernel device stays in STOP while the guest
runs. Could the cached state be synchronized with the preserved device in
this case?

The commit message says the device "is never stopped". Is that accurate
for a source that was paused before CPR?

[ ... ]

> @@ -810,6 +820,11 @@ static bool vfio_switchover_ack_needed(void *opaque)
>  {
>      VFIODevice *vbasedev = opaque;
>  
> +    /* No device state is sent during CPR, so no ACK to wait for */
> +    if (cpr_is_in_progress()) {
> +        return false;
> +    }
> +
>      return vfio_precopy_supported(vbasedev);
>  }

[ ... ]

> +static bool vfio_is_active(void *opaque)
> +{
> +    return !cpr_is_in_progress();
> +}

[Severity: Medium]
Is it a problem that the decision to send or load VFIO state is made only
from local runtime state?

Nothing in the stream tells the peer whether VFIO sections are present.
Consider a CPR incoming migration from a source that does not have this
patch:

vfio_is_active() returns false on the target, so loadvm_state_setup() skips
vfio_load_setup() and migration->data_fd stays -1. The old source still
sends VFIO_MIG_FLAG_DEV_DATA_STATE. vfio_load_state() has no CPR guard:

    case VFIO_MIG_FLAG_DEV_DATA_STATE:
        {
            uint64_t data_size = qemu_get_be64(f);

            if (data_size) {
                ret = vfio_load_buffer(f, vbasedev, data_size);

Would that call qemu_file_get_to_fd(f, -1, size) and fail? The config
section would also still be loaded into the live device.

The reverse pair (new source, old target) looks similar. The old target
runs vfio_load_setup() and puts the live device into RESUMING. Its
vfio_switchover_ack_needed() has no CPR check, so it waits for an ACK that
the new source never sends.

docs/devel/migration/CPR.rst describes the incoming side as a new QEMU
binary, so mixed-version CPR seems to be a documented use. Is a stream flag
or a compat property needed here, or is mixed-version CPR intentionally
unsupported? I found no later patch in the series that adds a CPR guard in
vfio_load_state().

[ ... ]

> @@ -882,6 +914,10 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
>      Error *local_err = NULL;
>      int ret;
>  
> +    if (cpr_is_in_progress()) {
> +        return;
> +    }
> +
>      if (running) {
>          new_state = VFIO_DEVICE_STATE_RUNNING;
>      } else {

[Severity: Medium]
Does this early return leave the cached migration->device_state out of sync
with the preserved device on the target?

On the target, vm_start() runs while the incoming mode is still set, so
this handler is skipped. For a guest paused before CPR, the kernel device
is in STOP, but vfio_migration_init() has cached RUNNING.

After the incoming migration completes, the later 'cont' calls
vfio_vmstate_change(running=true):

vfio_vmstate_change()
  vfio_migration_set_state()
    if (new_state == migration->device_state)
        return 0;

The request is silently skipped, so the kernel device is never resumed.
Should the cache be resynchronized, or the real state queried, when the
device is inherited through CPR?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261016-vfio_v1-v3-0-47487cb5a8e5@virtuozzo.com?part=21