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

[PATCH net-next v6 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.

On some 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 disable this
feature by default for such devices (e.g. 82599) and allow it for those where there isn't any
possible risk (e.g. on x550). The new netdev op is going to allow a system administrator to
change the default behaviour with "ip link set" command.

 - 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 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      |  10 ++
 drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c    | 119 ++++++++++++++++++
 drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h    |   2 +
 drivers/net/ethernet/intel/ixgbevf/ethtool.c      |  58 +++++++++
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |   4 +-
 drivers/net/ethernet/intel/ixgbevf/mbx.h          |  10 ++
 drivers/net/ethernet/intel/ixgbevf/vf.c           | 140 ++++++++++++++++++++++
 drivers/net/ethernet/intel/ixgbevf/vf.h           |   2 +
 include/linux/if_link.h                           |   1 +
 include/linux/netdevice.h                         |   8 ++
 include/uapi/linux/if_link.h                      |   8 ++
 net/core/rtnetlink.c                              |  32 ++++-
 14 files changed, 395 insertions(+), 7 deletions(-)

-- 
2.1.0

[PATCH net-next v6 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 5ae69e7..883a926 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 f5f5edd..3cc2c5a 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -464,6 +464,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,
 };
 
@@ -508,6 +511,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 e769510..719ecde 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

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

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 900 bytes --]

On Sun, 2015-03-22 at 21:01 +0200, Vlad Zolotarov wrote:
> 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(-)

Thanks Vlad, I have applied your patch to my next-queue tree
-- 
git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/next-queue.git
dev-queue

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH net-next v6 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.

Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
---
 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 | 27 ++++++++++++++++++++++++++
 drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h |  2 ++
 4 files changed, 37 insertions(+)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index 7068e9c..5e44e48 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 395dc6b..1999306 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -3600,6 +3600,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)
@@ -8040,6 +8046,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..4d67677 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
@@ -108,6 +108,19 @@ 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;
+
+		/* by default:
+		 *   - Enable RSS query for x550 devices. x550 VFs don't share
+		 *     RSS Redirection Table and RSS Hash Key with a PF, so
+		 *     there isn't any possible security threat in allowing them
+		 *     to query this information.
+		 *   - disable - for all the rest since they do share it with a
+		 *     PF.
+		 */
+		for (i = 0; i < adapter->num_vfs; i++)
+			adapter->vfinfo[i].rss_query_enabled =
+				       (adapter->hw.mac.type >= ixgbe_mac_X550);
+
 		return 0;
 	}
 
@@ -1330,6 +1343,19 @@ 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);
+
+	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 +1369,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

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

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 745 bytes --]

On Sun, 2015-03-22 at 21:01 +0200, 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.
> 
> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
> ---
>  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 | 27
> ++++++++++++++++++++++++++
>  drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h |  2 ++
>  4 files changed, 37 insertions(+)

Thanks Vlad, I have applied your patch to my next-queue tree
-- 
git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/next-queue.git
dev-queue

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Vlad Zolotarov]

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

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

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

Thus this patch does the following:

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

Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
---
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   |  8 ++++
 drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c | 65 ++++++++++++++++++++++++++
 2 files changed, 73 insertions(+)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h
index a5cb755..f9b5eae 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h
@@ -73,6 +73,7 @@ enum ixgbe_pfvf_api_rev {
 	ixgbe_mbox_api_10,	/* API version 1.0, linux/freebsd VF driver */
 	ixgbe_mbox_api_20,	/* API version 2.0, solaris Phase1 VF driver */
 	ixgbe_mbox_api_11,	/* API version 1.1, linux/freebsd VF driver */
+	ixgbe_mbox_api_12,	/* API version 1.2, linux/freebsd VF driver */
 	/* This value should always be last */
 	ixgbe_mbox_api_unknown,	/* indicates that API version is not known */
 };
@@ -97,6 +98,13 @@ enum ixgbe_pfvf_api_rev {
 #define IXGBE_VF_TRANS_VLAN	3	/* Indication of port vlan */
 #define IXGBE_VF_DEF_QUEUE	4	/* Default queue offset */
 
+/* mailbox API, version 1.2 VF requests */
+#define IXGBE_VF_GET_RETA	0x0a	/* VF request for RETA */
+
+/* GET_RETA request data indices within the mailbox */
+#define IXGBE_VF_RETA_SZ        1	/* Number of RETA DWs to bring */
+#define IXGBE_VF_RETA_OFFSET    2	/* Offset in RETA */
+
 /* length of permanent address message returned from PF */
 #define IXGBE_VF_PERMADDR_MSG_LEN 4
 /* word in permanent address message with the current multicast type */
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
index 4d67677..301bb1b 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
@@ -437,6 +437,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
@@ -904,6 +905,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:
@@ -927,6 +929,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;
@@ -954,6 +957,65 @@ static int ixgbe_get_vf_queues(struct ixgbe_adapter *adapter,
 	return 0;
 }
 
+static int ixgbe_get_vf_reta(struct ixgbe_adapter *adapter, u32 *msgbuf, u32 vf)
+{
+	struct ixgbe_hw *hw = &adapter->hw;
+	int i, j;
+	u32 *reta = &msgbuf[1];
+	u32 mask = 0;
+	u32 psrtype;
+	u32 reta_offset_dw = msgbuf[IXGBE_VF_RETA_OFFSET];
+	u32 dwords = msgbuf[IXGBE_VF_RETA_SZ];
+
+	/* verify the PF is supporting the correct API */
+	if (!adapter->vfinfo[vf].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 X550:
+	 * 512 (8 bit wide) entries containing 6 bit RSS index
+	 *
+	 * PSRTYPE[n].RQPL defines if 0, 1 or 2 bits from the redirection table
+	 * value should be used.
+	 */
+
+	if ((psrtype & (1 << 29)) == (1 << 29))
+		mask = 0x01010101;
+	else if ((psrtype & (2 << 29)) == (2 << 29))
+		mask = 0x03030303;
+	else
+		mask = 0;
+
+	switch (hw->mac.type) {
+	case ixgbe_mac_82599EB:
+	case ixgbe_mac_X540:
+		/* Read the appropriate portion of RETA */
+		for (i = 0; i < dwords; i++)
+			reta[i] = IXGBE_READ_REG(hw,
+						IXGBE_RETA(i + reta_offset_dw));
+		break;
+	case ixgbe_mac_X550:
+	case ixgbe_mac_X550EM_x:
+		/* X550 has a per-VF RETA */
+		for (i = 0, j = reta_offset_dw; i < dwords; i++, j++)
+			reta[i] = IXGBE_READ_REG(hw,
+						IXGBE_PFVFRETA(j, vf));
+		break;
+	default:
+		return -1;
+	}
+
+	/* Mask the relevant bits */
+	for (i = 0; i < dwords; i++)
+		reta[i] &= mask;
+
+	return 0;
+}
+
 static int ixgbe_rcv_msg_from_vf(struct ixgbe_adapter *adapter, u32 vf)
 {
 	u32 mbx_size = IXGBE_VFMAILBOX_SIZE;
@@ -1010,6 +1072,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

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

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 2142 bytes --]

On Sun, 2015-03-22 at 21:01 +0200, Vlad Zolotarov wrote:
> 82599 and x540 VFs and PF share the same RSS redirection table (RETA).
> Therefore we
> just return it for all VFs. x550 on the other hand provides a separate
> redirection
> table for each VF (there is a per-pool RETA table).
> 
> For 82599 and x540 RETA table is an array of 32 registers (128 bytes)
> and the maximum number of
> registers that may be delivered in a single VF-PF channel command is
> 15. Therefore
> we will deliver the whole table in 3 steps: 12, 12 and 8 registers in
> each
> step correspondingly.
> 
> For x550 VFs RETA is a 64 byte array, so we may deliver it in two
> steps: 12 and 4 registers
> correspondingly.
> 
> Thus this patch does the following:
> 
>   - Adds a new API version (to specify a new commands set).
>   - Adds the IXGBE_VF_GET_RETA command to the VF-PF commands set.
> 
> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
> ---
> 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   |  8 ++++
>  drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c | 65
> ++++++++++++++++++++++++++
>  2 files changed, 73 insertions(+)

Thanks Vlad, I have applied your patch to my next-queue tree
-- 
git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/next-queue.git
dev-queue

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Vlad Zolotarov]

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

Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
---
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 |  4 +-
 drivers/net/ethernet/intel/ixgbevf/mbx.h          |  8 ++
 drivers/net/ethernet/intel/ixgbevf/vf.c           | 92 +++++++++++++++++++++++
 drivers/net/ethernet/intel/ixgbevf/vf.h           |  1 +
 4 files changed, 104 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..4787fcf 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;
@@ -3712,6 +3713,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..7342554 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,13 @@ enum ixgbe_pfvf_api_rev {
 #define IXGBE_VF_TRANS_VLAN	3 /* Indication of port VLAN */
 #define IXGBE_VF_DEF_QUEUE	4 /* Default queue offset */
 
+/* mailbox API, version 1.2 VF requests */
+#define IXGBE_VF_GET_RETA	0x0a	/* VF request for RETA */
+
+/* GET_RETA request data indices within the mailbox */
+#define IXGBE_VF_RETA_SZ        1	/* Number of RETA DWs to bring */
+#define IXGBE_VF_RETA_OFFSET    2	/* Offset in RETA */
+
 /* length of permanent address message returned from PF */
 #define IXGBE_VF_PERMADDR_MSG_LEN	4
 /* word in permanent address message with the current multicast type */
diff --git a/drivers/net/ethernet/intel/ixgbevf/vf.c b/drivers/net/ethernet/intel/ixgbevf/vf.c
index 2614fd3..f31e097 100644
--- a/drivers/net/ethernet/intel/ixgbevf/vf.c
+++ b/drivers/net/ethernet/intel/ixgbevf/vf.c
@@ -256,6 +256,97 @@ static s32 ixgbevf_set_uc_addr_vf(struct ixgbe_hw *hw, u32 index, u8 *addr)
 	return ret_val;
 }
 
+static inline int _ixgbevf_get_reta(struct ixgbe_hw *hw, u32 *msgbuf,
+				    u32 *reta, u32 reta_offset_dw, u32 dwords)
+{
+	int err;
+
+	msgbuf[0]                    = IXGBE_VF_GET_RETA;
+	msgbuf[IXGBE_VF_RETA_SZ]     = dwords;
+	msgbuf[IXGBE_VF_RETA_OFFSET] = reta_offset_dw;
+
+	err = hw->mbx.ops.write_posted(hw, msgbuf, 3);
+
+	if (err)
+		return err;
+
+	err = hw->mbx.ops.read_posted(hw, msgbuf, 1 + dwords);
+
+	if (err)
+		return err;
+
+	msgbuf[0] &= ~IXGBE_VT_MSGTYPE_CTS;
+
+	/* If 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;
+
+	memcpy(reta + reta_offset_dw, msgbuf + 1, 4 * dwords);
+
+	return 0;
+}
+
+/**
+ * ixgbevf_get_reta - get the RSS redirection table (RETA) contents.
+ * @hw: pointer to the HW structure
+ * @reta: buffer to fill with RETA contents.
+ *
+ * The "reta" buffer should be big enough to contain 32 registers.
+ *
+ * Returns: 0 on success.
+ *          if API doesn't support this operation - (-EPERM).
+ */
+int ixgbevf_get_reta(struct ixgbe_hw *hw, u32 *reta)
+{
+	int err;
+	u32 msgbuf[IXGBE_VFMAILBOX_SIZE];
+
+	/* Return an error if API doesn't RETA querying. */
+	if (hw->api_version != ixgbe_mbox_api_12)
+		return -EPERM;
+
+	/* x550 devices have a separate RETA for each VF: 64 bytes each.
+	 *
+	 * We'll get it in 2 steps due to mailbox size limitation - we can bring
+	 * up to 15 dwords every time. Therefore we'll bring 12 and 4 dwords.
+	 *
+	 * Older devices share a RETA table with the PF: 128 bytes.
+	 *
+	 * For them we do it in 3 steps. Therefore we'll bring it in 3 steps:
+	 * 12, 12 and 8 dwords in each step correspondingly.
+	 */
+
+	/* RETA[0..11] */
+	err = _ixgbevf_get_reta(hw, msgbuf, reta, 0, 12);
+	if (err)
+		return err;
+
+	if (hw->mac.type >= ixgbe_mac_X550_vf) {
+		/* RETA[12..15] */
+		err = _ixgbevf_get_reta(hw, msgbuf, reta, 12, 4);
+		if (err)
+			return err;
+
+	} else {
+		/* RETA[12..23] */
+		err = _ixgbevf_get_reta(hw, msgbuf, reta, 12, 12);
+		if (err)
+			return err;
+
+		/* RETA[24..31] */
+		err = _ixgbevf_get_reta(hw, msgbuf, reta, 24, 8);
+	}
+
+	return err;
+}
+
 /**
  *  ixgbevf_set_rar_vf - set device MAC address
  *  @hw: pointer to hardware structure
@@ -545,6 +636,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..89bc88e 100644
--- a/drivers/net/ethernet/intel/ixgbevf/vf.h
+++ b/drivers/net/ethernet/intel/ixgbevf/vf.h
@@ -210,4 +210,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 ixgbe_hw *hw, u32 *reta);
 #endif /* __IXGBE_VF_H__ */
-- 
2.1.0

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

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 1081 bytes --]

On Sun, 2015-03-22 at 21:01 +0200, Vlad Zolotarov wrote:
>    - Added a new API version support.
>    - Added the query implementation in the ixgbevf.
> 
> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
> ---
> 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 |  4 +-
>  drivers/net/ethernet/intel/ixgbevf/mbx.h          |  8 ++
>  drivers/net/ethernet/intel/ixgbevf/vf.c           | 92
> +++++++++++++++++++++++
>  drivers/net/ethernet/intel/ixgbevf/vf.h           |  1 +
>  4 files changed, 104 insertions(+), 1 deletion(-)

Thanks Vlad, I have applied your patch to my next-queue tree
-- 
git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/next-queue.git
dev-queue

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Tantilov, Emil S]

