On Sat, Sep 05, 2026 at 09:31:41PM +0800, Honglei Huang wrote: > drm_gpusvm_get_pages() sizes the dma_addr array for one drm_pagemap_addr > per page, but the mapping loop advances by page order, so a range backed > by one huge page needs a single entry. For a 2 MiB THP that is an 8 KiB > array holding 16 bytes of address. > > Union that entry with the array pointer, discriminated by a new > inline_dma_mapping flag. When drm_gpusvm_dma_map_pages() ends up with one > entry it stores it inline and frees the array, after the last error > unwind, which still walks the array form. An unchecked dma_addr read is > now type confusion rather than a compile error, so reads go through the > new drm_gpusvm_pages_first_dma() accessor, including the two > xe_pt_stage_bind() paths. > > Only get_pages() and the free path write the union, never the notifier, > and both run under the driver lock that every address reader already > holds. The unlocked short circuit in drm_gpusvm_pages_valid_unlocked() > goes for the same reason: it cannot resolve the union, and every instance > it rejects has to be reset before the allocation loop reuses it. > > Suggested-by: Matthew Brost <[email protected]>
Reviewed-by: Matthew Brost <[email protected]> > Signed-off-by: Honglei Huang <[email protected]> > --- > drivers/gpu/drm/drm_gpusvm.c | 48 ++++++++++++++++++++++++++++------ > drivers/gpu/drm/xe/xe_pt.c | 7 ++--- > drivers/gpu/drm/xe/xe_svm.h | 18 +++++++++++++ > include/drm/drm_gpusvm.h | 50 +++++++++++++++++++++++++++++++++--- > 4 files changed, 109 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c > index 7efc35507f1..2c7c4c89dc4 100644 > --- a/drivers/gpu/drm/drm_gpusvm.c > +++ b/drivers/gpu/drm/drm_gpusvm.c > @@ -1241,6 +1241,8 @@ static void __drm_gpusvm_unmap_pages(struct drm_gpusvm > *gpusvm, > struct drm_gpusvm_pages_flags flags = { > .__flags = svm_pages->flags.__flags, > }; > + const struct drm_pagemap_addr *addrs = > + drm_gpusvm_pages_first_dma(svm_pages); > bool use_iova = dma_use_iova(&svm_pages->state); > > /* > @@ -1253,12 +1255,12 @@ static void __drm_gpusvm_unmap_pages(struct > drm_gpusvm *gpusvm, > if (svm_pages->state_offset) > dma_iova_unlink(dev, &svm_pages->state, 0, > svm_pages->state_offset, > - svm_pages->dma_addr[0].dir, 0); > + addrs[0].dir, 0); > dma_iova_free(dev, &svm_pages->state); > } > > for (i = 0, j = 0; i < npages; j++) { > - struct drm_pagemap_addr *addr = &svm_pages->dma_addr[j]; > + const struct drm_pagemap_addr *addr = &addrs[j]; > > if (addr->proto == DRM_INTERCONNECT_SYSTEM) { > /* > @@ -1299,6 +1301,18 @@ static void __drm_gpusvm_free_pages(struct drm_gpusvm > *gpusvm, > { > lockdep_assert_held(&gpusvm->notifier_lock); > > + if (svm_pages->flags.inline_dma_mapping) { > + struct drm_gpusvm_pages_flags flags = { > + .__flags = svm_pages->flags.__flags, > + }; > + > + svm_pages->inline_addr = (struct drm_pagemap_addr){}; > + flags.inline_dma_mapping = false; > + /* WRITE_ONCE pairs with READ_ONCE for opportunistic checks */ > + WRITE_ONCE(svm_pages->flags.__flags, flags.__flags); > + return; > + } > + > if (svm_pages->dma_addr) { > kvfree(svm_pages->dma_addr); > svm_pages->dma_addr = NULL; > @@ -1463,11 +1477,6 @@ static bool drm_gpusvm_pages_valid_unlocked(struct > drm_gpusvm *gpusvm, > bool pages_valid = true; > unsigned int p; > > - for (p = 0; p < num_pages; ++p) { > - if (!svm_pages[p].dma_addr) > - return false; > - } > - > drm_gpusvm_notifier_lock(gpusvm); > for (p = 0; p < num_pages; ++p) { > if (drm_gpusvm_pages_valid(gpusvm, &svm_pages[p])) > @@ -1480,6 +1489,21 @@ static bool drm_gpusvm_pages_valid_unlocked(struct > drm_gpusvm *gpusvm, > return pages_valid; > } > > +/** > + * drm_gpusvm_pages_inlinable() - Whether the dma address can be inlined > + * @nentries: Number of entries the mapping loop produced > + * > + * A THP maps as one huge page, so the whole range needs a single device > + * address: the dma_addr array can be freed and the address kept inline, > + * which is where the memory saving comes from. > + * > + * Return: True if the mapping fits in a single drm_pagemap_addr. > + */ > +static bool drm_gpusvm_pages_inlinable(unsigned long nentries) > +{ > + return nentries == 1; > +} > + > /** > * drm_gpusvm_dma_map_pages() - DMA map one drm_gpusvm_pages instance > * @gpusvm: Pointer to the GPU SVM structure > @@ -1632,6 +1656,14 @@ static int drm_gpusvm_dma_map_pages(struct drm_gpusvm > *gpusvm, > if (pagemap) > flags.has_devmem_pages = true; > > + if (drm_gpusvm_pages_inlinable(j)) { > + struct drm_pagemap_addr addr = svm_pages->dma_addr[0]; > + > + kvfree(svm_pages->dma_addr); > + svm_pages->inline_addr = addr; > + flags.inline_dma_mapping = true; > + } > + > /* WRITE_ONCE pairs with READ_ONCE for opportunistic checks */ > WRITE_ONCE(svm_pages->flags.__flags, flags.__flags); > > @@ -1740,7 +1772,7 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm, > > if (map_dma) { > for (p = 0; p < num_pages; ++p) { > - if (svm_pages[p].dma_addr) > + if (drm_gpusvm_pages_first_dma(&svm_pages[p])) > continue; > svm_pages[p].dma_addr = > kvzalloc_objs(*svm_pages[p].dma_addr, npages); > diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c > index 5d990c1c374..fa4b29da0b6 100644 > --- a/drivers/gpu/drm/xe/xe_pt.c > +++ b/drivers/gpu/drm/xe/xe_pt.c > @@ -831,7 +831,7 @@ xe_pt_stage_bind(struct xe_tile *tile, struct xe_vma *vma, > return -EAGAIN; > } > if (xe_svm_range_has_dma_mapping(range)) { > - xe_res_first_dma(range->pages.dma_addr, 0, > + xe_res_first_dma(xe_svm_range_first_dma(range), 0, > xe_svm_range_size(range), > &curs); > xe_svm_range_debug(range, "BIND PREPARE - MIXED"); > @@ -866,8 +866,9 @@ xe_pt_stage_bind(struct xe_tile *tile, struct xe_vma *vma, > > if (!xe_vma_is_null(vma) && !range && !is_purged) { > if (xe_vma_is_userptr(vma)) > - > xe_res_first_dma(to_userptr_vma(vma)->userptr.pages.dma_addr, 0, > - xe_vma_size(vma), &curs); > + xe_res_first_dma(drm_gpusvm_pages_first_dma > + (&to_userptr_vma(vma)->userptr.pages), > + 0, xe_vma_size(vma), &curs); > else if (xe_bo_is_vram(bo) || xe_bo_is_stolen(bo)) > xe_res_first(bo->ttm.resource, xe_vma_bo_offset(vma), > xe_vma_size(vma), &curs); > diff --git a/drivers/gpu/drm/xe/xe_svm.h b/drivers/gpu/drm/xe/xe_svm.h > index 2a0dc0d125c..7eb80d4d6db 100644 > --- a/drivers/gpu/drm/xe/xe_svm.h > +++ b/drivers/gpu/drm/xe/xe_svm.h > @@ -220,6 +220,18 @@ static inline unsigned long xe_svm_range_size(struct > xe_svm_range *range) > return drm_gpusvm_range_size(&range->base); > } > > +/** > + * xe_svm_range_first_dma() - Resolve the device address array of a SVM range > + * @range: SVM range > + * > + * Return: Pointer to the first device address, NULL if none is populated. > + */ > +static inline const struct drm_pagemap_addr * > +xe_svm_range_first_dma(struct xe_svm_range *range) > +{ > + return drm_gpusvm_pages_first_dma(&range->pages); > +} > + > void xe_svm_flush(struct xe_vm *vm); > > int xe_pagemap_shrinker_create(struct xe_device *xe); > @@ -436,6 +448,12 @@ static inline bool xe_svm_range_is_removed(struct > xe_svm_range *range) > return false; > } > > +static inline const struct drm_pagemap_addr * > +xe_svm_range_first_dma(struct xe_svm_range *range) > +{ > + return NULL; > +} > + > #define xe_svm_range_has_dma_mapping(...) false > #endif /* CONFIG_DRM_XE_GPUSVM */ > > diff --git a/include/drm/drm_gpusvm.h b/include/drm/drm_gpusvm.h > index ec7b81957b1..aaad5c9b510 100644 > --- a/include/drm/drm_gpusvm.h > +++ b/include/drm/drm_gpusvm.h > @@ -10,6 +10,7 @@ > #include <linux/kref.h> > #include <linux/interval_tree.h> > #include <linux/mmu_notifier.h> > +#include <drm/drm_pagemap.h> > > struct dev_pagemap_ops; > struct drm_device; > @@ -18,7 +19,6 @@ struct drm_gpusvm_notifier; > struct drm_gpusvm_ops; > struct drm_gpusvm_range; > struct drm_pagemap; > -struct drm_pagemap_addr; > > /** > * struct drm_gpusvm_ops - Operations structure for GPU SVM > @@ -112,6 +112,7 @@ struct drm_gpusvm_notifier { > * @unmapped: Flag indicating if the pages has been unmapped > * @has_devmem_pages: Flag indicating if the pages has devmem pages > * @has_dma_mapping: Flag indicating if the pages has a DMA mapping > + * @inline_dma_mapping: Flag indicating if the pages have an inline DMA > mapping > * @__flags: Flags for pages in u16 form (used for READ_ONCE) > */ > struct drm_gpusvm_pages_flags { > @@ -121,6 +122,7 @@ struct drm_gpusvm_pages_flags { > u16 unmapped : 1; > u16 has_devmem_pages : 1; > u16 has_dma_mapping : 1; > + u16 inline_dma_mapping : 1; > }; > u16 __flags; > }; > @@ -130,17 +132,27 @@ struct drm_gpusvm_pages_flags { > * struct drm_gpusvm_pages - Structure representing a GPU SVM mapped pages > * > * @drm: The DRM device that owns the dma mappings > - * @dma_addr: Device address array > + * @dma_addr: Device address array, valid while @flags.inline_dma_mapping is > + * not set > + * @inline_addr: Device address inline address, valid while > + * @flags.inline_dma_mapping is set > * @dpagemap: The struct drm_pagemap of the device pages we're dma-mapping. > * Note this is assuming only one drm_pagemap per range is > allowed. > * @state: DMA IOVA state for mapping. > * @state_offset: DMA IOVA offset for mapping. > * @notifier_seq: Notifier sequence number of the range's pages > * @flags: Flags for the range; see &struct drm_gpusvm_pages_flags > + * > + * @dma_addr and @inline_addr share storage, discriminated by > + * @flags.inline_dma_mapping. Driver should use drm_gpusvm_pages_first_dma() > + * to access the correct DMA address. > */ > struct drm_gpusvm_pages { > struct drm_device *drm; > - struct drm_pagemap_addr *dma_addr; > + union { > + struct drm_pagemap_addr *dma_addr; > + struct drm_pagemap_addr inline_addr; > + }; > struct drm_pagemap *dpagemap; > struct dma_iova_state state; > unsigned long state_offset; > @@ -365,6 +377,38 @@ static inline void drm_gpusvm_init_pages(struct > drm_gpusvm_pages *svm_pages, > svm_pages->notifier_seq = LONG_MAX; > } > > +/** > + * drm_gpusvm_pages_first_dma() - Resolve the device address array > + * @svm_pages: Pointer to the drm_gpusvm_pages. > + * > + * drm_gpusvm_pages use unions to optimize the storage of DMA addresses, > + * this function abstracts the access to the first device address. The driver > + * should use this helper instead of reading dma_addr directly to prevent > + * array out of bounds access. > + * > + * Only get_pages() and the free path switch between the two union members. > + * Both hold the notifier lock for read, so taking that lock does not stop > + * them; callers need the driver lock that does, which every reader of the > + * addresses holds anyway. The notifier never touches the union, so the > + * pointer returned here stays good and can then be used under the notifier > + * lock. > + * > + * Return: Pointer to the first device address, NULL if none is populated. > + */ > +static inline const struct drm_pagemap_addr * > +drm_gpusvm_pages_first_dma(const struct drm_gpusvm_pages *svm_pages) > +{ > + struct drm_gpusvm_pages_flags flags = { > + /* READ_ONCE pairs with the WRITE_ONCE of the flag writers */ > + .__flags = READ_ONCE(svm_pages->flags.__flags), > + }; > + > + if (flags.inline_dma_mapping) > + return &svm_pages->inline_addr; > + > + return READ_ONCE(svm_pages->dma_addr); > +} > + > /** > * enum drm_gpusvm_scan_result - Scan result from the drm_gpusvm_scan_mm() > function. > * @DRM_GPUSVM_SCAN_UNPOPULATED: At least one page was not present or > inaccessible. > -- > 2.34.1 >
