[PATCH v3 0/6] drm/msm: DMABUF_DEBUG fixes

[PATCH v3 0/6] 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.

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   |  93 +++++++-------
 drivers/gpu/drm/msm/msm_iommu.c | 210 +++++++++++++++++++++-----------
 2 files changed, 184 insertions(+), 119 deletions(-)

-- 
2.56.0

[PATCH v3 1/6] 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 v3 2/6] 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.

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

diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 181b9ad32672..c97395e4ea2b 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,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);
@@ -253,12 +235,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

Re: [PATCH v3 2/6] 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 pointer dereference in get_pages() on dma_map_sgtable() error
- [High] drm/msm/gem: dma_map_sgtable() maps via iommu-dma when ddev->dev has iommus
--

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

drm/msm/gem: dma_map/unmap_sgtable()

This commit replaces the cache sync helpers in msm_gem with unconditional
dma_map_sgtable() and dma_unmap_sgtable() calls in get_pages() and
put_pages() to populate scatterlist dma addresses and lengths. It also adds
sgtable cleanup to the error path in get_pages().

> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index 181b9ad326729..c97395e4ea2be 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);

[Severity: High]
Does calling dma_map_sgtable() on drm_dev_dma_dev(obj->dev) always take the
dma-direct path when dma_map_ops is null?

In kernel/dma/mapping.c:dma_go_direct(), use_dma_iommu(dev) is checked before
testing whether ops is null:

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

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

On platforms where the iommus property in device tree is attached directly to
the mdp device (obj->dev->dev) rather than the parent mdss node (as handled
in drivers/gpu/drm/msm/msm_kms.c:msm_kms_init_vm() when checking
device_iommu_mapped(mdp_dev)), such as qcom_iommu platforms (msm8916,
msm8939, msm8953, etc.) and qcom,mdp5 arm-smmu-qcom platforms (msm8996,
sdm630, sdm660) that are not matched in qcom_smmu_client_of_match[], the
iommu core allocates a default dma domain and sets DEV_FLAG_DMA_IOMMU on
obj->dev->dev.

When msm_iommu_new() later attaches an unmanaged domain via
iommu_attach_device(), DEV_FLAG_DMA_IOMMU and group->default_domain remain on
the device. As a result, get_pages() -> dma_map_sgtable() ->
__dma_map_sg_attrs() -> iommu_dma_map_sg() maps every buffer into the unused
default_domain and populates sg_dma_address(sg) with default_domain dma
addresses instead of physical addresses.

When paired with the subsequent change in this series ("drm/msm: Convert map
helper to use dma-address") where __do_map() passes sg_dma_address(sg) as the
physical address to pgtable->map(), won't this program the gpu and display
page tables with default_domain dma addresses instead of cpu physical
addresses on those platforms?

> +		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,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() in the get_pages() error path cause a
null pointer dereference and leak msm_obj->sgt when dma_map_sgtable() fails?

At the start of drivers/gpu/drm/msm/msm_gem.c:get_pages(), imported objects
are rejected:

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

so obj->import_attach is always null here, 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 pointer on a non-imported object:

drivers/gpu/drm/drm_prime.c:drm_prime_gem_destroy() {
	struct dma_buf_attachment *attach;
	struct dma_buf *dma_buf;

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

Since attach is null, dma_buf_unmap_attachment_unlocked() triggers a warning
and attach->dmabuf dereferences a null pointer while leaving msm_obj->sgt
unfreed. Should this error path free msm_obj->sgt using sg_free_table() and
kfree() as done in put_pages()?

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

[PATCH v3 3/6] 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 | 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

[PATCH v3 4/6] 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 | 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

Re: [PATCH v3 4/6] drm/msm: Convert iommu map/unmap to helpers

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 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()
--

commit efc83727eef15680f9e495c695cd848cb428e1a7
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() in
drivers/gpu/drm/msm/msm_iommu.c to use the __do_map() and __do_unmap()
helpers via domain->ops->map_pages() and domain->ops->unmap_pages().

> 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);
> +}
> +
> +static void
> +__flush_iommu(void *arg)
> +{
> +	struct iommu_domain *domain = arg;
> +	iommu_flush_iotlb_all(domain);
> +}