>-----Original Message-----
>From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On Behalf Of Vlad Zolotarov
>Sent: Sunday, March 22, 2015 12:01 PM
>
>Subject: [PATCH net-next v6 4/7] ixgbevf: Add a RETA query code
>
>   - Added a new API version support.
>   - Added the query implementation in the ixgbevf.
>
>Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>---
>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 |  4 +-
> drivers/net/ethernet/intel/ixgbevf/mbx.h          |  8 ++
> drivers/net/ethernet/intel/ixgbevf/vf.c           | 92 +++++++++++++++++++++++
> drivers/net/ethernet/intel/ixgbevf/vf.h           |  1 +
> 4 files changed, 104 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..4787fcf 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;
>@@ -3712,6 +3713,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:

You need another case for ixgbe_mbox_api_12 in ixgbevf_set_num_queues(), otherwise the driver will load with a single queue.

Thanks,
Emil

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

[Vlad Zolotarov]

On 03/24/15 01:46, Tantilov, Emil S wrote:
>> -----Original Message-----
>> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On Behalf Of Vlad Zolotarov
>> Sent: Sunday, March 22, 2015 12:01 PM
>>
>> Subject: [PATCH net-next v6 4/7] ixgbevf: Add a RETA query code
>>
>>    - Added a new API version support.
>>    - Added the query implementation in the ixgbevf.
>>
>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>> ---
>> 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 |  4 +-
>> drivers/net/ethernet/intel/ixgbevf/mbx.h          |  8 ++
>> drivers/net/ethernet/intel/ixgbevf/vf.c           | 92 +++++++++++++++++++++++
>> drivers/net/ethernet/intel/ixgbevf/vf.h           |  1 +
>> 4 files changed, 104 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..4787fcf 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;
>> @@ -3712,6 +3713,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:
> You need another case for ixgbe_mbox_api_12 in ixgbevf_set_num_queues(), otherwise the driver will load with a single queue.

Right. I've missed this place since it's been added after the first 
series version and 'git rebase' silently integrated it... ;)

I have a question about this new code though. Why does ixgbevf ignores 
IXGBE_VF_RX_QUEUES PF returns it in IXGBE_VF_GET_QUEUE?
PF returns there 1 by the way (see ixgbe_get_vf_queues()) despite the 
fact it configures the VF to have up to 4 queues... I do understand why 
it allows up to 4 queues but why does it return 1 for IXGBE_VF_RX_QUEUES 
in IXGBE_VF_GET_QUEUE? Shouldn't it return 4 there?

Could somebody clarify this design to me, please?

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

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

[Tantilov, Emil S]

>-----Original Message-----
>From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com] 
>Sent: Tuesday, March 24, 2015 3:26 AM
>Subject: Re: [PATCH net-next v6 4/7] ixgbevf: Add a RETA query code
>
>
>
>On 03/24/15 01:46, Tantilov, Emil S wrote:
>>> -----Original Message-----
>>> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On Behalf Of Vlad Zolotarov
>>> Sent: Sunday, March 22, 2015 12:01 PM
>>>
>>> Subject: [PATCH net-next v6 4/7] ixgbevf: Add a RETA query code
>>>
>>>    - Added a new API version support.
>>>    - Added the query implementation in the ixgbevf.
>>>
>>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>>> ---
>>> 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 |  4 +-
>>> drivers/net/ethernet/intel/ixgbevf/mbx.h          |  8 ++
>>> drivers/net/ethernet/intel/ixgbevf/vf.c           | 92 +++++++++++++++++++++++
>>> drivers/net/ethernet/intel/ixgbevf/vf.h           |  1 +
>>> 4 files changed, 104 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..4787fcf 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;
>>> @@ -3712,6 +3713,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:
>> You need another case for ixgbe_mbox_api_12 in ixgbevf_set_num_queues(), otherwise the driver will load with a >>single queue.
>
>Right. I've missed this place since it's been added after the first 
>series version and 'git rebase' silently integrated it... ;)
>
>I have a question about this new code though. Why does ixgbevf ignores 
>IXGBE_VF_RX_QUEUES PF returns it in IXGBE_VF_GET_QUEUE?
>PF returns there 1 by the way (see ixgbe_get_vf_queues()) despite the 
>fact it configures the VF to have up to 4 queues... I do understand why 
>it allows up to 4 queues but why does it return 1 for IXGBE_VF_RX_QUEUES 
>in IXGBE_VF_GET_QUEUE? Shouldn't it return 4 there?

I'm not sure where you see this, on my setup ixgbevf_get_queues() gets 4 in msg[IXGBE_VF_R/TX_QUEUES] which is used to set hw->mac.max_t/rx_queues.

BTW - there are other issues with your patches. The indirection table seems to come out as all 0s and the VF driver reports link down/up when querying it.

Where is this information useful anyway - what is the use case? There is no description in your patches for why all this is needed.

Thanks,
Emil

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

[Vlad Zolotarov]

On 03/24/15 20:12, Tantilov, Emil S wrote:
>> -----Original Message-----
>> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
>> Sent: Tuesday, March 24, 2015 3:26 AM
>> Subject: Re: [PATCH net-next v6 4/7] ixgbevf: Add a RETA query code
>>
>>
>>
>> On 03/24/15 01:46, Tantilov, Emil S wrote:
>>>> -----Original Message-----
>>>> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On Behalf Of Vlad Zolotarov
>>>> Sent: Sunday, March 22, 2015 12:01 PM
>>>>
>>>> Subject: [PATCH net-next v6 4/7] ixgbevf: Add a RETA query code
>>>>
>>>>     - Added a new API version support.
>>>>     - Added the query implementation in the ixgbevf.
>>>>
>>>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>>>> ---
>>>> 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 |  4 +-
>>>> drivers/net/ethernet/intel/ixgbevf/mbx.h          |  8 ++
>>>> drivers/net/ethernet/intel/ixgbevf/vf.c           | 92 +++++++++++++++++++++++
>>>> drivers/net/ethernet/intel/ixgbevf/vf.h           |  1 +
>>>> 4 files changed, 104 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..4787fcf 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;
>>>> @@ -3712,6 +3713,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:
>>> You need another case for ixgbe_mbox_api_12 in ixgbevf_set_num_queues(), otherwise the driver will load with a >>single queue.
>> Right. I've missed this place since it's been added after the first
>> series version and 'git rebase' silently integrated it... ;)
>>
>> I have a question about this new code though. Why does ixgbevf ignores
>> IXGBE_VF_RX_QUEUES PF returns it in IXGBE_VF_GET_QUEUE?
>> PF returns there 1 by the way (see ixgbe_get_vf_queues()) despite the
>> fact it configures the VF to have up to 4 queues... I do understand why
>> it allows up to 4 queues but why does it return 1 for IXGBE_VF_RX_QUEUES
>> in IXGBE_VF_GET_QUEUE? Shouldn't it return 4 there?
> I'm not sure where you see this, on my setup ixgbevf_get_queues() gets 4 in msg[IXGBE_VF_R/TX_QUEUES] which is used to set hw->mac.max_t/rx_queues.

Right. I misread the __ALIGN_MASK() macro.
But then I can't see where max_rx_queues is used. 
ixgbevf_set_num_queues() sets adapter->num_rx_queues ignoring the above 
value if num_tcs less or equal to 1:

/* fetch queue configuration from the PF */
	err = ixgbevf_get_queues(hw, &num_tcs, &def_q);

	spin_unlock_bh(&adapter->mbx_lock);

	if (err)
		return;

	/* we need as many queues as traffic classes */
	if (num_tcs > 1) {
		adapter->num_rx_queues = num_tcs;
	} else {
		u16 rss = min_t(u16, num_online_cpus(), IXGBEVF_MAX_RSS_QUEUES);

		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:
			break;
		}
	}

This means that if PF returned in IXGBE_VF_RX_QUEUES 1 and if u have 
more than 1 CPU u will still go and configure 2 Rx queues for a VF. This 
unless I miss something again... ;)
>
> BTW - there are other issues with your patches. The indirection table seems to come out as all 0s and the VF driver reports link down/up when querying it.

Worked just fine to me on x540.
What is your setup? How did u check it? Did u remember to patch "ip" 
tool and enable the querying?

>
> Where is this information useful anyway - what is the use case? There is no description in your patches for why all this is needed.

I'm not sure it's required to explain why would I want to add a standard 
ethtool functionality to a driver.
However, since u've asked - we are developing a software infrastructure 
that needs the ability to query the VF's indirection table and RSS hash 
key from inside the Guest OS (particularly on AWS) in order to be able 
to open the socket the way that its traffic would arrive to a specific CPU.

Once this patches are accepted here we may start asking Amazon to pull 
them too.

thanks,
vlad

>
> Thanks,
> Emil
>
>

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

[Tantilov, Emil S]

>-----Original Message-----
>From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com] 
>Sent: Tuesday, March 24, 2015 12:06 PM
> Subject: Re: [PATCH net-next v6 4/7] ixgbevf: Add a RETA query code
> I'm not sure where you see this, on my setup ixgbevf_get_queues() gets 4 in msg[IXGBE_VF_R/TX_QUEUES] which is used to set hw->mac.max_t/rx_queues.
>
>Right. I misread the __ALIGN_MASK() macro.
>But then I can't see where max_rx_queues is used. 

It is used when stopping the Tx/Rx queues in ixgbevf_stop_hw_vf().

>ixgbevf_set_num_queues() sets adapter->num_rx_queues ignoring the above 
>value if num_tcs less or equal to 1:
>
>/* fetch queue configuration from the PF */
>	err = ixgbevf_get_queues(hw, &num_tcs, &def_q);
>
>	spin_unlock_bh(&adapter->mbx_lock);
>
>	if (err)
>		return;
>
>	/* we need as many queues as traffic classes */
>	if (num_tcs > 1) {
>		adapter->num_rx_queues = num_tcs;
>	} else {
>		u16 rss = min_t(u16, num_online_cpus(), IXGBEVF_MAX_RSS_QUEUES);
>
>		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:
>			break;
>		}
>	}
>
>This means that if PF returned in IXGBE_VF_RX_QUEUES 1 and if u have 
>more than 1 CPU u will still go and configure 2 Rx queues for a VF. This 
>unless I miss something again... ;)

>From what I can see vmdq->mask can be only for 4 or 8 queues, so the PF will not return 1, unless you meant something else.

The comment about the 1 queue is outdated though - I think it's leftover from the time the VF only allowed single queue.

>> BTW - there are other issues with your patches. The indirection table seems to come out as all 0s and the VF driver reports link >> down/up when querying it.

>Worked just fine to me on x540.
>What is your setup? How did u check it? Did u remember to patch "ip" 
>tool and enable the querying?

I have x540 as well and used the modified ip, otherwise the operation won't be allowed. I will do some more debugging when I get a chance and will get back to you.

>>
>> Where is this information useful anyway - what is the use case? There is no description in your patches for why all this is >>needed.
>
>I'm not sure it's required to explain why would I want to add a standard 
>ethtool functionality to a driver.
>However, since u've asked - we are developing a software infrastructure 
>that needs the ability to query the VF's indirection table and RSS hash 
>key from inside the Guest OS (particularly on AWS) in order to be able 
>to open the socket the way that its traffic would arrive to a specific CPU.

Understanding the use case can help with the implementation. Because you need to get the RSS info from the PF for macs < x550 this opens up the possibility to abuse (even if inadvertently) the mailbox if someone decided to query that info in a loop - like we have seen this happen with net-snmp.

Have you tested what happens if you run:

while true
do
	ethtool --show-rxfh-indir ethX
done

in the background while passing traffic through the VF?

Perhaps storing the RSS key and the table is better option than having to invoke the mailbox on every read.

Thanks,
Emil

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

[Vlad Zolotarov]

On 03/24/15 23:04, Tantilov, Emil S wrote:
>> -----Original Message-----
>> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
>> Sent: Tuesday, March 24, 2015 12:06 PM
>> Subject: Re: [PATCH net-next v6 4/7] ixgbevf: Add a RETA query code
>> I'm not sure where you see this, on my setup ixgbevf_get_queues() gets 4 in msg[IXGBE_VF_R/TX_QUEUES] which is used to set hw->mac.max_t/rx_queues.
>>
>> Right. I misread the __ALIGN_MASK() macro.
>> But then I can't see where max_rx_queues is used.
> It is used when stopping the Tx/Rx queues in ixgbevf_stop_hw_vf().

Of course, I meant it's not used during the adapter->num_rx_queues value 
calculation. Let's move this discussion to the appropriate patch thread.

