>-----Original Message----- >From: Joao Martins <[email protected]> >Subject: Re: [PATCH 3/5] vfio/iommufd: Add >IOMMU_HWPT_GET_DIRTY_BITMAP_NO_CLEAR flag support > >On 23/09/2025 03:50, Duan, Zhenzhong wrote: >>> -----Original Message----- >>> From: Joao Martins <[email protected]> >>> Subject: Re: [PATCH 3/5] vfio/iommufd: Add >>> IOMMU_HWPT_GET_DIRTY_BITMAP_NO_CLEAR flag support >>> >>> On 22/09/2025 17:06, Joao Martins wrote: >>>> On 22/09/2025 17:02, Cédric Le Goater wrote: >>>>> On 9/22/25 07:49, Duan, Zhenzhong wrote: >>>>>> Hi Joao, >>>>>> >>>>>>> -----Original Message----- >>>>>>> From: Joao Martins <[email protected]> >>>>>>> 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 <[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/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_DIRT >Y_ >>> 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, >>>>>>>> IOMMUTLBE >ntr >>> y *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/99f83595d79d2e4170c9e456cf1a7b9521bd4f80 >>>>>> 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. >>>>> >>>> >>>> My recollection was that it wasn't that much churn added as this series is >>>> already doing the most of the churn. But I am checking how the code >would >>> look >>>> like to properly respond to his suggestion on why he changing it towards >>>> structuref field. >>> >>> The churn should be similar to this patch: >>> >>> >https://github.com/jpemartins/qemu/commit/5e1f11075299a5fa9564a26788 >>> dc9cc1717e297c >>> >>> It's mostly an interface parameter addition of flags. >> >> Yes, it's a general interface parameter but it's only used by IOMMUFD. >> That's my only argument. >> > >You could use that argument for anything that's not common and unique to >each >backend. type1 dirty tracking is effectively "frozen" in UAPI (IIRC) so I won't >expect flags. There's device dirty tracking, IOMMUFD dirty tracking and >vfio-user dirty tracking. While I am not anticipating any new thing here (at >least in the needs I have ahead, can't speak for other >companies/contributors); >but you could see the flags as future proofing it.
Yes, agree it's future proofing. > >The ugly part of the alternative is that it sort of limits the interface to be >single-user only as nobody call into IOMMUFD with the dirty_tracking_flags if >it's called concurrently for some other purpose than unmap. We sort of use >the >container data structure as a argument storage for a single call as opposed to >store some intermediary state. dirty_tracking_flags only take effect during the unmap path except you take it for other purpose. But yes, I made the change based on the fact that unmap path is protected by BQL, if the concurrent access is allowed in future, this change will break. Thanks Zhenzhong
