[PATCH net-next v3 0/5]: ixgbevf: Allow querying VFs RSS indirection table and key

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

 - PF driver: Add new VF-PF channel commands.
 - VF driver: Utilize these new commands and add the corresponding
              ethtool callbacks.

New in v3:
   - Added a missing support for x550 devices.
   - Mask the indirection table values according to PSRTYPE[n].RQPL.
   - Minimized the number of added VF-PF commands.

New in v2:
   - Added a detailed description to patches 4 and 5.

New in v1 (compared to RFC):
   - Use "if-else" statement instead of a "switch-case" for a single option case.
     More specifically: in cases where the newly added API version is the only one
     allowed. We may consider using a "switch-case" back again when the list of
     allowed API versions in these specific places grows up.

Vlad Zolotarov (5):
  ixgbe: Add a RETA query command to VF-PF channel API
  ixgbevf: Add a RETA query code
  ixgbe: Add GET_RSS_KEY command to VF-PF channel commands set
  ixgbevf: Add RSS Key query code
  ixgbevf: Add the appropriate ethtool ops to query RSS indirection
    table and key

 drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h      |  10 ++
 drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c    |  91 +++++++++++++++
 drivers/net/ethernet/intel/ixgbevf/ethtool.c      |  43 +++++++
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |   4 +-
 drivers/net/ethernet/intel/ixgbevf/mbx.h          |  10 ++
 drivers/net/ethernet/intel/ixgbevf/vf.c           | 132 ++++++++++++++++++++++
 drivers/net/ethernet/intel/ixgbevf/vf.h           |   2 +
 7 files changed, 291 insertions(+), 1 deletion(-)

-- 
2.1.0

82599 and x540 VFs and PF share the same RSS redirection table (RETA). Therefore we
just return it for all VFs. x550 on the other hand provides a separate redirection
table for each VF (there is a per-pool RETA table).

For 82599 and x540 RETA table is an array of 32 registers (128 bytes) and the maximum number of
registers that may be delivered in a single VF-PF channel command is 15. Therefore
we will deliver the whole table in 3 steps: 12, 12 and 8 registers in each
step correspondingly.

For x550 VFs RETA is a 64 byte array, so we may deliver it in two steps: 12 and 4 registers
correspondingly.

Thus this patch does the following:

  - Adds a new API version (to specify a new commands set).
  - Adds the IXGBE_VF_GET_RETA command to the VF-PF commands set.

Signed-off-by: Vlad Zolotarov <redacted>
---
New in v3:
   - Pass the number of dwords and offset in RETA in the IXGBE_VF_GET_RETA request message.
     This allows to reduce the added command set to a single command.
   - Added a support for all devices supported by the ixgbe driver that have
     SR-IOV functions support: 82599, x540 and x550. The original code supported
     only 82599 and x540.
   - Added the masking of the RETA entries according to the PSRTYPE[n].RQPL
     value.

New in v1 (compared to RFC):
   - Use "if-else" statement instead of a "switch-case" for a single option case
     (in ixgbe_get_vf_reta()).
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h   |  8 ++++
 drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c | 65 ++++++++++++++++++++++++++
 2 files changed, 73 insertions(+)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h
index a5cb755..f9b5eae 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h

@@ -73,6 +73,7 @@ enum ixgbe_pfvf_api_rev {
 	ixgbe_mbox_api_10,	/* API version 1.0, linux/freebsd VF driver */
 	ixgbe_mbox_api_20,	/* API version 2.0, solaris Phase1 VF driver */
 	ixgbe_mbox_api_11,	/* API version 1.1, linux/freebsd VF driver */
+	ixgbe_mbox_api_12,	/* API version 1.2, linux/freebsd VF driver */
 	/* This value should always be last */
 	ixgbe_mbox_api_unknown,	/* indicates that API version is not known */
 };

