Repository navigation
refactor(amdsmi): remove driver reload API and related functions - #8575
Conversation
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
There was a problem hiding this comment.
Pull request overview
This PR removes the AMD SMI “driver reload” API surface (previously used to reload the amdgpu kernel module) in response to security scan findings, and updates tests/docs/examples to use an explicit modprobe -r amdgpu && modprobe amdgpu workflow instead.
Changes:
- Remove
amdsmi_gpu_driver_reload()/rsmi_dev_amdgpu_driver_reload()and related command-execution helpers from the C/C++ implementation and public headers. - Update tests and examples to no longer call the removed API and to guide users toward manual driver reload steps.
- Add a standalone Python integration script that exercises a full “set -> reload -> verify -> restore” lifecycle using
modprobe.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/amdsmi/tools/hw_compatibility_status.py | Removes the driver-reload API from the compatibility test list. |
| projects/amdsmi/tests/python/integration/test_memory_partition_lifecycle.py | Adds a standalone lifecycle script using modprobe to apply staged memory-partition changes. |
| projects/amdsmi/tests/python/functional/gpu/test_partition.py | Refactors memory-partition functional tests to validate return codes/idempotency without invoking driver reload. |
| projects/amdsmi/tests/amd_smi_test/functional/memorypartition_read_write.cc | Removes driver-reload behavior from the functional test and adds capability-driven expectations. |
| projects/amdsmi/src/amd_smi/amd_smi.cc | Removes the amdsmi_gpu_driver_reload() implementation. |
| projects/amdsmi/rocm_smi/src/rocm_smi.cc | Removes the rsmi_dev_amdgpu_driver_reload() entry point. |
| projects/amdsmi/rocm_smi/src/rocm_smi_utils.cc | Removes executeCommand() utility (used by driver reload flow). |
| projects/amdsmi/rocm_smi/src/rocm_smi_device.cc | Removes Device::restartAMDGpuDriver() and polling helpers. |
| projects/amdsmi/rocm_smi/include/rocm_smi/rocm_smi.h | Removes the public rocm_smi driver reload API declaration and docs. |
| projects/amdsmi/rocm_smi/include/rocm_smi/rocm_smi_utils.h | Removes the executeCommand() declaration from the public utils header. |
| projects/amdsmi/rocm_smi/include/rocm_smi/rocm_smi_device.h | Removes driver reload method declarations from the device interface. |
| projects/amdsmi/py-interface/amdsmi_interface.py | Removes the Python-level amdsmi_gpu_driver_reload() wrapper. |
| projects/amdsmi/py-interface/init.py | Drops re-export of the removed Python wrapper function. |
| projects/amdsmi/include/amd_smi/amdsmi.h | Removes the C header declaration and documentation block for amdsmi_gpu_driver_reload(). |
| projects/amdsmi/example/amd_smi_partition_example.py | Updates the example to instruct manual modprobe reload instead of calling the removed API. |
| projects/amdsmi/example/amd_smi_partition_example.cc | Same as above for the C++ example. |
| projects/amdsmi/example/amd_smi_drm_example.cc | Replaces driver-reload API calls with instructions to run modprobe manually. |
| projects/amdsmi/docs/conceptual/partition.md | Updates partitioning documentation to remove references to the removed driver reload API. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
91f04f3 to
42250d7
Compare
75ff275 to
c44b89c
Compare
49f7f06 to
e235232
Compare
c44b89c to
e088c4c
Compare
e235232 to
8bf64e2
Compare
e088c4c to
cbfe8c6
Compare
8bf64e2 to
55b9056
Compare
55b9056 to
0c02484
Compare
cbfe8c6 to
4f71937
Compare
4f71937 to
559ce1f
Compare
559ce1f to
54ea327
Compare
|
Rocprofiler testfailures were not related to these changes. Happening with also other PRs. |
Signed-off-by: gabrpham <Gabriel.Pham@amd.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
54ea327 to
ee021e9
Compare
#9864) ## Motivation Bring the unified ABI changes onto the `release/therock-10.0` release branch. These already-merged `develop` commits change the amd_smi ABI (64-bit metrics counters, driver-reload removal, unified struct sync) and must land together so the 10.0 header, wrapper and interface stay consistent with each other — and so 10.0 does not ship an ABI that differs from `develop`. The two earlier attempts are closed: #9852 stacked these changes on the Python packaging series, and #9734 was that packaging series. With #9734 closed, the packaging rework is not coming to 10.0, so this PR applies the ABI changes to the raw release branch. ## Technical Details **Five commits** on the `release/therock-10.0` tip (`6ccde5dbc27`). Four are cherry-picks of merged `develop` commits (`-x` provenance preserved); one is a changelog retarget. | # | Commit | Cherry-picked from | Notes | |---|--------|--------------------|-------| | 1 | `15432e923f` fix(amdsmi): Bump GPU metrics accumulator counters (#8210) | `aeb9da985b` | byte-identical | | 2 | `7ff1686439` refactor(amdsmi): remove driver reload API and related functions (#8575) | `706209f4f5` | byte-identical | | 3 | `23ec5edc37` refactor(amdsmi): Sync with unified with known abi changes (#7844) | `923767a953` | 3 conflicts, resolved below | | 4 | `9030ac095c` docs(amdsmi): set changelog version to 10.0 | — | retargets the changelog heading from `7.15.0` to `10.0` | | 5 | `94cec8fda6` fix(rccl): correct UALoE fabric detection and diagnostics on the amdsmi path (#9464) | `0f674c16e8` | byte-identical — **required**, see below | ### Why the RCCL commit is part of this PR Commit 5 is not incidental: **#7844 does not build without it.** `#7844` flattens `amdsmi_fabric_info_t`, deleting `amdsmi_fabric_info_ver_t` and moving the version to a top-level `fabric_version`. RCCL on the release branch hardcodes the old nested spelling: ```cpp // projects/rccl/src/misc/amdsmi_wrap.cc @ 6ccde5d fabricInfo.fabric_info.version // deleted by #7844 fabricInfo.fabric_info.fabric_version.v1 // deleted by #7844 ``` Both identifiers cease to exist after commit 3, so RCCL fails to compile against the new header. `#9464` replaces those accesses with a `amdSmiFabricInfoIsFlat` SFINAE trait that compiles against **both** the flat and nested layouts, and fixes the UALoE detection bugs described in that PR. It is a large diff (8 files, +1140/−29, including new RCCL tests), so it is called out explicitly here. **These five commits must land as a unit.** Splitting the PR, or reverting the RCCL commit on its own, breaks the RCCL build. ### Conflicts resolved in #7844 Three conflicts, all because the release branch lacks changes `#7844` was written on top of: - **`py-interface/CMakeLists.txt`** — kept the release version. `#7844`'s only hunk here reworks the packaging site-packages install block, which does not exist on this branch (it came from the packaging series in the now-closed #9734). - **`py-interface/amdsmi_interface.py`** — keep-both: added the `AMDSMI_GPU_CUID_SIZE` alias (context on `develop`, which has #9273) alongside the release branch's `_AMDSMI_STRING_LENGTH` constants. - **`CHANGELOG.md`** — dropped the packaging-rework entry, which describes work not on this branch. `#7844`'s ABI entries merged cleanly. ## Issue Tracking JIRA ID: AILITOOLS-270, ROCM-1553, ROCM-26658 JIRA ID: AIDREL-13 JIRA ID: AIDREL-19 ## Test Plan - Cherry-picked #8210 → #8575 → #7844 → #9464 onto the `release/therock-10.0` tip (`6ccde5dbc27`) - Full amd_smi build (library + CLI + `amdsmitst`) - Regenerated `amdsmi_wrapper.py` and diffed against the committed file - Diffed every cherry-pick against its `develop` original - Verified the `amdsmi_fabric_info_t` flattening is ABI-preserving, by compiling both headers and comparing `sizeof`/`offsetof` - Checked every in-repo consumer of the changed/removed types for breakage ## Test Result - **Build:** clean (library + CLI + tests, 0 errors) - **Wrapper:** byte-identical after regeneration (0-line diff) - **Cherry-pick fidelity:** commits 1, 2 and 5 diff byte-identically against their `develop` originals (modulo hunk offsets); commit 3 differs only in the three conflicts documented above - **Fabric ABI is preserved:** `sizeof(amdsmi_fabric_info_t) = 320` before and after, with every offset unchanged — `bdf=0`, version `=8`, `v1=12`, `v1.accel_state=252`, `reserved=256` - **Removed symbols:** `amdsmi_gpu_driver_reload` has no remaining references outside the CHANGELOG; none of the 16 removed/renamed symbols has a stale consumer anywhere under `projects/` - **In-repo consumers:** - `projects/rdc` reads `amdsmi_gpu_metrics_t` and the widened accumulators and is *not* modified here — a `-fsyntax-only` A/B of `RdcMetricFetcherImpl.cc` against the old and new headers produces the identical error set (pre-existing, ESMI-gated), so nothing regresses. The fields only widen, and they feed a `uint64_t`-valued map. - `projects/rocshmem` keeps a private dlopen mirror of `amdsmi_fabric_info_t`; it does not include `amd_smi/amdsmi.h`, and the layout is unchanged, so it is unaffected. - `projects/rocprofiler-systems` uses `amdsmi_gpu_metrics_t` and builds green in this PR's TheRock CI. - `projects/rocm-smi-lib` has its own independent `rocm_smi.h`; unaffected. ## Known issues inherited from `develop` Faithful cherry-picks, so these come along unchanged; they are present on `develop` today and are not introduced by this backport. Listed so the release branch owners know what is shipping. Fixes belong on `develop` first. - **`gpu_metrics` accumulators lose their "unset" sentinel** (from #8210). The counters widened to 64-bit but the amdgpu table still reports 32, so the `0xFFFFFFFF` sentinel zero-extends and stops matching the 64-bit sentinel consumers check. `amdsmi_get_gpu_metrics_info()` reports `gfx_activity_acc`/`mem_activity_acc` as a literal `4294967295` instead of `N/A` on hardware that does not report them (reproduced on gfx1030). Unchanged on parts that do report real values. - **`tests/python/cli/test_set.py` fails instead of skipping** (from #7844). `cmds_need_permission` is assigned in `setUpClass` after the `_initialized` early-return, so `TestSet` raises `AttributeError`; and the filter that replaced the old blanket skip covers only 3 of the privileged `set` subcommands, so an unprivileged run also fails on `--perf-level`, `--profile`, `--perf-determinism`, `--clk-level` and others. - **`amd-smi set --json` and `amd-smi static --partition --json` disagree on one key** (from #7844). `set` now emits `accelerator_partition_mem_alloc_mode` while `static` and the `--compute-partition-mem-alloc-mode` flag keep `compute_partition_mem_alloc_mode`. - **Dead constants**: the keep-both resolution leaves `_AMDSMI_STRING_LENGTH` and `_AMDSMI_MAX_STRING_LENGTH` with zero references (every call site moved to the public `AMDSMI_MAX_STRING_LENGTH`). ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. --------- Signed-off-by: Bindhiya Kanangot Balakrishnan <Bindhiya.KanangotBalakrishnan@amd.com> Signed-off-by: Maisam Arif <Maisam.Arif@amd.com> Signed-off-by: gabrpham <Gabriel.Pham@amd.com> Signed-off-by: josnarlo <joseph.narlo@amd.com> Signed-off-by: Galantsev, Dmitrii <dmitrii.galantsev@amd.com> Co-authored-by: gabrpham <Gabriel.Pham@amd.com> Co-authored-by: Joseph Narlo <joseph.narlo@amd.com> Co-authored-by: Galantsev, Dmitrii <dmitrii.galantsev@amd.com> Co-authored-by: Mika Laitio <mika.laitio@amd.com> Co-authored-by: Nilesh M Negi <Nilesh.Negi@amd.com> Co-authored-by: Cursor <cursoragent@cursor.com>
## Motivation The ABI compliance check never flags a break. `check_report_content()` matches text that abi-compliance-checker 2.3 does not print, so every report reads as clean, and major mode passes with a warning even when the checker exits 1. It passed #8575 (removed `amdsmi_gpu_driver_reload`) and #11821 (grew `amdsmi_driver_info_t` from 768 to 1792 bytes, reverted in #13926). ## Technical Details **Report parser** (`tests/abi_check/abi_check.py`): - Read the counts from the report's section headings (`<h2>Removed Symbols <span class='failed'> 1 </span>`) - Fail on removed symbols, High or Medium severity problems, and any `Size of this type has been changed` row, naming the type - The checker rates struct growth Low because it assumes the library allocates the struct; amd-smi callers allocate their output structs, so growth breaks binaries built against the old header - Fail major mode when the checker exits non-zero; it used to finish "with warnings" and pass **Baseline** (`tests/abi_check/abi_check.py`): - Compare against `git merge-base <base> HEAD` instead of the base branch tip. Without this, once the check can fail, a PR that forked before an API landed on `develop` is reported as removing it **Tests and docs**: - `tests/abi_check/abi_check_test.py`: fixtures are fragments of real abi-compliance-checker 2.3 reports; adds merge-base and pass/fail tests - `.pre-commit-config.yaml`: run `abi_check_test.py` when `tests/abi_check/` changes, so the LSTT Formatting job runs it; no workflow ran it before - `tests/README.md`: describe both **Not changed**: `abi-compliance-check.yml`. The check stays non-blocking and still reports through the `MAJOR ABI BREAKAGE` / `MINOR ABI BREAKAGE` labels. The workflow runs the base branch's `abi_check.py`, so this PR's own ABI jobs still use the old parser. ## Issue Tracking Related: #11821, #13926 ## Test Plan - `abi_check_test.py` on Python 3.6, 3.10 and 3.14, against `develop`'s `abi_check.py` and this branch - `develop`'s and this branch's `abi_check.py` with abi-compliance-checker 2.3 on Ubuntu 22.04 (the CI job's tool and OS), major and minor mode, on real `amdsmi.h` pairs, each in a throwaway git repo so `git merge-base` and `git show` behave as in CI - `pre-commit run --from-ref origin/develop --to-ref HEAD` from `projects/amdsmi`, as the LSTT Formatting job runs it ## Test Result - `abi_check_test.py`: the new tests fail on `develop`'s `abi_check.py` (3 failures, 5 errors) and all 16 pass with this change on every Python version Real abi-compliance-checker 2.3 runs, exit code (major / minor): | `amdsmi.h` change | `develop` | This PR | This PR reports | |---|---|---|---| | #8575: removed `amdsmi_gpu_driver_reload` | 0 / 1 | **1** / 1 | Removed Symbols: 1 | | #11821: grew `amdsmi_driver_info_t` | 0 / 1 | **1** / 1 | `amdsmi_driver_info_t` 768 to 1792 bytes | | `release/therock-10.0` to `release/therock-10.1` | 0 / 1 | **1** / 1 | `amdsmi_enumeration_info_t` 276 to 280 bytes | | `release/therock-10.1` to `release/therock-10.2` | 0 / 1 | **1** / 1 | Medium data type problems; `amdsmi_driver_info_t`, `amdsmi_fabric_info_t` and its `fabric_info_` union grew | | #12804: added APIs | 0 / 0 | 0 / 0 | | | #11471: added APIs | 0 / 1 | 0 / 1 | | | `AMDSMI_LIB_VERSION_MINOR` bump | 0 / 1 | 0 / 1 | | | No change | 0 / 0 | 0 / 0 | | | PR forked before #12804, base branch then gained it | 0 / 1 | 0 / 0 | | Minor mode keeps its meaning (any change the checker reports with `-strict`); only its baseline changed. Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
## Motivation Callers allocate most amdsmi structs and load the library by name, so a changed layout corrupts memory in programs built against the old header instead of failing to load. Recent examples: - #11741 grew `amdsmi_fabric_info_t` from 320 to 552 bytes. - #11821 grew `amdsmi_driver_info_t` from 768 to 1792 bytes. - #10226 grew `amdsmi_enumeration_info_t` from 276 to 280 bytes. All three landed within SONAME 27. #14425 made the existing `ABI Compliance Check` report such changes, but it only labels a PR and cannot block it. This PR also covers the follow-ups from #14425's review. ## Technical Details **ABI Compliance Check (#14425 follow-ups)** - `abi_check.py` exits 1 for a break and 2 when it could not compare. That covers a checker error, a missing merge base, an old header it cannot fetch, or a report without the 2.3 summary table. `abi-compliance-check.yml` records exit 2 as `error`, which leaves the labels unchanged instead of adding a false `MAJOR ABI BREAKAGE`. - It defines `ENABLE_ESMI_LIB` before parsing, so the CPU API (69 functions, 12 types) is checked. - It fails on a changed non-string constant, such as `AMDSMI_MAX_MM_IP_COUNT` going from 8 to 16. - Smaller fixes: - a size row no longer inherits a type name from an earlier section; - it works on Python 3.6; - the patterns point at `ACC_COMMIT`; - new tests cover High severity, symbol problems and Low type problems that are not size changes. **AMDSMI ABI Gate (new, blocking once required)** - `tests/abi_check/abi_gate.py` parses `amdsmi.h` with libclang and records: - each type's size, alignment and field offsets; - enumerator values and function signatures; - integer macros; - the units, sentinel values and in/out directions in field and parameter doc comments. It compares the PR with its base and the newest `release/therock-*` branch, each read from git. Additions pass, and so do fields carved from `reserved*` arrays or bitfields when the struct keeps its size. - An intended break is recorded with `abi_gate.py acknowledge --why "#NNNN: ..."`, which adds one line per change (path, old and new value, reason) to `projects/amdsmi/abi/abi_exceptions.json`, together with a CHANGELOG line naming it. The owners of `projects/amdsmi/` review both. The file starts with the three `%` units #13540 documented on develop that 10.2 does not have. - `.github/workflows/amdsmi-abi-gate.yml` runs on every PR to develop and `release/therock-*`, on merge groups, and on pushes to develop. - It has a read-only token and no secrets. - It runs the checker from the base commit, or from develop when the base has none, such as an older release branch. - There is no `paths:` filter, so it can become a required check. PRs that do not touch amdsmi's headers pass in seconds. **TheRock CI** - `.github/skip-ci-config.toml` skips the TheRock build for `projects/amdsmi/abi/*` and `projects/amdsmi/tests/abi_check/*`, which no build reads. This PR does not make the gate required. That is a ruleset change after an advisory period, together with a "require workflows" rule, because on pull requests GitHub runs the PR's own copy of the workflow file. ## Issue Tracking JIRA ID: TBD Follows up #14425. ## Test Plan - `abi_check_test.py`, `abi_gate_test.py` and the amdsmi pre-commit hooks. - `abi_check.py` with abi-compliance-checker 2.3 on real headers: no change, an enum value change, a header that does not compile, and a missing base ref. - Gate replay on the 73 develop commits that changed `amdsmi.h` in the last 12 months, the `release/therock-*` transitions, and 25 bypass fixtures. - The gate workflow run locally with `act` for same-repo and fork PRs, merge groups and pushes to develop. ## Test Result - 29 and 86 unit tests pass. - `abi_check.py` exits 0, 1, 2 and 2 for the four headers. - The gate blocks all 19 layout and API breaks in the replay and misses none. Among them: #11741, #11821, #13926, #8575, #10226, the `AMDSMI_LINK_TYPE_PCIE`/`XGMI` value swap, and the same-size `amdsmi_version_t` change. - It also blocks 4 doc-only unit corrections, which pass once acknowledged. - A PR that replaces the checker with `exit 0` still fails. ## Submission Checklist - [ ] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. --------- Signed-off-by: Maisam Arif <Maisam.Arif@amd.com> Co-authored-by: Maisam Arif <Maisam.Arif@amd.com>
Motivation
Static code scan revealed potentially vulnerable code in the driver reload API.
Technical Details
amdsmi_gpu_driver_reload()made use of an insecure pattern. The functionality had already been removed from the amdsmi CLI, so we resolved the vulnerability by removing the API and its Python binding entirely, along with the underlying ROCm SMI driver-reload plumbing.JIRA ID
JIRA ID : ROCM-26658
Test Plan
N/A
Test Result
N/A
Submission Checklist
N/A