Re: [PATCH QEMU HCI-8.1 v2 19/24] 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:
- [Medium] vfio: log_global_start skipped in CPR mode is never retried when mode changes
--

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

vfio/listener: skip dirty page tracking during CPR

This commit makes vfio_listener_log_global_start() return early without
starting VFIO dirty page tracking when the migration mode is cpr-transfer
or cpr-exec. Stopping dirty tracking is left unconditional.

> diff --git a/hw/vfio/listener.c b/hw/vfio/listener.c
> --- 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_incoming_needed(NULL)) {
> +        return true;
> +    }
> +

[Severity: Medium]
Can this early return leave VFIO dirty tracking permanently off for a later
live migration?

The skip is decided from the current migrate_mode(), which is mutable
state, rather than from the reason global dirty logging was requested.
The listener callback runs only on the first transition of the global
dirty flags:

system/memory.c:memory_global_dirty_log_start() {
    ...
    flags &= ~global_dirty_tracking;
    if (!flags) {
        return true;
    }

    old_flags = global_dirty_tracking;
    global_dirty_tracking |= flags;
    ...
    if (!old_flags) {
        if (!memory_global_dirty_log_do_start(errp)) {
    ...
}

Consider this sequence:

1. While migrate_mode() is cpr-transfer or cpr-exec, some user takes a
   global dirty flag.  calc-dirty-rate in dirty-bitmap mode does this via
   global_dirty_log_change() calling memory_global_dirty_log_start().  A
   CPR migration that is still running, or whose stop is postponed, also
   qualifies.  vfio_listener_log_global_start() returns true and starts
   nothing.

2. The mode parameter is changed through migrate-set-parameters, or
   incoming_mode is reset by cpr_set_incoming_mode(MIG_MODE_NONE) in
   migration cleanup.

3. A normal migration calls
   memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION).  Since the other
   flag is still held, old_flags is non-zero, so
   memory_global_dirty_log_do_start() is not run again and
   vfio_listener_log_global_start() is never called for this migration.

Would VFIO device DMA writes then go unsynced during that live migration,
which could corrupt guest memory on the destination?

The path needs an unusual mode switch while a global dirty flag is held,
but it is reachable through QMP.  The series-final state does not change
hw/vfio/listener.c or system/memory.c, so this is not addressed by later
patches.

As a secondary effect, calc-dirty-rate while in a CPR mode silently
excludes VFIO device writes.  Would it be better to base the decision on
why logging was requested, or to make the start retriable when the mode
changes?

>      if (vfio_container_devices_dirty_tracking_is_supported(bcontainer)) {
>          ret = vfio_devices_dma_logging_start(bcontainer, errp);
>      } else {

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261013-vfio_v1-v2-0-433455c0a294@virtuozzo.com?part=19