Re: [PATCH v1 2/2] drm/msm: map page-less imported sg_tables from their DMA addresses

["Christian König" <christian.koenig@amd.com>]

On 9/28/26 07:38, Jianfeng Liu wrote:
> With CONFIG_DMABUF_DEBUG=y, dma_buf_map_attachment() hands importers a
> copy of the attachment sg_table with the struct page pointers stripped
> and sg->length zeroed; only sg_dma_address()/sg_dma_len() are carried
> over.  msm consumes sg->length and sg_phys() in both of its map paths:
> 
>  - msm_iommu_pagetable_map() (userspace managed, per-process GPU
>    pagetables) walks the sg_table with sg->length and sg_phys()
> 
>  - msm_iommu_map() (kernel managed mappings: display, and TTBR1 for the
>    GPU), via iommu_map_sgtable(), which consumes sg->length and
>    sg_phys() as well
> 
> With a page-stripped sg_table both paths silently map nothing and
> return success.  Userspace then observes arm-smmu translation faults
> once the GPU first touches the mapping, e.g. during hardware video
> decode:
> 
>   gpu fault: ttbr0=000000088a889000 iova=000000010741c000 dir=READ
>   type=TRANSLATION source=UCHE
> 
> and __arm_lpae_unmap() WARNs for the never-mapped ranges when the GEM
> handles are closed (a WARN storm of ~470 traces within a minute of
> video playback on my x1e78100 laptop).
> 
> For sg entries that still carry a struct page (native objects, and
> imports without the DMABUF_DEBUG wrapper) keep using sg_phys(), so
> native objects which msm never dma-maps itself (non-MSM_BO_WC) are
> unaffected.  For page-less entries, recover the physical address from
> the DMA address instead: dmabuf attachments are dma-mapped against
> the msm drm device, so the dma_addr -> phys lookup can be done with
> iommu_iova_to_phys() in that device's DMA-API domain, cached per VM
> in struct msm_mmu::dma_domain at msm_gem_vm_create() time.  If the
> drm device is direct mapped the DMA address already is a physical
> address and the lookup degenerates to the identity.

That is not in any way better than illegally using struct page. The point is we need to get away from using phys_addr at all here.

Regards,
Christian.