>
>> ixgbevf_set_num_queues() sets adapter->num_rx_queues ignoring the above
>> value if num_tcs less or equal to 1:
>>
>> /* fetch queue configuration from the PF */
>> 	err = ixgbevf_get_queues(hw, &num_tcs, &def_q);
>>
>> 	spin_unlock_bh(&adapter->mbx_lock);
>>
>> 	if (err)
>> 		return;
>>
>> 	/* we need as many queues as traffic classes */
>> 	if (num_tcs > 1) {
>> 		adapter->num_rx_queues = num_tcs;
>> 	} else {
>> 		u16 rss = min_t(u16, num_online_cpus(), IXGBEVF_MAX_RSS_QUEUES);

>>
>> 		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:
>> 			break;
>> 		}
>> 	}
>>
>> This means that if PF returned in IXGBE_VF_RX_QUEUES 1 and if u have
>> more than 1 CPU u will still go and configure 2 Rx queues for a VF. This
>> unless I miss something again... ;)
>  From what I can see vmdq->mask can be only for 4 or 8 queues, so the PF will not return 1, unless you meant something else.
This is only if PF is driven by a Linux ixgbe PF driver as it is in the 
upstream tree right now. AFAIK VF driver should be completely decoupled 
from the PF driver and all the configuration decisions should be made 
based on the VF-PF communication via VF-PF channel (mailbox).
Pls., see my comments on the thread of your patches that have added 
these lines.

>
> The comment about the 1 queue is outdated though - I think it's leftover from the time the VF only allowed single queue.

Looks like it. ;)

>
>>> BTW - there are other issues with your patches. The indirection table seems to come out as all 0s and the VF driver reports link >> down/up when querying it.
>> Worked just fine to me on x540.
>> What is your setup? How did u check it? Did u remember to patch "ip"
>> tool and enable the querying?
> I have x540 as well and used the modified ip, otherwise the operation won't be allowed. I will do some more debugging when I get a chance and will get back to you.

Pls., make sure u use v7 patches.

One problem that there may still be is that I may have protected the 
mbox access by adapter->mbx_lock spinlock similarly to 
ixgbevf_set_num_queues() since I don't think ethtool ensures the atomicy 
of its requests in a context of a specific device and thus mbox could be 
trashed by ethtool operations running in parallel on different CPUs.


>
>>> Where is this information useful anyway - what is the use case? There is no description in your patches for why all this is >>needed.
>> I'm not sure it's required to explain why would I want to add a standard
>> ethtool functionality to a driver.
>> However, since u've asked - we are developing a software infrastructure
>> that needs the ability to query the VF's indirection table and RSS hash
>> key from inside the Guest OS (particularly on AWS) in order to be able
>> to open the socket the way that its traffic would arrive to a specific CPU.
> Understanding the use case can help with the implementation. Because you need to get the RSS info from the PF for macs < x550 this opens up the possibility to abuse (even if inadvertently) the mailbox if someone decided to query that info in a loop - like we have seen this happen with net-snmp.
>
> Have you tested what happens if you run:
>
> while true
> do
> 	ethtool --show-rxfh-indir ethX
> done
>
> in the background while passing traffic through the VF?
I understand your concerns but let's start with clarifying a few things. 
First, VF driver is by definition not trusted. If it (or its user) 
decides to do anything malicious (like u proposed above) that would 
eventually hurt (only this) VF's performance - nobody should care. 
However the right question here would be: "How the above use case may 
hurt the corresponding PF or other VFs' performance?" And since the 
mailbox operation involves quite a few MMIO writes and reads this may 
slow the PF quite a bit and this may be a problem that should be taken 
care of. However it wasn't my patch series that have introduced it. The 
same problem would arise if Guest would change VF's MAC address in a 
tight loop like above. Namely any VF slow path operation that would 
eventually cause the VF-PF channel transaction may be used to create an 
attack on a PF.

This problem naturally may not be resolved on a VF level but only on a 
PF level since VF is not a trusted component. One option could be to 
allow only a specific number of mbox operations from a specific VF in a 
specific time period: e.g. at most 1 operation every jiffy.

I don't see any code handling any protection of this sort in a PF driver 
at the moment. However I maybe missing some HW configuration that limits 
the slow path interrupts rate thus limiting the number of mbox requests 
rate...

>
> Perhaps storing the RSS key and the table is better option than having to invoke the mailbox on every read.

I don't think this could work if I understand your proposal correctly. 
The only way to cache the result that would decrease the number of mbox 
transactions would be to cache it in the VF. But how could i invalidate 
this cache if the table content has been changed by a PF? I think the 
main source of a confusion here is that u assume that PF driver is a 
Linux ixgbe driver that doesn't support an indirection table change at 
the moment. As I have explained above - this should not be assumed.

thanks,
vlad

>
> Thanks,
> Emil
>
>

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

[Tantilov, Emil S]

>-----Original Message-----
>From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com] 
>Sent: Wednesday, March 25, 2015 2:28 AM
>Subject: Re: [PATCH net-next v6 4/7] ixgbevf: Add a RETA query code

<snip>

>> Have you tested what happens if you run:
>>
>> while true
>> do
>> 	ethtool --show-rxfh-indir ethX
>> done
>>
>> in the background while passing traffic through the VF?
>I understand your concerns but let's start with clarifying a few things. 
>First, VF driver is by definition not trusted. If it (or its user) 
>decides to do anything malicious (like u proposed above) that would 
>eventually hurt (only this) VF's performance - nobody should care. 
>However the right question here would be: "How the above use case may 
>hurt the corresponding PF or other VFs' performance?" And since the 
>mailbox operation involves quite a few MMIO writes and reads this may 
>slow the PF quite a bit and this may be a problem that should be taken 
>care of. However it wasn't my patch series that have introduced it. The 
>same problem would arise if Guest would change VF's MAC address in a 
>tight loop like above. Namely any VF slow path operation that would 
>eventually cause the VF-PF channel transaction may be used to create an 
>attack on a PF.

There are operations that can be disruptive to the VF I am not arguing that,
the issue introduced by these patches has mostly to do with the fact that now
we can hit the mailbox more often for what is mostly static information.

Especially with ethtool we already had to deal with an issue caused by net-snmp:
https://sourceforge.net/p/e1000/mailman/message/32188362/

Where net-snmp was being too aggressive when collecting information, even if most of it was static.

>
> Perhaps storing the RSS key and the table is better option than having to invoke the mailbox on every read.

>I don't think this could work if I understand your proposal correctly. 
>The only way to cache the result that would decrease the number of mbox 
>transactions would be to cache it in the VF. But how could i invalidate 
>this cache if the table content has been changed by a PF? I think the 
>main source of a confusion here is that u assume that PF driver is a 
>Linux ixgbe driver that doesn't support an indirection table change at 
>the moment. As I have explained above - this should not be assumed.

You keep mentioning other drivers - what other driver do you mean?
All the PF drivers that enable SRIOV are maintained and supported by Intel.

For HW older than X550 we can simply not allow the RSS hash to be modified if the driver is loaded in SRIOV mode.
This way the RSS info can be read once the driver is loaded. For X550 this can all be done in the VF, so you can avoid calling the mailbox altogether.
I understand this is a bit limiting, but this is due to HW limitation anyway (VFs do not have their own RSS config).

Thanks,
Emil

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

[Vlad Zolotarov]

On 03/25/15 20:35, Tantilov, Emil S wrote:
>> -----Original Message-----
>> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
>> Sent: Wednesday, March 25, 2015 2:28 AM
>> Subject: Re: [PATCH net-next v6 4/7] ixgbevf: Add a RETA query code
> <snip>
>
>>> Have you tested what happens if you run:
>>>
>>> while true
>>> do
>>> 	ethtool --show-rxfh-indir ethX
>>> done
>>>
>>> in the background while passing traffic through the VF?
>> I understand your concerns but let's start with clarifying a few things.
>> First, VF driver is by definition not trusted. If it (or its user)
>> decides to do anything malicious (like u proposed above) that would
>> eventually hurt (only this) VF's performance - nobody should care.
>> However the right question here would be: "How the above use case may
>> hurt the corresponding PF or other VFs' performance?" And since the
>> mailbox operation involves quite a few MMIO writes and reads this may
>> slow the PF quite a bit and this may be a problem that should be taken
>> care of. However it wasn't my patch series that have introduced it. The
>> same problem would arise if Guest would change VF's MAC address in a
>> tight loop like above. Namely any VF slow path operation that would
>> eventually cause the VF-PF channel transaction may be used to create an
>> attack on a PF.
> There are operations that can be disruptive to the VF I am not arguing that,
> the issue introduced by these patches has mostly to do with the fact that now
> we can hit the mailbox more often for what is mostly static information.
>
> Especially with ethtool we already had to deal with an issue caused by net-snmp:
> https://sourceforge.net/p/e1000/mailman/message/32188362/
>
> Where net-snmp was being too aggressive when collecting information, even if most of it was static.

Emil, I don't really understand what are u trying to protect here 
against. If a user would want to shoot him/herself in the leg - he/she 
would still be able to do it with the other mailbox involving operations 
like MAC change. So, what's the sense to add useless lines?

>
>> Perhaps storing the RSS key and the table is better option than having to invoke the mailbox on every read.
>> I don't think this could work if I understand your proposal correctly.
>> The only way to cache the result that would decrease the number of mbox
>> transactions would be to cache it in the VF. But how could i invalidate
>> this cache if the table content has been changed by a PF? I think the
>> main source of a confusion here is that u assume that PF driver is a
>> Linux ixgbe driver that doesn't support an indirection table change at
>> the moment. As I have explained above - this should not be assumed.
> You keep mentioning other drivers - what other driver do you mean?
> All the PF drivers that enable SRIOV are maintained and supported by Intel.
>
> For HW older than X550 we can simply not allow the RSS hash to be modified if the driver is loaded in SRIOV mode.
> This way the RSS info can be read once the driver is loaded. For X550 this can all be done in the VF, so you can avoid calling the mailbox altogether.
> I understand this is a bit limiting, but this is due to HW limitation anyway (VFs do not have their own RSS config).

Let me remind u that Linux, FreeBSD, XEN  and DPDK PF drivers are all 
open source so u can't actually go and "not allow" things. ;) And 
although Intel developers contribute most of the code there are and will 
be other contributors too so I doubt the proposed above approach fits 
the open source spirit well. ;)

The user should actually not query the indirection table and a hash key 
too often. And if he/she does - it should be his/her problem.
However, if like with the ixgbevf_set_num_queues() u insist on your way 
of doing this (on caching the indirection table and hash key) - then 
please let me know and I will add it. Because, frankly, I care about the 
PF part of this series much more than for the VF part... ;)

>
> Thanks,
> Emil
>
>

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

[Alexander Duyck]

On 03/25/2015 01:17 PM, Vlad Zolotarov wrote:
>
>
> On 03/25/15 20:35, Tantilov, Emil S wrote:
>>> -----Original Message-----
>>> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
>>> Sent: Wednesday, March 25, 2015 2:28 AM
>>> Subject: Re: [PATCH net-next v6 4/7] ixgbevf: Add a RETA query code
>> <snip>
>>
>>>> Have you tested what happens if you run:
>>>>
>>>> while true
>>>> do
>>>>     ethtool --show-rxfh-indir ethX
>>>> done
>>>>
>>>> in the background while passing traffic through the VF?
>>> I understand your concerns but let's start with clarifying a few 
>>> things.
>>> First, VF driver is by definition not trusted. If it (or its user)
>>> decides to do anything malicious (like u proposed above) that would
>>> eventually hurt (only this) VF's performance - nobody should care.
>>> However the right question here would be: "How the above use case may
>>> hurt the corresponding PF or other VFs' performance?" And since the
>>> mailbox operation involves quite a few MMIO writes and reads this may
>>> slow the PF quite a bit and this may be a problem that should be taken
>>> care of. However it wasn't my patch series that have introduced it. The
>>> same problem would arise if Guest would change VF's MAC address in a
>>> tight loop like above. Namely any VF slow path operation that would
>>> eventually cause the VF-PF channel transaction may be used to create an
>>> attack on a PF.
>> There are operations that can be disruptive to the VF I am not 
>> arguing that,
>> the issue introduced by these patches has mostly to do with the fact 
>> that now
>> we can hit the mailbox more often for what is mostly static information.
>>
>> Especially with ethtool we already had to deal with an issue caused 
>> by net-snmp:
>> https://sourceforge.net/p/e1000/mailman/message/32188362/
>>
>> Where net-snmp was being too aggressive when collecting information, 
>> even if most of it was static.
>
> Emil, I don't really understand what are u trying to protect here 
> against. If a user would want to shoot him/herself in the leg - he/she 
> would still be able to do it with the other mailbox involving 
> operations like MAC change. So, what's the sense to add useless lines?
>
>>
>>> Perhaps storing the RSS key and the table is better option than 
>>> having to invoke the mailbox on every read.
>>> I don't think this could work if I understand your proposal correctly.
>>> The only way to cache the result that would decrease the number of mbox
>>> transactions would be to cache it in the VF. But how could i invalidate
>>> this cache if the table content has been changed by a PF? I think the
>>> main source of a confusion here is that u assume that PF driver is a
>>> Linux ixgbe driver that doesn't support an indirection table change at
>>> the moment. As I have explained above - this should not be assumed.
>> You keep mentioning other drivers - what other driver do you mean?
>> All the PF drivers that enable SRIOV are maintained and supported by 
>> Intel.
>>
>> For HW older than X550 we can simply not allow the RSS hash to be 
>> modified if the driver is loaded in SRIOV mode.
>> This way the RSS info can be read once the driver is loaded. For X550 
>> this can all be done in the VF, so you can avoid calling the mailbox 
>> altogether.
>> I understand this is a bit limiting, but this is due to HW limitation 
>> anyway (VFs do not have their own RSS config).
>
> Let me remind u that Linux, FreeBSD, XEN  and DPDK PF drivers are all 
> open source so u can't actually go and "not allow" things. ;) And 
> although Intel developers contribute most of the code there are and 
> will be other contributors too so I doubt the proposed above approach 
> fits the open source spirit well. ;)

