[RFC PATCH v1 0/5] drm/gpusvm: share one HMM fault across per-device DMA mappings

[RFC PATCH v1 0/5] drm/gpusvm: share one HMM fault across per-device DMA mappings

[Honglei Huang]

This series is a follow up to the earlier work in the drm_gpusvm_pages
decoupling series [1], and follows the direction Matt suggested [2].

drm_gpusvm_get_pages() used to do two things at once: the MM level HMM
fault of the CPU range, and the device DMA mapping of the faulted pages.
When one CPU range is mirrored on several devices, each device had
to call get_pages() and redo the HMM fault every time. Matt suggested [2]
passing an array of drm_gpusvm_pages plus a count so the fault is done
once and shared, while keeping the notifier retry loop in common code so
drivers never open code it.

The series builds up to that in small steps, no functional change until
the last two patches add the new behaviour:

  - patch 1 pulls the HMM fault, and its -EBUSY retry, into
    drm_gpusvm_hmm_fault() so it can be shared.
  - patch 2 moves the dma_addr allocation out of the notifier locked
    section, so the mapping step becomes self contained.
  - patch 3 pulls the per-device mapping loop into
    drm_gpusvm_dma_map_pages().
  - patch 4 makes get_pages() take an array of drm_gpusvm_pages plus a
    count: fault once, then DMA map each instance, one per owning
    drm_device, under a single notifier retry gate. The common 1:1 case
    passes count == 1.
  - patch 5 adds no_dma_map so a driver that only needs
    the CPU pages faulted in (e.g. AMDXDNA using GPU SVM for userptr)
    can skip the device DMA mapping.

tests:
AMDGPU:
  SVM:DRM N:1 multi device support is work in progress on top of this
  series. The single device (1:1) path was tested with the amdgpu SVM
  adaptation on top. Based on amdgpu SVM [3].
  Tested on gfx943 (MI300X) and gfx906 (MI60) with XNACK on/off:
  - KFD test: 95%+ passed.
  - ROCR test: all passed.
  - HIP catch test: gfx943 (MI300X): 99% passed.
                    gfx906 (MI60): 99% passed.

links:
[1] drm_gpusvm_pages decoupling series:
    https://lore.kernel.org/amd-gfx/20260630102127.392396-1-honghuan@amd.com/
[2] Matt's suggested direction:
    https://lore.kernel.org/amd-gfx/aijdg7RWwrEDEMxC@gsse-cloud1.jf.intel.com/#:~:text=Hmm%2C%20this%20might,requires%20DMA%20mapping.
[3] amdgpu SVM:
    https://lore.kernel.org/amd-gfx/20260804094246.1719318-1-ray.huang@amd.com/

Honglei Huang (5):
  drm/gpusvm: extract drm_gpusvm_hmm_fault() helper
  drm/gpusvm: move dma_addr allocation before the notifier lock
  drm/gpusvm: extract drm_gpusvm_dma_map_pages() helper
  drm/gpusvm: let drm_gpusvm_get_pages() map an array of pages
  drm/gpusvm: make the DMA mapping step in get_pages() optional

 drivers/gpu/drm/drm_gpusvm.c    | 339 ++++++++++++++++++++++----------
 drivers/gpu/drm/xe/xe_svm.c     |   2 +-
 drivers/gpu/drm/xe/xe_userptr.c |   2 +-
 include/drm/drm_gpusvm.h        |   6 +
 4 files changed, 244 insertions(+), 105 deletions(-)


base-commit: 04984fcdbf6876c940c01026a7404c1e9cc91ba7
-- 
2.34.1

[RFC PATCH v1 1/5] drm/gpusvm: extract drm_gpusvm_hmm_fault() helper

[Honglei Huang]

Make the HMM fault step of drm_gpusvm_get_pages(), including its -EBUSY
retry loop, into a helper drm_gpusvm_hmm_fault(). The existing logic of
the public drm_gpusvm_get_pages() is not changed, only relocated, so
there is no functional change. Keeping the retry loop in common code
also means drivers never have to open-code their own fault/retry loop.

A single fault can later be shared by several drm_gpusvm_pages instances
that mirror the same CPU range. This prepares get_pages() to split the
shared MM-level fault from the per-device DMA mapping. No functional
change intended.

Suggested-by: Matthew Brost <matthew.brost@intel.com>
Signed-off-by: Honglei Huang <honghuan@amd.com>
---
 drivers/gpu/drm/drm_gpusvm.c | 67 ++++++++++++++++++++++++------------
 1 file changed, 45 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
index fcfe635bc195..507ef6f0a60e 100644
--- a/drivers/gpu/drm/drm_gpusvm.c
+++ b/drivers/gpu/drm/drm_gpusvm.c
@@ -1442,6 +1442,50 @@ static bool drm_gpusvm_pages_valid_unlocked(struct drm_gpusvm *gpusvm,
 	return pages_valid;
 }
 
