drm/msm: DMABUF_DEBUG fixes
Rob Clark
maintainercover drm/msm: DMABUF_DEBUG fixes
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.
Rob Clark (5):
drm/msm: Cleanup if pages_to_sg() fails
drm/msm/gem: dma_map/unmap_sgtable()
drm/msm: Extract out map/unmap helpers
drm/msm: Convert iommu map/unmap to helpers
drm/msm: Convert map helper to use dma-address
drivers/gpu/drm/msm/msm_gem.c | 57 ++++++---- drivers/gpu/drm/msm/msm_iommu.c | 182 ++++++++++++++++++++------------ 2 files changed, 153 insertions(+), 86 deletions(-)
Signature · 1 line
2.55.0
Rob Clark
maintainerPATCH 1/5 drm/msm: Cleanup if pages_to_sg() fails
We never really handled this. But in practice mapping to smmu would have failed without an sgt, which would have been pretty obvious.
--- drivers/gpu/drm/msm/msm_gem.c | 41 ++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 18 deletions(-)
drivers/gpu/drm/msm/msm_gem.c +23 -18
@@ -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);
Signature · 1 line
2.55.0
Rob Clark
maintainerPATCH 2/5 drm/msm/gem: dma_map/unmap_sgtable()
In reply to Rob ClarkThis 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.
--- drivers/gpu/drm/msm/msm_gem.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
drivers/gpu/drm/msm/msm_gem.c +16 -0
@@ -219,6 +219,15 @@ static struct page **get_pages(struct drm_gem_object *obj) goto error; } + ret =dma_map_sgtable (drm_dev_dma_dev (obj->dev), + msm_obj->sgt, +DMA_BIDIRECTIONAL , +DMA_ATTR_SKIP_CPU_SYNC ); +if (ret) { +DRM_DEV_ERROR (dev->dev,"could not map sgtable: %d\n" , ret); + goto error; + } +update_device_mem (dev->dev_private, obj->size);/* For non-cached buffers, ensure the new pages are clean
@@ -233,6 +242,8 @@ static struct page **get_pages(struct drm_gem_object *obj)return msm_obj->pages; error: +if (!IS_ERR (msm_obj->sgt)) +drm_prime_gem_destroy (obj, msm_obj->sgt); msm_obj->sgt =NULL ;drm_gem_put_pages (obj, msm_obj->pages, false, false);
@@ -260,6 +271,11 @@ static void put_pages(struct drm_gem_object *obj)if (msm_obj->flags &MSM_BO_WC )sync_for_cpu (msm_obj); +dma_unmap_sgtable (drm_dev_dma_dev (obj->dev), + msm_obj->sgt, +DMA_BIDIRECTIONAL , +DMA_ATTR_SKIP_CPU_SYNC ); +sg_free_table (msm_obj->sgt);kfree (msm_obj->sgt); msm_obj->sgt =NULL ;
Signature · 1 line
2.55.0
Rob Clark
maintainerPATCH 3/5 drm/msm: Extract out map/unmap helpers
In reply to Rob ClarkWe're going to re-use these in the next commit. No functional changes, just some ugly callbacks.
--- drivers/gpu/drm/msm/msm_iommu.c | 150 ++++++++++++++++++++------------ 1 file changed, 93 insertions(+), 57 deletions(-)
drivers/gpu/drm/msm/msm_iommu.c +93 -57
@@ -52,9 +52,8 @@ static struct msm_iommu_pagetable *to_pagetable(struct msm_mmu *mmu) }/* based on iommu_pgsize() in iommu.c: */ -static size_t calc_pgsize (struct msm_iommu_pagetable *pagetable, -unsigned long iova,phys_addr_t paddr, -size_t size,size_t *count) +static size_t calc_pgsize (unsigned long pgsize_bitmap,unsigned long iova, +phys_addr_t paddr,size_t size,size_t *count) {unsigned int pgsize_idx, pgsize_idx_next;unsigned long pgsizes;
@@ -62,7 +61,7 @@ static size_t calc_pgsize(struct msm_iommu_pagetable *pagetable,unsigned long addr_merge = paddr | iova;/* Page sizes supported by the hardware and small enough for @size */ - pgsizes = pagetable->pgsize_bitmap &GENMASK (__fls (size),0 ); + pgsizes = pgsize_bitmap &GENMASK (__fls (size),0 );/* Constrain the page sizes further based on the maximum alignment */ if (likely (addr_merge))
@@ -78,7 +77,7 @@ static size_t calc_pgsize(struct msm_iommu_pagetable *pagetable,return pgsize;/* Find the next biggest support page size, if it exists */ - pgsizes = pagetable->pgsize_bitmap & ~GENMASK (pgsize_idx,0 ); + pgsizes = pgsize_bitmap & ~GENMASK (pgsize_idx,0 );if (!pgsizes) goto out_set_count;
@@ -107,24 +106,27 @@ static size_t calc_pgsize(struct msm_iommu_pagetable *pagetable,return pgsize; } -static int msm_iommu_pagetable_unmap (struct msm_mmu *mmu,u64 iova, -size_t size) +typedef size_t (*unmap_fn )(void *arg,u64 iova,size_t pgsize,size_t count); +typedef int (*map_fn )(void *arg,phys_addr_t paddr,u64 iova,size_t pgsize, +size_t pgcount,int prot,size_t *mapped); + +static inline int +__do_unmap (unsigned long pgsize_bitmap,u64 iova,size_t size, +void *arg,unmap_fn unmap) { -struct msm_iommu_pagetable *pagetable =to_pagetable (mmu); -struct io_pgtable_ops *ops = pagetable->pgtbl_ops;int ret =0 ;while (size) {size_t pgsize, count;ssize_t unmapped; - pgsize =calc_pgsize (pagetable, iova, iova, size, &count); + pgsize =calc_pgsize (pgsize_bitmap, iova, iova, size, &count); - unmapped = ops->unmap_pages (ops, iova, pgsize, count,NULL ); + unmapped =unmap (arg, iova, pgsize, count);if (unmapped <=0 ) { ret = -EINVAL ;/* -* Continue attempting to unamp the remained of the +* Continue attempting to unmap the remained of the * range, so we don't end up with some dangling * mapped pages */
@@ -135,55 +137,18 @@ static int msm_iommu_pagetable_unmap(struct msm_mmu *mmu, u64 iova, size -= unmapped; } -iommu_flush_iotlb_all (to_msm_iommu (pagetable->parent)->domain); -return ret; } -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) +static inline int +__do_map (unsigned long pgsize_bitmap,u64 iova,struct sg_table *sgt,size_t off, +size_t len,int prot,void *arg,map_fn map,unmap_fn unmap) { -struct 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 +169,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 =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 +182,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, unmap);return -EINVAL ; } }
@@ -227,6 +191,78 @@ 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 io_pgtable_ops *ops = arg; +return ops->unmap_pages (ops, iova, pgsize, pgcount,NULL ); +} + +static int +__map_pgtable (void *arg,phys_addr_t paddr,u64 iova,size_t pgsize, +size_t pgcount,int prot,size_t *mapped) +{ +struct io_pgtable_ops *ops = arg; +return ops->map_pages (ops, iova, paddr, pgsize, pgcount, prot,GFP_KERNEL , mapped); +} + +static int msm_iommu_pagetable_unmap (struct msm_mmu *mmu,u64 iova,size_t size) +{ +struct msm_iommu_pagetable *pagetable =to_pagetable (mmu); +struct io_pgtable_ops *ops = pagetable->pgtbl_ops; +int ret =0 ; + + ret =__do_unmap (pagetable->pgsize_bitmap, iova, size, ops, __unmap_pgtable); + +iommu_flush_iotlb_all (to_msm_iommu (pagetable->parent)->domain); + +return ret; +} + +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); +struct io_pgtable_ops *ops = pagetable->pgtbl_ops; + +if (!sgt) +return msm_iommu_pagetable_map_prr (mmu, iova, len, prot); + +return __do_map (pagetable->pgsize_bitmap, iova, sgt, off, len, prot, + ops, __map_pgtable, __unmap_pgtable); +} +static void msm_iommu_pagetable_destroy (struct msm_mmu *mmu) {struct msm_iommu_pagetable *pagetable =to_pagetable (mmu);
Signature · 1 line
2.55.0
[email protected]
AI reviewRe: [RFT 3/5] drm/msm: Extract out map/unmap helpers
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] drm/msm: missing IOTLB flush on pagetable map error path --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/msm: missing IOTLB flush on pagetable map error path
--
commit 7d5e2545b852417f316f5873d935ec2b95998bbf
Author: Rob Clark <[email protected]>
drm/msm: Extract out map/unmap helpers
This commit extracts __do_map() and __do_unmap() helpers from msm_iommu_pagetable_map() and msm_iommu_pagetable_unmap() respectively. It aims to reuse these helpers without any functional changes.
Quoted 4 lines
drivers/gpu/drm/msm/msm_iommu.c +0
-0
[ ... ]
Quoted 14 lines
drivers/gpu/drm/msm/msm_iommu.c +2
-3@@ -204,10 +169,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 =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.
drivers/gpu/drm/msm/msm_iommu.c static int msm_iommu_pagetable_map(struct msm_mmu *mmu, u64 iova, · line 187
Quoted 7 lines
drivers/gpu/drm/msm/msm_iommu.c +1
-1@@ -218,7 +182,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, unmap);return -EINVAL ; }
[Severity: High]
Does this error path miss an IOTLB flush?
Before this commit, a failure in the mapping loop would call msm_iommu_pagetable_unmap(), which flushes the IOTLB after unmapping.
Now, the refactored __do_map() calls the new __do_unmap() helper directly. Because __do_unmap() skips the IOTLB flush, the error rollback leaves stale virtual-to-physical address translations for partially mapped pages in the TLB, which can lead to rogue DMA and memory corruption (use-after-free).
[ ... ]
Quoted 6 lines
drivers/gpu/drm/msm/msm_iommu.c +2
-0@@ -227,6 +191,78 @@ 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)
[ ... ]
drivers/gpu/drm/msm/msm_iommu.c static int msm_iommu_pagetable_map(struct msm_mmu *mmu, u64 iova, · line 264
Quoted 13 lines
drivers/gpu/drm/msm/msm_iommu.c +13
-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; + +if (!sgt) +return msm_iommu_pagetable_map_prr (mmu, iova, len, prot); + +return __do_map (pagetable->pgsize_bitmap, iova, sgt, off, len, prot, + ops, __map_pgtable, __unmap_pgtable); +}
[Severity: High]
Can this propagate the error from __do_map() without flushing the IOTLB?
When __do_map() hits an error and returns, msm_iommu_pagetable_map() passes the error up the stack without initiating a TLB flush, cementing the missing flush bug from the __do_map() failure path.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Re: [RFT 3/5] drm/msm: Extract out map/unmap helpers
Quoted 1 line
Does this error path miss an IOTLB flush?
Yes. msm_iommu_pagetable_unmap() called iommu_flush_iotlb_all() after rollback. __do_map() now calls __do_unmap() directly and loses that flush.
An arm64 QEMU/SMMUv3 test:
- mapped one 4 KiB page,
- hit -EEXIST on the second page, and
- verified that rollback removed the first PTE.
No stale translation was observed because the device had not accessed the mapping. This tested the low-level IOMMU sequence, not the MSM driver or Qualcomm hardware.
Restoring iommu_flush_iotlb_all() on this error path preserves the old behavior.
Karl
Re: [RFT 3/5] drm/msm: Extract out map/unmap helpers
Quoted 13 lines · Karl Mehltretter
Quoted 2 lines
Does this error path miss an IOTLB flush?
Yes.
msm_iommu_pagetable_unmap()callediommu_flush_iotlb_all()after rollback.__do_map()now calls__do_unmap()directly and loses that flush.An arm64 QEMU/SMMUv3 test:
- mapped one 4 KiB page,
- hit-EEXISTon the second page, and
- verified that rollback removed the first PTE.
Quoted 6 lines
No stale translation was observed because the device had not accessed the mapping. This tested the low-level IOMMU sequence, not the MSM driver or Qualcomm hardware.
Restoring
iommu_flush_iotlb_all()on this error path preserves the old behavior.
I have a fixup, but haven't sent a v2 yet.. current series is still enough to verify whether there is more work needed on older pre-arm-smmu hw or not
BR,
-R
Rob Clark
maintainerPATCH 4/5 drm/msm: Convert iommu map/unmap to helpers
In reply to Rob ClarkRe-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.
--- drivers/gpu/drm/msm/msm_iommu.c | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-)
drivers/gpu/drm/msm/msm_iommu.c +21 -11
@@ -720,12 +720,27 @@ static void msm_iommu_detach(struct msm_mmu *mmu)iommu_detach_device (iommu->domain, mmu->dev); } +static size_t +__unmap_iommu (void *arg,u64 iova,size_t pgsize,size_t pgcount) +{ +struct iommu_domain *domain = arg; +return domain->ops->unmap_pages (domain, iova, pgsize, pgcount,NULL ); +} + +static int +__map_iommu (void *arg,phys_addr_t paddr,u64 iova,size_t pgsize, +size_t pgcount,int prot,size_t *mapped) +{ +struct iommu_domain *domain = arg; +return domain->ops->map_pages (domain, iova, paddr, pgsize, pgcount, + prot,GFP_KERNEL , mapped); +} +static int msm_iommu_map (struct msm_mmu *mmu,uint64_t iova,struct sg_table *sgt,size_t off,size_t len,int prot) { -struct msm_iommu *iommu =to_msm_iommu (mmu); -ssize_t ret; +struct iommu_domain *domain =to_msm_iommu (mmu)->domain;WARN_ON (off !=0 );
@@ -733,23 +748,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, __map_iommu, __unmap_iommu); }static int msm_iommu_unmap (struct msm_mmu *mmu,uint64_t iova,size_t len) { -struct msm_iommu *iommu =to_msm_iommu (mmu); +struct iommu_domain *domain =to_msm_iommu (mmu)->domain;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, __unmap_iommu); }static void msm_iommu_destroy (struct msm_mmu *mmu)
Signature · 1 line
2.55.0
[email protected]
AI reviewRe: [RFT 4/5] drm/msm: Convert iommu map/unmap to helpers
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Critical] msm: missing TLB flush in unmap sequence causes GPU UAF - [Low] msm: build failure due to…
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] msm: missing TLB flush in unmap sequence causes GPU UAF
- [Low] msm: build failure due to undefined __do_map and __do_unmap
--
commit a6f7885817ad9924dcb4d6924c8061a9096367bc
Author: Rob Clark <[email protected]>
Subject: drm/msm: Convert iommu map/unmap to helpers
This patch converts the msm iommu map and unmap paths to use new helpers. It replaces calls to iommu_map_sgtable() and iommu_unmap() with __do_map() and __do_unmap(), utilizing __map_iommu() and __unmap_iommu() callbacks.
Quoted 4 lines
drivers/gpu/drm/msm/msm_iommu.c +0
-0
[ ... ]
drivers/gpu/drm/msm/msm_iommu.c static void msm_iommu_detach(struct msm_mmu *mmu) · line 692
Quoted 10 lines
drivers/gpu/drm/msm/msm_iommu.c +6
-0@@ -684,12 +684,27 @@ static void msm_iommu_detach(struct msm_mmu *mmu)iommu_detach_device (iommu->domain, mmu->dev); } +static size_t +__unmap_iommu (void *arg,u64 iova,size_t pgsize,size_t pgcount) +{ +struct iommu_domain *domain = arg; +return domain->ops->unmap_pages (domain, iova, pgsize, pgcount,NULL ); +}
[Severity: Critical]
By passing NULL for iotlb_gather, does this bypass the IOMMU driver's TLB gathering and flushing mechanism?
When iommu_unmap() was previously used, it provided an iotlb_gather structure and followed up with a synchronous TLB flush. Since this code is invoked from msm_iommu_unmap(), leaving the TLB unflushed might retain stale translations in hardware.
Can this lead to a use-after-free regression if the physical pages are later reallocated and accessed by the GPU through the stale TLB entries?
Quoted 3 lines
drivers/gpu/drm/msm/msm_iommu.c +3
-0+ +static int +__map_iommu(void *arg, phys_addr_t paddr, u64 iova, size_t pgsize,
[ ... ]
drivers/gpu/drm/msm/msm_iommu.c static int msm_iommu_map(struct msm_mmu *mmu, uint64_t iova, · line 717
Quoted 12 lines
drivers/gpu/drm/msm/msm_iommu.c +2
-5@@ -697,23 +712,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, __map_iommu, __unmap_iommu); }
[Severity: Low]
Is __do_map() defined anywhere in the tree?
It appears this replaces iommu_map_sgtable() with a call to __do_map(), but there is no declaration or definition of __do_map() available. Does this cause an implicit declaration compile error when building with DRM_MSM enabled?
Quoted 4 lines
drivers/gpu/drm/msm/msm_iommu.c +0
-1static int msm_iommu_unmap (struct msm_mmu *mmu,uint64_t iova,size_t len) { -struct msm_iommu *iommu =to_msm_iommu (mmu);
drivers/gpu/drm/msm/msm_iommu.c static int msm_iommu_map(struct msm_mmu *mmu, uint64_t iova, · line 763
Quoted 10 lines
drivers/gpu/drm/msm/msm_iommu.c +2
-3+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, __unmap_iommu); }
[Severity: Low]
Similar to the mapping path, is __do_unmap() actually defined?
It looks like the change from iommu_unmap() to __do_unmap() will fail to compile due to a missing definition.
Additionally, if __do_unmap() simply invokes __unmap_iommu() and returns, is the required TLB flush completely missing from this unmap sequence?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Re: [RFT 4/5] drm/msm: Convert iommu map/unmap to helpers
Quoted 2 lines
By passing NULL for
iotlb_gather, does this bypass the IOMMU driver's TLB gathering and flushing mechanism?
Yes. On arm64 QEMU/SMMUv3, unmap_pages(..., NULL) returned 4096 after device DMA populated the IOTLB, but the next DMA still reached the old page. It faulted after an explicit IOTLB flush.
Using iommu_unmap() invalidated the translation:
return iommu_unmap(domain, iova, pgsize * pgcount);
Quoted 6 lines
It appears this replaces
iommu_map_sgtable()with a call to__do_map(), but there is no declaration or definition of__do_map()available.[...]
Similar to the mapping path, is
__do_unmap()actually defined?
Patch 3 defines both helpers. msm_iommu.o builds with all five patches.
Karl
Re: [RFT 4/5] drm/msm: Convert iommu map/unmap to helpers
Quoted 12 lines · Karl Mehltretter
Quoted 3 lines
By passing NULL for
iotlb_gather, does this bypass the IOMMU driver's TLB gathering and flushing mechanism?Yes. On arm64 QEMU/SMMUv3,
unmap_pages(..., NULL)returned 4096 after device DMA populated the IOTLB, but the next DMA still reached the old page. It faulted after an explicit IOTLB flush.Using
iommu_unmap()invalidated the translation:return iommu_unmap(domain, iova, pgsize * pgcount);
Quoted 8 lines
Quoted 6 lines
It appears this replaces
iommu_map_sgtable()with a call to__do_map(), but there is no declaration or definition of__do_map()available.[...]
Similar to the mapping path, is
__do_unmap()actually defined?Patch 3 defines both helpers. msm_iommu.o builds with all five patches.
Sashiko is kinda funny sometimes :-)
BR,
-R
Rob Clark
maintainerPATCH 5/5 drm/msm: Convert map helper to use dma-address
In reply to Rob ClarkNow 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.
--- drivers/gpu/drm/msm/msm_iommu.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/gpu/drm/msm/msm_iommu.c +3 -3
@@ -148,9 +148,9 @@ __do_map(unsigned long pgsize_bitmap, u64 iova, struct sg_table *sgt, size_t offu64 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 ;
Signature · 1 line
2.55.0
Hi Rob,
Quoted 3 lines · 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). [...]
Tested on the machine that originally reported the breakage - this exercises exactly the externally-allocated-buffer case from your cover letter:
SFA14-11), v7.3-rc5 + this series, CONFIG_DMABUF_DEBUG=y
Hardware video decode in chromium (V4L2 decoder capture buffers from videobuf2-dma-contig imported into msm and rendered by the GPU) displays correctly, with zero arm-smmu faults and zero io-pgtable WARNs. For comparison, on plain v7.3-rc5 with DMABUF_DEBUG=y the same workload logs UCHE translation faults and a __arm_lpae_unmap() WARN storm (~470 traces per minute of playback).
This is also much cleaner than the translation-based approach in the follow-up series I withdrew - mapping from the DMA addresses is where I should have ended up in the first place.
One small suggestion for __do_map(): panthor treats "map_pages() mapped nothing" as an error (panthor_mmu.c):
/* If nothing was mapped, consider it an ENOMEM. */
if (!ret && !mapped)
ret = -ENOMEM;With the DMA fields now populated on both paths this should not trigger in practice, but it would turn any future regression back into a loud failure instead of a silent empty mapping - which is the failure mode this whole series fixes.
Happy to run more (heap-exported dmabuf import test, kmssink scanout of V4L2 frames) on any revision.
BR,
Jianfeng
Quoted 26 lines · Jianfeng Liu
Hi Rob,
Quoted 3 lines · 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). [...]
Tested on the machine that originally reported the breakage - this exercises exactly the externally-allocated-buffer case from your cover letter:
SFA14-11), v7.3-rc5 + this series,
CONFIG_DMABUF_DEBUG=yHardware video decode in chromium (V4L2 decoder capture buffers from videobuf2-dma-contig imported into msm and rendered by the GPU) displays correctly, with zero arm-smmu faults and zero io-pgtable WARNs. For comparison, on plain v7.3-rc5 with DMABUF_DEBUG=y the same workload logs UCHE translation faults and a
__arm_lpae_unmap()WARN storm (~470 traces per minute of playback).This is also much cleaner than the translation-based approach in the follow-up series I withdrew - mapping from the DMA addresses is where I should have ended up in the first place.
Quoted 6 lines
One small suggestion for
__do_map(): panthor treats "map_pages()mapped nothing" as an error (panthor_mmu.c):/* If nothing was mapped, consider it an ENOMEM. */ if (!ret && !mapped) ret = -ENOMEM;
Yeah, that is probably a good idea.
Thanks for testing
BR,
-R
Quoted 11 lines
With the DMA fields now populated on both paths this should not trigger in practice, but it would turn any future regression back into a loud failure instead of a silent empty mapping - which is the failure mode this whole series fixes.
Happy to run more (heap-exported dmabuf import test, kmssink scanout of V4L2 frames) on any revision.
BR,
Jianfeng