Re: [PATCH 4/7] drm/amdgpu: set the A+A VF SYSTEM bit in the GFX 12.1 helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] drm/amdgpu: gmc_v12_1_get_vm_pde() sets AMDGPU_PTE_SYSTEM on
  invalid and NPA PDEs
- [Medium] drm/amdgpu: gmc_v12_1_get_vm_pte() sets AMDGPU_PTE_SYSTEM
  when PTE is invalid
--

commit 914948e43f4fe117dbe355b73ada0eb4485a66d2
Author: Zhigang Luo <Zhigang.Luo@amd.com>

drm/amdgpu: set the A+A VF SYSTEM bit in the GFX 12.1 helpers

This commit sets AMDGPU_PTE_SYSTEM in gmc_v12_1_get_vm_pde() and
gmc_v12_1_get_vm_pte() when VRAM is backed by system physical memory on
SR-IOV VFs.

> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v12_1.c b/drivers/gpu/drm/amd/amdgpu/gmc_v12_1.c
> index 57aac3a9a3b0f..1c5998fa3ad6e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v12_1.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v12_1.c
> @@ -586,6 +586,12 @@ static void gmc_v12_1_get_vm_pde(struct amdgpu_device *adev, int level,
>  		*addr = adev->vm_manager.vram_base_offset + *addr -
>  			adev->gmc.vram_start;
>  
> +	/* SPM VRAM is reached at a system physical address. Set S after the
> +	 * translation above so that translation is not skipped.
> +	 */
> +	if (amdgpu_virt_vram_is_spm(adev))
> +		*flags |= AMDGPU_PTE_SYSTEM;
> +
>  	if (*addr & ~adev->gmc.pte_addr_mask) {
>  		dev_err(adev->dev, "Invalid PDE address: %llx\n", *addr);
>  		*addr = 0;

[Severity: High]
Does setting AMDGPU_PTE_SYSTEM here without checking (*flags &
AMDGPU_PTE_VALID) corrupt cleared PDEs and 2MB+ NPA leaf PDEs?

On GFX 12, gmc_v12_1_get_vm_pde() always sets AMDGPU_PTE_SNOOPED, and
(AMDGPU_PTE_SYSTEM | AMDGPU_PTE_SNOOPED) on an invalid entry is
AMDGPU_VM_NORETRY_FLAGS_GFX12.  When amdgpu_vm_pt_clear() clears a
non-PTB page directory (with AMDGPU_PTE_VALID == 0), this turns every
cleared PDE in a normal VM into a fatal no-retry fault entry, breaking
SVM demand paging on unpopulated ranges.

For 2MB+ AMDGPU_PL_NPA mappings updated at a leaf PDE level in
amdgpu_vm_pte_update_flags(), gmc_v12_1_get_npa_flags() clears
AMDGPU_PTE_VALID and leaves AMDGPU_PTE_SYSTEM clear (VSCT = 0011), which
amdgpu_gmc_get_vm_pde() then overwrites with AMDGPU_PTE_SYSTEM.

> @@ -753,6 +759,10 @@ static void gmc_v12_1_get_vm_pte(struct amdgpu_device *adev,
>  
>  	if (bo) {
>  		mem = bo->tbo.resource;
> +		if (amdgpu_virt_vram_is_spm(adev) &&
> +		    mem && mem->mem_type == TTM_PL_VRAM)
> +			*flags |= AMDGPU_PTE_SYSTEM;
> +
>  		if (mem && mem->mem_type == AMDGPU_PL_NPA) {
>  			dev_dbg(adev->dev,
>  				"Setting PTE for NPA BO, mem->type: %d, mem->start: %lx, mem->size: %u, cur_flags: %llx\n",
> @@ -761,6 +771,9 @@ static void gmc_v12_1_get_vm_pte(struct amdgpu_device *adev,
>  		} else if (*flags & AMDGPU_PTE_VALID) {
>  			gmc_v12_1_get_coherence_flags(adev, bo, flags);
>  		}
> +	} else if (amdgpu_virt_vram_is_spm(adev)) {
> +		/* SVM has no BO. System memory already has S. VRAM does not. */
> +		*flags |= AMDGPU_PTE_SYSTEM;
>  	}
>  }

[Severity: Medium]
Should both branches check (*flags & AMDGPU_PTE_VALID) before setting
AMDGPU_PTE_SYSTEM?

When amdgpu_vm_handle_moved() calls amdgpu_vm_bo_update() with clear =
true after dma_resv_trylock() fails, bo->tbo.base.resv is not held and
update_flags has AMDGPU_PTE_VALID == 0.  Checking mem->mem_type before
(*flags & AMDGPU_PTE_VALID) reads bo->tbo.resource without holding the
reservation lock and sets AMDGPU_PTE_SYSTEM on cleared PTEs.

In the else branch, amdgpu_vm_bo_update() also passes bo == NULL with
AMDGPU_PTE_VALID == 0 for PRT/sparse mappings (fpriv->prt_va), so
setting AMDGPU_PTE_SYSTEM unconditionally pollutes invalid non-SVM PTEs
(and forms AMDGPU_VM_NORETRY_FLAGS_GFX12 if a cleared or PRT mapping
spans a 2MB+ leaf PDE).

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