[v2,1/7] ethdev: support report register names and filter

  This patch adds "filter" and "names" fields to "rte_dev_reg_info"
structure. Names of registers in data fields can be reported and
the registers can be filtered by their names.

For compatibility, the original API rte_eth_dev_get_reg_info()
does not use the name and filter fields. The new API
rte_eth_dev_get_reg_info_ext() is added to support reporting
names and filtering by names. If the drivers does not report
the names, set them to "offset_XXX".

Signed-off-by: Jie Hai <haijie1@huawei.com>
---
 doc/guides/rel_notes/release_24_03.rst |  8 ++++++
 lib/ethdev/rte_dev_info.h              | 11 ++++++++
 lib/ethdev/rte_ethdev.c                | 36 ++++++++++++++++++++++++++
 lib/ethdev/rte_ethdev.h                | 22 ++++++++++++++++
 4 files changed, 77 insertions(+)
  

On 2/5/2024 10:51 AM, Jie Hai wrote:
> This patch adds "filter" and "names" fields to "rte_dev_reg_info"
> structure. Names of registers in data fields can be reported and
> the registers can be filtered by their names.
> 
> For compatibility, the original API rte_eth_dev_get_reg_info()
> does not use the name and filter fields. The new API
> rte_eth_dev_get_reg_info_ext() is added to support reporting
> names and filtering by names. If the drivers does not report
> the names, set them to "offset_XXX".
> 
> Signed-off-by: Jie Hai <haijie1@huawei.com>
> ---
>  doc/guides/rel_notes/release_24_03.rst |  8 ++++++
>  lib/ethdev/rte_dev_info.h              | 11 ++++++++
>  lib/ethdev/rte_ethdev.c                | 36 ++++++++++++++++++++++++++
>  lib/ethdev/rte_ethdev.h                | 22 ++++++++++++++++
>  4 files changed, 77 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/release_24_03.rst b/doc/guides/rel_notes/release_24_03.rst
> index 84d3144215c6..5d402341223a 100644
> --- a/doc/guides/rel_notes/release_24_03.rst
> +++ b/doc/guides/rel_notes/release_24_03.rst
> @@ -75,6 +75,11 @@ New Features
>    * Added support for Atomic Rules' TK242 packet-capture family of devices
>      with PCI IDs: ``0x1024, 0x1025, 0x1026``.
>  
> +* **Added support for dumping regiters with names and filter.**
>

s/regiters/registers/

> +
> +  * Added new API functions ``rte_eth_dev_get_reg_info_ext()`` to and filter
> +  * the registers by their names and get the information of registers(names,
> +  * values and other attributes).
>  

'*' makes a bullet, but above seems one sentences, if so please only
keep the first '*'.

>  Removed Items
>  -------------
> @@ -124,6 +129,9 @@ ABI Changes
>  
>  * No ABI change that would break compatibility with 23.11.
>  
> +* ethdev: Added ``filter`` and ``names`` fields to ``rte_dev_reg_info``
> +  structure for reporting names of regiters and filtering them by names.
> +
>  

This will break the ABI.

Think about a case, an application compiled with an old version of DPDK,
later same application started to use this version without re-compile,
application will send old version of 'struct rte_dev_reg_info', but new
version of DPDK will try to access or update new fields of the 'struct
rte_dev_reg_info'

One option is:
- to add a new 'struct rte_dev_reg_info_ext',
- 'rte_eth_dev_get_reg_info()' still uses old 'struct rte_dev_reg_info'
- 'get_reg()' dev_ops will use this new 'struct rte_dev_reg_info_ext'
- Add deprecation notice to update 'rte_eth_dev_get_reg_info()' to use
new struct in next LTS release


