Re: [PATCH QEMU HCI-8.1 v4 23/31] vfio/migration: don't migrate device state during CPR

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] vfio/migration: cpr_is_in_progress() is false on cpr-transfer source, gates ineffective
- [Medium] vfio/migration: cached device_state goes stale when a paused VM is CPR-transferred
--

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

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

This patch makes the VFIO migration handlers skip saving and loading
device state, and skip device state changes on VM stop/start, while
cpr_is_in_progress() is true. 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;
> +    }
> +

[ ... ]

> +/*
> + * 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]
Is cpr_is_in_progress() actually true on the cpr-transfer source while
these handlers run?

On the source it ends with:

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

For cpr-transfer, qmp_migrate() calls cpr_state_save(), which sets
cpr_state_file. When the target closes the CPR socket,
qmp_migrate_finish_cb() runs:

migration/migration.c:qmp_migrate_finish_cb() {
    ...
    qmp_migrate_finish(addr, NULL);

    cpr_state_close();
}

qmp_migrate_finish() only starts the outgoing connection, and
cpr_state_close() then sets cpr_state_file to NULL. The migration thread,
savevm setup, the VM stop at switchover and vfio_save_state() all run after
that.

At that point migrate_mode() is CPR_TRANSFER, cpr_is_incoming() is false and
cpr_state_file is NULL, so cpr_is_in_progress() returns false on the source.

If so, on the source vfio_is_active() returns true, so save_setup, iterate
and complete still run. The vmstate-change notifiers still move the device
to RUNNING_P2P and then STOP or STOP_COPY. vfio_save_state() still emits
the real config or data, and the switchover ACK is still required.

On the target, cpr_is_incoming() is true until
migration_incoming_state_destroy(), so vfio_is_active() is false there and
load_setup is skipped. That means no RESUMING state, and data_fd stays -1.

The loader does not consult is_active, so vfio_load_state() can still
receive the source's real DEV_SETUP, DATA or CONFIG sections. Could those
fail on data_fd == -1, or re-program MSI-X, which is what this patch is
meant to avoid?

cpr-exec looks unaffected, since there cpr_state_close() only runs in
migration_cleanup(). Does the cpr-transfer source need a different
predicate, or should the cpr_state_close() call be moved?

> @@ -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]
Can the cached migration->device_state go stale when a paused VM is
CPR-transferred?

On the paused source, vfio_vmstate_change(running=false) has already put
the kernel device into STOP before CPR starts. The target creates a fresh
VFIOMigration in vfio_migration_init():

    migration->device_state = VFIO_DEVICE_STATE_RUNNING;

Nothing queries the kernel for the real state, because the device fd is
inherited as is.

With this early return, the target's notifiers do nothing while
cpr_is_in_progress() is true. In process_incoming_migration_bh(), a paused
source takes this path:

    } else {
        runstate_set(RUN_STATE_PAUSED);
    }

so vm_start() is not called and no VFIO notifier runs. After that,
migration_incoming_state_destroy() resets the incoming mode and
cpr_is_in_progress() becomes false.

A later 'cont' then calls vfio_vmstate_change(running=true) with
new_state = RUNNING, and vfio_migration_set_state() hits:

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

so the VFIO_DEVICE_FEATURE ioctl is never issued. Would the kernel device
then stay in STOP while QEMU believes it is RUNNING?

Should the cached device_state be reconciled with the real device state
when CPR skips the transition on the target?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=23