[Intel-wired-lan] [PATCH net-next v1 0/2]: ixgbe: Add the appropriate ethtool ops to query RSS indirection table and key

[Intel-wired-lan] [PATCH net-next v1 0/2]: ixgbe: Add the appropriate ethtool ops to query RSS indirection table and key

[Vlad Zolotarov]

Add the ethtool ops to VF driver to allow querying the RSS indirection table
and RSS Random Key.

This series refactors the RSS configuration code to store the current contents of
RSS indirection table and Hash Key in the "adapter" struct so that we won't need
to access registers every time this info is queried.

Vlad Zolotarov (2):
  ixgbe: Refactor the RSS configuration code.
  ixgbe: Add the appropriate ethtool ops to query RSS indirection table
    and key

 drivers/net/ethernet/intel/ixgbe/ixgbe.h         |   3 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c |  42 +++++++
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c    | 143 ++++++++++++++++-------
 drivers/net/ethernet/intel/ixgbe/ixgbe_type.h    |   7 ++
 4 files changed, 152 insertions(+), 43 deletions(-)

-- 
2.1.0

[PATCH net-next v1 0/2]: ixgbe: Add the appropriate ethtool ops to query RSS indirection table and key

[Vlad Zolotarov]

Add the ethtool ops to VF driver to allow querying the RSS indirection table
and RSS Random Key.

This series refactors the RSS configuration code to store the current contents of
RSS indirection table and Hash Key in the "adapter" struct so that we won't need
to access registers every time this info is queried.

Vlad Zolotarov (2):
  ixgbe: Refactor the RSS configuration code.
  ixgbe: Add the appropriate ethtool ops to query RSS indirection table
    and key

 drivers/net/ethernet/intel/ixgbe/ixgbe.h         |   3 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c |  42 +++++++
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c    | 143 ++++++++++++++++-------
 drivers/net/ethernet/intel/ixgbe/ixgbe_type.h    |   7 ++
 4 files changed, 152 insertions(+), 43 deletions(-)

-- 
2.1.0

[Intel-wired-lan] [PATCH net-next v1 1/2] ixgbe: Refactor the RSS configuration code.

[Vlad Zolotarov]

This patch is a preparation for enablement of ethtool RSS indirection table
and hash key querying. We don't want to read registers every time the RSS info
is queried. Therefore we will store its current content in the
arrays in the adapter struct and will read it from there (instead of from
registers) when requested.

Will change the code that writes the indirection table and hash key into the HW registers
to take its content from these arrays. This will also simplify the indirection
table updating ethtool callback implementation in the future.

Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe.h      |   2 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 143 ++++++++++++++++++--------
 drivers/net/ethernet/intel/ixgbe/ixgbe_type.h |   7 ++
 3 files changed, 109 insertions(+), 43 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index 5e44e48..402d182 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -766,6 +766,8 @@ struct ixgbe_adapter {
 
 	u8 default_up;
 	unsigned long fwd_bitmask; /* Bitmask indicating in use pools */
+	u8 rss_indir_tbl[IXGBE_MAX_RETA_ENTRIES];
+	u32 rss_key[IXGBE_RSS_KEY_SIZE / sizeof(u32)];
 };
 
 static inline u8 ixgbe_max_rss_indices(struct ixgbe_adapter *adapter)
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 8853d52..e3de0c9 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -3228,51 +3228,54 @@ static void ixgbe_configure_srrctl(struct ixgbe_adapter *adapter,
 	IXGBE_WRITE_REG(hw, IXGBE_SRRCTL(reg_idx), srrctl);
 }
 
