Re: [PATCH QEMU HCI-8.1 v2 18/24] vfio/migration: don't migrate device state during CPR

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] vfio: migration save/load gated on mode parameter, not CPR in progress
- [Medium] vfio/migration: load_state not gated for CPR, unpatched source still hits live device
- [Medium] vfio/migration: switchover_ack_needed not gated for CPR, hangs migration
- [Medium] vfio/migration: stale multifd_transfer lets save thread stop device during CPR
--

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

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

This patch skips VFIO device state migration in the cpr-transfer and
cpr-exec modes. It adds a vfio_is_active() callback and an early return in
vfio_save_state(), both gated on cpr_incoming_needed(). It also makes the
VM state change handlers return early when cpr_is_in_progress().

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

[ ... ]

> @@ -674,6 +675,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_incoming_needed(NULL)) {
> +        qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
> +        return;
> +    }
> +

[Severity: Medium]
cpr_incoming_needed() only tests migrate_mode(), that is, the configured
mode parameter, and not whether a CPR operation is running.

If the mode parameter is left at cpr-transfer or cpr-exec (for example
after an earlier or failed CPR attempt) and a non-CPR save such as
snapshot-save runs, what happens?

save_snapshot()
  vm_stop(RUN_STATE_SAVE_VM)
  qemu_savevm_state()
    vfio_save_state()
      cpr_incoming_needed(NULL) is true -> only END_OF_STATE is written

In that case vfio_vmstate_change() is gated on cpr_is_in_progress(),
which requires migration_is_running() on the source, and that is false
for savevm. The device is therefore still moved to STOP, but no state is
written, so a later load restores nothing.

The commit message says the stop/cont hook uses cpr_is_in_progress()
because "the mode alone would also skip a plain stop/cont of a VM which
merely has it set". Does the same reasoning apply to vfio_save_state()
and vfio_is_active()? Should they use the same predicate as the
vmstate handlers?

[ ... ]

> @@ -824,7 +835,19 @@ static int vfio_switchover_start(void *opaque)
>      return 0;
>  }
>  
> +static bool vfio_is_active(void *opaque)
> +{
> +    return !cpr_incoming_needed(NULL);
> +}
> +
>  static const SaveVMHandlers savevm_vfio_handlers = {
> +    .is_active = vfio_is_active,
>      .save_prepare = vfio_save_prepare,
>      .save_setup = vfio_save_setup,
>      .save_cleanup = vfio_save_cleanup,

[Severity: Medium]
This isn't limited to the case above: vfio_is_active() also returns false
on the mode parameter alone, so all VFIO save handlers are disabled
whenever the mode is cpr-transfer or cpr-exec, even with no CPR handover
running.

Separately, is the load side covered? .is_active only suppresses setup,
save and pending-estimate handling, while .load_state is still called on
the destination for any section the source sends.

If the source is a QEMU without this patch, vfio_load_state() would still
see these sections:

vfio_load_state()
  case VFIO_MIG_FLAG_DEV_CONFIG_STATE:
    return vfio_load_device_config_state(f, opaque);

That would apply the config to the preserved device and re-program MSI-X,
which is the failure the commit message describes. A DATA_STATE section
would reach vfio_load_buffer()->qemu_file_get_to_fd() with data_fd still
-1, because vfio_load_setup() is skipped, and the incoming migration
would fail.

Is a version requirement on the source intended here, and should
vfio_load_state() have its own CPR check?

[Severity: Medium]
Can this hang a CPR migration when switchover-ack is enabled?

vfio_switchover_ack_needed() still returns vfio_precopy_supported() and
is not gated by is_active. On the destination,
qemu_loadvm_state_switchover_ack_needed() does:

    if (se->ops->switchover_ack_needed(se->opaque)) {
        mis->switchover_ack_pending_num++;
    }

without consulting is_active, so the VFIO device is counted as awaiting
an ACK. The fallback ACK on MIG_CMD_OPEN_RETURN_PATH then does not fire.

On the source, vfio_is_active() now returns false, so vfio_save_iterate()
is never called. That is the only place this is emitted:

    qemu_put_be64(f, VFIO_MIG_FLAG_DEV_INIT_DATA_SENT);

The destination therefore never calls qemu_loadvm_approve_switchover().
migration_can_switchover() then keeps returning s->switchover_acked
(false) while the VM is running:

    if (!runstate_is_running()) {
        return true;
    }
    return s->switchover_acked;

migrate_caps_check() only requires return-path for switchover-ack and
does not reject it in CPR modes. Should vfio_switchover_ack_needed() also
return false in CPR mode, or should the capability be rejected there?

[ ... ]

> @@ -857,6 +880,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 +910,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: Medium]
Can the multifd save thread still move the preserved device to STOP_COPY
during CPR?

qemu_savevm_state_complete_precopy_iterable() spawns
save_complete_precopy_thread for every handler that has one, without
checking .is_active. The thread's only guard is
vfio_multifd_transfer_enabled():

hw/vfio/migration-multifd.c:vfio_multifd_save_complete_precopy_thread() {
    if (!vfio_multifd_transfer_enabled(vbasedev)) {
        /* Nothing to do, vfio_save_complete_precopy() does the transfer. */
        return true;
    }
    ...
    if (vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP_COPY,
                                 VFIO_DEVICE_STATE_STOP, errp)) {

migration->multifd_transfer is written only in vfio_multifd_setup(), and
vfio_multifd_cleanup() does not reset it. With this patch
vfio_save_setup() is skipped in CPR modes, so the flag is no longer
refreshed.

If an earlier non-CPR migration attempt ran vfio_save_setup() with
multifd transfer enabled and then failed or was cancelled, and a later
cpr-transfer or cpr-exec is run in the same process, would the flag still
be true and the thread stop the preserved device and stream its state?

The same flag also gates vfio_switchover_start() without an is_active
check, though that is a destination-side hook in a fresh process and is
less exposed.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261013-vfio_v1-v2-0-433455c0a294@virtuozzo.com?part=18