[PATCH 1/3] drm/amdgpu: don't wait for the SPM VMID's jobs under the VMID manager lock

[PATCH 1/3] drm/amdgpu: don't wait for the SPM VMID's jobs under the VMID manager lock

[Jesse Zhang]

amdgpu_vmid_free_spm() waits for the jobs of the reserved VMID while
holding the VMID manager lock of the hub, which every VMID grab on the
hub takes as well. Freeing the reservation, by UNRESERVE_VMID or when
the VM is destroyed, so stalls the kernel queue submissions of every
process on the GFXHUB until those jobs completed, or until the job
timeout if one of them hangs. A full GPU reset even deadlocks, since
amdgpu_vmid_reset_all() needs the lock before the scheduler is started
again to signal the jobs.

On navi31, with a GFX job of the reserving process that waits for 1s,
an unrelated process' SDMA NOP takes 0.8s instead of no time when the
VMID gets unreserved meanwhile.

Nothing adds to the active fences of the VMID once it's no longer the
VM's reserved one, so drop the lock while waiting for them. Keep the
VMID reserved until then, so that nobody else can bind it before.

Fixes: 9b47649cabf4 ("drm/amdgpu: bind the SPM VMID's page table to the reserving VM")
Signed-off-by: Jesse Zhang <Jesse.Zhang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
index 1482fc031d44..d19bbcf25765 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
@@ -645,17 +645,18 @@ void amdgpu_vmid_free_spm(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 
 	mutex_lock(&id_mgr->lock);
 	id = vm->reserved_vmid[vmhub];
-	if (id) {
-		vm->reserved_vmid[vmhub] = NULL;
-		id_mgr->reserved_vmid = false;
+	vm->reserved_vmid[vmhub] = NULL;
+	mutex_unlock(&id_mgr->lock);
+	if (!id)
+		return;
 
-		/* Only for kernel queues and kfd/userqueues by pass that */
-		amdgpu_sync_wait(&id->active, false);
+	amdgpu_sync_wait(&id->active, false);
 
-		if (adev->vm_manager.spm_vmid && adev->gfxhub.funcs)
-			amdgpu_gmc_bind_vmid(adev, AMDGPU_GFXHUB(0),
-					     adev->vm_manager.spm_vmid, 0);
-	}
+	mutex_lock(&id_mgr->lock);
+	if (adev->vm_manager.spm_vmid && adev->gfxhub.funcs)
+		amdgpu_gmc_bind_vmid(adev, AMDGPU_GFXHUB(0),
+				     adev->vm_manager.spm_vmid, 0);
+	id_mgr->reserved_vmid = false;
 	mutex_unlock(&id_mgr->lock);
 }
 
-- 
2.50.1

[PATCH 2/3] drm/amdgpu: keep the CS user fence BO in GTT

[Jesse Zhang]

The CS ioctl hands the GPU the GART address of the user fence BO, but
since the BO keeps its own preferred and allowed domains, it no longer
makes sure the BO is in GTT. A user fence in a VRAM BO, which only needs
to be a page in size, gets its address from amdgpu_bo_gpu_offset(), which
warns about non-contiguous VRAM. On navi31, a CS with a user fence in a
4K VRAM BO, or in one created with AMDGPU_GEM_CREATE_VM_ALWAYS_VALID:

  WARNING: amdgpu_object.c:1531 at amdgpu_bo_gpu_offset+0x86/0xb0 [amdgpu], CPU#11: cs_gem_checks/18393
  RIP: 0010:amdgpu_bo_gpu_offset+0x86/0xb0 [amdgpu]
  Call Trace:
   <TASK>
   amdgpu_cs_parser_bos.isra.0+0x4eb/0x850 [amdgpu]
   amdgpu_cs_ioctl+0x97e/0x2510 [amdgpu]
   drm_ioctl_kernel+0xac/0x110 [drm]
   drm_ioctl+0x2c6/0x5b0 [drm]
   __x64_sys_ioctl+0xa0/0xf0
   do_syscall_64+0xa3/0x4d0
   entry_SYSCALL_64_after_hwframe+0x76/0x7e

and BOs which can't be in GTT at all, like GWS or OA ones of a page in
size, pass the checks as well.

