Skip to content

feat(amdsmi): reland gpu_block_t expansion with rdc build fix - #10973

Merged
marifamd merged 6 commits into
developfrom
users/marifamd/rdc-block-name-map-sync
Sep 1, 2026
Merged

marifamd merged 6 commits into
developfrom
users/marifamd/rdc-block-name-map-sync

Conversation

@marifamd

@marifamd marifamd commented Aug 31, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

PR #10179 (feat(amdsmi): expand amdsmi_gpu_block_t with new IP blocks) added 20
RAS IP blocks at bits 19-38 and moved AMDSMI_GPU_BLOCK_LAST from
AMDSMI_GPU_BLOCK_MPIO (1ULL << 18) to AMDSMI_GPU_BLOCK_UCIE_PCS
(1ULL << 38). It updated every AMD SMI consumer of that enum, but missed
projects/rdc, which had hard-coded the old tail. develop sat at a ~76%
TheRock CI failure rate until #10947 reverted it.

This PR fixes RDC first, relands #10179 unchanged on top, then repairs an
out-of-bounds read in an AMD SMI example that the wider enum makes worse.

Technical Details

Three commits, in dependency order. Each builds on its own.

1. fix(rdc): remove hard-coded AMD SMI GPU block enum bounds

Build break (tests/rdc_tests/test_common.cc, test_common.h) — the CI failure:

  • kBlockNameMap was a copy of AMD SMI's block name table guarded by
    static_assert(AMDSMI_GPU_BLOCK_LAST == AMDSMI_GPU_BLOCK_MPIO). Its only
    consumer, GetBlockNameStr(), has zero callers anywhere in projects/rdc.
    The map and its guard did nothing except fail the build when AMD SMI added a
    block. Removed the map, the assert, the function, and its declaration.
    GetErrStateNameStr() / kErrStateNameMap are left untouched.

Latent hang (rdc_libs/rdc/src/RdcMetricFetcherImpl.cc) — not a test, production library code:

  • get_ecc_total() and get_ecc_deferred_total() walked the enum with
    for (uint32_t b = AMDSMI_GPU_BLOCK_FIRST; b <= AMDSMI_GPU_BLOCK_LAST; b = b * 2).
    amdsmi_gpu_block_t is 64 bits wide (AMDSMI_GPU_BLOCK_RESERVED = 1ULL << 63).
    Once any block sits above bit 31, b * 2 wraps to 0, 0 <= LAST stays true,
    and the loop never terminates. Both counters widened to uint64_t.

PR #10179 had already applied this same widening inside AMD SMI, in
tests/amd_smi_test/functional/gpu/ras/err_cnt_read_test.cc. RDC's two copies
were the ones missed.

2. feat(amdsmi): expand amdsmi_gpu_block_t with new IP blocks

Relands be17474f7 verbatim (reverted in 2d4df4291), credited to the original
author. Header, generated Python wrapper, Python interface, Rust bindings, and
the AMD SMI test suite.

3. fix(amdsmi): bound the DRM example's RAS block name lookup

example/amd_smi_drm_example.cc walked every amdsmi_gpu_block_t value but
indexed char block_names[14][10], reading past the end of the array and
printing whatever followed on the stack:

Enum state Loop iterations Array entries Overread
Before #10179 (1ULL << 18) 19 14 5 entries (already broken)
After commit 2 (1ULL << 38) 39 14 25 entries

The bug predates #10179 — the array was already missing MCA, VCN, JPEG,
IH, and MPIO — but commit 2 widens it fivefold, so it is fixed here rather
than left in tree.

  • Filled in all 39 names, verified position-by-position against the enum.
  • Switched to const char*; the old 10-byte cell would have truncated
    KPX_SERDES (10 chars + NUL).
  • Bounded the lookup so a future block addition prints UNKNOWN instead of
    reading out of bounds.

Full-monorepo sweep

Checked every consumer of AMDSMI_GPU_BLOCK_LAST outside the header to confirm
nothing else breaks the way RDC did:

