Re: [PATCH v13 14/20] drm/amdgpu, arm64: untag user pointers in amdgpu_ttm_tt_get_user_pages

["Kuehling, Felix" <Felix.Kuehling@amd.com>]

On 2019-03-20 10:51 a.m., Andrey Konovalov wrote:
> This patch is a part of a series that extends arm64 kernel ABI to allow to
> pass tagged user pointers (with the top byte set to something else other
> than 0x00) as syscall arguments.
>
> amdgpu_ttm_tt_get_user_pages() uses provided user pointers for vma
> lookups, which can only by done with untagged pointers.
>
> Untag user pointers in this function.
>
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 73e71e61dc99..891b027fa33b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -751,10 +751,11 @@ int amdgpu_ttm_tt_get_user_pages(struct ttm_tt *ttm, struct page **pages)
>   		 * check that we only use anonymous memory to prevent problems
>   		 * with writeback
>   		 */
> -		unsigned long end = gtt->userptr + ttm->num_pages * PAGE_SIZE;
> +		unsigned long userptr = untagged_addr(gtt->userptr);
> +		unsigned long end = userptr + ttm->num_pages * PAGE_SIZE;
>   		struct vm_area_struct *vma;
>   
> -		vma = find_vma(mm, gtt->userptr);
> +		vma = find_vma(mm, userptr);
>   		if (!vma || vma->vm_file || vma->vm_end < end) {
>   			up_read(&mm->mmap_sem);
>   			return -EPERM;

We'll need to be careful that we don't break your change when the 
following commit gets applied through drm-next for Linux 5.2:

https://cgit.freedesktop.org/~agd5f/linux/commit/?h=drm-next-5.2-wip&id=915d3eecfa23693bac9e54cdacf84fb4efdcc5c4

Would it make sense to apply the untagging in amdgpu_ttm_tt_set_userptr 
instead? That would avoid this conflict and I think it would clearly put 
the untagging into the user mode code path where the tagged pointer 
originates.

In amdgpu_gem_userptr_ioctl and amdgpu_amdkfd_gpuvm.c (init_user_pages) 
we also set up an MMU notifier with the (tagged) pointer from user mode. 
That should probably also use the untagged address so that MMU notifiers 
for the untagged address get correctly matched up with the right BO. I'd 
move the untagging further up the call stack to cover that. For the GEM 
case I think amdgpu_gem_userptr_ioctl would be the right place. For the 
KFD case, I'd do this in amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu.

Regards,
   Felix