Only take BOs which can be in GTT as user fence and validate them only
there, like before, along with the other BOs of the CS. Validate the
user fence BO explicitly as well, a per VM BO shares the reservation of
the root PD and so isn't one of the locked objects of the CS.

Fixes: 1ea863fd736e ("drm/amdgpu: keep the prefered/allowed domains in the BO")
Signed-off-by: Jesse Zhang <Jesse.Zhang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index abd8a756779e..62758866378e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -153,6 +153,9 @@ static int amdgpu_cs_p1_user_fence(struct amdgpu_cs_parser *p,
 	if (amdgpu_ttm_tt_get_usermm(p->uf_bo->tbo.ttm))
 		return -EINVAL;
 
+	if (!(p->uf_bo->allowed_domains & AMDGPU_GEM_DOMAIN_GTT))
+		return -EINVAL;
+
 	*offset = data->offset;
 	return 0;
 }
@@ -823,6 +826,14 @@ static int amdgpu_cs_bo_validate(void *param, struct amdgpu_bo *bo)
 	uint32_t domain;
 	int r;
 
+	/* The GPU writes the user fence through the GART */
+	if (bo == p->uf_bo) {
+		if (bo->tbo.pin_count)
+			return bo->tbo.resource->mem_type == TTM_PL_TT ? 0 : -EINVAL;
+		domain = AMDGPU_GEM_DOMAIN_GTT;
+		goto retry;
+	}
+
 	if (bo->tbo.pin_count)
 		return 0;
 
@@ -858,7 +869,8 @@ static int amdgpu_cs_bo_validate(void *param, struct amdgpu_bo *bo)
 	    amdgpu_res_cpu_visible(adev, bo->tbo.resource))
 		p->bytes_moved_vis += ctx.bytes_moved;
 
