Re: [RFC PATCH v10 3/4] drm/virtio: implement userptr resource support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] drm/virtio: virtio_gpu_userptr_restore() skips DMA remap
  after failed map
- [Low] drm/virtio: duplicated virtio_gpu_userptr_free() and dead
  userptr struct fields
--

commit a322ca4c583b559738cbbb3a5e62c08918a07dd8
Author: Honglei Huang <honghuan@amd.com>

drm/virtio: implement userptr resource support

This commit adds virtio_gpu_object_userptr so the guest kernel can pin an
existing userspace mapping with FOLL_LONGTERM and pass its backing pages to
the host via VIRTIO_GPU_CMD_RESOURCE_CREATE_BLOB, including hibernation
restore support.

> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
> index 9df4c7117341e..a29429b41561f 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
[ ... ]
> @@ -138,12 +139,42 @@ struct virtio_gpu_object_vram {
>  	struct drm_mm_node vram_node;
>  };
>  
> +struct virtio_gpu_object_userptr;
> +
> +struct virtio_gpu_object_userptr_ops {
> +	int (*get_pages)(struct virtio_gpu_object_userptr *userptr);
> +	void (*put_pages)(struct virtio_gpu_object_userptr *userptr);
> +};
> +
> +struct virtio_gpu_object_userptr {
> +	struct virtio_gpu_object base;
> +	const struct virtio_gpu_object_userptr_ops *ops;
> +	/* Protects @pages, @sgt and @dma_mapped. */
> +	struct mutex lock;
> +
> +	uint64_t start;
> +	uint32_t npages;
> +	uint32_t bo_handle;
> +	uint32_t flags;
> +
> +	struct virtio_gpu_device *vgdev;
> +	struct drm_file *file;
> +	struct page **pages;
> +	struct sg_table *sgt;
> +	bool dma_mapped;
> +	enum dma_data_direction dma_dir;
> +	struct mm_struct *mm;
> +};
[ ... ]
> diff --git a/drivers/gpu/drm/virtio/virtgpu_userptr.c b/drivers/gpu/drm/virtio/virtgpu_userptr.c
> new file mode 100644
> index 0000000000000..b8606932550eb
> --- /dev/null
> +++ b/drivers/gpu/drm/virtio/virtgpu_userptr.c
> @@ -0,0 +1,350 @@
[ ... ]
> +static void virtio_gpu_userptr_free(struct drm_gem_object *obj)
> +{
> +	struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(obj);
> +	struct virtio_gpu_device *vgdev = obj->dev->dev_private;
> +
> +	/*
> +	 * Pages stay pinned until RESOURCE_UNREF finishes. Its callback
> +	 * calls virtio_gpu_cleanup_object to drop them.
> +	 */
> +	if (bo->created) {
> +		virtio_gpu_remove_from_restore_list(bo);
> +		virtio_gpu_cmd_unref_resource(vgdev, bo, false);
> +		virtio_gpu_notify(vgdev);
> +		return;
> +	}
> +
> +	virtio_gpu_cleanup_object(bo);
> +}

[Severity: Low]
This isn't a bug, but virtio_gpu_userptr_free() is identical to
virtio_gpu_free_object() in drivers/gpu/drm/virtio/virtgpu_object.c and
could reuse it directly if exported.

In struct virtio_gpu_object_userptr, bo_handle is never used and file is
only written in virtio_gpu_userptr_init() and never read (leaving an
unreferenced struct drm_file pointer on an object that can outlive the file
descriptor until RESOURCE_UNREF finishes).  Also,
struct virtio_gpu_object_userptr_ops has only one static implementation, so
virtio_gpu_userptr_get_pages() and virtio_gpu_userptr_put_pages() can be
called directly without the ops vtable.

[ ... ]

> +static int
> +virtio_gpu_userptr_get_entries(struct virtio_gpu_device *vgdev,
> +			       struct virtio_gpu_object_userptr *userptr,
> +			       struct virtio_gpu_mem_entry **ents,
> +			       unsigned int *nents)
> +{
> +	bool use_dma_api = virtio_gpu_use_dma_api(vgdev->vdev);
> +	struct scatterlist *sg;
> +	unsigned int count;
> +	int si;
> +
> +	count = use_dma_api ? userptr->sgt->nents : userptr->sgt->orig_nents;
> +	if (!count)
> +		return -EINVAL;
> +
> +	*ents = kvmalloc_array(count, sizeof(**ents), GFP_KERNEL);
> +	if (!*ents)
> +		return -ENOMEM;
> +
> +	if (use_dma_api) {
> +		for_each_sgtable_dma_sg(userptr->sgt, sg, si) {
> +			(*ents)[si].addr = cpu_to_le64(sg_dma_address(sg));
> +			(*ents)[si].length = cpu_to_le32(sg_dma_len(sg));
> +			(*ents)[si].padding = 0;
> +		}
> +	} else {
> +		for_each_sgtable_sg(userptr->sgt, sg, si) {
> +			(*ents)[si].addr = cpu_to_le64(sg_phys(sg));
> +			(*ents)[si].length = cpu_to_le32(sg->length);
> +			(*ents)[si].padding = 0;
> +		}
> +	}
> +
> +	*nents = count;
> +	return 0;
> +}
> +
> +int virtio_gpu_userptr_restore(struct virtio_gpu_device *vgdev,
> +			       struct virtio_gpu_object *bo,
> +			       struct virtio_gpu_mem_entry **ents,
> +			       unsigned int *nents)
> +{
> +	struct virtio_gpu_object_userptr *userptr = to_virtio_gpu_userptr(bo);
> +	int ret;
> +
> +	mutex_lock(&userptr->lock);
> +	if (!userptr->sgt || !userptr->pages) {
> +		mutex_unlock(&userptr->lock);
> +		return -EINVAL;
> +	}
> +
> +	if (userptr->dma_mapped) {
> +		struct device *dev = drm_dev_dma_dev(vgdev->ddev);
> +
> +		dma_unmap_sgtable(dev, userptr->sgt, userptr->dma_dir,
> +				  DMA_ATTR_REQUIRE_COHERENT);
> +		userptr->dma_mapped = false;
> +
> +		ret = dma_map_sgtable(dev, userptr->sgt, userptr->dma_dir,
> +				      DMA_ATTR_REQUIRE_COHERENT);
> +		if (ret) {
> +			mutex_unlock(&userptr->lock);
> +			return ret;
> +		}
> +		userptr->dma_mapped = true;
> +	}
> +
> +	ret = virtio_gpu_userptr_get_entries(vgdev, userptr, ents, nents);
> +	mutex_unlock(&userptr->lock);
> +	return ret;
> +}

[Severity: Medium]
If dma_map_sgtable() fails in virtio_gpu_userptr_restore(), will a later
restore attempt send unmapped DMA addresses to the host?

When dma_map_sgtable() fails after dma_unmap_sgtable(), userptr->dma_mapped
is left false while userptr->sgt->nents remains non-zero and the object
stays on vgdev->obj_restore_list.  A subsequent virtio_gpu_userptr_restore()
call then skips remapping because userptr->dma_mapped is false, but
virtio_gpu_userptr_get_entries() checks virtio_gpu_use_dma_api(vgdev->vdev)
and reads the unmapped sg_dma_address(sg) entries anyway.

Checking virtio_gpu_use_dma_api(vgdev->vdev) to decide whether
dma_map_sgtable() is needed, and only checking userptr->dma_mapped before
calling dma_unmap_sgtable(), would avoid this.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106082625.812860-1-honghuan@amd.com?part=3