diff mbox

[PART2,2/9] i40e: check current configured input set when adding ntuple filters

Message ID 20170206223852.31177-3-jacob.e.keller@intel.com
State Superseded
Delegated to: Jeff Kirsher
Headers show

Commit Message

Keller, Jacob E Feb. 6, 2017, 10:38 p.m. UTC
Do not assume that hardware has been programmed with the default mask,
but instead read the input set registers to determine what is currently
programmed. This ensures that all programmed filters match exactly how
the hardware will interpret them, avoiding confusion regarding filter
behavior.

This sets the initial ground-work for allowing custom input sets where
some fields are disabled. A future patch will fully implement this
feature.

The use of ntohl and ntohs is not strictly necessary for correctness,
but it helps avoid a sparse warning due to the construction of
"!~<be16>" degrades the be16 value into an integer when doing negation.
This could alternatively be fixed by the following code construction
without the byteswaps,

  if (!<value>)
    <disable field>
  else if (~<value>)
    <return error>
  else
    <enable field>

However, this construction is confusing because the error return occurs
in the middle, which is misleading to other readers. Additionally, it
makes the condition for the <enable field> much more difficult to
process due to it being in an else statement.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Change-Id: 3d8db46cb28ea0afdaac8c5b31a2bfb90e3a4102
---
 drivers/net/ethernet/intel/i40e/i40e.h         |  19 ++++
 drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 117 +++++++++++++++++++++----
 2 files changed, 121 insertions(+), 15 deletions(-)

Comments

Bowers, AndrewX March 20, 2017, 5:50 p.m. UTC | #1
> -----Original Message-----
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On
> Behalf Of Jacob Keller
> Sent: Monday, February 6, 2017 2:39 PM
> To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>
> Subject: [Intel-wired-lan] [PART2 PATCH 2/9] i40e: check current configured
> input set when adding ntuple filters
> 
> Do not assume that hardware has been programmed with the default mask,
> but instead read the input set registers to determine what is currently
> programmed. This ensures that all programmed filters match exactly how the
> hardware will interpret them, avoiding confusion regarding filter behavior.
> 
> This sets the initial ground-work for allowing custom input sets where some
> fields are disabled. A future patch will fully implement this feature.
> 
> The use of ntohl and ntohs is not strictly necessary for correctness, but it
> helps avoid a sparse warning due to the construction of "!~<be16>" degrades
> the be16 value into an integer when doing negation.
> This could alternatively be fixed by the following code construction without
> the byteswaps,
> 
>   if (!<value>)
>     <disable field>
>   else if (~<value>)
>     <return error>
>   else
>     <enable field>
> 
> However, this construction is confusing because the error return occurs in
> the middle, which is misleading to other readers. Additionally, it makes the
> condition for the <enable field> much more difficult to process due to it
> being in an else statement.
> 
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Change-Id: 3d8db46cb28ea0afdaac8c5b31a2bfb90e3a4102
> ---
>  drivers/net/ethernet/intel/i40e/i40e.h         |  19 ++++
>  drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 117
> +++++++++++++++++++++----
>  2 files changed, 121 insertions(+), 15 deletions(-)

Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
diff mbox

Patch

diff --git a/drivers/net/ethernet/intel/i40e/i40e.h b/drivers/net/ethernet/intel/i40e/i40e.h
index 9957eca8f71d..c32658ca5c22 100644
--- a/drivers/net/ethernet/intel/i40e/i40e.h
+++ b/drivers/net/ethernet/intel/i40e/i40e.h
@@ -742,6 +742,25 @@  static inline int i40e_get_fd_cnt_all(struct i40e_pf *pf)
 	return pf->hw.fdir_shared_filter_count + pf->fdir_pf_filter_count;
 }
 
+/**
+ * i40e_read_fd_input_set - reads value of flow director input set register
+ * @pf: pointer to the PF struct
+ * @addr: register addr
+ *
+ * This function reads value of flow director input set register
+ * specified by 'addr' (which is specific to flow-type)
+ **/
+static inline u64 i40e_read_fd_input_set(struct i40e_pf *pf, u16 addr)
+{
+	u64 val;
+
+	val = i40e_read_rx_ctl(&pf->hw, I40E_PRTQF_FD_INSET(addr, 1));
+	val <<= 32;
+	val += i40e_read_rx_ctl(&pf->hw, I40E_PRTQF_FD_INSET(addr, 0));
+
+	return val;
+}
+
 /* needed by i40e_ethtool.c */
 int i40e_up(struct i40e_vsi *vsi);
 void i40e_down(struct i40e_vsi *vsi);
diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
index 772dd234d581..5a7718e67c0a 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
@@ -2346,6 +2346,8 @@  static int i40e_get_ethtool_fdir_entry(struct i40e_pf *pf,
 			(struct ethtool_rx_flow_spec *)&cmd->fs;
 	struct i40e_fdir_filter *rule = NULL;
 	struct hlist_node *node2;
+	u64 input_set;
+	u16 index;
 
 	hlist_for_each_entry_safe(rule, node2,
 				  &pf->fdir_filter_list, fdir_node) {
@@ -2371,11 +2373,42 @@  static int i40e_get_ethtool_fdir_entry(struct i40e_pf *pf,
 	fsp->h_u.tcp_ip4_spec.ip4src = rule->dst_ip;
 	fsp->h_u.tcp_ip4_spec.ip4dst = rule->src_ip;
 
-	/* Set the mask fields */
-	fsp->m_u.tcp_ip4_spec.psrc = htons(0xFFFF);
-	fsp->m_u.tcp_ip4_spec.pdst = htons(0xFFFF);
-	fsp->m_u.tcp_ip4_spec.ip4src = htonl(0xFFFFFFFF);
-	fsp->m_u.tcp_ip4_spec.ip4dst = htonl(0xFFFFFFFF);
+	switch (rule->flow_type) {
+	case TCP_V4_FLOW:
+		index = I40E_FILTER_PCTYPE_NONF_IPV4_TCP;
+		break;
+	case UDP_V4_FLOW:
+		index = I40E_FILTER_PCTYPE_NONF_IPV4_UDP;
+		break;
+	case IP_USER_FLOW:
+		index = I40E_FILTER_PCTYPE_NONF_IPV4_OTHER;
+		break;
+	default:
+		/* If we have stored a filter with a flow type not listed here
+		 * it is almost certainly a driver bug. WARN(), and then
+		 * assign the input_set as if all fields are enabled to avoid
+		 * reading unassigned memory.
+		 */
+		WARN(1, "Missing input set index for flow_type %d\n",
+		     rule->flow_type);
+		input_set = 0xFFFFFFFFFFFFFFFFULL;
+		goto no_input_set;
+	}
+
+	input_set = i40e_read_fd_input_set(pf, index);
+
+no_input_set:
+	if (input_set & I40E_L3_SRC_MASK)
+		fsp->m_u.tcp_ip4_spec.ip4src = htonl(0xFFFF);
+
+	if (input_set & I40E_L3_DST_MASK)
+		fsp->m_u.tcp_ip4_spec.ip4dst = htonl(0xFFFF);
+
+	if (input_set & I40E_L4_SRC_MASK)
+		fsp->m_u.tcp_ip4_spec.psrc = htons(0xFFFFFFFF);
+
+	if (input_set & I40E_L4_DST_MASK)
+		fsp->m_u.tcp_ip4_spec.pdst = htons(0xFFFFFFFF);
 
 	if (rule->dest_ctl == I40E_FILTER_PROGRAM_DESC_DEST_DROP_PACKET)
 		fsp->ring_cookie = RX_CLS_FLOW_DISC;
@@ -2687,36 +2720,74 @@  static int i40e_del_fdir_entry(struct i40e_vsi *vsi,
 
 /**
  * i40e_check_fdir_input_set - Check that a given rx_flow_spec mask is valid
+ * @vsi: pointer to the targeted VSI
  * @fsp: pointer to Rx flow specification
  *
  * Ensures that a given ethtool_rx_flow_spec has a valid mask.
  **/
-static int i40e_check_fdir_input_set(struct ethtool_rx_flow_spec *fsp)
+static int i40e_check_fdir_input_set(struct i40e_vsi *vsi,
+				     struct ethtool_rx_flow_spec *fsp)
 {
+	struct i40e_pf *pf = vsi->back;
 	struct ethtool_tcpip4_spec *tcp_ip4_spec;
 	struct ethtool_usrip4_spec *usr_ip4_spec;
+	u64 current_mask, new_mask;
+	u16 index;
+
+	switch (fsp->flow_type & ~FLOW_EXT) {
+	case TCP_V4_FLOW:
+		index = I40E_FILTER_PCTYPE_NONF_IPV4_TCP;
+		break;
+	case UDP_V4_FLOW:
+		index = I40E_FILTER_PCTYPE_NONF_IPV4_UDP;
+		break;
+	case IP_USER_FLOW:
+		index = I40E_FILTER_PCTYPE_NONF_IPV4_OTHER;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	/* Read the current input set from register memory. */
+	current_mask = i40e_read_fd_input_set(pf, index);
+	new_mask = current_mask;
 
 	/* Verify the provided mask is valid. */
 	switch (fsp->flow_type & ~FLOW_EXT) {
-	case SCTP_V4_FLOW:
 	case TCP_V4_FLOW:
 	case UDP_V4_FLOW:
 		tcp_ip4_spec = &fsp->m_u.tcp_ip4_spec;
 
 		/* IPv4 source address */
-		if (!tcp_ip4_spec->ip4src || ~tcp_ip4_spec->ip4src)
+		if (!tcp_ip4_spec->ip4src)
+			new_mask &= ~I40E_L3_SRC_MASK;
+		else if (!~ntohl(tcp_ip4_spec->ip4src))
+			new_mask |= I40E_L3_SRC_MASK;
+		else
 			return -EOPNOTSUPP;
 
 		/* IPv4 destination address */
-		if (!tcp_ip4_spec->ip4dst || ~tcp_ip4_spec->ip4dst)
+		if (!tcp_ip4_spec->ip4dst)
+			new_mask &= ~I40E_L3_DST_MASK;
+		else if (!~ntohl(tcp_ip4_spec->ip4dst))
+			new_mask |= I40E_L3_DST_MASK;
+		else
 			return -EOPNOTSUPP;
 
 		/* L4 source port */
-		if (!tcp_ip4_spec->psrc || ~tcp_ip4_spec->psrc)
+		if (!tcp_ip4_spec->psrc)
+			new_mask &= ~I40E_L4_SRC_MASK;
+		else if (!~ntohs(tcp_ip4_spec->psrc))
+			new_mask |= I40E_L4_SRC_MASK;
+		else
 			return -EOPNOTSUPP;
 
 		/* L4 destination port */
-		if (!tcp_ip4_spec->pdst || ~tcp_ip4_spec->pdst)
+		if (!tcp_ip4_spec->pdst)
+			new_mask &= ~I40E_L4_DST_MASK;
+		else if (!~ntohs(tcp_ip4_spec->pdst))
+			new_mask |= I40E_L4_DST_MASK;
+		else
 			return -EOPNOTSUPP;
 
 		/* Filtering on Type of Service is not supported. */
@@ -2728,15 +2799,27 @@  static int i40e_check_fdir_input_set(struct ethtool_rx_flow_spec *fsp)
 		usr_ip4_spec = &fsp->m_u.usr_ip4_spec;
 
 		/* IPv4 source address */
-		if (!usr_ip4_spec->ip4src || ~usr_ip4_spec->ip4src)
+		if (!usr_ip4_spec->ip4src)
+			new_mask &= ~I40E_L3_SRC_MASK;
+		else if (!~ntohl(usr_ip4_spec->ip4src))
+			new_mask |= I40E_L3_SRC_MASK;
+		else
 			return -EOPNOTSUPP;
 
 		/* IPv4 destination address */
-		if (!usr_ip4_spec->ip4dst || ~usr_ip4_spec->ip4dst)
+		if (!usr_ip4_spec->ip4dst)
+			new_mask &= ~I40E_L3_DST_MASK;
+		else if (!~ntohl(usr_ip4_spec->ip4dst))
+			new_mask |= I40E_L3_DST_MASK;
+		else
 			return -EOPNOTSUPP;
 
 		/* First 4 bytes of L4 header */
-		if (!usr_ip4_spec->l4_4_bytes || ~usr_ip4_spec->l4_4_bytes)
+		if (!usr_ip4_spec->l4_4_bytes)
+			new_mask &= ~(I40E_L4_SRC_MASK | I40E_L4_DST_MASK);
+		else if (!~ntohl(usr_ip4_spec->l4_4_bytes))
+			new_mask |= I40E_L4_SRC_MASK | I40E_L4_DST_MASK;
+		else
 			return -EOPNOTSUPP;
 
 		/* Filtering on Type of Service is not supported. */
@@ -2750,11 +2833,15 @@  static int i40e_check_fdir_input_set(struct ethtool_rx_flow_spec *fsp)
 		/* L4 protocol doesn't have a mask field. */
 		if (usr_ip4_spec->proto)
 			return -EINVAL;
+
 		break;
 	default:
 		return -EOPNOTSUPP;
 	}
 
+	if (new_mask != current_mask)
+		return -EOPNOTSUPP;
+
 	return 0;
 }
 
@@ -2798,7 +2885,7 @@  static int i40e_add_fdir_ethtool(struct i40e_vsi *vsi,
 	if (fsp->flow_type & FLOW_MAC_EXT)
 		return -EINVAL;
 
-	ret = i40e_check_fdir_input_set(fsp);
+	ret = i40e_check_fdir_input_set(vsi, fsp);
 	if (ret)
 		return ret;