[PATCH] ethdev: fix byte order inconsistence between fdir flow and mask

[PATCH] ethdev: fix byte order inconsistence between fdir flow and mask

[Jingjing Wu]

Fixed issue of byte order in ethdev library that the structure
for setting fdir's mask and flow entry is inconsist and made
inputs of mask be in big endian.

fixes: 76c6f89e80d4 ("ixgbe: support new flow director masks")
       2d4c1a9ea2ac ("ethdev: add new flow director masks")

Reported-by: Yaacov Hazan <yaacovh@mellanox.com>
Signed-off-by: Jingjing Wu <jingjing.wu@intel.com>
---
 app/test-pmd/cmdline.c               |  6 ++---
 doc/guides/rel_notes/release_2_3.rst |  6 +++++
 drivers/net/ixgbe/ixgbe_fdir.c       | 47 ++++++++++++++++++++++--------------
 lib/librte_ether/rte_eth_ctrl.h      |  7 ++++--
 4 files changed, 43 insertions(+), 23 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 73298c9..13194c9 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -8687,13 +8687,13 @@ cmd_flow_director_mask_parsed(void *parsed_result,
 			return;
 		}
 
-		mask->vlan_tci_mask = res->vlan_mask;
+		mask->vlan_tci_mask = rte_cpu_to_be_16(res->vlan_mask);
 		IPV4_ADDR_TO_UINT(res->ipv4_src, mask->ipv4_mask.src_ip);
 		IPV4_ADDR_TO_UINT(res->ipv4_dst, mask->ipv4_mask.dst_ip);
 		IPV6_ADDR_TO_ARRAY(res->ipv6_src, mask->ipv6_mask.src_ip);
 		IPV6_ADDR_TO_ARRAY(res->ipv6_dst, mask->ipv6_mask.dst_ip);
-		mask->src_port_mask = res->port_src;
-		mask->dst_port_mask = res->port_dst;
+		mask->src_port_mask = rte_cpu_to_be_16(res->port_src);
+		mask->dst_port_mask = rte_cpu_to_be_16(res->port_dst);
 	}
 
 	cmd_reconfig_device_queue(res->port_id, 1, 1);
diff --git a/doc/guides/rel_notes/release_2_3.rst b/doc/guides/rel_notes/release_2_3.rst
index 99de186..28d0f27 100644
--- a/doc/guides/rel_notes/release_2_3.rst
+++ b/doc/guides/rel_notes/release_2_3.rst
@@ -19,6 +19,10 @@ Drivers
 Libraries
 ~~~~~~~~~
 
+* ** fix byte order inconsistence between fdir flow and mask **
+
+  Fixed issue in ethdev library that the structure for setting
+  fdir's mask and flow entry is inconsist in byte order.
 
 Examples
 ~~~~~~~~
@@ -39,6 +43,8 @@ API Changes
 ABI Changes
 -----------
 
+* The fields in  The ethdev structures ``rte_eth_fdir_masks`` were
+  changed to be in big endian.
 
 Shared Library Versions
 -----------------------
