diff mbox series

[v4,07/14] vfio/common: Add helper to consolidate iova/end calculation

Message ID 20230307020258.58215-8-joao.m.martins@oracle.com (mailing list archive)
State New, archived
Headers show
Series vfio/migration: Device dirty page tracking | expand

Commit Message

Joao Martins March 7, 2023, 2:02 a.m. UTC 
In preparation to be used in device dirty tracking, move the code that
calculates a iova/end range from the container/section.  This avoids
duplication on the common checks across listener callbacks.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
 hw/vfio/common.c | 37 ++++++++++++++++++++++++++++++-------
 1 file changed, 30 insertions(+), 7 deletions(-)

Comments

Alex Williamson March 7, 2023, 2:40 a.m. UTC | #1 
On Tue,  7 Mar 2023 02:02:51 +0000
Joao Martins <joao.m.martins@oracle.com> wrote:

> In preparation to be used in device dirty tracking, move the code that
> calculates a iova/end range from the container/section.  This avoids
> duplication on the common checks across listener callbacks.
> 
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> ---
>  hw/vfio/common.c | 37 ++++++++++++++++++++++++++++++-------
>  1 file changed, 30 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 54b4a4fc7daf..3a6491dbc523 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -961,6 +961,35 @@ static bool vfio_listener_valid_section(MemoryRegionSection *section)
>      return true;
>  }
>  
> +/*
> + * Called for the dirty tracking memory listener to calculate the iova/end
> + * for a given memory region section.
> + */
> +static bool vfio_get_section_iova_range(VFIOContainer *container,
> +                                        MemoryRegionSection *section,
> +                                        hwaddr *out_iova, hwaddr *out_end,
> +                                        Int128 *out_llend)
> +{
> +    Int128 llend;
> +    hwaddr iova;
> +
> +    iova = REAL_HOST_PAGE_ALIGN(section->offset_within_address_space);
> +    llend = int128_make64(section->offset_within_address_space);
> +    llend = int128_add(llend, section->size);
> +    llend = int128_and(llend, int128_exts64(qemu_real_host_page_mask()));
> +
> +    if (int128_ge(int128_make64(iova), llend)) {
> +        return false;
> +    }
> +
> +    *out_iova = iova;
> +    *out_end = int128_get64(int128_sub(llend, int128_one()));
> +    if (out_llend) {
> +        *out_llend = llend;
> +    }
> +    return true;
> +}
> +
>  static void vfio_listener_region_add(MemoryListener *listener,
>                                       MemoryRegionSection *section)
>  {
> @@ -976,12 +1005,7 @@ static void vfio_listener_region_add(MemoryListener *listener,
>          return;
>      }
>  
> -    iova = REAL_HOST_PAGE_ALIGN(section->offset_within_address_space);
> -    llend = int128_make64(section->offset_within_address_space);
> -    llend = int128_add(llend, section->size);
> -    llend = int128_and(llend, int128_exts64(qemu_real_host_page_mask()));
> -
> -    if (int128_ge(int128_make64(iova), llend)) {
> +    if (!vfio_get_section_iova_range(container, section, &iova, &end, &llend)) {
>          if (memory_region_is_ram_device(section->mr)) {
>              trace_vfio_listener_region_add_no_dma_map(
>                  memory_region_name(section->mr),
> @@ -991,7 +1015,6 @@ static void vfio_listener_region_add(MemoryListener *listener,
>          }
>          return;
>      }
> -    end = int128_get64(int128_sub(llend, int128_one()));
>  
>      if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) {
>          hwaddr pgsize = 0;

Shouldn't this convert vfio_listener_region_del() too?  Thanks,

Alex
Avihai Horon March 7, 2023, 9:52 a.m. UTC | #2 
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
> calculates a iova/end range from the container/section.  This avoids
> duplication on the common checks across listener callbacks.
>
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> ---
>   hw/vfio/common.c | 37 ++++++++++++++++++++++++++++++-------
>   1 file changed, 30 insertions(+), 7 deletions(-)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 54b4a4fc7daf..3a6491dbc523 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -961,6 +961,35 @@ static bool vfio_listener_valid_section(MemoryRegionSection *section)
>       return true;
>   }
>
> +/*
> + * Called for the dirty tracking memory listener to calculate the iova/end
> + * for a given memory region section.
> + */

Should we just delete this comment? The function name is pretty clear.
Besides that, the comment is not completely accurate -- in this patch we 
are not using it yet for dirty tracking and it's also used for 
region_{add,del}.

Thanks.

> +static bool vfio_get_section_iova_range(VFIOContainer *container,
> +                                        MemoryRegionSection *section,
> +                                        hwaddr *out_iova, hwaddr *out_end,
> +                                        Int128 *out_llend)
> +{
> +    Int128 llend;
> +    hwaddr iova;
> +
> +    iova = REAL_HOST_PAGE_ALIGN(section->offset_within_address_space);
> +    llend = int128_make64(section->offset_within_address_space);
> +    llend = int128_add(llend, section->size);
> +    llend = int128_and(llend, int128_exts64(qemu_real_host_page_mask()));
> +
> +    if (int128_ge(int128_make64(iova), llend)) {
> +        return false;
> +    }
> +
> +    *out_iova = iova;
> +    *out_end = int128_get64(int128_sub(llend, int128_one()));
> +    if (out_llend) {
> +        *out_llend = llend;
> +    }
> +    return true;
> +}
> +
>   static void vfio_listener_region_add(MemoryListener *listener,
>                                        MemoryRegionSection *section)
>   {
> @@ -976,12 +1005,7 @@ static void vfio_listener_region_add(MemoryListener *listener,
>           return;
>       }
>
> -    iova = REAL_HOST_PAGE_ALIGN(section->offset_within_address_space);
> -    llend = int128_make64(section->offset_within_address_space);
> -    llend = int128_add(llend, section->size);
> -    llend = int128_and(llend, int128_exts64(qemu_real_host_page_mask()));
> -
> -    if (int128_ge(int128_make64(iova), llend)) {
> +    if (!vfio_get_section_iova_range(container, section, &iova, &end, &llend)) {
>           if (memory_region_is_ram_device(section->mr)) {
>               trace_vfio_listener_region_add_no_dma_map(
>                   memory_region_name(section->mr),
> @@ -991,7 +1015,6 @@ static void vfio_listener_region_add(MemoryListener *listener,
>           }
>           return;
>       }
> -    end = int128_get64(int128_sub(llend, int128_one()));
>
>       if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) {
>           hwaddr pgsize = 0;
> --
> 2.17.2
>
Joao Martins March 7, 2023, 10:11 a.m. UTC | #3 
On 07/03/2023 02:40, Alex Williamson wrote:
> On Tue,  7 Mar 2023 02:02:51 +0000
> Joao Martins <joao.m.martins@oracle.com> wrote:
> 
>> In preparation to be used in device dirty tracking, move the code that
>> calculates a iova/end range from the container/section.  This avoids
>> duplication on the common checks across listener callbacks.
>>
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> ---
>>  hw/vfio/common.c | 37 ++++++++++++++++++++++++++++++-------
>>  1 file changed, 30 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 54b4a4fc7daf..3a6491dbc523 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -961,6 +961,35 @@ static bool vfio_listener_valid_section(MemoryRegionSection *section)
>>      return true;
>>  }
>>  
>> +/*
>> + * Called for the dirty tracking memory listener to calculate the iova/end
>> + * for a given memory region section.
>> + */
>> +static bool vfio_get_section_iova_range(VFIOContainer *container,
>> +                                        MemoryRegionSection *section,
>> +                                        hwaddr *out_iova, hwaddr *out_end,
>> +                                        Int128 *out_llend)
>> +{
>> +    Int128 llend;
>> +    hwaddr iova;
>> +
>> +    iova = REAL_HOST_PAGE_ALIGN(section->offset_within_address_space);
>> +    llend = int128_make64(section->offset_within_address_space);
>> +    llend = int128_add(llend, section->size);
>> +    llend = int128_and(llend, int128_exts64(qemu_real_host_page_mask()));
>> +
>> +    if (int128_ge(int128_make64(iova), llend)) {
>> +        return false;
>> +    }
>> +
>> +    *out_iova = iova;
>> +    *out_end = int128_get64(int128_sub(llend, int128_one()));
>> +    if (out_llend) {
>> +        *out_llend = llend;
>> +    }
>> +    return true;
>> +}
>> +
>>  static void vfio_listener_region_add(MemoryListener *listener,
>>                                       MemoryRegionSection *section)
>>  {
>> @@ -976,12 +1005,7 @@ static void vfio_listener_region_add(MemoryListener *listener,
>>          return;
>>      }
>>  
>> -    iova = REAL_HOST_PAGE_ALIGN(section->offset_within_address_space);
>> -    llend = int128_make64(section->offset_within_address_space);
>> -    llend = int128_add(llend, section->size);
>> -    llend = int128_and(llend, int128_exts64(qemu_real_host_page_mask()));
>> -
>> -    if (int128_ge(int128_make64(iova), llend)) {
>> +    if (!vfio_get_section_iova_range(container, section, &iova, &end, &llend)) {
>>          if (memory_region_is_ram_device(section->mr)) {
>>              trace_vfio_listener_region_add_no_dma_map(
>>                  memory_region_name(section->mr),
>> @@ -991,7 +1015,6 @@ static void vfio_listener_region_add(MemoryListener *listener,
>>          }
>>          return;
>>      }
>> -    end = int128_get64(int128_sub(llend, int128_one()));
>>  
>>      if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) {
>>          hwaddr pgsize = 0;
> 
> Shouldn't this convert vfio_listener_region_del() too?  Thanks,

Yeap.
Joao Martins March 7, 2023, 10:26 a.m. UTC | #4 
On 07/03/2023 09:52, 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
>> calculates a iova/end range from the container/section.  This avoids
>> duplication on the common checks across listener callbacks.
>>
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> ---
>>   hw/vfio/common.c | 37 ++++++++++++++++++++++++++++++-------
>>   1 file changed, 30 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 54b4a4fc7daf..3a6491dbc523 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -961,6 +961,35 @@ static bool
>> vfio_listener_valid_section(MemoryRegionSection *section)
>>       return true;
>>   }
>>
>> +/*
>> + * Called for the dirty tracking memory listener to calculate the iova/end
>> + * for a given memory region section.
>> + */
> 
> Should we just delete this comment? The function name is pretty clear.
> Besides that, the comment is not completely accurate -- in this patch we are not
> using it yet for dirty tracking and it's also used for region_{add,del}.
> 
Yes, let me delete it.

> Thanks.
> 
>> +static bool vfio_get_section_iova_range(VFIOContainer *container,
>> +                                        MemoryRegionSection *section,
>> +                                        hwaddr *out_iova, hwaddr *out_end,
>> +                                        Int128 *out_llend)
>> +{
>> +    Int128 llend;
>> +    hwaddr iova;
>> +
>> +    iova = REAL_HOST_PAGE_ALIGN(section->offset_within_address_space);
>> +    llend = int128_make64(section->offset_within_address_space);
>> +    llend = int128_add(llend, section->size);
>> +    llend = int128_and(llend, int128_exts64(qemu_real_host_page_mask()));
>> +
>> +    if (int128_ge(int128_make64(iova), llend)) {
>> +        return false;
>> +    }
>> +
>> +    *out_iova = iova;
>> +    *out_end = int128_get64(int128_sub(llend, int128_one()));
>> +    if (out_llend) {
>> +        *out_llend = llend;
>> +    }
>> +    return true;
>> +}
>> +
>>   static void vfio_listener_region_add(MemoryListener *listener,
>>                                        MemoryRegionSection *section)
>>   {
>> @@ -976,12 +1005,7 @@ static void vfio_listener_region_add(MemoryListener
>> *listener,
>>           return;
>>       }
>>
>> -    iova = REAL_HOST_PAGE_ALIGN(section->offset_within_address_space);
>> -    llend = int128_make64(section->offset_within_address_space);
>> -    llend = int128_add(llend, section->size);
>> -    llend = int128_and(llend, int128_exts64(qemu_real_host_page_mask()));
>> -
>> -    if (int128_ge(int128_make64(iova), llend)) {
>> +    if (!vfio_get_section_iova_range(container, section, &iova, &end, &llend)) {
>>           if (memory_region_is_ram_device(section->mr)) {
>>               trace_vfio_listener_region_add_no_dma_map(
>>                   memory_region_name(section->mr),
>> @@ -991,7 +1015,6 @@ static void vfio_listener_region_add(MemoryListener
>> *listener,
>>           }
>>           return;
>>       }
>> -    end = int128_get64(int128_sub(llend, int128_one()));
>>
>>       if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) {
>>           hwaddr pgsize = 0;
>> -- 
>> 2.17.2
>>
diff mbox series

Patch

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 54b4a4fc7daf..3a6491dbc523 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -961,6 +961,35 @@  static bool vfio_listener_valid_section(MemoryRegionSection *section)
     return true;
 }
 
+/*
+ * Called for the dirty tracking memory listener to calculate the iova/end
+ * for a given memory region section.
+ */
+static bool vfio_get_section_iova_range(VFIOContainer *container,
+                                        MemoryRegionSection *section,
+                                        hwaddr *out_iova, hwaddr *out_end,
+                                        Int128 *out_llend)
+{
+    Int128 llend;
+    hwaddr iova;
+
+    iova = REAL_HOST_PAGE_ALIGN(section->offset_within_address_space);
+    llend = int128_make64(section->offset_within_address_space);
+    llend = int128_add(llend, section->size);
+    llend = int128_and(llend, int128_exts64(qemu_real_host_page_mask()));
+
+    if (int128_ge(int128_make64(iova), llend)) {
+        return false;
+    }
+
+    *out_iova = iova;
+    *out_end = int128_get64(int128_sub(llend, int128_one()));
+    if (out_llend) {
+        *out_llend = llend;
+    }
+    return true;
+}
+
 static void vfio_listener_region_add(MemoryListener *listener,
                                      MemoryRegionSection *section)
 {
@@ -976,12 +1005,7 @@  static void vfio_listener_region_add(MemoryListener *listener,
         return;
     }
 
-    iova = REAL_HOST_PAGE_ALIGN(section->offset_within_address_space);
-    llend = int128_make64(section->offset_within_address_space);
-    llend = int128_add(llend, section->size);
-    llend = int128_and(llend, int128_exts64(qemu_real_host_page_mask()));
-
-    if (int128_ge(int128_make64(iova), llend)) {
+    if (!vfio_get_section_iova_range(container, section, &iova, &end, &llend)) {
         if (memory_region_is_ram_device(section->mr)) {
             trace_vfio_listener_region_add_no_dma_map(
                 memory_region_name(section->mr),
@@ -991,7 +1015,6 @@  static void vfio_listener_region_add(MemoryListener *listener,
         }
         return;
     }
-    end = int128_get64(int128_sub(llend, int128_one()));
 
     if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) {
         hwaddr pgsize = 0;