[Severity: Critical]
Does passing NULL for the gather parameter in __unmap_iommu() paired with
iommu_flush_iotlb_all() in __flush_iommu() leave stale leaf IOTLB entries
on platforms using qcom_iommu?

Previously, msm_iommu_unmap() called iommu_unmap(), which passed a valid
struct iommu_iotlb_gather to domain->ops->unmap_pages() and then called
iommu_iotlb_sync().

With gather set to NULL in __unmap_iommu(), __arm_lpae_unmap() in
drivers/iommu/io-pgtable-arm.c skips per-page TLB invalidation:

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);
    ...
}

On SoCs using drivers/iommu/arm/arm-smmu/qcom_iommu.c, skipping
io_pgtable_tlb_add_page() means qcom_iommu_tlb_add_page() is never called
to write ARM_SMMU_CB_S1_TLBIVAL. When __do_unmap() then calls
__flush_iommu() -> iommu_flush_iotlb_all(), qcom_iommu_flush_iotlb_all()
only issues a TLBSYNC barrier without invalidating the context bank by
ASID (qcom_iommu_tlb_inv_context()):

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);
}

As a result, unmapping leaf pages in msm_iommu_unmap() (or during error
rollback in __do_map()) issues no hardware TLB invalidation on qcom_iommu,
allowing the GPU or display engine to continue accessing unmapped and freed
GEM pages via stale IOTLB entries.

> +
> +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]
What happens on platforms that require .iotlb_sync_map or use generic_pt
when msm_iommu_map() and msm_iommu_unmap() bypass the core IOMMU API and
call domain->ops->map_pages() and domain->ops->unmap_pages() directly?

1. Previously, msm_iommu_map() called iommu_map_sgtable() ->
   iommu_map_sg(), which calls iommu_sync_map() after mapping the
   scatterlist:

   drivers/iommu/iommu.c:iommu_map_sg() {
       ...
	ret = iommu_sync_map(domain, iova, mapped);
	if (ret)
		goto out_err;
       ...
   }

   __do_map() only calls __map_iommu() -> domain->ops->map_pages() and
   never invokes domain->ops->iotlb_sync_map. On APQ8064/MSM8960
   (drivers/iommu/msm_iommu.c), the IOMMU driver relies on
   .iotlb_sync_map = msm_iommu_sync_map to flush the IOTLB for newly mapped
   ranges via __flush_iotlb_range(), which is now skipped.

2. In drivers/iommu/iommu.c:iommu_map_nosync() and __iommu_unmap(),
   domains converted to generic_pt (iommupt) use pt->ops->map_range and
   pt->ops->unmap_range and leave domain->ops->map_pages and
   domain->ops->unmap_pages as NULL:

   drivers/iommu/iommu.c:iommu_map_nosync() {
       ...
	if (pt)
		ret = pt->ops->map_range(pt, iova, paddr, size, prot, gfp,
					 &mapped);
	else
		ret = __iommu_map_domain_pgtbl(domain, iova, paddr, size, prot,
					       gfp, &mapped);
       ...
   }

   Directly dereferencing domain->ops->map_pages and
   domain->ops->unmap_pages in __map_iommu() and __unmap_iommu() will
   dereference NULL on any IOMMU driver using generic_pt.

3. On arm-smmu, replacing iommu_unmap() (which uses struct
   iommu_iotlb_gather for range-targeted ARM_SMMU_CB_S1_TLBIVAL
   invalidations followed by iommu_iotlb_sync()) with
   domain->ops->unmap_pages(..., NULL) and 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 shared
   GPU mappings.

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

[PATCH v3 5/6] 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 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

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