Actually these drivers already support multiple OSes just fine.  The 
part where I think you are confused is that you assume they all use the 
same Mailbox API which they likely wouldn't.  I would suggest taking a 
look at ixgbe_pfvf_api_rev in mbx.h of the VF driver. Different OSes 
have different things that can be supported, so for example the 
ixgbe_mbox_api_20 is reserved for a Solaris based PF/VF combination.  I 
would suspect that FreeBSD will likely have to conform to the existing 
APIs, or report that it only supports a different version of the mailbox 
API.

>
> The user should actually not query the indirection table and a hash 
> key too often. And if he/she does - it should be his/her problem.
> However, if like with the ixgbevf_set_num_queues() u insist on your 
> way of doing this (on caching the indirection table and hash key) - 
> then please let me know and I will add it. Because, frankly, I care 
> about the PF part of this series much more than for the VF part... ;)

I would say you don't need to cache it, but for 82599 and x540 there 
isn't any need to store more than 3 bits per entry, 384b, or 12 DWORDs 
for the entire RETA of the VF since the hardware can support at most 8 
queues w/ SR-IOV.  Then you only need one message instead of 3 which 
will reduce quite a bit of the complication with all of this.

Also it might make more sense to start working on displaying this on the 
PF before you start trying to do this on the VF.  As far as I know ixgbe 
still doesn't have this functionality and it would make much more sense 
to enable that first on ixgbe before you start trying to find a way to 
feed the data to the VF.

- Alex

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

[Tantilov, Emil S]

>-----Original Message-----
>From: Alexander Duyck [mailto:alexander.h.duyck@redhat.com] 
>Sent: Wednesday, March 25, 2015 2:05 PM
>Subject: Re: [PATCH net-next v6 4/7] ixgbevf: Add a RETA query code
>
>
>On 03/25/2015 01:17 PM, Vlad Zolotarov wrote:
>>
>>
>> On 03/25/15 20:35, Tantilov, Emil S wrote:
>>>> -----Original Message-----
>>>> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
>>>> Sent: Wednesday, March 25, 2015 2:28 AM
>>>> Subject: Re: [PATCH net-next v6 4/7] ixgbevf: Add a RETA query code
>>> <snip>
>>>
>>>>> Have you tested what happens if you run:
>>>>>
>>>>> while true
>>>>> do
>>>>>     ethtool --show-rxfh-indir ethX
>>>>> done
>>>>>
>>>>> in the background while passing traffic through the VF?
>>>> I understand your concerns but let's start with clarifying a few 
>>>> things.
>>>> First, VF driver is by definition not trusted. If it (or its user)
>>>> decides to do anything malicious (like u proposed above) that would
>>>> eventually hurt (only this) VF's performance - nobody should care.
>>>> However the right question here would be: "How the above use case may
>>>> hurt the corresponding PF or other VFs' performance?" And since the
>>>> mailbox operation involves quite a few MMIO writes and reads this may
>>>> slow the PF quite a bit and this may be a problem that should be taken
>>>> care of. However it wasn't my patch series that have introduced it. The
>>>> same problem would arise if Guest would change VF's MAC address in a
>>>> tight loop like above. Namely any VF slow path operation that would
>>>> eventually cause the VF-PF channel transaction may be used to create an
>>>> attack on a PF.
>>> There are operations that can be disruptive to the VF I am not 
>>> arguing that,
>>> the issue introduced by these patches has mostly to do with the fact 
>>> that now
>>> we can hit the mailbox more often for what is mostly static information.
>>>
>>> Especially with ethtool we already had to deal with an issue caused 
>>> by net-snmp:
>>> https://sourceforge.net/p/e1000/mailman/message/32188362/
>>>
>>> Where net-snmp was being too aggressive when collecting information, 
>>> even if most of it was static.
>>
>> Emil, I don't really understand what are u trying to protect here 
>> against. If a user would want to shoot him/herself in the leg - he/she 
>> would still be able to do it with the other mailbox involving 
>> operations like MAC change. So, what's the sense to add useless lines?
>>
>>>
>>>> Perhaps storing the RSS key and the table is better option than 
>>>> having to invoke the mailbox on every read.
>>>> I don't think this could work if I understand your proposal correctly.
>>>> The only way to cache the result that would decrease the number of mbox
>>>> transactions would be to cache it in the VF. But how could i invalidate
>>>> this cache if the table content has been changed by a PF? I think the
>>>> main source of a confusion here is that u assume that PF driver is a
>>>> Linux ixgbe driver that doesn't support an indirection table change at
>>>> the moment. As I have explained above - this should not be assumed.
>>> You keep mentioning other drivers - what other driver do you mean?
>>> All the PF drivers that enable SRIOV are maintained and supported by 
>>> Intel.
>>>
>>> For HW older than X550 we can simply not allow the RSS hash to be 
>>> modified if the driver is loaded in SRIOV mode.
>>> This way the RSS info can be read once the driver is loaded. For X550 
>>> this can all be done in the VF, so you can avoid calling the mailbox 
>>> altogether.
>>> I understand this is a bit limiting, but this is due to HW limitation 
>>> anyway (VFs do not have their own RSS config).
>>
>> Let me remind u that Linux, FreeBSD, XEN  and DPDK PF drivers are all 
>> open source so u can't actually go and "not allow" things. ;) And 
>> although Intel developers contribute most of the code there are and 
>> will be other contributors too so I doubt the proposed above approach 
>> fits the open source spirit well. ;)
>
>Actually these drivers already support multiple OSes just fine.  The 
>part where I think you are confused is that you assume they all use the 
>same Mailbox API which they likely wouldn't.  I would suggest taking a 
>look at ixgbe_pfvf_api_rev in mbx.h of the VF driver. Different OSes 
>have different things that can be supported, so for example the 
>ixgbe_mbox_api_20 is reserved for a Solaris based PF/VF combination.  I 
>would suspect that FreeBSD will likely have to conform to the existing 
>APIs, or report that it only supports a different version of the mailbox 
>API.
>
>> The user should actually not query the indirection table and a hash 
>> key too often. And if he/she does - it should be his/her problem.
>> However, if like with the ixgbevf_set_num_queues() u insist on your 
>> way of doing this (on caching the indirection table and hash key) - 
>> then please let me know and I will add it. Because, frankly, I care 
>> about the PF part of this series much more than for the VF part... ;)
>
>I would say you don't need to cache it, but for 82599 and x540 there 
>isn't any need to store more than 3 bits per entry, 384b, or 12 DWORDs 
>for the entire RETA of the VF since the hardware can support at most 8 
>queues w/ SR-IOV.  Then you only need one message instead of 3 which 
>will reduce quite a bit of the complication with all of this.

This sounds like a good idea to me.

Thanks Alex,
Emil

>
>Also it might make more sense to start working on displaying this on the 
>PF before you start trying to do this on the VF.  As far as I know ixgbe 
>still doesn't have this functionality and it would make much more sense 
>to enable that first on ixgbe before you start trying to find a way to 
>feed the data to the VF.
>
>- Alex

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

[Vlad Zolotarov]

On 03/25/15 23:04, Alexander Duyck wrote:
>
> On 03/25/2015 01:17 PM, Vlad Zolotarov wrote:
>>
>>
>> On 03/25/15 20:35, Tantilov, Emil S wrote:
>>>> -----Original Message-----
>>>> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
>>>> Sent: Wednesday, March 25, 2015 2:28 AM
>>>> Subject: Re: [PATCH net-next v6 4/7] ixgbevf: Add a RETA query code
>>> <snip>
>>>
>>>>> Have you tested what happens if you run:
>>>>>
>>>>> while true
>>>>> do
>>>>>     ethtool --show-rxfh-indir ethX
>>>>> done
>>>>>
>>>>> in the background while passing traffic through the VF?
>>>> I understand your concerns but let's start with clarifying a few 
>>>> things.
>>>> First, VF driver is by definition not trusted. If it (or its user)
>>>> decides to do anything malicious (like u proposed above) that would
>>>> eventually hurt (only this) VF's performance - nobody should care.
>>>> However the right question here would be: "How the above use case may
>>>> hurt the corresponding PF or other VFs' performance?" And since the
>>>> mailbox operation involves quite a few MMIO writes and reads this may
>>>> slow the PF quite a bit and this may be a problem that should be taken
>>>> care of. However it wasn't my patch series that have introduced it. 
>>>> The
>>>> same problem would arise if Guest would change VF's MAC address in a
>>>> tight loop like above. Namely any VF slow path operation that would
>>>> eventually cause the VF-PF channel transaction may be used to 
>>>> create an
>>>> attack on a PF.
>>> There are operations that can be disruptive to the VF I am not 
>>> arguing that,
>>> the issue introduced by these patches has mostly to do with the fact 
>>> that now
>>> we can hit the mailbox more often for what is mostly static 
>>> information.
>>>
>>> Especially with ethtool we already had to deal with an issue caused 
>>> by net-snmp:
>>> https://sourceforge.net/p/e1000/mailman/message/32188362/
>>>
>>> Where net-snmp was being too aggressive when collecting information, 
>>> even if most of it was static.
>>
>> Emil, I don't really understand what are u trying to protect here 
>> against. If a user would want to shoot him/herself in the leg - 
>> he/she would still be able to do it with the other mailbox involving 
>> operations like MAC change. So, what's the sense to add useless lines?
>>
>>>
>>>> Perhaps storing the RSS key and the table is better option than 
>>>> having to invoke the mailbox on every read.
>>>> I don't think this could work if I understand your proposal correctly.
>>>> The only way to cache the result that would decrease the number of 
>>>> mbox
>>>> transactions would be to cache it in the VF. But how could i 
>>>> invalidate
>>>> this cache if the table content has been changed by a PF? I think the
>>>> main source of a confusion here is that u assume that PF driver is a
>>>> Linux ixgbe driver that doesn't support an indirection table change at
>>>> the moment. As I have explained above - this should not be assumed.
>>> You keep mentioning other drivers - what other driver do you mean?
>>> All the PF drivers that enable SRIOV are maintained and supported by 
>>> Intel.
>>>
>>> For HW older than X550 we can simply not allow the RSS hash to be 
>>> modified if the driver is loaded in SRIOV mode.
>>> This way the RSS info can be read once the driver is loaded. For 
>>> X550 this can all be done in the VF, so you can avoid calling the 
>>> mailbox altogether.
>>> I understand this is a bit limiting, but this is due to HW 
>>> limitation anyway (VFs do not have their own RSS config).
>>
>> Let me remind u that Linux, FreeBSD, XEN  and DPDK PF drivers are all 
>> open source so u can't actually go and "not allow" things. ;) And 
>> although Intel developers contribute most of the code there are and 
>> will be other contributors too so I doubt the proposed above approach 
>> fits the open source spirit well. ;)
>
> Actually these drivers already support multiple OSes just fine. The 
> part where I think you are confused is that you assume they all use 
> the same Mailbox API which they likely wouldn't.  I would suggest 
> taking a look at ixgbe_pfvf_api_rev in mbx.h of the VF driver. 
> Different OSes have different things that can be supported, so for 
> example the ixgbe_mbox_api_20 is reserved for a Solaris based PF/VF 
> combination.  I would suspect that FreeBSD will likely have to conform 
> to the existing APIs, or report that it only supports a different 
> version of the mailbox API.

I didn't assume a common API at all. My point was that u can't just go 
and "forbid" standard things like changing the indirection table in the 
open source project(s). Therefore u shouldn't assume it and thus caching 
the indirection table doesn't seem a future-proof solution to me.


>
>>
>> The user should actually not query the indirection table and a hash 
>> key too often. And if he/she does - it should be his/her problem.
>> However, if like with the ixgbevf_set_num_queues() u insist on your 
>> way of doing this (on caching the indirection table and hash key) - 
>> then please let me know and I will add it. Because, frankly, I care 
>> about the PF part of this series much more than for the VF part... ;)
>
> I would say you don't need to cache it, but for 82599 and x540 there 
> isn't any need to store more than 3 bits per entry, 384b, or 12 DWORDs 
> for the entire RETA of the VF since the hardware can support at most 8 
> queues w/ SR-IOV.  Then you only need one message instead of 3 which 
> will reduce quite a bit of the complication with all of this.
>
> Also it might make more sense to start working on displaying this on 
> the PF before you start trying to do this on the VF.  As far as I know 
> ixgbe still doesn't have this functionality and it would make much 
> more sense to enable that first on ixgbe before you start trying to 
> find a way to feed the data to the VF.

Let's agree on the next steps:

 1. I'll reduce the series scope to 82599 and x540 devices.
 2. I'll add the same ethtool operations I've already added to VF to PF
    devices as well.
 3. I'll implement the compression the Alex so desperately wants... ;)
 4. I won't implement the caching of the indirection and RSS hash key
    query results in the VF level.


Pls., confirm that all u guys (Alex, Emil, Jeff) agree to that.
Thanks,
vlad



>
> - Alex

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

[Alexander Duyck]

