Re: [PATCH RFC 17/19] iommu/arm-smmu-v3: Add unmap_read_dirty() support

[Robin Murphy <robin.murphy@xxxxxxx>]

On 2022-04-28 22:09, Joao Martins wrote:
> Mostly reuses unmap existing code with the extra addition of
> marshalling into a bitmap of a page size. To tackle the race,
> switch away from a plain store to a cmpxchg() and check whether
> IOVA was dirtied or not once it succeeds.
>
> Signed-off-by: Joao Martins <joao.m.martins@xxxxxxxxxx>
> ---
>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 17 +++++
>   drivers/iommu/io-pgtable-arm.c              | 78 +++++++++++++++++----
>   2 files changed, 82 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 5f728f8f20a2..d1fb757056cc 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2499,6 +2499,22 @@ static size_t arm_smmu_unmap_pages(struct iommu_domain *domain, unsigned long io
>   	return ops->unmap_pages(ops, iova, pgsize, pgcount, gather);
>   }
>   
> +static size_t arm_smmu_unmap_pages_read_dirty(struct iommu_domain *domain,
> +					      unsigned long iova, size_t pgsize,
> +					      size_t pgcount,
> +					      struct iommu_iotlb_gather *gather,
> +					      struct iommu_dirty_bitmap *dirty)
> +{
> +	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> +	struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
> +
> +	if (!ops)
> +		return 0;
> +
> +	return ops->unmap_pages_read_dirty(ops, iova, pgsize, pgcount,
> +					   gather, dirty);
> +}
> +
>   static void arm_smmu_flush_iotlb_all(struct iommu_domain *domain)
>   {
>   	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> @@ -2938,6 +2954,7 @@ static struct iommu_ops arm_smmu_ops = {
>   		.free			= arm_smmu_domain_free,
>   		.read_and_clear_dirty	= arm_smmu_read_and_clear_dirty,
>   		.set_dirty_tracking_range = arm_smmu_set_dirty_tracking,
> +		.unmap_pages_read_dirty	= arm_smmu_unmap_pages_read_dirty,
>   	}
>   };
>   
> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index 361410aa836c..143ee7d73f88 100644
> --- a/drivers/iommu/io-pgtable-arm.c
> +++ b/drivers/iommu/io-pgtable-arm.c
> @@ -259,10 +259,30 @@ static void __arm_lpae_clear_pte(arm_lpae_iopte *ptep, struct io_pgtable_cfg *cf
>   		__arm_lpae_sync_pte(ptep, 1, cfg);
>   }
>   
> +static bool __arm_lpae_clear_dirty_pte(arm_lpae_iopte *ptep,
> +				       struct io_pgtable_cfg *cfg)
> +{
> +	arm_lpae_iopte tmp;
> +	bool dirty = false;
> +
> +	do {
> +		tmp = cmpxchg64(ptep, *ptep, 0);
> +		if ((tmp & ARM_LPAE_PTE_DBM) &&
> +		    !(tmp & ARM_LPAE_PTE_AP_RDONLY))
> +			dirty = true;
> +	} while (tmp);
> +
> +	if (!cfg->coherent_walk)
> +		__arm_lpae_sync_pte(ptep, 1, cfg);

Note that this doesn't do enough, since it's only making the CPU's 
clearing of the PTE visible to the SMMU; the cmpxchg could have happily 
succeeded on a stale cached copy of the writeable-clean PTE regardless 
of what the SMMU might have done in the meantime. If we were to even 
pretend to cope with a non-coherent SMMU writing back to the pagetables, 
I think we'd have to scrap the current DMA API approach and make the CPU 
view of the pagetables non-cacheable as well, but as mentioned, there's 
no guarantee that that would even be useful anyway.

Robin.

> +
> +	return dirty;
> +}
> +
>   static size_t __arm_lpae_unmap(struct arm_lpae_io_pgtable *data,
>   			       struct iommu_iotlb_gather *gather,
>   			       unsigned long iova, size_t size, size_t pgcount,
> -			       int lvl, arm_lpae_iopte *ptep);
> +			       int lvl, arm_lpae_iopte *ptep,
> +			       struct iommu_dirty_bitmap *dirty);
>   
>   static void __arm_lpae_init_pte(struct arm_lpae_io_pgtable *data,
>   				phys_addr_t paddr, arm_lpae_iopte prot,
> @@ -306,8 +326,13 @@ static int arm_lpae_init_pte(struct arm_lpae_io_pgtable *data,
>   			size_t sz = ARM_LPAE_BLOCK_SIZE(lvl, data);
>   
>   			tblp = ptep - ARM_LPAE_LVL_IDX(iova, lvl, data);
> +
> +			/*
> +			 * No need for dirty bitmap as arm_lpae_init_pte() is
> +			 * only called from __arm_lpae_map()
> +			 */
>   			if (__arm_lpae_unmap(data, NULL, iova + i * sz, sz, 1,
> -					     lvl, tblp) != sz) {
> +					     lvl, tblp, NULL) != sz) {
>   				WARN_ON(1);
>   				return -EINVAL;
>   			}
> @@ -564,7 +589,8 @@ static size_t arm_lpae_split_blk_unmap(struct arm_lpae_io_pgtable *data,
>   				       struct iommu_iotlb_gather *gather,
>   				       unsigned long iova, size_t size,
>   				       arm_lpae_iopte blk_pte, int lvl,
> -				       arm_lpae_iopte *ptep, size_t pgcount)
> +				       arm_lpae_iopte *ptep, size_t pgcount,
> +				       struct iommu_dirty_bitmap *dirty)
>   {
>   	struct io_pgtable_cfg *cfg = &data->iop.cfg;
>   	arm_lpae_iopte pte, *tablep;
> @@ -617,13 +643,15 @@ static size_t arm_lpae_split_blk_unmap(struct arm_lpae_io_pgtable *data,
>   		return num_entries * size;
>   	}
>   
> -	return __arm_lpae_unmap(data, gather, iova, size, pgcount, lvl, tablep);
> +	return __arm_lpae_unmap(data, gather, iova, size, pgcount,
> +				lvl, tablep, dirty);
>   }
>   
>   static size_t __arm_lpae_unmap(struct arm_lpae_io_pgtable *data,
>   			       struct iommu_iotlb_gather *gather,
>   			       unsigned long iova, size_t size, size_t pgcount,
> -			       int lvl, arm_lpae_iopte *ptep)
> +			       int lvl, arm_lpae_iopte *ptep,
> +			       struct iommu_dirty_bitmap *dirty)
>   {
>   	arm_lpae_iopte pte;
>   	struct io_pgtable *iop = &data->iop;
> @@ -649,7 +677,11 @@ static size_t __arm_lpae_unmap(struct arm_lpae_io_pgtable *data,
>   			if (WARN_ON(!pte))
>   				break;
>   
> -			__arm_lpae_clear_pte(ptep, &iop->cfg);
> +			if (likely(!dirty))
> +				__arm_lpae_clear_pte(ptep, &iop->cfg);
> +			else if (__arm_lpae_clear_dirty_pte(ptep, &iop->cfg))
> +				iommu_dirty_bitmap_record(dirty, iova, size);
> +
>   
>   			if (!iopte_leaf(pte, lvl, iop->fmt)) {
>   				/* Also flush any partial walks */
> @@ -671,17 +703,20 @@ static size_t __arm_lpae_unmap(struct arm_lpae_io_pgtable *data,
>   		 * minus the part we want to unmap
>   		 */
>   		return arm_lpae_split_blk_unmap(data, gather, iova, size, pte,
> -						lvl + 1, ptep, pgcount);
> +						lvl + 1, ptep, pgcount, dirty);
>   	}
>   
>   	/* Keep on walkin' */
>   	ptep = iopte_deref(pte, data);
> -	return __arm_lpae_unmap(data, gather, iova, size, pgcount, lvl + 1, ptep);
> +	return __arm_lpae_unmap(data, gather, iova, size, pgcount,
> +				lvl + 1, ptep, dirty);
>   }
>   
> -static size_t arm_lpae_unmap_pages(struct io_pgtable_ops *ops, unsigned long iova,
> -				   size_t pgsize, size_t pgcount,
> -				   struct iommu_iotlb_gather *gather)
> +static size_t __arm_lpae_unmap_pages(struct io_pgtable_ops *ops,
> +				     unsigned long iova,
> +				     size_t pgsize, size_t pgcount,
> +				     struct iommu_iotlb_gather *gather,
> +				     struct iommu_dirty_bitmap *dirty)
>   {
>   	struct arm_lpae_io_pgtable *data = io_pgtable_ops_to_data(ops);
>   	struct io_pgtable_cfg *cfg = &data->iop.cfg;
> @@ -697,13 +732,29 @@ static size_t arm_lpae_unmap_pages(struct io_pgtable_ops *ops, unsigned long iov
>   		return 0;
>   
>   	return __arm_lpae_unmap(data, gather, iova, pgsize, pgcount,
> -				data->start_level, ptep);
> +				data->start_level, ptep, dirty);
> +}
> +
> +static size_t arm_lpae_unmap_pages(struct io_pgtable_ops *ops, unsigned long iova,
> +				   size_t pgsize, size_t pgcount,
> +				   struct iommu_iotlb_gather *gather)
> +{
> +	return __arm_lpae_unmap_pages(ops, iova, pgsize, pgcount, gather, NULL);
>   }
>   
>   static size_t arm_lpae_unmap(struct io_pgtable_ops *ops, unsigned long iova,
>   			     size_t size, struct iommu_iotlb_gather *gather)
>   {
> -	return arm_lpae_unmap_pages(ops, iova, size, 1, gather);
> +	return __arm_lpae_unmap_pages(ops, iova, size, 1, gather, NULL);
> +}
> +
> +static size_t arm_lpae_unmap_pages_read_dirty(struct io_pgtable_ops *ops,
> +					      unsigned long iova,
> +					      size_t pgsize, size_t pgcount,
> +					      struct iommu_iotlb_gather *gather,
> +					      struct iommu_dirty_bitmap *dirty)
> +{
> +	return __arm_lpae_unmap_pages(ops, iova, pgsize, pgcount, gather, dirty);
>   }
>   
>   static phys_addr_t arm_lpae_iova_to_phys(struct io_pgtable_ops *ops,
> @@ -969,6 +1020,7 @@ arm_lpae_alloc_pgtable(struct io_pgtable_cfg *cfg)
>   		.iova_to_phys	= arm_lpae_iova_to_phys,
>   		.read_and_clear_dirty = arm_lpae_read_and_clear_dirty,
>   		.set_dirty_tracking   = arm_lpae_set_dirty_tracking,
> +		.unmap_pages_read_dirty     = arm_lpae_unmap_pages_read_dirty,
>   	};
>   
>   	return data;