[PATCH 12/18] drm/xe/eudebug: implement userptr_vma access
Matthew Brost
matthew.brost at intel.com
Mon Oct 21 22:34:35 UTC 2024
On Mon, Oct 21, 2024 at 11:54:30AM +0200, Hajda, Andrzej wrote:
> 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 at 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 at intel.com>
> > > Signed-off-by: Maciej Patelczyk <maciej.patelczyk at intel.com>
> > > Signed-off-by: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> > > Reviewed-by: Jonathan Cavitt <jonathan.cavitt at 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, ...).
>
I think remote refs are generally a bad idea but admittedly don't fully
understand what this would look like.
> 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
I looked here every example I found has mapping and accessing 1 page at
time not mapping 1 page and accessing many.
> 61
> Is it incorrect? Or our case is different?
>
A sglist segments are dma-addresses (virtual address), thus every 4k in
the segment can be a different physical page.
i.e., look at this snippet:
+ 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);
If 'cur.start' > 4k, then you are potentially pointing to an incorrect
page and corrupting memory.
Likewise if 'cur_len' > 4k, then you are potentially pointing to an
incorrect page and corrupting memory.
This loop would have to be changed to something like below which kmaps
and accesses 1 page at a time...
for (xe_res_first_sg(up->sg, offset, len, &cur); cur.remaining;
xe_res_next(&cur, cur_len)) {
int segment_len;
int remain;
cur_len = min(cur.size, cur.remaining);
remain = cur_len;
for (i = 0; i < cur_len; i += segment_len) {
phys_addr_t phys = iommu_iova_to_phys(sg_dma_address(cur.sgl) + i + cur.start);
struct page *page = phys_to_page(phys);
void *ptr = kmap_local_page(page);
int ptr_offset = offset & ~PAGE_MASK;
segment_len = min(remain, PAGE_SIZE - ptr_offset);
if (write)
memcpy(ptr + ptr_offset, buf + i, segment_len);
else
memcpy(buf + i, ptr + ptr_offset, segment_len);
kunmap_local(ptr);
offset += segment_len;
remain -= segment_len;
}
buf += cur_len;
}
> 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.
>
Or just call hmm_range_fault directly here and operate on returned pages
directly.
BTW eventually all the userptr stuff is going to change and be based on
GPU SVM [4]. Calling hmm_range_fault directly will always work though
and likely the safest option.
Matt
[4] https://patchwork.freedesktop.org/patch/619809/?series=137870&rev=2
> [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
> > >
>
More information about the Intel-xe
mailing list