On 03/26/2015 02:35 AM, Vlad Zolotarov wrote:
>
>
> On 03/25/15 23:04, Alexander Duyck wrote:
>>
>> On 03/25/2015 01:17 PM, Vlad Zolotarov wrote:
>>>
>>>
>>> On 03/25/15 20:35, Tantilov, Emil S wrote:
>>>>> -----Original Message-----
>>>>> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
>>>>> Sent: Wednesday, March 25, 2015 2:28 AM
>>>>> Subject: Re: [PATCH net-next v6 4/7] ixgbevf: Add a RETA query code
>>>>
>>>
<snip>
>>>>
>>>>> Perhaps storing the RSS key and the table is better option than 
>>>>> having to invoke the mailbox on every read.
>>>>> I don't think this could work if I understand your proposal 
>>>>> correctly.
>>>>> The only way to cache the result that would decrease the number of 
>>>>> mbox
>>>>> transactions would be to cache it in the VF. But how could i 
>>>>> invalidate
>>>>> this cache if the table content has been changed by a PF? I think the
>>>>> main source of a confusion here is that u assume that PF driver is a
>>>>> Linux ixgbe driver that doesn't support an indirection table 
>>>>> change at
>>>>> the moment. As I have explained above - this should not be assumed.
>>>> You keep mentioning other drivers - what other driver do you mean?
>>>> All the PF drivers that enable SRIOV are maintained and supported 
>>>> by Intel.
>>>>
>>>> For HW older than X550 we can simply not allow the RSS hash to be 
>>>> modified if the driver is loaded in SRIOV mode.
>>>> This way the RSS info can be read once the driver is loaded. For 
>>>> X550 this can all be done in the VF, so you can avoid calling the 
>>>> mailbox altogether.
>>>> I understand this is a bit limiting, but this is due to HW 
>>>> limitation anyway (VFs do not have their own RSS config).
>>>
>>> Let me remind u that Linux, FreeBSD, XEN  and DPDK PF drivers are 
>>> all open source so u can't actually go and "not allow" things. ;) 
>>> And although Intel developers contribute most of the code there are 
>>> and will be other contributors too so I doubt the proposed above 
>>> approach fits the open source spirit well. ;)
>>
>> Actually these drivers already support multiple OSes just fine. The 
>> part where I think you are confused is that you assume they all use 
>> the same Mailbox API which they likely wouldn't.  I would suggest 
>> taking a look at ixgbe_pfvf_api_rev in mbx.h of the VF driver. 
>> Different OSes have different things that can be supported, so for 
>> example the ixgbe_mbox_api_20 is reserved for a Solaris based PF/VF 
>> combination.  I would suspect that FreeBSD will likely have to 
>> conform to the existing APIs, or report that it only supports a 
>> different version of the mailbox API.
>
> I didn't assume a common API at all. My point was that u can't just go 
> and "forbid" standard things like changing the indirection table in 
> the open source project(s). Therefore u shouldn't assume it and thus 
> caching the indirection table doesn't seem a future-proof solution to me.
>

The point was the current driver doesn't provide any such option. If you 
plan on adding it, then add it first and then we can talk about it.  
Otherwise it isn't a requirement for any current solution that you might 
implement.  That was the point I was trying to get across.  My advice is 
to avoid scoping in features that aren't there.  If you want to go ahead 
and add it then do so, but otherwise don't limit implementation by 
things that have not yet been added.

>
>>
>>>
>>> The user should actually not query the indirection table and a hash 
>>> key too often. And if he/she does - it should be his/her problem.
>>> However, if like with the ixgbevf_set_num_queues() u insist on your 
>>> way of doing this (on caching the indirection table and hash key) - 
>>> then please let me know and I will add it. Because, frankly, I care 
>>> about the PF part of this series much more than for the VF part... ;)
>>
>> I would say you don't need to cache it, but for 82599 and x540 there 
>> isn't any need to store more than 3 bits per entry, 384b, or 12 
>> DWORDs for the entire RETA of the VF since the hardware can support 
>> at most 8 queues w/ SR-IOV.  Then you only need one message instead 
>> of 3 which will reduce quite a bit of the complication with all of this.
>>
>> Also it might make more sense to start working on displaying this on 
>> the PF before you start trying to do this on the VF.  As far as I 
>> know ixgbe still doesn't have this functionality and it would make 
>> much more sense to enable that first on ixgbe before you start trying 
>> to find a way to feed the data to the VF.
>
> Let's agree on the next steps:
>
> 1. I'll reduce the series scope to 82599 and x540 devices.

For mailbox implementation that is okay, my advice for the x550 would be 
to just read the registers.  They should already be defined in the 
driver so it is just a matter of reversing the process for writing the 
RETA.  You should have a solid grasp of that once you implement the 
solution for the PF.

> 2. I'll add the same ethtool operations I've already added to VF to PF
>    devices as well.

Yes, I would recommend PF first, and then VF.  The PF allows for a much 
simpler approach, and you will likely have a better idea of the solution 
for X550.  For the most part you can probably just copy the code already 
in igb.

> 3. I'll implement the compression the Alex so desperately wants... ;)

I've done enough of these mailbox type setups on this hardware to 
realize that you don't want to be dealing with a multi-part message.  It 
will make things so much easier for you to have this processed as a 
single message.

> 4. I won't implement the caching of the indirection and RSS hash key
>    query results in the VF level.

The idea with caching is to keep the number of messages across the 
mailbox to a minimum.  Compressing it down to 3 bits per entry will help 
some, but I am not sure if that is enough to completely mitigate the 
need for caching.  For now though the caching isn't a must-have, and it 
won't be needed for x550 since it can access the table directly.  The 
main thing I would recommend keeping an eye on would be how long it will 
take to complete the messages necessary to get the ethtool info.  If 
there are any scenarios where things take more than 1 second then I 
would say we are taking too long and we would have to look into caching 
because we want to avoid holding the rtnl lock for any extended period 
of time.

My advice is get the PF patches in first, then we can look at trying to 
get feature parity on the VF.

- Alex

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

[Vlad Zolotarov]

On 03/26/15 16:40, Alexander Duyck wrote:
>
> On 03/26/2015 02:35 AM, Vlad Zolotarov wrote:
>>
>>
>> On 03/25/15 23:04, Alexander Duyck wrote:
>>>
>>> On 03/25/2015 01:17 PM, Vlad Zolotarov wrote:
>>>>
>>>>
>>>> On 03/25/15 20:35, Tantilov, Emil S wrote:
>>>>>> -----Original Message-----
>>>>>> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
>>>>>> Sent: Wednesday, March 25, 2015 2:28 AM
>>>>>> Subject: Re: [PATCH net-next v6 4/7] ixgbevf: Add a RETA query code
>>>>>
>>>>
> <snip>
>>>>>
>>>>>> Perhaps storing the RSS key and the table is better option than 
>>>>>> having to invoke the mailbox on every read.
>>>>>> I don't think this could work if I understand your proposal 
>>>>>> correctly.
>>>>>> The only way to cache the result that would decrease the number 
>>>>>> of mbox
>>>>>> transactions would be to cache it in the VF. But how could i 
>>>>>> invalidate
>>>>>> this cache if the table content has been changed by a PF? I think 
>>>>>> the
>>>>>> main source of a confusion here is that u assume that PF driver is a
>>>>>> Linux ixgbe driver that doesn't support an indirection table 
>>>>>> change at
>>>>>> the moment. As I have explained above - this should not be assumed.
>>>>> You keep mentioning other drivers - what other driver do you mean?
>>>>> All the PF drivers that enable SRIOV are maintained and supported 
>>>>> by Intel.
>>>>>
>>>>> For HW older than X550 we can simply not allow the RSS hash to be 
>>>>> modified if the driver is loaded in SRIOV mode.
>>>>> This way the RSS info can be read once the driver is loaded. For 
>>>>> X550 this can all be done in the VF, so you can avoid calling the 
>>>>> mailbox altogether.
>>>>> I understand this is a bit limiting, but this is due to HW 
>>>>> limitation anyway (VFs do not have their own RSS config).
>>>>
>>>> Let me remind u that Linux, FreeBSD, XEN  and DPDK PF drivers are 
>>>> all open source so u can't actually go and "not allow" things. ;) 
>>>> And although Intel developers contribute most of the code there are 
>>>> and will be other contributors too so I doubt the proposed above 
>>>> approach fits the open source spirit well. ;)
>>>
>>> Actually these drivers already support multiple OSes just fine. The 
>>> part where I think you are confused is that you assume they all use 
>>> the same Mailbox API which they likely wouldn't.  I would suggest 
>>> taking a look at ixgbe_pfvf_api_rev in mbx.h of the VF driver. 
>>> Different OSes have different things that can be supported, so for 
>>> example the ixgbe_mbox_api_20 is reserved for a Solaris based PF/VF 
>>> combination.  I would suspect that FreeBSD will likely have to 
>>> conform to the existing APIs, or report that it only supports a 
>>> different version of the mailbox API.
>>
>> I didn't assume a common API at all. My point was that u can't just 
>> go and "forbid" standard things like changing the indirection table 
>> in the open source project(s). Therefore u shouldn't assume it and 
>> thus caching the indirection table doesn't seem a future-proof 
>> solution to me.
>>
>
> The point was the current driver doesn't provide any such option. If 
> you plan on adding it, then add it first and then we can talk about 
> it.  Otherwise it isn't a requirement for any current solution that 
> you might implement.  That was the point I was trying to get across.  
> My advice is to avoid scoping in features that aren't there.  If you 
> want to go ahead and add it then do so, but otherwise don't limit 
> implementation by things that have not yet been added.

Let me remind that the above is said in the context of caching. The 
indirection table modification is an important tool for load balancing. 
The fact that ixgbe hasn't implemented it yet doesn't seem justified by 
anything but by the fact that u just haven't got to it yet. If u won't 
do it sometimes soon - somebody else will. That's why IMHO caching in VF 
is not the most wise direction since I'm not aware of a tool how PF can 
indicate to VF (driver) to invalidate it. All this in the context of 
82599 and x540 devices of course.


>
>>
>>>
>>>>
>>>> The user should actually not query the indirection table and a hash 
>>>> key too often. And if he/she does - it should be his/her problem.
>>>> However, if like with the ixgbevf_set_num_queues() u insist on your 
>>>> way of doing this (on caching the indirection table and hash key) - 
>>>> then please let me know and I will add it. Because, frankly, I care 
>>>> about the PF part of this series much more than for the VF part... ;)
>>>
>>> I would say you don't need to cache it, but for 82599 and x540 there 
>>> isn't any need to store more than 3 bits per entry, 384b, or 12 
>>> DWORDs for the entire RETA of the VF since the hardware can support 
>>> at most 8 queues w/ SR-IOV.  Then you only need one message instead 
>>> of 3 which will reduce quite a bit of the complication with all of 
>>> this.
>>>
>>> Also it might make more sense to start working on displaying this on 
>>> the PF before you start trying to do this on the VF. As far as I 
>>> know ixgbe still doesn't have this functionality and it would make 
>>> much more sense to enable that first on ixgbe before you start 
>>> trying to find a way to feed the data to the VF.
>>
>> Let's agree on the next steps:
>>
>> 1. I'll reduce the series scope to 82599 and x540 devices.
>
> For mailbox implementation that is okay, my advice for the x550 would 
> be to just read the registers.  They should already be defined in the 
> driver so it is just a matter of reversing the process for writing the 
> RETA.  You should have a solid grasp of that once you implement the 
> solution for the PF.

The problem is that I have no means to validate the solution - I have 
only x540 devices (nor specs). I'd prefer not to work in darkness and 
let u, guys, do it...

>
>> 2. I'll add the same ethtool operations I've already added to VF to PF
>>    devices as well.
>
> Yes, I would recommend PF first, and then VF.  The PF allows for a 
> much simpler approach, and you will likely have a better idea of the 
> solution for X550.  For the most part you can probably just copy the 
> code already in igb.



>
>> 3. I'll implement the compression the Alex so desperately wants... ;)
>
> I've done enough of these mailbox type setups on this hardware to 
> realize that you don't want to be dealing with a multi-part message.  
> It will make things so much easier for you to have this processed as a 
> single message.
>
>> 4. I won't implement the caching of the indirection and RSS hash key
>>    query results in the VF level.
>
> The idea with caching is to keep the number of messages across the 
> mailbox to a minimum.  Compressing it down to 3 bits per entry will 
> help some, but I am not sure if that is enough to completely mitigate 
> the need for caching.  For now though the caching isn't a must-have, 
> and it won't be needed for x550 since it can access the table 
> directly.  The main thing I would recommend keeping an eye on would be 
> how long it will take to complete the messages necessary to get the 
> ethtool info.  If there are any scenarios where things take more than 
> 1 second then I would say we are taking too long and we would have to 
> look into caching because we want to avoid holding the rtnl lock for 
> any extended period of time.

I've described the limitations the caching approach imposes above. I 
suggest we follow your approach here - solve the problems when they 
arrive. ;) I haven't succeeded to make it take as long as even near one 
second.

>
> My advice is get the PF patches in first, then we can look at trying 
> to get feature parity on the VF.

"feature parity on the VF"?

I'll send the patches later today. I'll incorporate the new PF code into 
the existing series.

>
> - Alex

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

[Alexander Duyck]

On 03/26/2015 08:10 AM, Vlad Zolotarov wrote:
>
>
> On 03/26/15 16:40, Alexander Duyck wrote:
>>
>> On 03/26/2015 02:35 AM, Vlad Zolotarov wrote:
>>>
>>>
>>> On 03/25/15 23:04, Alexander Duyck wrote:
>>>>
>>>> On 03/25/2015 01:17 PM, Vlad Zolotarov wrote:
>>>>>
>>>>>
>>>>> On 03/25/15 20:35, Tantilov, Emil S wrote:
>>>>>>> -----Original Message-----
>>>>>>> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
>>>>>>> Sent: Wednesday, March 25, 2015 2:28 AM
>>>>>>> Subject: Re: [PATCH net-next v6 4/7] ixgbevf: Add a RETA query code
>>>>>>
>>>>>
>>

<snip>

