[PATCH v3 2/4] iommu/io-pgtable-arm: Add read_and_clear_dirty() support

[Joao Martins]

On 22/05/2024 17:56, Jason Gunthorpe wrote:
> On Wed, May 22, 2024 at 03:37:57PM +0100, Joao Martins wrote:
>  
>> This is just to catch the case where IOMMUFD can call into read_and_clear()
>> without dirty tracking enabled and without a bitmap structure to clear dirty
>> bits -- in order to ensure a clean PTE data snapshot after start(). 
> 
> Is that broken then?
> 

It's not: The check errors out the caller ends up calling read-and-clear with a
bitmap but without having started dirty tracking. the iopt_clear_dirty_data()
passes a null bitmap, it goes through and it walks and clears the IOPTEs
*without* recording them in the bitmap.

> iopt_clear_dirty_data() sets the NULL:
> 
> 	iommu_dirty_bitmap_init(&dirty, NULL, &gather);
> 		ret = ops->read_and_clear_dirty(domain, iopt_area_iova(area),
> 						iopt_area_length(area), 0,
> 						&dirty);
> 
Right

> But AMD, for instance, does nothing:
> 
> 	spin_lock_irqsave(&pdomain->lock, lflags);
> 	if (!pdomain->dirty_tracking && dirty->bitmap) {
> 		spin_unlock_irqrestore(&pdomain->lock, lflags);
> 		return -EINVAL;
> 	}
> 	spin_unlock_irqrestore(&pdomain->lock, lflags);
> 
> 	return ops->read_and_clear_dirty(ops, iova, size, flags, dirty);
> 
Same for Intel (except it has a comment explaining the why).

But it's on purpose.

> It certainly didn't clear the IOPTEs.
>
But notice that this gets called *without* dirty::bitmap set so it goes through
and walk the page tables and clear the dirty bits. It just doesn't record in the
bitmap.

> AFAIK the NULL should be ignored here:
> 
> static inline void iommu_dirty_bitmap_record(struct iommu_dirty_bitmap *dirty,
> 					     unsigned long iova,
> 					     unsigned long length)
> {
> 	if (dirty->bitmap)
> 		iova_bitmap_set(dirty->bitmap, iova, length);
>

Right, yes. As intended. When we pass NULL bitmap we mean that we don't care
about dirty info. But you are still clearing the IOPTEs in the driver.

> Not above. That looks like a bug. Yes?
> 
It's not a bug.

It might be too subtle or maybe strange -- I recall raising this as not being
too obvious IIRC.

All this doing was to allow read_and_clear iommu driver to clear the bits
without recording bitmap info -- which is what it is doing.

In any case I am happy to clean this up if there's something more elegant in
people's mind.