Re: [PATCH QEMU HCI-8.1 v6 32/34] 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 3 potential issue(s) to consider:
- [Medium] vfio: unconditional cpr-transfer blocker added in vfio_device_attach() regresses working mode
- [Medium] vfio: cpr-transfer blocker is skipped for vfio-user devices
- [Low] vfio: BAR mappings in CPR ramblock list lose PROT_READ-only protection under -cpr-ram-rdonly
--

commit 6269c4e3bb1402af4f2335050cc5373497aec6be
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 cpr-exec
carries it over with MADV_DOEXEC and the new QEMU reuses it instead of
calling mmap() again. It also adds a cpr-transfer migration blocker to VFIO
devices in vfio_device_attach().

> 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");
> +    if (migrate_add_blocker_modes(&vbasedev->cpr.mmap_blocker, errp,
> +                                  MIG_MODE_CPR_TRANSFER, -1)) {
> +        return false;
> +    }

[Severity: Medium]
Is this blocker too broad?

It is added in vfio_device_attach() for every VFIO device and every iommu
backend, whether or not a BAR is mmapped. At the base commit, cpr-transfer
with VFIO was not blocked. The vfio_devices_present() check was only
reached under:

    if (migrate_mode() == MIG_MODE_CPR_EXEC && migrate_cpr_is_blocked(errp))

The existing cpr-transfer notifier registration in
vfio_cpr_pci_register_device() and the cpr-legacy and cpr-iommufd transfer
blockers and notifiers also become unreachable for VFIO.

Can this regress cpr-transfer for vfio-pci devices that do not need the BAR
reuse path? Would it be better to add the blocker only when a BAR is
actually mapped?

migrate_add_blocker_modes() can also return -EBUSY here, for example during
savevm or an active cpr-transfer migration. Is it intended that a device
attach or hotplug now fails in those cases?

Also, docs/devel/migration/CPR.rst is not updated to mention the new
restriction.

> +
> +    if (!vfio_device_attach_by_iommu_type(iommu_type, name, vbasedev,
> +                                          as, errp)) {
> +        migrate_del_blocker(&vbasedev->cpr.mmap_blocker);
> +        return false;
> +    }
> +
> +    return true;
>  }

[ ... ]

> 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_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]
Can the BAR mapping lose its original protection here?

vfio_region_mmap() derives prot from VFIO_REGION_INFO_FLAG_READ and
VFIO_REGION_INFO_FLAG_WRITE, so a region with READ but not WRITE is mapped
PROT_READ. This function records only the address and length in the generic
ramblock list.

With -cpr-ram-rdonly, cpr_exec_cb() runs mark_ram_rdonly() over every entry
including the BARs. mark_ram_rw() then calls qemu_mprotect_rw() on every
entry, both in cpr_ram_restore_rw() and in the new QEMU path.

Could a read-only BAR mapping become writable after cpr-exec, or after a
failed cpr-exec rollback, when this debug option is used?

[ ... ]

> @@ -347,7 +392,9 @@ int vfio_region_mmap(VFIORegion *region)
>              ret = -errno;
>              goto no_mmap;
>          }
> +        vfio_region_cpr_save(region, i);

[Severity: Medium]
Is the cpr-transfer blocker skipped for vfio-user devices?

The blocker is added only in the vfio_device_attach() wrapper.
vfio_user_pci_realize() calls the lower-level function directly:

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

It can still reach vfio_bars_register() and then vfio_region_mmap(), and
vfio_user_get_region_info() shows that vfio-user regions can be mmappable.
So vfio_region_cpr_save() records the BAR address in cpr_state.ramblocks
for those devices too.

With cpr-transfer, can vfio_region_cpr_find() in the target process then
return an address that belongs to the source process? That address would be
passed to memory_region_init_ram_device_ptr(). I did not find any cpr or
blocker code under hw/vfio-user/ that would prevent this.

>  
> +mapped:
>          name = g_strdup_printf("%s mmaps[%d]",

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=32