[sashiko-bot]

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 b6b3547f9503a1fd43535b38edfaee1a6356607a
Author: Rob Clark <robin.clark@oss.qualcomm.com>

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

This commit switches __do_map() in drivers/gpu/drm/msm/msm_iommu.c from
iterating over scatterlist page entries with for_each_sgtable_sg() and
sg_phys() to iterating over DMA-mapped entries with for_each_sgtable_dma_sg(),
sg_dma_len(), and sg_dma_address().

> 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]
Could passing sg_dma_address(sg) and sg_dma_len(sg) as physical addresses into
ops->map() program virtual IOVAs from an inactive default DMA domain into the
GPU and display SMMU page tables on platforms where use_dma_iommu() is true on
drm_dev_dma_dev(obj->dev)?

In drivers/gpu/drm/msm/msm_gem.c:get_pages() (and via dma_buf_map_attachment()
for PRIME imports), the scatterlist is DMA-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 the display
controller node (mdp_dev, which is obj->dev->dev) rather than a parent mdss
node, as handled in 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;

and the IOMMU driver does not return IOMMU_DOMAIN_IDENTITY from
.def_domain_type, the IOMMU core allocates a default DMA domain during probe
and drivers/iommu/dma-iommu.c:iommu_setup_dma_ops() sets dev->dma_iommu:

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

For example, drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c:
qcom_smmu_client_of_match[] does not list "qcom,mdp5" (affecting MDP5
platforms such as SDM630, SDM660, and MSM8996), and qcom_iommu_ops in
drivers/iommu/arm/arm-smmu/qcom_iommu.c does not implement .def_domain_type at
all.

When msm_iommu_new() later attaches an unmanaged domain via
iommu_attach_device(), group->default_domain remains the DMA domain and
use_dma_iommu(dev) stays true. As a result, kernel/dma/mapping.c:
dma_go_direct() returns false even when get_dma_ops(dev) is NULL:

    if (use_dma_iommu(dev))
        return false;

    if (likely(!ops))
        return true;

This causes dma_map_sgtable() to call iommu_dma_map_sg(), which allocates
virtual IOVAs from the inactive group->default_domain, coalesces segments, and
stores those IOVAs and coalesced lengths into sg_dma_address(sg) and
sg_dma_len(sg).

When __do_map() subsequently passes phys = sg_dma_address(sg) and
size = sg_dma_len(sg) to io_pgtable_ops->map_pages() or
iommu_domain->ops->map_pages() (which expect CPU physical addresses), won't
this map bogus physical addresses into the active GPU and display page tables?

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

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

[Dmitry Baryshkov]

On Tue, Oct 27, 2026 at 04:04:48PM -0700, Rob Clark wrote:
> 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(-)

This commit seems to break GPU on db410c (which uses qcom_iommu).

[  104.954564] [drm:a3xx_idle] *ERROR* 03000600: timeout waiting for GPU
to idle!
[  104.954932] msm_mdp 1a01000.display-controller: [drm:adreno_load_gpu]
*ERROR* gpu hw init failed: -22
[  105.958559] [drm:a3xx_idle] *ERROR* 03000600: timeout waiting for GPU
to idle!
[  109.082571] [drm:a3xx_idle] *ERROR* 03000600: timeout waiting for GPU
to idle!
[  109.082670] msm_mdp 1a01000.display-controller: [drm:adreno_load_gpu]
*ERROR* gpu hw init failed: -22
[  110.086572] [drm:a3xx_idle] *ERROR* 03000600: timeout waiting for GPU
to idle!
[  111.098576] [drm:a3xx_idle] *ERROR* 03000600: timeout waiting for GPU
to idle!
[  111.098674] msm_mdp 1a01000.display-controller: [drm:adreno_load_gpu]
*ERROR* gpu hw init failed: -22
c[  112.102576] [drm:a3xx_idle] *ERROR* 03000600: timeout waiting for
GPU to idle!
> 
> 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
> 

