Re: [PATCH QEMU HCI-8.1 v5 34/36] 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 1 potential issue(s) to consider:
- [Medium] vfio: mdev CPR blocker removed; kernel capability check only runs once per container
--

commit df2c9f0ff7e7012bfa39fe116e7f04b6a2a0cd4e
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 mdev CPR blocker from the legacy VFIO
container code. It also drops the now-unused mdev_blocker field from
VFIODeviceCPR. The commit message says a kernel that reverts ef3a3f6a294b
is required, and that the VFIO_UPDATE_VADDR extension check in
vfio_cpr_supported() still blocks CPR otherwise.

> diff --git a/hw/vfio/container-legacy.c b/hw/vfio/container-legacy.c
> index e437eb0c7417..285e594b048b 100644
> --- 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;
> -        }
> -    }
> -
>      return true;

[Severity: Medium]
Can CPR now be accepted at command time and then fail partway through on
a host kernel that still carries ef3a3f6a294b?

The commit message relies on the VFIO_UPDATE_VADDR extension check in
vfio_cpr_supported() as the remaining gate:

hw/vfio/cpr-legacy.c:vfio_cpr_supported() {
    ...
    if (!ioctl(container->fd, VFIO_CHECK_EXTENSION, VFIO_UPDATE_VADDR)) {
        error_setg(errp, "VFIO container does not support VFIO_UPDATE_VADDR");
    ...
}

That check is per container. It is reached only through
vfio_legacy_cpr_register_container(), which vfio_container_connect()
calls when it creates a new container:

    if (!vfio_legacy_cpr_register_container(container, errp)) {
        goto fail;
    }

A group or device that joins an existing container goes through
vfio_container_group_add() and skips that check. The removed per-device
blocker was the only check that covered this case.

For example, an mdev could be added to a container that was already
registered for a physical device in the same address space. Nothing
would re-check it. If the kernel refuses VADDR unmap for that mdev,
the failure shows up late, in vfio_container_pre_save():

    if (!vfio_dma_unmap_vaddr_all(container, &local_err)) {
        error_report_err(local_err);
        return -1;
    }

That would fail the migration mid-CPR instead of blocking it up front.

I did not check the kernel source, so how ef3a3f6a294b treats an mdev
added after container creation is inferred, not verified.

Separately, the blocker is now lifted for every mdev type, not only
vGPUs. Would it make sense to limit it, or to add a per-device
capability check?

> -hiod_unref_exit:
> -    object_unref(vbasedev->hiod);
>  device_put_exit:

[ ... ]

> @@ -1017,7 +1005,6 @@ static void vfio_legacy_detach_device(VFIODevice *vbasedev)
>  
>      vfio_device_unprepare(vbasedev);
>  
> -    migrate_del_blocker(&vbasedev->cpr.mdev_blocker);
>      object_unref(vbasedev->hiod);

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261023-vfio_v1-v5-0-ba0bf3c6d122@virtuozzo.com?part=34