On 17/10/2023 16:31, Jason Gunthorpe wrote: > On Tue, Oct 17, 2023 at 03:11:46PM +0100, Joao Martins wrote: >> 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. > > I think NULL to mean clear but not record is OK, it doesn't matter too > much but ideally this would be sort of hidden in the iova APIs.. And it is hidden? unless you mean by hidden that there's explicit IOVA APIs that do this? Currently, iopt_clear_dirty_data() does this 'internal-only' usage of iommu_read_and_clear() and does this stuff.
