Re: [PATCH v2 2/8] vfio/iommufd: Query dirty bitmap before DMA unmap

[Yi Liu <yi.l.liu@intel.com>]

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