Re: [PATCH] drm/xe/userptr: Hold notifier_lock for write on inject test path

[Matthew Brost <matthew.brost@intel.com>]

On Thu, Jun 25, 2026 at 09:56:15PM +0000, Shuicheng Lin wrote:
> When CONFIG_DRM_XE_USERPTR_INVAL_INJECT=y, xe_pt_svm_userptr_pre_commit()
> runs vma_check_userptr() with the svm notifier_lock taken for read. The
> test injection causes vma_check_userptr() to call
> xe_vma_userptr_force_invalidate(), which feeds into
> xe_vma_userptr_do_inval() with drm_gpusvm_ctx.in_notifier=true. That
> flag tells drm_gpusvm_unmap_pages() the caller already holds
> notifier_lock for write and only asserts the mode. Because the caller
> actually holds it for read, the assertion fires:
> 
>   WARNING: drivers/gpu/drm/drm_gpusvm.c:1669 at \
>            drm_gpusvm_unmap_pages+0xd4/0x130 [drm_gpusvm_helper]
>   Call Trace:
>    xe_vma_userptr_do_inval+0x40d/0xfd0 [xe]
>    xe_vma_userptr_invalidate_pass1+0x3e6/0x8d0 [xe]
>    xe_vma_userptr_force_invalidate+0xde/0x290 [xe]
>    vma_check_userptr.constprop.0+0x1c6/0x220 [xe]
>    xe_pt_svm_userptr_pre_commit+0x6a3/0xc60 [xe]
>    ...
>    xe_vm_bind_ioctl+0x3a0a/0x4480 [xe]
> 
> Acquire notifier_lock for write in pre-commit when the inject Kconfig
> is enabled, via new helpers xe_pt_svm_userptr_notifier_lock()/_unlock().
> Rename xe_svm_assert_held_read() to
> xe_svm_assert_held_read_or_inject_write() so it asserts the correct
> mode under each build configuration. Production builds
> (CONFIG_DRM_XE_USERPTR_INVAL_INJECT=n) keep the existing read-mode
> behavior bit-for-bit.
> 
> Fixes: 9e9787414882 ("drm/xe/userptr: replace xe_hmm with gpusvm")
> Assisted-by: Claude:claude-opus-4.7
> Cc: Matthew Auld <matthew.auld@intel.com>
> Cc: Zongyao Bai <zongyao.bai@intel.com>
> Signed-off-by: Shuicheng Lin <shuicheng.lin@intel.com>

Reviewed-by: Matthew Brost <matthew.brost@intel.com>

