Re: [RFC PATCH v4 01/13] iommu: Introduce dirty log tracking framework

[Lu Baolu]

On 5/13/21 6:58 PM, Keqian Zhu wrote:
> On 2021/5/12 19:36, Lu Baolu wrote:
> > Hi keqian,
> >
> > On 5/12/21 4:44 PM, Keqian Zhu wrote:
> > > On 2021/5/12 11:20, Lu Baolu wrote:
> > > > On 5/11/21 3:40 PM, Keqian Zhu wrote:
> > > > > > For upper layers, before starting page tracking, they check the
> > > > > > dirty_page_trackable attribution of the domain and start it only it's
> > > > > > capable. Once the page tracking is switched on the vendor iommu driver
> > > > > > (or iommu core) should block further device attach/detach operations
> > > > > > until page tracking is stopped.
> > > > > But when a domain becomes capable after detaching a device, the upper layer
> > > > > still needs to query it and enable dirty log for it...
> > > > >
> > > > > To make things coordinated, maybe the upper layer can register a notifier,
> > > > > when the domain's capability change, the upper layer do not need to query, instead
> > > > > they just need to realize a callback, and do their specific policy in the callback.
> > > > > What do you think?
> > > >
> > > > That might be an option. But why not checking domain's attribution every
> > > > time a new tracking period is about to start?
> > > Hi Baolu,
> > >
> > > I'll add an attribution in iommu_domain, and the vendor iommu driver will update
> > > the attribution when attach/detach devices.
> > >
> > > The attribute should be protected by a lock, so the upper layer shouldn't access
> > > the attribute directly. Then the iommu_domain_support_dirty_log() still should be
> > > retained. Does this design looks good to you?
> >
> > Yes, that's what I was thinking of. But I am not sure whether it worth
> > of a lock here. It seems not to be a valid behavior for upper layer to
> > attach or detach any device while doing the dirty page tracking.
> Hi Baolu,
>
> Right, if the "detach|attach" interfaces and "dirty tracking" interfaces can be called concurrently,
> a lock in iommu_domain_support_dirty_log() is still not enough. I will add another note for the dirty
> tracking interfaces.
>
> Do you have other suggestions? I will accelerate the progress, so I plan to send out v5 next week.

No further comments expect below nit:

"iommu_switch_dirty_log: Perform actions to start|stop dirty log tracking"

How about splitting it into
 - iommu_start_dirty_log()
 - iommu_stop_dirty_log()

Not a strong opinion anyway.

Best regards,
baolu