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

2023-03-07 Thread 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 
> 
> Thanks.
> 
>>
>> Signed-off-by: Joao Martins 
>> Reviewed-by: Cédric Le Goater 
>> ---
>>   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, >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, >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(, "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, >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
>>



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

2023-03-07 Thread Avihai Horon



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.


Other than that, FWIW:

Reviewed-by: Avihai Horon 

Thanks.



Signed-off-by: Joao Martins 
Reviewed-by: Cédric Le Goater 
---
  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, >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, >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(, "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, >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