* [PATCH v2] drm/gpusvm, drm/xe: Fix userptr to not allow device private pages
@ 2025-09-30 7:21 Thomas Hellström
2025-09-30 11:16 ` Matthew Auld
0 siblings, 1 reply; 6+ messages in thread
From: Thomas Hellström @ 2025-09-30 7:21 UTC (permalink / raw)
To: intel-xe
Cc: Thomas Hellström, Matthew Auld, Himal Prasad Ghimiray,
Matthew Brost
When userptr is used on SVM-enabled VMs, a non-NULL
hmm_range::dev_private_owner value might mean that
hmm_range_fault() attempts to return device private pages.
Either that will fail, or the userptr code will not know
how to handle those.
Use NULL for hmm_range::dev_private_owner to migrate
such pages to system. In order to do that, move the
struct drm_gpusvm::device_private_page_owner field to
struct drm_gpusvm_ctx::device_private_page_owner so that
it doesn't remain immutable over the drm_gpusvm lifetime.
v2:
- Don't conditionally compile xe_svm_devm_owner().
- Kerneldoc xe_svm_devm_owner().
Fixes: 9e9787414882 ("drm/xe/userptr: replace xe_hmm with gpusvm")
Cc: Matthew Auld <matthew.auld@intel.com>
Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
drivers/gpu/drm/drm_gpusvm.c | 24 +++++++++++++-----------
drivers/gpu/drm/xe/xe_svm.c | 11 +++--------
drivers/gpu/drm/xe/xe_svm.h | 14 ++++++++++++++
drivers/gpu/drm/xe/xe_userptr.c | 1 +
drivers/gpu/drm/xe/xe_vm.c | 1 +
include/drm/drm_gpusvm.h | 7 ++++---
6 files changed, 36 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
index eeeeb99cfdf6..cb906765897e 100644
--- a/drivers/gpu/drm/drm_gpusvm.c
+++ b/drivers/gpu/drm/drm_gpusvm.c
@@ -361,7 +361,6 @@ static const struct mmu_interval_notifier_ops drm_gpusvm_notifier_ops = {
* @name: Name of the GPU SVM.
* @drm: Pointer to the DRM device structure.
* @mm: Pointer to the mm_struct for the address space.
- * @device_private_page_owner: Device private pages owner.
* @mm_start: Start address of GPU SVM.
* @mm_range: Range of the GPU SVM.
* @notifier_size: Size of individual notifiers.
@@ -383,7 +382,7 @@ static const struct mmu_interval_notifier_ops drm_gpusvm_notifier_ops = {
*/
int drm_gpusvm_init(struct drm_gpusvm *gpusvm,
const char *name, struct drm_device *drm,
- struct mm_struct *mm, void *device_private_page_owner,
+ struct mm_struct *mm,
unsigned long mm_start, unsigned long mm_range,
unsigned long notifier_size,
const struct drm_gpusvm_ops *ops,
@@ -395,15 +394,13 @@ int drm_gpusvm_init(struct drm_gpusvm *gpusvm,
mmgrab(mm);
} else {
/* No full SVM mode, only core drm_gpusvm_pages API. */
- if (ops || num_chunks || mm_range || notifier_size ||
- device_private_page_owner)
+ if (ops || num_chunks || mm_range || notifier_size)
return -EINVAL;
}
gpusvm->name = name;
gpusvm->drm = drm;
gpusvm->mm = mm;
- gpusvm->device_private_page_owner = device_private_page_owner;
gpusvm->mm_start = mm_start;
gpusvm->mm_range = mm_range;
gpusvm->notifier_size = notifier_size;
@@ -684,6 +681,7 @@ static unsigned int drm_gpusvm_hmm_pfn_to_order(unsigned long hmm_pfn,
* @notifier: Pointer to the GPU SVM notifier structure
* @start: Start address
* @end: End address
+ * @dev_private_owner: The device private page owner
*
* Check if pages between start and end have been faulted in on the CPU. Use to
* prevent migration of pages without CPU backing store.
@@ -692,14 +690,15 @@ static unsigned int drm_gpusvm_hmm_pfn_to_order(unsigned long hmm_pfn,
*/
static bool drm_gpusvm_check_pages(struct drm_gpusvm *gpusvm,
struct drm_gpusvm_notifier *notifier,
- unsigned long start, unsigned long end)
+ unsigned long start, unsigned long end,
+ void *dev_private_owner)
{
struct hmm_range hmm_range = {
.default_flags = 0,
.notifier = ¬ifier->notifier,
.start = start,
.end = end,
- .dev_private_owner = gpusvm->device_private_page_owner,
+ .dev_private_owner = dev_private_owner,
};
unsigned long timeout =
jiffies + msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);
@@ -753,6 +752,7 @@ static bool drm_gpusvm_check_pages(struct drm_gpusvm *gpusvm,
* @gpuva_start: Start address of GPUVA which mirrors CPU
* @gpuva_end: End address of GPUVA which mirrors CPU
* @check_pages_threshold: Check CPU pages for present threshold
+ * @dev_private_owner: The device private page owner
*
* This function determines the chunk size for the GPU SVM range based on the
* fault address, GPU SVM chunk sizes, existing GPU SVM ranges, and the virtual
@@ -767,7 +767,8 @@ drm_gpusvm_range_chunk_size(struct drm_gpusvm *gpusvm,
unsigned long fault_addr,
unsigned long gpuva_start,
unsigned long gpuva_end,
- unsigned long check_pages_threshold)
+ unsigned long check_pages_threshold,
+ void *dev_private_owner)
{
unsigned long start, end;
int i = 0;
@@ -814,7 +815,7 @@ drm_gpusvm_range_chunk_size(struct drm_gpusvm *gpusvm,
* process-many-malloc' mallocs at least 64k at a time.
*/
if (end - start <= check_pages_threshold &&
- !drm_gpusvm_check_pages(gpusvm, notifier, start, end)) {
+ !drm_gpusvm_check_pages(gpusvm, notifier, start, end, dev_private_owner)) {
++i;
goto retry;
}
@@ -957,7 +958,8 @@ drm_gpusvm_range_find_or_insert(struct drm_gpusvm *gpusvm,
chunk_size = drm_gpusvm_range_chunk_size(gpusvm, notifier, vas,
fault_addr, gpuva_start,
gpuva_end,
- ctx->check_pages_threshold);
+ ctx->check_pages_threshold,
+ ctx->device_private_page_owner);
if (chunk_size == LONG_MAX) {
err = -EINVAL;
goto err_notifier_remove;
@@ -1268,7 +1270,7 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
.notifier = notifier,
.start = pages_start,
.end = pages_end,
- .dev_private_owner = gpusvm->device_private_page_owner,
+ .dev_private_owner = ctx->device_private_page_owner,
};
void *zdd;
unsigned long timeout =
diff --git a/drivers/gpu/drm/xe/xe_svm.c b/drivers/gpu/drm/xe/xe_svm.c
index 7f2f1f041f1d..7e2db71ff34e 100644
--- a/drivers/gpu/drm/xe/xe_svm.c
+++ b/drivers/gpu/drm/xe/xe_svm.c
@@ -67,11 +67,6 @@ void xe_svm_range_debug(struct xe_svm_range *range, const char *operation)
range_debug(range, operation);
}
-static void *xe_svm_devm_owner(struct xe_device *xe)
-{
- return xe;
-}
-
static struct drm_gpusvm_range *
xe_svm_range_alloc(struct drm_gpusvm *gpusvm)
{
@@ -744,15 +739,14 @@ int xe_svm_init(struct xe_vm *vm)
xe_svm_garbage_collector_work_func);
err = drm_gpusvm_init(&vm->svm.gpusvm, "Xe SVM", &vm->xe->drm,
- current->mm, xe_svm_devm_owner(vm->xe), 0,
- vm->size,
+ current->mm, 0, vm->size,
xe_modparam.svm_notifier_size * SZ_1M,
&gpusvm_ops, fault_chunk_sizes,
ARRAY_SIZE(fault_chunk_sizes));
drm_gpusvm_driver_set_lock(&vm->svm.gpusvm, &vm->lock);
} else {
err = drm_gpusvm_init(&vm->svm.gpusvm, "Xe SVM (simple)",
- &vm->xe->drm, NULL, NULL, 0, 0, 0, NULL,
+ &vm->xe->drm, NULL, 0, 0, 0, NULL,
NULL, 0);
}
@@ -1017,6 +1011,7 @@ static int __xe_svm_handle_pagefault(struct xe_vm *vm, struct xe_vma *vma,
.devmem_only = need_vram && devmem_possible,
.timeslice_ms = need_vram && devmem_possible ?
vm->xe->atomic_svm_timeslice_ms : 0,
+ .device_private_page_owner = xe_svm_devm_owner(vm->xe),
};
struct xe_validation_ctx vctx;
struct drm_exec exec;
diff --git a/drivers/gpu/drm/xe/xe_svm.h b/drivers/gpu/drm/xe/xe_svm.h
index cef6ee7d6fe3..0955d2ac8d74 100644
--- a/drivers/gpu/drm/xe/xe_svm.h
+++ b/drivers/gpu/drm/xe/xe_svm.h
@@ -6,6 +6,20 @@
#ifndef _XE_SVM_H_
#define _XE_SVM_H_
+struct xe_device;
+
+/**
+ * xe_svm_devm_owner() - Return the owner of device private memory
+ * @xe: The xe device.
+ *
+ * Return: The owner of this device's device private memory to use in
+ * hmm_range_fault()-
+ */
+static inline void *xe_svm_devm_owner(struct xe_device *xe)
+{
+ return xe;
+}
+
#if IS_ENABLED(CONFIG_DRM_XE_GPUSVM)
#include <drm/drm_pagemap.h>
diff --git a/drivers/gpu/drm/xe/xe_userptr.c b/drivers/gpu/drm/xe/xe_userptr.c
index 91d09af71ced..f16e92cd8090 100644
--- a/drivers/gpu/drm/xe/xe_userptr.c
+++ b/drivers/gpu/drm/xe/xe_userptr.c
@@ -54,6 +54,7 @@ int xe_vma_userptr_pin_pages(struct xe_userptr_vma *uvma)
struct xe_device *xe = vm->xe;
struct drm_gpusvm_ctx ctx = {
.read_only = xe_vma_read_only(vma),
+ .device_private_page_owner = NULL,
};
lockdep_assert_held(&vm->lock);
diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
index 80b7f13ecd80..4e914928e0a9 100644
--- a/drivers/gpu/drm/xe/xe_vm.c
+++ b/drivers/gpu/drm/xe/xe_vm.c
@@ -2883,6 +2883,7 @@ static int prefetch_ranges(struct xe_vm *vm, struct xe_vma_op *op)
ctx.read_only = xe_vma_read_only(vma);
ctx.devmem_possible = devmem_possible;
ctx.check_pages_threshold = devmem_possible ? SZ_64K : 0;
+ ctx.device_private_page_owner = xe_svm_devm_owner(vm->xe);
/* TODO: Threading the migration */
xa_for_each(&op->prefetch_range.range, i, svm_range) {
diff --git a/include/drm/drm_gpusvm.h b/include/drm/drm_gpusvm.h
index 5434048a2ca4..b92faa9a26b2 100644
--- a/include/drm/drm_gpusvm.h
+++ b/include/drm/drm_gpusvm.h
@@ -179,7 +179,6 @@ struct drm_gpusvm_range {
* @name: Name of the GPU SVM
* @drm: Pointer to the DRM device structure
* @mm: Pointer to the mm_struct for the address space
- * @device_private_page_owner: Device private pages owner
* @mm_start: Start address of GPU SVM
* @mm_range: Range of the GPU SVM
* @notifier_size: Size of individual notifiers
@@ -204,7 +203,6 @@ struct drm_gpusvm {
const char *name;
struct drm_device *drm;
struct mm_struct *mm;
- void *device_private_page_owner;
unsigned long mm_start;
unsigned long mm_range;
unsigned long notifier_size;
@@ -226,6 +224,8 @@ struct drm_gpusvm {
/**
* struct drm_gpusvm_ctx - DRM GPU SVM context
*
+ * @device_private_page_owner: The device-private page owner to use for
+ * this operation
* @check_pages_threshold: Check CPU pages for present if chunk is less than or
* equal to threshold. If not present, reduce chunk
* size.
@@ -239,6 +239,7 @@ struct drm_gpusvm {
* Context that is DRM GPUSVM is operating in (i.e. user arguments).
*/
struct drm_gpusvm_ctx {
+ void *device_private_page_owner;
unsigned long check_pages_threshold;
unsigned long timeslice_ms;
unsigned int in_notifier :1;
@@ -249,7 +250,7 @@ struct drm_gpusvm_ctx {
int drm_gpusvm_init(struct drm_gpusvm *gpusvm,
const char *name, struct drm_device *drm,
- struct mm_struct *mm, void *device_private_page_owner,
+ struct mm_struct *mm,
unsigned long mm_start, unsigned long mm_range,
unsigned long notifier_size,
const struct drm_gpusvm_ops *ops,
--
2.51.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] drm/gpusvm, drm/xe: Fix userptr to not allow device private pages
2025-09-30 7:21 Thomas Hellström
@ 2025-09-30 11:16 ` Matthew Auld
2025-09-30 12:26 ` Thomas Hellström
0 siblings, 1 reply; 6+ messages in thread
From: Matthew Auld @ 2025-09-30 11:16 UTC (permalink / raw)
To: Thomas Hellström, intel-xe; +Cc: Himal Prasad Ghimiray, Matthew Brost
On 30/09/2025 08:21, Thomas Hellström wrote:
> When userptr is used on SVM-enabled VMs, a non-NULL
> hmm_range::dev_private_owner value might mean that
> hmm_range_fault() attempts to return device private pages.
> Either that will fail, or the userptr code will not know
> how to handle those.
>
> Use NULL for hmm_range::dev_private_owner to migrate
> such pages to system. In order to do that, move the
> struct drm_gpusvm::device_private_page_owner field to
> struct drm_gpusvm_ctx::device_private_page_owner so that
> it doesn't remain immutable over the drm_gpusvm lifetime.
>
> v2:
> - Don't conditionally compile xe_svm_devm_owner().
> - Kerneldoc xe_svm_devm_owner().
>
> Fixes: 9e9787414882 ("drm/xe/userptr: replace xe_hmm with gpusvm")
> Cc: Matthew Auld <matthew.auld@intel.com>
> Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
So in theory this should give the same behavior pre gpusvm conversion
for userptr?
I guess missing Cc: dri-devel ?
Reviewed-by: Matthew Auld <matthew.auld@intel.com>
> ---
> drivers/gpu/drm/drm_gpusvm.c | 24 +++++++++++++-----------
> drivers/gpu/drm/xe/xe_svm.c | 11 +++--------
> drivers/gpu/drm/xe/xe_svm.h | 14 ++++++++++++++
> drivers/gpu/drm/xe/xe_userptr.c | 1 +
> drivers/gpu/drm/xe/xe_vm.c | 1 +
> include/drm/drm_gpusvm.h | 7 ++++---
> 6 files changed, 36 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
> index eeeeb99cfdf6..cb906765897e 100644
> --- a/drivers/gpu/drm/drm_gpusvm.c
> +++ b/drivers/gpu/drm/drm_gpusvm.c
> @@ -361,7 +361,6 @@ static const struct mmu_interval_notifier_ops drm_gpusvm_notifier_ops = {
> * @name: Name of the GPU SVM.
> * @drm: Pointer to the DRM device structure.
> * @mm: Pointer to the mm_struct for the address space.
> - * @device_private_page_owner: Device private pages owner.
> * @mm_start: Start address of GPU SVM.
> * @mm_range: Range of the GPU SVM.
> * @notifier_size: Size of individual notifiers.
> @@ -383,7 +382,7 @@ static const struct mmu_interval_notifier_ops drm_gpusvm_notifier_ops = {
> */
> int drm_gpusvm_init(struct drm_gpusvm *gpusvm,
> const char *name, struct drm_device *drm,
> - struct mm_struct *mm, void *device_private_page_owner,
> + struct mm_struct *mm,
> unsigned long mm_start, unsigned long mm_range,
> unsigned long notifier_size,
> const struct drm_gpusvm_ops *ops,
> @@ -395,15 +394,13 @@ int drm_gpusvm_init(struct drm_gpusvm *gpusvm,
> mmgrab(mm);
> } else {
> /* No full SVM mode, only core drm_gpusvm_pages API. */
> - if (ops || num_chunks || mm_range || notifier_size ||
> - device_private_page_owner)
> + if (ops || num_chunks || mm_range || notifier_size)
> return -EINVAL;
> }
>
> gpusvm->name = name;
> gpusvm->drm = drm;
> gpusvm->mm = mm;
> - gpusvm->device_private_page_owner = device_private_page_owner;
> gpusvm->mm_start = mm_start;
> gpusvm->mm_range = mm_range;
> gpusvm->notifier_size = notifier_size;
> @@ -684,6 +681,7 @@ static unsigned int drm_gpusvm_hmm_pfn_to_order(unsigned long hmm_pfn,
> * @notifier: Pointer to the GPU SVM notifier structure
> * @start: Start address
> * @end: End address
> + * @dev_private_owner: The device private page owner
> *
> * Check if pages between start and end have been faulted in on the CPU. Use to
> * prevent migration of pages without CPU backing store.
> @@ -692,14 +690,15 @@ static unsigned int drm_gpusvm_hmm_pfn_to_order(unsigned long hmm_pfn,
> */
> static bool drm_gpusvm_check_pages(struct drm_gpusvm *gpusvm,
> struct drm_gpusvm_notifier *notifier,
> - unsigned long start, unsigned long end)
> + unsigned long start, unsigned long end,
> + void *dev_private_owner)
> {
> struct hmm_range hmm_range = {
> .default_flags = 0,
> .notifier = ¬ifier->notifier,
> .start = start,
> .end = end,
> - .dev_private_owner = gpusvm->device_private_page_owner,
> + .dev_private_owner = dev_private_owner,
> };
> unsigned long timeout =
> jiffies + msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);
> @@ -753,6 +752,7 @@ static bool drm_gpusvm_check_pages(struct drm_gpusvm *gpusvm,
> * @gpuva_start: Start address of GPUVA which mirrors CPU
> * @gpuva_end: End address of GPUVA which mirrors CPU
> * @check_pages_threshold: Check CPU pages for present threshold
> + * @dev_private_owner: The device private page owner
> *
> * This function determines the chunk size for the GPU SVM range based on the
> * fault address, GPU SVM chunk sizes, existing GPU SVM ranges, and the virtual
> @@ -767,7 +767,8 @@ drm_gpusvm_range_chunk_size(struct drm_gpusvm *gpusvm,
> unsigned long fault_addr,
> unsigned long gpuva_start,
> unsigned long gpuva_end,
> - unsigned long check_pages_threshold)
> + unsigned long check_pages_threshold,
> + void *dev_private_owner)
> {
> unsigned long start, end;
> int i = 0;
> @@ -814,7 +815,7 @@ drm_gpusvm_range_chunk_size(struct drm_gpusvm *gpusvm,
> * process-many-malloc' mallocs at least 64k at a time.
> */
> if (end - start <= check_pages_threshold &&
> - !drm_gpusvm_check_pages(gpusvm, notifier, start, end)) {
> + !drm_gpusvm_check_pages(gpusvm, notifier, start, end, dev_private_owner)) {
> ++i;
> goto retry;
> }
> @@ -957,7 +958,8 @@ drm_gpusvm_range_find_or_insert(struct drm_gpusvm *gpusvm,
> chunk_size = drm_gpusvm_range_chunk_size(gpusvm, notifier, vas,
> fault_addr, gpuva_start,
> gpuva_end,
> - ctx->check_pages_threshold);
> + ctx->check_pages_threshold,
> + ctx->device_private_page_owner);
> if (chunk_size == LONG_MAX) {
> err = -EINVAL;
> goto err_notifier_remove;
> @@ -1268,7 +1270,7 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
> .notifier = notifier,
> .start = pages_start,
> .end = pages_end,
> - .dev_private_owner = gpusvm->device_private_page_owner,
> + .dev_private_owner = ctx->device_private_page_owner,
> };
> void *zdd;
> unsigned long timeout =
> diff --git a/drivers/gpu/drm/xe/xe_svm.c b/drivers/gpu/drm/xe/xe_svm.c
> index 7f2f1f041f1d..7e2db71ff34e 100644
> --- a/drivers/gpu/drm/xe/xe_svm.c
> +++ b/drivers/gpu/drm/xe/xe_svm.c
> @@ -67,11 +67,6 @@ void xe_svm_range_debug(struct xe_svm_range *range, const char *operation)
> range_debug(range, operation);
> }
>
> -static void *xe_svm_devm_owner(struct xe_device *xe)
> -{
> - return xe;
> -}
> -
> static struct drm_gpusvm_range *
> xe_svm_range_alloc(struct drm_gpusvm *gpusvm)
> {
> @@ -744,15 +739,14 @@ int xe_svm_init(struct xe_vm *vm)
> xe_svm_garbage_collector_work_func);
>
> err = drm_gpusvm_init(&vm->svm.gpusvm, "Xe SVM", &vm->xe->drm,
> - current->mm, xe_svm_devm_owner(vm->xe), 0,
> - vm->size,
> + current->mm, 0, vm->size,
> xe_modparam.svm_notifier_size * SZ_1M,
> &gpusvm_ops, fault_chunk_sizes,
> ARRAY_SIZE(fault_chunk_sizes));
> drm_gpusvm_driver_set_lock(&vm->svm.gpusvm, &vm->lock);
> } else {
> err = drm_gpusvm_init(&vm->svm.gpusvm, "Xe SVM (simple)",
> - &vm->xe->drm, NULL, NULL, 0, 0, 0, NULL,
> + &vm->xe->drm, NULL, 0, 0, 0, NULL,
> NULL, 0);
> }
>
> @@ -1017,6 +1011,7 @@ static int __xe_svm_handle_pagefault(struct xe_vm *vm, struct xe_vma *vma,
> .devmem_only = need_vram && devmem_possible,
> .timeslice_ms = need_vram && devmem_possible ?
> vm->xe->atomic_svm_timeslice_ms : 0,
> + .device_private_page_owner = xe_svm_devm_owner(vm->xe),
> };
> struct xe_validation_ctx vctx;
> struct drm_exec exec;
> diff --git a/drivers/gpu/drm/xe/xe_svm.h b/drivers/gpu/drm/xe/xe_svm.h
> index cef6ee7d6fe3..0955d2ac8d74 100644
> --- a/drivers/gpu/drm/xe/xe_svm.h
> +++ b/drivers/gpu/drm/xe/xe_svm.h
> @@ -6,6 +6,20 @@
> #ifndef _XE_SVM_H_
> #define _XE_SVM_H_
>
> +struct xe_device;
> +
> +/**
> + * xe_svm_devm_owner() - Return the owner of device private memory
> + * @xe: The xe device.
> + *
> + * Return: The owner of this device's device private memory to use in
> + * hmm_range_fault()-
> + */
> +static inline void *xe_svm_devm_owner(struct xe_device *xe)
> +{
> + return xe;
> +}
> +
> #if IS_ENABLED(CONFIG_DRM_XE_GPUSVM)
>
> #include <drm/drm_pagemap.h>
> diff --git a/drivers/gpu/drm/xe/xe_userptr.c b/drivers/gpu/drm/xe/xe_userptr.c
> index 91d09af71ced..f16e92cd8090 100644
> --- a/drivers/gpu/drm/xe/xe_userptr.c
> +++ b/drivers/gpu/drm/xe/xe_userptr.c
> @@ -54,6 +54,7 @@ int xe_vma_userptr_pin_pages(struct xe_userptr_vma *uvma)
> struct xe_device *xe = vm->xe;
> struct drm_gpusvm_ctx ctx = {
> .read_only = xe_vma_read_only(vma),
> + .device_private_page_owner = NULL,
> };
>
> lockdep_assert_held(&vm->lock);
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index 80b7f13ecd80..4e914928e0a9 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -2883,6 +2883,7 @@ static int prefetch_ranges(struct xe_vm *vm, struct xe_vma_op *op)
> ctx.read_only = xe_vma_read_only(vma);
> ctx.devmem_possible = devmem_possible;
> ctx.check_pages_threshold = devmem_possible ? SZ_64K : 0;
> + ctx.device_private_page_owner = xe_svm_devm_owner(vm->xe);
>
> /* TODO: Threading the migration */
> xa_for_each(&op->prefetch_range.range, i, svm_range) {
> diff --git a/include/drm/drm_gpusvm.h b/include/drm/drm_gpusvm.h
> index 5434048a2ca4..b92faa9a26b2 100644
> --- a/include/drm/drm_gpusvm.h
> +++ b/include/drm/drm_gpusvm.h
> @@ -179,7 +179,6 @@ struct drm_gpusvm_range {
> * @name: Name of the GPU SVM
> * @drm: Pointer to the DRM device structure
> * @mm: Pointer to the mm_struct for the address space
> - * @device_private_page_owner: Device private pages owner
> * @mm_start: Start address of GPU SVM
> * @mm_range: Range of the GPU SVM
> * @notifier_size: Size of individual notifiers
> @@ -204,7 +203,6 @@ struct drm_gpusvm {
> const char *name;
> struct drm_device *drm;
> struct mm_struct *mm;
> - void *device_private_page_owner;
> unsigned long mm_start;
> unsigned long mm_range;
> unsigned long notifier_size;
> @@ -226,6 +224,8 @@ struct drm_gpusvm {
> /**
> * struct drm_gpusvm_ctx - DRM GPU SVM context
> *
> + * @device_private_page_owner: The device-private page owner to use for
> + * this operation
> * @check_pages_threshold: Check CPU pages for present if chunk is less than or
> * equal to threshold. If not present, reduce chunk
> * size.
> @@ -239,6 +239,7 @@ struct drm_gpusvm {
> * Context that is DRM GPUSVM is operating in (i.e. user arguments).
> */
> struct drm_gpusvm_ctx {
> + void *device_private_page_owner;
> unsigned long check_pages_threshold;
> unsigned long timeslice_ms;
> unsigned int in_notifier :1;
> @@ -249,7 +250,7 @@ struct drm_gpusvm_ctx {
>
> int drm_gpusvm_init(struct drm_gpusvm *gpusvm,
> const char *name, struct drm_device *drm,
> - struct mm_struct *mm, void *device_private_page_owner,
> + struct mm_struct *mm,
> unsigned long mm_start, unsigned long mm_range,
> unsigned long notifier_size,
> const struct drm_gpusvm_ops *ops,
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] drm/gpusvm, drm/xe: Fix userptr to not allow device private pages
2025-09-30 11:16 ` Matthew Auld
@ 2025-09-30 12:26 ` Thomas Hellström
2025-09-30 22:58 ` Matthew Brost
0 siblings, 1 reply; 6+ messages in thread
From: Thomas Hellström @ 2025-09-30 12:26 UTC (permalink / raw)
To: Matthew Auld, intel-xe; +Cc: Himal Prasad Ghimiray, Matthew Brost
On 9/30/25 13:16, Matthew Auld wrote:
> On 30/09/2025 08:21, Thomas Hellström wrote:
>> When userptr is used on SVM-enabled VMs, a non-NULL
>> hmm_range::dev_private_owner value might mean that
>> hmm_range_fault() attempts to return device private pages.
>> Either that will fail, or the userptr code will not know
>> how to handle those.
>>
>> Use NULL for hmm_range::dev_private_owner to migrate
>> such pages to system. In order to do that, move the
>> struct drm_gpusvm::device_private_page_owner field to
>> struct drm_gpusvm_ctx::device_private_page_owner so that
>> it doesn't remain immutable over the drm_gpusvm lifetime.
>>
>> v2:
>> - Don't conditionally compile xe_svm_devm_owner().
>> - Kerneldoc xe_svm_devm_owner().
>>
>> Fixes: 9e9787414882 ("drm/xe/userptr: replace xe_hmm with gpusvm")
>> Cc: Matthew Auld <matthew.auld@intel.com>
>> Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
>> Cc: Matthew Brost <matthew.brost@intel.com>
>> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>
> So in theory this should give the same behavior pre gpusvm conversion
> for userptr?
Actually not, the same problem is there, but we need to have a patch
that fixes this in Linus' tree before sending a backported patch that
fixes the behaviour before the conversion.
On top of this, we will then as a follow-up actually allow VRAM pages in
userptr with the SVM code modified to handle this, since l0 has a some
use-cases that are not fully ported to SVM yet that would otherwise see
multiple migration blits for the same memory.
>
> I guess missing Cc: dri-devel ?
Right. I'll resend with that CC.
>
> Reviewed-by: Matthew Auld <matthew.auld@intel.com>
Thanks,
Thomas
>
>> ---
>> drivers/gpu/drm/drm_gpusvm.c | 24 +++++++++++++-----------
>> drivers/gpu/drm/xe/xe_svm.c | 11 +++--------
>> drivers/gpu/drm/xe/xe_svm.h | 14 ++++++++++++++
>> drivers/gpu/drm/xe/xe_userptr.c | 1 +
>> drivers/gpu/drm/xe/xe_vm.c | 1 +
>> include/drm/drm_gpusvm.h | 7 ++++---
>> 6 files changed, 36 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
>> index eeeeb99cfdf6..cb906765897e 100644
>> --- a/drivers/gpu/drm/drm_gpusvm.c
>> +++ b/drivers/gpu/drm/drm_gpusvm.c
>> @@ -361,7 +361,6 @@ static const struct mmu_interval_notifier_ops
>> drm_gpusvm_notifier_ops = {
>> * @name: Name of the GPU SVM.
>> * @drm: Pointer to the DRM device structure.
>> * @mm: Pointer to the mm_struct for the address space.
>> - * @device_private_page_owner: Device private pages owner.
>> * @mm_start: Start address of GPU SVM.
>> * @mm_range: Range of the GPU SVM.
>> * @notifier_size: Size of individual notifiers.
>> @@ -383,7 +382,7 @@ static const struct mmu_interval_notifier_ops
>> drm_gpusvm_notifier_ops = {
>> */
>> int drm_gpusvm_init(struct drm_gpusvm *gpusvm,
>> const char *name, struct drm_device *drm,
>> - struct mm_struct *mm, void *device_private_page_owner,
>> + struct mm_struct *mm,
>> unsigned long mm_start, unsigned long mm_range,
>> unsigned long notifier_size,
>> const struct drm_gpusvm_ops *ops,
>> @@ -395,15 +394,13 @@ int drm_gpusvm_init(struct drm_gpusvm *gpusvm,
>> mmgrab(mm);
>> } else {
>> /* No full SVM mode, only core drm_gpusvm_pages API. */
>> - if (ops || num_chunks || mm_range || notifier_size ||
>> - device_private_page_owner)
>> + if (ops || num_chunks || mm_range || notifier_size)
>> return -EINVAL;
>> }
>> gpusvm->name = name;
>> gpusvm->drm = drm;
>> gpusvm->mm = mm;
>> - gpusvm->device_private_page_owner = device_private_page_owner;
>> gpusvm->mm_start = mm_start;
>> gpusvm->mm_range = mm_range;
>> gpusvm->notifier_size = notifier_size;
>> @@ -684,6 +681,7 @@ static unsigned int
>> drm_gpusvm_hmm_pfn_to_order(unsigned long hmm_pfn,
>> * @notifier: Pointer to the GPU SVM notifier structure
>> * @start: Start address
>> * @end: End address
>> + * @dev_private_owner: The device private page owner
>> *
>> * Check if pages between start and end have been faulted in on the
>> CPU. Use to
>> * prevent migration of pages without CPU backing store.
>> @@ -692,14 +690,15 @@ static unsigned int
>> drm_gpusvm_hmm_pfn_to_order(unsigned long hmm_pfn,
>> */
>> static bool drm_gpusvm_check_pages(struct drm_gpusvm *gpusvm,
>> struct drm_gpusvm_notifier *notifier,
>> - unsigned long start, unsigned long end)
>> + unsigned long start, unsigned long end,
>> + void *dev_private_owner)
>> {
>> struct hmm_range hmm_range = {
>> .default_flags = 0,
>> .notifier = ¬ifier->notifier,
>> .start = start,
>> .end = end,
>> - .dev_private_owner = gpusvm->device_private_page_owner,
>> + .dev_private_owner = dev_private_owner,
>> };
>> unsigned long timeout =
>> jiffies + msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);
>> @@ -753,6 +752,7 @@ static bool drm_gpusvm_check_pages(struct
>> drm_gpusvm *gpusvm,
>> * @gpuva_start: Start address of GPUVA which mirrors CPU
>> * @gpuva_end: End address of GPUVA which mirrors CPU
>> * @check_pages_threshold: Check CPU pages for present threshold
>> + * @dev_private_owner: The device private page owner
>> *
>> * This function determines the chunk size for the GPU SVM range
>> based on the
>> * fault address, GPU SVM chunk sizes, existing GPU SVM ranges, and
>> the virtual
>> @@ -767,7 +767,8 @@ drm_gpusvm_range_chunk_size(struct drm_gpusvm
>> *gpusvm,
>> unsigned long fault_addr,
>> unsigned long gpuva_start,
>> unsigned long gpuva_end,
>> - unsigned long check_pages_threshold)
>> + unsigned long check_pages_threshold,
>> + void *dev_private_owner)
>> {
>> unsigned long start, end;
>> int i = 0;
>> @@ -814,7 +815,7 @@ drm_gpusvm_range_chunk_size(struct drm_gpusvm
>> *gpusvm,
>> * process-many-malloc' mallocs at least 64k at a time.
>> */
>> if (end - start <= check_pages_threshold &&
>> - !drm_gpusvm_check_pages(gpusvm, notifier, start, end)) {
>> + !drm_gpusvm_check_pages(gpusvm, notifier, start, end,
>> dev_private_owner)) {
>> ++i;
>> goto retry;
>> }
>> @@ -957,7 +958,8 @@ drm_gpusvm_range_find_or_insert(struct drm_gpusvm
>> *gpusvm,
>> chunk_size = drm_gpusvm_range_chunk_size(gpusvm, notifier, vas,
>> fault_addr, gpuva_start,
>> gpuva_end,
>> - ctx->check_pages_threshold);
>> + ctx->check_pages_threshold,
>> + ctx->device_private_page_owner);
>> if (chunk_size == LONG_MAX) {
>> err = -EINVAL;
>> goto err_notifier_remove;
>> @@ -1268,7 +1270,7 @@ int drm_gpusvm_get_pages(struct drm_gpusvm
>> *gpusvm,
>> .notifier = notifier,
>> .start = pages_start,
>> .end = pages_end,
>> - .dev_private_owner = gpusvm->device_private_page_owner,
>> + .dev_private_owner = ctx->device_private_page_owner,
>> };
>> void *zdd;
>> unsigned long timeout =
>> diff --git a/drivers/gpu/drm/xe/xe_svm.c b/drivers/gpu/drm/xe/xe_svm.c
>> index 7f2f1f041f1d..7e2db71ff34e 100644
>> --- a/drivers/gpu/drm/xe/xe_svm.c
>> +++ b/drivers/gpu/drm/xe/xe_svm.c
>> @@ -67,11 +67,6 @@ void xe_svm_range_debug(struct xe_svm_range
>> *range, const char *operation)
>> range_debug(range, operation);
>> }
>> -static void *xe_svm_devm_owner(struct xe_device *xe)
>> -{
>> - return xe;
>> -}
>> -
>> static struct drm_gpusvm_range *
>> xe_svm_range_alloc(struct drm_gpusvm *gpusvm)
>> {
>> @@ -744,15 +739,14 @@ int xe_svm_init(struct xe_vm *vm)
>> xe_svm_garbage_collector_work_func);
>> err = drm_gpusvm_init(&vm->svm.gpusvm, "Xe SVM",
>> &vm->xe->drm,
>> - current->mm, xe_svm_devm_owner(vm->xe), 0,
>> - vm->size,
>> + current->mm, 0, vm->size,
>> xe_modparam.svm_notifier_size * SZ_1M,
>> &gpusvm_ops, fault_chunk_sizes,
>> ARRAY_SIZE(fault_chunk_sizes));
>> drm_gpusvm_driver_set_lock(&vm->svm.gpusvm, &vm->lock);
>> } else {
>> err = drm_gpusvm_init(&vm->svm.gpusvm, "Xe SVM (simple)",
>> - &vm->xe->drm, NULL, NULL, 0, 0, 0, NULL,
>> + &vm->xe->drm, NULL, 0, 0, 0, NULL,
>> NULL, 0);
>> }
>> @@ -1017,6 +1011,7 @@ static int __xe_svm_handle_pagefault(struct
>> xe_vm *vm, struct xe_vma *vma,
>> .devmem_only = need_vram && devmem_possible,
>> .timeslice_ms = need_vram && devmem_possible ?
>> vm->xe->atomic_svm_timeslice_ms : 0,
>> + .device_private_page_owner = xe_svm_devm_owner(vm->xe),
>> };
>> struct xe_validation_ctx vctx;
>> struct drm_exec exec;
>> diff --git a/drivers/gpu/drm/xe/xe_svm.h b/drivers/gpu/drm/xe/xe_svm.h
>> index cef6ee7d6fe3..0955d2ac8d74 100644
>> --- a/drivers/gpu/drm/xe/xe_svm.h
>> +++ b/drivers/gpu/drm/xe/xe_svm.h
>> @@ -6,6 +6,20 @@
>> #ifndef _XE_SVM_H_
>> #define _XE_SVM_H_
>> +struct xe_device;
>> +
>> +/**
>> + * xe_svm_devm_owner() - Return the owner of device private memory
>> + * @xe: The xe device.
>> + *
>> + * Return: The owner of this device's device private memory to use in
>> + * hmm_range_fault()-
>> + */
>> +static inline void *xe_svm_devm_owner(struct xe_device *xe)
>> +{
>> + return xe;
>> +}
>> +
>> #if IS_ENABLED(CONFIG_DRM_XE_GPUSVM)
>> #include <drm/drm_pagemap.h>
>> diff --git a/drivers/gpu/drm/xe/xe_userptr.c
>> b/drivers/gpu/drm/xe/xe_userptr.c
>> index 91d09af71ced..f16e92cd8090 100644
>> --- a/drivers/gpu/drm/xe/xe_userptr.c
>> +++ b/drivers/gpu/drm/xe/xe_userptr.c
>> @@ -54,6 +54,7 @@ int xe_vma_userptr_pin_pages(struct xe_userptr_vma
>> *uvma)
>> struct xe_device *xe = vm->xe;
>> struct drm_gpusvm_ctx ctx = {
>> .read_only = xe_vma_read_only(vma),
>> + .device_private_page_owner = NULL,
>> };
>> lockdep_assert_held(&vm->lock);
>> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
>> index 80b7f13ecd80..4e914928e0a9 100644
>> --- a/drivers/gpu/drm/xe/xe_vm.c
>> +++ b/drivers/gpu/drm/xe/xe_vm.c
>> @@ -2883,6 +2883,7 @@ static int prefetch_ranges(struct xe_vm *vm,
>> struct xe_vma_op *op)
>> ctx.read_only = xe_vma_read_only(vma);
>> ctx.devmem_possible = devmem_possible;
>> ctx.check_pages_threshold = devmem_possible ? SZ_64K : 0;
>> + ctx.device_private_page_owner = xe_svm_devm_owner(vm->xe);
>> /* TODO: Threading the migration */
>> xa_for_each(&op->prefetch_range.range, i, svm_range) {
>> diff --git a/include/drm/drm_gpusvm.h b/include/drm/drm_gpusvm.h
>> index 5434048a2ca4..b92faa9a26b2 100644
>> --- a/include/drm/drm_gpusvm.h
>> +++ b/include/drm/drm_gpusvm.h
>> @@ -179,7 +179,6 @@ struct drm_gpusvm_range {
>> * @name: Name of the GPU SVM
>> * @drm: Pointer to the DRM device structure
>> * @mm: Pointer to the mm_struct for the address space
>> - * @device_private_page_owner: Device private pages owner
>> * @mm_start: Start address of GPU SVM
>> * @mm_range: Range of the GPU SVM
>> * @notifier_size: Size of individual notifiers
>> @@ -204,7 +203,6 @@ struct drm_gpusvm {
>> const char *name;
>> struct drm_device *drm;
>> struct mm_struct *mm;
>> - void *device_private_page_owner;
>> unsigned long mm_start;
>> unsigned long mm_range;
>> unsigned long notifier_size;
>> @@ -226,6 +224,8 @@ struct drm_gpusvm {
>> /**
>> * struct drm_gpusvm_ctx - DRM GPU SVM context
>> *
>> + * @device_private_page_owner: The device-private page owner to use for
>> + * this operation
>> * @check_pages_threshold: Check CPU pages for present if chunk is
>> less than or
>> * equal to threshold. If not present,
>> reduce chunk
>> * size.
>> @@ -239,6 +239,7 @@ struct drm_gpusvm {
>> * Context that is DRM GPUSVM is operating in (i.e. user arguments).
>> */
>> struct drm_gpusvm_ctx {
>> + void *device_private_page_owner;
>> unsigned long check_pages_threshold;
>> unsigned long timeslice_ms;
>> unsigned int in_notifier :1;
>> @@ -249,7 +250,7 @@ struct drm_gpusvm_ctx {
>> int drm_gpusvm_init(struct drm_gpusvm *gpusvm,
>> const char *name, struct drm_device *drm,
>> - struct mm_struct *mm, void *device_private_page_owner,
>> + struct mm_struct *mm,
>> unsigned long mm_start, unsigned long mm_range,
>> unsigned long notifier_size,
>> const struct drm_gpusvm_ops *ops,
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] drm/gpusvm, drm/xe: Fix userptr to not allow device private pages
@ 2025-09-30 12:27 Thomas Hellström
0 siblings, 0 replies; 6+ messages in thread
From: Thomas Hellström @ 2025-09-30 12:27 UTC (permalink / raw)
To: intel-xe
Cc: dri-devel, Thomas Hellström, Matthew Auld,
Himal Prasad Ghimiray, Matthew Brost
When userptr is used on SVM-enabled VMs, a non-NULL
hmm_range::dev_private_owner value might mean that
hmm_range_fault() attempts to return device private pages.
Either that will fail, or the userptr code will not know
how to handle those.
Use NULL for hmm_range::dev_private_owner to migrate
such pages to system. In order to do that, move the
struct drm_gpusvm::device_private_page_owner field to
struct drm_gpusvm_ctx::device_private_page_owner so that
it doesn't remain immutable over the drm_gpusvm lifetime.
v2:
- Don't conditionally compile xe_svm_devm_owner().
- Kerneldoc xe_svm_devm_owner().
Fixes: 9e9787414882 ("drm/xe/userptr: replace xe_hmm with gpusvm")
Cc: Matthew Auld <matthew.auld@intel.com>
Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
drivers/gpu/drm/drm_gpusvm.c | 24 +++++++++++++-----------
drivers/gpu/drm/xe/xe_svm.c | 11 +++--------
drivers/gpu/drm/xe/xe_svm.h | 14 ++++++++++++++
drivers/gpu/drm/xe/xe_userptr.c | 1 +
drivers/gpu/drm/xe/xe_vm.c | 1 +
include/drm/drm_gpusvm.h | 7 ++++---
6 files changed, 36 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
index eeeeb99cfdf6..cb906765897e 100644
--- a/drivers/gpu/drm/drm_gpusvm.c
+++ b/drivers/gpu/drm/drm_gpusvm.c
@@ -361,7 +361,6 @@ static const struct mmu_interval_notifier_ops drm_gpusvm_notifier_ops = {
* @name: Name of the GPU SVM.
* @drm: Pointer to the DRM device structure.
* @mm: Pointer to the mm_struct for the address space.
- * @device_private_page_owner: Device private pages owner.
* @mm_start: Start address of GPU SVM.
* @mm_range: Range of the GPU SVM.
* @notifier_size: Size of individual notifiers.
@@ -383,7 +382,7 @@ static const struct mmu_interval_notifier_ops drm_gpusvm_notifier_ops = {
*/
int drm_gpusvm_init(struct drm_gpusvm *gpusvm,
const char *name, struct drm_device *drm,
- struct mm_struct *mm, void *device_private_page_owner,
+ struct mm_struct *mm,
unsigned long mm_start, unsigned long mm_range,
unsigned long notifier_size,
const struct drm_gpusvm_ops *ops,
@@ -395,15 +394,13 @@ int drm_gpusvm_init(struct drm_gpusvm *gpusvm,
mmgrab(mm);
} else {
/* No full SVM mode, only core drm_gpusvm_pages API. */
- if (ops || num_chunks || mm_range || notifier_size ||
- device_private_page_owner)
+ if (ops || num_chunks || mm_range || notifier_size)
return -EINVAL;
}
gpusvm->name = name;
gpusvm->drm = drm;
gpusvm->mm = mm;
- gpusvm->device_private_page_owner = device_private_page_owner;
gpusvm->mm_start = mm_start;
gpusvm->mm_range = mm_range;
gpusvm->notifier_size = notifier_size;
@@ -684,6 +681,7 @@ static unsigned int drm_gpusvm_hmm_pfn_to_order(unsigned long hmm_pfn,
* @notifier: Pointer to the GPU SVM notifier structure
* @start: Start address
* @end: End address
+ * @dev_private_owner: The device private page owner
*
* Check if pages between start and end have been faulted in on the CPU. Use to
* prevent migration of pages without CPU backing store.
@@ -692,14 +690,15 @@ static unsigned int drm_gpusvm_hmm_pfn_to_order(unsigned long hmm_pfn,
*/
static bool drm_gpusvm_check_pages(struct drm_gpusvm *gpusvm,
struct drm_gpusvm_notifier *notifier,
- unsigned long start, unsigned long end)
+ unsigned long start, unsigned long end,
+ void *dev_private_owner)
{
struct hmm_range hmm_range = {
.default_flags = 0,
.notifier = ¬ifier->notifier,
.start = start,
.end = end,
- .dev_private_owner = gpusvm->device_private_page_owner,
+ .dev_private_owner = dev_private_owner,
};
unsigned long timeout =
jiffies + msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);
@@ -753,6 +752,7 @@ static bool drm_gpusvm_check_pages(struct drm_gpusvm *gpusvm,
* @gpuva_start: Start address of GPUVA which mirrors CPU
* @gpuva_end: End address of GPUVA which mirrors CPU
* @check_pages_threshold: Check CPU pages for present threshold
+ * @dev_private_owner: The device private page owner
*
* This function determines the chunk size for the GPU SVM range based on the
* fault address, GPU SVM chunk sizes, existing GPU SVM ranges, and the virtual
@@ -767,7 +767,8 @@ drm_gpusvm_range_chunk_size(struct drm_gpusvm *gpusvm,
unsigned long fault_addr,
unsigned long gpuva_start,
unsigned long gpuva_end,
- unsigned long check_pages_threshold)
+ unsigned long check_pages_threshold,
+ void *dev_private_owner)
{
unsigned long start, end;
int i = 0;
@@ -814,7 +815,7 @@ drm_gpusvm_range_chunk_size(struct drm_gpusvm *gpusvm,
* process-many-malloc' mallocs at least 64k at a time.
*/
if (end - start <= check_pages_threshold &&
- !drm_gpusvm_check_pages(gpusvm, notifier, start, end)) {
+ !drm_gpusvm_check_pages(gpusvm, notifier, start, end, dev_private_owner)) {
++i;
goto retry;
}
@@ -957,7 +958,8 @@ drm_gpusvm_range_find_or_insert(struct drm_gpusvm *gpusvm,
chunk_size = drm_gpusvm_range_chunk_size(gpusvm, notifier, vas,
fault_addr, gpuva_start,
gpuva_end,
- ctx->check_pages_threshold);
+ ctx->check_pages_threshold,
+ ctx->device_private_page_owner);
if (chunk_size == LONG_MAX) {
err = -EINVAL;
goto err_notifier_remove;
@@ -1268,7 +1270,7 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
.notifier = notifier,
.start = pages_start,
.end = pages_end,
- .dev_private_owner = gpusvm->device_private_page_owner,
+ .dev_private_owner = ctx->device_private_page_owner,
};
void *zdd;
unsigned long timeout =
diff --git a/drivers/gpu/drm/xe/xe_svm.c b/drivers/gpu/drm/xe/xe_svm.c
index 7f2f1f041f1d..7e2db71ff34e 100644
--- a/drivers/gpu/drm/xe/xe_svm.c
+++ b/drivers/gpu/drm/xe/xe_svm.c
@@ -67,11 +67,6 @@ void xe_svm_range_debug(struct xe_svm_range *range, const char *operation)
range_debug(range, operation);
}
-static void *xe_svm_devm_owner(struct xe_device *xe)
-{
- return xe;
-}
-
static struct drm_gpusvm_range *
xe_svm_range_alloc(struct drm_gpusvm *gpusvm)
{
@@ -744,15 +739,14 @@ int xe_svm_init(struct xe_vm *vm)
xe_svm_garbage_collector_work_func);
err = drm_gpusvm_init(&vm->svm.gpusvm, "Xe SVM", &vm->xe->drm,
- current->mm, xe_svm_devm_owner(vm->xe), 0,
- vm->size,
+ current->mm, 0, vm->size,
xe_modparam.svm_notifier_size * SZ_1M,
&gpusvm_ops, fault_chunk_sizes,
ARRAY_SIZE(fault_chunk_sizes));
drm_gpusvm_driver_set_lock(&vm->svm.gpusvm, &vm->lock);
} else {
err = drm_gpusvm_init(&vm->svm.gpusvm, "Xe SVM (simple)",
- &vm->xe->drm, NULL, NULL, 0, 0, 0, NULL,
+ &vm->xe->drm, NULL, 0, 0, 0, NULL,
NULL, 0);
}
@@ -1017,6 +1011,7 @@ static int __xe_svm_handle_pagefault(struct xe_vm *vm, struct xe_vma *vma,
.devmem_only = need_vram && devmem_possible,
.timeslice_ms = need_vram && devmem_possible ?
vm->xe->atomic_svm_timeslice_ms : 0,
+ .device_private_page_owner = xe_svm_devm_owner(vm->xe),
};
struct xe_validation_ctx vctx;
struct drm_exec exec;
diff --git a/drivers/gpu/drm/xe/xe_svm.h b/drivers/gpu/drm/xe/xe_svm.h
index cef6ee7d6fe3..0955d2ac8d74 100644
--- a/drivers/gpu/drm/xe/xe_svm.h
+++ b/drivers/gpu/drm/xe/xe_svm.h
@@ -6,6 +6,20 @@
#ifndef _XE_SVM_H_
#define _XE_SVM_H_
+struct xe_device;
+
+/**
+ * xe_svm_devm_owner() - Return the owner of device private memory
+ * @xe: The xe device.
+ *
+ * Return: The owner of this device's device private memory to use in
+ * hmm_range_fault()-
+ */
+static inline void *xe_svm_devm_owner(struct xe_device *xe)
+{
+ return xe;
+}
+
#if IS_ENABLED(CONFIG_DRM_XE_GPUSVM)
#include <drm/drm_pagemap.h>
diff --git a/drivers/gpu/drm/xe/xe_userptr.c b/drivers/gpu/drm/xe/xe_userptr.c
index 91d09af71ced..f16e92cd8090 100644
--- a/drivers/gpu/drm/xe/xe_userptr.c
+++ b/drivers/gpu/drm/xe/xe_userptr.c
@@ -54,6 +54,7 @@ int xe_vma_userptr_pin_pages(struct xe_userptr_vma *uvma)
struct xe_device *xe = vm->xe;
struct drm_gpusvm_ctx ctx = {
.read_only = xe_vma_read_only(vma),
+ .device_private_page_owner = NULL,
};
lockdep_assert_held(&vm->lock);
diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
index 80b7f13ecd80..4e914928e0a9 100644
--- a/drivers/gpu/drm/xe/xe_vm.c
+++ b/drivers/gpu/drm/xe/xe_vm.c
@@ -2883,6 +2883,7 @@ static int prefetch_ranges(struct xe_vm *vm, struct xe_vma_op *op)
ctx.read_only = xe_vma_read_only(vma);
ctx.devmem_possible = devmem_possible;
ctx.check_pages_threshold = devmem_possible ? SZ_64K : 0;
+ ctx.device_private_page_owner = xe_svm_devm_owner(vm->xe);
/* TODO: Threading the migration */
xa_for_each(&op->prefetch_range.range, i, svm_range) {
diff --git a/include/drm/drm_gpusvm.h b/include/drm/drm_gpusvm.h
index 5434048a2ca4..b92faa9a26b2 100644
--- a/include/drm/drm_gpusvm.h
+++ b/include/drm/drm_gpusvm.h
@@ -179,7 +179,6 @@ struct drm_gpusvm_range {
* @name: Name of the GPU SVM
* @drm: Pointer to the DRM device structure
* @mm: Pointer to the mm_struct for the address space
- * @device_private_page_owner: Device private pages owner
* @mm_start: Start address of GPU SVM
* @mm_range: Range of the GPU SVM
* @notifier_size: Size of individual notifiers
@@ -204,7 +203,6 @@ struct drm_gpusvm {
const char *name;
struct drm_device *drm;
struct mm_struct *mm;
- void *device_private_page_owner;
unsigned long mm_start;
unsigned long mm_range;
unsigned long notifier_size;
@@ -226,6 +224,8 @@ struct drm_gpusvm {
/**
* struct drm_gpusvm_ctx - DRM GPU SVM context
*
+ * @device_private_page_owner: The device-private page owner to use for
+ * this operation
* @check_pages_threshold: Check CPU pages for present if chunk is less than or
* equal to threshold. If not present, reduce chunk
* size.
@@ -239,6 +239,7 @@ struct drm_gpusvm {
* Context that is DRM GPUSVM is operating in (i.e. user arguments).
*/
struct drm_gpusvm_ctx {
+ void *device_private_page_owner;
unsigned long check_pages_threshold;
unsigned long timeslice_ms;
unsigned int in_notifier :1;
@@ -249,7 +250,7 @@ struct drm_gpusvm_ctx {
int drm_gpusvm_init(struct drm_gpusvm *gpusvm,
const char *name, struct drm_device *drm,
- struct mm_struct *mm, void *device_private_page_owner,
+ struct mm_struct *mm,
unsigned long mm_start, unsigned long mm_range,
unsigned long notifier_size,
const struct drm_gpusvm_ops *ops,
--
2.51.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] drm/gpusvm, drm/xe: Fix userptr to not allow device private pages
2025-09-30 12:26 ` Thomas Hellström
@ 2025-09-30 22:58 ` Matthew Brost
2025-10-02 9:04 ` Maarten Lankhorst
0 siblings, 1 reply; 6+ messages in thread
From: Matthew Brost @ 2025-09-30 22:58 UTC (permalink / raw)
To: Thomas Hellström; +Cc: Matthew Auld, intel-xe, Himal Prasad Ghimiray
On Tue, Sep 30, 2025 at 02:26:00PM +0200, Thomas Hellström wrote:
>
> On 9/30/25 13:16, Matthew Auld wrote:
> > On 30/09/2025 08:21, Thomas Hellström wrote:
> > > When userptr is used on SVM-enabled VMs, a non-NULL
> > > hmm_range::dev_private_owner value might mean that
> > > hmm_range_fault() attempts to return device private pages.
> > > Either that will fail, or the userptr code will not know
> > > how to handle those.
> > >
> > > Use NULL for hmm_range::dev_private_owner to migrate
> > > such pages to system. In order to do that, move the
> > > struct drm_gpusvm::device_private_page_owner field to
> > > struct drm_gpusvm_ctx::device_private_page_owner so that
> > > it doesn't remain immutable over the drm_gpusvm lifetime.
> > >
> > > v2:
> > > - Don't conditionally compile xe_svm_devm_owner().
> > > - Kerneldoc xe_svm_devm_owner().
> > >
> > > Fixes: 9e9787414882 ("drm/xe/userptr: replace xe_hmm with gpusvm")
> > > Cc: Matthew Auld <matthew.auld@intel.com>
> > > Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> > > Cc: Matthew Brost <matthew.brost@intel.com>
> > > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> >
> > So in theory this should give the same behavior pre gpusvm conversion
> > for userptr?
>
> Actually not, the same problem is there, but we need to have a patch that
> fixes this in Linus' tree before sending a backported patch that fixes the
> behaviour before the conversion.
>
> On top of this, we will then as a follow-up actually allow VRAM pages in
> userptr with the SVM code modified to handle this, since l0 has a some
> use-cases that are not fully ported to SVM yet that would otherwise see
> multiple migration blits for the same memory.
>
> >
> > I guess missing Cc: dri-devel ?
>
> Right. I'll resend with that CC.
>
> >
> > Reviewed-by: Matthew Auld <matthew.auld@intel.com>
Reviewed-by: Matthew Brost <matthew.brost@intel.com>
>
> Thanks,
>
> Thomas
>
> >
> > > ---
> > > drivers/gpu/drm/drm_gpusvm.c | 24 +++++++++++++-----------
> > > drivers/gpu/drm/xe/xe_svm.c | 11 +++--------
> > > drivers/gpu/drm/xe/xe_svm.h | 14 ++++++++++++++
> > > drivers/gpu/drm/xe/xe_userptr.c | 1 +
> > > drivers/gpu/drm/xe/xe_vm.c | 1 +
> > > include/drm/drm_gpusvm.h | 7 ++++---
> > > 6 files changed, 36 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
> > > index eeeeb99cfdf6..cb906765897e 100644
> > > --- a/drivers/gpu/drm/drm_gpusvm.c
> > > +++ b/drivers/gpu/drm/drm_gpusvm.c
> > > @@ -361,7 +361,6 @@ static const struct mmu_interval_notifier_ops
> > > drm_gpusvm_notifier_ops = {
> > > * @name: Name of the GPU SVM.
> > > * @drm: Pointer to the DRM device structure.
> > > * @mm: Pointer to the mm_struct for the address space.
> > > - * @device_private_page_owner: Device private pages owner.
> > > * @mm_start: Start address of GPU SVM.
> > > * @mm_range: Range of the GPU SVM.
> > > * @notifier_size: Size of individual notifiers.
> > > @@ -383,7 +382,7 @@ static const struct mmu_interval_notifier_ops
> > > drm_gpusvm_notifier_ops = {
> > > */
> > > int drm_gpusvm_init(struct drm_gpusvm *gpusvm,
> > > const char *name, struct drm_device *drm,
> > > - struct mm_struct *mm, void *device_private_page_owner,
> > > + struct mm_struct *mm,
> > > unsigned long mm_start, unsigned long mm_range,
> > > unsigned long notifier_size,
> > > const struct drm_gpusvm_ops *ops,
> > > @@ -395,15 +394,13 @@ int drm_gpusvm_init(struct drm_gpusvm *gpusvm,
> > > mmgrab(mm);
> > > } else {
> > > /* No full SVM mode, only core drm_gpusvm_pages API. */
> > > - if (ops || num_chunks || mm_range || notifier_size ||
> > > - device_private_page_owner)
> > > + if (ops || num_chunks || mm_range || notifier_size)
> > > return -EINVAL;
> > > }
> > > gpusvm->name = name;
> > > gpusvm->drm = drm;
> > > gpusvm->mm = mm;
> > > - gpusvm->device_private_page_owner = device_private_page_owner;
> > > gpusvm->mm_start = mm_start;
> > > gpusvm->mm_range = mm_range;
> > > gpusvm->notifier_size = notifier_size;
> > > @@ -684,6 +681,7 @@ static unsigned int
> > > drm_gpusvm_hmm_pfn_to_order(unsigned long hmm_pfn,
> > > * @notifier: Pointer to the GPU SVM notifier structure
> > > * @start: Start address
> > > * @end: End address
> > > + * @dev_private_owner: The device private page owner
> > > *
> > > * Check if pages between start and end have been faulted in on
> > > the CPU. Use to
> > > * prevent migration of pages without CPU backing store.
> > > @@ -692,14 +690,15 @@ static unsigned int
> > > drm_gpusvm_hmm_pfn_to_order(unsigned long hmm_pfn,
> > > */
> > > static bool drm_gpusvm_check_pages(struct drm_gpusvm *gpusvm,
> > > struct drm_gpusvm_notifier *notifier,
> > > - unsigned long start, unsigned long end)
> > > + unsigned long start, unsigned long end,
> > > + void *dev_private_owner)
> > > {
> > > struct hmm_range hmm_range = {
> > > .default_flags = 0,
> > > .notifier = ¬ifier->notifier,
> > > .start = start,
> > > .end = end,
> > > - .dev_private_owner = gpusvm->device_private_page_owner,
> > > + .dev_private_owner = dev_private_owner,
> > > };
> > > unsigned long timeout =
> > > jiffies + msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);
> > > @@ -753,6 +752,7 @@ static bool drm_gpusvm_check_pages(struct
> > > drm_gpusvm *gpusvm,
> > > * @gpuva_start: Start address of GPUVA which mirrors CPU
> > > * @gpuva_end: End address of GPUVA which mirrors CPU
> > > * @check_pages_threshold: Check CPU pages for present threshold
> > > + * @dev_private_owner: The device private page owner
> > > *
> > > * This function determines the chunk size for the GPU SVM range
> > > based on the
> > > * fault address, GPU SVM chunk sizes, existing GPU SVM ranges,
> > > and the virtual
> > > @@ -767,7 +767,8 @@ drm_gpusvm_range_chunk_size(struct drm_gpusvm
> > > *gpusvm,
> > > unsigned long fault_addr,
> > > unsigned long gpuva_start,
> > > unsigned long gpuva_end,
> > > - unsigned long check_pages_threshold)
> > > + unsigned long check_pages_threshold,
> > > + void *dev_private_owner)
> > > {
> > > unsigned long start, end;
> > > int i = 0;
> > > @@ -814,7 +815,7 @@ drm_gpusvm_range_chunk_size(struct drm_gpusvm
> > > *gpusvm,
> > > * process-many-malloc' mallocs at least 64k at a time.
> > > */
> > > if (end - start <= check_pages_threshold &&
> > > - !drm_gpusvm_check_pages(gpusvm, notifier, start, end)) {
> > > + !drm_gpusvm_check_pages(gpusvm, notifier, start, end,
> > > dev_private_owner)) {
> > > ++i;
> > > goto retry;
> > > }
> > > @@ -957,7 +958,8 @@ drm_gpusvm_range_find_or_insert(struct
> > > drm_gpusvm *gpusvm,
> > > chunk_size = drm_gpusvm_range_chunk_size(gpusvm, notifier, vas,
> > > fault_addr, gpuva_start,
> > > gpuva_end,
> > > - ctx->check_pages_threshold);
> > > + ctx->check_pages_threshold,
> > > + ctx->device_private_page_owner);
> > > if (chunk_size == LONG_MAX) {
> > > err = -EINVAL;
> > > goto err_notifier_remove;
> > > @@ -1268,7 +1270,7 @@ int drm_gpusvm_get_pages(struct drm_gpusvm
> > > *gpusvm,
> > > .notifier = notifier,
> > > .start = pages_start,
> > > .end = pages_end,
> > > - .dev_private_owner = gpusvm->device_private_page_owner,
> > > + .dev_private_owner = ctx->device_private_page_owner,
> > > };
> > > void *zdd;
> > > unsigned long timeout =
> > > diff --git a/drivers/gpu/drm/xe/xe_svm.c b/drivers/gpu/drm/xe/xe_svm.c
> > > index 7f2f1f041f1d..7e2db71ff34e 100644
> > > --- a/drivers/gpu/drm/xe/xe_svm.c
> > > +++ b/drivers/gpu/drm/xe/xe_svm.c
> > > @@ -67,11 +67,6 @@ void xe_svm_range_debug(struct xe_svm_range
> > > *range, const char *operation)
> > > range_debug(range, operation);
> > > }
> > > -static void *xe_svm_devm_owner(struct xe_device *xe)
> > > -{
> > > - return xe;
> > > -}
> > > -
> > > static struct drm_gpusvm_range *
> > > xe_svm_range_alloc(struct drm_gpusvm *gpusvm)
> > > {
> > > @@ -744,15 +739,14 @@ int xe_svm_init(struct xe_vm *vm)
> > > xe_svm_garbage_collector_work_func);
> > > err = drm_gpusvm_init(&vm->svm.gpusvm, "Xe SVM",
> > > &vm->xe->drm,
> > > - current->mm, xe_svm_devm_owner(vm->xe), 0,
> > > - vm->size,
> > > + current->mm, 0, vm->size,
> > > xe_modparam.svm_notifier_size * SZ_1M,
> > > &gpusvm_ops, fault_chunk_sizes,
> > > ARRAY_SIZE(fault_chunk_sizes));
> > > drm_gpusvm_driver_set_lock(&vm->svm.gpusvm, &vm->lock);
> > > } else {
> > > err = drm_gpusvm_init(&vm->svm.gpusvm, "Xe SVM (simple)",
> > > - &vm->xe->drm, NULL, NULL, 0, 0, 0, NULL,
> > > + &vm->xe->drm, NULL, 0, 0, 0, NULL,
> > > NULL, 0);
> > > }
> > > @@ -1017,6 +1011,7 @@ static int __xe_svm_handle_pagefault(struct
> > > xe_vm *vm, struct xe_vma *vma,
> > > .devmem_only = need_vram && devmem_possible,
> > > .timeslice_ms = need_vram && devmem_possible ?
> > > vm->xe->atomic_svm_timeslice_ms : 0,
> > > + .device_private_page_owner = xe_svm_devm_owner(vm->xe),
> > > };
> > > struct xe_validation_ctx vctx;
> > > struct drm_exec exec;
> > > diff --git a/drivers/gpu/drm/xe/xe_svm.h b/drivers/gpu/drm/xe/xe_svm.h
> > > index cef6ee7d6fe3..0955d2ac8d74 100644
> > > --- a/drivers/gpu/drm/xe/xe_svm.h
> > > +++ b/drivers/gpu/drm/xe/xe_svm.h
> > > @@ -6,6 +6,20 @@
> > > #ifndef _XE_SVM_H_
> > > #define _XE_SVM_H_
> > > +struct xe_device;
> > > +
> > > +/**
> > > + * xe_svm_devm_owner() - Return the owner of device private memory
> > > + * @xe: The xe device.
> > > + *
> > > + * Return: The owner of this device's device private memory to use in
> > > + * hmm_range_fault()-
> > > + */
> > > +static inline void *xe_svm_devm_owner(struct xe_device *xe)
> > > +{
> > > + return xe;
> > > +}
> > > +
> > > #if IS_ENABLED(CONFIG_DRM_XE_GPUSVM)
> > > #include <drm/drm_pagemap.h>
> > > diff --git a/drivers/gpu/drm/xe/xe_userptr.c
> > > b/drivers/gpu/drm/xe/xe_userptr.c
> > > index 91d09af71ced..f16e92cd8090 100644
> > > --- a/drivers/gpu/drm/xe/xe_userptr.c
> > > +++ b/drivers/gpu/drm/xe/xe_userptr.c
> > > @@ -54,6 +54,7 @@ int xe_vma_userptr_pin_pages(struct xe_userptr_vma
> > > *uvma)
> > > struct xe_device *xe = vm->xe;
> > > struct drm_gpusvm_ctx ctx = {
> > > .read_only = xe_vma_read_only(vma),
> > > + .device_private_page_owner = NULL,
> > > };
> > > lockdep_assert_held(&vm->lock);
> > > diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> > > index 80b7f13ecd80..4e914928e0a9 100644
> > > --- a/drivers/gpu/drm/xe/xe_vm.c
> > > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > > @@ -2883,6 +2883,7 @@ static int prefetch_ranges(struct xe_vm *vm,
> > > struct xe_vma_op *op)
> > > ctx.read_only = xe_vma_read_only(vma);
> > > ctx.devmem_possible = devmem_possible;
> > > ctx.check_pages_threshold = devmem_possible ? SZ_64K : 0;
> > > + ctx.device_private_page_owner = xe_svm_devm_owner(vm->xe);
> > > /* TODO: Threading the migration */
> > > xa_for_each(&op->prefetch_range.range, i, svm_range) {
> > > diff --git a/include/drm/drm_gpusvm.h b/include/drm/drm_gpusvm.h
> > > index 5434048a2ca4..b92faa9a26b2 100644
> > > --- a/include/drm/drm_gpusvm.h
> > > +++ b/include/drm/drm_gpusvm.h
> > > @@ -179,7 +179,6 @@ struct drm_gpusvm_range {
> > > * @name: Name of the GPU SVM
> > > * @drm: Pointer to the DRM device structure
> > > * @mm: Pointer to the mm_struct for the address space
> > > - * @device_private_page_owner: Device private pages owner
> > > * @mm_start: Start address of GPU SVM
> > > * @mm_range: Range of the GPU SVM
> > > * @notifier_size: Size of individual notifiers
> > > @@ -204,7 +203,6 @@ struct drm_gpusvm {
> > > const char *name;
> > > struct drm_device *drm;
> > > struct mm_struct *mm;
> > > - void *device_private_page_owner;
> > > unsigned long mm_start;
> > > unsigned long mm_range;
> > > unsigned long notifier_size;
> > > @@ -226,6 +224,8 @@ struct drm_gpusvm {
> > > /**
> > > * struct drm_gpusvm_ctx - DRM GPU SVM context
> > > *
> > > + * @device_private_page_owner: The device-private page owner to use for
> > > + * this operation
> > > * @check_pages_threshold: Check CPU pages for present if chunk is
> > > less than or
> > > * equal to threshold. If not present,
> > > reduce chunk
> > > * size.
> > > @@ -239,6 +239,7 @@ struct drm_gpusvm {
> > > * Context that is DRM GPUSVM is operating in (i.e. user arguments).
> > > */
> > > struct drm_gpusvm_ctx {
> > > + void *device_private_page_owner;
> > > unsigned long check_pages_threshold;
> > > unsigned long timeslice_ms;
> > > unsigned int in_notifier :1;
> > > @@ -249,7 +250,7 @@ struct drm_gpusvm_ctx {
> > > int drm_gpusvm_init(struct drm_gpusvm *gpusvm,
> > > const char *name, struct drm_device *drm,
> > > - struct mm_struct *mm, void *device_private_page_owner,
> > > + struct mm_struct *mm,
> > > unsigned long mm_start, unsigned long mm_range,
> > > unsigned long notifier_size,
> > > const struct drm_gpusvm_ops *ops,
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] drm/gpusvm, drm/xe: Fix userptr to not allow device private pages
2025-09-30 22:58 ` Matthew Brost
@ 2025-10-02 9:04 ` Maarten Lankhorst
0 siblings, 0 replies; 6+ messages in thread
From: Maarten Lankhorst @ 2025-10-02 9:04 UTC (permalink / raw)
To: Matthew Brost, Thomas Hellström
Cc: Matthew Auld, intel-xe, Himal Prasad Ghimiray
Acked-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Den 2025-10-01 kl. 00:58, skrev Matthew Brost:
> On Tue, Sep 30, 2025 at 02:26:00PM +0200, Thomas Hellström wrote:
>>
>> On 9/30/25 13:16, Matthew Auld wrote:
>>> On 30/09/2025 08:21, Thomas Hellström wrote:
>>>> When userptr is used on SVM-enabled VMs, a non-NULL
>>>> hmm_range::dev_private_owner value might mean that
>>>> hmm_range_fault() attempts to return device private pages.
>>>> Either that will fail, or the userptr code will not know
>>>> how to handle those.
>>>>
>>>> Use NULL for hmm_range::dev_private_owner to migrate
>>>> such pages to system. In order to do that, move the
>>>> struct drm_gpusvm::device_private_page_owner field to
>>>> struct drm_gpusvm_ctx::device_private_page_owner so that
>>>> it doesn't remain immutable over the drm_gpusvm lifetime.
>>>>
>>>> v2:
>>>> - Don't conditionally compile xe_svm_devm_owner().
>>>> - Kerneldoc xe_svm_devm_owner().
>>>>
>>>> Fixes: 9e9787414882 ("drm/xe/userptr: replace xe_hmm with gpusvm")
>>>> Cc: Matthew Auld <matthew.auld@intel.com>
>>>> Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
>>>> Cc: Matthew Brost <matthew.brost@intel.com>
>>>> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>>>
>>> So in theory this should give the same behavior pre gpusvm conversion
>>> for userptr?
>>
>> Actually not, the same problem is there, but we need to have a patch that
>> fixes this in Linus' tree before sending a backported patch that fixes the
>> behaviour before the conversion.
>>
>> On top of this, we will then as a follow-up actually allow VRAM pages in
>> userptr with the SVM code modified to handle this, since l0 has a some
>> use-cases that are not fully ported to SVM yet that would otherwise see
>> multiple migration blits for the same memory.
>>
>>>
>>> I guess missing Cc: dri-devel ?
>>
>> Right. I'll resend with that CC.
>>
>>>
>>> Reviewed-by: Matthew Auld <matthew.auld@intel.com>
>
> Reviewed-by: Matthew Brost <matthew.brost@intel.com>
>
>>
>> Thanks,
>>
>> Thomas
>>
>>>
>>>> ---
>>>> drivers/gpu/drm/drm_gpusvm.c | 24 +++++++++++++-----------
>>>> drivers/gpu/drm/xe/xe_svm.c | 11 +++--------
>>>> drivers/gpu/drm/xe/xe_svm.h | 14 ++++++++++++++
>>>> drivers/gpu/drm/xe/xe_userptr.c | 1 +
>>>> drivers/gpu/drm/xe/xe_vm.c | 1 +
>>>> include/drm/drm_gpusvm.h | 7 ++++---
>>>> 6 files changed, 36 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
>>>> index eeeeb99cfdf6..cb906765897e 100644
>>>> --- a/drivers/gpu/drm/drm_gpusvm.c
>>>> +++ b/drivers/gpu/drm/drm_gpusvm.c
>>>> @@ -361,7 +361,6 @@ static const struct mmu_interval_notifier_ops
>>>> drm_gpusvm_notifier_ops = {
>>>> * @name: Name of the GPU SVM.
>>>> * @drm: Pointer to the DRM device structure.
>>>> * @mm: Pointer to the mm_struct for the address space.
>>>> - * @device_private_page_owner: Device private pages owner.
>>>> * @mm_start: Start address of GPU SVM.
>>>> * @mm_range: Range of the GPU SVM.
>>>> * @notifier_size: Size of individual notifiers.
>>>> @@ -383,7 +382,7 @@ static const struct mmu_interval_notifier_ops
>>>> drm_gpusvm_notifier_ops = {
>>>> */
>>>> int drm_gpusvm_init(struct drm_gpusvm *gpusvm,
>>>> const char *name, struct drm_device *drm,
>>>> - struct mm_struct *mm, void *device_private_page_owner,
>>>> + struct mm_struct *mm,
>>>> unsigned long mm_start, unsigned long mm_range,
>>>> unsigned long notifier_size,
>>>> const struct drm_gpusvm_ops *ops,
>>>> @@ -395,15 +394,13 @@ int drm_gpusvm_init(struct drm_gpusvm *gpusvm,
>>>> mmgrab(mm);
>>>> } else {
>>>> /* No full SVM mode, only core drm_gpusvm_pages API. */
>>>> - if (ops || num_chunks || mm_range || notifier_size ||
>>>> - device_private_page_owner)
>>>> + if (ops || num_chunks || mm_range || notifier_size)
>>>> return -EINVAL;
>>>> }
>>>> gpusvm->name = name;
>>>> gpusvm->drm = drm;
>>>> gpusvm->mm = mm;
>>>> - gpusvm->device_private_page_owner = device_private_page_owner;
>>>> gpusvm->mm_start = mm_start;
>>>> gpusvm->mm_range = mm_range;
>>>> gpusvm->notifier_size = notifier_size;
>>>> @@ -684,6 +681,7 @@ static unsigned int
>>>> drm_gpusvm_hmm_pfn_to_order(unsigned long hmm_pfn,
>>>> * @notifier: Pointer to the GPU SVM notifier structure
>>>> * @start: Start address
>>>> * @end: End address
>>>> + * @dev_private_owner: The device private page owner
>>>> *
>>>> * Check if pages between start and end have been faulted in on
>>>> the CPU. Use to
>>>> * prevent migration of pages without CPU backing store.
>>>> @@ -692,14 +690,15 @@ static unsigned int
>>>> drm_gpusvm_hmm_pfn_to_order(unsigned long hmm_pfn,
>>>> */
>>>> static bool drm_gpusvm_check_pages(struct drm_gpusvm *gpusvm,
>>>> struct drm_gpusvm_notifier *notifier,
>>>> - unsigned long start, unsigned long end)
>>>> + unsigned long start, unsigned long end,
>>>> + void *dev_private_owner)
>>>> {
>>>> struct hmm_range hmm_range = {
>>>> .default_flags = 0,
>>>> .notifier = ¬ifier->notifier,
>>>> .start = start,
>>>> .end = end,
>>>> - .dev_private_owner = gpusvm->device_private_page_owner,
>>>> + .dev_private_owner = dev_private_owner,
>>>> };
>>>> unsigned long timeout =
>>>> jiffies + msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);
>>>> @@ -753,6 +752,7 @@ static bool drm_gpusvm_check_pages(struct
>>>> drm_gpusvm *gpusvm,
>>>> * @gpuva_start: Start address of GPUVA which mirrors CPU
>>>> * @gpuva_end: End address of GPUVA which mirrors CPU
>>>> * @check_pages_threshold: Check CPU pages for present threshold
>>>> + * @dev_private_owner: The device private page owner
>>>> *
>>>> * This function determines the chunk size for the GPU SVM range
>>>> based on the
>>>> * fault address, GPU SVM chunk sizes, existing GPU SVM ranges,
>>>> and the virtual
>>>> @@ -767,7 +767,8 @@ drm_gpusvm_range_chunk_size(struct drm_gpusvm
>>>> *gpusvm,
>>>> unsigned long fault_addr,
>>>> unsigned long gpuva_start,
>>>> unsigned long gpuva_end,
>>>> - unsigned long check_pages_threshold)
>>>> + unsigned long check_pages_threshold,
>>>> + void *dev_private_owner)
>>>> {
>>>> unsigned long start, end;
>>>> int i = 0;
>>>> @@ -814,7 +815,7 @@ drm_gpusvm_range_chunk_size(struct drm_gpusvm
>>>> *gpusvm,
>>>> * process-many-malloc' mallocs at least 64k at a time.
>>>> */
>>>> if (end - start <= check_pages_threshold &&
>>>> - !drm_gpusvm_check_pages(gpusvm, notifier, start, end)) {
>>>> + !drm_gpusvm_check_pages(gpusvm, notifier, start, end,
>>>> dev_private_owner)) {
>>>> ++i;
>>>> goto retry;
>>>> }
>>>> @@ -957,7 +958,8 @@ drm_gpusvm_range_find_or_insert(struct
>>>> drm_gpusvm *gpusvm,
>>>> chunk_size = drm_gpusvm_range_chunk_size(gpusvm, notifier, vas,
>>>> fault_addr, gpuva_start,
>>>> gpuva_end,
>>>> - ctx->check_pages_threshold);
>>>> + ctx->check_pages_threshold,
>>>> + ctx->device_private_page_owner);
>>>> if (chunk_size == LONG_MAX) {
>>>> err = -EINVAL;
>>>> goto err_notifier_remove;
>>>> @@ -1268,7 +1270,7 @@ int drm_gpusvm_get_pages(struct drm_gpusvm
>>>> *gpusvm,
>>>> .notifier = notifier,
>>>> .start = pages_start,
>>>> .end = pages_end,
>>>> - .dev_private_owner = gpusvm->device_private_page_owner,
>>>> + .dev_private_owner = ctx->device_private_page_owner,
>>>> };
>>>> void *zdd;
>>>> unsigned long timeout =
>>>> diff --git a/drivers/gpu/drm/xe/xe_svm.c b/drivers/gpu/drm/xe/xe_svm.c
>>>> index 7f2f1f041f1d..7e2db71ff34e 100644
>>>> --- a/drivers/gpu/drm/xe/xe_svm.c
>>>> +++ b/drivers/gpu/drm/xe/xe_svm.c
>>>> @@ -67,11 +67,6 @@ void xe_svm_range_debug(struct xe_svm_range
>>>> *range, const char *operation)
>>>> range_debug(range, operation);
>>>> }
>>>> -static void *xe_svm_devm_owner(struct xe_device *xe)
>>>> -{
>>>> - return xe;
>>>> -}
>>>> -
>>>> static struct drm_gpusvm_range *
>>>> xe_svm_range_alloc(struct drm_gpusvm *gpusvm)
>>>> {
>>>> @@ -744,15 +739,14 @@ int xe_svm_init(struct xe_vm *vm)
>>>> xe_svm_garbage_collector_work_func);
>>>> err = drm_gpusvm_init(&vm->svm.gpusvm, "Xe SVM",
>>>> &vm->xe->drm,
>>>> - current->mm, xe_svm_devm_owner(vm->xe), 0,
>>>> - vm->size,
>>>> + current->mm, 0, vm->size,
>>>> xe_modparam.svm_notifier_size * SZ_1M,
>>>> &gpusvm_ops, fault_chunk_sizes,
>>>> ARRAY_SIZE(fault_chunk_sizes));
>>>> drm_gpusvm_driver_set_lock(&vm->svm.gpusvm, &vm->lock);
>>>> } else {
>>>> err = drm_gpusvm_init(&vm->svm.gpusvm, "Xe SVM (simple)",
>>>> - &vm->xe->drm, NULL, NULL, 0, 0, 0, NULL,
>>>> + &vm->xe->drm, NULL, 0, 0, 0, NULL,
>>>> NULL, 0);
>>>> }
>>>> @@ -1017,6 +1011,7 @@ static int __xe_svm_handle_pagefault(struct
>>>> xe_vm *vm, struct xe_vma *vma,
>>>> .devmem_only = need_vram && devmem_possible,
>>>> .timeslice_ms = need_vram && devmem_possible ?
>>>> vm->xe->atomic_svm_timeslice_ms : 0,
>>>> + .device_private_page_owner = xe_svm_devm_owner(vm->xe),
>>>> };
>>>> struct xe_validation_ctx vctx;
>>>> struct drm_exec exec;
>>>> diff --git a/drivers/gpu/drm/xe/xe_svm.h b/drivers/gpu/drm/xe/xe_svm.h
>>>> index cef6ee7d6fe3..0955d2ac8d74 100644
>>>> --- a/drivers/gpu/drm/xe/xe_svm.h
>>>> +++ b/drivers/gpu/drm/xe/xe_svm.h
>>>> @@ -6,6 +6,20 @@
>>>> #ifndef _XE_SVM_H_
>>>> #define _XE_SVM_H_
>>>> +struct xe_device;
>>>> +
>>>> +/**
>>>> + * xe_svm_devm_owner() - Return the owner of device private memory
>>>> + * @xe: The xe device.
>>>> + *
>>>> + * Return: The owner of this device's device private memory to use in
>>>> + * hmm_range_fault()-
>>>> + */
>>>> +static inline void *xe_svm_devm_owner(struct xe_device *xe)
>>>> +{
>>>> + return xe;
>>>> +}
>>>> +
>>>> #if IS_ENABLED(CONFIG_DRM_XE_GPUSVM)
>>>> #include <drm/drm_pagemap.h>
>>>> diff --git a/drivers/gpu/drm/xe/xe_userptr.c
>>>> b/drivers/gpu/drm/xe/xe_userptr.c
>>>> index 91d09af71ced..f16e92cd8090 100644
>>>> --- a/drivers/gpu/drm/xe/xe_userptr.c
>>>> +++ b/drivers/gpu/drm/xe/xe_userptr.c
>>>> @@ -54,6 +54,7 @@ int xe_vma_userptr_pin_pages(struct xe_userptr_vma
>>>> *uvma)
>>>> struct xe_device *xe = vm->xe;
>>>> struct drm_gpusvm_ctx ctx = {
>>>> .read_only = xe_vma_read_only(vma),
>>>> + .device_private_page_owner = NULL,
>>>> };
>>>> lockdep_assert_held(&vm->lock);
>>>> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
>>>> index 80b7f13ecd80..4e914928e0a9 100644
>>>> --- a/drivers/gpu/drm/xe/xe_vm.c
>>>> +++ b/drivers/gpu/drm/xe/xe_vm.c
>>>> @@ -2883,6 +2883,7 @@ static int prefetch_ranges(struct xe_vm *vm,
>>>> struct xe_vma_op *op)
>>>> ctx.read_only = xe_vma_read_only(vma);
>>>> ctx.devmem_possible = devmem_possible;
>>>> ctx.check_pages_threshold = devmem_possible ? SZ_64K : 0;
>>>> + ctx.device_private_page_owner = xe_svm_devm_owner(vm->xe);
>>>> /* TODO: Threading the migration */
>>>> xa_for_each(&op->prefetch_range.range, i, svm_range) {
>>>> diff --git a/include/drm/drm_gpusvm.h b/include/drm/drm_gpusvm.h
>>>> index 5434048a2ca4..b92faa9a26b2 100644
>>>> --- a/include/drm/drm_gpusvm.h
>>>> +++ b/include/drm/drm_gpusvm.h
>>>> @@ -179,7 +179,6 @@ struct drm_gpusvm_range {
>>>> * @name: Name of the GPU SVM
>>>> * @drm: Pointer to the DRM device structure
>>>> * @mm: Pointer to the mm_struct for the address space
>>>> - * @device_private_page_owner: Device private pages owner
>>>> * @mm_start: Start address of GPU SVM
>>>> * @mm_range: Range of the GPU SVM
>>>> * @notifier_size: Size of individual notifiers
>>>> @@ -204,7 +203,6 @@ struct drm_gpusvm {
>>>> const char *name;
>>>> struct drm_device *drm;
>>>> struct mm_struct *mm;
>>>> - void *device_private_page_owner;
>>>> unsigned long mm_start;
>>>> unsigned long mm_range;
>>>> unsigned long notifier_size;
>>>> @@ -226,6 +224,8 @@ struct drm_gpusvm {
>>>> /**
>>>> * struct drm_gpusvm_ctx - DRM GPU SVM context
>>>> *
>>>> + * @device_private_page_owner: The device-private page owner to use for
>>>> + * this operation
>>>> * @check_pages_threshold: Check CPU pages for present if chunk is
>>>> less than or
>>>> * equal to threshold. If not present,
>>>> reduce chunk
>>>> * size.
>>>> @@ -239,6 +239,7 @@ struct drm_gpusvm {
>>>> * Context that is DRM GPUSVM is operating in (i.e. user arguments).
>>>> */
>>>> struct drm_gpusvm_ctx {
>>>> + void *device_private_page_owner;
>>>> unsigned long check_pages_threshold;
>>>> unsigned long timeslice_ms;
>>>> unsigned int in_notifier :1;
>>>> @@ -249,7 +250,7 @@ struct drm_gpusvm_ctx {
>>>> int drm_gpusvm_init(struct drm_gpusvm *gpusvm,
>>>> const char *name, struct drm_device *drm,
>>>> - struct mm_struct *mm, void *device_private_page_owner,
>>>> + struct mm_struct *mm,
>>>> unsigned long mm_start, unsigned long mm_range,
>>>> unsigned long notifier_size,
>>>> const struct drm_gpusvm_ops *ops,
>>>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-10-02 9:04 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-09-30 12:27 [PATCH v2] drm/gpusvm, drm/xe: Fix userptr to not allow device private pages Thomas Hellström
-- strict thread matches above, loose matches on Subject: below --
2025-09-30 7:21 Thomas Hellström
2025-09-30 11:16 ` Matthew Auld
2025-09-30 12:26 ` Thomas Hellström
2025-09-30 22:58 ` Matthew Brost
2025-10-02 9:04 ` Maarten Lankhorst
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox