Re: [PATCH QEMU HCI-8.1 v5 27/36] vfio/listener: skip dirty page tracking during CPR

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] vfio: CPR skip in log_global_start() leaves VFIO dirty tracking off if dirty log stays on
- [Medium] vfio: CPR dirty-tracking skip is ineffective on the cpr-transfer source
--

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

vfio/listener: skip dirty page tracking during CPR

This patch makes vfio_listener_log_global_start() return early without
starting VFIO dirty page tracking while cpr_is_in_progress() is true. The
aim is to avoid leaving DMA logging running after cpr-transfer or cpr-exec.
Stopping tracking is left unconditional.

> diff --git a/hw/vfio/listener.c b/hw/vfio/listener.c
> index afcf518f567f..1270b9d5f3aa 100644
> --- a/hw/vfio/listener.c
> +++ b/hw/vfio/listener.c

[ ... ]

> @@ -1012,6 +1013,11 @@ static bool vfio_listener_log_global_start(MemoryListener *listener,
>                                               listener);
>      bool ret;
>  
> +    /* Preserved devices keep DMA-ing into preserved RAM, nothing to track */
> +    if (cpr_is_in_progress()) {
> +        return true;
> +    }
> +

[Severity: Medium]
Is this skip effective on the cpr-transfer source?

On the source, cpr_is_in_progress() reduces to this check, because
cpr_is_incoming() is false there:

migration/cpr.c:cpr_is_in_progress() {
    ...
    return cpr_state_file != NULL;
}

For cpr-transfer, qmp_migrate() only arms a HUP watch after
cpr_state_save(). When the target closes the CPR socket, the main loop
runs qmp_migrate_finish_cb() with the BQL held:

migration/migration.c:qmp_migrate_finish_cb() {
    ...
    qmp_migrate_finish(addr, NULL);

    cpr_state_close();
}

So cpr_state_file is already NULL by the time the migration thread gets
the BQL and runs setup:

migration_thread()
    bql_lock();
    ret = qemu_savevm_state_setup(s->to_dst_file, &local_err);
    bql_unlock();

The call chain from there is:

qemu_savevm_state_setup()
  ram_init_bitmaps()
    memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, errp)
      vfio_listener_log_global_start()

When that chain reaches the new check, cpr_is_in_progress() returns false.
VFIO dirty tracking would then still start on the cpr-transfer source, even
though the commit message says it is skipped for cpr-transfer. cpr-exec
does not appear to be affected.

I did not find a later change in the series that alters the timing of
cpr_state_close() or the predicate. Could the check be based on something
that is still set when setup runs on the cpr-transfer source?

[Severity: High]
Can VFIO dirty tracking stay off after a failed CPR when another dirty log
user is active?

The early return sets neither vbasedev->dirty_tracking nor
bcontainer->dirty_pages_started. The memory core calls the listeners only on
the transition from no dirty log users to one:

system/memory.c:memory_global_dirty_log_start() {
    old_flags = global_dirty_tracking;
    global_dirty_tracking |= flags;
    ...
    if (!old_flags) {
        if (!memory_global_dirty_log_do_start(errp)) {
    ...
}

A possible sequence is:

1. A CPR migration starts and global_dirty_tracking goes from 0 to
   GLOBAL_DIRTY_MIGRATION. vfio_listener_log_global_start() returns early.
2. calc-dirty-rate starts and adds GLOBAL_DIRTY_DIRTY_RATE.
3. The CPR fails or is cancelled. Only the migration flag is cleared, so
   global_dirty_tracking stays non-zero and log_global_stop is not called.
4. A later live migration calls
   memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION). old_flags is
   non-zero, so VFIO log_global_start() is not invoked again.

After that, vfio_log_sync_needed() returns false:

hw/vfio/listener.c:vfio_log_sync_needed() {
    ...
    if (!vfio_container_dirty_tracking_is_started(bcontainer)) {
        return false;
    }
    ...
}

The VFIO dirty bitmap sync is then skipped without any warning. Could that
cause a later live migration to send stale guest RAM for pages dirtied by
the device?

The commit message acknowledges this as a "Known gap", but I did not find
anything in the series that restarts tracking. Would it be possible to
restart VFIO tracking when the CPR ends or fails, or to avoid the early
return in a way that does not depend on the first-user transition?

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