[dpdk-dev,1/6] fm10k: Fix improper RX buffer size assignment

Message ID 1432887044-24777-2-git-send-email-jing.d.chen@intel.com (mailing list archive)
State Accepted, archived
Headers

Commit Message

Chen, Jing D May 29, 2015, 8:10 a.m. UTC
  From: "Chen Jing D(Mark)" <jing.d.chen@intel.com>

As RX buffer is aligned to 512B within mbuf, some bytes are reserved
for this purpose, and the worst case could be 511B. But SRR reg
assumes all buffers have the same size. In order to fill the gap,
we'll have to consider the worsst case and assume 512B is reserved.
If we don't do so, it's possible for HW to overwrite data to next
mbuf.

Signed-off-by: Chen Jing D(Mark) <jing.d.chen@intel.com>
---
 drivers/net/fm10k/fm10k.h        |    5 +++--
 drivers/net/fm10k/fm10k_ethdev.c |   12 ++++++++++--
 2 files changed, 13 insertions(+), 4 deletions(-)
  

Comments

Michael Qiu June 16, 2015, 11:55 a.m. UTC | #1
Tested-by: Michael Qiu <michael.qiu@intel.com>

- OS: Fedora20  3.11.10-301
- GCC: gcc version 4.8.3 2014911
- CPU: Intel(R) Xeon(R) CPU E5-2680 v2 @ 2.80GHz
- NIC: Ethernet controller: Intel Corporation Device 15a4 (rev 01)
- Default x86_64-native-linuxapp-gcc configuration


