All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 0/6] drm/msm: DMABUF_DEBUG fixes
@ 2026-10-30 21:32 Rob Clark
  2026-10-30 21:32 ` [PATCH v4 1/6] drm/msm: Cleanup if pages_to_sg() fails Rob Clark
                   ` (5 more replies)
  0 siblings, 6 replies; 11+ messages in thread
From: Rob Clark @ 2026-10-30 21:32 UTC (permalink / raw)
  To: dri-devel
  Cc: linux-arm-msm, freedreno, Jianfeng Liu, Christian König,
	Dmitry Baryshkov, Rob Clark, Abhinav Kumar, David Airlie,
	Jessica Zhang, open list, Marijn Suijten, Sean Paul,
	Simona Vetter

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.

Note, for older pre-arm-smmu devices, this iommu series is a dependency:
https://lore.kernel.org/all/20261029212344.346069-1-robin.clark@oss.qualcomm.com/

v4:
- Fix get_pages() error path
- Link to v3: https://lore.kernel.org/all/20261027230452.130031-1-robin.clark@oss.qualcomm.com/

v3:
- Fixing missing tlb flushes in error paths harder
- Do a single dma_map/unmap_sgtable()
- More paranoid error checking on map path
- Link to v2: https://lore.kernel.org/all/20261027152821.51144-1-robin.clark@oss.qualcomm.com/

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 (6):
  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
  drm/msm: Treat !mapped as an error

 drivers/gpu/drm/msm/msm_gem.c   |  96 +++++++--------
 drivers/gpu/drm/msm/msm_iommu.c | 210 +++++++++++++++++++++-----------
 2 files changed, 187 insertions(+), 119 deletions(-)

-- 
2.56.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v4 1/6] drm/msm: Cleanup if pages_to_sg() fails
  2026-10-30 21:32 [PATCH v4 0/6] drm/msm: DMABUF_DEBUG fixes Rob Clark
@ 2026-10-30 21:32 ` Rob Clark
  2026-10-30 21:32 ` [PATCH v4 2/6] drm/msm/gem: dma_map/unmap_sgtable() Rob Clark
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Rob Clark @ 2026-10-30 21:32 UTC (permalink / raw)
  To: dri-devel
  Cc: linux-arm-msm, freedreno, Jianfeng Liu, Christian König,
	Dmitry Baryshkov, Rob Clark, Abhinav Kumar, Jessica Zhang,
	Sean Paul, Marijn Suijten, David Airlie, Simona Vetter, open list

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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v4 2/6] drm/msm/gem: dma_map/unmap_sgtable()
  2026-10-30 21:32 [PATCH v4 0/6] drm/msm: DMABUF_DEBUG fixes Rob Clark
  2026-10-30 21:32 ` [PATCH v4 1/6] drm/msm: Cleanup if pages_to_sg() fails Rob Clark
@ 2026-10-30 21:32 ` Rob Clark
  2026-10-30 21:46   ` sashiko-bot
  2026-10-30 21:32 ` [PATCH v4 3/6] drm/msm: Extract out map/unmap helpers Rob Clark
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 11+ messages in thread
From: Rob Clark @ 2026-10-30 21:32 UTC (permalink / raw)
  To: dri-devel
  Cc: linux-arm-msm, freedreno, Jianfeng Liu, Christian König,
	Dmitry Baryshkov, Rob Clark, Abhinav Kumar, Jessica Zhang,
	Sean Paul, Marijn Suijten, David Airlie, Simona Vetter, open list

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.

This inlines sync_for_device() and sync_for_cpu() into their only
callers to avoid doubling up on the dma-mapping calls.

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 | 65 +++++++++++++++--------------------
 1 file changed, 28 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 181b9ad32672..bcdb57866cf6 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -116,34 +116,6 @@ void msm_gem_vma_put(struct drm_gem_object *obj)
 #endif
 }
 
-/*
- * Cache sync.. this is a bit over-complicated, to fit dma-mapping
- * API.  Really GPU cache is out of scope here (handled on cmdstream)
- * and all we need to do is invalidate newly allocated pages before
- * mapping to CPU as uncached/writecombine.
- *
- * On top of this, we have the added headache, that depending on
- * display generation, the display's iommu may be wired up to either
- * the toplevel drm device (mdss), or to the mdp sub-node, meaning
- * that here we either have dma-direct or iommu ops.
- *
- * Let this be a cautionary tail of abstraction gone wrong.
- */
-
-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);
-}
-
 static void update_lru_active(struct drm_gem_object *obj)
 {
 	struct msm_drm_private *priv = obj->dev->dev_private;
@@ -201,6 +173,7 @@ static struct page **get_pages(struct drm_gem_object *obj)
 		struct drm_device *dev = obj->dev;
 		struct page **p;
 		size_t npages = obj->size >> PAGE_SHIFT;
+		unsigned map_attrs = DMA_ATTR_SKIP_CPU_SYNC;
 
 		p = drm_gem_get_pages(obj);
 
@@ -219,13 +192,20 @@ static struct page **get_pages(struct drm_gem_object *obj)
 			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:
-		 */
+		/* For non-cached buffers, ensure the new pages are clean. */
 		if (msm_obj->flags & MSM_BO_WC)
-			sync_for_device(msm_obj);
+			map_attrs = 0;
+
+		ret = dma_map_sgtable(drm_dev_dma_dev(obj->dev),
+				      msm_obj->sgt,
+				      DMA_BIDIRECTIONAL,
+				      map_attrs);
+		if (ret) {
+			DRM_DEV_ERROR(dev->dev, "could not map sgtable: %d\n", ret);
+			goto error;
+		}
+
+		update_device_mem(dev->dev_private, obj->size);
 
 		update_lru(obj);
 	}
