On 20/10/2025 15:45, Yi Liu wrote:
External email: Use caution opening links or attachments
On 2025/10/20 18:00, Duan, Zhenzhong wrote:
Hi
-----Original Message-----
From: Avihai Horon <[email protected]>
Subject: Re: [PATCH v2 2/8] vfio/iommufd: Query dirty bitmap before DMA
unmap
Hi,
On 17/10/2025 11:22, Zhenzhong Duan wrote:
External email: Use caution opening links or attachments
When a existing mapping is unmapped, there could already be dirty bits
which need to be recorded before unmap.
If query dirty bitmap fails, we still need to do unmapping or else
there
is stale mapping and it's risky to guest.
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/iommufd.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index 976c0a8814..404e6249ca 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -74,7 +74,13 @@ static int iommufd_cdev_unmap(const
VFIOContainer *bcontainer,
if (iotlb &&
vfio_container_dirty_tracking_is_started(bcontainer)) {
if
(!vfio_container_devices_dirty_tracking_is_supported(bcontainer) &&
bcontainer->dirty_pages_supported) {
- /* TODO: query dirty bitmap before DMA unmap */
+ ret = vfio_container_query_dirty_bitmap(bcontainer, iova,
size,
+
iotlb->translated_addr,
+
&local_err);
+ if (ret) {
+ error_report_err(local_err);
+ }
+ /* Unmap stale mapping even if query dirty bitmap
fails */
return iommufd_backend_unmap_dma(be, ioas_id, iova,
size);
If query dirty bitmap fails, shouldn't we unmap and return the query
bitmap error to fail migration? Otherwise, migration may succeed with
some dirtied pages not being migrated.
Oh, good catch. Will make below change:
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -65,7 +65,7 @@ static int iommufd_cdev_unmap(const VFIOContainer
*bcontainer,
uint32_t ioas_id = container->ioas_id;
bool need_dirty_sync = false;
Error *local_err = NULL;
- int ret;
+ int ret, unmap_ret;
if (unmap_all) {
size = UINT64_MAX;
@@ -82,7 +82,14 @@ static int iommufd_cdev_unmap(const VFIOContainer
*bcontainer,
error_report_err(local_err);
}
/* Unmap stale mapping even if query dirty bitmap fails */
- return iommufd_backend_unmap_dma(be, ioas_id, iova, size);
+ unmap_ret = iommufd_backend_unmap_dma(be, ioas_id, iova,
size);
+
+ /*
+ * If dirty tracking fails, return the failure to VFIO
core to
+ * fail the migration, or else there will be dirty pages
missed
+ * to be migrated.
+ */
+ return unmap_ret ? : ret;
}
do we need a async way to fail migration? This unmap path is not
necessarily in the migration path.
I think in upper layers there is a check, if migration is active then
migration error is set to fail it.
vfio_iommu_map_notify():
...
ret = vfio_container_dma_unmap(bcontainer, iova,
iotlb->addr_mask + 1, iotlb, false);
if (ret) {
error_setg(&local_err,
"vfio_container_dma_unmap(%p, 0x%"HWADDR_PRIx", "
"0x%"HWADDR_PRIx") = %d (%s)",
bcontainer, iova,
iotlb->addr_mask + 1, ret, strerror(-ret));
if (migration_is_running()) {
migration_file_set_error(ret, local_err);
} else {
error_report_err(local_err);
}
}
Thanks.
