On 17/10/2023 14:10, Jason Gunthorpe wrote: > On Tue, Oct 17, 2023 at 12:22:34PM +0100, Joao Martins wrote: >> On 17/10/2023 03:08, Baolu Lu wrote: >>> On 10/17/23 12:00 AM, Joao Martins wrote: >>>>>> The iommu_dirty_bitmap is defined in iommu core. The iommu driver has no >>>>>> need to understand it and check its member anyway. >>>>>> >>>>> (...) The iommu driver has no need to understand it. iommu_dirty_bitmap_record() >>>>> already makes those checks in case there's no iova_bitmap to set bits to. >>>>> >>>> This is all true but the reason I am checking iommu_dirty_bitmap::bitmap is to >>>> essentially not record anything in the iova bitmap and just clear the dirty bits >>>> from the IOPTEs, all when dirty tracking is technically disabled. This is done >>>> internally only when starting dirty tracking, and thus to ensure that we cleanup >>>> all dirty bits before we enable dirty tracking to have a consistent snapshot as >>>> opposed to inheriting dirties from the past. >>> >>> It's okay since it serves a functional purpose. Can you please add some >>> comments around the code to explain the rationale. >>> >> >> I added this comment below: >> >> + /* >> + * IOMMUFD core calls into a dirty tracking disabled domain without an >> + * IOVA bitmap set in order to clean dirty bits in all PTEs that might >> + * have occured when we stopped dirty tracking. This ensures that we >> + * never inherit dirtied bits from a previous cycle. >> + */ >> >> Also fixed an issue where I could theoretically clear the bit with >> IOMMU_NO_CLEAR. Essentially passed the read_and_clear_dirty flags and let >> dma_sl_pte_test_and_clear_dirty() to test and test-and-clear, similar to AMD: > > How does all this work, does this leak into the uapi? UAPI is only ever expected to collect/clear dirty bits while dirty tracking is enabled. And it requires valid bitmaps before it gets to the IOMMU driver. The above where I pass no dirty::bitmap (but with an iotlb_gather) is internal usage only. Open to alternatives if this is prone to audit errors e.g. 1) via the iommu_dirty_bitmap structure, where I add one field which if true then iommufd core is able to call into iommu driver on a "clear IOPTE" manner or 2) via the ::flags ... the thing is that ::flags values is UAPI, so it feels weird to use these flags for internal purposes. With respect to IOMMU_NO_CLEAR that is UAPI (a flag in read-and-clear) where the user fetches bits, but does want to clear the hw IOPTE dirty bit (to avoid the TLB flush). > Why would we > want to not clear the dirty bits upon enable/disable if dirty > tracking? For the unmap-and-read-dirty case. Where you unmap, and want to get the dirty bits, but you don't care to clear them as you will be unmapping. But my comment above is not about that btw. It is just my broken check where either I test for dirty or test-and-clear. That's it. > I can understand that the driver needs help from the caller > due to the externalized locking, but do we leak this into the userspace? AFAICT no for the first. The IOMMU_NO_CLEAR is UAPI. I take it you were talking about the first.
