[PATCH v6 01/11] drm/amdgpu: validate userq input args

[PATCH v6 01/11] drm/amdgpu: validate userq input args

[Prike Liang]

This will help on validating the userq input args, and
rejecting for the invalid userq request at the IOCTLs
first place.

Signed-off-by: Prike Liang <Prike.Liang@amd.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c  | 81 +++++++++++++++-------
 drivers/gpu/drm/amd/amdgpu/mes_userqueue.c |  7 --
 2 files changed, 56 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
index 295e7186e156..7f9dfeae4322 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
@@ -359,27 +359,10 @@ amdgpu_userq_create(struct drm_file *filp, union drm_amdgpu_userq *args)
 		(args->in.flags & AMDGPU_USERQ_CREATE_FLAGS_QUEUE_PRIORITY_MASK) >>
 		AMDGPU_USERQ_CREATE_FLAGS_QUEUE_PRIORITY_SHIFT;
 
-	/* Usermode queues are only supported for GFX IP as of now */
-	if (args->in.ip_type != AMDGPU_HW_IP_GFX &&
-	    args->in.ip_type != AMDGPU_HW_IP_DMA &&
-	    args->in.ip_type != AMDGPU_HW_IP_COMPUTE) {
-		drm_file_err(uq_mgr->file, "Usermode queue doesn't support IP type %u\n",
-			     args->in.ip_type);
-		return -EINVAL;
-	}
-
 	r = amdgpu_userq_priority_permit(filp, priority);
 	if (r)
 		return r;
 
-	if ((args->in.flags & AMDGPU_USERQ_CREATE_FLAGS_QUEUE_SECURE) &&
-	    (args->in.ip_type != AMDGPU_HW_IP_GFX) &&
-	    (args->in.ip_type != AMDGPU_HW_IP_COMPUTE) &&
-	    !amdgpu_is_tmz(adev)) {
-		drm_file_err(uq_mgr->file, "Secure only supported on GFX/Compute queues\n");
-		return -EINVAL;
-	}
-
 	r = pm_runtime_get_sync(adev_to_drm(adev)->dev);
 	if (r < 0) {
 		drm_file_err(uq_mgr->file, "pm_runtime_get_sync() failed for userqueue create\n");
@@ -485,22 +468,45 @@ amdgpu_userq_create(struct drm_file *filp, union drm_amdgpu_userq *args)
 	return r;
 }
 
