Re: [PATCH v4 05/14] vfio/common: Add helper to validate iova/end against hostwin

[Joao Martins]

On 07/03/2023 08:57, Avihai Horon wrote:
> 
> On 07/03/2023 4:02, Joao Martins wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> In preparation to be used in device dirty tracking, move the code that
>> finds the container host DMA window against a iova range.  This avoids
>> duplication on the common checks across listener callbacks.
> 
> Eventually this isn't used by device dirty tracking, so "In preparation to be
> used in device dirty tracking" can be dropped.
> 
good catch, as this is here since the first range-version checks that still had
a over complicated version of vfio_get_section_range(). I'll remove it.

> Other than that, FWIW:
> 
> Reviewed-by: Avihai Horon <avihaih@nvidia.com>
> 
> Thanks.
> 
>>
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> Reviewed-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>   hw/vfio/common.c | 38 ++++++++++++++++++++------------------
>>   1 file changed, 20 insertions(+), 18 deletions(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index cec3de08d2b4..99acb998eb14 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -901,6 +901,22 @@ static void
>> vfio_unregister_ram_discard_listener(VFIOContainer *container,
>>       g_free(vrdl);
>>   }
>>
>> +static VFIOHostDMAWindow *vfio_find_hostwin(VFIOContainer *container,
>> +                                            hwaddr iova, hwaddr end)
>> +{
>> +    VFIOHostDMAWindow *hostwin;
>> +    bool hostwin_found = false;
>> +
>> +    QLIST_FOREACH(hostwin, &container->hostwin_list, hostwin_next) {
>> +        if (hostwin->min_iova <= iova && end <= hostwin->max_iova) {
>> +            hostwin_found = true;
>> +            break;
>> +        }
>> +    }
>> +
>> +    return hostwin_found ? hostwin : NULL;
>> +}
>> +
>>   static bool vfio_known_safe_misalignment(MemoryRegionSection *section)
>>   {
>>       MemoryRegion *mr = section->mr;
>> @@ -926,7 +942,6 @@ static void vfio_listener_region_add(MemoryListener
>> *listener,
>>       void *vaddr;
>>       int ret;
>>       VFIOHostDMAWindow *hostwin;
>> -    bool hostwin_found;
>>       Error *err = NULL;
>>
>>       if (vfio_listener_skipped_section(section)) {
>> @@ -1027,15 +1042,8 @@ static void vfio_listener_region_add(MemoryListener
>> *listener,
>>   #endif
>>       }
>>
>> -    hostwin_found = false;
>> -    QLIST_FOREACH(hostwin, &container->hostwin_list, hostwin_next) {
>> -        if (hostwin->min_iova <= iova && end <= hostwin->max_iova) {
>> -            hostwin_found = true;
>> -            break;
>> -        }
>> -    }
>> -
>> -    if (!hostwin_found) {
>> +    hostwin = vfio_find_hostwin(container, iova, end);
>> +    if (!hostwin) {
>>           error_setg(&err, "Container %p can't map guest IOVA region"
>>                      " 0x%"HWADDR_PRIx"..0x%"HWADDR_PRIx, container, iova, 
>> end);
>>           goto fail;
>> @@ -1237,15 +1245,9 @@ static void vfio_listener_region_del(MemoryListener
>> *listener,
>>       if (memory_region_is_ram_device(section->mr)) {
>>           hwaddr pgmask;
>>           VFIOHostDMAWindow *hostwin;
>> -        bool hostwin_found = false;
>>
>> -        QLIST_FOREACH(hostwin, &container->hostwin_list, hostwin_next) {
>> -            if (hostwin->min_iova <= iova && end <= hostwin->max_iova) {
>> -                hostwin_found = true;
>> -                break;
>> -            }
>> -        }
>> -        assert(hostwin_found); /* or region_add() would have failed */
>> +        hostwin = vfio_find_hostwin(container, iova, end);
>> +        assert(hostwin); /* or region_add() would have failed */
>>
>>           pgmask = (1ULL << ctz64(hostwin->iova_pgsizes)) - 1;
>>           try_unmap = !((iova & pgmask) || (int128_get64(llsize) & pgmask));
>> -- 
>> 2.17.2
>>