[PATCH net-next] bnx2x: add ethtool support to read and change RSS key

[PATCH net-next] bnx2x: add ethtool support to read and change RSS key

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

While trying to understand why bnx2x RSS hash was not matching
Toeplitz specifications, I implemented driver code to support
ethtool -x|-X to try various RSS key combinations easily.

Then, after some debugging, I understood bnx2x was reading the rss_key
in reverse order.

If we want consistent Toeplitz hashes with different NIC, we need
to byte swap user key.

Tested:

lpk51:~# cat /proc/sys/net/core/netdev_rss_key
61:4a:0d:3e:2e:b1:30:cd:42:f5:0f:9c:a9:71:dd:1c:2f:f1:d6:60:d1:de:b2:b2:1e:bd:3c:ed:bf:84:e3:e3:d5:88:a2:12:e4:f4:4a:f6:1a:10:c0:16:b4:56:4f:aa:a4:fa:0a:b1

lpk51:~# ethtool -x eth1 | tail -2
RSS hash key:
61:4a:0d:3e:2e:b1:30:cd:42:f5:0f:9c:a9:71:dd:1c:2f:f1:d6:60:d1:de:b2:b2:1e:bd:3c:ed:bf:84:e3:e3:d5:88:a2:12:e4:f4:4a:f6

lpk51:~# ethtool -X eth1 hkey
03:0e:e2:43:fa:82:0e:73:14:2d:c0:68:21:9e:82:99:b9:84:d0:22:e2:b3:64:9f:4a:af:00:fa:cc:05:b4:4a:17:05:14:73:76:58:bd:2f

lpk51:~# ethtool -x eth1 | tail -2
RSS hash key:
03:0e:e2:43:fa:82:0e:73:14:2d:c0:68:21:9e:82:99:b9:84:d0:22:e2:b3:64:9f:4a:af:00:fa:cc:05:b4:4a:17:05:14:73:76:58:bd:2f


Signed-off-by: Eric Dumazet <edumazet@google.com>
---
Since rss_key was random, I guess there is no hurry to fix this bug in
stable kernels, and/or to split this patch in two parts.

 drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c     |    7 
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h     |    4 
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c |   77 +++++-----
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_sp.c      |   11 +
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_sp.h      |    5 
 5 files changed, 60 insertions(+), 44 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
index 44173be..5f06267 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
@@ -2073,7 +2073,7 @@ static int bnx2x_init_rss(struct bnx2x *bp)
 	 * For 57712 and newer on the other hand it's a per-function
 	 * configuration.
 	 */
-	return bnx2x_config_rss_eth(bp, bp->port.pmf || !CHIP_IS_E1x(bp));
+	return bnx2x_config_rss_eth(bp);
 }
 
 int bnx2x_rss(struct bnx2x *bp, struct bnx2x_rss_config_obj *rss_obj,
@@ -2122,11 +2122,8 @@ int bnx2x_rss(struct bnx2x *bp, struct bnx2x_rss_config_obj *rss_obj,
 
 	memcpy(params.ind_table, rss_obj->ind_table, sizeof(params.ind_table));
 
-	if (config_hash) {
-		/* RSS keys */
-		netdev_rss_key_fill(params.rss_key, T_ETH_RSS_KEY * 4);
+	if (config_hash)
 		__set_bit(BNX2X_RSS_SET_SRCH, &params.rss_flags);
-	}
 
 	if (IS_PF(bp))
 		return bnx2x_config_rss(bp, &params);
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h
index b7d32e8..c5d6cce 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h
@@ -913,8 +913,10 @@ static inline int func_by_vn(struct bnx2x *bp, int vn)
 	return 2 * vn + BP_PORT(bp);
 }
 
-static inline int bnx2x_config_rss_eth(struct bnx2x *bp, bool config_hash)
+static inline int bnx2x_config_rss_eth(struct bnx2x *bp)
 {
+	bool config_hash = bp->port.pmf || !CHIP_IS_E1x(bp);
+
 	return bnx2x_rss(bp, &bp->rss_conf_obj, config_hash, true);
 }
 
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
index aeb7ce6..0c613fa 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
@@ -3419,6 +3419,11 @@ static u32 bnx2x_get_rxfh_indir_size(struct net_device *dev)
 	return T_ETH_INDIRECTION_TABLE_SIZE;
 }
 