> 
> Applied on top of "drm/msm/gem: Drop use of pages for imported
> dma-bufs" [1], which removes the remaining struct page consumers for
> imported buffers.  With both, hardware video decode works with
> DMABUF_DEBUG=y, tested with clapper and chromium on x1e78100
> (Snapdragon X1E78100): zero arm-smmu faults, zero io-pgtable WARNs,
> correct frames.  Without this patch, the same system logs a WARN
> trace per unmap and falls back to a copy path for video playback.
> 
> [1] <20260926183051.25754-1-robin.clark@oss.qualcomm.com>
> 
> Suggested-by: Rob Clark <robin.clark@oss.qualcomm.com>
> Cc: Rob Clark <robin.clark@oss.qualcomm.com>
> Cc: Dmitry Baryshkov <lumag@kernel.org>
> Cc: Christian König <christian.koenig@amd.com>
> 
> Signed-off-by: Jianfeng Liu <liujianfeng1994@gmail.com>
> ---
> 
>  drivers/gpu/drm/msm/msm_gem_vma.c |  9 ++++
>  drivers/gpu/drm/msm/msm_iommu.c   | 90 ++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/msm/msm_mmu.h     | 13 +++++
>  3 files changed, 110 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c
> index f687a629629d3..322b96e0e07ec 100644
> --- a/drivers/gpu/drm/msm/msm_gem_vma.c
> +++ b/drivers/gpu/drm/msm/msm_gem_vma.c
> @@ -840,6 +840,15 @@ msm_gem_vm_create(struct drm_device *drm, struct msm_mmu *mmu, const char *name,
>  		goto err_free_vm;
>  	}
>  
> +	/*
> +	 * dma-buf imports attach against the msm drm device, and their
> +	 * sg_dma_address() lives in that device's DMA-API domain.  Keep it
> +	 * so the map paths can translate page-less sg_table entries (the
> +	 * DMABUF_DEBUG wrapper) back to physical addresses.
> +	 */
> +	if (device_iommu_mapped(drm->dev))
> +		mmu->dma_domain = iommu_get_dma_domain(drm->dev);
> +
>  	if (!managed) {
>  		struct drm_sched_init_args args = {
>  			.ops = &msm_vm_bind_ops,
> diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
> index da6782fca6bd2..8407a37f9efee 100644
> --- a/drivers/gpu/drm/msm/msm_iommu.c
> +++ b/drivers/gpu/drm/msm/msm_iommu.c
> @@ -140,6 +140,24 @@ static int msm_iommu_pagetable_unmap(struct msm_mmu *mmu, u64 iova,
>  	return ret;
>  }
>  
> +/**
> + * msm_mmu_dma_to_phys() - recover the physical address of a dma address
> + *
> + * dma-buf attachments are dma-mapped against the msm drm device, so the
> + * DMA domain of that device (msm_mmu::dma_domain) holds the mapping.
> + * For a direct-mapped drm device the DMA address already is a physical
> + * address.
> + */
> +static phys_addr_t msm_mmu_dma_to_phys(struct msm_mmu *mmu, dma_addr_t dma_addr)
> +{
> +	struct iommu_domain *dma_domain = mmu->dma_domain;
> +
> +	if (!dma_domain)
> +		return (phys_addr_t)dma_addr;
> +
> +	return iommu_iova_to_phys(dma_domain, dma_addr);
> +}
> +
>  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);
> @@ -184,8 +202,35 @@ static int msm_iommu_pagetable_map(struct msm_mmu *mmu, u64 iova,
>  		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);
> +		size_t size;
> +		phys_addr_t phys;
> +
> +		if (sg_page(sg)) {
> +			/* CPU-view entry: native objects, and imported
> +			 * sg_tables that still carry struct page
> +			 */
> +			size = sg->length;
> +			phys = sg_phys(sg);
> +		} else {
> +			/*
> +			 * Page-less entry, e.g. the sg_table wrapper
> +			 * that dma_buf_map_attachment() hands out when
> +			 * CONFIG_DMABUF_DEBUG=y (page pointers stripped,
> +			 * sg->length zeroed, only the DMA fields carried
> +			 * over).  Recover the physical address by
> +			 * translating the DMA address through the drm
> +			 * device's DMA-API domain.
> +			 */
> +			size = sg_dma_len(sg);
> +			phys = msm_mmu_dma_to_phys(mmu, sg_dma_address(sg));
> +
> +			if (!size || !phys) {
> +				dev_err(mmu->dev,
> +					"cannot map page-less sg entry: dma=%pad len=%zu\n",
> +					&sg_dma_address(sg), size);
> +				return -EINVAL;
> +			}
> +		}
>  
>  		if (!len)
>  			break;
> @@ -697,6 +742,47 @@ static int msm_iommu_map(struct msm_mmu *mmu, uint64_t iova,
>  	if (iova & BIT_ULL(48))
>  		iova |= GENMASK_ULL(63, 49);
>  
> +	/*
> +	 * With CONFIG_DMABUF_DEBUG=y, imported sg_tables carry no struct
> +	 * page and sg->length is zeroed; iommu_map_sgtable() would consume
> +	 * zero length and silently map nothing.  Map from the (translated)
> +	 * DMA addresses instead.
> +	 */
> +	if (!sg_page(sgt->sgl)) {
> +		struct scatterlist *sg;
> +		size_t mapped = 0;
> +		unsigned int i;
> +
> +		for_each_sgtable_dma_sg(sgt, sg, i) {
> +			phys_addr_t phys =
> +				msm_mmu_dma_to_phys(mmu, sg_dma_address(sg));
> +			size_t size = sg_dma_len(sg);
> +
> +			if (!phys || !size) {
> +				ret = -EINVAL;
> +				goto err_unmap;
> +			}
> +
> +			ret = iommu_map(iommu->domain, iova + mapped, phys,
> +					size, prot, GFP_KERNEL);
> +			if (ret)
> +				goto err_unmap;
> +
> +			mapped += size;
> +		}
> +
> +		if (mapped != len) {
> +			ret = -EINVAL;
> +			goto err_unmap;
> +		}
> +
> +		return 0;
> +
> +err_unmap:
> +		iommu_unmap(iommu->domain, iova, mapped);
> +		return ret;
> +	}
> +
>  	ret = iommu_map_sgtable(iommu->domain, iova, sgt, prot);
>  	if (ret < 0)
>  		return ret;
> diff --git a/drivers/gpu/drm/msm/msm_mmu.h b/drivers/gpu/drm/msm/msm_mmu.h
> index 8915662fbd4d0..116daf6ce47cb 100644
> --- a/drivers/gpu/drm/msm/msm_mmu.h
> +++ b/drivers/gpu/drm/msm/msm_mmu.h
> @@ -64,6 +64,19 @@ struct msm_mmu {
>  	 * msm_gem_vm::mmu_lock.
>  	 */
>  	struct msm_mmu_prealloc *prealloc;
> +
> +	/**
> +	 * @dma_domain: DMA-API domain of the msm drm device
> +	 *
> +	 * dma-buf attachments are dma-mapped against the msm drm device,
> +	 * so this domain holds the mapping dma_addr -> phys for imported
> +	 * buffers.  Used to recover the physical address of sg_table
> +	 * entries which carry no struct page (e.g. the page-stripped
> +	 * sg_table wrapper that dma_buf_map_attachment() hands out when
> +	 * CONFIG_DMABUF_DEBUG=y).  NULL if the drm device is direct
> +	 * mapped, in which case DMA addresses are physical addresses.
> +	 */
> +	struct iommu_domain *dma_domain;
>  };
>  
>  static inline void msm_mmu_init(struct msm_mmu *mmu, struct device *dev,