[dpdk-dev] [PATCH] examples/ethtool: include case for 64-bit registers

[dpdk-dev] [PATCH] examples/ethtool: include case for 64-bit registers

[zr]

From: Zyta Szpak <zyta.szpak@semihalf.com>

rte_eth_dev_get_reg_length and rte_eth_dev_get_reg callbacks
do not provide register size to the app in any way. Example assuming
they are 32-bit wide always allocates not enough memory if the
registers are 64-bit wide. It results in memory corruption.
This commit is a quick fix to make enough room for 64-bit
register values when this returned value is given to malloc.

Signed-off-by: Zyta Szpak <zr@semihalf.com>
---
 examples/ethtool/lib/rte_ethtool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/examples/ethtool/lib/rte_ethtool.c b/examples/ethtool/lib/rte_ethtool.c
index 42e05f1..bf0a6ac 100644
--- a/examples/ethtool/lib/rte_ethtool.c
+++ b/examples/ethtool/lib/rte_ethtool.c
@@ -91,7 +91,7 @@ rte_ethtool_get_regs_len(uint8_t port_id)
 
 	count_regs = rte_eth_dev_get_reg_length(port_id);
 	if (count_regs > 0)
-		return count_regs * sizeof(uint32_t);
+		return count_regs * sizeof(uint64_t);
 	return count_regs;
 }
 
-- 
1.9.1

Re: [dpdk-dev] [PATCH] examples/ethtool: include case for 64-bit registers

[Remy Horton]

Morning,

On 11/05/2016 11:48, zr@semihalf.com wrote:
> From: Zyta Szpak <zyta.szpak@semihalf.com>
>
> rte_eth_dev_get_reg_length and rte_eth_dev_get_reg callbacks
> do not provide register size to the app in any way. Example assuming
> they are 32-bit wide always allocates not enough memory if the
> registers are 64-bit wide. It results in memory corruption.
> This commit is a quick fix to make enough room for 64-bit
> register values when this returned value is given to malloc.
[..]

This is a loose end that needs to be fixed but my feeling is that it 
ought to be done via querying the driver rather than overstating 
register bank size. My suggestion would be to add something like 
get_reg_wordsize to struct eth_dev_ops and then to use sizeof(uint32) as 
fallback for drivers that don't implement the callback.

Regards,

..Rémy

Re: [dpdk-dev] [PATCH] examples/ethtool: include case for 64-bit registers

[Zyta Szpak]

Hi,
sorry on my late reply I was on sick leave. Sure I can do that. This fix 
was the fastest possible without interfering with DPDK API. I will add 
the callback then.

Regards,
Zyta Szpak

On 20.05.2016 10:25, Remy Horton wrote:
> Morning,
>
> On 11/05/2016 11:48, zr@semihalf.com wrote:
>> From: Zyta Szpak <zyta.szpak@semihalf.com>
>>
>> rte_eth_dev_get_reg_length and rte_eth_dev_get_reg callbacks
>> do not provide register size to the app in any way. Example assuming
>> they are 32-bit wide always allocates not enough memory if the
>> registers are 64-bit wide. It results in memory corruption.
>> This commit is a quick fix to make enough room for 64-bit
>> register values when this returned value is given to malloc.
> [..]
>
> This is a loose end that needs to be fixed but my feeling is that it 
> ought to be done via querying the driver rather than overstating 
> register bank size. My suggestion would be to add something like 
> get_reg_wordsize to struct eth_dev_ops and then to use sizeof(uint32) 
> as fallback for drivers that don't implement the callback.
>
> Regards,
>
> ..Rémy

[dpdk-dev] [PATCH 1/2] ethdev: add callback to get register size in bytes

[zr]

From: Zyta Szpak <zr@semihalf.com>

Version 2 of fixing the fixed register width assumption.
rte_eth_dev_get_reg_length and rte_eth_dev_get_reg callbacks
do not provide register size to the app in any way. It is
needed to allocate proper number of bytes before retrieving
registers content with rte_eth_dev_get_reg.

Signed-off-by: Zyta Szpak <zr@semihalf.com>
---
 lib/librte_ether/rte_ethdev.c | 12 ++++++++++++
 lib/librte_ether/rte_ethdev.h | 18 ++++++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index a31018e..e0765f8 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -3231,6 +3231,18 @@ rte_eth_dev_get_reg_length(uint8_t port_id)
 }
 
 int
