Re: [PATCH QEMU HCI-8.1 v3 22/28] vfio/listener: skip dirty page tracking during CPR

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] vfio: CPR skip in log_global_start() is never retried when another dirty-tracking flag is active
--

commit a7865175c300cb526642364e7a12bfbf13a29863
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 tracking while a CPR migration (cpr-transfer or
cpr-exec) is in progress. The gate is cpr_is_in_progress(), so snapshots
still track dirty pages. Stopping stays 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: High]
Can this early return leave VFIO dirty tracking permanently off for a later
normal migration?

The listener hook runs only when the first dirty-tracking flag is set:

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)) {
    ...
}

Consider this sequence:

1. A CPR migration starts as the first consumer.
   memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION) sees
   old_flags == 0, so vfio_listener_log_global_start() runs. Because
   cpr_is_in_progress() is true, it returns true without starting tracking.

2. Another consumer starts, for example calculate-dirty-rate
   (GLOBAL_DIRTY_DIRTY_RATE) or the dirty limit (GLOBAL_DIRTY_LIMIT).
   old_flags is now non-zero, so no listener is called.

3. The CPR migration fails or is cancelled. Only the migration flag is
   cleared in memory_global_dirty_log_do_stop():

    global_dirty_tracking &= ~flags;
    ...
    if (!global_dirty_tracking) {
        ...
        MEMORY_LISTENER_CALL_GLOBAL(log_global_stop, Reverse);

   global_dirty_tracking stays non-zero, so no listener hook runs.

4. The user sets mode=normal and migrates.
   memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION) again sees
   old_flags != 0 and skips the listeners, so VFIO tracking is never
   started.

5. vfio_log_sync_needed() then returns false:

    if (!vfio_container_dirty_tracking_is_started(bcontainer)) {
        return false;
    }

   Both vbasedev->dirty_tracking and bcontainer->dirty_pages_started are
   still false, so pages dirtied by device DMA are not synced.

The destination could then receive stale guest RAM during the later normal
migration. Before this patch, VFIO tracking always started with the first
consumer, so this state was not reachable.

Nothing later in the series touches hw/vfio/listener.c or system/memory.c,
so the series does not restart tracking in this case.

Could the skip be retried, or could VFIO tracking be started when a later
consumer or a later non-CPR migration needs it?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261016-vfio_v1-v3-0-47487cb5a8e5@virtuozzo.com?part=22