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

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

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
>