Consumer Status
rdc/.../RdcMetricFetcherImpl.cc (x2) Fixed in commit 1 (uint64_t)
rdc/tests/.../test_common.cc Coupling removed in commit 1
amdsmi/src/amd_smi/amd_smi.cc (x2) Safe — auto deduces the 64-bit enum type
amdsmi/tests/.../err_cnt_read_test.cc Already uint64_t via #10179
amdsmi/tests/.../test_common.cc Map + assert updated by #10179
amdsmi_cli/subcommands/metric.py Safe — name lookup, not enumeration
amdsmi/example/amd_smi_drm_example.cc Pre-existing out-of-bounds read, fixed in commit 3
rocm-smi-lib/.../err_cnt_read.cc Unaffected — different enum (rsmi_gpu_block_t)

Issue Tracking

None.

Test Plan

  • Reproduced the original CI failure locally by restoring the expanded enum and
    compiling the affected translation units.
  • Compiled the RDC sources against both enum widths (1ULL << 18 and
    1ULL << 38) to confirm commit 1 stands alone and is not tied to commit 2.
  • Compiled rdc/tests/rdc_tests/test_common.cc, rdc_libs/rdc/src/RdcMetricFetcherImpl.cc,
    amdsmi/tests/amd_smi_test/test_common.cc, and amdsmi/example/amd_smi_drm_example.cc
    against the final combined tree (-Wall -Wextra).
  • Cross-checked the example's block name list against the enum programmatically,
    position by position, to confirm all 39 entries line up with their bit positions.
  • Standalone reproduction of the iteration bug comparing uint32_t vs uint64_t
    counters at both enum widths, with an iteration cap to detect non-termination.
  • Verified the enum expansion is consistent across all binding layers (header,
    ctypes wrapper, Python interface, Rust) and that generated values match the
    intended bit positions.
  • clang-format plus the full projects/rdc and projects/amdsmi pre-commit
    hook sets on every changed file.

Test Result

  • Before: test_common.cc:62:37: error: static assertion failed: kBlockNameMap needs to be updated
    (expression evaluates to '274877906944 == 262144') — identical to the CI log.
  • After: all three translation units compile clean, no warnings, on the combined tree.
  • Commit 1 alone compiles clean against the pre-expansion enum, so the two commits
    are independently valid.
  • Iteration reproduction: LAST = 1<<18 with uint32_t terminates in 19 steps;
    LAST = 1<<38 with uint32_t does not terminate; LAST = 1<<38 with
    uint64_t terminates in 39 steps.
  • Binding values spot-checked: MMSCH = 524288 (1<<19), OXRP = 2147483648
    (1<<31), PCIE_PL = 4294967296 (1<<32), UCIE_PCS = LAST = 274877906944
    (1<<38); 20 new blocks present in the header, ctypes wrapper, Python
    interface, and Rust bindings.
  • Example block names: 39/39 match the enum in order (bit 0 resolves to UMC, which
    aliases AMDSMI_GPU_BLOCK_FIRST); longest name KPX_SERDES at 10 chars confirms the
    old [10] cell would have truncated it.
  • pre-commit: codespell, trailing whitespace, end-of-file, large files,
    clang-format, ruff, mixed line ending, C++ test conventions, SPDX headers all pass.

The only removed symbol is an unused test-only helper in RDC. The enum expansion
is additive, and the example fix is confined to a sample program.

Submission Checklist

RDC pinned itself to the tail of amdsmi_gpu_block_t in two ways, so
expanding that enum in AMD SMI broke RDC rather than AMD SMI.

The test helper carried a copy of AMD SMI's block name map guarded by
static_assert(AMDSMI_GPU_BLOCK_LAST == AMDSMI_GPU_BLOCK_MPIO). Nothing in
RDC ever called GetBlockNameStr(), so the map and its guard only served
to fail the build when a new IP block was added. Both are removed along
with the unused declaration.

