Re: [PATCH] drm/amdgpu: clear the entire mapping when partially unmapping

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] drm/amdgpu: unmapped split endpoints in
  amdgpu_vm_bo_clear_mappings()
- [Low] drm/amdgpu: missing Fixes trailer in commit message
--

commit 193793bc04d0356a7faf15b1ecf5a2a8fdd585ce
Author: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>

> drm/amdgpu: clear the entire mapping when partially unmapping
>
> When we partially clear a BO mapping, we only add the partially unmapped
> region to the freed list. That will update the page table to clear that
> mapping and leave the other endpoints mapped.
>
> However, we move the endpoints to the invalids list. When later closing
> that BO, it will simply free the mapping and not clear the GPU page
> tables.
>
> Drop the optimization and clear the entire mapping from the page tables.
> As the endpoints are already in the invalids list, when that BO mapping
> needs to be accessed, the page tables will be updated.
>
> Though that may clear, then write page table entries and even lead to
> new page table allocations in certain situations, those would already
> happen in the clear path.
>
> When measuring a given workload, it was noticed an extra allocation in
> less than 10% of the cases, but no noticeable extra latency. In any
> case, correctness is more important and this fixes the scenario
> described.
>
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>

[Severity: Low]
Since this fixes a page table leak on partial unmap, should it include a
Fixes: tag for 387f49e54672 ("drm/amdgpu: fix clear_all and replace
handling in the VM (v2)") or dc54d3d1744d ("drm/amdgpu: implement
AMDGPU_VA_OP_CLEAR v2")?

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index a99c2ef14be96..808b77083a5e4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2108,12 +2108,6 @@ int amdgpu_vm_bo_clear_mappings(struct amdgpu_device *adev,
>  	list_for_each_entry_safe(tmp, next, &removed, list) {
>  		amdgpu_vm_it_remove(tmp, &vm->va);
>  		list_del(&tmp->list);
> -
> -		if (tmp->start < saddr)
> -		    tmp->start = saddr;
> -		if (tmp->last > eaddr)
> -		    tmp->last = eaddr;
> -
>  		tmp->bo_va = NULL;
>  		list_add(&tmp->list, &vm->freed);
>  		trace_amdgpu_vm_bo_unmap(NULL, tmp);

[Severity: High]
Does queuing the unclamped mapping onto vm->freed leave the surviving
before and after endpoints unmapped in the hardware page tables?

When amdgpu_vm_clear_freed() processes vm->freed, it now zeroes the PTEs
for the entire original [tmp->start, tmp->last] range, wiping the PTEs of
the surviving before and after sub-ranges.  However, two paths fail to
re-populate those endpoints from bo_va->invalids:

1. In amdgpu_vm_bo_clear_mappings(), amdgpu_vm_bo_needs_update() is only
called when amdgpu_vm_is_bo_always_valid(vm, bo) is true:

drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:amdgpu_vm_bo_clear_mappings() {
    ...
	if (!list_empty(&before->list)) {
		struct amdgpu_bo *bo = before->bo_va->base.bo;
    ...
		if (amdgpu_vm_is_bo_always_valid(vm, bo) &&
		    !before->bo_va->base.moved)
			amdgpu_vm_bo_needs_update(&before->bo_va->base);
	}
    ...
}

For normal individual BOs, before->bo_va and after->bo_va stay on
vm->individual.idle instead of moving to vm->individual.needs_update, so
amdgpu_vm_handle_moved() and amdgpu_userq_bo_validate() skip them and
never restore their cleared PTEs unless the BO is explicitly passed in a
legacy CS bo_list.

2. On immediate updates without AMDGPU_VM_DELAY_UPDATE,
amdgpu_gem_va_update_vm() runs amdgpu_vm_clear_freed() and only calls
amdgpu_vm_bo_update() on the ioctl target bo_va for MAP or REPLACE:

drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c:amdgpu_gem_va_update_vm() {
    ...
	r = amdgpu_vm_clear_freed(adev, vm, &vm->last_update);
	if (r)
		goto error;

	/* For MAP/REPLACE we also need to update the BO mappings. */
	if (operation == AMDGPU_VA_OP_MAP ||
	    operation == AMDGPU_VA_OP_REPLACE) {
		r = amdgpu_vm_bo_update(adev, bo_va, false);
		if (r)
			goto error;
	}
    ...
}

When userspace partially unmaps via AMDGPU_VA_OP_CLEAR (where bo_va is
NULL) or splits a mapping belonging to another bo_va via
AMDGPU_VA_OP_REPLACE, the surviving endpoints are cleared by
amdgpu_vm_clear_freed() and never re-populated, causing GPU page faults
when accessed (for instance on usermode queues synchronizing via
vm_timeline_syncobj_out).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-amdgpu_partial_mappings_fix-v1-1-02c5dfa20889@igalia.com?part=1