Integrate Broadcom Devices in amd-smi framework #71

mcharles-brcm · 2025-01-26T08:29:19Z

As part of this feature,

Added BRCM devices in amd-smi framework
The affected command is 'amd-smi list'
Discovered and added BRCM's NIC and SWITCH devices in the list
The discovery is specific to sysfs entries, which is created by BRCM driver. Hence, without BRCM driver the necessary NIC and SWITCH devices will not be listed in the list command
As of now added BFD and UUID elements added as part of the individual devices

Updated (1/27)

Added BRCM devices as part of amd-smi monitor command
Very limited monitor attributes are added as part of this feature
The affected command is 'amd-smi monitor'
If hwmon sysfs entry not created for the BRCM NIC devices then those controllers are removed from the monitor command
NIC's Monitor Attributes:
i) NIC_TEMP_CURRENT
ii) NIC_TEMP_CRIT_ALARM
iii) NIC_TEMP_EMERGENCY_ALARM
iv) NIC_TEMP_SHUTDOWN_ALARM
v) NIC_TEMP_MAX_ALARM
SWITCH's Monitor Attributes:
i) CURRENT_LINK_SPEED
ii) MAX_LINK_SPEED
iii) CURRENT_LINK_WIDTH
iv) MAX_LINK_WIDTH

Signed-off-by: Joseph Narlo <[email protected]>

1. As part of this we have added Broadcom's NIC and SWITCH devices in the amd-smi framework. 2. The affected command is 'amd-smi list' 3. Filter the NIC and SWITCH devices based on the Broadcom's vendor ids 4. Since these changes are depending on sysfs entries which is created by broadcom drivers, if no brcm driver present in the system then necessary elements will not created as part of the list command.

…MD-ROCm-Internal/amdsmi into SWDEV-504389/Synch_Comment_In_Linux_BM

Comments added

NIC/Switch LIST

List Nic/Switch

As part of this feature, 1. Added monitor attributes for BRCM devices - NIC and SWITCH 2. The affected amd-smi command is 'amd-smi monitor' 3. The list of monitor attributes are read from the corresponding BRCM device's sysfs path

BRCM Monitor

