Re: [PATCH 13/26] RFC drm/xe/eudebug: userptr vm pread/pwrite

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Thomas Hellström <thomas.hellstrom@linux.intel.com> writes:

> On Fri, 2024-12-20 at 13:31 +0200, Mika Kuoppala wrote:
>> Implement debugger vm access for userptrs.
>> 
>> When bind is done, take ref to current task so that
>> we know from which vm the address was bound. Then during
>> debugger pread/pwrite we use this target task as
>> parameter to access the debuggee vm with access_process_vm().
>> 
>> This is based on suggestions from Thomas, Joonas and Simona.
>> 
>> v2: need to add offset into vma (Dominik)
>> 
>> Cc: Matthew Brost <matthew.brost@intel.com>
>> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>> Cc: Dominik Grzegorzek <dominik.grzegorzek@intel.com>
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Simona Vetter <simona@ffwll.ch>
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> ---
>>  drivers/gpu/drm/xe/xe_eudebug.c  | 13 +++++++++++++
>>  drivers/gpu/drm/xe/xe_vm.c       |  4 ++++
>>  drivers/gpu/drm/xe/xe_vm.h       | 28 +++++++++++++++++++++++++++-
>>  drivers/gpu/drm/xe/xe_vm_types.h |  6 ++++++
>>  4 files changed, 50 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/xe/xe_eudebug.c
>> b/drivers/gpu/drm/xe/xe_eudebug.c
>> index 9d87df75348b..8b29192ab110 100644
>> --- a/drivers/gpu/drm/xe/xe_eudebug.c
>> +++ b/drivers/gpu/drm/xe/xe_eudebug.c
>> @@ -3074,6 +3074,19 @@ static int xe_eudebug_vma_access(struct xe_vma
>> *vma, u64 offset_in_vma,
>
> AFAICT all across the core mm code, unsigned long is used for mm
> offsets, rather than u64, which we use for gpu- and physical offsets.

Yup, changed these on the patch introducing the pread/pwrite.

>
>
>>  		xe_bo_put(bo);
>>  
>>  		return ret;
>> +	} else if (xe_vma_is_userptr(vma)) {
>> +		struct xe_userptr *userptr = &to_userptr_vma(vma)-
>> >userptr;
>> +
>> +		/*
>> +		 * XXX: access_remote_vm() would fit as userptr
>> notifier has
>> +		 * mm ref so we would not need to carry task ref at
>> all.
>> +		 * But access_remote_vm is not exported.
>> access_process_vm()
>> +		 * is exported so use it instead.
>> +		 */
>
> Could we add a follow-up patch that exports access_remote_vm() and
> changes this code to use access_remote_vm() instead?
>

Here is the diff:

diff --git a/drivers/gpu/drm/xe/xe_eudebug.c b/drivers/gpu/drm/xe/xe_eudebug.c
index 996fcb4b0e9e..3fdafbf30209 100644
--- a/drivers/gpu/drm/xe/xe_eudebug.c
+++ b/drivers/gpu/drm/xe/xe_eudebug.c
@@ -3763,16 +3763,25 @@ static int xe_eudebug_vma_access(struct xe_vma *vma, u64 offset_in_vma,
 		return ret;
 	} else if (xe_vma_is_userptr(vma)) {
 		struct xe_userptr *userptr = &to_userptr_vma(vma)->userptr;
+		struct xe_vm *vm = xe_vma_vm(vma);
+		struct mm_struct *mm = NULL;
+		int ret;
 
-		/*
-		 * XXX: access_remote_vm() would fit as userptr notifier has
-		 * mm ref so we would not need to carry task ref at all.
-		 * But access_remote_vm is not exported. access_process_vm()
-		 * is exported so use it instead.
-		 */
-		return access_process_vm(userptr->eudebug.task,
-					 xe_vma_userptr(vma), buf, bytes,
-					 write ? FOLL_WRITE : 0);
+		down_read(&vm->userptr.notifier_lock);
+		if (mmget_not_zero(userptr->notifier.mm))
+			mm = userptr->notifier.mm;
+		up_read(&vm->userptr.notifier_lock);
+
+		if (!mm)
+			return -EFAULT;
+
+		ret = access_remote_vm(mm,
+				       xe_vma_userptr(vma) + offset_in_vma,
+				       buf, bytes,
+				       write ? FOLL_WRITE : 0);
+		mmput(mm);
+
+		return ret;
 	}
 
 	return -EINVAL;
diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
index cbc7fdb74166..04157b6b26ea 100644
--- a/drivers/gpu/drm/xe/xe_vm.c
+++ b/drivers/gpu/drm/xe/xe_vm.c
@@ -1003,14 +1003,6 @@ static struct xe_vma *xe_vma_create(struct xe_vm *vm,
 			}
 
 			userptr->notifier_seq = LONG_MAX;
-#if IS_ENABLED(CONFIG_DRM_XE_EUDEBUG)
-			/*
-			 * We could use the mm which is on notifier. But
-			 * the access_remote_vm() is not exported. Thus
-			 * we get reference to task for access_process_vm()
-			 */
-			userptr->eudebug.task = get_task_struct(current);
-#endif
 		}
 
 		xe_vm_get(vm);
@@ -1035,9 +1027,6 @@ static void xe_vma_destroy_late(struct xe_vma *vma)
 		if (userptr->sg)
 			xe_hmm_userptr_free_sg(uvma);
 