-int amdgpu_userq_ioctl(struct drm_device *dev, void *data,
-		       struct drm_file *filp)
+static int amdgpu_userq_input_args_validate(struct drm_device *dev,
+					union drm_amdgpu_userq *args,
+					struct drm_file *filp)
 {
-	union drm_amdgpu_userq *args = data;
-	int r;
+	struct amdgpu_device *adev = drm_to_adev(dev);
 
 	switch (args->in.op) {
 	case AMDGPU_USERQ_OP_CREATE:
 		if (args->in.flags & ~(AMDGPU_USERQ_CREATE_FLAGS_QUEUE_PRIORITY_MASK |
 				       AMDGPU_USERQ_CREATE_FLAGS_QUEUE_SECURE))
 			return -EINVAL;
-		r = amdgpu_userq_create(filp, args);
-		if (r)
-			drm_file_err(filp, "Failed to create usermode queue\n");
-		break;
+		/* Usermode queues are only supported for GFX IP as of now */
+		if (args->in.ip_type != AMDGPU_HW_IP_GFX &&
+		    args->in.ip_type != AMDGPU_HW_IP_DMA &&
+		    args->in.ip_type != AMDGPU_HW_IP_COMPUTE) {
+			drm_file_err(filp, "Usermode queue doesn't support IP type %u\n",
+				     args->in.ip_type);
+			return -EINVAL;
+		}
+
+		if ((args->in.flags & AMDGPU_USERQ_CREATE_FLAGS_QUEUE_SECURE) &&
+		    (args->in.ip_type != AMDGPU_HW_IP_GFX) &&
+		    (args->in.ip_type != AMDGPU_HW_IP_COMPUTE) &&
+		    !amdgpu_is_tmz(adev)) {
+			drm_file_err(filp, "Secure only supported on GFX/Compute queues\n");
+			return -EINVAL;
+		}
 
+		if (args->in.queue_va == AMDGPU_BO_INVALID_OFFSET ||
+		    args->in.queue_va == 0 ||
+		    args->in.queue_size == 0) {
+			drm_file_err(filp, "invalidate userq queue va or size\n");
+			return -EINVAL;
+		}
+		if (!args->in.wptr_va || !args->in.rptr_va) {
+			drm_file_err(filp, "invalidate userq queue rptr or wptr\n");
+			return -EINVAL;
+		}
+		break;
 	case AMDGPU_USERQ_OP_FREE:
 		if (args->in.ip_type ||
 		    args->in.doorbell_handle ||
@@ -514,6 +520,31 @@ int amdgpu_userq_ioctl(struct drm_device *dev, void *data,
 		    args->in.mqd ||
 		    args->in.mqd_size)
 			return -EINVAL;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+int amdgpu_userq_ioctl(struct drm_device *dev, void *data,
+		       struct drm_file *filp)
+{
+	union drm_amdgpu_userq *args = data;
+	int r;
+
+	if (amdgpu_userq_input_args_validate(dev, args, filp) < 0)
+		return -EINVAL;
+
+	switch (args->in.op) {
+	case AMDGPU_USERQ_OP_CREATE:
+		r = amdgpu_userq_create(filp, args);
+		if (r)
+			drm_file_err(filp, "Failed to create usermode queue\n");
+		break;
+
+	case AMDGPU_USERQ_OP_FREE:
 		r = amdgpu_userq_destroy(filp, args->in.queue_id);
 		if (r)
 			drm_file_err(filp, "Failed to destroy usermode queue\n");
diff --git a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
index d6f50b13e2ba..1457fb49a794 100644
--- a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
+++ b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
@@ -215,13 +215,6 @@ static int mes_userq_mqd_create(struct amdgpu_userq_mgr *uq_mgr,
 		return -ENOMEM;
 	}
 
-	if (!mqd_user->wptr_va || !mqd_user->rptr_va ||
-	    !mqd_user->queue_va || mqd_user->queue_size == 0) {
-		DRM_ERROR("Invalid MQD parameters for userqueue\n");
-		r = -EINVAL;
-		goto free_props;
-	}
-
 	r = amdgpu_userq_create_object(uq_mgr, &queue->mqd, mqd_hw_default->mqd_size);
 	if (r) {
 		DRM_ERROR("Failed to create MQD object for userqueue\n");
-- 
2.34.1

[PATCH v6 02/11] drm/amdgpu: validate userq hw unmap status for destroying userq

[Prike Liang]

Before destroying the userq buffer object, it requires validating
the userq HW unmap status and ensuring the userq is unmapped from
hardware. If the user HW unmap failed, then it needs to reset the
queue for reusing.

Signed-off-by: Prike Liang <Prike.Liang@amd.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
index 7f9dfeae4322..3d2a7f8946cf 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
@@ -319,6 +319,11 @@ amdgpu_userq_destroy(struct drm_file *filp, int queue_id)
 	}
 	amdgpu_bo_unref(&queue->db_obj.obj);
 	r = amdgpu_userq_unmap_helper(uq_mgr, queue);
+	/*TODO: It requires a reset for userq hw unmap error*/
+	if (unlikely(r != AMDGPU_USERQ_STATE_UNMAPPED)) {
+		drm_warn(adev_to_drm(uq_mgr->adev), "trying to destroy a HW mapping userq\n");
+		r = -ETIMEDOUT;
+	}
 	amdgpu_userq_cleanup(uq_mgr, queue, queue_id);
 	mutex_unlock(&uq_mgr->userq_mutex);
 
-- 
2.34.1

[PATCH v6 03/11] drm/amdgpu: rework the userq doorbell object destroy

[Prike Liang]

This patch aims to unify and destroy the userq doorbell objects at
mes_userq_mqd_destroy(), and this change will also help with unpinning
and destroying the userq doorbell objects for amdgpu_userq_mgr_fini()
during releasing the drm files.

Signed-off-by: Prike Liang <Prike.Liang@amd.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c  | 6 ------
 drivers/gpu/drm/amd/amdgpu/mes_userqueue.c | 7 +++++++
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
index 3d2a7f8946cf..15e833b1b3e3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
@@ -312,12 +312,6 @@ amdgpu_userq_destroy(struct drm_file *filp, int queue_id)
 		return -EINVAL;
 	}
 	amdgpu_userq_wait_for_last_fence(uq_mgr, queue);
-	r = amdgpu_bo_reserve(queue->db_obj.obj, true);
-	if (!r) {
-		amdgpu_bo_unpin(queue->db_obj.obj);
-		amdgpu_bo_unreserve(queue->db_obj.obj);
-	}
-	amdgpu_bo_unref(&queue->db_obj.obj);
 	r = amdgpu_userq_unmap_helper(uq_mgr, queue);
 	/*TODO: It requires a reset for userq hw unmap error*/
 	if (unlikely(r != AMDGPU_USERQ_STATE_UNMAPPED)) {
diff --git a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
index 1457fb49a794..15aa1ca67a11 100644
--- a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
+++ b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
@@ -336,6 +336,13 @@ mes_userq_mqd_destroy(struct amdgpu_userq_mgr *uq_mgr,
 		      struct amdgpu_usermode_queue *queue)
 {
 	amdgpu_userq_destroy_object(uq_mgr, &queue->fw_obj);
+
+	if (!amdgpu_bo_reserve(queue->db_obj.obj, true)) {
+		amdgpu_bo_unpin(queue->db_obj.obj);
+		amdgpu_bo_unreserve(queue->db_obj.obj);
+		amdgpu_userq_destroy_object(uq_mgr, &queue->db_obj);
+	}
+
 	kfree(queue->userq_prop);
 	amdgpu_userq_destroy_object(uq_mgr, &queue->mqd);
 }
-- 
2.34.1

[PATCH v6 04/11] drm/amdgpu: validate userq buffer virtual address and size

[Prike Liang]

It needs to validate the userq object virtual address to
determin whether it is residented in a valid vm mapping.

Signed-off-by: Prike Liang <Prike.Liang@amd.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c  | 38 ++++++++++++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h  |  2 ++
 drivers/gpu/drm/amd/amdgpu/mes_userqueue.c | 25 ++++++++++++++
 3 files changed, 65 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
index 15e833b1b3e3..a41dd38b0adb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
@@ -44,6 +44,36 @@ u32 amdgpu_userq_get_supported_ip_mask(struct amdgpu_device *adev)
 	return userq_ip_mask;
 }
 
+int amdgpu_userq_input_va_validate(struct amdgpu_vm *vm, u64 addr,
+				u64 expected_size)
+{
+	struct amdgpu_bo_va_mapping *va_map;
+	u64 user_addr;
+	u64 size;
+	int r;
+
+	user_addr = (addr & AMDGPU_GMC_HOLE_MASK) >> AMDGPU_GPU_PAGE_SHIFT;
+	size = expected_size >> AMDGPU_GPU_PAGE_SHIFT;
+
+	r = amdgpu_bo_reserve(vm->root.bo, false);
+	if (r)
+		return r;
+
+	va_map = amdgpu_vm_bo_lookup_mapping(vm, user_addr);
+	if (!va_map)
+		goto out_err;
+	/* Only validate the userq whether resident in the VM mapping range */
+	if (user_addr >= va_map->start &&
+	    (size != 0 && user_addr + size - 1 <= va_map->last)) {
+		amdgpu_bo_unreserve(vm->root.bo);
+		return 0;
+	}
+
+out_err:
+	amdgpu_bo_unreserve(vm->root.bo);
+	return -EINVAL;
+}
+
 static int
 amdgpu_userq_unmap_helper(struct amdgpu_userq_mgr *uq_mgr,
 			  struct amdgpu_usermode_queue *queue)
@@ -386,6 +416,14 @@ amdgpu_userq_create(struct drm_file *filp, union drm_amdgpu_userq *args)
 		r = -EINVAL;
 		goto unlock;
 	}
+	/* Validate the userq virtual address.*/
+	if (amdgpu_userq_input_va_validate(&fpriv->vm, args->in.queue_va, args->in.queue_size) ||
+	    amdgpu_userq_input_va_validate(&fpriv->vm, args->in.rptr_va, PAGE_SIZE) ||
+	    amdgpu_userq_input_va_validate(&fpriv->vm, args->in.wptr_va, PAGE_SIZE)) {
+		drm_file_err(uq_mgr->file, "Usermode queue input virt address is invalid\n");
+		r = -EINVAL;
+		goto unlock;
+	}
 
 	queue = kzalloc(sizeof(struct amdgpu_usermode_queue), GFP_KERNEL);
 	if (!queue) {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
index ec040c2fd6c9..704935ca0c36 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
@@ -132,4 +132,6 @@ int amdgpu_userq_stop_sched_for_enforce_isolation(struct amdgpu_device *adev,
 int amdgpu_userq_start_sched_for_enforce_isolation(struct amdgpu_device *adev,
 						   u32 idx);
 
+int amdgpu_userq_input_va_validate(struct amdgpu_vm *vm, u64 addr,
+			u64 expected_size);
 #endif
diff --git a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
index 15aa1ca67a11..75b9a6294b53 100644
--- a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
+++ b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
@@ -206,6 +206,7 @@ static int mes_userq_mqd_create(struct amdgpu_userq_mgr *uq_mgr,
 	struct amdgpu_mqd *mqd_hw_default = &adev->mqds[queue->queue_type];
 	struct drm_amdgpu_userq_in *mqd_user = args_in;
 	struct amdgpu_mqd_prop *userq_props;
+	struct amdgpu_gfx_shadow_info shadow_info;
 	int r;
 
 	/* Structure to initialize MQD for userqueue using generic MQD init function */
@@ -231,6 +232,8 @@ static int mes_userq_mqd_create(struct amdgpu_userq_mgr *uq_mgr,
 	userq_props->doorbell_index = queue->doorbell_index;
 	userq_props->fence_address = queue->fence_drv->gpu_addr;
 
+	if (adev->gfx.funcs->get_gfx_shadow_info)
+		adev->gfx.funcs->get_gfx_shadow_info(adev, &shadow_info, true);
 	if (queue->queue_type == AMDGPU_HW_IP_COMPUTE) {
 		struct drm_amdgpu_userq_mqd_compute_gfx11 *compute_mqd;
 
@@ -247,6 +250,13 @@ static int mes_userq_mqd_create(struct amdgpu_userq_mgr *uq_mgr,
 			goto free_mqd;
 		}
 
+		if (amdgpu_userq_input_va_validate(queue->vm, compute_mqd->eop_va,
+					max_t(u32, PAGE_SIZE, AMDGPU_GPU_PAGE_SIZE))) {
+			drm_file_err(uq_mgr->file, "EOP VA is invalid\n");
+			r = -EINVAL;
+			goto free_mqd;
+		}
+
 		userq_props->eop_gpu_addr = compute_mqd->eop_va;
 		userq_props->hqd_pipe_priority = AMDGPU_GFX_PIPE_PRIO_NORMAL;
 		userq_props->hqd_queue_priority = AMDGPU_GFX_QUEUE_PRIORITY_MINIMUM;
@@ -274,6 +284,14 @@ static int mes_userq_mqd_create(struct amdgpu_userq_mgr *uq_mgr,
 		userq_props->csa_addr = mqd_gfx_v11->csa_va;
 		userq_props->tmz_queue =
 			mqd_user->flags & AMDGPU_USERQ_CREATE_FLAGS_QUEUE_SECURE;
+
+		if (amdgpu_userq_input_va_validate(queue->vm, mqd_gfx_v11->shadow_va,
+					shadow_info.shadow_size)) {
+			drm_file_err(uq_mgr->file, "shadow VA is invalid\n");
+			r = -EINVAL;
+			goto free_mqd;
+		}
+
 		kfree(mqd_gfx_v11);
 	} else if (queue->queue_type == AMDGPU_HW_IP_DMA) {
 		struct drm_amdgpu_userq_mqd_sdma_gfx11 *mqd_sdma_v11;
@@ -291,6 +309,13 @@ static int mes_userq_mqd_create(struct amdgpu_userq_mgr *uq_mgr,
 			goto free_mqd;
 		}
 
+		if (amdgpu_userq_input_va_validate(queue->vm, mqd_sdma_v11->csa_va,
+					shadow_info.csa_size)) {
+			drm_file_err(uq_mgr->file, "CSA VA is invalid\n");
+			r = -EINVAL;
+			goto free_mqd;
+		}
+
 		userq_props->csa_addr = mqd_sdma_v11->csa_va;
 		kfree(mqd_sdma_v11);
 	}
-- 
2.34.1

[PATCH v6 05/11] drm/amdgpu: add userq object va track helpers

[Prike Liang]

Add the userq object virtual address get(),mapped() and put()
helpers for tracking the userq obj va address usage.

Signed-off-by: Prike Liang <Prike.Liang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c  | 172 ++++++++++++++++++++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h  |  14 ++
 drivers/gpu/drm/amd/amdgpu/mes_userqueue.c |   4 +
 3 files changed, 189 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
index a41dd38b0adb..2856c2506bee 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
@@ -74,6 +74,174 @@ int amdgpu_userq_input_va_validate(struct amdgpu_vm *vm, u64 addr,
 	return -EINVAL;
 }
 
+int amdgpu_userq_buffer_va_get(struct amdgpu_vm *vm, u64 addr)
+{
+	struct amdgpu_bo_va_mapping *mapping;
+	u64 user_addr;
+	int r;
+
+	user_addr = (addr & AMDGPU_GMC_HOLE_MASK) >> AMDGPU_GPU_PAGE_SHIFT;
+	r = amdgpu_bo_reserve(vm->root.bo, false);
+	if (r)
+		return r;
+
+	mapping = amdgpu_vm_bo_lookup_mapping(vm, user_addr);
+	if (!mapping)
+		goto out_err;
+
+	/*
+	 * Need to unify the following userq va reference.
+	 *  mqd  bo
+	 *  rptr bo
+	 *  wptr bo
+	 *  eop  bo
+	 *  shadow bo
+	 *  csa bo
+	 */
+	/*amdgpu_bo_ref(mapping->bo_va->base.bo);*/
+	mapping->bo_va->queue_refcount++;
+
+	amdgpu_bo_unreserve(vm->root.bo);
+	return 0;
+
+out_err:
+	amdgpu_bo_unreserve(vm->root.bo);
+	return -EINVAL;
+}
+
+bool amdgpu_userq_buffer_va_mapped(struct amdgpu_vm *vm, u64 addr)
+{
+	struct amdgpu_bo_va_mapping *mapping;
+	u64 user_addr;
+	bool r;
+
+	user_addr = (addr & AMDGPU_GMC_HOLE_MASK) >> AMDGPU_GPU_PAGE_SHIFT;
+
+	if (amdgpu_bo_reserve(vm->root.bo, false))
+		return false;
+
+	mapping = amdgpu_vm_bo_lookup_mapping(vm, user_addr);
+	if (!IS_ERR_OR_NULL(mapping) && mapping->bo_va->queue_refcount > 0)
+		r = true;
+	else
+		r = false;
+	amdgpu_bo_unreserve(vm->root.bo);
+
+	return r;
+}
+
+bool amdgpu_userq_buffer_vas_mapped(struct amdgpu_vm *vm,
+			struct amdgpu_usermode_queue *queue)
+{
+
+	switch (queue->queue_type) {
+	case AMDGPU_HW_IP_GFX:
+		if (amdgpu_userq_buffer_va_mapped(vm, queue->queue_va) ||
+		    amdgpu_userq_buffer_va_mapped(vm, queue->rptr_va) ||
+		    amdgpu_userq_buffer_va_mapped(vm, queue->wptr_va) ||
+		    amdgpu_userq_buffer_va_mapped(vm, queue->shadow_va) ||
+		    amdgpu_userq_buffer_va_mapped(vm, queue->csa_va))
+			return true;
+		break;
+	case AMDGPU_HW_IP_COMPUTE:
+		if (amdgpu_userq_buffer_va_mapped(vm, queue->queue_va) ||
+		    amdgpu_userq_buffer_va_mapped(vm, queue->rptr_va) ||
+		    amdgpu_userq_buffer_va_mapped(vm, queue->wptr_va) ||
+		    amdgpu_userq_buffer_va_mapped(vm, queue->eop_va))
+			return true;
+		break;
+	case AMDGPU_HW_IP_DMA:
+		if (amdgpu_userq_buffer_va_mapped(vm, queue->queue_va) ||
+		    amdgpu_userq_buffer_va_mapped(vm, queue->rptr_va) ||
+		    amdgpu_userq_buffer_va_mapped(vm, queue->wptr_va) ||
+		    amdgpu_userq_buffer_va_mapped(vm, queue->csa_va))
+			return true;
+		break;
+	default:
+		break;
+	}
+
+	return false;
+}
+
+int amdgpu_userq_buffer_va_put(struct amdgpu_vm *vm, u64 addr)
+{
+	struct amdgpu_bo_va_mapping *mapping;
+	u64 user_addr;
+	int r;
+
+	user_addr = (addr & AMDGPU_GMC_HOLE_MASK) >> AMDGPU_GPU_PAGE_SHIFT;
+	r = amdgpu_bo_reserve(vm->root.bo, false);
+	if (r)
+		return r;
+
+	mapping = amdgpu_vm_bo_lookup_mapping(vm, user_addr);
+	if (!mapping)
+		goto out_err;
+	/*
+	 * TODO: It requires figuring out the root cause of userq va mapping
+	 * reference imbalance issue.
+	 */
+	/*amdgpu_bo_unref(&mapping->bo_va->base.bo);*/
+	mapping->bo_va->queue_refcount--;
+
+	amdgpu_bo_unreserve(vm->root.bo);
+	return 0;
+
+out_err:
+	amdgpu_bo_unreserve(vm->root.bo);
+	return -EINVAL;
+}
+
+static void amdgpu_userq_buffer_vas_get(struct amdgpu_vm *vm,
+			struct amdgpu_usermode_queue *queue)
+{
+
+
+	amdgpu_userq_buffer_va_get(vm, queue->queue_va);
+	amdgpu_userq_buffer_va_get(vm, queue->rptr_va);
+	amdgpu_userq_buffer_va_get(vm, queue->wptr_va);
+
+	switch (queue->queue_type) {
+	case AMDGPU_HW_IP_GFX:
+		amdgpu_userq_buffer_va_get(vm, queue->shadow_va);
+		amdgpu_userq_buffer_va_get(vm, queue->csa_va);
+		break;
+	case AMDGPU_HW_IP_COMPUTE:
+		amdgpu_userq_buffer_va_get(vm, queue->eop_va);
+		break;
+	case AMDGPU_HW_IP_DMA:
+		amdgpu_userq_buffer_va_get(vm, queue->csa_va);
+		break;
+	default:
+		break;
+	}
+}
+
+int amdgpu_userq_buffer_vas_put(struct amdgpu_vm *vm,
+			struct amdgpu_usermode_queue *queue)
+{
+	amdgpu_userq_buffer_va_put(vm, queue->queue_va);
+	amdgpu_userq_buffer_va_put(vm, queue->rptr_va);
+	amdgpu_userq_buffer_va_put(vm, queue->wptr_va);
+
+	switch (queue->queue_type) {
+	case AMDGPU_HW_IP_GFX:
+		amdgpu_userq_buffer_va_put(vm, queue->shadow_va);
+		amdgpu_userq_buffer_va_put(vm, queue->csa_va);
+		break;
+	case AMDGPU_HW_IP_COMPUTE:
+		amdgpu_userq_buffer_va_put(vm, queue->eop_va);
+		break;
+	case AMDGPU_HW_IP_DMA:
+		amdgpu_userq_buffer_va_put(vm, queue->csa_va);
+		break;
+	default:
+		break;
+	}
+	return 0;
+}
+
 static int
 amdgpu_userq_unmap_helper(struct amdgpu_userq_mgr *uq_mgr,
 			  struct amdgpu_usermode_queue *queue)
@@ -435,6 +603,9 @@ amdgpu_userq_create(struct drm_file *filp, union drm_amdgpu_userq *args)
 	queue->queue_type = args->in.ip_type;
 	queue->vm = &fpriv->vm;
 	queue->priority = priority;
+	queue->queue_va = args->in.queue_va;
+	queue->rptr_va = args->in.rptr_va;
+	queue->wptr_va = args->in.wptr_va;
 
 	db_info.queue_type = queue->queue_type;
 	db_info.doorbell_handle = queue->doorbell_handle;
@@ -465,7 +636,6 @@ amdgpu_userq_create(struct drm_file *filp, union drm_amdgpu_userq *args)
 		goto unlock;
 	}
 
-
 	qid = idr_alloc(&uq_mgr->userq_idr, queue, 1, AMDGPU_MAX_USERQ_COUNT, GFP_KERNEL);
 	if (qid < 0) {
 		drm_file_err(uq_mgr->file, "Failed to allocate a queue id\n");
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
index 704935ca0c36..194ec7a6b3b2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
@@ -52,6 +52,13 @@ struct amdgpu_usermode_queue {
 	enum amdgpu_userq_state state;
 	uint64_t		doorbell_handle;
 	uint64_t		doorbell_index;
+	uint64_t		queue_va;
+	uint64_t		rptr_va;
+	uint64_t		wptr_va;
+	uint64_t		eop_va;
+	uint64_t		shadow_va;
+	uint64_t		csa_va;
+
 	uint64_t		flags;
 	struct amdgpu_mqd_prop	*userq_prop;
 	struct amdgpu_userq_mgr *userq_mgr;
@@ -134,4 +141,11 @@ int amdgpu_userq_start_sched_for_enforce_isolation(struct amdgpu_device *adev,
 
 int amdgpu_userq_input_va_validate(struct amdgpu_vm *vm, u64 addr,
 			u64 expected_size);
+int amdgpu_userq_buffer_va_get(struct amdgpu_vm *vm, u64 addr);
+bool amdgpu_userq_buffer_va_mapped(struct amdgpu_vm *vm, u64 addr);
+bool amdgpu_userq_buffer_vas_mapped(struct amdgpu_vm *vm,
+			struct amdgpu_usermode_queue *queue);
+int amdgpu_userq_buffer_va_put(struct amdgpu_vm *vm, u64 addr);
+int amdgpu_userq_buffer_vas_put(struct amdgpu_vm *vm,
+			struct amdgpu_usermode_queue *queue);
 #endif
diff --git a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
index 75b9a6294b53..8c86c4f4f28c 100644
--- a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
+++ b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
@@ -263,6 +263,7 @@ static int mes_userq_mqd_create(struct amdgpu_userq_mgr *uq_mgr,
 		userq_props->hqd_active = false;
 		userq_props->tmz_queue =
 			mqd_user->flags & AMDGPU_USERQ_CREATE_FLAGS_QUEUE_SECURE;
+		queue->eop_va = compute_mqd->eop_va;
 		kfree(compute_mqd);
 	} else if (queue->queue_type == AMDGPU_HW_IP_GFX) {
 		struct drm_amdgpu_userq_mqd_gfx11 *mqd_gfx_v11;
@@ -284,6 +285,8 @@ static int mes_userq_mqd_create(struct amdgpu_userq_mgr *uq_mgr,
 		userq_props->csa_addr = mqd_gfx_v11->csa_va;
 		userq_props->tmz_queue =
 			mqd_user->flags & AMDGPU_USERQ_CREATE_FLAGS_QUEUE_SECURE;
+		queue->shadow_va = mqd_gfx_v11->shadow_va;
+		queue->csa_va = mqd_gfx_v11->csa_va;
 
 		if (amdgpu_userq_input_va_validate(queue->vm, mqd_gfx_v11->shadow_va,
 					shadow_info.shadow_size)) {
@@ -317,6 +320,7 @@ static int mes_userq_mqd_create(struct amdgpu_userq_mgr *uq_mgr,
 		}
 
 		userq_props->csa_addr = mqd_sdma_v11->csa_va;
+		queue->csa_va = mqd_sdma_v11->csa_va;
 		kfree(mqd_sdma_v11);
 	}
 
-- 
2.34.1

[PATCH v6 06/11] drm/amdgpu: track the userq bo va for its obj management

[Prike Liang]

The user queue object destroy requires ensuring its
VA keeps mapping prior to the queue being destroyed.
Otherwise, it seems a bug in the user space or VA
freed wrongly, and the kernel driver should report an
invalidated error to the user IOCLT request.

Signed-off-by: Prike Liang <Prike.Liang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
index 2856c2506bee..81fbb00b6d91 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
@@ -510,12 +510,24 @@ amdgpu_userq_destroy(struct drm_file *filp, int queue_id)
 		return -EINVAL;
 	}
 	amdgpu_userq_wait_for_last_fence(uq_mgr, queue);
+
+	/*
+	 * At this point the userq obj va should be mapped,
+	 * otherwise will return error to user.
+	 */
+	if (!amdgpu_userq_buffer_vas_mapped(&fpriv->vm, queue)) {
+		drm_warn(adev_to_drm(uq_mgr->adev), "the userq obj va shouldn't be umapped here\n");
+		r = -EINVAL;
+	}
+
 	r = amdgpu_userq_unmap_helper(uq_mgr, queue);
 	/*TODO: It requires a reset for userq hw unmap error*/
 	if (unlikely(r != AMDGPU_USERQ_STATE_UNMAPPED)) {
 		drm_warn(adev_to_drm(uq_mgr->adev), "trying to destroy a HW mapping userq\n");
 		r = -ETIMEDOUT;
 	}
+
+	amdgpu_userq_buffer_vas_put(&fpriv->vm, queue);
 	amdgpu_userq_cleanup(uq_mgr, queue, queue_id);
 	mutex_unlock(&uq_mgr->userq_mutex);
 
@@ -636,6 +648,9 @@ amdgpu_userq_create(struct drm_file *filp, union drm_amdgpu_userq *args)
 		goto unlock;
 	}
 
+	/* refer to the userq objects vm bo*/
+	amdgpu_userq_buffer_vas_get(queue->vm, queue);
+
 	qid = idr_alloc(&uq_mgr->userq_idr, queue, 1, AMDGPU_MAX_USERQ_COUNT, GFP_KERNEL);
 	if (qid < 0) {
 		drm_file_err(uq_mgr->file, "Failed to allocate a queue id\n");
-- 
2.34.1

[PATCH v6 07/11] drm/amdgpu: validate userq's last fence prior to destroying

[Prike Liang]

The userq requires validating queue status before destroying
it, if user tries to destroy a busy userq by IOCTL then the
driver should report an error for this illegal usage.

Signed-off-by: Prike Liang <Prike.Liang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
index 81fbb00b6d91..bcbe8d3f66ed 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
@@ -281,7 +281,7 @@ amdgpu_userq_map_helper(struct amdgpu_userq_mgr *uq_mgr,
 	return r;
 }
 
-static void
+static int
 amdgpu_userq_wait_for_last_fence(struct amdgpu_userq_mgr *uq_mgr,
 				 struct amdgpu_usermode_queue *queue)
 {
@@ -290,10 +290,14 @@ amdgpu_userq_wait_for_last_fence(struct amdgpu_userq_mgr *uq_mgr,
 
 	if (f && !dma_fence_is_signaled(f)) {
 		ret = dma_fence_wait_timeout(f, true, msecs_to_jiffies(100));
-		if (ret <= 0)
+		if (ret <= 0) {
 			drm_file_err(uq_mgr->file, "Timed out waiting for fence=%llu:%llu\n",
 				     f->context, f->seqno);
+			return -ETIMEDOUT;
+		}
 	}
+
+	return 0;
 }
 
 static void
@@ -509,7 +513,12 @@ amdgpu_userq_destroy(struct drm_file *filp, int queue_id)
 		mutex_unlock(&uq_mgr->userq_mutex);
 		return -EINVAL;
 	}
-	amdgpu_userq_wait_for_last_fence(uq_mgr, queue);
+
+	if (amdgpu_userq_wait_for_last_fence(uq_mgr, queue)) {
+		drm_warn(adev_to_drm(uq_mgr->adev), "Don't destroy a busy userq\n");
+		/* For the fence signal timeout case, it requires resetting the busy queue.*/
+		r = -ETIMEDOUT;
+	}
 
 	/*
 	 * At this point the userq obj va should be mapped,
-- 
2.34.1

[PATCH v6 08/11] drm/amdgpu: clean up the amdgpu_userq_active()

[Prike Liang]

This is no invocation for amdgpu_userq_active().

Signed-off-by: Prike Liang <Prike.Liang@amd.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 16 ----------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h |  2 --
 2 files changed, 18 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
index bcbe8d3f66ed..bfe7b229011e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
@@ -314,22 +314,6 @@ amdgpu_userq_cleanup(struct amdgpu_userq_mgr *uq_mgr,
 	kfree(queue);
 }
 
-int
-amdgpu_userq_active(struct amdgpu_userq_mgr *uq_mgr)
-{
-	struct amdgpu_usermode_queue *queue;
-	int queue_id;
-	int ret = 0;
-
-	mutex_lock(&uq_mgr->userq_mutex);
-	/* Resume all the queues for this process */
-	idr_for_each_entry(&uq_mgr->userq_idr, queue, queue_id)
-		ret += queue->state == AMDGPU_USERQ_STATE_MAPPED;
-
-	mutex_unlock(&uq_mgr->userq_mutex);
-	return ret;
-}
-
 static struct amdgpu_usermode_queue *
 amdgpu_userq_find(struct amdgpu_userq_mgr *uq_mgr, int qid)
 {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
index 194ec7a6b3b2..ca6ede32b260 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
@@ -120,8 +120,6 @@ void amdgpu_userq_destroy_object(struct amdgpu_userq_mgr *uq_mgr,
 void amdgpu_userq_evict(struct amdgpu_userq_mgr *uq_mgr,
 			struct amdgpu_eviction_fence *ev_fence);
 
-int amdgpu_userq_active(struct amdgpu_userq_mgr *uq_mgr);
-
 void amdgpu_userq_ensure_ev_fence(struct amdgpu_userq_mgr *userq_mgr,
 				  struct amdgpu_eviction_fence_mgr *evf_mgr);
 
-- 
2.34.1

[PATCH v6 09/11] drm/amdgpu: validate the shared bo for tracking usage size

[Prike Liang]

It requires validating the shared BO before updating its usage
size; otherwise, there is a potential NULL pointer error when the
BO released improperly.

Signed-off-by: Prike Liang <Prike.Liang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index f042372d9f2e..a574effdd3ec 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -321,12 +321,26 @@ static void amdgpu_vm_bo_reset_state_machine(struct amdgpu_vm *vm)
  */
 static void amdgpu_vm_update_shared(struct amdgpu_vm_bo_base *base)
 {
-	struct amdgpu_vm *vm = base->vm;
-	struct amdgpu_bo *bo = base->bo;
-	uint64_t size = amdgpu_bo_size(bo);
-	uint32_t bo_memtype = amdgpu_bo_mem_stats_placement(bo);
+	struct amdgpu_vm *vm;
+	struct amdgpu_bo *bo;
+	uint64_t size = 0;
+	uint32_t bo_memtype = TTM_PL_SYSTEM;
 	bool shared;
 
+	if (likely(base)) {
+		vm = base->vm;
+		bo = base->bo;
+	} else {
+		return;
+	}
+
+	if (likely(bo)) {
+		size = amdgpu_bo_size(bo);
+		bo_memtype = amdgpu_bo_mem_stats_placement(bo);
+	} else {
+		return;
+	}
+
 	spin_lock(&vm->status_lock);
 	shared = drm_gem_object_is_shared_for_memory_stats(&bo->tbo.base);
 	if (base->shared != shared) {
@@ -353,6 +367,9 @@ void amdgpu_vm_bo_update_shared(struct amdgpu_bo *bo)
 {
 	struct amdgpu_vm_bo_base *base;
 
+	if (unlikely(!bo))
+		return;
+
 	for (base = bo->vm_bo; base; base = base->next)
 		amdgpu_vm_update_shared(base);
 }
-- 
2.34.1

[PATCH v6 10/11] drm/amdgpu: validate the queue va for resuming the queue

[Prike Liang]

It requires validating the userq VA whether is mapped before
trying to resume the queue.

Signed-off-by: Prike Liang <Prike.Liang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
index bfe7b229011e..54f44fc834fe 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
@@ -777,11 +777,18 @@ static int
 amdgpu_userq_restore_all(struct amdgpu_userq_mgr *uq_mgr)
 {
 	struct amdgpu_usermode_queue *queue;
+	struct amdgpu_fpriv *fpriv = uq_mgr_to_fpriv(uq_mgr);
 	int queue_id;
 	int ret = 0, r;
 
 	/* Resume all the queues for this process */
 	idr_for_each_entry(&uq_mgr->userq_idr, queue, queue_id) {
+
+		if (!amdgpu_userq_buffer_vas_mapped(&fpriv->vm, queue)) {
+			drm_file_err(uq_mgr->file, "trying restore queue without va mappping\n");
+			continue;
+		}
+
 		r = amdgpu_userq_map_helper(uq_mgr, queue);
 		if (r)
 			ret = r;
-- 
2.34.1

[PATCH v6 11/11] drm/amdgpu: validate userq va for GEM unmap

[Prike Liang]

This change validates the userq to see whether can be
unmapped prior to the userq VA GEM unmap. The solution
is based on the following idea:
1) Find out the GEM unmap VA belonds to which userq,
2) Wait the userq fence and eviction fence signal,
3) If attached fence signal, then suspend the userq
   to avoid reusing by userspace,
4) If the userq attached fences signal failed, then
   return an error code and give a warning message
   for this illegal userspace request.

Suggested-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Prike Liang <Prike.Liang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 119 ++++++++++++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h |   2 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c    |  10 ++
 3 files changed, 131 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
index 54f44fc834fe..491a65e746b3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
@@ -1175,3 +1175,122 @@ int amdgpu_userq_start_sched_for_enforce_isolation(struct amdgpu_device *adev,
 	mutex_unlock(&adev->userq_mutex);
 	return ret;
 }
+
+/**
+ * amdgpu_userq_gem_va_unmap_queue_retrieve - find out userq by gem unmap va
+ * @queue: destinated userq for finding out from unmap va
+ * @va: the GEM unmap virtual address already aligned in mapping range
+ * Find out the corresponding userq by comparing
+ * the GEM unmap VA with userq VAs.
+ */
+static bool amdgpu_userq_gem_va_unmap_queue_retrieve(struct amdgpu_usermode_queue *queue,
+                                                       uint64_t va)
+{
+	va = va << AMDGPU_GPU_PAGE_SHIFT | AMDGPU_GMC_HOLE_END;
+
+	switch (queue->queue_type) {
+	case AMDGPU_HW_IP_GFX:
+		if (queue->queue_va == va ||
+		    queue->wptr_va  == va ||
+		    queue->rptr_va  == va ||
+		    queue->shadow_va == va ||
+		    queue->csa_va  == va)
+			return true;
+		break;
+	case AMDGPU_HW_IP_COMPUTE:
+		if (queue->queue_va == va ||
+		    queue->wptr_va == va ||
+		    queue->rptr_va  == va ||
+		    queue->eop_va  == va)
+			return true;
+		break;
+	case AMDGPU_HW_IP_DMA:
+		if (queue->queue_va == va ||
+		    queue->wptr_va == va ||
+		    queue->rptr_va == va ||
+		    queue->csa_va == va)
+			return true;
+		break;
+	default:
+		break;
+	}
+
+	return false;
+}
+
+
+int amdgpu_userq_gem_va_unmap_validate(struct amdgpu_device *adev,
+				uint64_t va)
+{
+	u32 ip_mask = amdgpu_userq_get_supported_ip_mask(adev);
+	struct amdgpu_usermode_queue *queue;
+	struct amdgpu_userq_mgr *uqm, *tmp;
+	int queue_id;
+	int ret;
+
+	if (!ip_mask)
+		return 0;
+
+	/**
+	 * validate the unmap va sequence:
+	 * 1) Find out the GEM unmap VA belonds to which userq,
+	 * 2) Wait the userq fence and eviction fence signal,
+	 * 3) If attached fence signal, then suspend the userq
+	 *    to avoid reusing by userspace,
+	 * 4) If the userq attached fences signal failed, then
+	 *    return an error code and give a warning message
+	 *    for this illegal userspace request.
+	 */
+
+	if (mutex_trylock(&adev->userq_mutex)) {
+		list_for_each_entry_safe(uqm, tmp, &adev->userq_mgr_list, list) {
+
+			if (!mutex_trylock(&uqm->userq_mutex))
+				continue;
+
+			idr_for_each_entry(&uqm->userq_idr, queue, queue_id) {
+				struct amdgpu_fpriv *fpriv = uq_mgr_to_fpriv(uqm);
+				struct amdgpu_eviction_fence_mgr *evf_mgr = &fpriv->evf_mgr;
+				struct amdgpu_eviction_fence *ev_fence;
+
+				if (!amdgpu_userq_gem_va_unmap_queue_retrieve(queue, va)) {
+					dev_dbg(uqm->adev->dev, "va: 0x%llx not belond to queue id: %d\n",
+						va, queue_id);
+					continue;
+				}
+
+				if (amdgpu_userq_wait_for_last_fence(uqm, queue)) {
+					drm_file_err(uqm->file, "userq fence signaled failed during unmapping its va\n");
+					ret = -ETIMEDOUT;
+					goto err;
+				}
+
+				spin_lock(&evf_mgr->ev_fence_lock);
+				ev_fence = evf_mgr->ev_fence;
+				spin_unlock(&evf_mgr->ev_fence_lock);
+				if (ev_fence && !dma_fence_is_signaled(&ev_fence->base)) {
+					ret = dma_fence_wait_timeout(&ev_fence->base, true, msecs_to_jiffies(100));
+					if (ret <= 0) {
+						dev_dbg(uqm->adev->dev, "the userq eviction fence signaled\t"
+								"failed during unmapping its va\n");
+						ret = -EBUSY;
+						goto err;
+					}
+				}
+				/* Here needs to deactivate and prevent reusing it*/
+				amdgpu_userq_unmap_helper(uqm, queue);
+			}
+			mutex_unlock(&uqm->userq_mutex);
+		}
+	} else {
+			/* do we need a try lock again before return*/
+			return -EBUSY;
+	}
+
+	mutex_unlock(&adev->userq_mutex);
+	return 0;
+err:
+	mutex_unlock(&uqm->userq_mutex);
+	mutex_unlock(&adev->userq_mutex);
+	return ret;
+}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
index ca6ede32b260..2aad5317b678 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
@@ -146,4 +146,6 @@ bool amdgpu_userq_buffer_vas_mapped(struct amdgpu_vm *vm,
 int amdgpu_userq_buffer_va_put(struct amdgpu_vm *vm, u64 addr);
 int amdgpu_userq_buffer_vas_put(struct amdgpu_vm *vm,
 			struct amdgpu_usermode_queue *queue);
+int amdgpu_userq_gem_va_unmap_validate(struct amdgpu_device *adev,
+				uint64_t va);
 #endif
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index a574effdd3ec..a34b96f2cedb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1946,6 +1946,7 @@ int amdgpu_vm_bo_unmap(struct amdgpu_device *adev,
 	struct amdgpu_bo_va_mapping *mapping;
 	struct amdgpu_vm *vm = bo_va->base.vm;
 	bool valid = true;
+	int r;
 
 	saddr /= AMDGPU_GPU_PAGE_SIZE;
 
@@ -1966,6 +1967,15 @@ int amdgpu_vm_bo_unmap(struct amdgpu_device *adev,
 			return -ENOENT;
 	}
 
+	/* It's unlikely to happen that the mapping userq hasn't been idled
+	 * during user requests GEM unmap IOCTL except for forcing the unmap
+	 * from user space.
+	 */
+
+	r = amdgpu_userq_gem_va_unmap_validate(adev, saddr);
+	if (unlikely(r && r != -EBUSY))
+		dev_warn(adev->dev, "Here should be an improper unmap request from user space\n");
+
 	list_del(&mapping->list);
 	amdgpu_vm_it_remove(mapping, &vm->va);
 	mapping->bo_va = NULL;
-- 
2.34.1

Re: [PATCH v6 03/11] drm/amdgpu: rework the userq doorbell object destroy

[Christian König]

On 11.07.25 11:39, Prike Liang wrote:
> This patch aims to unify and destroy the userq doorbell objects at
> mes_userq_mqd_destroy(), and this change will also help with unpinning
> and destroying the userq doorbell objects for amdgpu_userq_mgr_fini()
> during releasing the drm files.
> 
> Signed-off-by: Prike Liang <Prike.Liang@amd.com>
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c  | 6 ------
>  drivers/gpu/drm/amd/amdgpu/mes_userqueue.c | 7 +++++++
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> index 3d2a7f8946cf..15e833b1b3e3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> @@ -312,12 +312,6 @@ amdgpu_userq_destroy(struct drm_file *filp, int queue_id)
>  		return -EINVAL;
>  	}
>  	amdgpu_userq_wait_for_last_fence(uq_mgr, queue);
> -	r = amdgpu_bo_reserve(queue->db_obj.obj, true);
> -	if (!r) {
> -		amdgpu_bo_unpin(queue->db_obj.obj);
> -		amdgpu_bo_unreserve(queue->db_obj.obj);
> -	}
> -	amdgpu_bo_unref(&queue->db_obj.obj);
>  	r = amdgpu_userq_unmap_helper(uq_mgr, queue);
>  	/*TODO: It requires a reset for userq hw unmap error*/
>  	if (unlikely(r != AMDGPU_USERQ_STATE_UNMAPPED)) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
> index 1457fb49a794..15aa1ca67a11 100644
> --- a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
> +++ b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
> @@ -336,6 +336,13 @@ mes_userq_mqd_destroy(struct amdgpu_userq_mgr *uq_mgr,
>  		      struct amdgpu_usermode_queue *queue)
>  {
>  	amdgpu_userq_destroy_object(uq_mgr, &queue->fw_obj);
> +
> +	if (!amdgpu_bo_reserve(queue->db_obj.obj, true)) {
> +		amdgpu_bo_unpin(queue->db_obj.obj);
> +		amdgpu_bo_unreserve(queue->db_obj.obj);
> +		amdgpu_userq_destroy_object(uq_mgr, &queue->db_obj);
> +	}
> +

That makes no sense to do here. The pinning isn't done in mes_userq_mqd_create() either.

In general we should avoid pinning the MQD in the first place, that buffer needs to be fences instead.

Regards,
Christian.

>  	kfree(queue->userq_prop);
>  	amdgpu_userq_destroy_object(uq_mgr, &queue->mqd);
>  }

Re: [PATCH v6 04/11] drm/amdgpu: validate userq buffer virtual address and size

[Christian König]

On 11.07.25 11:39, Prike Liang wrote:
> It needs to validate the userq object virtual address to
> determin whether it is residented in a valid vm mapping.
> 
> Signed-off-by: Prike Liang <Prike.Liang@amd.com>
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c  | 38 ++++++++++++++++++++++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h  |  2 ++
>  drivers/gpu/drm/amd/amdgpu/mes_userqueue.c | 25 ++++++++++++++
>  3 files changed, 65 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> index 15e833b1b3e3..a41dd38b0adb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> @@ -44,6 +44,36 @@ u32 amdgpu_userq_get_supported_ip_mask(struct amdgpu_device *adev)
>  	return userq_ip_mask;
>  }
>  
> +int amdgpu_userq_input_va_validate(struct amdgpu_vm *vm, u64 addr,
> +				u64 expected_size)
> +{
> +	struct amdgpu_bo_va_mapping *va_map;
> +	u64 user_addr;
> +	u64 size;
> +	int r;
> +
> +	user_addr = (addr & AMDGPU_GMC_HOLE_MASK) >> AMDGPU_GPU_PAGE_SHIFT;
> +	size = expected_size >> AMDGPU_GPU_PAGE_SHIFT;
> +
> +	r = amdgpu_bo_reserve(vm->root.bo, false);
> +	if (r)
> +		return r;
> +
> +	va_map = amdgpu_vm_bo_lookup_mapping(vm, user_addr);
> +	if (!va_map)
> +		goto out_err;
> +	/* Only validate the userq whether resident in the VM mapping range */
> +	if (user_addr >= va_map->start &&

This check is unecessary.

> +	    (size != 0 && user_addr + size - 1 <= va_map->last)) {

The size != 0 check is unecessary as well and you need to be careful with wrap arounds.

Better write this like that (va_map->last - user_addr + 1 >= size) 

> +		amdgpu_bo_unreserve(vm->root.bo);
> +		return 0;
> +	}
> +
> +out_err:
> +	amdgpu_bo_unreserve(vm->root.bo);
> +	return -EINVAL;
> +}
> +
>  static int
>  amdgpu_userq_unmap_helper(struct amdgpu_userq_mgr *uq_mgr,
>  			  struct amdgpu_usermode_queue *queue)
> @@ -386,6 +416,14 @@ amdgpu_userq_create(struct drm_file *filp, union drm_amdgpu_userq *args)
>  		r = -EINVAL;
>  		goto unlock;
>  	}
> +	/* Validate the userq virtual address.*/
> +	if (amdgpu_userq_input_va_validate(&fpriv->vm, args->in.queue_va, args->in.queue_size) ||
> +	    amdgpu_userq_input_va_validate(&fpriv->vm, args->in.rptr_va, PAGE_SIZE) ||
> +	    amdgpu_userq_input_va_validate(&fpriv->vm, args->in.wptr_va, PAGE_SIZE)) {
> +		drm_file_err(uq_mgr->file, "Usermode queue input virt address is invalid\n");

No error message on invalid userspace parameters please.

Apart from those comments looks like the right thing to do to me.

Regards,
Christian.

> +		r = -EINVAL;
> +		goto unlock;
> +	}
>  
>  	queue = kzalloc(sizeof(struct amdgpu_usermode_queue), GFP_KERNEL);
>  	if (!queue) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
> index ec040c2fd6c9..704935ca0c36 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
> @@ -132,4 +132,6 @@ int amdgpu_userq_stop_sched_for_enforce_isolation(struct amdgpu_device *adev,
>  int amdgpu_userq_start_sched_for_enforce_isolation(struct amdgpu_device *adev,
>  						   u32 idx);
>  
> +int amdgpu_userq_input_va_validate(struct amdgpu_vm *vm, u64 addr,
> +			u64 expected_size);
>  #endif
> diff --git a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
> index 15aa1ca67a11..75b9a6294b53 100644
> --- a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
> +++ b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
> @@ -206,6 +206,7 @@ static int mes_userq_mqd_create(struct amdgpu_userq_mgr *uq_mgr,
>  	struct amdgpu_mqd *mqd_hw_default = &adev->mqds[queue->queue_type];
>  	struct drm_amdgpu_userq_in *mqd_user = args_in;
>  	struct amdgpu_mqd_prop *userq_props;
> +	struct amdgpu_gfx_shadow_info shadow_info;
>  	int r;
>  
>  	/* Structure to initialize MQD for userqueue using generic MQD init function */
> @@ -231,6 +232,8 @@ static int mes_userq_mqd_create(struct amdgpu_userq_mgr *uq_mgr,
>  	userq_props->doorbell_index = queue->doorbell_index;
>  	userq_props->fence_address = queue->fence_drv->gpu_addr;
>  
> +	if (adev->gfx.funcs->get_gfx_shadow_info)
> +		adev->gfx.funcs->get_gfx_shadow_info(adev, &shadow_info, true);
>  	if (queue->queue_type == AMDGPU_HW_IP_COMPUTE) {
>  		struct drm_amdgpu_userq_mqd_compute_gfx11 *compute_mqd;
>  
> @@ -247,6 +250,13 @@ static int mes_userq_mqd_create(struct amdgpu_userq_mgr *uq_mgr,
>  			goto free_mqd;
>  		}
>  
> +		if (amdgpu_userq_input_va_validate(queue->vm, compute_mqd->eop_va,
> +					max_t(u32, PAGE_SIZE, AMDGPU_GPU_PAGE_SIZE))) {
> +			drm_file_err(uq_mgr->file, "EOP VA is invalid\n");
> +			r = -EINVAL;
> +			goto free_mqd;
> +		}
> +
>  		userq_props->eop_gpu_addr = compute_mqd->eop_va;
>  		userq_props->hqd_pipe_priority = AMDGPU_GFX_PIPE_PRIO_NORMAL;
>  		userq_props->hqd_queue_priority = AMDGPU_GFX_QUEUE_PRIORITY_MINIMUM;
> @@ -274,6 +284,14 @@ static int mes_userq_mqd_create(struct amdgpu_userq_mgr *uq_mgr,
>  		userq_props->csa_addr = mqd_gfx_v11->csa_va;
>  		userq_props->tmz_queue =
>  			mqd_user->flags & AMDGPU_USERQ_CREATE_FLAGS_QUEUE_SECURE;
> +
> +		if (amdgpu_userq_input_va_validate(queue->vm, mqd_gfx_v11->shadow_va,
> +					shadow_info.shadow_size)) {
> +			drm_file_err(uq_mgr->file, "shadow VA is invalid\n");
> +			r = -EINVAL;
> +			goto free_mqd;
> +		}
> +
>  		kfree(mqd_gfx_v11);
>  	} else if (queue->queue_type == AMDGPU_HW_IP_DMA) {
>  		struct drm_amdgpu_userq_mqd_sdma_gfx11 *mqd_sdma_v11;
> @@ -291,6 +309,13 @@ static int mes_userq_mqd_create(struct amdgpu_userq_mgr *uq_mgr,
>  			goto free_mqd;
>  		}
>  
> +		if (amdgpu_userq_input_va_validate(queue->vm, mqd_sdma_v11->csa_va,
> +					shadow_info.csa_size)) {
> +			drm_file_err(uq_mgr->file, "CSA VA is invalid\n");
> +			r = -EINVAL;
> +			goto free_mqd;
> +		}
> +
>  		userq_props->csa_addr = mqd_sdma_v11->csa_va;
>  		kfree(mqd_sdma_v11);
>  	}

Re: [PATCH v6 06/11] drm/amdgpu: track the userq bo va for its obj management

[Christian König]

On 11.07.25 11:39, Prike Liang wrote:
> The user queue object destroy requires ensuring its
> VA keeps mapping prior to the queue being destroyed.
> Otherwise, it seems a bug in the user space or VA
> freed wrongly, and the kernel driver should report an
> invalidated error to the user IOCLT request.
> 
> Signed-off-by: Prike Liang <Prike.Liang@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> index 2856c2506bee..81fbb00b6d91 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> @@ -510,12 +510,24 @@ amdgpu_userq_destroy(struct drm_file *filp, int queue_id)
>  		return -EINVAL;
>  	}
>  	amdgpu_userq_wait_for_last_fence(uq_mgr, queue);
> +
> +	/*
> +	 * At this point the userq obj va should be mapped,
> +	 * otherwise will return error to user.
> +	 */
> +	if (!amdgpu_userq_buffer_vas_mapped(&fpriv->vm, queue)) {
> +		drm_warn(adev_to_drm(uq_mgr->adev), "the userq obj va shouldn't be umapped here\n");
> +		r = -EINVAL;
> +	}
> +

That is still not something we can do.

Destroying an userque can't fail in any way.

Regards,
Christian.

>  	r = amdgpu_userq_unmap_helper(uq_mgr, queue);
>  	/*TODO: It requires a reset for userq hw unmap error*/
>  	if (unlikely(r != AMDGPU_USERQ_STATE_UNMAPPED)) {
>  		drm_warn(adev_to_drm(uq_mgr->adev), "trying to destroy a HW mapping userq\n");
>  		r = -ETIMEDOUT;
>  	}
> +
> +	amdgpu_userq_buffer_vas_put(&fpriv->vm, queue);
>  	amdgpu_userq_cleanup(uq_mgr, queue, queue_id);
>  	mutex_unlock(&uq_mgr->userq_mutex);
>  
> @@ -636,6 +648,9 @@ amdgpu_userq_create(struct drm_file *filp, union drm_amdgpu_userq *args)
>  		goto unlock;
>  	}
>  
> +	/* refer to the userq objects vm bo*/
> +	amdgpu_userq_buffer_vas_get(queue->vm, queue);
> +
>  	qid = idr_alloc(&uq_mgr->userq_idr, queue, 1, AMDGPU_MAX_USERQ_COUNT, GFP_KERNEL);
>  	if (qid < 0) {
>  		drm_file_err(uq_mgr->file, "Failed to allocate a queue id\n");

Re: [PATCH v6 07/11] drm/amdgpu: validate userq's last fence prior to destroying

[Christian König]

On 11.07.25 11:39, Prike Liang wrote:
> The userq requires validating queue status before destroying
> it, if user tries to destroy a busy userq by IOCTL then the
> driver should report an error for this illegal usage.

Clear NAK, destroying a busy userqueue is perfectly valid!

Regards,
Christian.

> 
> Signed-off-by: Prike Liang <Prike.Liang@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> index 81fbb00b6d91..bcbe8d3f66ed 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> @@ -281,7 +281,7 @@ amdgpu_userq_map_helper(struct amdgpu_userq_mgr *uq_mgr,
>  	return r;
>  }
>  
> -static void
> +static int
>  amdgpu_userq_wait_for_last_fence(struct amdgpu_userq_mgr *uq_mgr,
>  				 struct amdgpu_usermode_queue *queue)
>  {
> @@ -290,10 +290,14 @@ amdgpu_userq_wait_for_last_fence(struct amdgpu_userq_mgr *uq_mgr,
>  
>  	if (f && !dma_fence_is_signaled(f)) {
>  		ret = dma_fence_wait_timeout(f, true, msecs_to_jiffies(100));
> -		if (ret <= 0)
> +		if (ret <= 0) {
>  			drm_file_err(uq_mgr->file, "Timed out waiting for fence=%llu:%llu\n",
>  				     f->context, f->seqno);
> +			return -ETIMEDOUT;
> +		}
>  	}
> +
> +	return 0;
>  }
>  
>  static void
> @@ -509,7 +513,12 @@ amdgpu_userq_destroy(struct drm_file *filp, int queue_id)
>  		mutex_unlock(&uq_mgr->userq_mutex);
>  		return -EINVAL;
>  	}
> -	amdgpu_userq_wait_for_last_fence(uq_mgr, queue);
> +
> +	if (amdgpu_userq_wait_for_last_fence(uq_mgr, queue)) {
> +		drm_warn(adev_to_drm(uq_mgr->adev), "Don't destroy a busy userq\n");
> +		/* For the fence signal timeout case, it requires resetting the busy queue.*/
> +		r = -ETIMEDOUT;
> +	}
>  
>  	/*
>  	 * At this point the userq obj va should be mapped,

Re: [PATCH v6 09/11] drm/amdgpu: validate the shared bo for tracking usage size

[Christian König]

On 11.07.25 11:39, Prike Liang wrote:
> It requires validating the shared BO before updating its usage
> size; otherwise, there is a potential NULL pointer error when the
> BO released improperly.

Clear NAK to that. You are obviously working around a bug elsewhere.

Regards,
Christian.

> 
> Signed-off-by: Prike Liang <Prike.Liang@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index f042372d9f2e..a574effdd3ec 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -321,12 +321,26 @@ static void amdgpu_vm_bo_reset_state_machine(struct amdgpu_vm *vm)
>   */
>  static void amdgpu_vm_update_shared(struct amdgpu_vm_bo_base *base)
>  {
> -	struct amdgpu_vm *vm = base->vm;
> -	struct amdgpu_bo *bo = base->bo;
> -	uint64_t size = amdgpu_bo_size(bo);
> -	uint32_t bo_memtype = amdgpu_bo_mem_stats_placement(bo);
> +	struct amdgpu_vm *vm;
> +	struct amdgpu_bo *bo;
> +	uint64_t size = 0;
> +	uint32_t bo_memtype = TTM_PL_SYSTEM;
>  	bool shared;
>  
> +	if (likely(base)) {
> +		vm = base->vm;
> +		bo = base->bo;
> +	} else {
> +		return;
> +	}
> +
> +	if (likely(bo)) {
> +		size = amdgpu_bo_size(bo);
> +		bo_memtype = amdgpu_bo_mem_stats_placement(bo);
> +	} else {
> +		return;
> +	}
> +
>  	spin_lock(&vm->status_lock);
>  	shared = drm_gem_object_is_shared_for_memory_stats(&bo->tbo.base);
>  	if (base->shared != shared) {
> @@ -353,6 +367,9 @@ void amdgpu_vm_bo_update_shared(struct amdgpu_bo *bo)
>  {
>  	struct amdgpu_vm_bo_base *base;
>  
> +	if (unlikely(!bo))
> +		return;
> +
>  	for (base = bo->vm_bo; base; base = base->next)
>  		amdgpu_vm_update_shared(base);
>  }

Re: [PATCH v6 10/11] drm/amdgpu: validate the queue va for resuming the queue

[Christian König]

On 11.07.25 11:39, Prike Liang wrote:
> It requires validating the userq VA whether is mapped before
> trying to resume the queue.
> 
> Signed-off-by: Prike Liang <Prike.Liang@amd.com>

Yeah that looks sane to me. Patch is Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> index bfe7b229011e..54f44fc834fe 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> @@ -777,11 +777,18 @@ static int
>  amdgpu_userq_restore_all(struct amdgpu_userq_mgr *uq_mgr)
>  {
>  	struct amdgpu_usermode_queue *queue;
> +	struct amdgpu_fpriv *fpriv = uq_mgr_to_fpriv(uq_mgr);
>  	int queue_id;
>  	int ret = 0, r;
>  
>  	/* Resume all the queues for this process */
>  	idr_for_each_entry(&uq_mgr->userq_idr, queue, queue_id) {
> +
> +		if (!amdgpu_userq_buffer_vas_mapped(&fpriv->vm, queue)) {
> +			drm_file_err(uq_mgr->file, "trying restore queue without va mappping\n");
> +			continue;
> +		}
> +
>  		r = amdgpu_userq_map_helper(uq_mgr, queue);
>  		if (r)
>  			ret = r;

RE: [PATCH v6 09/11] drm/amdgpu: validate the shared bo for tracking usage size

[Liang, Prike]

[Public]

> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig@amd.com>
> Sent: Friday, July 11, 2025 8:14 PM
> To: Liang, Prike <Prike.Liang@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
> Subject: Re: [PATCH v6 09/11] drm/amdgpu: validate the shared bo for tracking
> usage size
>
> On 11.07.25 11:39, Prike Liang wrote:
> > It requires validating the shared BO before updating its usage size;
> > otherwise, there is a potential NULL pointer error when the BO
> > released improperly.
>
> Clear NAK to that. You are obviously working around a bug elsewhere.
Yes, this is a workaround for the userq PT deference imbalance issue and will drop that.

> Regards,
> Christian.
>
> >
> > Signed-off-by: Prike Liang <Prike.Liang@amd.com>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 25 +++++++++++++++++++++----
> >  1 file changed, 21 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > index f042372d9f2e..a574effdd3ec 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > @@ -321,12 +321,26 @@ static void amdgpu_vm_bo_reset_state_machine(struct
> amdgpu_vm *vm)
> >   */
> >  static void amdgpu_vm_update_shared(struct amdgpu_vm_bo_base *base)
> > {
> > -   struct amdgpu_vm *vm = base->vm;
> > -   struct amdgpu_bo *bo = base->bo;
> > -   uint64_t size = amdgpu_bo_size(bo);
> > -   uint32_t bo_memtype = amdgpu_bo_mem_stats_placement(bo);
> > +   struct amdgpu_vm *vm;
> > +   struct amdgpu_bo *bo;
> > +   uint64_t size = 0;
> > +   uint32_t bo_memtype = TTM_PL_SYSTEM;
> >     bool shared;
> >
> > +   if (likely(base)) {
> > +           vm = base->vm;
> > +           bo = base->bo;
> > +   } else {
> > +           return;
> > +   }
> > +
> > +   if (likely(bo)) {
> > +           size = amdgpu_bo_size(bo);
> > +           bo_memtype = amdgpu_bo_mem_stats_placement(bo);
> > +   } else {
> > +           return;
> > +   }
> > +
> >     spin_lock(&vm->status_lock);
> >     shared = drm_gem_object_is_shared_for_memory_stats(&bo->tbo.base);
> >     if (base->shared != shared) {
> > @@ -353,6 +367,9 @@ void amdgpu_vm_bo_update_shared(struct amdgpu_bo
> > *bo)  {
> >     struct amdgpu_vm_bo_base *base;
> >
> > +   if (unlikely(!bo))
> > +           return;
> > +
> >     for (base = bo->vm_bo; base; base = base->next)
> >             amdgpu_vm_update_shared(base);
> >  }

RE: [PATCH v6 03/11] drm/amdgpu: rework the userq doorbell object destroy

[Liang, Prike]

[Public]

Regards,
      Prike

> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig@amd.com>
> Sent: Friday, July 11, 2025 8:01 PM
> To: Liang, Prike <Prike.Liang@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
> Subject: Re: [PATCH v6 03/11] drm/amdgpu: rework the userq doorbell object
> destroy
>
> On 11.07.25 11:39, Prike Liang wrote:
> > This patch aims to unify and destroy the userq doorbell objects at
> > mes_userq_mqd_destroy(), and this change will also help with unpinning
> > and destroying the userq doorbell objects for amdgpu_userq_mgr_fini()
> > during releasing the drm files.
> >
> > Signed-off-by: Prike Liang <Prike.Liang@amd.com>
> > Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c  | 6 ------
> > drivers/gpu/drm/amd/amdgpu/mes_userqueue.c | 7 +++++++
> >  2 files changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> > index 3d2a7f8946cf..15e833b1b3e3 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> > @@ -312,12 +312,6 @@ amdgpu_userq_destroy(struct drm_file *filp, int
> queue_id)
> >             return -EINVAL;
> >     }
> >     amdgpu_userq_wait_for_last_fence(uq_mgr, queue);
> > -   r = amdgpu_bo_reserve(queue->db_obj.obj, true);
> > -   if (!r) {
> > -           amdgpu_bo_unpin(queue->db_obj.obj);
> > -           amdgpu_bo_unreserve(queue->db_obj.obj);
> > -   }
> > -   amdgpu_bo_unref(&queue->db_obj.obj);
> >     r = amdgpu_userq_unmap_helper(uq_mgr, queue);
> >     /*TODO: It requires a reset for userq hw unmap error*/
> >     if (unlikely(r != AMDGPU_USERQ_STATE_UNMAPPED)) { diff --git
> > a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
> > b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
> > index 1457fb49a794..15aa1ca67a11 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
> > @@ -336,6 +336,13 @@ mes_userq_mqd_destroy(struct amdgpu_userq_mgr
> *uq_mgr,
> >                   struct amdgpu_usermode_queue *queue)  {
> >     amdgpu_userq_destroy_object(uq_mgr, &queue->fw_obj);
> > +
> > +   if (!amdgpu_bo_reserve(queue->db_obj.obj, true)) {
> > +           amdgpu_bo_unpin(queue->db_obj.obj);
> > +           amdgpu_bo_unreserve(queue->db_obj.obj);
> > +           amdgpu_userq_destroy_object(uq_mgr, &queue->db_obj);
> > +   }
> > +
>
> That makes no sense to do here. The pinning isn't done in mes_userq_mqd_create()
> either.
Yes, but the doorbell BO is pinned by amdgpu_userq_get_doorbell_index(), which is still
Invoked during userq BOs creation phase. This patch wants to free the doorbell object like some
other userq objects at the unified place of mes_userq_mqd_destroy().

> In general we should avoid pinning the MQD in the first place, that buffer needs to be
> fences instead.
If here not pin the userq doorbell BO, then will the doorbell index be changed when the doorbell
BO is moved?

> Regards,
> Christian.
>
> >     kfree(queue->userq_prop);
> >     amdgpu_userq_destroy_object(uq_mgr, &queue->mqd);  }

RE: [PATCH v6 04/11] drm/amdgpu: validate userq buffer virtual address and size

[Liang, Prike]

[Public]

Regards,
      Prike

> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig@amd.com>
> Sent: Friday, July 11, 2025 8:08 PM
> To: Liang, Prike <Prike.Liang@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
> Subject: Re: [PATCH v6 04/11] drm/amdgpu: validate userq buffer virtual address
> and size
>
>
>
> On 11.07.25 11:39, Prike Liang wrote:
> > It needs to validate the userq object virtual address to determin
> > whether it is residented in a valid vm mapping.
> >
> > Signed-off-by: Prike Liang <Prike.Liang@amd.com>
> > Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c  | 38
> > ++++++++++++++++++++++  drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h  |
> > 2 ++  drivers/gpu/drm/amd/amdgpu/mes_userqueue.c | 25 ++++++++++++++
> >  3 files changed, 65 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> > index 15e833b1b3e3..a41dd38b0adb 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> > @@ -44,6 +44,36 @@ u32 amdgpu_userq_get_supported_ip_mask(struct
> amdgpu_device *adev)
> >     return userq_ip_mask;
> >  }
> >
> > +int amdgpu_userq_input_va_validate(struct amdgpu_vm *vm, u64 addr,
> > +                           u64 expected_size)
> > +{
> > +   struct amdgpu_bo_va_mapping *va_map;
> > +   u64 user_addr;
> > +   u64 size;
> > +   int r;
> > +
> > +   user_addr = (addr & AMDGPU_GMC_HOLE_MASK) >>
> AMDGPU_GPU_PAGE_SHIFT;
> > +   size = expected_size >> AMDGPU_GPU_PAGE_SHIFT;
> > +
> > +   r = amdgpu_bo_reserve(vm->root.bo, false);
> > +   if (r)
> > +           return r;
> > +
> > +   va_map = amdgpu_vm_bo_lookup_mapping(vm, user_addr);
> > +   if (!va_map)
> > +           goto out_err;
> > +   /* Only validate the userq whether resident in the VM mapping range */
> > +   if (user_addr >= va_map->start &&
>
> This check is unecessary.
>
> > +       (size != 0 && user_addr + size - 1 <= va_map->last)) {
>
> The size != 0 check is unecessary as well and you need to be careful with wrap
> arounds.
>
> Better write this like that (va_map->last - user_addr + 1 >= size)
Thank you for the suggestion, will update this in later version.
>
> > +           amdgpu_bo_unreserve(vm->root.bo);
> > +           return 0;
> > +   }
> > +
> > +out_err:
> > +   amdgpu_bo_unreserve(vm->root.bo);
> > +   return -EINVAL;
> > +}
> > +
> >  static int
> >  amdgpu_userq_unmap_helper(struct amdgpu_userq_mgr *uq_mgr,
> >                       struct amdgpu_usermode_queue *queue) @@ -386,6
> +416,14 @@
> > amdgpu_userq_create(struct drm_file *filp, union drm_amdgpu_userq *args)
> >             r = -EINVAL;
> >             goto unlock;
> >     }
> > +   /* Validate the userq virtual address.*/
> > +   if (amdgpu_userq_input_va_validate(&fpriv->vm, args->in.queue_va, args-
> >in.queue_size) ||
> > +       amdgpu_userq_input_va_validate(&fpriv->vm, args->in.rptr_va,
> PAGE_SIZE) ||
> > +       amdgpu_userq_input_va_validate(&fpriv->vm, args->in.wptr_va,
> PAGE_SIZE)) {
> > +           drm_file_err(uq_mgr->file, "Usermode queue input virt address is
> > +invalid\n");
>
> No error message on invalid userspace parameters please.
OK, why can't give the alert log for the invalid user case?
It's useful for catching the userq invalid VA/size case, how about change the error message to a debug level?

> Apart from those comments looks like the right thing to do to me.
>
> Regards,
> Christian.
>
> > +           r = -EINVAL;
> > +           goto unlock;
> > +   }
> >
> >     queue = kzalloc(sizeof(struct amdgpu_usermode_queue), GFP_KERNEL);
> >     if (!queue) {
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
> > index ec040c2fd6c9..704935ca0c36 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
> > @@ -132,4 +132,6 @@ int
> > amdgpu_userq_stop_sched_for_enforce_isolation(struct amdgpu_device *adev,
> int amdgpu_userq_start_sched_for_enforce_isolation(struct amdgpu_device *adev,
> >                                                u32 idx);
> >
> > +int amdgpu_userq_input_va_validate(struct amdgpu_vm *vm, u64 addr,
> > +                   u64 expected_size);
> >  #endif
> > diff --git a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
> > b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
> > index 15aa1ca67a11..75b9a6294b53 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
> > @@ -206,6 +206,7 @@ static int mes_userq_mqd_create(struct
> amdgpu_userq_mgr *uq_mgr,
> >     struct amdgpu_mqd *mqd_hw_default = &adev->mqds[queue->queue_type];
> >     struct drm_amdgpu_userq_in *mqd_user = args_in;
> >     struct amdgpu_mqd_prop *userq_props;
> > +   struct amdgpu_gfx_shadow_info shadow_info;
> >     int r;
> >
> >     /* Structure to initialize MQD for userqueue using generic MQD init
> > function */ @@ -231,6 +232,8 @@ static int mes_userq_mqd_create(struct
> amdgpu_userq_mgr *uq_mgr,
> >     userq_props->doorbell_index = queue->doorbell_index;
> >     userq_props->fence_address = queue->fence_drv->gpu_addr;
> >
> > +   if (adev->gfx.funcs->get_gfx_shadow_info)
> > +           adev->gfx.funcs->get_gfx_shadow_info(adev, &shadow_info, true);
> >     if (queue->queue_type == AMDGPU_HW_IP_COMPUTE) {
> >             struct drm_amdgpu_userq_mqd_compute_gfx11 *compute_mqd;
> >
> > @@ -247,6 +250,13 @@ static int mes_userq_mqd_create(struct
> amdgpu_userq_mgr *uq_mgr,
> >                     goto free_mqd;
> >             }
> >
> > +           if (amdgpu_userq_input_va_validate(queue->vm, compute_mqd-
> >eop_va,
> > +                                   max_t(u32, PAGE_SIZE,
> AMDGPU_GPU_PAGE_SIZE))) {
> > +                   drm_file_err(uq_mgr->file, "EOP VA is invalid\n");
> > +                   r = -EINVAL;
> > +                   goto free_mqd;
> > +           }
> > +
> >             userq_props->eop_gpu_addr = compute_mqd->eop_va;
> >             userq_props->hqd_pipe_priority =
> AMDGPU_GFX_PIPE_PRIO_NORMAL;
> >             userq_props->hqd_queue_priority =
> > AMDGPU_GFX_QUEUE_PRIORITY_MINIMUM;
> > @@ -274,6 +284,14 @@ static int mes_userq_mqd_create(struct
> amdgpu_userq_mgr *uq_mgr,
> >             userq_props->csa_addr = mqd_gfx_v11->csa_va;
> >             userq_props->tmz_queue =
> >                     mqd_user->flags &
> AMDGPU_USERQ_CREATE_FLAGS_QUEUE_SECURE;
> > +
> > +           if (amdgpu_userq_input_va_validate(queue->vm, mqd_gfx_v11-
> >shadow_va,
> > +                                   shadow_info.shadow_size)) {
> > +                   drm_file_err(uq_mgr->file, "shadow VA is invalid\n");
> > +                   r = -EINVAL;
> > +                   goto free_mqd;
> > +           }
> > +
> >             kfree(mqd_gfx_v11);
> >     } else if (queue->queue_type == AMDGPU_HW_IP_DMA) {
> >             struct drm_amdgpu_userq_mqd_sdma_gfx11 *mqd_sdma_v11; @@
> -291,6
> > +309,13 @@ static int mes_userq_mqd_create(struct amdgpu_userq_mgr
> *uq_mgr,
> >                     goto free_mqd;
> >             }
> >
> > +           if (amdgpu_userq_input_va_validate(queue->vm, mqd_sdma_v11-
> >csa_va,
> > +                                   shadow_info.csa_size)) {
> > +                   drm_file_err(uq_mgr->file, "CSA VA is invalid\n");
> > +                   r = -EINVAL;
> > +                   goto free_mqd;
> > +           }
> > +
> >             userq_props->csa_addr = mqd_sdma_v11->csa_va;
> >             kfree(mqd_sdma_v11);
> >     }

Re: [PATCH v6 04/11] drm/amdgpu: validate userq buffer virtual address and size

[Christian König]

On 15.07.25 10:19, Liang, Prike wrote:
>>> +   /* Validate the userq virtual address.*/
>>> +   if (amdgpu_userq_input_va_validate(&fpriv->vm, args->in.queue_va, args-
>>> in.queue_size) ||
>>> +       amdgpu_userq_input_va_validate(&fpriv->vm, args->in.rptr_va,
>> PAGE_SIZE) ||
>>> +       amdgpu_userq_input_va_validate(&fpriv->vm, args->in.wptr_va,
>> PAGE_SIZE)) {
>>> +           drm_file_err(uq_mgr->file, "Usermode queue input virt address is
>>> +invalid\n");
>>
>> No error message on invalid userspace parameters please.
> OK, why can't give the alert log for the invalid user case?
> It's useful for catching the userq invalid VA/size case, how about change the error message to a debug level?

Never ever write into the system log that userspace did something wrong. That allows userspace to spam the log.

Only every write when the HW or the kernel detects that something is wrong. E.g. not responding HW or similar.

Regards,
Christian.

> 
>> Apart from those comments looks like the right thing to do to me.
>>
>> Regards,
>> Christian.

Re: [PATCH v6 03/11] drm/amdgpu: rework the userq doorbell object destroy

[Christian König]

On 15.07.25 10:07, Liang, Prike wrote:
> [Public]
> 
> Regards,
>       Prike
> 
>> -----Original Message-----
>> From: Koenig, Christian <Christian.Koenig@amd.com>
>> Sent: Friday, July 11, 2025 8:01 PM
>> To: Liang, Prike <Prike.Liang@amd.com>; amd-gfx@lists.freedesktop.org
>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
>> Subject: Re: [PATCH v6 03/11] drm/amdgpu: rework the userq doorbell object
>> destroy
>>
>> On 11.07.25 11:39, Prike Liang wrote:
>>> This patch aims to unify and destroy the userq doorbell objects at
>>> mes_userq_mqd_destroy(), and this change will also help with unpinning
>>> and destroying the userq doorbell objects for amdgpu_userq_mgr_fini()
>>> during releasing the drm files.
>>>
>>> Signed-off-by: Prike Liang <Prike.Liang@amd.com>
>>> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
>>> ---
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c  | 6 ------
>>> drivers/gpu/drm/amd/amdgpu/mes_userqueue.c | 7 +++++++
>>>  2 files changed, 7 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>> index 3d2a7f8946cf..15e833b1b3e3 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>> @@ -312,12 +312,6 @@ amdgpu_userq_destroy(struct drm_file *filp, int
>> queue_id)
>>>             return -EINVAL;
>>>     }
>>>     amdgpu_userq_wait_for_last_fence(uq_mgr, queue);
>>> -   r = amdgpu_bo_reserve(queue->db_obj.obj, true);
>>> -   if (!r) {
>>> -           amdgpu_bo_unpin(queue->db_obj.obj);
>>> -           amdgpu_bo_unreserve(queue->db_obj.obj);
>>> -   }
>>> -   amdgpu_bo_unref(&queue->db_obj.obj);
>>>     r = amdgpu_userq_unmap_helper(uq_mgr, queue);
>>>     /*TODO: It requires a reset for userq hw unmap error*/
>>>     if (unlikely(r != AMDGPU_USERQ_STATE_UNMAPPED)) { diff --git
>>> a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
>>> b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
>>> index 1457fb49a794..15aa1ca67a11 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
>>> @@ -336,6 +336,13 @@ mes_userq_mqd_destroy(struct amdgpu_userq_mgr
>> *uq_mgr,
>>>                   struct amdgpu_usermode_queue *queue)  {
>>>     amdgpu_userq_destroy_object(uq_mgr, &queue->fw_obj);
>>> +
>>> +   if (!amdgpu_bo_reserve(queue->db_obj.obj, true)) {
>>> +           amdgpu_bo_unpin(queue->db_obj.obj);
>>> +           amdgpu_bo_unreserve(queue->db_obj.obj);
>>> +           amdgpu_userq_destroy_object(uq_mgr, &queue->db_obj);
>>> +   }
>>> +
>>
>> That makes no sense to do here. The pinning isn't done in mes_userq_mqd_create()
>> either.
> Yes, but the doorbell BO is pinned by amdgpu_userq_get_doorbell_index(), which is still
> Invoked during userq BOs creation phase. This patch wants to free the doorbell object like some
> other userq objects at the unified place of mes_userq_mqd_destroy().

Yeah and exactly that is not a good idea.

The doorbell object is provided by userspace and not allocated by the kernel like the MQD.

So destroying it here makes no sense at all. You are most likely messing up the doorbell reference count with that.

>> In general we should avoid pinning the MQD in the first place, that buffer needs to be
>> fences instead.
> If here not pin the userq doorbell BO, then will the doorbell index be changed when the doorbell
> BO is moved?

Correct, yes. The doorbell index needs to be updated on each resume of the userqueue.

We haven't implemented that yet since we weren't sure if the MES FW could handle that (and because the eviction fences wasn't ready at that time).

Regards,
Christian.

> 
>> Regards,
>> Christian.
>>
>>>     kfree(queue->userq_prop);
>>>     amdgpu_userq_destroy_object(uq_mgr, &queue->mqd);  }
> 

RE: [PATCH v6 07/11] drm/amdgpu: validate userq's last fence prior to destroying

[Liang, Prike]

[Public]

Regards,
      Prike

> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig@amd.com>
> Sent: Friday, July 11, 2025 8:13 PM
> To: Liang, Prike <Prike.Liang@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
> Subject: Re: [PATCH v6 07/11] drm/amdgpu: validate userq's last fence prior to
> destroying
>
> On 11.07.25 11:39, Prike Liang wrote:
> > The userq requires validating queue status before destroying it, if
> > user tries to destroy a busy userq by IOCTL then the driver should
> > report an error for this illegal usage.
>
> Clear NAK, destroying a busy userqueue is perfectly valid!
Yes, the firmware should handle such case something like as preempting the queue.
If we directly unmap a hang queue and may further cause the MES firmware hang up,
so, do we need to detect the hang userq here by checking the userq fence status and reset the hang queue before further performs the unmap queue?

> Regards,
> Christian.
>
> >
> > Signed-off-by: Prike Liang <Prike.Liang@amd.com>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> > index 81fbb00b6d91..bcbe8d3f66ed 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> > @@ -281,7 +281,7 @@ amdgpu_userq_map_helper(struct amdgpu_userq_mgr
> *uq_mgr,
> >     return r;
> >  }
> >
> > -static void
> > +static int
> >  amdgpu_userq_wait_for_last_fence(struct amdgpu_userq_mgr *uq_mgr,
> >                              struct amdgpu_usermode_queue *queue)  { @@ -
> 290,10 +290,14 @@
> > amdgpu_userq_wait_for_last_fence(struct amdgpu_userq_mgr *uq_mgr,
> >
> >     if (f && !dma_fence_is_signaled(f)) {
> >             ret = dma_fence_wait_timeout(f, true, msecs_to_jiffies(100));
> > -           if (ret <= 0)
> > +           if (ret <= 0) {
> >                     drm_file_err(uq_mgr->file, "Timed out waiting for
> fence=%llu:%llu\n",
> >                                  f->context, f->seqno);
> > +                   return -ETIMEDOUT;
> > +           }
> >     }
> > +
> > +   return 0;
> >  }
> >
> >  static void
> > @@ -509,7 +513,12 @@ amdgpu_userq_destroy(struct drm_file *filp, int
> queue_id)
> >             mutex_unlock(&uq_mgr->userq_mutex);
> >             return -EINVAL;
> >     }
> > -   amdgpu_userq_wait_for_last_fence(uq_mgr, queue);
> > +
> > +   if (amdgpu_userq_wait_for_last_fence(uq_mgr, queue)) {
> > +           drm_warn(adev_to_drm(uq_mgr->adev), "Don't destroy a busy
> userq\n");
> > +           /* For the fence signal timeout case, it requires resetting the busy
> queue.*/
> > +           r = -ETIMEDOUT;
> > +   }
> >
> >     /*
> >      * At this point the userq obj va should be mapped,

RE: [PATCH v6 06/11] drm/amdgpu: track the userq bo va for its obj management

[Liang, Prike]

[Public]

Regards,
      Prike

> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig@amd.com>
> Sent: Friday, July 11, 2025 8:11 PM
> To: Liang, Prike <Prike.Liang@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
> Subject: Re: [PATCH v6 06/11] drm/amdgpu: track the userq bo va for its obj
> management
>
>
>
> On 11.07.25 11:39, Prike Liang wrote:
> > The user queue object destroy requires ensuring its VA keeps mapping
> > prior to the queue being destroyed.
> > Otherwise, it seems a bug in the user space or VA freed wrongly, and
> > the kernel driver should report an invalidated error to the user IOCLT
> > request.
> >
> > Signed-off-by: Prike Liang <Prike.Liang@amd.com>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> > index 2856c2506bee..81fbb00b6d91 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> > @@ -510,12 +510,24 @@ amdgpu_userq_destroy(struct drm_file *filp, int
> queue_id)
> >             return -EINVAL;
> >     }
> >     amdgpu_userq_wait_for_last_fence(uq_mgr, queue);
> > +
> > +   /*
> > +    * At this point the userq obj va should be mapped,
> > +    * otherwise will return error to user.
> > +    */
> > +   if (!amdgpu_userq_buffer_vas_mapped(&fpriv->vm, queue)) {
> > +           drm_warn(adev_to_drm(uq_mgr->adev), "the userq obj va shouldn't
> be umapped here\n");
> > +           r = -EINVAL;
> > +   }
> > +
>
> That is still not something we can do.
>
> Destroying an userque can't fail in any way.
Yes, the userq destroy will continue performing in this invalid case.
Can we keep this part for detecting this invalid destroy case?
Furthermore, it looks like this error code will not affect the destroy request at
userspace since the Mesa driver doesn't check the userq destroy return value.

> Regards,
> Christian.
>
> >     r = amdgpu_userq_unmap_helper(uq_mgr, queue);
> >     /*TODO: It requires a reset for userq hw unmap error*/
> >     if (unlikely(r != AMDGPU_USERQ_STATE_UNMAPPED)) {
> >             drm_warn(adev_to_drm(uq_mgr->adev), "trying to destroy a HW
> mapping userq\n");
> >             r = -ETIMEDOUT;
> >     }
> > +
> > +   amdgpu_userq_buffer_vas_put(&fpriv->vm, queue);
> >     amdgpu_userq_cleanup(uq_mgr, queue, queue_id);
> >     mutex_unlock(&uq_mgr->userq_mutex);
> >
> > @@ -636,6 +648,9 @@ amdgpu_userq_create(struct drm_file *filp, union
> drm_amdgpu_userq *args)
> >             goto unlock;
> >     }
> >
> > +   /* refer to the userq objects vm bo*/
> > +   amdgpu_userq_buffer_vas_get(queue->vm, queue);
> > +
> >     qid = idr_alloc(&uq_mgr->userq_idr, queue, 1,
> AMDGPU_MAX_USERQ_COUNT, GFP_KERNEL);
> >     if (qid < 0) {
> >             drm_file_err(uq_mgr->file, "Failed to allocate a queue id\n");

Re: [PATCH v6 07/11] drm/amdgpu: validate userq's last fence prior to destroying

[Christian König]

On 15.07.25 13:50, Liang, Prike wrote:
> [Public]
> 
> Regards,
>       Prike
> 
>> -----Original Message-----
>> From: Koenig, Christian <Christian.Koenig@amd.com>
>> Sent: Friday, July 11, 2025 8:13 PM
>> To: Liang, Prike <Prike.Liang@amd.com>; amd-gfx@lists.freedesktop.org
>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
>> Subject: Re: [PATCH v6 07/11] drm/amdgpu: validate userq's last fence prior to
>> destroying
>>
>> On 11.07.25 11:39, Prike Liang wrote:
>>> The userq requires validating queue status before destroying it, if
>>> user tries to destroy a busy userq by IOCTL then the driver should
>>> report an error for this illegal usage.
>>
>> Clear NAK, destroying a busy userqueue is perfectly valid!
> Yes, the firmware should handle such case something like as preempting the queue.
> If we directly unmap a hang queue and may further cause the MES firmware hang up,
> so, do we need to detect the hang userq here by checking the userq fence status and reset the hang queue before further performs the unmap queue?

No, waiting for the last fence should be perfectly sufficient since the hang detection is separate from this.

BTW please remove the 100ms timeout here, we should wait forever or until the hang detection has suspended the queue and signaled the fence with an error.

Regards,
Christian.


> 
>> Regards,
>> Christian.
>>
>>>
>>> Signed-off-by: Prike Liang <Prike.Liang@amd.com>
>>> ---
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 15 ++++++++++++---
>>>  1 file changed, 12 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>> index 81fbb00b6d91..bcbe8d3f66ed 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>> @@ -281,7 +281,7 @@ amdgpu_userq_map_helper(struct amdgpu_userq_mgr
>> *uq_mgr,
>>>     return r;
>>>  }
>>>
>>> -static void
>>> +static int
>>>  amdgpu_userq_wait_for_last_fence(struct amdgpu_userq_mgr *uq_mgr,
>>>                              struct amdgpu_usermode_queue *queue)  { @@ -
>> 290,10 +290,14 @@
>>> amdgpu_userq_wait_for_last_fence(struct amdgpu_userq_mgr *uq_mgr,
>>>
>>>     if (f && !dma_fence_is_signaled(f)) {
>>>             ret = dma_fence_wait_timeout(f, true, msecs_to_jiffies(100));
>>> -           if (ret <= 0)
>>> +           if (ret <= 0) {
>>>                     drm_file_err(uq_mgr->file, "Timed out waiting for
>> fence=%llu:%llu\n",
>>>                                  f->context, f->seqno);
>>> +                   return -ETIMEDOUT;
>>> +           }
>>>     }
>>> +
>>> +   return 0;
>>>  }
>>>
>>>  static void
>>> @@ -509,7 +513,12 @@ amdgpu_userq_destroy(struct drm_file *filp, int
>> queue_id)
>>>             mutex_unlock(&uq_mgr->userq_mutex);
>>>             return -EINVAL;
>>>     }
>>> -   amdgpu_userq_wait_for_last_fence(uq_mgr, queue);
>>> +
>>> +   if (amdgpu_userq_wait_for_last_fence(uq_mgr, queue)) {
>>> +           drm_warn(adev_to_drm(uq_mgr->adev), "Don't destroy a busy
>> userq\n");
>>> +           /* For the fence signal timeout case, it requires resetting the busy
>> queue.*/
>>> +           r = -ETIMEDOUT;
>>> +   }
>>>
>>>     /*
>>>      * At this point the userq obj va should be mapped,
> 

Re: [PATCH v6 06/11] drm/amdgpu: track the userq bo va for its obj management

[Christian König]

On 15.07.25 14:05, Liang, Prike wrote:
> [Public]
> 
> Regards,
>       Prike
> 
>> -----Original Message-----
>> From: Koenig, Christian <Christian.Koenig@amd.com>
>> Sent: Friday, July 11, 2025 8:11 PM
>> To: Liang, Prike <Prike.Liang@amd.com>; amd-gfx@lists.freedesktop.org
>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
>> Subject: Re: [PATCH v6 06/11] drm/amdgpu: track the userq bo va for its obj
>> management
>>
>>
>>
>> On 11.07.25 11:39, Prike Liang wrote:
>>> The user queue object destroy requires ensuring its VA keeps mapping
>>> prior to the queue being destroyed.
>>> Otherwise, it seems a bug in the user space or VA freed wrongly, and
>>> the kernel driver should report an invalidated error to the user IOCLT
>>> request.
>>>
>>> Signed-off-by: Prike Liang <Prike.Liang@amd.com>
>>> ---
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 15 +++++++++++++++
>>>  1 file changed, 15 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>> index 2856c2506bee..81fbb00b6d91 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>> @@ -510,12 +510,24 @@ amdgpu_userq_destroy(struct drm_file *filp, int
>> queue_id)
>>>             return -EINVAL;
>>>     }
>>>     amdgpu_userq_wait_for_last_fence(uq_mgr, queue);
>>> +
>>> +   /*
>>> +    * At this point the userq obj va should be mapped,
>>> +    * otherwise will return error to user.
>>> +    */
>>> +   if (!amdgpu_userq_buffer_vas_mapped(&fpriv->vm, queue)) {
>>> +           drm_warn(adev_to_drm(uq_mgr->adev), "the userq obj va shouldn't
>> be umapped here\n");
>>> +           r = -EINVAL;
>>> +   }
>>> +
>>
>> That is still not something we can do.
>>
>> Destroying an userque can't fail in any way.
> Yes, the userq destroy will continue performing in this invalid case.
> Can we keep this part for detecting this invalid destroy case?

No, exactly that's the point there is no such thing as an invalid destroy case.

Perfectly valid to destroy the queue no matter what state we are in.

The only invalid operation would be trying to destroy a queue which doesn't exists in the first place.

Regards,
Christian.

> Furthermore, it looks like this error code will not affect the destroy request at
> userspace since the Mesa driver doesn't check the userq destroy return value.
> 
>> Regards,
>> Christian.
>>
>>>     r = amdgpu_userq_unmap_helper(uq_mgr, queue);
>>>     /*TODO: It requires a reset for userq hw unmap error*/
>>>     if (unlikely(r != AMDGPU_USERQ_STATE_UNMAPPED)) {
>>>             drm_warn(adev_to_drm(uq_mgr->adev), "trying to destroy a HW
>> mapping userq\n");
>>>             r = -ETIMEDOUT;
>>>     }
>>> +
>>> +   amdgpu_userq_buffer_vas_put(&fpriv->vm, queue);
>>>     amdgpu_userq_cleanup(uq_mgr, queue, queue_id);
>>>     mutex_unlock(&uq_mgr->userq_mutex);
>>>
>>> @@ -636,6 +648,9 @@ amdgpu_userq_create(struct drm_file *filp, union
>> drm_amdgpu_userq *args)
>>>             goto unlock;
>>>     }
>>>
>>> +   /* refer to the userq objects vm bo*/
>>> +   amdgpu_userq_buffer_vas_get(queue->vm, queue);
>>> +
>>>     qid = idr_alloc(&uq_mgr->userq_idr, queue, 1,
>> AMDGPU_MAX_USERQ_COUNT, GFP_KERNEL);
>>>     if (qid < 0) {
>>>             drm_file_err(uq_mgr->file, "Failed to allocate a queue id\n");
> 

RE: [PATCH v6 06/11] drm/amdgpu: track the userq bo va for its obj management

[Liang, Prike]

[Public]

Regards,
      Prike

> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig@amd.com>
> Sent: Tuesday, July 15, 2025 8:18 PM
> To: Liang, Prike <Prike.Liang@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
> Subject: Re: [PATCH v6 06/11] drm/amdgpu: track the userq bo va for its obj
> management
>
> On 15.07.25 14:05, Liang, Prike wrote:
> > [Public]
> >
> > Regards,
> >       Prike
> >
> >> -----Original Message-----
> >> From: Koenig, Christian <Christian.Koenig@amd.com>
> >> Sent: Friday, July 11, 2025 8:11 PM
> >> To: Liang, Prike <Prike.Liang@amd.com>; amd-gfx@lists.freedesktop.org
> >> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
> >> Subject: Re: [PATCH v6 06/11] drm/amdgpu: track the userq bo va for
> >> its obj management
> >>
> >>
> >>
> >> On 11.07.25 11:39, Prike Liang wrote:
> >>> The user queue object destroy requires ensuring its VA keeps mapping
> >>> prior to the queue being destroyed.
> >>> Otherwise, it seems a bug in the user space or VA freed wrongly, and
> >>> the kernel driver should report an invalidated error to the user
> >>> IOCLT request.
> >>>
> >>> Signed-off-by: Prike Liang <Prike.Liang@amd.com>
> >>> ---
> >>>  drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 15 +++++++++++++++
> >>>  1 file changed, 15 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> >>> index 2856c2506bee..81fbb00b6d91 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> >>> @@ -510,12 +510,24 @@ amdgpu_userq_destroy(struct drm_file *filp,
> >>> int
> >> queue_id)
> >>>             return -EINVAL;
> >>>     }
> >>>     amdgpu_userq_wait_for_last_fence(uq_mgr, queue);
> >>> +
> >>> +   /*
> >>> +    * At this point the userq obj va should be mapped,
> >>> +    * otherwise will return error to user.
> >>> +    */
> >>> +   if (!amdgpu_userq_buffer_vas_mapped(&fpriv->vm, queue)) {
> >>> +           drm_warn(adev_to_drm(uq_mgr->adev), "the userq obj va
> >>> + shouldn't
> >> be umapped here\n");
> >>> +           r = -EINVAL;
> >>> +   }
> >>> +
> >>
> >> That is still not something we can do.
> >>
> >> Destroying an userque can't fail in any way.
> > Yes, the userq destroy will continue performing in this invalid case.
> > Can we keep this part for detecting this invalid destroy case?
>
> No, exactly that's the point there is no such thing as an invalid destroy case.
>
> Perfectly valid to destroy the queue no matter what state we are in.
>
> The only invalid operation would be trying to destroy a queue which doesn't exists in
> the first place.
We might need a specific error code to detect invalid case of the VA unmapped before destroying,
otherwise, it's difficult to identify and detect the test results for the IGT negative test.

> Regards,
> Christian.
>
> > Furthermore, it looks like this error code will not affect the destroy
> > request at userspace since the Mesa driver doesn't check the userq destroy return
> value.
> >
> >> Regards,
> >> Christian.
> >>
> >>>     r = amdgpu_userq_unmap_helper(uq_mgr, queue);
> >>>     /*TODO: It requires a reset for userq hw unmap error*/
> >>>     if (unlikely(r != AMDGPU_USERQ_STATE_UNMAPPED)) {
> >>>             drm_warn(adev_to_drm(uq_mgr->adev), "trying to destroy a
> >>> HW
> >> mapping userq\n");
> >>>             r = -ETIMEDOUT;
> >>>     }
> >>> +
> >>> +   amdgpu_userq_buffer_vas_put(&fpriv->vm, queue);
> >>>     amdgpu_userq_cleanup(uq_mgr, queue, queue_id);
> >>>     mutex_unlock(&uq_mgr->userq_mutex);
> >>>
> >>> @@ -636,6 +648,9 @@ amdgpu_userq_create(struct drm_file *filp, union
> >> drm_amdgpu_userq *args)
> >>>             goto unlock;
> >>>     }
> >>>
> >>> +   /* refer to the userq objects vm bo*/
> >>> +   amdgpu_userq_buffer_vas_get(queue->vm, queue);
> >>> +
> >>>     qid = idr_alloc(&uq_mgr->userq_idr, queue, 1,
> >> AMDGPU_MAX_USERQ_COUNT, GFP_KERNEL);
> >>>     if (qid < 0) {
> >>>             drm_file_err(uq_mgr->file, "Failed to allocate a queue
> >>> id\n");
> >

RE: [PATCH v6 03/11] drm/amdgpu: rework the userq doorbell object destroy

[Liang, Prike]

[Public]

Regards,
      Prike

> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig@amd.com>
> Sent: Tuesday, July 15, 2025 4:49 PM
> To: Liang, Prike <Prike.Liang@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
> Subject: Re: [PATCH v6 03/11] drm/amdgpu: rework the userq doorbell object
> destroy
>
> On 15.07.25 10:07, Liang, Prike wrote:
> > [Public]
> >
> > Regards,
> >       Prike
> >
> >> -----Original Message-----
> >> From: Koenig, Christian <Christian.Koenig@amd.com>
> >> Sent: Friday, July 11, 2025 8:01 PM
> >> To: Liang, Prike <Prike.Liang@amd.com>; amd-gfx@lists.freedesktop.org
> >> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
> >> Subject: Re: [PATCH v6 03/11] drm/amdgpu: rework the userq doorbell
> >> object destroy
> >>
> >> On 11.07.25 11:39, Prike Liang wrote:
> >>> This patch aims to unify and destroy the userq doorbell objects at
> >>> mes_userq_mqd_destroy(), and this change will also help with
> >>> unpinning and destroying the userq doorbell objects for
> >>> amdgpu_userq_mgr_fini() during releasing the drm files.
> >>>
> >>> Signed-off-by: Prike Liang <Prike.Liang@amd.com>
> >>> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> >>> ---
> >>>  drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c  | 6 ------
> >>> drivers/gpu/drm/amd/amdgpu/mes_userqueue.c | 7 +++++++
> >>>  2 files changed, 7 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> >>> index 3d2a7f8946cf..15e833b1b3e3 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> >>> @@ -312,12 +312,6 @@ amdgpu_userq_destroy(struct drm_file *filp, int
> >> queue_id)
> >>>             return -EINVAL;
> >>>     }
> >>>     amdgpu_userq_wait_for_last_fence(uq_mgr, queue);
> >>> -   r = amdgpu_bo_reserve(queue->db_obj.obj, true);
> >>> -   if (!r) {
> >>> -           amdgpu_bo_unpin(queue->db_obj.obj);
> >>> -           amdgpu_bo_unreserve(queue->db_obj.obj);
> >>> -   }
> >>> -   amdgpu_bo_unref(&queue->db_obj.obj);
> >>>     r = amdgpu_userq_unmap_helper(uq_mgr, queue);
> >>>     /*TODO: It requires a reset for userq hw unmap error*/
> >>>     if (unlikely(r != AMDGPU_USERQ_STATE_UNMAPPED)) { diff --git
> >>> a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
> >>> b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
> >>> index 1457fb49a794..15aa1ca67a11 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
> >>> @@ -336,6 +336,13 @@ mes_userq_mqd_destroy(struct amdgpu_userq_mgr
> >> *uq_mgr,
> >>>                   struct amdgpu_usermode_queue *queue)  {
> >>>     amdgpu_userq_destroy_object(uq_mgr, &queue->fw_obj);
> >>> +
> >>> +   if (!amdgpu_bo_reserve(queue->db_obj.obj, true)) {
> >>> +           amdgpu_bo_unpin(queue->db_obj.obj);
> >>> +           amdgpu_bo_unreserve(queue->db_obj.obj);
> >>> +           amdgpu_userq_destroy_object(uq_mgr, &queue->db_obj);
> >>> +   }
> >>> +
> >>
> >> That makes no sense to do here. The pinning isn't done in
> >> mes_userq_mqd_create() either.
> > Yes, but the doorbell BO is pinned by
> > amdgpu_userq_get_doorbell_index(), which is still Invoked during userq
> > BOs creation phase. This patch wants to free the doorbell object like some other
> userq objects at the unified place of mes_userq_mqd_destroy().
>
> Yeah and exactly that is not a good idea.
>
> The doorbell object is provided by userspace and not allocated by the kernel like the
> MQD.
>
> So destroying it here makes no sense at all. You are most likely messing up the
> doorbell reference count with that.
>
> >> In general we should avoid pinning the MQD in the first place, that
> >> buffer needs to be fences instead.
> > If here not pin the userq doorbell BO, then will the doorbell index be
> > changed when the doorbell BO is moved?
>
> Correct, yes. The doorbell index needs to be updated on each resume of the
> userqueue.
>
> We haven't implemented that yet since we weren't sure if the MES FW could handle
> that (and because the eviction fences wasn't ready at that time).
Thank you for the history lesson. If there's no objections, I plan to rework the userq doorbell object management separately.

> Regards,
> Christian.
>
> >
> >> Regards,
> >> Christian.
> >>
> >>>     kfree(queue->userq_prop);
> >>>     amdgpu_userq_destroy_object(uq_mgr, &queue->mqd);  }
> >