iommu: Split iommu_unmaps

iommu_map splits requests into pages that the iommu driver reports that it can handle. The iommu_unmap path does not do the same. This can cause problems not only from callers that might expect the same behavior as the map path, but even from the failure path of iommu_map, should it fail at a point where it has mapped and needs to unwind a set of pages that the iommu driver cannot handle directly. amd_iommu, for example, will BUG_ON if asked to unmap a non power of 2 size.

Fix this by extracting and generalizing the sizing code from the iommu_map path and use it for both map and unmap.

Signed-off-by Alex Williamson <[email protected]>

---
 drivers/iommu/iommu.c |   63 +++++++++++++++++++++++++++----------------------
 1 file changed, 35 insertions(+), 28 deletions(-)

drivers/iommu/iommu.c +35 -28

index d8f98b1..4b0b56b 100644
@@ -754,6 +754,38 @@ int iommu_domain_has_cap(struct iommu_domain *domain,
 }
 EXPORT_SYMBOL_GPL(iommu_domain_has_cap);
 
+static size_t iommu_pgsize(struct iommu_domain *domain,
+			   unsigned long addr_merge, size_t size)
+{
+	unsigned int pgsize_idx;
+	size_t pgsize;
+
+	/* Max page size that still fits into 'size' */
+	pgsize_idx = __fls(size);
+
+	/* need to consider alignment requirements ? */
+	if (likely(addr_merge)) {
+		/* Max page size allowed by address */
+		unsigned int align_pgsize_idx = __ffs(addr_merge);
+		pgsize_idx = min(pgsize_idx, align_pgsize_idx);
+	}
+
+	/* build a mask of acceptable page sizes */
+	pgsize = (1UL << (pgsize_idx + 1)) - 1;
+
+	/* throw away page sizes not supported by the hardware */
+	pgsize &= domain->ops->pgsize_bitmap;
+
+	/* make sure we're still sane */
+	BUG_ON(!pgsize);
+
+	/* pick the biggest page */
+	pgsize_idx = __fls(pgsize);
+	pgsize = 1UL << pgsize_idx;
+
+	return pgsize;
+}
+
 int iommu_map(struct iommu_domain *domain, unsigned long iova,
 	      phys_addr_t paddr, size_t size, int prot)
 {

@@ -785,32 +817,7 @@ int iommu_map(struct iommu_domain *domain, unsigned long iova,
 				(unsigned long)paddr, (unsigned long)size);
 
 	while (size) {
-		unsigned long pgsize, addr_merge = iova | paddr;
-		unsigned int pgsize_idx;
-
-		/* Max page size that still fits into 'size' */
-		pgsize_idx = __fls(size);
-
-		/* need to consider alignment requirements ? */
-		if (likely(addr_merge)) {
-			/* Max page size allowed by both iova and paddr */
-			unsigned int align_pgsize_idx = __ffs(addr_merge);
-
-			pgsize_idx = min(pgsize_idx, align_pgsize_idx);
-		}
-
-		/* build a mask of acceptable page sizes */
-		pgsize = (1UL << (pgsize_idx + 1)) - 1;
-
-		/* throw away page sizes not supported by the hardware */
-		pgsize &= domain->ops->pgsize_bitmap;
-
-		/* make sure we're still sane */
-		BUG_ON(!pgsize);
-
-		/* pick the biggest page */
-		pgsize_idx = __fls(pgsize);
-		pgsize = 1UL << pgsize_idx;
+		size_t pgsize = iommu_pgsize(domain, iova | paddr, size);
 
 		pr_debug("mapping: iova 0x%lx pa 0x%lx pgsize %lu\n", iova,
 					(unsigned long)paddr, pgsize);

@@ -863,9 +870,9 @@ size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova, size_t size)
 	 * or we hit an area that isn't mapped.
 	 */
 	while (unmapped < size) {
-		size_t left = size - unmapped;
+		size_t pgsize = iommu_pgsize(domain, iova, size - unmapped);
 
-		unmapped_page = domain->ops->unmap(domain, iova, left);
+		unmapped_page = domain->ops->unmap(domain, iova, pgsize);
 		if (!unmapped_page)
 			break;
 

iommu_map splits requests into pages that the iommu driver reports that it can handle. The iommu_unmap path does not do the same. This can cause problems not only from callers that might expect the same behavior as the map path, but even from the failure path of iommu_map, should it fail at a point where it has mapped and needs to unwind a set of pages that the iommu driver cannot handle directly. amd_iommu, for example, will BUG_ON if asked to unmap a non power of 2 size.

Fix this by extracting and generalizing the sizing code from the iommu_map path and use it for both map and unmap.

Signed-off-by Alex Williamson <[email protected]>

Quoted 8 lines · Alex Williamson

iommu_map splits requests into pages that the iommu driver reports that it can handle. The iommu_unmap path does not do the same. This can cause problems not only from callers that might expect the same behavior as the map path, but even from the failure path of iommu_map, should it fail at a point where it has mapped and needs to unwind a set of pages that the iommu driver cannot handle directly. amd_iommu, for example, will BUG_ON if asked to unmap a non power of 2 size.

Quoted 4 lines

Fix this by extracting and generalizing the sizing code from the iommu_map path and use it for both map and unmap.

Signed-off-by Alex Williamson <[email protected]>

Ick, this is horrid and looks like it will introduce a massive performance hit.

Surely the answer is to fix the AMD driver so that it will just get on with it and unmap the {address, range} that it's asked to map?

And fix the generic iommu_map() code while we're at it, to do it the same way.

IOTLB flushes are *slow*, and on certain hardware with non-cache-coherent page tables even the dcache flush for the page table is slow. If you deliberately break it up into individual pages and do the flush between each one, rather than just unmapping the range, you are pessimising the performance quite hard.

A while back, I went through the Intel IOMMU code to make sure it was doing this right — it used to have this kind of bogosity with repeated per-page cache and IOTLB flushes *internally*, and the resulting performance improvement was shown at http://david.woodhou.se/iommu.png

You will basically be undoing that work, by ensuring that the low-level driver never *sees* the full range.

If the AMD driver really can't handle more than one page at a time, let it loop for *itself* over the pages.

Quoted 14 lines · David Woodhouse

Quoted 12 lines · Alex Williamson

iommu_map splits requests into pages that the iommu driver reports that it can handle. The iommu_unmap path does not do the same. This can cause problems not only from callers that might expect the same behavior as the map path, but even from the failure path of iommu_map, should it fail at a point where it has mapped and needs to unwind a set of pages that the iommu driver cannot handle directly. amd_iommu, for example, will BUG_ON if asked to unmap a non power of 2 size.

Fix this by extracting and generalizing the sizing code from the iommu_map path and use it for both map and unmap.

Signed-off-by Alex Williamson <[email protected]>

Quoted 2 lines

Ick, this is horrid and looks like it will introduce a massive performance hit.

For x86 there are essentially two users of iommu_unmap(), KVM and VFIO. Both of them try to unmap an individual page and look at the result to see how much was actually unmapped. Everything else appears to be error paths. So where exactly is this massive performance hit?

Quoted 2 lines

Surely the answer is to fix the AMD driver so that it will just get on with it and unmap the {address, range} that it's asked to map?

The IOMMU API allows iommu drivers to expose the page sizes they support. Mappings are done using these sizes so it only seems fair that unmappings should as well. At least that's what amd_iommu was expecting.

That data is for dma_ops interfaces, not IOMMU API. How is changing iommu_unmap() in this way undoing any of your previous work?

Quoted 2 lines

If the AMD driver really can't handle more than one page at a time, let it loop for *itself* over the pages.

Sure, but that's a change to the API where I think this fix was correcting a bug in the implementation of the API. Are there users of iommu_unmap() that I don't know about? Given the in-tree users, there's not really a compelling argument to optimize. Thanks,