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

[PATCH net-next v9 0/7]: ixgbevf: Allow querying VFs 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. 

Currently we will support only 82599 and x540 devices. On these devices VFs share the
RSS Redirection Table and Hash Key with a PF and letting the VF query this information may
introduce some security risks. Therefore we will disable this feature by default.

The new netdev op is going to allow a system administrator to change the default behaviour with
"ip link set" command. The relevant iproute2 patch has already been sent and awaits for this series to
be accepted.

 - netdev: Add a new netdev op to allow/block VF from querying RSS Indirection Table and
   RSS Hash Key.
 - PF driver: Add new VF-PF channel commands.
 - VF driver: Utilize these new commands and add the corresponding
              ethtool callbacks.

New in v9:
   - Reduce the support to 82599 and x540 devices only.
   - ixgbe: improvements in query RETA VF-PF command implementation:
      - Use the cached RETA contents.
      - Compress the mailbox message.
   - ixgbevf: improvements in RETA query code:
      - Implement a "compression" of VF's RETA contents: pass only 2 bits
        per-entry.
      - RETA querying is done in a single mailbox operation thanks to compression.
   - Get the RSS HASH Key value from the PF's adapter->rss_key[].
   - Added IXGBEVF_RSS_HASH_KEY_SIZE macro.

New in v8:
   - Protect new mailbox operations with adapter.mbx_lock spinlock.

New in v7:
   - Add ixgbe_mbox_api_12 case in ixgbevf_set_num_queues().
   - Properly expand HW RETA into the ethtool buffer.

New in v6:
   - Add a proper return code when an VF query operations are blocked by PF.
   - Added a required get_rxnfc callback to ixgbevf_ethtool_ops.
   - Changed a description of PATCH7: set the correct ethtool options names.

New in v5:
   - Added a new netdev op to allow/block VF from querying RSS Indirection Table and
     RSS Hash Key.
   - Let VF query the RSS info only if VF is allowed to.

New in v4:
   - Forgot to run checkpatch on v3 and there were a few styling things to fix. ;)

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 (7):
  if_link: Add an additional parameter to ifla_vf_info for RSS querying
  ixgbe: Add a new netdev op to allow/prevent a VF from querying an RSS
    info
  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.h          |   1 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c     |   7 ++
 drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h      |   5 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c    | 104 ++++++++++++++++
 drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h    |   2 +
 drivers/net/ethernet/intel/ixgbevf/ethtool.c      |  58 +++++++++
 drivers/net/ethernet/intel/ixgbevf/ixgbevf.h      |   1 +
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |   5 +-
 drivers/net/ethernet/intel/ixgbevf/mbx.h          |   5 +
 drivers/net/ethernet/intel/ixgbevf/vf.c           | 142 ++++++++++++++++++++++
 drivers/net/ethernet/intel/ixgbevf/vf.h           |   3 +
 include/linux/if_link.h                           |   1 +
 include/linux/netdevice.h                         |   8 ++
 include/uapi/linux/if_link.h                      |   8 ++
 net/core/rtnetlink.c                              |  32 ++++-
 15 files changed, 375 insertions(+), 7 deletions(-)

-- 
2.1.0

[PATCH net-next v9 1/7] if_link: Add an additional parameter to ifla_vf_info for RSS querying

[Vlad Zolotarov]

Add configuration setting for drivers to allow/block an RSS Redirection Table and
a Hash Key querying for discrete VFs.

On some devices VF share the mentioned above information with PF and querying it may
adduce a theoretical security risk. We want to let a system administrator to decide if he/she
wants to take this risk or not.

Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
---
 include/linux/if_link.h      |  1 +
 include/linux/netdevice.h    |  8 ++++++++
 include/uapi/linux/if_link.h |  8 ++++++++
 net/core/rtnetlink.c         | 32 ++++++++++++++++++++++++++------
 4 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/include/linux/if_link.h b/include/linux/if_link.h
index 119130e..da49299 100644
--- a/include/linux/if_link.h
+++ b/include/linux/if_link.h
@@ -14,5 +14,6 @@ struct ifla_vf_info {
 	__u32 linkstate;
 	__u32 min_tx_rate;
 	__u32 max_tx_rate;
+	__u32 rss_query_en;
 };
 #endif /* _LINUX_IF_LINK_H */
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 08c4ab3..a858cda 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -875,6 +875,11 @@ typedef u16 (*select_queue_fallback_t)(struct net_device *dev,
  * int (*ndo_set_vf_link_state)(struct net_device *dev, int vf, int link_state);
  * int (*ndo_set_vf_port)(struct net_device *dev, int vf,
  *			  struct nlattr *port[]);
+ *
+ *      Enable or disable the VF ability to query its RSS Redirection Table and
+ *      Hash Key. This is needed since on some devices VF share this information
+ *      with PF and querying it may adduce a theoretical security risk.
+ * int (*ndo_set_vf_rss_query_en)(struct net_device *dev, int vf, bool setting);
  * int (*ndo_get_vf_port)(struct net_device *dev, int vf, struct sk_buff *skb);
  * int (*ndo_setup_tc)(struct net_device *dev, u8 tc)
  * 	Called to setup 'tc' number of traffic classes in the net device. This
@@ -1094,6 +1099,9 @@ struct net_device_ops {
 						   struct nlattr *port[]);
 	int			(*ndo_get_vf_port)(struct net_device *dev,
 						   int vf, struct sk_buff *skb);
+	int			(*ndo_set_vf_rss_query_en)(
+						   struct net_device *dev,
+						   int vf, bool setting);
 	int			(*ndo_setup_tc)(struct net_device *dev, u8 tc);
 #if IS_ENABLED(CONFIG_FCOE)
 	int			(*ndo_fcoe_enable)(struct net_device *dev);
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 7ffb18d..d9cd192 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -465,6 +465,9 @@ enum {
 	IFLA_VF_SPOOFCHK,	/* Spoof Checking on/off switch */
 	IFLA_VF_LINK_STATE,	/* link state enable/disable/auto switch */
 	IFLA_VF_RATE,		/* Min and Max TX Bandwidth Allocation */
+	IFLA_VF_RSS_QUERY_EN,	/* RSS Redirection Table and Hash Key query
+				 * on/off switch
+				 */
 	__IFLA_VF_MAX,
 };
 
@@ -509,6 +512,11 @@ struct ifla_vf_link_state {
 	__u32 link_state;
 };
 
+struct ifla_vf_rss_query_en {
+	__u32 vf;
+	__u32 setting;
+};
+
 /* VF ports management section
  *
  *	Nested layout of set/get msg is:
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index b96ac21..dcbf568 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -818,7 +818,8 @@ static inline int rtnl_vfinfo_size(const struct net_device *dev,
 			 nla_total_size(sizeof(struct ifla_vf_vlan)) +
 			 nla_total_size(sizeof(struct ifla_vf_spoofchk)) +
 			 nla_total_size(sizeof(struct ifla_vf_rate)) +
-			 nla_total_size(sizeof(struct ifla_vf_link_state)));
+			 nla_total_size(sizeof(struct ifla_vf_link_state)) +
+			 nla_total_size(sizeof(struct ifla_vf_rss_query_en)));
 		return size;
 	} else
 		return 0;
@@ -1132,14 +1133,16 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 			struct ifla_vf_tx_rate vf_tx_rate;
 			struct ifla_vf_spoofchk vf_spoofchk;
 			struct ifla_vf_link_state vf_linkstate;
+			struct ifla_vf_rss_query_en vf_rss_query_en;
 
 			/*
 			 * Not all SR-IOV capable drivers support the
-			 * spoofcheck query.  Preset to -1 so the user
-			 * space tool can detect that the driver didn't
-			 * report anything.
+			 * spoofcheck and "RSS query enable" query.  Preset to
+			 * -1 so the user space tool can detect that the driver
+			 * didn't report anything.
 			 */
 			ivi.spoofchk = -1;
+			ivi.rss_query_en = -1;
 			memset(ivi.mac, 0, sizeof(ivi.mac));
 			/* The default value for VF link state is "auto"
 			 * IFLA_VF_LINK_STATE_AUTO which equals zero
@@ -1152,7 +1155,8 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 				vf_rate.vf =
 				vf_tx_rate.vf =
 				vf_spoofchk.vf =
-				vf_linkstate.vf = ivi.vf;
+				vf_linkstate.vf =
+				vf_rss_query_en.vf = ivi.vf;
 
 			memcpy(vf_mac.mac, ivi.mac, sizeof(ivi.mac));
 			vf_vlan.vlan = ivi.vlan;
@@ -1162,6 +1166,7 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 			vf_rate.max_tx_rate = ivi.max_tx_rate;
 			vf_spoofchk.setting = ivi.spoofchk;
 			vf_linkstate.link_state = ivi.linkstate;
+			vf_rss_query_en.setting = ivi.rss_query_en;
 			vf = nla_nest_start(skb, IFLA_VF_INFO);
 			if (!vf) {
 				nla_nest_cancel(skb, vfinfo);
@@ -1176,7 +1181,10 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 			    nla_put(skb, IFLA_VF_SPOOFCHK, sizeof(vf_spoofchk),
 				    &vf_spoofchk) ||
 			    nla_put(skb, IFLA_VF_LINK_STATE, sizeof(vf_linkstate),
-				    &vf_linkstate))
+				    &vf_linkstate) ||
+			    nla_put(skb, IFLA_VF_RSS_QUERY_EN,
+				    sizeof(vf_rss_query_en),
+				    &vf_rss_query_en))
 				goto nla_put_failure;
 			nla_nest_end(skb, vf);
 		}
@@ -1290,6 +1298,7 @@ static const struct nla_policy ifla_vf_policy[IFLA_VF_MAX+1] = {
 	[IFLA_VF_SPOOFCHK]	= { .len = sizeof(struct ifla_vf_spoofchk) },
 	[IFLA_VF_RATE]		= { .len = sizeof(struct ifla_vf_rate) },
 	[IFLA_VF_LINK_STATE]	= { .len = sizeof(struct ifla_vf_link_state) },
+	[IFLA_VF_RSS_QUERY_EN]	= { .len = sizeof(struct ifla_vf_rss_query_en) },
 };
 
 static const struct nla_policy ifla_port_policy[IFLA_PORT_MAX+1] = {
@@ -1500,6 +1509,17 @@ static int do_setvfinfo(struct net_device *dev, struct nlattr *attr)
 								 ivl->link_state);
 			break;
 		}
+		case IFLA_VF_RSS_QUERY_EN: {
+			struct ifla_vf_rss_query_en *ivrssq_en;
+
+			ivrssq_en = nla_data(vf);
+			err = -EOPNOTSUPP;
+			if (ops->ndo_set_vf_rss_query_en)
+				err = ops->ndo_set_vf_rss_query_en(dev,
+							    ivrssq_en->vf,
+							    ivrssq_en->setting);
+			break;
+		}
 		default:
 			err = -EINVAL;
 			break;
-- 
2.1.0

[PATCH net-next v9 2/7] ixgbe: Add a new netdev op to allow/prevent a VF from querying an RSS info

[Vlad Zolotarov]

Implements the new netdev op to allow user to enable/disable the ability
of a specific VF to query its RSS Indirection Table and an RSS Hash Key.

This patch limits the new feature support to 82599 and x540 devices only.
Support for other devices will be added later.

Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
---
New in v9:
   - Reduce the support to 82599 and x540 devices only.
---
 drivers/net/ethernet/intel/ixgbe/ixgbe.h       |  1 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c  |  7 ++++++
 drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c | 30 ++++++++++++++++++++++++++
 drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h |  2 ++
 4 files changed, 40 insertions(+)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index 42ed4b4..639aa1b 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -151,6 +151,7 @@ struct vf_data_storage {
 	u16 tx_rate;
 	u16 vlan_count;
 	u8 spoofchk_enabled;
+	bool rss_query_enabled;
 	unsigned int vf_api;
 };
 
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index a99bc5d..1b04cb1 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -3656,6 +3656,12 @@ static void ixgbe_configure_virtualization(struct ixgbe_adapter *adapter)
 		if (hw->mac.ops.set_ethertype_anti_spoofing)
 			hw->mac.ops.set_ethertype_anti_spoofing(hw, true, i);
 	}
+
+	/* Enable/Disable RSS query feature  */
+	for (i = 0; i < adapter->num_vfs; i++)
+		ixgbe_ndo_set_vf_rss_query_en(adapter->netdev, i,
+					      adapter->vfinfo[i].rss_query_enabled);
+
 }
 
 static void ixgbe_set_rx_buffer_len(struct ixgbe_adapter *adapter)
@@ -8096,6 +8102,7 @@ static const struct net_device_ops ixgbe_netdev_ops = {
 	.ndo_set_vf_vlan	= ixgbe_ndo_set_vf_vlan,
 	.ndo_set_vf_rate	= ixgbe_ndo_set_vf_bw,
 	.ndo_set_vf_spoofchk	= ixgbe_ndo_set_vf_spoofchk,
+	.ndo_set_vf_rss_query_en = ixgbe_ndo_set_vf_rss_query_en,
 	.ndo_get_vf_config	= ixgbe_ndo_get_vf_config,
 	.ndo_get_stats64	= ixgbe_get_stats64,
 #ifdef CONFIG_IXGBE_DCB
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
index 09a291b..f08672a 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
@@ -108,6 +108,15 @@ static int __ixgbe_enable_sriov(struct ixgbe_adapter *adapter)
 		/* enable spoof checking for all VFs */
 		for (i = 0; i < adapter->num_vfs; i++)
 			adapter->vfinfo[i].spoofchk_enabled = true;
+
+		/* We support VF RSS querying only for 82599 and x540 devices at
+		 * the moment. These devices share RSS indirection table and
+		 * RSS hash key with PF therefore we want to disable the
+		 * querying by default.
+		 */
+		for (i = 0; i < adapter->num_vfs; i++)
+			adapter->vfinfo[i].rss_query_enabled = 0;
+
 		return 0;
 	}
 
@@ -1330,6 +1339,26 @@ int ixgbe_ndo_set_vf_spoofchk(struct net_device *netdev, int vf, bool setting)
 	return 0;
 }
 
