Re: [PATCH RFCv2 09/24] iommufd: Add IOMMU_HWPT_SET_DIRTY

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

On 19/05/2023 14:49, Jason Gunthorpe wrote:
> On Thu, May 18, 2023 at 09:46:35PM +0100, Joao Martins wrote:
>> +int iopt_set_dirty_tracking(struct io_pagetable *iopt,
>> +			    struct iommu_domain *domain, bool enable)
>> +{
>> +	const struct iommu_domain_ops *ops = domain->ops;
>> +	struct iommu_dirty_bitmap dirty;
>> +	struct iommu_iotlb_gather gather;
>> +	struct iopt_area *area;
>> +	int ret = 0;
>> +
>> +	if (!ops->set_dirty_tracking)
>> +		return -EOPNOTSUPP;
>> +
>> +	iommu_dirty_bitmap_init(&dirty, NULL, &gather);
>> +
>> +	down_write(&iopt->iova_rwsem);
>> +	for (area = iopt_area_iter_first(iopt, 0, ULONG_MAX);
>> +	     area && enable;
> 
> That's a goofy way to write this.. put this in a function and don't
> call it if enable is not set.
> 
I'll move this into a iopt_clear_dirty_data()

There's also another less positive aspect here on the implicit assumption I made
that a dirty::bitmap == NULL means we do not care about recording dirty bitmap.
Which is OK. But I use that field being NULL as means to ignore the iommu driver
status control to just clear the remnant dirty bits that could have been done
until we disable dirty tracking. I'm thinking in making this into a flags value,
but keeping internally only, I am not sure this should be exposed to userspace.

> Why is this down_write() ?
> 
> You can see that this locking already prevents racing dirty read with
> domain unmap.
> 
> This domain cannot be removed from the iopt eg through
> iopt_table_remove_domain() because this is holding the object
> reference on the hwpt
> 
> The area cannot be unmapped because this is holding the
> &iopt->iova_rwsem
> 
> There is no other way to call unmap..

down_read(&iopt->iova_rwsem) is more approprite;
iopt_read_and_clear_dirty_data() does so already.

But I should iterating over areas there too, which I wrongly not doing.

> You do have to check that area->pages != NULL though

OK