From 4b873b7a7e9216057aacd6575bcf43137df3b01b Mon Sep 17 00:00:00 2001 From: Maisam Arif Date: Fri, 2 Oct 2026 16:53:02 -0500 Subject: [PATCH] fix(amdsmi): report NPM power limit failures correctly Library (rocm_smi, amd_smi.cc, amdsmi.h): - Report a driver-rejected write (EPERM) as AMDSMI_STATUS_NO_PERM instead of NOT_SUPPORTED; WriteSysfsStr() dropped the errno - Treat a UINT64_MAX platform max as unavailable in the setter and restore the previous number parsing for existing NPM readers - Move the setter checks into validate_npm_board_limit() so they run in unit tests without root - Validate node handles against the handle registry itself and replace the BUILD_TESTS-only pointer hook with an unguarded amdsmi_test_get_node_handle(), like amdsmi_test_sleep() - Pin sizeof(amdsmi_npm_info_t) to 64 and correct the doxygen return codes for amdsmi_get_npm_info() and amdsmi_set_npm_limit() CLI (set_value.py, amdsmi_helpers.py, amdsmi_parser.py, node.py): - Record NOT_SUPPORTED and library errors so a failed set -n exits non-zero - Raise AmdSmiInvalidParameterException for -n with -g instead of calling sys.exit(2) - Use static -n help text and drop the hardware probe at parser build - Emit the CSV node column only for node -p Python API (amdsmi_interface.py): - Reject bool limits and fix the node_handle type hints and docstrings Tests and docs: - Add root-free validate_npm_board_limit() tests and use mkdtemp for temp board directories - Move the amdsmi_get_power_info() null check to the power read test - Remove __future__ imports (Python 3.6) and stale test comments - Update CHANGELOG, Python API reference, and CLI docs Signed-off-by: Maisam Arif --- projects/amdsmi/CHANGELOG.md | 9 +- projects/amdsmi/amdsmi_cli/amdsmi_helpers.py | 77 +---- projects/amdsmi/amdsmi_cli/amdsmi_parser.py | 12 +- .../amdsmi/amdsmi_cli/subcommands/node.py | 6 +- .../amdsmi_cli/subcommands/set_value.py | 14 +- .../amdsmi/docs/how-to/amdsmi-cli-tool.md | 1 - .../amdsmi/docs/reference/amdsmi-py-api.md | 55 +--- projects/amdsmi/include/amd_smi/amdsmi.h | 45 +-- .../amd_smi/impl/amd_smi_test_internal.h | 22 +- .../amdsmi/py-interface/amdsmi_interface.py | 82 ++--- .../rocm_smi/include/rocm_smi/rocm_smi.h | 4 +- .../rocm_smi/include/rocm_smi/rocm_smi_npm.h | 3 + projects/amdsmi/rocm_smi/src/rocm_smi.cc | 55 +--- projects/amdsmi/rocm_smi/src/rocm_smi_npm.cc | 60 ++-- projects/amdsmi/src/CMakeLists.txt | 6 - projects/amdsmi/src/amd_smi/amd_smi.cc | 101 ++---- .../amdsmi/tests/amd_smi_test/CMakeLists.txt | 5 - .../functional/gpu/power/power_read_test.cc | 7 + .../gpu/amdsmi_npm_balancing_mode_test.cc | 109 +++---- .../unit/gpu/amdsmi_npm_limit_test.cc | 282 ++++------------- .../unit/gpu/rocm_smi_npm_test.cc | 134 ++++++-- .../test_cli_current_node_power_management.py | 54 ++-- .../unit/gpu/test_cli_set_node_power_limit.py | 297 +++++------------- .../gpu/test_npm_info_current_node_power.py | 26 +- .../python/unit/gpu/test_npm_set_limit.py | 30 +- 25 files changed, 497 insertions(+), 999 deletions(-) diff --git a/projects/amdsmi/CHANGELOG.md b/projects/amdsmi/CHANGELOG.md index 75d80952dcec..21f410cc95c6 100644 --- a/projects/amdsmi/CHANGELOG.md +++ b/projects/amdsmi/CHANGELOG.md @@ -25,7 +25,7 @@ Full documentation for amd_smi_lib is available at [https://rocm.docs.amd.com/pr - **Added NPM (Node Power Management) power limit setting**. - New `amd-smi set --node-power-limit`/`-n` to set the node-level power limit in watts (node-wide, not per-GPU). - New API: `amdsmi_set_npm_limit()`. - - New `current_node_power` field in `amdsmi_npm_info_t` (returned by `amdsmi_get_npm_info()`), the current (instantaneous) node power in watts, queried once per node rather than once per GPU; `amd-smi node -p` now displays it when available. + - New `current_node_power` field in `amdsmi_npm_info_t` (returned by `amdsmi_get_npm_info()`), the current (instantaneous) node power in watts. `amd-smi node -p` shows it as `CURRENT_NODE_POWER` (`N/A` when unavailable). - New `max_node_power_limit` field in `amdsmi_npm_info_t` (returned by `amdsmi_get_npm_info()`), the platform max bound for `amdsmi_set_npm_limit()` requests. - `amdsmi_set_npm_limit()` rejects the request with `AMDSMI_STATUS_INVAL` when NPM is disabled on the node (`amdsmi_npm_info_t::status == AMDSMI_NPM_STATUS_DISABLED`), instead of writing a limit that would have no defined effect. `amd-smi set --node-power-limit` performs the same check up front to fail fast with a friendlier message. @@ -118,8 +118,11 @@ Full documentation for amd_smi_lib is available at [https://rocm.docs.amd.com/pr - **`amdsmi_get_npm_info()` and `amdsmi_set_npm_limit()` now reject `amdsmi_node_handle` values not vended by `amdsmi_get_node_handle()`**. - Previously any non-null handle was dereferenced directly; an unregistered/garbage handle now returns `AMDSMI_STATUS_INVAL` instead. -- **NPM sysfs numeric reads (e.g. `cur_node_power_limit`, `max_node_power_limit`) now reject negative or malformed content**. - - Previously a leading `-` (e.g. `"-1"`) parsed successfully as `UINT64_MAX`; such content now fails with `RSMI_STATUS_UNEXPECTED_DATA`. +- **`amd-smi node -p --csv` now starts with a `node` column**. + - Other `amd-smi node` CSV output (for example `--gtt` or `--base-board-temps` alone) has no `node` column. + +- **`amd-smi set --gtt` no longer reports unrelated argument errors as a `--gpu` conflict**. + - Only `--gpu`/`-g` given without a value is rewritten to the `--gtt` conflict message; other argument errors are shown as-is. - **`amd-smi`, `amd-smi version`, and `amd-smi static --driver` include the DKMS build in the amdgpu version**. - For example `6.19.14.31400000-2370381`. Labels and JSON/CSV keys are unchanged. diff --git a/projects/amdsmi/amdsmi_cli/amdsmi_helpers.py b/projects/amdsmi/amdsmi_cli/amdsmi_helpers.py index 244d07e1de58..0c268b78fb1c 100755 --- a/projects/amdsmi/amdsmi_cli/amdsmi_helpers.py +++ b/projects/amdsmi/amdsmi_cli/amdsmi_helpers.py @@ -1681,28 +1681,6 @@ def format_power_cap(self, value): return f"{converted} W" return value - def get_max_node_power_limit(self): - """Get the platform max NPM node power limit (Watts), for help-text bounds. - - Mirrors get_power_caps(): queried once at parser-build time so - --node-power-limit's help text can show its valid upper bound. NPM is - node-wide, so the first device that yields a reading is sufficient. - """ - for dev in self.get_gpu_handles(): - try: - node_handle = amdsmi_interface.amdsmi_get_node_handle(dev) - max_node_power_limit = amdsmi_interface.amdsmi_get_npm_info(node_handle)[ - "max_node_power_limit" - ] - if max_node_power_limit != "N/A": - return f"{max_node_power_limit} W" - except (amdsmi_interface.AmdSmiLibraryException, KeyError) as e: - logging.debug( - f"AMDSMIHelpers.get_max_node_power_limit - Unable to get NPM info for device {dev}: {str(e)}" - ) - continue - return "N/A" - @staticmethod def detect_gpu_od(bdf): """Detect if a GPU has the gpu_od sysfs interface. @@ -3629,34 +3607,7 @@ def validate_and_set_node_balancing_mode(self, node_handle, requested_mode, logg return message def validate_and_set_node_power_limit(self, node_handle, requested_limit, logger): - """Validate and set the NPM (Node Power Management) power limit for a node. - - Mirrors validate_and_set_power_cap(): fetches the platform max bound via - amdsmi_get_npm_info() (amdsmi_npm_info_t::max_node_power_limit, sourced from - board/max_node_power_limit) and rejects out-of-range requests before ever - issuing the write. This is an early, CLI-friendly pre-check only: - amdsmi_set_npm_limit() itself already validates `limit` against the same - bound internally and fails closed if that bound can't be read (see its - docstring); this CLI-side check exists purely to fail fast and surface a - clean, user-facing hint here rather than relying solely on the library - call's exception. - - There is no driver-exposed minimum bound beyond positivity (no sysfs file - or design-doc source documents a real minimum), so the lower bound enforced - here is simply "> 0". - - Also rejects the request up front if NPM itself is disabled on this node - (amdsmi_npm_info_t::status == AMDSMI_NPM_STATUS_DISABLED) -- there is no - max bound to validate against in that case, and writing - board/cur_node_power_limit while NPM is disabled has no defined effect. - Same fail-fast pattern as the max-bound check above: amdsmi_set_npm_limit() - itself already enforces this too, so this is a CLI-friendly early exit, - not the sole enforcement. - - Fails closed when the platform max is unavailable ("N/A"): rather than - allowing any positive value through in that degraded-driver scenario, the - request is rejected outright here too, matching amdsmi_set_npm_limit()'s - own fail-closed behavior for the same condition. + """Validate and set the NPM node power limit, rejecting before the write when out of range. Args: node_handle: Node handle obtained via amdsmi_get_node_handle() @@ -3672,29 +3623,25 @@ def validate_and_set_node_power_limit(self, node_handle, requested_limit, logger if npm_info["status"] == amdsmi_interface.amdsmi_wrapper.AMDSMI_NPM_STATUS_DISABLED: raise amdsmi_cli_exceptions.AmdSmiInvalidParameterValueException( - sys.argv[1] if len(sys.argv) > 1 else "unknown", + "set", f"{requested_limit}", self.get_output_format(), - hint="Node power limit cannot be set: NPM is disabled on this node", + hint="--node-power-limit cannot be set: NPM is disabled on this node", ) elif max_node_power_limit == "N/A": - # Fail closed: the platform max is unreadable (degraded driver / - # unavailable sysfs), so there is no bound to validate the - # request against. Reject rather than let an unbounded value - # through. + # Fail closed: without a platform max there is no bound to check against. raise amdsmi_cli_exceptions.AmdSmiInvalidParameterValueException( - sys.argv[1] if len(sys.argv) > 1 else "unknown", + "set", f"{requested_limit}", self.get_output_format(), - hint="Node power limit cannot be validated: platform maximum is unavailable", + hint="--node-power-limit cannot be validated: platform maximum is unavailable", ) elif not 0 < requested_limit <= max_node_power_limit: - # Raise so the caller exits with a non-zero return code raise amdsmi_cli_exceptions.AmdSmiInvalidParameterValueException( - sys.argv[1] if len(sys.argv) > 1 else "unknown", + "set", f"{requested_limit}", self.get_output_format(), - hint=f"Node power limit must be between 1W and {max_node_power_limit}W", + hint=f"--node-power-limit must be between 1W and {max_node_power_limit}W", ) amdsmi_interface.amdsmi_set_npm_limit(node_handle, requested_limit) @@ -3709,15 +3656,15 @@ def validate_and_set_node_power_limit(self, node_handle, requested_limit, logger if e.get_error_code() == amdsmi_interface.amdsmi_wrapper.AMDSMI_STATUS_NO_PERM: raise PermissionError("Command requires elevation") from e if e.get_error_code() == amdsmi_interface.amdsmi_wrapper.AMDSMI_STATUS_INVAL: - # Raise so the caller exits with a non-zero return code, same as - # the CLI-side bound checks above. + # Same exit code as the CLI-side range checks above. raise amdsmi_cli_exceptions.AmdSmiInvalidParameterValueException( - sys.argv[1] if len(sys.argv) > 1 else "unknown", + "set", f"{requested_limit}", self.get_output_format(), - hint=e.get_error_info(detailed=False), + hint=f"--node-power-limit rejected: {e.get_error_info(detailed=False)}", ) from e error_msg = f"[{e.get_error_info(detailed=False)}] Unable to set node power limit to {requested_limit} W" + self.error_collector.record_library_error(e.get_error_code()) if logger.is_json_format() or logger.is_csv_format(): return { "status": "error", diff --git a/projects/amdsmi/amdsmi_cli/amdsmi_parser.py b/projects/amdsmi/amdsmi_cli/amdsmi_parser.py index 2ad87b63848d..ac351d9d607a 100644 --- a/projects/amdsmi/amdsmi_cli/amdsmi_parser.py +++ b/projects/amdsmi/amdsmi_cli/amdsmi_parser.py @@ -2690,8 +2690,7 @@ def _add_set_value_parser(self, subparsers: argparse._SubParsersAction, func): ) # Node power limit is enabled on guest (1VF), maintain order - max_node_power_limit = self.helpers.get_max_node_power_limit() - set_node_power_limit_help = f"Set the node-level (NPM) power limit in watts.\n\tThis is a node-wide setting, not per-GPU.\n\tMax node power limit: {max_node_power_limit}" + set_node_power_limit_help = "Set the node-level (NPM) power limit in watts.\n\tThis is a node-wide setting, not per-GPU." set_value_exclusive_group.add_argument( "-n", "--node-power-limit", @@ -2919,15 +2918,6 @@ def _add_set_value_parser(self, subparsers: argparse._SubParsersAction, func): gtt_flags=("--node-balancing-mode",), reason="--node-balancing-mode is a system-wide setting, not per-GPU", ) - # Improve the --gpu error message if combined with --node-power-limit; - # actual rejection happens at runtime in set_value.py, since these two - # flags sit in separate argparse groups and argparse itself never - # raises for this combination. - self._guard_gtt_gpu_conflict( - set_value_parser, - gtt_flags=("--node-power-limit", "-n"), - reason="--node-power-limit is a node-wide setting, not per-GPU", - ) # Set accepts default devices of all self._add_device_arguments(set_value_parser, required=False) diff --git a/projects/amdsmi/amdsmi_cli/subcommands/node.py b/projects/amdsmi/amdsmi_cli/subcommands/node.py index 2f9ebc780972..ac07a0955334 100644 --- a/projects/amdsmi/amdsmi_cli/subcommands/node.py +++ b/projects/amdsmi/amdsmi_cli/subcommands/node.py @@ -226,10 +226,10 @@ def node( print("\n".join(node_output)) else: if self.logger.is_csv_format(): - # Only node 0 is currently supported; mirror the gpu/cpu/nic - # CSV surfaces, which always lead with a device identifier. - csv_dict = {"node": 0} + csv_dict = {} if args.power_management: + # Only node 0 exists today; the NPM design shows this column for -p output only. + csv_dict["node"] = 0 csv_dict["limit"] = npm_dict.get("limit", "N/A") csv_dict["status"] = npm_dict.get("status", "N/A") csv_dict["threshold"] = npm_dict.get("threshold", "N/A") diff --git a/projects/amdsmi/amdsmi_cli/subcommands/set_value.py b/projects/amdsmi/amdsmi_cli/subcommands/set_value.py index f40a0bc1ee7f..fed7b57e83c1 100644 --- a/projects/amdsmi/amdsmi_cli/subcommands/set_value.py +++ b/projects/amdsmi/amdsmi_cli/subcommands/set_value.py @@ -2130,7 +2130,7 @@ def set_value( process_isolation (int, optional): Value override for args.process_isolation. Defaults to None. Raises: AmdSmiRequiredCommandException: If no device target or argument is provided - AmdSmiInvalidParameterException: If GPU/CPU/CORE arguments are combined or --gtt/--node-balancing-mode is misused + AmdSmiInvalidParameterException: If GPU/CPU/CORE arguments are combined or --gtt/--node-balancing-mode/--node-power-limit is misused PermissionError: If a set operation requires elevation (AMDSMI_STATUS_NO_PERM) Return: @@ -2201,18 +2201,22 @@ def set_value( # Special node power limit handling (node-wide, not per-GPU) — handle before device dispatch if hasattr(args, "node_power_limit") and args.node_power_limit is not None: if hasattr(args, "gpu") and args.gpu is not None: - print( + msg = ( "amd-smi set: error: argument --node-power-limit/-n: not allowed with " - "argument --gpu/-g (--node-power-limit is a node-wide setting, not per-GPU)", - file=sys.stderr, + "argument --gpu/-g (--node-power-limit is a node-wide setting, not per-GPU)" + ) + raise AmdSmiInvalidParameterException( + "set", "--node-power-limit", self.helpers.get_output_format(), msg ) - sys.exit(2) requested_limit = args.node_power_limit if self.node_handle is None: self.logger.output["set_node_power_limit"] = ( "[AMDSMI_STATUS_NOT_SUPPORTED - Feature not supported] " "Unable to set node power limit: no NPM-capable node found" ) + self.helpers.error_collector.record_library_error( + amdsmi_interface.amdsmi_wrapper.AMDSMI_STATUS_NOT_SUPPORTED + ) self.logger.print_output() return result = self.helpers.validate_and_set_node_power_limit( diff --git a/projects/amdsmi/docs/how-to/amdsmi-cli-tool.md b/projects/amdsmi/docs/how-to/amdsmi-cli-tool.md index 7a98e9b8bfaa..9948e3d0e1e4 100644 --- a/projects/amdsmi/docs/how-to/amdsmi-cli-tool.md +++ b/projects/amdsmi/docs/how-to/amdsmi-cli-tool.md @@ -721,7 +721,6 @@ Set Arguments: This is a system-wide setting, not per-GPU. -n, --node-power-limit WATTS Set the node-level (NPM) power limit in watts. This is a node-wide setting, not per-GPU. - Max node power limit: 6000 W CPU Arguments: --cpu-pwr-limit PWR_LIMIT Set power limit for the given socket. Input parameter is power limit value. diff --git a/projects/amdsmi/docs/reference/amdsmi-py-api.md b/projects/amdsmi/docs/reference/amdsmi-py-api.md index 04d4fbe9feef..06c081b85395 100644 --- a/projects/amdsmi/docs/reference/amdsmi-py-api.md +++ b/projects/amdsmi/docs/reference/amdsmi-py-api.md @@ -3102,43 +3102,19 @@ finally: ### amdsmi_set_npm_limit -Description: Set the NPM (Node Power Management) power limit for the node -associated with `node_handle` by writing to the board's -`cur_node_power_limit` sysfs interface. - -This function rejects the request with `AmdSmiLibraryException` -(`AMDSMI_STATUS_INVAL`) if NPM is disabled on the node -(`amdsmi_get_npm_info()`'s `status` == `AMDSMI_NPM_STATUS_DISABLED`), since -writing `board/cur_node_power_limit` while NPM is disabled has no defined -effect. It also validates `limit` against the platform max bound -(`amdsmi_get_npm_info()`'s `max_node_power_limit`, sourced from -`board/max_node_power_limit`) internally before ever issuing the write, -raising `AmdSmiLibraryException` with `AMDSMI_STATUS_INVAL` if `limit` is `0` -or greater than that bound. If the platform max bound itself cannot be read -(e.g. the sysfs interface is missing or returns unexpected data), this -function fails closed and raises that underlying error rather than silently -allowing an unbounded `limit` through. The amd-smi CLI's -`set --node-power-limit` additionally performs the same checks itself ahead -of calling this function, purely to fail fast and present a friendlier, -earlier user-facing error message; it is not the only validation and is not -required for correctness. +Description: Sets the NPM (Node Power Management) power limit, in watts, for the +node associated with `node_handle`. Requires root. The library rejects the +request without writing if NPM is disabled or `limit` exceeds +`max_node_power_limit` from `amdsmi_get_npm_info`. Input parameters: * `node_handle` node handle obtained from `amdsmi_get_node_handle` -* `limit` new NPM power limit value to request (units match the - `board/cur_node_power_limit` sysfs interface). Must satisfy - `0 < limit <= UINT64_MAX`; out-of-range values raise - `AmdSmiParameterException` rather than silently wrapping modulo 2**64 the - way a raw `ctypes.c_uint64()` conversion would. Values that pass this local - bound check but are `0` or exceed the platform max are rejected by the - underlying library call instead (see Exceptions below) - -Output: None. This function raises an exception if the call did not succeed -(e.g. `AmdSmiLibraryException` with `AMDSMI_STATUS_INVAL` if `limit` is `0` or -exceeds the platform max bound, `AMDSMI_STATUS_NOT_SUPPORTED` if the sysfs -interface is unavailable, or `AMDSMI_STATUS_NO_PERM` if the write was -rejected) +* `limit` new node power limit in watts. Must be an `int` (not `bool`) in + `1..2**64-1`; otherwise `AmdSmiParameterException` is raised before the + library is called (for example, for a `limit` of `0`) + +Output: None Exceptions that can be thrown by `amdsmi_set_npm_limit` function: @@ -3147,12 +3123,13 @@ Exceptions that can be thrown by `amdsmi_set_npm_limit` function: #### Possible Library Exceptions -- `AMDSMI_STATUS_INVAL` - Invalid parameters, including when `limit` is `0` - or exceeds the platform max bound (or when that bound itself cannot be - read) -- `AMDSMI_STATUS_NOT_SUPPORTED` - Feature not supported -- `AMDSMI_STATUS_NO_PERM` - Permission Denied (e.g. the write was rejected by - the driver for this guest context) +- `AMDSMI_STATUS_INVAL` - Invalid parameters, NPM is disabled, or `limit` + exceeds `max_node_power_limit` +- `AMDSMI_STATUS_NO_PERM` - Not root, or the driver rejected the write (for + example, from a guest that is not permitted to set it) +- `AMDSMI_STATUS_NOT_SUPPORTED` - NPM sysfs interface is missing +- `AMDSMI_STATUS_UNEXPECTED_DATA` - NPM sysfs content is unparsable or the + platform max is unavailable Example: diff --git a/projects/amdsmi/include/amd_smi/amdsmi.h b/projects/amdsmi/include/amd_smi/amdsmi.h index ca7a8faf6c17..92807a3fde66 100644 --- a/projects/amdsmi/include/amd_smi/amdsmi.h +++ b/projects/amdsmi/include/amd_smi/amdsmi.h @@ -7805,20 +7805,16 @@ amdsmi_status_t amdsmi_get_gpu_xcd_counter(amdsmi_processor_handle processor_han * * @details This function queries the NPM controller for the given node and returns whether NPM is * enabled, along with the current node-level power limit in Watts. The NPM status and limit are set - * out-of-band and reported via this API. The returned amdsmi_npm_info_t::max_node_power_limit is - * the platform max bound (sourced from board/max_node_power_limit) that ::amdsmi_set_npm_limit - * itself validates requests against internally before issuing the write (mirroring - * ::amdsmi_set_power_cap and ::amdsmi_get_power_cap_info); callers may still query it here ahead - * of time to fail fast / present a clean error without invoking the setter. The returned - * amdsmi_npm_info_t::current_node_power is the current (instantaneous) node-level power reading in - * Watts (sourced from board/node_power); it is node-handle scoped (queried once per node), unlike - * amdsmi_power_info_t, which is scoped per GPU handle. + * out-of-band and reported via this API. amdsmi_npm_info_t::max_node_power_limit is the upper bound + * ::amdsmi_set_npm_limit accepts; amdsmi_npm_info_t::current_node_power is the instantaneous + * node power in Watts, scoped to the node rather than a single GPU. * * @param[in] node_handle Handle to the Node to query. * @param[out] info Pointer to amdsmi_npm_info_t structure to receive NPM status and limit. * Must be allocated by the user. * - * @return ::AMDSMI_STATUS_SUCCESS on success, non-zero on failure. + * @return ::AMDSMI_STATUS_SUCCESS on success, ::AMDSMI_STATUS_INVAL if node_handle is NULL or was + * not returned by ::amdsmi_get_node_handle, non-zero on other failure. */ amdsmi_status_t amdsmi_get_npm_info(amdsmi_node_handle node_handle, amdsmi_npm_info_t* info); @@ -7921,30 +7917,21 @@ amdsmi_status_t amdsmi_get_tray_info(amdsmi_node_handle node_handle, amdsmi_tray * * @platform{gpu_bm_linux} @platform{host} * - * @details This function returns ::AMDSMI_STATUS_INVAL if NPM is disabled on the node - * (amdsmi_npm_info_t::status == ::AMDSMI_NPM_STATUS_DISABLED), since writing - * board/cur_node_power_limit while NPM is disabled has no defined effect. It also validates - * `limit` against the platform max bound (amdsmi_npm_info_t::max_node_power_limit, sourced from - * board/max_node_power_limit) before ever issuing a write, returning ::AMDSMI_STATUS_INVAL if - * `limit` is `0` or greater than that bound (this mirrors ::amdsmi_set_power_cap, whose analogous - * range check against ::amdsmi_get_power_cap_info's bounds is likewise enforced internally rather - * than left purely to the caller). If the platform max bound itself cannot be read (e.g. the - * sysfs interface is missing or returns unexpected data), this function fails closed and returns - * that underlying error rather than silently allowing an unbounded `limit` through. Once - * validated, this function issues a Set NPM Limit request to GPU PMFW via the amdgpu driver for - * the given node, requesting the value of `limit` verbatim. The write path is restricted to host - * / 1 permitted VM only; the enforcement of that restriction is done by the kernel driver, not by - * this function. When both SMI and BMC set limits, GPU PMFW arbitrates by taking the minimum of - * the two, bounded by the platform max. + * @details Requires root. The request is rejected without writing if NPM is disabled on the node, + * or if `limit` is `0` or above amdsmi_npm_info_t::max_node_power_limit. In a guest, only the + * permitted VM may write; the driver enforces this. GPU PMFW applies the lower of the SMI and BMC + * limits. * * @param[in] node_handle Handle to the Node to set the limit on. * @param[in] limit Node-level power limit in Watts to request. * - * @return ::AMDSMI_STATUS_SUCCESS on success. ::AMDSMI_STATUS_INVAL if NPM is disabled on this - * node, or if `limit` is `0` or exceeds the platform max bound. ::AMDSMI_STATUS_NOT_SUPPORTED if - * the sysfs interface (board/npm_status, board/cur_node_power_limit, or - * board/max_node_power_limit) is unavailable on this platform. Non-zero on other failure (e.g. - * ::AMDSMI_STATUS_NO_PERM if the driver rejects the write for this guest context). + * @return + * - ::AMDSMI_STATUS_SUCCESS on success. + * - ::AMDSMI_STATUS_INVAL if node_handle is invalid, NPM is disabled, or `limit` is out of range. + * - ::AMDSMI_STATUS_NO_PERM if the caller is not root, or the driver rejected the write. + * - ::AMDSMI_STATUS_NOT_SUPPORTED if the NPM sysfs interface is missing. + * - ::AMDSMI_STATUS_UNEXPECTED_DATA if NPM sysfs content is unparsable or the platform max is + * unavailable. */ amdsmi_status_t amdsmi_set_npm_limit(amdsmi_node_handle node_handle, uint64_t limit); diff --git a/projects/amdsmi/include/amd_smi/impl/amd_smi_test_internal.h b/projects/amdsmi/include/amd_smi/impl/amd_smi_test_internal.h index 0892df8942a7..9f99ad1a9a5d 100644 --- a/projects/amdsmi/include/amd_smi/impl/amd_smi_test_internal.h +++ b/projects/amdsmi/include/amd_smi/impl/amd_smi_test_internal.h @@ -4,6 +4,8 @@ #ifndef AMD_SMI_INCLUDE_AMD_SMI_TEST_INTERNAL_H_ #define AMD_SMI_INCLUDE_AMD_SMI_TEST_INTERNAL_H_ +#include + #include "amd_smi/amdsmi.h" #include "amd_smi/impl/amd_smi_test_flags.h" @@ -12,22 +14,8 @@ // extern-declare the rsmi_status_t function directly. amdsmi_status_t amdsmi_test_sleep(amdsmi_processor_handle processor_handle, uint32_t seconds); -// Internal test-only hook: registers |node_handle| as a valid handle in the -// same node-handle registry that amdsmi_get_npm_info()/amdsmi_set_npm_limit() -// consult (via is_registered_node_handle()) before dereferencing their -// node_handle argument. In production, that registry is only ever populated -// by amdsmi_get_node_handle(); unit tests that fabricate a raw node_handle -// directly (bypassing amdsmi_get_node_handle(), whose oam_id == 0 gate is not -// satisfiable in the test environment) must call this first so the -// registered-handle check does not reject their forged-but-otherwise-valid -// handle. Not declared in amdsmi.h -- internal use via -// amd_smi_test_internal.h only. -// Unlike the other hooks in this header, this one lets a caller mark an -// arbitrary pointer as a "valid" node handle, so it is compiled only for -// BUILD_TESTS=ON builds (see src/CMakeLists.txt) rather than shipped in -// production builds. -#ifdef BUILD_TESTS -amdsmi_status_t amdsmi_test_register_node_handle(amdsmi_node_handle node_handle); -#endif // BUILD_TESTS +// Internal test-only: returns a registered node handle for an arbitrary board directory. +amdsmi_status_t amdsmi_test_get_node_handle(const std::string& board_path, + amdsmi_node_handle* node_handle); #endif // AMD_SMI_INCLUDE_AMD_SMI_TEST_INTERNAL_H_ diff --git a/projects/amdsmi/py-interface/amdsmi_interface.py b/projects/amdsmi/py-interface/amdsmi_interface.py index 9b8fd53e6586..d7507cba563d 100644 --- a/projects/amdsmi/py-interface/amdsmi_interface.py +++ b/projects/amdsmi/py-interface/amdsmi_interface.py @@ -5497,28 +5497,20 @@ def amdsmi_get_node_handle(processor_handle): return node_handle -def amdsmi_get_npm_info(node_handle: processor_handle_t) -> Dict[str, Any]: +def amdsmi_get_npm_info(node_handle: amdsmi_wrapper.amdsmi_node_handle) -> Dict[str, Any]: """ - Get NPM (Node Power Management) status and power limit info for the node - associated with node_handle. + Get NPM (Node Power Management) info for the node associated with node_handle. Parameters: - node_handle(node_handle_t): Node handle obtained via - amdsmi_get_node_handle() + node_handle(amdsmi_node_handle): Node handle obtained via amdsmi_get_node_handle() Returns: `Dict[str, Any]`: Dictionary with keys: - ``limit`` (`int`): Current NPM power limit in Watts. - - ``status`` (`int`): NPM status (enabled/disabled), see - ``amdsmi_npm_status_t``. - - ``ubb_power_threshold`` (`int`): UBB node power threshold in - Watts. - - ``max_node_power_limit`` (`int`): The platform max bound - consumed by amdsmi_set_npm_limit(): callers should ensure any - limit passed to that function does not exceed this value. - - ``current_node_power`` (`int`): The current (instantaneous) node - power in Watts (board/node_power), MI450+. Queried once per - node rather than once per GPU. + - ``status`` (`int`): NPM status, see ``amdsmi_npm_status_t``. + - ``ubb_power_threshold`` (`int`): UBB node power threshold in Watts. + - ``max_node_power_limit`` (`int`): Upper bound for amdsmi_set_npm_limit(), in Watts. + - ``current_node_power`` (`int`): Instantaneous node power in Watts. Raises: AmdSmiParameterException: If the node handle is invalid. @@ -5630,64 +5622,30 @@ def amdsmi_get_tray_info( } -def amdsmi_set_npm_limit(node_handle: processor_handle_t, limit: int) -> None: +def amdsmi_set_npm_limit(node_handle: amdsmi_wrapper.amdsmi_node_handle, limit: int) -> None: """ Set the NPM (Node Power Management) power limit for the node associated - with node_handle by writing to the board's cur_node_power_limit sysfs - interface. - - This function rejects the request with AmdSmiLibraryException - (AMDSMI_STATUS_INVAL) if NPM is disabled on the node - (amdsmi_get_npm_info()'s "status" == AMDSMI_NPM_STATUS_DISABLED), since - writing board/cur_node_power_limit while NPM is disabled has no defined - effect. It also validates `limit` against the platform max bound - (amdsmi_get_npm_info()'s "max_node_power_limit", sourced from - board/max_node_power_limit) internally before ever issuing the write, - raising AmdSmiLibraryException with AMDSMI_STATUS_INVAL if `limit` is `0` - or greater than that bound. If the platform max bound itself cannot be - read (e.g. the sysfs interface is missing or returns unexpected data), - this function fails closed and raises that underlying error rather than - silently allowing an unbounded `limit` through. The amd-smi CLI's - `set --node-power-limit` additionally performs the same checks itself - ahead of calling this function, purely to fail fast and present a - friendlier, earlier user-facing error message; it is not the only - validation and is not required for correctness. + with node_handle. Requires root. Parameters: - node_handle(node_handle_t): Node handle obtained via - amdsmi_get_node_handle() - limit(int): New NPM power limit value to request (units match the - board/cur_node_power_limit sysfs interface). Must satisfy - 0 < limit <= UINT64_MAX; out-of-range values raise - AmdSmiParameterException rather than silently wrapping modulo - 2**64 the way a raw ctypes.c_uint64() conversion would. Values - that pass this local bound check but are `0` or exceed the - platform max are rejected by the underlying library call instead - (see Raises below). - - Returns: - None: This function raises an exception if the call did not succeed - (e.g. AmdSmiLibraryException with AMDSMI_STATUS_INVAL if `limit` is - `0` or exceeds the platform max bound, AMDSMI_STATUS_NOT_SUPPORTED if - the sysfs interface is unavailable, or AMDSMI_STATUS_NO_PERM if the - write was rejected) + node_handle(amdsmi_node_handle): Node handle obtained via amdsmi_get_node_handle() + limit(int): New node power limit in Watts, 1..2**64-1 Raises: - AmdSmiParameterException: If `node_handle` or `limit` is the wrong - type, or `limit` is outside `0 < limit <= UINT64_MAX`. - AmdSmiLibraryException: If the underlying library call fails, - including AMDSMI_STATUS_INVAL when `limit` is `0` or exceeds the - platform max bound (or when that bound itself cannot be read). + AmdSmiParameterException: Raised before calling the library if node_handle + is the wrong type, or limit is not an int (bool is rejected) in 1..2**64-1. + AmdSmiLibraryException: AMDSMI_STATUS_INVAL if NPM is disabled or limit exceeds + max_node_power_limit; AMDSMI_STATUS_NO_PERM if not root or the driver rejects + the write; AMDSMI_STATUS_NOT_SUPPORTED if the NPM interface is missing; + AMDSMI_STATUS_UNEXPECTED_DATA if NPM sysfs content is unparsable or the + platform max is unavailable. """ if not isinstance(node_handle, amdsmi_wrapper.amdsmi_node_handle): raise AmdSmiParameterException(node_handle, amdsmi_wrapper.amdsmi_node_handle) - if not isinstance(limit, int): + if isinstance(limit, bool) or not isinstance(limit, int): raise AmdSmiParameterException(limit, int) if not 0 < limit <= 0xFFFFFFFFFFFFFFFF: - # ctypes.c_uint64() silently wraps out-of-range values modulo 2**64 - # instead of raising (e.g. ctypes.c_uint64(2**64 + 12345).value == - # 12345); reject explicitly here so a bad value is never wrapped into - # an unrelated one and written to hardware. + # ctypes.c_uint64() would silently wrap an out-of-range value modulo 2**64. raise AmdSmiParameterException( limit, int, msg=f"limit must satisfy 0 < limit <= {0xFFFFFFFFFFFFFFFF} (got {limit})" ) diff --git a/projects/amdsmi/rocm_smi/include/rocm_smi/rocm_smi.h b/projects/amdsmi/rocm_smi/include/rocm_smi/rocm_smi.h index 24bad84a36ef..5d53f4619d63 100644 --- a/projects/amdsmi/rocm_smi/include/rocm_smi/rocm_smi.h +++ b/projects/amdsmi/rocm_smi/include/rocm_smi/rocm_smi.h @@ -628,8 +628,8 @@ typedef struct { //!< (board/max_node_power_limit). uint32_t current_node_power; //!< The current (instantaneous) node power in Watts //!< (board/node_power). - uint64_t reserved[3]; //!< Reduced from reserved[4] to accommodate - //!< current_node_power. + uint64_t reserved[3]; //!< Reduced from reserved[5] to accommodate + //!< max_node_power_limit and current_node_power. } rsmi_npm_info_t; /** diff --git a/projects/amdsmi/rocm_smi/include/rocm_smi/rocm_smi_npm.h b/projects/amdsmi/rocm_smi/include/rocm_smi/rocm_smi_npm.h index da27baa8579e..3005266fcf85 100644 --- a/projects/amdsmi/rocm_smi/include/rocm_smi/rocm_smi_npm.h +++ b/projects/amdsmi/rocm_smi/include/rocm_smi/rocm_smi_npm.h @@ -36,6 +36,9 @@ rsmi_status_t set_npm_board_mode(const std::string& board_path, const std::strin // Resolved via resolve_npm_dir(), same fallback as get_npm_board_status(). rsmi_status_t get_npm_board_max_limit(const std::string& board_path, uint64_t* limit); +// Checks NPM is enabled and 1 <= limit <= platform max, without writing. +rsmi_status_t validate_npm_board_limit(const std::string& board_path, uint64_t limit); + // NPM board limit set (cur_node_power_limit: Set NPM Limit request to GPU // PMFW via amdgpu driver). Resolved via resolve_npm_dir(), same fallback as // get_npm_board_status(). diff --git a/projects/amdsmi/rocm_smi/src/rocm_smi.cc b/projects/amdsmi/rocm_smi/src/rocm_smi.cc index af8ce8030af1..331118904a0d 100644 --- a/projects/amdsmi/rocm_smi/src/rocm_smi.cc +++ b/projects/amdsmi/rocm_smi/src/rocm_smi.cc @@ -3256,9 +3256,7 @@ rsmi_status_t rsmi_dev_npm_info_get(uint32_t dv_ind, uintptr_t node_handle, rsmi_status_t ubb_status = amd::smi::get_ubb_power_limit(*board_path_str, &ubb_power_threshold_raw); - // Get platform max node power limit (optional - don't fail if not available). - // This is descriptive metadata (used by callers, e.g. amd-smi CLI, to bound - // requests to rsmi_dev_npm_limit_set()), not on the critical read path. + // Get platform max node power limit (optional - don't fail if not available) uint64_t npm_max_limit = UINT64_MAX; rsmi_status_t max_limit_status = amd::smi::get_npm_board_max_limit(*board_path_str, &npm_max_limit); @@ -3266,11 +3264,9 @@ rsmi_status_t rsmi_dev_npm_info_get(uint32_t dv_ind, uintptr_t node_handle, ss << __PRETTY_FUNCTION__ << " | get_npm_board_max_limit returned " << getRSMIStatusString(max_limit_status) << " ; using sentinel max limit"; LOG_DEBUG(ss); - npm_max_limit = UINT64_MAX; } - // Get current node power (optional - don't fail if not available). This is - // descriptive telemetry, not on the critical read path. + // Get current node power (optional - don't fail if not available) uint64_t node_power_raw = UINT64_MAX; rsmi_status_t node_power_status = amd::smi::get_npm_node_power(*board_path_str, &node_power_raw); if (node_power_status != RSMI_STATUS_SUCCESS) { @@ -3460,53 +3456,18 @@ rsmi_status_t rsmi_dev_npm_limit_set(uint32_t dv_ind, uintptr_t node_handle, uin return RSMI_STATUS_INVALID_ARGS; } - // Reject the write outright when NPM is disabled on this node: writing - // board/cur_node_power_limit has no defined effect in that state. This is - // the authoritative check -- the CLI's own pre-check (amdsmi_helpers.py) - // is a fail-fast convenience for that one caller, not a substitute for - // enforcing this here for every caller of rsmi_dev_npm_limit_set(). - bool npm_enabled = false; - rsmi_status_t status_ret = amd::smi::get_npm_board_status(*board_path_str, &npm_enabled); - if (status_ret != RSMI_STATUS_SUCCESS) { - ss << __PRETTY_FUNCTION__ - << " | get_npm_board_status failed: " << getRSMIStatusString(status_ret, false) - << " -> rejecting write (fail closed)"; - LOG_ERROR(ss); - return status_ret; - } - if (!npm_enabled) { - ss << __PRETTY_FUNCTION__ << " | NPM disabled on this node -> returning " - << getRSMIStatusString(RSMI_STATUS_INVALID_ARGS); - LOG_ERROR(ss); - return RSMI_STATUS_INVALID_ARGS; + // Enforced here for every caller; the CLI pre-check is only a fail-fast convenience. + rsmi_status_t ret = amd::smi::validate_npm_board_limit(*board_path_str, limit); + if (ret == RSMI_STATUS_SUCCESS) { + ret = amd::smi::set_npm_board_limit(*board_path_str, limit); } - - // Mirror rsmi_dev_power_cap_set(): query the platform bound before ever - // touching sysfs, and reject out-of-range requests with - // RSMI_STATUS_INVALID_ARGS. Fail closed if the bound itself can't be - // read (e.g. sysfs missing/unexpected contents) -- propagate that error - // and do not fall through to the write, rather than silently allowing an - // unbounded value through, consistent with the CLI layer's own - // fail-closed handling of an unreadable platform max. - uint64_t max_limit = 0; - rsmi_status_t ret = amd::smi::get_npm_board_max_limit(*board_path_str, &max_limit); if (ret != RSMI_STATUS_SUCCESS) { - ss << __PRETTY_FUNCTION__ - << " | get_npm_board_max_limit failed: " << getRSMIStatusString(ret, false) - << " -> rejecting write (fail closed)"; + ss << __PRETTY_FUNCTION__ << " | limit=" << limit << " -> returning " + << getRSMIStatusString(ret, false); LOG_ERROR(ss); return ret; } - if (limit == 0 || limit > max_limit) { - ss << __PRETTY_FUNCTION__ << " | limit=" << limit << " out of range (valid range is " - << "1.." << max_limit << ") -> returning " << getRSMIStatusString(RSMI_STATUS_INVALID_ARGS); - LOG_ERROR(ss); - return RSMI_STATUS_INVALID_ARGS; - } - - ret = amd::smi::set_npm_board_limit(*board_path_str, limit); - ss << __PRETTY_FUNCTION__ << " | ======= end ======= | returning " << getRSMIStatusString(ret, false); LOG_TRACE(ss); diff --git a/projects/amdsmi/rocm_smi/src/rocm_smi_npm.cc b/projects/amdsmi/rocm_smi/src/rocm_smi_npm.cc index 14f3db0c746a..c7ca23471951 100644 --- a/projects/amdsmi/rocm_smi/src/rocm_smi_npm.cc +++ b/projects/amdsmi/rocm_smi/src/rocm_smi_npm.cc @@ -3,7 +3,9 @@ #include "rocm_smi/rocm_smi_npm.h" -#include +#include +#include + #include #include #include @@ -149,19 +151,6 @@ static rsmi_status_t read_board_uint64(const std::string& board_path, const char rsmi_status_t r = read_npm_file(p, s); if (r != RSMI_STATUS_SUCCESS) return RSMI_STATUS_NOT_SUPPORTED; - // std::stoull() accepts a leading '-' and silently negates modulo 2^64 - // (e.g. "-1" successfully parses as UINT64_MAX with idx == s.size()), - // which is well-defined C++ behavior but would let corrupted/negative - // sysfs content masquerade as a huge-but-"valid" unsigned value instead of - // failing to parse. Reject anything that doesn't start with a digit - // before ever calling std::stoull() so negative/garbage content is always - // treated as a parse error. - // Behavior change: negative/malformed sysfs content previously parsed - // successfully as UINT64_MAX; it now returns RSMI_STATUS_UNEXPECTED_DATA. - if (s.empty() || !std::isdigit(static_cast(s[0]))) { - return RSMI_STATUS_UNEXPECTED_DATA; - } - try { size_t idx = 0; unsigned long long v = std::stoull(s, &idx, 10); @@ -183,6 +172,22 @@ rsmi_status_t get_npm_board_max_limit(const std::string& board_path, uint64_t* l return read_board_uint64(board_path, "max_node_power_limit", limit); } +rsmi_status_t validate_npm_board_limit(const std::string& board_path, uint64_t limit) { + bool enabled = false; + rsmi_status_t ret = get_npm_board_status(board_path, &enabled); + if (ret != RSMI_STATUS_SUCCESS) return ret; + if (!enabled) return RSMI_STATUS_INVALID_ARGS; + + uint64_t max_limit = 0; + ret = get_npm_board_max_limit(board_path, &max_limit); + if (ret != RSMI_STATUS_SUCCESS) return ret; + // UINT64_MAX means the platform max is unavailable (e.g. "-1"), not an unbounded limit. + if (max_limit == UINT64_MAX) return RSMI_STATUS_UNEXPECTED_DATA; + + if (limit == 0 || limit > max_limit) return RSMI_STATUS_INVALID_ARGS; + return RSMI_STATUS_SUCCESS; +} + rsmi_status_t set_npm_board_limit(const std::string& board_path, uint64_t limit) { if (board_path.empty()) return RSMI_STATUS_INVALID_ARGS; @@ -192,16 +197,25 @@ rsmi_status_t set_npm_board_limit(const std::string& board_path, uint64_t limit) fs::path p = resolve_npm_dir(bd) / "cur_node_power_limit"; if (!fs::exists(p) || !fs::is_regular_file(p)) return RSMI_STATUS_NOT_SUPPORTED; - // Write the numeric limit value as text to the sysfs file. This mirrors the - // WriteSysfsStr()-based pattern used elsewhere in rocm_smi for other - // write-capable attributes (e.g. power cap set path), which in turn - // triggers a Set NPM Limit request to GPU PMFW via the amdgpu driver. - int ret = WriteSysfsStr(p.string(), std::to_string(limit)); - // If the sysfs file doesn't exist, treat as not supported. - if (ret == ENOENT) { - return RSMI_STATUS_NOT_SUPPORTED; + // Not WriteSysfsStr(): it reports every post-open failure as ENOENT, hiding the driver's EPERM. + const std::string val = std::to_string(limit); + int err = 0; + int fd = open(p.c_str(), O_WRONLY | O_TRUNC | O_CLOEXEC); + if (fd < 0) { + err = errno; + } else { + ssize_t n = write(fd, val.data(), val.size()); + if (n < 0) { + err = errno; + } else if (static_cast(n) != val.size()) { + err = EIO; + } + close(fd); } - return ErrnoToRsmiStatus(ret); + + // EPERM here is the driver rejecting a non-permitted guest, not a missing feature. + if (err == EPERM) return RSMI_STATUS_PERMISSION; + return ErrnoToRsmiStatus(err); } rsmi_status_t get_ubb_power_limit(const std::string& board_path, uint64_t* limit) { diff --git a/projects/amdsmi/src/CMakeLists.txt b/projects/amdsmi/src/CMakeLists.txt index 7107e74d47ba..bd01bef0818e 100644 --- a/projects/amdsmi/src/CMakeLists.txt +++ b/projects/amdsmi/src/CMakeLists.txt @@ -216,12 +216,6 @@ function(amdsmi_configure_common_target _TARGET) target_include_directories(${_TARGET} PRIVATE ${AMDCUID_INCLUDE_DIR}) endif() - # Compiles in test-only escape hatches (e.g. amdsmi_test_register_node_handle()) - # that the GTest suite needs but that must not ship in production builds. - if(BUILD_TESTS) - target_compile_definitions(${_TARGET} PRIVATE BUILD_TESTS) - endif() - target_link_directories(${_TARGET} PRIVATE ${CMAKE_CURRENT_BINARY_DIR}/nic/ai-nic/amdsmi_unified/build/) endfunction() diff --git a/projects/amdsmi/src/amd_smi/amd_smi.cc b/projects/amdsmi/src/amd_smi/amd_smi.cc index fabdcb3006ec..f8f91d270cd4 100644 --- a/projects/amdsmi/src/amd_smi/amd_smi.cc +++ b/projects/amdsmi/src/amd_smi/amd_smi.cc @@ -655,36 +655,29 @@ amdsmi_status_t amdsmi_get_switch_processor_handles(amdsmi_socket_handle socket_ return AMDSMI_STATUS_SUCCESS; } -// Registry of amdsmi_node_handle values that were actually handed out by -// amdsmi_get_node_handle() (or, in unit tests only, injected via -// amdsmi_test_register_node_handle() -- see amd_smi_test_internal.h). An -// amdsmi_node_handle is internally a std::string* to a board sysfs path, but -// callers only ever see it as an opaque void*; a caller can pass any -// garbage-but-nonzero value through the public API. is_registered_node_handle() -// lets consumers of a node_handle (amdsmi_get_npm_info(), -// amdsmi_set_npm_limit()) confirm the pointer is one this library actually -// vended *before* reinterpret_cast'ing and dereferencing it, so an -// unregistered/garbage handle is rejected as AMDSMI_STATUS_INVAL instead of -// being dereferenced as an arbitrary pointer. -// -// g_node_registered_pointers is checked by pointer identity only (no -// dereference of the candidate handle), which is what makes the check safe -// to run on a completely untrusted node_handle value. It is kept separate -// from (but updated alongside) g_node_registry: the latter owns the -// std::string storage for handles amdsmi_get_node_handle() itself allocated, -// while the pointer set also accommodates test-injected handles that this -// library does not own. +// Node handles are registry-owned std::string* board paths; validating by address +// rejects foreign handles without dereferencing them. namespace { std::mutex g_node_mu; std::map> g_node_registry; -std::set g_node_registered_pointers; + +amdsmi_node_handle register_node_board_path(const std::string& board_path) { + std::lock_guard lk(g_node_mu); + auto& owned = g_node_registry[board_path]; + if (!owned) { + owned = std::make_unique(board_path); + } + return reinterpret_cast(owned.get()); +} bool is_registered_node_handle(amdsmi_node_handle node_handle) { if (node_handle == nullptr) { return false; } std::lock_guard lk(g_node_mu); - return g_node_registered_pointers.find(node_handle) != g_node_registered_pointers.end(); + return std::any_of(g_node_registry.begin(), g_node_registry.end(), [node_handle](const auto& e) { + return reinterpret_cast(e.second.get()) == node_handle; + }); } } // namespace @@ -738,50 +731,19 @@ amdsmi_status_t amdsmi_get_node_handle(amdsmi_processor_handle processor_handle, return AMDSMI_STATUS_NOT_SUPPORTED; } - // Store board path so node handle remains valid for library lifetime. - // g_node_mu/g_node_registry/g_node_registered_pointers are file-scope (see - // the anonymous namespace above amdsmi_get_node_handle()) so that - // is_registered_node_handle() can validate a node_handle from any caller, - // not just from within this function. - std::string board_path = found_board.string(); - { - std::lock_guard lk(g_node_mu); - auto it = g_node_registry.find(board_path); - if (it == g_node_registry.end()) { - auto ptr = std::make_unique(board_path); - amdsmi_node_handle h = reinterpret_cast(ptr.get()); - g_node_registry.emplace(board_path, std::move(ptr)); - g_node_registered_pointers.insert(h); - *node_handle = h; - } else { - *node_handle = reinterpret_cast(it->second.get()); - } - } - + *node_handle = register_node_board_path(found_board.string()); return AMDSMI_STATUS_SUCCESS; } -// Test-only wrapper: registers a raw, test-fabricated amdsmi_node_handle as -// valid in the same registry amdsmi_get_node_handle() populates, so that -// is_registered_node_handle() (consulted by amdsmi_get_npm_info() and -// amdsmi_set_npm_limit()) accepts it. Not declared in amdsmi.h -- internal -// use via amd_smi_test_internal.h only. See that header for why this exists: -// unit tests fabricate node_handle values directly (bypassing -// amdsmi_get_node_handle()'s oam_id == 0 gate, which the test environment -// cannot satisfy) and need a way to make that fabricated pointer pass the -// production registered-handle check without weakening the check itself. -// Guarded by BUILD_TESTS so this handle-forging escape hatch never ships in -// a production build (see amd_smi_test_internal.h and src/CMakeLists.txt). -#ifdef BUILD_TESTS -amdsmi_status_t amdsmi_test_register_node_handle(amdsmi_node_handle node_handle) { - if (node_handle == nullptr) { +// Test-only: registers an arbitrary board path, since amdsmi_get_node_handle() needs oam_id == 0. +amdsmi_status_t amdsmi_test_get_node_handle(const std::string& board_path, + amdsmi_node_handle* node_handle) { + if (board_path.empty() || node_handle == nullptr) { return AMDSMI_STATUS_INVAL; } - std::lock_guard lk(g_node_mu); - g_node_registered_pointers.insert(node_handle); + *node_handle = register_node_board_path(board_path); return AMDSMI_STATUS_SUCCESS; } -#endif // BUILD_TESTS amdsmi_status_t amdsmi_get_processor_count_from_handles(amdsmi_processor_handle* processor_handles, uint32_t* processor_count, @@ -1739,9 +1701,8 @@ amdsmi_status_t amdsmi_get_temp_metric(amdsmi_processor_handle processor_handle, return amdsmi_status; } -// amdsmi_get_npm_info() memcpy's rsmi_npm_info_t directly into -// amdsmi_npm_info_t; keep their shared fields byte-compatible so that copy -// stays correct if either struct changes. +// amdsmi_get_npm_info() memcpy's rsmi_npm_info_t into amdsmi_npm_info_t. +static_assert(sizeof(amdsmi_npm_info_t) == 64, "amdsmi_npm_info_t size is part of the ABI"); static_assert(sizeof(amdsmi_npm_info_t) == sizeof(rsmi_npm_info_t), "amdsmi_npm_info_t and rsmi_npm_info_t must stay the same size"); static_assert(offsetof(amdsmi_npm_info_t, status) == offsetof(rsmi_npm_info_t, status), @@ -1784,15 +1745,7 @@ amdsmi_status_t amdsmi_get_npm_info(amdsmi_node_handle node_handle, amdsmi_npm_i return AMDSMI_STATUS_INVAL; } - // Reject any node_handle this library did not itself hand out (via - // amdsmi_get_node_handle(), or a test-registered stand-in -- see - // is_registered_node_handle()) *before* the cast/dereference below: an - // unprivileged caller could otherwise pass an arbitrary non-null value and - // have it dereferenced as a std::string* here, ahead of any privilege - // check further down the call chain. - // Behavior change: previously any non-null node_handle was dereferenced - // here; a caller-supplied garbage handle now fails with - // AMDSMI_STATUS_INVAL instead of crashing. + // Catches caller bugs before the cast below; there is no in-process privilege boundary. if (!is_registered_node_handle(node_handle)) { return AMDSMI_STATUS_INVAL; } @@ -1911,13 +1864,7 @@ amdsmi_status_t amdsmi_set_npm_limit(amdsmi_node_handle node_handle, uint64_t li return AMDSMI_STATUS_INVAL; } - // Reject any node_handle this library did not itself hand out (via - // amdsmi_get_node_handle(), or a test-registered stand-in -- see - // is_registered_node_handle()) *before* the cast/dereference below: an - // unprivileged caller could otherwise pass an arbitrary non-null value and - // have it dereferenced as a std::string* here, ahead of the - // REQUIRE_ROOT_ACCESS privilege check that only exists one layer down in - // rsmi_dev_npm_limit_set(). + // Catches caller bugs before the cast below; there is no in-process privilege boundary. if (!is_registered_node_handle(node_handle)) { return AMDSMI_STATUS_INVAL; } diff --git a/projects/amdsmi/tests/amd_smi_test/CMakeLists.txt b/projects/amdsmi/tests/amd_smi_test/CMakeLists.txt index c5d42b665400..5ab65f95dd0a 100644 --- a/projects/amdsmi/tests/amd_smi_test/CMakeLists.txt +++ b/projects/amdsmi/tests/amd_smi_test/CMakeLists.txt @@ -104,11 +104,6 @@ target_include_directories( # Absolute path to the committed mock CPER fixtures (unit/gpu/mock_cper). target_compile_definitions(${TEST} PRIVATE AMDSMI_TEST_MOCK_DIR="${CMAKE_CURRENT_SOURCE_DIR}/unit/gpu/mock_cper") -# This directory is only added when BUILD_TESTS is ON (see top-level -# CMakeLists.txt), so the test-only amdsmi_test_register_node_handle() -# declaration (amd_smi_test_internal.h) is always needed here. -target_compile_definitions(${TEST} PRIVATE BUILD_TESTS) - if(ENABLE_WSL_BACKEND) target_compile_definitions(${TEST} PRIVATE ENABLE_WSL_BACKEND) target_include_directories(${TEST} PRIVATE ${PROJECT_SOURCE_DIR}/../rocr-runtime/libhsakmt/include) diff --git a/projects/amdsmi/tests/amd_smi_test/functional/gpu/power/power_read_test.cc b/projects/amdsmi/tests/amd_smi_test/functional/gpu/power/power_read_test.cc index 6a9d8e4647c1..03a5b4ebd744 100644 --- a/projects/amdsmi/tests/amd_smi_test/functional/gpu/power/power_read_test.cc +++ b/projects/amdsmi/tests/amd_smi_test/functional/gpu/power/power_read_test.cc @@ -55,6 +55,13 @@ void TestPowerRead::Run(void) { for (uint32_t i = 0; i < num_monitor_devs(); ++i) { PrintDeviceHeader(processor_handles_[i]); + DISPLAY_AMDSMI_API("amdsmi_get_power_info(nullptr)", "gpu=" + std::to_string(i), + VERB(STANDARD)); + err = amdsmi_get_power_info(processor_handles_[i], nullptr); + DISPLAY_AMDSMI_STATUS(VERB(STANDARD), __FILE__, __LINE__, err, AMDSMI_STATUS_INVAL); + // The WSL backend reports NOT_SUPPORTED before checking for a null pointer. + ASSERT_TRUE(err == AMDSMI_STATUS_INVAL || err == AMDSMI_STATUS_NOT_SUPPORTED); + amdsmi_power_cap_info_t info; DISPLAY_AMDSMI_API("amdsmi_get_power_cap_info", "gpu=" + std::to_string(i), VERB(STANDARD)); err = amdsmi_get_power_cap_info(processor_handles_[i], 0, &info); diff --git a/projects/amdsmi/tests/amd_smi_test/unit/gpu/amdsmi_npm_balancing_mode_test.cc b/projects/amdsmi/tests/amd_smi_test/unit/gpu/amdsmi_npm_balancing_mode_test.cc index afef644cbac4..95231d07231c 100644 --- a/projects/amdsmi/tests/amd_smi_test/unit/gpu/amdsmi_npm_balancing_mode_test.cc +++ b/projects/amdsmi/tests/amd_smi_test/unit/gpu/amdsmi_npm_balancing_mode_test.cc @@ -5,18 +5,9 @@ // (amd_smi.cc) and the underlying rsmi_dev_npm_balancing_mode_get()/set() // (rocm_smi.cc). // -// Like amdsmi_set_npm_limit(), these take an amdsmi_node_handle that is -// internally a `std::string*` board-path pointer normally vended by -// amdsmi_get_node_handle(). These functions only accept a node_handle that -// this library actually vended (tracked in a registry populated by -// amdsmi_get_node_handle(); see is_registered_node_handle() in amd_smi.cc). -// Since tests fabricate their node_handle directly instead of calling -// amdsmi_get_node_handle(), every fabricated handle below is registered via -// the test-only amdsmi_test_register_node_handle() hook -// (amd_smi_test_internal.h) immediately after construction, so it is treated -// as valid by that check. The *RejectsUnregisteredHandle tests below are the -// ones that specifically exercise the *absence* of that registration call, -// and must not call amdsmi_test_register_node_handle(). +// Like amdsmi_set_npm_limit(), these only accept node handles the library +// vended, so tests get theirs from amdsmi_test_get_node_handle(); the +// *RejectsUnregisteredHandle tests fabricate one on purpose. // // rsmi_dev_npm_balancing_mode_set() applies REQUIRE_ROOT_ACCESS before // touching any file, so: @@ -150,8 +141,8 @@ TEST(GpuUnit, GetNpmBalancingModeNullModeIsInval) { ASSERT_EQ(init.status(), AMDSMI_STATUS_SUCCESS); std::string board_path = "/tmp/amdsmi_npm_balancing_mode_test_null_mode_probe"; - amdsmi_node_handle handle = reinterpret_cast(&board_path); - ASSERT_EQ(amdsmi_test_register_node_handle(handle), AMDSMI_STATUS_SUCCESS); + amdsmi_node_handle handle = nullptr; + ASSERT_EQ(amdsmi_test_get_node_handle(board_path, &handle), AMDSMI_STATUS_SUCCESS); EXPECT_EQ(amdsmi_get_npm_balancing_mode(handle, nullptr), AMDSMI_STATUS_INVAL); } @@ -168,8 +159,8 @@ TEST(GpuUnit, GetNpmBalancingModeDisabledStillReportsLastSelectedMode) { board.WriteFile("npm_status", "disabled"); board.WriteFile("mode", "1"); std::string board_path = board.path().string(); - amdsmi_node_handle handle = reinterpret_cast(&board_path); - ASSERT_EQ(amdsmi_test_register_node_handle(handle), AMDSMI_STATUS_SUCCESS); + amdsmi_node_handle handle = nullptr; + ASSERT_EQ(amdsmi_test_get_node_handle(board_path, &handle), AMDSMI_STATUS_SUCCESS); amdsmi_npm_balancing_mode_t mode = AMDSMI_NPM_BALANCING_MODE_INVALID; // Get is never gated on NPM enablement; it reflects board/mode even @@ -194,8 +185,8 @@ TEST(GpuUnit, GetNpmBalancingModeEnabledDefaultsToPowerBalancing) { // AMDSMI_STATUS_SUCCESS + AMDSMI_NPM_BALANCING_MODE_INVALID (N/A). board.WriteFile("mode", "1"); std::string board_path = board.path().string(); - amdsmi_node_handle handle = reinterpret_cast(&board_path); - ASSERT_EQ(amdsmi_test_register_node_handle(handle), AMDSMI_STATUS_SUCCESS); + amdsmi_node_handle handle = nullptr; + ASSERT_EQ(amdsmi_test_get_node_handle(board_path, &handle), AMDSMI_STATUS_SUCCESS); amdsmi_npm_balancing_mode_t mode = AMDSMI_NPM_BALANCING_MODE_INVALID; EXPECT_EQ(amdsmi_get_npm_balancing_mode(handle, &mode), AMDSMI_STATUS_SUCCESS); @@ -216,8 +207,8 @@ TEST(GpuUnit, GetNpmBalancingModeEnabledMissingModeFileIsNotSupported) { // must report NOT_SUPPORTED, agreeing with amdsmi_set_npm_balancing_mode() // for the identical state (see SetNpmBalancingModeRootMissingModeFileIsNotSupported). std::string board_path = board.path().string(); - amdsmi_node_handle handle = reinterpret_cast(&board_path); - ASSERT_EQ(amdsmi_test_register_node_handle(handle), AMDSMI_STATUS_SUCCESS); + amdsmi_node_handle handle = nullptr; + ASSERT_EQ(amdsmi_test_get_node_handle(board_path, &handle), AMDSMI_STATUS_SUCCESS); amdsmi_npm_balancing_mode_t mode = AMDSMI_NPM_BALANCING_MODE_POWER_BALANCING; EXPECT_EQ(amdsmi_get_npm_balancing_mode(handle, &mode), AMDSMI_STATUS_NOT_SUPPORTED); @@ -235,8 +226,8 @@ TEST(GpuUnit, GetNpmBalancingModeGarbageValueIsUnexpectedData) { board.WriteFile("npm_status", "enabled"); board.WriteFile("mode", "0"); std::string board_path = board.path().string(); - amdsmi_node_handle handle = reinterpret_cast(&board_path); - ASSERT_EQ(amdsmi_test_register_node_handle(handle), AMDSMI_STATUS_SUCCESS); + amdsmi_node_handle handle = nullptr; + ASSERT_EQ(amdsmi_test_get_node_handle(board_path, &handle), AMDSMI_STATUS_SUCCESS); // "0" is not a valid encoding ("1"/"2" only); this is a genuine // read/decode error, not the "missing file" N/A case. @@ -256,8 +247,8 @@ TEST(GpuUnit, GetNpmBalancingModeEnabledReadsFrequencyBalancing) { board.WriteFile("npm_status", "enabled"); board.WriteFile("mode", "2"); std::string board_path = board.path().string(); - amdsmi_node_handle handle = reinterpret_cast(&board_path); - ASSERT_EQ(amdsmi_test_register_node_handle(handle), AMDSMI_STATUS_SUCCESS); + amdsmi_node_handle handle = nullptr; + ASSERT_EQ(amdsmi_test_get_node_handle(board_path, &handle), AMDSMI_STATUS_SUCCESS); amdsmi_npm_balancing_mode_t mode = AMDSMI_NPM_BALANCING_MODE_INVALID; EXPECT_EQ(amdsmi_get_npm_balancing_mode(handle, &mode), AMDSMI_STATUS_SUCCESS); @@ -287,8 +278,8 @@ TEST(GpuUnit, SetNpmBalancingModeInvalidModeIsInval) { ASSERT_EQ(init.status(), AMDSMI_STATUS_SUCCESS); std::string board_path = "/tmp/amdsmi_npm_balancing_mode_test_invalid_mode_probe"; - amdsmi_node_handle handle = reinterpret_cast(&board_path); - ASSERT_EQ(amdsmi_test_register_node_handle(handle), AMDSMI_STATUS_SUCCESS); + amdsmi_node_handle handle = nullptr; + ASSERT_EQ(amdsmi_test_get_node_handle(board_path, &handle), AMDSMI_STATUS_SUCCESS); EXPECT_EQ(amdsmi_set_npm_balancing_mode(handle, static_cast( AMDSMI_NPM_BALANCING_MODE_INVALID)), @@ -313,8 +304,8 @@ TEST(GpuUnit, SetNpmBalancingModeNonRootIsNoPerm) { // rsmi_dev_npm_balancing_mode_set() rejects before the path is ever // touched. std::string board_path = "/tmp/amdsmi_npm_balancing_mode_test_nonroot_probe"; - amdsmi_node_handle handle = reinterpret_cast(&board_path); - ASSERT_EQ(amdsmi_test_register_node_handle(handle), AMDSMI_STATUS_SUCCESS); + amdsmi_node_handle handle = nullptr; + ASSERT_EQ(amdsmi_test_get_node_handle(board_path, &handle), AMDSMI_STATUS_SUCCESS); EXPECT_EQ(amdsmi_set_npm_balancing_mode(handle, AMDSMI_NPM_BALANCING_MODE_FREQUENCY_BALANCING), AMDSMI_STATUS_NO_PERM); @@ -336,8 +327,8 @@ TEST(GpuUnit, SetNpmBalancingModeRootRejectsWhenNpmDisabled) { board.WriteFile("npm_status", "disabled"); board.WriteFile("mode", "1"); std::string board_path = board.path().string(); - amdsmi_node_handle handle = reinterpret_cast(&board_path); - ASSERT_EQ(amdsmi_test_register_node_handle(handle), AMDSMI_STATUS_SUCCESS); + amdsmi_node_handle handle = nullptr; + ASSERT_EQ(amdsmi_test_get_node_handle(board_path, &handle), AMDSMI_STATUS_SUCCESS); // Must not even open board/mode for write when NPM is disabled: this is // AMDSMI_STATUS_NOT_SUPPORTED, a deliberate divergence from @@ -364,8 +355,8 @@ TEST(GpuUnit, SetNpmBalancingModeRootRoundTrip) { board.WriteFile("npm_status", "enabled"); board.WriteFile("mode", "1"); std::string board_path = board.path().string(); - amdsmi_node_handle handle = reinterpret_cast(&board_path); - ASSERT_EQ(amdsmi_test_register_node_handle(handle), AMDSMI_STATUS_SUCCESS); + amdsmi_node_handle handle = nullptr; + ASSERT_EQ(amdsmi_test_get_node_handle(board_path, &handle), AMDSMI_STATUS_SUCCESS); EXPECT_EQ(amdsmi_set_npm_balancing_mode(handle, AMDSMI_NPM_BALANCING_MODE_FREQUENCY_BALANCING), AMDSMI_STATUS_SUCCESS); @@ -402,8 +393,8 @@ TEST(GpuUnit, SetNpmBalancingModeRootMissingModeFileIsNotSupported) { // the file to already exist (it does not create it), mirroring // set_npm_board_limit()'s missing-file handling. std::string board_path = board.path().string(); - amdsmi_node_handle handle = reinterpret_cast(&board_path); - ASSERT_EQ(amdsmi_test_register_node_handle(handle), AMDSMI_STATUS_SUCCESS); + amdsmi_node_handle handle = nullptr; + ASSERT_EQ(amdsmi_test_get_node_handle(board_path, &handle), AMDSMI_STATUS_SUCCESS); EXPECT_EQ(amdsmi_set_npm_balancing_mode(handle, AMDSMI_NPM_BALANCING_MODE_FREQUENCY_BALANCING), AMDSMI_STATUS_NOT_SUPPORTED); @@ -431,8 +422,8 @@ TEST(GpuUnit, SetNpmBalancingModeRootMissingBoardDirIsNotSupported) { std::error_code ec; fs::remove_all(board.path(), ec); } - amdsmi_node_handle handle = reinterpret_cast(&board_path); - ASSERT_EQ(amdsmi_test_register_node_handle(handle), AMDSMI_STATUS_SUCCESS); + amdsmi_node_handle handle = nullptr; + ASSERT_EQ(amdsmi_test_get_node_handle(board_path, &handle), AMDSMI_STATUS_SUCCESS); EXPECT_EQ(amdsmi_set_npm_balancing_mode(handle, AMDSMI_NPM_BALANCING_MODE_FREQUENCY_BALANCING), AMDSMI_STATUS_NOT_SUPPORTED); @@ -456,8 +447,8 @@ TEST(GpuUnit, SetNpmBalancingModeRootRejectsModeAbsentFromSupportedBitmask) { // bit 2 (FREQUENCY_BALANCING) only -- POWER_BALANCING (bit 1) is absent. board.WriteFile("supported_mode", "0x4"); std::string board_path = board.path().string(); - amdsmi_node_handle handle = reinterpret_cast(&board_path); - ASSERT_EQ(amdsmi_test_register_node_handle(handle), AMDSMI_STATUS_SUCCESS); + amdsmi_node_handle handle = nullptr; + ASSERT_EQ(amdsmi_test_get_node_handle(board_path, &handle), AMDSMI_STATUS_SUCCESS); EXPECT_EQ(amdsmi_set_npm_balancing_mode(handle, AMDSMI_NPM_BALANCING_MODE_POWER_BALANCING), AMDSMI_STATUS_SETTING_UNAVAILABLE); @@ -483,8 +474,8 @@ TEST(GpuUnit, SetNpmBalancingModeRootAllowsModePresentInSupportedBitmask) { // bits 1+2 -- both modes supported, matching today's real platforms. board.WriteFile("supported_mode", "0x6"); std::string board_path = board.path().string(); - amdsmi_node_handle handle = reinterpret_cast(&board_path); - ASSERT_EQ(amdsmi_test_register_node_handle(handle), AMDSMI_STATUS_SUCCESS); + amdsmi_node_handle handle = nullptr; + ASSERT_EQ(amdsmi_test_get_node_handle(board_path, &handle), AMDSMI_STATUS_SUCCESS); EXPECT_EQ(amdsmi_set_npm_balancing_mode(handle, AMDSMI_NPM_BALANCING_MODE_FREQUENCY_BALANCING), AMDSMI_STATUS_SUCCESS); @@ -511,8 +502,8 @@ TEST(GpuUnit, SetNpmBalancingModeRootRejectsCorruptSupportedModesFile) { board.WriteFile("mode", "1"); board.WriteFile("supported_mode", "-1"); std::string board_path = board.path().string(); - amdsmi_node_handle handle = reinterpret_cast(&board_path); - ASSERT_EQ(amdsmi_test_register_node_handle(handle), AMDSMI_STATUS_SUCCESS); + amdsmi_node_handle handle = nullptr; + ASSERT_EQ(amdsmi_test_get_node_handle(board_path, &handle), AMDSMI_STATUS_SUCCESS); EXPECT_EQ(amdsmi_set_npm_balancing_mode(handle, AMDSMI_NPM_BALANCING_MODE_FREQUENCY_BALANCING), AMDSMI_STATUS_SETTING_UNAVAILABLE); @@ -539,8 +530,8 @@ TEST(GpuUnit, SetNpmBalancingModeRootToleratesMissingSupportedModesFile) { board.WriteFile("mode", "1"); // Deliberately do not create board/supported_mode. std::string board_path = board.path().string(); - amdsmi_node_handle handle = reinterpret_cast(&board_path); - ASSERT_EQ(amdsmi_test_register_node_handle(handle), AMDSMI_STATUS_SUCCESS); + amdsmi_node_handle handle = nullptr; + ASSERT_EQ(amdsmi_test_get_node_handle(board_path, &handle), AMDSMI_STATUS_SUCCESS); EXPECT_EQ(amdsmi_set_npm_balancing_mode(handle, AMDSMI_NPM_BALANCING_MODE_FREQUENCY_BALANCING), AMDSMI_STATUS_SUCCESS); @@ -571,8 +562,8 @@ TEST(GpuUnit, GetNpmSupportedBalancingModesNullOutputIsInval) { ASSERT_EQ(init.status(), AMDSMI_STATUS_SUCCESS); std::string board_path = "/tmp/amdsmi_npm_supported_balancing_modes_test_null_out_probe"; - amdsmi_node_handle handle = reinterpret_cast(&board_path); - ASSERT_EQ(amdsmi_test_register_node_handle(handle), AMDSMI_STATUS_SUCCESS); + amdsmi_node_handle handle = nullptr; + ASSERT_EQ(amdsmi_test_get_node_handle(board_path, &handle), AMDSMI_STATUS_SUCCESS); EXPECT_EQ(amdsmi_get_npm_supported_balancing_modes(handle, nullptr), AMDSMI_STATUS_INVAL); } @@ -589,8 +580,8 @@ TEST(GpuUnit, GetNpmSupportedBalancingModesMissingFileIsNotSupported) { // Deliberately do not create board/supported_mode: not yet implemented // by the driver on all platforms. std::string board_path = board.path().string(); - amdsmi_node_handle handle = reinterpret_cast(&board_path); - ASSERT_EQ(amdsmi_test_register_node_handle(handle), AMDSMI_STATUS_SUCCESS); + amdsmi_node_handle handle = nullptr; + ASSERT_EQ(amdsmi_test_get_node_handle(board_path, &handle), AMDSMI_STATUS_SUCCESS); amdsmi_bit_field_t supported_modes = 0; EXPECT_EQ(amdsmi_get_npm_supported_balancing_modes(handle, &supported_modes), @@ -609,8 +600,8 @@ TEST(GpuUnit, GetNpmSupportedBalancingModesNotGatedOnNpmEnablement) { board.WriteFile("npm_status", "disabled"); board.WriteFile("supported_mode", "0x6"); std::string board_path = board.path().string(); - amdsmi_node_handle handle = reinterpret_cast(&board_path); - ASSERT_EQ(amdsmi_test_register_node_handle(handle), AMDSMI_STATUS_SUCCESS); + amdsmi_node_handle handle = nullptr; + ASSERT_EQ(amdsmi_test_get_node_handle(board_path, &handle), AMDSMI_STATUS_SUCCESS); amdsmi_bit_field_t supported_modes = 0; EXPECT_EQ(amdsmi_get_npm_supported_balancing_modes(handle, &supported_modes), @@ -629,8 +620,8 @@ TEST(GpuUnit, GetNpmSupportedBalancingModesDecodesHexBitmask) { TempBoardDir board; board.WriteFile("supported_mode", "0x2"); std::string board_path = board.path().string(); - amdsmi_node_handle handle = reinterpret_cast(&board_path); - ASSERT_EQ(amdsmi_test_register_node_handle(handle), AMDSMI_STATUS_SUCCESS); + amdsmi_node_handle handle = nullptr; + ASSERT_EQ(amdsmi_test_get_node_handle(board_path, &handle), AMDSMI_STATUS_SUCCESS); amdsmi_bit_field_t supported_modes = 0; EXPECT_EQ(amdsmi_get_npm_supported_balancing_modes(handle, &supported_modes), @@ -649,8 +640,8 @@ TEST(GpuUnit, GetNpmSupportedBalancingModesGarbageValueIsUnexpectedData) { TempBoardDir board; board.WriteFile("supported_mode", "not-a-hex-number"); std::string board_path = board.path().string(); - amdsmi_node_handle handle = reinterpret_cast(&board_path); - ASSERT_EQ(amdsmi_test_register_node_handle(handle), AMDSMI_STATUS_SUCCESS); + amdsmi_node_handle handle = nullptr; + ASSERT_EQ(amdsmi_test_get_node_handle(board_path, &handle), AMDSMI_STATUS_SUCCESS); amdsmi_bit_field_t supported_modes = 0; EXPECT_EQ(amdsmi_get_npm_supported_balancing_modes(handle, &supported_modes), @@ -660,7 +651,7 @@ TEST(GpuUnit, GetNpmSupportedBalancingModesGarbageValueIsUnexpectedData) { // --------------------------------------------------------------------- // Registered-node_handle validation // -// These tests deliberately do NOT call amdsmi_test_register_node_handle(): +// These tests deliberately do NOT call amdsmi_test_get_node_handle(): // the handle below points at real, validly-constructed heap memory, but it // was never registered, which is exactly the condition // is_registered_node_handle() targets. Asserting AMDSMI_STATUS_INVAL (rather @@ -675,10 +666,6 @@ TEST(GpuUnit, GetNpmBalancingModeRejectsUnregisteredHandle) { } ASSERT_EQ(init.status(), AMDSMI_STATUS_SUCCESS); - // Heap-allocated so this address cannot alias a stack slot some other test - // in this binary already passed to amdsmi_test_register_node_handle() -- - // see GetNpmInfoRejectsUnregisteredHandle in amdsmi_npm_limit_test.cc for - // the full rationale. auto unregistered_board_path = std::make_unique("/tmp/amdsmi_npm_balancing_mode_test_unregistered_probe1"); amdsmi_node_handle handle = reinterpret_cast(unregistered_board_path.get()); @@ -733,8 +720,8 @@ TEST(GpuUnit, GetNpmSupportedBalancingModesNegativeValueIsUnexpectedData) { // must be rejected as a parse error instead. board.WriteFile("supported_mode", "-1"); std::string board_path = board.path().string(); - amdsmi_node_handle handle = reinterpret_cast(&board_path); - ASSERT_EQ(amdsmi_test_register_node_handle(handle), AMDSMI_STATUS_SUCCESS); + amdsmi_node_handle handle = nullptr; + ASSERT_EQ(amdsmi_test_get_node_handle(board_path, &handle), AMDSMI_STATUS_SUCCESS); amdsmi_bit_field_t supported_modes = 0; EXPECT_EQ(amdsmi_get_npm_supported_balancing_modes(handle, &supported_modes), diff --git a/projects/amdsmi/tests/amd_smi_test/unit/gpu/amdsmi_npm_limit_test.cc b/projects/amdsmi/tests/amd_smi_test/unit/gpu/amdsmi_npm_limit_test.cc index f15361ae8c83..7098d98997a1 100644 --- a/projects/amdsmi/tests/amd_smi_test/unit/gpu/amdsmi_npm_limit_test.cc +++ b/projects/amdsmi/tests/amd_smi_test/unit/gpu/amdsmi_npm_limit_test.cc @@ -1,52 +1,20 @@ // Copyright Advanced Micro Devices, Inc. // SPDX-License-Identifier: MIT -// Unit tests for the public amdsmi_set_npm_limit() API (amd_smi.cc) and the -// current_node_power round-trip in amdsmi_get_npm_info(). -// -// amdsmi_set_npm_limit() takes an amdsmi_node_handle, which is internally a -// `std::string*` board-path pointer (see amdsmi_get_node_handle() in -// amd_smi.cc). The public acquisition path (amdsmi_get_node_handle()) gates -// on `asic_info.oam_id != 0`, which is not satisfiable on this test -// environment's hardware/mock setup (pre-existing, out of scope for this -// feature). To exercise amdsmi_set_npm_limit() itself we construct the -// node_handle directly by reinterpret_cast'ing the address of a local -// std::string, exactly as amdsmi_get_node_handle() would have handed back, -// bypassing only the acquisition helper -- not the function under test. This -// mirrors the approach used in the feature's own smoke test (see the -// implementer's handoff notes). -// -// amdsmi_get_npm_info()/amdsmi_set_npm_limit() only accept a node_handle that -// this library actually vended (tracked in a registry populated by -// amdsmi_get_node_handle(); see is_registered_node_handle() in amd_smi.cc, -// fixing a pre-cast/dereference validation gap). Since tests fabricate their -// node_handle directly instead of calling amdsmi_get_node_handle(), every -// fabricated handle below is registered via the test-only -// amdsmi_test_register_node_handle() hook (amd_smi_test_internal.h) -// immediately after construction, so it is treated as valid by that check. -// The NpmInfoRejectsUnregisteredHandle/SetNpmLimitRejectsUnregisteredHandle -// tests below are the ones that specifically exercise the *absence* of that -// registration call, and must not call amdsmi_test_register_node_handle(). -// -// rsmi_dev_npm_limit_set() (rocm_smi.cc) applies REQUIRE_ROOT_ACCESS before -// touching any file, so: -// - non-root callers always observe AMDSMI_STATUS_NO_PERM, regardless of -// whether the board_path/file exists -- this is verifiable without root -// and is asserted unconditionally below. -// - success / not-supported branches (reached only past the root gate) -// require root and are gated with GTEST_SKIP_ following the existing -// is_sudo_user() convention used throughout tests/amd_smi_test/main.cc. -// Since set_npm_board_limit()/rsmi_dev_npm_limit_set() operate on an -// arbitrary board_path string (not hardcoded to /sys), the root-gated cases -// use a plain std::filesystem temp directory instead of real /sys mocking. +// Unit tests for amdsmi_set_npm_limit() and amdsmi_get_npm_info(). Handles come from +// amdsmi_test_get_node_handle() because amdsmi_get_node_handle() needs oam_id == 0 hardware. +// Root-only cases point the handle at a temp board directory. #include +#include #include +#include #include #include #include #include +#include #include #include "amd_smi/amdsmi.h" @@ -84,16 +52,19 @@ namespace fs = std::filesystem; namespace { -// RAII helper identical in spirit to the one in rocm_smi_npm_test.cc; kept -// local to this file to avoid coupling across unit-test translation units. +// Kept local to avoid coupling unit-test translation units. class TempBoardDir { public: TempBoardDir() { - path_ = fs::temp_directory_path() / - fs::path("amdsmi_npm_limit_test_" + std::to_string(reinterpret_cast(this))); - fs::create_directories(path_); + std::string tmpl = (fs::temp_directory_path() / "amdsmi_npm_limit_test_XXXXXX").string(); + if (::mkdtemp(tmpl.data()) == nullptr) { + ADD_FAILURE() << "mkdtemp failed: " << std::strerror(errno); + return; + } + path_ = tmpl; } ~TempBoardDir() { + if (path_.empty()) return; std::error_code ec; fs::remove_all(path_, ec); } @@ -164,11 +135,10 @@ TEST(GpuUnit, SetNpmLimitNonRootIsNoPerm) { "see SetNpmLimitRoot* tests instead"); } - // Any non-empty board path is sufficient: REQUIRE_ROOT_ACCESS in - // rsmi_dev_npm_limit_set() rejects before the path is ever touched. - std::string board_path = "/tmp/amdsmi_npm_limit_test_nonroot_probe"; - amdsmi_node_handle handle = reinterpret_cast(&board_path); - ASSERT_EQ(amdsmi_test_register_node_handle(handle), AMDSMI_STATUS_SUCCESS); + // REQUIRE_ROOT_ACCESS rejects before the path is touched, so it need not exist. + amdsmi_node_handle handle = nullptr; + ASSERT_EQ(amdsmi_test_get_node_handle("/tmp/amdsmi_npm_limit_test_nonroot_probe", &handle), + AMDSMI_STATUS_SUCCESS); EXPECT_EQ(amdsmi_set_npm_limit(handle, 100), AMDSMI_STATUS_NO_PERM); } @@ -183,28 +153,18 @@ TEST(GpuUnit, SetNpmLimitRootSuccessWritesValue) { } TempBoardDir board; - // NPM must be enabled on this node now that rsmi_dev_npm_limit_set() - // rejects the write outright when it is not. board.WriteFile("npm_status", "enabled"); - // max_node_power_limit must be present and >= the requested limit now that - // rsmi_dev_npm_limit_set() range-checks against it before writing (mirrors - // rsmi_dev_power_cap_set()'s range_get()-before-write pattern). board.WriteFile("max_node_power_limit", "6400"); board.WriteFile("cur_node_power_limit", "0"); - std::string board_path = board.path().string(); - amdsmi_node_handle handle = reinterpret_cast(&board_path); - ASSERT_EQ(amdsmi_test_register_node_handle(handle), AMDSMI_STATUS_SUCCESS); + amdsmi_node_handle handle = nullptr; + ASSERT_EQ(amdsmi_test_get_node_handle(board.path().string(), &handle), AMDSMI_STATUS_SUCCESS); EXPECT_EQ(amdsmi_set_npm_limit(handle, 250), AMDSMI_STATUS_SUCCESS); EXPECT_EQ(board.ReadFile("cur_node_power_limit"), "250"); } // --------------------------------------------------------------------- -// amdsmi_set_npm_limit() -- NPM-disabled enforcement -// -// rsmi_dev_npm_limit_set() rejects the write outright when NPM is disabled -// on the node (before even reading the platform max): writing -// cur_node_power_limit while NPM is disabled has no defined effect. +// amdsmi_set_npm_limit() -- validation before write // --------------------------------------------------------------------- TEST(GpuUnit, SetNpmLimitRootRejectsWhenNpmDisabled) { @@ -220,25 +180,13 @@ TEST(GpuUnit, SetNpmLimitRootRejectsWhenNpmDisabled) { board.WriteFile("npm_status", "disabled"); board.WriteFile("max_node_power_limit", "6400"); board.WriteFile("cur_node_power_limit", "0"); - std::string board_path = board.path().string(); - amdsmi_node_handle handle = reinterpret_cast(&board_path); - ASSERT_EQ(amdsmi_test_register_node_handle(handle), AMDSMI_STATUS_SUCCESS); + amdsmi_node_handle handle = nullptr; + ASSERT_EQ(amdsmi_test_get_node_handle(board.path().string(), &handle), AMDSMI_STATUS_SUCCESS); EXPECT_EQ(amdsmi_set_npm_limit(handle, 250), AMDSMI_STATUS_INVAL); - // The write must not have happened. EXPECT_EQ(board.ReadFile("cur_node_power_limit"), "0"); } -// --------------------------------------------------------------------- -// amdsmi_set_npm_limit() -- platform-max range enforcement (F-5 fix) -// -// rsmi_dev_npm_limit_set() now mirrors rsmi_dev_power_cap_set(): it queries -// the platform max via get_npm_board_max_limit() and rejects out-of-range -// requests with AMDSMI_STATUS_INVAL before ever touching -// cur_node_power_limit, and fails closed (propagates the underlying error) -// if the max itself can't be read. -// --------------------------------------------------------------------- - TEST(GpuUnit, SetNpmLimitRootRejectsZero) { ScopedAmdSmiInit init; REQUIRE_AMDSMI_INIT(init); @@ -252,12 +200,10 @@ TEST(GpuUnit, SetNpmLimitRootRejectsZero) { board.WriteFile("npm_status", "enabled"); board.WriteFile("max_node_power_limit", "6400"); board.WriteFile("cur_node_power_limit", "6000"); - std::string board_path = board.path().string(); - amdsmi_node_handle handle = reinterpret_cast(&board_path); - ASSERT_EQ(amdsmi_test_register_node_handle(handle), AMDSMI_STATUS_SUCCESS); + amdsmi_node_handle handle = nullptr; + ASSERT_EQ(amdsmi_test_get_node_handle(board.path().string(), &handle), AMDSMI_STATUS_SUCCESS); EXPECT_EQ(amdsmi_set_npm_limit(handle, 0), AMDSMI_STATUS_INVAL); - // The write must not have happened. EXPECT_EQ(board.ReadFile("cur_node_power_limit"), "6000"); } @@ -274,12 +220,10 @@ TEST(GpuUnit, SetNpmLimitRootRejectsOverMax) { board.WriteFile("npm_status", "enabled"); board.WriteFile("max_node_power_limit", "6400"); board.WriteFile("cur_node_power_limit", "6000"); - std::string board_path = board.path().string(); - amdsmi_node_handle handle = reinterpret_cast(&board_path); - ASSERT_EQ(amdsmi_test_register_node_handle(handle), AMDSMI_STATUS_SUCCESS); + amdsmi_node_handle handle = nullptr; + ASSERT_EQ(amdsmi_test_get_node_handle(board.path().string(), &handle), AMDSMI_STATUS_SUCCESS); EXPECT_EQ(amdsmi_set_npm_limit(handle, 6401), AMDSMI_STATUS_INVAL); - // The write must not have happened. EXPECT_EQ(board.ReadFile("cur_node_power_limit"), "6000"); } @@ -294,16 +238,11 @@ TEST(GpuUnit, SetNpmLimitRootRejectsWhenMaxUnreadable) { TempBoardDir board; board.WriteFile("npm_status", "enabled"); - // Deliberately do not create max_node_power_limit: the platform max is - // unreadable, so the request must be rejected (fail closed) rather than - // falling through to an unbounded write. board.WriteFile("cur_node_power_limit", "6000"); - std::string board_path = board.path().string(); - amdsmi_node_handle handle = reinterpret_cast(&board_path); - ASSERT_EQ(amdsmi_test_register_node_handle(handle), AMDSMI_STATUS_SUCCESS); + amdsmi_node_handle handle = nullptr; + ASSERT_EQ(amdsmi_test_get_node_handle(board.path().string(), &handle), AMDSMI_STATUS_SUCCESS); EXPECT_EQ(amdsmi_set_npm_limit(handle, 250), AMDSMI_STATUS_NOT_SUPPORTED); - // The write must not have happened. EXPECT_EQ(board.ReadFile("cur_node_power_limit"), "6000"); } @@ -317,29 +256,14 @@ TEST(GpuUnit, SetNpmLimitRootRejectsWhenMaxCorrupted) { } TempBoardDir board; - // "-1" is a regression case, not generic garbage: std::stoull("-1", ...) - // is well-defined C++ behavior that successfully parses to UINT64_MAX - // (leading '-' negates modulo 2^64) rather than throwing, so a naive - // parser would treat this corrupted content as a "successfully read" - // huge platform max -- which would let any limit through the - // `limit > max_limit` check. The platform max must fail to parse and be - // treated as unreadable (fail closed), exactly like the missing-file case - // in SetNpmLimitRootRejectsWhenMaxUnreadable above, not silently accepted - // as UINT64_MAX. board.WriteFile("npm_status", "enabled"); + // "-1" parses to UINT64_MAX, which must not be accepted as a bound. board.WriteFile("max_node_power_limit", "-1"); board.WriteFile("cur_node_power_limit", "6000"); - std::string board_path = board.path().string(); - amdsmi_node_handle handle = reinterpret_cast(&board_path); - ASSERT_EQ(amdsmi_test_register_node_handle(handle), AMDSMI_STATUS_SUCCESS); - - // Unlike the missing-file case above (RSMI_STATUS_NOT_SUPPORTED), a - // present-but-corrupted file is a parse failure: - // get_npm_board_max_limit()/read_board_uint64() returns - // RSMI_STATUS_UNEXPECTED_DATA, which rsmi_dev_npm_limit_set() propagates - // as-is (fail closed) and which maps to AMDSMI_STATUS_UNEXPECTED_DATA. + amdsmi_node_handle handle = nullptr; + ASSERT_EQ(amdsmi_test_get_node_handle(board.path().string(), &handle), AMDSMI_STATUS_SUCCESS); + EXPECT_EQ(amdsmi_set_npm_limit(handle, 250), AMDSMI_STATUS_UNEXPECTED_DATA); - // The write must not have happened. EXPECT_EQ(board.ReadFile("cur_node_power_limit"), "6000"); } @@ -356,9 +280,8 @@ TEST(GpuUnit, SetNpmLimitRootAcceptsInRange) { board.WriteFile("npm_status", "enabled"); board.WriteFile("max_node_power_limit", "6400"); board.WriteFile("cur_node_power_limit", "0"); - std::string board_path = board.path().string(); - amdsmi_node_handle handle = reinterpret_cast(&board_path); - ASSERT_EQ(amdsmi_test_register_node_handle(handle), AMDSMI_STATUS_SUCCESS); + amdsmi_node_handle handle = nullptr; + ASSERT_EQ(amdsmi_test_get_node_handle(board.path().string(), &handle), AMDSMI_STATUS_SUCCESS); EXPECT_EQ(amdsmi_set_npm_limit(handle, 6400), AMDSMI_STATUS_SUCCESS); EXPECT_EQ(board.ReadFile("cur_node_power_limit"), "6400"); @@ -375,14 +298,9 @@ TEST(GpuUnit, SetNpmLimitRootMissingFileIsNotSupported) { TempBoardDir board; board.WriteFile("npm_status", "enabled"); - // max_node_power_limit present (so the new range check itself succeeds), - // but cur_node_power_limit deliberately absent -- exercises the write-time - // NOT_SUPPORTED path in set_npm_board_limit(), distinct from - // SetNpmLimitRootRejectsWhenMaxUnreadable above. board.WriteFile("max_node_power_limit", "6400"); - std::string board_path = board.path().string(); - amdsmi_node_handle handle = reinterpret_cast(&board_path); - ASSERT_EQ(amdsmi_test_register_node_handle(handle), AMDSMI_STATUS_SUCCESS); + amdsmi_node_handle handle = nullptr; + ASSERT_EQ(amdsmi_test_get_node_handle(board.path().string(), &handle), AMDSMI_STATUS_SUCCESS); EXPECT_EQ(amdsmi_set_npm_limit(handle, 250), AMDSMI_STATUS_NOT_SUPPORTED); } @@ -396,51 +314,23 @@ TEST(GpuUnit, SetNpmLimitRootMissingBoardDirIsNotSupported) { GTEST_SKIP_("Invalid permission - Must run as super user"); } - fs::path missing = fs::temp_directory_path() / "amdsmi_npm_limit_test_missing_board_dir"; - std::error_code ec; - fs::remove_all(missing, ec); - std::string board_path = missing.string(); - amdsmi_node_handle handle = reinterpret_cast(&board_path); - ASSERT_EQ(amdsmi_test_register_node_handle(handle), AMDSMI_STATUS_SUCCESS); + TempBoardDir parent; + fs::path missing = parent.path() / "missing"; + amdsmi_node_handle handle = nullptr; + ASSERT_EQ(amdsmi_test_get_node_handle(missing.string(), &handle), AMDSMI_STATUS_SUCCESS); EXPECT_EQ(amdsmi_set_npm_limit(handle, 250), AMDSMI_STATUS_NOT_SUPPORTED); } // --------------------------------------------------------------------- -// Registered-node_handle validation (F-2 fix) -// -// amdsmi_get_npm_info()/amdsmi_set_npm_limit() previously -// reinterpret_cast()'d and immediately dereferenced -// (->empty()) their caller-supplied node_handle with no check that the -// pointer was ever actually vended by this library. An unprivileged caller -// could pass any non-null garbage value and hit that dereference *before* -// any privilege check (REQUIRE_ROOT_ACCESS lives one layer down, only inside -// rsmi_dev_npm_limit_set(), and not at all in the read path). Both functions -// now call is_registered_node_handle() first and reject anything not found -// in the node_handle registry with AMDSMI_STATUS_INVAL, never dereferencing -// an unregistered pointer. -// -// These tests deliberately do NOT call amdsmi_test_register_node_handle(): -// the handle below points at real, validly-constructed stack/heap memory -// (so a would-be crash is not guaranteed/portable to reproduce in CI), but -// it was never registered, which is exactly the condition the fix targets. -// Asserting AMDSMI_STATUS_INVAL (rather than merely "did not crash") -// demonstrates the registry check itself is what stopped execution, not -// incidental luck about what garbage bytes happened to be at that address. +// Registered-node_handle validation // --------------------------------------------------------------------- TEST(GpuUnit, GetNpmInfoRejectsUnregisteredHandle) { ScopedAmdSmiInit init; REQUIRE_AMDSMI_INIT(init); - // Heap-allocated (not stack-allocated) so this address cannot alias a - // stack slot some other test in this binary already passed to - // amdsmi_test_register_node_handle(): registered pointers live for the - // rest of the process, and other tests here register the address of a - // same-shaped stack-local std::string, which the allocator/compiler can - // legitimately hand back again for a later test's stack frame. A fresh - // heap allocation that nothing in this file ever registers cannot collide - // with that set. + // Heap-allocated and never registered, so it cannot alias a registry-owned handle. auto unregistered_board_path = std::make_unique("/tmp/amdsmi_npm_limit_test_unregistered_probe"); amdsmi_node_handle handle = reinterpret_cast(unregistered_board_path.get()); @@ -453,15 +343,11 @@ TEST(GpuUnit, SetNpmLimitRejectsUnregisteredHandle) { ScopedAmdSmiInit init; REQUIRE_AMDSMI_INIT(init); - // See GetNpmInfoRejectsUnregisteredHandle above for why this must be - // heap-allocated rather than a stack-local std::string. auto unregistered_board_path = std::make_unique("/tmp/amdsmi_npm_limit_test_unregistered_probe2"); amdsmi_node_handle handle = reinterpret_cast(unregistered_board_path.get()); - // AMDSMI_STATUS_INVAL must be returned regardless of caller privilege: the - // registry check runs before REQUIRE_ROOT_ACCESS is ever reached, whether - // or not this test process happens to be root. + // The registry check runs before REQUIRE_ROOT_ACCESS, so INVAL regardless of privilege. EXPECT_EQ(amdsmi_set_npm_limit(handle, 100), AMDSMI_STATUS_INVAL); } @@ -473,32 +359,19 @@ TEST(GpuUnit, SetNpmLimitAcceptsHandleAfterTestRegistration) { if (amd::smi::is_sudo_user()) { GTEST_SKIP_( "Running as root: this test isolates the registry check from " - "REQUIRE_ROOT_ACCESS by asserting NO_PERM (not INVAL); see " - "SetNpmLimitNonRootIsNoPerm for the rationale"); + "REQUIRE_ROOT_ACCESS by asserting NO_PERM (not INVAL)"); } - // Same handle shape as SetNpmLimitRejectsUnregisteredHandle above, but - // registered first: the registry check must pass, and the request must - // proceed to (and be stopped by) REQUIRE_ROOT_ACCESS instead of being - // rejected as INVAL -- demonstrating the fix does not reject legitimately - // vended handles. - std::string board_path = "/tmp/amdsmi_npm_limit_test_registered_probe"; - amdsmi_node_handle handle = reinterpret_cast(&board_path); - ASSERT_EQ(amdsmi_test_register_node_handle(handle), AMDSMI_STATUS_SUCCESS); + // A registered handle must get past the registry check and stop at REQUIRE_ROOT_ACCESS. + amdsmi_node_handle handle = nullptr; + ASSERT_EQ(amdsmi_test_get_node_handle("/tmp/amdsmi_npm_limit_test_registered_probe", &handle), + AMDSMI_STATUS_SUCCESS); EXPECT_EQ(amdsmi_set_npm_limit(handle, 100), AMDSMI_STATUS_NO_PERM); } #if defined(ENABLE_WSL_BACKEND) -// When the WSL backend is active, amdsmi_set_npm_limit() must short-circuit -// to AMDSMI_STATUS_NOT_SUPPORTED before ever touching board_path/sysfs (see -// the `#ifdef ENABLE_WSL_BACKEND` / WSLGPUBackend::IsActive() guard at the -// top of amdsmi_set_npm_limit() in amd_smi.cc). This is only observable on a -// real WSL2 machine where amdsmi_init() -> populate_amd_gpu_devices() -> -// WSLGPUBackend::TryPopulate() actually succeeded (requires /dev/dxg and a -// loadable librocdxg); otherwise IsActive() stays false even when compiled -// with ENABLE_WSL_BACKEND=ON, so this test skips itself in that case rather -// than asserting anything about the (untaken) native-Linux path. +// Only observable on real WSL2 where WSLGPUBackend::TryPopulate() succeeded. TEST(GpuUnit, SetNpmLimitNotSupportedWhenWslBackendActive) { ScopedAmdSmiInit init; REQUIRE_AMDSMI_INIT(init); @@ -510,11 +383,9 @@ TEST(GpuUnit, SetNpmLimitNotSupportedWhenWslBackendActive) { "NOT_SUPPORTED short-circuit cannot be exercised here"); } - // Any board path is sufficient: the WSL-backend guard runs before the - // path is ever dereferenced. - std::string board_path = "/tmp/amdsmi_npm_limit_test_wsl_probe"; - amdsmi_node_handle handle = reinterpret_cast(&board_path); - ASSERT_EQ(amdsmi_test_register_node_handle(handle), AMDSMI_STATUS_SUCCESS); + amdsmi_node_handle handle = nullptr; + ASSERT_EQ(amdsmi_test_get_node_handle("/tmp/amdsmi_npm_limit_test_wsl_probe", &handle), + AMDSMI_STATUS_SUCCESS); EXPECT_EQ(amdsmi_set_npm_limit(handle, 100), AMDSMI_STATUS_NOT_SUPPORTED); } @@ -524,23 +395,7 @@ TEST(GpuUnit, SetNpmLimitNotSupportedWhenWslBackendActive) { // amdsmi_get_npm_info() -- current_node_power round trip // --------------------------------------------------------------------- -// current_node_power moved from amdsmi_power_info_t (per-GPU, amd-smi metric -// --power) to amdsmi_npm_info_t (per-node, amd-smi node -p / -// amdsmi_get_npm_info()) -- see the "Set npm power limit" design doc. It is -// still populated by resolving the board directory from the real device's -// enumeration info (drm_render) via amdsmi_get_node_handle(), not from a -// caller-supplied path, so it cannot be pointed at a temp directory the way -// amdsmi_set_npm_limit() can. This test therefore validates against whatever -// the current hardware reports: either a concrete value (if -// board/node_power exists) or the UINT32_MAX sentinel/"N/A" case (if it -// doesn't, e.g. no board/ directory on this platform) -- both are valid, -// contract-conformant outcomes, and the test asserts the round trip is -// internally consistent either way. -// -// amdsmi_get_node_handle() additionally gates on asic_info.oam_id == 0 (see -// the file header comment above), so on hardware/mocks where that gate is -// not satisfied this test will skip rather than fail -- it is exercising the -// real acquisition path end-to-end, not a test-fabricated handle. +// Uses the real amdsmi_get_node_handle() path; skips where no node handle is available. TEST(GpuUnit, GetNpmInfoCurrentNodePowerRoundTrip) { ScopedAmdSmiInit init; REQUIRE_AMDSMI_INIT(init); @@ -575,15 +430,8 @@ TEST(GpuUnit, GetNpmInfoCurrentNodePowerRoundTrip) { } exercised_at_least_one_node = true; - // Contract: current_node_power is either the sentinel (falls back - // silently, e.g. no board/node_power file) or a concrete uint32 value - // read from board/node_power -- never left uninitialized (e.g. - // garbage/0 as a side effect of a partially-implemented read). + // UINT32_MAX means board/node_power was unavailable. if (npm_info.current_node_power != kSentinel) { - // A "real" reading was obtained: sanity-check it is a plausible - // wattage rather than accidentally aliasing the sentinel's bit - // pattern or being negative-when-reinterpreted (uint32_t can't be - // negative, but guard against absurd overflow wrap regardless). EXPECT_LT(npm_info.current_node_power, kSentinel); } } @@ -595,23 +443,3 @@ TEST(GpuUnit, GetNpmInfoCurrentNodePowerRoundTrip) { "this system"); } } - -TEST(GpuUnit, GetPowerInfoNullInfoIsInval) { - ScopedAmdSmiInit init; - REQUIRE_AMDSMI_INIT(init); - - amdsmi_socket_handle sockets[16]; - uint32_t socket_count = 16; - ASSERT_EQ(amdsmi_get_socket_handles(&socket_count, sockets), AMDSMI_STATUS_SUCCESS); - if (socket_count == 0) { - GTEST_SKIP_("No sockets discovered on this system"); - } - amdsmi_processor_handle procs[16]; - uint32_t proc_count = 16; - ASSERT_EQ(amdsmi_get_processor_handles(sockets[0], &proc_count, procs), AMDSMI_STATUS_SUCCESS); - if (proc_count == 0) { - GTEST_SKIP_("No processors discovered on this system"); - } - - EXPECT_EQ(amdsmi_get_power_info(procs[0], nullptr), AMDSMI_STATUS_INVAL); -} diff --git a/projects/amdsmi/tests/amd_smi_test/unit/gpu/rocm_smi_npm_test.cc b/projects/amdsmi/tests/amd_smi_test/unit/gpu/rocm_smi_npm_test.cc index 0ea313bf0e90..0363e57fe657 100644 --- a/projects/amdsmi/tests/amd_smi_test/unit/gpu/rocm_smi_npm_test.cc +++ b/projects/amdsmi/tests/amd_smi_test/unit/gpu/rocm_smi_npm_test.cc @@ -1,23 +1,18 @@ // Copyright Advanced Micro Devices, Inc. // SPDX-License-Identifier: MIT -// Hardware-free unit tests for the NPM (Node Power Management) helpers in -// rocm_smi_npm.cc: set_npm_board_limit() and get_npm_node_power(). -// -// These functions take an arbitrary board_path string and operate on plain -// files underneath it (they do not require /sys or any specific sysfs -// layout), so we exercise them against a real temporary directory created -// via std::filesystem instead of mocking /sys. This mirrors the existing -// (unexercised prior to this change) get_npm_board_limit()/ -// get_npm_board_status() logic in the same translation unit, all of which -// share the same "board_path directory + single-value file" convention. +// Hardware-free unit tests for the NPM board helpers in rocm_smi_npm.cc, run +// against a temp directory standing in for the sysfs board/ directory. #include "rocm_smi/rocm_smi_npm.h" #include +#include #include #include +#include +#include #include #include #include @@ -26,19 +21,20 @@ namespace fs = std::filesystem; namespace { -// RAII helper: creates a unique temp directory for the duration of a test -// and removes it (recursively) on destruction. +// RAII helper: creates a private (0700) temp directory and removes it on destruction. class TempBoardDir { public: TempBoardDir() { - path_ = fs::temp_directory_path() / - fs::path("amdsmi_npm_test_" + - std::to_string(::testing::UnitTest::GetInstance()->random_seed()) + "_" + - std::to_string(reinterpret_cast(this))); - fs::create_directories(path_); + std::string tmpl = (fs::temp_directory_path() / "amdsmi_npm_test_XXXXXX").string(); + if (::mkdtemp(tmpl.data()) == nullptr) { + ADD_FAILURE() << "mkdtemp failed: " << std::strerror(errno); + return; + } + path_ = tmpl; } ~TempBoardDir() { + if (path_.empty()) return; std::error_code ec; fs::remove_all(path_, ec); } @@ -72,9 +68,8 @@ TEST(GpuUnit, SetNpmBoardLimitEmptyPathIsInvalidArgs) { } TEST(GpuUnit, SetNpmBoardLimitMissingBoardDirIsNotSupported) { - fs::path missing = fs::temp_directory_path() / "amdsmi_npm_test_definitely_missing_dir_xyz"; - std::error_code ec; - fs::remove_all(missing, ec); // ensure it really doesn't exist + TempBoardDir parent; + fs::path missing = parent.path() / "missing"; EXPECT_EQ(amd::smi::set_npm_board_limit(missing.string(), 100), RSMI_STATUS_NOT_SUPPORTED); } @@ -126,9 +121,8 @@ TEST(GpuUnit, GetNpmNodePowerEmptyPathIsInvalidArgs) { } TEST(GpuUnit, GetNpmNodePowerMissingBoardDirIsNotSupported) { - fs::path missing = fs::temp_directory_path() / "amdsmi_npm_test_definitely_missing_dir_pwr"; - std::error_code ec; - fs::remove_all(missing, ec); + TempBoardDir parent; + fs::path missing = parent.path() / "missing"; uint64_t power = 0; EXPECT_EQ(amd::smi::get_npm_node_power(missing.string(), &power), RSMI_STATUS_NOT_SUPPORTED); @@ -176,9 +170,8 @@ TEST(GpuUnit, GetNpmBoardMaxLimitEmptyPathIsInvalidArgs) { } TEST(GpuUnit, GetNpmBoardMaxLimitMissingBoardDirIsNotSupported) { - fs::path missing = fs::temp_directory_path() / "amdsmi_npm_test_definitely_missing_dir_max"; - std::error_code ec; - fs::remove_all(missing, ec); + TempBoardDir parent; + fs::path missing = parent.path() / "missing"; uint64_t limit = 0; EXPECT_EQ(amd::smi::get_npm_board_max_limit(missing.string(), &limit), RSMI_STATUS_NOT_SUPPORTED); @@ -209,3 +202,92 @@ TEST(GpuUnit, GetNpmBoardMaxLimitNonNumericContentsIsUnexpectedData) { EXPECT_EQ(amd::smi::get_npm_board_max_limit(board.path().string(), &limit), RSMI_STATUS_UNEXPECTED_DATA); } + +TEST(GpuUnit, GetNpmBoardMaxLimitLeadingSpaceParses) { + TempBoardDir board; + board.WriteFile("max_node_power_limit", " 6400"); + + uint64_t limit = 0; + EXPECT_EQ(amd::smi::get_npm_board_max_limit(board.path().string(), &limit), RSMI_STATUS_SUCCESS); + EXPECT_EQ(limit, 6400u); +} + +// --------------------------------------------------------------------- +// validate_npm_board_limit() +// --------------------------------------------------------------------- + +TEST(GpuUnit, ValidateNpmBoardLimitEnabledInRangeIsSuccess) { + TempBoardDir board; + board.WriteFile("npm_status", "enabled"); + board.WriteFile("max_node_power_limit", "6400"); + + EXPECT_EQ(amd::smi::validate_npm_board_limit(board.path().string(), 250), RSMI_STATUS_SUCCESS); +} + +TEST(GpuUnit, ValidateNpmBoardLimitAtMaxIsSuccess) { + TempBoardDir board; + board.WriteFile("npm_status", "enabled"); + board.WriteFile("max_node_power_limit", "6400"); + + EXPECT_EQ(amd::smi::validate_npm_board_limit(board.path().string(), 6400), RSMI_STATUS_SUCCESS); +} + +TEST(GpuUnit, ValidateNpmBoardLimitDisabledIsInvalidArgs) { + TempBoardDir board; + board.WriteFile("npm_status", "disabled"); + board.WriteFile("max_node_power_limit", "6400"); + + EXPECT_EQ(amd::smi::validate_npm_board_limit(board.path().string(), 250), + RSMI_STATUS_INVALID_ARGS); +} + +TEST(GpuUnit, ValidateNpmBoardLimitMissingStatusIsNotSupported) { + TempBoardDir board; + board.WriteFile("max_node_power_limit", "6400"); + + EXPECT_EQ(amd::smi::validate_npm_board_limit(board.path().string(), 250), + RSMI_STATUS_NOT_SUPPORTED); +} + +TEST(GpuUnit, ValidateNpmBoardLimitMissingMaxIsNotSupported) { + TempBoardDir board; + board.WriteFile("npm_status", "enabled"); + + EXPECT_EQ(amd::smi::validate_npm_board_limit(board.path().string(), 250), + RSMI_STATUS_NOT_SUPPORTED); +} + +TEST(GpuUnit, ValidateNpmBoardLimitNegativeMaxIsUnexpectedData) { + TempBoardDir board; + board.WriteFile("npm_status", "enabled"); + board.WriteFile("max_node_power_limit", "-1"); + + EXPECT_EQ(amd::smi::validate_npm_board_limit(board.path().string(), 250), + RSMI_STATUS_UNEXPECTED_DATA); +} + +TEST(GpuUnit, ValidateNpmBoardLimitU64MaxIsUnexpectedData) { + TempBoardDir board; + board.WriteFile("npm_status", "enabled"); + board.WriteFile("max_node_power_limit", "18446744073709551615"); + + EXPECT_EQ(amd::smi::validate_npm_board_limit(board.path().string(), 250), + RSMI_STATUS_UNEXPECTED_DATA); +} + +TEST(GpuUnit, ValidateNpmBoardLimitZeroIsInvalidArgs) { + TempBoardDir board; + board.WriteFile("npm_status", "enabled"); + board.WriteFile("max_node_power_limit", "6400"); + + EXPECT_EQ(amd::smi::validate_npm_board_limit(board.path().string(), 0), RSMI_STATUS_INVALID_ARGS); +} + +TEST(GpuUnit, ValidateNpmBoardLimitOverMaxIsInvalidArgs) { + TempBoardDir board; + board.WriteFile("npm_status", "enabled"); + board.WriteFile("max_node_power_limit", "6400"); + + EXPECT_EQ(amd::smi::validate_npm_board_limit(board.path().string(), 6401), + RSMI_STATUS_INVALID_ARGS); +} diff --git a/projects/amdsmi/tests/python/unit/gpu/test_cli_current_node_power_management.py b/projects/amdsmi/tests/python/unit/gpu/test_cli_current_node_power_management.py index 81cc66ed22ea..1fd2cc211d9b 100644 --- a/projects/amdsmi/tests/python/unit/gpu/test_cli_current_node_power_management.py +++ b/projects/amdsmi/tests/python/unit/gpu/test_cli_current_node_power_management.py @@ -2,35 +2,13 @@ # Copyright Advanced Micro Devices, Inc. # SPDX-License-Identifier: MIT -"""Mock-based unit tests for the ``CURRENT_NODE_POWER`` field in ``amd-smi node -p``. - -current_node_power moved from amdsmi_power_info_t (per-GPU, amd-smi metric ---power) to amdsmi_npm_info_t (per-node, amd-smi node -p / -amdsmi_get_npm_info()) -- see the "Set npm power limit" design doc. This file -replaces the previous test_metric_power_node_power.py, which exercised the -now-removed amd-smi metric --power NODE_POWER surface. - -``NodeCommands.node()`` in ``amdsmi_cli/subcommands/node.py`` now seeds -``npm_dict["current_node_power"] = "N/A"`` and, on a successful -``amdsmi_get_npm_info()`` call, overwrites it with -``npm_info.get("current_node_power", "N/A")`` -- directly mirroring the -pre-existing ``threshold``/``limit`` fields it sits next to. These tests -drive the real ``NodeCommands.node()`` dispatch (stubbing only the -underlying ``amdsmi`` package, per the -``test_vcn_busy_navi.py``/``test_cli_set_clk_limit.py`` stub-and-importlib -pattern) and assert ``current_node_power`` is present with the correct -format/value across all output modes: - -* human-readable: unit-suffixed string printed as ``CURRENT_NODE_POWER: 5800 W``. -* JSON/file output: ``{"value": 5800, "unit": "W"}`` (unit_format's json - branch), nested under ``power_management``. -* CSV: the raw value (unit_format is not applied on the CSV path). - -Also covers the "N/A" sentinel pass-through (key absent from the library's -dict, or explicitly UINT32_MAX-turned-"N/A" upstream in -``amdsmi_get_npm_info()``'s Python wrapper) and the -``AmdSmiLibraryException`` fallback path (whole npm_dict stays at its "N/A" -defaults, current_node_power included). +"""Mock-based unit tests for ``CURRENT_NODE_POWER`` in ``amd-smi node -p``. + +Drives the real ``NodeCommands.node()`` with the ``amdsmi`` package stubbed and +checks the new ``current_node_power`` field from ``amdsmi_get_npm_info()``: +human-readable, JSON, and CSV output, "N/A" pass-through, the +``AmdSmiLibraryException`` fallback, and that the CSV ``node`` column is emitted +only with ``-p``. """ import argparse @@ -70,6 +48,7 @@ def _build_fake_modules(): amdsmi_get_npm_info=lambda _h: dict(_DEFAULT_NPM_INFO), amdsmi_get_npm_balancing_mode=lambda _h: "PB", amdsmi_get_npm_supported_balancing_modes=lambda _h: ["PB", "FB"], + amdsmi_get_tray_info=lambda: {"max_acc_per_tray": 4, "tray_type": "OAM"}, ) exception = fake_module("amdsmi.amdsmi_exception", AmdSmiLibraryException=_FakeLibraryException) amdsmi_pkg = fake_module( @@ -142,7 +121,7 @@ def _build_node_args(**overrides): return argparse.Namespace(**defaults) -def _run_node(node_module, output_format="human", destination="stdout"): +def _run_node(node_module, output_format="human", destination="stdout", **arg_overrides): helpers = _FakeHelpers() logger = _FakeLogger(output_format=output_format, destination=destination) commands = object.__new__(node_module.NodeCommands) @@ -151,7 +130,7 @@ def _run_node(node_module, output_format="human", destination="stdout"): commands.group_check_printed = True commands.node_handle = object() - commands.node(_build_node_args()) + commands.node(_build_node_args(**arg_overrides)) return logger, commands @@ -191,8 +170,19 @@ def test_current_node_power_csv_format(self): _, commands = _run_node(self.node_module, output_format="csv", destination="file") # CSV path stores raw values, no unit_format applied. self.assertEqual(commands.logger.output["current_node_power"], 5800) - # CSV rows always lead with a device identifier, mirroring gpu/cpu/nic. self.assertEqual(commands.logger.output["node"], 0) + self.assertEqual(next(iter(commands.logger.output)), "node") + + def test_csv_omits_node_column_without_power_management(self): + _, commands = _run_node( + self.node_module, + output_format="csv", + destination="file", + power_management=False, + tray=True, + ) + self.assertNotIn("node", commands.logger.output) + self.assertEqual(commands.logger.output["tray_type"], "OAM") def test_current_node_power_na_key_absent_from_library_dict(self): # If current_node_power is missing from the dict entirely (e.g. an diff --git a/projects/amdsmi/tests/python/unit/gpu/test_cli_set_node_power_limit.py b/projects/amdsmi/tests/python/unit/gpu/test_cli_set_node_power_limit.py index 76586e50b691..e8135d4a85fb 100644 --- a/projects/amdsmi/tests/python/unit/gpu/test_cli_set_node_power_limit.py +++ b/projects/amdsmi/tests/python/unit/gpu/test_cli_set_node_power_limit.py @@ -4,70 +4,12 @@ """Mock-based unit tests for ``amd-smi set --node-power-limit``. -Two layers are covered here: - -* ``TestCliSetNodePowerLimit`` drives ``SetValueCommands.set_value()``'s - node-wide (not per-GPU) early-dispatch block, modeled on the existing - ``--gtt`` block: - - * ``--node-power-limit`` combined with ``--gpu`` is rejected before any - library call, exiting with status 2 and a stderr message (mirrors the - ``--gtt``/``--gpu`` argparse-level guard's runtime-level counterpart). - * ``self.node_handle is None`` (no NPM-capable node resolved at startup, - e.g. the pre-existing ``oam_id != 0`` gate on current mock hardware) - short-circuits to a NOT_SUPPORTED-style message without calling the API. - * A successful set reports "Successfully set node power limit to W." - * ``AMDSMI_STATUS_NO_PERM`` from the library is translated into a raised - ``PermissionError`` (so the CLI's elevation-prompt wrapper can catch it), - not swallowed into an output message. - * Any other library exception (e.g. NOT_SUPPORTED bubbling up from - ``rsmi_dev_npm_limit_set()`` for a missing sysfs file) is caught and - reported as an output message referencing the requested value, without - raising. - - ``self.helpers`` is a minimal stub (mirroring ``test_cli_set_clk_limit.py``'s - ``_StubHelpers`` pattern) that reproduces - ``validate_and_set_node_power_limit()``'s call/status-mapping contract, so - these tests can focus on ``set_value.py``'s own dispatch behavior without - re-deriving the validation logic tested in depth below. - -* ``TestValidateAndSetNodePowerLimit`` drives the real - ``amdsmi_helpers.AMDSMIHelpers.validate_and_set_node_power_limit()`` - directly (added for the original F-1 fix, and updated for the F-2 fix - below), against a real import of ``amdsmi_helpers`` with only the C library - faked -- this is the validation logic itself, previously exercised nowhere - in the tree: - - * a request above the platform max (``amdsmi_get_npm_info()``'s - ``max_node_power_limit``) is rejected before any write. - * a request of ``0`` is rejected. - * an in-range request succeeds and calls ``amdsmi_set_npm_limit()`` with - the requested value verbatim. - * an unreadable platform max (``max_node_power_limit == "N/A"``) now fails - *closed*: the request is rejected, not allowed through on a bare - positivity check. (Previously this class of degraded-driver scenario - was fail-open; see the F-2 handoff finding this test locks in.) - -* ``TestNodePowerLimitGuestRegistration`` drives ``AMDSMIParser`` directly - (loaded the same way as ``test_output_file_stdin.py``): commit 567bed2f89d - moved ``-n``/``--node-power-limit`` argparse registration out of the - ``is_baremetal()``-only block so it is gated only by - ``is_amdgpu_initialized()`` (same pattern as ``-o``/``--power-cap``), - since NPM is exposed to 1VF guests. Asserts the argument is still - registered and parseable under a non-baremetal (guest) helpers stub. - -* ``TestGetMaxNodePowerLimit`` drives the real - ``amdsmi_helpers.AMDSMIHelpers.get_max_node_power_limit()`` (added by - ece891b6787 to size --node-power-limit's help-text bound), against a real - import of ``amdsmi_helpers`` with only the C library faked -- mirroring - ``TestValidateAndSetNodePowerLimit``'s pattern: - - * a first-device success returns immediately without probing further - devices. - * an exception on every device falls back to "N/A". - * a device that succeeds but reports "N/A" as the value itself (not an - exception) is a distinct code path from the exception branch; the loop - must still continue to the next device. +* ``TestCliSetNodePowerLimit``: the node-wide dispatch block in ``set_value.py``, + with ``self.helpers`` stubbed. +* ``TestValidateAndSetNodePowerLimit``: the real + ``AMDSMIHelpers.validate_and_set_node_power_limit()`` with only the C library faked. +* ``TestSetValueParser``: ``-n`` registers on a 1VF guest, and the ``--gtt``/``--gpu`` + guard rewrites only argparse's missing ``--gpu`` value error. """ import argparse @@ -179,19 +121,29 @@ def is_csv_format(self): return False +class _RecordingErrorCollector: + def __init__(self): + self.codes = [] + self.library_codes = [] + + def record(self, exit_code): + self.codes.append(exit_code) + + def record_library_error(self, error_code): + self.library_codes.append(error_code) + + class _StubHelpers: - """Minimal ``self.helpers`` stub for the node-power-limit dispatch block. - - Mirrors ``test_cli_set_clk_limit.py``'s ``_StubHelpers`` pattern: just - enough of ``validate_and_set_node_power_limit()``'s call/status-mapping - contract (call the C entry point, map ``AMDSMI_STATUS_NO_PERM`` to a - raised ``PermissionError``, otherwise format success/error strings) for - ``set_value.py``'s dispatch block to exercise, without pulling in the real - max-bound validation -- that logic is exercised directly and in depth by - ``TestValidateAndSetNodePowerLimit`` below, against the real - ``amdsmi_helpers.AMDSMIHelpers`` implementation. + """``self.helpers`` stub reproducing validate_and_set_node_power_limit()'s + status mapping; the real validation is covered by TestValidateAndSetNodePowerLimit. """ + def __init__(self): + self.error_collector = _RecordingErrorCollector() + + def get_output_format(self): + return "human" + def validate_and_set_node_power_limit(self, node_handle, requested_limit, logger): # Resolved lazily (not at module scope) because the fake "amdsmi" # package is only installed into sys.modules once a test class's @@ -205,6 +157,7 @@ def validate_and_set_node_power_limit(self, node_handle, requested_limit, logger except exception.AmdSmiLibraryException as e: if e.get_error_code() == interface.amdsmi_wrapper.AMDSMI_STATUS_NO_PERM: raise PermissionError("Command requires elevation") from e + self.error_collector.record_library_error(e.get_error_code()) return ( f"[{e.get_error_info(detailed=False)}] " f"Unable to set node power limit to {requested_limit} W" @@ -232,22 +185,23 @@ def _make_command(self, node_handle): def _make_args(self, node_power_limit, gpu=None): return types.SimpleNamespace(gpu=gpu, node_power_limit=node_power_limit) - def test_gpu_conflict_exits_with_status_2(self): + def test_gpu_conflict_raises_invalid_parameter(self): + calls = [] + self.interface.amdsmi_set_npm_limit = lambda h, limit: calls.append((h, limit)) cmd = self._make_command(node_handle=object()) args = self._make_args(node_power_limit=250, gpu="gpu0") - stderr = io.StringIO() - with contextlib.redirect_stderr(stderr): - with self.assertRaises(SystemExit) as ctx: - cmd.set_value(args) + with self.assertRaises(self.module.AmdSmiInvalidParameterException) as ctx: + cmd.set_value(args) - self.assertEqual(ctx.exception.code, 2) - self.assertIn("--node-power-limit", stderr.getvalue()) - self.assertIn("--gpu", stderr.getvalue()) + self.assertIn( + "--node-power-limit/-n: not allowed with argument --gpu/-g", str(ctx.exception) + ) + self.assertEqual(calls, [], "API must not be called when --gpu is also given") - def test_no_node_handle_reports_not_supported_without_calling_api(self): + def test_no_node_handle_records_not_supported_without_calling_api(self): calls = [] - self.interface.amdsmi_set_npm_limit = lambda h, l: calls.append((h, l)) + self.interface.amdsmi_set_npm_limit = lambda h, limit: calls.append((h, limit)) cmd = self._make_command(node_handle=None) args = self._make_args(node_power_limit=250) @@ -257,10 +211,11 @@ def test_no_node_handle_reports_not_supported_without_calling_api(self): message = cmd.logger.output["set_node_power_limit"] self.assertIn("NOT_SUPPORTED", message) self.assertIn("no NPM-capable node found", message) + self.assertEqual(cmd.helpers.error_collector.library_codes, [_STATUS_NOT_SUPPORTED]) def test_success_reports_value_in_watts(self): calls = [] - self.interface.amdsmi_set_npm_limit = lambda h, l: calls.append((h, l)) + self.interface.amdsmi_set_npm_limit = lambda h, limit: calls.append((h, limit)) node_handle = object() cmd = self._make_command(node_handle=node_handle) args = self._make_args(node_power_limit=6000) @@ -371,25 +326,27 @@ def tearDownClass(cls): def setUp(self): self.calls = [] - self.interface.amdsmi_set_npm_limit = lambda h, l: self.calls.append((h, l)) + self.error_collector = _RecordingErrorCollector() + self.interface.amdsmi_set_npm_limit = lambda h, limit: self.calls.append((h, limit)) def _validate(self, requested_limit, max_node_power_limit, npm_status=_NPM_STATUS_ENABLED): self.interface.amdsmi_get_npm_info = lambda _h: { "max_node_power_limit": max_node_power_limit, "status": npm_status, } - # A lightweight duck-typed ``self`` -- exercises the real, unbound - # ``validate_and_set_node_power_limit`` method body without paying for - # ``AMDSMIHelpers.__init__``'s platform/hypervisor probing, which is - # irrelevant to this validation logic. - fake_self = types.SimpleNamespace(get_output_format=lambda: "human") + # Duck-typed ``self`` skips AMDSMIHelpers.__init__'s platform probing. + fake_self = types.SimpleNamespace( + get_output_format=lambda: "human", error_collector=self.error_collector + ) logger = _FakeLogger() return self.validate(fake_self, object(), requested_limit, logger) def test_over_max_rejects(self): with self.assertRaises(self.exceptions_module.AmdSmiInvalidParameterValueException) as ctx: self._validate(requested_limit=6401, max_node_power_limit=6400) - self.assertIn("must be between 1W and 6400W", str(ctx.exception)) + message = str(ctx.exception) + self.assertIn("--node-power-limit must be between 1W and 6400W", message) + self.assertIn("amd-smi set -h", message) self.assertEqual(self.calls, [], "API must not be called on a rejected request") def test_zero_rejects(self): @@ -416,120 +373,41 @@ def test_npm_disabled_rejects(self): self.assertIn("NPM is disabled", str(ctx.exception)) self.assertEqual(self.calls, [], "API must not be called when NPM is disabled") - def test_na_max_now_rejects(self): - # F-2 fail-closed fix: an unreadable platform max ("N/A") must reject - # the request outright, not fall back to a bare "> 0" check that would - # let an unbounded value through in a degraded-driver scenario. + def test_na_max_rejects(self): + # Fail closed: an unreadable platform max must not fall back to a bare > 0 check. with self.assertRaises(self.exceptions_module.AmdSmiInvalidParameterValueException) as ctx: self._validate(requested_limit=250, max_node_power_limit="N/A") self.assertIn("platform maximum is unavailable", str(ctx.exception)) self.assertEqual(self.calls, [], "API must not be called when the platform max is unknown") def test_library_inval_raises_instead_of_returning(self): - # amdsmi_set_npm_limit() itself can still reject a request that passed - # the CLI-side pre-check (e.g. a race against a shrinking platform - # max). AMDSMI_STATUS_INVAL from the library must raise, exiting - # non-zero, not be swallowed into a returned error string/dict. - self.interface.amdsmi_set_npm_limit = lambda h, l: (_ for _ in ()).throw( - _FakeLibraryException(_STATUS_INVAL, "AMDSMI_STATUS_INVAL - Invalid parameters") - ) + # The library can still reject a request that passed the CLI pre-check. + def _raise(_handle, _limit): + raise _FakeLibraryException(_STATUS_INVAL, "AMDSMI_STATUS_INVAL - Invalid parameters") + + self.interface.amdsmi_set_npm_limit = _raise with self.assertRaises(self.exceptions_module.AmdSmiInvalidParameterValueException): self._validate(requested_limit=250, max_node_power_limit=6400) + def test_library_error_records_status(self): + def _raise(_handle, _limit): + raise _FakeLibraryException( + _STATUS_NOT_SUPPORTED, "AMDSMI_STATUS_NOT_SUPPORTED - Feature not supported" + ) -# --------------------------------------------------------------------------- -# Direct tests for amdsmi_helpers.AMDSMIHelpers.get_max_node_power_limit() -# --------------------------------------------------------------------------- - - -class TestGetMaxNodePowerLimit(unittest.TestCase): - """Covers get_max_node_power_limit() (added by ece891b6787), which mirrors - get_power_caps()'s per-device probe loop: it queries devices in order and - returns the first successful reading, falling back to "N/A" only if every - device fails or reports "N/A" itself. - """ - - _EXTRA_STUBBED_NAMES = TestValidateAndSetNodePowerLimit._EXTRA_STUBBED_NAMES - - @classmethod - def setUpClass(cls): - if not _CLI_DIR: - raise unittest.SkipTest("amd-smi CLI source not found") - modules = _install_fake_amdsmi_for_helpers() - modules.update({name: None for name in cls._EXTRA_STUBBED_NAMES}) - stub_modules(cls, modules) - cls.interface = modules["amdsmi.amdsmi_interface"] - - cls._path_added = _CLI_DIR not in sys.path - if cls._path_added: - sys.path.insert(0, _CLI_DIR) - - import amdsmi_helpers as amdsmi_helpers_module - - # staticmethod() wrapping avoids auto-binding this TestCase instance - # as `self`; see TestValidateAndSetNodePowerLimit's setUpClass. - cls.get_max = staticmethod(amdsmi_helpers_module.AMDSMIHelpers.get_max_node_power_limit) - - @classmethod - def tearDownClass(cls): - if getattr(cls, "_path_added", False) and _CLI_DIR in sys.path: - sys.path.remove(_CLI_DIR) - - def _fake_self(self, gpu_handles): - # get_max_node_power_limit() only calls self.get_gpu_handles(); a - # duck-typed stub avoids paying for real device enumeration. - return types.SimpleNamespace(get_gpu_handles=lambda: gpu_handles) - - def test_first_device_success_short_circuits(self): - calls = [] - - def node_handle(dev): - calls.append(dev) - return f"node_{dev}" - - self.interface.amdsmi_get_node_handle = node_handle - self.interface.amdsmi_get_npm_info = lambda _h: {"max_node_power_limit": 6000} - - result = self.get_max(self._fake_self(["dev0", "dev1"])) - - self.assertEqual(result, "6000 W") - self.assertEqual(calls, ["dev0"], "second device must not be probed after a success") - - def test_all_devices_exception_falls_back_to_na(self): - def _raise(_dev): - raise _FakeLibraryException("npm info unavailable") - - self.interface.amdsmi_get_node_handle = _raise - - result = self.get_max(self._fake_self(["dev0", "dev1"])) - - self.assertEqual(result, "N/A") - - def test_na_value_on_first_device_continues_to_next(self): - # Distinct from the exception path: the device call succeeds but - # reports "N/A" as the value itself, so the loop must fall through to - # the next device rather than returning "N/A" early. - def node_handle(dev): - return f"node_{dev}" - - def npm_info(node_handle_): - if node_handle_ == "node_dev0": - return {"max_node_power_limit": "N/A"} - return {"max_node_power_limit": 6400} - - self.interface.amdsmi_get_node_handle = node_handle - self.interface.amdsmi_get_npm_info = npm_info - - result = self.get_max(self._fake_self(["dev0", "dev1"])) + self.interface.amdsmi_set_npm_limit = _raise + result = self._validate(requested_limit=250, max_node_power_limit=6400) - self.assertEqual(result, "6400 W") + self.assertIn("Unable to set node power limit to 250 W", result) + self.assertEqual(self.error_collector.library_codes, [_STATUS_NOT_SUPPORTED]) # --------------------------------------------------------------------------- -# --node-power-limit registration on a non-baremetal (1VF guest) platform +# set parser: --node-power-limit on a 1VF guest, and the --gtt/--gpu guard # --------------------------------------------------------------------------- PARSER_PATH = os.path.join(_CLI_DIR, "amdsmi_parser.py") if _CLI_DIR else "" +_GTT_GPU_CONFLICT = "argument --gtt/-G: not allowed with argument --gpu/-g" def _build_fake_modules_for_parser(): @@ -601,9 +479,6 @@ def is_brcm_switch_initialized(self): def get_power_caps(self): return ("0 W", "550 W", "0 W", "550 W") - def get_max_node_power_limit(self): - return "6000 W" - def get_output_format(self): return "human" @@ -611,7 +486,7 @@ def get_device_handles_from_gpu_selections(self, **_kwargs): return 1, True, [object()] -class TestNodePowerLimitGuestRegistration(unittest.TestCase): +class TestSetValueParser(unittest.TestCase): @classmethod def setUpClass(cls): if not PARSER_PATH or not os.path.isfile(PARSER_PATH): @@ -650,12 +525,8 @@ def test_node_power_limit_registered_on_guest(self): def test_unrelated_gpu_error_keeps_argparse_error(self): parser = argparse.ArgumentParser() - self.parser_mod.AMDSMIParser._guard_gtt_gpu_conflict( - parser, - gtt_flags=("--node-power-limit", "-n"), - reason="--node-power-limit is a node-wide setting, not per-GPU", - ) - argv = ["amd-smi", "set", "--node-power-limit", "100", "--gpu", "0"] + self.parser_mod.AMDSMIParser._guard_gtt_gpu_conflict(parser, gtt_flags=("--gtt", "-G")) + argv = ["amd-smi", "set", "--gtt", "8", "--gpu", "0"] with mock.patch.object(sys, "argv", argv), contextlib.redirect_stderr( io.StringIO() @@ -664,18 +535,12 @@ def test_unrelated_gpu_error_keeps_argparse_error(self): self.assertEqual(raised.exception.code, 2) self.assertIn("argument --cpu", stderr.getvalue()) - self.assertNotIn( - "--node-power-limit/-n: not allowed with argument --gpu/-g", stderr.getvalue() - ) + self.assertNotIn(_GTT_GPU_CONFLICT, stderr.getvalue()) - def test_missing_gpu_value_gets_node_power_limit_error(self): + def test_missing_gpu_value_gets_gtt_error(self): parser = argparse.ArgumentParser() - self.parser_mod.AMDSMIParser._guard_gtt_gpu_conflict( - parser, - gtt_flags=("--node-power-limit", "-n"), - reason="--node-power-limit is a node-wide setting, not per-GPU", - ) - argv = ["amd-smi", "set", "--node-power-limit", "100", "--gpu"] + self.parser_mod.AMDSMIParser._guard_gtt_gpu_conflict(parser, gtt_flags=("--gtt", "-G")) + argv = ["amd-smi", "set", "--gtt", "8", "--gpu"] with mock.patch.object(sys, "argv", argv), contextlib.redirect_stderr( io.StringIO() @@ -684,17 +549,13 @@ def test_missing_gpu_value_gets_node_power_limit_error(self): self.assertEqual(raised.exception.code, 2) self.assertIn( - "--node-power-limit/-n: not allowed with argument --gpu/-g", stderr.getvalue() + _GTT_GPU_CONFLICT + " (--gtt is a system-wide setting, not per-GPU)", stderr.getvalue() ) def test_missing_unrelated_value_keeps_argparse_error(self): parser = argparse.ArgumentParser() - self.parser_mod.AMDSMIParser._guard_gtt_gpu_conflict( - parser, - gtt_flags=("--node-power-limit", "-n"), - reason="--node-power-limit is a node-wide setting, not per-GPU", - ) - argv = ["amd-smi", "set", "--node-power-limit", "100", "--gpu", "0", "--power-cap"] + self.parser_mod.AMDSMIParser._guard_gtt_gpu_conflict(parser, gtt_flags=("--gtt", "-G")) + argv = ["amd-smi", "set", "--gtt", "8", "--gpu", "0", "--power-cap"] with mock.patch.object(sys, "argv", argv), contextlib.redirect_stderr( io.StringIO() @@ -703,6 +564,4 @@ def test_missing_unrelated_value_keeps_argparse_error(self): self.assertEqual(raised.exception.code, 2) self.assertIn("argument -o/--power-cap", stderr.getvalue()) - self.assertNotIn( - "--node-power-limit/-n: not allowed with argument --gpu/-g", stderr.getvalue() - ) + self.assertNotIn(_GTT_GPU_CONFLICT, stderr.getvalue()) diff --git a/projects/amdsmi/tests/python/unit/gpu/test_npm_info_current_node_power.py b/projects/amdsmi/tests/python/unit/gpu/test_npm_info_current_node_power.py index fecf711fc904..b9ac0b7c9943 100644 --- a/projects/amdsmi/tests/python/unit/gpu/test_npm_info_current_node_power.py +++ b/projects/amdsmi/tests/python/unit/gpu/test_npm_info_current_node_power.py @@ -4,26 +4,14 @@ """Unit tests for the "current_node_power" key in amdsmi_get_npm_info()'s dict. -current_node_power moved from amdsmi_power_info_t (per-GPU, amd-smi metric ---power) to amdsmi_npm_info_t (per-node, amd-smi node -p / -amdsmi_get_npm_info()) -- see the "Set npm power limit" design doc. This file -replaces the previous test_power_info_node_power.py, which exercised the -now-removed amdsmi_power_info_t.node_power field. - -Uses the real compiled amdsmi package (same resolution as test_check_res.py / -test_apu_metrics.py, via `from common.common import amdsmi`) but mocks the -underlying ctypes call (`amdsmi_wrapper.amdsmi_get_npm_info`) to populate an -`amdsmi_npm_info_t` struct directly -- no real hardware/root access needed. - -Per the frozen contract, `amdsmi_npm_info_t` gained a new `current_node_power` -uint32 field (right after `max_node_power_limit`), and the Python dict -returned by `amdsmi_get_npm_info()` gained a matching `"current_node_power"` -key that follows the same UINT32_MAX -> "N/A" sentinel convention already -used for `"ubb_power_threshold"`/`"max_node_power_limit"` (see -`_validate_if_max_uint` usage in `amdsmi_get_npm_info()`, amdsmi_interface.py). -""" +Uses the real compiled amdsmi package (via `from common.common import amdsmi`) +but mocks the ctypes call (`amdsmi_wrapper.amdsmi_get_npm_info`), so no +hardware or root is needed. -from __future__ import annotations +`current_node_power` is a new uint32 field in `amdsmi_npm_info_t` (after +`max_node_power_limit`) and follows the same UINT32_MAX -> "N/A" convention as +`"ubb_power_threshold"`. +""" import ctypes import unittest diff --git a/projects/amdsmi/tests/python/unit/gpu/test_npm_set_limit.py b/projects/amdsmi/tests/python/unit/gpu/test_npm_set_limit.py index 69322f67e87c..96618dfd5c7b 100644 --- a/projects/amdsmi/tests/python/unit/gpu/test_npm_set_limit.py +++ b/projects/amdsmi/tests/python/unit/gpu/test_npm_set_limit.py @@ -4,28 +4,12 @@ """Unit tests for the amdsmi_set_npm_limit() Python wrapper. -Uses the real compiled amdsmi package (resolved the same way test_check_res.py -and test_apu_metrics.py do, via `from common.common import amdsmi`), but mocks -the underlying ctypes call (`amdsmi_wrapper.amdsmi_set_npm_limit`) so no real -hardware/root access is required -- these tests exercise only the Python-level -parameter validation and status-to-exception mapping added around the new C -API, per the frozen contract: - - def amdsmi_set_npm_limit(node_handle: processor_handle_t, limit: int) -> None - -which: - * raises AmdSmiParameterException if node_handle is not an - amdsmi_wrapper.amdsmi_node_handle instance - * raises AmdSmiParameterException if limit is not an int - * otherwise calls amdsmi_wrapper.amdsmi_set_npm_limit(node_handle, - ctypes.c_uint64(limit)) and funnels the returned status code through the - standard _check_res() mapping (AmdSmiLibraryException with the propagated - error code on any non-SUCCESS status, e.g. AMDSMI_STATUS_NO_PERM or - AMDSMI_STATUS_NOT_SUPPORTED; no exception on AMDSMI_STATUS_SUCCESS) +Uses the real compiled amdsmi package (via `from common.common import amdsmi`) +but mocks the ctypes call (`amdsmi_wrapper.amdsmi_set_npm_limit`), so no +hardware or root is needed. Covers the local parameter checks (node handle +type; limit an int, not bool, in 1..2**64-1) and the _check_res() status mapping. """ -from __future__ import annotations - import ctypes import unittest from unittest import mock @@ -59,6 +43,12 @@ def test_rejects_float_limit(self): with self.assertRaises(amdsmi.AmdSmiParameterException): amdsmi.amdsmi_set_npm_limit(node_handle, 100.5) + def test_rejects_bool_limit(self): + # bool is an int subclass; True must not be written as a 1 W limit. + node_handle = _make_node_handle() + with self.assertRaises(amdsmi.AmdSmiParameterException): + amdsmi.amdsmi_set_npm_limit(node_handle, True) + def test_rejects_negative_limit(self): # A Python int can represent negative values a raw ctypes.c_uint64() # cannot (it would silently wrap modulo 2**64); the explicit range