Re: [PATCH v2 2/3] iommu/io-pgtable-arm-v7s: Remove split on unmap behavior

[Jason Gunthorpe <jgg@nvidia.com>]

On Mon, Nov 04, 2024 at 07:53:46PM +0000, Robin Murphy wrote:
> On 2024-11-04 5:41 pm, Jason Gunthorpe wrote:
> > A minority of page table implementations (arm_lpae, armv7) are unique in
> > how they handle partial unmap of large IOPTEs.
> > 
> > Other implementations will unmap the large IOPTE and return it's
> > length. For example if a 2M IOPTE is present and the first 4K is requested
> > to be unmapped then unmap will remove the whole 2M and report 2M as the
> > result.
> > 
> > armv7 instead will break up contiguous entries and replace an entry with a
> > whole table so it can unmap the requested 4k.
> > 
> > This seems copied from the arm_lpae implementation, which was analyzed
> > here:
> > 
> >   https://lore.kernel.org/all/20241024134411.GA6956@nvidia.com/
> > 
> > Bring consistency to the implementations and remove this unused
> > functionality.
> > 
> > There are no uses outside iommu, this effects the ARM_V7S drivers
> > msm_iommu, mtk_iommu, and arm-smmmu.
> > 
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > ---
> >   drivers/iommu/io-pgtable-arm-v7s.c | 125 +----------------------------
> >   1 file changed, 4 insertions(+), 121 deletions(-)
> 
> Yikes, I'd forgotten quite how much horribleness was devoted to this,
> despite it being the "simpler" non-recursive one...

Yes, it is the contiguous page support that makes it so complex..

> However, there are also "partial unmap" cases in both sets of selftests, so
> I think there's still a bit more to remove yet :)

Sneaky, I got it thanks

Runs OK now:

arm-v7s io-pgtable: self test ok
arm-lpae io-pgtable: selftest: pgsize_bitmap 0x40201000, IAS 32

Jason

--- a/drivers/iommu/io-pgtable-arm-v7s.c
+++ b/drivers/iommu/io-pgtable-arm-v7s.c
@@ -819,7 +819,7 @@ static int __init arm_v7s_do_selftests(void)
 		.quirks = IO_PGTABLE_QUIRK_ARM_NS,
 		.pgsize_bitmap = SZ_4K | SZ_64K | SZ_1M | SZ_16M,
 	};
-	unsigned int iova, size, iova_start;
+	unsigned int iova, size;
 	unsigned int i, loopnr = 0;
 	size_t mapped;
 
@@ -871,25 +871,6 @@ static int __init arm_v7s_do_selftests(void)
 		loopnr++;
 	}
 
-	/* Partial unmap */
-	i = 1;
-	size = 1UL << __ffs(cfg.pgsize_bitmap);
-	while (i < loopnr) {
-		iova_start = i * SZ_16M;
-		if (ops->unmap_pages(ops, iova_start + size, size, 1, NULL) != size)
-			return __FAIL(ops);
-
-		/* Remap of partial unmap */
-		if (ops->map_pages(ops, iova_start + size, size, size, 1,
-				   IOMMU_READ, GFP_KERNEL, &mapped))
-			return __FAIL(ops);
-
-		if (ops->iova_to_phys(ops, iova_start + size + 42)
-		    != (size + 42))
-			return __FAIL(ops);
-		i++;
-	}
-
 	/* Full unmap */
 	iova = 0;
 	for_each_set_bit(i, &cfg.pgsize_bitmap, BITS_PER_LONG) {