+/**
+ * drm_gpusvm_hmm_fault() - Run the shared HMM fault for a CPU range
+ * @gpusvm: Pointer to the GPU SVM structure
+ * @mm: The mm corresponding to the CPU range
+ * @hmm_range: The hmm_range to fault.
+ * @pfns: The pfn array to populate (size @npages)
+ * @timeout: jiffies deadline for the -EBUSY retry loop
+ *
+ * Fault the CPU pages of the range into @pfns. This is the MM level step.
+ *
+ * Return: 0 on success, negative error code on failure.
+ */
+static int drm_gpusvm_hmm_fault(struct drm_gpusvm *gpusvm,
+				struct mm_struct *mm,
+				struct hmm_range *hmm_range,
+				unsigned long *pfns,
+				unsigned long timeout)
+{
+	int err;
+
+	if (!mmget_not_zero(mm))
+		return -EFAULT;
+
+	hmm_range->hmm_pfns = pfns;
+	while (true) {
+		mmap_read_lock(mm);
+		err = hmm_range_fault(hmm_range);
+		mmap_read_unlock(mm);
+
+		if (err == -EBUSY) {
+			if (time_after(jiffies, timeout))
+				break;
+
+			hmm_range->notifier_seq =
+				mmu_interval_read_begin(hmm_range->notifier);
+			continue;
+		}
+		break;
+	}
+	mmput(mm);
+
+	return err;
+}
+
 /**
  * drm_gpusvm_get_pages() - Get pages and populate GPU SVM pages struct
  * @gpusvm: Pointer to the GPU SVM structure
@@ -1503,28 +1547,7 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
 	if (!pfns)
 		return -ENOMEM;
 
-	if (!mmget_not_zero(mm)) {
-		err = -EFAULT;
-		goto err_free;
-	}
-
-	hmm_range.hmm_pfns = pfns;
-	while (true) {
-		mmap_read_lock(mm);
-		err = hmm_range_fault(&hmm_range);
-		mmap_read_unlock(mm);
-
-		if (err == -EBUSY) {
-			if (time_after(jiffies, timeout))
-				break;
-
-			hmm_range.notifier_seq =
-				mmu_interval_read_begin(notifier);
-			continue;
-		}
-		break;
-	}
-	mmput(mm);
+	err = drm_gpusvm_hmm_fault(gpusvm, mm, &hmm_range, pfns, timeout);
 	if (err)
 		goto err_free;
 
-- 
2.34.1

Re: [RFC PATCH v1 1/5] drm/gpusvm: extract drm_gpusvm_hmm_fault() helper

[Matthew Brost]

On Thu, Aug 27, 2026 at 03:14:45PM +0800, Honglei Huang wrote:
> Make the HMM fault step of drm_gpusvm_get_pages(), including its -EBUSY
> retry loop, into a helper drm_gpusvm_hmm_fault(). The existing logic of
> the public drm_gpusvm_get_pages() is not changed, only relocated, so
> there is no functional change. Keeping the retry loop in common code
> also means drivers never have to open-code their own fault/retry loop.
> 
> A single fault can later be shared by several drm_gpusvm_pages instances
> that mirror the same CPU range. This prepares get_pages() to split the
> shared MM-level fault from the per-device DMA mapping. No functional
> change intended.
> 

I think you might want to just wait on this until Sunday for this
series. I think this patch [1] is in the core MM tree so when drm-tip
moves to 7.3.rc1, Sunday, we will have a version of this helper to core
MM used in gpusvm.

Matt

[1] https://lore.freedesktop.org/nouveau/20260722-hmm-v10-v1-6-606464dd601a@gmail.com/T/#m68f663ce3e802d7692363c70e6364569134cd6c7

> Suggested-by: Matthew Brost <matthew.brost@intel.com>
> Signed-off-by: Honglei Huang <honghuan@amd.com>
> ---
>  drivers/gpu/drm/drm_gpusvm.c | 67 ++++++++++++++++++++++++------------
>  1 file changed, 45 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
> index fcfe635bc195..507ef6f0a60e 100644
> --- a/drivers/gpu/drm/drm_gpusvm.c
> +++ b/drivers/gpu/drm/drm_gpusvm.c
> @@ -1442,6 +1442,50 @@ static bool drm_gpusvm_pages_valid_unlocked(struct drm_gpusvm *gpusvm,
>  	return pages_valid;
>  }
>  
> +/**
> + * drm_gpusvm_hmm_fault() - Run the shared HMM fault for a CPU range
> + * @gpusvm: Pointer to the GPU SVM structure
> + * @mm: The mm corresponding to the CPU range
> + * @hmm_range: The hmm_range to fault.
> + * @pfns: The pfn array to populate (size @npages)
> + * @timeout: jiffies deadline for the -EBUSY retry loop
> + *
> + * Fault the CPU pages of the range into @pfns. This is the MM level step.
> + *
> + * Return: 0 on success, negative error code on failure.
> + */
> +static int drm_gpusvm_hmm_fault(struct drm_gpusvm *gpusvm,
> +				struct mm_struct *mm,
> +				struct hmm_range *hmm_range,
> +				unsigned long *pfns,
> +				unsigned long timeout)
> +{
> +	int err;
> +
> +	if (!mmget_not_zero(mm))
> +		return -EFAULT;
> +
> +	hmm_range->hmm_pfns = pfns;
> +	while (true) {
> +		mmap_read_lock(mm);
> +		err = hmm_range_fault(hmm_range);
> +		mmap_read_unlock(mm);
> +
> +		if (err == -EBUSY) {
> +			if (time_after(jiffies, timeout))
> +				break;
> +
> +			hmm_range->notifier_seq =
> +				mmu_interval_read_begin(hmm_range->notifier);
> +			continue;
> +		}
> +		break;
> +	}
> +	mmput(mm);
> +
> +	return err;
> +}
> +
>  /**
>   * drm_gpusvm_get_pages() - Get pages and populate GPU SVM pages struct
>   * @gpusvm: Pointer to the GPU SVM structure
> @@ -1503,28 +1547,7 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
>  	if (!pfns)
>  		return -ENOMEM;
>  
> -	if (!mmget_not_zero(mm)) {
> -		err = -EFAULT;
> -		goto err_free;
> -	}
> -
> -	hmm_range.hmm_pfns = pfns;
> -	while (true) {
> -		mmap_read_lock(mm);
> -		err = hmm_range_fault(&hmm_range);
> -		mmap_read_unlock(mm);
> -
> -		if (err == -EBUSY) {
> -			if (time_after(jiffies, timeout))
> -				break;
> -
> -			hmm_range.notifier_seq =
> -				mmu_interval_read_begin(notifier);
> -			continue;
> -		}
> -		break;
> -	}
> -	mmput(mm);
> +	err = drm_gpusvm_hmm_fault(gpusvm, mm, &hmm_range, pfns, timeout);
>  	if (err)
>  		goto err_free;
>  
> -- 
> 2.34.1
> 

Re: [RFC PATCH v1 1/5] drm/gpusvm: extract drm_gpusvm_hmm_fault() helper

[Huang, Honglei]

On 8/27/2026 3:30 PM, Matthew Brost wrote:
> On Thu, Aug 27, 2026 at 03:14:45PM +0800, Honglei Huang wrote:
>> Make the HMM fault step of drm_gpusvm_get_pages(), including its -EBUSY
>> retry loop, into a helper drm_gpusvm_hmm_fault(). The existing logic of
>> the public drm_gpusvm_get_pages() is not changed, only relocated, so
>> there is no functional change. Keeping the retry loop in common code
>> also means drivers never have to open-code their own fault/retry loop.
>>
>> A single fault can later be shared by several drm_gpusvm_pages instances
>> that mirror the same CPU range. This prepares get_pages() to split the
>> shared MM-level fault from the per-device DMA mapping. No functional
>> change intended.
>>
> 
> I think you might want to just wait on this until Sunday for this
> series. I think this patch [1] is in the core MM tree so when drm-tip
> moves to 7.3.rc1, Sunday, we will have a version of this helper to core
> MM used in gpusvm.

Got it, will wait until Sunday. Thanks for the information.

Regards,
Honglei

> 
> Matt
> 
> [1] https://lore.freedesktop.org/nouveau/20260722-hmm-v10-v1-6-606464dd601a@gmail.com/T/#m68f663ce3e802d7692363c70e6364569134cd6c7
> 
>> Suggested-by: Matthew Brost <matthew.brost@intel.com>
>> Signed-off-by: Honglei Huang <honghuan@amd.com>
>> ---
>>   drivers/gpu/drm/drm_gpusvm.c | 67 ++++++++++++++++++++++++------------
>>   1 file changed, 45 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
>> index fcfe635bc195..507ef6f0a60e 100644
>> --- a/drivers/gpu/drm/drm_gpusvm.c
>> +++ b/drivers/gpu/drm/drm_gpusvm.c
>> @@ -1442,6 +1442,50 @@ static bool drm_gpusvm_pages_valid_unlocked(struct drm_gpusvm *gpusvm,
>>   	return pages_valid;
>>   }
>>   
>> +/**
>> + * drm_gpusvm_hmm_fault() - Run the shared HMM fault for a CPU range
>> + * @gpusvm: Pointer to the GPU SVM structure
>> + * @mm: The mm corresponding to the CPU range
>> + * @hmm_range: The hmm_range to fault.
>> + * @pfns: The pfn array to populate (size @npages)
>> + * @timeout: jiffies deadline for the -EBUSY retry loop
>> + *
>> + * Fault the CPU pages of the range into @pfns. This is the MM level step.
>> + *
>> + * Return: 0 on success, negative error code on failure.
>> + */
>> +static int drm_gpusvm_hmm_fault(struct drm_gpusvm *gpusvm,
>> +				struct mm_struct *mm,
>> +				struct hmm_range *hmm_range,
>> +				unsigned long *pfns,
>> +				unsigned long timeout)
>> +{
>> +	int err;
>> +
>> +	if (!mmget_not_zero(mm))
>> +		return -EFAULT;
>> +
>> +	hmm_range->hmm_pfns = pfns;
>> +	while (true) {
>> +		mmap_read_lock(mm);
>> +		err = hmm_range_fault(hmm_range);
>> +		mmap_read_unlock(mm);
>> +
>> +		if (err == -EBUSY) {
>> +			if (time_after(jiffies, timeout))
>> +				break;
>> +
>> +			hmm_range->notifier_seq =
>> +				mmu_interval_read_begin(hmm_range->notifier);
>> +			continue;
>> +		}
>> +		break;
>> +	}
>> +	mmput(mm);
>> +
>> +	return err;
>> +}
>> +
>>   /**
>>    * drm_gpusvm_get_pages() - Get pages and populate GPU SVM pages struct
>>    * @gpusvm: Pointer to the GPU SVM structure
>> @@ -1503,28 +1547,7 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
>>   	if (!pfns)
>>   		return -ENOMEM;
>>   
>> -	if (!mmget_not_zero(mm)) {
>> -		err = -EFAULT;
>> -		goto err_free;
>> -	}
>> -
>> -	hmm_range.hmm_pfns = pfns;
>> -	while (true) {
>> -		mmap_read_lock(mm);
>> -		err = hmm_range_fault(&hmm_range);
>> -		mmap_read_unlock(mm);
>> -
>> -		if (err == -EBUSY) {
>> -			if (time_after(jiffies, timeout))
>> -				break;
>> -
>> -			hmm_range.notifier_seq =
>> -				mmu_interval_read_begin(notifier);
>> -			continue;
>> -		}
>> -		break;
>> -	}
>> -	mmput(mm);
>> +	err = drm_gpusvm_hmm_fault(gpusvm, mm, &hmm_range, pfns, timeout);
>>   	if (err)
>>   		goto err_free;
>>   
>> -- 
>> 2.34.1
>>

[RFC PATCH v1 2/5] drm/gpusvm: move dma_addr allocation before the notifier lock

[Honglei Huang]

The dma_addr allocation was in a lazy allocation flow, it needs unlock
and goto map_pages. The allocation only needs npages, so just do it
before taking the lock. Drop the map_pages label and the relock flow, so
the sequence becomes fault, allocate, then lock, validate, map and
unlock. No functional change intended.

Signed-off-by: Honglei Huang <honghuan@amd.com>
---
 drivers/gpu/drm/drm_gpusvm.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
index 507ef6f0a60e..3496cb568836 100644
--- a/drivers/gpu/drm/drm_gpusvm.c
+++ b/drivers/gpu/drm/drm_gpusvm.c
@@ -1551,10 +1551,18 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
 	if (err)
 		goto err_free;
 
+	if (!svm_pages->dma_addr) {
+		svm_pages->dma_addr =
+			kvmalloc_objs(*svm_pages->dma_addr, npages);
+		if (!svm_pages->dma_addr) {
+			err = -ENOMEM;
+			goto err_free;
+		}
+	}
+
 	*state = (struct dma_iova_state){};
 	svm_pages->state_offset = 0;
 
-map_pages:
 	/*
 	 * Perform all dma mappings under the notifier lock to not
 	 * access freed pages. A notifier will either block on
@@ -1575,18 +1583,6 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
 		goto retry;
 	}
 
-	if (!svm_pages->dma_addr) {
-		/* Unlock and restart mapping to allocate memory. */
-		drm_gpusvm_notifier_unlock(gpusvm);
-		svm_pages->dma_addr =
-			kvmalloc_objs(*svm_pages->dma_addr, npages);
-		if (!svm_pages->dma_addr) {
-			err = -ENOMEM;
-			goto err_free;
-		}
-		goto map_pages;
-	}
-
 	zdd = NULL;
 	pagemap = NULL;
 	num_dma_mapped = 0;
