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

From: Joao Martins <joao.m.martins@oracle.com>
To: Jason Gunthorpe <jgg@nvidia.com>, "Tian, Kevin" <kevin.tian@intel.com>
Cc: Jean-Philippe Brucker <jean-philippe@linaro.org>,
	Yishai Hadas <yishaih@nvidia.com>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	Will Deacon <will@kernel.org>, Cornelia Huck <cohuck@redhat.com>,
	Alex Williamson <alex.williamson@redhat.com>,
	"iommu@lists.linux-foundation.org"
	<iommu@lists.linux-foundation.org>,
	David Woodhouse <dwmw2@infradead.org>,
	Robin Murphy <robin.murphy@arm.com>
Subject: Re: [PATCH RFC 02/19] iommufd: Dirty tracking for io_pagetable
Date: Fri, 29 Apr 2022 15:28:06 +0100	[thread overview]
Message-ID: <3fae7334-df70-183f-6629-9aad704b54fb@oracle.com> (raw)
In-Reply-To: <20220429115616.GP8364@nvidia.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.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu