[PATCH v1 0/2] hw/vfio: Improve vfio_get_dirty_bitmap() tracepoint

[PATCH v1 0/2] hw/vfio: Improve vfio_get_dirty_bitmap() tracepoint

[Joao Martins]

Hey,

This tiny series changes the tracepoint to include the number of
dirty pages via the vfio_get_dirty_bitmap. I find it useful for
observability in general to understand the number of dirty pages in an
IOVA range. With dirty tracking supported by device or IOMMU it's specially
relevant data to include in the tracepoint.

First patch includes the dirty argument in the helper function setting
dirty bits and the second patch just expands the tracepoint to include
the dirty pages.

Thanks,
	Joao

[1] https://lore.kernel.org/qemu-devel/20220428211351.3897-1-joao.m.martins@oracle.com/#r

Joao Martins (2):
  exec/ram_addr: add dirty arg to cpu_physical_memory_set_dirty_lebitmap()
  hw/vfio: Add nr of dirty pages to vfio_get_dirty_bitmap tracepoint

 accel/kvm/kvm-all.c     |  2 +-
 hw/vfio/common.c        |  7 ++++---
 hw/vfio/trace-events    |  2 +-
 include/exec/ram_addr.h | 10 +++++++++-
 4 files changed, 15 insertions(+), 6 deletions(-)

-- 
2.31.1

[PATCH v1 1/2] exec/ram_addr: add dirty arg to cpu_physical_memory_set_dirty_lebitmap()

[Joao Martins]

In preparation to including the number of dirty pages in the
vfio_get_dirty_bitmap() tracepoint, return the number of dirty pages in the
range passed by @dirty argument in cpu_physical_memory_set_dirty_lebitmap().

For now just set the callers to NULL, no functional change intended.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
 accel/kvm/kvm-all.c     |  2 +-
 hw/vfio/common.c        |  4 ++--
 include/exec/ram_addr.h | 10 +++++++++-
 3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index cf3a88d90e92..1524a34f1786 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -559,7 +559,7 @@ static void kvm_slot_sync_dirty_pages(KVMSlot *slot)
     ram_addr_t start = slot->ram_start_offset;
     ram_addr_t pages = slot->memory_size / qemu_real_host_page_size();
 
-    cpu_physical_memory_set_dirty_lebitmap(slot->dirty_bmap, start, pages);
+    cpu_physical_memory_set_dirty_lebitmap(slot->dirty_bmap, start, pages, NULL);
 }
 
 static void kvm_slot_reset_dirty_pages(KVMSlot *slot)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 78358ede2764..dcbf7c574d85 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -587,7 +587,7 @@ static int vfio_dma_unmap_bitmap(VFIOContainer *container,
     ret = ioctl(container->fd, VFIO_IOMMU_UNMAP_DMA, unmap);
     if (!ret) {
         cpu_physical_memory_set_dirty_lebitmap(vbmap.bitmap,
-                iotlb->translated_addr, vbmap.pages);
+                iotlb->translated_addr, vbmap.pages, NULL);
     } else {
         error_report("VFIO_UNMAP_DMA with DIRTY_BITMAP : %m");
     }
@@ -1773,7 +1773,7 @@ static int vfio_get_dirty_bitmap(VFIOContainer *container, uint64_t iova,
     }
 
     cpu_physical_memory_set_dirty_lebitmap(vbmap.bitmap, ram_addr,
-                                           vbmap.pages);
+                                           vbmap.pages, NULL);
 
     trace_vfio_get_dirty_bitmap(container->fd, iova, size, vbmap.size,
                                 ram_addr);
diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index f4fb6a211175..07bf9e1502b6 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -335,7 +335,8 @@ static inline void cpu_physical_memory_set_dirty_range(ram_addr_t start,
 #if !defined(_WIN32)
 static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
                                                           ram_addr_t start,
-                                                          ram_addr_t pages)
+                                                          ram_addr_t pages,
+                                                          unsigned long *dirty)
 {
     unsigned long i, j;
     unsigned long page_number, c;
@@ -380,6 +381,10 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
                         }
                     }
 