-- 
2.34.1

Re: [RFC PATCH v1 2/5] drm/gpusvm: move dma_addr allocation before the notifier lock

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Device mappings are permanently leaked on the error path because `svm_pages->dpagemap` is assigned too late.
--

commit 69970ff2a9a2262bbdbdbce9ebee1a8d4c896b22
Author: Honglei Huang <honghuan@amd.com>

drm/gpusvm: move dma_addr allocation before the notifier lock

The dma_addr allocation was in a lazy allocation flow, it needs unlock
and goto map_pages. The allocation only needs npages, so just do it
before taking the lock. Drop the map_pages label and the relock flow, so
the sequence becomes fault, allocate, then lock, validate, map and
unlock. No functional change intended.

> diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
> index 507ef6f0a60e7..3496cb5688365 100644
> --- a/drivers/gpu/drm/drm_gpusvm.c
> +++ b/drivers/gpu/drm/drm_gpusvm.c
[ ... ]
> @@ -1575,18 +1583,6 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
>  		goto retry;
>  	}
>  
> -	if (!svm_pages->dma_addr) {
> -		/* Unlock and restart mapping to allocate memory. */
> -		drm_gpusvm_notifier_unlock(gpusvm);
> -		svm_pages->dma_addr =
> -			kvmalloc_objs(*svm_pages->dma_addr, npages);
> -		if (!svm_pages->dma_addr) {
> -			err = -ENOMEM;
> -			goto err_free;
> -		}
> -		goto map_pages;
> -	}
> -
>  	zdd = NULL;
>  	pagemap = NULL;
>  	num_dma_mapped = 0;

[Severity: High]
This is a pre-existing issue, but does the error path here permanently leak
device mappings?

If dpagemap->ops->device_map() succeeds for some pages in
drm_gpusvm_get_pages(), but a subsequent mapping operation (like
dma_map_page() or dma_iova_link()) fails, the code jumps to err_unmap:

	err_unmap:
		svm_pages->flags.has_dma_mapping = true;
		__drm_gpusvm_unmap_pages(gpusvm, svm_pages, num_dma_mapped);

Inside __drm_gpusvm_unmap_pages(), it reads:

	struct drm_pagemap *dpagemap = svm_pages->dpagemap;

