Re: [PATCH QEMU HCI-8.1 17/21] 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:
- [Medium] vfio: log_global_stop() gated on live migrate_mode(), can leak dirty tracking
- [Low] vfio: log_global_start/stop skip tracking by configured mode, not by CPR migration
--

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

vfio/listener: skip dirty page tracking during CPR

This patch makes vfio_listener_log_global_start() and
vfio_listener_log_global_stop() return early when cpr_incoming_needed()
is true. The goal is to avoid starting or stopping VFIO dirty tracking
in the cpr-transfer and cpr-exec modes, where guest RAM is preserved in
place.

> 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: Low]
Is the cpr_incoming_needed(NULL) check the right condition here?

cpr_incoming_needed() returns true whenever migrate_mode() is
MIG_MODE_CPR_TRANSFER or MIG_MODE_CPR_EXEC. migrate_mode() falls back to
the persistent parameter when no incoming mode is set:

migration/options.c:migrate_mode() {
    MigMode mode = cpr_get_incoming_mode();

    if (mode == MIG_MODE_NONE) {
        mode = migrate_get_current()->parameters.mode;
    }
    ...
}

So the check is true whenever a CPR mode is merely configured through
migrate-set-parameters, whether or not a CPR migration is running.

The listener callbacks also run for the other global dirty-log clients.
One example is calc-dirty-rate, and dirty-limit reaches them through
global_dirty_log_change() in migration/dirtyrate.c:

ret = memory_global_dirty_log_start(flag, &local_err);

With a CPR mode configured, those clients no longer start VFIO container
or device dirty tracking. Because vfio_log_sync_needed() requires
vfio_container_dirty_tracking_is_started(), log_sync is skipped too. Can
the dirty-rate and dirty-limit results then undercount writes made by VFIO
devices?

Would a check tied to a running CPR migration, or to the
GLOBAL_DIRTY_MIGRATION flag, be more precise? The commit message says
"during CPR" but does not mention this wider scope.

Also, cpr_incoming_needed() is the incoming-side VMState predicate, and
here it is reused for a source-side decision. That is not obvious to a
reader.

> @@ -1031,6 +1037,10 @@ static void vfio_listener_log_global_stop(MemoryListener *listener)
>      Error *local_err = NULL;
>      int ret = 0;
>  
> +    if (cpr_incoming_needed(NULL)) {
> +        return;
> +    }
> +

[Severity: Medium]
Can this early return leave VFIO dirty tracking running?

The stop is skipped based on the current migration mode, not on whether
tracking was actually started. Consider this sequence:

memory_global_dirty_log_start()
  vfio_listener_log_global_start()   /* mode is normal, tracking starts */

qmp migrate-set-parameters mode=cpr-transfer (or cpr-exec)
  migrate_params_apply()
    s->parameters.mode = params->mode;   /* no check for active tracking */

memory_global_dirty_log_do_stop()
  vfio_listener_log_global_stop()
    cpr_incoming_needed(NULL)  /* now true */
    return;

Neither vfio_devices_dma_logging_stop() nor
vfio_container_set_dirty_page_tracking(bcontainer, false, ...) runs.
vbasedev->dirty_tracking and the container dirty_pages_started state stay
set, and DMA logging stays enabled on the device. That is the vGPU
situation the commit message wants to avoid.

The window can be wide. memory_global_dirty_log_stop() postpones the stop
when the VM is not running:

if (!runstate_is_running()) {
    /* Postpone the dirty log stop, e.g., to when VM starts again */

so the mode can also change during that gap.

The reverse case (start skipped under CPR, stop run under a normal mode)
looks harmless, since vfio_devices_dma_logging_stop() checks
vbasedev->dirty_tracking and vfio_container_set_dirty_page_tracking()
returns early when the state already matches.

Could the cpr_incoming_needed() check be dropped from
vfio_listener_log_global_stop(), given that the stop helpers are
idempotent? Alternatively, could start record whether tracking was
actually started, with stop testing that state?

I did not find changes to hw/vfio/listener.c, migration/options.c,
system/memory.c or migration/cpr.c later in the series that address this.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261009-vfio_v1-v1-0-2ae91d5b8048@virtuozzo.com?part=17