On 9/16/2026 8:14 PM, Christian König wrote:
Hi Srini, please keep Timur, Natalie and Samuel on CC. On 9/16/26 07:37, Srinivasan Shanmugam wrote:Render-node user queues had no mechanism to install a userspace second-level trap handler equivalent to what KFD provides for compute. On dGPUs the CWSR TMA buffer is placed in VRAM and accessed via MMIO (PCI BAR). Raw CPU pointer dereferences are unsafe on platforms that lack full load/store support for MMIO. Replace void *tma_cpu_addr with struct iosys_map tma_map and use iosys_map_wr() for all TMA writes. Writing TBA and TMA as two separate stores creates a race where a trapping wave can observe a mismatched pair. For user queues, evict all queues and flush the GPU TLB before writing, then restore queues after. For kernel queues, wait for the VM to be idle via amdgpu_vm_wait_idle() before eviction and the descriptor write. Evict user queues in amdgpu_trap_free() at file close. Without eviction a freed TMA BO can be accessed via a stale TBA, causing a GPU page fault. Export amdgpu_userq_restore_all() for immediate queue restore after SET/CLEAR without waiting for the eviction fence. Add VM_ALWAYS_VALID validation in amdgpu_trap_validate_user_addr(). SET_L2_TRAP returns -EINVAL if TBA or TMA BO does not have this flag.Works for me, Timur/Natalie/Samuel any objections to that?The old TRAP ioctl and its internal handler are removed. The new SET/CLEAR path is exposed via the VM ioctl in a later patch. SET_L2_TRAP is per-VM (per-process). Each process has its own TMA buffer allocated in amdgpu_trap_alloc(). There is no conflict between processes on user queue VMIDs. Cc: Christian König <[email protected]> Cc: Alex Deucher <[email protected]> Cc: Felix Kuehling <[email protected]> Cc: Lijo Lazar <[email protected]> Signed-off-by: Srinivasan Shanmugam <[email protected]> --- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 1 - drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_trap.c | 284 ++++++++++++++++------ drivers/gpu/drm/amd/amdgpu/amdgpu_trap.h | 64 ++++- drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 22 +- drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h | 1 + include/uapi/drm/amdgpu_drm.h | 2 - 7 files changed, 285 insertions(+), 91 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c index a93ee106ab92..6473faf18d71 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c @@ -3079,7 +3079,6 @@ const struct drm_ioctl_desc amdgpu_ioctls_kms[] = { DRM_IOCTL_DEF_DRV(AMDGPU_SCHED, amdgpu_sched_ioctl, DRM_MASTER), DRM_IOCTL_DEF_DRV(AMDGPU_BO_LIST, amdgpu_bo_list_ioctl, DRM_AUTH|DRM_RENDER_ALLOW), DRM_IOCTL_DEF_DRV(AMDGPU_FENCE_TO_HANDLE, amdgpu_cs_fence_to_handle_ioctl, DRM_AUTH|DRM_RENDER_ALLOW), - DRM_IOCTL_DEF_DRV(AMDGPU_TRAP, amdgpu_trap_ioctl, DRM_AUTH | DRM_RENDER_ALLOW), /* KMS */ DRM_IOCTL_DEF_DRV(AMDGPU_GEM_MMAP, amdgpu_gem_mmap_ioctl, DRM_AUTH|DRM_RENDER_ALLOW), DRM_IOCTL_DEF_DRV(AMDGPU_GEM_WAIT_IDLE, amdgpu_gem_wait_idle_ioctl, DRM_AUTH|DRM_RENDER_ALLOW), diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c index 47e4680db797..559c9222fc48 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c @@ -1716,7 +1716,7 @@ void amdgpu_driver_postclose_kms(struct drm_device *dev, }amdgpu_ctx_mgr_fini(&fpriv->ctx_mgr);- amdgpu_trap_free(adev, &fpriv->vm, &fpriv->trap_obj); + amdgpu_trap_free(adev, &fpriv->vm, &fpriv->userq_mgr, &fpriv->trap_obj);if (pasid)amdgpu_pasid_free_delayed(pd->tbo.base.resv, pasid); diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_trap.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_trap.c index 31f653ec3fb1..3313bc099cf4 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_trap.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_trap.c @@ -19,12 +19,23 @@ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR * OTHER DEALINGS IN THE SOFTWARE. */ +#include <linux/iosys-map.h> +#include <drm/ttm/ttm_bo.h> + #include <drm/drm_exec.h>#include "amdgpu.h"+#include "amdgpu_gmc.h" +#include "amdgpu_userq.h" +#include "amdgpu_vm.h" #include "cwsr_trap_handler.h" #include "amdgpu_trap.h"+/* First-level TMA layout shared with second-level trap handler ABI */+#define AMDGPU_TRAP_L2_TBA_INDEX 0 +#define AMDGPU_TRAP_L2_TMA_INDEX 1 +#define AMDGPU_TRAP_DEBUG_FLAG_INDEX 2 + extern int cwsr_enable;#define AMDGPU_TRAP_TBA_MAX_SIZE (2 * AMDGPU_GPU_PAGE_SIZE)@@ -380,6 +391,7 @@ int amdgpu_trap_alloc(struct amdgpu_device *adev, struct amdgpu_vm *vm, struct amdgpu_trap_obj *cwsr; struct amdgpu_bo *bo; struct drm_exec exec; + void *cpu_addr; int r;if (!amdgpu_trap_is_enabled(adev))@@ -407,10 +419,21 @@ int amdgpu_trap_alloc(struct amdgpu_device *adev, struct amdgpu_vm *vm,r = amdgpu_bo_create_kernel(adev, AMDGPU_TRAP_TMA_MAX_SIZE, PAGE_SIZE,AMDGPU_GEM_DOMAIN_GTT, &cwsr->tma_bo, NULL, - &cwsr->tma_cpu_addr); + &cpu_addr); if (r) goto err;+ /*+ * Build an iosys_map for the TMA CPU mapping. On dGPUs the TMA BO + * may be placed in VRAM and accessed via MMIO (PCI BAR). Check + * TTM_BO_MAP_IOMEM_MASK to select the correct write accessor. + */ + if (cwsr->tma_bo->kmap.bo_kmap_type & TTM_BO_MAP_IOMEM_MASK) + iosys_map_set_vaddr_iomem(&cwsr->tma_map, + (void __iomem *)cpu_addr); + else + iosys_map_set_vaddr(&cwsr->tma_map, cpu_addr); +Clear NAK to that hack. Please use ttm_bo_vmap() instead if you need an iosys_map.r = amdgpu_trap_map_region(adev, vm, cwsr, AMDGPU_TRAP_TMA); if (r) goto err; @@ -433,6 +456,29 @@ int amdgpu_trap_alloc(struct amdgpu_device *adev, struct amdgpu_vm *vm, return r; }+void amdgpu_trap_clear_l2_handler(struct amdgpu_vm *vm,+ struct amdgpu_trap_obj *trap_obj) +{ + if (!trap_obj || iosys_map_is_null(&trap_obj->tma_map)) + return; + + dma_resv_assert_held(vm->root.bo->tbo.base.resv); + + /* + * Zero TBA first to disable dispatch before clearing TMA. + */ + iosys_map_wr(&trap_obj->tma_map, + AMDGPU_TRAP_L2_TBA_INDEX * sizeof(u64), u64, 0); + dma_wmb(); + iosys_map_wr(&trap_obj->tma_map, + AMDGPU_TRAP_L2_TMA_INDEX * sizeof(u64), u64, 0);It's most likely a good idea to grab the reset lock around all iosys_map_wr() as well.
Thanks Christian. May I kno pls, do you mean taking the existing reset-domain lock around the complete TMA descriptor update (all iosys_map_wr() calls and the final barrier), or only around each individual write?
+ + trap_obj->l2_tba_addr = 0; + trap_obj->l2_tba_size = 0; + trap_obj->l2_tma_addr = 0; + trap_obj->l2_tma_size = 0; +} + int amdgpu_trap_validate_params(struct amdgpu_device *adev, struct amdgpu_trap_params *trap_params, int num_xcc) @@ -464,6 +510,7 @@ int amdgpu_trap_validate_params(struct amdgpu_device *adev, }void amdgpu_trap_free(struct amdgpu_device *adev, struct amdgpu_vm *vm,+ struct amdgpu_userq_mgr *uq_mgr, struct amdgpu_trap_obj **trap_obj) { struct amdgpu_bo *tba_bo; @@ -473,32 +520,41 @@ void amdgpu_trap_free(struct amdgpu_device *adev, struct amdgpu_vm *vm,if (!trap_obj || !*trap_obj || !(*trap_obj)->tma_bo)return; + + /* + * Evict all user queues before freeing. At file-close time waves + * may still be active; without eviction a wave can trap into the + * handler after the TMA BO is freed, causing a GPU page fault. + */ + if (uq_mgr) + amdgpu_userq_evict(uq_mgr); + tba_bo = adev->trap_info->isa_bo; tma_bo = (*trap_obj)->tma_bo;if (!tba_bo || !tma_bo)return;- drm_exec_init(&exec, 0, 0);- drm_exec_until_all_locked(&exec) - { + drm_exec_init(&exec, DRM_EXEC_INTERRUPTIBLE_WAIT, 0); + drm_exec_until_all_locked(&exec) { r = amdgpu_vm_lock_pd(vm, &exec, 0); - if (likely(!r)) - r = drm_exec_lock_obj(&exec, &tba_bo->tbo.base); - drm_exec_retry_on_contention(&exec); if (likely(!r)) r = drm_exec_lock_obj(&exec, &tma_bo->tbo.base); + if (likely(!r)) + r = drm_exec_lock_obj(&exec, &tba_bo->tbo.base);Please don't do such hacks. drm_exec_retry_on_contention() should be called after each drm_exec_lock_obj() call and if (r) goto error handling used here.drm_exec_retry_on_contention(&exec); if (unlikely(r)) { dev_err(adev->dev, "failed to reserve CWSR BOs: err=%d\n", r); - goto err; + goto out; } }+ amdgpu_trap_clear_l2_handler(vm, *trap_obj);+ amdgpu_trap_unmap_region(adev, *trap_obj, AMDGPU_TRAP_TBA); amdgpu_trap_unmap_region(adev, *trap_obj, AMDGPU_TRAP_TMA); -err: +out: drm_exec_fini(&exec); amdgpu_bo_free_kernel(&(*trap_obj)->tma_bo, NULL, NULL); kfree(*trap_obj); @@ -509,120 +565,186 @@ static int amdgpu_trap_validate_user_addr(struct amdgpu_device *adev, struct amdgpu_vm *vm, struct amdgpu_trap_usr_addr *usr_addr) { + struct amdgpu_bo *bo; struct amdgpu_bo_va_mapping *va_map; - uint64_t addr; - uint32_t size; - int r; + u64 addr; + u32 size; + + /* + * Caller holds the VM root BO reservation; assert rather than + * taking it again. + */ + dma_resv_assert_held(vm->root.bo->tbo.base.resv);addr = (usr_addr->addr & AMDGPU_GMC_HOLE_MASK) >> AMDGPU_GPU_PAGE_SHIFT;size = usr_addr->size >> AMDGPU_GPU_PAGE_SHIFT;- r = amdgpu_bo_reserve(vm->root.bo, false);- if (r) - return r; - va_map = amdgpu_vm_bo_lookup_mapping(vm, addr); - if (!va_map) { - r = -EINVAL; - goto err; - } - /* validate whether resident in the VM mapping range */ - if (addr >= va_map->start && va_map->last - addr + 1 >= size) { - amdgpu_bo_unreserve(vm->root.bo); - return 0; + if (!va_map) + return -EINVAL; + + /* + * TBA/TMA BOs must be VM_ALWAYS_VALID. The GPU jumps to TBA on + * hardware exception without any software job submission, so the + * kernel cannot add TBA/TMA to a job's used-buffer list to keep + * page tables current. Without VM_ALWAYS_VALID the BO can be + * evicted and the page table entry becomes stale, causing a GPU + * page fault or memory corruption on the next shader exception. + */That comment is not correct. This would work completely without AMDGPU_GEM_CREATE_VM_ALWAYS_VALID. Please just completely drop it.+ bo = va_map->bo_va->base.bo; + if (!(bo->flags & AMDGPU_GEM_CREATE_VM_ALWAYS_VALID)) { + dev_dbg(adev->dev, + "TBA/TMA BO at VA 0x%llx must use VM_ALWAYS_VALID\n", + usr_addr->addr); + return -EINVAL; }- r = -EINVAL;-err: - amdgpu_bo_unreserve(vm->root.bo); + if (addr >= va_map->start && va_map->last - addr + 1 >= size) + return 0;- return r;+ return -EINVAL; }-static int amdgpu_trap_set_l2_trap_handler(struct amdgpu_device *adev, struct amdgpu_vm *vm,- struct amdgpu_trap_obj *cwsr_obj, - struct amdgpu_trap_usr_addr *tma, - struct amdgpu_trap_usr_addr *tba) +int amdgpu_trap_vm_set_l2_trap(struct amdgpu_device *adev, + struct amdgpu_vm *vm, + struct amdgpu_userq_mgr *uq_mgr, + struct amdgpu_trap_obj *trap_obj, + u64 tba_va, uint32_t tba_sz, + u64 tma_va, uint32_t tma_sz) { - uint64_t *l1tma; + struct amdgpu_trap_usr_addr tba = { .addr = tba_va, .size = tba_sz }; + struct amdgpu_trap_usr_addr tma = { .addr = tma_va, .size = tma_sz }; + long timeout; int r;if (!amdgpu_trap_is_enabled(adev))return -EOPNOTSUPP; - - if (!cwsr_obj || !cwsr_obj->tma_cpu_addr || !tma || !tba) + if (!trap_obj || iosys_map_is_null(&trap_obj->tma_map)) return -EINVAL; - r = amdgpu_trap_validate_user_addr(adev, vm, tma); + + /* + * Wait for any kernel-queue work using this VM to drain before + * evicting user queues and writing the new descriptor. Without + * this wait a kernel-queue wave can observe a partially-written + * TBA/TMA pair during the update window. SET fails if the wait + * is interrupted or times out so the caller can retry. + */ + timeout = amdgpu_vm_wait_idle(vm, MAX_SCHEDULE_TIMEOUT); + if (timeout <= 0) + return timeout < 0 ? timeout : -ETIMEDOUT; + + /* + * Evict all user queues before updating TBA/TMA. Writing TBA and + * TMA as two separate stores creates a race: a wave that traps + * between them reads a mismatched pair. Evicting first ensures no + * wave is active during the update. + */ + amdgpu_userq_evict(uq_mgr); + + /* Flush GPU TLB to remove stale cached TBA/TMA values */ + amdgpu_gmc_flush_gpu_tlb_pasid(adev, vm->pasid, 2, true, 0); + + r = amdgpu_bo_reserve(vm->root.bo, false); if (r) return r; - r = amdgpu_trap_validate_user_addr(adev, vm, tba); + + r = amdgpu_trap_validate_user_addr(adev, vm, &tba); + if (r) + goto out_unreserve; + r = amdgpu_trap_validate_user_addr(adev, vm, &tma); + if (r) + goto out_unreserve; + + /* Zero TBA first to disable residual handler, then TMA, then TBA */That doesn't seem to make sense. Why should that be necessary.
I added the initial TBA=0 wrtie defensively to avoid exposing a stale descriptor during the update. Since execution is quiesced before updating the TMA, it isn't necessary, I'll remove it and place the barrier after the final TBA/TMA write.
Thanks, Srini