And later checks it before unmapping:

	else if (dpagemap && dpagemap->ops->device_unmap)
		dpagemap->ops->device_unmap(dpagemap, dev, addr);

However, back in drm_gpusvm_get_pages(), svm_pages->dpagemap is only
assigned at the very end of the success path:

	if (pagemap) {
		flags.has_devmem_pages = true;
		drm_pagemap_get(dpagemap);
		drm_pagemap_put(svm_pages->dpagemap);
		svm_pages->dpagemap = dpagemap;
	}

Does this mean that during the error path, svm_pages->dpagemap is NULL,
causing __drm_gpusvm_unmap_pages() to silently skip calling
dpagemap->ops->device_unmap() and permanently leak the mappings?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827071449.520398-1-honghuan@amd.com?part=2

[RFC PATCH v1 3/5] drm/gpusvm: extract drm_gpusvm_dma_map_pages() helper

[Honglei Huang]

Move the per-device DMA mapping loop of drm_gpusvm_get_pages() into a
helper drm_gpusvm_dma_map_pages(). The mapping logic is only moved, not
changed, so there is no functional change.

The helper maps the already-faulted pfns into one drm_gpusvm_pages
instance under the notifier lock and unwinds its own partial mapping on
error. The HMM fault and the notifier retry loop stay in get_pages()
common code rather than being pushed down to drivers, so no driver has
to reimplement the subtle fault and retry logic.

With the mapping isolated per instance, get_pages() can later fault once
and DMA map an array of drm_gpusvm_pages plus a count, one per owning
drm_device.

Suggested-by: Matthew Brost <matthew.brost@intel.com>
Signed-off-by: Honglei Huang <honghuan@amd.com>
---
 drivers/gpu/drm/drm_gpusvm.c | 205 ++++++++++++++++++++---------------
 1 file changed, 119 insertions(+), 86 deletions(-)

diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
index 3496cb568836..753361b965cf 100644
--- a/drivers/gpu/drm/drm_gpusvm.c
+++ b/drivers/gpu/drm/drm_gpusvm.c
@@ -1487,105 +1487,41 @@ static int drm_gpusvm_hmm_fault(struct drm_gpusvm *gpusvm,
 }
 
 /**
- * drm_gpusvm_get_pages() - Get pages and populate GPU SVM pages struct
+ * drm_gpusvm_dma_map_pages() - DMA map one drm_gpusvm_pages instance
  * @gpusvm: Pointer to the GPU SVM structure
- * @svm_pages: The SVM pages to populate. This will contain the dma-addresses
- * @mm: The mm corresponding to the CPU range
- * @notifier: The corresponding notifier for the given CPU range
- * @pages_start: Start CPU address for the pages
- * @pages_end: End CPU address for the pages (exclusive)
+ * @svm_pages: The SVM pages instance to populate with dma-addresses
+ * @pfns: The already-faulted pfn array (size @npages)
+ * @npages: Number of pages in the CPU range
  * @ctx: GPU SVM context
+ * @dma_dir: DMA data direction for the mappings
  *
- * This function gets and maps pages for CPU range and ensures they are
- * mapped for DMA access.
+ * Map the faulted @pfns into @svm_pages for DMA access through its owning
+ * drm_device. Must be called under the notifier lock. On failure this unwinds
+ * the partial mapping of this instance before returning.
  *
  * Return: 0 on success, negative error code on failure.
  */
-int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
-			 struct drm_gpusvm_pages *svm_pages,
-			 struct mm_struct *mm,
-			 struct mmu_interval_notifier *notifier,
-			 unsigned long pages_start, unsigned long pages_end,
-			 const struct drm_gpusvm_ctx *ctx)
+static int drm_gpusvm_dma_map_pages(struct drm_gpusvm *gpusvm,
+				    struct drm_gpusvm_pages *svm_pages,
+				    unsigned long *pfns,
+				    unsigned long npages,
+				    const struct drm_gpusvm_ctx *ctx,
+				    enum dma_data_direction dma_dir)
 {
-	struct hmm_range hmm_range = {
-		.default_flags = HMM_PFN_REQ_FAULT | (ctx->read_only ? 0 :
-			HMM_PFN_REQ_WRITE),
-		.notifier = notifier,
-		.start = pages_start,
-		.end = pages_end,
-		.dev_private_owner = ctx->device_private_page_owner,
-	};
-	void *zdd;
-	unsigned long timeout =
-		jiffies + msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);
+	void *zdd = NULL;
 	unsigned long i, j;
-	unsigned long npages = npages_in_range(pages_start, pages_end);
-	unsigned long num_dma_mapped;
+	unsigned long num_dma_mapped = 0;
 	unsigned int order = 0;
-	unsigned long *pfns;
 	int err = 0;
-	struct dev_pagemap *pagemap;
+	struct dev_pagemap *pagemap = NULL;
 	struct drm_pagemap *dpagemap;
 	struct drm_gpusvm_pages_flags flags;
-	enum dma_data_direction dma_dir = ctx->read_only ? DMA_TO_DEVICE :
-							   DMA_BIDIRECTIONAL;
 	struct dma_iova_state *state = &svm_pages->state;
 
-	if (!svm_pages->drm)
-		return -EINVAL;
-
-retry:
-	if (time_after(jiffies, timeout))
-		return -EBUSY;
-
-	hmm_range.notifier_seq = mmu_interval_read_begin(notifier);
-	if (drm_gpusvm_pages_valid_unlocked(gpusvm, svm_pages))
-		goto set_seqno;
-
-	pfns = kvmalloc_array(npages, sizeof(*pfns), GFP_KERNEL);
-	if (!pfns)
-		return -ENOMEM;
-
-	err = drm_gpusvm_hmm_fault(gpusvm, mm, &hmm_range, pfns, timeout);
-	if (err)
-		goto err_free;
-
-	if (!svm_pages->dma_addr) {
-		svm_pages->dma_addr =
-			kvmalloc_objs(*svm_pages->dma_addr, npages);
-		if (!svm_pages->dma_addr) {
-			err = -ENOMEM;
-			goto err_free;
-		}
-	}
-
-	*state = (struct dma_iova_state){};
-	svm_pages->state_offset = 0;
-
-	/*
-	 * Perform all dma mappings under the notifier lock to not
-	 * access freed pages. A notifier will either block on
-	 * the notifier lock or unmap dma.
-	 */
-	drm_gpusvm_notifier_lock(gpusvm);
+	lockdep_assert_held(&gpusvm->notifier_lock);
 
 	flags.__flags = svm_pages->flags.__flags;
-	if (flags.unmapped) {
-		drm_gpusvm_notifier_unlock(gpusvm);
-		err = -EFAULT;
-		goto err_free;
-	}
 