@@ -233,6 +213,11 @@ static struct page **get_pages(struct drm_gem_object *obj)
 	return msm_obj->pages;
 
 error:
+	if (!IS_ERR(msm_obj->sgt)) {
+		sg_free_table(msm_obj->sgt);
+		kfree(msm_obj->sgt);
+		msm_obj->sgt = NULL;
+	}
 	msm_obj->sgt = NULL;
 
 	drm_gem_put_pages(obj, msm_obj->pages, false, false);
@@ -253,12 +238,18 @@ static void put_pages(struct drm_gem_object *obj)
 		drm_gpuvm_bo_gem_evict(obj, true);
 
 	if (msm_obj->pages) {
+		unsigned unmap_attrs = DMA_ATTR_SKIP_CPU_SYNC;
+
 		/* For non-cached buffers, ensure the new
-		 * pages are clean because display controller,
-		 * GPU, etc. are not coherent:
+		 * pages are clean:
 		 */
 		if (msm_obj->flags & MSM_BO_WC)
-			sync_for_cpu(msm_obj);
+			unmap_attrs = 0;
+
+		dma_unmap_sgtable(drm_dev_dma_dev(obj->dev),
+				  msm_obj->sgt,
+				  DMA_BIDIRECTIONAL,
+				  unmap_attrs);
 
 		sg_free_table(msm_obj->sgt);
 		kfree(msm_obj->sgt);
-- 
2.56.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v4 3/6] drm/msm: Extract out map/unmap helpers
  2026-10-30 21:32 [PATCH v4 0/6] drm/msm: DMABUF_DEBUG fixes Rob Clark
  2026-10-30 21:32 ` [PATCH v4 1/6] drm/msm: Cleanup if pages_to_sg() fails Rob Clark
  2026-10-30 21:32 ` [PATCH v4 2/6] drm/msm/gem: dma_map/unmap_sgtable() Rob Clark
@ 2026-10-30 21:32 ` Rob Clark
  2026-10-30 21:32 ` [PATCH v4 4/6] drm/msm: Convert iommu map/unmap to helpers Rob Clark
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Rob Clark @ 2026-10-30 21:32 UTC (permalink / raw)
  To: dri-devel
  Cc: linux-arm-msm, freedreno, Jianfeng Liu, Christian König,
	Dmitry Baryshkov, Rob Clark, Abhinav Kumar, Jessica Zhang,
	Sean Paul, Marijn Suijten, David Airlie, Simona Vetter, open list

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 | 159 +++++++++++++++++++++-----------
 1 file changed, 103 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
index da6782fca6bd..5c13ad6d5a36 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,30 @@ 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)
+struct map_ops {
+	size_t (*unmap)(void *arg, u64 iova, size_t pgsize, size_t count);
+	int (*map)(void *arg, phys_addr_t padder, u64 iova, size_t pgsize,
+		   size_t pkcount, int prot, size_t *mapped);
+	void (*flush)(void *arg);
+};
+
+static inline int
+__do_unmap(unsigned long pgsize_bitmap, u64 iova, size_t size, void *arg,
+	   const struct map_ops *ops)
 {
-	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 = ops->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,55 +140,20 @@ static int msm_iommu_pagetable_unmap(struct msm_mmu *mmu, u64 iova,
 		size -= unmapped;
 	}
 
-	iommu_flush_iotlb_all(to_msm_iommu(pagetable->parent)->domain);
+	ops->flush(arg);
 
 	return ret;
 }
 