+static u32 bnx2x_get_rxfh_key_size(struct net_device *dev)
+{
+	return T_ETH_RSS_KEY * 4;
+}
+
 static int bnx2x_get_rxfh(struct net_device *dev, u32 *indir, u8 *key,
 			  u8 *hfunc)
 {
@@ -3428,24 +3433,25 @@ static int bnx2x_get_rxfh(struct net_device *dev, u32 *indir, u8 *key,
 
 	if (hfunc)
 		*hfunc = ETH_RSS_HASH_TOP;
-	if (!indir)
-		return 0;
+	if (indir) {
 
-	/* Get the current configuration of the RSS indirection table */
-	bnx2x_get_rss_ind_table(&bp->rss_conf_obj, ind_table);
-
-	/*
-	 * We can't use a memcpy() as an internal storage of an
-	 * indirection table is a u8 array while indir->ring_index
-	 * points to an array of u32.
-	 *
-	 * Indirection table contains the FW Client IDs, so we need to
-	 * align the returned table to the Client ID of the leading RSS
-	 * queue.
-	 */
-	for (i = 0; i < T_ETH_INDIRECTION_TABLE_SIZE; i++)
-		indir[i] = ind_table[i] - bp->fp->cl_id;
+		/* Get the current configuration of the RSS indirection table */
+		bnx2x_get_rss_ind_table(&bp->rss_conf_obj, ind_table);
 
+		/*
+		 * We can't use a memcpy() as an internal storage of an
+		 * indirection table is a u8 array while indir->ring_index
+		 * points to an array of u32.
+		 *
+		 * Indirection table contains the FW Client IDs, so we need to
+		 * align the returned table to the Client ID of the leading RSS
+		 * queue.
+		 */
+		for (i = 0; i < T_ETH_INDIRECTION_TABLE_SIZE; i++)
+			indir[i] = ind_table[i] - bp->fp->cl_id;
+	}
+	if (key)
+		memcpy(key, bp->rss_conf_obj.rss_key, T_ETH_RSS_KEY * 4);
 	return 0;
 }
 
@@ -3453,32 +3459,34 @@ static int bnx2x_set_rxfh(struct net_device *dev, const u32 *indir,
 			  const u8 *key, const u8 hfunc)
 {
 	struct bnx2x *bp = netdev_priv(dev);
-	size_t i;
 
 	/* We require at least one supported parameter to be changed and no
 	 * change in any of the unsupported parameters
 	 */
-	if (key ||
-	    (hfunc != ETH_RSS_HASH_NO_CHANGE && hfunc != ETH_RSS_HASH_TOP))
+	if (hfunc != ETH_RSS_HASH_NO_CHANGE && hfunc != ETH_RSS_HASH_TOP)
 		return -EOPNOTSUPP;
 
-	if (!indir)
-		return 0;
-
-	for (i = 0; i < T_ETH_INDIRECTION_TABLE_SIZE; i++) {
-		/*
-		 * The same as in bnx2x_get_rxfh: we can't use a memcpy()
-		 * as an internal storage of an indirection table is a u8 array
-		 * while indir->ring_index points to an array of u32.
-		 *
-		 * Indirection table contains the FW Client IDs, so we need to
-		 * align the received table to the Client ID of the leading RSS
-		 * queue
-		 */
-		bp->rss_conf_obj.ind_table[i] = indir[i] + bp->fp->cl_id;
+	if (indir) {
+		size_t i;
+
+		for (i = 0; i < T_ETH_INDIRECTION_TABLE_SIZE; i++) {
+			/*
+			 * The same as in bnx2x_get_rxfh: we can't use a memcpy()
+			 * as an internal storage of an indirection table is a u8 array
+			 * while indir->ring_index points to an array of u32.
+			 *
+			 * Indirection table contains the FW Client IDs, so we need to
+			 * align the received table to the Client ID of the leading RSS
+			 * queue
+			 */
+			bp->rss_conf_obj.ind_table[i] = indir[i] + bp->fp->cl_id;
+		}
 	}
 
-	return bnx2x_config_rss_eth(bp, false);
+	if (key)
+		memcpy(bp->rss_conf_obj.rss_key, key, T_ETH_RSS_KEY * 4);
+
+	return bnx2x_config_rss_eth(bp);
 }
 
 /**
@@ -3627,6 +3635,7 @@ static const struct ethtool_ops bnx2x_ethtool_ops = {
 	.get_rxnfc		= bnx2x_get_rxnfc,
 	.set_rxnfc		= bnx2x_set_rxnfc,
 	.get_rxfh_indir_size	= bnx2x_get_rxfh_indir_size,
+	.get_rxfh_key_size	= bnx2x_get_rxfh_key_size,
 	.get_rxfh		= bnx2x_get_rxfh,
 	.set_rxfh		= bnx2x_set_rxfh,
 	.get_channels		= bnx2x_get_channels,
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sp.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sp.c
index c9bd7f1..90d2a2f 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sp.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sp.c
@@ -4319,8 +4319,14 @@ static int bnx2x_setup_rss(struct bnx2x *bp,
 
 	/* RSS keys */
 	if (test_bit(BNX2X_RSS_SET_SRCH, &p->rss_flags)) {
-		memcpy(&data->rss_key[0], &p->rss_key[0],
-		       sizeof(data->rss_key));
+		u8 *dst = (u8 *)(data->rss_key) + sizeof(data->rss_key);
+		const u8 *src = (const u8 *)bp->rss_conf_obj.rss_key;
+		int i;
+
+		/* Apparently, bnx2x reads this array in reverse order */
+		for (i = 0; i < sizeof(data->rss_key); i++)
+			*--dst = *src++;
+
 		caps |= ETH_RSS_UPDATE_RAMROD_DATA_UPDATE_RSS_KEY;
 	}
 
@@ -4408,6 +4414,7 @@ void bnx2x_init_rss_config_obj(struct bnx2x *bp,
 	bnx2x_init_raw_obj(&rss_obj->raw, cl_id, cid, func_id, rdata,
 			   rdata_mapping, state, pstate, type);
 
+	netdev_rss_key_fill(rss_obj->rss_key, sizeof(rss_obj->rss_key));
 	rss_obj->engine_id  = engine_id;
 	rss_obj->config_rss = bnx2x_setup_rss;
 }
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sp.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sp.h
index 4048fc5..6431979 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sp.h
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sp.h
@@ -734,8 +734,6 @@ struct bnx2x_config_rss_params {
 	/* Indirection table */
 	u8		ind_table[T_ETH_INDIRECTION_TABLE_SIZE];
 
-	/* RSS hash values */
-	u32		rss_key[10];
 
 	/* valid only iff BNX2X_RSS_UPDATE_TOE is set */
 	u16		toe_rss_bitmap;
@@ -754,6 +752,9 @@ struct bnx2x_rss_config_obj {
 	u8			udp_rss_v4;
 	u8			udp_rss_v6;
 
+	/* RSS hash values */
+	u32		rss_key[T_ETH_RSS_KEY];
+
 	int (*config_rss)(struct bnx2x *bp,
 			  struct bnx2x_config_rss_params *p);
 };

Re: [PATCH net-next] bnx2x: add ethtool support to read and change RSS key

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 18 Sep 2015 15:21:52 -0700

> From: Eric Dumazet <edumazet@google.com>
> 
> While trying to understand why bnx2x RSS hash was not matching
> Toeplitz specifications, I implemented driver code to support
> ethtool -x|-X to try various RSS key combinations easily.
> 
> Then, after some debugging, I understood bnx2x was reading the rss_key
> in reverse order.
> 
> If we want consistent Toeplitz hashes with different NIC, we need
> to byte swap user key.
> 
> Tested:
 ...
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied, thanks Eric.

Re: [PATCH net-next] bnx2x: add ethtool support to read and change RSS key

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Tue, 22 Sep 2015 14:59:35 -0700 (PDT)

> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Fri, 18 Sep 2015 15:21:52 -0700
> 
>> From: Eric Dumazet <edumazet@google.com>
>> 
>> While trying to understand why bnx2x RSS hash was not matching
>> Toeplitz specifications, I implemented driver code to support
>> ethtool -x|-X to try various RSS key combinations easily.
>> 
>> Then, after some debugging, I understood bnx2x was reading the rss_key
>> in reverse order.
>> 
>> If we want consistent Toeplitz hashes with different NIC, we need
>> to byte swap user key.
>> 
>> Tested:
>  ...
>> Signed-off-by: Eric Dumazet <edumazet@google.com>
> 
> Applied, thanks Eric.

Reverting, this doesn't compile:

drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c: In function ‘bnx2x_vfpf_config_rss’:
drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c:814:29: error: ‘struct bnx2x_config_rss_params’ has no member named ‘rss_key’
  memcpy(req->rss_key, params->rss_key, sizeof(params->rss_key));
                             ^
drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c:814:53: error: ‘struct bnx2x_config_rss_params’ has no member named ‘rss_key’
  memcpy(req->rss_key, params->rss_key, sizeof(params->rss_key));
                                                     ^
drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c: In function ‘bnx2x_vf_mbx_update_rss’:
drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c:1988:12: error: ‘struct bnx2x_config_rss_params’ has no member named ‘rss_key’
  memcpy(rss.rss_key, rss_tlv->rss_key, sizeof(rss_tlv->rss_key));
            ^

Re: [PATCH net-next] bnx2x: add ethtool support to read and change RSS key

[Eric Dumazet]

On Tue, 2015-09-22 at 16:06 -0700, David Miller wrote:
> From: David Miller <davem@davemloft.net>
> Date: Tue, 22 Sep 2015 14:59:35 -0700 (PDT)
> 
> > From: Eric Dumazet <eric.dumazet@gmail.com>
> > Date: Fri, 18 Sep 2015 15:21:52 -0700
> > 
> >> From: Eric Dumazet <edumazet@google.com>
> >> 
> >> While trying to understand why bnx2x RSS hash was not matching
> >> Toeplitz specifications, I implemented driver code to support
> >> ethtool -x|-X to try various RSS key combinations easily.
> >> 
> >> Then, after some debugging, I understood bnx2x was reading the rss_key
> >> in reverse order.
> >> 
> >> If we want consistent Toeplitz hashes with different NIC, we need
> >> to byte swap user key.
> >> 
> >> Tested:
> >  ...
> >> Signed-off-by: Eric Dumazet <edumazet@google.com>
> > 
> > Applied, thanks Eric.
> 
> Reverting, this doesn't compile:
> 
> drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c: In function ‘bnx2x_vfpf_config_rss’:
> drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c:814:29: error: ‘struct bnx2x_config_rss_params’ has no member named ‘rss_key’
>   memcpy(req->rss_key, params->rss_key, sizeof(params->rss_key));
>                              ^
> drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c:814:53: error: ‘struct bnx2x_config_rss_params’ has no member named ‘rss_key’
>   memcpy(req->rss_key, params->rss_key, sizeof(params->rss_key));
>                                                      ^
> drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c: In function ‘bnx2x_vf_mbx_update_rss’:
> drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c:1988:12: error: ‘struct bnx2x_config_rss_params’ has no member named ‘rss_key’
>   memcpy(rss.rss_key, rss_tlv->rss_key, sizeof(rss_tlv->rss_key));
>  

Ah sorry for this.

I'll submit the byte-swapping fix only separately.

[PATCH net] bnx2x: byte swap rss_key to comply to Toeplitz specs

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

After a good amount of debugging, I found bnx2x was byte swaping
the 40 bytes of rss_key.

If we byte swap the key, then bnx2x generates hashes matching
MSDN specs as documented in (Verifying the RSS Hash Calculation)

https://msdn.microsoft.com/en-us/library/windows/hardware/ff571021%
28v=vs.85%29.aspx

It is mostly a non issue, unless we want to mix different NIC
in a host, and want consistent hashing among all of them, ie
if they all use the boot time generated rss key, or if some application
is choosing specific tuple(s) so that incoming traffic lands into known
rx queue(s).

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_sp.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sp.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sp.c
index c9bd7f16018e..ff702a707a91 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sp.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sp.c
@@ -4319,8 +4319,16 @@ static int bnx2x_setup_rss(struct bnx2x *bp,
 
 	/* RSS keys */
 	if (test_bit(BNX2X_RSS_SET_SRCH, &p->rss_flags)) {
-		memcpy(&data->rss_key[0], &p->rss_key[0],
-		       sizeof(data->rss_key));
+		u8 *dst = (u8 *)(data->rss_key) + sizeof(data->rss_key);
+		const u8 *src = (const u8 *)p->rss_key;
+		int i;
+
+		/* Apparently, bnx2x reads this array in reverse order
+		 * We need to byte swap rss_key to comply with Toeplitz specs.
+		 */
+		for (i = 0; i < sizeof(data->rss_key); i++)
+			*--dst = *src++;
+
 		caps |= ETH_RSS_UPDATE_RAMROD_DATA_UPDATE_RSS_KEY;
 	}
 

Re: [PATCH net] bnx2x: byte swap rss_key to comply to Toeplitz specs

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 22 Sep 2015 17:04:58 -0700

> From: Eric Dumazet <edumazet@google.com>
> 
> After a good amount of debugging, I found bnx2x was byte swaping
> the 40 bytes of rss_key.
> 
> If we byte swap the key, then bnx2x generates hashes matching
> MSDN specs as documented in (Verifying the RSS Hash Calculation)
> 
> https://msdn.microsoft.com/en-us/library/windows/hardware/ff571021%
> 28v=vs.85%29.aspx
> 
> It is mostly a non issue, unless we want to mix different NIC
> in a host, and want consistent hashing among all of them, ie
> if they all use the boot time generated rss key, or if some application
> is choosing specific tuple(s) so that incoming traffic lands into known
> rx queue(s).
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied, thanks.