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 <joao.m.martins@oracle.com>
Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Tested-by: Xudong Hao <xudong.hao@intel.com>
Tested-by: Giovannio Cabiddu <giovanni.cabiddu@intel.com>
---
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);
}
--
2.47.1On 2025/10/17 16:22, Zhenzhong Duan wrote:
> When a existing mapping is unmapped, there could already be dirty bits
> which need to be recorded before unmap.
s/a/an/
> 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 <joao.m.martins@oracle.com>
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> Tested-by: Xudong Hao <xudong.hao@intel.com>
> Tested-by: Giovannio Cabiddu <giovanni.cabiddu@intel.com>
> ---
> 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);
> }
>
Reviewed-by: Yi Liu <yi.l.liu@intel.com>
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 <joao.m.martins@oracle.com>
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> Tested-by: Xudong Hao <xudong.hao@intel.com>
> Tested-by: Giovannio Cabiddu <giovanni.cabiddu@intel.com>
> ---
> 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.
Thanks.
> }
>
> --
> 2.47.1
>
Hi
>-----Original Message-----
>From: Avihai Horon <avihaih@nvidia.com>
>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 <joao.m.martins@oracle.com>
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> Tested-by: Xudong Hao <xudong.hao@intel.com>
>> Tested-by: Giovannio Cabiddu <giovanni.cabiddu@intel.com>
>> ---
>> 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
On 2025/10/20 18:00, Duan, Zhenzhong wrote:
> Hi
>
>> -----Original Message-----
>> From: Avihai Horon <avihaih@nvidia.com>
>> 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 <joao.m.martins@oracle.com>
>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>> Tested-by: Xudong Hao <xudong.hao@intel.com>
>>> Tested-by: Giovannio Cabiddu <giovanni.cabiddu@intel.com>
>>> ---
>>> 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.
Regards,
Yi Liu
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 <avihaih@nvidia.com>
>>> 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 <joao.m.martins@oracle.com>
>>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>>> Tested-by: Xudong Hao <xudong.hao@intel.com>
>>>> Tested-by: Giovannio Cabiddu <giovanni.cabiddu@intel.com>
>>>> ---
>>>> 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.
On 2025/10/20 21:04, Avihai Horon wrote:
>
> 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 <avihaih@nvidia.com>
>>>> 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 <joao.m.martins@oracle.com>
>>>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>>>> Tested-by: Xudong Hao <xudong.hao@intel.com>
>>>>> Tested-by: Giovannio Cabiddu <giovanni.cabiddu@intel.com>
>>>>> ---
>>>>> 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);
> }
> }
>
got it. thanks. :)
On 10/17/25 10:22, Zhenzhong Duan wrote:
> 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 <joao.m.martins@oracle.com>
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> Tested-by: Xudong Hao <xudong.hao@intel.com>
> Tested-by: Giovannio Cabiddu <giovanni.cabiddu@intel.com>
> ---
> 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);
> }
>
Reviewed-by: Cédric Le Goater <clg@redhat.com>
Thanks,
C.
© 2016 - 2025 Red Hat, Inc.