>>
>>>
>>>>
>>>>>
>>>>> The user should actually not query the indirection table and a 
>>>>> hash key too often. And if he/she does - it should be his/her 
>>>>> problem.
>>>>> However, if like with the ixgbevf_set_num_queues() u insist on 
>>>>> your way of doing this (on caching the indirection table and hash 
>>>>> key) - then please let me know and I will add it. Because, 
>>>>> frankly, I care about the PF part of this series much more than 
>>>>> for the VF part... ;)
>>>>
>>>> I would say you don't need to cache it, but for 82599 and x540 
>>>> there isn't any need to store more than 3 bits per entry, 384b, or 
>>>> 12 DWORDs for the entire RETA of the VF since the hardware can 
>>>> support at most 8 queues w/ SR-IOV. Then you only need one message 
>>>> instead of 3 which will reduce quite a bit of the complication with 
>>>> all of this.
>>>>
>>>> Also it might make more sense to start working on displaying this 
>>>> on the PF before you start trying to do this on the VF. As far as I 
>>>> know ixgbe still doesn't have this functionality and it would make 
>>>> much more sense to enable that first on ixgbe before you start 
>>>> trying to find a way to feed the data to the VF.
>>>
>>> Let's agree on the next steps:
>>>
>>> 1. I'll reduce the series scope to 82599 and x540 devices.
>>
>> For mailbox implementation that is okay, my advice for the x550 would 
>> be to just read the registers.  They should already be defined in the 
>> driver so it is just a matter of reversing the process for writing 
>> the RETA.  You should have a solid grasp of that once you implement 
>> the solution for the PF.
>
> The problem is that I have no means to validate the solution - I have 
> only x540 devices (nor specs). I'd prefer not to work in darkness and 
> let u, guys, do it...

If it helps the specs for the x540 can be found at 
http://www.intel.com/content/www/us/en/network-adapters/10-gigabit-network-adapters/ethernet-x540-datasheet.html. 
Section 8 has a full list of the register definitions.

All the code you need should already be stored in ixgbe_setup_reta and 
ixgbe_setup_vfreta.  If you do it right you shouldn't even need to read 
the hardware.  Odds are you could generate it in software and that is 
what you would be accessing on the PF.  After all, if at some point you 
reset the hardware you would need to restore it from software wouldn't 
you, and since the table is static for now you should be able to 
calculate the value without need of reading a register.  If at some 
point in the future you can change the table then it means a copy has to 
be maintained in kernel memory and at that point you could use that for 
the data you would be sending to the VF.

>>
>>> 4. I won't implement the caching of the indirection and RSS hash key
>>>    query results in the VF level.
>>
>> The idea with caching is to keep the number of messages across the 
>> mailbox to a minimum.  Compressing it down to 3 bits per entry will 
>> help some, but I am not sure if that is enough to completely mitigate 
>> the need for caching.  For now though the caching isn't a must-have, 
>> and it won't be needed for x550 since it can access the table 
>> directly.  The main thing I would recommend keeping an eye on would 
>> be how long it will take to complete the messages necessary to get 
>> the ethtool info.  If there are any scenarios where things take more 
>> than 1 second then I would say we are taking too long and we would 
>> have to look into caching because we want to avoid holding the rtnl 
>> lock for any extended period of time.
>
> I've described the limitations the caching approach imposes above. I 
> suggest we follow your approach here - solve the problems when they 
> arrive. ;) I haven't succeeded to make it take as long as even near 
> one second.
>
>>
>> My advice is get the PF patches in first, then we can look at trying 
>> to get feature parity on the VF.
>
> "feature parity on the VF"?

Get "ethtool -x/X" working on the PF, and then get it working on the 
VF.  That way it makes for very simple verification as you can inspect 
that the PF and VF tables are consistent.  As I said if nothing else you 
can probably take a look at the igb driver to see how this was done on 
the 1Gbps Intel hardware.

>
> I'll send the patches later today. I'll incorporate the new PF code 
> into the existing series.

You might want to break things out into a couple series of patches. Do 
the PF driver first just to make sure you have an understanding of what 
you expect, think of it as a proof of concept if nothing else for how 
you want the VF to function, then submit the VF series as a separate 
patch set and include the mailbox API changes.

- Alex

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

[Vlad Zolotarov]

On 03/26/15 17:59, Alexander Duyck wrote:
>
> On 03/26/2015 08:10 AM, Vlad Zolotarov wrote:
>>
>>
>> On 03/26/15 16:40, Alexander Duyck wrote:
>>>
>>> On 03/26/2015 02:35 AM, Vlad Zolotarov wrote:
>>>>
>>>>
>>>> On 03/25/15 23:04, Alexander Duyck wrote:
>>>>>
>>>>> On 03/25/2015 01:17 PM, Vlad Zolotarov wrote:
>>>>>>
>>>>>>
>>>>>> On 03/25/15 20:35, Tantilov, Emil S wrote:
>>>>>>>> -----Original Message-----
>>>>>>>> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
>>>>>>>> Sent: Wednesday, March 25, 2015 2:28 AM
>>>>>>>> Subject: Re: [PATCH net-next v6 4/7] ixgbevf: Add a RETA query 
>>>>>>>> code
>>>>>>>
>>>>>>
>>>
>
> <snip>
>
>>>
>>>>
>>>>>
>>>>>>
>>>>>> The user should actually not query the indirection table and a 
>>>>>> hash key too often. And if he/she does - it should be his/her 
>>>>>> problem.
>>>>>> However, if like with the ixgbevf_set_num_queues() u insist on 
>>>>>> your way of doing this (on caching the indirection table and hash 
>>>>>> key) - then please let me know and I will add it. Because, 
>>>>>> frankly, I care about the PF part of this series much more than 
>>>>>> for the VF part... ;)
>>>>>
>>>>> I would say you don't need to cache it, but for 82599 and x540 
>>>>> there isn't any need to store more than 3 bits per entry, 384b, or 
>>>>> 12 DWORDs for the entire RETA of the VF since the hardware can 
>>>>> support at most 8 queues w/ SR-IOV. Then you only need one message 
>>>>> instead of 3 which will reduce quite a bit of the complication 
>>>>> with all of this.
>>>>>
>>>>> Also it might make more sense to start working on displaying this 
>>>>> on the PF before you start trying to do this on the VF. As far as 
>>>>> I know ixgbe still doesn't have this functionality and it would 
>>>>> make much more sense to enable that first on ixgbe before you 
>>>>> start trying to find a way to feed the data to the VF.
>>>>
>>>> Let's agree on the next steps:
>>>>
>>>> 1. I'll reduce the series scope to 82599 and x540 devices.
>>>
>>> For mailbox implementation that is okay, my advice for the x550 
>>> would be to just read the registers.  They should already be defined 
>>> in the driver so it is just a matter of reversing the process for 
>>> writing the RETA.  You should have a solid grasp of that once you 
>>> implement the solution for the PF.
>>
>> The problem is that I have no means to validate the solution - I have 
>> only x540 devices (nor specs). I'd prefer not to work in darkness and 
>> let u, guys, do it...
>
> If it helps the specs for the x540 can be found at 
> http://www.intel.com/content/www/us/en/network-adapters/10-gigabit-network-adapters/ethernet-x540-datasheet.html. 
> Section 8 has a full list of the register definitions.

I meant I don't have x550 specs - I'm working against x540 spec quite 
intensively... ;)

>
> All the code you need should already be stored in ixgbe_setup_reta and 
> ixgbe_setup_vfreta.  If you do it right you shouldn't even need to 
> read the hardware.  Odds are you could generate it in software and 
> that is what you would be accessing on the PF.  After all, if at some 
> point you reset the hardware you would need to restore it from 
> software wouldn't you, and since the table is static for now you 
> should be able to calculate the value without need of reading a 
> register.  If at some point in the future you can change the table 
> then it means a copy has to be maintained in kernel memory and at that 
> point you could use that for the data you would be sending to the VF.
>
>>>
>>>> 4. I won't implement the caching of the indirection and RSS hash key
>>>>    query results in the VF level.
>>>
>>> The idea with caching is to keep the number of messages across the 
>>> mailbox to a minimum.  Compressing it down to 3 bits per entry will 
>>> help some, but I am not sure if that is enough to completely 
>>> mitigate the need for caching.  For now though the caching isn't a 
>>> must-have, and it won't be needed for x550 since it can access the 
>>> table directly.  The main thing I would recommend keeping an eye on 
>>> would be how long it will take to complete the messages necessary to 
>>> get the ethtool info.  If there are any scenarios where things take 
>>> more than 1 second then I would say we are taking too long and we 
>>> would have to look into caching because we want to avoid holding the 
>>> rtnl lock for any extended period of time.
>>
>> I've described the limitations the caching approach imposes above. I 
>> suggest we follow your approach here - solve the problems when they 
>> arrive. ;) I haven't succeeded to make it take as long as even near 
>> one second.
>>
>>>
>>> My advice is get the PF patches in first, then we can look at trying 
>>> to get feature parity on the VF.
>>
>> "feature parity on the VF"?
>
> Get "ethtool -x/X" working on the PF, and then get it working on the 
> VF.  That way it makes for very simple verification as you can inspect 
> that the PF and VF tables are consistent.  As I said if nothing else 
> you can probably take a look at the igb driver to see how this was 
> done on the 1Gbps Intel hardware.
>
>>
>> I'll send the patches later today. I'll incorporate the new PF code 
>> into the existing series.
>
> You might want to break things out into a couple series of patches. Do 
> the PF driver first just to make sure you have an understanding of 
> what you expect, think of it as a proof of concept if nothing else for 
> how you want the VF to function, then submit the VF series as a 
> separate patch set and include the mailbox API changes.
>
> - Alex
>

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

[Vlad Zolotarov]

On 03/26/15 17:59, Alexander Duyck wrote:
>
> On 03/26/2015 08:10 AM, Vlad Zolotarov wrote:
>>
>>
>> On 03/26/15 16:40, Alexander Duyck wrote:
>>>
>>> On 03/26/2015 02:35 AM, Vlad Zolotarov wrote:
>>>>
>>>>
>>>> On 03/25/15 23:04, Alexander Duyck wrote:
>>>>>
>>>>> On 03/25/2015 01:17 PM, Vlad Zolotarov wrote:
>>>>>>
>>>>>>
>>>>>> On 03/25/15 20:35, Tantilov, Emil S wrote:
>>>>>>>> -----Original Message-----
>>>>>>>> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
>>>>>>>> Sent: Wednesday, March 25, 2015 2:28 AM
>>>>>>>> Subject: Re: [PATCH net-next v6 4/7] ixgbevf: Add a RETA query 
>>>>>>>> code
>>>>>>>
>>>>>>
>>>
>
> <snip>
>
>>>
>>>>
>>>>>
>>>>>>
>>>>>> The user should actually not query the indirection table and a 
>>>>>> hash key too often. And if he/she does - it should be his/her 
>>>>>> problem.
>>>>>> However, if like with the ixgbevf_set_num_queues() u insist on 
>>>>>> your way of doing this (on caching the indirection table and hash 
>>>>>> key) - then please let me know and I will add it. Because, 
>>>>>> frankly, I care about the PF part of this series much more than 
>>>>>> for the VF part... ;)
>>>>>
>>>>> I would say you don't need to cache it, but for 82599 and x540 
>>>>> there isn't any need to store more than 3 bits per entry, 384b, or 
>>>>> 12 DWORDs for the entire RETA of the VF since the hardware can 
>>>>> support at most 8 queues w/ SR-IOV. Then you only need one message 
>>>>> instead of 3 which will reduce quite a bit of the complication 
>>>>> with all of this.
>>>>>
>>>>> Also it might make more sense to start working on displaying this 
>>>>> on the PF before you start trying to do this on the VF. As far as 
>>>>> I know ixgbe still doesn't have this functionality and it would 
>>>>> make much more sense to enable that first on ixgbe before you 
>>>>> start trying to find a way to feed the data to the VF.
>>>>
>>>> Let's agree on the next steps:
>>>>
>>>> 1. I'll reduce the series scope to 82599 and x540 devices.
>>>
>>> For mailbox implementation that is okay, my advice for the x550 
>>> would be to just read the registers.  They should already be defined 
>>> in the driver so it is just a matter of reversing the process for 
>>> writing the RETA.  You should have a solid grasp of that once you 
>>> implement the solution for the PF.
>>
>> The problem is that I have no means to validate the solution - I have 
>> only x540 devices (nor specs). I'd prefer not to work in darkness and 
>> let u, guys, do it...
>
> If it helps the specs for the x540 can be found at 
> http://www.intel.com/content/www/us/en/network-adapters/10-gigabit-network-adapters/ethernet-x540-datasheet.html. 
> Section 8 has a full list of the register definitions.
>
> All the code you need should already be stored in ixgbe_setup_reta and 
> ixgbe_setup_vfreta.  If you do it right you shouldn't even need to 
> read the hardware.  Odds are you could generate it in software and 
> that is what you would be accessing on the PF. 

I have no problems implementing the PF x550 part. I would need the 
proper spec to implement the VF part though. And since I can't have it 
now I'd rather not to it at the moment... ;)


Sure. This is what I have already implemented. The same is for RSS hash 
key.

> After all, if at some point you reset the hardware you would need to 
> restore it from software wouldn't you, and since the table is static 
> for now you should be able to calculate the value without need of 
> reading a register.  If at some point in the future you can change the 
> table then it means a copy has to be maintained in kernel memory and 
> at that point you could use that for the data you would be sending to 
> the VF.

Right.

