On 28/02/2025 10:44, Thomas Hellström wrote:
The pnfs that we obtain from hmm_range_fault() point to pages that we don't have a reference on, and the guarantee that they are still in the cpu page-tables is that the notifier lock must be held and the notifier seqno is still valid.
So while building the sg table and marking the pages accesses / dirty we need to hold this lock with a validated seqno.
However, the lock is reclaim tainted which makes sg_alloc_table_from_pages_segment() unusable, since it internally allocates memory.
Instead build the sg-table manually. For the non-iommu case this might lead to fewer coalesces, but if that's a problem it can be fixed up later in the resource cursor code. For the iommu case, the whole sg-table may still be coalesced to a single contigous device va region.
This avoids marking pages that we don't own dirty and accessed, and it also avoid dereferencing struct pages that we don't own.
Fixes: 81e058a3e7fd ("drm/xe: Introduce helper to populate userptr") Cc: Oak Zeng oak.zeng@intel.com Cc: stable@vger.kernel.org # v6.10+ Signed-off-by: Thomas Hellström thomas.hellstrom@linux.intel.com
drivers/gpu/drm/xe/xe_hmm.c | 115 ++++++++++++++++++++++++++---------- 1 file changed, 85 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/xe/xe_hmm.c b/drivers/gpu/drm/xe/xe_hmm.c index c56738fa713b..d3b5551496d0 100644 --- a/drivers/gpu/drm/xe/xe_hmm.c +++ b/drivers/gpu/drm/xe/xe_hmm.c @@ -42,6 +42,36 @@ static void xe_mark_range_accessed(struct hmm_range *range, bool write) } } +static int xe_alloc_sg(struct sg_table *st, struct hmm_range *range,
struct rw_semaphore *notifier_sem)+{
- unsigned long i, npages, hmm_pfn;
- unsigned long num_chunks = 0;
- int ret;
- /* HMM docs says this is needed. */
- ret = down_read_interruptible(notifier_sem);
- if (ret)
return ret;- if (mmu_interval_read_retry(range->notifier, range->notifier_seq))
return -EAGAIN;- npages = xe_npages_in_range(range->start, range->end);
- for (i = 0; i < npages;) {
hmm_pfn = range->hmm_pfns[i];if (!(hmm_pfn & HMM_PFN_VALID)) {
Is this possible? The default_flags are always REQ_FAULT, so that should ensure PFN_VALID, or the hmm_fault would have returned an error?
From the docs:
"HMM_PFN_REQ_FAULT - The output must have HMM_PFN_VALID or hmm_range_fault() will fail"
Should this be an assert?
Also probably dumb question, but why do we need to hold the notifier lock over this loop? What is it protecting?
up_read(notifier_sem);return -EFAULT;}num_chunks++;i += 1UL << hmm_pfn_to_map_order(hmm_pfn);- }
- up_read(notifier_sem);
- return sg_alloc_table(st, num_chunks, GFP_KERNEL);
+}
- /**
- xe_build_sg() - build a scatter gather table for all the physical pages/pfn
- in a hmm_range. dma-map pages if necessary. dma-address is save in sg table
@@ -50,6 +80,7 @@ static void xe_mark_range_accessed(struct hmm_range *range, bool write)
- @range: the hmm range that we build the sg table from. range->hmm_pfns[]
- has the pfn numbers of pages that back up this hmm address range.
- @st: pointer to the sg table.
- @notifier_sem: The xe notifier lock.
- @write: whether we write to this range. This decides dma map direction
- for system pages. If write we map it bi-diretional; otherwise
- DMA_TO_DEVICE
@@ -76,38 +107,33 @@ static void xe_mark_range_accessed(struct hmm_range *range, bool write)
- Returns 0 if successful; -ENOMEM if fails to allocate memory
*/ static int xe_build_sg(struct xe_device *xe, struct hmm_range *range,
struct sg_table *st, bool write)
struct sg_table *st,struct rw_semaphore *notifier_sem,bool write)
- struct page **pages;
- u64 i, npages;
- int ret;
- npages = xe_npages_in_range(range->start, range->end);
- pages = kvmalloc_array(npages, sizeof(*pages), GFP_KERNEL);
- if (!pages)
return -ENOMEM;- for (i = 0; i < npages; i++) {
pages[i] = hmm_pfn_to_page(range->hmm_pfns[i]);xe_assert(xe, !is_device_private_page(pages[i]));- }
- ret = sg_alloc_table_from_pages_segment(st, pages, npages, 0, npages << PAGE_SHIFT,
xe_sg_segment_size(dev), GFP_KERNEL);- if (ret)
goto free_pages;- ret = dma_map_sgtable(dev, st, write ? DMA_BIDIRECTIONAL : DMA_TO_DEVICE,
DMA_ATTR_SKIP_CPU_SYNC | DMA_ATTR_NO_KERNEL_MAPPING);- if (ret) {
sg_free_table(st);st = NULL;
- unsigned long hmm_pfn, size;
- struct scatterlist *sgl;
- struct page *page;
- unsigned long i, j;
- lockdep_assert_held(notifier_sem);
- i = 0;
- for_each_sg(st->sgl, sgl, st->nents, j) {
hmm_pfn = range->hmm_pfns[i];page = hmm_pfn_to_page(hmm_pfn);xe_assert(xe, !is_device_private_page(page));size = 1UL << hmm_pfn_to_map_order(hmm_pfn);sg_set_page(sgl, page, size << PAGE_SHIFT, 0);if (unlikely(j == st->nents - 1))sg_mark_end(sgl);i += size;- xe_assert(xe, i == xe_npages_in_range(range->start, range->end));
-free_pages:
- kvfree(pages);
- return ret;
- return dma_map_sgtable(dev, st, write ? DMA_BIDIRECTIONAL : DMA_TO_DEVICE,
DMA_ATTR_SKIP_CPU_SYNC | DMA_ATTR_NO_KERNEL_MAPPING);/** @@ -235,16 +261,45 @@ int xe_hmm_userptr_populate_range(struct xe_userptr_vma *uvma, if (ret) goto free_pfns;
- ret = xe_build_sg(vm->xe, &hmm_range, &userptr->sgt, write);
- if (unlikely(userptr->sg)) {
ret = down_write_killable(&vm->userptr.notifier_lock);if (ret)goto free_pfns;xe_hmm_userptr_free_sg(uvma);up_write(&vm->userptr.notifier_lock);- }
- ret = xe_alloc_sg(&userptr->sgt, &hmm_range, &vm->userptr.notifier_lock); if (ret) goto free_pfns;
- ret = down_read_interruptible(&vm->userptr.notifier_lock);
- if (ret)
goto free_st;- if (mmu_interval_read_retry(hmm_range.notifier, hmm_range.notifier_seq)) {
ret = -EAGAIN;goto out_unlock;- }
- ret = xe_build_sg(vm->xe, &hmm_range, &userptr->sgt,
&vm->userptr.notifier_lock, write);- if (ret)
goto out_unlock;- xe_mark_range_accessed(&hmm_range, write); userptr->sg = &userptr->sgt; userptr->notifier_seq = hmm_range.notifier_seq;
- up_read(&vm->userptr.notifier_lock);
- kvfree(pfns);
- return 0;
+out_unlock:
- up_read(&vm->userptr.notifier_lock);
+free_st:
- sg_free_table(&userptr->sgt); free_pfns: kvfree(pfns); return ret; }