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

[Joao Martins]

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 :/

> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index d35c1b8c8e65ce..b2cb557d3ea427 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -2645,13 +2645,6 @@ static int amd_iommu_read_and_clear_dirty(struct iommu_domain *domain,
>  	if (!ops || !ops->read_and_clear_dirty)
>  		return -EOPNOTSUPP;
>  
> -	spin_lock_irqsave(&pdomain->lock, lflags);
> -	if (!pdomain->dirty_tracking && dirty->bitmap) {
> -		spin_unlock_irqrestore(&pdomain->lock, lflags);
> -		return -EINVAL;
> -	}
> -	spin_unlock_irqrestore(&pdomain->lock, lflags);
> -
>  	return ops->read_and_clear_dirty(ops, iova, size, flags, dirty);
>  }
>  
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 50eb9aed47cc58..844f2cf061911f 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4797,15 +4797,6 @@ static int intel_iommu_read_and_clear_dirty(struct iommu_domain *domain,
>  	unsigned long end = iova + size - 1;
>  	unsigned long pgsize;
>  
> -	/*
> -	 * IOMMUFD core calls into a dirty tracking disabled domain without an
> -	 * IOVA bitmap set in order to clean dirty bits in all PTEs that might
> -	 * have occurred when we stopped dirty tracking. This ensures that we
> -	 * never inherit dirtied bits from a previous cycle.
> -	 */
> -	if (!dmar_domain->dirty_tracking && dirty->bitmap)
> -		return -EINVAL;
> -
>  	do {
>  		struct dma_pte *pte;
>  		int lvl = 0;
> 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;
> +	}
>  
>  	/* 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;
>  };
>