diff mbox series

[v2,04/17] vfio/common: Extract vIOMMU code from vfio_sync_dirty_bitmap()

Message ID 20240617063409.34393-5-clg@redhat.com
State New
Headers show
Series vfio: QOMify VFIOContainer | expand

Commit Message

Cédric Le Goater June 17, 2024, 6:33 a.m. UTC 
From: Avihai Horon <avihaih@nvidia.com>

Extract vIOMMU code from vfio_sync_dirty_bitmap() to a new function and
restructure the code.

This is done in preparation for optimizing vIOMMU deviice dirty page
tracking. No functional changes intended.

Signed-off-by: Avihai Horon <avihaih@nvidia.com>
Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
[ clg: - Rebased on upstream ]
Signed-off-by: Cédric Le Goater <clg@redhat.com>
Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/vfio/common.c | 63 +++++++++++++++++++++++++++++-------------------
 1 file changed, 38 insertions(+), 25 deletions(-)

Comments

Eric Auger June 17, 2024, 2 p.m. UTC | #1 
Hi Cédric,

On 6/17/24 08:33, Cédric Le Goater wrote:
> From: Avihai Horon <avihaih@nvidia.com>
>
> Extract vIOMMU code from vfio_sync_dirty_bitmap() to a new function and
> restructure the code.
>
> This is done in preparation for optimizing vIOMMU deviice dirty page
device
> tracking. No functional changes intended.
>
> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> [ clg: - Rebased on upstream ]
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  hw/vfio/common.c | 63 +++++++++++++++++++++++++++++-------------------
>  1 file changed, 38 insertions(+), 25 deletions(-)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index fe215918bdf66ddbe3c5db803e10ce1aa9756b90..f28641bad5cf4b71fcdc0a6c9d42b24c8d786248 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -1302,37 +1302,50 @@ vfio_sync_ram_discard_listener_dirty_bitmap(VFIOContainerBase *bcontainer,
>                                                  &vrdl);
>  }
>  
> +static int vfio_sync_iommu_dirty_bitmap(VFIOContainerBase *bcontainer,
> +                                        MemoryRegionSection *section)
> +{
> +    VFIOGuestIOMMU *giommu;
> +    bool found = false;
> +    Int128 llend;
> +    vfio_giommu_dirty_notifier gdn;
> +    int idx;
> +
> +    QLIST_FOREACH(giommu, &bcontainer->giommu_list, giommu_next) {
> +        if (MEMORY_REGION(giommu->iommu_mr) == section->mr &&
> +            giommu->n.start == section->offset_within_region) {
> +            found = true;
> +            break;
> +        }
> +    }
> +
> +    if (!found) {
> +        return 0;
> +    }
> +
> +    gdn.giommu = giommu;
> +    idx = memory_region_iommu_attrs_to_index(giommu->iommu_mr,
> +                                             MEMTXATTRS_UNSPECIFIED);
> +
> +    llend = int128_add(int128_make64(section->offset_within_region),
> +                       section->size);
> +    llend = int128_sub(llend, int128_one());
> +
> +    iommu_notifier_init(&gdn.n, vfio_iommu_map_dirty_notify, IOMMU_NOTIFIER_MAP,
> +                        section->offset_within_region, int128_get64(llend),
> +                        idx);
> +    memory_region_iommu_replay(giommu->iommu_mr, &gdn.n);
> +
> +    return 0;
if this does not return anything else than 0, shouldn't it be void?

Thanks

Eric
> +}
> +
>  static int vfio_sync_dirty_bitmap(VFIOContainerBase *bcontainer,
>                                    MemoryRegionSection *section, Error **errp)
>  {
>      ram_addr_t ram_addr;
>  
>      if (memory_region_is_iommu(section->mr)) {
> -        VFIOGuestIOMMU *giommu;
> -
> -        QLIST_FOREACH(giommu, &bcontainer->giommu_list, giommu_next) {
> -            if (MEMORY_REGION(giommu->iommu_mr) == section->mr &&
> -                giommu->n.start == section->offset_within_region) {
> -                Int128 llend;
> -                vfio_giommu_dirty_notifier gdn = { .giommu = giommu };
> -                int idx = memory_region_iommu_attrs_to_index(giommu->iommu_mr,
> -                                                       MEMTXATTRS_UNSPECIFIED);
> -
> -                llend = int128_add(int128_make64(section->offset_within_region),
> -                                   section->size);
> -                llend = int128_sub(llend, int128_one());
> -
> -                iommu_notifier_init(&gdn.n,
> -                                    vfio_iommu_map_dirty_notify,
> -                                    IOMMU_NOTIFIER_MAP,
> -                                    section->offset_within_region,
> -                                    int128_get64(llend),
> -                                    idx);
> -                memory_region_iommu_replay(giommu->iommu_mr, &gdn.n);
> -                break;
> -            }
> -        }
> -        return 0;
> +        return vfio_sync_iommu_dirty_bitmap(bcontainer, section);
>      } else if (memory_region_has_ram_discard_manager(section->mr)) {
>          int ret;
>
Cédric Le Goater June 18, 2024, 11:22 a.m. UTC | #2 
On 6/17/24 4:00 PM, Eric Auger wrote:
> Hi Cédric,
> 
> On 6/17/24 08:33, Cédric Le Goater wrote:
>> From: Avihai Horon <avihaih@nvidia.com>
>>
>> Extract vIOMMU code from vfio_sync_dirty_bitmap() to a new function and
>> restructure the code.
>>
>> This is done in preparation for optimizing vIOMMU deviice dirty page
> device

fixed.


>> tracking. No functional changes intended.
>>
>> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> [ clg: - Rebased on upstream ]
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>   hw/vfio/common.c | 63 +++++++++++++++++++++++++++++-------------------
>>   1 file changed, 38 insertions(+), 25 deletions(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index fe215918bdf66ddbe3c5db803e10ce1aa9756b90..f28641bad5cf4b71fcdc0a6c9d42b24c8d786248 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -1302,37 +1302,50 @@ vfio_sync_ram_discard_listener_dirty_bitmap(VFIOContainerBase *bcontainer,
>>                                                   &vrdl);
>>   }
>>   
>> +static int vfio_sync_iommu_dirty_bitmap(VFIOContainerBase *bcontainer,
>> +                                        MemoryRegionSection *section)
>> +{
>> +    VFIOGuestIOMMU *giommu;
>> +    bool found = false;
>> +    Int128 llend;
>> +    vfio_giommu_dirty_notifier gdn;
>> +    int idx;
>> +
>> +    QLIST_FOREACH(giommu, &bcontainer->giommu_list, giommu_next) {
>> +        if (MEMORY_REGION(giommu->iommu_mr) == section->mr &&
>> +            giommu->n.start == section->offset_within_region) {
>> +            found = true;
>> +            break;
>> +        }
>> +    }
>> +
>> +    if (!found) {
>> +        return 0;
>> +    }
>> +
>> +    gdn.giommu = giommu;
>> +    idx = memory_region_iommu_attrs_to_index(giommu->iommu_mr,
>> +                                             MEMTXATTRS_UNSPECIFIED);
>> +
>> +    llend = int128_add(int128_make64(section->offset_within_region),
>> +                       section->size);
>> +    llend = int128_sub(llend, int128_one());
>> +
>> +    iommu_notifier_init(&gdn.n, vfio_iommu_map_dirty_notify, IOMMU_NOTIFIER_MAP,
>> +                        section->offset_within_region, int128_get64(llend),
>> +                        idx);
>> +    memory_region_iommu_replay(giommu->iommu_mr, &gdn.n);
>> +
>> +    return 0;
> if this does not return anything else than 0, shouldn't it be void?

Yes it could. The return value is practical for the caller below though. Let's
keep it that way for now, it's harmless.


Thanks,

C.


> 
> Thanks
> 
> Eric
>> +}
>> +
>>   static int vfio_sync_dirty_bitmap(VFIOContainerBase *bcontainer,
>>                                     MemoryRegionSection *section, Error **errp)
>>   {
>>       ram_addr_t ram_addr;
>>   
>>       if (memory_region_is_iommu(section->mr)) {
>> -        VFIOGuestIOMMU *giommu;
>> -
>> -        QLIST_FOREACH(giommu, &bcontainer->giommu_list, giommu_next) {
>> -            if (MEMORY_REGION(giommu->iommu_mr) == section->mr &&
>> -                giommu->n.start == section->offset_within_region) {
>> -                Int128 llend;
>> -                vfio_giommu_dirty_notifier gdn = { .giommu = giommu };
>> -                int idx = memory_region_iommu_attrs_to_index(giommu->iommu_mr,
>> -                                                       MEMTXATTRS_UNSPECIFIED);
>> -
>> -                llend = int128_add(int128_make64(section->offset_within_region),
>> -                                   section->size);
>> -                llend = int128_sub(llend, int128_one());
>> -
>> -                iommu_notifier_init(&gdn.n,
>> -                                    vfio_iommu_map_dirty_notify,
>> -                                    IOMMU_NOTIFIER_MAP,
>> -                                    section->offset_within_region,
>> -                                    int128_get64(llend),
>> -                                    idx);
>> -                memory_region_iommu_replay(giommu->iommu_mr, &gdn.n);
>> -                break;
>> -            }
>> -        }
>> -        return 0;
>> +        return vfio_sync_iommu_dirty_bitmap(bcontainer, section);
>>       } else if (memory_region_has_ram_discard_manager(section->mr)) {
>>           int ret;
>>   
>
diff mbox series

Patch

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index fe215918bdf66ddbe3c5db803e10ce1aa9756b90..f28641bad5cf4b71fcdc0a6c9d42b24c8d786248 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1302,37 +1302,50 @@  vfio_sync_ram_discard_listener_dirty_bitmap(VFIOContainerBase *bcontainer,
                                                 &vrdl);
 }
 
+static int vfio_sync_iommu_dirty_bitmap(VFIOContainerBase *bcontainer,
+                                        MemoryRegionSection *section)
+{
+    VFIOGuestIOMMU *giommu;
+    bool found = false;
+    Int128 llend;
+    vfio_giommu_dirty_notifier gdn;
+    int idx;
+
+    QLIST_FOREACH(giommu, &bcontainer->giommu_list, giommu_next) {
+        if (MEMORY_REGION(giommu->iommu_mr) == section->mr &&
+            giommu->n.start == section->offset_within_region) {
+            found = true;
+            break;
+        }
+    }
+
+    if (!found) {
+        return 0;
+    }
+
+    gdn.giommu = giommu;
+    idx = memory_region_iommu_attrs_to_index(giommu->iommu_mr,
+                                             MEMTXATTRS_UNSPECIFIED);
+
+    llend = int128_add(int128_make64(section->offset_within_region),
+                       section->size);
+    llend = int128_sub(llend, int128_one());
+
+    iommu_notifier_init(&gdn.n, vfio_iommu_map_dirty_notify, IOMMU_NOTIFIER_MAP,
+                        section->offset_within_region, int128_get64(llend),
+                        idx);
+    memory_region_iommu_replay(giommu->iommu_mr, &gdn.n);
+
+    return 0;
+}
+
 static int vfio_sync_dirty_bitmap(VFIOContainerBase *bcontainer,
                                   MemoryRegionSection *section, Error **errp)
 {
     ram_addr_t ram_addr;
 
     if (memory_region_is_iommu(section->mr)) {
-        VFIOGuestIOMMU *giommu;
-
-        QLIST_FOREACH(giommu, &bcontainer->giommu_list, giommu_next) {
-            if (MEMORY_REGION(giommu->iommu_mr) == section->mr &&
-                giommu->n.start == section->offset_within_region) {
-                Int128 llend;
-                vfio_giommu_dirty_notifier gdn = { .giommu = giommu };
-                int idx = memory_region_iommu_attrs_to_index(giommu->iommu_mr,
-                                                       MEMTXATTRS_UNSPECIFIED);
-
-                llend = int128_add(int128_make64(section->offset_within_region),
-                                   section->size);
-                llend = int128_sub(llend, int128_one());
-
-                iommu_notifier_init(&gdn.n,
-                                    vfio_iommu_map_dirty_notify,
-                                    IOMMU_NOTIFIER_MAP,
-                                    section->offset_within_region,
-                                    int128_get64(llend),
-                                    idx);
-                memory_region_iommu_replay(giommu->iommu_mr, &gdn.n);
-                break;
-            }
-        }
-        return 0;
+        return vfio_sync_iommu_dirty_bitmap(bcontainer, section);
     } else if (memory_region_has_ram_discard_manager(section->mr)) {
         int ret;