>
>>>
>>>> 4. I won't implement the caching of the indirection and RSS hash key
>>>>    query results in the VF level.
>>>
>>> The idea with caching is to keep the number of messages across the 
>>> mailbox to a minimum.  Compressing it down to 3 bits per entry will 
>>> help some, but I am not sure if that is enough to completely 
>>> mitigate the need for caching.  For now though the caching isn't a 
>>> must-have, and it won't be needed for x550 since it can access the 
>>> table directly.  The main thing I would recommend keeping an eye on 
>>> would be how long it will take to complete the messages necessary to 
>>> get the ethtool info.  If there are any scenarios where things take 
>>> more than 1 second then I would say we are taking too long and we 
>>> would have to look into caching because we want to avoid holding the 
>>> rtnl lock for any extended period of time.
>>
>> I've described the limitations the caching approach imposes above. I 
>> suggest we follow your approach here - solve the problems when they 
>> arrive. ;) I haven't succeeded to make it take as long as even near 
>> one second.
>>
>>>
>>> My advice is get the PF patches in first, then we can look at trying 
>>> to get feature parity on the VF.
>>
>> "feature parity on the VF"?
>
> Get "ethtool -x/X" working on the PF, and then get it working on the 
> VF.  That way it makes for very simple verification as you can inspect 
> that the PF and VF tables are consistent.  As I said if nothing else 
> you can probably take a look at the igb driver to see how this was 
> done on the 1Gbps Intel hardware.

If memory serves me well I've written the ethtool -x|-X implementation 
for bnx2x once so I have a general idea of what there should be. ;)
Making ethtool -X work is a bit more sensitive since a simplistic 
implementation like u have in igb may lead to a momentary out-of-order 
packet arrival in the context of the same stream - the packet that 
arrives later on wire may be served earlier since there may still be 
unhandled packets on the "old" RSS queue when it arrives. This is unless 
updating the RSS indirection table registers ensures the fast path 
queues flushing.

The proper solution should ensure the fast path queues draining before 
updating the indirection table. In any case, I'd prefer not to implement 
the -X option and concentrate on -x option only.

>
>>
>> I'll send the patches later today. I'll incorporate the new PF code 
>> into the existing series.
>
> You might want to break things out into a couple series of patches. Do 
> the PF driver first just to make sure you have an understanding of 
> what you expect, think of it as a proof of concept if nothing else for 
> how you want the VF to function, then submit the VF series as a 
> separate patch set and include the mailbox API changes.

Sound reasonable.

>
> - Alex
>

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

[Vlad Zolotarov]

On 03/25/15 23:04, Alexander Duyck wrote:
>
> On 03/25/2015 01:17 PM, Vlad Zolotarov wrote:
>>
>>
>> On 03/25/15 20:35, Tantilov, Emil S wrote:
>>>> -----Original Message-----
>>>> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
>>>> Sent: Wednesday, March 25, 2015 2:28 AM
>>>> Subject: Re: [PATCH net-next v6 4/7] ixgbevf: Add a RETA query code
>>> <snip>
>>>
>>>>> Have you tested what happens if you run:
>>>>>
>>>>> while true
>>>>> do
>>>>>     ethtool --show-rxfh-indir ethX
>>>>> done
>>>>>
>>>>> in the background while passing traffic through the VF?
>>>> I understand your concerns but let's start with clarifying a few 
>>>> things.
>>>> First, VF driver is by definition not trusted. If it (or its user)
>>>> decides to do anything malicious (like u proposed above) that would
>>>> eventually hurt (only this) VF's performance - nobody should care.
>>>> However the right question here would be: "How the above use case may
>>>> hurt the corresponding PF or other VFs' performance?" And since the
>>>> mailbox operation involves quite a few MMIO writes and reads this may
>>>> slow the PF quite a bit and this may be a problem that should be taken
>>>> care of. However it wasn't my patch series that have introduced it. 
>>>> The
>>>> same problem would arise if Guest would change VF's MAC address in a
>>>> tight loop like above. Namely any VF slow path operation that would
>>>> eventually cause the VF-PF channel transaction may be used to 
>>>> create an
>>>> attack on a PF.
>>> There are operations that can be disruptive to the VF I am not 
>>> arguing that,
>>> the issue introduced by these patches has mostly to do with the fact 
>>> that now
>>> we can hit the mailbox more often for what is mostly static 
>>> information.
>>>
>>> Especially with ethtool we already had to deal with an issue caused 
>>> by net-snmp:
>>> https://sourceforge.net/p/e1000/mailman/message/32188362/
>>>
>>> Where net-snmp was being too aggressive when collecting information, 
>>> even if most of it was static.
>>
>> Emil, I don't really understand what are u trying to protect here 
>> against. If a user would want to shoot him/herself in the leg - 
>> he/she would still be able to do it with the other mailbox involving 
>> operations like MAC change. So, what's the sense to add useless lines?
>>
>>>
>>>> Perhaps storing the RSS key and the table is better option than 
>>>> having to invoke the mailbox on every read.
>>>> I don't think this could work if I understand your proposal correctly.
>>>> The only way to cache the result that would decrease the number of 
>>>> mbox
>>>> transactions would be to cache it in the VF. But how could i 
>>>> invalidate
>>>> this cache if the table content has been changed by a PF? I think the
>>>> main source of a confusion here is that u assume that PF driver is a
>>>> Linux ixgbe driver that doesn't support an indirection table change at
>>>> the moment. As I have explained above - this should not be assumed.
>>> You keep mentioning other drivers - what other driver do you mean?
>>> All the PF drivers that enable SRIOV are maintained and supported by 
>>> Intel.
>>>
>>> For HW older than X550 we can simply not allow the RSS hash to be 
>>> modified if the driver is loaded in SRIOV mode.
>>> This way the RSS info can be read once the driver is loaded. For 
>>> X550 this can all be done in the VF, so you can avoid calling the 
>>> mailbox altogether.
>>> I understand this is a bit limiting, but this is due to HW 
>>> limitation anyway (VFs do not have their own RSS config).
>>
>> Let me remind u that Linux, FreeBSD, XEN  and DPDK PF drivers are all 
>> open source so u can't actually go and "not allow" things. ;) And 
>> although Intel developers contribute most of the code there are and 
>> will be other contributors too so I doubt the proposed above approach 
>> fits the open source spirit well. ;)
>
> Actually these drivers already support multiple OSes just fine. The 
> part where I think you are confused is that you assume they all use 
> the same Mailbox API which they likely wouldn't.  I would suggest 
> taking a look at ixgbe_pfvf_api_rev in mbx.h of the VF driver. 
> Different OSes have different things that can be supported, so for 
> example the ixgbe_mbox_api_20 is reserved for a Solaris based PF/VF 
> combination.  I would suspect that FreeBSD will likely have to conform 
> to the existing APIs, or report that it only supports a different 
> version of the mailbox API.
>
>>
>> The user should actually not query the indirection table and a hash 
>> key too often. And if he/she does - it should be his/her problem.
>> However, if like with the ixgbevf_set_num_queues() u insist on your 
>> way of doing this (on caching the indirection table and hash key) - 
>> then please let me know and I will add it. Because, frankly, I care 
>> about the PF part of this series much more than for the VF part... ;)
>
> I would say you don't need to cache it, but for 82599 and x540 there 
> isn't any need to store more than 3 bits per entry, 384b, or 12 DWORDs 
> for the entire RETA of the VF since the hardware can support at most 8 
> queues w/ SR-IOV.

Isn't it 2 bits? VF may have up to 8 queues in total but it supports up 
to 4 RSS queues (according to MRQC.MRQE and PSRTYPE[n].RQPL description...

> Then you only need one message instead of 3 which will reduce quite a 
> bit of the complication with all of this.
>
> Also it might make more sense to start working on displaying this on 
> the PF before you start trying to do this on the VF.  As far as I know 
> ixgbe still doesn't have this functionality and it would make much 
> more sense to enable that first on ixgbe before you start trying to 
> find a way to feed the data to the VF.
>
> - Alex

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

[Alexander Duyck]

On 03/26/2015 11:10 AM, Vlad Zolotarov wrote:
>
>
> On 03/25/15 23:04, Alexander Duyck wrote:
>>
>> On 03/25/2015 01:17 PM, Vlad Zolotarov wrote:
>>>
>>>
>>> On 03/25/15 20:35, Tantilov, Emil S wrote:
>>>>> -----Original Message-----
>>>>> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
>>>>> Sent: Wednesday, March 25, 2015 2:28 AM
>>>>> Subject: Re: [PATCH net-next v6 4/7] ixgbevf: Add a RETA query code
>>>> <snip>
>>>>
>>>>>> Have you tested what happens if you run:
>>>>>>
>>>>>> while true
>>>>>> do
>>>>>>     ethtool --show-rxfh-indir ethX
>>>>>> done
>>>>>>
>>>>>> in the background while passing traffic through the VF?
>>>>> I understand your concerns but let's start with clarifying a few 
>>>>> things.
>>>>> First, VF driver is by definition not trusted. If it (or its user)
>>>>> decides to do anything malicious (like u proposed above) that would
>>>>> eventually hurt (only this) VF's performance - nobody should care.
>>>>> However the right question here would be: "How the above use case may
>>>>> hurt the corresponding PF or other VFs' performance?" And since the
>>>>> mailbox operation involves quite a few MMIO writes and reads this may
>>>>> slow the PF quite a bit and this may be a problem that should be 
>>>>> taken
>>>>> care of. However it wasn't my patch series that have introduced 
>>>>> it. The
>>>>> same problem would arise if Guest would change VF's MAC address in a
>>>>> tight loop like above. Namely any VF slow path operation that would
>>>>> eventually cause the VF-PF channel transaction may be used to 
>>>>> create an
>>>>> attack on a PF.
>>>> There are operations that can be disruptive to the VF I am not 
>>>> arguing that,
>>>> the issue introduced by these patches has mostly to do with the 
>>>> fact that now
>>>> we can hit the mailbox more often for what is mostly static 
>>>> information.
>>>>
>>>> Especially with ethtool we already had to deal with an issue caused 
>>>> by net-snmp:
>>>> https://sourceforge.net/p/e1000/mailman/message/32188362/
>>>>
>>>> Where net-snmp was being too aggressive when collecting 
>>>> information, even if most of it was static.
>>>
>>> Emil, I don't really understand what are u trying to protect here 
>>> against. If a user would want to shoot him/herself in the leg - 
>>> he/she would still be able to do it with the other mailbox involving 
>>> operations like MAC change. So, what's the sense to add useless lines?
>>>
>>>>
>>>>> Perhaps storing the RSS key and the table is better option than 
>>>>> having to invoke the mailbox on every read.
>>>>> I don't think this could work if I understand your proposal 
>>>>> correctly.
>>>>> The only way to cache the result that would decrease the number of 
>>>>> mbox
>>>>> transactions would be to cache it in the VF. But how could i 
>>>>> invalidate
>>>>> this cache if the table content has been changed by a PF? I think the
>>>>> main source of a confusion here is that u assume that PF driver is a
>>>>> Linux ixgbe driver that doesn't support an indirection table 
>>>>> change at
>>>>> the moment. As I have explained above - this should not be assumed.
>>>> You keep mentioning other drivers - what other driver do you mean?
>>>> All the PF drivers that enable SRIOV are maintained and supported 
>>>> by Intel.
>>>>
>>>> For HW older than X550 we can simply not allow the RSS hash to be 
>>>> modified if the driver is loaded in SRIOV mode.
>>>> This way the RSS info can be read once the driver is loaded. For 
>>>> X550 this can all be done in the VF, so you can avoid calling the 
>>>> mailbox altogether.
>>>> I understand this is a bit limiting, but this is due to HW 
>>>> limitation anyway (VFs do not have their own RSS config).
>>>
>>> Let me remind u that Linux, FreeBSD, XEN  and DPDK PF drivers are 
>>> all open source so u can't actually go and "not allow" things. ;) 
>>> And although Intel developers contribute most of the code there are 
>>> and will be other contributors too so I doubt the proposed above 
>>> approach fits the open source spirit well. ;)
>>
>> Actually these drivers already support multiple OSes just fine. The 
>> part where I think you are confused is that you assume they all use 
>> the same Mailbox API which they likely wouldn't.  I would suggest 
>> taking a look at ixgbe_pfvf_api_rev in mbx.h of the VF driver. 
>> Different OSes have different things that can be supported, so for 
>> example the ixgbe_mbox_api_20 is reserved for a Solaris based PF/VF 
>> combination.  I would suspect that FreeBSD will likely have to 
>> conform to the existing APIs, or report that it only supports a 
>> different version of the mailbox API.
>>
>>>
>>> The user should actually not query the indirection table and a hash 
>>> key too often. And if he/she does - it should be his/her problem.
>>> However, if like with the ixgbevf_set_num_queues() u insist on your 
>>> way of doing this (on caching the indirection table and hash key) - 
>>> then please let me know and I will add it. Because, frankly, I care 
>>> about the PF part of this series much more than for the VF part... ;)
>>
>> I would say you don't need to cache it, but for 82599 and x540 there 
>> isn't any need to store more than 3 bits per entry, 384b, or 12 
>> DWORDs for the entire RETA of the VF since the hardware can support 
>> at most 8 queues w/ SR-IOV.
>
> Isn't it 2 bits? VF may have up to 8 queues in total but it supports 
> up to 4 RSS queues (according to MRQC.MRQE and PSRTYPE[n].RQPL 
> description...

You might be right.  I was thinking that there was a mode with 16 pools, 
each with 8 RSS queues per pool, however it looks like that mode is 
Virtualization w/ DCB only.

- Alex

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

[Tantilov, Emil S]

>-----Original Message-----
>From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com] 
>Sent: Tuesday, March 24, 2015 11:41 AM
>To: Tantilov, Emil S; netdev@vger.kernel.org
>Cc: Kirsher, Jeffrey T; avi@cloudius-systems.com; gleb@cloudius-systems.com
>Subject: Re: [PATCH net-next v6 4/7] ixgbevf: Add a RETA query code
>
>
>
>>On 03/24/15 20:12, Tantilov, Emil S wrote:
>>BTW - there are other issues with your patches. The indirection table seems to come out as all 0s and the VF driver reports link down/up when querying it.
>
>Worked just fine to me on x540.
>What is your setup? How did u check it? Did u remember to patch "ip" tool and enable the querying?

