Re: [PATCH 12/18] drm/xe/eudebug: implement userptr_vma access

["Hajda, Andrzej" <andrzej.hajda@intel.com>]

W dniu 20.10.2024 o 20:16, Matthew Brost pisze:
> On Tue, Oct 01, 2024 at 05:43:00PM +0300, Mika Kuoppala wrote:
>> From: Andrzej Hajda <andrzej.hajda@intel.com>
>>
>> Debugger needs to read/write program's vmas including userptr_vma.
>> Since hmm_range_fault is used to pin userptr vmas, it is possible
>> to map those vmas from debugger context.
>>
>> v2: pin pages vs notifier, move to vm.c (Matthew)
>>
>> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
>> Signed-off-by: Maciej Patelczyk <maciej.patelczyk@intel.com>
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_eudebug.c |  2 +-
>>   drivers/gpu/drm/xe/xe_vm.c      | 47 +++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/xe/xe_vm.h      |  3 +++
>>   3 files changed, 51 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_eudebug.c b/drivers/gpu/drm/xe/xe_eudebug.c
>> index edad6d533d0b..b09d7414cfe3 100644
>> --- a/drivers/gpu/drm/xe/xe_eudebug.c
>> +++ b/drivers/gpu/drm/xe/xe_eudebug.c
>> @@ -3023,7 +3023,7 @@ static int xe_eudebug_vma_access(struct xe_vma *vma, u64 offset,
>>   		return ret;
>>   	}
>>   
>> -	return -EINVAL;
>> +	return xe_uvma_access(to_userptr_vma(vma), offset, buf, bytes, write);
>>   }
>>   
>>   static int xe_eudebug_vm_access(struct xe_vm *vm, u64 offset,
>> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
>> index a836dfc5a86f..5f891e76993b 100644
>> --- a/drivers/gpu/drm/xe/xe_vm.c
>> +++ b/drivers/gpu/drm/xe/xe_vm.c
>> @@ -3421,3 +3421,50 @@ void xe_vm_snapshot_free(struct xe_vm_snapshot *snap)
>>   	}
>>   	kvfree(snap);
>>   }
>> +
>> +int xe_uvma_access(struct xe_userptr_vma *uvma, u64 offset,
>> +		   void *buf, u64 len, bool write)
>> +{
> 
> Maybe dump question but are we overthinking this here?
> 
> Can we just use kthread_use_mm, copy_to_user, copy_from_user?
> 
> If not then my previous comments still apply here.

This function is called from debugger process context and kthread_use_mm 
is allowed only from kthread. Spawning kthread just for this is an 
option but looks odd and suboptimal, could be kind of last resort, or not?

Another options:
1. Keep reference to remote task in xe_userptr and use 
access_process_vm(up->task, ...).

2. Pass xe_eudebug.target_task reference down from eudebug framework to 
this helper and use access_process_vm. Current call chain is:
__xe_eudebug_vm_access - has access to xe_eudebug.target_task
->__vm_read_write
--->xe_eudebug_vm_access
---->xe_eudebug_vm_access
----->xe_eudebug_vma_access
------>xe_vm_userptr_access
So to achieve this multiple changes are required, but maybe it is valid 
path to go?
One potential issue with 1 and 2 is that multiple UMD tests were failing 
when access_process_vm/access_remote_vm were used, they were not 
investigated as this approach was dropped due to different reasons.

3. Continue approach from this patch, but with corrected page iterator 
of up->sg sg list[1]. This was nacked by you(?) [2] but I have problem 
understanding why? I see lot of code in kernel mapping sg pages:
linux$ git grep ' kmap.*sg' | wc -l
61
Is it incorrect? Or our case is different?

4. As you suggested in [3](?), modify xe_hmm_userptr_populate_range to 
keep hmm_range.hmm_pfns(or sth similar) in xe_userptr and use it later 
(instead of up->sg) to iterate over pages.

[1]: 
https://lore.kernel.org/intel-xe/20241011-xe_res_cursor_add_page_iterator-v3-1-0f8b8d3ab021@intel.com/
[2]: 
https://lore.kernel.org/intel-xe/Zw32fauoUmB6Iojk@DUT025-TGLU.fm.intel.com/
[3]: 
https://patchwork.freedesktop.org/patch/617481/?series=136572&rev=2#comment_1126527

Regards
Andrzej

> 
> Matt
> 
>> +	struct xe_vm *vm = xe_vma_vm(&uvma->vma);
>> +	struct xe_userptr *up = &uvma->userptr;
>> +	struct xe_res_cursor cur = {};
>> +	int cur_len, ret = 0;
>> +
>> +	while (true) {
>> +		down_read(&vm->userptr.notifier_lock);
>> +		if (!xe_vma_userptr_check_repin(uvma))
>> +			break;
>> +
>> +		spin_lock(&vm->userptr.invalidated_lock);
>> +		list_del_init(&uvma->userptr.invalidate_link);
>> +		spin_unlock(&vm->userptr.invalidated_lock);
>> +
>> +		up_read(&vm->userptr.notifier_lock);
>> +		ret = xe_vma_userptr_pin_pages(uvma);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	if (!up->sg) {
>> +		ret = -EINVAL;
>> +		goto out_unlock_notifier;
>> +	}
>> +
>> +	for (xe_res_first_sg(up->sg, offset, len, &cur); cur.remaining;
>> +	     xe_res_next(&cur, cur_len)) {
>> +		void *ptr = kmap_local_page(sg_page(cur.sgl)) + cur.start;
>> +
>> +		cur_len = min(cur.size, cur.remaining);
>> +		if (write)
>> +			memcpy(ptr, buf, cur_len);
>> +		else
>> +			memcpy(buf, ptr, cur_len);
>> +		kunmap_local(ptr);
>> +		buf += cur_len;
>> +	}
>> +	ret = len;
>> +
>> +out_unlock_notifier:
>> +	up_read(&vm->userptr.notifier_lock);
>> +	return ret;
>> +}
>> diff --git a/drivers/gpu/drm/xe/xe_vm.h b/drivers/gpu/drm/xe/xe_vm.h
>> index c864dba35e1d..99b9a9b011de 100644
>> --- a/drivers/gpu/drm/xe/xe_vm.h
>> +++ b/drivers/gpu/drm/xe/xe_vm.h
>> @@ -281,3 +281,6 @@ 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);
>> +
>> +int xe_uvma_access(struct xe_userptr_vma *uvma, u64 offset,
>> +		   void *buf, u64 len, bool write);
>> -- 
>> 2.34.1
>>