Re: [PATCH QEMU HCI-8.1 v7 33/35] 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 4 potential issue(s) to consider:
- [Medium] vfio: cpr-transfer blocked for every VFIO device, not only those with saved BAR mmaps
- [Medium] vfio: cpr-transfer blocker skipped for vfio-user, stale BAR mappings reused
- [Low] vfio/region: BAR mappings enter CPR ramblock list walked by cpr-ram-rdonly mprotect
- [Low] vfio: unclaimed BAR mappings preserved by cpr-exec are never cleaned up
--

commit 8715bbf752f1d56051fe39f5336635bb75b99caa
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

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

This patch saves each VFIO BAR mapping in the CPR ramblock list so that
cpr-exec preserves it with MADV_DOEXEC, and the new QEMU reuses it instead
of calling mmap() again. It also blocks cpr-transfer for all VFIO devices
and updates CPR.rst to say so.

> diff --git a/docs/devel/migration/CPR.rst b/docs/devel/migration/CPR.rst
> --- a/docs/devel/migration/CPR.rst
> +++ b/docs/devel/migration/CPR.rst

[ ... ]

> -RAM in place, albeit with new virtual addresses in new QEMU.  Devices
> -and their pinned memory pages are also preserved for VFIO and IOMMUFD.
> +RAM in place, albeit with new virtual addresses in new QEMU.  VFIO and
> +IOMMUFD devices are not supported in this mode: use cpr-exec for them.

This documents the new policy, but only this paragraph was changed. Are
the other cpr-transfer and VFIO descriptions in CPR.rst still accurate?

> 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,
> +    error_setg(&vbasedev->cpr.mmap_blocker,
> +               "cpr-transfer is not supported with VFIO devices; "
> +               "use the cpr-exec mode");
> +    if (migrate_add_blocker_modes(&vbasedev->cpr.mmap_blocker, errp,
> +                                  MIG_MODE_CPR_TRANSFER, -1)) {
> +        return false;
> +    }

[Severity: Medium]
Is it intended that this blocker is installed in vfio_device_attach() for
every VFIO device?

The blocker is added before any backend-specific work and without checking
device type, no_mmap, or whether any region will ever be saved through
vfio_region_cpr_save(). Devices such as vfio-ap, vfio-ccw, x-no-mmap
devices, devices with no mmap-able BARs, and PCI devices whose drivers
allow re-mapping would also lose cpr-transfer.

The commit message gives the NVIDIA vGPU re-map restriction as the
rationale. Would it be narrower to block only when
vfio_region_cpr_save() actually saves a mapping, or only on the PCI path?

The series does not appear to narrow this blocker later. The final state
still has the same unconditional mmap_blocker in vfio_device_attach().

Also, the cpr-transfer specific VFIO code now looks unreachable for
cpr-transfer, but it is left in place: the id_blocker with
MIG_MODE_CPR_TRANSFER in device.c, the blockers in cpr-legacy.c and
cpr-iommufd.c, and the ones in cpr.c. Should that code be cleaned up?

[Severity: Medium]
Does this blocker cover vfio-user devices? vfio_user_pci_realize() in
hw/vfio-user/pci.c calls vfio_device_attach_by_iommu_type() directly:

    if (!vfio_device_attach_by_iommu_type(TYPE_VFIO_IOMMU_USER,
                                          vbasedev->name, vbasedev,
                                          as, errp)) {

That bypasses vfio_device_attach(), so vbasedev->cpr.mmap_blocker is never
installed for vfio-user. hw/vfio-user/ has no other CPR blocker.

vfio-user regions can be mmapped through server-supplied region fds, and
vfio_bar_register() in hw/vfio/pci.c calls vfio_region_mmap(). That now
reaches vfio_region_cpr_save(), which puts the mapping in
cpr_state.ramblocks. That list is serialized for both cpr-exec and
cpr-transfer.

On a cpr-transfer target, vfio_region_cpr_find() looks up
"<vbasedev->name> region N mmaps[i]". For vfio-user, vbasedev->name is
"vfio-user:<sock>", so the name matches the saved entry.

> +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 && ram.length != region->mmaps[index].size) {
[ ... ]
> +    return (void *)(uintptr_t)ram.addr;
> +}

Nothing checks that the incoming mode is cpr-exec here. Can a cpr-transfer
target take the old process's address from this function and pass it to
memory_region_init_ram_device_ptr() in vfio_region_mmap() as if it were a
local mapping? If so, the first guest access to that BAR would touch an
invalid or unrelated address.

Should vfio_region_cpr_find() be gated on cpr_exec_is_incoming(), or should
the vfio-user attach path also install the blocker?

> +static void vfio_region_cpr_save(VFIORegion *region, int index)
> +{
[ ... ]
> +    cpr_save_ramblock(name, (cpr_ramblock_t) { (uintptr_t)addr, size });
> +}

[Severity: Low]
Is it intended that BAR mappings, including read-only ones, now go into the
same ramblock list that cpr_exec_cb() walks for the -cpr-ram-rdonly debug
option?

With that option set, cpr_exec_cb() calls
cpr_walk_ramblock(mark_ram_rdonly), and mark_ram_rw() later calls
qemu_mprotect_rw() on each entry and abort()s on failure:

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

A BAR that vfio_region_mmap() mapped read-only (no
VFIO_REGION_INFO_FLAG_WRITE) would be turned into PROT_READ|PROT_WRITE by
cpr_ram_restore_rw(). If a driver rejects mprotect on these device PFN
mappings, QEMU would abort.

I did not confirm that any driver refuses mprotect. Should these entries be
excluded from the mprotect walks?

[Severity: Low]
Can a preserved BAR mapping that new QEMU never claims be left behind?

The only place a saved mapping is claimed is vfio_region_mmap():

    region->mmaps[i].mmap = vfio_region_cpr_find(region, i);
    if (region->mmaps[i].mmap) {
        goto mapped;
    }

The only cleanup is cpr_delete_ramblock() in vfio_subregion_unmap() and
vfio_region_finalize(), and both run only for regions that were set up on
the new side. If new QEMU never reaches vfio_region_mmap() for a region
(for example a different x-no-mmap setting, a changed region layout, or a
failed realize), the mapping stays mapped with its MADV_DOEXEC mark and its
cpr_state.ramblocks entry.

A later cpr-exec would then apply MADV_DOEXEC to it again through
cpr_walk_ramblock(preserve_ram). Because it is never unmapped, a driver
such as the NVIDIA vGPU driver would still see the BAR as mapped.

Is there a sweep of unclaimed region entries planned? None of the later
patches in the series appear to add one.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261103-vfio_v1-v7-0-4467fdccbbe6@virtuozzo.com?part=33