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.
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.
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.
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;
};
--
2.54.0
