Re: [PATCH 3/5] vfio/iommufd: Add IOMMU_HWPT_GET_DIRTY_BITMAP_NO_CLEAR flag support

["Cédric Le Goater" <clg@redhat.com>]

Hello Zhenzhong, Joao,

On 10/9/25 12:20, Duan, Zhenzhong wrote:
> Hi Cédric,
> 
>> -----Original Message-----
>> From: Cédric Le Goater <clg@redhat.com>
>> 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 <joao.m.martins@oracle.com>
>>>> 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 <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/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.