Re: [PATCH QEMU HCI-8.1 v2 23/24] VZ: vfio/region: preserve BAR mappings across cpr-exec #VSTOR-103860

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] vfio: vfio_region_cpr_find() exits QEMU for lazily mmapped display region after incoming mode reset
- [Medium] vfio: vfio_region_mmap() adds cpr-transfer blocker with error_abort
- [Medium] vfio: cpr-exec new QEMU exit(1)s when a BAR was not mmapped by old QEMU
- [Medium] vfio: display region CPR entry leaked, vfio_region_exit() runs after unparent
- [Low] vfio: cpr-transfer blocker added before mmap and kept after failure
- [Low] vfio: cpr-transfer is blocked for every VFIO device with mmapped regions
--

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

VZ: vfio/region: preserve BAR mappings across cpr-exec #VSTOR-103860

This patch saves each VFIO BAR mmap in the CPR ramblock list, so that the
new QEMU can reuse the mapping after cpr-exec instead of calling mmap()
again. It also adds a cpr-transfer blocker for devices with mmapped
regions.

> diff --git a/hw/vfio/device.c b/hw/vfio/device.c
> index 5a14a7d5c0a2e..88b9e8752cd93 100644
> --- a/hw/vfio/device.c
> +++ b/hw/vfio/device.c
> @@ -514,6 +514,7 @@ void vfio_device_unprepare(VFIODevice *vbasedev)
>  
>      g_clear_pointer(&vbasedev->reginfo, g_free);
>      g_clear_pointer(&vbasedev->region_fds, g_free);
> +    migrate_del_blocker(&vbasedev->cpr.mmap_blocker);
>  
>      QLIST_REMOVE(vbasedev, container_next);
>      QLIST_REMOVE(vbasedev, global_next);

[ ... ]

> diff --git a/hw/vfio/region.c b/hw/vfio/region.c
> index 1deaef12fd0b8..fce09a0d17b1a 100644
> --- a/hw/vfio/region.c
> +++ b/hw/vfio/region.c

[ ... ]

> +static char *vfio_region_cpr_name(VFIORegion *region, int index)
> +{
> +    g_autofree char *path = qdev_get_dev_path(region->vbasedev->dev);
> +    return g_strdup_printf("%s%s%s mmaps[%d]", path ?: "", path ? "/" : "",
> +                           memory_region_name(region->mem), index);
> +}

[Severity: Medium]
The name built here depends on qdev_get_dev_path(), which returns NULL once
device_unparent() has cleared dev->parent_bus. Can the save-time and
delete-time names diverge for the display region?

The display region is mmapped lazily:

vfio_display_region_update()
  vfio_region_setup()
  vfio_region_mmap()
    vfio_region_cpr_save()   /* saved as "<path>/display mmaps[0]" */

On hot-unplug, vfio_exitfn() runs vfio_pci_bars_exit(), which covers only
the BAR regions. The display region is torn down later, from
vfio_pci_finalize():

vfio_pci_finalize()
  vfio_pci_put_device()
    vfio_display_finalize()
      vfio_display_region_exit()
        vfio_region_exit()

By then device_unparent() has run, so the name becomes "display mmaps[0]"
without the path prefix. Then cpr_delete_ramblock() in vfio_region_exit()
finds nothing, and the saved entry stays while vfio_region_finalize()
munmap()s the region.

Could this leave a stale entry that makes a later hot-replug of a device
with the same path hit exit(1) in vfio_region_cpr_find(), or makes a later
cpr-exec try MADV_DOEXEC on an unmapped range?