>  Known Issues
>  ------------
> diff --git a/lib/ethdev/rte_dev_info.h b/lib/ethdev/rte_dev_info.h
> index 67cf0ae52668..2f4541bd46c8 100644
> --- a/lib/ethdev/rte_dev_info.h
> +++ b/lib/ethdev/rte_dev_info.h
> @@ -11,6 +11,11 @@ extern "C" {
>  
>  #include <stdint.h>
>  
> +#define RTE_ETH_REG_NAME_SIZE 128
> +struct rte_eth_reg_name {
> +	char name[RTE_ETH_REG_NAME_SIZE];
> +};
> +
>  /*
>   * Placeholder for accessing device registers
>   */
> @@ -20,6 +25,12 @@ struct rte_dev_reg_info {
>  	uint32_t length; /**< Number of registers to fetch */
>  	uint32_t width; /**< Size of device register */
>  	uint32_t version; /**< Device version */
> +	/**
> +	 * Filter for target subset of registers.
> +	 * This field could affects register selection for data/length/names.
> +	 */
> +	char *filter;
> +	struct rte_eth_reg_name *names; /**< Registers name saver */
>  };
>  
>  /*
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index f1c658f49e80..3e0294e49092 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -6388,8 +6388,39 @@ rte_eth_read_clock(uint16_t port_id, uint64_t *clock)
>  
>  int
>  rte_eth_dev_get_reg_info(uint16_t port_id, struct rte_dev_reg_info *info)
> +{
> +	struct rte_dev_reg_info reg_info;
> +	int ret;
> +
> +	if (info == NULL) {
> +		RTE_ETHDEV_LOG_LINE(ERR,
> +			"Cannot get ethdev port %u register info to NULL",
> +			port_id);
> +		return -EINVAL;
> +	}
> +
> +	reg_info.length = info->length;
> +	reg_info.data = info->data;
> +	reg_info.names = NULL;
> +	reg_info.filter = NULL;
> +
> +	ret = rte_eth_dev_get_reg_info_ext(port_id, &reg_info);
> +	if (ret != 0)
> +		return ret;
> +
> +	info->length = reg_info.length;
> +	info->width = reg_info.width;
> +	info->version = reg_info.version;
> +	info->offset = reg_info.offset;
> +
> +	return 0;
> +}
> +
> +int
> +rte_eth_dev_get_reg_info_ext(uint16_t port_id, struct rte_dev_reg_info *info)
>  {
>  	struct rte_eth_dev *dev;
> +	uint32_t i;
>  	int ret;
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> @@ -6408,6 +6439,11 @@ rte_eth_dev_get_reg_info(uint16_t port_id, struct rte_dev_reg_info *info)
>  
>  	rte_ethdev_trace_get_reg_info(port_id, info, ret);
>  
> +	/* Report the default names if drivers not report. */
> +	if (info->names != NULL && strlen(info->names[0].name) == 0)
> +		for (i = 0; i < info->length; i++)
> +			sprintf(info->names[i].name, "offset_%x",
> +				info->offset + i * info->width);
>

Better to use 'snprintf()'

>  	return ret;
>  }
>  
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index 2687c23fa6fb..3abc2ad3f865 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -5053,6 +5053,28 @@ __rte_experimental
>  int rte_eth_get_monitor_addr(uint16_t port_id, uint16_t queue_id,
>  		struct rte_power_monitor_cond *pmc);
>  
> +/**
> + * Retrieve the filtered device registers (values and names) and
> + * register attributes (number of registers and register size)
> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @param info
> + *   Pointer to rte_dev_reg_info structure to fill in. If info->data is
> + *   NULL, the function fills in the width and length fields. If non-NULL,
> + *   the values of registers whose name contains the filter string are put
> + *   into the buffer pointed at by the data field. Do the same for the names
> + *   of registers if info->names is not NULL.
>

May be good to mention if info->names is not NULL, but driver doesn't
support names, ehtdev fills the names automatically.

As both 'rte_eth_dev_get_reg_info()' & 'rte_eth_dev_get_reg_info_ext()'
use same dev_ops ('get_reg()'), it is possible that driver doesn't
support filtering, so if the user provides info->filter but driver
doesn't support it, I think API should return error, what do you think?

And can you please make it clear above, if filtering is provided with
info->data = NULL, are the returned width and length fields for filtered
number of registers or all registers?


> + * @return
> + *   - (0) if successful.
> + *   - (-ENOTSUP) if hardware doesn't support.
> + *   - (-EINVAL) if bad parameter.
> + *   - (-ENODEV) if *port_id* invalid.
> + *   - (-EIO) if device is removed.
> + *   - others depends on the specific operations implementation.
> + */
> +int rte_eth_dev_get_reg_info_ext(uint16_t port_id, struct rte_dev_reg_info *info);
> +
>

Can you please make the new API as experimental. That is the requirement
for new APIs.

Also need to add the API to version.map


