Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx> 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@xxxxxxxxx>
>> Cc: Andrzej Hajda <andrzej.hajda@xxxxxxxxx>
>> Cc: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx>
>> Cc: Dominik Grzegorzek <dominik.grzegorzek@xxxxxxxxx>
>> Cc: Christian König <christian.koenig@xxxxxxx>
>> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
>> Cc: Simona Vetter <simona@xxxxxxxx>
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx>
>> ---
>> 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
