Re: [PATCH QEMU HCI-8.1 v4 31/31] VZ: vfio/container: lift the CPR blocker for mdev devices #VSTOR-103860

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] vfio: mdev CPR blocker dropped; remaining UPDATE_VADDR check only covers the first group
- [Low] vfio: CPR mdev blocker removed for all mdev types, including vfio-ap and vfio-ccw
--

commit 7cbd800afda6ddbe0b7b6ef4fa513b3145de2ff4
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

VZ: vfio/container: lift the CPR blocker for mdev devices #VSTOR-103860

This patch removes the per-device CPR migration blocker for mdev devices
in vfio_legacy_attach_device() and the mdev_blocker field in
VFIODeviceCPR. The aim is to allow cpr-transfer and cpr-exec for mdev
vGPUs. The commit message says this needs a kernel that reverts
ef3a3f6a294b.

> diff --git a/hw/vfio/container-legacy.c b/hw/vfio/container-legacy.c
> --- a/hw/vfio/container-legacy.c
> +++ b/hw/vfio/container-legacy.c
> @@ -988,20 +988,8 @@ static bool vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev,
>          goto device_put_exit;
>      }
>  
> -    if (vbasedev->mdev) {
> -        error_setg(&vbasedev->cpr.mdev_blocker,
> -                   "CPR does not support vfio mdev %s", vbasedev->name);
> -        if (migrate_add_blocker_modes(&vbasedev->cpr.mdev_blocker, errp,
> -                                      MIG_MODE_CPR_TRANSFER, MIG_MODE_CPR_EXEC,
> -                                      -1) < 0) {
> -            goto hiod_unref_exit;
> -        }
> -    }
> -

[Severity: Medium]
Is the commit message accurate where it says "Without it the
VFIO_UPDATE_VADDR extension check in vfio_cpr_supported() still blocks
CPR"?

That check is container-wide.  It runs once, from
vfio_legacy_cpr_register_container(), right after the first group of an
address space has been attached:

    if (!vfio_cpr_supported(container, cpr_blocker)) {
        return migrate_add_blocker_modes(cpr_blocker, errp,
                                         MIG_MODE_CPR_TRANSFER,
                                         MIG_MODE_CPR_EXEC, -1) == 0;
    }

When a later group joins an existing container, vfio_container_connect()
returns early:

    QLIST_FOREACH(bcontainer, &space->containers, next) {
        container = VFIO_IOMMU_LEGACY(bcontainer);
        if (!ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &container->fd)) {
            return vfio_container_group_add(container, group, errp);
        }
    }

This path never calls vfio_cpr_supported() again.

Consider a passthrough group that opens the container first, followed by
an mdev group that joins it. On a kernel that still carries ef3a3f6a294b,
the extension check passes before the mdev attaches. The removed
mdev_blocker was the only guard for that case.

Can CPR then be refused only late, from vfio_container_pre_save()
through vfio_dma_unmap_vaddr_all(), instead of being blocked up front?

Does the patch also leave QEMU silently dependent on a patched kernel,
with no runtime probe, opt-in property or per-device blocker? That also
differs from upstream commit dac0dd68d9b.

[Severity: Low]
Is it intended that this blocker is removed for every mdev type? The
removed check was keyed only on vbasedev->mdev, so it covered all
mdev-backed devices attached through vfio_legacy_attach_device().

vfio-ap (vfio_ap_realize() in hw/vfio/ap.c) and vfio-ccw
(vfio_ccw_realize() in hw/vfio/ccw.c) both set:

    vbasedev->mdev = true;

so they also lose the blocker. vfio-pci does the same when
vfio_device_is_mdev() matches.

The commit message gives the reason only for mdev vGPUs. The only
remaining gate is the VFIO_CHECK_EXTENSION(VFIO_UPDATE_VADDR) test in
vfio_cpr_supported(), which does not look at device type.

Has the VADDR-update flow been checked for vfio-ap and vfio-ccw? If not,
should the blocker be kept for those device types?

>      return true;
>  
> -hiod_unref_exit:
> -    object_unref(vbasedev->hiod);
>  device_put_exit:
>      vfio_device_put(vbasedev);
>  group_put_exit:

[ ... ]

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