Re: [PATCH v2 07/29] vfio: add per-region fd support

[Cédric Le Goater]

On 5/21/25 10:25, John Levon wrote:

On Wed, May 21, 2025 at 10:11:06AM +0200, Cédric Le Goater wrote:

-     * 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.

Will add another preparatory patch, thanks.

@@ -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.

The idea was to avoid having every bit of code that needed the region fd having
to remember to do:

@@ -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,

That is, region->fd is *always* correct, and there is less chance of a bug where
somebody incorrectly uses vbasedev fd instead. IMO "region->fd" is much
cleaner/clearer.

maybe. It's only used in one place : vfio_region_mmap(). I think caching the
fd value under VFIORegion is overkill.


Thanks,

C.

But, if you don't like that, yes, I can drop region->fd in favour of the above.

thanks
john