AMD General > -----Original Message----- > From: SHANMUGAM, SRINIVASAN > Sent: Thursday, August 27, 2026 5:07 PM > To: Koenig, Christian <[email protected]>; Natalie Vock > <[email protected]>; Deucher, Alexander <[email protected]> > Cc: [email protected]; Kuehling, Felix <[email protected]>; > Zhu, James <[email protected]>; Lazar, Lijo <[email protected]>; Six, > Lancelot <[email protected]>; Pelloux-Prayer, Pierre-Eric <Pierre- > [email protected]>; Timur Kristóf <[email protected]>; Samuel > Pitoiset <[email protected]> > Subject: RE: [RFC PATCH 1/3] drm/amdgpu/uapi: Add second-level trap handler > ops to VM ioctl > > > > > -----Original Message----- > > From: Koenig, Christian <[email protected]> > > Sent: Wednesday, August 26, 2026 3:20 PM > > To: Natalie Vock <[email protected]>; SHANMUGAM, SRINIVASAN > > <[email protected]>; Deucher, Alexander > > <[email protected]> > > Cc: [email protected]; Kuehling, Felix > > <[email protected]>; Zhu, James <[email protected]>; Lazar, Lijo > > <[email protected]>; Six, Lancelot <[email protected]>; > > Pelloux-Prayer, Pierre-Eric <Pierre- [email protected]>; > > Timur Kristóf <[email protected]>; Samuel Pitoiset > > <[email protected]> > > Subject: Re: [RFC PATCH 1/3] drm/amdgpu/uapi: Add second-level trap > > handler ops to VM ioctl > > > > On 8/25/26 12:13, Natalie Vock wrote: > > > On 8/25/26 09:34, Christian König wrote: > > >> On 8/25/26 07:05, SHANMUGAM, SRINIVASAN wrote: > > >>> AMD General > > >>> > > >>>> -----Original Message----- > > >>>> From: Natalie Vock <[email protected]> > > >>>> Sent: Thursday, August 20, 2026 3:29 PM > > >>>> To: SHANMUGAM, SRINIVASAN > > <[email protected]>; Koenig, > > >>>> Christian <[email protected]>; Deucher, Alexander > > >>>> <[email protected]> > > >>>> Cc: [email protected]; Kuehling, Felix > > >>>> <[email protected]>; Zhu, James <[email protected]>; Lazar, > > >>>> Lijo <[email protected]>; Six, Lancelot <[email protected]>; > > >>>> Pelloux-Prayer, Pierre-Eric <Pierre- > > >>>> [email protected]>; Timur Kristóf > > >>>> <[email protected]>; Samuel Pitoiset <[email protected]> > > >>>> Subject: Re: [RFC PATCH 1/3] drm/amdgpu/uapi: Add second-level > > >>>> trap handler ops to VM ioctl > > >>>> > > >>>> Hi, > > >>>> > > >>>> first of all: Thanks for working on this! It's great seeing trap > > >>>> handler support come together. > > >>>> > > >>>> On 8/20/26 09:01, Srinivasan Shanmugam wrote: > > >>>>> When a GPU shader hits an exception, memory fault, or debug > > >>>>> breakpoint, the hardware jumps to the first-level trap handler. > > >>>>> The first-level handler (managed by the kernel via CWSR) checks > > >>>>> the TMA buffer for a second-level handler address. If one is > > >>>>> installed, it forwards the trap to that userspace handler, > > >>>>> allowing the runtime or debugger to handle shader exceptions > > >>>>> without > > modifying the kernel trap handler. > > >>>>> > > >>>>> KFD already supports this for compute workloads. Render-node > > >>>>> user queues had no equivalent mechanism. Add it. > > >>>>> > > >>>>> The second-level handler is a per-VM setting — it applies to all > > >>>>> shader waves executing under that VMID regardless of queue type. > > >>>>> GFX and compute queues from the same process share the same > > >>>>> VMID, so one SET_L2_TRAP call covers all queue types for that process. > > >>>>> This configuration is not CWSR-specific; CWSR is only the > > >>>>> first-level handler mechanism. The correct home for this setting > > >>>>> is the VM ioctl (DRM_AMDGPU_VM), following the same pattern as > > >>>>> AMDGPU_VM_OP_RESERVE_VMID. > > >>>>> > > >>>>> Add two new VM ioctl operations: > > >>>>> AMDGPU_VM_OP_SET_L2_TRAP (op = 3) — install second-level > > >>>>> handler > > >>>>> AMDGPU_VM_OP_CLEAR_L2_TRAP (op = 4) — remove second- > level > > >>>> handler > > >>>>> > > >>>>> Extend drm_amdgpu_vm_in with a 32-byte union for op-specific data. > > >>>>> The l2trap member carries the GPU virtual addresses and sizes of > > >>>>> the TBA (handler code) and TMA (handler scratch memory). > > >>>> > > >>>> This should be a BO handle and offset+size, instead. The BOs > > >>>> associated with the TBA/TMA must be tracked as used by every > > >>>> submission from the VM that has this trap handler installed, > > >>>> otherwise you introduce a ton of race conditions. Off the top of > > >>>> my head, here > > are a few: > > >>>> 1. The GEM VA ioctl can spuriously fail to actually update page tables. > > >>>> This is okay and intentional, and BOs with outdated page > > >>>> tables will > > >>>> be updated on the next submit if they're used by the > > >>>> submission. If > > >>>> the TBA/TMA BOs aren't marked in the set of used buffers, > > >>>> the PTs may > > >>>> end up never being updated and subsequent accesses will fault. > > >>>> 2. The TBA/TMA may be evicted/moved around concurrently with > > >>>> executing > > >>>> submissions if these submissions didn't add their fences to > > >>>> the > > >>>> TBA/TMA resv, which would likely randomly corrupt things or hang. > > >>>> > > >>>> A simpler solution could be requiring the TBA/TMA buffers to be > > >>>> VM_ALWAYS_VALID, in which case synchronization to all submissions > > >>>> in the VM is taken care of automagically. This prevents exporting > > >>>> the TBA/TMA to an fd, but I don't expect anyone would want to do this. > > >>> > > >>> Hi Natalie, > > >>> > > >>> Thanks for the feedback. > > >>> > > >>> We will update the UAPI in v2 to pass BO handles alongside the GPU > > >>> VA for TBA/TMA. This ensures the kernel can properly track and pin > > >>> the buffers. > > >> > > >> Please don't. It is the responsibility of userspace to make sure > > >> that the BOs are > > either in the used list or valid per VM. > > > > > > Oh, right, I guess we can do it this way too. That's fine by me, but > > > I think my point about syncing TBA/TMA binds/unbinds in the same way > > > as VM map/unmap ops still applies. I suppose for user queues the > > > existing approach could be fine, but as Timur points out RADV won't > > > be able to use this unless support for kernel queues is added. > > > > There are also tons of HW restrictions this patch set needs to take care of. > > > > E.g. you can hard crash the ASIC when you mess up the second level > > trap handler and we somehow need to prevent that. Srini is working on > > at least catching the worst userspace errors but I doubt that this is fully > > fixable. > > Hi @Koenig, Christian, > > We checked the KFD implementation and all GFX versions of the CWSR trap > handler ASM. > > KFD's kfd_ioctl_set_trap_handler() only validates the GPU ID and process > binding > — no address validation: > > /* kfd_chardev.c */ > kfd_process_set_trap_handler(&pdd->qpd, > args->tba_addr, /* no validation */ > args->tma_addr); /* no validation */ > > The first-level ASM handler only checks TBA != 0: > > /* cwsr_trap_handler_gfx9.asm — same in gfx10/gfx12 */ > s_cbranch_scc0 L_NO_NEXT_TRAP /* skip only if TBA == 0 */ > s_setpc_b64 [ttmp2, ttmp3] /* jump — no hw validation */ > > We also found that hardware stores addresses as (addr >> 8): > > /* amdgpu_amdkfd_gfx_v9.c */ > WREG32(mmSQ_SHADER_TBA_LO, lower_32_bits(tba_addr >> 8)); > WREG32(mmSQ_SHADER_TBA_HI, upper_32_bits(tba_addr >> 8)); > > So TBA and TMA must be 256-byte aligned.
Correction to my previous reply: I incorrectly stated that TBA/TMA must be 256-byte aligned. The (addr >> 8) shift in amdgpu_amdkfd_gfx_v9.c applies to SQ_SHADER_TBA/TMA hardware registers used for the first-level handler programming — not to the second-level TBA/TMA stored as full 64-bit values in the first-level TMA buffer via iosys_map_wr(). For our series, the correct alignment requirement for TBA is 4-byte instruction alignment (for s_setpc_b64 in the first-level ASM handler). Corrected v2 validation plan: 1. tba_va and tma_va must be non-zero 2. tba_va must be 4-byte aligned (instruction alignment) 3. Both must be within valid VM GPU VA range Thanks, Srini