> +/* Same checks as for RAM blocks in ram_block_add() */
> +static void *vfio_region_cpr_find(VFIORegion *region, int index)
> +{
> +    g_autofree char *name = vfio_region_cpr_name(region, index);
> +    cpr_ramblock_t ram = cpr_find_ramblock(name);
> +
> +    if (cpr_exec_is_incoming()) {
> +        if (!ram.addr || !ram.length) {
> +            error_report("CPR mode=exec is requested, but mapping %s can't "
> +                         "be reused: addr=%p length=%zu. Aborting",
> +                         name, (void *)ram.addr, ram.length);
> +            exit(1);
> +        }

[Severity: Medium]
Can this exit(1) be reached for a region that old QEMU legitimately could
not mmap?

In old QEMU, vfio_bar_register() treats a vfio_region_mmap() failure as
non-fatal:

    if (vfio_region_mmap(&bar->region)) {
        error_report("Failed to mmap %s BAR %d. Performance may be slow",
                     vdev->vbasedev.name, nr);
    }

The no_mmap path in vfio_region_mmap() calls vfio_subregion_unmap(), which
calls cpr_delete_ramblock(). A failed region therefore has no CPR entry.

The only blocker added by this patch is mmap_blocker, which applies to
cpr-transfer only. Nothing blocks cpr-exec in this state, so the new QEMU
runs vfio_region_mmap() again. Then vfio_region_cpr_find() sees
cpr_exec_is_incoming() true and no entry, and exits.

The comment says this mirrors ram_block_add(), but RAM blocks are always
allocated while BAR mmaps are optional. Should this fall back to mmap() or
the slow path when no entry exists, or should cpr-exec be blocked when a
region could not be mapped?

> +        assert(ram.length == region->mmaps[index].size);
> +    } else if (ram.addr || ram.length) {
> +        error_report("CPR mode=exec wasn't requested, but mapping %s was "
> +                     "saved: addr=%p length=%zu. Aborting",
> +                     name, (void *)ram.addr, ram.length);
> +        exit(1);
> +    }

[Severity: High]
Can this second branch kill QEMU after a cpr-exec that had otherwise
succeeded, for the lazily mapped display region?

The sequence would be:

1. Old QEMU maps the display buffer from vfio_display_region_update(), and
   vfio_region_cpr_save() adds "<path>/display mmaps[0]" to the ramblock
   list. cpr_exec_cb() preserves it.

2. New QEMU loads the entry in cpr_state_load(), which also sets
   incoming_mode to MIG_MODE_CPR_EXEC. Nothing maps the display region at
   realize time, since vfio_display_region_init() only registers the
   gfx_update callback.

3. migration_incoming_state_destroy() calls
   cpr_set_incoming_mode(MIG_MODE_NONE).

4. The first gfx_update with an active plane after that calls
   vfio_region_setup() and vfio_region_mmap(), and then
   vfio_region_cpr_find().

The saved entry is still present, because only vfio_subregion_unmap() and
vfio_region_exit() delete it. cpr_exec_is_incoming() is now false, so the
"wasn't requested, but mapping %s was saved" branch calls exit(1).

The check was copied from ram_block_add(), which runs at realize time
while the incoming mode is still set. Is it valid for a mapping that can be
created after the mode is reset?

Even without the exit, the preserved mapping would be neither reused nor
released.

[ ... ]

> @@ -307,6 +352,19 @@ int vfio_region_mmap(VFIORegion *region)
>          return 0;
>      }
>  
> +    /*
> +     * Only cpr-exec keeps the address space, a cpr-transfer target would
> +     * have to map the regions anew, which some drivers (NVIDIA vGPU) refuse
> +     * while the old mappings exist.  Not supported.
> +     */
> +    if (!region->vbasedev->cpr.mmap_blocker) {
> +        error_setg(&region->vbasedev->cpr.mmap_blocker,
> +                   "cpr-transfer is not supported with mmapped VFIO regions "
> +                   "(%s); use the cpr-exec mode", region->vbasedev->name);
> +        migrate_add_blocker_modes(&region->vbasedev->cpr.mmap_blocker,
> +                                  &error_abort, MIG_MODE_CPR_TRANSFER, -1);
> +    }

[Severity: Medium]
Can passing &error_abort here abort QEMU at runtime?

migrate_add_blocker_modes() calls is_busy(), which fails with -EBUSY and
fills errp when the following holds:

    if (runstate_check(RUN_STATE_SAVE_VM) || migration_is_running()) {
        error_propagate_prepend(errp, *reasonp, ...

vfio_region_mmap() is not only a realize-time call. The gfx_update
callback reaches it lazily:

vfio_display_region_update()
  vfio_region_mmap(&dpy->region.buffer)

For a device whose BARs have no mmap, or whose BAR mmaps failed, the first
blocker registration happens at runtime. If a migration or snapshot is
running at that moment, error_abort turns the -EBUSY into abort().

Since is_busy() does not depend on the mode, would this also hit ordinary
non-CPR migrations? Could the -EBUSY be handled instead of aborting?

[Severity: Low]
Is the blocker installed too early here? It is added before the mmap loop
and is not removed on the no_mmap error path, nor by vfio_subregion_unmap()
or vfio_region_exit().

The only removal is migrate_del_blocker() in vfio_device_unprepare(). The
commit message says the block applies "for devices with mmapped regions",
but a device whose first mmap() fails, or whose mappings were all undone,
still blocks cpr-transfer until vfio_device_unprepare().

[Severity: Low]
Is the blocker intended to apply to every device with mmapped regions?

It is registered unconditionally for any region with nr_mmaps > 0, so
nearly every vfio-pci device blocks cpr-transfer. Before this patch, VFIO
devices reached cpr-transfer through no such check, since
vfio_devices_present() was only consulted through
migration_is_blocked()->migrate_cpr_is_blocked() when migrate_mode() ==
MIG_MODE_CPR_EXEC.

The commit message gives the NVIDIA vGPU case as the reason, but the code
does not narrow the block to drivers that refuse to remap. Could the
blocker be limited to the affected drivers or devices?

[ ... ]

> @@ -314,6 +372,11 @@ int vfio_region_mmap(VFIORegion *region)
>          size_t align = MIN(1ULL << ctz64(region->mmaps[i].size), 1 * GiB);
>          void *map_base, *map_align;
>  
> +        region->mmaps[i].mmap = vfio_region_cpr_find(region, i);
> +        if (region->mmaps[i].mmap) {
> +            goto mapped;
> +        }
> +

[ ... ]

> @@ -410,6 +475,13 @@ void vfio_region_exit(VFIORegion *region)
>  
>      for (i = 0; i < region->nr_mmaps; i++) {
>          if (region->mmaps[i].mmap) {
> +            /*
> +             * The mapping is unmapped by vfio_region_finalize(), but by
> +             * then the device is unparented and its path, part of the CPR
> +             * name, is gone.  So drop the CPR entry here.
> +             */
> +            g_autofree char *cpr_name = vfio_region_cpr_name(region, i);
> +            cpr_delete_ramblock(cpr_name);
>              memory_region_del_subregion(region->mem, &region->mmaps[i].mem);
>          }
>      }

This comment is correct for the BAR path through vfio_exitfn(), but is the
same ordering guaranteed when vfio_region_exit() is reached from
vfio_display_finalize()? As described above, the device is already
unparented there.

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