Re: [PATCH V1 22/26] vfio/iommufd: invariant device name

["Cédric Le Goater" <clg@redhat.com>]

On 1/29/25 15:43, Steve Sistare wrote:
> cpr-transfer will use the device name as a key to find the value
> of the device descriptor in new QEMU.  However, if the descriptor
> number is specified by a command-line fd parameter, then
> vfio_device_get_name creates a name that includes the fd number.
> This causes a chicken-and-egg problem: new QEMU must know the fd
> number to construct a name to find the fd number.
> 
> To fix, create an invariant name based on the id command-line
> parameter.  If id is not defined, add a CPR blocker.
> 
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>   hw/vfio/helpers.c             | 18 +++++++++++++++---
>   hw/vfio/iommufd.c             |  2 ++
>   include/hw/vfio/vfio-common.h |  1 +
>   3 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/vfio/helpers.c b/hw/vfio/helpers.c
> index 913796f..bd94b86 100644
> --- a/hw/vfio/helpers.c
> +++ b/hw/vfio/helpers.c
> @@ -25,6 +25,8 @@
>   #include "hw/vfio/vfio-common.h"
>   #include "hw/hw.h"
>   #include "trace.h"
> +#include "migration/blocker.h"
> +#include "migration/cpr.h"
>   #include "qapi/error.h"
>   #include "qemu/error-report.h"
>   #include "qemu/units.h"
> @@ -636,6 +638,7 @@ bool vfio_device_get_name(VFIODevice *vbasedev, Error **errp)
>   {
>       ERRP_GUARD();
>       struct stat st;
> +    bool ret = true;
>   
>       if (vbasedev->fd < 0) {
>           if (stat(vbasedev->sysfsdev, &st) < 0) {
> @@ -653,15 +656,24 @@ bool vfio_device_get_name(VFIODevice *vbasedev, Error **errp)
>               return false;
>           }
>           /*
> -         * Give a name with fd so any function printing out vbasedev->name
> +         * Give a name so any function printing out vbasedev->name
>            * will not break.
>            */
>           if (!vbasedev->name) {
> -            vbasedev->name = g_strdup_printf("VFIO_FD%d", vbasedev->fd);
> +            if (vbasedev->dev->id) {
> +                vbasedev->name = g_strdup(vbasedev->dev->id);
> +            } else {
> +                vbasedev->name = g_strdup_printf("VFIO_FD%d", vbasedev->fd);
> +                error_setg(&vbasedev->cpr_id_blocker,
> +                           "vfio device with fd=%d needs an id property",
> +                           vbasedev->fd);
> +                ret = migrate_add_blocker_modes(&vbasedev->cpr_id_blocker, errp,
> +                                                MIG_MODE_CPR_TRANSFER, -1) == 0;

cpr helper please.



> +            }
>           }
>       }
>   
> -    return true;
> +    return ret;
>   }
>   
>   void vfio_device_set_fd(VFIODevice *vbasedev, const char *str, Error **errp)
> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> index 2f888e5..8308715 100644
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -24,6 +24,7 @@
>   #include "system/reset.h"
>   #include "qemu/cutils.h"
>   #include "qemu/chardev_open.h"
> +#include "migration/blocker.h"
>   #include "pci.h"
>   #include "exec/ram_addr.h"
>   
> @@ -657,6 +658,7 @@ static void iommufd_cdev_detach(VFIODevice *vbasedev)
>       iommufd_cdev_container_destroy(container);
>       vfio_put_address_space(space);
>   
> +    migrate_del_blocker(&vbasedev->cpr_id_blocker);
>       iommufd_cdev_unbind_and_disconnect(vbasedev);
>       close(vbasedev->fd);
>   }
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index ca10abc..37e7c26 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -147,6 +147,7 @@ typedef struct VFIODevice {
>       VFIOMigration *migration;
>       Error *migration_blocker;
>       Error *cpr_mdev_blocker;
> +    Error *cpr_id_blocker;

a struct VFIODeviceCPR would be welcome.


Thanks,

C.



>       OnOffAuto pre_copy_dirty_page_tracking;
>       OnOffAuto device_dirty_page_tracking;
>       bool dirty_pages_supported;