Re: [PATCH QEMU HCI-8.1 v6 25/34] 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: cpr_is_in_progress() is false on cpr-transfer source
- [High] migration: cpr_is_in_progress() is false on the cpr-transfer source
- [Medium] vfio/migration: stop/cont ignored for whole CPR outgoing window, not just the handover
--

Commit 54a8e2428cc329d383d08292ed034898e8bf828e
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

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

This patch gates the VFIO migration handlers on cpr_is_in_progress(). With
the gates in place, device state is neither saved nor loaded during
cpr-transfer or cpr-exec, and the device state is not changed on VM
stop/start. 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]
Is cpr_is_in_progress() still true here on a cpr-transfer source?

On the source it returns cpr_state_file != NULL:

migration/cpr.c:cpr_is_in_progress() {
    ...
    return cpr_state_file != NULL;
}

For cpr-transfer, qmp_migrate() only registers a HUP watch:

qmp_migrate()
    cpr_transfer_add_hup_watch(s, qmp_migrate_finish_cb, addr)

When the target closes the CPR socket, the callback runs:

qmp_migrate_finish_cb()
    qmp_migrate_finish(addr, NULL);
    cpr_state_close();

qmp_migrate_finish() ends up in socket_start_outgoing_migration(), which
only starts qio_channel_socket_connect_async(). The completion
(socket_outgoing_migration() -> migration_channel_connect()), the migration
thread, qemu_savevm_state_setup(), vm_stop() and vfio_save_state() all run
after the callback has returned, with cpr_state_file already NULL.

If so, cpr_is_in_progress() is false for the whole data migration on a
cpr-transfer source, and the gates in vfio_save_state(),
vfio_switchover_ack_needed(), vfio_vmstate_change_prepare() and
vfio_vmstate_change() are all bypassed. Wouldn't the device then still be
stopped and put into PRE_COPY or STOP_COPY, and its config and data state
still be sent, which is the failure the commit message describes for the
NVIDIA vGPU?

The same question applies to the source-side comment in
cpr_is_in_progress(), which says the state is open "from cpr_state_save()
to cpr_state_close()". For cpr-transfer that window ends before the data
migration starts, whereas for cpr-exec cpr_state_close() runs in
migration_cleanup() after qemu_savevm_state_cleanup().

I did not find a later patch in the series that changes cpr_is_in_progress()
or the cpr_state_close() call sites.

[ ... ]

> +/*
> + * cpr-transfer and cpr-exec hand the running device over to the new QEMU
> + * as is: its fds, mappings and interrupts are preserved and it never stops.
> + * Device state migration must stay out of it, else the device is stopped,
> + * its state saved and then resumed into the very same device.
> + */
> +static bool vfio_is_active(void *opaque)
> +{
> +    return !cpr_is_in_progress();
> +}
> +

[Severity: High]
On a cpr-transfer source, wouldn't vfio_is_active() return true here, given
that cpr_state_file has already been cleared by qmp_migrate_finish_cb()?

In that case save_setup, iterate and complete would all still run for the
device.

On the target, cpr_is_in_progress() is true because the incoming mode is
set, so load_setup is skipped. vfio_load_state() has no CPR gate:

hw/vfio/migration.c:vfio_load_state() {
    ...
    data = qemu_get_be64(f);
    while (data != VFIO_MIG_FLAG_END_OF_STATE) {
    ...
}

Could real DEV_CONFIG_STATE or DEV_DATA_STATE sent by the source then be
loaded into the preserved live device? That would re-program MSI-X, or
write to a data_fd that was never opened. The two sides would also disagree
about whether device state is migrated.

[ ... ]

> @@ -857,6 +884,11 @@ static void vfio_vmstate_change_prepare(void *opaque, bool running,
>      Error *local_err = NULL;
>      int ret;
>  
> +    /* The device is preserved, not a plain stop/cont: keep its state */
> +    if (cpr_is_in_progress()) {
> +        return;
> +    }
> +

[Severity: Medium]
Can this gate skip a user-issued stop or cont while a CPR migration has been
started but the VM has not yet been stopped for handover?

On the source, cpr_state_save() sets cpr_state_file from qmp_migrate(),
before the migration thread starts. For cpr-transfer, the source then
returns to the main loop and waits for the target's HUP, with
cpr_state_file open the whole time:

qmp_migrate()
    cpr_state_save()
    cpr_transfer_add_hup_watch(s, qmp_migrate_finish_cb, addr)

During that window, a QMP stop or cont reaches vfio_vmstate_change_prepare()
and vfio_vmstate_change(), and both return early on cpr_is_in_progress().

The commit message says that a plain stop/cont must still change the device
state when the mode is merely set. Does that hold once CPR migration has
started but has not yet stopped the VM? A stopped guest could keep a
running, DMA-capable device, and migration->device_state would not be
updated.

The final-state series still has the source-side early return in
vfio_vmstate_change().

> @@ -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;
> +    }
> +

[Severity: High]
Is this gate effective on a cpr-transfer source?

With cpr_state_file already cleared in qmp_migrate_finish_cb(), the early
return would not be taken when the migration thread calls vm_stop(). The
device would then still be moved to STOP or STOP_COPY, and the same applies
to vfio_save_state(), vfio_is_active() and vfio_vmstate_change_prepare().

In contrast, cpr-exec appears to work, because cpr_state_close() is called
from migration_cleanup() after qemu_savevm_state_cleanup():

migration/migration.c:migration_cleanup() {
    ...
    qemu_savevm_state_cleanup();
    cpr_state_close();
}

Should the early cpr_state_close() in qmp_migrate_finish_cb() be reconsidered,
or the predicate changed, so that cpr_is_in_progress() holds for the data
migration in cpr-transfer as well? The close in qmp_migrate_finish_cb()
seems to be the defect, rather than any mismatch between stop and start
values.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=25