Re: [PATCH RFC 02/19] iommufd: Dirty tracking for io_pagetable

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

On 4/29/22 12:56, Jason Gunthorpe wrote:
> On Fri, Apr 29, 2022 at 08:07:14AM +0000, Tian, Kevin wrote:
>>> From: Joao Martins <joao.m.martins@oracle.com>
>>> Sent: Friday, April 29, 2022 5:09 AM
>>>
>>> +static int __set_dirty_tracking_range_locked(struct iommu_domain
>>> *domain,
>>
>> suppose anything using iommu_domain as the first argument should
>> be put in the iommu layer. Here it's more reasonable to use iopt
>> as the first argument or simply merge with the next function.
>>
>>> +					     struct io_pagetable *iopt,
>>> +					     bool enable)
>>> +{
>>> +	const struct iommu_domain_ops *ops = domain->ops;
>>> +	struct iommu_iotlb_gather gather;
>>> +	struct iopt_area *area;
>>> +	int ret = -EOPNOTSUPP;
>>> +	unsigned long iova;
>>> +	size_t size;
>>> +
>>> +	iommu_iotlb_gather_init(&gather);
>>> +
>>> +	for (area = iopt_area_iter_first(iopt, 0, ULONG_MAX); area;
>>> +	     area = iopt_area_iter_next(area, 0, ULONG_MAX)) {
>>
>> how is this different from leaving iommu driver to walk the page table
>> and the poke the modifier bit for all present PTEs? As commented in last
>> patch this may allow removing the range op completely.
> 
> Yea, I'm not super keen on the two ops either, especially since they
> are so wildly different.
> 
/me ack

> I would expect that set_dirty_tracking turns on tracking for the
> entire iommu domain, for all present and future maps
> 
Yes.

I didn't do that correctly on ARM, neither on device-attach
(for x86 e.g. on hotplug).

> While set_dirty_tracking_range - I guess it only does the range, so if
> we make a new map then the new range will be untracked? But that is
> now racy, we have to map and then call set_dirty_tracking_range
> 
> It seems better for the iommu driver to deal with this and ARM should
> atomically make the new maps dirty tracking..
> 

Next iteration I'll need to fix the way IOMMUs handle dirty-tracking
probing and tracking in its private intermediate structures.

But yes, I was trying to transfer this to the iommu driver (perhaps in a
convoluted way).

>>> +int iopt_set_dirty_tracking(struct io_pagetable *iopt,
>>> +			    struct iommu_domain *domain, bool enable)
>>> +{
>>> +	struct iommu_domain *dom;
>>> +	unsigned long index;
>>> +	int ret = -EOPNOTSUPP;
> 
> Returns EOPNOTSUPP if the xarray is empty?
> 
Argh no. Maybe -EINVAL is better here.