On 7/1/26 13:33, Natalie Vock wrote:
> We claimed vm_bo->moved was protected by the BO being reserved, but then
> accessed it in a bunch of places without a reservation to the BO anyway.
Can you point out where? That clearly doesn't sound correct to me.
We have some questionable uses for userptrs, but those should be irrelevant.
>
> It's not sensible to protect this by BO reservation in any case - it's a
> property relating to VM state, just like vm_status. Let's protect this
> by vm->status_lock as well. We usually grab the lock at some point when
> we access the field anyway, so it fits well with the current usage of
> the field.
That won't work. You can't hold the status lock while the moved flag can't be
modified.
> We also need to remove some unprotected accesses of the field when
> removing a mapping. Checking for the field there was a microoptimization
> anyway.
>
> Lastly, and most critically, we also need to handle buffer
> moves/invalidations racing with amdgpu_vm_bo_update. Otherwise we might
> accidentally undo the invalidation without the PTs actually being
> properly updated.
That's not correct as far as I can see the moves/invalidations can perfectly
happen in paralell and are handled before the next CS.
Regards,
Christian.
>
> Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)")
> Signed-off-by: Natalie Vock <[email protected]>
> ---
> FWIW, I'm not 100% positive on whether the Fixes tag is correct - I'm
> fairly certain the VM update <-> invalidation race existed ever since
> the driver's inception, but it may have been hidden in the initial
> revision. I suspect backporting it all the way to wherever the race
> started manifesting is roughly similarly painful, though.
>
> Also, this patchset is based on my previous one to rename the "moved" VM
> state to "needs_update". I think it got reviewed and should've been
> picked up, but I'm not sure I see it in amd-staging-drm-next?
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 108 +++++++++++++++++--------
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 3 +-
> 2 files changed, 75 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 32719f31b6c9e..3451dca7de194 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -214,11 +214,12 @@ static void amdgpu_vm_bo_evicted(struct
> amdgpu_vm_bo_base *vm_bo)
> * amdgpu_vm_bo_needs_update - vm_bo needs pagetable update
> *
> * @vm_bo: vm_bo which is out of date
> + * @moved: whether the vm_bo was moved
> *
> * State for vm_bo objects meaning the underlying BO had mapping changes
> (move, PRT bind/unbind)
> * but the new location is not yet reflected in the page tables.
> */
> -static void amdgpu_vm_bo_needs_update(struct amdgpu_vm_bo_base *vm_bo)
> +static void amdgpu_vm_bo_needs_update(struct amdgpu_vm_bo_base *vm_bo, bool
> moved)
> {
> struct amdgpu_vm_bo_status *lists;
> struct amdgpu_bo *bo = vm_bo->bo;
> @@ -232,11 +233,28 @@ static void amdgpu_vm_bo_needs_update(struct
> amdgpu_vm_bo_base *vm_bo)
> vm_bo->moved = false;
> list_move(&vm_bo->vm_status, &lists->idle);
> } else {
> + if (moved)
> + vm_bo->moved = true;
> list_move(&vm_bo->vm_status, &lists->needs_update);
> }
> amdgpu_vm_bo_unlock_lists(vm_bo);
> }
>
> +/**
> + * amdgpu_vm_bo_idle_locked - vm_bo is idle, already-locked version
> + *
> + * @vm_bo: vm_bo which is now idle
> + *
> + * State for vm_bo objects meaning we are done with the state machine and no
> + * further action is necessary. Needs to supply a locked status list.
> + */
> +static void amdgpu_vm_bo_idle_locked(struct amdgpu_vm_bo_base *vm_bo,
> + struct amdgpu_vm_bo_status *lists)
> +{
> + if (!amdgpu_vm_is_bo_always_valid(vm_bo->vm, vm_bo->bo))
> + vm_bo->moved = false;
> + list_move(&vm_bo->vm_status, &lists->idle);
> +}
> /**
> * amdgpu_vm_bo_idle - vm_bo is idle
> *
> @@ -250,9 +268,7 @@ static void amdgpu_vm_bo_idle(struct amdgpu_vm_bo_base
> *vm_bo)
> struct amdgpu_vm_bo_status *lists;
>
> lists = amdgpu_vm_bo_lock_lists(vm_bo);
> - if (!amdgpu_vm_is_bo_always_valid(vm_bo->vm, vm_bo->bo))
> - vm_bo->moved = false;
> - list_move(&vm_bo->vm_status, &lists->idle);
> + amdgpu_vm_bo_idle_locked(vm_bo, lists);
> amdgpu_vm_bo_unlock_lists(vm_bo);
> }
>
> @@ -273,9 +289,9 @@ static void amdgpu_vm_bo_reset_state_machine(struct
> amdgpu_vm *vm)
> */
> amdgpu_vm_assert_locked(vm);
> list_for_each_entry_safe(vm_bo, tmp, &vm->kernel.idle, vm_status)
> - amdgpu_vm_bo_needs_update(vm_bo);
> + amdgpu_vm_bo_needs_update(vm_bo, false);
> list_for_each_entry_safe(vm_bo, tmp, &vm->always_valid.idle, vm_status)
> - amdgpu_vm_bo_needs_update(vm_bo);
> + amdgpu_vm_bo_needs_update(vm_bo, false);
>
> spin_lock(&vm->individual_lock);
> list_for_each_entry_safe(vm_bo, tmp, &vm->individual.idle, vm_status) {
> @@ -435,7 +451,7 @@ void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base
> *base,
> */
> if (bo->preferred_domains &
> amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type))
> - amdgpu_vm_bo_needs_update(base);
> + amdgpu_vm_bo_needs_update(base, false);
> else
> amdgpu_vm_bo_evicted(base);
> }
> @@ -607,8 +623,7 @@ int amdgpu_vm_validate(struct amdgpu_device *adev, struct
> amdgpu_vm *vm,
> return r;
>
> vm->update_funcs->map_table(to_amdgpu_bo_vm(bo_base->bo));
> - bo_base->moved = true;
> - amdgpu_vm_bo_needs_update(bo_base);
> + amdgpu_vm_bo_needs_update(bo_base, true);
> }
>
> /*
> @@ -625,8 +640,7 @@ int amdgpu_vm_validate(struct amdgpu_device *adev, struct
> amdgpu_vm *vm,
> if (r)
> return r;
>
> - bo_base->moved = true;
> - amdgpu_vm_bo_needs_update(bo_base);
> + amdgpu_vm_bo_needs_update(bo_base, true);
> }
>
> if (!ticket)
> @@ -646,8 +660,7 @@ int amdgpu_vm_validate(struct amdgpu_device *adev, struct
> amdgpu_vm *vm,
> if (r)
> return r;
>
> - bo_base->moved = true;
> - amdgpu_vm_bo_needs_update(bo_base);
> + amdgpu_vm_bo_needs_update(bo_base, true);
>
> /* It's a bit inefficient to always jump back to the start, but
> * we would need to re-structure the KFD for properly fixing
> @@ -1266,16 +1279,38 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev,
> struct amdgpu_bo_va *bo_va,
> struct amdgpu_bo *bo = bo_va->base.bo;
> struct amdgpu_vm *vm = bo_va->base.vm;
> struct amdgpu_bo_va_mapping *mapping;
> + struct amdgpu_vm_bo_status *lists;
> struct dma_fence **last_update;
> dma_addr_t *pages_addr = NULL;
> struct ttm_resource *mem;
> struct amdgpu_sync sync;
> bool flush_tlb = clear;
> + bool uncached, moved;
> uint64_t vram_base;
> uint64_t flags;
> - bool uncached;
> int r;
>
> + /* Everything we access in here is protected by the VM PD lock. */
> + amdgpu_vm_assert_locked(vm);
> +
> + lists = amdgpu_vm_bo_lock_lists(&bo_va->base);
> + /*
> + * We can't hold the spinlock for the entire VM update, so temporarily
> remove
> + * the BO from the state machine entirely. This does not prevent all
> types of
> + * races: We might not hold the BO's resv here, so TTM is free to move
> the buffer
> + * and thereby invalidate it. However, it allows us to detect if we
> raced with
> + * something that invalidated the BO again and handle that
> appropriately below.
> + */
> + list_del_init(&bo_va->base.vm_status);
> +
> + /*
> + * The moved flag is also protected by the status lock. It's fine if
> some buffer
> + * update changes bo_va->base.moved while we're updating the PTs after
> unlocking
> + * the status lock, since that also invalidates the BO's VM status
> again.
> + */
> + moved = bo_va->base.moved;
> + amdgpu_vm_bo_unlock_lists(&bo_va->base);
> +
> amdgpu_sync_create(&sync);
> if (clear) {
> mem = NULL;
> @@ -1343,7 +1378,7 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev,
> struct amdgpu_bo_va *bo_va,
> else
> last_update = &bo_va->last_pt_update;
>
> - if (!clear && bo_va->base.moved) {
> + if (!clear && moved) {
> flush_tlb = true;
> list_splice_init(&bo_va->valids, &bo_va->invalids);
>
> @@ -1389,20 +1424,38 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev,
> struct amdgpu_bo_va *bo_va,
> else
> amdgpu_vm_bo_idle(&bo_va->base);
> } else {
> - amdgpu_vm_bo_idle(&bo_va->base);
> + lists = amdgpu_vm_bo_lock_lists(&bo_va->base);
> + /*
> + * Now that we're holding the lock again, check if the
> + * buffer got invalidated while we weren't looking.
> + * We initialized vm_status to an empty list head above,
> + * if that's still the case we can safely mark the BO as
> + * done.
> + *
> + * Note: We only need to do this for BOs that are not
> + * VM-always-valid, because we hold the VM's reservation
> + * which by definition reserves all VM-always-valid BOs.
> + */
> + if (list_empty(&bo_va->base.vm_status))
> + amdgpu_vm_bo_idle_locked(&bo_va->base, lists);
> + amdgpu_vm_bo_unlock_lists(&bo_va->base);
> }
>
> list_splice_init(&bo_va->invalids, &bo_va->valids);
> bo_va->cleared = clear;
> - bo_va->base.moved = false;
>
> if (trace_amdgpu_vm_bo_mapping_enabled()) {
> list_for_each_entry(mapping, &bo_va->valids, list)
> trace_amdgpu_vm_bo_mapping(mapping);
> }
>
> + amdgpu_sync_free(&sync);
> + return 0;
> +
> error_free:
> amdgpu_sync_free(&sync);
> + if (r)
> + amdgpu_vm_bo_needs_update(&bo_va->base, false);
> return r;
> }
>
> @@ -1779,7 +1832,6 @@ static void amdgpu_vm_bo_insert_map(struct
> amdgpu_device *adev,
> struct amdgpu_bo_va_mapping *mapping)
> {
> struct amdgpu_vm *vm = bo_va->base.vm;
> - struct amdgpu_bo *bo = bo_va->base.bo;
>
> mapping->bo_va = bo_va;
> list_add(&mapping->list, &bo_va->invalids);
> @@ -1788,8 +1840,7 @@ static void amdgpu_vm_bo_insert_map(struct
> amdgpu_device *adev,
> if (mapping->flags & AMDGPU_VM_PAGE_PRT)
> amdgpu_vm_prt_get(adev);
>
> - if (amdgpu_vm_is_bo_always_valid(vm, bo) && !bo_va->base.moved)
> - amdgpu_vm_bo_needs_update(&bo_va->base);
> + amdgpu_vm_bo_needs_update(&bo_va->base, false);
>
> trace_amdgpu_vm_bo_map(bo_va, mapping);
> }
> @@ -2090,30 +2141,22 @@ int amdgpu_vm_bo_clear_mappings(struct amdgpu_device
> *adev,
>
> /* Insert partial mapping before the range */
> if (!list_empty(&before->list)) {
> - struct amdgpu_bo *bo = before->bo_va->base.bo;
> -
> amdgpu_vm_it_insert(before, &vm->va);
> if (before->flags & AMDGPU_VM_PAGE_PRT)
> amdgpu_vm_prt_get(adev);
>
> - if (amdgpu_vm_is_bo_always_valid(vm, bo) &&
> - !before->bo_va->base.moved)
> - amdgpu_vm_bo_needs_update(&before->bo_va->base);
> + amdgpu_vm_bo_needs_update(&before->bo_va->base, false);
> } else {
> kfree(before);
> }
>
> /* Insert partial mapping after the range */
> if (!list_empty(&after->list)) {
> - struct amdgpu_bo *bo = after->bo_va->base.bo;
> -
> amdgpu_vm_it_insert(after, &vm->va);
> if (after->flags & AMDGPU_VM_PAGE_PRT)
> amdgpu_vm_prt_get(adev);
>
> - if (amdgpu_vm_is_bo_always_valid(vm, bo) &&
> - !after->bo_va->base.moved)
> - amdgpu_vm_bo_needs_update(&after->bo_va->base);
> + amdgpu_vm_bo_needs_update(&after->bo_va->base, false);
> } else {
> kfree(after);
> }
> @@ -2285,10 +2328,7 @@ void amdgpu_vm_bo_invalidate(struct amdgpu_bo *bo,
> bool evicted)
> continue;
> }
>
> - if (bo_base->moved)
> - continue;
> - bo_base->moved = true;
> - amdgpu_vm_bo_needs_update(bo_base);
> + amdgpu_vm_bo_needs_update(bo_base, true);
> }
> }
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index c1bd4d35831e0..0acd889568a38 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -212,8 +212,7 @@ struct amdgpu_vm_bo_base {
> * protected by vm BO being reserved */
> bool shared;
>
> - /* if the BO was moved and all mappings are invalid
> - * protected by the BO being reserved */
> + /* protected by the vm's status lock */
> bool moved;
> };
>
