Re: [PATCH v1 4/7] vfio/region: Add a helper to get region index from memory region

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

On 10/7/25 06:50, Kasireddy, Vivek wrote:
> Hi Cedric,
> 
>> Subject: Re: [PATCH v1 4/7] vfio/region: Add a helper to get region index from
>> memory region
>>
>> Hello Vivek,
>>
>> On 10/4/25 01:35, Vivek Kasireddy wrote:
>>> Having a way to figure out the region index (or bar) associated
>>> with a memory region is helpful in various scenarios. For example,
>>> this capability can be useful in retrieving the region info needed
>>> for mapping a part of a VFIO region or creating a dmabuf.
>>>
>>> Cc: Alex Williamson <alex.williamson@redhat.com>
>>> Cc: Cédric Le Goater <clg@redhat.com>
>>> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
>>> ---
>>>    hw/vfio/region.c              | 14 ++++++++++++++
>>>    include/hw/vfio/vfio-device.h |  2 ++
>>>    2 files changed, 16 insertions(+)
>>>
>>> diff --git a/hw/vfio/region.c b/hw/vfio/region.c
>>> index b165ab0b93..8837afc97f 100644
>>> --- a/hw/vfio/region.c
>>> +++ b/hw/vfio/region.c
>>> @@ -398,3 +398,17 @@ void vfio_region_mmaps_set_enabled(VFIORegion
>> *region, bool enabled)
>>>        trace_vfio_region_mmaps_set_enabled(memory_region_name(region-
>>> mem),
>>>                                            enabled);
>>>    }
>>> +
>>> +int vfio_get_region_index_from_mr(MemoryRegion *mr)
>>> +{
>>> +    VFIORegion *region = mr->opaque;
>>> +
>>> +    if (mr->ops != &vfio_region_ops) {
>>> +        mr = mr->container;
>>
>> May be start the routine with :
>>
>>       while (mr->container) {
>>           mr = mr->container;
>>       }
> Ok, makes sense.
> 
>>
>>> +        if (mr->ops != &vfio_region_ops) {
>>> +            return -1;
>>
>> I think I would prefer returning a 'VFIORegion *' which should be
>> NULL in case of error. Looks cleaner.
> Given that VFIORegion type is no longer exposed to other subsystems,
> I am guessing you meant adding two helpers: internal helper that would
> return VFIORegion * and an external routine that would call the internal
> helper and return region->nr?


After seeing how vfio_get_region_index_from_mr() was used in
vfio_dmabuf_mmap(), I think it is fine to keep your proposal
unchanged. we can refine it later.


Thanks,

C.


> 
>>
>>
>>> +        }
>>> +	region = mr->opaque;
>>> +    }
>>> +    return region->nr;
>>> +}
>>> diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h
>>> index 7e9aed6d3c..bdb106c937 100644
>>> --- a/include/hw/vfio/vfio-device.h
>>> +++ b/include/hw/vfio/vfio-device.h
>>> @@ -277,6 +277,8 @@ bool vfio_device_has_region_cap(VFIODevice
>> *vbasedev, int region, uint16_t cap_t
>>>
>>>    int vfio_device_get_irq_info(VFIODevice *vbasedev, int index,
>>>                                    struct vfio_irq_info *info);
>>> +
>>> +int vfio_get_region_index_from_mr(MemoryRegion *mr);
>> Please add documentation.
> Sure, will do.
> 
> Thanks,
> Vivek
> 
>>
>> Thanks,
>>
>> C.
>>
>>
>>>    #endif
>>>
>>>    /* Returns 0 on success, or a negative errno. */
>