On 5/29/2015 4:11 PM, Chen, Jing D wrote:
> From: "Chen Jing D(Mark)" <jing.d.chen@intel.com>
>
> As RX buffer is aligned to 512B within mbuf, some bytes are reserved
> for this purpose, and the worst case could be 511B. But SRR reg
> assumes all buffers have the same size. In order to fill the gap,
> we'll have to consider the worsst case and assume 512B is reserved.
> If we don't do so, it's possible for HW to overwrite data to next
> mbuf.
>
> Signed-off-by: Chen Jing D(Mark) <jing.d.chen@intel.com>
> ---
>  drivers/net/fm10k/fm10k.h        |    5 +++--
>  drivers/net/fm10k/fm10k_ethdev.c |   12 ++++++++++--
>  2 files changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/fm10k/fm10k.h b/drivers/net/fm10k/fm10k.h
> index 0e31796..ad7a7d1 100644
> --- a/drivers/net/fm10k/fm10k.h
> +++ b/drivers/net/fm10k/fm10k.h
> @@ -191,7 +191,8 @@ struct fm10k_tx_queue {
>  
>  /* enforce 512B alignment on default Rx DMA addresses */
>  #define MBUF_DMA_ADDR_DEFAULT(mb) \
> -	((uint64_t) RTE_ALIGN(((mb)->buf_physaddr + RTE_PKTMBUF_HEADROOM), 512))
> +	((uint64_t) RTE_ALIGN(((mb)->buf_physaddr + RTE_PKTMBUF_HEADROOM),\
> +			FM10K_RX_DATABUF_ALIGN))
>  
>  static inline void fifo_reset(struct fifo *fifo, uint32_t len)
>  {
> @@ -263,7 +264,7 @@ fm10k_addr_alignment_valid(struct rte_mbuf *mb)
>  	uint64_t boundary1, boundary2;
>  
>  	/* 512B aligned? */
> -	if (RTE_ALIGN(addr, 512) == addr)
> +	if (RTE_ALIGN(addr, FM10K_RX_DATABUF_ALIGN) == addr)
>  		return 1;
>  
>  	/* 8B aligned, and max Ethernet frame would not cross a 4KB boundary? */
> diff --git a/drivers/net/fm10k/fm10k_ethdev.c b/drivers/net/fm10k/fm10k_ethdev.c
> index 275c19c..a5e09a0 100644
> --- a/drivers/net/fm10k/fm10k_ethdev.c
> +++ b/drivers/net/fm10k/fm10k_ethdev.c
> @@ -41,7 +41,6 @@
>  #include "fm10k.h"
>  #include "base/fm10k_api.h"
>  
> -#define FM10K_RX_BUFF_ALIGN 512
>  /* Default delay to acquire mailbox lock */
>  #define FM10K_MBXLOCK_DELAY_US 20
>  #define UINT64_LOWER_32BITS_MASK 0x00000000ffffffffULL
> @@ -426,6 +425,15 @@ fm10k_dev_rx_init(struct rte_eth_dev *dev)
>  		/* Configure the Rx buffer size for one buff without split */
>  		buf_size = (uint16_t)(rte_pktmbuf_data_room_size(rxq->mp) -
>  			RTE_PKTMBUF_HEADROOM);
> +		/* As RX buffer is aligned to 512B within mbuf, some bytes are
> +		 * reserved for this purpose, and the worst case could be 511B.
> +		 * But SRR reg assumes all buffers have the same size. In order
> +		 * to fill the gap, we'll have to consider the worst case and
> +		 * assume 512B is reserved. If we don't do so, it's possible
> +		 * for HW to overwrite data to next mbuf.
> +		 */
> +		buf_size -= FM10K_RX_DATABUF_ALIGN;
> +
>  		FM10K_WRITE_REG(hw, FM10K_SRRCTL(i),
>  				buf_size >> FM10K_SRRCTL_BSIZEPKT_SHIFT);
>  
> @@ -909,7 +917,7 @@ mempool_element_size_valid(struct rte_mempool *mp)
>  			RTE_PKTMBUF_HEADROOM;
>  
>  	/* account for up to 512B of alignment */
> -	min_size -= FM10K_RX_BUFF_ALIGN;
> +	min_size -= FM10K_RX_DATABUF_ALIGN;
>  
>  	/* sanity check for overflow */
>  	if (min_size > mp->elt_size)
  

Patch

diff --git a/drivers/net/fm10k/fm10k.h b/drivers/net/fm10k/fm10k.h
index 0e31796..ad7a7d1 100644
--- a/drivers/net/fm10k/fm10k.h
+++ b/drivers/net/fm10k/fm10k.h
@@ -191,7 +191,8 @@  struct fm10k_tx_queue {
 
 /* enforce 512B alignment on default Rx DMA addresses */
 #define MBUF_DMA_ADDR_DEFAULT(mb) \
-	((uint64_t) RTE_ALIGN(((mb)->buf_physaddr + RTE_PKTMBUF_HEADROOM), 512))
+	((uint64_t) RTE_ALIGN(((mb)->buf_physaddr + RTE_PKTMBUF_HEADROOM),\
+			FM10K_RX_DATABUF_ALIGN))
 
 static inline void fifo_reset(struct fifo *fifo, uint32_t len)
 {
@@ -263,7 +264,7 @@  fm10k_addr_alignment_valid(struct rte_mbuf *mb)
 	uint64_t boundary1, boundary2;
 
 	/* 512B aligned? */
-	if (RTE_ALIGN(addr, 512) == addr)
+	if (RTE_ALIGN(addr, FM10K_RX_DATABUF_ALIGN) == addr)
 		return 1;
 
 	/* 8B aligned, and max Ethernet frame would not cross a 4KB boundary? */
diff --git a/drivers/net/fm10k/fm10k_ethdev.c b/drivers/net/fm10k/fm10k_ethdev.c
index 275c19c..a5e09a0 100644
--- a/drivers/net/fm10k/fm10k_ethdev.c
+++ b/drivers/net/fm10k/fm10k_ethdev.c
@@ -41,7 +41,6 @@ 
 #include "fm10k.h"
 #include "base/fm10k_api.h"
 
-#define FM10K_RX_BUFF_ALIGN 512
 /* Default delay to acquire mailbox lock */
 #define FM10K_MBXLOCK_DELAY_US 20
 #define UINT64_LOWER_32BITS_MASK 0x00000000ffffffffULL
@@ -426,6 +425,15 @@  fm10k_dev_rx_init(struct rte_eth_dev *dev)
 		/* Configure the Rx buffer size for one buff without split */
 		buf_size = (uint16_t)(rte_pktmbuf_data_room_size(rxq->mp) -
 			RTE_PKTMBUF_HEADROOM);
+		/* As RX buffer is aligned to 512B within mbuf, some bytes are
+		 * reserved for this purpose, and the worst case could be 511B.
+		 * But SRR reg assumes all buffers have the same size. In order
+		 * to fill the gap, we'll have to consider the worst case and
+		 * assume 512B is reserved. If we don't do so, it's possible
+		 * for HW to overwrite data to next mbuf.
+		 */
+		buf_size -= FM10K_RX_DATABUF_ALIGN;
+
 		FM10K_WRITE_REG(hw, FM10K_SRRCTL(i),
 				buf_size >> FM10K_SRRCTL_BSIZEPKT_SHIFT);
 
@@ -909,7 +917,7 @@  mempool_element_size_valid(struct rte_mempool *mp)
 			RTE_PKTMBUF_HEADROOM;
 
 	/* account for up to 512B of alignment */
-	min_size -= FM10K_RX_BUFF_ALIGN;
+	min_size -= FM10K_RX_DATABUF_ALIGN;
 
 	/* sanity check for overflow */
 	if (min_size > mp->elt_size)