-static int msm_iommu_pagetable_map_prr(struct msm_mmu *mmu, u64 iova, size_t len, int prot)
+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, const struct map_ops *ops)
 {
-	struct msm_iommu_pagetable *pagetable = to_pagetable(mmu);
-	struct io_pgtable_ops *ops = pagetable->pgtbl_ops;
-	struct msm_iommu *iommu = to_msm_iommu(pagetable->parent);
-	phys_addr_t phys = page_to_phys(iommu->prr_page);
-	u64 addr = iova;
-
-	while (len) {
-		size_t mapped = 0;
-		size_t size = PAGE_SIZE;
-		int ret;
-
-		ret = ops->map_pages(ops, addr, phys, size, 1, prot, GFP_KERNEL, &mapped);
-
-		/* map_pages could fail after mapping some of the pages,
-		 * so update the counters before error handling.
-		 */
-		addr += mapped;
-		len  -= mapped;
-
-		if (ret) {
-			msm_iommu_pagetable_unmap(mmu, iova, addr - iova);
-			return -EINVAL;
-		}
-	}
-
-	return 0;
-}
-
-static int msm_iommu_pagetable_map(struct msm_mmu *mmu, u64 iova,
-				   struct sg_table *sgt, size_t off, size_t len,
-				   int prot)
-{
-	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;
 
-	if (!sgt)
-		return msm_iommu_pagetable_map_prr(mmu, iova, len, prot);
-
-	for_each_sgtable_sg(sgt, sg, i) {
+	for_each_sgtable_sg (sgt, sg, i) {
 		size_t size = sg->length;
 		phys_addr_t phys = sg_phys(sg);
 
@@ -204,10 +174,9 @@ static int msm_iommu_pagetable_map(struct msm_mmu *mmu, u64 iova,
 			size_t pgsize, count, mapped = 0;
 			int ret;
 
-			pgsize = calc_pgsize(pagetable, addr, phys, size, &count);
+			pgsize = calc_pgsize(pgsize_bitmap, addr, phys, size, &count);
 
-			ret = ops->map_pages(ops, addr, phys, pgsize, count,
-					     prot, GFP_KERNEL, &mapped);
+			ret = ops->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.
@@ -218,7 +187,7 @@ static int msm_iommu_pagetable_map(struct msm_mmu *mmu, u64 iova,
 			len  -= mapped;
 
 			if (ret) {
-				msm_iommu_pagetable_unmap(mmu, iova, addr - iova);
+				__do_unmap(pgsize_bitmap, iova, addr - iova, arg, ops);
 				return -EINVAL;
 			}
 		}
@@ -227,6 +196,84 @@ static int msm_iommu_pagetable_map(struct msm_mmu *mmu, u64 iova,
 	return 0;
 }
 
+static size_t
+__unmap_pgtable(void *arg, u64 iova, size_t pgsize, size_t pgcount)
+{
+	struct msm_iommu_pagetable *pagetable = arg;
+	struct io_pgtable_ops *ops = pagetable->pgtbl_ops;
+	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 msm_iommu_pagetable *pagetable = arg;
+	struct io_pgtable_ops *ops = pagetable->pgtbl_ops;
+	return ops->map_pages(ops, iova, paddr, pgsize, pgcount, prot, GFP_KERNEL, mapped);
+}
+
+static void
+__flush_pgtable(void *arg)
+{
+	struct msm_iommu_pagetable *pagetable = arg;
+	iommu_flush_iotlb_all(to_msm_iommu(pagetable->parent)->domain);
+}
+
+static const struct map_ops pgtable_ops = {
+	__unmap_pgtable, __map_pgtable, __flush_pgtable
+};
+
+static int msm_iommu_pagetable_unmap(struct msm_mmu *mmu, u64 iova, size_t size)
+{
+	struct msm_iommu_pagetable *pagetable = to_pagetable(mmu);
+
+	return __do_unmap(pagetable->pgsize_bitmap, iova, size, pagetable, &pgtable_ops);
+}
+
+static int msm_iommu_pagetable_map_prr(struct msm_mmu *mmu, u64 iova, size_t len, int prot)
+{
+	struct msm_iommu_pagetable *pagetable = to_pagetable(mmu);
+	struct io_pgtable_ops *ops = pagetable->pgtbl_ops;
+	struct msm_iommu *iommu = to_msm_iommu(pagetable->parent);
+	phys_addr_t phys = page_to_phys(iommu->prr_page);
+	u64 addr = iova;
+
+	while (len) {
+		size_t mapped = 0;
+		size_t size = PAGE_SIZE;
+		int ret;
+
+		ret = ops->map_pages(ops, addr, phys, size, 1, prot, GFP_KERNEL, &mapped);
+
+		/* map_pages could fail after mapping some of the pages,
+		 * so update the counters before error handling.
+		 */
+		addr += mapped;
+		len  -= mapped;
+
+		if (ret) {
+			msm_iommu_pagetable_unmap(mmu, iova, addr - iova);
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
+static int msm_iommu_pagetable_map(struct msm_mmu *mmu, u64 iova,
+				   struct sg_table *sgt, size_t off, size_t len,
+				   int prot)
+{
+	struct msm_iommu_pagetable *pagetable = to_pagetable(mmu);
+
+	if (!sgt)
+		return msm_iommu_pagetable_map_prr(mmu, iova, len, prot);
+
+	return __do_map(pagetable->pgsize_bitmap, iova, sgt, off, len, prot,
+			pagetable, &pgtable_ops);
+}
+
 static void msm_iommu_pagetable_destroy(struct msm_mmu *mmu)
 {
 	struct msm_iommu_pagetable *pagetable = to_pagetable(mmu);
-- 
2.56.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v4 4/6] drm/msm: Convert iommu map/unmap to helpers
  2026-10-30 21:32 [PATCH v4 0/6] drm/msm: DMABUF_DEBUG fixes Rob Clark
                   ` (2 preceding siblings ...)
  2026-10-30 21:32 ` [PATCH v4 3/6] drm/msm: Extract out map/unmap helpers Rob Clark
@ 2026-10-30 21:32 ` Rob Clark
  2026-10-30 21:49   ` sashiko-bot
  2026-10-30 21:32 ` [PATCH v4 5/6] drm/msm: Convert map helper to use dma-address Rob Clark
  2026-10-30 21:32 ` [PATCH v4 6/6] drm/msm: Treat !mapped as an error Rob Clark
  5 siblings, 1 reply; 11+ messages in thread
From: Rob Clark @ 2026-10-30 21:32 UTC (permalink / raw)
  To: dri-devel
  Cc: linux-arm-msm, freedreno, Jianfeng Liu, Christian König,
	Dmitry Baryshkov, Rob Clark, Abhinav Kumar, Jessica Zhang,
	Sean Paul, Marijn Suijten, David Airlie, Simona Vetter, open list

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 | 43 ++++++++++++++++++++++++---------
 1 file changed, 32 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
index 5c13ad6d5a36..479e5450a4dc 100644
--- a/drivers/gpu/drm/msm/msm_iommu.c
+++ b/drivers/gpu/drm/msm/msm_iommu.c
@@ -731,12 +731,38 @@ 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 void
+__flush_iommu(void *arg)
+{
+	struct iommu_domain *domain = arg;
+	iommu_flush_iotlb_all(domain);
+}
+
+static const struct map_ops iommu_ops = {
+	__unmap_iommu, __map_iommu, __flush_iommu
+};
+
 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 +770,18 @@ 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, &iommu_ops);
 }
 
 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;
 
 	if (iova & BIT_ULL(48))
 		iova |= GENMASK_ULL(63, 49);
 
-	iommu_unmap(iommu->domain, iova, len);
-
-	return 0;
+	return __do_unmap(domain->pgsize_bitmap, iova, len, domain, &iommu_ops);
 }
 
 static void msm_iommu_destroy(struct msm_mmu *mmu)
-- 
2.56.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v4 5/6] drm/msm: Convert map helper to use dma-address
  2026-10-30 21:32 [PATCH v4 0/6] drm/msm: DMABUF_DEBUG fixes Rob Clark
                   ` (3 preceding siblings ...)
  2026-10-30 21:32 ` [PATCH v4 4/6] drm/msm: Convert iommu map/unmap to helpers Rob Clark
@ 2026-10-30 21:32 ` Rob Clark
  2026-10-30 21:57   ` sashiko-bot
  2026-10-30 21:32 ` [PATCH v4 6/6] drm/msm: Treat !mapped as an error Rob Clark
  5 siblings, 1 reply; 11+ messages in thread
From: Rob Clark @ 2026-10-30 21:32 UTC (permalink / raw)
  To: dri-devel
  Cc: linux-arm-msm, freedreno, Jianfeng Liu, Christian König,
	Dmitry Baryshkov, Rob Clark, Abhinav Kumar, Jessica Zhang,
	Sean Paul, Marijn Suijten, David Airlie, Simona Vetter, open list

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 479e5450a4dc..c2950d32c507 100644
--- a/drivers/gpu/drm/msm/msm_iommu.c
+++ b/drivers/gpu/drm/msm/msm_iommu.c
@@ -153,9 +153,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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v4 6/6] drm/msm: Treat !mapped as an error
  2026-10-30 21:32 [PATCH v4 0/6] drm/msm: DMABUF_DEBUG fixes Rob Clark
                   ` (4 preceding siblings ...)
  2026-10-30 21:32 ` [PATCH v4 5/6] drm/msm: Convert map helper to use dma-address Rob Clark
@ 2026-10-30 21:32 ` Rob Clark
  2026-10-30 21:54   ` sashiko-bot
  5 siblings, 1 reply; 11+ messages in thread
From: Rob Clark @ 2026-10-30 21:32 UTC (permalink / raw)
  To: dri-devel
  Cc: linux-arm-msm, freedreno, Jianfeng Liu, Christian König,
	Dmitry Baryshkov, Rob Clark, Abhinav Kumar, Jessica Zhang,
	Sean Paul, Marijn Suijten, David Airlie, Simona Vetter, open list

Rather than silenty mapping nothing, treat this as an error.  It should
never happen, but if it does we want to flag the VM as unusable.

Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/msm_iommu.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
index c2950d32c507..255972323f9a 100644
--- a/drivers/gpu/drm/msm/msm_iommu.c
+++ b/drivers/gpu/drm/msm/msm_iommu.c
@@ -186,6 +186,10 @@ __do_map(unsigned long pgsize_bitmap, u64 iova, struct sg_table *sgt, size_t off
 			size -= mapped;
 			len  -= mapped;
 
+			/* If nothing was mapped, consider it an ENOMEM. */
+			if (!ret && !mapped)
+				ret = -ENOMEM;
+
 			if (ret) {
 				__do_unmap(pgsize_bitmap, iova, addr - iova, arg, ops);
 				return -EINVAL;
-- 
2.56.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 2/6] drm/msm/gem: dma_map/unmap_sgtable()
  2026-10-30 21:32 ` [PATCH v4 2/6] drm/msm/gem: dma_map/unmap_sgtable() Rob Clark
@ 2026-10-30 21:46   ` sashiko-bot
  0 siblings, 0 replies; 11+ messages in thread
From: sashiko-bot @ 2026-10-30 21:46 UTC (permalink / raw)
  To: Rob Clark, dri-devel

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/msm/gem: dma_map_sgtable() in get_pages() uses iommu-dma on MDP IOMMUs
--

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

drm/msm/gem: dma_map/unmap_sgtable()

This commit inlines sync_for_device() and sync_for_cpu() into get_pages()
and put_pages(), calling dma_map_sgtable() and dma_unmap_sgtable() on all
GEM objects using DMA_ATTR_SKIP_CPU_SYNC for cached buffers to populate the
scatterlist DMA addresses and lengths.

> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index 181b9ad326729..bcdb57866cf6c 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c

[ ... ]

> @@ -219,13 +192,20 @@ static struct page **get_pages(struct drm_gem_object *obj)
>  			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:
> -		 */
> +		/* For non-cached buffers, ensure the new pages are clean. */
>  		if (msm_obj->flags & MSM_BO_WC)
> -			sync_for_device(msm_obj);
> +			map_attrs = 0;
> +
> +		ret = dma_map_sgtable(drm_dev_dma_dev(obj->dev),
> +				      msm_obj->sgt,
> +				      DMA_BIDIRECTIONAL,
> +				      map_attrs);
> +		if (ret) {
> +			DRM_DEV_ERROR(dev->dev, "could not map sgtable: %d\n", ret);
> +			goto error;
> +		}

[Severity: High]
As reported by Sashiko on v3 [1], will calling dma_map_sgtable() on
drm_dev_dma_dev(obj->dev) in get_pages() invoke iommu_dma_map_sg() instead
of dma_direct_map_sg() on platforms where the display controller device has
an IOMMU attached directly to the MDP node?

In msm_kms_init_vm(), the IOMMU can be attached directly to mdp_dev
(which is obj->dev->dev, returned by drm_dev_dma_dev(obj->dev)):

drivers/gpu/drm/msm/msm_kms.c:msm_kms_init_vm() {
	if (device_iommu_mapped(mdp_dev))
		iommu_dev = mdp_dev;
	else if (mdss_dev && device_iommu_mapped(mdss_dev))
		iommu_dev = mdss_dev;
}

This occurs on qcom_iommu platforms (such as MSM8916, MSM8939, MSM8953, and
MSM8976) where qcom_iommu_ops does not implement def_domain_type, as well as
on arm-smmu-qcom MDP5 platforms (such as MSM8996, SDM630, and SDM660) where
the iommus property is on the qcom,mdp5 node rather than the parent mdss
node and is not matched by qcom_smmu_client_of_match[]:

drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c:
static const struct of_device_id qcom_smmu_client_of_match[] __maybe_unused = {
	{ .compatible = "qcom,adreno" },
	{ .compatible = "qcom,adreno-gmu" },
	{ .compatible = "qcom,glymur-mdss" },
	{ .compatible = "qcom,mdp4" },
	{ .compatible = "qcom,mdss" },
...

On these platforms, iommu_setup_dma_ops() allocates a DMA default_domain and
sets use_dma_iommu(dev) to true on obj->dev->dev. When msm_iommu_new() later
attaches an unmanaged domain via iommu_attach_device(), use_dma_iommu(dev)
and group->default_domain remain set.

Even though get_dma_ops(dev) is NULL, dma_go_direct() checks
use_dma_iommu(dev) before checking ops:

kernel/dma/mapping.c:dma_go_direct() {
	if (use_dma_iommu(dev))
		return false;

	if (likely(!ops))
		return true;
}

As a result, get_pages() -> dma_map_sgtable() -> __dma_map_sg_attrs() routes
into iommu_dma_map_sg() for every GEM object (now including MSM_BO_CACHED
buffers). That maps the buffer into the unused group->default_domain,
coalesces sgt->nents, and populates sg_dma_address(sg) with default_domain
IOVAs rather than CPU physical addresses.

When paired with commit 2ee69d02162f ("drm/msm: Convert map helper to use
dma-address") later in this series, where __do_map() in
drivers/gpu/drm/msm/msm_iommu.c passes sg_dma_address(sg) as the physical
address to iommu_map(), won't this program the GPU and display page tables
with default_domain IOVAs instead of physical addresses?

[1] https://lore.kernel.org/r/20261027230452.130031-3-robin.clark@oss.qualcomm.com

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 4/6] drm/msm: Convert iommu map/unmap to helpers
  2026-10-30 21:32 ` [PATCH v4 4/6] drm/msm: Convert iommu map/unmap to helpers Rob Clark
@ 2026-10-30 21:49   ` sashiko-bot
  0 siblings, 0 replies; 11+ messages in thread
From: sashiko-bot @ 2026-10-30 21:49 UTC (permalink / raw)
  To: Rob Clark, dri-devel

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] drm/msm: missing leaf IOTLB invalidation in __unmap_iommu() on qcom_iommu
- [High] drm/msm: bypassed iommu_sync_map() and core IOMMU API in msm_iommu_map()
- [Low] drm/msm: build failure in __map_iommu() when !CONFIG_IOMMU_API
--

commit e0c13e8fe949f41860b8d6b287a4884b707293a0
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
internal __do_map() and __do_unmap() helpers with callbacks that invoke
iommu_domain_ops directly instead of calling iommu_map_sgtable() and
iommu_unmap().

> diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
> index 5c13ad6d5a366..479e5450a4dcd 100644
> --- a/drivers/gpu/drm/msm/msm_iommu.c
> +++ b/drivers/gpu/drm/msm/msm_iommu.c
> @@ -731,12 +731,38 @@ 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);
> +}

[Severity: Low]
Does dereferencing domain->ops in __unmap_iommu() and __map_iommu() break the
build when CONFIG_IOMMU_API is disabled?

In drivers/gpu/drm/msm/Kconfig, DRM_MSM depends on IOMMU_SUPPORT and selects
IOMMU_IO_PGTABLE, neither of which selects CONFIG_IOMMU_API. In
include/linux/iommu.h, struct iommu_domain_ops is only forward-declared when
CONFIG_IOMMU_API is not enabled, so dereferencing domain->ops->unmap_pages and
domain->ops->map_pages fails to compile with an incomplete type error under
SOC_IMX5 or COMPILE_TEST configurations without an IOMMU driver that selects
CONFIG_IOMMU_API.

> +
> +static void
> +__flush_iommu(void *arg)
> +{
> +	struct iommu_domain *domain = arg;
> +	iommu_flush_iotlb_all(domain);
> +}

[Severity: Critical]
As reported by Sashiko on v3 [1], does passing NULL for the gather parameter
in __unmap_iommu() followed by iommu_flush_iotlb_all() in __flush_iommu() skip
hardware IOTLB invalidation completely on platforms using qcom_iommu?

When msm_iommu_unmap() (or the error rollback path in msm_iommu_map() ->
__do_map()) calls __do_unmap(), __unmap_iommu() calls
domain->ops->unmap_pages(domain, iova, pgsize, pgcount, NULL).

On SoCs using drivers/iommu/arm/arm-smmu/qcom_iommu.c (such as MSM8916,
MSM8953, and MSM8976 with Adreno A3xx/A4xx/A5xx and MDP5),
qcom_iommu_init_domain() uses the ARM_32_LPAE_S1 page table format in
drivers/iommu/io-pgtable-arm.c. In __arm_lpae_unmap(), per-page TLB
invalidation via io_pgtable_tlb_add_page() -> qcom_iommu_tlb_add_page()
(which writes ARM_SMMU_CB_S1_TLBIVAL) is skipped when gather is 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 __do_unmap() then finishes by calling __flush_iommu() ->
iommu_flush_iotlb_all(domain), qcom_iommu_flush_iotlb_all() only issues a
TLBSYNC barrier without invalidating the context bank by ASID:

drivers/iommu/arm/arm-smmu/qcom_iommu.c:qcom_iommu_flush_iotlb_all() {
	...
	pm_runtime_get_sync(qcom_domain->iommu->dev);
	qcom_iommu_tlb_sync(pgtable->cookie);
	pm_runtime_put_sync(qcom_domain->iommu->dev);
}

As a result, neither ARM_SMMU_CB_S1_TLBIVAL nor ARM_SMMU_CB_S1_TLBIASID is
written during unmap, leaving stale TLB entries pointing to unmapped and freed
GEM buffer pages accessible to GPU and display DMA.

> +
> +static const struct map_ops iommu_ops = {
> +	__unmap_iommu, __map_iommu, __flush_iommu
> +};
> +
>  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 +770,18 @@ 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, &iommu_ops);
>  }
>  
>  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;
>  
>  	if (iova & BIT_ULL(48))
>  		iova |= GENMASK_ULL(63, 49);
>  
> -	iommu_unmap(iommu->domain, iova, len);
> -
> -	return 0;
> +	return __do_unmap(domain->pgsize_bitmap, iova, len, domain, &iommu_ops);
>  }

[Severity: High]
As reported by Sashiko on v3 [2], can bypassing the core IOMMU API in
msm_iommu_map() and msm_iommu_unmap() cause missing IOTLB synchronization on
map as well as regressions with arm-smmu and generic_pt domains?

First, replacing iommu_map_sgtable() with __do_map() -> __map_iommu() ->
domain->ops->map_pages() never calls iommu_sync_map()
(domain->ops->iotlb_sync_map) after mapping pages. On APQ8064/MSM8960
(drivers/iommu/msm_iommu.c, used with MDP4 and Adreno 320),
msm_iommu_ops.default_domain_ops relies on .iotlb_sync_map =
msm_iommu_sync_map to flush the IOTLB range via __flush_iotlb_range() after
mapping and leaves .flush_iotlb_all as NULL.

Second, on arm-smmu, iommu_unmap() previously used struct iommu_iotlb_gather
to perform VA-targeted leaf invalidations (ARM_SMMU_CB_S1_TLBIVAL) followed by
iommu_iotlb_sync(). Calling domain->ops->unmap_pages() with a NULL gather
followed by iommu_flush_iotlb_all() forces a full ASID invalidation
(ARM_SMMU_CB_S1_TLBIASID) on every unmap, evicting all TLB entries for active
display scanout buffers and GPU mappings sharing the ASID.

Third, in drivers/iommu/iommu.c (iommu_map_nosync() and __iommu_unmap()),
domains converted to the generic page table library (domain->is_iommupt) use
pt->ops->map_range and unmap_range and leave domain->ops->map_pages and
unmap_pages as NULL, while core wrappers also handle iommu_debug_map() /
iommu_debug_unmap_begin() and IOMMU tracepoints.

[1] https://lore.kernel.org/r/20261027230452.130031-5-robin.clark@oss.qualcomm.com
[2] https://lore.kernel.org/r/20261027230452.130031-5-robin.clark@oss.qualcomm.com

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 6/6] drm/msm: Treat !mapped as an error
  2026-10-30 21:32 ` [PATCH v4 6/6] drm/msm: Treat !mapped as an error Rob Clark
@ 2026-10-30 21:54   ` sashiko-bot
  0 siblings, 0 replies; 11+ messages in thread
From: sashiko-bot @ 2026-10-30 21:54 UTC (permalink / raw)
  To: Rob Clark, dri-devel

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/msm: __do_map() still returns 0 when sg_table maps nothing or short len
--

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

drm/msm: Treat !mapped as an error

This commit treats a zero-byte mapping result in __do_map() as an error so
that the caller can mark the VM as unusable rather than silently mapping
nothing.

> diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
> index c2950d32c5075..255972323f9a9 100644
> --- a/drivers/gpu/drm/msm/msm_iommu.c
> +++ b/drivers/gpu/drm/msm/msm_iommu.c
> @@ -186,6 +186,10 @@ __do_map(unsigned long pgsize_bitmap, u64 iova, struct sg_table *sgt, size_t off
>  			size -= mapped;
>  			len  -= mapped;
>  
> +			/* If nothing was mapped, consider it an ENOMEM. */
> +			if (!ret && !mapped)
> +				ret = -ENOMEM;
> +
>  			if (ret) {
>  				__do_unmap(pgsize_bitmap, iova, addr - iova, arg, ops);
>  				return -EINVAL;

[Severity: Medium]
As reported by Sashiko on v3 [1], can __do_map() in
drivers/gpu/drm/msm/msm_iommu.c still silently return 0 without mapping the
requested range when sg_table entries have zero or short DMA lengths?

The new check is inside the inner while (size) loop, where size is already
non-zero (and where ops->map() returning ret == 0 with mapped == 0 would
previously have spun in an infinite loop rather than returning early; note
also that setting ret = -ENOMEM is immediately overwritten by returning
-EINVAL).

However, when an sg_table entry has size = sg_dma_len(sg) == 0 (for example,
an unmapped sg_table when CONFIG_NEED_SG_DMA_LENGTH is enabled), the earlier
check in __do_map():

	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;

		if (size <= off) {
			off -= size;
			continue;
		}

evaluates 0 <= off as true for unsigned size_t off and continues past the
while (size) loop without ever reaching the !mapped check.

Similarly, if sgt->nents is 0 or the total sg_dma_len(sg) across sgt is less
than off + len, the outer for_each_sgtable_dma_sg() loop finishes with
len > 0 and __do_map() unconditionally returns 0 at the end of the function:

			if (ret) {
				__do_unmap(pgsize_bitmap, iova, addr - iova, arg, ops);
				return -EINVAL;
			}
		}
	}

	return 0;
}

without calling __do_unmap() on any partially mapped range or returning an
error to mark the VM as unusable.

Should __do_map() check whether len != 0 after the for_each_sgtable_dma_sg()
loop (unmapping [iova, addr) and returning an error if so), and should the
sibling while (len) loop in msm_iommu_pagetable_map_prr() also guard against
!ret && !mapped?

[1] https://lore.kernel.org/r/20261027230452.130031-7-robin.clark@oss.qualcomm.com

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 5/6] drm/msm: Convert map helper to use dma-address
  2026-10-30 21:32 ` [PATCH v4 5/6] drm/msm: Convert map helper to use dma-address Rob Clark
@ 2026-10-30 21:57   ` sashiko-bot
  0 siblings, 0 replies; 11+ messages in thread
From: sashiko-bot @ 2026-10-30 21:57 UTC (permalink / raw)
  To: Rob Clark, dri-devel

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] drm/msm: bogus physical addresses in __do_map() when dma_iommu is active
--

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

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

This commit switches __do_map() in msm_iommu.c from iterating over scatterlist
entries with sg_phys(sg) and sg->length to iterating with
for_each_sgtable_dma_sg(), sg_dma_address(sg), and sg_dma_len(sg) to avoid
issues when CONFIG_DMABUF_DEBUG strips page pointers from scatterlists.

> diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
> index 479e5450a4dcd..c2950d32c5075 100644
> --- a/drivers/gpu/drm/msm/msm_iommu.c
> +++ b/drivers/gpu/drm/msm/msm_iommu.c
> @@ -153,9 +153,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);

[Severity: Critical]
As reported by Dmitry Baryshkov on v3 [1], can passing sg_dma_address(sg) and
sg_dma_len(sg) as physical addresses to ops->map() in __do_map() program bogus
physical addresses into the SMMU page tables when use_dma_iommu() is true on
the DRM DMA device?

In get_pages() in drivers/gpu/drm/msm/msm_gem.c, scatterlists are mapped
against drm_dev_dma_dev(obj->dev):

    ret = dma_map_sgtable(drm_dev_dma_dev(obj->dev),
                          msm_obj->sgt,
                          DMA_BIDIRECTIONAL,
                          map_attrs);

On platforms where the iommus property is placed directly on obj->dev->dev
(such as MDP/DPU display controller nodes checked in msm_kms_init_vm(), or
when separate_gpu_kms is used) and the IOMMU driver does not return
IOMMU_DOMAIN_IDENTITY from .def_domain_type, the IOMMU core allocates an
IOMMU_DOMAIN_DMA default domain during bus probe and iommu_setup_dma_ops()
sets use_dma_iommu(dev) to true:

    dma_iommu = iommu_is_dma_domain(domain);
    dev_assign_dma_iommu(dev, dma_iommu);

This happens, for example, with qcom_iommu_ops in
drivers/iommu/arm/arm-smmu/qcom_iommu.c (used on MSM8916 / db410c) which does
not implement .def_domain_type, and with qcom_smmu_client_of_match in
drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c which does not match qcom,mdp5
display-controller nodes.

When msm_iommu_new() later attaches its own unmanaged domain via
iommu_attach_device(iommu->domain, dev), group->default_domain remains the DMA
domain and use_dma_iommu(dev) stays true. As a result, dma_go_direct() returns
false and dma_map_sgtable() calls iommu_dma_map_sg(), which allocates a
virtual IOVA from the inactive default_domain, coalesces segments into
sg_dma_len(sg), and stores that IOVA in sg_dma_address(sg).

When __do_map() then passes phys = sg_dma_address(sg) and size =
sg_dma_len(sg) to ops->map() (via msm_iommu_pagetable_map() or
msm_iommu_map()), wouldn't it program virtual IOVAs from the unused default
domain into the active GPU and display page tables as if they were CPU
physical addresses?

[1] https://lore.kernel.org/r/wxfx4nhwdx6dud4ahy3rfz3btfto4t2zor2pjbotlqpglvwolx@jhnm65bzecnq

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2026-10-30 21:57 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-30 21:32 [PATCH v4 0/6] drm/msm: DMABUF_DEBUG fixes Rob Clark
2026-10-30 21:32 ` [PATCH v4 1/6] drm/msm: Cleanup if pages_to_sg() fails Rob Clark
2026-10-30 21:32 ` [PATCH v4 2/6] drm/msm/gem: dma_map/unmap_sgtable() Rob Clark
2026-10-30 21:46   ` sashiko-bot
2026-10-30 21:32 ` [PATCH v4 3/6] drm/msm: Extract out map/unmap helpers Rob Clark
2026-10-30 21:32 ` [PATCH v4 4/6] drm/msm: Convert iommu map/unmap to helpers Rob Clark
2026-10-30 21:49   ` sashiko-bot
2026-10-30 21:32 ` [PATCH v4 5/6] drm/msm: Convert map helper to use dma-address Rob Clark
2026-10-30 21:57   ` sashiko-bot
2026-10-30 21:32 ` [PATCH v4 6/6] drm/msm: Treat !mapped as an error Rob Clark
2026-10-30 21:54   ` sashiko-bot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.