+                    if (dirty) {
+                        *dirty += ctpopl(temp);
+                    }
+
                     if (tcg_enabled()) {
                         qatomic_or(&blocks[DIRTY_MEMORY_CODE][idx][offset],
                                    temp);
@@ -411,6 +416,9 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
                 if (unlikely(global_dirty_tracking & GLOBAL_DIRTY_DIRTY_RATE)) {
                     total_dirty_pages += ctpopl(c);
                 }
+                if (dirty) {
+                    *dirty += ctpopl(c);
+                }
                 do {
                     j = ctzl(c);
                     c &= ~(1ul << j);
-- 
2.31.1

[PATCH v1 2/2] hw/vfio: Add nr of dirty pages to vfio_get_dirty_bitmap tracepoint

[Joao Martins]

Include the number of dirty pages on the vfio_get_dirty_bitmap tracepoint.
These are fetched from the newly added argument in
cpu_physical_memory_set_lebitmap().

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
 hw/vfio/common.c     | 5 +++--
 hw/vfio/trace-events | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index dcbf7c574d85..a08c7dcad8cd 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1747,6 +1747,7 @@ static int vfio_get_dirty_bitmap(VFIOContainer *container, uint64_t iova,
 {
     bool all_device_dirty_tracking =
         vfio_devices_all_device_dirty_tracking(container);
+    uint64_t dirty = 0;
     VFIOBitmap vbmap;
     int ret;
 
@@ -1773,10 +1774,10 @@ static int vfio_get_dirty_bitmap(VFIOContainer *container, uint64_t iova,
     }
 
     cpu_physical_memory_set_dirty_lebitmap(vbmap.bitmap, ram_addr,
-                                           vbmap.pages, NULL);
+                                           vbmap.pages, &dirty);
 
     trace_vfio_get_dirty_bitmap(container->fd, iova, size, vbmap.size,
-                                ram_addr);
+                                ram_addr, dirty);
 out:
     g_free(vbmap.bitmap);
 
diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
index 646e42fd27f9..9265a406eda1 100644
--- a/hw/vfio/trace-events
+++ b/hw/vfio/trace-events
@@ -120,7 +120,7 @@ vfio_region_sparse_mmap_header(const char *name, int index, int nr_areas) "Devic
 vfio_region_sparse_mmap_entry(int i, unsigned long start, unsigned long end) "sparse entry %d [0x%lx - 0x%lx]"
 vfio_get_dev_region(const char *name, int index, uint32_t type, uint32_t subtype) "%s index %d, %08x/%08x"
 vfio_dma_unmap_overflow_workaround(void) ""
-vfio_get_dirty_bitmap(int fd, uint64_t iova, uint64_t size, uint64_t bitmap_size, uint64_t start) "container fd=%d, iova=0x%"PRIx64" size= 0x%"PRIx64" bitmap_size=0x%"PRIx64" start=0x%"PRIx64
+vfio_get_dirty_bitmap(int fd, uint64_t iova, uint64_t size, uint64_t bitmap_size, uint64_t start, uint64_t dirty) "container fd=%d, iova=0x%"PRIx64" size= 0x%"PRIx64" bitmap_size=0x%"PRIx64" start=0x%"PRIx64" dirty=%"PRIu64
 vfio_iommu_map_dirty_notify(uint64_t iova_start, uint64_t iova_end) "iommu dirty @ 0x%"PRIx64" - 0x%"PRIx64
 
 # platform.c
-- 
2.31.1

Re: [PATCH v1 1/2] exec/ram_addr: add dirty arg to cpu_physical_memory_set_dirty_lebitmap()

[Cédric Le Goater]

On 5/23/23 17:12, Joao Martins wrote:
> In preparation to including the number of dirty pages in the
> vfio_get_dirty_bitmap() tracepoint, return the number of dirty pages in the
> range passed by @dirty argument in cpu_physical_memory_set_dirty_lebitmap().
> 
> For now just set the callers to NULL, no functional change intended.

Why not return the number of dirty pages to be consistent with
cpu_physical_memory_sync_dirty_bitmap () ?

bitmap_count_one() would also give the same result but at the cost
of extra loops.

Thanks,

C.

> 
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> ---
>   accel/kvm/kvm-all.c     |  2 +-
>   hw/vfio/common.c        |  4 ++--
>   include/exec/ram_addr.h | 10 +++++++++-
>   3 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index cf3a88d90e92..1524a34f1786 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -559,7 +559,7 @@ static void kvm_slot_sync_dirty_pages(KVMSlot *slot)
>       ram_addr_t start = slot->ram_start_offset;
>       ram_addr_t pages = slot->memory_size / qemu_real_host_page_size();
>   
> -    cpu_physical_memory_set_dirty_lebitmap(slot->dirty_bmap, start, pages);
> +    cpu_physical_memory_set_dirty_lebitmap(slot->dirty_bmap, start, pages, NULL);
>   }
>   
>   static void kvm_slot_reset_dirty_pages(KVMSlot *slot)
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 78358ede2764..dcbf7c574d85 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -587,7 +587,7 @@ static int vfio_dma_unmap_bitmap(VFIOContainer *container,
>       ret = ioctl(container->fd, VFIO_IOMMU_UNMAP_DMA, unmap);
>       if (!ret) {
>           cpu_physical_memory_set_dirty_lebitmap(vbmap.bitmap,
> -                iotlb->translated_addr, vbmap.pages);
> +                iotlb->translated_addr, vbmap.pages, NULL);
>       } else {
>           error_report("VFIO_UNMAP_DMA with DIRTY_BITMAP : %m");
>       }
> @@ -1773,7 +1773,7 @@ static int vfio_get_dirty_bitmap(VFIOContainer *container, uint64_t iova,
>       }
>   
>       cpu_physical_memory_set_dirty_lebitmap(vbmap.bitmap, ram_addr,
> -                                           vbmap.pages);
> +                                           vbmap.pages, NULL);
>   
>       trace_vfio_get_dirty_bitmap(container->fd, iova, size, vbmap.size,
>                                   ram_addr);
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index f4fb6a211175..07bf9e1502b6 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -335,7 +335,8 @@ static inline void cpu_physical_memory_set_dirty_range(ram_addr_t start,
>   #if !defined(_WIN32)
>   static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
>                                                             ram_addr_t start,
> -                                                          ram_addr_t pages)
> +                                                          ram_addr_t pages,
> +                                                          unsigned long *dirty)
>   {
>       unsigned long i, j;
>       unsigned long page_number, c;
> @@ -380,6 +381,10 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
>                           }
>                       }
>   
> +                    if (dirty) {
> +                        *dirty += ctpopl(temp);
> +                    }
> +
>                       if (tcg_enabled()) {
>                           qatomic_or(&blocks[DIRTY_MEMORY_CODE][idx][offset],
>                                      temp);
> @@ -411,6 +416,9 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
>                   if (unlikely(global_dirty_tracking & GLOBAL_DIRTY_DIRTY_RATE)) {
>                       total_dirty_pages += ctpopl(c);
>                   }
> +                if (dirty) {
> +                    *dirty += ctpopl(c);
> +                }
>                   do {
>                       j = ctzl(c);
>                       c &= ~(1ul << j);

Re: [PATCH v1 1/2] exec/ram_addr: add dirty arg to cpu_physical_memory_set_dirty_lebitmap()

[Joao Martins]

On 23/05/2023 17:07, Cédric Le Goater wrote:
> On 5/23/23 17:12, Joao Martins wrote:
>> In preparation to including the number of dirty pages in the
>> vfio_get_dirty_bitmap() tracepoint, return the number of dirty pages in the
>> range passed by @dirty argument in cpu_physical_memory_set_dirty_lebitmap().
>>
>> For now just set the callers to NULL, no functional change intended.
> 
> Why not return the number of dirty pages to be consistent with
> cpu_physical_memory_sync_dirty_bitmap () ?
> 

Great idea, let me switch to that. Didn't realize sync variant was using the
return value as dirty pages. That also avoids some churn in the current callers.

> bitmap_count_one() would also give the same result but at the cost
> of extra loops.
> 
> Thanks,
> 
> C.
> 
>>
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> ---
>>   accel/kvm/kvm-all.c     |  2 +-
>>   hw/vfio/common.c        |  4 ++--
>>   include/exec/ram_addr.h | 10 +++++++++-
>>   3 files changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index cf3a88d90e92..1524a34f1786 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -559,7 +559,7 @@ static void kvm_slot_sync_dirty_pages(KVMSlot *slot)
>>       ram_addr_t start = slot->ram_start_offset;
>>       ram_addr_t pages = slot->memory_size / qemu_real_host_page_size();
>>   -    cpu_physical_memory_set_dirty_lebitmap(slot->dirty_bmap, start, pages);
>> +    cpu_physical_memory_set_dirty_lebitmap(slot->dirty_bmap, start, pages,
>> NULL);
>>   }
>>     static void kvm_slot_reset_dirty_pages(KVMSlot *slot)
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 78358ede2764..dcbf7c574d85 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -587,7 +587,7 @@ static int vfio_dma_unmap_bitmap(VFIOContainer *container,
>>       ret = ioctl(container->fd, VFIO_IOMMU_UNMAP_DMA, unmap);
>>       if (!ret) {
>>           cpu_physical_memory_set_dirty_lebitmap(vbmap.bitmap,
>> -                iotlb->translated_addr, vbmap.pages);
>> +                iotlb->translated_addr, vbmap.pages, NULL);
>>       } else {
>>           error_report("VFIO_UNMAP_DMA with DIRTY_BITMAP : %m");
>>       }
>> @@ -1773,7 +1773,7 @@ static int vfio_get_dirty_bitmap(VFIOContainer
>> *container, uint64_t iova,
>>       }
>>         cpu_physical_memory_set_dirty_lebitmap(vbmap.bitmap, ram_addr,
>> -                                           vbmap.pages);
>> +                                           vbmap.pages, NULL);
>>         trace_vfio_get_dirty_bitmap(container->fd, iova, size, vbmap.size,
>>                                   ram_addr);
>> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
>> index f4fb6a211175..07bf9e1502b6 100644
>> --- a/include/exec/ram_addr.h
>> +++ b/include/exec/ram_addr.h
>> @@ -335,7 +335,8 @@ static inline void
>> cpu_physical_memory_set_dirty_range(ram_addr_t start,
>>   #if !defined(_WIN32)
>>   static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long
>> *bitmap,
>>                                                             ram_addr_t start,
>> -                                                          ram_addr_t pages)
>> +                                                          ram_addr_t pages,
>> +                                                          unsigned long *dirty)
>>   {
>>       unsigned long i, j;
>>       unsigned long page_number, c;
>> @@ -380,6 +381,10 @@ static inline void
>> cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
>>                           }
>>                       }
>>   +                    if (dirty) {
>> +                        *dirty += ctpopl(temp);
>> +                    }
>> +
>>                       if (tcg_enabled()) {
>>                           qatomic_or(&blocks[DIRTY_MEMORY_CODE][idx][offset],
>>                                      temp);
>> @@ -411,6 +416,9 @@ static inline void
>> cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
>>                   if (unlikely(global_dirty_tracking &
>> GLOBAL_DIRTY_DIRTY_RATE)) {
>>                       total_dirty_pages += ctpopl(c);
>>                   }
>> +                if (dirty) {
>> +                    *dirty += ctpopl(c);
>> +                }
>>                   do {
>>                       j = ctzl(c);
>>                       c &= ~(1ul << j);
>