@@ -97,6 +98,13 @@ enum ixgbe_pfvf_api_rev {
 #define IXGBE_VF_TRANS_VLAN	3	/* Indication of port vlan */
 #define IXGBE_VF_DEF_QUEUE	4	/* Default queue offset */
 
+/* mailbox API, version 1.2 VF requests */
+#define IXGBE_VF_GET_RETA	0x0a	/* VF request for RETA */
+
+/* GET_RETA request data indices within the mailbox */
+#define IXGBE_VF_RETA_SZ        1	/* Number of RETA DWs to bring */
+#define IXGBE_VF_RETA_OFFSET    2	/* Offset in RETA */
+
 /* length of permanent address message returned from PF */
 #define IXGBE_VF_PERMADDR_MSG_LEN 4
 /* word in permanent address message with the current multicast type */

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
index c76ba90..d4f095d 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c

@@ -427,6 +427,7 @@ static s32 ixgbe_set_vf_lpe(struct ixgbe_adapter *adapter, u32 *msgbuf, u32 vf)
 #endif /* CONFIG_FCOE */
 		switch (adapter->vfinfo[vf].vf_api) {
 		case ixgbe_mbox_api_11:
+		case ixgbe_mbox_api_12:
 			/*
 			 * Version 1.1 supports jumbo frames on VFs if PF has
 			 * jumbo frames enabled which means legacy VFs are

@@ -894,6 +895,7 @@ static int ixgbe_negotiate_vf_api(struct ixgbe_adapter *adapter,
 	switch (api) {
 	case ixgbe_mbox_api_10:
 	case ixgbe_mbox_api_11:
+	case ixgbe_mbox_api_12:
 		adapter->vfinfo[vf].vf_api = api;
 		return 0;
 	default:

@@ -917,6 +919,7 @@ static int ixgbe_get_vf_queues(struct ixgbe_adapter *adapter,
 	switch (adapter->vfinfo[vf].vf_api) {
 	case ixgbe_mbox_api_20:
 	case ixgbe_mbox_api_11:
+	case ixgbe_mbox_api_12:
 		break;
 	default:
 		return -1;

@@ -944,6 +947,65 @@ static int ixgbe_get_vf_queues(struct ixgbe_adapter *adapter,
 	return 0;
 }
 
+static int ixgbe_get_vf_reta(struct ixgbe_adapter *adapter, u32 *msgbuf, u32 vf)
+{
+	struct ixgbe_hw *hw = &adapter->hw;
+	int i, j;
+	u32 *reta = &msgbuf[1];
+	u32 mask = 0;
+	u32 psrtype;
+	u32 reta_offset_dw = msgbuf[IXGBE_VF_RETA_OFFSET];
+	u32 dwords = msgbuf[IXGBE_VF_RETA_SZ];
+
+	/* verify the PF is supporting the correct API */
+	if (adapter->vfinfo[vf].vf_api != ixgbe_mbox_api_12)
+		return -EPERM;
+
+	psrtype = IXGBE_READ_REG(hw, IXGBE_PSRTYPE(vf));
+
+	/* The redirection table is composed 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
+	 *
+	 * PSRTYPE[n].RQPL defines if 0, 1 or 2 bits from the redirection table
+	 * value should be used.
+	 */
+
+	if ((psrtype & (1 << 29)) == (1 << 29))
+		mask = 0x01010101;
+	else if ((psrtype & (2 << 29)) == (2 << 29))
+		mask = 0x03030303;
+	else
+		mask = 0;
+
+	switch (hw->mac.type) {
+	case ixgbe_mac_82599EB:
+	case ixgbe_mac_X540:
+		/* Read the appropriate portion of RETA */
+		for (i = 0; i < dwords; i++)
+			reta[i] = IXGBE_READ_REG(hw,
+						IXGBE_RETA(i + reta_offset_dw));
+		break;
+	case ixgbe_mac_X550:
+	case ixgbe_mac_X550EM_x:
+		/* X550 has a per-VF RETA */
+		for (i = 0, j = reta_offset_dw; i < dwords; i++, j++)
+			reta[i] = IXGBE_READ_REG(hw,
+						IXGBE_PFVFRETA(j, vf));
+		break;
+	default:
+		return -1;
+
+	}
+
+	/* Mask the relevant bits */
+	for (i = 0; i < dwords; i++)
+		reta[i] &= mask;
+
+	return 0;
+}
+
 static int ixgbe_rcv_msg_from_vf(struct ixgbe_adapter *adapter, u32 vf)
 {
 	u32 mbx_size = IXGBE_VFMAILBOX_SIZE;

@@ -1000,6 +1062,9 @@ static int ixgbe_rcv_msg_from_vf(struct ixgbe_adapter *adapter, u32 vf)
 	case IXGBE_VF_GET_QUEUES:
 		retval = ixgbe_get_vf_queues(adapter, msgbuf, vf);
 		break;
+	case IXGBE_VF_GET_RETA:
+		retval = ixgbe_get_vf_reta(adapter, msgbuf, vf);
+		break;
 	default:
 		e_err(drv, "Unhandled Msg %8.8x\n", msgbuf[0]);
 		retval = IXGBE_ERR_MBX;

-- 
2.1.0

   - Added a new API version support.
   - Added the query implementation in the ixgbevf.

Signed-off-by: Vlad Zolotarov <redacted>
---
New in v3:
   - Adjusted to the new interface IXGBE_VF_GET_RETA command.
   - Added a proper support for x550 devices.

New in v1 (compared to RFC):
   - Use "if-else" statement instead of a "switch-case" for a single option case
     (in ixgbevf_get_reta()).
---
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |  4 +-
 drivers/net/ethernet/intel/ixgbevf/mbx.h          |  8 +++
 drivers/net/ethernet/intel/ixgbevf/vf.c           | 88 +++++++++++++++++++++++
 drivers/net/ethernet/intel/ixgbevf/vf.h           |  1 +
 4 files changed, 100 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
index 62a0d8e..ba6ab61 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
+++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c

@@ -1880,7 +1880,8 @@ static void ixgbevf_init_last_counter_stats(struct ixgbevf_adapter *adapter)
 static void ixgbevf_negotiate_api(struct ixgbevf_adapter *adapter)
 {
 	struct ixgbe_hw *hw = &adapter->hw;
-	int api[] = { ixgbe_mbox_api_11,
+	int api[] = { ixgbe_mbox_api_12,
+		      ixgbe_mbox_api_11,
 		      ixgbe_mbox_api_10,
 		      ixgbe_mbox_api_unknown };
 	int err = 0, idx = 0;

@@ -3525,6 +3526,7 @@ static int ixgbevf_change_mtu(struct net_device *netdev, int new_mtu)
 
 	switch (adapter->hw.api_version) {
 	case ixgbe_mbox_api_11:
+	case ixgbe_mbox_api_12:
 		max_possible_frame = IXGBE_MAX_JUMBO_FRAME_SIZE;
 		break;
 	default:

diff --git a/drivers/net/ethernet/intel/ixgbevf/mbx.h b/drivers/net/ethernet/intel/ixgbevf/mbx.h
index 0bc3005..951a506 100644
--- a/drivers/net/ethernet/intel/ixgbevf/mbx.h
+++ b/drivers/net/ethernet/intel/ixgbevf/mbx.h

@@ -86,6 +86,7 @@ enum ixgbe_pfvf_api_rev {
 	ixgbe_mbox_api_10,	/* API version 1.0, linux/freebsd VF driver */
 	ixgbe_mbox_api_20,	/* API version 2.0, solaris Phase1 VF driver */
 	ixgbe_mbox_api_11,	/* API version 1.1, linux/freebsd VF driver */
+	ixgbe_mbox_api_12,	/* API version 1.2, linux/freebsd VF driver */
 	/* This value should always be last */
 	ixgbe_mbox_api_unknown,	/* indicates that API version is not known */
 };

@@ -110,6 +111,13 @@ enum ixgbe_pfvf_api_rev {
 #define IXGBE_VF_TRANS_VLAN	3	/* Indication of port vlan */
 #define IXGBE_VF_DEF_QUEUE	4	/* Default queue offset */
 
+/* mailbox API, version 1.2 VF requests */
+#define IXGBE_VF_GET_RETA	0x0a	/* VF request for RETA */
+
+/* GET_RETA request data indices within the mailbox */
+#define IXGBE_VF_RETA_SZ        1	/* Number of RETA DWs to bring */
+#define IXGBE_VF_RETA_OFFSET    2	/* Offset in RETA */
+
 /* length of permanent address message returned from PF */
 #define IXGBE_VF_PERMADDR_MSG_LEN 4
 /* word in permanent address message with the current multicast type */

diff --git a/drivers/net/ethernet/intel/ixgbevf/vf.c b/drivers/net/ethernet/intel/ixgbevf/vf.c
index cdb53be..cb5a4cf 100644
--- a/drivers/net/ethernet/intel/ixgbevf/vf.c
+++ b/drivers/net/ethernet/intel/ixgbevf/vf.c

@@ -258,6 +258,93 @@ static s32 ixgbevf_set_uc_addr_vf(struct ixgbe_hw *hw, u32 index, u8 *addr)
 	return ret_val;
 }
 
+static inline int _ixgbevf_get_reta(struct ixgbe_hw *hw, u32 *msgbuf,
+				    u32 *reta, u32 reta_offset_dw, u32 dwords)
+{
+	int err;
+
+	msgbuf[0]                    = IXGBE_VF_GET_RETA;
+	msgbuf[IXGBE_VF_RETA_SZ]     = dwords;
+	msgbuf[IXGBE_VF_RETA_OFFSET] = reta_offset_dw;
+
+	err = hw->mbx.ops.write_posted(hw, msgbuf, 3);
+
+	if (err)
+		return err;
+
+	err = hw->mbx.ops.read_posted(hw, msgbuf, 1 + dwords);
+
+	if (err)
+		return err;
+
+	msgbuf[0] &= ~IXGBE_VT_MSGTYPE_CTS;
+
+	/* If we didn't get an ACK there must have been
+	 * some sort of mailbox error so we should treat it
+	 * as such.
+	 */
+	if (msgbuf[0] != (IXGBE_VF_GET_RETA | IXGBE_VT_MSGTYPE_ACK))
+		return IXGBE_ERR_MBX;
+
+	memcpy(reta + reta_offset_dw, msgbuf + 1, 4 * dwords);
+
+	return 0;
+}
+
+/**
+ * ixgbevf_get_reta - get the RSS redirection table (RETA) contents.
+ * @hw: pointer to the HW structure
+ * @reta: buffer to fill with RETA contents.
+ *
+ * The "reta" buffer should be big enough to contain 32 registers.
+ *
+ * Returns: 0 on success.
+ *          if API doesn't support this operation - (-EPERM).
+ */
+int ixgbevf_get_reta(struct ixgbe_hw *hw, u32 *reta)
+{
+	int err;
+	u32 msgbuf[IXGBE_VFMAILBOX_SIZE];
+
+	/* Return an error if API doesn't RETA querying. */
+	if (hw->api_version != ixgbe_mbox_api_12)
+		return -EPERM;
+
+	/* x550 devices have a separate RETA for each VF: 64 bytes each.
+	 *
+	 * We'll get it in 2 steps due to mailbox size limitation - we can bring
+	 * up to 15 dwords every time. Therefore we'll bring 12 and 4 dwords.
+	 *
+	 * Older devices share a RETA table with the PF: 128 bytes.
+	 *
+	 * For them we do it in 3 steps. Therefore we'll bring it in 3 steps:
+	 * 12, 12 and 8 dwords in each step correspondingly.
+	 */
+
+	/* RETA[0..11] */
+	err = _ixgbevf_get_reta(hw, msgbuf, reta, 0, 12);
+	if (err)
+		return err;
+
+	if (hw->mac.type >= ixgbe_mac_X550_vf) {
+		/* RETA[12..15] */
+		err = _ixgbevf_get_reta(hw, msgbuf, reta, 12, 4);
+		if (err)
+			return err;
+
+	} else {
+		/* RETA[12..23] */
+		err = _ixgbevf_get_reta(hw, msgbuf, reta, 12, 12);
+		if (err)
+			return err;
+
+		/* RETA[24..31] */
+		err = _ixgbevf_get_reta(hw, msgbuf, reta, 24, 8);
+	}
+
+	return err;
+}
+
 /**
  *  ixgbevf_set_rar_vf - set device MAC address
  *  @hw: pointer to hardware structure

@@ -545,6 +632,7 @@ int ixgbevf_get_queues(struct ixgbe_hw *hw, unsigned int *num_tcs,
 	/* do nothing if API doesn't support ixgbevf_get_queues */
 	switch (hw->api_version) {
 	case ixgbe_mbox_api_11:
+	case ixgbe_mbox_api_12:
 		break;
 	default:
 		return 0;

diff --git a/drivers/net/ethernet/intel/ixgbevf/vf.h b/drivers/net/ethernet/intel/ixgbevf/vf.h
index 5b17242..73c1b33 100644
--- a/drivers/net/ethernet/intel/ixgbevf/vf.h
+++ b/drivers/net/ethernet/intel/ixgbevf/vf.h

@@ -208,5 +208,6 @@ void ixgbevf_rlpml_set_vf(struct ixgbe_hw *hw, u16 max_size);
 int ixgbevf_negotiate_api_version(struct ixgbe_hw *hw, int api);
 int ixgbevf_get_queues(struct ixgbe_hw *hw, unsigned int *num_tcs,
 		       unsigned int *default_tc);
+int ixgbevf_get_reta(struct ixgbe_hw *hw, u32 *reta);
 #endif /* __IXGBE_VF_H__ */
 

-- 
2.1.0

For 82599 and x540 VFs and PF share the same RSS Key. Therefore we will return
the same RSS key for all VFs.

x550 on the other hand has a separate RSS Key for every pool.

Signed-off-by: Vlad Zolotarov <redacted>
---
New in v3:
   - Added a support for x550 devices.

New in v1 (compared to RFC):
   - Use "if-else" statement instead of a "switch-case" for a single option case
     (in ixgbe_get_vf_rss_key()).
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h   |  2 ++
 drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c | 26 ++++++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h
index f9b5eae..3f14373 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h

@@ -105,6 +105,8 @@ enum ixgbe_pfvf_api_rev {
 #define IXGBE_VF_RETA_SZ        1	/* Number of RETA DWs to bring */
 #define IXGBE_VF_RETA_OFFSET    2	/* Offset in RETA */
 
+#define IXGBE_VF_GET_RSS_KEY	0x0b	/* get RSS key */
+
 /* length of permanent address message returned from PF */
 #define IXGBE_VF_PERMADDR_MSG_LEN 4
 /* word in permanent address message with the current multicast type */

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
index d4f095d..9a6a112 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c

@@ -1006,6 +1006,29 @@ static int ixgbe_get_vf_reta(struct ixgbe_adapter *adapter, u32 *msgbuf, u32 vf)
 	return 0;
 }
 
+static int ixgbe_get_vf_rss_key(struct ixgbe_adapter *adapter,
+				u32 *msgbuf, u32 vf)
+{
+	struct ixgbe_hw *hw = &adapter->hw;
+	int i;
+	u32 *rss_key = &msgbuf[1];
+
+	/* verify the PF is supporting the correct API */
+	if (adapter->vfinfo[vf].vf_api != ixgbe_mbox_api_12)
+		return -EPERM;
+
+	/* Read the RSS KEY */
+	if (hw->mac.type >= ixgbe_mac_X550) {
+		for (i = 0; i < 10; i++)
+			rss_key[i] = IXGBE_READ_REG(hw,
+						IXGBE_PFVFRSSRK(i, vf));
+	} else
+		for (i = 0; i < 10; i++)
+			rss_key[i] = IXGBE_READ_REG(hw, IXGBE_RSSRK(i));
+
+	return 0;
+}
+
 static int ixgbe_rcv_msg_from_vf(struct ixgbe_adapter *adapter, u32 vf)
 {
 	u32 mbx_size = IXGBE_VFMAILBOX_SIZE;

@@ -1065,6 +1088,9 @@ static int ixgbe_rcv_msg_from_vf(struct ixgbe_adapter *adapter, u32 vf)
 	case IXGBE_VF_GET_RETA:
 		retval = ixgbe_get_vf_reta(adapter, msgbuf, vf);
 		break;
+	case IXGBE_VF_GET_RSS_KEY:
+		retval = ixgbe_get_vf_rss_key(adapter, msgbuf, vf);
+		break;
 	default:
 		e_err(drv, "Unhandled Msg %8.8x\n", msgbuf[0]);
 		retval = IXGBE_ERR_MBX;

-- 
2.1.0

Add the ixgbevf_get_rss_key() function that queries the PF for an RSS Random Key
using a new VF-PF channel IXGBE_VF_GET_RSS_KEY command.

Signed-off-by: Vlad Zolotarov <redacted>
---
New in v2:
   - Added a more detailed patch description.

New in v1 (compared to RFC):
   - Use "if-else" statement instead of a "switch-case" for a single option case
     (in ixgbevf_get_rss_key()).
---
 drivers/net/ethernet/intel/ixgbevf/mbx.h |  2 ++
 drivers/net/ethernet/intel/ixgbevf/vf.c  | 44 ++++++++++++++++++++++++++++++++
 drivers/net/ethernet/intel/ixgbevf/vf.h  |  1 +
 3 files changed, 47 insertions(+)

diff --git a/drivers/net/ethernet/intel/ixgbevf/mbx.h b/drivers/net/ethernet/intel/ixgbevf/mbx.h
index 951a506..f79432e 100644
--- a/drivers/net/ethernet/intel/ixgbevf/mbx.h
+++ b/drivers/net/ethernet/intel/ixgbevf/mbx.h

@@ -118,6 +118,8 @@ enum ixgbe_pfvf_api_rev {
 #define IXGBE_VF_RETA_SZ        1	/* Number of RETA DWs to bring */
 #define IXGBE_VF_RETA_OFFSET    2	/* Offset in RETA */
 
+#define IXGBE_VF_GET_RSS_KEY	0x0b	/* get RSS hash key */
+
 /* length of permanent address message returned from PF */
 #define IXGBE_VF_PERMADDR_MSG_LEN 4
 /* word in permanent address message with the current multicast type */

diff --git a/drivers/net/ethernet/intel/ixgbevf/vf.c b/drivers/net/ethernet/intel/ixgbevf/vf.c
index cb5a4cf..f42a67d 100644
--- a/drivers/net/ethernet/intel/ixgbevf/vf.c
+++ b/drivers/net/ethernet/intel/ixgbevf/vf.c

@@ -292,6 +292,50 @@ static inline int _ixgbevf_get_reta(struct ixgbe_hw *hw, u32 *msgbuf,
 }
 
 /**
+ * ixgbevf_get_rss_key - get the RSS Random Key
+ * @hw: pointer to the HW structure
+ * @reta: buffer to fill with RETA contents.
+ *
+ * The "rss_key" buffer should be big enough to contain 10 registers.
+ *
+ * Returns: 0 on success.
+ *          if API doesn't support this operation - (-EPERM).
+ */
+int ixgbevf_get_rss_key(struct ixgbe_hw *hw, u8 *rss_key)
+{
+	int err;
+	u32 msgbuf[IXGBE_VFMAILBOX_SIZE];
+
+	/* Return and error if API doesn't support RSS Random Key retrieval */
+	if (hw->api_version != ixgbe_mbox_api_12)
+		return -EPERM;
+
+	msgbuf[0] = IXGBE_VF_GET_RSS_KEY;
+	err = hw->mbx.ops.write_posted(hw, msgbuf, 1);
+
+	if (err)
+		return err;
+
+	err = hw->mbx.ops.read_posted(hw, msgbuf, 11);
+
+	if (err)
+		return err;
+
+	msgbuf[0] &= ~IXGBE_VT_MSGTYPE_CTS;
+
+	/* If we didn't get an ACK there must have been
+	 * some sort of mailbox error so we should treat it
+	 * as such.
+	 */
+	if (msgbuf[0] != (IXGBE_VF_GET_RSS_KEY | IXGBE_VT_MSGTYPE_ACK))
+		return IXGBE_ERR_MBX;
+
+	memcpy(rss_key, msgbuf + 1, 40);
+
+	return 0;
+}
+
+/**
  * ixgbevf_get_reta - get the RSS redirection table (RETA) contents.
  * @hw: pointer to the HW structure
  * @reta: buffer to fill with RETA contents.

diff --git a/drivers/net/ethernet/intel/ixgbevf/vf.h b/drivers/net/ethernet/intel/ixgbevf/vf.h
index 73c1b33..54f53f2b8 100644
--- a/drivers/net/ethernet/intel/ixgbevf/vf.h
+++ b/drivers/net/ethernet/intel/ixgbevf/vf.h

@@ -209,5 +209,6 @@ int ixgbevf_negotiate_api_version(struct ixgbe_hw *hw, int api);
 int ixgbevf_get_queues(struct ixgbe_hw *hw, unsigned int *num_tcs,
 		       unsigned int *default_tc);
 int ixgbevf_get_reta(struct ixgbe_hw *hw, u32 *reta);
+int ixgbevf_get_rss_key(struct ixgbe_hw *hw, u8 *rss_key);
 #endif /* __IXGBE_VF_H__ */
 

-- 
2.1.0

Added get_rxfh_indir_size, get_rxfh_key_size and get_rxfh ethtool_ops callbacks
implementations.

This enables the ethtool's "-x" and "-n rx-flow-hash" options for 82599 VF devices.

Signed-off-by: Vlad Zolotarov <redacted>
---
New in v3:
   - Added a proper support for x550 devices: return the correct redirection table size.

New in v2:
   - Added a detailed description to the patch.
---
 drivers/net/ethernet/intel/ixgbevf/ethtool.c | 43 ++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/drivers/net/ethernet/intel/ixgbevf/ethtool.c b/drivers/net/ethernet/intel/ixgbevf/ethtool.c
index cc0e5b7..eba8c0f 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ethtool.c
+++ b/drivers/net/ethernet/intel/ixgbevf/ethtool.c

@@ -792,6 +792,46 @@ static int ixgbevf_set_coalesce(struct net_device *netdev,
 	return 0;
 }
 
+static u32 ixgbevf_get_rxfh_indir_size(struct net_device *netdev)
+{
+	struct ixgbevf_adapter *adapter = netdev_priv(netdev);
+
+	if (adapter->hw.mac.type >= ixgbe_mac_X550_vf) {
+		return 64;
+	} else {
+		return 128;
+	}
+}
+
+static u32 ixgbevf_get_rxfh_key_size(struct net_device *netdev)
+{
+	return 40;
+}
+
+static int ixgbevf_get_rxfh(struct net_device *netdev, u32 *indir, u8 *key,
+			    u8 *hfunc)
+{
+	struct ixgbevf_adapter *adapter = netdev_priv(netdev);
+	int err;
+
+	if (hfunc)
+		*hfunc = ETH_RSS_HASH_TOP;
+
+	if (indir) {
+		err = ixgbevf_get_reta(&adapter->hw, indir);
+		if (err)
+			return err;
+	}
+
+	if (key) {
+		err = ixgbevf_get_rss_key(&adapter->hw, key);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
 static const struct ethtool_ops ixgbevf_ethtool_ops = {
 	.get_settings           = ixgbevf_get_settings,
 	.get_drvinfo            = ixgbevf_get_drvinfo,

@@ -809,6 +849,9 @@ static const struct ethtool_ops ixgbevf_ethtool_ops = {
 	.get_ethtool_stats      = ixgbevf_get_ethtool_stats,
 	.get_coalesce           = ixgbevf_get_coalesce,
 	.set_coalesce           = ixgbevf_set_coalesce,
+	.get_rxfh_indir_size    = ixgbevf_get_rxfh_indir_size,
+	.get_rxfh_key_size      = ixgbevf_get_rxfh_key_size,
+	.get_rxfh		= ixgbevf_get_rxfh,
 };
 
 void ixgbevf_set_ethtool_ops(struct net_device *netdev)

-- 
2.1.0

On 01/05/15 16:15, Vlad Zolotarov wrote:

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

  - PF driver: Add new VF-PF channel commands.
  - VF driver: Utilize these new commands and add the corresponding
               ethtool callbacks.

Oops - forgot to run checkpatch before sending and there were some 
issues with styling... ;)
Have just sent v4 with all styling issues fixed... ;)

New in v3:
    - Added a missing support for x550 devices.
    - Mask the indirection table values according to PSRTYPE[n].RQPL.
    - Minimized the number of added VF-PF commands.

New in v2:
    - Added a detailed description to patches 4 and 5.

New in v1 (compared to RFC):
    - Use "if-else" statement instead of a "switch-case" for a single option case.
      More specifically: in cases where the newly added API version is the only one
      allowed. We may consider using a "switch-case" back again when the list of
      allowed API versions in these specific places grows up.

Vlad Zolotarov (5):
   ixgbe: Add a RETA query command to VF-PF channel API
   ixgbevf: Add a RETA query code
   ixgbe: Add GET_RSS_KEY command to VF-PF channel commands set
   ixgbevf: Add RSS Key query code
   ixgbevf: Add the appropriate ethtool ops to query RSS indirection
     table and key

  drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h      |  10 ++
  drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c    |  91 +++++++++++++++
  drivers/net/ethernet/intel/ixgbevf/ethtool.c      |  43 +++++++
  drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |   4 +-
  drivers/net/ethernet/intel/ixgbevf/mbx.h          |  10 ++
  drivers/net/ethernet/intel/ixgbevf/vf.c           | 132 ++++++++++++++++++++++
  drivers/net/ethernet/intel/ixgbevf/vf.h           |   2 +
  7 files changed, 291 insertions(+), 1 deletion(-)

On Mon, Jan 5, 2015 at 6:15 AM, Vlad Zolotarov
[off-list ref] wrote:

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

 - PF driver: Add new VF-PF channel commands.
 - VF driver: Utilize these new commands and add the corresponding
              ethtool callbacks.

New in v3:
   - Added a missing support for x550 devices.
   - Mask the indirection table values according to PSRTYPE[n].RQPL.
   - Minimized the number of added VF-PF commands.

New in v2:
   - Added a detailed description to patches 4 and 5.

New in v1 (compared to RFC):
   - Use "if-else" statement instead of a "switch-case" for a single option case.
     More specifically: in cases where the newly added API version is the only one
     allowed. We may consider using a "switch-case" back again when the list of
     allowed API versions in these specific places grows up.

Vlad Zolotarov (5):
  ixgbe: Add a RETA query command to VF-PF channel API
  ixgbevf: Add a RETA query code
  ixgbe: Add GET_RSS_KEY command to VF-PF channel commands set
  ixgbevf: Add RSS Key query code
  ixgbevf: Add the appropriate ethtool ops to query RSS indirection
    table and key

 drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h      |  10 ++
 drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c    |  91 +++++++++++++++
 drivers/net/ethernet/intel/ixgbevf/ethtool.c      |  43 +++++++
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |   4 +-
 drivers/net/ethernet/intel/ixgbevf/mbx.h          |  10 ++
 drivers/net/ethernet/intel/ixgbevf/vf.c           | 132 ++++++++++++++++++++++
 drivers/net/ethernet/intel/ixgbevf/vf.h           |   2 +
 7 files changed, 291 insertions(+), 1 deletion(-)

I've given this code a review and I don't see a way to
set a policy in the PF driver as to whether this request should be
allowed or not.  We cannot enable this query by default - it is a
security risk. To make this acceptable you need to do a
couple of things.

A) Have the query disabled by default such that when a VF driver
requests the RSS info the request is denied.

B) Add hooks to allow system admins to set the policy in the PF driver
as to whether the RSS info requests from the VFs are allowed or
denied.  Only provide the VF the privilege to request the RSS info if
the system admin has explicitly set the policy to allow it.  All other
times the request should be denied.

As it stands this is a non-starter.  Privileged information cannot be
made available to VFs without a way for the system admin to set
policy as to whether the information should be made available or not.

- Greg Rose
Intel Corp
Networking Division
[off-list ref]

--
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Jan 05, 2015 at 03:54:52PM -0800, Greg Rose wrote:

On Mon, Jan 5, 2015 at 6:15 AM, Vlad Zolotarov
[off-list ref] wrote:

quoted

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

 - PF driver: Add new VF-PF channel commands.
 - VF driver: Utilize these new commands and add the corresponding
              ethtool callbacks.

New in v3:
   - Added a missing support for x550 devices.
   - Mask the indirection table values according to PSRTYPE[n].RQPL.
   - Minimized the number of added VF-PF commands.

New in v2:
   - Added a detailed description to patches 4 and 5.

New in v1 (compared to RFC):
   - Use "if-else" statement instead of a "switch-case" for a single option case.
     More specifically: in cases where the newly added API version is the only one
     allowed. We may consider using a "switch-case" back again when the list of
     allowed API versions in these specific places grows up.

Vlad Zolotarov (5):
  ixgbe: Add a RETA query command to VF-PF channel API
  ixgbevf: Add a RETA query code
  ixgbe: Add GET_RSS_KEY command to VF-PF channel commands set
  ixgbevf: Add RSS Key query code
  ixgbevf: Add the appropriate ethtool ops to query RSS indirection
    table and key

 drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h      |  10 ++
 drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c    |  91 +++++++++++++++
 drivers/net/ethernet/intel/ixgbevf/ethtool.c      |  43 +++++++
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |   4 +-
 drivers/net/ethernet/intel/ixgbevf/mbx.h          |  10 ++
 drivers/net/ethernet/intel/ixgbevf/vf.c           | 132 ++++++++++++++++++++++
 drivers/net/ethernet/intel/ixgbevf/vf.h           |   2 +
 7 files changed, 291 insertions(+), 1 deletion(-)

I've given this code a review and I don't see a way to
set a policy in the PF driver as to whether this request should be
allowed or not.  We cannot enable this query by default - it is a
security risk. To make this acceptable you need to do a
couple of things.

Can you please elaborate on the security risk this information poses?
Is toeplitz hash function cryptographically strong enough so that VF
cannot reconstruct the hash key from hash result provided in packet
descriptor? The abstract of this paper [1] claims it is not, but I do
not have access to the full article unfortunately hence the question.

[1] http://ieeexplore.ieee.org/xpl/login.jsp?tp=&arnumber=5503170&url=http%3A%2F%2Fieeexplore.ieee.org%2Fxpls%2Fabs_all.jsp%3Farnumber%3D5503170

--
			Gleb.

On 01/06/15 08:55, Gleb Natapov wrote:

On Mon, Jan 05, 2015 at 03:54:52PM -0800, Greg Rose wrote:

quoted

On Mon, Jan 5, 2015 at 6:15 AM, Vlad Zolotarov
[off-list ref] wrote:

quoted

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

  - PF driver: Add new VF-PF channel commands.
  - VF driver: Utilize these new commands and add the corresponding
               ethtool callbacks.

New in v3:
    - Added a missing support for x550 devices.
    - Mask the indirection table values according to PSRTYPE[n].RQPL.
    - Minimized the number of added VF-PF commands.

New in v2:
    - Added a detailed description to patches 4 and 5.

New in v1 (compared to RFC):
    - Use "if-else" statement instead of a "switch-case" for a single option case.
      More specifically: in cases where the newly added API version is the only one
      allowed. We may consider using a "switch-case" back again when the list of
      allowed API versions in these specific places grows up.

Vlad Zolotarov (5):
   ixgbe: Add a RETA query command to VF-PF channel API
   ixgbevf: Add a RETA query code
   ixgbe: Add GET_RSS_KEY command to VF-PF channel commands set
   ixgbevf: Add RSS Key query code
   ixgbevf: Add the appropriate ethtool ops to query RSS indirection
     table and key

  drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h      |  10 ++
  drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c    |  91 +++++++++++++++
  drivers/net/ethernet/intel/ixgbevf/ethtool.c      |  43 +++++++
  drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |   4 +-
  drivers/net/ethernet/intel/ixgbevf/mbx.h          |  10 ++
  drivers/net/ethernet/intel/ixgbevf/vf.c           | 132 ++++++++++++++++++++++
  drivers/net/ethernet/intel/ixgbevf/vf.h           |   2 +
  7 files changed, 291 insertions(+), 1 deletion(-)

I've given this code a review and I don't see a way to
set a policy in the PF driver as to whether this request should be
allowed or not.  We cannot enable this query by default - it is a
security risk. To make this acceptable you need to do a
couple of things.

Can you please elaborate on the security risk this information poses?
Is toeplitz hash function cryptographically strong enough so that VF
cannot reconstruct the hash key from hash result provided in packet
descriptor? The abstract of this paper [1] claims it is not, but I do
not have access to the full article unfortunately hence the question.

[1] http://ieeexplore.ieee.org/xpl/login.jsp?tp=&arnumber=5503170&url=http%3A%2F%2Fieeexplore.ieee.org%2Fxpls%2Fabs_all.jsp%3Farnumber%3D5503170

I agree with Gleb here: when we started with just thinking about the 
idea of this patch the possible security issue was the first thing that 
came into our minds.
But eventually we couldn't come up with any security risk or attack 
example that is exclusively caused by the fact that VF knows the 
indirection table and/or RSS hash key of the PF.

So, Greg, if we have missed anything and your have such an example could 
you share it here, please?

Thanks,
vlad

--
			Gleb.

On Tue, Jan 6, 2015 at 2:58 AM, Vlad Zolotarov
[off-list ref] wrote:

On 01/06/15 08:55, Gleb Natapov wrote:

quoted

On Mon, Jan 05, 2015 at 03:54:52PM -0800, Greg Rose wrote:

quoted

On Mon, Jan 5, 2015 at 6:15 AM, Vlad Zolotarov
[off-list ref] wrote:

quoted

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

  - PF driver: Add new VF-PF channel commands.
  - VF driver: Utilize these new commands and add the corresponding
               ethtool callbacks.

New in v3:
    - Added a missing support for x550 devices.
    - Mask the indirection table values according to PSRTYPE[n].RQPL.
    - Minimized the number of added VF-PF commands.

New in v2:
    - Added a detailed description to patches 4 and 5.

New in v1 (compared to RFC):
    - Use "if-else" statement instead of a "switch-case" for a single
option case.
      More specifically: in cases where the newly added API version is
the only one
      allowed. We may consider using a "switch-case" back again when the
list of
      allowed API versions in these specific places grows up.

Vlad Zolotarov (5):
   ixgbe: Add a RETA query command to VF-PF channel API
   ixgbevf: Add a RETA query code
   ixgbe: Add GET_RSS_KEY command to VF-PF channel commands set
   ixgbevf: Add RSS Key query code
   ixgbevf: Add the appropriate ethtool ops to query RSS indirection
     table and key

  drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h      |  10 ++
  drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c    |  91
+++++++++++++++
  drivers/net/ethernet/intel/ixgbevf/ethtool.c      |  43 +++++++
  drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |   4 +-
  drivers/net/ethernet/intel/ixgbevf/mbx.h          |  10 ++
  drivers/net/ethernet/intel/ixgbevf/vf.c           | 132
++++++++++++++++++++++
  drivers/net/ethernet/intel/ixgbevf/vf.h           |   2 +
  7 files changed, 291 insertions(+), 1 deletion(-)

I've given this code a review and I don't see a way to
set a policy in the PF driver as to whether this request should be
allowed or not.  We cannot enable this query by default - it is a
security risk. To make this acceptable you need to do a
couple of things.

Can you please elaborate on the security risk this information poses?
Is toeplitz hash function cryptographically strong enough so that VF
cannot reconstruct the hash key from hash result provided in packet
descriptor? The abstract of this paper [1] claims it is not, but I do
not have access to the full article unfortunately hence the question.

[1]
http://ieeexplore.ieee.org/xpl/login.jsp?tp=&arnumber=5503170&url=http%3A%2F%2Fieeexplore.ieee.org%2Fxpls%2Fabs_all.jsp%3Farnumber%3D5503170

I agree with Gleb here: when we started with just thinking about the idea of
this patch the possible security issue was the first thing that came into
our minds.
But eventually we couldn't come up with any security risk or attack example
that is exclusively caused by the fact that VF knows the indirection table
and/or RSS hash key of the PF.

So, Greg, if we have missed anything and your have such an example could you
share it here, please?

I don't have any examples and that is not my area of expertise.  But
just because we can't think of a security risk or attack example
doesn't mean there isn't one.

Just add a policy hook so that the system admin can decide whether
this information should be shared with the VFs and then we're covered
for cases of both known and unknown exploits, risks, etc.

Thanks,

- Greg

Thanks,
vlad

quoted

--
                        Gleb.

On 01/06/15 18:59, Greg Rose wrote:

On Tue, Jan 6, 2015 at 2:58 AM, Vlad Zolotarov
[off-list ref] wrote:

quoted

On 01/06/15 08:55, Gleb Natapov wrote:

quoted

On Mon, Jan 05, 2015 at 03:54:52PM -0800, Greg Rose wrote:

quoted

On Mon, Jan 5, 2015 at 6:15 AM, Vlad Zolotarov
[off-list ref] wrote:

quoted

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

   - PF driver: Add new VF-PF channel commands.
   - VF driver: Utilize these new commands and add the corresponding
                ethtool callbacks.

New in v3:
     - Added a missing support for x550 devices.
     - Mask the indirection table values according to PSRTYPE[n].RQPL.
     - Minimized the number of added VF-PF commands.

New in v2:
     - Added a detailed description to patches 4 and 5.

New in v1 (compared to RFC):
     - Use "if-else" statement instead of a "switch-case" for a single
option case.
       More specifically: in cases where the newly added API version is
the only one
       allowed. We may consider using a "switch-case" back again when the
list of
       allowed API versions in these specific places grows up.

Vlad Zolotarov (5):
    ixgbe: Add a RETA query command to VF-PF channel API
    ixgbevf: Add a RETA query code
    ixgbe: Add GET_RSS_KEY command to VF-PF channel commands set
    ixgbevf: Add RSS Key query code
    ixgbevf: Add the appropriate ethtool ops to query RSS indirection
      table and key

   drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h      |  10 ++
   drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c    |  91
+++++++++++++++
   drivers/net/ethernet/intel/ixgbevf/ethtool.c      |  43 +++++++
   drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |   4 +-
   drivers/net/ethernet/intel/ixgbevf/mbx.h          |  10 ++
   drivers/net/ethernet/intel/ixgbevf/vf.c           | 132
++++++++++++++++++++++
   drivers/net/ethernet/intel/ixgbevf/vf.h           |   2 +
   7 files changed, 291 insertions(+), 1 deletion(-)

I've given this code a review and I don't see a way to
set a policy in the PF driver as to whether this request should be
allowed or not.  We cannot enable this query by default - it is a
security risk. To make this acceptable you need to do a
couple of things.

Can you please elaborate on the security risk this information poses?
Is toeplitz hash function cryptographically strong enough so that VF
cannot reconstruct the hash key from hash result provided in packet
descriptor? The abstract of this paper [1] claims it is not, but I do
not have access to the full article unfortunately hence the question.

[1]
http://ieeexplore.ieee.org/xpl/login.jsp?tp=&arnumber=5503170&url=http%3A%2F%2Fieeexplore.ieee.org%2Fxpls%2Fabs_all.jsp%3Farnumber%3D5503170

I agree with Gleb here: when we started with just thinking about the idea of
this patch the possible security issue was the first thing that came into
our minds.
But eventually we couldn't come up with any security risk or attack example
that is exclusively caused by the fact that VF knows the indirection table
and/or RSS hash key of the PF.

So, Greg, if we have missed anything and your have such an example could you
share it here, please?

I don't have any examples and that is not my area of expertise.  But
just because we can't think of a security risk or attack example
doesn't mean there isn't one.

Just add a policy hook so that the system admin can decide whether
this information should be shared with the VFs and then we're covered
for cases of both known and unknown exploits, risks, etc.

I absolutely disagree with u in regard of defining an RSS redirection 
table and RSS hash key as a security sensitive data. I don't know how u 
got to this conclusion.
However I don't want to argue about any longer. Let's move on.

Let's clarify one thing about this "hook". Do u agree that it should 
cover only the cases when VF shares the mentioned above data with PF - 
namely for all devices but x550?

thanks,
vlad

Thanks,

- Greg

quoted

Thanks,
vlad

quoted

--
                         Gleb.

On Tue, Jan 06, 2015 at 08:59:41AM -0800, Greg Rose wrote:

On Tue, Jan 6, 2015 at 2:58 AM, Vlad Zolotarov
[off-list ref] wrote:

quoted

On 01/06/15 08:55, Gleb Natapov wrote:

quoted

On Mon, Jan 05, 2015 at 03:54:52PM -0800, Greg Rose wrote:

quoted

On Mon, Jan 5, 2015 at 6:15 AM, Vlad Zolotarov
[off-list ref] wrote:

quoted

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

  - PF driver: Add new VF-PF channel commands.
  - VF driver: Utilize these new commands and add the corresponding
               ethtool callbacks.

New in v3:
    - Added a missing support for x550 devices.
    - Mask the indirection table values according to PSRTYPE[n].RQPL.
    - Minimized the number of added VF-PF commands.

New in v2:
    - Added a detailed description to patches 4 and 5.

New in v1 (compared to RFC):
    - Use "if-else" statement instead of a "switch-case" for a single
option case.
      More specifically: in cases where the newly added API version is
the only one
      allowed. We may consider using a "switch-case" back again when the
list of
      allowed API versions in these specific places grows up.

Vlad Zolotarov (5):
   ixgbe: Add a RETA query command to VF-PF channel API
   ixgbevf: Add a RETA query code
   ixgbe: Add GET_RSS_KEY command to VF-PF channel commands set
   ixgbevf: Add RSS Key query code
   ixgbevf: Add the appropriate ethtool ops to query RSS indirection
     table and key

  drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h      |  10 ++
  drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c    |  91
+++++++++++++++
  drivers/net/ethernet/intel/ixgbevf/ethtool.c      |  43 +++++++
  drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |   4 +-
  drivers/net/ethernet/intel/ixgbevf/mbx.h          |  10 ++
  drivers/net/ethernet/intel/ixgbevf/vf.c           | 132
++++++++++++++++++++++
  drivers/net/ethernet/intel/ixgbevf/vf.h           |   2 +
  7 files changed, 291 insertions(+), 1 deletion(-)

I've given this code a review and I don't see a way to
set a policy in the PF driver as to whether this request should be
allowed or not.  We cannot enable this query by default - it is a
security risk. To make this acceptable you need to do a
couple of things.

Can you please elaborate on the security risk this information poses?
Is toeplitz hash function cryptographically strong enough so that VF
cannot reconstruct the hash key from hash result provided in packet
descriptor? The abstract of this paper [1] claims it is not, but I do
not have access to the full article unfortunately hence the question.

[1]
http://ieeexplore.ieee.org/xpl/login.jsp?tp=&arnumber=5503170&url=http%3A%2F%2Fieeexplore.ieee.org%2Fxpls%2Fabs_all.jsp%3Farnumber%3D5503170

I agree with Gleb here: when we started with just thinking about the idea of
this patch the possible security issue was the first thing that came into
our minds.
But eventually we couldn't come up with any security risk or attack example
that is exclusively caused by the fact that VF knows the indirection table
and/or RSS hash key of the PF.

So, Greg, if we have missed anything and your have such an example could you
share it here, please?

I don't have any examples and that is not my area of expertise.  But
just because we can't think of a security risk or attack example
doesn't mean there isn't one.

Is RSS hash security feature at all? Against what kind of attack? It
looks like some drivers (igb among them) use non random value for the key.

Just add a policy hook so that the system admin can decide whether
this information should be shared with the VFs and then we're covered
for cases of both known and unknown exploits, risks, etc.

Default off means that it will stay that way for most installations and
information will not be available for "cloud" users. It is hard to get
proper support on public cloud for less trivial issues than changing
host HW configuration.

--
			Gleb.

I accidentally replied just to Vlad - here is a reply to all.

On Tue, Jan 6, 2015 at 9:30 AM, Vlad Zolotarov
[off-list ref] wrote:

On 01/06/15 18:59, Greg Rose wrote:

quoted

[snip]

quoted

I don't have any examples and that is not my area of expertise.  But
just because we can't think of a security risk or attack example
doesn't mean there isn't one.

Just add a policy hook so that the system admin can decide whether
this information should be shared with the VFs and then we're covered
for cases of both known and unknown exploits, risks, etc.

I absolutely disagree with u in regard of defining an RSS redirection table
and RSS hash key as a security sensitive data. I don't know how u got to
this conclusion.

I have not reached any such conclusion - let me reiterate:  I have no
idea.  It is not my area of expertise.  However, to take the lowest
risk route just add a policy hook so that a system admin can turn the
feature on through the PF driver (which is acknowledged as secure) if
they wish then there is no worry.

However I don't want to argue about any longer. Let's move on.

Let's clarify one thing about this "hook". Do u agree that it should cover
only the cases when VF shares the mentioned above data with PF - namely for
all devices but x550?

Look at how spoof checking is turned off/on for each VF using the "ip
link set" commands.  That's what I'm envisioning - some way to decide
on a per VF basis which VFs should be allowed to perform the query.

Thanks,

- Greg

On Tue, Jan 6, 2015 at 10:04 AM, Gleb Natapov [off-list ref] wrote:

On Tue, Jan 06, 2015 at 08:59:41AM -0800, Greg Rose wrote:

quoted

On Tue, Jan 6, 2015 at 2:58 AM, Vlad Zolotarov
[off-list ref] wrote:

quoted

I agree with Gleb here: when we started with just thinking about the idea of
this patch the possible security issue was the first thing that came into
our minds.
But eventually we couldn't come up with any security risk or attack example
that is exclusively caused by the fact that VF knows the indirection table
and/or RSS hash key of the PF.

So, Greg, if we have missed anything and your have such an example could you
share it here, please?

I don't have any examples and that is not my area of expertise.  But
just because we can't think of a security risk or attack example
doesn't mean there isn't one.

Is RSS hash security feature at all? Against what kind of attack? It
looks like some drivers (igb among them) use non random value for the key.

I don't believe RSS hashing itself is a security feature - I don't
know that sharing the RSS info with a VF is a security risk.  I'm just
asking that we preserve default behavior to avoid the possibility.

quoted

Just add a policy hook so that the system admin can decide whether
this information should be shared with the VFs and then we're covered
for cases of both known and unknown exploits, risks, etc.

Default off means that it will stay that way for most installations and
information will not be available for "cloud" users. It is hard to get
proper support on public cloud for less trivial issues than changing
host HW configuration.

Someone in the host is configuring the VF HW to begin with.  Someone
had to create the VFs in the first place so I presume they could set
the policy for this feature as well at the same time.  To return to an
example I provided to Vlad - anti-spoof checking is on by default but
we allow system admins to turn it off so that other features, such as
bonding, can be used.  I just want to preserve current behavior while
allowing the feature you want to add to be available for those who
want it.

If Dave and the rest of community feel that there is no risk to these
patches and that they should be applied then I'll go away and shut up
about it.  But for now I'm just approaching this from a "better safe
than sorry" viewpoint.

Thanks,

- Greg

--
                        Gleb.

On Tue, Jan 06, 2015 at 10:30:59AM -0800, Greg Rose wrote:

On Tue, Jan 6, 2015 at 10:04 AM, Gleb Natapov [off-list ref] wrote:

quoted

On Tue, Jan 06, 2015 at 08:59:41AM -0800, Greg Rose wrote:

quoted

On Tue, Jan 6, 2015 at 2:58 AM, Vlad Zolotarov
[off-list ref] wrote:

quoted

I agree with Gleb here: when we started with just thinking about the idea of
this patch the possible security issue was the first thing that came into
our minds.
But eventually we couldn't come up with any security risk or attack example
that is exclusively caused by the fact that VF knows the indirection table
and/or RSS hash key of the PF.

So, Greg, if we have missed anything and your have such an example could you
share it here, please?

I don't have any examples and that is not my area of expertise.  But
just because we can't think of a security risk or attack example
doesn't mean there isn't one.

Is RSS hash security feature at all? Against what kind of attack? It
looks like some drivers (igb among them) use non random value for the key.

I don't believe RSS hashing itself is a security feature - I don't
know that sharing the RSS info with a VF is a security risk.  I'm just
asking that we preserve default behavior to avoid the possibility.

quoted

quoted

Just add a policy hook so that the system admin can decide whether
this information should be shared with the VFs and then we're covered
for cases of both known and unknown exploits, risks, etc.

Default off means that it will stay that way for most installations and
information will not be available for "cloud" users. It is hard to get
proper support on public cloud for less trivial issues than changing
host HW configuration.

Someone in the host is configuring the VF HW to begin with.  Someone
had to create the VFs in the first place so I presume they could set
the policy for this feature as well at the same time.  To return to an
example I provided to Vlad - anti-spoof checking is on by default but
we allow system admins to turn it off so that other features, such as
bonding, can be used.  I just want to preserve current behavior while
allowing the feature you want to add to be available for those who
want it.

If Dave and the rest of community feel that there is no risk to these
patches and that they should be applied then I'll go away and shut up
about it.  But for now I'm just approaching this from a "better safe
than sorry" viewpoint.

Thanks Greg for explaining your position clearly on this matter. I CCed
Dave to get his opinion. Vlad is going to work on adding this knob
anyway meanwhile, but we still have a hope that default could be "on".

--
			Gleb.

On Tue, 2015-01-06 at 20:04 +0200, Gleb Natapov wrote:

Is RSS hash security feature at all? Against what kind of attack? It
looks like some drivers (igb among them) use non random value for the key.

Are you sure ? I thought I toko care of this already.

For igb this was in : 

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=eb31f8493eee1efcc194fdc98df830a503f572f1

On Tue, Jan 06, 2015 at 10:59:04AM -0800, Eric Dumazet wrote:

On Tue, 2015-01-06 at 20:04 +0200, Gleb Natapov wrote:

quoted

Is RSS hash security feature at all? Against what kind of attack? It
looks like some drivers (igb among them) use non random value for the key.

Are you sure ? I thought I toko care of this already.

For igb this was in : 

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=eb31f8493eee1efcc194fdc98df830a503f572f1

Indeed. I looked in a couple of month old tree.

--
			Gleb.

On 01/06/15 20:22, Greg Rose wrote:

I accidentally replied just to Vlad - here is a reply to all.

On Tue, Jan 6, 2015 at 9:30 AM, Vlad Zolotarov
[off-list ref]  wrote:

quoted

On 01/06/15 18:59, Greg Rose wrote:

[snip]

quoted

quoted

I don't have any examples and that is not my area of expertise.  But
just because we can't think of a security risk or attack example
doesn't mean there isn't one.

Just add a policy hook so that the system admin can decide whether
this information should be shared with the VFs and then we're covered
for cases of both known and unknown exploits, risks, etc.

I absolutely disagree with u in regard of defining an RSS redirection table
and RSS hash key as a security sensitive data. I don't know how u got to
this conclusion.

I have not reached any such conclusion - let me reiterate:  I have no
idea.  It is not my area of expertise.  However, to take the lowest
risk route just add a policy hook so that a system admin can turn the
feature on through the PF driver (which is acknowledged as secure) if
they wish then there is no worry.

NP. Let's move on.

quoted

However I don't want to argue about any longer. Let's move on.

Let's clarify one thing about this "hook". Do u agree that it should cover
only the cases when VF shares the mentioned above data with PF - namely for
all devices but x550?

Look at how spoof checking is turned off/on for each VF using the "ip
link set" commands.  That's what I'm envisioning - some way to decide
on a per VF basis which VFs should be allowed to perform the query.

I will but let's agree that x550 VFs should be out of this since their 
RSS indirection table and Key belong to the specific domain and don't 
impose any even theoretical thread.

thanks,
vlad

Thanks,

- Greg

On Tue, Jan 6, 2015 at 12:13 PM, Vlad Zolotarov
[off-list ref] wrote:

On 01/06/15 20:22, Greg Rose wrote:

quoted

[snip]

quoted

I have not reached any such conclusion - let me reiterate:  I have no
idea.  It is not my area of expertise.  However, to take the lowest
risk route just add a policy hook so that a system admin can turn the
feature on through the PF driver (which is acknowledged as secure) if
they wish then there is no worry.

NP. Let's move on.

quoted

quoted

However I don't want to argue about any longer. Let's move on.

Let's clarify one thing about this "hook". Do u agree that it should
cover
only the cases when VF shares the mentioned above data with PF - namely
for
all devices but x550?

Look at how spoof checking is turned off/on for each VF using the "ip
link set" commands.  That's what I'm envisioning - some way to decide
on a per VF basis which VFs should be allowed to perform the query.

I will but let's agree that x550 VFs should be out of this since their RSS
indirection table and Key belong to the specific domain and don't impose any
even theoretical thread.

Sounds good to me.

Thanks!

- Greg

thanks,
vlad

quoted

Thanks,

- Greg