On 9/16/26 17:10, 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?
>>
>>> 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?
One read side lock around all iosys_map_wr() calls and the barrier should be
sufficient.
The problem is that during a reset you can't access VRAM for a moment without
crashing the system.
So we need to avoid writing to VRAM at the same time as a GPU reset is ongoing.
Regards,
Christian.
>
>
>>
>>> +
>>> + 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
>