…imestamp from firmware. (ROCm#61) * SWDEV-511296 - update violation_status->violation_timestamp to read values from firmware. Signed-off-by: Greg Scaffidi <[email protected]> * SWDEV-511296 - update violation_status->violation_timestamp to read values from firmware. Signed-off-by: Greg Scaffidi <[email protected]> * SWDEV-511296 - update violation_status->violation_timestamp to read values from firmware. Signed-off-by: Greg Scaffidi <[email protected]> --------- Signed-off-by: Greg Scaffidi <[email protected]>

Fix ordering of RHEL 8 build process Signed-off-by: Williams, Justin <[email protected]>

dmitrii-galantsev · 2025-02-21T09:09:38Z

ohhh that's cool, I'll ask some teammates to see if they want to take on broadcom things.

continue internally

Affected Commands: 1. amd-smi topology -nic Display nic and gpu connectivity 2. amd-smi topology -show_numa Display nic,gpu's numa and cpu affinity

1. Remove duplicate declaration 2. Resolve Alignment issue

bill-shuzhou-liu · 2025-05-09T15:45:53Z

include/amd_smi/amdsmi.h

@@ -994,6 +996,25 @@ typedef struct {
 *
 * @cond @tag{gpu_bm_linux} @tag{host} @endcond
 */
+typedef struct {


add comment to explain the difference of those temperature

Added the comment statements to the methods.

bill-shuzhou-liu · 2025-05-09T15:46:36Z

include/amd_smi/amdsmi.h

+    uint32_t nic_temp_input;
+    uint32_t nic_temp_max;
+    uint32_t nic_temp_shutdown;
+} amdsmi_nic_temperature_metric_t;


if it is Broadcom specific metrics, maybe we should use the amdsmi_brcm_nic_temperature_metric_t?

renamed as amdsmi_brcm_nic_temperature_metric_t

bill-shuzhou-liu · 2025-05-09T15:46:52Z

include/amd_smi/amdsmi.h

+} amdsmi_nic_temperature_metric_t;
+
+typedef struct {
+    char current_link_speed[AMDSMI_MAX_STRING_LENGTH];


I will assume this is the integer or float, and not string?

As per BRCM NIC driver implementation the complete string added in the sysfs entry and the same populated in amd-smi.

No code changes made.

bill-shuzhou-liu · 2025-05-09T15:48:46Z

include/amd_smi/amdsmi.h

@@ -2951,7 +2972,17 @@ amdsmi_status_t amdsmi_get_gpu_bdf_id(amdsmi_processor_handle processor_handle,
 *  @return ::amdsmi_status_t | ::AMDSMI_STATUS_SUCCESS on success, non-zero on fail
 */
 amdsmi_status_t amdsmi_get_gpu_topo_numa_affinity(amdsmi_processor_handle processor_handle, int32_t *numa_node);
-
+amdsmi_status_t amdsmi_get_gpu_topo_cpu_affinity(amdsmi_processor_handle processor_handle, 


this file is user facing API, we may want to reduce the amount of API. In this case, we have cpu/gpu/nic, maybe just one API with a enum parameter to specify the type we want to use?

Here the idea is do not want to conflict with gpu implementation.
In addition to that to identify the devices in python interface using the corresponding wrapper methods.

No code changes made.

bill-shuzhou-liu · 2025-05-09T15:49:17Z

include/amd_smi/amdsmi.h

-
+amdsmi_status_t amdsmi_get_gpu_topo_cpu_affinity(amdsmi_processor_handle processor_handle, 
+                                                    unsigned int *cpu_aff_length, char *cpu_aff_data);
+amdsmi_status_t amdsmi_get_nic_topo_numa_affinity(amdsmi_processor_handle processor_handle, int32_t *numa_node);


We need to add comment for API defined in this file, so that doxygen can generate proper document.

Updated the comment and deliver the changes.

bill-shuzhou-liu · 2025-05-09T16:04:57Z

src/amd_smi/amd_smi_lspci_commands.cc

+    char buffer[128];
+
+    // Open a pipe to execute the command
+    std::shared_ptr<FILE> pipe(popen(command.c_str(), "r"), pclose);


we should try to avoid call external command if possible as this is a library. maybe you can refer to some code here:
https://github.com/pciutils/pciutils/blob/master/lspci.c

Like lspci command, we are planning to add ethtool, sg_ses commands also to populate the nic and switch devices details. In this case, we are believing this is better approach to get the necessary information.

No code changes made.

bill-shuzhou-liu · 2025-05-09T16:28:26Z

include/amd_smi/impl/amd_smi_no_drm_nic.h

+namespace amd {
+namespace smi {
+
+class AMDSmiNoDrmNIC {


can you explain what is no drm nic? I think drm is related to GPU only? In amdsmi, some optional function needs to refer the libdrm, but we do not want to add the hard dependency to it, so we dynamically load it.

Removed the dependency "xf86drm.h" and delivered the changes. Other than GPU's base class referred as no_drm. It can be NIC and Switch.

bill-shuzhou-liu · 2025-05-09T16:40:17Z

rocm_smi/include/rocm_smi/rocm_smi.h

@@ -1500,6 +1500,8 @@ rsmi_status_t rsmi_driver_status(rsmi_driver_state_t* state);
 *  @retval ::RSMI_STATUS_SUCCESS is returned upon successful call.
 */
 rsmi_status_t rsmi_num_monitor_devices(uint32_t *num_devices);
+rsmi_status_t rsmi_num_nic_monitor_devices(uint32_t *num_devices);


add comments for public facing header so that the doxygen can generate API document properly.

Updated the comments and deliver the changes.

bill-shuzhou-liu · 2025-05-09T16:43:04Z

src/amd_smi/amd_smi.cc

@@ -562,38 +642,74 @@ amdsmi_get_gpu_device_bdf(amdsmi_processor_handle processor_handle, amdsmi_bdf_t
    return AMDSMI_STATUS_SUCCESS;
 }

-amdsmi_status_t


Not sure why we need to remove this function?

This difference due to merge conflict and it was resolved.

bill-shuzhou-liu · 2025-05-09T16:44:14Z

src/amd_smi/amd_smi_drm.cc

@@ -324,6 +325,13 @@ amdsmi_status_t AMDSmiDrm::get_bdf_by_index(uint32_t gpu_index, amdsmi_bdf_t *bd
    return AMDSMI_STATUS_SUCCESS;
 }

+amdsmi_status_t AMDSmiDrm::amdgpu_query_cpu_affinity(std::string devicePath, std::string &cpu_affinity) {
+  std::string cpuAffFile = "cpulistaffinity";
+  cpu_affinity = smi_brcm_get_value_string(devicePath, cpuAffFile);


so it will never fail?

Yes. This is handled in amd_smi_utils.cc. If file not present returning as "N/A".
Hence, expected return value is always success.

bill-shuzhou-liu · 2025-05-09T20:58:47Z

example/amd_smi_list.cc

+    uint32_t socket_count = 0;
+
+    // Initialize amdsmi for AMD CPUs
+    ret = amdsmi_init(AMDSMI_INIT_AMD_GPUS);


if you only init the amdsmi to discover GPU device, then you should not be able to find out the NIC device in the following amdsmi_get_processor_handles_by_type()

if you want to have an example of code to support both NIC and GPU device, you may need to pass both as parameter.

As part of AMDSMI_INIT_AMD_GPUS only BRCM NIC and Switch flags are initialized.

bill-shuzhou-liu · 2025-05-09T21:05:35Z

rocm_smi/src/rocm_smi_main.cc

+      * Construct the BDF for the discovered NIC devices 
+      * Push NIC SMI Device object to nic_devices vector
+    */
+    ret = DiscoverBRCMnicDevices();


If a system does not have the NIC device, we should not fail the whole init() as the user may only care about the GPU.

Handled graceful exit in DiscoverBRCMnicDevices and DiscoverBRCMswitchDevices and make sure always return 0.

bill-shuzhou-liu · 2025-05-09T21:06:59Z

rocm_smi/src/rocm_smi_main.cc

+    uint64_t bdfid;
+    for (auto &device : nic_devices_) {
+      if (ConstructBDFID(device->path(), &bdfid) != 0) {
+        std::cerr << "Failed to construct BDFID." << std::endl;


similarly, any NIC device init fail should not fail the whole function as people may just want to work on GPU.

Handled dead code.

bill-shuzhou-liu · 2025-05-09T21:09:04Z

rocm_smi/src/rocm_smi_main.cc

@@ -653,7 +780,63 @@ void RocmSMI::AddToDeviceList(std::string dev_name, uint64_t bdfid) {
  LOG_DEBUG(ss);
 }

+void RocmSMI::AddToNICDeviceList(std::string dev_name, uint64_t bdfid, uint32_t card_indx) {


A lot of code is similar to AddToSWITCHDeviceList(), consider to remove duplicate code.

Since both are two different elements do not want to make function overload.
Moreover, there is a chance to have a server with any one of the BRCM device - either BRCM NIC or BRCM Switch.

bill-shuzhou-liu · 2025-05-09T21:10:57Z

rocm_smi/src/rocm_smi_main.cc

@@ -691,6 +874,84 @@ static bool isAMDGPU(std::string dev_path) {
  return isAmdGpu;
 }

+static bool isBRCMnic(std::string dev_path) {


code is similar to isBRCMswitch(), consider to remove duplicate code.

The OEM, device id and device's sysfs paths are different and handle multiple if else will not be looks good.
No code changes made.

bill-shuzhou-liu · 2025-05-09T21:12:22Z

rocm_smi/src/rocm_smi_main.cc

@@ -1096,6 +1357,142 @@ uint32_t RocmSMI::DiscoverAmdgpuDevices(void) {
  return 0;
 }

+uint32_t RocmSMI::DiscoverBRCMnicDevices(void) {


similar code to DiscoverBRCMswitchDevices(), remove duplication

Since both are two different elements do not want to make function overload.
Moreover, there is a chance to have a server with any one of the BRCM device - either BRCM NIC or BRCM Switch.

bill-shuzhou-liu · 2025-05-12T13:04:15Z

include/amd_smi/amdsmi.h

+amdsmi_status_t amdsmi_get_gpu_topo_cpu_affinity(amdsmi_processor_handle processor_handle, 
+                                                    unsigned int *cpu_aff_length, char *cpu_aff_data);
+amdsmi_status_t amdsmi_get_nic_topo_numa_affinity(amdsmi_processor_handle processor_handle, int32_t *numa_node);
+amdsmi_status_t amdsmi_get_nic_topo_cpu_affinity(amdsmi_processor_handle processor_handle, 


For any new API defined in the amdsmi.h, we need a unit test, you can refer to the files at tests/amd_smi_test/ folders.

bill-shuzhou-liu · 2025-05-12T13:09:09Z

rocm_smi/src/rocm_smi_main.cc


 static const char *kDeviceNamePrefix = "card";
+static const char *kNICPrefix = "e"; //changed to 'e' since SLES syspath is 'eth' and RHEL/Ubuntu is 'en'


wl for wireless LAN (e.g., wlp2s0). But it may not be applied to our case, so we can still use this.

Yes. Moreover, we are filtering by specific vendor / driver supported nic devices only.

bill-shuzhou-liu · 2025-05-12T13:10:30Z

rocm_smi/src/rocm_smi_main.cc

+     * Push SWITCH SMI Device object to switch_devices vector
+    */
+
+    ret = DiscoverBRCMswitchDevices();


The below switch code is very similar to handle BRCM NIC devices. Consider removing duplications.

Since both are two different elements do not want to make function overload.
Moreover, there is a chance to have a server with any one of the BRCM device - either BRCM NIC or BRCM Switch.

bill-shuzhou-liu · 2025-05-12T13:11:42Z

rocm_smi/src/rocm_smi_main.cc

+    err_msg += kPathSwitchRoot;
+    err_msg += ".";
+    perror(err_msg.c_str());
+    return 1;


instead of using hardcode number, we may prefer to use the macro to make the code more readable.

Followed same as DiscoverAmdgpuDevices implementation.

bill-shuzhou-liu · 2025-05-12T13:13:38Z

rocm_smi/src/rocm_smi_main.cc

+    auto path_dir = opendir(path.c_str());
+    if (path_dir == nullptr) {
+      //move to the next index
+      if (numofretry == 3) continue;


how do we come up to use the hardcode number 3 instead of other number for retry?

To handle the negative test scenario added this case - the test server had only one invalid sysfs entry and the execution failed.
Note: In the good scenario the retry count is not required.
For safer side added the retry count as 3. In general we have user editable configuration file for feeding these values, but not sure that can be applicable for amd-smi.

bill-shuzhou-liu · 2025-05-12T14:00:48Z

src/amd_smi/amd_smi_system.cc

@@ -247,6 +251,122 @@ amdsmi_status_t AMDSmiSystem::populate_amd_gpu_devices() {
    return AMDSMI_STATUS_SUCCESS;
 }

+amdsmi_status_t AMDSmiSystem::populate_brcm_nic_devices() {
+    uint32_t device_count = 0;
+    amdsmi_status_t amd_smi_status = no_drm_nic.init();


do you want to check the return status?

bill-shuzhou-liu · 2025-05-12T14:02:14Z

src/amd_smi/amd_smi_system.cc

+  return AMDSMI_STATUS_SUCCESS;
+}
+
+amdsmi_status_t AMDSmiSystem::populate_brcm_switch_devices() {


consider combining with populate_brcm_nic_devices() to remove duplicate code.

Since both are two different elements do not want to make function overload.
Moreover, there is a chance to have a server with any one of the BRCM device - either BRCM NIC or BRCM Switch.

bill-shuzhou-liu · 2025-05-12T14:07:06Z

src/amd_smi/amd_smi_utils.cc

+  }
+  else {
+    std::string line;
+    getline(file, line);


handle the errors as this function may throw exceptions.

bill-shuzhou-liu · 2025-05-12T14:07:29Z

src/amd_smi/amd_smi_utils.cc

+  else {
+    std::string line;
+    getline(file, line);
+    return static_cast<uint32_t>(stoi(line));


if the line is not number, what will happen?

bill-shuzhou-liu · 2025-05-12T14:07:46Z

src/amd_smi/amd_smi_utils.cc

+    return "N/A";
+  }
+  else {
+    getline(file, temp);


handle error

bill-shuzhou-liu · 2025-05-12T14:09:57Z

example/CMakeLists.txt

@@ -63,6 +63,11 @@ add_executable(${SMI_NODRM_EXAMPLE_EXE} "amd_smi_nodrm_example.cc")
 target_link_libraries(${SMI_NODRM_EXAMPLE_EXE} amd_smi)
 add_dependencies(${SMI_NODRM_EXAMPLE_EXE} amd_smi)

+set(SMI_LIST_EXAMPLE_EXE "amd_smi_list")


if you just want to show to use the nic device, maybe using the name amd_smi_nic.cc instead.

Updated the amd_smi_list example code to print all GPU, NIC and Switch devices BDF and UUID details.

Remove duplicate methods from merges

Incorporated review comments.

Incorporated Review Comments Phase 2.1

jcnarlo and others added 10 commits January 23, 2025 11:34

[SWDEV-504583] Resolve Additional Compiler Warnings

33679ac

Signed-off-by: Joseph Narlo <[email protected]>

[SWDEV-504583] Resolve Additional Compiler Warnings (ROCm#59)

dd2ce07

Merge branch 'amd-staging' into SWDEV-504389/Synch_Comment_In_Linux_BM

e5f94bd

Merge branch 'amd-staging' into SWDEV-504389/Synch_Comment_In_Linux_BM

1d305dc

Merge branch 'SWDEV-504389/Synch_Comment_In_Linux_BM' of github.com:A…

b0f2a3c

…MD-ROCm-Internal/amdsmi into SWDEV-504389/Synch_Comment_In_Linux_BM

[SWDEV-504389] Sync Comments in Linux BM (ROCm#63)

e18c0c2

Comments added

Update amdsmi_logger.py

67d7ee7

NIC/Switch LIST

aea6e8a

NIC/Switch LIST

Merge pull request #1 from shbiradar2025/amd-staging

e7637e8

List Nic/Switch

mcharles-brcm requested review from marifamd, bill-shuzhou-liu, dmitrii-galantsev, charis-poag-amd and oliveiradan as code owners January 26, 2025 08:29

mcharles-brcm and others added 8 commits January 26, 2025 14:17

Merge branch 'ROCm:amd-staging' into amd-staging

e933b64

brcm devices monitor command

358b2d2

As part of this feature, 1. Added monitor attributes for BRCM devices - NIC and SWITCH 2. The affected amd-smi command is 'amd-smi monitor' 3. The list of monitor attributes are read from the corresponding BRCM device's sysfs path

BRCM Monitor

2949a5a

BRCM Monitor

Monitor Logger

dbc802d

updated interface

f6061c6

Merge pull request #2 from shbirada-brcm/amd-staging

b6984b3

BRCM Monitor

Merge branch 'ROCm:amd-staging' into amd-staging

d32bb1a

rahulc-gh pushed a commit that referenced this pull request Jan 30, 2025

[SWDEV-500518] RHEL8 fix (#71)

782dd07

Fix ordering of RHEL 8 build process Signed-off-by: Williams, Justin <[email protected]>

dmitrii-galantsev force-pushed the amd-staging branch from 9872083 to 09379f8 Compare February 6, 2025 03:09

Merge branch 'amd-staging' into amd-staging

ac5b8f8

mcharles-brcm added 3 commits March 13, 2025 12:42

Integrate BRCM devices in topology command

04d9f51

Affected Commands: 1. amd-smi topology -nic Display nic and gpu connectivity 2. amd-smi topology -show_numa Display nic,gpu's numa and cpu affinity

Merge branch 'amd-staging' into amd-staging

09e67e5

Remove duplicate entries issues

eccd034

1. Remove duplicate declaration 2. Resolve Alignment issue

mcharles-brcm added 8 commits March 13, 2025 16:05

Resolve duplicate entries in python

7b11bb3

Merge branch 'amd-staging' into amd-staging

a748f01

Merge branch 'ROCm:amd-staging' into amd-staging

8a9842a

Merge branch 'ROCm:amd-staging' into amd-staging

cbc1fdb

Merge branch 'amd-staging' into amd-staging

827f42c

Merge branch 'amd-staging' into amd-staging

f65f278

Merge branch 'amd-staging' into amd-staging

46efd34

Update amdsmi_commands.py

cb30a00

dmitrii-galantsev force-pushed the amd-staging branch from 234704f to bd82e88 Compare May 7, 2025 06:55

bill-shuzhou-liu reviewed May 9, 2025

View reviewed changes

bill-shuzhou-liu reviewed May 12, 2025

View reviewed changes

mcharles-brcm added 3 commits May 13, 2025 20:19

Merge branch 'amd-staging' into amd-staging

47f0ad9

Remove duplicate methods from merges

ded8146

Remove duplicate methods from merges

Merge branch 'ROCm:amd-staging' into amd-staging

789cf6f

mcharles-brcm requested a review from bill-shuzhou-liu May 20, 2025 05:31

mcharles-brcm added 6 commits May 20, 2025 16:26

Review Comments Phase - 1

cd2bada

Incorporated review comments.

Merge branch 'amd-staging' into amd-staging

a936fc1

Merge branch 'ROCm:amd-staging' into amd-staging

2c53e54

Review Comments Phase 2.1

62c8880

Incorporated Review Comments Phase 2.1

Update sys_info_read.cc

a0a5900

Update sys_info_read.cc

bfff8d5


		static const char *kDeviceNamePrefix = "card";
		static const char *kNICPrefix = "e"; //changed to 'e' since SLES syspath is 'eth' and RHEL/Ubuntu is 'en'

Integrate Broadcom Devices in amd-smi framework #71

Are you sure you want to change the base?

Integrate Broadcom Devices in amd-smi framework #71

Uh oh!

Conversation

mcharles-brcm commented Jan 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmitrii-galantsev commented Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mcharles-brcm commented Jan 26, 2025 •

edited

Loading

dmitrii-galantsev commented Feb 21, 2025 •

edited

Loading