-	if (unlikely(r == -ENOMEM) && domain != bo->allowed_domains) {
+	if (unlikely(r == -ENOMEM) && domain != bo->allowed_domains &&
+	    bo != p->uf_bo) {
 		domain = bo->allowed_domains;
 		goto retry;
 	}
@@ -992,6 +1004,10 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
 	}
 
 	if (p->uf_bo) {
+		r = amdgpu_cs_bo_validate(p, p->uf_bo);
+		if (unlikely(r))
+			goto out_free_user_pages;
+
 		r = amdgpu_ttm_alloc_gart(&p->uf_bo->tbo);
 		if (unlikely(r))
 			goto out_free_user_pages;
-- 
2.50.1

[PATCH 3/3] drm/amdgpu: reject CS chunks too large for kvmalloc()

[Jesse Zhang]

The CS ioctl copies every chunk with vmemdup_array_user() before it
looks at it, for whatever length_dw userspace gives. kvmalloc() warns
about allocations above INT_MAX when called without __GFP_NOWARN, so any
process with the render node can trigger a warning. On navi31, a CS with
a chunk of length_dw 0x20000000 returns -ENOMEM after:

  WARNING: mm/slub.c:6841 at __kvmalloc_node_noprof+0x5a5/0x7d0

Reject such chunks with -EINVAL before trying to copy them.

Fixes: 2098105ec65c ("drm: drop drm_[cm]alloc* helpers")
Signed-off-by: Jesse Zhang <Jesse.Zhang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 62758866378e..649bf0947e68 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -221,6 +221,13 @@ static int amdgpu_cs_pass1(struct amdgpu_cs_parser *p,
 
 		size = p->chunks[i].length_dw;
 
+		if (size > INT_MAX / sizeof(uint32_t)) {
+			ret = -EINVAL;
+			i--;
+			goto free_partial_kdata;
+		}
+
 		p->chunks[i].kdata = vmemdup_array_user(u64_to_user_ptr(user_chunk.chunk_data),
 							size,
 							sizeof(uint32_t));
-- 
2.50.1

Re: [PATCH 3/3] drm/amdgpu: reject CS chunks too large for kvmalloc()

[Christian König]

On 10/20/26 10:37, Jesse Zhang wrote:
> The CS ioctl copies every chunk with vmemdup_array_user() before it
> looks at it, for whatever length_dw userspace gives. kvmalloc() warns
> about allocations above INT_MAX when called without __GFP_NOWARN, so any
> process with the render node can trigger a warning. On navi31, a CS with
> a chunk of length_dw 0x20000000 returns -ENOMEM after:
> 
>   WARNING: mm/slub.c:6841 at __kvmalloc_node_noprof+0x5a5/0x7d0
> 
> Reject such chunks with -EINVAL before trying to copy them.
> 
> Fixes: 2098105ec65c ("drm: drop drm_[cm]alloc* helpers")
> Signed-off-by: Jesse Zhang <Jesse.Zhang@amd.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 62758866378e..649bf0947e68 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -221,6 +221,13 @@ static int amdgpu_cs_pass1(struct amdgpu_cs_parser *p,
>  
>  		size = p->chunks[i].length_dw;
>  
> +		if (size > INT_MAX / sizeof(uint32_t)) {
> +			ret = -EINVAL;
> +			i--;
> +			goto free_partial_kdata;
> +		}
> +
>  		p->chunks[i].kdata = vmemdup_array_user(u64_to_user_ptr(user_chunk.chunk_data),
>  							size,
>  							sizeof(uint32_t));

Re: [PATCH 1/3] drm/amdgpu: don't wait for the SPM VMID's jobs under the VMID manager lock

[Christian König]

On 10/20/26 10:37, Jesse Zhang wrote:
> amdgpu_vmid_free_spm() waits for the jobs of the reserved VMID while
> holding the VMID manager lock of the hub, which every VMID grab on the
> hub takes as well. Freeing the reservation, by UNRESERVE_VMID or when
> the VM is destroyed, so stalls the kernel queue submissions of every
> process on the GFXHUB until those jobs completed, or until the job
> timeout if one of them hangs. A full GPU reset even deadlocks, since
> amdgpu_vmid_reset_all() needs the lock before the scheduler is started
> again to signal the jobs.
> 
> On navi31, with a GFX job of the reserving process that waits for 1s,
> an unrelated process' SDMA NOP takes 0.8s instead of no time when the
> VMID gets unreserved meanwhile.
> 
> Nothing adds to the active fences of the VMID once it's no longer the
> VM's reserved one, so drop the lock while waiting for them. Keep the
> VMID reserved until then, so that nobody else can bind it before.
> 
> Fixes: 9b47649cabf4 ("drm/amdgpu: bind the SPM VMID's page table to the reserving VM")
> Signed-off-by: Jesse Zhang <Jesse.Zhang@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> index 1482fc031d44..d19bbcf25765 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> @@ -645,17 +645,18 @@ void amdgpu_vmid_free_spm(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>  
>  	mutex_lock(&id_mgr->lock);
>  	id = vm->reserved_vmid[vmhub];
> -	if (id) {
> -		vm->reserved_vmid[vmhub] = NULL;
> -		id_mgr->reserved_vmid = false;
> +	vm->reserved_vmid[vmhub] = NULL;
> +	mutex_unlock(&id_mgr->lock);
> +	if (!id)
> +		return;
>  
> -		/* Only for kernel queues and kfd/userqueues by pass that */
> -		amdgpu_sync_wait(&id->active, false);
> +	amdgpu_sync_wait(&id->active, false);

Accessing id->active without holding the lock is a pretty big no-go as well.

We should probably rather move the amdgpu_sync_wait into the caller of the function and wait for vm->root.bo->tbo.base.resv instead.

That should have the same effect, but we don't need to hold the VMID lock for that.

Regards,
Christian.

>  
> -		if (adev->vm_manager.spm_vmid && adev->gfxhub.funcs)
> -			amdgpu_gmc_bind_vmid(adev, AMDGPU_GFXHUB(0),
> -					     adev->vm_manager.spm_vmid, 0);
> -	}
> +	mutex_lock(&id_mgr->lock);
> +	if (adev->vm_manager.spm_vmid && adev->gfxhub.funcs)
> +		amdgpu_gmc_bind_vmid(adev, AMDGPU_GFXHUB(0),
> +				     adev->vm_manager.spm_vmid, 0);
> +	id_mgr->reserved_vmid = false;
>  	mutex_unlock(&id_mgr->lock);
>  }
>