The ECC total fetchers iterated blocks with a uint32_t counter. Once a
block is defined above bit 31 the counter wraps to zero, the loop stops
making progress, and get_ecc_total()/get_ecc_deferred_total() never
return. The counter is now uint64_t, matching the enum's width.

Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
@marifamd
marifamd requested review from a team as code owners August 31, 2026 15:58
@therock-pr-bot

Copy link
Copy Markdown

✅ All Policy Checks Passed

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ✅ Pass —
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —

🎉 All policy checks passed!

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

Relands the expansion from be17474, which was reverted in 2d4df42
because projects/rdc failed to build against it. The preceding commit
removes RDC's coupling to the enum tail, so these blocks land unchanged.

Adds 20 RAS IP blocks at bits 19-38 and moves AMDSMI_GPU_BLOCK_LAST to
AMDSMI_GPU_BLOCK_UCIE_PCS across the public header, the generated Python
wrapper, the Python interface, the Rust bindings, and the AMD SMI test
suite.

Co-authored-by: Bindhiya Kanangot Balakrishnan <Bindhiya.KanangotBalakrishnan@amd.com>
Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
@marifamd
marifamd requested a review from a team as a code owner August 31, 2026 16:33
@marifamd marifamd changed the title fix(rdc): remove hard-coded AMD SMI GPU block enum bounds feat(amdsmi): reland gpu_block_t expansion with rdc build fix Aug 31, 2026
The example walked every amdsmi_gpu_block_t value but indexed a
14-entry name array, so blocks from MCA onward read past the end and
printed whatever followed on the stack. The list was already five short
of the pre-expansion enum; widening the enum to bit 38 stretched the
overread to 25 entries.

Fill in all 39 names, switch to const char* so KPX_SERDES is not
truncated by the old 10-byte cell, and bound the lookup so a future
block addition prints UNKNOWN instead of reading out of bounds.

Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
@marifamd
marifamd merged commit 721f051 into develop Sep 1, 2026
71 of 73 checks passed
@marifamd
marifamd deleted the users/marifamd/rdc-block-name-map-sync branch September 1, 2026 05:08
@systems-assistant

Copy link
Copy Markdown
Contributor

TheRock Submodule Bump Activity

Newest first

marifamd added a commit that referenced this pull request Sep 24, 2026
## Motivation