>  /**
>   * Retrieve device registers and register attributes (number of registers and
>   * register size)
  

Hi, Ferruh,
Thanks for your review.

On 2024/2/8 1:00, Ferruh Yigit wrote:
> On 2/5/2024 10:51 AM, Jie Hai wrote:
>> This patch adds "filter" and "names" fields to "rte_dev_reg_info"
>> structure. Names of registers in data fields can be reported and
>> the registers can be filtered by their names.
>>
>> For compatibility, the original API rte_eth_dev_get_reg_info()
>> does not use the name and filter fields. The new API
>> rte_eth_dev_get_reg_info_ext() is added to support reporting
>> names and filtering by names. If the drivers does not report
>> the names, set them to "offset_XXX".
>>
>> Signed-off-by: Jie Hai <haijie1@huawei.com>
>> ---
>>   doc/guides/rel_notes/release_24_03.rst |  8 ++++++
>>   lib/ethdev/rte_dev_info.h              | 11 ++++++++
>>   lib/ethdev/rte_ethdev.c                | 36 ++++++++++++++++++++++++++
>>   lib/ethdev/rte_ethdev.h                | 22 ++++++++++++++++
>>   4 files changed, 77 insertions(+)
>>
>> diff --git a/doc/guides/rel_notes/release_24_03.rst b/doc/guides/rel_notes/release_24_03.rst
>> index 84d3144215c6..5d402341223a 100644
>> --- a/doc/guides/rel_notes/release_24_03.rst
>> +++ b/doc/guides/rel_notes/release_24_03.rst
>> @@ -75,6 +75,11 @@ New Features
>>     * Added support for Atomic Rules' TK242 packet-capture family of devices
>>       with PCI IDs: ``0x1024, 0x1025, 0x1026``.
>>   
>> +* **Added support for dumping regiters with names and filter.**
>>
> 
> s/regiters/registers/
Will correct in v3.
> 
>> +
>> +  * Added new API functions ``rte_eth_dev_get_reg_info_ext()`` to and filter
>> +  * the registers by their names and get the information of registers(names,
>> +  * values and other attributes).
>>   
> 
> '*' makes a bullet, but above seems one sentences, if so please only
> keep the first '*'.
Will correct in v3.
> 
>>   Removed Items
>>   -------------
>> @@ -124,6 +129,9 @@ ABI Changes
>>   
>>   * No ABI change that would break compatibility with 23.11.
>>   
>> +* ethdev: Added ``filter`` and ``names`` fields to ``rte_dev_reg_info``
>> +  structure for reporting names of regiters and filtering them by names.
>> +
>>   
> 
> This will break the ABI.
> 
> Think about a case, an application compiled with an old version of DPDK,
> later same application started to use this version without re-compile,
> application will send old version of 'struct rte_dev_reg_info', but new
> version of DPDK will try to access or update new fields of the 'struct
> rte_dev_reg_info'
> 
Actually, we use a local variable "struct rte_dev_reg_info reg_info" in
'rte_eth_dev_get_reg_info()' to pass to the driver, and the new fields
are set to zero. The old drivers do not visit the new fields.
We make constrains that if filter is NULL, do not filter and get info of 
all registers.
For an old version APP and new version ethdev, driver will not visit the 
new fields. so it does not break the ABI.
> One option is:
> - to add a new 'struct rte_dev_reg_info_ext',
> - 'rte_eth_dev_get_reg_info()' still uses old 'struct rte_dev_reg_info'
> - 'get_reg()' dev_ops will use this new 'struct rte_dev_reg_info_ext'
> - Add deprecation notice to update 'rte_eth_dev_get_reg_info()' to use
> new struct in next LTS release
> 

> 
>>   Known Issues
>>   ------------
>> diff --git a/lib/ethdev/rte_dev_info.h b/lib/ethdev/rte_dev_info.h
>> index 67cf0ae52668..2f4541bd46c8 100644
>> --- a/lib/ethdev/rte_dev_info.h
>> +++ b/lib/ethdev/rte_dev_info.h
>> @@ -11,6 +11,11 @@ extern "C" {
>>   
>>   #include <stdint.h>
>>   
>> +#define RTE_ETH_REG_NAME_SIZE 128
>> +struct rte_eth_reg_name {
>> +	char name[RTE_ETH_REG_NAME_SIZE];
>> +};
>> +
>>   /*
>>    * Placeholder for accessing device registers
>>    */
>> @@ -20,6 +25,12 @@ struct rte_dev_reg_info {
>>   	uint32_t length; /**< Number of registers to fetch */
>>   	uint32_t width; /**< Size of device register */
>>   	uint32_t version; /**< Device version */
>> +	/**
>> +	 * Filter for target subset of registers.
>> +	 * This field could affects register selection for data/length/names.
>> +	 */
>> +	char *filter;
>> +	struct rte_eth_reg_name *names; /**< Registers name saver */
>>   };
>>   
>>   /*
>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>> index f1c658f49e80..3e0294e49092 100644
>> --- a/lib/ethdev/rte_ethdev.c
>> +++ b/lib/ethdev/rte_ethdev.c
>> @@ -6388,8 +6388,39 @@ rte_eth_read_clock(uint16_t port_id, uint64_t *clock)
>>   
>>   int
>>   rte_eth_dev_get_reg_info(uint16_t port_id, struct rte_dev_reg_info *info)
>> +{
>> +	struct rte_dev_reg_info reg_info;
>> +	int ret;
>> +
>> +	if (info == NULL) {
>> +		RTE_ETHDEV_LOG_LINE(ERR,
>> +			"Cannot get ethdev port %u register info to NULL",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>> +	reg_info.length = info->length;
>> +	reg_info.data = info->data;
>> +	reg_info.names = NULL;
>> +	reg_info.filter = NULL;
>> +
>> +	ret = rte_eth_dev_get_reg_info_ext(port_id, &reg_info);
>> +	if (ret != 0)
>> +		return ret;
>> +
>> +	info->length = reg_info.length;
>> +	info->width = reg_info.width;
>> +	info->version = reg_info.version;
>> +	info->offset = reg_info.offset;
>> +
>> +	return 0;
>> +}
>> +
>> +int
>> +rte_eth_dev_get_reg_info_ext(uint16_t port_id, struct rte_dev_reg_info *info)
>>   {
>>   	struct rte_eth_dev *dev;
>> +	uint32_t i;
>>   	int ret;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> @@ -6408,6 +6439,11 @@ rte_eth_dev_get_reg_info(uint16_t port_id, struct rte_dev_reg_info *info)
>>   
>>   	rte_ethdev_trace_get_reg_info(port_id, info, ret);
>>   
>> +	/* Report the default names if drivers not report. */
>> +	if (info->names != NULL && strlen(info->names[0].name) == 0)
>> +		for (i = 0; i < info->length; i++)
>> +			sprintf(info->names[i].name, "offset_%x",
>> +				info->offset + i * info->width);
>>
> 
> Better to use 'snprintf()'
> 
Will correct in v3.
>>   	return ret;
>>   }
>>   
>> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
>> index 2687c23fa6fb..3abc2ad3f865 100644
>> --- a/lib/ethdev/rte_ethdev.h
>> +++ b/lib/ethdev/rte_ethdev.h
>> @@ -5053,6 +5053,28 @@ __rte_experimental
>>   int rte_eth_get_monitor_addr(uint16_t port_id, uint16_t queue_id,
>>   		struct rte_power_monitor_cond *pmc);
>>   
>> +/**
>> + * Retrieve the filtered device registers (values and names) and
>> + * register attributes (number of registers and register size)
>> + *
>> + * @param port_id
>> + *   The port identifier of the Ethernet device.
>> + * @param info
>> + *   Pointer to rte_dev_reg_info structure to fill in. If info->data is
>> + *   NULL, the function fills in the width and length fields. If non-NULL,
>> + *   the values of registers whose name contains the filter string are put
>> + *   into the buffer pointed at by the data field. Do the same for the names
>> + *   of registers if info->names is not NULL.
>>
> 
> May be good to mention if info->names is not NULL, but driver doesn't
> support names, ehtdev fills the names automatically.
> 
> As both 'rte_eth_dev_get_reg_info()' & 'rte_eth_dev_get_reg_info_ext()'
> use same dev_ops ('get_reg()'), it is possible that driver doesn't
> support filtering, so if the user provides info->filter but driver
> doesn't support it, I think API should return error, what do you think?
> 
> And can you please make it clear above, if filtering is provided with
> info->data = NULL, are the returned width and length fields for filtered
> number of registers or all registers?
> 
> 
Will correct in v3.
>> + * @return
>> + *   - (0) if successful.
>> + *   - (-ENOTSUP) if hardware doesn't support.
>> + *   - (-EINVAL) if bad parameter.
>> + *   - (-ENODEV) if *port_id* invalid.
>> + *   - (-EIO) if device is removed.
>> + *   - others depends on the specific operations implementation.
>> + */
>> +int rte_eth_dev_get_reg_info_ext(uint16_t port_id, struct rte_dev_reg_info *info);
>> +
>>
> 
> Can you please make the new API as experimental. That is the requirement
> for new APIs.
> 
> Also need to add the API to version.map
Will correct in v3.
> 
> 
>>   /**
>>    * Retrieve device registers and register attributes (number of registers and
>>    * register size)
> 
> .

Best regards,
Jie Hai