The issue with the link is that you do not have proper locking when calling the mailbox which is messing up the link check subtask. Basically you need to protect the calls to ixgbevf_get_rss_key and get_reta with the mbx_lock similar to how the driver does it in all cases where the mailbox is called. 

Thanks,
Emil

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

[Vlad Zolotarov]

On 03/25/15 00:50, Tantilov, Emil S wrote:
>> -----Original Message-----
>> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
>> Sent: Tuesday, March 24, 2015 11:41 AM
>> To: Tantilov, Emil S; netdev@vger.kernel.org
>> Cc: Kirsher, Jeffrey T; avi@cloudius-systems.com; gleb@cloudius-systems.com
>> Subject: Re: [PATCH net-next v6 4/7] ixgbevf: Add a RETA query code
>>
>>
>>
>>> On 03/24/15 20:12, Tantilov, Emil S wrote:
>>> BTW - there are other issues with your patches. The indirection table seems to come out as all 0s and the VF driver reports link down/up when querying it.
>> Worked just fine to me on x540.
>> What is your setup? How did u check it? Did u remember to patch "ip" tool and enable the querying?
> The issue with the link is that you do not have proper locking when calling the mailbox which is messing up the link check subtask. Basically you need to protect the calls to ixgbevf_get_rss_key and get_reta with the mbx_lock similar to how the driver does it in all cases where the mailbox is called.

:D Just wrote u the same thing in a reply on your first email...
Sure. Let me fix it in v8.

thanks,
vlad

>
> Thanks,
> Emil
>

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

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 1646 bytes --]

On Wed, 2015-03-25 at 11:29 +0200, Vlad Zolotarov wrote:
> On 03/25/15 00:50, Tantilov, Emil S wrote:
> >> -----Original Message-----
> >> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
> >> Sent: Tuesday, March 24, 2015 11:41 AM
> >> To: Tantilov, Emil S; netdev@vger.kernel.org
> >> Cc: Kirsher, Jeffrey T; avi@cloudius-systems.com;
> gleb@cloudius-systems.com
> >> Subject: Re: [PATCH net-next v6 4/7] ixgbevf: Add a RETA query code
> >>
> >>
> >>
> >>> On 03/24/15 20:12, Tantilov, Emil S wrote:
> >>> BTW - there are other issues with your patches. The indirection
> table seems to come out as all 0s and the VF driver reports link
> down/up when querying it.
> >> Worked just fine to me on x540.
> >> What is your setup? How did u check it? Did u remember to patch
> "ip" tool and enable the querying?
> > The issue with the link is that you do not have proper locking when
> calling the mailbox which is messing up the link check subtask.
> Basically you need to protect the calls to ixgbevf_get_rss_key and
> get_reta with the mbx_lock similar to how the driver does it in all
> cases where the mailbox is called.
> 
> :D Just wrote u the same thing in a reply on your first email...
> Sure. Let me fix it in v8.

Since you will be producing a v8, can you please CC
intel-wired-lan@lists.osuosl.org it is a mailing list we just created
for all Linux kernel patches and kernel development for Intel wired
Ethernet drivers.

We also have a public patchworks project:
http://patchwork.ozlabs.org/project/intel-wired-lan/list/

I had planned on making an announcement on netdev later this week.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Vlad Zolotarov]

On 03/25/15 11:39, Jeff Kirsher wrote:
> On Wed, 2015-03-25 at 11:29 +0200, Vlad Zolotarov wrote:
>> On 03/25/15 00:50, Tantilov, Emil S wrote:
>>>> -----Original Message-----
>>>> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
>>>> Sent: Tuesday, March 24, 2015 11:41 AM
>>>> To: Tantilov, Emil S; netdev@vger.kernel.org
>>>> Cc: Kirsher, Jeffrey T; avi@cloudius-systems.com;
>> gleb@cloudius-systems.com
>>>> Subject: Re: [PATCH net-next v6 4/7] ixgbevf: Add a RETA query code
>>>>
>>>>
>>>>
>>>>> On 03/24/15 20:12, Tantilov, Emil S wrote:
>>>>> BTW - there are other issues with your patches. The indirection
>> table seems to come out as all 0s and the VF driver reports link
>> down/up when querying it.
>>>> Worked just fine to me on x540.
>>>> What is your setup? How did u check it? Did u remember to patch
>> "ip" tool and enable the querying?
>>> The issue with the link is that you do not have proper locking when
>> calling the mailbox which is messing up the link check subtask.
>> Basically you need to protect the calls to ixgbevf_get_rss_key and
>> get_reta with the mbx_lock similar to how the driver does it in all
>> cases where the mailbox is called.
>>
>> :D Just wrote u the same thing in a reply on your first email...
>> Sure. Let me fix it in v8.
> Since you will be producing a v8, can you please CC
> intel-wired-lan@lists.osuosl.org it is a mailing list we just created
> for all Linux kernel patches and kernel development for Intel wired
> Ethernet drivers.
>
> We also have a public patchworks project:
> http://patchwork.ozlabs.org/project/intel-wired-lan/list/
>
> I had planned on making an announcement on netdev later this week.

Sure thing, Jeff.

[PATCH net-next v6 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.

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

Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
---
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   |  2 ++
 drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c | 27 ++++++++++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h
index f9b5eae..3f14373 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h
@@ -105,6 +105,8 @@ enum ixgbe_pfvf_api_rev {
 #define IXGBE_VF_RETA_SZ        1	/* Number of RETA DWs to bring */
 #define IXGBE_VF_RETA_OFFSET    2	/* Offset in RETA */
 
+#define IXGBE_VF_GET_RSS_KEY	0x0b	/* get RSS key */
+
 /* length of permanent address message returned from PF */
 #define IXGBE_VF_PERMADDR_MSG_LEN 4
 /* word in permanent address message with the current multicast type */
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
index 301bb1b..78517a1 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
@@ -1016,6 +1016,30 @@ static int ixgbe_get_vf_reta(struct ixgbe_adapter *adapter, u32 *msgbuf, u32 vf)
 	return 0;
 }
 
+static int ixgbe_get_vf_rss_key(struct ixgbe_adapter *adapter,
+				u32 *msgbuf, u32 vf)
+{
+	struct ixgbe_hw *hw = &adapter->hw;
+	int i;
+	u32 *rss_key = &msgbuf[1];
+
+	/* verify the PF is supporting the correct API */
+	if (!adapter->vfinfo[vf].rss_query_enabled ||
+	    (adapter->vfinfo[vf].vf_api != ixgbe_mbox_api_12))
+		return -EPERM;
+
+	/* Read the RSS KEY */
+	if (hw->mac.type >= ixgbe_mac_X550) {
+		for (i = 0; i < 10; i++)
+			rss_key[i] = IXGBE_READ_REG(hw,
+						IXGBE_PFVFRSSRK(i, vf));
+	} else
+		for (i = 0; i < 10; i++)
+			rss_key[i] = IXGBE_READ_REG(hw, IXGBE_RSSRK(i));
+
+	return 0;
+}
+
 static int ixgbe_rcv_msg_from_vf(struct ixgbe_adapter *adapter, u32 vf)
 {
 	u32 mbx_size = IXGBE_VFMAILBOX_SIZE;
@@ -1075,6 +1099,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

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

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 1002 bytes --]

On Sun, 2015-03-22 at 21:01 +0200, 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.
> 
> x550 on the other hand has a separate RSS Key for every pool.
> 
> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
> ---
> 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   |  2 ++
>  drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c | 27
> ++++++++++++++++++++++++++
>  2 files changed, 29 insertions(+)

Thanks Vlad, I have applied your patch to my next-queue tree
-- 
git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/next-queue.git
dev-queue

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH net-next v6 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.

Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
---
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/mbx.h |  2 ++
 drivers/net/ethernet/intel/ixgbevf/vf.c  | 48 ++++++++++++++++++++++++++++++++
 drivers/net/ethernet/intel/ixgbevf/vf.h  |  1 +
 3 files changed, 51 insertions(+)

diff --git a/drivers/net/ethernet/intel/ixgbevf/mbx.h b/drivers/net/ethernet/intel/ixgbevf/mbx.h
index 7342554..923bb0f 100644
--- a/drivers/net/ethernet/intel/ixgbevf/mbx.h
+++ b/drivers/net/ethernet/intel/ixgbevf/mbx.h
@@ -115,6 +115,8 @@ enum ixgbe_pfvf_api_rev {
 #define IXGBE_VF_RETA_SZ        1	/* Number of RETA DWs to bring */
 #define IXGBE_VF_RETA_OFFSET    2	/* Offset in RETA */
 
+#define IXGBE_VF_GET_RSS_KEY	0x0b	/* get RSS hash key */
+
 /* length of permanent address message returned from PF */
 #define IXGBE_VF_PERMADDR_MSG_LEN	4
 /* word in permanent address message with the current multicast type */
diff --git a/drivers/net/ethernet/intel/ixgbevf/vf.c b/drivers/net/ethernet/intel/ixgbevf/vf.c
index f31e097..820f5e0 100644
--- a/drivers/net/ethernet/intel/ixgbevf/vf.c
+++ b/drivers/net/ethernet/intel/ixgbevf/vf.c
@@ -294,6 +294,54 @@ static inline int _ixgbevf_get_reta(struct ixgbe_hw *hw, u32 *msgbuf,
 }
 
 /**
+ * ixgbevf_get_rss_key - get the RSS Random Key
+ * @hw: pointer to the HW structure
+ * @reta: buffer to fill with RETA contents.
+ *
+ * The "rss_key" buffer should be big enough to contain 10 registers.
+ *
+ * Returns: 0 on success.
+ *          if API doesn't support this operation - (-EPERM).
+ */
+int ixgbevf_get_rss_key(struct ixgbe_hw *hw, u8 *rss_key)
+{
+	int err;
+	u32 msgbuf[IXGBE_VFMAILBOX_SIZE];
+
+	/* Return and error if API doesn't support RSS Random Key retrieval */
+	if (hw->api_version != ixgbe_mbox_api_12)
+		return -EPERM;
+
+	msgbuf[0] = IXGBE_VF_GET_RSS_KEY;
+	err = hw->mbx.ops.write_posted(hw, msgbuf, 1);
+
+	if (err)
+		return err;
+
+	err = hw->mbx.ops.read_posted(hw, msgbuf, 11);
+
+	if (err)
+		return err;
+
+	msgbuf[0] &= ~IXGBE_VT_MSGTYPE_CTS;
+
+	/* If 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, 40);
+
+	return 0;
+}
+
+/**
  * ixgbevf_get_reta - get the RSS redirection table (RETA) contents.
  * @hw: pointer to the HW structure
  * @reta: buffer to fill with RETA contents.
diff --git a/drivers/net/ethernet/intel/ixgbevf/vf.h b/drivers/net/ethernet/intel/ixgbevf/vf.h
index 89bc88e..0bdc5ad 100644
--- a/drivers/net/ethernet/intel/ixgbevf/vf.h
+++ b/drivers/net/ethernet/intel/ixgbevf/vf.h
@@ -211,4 +211,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 ixgbe_hw *hw, u32 *reta);
+int ixgbevf_get_rss_key(struct ixgbe_hw *hw, u8 *rss_key);
 #endif /* __IXGBE_VF_H__ */
-- 
2.1.0

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

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 976 bytes --]

On Sun, 2015-03-22 at 21:01 +0200, 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.
> 
> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
> ---
> 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/mbx.h |  2 ++
>  drivers/net/ethernet/intel/ixgbevf/vf.c  | 48
> ++++++++++++++++++++++++++++++++
>  drivers/net/ethernet/intel/ixgbevf/vf.h  |  1 +
>  3 files changed, 51 insertions(+)

Thanks Vlad, I have applied your patch to my next-queue tree
-- 
git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/next-queue.git
dev-queue

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH net-next v6 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.

Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
---
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.

Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
---
 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..037a4af 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 40;
+}
+
+static int ixgbevf_get_rxfh(struct net_device *netdev, u32 *indir, u8 *key,
+			    u8 *hfunc)
+{
+	struct ixgbevf_adapter *adapter = netdev_priv(netdev);
+	int err;
+
+	if (hfunc)
+		*hfunc = ETH_RSS_HASH_TOP;
+
+	if (indir) {
+		err = ixgbevf_get_reta(&adapter->hw, indir);
+		if (err)
+			return err;
+	}
+
+	if (key) {
+		err = ixgbevf_get_rss_key(&adapter->hw, key);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
 static const struct ethtool_ops ixgbevf_ethtool_ops = {
 	.get_settings		= ixgbevf_get_settings,
 	.get_drvinfo		= ixgbevf_get_drvinfo,
@@ -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 v6 7/7] ixgbevf: Add the appropriate ethtool ops to query RSS indirection table and key

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 1045 bytes --]

On Sun, 2015-03-22 at 21:01 +0200, 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.
> 
> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
> ---
> 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.
> 
> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
> ---
>  drivers/net/ethernet/intel/ixgbevf/ethtool.c | 58
> ++++++++++++++++++++++++++++
>  1 file changed, 58 insertions(+)

Thanks Vlad, I have applied your patch to my next-queue tree
-- 
git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/next-queue.git
dev-queue

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]