[PATCH 9/9] drm/amdgpu: add graceful VM fault handling v2
Next step towards HMM support. For now just silence the retry fault and optionally redirect the request to the dummy page. v2: make sure the VM is not destroyed while we handle the fault. Signed-off-by: Christian König --- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 74 ++ drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 2 + drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 4 ++ 3 files changed, 80 insertions(+) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index 951608fc1925..410d89966a66 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -3142,3 +3142,77 @@ void amdgpu_vm_set_task_info(struct amdgpu_vm *vm) } } } + +/** + * amdgpu_vm_handle_fault - graceful handling of VM faults. + * @adev: amdgpu device pointer + * @pasid: PASID of the VM + * @addr: Address of the fault + * + * Try to gracefully handle a VM fault. Return true if the fault was handled and + * shouldn't be reported any more. + */ +bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, unsigned int pasid, + uint64_t addr) +{ + struct amdgpu_ring *ring = &adev->sdma.instance[0].page; + struct amdgpu_bo *root; + uint64_t value, flags; + struct amdgpu_vm *vm; + long r; + + if (!ring->sched.ready) + return false; + + spin_lock(&adev->vm_manager.pasid_lock); + vm = idr_find(&adev->vm_manager.pasid_idr, pasid); + if (vm) + root = amdgpu_bo_ref(vm->root.base.bo); + else + root = NULL; + spin_unlock(&adev->vm_manager.pasid_lock); + + if (!root) + return false; + + r = amdgpu_bo_reserve(root, true); + if (r) + goto error_unref; + + spin_lock(&adev->vm_manager.pasid_lock); + vm = idr_find(&adev->vm_manager.pasid_idr, pasid); + spin_unlock(&adev->vm_manager.pasid_lock); + + if (!vm || vm->root.base.bo != root) + goto error_unlock; + + addr /= AMDGPU_GPU_PAGE_SIZE; + flags = AMDGPU_PTE_VALID | AMDGPU_PTE_SNOOPED | + AMDGPU_PTE_SYSTEM; + + if (amdgpu_vm_fault_stop == AMDGPU_VM_FAULT_STOP_NEVER) { + /* Redirect the access to the dummy page */ + value = adev->dummy_page_addr; + flags |= AMDGPU_PTE_EXECUTABLE | AMDGPU_PTE_READABLE | + AMDGPU_PTE_WRITEABLE; + } else { + value = 0; + } + + r = amdgpu_vm_bo_update_mapping(adev, vm, true, NULL, addr, addr + 1, + flags, value, NULL, NULL); + if (r) + goto error_unlock; + + r = amdgpu_vm_update_pdes(adev, vm, true); + +error_unlock: + amdgpu_bo_unreserve(root); + if (r < 0) + DRM_ERROR("Can't handle page fault (%ld)\n", r); + +error_unref: + amdgpu_bo_unref(&root); + + return false; +} diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h index 0a97dc839f3b..4dbbe1b6b413 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h @@ -413,6 +413,8 @@ void amdgpu_vm_check_compute_bug(struct amdgpu_device *adev); void amdgpu_vm_get_task_info(struct amdgpu_device *adev, unsigned int pasid, struct amdgpu_task_info *task_info); +bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, unsigned int pasid, + uint64_t addr); void amdgpu_vm_set_task_info(struct amdgpu_vm *vm); diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c index 9d15679df6e0..15a1ce51befa 100644 --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c @@ -353,6 +353,10 @@ static int gmc_v9_0_process_interrupt(struct amdgpu_device *adev, } /* If it's the first fault for this address, process it normally */ + if (retry_fault && !in_interrupt() && + amdgpu_vm_handle_fault(adev, entry->pasid, addr)) + return 1; /* This also prevents sending it to KFD */ + if (!amdgpu_sriov_vf(adev)) { /* * Issue a dummy read to wait for the status register to -- 2.17.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 9/9] drm/amdgpu: add graceful VM fault handling v2
This series looks nice and clear for me, two questions embedded below. Are we going to use dedicated sdma page queue for direct VM update path during a fault? Thanks, Philip On 2019-09-04 11:02 a.m., Christian König wrote: > Next step towards HMM support. For now just silence the retry fault and > optionally redirect the request to the dummy page. > > v2: make sure the VM is not destroyed while we handle the fault. > > Signed-off-by: Christian König > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 74 ++ > drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 2 + > drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 4 ++ > 3 files changed, 80 insertions(+) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > index 951608fc1925..410d89966a66 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > @@ -3142,3 +3142,77 @@ void amdgpu_vm_set_task_info(struct amdgpu_vm *vm) > } > } > } > + > +/** > + * amdgpu_vm_handle_fault - graceful handling of VM faults. > + * @adev: amdgpu device pointer > + * @pasid: PASID of the VM > + * @addr: Address of the fault > + * > + * Try to gracefully handle a VM fault. Return true if the fault was handled > and > + * shouldn't be reported any more. > + */ > +bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, unsigned int pasid, > + uint64_t addr) > +{ > + struct amdgpu_ring *ring = &adev->sdma.instance[0].page; > + struct amdgpu_bo *root; > + uint64_t value, flags; > + struct amdgpu_vm *vm; > + long r; > + > + if (!ring->sched.ready) > + return false; > + > + spin_lock(&adev->vm_manager.pasid_lock); > + vm = idr_find(&adev->vm_manager.pasid_idr, pasid); > + if (vm) > + root = amdgpu_bo_ref(vm->root.base.bo); > + else > + root = NULL; > + spin_unlock(&adev->vm_manager.pasid_lock); > + > + if (!root) > + return false; > + > + r = amdgpu_bo_reserve(root, true); > + if (r) > + goto error_unref; > + > + spin_lock(&adev->vm_manager.pasid_lock); > + vm = idr_find(&adev->vm_manager.pasid_idr, pasid); > + spin_unlock(&adev->vm_manager.pasid_lock); > + Here get vm from pasid second time, and check if PD bo is changed, is this to handle vm fault race with vm destory? > + if (!vm || vm->root.base.bo != root) > + goto error_unlock; > + > + addr /= AMDGPU_GPU_PAGE_SIZE; > + flags = AMDGPU_PTE_VALID | AMDGPU_PTE_SNOOPED | > + AMDGPU_PTE_SYSTEM; > + > + if (amdgpu_vm_fault_stop == AMDGPU_VM_FAULT_STOP_NEVER) { > + /* Redirect the access to the dummy page */ > + value = adev->dummy_page_addr; > + flags |= AMDGPU_PTE_EXECUTABLE | AMDGPU_PTE_READABLE | > + AMDGPU_PTE_WRITEABLE; > + } else { > + value = 0; > + } > + > + r = amdgpu_vm_bo_update_mapping(adev, vm, true, NULL, addr, addr + 1, > + flags, value, NULL, NULL); > + if (r) > + goto error_unlock; > + After fault address redirect to dummy page, will the fault recover and retry continue to execute? Is this dangerous to update PTE to use system memory address 0? > + r = amdgpu_vm_update_pdes(adev, vm, true); > + > +error_unlock: > + amdgpu_bo_unreserve(root); > + if (r < 0) > + DRM_ERROR("Can't handle page fault (%ld)\n", r); > + > +error_unref: > + amdgpu_bo_unref(&root); > + > + return false; > +} > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h > index 0a97dc839f3b..4dbbe1b6b413 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h > @@ -413,6 +413,8 @@ void amdgpu_vm_check_compute_bug(struct amdgpu_device > *adev); > > void amdgpu_vm_get_task_info(struct amdgpu_device *adev, unsigned int pasid, >struct amdgpu_task_info *task_info); > +bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, unsigned int pasid, > + uint64_t addr); > > void amdgpu_vm_set_task_info(struct amdgpu_vm *vm); > > diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c > b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c > index 9d15679df6e0..15a1ce51befa 100644 > --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c > @@ -353,6 +353,10 @@ static int gmc_v9_0_process_interrupt(struct > amdgpu_device *adev, > } > > /* If it's the first fault for this address, process it normally */ > + if (retry_fault && !in_interrupt() && > + amdgpu_vm_handle_fault(adev, entry->pasid, addr)) > + return 1; /* This also prevents sending it to KFD */ > + > if (!amdgpu_sriov_vf(adev)) { > /* >* Issue a dummy read to wait for the status regis
Re: [PATCH 9/9] drm/amdgpu: add graceful VM fault handling v2
On 2019-09-04 11:02 a.m., Christian König wrote: > Next step towards HMM support. For now just silence the retry fault and > optionally redirect the request to the dummy page. > > v2: make sure the VM is not destroyed while we handle the fault. > > Signed-off-by: Christian König > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 74 ++ > drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 2 + > drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 4 ++ > 3 files changed, 80 insertions(+) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > index 951608fc1925..410d89966a66 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > @@ -3142,3 +3142,77 @@ void amdgpu_vm_set_task_info(struct amdgpu_vm *vm) > } > } > } > + > +/** > + * amdgpu_vm_handle_fault - graceful handling of VM faults. > + * @adev: amdgpu device pointer > + * @pasid: PASID of the VM > + * @addr: Address of the fault > + * > + * Try to gracefully handle a VM fault. Return true if the fault was handled > and > + * shouldn't be reported any more. > + */ > +bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, unsigned int pasid, > + uint64_t addr) > +{ > + struct amdgpu_ring *ring = &adev->sdma.instance[0].page; > + struct amdgpu_bo *root; > + uint64_t value, flags; > + struct amdgpu_vm *vm; > + long r; > + > + if (!ring->sched.ready) > + return false; > + > + spin_lock(&adev->vm_manager.pasid_lock); > + vm = idr_find(&adev->vm_manager.pasid_idr, pasid); > + if (vm) > + root = amdgpu_bo_ref(vm->root.base.bo); > + else > + root = NULL; > + spin_unlock(&adev->vm_manager.pasid_lock); > + > + if (!root) > + return false; > + > + r = amdgpu_bo_reserve(root, true); > + if (r) > + goto error_unref; > + > + spin_lock(&adev->vm_manager.pasid_lock); > + vm = idr_find(&adev->vm_manager.pasid_idr, pasid); > + spin_unlock(&adev->vm_manager.pasid_lock); I think this deserves a comment. If I understand it correctly, you're looking up the vm twice so that you have the VM root reservation to protect against user-after-free. Otherwise the vm pointer is only valid as long as you're holding the spin-lock. > + > + if (!vm || vm->root.base.bo != root) The check of vm->root.base.bo should probably still be under the spin_lock. Because you're not sure yet it's the right VM, you can't rely on the reservation here to prevent use-after-free. > + goto error_unlock; > + > + addr /= AMDGPU_GPU_PAGE_SIZE; > + flags = AMDGPU_PTE_VALID | AMDGPU_PTE_SNOOPED | > + AMDGPU_PTE_SYSTEM; > + > + if (amdgpu_vm_fault_stop == AMDGPU_VM_FAULT_STOP_NEVER) { > + /* Redirect the access to the dummy page */ > + value = adev->dummy_page_addr; > + flags |= AMDGPU_PTE_EXECUTABLE | AMDGPU_PTE_READABLE | > + AMDGPU_PTE_WRITEABLE; > + } else { > + value = 0; > + } > + > + r = amdgpu_vm_bo_update_mapping(adev, vm, true, NULL, addr, addr + 1, > + flags, value, NULL, NULL); > + if (r) > + goto error_unlock; > + > + r = amdgpu_vm_update_pdes(adev, vm, true); > + > +error_unlock: > + amdgpu_bo_unreserve(root); > + if (r < 0) > + DRM_ERROR("Can't handle page fault (%ld)\n", r); > + > +error_unref: > + amdgpu_bo_unref(&root); > + > + return false; > +} > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h > index 0a97dc839f3b..4dbbe1b6b413 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h > @@ -413,6 +413,8 @@ void amdgpu_vm_check_compute_bug(struct amdgpu_device > *adev); > > void amdgpu_vm_get_task_info(struct amdgpu_device *adev, unsigned int pasid, >struct amdgpu_task_info *task_info); > +bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, unsigned int pasid, > + uint64_t addr); > > void amdgpu_vm_set_task_info(struct amdgpu_vm *vm); > > diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c > b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c > index 9d15679df6e0..15a1ce51befa 100644 > --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c > @@ -353,6 +353,10 @@ static int gmc_v9_0_process_interrupt(struct > amdgpu_device *adev, > } > > /* If it's the first fault for this address, process it normally */ > + if (retry_fault && !in_interrupt() && > + amdgpu_vm_handle_fault(adev, entry->pasid, addr)) > + return 1; /* This also prevents sending it to KFD */ The !in_interrupt() is meant to only do this on the rerouted interrupt ring that's handled by a worker function? Looks like amdgpu_vm_han
Re: [PATCH 9/9] drm/amdgpu: add graceful VM fault handling v2
Am 04.09.19 um 22:12 schrieb Yang, Philip: This series looks nice and clear for me, two questions embedded below. Are we going to use dedicated sdma page queue for direct VM update path during a fault? Thanks, Philip On 2019-09-04 11:02 a.m., Christian König wrote: Next step towards HMM support. For now just silence the retry fault and optionally redirect the request to the dummy page. v2: make sure the VM is not destroyed while we handle the fault. Signed-off-by: Christian König --- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 74 ++ drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 2 + drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 4 ++ 3 files changed, 80 insertions(+) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index 951608fc1925..410d89966a66 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -3142,3 +3142,77 @@ void amdgpu_vm_set_task_info(struct amdgpu_vm *vm) } } } + +/** + * amdgpu_vm_handle_fault - graceful handling of VM faults. + * @adev: amdgpu device pointer + * @pasid: PASID of the VM + * @addr: Address of the fault + * + * Try to gracefully handle a VM fault. Return true if the fault was handled and + * shouldn't be reported any more. + */ +bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, unsigned int pasid, + uint64_t addr) +{ + struct amdgpu_ring *ring = &adev->sdma.instance[0].page; + struct amdgpu_bo *root; + uint64_t value, flags; + struct amdgpu_vm *vm; + long r; + + if (!ring->sched.ready) + return false; + + spin_lock(&adev->vm_manager.pasid_lock); + vm = idr_find(&adev->vm_manager.pasid_idr, pasid); + if (vm) + root = amdgpu_bo_ref(vm->root.base.bo); + else + root = NULL; + spin_unlock(&adev->vm_manager.pasid_lock); + + if (!root) + return false; + + r = amdgpu_bo_reserve(root, true); + if (r) + goto error_unref; + + spin_lock(&adev->vm_manager.pasid_lock); + vm = idr_find(&adev->vm_manager.pasid_idr, pasid); + spin_unlock(&adev->vm_manager.pasid_lock); + Here get vm from pasid second time, and check if PD bo is changed, is this to handle vm fault race with vm destory? Yes, exactly. + if (!vm || vm->root.base.bo != root) + goto error_unlock; + + addr /= AMDGPU_GPU_PAGE_SIZE; + flags = AMDGPU_PTE_VALID | AMDGPU_PTE_SNOOPED | + AMDGPU_PTE_SYSTEM; + + if (amdgpu_vm_fault_stop == AMDGPU_VM_FAULT_STOP_NEVER) { + /* Redirect the access to the dummy page */ + value = adev->dummy_page_addr; + flags |= AMDGPU_PTE_EXECUTABLE | AMDGPU_PTE_READABLE | + AMDGPU_PTE_WRITEABLE; + } else { + value = 0; + } + + r = amdgpu_vm_bo_update_mapping(adev, vm, true, NULL, addr, addr + 1, + flags, value, NULL, NULL); + if (r) + goto error_unlock; + After fault address redirect to dummy page, will the fault recover and retry continue to execute? Yes, the read/write operation will just retry and use the value from the dummy page instead. Is this dangerous to update PTE to use system memory address 0? What are you talking about? The dummy page is a page allocate by TTM where we redirect faulty accesses to. Regards, Christian. + r = amdgpu_vm_update_pdes(adev, vm, true); + +error_unlock: + amdgpu_bo_unreserve(root); + if (r < 0) + DRM_ERROR("Can't handle page fault (%ld)\n", r); + +error_unref: + amdgpu_bo_unref(&root); + + return false; +} diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h index 0a97dc839f3b..4dbbe1b6b413 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h @@ -413,6 +413,8 @@ void amdgpu_vm_check_compute_bug(struct amdgpu_device *adev); void amdgpu_vm_get_task_info(struct amdgpu_device *adev, unsigned int pasid, struct amdgpu_task_info *task_info); +bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, unsigned int pasid, + uint64_t addr); void amdgpu_vm_set_task_info(struct amdgpu_vm *vm); diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c index 9d15679df6e0..15a1ce51befa 100644 --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c @@ -353,6 +353,10 @@ static int gmc_v9_0_process_interrupt(struct amdgpu_device *adev, } /* If it's the first fault for this address, process it normally */ + if (retry_fault && !in_interrupt() && + amdgpu_vm_handle_fault(adev, entry->pasid, addr)) + return 1; /*
Re: [PATCH 9/9] drm/amdgpu: add graceful VM fault handling v2
Am 05.09.19 um 00:47 schrieb Kuehling, Felix: On 2019-09-04 11:02 a.m., Christian König wrote: Next step towards HMM support. For now just silence the retry fault and optionally redirect the request to the dummy page. v2: make sure the VM is not destroyed while we handle the fault. Signed-off-by: Christian König --- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 74 ++ drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 2 + drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 4 ++ 3 files changed, 80 insertions(+) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index 951608fc1925..410d89966a66 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -3142,3 +3142,77 @@ void amdgpu_vm_set_task_info(struct amdgpu_vm *vm) } } } + +/** + * amdgpu_vm_handle_fault - graceful handling of VM faults. + * @adev: amdgpu device pointer + * @pasid: PASID of the VM + * @addr: Address of the fault + * + * Try to gracefully handle a VM fault. Return true if the fault was handled and + * shouldn't be reported any more. + */ +bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, unsigned int pasid, + uint64_t addr) +{ + struct amdgpu_ring *ring = &adev->sdma.instance[0].page; + struct amdgpu_bo *root; + uint64_t value, flags; + struct amdgpu_vm *vm; + long r; + + if (!ring->sched.ready) + return false; + + spin_lock(&adev->vm_manager.pasid_lock); + vm = idr_find(&adev->vm_manager.pasid_idr, pasid); + if (vm) + root = amdgpu_bo_ref(vm->root.base.bo); + else + root = NULL; + spin_unlock(&adev->vm_manager.pasid_lock); + + if (!root) + return false; + + r = amdgpu_bo_reserve(root, true); + if (r) + goto error_unref; + + spin_lock(&adev->vm_manager.pasid_lock); + vm = idr_find(&adev->vm_manager.pasid_idr, pasid); + spin_unlock(&adev->vm_manager.pasid_lock); I think this deserves a comment. If I understand it correctly, you're looking up the vm twice so that you have the VM root reservation to protect against user-after-free. Otherwise the vm pointer is only valid as long as you're holding the spin-lock. + + if (!vm || vm->root.base.bo != root) The check of vm->root.base.bo should probably still be under the spin_lock. Because you're not sure yet it's the right VM, you can't rely on the reservation here to prevent use-after-free. Good point, going to fix that. + goto error_unlock; + + addr /= AMDGPU_GPU_PAGE_SIZE; + flags = AMDGPU_PTE_VALID | AMDGPU_PTE_SNOOPED | + AMDGPU_PTE_SYSTEM; + + if (amdgpu_vm_fault_stop == AMDGPU_VM_FAULT_STOP_NEVER) { + /* Redirect the access to the dummy page */ + value = adev->dummy_page_addr; + flags |= AMDGPU_PTE_EXECUTABLE | AMDGPU_PTE_READABLE | + AMDGPU_PTE_WRITEABLE; + } else { + value = 0; + } + + r = amdgpu_vm_bo_update_mapping(adev, vm, true, NULL, addr, addr + 1, + flags, value, NULL, NULL); + if (r) + goto error_unlock; + + r = amdgpu_vm_update_pdes(adev, vm, true); + +error_unlock: + amdgpu_bo_unreserve(root); + if (r < 0) + DRM_ERROR("Can't handle page fault (%ld)\n", r); + +error_unref: + amdgpu_bo_unref(&root); + + return false; +} diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h index 0a97dc839f3b..4dbbe1b6b413 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h @@ -413,6 +413,8 @@ void amdgpu_vm_check_compute_bug(struct amdgpu_device *adev); void amdgpu_vm_get_task_info(struct amdgpu_device *adev, unsigned int pasid, struct amdgpu_task_info *task_info); +bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, unsigned int pasid, + uint64_t addr); void amdgpu_vm_set_task_info(struct amdgpu_vm *vm); diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c index 9d15679df6e0..15a1ce51befa 100644 --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c @@ -353,6 +353,10 @@ static int gmc_v9_0_process_interrupt(struct amdgpu_device *adev, } /* If it's the first fault for this address, process it normally */ + if (retry_fault && !in_interrupt() && + amdgpu_vm_handle_fault(adev, entry->pasid, addr)) + return 1; /* This also prevents sending it to KFD */ The !in_interrupt() is meant to only do this on the rerouted interrupt ring that's handled by a worker function? Yes, exactly. But I plan to add a workaround where the CPU redirects
Re: [PATCH 9/9] drm/amdgpu: add graceful VM fault handling v2
On 2019-09-09 8:03 a.m., Christian König wrote: > Am 04.09.19 um 22:12 schrieb Yang, Philip: >> This series looks nice and clear for me, two questions embedded below. >> >> Are we going to use dedicated sdma page queue for direct VM update path >> during a fault? >> >> Thanks, >> Philip >> >> On 2019-09-04 11:02 a.m., Christian König wrote: >>> Next step towards HMM support. For now just silence the retry fault and >>> optionally redirect the request to the dummy page. >>> >>> v2: make sure the VM is not destroyed while we handle the fault. >>> >>> Signed-off-by: Christian König >>> --- >>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 74 >>> ++ >>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 2 + >>> drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 4 ++ >>> 3 files changed, 80 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >>> index 951608fc1925..410d89966a66 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >>> @@ -3142,3 +3142,77 @@ void amdgpu_vm_set_task_info(struct amdgpu_vm >>> *vm) >>> } >>> } >>> } >>> + >>> +/** >>> + * amdgpu_vm_handle_fault - graceful handling of VM faults. >>> + * @adev: amdgpu device pointer >>> + * @pasid: PASID of the VM >>> + * @addr: Address of the fault >>> + * >>> + * Try to gracefully handle a VM fault. Return true if the fault was >>> handled and >>> + * shouldn't be reported any more. >>> + */ >>> +bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, unsigned int >>> pasid, >>> + uint64_t addr) >>> +{ >>> + struct amdgpu_ring *ring = &adev->sdma.instance[0].page; >>> + struct amdgpu_bo *root; >>> + uint64_t value, flags; >>> + struct amdgpu_vm *vm; >>> + long r; >>> + >>> + if (!ring->sched.ready) >>> + return false; >>> + >>> + spin_lock(&adev->vm_manager.pasid_lock); >>> + vm = idr_find(&adev->vm_manager.pasid_idr, pasid); >>> + if (vm) >>> + root = amdgpu_bo_ref(vm->root.base.bo); >>> + else >>> + root = NULL; >>> + spin_unlock(&adev->vm_manager.pasid_lock); >>> + >>> + if (!root) >>> + return false; >>> + >>> + r = amdgpu_bo_reserve(root, true); >>> + if (r) >>> + goto error_unref; >>> + >>> + spin_lock(&adev->vm_manager.pasid_lock); >>> + vm = idr_find(&adev->vm_manager.pasid_idr, pasid); >>> + spin_unlock(&adev->vm_manager.pasid_lock); >>> + >> Here get vm from pasid second time, and check if PD bo is changed, is >> this to handle vm fault race with vm destory? > > Yes, exactly. > >> >>> + if (!vm || vm->root.base.bo != root) >>> + goto error_unlock; >>> + >>> + addr /= AMDGPU_GPU_PAGE_SIZE; >>> + flags = AMDGPU_PTE_VALID | AMDGPU_PTE_SNOOPED | >>> + AMDGPU_PTE_SYSTEM; >>> + >>> + if (amdgpu_vm_fault_stop == AMDGPU_VM_FAULT_STOP_NEVER) { >>> + /* Redirect the access to the dummy page */ >>> + value = adev->dummy_page_addr; >>> + flags |= AMDGPU_PTE_EXECUTABLE | AMDGPU_PTE_READABLE | >>> + AMDGPU_PTE_WRITEABLE; >>> + } else { >>> + value = 0; >>> + } >>> + >>> + r = amdgpu_vm_bo_update_mapping(adev, vm, true, NULL, addr, addr >>> + 1, >>> + flags, value, NULL, NULL); >>> + if (r) >>> + goto error_unlock; >>> + >> After fault address redirect to dummy page, will the fault recover and >> retry continue to execute? > > Yes, the read/write operation will just retry and use the value from the > dummy page instead. > >> Is this dangerous to update PTE to use system >> memory address 0? > > What are you talking about? The dummy page is a page allocate by TTM > where we redirect faulty accesses to. > For amdgpu_vm_fault_stop equals to AMDGPU_VM_FAULT_STOP_FIRST/ALWAYS case, value is 0, this will redirect to system memory 0. Maybe redirect is only needed for AMDGPU_VM_FAULT_STOP_NEVER? Regards, Philip > Regards, > Christian. > >> >>> + r = amdgpu_vm_update_pdes(adev, vm, true); >>> + >>> +error_unlock: >>> + amdgpu_bo_unreserve(root); >>> + if (r < 0) >>> + DRM_ERROR("Can't handle page fault (%ld)\n", r); >>> + >>> +error_unref: >>> + amdgpu_bo_unref(&root); >>> + >>> + return false; >>> +} >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h >>> index 0a97dc839f3b..4dbbe1b6b413 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h >>> @@ -413,6 +413,8 @@ void amdgpu_vm_check_compute_bug(struct >>> amdgpu_device *adev); >>> void amdgpu_vm_get_task_info(struct amdgpu_device *adev, unsigned >>> int pasid, >>> struct amdgpu_task_info *task_info); >>> +bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, unsigned int >>> pasid, >>> + uint64_t addr); >>> void amdgpu_vm_set_task_info(struct amdgpu_vm *vm); >>> diff --git
RE: [PATCH 9/9] drm/amdgpu: add graceful VM fault handling v2
Is looking up vm twice necessary? I think we are in interrupt context, is it possible that the user space application can be switched in between? My understanding is, if user space application is can't kick in during interrupt handling, application shouldn't have chance to exit (then their vm being destroyed). Regards, Oak -Original Message- From: amd-gfx On Behalf Of Christian König Sent: Monday, September 9, 2019 8:08 AM To: Kuehling, Felix ; amd-gfx@lists.freedesktop.org Subject: Re: [PATCH 9/9] drm/amdgpu: add graceful VM fault handling v2 Am 05.09.19 um 00:47 schrieb Kuehling, Felix: > On 2019-09-04 11:02 a.m., Christian König wrote: >> Next step towards HMM support. For now just silence the retry fault >> and optionally redirect the request to the dummy page. >> >> v2: make sure the VM is not destroyed while we handle the fault. >> >> Signed-off-by: Christian König >> --- >>drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 74 ++ >>drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 2 + >>drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 4 ++ >>3 files changed, 80 insertions(+) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >> index 951608fc1925..410d89966a66 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >> @@ -3142,3 +3142,77 @@ void amdgpu_vm_set_task_info(struct amdgpu_vm *vm) >> } >> } >>} >> + >> +/** >> + * amdgpu_vm_handle_fault - graceful handling of VM faults. >> + * @adev: amdgpu device pointer >> + * @pasid: PASID of the VM >> + * @addr: Address of the fault >> + * >> + * Try to gracefully handle a VM fault. Return true if the fault was >> +handled and >> + * shouldn't be reported any more. >> + */ >> +bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, unsigned int pasid, >> +uint64_t addr) >> +{ >> +struct amdgpu_ring *ring = &adev->sdma.instance[0].page; >> +struct amdgpu_bo *root; >> +uint64_t value, flags; >> +struct amdgpu_vm *vm; >> +long r; >> + >> +if (!ring->sched.ready) >> +return false; >> + >> +spin_lock(&adev->vm_manager.pasid_lock); >> +vm = idr_find(&adev->vm_manager.pasid_idr, pasid); >> +if (vm) >> +root = amdgpu_bo_ref(vm->root.base.bo); >> +else >> +root = NULL; >> +spin_unlock(&adev->vm_manager.pasid_lock); >> + >> +if (!root) >> +return false; >> + >> +r = amdgpu_bo_reserve(root, true); >> +if (r) >> +goto error_unref; >> + >> +spin_lock(&adev->vm_manager.pasid_lock); >> +vm = idr_find(&adev->vm_manager.pasid_idr, pasid); >> +spin_unlock(&adev->vm_manager.pasid_lock); > I think this deserves a comment. If I understand it correctly, you're > looking up the vm twice so that you have the VM root reservation to > protect against user-after-free. Otherwise the vm pointer is only > valid as long as you're holding the spin-lock. > > >> + >> +if (!vm || vm->root.base.bo != root) > The check of vm->root.base.bo should probably still be under the > spin_lock. Because you're not sure yet it's the right VM, you can't > rely on the reservation here to prevent use-after-free. Good point, going to fix that. > > >> +goto error_unlock; >> + >> +addr /= AMDGPU_GPU_PAGE_SIZE; >> +flags = AMDGPU_PTE_VALID | AMDGPU_PTE_SNOOPED | >> +AMDGPU_PTE_SYSTEM; >> + >> +if (amdgpu_vm_fault_stop == AMDGPU_VM_FAULT_STOP_NEVER) { >> +/* Redirect the access to the dummy page */ >> +value = adev->dummy_page_addr; >> +flags |= AMDGPU_PTE_EXECUTABLE | AMDGPU_PTE_READABLE | >> +AMDGPU_PTE_WRITEABLE; >> +} else { >> +value = 0; >> +} >> + >> +r = amdgpu_vm_bo_update_mapping(adev, vm, true, NULL, addr, addr + 1, >> +flags, value, NULL, NULL); >> +if (r) >> +goto error_unlock; >> + >> +r = amdgpu_vm_update_pdes(adev, vm, true); >> + >> +error_unlock: >> +amdgpu_bo_unreserve(root); >> +if (r < 0) >> +DRM_ERROR("Can't handle page fault (%ld)\n", r); >> + >> +error_unref: >> +amdgpu_bo_u
Re: [PATCH 9/9] drm/amdgpu: add graceful VM fault handling v2
Well first of all we are not in interrupt context here, this is handled by a work item or otherwise we couldn't do all the locking. But even in interrupt context another CPU can easily destroy the VM when we just handle a stale fault or the process was killed. So this extra double checking is strictly necessary. Regards, Christian. Am 09.09.19 um 16:08 schrieb Zeng, Oak: > Is looking up vm twice necessary? I think we are in interrupt context, is it > possible that the user space application can be switched in between? My > understanding is, if user space application is can't kick in during interrupt > handling, application shouldn't have chance to exit (then their vm being > destroyed). > > Regards, > Oak > > -Original Message- > From: amd-gfx On Behalf Of Christian > König > Sent: Monday, September 9, 2019 8:08 AM > To: Kuehling, Felix ; amd-gfx@lists.freedesktop.org > Subject: Re: [PATCH 9/9] drm/amdgpu: add graceful VM fault handling v2 > > Am 05.09.19 um 00:47 schrieb Kuehling, Felix: >> On 2019-09-04 11:02 a.m., Christian König wrote: >>> Next step towards HMM support. For now just silence the retry fault >>> and optionally redirect the request to the dummy page. >>> >>> v2: make sure the VM is not destroyed while we handle the fault. >>> >>> Signed-off-by: Christian König >>> --- >>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 74 ++ >>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 2 + >>> drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 4 ++ >>> 3 files changed, 80 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >>> index 951608fc1925..410d89966a66 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >>> @@ -3142,3 +3142,77 @@ void amdgpu_vm_set_task_info(struct amdgpu_vm *vm) >>> } >>> } >>> } >>> + >>> +/** >>> + * amdgpu_vm_handle_fault - graceful handling of VM faults. >>> + * @adev: amdgpu device pointer >>> + * @pasid: PASID of the VM >>> + * @addr: Address of the fault >>> + * >>> + * Try to gracefully handle a VM fault. Return true if the fault was >>> +handled and >>> + * shouldn't be reported any more. >>> + */ >>> +bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, unsigned int pasid, >>> + uint64_t addr) >>> +{ >>> + struct amdgpu_ring *ring = &adev->sdma.instance[0].page; >>> + struct amdgpu_bo *root; >>> + uint64_t value, flags; >>> + struct amdgpu_vm *vm; >>> + long r; >>> + >>> + if (!ring->sched.ready) >>> + return false; >>> + >>> + spin_lock(&adev->vm_manager.pasid_lock); >>> + vm = idr_find(&adev->vm_manager.pasid_idr, pasid); >>> + if (vm) >>> + root = amdgpu_bo_ref(vm->root.base.bo); >>> + else >>> + root = NULL; >>> + spin_unlock(&adev->vm_manager.pasid_lock); >>> + >>> + if (!root) >>> + return false; >>> + >>> + r = amdgpu_bo_reserve(root, true); >>> + if (r) >>> + goto error_unref; >>> + >>> + spin_lock(&adev->vm_manager.pasid_lock); >>> + vm = idr_find(&adev->vm_manager.pasid_idr, pasid); >>> + spin_unlock(&adev->vm_manager.pasid_lock); >> I think this deserves a comment. If I understand it correctly, you're >> looking up the vm twice so that you have the VM root reservation to >> protect against user-after-free. Otherwise the vm pointer is only >> valid as long as you're holding the spin-lock. >> >> >>> + >>> + if (!vm || vm->root.base.bo != root) >> The check of vm->root.base.bo should probably still be under the >> spin_lock. Because you're not sure yet it's the right VM, you can't >> rely on the reservation here to prevent use-after-free. > Good point, going to fix that. > >> >>> + goto error_unlock; >>> + >>> + addr /= AMDGPU_GPU_PAGE_SIZE; >>> + flags = AMDGPU_PTE_VALID | AMDGPU_PTE_SNOOPED | >>> + AMDGPU_PTE_SYSTEM; >>> + >>> + if (amdgpu_vm_fault_stop == AMDGPU_VM_FAULT_STOP_NEVER) { >>> + /* Redirect the access to the dummy page */ >>> +
Re: [PATCH 9/9] drm/amdgpu: add graceful VM fault handling v2
Am 09.09.19 um 15:58 schrieb Yang, Philip: > > On 2019-09-09 8:03 a.m., Christian König wrote: >> Am 04.09.19 um 22:12 schrieb Yang, Philip: >>> This series looks nice and clear for me, two questions embedded below. >>> >>> Are we going to use dedicated sdma page queue for direct VM update path >>> during a fault? >>> >>> Thanks, >>> Philip >>> >>> On 2019-09-04 11:02 a.m., Christian König wrote: Next step towards HMM support. For now just silence the retry fault and optionally redirect the request to the dummy page. v2: make sure the VM is not destroyed while we handle the fault. Signed-off-by: Christian König --- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 74 ++ drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 2 + drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 4 ++ 3 files changed, 80 insertions(+) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index 951608fc1925..410d89966a66 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -3142,3 +3142,77 @@ void amdgpu_vm_set_task_info(struct amdgpu_vm *vm) } } } + +/** + * amdgpu_vm_handle_fault - graceful handling of VM faults. + * @adev: amdgpu device pointer + * @pasid: PASID of the VM + * @addr: Address of the fault + * + * Try to gracefully handle a VM fault. Return true if the fault was handled and + * shouldn't be reported any more. + */ +bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, unsigned int pasid, + uint64_t addr) +{ + struct amdgpu_ring *ring = &adev->sdma.instance[0].page; + struct amdgpu_bo *root; + uint64_t value, flags; + struct amdgpu_vm *vm; + long r; + + if (!ring->sched.ready) + return false; + + spin_lock(&adev->vm_manager.pasid_lock); + vm = idr_find(&adev->vm_manager.pasid_idr, pasid); + if (vm) + root = amdgpu_bo_ref(vm->root.base.bo); + else + root = NULL; + spin_unlock(&adev->vm_manager.pasid_lock); + + if (!root) + return false; + + r = amdgpu_bo_reserve(root, true); + if (r) + goto error_unref; + + spin_lock(&adev->vm_manager.pasid_lock); + vm = idr_find(&adev->vm_manager.pasid_idr, pasid); + spin_unlock(&adev->vm_manager.pasid_lock); + >>> Here get vm from pasid second time, and check if PD bo is changed, is >>> this to handle vm fault race with vm destory? >> Yes, exactly. >> + if (!vm || vm->root.base.bo != root) + goto error_unlock; + + addr /= AMDGPU_GPU_PAGE_SIZE; + flags = AMDGPU_PTE_VALID | AMDGPU_PTE_SNOOPED | + AMDGPU_PTE_SYSTEM; + + if (amdgpu_vm_fault_stop == AMDGPU_VM_FAULT_STOP_NEVER) { + /* Redirect the access to the dummy page */ + value = adev->dummy_page_addr; + flags |= AMDGPU_PTE_EXECUTABLE | AMDGPU_PTE_READABLE | + AMDGPU_PTE_WRITEABLE; + } else { + value = 0; + } + + r = amdgpu_vm_bo_update_mapping(adev, vm, true, NULL, addr, addr + 1, + flags, value, NULL, NULL); + if (r) + goto error_unlock; + >>> After fault address redirect to dummy page, will the fault recover and >>> retry continue to execute? >> Yes, the read/write operation will just retry and use the value from the >> dummy page instead. >> >>> Is this dangerous to update PTE to use system >>> memory address 0? >> What are you talking about? The dummy page is a page allocate by TTM >> where we redirect faulty accesses to. >> > For amdgpu_vm_fault_stop equals to AMDGPU_VM_FAULT_STOP_FIRST/ALWAYS > case, value is 0, this will redirect to system memory 0. Maybe redirect > is only needed for AMDGPU_VM_FAULT_STOP_NEVER? The value 0 doesn't redirect to system memory, it results in a silence retry when neither the R nor the W bit is set in a PTE. Regards, Christian. > > Regards, > Philip > >> Regards, >> Christian. >> + r = amdgpu_vm_update_pdes(adev, vm, true); + +error_unlock: + amdgpu_bo_unreserve(root); + if (r < 0) + DRM_ERROR("Can't handle page fault (%ld)\n", r); + +error_unref: + amdgpu_bo_unref(&root); + + return false; +} diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h index 0a97dc839f3b..4dbbe1b6b413 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h @@ -413,6 +413,8 @@ void amdgpu_vm_check_compute_bug(struct amdgpu_device *adev); void amdgpu_vm_get_
RE: [PATCH 9/9] drm/amdgpu: add graceful VM fault handling v2
Regards, Oak -Original Message- From: Koenig, Christian Sent: Monday, September 9, 2019 1:14 PM To: Zeng, Oak ; Kuehling, Felix ; amd-gfx@lists.freedesktop.org Subject: Re: [PATCH 9/9] drm/amdgpu: add graceful VM fault handling v2 > Well first of all we are not in interrupt context here, this is handled by a > work item or otherwise we couldn't do all the locking. This is called from amdgpu_irq_handler. I think this is interrupt context. This is also the reason why we use spin lock instead of other sleepable lock like a semaphore. > But even in interrupt context another CPU can easily destroy the VM when we > just handle a stale fault or the process was killed. Agree with this point. So this extra double checking is strictly necessary. Regards, Christian. Am 09.09.19 um 16:08 schrieb Zeng, Oak: > Is looking up vm twice necessary? I think we are in interrupt context, is it > possible that the user space application can be switched in between? My > understanding is, if user space application is can't kick in during interrupt > handling, application shouldn't have chance to exit (then their vm being > destroyed). > > Regards, > Oak > > -Original Message- > From: amd-gfx On Behalf Of > Christian König > Sent: Monday, September 9, 2019 8:08 AM > To: Kuehling, Felix ; > amd-gfx@lists.freedesktop.org > Subject: Re: [PATCH 9/9] drm/amdgpu: add graceful VM fault handling v2 > > Am 05.09.19 um 00:47 schrieb Kuehling, Felix: >> On 2019-09-04 11:02 a.m., Christian König wrote: >>> Next step towards HMM support. For now just silence the retry fault >>> and optionally redirect the request to the dummy page. >>> >>> v2: make sure the VM is not destroyed while we handle the fault. >>> >>> Signed-off-by: Christian König >>> --- >>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 74 ++ >>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 2 + >>> drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 4 ++ >>> 3 files changed, 80 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >>> index 951608fc1925..410d89966a66 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >>> @@ -3142,3 +3142,77 @@ void amdgpu_vm_set_task_info(struct amdgpu_vm *vm) >>> } >>> } >>> } >>> + >>> +/** >>> + * amdgpu_vm_handle_fault - graceful handling of VM faults. >>> + * @adev: amdgpu device pointer >>> + * @pasid: PASID of the VM >>> + * @addr: Address of the fault >>> + * >>> + * Try to gracefully handle a VM fault. Return true if the fault >>> +was handled and >>> + * shouldn't be reported any more. >>> + */ >>> +bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, unsigned int pasid, >>> + uint64_t addr) >>> +{ >>> + struct amdgpu_ring *ring = &adev->sdma.instance[0].page; >>> + struct amdgpu_bo *root; >>> + uint64_t value, flags; >>> + struct amdgpu_vm *vm; >>> + long r; >>> + >>> + if (!ring->sched.ready) >>> + return false; >>> + >>> + spin_lock(&adev->vm_manager.pasid_lock); >>> + vm = idr_find(&adev->vm_manager.pasid_idr, pasid); >>> + if (vm) >>> + root = amdgpu_bo_ref(vm->root.base.bo); >>> + else >>> + root = NULL; >>> + spin_unlock(&adev->vm_manager.pasid_lock); >>> + >>> + if (!root) >>> + return false; >>> + >>> + r = amdgpu_bo_reserve(root, true); >>> + if (r) >>> + goto error_unref; >>> + >>> + spin_lock(&adev->vm_manager.pasid_lock); >>> + vm = idr_find(&adev->vm_manager.pasid_idr, pasid); >>> + spin_unlock(&adev->vm_manager.pasid_lock); >> I think this deserves a comment. If I understand it correctly, you're >> looking up the vm twice so that you have the VM root reservation to >> protect against user-after-free. Otherwise the vm pointer is only >> valid as long as you're holding the spin-lock. >> >> >>> + >>> + if (!vm || vm->root.base.bo != root) >> The check of vm->root.base.bo should probably still be under the >> spin_lock. Because you're not sure yet it's the right VM, you can't >> rely on the reservatio
Re: [PATCH 9/9] drm/amdgpu: add graceful VM fault handling v2
Am 10.09.19 um 15:30 schrieb Zeng, Oak: > > Regards, > Oak > > -Original Message- > From: Koenig, Christian > Sent: Monday, September 9, 2019 1:14 PM > To: Zeng, Oak ; Kuehling, Felix ; > amd-gfx@lists.freedesktop.org > Subject: Re: [PATCH 9/9] drm/amdgpu: add graceful VM fault handling v2 > >> Well first of all we are not in interrupt context here, this is handled by a >> work item or otherwise we couldn't do all the locking. > This is called from amdgpu_irq_handler. I think this is interrupt context. No, that's incorrect. Only the primary interrupt ring is handled in interrupt context. Ring 1 and 2 are handled with a work item. > This is also the reason why we use spin lock instead of other sleepable > lock like a semaphore. Well we do have a sleep-able lock here to make it possible to update page tables and directories. That's also the reason why we protect the call with in !in_interrupt(). Regards, Christian. > >> But even in interrupt context another CPU can easily destroy the VM when we >> just handle a stale fault or the process was killed. > Agree with this point. > > So this extra double checking is strictly necessary. > > Regards, > Christian. > > Am 09.09.19 um 16:08 schrieb Zeng, Oak: >> Is looking up vm twice necessary? I think we are in interrupt context, is it >> possible that the user space application can be switched in between? My >> understanding is, if user space application is can't kick in during >> interrupt handling, application shouldn't have chance to exit (then their vm >> being destroyed). >> >> Regards, >> Oak >> >> -Original Message----- >> From: amd-gfx On Behalf Of >> Christian König >> Sent: Monday, September 9, 2019 8:08 AM >> To: Kuehling, Felix ; >> amd-gfx@lists.freedesktop.org >> Subject: Re: [PATCH 9/9] drm/amdgpu: add graceful VM fault handling v2 >> >> Am 05.09.19 um 00:47 schrieb Kuehling, Felix: >>> On 2019-09-04 11:02 a.m., Christian König wrote: >>>> Next step towards HMM support. For now just silence the retry fault >>>> and optionally redirect the request to the dummy page. >>>> >>>> v2: make sure the VM is not destroyed while we handle the fault. >>>> >>>> Signed-off-by: Christian König >>>> --- >>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 74 ++ >>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 2 + >>>> drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 4 ++ >>>> 3 files changed, 80 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >>>> index 951608fc1925..410d89966a66 100644 >>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >>>> @@ -3142,3 +3142,77 @@ void amdgpu_vm_set_task_info(struct amdgpu_vm *vm) >>>>} >>>>} >>>> } >>>> + >>>> +/** >>>> + * amdgpu_vm_handle_fault - graceful handling of VM faults. >>>> + * @adev: amdgpu device pointer >>>> + * @pasid: PASID of the VM >>>> + * @addr: Address of the fault >>>> + * >>>> + * Try to gracefully handle a VM fault. Return true if the fault >>>> +was handled and >>>> + * shouldn't be reported any more. >>>> + */ >>>> +bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, unsigned int >>>> pasid, >>>> + uint64_t addr) >>>> +{ >>>> + struct amdgpu_ring *ring = &adev->sdma.instance[0].page; >>>> + struct amdgpu_bo *root; >>>> + uint64_t value, flags; >>>> + struct amdgpu_vm *vm; >>>> + long r; >>>> + >>>> + if (!ring->sched.ready) >>>> + return false; >>>> + >>>> + spin_lock(&adev->vm_manager.pasid_lock); >>>> + vm = idr_find(&adev->vm_manager.pasid_idr, pasid); >>>> + if (vm) >>>> + root = amdgpu_bo_ref(vm->root.base.bo); >>>> + else >>>> + root = NULL; >>>> + spin_unlock(&adev->vm_manager.pasid_lock); >>>> + >>>> + if (!root) >>>> + return false; >>>> + >>>> + r = amdgpu_bo_reserve(root, true); >>>> + if (r) >>>> + goto er