[PATCH v2] drm/amdgpu: clear root PT after setting vm->is_npa = true

[PATCH v2] drm/amdgpu: clear root PT after setting vm->is_npa = true

[Shahyan Soltani]

Previously, the root PT was never cleared after setting vm->is_npa =
true within amdgpu_vm_make_npa(). This resulted in the flag
adev->gmc.no_retry_flags to never be applied for the NPA VMs.

Call amdgpu_vm_pt_clear() after vm->is_npa = true is set and the bo has
been pinned/validated. This allows the root PT of the NPA VM to be
cleared with the correct value.

If the pinning or reservation of the bo failed, the vm->is_npa flag will
no longer remain as true. This is to avoid conditionals outside of
amdgpu_vm_make_npa() that depend on the vm->is_npa flag to determine if
the VM is a correctly initialized NPA VM.

Warn if the VM already has mappings, since clearing the root PT will
break existing mappings. Do a heavy weight TLB flush after the root PT
clear to avoid any stale translations being left behind from a previous
table.

Remove the redundant second setting of vm->is_npa = true.

v2:
- Add the WARN_ON sanity check regarding an empty VM. (Felix)
- Change the return handling to only set vm->is_npa = true upon a
  successful pin (Felix)
- Add the heavy weight TLB flush after the root PT clear (Philip)

