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

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] vfio/migration: CPR gating via cpr_incoming_needed() reads persistent parameters.mode
--

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

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

This patch skips VFIO device state migration when the migration mode is
cpr-transfer or cpr-exec. It gates the save_state handler, adds an
is_active callback, and returns early from the two vmstate-change
handlers, all using cpr_incoming_needed().

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

[ ... ]

> @@ -857,6 +880,10 @@ static void vfio_vmstate_change_prepare(void *opaque, bool running,
>      Error *local_err = NULL;
>      int ret;
>  
> +    if (cpr_incoming_needed(NULL)) {
> +        return;
> +    }
> +
>      new_state = migration->device_state == VFIO_DEVICE_STATE_PRE_COPY ?
>                      VFIO_DEVICE_STATE_PRE_COPY_P2P :
>                      VFIO_DEVICE_STATE_RUNNING_P2P;

[Severity: Medium]
Can this early return skip the P2P quiesce step outside of an in-flight
CPR?

cpr_incoming_needed() only checks the configured mode:

migration/cpr.c:cpr_incoming_needed() {
    MigMode mode = migrate_mode();
    return mode == MIG_MODE_CPR_TRANSFER || mode == MIG_MODE_CPR_EXEC;
}

migrate_mode() uses the incoming mode when set, and otherwise falls back
to the persistent parameters.mode:

migration/options.c:migrate_mode() {
    MigMode mode = cpr_get_incoming_mode();

    if (mode == MIG_MODE_NONE) {
        mode = migrate_get_current()->parameters.mode;
    }
    ...
}

On the destination, vm_start() runs in process_incoming_migration_bh()
before migration_incoming_state_destroy() calls
cpr_set_incoming_mode(MIG_MODE_NONE). After that, only parameters.mode is
seen, and it stays cpr-transfer or cpr-exec. A later plain stop or cont on
the destination would then return early here and in vfio_vmstate_change().

In that case the device is never moved to STOP and could keep running,
and possibly DMAing, while the guest is paused.

The same applies if the mode is changed between a stop and the matching
start. It also applies if a normal migration is started while the VM is
already stopped, where the device would never be moved to STOP_COPY.

The gate works in the intended CPR flow. On the source the mode is set
before the migrate command, and on the destination incoming_mode is still
set during vm_start(). The concern is that these handlers run on every
stop and cont, not only during a CPR.

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

[Severity: Medium]
Does this early return leave the tracked migration->device_state out of
sync with the VM runstate?

The same persistent parameters.mode dependence applies here as in
vfio_vmstate_change_prepare(). A stop or cont while parameters.mode is
still cpr-transfer or cpr-exec would skip the RUNNING/STOP transition.

Is there a per-migration indicator that could be used instead of
cpr_incoming_needed()?

The commit message says "Use cpr_incoming_needed(), which tells exactly
when the device is preserved by CPR, both on the old and the new QEMU
side." Is this accurate?

cpr_incoming_needed() is used elsewhere for vmstate .needed callbacks,
which are evaluated only during a CPR save or load. Here it is used in
handlers that run on every stop and cont.

The series-final state (patches 17 to 21) leaves these gating sites
unchanged: hw/vfio/migration.c in vfio_save_state(), vfio_is_active(),
vfio_vmstate_change_prepare() and vfio_vmstate_change().

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261009-vfio_v1-v1-0-2ae91d5b8048@virtuozzo.com?part=16