+rte_eth_dev_get_reg_width(uint8_t port_id)
+{
+	struct rte_eth_dev *dev;
+
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+
+	dev = &rte_eth_devices[port_id];
+	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->get_reg_width, -ENOTSUP);
+	return (*dev->dev_ops->get_reg_width)(dev);
+}
+
+int
 rte_eth_dev_get_reg_info(uint8_t port_id, struct rte_dev_reg_info *info)
 {
 	struct rte_eth_dev *dev;
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 2757510..552eaed 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -1292,6 +1292,9 @@ typedef int (*eth_timesync_write_time)(struct rte_eth_dev *dev,
 typedef int (*eth_get_reg_length_t)(struct rte_eth_dev *dev);
 /**< @internal Retrieve device register count  */
 
+typedef int (*eth_get_reg_width_t)(struct rte_eth_dev *dev);
+/**< @internal Retrieve device register byte number */
+
 typedef int (*eth_get_reg_t)(struct rte_eth_dev *dev,
 				struct rte_dev_reg_info *info);
 /**< @internal Retrieve registers  */
@@ -1455,6 +1458,8 @@ struct eth_dev_ops {
 
 	eth_get_reg_length_t get_reg_length;
 	/**< Get # of registers */
+	eth_get_reg_width_t get_reg_width;
+	/**< Get # of bytes in register */
 	eth_get_reg_t get_reg;
 	/**< Get registers */
 	eth_get_eeprom_length_t get_eeprom_length;
@@ -3971,6 +3976,19 @@ int rte_eth_tx_queue_info_get(uint8_t port_id, uint16_t queue_id,
  */
 int rte_eth_dev_get_reg_length(uint8_t port_id);
 
+/*
+ * Retrieve the number of bytes in register for a specific device
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @return
+ *   - (>=0) number of registers if successful.
+ *   - (-ENOTSUP) if hardware doesn't support.
+ *   - (-ENODEV) if *port_id* invalid.
+ *   - others depends on the specific operations implementation.
+ */
+int rte_eth_dev_get_reg_width(uint8_t port_id);
+
 /**
  * Retrieve device registers and register attributes
  *
-- 
1.9.1

[dpdk-dev] [PATCH 2/2] examples/ethtool: get reg width to allocate memory

[zr]

From: Zyta Szpak <zr@semihalf.com>

Version 2 of fixing the fixed register width assumption.
Not every device uses 32-bit wide register. The app was allocating too
little space for 64-bit registers which resulted in memory corruption.
This commit resolves this by getting the size of register in bytes for
a specific device. If the device does not implement this function, it
fallsback to sizeof(uint32_t)

Signed-off-by: Zyta Szpak <zr@semihalf.com>
---
 examples/ethtool/lib/rte_ethtool.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/examples/ethtool/lib/rte_ethtool.c b/examples/ethtool/lib/rte_ethtool.c
index 42e05f1..59191ca 100644
--- a/examples/ethtool/lib/rte_ethtool.c
+++ b/examples/ethtool/lib/rte_ethtool.c
@@ -88,10 +88,14 @@ int
 rte_ethtool_get_regs_len(uint8_t port_id)
 {
 	int count_regs;
+	int reg_width;
 
 	count_regs = rte_eth_dev_get_reg_length(port_id);
+	reg_width = rte_eth_dev_get_reg_width(port_id);
+	if (reg_width < 0)
+		reg_width = sizeof(uint32_t);
 	if (count_regs > 0)
-		return count_regs * sizeof(uint32_t);
+		return count_regs * reg_width;
 	return count_regs;
 }
 
-- 
1.9.1

Re: [dpdk-dev] [PATCH 1/2] ethdev: add callback to get register size in bytes

[Remy Horton]

'noon,

Was expecting rte_eth_dev_get_reg_width() itself to default to 
sizeof(uint32_t) rather than -ENOTSUP, but that is purely personal taste 
which others might disagree with. You'll also need a documentation 
update & Fixes: line.


On 25/05/2016 07:36, zr@semihalf.com wrote:
> From: Zyta Szpak <zr@semihalf.com>
[..]
> Signed-off-by: Zyta Szpak <zr@semihalf.com>

Acked-by: Remy Horton <remy.horton@intel.com>

Re: [dpdk-dev] [PATCH 2/2] examples/ethtool: get reg width to allocate memory

[Remy Horton]

On 25/05/2016 07:36, zr@semihalf.com wrote:
> From: Zyta Szpak <zr@semihalf.com>
[..]
> Signed-off-by: Zyta Szpak <zr@semihalf.com>
> ---
>   examples/ethtool/lib/rte_ethtool.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)

Acked-by: Remy Horton <remy.horton@intel.com>

Re: [dpdk-dev] [PATCH 1/2] ethdev: add callback to get register size in bytes

[Panu Matilainen]

On 05/25/2016 09:36 AM, zr@semihalf.com wrote:
> From: Zyta Szpak <zr@semihalf.com>
>
> Version 2 of fixing the fixed register width assumption.
> rte_eth_dev_get_reg_length and rte_eth_dev_get_reg callbacks
> do not provide register size to the app in any way. It is
> needed to allocate proper number of bytes before retrieving
> registers content with rte_eth_dev_get_reg.
>
> Signed-off-by: Zyta Szpak <zr@semihalf.com>
> ---
>  lib/librte_ether/rte_ethdev.c | 12 ++++++++++++
>  lib/librte_ether/rte_ethdev.h | 18 ++++++++++++++++++
>  2 files changed, 30 insertions(+)
>
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index a31018e..e0765f8 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -3231,6 +3231,18 @@ rte_eth_dev_get_reg_length(uint8_t port_id)
>  }
>
>  int
> +rte_eth_dev_get_reg_width(uint8_t port_id)
> +{
> +	struct rte_eth_dev *dev;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +
> +	dev = &rte_eth_devices[port_id];
> +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->get_reg_width, -ENOTSUP);
> +	return (*dev->dev_ops->get_reg_width)(dev);
> +}
> +
> +int
>  rte_eth_dev_get_reg_info(uint8_t port_id, struct rte_dev_reg_info *info)
>  {
>  	struct rte_eth_dev *dev;
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index 2757510..552eaed 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -1292,6 +1292,9 @@ typedef int (*eth_timesync_write_time)(struct rte_eth_dev *dev,
>  typedef int (*eth_get_reg_length_t)(struct rte_eth_dev *dev);
>  /**< @internal Retrieve device register count  */
>
> +typedef int (*eth_get_reg_width_t)(struct rte_eth_dev *dev);
> +/**< @internal Retrieve device register byte number */
> +
>  typedef int (*eth_get_reg_t)(struct rte_eth_dev *dev,
>  				struct rte_dev_reg_info *info);
>  /**< @internal Retrieve registers  */
> @@ -1455,6 +1458,8 @@ struct eth_dev_ops {
>
>  	eth_get_reg_length_t get_reg_length;
>  	/**< Get # of registers */
> +	eth_get_reg_width_t get_reg_width;
> +	/**< Get # of bytes in register */
>  	eth_get_reg_t get_reg;
>  	/**< Get registers */
>  	eth_get_eeprom_length_t get_eeprom_length;

This is an ABI break, but maybe it is part of that "driver 
implementation API" which is exempt from the ABI/API policies. Thomas?

> @@ -3971,6 +3976,19 @@ int rte_eth_tx_queue_info_get(uint8_t port_id, uint16_t queue_id,
>   */
>  int rte_eth_dev_get_reg_length(uint8_t port_id);
>
> +/*
> + * Retrieve the number of bytes in register for a specific device
> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @return
> + *   - (>=0) number of registers if successful.
> + *   - (-ENOTSUP) if hardware doesn't support.
> + *   - (-ENODEV) if *port_id* invalid.
> + *   - others depends on the specific operations implementation.
> + */
> +int rte_eth_dev_get_reg_width(uint8_t port_id);
> +
>  /**
>   * Retrieve device registers and register attributes
>   *

The function needs to be exported via rte_ether_version.map as well.

	- Panu -
>

Re: [dpdk-dev] [PATCH 1/2] ethdev: add callback to get register size in bytes

[Thomas Monjalon]

2016-05-27 13:28, Panu Matilainen:
> On 05/25/2016 09:36 AM, zr@semihalf.com wrote:
> > @@ -1455,6 +1458,8 @@ struct eth_dev_ops {
> >
> >  	eth_get_reg_length_t get_reg_length;
> >  	/**< Get # of registers */
> > +	eth_get_reg_width_t get_reg_width;
> > +	/**< Get # of bytes in register */
> >  	eth_get_reg_t get_reg;
> >  	/**< Get registers */
> >  	eth_get_eeprom_length_t get_eeprom_length;
> 
> This is an ABI break, but maybe it is part of that "driver 
> implementation API" which is exempt from the ABI/API policies. Thomas?

Yes dev_ops are for drivers, not for applications.
Thus it should not be impacted by the ABI policy.

Re: [dpdk-dev] [PATCH 1/2] ethdev: add callback to get register size in bytes

[Zyta Szpak]

Hi,
It is the standard DPDK return value -ENOTSUP when the function is not 
supported by Ethernet device. I think it is safer to keep it this way 
rather than default implicitly to sizeof(uint32_t) and more generic.

Regards,
Zyta

On 25.05.2016 15:14, Remy Horton wrote:
> 'noon,
>
> Was expecting rte_eth_dev_get_reg_width() itself to default to 
> sizeof(uint32_t) rather than -ENOTSUP, but that is purely personal 
> taste which others might disagree with. You'll also need a 
> documentation update & Fixes: line.
>
>
> On 25/05/2016 07:36, zr@semihalf.com wrote:
>> From: Zyta Szpak <zr@semihalf.com>
> [..]
>> Signed-off-by: Zyta Szpak <zr@semihalf.com>
>
> Acked-by: Remy Horton <remy.horton@intel.com>

Re: [dpdk-dev] [PATCH 1/2] ethdev: add callback to get register size in bytes

[Zyta Szpak]

On 27.05.2016 12:28, Panu Matilainen wrote:
> On 05/25/2016 09:36 AM, zr@semihalf.com wrote:
>> From: Zyta Szpak <zr@semihalf.com>
>>
>> Version 2 of fixing the fixed register width assumption.
>> rte_eth_dev_get_reg_length and rte_eth_dev_get_reg callbacks
>> do not provide register size to the app in any way. It is
>> needed to allocate proper number of bytes before retrieving
>> registers content with rte_eth_dev_get_reg.
>>
>> Signed-off-by: Zyta Szpak <zr@semihalf.com>
>> ---
>>  lib/librte_ether/rte_ethdev.c | 12 ++++++++++++
>>  lib/librte_ether/rte_ethdev.h | 18 ++++++++++++++++++
>>  2 files changed, 30 insertions(+)
>>
>> diff --git a/lib/librte_ether/rte_ethdev.c 
>> b/lib/librte_ether/rte_ethdev.c
>> index a31018e..e0765f8 100644
>> --- a/lib/librte_ether/rte_ethdev.c
>> +++ b/lib/librte_ether/rte_ethdev.c
>> @@ -3231,6 +3231,18 @@ rte_eth_dev_get_reg_length(uint8_t port_id)
>>  }
>>
>>  int
>> +rte_eth_dev_get_reg_width(uint8_t port_id)
>> +{
>> +    struct rte_eth_dev *dev;
>> +
>> +    RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +    dev = &rte_eth_devices[port_id];
>> +    RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->get_reg_width, -ENOTSUP);
>> +    return (*dev->dev_ops->get_reg_width)(dev);
>> +}
>> +
>> +int
>>  rte_eth_dev_get_reg_info(uint8_t port_id, struct rte_dev_reg_info 
>> *info)
>>  {
>>      struct rte_eth_dev *dev;
>> diff --git a/lib/librte_ether/rte_ethdev.h 
>> b/lib/librte_ether/rte_ethdev.h
>> index 2757510..552eaed 100644
>> --- a/lib/librte_ether/rte_ethdev.h
>> +++ b/lib/librte_ether/rte_ethdev.h
>> @@ -1292,6 +1292,9 @@ typedef int (*eth_timesync_write_time)(struct 
>> rte_eth_dev *dev,
>>  typedef int (*eth_get_reg_length_t)(struct rte_eth_dev *dev);
>>  /**< @internal Retrieve device register count  */
>>
>> +typedef int (*eth_get_reg_width_t)(struct rte_eth_dev *dev);
>> +/**< @internal Retrieve device register byte number */
>> +
>>  typedef int (*eth_get_reg_t)(struct rte_eth_dev *dev,
>>                  struct rte_dev_reg_info *info);
>>  /**< @internal Retrieve registers  */
>> @@ -1455,6 +1458,8 @@ struct eth_dev_ops {
>>
>>      eth_get_reg_length_t get_reg_length;
>>      /**< Get # of registers */
>> +    eth_get_reg_width_t get_reg_width;
>> +    /**< Get # of bytes in register */
>>      eth_get_reg_t get_reg;
>>      /**< Get registers */
>>      eth_get_eeprom_length_t get_eeprom_length;
>
> This is an ABI break, but maybe it is part of that "driver 
> implementation API" which is exempt from the ABI/API policies. Thomas?
>
>> @@ -3971,6 +3976,19 @@ int rte_eth_tx_queue_info_get(uint8_t port_id, 
>> uint16_t queue_id,
>>   */
>>  int rte_eth_dev_get_reg_length(uint8_t port_id);
>>
>> +/*
>> + * Retrieve the number of bytes in register for a specific device
>> + *
>> + * @param port_id
>> + *   The port identifier of the Ethernet device.
>> + * @return
>> + *   - (>=0) number of registers if successful.
>> + *   - (-ENOTSUP) if hardware doesn't support.
>> + *   - (-ENODEV) if *port_id* invalid.
>> + *   - others depends on the specific operations implementation.
>> + */
>> +int rte_eth_dev_get_reg_width(uint8_t port_id);
>> +
>>  /**
>>   * Retrieve device registers and register attributes
>>   *
>
> The function needs to be exported via rte_ether_version.map as well.
OK, right!
>
>     - Panu -
>>
>