+int ixgbe_ndo_set_vf_rss_query_en(struct net_device *netdev, int vf,
+				  bool setting)
+{
+	struct ixgbe_adapter *adapter = netdev_priv(netdev);
+
+	/* This operation is currently supported only for 82599 and x540
+	 * devices.
+	 */
+	if (adapter->hw.mac.type < ixgbe_mac_82599EB ||
+	    adapter->hw.mac.type >= ixgbe_mac_X550)
+		return -EPERM;
+
+	if (vf >= adapter->num_vfs)
+		return -EINVAL;
+
+	adapter->vfinfo[vf].rss_query_enabled = setting;
+
+	return 0;
+}
+
 int ixgbe_ndo_get_vf_config(struct net_device *netdev,
 			    int vf, struct ifla_vf_info *ivi)
 {
@@ -1343,5 +1372,6 @@ int ixgbe_ndo_get_vf_config(struct net_device *netdev,
 	ivi->vlan = adapter->vfinfo[vf].pf_vlan;
 	ivi->qos = adapter->vfinfo[vf].pf_qos;
 	ivi->spoofchk = adapter->vfinfo[vf].spoofchk_enabled;
+	ivi->rss_query_en = adapter->vfinfo[vf].rss_query_enabled;
 	return 0;
 }
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h
index 32c26d5..2c197e6 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h
@@ -47,6 +47,8 @@ int ixgbe_ndo_set_vf_vlan(struct net_device *netdev, int queue, u16 vlan,
 int ixgbe_ndo_set_vf_bw(struct net_device *netdev, int vf, int min_tx_rate,
 			int max_tx_rate);
 int ixgbe_ndo_set_vf_spoofchk(struct net_device *netdev, int vf, bool setting);
+int ixgbe_ndo_set_vf_rss_query_en(struct net_device *netdev, int vf,
+				  bool setting);
 int ixgbe_ndo_get_vf_config(struct net_device *netdev,
 			    int vf, struct ifla_vf_info *ivi);
 void ixgbe_check_vf_rate_limit(struct ixgbe_adapter *adapter);
-- 
2.1.0

[PATCH net-next v9 3/7] ixgbe: Add a RETA query command to VF-PF channel API

[Vlad Zolotarov]

Add this new command for 82599 and x540 devices only. Support for other devices
will be added later.

82599 and x540 VFs and PF share the same RSS redirection table (RETA). Therefore we
just return it for all VFs.

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.

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 <vladz@cloudius-systems.com>
---
New in v9:
   - Reduce the support to 82599 and x540 devices only.
   - Improvements in query RETA VF-PF command implementation:
      - Use the cached RETA contents.
      - Compress the mailbox message.

New in v5:
   - Use the newly added netdev op to allow/prevent the RETA query on a per-VF basis.

New in v4:
   - Deleted an empty line in ixgbe_get_vf_reta() switch-case.

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   |  4 +++
 drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c | 49 ++++++++++++++++++++++++++
 2 files changed, 53 insertions(+)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h
index a5cb755..3522f53 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,9 @@ 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 */
+
 /* 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 f08672a..8424e7f 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
@@ -433,6 +433,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
@@ -900,6 +901,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:
@@ -923,6 +925,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;
@@ -950,6 +953,49 @@ 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;
+	u32 i, j;
+	u32 *out_buf = &msgbuf[1];
+	const u8 *reta = adapter->rss_indir_tbl;
+	u8 mask = 0;
+	u32 psrtype;
+	u32 reta_size = ixgbe_rss_indir_tbl_entries(adapter);
+
+	/* verify the PF is supporting the correct API */
+	if (!adapter->vfinfo[vf].rss_query_enabled ||
+	    (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
+	 *
+	 * 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 = 0x01;
+	else if ((psrtype & (2 << 29)) == (2 << 29))
+		mask = 0x03;
+
+	/* Compress the RETA by saving only 2 bits from each entry. This way
+	 * we will be able to transfer the whole RETA in a single mailbox
+	 * operation.
+	 */
+	for (i = 0; i < reta_size / 16; i++) {
+		out_buf[i] = 0;
+		for (j = 0; j < 16; j++)
+			out_buf[i] |= (u32)(reta[16 * i + j] & mask) << (2 * j);
+	}
+
+	return 0;
+}
+
 static int ixgbe_rcv_msg_from_vf(struct ixgbe_adapter *adapter, u32 vf)
 {
 	u32 mbx_size = IXGBE_VFMAILBOX_SIZE;
@@ -1006,6 +1052,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

[PATCH net-next v9 4/7] ixgbevf: Add a RETA query code

[Vlad Zolotarov]

We will currently support only 82599 and x540 deviced. Support for other devices
will be added later.

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

Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
---
New in v9:
   - Reduce the support to 82599 and x540 devices only.
   - Improvements in RETA query code:
      - Implement a "compression" of VF's RETA contents: pass only 2 bits
        per-entry.
      - RETA querying is done in a single mailbox operation thanks to compression.

New in v8:
   - Protect mailbox with a spinlock.

New in v7:
   - Add ixgbe_mbox_api_12 case in ixgbevf_set_num_queues().
   - Properly expand HW RETA into the ethtool buffer.

New in v6:
   - Add a proper return code when an operation is blocked by PF.

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 |  5 +-
 drivers/net/ethernet/intel/ixgbevf/mbx.h          |  4 ++
 drivers/net/ethernet/intel/ixgbevf/vf.c           | 76 +++++++++++++++++++++++
 drivers/net/ethernet/intel/ixgbevf/vf.h           |  2 +
 4 files changed, 86 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
index 4ee15ad..a16d267 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
+++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
@@ -2030,7 +2030,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;
@@ -2332,6 +2333,7 @@ static void ixgbevf_set_num_queues(struct ixgbevf_adapter *adapter)
 
 		switch (hw->api_version) {
 		case ixgbe_mbox_api_11:
+		case ixgbe_mbox_api_12:
 			adapter->num_rx_queues = rss;
 			adapter->num_tx_queues = rss;
 		default:
@@ -3712,6 +3714,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 6253e93..66e138b 100644
--- a/drivers/net/ethernet/intel/ixgbevf/mbx.h
+++ b/drivers/net/ethernet/intel/ixgbevf/mbx.h
@@ -83,6 +83,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 */
 };
@@ -107,6 +108,9 @@ 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 */
+
 /* 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 2614fd3..2676c0b 100644
--- a/drivers/net/ethernet/intel/ixgbevf/vf.c
+++ b/drivers/net/ethernet/intel/ixgbevf/vf.c
@@ -256,6 +256,81 @@ static s32 ixgbevf_set_uc_addr_vf(struct ixgbe_hw *hw, u32 index, u8 *addr)
 	return ret_val;
 }
 
+static inline int ixgbevf_get_reta_locked(struct ixgbe_hw *hw, u32 *msgbuf,
+					  u32 *reta)
+{
+	int err, i, j;
+	u32 *hw_reta = &msgbuf[1];
+
+	/* We have to use a mailbox for 82599 and x540 devices only.
+	 * For these devices RETA has 128 entries.
+	 * Also these VFs support up to 4 RSS queues. Therefore PF will compress
+	 * 16 RETA entries in each DWORD giving 2 bits to each entry.
+	 */
+	int dwords = 128 / 16;
+
+	msgbuf[0] = IXGBE_VF_GET_RETA;
+
+	err = hw->mbx.ops.write_posted(hw, msgbuf, 1);
+
+	if (err)
+		return err;
+
+	err = hw->mbx.ops.read_posted(hw, msgbuf, dwords + 1);
+
+	if (err)
+		return err;
+
+	msgbuf[0] &= ~IXGBE_VT_MSGTYPE_CTS;
+
+	/* If the operation has been refused by a PF return -EPERM */
+	if (msgbuf[0] == (IXGBE_VF_GET_RETA | IXGBE_VT_MSGTYPE_NACK))
+		return -EPERM;
+
+	/* 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;
+
+	for (i = 0; i < dwords; i++)
+		for (j = 0; j < 16; j++)
+			reta[i * 16 + j] = (hw_reta[i] >> (2 * j)) & 0x3;
+
+	return 0;
+}
+
+/**
+ * ixgbevf_get_reta - get the RSS redirection table (RETA) contents.
+ * @adapter: pointer to the port handle
+ * @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 ixgbevf_adapter *adapter, u32 *reta)
+{
+	int err;
+	u32 msgbuf[IXGBE_VFMAILBOX_SIZE];
+
+	/* We support the RSS querying for 82599 and x540 devices only.
+	 * Thus return an error if API doesn't support RETA querying or querying
+	 * is not supported for this device type.
+	 */
+	if (adapter->hw.api_version != ixgbe_mbox_api_12 ||
+	    adapter->hw.mac.type >= ixgbe_mac_X550_vf)
+		return -EPERM;
+
+	spin_lock_bh(&adapter->mbx_lock);
+	err = ixgbevf_get_reta_locked(&adapter->hw, msgbuf, reta);
+	spin_unlock_bh(&adapter->mbx_lock);
+
+	return err;
+}
+
 /**
  *  ixgbevf_set_rar_vf - set device MAC address
  *  @hw: pointer to hardware structure
@@ -545,6 +620,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 6688250..7944962 100644
--- a/drivers/net/ethernet/intel/ixgbevf/vf.h
+++ b/drivers/net/ethernet/intel/ixgbevf/vf.h
@@ -38,6 +38,7 @@
 #include "mbx.h"
 
 struct ixgbe_hw;
+struct ixgbevf_adapter;
 
 /* iterator type for walking multicast address lists */
 typedef u8* (*ixgbe_mc_addr_itr) (struct ixgbe_hw *hw, u8 **mc_addr_ptr,
@@ -210,4 +211,5 @@ 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 ixgbevf_adapter *adapter, u32 *reta);
 #endif /* __IXGBE_VF_H__ */
-- 
2.1.0

[PATCH net-next v9 5/7] ixgbe: Add GET_RSS_KEY command to VF-PF channel commands set

[Vlad Zolotarov]

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

Support for other devices will be added later.

Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
---
New in v9:
   - Reduce the support to 82599 and x540 devices only.
   - Get rid of registers access in GET_VF_RSS_KEY flow:
      - Get the RSS HASH Key value from the PF's adapter->rss_key[].

New in v5:
   - Use a newly added netdev op to allow/prevent the RSS Hash Key querying on a per-VF
     basis.

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   |  1 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c | 25 +++++++++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h
index 3522f53..b1e4703 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h
@@ -100,6 +100,7 @@ enum ixgbe_pfvf_api_rev {
 
 /* mailbox API, version 1.2 VF requests */
 #define IXGBE_VF_GET_RETA	0x0a	/* VF request for 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
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
index 8424e7f..4d87458 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
@@ -996,6 +996,28 @@ 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;
+	u32 *rss_key = &msgbuf[1];
+
+	/* verify the PF is supporting the correct API */
+	if (!adapter->vfinfo[vf].rss_query_enabled ||
+	    (adapter->vfinfo[vf].vf_api != ixgbe_mbox_api_12))
+		return -EPERM;
+
+	/* Read the RSS KEY.
+	 * We only support 82599 and x540 devices at the moment.
+	 */
+	if (hw->mac.type >= ixgbe_mac_X550)
+		return -1;
+
+	memcpy(rss_key, adapter->rss_key, sizeof(adapter->rss_key));
+
+	return 0;
+}
+
 static int ixgbe_rcv_msg_from_vf(struct ixgbe_adapter *adapter, u32 vf)
 {
 	u32 mbx_size = IXGBE_VFMAILBOX_SIZE;
@@ -1055,6 +1077,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

[PATCH net-next v9 6/7] ixgbevf: Add RSS Key query code

[Vlad Zolotarov]

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.

This patch adds the support for 82599 and x540 devices only. Support for other
devices will be added later.

Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
---
New in v9:
   - Reduce the support to 82599 and x540 devices only.
   - Added IXGBEVF_RSS_HASH_KEY_SIZE macro.

New in v8:
   - Protect a mailbox access.

New in v6:
   - Return a proper return code when an operation is blocked by PF.

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/ixgbevf.h |  1 +
 drivers/net/ethernet/intel/ixgbevf/mbx.h     |  1 +
 drivers/net/ethernet/intel/ixgbevf/vf.c      | 66 ++++++++++++++++++++++++++++
 drivers/net/ethernet/intel/ixgbevf/vf.h      |  1 +
 4 files changed, 69 insertions(+)

diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h b/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
index bc939a1..6771ae3 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
+++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
@@ -145,6 +145,7 @@ struct ixgbevf_ring {
 #define MAX_RX_QUEUES IXGBE_VF_MAX_RX_QUEUES
 #define MAX_TX_QUEUES IXGBE_VF_MAX_TX_QUEUES
 #define IXGBEVF_MAX_RSS_QUEUES	2
+#define IXGBEVF_RSS_HASH_KEY_SIZE	40
 
 #define IXGBEVF_DEFAULT_TXD	1024
 #define IXGBEVF_DEFAULT_RXD	512
diff --git a/drivers/net/ethernet/intel/ixgbevf/mbx.h b/drivers/net/ethernet/intel/ixgbevf/mbx.h
index 66e138b..82f44e0 100644
--- a/drivers/net/ethernet/intel/ixgbevf/mbx.h
+++ b/drivers/net/ethernet/intel/ixgbevf/mbx.h
@@ -110,6 +110,7 @@ enum ixgbe_pfvf_api_rev {
 
 /* mailbox API, version 1.2 VF requests */
 #define IXGBE_VF_GET_RETA	0x0a	/* VF request for 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
diff --git a/drivers/net/ethernet/intel/ixgbevf/vf.c b/drivers/net/ethernet/intel/ixgbevf/vf.c
index 2676c0b..ec68145 100644
--- a/drivers/net/ethernet/intel/ixgbevf/vf.c
+++ b/drivers/net/ethernet/intel/ixgbevf/vf.c
@@ -301,6 +301,72 @@ static inline int ixgbevf_get_reta_locked(struct ixgbe_hw *hw, u32 *msgbuf,
 	return 0;
 }
 
+static inline int ixgbevf_get_rss_key_locked(struct ixgbe_hw *hw, u8 *rss_key)
+{
+	int err;
+	u32 msgbuf[IXGBE_VFMAILBOX_SIZE];
+
+	/* We currently support the RSS Random Key retrieval for 82599 and x540
+	 * devices only.
+	 *
+	 * Thus return an error if API doesn't support RSS Random Key retrieval
+	 * or if the operation is not supported for this device type.
+	 */
+	if (hw->api_version != ixgbe_mbox_api_12 ||
+	    hw->mac.type >= ixgbe_mac_X550_vf)
+		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 the operation has been refused by a PF return -EPERM */
+	if (msgbuf[0] == (IXGBE_VF_GET_RETA | IXGBE_VT_MSGTYPE_NACK))
+		return -EPERM;
+
+	/* 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, IXGBEVF_RSS_HASH_KEY_SIZE);
+
+	return 0;
+}
+
+/**
+ * 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.
+ * Ensures the atomicy of a mailbox access using the adapter.mbx_lock spinlock.
+ *
+ * Returns: 0 on success.
+ *          if API doesn't support this operation - (-EPERM).
+ */
+int ixgbevf_get_rss_key(struct ixgbevf_adapter *a, u8 *rss_key)
+{
+	int rc;
+
+	spin_lock_bh(&a->mbx_lock);
+	rc = ixgbevf_get_rss_key_locked(&a->hw, rss_key);
+	spin_unlock_bh(&a->mbx_lock);
+
+	return rc;
+}
+
 /**
  * ixgbevf_get_reta - get the RSS redirection table (RETA) contents.
  * @adapter: pointer to the port handle
diff --git a/drivers/net/ethernet/intel/ixgbevf/vf.h b/drivers/net/ethernet/intel/ixgbevf/vf.h
index 7944962..a054716 100644
--- a/drivers/net/ethernet/intel/ixgbevf/vf.h
+++ b/drivers/net/ethernet/intel/ixgbevf/vf.h
@@ -212,4 +212,5 @@ 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 ixgbevf_adapter *adapter, u32 *reta);
+int ixgbevf_get_rss_key(struct ixgbevf_adapter *adapter, u8 *rss_key);
 #endif /* __IXGBE_VF_H__ */
-- 
2.1.0

[PATCH net-next v9 7/7] ixgbevf: 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 for VF devices.

This patch adds the support for 82599 and x540 devices only. Support for other
devices will be added later.

Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
---
New in v9:
   - Use IXGBEVF_RSS_HASH_KEY_SIZE macro.

New in v6:
   - Added a required get_rxnfc callback to ixgbevf_ethtool_ops.

New in v4:
   - Removed not needed braces in if-statement in ixgbevf_get_rxfh_indir_size().

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 | 58 ++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/drivers/net/ethernet/intel/ixgbevf/ethtool.c b/drivers/net/ethernet/intel/ixgbevf/ethtool.c
index e83c85b..4d2f59f 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ethtool.c
+++ b/drivers/net/ethernet/intel/ixgbevf/ethtool.c
@@ -794,6 +794,60 @@ static int ixgbevf_set_coalesce(struct net_device *netdev,
 	return 0;
 }
 
+static int ixgbevf_get_rxnfc(struct net_device *dev, struct ethtool_rxnfc *info,
+			     u32 *rules __always_unused)
+{
+	struct ixgbevf_adapter *adapter = netdev_priv(dev);
+
+	switch (info->cmd) {
+	case ETHTOOL_GRXRINGS:
+		info->data = adapter->num_rx_queues;
+		return 0;
+	default:
+		hw_dbg(&adapter->hw, "Command parameters not supported\n");
+		return -EOPNOTSUPP;
+	}
+}
+
+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 IXGBEVF_RSS_HASH_KEY_SIZE;
+}
+
+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, indir);
+		if (err)
+			return err;
+	}
+
+	if (key) {
+		err = ixgbevf_get_rss_key(adapter, 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,
@@ -811,6 +865,10 @@ 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_rxnfc		= ixgbevf_get_rxnfc,
+	.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

Re: [PATCH net-next v9 6/7] ixgbevf: Add RSS Key query code

[Alexander Duyck]

On 03/29/2015 09:11 AM, Vlad Zolotarov wrote:
> 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.
> 
> This patch adds the support for 82599 and x540 devices only. Support for other
> devices will be added later.
> 
> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
> ---
> New in v9:
>    - Reduce the support to 82599 and x540 devices only.
>    - Added IXGBEVF_RSS_HASH_KEY_SIZE macro.
> 
> New in v8:
>    - Protect a mailbox access.
> 
> New in v6:
>    - Return a proper return code when an operation is blocked by PF.
> 
> 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/ixgbevf.h |  1 +
>  drivers/net/ethernet/intel/ixgbevf/mbx.h     |  1 +
>  drivers/net/ethernet/intel/ixgbevf/vf.c      | 66 ++++++++++++++++++++++++++++
>  drivers/net/ethernet/intel/ixgbevf/vf.h      |  1 +
>  4 files changed, 69 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h b/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
> index bc939a1..6771ae3 100644
> --- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
> +++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
> @@ -145,6 +145,7 @@ struct ixgbevf_ring {
>  #define MAX_RX_QUEUES IXGBE_VF_MAX_RX_QUEUES
>  #define MAX_TX_QUEUES IXGBE_VF_MAX_TX_QUEUES
>  #define IXGBEVF_MAX_RSS_QUEUES	2
> +#define IXGBEVF_RSS_HASH_KEY_SIZE	40
>  
>  #define IXGBEVF_DEFAULT_TXD	1024
>  #define IXGBEVF_DEFAULT_RXD	512
> diff --git a/drivers/net/ethernet/intel/ixgbevf/mbx.h b/drivers/net/ethernet/intel/ixgbevf/mbx.h
> index 66e138b..82f44e0 100644
> --- a/drivers/net/ethernet/intel/ixgbevf/mbx.h
> +++ b/drivers/net/ethernet/intel/ixgbevf/mbx.h
> @@ -110,6 +110,7 @@ enum ixgbe_pfvf_api_rev {
>  
>  /* mailbox API, version 1.2 VF requests */
>  #define IXGBE_VF_GET_RETA	0x0a	/* VF request for 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
> diff --git a/drivers/net/ethernet/intel/ixgbevf/vf.c b/drivers/net/ethernet/intel/ixgbevf/vf.c
> index 2676c0b..ec68145 100644
> --- a/drivers/net/ethernet/intel/ixgbevf/vf.c
> +++ b/drivers/net/ethernet/intel/ixgbevf/vf.c
> @@ -301,6 +301,72 @@ static inline int ixgbevf_get_reta_locked(struct ixgbe_hw *hw, u32 *msgbuf,
>  	return 0;
>  }
>  
> +static inline int ixgbevf_get_rss_key_locked(struct ixgbe_hw *hw, u8 *rss_key)
> +{
> +	int err;
> +	u32 msgbuf[IXGBE_VFMAILBOX_SIZE];
> +
> +	/* We currently support the RSS Random Key retrieval for 82599 and x540
> +	 * devices only.
> +	 *
> +	 * Thus return an error if API doesn't support RSS Random Key retrieval
> +	 * or if the operation is not supported for this device type.
> +	 */
> +	if (hw->api_version != ixgbe_mbox_api_12 ||
> +	    hw->mac.type >= ixgbe_mac_X550_vf)
> +		return -EPERM;
> +

The return type here should be not supported, or IXGBE_NOT_IMPLEMENTED,
not no permissions.

> +	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 the operation has been refused by a PF return -EPERM */
> +	if (msgbuf[0] == (IXGBE_VF_GET_RETA | IXGBE_VT_MSGTYPE_NACK))
> +		return -EPERM;
> +
> +	/* 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, IXGBEVF_RSS_HASH_KEY_SIZE);
> +
> +	return 0;
> +}
> +
> +/**
> + * 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.
> + * Ensures the atomicy of a mailbox access using the adapter.mbx_lock spinlock.
> + *
> + * Returns: 0 on success.
> + *          if API doesn't support this operation - (-EPERM).
> + */
> +int ixgbevf_get_rss_key(struct ixgbevf_adapter *a, u8 *rss_key)
> +{
> +	int rc;
> +
> +	spin_lock_bh(&a->mbx_lock);
> +	rc = ixgbevf_get_rss_key_locked(&a->hw, rss_key);
> +	spin_unlock_bh(&a->mbx_lock);
> +
> +	return rc;
> +}
> +

Since there is currently no cases where you would be getting the rss_key
without the RETA you might just want to combine this function with
ixgbevf_get_reta so you only take the lock once and process both
messages in one pass instead of having to bounce the spinlock.

>  /**
>   * ixgbevf_get_reta - get the RSS redirection table (RETA) contents.
>   * @adapter: pointer to the port handle
> diff --git a/drivers/net/ethernet/intel/ixgbevf/vf.h b/drivers/net/ethernet/intel/ixgbevf/vf.h
> index 7944962..a054716 100644
> --- a/drivers/net/ethernet/intel/ixgbevf/vf.h
> +++ b/drivers/net/ethernet/intel/ixgbevf/vf.h
> @@ -212,4 +212,5 @@ 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 ixgbevf_adapter *adapter, u32 *reta);
> +int ixgbevf_get_rss_key(struct ixgbevf_adapter *adapter, u8 *rss_key);
>  #endif /* __IXGBE_VF_H__ */
> 

Re: [PATCH net-next v9 5/7] ixgbe: Add GET_RSS_KEY command to VF-PF channel commands set

[Alexander Duyck]

On 03/29/2015 09:11 AM, Vlad Zolotarov wrote:
> For 82599 and x540 VFs and PF share the same RSS Key. Therefore we will return
> the same RSS key for all VFs.
> 
> Support for other devices will be added later.
> 
> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
> ---
> New in v9:
>    - Reduce the support to 82599 and x540 devices only.
>    - Get rid of registers access in GET_VF_RSS_KEY flow:
>       - Get the RSS HASH Key value from the PF's adapter->rss_key[].
> 
> New in v5:
>    - Use a newly added netdev op to allow/prevent the RSS Hash Key querying on a per-VF
>      basis.
> 
> 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   |  1 +
>  drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c | 25 +++++++++++++++++++++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h
> index 3522f53..b1e4703 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h
> @@ -100,6 +100,7 @@ enum ixgbe_pfvf_api_rev {
>  
>  /* mailbox API, version 1.2 VF requests */
>  #define IXGBE_VF_GET_RETA	0x0a	/* VF request for 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
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
> index 8424e7f..4d87458 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
> @@ -996,6 +996,28 @@ 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;
> +	u32 *rss_key = &msgbuf[1];
> +
> +	/* verify the PF is supporting the correct API */
> +	if (!adapter->vfinfo[vf].rss_query_enabled ||
> +	    (adapter->vfinfo[vf].vf_api != ixgbe_mbox_api_12))
> +		return -EPERM;
> +
> +	/* Read the RSS KEY.
> +	 * We only support 82599 and x540 devices at the moment.
> +	 */
> +	if (hw->mac.type >= ixgbe_mac_X550)
> +		return -1;
> +

The return should be not supported, not -1 since that is too ambiguous.

> +	memcpy(rss_key, adapter->rss_key, sizeof(adapter->rss_key));
> +
> +	return 0;
> +}
> +
>  static int ixgbe_rcv_msg_from_vf(struct ixgbe_adapter *adapter, u32 vf)
>  {
>  	u32 mbx_size = IXGBE_VFMAILBOX_SIZE;
> @@ -1055,6 +1077,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;
> 

Re: [PATCH net-next v9 7/7] ixgbevf: Add the appropriate ethtool ops to query RSS indirection table and key

[Alexander Duyck]

On 03/29/2015 09:11 AM, Vlad Zolotarov wrote:
> 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 for VF devices.
> 
> This patch adds the support for 82599 and x540 devices only. Support for other
> devices will be added later.
> 
> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
> ---
> New in v9:
>    - Use IXGBEVF_RSS_HASH_KEY_SIZE macro.
> 
> New in v6:
>    - Added a required get_rxnfc callback to ixgbevf_ethtool_ops.
> 
> New in v4:
>    - Removed not needed braces in if-statement in ixgbevf_get_rxfh_indir_size().
> 
> 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 | 58 ++++++++++++++++++++++++++++
>  1 file changed, 58 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbevf/ethtool.c b/drivers/net/ethernet/intel/ixgbevf/ethtool.c
> index e83c85b..4d2f59f 100644
> --- a/drivers/net/ethernet/intel/ixgbevf/ethtool.c
> +++ b/drivers/net/ethernet/intel/ixgbevf/ethtool.c
> @@ -794,6 +794,60 @@ static int ixgbevf_set_coalesce(struct net_device *netdev,
>  	return 0;
>  }
>  
> +static int ixgbevf_get_rxnfc(struct net_device *dev, struct ethtool_rxnfc *info,
> +			     u32 *rules __always_unused)
> +{
> +	struct ixgbevf_adapter *adapter = netdev_priv(dev);
> +
> +	switch (info->cmd) {
> +	case ETHTOOL_GRXRINGS:
> +		info->data = adapter->num_rx_queues;
> +		return 0;
> +	default:
> +		hw_dbg(&adapter->hw, "Command parameters not supported\n");
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +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;
> +}
> +

You should return 0 for x550, not 64 since it isn't supported.

My suggestion would be to alter the code so that you return 128 for
everything <= x540_vf, and place a return 0 as the default return
without a need for the if statement.

> +static u32 ixgbevf_get_rxfh_key_size(struct net_device *netdev)
> +{
> +	return IXGBEVF_RSS_HASH_KEY_SIZE;
> +}
> +

Same thing here.  If you don't support this on x550 you should probably
return 0 instead of returning a value.

> +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, indir);
> +		if (err)
> +			return err;
> +	}
> +
> +	if (key) {
> +		err = ixgbevf_get_rss_key(adapter, key);
> +		if (err)
> +			return err;
> +	}
> +
> +	return 0;
> +}
> +

My advice here would be to just wrap these two functions in one mailbox
lock and update the second check so that it is if (!err && key) so that
you can simplify the path and just return err at the end.

>  static const struct ethtool_ops ixgbevf_ethtool_ops = {
>  	.get_settings		= ixgbevf_get_settings,
>  	.get_drvinfo		= ixgbevf_get_drvinfo,
> @@ -811,6 +865,10 @@ 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_rxnfc		= ixgbevf_get_rxnfc,
> +	.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)
> 

Re: [PATCH net-next v9 2/7] ixgbe: Add a new netdev op to allow/prevent a VF from querying an RSS info

[Alexander Duyck]

On 03/29/2015 09:11 AM, Vlad Zolotarov wrote:
> Implements the new netdev op to allow user to enable/disable the ability
> of a specific VF to query its RSS Indirection Table and an RSS Hash Key.
>
> This patch limits the new feature support to 82599 and x540 devices only.
> Support for other devices will be added later.
>
> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
> ---
> New in v9:
>    - Reduce the support to 82599 and x540 devices only.
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe.h       |  1 +
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c  |  7 ++++++
>  drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c | 30 ++++++++++++++++++++++++++
>  drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h |  2 ++
>  4 files changed, 40 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> index 42ed4b4..639aa1b 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> @@ -151,6 +151,7 @@ struct vf_data_storage {
>  	u16 tx_rate;
>  	u16 vlan_count;
>  	u8 spoofchk_enabled;
> +	bool rss_query_enabled;
>  	unsigned int vf_api;
>  };
>  
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index a99bc5d..1b04cb1 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -3656,6 +3656,12 @@ static void ixgbe_configure_virtualization(struct ixgbe_adapter *adapter)
>  		if (hw->mac.ops.set_ethertype_anti_spoofing)
>  			hw->mac.ops.set_ethertype_anti_spoofing(hw, true, i);
>  	}
> +
> +	/* Enable/Disable RSS query feature  */
> +	for (i = 0; i < adapter->num_vfs; i++)
> +		ixgbe_ndo_set_vf_rss_query_en(adapter->netdev, i,
> +					      adapter->vfinfo[i].rss_query_enabled);
> +
>  }
>  
>  static void ixgbe_set_rx_buffer_len(struct ixgbe_adapter *adapter)

Instead of looking through the VFs twice you could probably look at
combining  this with the loop directly above it.
> @@ -8096,6 +8102,7 @@ static const struct net_device_ops ixgbe_netdev_ops = {
>  	.ndo_set_vf_vlan	= ixgbe_ndo_set_vf_vlan,
>  	.ndo_set_vf_rate	= ixgbe_ndo_set_vf_bw,
>  	.ndo_set_vf_spoofchk	= ixgbe_ndo_set_vf_spoofchk,
> +	.ndo_set_vf_rss_query_en = ixgbe_ndo_set_vf_rss_query_en,
>  	.ndo_get_vf_config	= ixgbe_ndo_get_vf_config,
>  	.ndo_get_stats64	= ixgbe_get_stats64,
>  #ifdef CONFIG_IXGBE_DCB
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
> index 09a291b..f08672a 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
> @@ -108,6 +108,15 @@ static int __ixgbe_enable_sriov(struct ixgbe_adapter *adapter)
>  		/* enable spoof checking for all VFs */
>  		for (i = 0; i < adapter->num_vfs; i++)
>  			adapter->vfinfo[i].spoofchk_enabled = true;
> +
> +		/* We support VF RSS querying only for 82599 and x540 devices at
> +		 * the moment. These devices share RSS indirection table and
> +		 * RSS hash key with PF therefore we want to disable the
> +		 * querying by default.
> +		 */
> +		for (i = 0; i < adapter->num_vfs; i++)
> +			adapter->vfinfo[i].rss_query_enabled = 0;
> +
>  		return 0;
>  	}
>  

Same here, no point in looping through all the VFs twice, just take care
of sppofchk_enabled, and rss_query_enabled in one loop.

> @@ -1330,6 +1339,26 @@ int ixgbe_ndo_set_vf_spoofchk(struct net_device *netdev, int vf, bool setting)
>  	return 0;
>  }
>  
> +int ixgbe_ndo_set_vf_rss_query_en(struct net_device *netdev, int vf,
> +				  bool setting)
> +{
> +	struct ixgbe_adapter *adapter = netdev_priv(netdev);
> +
> +	/* This operation is currently supported only for 82599 and x540
> +	 * devices.
> +	 */
> +	if (adapter->hw.mac.type < ixgbe_mac_82599EB ||
> +	    adapter->hw.mac.type >= ixgbe_mac_X550)
> +		return -EPERM;
> +

This should be not supported, not EPERM.

> +	if (vf >= adapter->num_vfs)
> +		return -EINVAL;
> +
> +	adapter->vfinfo[vf].rss_query_enabled = setting;
> +
> +	return 0;
> +}
> +
>  int ixgbe_ndo_get_vf_config(struct net_device *netdev,
>  			    int vf, struct ifla_vf_info *ivi)
>  {
> @@ -1343,5 +1372,6 @@ int ixgbe_ndo_get_vf_config(struct net_device *netdev,
>  	ivi->vlan = adapter->vfinfo[vf].pf_vlan;
>  	ivi->qos = adapter->vfinfo[vf].pf_qos;
>  	ivi->spoofchk = adapter->vfinfo[vf].spoofchk_enabled;
> +	ivi->rss_query_en = adapter->vfinfo[vf].rss_query_enabled;
>  	return 0;
>  }
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h
> index 32c26d5..2c197e6 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h
> @@ -47,6 +47,8 @@ int ixgbe_ndo_set_vf_vlan(struct net_device *netdev, int queue, u16 vlan,
>  int ixgbe_ndo_set_vf_bw(struct net_device *netdev, int vf, int min_tx_rate,
>  			int max_tx_rate);
>  int ixgbe_ndo_set_vf_spoofchk(struct net_device *netdev, int vf, bool setting);
> +int ixgbe_ndo_set_vf_rss_query_en(struct net_device *netdev, int vf,
> +				  bool setting);
>  int ixgbe_ndo_get_vf_config(struct net_device *netdev,
>  			    int vf, struct ifla_vf_info *ivi);
>  void ixgbe_check_vf_rate_limit(struct ixgbe_adapter *adapter);

Re: [PATCH net-next v9 3/7] ixgbe: Add a RETA query command to VF-PF channel API

[Alexander Duyck]

On 03/29/2015 09:11 AM, Vlad Zolotarov wrote:
> Add this new command for 82599 and x540 devices only. Support for other devices
> will be added later.
> 
> 82599 and x540 VFs and PF share the same RSS redirection table (RETA). Therefore we
> just return it for all VFs.
> 
> 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.
> 
> 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 <vladz@cloudius-systems.com>
> ---
> New in v9:
>    - Reduce the support to 82599 and x540 devices only.
>    - Improvements in query RETA VF-PF command implementation:
>       - Use the cached RETA contents.
>       - Compress the mailbox message.
> 
> New in v5:
>    - Use the newly added netdev op to allow/prevent the RETA query on a per-VF basis.
> 
> New in v4:
>    - Deleted an empty line in ixgbe_get_vf_reta() switch-case.
> 
> 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   |  4 +++
>  drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c | 49 ++++++++++++++++++++++++++
>  2 files changed, 53 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h
> index a5cb755..3522f53 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,9 @@ 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 */
> +
>  /* 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 f08672a..8424e7f 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
> @@ -433,6 +433,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
> @@ -900,6 +901,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:
> @@ -923,6 +925,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;
> @@ -950,6 +953,49 @@ 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;
> +	u32 i, j;
> +	u32 *out_buf = &msgbuf[1];
> +	const u8 *reta = adapter->rss_indir_tbl;
> +	u8 mask = 0;
> +	u32 psrtype;
> +	u32 reta_size = ixgbe_rss_indir_tbl_entries(adapter);
> +
> +	/* verify the PF is supporting the correct API */
> +	if (!adapter->vfinfo[vf].rss_query_enabled ||
> +	    (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
> +	 *
> +	 * 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 = 0x01;
> +	else if ((psrtype & (2 << 29)) == (2 << 29))
> +		mask = 0x03;
> +

The psrtype and mask should be dropped.  Any masking should happen on
the VF end as the VF controls the psrtype register and the value could
change depending on the VF settings.  You should just be using 0x3 as
the mask always since you are encoding 2 bits per entry in the buffer.

> +	/* Compress the RETA by saving only 2 bits from each entry. This way
> +	 * we will be able to transfer the whole RETA in a single mailbox
> +	 * operation.
> +	 */
> +	for (i = 0; i < reta_size / 16; i++) {
> +		out_buf[i] = 0;
> +		for (j = 0; j < 16; j++)
> +			out_buf[i] |= (u32)(reta[16 * i + j] & mask) << (2 * j);
> +	}
> +
> +	return 0;
> +}
> +
>  static int ixgbe_rcv_msg_from_vf(struct ixgbe_adapter *adapter, u32 vf)
>  {
>  	u32 mbx_size = IXGBE_VFMAILBOX_SIZE;
> @@ -1006,6 +1052,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;
> 

Re: [PATCH net-next v9 4/7] ixgbevf: Add a RETA query code

[Alexander Duyck]

On 03/29/2015 09:11 AM, Vlad Zolotarov wrote:
> We will currently support only 82599 and x540 deviced. Support for other devices
> will be added later.
> 
>    - Added a new API version support.
>    - Added the query implementation in the ixgbevf.
> 
> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
> ---
> New in v9:
>    - Reduce the support to 82599 and x540 devices only.
>    - Improvements in RETA query code:
>       - Implement a "compression" of VF's RETA contents: pass only 2 bits
>         per-entry.
>       - RETA querying is done in a single mailbox operation thanks to compression.
> 
> New in v8:
>    - Protect mailbox with a spinlock.
> 
> New in v7:
>    - Add ixgbe_mbox_api_12 case in ixgbevf_set_num_queues().
>    - Properly expand HW RETA into the ethtool buffer.
> 
> New in v6:
>    - Add a proper return code when an operation is blocked by PF.
> 
> 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 |  5 +-
>  drivers/net/ethernet/intel/ixgbevf/mbx.h          |  4 ++
>  drivers/net/ethernet/intel/ixgbevf/vf.c           | 76 +++++++++++++++++++++++
>  drivers/net/ethernet/intel/ixgbevf/vf.h           |  2 +
>  4 files changed, 86 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
> index 4ee15ad..a16d267 100644
> --- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
> +++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
> @@ -2030,7 +2030,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;
> @@ -2332,6 +2333,7 @@ static void ixgbevf_set_num_queues(struct ixgbevf_adapter *adapter)
>  
>  		switch (hw->api_version) {
>  		case ixgbe_mbox_api_11:
> +		case ixgbe_mbox_api_12:
>  			adapter->num_rx_queues = rss;
>  			adapter->num_tx_queues = rss;
>  		default:
> @@ -3712,6 +3714,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 6253e93..66e138b 100644
> --- a/drivers/net/ethernet/intel/ixgbevf/mbx.h
> +++ b/drivers/net/ethernet/intel/ixgbevf/mbx.h
> @@ -83,6 +83,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 */
>  };
> @@ -107,6 +108,9 @@ 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 */
> +
>  /* 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 2614fd3..2676c0b 100644
> --- a/drivers/net/ethernet/intel/ixgbevf/vf.c
> +++ b/drivers/net/ethernet/intel/ixgbevf/vf.c
> @@ -256,6 +256,81 @@ static s32 ixgbevf_set_uc_addr_vf(struct ixgbe_hw *hw, u32 index, u8 *addr)
>  	return ret_val;
>  }
>  
> +static inline int ixgbevf_get_reta_locked(struct ixgbe_hw *hw, u32 *msgbuf,
> +					  u32 *reta)
> +{
> +	int err, i, j;
> +	u32 *hw_reta = &msgbuf[1];
> +
> +	/* We have to use a mailbox for 82599 and x540 devices only.
> +	 * For these devices RETA has 128 entries.
> +	 * Also these VFs support up to 4 RSS queues. Therefore PF will compress
> +	 * 16 RETA entries in each DWORD giving 2 bits to each entry.
> +	 */
> +	int dwords = 128 / 16;
> +
> +	msgbuf[0] = IXGBE_VF_GET_RETA;
> +
> +	err = hw->mbx.ops.write_posted(hw, msgbuf, 1);
> +
> +	if (err)
> +		return err;
> +
> +	err = hw->mbx.ops.read_posted(hw, msgbuf, dwords + 1);
> +
> +	if (err)
> +		return err;
> +
> +	msgbuf[0] &= ~IXGBE_VT_MSGTYPE_CTS;
> +
> +	/* If the operation has been refused by a PF return -EPERM */
> +	if (msgbuf[0] == (IXGBE_VF_GET_RETA | IXGBE_VT_MSGTYPE_NACK))
> +		return -EPERM;
> +
> +	/* 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;
> +
> +	for (i = 0; i < dwords; i++)
> +		for (j = 0; j < 16; j++)
> +			reta[i * 16 + j] = (hw_reta[i] >> (2 * j)) & 0x3;
> +
> +	return 0;
> +}
> +
> +/**
> + * ixgbevf_get_reta - get the RSS redirection table (RETA) contents.
> + * @adapter: pointer to the port handle
> + * @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 ixgbevf_adapter *adapter, u32 *reta)
> +{
> +	int err;
> +	u32 msgbuf[IXGBE_VFMAILBOX_SIZE];
> +
> +	/* We support the RSS querying for 82599 and x540 devices only.
> +	 * Thus return an error if API doesn't support RETA querying or querying
> +	 * is not supported for this device type.
> +	 */
> +	if (adapter->hw.api_version != ixgbe_mbox_api_12 ||
> +	    adapter->hw.mac.type >= ixgbe_mac_X550_vf)
> +		return -EPERM;
> +
> +	spin_lock_bh(&adapter->mbx_lock);
> +	err = ixgbevf_get_reta_locked(&adapter->hw, msgbuf, reta);
> +	spin_unlock_bh(&adapter->mbx_lock);
> +
> +	return err;
> +}
> +

You really shouldn't be pushing the lock down to this level.  The caller
should be one to hold the mailbox lock.  Also the functions at this
level are normally only passed the hardware struct, not the adapter
struct as most of this code is meant to be usable in other OSes such as
FreeBSD and the Linux spin_lock calls are not generic across OSes.

>  /**
>   *  ixgbevf_set_rar_vf - set device MAC address
>   *  @hw: pointer to hardware structure
> @@ -545,6 +620,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 6688250..7944962 100644
> --- a/drivers/net/ethernet/intel/ixgbevf/vf.h
> +++ b/drivers/net/ethernet/intel/ixgbevf/vf.h
> @@ -38,6 +38,7 @@
>  #include "mbx.h"
>  
>  struct ixgbe_hw;
> +struct ixgbevf_adapter;
>  
>  /* iterator type for walking multicast address lists */
>  typedef u8* (*ixgbe_mc_addr_itr) (struct ixgbe_hw *hw, u8 **mc_addr_ptr,
> @@ -210,4 +211,5 @@ 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 ixgbevf_adapter *adapter, u32 *reta);
>  #endif /* __IXGBE_VF_H__ */
> 

Re: [PATCH net-next v9 2/7] ixgbe: Add a new netdev op to allow/prevent a VF from querying an RSS info

[Vlad Zolotarov]

On 03/30/15 01:32, Alexander Duyck wrote:
> On 03/29/2015 09:11 AM, Vlad Zolotarov wrote:
>> Implements the new netdev op to allow user to enable/disable the ability
>> of a specific VF to query its RSS Indirection Table and an RSS Hash Key.
>>
>> This patch limits the new feature support to 82599 and x540 devices only.
>> Support for other devices will be added later.
>>
>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>> ---
>> New in v9:
>>     - Reduce the support to 82599 and x540 devices only.
>> ---
>>   drivers/net/ethernet/intel/ixgbe/ixgbe.h       |  1 +
>>   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c  |  7 ++++++
>>   drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c | 30 ++++++++++++++++++++++++++
>>   drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h |  2 ++
>>   4 files changed, 40 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> index 42ed4b4..639aa1b 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> @@ -151,6 +151,7 @@ struct vf_data_storage {
>>   	u16 tx_rate;
>>   	u16 vlan_count;
>>   	u8 spoofchk_enabled;
>> +	bool rss_query_enabled;
>>   	unsigned int vf_api;
>>   };
>>   
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> index a99bc5d..1b04cb1 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> @@ -3656,6 +3656,12 @@ static void ixgbe_configure_virtualization(struct ixgbe_adapter *adapter)
>>   		if (hw->mac.ops.set_ethertype_anti_spoofing)
>>   			hw->mac.ops.set_ethertype_anti_spoofing(hw, true, i);
>>   	}
>> +
>> +	/* Enable/Disable RSS query feature  */
>> +	for (i = 0; i < adapter->num_vfs; i++)
>> +		ixgbe_ndo_set_vf_rss_query_en(adapter->netdev, i,
>> +					      adapter->vfinfo[i].rss_query_enabled);
>> +
>>   }
>>   
>>   static void ixgbe_set_rx_buffer_len(struct ixgbe_adapter *adapter)
> Instead of looking through the VFs twice you could probably look at
> combining  this with the loop directly above it.
>> @@ -8096,6 +8102,7 @@ static const struct net_device_ops ixgbe_netdev_ops = {
>>   	.ndo_set_vf_vlan	= ixgbe_ndo_set_vf_vlan,
>>   	.ndo_set_vf_rate	= ixgbe_ndo_set_vf_bw,
>>   	.ndo_set_vf_spoofchk	= ixgbe_ndo_set_vf_spoofchk,
>> +	.ndo_set_vf_rss_query_en = ixgbe_ndo_set_vf_rss_query_en,
>>   	.ndo_get_vf_config	= ixgbe_ndo_get_vf_config,
>>   	.ndo_get_stats64	= ixgbe_get_stats64,
>>   #ifdef CONFIG_IXGBE_DCB
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
>> index 09a291b..f08672a 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
>> @@ -108,6 +108,15 @@ static int __ixgbe_enable_sriov(struct ixgbe_adapter *adapter)
>>   		/* enable spoof checking for all VFs */
>>   		for (i = 0; i < adapter->num_vfs; i++)
>>   			adapter->vfinfo[i].spoofchk_enabled = true;
>> +
>> +		/* We support VF RSS querying only for 82599 and x540 devices at
>> +		 * the moment. These devices share RSS indirection table and
>> +		 * RSS hash key with PF therefore we want to disable the
>> +		 * querying by default.
>> +		 */
>> +		for (i = 0; i < adapter->num_vfs; i++)
>> +			adapter->vfinfo[i].rss_query_enabled = 0;
>> +
>>   		return 0;
>>   	}
>>   
> Same here, no point in looping through all the VFs twice, just take care
> of sppofchk_enabled, and rss_query_enabled in one loop.
>
>> @@ -1330,6 +1339,26 @@ int ixgbe_ndo_set_vf_spoofchk(struct net_device *netdev, int vf, bool setting)
>>   	return 0;
>>   }
>>   
>> +int ixgbe_ndo_set_vf_rss_query_en(struct net_device *netdev, int vf,
>> +				  bool setting)
>> +{
>> +	struct ixgbe_adapter *adapter = netdev_priv(netdev);
>> +
>> +	/* This operation is currently supported only for 82599 and x540
>> +	 * devices.
>> +	 */
>> +	if (adapter->hw.mac.type < ixgbe_mac_82599EB ||
>> +	    adapter->hw.mac.type >= ixgbe_mac_X550)
>> +		return -EPERM;
>> +
> This should be not supported, not EPERM.

Agree with all the above. Fixing in v10.

>
>> +	if (vf >= adapter->num_vfs)
>> +		return -EINVAL;
>> +
>> +	adapter->vfinfo[vf].rss_query_enabled = setting;
>> +
>> +	return 0;
>> +}
>> +
>>   int ixgbe_ndo_get_vf_config(struct net_device *netdev,
>>   			    int vf, struct ifla_vf_info *ivi)
>>   {
>> @@ -1343,5 +1372,6 @@ int ixgbe_ndo_get_vf_config(struct net_device *netdev,
>>   	ivi->vlan = adapter->vfinfo[vf].pf_vlan;
>>   	ivi->qos = adapter->vfinfo[vf].pf_qos;
>>   	ivi->spoofchk = adapter->vfinfo[vf].spoofchk_enabled;
>> +	ivi->rss_query_en = adapter->vfinfo[vf].rss_query_enabled;
>>   	return 0;
>>   }
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h
>> index 32c26d5..2c197e6 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h
>> @@ -47,6 +47,8 @@ int ixgbe_ndo_set_vf_vlan(struct net_device *netdev, int queue, u16 vlan,
>>   int ixgbe_ndo_set_vf_bw(struct net_device *netdev, int vf, int min_tx_rate,
>>   			int max_tx_rate);
>>   int ixgbe_ndo_set_vf_spoofchk(struct net_device *netdev, int vf, bool setting);
>> +int ixgbe_ndo_set_vf_rss_query_en(struct net_device *netdev, int vf,
>> +				  bool setting);
>>   int ixgbe_ndo_get_vf_config(struct net_device *netdev,
>>   			    int vf, struct ifla_vf_info *ivi);
>>   void ixgbe_check_vf_rate_limit(struct ixgbe_adapter *adapter);

Re: [PATCH net-next v9 3/7] ixgbe: Add a RETA query command to VF-PF channel API

[Vlad Zolotarov]

On 03/30/15 01:45, Alexander Duyck wrote:
> On 03/29/2015 09:11 AM, Vlad Zolotarov wrote:
>> Add this new command for 82599 and x540 devices only. Support for other devices
>> will be added later.
>>
>> 82599 and x540 VFs and PF share the same RSS redirection table (RETA). Therefore we
>> just return it for all VFs.
>>
>> 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.
>>
>> 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 <vladz@cloudius-systems.com>
>> ---
>> New in v9:
>>     - Reduce the support to 82599 and x540 devices only.
>>     - Improvements in query RETA VF-PF command implementation:
>>        - Use the cached RETA contents.
>>        - Compress the mailbox message.
>>
>> New in v5:
>>     - Use the newly added netdev op to allow/prevent the RETA query on a per-VF basis.
>>
>> New in v4:
>>     - Deleted an empty line in ixgbe_get_vf_reta() switch-case.
>>
>> 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   |  4 +++
>>   drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c | 49 ++++++++++++++++++++++++++
>>   2 files changed, 53 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h
>> index a5cb755..3522f53 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,9 @@ 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 */
>> +
>>   /* 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 f08672a..8424e7f 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
>> @@ -433,6 +433,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
>> @@ -900,6 +901,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:
>> @@ -923,6 +925,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;
>> @@ -950,6 +953,49 @@ 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;
>> +	u32 i, j;
>> +	u32 *out_buf = &msgbuf[1];
>> +	const u8 *reta = adapter->rss_indir_tbl;
>> +	u8 mask = 0;
>> +	u32 psrtype;
>> +	u32 reta_size = ixgbe_rss_indir_tbl_entries(adapter);
>> +
>> +	/* verify the PF is supporting the correct API */
>> +	if (!adapter->vfinfo[vf].rss_query_enabled ||
>> +	    (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
>> +	 *
>> +	 * 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 = 0x01;
>> +	else if ((psrtype & (2 << 29)) == (2 << 29))
>> +		mask = 0x03;
>> +
> The psrtype and mask should be dropped.  Any masking should happen on
> the VF end as the VF controls the psrtype register and the value could
> change depending on the VF settings.  You should just be using 0x3 as
> the mask always since you are encoding 2 bits per entry in the buffer.

Namely no masking at all here. Sounds reasonable. I tried to drop 
PSRTYPE reading and then noticed that it's a VF that eventually changes 
it so I reverted it back.
U r right, I should have gone a one step further here.

>
>> +	/* Compress the RETA by saving only 2 bits from each entry. This way
>> +	 * we will be able to transfer the whole RETA in a single mailbox
>> +	 * operation.
>> +	 */
>> +	for (i = 0; i < reta_size / 16; i++) {
>> +		out_buf[i] = 0;
>> +		for (j = 0; j < 16; j++)
>> +			out_buf[i] |= (u32)(reta[16 * i + j] & mask) << (2 * j);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   static int ixgbe_rcv_msg_from_vf(struct ixgbe_adapter *adapter, u32 vf)
>>   {
>>   	u32 mbx_size = IXGBE_VFMAILBOX_SIZE;
>> @@ -1006,6 +1052,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;
>>

Re: [PATCH net-next v9 4/7] ixgbevf: Add a RETA query code

[Vlad Zolotarov]

On 03/30/15 01:51, Alexander Duyck wrote:
> On 03/29/2015 09:11 AM, Vlad Zolotarov wrote:
>> We will currently support only 82599 and x540 deviced. Support for other devices
>> will be added later.
>>
>>     - Added a new API version support.
>>     - Added the query implementation in the ixgbevf.
>>
>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>> ---
>> New in v9:
>>     - Reduce the support to 82599 and x540 devices only.
>>     - Improvements in RETA query code:
>>        - Implement a "compression" of VF's RETA contents: pass only 2 bits
>>          per-entry.
>>        - RETA querying is done in a single mailbox operation thanks to compression.
>>
>> New in v8:
>>     - Protect mailbox with a spinlock.
>>
>> New in v7:
>>     - Add ixgbe_mbox_api_12 case in ixgbevf_set_num_queues().
>>     - Properly expand HW RETA into the ethtool buffer.
>>
>> New in v6:
>>     - Add a proper return code when an operation is blocked by PF.
>>
>> 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 |  5 +-
>>   drivers/net/ethernet/intel/ixgbevf/mbx.h          |  4 ++
>>   drivers/net/ethernet/intel/ixgbevf/vf.c           | 76 +++++++++++++++++++++++
>>   drivers/net/ethernet/intel/ixgbevf/vf.h           |  2 +
>>   4 files changed, 86 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
>> index 4ee15ad..a16d267 100644
>> --- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
>> @@ -2030,7 +2030,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;
>> @@ -2332,6 +2333,7 @@ static void ixgbevf_set_num_queues(struct ixgbevf_adapter *adapter)
>>   
>>   		switch (hw->api_version) {
>>   		case ixgbe_mbox_api_11:
>> +		case ixgbe_mbox_api_12:
>>   			adapter->num_rx_queues = rss;
>>   			adapter->num_tx_queues = rss;
>>   		default:
>> @@ -3712,6 +3714,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 6253e93..66e138b 100644
>> --- a/drivers/net/ethernet/intel/ixgbevf/mbx.h
>> +++ b/drivers/net/ethernet/intel/ixgbevf/mbx.h
>> @@ -83,6 +83,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 */
>>   };
>> @@ -107,6 +108,9 @@ 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 */
>> +
>>   /* 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 2614fd3..2676c0b 100644
>> --- a/drivers/net/ethernet/intel/ixgbevf/vf.c
>> +++ b/drivers/net/ethernet/intel/ixgbevf/vf.c
>> @@ -256,6 +256,81 @@ static s32 ixgbevf_set_uc_addr_vf(struct ixgbe_hw *hw, u32 index, u8 *addr)
>>   	return ret_val;
>>   }
>>   
>> +static inline int ixgbevf_get_reta_locked(struct ixgbe_hw *hw, u32 *msgbuf,
>> +					  u32 *reta)
>> +{
>> +	int err, i, j;
>> +	u32 *hw_reta = &msgbuf[1];
>> +
>> +	/* We have to use a mailbox for 82599 and x540 devices only.
>> +	 * For these devices RETA has 128 entries.
>> +	 * Also these VFs support up to 4 RSS queues. Therefore PF will compress
>> +	 * 16 RETA entries in each DWORD giving 2 bits to each entry.
>> +	 */
>> +	int dwords = 128 / 16;
>> +
>> +	msgbuf[0] = IXGBE_VF_GET_RETA;
>> +
>> +	err = hw->mbx.ops.write_posted(hw, msgbuf, 1);
>> +
>> +	if (err)
>> +		return err;
>> +
>> +	err = hw->mbx.ops.read_posted(hw, msgbuf, dwords + 1);
>> +
>> +	if (err)
>> +		return err;
>> +
>> +	msgbuf[0] &= ~IXGBE_VT_MSGTYPE_CTS;
>> +
>> +	/* If the operation has been refused by a PF return -EPERM */
>> +	if (msgbuf[0] == (IXGBE_VF_GET_RETA | IXGBE_VT_MSGTYPE_NACK))
>> +		return -EPERM;
>> +
>> +	/* 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;
>> +
>> +	for (i = 0; i < dwords; i++)
>> +		for (j = 0; j < 16; j++)
>> +			reta[i * 16 + j] = (hw_reta[i] >> (2 * j)) & 0x3;
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * ixgbevf_get_reta - get the RSS redirection table (RETA) contents.
>> + * @adapter: pointer to the port handle
>> + * @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 ixgbevf_adapter *adapter, u32 *reta)
>> +{
>> +	int err;
>> +	u32 msgbuf[IXGBE_VFMAILBOX_SIZE];
>> +
>> +	/* We support the RSS querying for 82599 and x540 devices only.
>> +	 * Thus return an error if API doesn't support RETA querying or querying
>> +	 * is not supported for this device type.
>> +	 */
>> +	if (adapter->hw.api_version != ixgbe_mbox_api_12 ||
>> +	    adapter->hw.mac.type >= ixgbe_mac_X550_vf)
>> +		return -EPERM;
>> +
>> +	spin_lock_bh(&adapter->mbx_lock);
>> +	err = ixgbevf_get_reta_locked(&adapter->hw, msgbuf, reta);
>> +	spin_unlock_bh(&adapter->mbx_lock);
>> +
>> +	return err;
>> +}
>> +
> You really shouldn't be pushing the lock down to this level.  The caller
> should be one to hold the mailbox lock.  Also the functions at this
> level are normally only passed the hardware struct, not the adapter
> struct as most of this code is meant to be usable in other OSes such as
> FreeBSD and the Linux spin_lock calls are not generic across OSes.

I see... Ok. Let me fix that in v10.

>
>>   /**
>>    *  ixgbevf_set_rar_vf - set device MAC address
>>    *  @hw: pointer to hardware structure
>> @@ -545,6 +620,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 6688250..7944962 100644
>> --- a/drivers/net/ethernet/intel/ixgbevf/vf.h
>> +++ b/drivers/net/ethernet/intel/ixgbevf/vf.h
>> @@ -38,6 +38,7 @@
>>   #include "mbx.h"
>>   
>>   struct ixgbe_hw;
>> +struct ixgbevf_adapter;
>>   
>>   /* iterator type for walking multicast address lists */
>>   typedef u8* (*ixgbe_mc_addr_itr) (struct ixgbe_hw *hw, u8 **mc_addr_ptr,
>> @@ -210,4 +211,5 @@ 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 ixgbevf_adapter *adapter, u32 *reta);
>>   #endif /* __IXGBE_VF_H__ */
>>

Re: [PATCH net-next v9 4/7] ixgbevf: Add a RETA query code

[Vlad Zolotarov]

On 03/30/15 01:51, Alexander Duyck wrote:
> On 03/29/2015 09:11 AM, Vlad Zolotarov wrote:
>> We will currently support only 82599 and x540 deviced. Support for other devices
>> will be added later.
>>
>>     - Added a new API version support.
>>     - Added the query implementation in the ixgbevf.
>>
>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>> ---
>> New in v9:
>>     - Reduce the support to 82599 and x540 devices only.
>>     - Improvements in RETA query code:
>>        - Implement a "compression" of VF's RETA contents: pass only 2 bits
>>          per-entry.
>>        - RETA querying is done in a single mailbox operation thanks to compression.
>>
>> New in v8:
>>     - Protect mailbox with a spinlock.
>>
>> New in v7:
>>     - Add ixgbe_mbox_api_12 case in ixgbevf_set_num_queues().
>>     - Properly expand HW RETA into the ethtool buffer.
>>
>> New in v6:
>>     - Add a proper return code when an operation is blocked by PF.
>>
>> 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 |  5 +-
>>   drivers/net/ethernet/intel/ixgbevf/mbx.h          |  4 ++
>>   drivers/net/ethernet/intel/ixgbevf/vf.c           | 76 +++++++++++++++++++++++
>>   drivers/net/ethernet/intel/ixgbevf/vf.h           |  2 +
>>   4 files changed, 86 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
>> index 4ee15ad..a16d267 100644
>> --- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
>> @@ -2030,7 +2030,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;
>> @@ -2332,6 +2333,7 @@ static void ixgbevf_set_num_queues(struct ixgbevf_adapter *adapter)
>>   
>>   		switch (hw->api_version) {
>>   		case ixgbe_mbox_api_11:
>> +		case ixgbe_mbox_api_12:
>>   			adapter->num_rx_queues = rss;
>>   			adapter->num_tx_queues = rss;
>>   		default:
>> @@ -3712,6 +3714,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 6253e93..66e138b 100644
>> --- a/drivers/net/ethernet/intel/ixgbevf/mbx.h
>> +++ b/drivers/net/ethernet/intel/ixgbevf/mbx.h
>> @@ -83,6 +83,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 */
>>   };
>> @@ -107,6 +108,9 @@ 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 */
>> +
>>   /* 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 2614fd3..2676c0b 100644
>> --- a/drivers/net/ethernet/intel/ixgbevf/vf.c
>> +++ b/drivers/net/ethernet/intel/ixgbevf/vf.c
>> @@ -256,6 +256,81 @@ static s32 ixgbevf_set_uc_addr_vf(struct ixgbe_hw *hw, u32 index, u8 *addr)
>>   	return ret_val;
>>   }
>>   
>> +static inline int ixgbevf_get_reta_locked(struct ixgbe_hw *hw, u32 *msgbuf,
>> +					  u32 *reta)
>> +{
>> +	int err, i, j;
>> +	u32 *hw_reta = &msgbuf[1];
>> +
>> +	/* We have to use a mailbox for 82599 and x540 devices only.
>> +	 * For these devices RETA has 128 entries.
>> +	 * Also these VFs support up to 4 RSS queues. Therefore PF will compress
>> +	 * 16 RETA entries in each DWORD giving 2 bits to each entry.
>> +	 */
>> +	int dwords = 128 / 16;
>> +
>> +	msgbuf[0] = IXGBE_VF_GET_RETA;
>> +
>> +	err = hw->mbx.ops.write_posted(hw, msgbuf, 1);
>> +
>> +	if (err)
>> +		return err;
>> +
>> +	err = hw->mbx.ops.read_posted(hw, msgbuf, dwords + 1);
>> +
>> +	if (err)
>> +		return err;
>> +
>> +	msgbuf[0] &= ~IXGBE_VT_MSGTYPE_CTS;
>> +
>> +	/* If the operation has been refused by a PF return -EPERM */
>> +	if (msgbuf[0] == (IXGBE_VF_GET_RETA | IXGBE_VT_MSGTYPE_NACK))
>> +		return -EPERM;
>> +
>> +	/* 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;
>> +
>> +	for (i = 0; i < dwords; i++)
>> +		for (j = 0; j < 16; j++)
>> +			reta[i * 16 + j] = (hw_reta[i] >> (2 * j)) & 0x3;
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * ixgbevf_get_reta - get the RSS redirection table (RETA) contents.
>> + * @adapter: pointer to the port handle
>> + * @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 ixgbevf_adapter *adapter, u32 *reta)
>> +{
>> +	int err;
>> +	u32 msgbuf[IXGBE_VFMAILBOX_SIZE];
>> +
>> +	/* We support the RSS querying for 82599 and x540 devices only.
>> +	 * Thus return an error if API doesn't support RETA querying or querying
>> +	 * is not supported for this device type.
>> +	 */
>> +	if (adapter->hw.api_version != ixgbe_mbox_api_12 ||
>> +	    adapter->hw.mac.type >= ixgbe_mac_X550_vf)
>> +		return -EPERM;

By the way, since u've commented on the return codes in PATCH02 don't u 
think that we should return -EOPNOTSUPP here too?

>> +
>> +	spin_lock_bh(&adapter->mbx_lock);
>> +	err = ixgbevf_get_reta_locked(&adapter->hw, msgbuf, reta);
>> +	spin_unlock_bh(&adapter->mbx_lock);
>> +
>> +	return err;
>> +}
>> +
> You really shouldn't be pushing the lock down to this level.  The caller
> should be one to hold the mailbox lock.  Also the functions at this
> level are normally only passed the hardware struct, not the adapter
> struct as most of this code is meant to be usable in other OSes such as
> FreeBSD and the Linux spin_lock calls are not generic across OSes.
>
>>   /**
>>    *  ixgbevf_set_rar_vf - set device MAC address
>>    *  @hw: pointer to hardware structure
>> @@ -545,6 +620,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 6688250..7944962 100644
>> --- a/drivers/net/ethernet/intel/ixgbevf/vf.h
>> +++ b/drivers/net/ethernet/intel/ixgbevf/vf.h
>> @@ -38,6 +38,7 @@
>>   #include "mbx.h"
>>   
>>   struct ixgbe_hw;
>> +struct ixgbevf_adapter;
>>   
>>   /* iterator type for walking multicast address lists */
>>   typedef u8* (*ixgbe_mc_addr_itr) (struct ixgbe_hw *hw, u8 **mc_addr_ptr,
>> @@ -210,4 +211,5 @@ 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 ixgbevf_adapter *adapter, u32 *reta);
>>   #endif /* __IXGBE_VF_H__ */
>>

Re: [PATCH net-next v9 5/7] ixgbe: Add GET_RSS_KEY command to VF-PF channel commands set

[Vlad Zolotarov]

On 03/30/15 01:04, Alexander Duyck wrote:
> On 03/29/2015 09:11 AM, Vlad Zolotarov wrote:
>> For 82599 and x540 VFs and PF share the same RSS Key. Therefore we will return
>> the same RSS key for all VFs.
>>
>> Support for other devices will be added later.
>>
>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>> ---
>> New in v9:
>>     - Reduce the support to 82599 and x540 devices only.
>>     - Get rid of registers access in GET_VF_RSS_KEY flow:
>>        - Get the RSS HASH Key value from the PF's adapter->rss_key[].
>>
>> New in v5:
>>     - Use a newly added netdev op to allow/prevent the RSS Hash Key querying on a per-VF
>>       basis.
>>
>> 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   |  1 +
>>   drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c | 25 +++++++++++++++++++++++++
>>   2 files changed, 26 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h
>> index 3522f53..b1e4703 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h
>> @@ -100,6 +100,7 @@ enum ixgbe_pfvf_api_rev {
>>   
>>   /* mailbox API, version 1.2 VF requests */
>>   #define IXGBE_VF_GET_RETA	0x0a	/* VF request for 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
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
>> index 8424e7f..4d87458 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
>> @@ -996,6 +996,28 @@ 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;
>> +	u32 *rss_key = &msgbuf[1];
>> +
>> +	/* verify the PF is supporting the correct API */
>> +	if (!adapter->vfinfo[vf].rss_query_enabled ||
>> +	    (adapter->vfinfo[vf].vf_api != ixgbe_mbox_api_12))
>> +		return -EPERM;
>> +
>> +	/* Read the RSS KEY.
>> +	 * We only support 82599 and x540 devices at the moment.
>> +	 */
>> +	if (hw->mac.type >= ixgbe_mac_X550)
>> +		return -1;
>> +
> The return should be not supported, not -1 since that is too ambiguous.
Right and I think the (adapter->vfinfo[vf].vf_api != ixgbe_mbox_api_12) 
should also return -EOPNOTSUPP.
>
>> +	memcpy(rss_key, adapter->rss_key, sizeof(adapter->rss_key));
>> +
>> +	return 0;
>> +}
>> +
>>   static int ixgbe_rcv_msg_from_vf(struct ixgbe_adapter *adapter, u32 vf)
>>   {
>>   	u32 mbx_size = IXGBE_VFMAILBOX_SIZE;
>> @@ -1055,6 +1077,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;
>>

Re: [PATCH net-next v9 5/7] ixgbe: Add GET_RSS_KEY command to VF-PF channel commands set

[Vlad Zolotarov]

On 03/30/15 01:04, Alexander Duyck wrote:
> On 03/29/2015 09:11 AM, Vlad Zolotarov wrote:
>> For 82599 and x540 VFs and PF share the same RSS Key. Therefore we will return
>> the same RSS key for all VFs.
>>
>> Support for other devices will be added later.
>>
>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>> ---
>> New in v9:
>>     - Reduce the support to 82599 and x540 devices only.
>>     - Get rid of registers access in GET_VF_RSS_KEY flow:
>>        - Get the RSS HASH Key value from the PF's adapter->rss_key[].
>>
>> New in v5:
>>     - Use a newly added netdev op to allow/prevent the RSS Hash Key querying on a per-VF
>>       basis.
>>
>> 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   |  1 +
>>   drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c | 25 +++++++++++++++++++++++++
>>   2 files changed, 26 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h
>> index 3522f53..b1e4703 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h
>> @@ -100,6 +100,7 @@ enum ixgbe_pfvf_api_rev {
>>   
>>   /* mailbox API, version 1.2 VF requests */
>>   #define IXGBE_VF_GET_RETA	0x0a	/* VF request for 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
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
>> index 8424e7f..4d87458 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
>> @@ -996,6 +996,28 @@ 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;
>> +	u32 *rss_key = &msgbuf[1];
>> +
>> +	/* verify the PF is supporting the correct API */
>> +	if (!adapter->vfinfo[vf].rss_query_enabled ||
>> +	    (adapter->vfinfo[vf].vf_api != ixgbe_mbox_api_12))
>> +		return -EPERM;
>> +
>> +	/* Read the RSS KEY.
>> +	 * We only support 82599 and x540 devices at the moment.
>> +	 */
>> +	if (hw->mac.type >= ixgbe_mac_X550)
>> +		return -1;
>> +
> The return should be not supported, not -1 since that is too ambiguous.

On the other hand - why this condition here at all? It will never return 
TRUE anyway since the operation will be blocked by 
adapter->vfinfo[vf].rss_query_enabled
check before since ixgbe_ndo_set_vf_rss_query_en() will prevent the 
rss_query_enabling for the devices newer than x540. So IMHO the right 
thing to do here should be to remove the last "if" at all and split the 
previous one in two so that we will return -EOPNOTSUPP for 
(adapter->vfinfo[vf].vf_api != ixgbe_mbox_api_12) condition.

>
>> +	memcpy(rss_key, adapter->rss_key, sizeof(adapter->rss_key));
>> +
>> +	return 0;
>> +}
>> +
>>   static int ixgbe_rcv_msg_from_vf(struct ixgbe_adapter *adapter, u32 vf)
>>   {
>>   	u32 mbx_size = IXGBE_VFMAILBOX_SIZE;
>> @@ -1055,6 +1077,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;
>>

Re: [PATCH net-next v9 5/7] ixgbe: Add GET_RSS_KEY command to VF-PF channel commands set

[Vlad Zolotarov]

On 03/30/15 01:04, Alexander Duyck wrote:
> On 03/29/2015 09:11 AM, Vlad Zolotarov wrote:
>> For 82599 and x540 VFs and PF share the same RSS Key. Therefore we will return
>> the same RSS key for all VFs.
>>
>> Support for other devices will be added later.
>>
>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>> ---
>> New in v9:
>>     - Reduce the support to 82599 and x540 devices only.
>>     - Get rid of registers access in GET_VF_RSS_KEY flow:
>>        - Get the RSS HASH Key value from the PF's adapter->rss_key[].
>>
>> New in v5:
>>     - Use a newly added netdev op to allow/prevent the RSS Hash Key querying on a per-VF
>>       basis.
>>
>> 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   |  1 +
>>   drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c | 25 +++++++++++++++++++++++++
>>   2 files changed, 26 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h
>> index 3522f53..b1e4703 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h
>> @@ -100,6 +100,7 @@ enum ixgbe_pfvf_api_rev {
>>   
>>   /* mailbox API, version 1.2 VF requests */
>>   #define IXGBE_VF_GET_RETA	0x0a	/* VF request for 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
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
>> index 8424e7f..4d87458 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
>> @@ -996,6 +996,28 @@ 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;
>> +	u32 *rss_key = &msgbuf[1];
>> +
>> +	/* verify the PF is supporting the correct API */
>> +	if (!adapter->vfinfo[vf].rss_query_enabled ||
>> +	    (adapter->vfinfo[vf].vf_api != ixgbe_mbox_api_12))
>> +		return -EPERM;
>> +
>> +	/* Read the RSS KEY.
>> +	 * We only support 82599 and x540 devices at the moment.
>> +	 */
>> +	if (hw->mac.type >= ixgbe_mac_X550)
>> +		return -1;
>> +
> The return should be not supported, not -1 since that is too ambiguous.

By the way, this (-1) didn't come from nowhere - I looked at other 
functions in this file (e.g. ixgbe_get_vf_queues() and 
ixgbe_negotiate_vf_api()) and they are returning
this "ambiguous" (-1) just fine in the situations where -EOPNOTSUPP and 
-EINVAL (correspondingly) would be much more appropriate.
So, I went and checked who was the guy who added this terrible 
"ambiguity" and, surprise-surprise!!!, it was mister Alexander Duyck! :D 
In both (!!!) cases! Bang-bang!!! :D Of course it was "two years ago" 
Alex but still... ;)

Anyway, since we fight the ambiguity now I think u, guys, would like to 
send a cleanup follow-up patches that would adjust your "ambiguous code" 
with the new "super-clear styling" we are introducing in this patches... ;)

>
>> +	memcpy(rss_key, adapter->rss_key, sizeof(adapter->rss_key));
>> +
>> +	return 0;
>> +}
>> +
>>   static int ixgbe_rcv_msg_from_vf(struct ixgbe_adapter *adapter, u32 vf)
>>   {
>>   	u32 mbx_size = IXGBE_VFMAILBOX_SIZE;
>> @@ -1055,6 +1077,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;
>>

Re: [PATCH net-next v9 6/7] ixgbevf: Add RSS Key query code

[Vlad Zolotarov]

On 03/30/15 01:04, Alexander Duyck wrote:
> On 03/29/2015 09:11 AM, Vlad Zolotarov wrote:
>> 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.
>>
>> This patch adds the support for 82599 and x540 devices only. Support for other
>> devices will be added later.
>>
>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>> ---
>> New in v9:
>>     - Reduce the support to 82599 and x540 devices only.
>>     - Added IXGBEVF_RSS_HASH_KEY_SIZE macro.
>>
>> New in v8:
>>     - Protect a mailbox access.
>>
>> New in v6:
>>     - Return a proper return code when an operation is blocked by PF.
>>
>> 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/ixgbevf.h |  1 +
>>   drivers/net/ethernet/intel/ixgbevf/mbx.h     |  1 +
>>   drivers/net/ethernet/intel/ixgbevf/vf.c      | 66 ++++++++++++++++++++++++++++
>>   drivers/net/ethernet/intel/ixgbevf/vf.h      |  1 +
>>   4 files changed, 69 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h b/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
>> index bc939a1..6771ae3 100644
>> --- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
>> +++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
>> @@ -145,6 +145,7 @@ struct ixgbevf_ring {
>>   #define MAX_RX_QUEUES IXGBE_VF_MAX_RX_QUEUES
>>   #define MAX_TX_QUEUES IXGBE_VF_MAX_TX_QUEUES
>>   #define IXGBEVF_MAX_RSS_QUEUES	2
>> +#define IXGBEVF_RSS_HASH_KEY_SIZE	40
>>   
>>   #define IXGBEVF_DEFAULT_TXD	1024
>>   #define IXGBEVF_DEFAULT_RXD	512
>> diff --git a/drivers/net/ethernet/intel/ixgbevf/mbx.h b/drivers/net/ethernet/intel/ixgbevf/mbx.h
>> index 66e138b..82f44e0 100644
>> --- a/drivers/net/ethernet/intel/ixgbevf/mbx.h
>> +++ b/drivers/net/ethernet/intel/ixgbevf/mbx.h
>> @@ -110,6 +110,7 @@ enum ixgbe_pfvf_api_rev {
>>   
>>   /* mailbox API, version 1.2 VF requests */
>>   #define IXGBE_VF_GET_RETA	0x0a	/* VF request for 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
>> diff --git a/drivers/net/ethernet/intel/ixgbevf/vf.c b/drivers/net/ethernet/intel/ixgbevf/vf.c
>> index 2676c0b..ec68145 100644
>> --- a/drivers/net/ethernet/intel/ixgbevf/vf.c
>> +++ b/drivers/net/ethernet/intel/ixgbevf/vf.c
>> @@ -301,6 +301,72 @@ static inline int ixgbevf_get_reta_locked(struct ixgbe_hw *hw, u32 *msgbuf,
>>   	return 0;
>>   }
>>   
>> +static inline int ixgbevf_get_rss_key_locked(struct ixgbe_hw *hw, u8 *rss_key)
>> +{
>> +	int err;
>> +	u32 msgbuf[IXGBE_VFMAILBOX_SIZE];
>> +
>> +	/* We currently support the RSS Random Key retrieval for 82599 and x540
>> +	 * devices only.
>> +	 *
>> +	 * Thus return an error if API doesn't support RSS Random Key retrieval
>> +	 * or if the operation is not supported for this device type.
>> +	 */
>> +	if (hw->api_version != ixgbe_mbox_api_12 ||
>> +	    hw->mac.type >= ixgbe_mac_X550_vf)
>> +		return -EPERM;
>> +
> The return type here should be not supported, or IXGBE_NOT_IMPLEMENTED,
> not no permissions.

I think a standard EOPNOTSUPP would be a much better choice.
Why do u have this ixgbe-specific return values in addition to standard 
POSIX ones anyway?

>
>> +	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 the operation has been refused by a PF return -EPERM */
>> +	if (msgbuf[0] == (IXGBE_VF_GET_RETA | IXGBE_VT_MSGTYPE_NACK))
>> +		return -EPERM;
>> +
>> +	/* 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, IXGBEVF_RSS_HASH_KEY_SIZE);
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * 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.
>> + * Ensures the atomicy of a mailbox access using the adapter.mbx_lock spinlock.
>> + *
>> + * Returns: 0 on success.
>> + *          if API doesn't support this operation - (-EPERM).
>> + */
>> +int ixgbevf_get_rss_key(struct ixgbevf_adapter *a, u8 *rss_key)
>> +{
>> +	int rc;
>> +
>> +	spin_lock_bh(&a->mbx_lock);
>> +	rc = ixgbevf_get_rss_key_locked(&a->hw, rss_key);
>> +	spin_unlock_bh(&a->mbx_lock);
>> +
>> +	return rc;
>> +}
>> +
> Since there is currently no cases where you would be getting the rss_key
> without the RETA you might just want to combine this function with
> ixgbevf_get_reta so you only take the lock once and process both
> messages in one pass instead of having to bounce the spinlock.
>
>>   /**
>>    * ixgbevf_get_reta - get the RSS redirection table (RETA) contents.
>>    * @adapter: pointer to the port handle
>> diff --git a/drivers/net/ethernet/intel/ixgbevf/vf.h b/drivers/net/ethernet/intel/ixgbevf/vf.h
>> index 7944962..a054716 100644
>> --- a/drivers/net/ethernet/intel/ixgbevf/vf.h
>> +++ b/drivers/net/ethernet/intel/ixgbevf/vf.h
>> @@ -212,4 +212,5 @@ 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 ixgbevf_adapter *adapter, u32 *reta);
>> +int ixgbevf_get_rss_key(struct ixgbevf_adapter *adapter, u8 *rss_key);
>>   #endif /* __IXGBE_VF_H__ */
>>

Re: [PATCH net-next v9 6/7] ixgbevf: Add RSS Key query code

[Vlad Zolotarov]

On 03/30/15 01:04, Alexander Duyck wrote:
> On 03/29/2015 09:11 AM, Vlad Zolotarov wrote:
>> 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.
>>
>> This patch adds the support for 82599 and x540 devices only. Support for other
>> devices will be added later.
>>
>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>> ---
>> New in v9:
>>     - Reduce the support to 82599 and x540 devices only.
>>     - Added IXGBEVF_RSS_HASH_KEY_SIZE macro.
>>
>> New in v8:
>>     - Protect a mailbox access.
>>
>> New in v6:
>>     - Return a proper return code when an operation is blocked by PF.
>>
>> 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/ixgbevf.h |  1 +
>>   drivers/net/ethernet/intel/ixgbevf/mbx.h     |  1 +
>>   drivers/net/ethernet/intel/ixgbevf/vf.c      | 66 ++++++++++++++++++++++++++++
>>   drivers/net/ethernet/intel/ixgbevf/vf.h      |  1 +
>>   4 files changed, 69 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h b/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
>> index bc939a1..6771ae3 100644
>> --- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
>> +++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
>> @@ -145,6 +145,7 @@ struct ixgbevf_ring {
>>   #define MAX_RX_QUEUES IXGBE_VF_MAX_RX_QUEUES
>>   #define MAX_TX_QUEUES IXGBE_VF_MAX_TX_QUEUES
>>   #define IXGBEVF_MAX_RSS_QUEUES	2
>> +#define IXGBEVF_RSS_HASH_KEY_SIZE	40
>>   
>>   #define IXGBEVF_DEFAULT_TXD	1024
>>   #define IXGBEVF_DEFAULT_RXD	512
>> diff --git a/drivers/net/ethernet/intel/ixgbevf/mbx.h b/drivers/net/ethernet/intel/ixgbevf/mbx.h
>> index 66e138b..82f44e0 100644
>> --- a/drivers/net/ethernet/intel/ixgbevf/mbx.h
>> +++ b/drivers/net/ethernet/intel/ixgbevf/mbx.h
>> @@ -110,6 +110,7 @@ enum ixgbe_pfvf_api_rev {
>>   
>>   /* mailbox API, version 1.2 VF requests */
>>   #define IXGBE_VF_GET_RETA	0x0a	/* VF request for 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
>> diff --git a/drivers/net/ethernet/intel/ixgbevf/vf.c b/drivers/net/ethernet/intel/ixgbevf/vf.c
>> index 2676c0b..ec68145 100644
>> --- a/drivers/net/ethernet/intel/ixgbevf/vf.c
>> +++ b/drivers/net/ethernet/intel/ixgbevf/vf.c
>> @@ -301,6 +301,72 @@ static inline int ixgbevf_get_reta_locked(struct ixgbe_hw *hw, u32 *msgbuf,
>>   	return 0;
>>   }
>>   
>> +static inline int ixgbevf_get_rss_key_locked(struct ixgbe_hw *hw, u8 *rss_key)
>> +{
>> +	int err;
>> +	u32 msgbuf[IXGBE_VFMAILBOX_SIZE];
>> +
>> +	/* We currently support the RSS Random Key retrieval for 82599 and x540
>> +	 * devices only.
>> +	 *
>> +	 * Thus return an error if API doesn't support RSS Random Key retrieval
>> +	 * or if the operation is not supported for this device type.
>> +	 */
>> +	if (hw->api_version != ixgbe_mbox_api_12 ||
>> +	    hw->mac.type >= ixgbe_mac_X550_vf)
>> +		return -EPERM;
>> +
> The return type here should be not supported, or IXGBE_NOT_IMPLEMENTED,
> not no permissions.
>
>> +	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 the operation has been refused by a PF return -EPERM */
>> +	if (msgbuf[0] == (IXGBE_VF_GET_RETA | IXGBE_VT_MSGTYPE_NACK))
>> +		return -EPERM;
>> +
>> +	/* 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, IXGBEVF_RSS_HASH_KEY_SIZE);
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * 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.
>> + * Ensures the atomicy of a mailbox access using the adapter.mbx_lock spinlock.
>> + *
>> + * Returns: 0 on success.
>> + *          if API doesn't support this operation - (-EPERM).
>> + */
>> +int ixgbevf_get_rss_key(struct ixgbevf_adapter *a, u8 *rss_key)
>> +{
>> +	int rc;
>> +
>> +	spin_lock_bh(&a->mbx_lock);
>> +	rc = ixgbevf_get_rss_key_locked(&a->hw, rss_key);
>> +	spin_unlock_bh(&a->mbx_lock);
>> +
>> +	return rc;
>> +}
>> +
> Since there is currently no cases where you would be getting the rss_key
> without the RETA you might just want to combine this function with
> ixgbevf_get_reta so you only take the lock once and process both
> messages in one pass instead of having to bounce the spinlock.

I'd rather not do this.
Let's start from the beginning: the locks here should be removed and
added at the caller level to match what u wrote about patch04.
Then about the uniting the two functions mentioned above - there isn't
any added value of doing this. Taking a lock only once may be done
without uniting (see PATCH07 in v10 series).
On the other hand, this uniting is going to make the code awkward and
unclear ("why are these together anyway?").
So, I'd rather keep these functions as they are apart from fixing the locking issue. I'll essentially export the _locked functions.

thanks,
vlad


>
>>   /**
>>    * ixgbevf_get_reta - get the RSS redirection table (RETA) contents.
>>    * @adapter: pointer to the port handle
>> diff --git a/drivers/net/ethernet/intel/ixgbevf/vf.h b/drivers/net/ethernet/intel/ixgbevf/vf.h
>> index 7944962..a054716 100644
>> --- a/drivers/net/ethernet/intel/ixgbevf/vf.h
>> +++ b/drivers/net/ethernet/intel/ixgbevf/vf.h
>> @@ -212,4 +212,5 @@ 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 ixgbevf_adapter *adapter, u32 *reta);
>> +int ixgbevf_get_rss_key(struct ixgbevf_adapter *adapter, u8 *rss_key);
>>   #endif /* __IXGBE_VF_H__ */
>>

Re: [PATCH net-next v9 7/7] ixgbevf: Add the appropriate ethtool ops to query RSS indirection table and key

[Vlad Zolotarov]

On 03/30/15 01:19, Alexander Duyck wrote:
> On 03/29/2015 09:11 AM, Vlad Zolotarov wrote:
>> 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 for VF devices.
>>
>> This patch adds the support for 82599 and x540 devices only. Support for other
>> devices will be added later.
>>
>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>> ---
>> New in v9:
>>     - Use IXGBEVF_RSS_HASH_KEY_SIZE macro.
>>
>> New in v6:
>>     - Added a required get_rxnfc callback to ixgbevf_ethtool_ops.
>>
>> New in v4:
>>     - Removed not needed braces in if-statement in ixgbevf_get_rxfh_indir_size().
>>
>> 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 | 58 ++++++++++++++++++++++++++++
>>   1 file changed, 58 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbevf/ethtool.c b/drivers/net/ethernet/intel/ixgbevf/ethtool.c
>> index e83c85b..4d2f59f 100644
>> --- a/drivers/net/ethernet/intel/ixgbevf/ethtool.c
>> +++ b/drivers/net/ethernet/intel/ixgbevf/ethtool.c
>> @@ -794,6 +794,60 @@ static int ixgbevf_set_coalesce(struct net_device *netdev,
>>   	return 0;
>>   }
>>   
>> +static int ixgbevf_get_rxnfc(struct net_device *dev, struct ethtool_rxnfc *info,
>> +			     u32 *rules __always_unused)
>> +{
>> +	struct ixgbevf_adapter *adapter = netdev_priv(dev);
>> +
>> +	switch (info->cmd) {
>> +	case ETHTOOL_GRXRINGS:
>> +		info->data = adapter->num_rx_queues;
>> +		return 0;
>> +	default:
>> +		hw_dbg(&adapter->hw, "Command parameters not supported\n");
>> +		return -EOPNOTSUPP;
>> +	}
>> +}
>> +
>> +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;
>> +}
>> +
> You should return 0 for x550, not 64 since it isn't supported.
>
> My suggestion would be to alter the code so that you return 128 for
> everything <= x540_vf, and place a return 0 as the default return
> without a need for the if statement.
ok
>
>> +static u32 ixgbevf_get_rxfh_key_size(struct net_device *netdev)
>> +{
>> +	return IXGBEVF_RSS_HASH_KEY_SIZE;
>> +}
>> +
> Same thing here.  If you don't support this on x550 you should probably
> return 0 instead of returning a value.
ok
>
>> +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, indir);
>> +		if (err)
>> +			return err;
>> +	}
>> +
>> +	if (key) {
>> +		err = ixgbevf_get_rss_key(adapter, key);
>> +		if (err)
>> +			return err;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
> My advice here would be to just wrap these two functions in one mailbox
> lock and update the second check so that it is if (!err && key) so that
> you can simplify the path and just return err at the end.

Yeah, something like that. U see - no need to unite them... ;)

>
>>   static const struct ethtool_ops ixgbevf_ethtool_ops = {
>>   	.get_settings		= ixgbevf_get_settings,
>>   	.get_drvinfo		= ixgbevf_get_drvinfo,
>> @@ -811,6 +865,10 @@ 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_rxnfc		= ixgbevf_get_rxnfc,
>> +	.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)
>>

Re: [PATCH net-next v9 4/7] ixgbevf: Add a RETA query code

[Alexander Duyck]

On 03/30/2015 02:12 AM, Vlad Zolotarov wrote:
>
>
> On 03/30/15 01:51, Alexander Duyck wrote:
>> On 03/29/2015 09:11 AM, Vlad Zolotarov wrote:
>>> We will currently support only 82599 and x540 deviced. Support for 
>>> other devices
>>> will be added later.
>>>
>>>     - Added a new API version support.
>>>     - Added the query implementation in the ixgbevf.
>>>
>>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>>> ---
>>> New in v9:
>>>     - Reduce the support to 82599 and x540 devices only.
>>>     - Improvements in RETA query code:
>>>        - Implement a "compression" of VF's RETA contents: pass only 
>>> 2 bits
>>>          per-entry.
>>>        - RETA querying is done in a single mailbox operation thanks 
>>> to compression.
>>>
>>> New in v8:
>>>     - Protect mailbox with a spinlock.
>>>
>>> New in v7:
>>>     - Add ixgbe_mbox_api_12 case in ixgbevf_set_num_queues().
>>>     - Properly expand HW RETA into the ethtool buffer.
>>>
>>> New in v6:
>>>     - Add a proper return code when an operation is blocked by PF.
>>>
>>> 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 |  5 +-
>>>   drivers/net/ethernet/intel/ixgbevf/mbx.h          |  4 ++
>>>   drivers/net/ethernet/intel/ixgbevf/vf.c           | 76 
>>> +++++++++++++++++++++++
>>>   drivers/net/ethernet/intel/ixgbevf/vf.h           |  2 +
>>>   4 files changed, 86 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c 
>>> b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
>>> index 4ee15ad..a16d267 100644
>>> --- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
>>> +++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
>>> @@ -2030,7 +2030,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;
>>> @@ -2332,6 +2333,7 @@ static void ixgbevf_set_num_queues(struct 
>>> ixgbevf_adapter *adapter)
>>>             switch (hw->api_version) {
>>>           case ixgbe_mbox_api_11:
>>> +        case ixgbe_mbox_api_12:
>>>               adapter->num_rx_queues = rss;
>>>               adapter->num_tx_queues = rss;
>>>           default:
>>> @@ -3712,6 +3714,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 6253e93..66e138b 100644
>>> --- a/drivers/net/ethernet/intel/ixgbevf/mbx.h
>>> +++ b/drivers/net/ethernet/intel/ixgbevf/mbx.h
>>> @@ -83,6 +83,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 */
>>>   };
>>> @@ -107,6 +108,9 @@ 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 */
>>> +
>>>   /* 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 2614fd3..2676c0b 100644
>>> --- a/drivers/net/ethernet/intel/ixgbevf/vf.c
>>> +++ b/drivers/net/ethernet/intel/ixgbevf/vf.c
>>> @@ -256,6 +256,81 @@ static s32 ixgbevf_set_uc_addr_vf(struct 
>>> ixgbe_hw *hw, u32 index, u8 *addr)
>>>       return ret_val;
>>>   }
>>>   +static inline int ixgbevf_get_reta_locked(struct ixgbe_hw *hw, 
>>> u32 *msgbuf,
>>> +                      u32 *reta)
>>> +{
>>> +    int err, i, j;
>>> +    u32 *hw_reta = &msgbuf[1];
>>> +
>>> +    /* We have to use a mailbox for 82599 and x540 devices only.
>>> +     * For these devices RETA has 128 entries.
>>> +     * Also these VFs support up to 4 RSS queues. Therefore PF will 
>>> compress
>>> +     * 16 RETA entries in each DWORD giving 2 bits to each entry.
>>> +     */
>>> +    int dwords = 128 / 16;
>>> +
>>> +    msgbuf[0] = IXGBE_VF_GET_RETA;
>>> +
>>> +    err = hw->mbx.ops.write_posted(hw, msgbuf, 1);
>>> +
>>> +    if (err)
>>> +        return err;
>>> +
>>> +    err = hw->mbx.ops.read_posted(hw, msgbuf, dwords + 1);
>>> +
>>> +    if (err)
>>> +        return err;
>>> +
>>> +    msgbuf[0] &= ~IXGBE_VT_MSGTYPE_CTS;
>>> +
>>> +    /* If the operation has been refused by a PF return -EPERM */
>>> +    if (msgbuf[0] == (IXGBE_VF_GET_RETA | IXGBE_VT_MSGTYPE_NACK))
>>> +        return -EPERM;
>>> +
>>> +    /* 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;
>>> +
>>> +    for (i = 0; i < dwords; i++)
>>> +        for (j = 0; j < 16; j++)
>>> +            reta[i * 16 + j] = (hw_reta[i] >> (2 * j)) & 0x3;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +/**
>>> + * ixgbevf_get_reta - get the RSS redirection table (RETA) contents.
>>> + * @adapter: pointer to the port handle
>>> + * @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 ixgbevf_adapter *adapter, u32 *reta)
>>> +{
>>> +    int err;
>>> +    u32 msgbuf[IXGBE_VFMAILBOX_SIZE];
>>> +
>>> +    /* We support the RSS querying for 82599 and x540 devices only.
>>> +     * Thus return an error if API doesn't support RETA querying or 
>>> querying
>>> +     * is not supported for this device type.
>>> +     */
>>> +    if (adapter->hw.api_version != ixgbe_mbox_api_12 ||
>>> +        adapter->hw.mac.type >= ixgbe_mac_X550_vf)
>>> +        return -EPERM;
>
> By the way, since u've commented on the return codes in PATCH02 don't 
> u think that we should return -EOPNOTSUPP here too?

Yes.  I just missed that in the review.  Though I don't know if 
EOPNOTSUPP is the correct response here since this is really meant to be 
an OS agnostic section.  You might just return IXGBE_ERR_MBX since there 
isn't a mailbox messages supported for x550_vf or API before 12.

- Alex

Re: [PATCH net-next v9 5/7] ixgbe: Add GET_RSS_KEY command to VF-PF channel commands set

[Alexander Duyck]

On 03/30/2015 03:41 AM, Vlad Zolotarov wrote:
>
>
> On 03/30/15 01:04, Alexander Duyck wrote:
>> On 03/29/2015 09:11 AM, Vlad Zolotarov wrote:
>>> For 82599 and x540 VFs and PF share the same RSS Key. Therefore we 
>>> will return
>>> the same RSS key for all VFs.
>>>
>>> Support for other devices will be added later.
>>>
>>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>>> ---
>>> New in v9:
>>>     - Reduce the support to 82599 and x540 devices only.
>>>     - Get rid of registers access in GET_VF_RSS_KEY flow:
>>>        - Get the RSS HASH Key value from the PF's adapter->rss_key[].
>>>
>>> New in v5:
>>>     - Use a newly added netdev op to allow/prevent the RSS Hash Key 
>>> querying on a per-VF
>>>       basis.
>>>
>>> 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   |  1 +
>>>   drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c | 25 
>>> +++++++++++++++++++++++++
>>>   2 files changed, 26 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h 
>>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h
>>> index 3522f53..b1e4703 100644
>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h
>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h
>>> @@ -100,6 +100,7 @@ enum ixgbe_pfvf_api_rev {
>>>     /* mailbox API, version 1.2 VF requests */
>>>   #define IXGBE_VF_GET_RETA    0x0a    /* VF request for 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
>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c 
>>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
>>> index 8424e7f..4d87458 100644
>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
>>> @@ -996,6 +996,28 @@ 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;
>>> +    u32 *rss_key = &msgbuf[1];
>>> +
>>> +    /* verify the PF is supporting the correct API */
>>> +    if (!adapter->vfinfo[vf].rss_query_enabled ||
>>> +        (adapter->vfinfo[vf].vf_api != ixgbe_mbox_api_12))
>>> +        return -EPERM;
>>> +
>>> +    /* Read the RSS KEY.
>>> +     * We only support 82599 and x540 devices at the moment.
>>> +     */
>>> +    if (hw->mac.type >= ixgbe_mac_X550)
>>> +        return -1;
>>> +
>> The return should be not supported, not -1 since that is too ambiguous.
>
> By the way, this (-1) didn't come from nowhere - I looked at other 
> functions in this file (e.g. ixgbe_get_vf_queues() and 
> ixgbe_negotiate_vf_api()) and they are returning
> this "ambiguous" (-1) just fine in the situations where -EOPNOTSUPP 
> and -EINVAL (correspondingly) would be much more appropriate.
> So, I went and checked who was the guy who added this terrible 
> "ambiguity" and, surprise-surprise!!!, it was mister Alexander Duyck! 
> :D In both (!!!) cases! Bang-bang!!! :D Of course it was "two years 
> ago" Alex but still... ;)

Yeah, it was a bad habit I used to have.  That is why I am able to spot 
this kind of thing so easily now.

> Anyway, since we fight the ambiguity now I think u, guys, would like 
> to send a cleanup follow-up patches that would adjust your "ambiguous 
> code" with the new "super-clear styling" we are introducing in this 
> patches... ;)

Well, I'm not part of Intel anymore and have more then enough on my 
plate at the moment.  I'm sure when someone over there gets a few spare 
cycles, or can get some kernel janitors on it they will clean up the 
error return values.

If nothing else it is useful to have the error returns make sense as 
more often then not they make their way all the way back to the use in 
the form of the error message so it is best to return something like a 
not supported, than not permitted.

- Alex

Re: [PATCH net-next v9 6/7] ixgbevf: Add RSS Key query code

[Alexander Duyck]

On 03/30/2015 05:38 AM, Vlad Zolotarov wrote:
>
>
> On 03/30/15 01:04, Alexander Duyck wrote:
>> On 03/29/2015 09:11 AM, Vlad Zolotarov wrote:
>>> 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.
>>>
>>> This patch adds the support for 82599 and x540 devices only. Support 
>>> for other
>>> devices will be added later.
>>>
>>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>>> ---
>>> New in v9:
>>>     - Reduce the support to 82599 and x540 devices only.
>>>     - Added IXGBEVF_RSS_HASH_KEY_SIZE macro.
>>>
>>> New in v8:
>>>     - Protect a mailbox access.
>>>
>>> New in v6:
>>>     - Return a proper return code when an operation is blocked by PF.
>>>
>>> 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/ixgbevf.h |  1 +
>>>   drivers/net/ethernet/intel/ixgbevf/mbx.h     |  1 +
>>>   drivers/net/ethernet/intel/ixgbevf/vf.c      | 66 
>>> ++++++++++++++++++++++++++++
>>>   drivers/net/ethernet/intel/ixgbevf/vf.h      |  1 +
>>>   4 files changed, 69 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h 
>>> b/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
>>> index bc939a1..6771ae3 100644
>>> --- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
>>> +++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
>>> @@ -145,6 +145,7 @@ struct ixgbevf_ring {
>>>   #define MAX_RX_QUEUES IXGBE_VF_MAX_RX_QUEUES
>>>   #define MAX_TX_QUEUES IXGBE_VF_MAX_TX_QUEUES
>>>   #define IXGBEVF_MAX_RSS_QUEUES    2
>>> +#define IXGBEVF_RSS_HASH_KEY_SIZE    40
>>>     #define IXGBEVF_DEFAULT_TXD    1024
>>>   #define IXGBEVF_DEFAULT_RXD    512
>>> diff --git a/drivers/net/ethernet/intel/ixgbevf/mbx.h 
>>> b/drivers/net/ethernet/intel/ixgbevf/mbx.h
>>> index 66e138b..82f44e0 100644
>>> --- a/drivers/net/ethernet/intel/ixgbevf/mbx.h
>>> +++ b/drivers/net/ethernet/intel/ixgbevf/mbx.h
>>> @@ -110,6 +110,7 @@ enum ixgbe_pfvf_api_rev {
>>>     /* mailbox API, version 1.2 VF requests */
>>>   #define IXGBE_VF_GET_RETA    0x0a    /* VF request for 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
>>> diff --git a/drivers/net/ethernet/intel/ixgbevf/vf.c 
>>> b/drivers/net/ethernet/intel/ixgbevf/vf.c
>>> index 2676c0b..ec68145 100644
>>> --- a/drivers/net/ethernet/intel/ixgbevf/vf.c
>>> +++ b/drivers/net/ethernet/intel/ixgbevf/vf.c
>>> @@ -301,6 +301,72 @@ static inline int 
>>> ixgbevf_get_reta_locked(struct ixgbe_hw *hw, u32 *msgbuf,
>>>       return 0;
>>>   }
>>>   +static inline int ixgbevf_get_rss_key_locked(struct ixgbe_hw *hw, 
>>> u8 *rss_key)
>>> +{
>>> +    int err;
>>> +    u32 msgbuf[IXGBE_VFMAILBOX_SIZE];
>>> +
>>> +    /* We currently support the RSS Random Key retrieval for 82599 
>>> and x540
>>> +     * devices only.
>>> +     *
>>> +     * Thus return an error if API doesn't support RSS Random Key 
>>> retrieval
>>> +     * or if the operation is not supported for this device type.
>>> +     */
>>> +    if (hw->api_version != ixgbe_mbox_api_12 ||
>>> +        hw->mac.type >= ixgbe_mac_X550_vf)
>>> +        return -EPERM;
>>> +
>> The return type here should be not supported, or IXGBE_NOT_IMPLEMENTED,
>> not no permissions.
>
> I think a standard EOPNOTSUPP would be a much better choice.
> Why do u have this ixgbe-specific return values in addition to 
> standard POSIX ones anyway?

It has to do with being OS agnostic.  If you want to port this to BSD, 
Linux, DPDK, etc, then you can't always rely on all the error return 
values being there so there is a tendency to implement your own.  As 
with the other function you might be best off just returning 
IXGBE_ERR_MBX since the x550 and API_12 don't support the RSS messages.

- Alex

Re: [PATCH net-next v9 6/7] ixgbevf: Add RSS Key query code

[Alexander Duyck]

On 03/30/2015 06:53 AM, Vlad Zolotarov wrote:
>
>
> On 03/30/15 01:04, Alexander Duyck wrote:
>> On 03/29/2015 09:11 AM, Vlad Zolotarov wrote:
>>> 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.
>>>
>>> This patch adds the support for 82599 and x540 devices only. Support 
>>> for other
>>> devices will be added later.
>>>
>>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>>> ---
>>> New in v9:
>>>     - Reduce the support to 82599 and x540 devices only.
>>>     - Added IXGBEVF_RSS_HASH_KEY_SIZE macro.
>>>
>>> New in v8:
>>>     - Protect a mailbox access.
>>>
>>> New in v6:
>>>     - Return a proper return code when an operation is blocked by PF.
>>>
>>> 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/ixgbevf.h |  1 +
>>>   drivers/net/ethernet/intel/ixgbevf/mbx.h     |  1 +
>>>   drivers/net/ethernet/intel/ixgbevf/vf.c      | 66 
>>> ++++++++++++++++++++++++++++
>>>   drivers/net/ethernet/intel/ixgbevf/vf.h      |  1 +
>>>   4 files changed, 69 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h 
>>> b/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
>>> index bc939a1..6771ae3 100644
>>> --- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
>>> +++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
>>> @@ -145,6 +145,7 @@ struct ixgbevf_ring {
>>>   #define MAX_RX_QUEUES IXGBE_VF_MAX_RX_QUEUES
>>>   #define MAX_TX_QUEUES IXGBE_VF_MAX_TX_QUEUES
>>>   #define IXGBEVF_MAX_RSS_QUEUES    2
>>> +#define IXGBEVF_RSS_HASH_KEY_SIZE    40
>>>     #define IXGBEVF_DEFAULT_TXD    1024
>>>   #define IXGBEVF_DEFAULT_RXD    512
>>> diff --git a/drivers/net/ethernet/intel/ixgbevf/mbx.h 
>>> b/drivers/net/ethernet/intel/ixgbevf/mbx.h
>>> index 66e138b..82f44e0 100644
>>> --- a/drivers/net/ethernet/intel/ixgbevf/mbx.h
>>> +++ b/drivers/net/ethernet/intel/ixgbevf/mbx.h
>>> @@ -110,6 +110,7 @@ enum ixgbe_pfvf_api_rev {
>>>     /* mailbox API, version 1.2 VF requests */
>>>   #define IXGBE_VF_GET_RETA    0x0a    /* VF request for 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
>>> diff --git a/drivers/net/ethernet/intel/ixgbevf/vf.c 
>>> b/drivers/net/ethernet/intel/ixgbevf/vf.c
>>> index 2676c0b..ec68145 100644
>>> --- a/drivers/net/ethernet/intel/ixgbevf/vf.c
>>> +++ b/drivers/net/ethernet/intel/ixgbevf/vf.c
>>> @@ -301,6 +301,72 @@ static inline int 
>>> ixgbevf_get_reta_locked(struct ixgbe_hw *hw, u32 *msgbuf,
>>>       return 0;
>>>   }
>>>   +static inline int ixgbevf_get_rss_key_locked(struct ixgbe_hw *hw, 
>>> u8 *rss_key)
>>> +{
>>> +    int err;
>>> +    u32 msgbuf[IXGBE_VFMAILBOX_SIZE];
>>> +
>>> +    /* We currently support the RSS Random Key retrieval for 82599 
>>> and x540
>>> +     * devices only.
>>> +     *
>>> +     * Thus return an error if API doesn't support RSS Random Key 
>>> retrieval
>>> +     * or if the operation is not supported for this device type.
>>> +     */
>>> +    if (hw->api_version != ixgbe_mbox_api_12 ||
>>> +        hw->mac.type >= ixgbe_mac_X550_vf)
>>> +        return -EPERM;
>>> +
>> The return type here should be not supported, or IXGBE_NOT_IMPLEMENTED,
>> not no permissions.
>>
>>> +    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 the operation has been refused by a PF return -EPERM */
>>> +    if (msgbuf[0] == (IXGBE_VF_GET_RETA | IXGBE_VT_MSGTYPE_NACK))
>>> +        return -EPERM;
>>> +
>>> +    /* 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, IXGBEVF_RSS_HASH_KEY_SIZE);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +/**
>>> + * 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.
>>> + * Ensures the atomicy of a mailbox access using the 
>>> adapter.mbx_lock spinlock.
>>> + *
>>> + * Returns: 0 on success.
>>> + *          if API doesn't support this operation - (-EPERM).
>>> + */
>>> +int ixgbevf_get_rss_key(struct ixgbevf_adapter *a, u8 *rss_key)
>>> +{
>>> +    int rc;
>>> +
>>> +    spin_lock_bh(&a->mbx_lock);
>>> +    rc = ixgbevf_get_rss_key_locked(&a->hw, rss_key);
>>> +    spin_unlock_bh(&a->mbx_lock);
>>> +
>>> +    return rc;
>>> +}
>>> +
>> Since there is currently no cases where you would be getting the rss_key
>> without the RETA you might just want to combine this function with
>> ixgbevf_get_reta so you only take the lock once and process both
>> messages in one pass instead of having to bounce the spinlock.
>
> I'd rather not do this.
> Let's start from the beginning: the locks here should be removed and
> added at the caller level to match what u wrote about patch04.
> Then about the uniting the two functions mentioned above - there isn't
> any added value of doing this. Taking a lock only once may be done
> without uniting (see PATCH07 in v10 series).
> On the other hand, this uniting is going to make the code awkward and
> unclear ("why are these together anyway?").
> So, I'd rather keep these functions as they are apart from fixing the 
> locking issue. I'll essentially export the _locked functions.
>
> thanks,
> vlad

Agreed, just export the _locked functions and drop the _locked extension.

- Alex

Re: [PATCH net-next v9 4/7] ixgbevf: Add a RETA query code

[Vlad Zolotarov]

On 03/30/15 17:58, Alexander Duyck wrote:
>
> On 03/30/2015 02:12 AM, Vlad Zolotarov wrote:
>>
>>
>> On 03/30/15 01:51, Alexander Duyck wrote:
>>> On 03/29/2015 09:11 AM, Vlad Zolotarov wrote:
>>>> We will currently support only 82599 and x540 deviced. Support for 
>>>> other devices
>>>> will be added later.
>>>>
>>>>     - Added a new API version support.
>>>>     - Added the query implementation in the ixgbevf.
>>>>
>>>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>>>> ---
>>>> New in v9:
>>>>     - Reduce the support to 82599 and x540 devices only.
>>>>     - Improvements in RETA query code:
>>>>        - Implement a "compression" of VF's RETA contents: pass only 
>>>> 2 bits
>>>>          per-entry.
>>>>        - RETA querying is done in a single mailbox operation thanks 
>>>> to compression.
>>>>
>>>> New in v8:
>>>>     - Protect mailbox with a spinlock.
>>>>
>>>> New in v7:
>>>>     - Add ixgbe_mbox_api_12 case in ixgbevf_set_num_queues().
>>>>     - Properly expand HW RETA into the ethtool buffer.
>>>>
>>>> New in v6:
>>>>     - Add a proper return code when an operation is blocked by PF.
>>>>
>>>> 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 |  5 +-
>>>>   drivers/net/ethernet/intel/ixgbevf/mbx.h          |  4 ++
>>>>   drivers/net/ethernet/intel/ixgbevf/vf.c           | 76 
>>>> +++++++++++++++++++++++
>>>>   drivers/net/ethernet/intel/ixgbevf/vf.h           |  2 +
>>>>   4 files changed, 86 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c 
>>>> b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
>>>> index 4ee15ad..a16d267 100644
>>>> --- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
>>>> +++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
>>>> @@ -2030,7 +2030,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;
>>>> @@ -2332,6 +2333,7 @@ static void ixgbevf_set_num_queues(struct 
>>>> ixgbevf_adapter *adapter)
>>>>             switch (hw->api_version) {
>>>>           case ixgbe_mbox_api_11:
>>>> +        case ixgbe_mbox_api_12:
>>>>               adapter->num_rx_queues = rss;
>>>>               adapter->num_tx_queues = rss;
>>>>           default:
>>>> @@ -3712,6 +3714,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 6253e93..66e138b 100644
>>>> --- a/drivers/net/ethernet/intel/ixgbevf/mbx.h
>>>> +++ b/drivers/net/ethernet/intel/ixgbevf/mbx.h
>>>> @@ -83,6 +83,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 */
>>>>   };
>>>> @@ -107,6 +108,9 @@ 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 */
>>>> +
>>>>   /* 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 2614fd3..2676c0b 100644
>>>> --- a/drivers/net/ethernet/intel/ixgbevf/vf.c
>>>> +++ b/drivers/net/ethernet/intel/ixgbevf/vf.c
>>>> @@ -256,6 +256,81 @@ static s32 ixgbevf_set_uc_addr_vf(struct 
>>>> ixgbe_hw *hw, u32 index, u8 *addr)
>>>>       return ret_val;
>>>>   }
>>>>   +static inline int ixgbevf_get_reta_locked(struct ixgbe_hw *hw, 
>>>> u32 *msgbuf,
>>>> +                      u32 *reta)
>>>> +{
>>>> +    int err, i, j;
>>>> +    u32 *hw_reta = &msgbuf[1];
>>>> +
>>>> +    /* We have to use a mailbox for 82599 and x540 devices only.
>>>> +     * For these devices RETA has 128 entries.
>>>> +     * Also these VFs support up to 4 RSS queues. Therefore PF 
>>>> will compress
>>>> +     * 16 RETA entries in each DWORD giving 2 bits to each entry.
>>>> +     */
>>>> +    int dwords = 128 / 16;
>>>> +
>>>> +    msgbuf[0] = IXGBE_VF_GET_RETA;
>>>> +
>>>> +    err = hw->mbx.ops.write_posted(hw, msgbuf, 1);
>>>> +
>>>> +    if (err)
>>>> +        return err;
>>>> +
>>>> +    err = hw->mbx.ops.read_posted(hw, msgbuf, dwords + 1);
>>>> +
>>>> +    if (err)
>>>> +        return err;
>>>> +
>>>> +    msgbuf[0] &= ~IXGBE_VT_MSGTYPE_CTS;
>>>> +
>>>> +    /* If the operation has been refused by a PF return -EPERM */
>>>> +    if (msgbuf[0] == (IXGBE_VF_GET_RETA | IXGBE_VT_MSGTYPE_NACK))
>>>> +        return -EPERM;
>>>> +
>>>> +    /* 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;
>>>> +
>>>> +    for (i = 0; i < dwords; i++)
>>>> +        for (j = 0; j < 16; j++)
>>>> +            reta[i * 16 + j] = (hw_reta[i] >> (2 * j)) & 0x3;
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +/**
>>>> + * ixgbevf_get_reta - get the RSS redirection table (RETA) contents.
>>>> + * @adapter: pointer to the port handle
>>>> + * @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 ixgbevf_adapter *adapter, u32 *reta)
>>>> +{
>>>> +    int err;
>>>> +    u32 msgbuf[IXGBE_VFMAILBOX_SIZE];
>>>> +
>>>> +    /* We support the RSS querying for 82599 and x540 devices only.
>>>> +     * Thus return an error if API doesn't support RETA querying 
>>>> or querying
>>>> +     * is not supported for this device type.
>>>> +     */
>>>> +    if (adapter->hw.api_version != ixgbe_mbox_api_12 ||
>>>> +        adapter->hw.mac.type >= ixgbe_mac_X550_vf)
>>>> +        return -EPERM;
>>
>> By the way, since u've commented on the return codes in PATCH02 don't 
>> u think that we should return -EOPNOTSUPP here too?
>
> Yes.  I just missed that in the review.  Though I don't know if 
> EOPNOTSUPP is the correct response here since this is really meant to 
> be an OS agnostic section. 

Well, it's gona be a super correct response (for any Posix compliant OS 
like Linux and FreeBSD) since (in Linux) this error code is forwarded 
directly to ethtool application and it prints the corresponding error 
message.
Pls., note that there are other functions in vf.c that already return 
other POSIX codes (e.g. -ENOMEM in ixgbevf_set_uc_addr_vf()).


> You might just return IXGBE_ERR_MBX since there isn't a mailbox 
> messages supported for x550_vf or API before 12.

I remember Dave Miller was really angry in the past when he smelled that 
some Linux code is being shared with other not-so-open-source OSes... ;)

>
> - Alex

Re: [PATCH net-next v9 6/7] ixgbevf: Add RSS Key query code

[Vlad Zolotarov]

On 03/30/15 18:10, Alexander Duyck wrote:
>
> On 03/30/2015 06:53 AM, Vlad Zolotarov wrote:
>>
>>
>> On 03/30/15 01:04, Alexander Duyck wrote:
>>> On 03/29/2015 09:11 AM, Vlad Zolotarov wrote:
>>>> 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.
>>>>
>>>> This patch adds the support for 82599 and x540 devices only. 
>>>> Support for other
>>>> devices will be added later.
>>>>
>>>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>>>> ---
>>>> New in v9:
>>>>     - Reduce the support to 82599 and x540 devices only.
>>>>     - Added IXGBEVF_RSS_HASH_KEY_SIZE macro.
>>>>
>>>> New in v8:
>>>>     - Protect a mailbox access.
>>>>
>>>> New in v6:
>>>>     - Return a proper return code when an operation is blocked by PF.
>>>>
>>>> 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/ixgbevf.h |  1 +
>>>>   drivers/net/ethernet/intel/ixgbevf/mbx.h     |  1 +
>>>>   drivers/net/ethernet/intel/ixgbevf/vf.c      | 66 
>>>> ++++++++++++++++++++++++++++
>>>>   drivers/net/ethernet/intel/ixgbevf/vf.h      |  1 +
>>>>   4 files changed, 69 insertions(+)
>>>>
>>>> diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h 
>>>> b/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
>>>> index bc939a1..6771ae3 100644
>>>> --- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
>>>> +++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
>>>> @@ -145,6 +145,7 @@ struct ixgbevf_ring {
>>>>   #define MAX_RX_QUEUES IXGBE_VF_MAX_RX_QUEUES
>>>>   #define MAX_TX_QUEUES IXGBE_VF_MAX_TX_QUEUES
>>>>   #define IXGBEVF_MAX_RSS_QUEUES    2
>>>> +#define IXGBEVF_RSS_HASH_KEY_SIZE    40
>>>>     #define IXGBEVF_DEFAULT_TXD    1024
>>>>   #define IXGBEVF_DEFAULT_RXD    512
>>>> diff --git a/drivers/net/ethernet/intel/ixgbevf/mbx.h 
>>>> b/drivers/net/ethernet/intel/ixgbevf/mbx.h
>>>> index 66e138b..82f44e0 100644
>>>> --- a/drivers/net/ethernet/intel/ixgbevf/mbx.h
>>>> +++ b/drivers/net/ethernet/intel/ixgbevf/mbx.h
>>>> @@ -110,6 +110,7 @@ enum ixgbe_pfvf_api_rev {
>>>>     /* mailbox API, version 1.2 VF requests */
>>>>   #define IXGBE_VF_GET_RETA    0x0a    /* VF request for 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
>>>> diff --git a/drivers/net/ethernet/intel/ixgbevf/vf.c 
>>>> b/drivers/net/ethernet/intel/ixgbevf/vf.c
>>>> index 2676c0b..ec68145 100644
>>>> --- a/drivers/net/ethernet/intel/ixgbevf/vf.c
>>>> +++ b/drivers/net/ethernet/intel/ixgbevf/vf.c
>>>> @@ -301,6 +301,72 @@ static inline int 
>>>> ixgbevf_get_reta_locked(struct ixgbe_hw *hw, u32 *msgbuf,
>>>>       return 0;
>>>>   }
>>>>   +static inline int ixgbevf_get_rss_key_locked(struct ixgbe_hw 
>>>> *hw, u8 *rss_key)
>>>> +{
>>>> +    int err;
>>>> +    u32 msgbuf[IXGBE_VFMAILBOX_SIZE];
>>>> +
>>>> +    /* We currently support the RSS Random Key retrieval for 82599 
>>>> and x540
>>>> +     * devices only.
>>>> +     *
>>>> +     * Thus return an error if API doesn't support RSS Random Key 
>>>> retrieval
>>>> +     * or if the operation is not supported for this device type.
>>>> +     */
>>>> +    if (hw->api_version != ixgbe_mbox_api_12 ||
>>>> +        hw->mac.type >= ixgbe_mac_X550_vf)
>>>> +        return -EPERM;
>>>> +
>>> The return type here should be not supported, or IXGBE_NOT_IMPLEMENTED,
>>> not no permissions.
>>>
>>>> +    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 the operation has been refused by a PF return -EPERM */
>>>> +    if (msgbuf[0] == (IXGBE_VF_GET_RETA | IXGBE_VT_MSGTYPE_NACK))
>>>> +        return -EPERM;
>>>> +
>>>> +    /* 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, IXGBEVF_RSS_HASH_KEY_SIZE);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +/**
>>>> + * 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.
>>>> + * Ensures the atomicy of a mailbox access using the 
>>>> adapter.mbx_lock spinlock.
>>>> + *
>>>> + * Returns: 0 on success.
>>>> + *          if API doesn't support this operation - (-EPERM).
>>>> + */
>>>> +int ixgbevf_get_rss_key(struct ixgbevf_adapter *a, u8 *rss_key)
>>>> +{
>>>> +    int rc;
>>>> +
>>>> +    spin_lock_bh(&a->mbx_lock);
>>>> +    rc = ixgbevf_get_rss_key_locked(&a->hw, rss_key);
>>>> +    spin_unlock_bh(&a->mbx_lock);
>>>> +
>>>> +    return rc;
>>>> +}
>>>> +
>>> Since there is currently no cases where you would be getting the 
>>> rss_key
>>> without the RETA you might just want to combine this function with
>>> ixgbevf_get_reta so you only take the lock once and process both
>>> messages in one pass instead of having to bounce the spinlock.
>>
>> I'd rather not do this.
>> Let's start from the beginning: the locks here should be removed and
>> added at the caller level to match what u wrote about patch04.
>> Then about the uniting the two functions mentioned above - there isn't
>> any added value of doing this. Taking a lock only once may be done
>> without uniting (see PATCH07 in v10 series).
>> On the other hand, this uniting is going to make the code awkward and
>> unclear ("why are these together anyway?").
>> So, I'd rather keep these functions as they are apart from fixing the 
>> locking issue. I'll essentially export the _locked functions.
>>
>> thanks,
>> vlad
>
> Agreed, just export the _locked functions and drop the _locked extension.

Hmmm... I think keeping the _locked extension would make sense since it 
would explicitly hint that these functions have to be called under the 
lock. There are other similar examples in the ixgbe/ixgbevf code already.

>
> - Alex

Re: [PATCH net-next v9 6/7] ixgbevf: Add RSS Key query code

[Vlad Zolotarov]

On 03/30/15 18:07, Alexander Duyck wrote:
>
> On 03/30/2015 05:38 AM, Vlad Zolotarov wrote:
>>
>>
>> On 03/30/15 01:04, Alexander Duyck wrote:
>>> On 03/29/2015 09:11 AM, Vlad Zolotarov wrote:
>>>> 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.
>>>>
>>>> This patch adds the support for 82599 and x540 devices only. 
>>>> Support for other
>>>> devices will be added later.
>>>>
>>>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>>>> ---
>>>> New in v9:
>>>>     - Reduce the support to 82599 and x540 devices only.
>>>>     - Added IXGBEVF_RSS_HASH_KEY_SIZE macro.
>>>>
>>>> New in v8:
>>>>     - Protect a mailbox access.
>>>>
>>>> New in v6:
>>>>     - Return a proper return code when an operation is blocked by PF.
>>>>
>>>> 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/ixgbevf.h |  1 +
>>>>   drivers/net/ethernet/intel/ixgbevf/mbx.h     |  1 +
>>>>   drivers/net/ethernet/intel/ixgbevf/vf.c      | 66 
>>>> ++++++++++++++++++++++++++++
>>>>   drivers/net/ethernet/intel/ixgbevf/vf.h      |  1 +
>>>>   4 files changed, 69 insertions(+)
>>>>
>>>> diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h 
>>>> b/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
>>>> index bc939a1..6771ae3 100644
>>>> --- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
>>>> +++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
>>>> @@ -145,6 +145,7 @@ struct ixgbevf_ring {
>>>>   #define MAX_RX_QUEUES IXGBE_VF_MAX_RX_QUEUES
>>>>   #define MAX_TX_QUEUES IXGBE_VF_MAX_TX_QUEUES
>>>>   #define IXGBEVF_MAX_RSS_QUEUES    2
>>>> +#define IXGBEVF_RSS_HASH_KEY_SIZE    40
>>>>     #define IXGBEVF_DEFAULT_TXD    1024
>>>>   #define IXGBEVF_DEFAULT_RXD    512
>>>> diff --git a/drivers/net/ethernet/intel/ixgbevf/mbx.h 
>>>> b/drivers/net/ethernet/intel/ixgbevf/mbx.h
>>>> index 66e138b..82f44e0 100644
>>>> --- a/drivers/net/ethernet/intel/ixgbevf/mbx.h
>>>> +++ b/drivers/net/ethernet/intel/ixgbevf/mbx.h
>>>> @@ -110,6 +110,7 @@ enum ixgbe_pfvf_api_rev {
>>>>     /* mailbox API, version 1.2 VF requests */
>>>>   #define IXGBE_VF_GET_RETA    0x0a    /* VF request for 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
>>>> diff --git a/drivers/net/ethernet/intel/ixgbevf/vf.c 
>>>> b/drivers/net/ethernet/intel/ixgbevf/vf.c
>>>> index 2676c0b..ec68145 100644
>>>> --- a/drivers/net/ethernet/intel/ixgbevf/vf.c
>>>> +++ b/drivers/net/ethernet/intel/ixgbevf/vf.c
>>>> @@ -301,6 +301,72 @@ static inline int 
>>>> ixgbevf_get_reta_locked(struct ixgbe_hw *hw, u32 *msgbuf,
>>>>       return 0;
>>>>   }
>>>>   +static inline int ixgbevf_get_rss_key_locked(struct ixgbe_hw 
>>>> *hw, u8 *rss_key)
>>>> +{
>>>> +    int err;
>>>> +    u32 msgbuf[IXGBE_VFMAILBOX_SIZE];
>>>> +
>>>> +    /* We currently support the RSS Random Key retrieval for 82599 
>>>> and x540
>>>> +     * devices only.
>>>> +     *
>>>> +     * Thus return an error if API doesn't support RSS Random Key 
>>>> retrieval
>>>> +     * or if the operation is not supported for this device type.
>>>> +     */
>>>> +    if (hw->api_version != ixgbe_mbox_api_12 ||
>>>> +        hw->mac.type >= ixgbe_mac_X550_vf)
>>>> +        return -EPERM;
>>>> +
>>> The return type here should be not supported, or IXGBE_NOT_IMPLEMENTED,
>>> not no permissions.
>>
>> I think a standard EOPNOTSUPP would be a much better choice.
>> Why do u have this ixgbe-specific return values in addition to 
>> standard POSIX ones anyway?
>
> It has to do with being OS agnostic.  If you want to port this to BSD, 
> Linux, DPDK, etc, then you can't always rely on all the error return 
> values being there so there is a tendency to implement your own.  As 
> with the other function you might be best off just returning 
> IXGBE_ERR_MBX since the x550 and API_12 don't support the RSS messages.

EOPNOTSUPP is a standard POSIX.1 error code. It's defined both in Linux 
and in FreeBSD. As far as I know DPDK currently supports either Linux or 
BSD targets. So, for all mentioned above options there should not be any 
problem with this error code at all and in the scope of these OSes/SDKs 
the new lines are absolutely portable.

>
> - Alex

Re: [PATCH net-next v9 6/7] ixgbevf: Add RSS Key query code

[Vlad Zolotarov]

On 03/30/15 18:07, Alexander Duyck wrote:
>
> On 03/30/2015 05:38 AM, Vlad Zolotarov wrote:
>>
>>
>> On 03/30/15 01:04, Alexander Duyck wrote:
>>> On 03/29/2015 09:11 AM, Vlad Zolotarov wrote:
>>>> 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.
>>>>
>>>> This patch adds the support for 82599 and x540 devices only. 
>>>> Support for other
>>>> devices will be added later.
>>>>
>>>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>>>> ---
>>>> New in v9:
>>>>     - Reduce the support to 82599 and x540 devices only.
>>>>     - Added IXGBEVF_RSS_HASH_KEY_SIZE macro.
>>>>
>>>> New in v8:
>>>>     - Protect a mailbox access.
>>>>
>>>> New in v6:
>>>>     - Return a proper return code when an operation is blocked by PF.
>>>>
>>>> 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/ixgbevf.h |  1 +
>>>>   drivers/net/ethernet/intel/ixgbevf/mbx.h     |  1 +
>>>>   drivers/net/ethernet/intel/ixgbevf/vf.c      | 66 
>>>> ++++++++++++++++++++++++++++
>>>>   drivers/net/ethernet/intel/ixgbevf/vf.h      |  1 +
>>>>   4 files changed, 69 insertions(+)
>>>>
>>>> diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h 
>>>> b/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
>>>> index bc939a1..6771ae3 100644
>>>> --- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
>>>> +++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
>>>> @@ -145,6 +145,7 @@ struct ixgbevf_ring {
>>>>   #define MAX_RX_QUEUES IXGBE_VF_MAX_RX_QUEUES
>>>>   #define MAX_TX_QUEUES IXGBE_VF_MAX_TX_QUEUES
>>>>   #define IXGBEVF_MAX_RSS_QUEUES    2
>>>> +#define IXGBEVF_RSS_HASH_KEY_SIZE    40
>>>>     #define IXGBEVF_DEFAULT_TXD    1024
>>>>   #define IXGBEVF_DEFAULT_RXD    512
>>>> diff --git a/drivers/net/ethernet/intel/ixgbevf/mbx.h 
>>>> b/drivers/net/ethernet/intel/ixgbevf/mbx.h
>>>> index 66e138b..82f44e0 100644
>>>> --- a/drivers/net/ethernet/intel/ixgbevf/mbx.h
>>>> +++ b/drivers/net/ethernet/intel/ixgbevf/mbx.h
>>>> @@ -110,6 +110,7 @@ enum ixgbe_pfvf_api_rev {
>>>>     /* mailbox API, version 1.2 VF requests */
>>>>   #define IXGBE_VF_GET_RETA    0x0a    /* VF request for 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
>>>> diff --git a/drivers/net/ethernet/intel/ixgbevf/vf.c 
>>>> b/drivers/net/ethernet/intel/ixgbevf/vf.c
>>>> index 2676c0b..ec68145 100644
>>>> --- a/drivers/net/ethernet/intel/ixgbevf/vf.c
>>>> +++ b/drivers/net/ethernet/intel/ixgbevf/vf.c
>>>> @@ -301,6 +301,72 @@ static inline int 
>>>> ixgbevf_get_reta_locked(struct ixgbe_hw *hw, u32 *msgbuf,
>>>>       return 0;
>>>>   }
>>>>   +static inline int ixgbevf_get_rss_key_locked(struct ixgbe_hw 
>>>> *hw, u8 *rss_key)
>>>> +{
>>>> +    int err;
>>>> +    u32 msgbuf[IXGBE_VFMAILBOX_SIZE];
>>>> +
>>>> +    /* We currently support the RSS Random Key retrieval for 82599 
>>>> and x540
>>>> +     * devices only.
>>>> +     *
>>>> +     * Thus return an error if API doesn't support RSS Random Key 
>>>> retrieval
>>>> +     * or if the operation is not supported for this device type.
>>>> +     */
>>>> +    if (hw->api_version != ixgbe_mbox_api_12 ||
>>>> +        hw->mac.type >= ixgbe_mac_X550_vf)
>>>> +        return -EPERM;
>>>> +
>>> The return type here should be not supported, or IXGBE_NOT_IMPLEMENTED,
>>> not no permissions.
>>
>> I think a standard EOPNOTSUPP would be a much better choice.
>> Why do u have this ixgbe-specific return values in addition to 
>> standard POSIX ones anyway?
>
> It has to do with being OS agnostic.  If you want to port this to BSD, 
> Linux, DPDK, etc, then you can't always rely on all the error return 
> values being there so there is a tendency to implement your own.  As 
> with the other function you might be best off just returning 
> IXGBE_ERR_MBX since the x550 and API_12 don't support the RSS messages.

Returning the IXGBE_ERR_MBX from the ethtool callback would be the most 
confusing since it is defined to -100 which corresponds to ENETDOWN 
value in Linux. This would result in the most confusing ethtool error 
message.

>
> - Alex

Re: [PATCH net-next v9 6/7] ixgbevf: Add RSS Key query code

[Alexander Duyck]

On 03/30/2015 08:17 AM, Vlad Zolotarov wrote:
>
>
> On 03/30/15 18:10, Alexander Duyck wrote:
>>
>> On 03/30/2015 06:53 AM, Vlad Zolotarov wrote:
>>>
>>>
>>> On 03/30/15 01:04, Alexander Duyck wrote:
>>>> On 03/29/2015 09:11 AM, Vlad Zolotarov wrote:
>>>>> 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.
>>>>>
>>>>> This patch adds the support for 82599 and x540 devices only. 
>>>>> Support for other
>>>>> devices will be added later.
>>>>>
>>>>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>>>>> ---
>>>>> New in v9:
>>>>>     - Reduce the support to 82599 and x540 devices only.
>>>>>     - Added IXGBEVF_RSS_HASH_KEY_SIZE macro.
>>>>>
>>>>> New in v8:
>>>>>     - Protect a mailbox access.
>>>>>
>>>>> New in v6:
>>>>>     - Return a proper return code when an operation is blocked by PF.
>>>>>
>>>>> 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/ixgbevf.h |  1 +
>>>>>   drivers/net/ethernet/intel/ixgbevf/mbx.h     |  1 +
>>>>>   drivers/net/ethernet/intel/ixgbevf/vf.c      | 66 
>>>>> ++++++++++++++++++++++++++++
>>>>>   drivers/net/ethernet/intel/ixgbevf/vf.h      |  1 +
>>>>>   4 files changed, 69 insertions(+)
>>>>>
>>>>> diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h 
>>>>> b/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
>>>>> index bc939a1..6771ae3 100644
>>>>> --- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
>>>>> +++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
>>>>> @@ -145,6 +145,7 @@ struct ixgbevf_ring {
>>>>>   #define MAX_RX_QUEUES IXGBE_VF_MAX_RX_QUEUES
>>>>>   #define MAX_TX_QUEUES IXGBE_VF_MAX_TX_QUEUES
>>>>>   #define IXGBEVF_MAX_RSS_QUEUES    2
>>>>> +#define IXGBEVF_RSS_HASH_KEY_SIZE    40
>>>>>     #define IXGBEVF_DEFAULT_TXD    1024
>>>>>   #define IXGBEVF_DEFAULT_RXD    512
>>>>> diff --git a/drivers/net/ethernet/intel/ixgbevf/mbx.h 
>>>>> b/drivers/net/ethernet/intel/ixgbevf/mbx.h
>>>>> index 66e138b..82f44e0 100644
>>>>> --- a/drivers/net/ethernet/intel/ixgbevf/mbx.h
>>>>> +++ b/drivers/net/ethernet/intel/ixgbevf/mbx.h
>>>>> @@ -110,6 +110,7 @@ enum ixgbe_pfvf_api_rev {
>>>>>     /* mailbox API, version 1.2 VF requests */
>>>>>   #define IXGBE_VF_GET_RETA    0x0a    /* VF request for 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
>>>>> diff --git a/drivers/net/ethernet/intel/ixgbevf/vf.c 
>>>>> b/drivers/net/ethernet/intel/ixgbevf/vf.c
>>>>> index 2676c0b..ec68145 100644
>>>>> --- a/drivers/net/ethernet/intel/ixgbevf/vf.c
>>>>> +++ b/drivers/net/ethernet/intel/ixgbevf/vf.c
>>>>> @@ -301,6 +301,72 @@ static inline int 
>>>>> ixgbevf_get_reta_locked(struct ixgbe_hw *hw, u32 *msgbuf,
>>>>>       return 0;
>>>>>   }
>>>>>   +static inline int ixgbevf_get_rss_key_locked(struct ixgbe_hw 
>>>>> *hw, u8 *rss_key)
>>>>> +{
>>>>> +    int err;
>>>>> +    u32 msgbuf[IXGBE_VFMAILBOX_SIZE];
>>>>> +
>>>>> +    /* We currently support the RSS Random Key retrieval for 
>>>>> 82599 and x540
>>>>> +     * devices only.
>>>>> +     *
>>>>> +     * Thus return an error if API doesn't support RSS Random Key 
>>>>> retrieval
>>>>> +     * or if the operation is not supported for this device type.
>>>>> +     */
>>>>> +    if (hw->api_version != ixgbe_mbox_api_12 ||
>>>>> +        hw->mac.type >= ixgbe_mac_X550_vf)
>>>>> +        return -EPERM;
>>>>> +
>>>> The return type here should be not supported, or 
>>>> IXGBE_NOT_IMPLEMENTED,
>>>> not no permissions.
>>>>
>>>>> +    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 the operation has been refused by a PF return -EPERM */
>>>>> +    if (msgbuf[0] == (IXGBE_VF_GET_RETA | IXGBE_VT_MSGTYPE_NACK))
>>>>> +        return -EPERM;
>>>>> +
>>>>> +    /* 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, IXGBEVF_RSS_HASH_KEY_SIZE);
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * 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.
>>>>> + * Ensures the atomicy of a mailbox access using the 
>>>>> adapter.mbx_lock spinlock.
>>>>> + *
>>>>> + * Returns: 0 on success.
>>>>> + *          if API doesn't support this operation - (-EPERM).
>>>>> + */
>>>>> +int ixgbevf_get_rss_key(struct ixgbevf_adapter *a, u8 *rss_key)
>>>>> +{
>>>>> +    int rc;
>>>>> +
>>>>> +    spin_lock_bh(&a->mbx_lock);
>>>>> +    rc = ixgbevf_get_rss_key_locked(&a->hw, rss_key);
>>>>> +    spin_unlock_bh(&a->mbx_lock);
>>>>> +
>>>>> +    return rc;
>>>>> +}
>>>>> +
>>>> Since there is currently no cases where you would be getting the 
>>>> rss_key
>>>> without the RETA you might just want to combine this function with
>>>> ixgbevf_get_reta so you only take the lock once and process both
>>>> messages in one pass instead of having to bounce the spinlock.
>>>
>>> I'd rather not do this.
>>> Let's start from the beginning: the locks here should be removed and
>>> added at the caller level to match what u wrote about patch04.
>>> Then about the uniting the two functions mentioned above - there isn't
>>> any added value of doing this. Taking a lock only once may be done
>>> without uniting (see PATCH07 in v10 series).
>>> On the other hand, this uniting is going to make the code awkward and
>>> unclear ("why are these together anyway?").
>>> So, I'd rather keep these functions as they are apart from fixing 
>>> the locking issue. I'll essentially export the _locked functions.
>>>
>>> thanks,
>>> vlad
>>
>> Agreed, just export the _locked functions and drop the _locked 
>> extension.
>
> Hmmm... I think keeping the _locked extension would make sense since 
> it would explicitly hint that these functions have to be called under 
> the lock. There are other similar examples in the ixgbe/ixgbevf code 
> already.

The thing is the mailbox lock is only really needed to avoid contention 
with the few rare cases where the mailbox is accessed outside of the 
RTNL lock, also there is going to end up needing to be a different 
version of the function for x550 since it has direct register access to 
the redirection table via registers (mailbox lock is not needed since it 
is direct register access), so it might be worth it to take that into 
account in the naming.

One option you might consider would be to look at adding get_reta and 
get_rssrk function pointers to the ixgbe_mac_operations structure and 
then splitting up ixgbevf_mac_ops so that you have the original, and an 
x550 version that doesn't populate those two function pointers.  That 
might make it easier to then implement the x550 version later.  You 
could probably even do something similar for the PF functionality as 
that way you could easily identify the functions that support getting 
the RETA and RSSRK by simply checking to see if those function pointers 
are present.

As far as the other patches I am okay with the return being -ENOTSUPP 
for the x550 for now.

- Alex

Re: [PATCH net-next v9 6/7] ixgbevf: Add RSS Key query code

[Vlad Zolotarov]

On 03/30/15 19:54, Alexander Duyck wrote:
>
> On 03/30/2015 08:17 AM, Vlad Zolotarov wrote:
>>
>>
>> On 03/30/15 18:10, Alexander Duyck wrote:
>>>
>>> On 03/30/2015 06:53 AM, Vlad Zolotarov wrote:
>>>>
>>>>
>>>> On 03/30/15 01:04, Alexander Duyck wrote:
>>>>> On 03/29/2015 09:11 AM, Vlad Zolotarov wrote:
>>>>>> 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.
>>>>>>
>>>>>> This patch adds the support for 82599 and x540 devices only. 
>>>>>> Support for other
>>>>>> devices will be added later.
>>>>>>
>>>>>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>>>>>> ---
>>>>>> New in v9:
>>>>>>     - Reduce the support to 82599 and x540 devices only.
>>>>>>     - Added IXGBEVF_RSS_HASH_KEY_SIZE macro.
>>>>>>
>>>>>> New in v8:
>>>>>>     - Protect a mailbox access.
>>>>>>
>>>>>> New in v6:
>>>>>>     - Return a proper return code when an operation is blocked by 
>>>>>> PF.
>>>>>>
>>>>>> 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/ixgbevf.h |  1 +
>>>>>>   drivers/net/ethernet/intel/ixgbevf/mbx.h     |  1 +
>>>>>>   drivers/net/ethernet/intel/ixgbevf/vf.c      | 66 
>>>>>> ++++++++++++++++++++++++++++
>>>>>>   drivers/net/ethernet/intel/ixgbevf/vf.h      |  1 +
>>>>>>   4 files changed, 69 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h 
>>>>>> b/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
>>>>>> index bc939a1..6771ae3 100644
>>>>>> --- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
>>>>>> +++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
>>>>>> @@ -145,6 +145,7 @@ struct ixgbevf_ring {
>>>>>>   #define MAX_RX_QUEUES IXGBE_VF_MAX_RX_QUEUES
>>>>>>   #define MAX_TX_QUEUES IXGBE_VF_MAX_TX_QUEUES
>>>>>>   #define IXGBEVF_MAX_RSS_QUEUES    2
>>>>>> +#define IXGBEVF_RSS_HASH_KEY_SIZE    40
>>>>>>     #define IXGBEVF_DEFAULT_TXD    1024
>>>>>>   #define IXGBEVF_DEFAULT_RXD    512
>>>>>> diff --git a/drivers/net/ethernet/intel/ixgbevf/mbx.h 
>>>>>> b/drivers/net/ethernet/intel/ixgbevf/mbx.h
>>>>>> index 66e138b..82f44e0 100644
>>>>>> --- a/drivers/net/ethernet/intel/ixgbevf/mbx.h
>>>>>> +++ b/drivers/net/ethernet/intel/ixgbevf/mbx.h
>>>>>> @@ -110,6 +110,7 @@ enum ixgbe_pfvf_api_rev {
>>>>>>     /* mailbox API, version 1.2 VF requests */
>>>>>>   #define IXGBE_VF_GET_RETA    0x0a    /* VF request for 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
>>>>>> diff --git a/drivers/net/ethernet/intel/ixgbevf/vf.c 
>>>>>> b/drivers/net/ethernet/intel/ixgbevf/vf.c
>>>>>> index 2676c0b..ec68145 100644
>>>>>> --- a/drivers/net/ethernet/intel/ixgbevf/vf.c
>>>>>> +++ b/drivers/net/ethernet/intel/ixgbevf/vf.c
>>>>>> @@ -301,6 +301,72 @@ static inline int 
>>>>>> ixgbevf_get_reta_locked(struct ixgbe_hw *hw, u32 *msgbuf,
>>>>>>       return 0;
>>>>>>   }
>>>>>>   +static inline int ixgbevf_get_rss_key_locked(struct ixgbe_hw 
>>>>>> *hw, u8 *rss_key)
>>>>>> +{
>>>>>> +    int err;
>>>>>> +    u32 msgbuf[IXGBE_VFMAILBOX_SIZE];
>>>>>> +
>>>>>> +    /* We currently support the RSS Random Key retrieval for 
>>>>>> 82599 and x540
>>>>>> +     * devices only.
>>>>>> +     *
>>>>>> +     * Thus return an error if API doesn't support RSS Random 
>>>>>> Key retrieval
>>>>>> +     * or if the operation is not supported for this device type.
>>>>>> +     */
>>>>>> +    if (hw->api_version != ixgbe_mbox_api_12 ||
>>>>>> +        hw->mac.type >= ixgbe_mac_X550_vf)
>>>>>> +        return -EPERM;
>>>>>> +
>>>>> The return type here should be not supported, or 
>>>>> IXGBE_NOT_IMPLEMENTED,
>>>>> not no permissions.
>>>>>
>>>>>> +    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 the operation has been refused by a PF return -EPERM */
>>>>>> +    if (msgbuf[0] == (IXGBE_VF_GET_RETA | IXGBE_VT_MSGTYPE_NACK))
>>>>>> +        return -EPERM;
>>>>>> +
>>>>>> +    /* 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, IXGBEVF_RSS_HASH_KEY_SIZE);
>>>>>> +
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +
>>>>>> +/**
>>>>>> + * 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.
>>>>>> + * Ensures the atomicy of a mailbox access using the 
>>>>>> adapter.mbx_lock spinlock.
>>>>>> + *
>>>>>> + * Returns: 0 on success.
>>>>>> + *          if API doesn't support this operation - (-EPERM).
>>>>>> + */
>>>>>> +int ixgbevf_get_rss_key(struct ixgbevf_adapter *a, u8 *rss_key)
>>>>>> +{
>>>>>> +    int rc;
>>>>>> +
>>>>>> +    spin_lock_bh(&a->mbx_lock);
>>>>>> +    rc = ixgbevf_get_rss_key_locked(&a->hw, rss_key);
>>>>>> +    spin_unlock_bh(&a->mbx_lock);
>>>>>> +
>>>>>> +    return rc;
>>>>>> +}
>>>>>> +
>>>>> Since there is currently no cases where you would be getting the 
>>>>> rss_key
>>>>> without the RETA you might just want to combine this function with
>>>>> ixgbevf_get_reta so you only take the lock once and process both
>>>>> messages in one pass instead of having to bounce the spinlock.
>>>>
>>>> I'd rather not do this.
>>>> Let's start from the beginning: the locks here should be removed and
>>>> added at the caller level to match what u wrote about patch04.
>>>> Then about the uniting the two functions mentioned above - there isn't
>>>> any added value of doing this. Taking a lock only once may be done
>>>> without uniting (see PATCH07 in v10 series).
>>>> On the other hand, this uniting is going to make the code awkward and
>>>> unclear ("why are these together anyway?").
>>>> So, I'd rather keep these functions as they are apart from fixing 
>>>> the locking issue. I'll essentially export the _locked functions.
>>>>
>>>> thanks,
>>>> vlad
>>>
>>> Agreed, just export the _locked functions and drop the _locked 
>>> extension.
>>
>> Hmmm... I think keeping the _locked extension would make sense since 
>> it would explicitly hint that these functions have to be called under 
>> the lock. There are other similar examples in the ixgbe/ixgbevf code 
>> already.
>
> The thing is the mailbox lock is only really needed to avoid 
> contention with the few rare cases where the mailbox is accessed 
> outside of the RTNL lock, 

Since there are such cases - every access to mailbox has to be protected.

> also there is going to end up needing to be a different version of the 
> function for x550 since it has direct register access to the 
> redirection table via registers (mailbox lock is not needed since it 
> is direct register access), so it might be worth it to take that into 
> account in the naming.

That's exactly what I'm going to do here, don't u think? ;) Since for 
82599 and x540 it should be "locked" the name of the corresponding 
function reflects it.

>
> One option you might consider would be to look at adding get_reta and 
> get_rssrk function pointers to the ixgbe_mac_operations structure and 
> then splitting up ixgbevf_mac_ops so that you have the original, and 
> an x550 version that doesn't populate those two function pointers.  
> That might make it easier to then implement the x550 version later.  
> You could probably even do something similar for the PF functionality 
> as that way you could easily identify the functions that support 
> getting the RETA and RSSRK by simply checking to see if those function 
> pointers are present.

I'll leave this to consider to the one that is going to add the x550 
support.

>
> As far as the other patches I am okay with the return being -ENOTSUPP 
> for the x550 for now.
>
> - Alex
>