From: Honglei Huang <Honglei1.Huang@amd.com>

Add interval tree to manage the userptrs to prevent repeat creation.
If the userptr exists, the ioctl will return the existing BO, and it's
offset with the create ioctl address.

Signed-off-by: Honglei Huang <Honglei1.Huang@amd.com>
---
 drivers/gpu/drm/virtio/virtgpu_drv.h     |  16 ++-
 drivers/gpu/drm/virtio/virtgpu_ioctl.c   |  13 ++-
 drivers/gpu/drm/virtio/virtgpu_userptr.c | 129 ++++++++++++++++++++++-
 include/uapi/drm/virtgpu_drm.h           |   1 +
 4 files changed, 152 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index f3dcbd241f5a..fa5dd46e3732 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -54,6 +54,7 @@
 #define STATE_INITIALIZING 0
 #define STATE_OK 1
 #define STATE_ERR 2
+#define STATE_RES_EXISTS 3
 
 #define MAX_CAPSET_ID 63
 #define MAX_RINGS 64
@@ -114,18 +115,23 @@ struct virtio_gpu_object_vram {
 };
 
 struct virtio_gpu_object_userptr;
+struct virtio_gpu_fpriv;
 
 struct virtio_gpu_object_userptr_ops {
 	int (*get_pages)(struct virtio_gpu_object_userptr *userptr);
 	void (*put_pages)(struct virtio_gpu_object_userptr *userptr);
 	void (*release)(struct virtio_gpu_object_userptr *userptr);
+	int (*insert)(struct virtio_gpu_object_userptr *userptr, struct virtio_gpu_fpriv *fpriv);
+	int (*remove)(struct virtio_gpu_object_userptr *userptr, struct virtio_gpu_fpriv *fpriv);
 };
 struct virtio_gpu_object_userptr {
 	struct virtio_gpu_object base;
 	const struct virtio_gpu_object_userptr_ops *ops;
 	struct mutex lock;
 
+	uint64_t ptr;
 	uint64_t start;
+	uint64_t last;
 	uint32_t npages;
 	uint32_t bo_handle;
 	uint32_t flags;
@@ -134,6 +140,8 @@ struct virtio_gpu_object_userptr {
 	struct drm_file *file;
 	struct page **pages;
 	struct sg_table *sgt;
+
+	struct interval_tree_node it_node;
 };
 
 #define to_virtio_gpu_shmem(virtio_gpu_object) \
@@ -307,6 +315,8 @@ struct virtio_gpu_fpriv {
 	struct mutex context_lock;
 	char debug_name[DEBUG_NAME_MAX_LEN];
 	bool explicit_debug_name;
+	struct rb_root_cached userptrs_tree;
+	struct mutex userptrs_tree_lock;
 };
 
 /* virtgpu_ioctl.c */
@@ -520,6 +530,10 @@ int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
 int virtio_gpu_userptr_create(struct virtio_gpu_device *vgdev,
 			      struct drm_file *file,
 			      struct virtio_gpu_object_params *params,
-			      struct virtio_gpu_object **bo_ptr);
+			      struct virtio_gpu_object **bo_ptr,
+			      struct drm_virtgpu_resource_create_blob *rc_blob);
 bool virtio_gpu_is_userptr(struct virtio_gpu_object *bo);
+void virtio_gpu_userptr_interval_tree_init(struct virtio_gpu_fpriv *vfpriv);
+void virtio_gpu_userptr_set_handle(struct virtio_gpu_object *qobj,
+				   uint32_t handle);
 #endif
diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index 8a89774d0737..b512d4b37981 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -534,8 +534,11 @@ static int virtio_gpu_resource_create_blob_ioctl(struct drm_device *dev,
 
 	if (guest_blob && !params.userptr)
 		ret = virtio_gpu_object_create(vgdev, &params, &bo, NULL);
-	else if (guest_blob && params.userptr)
-		ret = virtio_gpu_userptr_create(vgdev, file, &params, &bo);
+	else if (guest_blob && params.userptr) {
+		ret = virtio_gpu_userptr_create(vgdev, file, &params, &bo, rc_blob);
+		if (ret > 0)
+			return ret;
+	}
 	else if (!guest_blob && host3d_blob)
 		ret = virtio_gpu_vram_create(vgdev, &params, &bo);
 	else
@@ -567,6 +570,9 @@ static int virtio_gpu_resource_create_blob_ioctl(struct drm_device *dev,
 	rc_blob->res_handle = bo->hw_res_handle;
 	rc_blob->bo_handle = handle;
 
+	if (guest_blob && params.userptr)
+		virtio_gpu_userptr_set_handle(bo, handle);
+
 	/*
 	 * The handle owns the reference now.  But we must drop our
 	 * remaining reference *after* we no longer need to dereference
@@ -691,6 +697,9 @@ static int virtio_gpu_context_init_ioctl(struct drm_device *dev,
 		}
 	}
 
+	if (vgdev->has_resource_userptr)
+		virtio_gpu_userptr_interval_tree_init(vfpriv);
+
 	virtio_gpu_create_context_locked(vgdev, vfpriv);
 	virtio_gpu_notify(vgdev);
 
diff --git a/drivers/gpu/drm/virtio/virtgpu_userptr.c b/drivers/gpu/drm/virtio/virtgpu_userptr.c
index b4a08811d345..03398c3b9f30 100644
--- a/drivers/gpu/drm/virtio/virtgpu_userptr.c
+++ b/drivers/gpu/drm/virtio/virtgpu_userptr.c
@@ -10,6 +10,92 @@
 static struct sg_table *
 virtio_gpu_userptr_get_sg_table(struct drm_gem_object *obj);
 
+static int virtio_gpu_userptr_insert(struct virtio_gpu_object_userptr *userptr,
+				     struct virtio_gpu_fpriv *vfpriv)
+{
+	if (!userptr->ops->insert)
+		return -EINVAL;
+
+	return userptr->ops->insert(userptr, vfpriv);
+}
+
+static int virtio_gpu_userptr_remove(struct virtio_gpu_object_userptr *userptr,
+				     struct virtio_gpu_fpriv *vfpriv)
+{
+	if (!userptr->ops->remove)
+		return -EINVAL;
+
+	return userptr->ops->remove(userptr, vfpriv);
+}
+
+static uint64_t virtio_gpu_userptr_get_offset(struct virtio_gpu_object *qobj,
+					      uint64_t addr)
+{
+	struct virtio_gpu_object_userptr *userptr = to_virtio_gpu_userptr(qobj);
+
+	return PAGE_ALIGN_DOWN(addr) - PAGE_ALIGN_DOWN(userptr->ptr);
+}
+
+static struct virtio_gpu_object_userptr *
+virtio_gpu_userptr_from_addr_range(struct virtio_gpu_fpriv *vfpriv,
+				   u_int64_t start, u_int64_t last)
+{
+	struct interval_tree_node *node;
+	struct virtio_gpu_object_userptr *userptr = NULL;
+	struct virtio_gpu_object_userptr *ret = NULL;
+
+	node = interval_tree_iter_first(&vfpriv->userptrs_tree, start, last);
+
+	while (node) {
+		struct interval_tree_node *next;
+
+		userptr = container_of(node, struct virtio_gpu_object_userptr,
+				       it_node);
+
+		if (start >= userptr->start && last <= userptr->last) {
+			ret = userptr;
+			return ret;
+		}
+
+		next = interval_tree_iter_next(node, start, last);
+		node = next;
+	}
+
+	return ret;
+}
+
+static int virtio_gpu_userptr_insert_interval_tree(
+	struct virtio_gpu_object_userptr *userptr,
+	struct virtio_gpu_fpriv *vfpriv)
+{
+	if (userptr->it_node.start != 0 && userptr->it_node.last != 0) {
+		userptr->it_node.start = userptr->start;
+		userptr->it_node.last = userptr->last;
+		interval_tree_insert(&userptr->it_node, &vfpriv->userptrs_tree);
+		return 0;
+	} else
+		return -EINVAL;
+}
+
+static int virtio_gpu_userptr_remove_interval_tree(
+	struct virtio_gpu_object_userptr *userptr,
+	struct virtio_gpu_fpriv *vfpriv)
+{
+	if (userptr->it_node.start != 0 && userptr->it_node.last != 0) {
+		interval_tree_remove(&userptr->it_node, &vfpriv->userptrs_tree);
+		return 0;
+	} else
+		return -EINVAL;
+}
+
+void virtio_gpu_userptr_set_handle(struct virtio_gpu_object *qobj,
+				   uint32_t handle)
+{
+	struct virtio_gpu_object_userptr *userptr = to_virtio_gpu_userptr(qobj);
+
+	userptr->bo_handle = handle;
+}
+
 static void virtio_gpu_userptr_free(struct drm_gem_object *obj)
 {
 	struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(obj);
@@ -27,6 +113,11 @@ static void virtio_gpu_userptr_free(struct drm_gem_object *obj)
 static void virtio_gpu_userptr_object_close(struct drm_gem_object *obj,
 					    struct drm_file *file)
 {
+	struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(obj);
+	struct virtio_gpu_object_userptr *userptr = to_virtio_gpu_userptr(bo);
+
+	virtio_gpu_userptr_remove(userptr, file->driver_priv);
+
 	virtio_gpu_gem_object_close(obj, file);
 }
 
@@ -63,9 +154,9 @@ virtio_gpu_userptr_get_pages(struct virtio_gpu_object_userptr *userptr)
 	do {
 		num_pages = userptr->npages - pinned;
 
-		ret = pin_user_pages_fast(userptr->start + pinned * PAGE_SIZE,
-					  num_pages, flag,
-					  userptr->pages + pinned);
+		ret = pin_user_pages_fast(
+			PAGE_ALIGN_DOWN(userptr->start) + pinned * PAGE_SIZE,
+			num_pages, flag, userptr->pages + pinned);
 
 		if (ret < 0) {
 			if (pinned)
@@ -127,6 +218,12 @@ virtio_gpu_userptr_get_sg_table(struct drm_gem_object *obj)
 	return userptr->sgt;
 }
 
+void virtio_gpu_userptr_interval_tree_init(struct virtio_gpu_fpriv *vfpriv)
+{
+	vfpriv->userptrs_tree = RB_ROOT_CACHED;
+	mutex_init(&vfpriv->userptrs_tree_lock);
+}
+
 static int
 virtio_gpu_userptr_init(struct drm_device *dev, struct drm_file *file,
 			struct virtio_gpu_object_userptr *userptr,
@@ -144,6 +241,8 @@ virtio_gpu_userptr_init(struct drm_device *dev, struct drm_file *file,
 	aligned_size = roundup(page_offset + params->size, PAGE_SIZE);
 
 	userptr->start = aligned_addr;
+	userptr->last = aligned_addr + aligned_size - 1UL;
+	userptr->ptr = params->userptr;
 	userptr->npages = aligned_size >> PAGE_SHIFT;
 	userptr->flags = params->blob_flags;
 
@@ -167,13 +266,17 @@ static const struct virtio_gpu_object_userptr_ops virtio_gpu_userptr_ops = {
 	.get_pages = virtio_gpu_userptr_get_pages,
 	.put_pages = virtio_gpu_userptr_put_pages,
 	.release = virtio_gpu_userptr_release,
+	.insert = virtio_gpu_userptr_insert_interval_tree,
+	.remove = virtio_gpu_userptr_remove_interval_tree,
 };
 
 int virtio_gpu_userptr_create(struct virtio_gpu_device *vgdev,
 			      struct drm_file *file,
 			      struct virtio_gpu_object_params *params,
-			      struct virtio_gpu_object **bo_ptr)
+			      struct virtio_gpu_object **bo_ptr,
+			      struct drm_virtgpu_resource_create_blob *rc_blob)
 {
+	struct virtio_gpu_fpriv *vfpriv = file->driver_priv;
 	struct virtio_gpu_object_userptr *userptr;
 	int ret, si;
 	struct sg_table *sgt;
@@ -187,6 +290,20 @@ int virtio_gpu_userptr_create(struct virtio_gpu_device *vgdev,
 		       params->size))
 		return -EFAULT;
 
+	mutex_lock(&vfpriv->userptrs_tree_lock);
+
+	userptr = virtio_gpu_userptr_from_addr_range(
+		vfpriv, params->userptr, params->userptr + params->size - 1UL);
+	if (userptr) {
+		*bo_ptr = &userptr->base;
+		rc_blob->res_handle = userptr->base.hw_res_handle;
+		rc_blob->bo_handle = userptr->bo_handle;
+		rc_blob->offset = virtio_gpu_userptr_get_offset(
+			&userptr->base, rc_blob->userptr);
+		mutex_unlock(&vfpriv->userptrs_tree_lock);
+		return STATE_RES_EXISTS;
+	}
+
 	userptr = kzalloc(sizeof(*userptr), GFP_KERNEL);
 	if (!userptr)
 		return -ENOMEM;
@@ -218,6 +335,9 @@ int virtio_gpu_userptr_create(struct virtio_gpu_device *vgdev,
 		(ents)[si].padding = 0;
 	}
 
+	virtio_gpu_userptr_insert(userptr, vfpriv);
+	mutex_unlock(&vfpriv->userptrs_tree_lock);
+
 	virtio_gpu_cmd_resource_create_blob(vgdev, &userptr->base, params, ents,
 					    sgt->nents);
 
@@ -225,6 +345,7 @@ int virtio_gpu_userptr_create(struct virtio_gpu_device *vgdev,
 	return 0;
 
 failed_free:
+	mutex_unlock(&vfpriv->userptrs_tree_lock);
 	kfree(userptr);
 	return ret;
 }
diff --git a/include/uapi/drm/virtgpu_drm.h b/include/uapi/drm/virtgpu_drm.h
index 071f31752721..07c22cf1a9e0 100644
--- a/include/uapi/drm/virtgpu_drm.h
+++ b/include/uapi/drm/virtgpu_drm.h
@@ -196,6 +196,7 @@ struct drm_virtgpu_resource_create_blob {
 	__u64 cmd;
 	__u64 blob_id;
 	__u64 userptr;
+	__u64 offset;
 };
 
 #define VIRTGPU_CONTEXT_PARAM_CAPSET_ID       0x0001
-- 
2.34.1

If the purpose of this feature is to dedup usrptr BOs of a the single
process/application, can this can be done in userspace?

On 3/21/25 11:00, Honglei Huang wrote:
>  int virtio_gpu_userptr_create(struct virtio_gpu_device *vgdev,
>  			      struct drm_file *file,
>  			      struct virtio_gpu_object_params *params,
> -			      struct virtio_gpu_object **bo_ptr)
> +			      struct virtio_gpu_object **bo_ptr,
> +			      struct drm_virtgpu_resource_create_blob *rc_blob)
>  {
> +	struct virtio_gpu_fpriv *vfpriv = file->driver_priv;
>  	struct virtio_gpu_object_userptr *userptr;
>  	int ret, si;
>  	struct sg_table *sgt;
> @@ -187,6 +290,20 @@ int virtio_gpu_userptr_create(struct virtio_gpu_device *vgdev,
>  		       params->size))
>  		return -EFAULT;
>  
> +	mutex_lock(&vfpriv->userptrs_tree_lock);
> +
> +	userptr = virtio_gpu_userptr_from_addr_range(
> +		vfpriv, params->userptr, params->userptr + params->size - 1UL);

Is it possible that userptr address will be same for two different
processes?

What if userptr->flags mismatch?

> +	if (userptr) {
> +		*bo_ptr = &userptr->base;
> +		rc_blob->res_handle = userptr->base.hw_res_handle;
> +		rc_blob->bo_handle = userptr->bo_handle;

Doesn't BO refcount need to be bumped?

> +		rc_blob->offset = virtio_gpu_userptr_get_offset(
> +			&userptr->base, rc_blob->userptr);
> +		mutex_unlock(&vfpriv->userptrs_tree_lock);
> +		return STATE_RES_EXISTS;

STATE_RES_EXISTS isn't a error code

-- 
Best regards,
Dmitry

On 2025/3/30 19:57, Dmitry Osipenko wrote:
> If the purpose of this feature is to dedup usrptr BOs of a the single
> process/application, can this can be done in userspace?
> 
> On 3/21/25 11:00, Honglei Huang wrote:
>>   int virtio_gpu_userptr_create(struct virtio_gpu_device *vgdev,
>>   			      struct drm_file *file,
>>   			      struct virtio_gpu_object_params *params,
>> -			      struct virtio_gpu_object **bo_ptr)
>> +			      struct virtio_gpu_object **bo_ptr,
>> +			      struct drm_virtgpu_resource_create_blob *rc_blob)
>>   {
>> +	struct virtio_gpu_fpriv *vfpriv = file->driver_priv;
>>   	struct virtio_gpu_object_userptr *userptr;
>>   	int ret, si;
>>   	struct sg_table *sgt;
>> @@ -187,6 +290,20 @@ int virtio_gpu_userptr_create(struct virtio_gpu_device *vgdev,
>>   		       params->size))
>>   		return -EFAULT;
>>   
>> +	mutex_lock(&vfpriv->userptrs_tree_lock);
>> +
>> +	userptr = virtio_gpu_userptr_from_addr_range(
>> +		vfpriv, params->userptr, params->userptr + params->size - 1UL);
> 
> Is it possible that userptr address will be same for two different
> processes?
> 
> What if userptr->flags mismatch?

This situation need to be considered, I will add flag check, and add
handle the situation of increasing write permissions.

> 
>> +	if (userptr) {
>> +		*bo_ptr = &userptr->base;
>> +		rc_blob->res_handle = userptr->base.hw_res_handle;
>> +		rc_blob->bo_handle = userptr->bo_handle;
> 
> Doesn't BO refcount need to be bumped?

Will add Bo refcount in next version.

> 
>> +		rc_blob->offset = virtio_gpu_userptr_get_offset(
>> +			&userptr->base, rc_blob->userptr);
>> +		mutex_unlock(&vfpriv->userptrs_tree_lock);
>> +		return STATE_RES_EXISTS;
> 
> STATE_RES_EXISTS isn't a error code

Will use the system's built-in error code, really thanks for the review.

>

On 4/2/25 04:53, Huang, Honglei1 wrote:
> 
> On 2025/3/30 19:57, Dmitry Osipenko wrote:
>> If the purpose of this feature is to dedup usrptr BOs of a the single
>> process/application, can this can be done in userspace? 

I assume it can be done in userspace, don't see why it needs to be in
kernel.

-- 
Best regards,
Dmitry

On 4/2/25 8:34 AM, Dmitry Osipenko wrote:
> On 4/2/25 04:53, Huang, Honglei1 wrote:
>>
>> On 2025/3/30 19:57, Dmitry Osipenko wrote:
>>> If the purpose of this feature is to dedup usrptr BOs of a the single
>>> process/application, can this can be done in userspace? 
> 
> I assume it can be done in userspace, don't see why it needs to be in
> kernel.

The kernel definitely does not need to be responsible for deduplication,
but is it safe to allow userspace to create overlapping BOs, especially
ones that are partially but not entirely overlapping?  If the userspace
libraries ~everyone will be using refuse to create such BOs, then
overlapping BOs will be tested by ~nobody, and untested kernel code is
a good place for security vulnerabilities to linger.

If there are no legitimate use-cases for overlapping BOs, I would treat
attempts to create them as an errors and return -EINVAL, indicating that
the userspace code attempting to create them is buggy.  Userspace can
deduplicate the BOs itself if necessary.  Of course, there need to be
tests for userspace attempting to create overlapping BOs, including
attempting to do so concurrently from multiple threads.

That said, probably the most important part is consistency with userptr
in other (non-virtio) drivers, such as Intel and AMD.  If they allow
overlapping userptr BOs, then virtio should too; if they do not, then
virtio should also forbid them.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)

Hi Dmitry:

Really sorry for missed this comment. Yes it can be done in UMD, 
actually the interval tree is used with the MMU notifier normally,
it is for preventing create same MMU notifier for overlapped areas.
Cause this version patch set doesn't have MMU notifier, removing 
interval tree is reasonable.

Hi Demi:
Adding interval tree can make virtio userptr has robust check, it can be 
done in UMD. And for AMD userptr driver, it is a SVM type driver, it has
both interval tree and MMU notifier but userptr memory is moveable in 
it. No interval tree for Intel i386, not sure about the Intel XE driver.

Maybe I can remove the interval tree in next version.


On 2025/4/3 2:45, Demi Marie Obenour wrote:
> On 4/2/25 8:34 AM, Dmitry Osipenko wrote:
>> On 4/2/25 04:53, Huang, Honglei1 wrote:
>>>
>>> On 2025/3/30 19:57, Dmitry Osipenko wrote:
>>>> If the purpose of this feature is to dedup usrptr BOs of a the single
>>>> process/application, can this can be done in userspace?
>>
>> I assume it can be done in userspace, don't see why it needs to be in
>> kernel.
> 
> The kernel definitely does not need to be responsible for deduplication,
> but is it safe to allow userspace to create overlapping BOs, especially
> ones that are partially but not entirely overlapping?  If the userspace
> libraries ~everyone will be using refuse to create such BOs, then
> overlapping BOs will be tested by ~nobody, and untested kernel code is
> a good place for security vulnerabilities to linger.
> 
> If there are no legitimate use-cases for overlapping BOs, I would treat
> attempts to create them as an errors and return -EINVAL, indicating that
> the userspace code attempting to create them is buggy.  Userspace can
> deduplicate the BOs itself if necessary.  Of course, there need to be
> tests for userspace attempting to create overlapping BOs, including
> attempting to do so concurrently from multiple threads.
> 
> That said, probably the most important part is consistency with userptr
> in other (non-virtio) drivers, such as Intel and AMD.  If they allow
> overlapping userptr BOs, then virtio should too; if they do not, then
> virtio should also forbid them.

On 4/3/25 06:28, Huang, Honglei1 wrote:
> 
> Hi Dmitry:
> 
> Really sorry for missed this comment. Yes it can be done in UMD,
> actually the interval tree is used with the MMU notifier normally,
> it is for preventing create same MMU notifier for overlapped areas.
> Cause this version patch set doesn't have MMU notifier, removing
> interval tree is reasonable.
> 
> Hi Demi:
> Adding interval tree can make virtio userptr has robust check, it can be
> done in UMD. And for AMD userptr driver, it is a SVM type driver, it has
> both interval tree and MMU notifier but userptr memory is moveable in
> it. No interval tree for Intel i386, not sure about the Intel XE driver.
> 
> Maybe I can remove the interval tree in next version.

Great, thanks for the clarification. And no need to be sorry, it happens
to everyone on the ML :D

-- 
Best regards,
Dmitry

On 3/30/25 14:57, Dmitry Osipenko wrote:
>> +	userptr = virtio_gpu_userptr_from_addr_range(
>> +		vfpriv, params->userptr, params->userptr + params->size - 1UL);
> Is it possible that userptr address will be same for two different
> processes?

See now the vfpriv->userptrs_tree, i.e. it's not a global tree.

-- 
Best regards,
Dmitry