Re: [PATCH RFC 15/19] iommu/arm-smmu-v3: Add set_dirty_tracking_range() support

[Joao Martins <joao.m.martins@oracle.com>]

On 4/29/22 17:11, Jason Gunthorpe wrote:
> On Fri, Apr 29, 2022 at 03:45:23PM +0100, Joao Martins wrote:
>> On 4/29/22 13:23, Jason Gunthorpe wrote:
>>> On Fri, Apr 29, 2022 at 01:06:06PM +0100, Joao Martins wrote:
>>>
>>>>> TBH I'd be inclined to just enable DBM unconditionally in 
>>>>> arm_smmu_domain_finalise() if the SMMU supports it. Trying to toggle it 
>>>>> dynamically (especially on a live domain) seems more trouble that it's 
>>>>> worth.
>>>>
>>>> Hmmm, but then it would strip userland/VMM from any sort of control (contrary
>>>> to what we can do on the CPU/KVM side). e.g. the first time you do
>>>> GET_DIRTY_IOVA it would return all dirtied IOVAs since the beginning
>>>> of guest time, as opposed to those only after you enabled dirty-tracking.
>>>
>>> It just means that on SMMU the start tracking op clears all the dirty
>>> bits.
>>>
>> Hmm, OK. But aren't really picking a poison here? On ARM it's the difference
>> from switching the setting the DBM bit and put the IOPTE as writeable-clean (which
>> is clearing another bit) versus read-and-clear-when-dirty-track-start which means
>> we need to re-walk the pagetables to clear one bit.
> 
> Yes, I don't think a iopte walk is avoidable?
> 
Correct -- exactly why I am still more learning towards enable DBM bit only at start
versus enabling DBM at domain-creation while clearing dirty at start.

>> It's walking over ranges regardless.
> 
> Also, keep in mind start should always come up in a 0 dirties state on
> all platforms. So all implementations need to do something to wipe the
> dirty state, either explicitly during start or restoring all clean
> during stop.
> 
> A common use model might be to just destroy the iommu_domain without
> doing stop so prefering the clearing io page table at stop might be a
> better overall design.

If we want to ensure that the IOPTE dirty state is immutable before start
and after stop maybe this behaviour could be a new flag in the set-dirty-tracking
(or be implicit as you suggest).  but ... hmm, at the same time, I wonder if
it's better to let userspace fetch the dirties that were there /right after stopping/
(via GET_DIRTY_IOVA) rather than just discarding them implicitly at SET_DIRTY_TRACKING(0|1).