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.
But, if you don't like that, yes, I can drop region->fd in favour of the above.
thanks
john
