On 20/03/2025 11:13, Avihai Horon wrote:
>
> On 19/03/2025 14:21, Joao Martins wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On 18/03/2025 09:54, Cédric Le Goater wrote:
>>> Rename these routines :
>>>
>>> vfio_devices_all_device_dirty_tracking_started ->
>>> vfio_dirty_tracking_devices_is_started_all
>>> vfio_devices_all_dirty_tracking_started ->
>>> vfio_dirty_tracking_devices_is_started
>>> vfio_devices_all_device_dirty_tracking ->
>>> vfio_dirty_tracking_devices_is_supported
>>> vfio_devices_dma_logging_start ->
>>> vfio_dirty_tracking_devices_dma_logging_start
>>> vfio_devices_dma_logging_stop ->
>>> vfio_dirty_tracking_devices_dma_logging_stop
>>> vfio_devices_query_dirty_bitmap ->
>>> vfio_dirty_tracking_devices_query_dirty_bitmap
>>> vfio_get_dirty_bitmap ->
>>> vfio_dirty_tracking_query_dirty_bitmap
>>>
>>> to better reflect the namespace they belong to.
>>>
>>> Signed-off-by: Cédric Le Goater <c...@redhat.com>
>> The change itself is fine.
>>
>> But on the other hand, it looks relatively long names, no? I am bit at two
>> minds
>> (as I generally prefer shorter code), but I can't find any alternatives if
>> you
>> really wanna have one namespaces associated with the subsystem:file as a C
>> namespace.
>>
>> Every once and a while me and Avihai use the acronym DPT (Dirty Page
>> Tracking)
>> when talking about this stuff, but it seems a detour from the code style to
>> abbreviate namespaces into acronyms.
>>
>> Having said that:
>>
>> Reviewed-by: Joao Martins <joao.m.mart...@oracle.com>
>>
>> P.S. We could also remove 'devices' as the prefix for VF dirty tracking after
>> namespace, and thus drop 'dma logging'. That should put 'start/stop' a little
>> shorter.
>
> This file is not related only to dirty tracking, but to memory in general.
> Maybe a better naming would be memory.{c,h}?
> Then we can have vfio_memory_* or vfio_mem_* prefix and rename to the below:>
> vfio_devices_all_device_dirty_tracking_started ->
> vfio_mem_device_dpt_is_started
> vfio_devices_all_dirty_tracking_started -> vfio_mem_dpt_is_started
> vfio_devices_all_device_dirty_tracking ->
> vfio_mem_device_dpt_is_supported
> vfio_devices_dma_logging_start -> vfio_mem_device_dpt_start
> vfio_devices_dma_logging_stop -> vfio_mem_device_dpt_stop
> vfio_devices_query_dirty_bitmap -> vfio_mem_device_dpt_query
> vfio_get_dirty_bitmap -> vfio_mem_dpt_query
>
> dpt can be changed to dirty_tracking if that's clearer and not too long.
> In patch #31 we can rename to vfio_mem_{register,unregister} or
> vfio_mem_listener_{register,unregister}.
> More internal functions can be gradually renamed and added the vfio_mem_*
> prefix.
>
> Will that work?
>
I would associate to memory if we were talking about Host windows, DMA mapping
and etc. I believe that's more fundamentally related to memory handling of VFIO
to justify said prefix.
Here the code Cedric moved is really about dirty page tracking, or tracking
changes made by VFs to memory. Calling it memory.c would be a bit of a misnomer
IMHO :(
> Thanks.
>
>>> ---
>>> hw/vfio/dirty-tracking.h | 6 +++---
>>> hw/vfio/container.c | 6 +++---
>>> hw/vfio/dirty-tracking.c | 44 ++++++++++++++++++++--------------------
>>> hw/vfio/trace-events | 2 +-
>>> 4 files changed, 29 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/hw/vfio/dirty-tracking.h b/hw/vfio/dirty-tracking.h
>>> index
>>> 322af30b0d5370600719594d4aed4c407f7d2295..db9494202a780108ce78b642950bfed78ba3f253
>>> 100644
>>> --- a/hw/vfio/dirty-tracking.h
>>> +++ b/hw/vfio/dirty-tracking.h
>>> @@ -11,9 +11,9 @@
>>>
>>> extern const MemoryListener vfio_memory_listener;
>>>
>>> -bool vfio_devices_all_dirty_tracking_started(const VFIOContainerBase
>>> *bcontainer);
>>> -bool vfio_devices_all_device_dirty_tracking(const VFIOContainerBase
>>> *bcontainer);
>>> -int vfio_get_dirty_bitmap(const VFIOContainerBase *bcontainer, uint64_t
>>> iova,
>>> +bool vfio_dirty_tracking_devices_is_started(const VFIOContainerBase
>>> *bcontainer);
>>> +bool vfio_dirty_tracking_devices_is_supported(const VFIOContainerBase
>>> *bcontainer);
>>> +int vfio_dirty_tracking_query_dirty_bitmap(const VFIOContainerBase
>>> *bcontainer, uint64_t iova,
>>> uint64_t size, ram_addr_t ram_addr, Error
>>> **errp);
>>>
>>> #endif /* HW_VFIO_DIRTY_TRACKING_H */
>>> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>>> index
>>> 40d6c1fecbf9c37c22b8c19f8e9e8b6c5c381249..7b3ec798a77052b8cb0b47d3dceaca1428cb50bd
>>> 100644
>>> --- a/hw/vfio/container.c
>>> +++ b/hw/vfio/container.c
>>> @@ -138,8 +138,8 @@ static int vfio_legacy_dma_unmap(const VFIOContainerBase
>>> *bcontainer,
>>> int ret;
>>> Error *local_err = NULL;
>>>
>>> - if (iotlb && vfio_devices_all_dirty_tracking_started(bcontainer)) {
>>> - if (!vfio_devices_all_device_dirty_tracking(bcontainer) &&
>>> + if (iotlb && vfio_dirty_tracking_devices_is_started(bcontainer)) {
>>> + if (!vfio_dirty_tracking_devices_is_supported(bcontainer) &&
>>> bcontainer->dirty_pages_supported) {
>>> return vfio_dma_unmap_bitmap(container, iova, size, iotlb);
>>> }
>>> @@ -170,7 +170,7 @@ static int vfio_legacy_dma_unmap(const VFIOContainerBase
>>> *bcontainer,
>>> }
>>>
>>> if (need_dirty_sync) {
>>> - ret = vfio_get_dirty_bitmap(bcontainer, iova, size,
>>> + ret = vfio_dirty_tracking_query_dirty_bitmap(bcontainer, iova,
>>> size,
>>> iotlb->translated_addr, &local_err);
>>> if (ret) {
>>> error_report_err(local_err);
>>> diff --git a/hw/vfio/dirty-tracking.c b/hw/vfio/dirty-tracking.c
>>> index
>>> 9b20668a6d0df93a2cfde12d9a5cd7c223ae3ca1..8e47ccbb9aea748e57271508ddcd10e394abf16c
>>> 100644
>>> --- a/hw/vfio/dirty-tracking.c
>>> +++ b/hw/vfio/dirty-tracking.c
>>> @@ -45,7 +45,7 @@
>>> * Device state interfaces
>>> */
>>>
>>> -static bool vfio_devices_all_device_dirty_tracking_started(
>>> +static bool vfio_dirty_tracking_devices_is_started_all(
>>> const VFIOContainerBase *bcontainer)
>>> {
>>> VFIODevice *vbasedev;
>>> @@ -59,10 +59,9 @@ static bool
>>> vfio_devices_all_device_dirty_tracking_started(
>>> return true;
>>> }
>>>
>>> -bool vfio_devices_all_dirty_tracking_started(
>>> - const VFIOContainerBase *bcontainer)
>>> +bool vfio_dirty_tracking_devices_is_started(const VFIOContainerBase
>>> *bcontainer)
>>> {
>>> - return vfio_devices_all_device_dirty_tracking_started(bcontainer) ||
>>> + return vfio_dirty_tracking_devices_is_started_all(bcontainer) ||
>>> bcontainer->dirty_pages_started;
>>> }
>>>
>>> @@ -70,7 +69,7 @@ static bool vfio_log_sync_needed(const VFIOContainerBase
>>> *bcontainer)
>>> {
>>> VFIODevice *vbasedev;
>>>
>>> - if (!vfio_devices_all_dirty_tracking_started(bcontainer)) {
>>> + if (!vfio_dirty_tracking_devices_is_started(bcontainer)) {
>>> return false;
>>> }
>>>
>>> @@ -90,7 +89,7 @@ static bool vfio_log_sync_needed(const VFIOContainerBase
>>> *bcontainer)
>>> return true;
>>> }
>>>
>>> -bool vfio_devices_all_device_dirty_tracking(const VFIOContainerBase
>>> *bcontainer)
>>> +bool vfio_dirty_tracking_devices_is_supported(const VFIOContainerBase
>>> *bcontainer)
>>> {
>>> VFIODevice *vbasedev;
>>>
>>> @@ -809,7 +808,7 @@ static void vfio_dirty_tracking_init(VFIOContainerBase
>>> *bcontainer,
>>> memory_listener_unregister(&dirty.listener);
>>> }
>>>
>>> -static void vfio_devices_dma_logging_stop(VFIOContainerBase *bcontainer)
>>> +static void vfio_dirty_tracking_devices_dma_logging_stop(VFIOContainerBase
>>> *bcontainer)
>>> {
>>> uint64_t buf[DIV_ROUND_UP(sizeof(struct vfio_device_feature),
>>> sizeof(uint64_t))] = {};
>>> @@ -907,7 +906,7 @@ static void
>>> vfio_device_feature_dma_logging_start_destroy(
>>> g_free(feature);
>>> }
>>>
>>> -static bool vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer,
>>> +static bool vfio_dirty_tracking_devices_dma_logging_start(VFIOContainerBase
>>> *bcontainer,
>>> Error **errp)
>>> {
>>> struct vfio_device_feature *feature;
>>> @@ -940,7 +939,7 @@ static bool
>>> vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer,
>>>
>>> out:
>>> if (ret) {
>>> - vfio_devices_dma_logging_stop(bcontainer);
>>> + vfio_dirty_tracking_devices_dma_logging_stop(bcontainer);
>>> }
>>>
>>> vfio_device_feature_dma_logging_start_destroy(feature);
>>> @@ -956,8 +955,8 @@ static bool
>>> vfio_listener_log_global_start(MemoryListener
>>> *listener,
>>> listener);
>>> bool ret;
>>>
>>> - if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
>>> - ret = vfio_devices_dma_logging_start(bcontainer, errp);
>>> + if (vfio_dirty_tracking_devices_is_supported(bcontainer)) {
>>> + ret = vfio_dirty_tracking_devices_dma_logging_start(bcontainer,
>>> errp);
>>> } else {
>>> ret = vfio_container_set_dirty_page_tracking(bcontainer, true,
>>> errp) == 0;
>>> }
>>> @@ -975,8 +974,8 @@ static void vfio_listener_log_global_stop(MemoryListener
>>> *listener)
>>> Error *local_err = NULL;
>>> int ret = 0;
>>>
>>> - if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
>>> - vfio_devices_dma_logging_stop(bcontainer);
>>> + if (vfio_dirty_tracking_devices_is_supported(bcontainer)) {
>>> + vfio_dirty_tracking_devices_dma_logging_stop(bcontainer);
>>> } else {
>>> ret = vfio_container_set_dirty_page_tracking(bcontainer, false,
>>> &local_err);
>>> @@ -1016,7 +1015,7 @@ static int vfio_device_dma_logging_report(VFIODevice
>>> *vbasedev, hwaddr iova,
>>> return 0;
>>> }
>>>
>>> -static int vfio_devices_query_dirty_bitmap(const VFIOContainerBase
>>> *bcontainer,
>>> +static int vfio_dirty_tracking_devices_query_dirty_bitmap(const
>>> VFIOContainerBase *bcontainer,
>>> VFIOBitmap *vbmap, hwaddr iova, hwaddr size, Error
>>> **errp)
>>> {
>>> VFIODevice *vbasedev;
>>> @@ -1038,11 +1037,11 @@ static int vfio_devices_query_dirty_bitmap(const
>>> VFIOContainerBase *bcontainer,
>>> return 0;
>>> }
>>>
>>> -int vfio_get_dirty_bitmap(const VFIOContainerBase *bcontainer, uint64_t
>>> iova,
>>> +int vfio_dirty_tracking_query_dirty_bitmap(const VFIOContainerBase
>>> *bcontainer, uint64_t iova,
>>> uint64_t size, ram_addr_t ram_addr, Error
>>> **errp)
>>> {
>>> bool all_device_dirty_tracking =
>>> - vfio_devices_all_device_dirty_tracking(bcontainer);
>>> + vfio_dirty_tracking_devices_is_supported(bcontainer);
>>> uint64_t dirty_pages;
>>> VFIOBitmap vbmap;
>>> int ret;
>>> @@ -1062,8 +1061,8 @@ int vfio_get_dirty_bitmap(const VFIOContainerBase
>>> *bcontainer, uint64_t iova,
>>> }
>>>
>>> if (all_device_dirty_tracking) {
>>> - ret = vfio_devices_query_dirty_bitmap(bcontainer, &vbmap, iova,
>>> size,
>>> - errp);
>>> + ret = vfio_dirty_tracking_devices_query_dirty_bitmap(bcontainer,
>>> &vbmap,
>>> + iova, size,
>>> errp);
>>> } else {
>>> ret = vfio_container_query_dirty_bitmap(bcontainer, &vbmap, iova,
>>> size,
>>> errp);
>>> @@ -1076,7 +1075,8 @@ int vfio_get_dirty_bitmap(const VFIOContainerBase
>>> *bcontainer, uint64_t iova,
>>> dirty_pages = cpu_physical_memory_set_dirty_lebitmap(vbmap.bitmap,
>>> ram_addr,
>>> vbmap.pages);
>>>
>>> - trace_vfio_get_dirty_bitmap(iova, size, vbmap.size, ram_addr,
>>> dirty_pages);
>>> + trace_vfio_dirty_tracking_query_dirty_bitmap(iova, size, vbmap.size,
>>> ram_addr,
>>> + dirty_pages);
>>> out:
>>> g_free(vbmap.bitmap);
>>>
>>> @@ -1113,7 +1113,7 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier
>>> *n, IOMMUTLBEntry *iotlb)
>>> goto out_unlock;
>>> }
>>>
>>> - ret = vfio_get_dirty_bitmap(bcontainer, iova, iotlb->addr_mask + 1,
>>> + ret = vfio_dirty_tracking_query_dirty_bitmap(bcontainer, iova, iotlb-
>>> >addr_mask + 1,
>>> translated_addr, &local_err);
>>> if (ret) {
>>> error_prepend(&local_err,
>>> @@ -1147,7 +1147,7 @@ static int
>>> vfio_ram_discard_get_dirty_bitmap(MemoryRegionSection *section,
>>> * Sync the whole mapped region (spanning multiple individual
>>> mappings)
>>> * in one go.
>>> */
>>> - ret = vfio_get_dirty_bitmap(vrdl->bcontainer, iova, size, ram_addr,
>>> + ret = vfio_dirty_tracking_query_dirty_bitmap(vrdl->bcontainer, iova,
>>> size, ram_addr,
>>> &local_err);
>>> if (ret) {
>>> error_report_err(local_err);
>>> @@ -1241,7 +1241,7 @@ static int vfio_sync_dirty_bitmap(VFIOContainerBase
>>> *bcontainer,
>>> ram_addr = memory_region_get_ram_addr(section->mr) +
>>> section->offset_within_region;
>>>
>>> - return vfio_get_dirty_bitmap(bcontainer,
>>> + return vfio_dirty_tracking_query_dirty_bitmap(bcontainer,
>>>
>>> REAL_HOST_PAGE_ALIGN(section->offset_within_address_space),
>>> int128_get64(section->size), ram_addr,
>>> errp);
>>> }
>>> diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
>>> index
>>> 512f4913b72d9a1e8a04df24318a4947fa361e28..6cf8ec3530c68e7528eefa80b7c8ecb401319f88
>>> 100644
>>> --- a/hw/vfio/trace-events
>>> +++ b/hw/vfio/trace-events
>>> @@ -100,7 +100,7 @@ vfio_listener_region_del(uint64_t start, uint64_t end)
>>> "region_del 0x%"PRIx64" -
>>> vfio_device_dirty_tracking_update(uint64_t start, uint64_t end, uint64_t
>>> min, uint64_t max) "section 0x%"PRIx64" - 0x%"PRIx64" -> update [0x%"PRIx64"
>>> - 0x%"PRIx64"]"
>>> vfio_device_dirty_tracking_start(int nr_ranges, uint64_t min32, uint64_t
>>> max32, uint64_t min64, uint64_t max64, uint64_t minpci, uint64_t maxpci)
>>> "nr_ranges %d 32:[0x%"PRIx64" - 0x%"PRIx64"], 64:[0x%"PRIx64" -
>>> 0x%"PRIx64"],
>>> pci64:[0x%"PRIx64" - 0x%"PRIx64"]"
>>> vfio_legacy_dma_unmap_overflow_workaround(void) ""
>>> -vfio_get_dirty_bitmap(uint64_t iova, uint64_t size, uint64_t bitmap_size,
>>> uint64_t start, uint64_t dirty_pages) "iova=0x%"PRIx64" size= 0x%"PRIx64"
>>> bitmap_size=0x%"PRIx64" start=0x%"PRIx64" dirty_pages=%"PRIu64
>>> +vfio_dirty_tracking_query_dirty_bitmap(uint64_t iova, uint64_t size,
>>> uint64_t bitmap_size, uint64_t start, uint64_t dirty_pages)
>>> "iova=0x%"PRIx64"
>>> size= 0x%"PRIx64" bitmap_size=0x%"PRIx64" start=0x%"PRIx64"
>>> dirty_pages=%"PRIu64
>>> vfio_iommu_map_dirty_notify(uint64_t iova_start, uint64_t iova_end) "iommu
>>> dirty @ 0x%"PRIx64" - 0x%"PRIx64
>>>
>>> # region.c