-	if (mmu_interval_read_retry(notifier, hmm_range.notifier_seq)) {
-		drm_gpusvm_notifier_unlock(gpusvm);
-		kvfree(pfns);
-		goto retry;
-	}
-
-	zdd = NULL;
-	pagemap = NULL;
-	num_dma_mapped = 0;
 	for (i = 0, j = 0; i < npages; ++j) {
 		struct page *page = hmm_pfn_to_page(pfns[i]);
 
@@ -1695,17 +1631,114 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
 	/* WRITE_ONCE pairs with READ_ONCE for opportunistic checks */
 	WRITE_ONCE(svm_pages->flags.__flags, flags.__flags);
 
+	return 0;
+
+err_unmap:
+	svm_pages->flags.has_dma_mapping = true;
+	__drm_gpusvm_unmap_pages(gpusvm, svm_pages, num_dma_mapped);
+	return err;
+}
+
+/**
+ * drm_gpusvm_get_pages() - Get pages and populate GPU SVM pages struct
+ * @gpusvm: Pointer to the GPU SVM structure
+ * @svm_pages: The SVM pages to populate. This will contain the dma-addresses
+ * @mm: The mm corresponding to the CPU range
+ * @notifier: The corresponding notifier for the given CPU range
+ * @pages_start: Start CPU address for the pages
+ * @pages_end: End CPU address for the pages (exclusive)
+ * @ctx: GPU SVM context
+ *
+ * This function gets and maps pages for CPU range and ensures they are
+ * mapped for DMA access.
+ *
+ * Return: 0 on success, negative error code on failure.
+ */
+int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
+			 struct drm_gpusvm_pages *svm_pages,
+			 struct mm_struct *mm,
+			 struct mmu_interval_notifier *notifier,
+			 unsigned long pages_start, unsigned long pages_end,
+			 const struct drm_gpusvm_ctx *ctx)
+{
+	struct hmm_range hmm_range = {
+		.default_flags = HMM_PFN_REQ_FAULT | (ctx->read_only ? 0 :
+			HMM_PFN_REQ_WRITE),
+		.notifier = notifier,
+		.start = pages_start,
+		.end = pages_end,
+		.dev_private_owner = ctx->device_private_page_owner,
+	};
+	unsigned long timeout =
+		jiffies + msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);
+	unsigned long npages = npages_in_range(pages_start, pages_end);
+	unsigned long *pfns;
+	int err = 0;
+	enum dma_data_direction dma_dir = ctx->read_only ? DMA_TO_DEVICE :
+							   DMA_BIDIRECTIONAL;
+
+	if (!svm_pages->drm)
+		return -EINVAL;
+
+retry:
+	if (time_after(jiffies, timeout))
+		return -EBUSY;
+
+	hmm_range.notifier_seq = mmu_interval_read_begin(notifier);
+	if (drm_gpusvm_pages_valid_unlocked(gpusvm, svm_pages))
+		goto set_seqno;
+
+	pfns = kvmalloc_array(npages, sizeof(*pfns), GFP_KERNEL);
+	if (!pfns)
+		return -ENOMEM;
+
+	err = drm_gpusvm_hmm_fault(gpusvm, mm, &hmm_range, pfns, timeout);
+	if (err)
+		goto err_free;
+
+	if (!svm_pages->dma_addr) {
+		svm_pages->dma_addr =
+			kvmalloc_objs(*svm_pages->dma_addr, npages);
+		if (!svm_pages->dma_addr) {
+			err = -ENOMEM;
+			goto err_free;
+		}
+	}
+
+	svm_pages->state = (struct dma_iova_state){};
+	svm_pages->state_offset = 0;
+
+	/*
+	 * Perform all dma mappings under the notifier lock to not
+	 * access freed pages. A notifier will either block on
+	 * the notifier lock or unmap dma.
+	 */
+	drm_gpusvm_notifier_lock(gpusvm);
+
+	if (svm_pages->flags.unmapped) {
+		drm_gpusvm_notifier_unlock(gpusvm);
+		err = -EFAULT;
+		goto err_free;
+	}
+
+	if (mmu_interval_read_retry(notifier, hmm_range.notifier_seq)) {
+		drm_gpusvm_notifier_unlock(gpusvm);
+		kvfree(pfns);
+		goto retry;
+	}
+
+	err = drm_gpusvm_dma_map_pages(gpusvm, svm_pages, pfns, npages, ctx,
+				       dma_dir);
 	drm_gpusvm_notifier_unlock(gpusvm);
+	if (err)
+		goto err_free;
+
 	kvfree(pfns);
 set_seqno:
 	svm_pages->notifier_seq = hmm_range.notifier_seq;
 
 	return 0;
 
-err_unmap:
-	svm_pages->flags.has_dma_mapping = true;
-	__drm_gpusvm_unmap_pages(gpusvm, svm_pages, num_dma_mapped);
-	drm_gpusvm_notifier_unlock(gpusvm);
 err_free:
 	kvfree(pfns);
 	if (err == -EAGAIN)
-- 
2.34.1

Re: [RFC PATCH v1 3/5] drm/gpusvm: extract drm_gpusvm_dma_map_pages() helper

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Device memory mappings are permanently leaked on the error path due to deferred assignment of svm_pages->dpagemap
--

commit 09830ce1d99e5366614becf544e5d523ceeb461d
Author: Honglei Huang <honghuan@amd.com>

drm/gpusvm: extract drm_gpusvm_dma_map_pages() helper

This commit moves the per-device DMA mapping loop of drm_gpusvm_get_pages()
into a helper drm_gpusvm_dma_map_pages(). The mapping logic is only moved,
not changed, so there is no functional change.

> diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
> index 3496cb5688365..753361b965cf9 100644
> --- a/drivers/gpu/drm/drm_gpusvm.c
> +++ b/drivers/gpu/drm/drm_gpusvm.c
[ ... ]
> @@ -1695,17 +1631,114 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
>  	/* WRITE_ONCE pairs with READ_ONCE for opportunistic checks */
>  	WRITE_ONCE(svm_pages->flags.__flags, flags.__flags);
>  
> +	return 0;
> +
> +err_unmap:
> +	svm_pages->flags.has_dma_mapping = true;
> +	__drm_gpusvm_unmap_pages(gpusvm, svm_pages, num_dma_mapped);

[Severity: High]
This is a pre-existing issue, but does this code permanently leak device
memory mappings on the error path?

When a device page is successfully mapped in drm_gpusvm_dma_map_pages(),
dpagemap->ops->device_map() is used. However, svm_pages->dpagemap is only
updated at the end of the function upon complete success.

If an error occurs on a subsequent page (such as returning -EOPNOTSUPP
due to encountering a mixed device memory mapping), the code jumps to
err_unmap and calls __drm_gpusvm_unmap_pages(). 

Because svm_pages->dpagemap is still NULL at this point, the unmap helper
will skip the cleanup callback:

drivers/gpu/drm/drm_gpusvm.c:__drm_gpusvm_unmap_pages() {
    ...
    struct drm_pagemap *dpagemap = svm_pages->dpagemap; 
    ...
    else if (dpagemap && dpagemap->ops->device_unmap)
        dpagemap->ops->device_unmap(...)
    ...
}

