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? Why would we want to not clear the dirty bits upon enable/disable if dirty tracking? I can understand that the driver needs help from the caller due to the externalized locking, but do we leak this into the userspace? Jason
