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);
>