Re: [PATCH v3 19/19] iommu/intel: Access/Dirty bit support for SL domains

[Jason Gunthorpe <jgg@nvidia.com>]

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