Should the local dpagemap be temporarily assigned to svm_pages->dpagemap
before calling the unmap helper, or is there another way to ensure these
partially mapped pages are properly cleaned up?

> +	return err;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827071449.520398-1-honghuan@amd.com?part=3

[RFC PATCH v1 4/5] drm/gpusvm: let drm_gpusvm_get_pages() map an array of pages

[Honglei Huang]

With the N:1 drm_gpusvm_pages layout, one CPU range mirrored on several
drm_devices, the caller had to invoke get_pages() once per device and
repeat the HMM fault every time.

Make get_pages() take a contiguous array of drm_gpusvm_pages plus a
count: fault once through drm_gpusvm_hmm_fault(), then DMA map each
instance by drm_gpusvm_dma_map_pages() under a single read_retry gate.
xe range and userptr callers are updated.

Document the N:1 array usage in the Overview, showing how get_pages()
and drm_gpusvm_range_set_unmapped() take the whole array and its count
while the unmap and free paths stay per-instance.

Suggested-by: Matthew Brost <matthew.brost@intel.com>
Signed-off-by: Honglei Huang <honghuan@amd.com>
---
 drivers/gpu/drm/drm_gpusvm.c    | 101 ++++++++++++++++++++++++--------
 drivers/gpu/drm/xe/xe_svm.c     |   2 +-
 drivers/gpu/drm/xe/xe_userptr.c |   2 +-
 include/drm/drm_gpusvm.h        |   1 +
 4 files changed, 80 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
index 753361b965cf..42e606b94681 100644
--- a/drivers/gpu/drm/drm_gpusvm.c
+++ b/drivers/gpu/drm/drm_gpusvm.c
@@ -89,6 +89,27 @@
  *	Each drm_gpusvm_pages must be zero-initialised and initialised with
  *	drm_gpusvm_init_pages(), called once per entry.
  *
+ *	The 1:1 examples below pass @num_pages == 1 and &drange->pages. In the
+ *	N:1 case the driver instead passes the whole array and its count, so a
+ *	single call faults the CPU range once and DMA maps it for every owning
+ *	drm_device, e.g.:
+ *
+ *	.. code-block:: c
+ *
+ *		// GPU fault handler: one fault, one DMA mapping per device
+ *		err = drm_gpusvm_get_pages(gpusvm, drange->pages,
+ *					   drange->num_pages, gpusvm->mm,
+ *					   &range->notifier->notifier,
+ *					   drm_gpusvm_range_start(range),
+ *					   drm_gpusvm_range_end(range), &ctx);
+ *
+ *		// Notifier callback: mark every instance unmapped in one call
+ *		drm_gpusvm_range_set_unmapped(range, drange->pages,
+ *					      drange->num_pages, mmu_range);
+ *
+ *	The unmap and free paths stay per-instance: iterate @num_pages and call
+ *	drm_gpusvm_unmap_pages() / drm_gpusvm_free_pages() for each entry.
+ *
  * - Operations:
  *	Define the interface for driver-specific GPU SVM operations such as
  *	range allocation, notifier allocation, and invalidations.
@@ -232,7 +253,7 @@
  *				goto retry;
  *		}
  *
