Re: [PATCH v4 08/17] iommufd: Algorithms for PFN storage

[Jason Gunthorpe <jgg@nvidia.com>]

On Mon, Nov 14, 2022 at 05:50:50AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Tuesday, November 8, 2022 8:49 AM
> > 
> > @@ -171,7 +183,7 @@ static struct iopt_area
> > *iopt_pages_find_domain_area(struct iopt_pages *pages,
> >   */
> >  struct pfn_batch {
> >  	unsigned long *pfns;
> > -	u16 *npfns;
> > +	u32 *npfns;
> 
> why not making it u32 and removing later FIXME directly in patch7?

Rebase error, I fixed it

> >  static bool batch_add_pfn(struct pfn_batch *batch, unsigned long pfn)
> >  {
> > -	/* FIXME: U16 is too small */
> > +	const unsigned int MAX_NPFNS = type_max(typeof(*batch->npfns));
> 
> use lowercase i.e. max_npfns.

It is customary to use caps for constants, eg enum values and things

> > +static void __iopt_area_unfill_domain(struct iopt_area *area,
> > +				      struct iopt_pages *pages,
> > +				      struct iommu_domain *domain,
> > +				      unsigned long last_index)
> > +{
> > +	struct interval_tree_double_span_iter span;
> > +	unsigned long start_index = iopt_area_index(area);
> > +	unsigned long unmapped_end_index = start_index;
> > +	u64 backup[BATCH_BACKUP_SIZE];
> > +	struct pfn_batch batch;
> > +
> > +	lockdep_assert_held(&pages->mutex);
> > +
> > +	batch_init_backup(&batch, last_index + 1, backup, sizeof(backup));
> > +	interval_tree_for_each_double_span(&span, &pages-
> > >domains_itree,
> > +					   &pages->access_itree, start_index,
> > +					   last_index) {
> > +		if (span.is_used) {
> > +			batch_skip_carry(&batch,
> > +					 span.last_used - span.start_used + 1);
> > +			continue;
> > +		}
> > +		iopt_area_unpin_domain(&batch, area, pages, domain,
> > +				       span.start_hole, span.last_hole,
> > +				       &unmapped_end_index, last_index);
> > +	}
> > +	if (unmapped_end_index != last_index + 1)
> > +		iopt_area_unmap_domain_range(area, domain,
> > unmapped_end_index,
> > +					     last_index);
> 
> a comment marking that it's for the last trailing used span of which
> the pages are not contiguous to previous span.

	/*
	 * If the range ends in a access then we do the residual unmap without
	 * any unpins.
	 */

> btw it is not easy to understand how this func plus unpin_domain()
> actually work. more comments are welcomed to help readability.

	/*
	 * For security we must not unpin something that is still DMA mapped,
	 * so this must unmap any IOVA before we go ahead and unpin the pages.
	 * This creates a complexity where we need to skip over unpinning pages
	 * held in the xarray, but continue to unmap from the domain.
	 *
	 * The domain unmap cannot stop in the middle of a contiguous range of
	 * PFNs. To solve this problem the unpinning step will read ahead to the
	 * end of any contiguous span, unmap that whole span, and then only
	 * unpin the leading part that does not have any accesses. The residual
	 * PFNs that were unmapped but not unpinned are called a "carry" in the
	 * batch as they are moved to the front of the PFN list and continue on
	 * to the next iteration(s).
	 */

> > +/*
> > + * This can do everything and is fully coherent with what a iommu_domain
> > would
> > + * see.
> > + */
> > +static int iopt_pages_rw_slow(struct iopt_pages *pages,
> 
> Can you elaborate what guarantees coherency in this function and how it
> becomes different in other rw variations?

/*
 * This uses the pfn_reader instead of taking a shortcut by using the mm. It can
 * do every scenario and is fully consistent with what an iommu_domain would
 * see.
 */

> > + * iopt_pages_remove_access() - Release an in-kernel access for PFNs
> > + * @area: The source of PFNs
> > + * @start_index: First page index
> > + * @last_index: Inclusive last page index
> > + *
> > + * Undo iopt_pages_add_access() and unpin the pages if necessary. The
> > caller
> > + * must stop using the PFNs before calling this.
> > + */
> > +void iopt_pages_remove_access(struct iopt_area *area, unsigned long
> > start_index,
> > +			      unsigned long last_index)
> 
> this is called iopt_pages_xxx() but the first parameter is iopt_area.
> 
> also it's not balanced with iopt_pages_add_access() which requires the
> caller to hold pages->mutex and populate area->num_accesses.

OK, see below

Thanks,
Jason

--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -565,7 +565,7 @@ void iommufd_access_unpin_pages(struct iommufd_access *access,
 
 	down_read(&iopt->iova_rwsem);
 	iopt_for_each_contig_area(&iter, area, iopt, iova, last_iova)
-		iopt_pages_remove_access(
+		iopt_area_remove_access(
 			area, iopt_area_iova_to_index(area, iter.cur_iova),
 			iopt_area_iova_to_index(
 				area,
@@ -650,15 +650,10 @@ int iommufd_access_pin_pages(struct iommufd_access *access, unsigned long iova,
 			goto err_remove;
 		}
 
-		mutex_lock(&area->pages->mutex);
-		rc = iopt_pages_add_access(area->pages, index, last_index,
-					   out_pages, flags);
-		if (rc) {
-			mutex_unlock(&area->pages->mutex);
+		rc = iopt_pages_add_access(area, index, last_index, out_pages,
+					   flags);
+		if (rc)
 			goto err_remove;
-		}
-		area->num_accesses++;
-		mutex_unlock(&area->pages->mutex);
 		out_pages += last_index - index + 1;
 	}
 	if (!iopt_area_contig_done(&iter)) {
@@ -673,7 +668,7 @@ int iommufd_access_pin_pages(struct iommufd_access *access, unsigned long iova,
 	if (iova < iter.cur_iova) {
 		last_iova = iter.cur_iova - 1;
 		iopt_for_each_contig_area(&iter, area, iopt, iova, last_iova)
-			iopt_pages_remove_access(
+			iopt_area_remove_access(
 				area,
 				iopt_area_iova_to_index(area, iter.cur_iova),
 				iopt_area_iova_to_index(
diff --git a/drivers/iommu/iommufd/io_pagetable.h b/drivers/iommu/iommufd/io_pagetable.h
index 3b85fa344f6be3..68bc3957534dd7 100644
--- a/drivers/iommu/iommufd/io_pagetable.h
+++ b/drivers/iommu/iommufd/io_pagetable.h
@@ -221,10 +221,10 @@ int iopt_pages_fill_xarray(struct iopt_pages *pages, unsigned long start,
 void iopt_pages_unfill_xarray(struct iopt_pages *pages, unsigned long start,
 			      unsigned long last);
 
-int iopt_pages_add_access(struct iopt_pages *pages, unsigned long start,
-			unsigned long last, struct page **out_pages,
-			unsigned int flags);
-void iopt_pages_remove_access(struct iopt_area *area, unsigned long start,
+int iopt_area_add_access(struct iopt_area *area, unsigned long start,
+			 unsigned long last, struct page **out_pages,
+			 unsigned int flags);
+void iopt_area_remove_access(struct iopt_area *area, unsigned long start,
 			    unsigned long last);
 int iopt_pages_rw_access(struct iopt_pages *pages, unsigned long start_byte,
 			 void *data, unsigned long length, unsigned int flags);
diff --git a/drivers/iommu/iommufd/pages.c b/drivers/iommu/iommufd/pages.c
index 01d2447eac4ede..e5f267d9e2b491 100644
--- a/drivers/iommu/iommufd/pages.c
+++ b/drivers/iommu/iommufd/pages.c
@@ -1807,8 +1826,8 @@ iopt_pages_get_exact_access(struct iopt_pages *pages, unsigned long index,
 }
 
 /**
- * iopt_pages_add_access() - Record an in-knerel access for PFNs
- * @pages: The source of PFNs
+ * iopt_area_add_access() - Record an in-knerel access for PFNs
+ * @area: The source of PFNs
  * @start_index: First page index
  * @last_index: Inclusive last page index
  * @out_pages: Output list of struct page's representing the PFNs
@@ -1819,40 +1838,49 @@ iopt_pages_get_exact_access(struct iopt_pages *pages, unsigned long index,
  *
  * This should be undone through a matching call to iopt_pages_remove_access()
  */
-int iopt_pages_add_access(struct iopt_pages *pages, unsigned long start_index,
+int iopt_area_add_access(struct iopt_area *area, unsigned long start_index,
 			  unsigned long last_index, struct page **out_pages,
 			  unsigned int flags)
 {
+	struct iopt_pages *pages = area->pages;
 	struct iopt_pages_access *access;
 	int rc;
 
 	if ((flags & IOMMUFD_ACCESS_RW_WRITE) && !pages->writable)
 		return -EPERM;
 
+	mutex_lock(&pages->mutex);
 	access = iopt_pages_get_exact_access(pages, start_index, last_index);
 	if (access) {
 		refcount_inc(&access->refcount);
 		iopt_pages_fill_from_xarray(pages, start_index, last_index,
 					    out_pages);
+		mutex_unlock(&pages->mutex);
 		return 0;
 	}
 
 	access = kzalloc(sizeof(*access), GFP_KERNEL_ACCOUNT);
-	if (!access)
-		return -ENOMEM;
+	if (!access) {
+		rc = -ENOMEM;
+		goto err_unlock;
+	}
 
 	rc = iopt_pages_fill_xarray(pages, start_index, last_index, out_pages);
 	if (rc)
-		goto out_free;
+		goto err_free;
 
 	access->node.start = start_index;
 	access->node.last = last_index;
 	refcount_set(&access->refcount, 1);
+	area->num_accesses++;
 	interval_tree_insert(&access->node, &pages->access_itree);
+	mutex_unlock(&pages->mutex);
 	return 0;
 
-out_free:
+err_free:
 	kfree(access);
+err_unlock:
+	mutex_unlock(&pages->mutex);
 	return rc;
 }
 
@@ -1865,11 +1893,11 @@ int iopt_pages_add_access(struct iopt_pages *pages, unsigned long start_index,
  * Undo iopt_pages_add_access() and unpin the pages if necessary. The caller
  * must stop using the PFNs before calling this.
  */
-void iopt_pages_remove_access(struct iopt_area *area, unsigned long start_index,
-			      unsigned long last_index)
+void iopt_area_remove_access(struct iopt_area *area, unsigned long start_index,
+			     unsigned long last_index)
 {
-	struct iopt_pages_access *access;
 	struct iopt_pages *pages = area->pages;
+	struct iopt_pages_access *access;
 
 	mutex_lock(&pages->mutex);
 	access = iopt_pages_get_exact_access(pages, start_index, last_index);