-static void ixgbe_setup_reta(struct ixgbe_adapter *adapter, const u32 *seed)
+/**
+ * Return a number of entries in the RSS indirection table
+ *
+ * @adapter: device handle
+ *
+ *  - 82598/82599/X540:     128
+ *  - X550(non-SRIOV mode): 512
+ *  - X550(SRIOV mode):     64
+ */
+static u32 ixgbe_rss_indir_tbl_entries(struct ixgbe_adapter *adapter)
+{
+	if (adapter->hw.mac.type < ixgbe_mac_X550)
+		return 128;
+	else if (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED)
+		return 64;
+	else
+		return 512;
+}
+
+/**
+ * Write the RETA table to HW
+ *
+ * @adapter: device handle
+ *
+ * Write the RSS redirection table stored in adapter.rss_indir_tbl[] to HW.
+ */
+static void ixgbe_store_reta(struct ixgbe_adapter *adapter)
 {
+	u32 i, reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
 	struct ixgbe_hw *hw = &adapter->hw;
 	u32 reta = 0;
-	int i, j;
-	int reta_entries = 128;
-	u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
 	int indices_multi;
-
-	/*
-	 * Program table for at least 2 queues w/ SR-IOV so that VFs can
-	 * make full use of any rings they may have.  We will use the
-	 * PSRTYPE register to control how many rings we use within the PF.
-	 */
-	if ((adapter->flags & IXGBE_FLAG_SRIOV_ENABLED) && (rss_i < 2))
-		rss_i = 2;
-
-	/* Fill out hash function seeds */
-	for (i = 0; i < 10; i++)
-		IXGBE_WRITE_REG(hw, IXGBE_RSSRK(i), seed[i]);
+	u8 *indir_tbl = adapter->rss_indir_tbl;
 
 	/* Fill out the redirection table as follows:
-	 * 82598: 128 (8 bit wide) entries containing pair of 4 bit RSS indices
-	 * 82599/X540: 128 (8 bit wide) entries containing 4 bit RSS index
-	 * X550: 512 (8 bit wide) entries containing 6 bit RSS index
+	 *  - 82598:      8 bit wide entries containing pair of 4 bit RSS
+	 *    indices.
+	 *  - 82599/X540: 8 bit wide entries containing 4 bit RSS index
+	 *  - X550:       8 bit wide entries containing 6 bit RSS index
 	 */
 	if (adapter->hw.mac.type == ixgbe_mac_82598EB)
 		indices_multi = 0x11;
 	else
 		indices_multi = 0x1;
 
-	switch (adapter->hw.mac.type) {
-	case ixgbe_mac_X550:
-	case ixgbe_mac_X550EM_x:
-		if (!(adapter->flags & IXGBE_FLAG_SRIOV_ENABLED))
-			reta_entries = 512;
-	default:
-		break;
-	}
-
-	/* Fill out redirection table */
-	for (i = 0, j = 0; i < reta_entries; i++, j++) {
-		if (j == rss_i)
-			j = 0;
-		reta = (reta << 8) | (j * indices_multi);
+	/* Write redirection table to HW */
+	for (i = 0; i < reta_entries; i++) {
+		reta = (reta << 8) | (indices_multi * indir_tbl[i]);
 		if ((i & 3) == 3) {
 			if (i < 128)
 				IXGBE_WRITE_REG(hw, IXGBE_RETA(i >> 2), reta);
@@ -3283,34 +3286,88 @@ static void ixgbe_setup_reta(struct ixgbe_adapter *adapter, const u32 *seed)
 	}
 }
 
-static void ixgbe_setup_vfreta(struct ixgbe_adapter *adapter, const u32 *seed)
+/**
+ * Write the RETA table to HW (for x550 devices in SRIOV mode)
+ *
+ * @adapter: device handle
+ *
+ * Write the RSS redirection table stored in adapter.rss_indir_tbl[] to HW.
+ */
+static void ixgbe_store_vfreta(struct ixgbe_adapter *adapter)
 {
+	u32 i, reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
 	struct ixgbe_hw *hw = &adapter->hw;
 	u32 vfreta = 0;
+	unsigned int pf_pool = adapter->num_vfs;
+
+	/* Write redirection table to HW */
+	for (i = 0; i < reta_entries; i++) {
+		vfreta = (vfreta << 8) | adapter->rss_indir_tbl[i];
+		if ((i & 3) == 3)
+			IXGBE_WRITE_REG(hw, IXGBE_PFVFRETA(i >> 2, pf_pool),
+					vfreta);
+	}
+}
+
+static void ixgbe_setup_reta(struct ixgbe_adapter *adapter)
+{
+	struct ixgbe_hw *hw = &adapter->hw;
+	u32 i, j;
+	u32 reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
+	u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
+
+	/*
+	 * Program table for at least 2 queues w/ SR-IOV so that VFs can
+	 * make full use of any rings they may have.  We will use the
+	 * PSRTYPE register to control how many rings we use within the PF.
+	 */
+	if ((adapter->flags & IXGBE_FLAG_SRIOV_ENABLED) && (rss_i < 2))
+		rss_i = 2;
+
+	/* Fill out hash function seeds */
+	for (i = 0; i < 10; i++)
+		IXGBE_WRITE_REG(hw, IXGBE_RSSRK(i), adapter->rss_key[i]);
+
+	/* Fill out redirection table */
+	memset(adapter->rss_indir_tbl, 0, sizeof(adapter->rss_indir_tbl));
+
+	for (i = 0, j = 0; i < reta_entries; i++, j++) {
+		if (j == rss_i)
+			j = 0;
+
+		adapter->rss_indir_tbl[i] = j;
+	}
+
+	ixgbe_store_reta(adapter);
+}
+
+static void ixgbe_setup_vfreta(struct ixgbe_adapter *adapter)
+{
+	struct ixgbe_hw *hw = &adapter->hw;
 	u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
 	unsigned int pf_pool = adapter->num_vfs;
 	int i, j;
 
 	/* Fill out hash function seeds */
 	for (i = 0; i < 10; i++)
-		IXGBE_WRITE_REG(hw, IXGBE_PFVFRSSRK(i, pf_pool), seed[i]);
+		IXGBE_WRITE_REG(hw, IXGBE_PFVFRSSRK(i, pf_pool),
+				adapter->rss_key[i]);
 
 	/* Fill out the redirection table */
 	for (i = 0, j = 0; i < 64; i++, j++) {
 		if (j == rss_i)
 			j = 0;
-		vfreta = (vfreta << 8) | j;
-		if ((i & 3) == 3)
-			IXGBE_WRITE_REG(hw, IXGBE_PFVFRETA(i >> 2, pf_pool),
-					vfreta);
+
+		adapter->rss_indir_tbl[i] = j;
 	}
+
+	ixgbe_store_vfreta(adapter);
 }
 
 static void ixgbe_setup_mrqc(struct ixgbe_adapter *adapter)
 {
 	struct ixgbe_hw *hw = &adapter->hw;
 	u32 mrqc = 0, rss_field = 0, vfmrqc = 0;
-	u32 rss_key[10];
 	u32 rxcsum;
 
 	/* Disable indicating checksum in descriptor, enables RSS hash */
@@ -3354,7 +3411,7 @@ static void ixgbe_setup_mrqc(struct ixgbe_adapter *adapter)
 	if (adapter->flags2 & IXGBE_FLAG2_RSS_FIELD_IPV6_UDP)
 		rss_field |= IXGBE_MRQC_RSS_FIELD_IPV6_UDP;
 
-	netdev_rss_key_fill(rss_key, sizeof(rss_key));
+	netdev_rss_key_fill(adapter->rss_key, sizeof(adapter->rss_key));
 	if ((hw->mac.type >= ixgbe_mac_X550) &&
 	    (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED)) {
 		unsigned int pf_pool = adapter->num_vfs;
@@ -3364,12 +3421,12 @@ static void ixgbe_setup_mrqc(struct ixgbe_adapter *adapter)
 		IXGBE_WRITE_REG(hw, IXGBE_MRQC, mrqc);
 
 		/* Setup RSS through the VF registers */
-		ixgbe_setup_vfreta(adapter, rss_key);
+		ixgbe_setup_vfreta(adapter);
 		vfmrqc = IXGBE_MRQC_RSSEN;
 		vfmrqc |= rss_field;
 		IXGBE_WRITE_REG(hw, IXGBE_PFVFMRQC(pf_pool), vfmrqc);
 	} else {
-		ixgbe_setup_reta(adapter, rss_key);
+		ixgbe_setup_reta(adapter);
 		mrqc |= rss_field;
 		IXGBE_WRITE_REG(hw, IXGBE_MRQC, mrqc);
 	}
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
index c3ddc94..0c671b1 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
@@ -311,6 +311,13 @@ struct ixgbe_thermal_sensor_data {
 #define IXGBE_ERETA(_i)	(0x0EE80 + ((_i) * 4))  /* 96 of these (0-95) */
 #define IXGBE_RSSRK(_i) (0x05C80 + ((_i) * 4))  /* 10 of these (0-9) */
 
+/* maximum number of RETA entries among all devices supported by ixgbe
+ * driver: currently it's x550 device in non-SRIOV mode
+ */
+#define IXGBE_MAX_RETA_ENTRIES 512
+
+#define IXGBE_RSS_KEY_SIZE     40  /* size of RSS Hash Key in bytes */
+
 /* Registers for setting up RSS on X550 with SRIOV
  * _p - pool number (0..63)
  * _i - index (0..10 for PFVFRSSRK, 0..15 for PFVFRETA)
-- 
2.1.0

[PATCH net-next v1 1/2] ixgbe: Refactor the RSS configuration code.

[Vlad Zolotarov]

This patch is a preparation for enablement of ethtool RSS indirection table
and hash key querying. We don't want to read registers every time the RSS info
is queried. Therefore we will store its current content in the
arrays in the adapter struct and will read it from there (instead of from
registers) when requested.

Will change the code that writes the indirection table and hash key into the HW registers
to take its content from these arrays. This will also simplify the indirection
table updating ethtool callback implementation in the future.

Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe.h      |   2 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 143 ++++++++++++++++++--------
 drivers/net/ethernet/intel/ixgbe/ixgbe_type.h |   7 ++
 3 files changed, 109 insertions(+), 43 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index 5e44e48..402d182 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -766,6 +766,8 @@ struct ixgbe_adapter {
 
 	u8 default_up;
 	unsigned long fwd_bitmask; /* Bitmask indicating in use pools */
+	u8 rss_indir_tbl[IXGBE_MAX_RETA_ENTRIES];
+	u32 rss_key[IXGBE_RSS_KEY_SIZE / sizeof(u32)];
 };
 
 static inline u8 ixgbe_max_rss_indices(struct ixgbe_adapter *adapter)
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 8853d52..e3de0c9 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -3228,51 +3228,54 @@ static void ixgbe_configure_srrctl(struct ixgbe_adapter *adapter,
 	IXGBE_WRITE_REG(hw, IXGBE_SRRCTL(reg_idx), srrctl);
 }
 
-static void ixgbe_setup_reta(struct ixgbe_adapter *adapter, const u32 *seed)
+/**
+ * Return a number of entries in the RSS indirection table
+ *
+ * @adapter: device handle
+ *
+ *  - 82598/82599/X540:     128
+ *  - X550(non-SRIOV mode): 512
+ *  - X550(SRIOV mode):     64
+ */
+static u32 ixgbe_rss_indir_tbl_entries(struct ixgbe_adapter *adapter)
+{
+	if (adapter->hw.mac.type < ixgbe_mac_X550)
+		return 128;
+	else if (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED)
+		return 64;
+	else
+		return 512;
+}
+
+/**
+ * Write the RETA table to HW
+ *
+ * @adapter: device handle
+ *
+ * Write the RSS redirection table stored in adapter.rss_indir_tbl[] to HW.
+ */
+static void ixgbe_store_reta(struct ixgbe_adapter *adapter)
 {
+	u32 i, reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
 	struct ixgbe_hw *hw = &adapter->hw;
 	u32 reta = 0;
-	int i, j;
-	int reta_entries = 128;
-	u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
 	int indices_multi;
-
-	/*
-	 * Program table for at least 2 queues w/ SR-IOV so that VFs can
-	 * make full use of any rings they may have.  We will use the
-	 * PSRTYPE register to control how many rings we use within the PF.
-	 */
-	if ((adapter->flags & IXGBE_FLAG_SRIOV_ENABLED) && (rss_i < 2))
-		rss_i = 2;
-
-	/* Fill out hash function seeds */
-	for (i = 0; i < 10; i++)
-		IXGBE_WRITE_REG(hw, IXGBE_RSSRK(i), seed[i]);
+	u8 *indir_tbl = adapter->rss_indir_tbl;
 
 	/* Fill out the redirection table as follows:
-	 * 82598: 128 (8 bit wide) entries containing pair of 4 bit RSS indices
-	 * 82599/X540: 128 (8 bit wide) entries containing 4 bit RSS index
-	 * X550: 512 (8 bit wide) entries containing 6 bit RSS index
+	 *  - 82598:      8 bit wide entries containing pair of 4 bit RSS
+	 *    indices.
+	 *  - 82599/X540: 8 bit wide entries containing 4 bit RSS index
+	 *  - X550:       8 bit wide entries containing 6 bit RSS index
 	 */
 	if (adapter->hw.mac.type == ixgbe_mac_82598EB)
 		indices_multi = 0x11;
 	else
 		indices_multi = 0x1;
 
-	switch (adapter->hw.mac.type) {
-	case ixgbe_mac_X550:
-	case ixgbe_mac_X550EM_x:
-		if (!(adapter->flags & IXGBE_FLAG_SRIOV_ENABLED))
-			reta_entries = 512;
-	default:
-		break;
-	}
-
-	/* Fill out redirection table */
-	for (i = 0, j = 0; i < reta_entries; i++, j++) {
-		if (j == rss_i)
-			j = 0;
-		reta = (reta << 8) | (j * indices_multi);
+	/* Write redirection table to HW */
+	for (i = 0; i < reta_entries; i++) {
+		reta = (reta << 8) | (indices_multi * indir_tbl[i]);
 		if ((i & 3) == 3) {
 			if (i < 128)
 				IXGBE_WRITE_REG(hw, IXGBE_RETA(i >> 2), reta);
@@ -3283,34 +3286,88 @@ static void ixgbe_setup_reta(struct ixgbe_adapter *adapter, const u32 *seed)
 	}
 }
 
-static void ixgbe_setup_vfreta(struct ixgbe_adapter *adapter, const u32 *seed)
+/**
+ * Write the RETA table to HW (for x550 devices in SRIOV mode)
+ *
+ * @adapter: device handle
+ *
+ * Write the RSS redirection table stored in adapter.rss_indir_tbl[] to HW.
+ */
+static void ixgbe_store_vfreta(struct ixgbe_adapter *adapter)
 {
+	u32 i, reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
 	struct ixgbe_hw *hw = &adapter->hw;
 	u32 vfreta = 0;
+	unsigned int pf_pool = adapter->num_vfs;
+
+	/* Write redirection table to HW */
+	for (i = 0; i < reta_entries; i++) {
+		vfreta = (vfreta << 8) | adapter->rss_indir_tbl[i];
+		if ((i & 3) == 3)
+			IXGBE_WRITE_REG(hw, IXGBE_PFVFRETA(i >> 2, pf_pool),
+					vfreta);
+	}
+}
+
+static void ixgbe_setup_reta(struct ixgbe_adapter *adapter)
+{
+	struct ixgbe_hw *hw = &adapter->hw;
+	u32 i, j;
+	u32 reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
+	u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
+
+	/*
+	 * Program table for at least 2 queues w/ SR-IOV so that VFs can
+	 * make full use of any rings they may have.  We will use the
+	 * PSRTYPE register to control how many rings we use within the PF.
+	 */
+	if ((adapter->flags & IXGBE_FLAG_SRIOV_ENABLED) && (rss_i < 2))
+		rss_i = 2;
+
+	/* Fill out hash function seeds */
+	for (i = 0; i < 10; i++)
+		IXGBE_WRITE_REG(hw, IXGBE_RSSRK(i), adapter->rss_key[i]);
+
+	/* Fill out redirection table */
+	memset(adapter->rss_indir_tbl, 0, sizeof(adapter->rss_indir_tbl));
+
+	for (i = 0, j = 0; i < reta_entries; i++, j++) {
+		if (j == rss_i)
+			j = 0;
+
+		adapter->rss_indir_tbl[i] = j;
+	}
+
+	ixgbe_store_reta(adapter);
+}
+
+static void ixgbe_setup_vfreta(struct ixgbe_adapter *adapter)
+{
+	struct ixgbe_hw *hw = &adapter->hw;
 	u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
 	unsigned int pf_pool = adapter->num_vfs;
 	int i, j;
 
 	/* Fill out hash function seeds */
 	for (i = 0; i < 10; i++)
-		IXGBE_WRITE_REG(hw, IXGBE_PFVFRSSRK(i, pf_pool), seed[i]);
+		IXGBE_WRITE_REG(hw, IXGBE_PFVFRSSRK(i, pf_pool),
+				adapter->rss_key[i]);
 
 	/* Fill out the redirection table */
 	for (i = 0, j = 0; i < 64; i++, j++) {
 		if (j == rss_i)
 			j = 0;
-		vfreta = (vfreta << 8) | j;
-		if ((i & 3) == 3)
-			IXGBE_WRITE_REG(hw, IXGBE_PFVFRETA(i >> 2, pf_pool),
-					vfreta);
+
+		adapter->rss_indir_tbl[i] = j;
 	}
+
+	ixgbe_store_vfreta(adapter);
 }
 
 static void ixgbe_setup_mrqc(struct ixgbe_adapter *adapter)
 {
 	struct ixgbe_hw *hw = &adapter->hw;
 	u32 mrqc = 0, rss_field = 0, vfmrqc = 0;
-	u32 rss_key[10];
 	u32 rxcsum;
 
 	/* Disable indicating checksum in descriptor, enables RSS hash */
@@ -3354,7 +3411,7 @@ static void ixgbe_setup_mrqc(struct ixgbe_adapter *adapter)
 	if (adapter->flags2 & IXGBE_FLAG2_RSS_FIELD_IPV6_UDP)
 		rss_field |= IXGBE_MRQC_RSS_FIELD_IPV6_UDP;
 
-	netdev_rss_key_fill(rss_key, sizeof(rss_key));
+	netdev_rss_key_fill(adapter->rss_key, sizeof(adapter->rss_key));
 	if ((hw->mac.type >= ixgbe_mac_X550) &&
 	    (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED)) {
 		unsigned int pf_pool = adapter->num_vfs;
@@ -3364,12 +3421,12 @@ static void ixgbe_setup_mrqc(struct ixgbe_adapter *adapter)
 		IXGBE_WRITE_REG(hw, IXGBE_MRQC, mrqc);
 
 		/* Setup RSS through the VF registers */
-		ixgbe_setup_vfreta(adapter, rss_key);
+		ixgbe_setup_vfreta(adapter);
 		vfmrqc = IXGBE_MRQC_RSSEN;
 		vfmrqc |= rss_field;
 		IXGBE_WRITE_REG(hw, IXGBE_PFVFMRQC(pf_pool), vfmrqc);
 	} else {
-		ixgbe_setup_reta(adapter, rss_key);
+		ixgbe_setup_reta(adapter);
 		mrqc |= rss_field;
 		IXGBE_WRITE_REG(hw, IXGBE_MRQC, mrqc);
 	}
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
index c3ddc94..0c671b1 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
@@ -311,6 +311,13 @@ struct ixgbe_thermal_sensor_data {
 #define IXGBE_ERETA(_i)	(0x0EE80 + ((_i) * 4))  /* 96 of these (0-95) */
 #define IXGBE_RSSRK(_i) (0x05C80 + ((_i) * 4))  /* 10 of these (0-9) */
 
+/* maximum number of RETA entries among all devices supported by ixgbe
+ * driver: currently it's x550 device in non-SRIOV mode
+ */
+#define IXGBE_MAX_RETA_ENTRIES 512
+
+#define IXGBE_RSS_KEY_SIZE     40  /* size of RSS Hash Key in bytes */
+
 /* Registers for setting up RSS on X550 with SRIOV
  * _p - pool number (0..63)
  * _i - index (0..10 for PFVFRSSRK, 0..15 for PFVFRETA)
-- 
2.1.0

[Intel-wired-lan] [PATCH net-next v1 1/2] ixgbe: Refactor the RSS configuration code.

[Tantilov, Emil S]

>-----Original Message-----
>From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On Behalf Of Vlad Zolotarov
>Sent: Thursday, March 26, 2015 10:57 AM
>Subject: [Intel-wired-lan] [PATCH net-next v1 1/2] ixgbe: Refactor the RSS configuration code.
>
>This patch is a preparation for enablement of ethtool RSS indirection table
>and hash key querying. We don't want to read registers every time the RSS info
>is queried. Therefore we will store its current content in the
>arrays in the adapter struct and will read it from there (instead of from
>registers) when requested.
>
>Will change the code that writes the indirection table and hash key into the HW registers
>to take its content from these arrays. This will also simplify the indirection
>table updating ethtool callback implementation in the future.
>
>Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>---
> drivers/net/ethernet/intel/ixgbe/ixgbe.h      |   2 +
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 143 ++++++++++++++++++--------
> drivers/net/ethernet/intel/ixgbe/ixgbe_type.h |   7 ++
> 3 files changed, 109 insertions(+), 43 deletions(-)
>
>diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>index 5e44e48..402d182 100644
>--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>@@ -766,6 +766,8 @@ struct ixgbe_adapter {
> 
> 	u8 default_up;
> 	unsigned long fwd_bitmask; /* Bitmask indicating in use pools */
>+	u8 rss_indir_tbl[IXGBE_MAX_RETA_ENTRIES];
>+	u32 rss_key[IXGBE_RSS_KEY_SIZE / sizeof(u32)];
> };
> 
> static inline u8 ixgbe_max_rss_indices(struct ixgbe_adapter *adapter)
>diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>index 8853d52..e3de0c9 100644
>--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>@@ -3228,51 +3228,54 @@ static void ixgbe_configure_srrctl(struct ixgbe_adapter *adapter,
> 	IXGBE_WRITE_REG(hw, IXGBE_SRRCTL(reg_idx), srrctl);
> }
> 
>-static void ixgbe_setup_reta(struct ixgbe_adapter *adapter, const u32 *seed)
>+/**
>+ * Return a number of entries in the RSS indirection table

The preferred style for multiline comments is to not start with an empty "/*" checkpatch would generally 
catch those.

>+ *
>+ * @adapter: device handle
>+ *
>+ *  - 82598/82599/X540:     128
>+ *  - X550(non-SRIOV mode): 512
>+ *  - X550(SRIOV mode):     64
>+ */
>+static u32 ixgbe_rss_indir_tbl_entries(struct ixgbe_adapter *adapter)
>+{
>+	if (adapter->hw.mac.type < ixgbe_mac_X550)
>+		return 128;
>+	else if (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED)
>+		return 64;
>+	else
>+		return 512;
>+}
>+
>+/**
>+ * Write the RETA table to HW
>+ *
>+ * @adapter: device handle
>+ *
>+ * Write the RSS redirection table stored in adapter.rss_indir_tbl[] to HW.
>+ */
>+static void ixgbe_store_reta(struct ixgbe_adapter *adapter)

What is the point of having separate functions for writing the registers? Unless you intend to use
them in other parts of the code, those can be folded inside ixgbe_setupvf/reta().

> {
>+	u32 i, reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
> 	struct ixgbe_hw *hw = &adapter->hw;
> 	u32 reta = 0;
>-	int i, j;
>-	int reta_entries = 128;
>-	u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
> 	int indices_multi;
>-
>-	/*
>-	 * Program table for at least 2 queues w/ SR-IOV so that VFs can
>-	 * make full use of any rings they may have.  We will use the
>-	 * PSRTYPE register to control how many rings we use within the PF.
>-	 */
>-	if ((adapter->flags & IXGBE_FLAG_SRIOV_ENABLED) && (rss_i < 2))
>-		rss_i = 2;
>-
>-	/* Fill out hash function seeds */
>-	for (i = 0; i < 10; i++)
>-		IXGBE_WRITE_REG(hw, IXGBE_RSSRK(i), seed[i]);
>+	u8 *indir_tbl = adapter->rss_indir_tbl;
> 
> 	/* Fill out the redirection table as follows:
>-	 * 82598: 128 (8 bit wide) entries containing pair of 4 bit RSS indices
>-	 * 82599/X540: 128 (8 bit wide) entries containing 4 bit RSS index
>-	 * X550: 512 (8 bit wide) entries containing 6 bit RSS index
>+	 *  - 82598:      8 bit wide entries containing pair of 4 bit RSS
>+	 *    indices.
>+	 *  - 82599/X540: 8 bit wide entries containing 4 bit RSS index
>+	 *  - X550:       8 bit wide entries containing 6 bit RSS index
> 	 */
> 	if (adapter->hw.mac.type == ixgbe_mac_82598EB)
> 		indices_multi = 0x11;
> 	else
> 		indices_multi = 0x1;
> 
>-	switch (adapter->hw.mac.type) {
>-	case ixgbe_mac_X550:
>-	case ixgbe_mac_X550EM_x:
>-		if (!(adapter->flags & IXGBE_FLAG_SRIOV_ENABLED))
>-			reta_entries = 512;
>-	default:
>-		break;
>-	}
>-
>-	/* Fill out redirection table */
>-	for (i = 0, j = 0; i < reta_entries; i++, j++) {
>-		if (j == rss_i)
>-			j = 0;
>-		reta = (reta << 8) | (j * indices_multi);
>+	/* Write redirection table to HW */
>+	for (i = 0; i < reta_entries; i++) {
>+		reta = (reta << 8) | (indices_multi * indir_tbl[i]);
> 		if ((i & 3) == 3) {
> 			if (i < 128)
> 				IXGBE_WRITE_REG(hw, IXGBE_RETA(i >> 2), reta);
>@@ -3283,34 +3286,88 @@ static void ixgbe_setup_reta(struct ixgbe_adapter *adapter, const u32 *seed)
> 	}
> }
> 
>-static void ixgbe_setup_vfreta(struct ixgbe_adapter *adapter, const u32 *seed)
>+/**
>+ * Write the RETA table to HW (for x550 devices in SRIOV mode)
>+ *
>+ * @adapter: device handle
>+ *
>+ * Write the RSS redirection table stored in adapter.rss_indir_tbl[] to HW.
>+ */
>+static void ixgbe_store_vfreta(struct ixgbe_adapter *adapter)
> {
>+	u32 i, reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
> 	struct ixgbe_hw *hw = &adapter->hw;
> 	u32 vfreta = 0;
>+	unsigned int pf_pool = adapter->num_vfs;
>+
>+	/* Write redirection table to HW */
>+	for (i = 0; i < reta_entries; i++) {
>+		vfreta = (vfreta << 8) | adapter->rss_indir_tbl[i];
>+		if ((i & 3) == 3)
>+			IXGBE_WRITE_REG(hw, IXGBE_PFVFRETA(i >> 2, pf_pool),
>+					vfreta);
>+	}
>+}
>+
>+static void ixgbe_setup_reta(struct ixgbe_adapter *adapter)
>+{
>+	struct ixgbe_hw *hw = &adapter->hw;
>+	u32 i, j;
>+	u32 reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
>+	u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
>+
>+	/*
>+	 * Program table for at least 2 queues w/ SR-IOV so that VFs can
>+	 * make full use of any rings they may have.  We will use the
>+	 * PSRTYPE register to control how many rings we use within the PF.
>+	 */
>+	if ((adapter->flags & IXGBE_FLAG_SRIOV_ENABLED) && (rss_i < 2))
>+		rss_i = 2;
>+
>+	/* Fill out hash function seeds */
>+	for (i = 0; i < 10; i++)
>+		IXGBE_WRITE_REG(hw, IXGBE_RSSRK(i), adapter->rss_key[i]);
>+
>+	/* Fill out redirection table */
>+	memset(adapter->rss_indir_tbl, 0, sizeof(adapter->rss_indir_tbl));
>+
>+	for (i = 0, j = 0; i < reta_entries; i++, j++) {
>+		if (j == rss_i)
>+			j = 0;
>+
>+		adapter->rss_indir_tbl[i] = j;
>+	}
>+
>+	ixgbe_store_reta(adapter);
>+}
>+
>+static void ixgbe_setup_vfreta(struct ixgbe_adapter *adapter)
>+{
>+	struct ixgbe_hw *hw = &adapter->hw;
> 	u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
> 	unsigned int pf_pool = adapter->num_vfs;
> 	int i, j;
> 
> 	/* Fill out hash function seeds */
> 	for (i = 0; i < 10; i++)
>-		IXGBE_WRITE_REG(hw, IXGBE_PFVFRSSRK(i, pf_pool), seed[i]);
>+		IXGBE_WRITE_REG(hw, IXGBE_PFVFRSSRK(i, pf_pool),
>+				adapter->rss_key[i]);
> 
> 	/* Fill out the redirection table */
> 	for (i = 0, j = 0; i < 64; i++, j++) {
> 		if (j == rss_i)
> 			j = 0;
>-		vfreta = (vfreta << 8) | j;
>-		if ((i & 3) == 3)
>-			IXGBE_WRITE_REG(hw, IXGBE_PFVFRETA(i >> 2, pf_pool),
>-					vfreta);
>+
>+		adapter->rss_indir_tbl[i] = j;
> 	}
>+
>+	ixgbe_store_vfreta(adapter);
> }
> 
> static void ixgbe_setup_mrqc(struct ixgbe_adapter *adapter)
> {
> 	struct ixgbe_hw *hw = &adapter->hw;
> 	u32 mrqc = 0, rss_field = 0, vfmrqc = 0;
>-	u32 rss_key[10];
> 	u32 rxcsum;
> 
> 	/* Disable indicating checksum in descriptor, enables RSS hash */
>@@ -3354,7 +3411,7 @@ static void ixgbe_setup_mrqc(struct ixgbe_adapter *adapter)
> 	if (adapter->flags2 & IXGBE_FLAG2_RSS_FIELD_IPV6_UDP)
> 		rss_field |= IXGBE_MRQC_RSS_FIELD_IPV6_UDP;
> 
>-	netdev_rss_key_fill(rss_key, sizeof(rss_key));
>+	netdev_rss_key_fill(adapter->rss_key, sizeof(adapter->rss_key));
> 	if ((hw->mac.type >= ixgbe_mac_X550) &&
> 	    (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED)) {
> 		unsigned int pf_pool = adapter->num_vfs;
>@@ -3364,12 +3421,12 @@ static void ixgbe_setup_mrqc(struct ixgbe_adapter *adapter)
> 		IXGBE_WRITE_REG(hw, IXGBE_MRQC, mrqc);
> 
> 		/* Setup RSS through the VF registers */
>-		ixgbe_setup_vfreta(adapter, rss_key);
>+		ixgbe_setup_vfreta(adapter);
> 		vfmrqc = IXGBE_MRQC_RSSEN;
> 		vfmrqc |= rss_field;
> 		IXGBE_WRITE_REG(hw, IXGBE_PFVFMRQC(pf_pool), vfmrqc);
> 	} else {
>-		ixgbe_setup_reta(adapter, rss_key);
>+		ixgbe_setup_reta(adapter);
> 		mrqc |= rss_field;
> 		IXGBE_WRITE_REG(hw, IXGBE_MRQC, mrqc);
> 	}
>diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
>index c3ddc94..0c671b1 100644
>--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
>+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
>@@ -311,6 +311,13 @@ struct ixgbe_thermal_sensor_data {
> #define IXGBE_ERETA(_i)	(0x0EE80 + ((_i) * 4))  /* 96 of these (0-95) */
> #define IXGBE_RSSRK(_i) (0x05C80 + ((_i) * 4))  /* 10 of these (0-9) */
> 
>+/* maximum number of RETA entries among all devices supported by ixgbe
>+ * driver: currently it's x550 device in non-SRIOV mode
>+ */
>+#define IXGBE_MAX_RETA_ENTRIES 512
>+
>+#define IXGBE_RSS_KEY_SIZE     40  /* size of RSS Hash Key in bytes */
>+

These defines should be in ixgbe.h

> /* Registers for setting up RSS on X550 with SRIOV
>  * _p - pool number (0..63)
>  * _i - index (0..10 for PFVFRSSRK, 0..15 for PFVFRETA)

Thanks,
Emil

-- 
2.1.0

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan at lists.osuosl.org
http://lists.osuosl.org/mailman/listinfo/intel-wired-lan

RE: [Intel-wired-lan] [PATCH net-next v1 1/2] ixgbe: Refactor the RSS configuration code.

[Tantilov, Emil S]

>-----Original Message-----
>From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On Behalf Of Vlad Zolotarov
>Sent: Thursday, March 26, 2015 10:57 AM
>Subject: [Intel-wired-lan] [PATCH net-next v1 1/2] ixgbe: Refactor the RSS configuration code.
>
>This patch is a preparation for enablement of ethtool RSS indirection table
>and hash key querying. We don't want to read registers every time the RSS info
>is queried. Therefore we will store its current content in the
>arrays in the adapter struct and will read it from there (instead of from
>registers) when requested.
>
>Will change the code that writes the indirection table and hash key into the HW registers
>to take its content from these arrays. This will also simplify the indirection
>table updating ethtool callback implementation in the future.
>
>Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>---
> drivers/net/ethernet/intel/ixgbe/ixgbe.h      |   2 +
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 143 ++++++++++++++++++--------
> drivers/net/ethernet/intel/ixgbe/ixgbe_type.h |   7 ++
> 3 files changed, 109 insertions(+), 43 deletions(-)
>
>diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>index 5e44e48..402d182 100644
>--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>@@ -766,6 +766,8 @@ struct ixgbe_adapter {
> 
> 	u8 default_up;
> 	unsigned long fwd_bitmask; /* Bitmask indicating in use pools */
>+	u8 rss_indir_tbl[IXGBE_MAX_RETA_ENTRIES];
>+	u32 rss_key[IXGBE_RSS_KEY_SIZE / sizeof(u32)];
> };
> 
> static inline u8 ixgbe_max_rss_indices(struct ixgbe_adapter *adapter)
>diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>index 8853d52..e3de0c9 100644
>--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>@@ -3228,51 +3228,54 @@ static void ixgbe_configure_srrctl(struct ixgbe_adapter *adapter,
> 	IXGBE_WRITE_REG(hw, IXGBE_SRRCTL(reg_idx), srrctl);
> }
> 
>-static void ixgbe_setup_reta(struct ixgbe_adapter *adapter, const u32 *seed)
>+/**
>+ * Return a number of entries in the RSS indirection table

The preferred style for multiline comments is to not start with an empty "/*" checkpatch would generally 
catch those.

>+ *
>+ * @adapter: device handle
>+ *
>+ *  - 82598/82599/X540:     128
>+ *  - X550(non-SRIOV mode): 512
>+ *  - X550(SRIOV mode):     64
>+ */
>+static u32 ixgbe_rss_indir_tbl_entries(struct ixgbe_adapter *adapter)
>+{
>+	if (adapter->hw.mac.type < ixgbe_mac_X550)
>+		return 128;
>+	else if (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED)
>+		return 64;
>+	else
>+		return 512;
>+}
>+
>+/**
>+ * Write the RETA table to HW
>+ *
>+ * @adapter: device handle
>+ *
>+ * Write the RSS redirection table stored in adapter.rss_indir_tbl[] to HW.
>+ */
>+static void ixgbe_store_reta(struct ixgbe_adapter *adapter)

What is the point of having separate functions for writing the registers? Unless you intend to use
them in other parts of the code, those can be folded inside ixgbe_setupvf/reta().

> {
>+	u32 i, reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
> 	struct ixgbe_hw *hw = &adapter->hw;
> 	u32 reta = 0;
>-	int i, j;
>-	int reta_entries = 128;
>-	u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
> 	int indices_multi;
>-
>-	/*
>-	 * Program table for at least 2 queues w/ SR-IOV so that VFs can
>-	 * make full use of any rings they may have.  We will use the
>-	 * PSRTYPE register to control how many rings we use within the PF.
>-	 */
>-	if ((adapter->flags & IXGBE_FLAG_SRIOV_ENABLED) && (rss_i < 2))
>-		rss_i = 2;
>-
>-	/* Fill out hash function seeds */
>-	for (i = 0; i < 10; i++)
>-		IXGBE_WRITE_REG(hw, IXGBE_RSSRK(i), seed[i]);
>+	u8 *indir_tbl = adapter->rss_indir_tbl;
> 
> 	/* Fill out the redirection table as follows:
>-	 * 82598: 128 (8 bit wide) entries containing pair of 4 bit RSS indices
>-	 * 82599/X540: 128 (8 bit wide) entries containing 4 bit RSS index
>-	 * X550: 512 (8 bit wide) entries containing 6 bit RSS index
>+	 *  - 82598:      8 bit wide entries containing pair of 4 bit RSS
>+	 *    indices.
>+	 *  - 82599/X540: 8 bit wide entries containing 4 bit RSS index
>+	 *  - X550:       8 bit wide entries containing 6 bit RSS index
> 	 */
> 	if (adapter->hw.mac.type == ixgbe_mac_82598EB)
> 		indices_multi = 0x11;
> 	else
> 		indices_multi = 0x1;
> 
>-	switch (adapter->hw.mac.type) {
>-	case ixgbe_mac_X550:
>-	case ixgbe_mac_X550EM_x:
>-		if (!(adapter->flags & IXGBE_FLAG_SRIOV_ENABLED))
>-			reta_entries = 512;
>-	default:
>-		break;
>-	}
>-
>-	/* Fill out redirection table */
>-	for (i = 0, j = 0; i < reta_entries; i++, j++) {
>-		if (j == rss_i)
>-			j = 0;
>-		reta = (reta << 8) | (j * indices_multi);
>+	/* Write redirection table to HW */
>+	for (i = 0; i < reta_entries; i++) {
>+		reta = (reta << 8) | (indices_multi * indir_tbl[i]);
> 		if ((i & 3) == 3) {
> 			if (i < 128)
> 				IXGBE_WRITE_REG(hw, IXGBE_RETA(i >> 2), reta);
>@@ -3283,34 +3286,88 @@ static void ixgbe_setup_reta(struct ixgbe_adapter *adapter, const u32 *seed)
> 	}
> }
> 
>-static void ixgbe_setup_vfreta(struct ixgbe_adapter *adapter, const u32 *seed)
>+/**
>+ * Write the RETA table to HW (for x550 devices in SRIOV mode)
>+ *
>+ * @adapter: device handle
>+ *
>+ * Write the RSS redirection table stored in adapter.rss_indir_tbl[] to HW.
>+ */
>+static void ixgbe_store_vfreta(struct ixgbe_adapter *adapter)
> {
>+	u32 i, reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
> 	struct ixgbe_hw *hw = &adapter->hw;
> 	u32 vfreta = 0;
>+	unsigned int pf_pool = adapter->num_vfs;
>+
>+	/* Write redirection table to HW */
>+	for (i = 0; i < reta_entries; i++) {
>+		vfreta = (vfreta << 8) | adapter->rss_indir_tbl[i];
>+		if ((i & 3) == 3)
>+			IXGBE_WRITE_REG(hw, IXGBE_PFVFRETA(i >> 2, pf_pool),
>+					vfreta);
>+	}
>+}
>+
>+static void ixgbe_setup_reta(struct ixgbe_adapter *adapter)
>+{
>+	struct ixgbe_hw *hw = &adapter->hw;
>+	u32 i, j;
>+	u32 reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
>+	u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
>+
>+	/*
>+	 * Program table for at least 2 queues w/ SR-IOV so that VFs can
>+	 * make full use of any rings they may have.  We will use the
>+	 * PSRTYPE register to control how many rings we use within the PF.
>+	 */
>+	if ((adapter->flags & IXGBE_FLAG_SRIOV_ENABLED) && (rss_i < 2))
>+		rss_i = 2;
>+
>+	/* Fill out hash function seeds */
>+	for (i = 0; i < 10; i++)
>+		IXGBE_WRITE_REG(hw, IXGBE_RSSRK(i), adapter->rss_key[i]);
>+
>+	/* Fill out redirection table */
>+	memset(adapter->rss_indir_tbl, 0, sizeof(adapter->rss_indir_tbl));
>+
>+	for (i = 0, j = 0; i < reta_entries; i++, j++) {
>+		if (j == rss_i)
>+			j = 0;
>+
>+		adapter->rss_indir_tbl[i] = j;
>+	}
>+
>+	ixgbe_store_reta(adapter);
>+}
>+
>+static void ixgbe_setup_vfreta(struct ixgbe_adapter *adapter)
>+{
>+	struct ixgbe_hw *hw = &adapter->hw;
> 	u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
> 	unsigned int pf_pool = adapter->num_vfs;
> 	int i, j;
> 
> 	/* Fill out hash function seeds */
> 	for (i = 0; i < 10; i++)
>-		IXGBE_WRITE_REG(hw, IXGBE_PFVFRSSRK(i, pf_pool), seed[i]);
>+		IXGBE_WRITE_REG(hw, IXGBE_PFVFRSSRK(i, pf_pool),
>+				adapter->rss_key[i]);
> 
> 	/* Fill out the redirection table */
> 	for (i = 0, j = 0; i < 64; i++, j++) {
> 		if (j == rss_i)
> 			j = 0;
>-		vfreta = (vfreta << 8) | j;
>-		if ((i & 3) == 3)
>-			IXGBE_WRITE_REG(hw, IXGBE_PFVFRETA(i >> 2, pf_pool),
>-					vfreta);
>+
>+		adapter->rss_indir_tbl[i] = j;
> 	}
>+
>+	ixgbe_store_vfreta(adapter);
> }
> 
> static void ixgbe_setup_mrqc(struct ixgbe_adapter *adapter)
> {
> 	struct ixgbe_hw *hw = &adapter->hw;
> 	u32 mrqc = 0, rss_field = 0, vfmrqc = 0;
>-	u32 rss_key[10];
> 	u32 rxcsum;
> 
> 	/* Disable indicating checksum in descriptor, enables RSS hash */
>@@ -3354,7 +3411,7 @@ static void ixgbe_setup_mrqc(struct ixgbe_adapter *adapter)
> 	if (adapter->flags2 & IXGBE_FLAG2_RSS_FIELD_IPV6_UDP)
> 		rss_field |= IXGBE_MRQC_RSS_FIELD_IPV6_UDP;
> 
>-	netdev_rss_key_fill(rss_key, sizeof(rss_key));
>+	netdev_rss_key_fill(adapter->rss_key, sizeof(adapter->rss_key));
> 	if ((hw->mac.type >= ixgbe_mac_X550) &&
> 	    (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED)) {
> 		unsigned int pf_pool = adapter->num_vfs;
>@@ -3364,12 +3421,12 @@ static void ixgbe_setup_mrqc(struct ixgbe_adapter *adapter)
> 		IXGBE_WRITE_REG(hw, IXGBE_MRQC, mrqc);
> 
> 		/* Setup RSS through the VF registers */
>-		ixgbe_setup_vfreta(adapter, rss_key);
>+		ixgbe_setup_vfreta(adapter);
> 		vfmrqc = IXGBE_MRQC_RSSEN;
> 		vfmrqc |= rss_field;
> 		IXGBE_WRITE_REG(hw, IXGBE_PFVFMRQC(pf_pool), vfmrqc);
> 	} else {
>-		ixgbe_setup_reta(adapter, rss_key);
>+		ixgbe_setup_reta(adapter);
> 		mrqc |= rss_field;
> 		IXGBE_WRITE_REG(hw, IXGBE_MRQC, mrqc);
> 	}
>diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
>index c3ddc94..0c671b1 100644
>--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
>+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
>@@ -311,6 +311,13 @@ struct ixgbe_thermal_sensor_data {
> #define IXGBE_ERETA(_i)	(0x0EE80 + ((_i) * 4))  /* 96 of these (0-95) */
> #define IXGBE_RSSRK(_i) (0x05C80 + ((_i) * 4))  /* 10 of these (0-9) */
> 
>+/* maximum number of RETA entries among all devices supported by ixgbe
>+ * driver: currently it's x550 device in non-SRIOV mode
>+ */
>+#define IXGBE_MAX_RETA_ENTRIES 512
>+
>+#define IXGBE_RSS_KEY_SIZE     40  /* size of RSS Hash Key in bytes */
>+

These defines should be in ixgbe.h

> /* Registers for setting up RSS on X550 with SRIOV
>  * _p - pool number (0..63)
>  * _i - index (0..10 for PFVFRSSRK, 0..15 for PFVFRETA)

Thanks,
Emil

-- 
2.1.0

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@lists.osuosl.org
http://lists.osuosl.org/mailman/listinfo/intel-wired-lan

[Intel-wired-lan] [PATCH net-next v1 1/2] ixgbe: Refactor the RSS configuration code.

[Vlad Zolotarov]

On 03/26/15 22:20, Tantilov, Emil S wrote:
>> -----Original Message-----
>> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On Behalf Of Vlad Zolotarov
>> Sent: Thursday, March 26, 2015 10:57 AM
>> Subject: [Intel-wired-lan] [PATCH net-next v1 1/2] ixgbe: Refactor the RSS configuration code.
>>
>> This patch is a preparation for enablement of ethtool RSS indirection table
>> and hash key querying. We don't want to read registers every time the RSS info
>> is queried. Therefore we will store its current content in the
>> arrays in the adapter struct and will read it from there (instead of from
>> registers) when requested.
>>
>> Will change the code that writes the indirection table and hash key into the HW registers
>> to take its content from these arrays. This will also simplify the indirection
>> table updating ethtool callback implementation in the future.
>>
>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>> ---
>> drivers/net/ethernet/intel/ixgbe/ixgbe.h      |   2 +
>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 143 ++++++++++++++++++--------
>> drivers/net/ethernet/intel/ixgbe/ixgbe_type.h |   7 ++
>> 3 files changed, 109 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> index 5e44e48..402d182 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> @@ -766,6 +766,8 @@ struct ixgbe_adapter {
>>
>> 	u8 default_up;
>> 	unsigned long fwd_bitmask; /* Bitmask indicating in use pools */
>> +	u8 rss_indir_tbl[IXGBE_MAX_RETA_ENTRIES];
>> +	u32 rss_key[IXGBE_RSS_KEY_SIZE / sizeof(u32)];
>> };
>>
>> static inline u8 ixgbe_max_rss_indices(struct ixgbe_adapter *adapter)
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> index 8853d52..e3de0c9 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> @@ -3228,51 +3228,54 @@ static void ixgbe_configure_srrctl(struct ixgbe_adapter *adapter,
>> 	IXGBE_WRITE_REG(hw, IXGBE_SRRCTL(reg_idx), srrctl);
>> }
>>
>> -static void ixgbe_setup_reta(struct ixgbe_adapter *adapter, const u32 *seed)
>> +/**
>> + * Return a number of entries in the RSS indirection table
> The preferred style for multiline comments is to not start with an empty "/*" checkpatch would generally
> catch those.

It's not a multi-line comment but a function description and its styling 
is perfectly fine.

>
>> + *
>> + * @adapter: device handle
>> + *
>> + *  - 82598/82599/X540:     128
>> + *  - X550(non-SRIOV mode): 512
>> + *  - X550(SRIOV mode):     64
>> + */
>> +static u32 ixgbe_rss_indir_tbl_entries(struct ixgbe_adapter *adapter)
>> +{
>> +	if (adapter->hw.mac.type < ixgbe_mac_X550)
>> +		return 128;
>> +	else if (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED)
>> +		return 64;
>> +	else
>> +		return 512;
>> +}
>> +
>> +/**
>> + * Write the RETA table to HW
>> + *
>> + * @adapter: device handle
>> + *
>> + * Write the RSS redirection table stored in adapter.rss_indir_tbl[] to HW.
>> + */
>> +static void ixgbe_store_reta(struct ixgbe_adapter *adapter)
> What is the point of having separate functions for writing the registers? Unless you intend to use
> them in other parts of the code, those can be folded inside ixgbe_setupvf/reta().

The one that is going to implement the ethtool -X is going to use it. ;)

>
>> {
>> +	u32 i, reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
>> 	struct ixgbe_hw *hw = &adapter->hw;
>> 	u32 reta = 0;
>> -	int i, j;
>> -	int reta_entries = 128;
>> -	u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
>> 	int indices_multi;
>> -
>> -	/*
>> -	 * Program table for at least 2 queues w/ SR-IOV so that VFs can
>> -	 * make full use of any rings they may have.  We will use the
>> -	 * PSRTYPE register to control how many rings we use within the PF.
>> -	 */
>> -	if ((adapter->flags & IXGBE_FLAG_SRIOV_ENABLED) && (rss_i < 2))
>> -		rss_i = 2;
>> -
>> -	/* Fill out hash function seeds */
>> -	for (i = 0; i < 10; i++)
>> -		IXGBE_WRITE_REG(hw, IXGBE_RSSRK(i), seed[i]);
>> +	u8 *indir_tbl = adapter->rss_indir_tbl;
>>
>> 	/* Fill out the redirection table as follows:
>> -	 * 82598: 128 (8 bit wide) entries containing pair of 4 bit RSS indices
>> -	 * 82599/X540: 128 (8 bit wide) entries containing 4 bit RSS index
>> -	 * X550: 512 (8 bit wide) entries containing 6 bit RSS index
>> +	 *  - 82598:      8 bit wide entries containing pair of 4 bit RSS
>> +	 *    indices.
>> +	 *  - 82599/X540: 8 bit wide entries containing 4 bit RSS index
>> +	 *  - X550:       8 bit wide entries containing 6 bit RSS index
>> 	 */
>> 	if (adapter->hw.mac.type == ixgbe_mac_82598EB)
>> 		indices_multi = 0x11;
>> 	else
>> 		indices_multi = 0x1;
>>
>> -	switch (adapter->hw.mac.type) {
>> -	case ixgbe_mac_X550:
>> -	case ixgbe_mac_X550EM_x:
>> -		if (!(adapter->flags & IXGBE_FLAG_SRIOV_ENABLED))
>> -			reta_entries = 512;
>> -	default:
>> -		break;
>> -	}
>> -
>> -	/* Fill out redirection table */
>> -	for (i = 0, j = 0; i < reta_entries; i++, j++) {
>> -		if (j == rss_i)
>> -			j = 0;
>> -		reta = (reta << 8) | (j * indices_multi);
>> +	/* Write redirection table to HW */
>> +	for (i = 0; i < reta_entries; i++) {
>> +		reta = (reta << 8) | (indices_multi * indir_tbl[i]);
>> 		if ((i & 3) == 3) {
>> 			if (i < 128)
>> 				IXGBE_WRITE_REG(hw, IXGBE_RETA(i >> 2), reta);
>> @@ -3283,34 +3286,88 @@ static void ixgbe_setup_reta(struct ixgbe_adapter *adapter, const u32 *seed)
>> 	}
>> }
>>
>> -static void ixgbe_setup_vfreta(struct ixgbe_adapter *adapter, const u32 *seed)
>> +/**
>> + * Write the RETA table to HW (for x550 devices in SRIOV mode)
>> + *
>> + * @adapter: device handle
>> + *
>> + * Write the RSS redirection table stored in adapter.rss_indir_tbl[] to HW.
>> + */
>> +static void ixgbe_store_vfreta(struct ixgbe_adapter *adapter)
>> {
>> +	u32 i, reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
>> 	struct ixgbe_hw *hw = &adapter->hw;
>> 	u32 vfreta = 0;
>> +	unsigned int pf_pool = adapter->num_vfs;
>> +
>> +	/* Write redirection table to HW */
>> +	for (i = 0; i < reta_entries; i++) {
>> +		vfreta = (vfreta << 8) | adapter->rss_indir_tbl[i];
>> +		if ((i & 3) == 3)
>> +			IXGBE_WRITE_REG(hw, IXGBE_PFVFRETA(i >> 2, pf_pool),
>> +					vfreta);
>> +	}
>> +}
>> +
>> +static void ixgbe_setup_reta(struct ixgbe_adapter *adapter)
>> +{
>> +	struct ixgbe_hw *hw = &adapter->hw;
>> +	u32 i, j;
>> +	u32 reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
>> +	u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
>> +
>> +	/*
>> +	 * Program table for at least 2 queues w/ SR-IOV so that VFs can
>> +	 * make full use of any rings they may have.  We will use the
>> +	 * PSRTYPE register to control how many rings we use within the PF.
>> +	 */
>> +	if ((adapter->flags & IXGBE_FLAG_SRIOV_ENABLED) && (rss_i < 2))
>> +		rss_i = 2;
>> +
>> +	/* Fill out hash function seeds */
>> +	for (i = 0; i < 10; i++)
>> +		IXGBE_WRITE_REG(hw, IXGBE_RSSRK(i), adapter->rss_key[i]);
>> +
>> +	/* Fill out redirection table */
>> +	memset(adapter->rss_indir_tbl, 0, sizeof(adapter->rss_indir_tbl));
>> +
>> +	for (i = 0, j = 0; i < reta_entries; i++, j++) {
>> +		if (j == rss_i)
>> +			j = 0;
>> +
>> +		adapter->rss_indir_tbl[i] = j;
>> +	}
>> +
>> +	ixgbe_store_reta(adapter);
>> +}
>> +
>> +static void ixgbe_setup_vfreta(struct ixgbe_adapter *adapter)
>> +{
>> +	struct ixgbe_hw *hw = &adapter->hw;
>> 	u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
>> 	unsigned int pf_pool = adapter->num_vfs;
>> 	int i, j;
>>
>> 	/* Fill out hash function seeds */
>> 	for (i = 0; i < 10; i++)
>> -		IXGBE_WRITE_REG(hw, IXGBE_PFVFRSSRK(i, pf_pool), seed[i]);
>> +		IXGBE_WRITE_REG(hw, IXGBE_PFVFRSSRK(i, pf_pool),
>> +				adapter->rss_key[i]);
>>
>> 	/* Fill out the redirection table */
>> 	for (i = 0, j = 0; i < 64; i++, j++) {
>> 		if (j == rss_i)
>> 			j = 0;
>> -		vfreta = (vfreta << 8) | j;
>> -		if ((i & 3) == 3)
>> -			IXGBE_WRITE_REG(hw, IXGBE_PFVFRETA(i >> 2, pf_pool),
>> -					vfreta);
>> +
>> +		adapter->rss_indir_tbl[i] = j;
>> 	}
>> +
>> +	ixgbe_store_vfreta(adapter);
>> }
>>
>> static void ixgbe_setup_mrqc(struct ixgbe_adapter *adapter)
>> {
>> 	struct ixgbe_hw *hw = &adapter->hw;
>> 	u32 mrqc = 0, rss_field = 0, vfmrqc = 0;
>> -	u32 rss_key[10];
>> 	u32 rxcsum;
>>
>> 	/* Disable indicating checksum in descriptor, enables RSS hash */
>> @@ -3354,7 +3411,7 @@ static void ixgbe_setup_mrqc(struct ixgbe_adapter *adapter)
>> 	if (adapter->flags2 & IXGBE_FLAG2_RSS_FIELD_IPV6_UDP)
>> 		rss_field |= IXGBE_MRQC_RSS_FIELD_IPV6_UDP;
>>
>> -	netdev_rss_key_fill(rss_key, sizeof(rss_key));
>> +	netdev_rss_key_fill(adapter->rss_key, sizeof(adapter->rss_key));
>> 	if ((hw->mac.type >= ixgbe_mac_X550) &&
>> 	    (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED)) {
>> 		unsigned int pf_pool = adapter->num_vfs;
>> @@ -3364,12 +3421,12 @@ static void ixgbe_setup_mrqc(struct ixgbe_adapter *adapter)
>> 		IXGBE_WRITE_REG(hw, IXGBE_MRQC, mrqc);
>>
>> 		/* Setup RSS through the VF registers */
>> -		ixgbe_setup_vfreta(adapter, rss_key);
>> +		ixgbe_setup_vfreta(adapter);
>> 		vfmrqc = IXGBE_MRQC_RSSEN;
>> 		vfmrqc |= rss_field;
>> 		IXGBE_WRITE_REG(hw, IXGBE_PFVFMRQC(pf_pool), vfmrqc);
>> 	} else {
>> -		ixgbe_setup_reta(adapter, rss_key);
>> +		ixgbe_setup_reta(adapter);
>> 		mrqc |= rss_field;
>> 		IXGBE_WRITE_REG(hw, IXGBE_MRQC, mrqc);
>> 	}
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
>> index c3ddc94..0c671b1 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
>> @@ -311,6 +311,13 @@ struct ixgbe_thermal_sensor_data {
>> #define IXGBE_ERETA(_i)	(0x0EE80 + ((_i) * 4))  /* 96 of these (0-95) */
>> #define IXGBE_RSSRK(_i) (0x05C80 + ((_i) * 4))  /* 10 of these (0-9) */
>>
>> +/* maximum number of RETA entries among all devices supported by ixgbe
>> + * driver: currently it's x550 device in non-SRIOV mode
>> + */
>> +#define IXGBE_MAX_RETA_ENTRIES 512
>> +
>> +#define IXGBE_RSS_KEY_SIZE     40  /* size of RSS Hash Key in bytes */
>> +
> These defines should be in ixgbe.h

Why? These are HW related macros and IMHO they belong to ixgbe_type.h 
with other HW related macros and not to ixgbe.h where SW related macros are.

>
>> /* Registers for setting up RSS on X550 with SRIOV
>>   * _p - pool number (0..63)
>>   * _i - index (0..10 for PFVFRSSRK, 0..15 for PFVFRETA)
> Thanks,
> Emil
>

Re: [Intel-wired-lan] [PATCH net-next v1 1/2] ixgbe: Refactor the RSS configuration code.

[Vlad Zolotarov]

On 03/26/15 22:20, Tantilov, Emil S wrote:
>> -----Original Message-----
>> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On Behalf Of Vlad Zolotarov
>> Sent: Thursday, March 26, 2015 10:57 AM
>> Subject: [Intel-wired-lan] [PATCH net-next v1 1/2] ixgbe: Refactor the RSS configuration code.
>>
>> This patch is a preparation for enablement of ethtool RSS indirection table
>> and hash key querying. We don't want to read registers every time the RSS info
>> is queried. Therefore we will store its current content in the
>> arrays in the adapter struct and will read it from there (instead of from
>> registers) when requested.
>>
>> Will change the code that writes the indirection table and hash key into the HW registers
>> to take its content from these arrays. This will also simplify the indirection
>> table updating ethtool callback implementation in the future.
>>
>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>> ---
>> drivers/net/ethernet/intel/ixgbe/ixgbe.h      |   2 +
>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 143 ++++++++++++++++++--------
>> drivers/net/ethernet/intel/ixgbe/ixgbe_type.h |   7 ++
>> 3 files changed, 109 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> index 5e44e48..402d182 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> @@ -766,6 +766,8 @@ struct ixgbe_adapter {
>>
>> 	u8 default_up;
>> 	unsigned long fwd_bitmask; /* Bitmask indicating in use pools */
>> +	u8 rss_indir_tbl[IXGBE_MAX_RETA_ENTRIES];
>> +	u32 rss_key[IXGBE_RSS_KEY_SIZE / sizeof(u32)];
>> };
>>
>> static inline u8 ixgbe_max_rss_indices(struct ixgbe_adapter *adapter)
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> index 8853d52..e3de0c9 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> @@ -3228,51 +3228,54 @@ static void ixgbe_configure_srrctl(struct ixgbe_adapter *adapter,
>> 	IXGBE_WRITE_REG(hw, IXGBE_SRRCTL(reg_idx), srrctl);
>> }
>>
>> -static void ixgbe_setup_reta(struct ixgbe_adapter *adapter, const u32 *seed)
>> +/**
>> + * Return a number of entries in the RSS indirection table
> The preferred style for multiline comments is to not start with an empty "/*" checkpatch would generally
> catch those.

It's not a multi-line comment but a function description and its styling 
is perfectly fine.

>
>> + *
>> + * @adapter: device handle
>> + *
>> + *  - 82598/82599/X540:     128
>> + *  - X550(non-SRIOV mode): 512
>> + *  - X550(SRIOV mode):     64
>> + */
>> +static u32 ixgbe_rss_indir_tbl_entries(struct ixgbe_adapter *adapter)
>> +{
>> +	if (adapter->hw.mac.type < ixgbe_mac_X550)
>> +		return 128;
>> +	else if (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED)
>> +		return 64;
>> +	else
>> +		return 512;
>> +}
>> +
>> +/**
>> + * Write the RETA table to HW
>> + *
>> + * @adapter: device handle
>> + *
>> + * Write the RSS redirection table stored in adapter.rss_indir_tbl[] to HW.
>> + */
>> +static void ixgbe_store_reta(struct ixgbe_adapter *adapter)
> What is the point of having separate functions for writing the registers? Unless you intend to use
> them in other parts of the code, those can be folded inside ixgbe_setupvf/reta().

The one that is going to implement the ethtool -X is going to use it. ;)

>
>> {
>> +	u32 i, reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
>> 	struct ixgbe_hw *hw = &adapter->hw;
>> 	u32 reta = 0;
>> -	int i, j;
>> -	int reta_entries = 128;
>> -	u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
>> 	int indices_multi;
>> -
>> -	/*
>> -	 * Program table for at least 2 queues w/ SR-IOV so that VFs can
>> -	 * make full use of any rings they may have.  We will use the
>> -	 * PSRTYPE register to control how many rings we use within the PF.
>> -	 */
>> -	if ((adapter->flags & IXGBE_FLAG_SRIOV_ENABLED) && (rss_i < 2))
>> -		rss_i = 2;
>> -
>> -	/* Fill out hash function seeds */
>> -	for (i = 0; i < 10; i++)
>> -		IXGBE_WRITE_REG(hw, IXGBE_RSSRK(i), seed[i]);
>> +	u8 *indir_tbl = adapter->rss_indir_tbl;
>>
>> 	/* Fill out the redirection table as follows:
>> -	 * 82598: 128 (8 bit wide) entries containing pair of 4 bit RSS indices
>> -	 * 82599/X540: 128 (8 bit wide) entries containing 4 bit RSS index
>> -	 * X550: 512 (8 bit wide) entries containing 6 bit RSS index
>> +	 *  - 82598:      8 bit wide entries containing pair of 4 bit RSS
>> +	 *    indices.
>> +	 *  - 82599/X540: 8 bit wide entries containing 4 bit RSS index
>> +	 *  - X550:       8 bit wide entries containing 6 bit RSS index
>> 	 */
>> 	if (adapter->hw.mac.type == ixgbe_mac_82598EB)
>> 		indices_multi = 0x11;
>> 	else
>> 		indices_multi = 0x1;
>>
>> -	switch (adapter->hw.mac.type) {
>> -	case ixgbe_mac_X550:
>> -	case ixgbe_mac_X550EM_x:
>> -		if (!(adapter->flags & IXGBE_FLAG_SRIOV_ENABLED))
>> -			reta_entries = 512;
>> -	default:
>> -		break;
>> -	}
>> -
>> -	/* Fill out redirection table */
>> -	for (i = 0, j = 0; i < reta_entries; i++, j++) {
>> -		if (j == rss_i)
>> -			j = 0;
>> -		reta = (reta << 8) | (j * indices_multi);
>> +	/* Write redirection table to HW */
>> +	for (i = 0; i < reta_entries; i++) {
>> +		reta = (reta << 8) | (indices_multi * indir_tbl[i]);
>> 		if ((i & 3) == 3) {
>> 			if (i < 128)
>> 				IXGBE_WRITE_REG(hw, IXGBE_RETA(i >> 2), reta);
>> @@ -3283,34 +3286,88 @@ static void ixgbe_setup_reta(struct ixgbe_adapter *adapter, const u32 *seed)
>> 	}
>> }
>>
>> -static void ixgbe_setup_vfreta(struct ixgbe_adapter *adapter, const u32 *seed)
>> +/**
>> + * Write the RETA table to HW (for x550 devices in SRIOV mode)
>> + *
>> + * @adapter: device handle
>> + *
>> + * Write the RSS redirection table stored in adapter.rss_indir_tbl[] to HW.
>> + */
>> +static void ixgbe_store_vfreta(struct ixgbe_adapter *adapter)
>> {
>> +	u32 i, reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
>> 	struct ixgbe_hw *hw = &adapter->hw;
>> 	u32 vfreta = 0;
>> +	unsigned int pf_pool = adapter->num_vfs;
>> +
>> +	/* Write redirection table to HW */
>> +	for (i = 0; i < reta_entries; i++) {
>> +		vfreta = (vfreta << 8) | adapter->rss_indir_tbl[i];
>> +		if ((i & 3) == 3)
>> +			IXGBE_WRITE_REG(hw, IXGBE_PFVFRETA(i >> 2, pf_pool),
>> +					vfreta);
>> +	}
>> +}
>> +
>> +static void ixgbe_setup_reta(struct ixgbe_adapter *adapter)
>> +{
>> +	struct ixgbe_hw *hw = &adapter->hw;
>> +	u32 i, j;
>> +	u32 reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
>> +	u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
>> +
>> +	/*
>> +	 * Program table for at least 2 queues w/ SR-IOV so that VFs can
>> +	 * make full use of any rings they may have.  We will use the
>> +	 * PSRTYPE register to control how many rings we use within the PF.
>> +	 */
>> +	if ((adapter->flags & IXGBE_FLAG_SRIOV_ENABLED) && (rss_i < 2))
>> +		rss_i = 2;
>> +
>> +	/* Fill out hash function seeds */
>> +	for (i = 0; i < 10; i++)
>> +		IXGBE_WRITE_REG(hw, IXGBE_RSSRK(i), adapter->rss_key[i]);
>> +
>> +	/* Fill out redirection table */
>> +	memset(adapter->rss_indir_tbl, 0, sizeof(adapter->rss_indir_tbl));
>> +
>> +	for (i = 0, j = 0; i < reta_entries; i++, j++) {
>> +		if (j == rss_i)
>> +			j = 0;
>> +
>> +		adapter->rss_indir_tbl[i] = j;
>> +	}
>> +
>> +	ixgbe_store_reta(adapter);
>> +}
>> +
>> +static void ixgbe_setup_vfreta(struct ixgbe_adapter *adapter)
>> +{
>> +	struct ixgbe_hw *hw = &adapter->hw;
>> 	u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
>> 	unsigned int pf_pool = adapter->num_vfs;
>> 	int i, j;
>>
>> 	/* Fill out hash function seeds */
>> 	for (i = 0; i < 10; i++)
>> -		IXGBE_WRITE_REG(hw, IXGBE_PFVFRSSRK(i, pf_pool), seed[i]);
>> +		IXGBE_WRITE_REG(hw, IXGBE_PFVFRSSRK(i, pf_pool),
>> +				adapter->rss_key[i]);
>>
>> 	/* Fill out the redirection table */
>> 	for (i = 0, j = 0; i < 64; i++, j++) {
>> 		if (j == rss_i)
>> 			j = 0;
>> -		vfreta = (vfreta << 8) | j;
>> -		if ((i & 3) == 3)
>> -			IXGBE_WRITE_REG(hw, IXGBE_PFVFRETA(i >> 2, pf_pool),
>> -					vfreta);
>> +
>> +		adapter->rss_indir_tbl[i] = j;
>> 	}
>> +
>> +	ixgbe_store_vfreta(adapter);
>> }
>>
>> static void ixgbe_setup_mrqc(struct ixgbe_adapter *adapter)
>> {
>> 	struct ixgbe_hw *hw = &adapter->hw;
>> 	u32 mrqc = 0, rss_field = 0, vfmrqc = 0;
>> -	u32 rss_key[10];
>> 	u32 rxcsum;
>>
>> 	/* Disable indicating checksum in descriptor, enables RSS hash */
>> @@ -3354,7 +3411,7 @@ static void ixgbe_setup_mrqc(struct ixgbe_adapter *adapter)
>> 	if (adapter->flags2 & IXGBE_FLAG2_RSS_FIELD_IPV6_UDP)
>> 		rss_field |= IXGBE_MRQC_RSS_FIELD_IPV6_UDP;
>>
>> -	netdev_rss_key_fill(rss_key, sizeof(rss_key));
>> +	netdev_rss_key_fill(adapter->rss_key, sizeof(adapter->rss_key));
>> 	if ((hw->mac.type >= ixgbe_mac_X550) &&
>> 	    (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED)) {
>> 		unsigned int pf_pool = adapter->num_vfs;
>> @@ -3364,12 +3421,12 @@ static void ixgbe_setup_mrqc(struct ixgbe_adapter *adapter)
>> 		IXGBE_WRITE_REG(hw, IXGBE_MRQC, mrqc);
>>
>> 		/* Setup RSS through the VF registers */
>> -		ixgbe_setup_vfreta(adapter, rss_key);
>> +		ixgbe_setup_vfreta(adapter);
>> 		vfmrqc = IXGBE_MRQC_RSSEN;
>> 		vfmrqc |= rss_field;
>> 		IXGBE_WRITE_REG(hw, IXGBE_PFVFMRQC(pf_pool), vfmrqc);
>> 	} else {
>> -		ixgbe_setup_reta(adapter, rss_key);
>> +		ixgbe_setup_reta(adapter);
>> 		mrqc |= rss_field;
>> 		IXGBE_WRITE_REG(hw, IXGBE_MRQC, mrqc);
>> 	}
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
>> index c3ddc94..0c671b1 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
>> @@ -311,6 +311,13 @@ struct ixgbe_thermal_sensor_data {
>> #define IXGBE_ERETA(_i)	(0x0EE80 + ((_i) * 4))  /* 96 of these (0-95) */
>> #define IXGBE_RSSRK(_i) (0x05C80 + ((_i) * 4))  /* 10 of these (0-9) */
>>
>> +/* maximum number of RETA entries among all devices supported by ixgbe
>> + * driver: currently it's x550 device in non-SRIOV mode
>> + */
>> +#define IXGBE_MAX_RETA_ENTRIES 512
>> +
>> +#define IXGBE_RSS_KEY_SIZE     40  /* size of RSS Hash Key in bytes */
>> +
> These defines should be in ixgbe.h

Why? These are HW related macros and IMHO they belong to ixgbe_type.h 
with other HW related macros and not to ixgbe.h where SW related macros are.

>
>> /* Registers for setting up RSS on X550 with SRIOV
>>   * _p - pool number (0..63)
>>   * _i - index (0..10 for PFVFRSSRK, 0..15 for PFVFRETA)
> Thanks,
> Emil
>

[Intel-wired-lan] [PATCH net-next v1 1/2] ixgbe: Refactor the RSS configuration code.

[Tantilov, Emil S]

>-----Original Message-----
>From: Vlad Zolotarov [mailto:vladz at cloudius-systems.com] 
>Sent: Thursday, March 26, 2015 1:55 PM
>To: Tantilov, Emil S; netdev at vger.kernel.org
>Cc: avi at cloudius-systems.com; intel-wired-lan at lists.osuosl.org; gleb at cloudius-systems.com
>Subject: Re: [Intel-wired-lan] [PATCH net-next v1 1/2] ixgbe: Refactor the RSS configuration code.
>
>
>
>On 03/26/15 22:20, Tantilov, Emil S wrote:
>>> -----Original Message-----
>>> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On Behalf Of Vlad Zolotarov
>>> Sent: Thursday, March 26, 2015 10:57 AM
>>> Subject: [Intel-wired-lan] [PATCH net-next v1 1/2] ixgbe: Refactor the RSS configuration code.
>>>
>>> This patch is a preparation for enablement of ethtool RSS indirection table
>>> and hash key querying. We don't want to read registers every time the RSS info
>>> is queried. Therefore we will store its current content in the
>>> arrays in the adapter struct and will read it from there (instead of from
>>> registers) when requested.
>>>
>>> Will change the code that writes the indirection table and hash key into the HW registers
>>> to take its content from these arrays. This will also simplify the indirection
>>> table updating ethtool callback implementation in the future.
>>>
>>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>>> ---
>>> drivers/net/ethernet/intel/ixgbe/ixgbe.h      |   2 +
>>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 143 ++++++++++++++++++--------
>>> drivers/net/ethernet/intel/ixgbe/ixgbe_type.h |   7 ++
>>> 3 files changed, 109 insertions(+), 43 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>> index 5e44e48..402d182 100644
>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>> @@ -766,6 +766,8 @@ struct ixgbe_adapter {
>>>
>>> 	u8 default_up;
>>> 	unsigned long fwd_bitmask; /* Bitmask indicating in use pools */
>>> +	u8 rss_indir_tbl[IXGBE_MAX_RETA_ENTRIES];
>>> +	u32 rss_key[IXGBE_RSS_KEY_SIZE / sizeof(u32)];
>>> };
>>>
>>> static inline u8 ixgbe_max_rss_indices(struct ixgbe_adapter *adapter)
>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> index 8853d52..e3de0c9 100644
>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> @@ -3228,51 +3228,54 @@ static void ixgbe_configure_srrctl(struct ixgbe_adapter *adapter,
>>> 	IXGBE_WRITE_REG(hw, IXGBE_SRRCTL(reg_idx), srrctl);
>>> }
>>>
>>> -static void ixgbe_setup_reta(struct ixgbe_adapter *adapter, const u32 *seed)
>>> +/**
>>> + * Return a number of entries in the RSS indirection table
>> The preferred style for multiline comments is to not start with an empty "/*" checkpatch would generally
>> catch those.
>
>It's not a multi-line comment but a function description and its styling 
>is perfectly fine.

Right - I replied under the wrong comment. There is however an old style comment that you moved that shows up in checkpatch.

>>> + *
>>> + * @adapter: device handle
>>> + *
>>> + *  - 82598/82599/X540:     128
>>> + *  - X550(non-SRIOV mode): 512
>>> + *  - X550(SRIOV mode):     64
>>> + */
>>> +static u32 ixgbe_rss_indir_tbl_entries(struct ixgbe_adapter *adapter)
>>> +{
>>> +	if (adapter->hw.mac.type < ixgbe_mac_X550)
>>> +		return 128;
>>> +	else if (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED)
>>> +		return 64;
>>> +	else
>>> +		return 512;
>>> +}
>>> +
>>> +/**
>>> + * Write the RETA table to HW
>>> + *
>>> + * @adapter: device handle
>>> + *
>>> + * Write the RSS redirection table stored in adapter.rss_indir_tbl[] to HW.
>>> + */
>>> +static void ixgbe_store_reta(struct ixgbe_adapter *adapter)
>> What is the point of having separate functions for writing the registers? Unless you intend to use
>> them in other parts of the code, those can be folded inside ixgbe_setupvf/reta().
>
>The one that is going to implement the ethtool -X is going to use it. ;)

Alright that makes sense.

>>> {
>>> +	u32 i, reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
>>> 	struct ixgbe_hw *hw = &adapter->hw;
>>> 	u32 reta = 0;
>>> -	int i, j;
>>> -	int reta_entries = 128;
>>> -	u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
>>> 	int indices_multi;
>>> -
>>> -	/*
>>> -	 * Program table for at least 2 queues w/ SR-IOV so that VFs can
>>> -	 * make full use of any rings they may have.  We will use the
>>> -	 * PSRTYPE register to control how many rings we use within the PF.
>>> -	 */
>>> -	if ((adapter->flags & IXGBE_FLAG_SRIOV_ENABLED) && (rss_i < 2)) >
>>> -		rss_i = 2;
>>> -
>>> -	/* Fill out hash function seeds */
>>> -	for (i = 0; i < 10; i++)
>>> -		IXGBE_WRITE_REG(hw, IXGBE_RSSRK(i), seed[i]);
>>> +	u8 *indir_tbl = adapter->rss_indir_tbl;
>>>
>>> 	/* Fill out the redirection table as follows:
>>> -	 * 82598: 128 (8 bit wide) entries containing pair of 4 bit RSS indices
>>> -	 * 82599/X540: 128 (8 bit wide) entries containing 4 bit RSS index
>>> -	 * X550: 512 (8 bit wide) entries containing 6 bit RSS index
>>> +	 *  - 82598:      8 bit wide entries containing pair of 4 bit RSS
>>> +	 *    indices.
>>> +	 *  - 82599/X540: 8 bit wide entries containing 4 bit RSS index
>>> +	 *  - X550:       8 bit wide entries containing 6 bit RSS index
>>> 	 */
>>> 	if (adapter->hw.mac.type == ixgbe_mac_82598EB)
>>> 		indices_multi = 0x11;
>>> 	else
>>> 		indices_multi = 0x1;
>>>
>>> -	switch (adapter->hw.mac.type) {
>>> -	case ixgbe_mac_X550:
>>> -	case ixgbe_mac_X550EM_x:
>>> -		if (!(adapter->flags & IXGBE_FLAG_SRIOV_ENABLED))
>>> -			reta_entries = 512;
>>> -	default:
>>> -		break;
>>> -	}
>>> -
>>> -	/* Fill out redirection table */
>>> -	for (i = 0, j = 0; i < reta_entries; i++, j++) {
>>> -		if (j == rss_i)
>>> -			j = 0;
>>> -		reta = (reta << 8) | (j * indices_multi);
>>> +	/* Write redirection table to HW */
>>> +	for (i = 0; i < reta_entries; i++) {
>>> +		reta = (reta << 8) | (indices_multi * indir_tbl[i]);
>>> 		if ((i & 3) == 3) {
>>> 			if (i < 128)
>>> 				IXGBE_WRITE_REG(hw, IXGBE_RETA(i >> 2), reta);
>>> @@ -3283,34 +3286,88 @@ static void ixgbe_setup_reta(struct ixgbe_adapter *adapter, const u32 *seed)
>>> 	}
>>> }
>>>
>>> -static void ixgbe_setup_vfreta(struct ixgbe_adapter *adapter, const u32 *seed)
>>> +/**
>>> + * Write the RETA table to HW (for x550 devices in SRIOV mode)
>>> + *
>>> + * @adapter: device handle
>>> + *
>>> + * Write the RSS redirection table stored in adapter.rss_indir_tbl[] to HW.
>>> + */
>>> +static void ixgbe_store_vfreta(struct ixgbe_adapter *adapter)
>>> {
>>> +	u32 i, reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
>>> 	struct ixgbe_hw *hw = &adapter->hw;
>>> 	u32 vfreta = 0;
>>> +	unsigned int pf_pool = adapter->num_vfs;
>>> +
>>> +	/* Write redirection table to HW */
>>> +	for (i = 0; i < reta_entries; i++) {
>>> +		vfreta = (vfreta << 8) | adapter->rss_indir_tbl[i];
>>> +		if ((i & 3) == 3)
>>> +			IXGBE_WRITE_REG(hw, IXGBE_PFVFRETA(i >> 2, pf_pool),
>>> +					vfreta);
>>> +	}
>>> +}
>>> +
>>> +static void ixgbe_setup_reta(struct ixgbe_adapter *adapter)
>>> +{
>>> +	struct ixgbe_hw *hw = &adapter->hw;
>>> +	u32 i, j;
>>> +	u32 reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
>>> +	u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
>>> +
>>> +	/*
>>> +	 * Program table for at least 2 queues w/ SR-IOV so that VFs can
>>> +	 * make full use of any rings they may have.  We will use the
>>> +	 * PSRTYPE register to control how many rings we use within the PF.
>>> +	 */

This one.

>>> +	if ((adapter->flags & IXGBE_FLAG_SRIOV_ENABLED) && (rss_i < 2))
>>> +		rss_i = 2;
>>> +
>>> +	/* Fill out hash function seeds */
>>> +	for (i = 0; i < 10; i++)
>>> +		IXGBE_WRITE_REG(hw, IXGBE_RSSRK(i), adapter->rss_key[i]);
>>> +
>>> +	/* Fill out redirection table */
>>> +	memset(adapter->rss_indir_tbl, 0, sizeof(adapter->rss_indir_tbl));
>>> +
>>> +	for (i = 0, j = 0; i < reta_entries; i++, j++) {
>>> +		if (j == rss_i)
>>> +			j = 0;
>>> +
>>> +		adapter->rss_indir_tbl[i] = j;
>>> +	}
>>> +
>>> +	ixgbe_store_reta(adapter);
>>> +}
>>> +
>>> +static void ixgbe_setup_vfreta(struct ixgbe_adapter *adapter)
>>> +{
>>> +	struct ixgbe_hw *hw = &adapter->hw;
>>> 	u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
>>> 	unsigned int pf_pool = adapter->num_vfs;
>>> 	int i, j;
>>>
>>> 	/* Fill out hash function seeds */
>>> 	for (i = 0; i < 10; i++)
>>> -		IXGBE_WRITE_REG(hw, IXGBE_PFVFRSSRK(i, pf_pool), seed[i]);
>>> +		IXGBE_WRITE_REG(hw, IXGBE_PFVFRSSRK(i, pf_pool),
>>> +				adapter->rss_key[i]);
>>>
>>> 	/* Fill out the redirection table */
>>> 	for (i = 0, j = 0; i < 64; i++, j++) {
>>> 		if (j == rss_i)
>>> 			j = 0;
>>> -		vfreta = (vfreta << 8) | j;
>>> -		if ((i & 3) == 3)
>>> -			IXGBE_WRITE_REG(hw, IXGBE_PFVFRETA(i >> 2, pf_pool),
>>> -					vfreta);
>>> +
>>> +		adapter->rss_indir_tbl[i] = j;
>>> 	}
>>> +
>>> +	ixgbe_store_vfreta(adapter);
>>> }
>>>
>>> static void ixgbe_setup_mrqc(struct ixgbe_adapter *adapter)
>>> {
>>> 	struct ixgbe_hw *hw = &adapter->hw;
>>> 	u32 mrqc = 0, rss_field = 0, vfmrqc = 0;
>>> -	u32 rss_key[10];
>>> 	u32 rxcsum;
>>>
>>> 	/* Disable indicating checksum in descriptor, enables RSS hash */
>>> @@ -3354,7 +3411,7 @@ static void ixgbe_setup_mrqc(struct ixgbe_adapter *adapter)
>>> 	if (adapter->flags2 & IXGBE_FLAG2_RSS_FIELD_IPV6_UDP)
>>> 		rss_field |= IXGBE_MRQC_RSS_FIELD_IPV6_UDP;
>>>
>>> -	netdev_rss_key_fill(rss_key, sizeof(rss_key));
>>> +	netdev_rss_key_fill(adapter->rss_key, sizeof(adapter->rss_key));
>>> 	if ((hw->mac.type >= ixgbe_mac_X550) &&
>>> 	    (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED)) {
>>> 		unsigned int pf_pool = adapter->num_vfs;
>>> @@ -3364,12 +3421,12 @@ static void ixgbe_setup_mrqc(struct ixgbe_adapter *adapter)
>>> 		IXGBE_WRITE_REG(hw, IXGBE_MRQC, mrqc);
>>>
>>> 		/* Setup RSS through the VF registers */
>>> -		ixgbe_setup_vfreta(adapter, rss_key);
>>> +		ixgbe_setup_vfreta(adapter);
>>> 		vfmrqc = IXGBE_MRQC_RSSEN;
>>> 		vfmrqc |= rss_field;
>>> 		IXGBE_WRITE_REG(hw, IXGBE_PFVFMRQC(pf_pool), vfmrqc);
>>> 	} else {
>>> -		ixgbe_setup_reta(adapter, rss_key);
>>> +		ixgbe_setup_reta(adapter);
>>> 		mrqc |= rss_field;
>>> 		IXGBE_WRITE_REG(hw, IXGBE_MRQC, mrqc);
>>> 	}
>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
>>> index c3ddc94..0c671b1 100644
>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h>
>>> @@ -311,6 +311,13 @@ struct ixgbe_thermal_sensor_data {
>>> #define IXGBE_ERETA(_i)	(0x0EE80 + ((_i) * 4))  /* 96 of these (0-95) */
>>> #define IXGBE_RSSRK(_i) (0x05C80 + ((_i) * 4))  /* 10 of these (0-9) */
>>>
>>> +/* maximum number of RETA entries among all devices supported by ixgbe
>>> + * driver: currently it's x550 device in non-SRIOV mode
>>> + */
>>> +#define IXGBE_MAX_RETA_ENTRIES 512
>>> +
>>> +#define IXGBE_RSS_KEY_SIZE     40  /* size of RSS Hash Key in bytes */
>>> +
>> These defines should be in ixgbe.h
>
>Why? These are HW related macros and IMHO they belong to ixgbe_type.h 
>with other HW related macros and not to ixgbe.h where SW related macros are.

These are just size/limits defines and we keep many of those in ixgbe.h. If it happens that we need them in the files that deal strictly with HW then it probably makes sense to have them in ixgbe_type.h, but I don't see this being the case here.

Thanks,
Emil

RE: [Intel-wired-lan] [PATCH net-next v1 1/2] ixgbe: Refactor the RSS configuration code.

[Tantilov, Emil S]

>-----Original Message-----
>From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com] 
>Sent: Thursday, March 26, 2015 1:55 PM
>To: Tantilov, Emil S; netdev@vger.kernel.org
>Cc: avi@cloudius-systems.com; intel-wired-lan@lists.osuosl.org; gleb@cloudius-systems.com
>Subject: Re: [Intel-wired-lan] [PATCH net-next v1 1/2] ixgbe: Refactor the RSS configuration code.
>
>
>
>On 03/26/15 22:20, Tantilov, Emil S wrote:
>>> -----Original Message-----
>>> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On Behalf Of Vlad Zolotarov
>>> Sent: Thursday, March 26, 2015 10:57 AM
>>> Subject: [Intel-wired-lan] [PATCH net-next v1 1/2] ixgbe: Refactor the RSS configuration code.
>>>
>>> This patch is a preparation for enablement of ethtool RSS indirection table
>>> and hash key querying. We don't want to read registers every time the RSS info
>>> is queried. Therefore we will store its current content in the
>>> arrays in the adapter struct and will read it from there (instead of from
>>> registers) when requested.
>>>
>>> Will change the code that writes the indirection table and hash key into the HW registers
>>> to take its content from these arrays. This will also simplify the indirection
>>> table updating ethtool callback implementation in the future.
>>>
>>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>>> ---
>>> drivers/net/ethernet/intel/ixgbe/ixgbe.h      |   2 +
>>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 143 ++++++++++++++++++--------
>>> drivers/net/ethernet/intel/ixgbe/ixgbe_type.h |   7 ++
>>> 3 files changed, 109 insertions(+), 43 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>> index 5e44e48..402d182 100644
>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>> @@ -766,6 +766,8 @@ struct ixgbe_adapter {
>>>
>>> 	u8 default_up;
>>> 	unsigned long fwd_bitmask; /* Bitmask indicating in use pools */
>>> +	u8 rss_indir_tbl[IXGBE_MAX_RETA_ENTRIES];
>>> +	u32 rss_key[IXGBE_RSS_KEY_SIZE / sizeof(u32)];
>>> };
>>>
>>> static inline u8 ixgbe_max_rss_indices(struct ixgbe_adapter *adapter)
>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> index 8853d52..e3de0c9 100644
>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> @@ -3228,51 +3228,54 @@ static void ixgbe_configure_srrctl(struct ixgbe_adapter *adapter,
>>> 	IXGBE_WRITE_REG(hw, IXGBE_SRRCTL(reg_idx), srrctl);
>>> }
>>>
>>> -static void ixgbe_setup_reta(struct ixgbe_adapter *adapter, const u32 *seed)
>>> +/**
>>> + * Return a number of entries in the RSS indirection table
>> The preferred style for multiline comments is to not start with an empty "/*" checkpatch would generally
>> catch those.
>
>It's not a multi-line comment but a function description and its styling 
>is perfectly fine.

Right - I replied under the wrong comment. There is however an old style comment that you moved that shows up in checkpatch.

>>> + *
>>> + * @adapter: device handle
>>> + *
>>> + *  - 82598/82599/X540:     128
>>> + *  - X550(non-SRIOV mode): 512
>>> + *  - X550(SRIOV mode):     64
>>> + */
>>> +static u32 ixgbe_rss_indir_tbl_entries(struct ixgbe_adapter *adapter)
>>> +{
>>> +	if (adapter->hw.mac.type < ixgbe_mac_X550)
>>> +		return 128;
>>> +	else if (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED)
>>> +		return 64;
>>> +	else
>>> +		return 512;
>>> +}
>>> +
>>> +/**
>>> + * Write the RETA table to HW
>>> + *
>>> + * @adapter: device handle
>>> + *
>>> + * Write the RSS redirection table stored in adapter.rss_indir_tbl[] to HW.
>>> + */
>>> +static void ixgbe_store_reta(struct ixgbe_adapter *adapter)
>> What is the point of having separate functions for writing the registers? Unless you intend to use
>> them in other parts of the code, those can be folded inside ixgbe_setupvf/reta().
>
>The one that is going to implement the ethtool -X is going to use it. ;)

Alright that makes sense.

>>> {
>>> +	u32 i, reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
>>> 	struct ixgbe_hw *hw = &adapter->hw;
>>> 	u32 reta = 0;
>>> -	int i, j;
>>> -	int reta_entries = 128;
>>> -	u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
>>> 	int indices_multi;
>>> -
>>> -	/*
>>> -	 * Program table for at least 2 queues w/ SR-IOV so that VFs can
>>> -	 * make full use of any rings they may have.  We will use the
>>> -	 * PSRTYPE register to control how many rings we use within the PF.
>>> -	 */
>>> -	if ((adapter->flags & IXGBE_FLAG_SRIOV_ENABLED) && (rss_i < 2)) >
>>> -		rss_i = 2;
>>> -
>>> -	/* Fill out hash function seeds */
>>> -	for (i = 0; i < 10; i++)
>>> -		IXGBE_WRITE_REG(hw, IXGBE_RSSRK(i), seed[i]);
>>> +	u8 *indir_tbl = adapter->rss_indir_tbl;
>>>
>>> 	/* Fill out the redirection table as follows:
>>> -	 * 82598: 128 (8 bit wide) entries containing pair of 4 bit RSS indices
>>> -	 * 82599/X540: 128 (8 bit wide) entries containing 4 bit RSS index
>>> -	 * X550: 512 (8 bit wide) entries containing 6 bit RSS index
>>> +	 *  - 82598:      8 bit wide entries containing pair of 4 bit RSS
>>> +	 *    indices.
>>> +	 *  - 82599/X540: 8 bit wide entries containing 4 bit RSS index
>>> +	 *  - X550:       8 bit wide entries containing 6 bit RSS index
>>> 	 */
>>> 	if (adapter->hw.mac.type == ixgbe_mac_82598EB)
>>> 		indices_multi = 0x11;
>>> 	else
>>> 		indices_multi = 0x1;
>>>
>>> -	switch (adapter->hw.mac.type) {
>>> -	case ixgbe_mac_X550:
>>> -	case ixgbe_mac_X550EM_x:
>>> -		if (!(adapter->flags & IXGBE_FLAG_SRIOV_ENABLED))
>>> -			reta_entries = 512;
>>> -	default:
>>> -		break;
>>> -	}
>>> -
>>> -	/* Fill out redirection table */
>>> -	for (i = 0, j = 0; i < reta_entries; i++, j++) {
>>> -		if (j == rss_i)
>>> -			j = 0;
>>> -		reta = (reta << 8) | (j * indices_multi);
>>> +	/* Write redirection table to HW */
>>> +	for (i = 0; i < reta_entries; i++) {
>>> +		reta = (reta << 8) | (indices_multi * indir_tbl[i]);
>>> 		if ((i & 3) == 3) {
>>> 			if (i < 128)
>>> 				IXGBE_WRITE_REG(hw, IXGBE_RETA(i >> 2), reta);
>>> @@ -3283,34 +3286,88 @@ static void ixgbe_setup_reta(struct ixgbe_adapter *adapter, const u32 *seed)
>>> 	}
>>> }
>>>
>>> -static void ixgbe_setup_vfreta(struct ixgbe_adapter *adapter, const u32 *seed)
>>> +/**
>>> + * Write the RETA table to HW (for x550 devices in SRIOV mode)
>>> + *
>>> + * @adapter: device handle
>>> + *
>>> + * Write the RSS redirection table stored in adapter.rss_indir_tbl[] to HW.
>>> + */
>>> +static void ixgbe_store_vfreta(struct ixgbe_adapter *adapter)
>>> {
>>> +	u32 i, reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
>>> 	struct ixgbe_hw *hw = &adapter->hw;
>>> 	u32 vfreta = 0;
>>> +	unsigned int pf_pool = adapter->num_vfs;
>>> +
>>> +	/* Write redirection table to HW */
>>> +	for (i = 0; i < reta_entries; i++) {
>>> +		vfreta = (vfreta << 8) | adapter->rss_indir_tbl[i];
>>> +		if ((i & 3) == 3)
>>> +			IXGBE_WRITE_REG(hw, IXGBE_PFVFRETA(i >> 2, pf_pool),
>>> +					vfreta);
>>> +	}
>>> +}
>>> +
>>> +static void ixgbe_setup_reta(struct ixgbe_adapter *adapter)
>>> +{
>>> +	struct ixgbe_hw *hw = &adapter->hw;
>>> +	u32 i, j;
>>> +	u32 reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
>>> +	u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
>>> +
>>> +	/*
>>> +	 * Program table for at least 2 queues w/ SR-IOV so that VFs can
>>> +	 * make full use of any rings they may have.  We will use the
>>> +	 * PSRTYPE register to control how many rings we use within the PF.
>>> +	 */

This one.

>>> +	if ((adapter->flags & IXGBE_FLAG_SRIOV_ENABLED) && (rss_i < 2))
>>> +		rss_i = 2;
>>> +
>>> +	/* Fill out hash function seeds */
>>> +	for (i = 0; i < 10; i++)
>>> +		IXGBE_WRITE_REG(hw, IXGBE_RSSRK(i), adapter->rss_key[i]);
>>> +
>>> +	/* Fill out redirection table */
>>> +	memset(adapter->rss_indir_tbl, 0, sizeof(adapter->rss_indir_tbl));
>>> +
>>> +	for (i = 0, j = 0; i < reta_entries; i++, j++) {
>>> +		if (j == rss_i)
>>> +			j = 0;
>>> +
>>> +		adapter->rss_indir_tbl[i] = j;
>>> +	}
>>> +
>>> +	ixgbe_store_reta(adapter);
>>> +}
>>> +
>>> +static void ixgbe_setup_vfreta(struct ixgbe_adapter *adapter)
>>> +{
>>> +	struct ixgbe_hw *hw = &adapter->hw;
>>> 	u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
>>> 	unsigned int pf_pool = adapter->num_vfs;
>>> 	int i, j;
>>>
>>> 	/* Fill out hash function seeds */
>>> 	for (i = 0; i < 10; i++)
>>> -		IXGBE_WRITE_REG(hw, IXGBE_PFVFRSSRK(i, pf_pool), seed[i]);
>>> +		IXGBE_WRITE_REG(hw, IXGBE_PFVFRSSRK(i, pf_pool),
>>> +				adapter->rss_key[i]);
>>>
>>> 	/* Fill out the redirection table */
>>> 	for (i = 0, j = 0; i < 64; i++, j++) {
>>> 		if (j == rss_i)
>>> 			j = 0;
>>> -		vfreta = (vfreta << 8) | j;
>>> -		if ((i & 3) == 3)
>>> -			IXGBE_WRITE_REG(hw, IXGBE_PFVFRETA(i >> 2, pf_pool),
>>> -					vfreta);
>>> +
>>> +		adapter->rss_indir_tbl[i] = j;
>>> 	}
>>> +
>>> +	ixgbe_store_vfreta(adapter);
>>> }
>>>
>>> static void ixgbe_setup_mrqc(struct ixgbe_adapter *adapter)
>>> {
>>> 	struct ixgbe_hw *hw = &adapter->hw;
>>> 	u32 mrqc = 0, rss_field = 0, vfmrqc = 0;
>>> -	u32 rss_key[10];
>>> 	u32 rxcsum;
>>>
>>> 	/* Disable indicating checksum in descriptor, enables RSS hash */
>>> @@ -3354,7 +3411,7 @@ static void ixgbe_setup_mrqc(struct ixgbe_adapter *adapter)
>>> 	if (adapter->flags2 & IXGBE_FLAG2_RSS_FIELD_IPV6_UDP)
>>> 		rss_field |= IXGBE_MRQC_RSS_FIELD_IPV6_UDP;
>>>
>>> -	netdev_rss_key_fill(rss_key, sizeof(rss_key));
>>> +	netdev_rss_key_fill(adapter->rss_key, sizeof(adapter->rss_key));
>>> 	if ((hw->mac.type >= ixgbe_mac_X550) &&
>>> 	    (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED)) {
>>> 		unsigned int pf_pool = adapter->num_vfs;
>>> @@ -3364,12 +3421,12 @@ static void ixgbe_setup_mrqc(struct ixgbe_adapter *adapter)
>>> 		IXGBE_WRITE_REG(hw, IXGBE_MRQC, mrqc);
>>>
>>> 		/* Setup RSS through the VF registers */
>>> -		ixgbe_setup_vfreta(adapter, rss_key);
>>> +		ixgbe_setup_vfreta(adapter);
>>> 		vfmrqc = IXGBE_MRQC_RSSEN;
>>> 		vfmrqc |= rss_field;
>>> 		IXGBE_WRITE_REG(hw, IXGBE_PFVFMRQC(pf_pool), vfmrqc);
>>> 	} else {
>>> -		ixgbe_setup_reta(adapter, rss_key);
>>> +		ixgbe_setup_reta(adapter);
>>> 		mrqc |= rss_field;
>>> 		IXGBE_WRITE_REG(hw, IXGBE_MRQC, mrqc);
>>> 	}
>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
>>> index c3ddc94..0c671b1 100644
>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h>
>>> @@ -311,6 +311,13 @@ struct ixgbe_thermal_sensor_data {
>>> #define IXGBE_ERETA(_i)	(0x0EE80 + ((_i) * 4))  /* 96 of these (0-95) */
>>> #define IXGBE_RSSRK(_i) (0x05C80 + ((_i) * 4))  /* 10 of these (0-9) */
>>>
>>> +/* maximum number of RETA entries among all devices supported by ixgbe
>>> + * driver: currently it's x550 device in non-SRIOV mode
>>> + */
>>> +#define IXGBE_MAX_RETA_ENTRIES 512
>>> +
>>> +#define IXGBE_RSS_KEY_SIZE     40  /* size of RSS Hash Key in bytes */
>>> +
>> These defines should be in ixgbe.h
>
>Why? These are HW related macros and IMHO they belong to ixgbe_type.h 
>with other HW related macros and not to ixgbe.h where SW related macros are.

These are just size/limits defines and we keep many of those in ixgbe.h. If it happens that we need them in the files that deal strictly with HW then it probably makes sense to have them in ixgbe_type.h, but I don't see this being the case here.

Thanks,
Emil

[Intel-wired-lan] [PATCH net-next v1 1/2] ixgbe: Refactor the RSS configuration code.

[Vlad Zolotarov]

On 03/26/15 23:27, Tantilov, Emil S wrote:
>> -----Original Message-----
>> From: Vlad Zolotarov [mailto:vladz at cloudius-systems.com]
>> Sent: Thursday, March 26, 2015 1:55 PM
>> To: Tantilov, Emil S; netdev at vger.kernel.org
>> Cc: avi at cloudius-systems.com; intel-wired-lan at lists.osuosl.org; gleb at cloudius-systems.com
>> Subject: Re: [Intel-wired-lan] [PATCH net-next v1 1/2] ixgbe: Refactor the RSS configuration code.
>>
>>
>>
>> On 03/26/15 22:20, Tantilov, Emil S wrote:
>>>> -----Original Message-----
>>>> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On Behalf Of Vlad Zolotarov
>>>> Sent: Thursday, March 26, 2015 10:57 AM
>>>> Subject: [Intel-wired-lan] [PATCH net-next v1 1/2] ixgbe: Refactor the RSS configuration code.
>>>>
>>>> This patch is a preparation for enablement of ethtool RSS indirection table
>>>> and hash key querying. We don't want to read registers every time the RSS info
>>>> is queried. Therefore we will store its current content in the
>>>> arrays in the adapter struct and will read it from there (instead of from
>>>> registers) when requested.
>>>>
>>>> Will change the code that writes the indirection table and hash key into the HW registers
>>>> to take its content from these arrays. This will also simplify the indirection
>>>> table updating ethtool callback implementation in the future.
>>>>
>>>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>>>> ---
>>>> drivers/net/ethernet/intel/ixgbe/ixgbe.h      |   2 +
>>>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 143 ++++++++++++++++++--------
>>>> drivers/net/ethernet/intel/ixgbe/ixgbe_type.h |   7 ++
>>>> 3 files changed, 109 insertions(+), 43 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>>> index 5e44e48..402d182 100644
>>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>>> @@ -766,6 +766,8 @@ struct ixgbe_adapter {
>>>>
>>>> 	u8 default_up;
>>>> 	unsigned long fwd_bitmask; /* Bitmask indicating in use pools */
>>>> +	u8 rss_indir_tbl[IXGBE_MAX_RETA_ENTRIES];
>>>> +	u32 rss_key[IXGBE_RSS_KEY_SIZE / sizeof(u32)];
>>>> };
>>>>
>>>> static inline u8 ixgbe_max_rss_indices(struct ixgbe_adapter *adapter)
>>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>> index 8853d52..e3de0c9 100644
>>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>> @@ -3228,51 +3228,54 @@ static void ixgbe_configure_srrctl(struct ixgbe_adapter *adapter,
>>>> 	IXGBE_WRITE_REG(hw, IXGBE_SRRCTL(reg_idx), srrctl);
>>>> }
>>>>
>>>> -static void ixgbe_setup_reta(struct ixgbe_adapter *adapter, const u32 *seed)
>>>> +/**
>>>> + * Return a number of entries in the RSS indirection table
>>> The preferred style for multiline comments is to not start with an empty "/*" checkpatch would generally
>>> catch those.
>> It's not a multi-line comment but a function description and its styling
>> is perfectly fine.
> Right - I replied under the wrong comment. There is however an old style comment that you moved that shows up in checkpatch.
>
>>>> + *
>>>> + * @adapter: device handle
>>>> + *
>>>> + *  - 82598/82599/X540:     128
>>>> + *  - X550(non-SRIOV mode): 512
>>>> + *  - X550(SRIOV mode):     64
>>>> + */
>>>> +static u32 ixgbe_rss_indir_tbl_entries(struct ixgbe_adapter *adapter)
>>>> +{
>>>> +	if (adapter->hw.mac.type < ixgbe_mac_X550)
>>>> +		return 128;
>>>> +	else if (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED)
>>>> +		return 64;
>>>> +	else
>>>> +		return 512;
>>>> +}
>>>> +
>>>> +/**
>>>> + * Write the RETA table to HW
>>>> + *
>>>> + * @adapter: device handle
>>>> + *
>>>> + * Write the RSS redirection table stored in adapter.rss_indir_tbl[] to HW.
>>>> + */
>>>> +static void ixgbe_store_reta(struct ixgbe_adapter *adapter)
>>> What is the point of having separate functions for writing the registers? Unless you intend to use
>>> them in other parts of the code, those can be folded inside ixgbe_setupvf/reta().
>> The one that is going to implement the ethtool -X is going to use it. ;)
> Alright that makes sense.
>
>>>> {
>>>> +	u32 i, reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
>>>> 	struct ixgbe_hw *hw = &adapter->hw;
>>>> 	u32 reta = 0;
>>>> -	int i, j;
>>>> -	int reta_entries = 128;
>>>> -	u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
>>>> 	int indices_multi;
>>>> -
>>>> -	/*
>>>> -	 * Program table for at least 2 queues w/ SR-IOV so that VFs can
>>>> -	 * make full use of any rings they may have.  We will use the
>>>> -	 * PSRTYPE register to control how many rings we use within the PF.
>>>> -	 */
>>>> -	if ((adapter->flags & IXGBE_FLAG_SRIOV_ENABLED) && (rss_i < 2)) >
>>>> -		rss_i = 2;
>>>> -
>>>> -	/* Fill out hash function seeds */
>>>> -	for (i = 0; i < 10; i++)
>>>> -		IXGBE_WRITE_REG(hw, IXGBE_RSSRK(i), seed[i]);
>>>> +	u8 *indir_tbl = adapter->rss_indir_tbl;
>>>>
>>>> 	/* Fill out the redirection table as follows:
>>>> -	 * 82598: 128 (8 bit wide) entries containing pair of 4 bit RSS indices
>>>> -	 * 82599/X540: 128 (8 bit wide) entries containing 4 bit RSS index
>>>> -	 * X550: 512 (8 bit wide) entries containing 6 bit RSS index
>>>> +	 *  - 82598:      8 bit wide entries containing pair of 4 bit RSS
>>>> +	 *    indices.
>>>> +	 *  - 82599/X540: 8 bit wide entries containing 4 bit RSS index
>>>> +	 *  - X550:       8 bit wide entries containing 6 bit RSS index
>>>> 	 */
>>>> 	if (adapter->hw.mac.type == ixgbe_mac_82598EB)
>>>> 		indices_multi = 0x11;
>>>> 	else
>>>> 		indices_multi = 0x1;
>>>>
>>>> -	switch (adapter->hw.mac.type) {
>>>> -	case ixgbe_mac_X550:
>>>> -	case ixgbe_mac_X550EM_x:
>>>> -		if (!(adapter->flags & IXGBE_FLAG_SRIOV_ENABLED))
>>>> -			reta_entries = 512;
>>>> -	default:
>>>> -		break;
>>>> -	}
>>>> -
>>>> -	/* Fill out redirection table */
>>>> -	for (i = 0, j = 0; i < reta_entries; i++, j++) {
>>>> -		if (j == rss_i)
>>>> -			j = 0;
>>>> -		reta = (reta << 8) | (j * indices_multi);
>>>> +	/* Write redirection table to HW */
>>>> +	for (i = 0; i < reta_entries; i++) {
>>>> +		reta = (reta << 8) | (indices_multi * indir_tbl[i]);
>>>> 		if ((i & 3) == 3) {
>>>> 			if (i < 128)
>>>> 				IXGBE_WRITE_REG(hw, IXGBE_RETA(i >> 2), reta);
>>>> @@ -3283,34 +3286,88 @@ static void ixgbe_setup_reta(struct ixgbe_adapter *adapter, const u32 *seed)
>>>> 	}
>>>> }
>>>>
>>>> -static void ixgbe_setup_vfreta(struct ixgbe_adapter *adapter, const u32 *seed)
>>>> +/**
>>>> + * Write the RETA table to HW (for x550 devices in SRIOV mode)
>>>> + *
>>>> + * @adapter: device handle
>>>> + *
>>>> + * Write the RSS redirection table stored in adapter.rss_indir_tbl[] to HW.
>>>> + */
>>>> +static void ixgbe_store_vfreta(struct ixgbe_adapter *adapter)
>>>> {
>>>> +	u32 i, reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
>>>> 	struct ixgbe_hw *hw = &adapter->hw;
>>>> 	u32 vfreta = 0;
>>>> +	unsigned int pf_pool = adapter->num_vfs;
>>>> +
>>>> +	/* Write redirection table to HW */
>>>> +	for (i = 0; i < reta_entries; i++) {
>>>> +		vfreta = (vfreta << 8) | adapter->rss_indir_tbl[i];
>>>> +		if ((i & 3) == 3)
>>>> +			IXGBE_WRITE_REG(hw, IXGBE_PFVFRETA(i >> 2, pf_pool),
>>>> +					vfreta);
>>>> +	}
>>>> +}
>>>> +
>>>> +static void ixgbe_setup_reta(struct ixgbe_adapter *adapter)
>>>> +{
>>>> +	struct ixgbe_hw *hw = &adapter->hw;
>>>> +	u32 i, j;
>>>> +	u32 reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
>>>> +	u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
>>>> +
>>>> +	/*
>>>> +	 * Program table for at least 2 queues w/ SR-IOV so that VFs can
>>>> +	 * make full use of any rings they may have.  We will use the
>>>> +	 * PSRTYPE register to control how many rings we use within the PF.
>>>> +	 */
> This one.

I saw this warning but it wasn't me who wrote this comment so it didn't 
feel right to fix it in the functional patch. U know what, I'll fix it 
in order to silence the checkpatch.

>
>>>> +	if ((adapter->flags & IXGBE_FLAG_SRIOV_ENABLED) && (rss_i < 2))
>>>> +		rss_i = 2;
>>>> +
>>>> +	/* Fill out hash function seeds */
>>>> +	for (i = 0; i < 10; i++)
>>>> +		IXGBE_WRITE_REG(hw, IXGBE_RSSRK(i), adapter->rss_key[i]);
>>>> +
>>>> +	/* Fill out redirection table */
>>>> +	memset(adapter->rss_indir_tbl, 0, sizeof(adapter->rss_indir_tbl));
>>>> +
>>>> +	for (i = 0, j = 0; i < reta_entries; i++, j++) {
>>>> +		if (j == rss_i)
>>>> +			j = 0;
>>>> +
>>>> +		adapter->rss_indir_tbl[i] = j;
>>>> +	}
>>>> +
>>>> +	ixgbe_store_reta(adapter);
>>>> +}
>>>> +
>>>> +static void ixgbe_setup_vfreta(struct ixgbe_adapter *adapter)
>>>> +{
>>>> +	struct ixgbe_hw *hw = &adapter->hw;
>>>> 	u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
>>>> 	unsigned int pf_pool = adapter->num_vfs;
>>>> 	int i, j;
>>>>
>>>> 	/* Fill out hash function seeds */
>>>> 	for (i = 0; i < 10; i++)
>>>> -		IXGBE_WRITE_REG(hw, IXGBE_PFVFRSSRK(i, pf_pool), seed[i]);
>>>> +		IXGBE_WRITE_REG(hw, IXGBE_PFVFRSSRK(i, pf_pool),
>>>> +				adapter->rss_key[i]);
>>>>
>>>> 	/* Fill out the redirection table */
>>>> 	for (i = 0, j = 0; i < 64; i++, j++) {
>>>> 		if (j == rss_i)
>>>> 			j = 0;
>>>> -		vfreta = (vfreta << 8) | j;
>>>> -		if ((i & 3) == 3)
>>>> -			IXGBE_WRITE_REG(hw, IXGBE_PFVFRETA(i >> 2, pf_pool),
>>>> -					vfreta);
>>>> +
>>>> +		adapter->rss_indir_tbl[i] = j;
>>>> 	}
>>>> +
>>>> +	ixgbe_store_vfreta(adapter);
>>>> }
>>>>
>>>> static void ixgbe_setup_mrqc(struct ixgbe_adapter *adapter)
>>>> {
>>>> 	struct ixgbe_hw *hw = &adapter->hw;
>>>> 	u32 mrqc = 0, rss_field = 0, vfmrqc = 0;
>>>> -	u32 rss_key[10];
>>>> 	u32 rxcsum;
>>>>
>>>> 	/* Disable indicating checksum in descriptor, enables RSS hash */
>>>> @@ -3354,7 +3411,7 @@ static void ixgbe_setup_mrqc(struct ixgbe_adapter *adapter)
>>>> 	if (adapter->flags2 & IXGBE_FLAG2_RSS_FIELD_IPV6_UDP)
>>>> 		rss_field |= IXGBE_MRQC_RSS_FIELD_IPV6_UDP;
>>>>
>>>> -	netdev_rss_key_fill(rss_key, sizeof(rss_key));
>>>> +	netdev_rss_key_fill(adapter->rss_key, sizeof(adapter->rss_key));
>>>> 	if ((hw->mac.type >= ixgbe_mac_X550) &&
>>>> 	    (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED)) {
>>>> 		unsigned int pf_pool = adapter->num_vfs;
>>>> @@ -3364,12 +3421,12 @@ static void ixgbe_setup_mrqc(struct ixgbe_adapter *adapter)
>>>> 		IXGBE_WRITE_REG(hw, IXGBE_MRQC, mrqc);
>>>>
>>>> 		/* Setup RSS through the VF registers */
>>>> -		ixgbe_setup_vfreta(adapter, rss_key);
>>>> +		ixgbe_setup_vfreta(adapter);
>>>> 		vfmrqc = IXGBE_MRQC_RSSEN;
>>>> 		vfmrqc |= rss_field;
>>>> 		IXGBE_WRITE_REG(hw, IXGBE_PFVFMRQC(pf_pool), vfmrqc);
>>>> 	} else {
>>>> -		ixgbe_setup_reta(adapter, rss_key);
>>>> +		ixgbe_setup_reta(adapter);
>>>> 		mrqc |= rss_field;
>>>> 		IXGBE_WRITE_REG(hw, IXGBE_MRQC, mrqc);
>>>> 	}
>>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
>>>> index c3ddc94..0c671b1 100644
>>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
>>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h>
>>>> @@ -311,6 +311,13 @@ struct ixgbe_thermal_sensor_data {
>>>> #define IXGBE_ERETA(_i)	(0x0EE80 + ((_i) * 4))  /* 96 of these (0-95) */
>>>> #define IXGBE_RSSRK(_i) (0x05C80 + ((_i) * 4))  /* 10 of these (0-9) */
>>>>
>>>> +/* maximum number of RETA entries among all devices supported by ixgbe
>>>> + * driver: currently it's x550 device in non-SRIOV mode
>>>> + */
>>>> +#define IXGBE_MAX_RETA_ENTRIES 512
>>>> +
>>>> +#define IXGBE_RSS_KEY_SIZE     40  /* size of RSS Hash Key in bytes */
>>>> +
>>> These defines should be in ixgbe.h
>> Why? These are HW related macros and IMHO they belong to ixgbe_type.h
>> with other HW related macros and not to ixgbe.h where SW related macros are.
> These are just size/limits defines and we keep many of those in ixgbe.h.

These are HW sizes/limits.

> If it happens that we need them in the files that deal strictly with HW then it probably makes sense to have them in ixgbe_type.h, but I don't see this being the case here.

That's a strange strategy to separate the values not by their nature but 
by the nature of their usage but whatever... I'll move it to ixgbe.h in v2.

>
> Thanks,
> Emil
>

Re: [Intel-wired-lan] [PATCH net-next v1 1/2] ixgbe: Refactor the RSS configuration code.

[Vlad Zolotarov]

On 03/26/15 23:27, Tantilov, Emil S wrote:
>> -----Original Message-----
>> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
>> Sent: Thursday, March 26, 2015 1:55 PM
>> To: Tantilov, Emil S; netdev@vger.kernel.org
>> Cc: avi@cloudius-systems.com; intel-wired-lan@lists.osuosl.org; gleb@cloudius-systems.com
>> Subject: Re: [Intel-wired-lan] [PATCH net-next v1 1/2] ixgbe: Refactor the RSS configuration code.
>>
>>
>>
>> On 03/26/15 22:20, Tantilov, Emil S wrote:
>>>> -----Original Message-----
>>>> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On Behalf Of Vlad Zolotarov
>>>> Sent: Thursday, March 26, 2015 10:57 AM
>>>> Subject: [Intel-wired-lan] [PATCH net-next v1 1/2] ixgbe: Refactor the RSS configuration code.
>>>>
>>>> This patch is a preparation for enablement of ethtool RSS indirection table
>>>> and hash key querying. We don't want to read registers every time the RSS info
>>>> is queried. Therefore we will store its current content in the
>>>> arrays in the adapter struct and will read it from there (instead of from
>>>> registers) when requested.
>>>>
>>>> Will change the code that writes the indirection table and hash key into the HW registers
>>>> to take its content from these arrays. This will also simplify the indirection
>>>> table updating ethtool callback implementation in the future.
>>>>
>>>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>>>> ---
>>>> drivers/net/ethernet/intel/ixgbe/ixgbe.h      |   2 +
>>>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 143 ++++++++++++++++++--------
>>>> drivers/net/ethernet/intel/ixgbe/ixgbe_type.h |   7 ++
>>>> 3 files changed, 109 insertions(+), 43 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>>> index 5e44e48..402d182 100644
>>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>>> @@ -766,6 +766,8 @@ struct ixgbe_adapter {
>>>>
>>>> 	u8 default_up;
>>>> 	unsigned long fwd_bitmask; /* Bitmask indicating in use pools */
>>>> +	u8 rss_indir_tbl[IXGBE_MAX_RETA_ENTRIES];
>>>> +	u32 rss_key[IXGBE_RSS_KEY_SIZE / sizeof(u32)];
>>>> };
>>>>
>>>> static inline u8 ixgbe_max_rss_indices(struct ixgbe_adapter *adapter)
>>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>> index 8853d52..e3de0c9 100644
>>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>> @@ -3228,51 +3228,54 @@ static void ixgbe_configure_srrctl(struct ixgbe_adapter *adapter,
>>>> 	IXGBE_WRITE_REG(hw, IXGBE_SRRCTL(reg_idx), srrctl);
>>>> }
>>>>
>>>> -static void ixgbe_setup_reta(struct ixgbe_adapter *adapter, const u32 *seed)
>>>> +/**
>>>> + * Return a number of entries in the RSS indirection table
>>> The preferred style for multiline comments is to not start with an empty "/*" checkpatch would generally
>>> catch those.
>> It's not a multi-line comment but a function description and its styling
>> is perfectly fine.
> Right - I replied under the wrong comment. There is however an old style comment that you moved that shows up in checkpatch.
>
>>>> + *
>>>> + * @adapter: device handle
>>>> + *
>>>> + *  - 82598/82599/X540:     128
>>>> + *  - X550(non-SRIOV mode): 512
>>>> + *  - X550(SRIOV mode):     64
>>>> + */
>>>> +static u32 ixgbe_rss_indir_tbl_entries(struct ixgbe_adapter *adapter)
>>>> +{
>>>> +	if (adapter->hw.mac.type < ixgbe_mac_X550)
>>>> +		return 128;
>>>> +	else if (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED)
>>>> +		return 64;
>>>> +	else
>>>> +		return 512;
>>>> +}
>>>> +
>>>> +/**
>>>> + * Write the RETA table to HW
>>>> + *
>>>> + * @adapter: device handle
>>>> + *
>>>> + * Write the RSS redirection table stored in adapter.rss_indir_tbl[] to HW.
>>>> + */
>>>> +static void ixgbe_store_reta(struct ixgbe_adapter *adapter)
>>> What is the point of having separate functions for writing the registers? Unless you intend to use
>>> them in other parts of the code, those can be folded inside ixgbe_setupvf/reta().
>> The one that is going to implement the ethtool -X is going to use it. ;)
> Alright that makes sense.
>
>>>> {
>>>> +	u32 i, reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
>>>> 	struct ixgbe_hw *hw = &adapter->hw;
>>>> 	u32 reta = 0;
>>>> -	int i, j;
>>>> -	int reta_entries = 128;
>>>> -	u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
>>>> 	int indices_multi;
>>>> -
>>>> -	/*
>>>> -	 * Program table for at least 2 queues w/ SR-IOV so that VFs can
>>>> -	 * make full use of any rings they may have.  We will use the
>>>> -	 * PSRTYPE register to control how many rings we use within the PF.
>>>> -	 */
>>>> -	if ((adapter->flags & IXGBE_FLAG_SRIOV_ENABLED) && (rss_i < 2)) >
>>>> -		rss_i = 2;
>>>> -
>>>> -	/* Fill out hash function seeds */
>>>> -	for (i = 0; i < 10; i++)
>>>> -		IXGBE_WRITE_REG(hw, IXGBE_RSSRK(i), seed[i]);
>>>> +	u8 *indir_tbl = adapter->rss_indir_tbl;
>>>>
>>>> 	/* Fill out the redirection table as follows:
>>>> -	 * 82598: 128 (8 bit wide) entries containing pair of 4 bit RSS indices
>>>> -	 * 82599/X540: 128 (8 bit wide) entries containing 4 bit RSS index
>>>> -	 * X550: 512 (8 bit wide) entries containing 6 bit RSS index
>>>> +	 *  - 82598:      8 bit wide entries containing pair of 4 bit RSS
>>>> +	 *    indices.
>>>> +	 *  - 82599/X540: 8 bit wide entries containing 4 bit RSS index
>>>> +	 *  - X550:       8 bit wide entries containing 6 bit RSS index
>>>> 	 */
>>>> 	if (adapter->hw.mac.type == ixgbe_mac_82598EB)
>>>> 		indices_multi = 0x11;
>>>> 	else
>>>> 		indices_multi = 0x1;
>>>>
>>>> -	switch (adapter->hw.mac.type) {
>>>> -	case ixgbe_mac_X550:
>>>> -	case ixgbe_mac_X550EM_x:
>>>> -		if (!(adapter->flags & IXGBE_FLAG_SRIOV_ENABLED))
>>>> -			reta_entries = 512;
>>>> -	default:
>>>> -		break;
>>>> -	}
>>>> -
>>>> -	/* Fill out redirection table */
>>>> -	for (i = 0, j = 0; i < reta_entries; i++, j++) {
>>>> -		if (j == rss_i)
>>>> -			j = 0;
>>>> -		reta = (reta << 8) | (j * indices_multi);
>>>> +	/* Write redirection table to HW */
>>>> +	for (i = 0; i < reta_entries; i++) {
>>>> +		reta = (reta << 8) | (indices_multi * indir_tbl[i]);
>>>> 		if ((i & 3) == 3) {
>>>> 			if (i < 128)
>>>> 				IXGBE_WRITE_REG(hw, IXGBE_RETA(i >> 2), reta);
>>>> @@ -3283,34 +3286,88 @@ static void ixgbe_setup_reta(struct ixgbe_adapter *adapter, const u32 *seed)
>>>> 	}
>>>> }
>>>>
>>>> -static void ixgbe_setup_vfreta(struct ixgbe_adapter *adapter, const u32 *seed)
>>>> +/**
>>>> + * Write the RETA table to HW (for x550 devices in SRIOV mode)
>>>> + *
>>>> + * @adapter: device handle
>>>> + *
>>>> + * Write the RSS redirection table stored in adapter.rss_indir_tbl[] to HW.
>>>> + */
>>>> +static void ixgbe_store_vfreta(struct ixgbe_adapter *adapter)
>>>> {
>>>> +	u32 i, reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
>>>> 	struct ixgbe_hw *hw = &adapter->hw;
>>>> 	u32 vfreta = 0;
>>>> +	unsigned int pf_pool = adapter->num_vfs;
>>>> +
>>>> +	/* Write redirection table to HW */
>>>> +	for (i = 0; i < reta_entries; i++) {
>>>> +		vfreta = (vfreta << 8) | adapter->rss_indir_tbl[i];
>>>> +		if ((i & 3) == 3)
>>>> +			IXGBE_WRITE_REG(hw, IXGBE_PFVFRETA(i >> 2, pf_pool),
>>>> +					vfreta);
>>>> +	}
>>>> +}
>>>> +
>>>> +static void ixgbe_setup_reta(struct ixgbe_adapter *adapter)
>>>> +{
>>>> +	struct ixgbe_hw *hw = &adapter->hw;
>>>> +	u32 i, j;
>>>> +	u32 reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
>>>> +	u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
>>>> +
>>>> +	/*
>>>> +	 * Program table for at least 2 queues w/ SR-IOV so that VFs can
>>>> +	 * make full use of any rings they may have.  We will use the
>>>> +	 * PSRTYPE register to control how many rings we use within the PF.
>>>> +	 */
> This one.

I saw this warning but it wasn't me who wrote this comment so it didn't 
feel right to fix it in the functional patch. U know what, I'll fix it 
in order to silence the checkpatch.

>
>>>> +	if ((adapter->flags & IXGBE_FLAG_SRIOV_ENABLED) && (rss_i < 2))
>>>> +		rss_i = 2;
>>>> +
>>>> +	/* Fill out hash function seeds */
>>>> +	for (i = 0; i < 10; i++)
>>>> +		IXGBE_WRITE_REG(hw, IXGBE_RSSRK(i), adapter->rss_key[i]);
>>>> +
>>>> +	/* Fill out redirection table */
>>>> +	memset(adapter->rss_indir_tbl, 0, sizeof(adapter->rss_indir_tbl));
>>>> +
>>>> +	for (i = 0, j = 0; i < reta_entries; i++, j++) {
>>>> +		if (j == rss_i)
>>>> +			j = 0;
>>>> +
>>>> +		adapter->rss_indir_tbl[i] = j;
>>>> +	}
>>>> +
>>>> +	ixgbe_store_reta(adapter);
>>>> +}
>>>> +
>>>> +static void ixgbe_setup_vfreta(struct ixgbe_adapter *adapter)
>>>> +{
>>>> +	struct ixgbe_hw *hw = &adapter->hw;
>>>> 	u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
>>>> 	unsigned int pf_pool = adapter->num_vfs;
>>>> 	int i, j;
>>>>
>>>> 	/* Fill out hash function seeds */
>>>> 	for (i = 0; i < 10; i++)
>>>> -		IXGBE_WRITE_REG(hw, IXGBE_PFVFRSSRK(i, pf_pool), seed[i]);
>>>> +		IXGBE_WRITE_REG(hw, IXGBE_PFVFRSSRK(i, pf_pool),
>>>> +				adapter->rss_key[i]);
>>>>
>>>> 	/* Fill out the redirection table */
>>>> 	for (i = 0, j = 0; i < 64; i++, j++) {
>>>> 		if (j == rss_i)
>>>> 			j = 0;
>>>> -		vfreta = (vfreta << 8) | j;
>>>> -		if ((i & 3) == 3)
>>>> -			IXGBE_WRITE_REG(hw, IXGBE_PFVFRETA(i >> 2, pf_pool),
>>>> -					vfreta);
>>>> +
>>>> +		adapter->rss_indir_tbl[i] = j;
>>>> 	}
>>>> +
>>>> +	ixgbe_store_vfreta(adapter);
>>>> }
>>>>
>>>> static void ixgbe_setup_mrqc(struct ixgbe_adapter *adapter)
>>>> {
>>>> 	struct ixgbe_hw *hw = &adapter->hw;
>>>> 	u32 mrqc = 0, rss_field = 0, vfmrqc = 0;
>>>> -	u32 rss_key[10];
>>>> 	u32 rxcsum;
>>>>
>>>> 	/* Disable indicating checksum in descriptor, enables RSS hash */
>>>> @@ -3354,7 +3411,7 @@ static void ixgbe_setup_mrqc(struct ixgbe_adapter *adapter)
>>>> 	if (adapter->flags2 & IXGBE_FLAG2_RSS_FIELD_IPV6_UDP)
>>>> 		rss_field |= IXGBE_MRQC_RSS_FIELD_IPV6_UDP;
>>>>
>>>> -	netdev_rss_key_fill(rss_key, sizeof(rss_key));
>>>> +	netdev_rss_key_fill(adapter->rss_key, sizeof(adapter->rss_key));
>>>> 	if ((hw->mac.type >= ixgbe_mac_X550) &&
>>>> 	    (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED)) {
>>>> 		unsigned int pf_pool = adapter->num_vfs;
>>>> @@ -3364,12 +3421,12 @@ static void ixgbe_setup_mrqc(struct ixgbe_adapter *adapter)
>>>> 		IXGBE_WRITE_REG(hw, IXGBE_MRQC, mrqc);
>>>>
>>>> 		/* Setup RSS through the VF registers */
>>>> -		ixgbe_setup_vfreta(adapter, rss_key);
>>>> +		ixgbe_setup_vfreta(adapter);
>>>> 		vfmrqc = IXGBE_MRQC_RSSEN;
>>>> 		vfmrqc |= rss_field;
>>>> 		IXGBE_WRITE_REG(hw, IXGBE_PFVFMRQC(pf_pool), vfmrqc);
>>>> 	} else {
>>>> -		ixgbe_setup_reta(adapter, rss_key);
>>>> +		ixgbe_setup_reta(adapter);
>>>> 		mrqc |= rss_field;
>>>> 		IXGBE_WRITE_REG(hw, IXGBE_MRQC, mrqc);
>>>> 	}
>>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
>>>> index c3ddc94..0c671b1 100644
>>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
>>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h>
>>>> @@ -311,6 +311,13 @@ struct ixgbe_thermal_sensor_data {
>>>> #define IXGBE_ERETA(_i)	(0x0EE80 + ((_i) * 4))  /* 96 of these (0-95) */
>>>> #define IXGBE_RSSRK(_i) (0x05C80 + ((_i) * 4))  /* 10 of these (0-9) */
>>>>
>>>> +/* maximum number of RETA entries among all devices supported by ixgbe
>>>> + * driver: currently it's x550 device in non-SRIOV mode
>>>> + */
>>>> +#define IXGBE_MAX_RETA_ENTRIES 512
>>>> +
>>>> +#define IXGBE_RSS_KEY_SIZE     40  /* size of RSS Hash Key in bytes */
>>>> +
>>> These defines should be in ixgbe.h
>> Why? These are HW related macros and IMHO they belong to ixgbe_type.h
>> with other HW related macros and not to ixgbe.h where SW related macros are.
> These are just size/limits defines and we keep many of those in ixgbe.h.

These are HW sizes/limits.

> If it happens that we need them in the files that deal strictly with HW then it probably makes sense to have them in ixgbe_type.h, but I don't see this being the case here.

That's a strange strategy to separate the values not by their nature but 
by the nature of their usage but whatever... I'll move it to ixgbe.h in v2.

>
> Thanks,
> Emil
>

[Intel-wired-lan] [PATCH net-next v1 1/2] ixgbe: Refactor the RSS configuration code.

[Alexander Duyck]

On 03/26/2015 10:56 AM, Vlad Zolotarov wrote:
> This patch is a preparation for enablement of ethtool RSS indirection table
> and hash key querying. We don't want to read registers every time the RSS info
> is queried. Therefore we will store its current content in the
> arrays in the adapter struct and will read it from there (instead of from
> registers) when requested.
>
> Will change the code that writes the indirection table and hash key into the HW registers
> to take its content from these arrays. This will also simplify the indirection
> table updating ethtool callback implementation in the future.
>
> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
> ---
>   drivers/net/ethernet/intel/ixgbe/ixgbe.h      |   2 +
>   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 143 ++++++++++++++++++--------
>   drivers/net/ethernet/intel/ixgbe/ixgbe_type.h |   7 ++
>   3 files changed, 109 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> index 5e44e48..402d182 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> @@ -766,6 +766,8 @@ struct ixgbe_adapter {
>   
>   	u8 default_up;
>   	unsigned long fwd_bitmask; /* Bitmask indicating in use pools */
> +	u8 rss_indir_tbl[IXGBE_MAX_RETA_ENTRIES];
> +	u32 rss_key[IXGBE_RSS_KEY_SIZE / sizeof(u32)];
>   };
>   
>   static inline u8 ixgbe_max_rss_indices(struct ixgbe_adapter *adapter)
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 8853d52..e3de0c9 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -3228,51 +3228,54 @@ static void ixgbe_configure_srrctl(struct ixgbe_adapter *adapter,
>   	IXGBE_WRITE_REG(hw, IXGBE_SRRCTL(reg_idx), srrctl);
>   }
>   
> -static void ixgbe_setup_reta(struct ixgbe_adapter *adapter, const u32 *seed)
> +/**
> + * Return a number of entries in the RSS indirection table
> + *
> + * @adapter: device handle
> + *
> + *  - 82598/82599/X540:     128
> + *  - X550(non-SRIOV mode): 512
> + *  - X550(SRIOV mode):     64
> + */
> +static u32 ixgbe_rss_indir_tbl_entries(struct ixgbe_adapter *adapter)
> +{
> +	if (adapter->hw.mac.type < ixgbe_mac_X550)
> +		return 128;
> +	else if (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED)
> +		return 64;
> +	else
> +		return 512;
> +}
> +
> +/**
> + * Write the RETA table to HW
> + *
> + * @adapter: device handle
> + *
> + * Write the RSS redirection table stored in adapter.rss_indir_tbl[] to HW.
> + */
> +static void ixgbe_store_reta(struct ixgbe_adapter *adapter)
>   {
> +	u32 i, reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
>   	struct ixgbe_hw *hw = &adapter->hw;
>   	u32 reta = 0;
> -	int i, j;
> -	int reta_entries = 128;
> -	u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
>   	int indices_multi;
> -
> -	/*
> -	 * Program table for at least 2 queues w/ SR-IOV so that VFs can
> -	 * make full use of any rings they may have.  We will use the
> -	 * PSRTYPE register to control how many rings we use within the PF.
> -	 */
> -	if ((adapter->flags & IXGBE_FLAG_SRIOV_ENABLED) && (rss_i < 2))
> -		rss_i = 2;
> -
> -	/* Fill out hash function seeds */
> -	for (i = 0; i < 10; i++)
> -		IXGBE_WRITE_REG(hw, IXGBE_RSSRK(i), seed[i]);
> +	u8 *indir_tbl = adapter->rss_indir_tbl;
>   
>   	/* Fill out the redirection table as follows:
> -	 * 82598: 128 (8 bit wide) entries containing pair of 4 bit RSS indices
> -	 * 82599/X540: 128 (8 bit wide) entries containing 4 bit RSS index
> -	 * X550: 512 (8 bit wide) entries containing 6 bit RSS index
> +	 *  - 82598:      8 bit wide entries containing pair of 4 bit RSS
> +	 *    indices.
> +	 *  - 82599/X540: 8 bit wide entries containing 4 bit RSS index
> +	 *  - X550:       8 bit wide entries containing 6 bit RSS index
>   	 */
>   	if (adapter->hw.mac.type == ixgbe_mac_82598EB)
>   		indices_multi = 0x11;
>   	else
>   		indices_multi = 0x1;
>   
> -	switch (adapter->hw.mac.type) {
> -	case ixgbe_mac_X550:
> -	case ixgbe_mac_X550EM_x:
> -		if (!(adapter->flags & IXGBE_FLAG_SRIOV_ENABLED))
> -			reta_entries = 512;
> -	default:
> -		break;
> -	}
> -
> -	/* Fill out redirection table */
> -	for (i = 0, j = 0; i < reta_entries; i++, j++) {
> -		if (j == rss_i)
> -			j = 0;
> -		reta = (reta << 8) | (j * indices_multi);
> +	/* Write redirection table to HW */
> +	for (i = 0; i < reta_entries; i++) {
> +		reta = (reta << 8) | (indices_multi * indir_tbl[i]);
>   		if ((i & 3) == 3) {
>   			if (i < 128)
>   				IXGBE_WRITE_REG(hw, IXGBE_RETA(i >> 2), reta);

I am pretty sure this is being written in the wrong byte order. Entry 0 
should be the lowest bits in the register, not the highest.

This loop probably needs to be rewritten so that you walk though 
processing 4 entries at a time, and generate the register starting at 
rss_indr_tbl[i + 3] and work your way down to rss_indr_tbl[i].

> @@ -3283,34 +3286,88 @@ static void ixgbe_setup_reta(struct ixgbe_adapter *adapter, const u32 *seed)
>   	}
>   }
>   
> -static void ixgbe_setup_vfreta(struct ixgbe_adapter *adapter, const u32 *seed)
> +/**
> + * Write the RETA table to HW (for x550 devices in SRIOV mode)
> + *
> + * @adapter: device handle
> + *
> + * Write the RSS redirection table stored in adapter.rss_indir_tbl[] to HW.
> + */
> +static void ixgbe_store_vfreta(struct ixgbe_adapter *adapter)
>   {
> +	u32 i, reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
>   	struct ixgbe_hw *hw = &adapter->hw;
>   	u32 vfreta = 0;
> +	unsigned int pf_pool = adapter->num_vfs;
> +
> +	/* Write redirection table to HW */
> +	for (i = 0; i < reta_entries; i++) {
> +		vfreta = (vfreta << 8) | adapter->rss_indir_tbl[i];
> +		if ((i & 3) == 3)
> +			IXGBE_WRITE_REG(hw, IXGBE_PFVFRETA(i >> 2, pf_pool),
> +					vfreta);
> +	}
> +}
> +

Same here, the byte ordering is wrong.

An alternative would be to store the RETA as a union that is both a u8 
array and a __le32 array.  Then you could write everything in the first 
pass as bytes, and when you go to write it to the registers you could 
read it using le32_to_cpu, then write the value into the register.

- Alex

Re: [PATCH net-next v1 1/2] ixgbe: Refactor the RSS configuration code.

[Alexander Duyck]

On 03/26/2015 10:56 AM, Vlad Zolotarov wrote:
> This patch is a preparation for enablement of ethtool RSS indirection table
> and hash key querying. We don't want to read registers every time the RSS info
> is queried. Therefore we will store its current content in the
> arrays in the adapter struct and will read it from there (instead of from
> registers) when requested.
>
> Will change the code that writes the indirection table and hash key into the HW registers
> to take its content from these arrays. This will also simplify the indirection
> table updating ethtool callback implementation in the future.
>
> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
> ---
>   drivers/net/ethernet/intel/ixgbe/ixgbe.h      |   2 +
>   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 143 ++++++++++++++++++--------
>   drivers/net/ethernet/intel/ixgbe/ixgbe_type.h |   7 ++
>   3 files changed, 109 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> index 5e44e48..402d182 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> @@ -766,6 +766,8 @@ struct ixgbe_adapter {
>   
>   	u8 default_up;
>   	unsigned long fwd_bitmask; /* Bitmask indicating in use pools */
> +	u8 rss_indir_tbl[IXGBE_MAX_RETA_ENTRIES];
> +	u32 rss_key[IXGBE_RSS_KEY_SIZE / sizeof(u32)];
>   };
>   
>   static inline u8 ixgbe_max_rss_indices(struct ixgbe_adapter *adapter)
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 8853d52..e3de0c9 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -3228,51 +3228,54 @@ static void ixgbe_configure_srrctl(struct ixgbe_adapter *adapter,
>   	IXGBE_WRITE_REG(hw, IXGBE_SRRCTL(reg_idx), srrctl);
>   }
>   
> -static void ixgbe_setup_reta(struct ixgbe_adapter *adapter, const u32 *seed)
> +/**
> + * Return a number of entries in the RSS indirection table
> + *
> + * @adapter: device handle
> + *
> + *  - 82598/82599/X540:     128
> + *  - X550(non-SRIOV mode): 512
> + *  - X550(SRIOV mode):     64
> + */
> +static u32 ixgbe_rss_indir_tbl_entries(struct ixgbe_adapter *adapter)
> +{
> +	if (adapter->hw.mac.type < ixgbe_mac_X550)
> +		return 128;
> +	else if (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED)
> +		return 64;
> +	else
> +		return 512;
> +}
> +
> +/**
> + * Write the RETA table to HW
> + *
> + * @adapter: device handle
> + *
> + * Write the RSS redirection table stored in adapter.rss_indir_tbl[] to HW.
> + */
> +static void ixgbe_store_reta(struct ixgbe_adapter *adapter)
>   {
> +	u32 i, reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
>   	struct ixgbe_hw *hw = &adapter->hw;
>   	u32 reta = 0;
> -	int i, j;
> -	int reta_entries = 128;
> -	u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
>   	int indices_multi;
> -
> -	/*
> -	 * Program table for at least 2 queues w/ SR-IOV so that VFs can
> -	 * make full use of any rings they may have.  We will use the
> -	 * PSRTYPE register to control how many rings we use within the PF.
> -	 */
> -	if ((adapter->flags & IXGBE_FLAG_SRIOV_ENABLED) && (rss_i < 2))
> -		rss_i = 2;
> -
> -	/* Fill out hash function seeds */
> -	for (i = 0; i < 10; i++)
> -		IXGBE_WRITE_REG(hw, IXGBE_RSSRK(i), seed[i]);
> +	u8 *indir_tbl = adapter->rss_indir_tbl;
>   
>   	/* Fill out the redirection table as follows:
> -	 * 82598: 128 (8 bit wide) entries containing pair of 4 bit RSS indices
> -	 * 82599/X540: 128 (8 bit wide) entries containing 4 bit RSS index
> -	 * X550: 512 (8 bit wide) entries containing 6 bit RSS index
> +	 *  - 82598:      8 bit wide entries containing pair of 4 bit RSS
> +	 *    indices.
> +	 *  - 82599/X540: 8 bit wide entries containing 4 bit RSS index
> +	 *  - X550:       8 bit wide entries containing 6 bit RSS index
>   	 */
>   	if (adapter->hw.mac.type == ixgbe_mac_82598EB)
>   		indices_multi = 0x11;
>   	else
>   		indices_multi = 0x1;
>   
> -	switch (adapter->hw.mac.type) {
> -	case ixgbe_mac_X550:
> -	case ixgbe_mac_X550EM_x:
> -		if (!(adapter->flags & IXGBE_FLAG_SRIOV_ENABLED))
> -			reta_entries = 512;
> -	default:
> -		break;
> -	}
> -
> -	/* Fill out redirection table */
> -	for (i = 0, j = 0; i < reta_entries; i++, j++) {
> -		if (j == rss_i)
> -			j = 0;
> -		reta = (reta << 8) | (j * indices_multi);
> +	/* Write redirection table to HW */
> +	for (i = 0; i < reta_entries; i++) {
> +		reta = (reta << 8) | (indices_multi * indir_tbl[i]);
>   		if ((i & 3) == 3) {
>   			if (i < 128)
>   				IXGBE_WRITE_REG(hw, IXGBE_RETA(i >> 2), reta);

I am pretty sure this is being written in the wrong byte order. Entry 0 
should be the lowest bits in the register, not the highest.

This loop probably needs to be rewritten so that you walk though 
processing 4 entries at a time, and generate the register starting at 
rss_indr_tbl[i + 3] and work your way down to rss_indr_tbl[i].

> @@ -3283,34 +3286,88 @@ static void ixgbe_setup_reta(struct ixgbe_adapter *adapter, const u32 *seed)
>   	}
>   }
>   
> -static void ixgbe_setup_vfreta(struct ixgbe_adapter *adapter, const u32 *seed)
> +/**
> + * Write the RETA table to HW (for x550 devices in SRIOV mode)
> + *
> + * @adapter: device handle
> + *
> + * Write the RSS redirection table stored in adapter.rss_indir_tbl[] to HW.
> + */
> +static void ixgbe_store_vfreta(struct ixgbe_adapter *adapter)
>   {
> +	u32 i, reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
>   	struct ixgbe_hw *hw = &adapter->hw;
>   	u32 vfreta = 0;
> +	unsigned int pf_pool = adapter->num_vfs;
> +
> +	/* Write redirection table to HW */
> +	for (i = 0; i < reta_entries; i++) {
> +		vfreta = (vfreta << 8) | adapter->rss_indir_tbl[i];
> +		if ((i & 3) == 3)
> +			IXGBE_WRITE_REG(hw, IXGBE_PFVFRETA(i >> 2, pf_pool),
> +					vfreta);
> +	}
> +}
> +

Same here, the byte ordering is wrong.

An alternative would be to store the RETA as a union that is both a u8 
array and a __le32 array.  Then you could write everything in the first 
pass as bytes, and when you go to write it to the registers you could 
read it using le32_to_cpu, then write the value into the register.

- Alex

[Intel-wired-lan] [PATCH net-next v1 1/2] ixgbe: Refactor the RSS configuration code.

[Vlad Zolotarov]

On 03/30/15 19:19, Alexander Duyck wrote:
>
> On 03/26/2015 10:56 AM, Vlad Zolotarov wrote:
>> This patch is a preparation for enablement of ethtool RSS indirection 
>> table
>> and hash key querying. We don't want to read registers every time the 
>> RSS info
>> is queried. Therefore we will store its current content in the
>> arrays in the adapter struct and will read it from there (instead of 
>> from
>> registers) when requested.
>>
>> Will change the code that writes the indirection table and hash key 
>> into the HW registers
>> to take its content from these arrays. This will also simplify the 
>> indirection
>> table updating ethtool callback implementation in the future.
>>
>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>> ---
>>   drivers/net/ethernet/intel/ixgbe/ixgbe.h      |   2 +
>>   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 143 
>> ++++++++++++++++++--------
>>   drivers/net/ethernet/intel/ixgbe/ixgbe_type.h |   7 ++
>>   3 files changed, 109 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h 
>> b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> index 5e44e48..402d182 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> @@ -766,6 +766,8 @@ struct ixgbe_adapter {
>>         u8 default_up;
>>       unsigned long fwd_bitmask; /* Bitmask indicating in use pools */
>> +    u8 rss_indir_tbl[IXGBE_MAX_RETA_ENTRIES];
>> +    u32 rss_key[IXGBE_RSS_KEY_SIZE / sizeof(u32)];
>>   };
>>     static inline u8 ixgbe_max_rss_indices(struct ixgbe_adapter 
>> *adapter)
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c 
>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> index 8853d52..e3de0c9 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> @@ -3228,51 +3228,54 @@ static void ixgbe_configure_srrctl(struct 
>> ixgbe_adapter *adapter,
>>       IXGBE_WRITE_REG(hw, IXGBE_SRRCTL(reg_idx), srrctl);
>>   }
>>   -static void ixgbe_setup_reta(struct ixgbe_adapter *adapter, const 
>> u32 *seed)
>> +/**
>> + * Return a number of entries in the RSS indirection table
>> + *
>> + * @adapter: device handle
>> + *
>> + *  - 82598/82599/X540:     128
>> + *  - X550(non-SRIOV mode): 512
>> + *  - X550(SRIOV mode):     64
>> + */
>> +static u32 ixgbe_rss_indir_tbl_entries(struct ixgbe_adapter *adapter)
>> +{
>> +    if (adapter->hw.mac.type < ixgbe_mac_X550)
>> +        return 128;
>> +    else if (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED)
>> +        return 64;
>> +    else
>> +        return 512;
>> +}
>> +
>> +/**
>> + * Write the RETA table to HW
>> + *
>> + * @adapter: device handle
>> + *
>> + * Write the RSS redirection table stored in adapter.rss_indir_tbl[] 
>> to HW.
>> + */
>> +static void ixgbe_store_reta(struct ixgbe_adapter *adapter)
>>   {
>> +    u32 i, reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
>>       struct ixgbe_hw *hw = &adapter->hw;
>>       u32 reta = 0;
>> -    int i, j;
>> -    int reta_entries = 128;
>> -    u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
>>       int indices_multi;
>> -
>> -    /*
>> -     * Program table for at least 2 queues w/ SR-IOV so that VFs can
>> -     * make full use of any rings they may have.  We will use the
>> -     * PSRTYPE register to control how many rings we use within the PF.
>> -     */
>> -    if ((adapter->flags & IXGBE_FLAG_SRIOV_ENABLED) && (rss_i < 2))
>> -        rss_i = 2;
>> -
>> -    /* Fill out hash function seeds */
>> -    for (i = 0; i < 10; i++)
>> -        IXGBE_WRITE_REG(hw, IXGBE_RSSRK(i), seed[i]);
>> +    u8 *indir_tbl = adapter->rss_indir_tbl;
>>         /* Fill out the redirection table as follows:
>> -     * 82598: 128 (8 bit wide) entries containing pair of 4 bit RSS 
>> indices
>> -     * 82599/X540: 128 (8 bit wide) entries containing 4 bit RSS index
>> -     * X550: 512 (8 bit wide) entries containing 6 bit RSS index
>> +     *  - 82598:      8 bit wide entries containing pair of 4 bit RSS
>> +     *    indices.
>> +     *  - 82599/X540: 8 bit wide entries containing 4 bit RSS index
>> +     *  - X550:       8 bit wide entries containing 6 bit RSS index
>>        */
>>       if (adapter->hw.mac.type == ixgbe_mac_82598EB)
>>           indices_multi = 0x11;
>>       else
>>           indices_multi = 0x1;
>>   -    switch (adapter->hw.mac.type) {
>> -    case ixgbe_mac_X550:
>> -    case ixgbe_mac_X550EM_x:
>> -        if (!(adapter->flags & IXGBE_FLAG_SRIOV_ENABLED))
>> -            reta_entries = 512;
>> -    default:
>> -        break;
>> -    }
>> -
>> -    /* Fill out redirection table */
>> -    for (i = 0, j = 0; i < reta_entries; i++, j++) {
>> -        if (j == rss_i)
>> -            j = 0;
>> -        reta = (reta << 8) | (j * indices_multi);
>> +    /* Write redirection table to HW */
>> +    for (i = 0; i < reta_entries; i++) {
>> +        reta = (reta << 8) | (indices_multi * indir_tbl[i]);
>>           if ((i & 3) == 3) {
>>               if (i < 128)
>>                   IXGBE_WRITE_REG(hw, IXGBE_RETA(i >> 2), reta);
>
> I am pretty sure this is being written in the wrong byte order. Entry 
> 0 should be the lowest bits in the register, not the highest.

The order of entries doesn't seem to be important here.

thanks,
vlad

>
> This loop probably needs to be rewritten so that you walk though 
> processing 4 entries at a time, and generate the register starting at 
> rss_indr_tbl[i + 3] and work your way down to rss_indr_tbl[i].
>
>> @@ -3283,34 +3286,88 @@ static void ixgbe_setup_reta(struct 
>> ixgbe_adapter *adapter, const u32 *seed)
>>       }
>>   }
>>   -static void ixgbe_setup_vfreta(struct ixgbe_adapter *adapter, 
>> const u32 *seed)
>> +/**
>> + * Write the RETA table to HW (for x550 devices in SRIOV mode)
>> + *
>> + * @adapter: device handle
>> + *
>> + * Write the RSS redirection table stored in adapter.rss_indir_tbl[] 
>> to HW.
>> + */
>> +static void ixgbe_store_vfreta(struct ixgbe_adapter *adapter)
>>   {
>> +    u32 i, reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
>>       struct ixgbe_hw *hw = &adapter->hw;
>>       u32 vfreta = 0;
>> +    unsigned int pf_pool = adapter->num_vfs;
>> +
>> +    /* Write redirection table to HW */
>> +    for (i = 0; i < reta_entries; i++) {
>> +        vfreta = (vfreta << 8) | adapter->rss_indir_tbl[i];
>> +        if ((i & 3) == 3)
>> +            IXGBE_WRITE_REG(hw, IXGBE_PFVFRETA(i >> 2, pf_pool),
>> +                    vfreta);
>> +    }
>> +}
>> +
>
> Same here, the byte ordering is wrong.
>
> An alternative would be to store the RETA as a union that is both a u8 
> array and a __le32 array.  Then you could write everything in the 
> first pass as bytes, and when you go to write it to the registers you 
> could read it using le32_to_cpu, then write the value into the register.
>
> - Alex

Re: [PATCH net-next v1 1/2] ixgbe: Refactor the RSS configuration code.

[Vlad Zolotarov]

On 03/30/15 19:19, Alexander Duyck wrote:
>
> On 03/26/2015 10:56 AM, Vlad Zolotarov wrote:
>> This patch is a preparation for enablement of ethtool RSS indirection 
>> table
>> and hash key querying. We don't want to read registers every time the 
>> RSS info
>> is queried. Therefore we will store its current content in the
>> arrays in the adapter struct and will read it from there (instead of 
>> from
>> registers) when requested.
>>
>> Will change the code that writes the indirection table and hash key 
>> into the HW registers
>> to take its content from these arrays. This will also simplify the 
>> indirection
>> table updating ethtool callback implementation in the future.
>>
>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>> ---
>>   drivers/net/ethernet/intel/ixgbe/ixgbe.h      |   2 +
>>   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 143 
>> ++++++++++++++++++--------
>>   drivers/net/ethernet/intel/ixgbe/ixgbe_type.h |   7 ++
>>   3 files changed, 109 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h 
>> b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> index 5e44e48..402d182 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> @@ -766,6 +766,8 @@ struct ixgbe_adapter {
>>         u8 default_up;
>>       unsigned long fwd_bitmask; /* Bitmask indicating in use pools */
>> +    u8 rss_indir_tbl[IXGBE_MAX_RETA_ENTRIES];
>> +    u32 rss_key[IXGBE_RSS_KEY_SIZE / sizeof(u32)];
>>   };
>>     static inline u8 ixgbe_max_rss_indices(struct ixgbe_adapter 
>> *adapter)
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c 
>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> index 8853d52..e3de0c9 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> @@ -3228,51 +3228,54 @@ static void ixgbe_configure_srrctl(struct 
>> ixgbe_adapter *adapter,
>>       IXGBE_WRITE_REG(hw, IXGBE_SRRCTL(reg_idx), srrctl);
>>   }
>>   -static void ixgbe_setup_reta(struct ixgbe_adapter *adapter, const 
>> u32 *seed)
>> +/**
>> + * Return a number of entries in the RSS indirection table
>> + *
>> + * @adapter: device handle
>> + *
>> + *  - 82598/82599/X540:     128
>> + *  - X550(non-SRIOV mode): 512
>> + *  - X550(SRIOV mode):     64
>> + */
>> +static u32 ixgbe_rss_indir_tbl_entries(struct ixgbe_adapter *adapter)
>> +{
>> +    if (adapter->hw.mac.type < ixgbe_mac_X550)
>> +        return 128;
>> +    else if (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED)
>> +        return 64;
>> +    else
>> +        return 512;
>> +}
>> +
>> +/**
>> + * Write the RETA table to HW
>> + *
>> + * @adapter: device handle
>> + *
>> + * Write the RSS redirection table stored in adapter.rss_indir_tbl[] 
>> to HW.
>> + */
>> +static void ixgbe_store_reta(struct ixgbe_adapter *adapter)
>>   {
>> +    u32 i, reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
>>       struct ixgbe_hw *hw = &adapter->hw;
>>       u32 reta = 0;
>> -    int i, j;
>> -    int reta_entries = 128;
>> -    u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
>>       int indices_multi;
>> -
>> -    /*
>> -     * Program table for at least 2 queues w/ SR-IOV so that VFs can
>> -     * make full use of any rings they may have.  We will use the
>> -     * PSRTYPE register to control how many rings we use within the PF.
>> -     */
>> -    if ((adapter->flags & IXGBE_FLAG_SRIOV_ENABLED) && (rss_i < 2))
>> -        rss_i = 2;
>> -
>> -    /* Fill out hash function seeds */
>> -    for (i = 0; i < 10; i++)
>> -        IXGBE_WRITE_REG(hw, IXGBE_RSSRK(i), seed[i]);
>> +    u8 *indir_tbl = adapter->rss_indir_tbl;
>>         /* Fill out the redirection table as follows:
>> -     * 82598: 128 (8 bit wide) entries containing pair of 4 bit RSS 
>> indices
>> -     * 82599/X540: 128 (8 bit wide) entries containing 4 bit RSS index
>> -     * X550: 512 (8 bit wide) entries containing 6 bit RSS index
>> +     *  - 82598:      8 bit wide entries containing pair of 4 bit RSS
>> +     *    indices.
>> +     *  - 82599/X540: 8 bit wide entries containing 4 bit RSS index
>> +     *  - X550:       8 bit wide entries containing 6 bit RSS index
>>        */
>>       if (adapter->hw.mac.type == ixgbe_mac_82598EB)
>>           indices_multi = 0x11;
>>       else
>>           indices_multi = 0x1;
>>   -    switch (adapter->hw.mac.type) {
>> -    case ixgbe_mac_X550:
>> -    case ixgbe_mac_X550EM_x:
>> -        if (!(adapter->flags & IXGBE_FLAG_SRIOV_ENABLED))
>> -            reta_entries = 512;
>> -    default:
>> -        break;
>> -    }
>> -
>> -    /* Fill out redirection table */
>> -    for (i = 0, j = 0; i < reta_entries; i++, j++) {
>> -        if (j == rss_i)
>> -            j = 0;
>> -        reta = (reta << 8) | (j * indices_multi);
>> +    /* Write redirection table to HW */
>> +    for (i = 0; i < reta_entries; i++) {
>> +        reta = (reta << 8) | (indices_multi * indir_tbl[i]);
>>           if ((i & 3) == 3) {
>>               if (i < 128)
>>                   IXGBE_WRITE_REG(hw, IXGBE_RETA(i >> 2), reta);
>
> I am pretty sure this is being written in the wrong byte order. Entry 
> 0 should be the lowest bits in the register, not the highest.

The order of entries doesn't seem to be important here.

thanks,
vlad

>
> This loop probably needs to be rewritten so that you walk though 
> processing 4 entries at a time, and generate the register starting at 
> rss_indr_tbl[i + 3] and work your way down to rss_indr_tbl[i].
>
>> @@ -3283,34 +3286,88 @@ static void ixgbe_setup_reta(struct 
>> ixgbe_adapter *adapter, const u32 *seed)
>>       }
>>   }
>>   -static void ixgbe_setup_vfreta(struct ixgbe_adapter *adapter, 
>> const u32 *seed)
>> +/**
>> + * Write the RETA table to HW (for x550 devices in SRIOV mode)
>> + *
>> + * @adapter: device handle
>> + *
>> + * Write the RSS redirection table stored in adapter.rss_indir_tbl[] 
>> to HW.
>> + */
>> +static void ixgbe_store_vfreta(struct ixgbe_adapter *adapter)
>>   {
>> +    u32 i, reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
>>       struct ixgbe_hw *hw = &adapter->hw;
>>       u32 vfreta = 0;
>> +    unsigned int pf_pool = adapter->num_vfs;
>> +
>> +    /* Write redirection table to HW */
>> +    for (i = 0; i < reta_entries; i++) {
>> +        vfreta = (vfreta << 8) | adapter->rss_indir_tbl[i];
>> +        if ((i & 3) == 3)
>> +            IXGBE_WRITE_REG(hw, IXGBE_PFVFRETA(i >> 2, pf_pool),
>> +                    vfreta);
>> +    }
>> +}
>> +
>
> Same here, the byte ordering is wrong.
>
> An alternative would be to store the RETA as a union that is both a u8 
> array and a __le32 array.  Then you could write everything in the 
> first pass as bytes, and when you go to write it to the registers you 
> could read it using le32_to_cpu, then write the value into the register.
>
> - Alex

[Intel-wired-lan] [PATCH net-next v1 1/2] ixgbe: Refactor the RSS configuration code.

[Alexander Duyck]

On 03/30/2015 09:59 AM, Vlad Zolotarov wrote:
>
>
> On 03/30/15 19:19, Alexander Duyck wrote:
>>
>> On 03/26/2015 10:56 AM, Vlad Zolotarov wrote:
>>> This patch is a preparation for enablement of ethtool RSS
>>> indirection table
>>> and hash key querying. We don't want to read registers every time
>>> the RSS info
>>> is queried. Therefore we will store its current content in the
>>> arrays in the adapter struct and will read it from there (instead of
>>> from
>>> registers) when requested.
>>>
>>> Will change the code that writes the indirection table and hash key
>>> into the HW registers
>>> to take its content from these arrays. This will also simplify the
>>> indirection
>>> table updating ethtool callback implementation in the future.
>>>
>>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>>> ---
>>>   drivers/net/ethernet/intel/ixgbe/ixgbe.h      |   2 +
>>>   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 143
>>> ++++++++++++++++++--------
>>>   drivers/net/ethernet/intel/ixgbe/ixgbe_type.h |   7 ++
>>>   3 files changed, 109 insertions(+), 43 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>> b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>> index 5e44e48..402d182 100644
>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>> @@ -766,6 +766,8 @@ struct ixgbe_adapter {
>>>         u8 default_up;
>>>       unsigned long fwd_bitmask; /* Bitmask indicating in use pools */
>>> +    u8 rss_indir_tbl[IXGBE_MAX_RETA_ENTRIES];
>>> +    u32 rss_key[IXGBE_RSS_KEY_SIZE / sizeof(u32)];
>>>   };
>>>     static inline u8 ixgbe_max_rss_indices(struct ixgbe_adapter
>>> *adapter)
>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> index 8853d52..e3de0c9 100644
>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> @@ -3228,51 +3228,54 @@ static void ixgbe_configure_srrctl(struct
>>> ixgbe_adapter *adapter,
>>>       IXGBE_WRITE_REG(hw, IXGBE_SRRCTL(reg_idx), srrctl);
>>>   }
>>>   -static void ixgbe_setup_reta(struct ixgbe_adapter *adapter, const
>>> u32 *seed)
>>> +/**
>>> + * Return a number of entries in the RSS indirection table
>>> + *
>>> + * @adapter: device handle
>>> + *
>>> + *  - 82598/82599/X540:     128
>>> + *  - X550(non-SRIOV mode): 512
>>> + *  - X550(SRIOV mode):     64
>>> + */
>>> +static u32 ixgbe_rss_indir_tbl_entries(struct ixgbe_adapter *adapter)
>>> +{
>>> +    if (adapter->hw.mac.type < ixgbe_mac_X550)
>>> +        return 128;
>>> +    else if (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED)
>>> +        return 64;
>>> +    else
>>> +        return 512;
>>> +}
>>> +
>>> +/**
>>> + * Write the RETA table to HW
>>> + *
>>> + * @adapter: device handle
>>> + *
>>> + * Write the RSS redirection table stored in
>>> adapter.rss_indir_tbl[] to HW.
>>> + */
>>> +static void ixgbe_store_reta(struct ixgbe_adapter *adapter)
>>>   {
>>> +    u32 i, reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
>>>       struct ixgbe_hw *hw = &adapter->hw;
>>>       u32 reta = 0;
>>> -    int i, j;
>>> -    int reta_entries = 128;
>>> -    u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
>>>       int indices_multi;
>>> -
>>> -    /*
>>> -     * Program table for at least 2 queues w/ SR-IOV so that VFs can
>>> -     * make full use of any rings they may have.  We will use the
>>> -     * PSRTYPE register to control how many rings we use within the
>>> PF.
>>> -     */
>>> -    if ((adapter->flags & IXGBE_FLAG_SRIOV_ENABLED) && (rss_i < 2))
>>> -        rss_i = 2;
>>> -
>>> -    /* Fill out hash function seeds */
>>> -    for (i = 0; i < 10; i++)
>>> -        IXGBE_WRITE_REG(hw, IXGBE_RSSRK(i), seed[i]);
>>> +    u8 *indir_tbl = adapter->rss_indir_tbl;
>>>         /* Fill out the redirection table as follows:
>>> -     * 82598: 128 (8 bit wide) entries containing pair of 4 bit RSS
>>> indices
>>> -     * 82599/X540: 128 (8 bit wide) entries containing 4 bit RSS index
>>> -     * X550: 512 (8 bit wide) entries containing 6 bit RSS index
>>> +     *  - 82598:      8 bit wide entries containing pair of 4 bit RSS
>>> +     *    indices.
>>> +     *  - 82599/X540: 8 bit wide entries containing 4 bit RSS index
>>> +     *  - X550:       8 bit wide entries containing 6 bit RSS index
>>>        */
>>>       if (adapter->hw.mac.type == ixgbe_mac_82598EB)
>>>           indices_multi = 0x11;
>>>       else
>>>           indices_multi = 0x1;
>>>   -    switch (adapter->hw.mac.type) {
>>> -    case ixgbe_mac_X550:
>>> -    case ixgbe_mac_X550EM_x:
>>> -        if (!(adapter->flags & IXGBE_FLAG_SRIOV_ENABLED))
>>> -            reta_entries = 512;
>>> -    default:
>>> -        break;
>>> -    }
>>> -
>>> -    /* Fill out redirection table */
>>> -    for (i = 0, j = 0; i < reta_entries; i++, j++) {
>>> -        if (j == rss_i)
>>> -            j = 0;
>>> -        reta = (reta << 8) | (j * indices_multi);
>>> +    /* Write redirection table to HW */
>>> +    for (i = 0; i < reta_entries; i++) {
>>> +        reta = (reta << 8) | (indices_multi * indir_tbl[i]);
>>>           if ((i & 3) == 3) {
>>>               if (i < 128)
>>>                   IXGBE_WRITE_REG(hw, IXGBE_RETA(i >> 2), reta);
>>
>> I am pretty sure this is being written in the wrong byte order. Entry
>> 0 should be the lowest bits in the register, not the highest.
>
> The order of entries doesn't seem to be important here.
>
> thanks,
> vlad

Huh?  It is populating the register as (0 << 24) | (1 << 16) | (2 << 8)
| (3 << 0) which means the 3rd entry is populating the entry belonging
to what should be entry 0.  This is fine as long as we aren't reporting
it or allowing changes.  However, now that it is being reported via your
changes it matters.  Otherwise if someone were to take the key and the
redirection table and do the calculations themselves the results
provided by the hardware would appear to be wrong.

In order to get the ordering right we need to either byteswap the reta
value before we write it, or generate the value in the correct order
which should be (3 << 24) | (2 << 16) | (1 << 8) | 0.

- Alex

Re: [Intel-wired-lan] [PATCH net-next v1 1/2] ixgbe: Refactor the RSS configuration code.

[Alexander Duyck]

On 03/30/2015 09:59 AM, Vlad Zolotarov wrote:
>
>
> On 03/30/15 19:19, Alexander Duyck wrote:
>>
>> On 03/26/2015 10:56 AM, Vlad Zolotarov wrote:
>>> This patch is a preparation for enablement of ethtool RSS
>>> indirection table
>>> and hash key querying. We don't want to read registers every time
>>> the RSS info
>>> is queried. Therefore we will store its current content in the
>>> arrays in the adapter struct and will read it from there (instead of
>>> from
>>> registers) when requested.
>>>
>>> Will change the code that writes the indirection table and hash key
>>> into the HW registers
>>> to take its content from these arrays. This will also simplify the
>>> indirection
>>> table updating ethtool callback implementation in the future.
>>>
>>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>>> ---
>>>   drivers/net/ethernet/intel/ixgbe/ixgbe.h      |   2 +
>>>   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 143
>>> ++++++++++++++++++--------
>>>   drivers/net/ethernet/intel/ixgbe/ixgbe_type.h |   7 ++
>>>   3 files changed, 109 insertions(+), 43 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>> b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>> index 5e44e48..402d182 100644
>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>> @@ -766,6 +766,8 @@ struct ixgbe_adapter {
>>>         u8 default_up;
>>>       unsigned long fwd_bitmask; /* Bitmask indicating in use pools */
>>> +    u8 rss_indir_tbl[IXGBE_MAX_RETA_ENTRIES];
>>> +    u32 rss_key[IXGBE_RSS_KEY_SIZE / sizeof(u32)];
>>>   };
>>>     static inline u8 ixgbe_max_rss_indices(struct ixgbe_adapter
>>> *adapter)
>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> index 8853d52..e3de0c9 100644
>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> @@ -3228,51 +3228,54 @@ static void ixgbe_configure_srrctl(struct
>>> ixgbe_adapter *adapter,
>>>       IXGBE_WRITE_REG(hw, IXGBE_SRRCTL(reg_idx), srrctl);
>>>   }
>>>   -static void ixgbe_setup_reta(struct ixgbe_adapter *adapter, const
>>> u32 *seed)
>>> +/**
>>> + * Return a number of entries in the RSS indirection table
>>> + *
>>> + * @adapter: device handle
>>> + *
>>> + *  - 82598/82599/X540:     128
>>> + *  - X550(non-SRIOV mode): 512
>>> + *  - X550(SRIOV mode):     64
>>> + */
>>> +static u32 ixgbe_rss_indir_tbl_entries(struct ixgbe_adapter *adapter)
>>> +{
>>> +    if (adapter->hw.mac.type < ixgbe_mac_X550)
>>> +        return 128;
>>> +    else if (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED)
>>> +        return 64;
>>> +    else
>>> +        return 512;
>>> +}
>>> +
>>> +/**
>>> + * Write the RETA table to HW
>>> + *
>>> + * @adapter: device handle
>>> + *
>>> + * Write the RSS redirection table stored in
>>> adapter.rss_indir_tbl[] to HW.
>>> + */
>>> +static void ixgbe_store_reta(struct ixgbe_adapter *adapter)
>>>   {
>>> +    u32 i, reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
>>>       struct ixgbe_hw *hw = &adapter->hw;
>>>       u32 reta = 0;
>>> -    int i, j;
>>> -    int reta_entries = 128;
>>> -    u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
>>>       int indices_multi;
>>> -
>>> -    /*
>>> -     * Program table for at least 2 queues w/ SR-IOV so that VFs can
>>> -     * make full use of any rings they may have.  We will use the
>>> -     * PSRTYPE register to control how many rings we use within the
>>> PF.
>>> -     */
>>> -    if ((adapter->flags & IXGBE_FLAG_SRIOV_ENABLED) && (rss_i < 2))
>>> -        rss_i = 2;
>>> -
>>> -    /* Fill out hash function seeds */
>>> -    for (i = 0; i < 10; i++)
>>> -        IXGBE_WRITE_REG(hw, IXGBE_RSSRK(i), seed[i]);
>>> +    u8 *indir_tbl = adapter->rss_indir_tbl;
>>>         /* Fill out the redirection table as follows:
>>> -     * 82598: 128 (8 bit wide) entries containing pair of 4 bit RSS
>>> indices
>>> -     * 82599/X540: 128 (8 bit wide) entries containing 4 bit RSS index
>>> -     * X550: 512 (8 bit wide) entries containing 6 bit RSS index
>>> +     *  - 82598:      8 bit wide entries containing pair of 4 bit RSS
>>> +     *    indices.
>>> +     *  - 82599/X540: 8 bit wide entries containing 4 bit RSS index
>>> +     *  - X550:       8 bit wide entries containing 6 bit RSS index
>>>        */
>>>       if (adapter->hw.mac.type == ixgbe_mac_82598EB)
>>>           indices_multi = 0x11;
>>>       else
>>>           indices_multi = 0x1;
>>>   -    switch (adapter->hw.mac.type) {
>>> -    case ixgbe_mac_X550:
>>> -    case ixgbe_mac_X550EM_x:
>>> -        if (!(adapter->flags & IXGBE_FLAG_SRIOV_ENABLED))
>>> -            reta_entries = 512;
>>> -    default:
>>> -        break;
>>> -    }
>>> -
>>> -    /* Fill out redirection table */
>>> -    for (i = 0, j = 0; i < reta_entries; i++, j++) {
>>> -        if (j == rss_i)
>>> -            j = 0;
>>> -        reta = (reta << 8) | (j * indices_multi);
>>> +    /* Write redirection table to HW */
>>> +    for (i = 0; i < reta_entries; i++) {
>>> +        reta = (reta << 8) | (indices_multi * indir_tbl[i]);
>>>           if ((i & 3) == 3) {
>>>               if (i < 128)
>>>                   IXGBE_WRITE_REG(hw, IXGBE_RETA(i >> 2), reta);
>>
>> I am pretty sure this is being written in the wrong byte order. Entry
>> 0 should be the lowest bits in the register, not the highest.
>
> The order of entries doesn't seem to be important here.
>
> thanks,
> vlad

Huh?  It is populating the register as (0 << 24) | (1 << 16) | (2 << 8)
| (3 << 0) which means the 3rd entry is populating the entry belonging
to what should be entry 0.  This is fine as long as we aren't reporting
it or allowing changes.  However, now that it is being reported via your
changes it matters.  Otherwise if someone were to take the key and the
redirection table and do the calculations themselves the results
provided by the hardware would appear to be wrong.

In order to get the ordering right we need to either byteswap the reta
value before we write it, or generate the value in the correct order
which should be (3 << 24) | (2 << 16) | (1 << 8) | 0.

- Alex

[Intel-wired-lan] [PATCH net-next v1 1/2] ixgbe: Refactor the RSS configuration code.

[Vlad Zolotarov]

On 03/30/15 20:04, Alexander Duyck wrote:
> On 03/30/2015 09:59 AM, Vlad Zolotarov wrote:
>>
>> On 03/30/15 19:19, Alexander Duyck wrote:
>>> On 03/26/2015 10:56 AM, Vlad Zolotarov wrote:
>>>> This patch is a preparation for enablement of ethtool RSS
>>>> indirection table
>>>> and hash key querying. We don't want to read registers every time
>>>> the RSS info
>>>> is queried. Therefore we will store its current content in the
>>>> arrays in the adapter struct and will read it from there (instead of
>>>> from
>>>> registers) when requested.
>>>>
>>>> Will change the code that writes the indirection table and hash key
>>>> into the HW registers
>>>> to take its content from these arrays. This will also simplify the
>>>> indirection
>>>> table updating ethtool callback implementation in the future.
>>>>
>>>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>>>> ---
>>>>    drivers/net/ethernet/intel/ixgbe/ixgbe.h      |   2 +
>>>>    drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 143
>>>> ++++++++++++++++++--------
>>>>    drivers/net/ethernet/intel/ixgbe/ixgbe_type.h |   7 ++
>>>>    3 files changed, 109 insertions(+), 43 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>>> b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>>> index 5e44e48..402d182 100644
>>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>>> @@ -766,6 +766,8 @@ struct ixgbe_adapter {
>>>>          u8 default_up;
>>>>        unsigned long fwd_bitmask; /* Bitmask indicating in use pools */
>>>> +    u8 rss_indir_tbl[IXGBE_MAX_RETA_ENTRIES];
>>>> +    u32 rss_key[IXGBE_RSS_KEY_SIZE / sizeof(u32)];
>>>>    };
>>>>      static inline u8 ixgbe_max_rss_indices(struct ixgbe_adapter
>>>> *adapter)
>>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>> index 8853d52..e3de0c9 100644
>>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>> @@ -3228,51 +3228,54 @@ static void ixgbe_configure_srrctl(struct
>>>> ixgbe_adapter *adapter,
>>>>        IXGBE_WRITE_REG(hw, IXGBE_SRRCTL(reg_idx), srrctl);
>>>>    }
>>>>    -static void ixgbe_setup_reta(struct ixgbe_adapter *adapter, const
>>>> u32 *seed)
>>>> +/**
>>>> + * Return a number of entries in the RSS indirection table
>>>> + *
>>>> + * @adapter: device handle
>>>> + *
>>>> + *  - 82598/82599/X540:     128
>>>> + *  - X550(non-SRIOV mode): 512
>>>> + *  - X550(SRIOV mode):     64
>>>> + */
>>>> +static u32 ixgbe_rss_indir_tbl_entries(struct ixgbe_adapter *adapter)
>>>> +{
>>>> +    if (adapter->hw.mac.type < ixgbe_mac_X550)
>>>> +        return 128;
>>>> +    else if (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED)
>>>> +        return 64;
>>>> +    else
>>>> +        return 512;
>>>> +}
>>>> +
>>>> +/**
>>>> + * Write the RETA table to HW
>>>> + *
>>>> + * @adapter: device handle
>>>> + *
>>>> + * Write the RSS redirection table stored in
>>>> adapter.rss_indir_tbl[] to HW.
>>>> + */
>>>> +static void ixgbe_store_reta(struct ixgbe_adapter *adapter)
>>>>    {
>>>> +    u32 i, reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
>>>>        struct ixgbe_hw *hw = &adapter->hw;
>>>>        u32 reta = 0;
>>>> -    int i, j;
>>>> -    int reta_entries = 128;
>>>> -    u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
>>>>        int indices_multi;
>>>> -
>>>> -    /*
>>>> -     * Program table for at least 2 queues w/ SR-IOV so that VFs can
>>>> -     * make full use of any rings they may have.  We will use the
>>>> -     * PSRTYPE register to control how many rings we use within the
>>>> PF.
>>>> -     */
>>>> -    if ((adapter->flags & IXGBE_FLAG_SRIOV_ENABLED) && (rss_i < 2))
>>>> -        rss_i = 2;
>>>> -
>>>> -    /* Fill out hash function seeds */
>>>> -    for (i = 0; i < 10; i++)
>>>> -        IXGBE_WRITE_REG(hw, IXGBE_RSSRK(i), seed[i]);
>>>> +    u8 *indir_tbl = adapter->rss_indir_tbl;
>>>>          /* Fill out the redirection table as follows:
>>>> -     * 82598: 128 (8 bit wide) entries containing pair of 4 bit RSS
>>>> indices
>>>> -     * 82599/X540: 128 (8 bit wide) entries containing 4 bit RSS index
>>>> -     * X550: 512 (8 bit wide) entries containing 6 bit RSS index
>>>> +     *  - 82598:      8 bit wide entries containing pair of 4 bit RSS
>>>> +     *    indices.
>>>> +     *  - 82599/X540: 8 bit wide entries containing 4 bit RSS index
>>>> +     *  - X550:       8 bit wide entries containing 6 bit RSS index
>>>>         */
>>>>        if (adapter->hw.mac.type == ixgbe_mac_82598EB)
>>>>            indices_multi = 0x11;
>>>>        else
>>>>            indices_multi = 0x1;
>>>>    -    switch (adapter->hw.mac.type) {
>>>> -    case ixgbe_mac_X550:
>>>> -    case ixgbe_mac_X550EM_x:
>>>> -        if (!(adapter->flags & IXGBE_FLAG_SRIOV_ENABLED))
>>>> -            reta_entries = 512;
>>>> -    default:
>>>> -        break;
>>>> -    }
>>>> -
>>>> -    /* Fill out redirection table */
>>>> -    for (i = 0, j = 0; i < reta_entries; i++, j++) {
>>>> -        if (j == rss_i)
>>>> -            j = 0;
>>>> -        reta = (reta << 8) | (j * indices_multi);
>>>> +    /* Write redirection table to HW */
>>>> +    for (i = 0; i < reta_entries; i++) {
>>>> +        reta = (reta << 8) | (indices_multi * indir_tbl[i]);
>>>>            if ((i & 3) == 3) {
>>>>                if (i < 128)
>>>>                    IXGBE_WRITE_REG(hw, IXGBE_RETA(i >> 2), reta);
>>> I am pretty sure this is being written in the wrong byte order. Entry
>>> 0 should be the lowest bits in the register, not the highest.
>> The order of entries doesn't seem to be important here.
>>
>> thanks,
>> vlad
> Huh?  It is populating the register as (0 << 24) | (1 << 16) | (2 << 8)
> | (3 << 0) which means the 3rd entry is populating the entry belonging
> to what should be entry 0.  This is fine as long as we aren't reporting
> it or allowing changes.  However, now that it is being reported via your
> changes it matters.  Otherwise if someone were to take the key and the
> redirection table and do the calculations themselves the results
> provided by the hardware would appear to be wrong.

Hmmm. Right. No prob. I'll biteswap32 them before writing.

>
> In order to get the ordering right we need to either byteswap the reta
> value before we write it, or generate the value in the correct order
> which should be (3 << 24) | (2 << 16) | (1 << 8) | 0.
>
> - Alex

Re: [Intel-wired-lan] [PATCH net-next v1 1/2] ixgbe: Refactor the RSS configuration code.

[Vlad Zolotarov]

On 03/30/15 20:04, Alexander Duyck wrote:
> On 03/30/2015 09:59 AM, Vlad Zolotarov wrote:
>>
>> On 03/30/15 19:19, Alexander Duyck wrote:
>>> On 03/26/2015 10:56 AM, Vlad Zolotarov wrote:
>>>> This patch is a preparation for enablement of ethtool RSS
>>>> indirection table
>>>> and hash key querying. We don't want to read registers every time
>>>> the RSS info
>>>> is queried. Therefore we will store its current content in the
>>>> arrays in the adapter struct and will read it from there (instead of
>>>> from
>>>> registers) when requested.
>>>>
>>>> Will change the code that writes the indirection table and hash key
>>>> into the HW registers
>>>> to take its content from these arrays. This will also simplify the
>>>> indirection
>>>> table updating ethtool callback implementation in the future.
>>>>
>>>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>>>> ---
>>>>    drivers/net/ethernet/intel/ixgbe/ixgbe.h      |   2 +
>>>>    drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 143
>>>> ++++++++++++++++++--------
>>>>    drivers/net/ethernet/intel/ixgbe/ixgbe_type.h |   7 ++
>>>>    3 files changed, 109 insertions(+), 43 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>>> b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>>> index 5e44e48..402d182 100644
>>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>>> @@ -766,6 +766,8 @@ struct ixgbe_adapter {
>>>>          u8 default_up;
>>>>        unsigned long fwd_bitmask; /* Bitmask indicating in use pools */
>>>> +    u8 rss_indir_tbl[IXGBE_MAX_RETA_ENTRIES];
>>>> +    u32 rss_key[IXGBE_RSS_KEY_SIZE / sizeof(u32)];
>>>>    };
>>>>      static inline u8 ixgbe_max_rss_indices(struct ixgbe_adapter
>>>> *adapter)
>>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>> index 8853d52..e3de0c9 100644
>>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>> @@ -3228,51 +3228,54 @@ static void ixgbe_configure_srrctl(struct
>>>> ixgbe_adapter *adapter,
>>>>        IXGBE_WRITE_REG(hw, IXGBE_SRRCTL(reg_idx), srrctl);
>>>>    }
>>>>    -static void ixgbe_setup_reta(struct ixgbe_adapter *adapter, const
>>>> u32 *seed)
>>>> +/**
>>>> + * Return a number of entries in the RSS indirection table
>>>> + *
>>>> + * @adapter: device handle
>>>> + *
>>>> + *  - 82598/82599/X540:     128
>>>> + *  - X550(non-SRIOV mode): 512
>>>> + *  - X550(SRIOV mode):     64
>>>> + */
>>>> +static u32 ixgbe_rss_indir_tbl_entries(struct ixgbe_adapter *adapter)
>>>> +{
>>>> +    if (adapter->hw.mac.type < ixgbe_mac_X550)
>>>> +        return 128;
>>>> +    else if (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED)
>>>> +        return 64;
>>>> +    else
>>>> +        return 512;
>>>> +}
>>>> +
>>>> +/**
>>>> + * Write the RETA table to HW
>>>> + *
>>>> + * @adapter: device handle
>>>> + *
>>>> + * Write the RSS redirection table stored in
>>>> adapter.rss_indir_tbl[] to HW.
>>>> + */
>>>> +static void ixgbe_store_reta(struct ixgbe_adapter *adapter)
>>>>    {
>>>> +    u32 i, reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
>>>>        struct ixgbe_hw *hw = &adapter->hw;
>>>>        u32 reta = 0;
>>>> -    int i, j;
>>>> -    int reta_entries = 128;
>>>> -    u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
>>>>        int indices_multi;
>>>> -
>>>> -    /*
>>>> -     * Program table for at least 2 queues w/ SR-IOV so that VFs can
>>>> -     * make full use of any rings they may have.  We will use the
>>>> -     * PSRTYPE register to control how many rings we use within the
>>>> PF.
>>>> -     */
>>>> -    if ((adapter->flags & IXGBE_FLAG_SRIOV_ENABLED) && (rss_i < 2))
>>>> -        rss_i = 2;
>>>> -
>>>> -    /* Fill out hash function seeds */
>>>> -    for (i = 0; i < 10; i++)
>>>> -        IXGBE_WRITE_REG(hw, IXGBE_RSSRK(i), seed[i]);
>>>> +    u8 *indir_tbl = adapter->rss_indir_tbl;
>>>>          /* Fill out the redirection table as follows:
>>>> -     * 82598: 128 (8 bit wide) entries containing pair of 4 bit RSS
>>>> indices
>>>> -     * 82599/X540: 128 (8 bit wide) entries containing 4 bit RSS index
>>>> -     * X550: 512 (8 bit wide) entries containing 6 bit RSS index
>>>> +     *  - 82598:      8 bit wide entries containing pair of 4 bit RSS
>>>> +     *    indices.
>>>> +     *  - 82599/X540: 8 bit wide entries containing 4 bit RSS index
>>>> +     *  - X550:       8 bit wide entries containing 6 bit RSS index
>>>>         */
>>>>        if (adapter->hw.mac.type == ixgbe_mac_82598EB)
>>>>            indices_multi = 0x11;
>>>>        else
>>>>            indices_multi = 0x1;
>>>>    -    switch (adapter->hw.mac.type) {
>>>> -    case ixgbe_mac_X550:
>>>> -    case ixgbe_mac_X550EM_x:
>>>> -        if (!(adapter->flags & IXGBE_FLAG_SRIOV_ENABLED))
>>>> -            reta_entries = 512;
>>>> -    default:
>>>> -        break;
>>>> -    }
>>>> -
>>>> -    /* Fill out redirection table */
>>>> -    for (i = 0, j = 0; i < reta_entries; i++, j++) {
>>>> -        if (j == rss_i)
>>>> -            j = 0;
>>>> -        reta = (reta << 8) | (j * indices_multi);
>>>> +    /* Write redirection table to HW */
>>>> +    for (i = 0; i < reta_entries; i++) {
>>>> +        reta = (reta << 8) | (indices_multi * indir_tbl[i]);
>>>>            if ((i & 3) == 3) {
>>>>                if (i < 128)
>>>>                    IXGBE_WRITE_REG(hw, IXGBE_RETA(i >> 2), reta);
>>> I am pretty sure this is being written in the wrong byte order. Entry
>>> 0 should be the lowest bits in the register, not the highest.
>> The order of entries doesn't seem to be important here.
>>
>> thanks,
>> vlad
> Huh?  It is populating the register as (0 << 24) | (1 << 16) | (2 << 8)
> | (3 << 0) which means the 3rd entry is populating the entry belonging
> to what should be entry 0.  This is fine as long as we aren't reporting
> it or allowing changes.  However, now that it is being reported via your
> changes it matters.  Otherwise if someone were to take the key and the
> redirection table and do the calculations themselves the results
> provided by the hardware would appear to be wrong.

Hmmm. Right. No prob. I'll biteswap32 them before writing.

>
> In order to get the ordering right we need to either byteswap the reta
> value before we write it, or generate the value in the correct order
> which should be (3 << 24) | (2 << 16) | (1 << 8) | 0.
>
> - Alex

[Intel-wired-lan] [PATCH net-next v1 2/2] ixgbe: Add the appropriate ethtool ops to query RSS indirection table and key

[Vlad Zolotarov]

Added get_rxfh_indir_size, get_rxfh_key_size and get_rxfh ethtool_ops callbacks
implementations.

This enables the ethtool's "-x" and "--show-rxfh[-indir]" options.

Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe.h         |  1 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 42 ++++++++++++++++++++++++
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c    |  2 +-
 3 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index 402d182..b17bbef 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -957,4 +957,5 @@ void ixgbe_sriov_reinit(struct ixgbe_adapter *adapter);
 netdev_tx_t ixgbe_xmit_frame_ring(struct sk_buff *skb,
 				  struct ixgbe_adapter *adapter,
 				  struct ixgbe_ring *tx_ring);
+u32 ixgbe_rss_indir_tbl_entries(struct ixgbe_adapter *adapter);
 #endif /* _IXGBE_H_ */
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
index 02ffb30..a4423ea 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
@@ -2853,6 +2853,45 @@ static int ixgbe_set_rxnfc(struct net_device *dev, struct ethtool_rxnfc *cmd)
 	return ret;
 }
 
+static u32 ixgbe_get_rxfh_key_size(struct net_device *netdev)
+{
+	struct ixgbe_adapter *adapter = netdev_priv(netdev);
+
+	return sizeof(adapter->rss_key);
+}
+
+static u32 ixgbe_rss_indir_size(struct net_device *netdev)
+{
+	struct ixgbe_adapter *adapter = netdev_priv(netdev);
+
+	return ixgbe_rss_indir_tbl_entries(adapter);
+}
+
+static void ixgbe_get_reta(struct ixgbe_adapter *adapter, u32 *indir)
+{
+	int i, reta_size = ixgbe_rss_indir_tbl_entries(adapter);
+
+	for (i = 0; i < reta_size; i++)
+		indir[i] = adapter->rss_indir_tbl[i];
+}
+
+static int ixgbe_get_rxfh(struct net_device *netdev, u32 *indir, u8 *key,
+			    u8 *hfunc)
+{
+	struct ixgbe_adapter *adapter = netdev_priv(netdev);
+
+	if (hfunc)
+		*hfunc = ETH_RSS_HASH_TOP;
+
+	if (indir)
+		ixgbe_get_reta(adapter, indir);
+
+	if (key)
+		memcpy(key, adapter->rss_key, ixgbe_get_rxfh_key_size(netdev));
+
+	return 0;
+}
+
 static int ixgbe_get_ts_info(struct net_device *dev,
 			     struct ethtool_ts_info *info)
 {
@@ -3110,6 +3149,9 @@ static const struct ethtool_ops ixgbe_ethtool_ops = {
 	.set_coalesce           = ixgbe_set_coalesce,
 	.get_rxnfc		= ixgbe_get_rxnfc,
 	.set_rxnfc		= ixgbe_set_rxnfc,
+	.get_rxfh_indir_size	= ixgbe_rss_indir_size,
+	.get_rxfh_key_size	= ixgbe_get_rxfh_key_size,
+	.get_rxfh		= ixgbe_get_rxfh,
 	.get_channels		= ixgbe_get_channels,
 	.set_channels		= ixgbe_set_channels,
 	.get_ts_info		= ixgbe_get_ts_info,
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index e3de0c9..a0db522 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -3237,7 +3237,7 @@ static void ixgbe_configure_srrctl(struct ixgbe_adapter *adapter,
  *  - X550(non-SRIOV mode): 512
  *  - X550(SRIOV mode):     64
  */
-static u32 ixgbe_rss_indir_tbl_entries(struct ixgbe_adapter *adapter)
+u32 ixgbe_rss_indir_tbl_entries(struct ixgbe_adapter *adapter)
 {
 	if (adapter->hw.mac.type < ixgbe_mac_X550)
 		return 128;
-- 
2.1.0

[PATCH net-next v1 2/2] ixgbe: Add the appropriate ethtool ops to query RSS indirection table and key

[Vlad Zolotarov]

Added get_rxfh_indir_size, get_rxfh_key_size and get_rxfh ethtool_ops callbacks
implementations.

This enables the ethtool's "-x" and "--show-rxfh[-indir]" options.

Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe.h         |  1 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 42 ++++++++++++++++++++++++
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c    |  2 +-
 3 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index 402d182..b17bbef 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -957,4 +957,5 @@ void ixgbe_sriov_reinit(struct ixgbe_adapter *adapter);
 netdev_tx_t ixgbe_xmit_frame_ring(struct sk_buff *skb,
 				  struct ixgbe_adapter *adapter,
 				  struct ixgbe_ring *tx_ring);
+u32 ixgbe_rss_indir_tbl_entries(struct ixgbe_adapter *adapter);
 #endif /* _IXGBE_H_ */
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
index 02ffb30..a4423ea 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
@@ -2853,6 +2853,45 @@ static int ixgbe_set_rxnfc(struct net_device *dev, struct ethtool_rxnfc *cmd)
 	return ret;
 }
 
+static u32 ixgbe_get_rxfh_key_size(struct net_device *netdev)
+{
+	struct ixgbe_adapter *adapter = netdev_priv(netdev);
+
+	return sizeof(adapter->rss_key);
+}
+
+static u32 ixgbe_rss_indir_size(struct net_device *netdev)
+{
+	struct ixgbe_adapter *adapter = netdev_priv(netdev);
+
+	return ixgbe_rss_indir_tbl_entries(adapter);
+}
+
+static void ixgbe_get_reta(struct ixgbe_adapter *adapter, u32 *indir)
+{
+	int i, reta_size = ixgbe_rss_indir_tbl_entries(adapter);
+
+	for (i = 0; i < reta_size; i++)
+		indir[i] = adapter->rss_indir_tbl[i];
+}
+
+static int ixgbe_get_rxfh(struct net_device *netdev, u32 *indir, u8 *key,
+			    u8 *hfunc)
+{
+	struct ixgbe_adapter *adapter = netdev_priv(netdev);
+
+	if (hfunc)
+		*hfunc = ETH_RSS_HASH_TOP;
+
+	if (indir)
+		ixgbe_get_reta(adapter, indir);
+
+	if (key)
+		memcpy(key, adapter->rss_key, ixgbe_get_rxfh_key_size(netdev));
+
+	return 0;
+}
+
 static int ixgbe_get_ts_info(struct net_device *dev,
 			     struct ethtool_ts_info *info)
 {
@@ -3110,6 +3149,9 @@ static const struct ethtool_ops ixgbe_ethtool_ops = {
 	.set_coalesce           = ixgbe_set_coalesce,
 	.get_rxnfc		= ixgbe_get_rxnfc,
 	.set_rxnfc		= ixgbe_set_rxnfc,
+	.get_rxfh_indir_size	= ixgbe_rss_indir_size,
+	.get_rxfh_key_size	= ixgbe_get_rxfh_key_size,
+	.get_rxfh		= ixgbe_get_rxfh,
 	.get_channels		= ixgbe_get_channels,
 	.set_channels		= ixgbe_set_channels,
 	.get_ts_info		= ixgbe_get_ts_info,
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index e3de0c9..a0db522 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -3237,7 +3237,7 @@ static void ixgbe_configure_srrctl(struct ixgbe_adapter *adapter,
  *  - X550(non-SRIOV mode): 512
  *  - X550(SRIOV mode):     64
  */
-static u32 ixgbe_rss_indir_tbl_entries(struct ixgbe_adapter *adapter)
+u32 ixgbe_rss_indir_tbl_entries(struct ixgbe_adapter *adapter)
 {
 	if (adapter->hw.mac.type < ixgbe_mac_X550)
 		return 128;
-- 
2.1.0