Hello Zhenzhong, Joao,
On 10/9/25 12:20, Duan, Zhenzhong wrote:
Hi Cédric,
-----Original Message-----
From: Cédric Le Goater <[email protected]>
Subject: Re: [PATCH 3/5] vfio/iommufd: Add
IOMMU_HWPT_GET_DIRTY_BITMAP_NO_CLEAR flag support
On 9/22/25 07:49, Duan, Zhenzhong wrote:
Hi Joao,
-----Original Message-----
From: Joao Martins <[email protected]>
Subject: Re: [PATCH 3/5] vfio/iommufd: Add
IOMMU_HWPT_GET_DIRTY_BITMAP_NO_CLEAR flag support
On 10/09/2025 03:36, Zhenzhong Duan wrote:
Pass IOMMU_HWPT_GET_DIRTY_BITMAP_NO_CLEAR when doing the
last
dirty
bitmap query right before unmap, no PTEs flushes. This accelerates the
query without issue because unmap will tear down the mapping anyway.
Add a new element dirty_tracking_flags in VFIOIOMMUFDContainer to
be used for the flags of iommufd dirty tracking. Currently it is
set to either IOMMU_HWPT_GET_DIRTY_BITMAP_NO_CLEAR or 0 based
on
the scenario.
Co-developed-by: Joao Martins <[email protected]>
Signed-off-by: Joao Martins <[email protected]>
Signed-off-by: Zhenzhong Duan <[email protected]>
Tested-by: Xudong Hao <[email protected]>
Tested-by: Giovannio Cabiddu <[email protected]>
---
hw/vfio/vfio-iommufd.h | 1 +
include/system/iommufd.h | 2 +-
backends/iommufd.c | 5 +++--
hw/vfio/iommufd.c | 6 +++++-
backends/trace-events | 2 +-
5 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/hw/vfio/vfio-iommufd.h b/hw/vfio/vfio-iommufd.h
index 07ea0f4304..e0af241c75 100644
--- a/hw/vfio/vfio-iommufd.h
+++ b/hw/vfio/vfio-iommufd.h
@@ -26,6 +26,7 @@ typedef struct VFIOIOMMUFDContainer {
VFIOContainerBase bcontainer;
IOMMUFDBackend *be;
uint32_t ioas_id;
+ uint64_t dirty_tracking_flags;
QLIST_HEAD(, VFIOIOASHwpt) hwpt_list;
} VFIOIOMMUFDContainer;
diff --git a/include/system/iommufd.h b/include/system/iommufd.h
index c9c72ffc45..63898e7b0d 100644
--- a/include/system/iommufd.h
+++ b/include/system/iommufd.h
@@ -64,7 +64,7 @@ bool
iommufd_backend_set_dirty_tracking(IOMMUFDBackend *be, uint32_t
hwpt_id,
bool iommufd_backend_get_dirty_bitmap(IOMMUFDBackend *be,
uint32_t hwpt_id,
uint64_t iova,
ram_addr_t
size,
uint64_t page_size,
uint64_t *data,
- Error **errp);
+ uint64_t flags, Error
**errp);
bool iommufd_backend_invalidate_cache(IOMMUFDBackend *be,
uint32_t id,
uint32_t data_type,
uint32_t entry_len,
uint32_t *entry_num,
void
*data,
diff --git a/backends/iommufd.c b/backends/iommufd.c
index 2a33c7ab0b..3c4f6157e2 100644
--- a/backends/iommufd.c
+++ b/backends/iommufd.c
@@ -361,7 +361,7 @@ bool
iommufd_backend_get_dirty_bitmap(IOMMUFDBackend *be,
uint32_t hwpt_id,
uint64_t iova,
ram_addr_t
size,
uint64_t page_size,
uint64_t *data,
- Error **errp)
+ uint64_t flags, Error
**errp)
{
int ret;
struct iommu_hwpt_get_dirty_bitmap get_dirty_bitmap = {
@@ -371,11 +371,12 @@ bool
iommufd_backend_get_dirty_bitmap(IOMMUFDBackend *be,
.length = size,
.page_size = page_size,
.data = (uintptr_t)data,
+ .flags = flags,
};
ret = ioctl(be->fd, IOMMU_HWPT_GET_DIRTY_BITMAP,
&get_dirty_bitmap);
trace_iommufd_backend_get_dirty_bitmap(be->fd, hwpt_id,
iova,
size,
- page_size, ret ?
errno :
0);
+ flags, page_size,
ret ?
errno : 0);
if (ret) {
error_setg_errno(errp, errno,
"IOMMU_HWPT_GET_DIRTY_BITMAP
(iova: 0x%"HWADDR_PRIx
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index 0057488ce9..c897aa6b17 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -62,7 +62,7 @@ static int iommufd_cdev_unmap_one(const
VFIOContainerBase *bcontainer,
hwaddr iova, ram_addr_t
size,
IOMMUTLBEntry *iotlb)
{
- const VFIOIOMMUFDContainer *container =
+ VFIOIOMMUFDContainer *container =
container_of(bcontainer, VFIOIOMMUFDContainer,
bcontainer);
bool need_dirty_sync = false;
Error *local_err = NULL;
@@ -73,9 +73,12 @@ static int iommufd_cdev_unmap_one(const
VFIOContainerBase *bcontainer,
if (iotlb && vfio_container_dirty_tracking_is_started(bcontainer))
{
if
(!vfio_container_devices_dirty_tracking_is_supported(bcontainer) &&
bcontainer->dirty_pages_supported) {
+ container->dirty_tracking_flags =
+
IOMMU_HWPT_GET_DIRTY_BITMAP_NO_CLEAR;
ret = vfio_container_query_dirty_bitmap(bcontainer,
iova,
size,
iotlb->translated_addr,
&local_err);
+ container->dirty_tracking_flags = 0;
Why not changing vfio_container_query_dirty_bitmap to pass a flags too,
like
the
original patches? This is a little unnecssary odd style to pass a flag via
container structure rather and then clearing.
Just want to be simpler, original patch introduced a new parameter to
almost all
variants of *_query_dirty_bitmap() while the flags parameter is only used by
IOMMUFD backend when doing unmap_bitmap. Currently we already have
three
backends, legacy VFIO, IOMMUFD and VFIO-user, only IOMMUFD need the
flag.
I take container->dirty_tracking_flags as a notification mechanism, so set it
before
vfio_container_query_dirty_bitmap() and clear it thereafter. Maybe clearing
it in
iommufd_query_dirty_bitmap() is easier to be acceptable?
Part of the reason the original series had a VFIO_GET_DIRTY_NO_FLUSH
for
generic
container abstraction was to not mix IOMMUFD UAPI specifics into base
container
API. Then in getting a VFIO_GET_DIRTY_NO_FLUSH, then type1 backend
could just
ignore the flag, while IOMMUFD translates it to
IOMMU_HWPT_GET_DIRTY_BITMAP_NO_CLEAR
I did port original patch
https://github.com/yiliu1765/qemu/commit/99f83595d79d2e4170c9e456cf1
a7b9521bd4f80
But it looks complex to have 'flags' parameter everywhere.
I think I would prefer like Joao to avoid caching information if possible
but I haven't check closely the mess it would introduce in the code. Let
me check.
Kindly ping. Just in case you have further comments on this.
Regarding the whole series,
* [1/5] vfio/iommufd: Add framework code to support getting dirty bitmap before
unmap
Needs refactoring. iommufd_cdev_unmap_one() seems superfluous now ?
* [2/5] vfio/iommufd: Query dirty bitmap before DMA unmap
Looks OK.
In an extra patch, could we rename 'vfio_dma_unmap_bitmap()' to
'vfio_legacy_dma_unmap_get_dirty_bitmap()' ? Helps (me) remember
what it does.
* [3/5] vfio/iommufd: Add IOMMU_HWPT_GET_DIRTY_BITMAP_NO_CLEAR flag support
Sorry, I lost track of the discussion between you and Joao regarding
the new '->dirty_tracking_flags' attribute.
I'd prefer adding a 'backend_flag' parameter, even if it’s currently
only used by IOMMUFD. Since we’re not going to redefine all possible
IOMMU backend flags, we can treat it as an opaque parameter for
the upper container layer and document it as such.
Is that ok for you ? A bit more churn for you but Joao did provide
some parts.
* [4/5] intel_iommu: Optimize unmap_bitmap during migration
Ack by Clément may be ?
* [5/5] vfio/migration: Allow live migration with vIOMMU without VFs
using device dirty tracking
ok.
Thanks,
C.
