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

[Jason Gunthorpe <jgg@nvidia.com>]

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.

More like this puts it in the common code and writes it in a more
straightforward way with better locking:

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;
 };