[v7,2/8] ethdev: add telemetry cmd for registers

This patch adds a telemetry command for registers dump,
and supports obtaining the registers of a specified module.

In one way, the number of registers that can be exported
is limited by the number of elements carried by dict and
container. In another way, the length of the string
exported by telemetry is limited by MAX_OUTPUT_LEN.
Therefore, when the number of registers to be exported
exceeds, some information will be lost. Warn on the former
case.

An example usage is shown below:
--> /ethdev/regs,0,ring
{
  "/ethdev/regs": {
    "registers_length": 318,
    "registers_width": 4,
    "register_offset": "0x0",
    "version": "0x1140011",
    "group_0": {
      "Q0_ring_rx_bd_num": "0x0",
      "Q0_ring_rx_bd_len": "0x0",
      ...
      },
    "group_1": {
        ...
    },
    ...
}

Signed-off-by: Jie Hai <haijie1@huawei.com>
---
 lib/ethdev/rte_ethdev_telemetry.c | 128 ++++++++++++++++++++++++++++++
 1 file changed, 128 insertions(+)
  

On 2024/9/14 15:13, Jie Hai wrote:
> This patch adds a telemetry command for registers dump,
> and supports obtaining the registers of a specified module.
> 
> In one way, the number of registers that can be exported
> is limited by the number of elements carried by dict and
> container. In another way, the length of the string
> exported by telemetry is limited by MAX_OUTPUT_LEN.
> Therefore, when the number of registers to be exported
> exceeds, some information will be lost. Warn on the former
> case.
> 
> An example usage is shown below:
> --> /ethdev/regs,0,ring
> {
>   "/ethdev/regs": {
>     "registers_length": 318,
>     "registers_width": 4,
>     "register_offset": "0x0",
>     "version": "0x1140011",
>     "group_0": {
>       "Q0_ring_rx_bd_num": "0x0",
>       "Q0_ring_rx_bd_len": "0x0",
>       ...
>       },
>     "group_1": {
>         ...
>     },
>     ...
> }
> 
> Signed-off-by: Jie Hai <haijie1@huawei.com>
> ---
>  lib/ethdev/rte_ethdev_telemetry.c | 128 ++++++++++++++++++++++++++++++
>  1 file changed, 128 insertions(+)
> 
> diff --git a/lib/ethdev/rte_ethdev_telemetry.c b/lib/ethdev/rte_ethdev_telemetry.c
> index 6b873e7abe68..1d59c693883e 100644
> --- a/lib/ethdev/rte_ethdev_telemetry.c
> +++ b/lib/ethdev/rte_ethdev_telemetry.c
> @@ -1395,6 +1395,132 @@ eth_dev_handle_port_tm_node_caps(const char *cmd __rte_unused,
>  	return ret;
>  }
>  
> +static void
> +eth_dev_add_reg_data(struct rte_tel_data *d, struct rte_dev_reg_info *reg_info,
> +		     uint32_t idx)
> +{
> +	if (reg_info->width == sizeof(uint32_t))
> +		rte_tel_data_add_dict_uint_hex(d, reg_info->names[idx].name,
> +			*((uint32_t *)reg_info->data + idx), 0);
> +	else
> +		rte_tel_data_add_dict_uint_hex(d, reg_info->names[idx].name,
> +			*((uint64_t *)reg_info->data + idx), 0);
> +}
> +
> +static int
> +eth_dev_store_regs(struct rte_tel_data *d, struct rte_dev_reg_info *reg_info)
> +{
> +	struct rte_tel_data *groups[RTE_TEL_MAX_DICT_ENTRIES];
> +	char group_name[RTE_TEL_MAX_STRING_LEN] = {0};
> +	struct rte_tel_data *group = NULL;
> +	uint32_t grp_num = 0;
> +	uint32_t i;
> +	int ret;
> +
> +	rte_tel_data_start_dict(d);
> +	rte_tel_data_add_dict_uint(d, "register_length", reg_info->length);
> +	rte_tel_data_add_dict_uint(d, "register_width", reg_info->width);
> +	rte_tel_data_add_dict_uint_hex(d, "register_offset", reg_info->offset, 0);
> +	rte_tel_data_add_dict_uint_hex(d, "version", reg_info->version, 0);
> +
> +	for (i = 0; i < reg_info->length; i++) {
> +		if (i % RTE_TEL_MAX_DICT_ENTRIES != 0) {
> +			eth_dev_add_reg_data(group, reg_info, i);
> +			continue;
> +		}
> +
> +		group = rte_tel_data_alloc();
> +		if (group == NULL) {
> +			ret = -ENOMEM;
> +			RTE_ETHDEV_LOG_LINE(WARNING, "No enough memory for group data");
> +			goto out;
> +		}
> +		groups[grp_num++] = group;

Although the probability is very small, the groups array may overflow if the length is very large.

Suggest add judgement at the begin of this function:

uint32_t max_cap = (RTE_TEL_MAX_DICT_ENTRIES - 4) * RTE_TEL_MAX_DICT_ENTRIES;   // it will be 65000+ register
if (reg_info->length > max_cap) {
    // just output error and exit
}

> +		rte_tel_data_start_dict(group);
> +		eth_dev_add_reg_data(group, reg_info, i);
> +	}
> +
> +	for (i = 0; i < grp_num; i++) {
> +		snprintf(group_name, RTE_TEL_MAX_STRING_LEN, "group_%u", i);
> +		ret = rte_tel_data_add_dict_container(d, group_name, groups[i], 0);
> +		if (ret == -ENOSPC) {
> +			RTE_ETHDEV_LOG_LINE(WARNING,
> +				"Reduce register number to be displayed from %u to %u due to limited capacity of telemetry",
> +				reg_info->length, i * RTE_TEL_MAX_DICT_ENTRIES);
> +			break;
> +		}

It may lead to memory leak when break. If we add such judgement before, there is no need to add such judgement here.

> +	}
> +	return 0;
> +out:
> +	for (i = 0; i < grp_num; i++)
> +		rte_tel_data_free(groups[i]);
> +
> +	return ret;
> +}
> +
> +static int
> +eth_dev_get_port_regs(int port_id, struct rte_tel_data *d, char *filter)
> +{
> +	struct rte_dev_reg_info reg_info;
> +	int ret;
> +
> +	memset(&reg_info, 0, sizeof(reg_info));
> +	reg_info.filter = filter;
> +
> +	ret = rte_eth_dev_get_reg_info_ext(port_id, &reg_info);
> +	if (ret != 0) {
> +		RTE_ETHDEV_LOG_LINE(ERR, "Error getting device reg info: %d", ret);

how about "Failed to get device reg info, ret = %d"

> +		return ret;
> +	}
> +
> +	reg_info.data = calloc(reg_info.length, reg_info.width);
> +	if (reg_info.data == NULL) {
> +		RTE_ETHDEV_LOG_LINE(ERR, "Fail to allocate memory for reg_info.data");
> +		return -ENOMEM;
> +	}
> +
> +	reg_info.names = calloc(reg_info.length, sizeof(struct rte_eth_reg_name));
> +	if (reg_info.names == NULL) {
> +		RTE_ETHDEV_LOG_LINE(ERR, "Fail to allocate memory for reg_info.names");
> +		free(reg_info.data);
> +		return -ENOMEM;
> +	}
> +
> +	ret = rte_eth_dev_get_reg_info_ext(port_id, &reg_info);
> +	if (ret != 0) {
> +		RTE_ETHDEV_LOG_LINE(ERR, "Error getting regs from device: %d", ret);

It seemed the ret is device id, how about "Failed to get device reg info, ret = %d"

> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	ret = eth_dev_store_regs(d, &reg_info);
> +out:
> +	free(reg_info.data);
> +	free(reg_info.names);
> +
> +	return ret;
> +}
> +
> +static int
> +eth_dev_handle_port_regs(const char *cmd __rte_unused,
> +		const char *params,
> +		struct rte_tel_data *d)
> +{
> +	char *filter, *end_param;
> +	uint16_t port_id;
> +	int ret;
> +
> +	ret = eth_dev_parse_port_params(params, &port_id, &end_param, true);
> +	if (ret != 0)
> +		return ret;
> +
> +	filter = strtok(end_param, ",");
> +	if (filter != NULL && strlen(filter) == 0)
> +		filter = NULL;
> +
> +	return eth_dev_get_port_regs(port_id, d, filter);
> +}
> +
>  RTE_INIT(ethdev_init_telemetry)
>  {
>  	rte_telemetry_register_cmd("/ethdev/list", eth_dev_handle_port_list,
> @@ -1436,4 +1562,6 @@ RTE_INIT(ethdev_init_telemetry)
>  			"Returns TM Level Capabilities info for a port. Parameters: int port_id, int level_id (see tm_capability for the max)");
>  	rte_telemetry_register_cmd("/ethdev/tm_node_capability", eth_dev_handle_port_tm_node_caps,
>  			"Returns TM Node Capabilities info for a port. Parameters: int port_id, int node_id (see tm_capability for the max)");
> +	rte_telemetry_register_cmd("/ethdev/regs", eth_dev_handle_port_regs,
> +			"Returns all or filtered registers info for a port. Parameters: int port_id, string module_name (Optional if show all)");
>  }
  

On 9/14/2024 8:13 AM, Jie Hai wrote:
> This patch adds a telemetry command for registers dump,
> and supports obtaining the registers of a specified module.
> 
> In one way, the number of registers that can be exported
> is limited by the number of elements carried by dict and
> container. In another way, the length of the string
> exported by telemetry is limited by MAX_OUTPUT_LEN.
> Therefore, when the number of registers to be exported
> exceeds, some information will be lost. Warn on the former
> case.
> 
> An example usage is shown below:
> --> /ethdev/regs,0,ring
> {
>   "/ethdev/regs": {
>     "registers_length": 318,
>     "registers_width": 4,
>     "register_offset": "0x0",
>     "version": "0x1140011",
>     "group_0": {
>       "Q0_ring_rx_bd_num": "0x0",
>       "Q0_ring_rx_bd_len": "0x0",
>       ...
>       },
>     "group_1": {
>         ...
>     },
>     ...
> }
> 
> Signed-off-by: Jie Hai <haijie1@huawei.com>
>

Reviewed-by: Ferruh Yigit <ferruh.yigit@amd.com>
  

Hi, fengchengwen,

Thanks for your review, all will change in next version.

Thanks,
Jie Hai
On 2024/9/14 16:20, fengchengwen wrote:
> On 2024/9/14 15:13, Jie Hai wrote:
>> This patch adds a telemetry command for registers dump,
>> and supports obtaining the registers of a specified module.
>>
>> In one way, the number of registers that can be exported
>> is limited by the number of elements carried by dict and
>> container. In another way, the length of the string
>> exported by telemetry is limited by MAX_OUTPUT_LEN.
>> Therefore, when the number of registers to be exported
>> exceeds, some information will be lost. Warn on the former
>> case.
>>
>> An example usage is shown below:
>> --> /ethdev/regs,0,ring
>> {
>>    "/ethdev/regs": {
>>      "registers_length": 318,
>>      "registers_width": 4,
>>      "register_offset": "0x0",
>>      "version": "0x1140011",
>>      "group_0": {
>>        "Q0_ring_rx_bd_num": "0x0",
>>        "Q0_ring_rx_bd_len": "0x0",
>>        ...
>>        },
>>      "group_1": {
>>          ...
>>      },
>>      ...
>> }
>>
>> Signed-off-by: Jie Hai <haijie1@huawei.com>
>> ---
>>   lib/ethdev/rte_ethdev_telemetry.c | 128 ++++++++++++++++++++++++++++++
>>   1 file changed, 128 insertions(+)
>>
>> diff --git a/lib/ethdev/rte_ethdev_telemetry.c b/lib/ethdev/rte_ethdev_telemetry.c
>> index 6b873e7abe68..1d59c693883e 100644
>> --- a/lib/ethdev/rte_ethdev_telemetry.c
>> +++ b/lib/ethdev/rte_ethdev_telemetry.c
>> @@ -1395,6 +1395,132 @@ eth_dev_handle_port_tm_node_caps(const char *cmd __rte_unused,
>>   	return ret;
>>   }
>>   
>> +static void
>> +eth_dev_add_reg_data(struct rte_tel_data *d, struct rte_dev_reg_info *reg_info,
>> +		     uint32_t idx)
>> +{
>> +	if (reg_info->width == sizeof(uint32_t))
>> +		rte_tel_data_add_dict_uint_hex(d, reg_info->names[idx].name,
>> +			*((uint32_t *)reg_info->data + idx), 0);
>> +	else
>> +		rte_tel_data_add_dict_uint_hex(d, reg_info->names[idx].name,
>> +			*((uint64_t *)reg_info->data + idx), 0);
>> +}
>> +
>> +static int
>> +eth_dev_store_regs(struct rte_tel_data *d, struct rte_dev_reg_info *reg_info)
>> +{
>> +	struct rte_tel_data *groups[RTE_TEL_MAX_DICT_ENTRIES];
>> +	char group_name[RTE_TEL_MAX_STRING_LEN] = {0};
>> +	struct rte_tel_data *group = NULL;
>> +	uint32_t grp_num = 0;
>> +	uint32_t i;
>> +	int ret;
>> +
>> +	rte_tel_data_start_dict(d);
>> +	rte_tel_data_add_dict_uint(d, "register_length", reg_info->length);
>> +	rte_tel_data_add_dict_uint(d, "register_width", reg_info->width);
>> +	rte_tel_data_add_dict_uint_hex(d, "register_offset", reg_info->offset, 0);
>> +	rte_tel_data_add_dict_uint_hex(d, "version", reg_info->version, 0);
>> +
>> +	for (i = 0; i < reg_info->length; i++) {
>> +		if (i % RTE_TEL_MAX_DICT_ENTRIES != 0) {
>> +			eth_dev_add_reg_data(group, reg_info, i);
>> +			continue;
>> +		}
>> +
>> +		group = rte_tel_data_alloc();
>> +		if (group == NULL) {
>> +			ret = -ENOMEM;
>> +			RTE_ETHDEV_LOG_LINE(WARNING, "No enough memory for group data");
>> +			goto out;
>> +		}
>> +		groups[grp_num++] = group;
> 
> Although the probability is very small, the groups array may overflow if the length is very large.
> 
> Suggest add judgement at the begin of this function:
> 
> uint32_t max_cap = (RTE_TEL_MAX_DICT_ENTRIES - 4) * RTE_TEL_MAX_DICT_ENTRIES;   // it will be 65000+ register
> if (reg_info->length > max_cap) {
>      // just output error and exit
> }
> 
>> +		rte_tel_data_start_dict(group);
>> +		eth_dev_add_reg_data(group, reg_info, i);
>> +	}
>> +
>> +	for (i = 0; i < grp_num; i++) {
>> +		snprintf(group_name, RTE_TEL_MAX_STRING_LEN, "group_%u", i);
>> +		ret = rte_tel_data_add_dict_container(d, group_name, groups[i], 0);
>> +		if (ret == -ENOSPC) {
>> +			RTE_ETHDEV_LOG_LINE(WARNING,
>> +				"Reduce register number to be displayed from %u to %u due to limited capacity of telemetry",
>> +				reg_info->length, i * RTE_TEL_MAX_DICT_ENTRIES);
>> +			break;
>> +		}
> 
> It may lead to memory leak when break. If we add such judgement before, there is no need to add such judgement here.
> 
>> +	}
>> +	return 0;
>> +out:
>> +	for (i = 0; i < grp_num; i++)
>> +		rte_tel_data_free(groups[i]);
>> +
>> +	return ret;
>> +}
>> +
>> +static int
>> +eth_dev_get_port_regs(int port_id, struct rte_tel_data *d, char *filter)
>> +{
>> +	struct rte_dev_reg_info reg_info;
>> +	int ret;
>> +
>> +	memset(&reg_info, 0, sizeof(reg_info));
>> +	reg_info.filter = filter;
>> +
>> +	ret = rte_eth_dev_get_reg_info_ext(port_id, &reg_info);
>> +	if (ret != 0) {
>> +		RTE_ETHDEV_LOG_LINE(ERR, "Error getting device reg info: %d", ret);
> 
> how about "Failed to get device reg info, ret = %d"
> 
>> +		return ret;
>> +	}
>> +
>> +	reg_info.data = calloc(reg_info.length, reg_info.width);
>> +	if (reg_info.data == NULL) {
>> +		RTE_ETHDEV_LOG_LINE(ERR, "Fail to allocate memory for reg_info.data");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	reg_info.names = calloc(reg_info.length, sizeof(struct rte_eth_reg_name));
>> +	if (reg_info.names == NULL) {
>> +		RTE_ETHDEV_LOG_LINE(ERR, "Fail to allocate memory for reg_info.names");
>> +		free(reg_info.data);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	ret = rte_eth_dev_get_reg_info_ext(port_id, &reg_info);
>> +	if (ret != 0) {
>> +		RTE_ETHDEV_LOG_LINE(ERR, "Error getting regs from device: %d", ret);
> 
> It seemed the ret is device id, how about "Failed to get device reg info, ret = %d"
> 
>> +		ret = -EINVAL;
>> +		goto out;
>> +	}
>> +
>> +	ret = eth_dev_store_regs(d, &reg_info);
>> +out:
>> +	free(reg_info.data);
>> +	free(reg_info.names);
>> +
>> +	return ret;
>> +}
>> +
>> +static int
>> +eth_dev_handle_port_regs(const char *cmd __rte_unused,
>> +		const char *params,
>> +		struct rte_tel_data *d)
>> +{
>> +	char *filter, *end_param;
>> +	uint16_t port_id;
>> +	int ret;
>> +
>> +	ret = eth_dev_parse_port_params(params, &port_id, &end_param, true);
>> +	if (ret != 0)
>> +		return ret;
>> +
>> +	filter = strtok(end_param, ",");
>> +	if (filter != NULL && strlen(filter) == 0)
>> +		filter = NULL;
>> +
>> +	return eth_dev_get_port_regs(port_id, d, filter);
>> +}
>> +
>>   RTE_INIT(ethdev_init_telemetry)
>>   {
>>   	rte_telemetry_register_cmd("/ethdev/list", eth_dev_handle_port_list,
>> @@ -1436,4 +1562,6 @@ RTE_INIT(ethdev_init_telemetry)
>>   			"Returns TM Level Capabilities info for a port. Parameters: int port_id, int level_id (see tm_capability for the max)");
>>   	rte_telemetry_register_cmd("/ethdev/tm_node_capability", eth_dev_handle_port_tm_node_caps,
>>   			"Returns TM Node Capabilities info for a port. Parameters: int port_id, int node_id (see tm_capability for the max)");
>> +	rte_telemetry_register_cmd("/ethdev/regs", eth_dev_handle_port_regs,
>> +			"Returns all or filtered registers info for a port. Parameters: int port_id, string module_name (Optional if show all)");
>>   }
> 
> .