Re: [PATCH QEMU HCI-8.1 v6 26/34] 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 incoming caches STOP even if the device is RUNNING
--

commit 198a3c353e05a9a3251b20ef27d8db4a03370b16
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 for a
device inherited across CPR. It also makes vfio_vmstate_change() record
RUNNING or STOP on the CPR target, depending on whether the VM is started.

> 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 left it */
> +    if (cpr_is_incoming()) {
> +        vbasedev->migration->device_state = running ?
> +            VFIO_DEVICE_STATE_RUNNING : VFIO_DEVICE_STATE_STOP;
> +        return;
> +    }
> +

[Severity: Medium]
Can this handler leave the cached state wrong when the target stays paused?

This branch only runs when vm_state_notify() fires. On the target,
process_incoming_migration_bh() takes this path when the VM is not
started (for example with -S or autostart off):

    } else {
        runstate_set(RUN_STATE_PAUSED);
    }

runstate_set() only validates and records the transition. It does not call
vm_state_notify(), so vfio_vmstate_change() never runs.

If the source VM was running at CPR time, the device was left RUNNING. This
is because vfio_vmstate_change() and vfio_vmstate_change_prepare() return
early under cpr_is_in_progress() on the source. The target is paused but the
device is still RUNNING.

[ ... ]

> @@ -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]
Does this cache STOP for every CPR-incoming device, even when the source
left it RUNNING?

If the target stays paused, the cache stays STOP while the device is really
RUNNING. The commit message says the device is "STOP if it stays paused",
which doesn't hold when the source VM was running.

Is the real state ever queried, for example through
VFIO_DEVICE_FEATURE_MIG_DEVICE_STATE GET? As far as I can tell, no later
patch in the series does that.

The stale value affects these callers:

hw/vfio/migration.c:vfio_migration_set_state() {
    ...
    if (new_state == migration->device_state) {
        return 0;
    }
    ...
}

Any request matching the stale cached state is skipped without consulting
the kernel.

vfio_device_state_is_running() and vfio_device_state_is_precopy() also
return wrong answers. The dirty-tracking check in hw/vfio/listener.c uses
them:

    (vfio_device_state_is_running(vbasedev) ||
     vfio_device_state_is_precopy(vbasedev))

The vfio_save_setup() switch uses the cached state too.

With the target paused, 'cont' sends a redundant STOP->RUNNING request to a
device that is already RUNNING. This is probably benign.

A live migration out of the paused target before 'cont' still issues the
STOP_COPY request, since STOP_COPY differs from the cached STOP. I did not
find a proven data loss on that path.

The stale cache also persists after the incoming migration finishes.
migration_incoming_state_destroy() clears the incoming mode, so the
cpr_is_incoming() branch in vfio_vmstate_change() can no longer correct the
state when 'cont' runs later.

Could the cached state be taken from the device itself, or could the paused
target case be handled explicitly?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=26