Re: [PATCH QEMU HCI-8.1 v4 29/31] 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 5 potential issue(s) to consider:
- [Medium] vfio: vfio_device_attach() now fails hotplug while migration/snapshot runs
- [Medium] vfio: unconditional cpr-transfer blocker in vfio_device_attach() for all VFIO devices
- [Low] vfio/region: vfio_region_cpr_find() aborts on mmap size mismatch via assert()
- [Low] vfio: BAR mappings joined to CPR ramblock list hit abort()-on-failure mprotect in cpr-ram-rdonly mode
- [Low] vfio: preserved display-region mapping not released if never remapped after cpr-exec
--

commit af431a92f0ea91854f19f143a605e436ef6246ae
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
cpr_exec_cb() marks it with MADV_DOEXEC. New QEMU then reuses the saved
mapping instead of calling mmap() again. It also adds a blocker that stops
cpr-transfer for all VFIO devices.

> diff --git a/hw/vfio/device.c b/hw/vfio/device.c
> --- a/hw/vfio/device.c
> +++ b/hw/vfio/device.c
> @@ -464,12 +464,32 @@ bool vfio_device_attach(char *name, VFIODevice *vbasedev,
>                               TYPE_VFIO_IOMMU_IOMMUFD :
>                               TYPE_VFIO_IOMMU_LEGACY;
>  
> -    return vfio_device_attach_by_iommu_type(iommu_type, name, vbasedev,
> -                                            as, errp);
> +    /*
> +     * Only cpr-exec keeps the address space and so the region mappings,
> +     * see vfio_region_mmap().  A cpr-transfer target would have to map the
> +     * regions anew, which some drivers (NVIDIA vGPU) refuse.
> +     */
> +    error_setg(&vbasedev->cpr.mmap_blocker,
> +               "cpr-transfer is not supported with VFIO devices; "
> +               "use the cpr-exec mode");

[Severity: Medium]
The blocker is installed here for every VFIO device, before no_mmap, the
device type or the region state is looked at. Does this also disable
cpr-transfer for devices that never map a BAR (no_mmap=on, vfio-ap,
vfio-ccw) and for drivers that accept a re-mmap?

The stated reason is the NVIDIA vGPU re-mmap problem, which only exists
once vfio_region_cpr_save() has actually saved a mapping. Could the
blocker be added only when a BAR mapping is saved, or be made opt-in per
device?

Also, docs/devel/migration/CPR.rst still says in the cpr-transfer mode
section that VFIO devices are supported:

    This mode supports VFIO devices provided the user first puts the

Should the documentation be updated along with this change?

> +    if (migrate_add_blocker_modes(&vbasedev->cpr.mmap_blocker, errp,
> +                                  MIG_MODE_CPR_TRANSFER, -1)) {
> +        return false;
> +    }

[Severity: Medium]
Can this make vfio_device_attach() fail during a migration or snapshot,
whatever the CPR mode? migrate_add_blocker_modes() runs is_busy() no
matter which mode mask is passed:

vfio_device_attach()
  migrate_add_blocker_modes()
    is_busy()
      if (runstate_check(RUN_STATE_SAVE_VM) || migration_is_running()) {
          error_propagate_prepend(errp, *reasonp,
              "disallowing migration blocker (migration/snapshot in progress) for: ");
          ...
          return true;
      }

This returns -EBUSY, so realizing or hotplugging any VFIO device (PCI,
AP, CCW; legacy or iommufd) while a migration is running or
RUN_STATE_SAVE_VM is set would now fail. Is that intended?

The later patches in the series do not change this call.

> diff --git a/hw/vfio/region.c b/hw/vfio/region.c
> --- a/hw/vfio/region.c
> +++ b/hw/vfio/region.c

[ ... ]

> +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 (ram.addr) {
> +        assert(ram.length == region->mmaps[index].size);
> +    }

[Severity: Low]
If the preserved length differs from the size that new QEMU computes for
this mmap slot, this aborts after old QEMU has already exec'd. Could this
report an error with error_report() and exit(1), as the ramblock reuse
path in system/physmem.c does under cpr_exec_is_incoming()?

Only the length is compared here. Offset, prot and fd_offset are not
validated, so a layout change that keeps the same length would silently
reuse a mapping of the wrong window. Is a check of those intended?

[ ... ]

> +static void vfio_region_cpr_save(VFIORegion *region, int index)
> +{
> +    g_autofree char *name = vfio_region_cpr_name(region, index);
> +    void *addr = region->mmaps[index].mmap;
> +    size_t size = region->mmaps[index].size;
> +
> +    cpr_save_ramblock(name, (cpr_ramblock_t) { (uintptr_t)addr, size });
> +}

[Severity: Low]
This puts device BAR mappings into the list that migration/cpr-exec.c
walks. vfio_region_mmap() can map a BAR PROT_READ only, depending on
region->flags. With the debug-only -cpr-ram-rdonly option, cpr_exec_cb()
calls mark_ram_rdonly() on every entry. On the exec-failure path, and when
cpr_exec_is_incoming() && cpr_ram_rdonly, mark_ram_rw() requests RW on
every entry:

    if (qemu_mprotect_rw((void *) ram.addr, ram.length)) {
        error_report("qemu_mprotect_rw() failed on memory region %s:"
        ...
        abort();
    }

Can this abort() QEMU if a driver refuses mprotect on a shared PFN
mapping, for example by clearing VM_MAYWRITE? Would it also make a
read-only BAR mapping writable?

I could not confirm the driver behaviour from the repository, so this is
a question about the interaction.

[ ... ]

> @@ -427,6 +470,8 @@ void vfio_region_finalize(VFIORegion *region)
>  
>      for (i = 0; i < region->nr_mmaps; i++) {
>          if (region->mmaps[i].mmap) {
> +            g_autofree char *cpr_name = vfio_region_cpr_name(region, i);
> +            cpr_delete_ramblock(cpr_name);
>              munmap(region->mmaps[i].mmap, region->mmaps[i].size);
>          }
>      }

[Severity: Low]
Can the preserved entry for the lazily mapped display region be leaked
here? After cpr-exec, if new QEMU has not yet set up and mapped the
display region, vfio_region_setup() and vfio_region_mmap() are never
called for it. region->mem and region->mmaps[i].mmap stay NULL:

hw/vfio/display.c:vfio_display_region_exit() {
    if (!dpy->region.buffer.size) {
        return;
    }

    vfio_region_exit(&dpy->region.buffer);
    vfio_region_finalize(&dpy->region.buffer);
}

Here the early return, and the early return in vfio_region_finalize() when
!region->mem, skip the cpr_delete_ramblock() call. The entry then stays
in the list, so cpr_exec_cb() marks the VMA with MADV_DOEXEC again on the
next cpr-exec. The VMA also keeps a reference on the device file.

A re-added device with the same name could also pick the stale mapping up
through vfio_region_cpr_find(). Should the preserved entry be released
when the region is never remapped?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=29