On 9/16/2026 8:40 PM, SRINIVASAN SHANMUGAM wrote:
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?Clear NAK to that hack. Please use ttm_bo_vmap() instead if you need an iosys_map.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 processeson 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.cindex 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.cindex 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.cindex 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); +It's most likely a good idea to grab the reset lock around all iosys_map_wr() as well.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);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?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.+ + 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);That comment is not correct. This would work completely without AMDGPU_GEM_CREATE_VM_ALWAYS_VALID.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. + */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;
Hi Alex,For the kernel queue case, which lock should I use to prevent new work from using this GPUVM while the TMA is being updated? Is locking the VM root BO sufficient, or should I use another existing lock? Also, once that lock is held, is amdgpu_vm_wait_idle() the correct way for the existing kernel-queue work to finish?
Thanks, Srini
