Re: [PATCH V1] accel/amdxdna: Remap BO pages instead of using vm_insert_pages

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] accel/amdxdna: permanent -EFAULT in aie2_populate_range() on VM_PFNMAP BOs
--

commit 9b00bf66bf06146c9db533ea2617347eed41d1ba
Author: Lizhi Hou <lizhi.hou@amd.com>

accel/amdxdna: Remap BO pages instead of using vm_insert_pages

This commit keeps the VM_PFNMAP flag on shmem and dma-buf BO mappings and
populates their page tables upfront with remap_pfn_range() instead of
switching to VM_MIXEDMAP and calling vm_insert_pages(). It also removes the
custom mixedmap VM operations in favor of drm_gem_shmem_vm_ops and skips
marking VM_PFNMAP imported mappings invalid during mmap.

> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
> index f14484bc31ec0..d78dbce3c2e89 100644
> --- a/drivers/accel/amdxdna/amdxdna_gem.c
> +++ b/drivers/accel/amdxdna/amdxdna_gem.c
[ ... ]
> @@ -502,7 +542,8 @@ static int amdxdna_insert_pages(struct amdxdna_gem_obj *abo,
>  			return ret;
>  		}
>  
> -		amdxdna_mark_mapp_invalid(abo, vma);
> +		if (!(vma->vm_flags & VM_PFNMAP))
> +			amdxdna_mark_mapp_invalid(abo, vma);
>  
>  		/* Drop the reference drm_gem_mmap_obj() acquired.*/
>  		drm_gem_object_put(to_gobj(abo));
> @@ -515,10 +556,9 @@ static int amdxdna_insert_pages(struct amdxdna_gem_obj *abo,
>  		return ret;
>  	}
>  
> -	vm_flags_mod(vma, VM_MIXEDMAP, VM_PFNMAP);
> -	ret = vm_insert_pages(vma, vma->vm_start, abo->base.pages, &num_pages);
> +	ret = amdxdna_remap_pages(abo, vma);

[Severity: Medium]
Can keeping VM_PFNMAP on BO mappings while still registering an HMM interval
notifier cause aie2_populate_range() to fail permanently with -EFAULT?

When PASID is enabled (abo->mem.dma_addr == AMDXDNA_INVALID_ADDR),
amdxdna_gem_obj_mmap() calls amdxdna_hmm_register(), which registers
amdxdna_hmm_ops with default_flags = HMM_PFN_REQ_FAULT on the VMA range.
With this change, both native shmem BOs and imported VM_PFNMAP dma-bufs keep
VM_PFNMAP (and remap_pfn_range() sets VM_IO | VM_PFNMAP).

While amdxdna_insert_pages() avoids calling amdxdna_mark_mapp_invalid() on
VM_PFNMAP VMAs during initial mmap, amdxdna_hmm_invalidate() still marks the
mapping invalid unconditionally whenever an MMU notifier event fires:

drivers/accel/amdxdna/amdxdna_gem.c:amdxdna_hmm_invalidate() {
	...
	down_write(&xdna->notifier_lock);
	abo->mem.map_invalid = true;
	mapp->invalid = true;
	mmu_interval_set_seq(&mapp->notifier, cur_seq);
	up_write(&xdna->notifier_lock);

	ret = xdna->dev_info->ops->hmm_invalidate(abo, range->event);

	if (range->event == MMU_NOTIFY_UNMAP) {
		down_write(&xdna->notifier_lock);
		mapp->unmapped = true;
		up_write(&xdna->notifier_lock);
	}
	...
}

If any non-unmap MMU notifier event occurs on the range (for example, writing
4 to /proc/<pid>/clear_refs, pagemap_scan, or mprotect()), or if a command
submission runs concurrently while MMU_NOTIFY_UNMAP is in progress before
mapp->unmapped is set, aie2_cmd_submit() sees abo->mem.map_invalid and calls
aie2_populate_range() -> hmm_range_fault_unlocked_timeout().

In mm/hmm.c, hmm_vma_walk_test() rejects VM_IO | VM_PFNMAP VMAs when fault is
requested:

mm/hmm.c:hmm_vma_walk_test() {
	...
	if (!(vma->vm_flags & (VM_IO | VM_PFNMAP)) &&
	    vma->vm_flags & VM_READ)
		return 0;
	...
	if (hmm_range_need_fault(hmm_vma_walk,
				 range->hmm_pfns +
					 ((start - range->start) >> PAGE_SHIFT),
				 (end - start) >> PAGE_SHIFT, 0))
		return -EFAULT;
	...
}

Because aie2_populate_range() aborts on error without clearing mapp->invalid
or abo->mem.map_invalid, wouldn't all subsequent command submissions using
this BO fail permanently with -EFAULT?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103190821.2019203-1-lizhi.hou@amd.com?part=1