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;
}
Thanks
Zhenzhong
