Re: [PATCH QEMU HCI-8.1 v5 26/36] vfio/migration: don't assume the device is running on CPR incoming

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] vfio/migration: cached STOP on CPR incoming is wrong if target stays paused
--

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

vfio/migration: don't assume the device is running on CPR incoming

This patch makes vfio_migration_init() cache STOP instead of RUNNING when
CPR is incoming. It also makes vfio_vmstate_change() record RUNNING or STOP
in the cached device_state on the CPR target, instead of leaving the device
untouched. The goal is to stop 'cont' from skipping the ioctl when the
source VM was paused.

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

[ ... ]

> @@ -914,6 +914,14 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
>      Error *local_err = NULL;
>      int ret;
>  
> +    /* CPR target only: the device kept its state, note where the VM leaves it */
> +    if (cpr_is_incoming()) {
> +        vbasedev->migration->device_state = running ?
> +            VFIO_DEVICE_STATE_RUNNING : VFIO_DEVICE_STATE_STOP;
> +        return;
> +    }
> +

[Severity: Low]
Can the cached STOP be left stale here when the target stays paused?

If the source VM was running, CPR skips the device stop under
cpr_is_in_progress(), so the kernel device stays RUNNING. On the target,
vfio_migration_init() then caches STOP unconditionally:

    migration->device_state = cpr_is_incoming() ? VFIO_DEVICE_STATE_STOP :
                                                  VFIO_DEVICE_STATE_RUNNING;

If the target stays paused (-S, autostart=false, or a block-activate
failure), process_incoming_migration_bh() takes this path:

migration/migration.c:process_incoming_migration_bh() {
    ...
    } else {
        runstate_set(RUN_STATE_PAUSED);
    }
    ...
}

runstate_set() does not call vm_state_notify(), so vfio_vmstate_change()
is never invoked. The new cpr_is_incoming() block above is therefore never
reached, and the cache stays at STOP while the device is really RUNNING.

The commit message says "STOP if it stays paused" and "At CPR the device
can only be in one of the two". Is that accurate for a running source
whose target stays paused?

The cached state is also consumed elsewhere, for example:

hw/vfio/listener.c:vfio_log_sync_needed() {
    ...
    if (vbasedev->pre_copy_dirty_page_tracking == ON_OFF_AUTO_OFF &&
        (vfio_device_state_is_running(vbasedev) ||
         vfio_device_state_is_precopy(vbasedev))) {
        return false;
    }
    ...
}

Could a cached STOP on a really RUNNING device give the wrong dirty-sync
decision there, if the still-paused target is migrated out again?

A later 'cont' would also issue a redundant STOP->RUNNING transition, and
vfio_device_state_is_running() would return the wrong answer until then.
Would it be possible to determine the real device state on CPR incoming,
rather than assuming STOP?

> +    /* CPR source only: the device keeps running across the exec */
>      if (cpr_is_in_progress()) {
>          return;
>      }

[ ... ]

> @@ -1039,7 +1047,9 @@ static int vfio_migration_init(VFIODevice *vbasedev)
> -    migration->device_state = VFIO_DEVICE_STATE_RUNNING;
> +    /* A device inherited across CPR is in STOP unless the VM gets started */
> +    migration->device_state = cpr_is_incoming() ? VFIO_DEVICE_STATE_STOP :
> +                                                  VFIO_DEVICE_STATE_RUNNING;

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