> ---
>  drivers/gpu/drm/xe/xe_pt.c  | 43 ++++++++++++++++++++++++++++++-------
>  drivers/gpu/drm/xe/xe_svm.h | 15 +++++++++++--
>  2 files changed, 48 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
> index 0959e0e88a14..4f0f438d6b9b 100644
> --- a/drivers/gpu/drm/xe/xe_pt.c
> +++ b/drivers/gpu/drm/xe/xe_pt.c
> @@ -1086,7 +1086,7 @@ static void xe_pt_commit_locks_assert(struct xe_vma *vma)
>  	xe_pt_commit_prepare_locks_assert(vma);
>  
>  	if (xe_vma_is_userptr(vma))
> -		xe_svm_assert_held_read(vm);
> +		xe_svm_assert_held_read_or_inject_write(vm);
>  }
>  
>  static void xe_pt_commit(struct xe_vma *vma,
> @@ -1406,6 +1406,33 @@ static int xe_pt_pre_commit(struct xe_migrate_pt_update *pt_update)
>  				     pt_update_ops, rftree);
>  }
>  
> +/*
> + * Acquire/release the svm notifier_lock around xe_pt_svm_userptr_pre_commit()
> + * and the matching late release in xe_pt_update_ops_run(). Read mode by
> + * default; write mode when CONFIG_DRM_XE_USERPTR_INVAL_INJECT is on,
> + * because a userptr op in this critical section may invoke the injected
> + * xe_vma_userptr_force_invalidate() path that calls
> + * drm_gpusvm_unmap_pages() with ctx->in_notifier=true, which requires the
> + * lock held for write.
> + */
> +static void xe_pt_svm_userptr_notifier_lock(struct xe_vm *vm)
> +{
> +#if IS_ENABLED(CONFIG_DRM_XE_USERPTR_INVAL_INJECT)
> +	down_write(&vm->svm.gpusvm.notifier_lock);
> +#else
> +	xe_svm_notifier_lock(vm);
> +#endif
> +}
> +
> +static void xe_pt_svm_userptr_notifier_unlock(struct xe_vm *vm)
> +{
> +#if IS_ENABLED(CONFIG_DRM_XE_USERPTR_INVAL_INJECT)
> +	up_write(&vm->svm.gpusvm.notifier_lock);
> +#else
> +	xe_svm_notifier_unlock(vm);
> +#endif
> +}
> +
>  #if IS_ENABLED(CONFIG_DRM_GPUSVM)
>  #ifdef CONFIG_DRM_XE_USERPTR_INVAL_INJECT
>  
> @@ -1437,7 +1464,7 @@ static int vma_check_userptr(struct xe_vm *vm, struct xe_vma *vma,
>  	struct xe_userptr_vma *uvma;
>  	unsigned long notifier_seq;
>  
> -	xe_svm_assert_held_read(vm);
> +	xe_svm_assert_held_read_or_inject_write(vm);
>  
>  	if (!xe_vma_is_userptr(vma))
>  		return 0;
> @@ -1467,7 +1494,7 @@ static int op_check_svm_userptr(struct xe_vm *vm, struct xe_vma_op *op,
>  {
>  	int err = 0;
>  
> -	xe_svm_assert_held_read(vm);
> +	xe_svm_assert_held_read_or_inject_write(vm);
>  
>  	switch (op->base.op) {
>  	case DRM_GPUVA_OP_MAP:
> @@ -1539,12 +1566,12 @@ static int xe_pt_svm_userptr_pre_commit(struct xe_migrate_pt_update *pt_update)
>  	if (err)
>  		return err;
>  
> -	xe_svm_notifier_lock(vm);
> +	xe_pt_svm_userptr_notifier_lock(vm);
>  
>  	list_for_each_entry(op, &vops->list, link) {
>  		err = op_check_svm_userptr(vm, op, pt_update_ops);
>  		if (err) {
> -			xe_svm_notifier_unlock(vm);
> +			xe_pt_svm_userptr_notifier_unlock(vm);
>  			break;
>  		}
>  	}
> @@ -2409,7 +2436,7 @@ static void bind_op_commit(struct xe_vm *vm, struct xe_tile *tile,
>  			   vma->tile_invalidated & ~BIT(tile->id));
>  	vma->tile_staged &= ~BIT(tile->id);
>  	if (xe_vma_is_userptr(vma)) {
> -		xe_svm_assert_held_read(vm);
> +		xe_svm_assert_held_read_or_inject_write(vm);
>  		to_userptr_vma(vma)->userptr.initial_bind = true;
>  	}
>  
> @@ -2445,7 +2472,7 @@ static void unbind_op_commit(struct xe_vm *vm, struct xe_tile *tile,
>  	if (!vma->tile_present) {
>  		list_del_init(&vma->combined_links.rebind);
>  		if (xe_vma_is_userptr(vma)) {
> -			xe_svm_assert_held_read(vm);
> +			xe_svm_assert_held_read_or_inject_write(vm);
>  
>  			spin_lock(&vm->userptr.invalidated_lock);
>  			list_del_init(&to_userptr_vma(vma)->userptr.invalidate_link);
> @@ -2721,7 +2748,7 @@ xe_pt_update_ops_run(struct xe_tile *tile, struct xe_vma_ops *vops)
>  	}
>  
>  	if (pt_update_ops->needs_svm_lock)
> -		xe_svm_notifier_unlock(vm);
> +		xe_pt_svm_userptr_notifier_unlock(vm);
>  
>  	/*
>  	 * The last fence is only used for zero bind queue idling; migrate
> diff --git a/drivers/gpu/drm/xe/xe_svm.h b/drivers/gpu/drm/xe/xe_svm.h
> index b7b8eeacf196..3ca46a6f98c7 100644
> --- a/drivers/gpu/drm/xe/xe_svm.h
> +++ b/drivers/gpu/drm/xe/xe_svm.h
> @@ -394,8 +394,19 @@ static inline struct drm_pagemap *xe_drm_pagemap_from_fd(int fd, u32 region_inst
>  #define xe_svm_assert_in_notifier(vm__) \
>  	lockdep_assert_held_write(&(vm__)->svm.gpusvm.notifier_lock)
>  
> -#define xe_svm_assert_held_read(vm__) \
> +/*
> + * Assert the svm notifier_lock is held. Read mode by default; write mode
> + * when CONFIG_DRM_XE_USERPTR_INVAL_INJECT is on, because that path forces
> + * a userptr invalidation that ends in drm_gpusvm_unmap_pages() with
> + * ctx->in_notifier=true, which requires the lock held for write.
> + */
> +#if IS_ENABLED(CONFIG_DRM_XE_USERPTR_INVAL_INJECT)
> +#define xe_svm_assert_held_read_or_inject_write(vm__) \
> +	lockdep_assert_held_write(&(vm__)->svm.gpusvm.notifier_lock)
> +#else
> +#define xe_svm_assert_held_read_or_inject_write(vm__) \
>  	lockdep_assert_held_read(&(vm__)->svm.gpusvm.notifier_lock)
> +#endif
>  
>  #define xe_svm_notifier_lock(vm__)	\
>  	drm_gpusvm_notifier_lock(&(vm__)->svm.gpusvm)
> @@ -409,7 +420,7 @@ static inline struct drm_pagemap *xe_drm_pagemap_from_fd(int fd, u32 region_inst
>  #else
>  #define xe_svm_assert_in_notifier(...) do {} while (0)
>  
> -static inline void xe_svm_assert_held_read(struct xe_vm *vm)
> +static inline void xe_svm_assert_held_read_or_inject_write(struct xe_vm *vm)
>  {
>  }
>  
> -- 
> 2.43.0
>