[PATCH v2 0/5] drm/msm: DMABUF_DEBUG fixes

[PATCH v2 0/5] drm/msm: DMABUF_DEBUG fixes

[Rob Clark]

With DMABUF_DEBUG=y, the page information is stripped from the sgt that
we get for an externally allocated buffer that is dma-buf imported (as
opposed to an exported GEM buffer that is re-imported).  This is a bit
unfortunate, since we need that information for mapping the buffer in
both paths:

1. Userspace GPU pgtables (TTBR0), where we use io_pgtable directly.
   Fortunately this we can fix ourselves easily.

2. Iommu managed pgtables (scanout, kernel GPU buffers (TTBR1)).  In
   normal usage that iommu subsystem envisions the dma-mapping and
   iommu-mapping are combined.  We can't really work around this but
   we can stop trying to pretend to be a normal iommu user and just
   go directly to the iommu_domain_ops.

This all depends on being dma_map_direct() for the dma_map_sgtable()
to work.  This should be the case on modern things that use arm-smmu.
It's less clear when it comes to older pre-arm-smmu hw.  It's possible
that this series causes those things to catch fire.  If so, look to
commit 0e764a01015d ("iommu/arm-smmu: Allow client devices to select
direct mapping") for inspiration.

v2:
- Fix missing tlb flushes in error paths
- Link to v1: https://lore.kernel.org/all/20261006131000.81501-1-robin.clark@oss.qualcomm.com/

Rob Clark (5):
  drm/msm: Cleanup if pages_to_sg() fails
  drm/msm/gem: dma_map/unmap_sgtable()
  drm/msm: Extract out map/unmap helpers
  drm/msm: Convert iommu map/unmap to helpers
  drm/msm: Convert map helper to use dma-address

 drivers/gpu/drm/msm/msm_gem.c   |  57 +++++++---
 drivers/gpu/drm/msm/msm_iommu.c | 190 +++++++++++++++++++++-----------
 2 files changed, 165 insertions(+), 82 deletions(-)

-- 
2.56.0

[PATCH v2 1/5] drm/msm: Cleanup if pages_to_sg() fails

[Rob Clark]

We never really handled this.  But in practice mapping to smmu would
have failed without an sgt, which would have been pretty obvious.

Tested-by: Jianfeng Liu <liujianfeng1994@gmail.com> # x1e78100 (Acer SFA14-11), v7.3-rc5 + this series, CONFIG_DMABUF_DEBUG=y
Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/msm_gem.c | 41 ++++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 130ae27ee00b..181b9ad32672 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -190,6 +190,7 @@ static void update_lru(struct drm_gem_object *obj)
 static struct page **get_pages(struct drm_gem_object *obj)
 {
 	struct msm_gem_object *msm_obj = to_msm_bo(obj);
+	int ret = 0;
 
 	msm_gem_assert_locked(obj);
 
@@ -209,19 +210,17 @@ static struct page **get_pages(struct drm_gem_object *obj)
 			return p;
 		}
 
-		update_device_mem(dev->dev_private, obj->size);
-
 		msm_obj->pages = p;
 
 		msm_obj->sgt = drm_prime_pages_to_sg(obj->dev, p, npages);
 		if (IS_ERR(msm_obj->sgt)) {
-			void *ptr = ERR_CAST(msm_obj->sgt);
-
 			DRM_DEV_ERROR(dev->dev, "failed to allocate sgt\n");
-			msm_obj->sgt = NULL;
-			return ptr;
+			ret = PTR_ERR(msm_obj->sgt);
+			goto error;
 		}
 
+		update_device_mem(dev->dev_private, obj->size);
+
 		/* For non-cached buffers, ensure the new pages are clean
 		 * because display controller, GPU, etc. are not coherent:
 		 */
@@ -232,6 +231,14 @@ static struct page **get_pages(struct drm_gem_object *obj)
 	}
 
 	return msm_obj->pages;
+
+error:
+	msm_obj->sgt = NULL;
+
+	drm_gem_put_pages(obj, msm_obj->pages, false, false);
+	msm_obj->pages = NULL;
+
+	return ERR_PTR(ret);
 }
 
 static void put_pages(struct drm_gem_object *obj)
@@ -246,18 +253,16 @@ static void put_pages(struct drm_gem_object *obj)
 		drm_gpuvm_bo_gem_evict(obj, true);
 
 	if (msm_obj->pages) {
-		if (msm_obj->sgt) {
-			/* For non-cached buffers, ensure the new
-			 * pages are clean because display controller,
-			 * GPU, etc. are not coherent:
-			 */
-			if (msm_obj->flags & MSM_BO_WC)
-				sync_for_cpu(msm_obj);
-
-			sg_free_table(msm_obj->sgt);
-			kfree(msm_obj->sgt);
-			msm_obj->sgt = NULL;
-		}
+		/* For non-cached buffers, ensure the new
+		 * pages are clean because display controller,
+		 * GPU, etc. are not coherent:
+		 */
+		if (msm_obj->flags & MSM_BO_WC)
+			sync_for_cpu(msm_obj);
+
+		sg_free_table(msm_obj->sgt);
+		kfree(msm_obj->sgt);
+		msm_obj->sgt = NULL;
 
 		update_device_mem(obj->dev->dev_private, -obj->size);
 
-- 
2.56.0

[PATCH v2 2/5] drm/msm/gem: dma_map/unmap_sgtable()

[Rob Clark]

This should functionally be a no-op for us, since we are managing our
SMMU directly (dma_map_ops is NULL), but it populates the dma_len/
dma_address's so that we can use them for setting up pgtable mappings.

Tested-by: Jianfeng Liu <liujianfeng1994@gmail.com> # x1e78100 (Acer SFA14-11), v7.3-rc5 + this series, CONFIG_DMABUF_DEBUG=y
Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/msm_gem.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 181b9ad32672..72e3cd8bb1e7 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -219,6 +219,15 @@ static struct page **get_pages(struct drm_gem_object *obj)
 			goto error;
 		}
 
+		ret = dma_map_sgtable(drm_dev_dma_dev(obj->dev),
+				      msm_obj->sgt,
+				      DMA_BIDIRECTIONAL,
+				      DMA_ATTR_SKIP_CPU_SYNC);
+		if (ret) {
+			DRM_DEV_ERROR(dev->dev, "could not map sgtable: %d\n", ret);
+			goto error;
+		}
+
 		update_device_mem(dev->dev_private, obj->size);
 
 		/* For non-cached buffers, ensure the new pages are clean
@@ -233,6 +242,8 @@ static struct page **get_pages(struct drm_gem_object *obj)
 	return msm_obj->pages;
 
 error:
+	if (!IS_ERR(msm_obj->sgt))
+		drm_prime_gem_destroy(obj, msm_obj->sgt);
 	msm_obj->sgt = NULL;
 
 	drm_gem_put_pages(obj, msm_obj->pages, false, false);
@@ -260,6 +271,11 @@ static void put_pages(struct drm_gem_object *obj)
 		if (msm_obj->flags & MSM_BO_WC)
 			sync_for_cpu(msm_obj);
 
+		dma_unmap_sgtable(drm_dev_dma_dev(obj->dev),
+				  msm_obj->sgt,
+				  DMA_BIDIRECTIONAL,
+				  DMA_ATTR_SKIP_CPU_SYNC);
+
 		sg_free_table(msm_obj->sgt);
 		kfree(msm_obj->sgt);
 		msm_obj->sgt = NULL;
-- 
2.56.0

Re: [PATCH v2 2/5] drm/msm/gem: dma_map/unmap_sgtable()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/msm: NULL deref in get_pages() error path via drm_prime_gem_destroy()
- [High] drm/msm: double dma_map/unmap_sgtable() in get_pages() and put_pages()
--

commit 5b4a3d83276519e9cb8831bdfa37a513b1d334a3
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm/gem: dma_map/unmap_sgtable()

This commit adds dma_map_sgtable() and dma_unmap_sgtable() calls in
get_pages() and put_pages() to populate the sgtable's dma_address and
dma_len fields for setting up pagetable mappings.

> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index 181b9ad326729..72e3cd8bb1e7c 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
> @@ -219,6 +219,15 @@ static struct page **get_pages(struct drm_gem_object *obj)
>  			goto error;
>  		}
>  
> +		ret = dma_map_sgtable(drm_dev_dma_dev(obj->dev),
> +				      msm_obj->sgt,
> +				      DMA_BIDIRECTIONAL,
> +				      DMA_ATTR_SKIP_CPU_SYNC);
> +		if (ret) {
> +			DRM_DEV_ERROR(dev->dev, "could not map sgtable: %d\n", ret);
> +			goto error;
> +		}
> +
>  		update_device_mem(dev->dev_private, obj->size);
>  
>  		/* For non-cached buffers, ensure the new pages are clean
> @@ -233,6 +242,8 @@ static struct page **get_pages(struct drm_gem_object *obj)
>  	return msm_obj->pages;
>  
>  error:
> +	if (!IS_ERR(msm_obj->sgt))
> +		drm_prime_gem_destroy(obj, msm_obj->sgt);

[Severity: High]
Will calling drm_prime_gem_destroy() here dereference a NULL pointer and
leak msm_obj->sgt when dma_map_sgtable() fails?

At the start of get_pages(), imported DMA-BUF objects are rejected:

drivers/gpu/drm/msm/msm_gem.c:get_pages()
	if (drm_WARN_ON_ONCE(obj->dev, drm_gem_is_imported(obj)))
		return ERR_PTR(-EINVAL);

Because get_pages() only runs on native GEM objects, obj->import_attach is
always NULL and msm_obj->sgt is allocated via drm_prime_pages_to_sg().

If dma_map_sgtable() fails and jumps to error, drm_prime_gem_destroy() is
called with a valid msm_obj->sgt, which then dereferences attach (NULL) at
attach->dmabuf while obj->resv is held, and fails to free msm_obj->sgt:

drivers/gpu/drm/drm_prime.c:drm_prime_gem_destroy()
	attach = obj->import_attach;
	if (sg)
		dma_buf_unmap_attachment_unlocked(attach, sg, DMA_BIDIRECTIONAL);
	dma_buf = attach->dmabuf;

Should the error path instead free msm_obj->sgt with
sg_free_table(msm_obj->sgt) and kfree(msm_obj->sgt) to match put_pages()?

>  	msm_obj->sgt = NULL;
>  
>  	drm_gem_put_pages(obj, msm_obj->pages, false, false);
> @@ -260,6 +271,11 @@ static void put_pages(struct drm_gem_object *obj)
>  		if (msm_obj->flags & MSM_BO_WC)
>  			sync_for_cpu(msm_obj);
>  
> +		dma_unmap_sgtable(drm_dev_dma_dev(obj->dev),
> +				  msm_obj->sgt,
> +				  DMA_BIDIRECTIONAL,
> +				  DMA_ATTR_SKIP_CPU_SYNC);

[Severity: High]
Does this map and unmap msm_obj->sgt twice whenever MSM_BO_WC is set?

In get_pages(), right after dma_map_sgtable() maps msm_obj->sgt with
DMA_ATTR_SKIP_CPU_SYNC, objects with MSM_BO_WC call sync_for_device():

drivers/gpu/drm/msm/msm_gem.c:get_pages()
		if (msm_obj->flags & MSM_BO_WC)
			sync_for_device(msm_obj);

And in put_pages(), before the new dma_unmap_sgtable() call, MSM_BO_WC
objects call sync_for_cpu(). However, sync_for_device() and sync_for_cpu()
themselves call dma_map_sgtable() and dma_unmap_sgtable() on the same
msm_obj->sgt and device:

drivers/gpu/drm/msm/msm_gem.c:
static void sync_for_device(struct msm_gem_object *msm_obj)
{
	struct device *dev = msm_obj->base.dev->dev;

	dma_map_sgtable(dev, msm_obj->sgt, DMA_BIDIRECTIONAL, 0);
}

static void sync_for_cpu(struct msm_gem_object *msm_obj)
{
	struct device *dev = msm_obj->base.dev->dev;

	dma_unmap_sgtable(dev, msm_obj->sgt, DMA_BIDIRECTIONAL, 0);
}

On platforms where the top-level DRM device uses IOMMU DMA ops or SWIOTLB,
the second dma_map_sgtable() call in sync_for_device() overwrites
sg_dma_address and sg_dma_len, leaking the first IOVA or bounce mapping,
while put_pages() unmaps and frees the second range twice. With
CONFIG_DMA_API_DEBUG enabled, this also triggers overlapping mapping and
double unmap warnings.

Should sync_for_device() and sync_for_cpu() use dma_sync_sgtable_for_device()
and dma_sync_sgtable_for_cpu() (or fold the CPU sync attribute into the map
and unmap calls in get_pages() and put_pages()) instead of mapping and
unmapping msm_obj->sgt a second time?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027152821.51144-1-robin.clark@oss.qualcomm.com?part=2

[PATCH v2 3/5] drm/msm: Extract out map/unmap helpers

[Rob Clark]

We're going to re-use these in the next commit.  No functional changes,
just some ugly callbacks.

Tested-by: Jianfeng Liu <liujianfeng1994@gmail.com> # x1e78100 (Acer SFA14-11), v7.3-rc5 + this series, CONFIG_DMABUF_DEBUG=y
Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/msm_iommu.c | 155 +++++++++++++++++++++-----------
 1 file changed, 101 insertions(+), 54 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
index da6782fca6bd..78c82f4a01b9 100644
--- a/drivers/gpu/drm/msm/msm_iommu.c
+++ b/drivers/gpu/drm/msm/msm_iommu.c
@@ -52,9 +52,8 @@ static struct msm_iommu_pagetable *to_pagetable(struct msm_mmu *mmu)
 }
 
 /* based on iommu_pgsize() in iommu.c: */
