Re: [PATCH V3] accel/amdxdna: Drop dma-buf wrapping for ubuf

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] accel/amdxdna: amdxdna_ubuf_hmm_register() accepts VM_PFNMAP
  and VM_IO VMAs
- [Medium] accel/amdxdna: missing is_private_bo() check in
  amdxdna_flush_bo()
--

commit 1d6d9a5c2780dbf488fd71cbc00a82dc1ec39b14
Author: Lizhi Hou <lizhi.hou@amd.com>

accel/amdxdna: Drop dma-buf wrapping for ubuf

This commit converts ubuf buffer objects from imported dma-buf objects to
private DRM GEM objects and tracks their userspace virtual addresses with
HMM instead of long-term page pinning.

> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
> index ffe12a722dfea..a94195ea4e045 100644
> --- a/drivers/accel/amdxdna/amdxdna_gem.c
> +++ b/drivers/accel/amdxdna/amdxdna_gem.c

[ ... ]

> @@ -1364,7 +1370,7 @@ static int amdxdna_bo_pin(struct amdxdna_gem_obj *abo)
>  	struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev);
>  	int ret;
>  
> -	if (is_import_bo(abo))
> +	if (is_import_bo(abo) || is_private_bo(abo))
>  		return 0;
>  
>  	ret = drm_gem_shmem_pin(&abo->base);
> @@ -1377,7 +1383,7 @@ static void amdxdna_bo_unpin(struct amdxdna_gem_obj *abo)
>  {
>  	struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev);
>  
> -	if (is_import_bo(abo))
> +	if (is_import_bo(abo) || is_private_bo(abo))
>  		return;
>  
>  	drm_gem_shmem_unpin(&abo->base);

[Severity: Medium]
Should amdxdna_flush_bo() also check is_private_bo(abo) alongside
is_import_bo(abo)?

Previously, ubuf BOs were imported via amdxdna_gem_prime_import(), so
is_import_bo(abo) was true and amdxdna_flush_bo() returned -EOPNOTSUPP.
Because ubuf BOs are now initialized with drm_gem_private_object_init()
and only set abo->private_buffer, DRM_IOCTL_AMDXDNA_SYNC_BO on a ubuf BO
misses the early return in amdxdna_flush_bo():

drivers/accel/amdxdna/amdxdna_gem.c:amdxdna_flush_bo() {
	if (is_import_bo(abo))
		return -EOPNOTSUPP;
    ...
	if (amdxdna_gem_vmap(abo))
		drm_clflush_virt_range(amdxdna_gem_vmap(abo) + offset, size);
	else if (abo->base.pages)
		drm_clflush_pages(abo->base.pages, abo->mem.size >> PAGE_SHIFT);
	else
		return -EINVAL;
}

As a result, syncing a ubuf BO returns 0 when size is 0, or falls through
to amdxdna_gem_vmap(abo) and abo->base.pages and returns -EINVAL instead
of -EOPNOTSUPP.

[ ... ]

> diff --git a/drivers/accel/amdxdna/amdxdna_ubuf.c b/drivers/accel/amdxdna/amdxdna_ubuf.c
> index 0e0cd69cd1fbf..a5e202fbc72b9 100644
> --- a/drivers/accel/amdxdna/amdxdna_ubuf.c
> +++ b/drivers/accel/amdxdna/amdxdna_ubuf.c
> @@ -6,182 +6,127 @@

[ ... ]

> +static int amdxdna_ubuf_hmm_register(struct amdxdna_client *client,
> +				     struct amdxdna_gem_obj *abo,
> +				     struct amdxdna_drm_va_entry *va_ent)
>  {
> -	struct amdxdna_ubuf_priv *ubuf = attach->dmabuf->priv;
> -	struct sg_table *sg;
> +	struct vm_area_struct *vma;
>  	int ret;
>  
> -	sg = kzalloc_obj(*sg);
> -	if (!sg)
> -		return ERR_PTR(-ENOMEM);
> -
> -	ret = sg_alloc_table_from_pages(sg, ubuf->pages, ubuf->nr_pages, 0,
> -					ubuf->nr_pages << PAGE_SHIFT, GFP_KERNEL);
> -	if (ret)
> -		goto err_free_sg;
> +	mmap_write_lock(client->mm);
> +	vma = find_vma(client->mm, va_ent->vaddr);
> +	if (!vma || vma->vm_start > va_ent->vaddr ||
> +	    vma->vm_end - va_ent->vaddr < va_ent->len) {
> +		ret = -EINVAL;
> +		goto unlock;
> +	}
>  
> -	ret = dma_map_sgtable(attach->dev, sg, direction, 0);
> -	if (ret)
> -		goto err_free_table;
> +	ret = amdxdna_hmm_register(abo, vma, va_ent->vaddr - vma->vm_start, va_ent->len);
> +	if (!ret && !(vma->vm_flags & (VM_PFNMAP | VM_IO)))
> +		amdxdna_mark_map_invalid(abo);

[Severity: Medium]
Should amdxdna_ubuf_hmm_register() reject VMAs with VM_PFNMAP or VM_IO
instead of skipping amdxdna_mark_map_invalid(abo)?

Previously, amdxdna_get_ubuf() called pin_user_pages_fast(), which
rejected VM_PFNMAP and VM_IO mappings.  Skipping
amdxdna_mark_map_invalid(abo) leaves abo->mem.map_invalid false, so
amdxdna_umap_stale() always returns false and amdxdna_gem_umap_populate()
never faults in the range on command submission; if a VM_PFNMAP or VM_IO
VMA passed to DRM_IOCTL_AMDXDNA_CREATE_BO is lazily populated or has its
PTEs zapped, the device will hit IOMMU page faults when accessing the
PASID address.

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