[PATCH v3 2/4] iommu/io-pgtable-arm: Add read_and_clear_dirty() support

[Joao Martins]

On 22/05/2024 19:15, Joao Martins wrote:
> On 22/05/2024 18:50, Jason Gunthorpe wrote:
>> On Wed, May 22, 2024 at 06:10:59PM +0100, Joao Martins wrote:
>>> On 22/05/2024 17:56, Jason Gunthorpe wrote:
>>>> On Wed, May 22, 2024 at 03:37:57PM +0100, Joao Martins wrote:
>>>>  
>>>>> This is just to catch the case where IOMMUFD can call into read_and_clear()
>>>>> without dirty tracking enabled and without a bitmap structure to clear dirty
>>>>> bits -- in order to ensure a clean PTE data snapshot after start(). 
>>>>
>>>> Is that broken then?
>>>>
>>>
>>> It's not: The check errors out the caller ends up calling read-and-clear with a
>>> bitmap but without having started dirty tracking. the iopt_clear_dirty_data()
>>> passes a null bitmap, it goes through and it walks and clears the IOPTEs
>>> *without* recording them in the bitmap.
>>
>> It is not "without recording them in the bitmap", saying that is the
>> confusing thing. The purpose of that 'if' is to return -EINVAL if
>> dirty tracking is not turned on and we query the bitmap.
>>
> Right.
> 
>> More like this puts it in the common code and writes it in a more
>> straightforward way with better locking:
>>
> Yes, This snip you pasted would be the equivalent to the current way indeed.
> Looks good
> 
> I think I was trying too hard not to duplicate 'state of dirty tracking' between
> iommu driver and iommufd core that I unintendedly ended up convoluting with this
> check in the driver :/
> 

To this point above (...)

>> diff --git a/drivers/iommu/iommufd/io_pagetable.c b/drivers/iommu/iommufd/io_pagetable.c
>> index 05fd9d3abf1b80..d116179809042d 100644
>> --- a/drivers/iommu/iommufd/io_pagetable.c
>> +++ b/drivers/iommu/iommufd/io_pagetable.c
>> @@ -536,7 +536,10 @@ int iopt_read_and_clear_dirty_data(struct io_pagetable *iopt,
>>  		return ret;
>>  
>>  	down_read(&iopt->iova_rwsem);
>> -	ret = iommu_read_and_clear_dirty(domain, iopt, flags, bitmap);
>> +	if (!iopt->dirty_tracking_enabled)
>> +		ret = -EINVAL;
>> +	else
>> +		ret = iommu_read_and_clear_dirty(domain, iopt, flags, bitmap);
>>  	up_read(&iopt->iova_rwsem);
>>  
>>  	return ret;
>> @@ -580,7 +583,11 @@ int iopt_set_dirty_tracking(struct io_pagetable *iopt,
>>  	if (!ops)
>>  		return -EOPNOTSUPP;
>>  
>> -	down_read(&iopt->iova_rwsem);
>> +	down_write(&iopt->iova_rwsem);
>> +	if (iopt->dirty_tracking_enabled == enable) {
>> +		ret = 0;
>> +		goto out_unlock;
>> +	}
>>  

We have a similar check in set_dirty_tracking iommu op of intel/amd iommu
drivers that could be removed. Though I am not sure it's right to remove it even
as we move this up the stack

>>  	/* Clear dirty bits from PTEs to ensure a clean snapshot */
>>  	if (enable) {
>> @@ -590,9 +597,11 @@ int iopt_set_dirty_tracking(struct io_pagetable *iopt,
>>  	}
>>  
>>  	ret = ops->set_dirty_tracking(domain, enable);
>> -
>> +	if (ret)
>> +		goto out_unlock;
>> +	iopt->dirty_tracking_enabled = enable;
>>  out_unlock:
>> -	up_read(&iopt->iova_rwsem);
>> +	up_write(&iopt->iova_rwsem);
>>  	return ret;
>>  }
>>  
>> diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
>> index 991f864d1f9bc1..de3761e15cab54 100644
>> --- a/drivers/iommu/iommufd/iommufd_private.h
>> +++ b/drivers/iommu/iommufd/iommufd_private.h
>> @@ -52,6 +52,7 @@ struct io_pagetable {
>>  	/* IOVA that cannot be allocated, struct iopt_reserved */
>>  	struct rb_root_cached reserved_itree;
>>  	u8 disable_large_pages;
>> +	u8 dirty_tracking_enabled;
>>  	unsigned long iova_alignment;
>>  };
>>  
>