-- 
With best wishes
Dmitry

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

[Rob Clark]

On Tue, Oct 27, 2026 at 6:14 PM Dmitry Baryshkov
<dmitry.baryshkov@oss.qualcomm.com> wrote:
>
> On Tue, Oct 27, 2026 at 04:04:48PM -0700, Rob Clark wrote:
> > 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(-)
>
> This commit seems to break GPU on db410c (which uses qcom_iommu).
>
> [  104.954564] [drm:a3xx_idle] *ERROR* 03000600: timeout waiting for GPU
> to idle!
> [  104.954932] msm_mdp 1a01000.display-controller: [drm:adreno_load_gpu]
> *ERROR* gpu hw init failed: -22
> [  105.958559] [drm:a3xx_idle] *ERROR* 03000600: timeout waiting for GPU
> to idle!
> [  109.082571] [drm:a3xx_idle] *ERROR* 03000600: timeout waiting for GPU
> to idle!
> [  109.082670] msm_mdp 1a01000.display-controller: [drm:adreno_load_gpu]
> *ERROR* gpu hw init failed: -22
> [  110.086572] [drm:a3xx_idle] *ERROR* 03000600: timeout waiting for GPU
> to idle!
> [  111.098576] [drm:a3xx_idle] *ERROR* 03000600: timeout waiting for GPU
> to idle!
> [  111.098674] msm_mdp 1a01000.display-controller: [drm:adreno_load_gpu]
> *ERROR* gpu hw init failed: -22
> c[  112.102576] [drm:a3xx_idle] *ERROR* 03000600: timeout waiting for
> GPU to idle!

If that came down to this patch (and not the earlier msm_iommu.c
bits), then I'd look to the hint in the cover letter:

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.


BR,
-R

> >
> > 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
> >
>
> --
> With best wishes
> Dmitry

[PATCH v3 6/6] drm/msm: Treat !mapped as an error

[Rob Clark]

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

Re: [PATCH v3 6/6] drm/msm: Treat !mapped as an error

[sashiko-bot]

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 363087af17cd51ea419c705b08215e2c785556f3
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Treat !mapped as an error

This commit treats zero mapped bytes after an ops->map() call in __do_map()
as -ENOMEM so that mapping nothing is handled as an error rather than
silently ignored.

> 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;

[Severity: Medium]
Can __do_map() in drivers/gpu/drm/msm/msm_iommu.c still silently return 0
without mapping anything (or after mapping fewer than len bytes) when called
via vm_map_op() -> msm_iommu_pagetable_map() or msm_iommu_map()?

The new !mapped check is placed inside the inner while (size) loop, which
only executes when size = sg_dma_len(sg) > 0 (and where ret == 0 with
mapped == 0 would previously have caused an infinite loop rather than
silently returning).

However, if sg_dma_len(sg) == 0 (for example, when an sg_table is not
DMA-mapped, or analogous to the sg->length == 0 case under
CONFIG_DMABUF_DEBUG before commit b6b3547f9503 ("drm/msm: Convert map helper
to use dma-address")), the outer loop in __do_map() skips the inner while
(size) loop altogether because 0 <= off is always true for unsigned size_t:

drivers/gpu/drm/msm/msm_iommu.c:__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;
		}
		...
		while (size) {
			...
		}
	}

	return 0;
}

Similarly, if sgt->nents == 0 or the total DMA length across sgt is smaller
than off + len (which msm_iommu_map() checked via return (ret == len) ? 0 :
-EINVAL before commit efc83727eef1 ("drm/msm: Convert iommu map/unmap to
helpers")), for_each_sgtable_dma_sg() finishes with len > 0 and __do_map()
still returns 0 without unmapping addr - iova via __do_unmap() or marking
the VM unusable.

Should __do_map() also check if len != 0 after the for_each_sgtable_dma_sg()
loop and unmap any partial range before returning an error?

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