Suggested-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Shahyan Soltani <shahyan.soltani@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 94b1cfe05e3e..c7187e0f99b3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2820,20 +2820,27 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm)
  */
 int amdgpu_vm_make_npa(struct amdgpu_device *adev, struct amdgpu_vm *vm)
 {
-	int r = amdgpu_vm_make_compute(adev, vm);
+	int r;
 
+	r = amdgpu_vm_make_compute(adev, vm);
 	if (r)
 		return r;
-	vm->is_npa = true;
+
 	r = amdgpu_bo_reserve(vm->root.bo, false);
 	if (r)
 		return r;
 	r = amdgpu_bo_pin(vm->root.bo, AMDGPU_GEM_DOMAIN_VRAM);
+	if (!r) {
+		/* Check if VM mappings are empty, clearing would break existing mapping */
+		WARN_ON(!RB_EMPTY_ROOT(&vm->va.rb_root));
+		vm->is_npa = true;
+		r = amdgpu_vm_pt_clear(adev, vm, to_amdgpu_bo_vm(vm->root.bo), false);
+		amdgpu_ualink_flush_tlb(adev, TLB_FLUSH_HEAVYWEIGHT);
+	}
 	amdgpu_bo_unreserve(vm->root.bo);
 	if (r)
 		return r;
 
-	vm->is_npa = true;
 	vm->need_tlb_fence = false;
 
 	return 0;
-- 
2.56.0

Re: [PATCH v2] drm/amdgpu: clear root PT after setting vm->is_npa = true

[Kuehling, Felix]

On 2026-11-13 15:29, Shahyan Soltani wrote:
> Previously, the root PT was never cleared after setting vm->is_npa =
> true within amdgpu_vm_make_npa(). This resulted in the flag
> adev->gmc.no_retry_flags to never be applied for the NPA VMs.
>
> Call amdgpu_vm_pt_clear() after vm->is_npa = true is set and the bo has
> been pinned/validated. This allows the root PT of the NPA VM to be
> cleared with the correct value.
>
> If the pinning or reservation of the bo failed, the vm->is_npa flag will
> no longer remain as true. This is to avoid conditionals outside of
> amdgpu_vm_make_npa() that depend on the vm->is_npa flag to determine if
> the VM is a correctly initialized NPA VM.
>
> Warn if the VM already has mappings, since clearing the root PT will
> break existing mappings. Do a heavy weight TLB flush after the root PT
> clear to avoid any stale translations being left behind from a previous
> table.
>
> Remove the redundant second setting of vm->is_npa = true.
>
> v2:
> - Add the WARN_ON sanity check regarding an empty VM. (Felix)
> - Change the return handling to only set vm->is_npa = true upon a
>    successful pin (Felix)
> - Add the heavy weight TLB flush after the root PT clear (Philip)
>
> Suggested-by: Christian König <christian.koenig@amd.com>
> Signed-off-by: Shahyan Soltani <shahyan.soltani@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 13 ++++++++++---
>   1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 94b1cfe05e3e..c7187e0f99b3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2820,20 +2820,27 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>    */
>   int amdgpu_vm_make_npa(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>   {
> -	int r = amdgpu_vm_make_compute(adev, vm);
> +	int r;
>   
> +	r = amdgpu_vm_make_compute(adev, vm);
>   	if (r)
>   		return r;
> -	vm->is_npa = true;
> +
>   	r = amdgpu_bo_reserve(vm->root.bo, false);
>   	if (r)
>   		return r;
>   	r = amdgpu_bo_pin(vm->root.bo, AMDGPU_GEM_DOMAIN_VRAM);
> +	if (!r) {
> +		/* Check if VM mappings are empty, clearing would break existing mapping */
> +		WARN_ON(!RB_EMPTY_ROOT(&vm->va.rb_root));
> +		vm->is_npa = true;
> +		r = amdgpu_vm_pt_clear(adev, vm, to_amdgpu_bo_vm(vm->root.bo), false);
> +		amdgpu_ualink_flush_tlb(adev, TLB_FLUSH_HEAVYWEIGHT);

The TLB flush isn't really needed IMO. We don't expect there to be any 
previous mappings that would need to be flushed. If there were previous 
mappings, we're in a broken state anyway, and the WARN_ON would tell you 
about it.

With the flush_tlb removed, this patch is

Reviewed-by: Felix Kuehling <felix.kuehling@amd.com>


> +	}
>   	amdgpu_bo_unreserve(vm->root.bo);
>   	if (r)
>   		return r;
>   
> -	vm->is_npa = true;
>   	vm->need_tlb_fence = false;
>   
>   	return 0;

Re: [PATCH v2] drm/amdgpu: clear root PT after setting vm->is_npa = true

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [High] drm/amdgpu: build failure calling static
  amdgpu_ualink_flush_tlb()
- [High] drm/amdgpu: missing dma_resv_reserve_fences() in
  amdgpu_vm_make_npa()
- [Medium] drm/amdgpu: missing error handling on amdgpu_vm_pt_clear()
  failure
- [Medium] drm/amdgpu: premature TLB flush in amdgpu_vm_make_npa()
- [Low] drm/amdgpu: missing Fixes trailer on amdgpu_vm_make_npa() bug
  fix
--

commit d44f1102f39999e9e70a8ca65fdacefed70663a9
Author: Shahyan Soltani <shahyan.soltani@amd.com>

> drm/amdgpu: clear root PT after setting vm->is_npa = true
>
> Previously, the root PT was never cleared after setting vm->is_npa =
> true within amdgpu_vm_make_npa(). This resulted in the flag
> adev->gmc.no_retry_flags to never be applied for the NPA VMs.
>
> Call amdgpu_vm_pt_clear() after vm->is_npa = true is set and the bo has
> been pinned/validated. This allows the root PT of the NPA VM to be
> cleared with the correct value.
>
> If the pinning or reservation of the bo failed, the vm->is_npa flag will
> no longer remain as true. This is to avoid conditionals outside of
> amdgpu_vm_make_npa() that depend on the vm->is_npa flag to determine if
> the VM is a correctly initialized NPA VM.
>
> Warn if the VM already has mappings, since clearing the root PT will
> break existing mappings. Do a heavy weight TLB flush after the root PT
> clear to avoid any stale translations being left behind from a previous
> table.
>
> Remove the redundant second setting of vm->is_npa = true.
>
> v2:
> - Add the WARN_ON sanity check regarding an empty VM. (Felix)
> - Change the return handling to only set vm->is_npa = true upon a
>   successful pin (Felix)
> - Add the heavy weight TLB flush after the root PT clear (Philip)
>
> Suggested-by: Christian König <christian.koenig@amd.com>
> Signed-off-by: Shahyan Soltani <shahyan.soltani@amd.com>

[Severity: Low]
Since this fixes the initialization of the root page directory in
amdgpu_vm_make_npa(), should it include a Fixes tag?

  Fixes: e05a54573df7 ("drm/amdgpu: Pin page tables in NPA VMs")

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 2ba33212d5071..5c51dd6bd8d2f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2818,20 +2818,27 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>   */
>  int amdgpu_vm_make_npa(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>  {
> -	int r = amdgpu_vm_make_compute(adev, vm);
> +	int r;
>  
> +	r = amdgpu_vm_make_compute(adev, vm);
>  	if (r)
>  		return r;
> -	vm->is_npa = true;
> +
>  	r = amdgpu_bo_reserve(vm->root.bo, false);
>  	if (r)
>  		return r;
>  	r = amdgpu_bo_pin(vm->root.bo, AMDGPU_GEM_DOMAIN_VRAM);
> +	if (!r) {
> +		/* Check if VM mappings are empty, clearing would break existing mapping */
> +		WARN_ON(!RB_EMPTY_ROOT(&vm->va.rb_root));
> +		vm->is_npa = true;
> +		r = amdgpu_vm_pt_clear(adev, vm, to_amdgpu_bo_vm(vm->root.bo), false);

[Severity: High]
Does amdgpu_vm_pt_clear() need a fence slot reserved on
vm->root.bo->tbo.base.resv first, as in amdgpu_vm_init()?

Earlier in amdgpu_vm_make_npa(), amdgpu_vm_make_compute() unreserves
vm->root.bo, which resets fobj->max_fences when mutex debugging is
enabled.  Because vm->root.bo is already in compatible memory,
ttm_bo_validate() inside amdgpu_bo_pin() and amdgpu_vm_pt_clear() returns
early without calling dma_resv_reserve_fences().  When page tables are
updated via sdma, amdgpu_vm_pt_clear() -> amdgpu_vm_sdma_commit() calls
dma_resv_add_fence() on vm->root.bo->tbo.base.resv and triggers a kernel
crash when fobj->num_fences >= fobj->max_fences.

> +		amdgpu_ualink_flush_tlb(adev, TLB_FLUSH_HEAVYWEIGHT);

[Severity: High]
Does this break the build by calling amdgpu_ualink_flush_tlb() from
amdgpu_vm.c?

amdgpu_ualink_flush_tlb() is declared and defined as static in
drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c and has no prototype in any
header, causing an implicit function declaration error when compiling
amdgpu_vm.c and an undefined reference at link time.

[Severity: Medium]
Can amdgpu_ualink_flush_tlb() also run here before the root page directory
clear has taken effect?

When vm->use_cpu_for_update is false, amdgpu_vm_pt_clear() submits an
asynchronous sdma job stored in vm->last_update, and nothing waits on
vm->last_update before flushing.  In addition, the caller
amdgpu_ualink_manager_start() only binds npa_vm.root.bo to
adev->vm_manager.npa_vmid via adev->mmhub.funcs->setup_vm_pt_regs() after
amdgpu_vm_make_npa() returns, so flushing npa_vmid inside
amdgpu_vm_make_npa() happens before the page table base register is
programmed.

> +	}
>  	amdgpu_bo_unreserve(vm->root.bo);
>  	if (r)
>  		return r;

[Severity: Medium]
If amdgpu_vm_pt_clear() fails, does this still flush the tlb and return an
error with vm->is_npa set to true and vm->root.bo left pinned?

Checking r after amdgpu_vm_pt_clear() and unpinning vm->root.bo while
resetting vm->is_npa = false on failure would avoid leaving the vm in a
half-initialized state.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113202944.3006069-1-shahyan.soltani@amd.com?part=1