`amdsmi-license-headers` is `pass_filenames: false`, so it rescans the
whole tree. Once a violation lands on `develop`, every later PR goes 
red instead of the merge that caused it. #10010 merged on a
stale-green formatting run and reddened #10973 and #10998 until #8561
fixed the headers. Third time against this hook (#10402, #10845,
#8561), each fixed reactively.

## Technical Details

`.github/workflows/lstt-formatting.yml`:

- `push` trigger on `develop`, same path filter as `pull_request`, so a
whole-tree hook runs on the merge commit.
- `--from-ref` is event-aware via `FROM_REF`: `github.event.before` on
push, base branch on PR. PR behavior unchanged.
- `cancel-in-progress` limited to `pull_request`, so consecutive merges
each keep their verdict.

Detection at merge, not prevention. Prevention is "Require branches to
be up to date before merging". `develop`'s ruleset has
`required_status_checks` with 
`strict_required_status_checks_policy: false`, so a PR can merge on a
check that last ran against an older base — which is how the stale-green merge
happened.


Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
anujshuk-amd pushed a commit that referenced this pull request Sep 25, 2026
## Motivation

`amdsmi-license-headers` is `pass_filenames: false`, so it rescans the
whole tree. Once a violation lands on `develop`, every later PR goes 
red instead of the merge that caused it. #10010 merged on a
stale-green formatting run and reddened #10973 and #10998 until #8561
fixed the headers. Third time against this hook (#10402, #10845,
#8561), each fixed reactively.

## Technical Details

`.github/workflows/lstt-formatting.yml`:

- `push` trigger on `develop`, same path filter as `pull_request`, so a
whole-tree hook runs on the merge commit.
- `--from-ref` is event-aware via `FROM_REF`: `github.event.before` on
push, base branch on PR. PR behavior unchanged.
- `cancel-in-progress` limited to `pull_request`, so consecutive merges
each keep their verdict.

Detection at merge, not prevention. Prevention is "Require branches to
be up to date before merging". `develop`'s ruleset has
`required_status_checks` with 
`strict_required_status_checks_policy: false`, so a PR can merge on a
check that last ran against an older base — which is how the stale-green merge
happened.


Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
dgaliffiAMD pushed a commit that referenced this pull request Sep 25, 2026
## Motivation

`amdsmi-license-headers` is `pass_filenames: false`, so it rescans the
whole tree. Once a violation lands on `develop`, every later PR goes 
red instead of the merge that caused it. #10010 merged on a
stale-green formatting run and reddened #10973 and #10998 until #8561
fixed the headers. Third time against this hook (#10402, #10845,
#8561), each fixed reactively.

## Technical Details

`.github/workflows/lstt-formatting.yml`:

- `push` trigger on `develop`, same path filter as `pull_request`, so a
whole-tree hook runs on the merge commit.
- `--from-ref` is event-aware via `FROM_REF`: `github.event.before` on
push, base branch on PR. PR behavior unchanged.
- `cancel-in-progress` limited to `pull_request`, so consecutive merges
each keep their verdict.

Detection at merge, not prevention. Prevention is "Require branches to
be up to date before merging". `develop`'s ruleset has
`required_status_checks` with 
`strict_required_status_checks_policy: false`, so a PR can merge on a
check that last ran against an older base — which is how the stale-green merge
happened.


Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
marifamd pushed a commit that referenced this pull request Oct 27, 2026
## Motivation

A release notes review found that the RDC changelog on develop mixes RDC
1.4.0 entries, which ship in ROCm 10.2.0, with entries that already
shipped in ROCm 10.1.0 as RDC 1.3.1. #11722 opened the 1.4.0 section as
"for ROCm 10.1.0" after ROCm 10.1 had branched, so later RDC entries
landed under the wrong release.

## Technical Details

- Retitle `## RDC 1.4.0 for ROCm 10.1.0` to `## RDC 1.4.0 for ROCm
10.2.0`.
- Add `## RDC 1.3.1 for ROCm 10.1.0` with the two entries from #10973,
which ROCm 10.1.0 contains:
  - Removed: the unused `GetBlockNameStr()` test helper.
  - Resolved: the `RDC_FI_ECC_*_TOTAL` hang on GPU blocks above bit 31.
- Keep the `RDC_FI_GPU_MEMORY_CUR_BANDWIDTH` fix (#11634) under 1.4.0 as
a one-line entry that says the fix is now included. #11634 merged on
2026-09-15, after the ROCm 10.1 branch point (2026-09-08), and ROCm
10.1.0 does not contain it. The published ROCm 10.1.0 release notes
already list this fix and can't be changed, so the 1.4.0 entry doesn't
repeat the full description.
- The two #10973 entries move unchanged. No other files change.

Evidence:

- TheRock tag `therock-10.1` pins rocm-systems `7da43026e4f`, which is
also the rocm-systems `therock-10.1` tag and the head of
`release/therock-10.1`. Its RDC version is 1.3.1.
- `721f051b86c` (#10973) is an ancestor of `7da43026e4f`. `fc654c7c9db`
(#11634) is not. Both are in `release/bkc/therock-10.2-20261013`.
- The released ROCm 10.1.0 packages match. `amdrocm-amdsmi10.1` 10.1.0-3
reports version `27.1.0+7da43026`. The `librdc.so` in `amdrocm-rdc10.1`
10.1.0-3 does not contain `derive_mem_activity_percent()`, the function
#11634 added. The 10.2 nightly from 2026-10-21 does.

## Issue Tracking

No JIRA ticket. Reported in a release notes review.

## Test Plan

Changelog-only change. A script moved the #10973 entries byte for byte
and checked that the only added or removed non-blank lines are section
headings. A second script replaced the #11634 entry by exact match and
checked that it stays in the 1.4.0 section.

---------

Signed-off-by: Adam360x <Adam.pryor@amd.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants