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@xxxxxxxxxx>
>>> 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.
