Re: [PATCH for-10.1 30/32] vfio: Rename VFIO dirty tracking services

Avihai Horon Thu, 20 Mar 2025 05:30:14 -0700


On 20/03/2025 13:56, Joao Martins wrote:

External email: Use caution opening links or attachments


On 20/03/2025 11:45, Avihai Horon wrote:

On 20/03/2025 13:18, Joao Martins wrote:

External email: Use caution opening links or attachments


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 :(

Hmm, yes, the majority of the code is related to dirty tracking, but maybe we
can view dirty tracking as a sub-field of memory.
Dirty tracking doesn't seem the perfect fit IMHO, as this file also
contains vfio_dirty_tracking_register and .region_add/.region_del which are not
entirely related to dirty tracking.

Ah yes, it's a small portion but still region_{add,del} is indeed about DMA
mapping and not at all related to dirty tracking.

It's almost as if we should be moving ::region_add/region_del alongside
vfio_dirty_tracking_{un,}register into a memory.c file and leave this one as
dirty_tracking.c / dpt.c


Yes, this could also work.

Need to consider if it's worth the additional split churn. I have nostrong opinion.


Which reminds me that perhaps vfio_dirty_tracking_register() and the name might
be misleading and should instead me vfio_memory_register() /
vfio_memory_unregister().


Indeed.

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

Re: [PATCH for-10.1 30/32] vfio: Rename VFIO dirty tracking services

Reply via email to