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

[sashiko-bot@kernel.org]

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