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

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

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.


>  		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?



> +		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