Re: [PATCH QEMU HCI-8.1 v7 27/35] 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:
- [Medium] vfio/migration: CPR target caches STOP for a device the source left RUNNING
--

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

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

This patch changes the CPR target so that vfio_migration_init() caches
STOP instead of RUNNING for the device. The VM state handler then
updates the cached state to RUNNING or STOP when the target VM starts
or stays paused.

> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index b142c9f228911..04d0de7cef0ac 100644
> --- 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 left it */
> +    if (cpr_is_incoming()) {
> +        vbasedev->migration->device_state = running ?
> +            VFIO_DEVICE_STATE_RUNNING : VFIO_DEVICE_STATE_STOP;
> +        return;
> +    }
> +
> +    /* 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)
>      vbasedev->migration = g_new0(VFIOMigration, 1);
>      migration = vbasedev->migration;
>      migration->vbasedev = 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;

[Severity: Medium]
Can this cache a wrong state when the CPR source VM was running?

On the source, migration_stop_vm() calls
vm_stop_force_state(RUN_STATE_FINISH_MIGRATE). Since
cpr_is_in_progress() is true, both vfio_vmstate_change() and
vfio_vmstate_change_prepare() return early, so the device stays
RUNNING in the kernel.

On the target, vfio_migration_init() runs while cpr_is_incoming() is
true and caches STOP unconditionally.

The cache is corrected only if vfio_vmstate_change() runs. That
requires vm_start() to be called while the incoming mode is still set.
In process_incoming_migration_bh() the paused path does not run VM
change-state handlers:

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

This path is taken with -S, with autostart off, or when
migration_block_activate() fails. In those cases the new branch in
vfio_vmstate_change() never runs. The cache stays STOP while the device
is RUNNING in the kernel.

Once migration_incoming_state_destroy() clears the incoming mode, later
transitions rely on that cache. In vfio_migration_set_state():

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

Could a later request for STOP be skipped here, leaving a running
device that is believed to be stopped? The cached value is also read by
vfio_device_state_is_running() and by the switch in vfio_save_setup().

The commit message says the device is cached as "STOP if it stays
paused" and that "At CPR the device can only be in one of the two."
Is that accurate for a source that was running? The patch infers the
device state from the target's run state, not from the source's run
state.

Would it be possible to carry the real device state from the source, or
to query it, instead of inferring it from the target's run state?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261103-vfio_v1-v7-0-4467fdccbbe6@virtuozzo.com?part=27