You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
[rocjitsu] Prevent hangs when memory instructions are rejected - #14573
Rejecting an illegal memory access left the process hanging in hipDeviceSynchronize() with no response. A deeper inspection found the issue in the queue handling which falsely assumed the VM had already reported the fault, even when the instruction was rejected before translation.
This PR reports those failures as aperture violations and corrects the classification of unsupported operations and retired VM snapshots (see table below). This complements kuhar’s LDS bounds fix in #14434, which clips out-of-range accesses and completes normally. The audit also found missing checks and misclassified failures:
Case
Before
After
Rejection before translation
Could leave the host waiting indefinitely
Delivers the appropriate queue exception
Misaligned LDS atomic
Could modify memory
Aperture violation before writes
Private FLAT atomic64
Could access scratch incorrectly
Aperture violation
FLAT address in the aperture hole
Reached ordinary memory handling
Aperture violation
Negative S_BUFFER immediate
Could be hidden by descriptor masking
Aperture violation
Unsupported operation / retired VM snapshot
Classified as a translation fault
Unsupported / Revoked
Missing scratch backing
Returned a fault without reporting it
Reports the VM fault
The new LDS and FLAT checks cover RDNA3/3.5/4 and CDNA5. Regressions check runtime/debugger notification as well as memory effects: 1,185 passed, 275 skipped.
Distinguish instruction MEMVIOL from translation faults, unsupported execution and retired snapshots. Preserve typed outcomes through KFD delivery and cancel terminal memory wait producers before handing off a wave.
Reject negative scalar-buffer immediates before masked memory accesses, preserve MEMVIOL ahead of debugger probes, and report missing scratch backing through a snapshot protected by its revocation lease.
Validation: rocjitsu_tests and rocjitsu_shared build. Focused CTest selection: 1157 passed, 275 skipped, no failures. Changed-line formatting and git diff --check pass.
Validate the requesting LDS lanes before either pipeline commits side effects. Cover direct DS, uniform FLAT-to-LDS and mixed global/LDS requests on RDNA3, RDNA3.5, RDNA4 and CDNA5; retain existing policies on older targets.
Decoded RDNA4/CDNA5 tests cover both widths, aligned controls, inactive misaligned lanes and an earlier global lane. An AQL regression checks queue exception delivery and unsuccessful completion. Physical qualification is the saved gfx1201 alignment capture; CDNA5 follows its ISA requirement.
Validation: rocjitsu_tests builds; 72 focused CTests pass. Changed-line formatting and git diff --check pass.
Record address-formation MEMVIOL lanes on RDNA3, RDNA3.5, RDNA4 and CDNA5 without conflating private provenance with scratch layout. Reject any requesting violating lane before global or LDS effects, and preserve the outcome ahead of debugger translation probes.
Decoded tests cover returning and non-returning atomics, supported private atomic32, global atomic64, mixed waves and inactive private lanes on all four targets. A translated debugger regression verifies that an earlier unmapped global lane cannot replace the instruction violation with a page fault.
Validation: rocjitsu_tests builds; 75 focused CTests pass. Changed-line formatting and git diff --check pass. No new physical measurement is claimed.
Classify forbidden FLAT addresses before memory access while preserving configured shared/private apertures and canonical global addresses. Keep architectural address width separate from the private backing layout: RDNA3-4 use signed 48-bit globals and CDNA5 uses signed 57-bit globals.
RDNA tests the base before IOFFSET. Follow the specific CDNA5 section 10.1.1 final-address rule rather than its conflicting generic section 10.3 wording. Reuse the requesting-lane MEMVIOL mask and existing debugger/runtime routing.
Decoded load/store regressions on all four targets deliberately back invalid addresses in physical memory, check mixed and inactive lanes, preserve high canonical addresses and distinguish offset ordering at the aperture boundary.
Validation: rocjitsu_tests and rocjitsu_shared build. Focused VM, routing, debugger and memory checks select 1460 tests: 1185 passed, 275 skipped, no failures. Changed-line formatting and git diff --check pass. No new physical measurement is claimed.
This PR addresses rocjitsu synchronization hangs by reporting rejected memory instructions as aperture violations rather than waiting for translation-fault notifications. It complements the LDS bounds work with instruction validation and clearer failure classification.
Changes:
Propagate aperture violations to runtime and debugger owners, while distinguishing unsupported accesses and retired VM snapshots.
Share FLAT shared-lane classification and LDS alignment validation between routing and debugger preflight. Cover mixed unmapped-global and LDS lanes, both atomic widths, inactive lanes, and aligned controls. Document that RDNA cross-aperture immediate offsets are undefined rather than asserting a hardware outcome for that review example.
Fix GCC dangling-else diagnostics around GoogleTest assertions and apply the pinned formatter. Validation: 553 focused tests passed, 36 skipped; the new regression fails for both widths with debugger preflight disabled. GCC 13 syntax checks and pre-commit pass.
The reason will be displayed to describe this comment to others. Learn more.
Does the Revoked path also need a terminal notification that does not depend on a VM fault event? In emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/vm/amdgpu/compute_unit.cpp:530, it publishes kAqlQueueMemoryViolation even though the retired/missing snapshot produces no fault report. notify_dispatch_vm_fault() cancels dispatch work but does not create that event, and ROCr's AqlQueue::ExceptionHandler defers this status to VMFaultHandler.
For a still-live queue, this appears to retain the same missing-event hang risk addressed by the aperture path. Please use an appropriate terminal queue-abort path, or establish why a live owner cannot reach this case. InfrastructureFailuresDoNotReachTheArchitecturalFaultOwner currently checks the emitted status without exercising ROCr completion.
Can we give the repeated private FLAT atomic64 rule one shared predicate while keeping each architecture's offset ordering explicit? Under emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/arch/amdgpu/, rdna3/addr_calc.cpp:143 and the RDNA3.5/RDNA4/CDNA5 counterparts repeat the private-address + atomic + 8-byte rejection. A predicate next to shared/flat_address.h:49 would keep the rule and its explanation together without depending on VectorMemState.
The same cleanup can put the RDNA3/3.5 hole check inside the existing FLAT segment guard and pass the 48/57-bit VA width explicitly from each architecture. Preserve RDNA's pre-offset hole check and CDNA5's post-offset check; this does not require extending coverage to CDNA1–4.
It would help to expose the new contracts in the API documentation. Under emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/:
Document outcome at vm/amdgpu/compute_unit.h:439 and kmd/linux/simulated_kfd.h:717: callers supply Faulted or ApertureViolation, and only the former permits XNACK replay. The KFD implementation already asserts this subset of the enum.
Attach the address-width/aperture explanation at isa/arch/amdgpu/shared/flat_address.h:47 to the helper with ///.
Give rejects_misaligned_lds_atomic at vm/amdgpu/memory_pipeline.h:31 a brief description and an @param address_bias explaining that it is added to each lane address before checking alignment. This makes its pre-routing use discoverable alongside the other documented APIs.
We should ensure that a debugger probe handed to the runtime owner also delivers the VM fault event that ROCr needs. At emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/vm/amdgpu/compute_unit.cpp:2311, query_access() can yield a claimed Faulted access without reaching the pipeline's reporter. With SCRATCH_BASE=0 and scratch_load_b32 or scratch_store_b32 at ioffset=0x20000, the current-head repro gets one VM fault report with debugging off and zero with debugging on.
This predates the PR and also affects unmapped global accesses; the new LdsAlignmentMemviolPrecedesDebuggerTranslationProbe test encodes zero reports for a claimed non-aperture fault. Please cover runtime ownership explicitly, including a process sharing a CU with a debugged process and a debugger stop that falls back to the runtime. Report the fault on that fallback or let the pipeline handle it; a debugger-accepted stop should not unconditionally trigger ROCr's fatal handler. The missing event is reproduced in simulator tests; the resulting HIP synchronization hang has not been tested end to end.
The reason will be displayed to describe this comment to others. Learn more.
This would be easier to maintain if the transport adapters asserted that instruction-only outcomes are unexpected. ApertureViolation requires the issuer to publish a fault that no transport reported, but the new arms silently map it to Faulted; Unsupported similarly becomes Malformed.
The current producers do not emit these values. Please make that boundary explicit in command_processor.cpp:259, sdma_ring_consumer.cpp:22, sdma_packet_processor.cpp:115, hbm_controller.h:135, and memory_side_cache.cpp:99, so these adapters do not appear to implement the instruction-level notification contract. This need not require a result-type refactor.
The reason will be displayed to describe this comment to others. Learn more.
I think we should clarify the VMID-dependent behavior here and qualify the gfx90a claim in MisalignedScalarAtomicPublishesApertureViolation. ScalarAtomicTest.MisalignedTwoDwordAtomicFaultsWithoutUpdate expects Malformed plus a VM fault report for translated VMs, but ApertureViolation without a report for VMID 0. Saying the paths "match" hides that classification difference.
Please state that the gfx90a observation establishes rejection without writes, while the exact exception classification needs separate qualification. Document the current split, or move alignment rejection before translation and test both paths if uniform classification is intended.
The reason will be displayed to describe this comment to others. Learn more.
Please move "Uniform shared-aperture FLAT must reject unsupported DDS too" directly above rejects_flat_dds. The intervening MEMVIOL check now makes that sentence look like an explanation of memviol_lane_mask, which rejects a different condition.
The reason will be displayed to describe this comment to others. Learn more.
We should preserve DDS rejection before debugger translation probes too, preferably through a shared preflight used by the pipeline and this site. At the current head, a CDNA5 flat_load_b32 with lane 0 at unmapped 0x20000 and lane 1 at shared_base + 0xffffffc0 reaches Unsupported with debugging off. With debugging on, the probe instead offers Faulted to the memory-violation handler and produces no VM fault report.
The guard mirrors address-formation and LDS-alignment checks but omits rejects_flat_dds. Please add a debugger-enabled mixed-DDS regression and keep the bypass rationale with the shared check, so new pre-translation rejections cannot silently take a different notification path. The current LDS fix does not cover this case.
The reason will be displayed to describe this comment to others. Learn more.
Should we also reject misaligned VMID 0 vector atomics, or explicitly track them as remaining alignment work? The ISA's payload-alignment rule includes global/BUFFER atomics, but this preflight only visits LDS lanes. VMID 0 global lanes still reach execute_atomic_rmw → L2Cache::atomic_rmw → SparseMemory::atomic_modify, which checks page crossing but not payload alignment, so an in-page misaligned atomic can modify memory.
This gap predates the PR. Translated VMs already reject the write through GpuVmAccess; the unchecked-write concern is specifically the VMID 0 path. A focused global/BUFFER regression would distinguish it from the scalar VMID 0 and LDS cases covered here.
The reason will be displayed to describe this comment to others. Learn more.
It would be useful to cover RDNA3 and RDNA3.5 in the LDS-alignment matrix as well. rejects_misaligned_lds_atomic enables both families, but these tests only exercise RDNA4 and CDNA5.
Please extend the helper with the appropriate instruction encoders and add both families, including the mixed-FLAT case where no lane may commit. The private-atomic64 and hole-address tests already cover them; the missing coverage is specific to LDS alignment.
For rejected data accesses, wf.pc has already advanced, so this warning names the next instruction rather than the failing one. The missing-GPU-VM path also reaches this branch and exits as "unsupported memory access," hiding the actual infrastructure failure unless VM logging is enabled. Pass the issuing PC and a cause-specific reason into this path so the warning and exit reason identify the failure correctly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
kuhar
left a comment
Rejecting an illegal memory access left the process hanging in
hipDeviceSynchronize()with no response. A deeper inspection found the issue in the queue handling which falsely assumed the VM had already reported the fault, even when the instruction was rejected before translation.This PR reports those failures as aperture violations and corrects the classification of unsupported operations and retired VM snapshots (see table below). This complements kuhar’s LDS bounds fix in #14434, which clips out-of-range accesses and completes normally. The audit also found missing checks and misclassified failures:
Unsupported/RevokedThe new LDS and FLAT checks cover RDNA3/3.5/4 and CDNA5. Regressions check runtime/debugger notification as well as memory effects: 1,185 passed, 275 skipped.