Re: [PATCH 5/7] drm/amdgpu: fill the A+A VF PDB0 through get_vm_pde

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] drm/amdgpu: amdgpu_gmc_init_pdb0() corrupts GFX9 PDB0 via
  get_vm_pde()
--

commit 94fec17ddae148fbc91f8d2688eb078363097ddd
Author: Zhigang Luo <Zhigang.Luo@amd.com>

drm/amdgpu: fill the A+A VF PDB0 through get_vm_pde

This commit routes the VRAM PDE_PTE entries and the GART PTB PDE0 entry in
amdgpu_gmc_init_pdb0() through amdgpu_gmc_get_vm_pde().

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> index 659e89e5e78f1..04cf02dd22acb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> @@ -1272,7 +1272,7 @@ void amdgpu_gmc_init_pdb0(struct amdgpu_device *adev)
>  	u64 vram_size = adev->gmc.xgmi.node_segment_size * adev->gmc.xgmi.num_physical_nodes;
>  	u64 pde0_page_size = (1ULL<<adev->gmc.vmid0_page_table_block_size)<<21;
>  	u64 vram_addr, vram_end;
> -	u64 gart_ptb_gpu_pa = amdgpu_gmc_vram_pa(adev, adev->gart.bo);
> +	u64 pde_addr, pde_flags;
>  	int idx;
>  
>  	if (!drm_dev_enter(adev_to_drm(adev), &idx))
> @@ -1290,10 +1290,16 @@ void amdgpu_gmc_init_pdb0(struct amdgpu_device *adev)
>  	vram_end = vram_addr + vram_size;
>  
>  	/* The first n PDE0 entries are used as PTE,
> -	 * pointing to vram
> +	 * pointing to vram. PDE_PTE already holds a physical address, so
> +	 * get_vm_pde() skips translation and sets S for SPM.
>  	 */
> -	for (i = 0; vram_addr < vram_end; i++, vram_addr += pde0_page_size)
> -		amdgpu_gmc_set_pte_pde(adev, adev->gmc.ptr_pdb0, i, vram_addr, flags);
> +	for (i = 0; vram_addr < vram_end; i++, vram_addr += pde0_page_size) {
> +		pde_addr = vram_addr;
> +		pde_flags = flags;
> +		amdgpu_gmc_get_vm_pde(adev, AMDGPU_VM_PDB0, &pde_addr, &pde_flags);
> +		amdgpu_gmc_set_pte_pde(adev, adev->gmc.ptr_pdb0, i, pde_addr,
> +				       pde_flags);
> +	}
>  
>  	/* The n+1'th PDE0 entry points to a huge
>  	 * PTB who has more than 512 entries each
> @@ -1301,8 +1307,12 @@ void amdgpu_gmc_init_pdb0(struct amdgpu_device *adev)
>  	 */
>  	flags = AMDGPU_PTE_VALID;
>  	flags |= AMDGPU_PTE_SNOOPED | AMDGPU_PDE_BFS_FLAG(adev, 0);
> -	/* Requires gart_ptb_gpu_pa to be 4K aligned */
> -	amdgpu_gmc_set_pte_pde(adev, adev->gmc.ptr_pdb0, i, gart_ptb_gpu_pa, flags);
> +	/* MC address. get_vm_pde() translates it once and sets S for SPM.
> +	 * Requires the GART PT to be 4K aligned.
> +	 */
> +	pde_addr = amdgpu_bo_gpu_offset(adev->gart.bo);
> +	amdgpu_gmc_get_vm_pde(adev, AMDGPU_VM_PDB0, &pde_addr, &flags);
> +	amdgpu_gmc_set_pte_pde(adev, adev->gmc.ptr_pdb0, i, pde_addr, flags);

[Severity: High]
Does passing AMDGPU_VM_PDB0 to amdgpu_gmc_get_vm_pde() corrupt the VMID0
PDB0 table on GFX9 devices?

amdgpu_gmc_init_pdb0() is also called from gmc_v9_0_gart_enable() on GFX9
ASICs with PDB0 enabled (such as Aldebaran and Aqua Vanjaram), where
adev->gmc.translate_further is true for 4-level user VMs:

drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c:gmc_v9_0_get_vm_pde() {
	...
	if (!adev->gmc.translate_further)
		return;

	if (level == AMDGPU_VM_PDB1) {
		...
	} else if (level == AMDGPU_VM_PDB0) {
		if (*flags & AMDGPU_PDE_PTE) {
			*flags &= ~AMDGPU_PDE_PTE;
			if (!(*flags & AMDGPU_PTE_VALID))
				*addr |= 1 << PAGE_SHIFT;
		} else {
			*flags |= AMDGPU_PTE_TF;
		}
	}
}

Unlike VM_CONTEXT1..15, VM_CONTEXT0_CNTL.PAGE_TABLE_DEPTH is programmed to
vmid0_page_table_depth (1) without subtracting 1 for translate_further.
Clearing AMDGPU_PDE_PTE on the VRAM entries causes the hardware walker to
treat each VRAM physical address as a child PTB pointer instead of a leaf
PTE, and setting AMDGPU_PTE_TF on the GART entry sets a PTE-only bit in a
depth-1 PDE0 entry, breaking system-domain VRAM and GART translation on
GFX9.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113145702.323968-1-Zhigang.Luo@amd.com?part=5