On 5/20/25 17:03, John Levon wrote:
> For vfio-user, each region has its own fd rather than sharing
> vbasedev's. Add the necessary plumbing to support this. For vfio
> backends with a shared fd, initialize region->fd to the shared one.
>
> Signed-off-by: John Levon <john.levon@nutanix.com>
> ---
> include/hw/vfio/vfio-device.h | 7 +++++--
> include/hw/vfio/vfio-region.h | 1 +
> hw/vfio/device.c | 28 ++++++++++++++++++++++++++--
> hw/vfio/region.c | 7 ++++++-
> 4 files changed, 38 insertions(+), 5 deletions(-)
>
> diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h
> index 923f9cd116..5cb817fd6a 100644
> --- a/include/hw/vfio/vfio-device.h
> +++ b/include/hw/vfio/vfio-device.h
> @@ -66,6 +66,7 @@ typedef struct VFIODevice {
> OnOffAuto enable_migration;
> OnOffAuto migration_multifd_transfer;
> bool migration_events;
> + bool use_region_fds;
> VFIODeviceOps *ops;
> VFIODeviceIOOps *io_ops;
> unsigned int num_irqs;
> @@ -84,6 +85,7 @@ typedef struct VFIODevice {
> VFIOIOASHwpt *hwpt;
> QLIST_ENTRY(VFIODevice) hwpt_next;
> struct vfio_region_info **reginfo;
> + int *region_fds;
> } VFIODevice;
>
> struct VFIODeviceOps {
> @@ -172,10 +174,11 @@ struct VFIODeviceIOOps {
> /**
> * @get_region_info
> *
> - * Fill in @info with information on the region given by @info->index.
> + * Fill in @info (and optionally @fd) with information on the region given
> + * by @info->index.
The whole VFIODeviceIOOps struct needs better documentation. The arguments
are missing.
> */
> int (*get_region_info)(VFIODevice *vdev,
> - struct vfio_region_info *info);
> + struct vfio_region_info *info, int *fd);
>
> /**
> * @get_irq_info
> diff --git a/include/hw/vfio/vfio-region.h b/include/hw/vfio/vfio-region.h
> index cbffb26962..80e83b23fd 100644
> --- a/include/hw/vfio/vfio-region.h
> +++ b/include/hw/vfio/vfio-region.h
> @@ -29,6 +29,7 @@ typedef struct VFIORegion {
> uint32_t nr_mmaps;
> VFIOMmap *mmaps;
> uint8_t nr; /* cache the region number for debug */
> + int fd; /* fd to mmap() region */
Could you split this change ? I am not sure it is needed.
> } VFIORegion;
>
>
> diff --git a/hw/vfio/device.c b/hw/vfio/device.c
> index d0068086ae..41db403992 100644
> --- a/hw/vfio/device.c
> +++ b/hw/vfio/device.c
> @@ -226,6 +226,7 @@ int vfio_device_get_region_info(VFIODevice *vbasedev, int index,
> struct vfio_region_info **info)
> {
> size_t argsz = sizeof(struct vfio_region_info);
> + int fd = -1;
> int ret;
>
> /* check cache */
> @@ -240,7 +241,7 @@ int vfio_device_get_region_info(VFIODevice *vbasedev, int index,
> retry:
> (*info)->argsz = argsz;
>
> - ret = vbasedev->io_ops->get_region_info(vbasedev, *info);
> + ret = vbasedev->io_ops->get_region_info(vbasedev, *info, &fd);
> if (ret != 0) {
> g_free(*info);
> *info = NULL;
> @@ -251,11 +252,19 @@ retry:
> argsz = (*info)->argsz;
> *info = g_realloc(*info, argsz);
>
> + if (fd != -1) {
> + close(fd);
> + fd = -1;
> + }
> +
> goto retry;
> }
>
> /* fill cache */
> vbasedev->reginfo[index] = *info;
> + if (vbasedev->region_fds != NULL) {
> + vbasedev->region_fds[index] = fd;
> + }
>
> return 0;
> }
> @@ -360,6 +369,7 @@ void vfio_device_init(VFIODevice *vbasedev, int type, VFIODeviceOps *ops,
> vbasedev->io_ops = &vfio_device_io_ops_ioctl;
> vbasedev->dev = dev;
> vbasedev->fd = -1;
> + vbasedev->use_region_fds = false;
I wish we could avoid this bool. I have no idea yet.
>
> vbasedev->ram_block_discard_allowed = ram_discard;
> }
> @@ -470,6 +480,9 @@ void vfio_device_prepare(VFIODevice *vbasedev, VFIOContainerBase *bcontainer,
>
> vbasedev->reginfo = g_new0(struct vfio_region_info *,
> vbasedev->num_regions);
> + if (vbasedev->use_region_fds) {
> + vbasedev->region_fds = g_new0(int, vbasedev->num_regions);
> + }
> }
>
> void vfio_device_unprepare(VFIODevice *vbasedev)
> @@ -478,9 +491,17 @@ void vfio_device_unprepare(VFIODevice *vbasedev)
>
> for (i = 0; i < vbasedev->num_regions; i++) {
> g_free(vbasedev->reginfo[i]);
> + if (vbasedev->region_fds != NULL && vbasedev->region_fds[i] != -1) {
> + close(vbasedev->region_fds[i]);
> + }
> +
> }
> g_free(vbasedev->reginfo);
> vbasedev->reginfo = NULL;
> + if (vbasedev->region_fds != NULL) {
g_free is NULL safe. No need to test for it. g_clear_pointer() is a nice
helper too.
> + g_free(vbasedev->region_fds);
> + vbasedev->region_fds = NULL;
> + }
>
> QLIST_REMOVE(vbasedev, container_next);
> QLIST_REMOVE(vbasedev, global_next);
> @@ -502,10 +523,13 @@ static int vfio_device_io_device_feature(VFIODevice *vbasedev,
> }
>
> static int vfio_device_io_get_region_info(VFIODevice *vbasedev,
> - struct vfio_region_info *info)
> + struct vfio_region_info *info,
> + int *fd)
> {
> int ret;
>
> + *fd = -1;
> +
> ret = ioctl(vbasedev->fd, VFIO_DEVICE_GET_REGION_INFO, info);
>
> return ret < 0 ? -errno : ret;
> diff --git a/hw/vfio/region.c b/hw/vfio/region.c
> index 34752c3f65..3c93da91d8 100644
> --- a/hw/vfio/region.c
> +++ b/hw/vfio/region.c
> @@ -200,6 +200,11 @@ int vfio_region_setup(Object *obj, VFIODevice *vbasedev, VFIORegion *region,
> region->size = info->size;
> region->fd_offset = info->offset;
> region->nr = index;
> + if (vbasedev->region_fds != NULL) {
> + region->fd = vbasedev->region_fds[index];
> + } else {
> + region->fd = vbasedev->fd;
> + }
>
> if (region->size) {
> region->mem = g_new0(MemoryRegion, 1);
> @@ -278,7 +283,7 @@ int vfio_region_mmap(VFIORegion *region)
>
> region->mmaps[i].mmap = mmap(map_align, region->mmaps[i].size, prot,
> MAP_SHARED | MAP_FIXED,
> - region->vbasedev->fd,
> + region->fd,
Would this work ?
vbasedev->region_fds ? vbasedev->region_fds[region->nr] : vbasedev->fd,
Thanks,
C.
> region->fd_offset +
> region->mmaps[i].offset);
> if (region->mmaps[i].mmap == MAP_FAILED) {