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 <clg@redhat.com>
---
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
--
2.48.1
On Tue, Mar 18, 2025 at 10:54:13AM +0100, 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 <clg@redhat.com> Reviewed-by: John Levon <john.levon@nutanix.com> regards john
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 <clg@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.martins@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.
> ---
> 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
On 3/19/25 13:21, Joao Martins wrote: > 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 <clg@redhat.com> > > The change itself is fine. > > But on the other hand, it looks relatively long names, no? I agree. > 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. I am ok to use a TLA for Dirty Page Tracking. Would DPT statisfy everyone ? > > Having said that: > > Reviewed-by: Joao Martins <joao.m.martins@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. Could you please send your proposal as a list, like the commit log does and let's discuss. Thanks, C.
On 21/03/2025 11:22, Cédric Le Goater wrote: > On 3/19/25 13:21, Joao Martins wrote: >> 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 <clg@redhat.com> >> >> The change itself is fine. >> >> But on the other hand, it looks relatively long names, no? > > I agree. > >> 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. > > I am ok to use a TLA for Dirty Page Tracking. Would DPT statisfy everyone ? > It would, but the new version looks shorter so maybe we don't need to go against style with TLAs. >> >> Having said that: >> >> Reviewed-by: Joao Martins <joao.m.martins@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. > > Could you please send your proposal as a list, like the commit log does > and let's discuss. The idea was : vfio_dirty_tracking_devices_dma_logging_start -> vfio_container_dma_logging_start vfio_dirty_tracking_devices_dma_logging_stop -> vfio_container_dma_logging_stop But honestly, doesn't make that much of the difference. I'll have a look at your v2 hopefully tomorrow or Wednesday on the device dirty tracking parts and iommufd dirty tracking.
On 3/31/25 14:49, Joao Martins wrote: > On 21/03/2025 11:22, Cédric Le Goater wrote: >> On 3/19/25 13:21, Joao Martins wrote: >>> 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 <clg@redhat.com> >>> >>> The change itself is fine. >>> >>> But on the other hand, it looks relatively long names, no? >> >> I agree. >> >>> 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. >> >> I am ok to use a TLA for Dirty Page Tracking. Would DPT statisfy everyone ? >> > > It would, but the new version looks shorter so maybe we don't need to go against > style with TLAs. > >>> >>> Having said that: >>> >>> Reviewed-by: Joao Martins <joao.m.martins@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. >> >> Could you please send your proposal as a list, like the commit log does >> and let's discuss. > > The idea was : > > vfio_dirty_tracking_devices_dma_logging_start -> > vfio_container_dma_logging_start > vfio_dirty_tracking_devices_dma_logging_stop -> > vfio_container_dma_logging_stop > > But honestly, doesn't make that much of the difference. > > I'll have a look at your v2 hopefully tomorrow or Wednesday on the device dirty > tracking parts and iommufd dirty tracking. Thanks, there should be progress in v2. Patch 1~25 should be ready for vfio-next. I can just apply them and we can continue working on the final part. Feel free to propose changes. C. >
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 <clg@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.martins@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?
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
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 <clg@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.martins@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
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 <clg@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.martins@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.
>
>> 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
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 <clg@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.martins@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
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().
>>
>>> 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
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 <clg@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.martins@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 no
strong 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
© 2016 - 2025 Red Hat, Inc.