-#if IS_ENABLED(CONFIG_DRM_XE_EUDEBUG)
-		put_task_struct(userptr->eudebug.task);
-#endif
 		/*
 		 * Since userptr pages are not pinned, we can't remove
 		 * the notifer until we're sure the GPU is not accessing
diff --git a/drivers/gpu/drm/xe/xe_vm_types.h b/drivers/gpu/drm/xe/xe_vm_types.h
index 0be999dd513f..1c5776194e54 100644
--- a/drivers/gpu/drm/xe/xe_vm_types.h
+++ b/drivers/gpu/drm/xe/xe_vm_types.h
@@ -68,12 +68,6 @@ struct xe_userptr {
 #if IS_ENABLED(CONFIG_DRM_XE_USERPTR_INVAL_INJECT)
 	u32 divisor;
 #endif
-
-#if IS_ENABLED(CONFIG_DRM_XE_EUDEBUG)
-	struct {
-		struct task_struct *task;
-	} eudebug;
-#endif
 };
 
 #if IS_ENABLED(CONFIG_DRM_XE_EUDEBUG)

I will reply also with the export patch and
the complete patch. for reference, they can be found here also:

https://gitlab.freedesktop.org/miku/kernel/-/commit/3ffbc66fb6dd2ff0a9f5f282266a97e073f10deb
https://gitlab.freedesktop.org/miku/kernel/-/commit/ee2ebe9a5debabf984b2cfab34bf0996ace63ab7

Thanks,
-Mika

>
>
>> +		return access_process_vm(userptr->eudebug.task,
>> +					 xe_vma_userptr(vma) +
>> offset_in_vma,
>> +					 buf, bytes,
>> +					 write ? FOLL_WRITE : 0);
>>  	}
>>  
>>  	return -EINVAL;
>> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
>> index 1cb21325d8dd..235ae2db5188 100644
>> --- a/drivers/gpu/drm/xe/xe_vm.c
>> +++ b/drivers/gpu/drm/xe/xe_vm.c
>> @@ -999,6 +999,8 @@ static struct xe_vma *xe_vma_create(struct xe_vm
>> *vm,
>>  			}
>>  
>>  			userptr->notifier_seq = LONG_MAX;
>> +
>> +			xe_eudebug_track_userptr_task(userptr);
>>  		}
>>  
>>  		xe_vm_get(vm);
>> @@ -1023,6 +1025,8 @@ static void xe_vma_destroy_late(struct xe_vma
>> *vma)
>>  		if (userptr->sg)
>>  			xe_hmm_userptr_free_sg(uvma);
>>  
>> +		xe_eudebug_untrack_userptr_task(userptr);
>> +
>>  		/*
>>  		 * Since userptr pages are not pinned, we can't
>> remove
>>  		 * the notifer until we're sure the GPU is not
>> accessing
>> diff --git a/drivers/gpu/drm/xe/xe_vm.h b/drivers/gpu/drm/xe/xe_vm.h
>> index 23adb7442881..4334cf2b0d9d 100644
>> --- a/drivers/gpu/drm/xe/xe_vm.h
>> +++ b/drivers/gpu/drm/xe/xe_vm.h
>> @@ -274,9 +274,35 @@ static inline void vm_dbg(const struct
>> drm_device *dev,
>>  			  const char *format, ...)
>>  { /* noop */ }
>>  #endif
>> -#endif
>>  
>>  struct xe_vm_snapshot *xe_vm_snapshot_capture(struct xe_vm *vm);
>>  void xe_vm_snapshot_capture_delayed(struct xe_vm_snapshot *snap);
>>  void xe_vm_snapshot_print(struct xe_vm_snapshot *snap, struct
>> drm_printer *p);
>>  void xe_vm_snapshot_free(struct xe_vm_snapshot *snap);
>> +
>> +#if IS_ENABLED(CONFIG_DRM_XE_EUDEBUG)
>> +static inline void xe_eudebug_track_userptr_task(struct xe_userptr
>> *userptr)
>> +{
>> +	/*
>> +	 * We could use the mm which is on notifier. But
>> +	 * the access_remote_vm() is not exported. Thus
>> +	 * we get reference to task for access_process_vm()
>> +	 */
>> +	userptr->eudebug.task = get_task_struct(current);
>> +}
>> +
>> +static inline void xe_eudebug_untrack_userptr_task(struct xe_userptr
>> *userptr)
>> +{
>> +	put_task_struct(userptr->eudebug.task);
>> +}
>> +#else
>> +static inline void xe_eudebug_track_userptr_task(struct xe_userptr
>> *userptr)
>> +{
>> +}
>> +
>> +static inline void xe_eudebug_untrack_userptr_task(struct xe_userptr
>> *userptr)
>> +{
>> +}
>> +#endif /* CONFIG_DRM_XE_EUDEBUG */
>> +
>> +#endif
>> diff --git a/drivers/gpu/drm/xe/xe_vm_types.h
>> b/drivers/gpu/drm/xe/xe_vm_types.h
>> index 557b047ebdd7..26176ccbcbbc 100644
>> --- a/drivers/gpu/drm/xe/xe_vm_types.h
>> +++ b/drivers/gpu/drm/xe/xe_vm_types.h
>> @@ -68,6 +68,12 @@ struct xe_userptr {
>>  #if IS_ENABLED(CONFIG_DRM_XE_USERPTR_INVAL_INJECT)
>>  	u32 divisor;
>>  #endif
>> +
>> +#if IS_ENABLED(CONFIG_DRM_XE_EUDEBUG)
>> +	struct {
>> +		struct task_struct *task;
>> +	} eudebug;
>> +#endif
>>  };
>>  
>>  struct xe_vma {
>
> Otherwise LGTM.
> Thanks,
> Thomas