diff --git a/drivers/net/ixgbe/ixgbe_fdir.c b/drivers/net/ixgbe/ixgbe_fdir.c
index e03219b..7423b2d 100644
--- a/drivers/net/ixgbe/ixgbe_fdir.c
+++ b/drivers/net/ixgbe/ixgbe_fdir.c
@@ -309,6 +309,7 @@ fdir_set_input_mask_82599(struct rte_eth_dev *dev,
 	uint32_t fdiripv6m; /* IPv6 source and destination masks. */
 	uint16_t dst_ipv6m = 0;
 	uint16_t src_ipv6m = 0;
+	volatile uint32_t *reg;
 
 	PMD_INIT_FUNC_TRACE();
 
@@ -322,16 +323,16 @@ fdir_set_input_mask_82599(struct rte_eth_dev *dev,
 		/* use the L4 protocol mask for raw IPv4/IPv6 traffic */
 		fdirm |= IXGBE_FDIRM_L4P;
 
-	if (input_mask->vlan_tci_mask == 0x0FFF)
+	if (input_mask->vlan_tci_mask == rte_cpu_to_be_16(0x0FFF))
 		/* mask VLAN Priority */
 		fdirm |= IXGBE_FDIRM_VLANP;
-	else if (input_mask->vlan_tci_mask == 0xE000)
+	else if (input_mask->vlan_tci_mask == rte_cpu_to_be_16(0xE000))
 		/* mask VLAN ID */
 		fdirm |= IXGBE_FDIRM_VLANID;
 	else if (input_mask->vlan_tci_mask == 0)
 		/* mask VLAN ID and Priority */
 		fdirm |= IXGBE_FDIRM_VLANID | IXGBE_FDIRM_VLANP;
-	else if (input_mask->vlan_tci_mask != 0xEFFF) {
+	else if (input_mask->vlan_tci_mask != rte_cpu_to_be_16(0xEFFF)) {
 		PMD_INIT_LOG(ERR, "invalid vlan_tci_mask");
 		return -EINVAL;
 	}
@@ -340,19 +341,26 @@ fdir_set_input_mask_82599(struct rte_eth_dev *dev,
 	IXGBE_WRITE_REG(hw, IXGBE_FDIRM, fdirm);
 
 	/* store the TCP/UDP port masks, bit reversed from port layout */
-	fdirtcpm = reverse_fdir_bitmasks(input_mask->dst_port_mask,
-					 input_mask->src_port_mask);
+	fdirtcpm = reverse_fdir_bitmasks(
+			rte_be_to_cpu_16(input_mask->dst_port_mask),
+			rte_be_to_cpu_16(input_mask->src_port_mask));
 
-	/* write all the same so that UDP, TCP and SCTP use the same mask */
+	/* write all the same so that UDP, TCP and SCTP use the same mask
+	 * (little-endian)
+	*/
 	IXGBE_WRITE_REG(hw, IXGBE_FDIRTCPM, ~fdirtcpm);
 	IXGBE_WRITE_REG(hw, IXGBE_FDIRUDPM, ~fdirtcpm);
 	IXGBE_WRITE_REG(hw, IXGBE_FDIRSCTPM, ~fdirtcpm);
 	info->mask.src_port_mask = input_mask->src_port_mask;
 	info->mask.dst_port_mask = input_mask->dst_port_mask;
 
-	/* Store source and destination IPv4 masks (big-endian) */
-	IXGBE_WRITE_REG(hw, IXGBE_FDIRSIP4M, ~(input_mask->ipv4_mask.src_ip));
-	IXGBE_WRITE_REG(hw, IXGBE_FDIRDIP4M, ~(input_mask->ipv4_mask.dst_ip));
+	/* Store source and destination IPv4 masks (big-endian),
+	 * can not use IXGBE_WRITE_REG.
+	 */
+	reg = IXGBE_PCI_REG_ADDR(hw, IXGBE_FDIRSIP4M);
+	*reg = ~(input_mask->ipv4_mask.src_ip);
+	reg = IXGBE_PCI_REG_ADDR(hw, IXGBE_FDIRDIP4M);
+	*reg = ~(input_mask->ipv4_mask.dst_ip);
 	info->mask.src_ipv4_mask = input_mask->ipv4_mask.src_ip;
 	info->mask.dst_ipv4_mask = input_mask->ipv4_mask.dst_ip;
 
@@ -401,16 +409,16 @@ fdir_set_input_mask_x550(struct rte_eth_dev *dev,
 	/* some bits must be set for mac vlan or tunnel mode */
 	fdirm |= IXGBE_FDIRM_L4P | IXGBE_FDIRM_L3P;
 
-	if (input_mask->vlan_tci_mask == 0x0FFF)
+	if (input_mask->vlan_tci_mask == rte_cpu_to_be_16(0x0FFF))
 		/* mask VLAN Priority */
 		fdirm |= IXGBE_FDIRM_VLANP;
-	else if (input_mask->vlan_tci_mask == 0xE000)
+	else if (input_mask->vlan_tci_mask == rte_cpu_to_be_16(0xE000))
 		/* mask VLAN ID */
 		fdirm |= IXGBE_FDIRM_VLANID;
 	else if (input_mask->vlan_tci_mask == 0)
 		/* mask VLAN ID and Priority */
 		fdirm |= IXGBE_FDIRM_VLANID | IXGBE_FDIRM_VLANP;
-	else if (input_mask->vlan_tci_mask != 0xEFFF) {
+	else if (input_mask->vlan_tci_mask != rte_cpu_to_be_16(0xEFFF)) {
 		PMD_INIT_LOG(ERR, "invalid vlan_tci_mask");
 		return -EINVAL;
 	}
@@ -444,7 +452,7 @@ fdir_set_input_mask_x550(struct rte_eth_dev *dev,
 		info->mask.tunnel_type_mask =
 			input_mask->tunnel_type_mask;
 
-		switch (input_mask->tunnel_id_mask & 0xFFFFFFFF) {
+		switch (rte_be_to_cpu_32(input_mask->tunnel_id_mask)) {
 		case 0x0:
 			/* Mask vxlan id */
 			fdiripv6m |= IXGBE_FDIRIP6M_TNI_VNI;
@@ -904,13 +912,16 @@ fdir_write_perfect_filter_82599(struct ixgbe_hw *hw,
 	u32 addr_low, addr_high;
 	u32 tunnel_type = 0;
 	int err = 0;
+	volatile uint32_t *reg;
 
 	if (mode == RTE_FDIR_MODE_PERFECT) {
-		/* record the IPv4 address (big-endian) */
-		IXGBE_WRITE_REG(hw, IXGBE_FDIRIPSA,
-				input->formatted.src_ip[0]);
-		IXGBE_WRITE_REG(hw, IXGBE_FDIRIPDA,
-				input->formatted.dst_ip[0]);
+		/* record the IPv4 address (big-endian)
+		 * can not use IXGBE_WRITE_REG.
+		 */
+		reg = IXGBE_PCI_REG_ADDR(hw, IXGBE_FDIRIPSA);
+		*reg = input->formatted.src_ip[0];
+		reg = IXGBE_PCI_REG_ADDR(hw, IXGBE_FDIRIPDA);
+		*reg = input->formatted.dst_ip[0];
 
 		/* record source and destination port (little-endian)*/
 		fdirport = IXGBE_NTOHS(input->formatted.dst_port);
diff --git a/lib/librte_ether/rte_eth_ctrl.h b/lib/librte_ether/rte_eth_ctrl.h
index ce224ad..80f9048 100644
--- a/lib/librte_ether/rte_eth_ctrl.h
+++ b/lib/librte_ether/rte_eth_ctrl.h
@@ -501,6 +501,7 @@ struct rte_eth_tunnel_flow {
 
 /**
  * An union contains the inputs for all types of flow
+ * Items in flows need to be in big endian
  */
 union rte_eth_fdir_flow {
 	struct rte_eth_l2_flow     l2_flow;
@@ -586,6 +587,7 @@ struct rte_eth_fdir_filter {
 /**
  *  A structure used to configure FDIR masks that are used by the device
  *  to match the various fields of RX packet headers.
+ *  Items in it need to be in big endian
  */
 struct rte_eth_fdir_masks {
 	uint16_t vlan_tci_mask;
@@ -593,9 +595,10 @@ struct rte_eth_fdir_masks {
 	struct rte_eth_ipv6_flow   ipv6_mask;
 	uint16_t src_port_mask;
 	uint16_t dst_port_mask;
-	uint8_t mac_addr_byte_mask;  /** Per byte MAC address mask */
+	uint8_t mac_addr_byte_mask;  /** Bit mask for associated byte */
 	uint32_t tunnel_id_mask;  /** tunnel ID mask */
-	uint8_t tunnel_type_mask;
+	uint8_t tunnel_type_mask; /**< 1 - Match tunnel type,
+				       0 - Ignore tunnel type. */
 };
 
 /**
-- 
2.4.0

Re: [PATCH] ethdev: fix byte order inconsistence between fdir flow and mask

[Thomas Monjalon]

2016-01-27 16:37, Jingjing Wu:
> Fixed issue of byte order in ethdev library that the structure
> for setting fdir's mask and flow entry is inconsist and made
> inputs of mask be in big endian.

Please be more precise. Which one is big endian?
Wasn't it tested before?

> fixes: 76c6f89e80d4 ("ixgbe: support new flow director masks")
>        2d4c1a9ea2ac ("ethdev: add new flow director masks")

Please put Fixes: on the two lines.

> --- a/doc/guides/rel_notes/release_2_3.rst
> +++ b/doc/guides/rel_notes/release_2_3.rst
> @@ -19,6 +19,10 @@ Drivers
>  Libraries
>  ~~~~~~~~~
>  
> +* ** fix byte order inconsistence between fdir flow and mask **
> +
> +  Fixed issue in ethdev library that the structure for setting
> +  fdir's mask and flow entry is inconsist in byte order.

John, comment on release notes formatting?
It's important to have the first items well formatted.

> @@ -39,6 +43,8 @@ API Changes
>  ABI Changes
>  -----------
>  
> +* The fields in  The ethdev structures ``rte_eth_fdir_masks`` were
> +  changed to be in big endian.

Please take care of uppercase typo here.

> -	/* write all the same so that UDP, TCP and SCTP use the same mask */
> +	/* write all the same so that UDP, TCP and SCTP use the same mask
> +	 * (little-endian)
> +	*/

Spacing typo here.
Sorry for the nits ;)

> -	uint8_t mac_addr_byte_mask;  /** Per byte MAC address mask */
> +	uint8_t mac_addr_byte_mask;  /** Bit mask for associated byte */
>  	uint32_t tunnel_id_mask;  /** tunnel ID mask */
> -	uint8_t tunnel_type_mask;
> +	uint8_t tunnel_type_mask; /**< 1 - Match tunnel type,
> +				       0 - Ignore tunnel type. */

These changes seem unrelated with the patch.
It's good to improve doc of this API but it's maybe not enough.
Example:
	uint8_t mac_addr_byte_mask;  /** Bit mask for associated byte */
Are we sure everybody understand how to fill it?

Re: [PATCH] ethdev: fix byte order inconsistence between fdir flow and mask

[Wu, Jingjing]

> 
> 2016-01-27 16:37, Jingjing Wu:
> > Fixed issue of byte order in ethdev library that the structure for
> > setting fdir's mask and flow entry is inconsist and made inputs of
> > mask be in big endian.
> 
> Please be more precise. Which one is big endian?
> Wasn't it tested before?
> 
It is tested, it works. But byte order fields in the structures for fdir are not
consist, few are in host endian. It will make user confused, so this patch
is to make all fields in the same byte order.

> > fixes: 76c6f89e80d4 ("ixgbe: support new flow director masks")
> >        2d4c1a9ea2ac ("ethdev: add new flow director masks")
> 
> Please put Fixes: on the two lines.
> 
OK. Will add in later version.

> > --- a/doc/guides/rel_notes/release_2_3.rst
> > +++ b/doc/guides/rel_notes/release_2_3.rst
> > @@ -19,6 +19,10 @@ Drivers
> >  Libraries
> >  ~~~~~~~~~
> >
> > +* ** fix byte order inconsistence between fdir flow and mask **
> > +
> > +  Fixed issue in ethdev library that the structure for setting
> > + fdir's mask and flow entry is inconsist in byte order.
> 
> John, comment on release notes formatting?
> It's important to have the first items well formatted.
> 
> > @@ -39,6 +43,8 @@ API Changes
> >  ABI Changes
> >  -----------
> >
> > +* The fields in  The ethdev structures ``rte_eth_fdir_masks`` were
> > +  changed to be in big endian.
> 
> Please take care of uppercase typo here.
Ok. Thanks for point.
> 
> > -	/* write all the same so that UDP, TCP and SCTP use the same mask
> */
> > +	/* write all the same so that UDP, TCP and SCTP use the same mask
> > +	 * (little-endian)
> > +	*/
> 
> Spacing typo here.
> Sorry for the nits ;)
> 
Will fix this in later version.

> > -	uint8_t mac_addr_byte_mask;  /** Per byte MAC address mask */
> > +	uint8_t mac_addr_byte_mask;  /** Bit mask for associated byte */
> >  	uint32_t tunnel_id_mask;  /** tunnel ID mask */
> > -	uint8_t tunnel_type_mask;
> > +	uint8_t tunnel_type_mask; /**< 1 - Match tunnel type,
> > +				       0 - Ignore tunnel type. */
> 
> These changes seem unrelated with the patch.
> It's good to improve doc of this API but it's maybe not enough.
> Example:
> 	uint8_t mac_addr_byte_mask;  /** Bit mask for associated byte */
> Are we sure everybody understand how to fill it?

OK. Will improve this.

Thanks
JIngjing

[PATCH v2] ethdev: fix byte order inconsistence between fdir flow and mask

[Jingjing Wu]

Fixed issue of byte order in ethdev library that the structure
for setting fdir's mask and flow entry is inconsist and made
inputs of mask be in big endian.

Fixes: 76c6f89e80d4 ("ixgbe: support new flow director masks")
Fixes: 2d4c1a9ea2ac ("ethdev: add new flow director masks")

Reported-by: Yaacov Hazan <yaacovh@mellanox.com>
Signed-off-by: Jingjing Wu <jingjing.wu@intel.com>
---
v2 changes:
  fix typo and reword API doc.

 app/test-pmd/cmdline.c               |  6 ++---
 doc/guides/rel_notes/release_2_3.rst |  6 +++++
 drivers/net/ixgbe/ixgbe_fdir.c       | 47 ++++++++++++++++++++++--------------
 lib/librte_ether/rte_eth_ctrl.h      | 17 ++++++++++---
 4 files changed, 51 insertions(+), 25 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 73298c9..13194c9 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -8687,13 +8687,13 @@ cmd_flow_director_mask_parsed(void *parsed_result,
 			return;
 		}
 
-		mask->vlan_tci_mask = res->vlan_mask;
+		mask->vlan_tci_mask = rte_cpu_to_be_16(res->vlan_mask);
 		IPV4_ADDR_TO_UINT(res->ipv4_src, mask->ipv4_mask.src_ip);
 		IPV4_ADDR_TO_UINT(res->ipv4_dst, mask->ipv4_mask.dst_ip);
 		IPV6_ADDR_TO_ARRAY(res->ipv6_src, mask->ipv6_mask.src_ip);
 		IPV6_ADDR_TO_ARRAY(res->ipv6_dst, mask->ipv6_mask.dst_ip);
-		mask->src_port_mask = res->port_src;
-		mask->dst_port_mask = res->port_dst;
+		mask->src_port_mask = rte_cpu_to_be_16(res->port_src);
+		mask->dst_port_mask = rte_cpu_to_be_16(res->port_dst);
 	}
 
 	cmd_reconfig_device_queue(res->port_id, 1, 1);
diff --git a/doc/guides/rel_notes/release_2_3.rst b/doc/guides/rel_notes/release_2_3.rst
index 99de186..77e4edd 100644
--- a/doc/guides/rel_notes/release_2_3.rst
+++ b/doc/guides/rel_notes/release_2_3.rst
@@ -19,6 +19,10 @@ Drivers
 Libraries
 ~~~~~~~~~
 
+* ** fix byte order inconsistence between fdir flow and mask **
+
+  Fixed issue in ethdev library that the structure for setting
+  fdir's mask and flow entry is inconsist in byte order.
 
 Examples
 ~~~~~~~~
@@ -39,6 +43,8 @@ API Changes
 ABI Changes
 -----------
 
+* The fields in ethdev structure ``rte_eth_fdir_masks`` were
+  changed to be in big endian.
 
 Shared Library Versions
 -----------------------
diff --git a/drivers/net/ixgbe/ixgbe_fdir.c b/drivers/net/ixgbe/ixgbe_fdir.c
index e03219b..2c874b1 100644
--- a/drivers/net/ixgbe/ixgbe_fdir.c
+++ b/drivers/net/ixgbe/ixgbe_fdir.c
@@ -309,6 +309,7 @@ fdir_set_input_mask_82599(struct rte_eth_dev *dev,
 	uint32_t fdiripv6m; /* IPv6 source and destination masks. */
 	uint16_t dst_ipv6m = 0;
 	uint16_t src_ipv6m = 0;
+	volatile uint32_t *reg;
 
 	PMD_INIT_FUNC_TRACE();
 
@@ -322,16 +323,16 @@ fdir_set_input_mask_82599(struct rte_eth_dev *dev,
 		/* use the L4 protocol mask for raw IPv4/IPv6 traffic */
 		fdirm |= IXGBE_FDIRM_L4P;
 
-	if (input_mask->vlan_tci_mask == 0x0FFF)
+	if (input_mask->vlan_tci_mask == rte_cpu_to_be_16(0x0FFF))
 		/* mask VLAN Priority */
 		fdirm |= IXGBE_FDIRM_VLANP;
-	else if (input_mask->vlan_tci_mask == 0xE000)
+	else if (input_mask->vlan_tci_mask == rte_cpu_to_be_16(0xE000))
 		/* mask VLAN ID */
 		fdirm |= IXGBE_FDIRM_VLANID;
 	else if (input_mask->vlan_tci_mask == 0)
 		/* mask VLAN ID and Priority */
 		fdirm |= IXGBE_FDIRM_VLANID | IXGBE_FDIRM_VLANP;
-	else if (input_mask->vlan_tci_mask != 0xEFFF) {
+	else if (input_mask->vlan_tci_mask != rte_cpu_to_be_16(0xEFFF)) {
 		PMD_INIT_LOG(ERR, "invalid vlan_tci_mask");
 		return -EINVAL;
 	}
@@ -340,19 +341,26 @@ fdir_set_input_mask_82599(struct rte_eth_dev *dev,
 	IXGBE_WRITE_REG(hw, IXGBE_FDIRM, fdirm);
 
 	/* store the TCP/UDP port masks, bit reversed from port layout */
-	fdirtcpm = reverse_fdir_bitmasks(input_mask->dst_port_mask,
-					 input_mask->src_port_mask);
+	fdirtcpm = reverse_fdir_bitmasks(
+			rte_be_to_cpu_16(input_mask->dst_port_mask),
+			rte_be_to_cpu_16(input_mask->src_port_mask));
 
-	/* write all the same so that UDP, TCP and SCTP use the same mask */
+	/* write all the same so that UDP, TCP and SCTP use the same mask
+	 * (little-endian)
+	 */
 	IXGBE_WRITE_REG(hw, IXGBE_FDIRTCPM, ~fdirtcpm);
 	IXGBE_WRITE_REG(hw, IXGBE_FDIRUDPM, ~fdirtcpm);
 	IXGBE_WRITE_REG(hw, IXGBE_FDIRSCTPM, ~fdirtcpm);
 	info->mask.src_port_mask = input_mask->src_port_mask;
 	info->mask.dst_port_mask = input_mask->dst_port_mask;
 
-	/* Store source and destination IPv4 masks (big-endian) */
-	IXGBE_WRITE_REG(hw, IXGBE_FDIRSIP4M, ~(input_mask->ipv4_mask.src_ip));
-	IXGBE_WRITE_REG(hw, IXGBE_FDIRDIP4M, ~(input_mask->ipv4_mask.dst_ip));
+	/* Store source and destination IPv4 masks (big-endian),
+	 * can not use IXGBE_WRITE_REG.
+	 */
+	reg = IXGBE_PCI_REG_ADDR(hw, IXGBE_FDIRSIP4M);
+	*reg = ~(input_mask->ipv4_mask.src_ip);
+	reg = IXGBE_PCI_REG_ADDR(hw, IXGBE_FDIRDIP4M);
+	*reg = ~(input_mask->ipv4_mask.dst_ip);
 	info->mask.src_ipv4_mask = input_mask->ipv4_mask.src_ip;
 	info->mask.dst_ipv4_mask = input_mask->ipv4_mask.dst_ip;
 
@@ -401,16 +409,16 @@ fdir_set_input_mask_x550(struct rte_eth_dev *dev,
 	/* some bits must be set for mac vlan or tunnel mode */
 	fdirm |= IXGBE_FDIRM_L4P | IXGBE_FDIRM_L3P;
 
-	if (input_mask->vlan_tci_mask == 0x0FFF)
+	if (input_mask->vlan_tci_mask == rte_cpu_to_be_16(0x0FFF))
 		/* mask VLAN Priority */
 		fdirm |= IXGBE_FDIRM_VLANP;
-	else if (input_mask->vlan_tci_mask == 0xE000)
+	else if (input_mask->vlan_tci_mask == rte_cpu_to_be_16(0xE000))
 		/* mask VLAN ID */
 		fdirm |= IXGBE_FDIRM_VLANID;
 	else if (input_mask->vlan_tci_mask == 0)
 		/* mask VLAN ID and Priority */
 		fdirm |= IXGBE_FDIRM_VLANID | IXGBE_FDIRM_VLANP;
-	else if (input_mask->vlan_tci_mask != 0xEFFF) {
+	else if (input_mask->vlan_tci_mask != rte_cpu_to_be_16(0xEFFF)) {
 		PMD_INIT_LOG(ERR, "invalid vlan_tci_mask");
 		return -EINVAL;
 	}
@@ -444,7 +452,7 @@ fdir_set_input_mask_x550(struct rte_eth_dev *dev,
 		info->mask.tunnel_type_mask =
 			input_mask->tunnel_type_mask;
 
-		switch (input_mask->tunnel_id_mask & 0xFFFFFFFF) {
+		switch (rte_be_to_cpu_32(input_mask->tunnel_id_mask)) {
 		case 0x0:
 			/* Mask vxlan id */
 			fdiripv6m |= IXGBE_FDIRIP6M_TNI_VNI;
@@ -904,13 +912,16 @@ fdir_write_perfect_filter_82599(struct ixgbe_hw *hw,
 	u32 addr_low, addr_high;
 	u32 tunnel_type = 0;
 	int err = 0;
+	volatile uint32_t *reg;
 
 	if (mode == RTE_FDIR_MODE_PERFECT) {
-		/* record the IPv4 address (big-endian) */
-		IXGBE_WRITE_REG(hw, IXGBE_FDIRIPSA,
-				input->formatted.src_ip[0]);
-		IXGBE_WRITE_REG(hw, IXGBE_FDIRIPDA,
-				input->formatted.dst_ip[0]);
+		/* record the IPv4 address (big-endian)
+		 * can not use IXGBE_WRITE_REG.
+		 */
+		reg = IXGBE_PCI_REG_ADDR(hw, IXGBE_FDIRIPSA);
+		*reg = input->formatted.src_ip[0];
+		reg = IXGBE_PCI_REG_ADDR(hw, IXGBE_FDIRIPDA);
+		*reg = input->formatted.dst_ip[0];
 
 		/* record source and destination port (little-endian)*/
 		fdirport = IXGBE_NTOHS(input->formatted.dst_port);
diff --git a/lib/librte_ether/rte_eth_ctrl.h b/lib/librte_ether/rte_eth_ctrl.h
index ce224ad..d433e0b 100644
--- a/lib/librte_ether/rte_eth_ctrl.h
+++ b/lib/librte_ether/rte_eth_ctrl.h
@@ -501,6 +501,7 @@ struct rte_eth_tunnel_flow {
 
 /**
  * An union contains the inputs for all types of flow
+ * Items in flows need to be in big endian
  */
 union rte_eth_fdir_flow {
 	struct rte_eth_l2_flow     l2_flow;
@@ -588,14 +589,22 @@ struct rte_eth_fdir_filter {
  *  to match the various fields of RX packet headers.
  */
 struct rte_eth_fdir_masks {
-	uint16_t vlan_tci_mask;
+	uint16_t vlan_tci_mask;   /**< Bit mask for vlan_tci in big endian */
+	/** Bit mask for ipv4 flow in big endian. */
 	struct rte_eth_ipv4_flow   ipv4_mask;
+	/** Bit maks for ipv6 flow in big endian. */
 	struct rte_eth_ipv6_flow   ipv6_mask;
+	/** Bit mask for L4 source port in big endian. */
 	uint16_t src_port_mask;
+	/** Bit mask for L4 destination port in big endian. */
 	uint16_t dst_port_mask;
-	uint8_t mac_addr_byte_mask;  /** Per byte MAC address mask */
-	uint32_t tunnel_id_mask;  /** tunnel ID mask */
-	uint8_t tunnel_type_mask;
+	/** 6 bit mask for proper 6 bytes of Mac address, bit 0 matches the
+	    first byte on the wire */
+	uint8_t mac_addr_byte_mask;
+	/** Bit mask for tunnel ID in big endian. */
+	uint32_t tunnel_id_mask;
+	uint8_t tunnel_type_mask; /**< 1 - Match tunnel type,
+				       0 - Ignore tunnel type. */
 };
 
 /**
-- 
2.4.0

Re: [PATCH v2] ethdev: fix byte order inconsistence between fdir flow and mask

[Zhe Tao]

On Mon, Feb 01, 2016 at 10:48:21AM +0800, Jingjing Wu wrote:
> Fixed issue of byte order in ethdev library that the structure
> for setting fdir's mask and flow entry is inconsist and made
> inputs of mask be in big endian.
> 
> Fixes: 76c6f89e80d4 ("ixgbe: support new flow director masks")
> Fixes: 2d4c1a9ea2ac ("ethdev: add new flow director masks")
> 
> Reported-by: Yaacov Hazan <yaacovh@mellanox.com>
> Signed-off-by: Jingjing Wu <jingjing.wu@intel.com>
> ---
> v2 changes:
>   fix typo and reword API doc.
> 
>  app/test-pmd/cmdline.c               |  6 ++---
>  doc/guides/rel_notes/release_2_3.rst |  6 +++++
>  drivers/net/ixgbe/ixgbe_fdir.c       | 47 ++++++++++++++++++++++--------------
>  lib/librte_ether/rte_eth_ctrl.h      | 17 ++++++++++---
>  4 files changed, 51 insertions(+), 25 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 73298c9..13194c9 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -8687,13 +8687,13 @@ cmd_flow_director_mask_parsed(void *parsed_result,
>  			return;
>  		}
Acked-by: Zhe Tao <zhe.tao@intel.com>

Re: [PATCH v2] ethdev: fix byte order inconsistence between fdir flow and mask

[Lu, Wenzhuo]

Hi,


> -----Original Message-----
> From: Wu, Jingjing
> Sent: Monday, February 1, 2016 10:48 AM
> To: dev@dpdk.org
> Cc: Wu, Jingjing; Lu, Wenzhuo; Pei, Yulong; yaacovh@mellanox.com
> Subject: [PATCH v2] ethdev: fix byte order inconsistence between fdir flow and
> mask
> 
> Fixed issue of byte order in ethdev library that the structure for setting fdir's
> mask and flow entry is inconsist and made inputs of mask be in big endian.
> 
> Fixes: 76c6f89e80d4 ("ixgbe: support new flow director masks")
> Fixes: 2d4c1a9ea2ac ("ethdev: add new flow director masks")
> 
> Reported-by: Yaacov Hazan <yaacovh@mellanox.com>
> Signed-off-by: Jingjing Wu <jingjing.wu@intel.com>
Acked-by: Wenzhuo Lu <wenzhuo.lu@intel.com>

Re: [PATCH v2] ethdev: fix byte order inconsistence between fdir flow and mask

[Thomas Monjalon]

> > Fixed issue of byte order in ethdev library that the structure for setting fdir's
> > mask and flow entry is inconsist and made inputs of mask be in big endian.
> > 
> > Fixes: 76c6f89e80d4 ("ixgbe: support new flow director masks")
> > Fixes: 2d4c1a9ea2ac ("ethdev: add new flow director masks")
> > 
> > Reported-by: Yaacov Hazan <yaacovh@mellanox.com>
> > Signed-off-by: Jingjing Wu <jingjing.wu@intel.com>
> Acked-by: Wenzhuo Lu <wenzhuo.lu@intel.com>

Applied, thanks

Re: [PATCH v2] ethdev: fix byte order inconsistence between fdir flow and mask

[Thomas Monjalon]

Hi Jingjing,

2016-02-01 10:48, Jingjing Wu:
> @@ -39,6 +43,8 @@ API Changes
>  ABI Changes
>  -----------
>  
> +* The fields in ethdev structure ``rte_eth_fdir_masks`` were
> +  changed to be in big endian.

I think it is an API change.
Please could you fix it? Thanks