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.. Jason