- *		err = drm_gpusvm_get_pages(gpusvm, &drange->pages,
+ *		err = drm_gpusvm_get_pages(gpusvm, &drange->pages, 1,
  *					   gpusvm->mm, &range->notifier->notifier,
  *					   drm_gpusvm_range_start(range),
  *					   drm_gpusvm_range_end(range), &ctx);
@@ -1642,20 +1663,26 @@ static int drm_gpusvm_dma_map_pages(struct drm_gpusvm *gpusvm,
 /**
  * drm_gpusvm_get_pages() - Get pages and populate GPU SVM pages struct
  * @gpusvm: Pointer to the GPU SVM structure
- * @svm_pages: The SVM pages to populate. This will contain the dma-addresses
+ * @svm_pages: Array of SVM pages instances to populate with dma addresses
+ * @num_pages: Number of drm_gpusvm_pages instances in @svm_pages
  * @mm: The mm corresponding to the CPU range
  * @notifier: The corresponding notifier for the given CPU range
  * @pages_start: Start CPU address for the pages
  * @pages_end: End CPU address for the pages (exclusive)
  * @ctx: GPU SVM context
  *
- * This function gets and maps pages for CPU range and ensures they are
- * mapped for DMA access.
+ * This function gets and maps pages for a CPU range and ensures they are
+ * mapped for DMA access. The HMM fault for the CPU range is performed once
+ * by drm_gpusvm_hmm_fault(). The DMA mapping by drm_gpusvm_dma_map_pages()
+ * is then done per instance, one per owning drm_device. The retry against
+ * notifier races is kept here in common code so drivers never open code it.
+ * The common 1:1 case passes @num_pages == 1.
  *
  * Return: 0 on success, negative error code on failure.
  */
 int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
 			 struct drm_gpusvm_pages *svm_pages,
+			 unsigned int num_pages,
 			 struct mm_struct *mm,
 			 struct mmu_interval_notifier *notifier,
 			 unsigned long pages_start, unsigned long pages_end,
@@ -1676,16 +1703,24 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
 	int err = 0;
 	enum dma_data_direction dma_dir = ctx->read_only ? DMA_TO_DEVICE :
 							   DMA_BIDIRECTIONAL;
+	unsigned int p;
+	bool all_valid;
 
-	if (!svm_pages->drm)
-		return -EINVAL;
+	for (p = 0; p < num_pages; ++p)
+		if (!svm_pages[p].drm)
+			return -EINVAL;
 
 retry:
 	if (time_after(jiffies, timeout))
 		return -EBUSY;
 
 	hmm_range.notifier_seq = mmu_interval_read_begin(notifier);
-	if (drm_gpusvm_pages_valid_unlocked(gpusvm, svm_pages))
+
+	all_valid = true;
+	for (p = 0; p < num_pages; ++p)
+		if (!drm_gpusvm_pages_valid_unlocked(gpusvm, &svm_pages[p]))
+			all_valid = false;
+	if (all_valid)
 		goto set_seqno;
 
 	pfns = kvmalloc_array(npages, sizeof(*pfns), GFP_KERNEL);
@@ -1696,18 +1731,19 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
 	if (err)
 		goto err_free;
 
-	if (!svm_pages->dma_addr) {
-		svm_pages->dma_addr =
-			kvmalloc_objs(*svm_pages->dma_addr, npages);
-		if (!svm_pages->dma_addr) {
+	for (p = 0; p < num_pages; ++p) {
+		if (svm_pages[p].dma_addr)
+			continue;
+		svm_pages[p].dma_addr =
+			kvmalloc_objs(*svm_pages[p].dma_addr, npages);
+		if (!svm_pages[p].dma_addr) {
 			err = -ENOMEM;
 			goto err_free;
 		}
+		svm_pages[p].state = (struct dma_iova_state){};
+		svm_pages[p].state_offset = 0;
 	}
 
-	svm_pages->state = (struct dma_iova_state){};
-	svm_pages->state_offset = 0;
-
 	/*
 	 * Perform all dma mappings under the notifier lock to not
 	 * access freed pages. A notifier will either block on
@@ -1715,10 +1751,12 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
 	 */
 	drm_gpusvm_notifier_lock(gpusvm);
 
-	if (svm_pages->flags.unmapped) {
-		drm_gpusvm_notifier_unlock(gpusvm);
-		err = -EFAULT;
-		goto err_free;
+	for (p = 0; p < num_pages; ++p) {
+		if (svm_pages[p].flags.unmapped) {
+			drm_gpusvm_notifier_unlock(gpusvm);
+			err = -EFAULT;
+			goto err_free;
+		}
 	}
 
 	if (mmu_interval_read_retry(notifier, hmm_range.notifier_seq)) {
@@ -1727,15 +1765,30 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
 		goto retry;
 	}
 
-	err = drm_gpusvm_dma_map_pages(gpusvm, svm_pages, pfns, npages, ctx,
-				       dma_dir);
-	drm_gpusvm_notifier_unlock(gpusvm);
-	if (err)
-		goto err_free;
+	for (p = 0; p < num_pages; ++p) {
+		if (drm_gpusvm_pages_valid(gpusvm, &svm_pages[p]))
+			continue;
+
+		err = drm_gpusvm_dma_map_pages(gpusvm, &svm_pages[p], pfns,
+					       npages, ctx, dma_dir);
+		if (err) {
+			/*
+			 * drm_gpusvm_dma_map_pages() already cleaned up the
+			 * instance that failed. Leave the earlier ones mapped:
+			 * on -EAGAIN the retry reuses them, on other errors the
+			 * driver frees them with the range. They may also be
+			 * used by other drm_devices, so do not unmap them here.
+			 */
+			drm_gpusvm_notifier_unlock(gpusvm);
+			goto err_free;
+		}
+	}
 
+	drm_gpusvm_notifier_unlock(gpusvm);
 	kvfree(pfns);
 set_seqno:
-	svm_pages->notifier_seq = hmm_range.notifier_seq;
+	for (p = 0; p < num_pages; ++p)
+		svm_pages[p].notifier_seq = hmm_range.notifier_seq;
 
 	return 0;
 
diff --git a/drivers/gpu/drm/xe/xe_svm.c b/drivers/gpu/drm/xe/xe_svm.c
index 627a741293d5..1c7793d8caa8 100644
--- a/drivers/gpu/drm/xe/xe_svm.c
+++ b/drivers/gpu/drm/xe/xe_svm.c
@@ -1598,7 +1598,7 @@ int xe_svm_range_get_pages(struct xe_vm *vm, struct xe_svm_range *range,
 
 	lockdep_assert_held(&range->lock);
 
-	err = drm_gpusvm_get_pages(&vm->svm.gpusvm, &range->pages,
+	err = drm_gpusvm_get_pages(&vm->svm.gpusvm, &range->pages, 1,
 				   vm->svm.gpusvm.mm,
 				   &range->base.notifier->notifier,
 				   drm_gpusvm_range_start(&range->base),
diff --git a/drivers/gpu/drm/xe/xe_userptr.c b/drivers/gpu/drm/xe/xe_userptr.c
index 90ac141fc12d..9c1dac0fce6f 100644
--- a/drivers/gpu/drm/xe/xe_userptr.c
+++ b/drivers/gpu/drm/xe/xe_userptr.c
@@ -91,7 +91,7 @@ int xe_vma_userptr_pin_pages(struct xe_userptr_vma *uvma)
 	if (vma->gpuva.flags & XE_VMA_DESTROYED)
 		return 0;
 
-	return drm_gpusvm_get_pages(&vm->svm.gpusvm, &uvma->userptr.pages,
+	return drm_gpusvm_get_pages(&vm->svm.gpusvm, &uvma->userptr.pages, 1,
 				    uvma->userptr.notifier.mm,
 				    &uvma->userptr.notifier,
 				    xe_vma_userptr(vma),
diff --git a/include/drm/drm_gpusvm.h b/include/drm/drm_gpusvm.h
index b7d987bf76aa..d2b6f3d2b842 100644
--- a/include/drm/drm_gpusvm.h
+++ b/include/drm/drm_gpusvm.h
@@ -324,6 +324,7 @@ void drm_gpusvm_range_set_unmapped(struct drm_gpusvm_range *range,
 
 int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
 			 struct drm_gpusvm_pages *svm_pages,
+			 unsigned int num_pages,
 			 struct mm_struct *mm,
 			 struct mmu_interval_notifier *notifier,
 			 unsigned long pages_start, unsigned long pages_end,
-- 
2.34.1

Re: [RFC PATCH v1 4/5] drm/gpusvm: let drm_gpusvm_get_pages() map an array of pages

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Device memory mappings are leaked when drm_gpusvm_dma_map_pages() fails midway.
--

commit 9ac7ddc26000d29a77c97e82c04194e8d027c3ec
Author: Honglei Huang <honghuan@amd.com>

drm/gpusvm: let drm_gpusvm_get_pages() map an array of pages

This patch updates drm_gpusvm_get_pages() to take a contiguous array of
drm_gpusvm_pages plus a count. It performs the HMM fault once for the CPU
range and then DMA maps each instance per owning drm_device.

> diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
> --- a/drivers/gpu/drm/drm_gpusvm.c
> +++ b/drivers/gpu/drm/drm_gpusvm.c
[ ... ]
> @@ -1727,15 +1765,30 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
>  		goto retry;
>  	}
>  
> -	err = drm_gpusvm_dma_map_pages(gpusvm, svm_pages, pfns, npages, ctx,
> -				       dma_dir);
> -	drm_gpusvm_notifier_unlock(gpusvm);
> -	if (err)
> -		goto err_free;
> +	for (p = 0; p < num_pages; ++p) {
> +		if (drm_gpusvm_pages_valid(gpusvm, &svm_pages[p]))
> +			continue;
> +
> +		err = drm_gpusvm_dma_map_pages(gpusvm, &svm_pages[p], pfns,
> +					       npages, ctx, dma_dir);

[Severity: High]
This is a pre-existing issue, but I noticed a potential leak in how
drm_gpusvm_dma_map_pages() handles its error path.

If drm_gpusvm_dma_map_pages() maps one or more device pages successfully but a
subsequent page fails (for example, yielding -EOPNOTSUPP due to mixed memory,
or -EFAULT from dma_mapping_error), it jumps to err_unmap:

drm_gpusvm_dma_map_pages() {
    ...
err_unmap:
    svm_pages->flags.has_dma_mapping = true;
    __drm_gpusvm_unmap_pages(gpusvm, svm_pages, num_dma_mapped);
    return err;
}

The rollback calls __drm_gpusvm_unmap_pages(), which relies on
svm_pages->dpagemap to invoke device_unmap:

__drm_gpusvm_unmap_pages() {
    ...
            else if (dpagemap && dpagemap->ops->device_unmap)
                dpagemap->ops->device_unmap(dpagemap, dev, addr);
    ...
}

However, svm_pages->dpagemap is only updated at the very end of
drm_gpusvm_dma_map_pages() upon success. During the error path, it is still
NULL.

Can this result in __drm_gpusvm_unmap_pages() silently skipping the unmapping
of all previously mapped device pages, thereby leaking device mapping
resources?

> +		if (err) {
> +			/*
> +			 * drm_gpusvm_dma_map_pages() already cleaned up the
> +			 * instance that failed. Leave the earlier ones mapped:
> +			 * on -EAGAIN the retry reuses them, on other errors the
> +			 * driver frees them with the range. They may also be
> +			 * used by other drm_devices, so do not unmap them here.
> +			 */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827071449.520398-1-honghuan@amd.com?part=4

[RFC PATCH v1 5/5] drm/gpusvm: make the DMA mapping step in get_pages() optional

[Honglei Huang]

Some drivers (e.g. AMDXDNA) only need the CPU pages faulted in and tracked
by the notifier, no need DMA mapping.

Add a drm_gpusvm_ctx::no_dma_map flag. When set, get_pages() does the
shared HMM fault and records notifier_seq, but skips svm_pages->drm
validation, the dma_addr allocation and drm_gpusvm_dma_map_pages().
With no mapping state to check, the fault is redone on every call. The
default (no_dma_map == 0) is unchanged.

Suggested-by: Matthew Brost <matthew.brost@intel.com>
Signed-off-by: Honglei Huang <honghuan@amd.com>
---
 drivers/gpu/drm/drm_gpusvm.c | 64 ++++++++++++++++++++++++++----------
 include/drm/drm_gpusvm.h     |  5 +++
 2 files changed, 51 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
index 42e606b94681..c89ff4a9d081 100644
--- a/drivers/gpu/drm/drm_gpusvm.c
+++ b/drivers/gpu/drm/drm_gpusvm.c
@@ -1703,11 +1703,11 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
 	int err = 0;
 	enum dma_data_direction dma_dir = ctx->read_only ? DMA_TO_DEVICE :
 							   DMA_BIDIRECTIONAL;
+	const bool map_dma = !ctx->no_dma_map;
 	unsigned int p;
-	bool all_valid;
 
 	for (p = 0; p < num_pages; ++p)
-		if (!svm_pages[p].drm)
+		if (map_dma && !svm_pages[p].drm)
 			return -EINVAL;
 
 retry:
@@ -1716,12 +1716,24 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
 
 	hmm_range.notifier_seq = mmu_interval_read_begin(notifier);
 
-	all_valid = true;
-	for (p = 0; p < num_pages; ++p)
-		if (!drm_gpusvm_pages_valid_unlocked(gpusvm, &svm_pages[p]))
-			all_valid = false;
-	if (all_valid)
-		goto set_seqno;
+	/*
+	 * The HMM fault is shared by all the drm_gpusvm_pages instances (they
+	 * all mirror the same CPU range); only the DMA mapping below is
+	 * per-instance. In no_dma_map mode there is no DMA mapping state to
+	 * validate, so the fault is always redone. Otherwise skip the fault
+	 * entirely if every instance is already valid.
+	 * drm_gpusvm_pages_valid_unlocked() also drops the stale dma_addr array
+	 * of any instance that is no longer valid.
+	 */
+	if (map_dma) {
+		bool all_valid = true;
+
+		for (p = 0; p < num_pages; ++p)
+			if (!drm_gpusvm_pages_valid_unlocked(gpusvm, &svm_pages[p]))
+				all_valid = false;
+		if (all_valid)
+			goto set_seqno;
+	}
 
 	pfns = kvmalloc_array(npages, sizeof(*pfns), GFP_KERNEL);
 	if (!pfns)
@@ -1731,17 +1743,24 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
 	if (err)
 		goto err_free;
 
-	for (p = 0; p < num_pages; ++p) {
-		if (svm_pages[p].dma_addr)
-			continue;
-		svm_pages[p].dma_addr =
-			kvmalloc_objs(*svm_pages[p].dma_addr, npages);
-		if (!svm_pages[p].dma_addr) {
-			err = -ENOMEM;
-			goto err_free;
+	/*
+	 * Allocate the dma_addr array of each instance outside the notifier
+	 * lock. A still-valid instance keeps its existing dma_addr array and
+	 * is not reallocated. Skipped entirely in no_dma_map mode.
+	 */
+	if (map_dma) {
+		for (p = 0; p < num_pages; ++p) {
+			if (svm_pages[p].dma_addr)
+				continue;
+			svm_pages[p].dma_addr =
+				kvmalloc_objs(*svm_pages[p].dma_addr, npages);
+			if (!svm_pages[p].dma_addr) {
+				err = -ENOMEM;
+				goto err_free;
+			}
+			svm_pages[p].state = (struct dma_iova_state){};
+			svm_pages[p].state_offset = 0;
 		}
-		svm_pages[p].state = (struct dma_iova_state){};
-		svm_pages[p].state_offset = 0;
 	}
 
 	/*
@@ -1765,6 +1784,14 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
 		goto retry;
 	}
 
+	/*
+	 * no_dma_map: the caller only needs the HMM fault, not a device DMA
+	 * mapping. The fault has been validated under the notifier lock
+	 * above; skip the per-instance DMA mapping entirely.
+	 */
+	if (!map_dma)
+		goto done_mapping;
+
 	for (p = 0; p < num_pages; ++p) {
 		if (drm_gpusvm_pages_valid(gpusvm, &svm_pages[p]))
 			continue;
@@ -1784,6 +1811,7 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
 		}
 	}
 
+done_mapping:
 	drm_gpusvm_notifier_unlock(gpusvm);
 	kvfree(pfns);
 set_seqno:
diff --git a/include/drm/drm_gpusvm.h b/include/drm/drm_gpusvm.h
index d2b6f3d2b842..84db209cf148 100644
--- a/include/drm/drm_gpusvm.h
+++ b/include/drm/drm_gpusvm.h
@@ -254,6 +254,10 @@ struct drm_gpusvm {
  * @allow_mixed: Allow mixed mappings in get pages. Mixing between system and
  *               single dpagemap is supported, mixing between multiple dpagemap
  *               is unsupported.
+ * @no_dma_map: Only fault the CPU pages for the range; skip the device DMA
+ *              mapping step. Used by drivers (e.g. AMDXDNA userptr) that
+ *              consume the faulted pages without needing a DMA mapping. In
+ *              this mode @drm on the drm_gpusvm_pages is not required.
  *
  * Context that is DRM GPUSVM is operating in (i.e. user arguments).
  */
@@ -266,6 +270,7 @@ struct drm_gpusvm_ctx {
 	unsigned int devmem_possible :1;
 	unsigned int devmem_only :1;
 	unsigned int allow_mixed :1;
+	unsigned int no_dma_map :1;
 };
 
 int drm_gpusvm_init(struct drm_gpusvm *gpusvm,
-- 
2.34.1