Re: [PATCH QEMU HCI-8.1 v5 25/36] 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 for cpr-transfer source after HUP, so the gating is bypassed
- [Medium] vfio/cpr: cpr_is_in_progress() is false on the cpr-transfer source during stream
- [Medium] vfio/migration: vfio_load_state() lacks a CPR guard, so a non-empty stream re-programs MSI-X
--

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

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

This patch makes the VFIO migration handlers do nothing while a CPR
migration (cpr-transfer or cpr-exec) is in progress. It gates the save,
load and VM stop/start paths 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]
Is cpr_is_in_progress() still true on the cpr-transfer source when this
runs?

On the source the predicate reduces to cpr_state_file != NULL:

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

For cpr-transfer, qmp_migrate() calls cpr_state_save() and only arms
cpr_transfer_add_hup_watch().  When the target closes the CPR socket:

qmp_migrate_finish_cb()
    qmp_migrate_finish(addr, NULL);   /* only starts an async connect */
    cpr_state_close();                /* cpr_state_file = NULL */

The migration thread (vm_stop, the vmstate change notifiers,
qemu_savevm_state_setup(), vfio_save_state(), save_complete) runs only
after the connect completes.  By then cpr_is_in_progress() returns false
on the source.

Would that make vfio_is_active(), vfio_save_state(),
vfio_switchover_ack_needed(), vfio_vmstate_change_prepare() and
vfio_vmstate_change() all take the normal migration path on a cpr-transfer
source?  The old QEMU would then stop the live device and save its config
and state.

The target has cpr_is_incoming() true, so it skips load_setup but still
parses the stream.  That looks like the vGPU breakage this patch is meant
to fix, together with a source/target mismatch.

The comment in cpr_is_in_progress() says "An outgoing CPR migration has
the CPR state open, from cpr_state_save() to cpr_state_close()", and the
commit message relies on this.  Does that hold for cpr-transfer?

cpr-exec looks fine, since qmp_migrate_finish() is called directly there
and cpr_state_close() runs only on error or in migration_cleanup().

Nothing later in the series appears to change cpr_is_in_progress(),
cpr_state_save(), cpr_state_close() or qmp_migrate_finish_cb().  The later
patch "VZ: vfio/region: preserve BAR mappings across cpr-exec" adds an
mmap_blocker in vfio_device_attach() that blocks cpr-transfer for VFIO
devices.  That makes the path unreachable at series end, but it doesn't
change the predicate.

[Severity: High]
Is the source-side predicate also an unlatched sample of mutable globals?

qmp_migrate_set_parameters() does not reject changing "mode" while a
migration is running.  Could migrate_mode(), which cpr_is_in_progress()
reads, change between the VM stop and the later vm start or
vfio_save_state() call?  The gates in vfio_vmstate_change_prepare(),
vfio_vmstate_change() and vfio_save_state() would then disagree with each
other.

[ ... ]

> @@ -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();
> +}
> +
>  static const SaveVMHandlers savevm_vfio_handlers = {
> +    .is_active = vfio_is_active,
>      .save_prepare = vfio_save_prepare,

[Severity: Medium]
Should vfio_load_state() have a matching cpr_is_in_progress() guard?

The patch gates only the save side and load_setup (through is_active).
vfio_load_state() has no check:

hw/vfio/migration.c:vfio_load_state() {
    ...
    case VFIO_MIG_FLAG_DEV_CONFIG_STATE:
        ...
        return vfio_load_device_config_state(f, opaque);
}

Nothing in the loadvm section path consults is_active before calling
load_state.  If the target receives a non-empty VFIO section during CPR,
it reaches vfio_pci_load_config() on the preserved device even though
vfio_load_setup() never ran.  That would re-program MSI-X and disable
vector 0.

A DEV_DATA_STATE section would reach vfio_load_buffer() with data_fd == -1
and fail with EBADF, aborting the migration.

A non-empty section is reachable when the source predicate returns false,
as described above for the cpr-transfer source.  The later patch "vfio/migration: don't assume
the device is running on CPR incoming" only adds cpr_is_incoming()
handling in vfio_vmstate_change() and migration init, and adds no
load-side guard.

[ ... ]

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

[ ... ]

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

The same cpr_is_in_progress() question applies to both of these gates on
the cpr-transfer source, since cpr_state_close() has already cleared
cpr_state_file when the VM is stopped.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261023-vfio_v1-v5-0-ba0bf3c6d122@virtuozzo.com?part=25