-static size_t calc_pgsize(struct msm_iommu_pagetable *pagetable,
-			   unsigned long iova, phys_addr_t paddr,
-			   size_t size, size_t *count)
+static size_t calc_pgsize(unsigned long pgsize_bitmap, unsigned long iova,
+			  phys_addr_t paddr, size_t size, size_t *count)
 {
 	unsigned int pgsize_idx, pgsize_idx_next;
 	unsigned long pgsizes;
@@ -62,7 +61,7 @@ static size_t calc_pgsize(struct msm_iommu_pagetable *pagetable,
 	unsigned long addr_merge = paddr | iova;
 
 	/* Page sizes supported by the hardware and small enough for @size */
-	pgsizes = pagetable->pgsize_bitmap & GENMASK(__fls(size), 0);
+	pgsizes = pgsize_bitmap & GENMASK(__fls(size), 0);
 
 	/* Constrain the page sizes further based on the maximum alignment */
 	if (likely(addr_merge))
@@ -78,7 +77,7 @@ static size_t calc_pgsize(struct msm_iommu_pagetable *pagetable,
 		return pgsize;
 
 	/* Find the next biggest support page size, if it exists */
-	pgsizes = pagetable->pgsize_bitmap & ~GENMASK(pgsize_idx, 0);
+	pgsizes = pgsize_bitmap & ~GENMASK(pgsize_idx, 0);
 	if (!pgsizes)
 		goto out_set_count;
 
@@ -107,24 +106,27 @@ static size_t calc_pgsize(struct msm_iommu_pagetable *pagetable,
 	return pgsize;
 }
 
-static int msm_iommu_pagetable_unmap(struct msm_mmu *mmu, u64 iova,
-		size_t size)
+typedef size_t (*unmap_fn)(void *arg, u64 iova, size_t pgsize, size_t count);
+typedef int (*map_fn)(void *arg, phys_addr_t paddr, u64 iova, size_t pgsize,
+		      size_t pgcount, int prot, size_t *mapped);
+
+static inline int
+__do_unmap(unsigned long pgsize_bitmap, u64 iova, size_t size,
+	   void *arg, unmap_fn unmap)
 {
-	struct msm_iommu_pagetable *pagetable = to_pagetable(mmu);
-	struct io_pgtable_ops *ops = pagetable->pgtbl_ops;
 	int ret = 0;
 
 	while (size) {
 		size_t pgsize, count;
 		ssize_t unmapped;
 
-		pgsize = calc_pgsize(pagetable, iova, iova, size, &count);
+		pgsize = calc_pgsize(pgsize_bitmap, iova, iova, size, &count);
 
-		unmapped = ops->unmap_pages(ops, iova, pgsize, count, NULL);
+		unmapped = unmap(arg, iova, pgsize, count);
 		if (unmapped <= 0) {
 			ret = -EINVAL;
 			/*
-			 * Continue attempting to unamp the remained of the
+			 * Continue attempting to unmap the remained of the
 			 * range, so we don't end up with some dangling
 			 * mapped pages
 			 */
@@ -135,6 +137,83 @@ static int msm_iommu_pagetable_unmap(struct msm_mmu *mmu, u64 iova,
 		size -= unmapped;
 	}
 
+	return ret;
+}
+
+static inline int
+__do_map(unsigned long pgsize_bitmap, u64 iova, struct sg_table *sgt, size_t off,
+	 size_t len, int prot, void *arg, map_fn map, unmap_fn unmap)
+{
+	struct scatterlist *sg;
+	u64 addr = iova;
+	unsigned int i;
+
+	for_each_sgtable_sg (sgt, sg, i) {
+		size_t size = sg->length;
+		phys_addr_t phys = sg_phys(sg);
+
+		if (!len)
+			break;
+
+		if (size <= off) {
+			off -= size;
+			continue;
+		}
+
+		phys += off;
+		size -= off;
+		size = min_t(size_t, size, len);
+		off = 0;
+
+		while (size) {
+			size_t pgsize, count, mapped = 0;
+			int ret;
+
+			pgsize = calc_pgsize(pgsize_bitmap, addr, phys, size, &count);
+
+			ret = map(arg, phys, addr, pgsize, count, prot, &mapped);
+
+			/* map_pages could fail after mapping some of the pages,
+			 * so update the counters before error handling.
+			 */
+			phys += mapped;
+			addr += mapped;
+			size -= mapped;
+			len  -= mapped;
+
+			if (ret) {
+				__do_unmap(pgsize_bitmap, iova, addr - iova, arg, unmap);
+				return -EINVAL;
+			}
+		}
+	}
+
+	return 0;
+}
+
+static size_t
+__unmap_pgtable(void *arg, u64 iova, size_t pgsize, size_t pgcount)
+{
+	struct io_pgtable_ops *ops = arg;
+	return ops->unmap_pages(ops, iova, pgsize, pgcount, NULL);
+}
+
+static int
+__map_pgtable(void *arg, phys_addr_t paddr, u64 iova, size_t pgsize,
+	      size_t pgcount, int prot, size_t *mapped)
+{
+	struct io_pgtable_ops *ops = arg;
+	return ops->map_pages(ops, iova, paddr, pgsize, pgcount, prot, GFP_KERNEL, mapped);
+}
+
+static int msm_iommu_pagetable_unmap(struct msm_mmu *mmu, u64 iova, size_t size)
+{
+	struct msm_iommu_pagetable *pagetable = to_pagetable(mmu);
+	struct io_pgtable_ops *ops = pagetable->pgtbl_ops;
+	int ret = 0;
+
+	ret = __do_unmap(pagetable->pgsize_bitmap, iova, size, ops, __unmap_pgtable);
+
 	iommu_flush_iotlb_all(to_msm_iommu(pagetable->parent)->domain);
 
 	return ret;
@@ -176,55 +255,23 @@ static int msm_iommu_pagetable_map(struct msm_mmu *mmu, u64 iova,
 {
 	struct msm_iommu_pagetable *pagetable = to_pagetable(mmu);
 	struct io_pgtable_ops *ops = pagetable->pgtbl_ops;
-	struct scatterlist *sg;
-	u64 addr = iova;
-	unsigned int i;
+	int ret;
 
 	if (!sgt)
 		return msm_iommu_pagetable_map_prr(mmu, iova, len, prot);
 
-	for_each_sgtable_sg(sgt, sg, i) {
-		size_t size = sg->length;
-		phys_addr_t phys = sg_phys(sg);
-
-		if (!len)
-			break;
-
-		if (size <= off) {
-			off -= size;
-			continue;
-		}
-
-		phys += off;
-		size -= off;
-		size = min_t(size_t, size, len);
-		off = 0;
-
-		while (size) {
-			size_t pgsize, count, mapped = 0;
-			int ret;
-
-			pgsize = calc_pgsize(pagetable, addr, phys, size, &count);
-
-			ret = ops->map_pages(ops, addr, phys, pgsize, count,
-					     prot, GFP_KERNEL, &mapped);
-
-			/* map_pages could fail after mapping some of the pages,
-			 * so update the counters before error handling.
-			 */
-			phys += mapped;
-			addr += mapped;
-			size -= mapped;
-			len  -= mapped;
+	ret = __do_map(pagetable->pgsize_bitmap, iova, sgt, off, len, prot,
+		       ops, __map_pgtable, __unmap_pgtable);
 
-			if (ret) {
-				msm_iommu_pagetable_unmap(mmu, iova, addr - iova);
-				return -EINVAL;
-			}
-		}
+	if (ret) {
+		/*
+		 * If the map failed mid-way, it will have triggered an
+		 * unmap, so we need to flush the TLB:
+		 */
+		iommu_flush_iotlb_all(to_msm_iommu(pagetable->parent)->domain);
 	}
 
-	return 0;
+	return ret;
 }
 
 static void msm_iommu_pagetable_destroy(struct msm_mmu *mmu)
-- 
2.56.0

[PATCH v2 4/5] drm/msm: Convert iommu map/unmap to helpers

[Rob Clark]

Re-use the previously introduced map/unmap helpers in the iommu path
(TTBR1 for GPU, ie. kernel managed buffers, and display/etc).

The callbacks work in terms of iommu_domain/ops instead of io_pgtable.
It would be kinda nice to just deal with io_pgtable, but the tlb ops
that arm-smmu uses for it's own io_pgtable require doing runpm, which
we wouldn't otherwise be able to do.

Tested-by: Jianfeng Liu <liujianfeng1994@gmail.com> # x1e78100 (Acer SFA14-11), v7.3-rc5 + this series, CONFIG_DMABUF_DEBUG=y
Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/msm_iommu.c | 35 +++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
index 78c82f4a01b9..435736c8d715 100644
--- a/drivers/gpu/drm/msm/msm_iommu.c
+++ b/drivers/gpu/drm/msm/msm_iommu.c
@@ -731,12 +731,27 @@ static void msm_iommu_detach(struct msm_mmu *mmu)
 	iommu_detach_device(iommu->domain, mmu->dev);
 }
 
+static size_t
+__unmap_iommu(void *arg, u64 iova, size_t pgsize, size_t pgcount)
+{
+	struct iommu_domain *domain = arg;
+	return domain->ops->unmap_pages(domain, iova, pgsize, pgcount, NULL);
+}
+
+static int
+__map_iommu(void *arg, phys_addr_t paddr, u64 iova, size_t pgsize,
+	    size_t pgcount, int prot, size_t *mapped)
+{
+	struct iommu_domain *domain = arg;
+	return domain->ops->map_pages(domain, iova, paddr, pgsize, pgcount,
+				      prot, GFP_KERNEL, mapped);
+}
+
 static int msm_iommu_map(struct msm_mmu *mmu, uint64_t iova,
 			 struct sg_table *sgt, size_t off, size_t len,
 			 int prot)
 {
-	struct msm_iommu *iommu = to_msm_iommu(mmu);
-	ssize_t ret;
+	struct iommu_domain *domain = to_msm_iommu(mmu)->domain;
 
 	WARN_ON(off != 0);
 
@@ -744,23 +759,23 @@ static int msm_iommu_map(struct msm_mmu *mmu, uint64_t iova,
 	if (iova & BIT_ULL(48))
 		iova |= GENMASK_ULL(63, 49);
 
-	ret = iommu_map_sgtable(iommu->domain, iova, sgt, prot);
-	if (ret < 0)
-		return ret;
-
-	return (ret == len) ? 0 : -EINVAL;
+	return __do_map(domain->pgsize_bitmap, iova, sgt, off, len, prot,
+			domain, __map_iommu, __unmap_iommu);
 }
 
 static int msm_iommu_unmap(struct msm_mmu *mmu, uint64_t iova, size_t len)
 {
-	struct msm_iommu *iommu = to_msm_iommu(mmu);
+	struct iommu_domain *domain = to_msm_iommu(mmu)->domain;
+	int ret;
 
 	if (iova & BIT_ULL(48))
 		iova |= GENMASK_ULL(63, 49);
 
-	iommu_unmap(iommu->domain, iova, len);
+	ret = __do_unmap(domain->pgsize_bitmap, iova, len, domain, __unmap_iommu);
 
-	return 0;
+	iommu_flush_iotlb_all(domain);
+
+	return ret;
 }
 
 static void msm_iommu_destroy(struct msm_mmu *mmu)
-- 
2.56.0

Re: [PATCH v2 4/5] drm/msm: Convert iommu map/unmap to helpers

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] drm/msm: skipped leaf IOTLB invalidation in msm_iommu_unmap()
- [High] drm/msm: missing IOTLB flush on error rollback in msm_iommu_map()
- [High] drm/msm: missing iotlb_sync_map call in msm_iommu_map()
--

commit 65df32b670bbcd4a7d2a95abeebac540248b0bbd
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Convert iommu map/unmap to helpers

This commit converts msm_iommu_map() and msm_iommu_unmap() to use the
__do_map() and __do_unmap() helpers with callbacks that invoke
domain->ops->map_pages() and domain->ops->unmap_pages() directly.

> diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
> index 78c82f4a01b92..435736c8d7150 100644
> --- a/drivers/gpu/drm/msm/msm_iommu.c
> +++ b/drivers/gpu/drm/msm/msm_iommu.c
> @@ -731,12 +731,27 @@ static void msm_iommu_detach(struct msm_mmu *mmu)
>  	iommu_detach_device(iommu->domain, mmu->dev);
>  }
>  
> +static size_t
> +__unmap_iommu(void *arg, u64 iova, size_t pgsize, size_t pgcount)
> +{
> +	struct iommu_domain *domain = arg;
> +	return domain->ops->unmap_pages(domain, iova, pgsize, pgcount, NULL);
> +}
> +
> +static int
> +__map_iommu(void *arg, phys_addr_t paddr, u64 iova, size_t pgsize,
> +	    size_t pgcount, int prot, size_t *mapped)
> +{
> +	struct iommu_domain *domain = arg;
> +	return domain->ops->map_pages(domain, iova, paddr, pgsize, pgcount,
> +				      prot, GFP_KERNEL, mapped);
> +}
> +
>  static int msm_iommu_map(struct msm_mmu *mmu, uint64_t iova,
>  			 struct sg_table *sgt, size_t off, size_t len,
>  			 int prot)
>  {
> -	struct msm_iommu *iommu = to_msm_iommu(mmu);
> -	ssize_t ret;
> +	struct iommu_domain *domain = to_msm_iommu(mmu)->domain;
>  
>  	WARN_ON(off != 0);
>  
> @@ -744,23 +759,23 @@ static int msm_iommu_map(struct msm_mmu *mmu, uint64_t iova,
>  	if (iova & BIT_ULL(48))
>  		iova |= GENMASK_ULL(63, 49);
>  
> -	ret = iommu_map_sgtable(iommu->domain, iova, sgt, prot);
> -	if (ret < 0)
> -		return ret;
> -
> -	return (ret == len) ? 0 : -EINVAL;
> +	return __do_map(domain->pgsize_bitmap, iova, sgt, off, len, prot,
> +			domain, __map_iommu, __unmap_iommu);
>  }

[Severity: High]
Does msm_iommu_map() miss an IOTLB flush when __do_map() fails mid-way?

If __map_iommu() fails after some pages have already been mapped,
__do_map() unwinds the partial mappings by calling __do_unmap() with
__unmap_iommu():

drivers/gpu/drm/msm/msm_iommu.c:__do_map() {
    ...
			if (ret) {
				__do_unmap(pgsize_bitmap, iova, addr - iova, arg, unmap);
				return -EINVAL;
			}
    ...
}

Because __unmap_iommu() passes NULL for the iotlb_gather parameter to
domain->ops->unmap_pages() and __do_unmap() does not flush the IOTLB,
stale translations for the rolled-back pages can remain cached in the
IOTLB after msm_iommu_map() returns an error.

In comparison, msm_iommu_pagetable_map() in the same file explicitly
flushes the IOTLB on error:

drivers/gpu/drm/msm/msm_iommu.c:msm_iommu_pagetable_map() {
    ...
	if (ret) {
		/*
		 * If the map failed mid-way, it will have triggered an
		 * unmap, so we need to flush the TLB:
		 */
		iommu_flush_iotlb_all(to_msm_iommu(pagetable->parent)->domain);
	}
    ...
}

Also, by replacing iommu_map_sgtable() with direct calls to
domain->ops->map_pages() via __map_iommu(), msm_iommu_map() no longer
invokes domain->ops->iotlb_sync_map().

On platforms using drivers/iommu/msm_iommu.c (such as APQ8064),
msm_iommu_ops defines .iotlb_sync_map = msm_iommu_sync_map to flush the
IOTLB after mapping pages. Without calling iotlb_sync_map, will those
platforms miss required IOTLB synchronization on map?

>  
>  static int msm_iommu_unmap(struct msm_mmu *mmu, uint64_t iova, size_t len)
>  {
> -	struct msm_iommu *iommu = to_msm_iommu(mmu);
> +	struct iommu_domain *domain = to_msm_iommu(mmu)->domain;
> +	int ret;
>  
>  	if (iova & BIT_ULL(48))
>  		iova |= GENMASK_ULL(63, 49);
>  
> -	iommu_unmap(iommu->domain, iova, len);
> +	ret = __do_unmap(domain->pgsize_bitmap, iova, len, domain, __unmap_iommu);
>  
> -	return 0;
> +	iommu_flush_iotlb_all(domain);
> +
> +	return ret;
>  }

[Severity: Critical]
Can replacing iommu_unmap() with __do_unmap() and iommu_flush_iotlb_all()
skip leaf IOTLB invalidations on SoCs using qcom_iommu?

Here, msm_iommu_unmap() calls __do_unmap() with __unmap_iommu(), which
passes a NULL iotlb_gather pointer to domain->ops->unmap_pages().

In __arm_lpae_unmap() in drivers/iommu/io-pgtable-arm.c, per-page leaf TLB
invalidations are only issued when gather is non-NULL:

drivers/iommu/io-pgtable-arm.c:__arm_lpae_unmap() {
    ...
		if (gather && !iommu_iotlb_gather_queued(gather))
			for (int j = 0; j < i; j++)
				io_pgtable_tlb_add_page(iop, gather, iova + j * size, size);
    ...
}

When msm_iommu_unmap() subsequently calls iommu_flush_iotlb_all(domain),
qcom_iommu_flush_iotlb_all() in drivers/iommu/arm/arm-smmu/qcom_iommu.c
only issues a TLBSYNC without issuing a context-wide TLBIASID invalidation:

drivers/iommu/arm/arm-smmu/qcom_iommu.c:qcom_iommu_flush_iotlb_all() {
	struct qcom_iommu_domain *qcom_domain = to_qcom_iommu_domain(domain);
	struct io_pgtable *pgtable = container_of(qcom_domain->pgtbl_ops,
						  struct io_pgtable, ops);
	if (!qcom_domain->pgtbl_ops)
		return;

	pm_runtime_get_sync(qcom_domain->iommu->dev);
	qcom_iommu_tlb_sync(pgtable->cookie);
	pm_runtime_put_sync(qcom_domain->iommu->dev);
}

Because gather is NULL during unmap_pages() (skipping
qcom_iommu_tlb_add_page() and its ARM_SMMU_CB_S1_TLBIVAL writes) and
qcom_iommu_flush_iotlb_all() only syncs prior invalidations, wouldn't leaf
IOTLB entries remain uninvalidated after unmapping buffers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027152821.51144-1-robin.clark@oss.qualcomm.com?part=4

[PATCH v2 5/5] drm/msm: Convert map helper to use dma-address

[Rob Clark]

Now that we have everything unified in one path, switch over to using
dma addresses for setting up the mapping.  This avoids issues with
CONFIG_DMABUF_DEBUG stripping the pages from the sgt.

Tested-by: Jianfeng Liu <liujianfeng1994@gmail.com> # x1e78100 (Acer SFA14-11), v7.3-rc5 + this series, CONFIG_DMABUF_DEBUG=y
Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/msm_iommu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
index 435736c8d715..6258c942e66a 100644
--- a/drivers/gpu/drm/msm/msm_iommu.c
+++ b/drivers/gpu/drm/msm/msm_iommu.c
@@ -148,9 +148,9 @@ __do_map(unsigned long pgsize_bitmap, u64 iova, struct sg_table *sgt, size_t off
 	u64 addr = iova;
 	unsigned int i;
 
-	for_each_sgtable_sg (sgt, sg, i) {
-		size_t size = sg->length;
-		phys_addr_t phys = sg_phys(sg);
+	for_each_sgtable_dma_sg (sgt, sg, i) {
+		size_t size = sg_dma_len(sg);
+		phys_addr_t phys = sg_dma_address(sg);
 
 		if (!len)
 			break;
-- 
2.56.0

Re: [PATCH v2 5/5] drm/msm: Convert map helper to use dma-address

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] drm/msm: __do_map() uses sg_dma_address() as CPU physical address
- [High] drm/msm: double dma_map_sgtable() and unmap in get_pages() and put_pages()
- [High] drm/msm: NULL deref in get_pages() via drm_prime_gem_destroy() on error
- [High] drm/msm: missing IOTLB invalidation in __unmap_iommu() and msm_iommu_map()
--

commit e639a62aaac331070fa9f9ceece15e985cb308ad
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Convert map helper to use dma-address

This series updates the MSM DRM driver to DMA-map GEM scatter-gather tables
and converts the IOMMU and page table mapping helpers to iterate over DMA
addresses and lengths rather than struct page pointers.

> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index 130ae27ee00be..72e3cd8bb1e7c 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
> @@ -190,6 +190,7 @@ static void update_lru(struct drm_gem_object *obj)
>  static struct page **get_pages(struct drm_gem_object *obj)
>  {
>  	struct msm_gem_object *msm_obj = to_msm_bo(obj);
> +	int ret = 0;
>  
>  	msm_gem_assert_locked(obj);
>  
> @@ -209,19 +210,26 @@ static struct page **get_pages(struct drm_gem_object *obj)
>  			return p;
>  		}
>  
> -		update_device_mem(dev->dev_private, obj->size);
> -
>  		msm_obj->pages = p;
>  
>  		msm_obj->sgt = drm_prime_pages_to_sg(obj->dev, p, npages);
>  		if (IS_ERR(msm_obj->sgt)) {
> -			void *ptr = ERR_CAST(msm_obj->sgt);
> -
>  			DRM_DEV_ERROR(dev->dev, "failed to allocate sgt\n");
> -			msm_obj->sgt = NULL;
> -			return ptr;
> +			ret = PTR_ERR(msm_obj->sgt);
> +			goto error;
>  		}
>  
> +		ret = dma_map_sgtable(drm_dev_dma_dev(obj->dev),
> +				      msm_obj->sgt,
> +				      DMA_BIDIRECTIONAL,
> +				      DMA_ATTR_SKIP_CPU_SYNC);
> +		if (ret) {
> +			DRM_DEV_ERROR(dev->dev, "could not map sgtable: %d\n", ret);
> +			goto error;
> +		}
> +
> +		update_device_mem(dev->dev_private, obj->size);
> +
>  		/* For non-cached buffers, ensure the new pages are clean
>  		 * because display controller, GPU, etc. are not coherent:
>  		 */
> @@ -232,6 +240,16 @@ static struct page **get_pages(struct drm_gem_object *obj)
>  	}
>  
>  	return msm_obj->pages;
> +
> +error:
> +	if (!IS_ERR(msm_obj->sgt))
> +		drm_prime_gem_destroy(obj, msm_obj->sgt);

[Severity: High]
Won't calling drm_prime_gem_destroy() in the error path of get_pages() cause a
NULL pointer dereference when dma_map_sgtable() fails?

In drivers/gpu/drm/msm/msm_gem.c, get_pages() is only used for non-imported
GEM objects and rejects imported objects at the start:

    if (drm_WARN_ON_ONCE(obj->dev, drm_gem_is_imported(obj)))
        return ERR_PTR(-EINVAL);

Because obj->import_attach is NULL for native GEM objects, jumping to error
after drm_prime_pages_to_sg() succeeds calls drm_prime_gem_destroy(), which
dereferences attach->dmabuf:

    attach = obj->import_attach;
    if (sg)
        dma_buf_unmap_attachment_unlocked(attach, sg, DMA_BIDIRECTIONAL);
    dma_buf = attach->dmabuf;

Should this error path call sg_free_table(msm_obj->sgt) and
kfree(msm_obj->sgt) instead of drm_prime_gem_destroy()?

> +	msm_obj->sgt = NULL;
> +
> +	drm_gem_put_pages(obj, msm_obj->pages, false, false);
> +	msm_obj->pages = NULL;
> +
> +	return ERR_PTR(ret);
>  }
>  
>  static void put_pages(struct drm_gem_object *obj)
> @@ -246,18 +264,21 @@ static void put_pages(struct drm_gem_object *obj)
>  		drm_gpuvm_bo_gem_evict(obj, true);
>  
>  	if (msm_obj->pages) {
> -		if (msm_obj->sgt) {
> -			/* For non-cached buffers, ensure the new
> -			 * pages are clean because display controller,
> -			 * GPU, etc. are not coherent:
> -			 */
> -			if (msm_obj->flags & MSM_BO_WC)
> -				sync_for_cpu(msm_obj);
> +		/* For non-cached buffers, ensure the new
> +		 * pages are clean because display controller,
> +		 * GPU, etc. are not coherent:
> +		 */
> +		if (msm_obj->flags & MSM_BO_WC)
> +			sync_for_cpu(msm_obj);
>  
> -			sg_free_table(msm_obj->sgt);
> -			kfree(msm_obj->sgt);
> -			msm_obj->sgt = NULL;
> -		}
> +		dma_unmap_sgtable(drm_dev_dma_dev(obj->dev),
> +				  msm_obj->sgt,
> +				  DMA_BIDIRECTIONAL,
> +				  DMA_ATTR_SKIP_CPU_SYNC);

[Severity: High]
Doesn't this map msm_obj->sgt twice in get_pages() and unmap it twice in
put_pages() whenever MSM_BO_WC is set?

In get_pages(), dma_map_sgtable() is called with DMA_ATTR_SKIP_CPU_SYNC, and
then if (msm_obj->flags & MSM_BO_WC) is true, sync_for_device() is called:

    static void sync_for_device(struct msm_gem_object *msm_obj)
    {
        struct device *dev = msm_obj->base.dev->dev;

        dma_map_sgtable(dev, msm_obj->sgt, DMA_BIDIRECTIONAL, 0);
    }

Symmetrically, in put_pages(), sync_for_cpu() calls dma_unmap_sgtable(dev,
msm_obj->sgt, DMA_BIDIRECTIONAL, 0), followed immediately by a second
dma_unmap_sgtable() with DMA_ATTR_SKIP_CPU_SYNC on the same sg_table and
device.

With CONFIG_DMA_API_DEBUG enabled, mapping an already-mapped scatterlist
triggers overlapping DMA-API debug warnings in debug_dma_map_sg() and
double-unmap warnings in debug_dma_unmap_sg(). Additionally, if
use_dma_iommu(dev) is true, the second dma_map_sgtable() overwrites
sg_dma_address(sg) and leaks the first IOVA range in group->default_domain,
while the second dma_unmap_sgtable() frees the second IOVA range twice in
iommu_dma_unmap_sg().

> diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
> index da6782fca6bd2..6258c942e66ad 100644
> --- a/drivers/gpu/drm/msm/msm_iommu.c
> +++ b/drivers/gpu/drm/msm/msm_iommu.c
[ ... ]
> @@ -135,6 +137,83 @@ static int msm_iommu_pagetable_unmap(struct msm_mmu *mmu, u64 iova, size_t size)
>  		size -= unmapped;
>  	}
>  
> +	return ret;
> +}
> +
> +static inline int
> +__do_map(unsigned long pgsize_bitmap, u64 iova, struct sg_table *sgt, size_t off,
> +	 size_t len, int prot, void *arg, map_fn map, unmap_fn unmap)
> +{
> +	struct scatterlist *sg;
> +	u64 addr = iova;
> +	unsigned int i;
> +
> +	for_each_sgtable_dma_sg (sgt, sg, i) {
> +		size_t size = sg_dma_len(sg);
> +		phys_addr_t phys = sg_dma_address(sg);

[Severity: Critical]
Is it safe to treat sg_dma_address(sg) and sg_dma_len(sg) as CPU physical
addresses and physical segment lengths when programming the GPU and display
page tables in __do_map()?

Both __map_pgtable() (calling io_pgtable_ops->map_pages()) and __map_iommu()
(calling iommu_domain->ops->map_pages()) expect paddr to be a CPU physical
address.

In get_pages() and drm_gem_prime_import(), the sg_table is DMA-mapped for
drm_dev_dma_dev(obj->dev) (which is obj->dev->dev). As noted in the comment
above sync_for_device() in drivers/gpu/drm/msm/msm_gem.c and in
msm_kms_init_vm() in drivers/gpu/drm/msm/msm_kms.c, depending on the display
generation, the display IOMMU is wired either to the parent mdss device or
directly to the mdp sub-node (obj->dev->dev):

    if (device_iommu_mapped(mdp_dev))
        iommu_dev = mdp_dev;
    else if (mdss_dev && device_iommu_mapped(mdss_dev))
        iommu_dev = mdss_dev;

On platforms where the IOMMU is wired to the mdp node and the IOMMU driver
does not select IOMMU_DOMAIN_IDENTITY as the default domain (such as SDM630 /
SDM660 with "qcom,mdp5", MSM8996, and platforms using qcom_iommu), the IOMMU
core allocates a DMA default_domain during probe and calls
iommu_setup_dma_ops(), setting use_dma_iommu(dev) to true.

Even when msm_iommu_new() later attaches an unmanaged domain via
iommu_attach_device(), use_dma_iommu(dev) remains true. In
kernel/dma/mapping.c, dma_go_direct() checks use_dma_iommu(dev) before !ops:

    static bool dma_go_direct(struct device *dev, dma_addr_t mask,
            const struct dma_map_ops *ops)
    {
        if (use_dma_iommu(dev))
            return false;

        if (likely(!ops))
            return true;

As a result, dma_map_sgtable() calls iommu_dma_map_sg(), which allocates a
virtual IOVA in group->default_domain, coalesces physically non-contiguous
segments into contiguous IOVA ranges, and writes that virtual IOVA into
sg_dma_address(sg) and the coalesced length into sg_dma_len(sg). Passing those
values to __do_map() will program the GPU and display page tables with virtual
IOVAs instead of physical addresses.

[ ... ]
> @@ -684,12 +731,27 @@ static void msm_iommu_detach(struct msm_mmu *mmu)
>  	iommu_detach_device(iommu->domain, mmu->dev);
>  }
>  
> +static size_t
> +__unmap_iommu(void *arg, u64 iova, size_t pgsize, size_t pgcount)
> +{
> +	struct iommu_domain *domain = arg;
> +	return domain->ops->unmap_pages(domain, iova, pgsize, pgcount, NULL);
> +}
> +
> +static int
> +__map_iommu(void *arg, phys_addr_t paddr, u64 iova, size_t pgsize,
> +	    size_t pgcount, int prot, size_t *mapped)
> +{
> +	struct iommu_domain *domain = arg;
> +	return domain->ops->map_pages(domain, iova, paddr, pgsize, pgcount,
> +				      prot, GFP_KERNEL, mapped);
> +}
> +
>  static int msm_iommu_map(struct msm_mmu *mmu, uint64_t iova,
>  			 struct sg_table *sgt, size_t off, size_t len,
>  			 int prot)
>  {
> -	struct msm_iommu *iommu = to_msm_iommu(mmu);
> -	ssize_t ret;
> +	struct iommu_domain *domain = to_msm_iommu(mmu)->domain;
>  
>  	WARN_ON(off != 0);
>  
> @@ -697,23 +759,23 @@ static int msm_iommu_map(struct msm_mmu *mmu, uint64_t iova,
>  	if (iova & BIT_ULL(48))
>  		iova |= GENMASK_ULL(63, 49);
>  
> -	ret = iommu_map_sgtable(iommu->domain, iova, sgt, prot);
> -	if (ret < 0)
> -		return ret;
> -
> -	return (ret == len) ? 0 : -EINVAL;
> +	return __do_map(domain->pgsize_bitmap, iova, sgt, off, len, prot,
> +			domain, __map_iommu, __unmap_iommu);
>  }
>  
>  static int msm_iommu_unmap(struct msm_mmu *mmu, uint64_t iova, size_t len)
>  {
> -	struct msm_iommu *iommu = to_msm_iommu(mmu);
> +	struct iommu_domain *domain = to_msm_iommu(mmu)->domain;
> +	int ret;
>  
>  	if (iova & BIT_ULL(48))
>  		iova |= GENMASK_ULL(63, 49);
>  
> -	iommu_unmap(iommu->domain, iova, len);
> +	ret = __do_unmap(domain->pgsize_bitmap, iova, len, domain, __unmap_iommu);
>  
> -	return 0;
> +	iommu_flush_iotlb_all(domain);
> +
> +	return ret;
>  }

[Severity: High]
Can replacing iommu_map_sgtable() and iommu_unmap() with __do_map() and
__do_unmap() leave stale IOTLB entries behind?

There appear to be three cases where IOTLB maintenance is skipped:

1. In msm_iommu_map(), if __do_map() fails after mapping some pages, it rolls
back the partial mapping via __do_unmap(..., __unmap_iommu). Unlike
msm_iommu_pagetable_map(), msm_iommu_map() does not call
iommu_flush_iotlb_all(domain) on error.

2. In __unmap_iommu(), NULL is passed for the struct iommu_iotlb_gather
pointer to domain->ops->unmap_pages(). In drivers/iommu/io-pgtable-arm.c,
__arm_lpae_unmap() only calls io_pgtable_tlb_add_page() when gather is
non-NULL:

    if (gather && !iommu_iotlb_gather_queued(gather))
        io_pgtable_tlb_add_page(data, gather, iova, size);

On SoCs using drivers/iommu/arm/arm-smmu/qcom_iommu.c,
qcom_iommu_flush_iotlb_all() only issues ARM_SMMU_CB_TLBSYNC via
qcom_iommu_tlb_sync() and relies on qcom_iommu_tlb_add_page() issuing
ARM_SMMU_CB_S1_TLBIVAL during unmap_pages(). Passing gather as NULL skips the
TLB invalidation commands altogether.

3. On APQ8064 (drivers/iommu/msm_iommu.c), msm_iommu_ops implements
.iotlb_sync_map = msm_iommu_sync_map while leaving .flush_iotlb_all = NULL, so
calling domain->ops->map_pages() directly in msm_iommu_map() bypasses the
iotlb_sync_map callback that iommu_map_sgtable() previously invoked.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027152821.51144-1-robin.clark@oss.qualcomm.com?part=5