[net-next 00/11][pull request] Intel Wired LAN Driver Updates 2015-05-02

[net-next 00/11][pull request] Intel Wired LAN Driver Updates 2015-05-02

[Jeff Kirsher]

This series contains updates to igb, e100, e1000e and ixgbe.

Todd cleans up igb_enable_mas() since it should only be called for the
82575 silicon and has no clear return, so modify the function to void.

Jean Sacren found upon inspection that 'err' did not need to be
initialized, since it is immediately overwritten.

Alex Duyck provides two patches for e1000e, the first cleans up the
handling VLAN_HLEN as a part of max frame size.  Fixes the issue:
c751a3d58cf2d ("e1000e: Correctly include VLAN_HLEN when changing
interface MTU").  The second fixes an issue where the driver was not
allowing jumbo frames to be enabled when CRC stripping was disabled,
however it was allowing CRC stripping to be disabled while jumbo frames
were enabled.

Jeff (me) fixes a warning found on PPC where the use of do_div() needed
to use u64 arg and not s64.

Hiroshi Shimanoto adds three patches to to allow administrators to
enable multicast promiscuous mode on VFs by introducing a new mbox
API, IXGBE_VF_SET_MC_PROMISC, to enable/disable multicast promiscuous
mode in the VF.  Second patch adds the if_link parts to add netlink
directives and ndo entry to allow VF multicast promiscuous mode.  Last
patch implements the new netdev op to allow VF multicast promiscuous
mode.

Mark provides three ixgbe patches, first to fix the Intel On-chip System
Fabric (IOSF) Sideband message interfaces, to serialize access using both
PHY bits in the SWFW_SEMAPHORE register.  Then fixes how semaphore bits
were released, since they should be released in reverse of the order that
they were taken.  Lastly updates ixgbe to use a signed type to hold
error codes, since error codes are negative, so consistently use signed
types when handling them.

The following are changes since commit 629161f649ca259cfc1473a98347b941dd7a52bc:
  net: rocker: Use ether_addr_equal
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/next-queue master

Alexander Duyck (2):
  e1000e: Cleanup handling of VLAN_HLEN as a part of max frame size
  e1000e: Do not allow CRC stripping to be disabled on 82579 w/ jumbo
    frames

Hiroshi Shimamoto (3):
  ixgbe, ixgbevf: Add new mbox API to enable MC promiscuous mode
  if_link: Add VF multicast promiscuous control
  ixgbe: Add new ndo to allow VF multicast promiscuous mode

Jean Sacren (1):
  e100: don't initialize int object to zero

Jeff Kirsher (1):
  e1000e: fix call to do_div() to use u64 arg

Mark Rustad (3):
  ixgbe: Fix IOSF SB access issues
  ixgbe: Release semaphore bits in the right order
  ixgbe: Use a signed type to hold error codes

Todd Fujinaka (1):
  igb: simplify and clean up igb_enable_mas()

 drivers/net/ethernet/intel/e100.c                 |   2 +-
 drivers/net/ethernet/intel/e1000e/82571.c         |   2 +-
 drivers/net/ethernet/intel/e1000e/ich8lan.c       |  23 ++---
 drivers/net/ethernet/intel/e1000e/netdev.c        |  32 +++++--
 drivers/net/ethernet/intel/igb/igb_main.c         |  27 ++----
 drivers/net/ethernet/intel/ixgbe/ixgbe.h          |   2 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c  |  10 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c     |   9 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h      |   2 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c      |   4 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c    | 104 +++++++++++++++++++++
 drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h    |   2 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c     |   8 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c     | 106 ++++++++++++++--------
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |   3 +
 drivers/net/ethernet/intel/ixgbevf/mbx.h          |   2 +
 drivers/net/ethernet/intel/ixgbevf/vf.c           |  27 +++++-
 drivers/net/ethernet/intel/ixgbevf/vf.h           |   1 +
 include/linux/if_link.h                           |   1 +
 include/linux/netdevice.h                         |   3 +
 include/uapi/linux/if_link.h                      |   6 ++
 net/core/rtnetlink.c                              |  19 +++-
 22 files changed, 297 insertions(+), 98 deletions(-)

-- 
2.1.0

[net-next 01/11] igb: simplify and clean up igb_enable_mas()

[Jeff Kirsher]

From: Todd Fujinaka <todd.fujinaka@intel.com>

igb_enable_mas() should only be called for the 82575 and has no clear
return so changing it to void. Also simplify the odd conditional
expression.

Signed-off-by: Todd Fujinaka <todd.fujinaka@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 27 +++++++--------------------
 1 file changed, 7 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 8457d03..e636646 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -1834,31 +1834,19 @@ void igb_reinit_locked(struct igb_adapter *adapter)
  *
  * @adapter: adapter struct
  **/
-static s32 igb_enable_mas(struct igb_adapter *adapter)
+static void igb_enable_mas(struct igb_adapter *adapter)
 {
 	struct e1000_hw *hw = &adapter->hw;
-	u32 connsw;
-	s32 ret_val = 0;
-
-	connsw = rd32(E1000_CONNSW);
-	if (!(hw->phy.media_type == e1000_media_type_copper))
-		return ret_val;
+	u32 connsw = rd32(E1000_CONNSW);
 
 	/* configure for SerDes media detect */
-	if (!(connsw & E1000_CONNSW_SERDESD)) {
+	if ((hw->phy.media_type == e1000_media_type_copper) &&
+	    (!(connsw & E1000_CONNSW_SERDESD))) {
 		connsw |= E1000_CONNSW_ENRGSRC;
 		connsw |= E1000_CONNSW_AUTOSENSE_EN;
 		wr32(E1000_CONNSW, connsw);
 		wrfl();
-	} else if (connsw & E1000_CONNSW_SERDESD) {
-		/* already SerDes, no need to enable anything */
-		return ret_val;
-	} else {
-		netdev_info(adapter->netdev,
-			"MAS: Unable to configure feature, disabling..\n");
-		adapter->flags &= ~IGB_FLAG_MAS_ENABLE;
 	}
-	return ret_val;
 }
 
 void igb_reset(struct igb_adapter *adapter)
@@ -1978,10 +1966,9 @@ void igb_reset(struct igb_adapter *adapter)
 		adapter->ei.get_invariants(hw);
 		adapter->flags &= ~IGB_FLAG_MEDIA_RESET;
 	}
-	if (adapter->flags & IGB_FLAG_MAS_ENABLE) {
-		if (igb_enable_mas(adapter))
-			dev_err(&pdev->dev,
-				"Error enabling Media Auto Sense\n");
+	if ((mac->type == e1000_82575) &&
+	    (adapter->flags & IGB_FLAG_MAS_ENABLE)) {
+		igb_enable_mas(adapter);
 	}
 	if (hw->mac.ops.init_hw(hw))
 		dev_err(&pdev->dev, "Hardware Error\n");
-- 
2.1.0

[net-next 02/11] e100: don't initialize int object to zero

[Jeff Kirsher]

From: Jean Sacren <sakiwit@gmail.com>

'err' will be overwritten so no need to initialize it to zero.

Signed-off-by: Jean Sacren <sakiwit@gmail.com>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/e100.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/e100.c b/drivers/net/ethernet/intel/e100.c
index 1a450f4..35357ae 100644
--- a/drivers/net/ethernet/intel/e100.c
+++ b/drivers/net/ethernet/intel/e100.c
@@ -874,7 +874,7 @@ static int e100_exec_cb(struct nic *nic, struct sk_buff *skb,
 {
 	struct cb *cb;
 	unsigned long flags;
-	int err = 0;
+	int err;
 
 	spin_lock_irqsave(&nic->cb_lock, flags);
 
-- 
2.1.0

[net-next 03/11] e1000e: Cleanup handling of VLAN_HLEN as a part of max frame size

[Jeff Kirsher]

From: Alexander Duyck <alexander.h.duyck@redhat.com>

When the VLAN_HLEN was added to the calculation for the maximum frame size
there seems to have been a number of issues added to the driver.

The first issue is that in some cases the maximum frame size for a device
never really reached the actual maximum frame size as the VLAN header
length was not included the calculation for that value.  As a result some
parts only supported a maximum frame size of either 1496 in the case of
parts that didn't support jumbo frames, and 8996 in the case of the parts
that do.

The second issue is the fact that there were several checks that weren't
updated so as a result setting an MTU of 1500 was treated as enabling jumbo
frames as the calculated value was 1522 instead of 1518.  I have addressed
those by replacing ETH_FRAME_LEN with VLAN_ETH_FRAME_LEN where appropriate.

The final issue was the fact that lowering the MTU below 1500 would cause
the driver to allocate 2K buffers for the rings.  This is an old issue that
was fixed several years ago in igb/ixgbe and I am addressing now by just
replacing == with a <= so that we always just round up to 1522 for anything
that isn't a jumbo frame.

Fixes: c751a3d58cf2d ("e1000e: Correctly include VLAN_HLEN when changing interface MTU")
Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/e1000e/82571.c   |  2 +-
 drivers/net/ethernet/intel/e1000e/ich8lan.c | 10 +++++-----
 drivers/net/ethernet/intel/e1000e/netdev.c  | 18 ++++++++----------
 3 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/82571.c b/drivers/net/ethernet/intel/e1000e/82571.c
index dc79ed8..32e7775 100644
--- a/drivers/net/ethernet/intel/e1000e/82571.c
+++ b/drivers/net/ethernet/intel/e1000e/82571.c
@@ -2010,7 +2010,7 @@ const struct e1000_info e1000_82573_info = {
 	.flags2			= FLAG2_DISABLE_ASPM_L1
 				  | FLAG2_DISABLE_ASPM_L0S,
 	.pba			= 20,
-	.max_hw_frame_size	= ETH_FRAME_LEN + ETH_FCS_LEN,
+	.max_hw_frame_size	= VLAN_ETH_FRAME_LEN + ETH_FCS_LEN,
 	.get_variants		= e1000_get_variants_82571,
 	.mac_ops		= &e82571_mac_ops,
 	.phy_ops		= &e82_phy_ops_m88,
diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c
index 9d81c03..e2498db 100644
--- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
+++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
@@ -1563,7 +1563,7 @@ static s32 e1000_get_variants_ich8lan(struct e1000_adapter *adapter)
 	    ((adapter->hw.mac.type >= e1000_pch2lan) &&
 	     (!(er32(CTRL_EXT) & E1000_CTRL_EXT_LSECCK)))) {
 		adapter->flags &= ~FLAG_HAS_JUMBO_FRAMES;
-		adapter->max_hw_frame_size = ETH_FRAME_LEN + ETH_FCS_LEN;
+		adapter->max_hw_frame_size = VLAN_ETH_FRAME_LEN + ETH_FCS_LEN;
 
 		hw->mac.ops.blink_led = NULL;
 	}
@@ -5681,7 +5681,7 @@ const struct e1000_info e1000_ich8_info = {
 				  | FLAG_HAS_FLASH
 				  | FLAG_APME_IN_WUC,
 	.pba			= 8,
-	.max_hw_frame_size	= ETH_FRAME_LEN + ETH_FCS_LEN,
+	.max_hw_frame_size	= VLAN_ETH_FRAME_LEN + ETH_FCS_LEN,
 	.get_variants		= e1000_get_variants_ich8lan,
 	.mac_ops		= &ich8_mac_ops,
 	.phy_ops		= &ich8_phy_ops,
@@ -5754,7 +5754,7 @@ const struct e1000_info e1000_pch2_info = {
 	.flags2			= FLAG2_HAS_PHY_STATS
 				  | FLAG2_HAS_EEE,
 	.pba			= 26,
-	.max_hw_frame_size	= 9018,
+	.max_hw_frame_size	= 9022,
 	.get_variants		= e1000_get_variants_ich8lan,
 	.mac_ops		= &ich8_mac_ops,
 	.phy_ops		= &ich8_phy_ops,
@@ -5774,7 +5774,7 @@ const struct e1000_info e1000_pch_lpt_info = {
 	.flags2			= FLAG2_HAS_PHY_STATS
 				  | FLAG2_HAS_EEE,
 	.pba			= 26,
-	.max_hw_frame_size	= 9018,
+	.max_hw_frame_size	= 9022,
 	.get_variants		= e1000_get_variants_ich8lan,
 	.mac_ops		= &ich8_mac_ops,
 	.phy_ops		= &ich8_phy_ops,
@@ -5794,7 +5794,7 @@ const struct e1000_info e1000_pch_spt_info = {
 	.flags2			= FLAG2_HAS_PHY_STATS
 				  | FLAG2_HAS_EEE,
 	.pba			= 26,
-	.max_hw_frame_size	= 9018,
+	.max_hw_frame_size	= 9022,
 	.get_variants		= e1000_get_variants_ich8lan,
 	.mac_ops		= &ich8_mac_ops,
 	.phy_ops		= &ich8_phy_ops,
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index c509a5c..68913d1 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -3807,7 +3807,7 @@ void e1000e_reset(struct e1000_adapter *adapter)
 	/* reset Packet Buffer Allocation to default */
 	ew32(PBA, pba);
 
-	if (adapter->max_frame_size > ETH_FRAME_LEN + ETH_FCS_LEN) {
+	if (adapter->max_frame_size > (VLAN_ETH_FRAME_LEN + ETH_FCS_LEN)) {
 		/* To maintain wire speed transmits, the Tx FIFO should be
 		 * large enough to accommodate two full transmit packets,
 		 * rounded up to the next 1KB and expressed in KB.  Likewise,
@@ -4196,9 +4196,9 @@ static int e1000_sw_init(struct e1000_adapter *adapter)
 {
 	struct net_device *netdev = adapter->netdev;
 
-	adapter->rx_buffer_len = ETH_FRAME_LEN + VLAN_HLEN + ETH_FCS_LEN;
+	adapter->rx_buffer_len = VLAN_ETH_FRAME_LEN + ETH_FCS_LEN;
 	adapter->rx_ps_bsize0 = 128;
-	adapter->max_frame_size = netdev->mtu + ETH_HLEN + ETH_FCS_LEN;
+	adapter->max_frame_size = netdev->mtu + VLAN_ETH_HLEN + ETH_FCS_LEN;
 	adapter->min_frame_size = ETH_ZLEN + ETH_FCS_LEN;
 	adapter->tx_ring_count = E1000_DEFAULT_TXD;
 	adapter->rx_ring_count = E1000_DEFAULT_RXD;
@@ -5781,17 +5781,17 @@ struct rtnl_link_stats64 *e1000e_get_stats64(struct net_device *netdev,
 static int e1000_change_mtu(struct net_device *netdev, int new_mtu)
 {
 	struct e1000_adapter *adapter = netdev_priv(netdev);
-	int max_frame = new_mtu + VLAN_HLEN + ETH_HLEN + ETH_FCS_LEN;
+	int max_frame = new_mtu + VLAN_ETH_HLEN + ETH_FCS_LEN;
 
 	/* Jumbo frame support */
-	if ((max_frame > ETH_FRAME_LEN + ETH_FCS_LEN) &&
+	if ((max_frame > (VLAN_ETH_FRAME_LEN + ETH_FCS_LEN)) &&
 	    !(adapter->flags & FLAG_HAS_JUMBO_FRAMES)) {
 		e_err("Jumbo Frames not supported.\n");
 		return -EINVAL;
 	}
 
 	/* Supported frame sizes */
-	if ((new_mtu < ETH_ZLEN + ETH_FCS_LEN + VLAN_HLEN) ||
+	if ((new_mtu < (VLAN_ETH_ZLEN + ETH_FCS_LEN)) ||
 	    (max_frame > adapter->max_hw_frame_size)) {
 		e_err("Unsupported MTU setting\n");
 		return -EINVAL;
@@ -5831,10 +5831,8 @@ static int e1000_change_mtu(struct net_device *netdev, int new_mtu)
 		adapter->rx_buffer_len = 4096;
 
 	/* adjust allocation if LPE protects us, and we aren't using SBP */
-	if ((max_frame == ETH_FRAME_LEN + ETH_FCS_LEN) ||
-	    (max_frame == ETH_FRAME_LEN + VLAN_HLEN + ETH_FCS_LEN))
-		adapter->rx_buffer_len = ETH_FRAME_LEN + VLAN_HLEN
-		    + ETH_FCS_LEN;
+	if (max_frame <= (VLAN_ETH_FRAME_LEN + ETH_FCS_LEN))
+		adapter->rx_buffer_len = VLAN_ETH_FRAME_LEN + ETH_FCS_LEN;
 
 	if (netif_running(netdev))
 		e1000e_up(adapter);
-- 
2.1.0

[net-next 04/11] e1000e: Do not allow CRC stripping to be disabled on 82579 w/ jumbo frames

[Jeff Kirsher]

From: Alexander Duyck <alexander.h.duyck@redhat.com>

 The driver wasn't allowing jumbo frames to be
 enabled when CRC stripping was disabled, however it was allowing CRC
 stripping to be disabled while jumbo frames were enabled.  This fixes that by
 making it so that the NETIF_F_RXFCS flag cannot be set when jumbo frames are
 enabled on 82579 and newer parts.

Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/e1000e/netdev.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 68913d1..7dd2c11 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -6676,6 +6676,19 @@ static void e1000_eeprom_checks(struct e1000_adapter *adapter)
 	}
 }
 
+static netdev_features_t e1000_fix_features(struct net_device *netdev,
+					    netdev_features_t features)
+{
+	struct e1000_adapter *adapter = netdev_priv(netdev);
+	struct e1000_hw *hw = &adapter->hw;
+
+	/* Jumbo frame workaround on 82579 and newer requires CRC be stripped */
+	if ((hw->mac.type >= e1000_pch2lan) && (netdev->mtu > ETH_DATA_LEN))
+		features &= ~NETIF_F_RXFCS;
+
+	return features;
+}
+
 static int e1000_set_features(struct net_device *netdev,
 			      netdev_features_t features)
 {
@@ -6732,6 +6745,7 @@ static const struct net_device_ops e1000e_netdev_ops = {
 	.ndo_poll_controller	= e1000_netpoll,
 #endif
 	.ndo_set_features = e1000_set_features,
+	.ndo_fix_features = e1000_fix_features,
 };
 
 /**
-- 
2.1.0

[net-next 05/11] e1000e: fix call to do_div() to use u64 arg

[Jeff Kirsher]

We were using s64 for lat_ns (latency nano-second value) since in
our calculations a negative value could be a resultant.  For negative
values, we then assign lat_ns to be zero, so the value passed to
do_div() was never negative, but do_div() expects the argument type
to be u64, so do a cast to resolve a compile warning seen on
PowerPC.

CC: Yanjiang Jin <yanjiang.jin@windriver.com>
CC: Yanir Lubetkin <yanirx.lubetkin@intel.com>
Reported-by: Yanjiang Jin <yanjiang.jin@windriver.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
---
 drivers/net/ethernet/intel/e1000e/ich8lan.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c
index e2498db..e18443a 100644
--- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
+++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
@@ -1015,7 +1015,7 @@ static s32 e1000_platform_pm_pch_lpt(struct e1000_hw *hw, bool link)
 		u16 max_snoop, max_nosnoop;
 		u16 max_ltr_enc;	/* max LTR latency encoded */
 		s64 lat_ns;	/* latency (ns) */
-		s64 value;
+		u64 value;
 		u32 rxa;
 
 		if (!hw->adapter->max_frame_size) {
@@ -1042,12 +1042,13 @@ static s32 e1000_platform_pm_pch_lpt(struct e1000_hw *hw, bool link)
 		 */
 		lat_ns = ((s64)rxa * 1024 -
 			  (2 * (s64)hw->adapter->max_frame_size)) * 8 * 1000;
-		if (lat_ns < 0)
-			lat_ns = 0;
-		else
-			do_div(lat_ns, speed);
+		if (lat_ns < 0) {
+			value = 0;
+		} else {
+			value = lat_ns;
+			do_div(value, speed);
+		}
 
-		value = lat_ns;
 		while (value > PCI_LTR_VALUE_MASK) {
 			scale++;
 			value = DIV_ROUND_UP(value, (1 << 5));
-- 
2.1.0

[net-next 06/11] ixgbe, ixgbevf: Add new mbox API to enable MC promiscuous mode

[Jeff Kirsher]

From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>

The limitation of the number of multicast address for VF is not enough
for the large scale server with SR-IOV feature.
IPv6 requires the multicast MAC address for each IP address to handle
the Neighbor Solicitation message.
We couldn't assign over 30 IPv6 addresses to a single VF interface.

The easy way to solve this is enabling multicast promiscuous mode.
It is good to have a functionality to enable multicast promiscuous mode
for each VF from VF driver.

This patch introduces the new mbox API, IXGBE_VF_SET_MC_PROMISC, to
enable/disable multicast promiscuous mode in VF. If multicast
promiscuous mode is enabled the VF can receive all multicast packets.

With this patch, the ixgbevf driver automatically enable multicast
promiscuous mode when the number of multicast addresses is over than 30
if possible.

CC: Sy Choi Jong <sy.jong.choi@intel.com>
Signed-off-by: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
Reviewed-by: Hayato Momma <h-momma@ce.jp.nec.com>
Tested-by: Phil Schmitt <phillip.j.schmitt@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe.h          |  1 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h      |  2 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c    | 76 +++++++++++++++++++++++
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |  3 +
 drivers/net/ethernet/intel/ixgbevf/mbx.h          |  2 +
 drivers/net/ethernet/intel/ixgbevf/vf.c           | 27 +++++++-
 drivers/net/ethernet/intel/ixgbevf/vf.h           |  1 +
 7 files changed, 111 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index 636f9e3..08e65b6 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -146,6 +146,7 @@ struct vf_data_storage {
 	u16 vlans_enabled;
 	bool clear_to_send;
 	bool pf_set_mac;
+	bool mc_promisc;
 	u16 pf_vlan; /* When set, guest VLAN config not allowed. */
 	u16 pf_qos;
 	u16 tx_rate;
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h
index b1e4703..dd623ca 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h
@@ -102,6 +102,8 @@ enum ixgbe_pfvf_api_rev {
 #define IXGBE_VF_GET_RETA	0x0a	/* VF request for RETA */
 #define IXGBE_VF_GET_RSS_KEY	0x0b	/* get RSS key */
 
+#define IXGBE_VF_SET_MC_PROMISC	0x0c	/* VF requests PF to set MC promiscuous */
+
 /* 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 1d17b58..615f651 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
@@ -116,6 +116,9 @@ static int __ixgbe_enable_sriov(struct ixgbe_adapter *adapter)
 			 * we want to disable the querying by default.
 			 */
 			adapter->vfinfo[i].rss_query_enabled = 0;
+
+			/* Turn multicast promiscuous mode off for all VFs */
+			adapter->vfinfo[i].mc_promisc = false;
 		}
 
 		return 0;
@@ -318,6 +321,40 @@ int ixgbe_pci_sriov_configure(struct pci_dev *dev, int num_vfs)
 		return ixgbe_pci_sriov_enable(dev, num_vfs);
 }
 
+static int ixgbe_enable_vf_mc_promisc(struct ixgbe_adapter *adapter, u32 vf)
+{
+	struct ixgbe_hw *hw;
+	u32 vmolr;
+
+	hw = &adapter->hw;
+	vmolr = IXGBE_READ_REG(hw, IXGBE_VMOLR(vf));
+
+	e_info(drv, "VF %u: enabling multicast promiscuous\n", vf);
+
+	vmolr |= IXGBE_VMOLR_MPE;
+
+	IXGBE_WRITE_REG(hw, IXGBE_VMOLR(vf), vmolr);
+
+	return 0;
+}
+
+static int ixgbe_disable_vf_mc_promisc(struct ixgbe_adapter *adapter, u32 vf)
+{
+	struct ixgbe_hw *hw;
+	u32 vmolr;
+
+	hw = &adapter->hw;
+	vmolr = IXGBE_READ_REG(hw, IXGBE_VMOLR(vf));
+
+	e_info(drv, "VF %u: disabling multicast promiscuous\n", vf);
+
+	vmolr &= ~IXGBE_VMOLR_MPE;
+
+	IXGBE_WRITE_REG(hw, IXGBE_VMOLR(vf), vmolr);
+
+	return 0;
+}
+
 static int ixgbe_set_vf_multicasts(struct ixgbe_adapter *adapter,
 				   u32 *msgbuf, u32 vf)
 {
@@ -332,6 +369,12 @@ static int ixgbe_set_vf_multicasts(struct ixgbe_adapter *adapter,
 	u32 mta_reg;
 	u32 vmolr = IXGBE_READ_REG(hw, IXGBE_VMOLR(vf));
 
+	/* Disable multicast promiscuous first */
+	if (adapter->vfinfo[vf].mc_promisc) {
+		ixgbe_disable_vf_mc_promisc(adapter, vf);
+		adapter->vfinfo[vf].mc_promisc = false;
+	}
+
 	/* only so many hash values supported */
 	entries = min(entries, IXGBE_MAX_VF_MC_ENTRIES);
 
@@ -718,6 +761,12 @@ static int ixgbe_vf_reset_msg(struct ixgbe_adapter *adapter, u32 vf)
 		IXGBE_WRITE_REG(hw, IXGBE_PVFTDWBALn(q_per_pool, vf, i), 0);
 	}
 
+	/* Disable multicast promiscuous at reset */
+	if (adapter->vfinfo[vf].mc_promisc) {
+		ixgbe_disable_vf_mc_promisc(adapter, vf);
+		adapter->vfinfo[vf].mc_promisc = false;
+	}
+
 	/* reply to reset with ack and vf mac address */
 	msgbuf[0] = IXGBE_VF_RESET;
 	if (!is_zero_ether_addr(vf_mac)) {
@@ -1001,6 +1050,30 @@ static int ixgbe_get_vf_rss_key(struct ixgbe_adapter *adapter,
 	return 0;
 }
 
+static int ixgbe_set_vf_mc_promisc(struct ixgbe_adapter *adapter,
+				   u32 *msgbuf, u32 vf)
+{
+	bool enable = !!msgbuf[1];	/* msgbuf contains the flag to enable */
+
+	switch (adapter->vfinfo[vf].vf_api) {
+	case ixgbe_mbox_api_12:
+		break;
+	default:
+		return -1;
+	}
+
+	/* nothing to do */
+	if (adapter->vfinfo[vf].mc_promisc == enable)
+		return 0;
+
+	adapter->vfinfo[vf].mc_promisc = enable;
+
+	if (enable)
+		return ixgbe_enable_vf_mc_promisc(adapter, vf);
+	else
+		return ixgbe_disable_vf_mc_promisc(adapter, vf);
+}
+
 static int ixgbe_rcv_msg_from_vf(struct ixgbe_adapter *adapter, u32 vf)
 {
 	u32 mbx_size = IXGBE_VFMAILBOX_SIZE;
@@ -1063,6 +1136,9 @@ static int ixgbe_rcv_msg_from_vf(struct ixgbe_adapter *adapter, u32 vf)
 	case IXGBE_VF_GET_RSS_KEY:
 		retval = ixgbe_get_vf_rss_key(adapter, msgbuf, vf);
 		break;
+	case IXGBE_VF_SET_MC_PROMISC:
+		retval = ixgbe_set_vf_mc_promisc(adapter, msgbuf, vf);
+		break;
 	default:
 		e_err(drv, "Unhandled Msg %8.8x\n", msgbuf[0]);
 		retval = IXGBE_ERR_MBX;
diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
index a16d267..2b30da9 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
+++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
@@ -2215,6 +2215,9 @@ void ixgbevf_down(struct ixgbevf_adapter *adapter)
 				IXGBE_TXDCTL_SWFLSH);
 	}
 
+	/* drop multicast promiscuous mode flag */
+	adapter->hw.mac.mc_promisc = false;
+
 	if (!pci_channel_offline(adapter->pdev))
 		ixgbevf_reset(adapter);
 
diff --git a/drivers/net/ethernet/intel/ixgbevf/mbx.h b/drivers/net/ethernet/intel/ixgbevf/mbx.h
index 82f44e0..53fba25 100644
--- a/drivers/net/ethernet/intel/ixgbevf/mbx.h
+++ b/drivers/net/ethernet/intel/ixgbevf/mbx.h
@@ -112,6 +112,8 @@ enum ixgbe_pfvf_api_rev {
 #define IXGBE_VF_GET_RETA	0x0a	/* VF request for RETA */
 #define IXGBE_VF_GET_RSS_KEY	0x0b	/* get RSS hash key */
 
+#define IXGBE_VF_SET_MC_PROMISC	0x0c	/* VF requests PF to set MC promiscuous */
+
 /* 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 d1339b0..b5aac76 100644
--- a/drivers/net/ethernet/intel/ixgbevf/vf.c
+++ b/drivers/net/ethernet/intel/ixgbevf/vf.c
@@ -120,6 +120,9 @@ static s32 ixgbevf_reset_hw_vf(struct ixgbe_hw *hw)
 	ether_addr_copy(hw->mac.perm_addr, addr);
 	hw->mac.mc_filter_type = msgbuf[IXGBE_VF_MC_TYPE_WORD];
 
+	/* after reset, MC promiscuous mode is disabled */
+	hw->mac.mc_promisc = false;
+
 	return 0;
 }
 
@@ -448,8 +451,28 @@ static s32 ixgbevf_update_mc_addr_list_vf(struct ixgbe_hw *hw,
 	 */
 
 	cnt = netdev_mc_count(netdev);
-	if (cnt > 30)
+	if (cnt > 30) {
+		/* If the API has the capability to handle MC promiscuous
+		 * mode, turn it on.
+		 */
+		if (hw->api_version == ixgbe_mbox_api_12) {
+			if (!hw->mac.mc_promisc) {
+				struct ixgbevf_adapter *adapter = hw->back;
+
+				dev_info(&adapter->pdev->dev, "Request MC PROMISC\n");
+
+				/* enabling multicast promiscuous */
+				msgbuf[0] = IXGBE_VF_SET_MC_PROMISC;
+				msgbuf[1] = 1;
+				ixgbevf_write_msg_read_ack(hw, msgbuf, 2);
+
+				hw->mac.mc_promisc = true;
+			}
+
+			return 0;
+		}
 		cnt = 30;
+	}
 	msgbuf[0] = IXGBE_VF_SET_MULTICAST;
 	msgbuf[0] |= cnt << IXGBE_VT_MSGINFO_SHIFT;
 
@@ -465,6 +488,8 @@ static s32 ixgbevf_update_mc_addr_list_vf(struct ixgbe_hw *hw,
 
 	ixgbevf_write_msg_read_ack(hw, msgbuf, IXGBE_VFMAILBOX_SIZE);
 
+	hw->mac.mc_promisc = false;
+
 	return 0;
 }
 
diff --git a/drivers/net/ethernet/intel/ixgbevf/vf.h b/drivers/net/ethernet/intel/ixgbevf/vf.h
index d40f036..f910b78 100644
--- a/drivers/net/ethernet/intel/ixgbevf/vf.h
+++ b/drivers/net/ethernet/intel/ixgbevf/vf.h
@@ -86,6 +86,7 @@ struct ixgbe_mac_info {
 	enum ixgbe_mac_type type;
 
 	s32  mc_filter_type;
+	bool mc_promisc;
 
 	bool get_link_status;
 	u32  max_tx_queues;
-- 
2.1.0

[net-next 07/11] if_link: Add VF multicast promiscuous control

[Jeff Kirsher]

From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>

Add netlink directives and ndo entry to allow VF multicast promiscuous mode.

This controls the permission to enter VF multicast promiscuous mode.
The administrator will dedicatedly allow multicast promiscuous per VF.

When the VF is under multicast promiscuous mode, all multicast packets are
sent to the VF.

Don't allow VF multicast promiscuous if the VM isn't fully trusted.

CC: Sy Choi Jong <sy.jong.choi@intel.com>
Signed-off-by: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
Reviewed-by: Hayato Momma <h-momma@ce.jp.nec.com>
Tested-by: Phil Schmitt <phillip.j.schmitt@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 include/linux/if_link.h      |  1 +
 include/linux/netdevice.h    |  3 +++
 include/uapi/linux/if_link.h |  6 ++++++
 net/core/rtnetlink.c         | 19 +++++++++++++++++--
 4 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/include/linux/if_link.h b/include/linux/if_link.h
index da49299..df212f4 100644
--- a/include/linux/if_link.h
+++ b/include/linux/if_link.h
@@ -15,5 +15,6 @@ struct ifla_vf_info {
 	__u32 min_tx_rate;
 	__u32 max_tx_rate;
 	__u32 rss_query_en;
+	__u32 mc_promisc;
 };
 #endif /* _LINUX_IF_LINK_H */
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index dbad4d7..66ffe63 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -874,6 +874,7 @@ typedef u16 (*select_queue_fallback_t)(struct net_device *dev,
  * int (*ndo_set_vf_rate)(struct net_device *dev, int vf, int min_tx_rate,
  *			  int max_tx_rate);
  * int (*ndo_set_vf_spoofchk)(struct net_device *dev, int vf, bool setting);
+ * int (*ndo_set_vf_mc_promisc)(struct net_device *dev, int vf, bool setting);
  * int (*ndo_get_vf_config)(struct net_device *dev,
  *			    int vf, struct ifla_vf_info *ivf);
  * int (*ndo_set_vf_link_state)(struct net_device *dev, int vf, int link_state);
@@ -1095,6 +1096,8 @@ struct net_device_ops {
 						   int max_tx_rate);
 	int			(*ndo_set_vf_spoofchk)(struct net_device *dev,
 						       int vf, bool setting);
+	int			(*ndo_set_vf_mc_promisc)(struct net_device *dev,
+							 int vf, bool setting);
 	int			(*ndo_get_vf_config)(struct net_device *dev,
 						     int vf,
 						     struct ifla_vf_info *ivf);
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index d9cd192..44c3bbe 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -468,6 +468,7 @@ enum {
 	IFLA_VF_RSS_QUERY_EN,	/* RSS Redirection Table and Hash Key query
 				 * on/off switch
 				 */
+	IFLA_VF_MC_PROMISC,	/* Multicast Promiscuous allow/disallow */
 	__IFLA_VF_MAX,
 };
 
@@ -517,6 +518,11 @@ struct ifla_vf_rss_query_en {
 	__u32 setting;
 };
 
+struct ifla_vf_mc_promisc {
+	__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 358d52a..e8a5801 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -819,7 +819,8 @@ static inline int rtnl_vfinfo_size(const struct net_device *dev,
 			 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_rss_query_en)));
+			 nla_total_size(sizeof(struct ifla_vf_rss_query_en)) +
+			 nla_total_size(sizeof(struct ifla_vf_mc_promisc)));
 		return size;
 	} else
 		return 0;
@@ -1134,6 +1135,7 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 			struct ifla_vf_spoofchk vf_spoofchk;
 			struct ifla_vf_link_state vf_linkstate;
 			struct ifla_vf_rss_query_en vf_rss_query_en;
+			struct ifla_vf_mc_promisc vf_mc_promisc;
 
 			/*
 			 * Not all SR-IOV capable drivers support the
@@ -1143,6 +1145,7 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 			 */
 			ivi.spoofchk = -1;
 			ivi.rss_query_en = -1;
+			ivi.mc_promisc = -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
@@ -1156,7 +1159,8 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 				vf_tx_rate.vf =
 				vf_spoofchk.vf =
 				vf_linkstate.vf =
-				vf_rss_query_en.vf = ivi.vf;
+				vf_rss_query_en.vf =
+				vf_mc_promisc.vf = ivi.vf;
 
 			memcpy(vf_mac.mac, ivi.mac, sizeof(ivi.mac));
 			vf_vlan.vlan = ivi.vlan;
@@ -1167,6 +1171,7 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 			vf_spoofchk.setting = ivi.spoofchk;
 			vf_linkstate.link_state = ivi.linkstate;
 			vf_rss_query_en.setting = ivi.rss_query_en;
+			vf_mc_promisc.setting = ivi.mc_promisc;
 			vf = nla_nest_start(skb, IFLA_VF_INFO);
 			if (!vf) {
 				nla_nest_cancel(skb, vfinfo);
@@ -1520,6 +1525,16 @@ static int do_setvfinfo(struct net_device *dev, struct nlattr *attr)
 							    ivrssq_en->setting);
 			break;
 		}
+		case IFLA_VF_MC_PROMISC: {
+			struct ifla_vf_mc_promisc *ivm;
+
+			ivm = nla_data(vf);
+			err = -EOPNOTSUPP;
+			if (ops->ndo_set_vf_mc_promisc)
+				err = ops->ndo_set_vf_mc_promisc(dev, ivm->vf,
+								 ivm->setting);
+			break;
+		}
 		default:
 			err = -EINVAL;
 			break;
-- 
2.1.0

[net-next 08/11] ixgbe: Add new ndo to allow VF multicast promiscuous mode

[Jeff Kirsher]

From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>

Implements the new netdev op to allow VF multicast promiscuous mode.
The multicast promiscuous mode is not allowed for all VFs by default.

The administrator can allow to VF multicast promiscuous mode for only
trusted VM. After allowing multicast promiscuous mode from the host,
we can use over 30 IPv6 addresses on VM.
 # ip link set dev eth0 vf 1 mc_promisc on

When disallowing multicast promiscuous mode, ixgbevf can only handle 30
IPv6 addresses at most.
 # ip link set dev eth0 vf 1 mc_promisc off

CC: Sy Jong Choi <sy.jong.choi@intel.com>
Signed-off-by: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
Reviewed-by: Hayato Momma <h-momma@ce.jp.nec.com>
Tested-by: Phil Schmitt <phillip.j.schmitt@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe.h       |  1 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c  |  5 ++++
 drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c | 32 ++++++++++++++++++++++++--
 drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h |  2 ++
 4 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index 08e65b6..4a9f74d 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -153,6 +153,7 @@ struct vf_data_storage {
 	u16 vlan_count;
 	u8 spoofchk_enabled;
 	bool rss_query_enabled;
+	u8 mc_promisc_allowed;
 	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 d3f4b0c..36072af 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -3662,6 +3662,10 @@ static void ixgbe_configure_virtualization(struct ixgbe_adapter *adapter)
 		/* Enable/Disable RSS query feature  */
 		ixgbe_ndo_set_vf_rss_query_en(adapter->netdev, i,
 					  adapter->vfinfo[i].rss_query_enabled);
+
+		/* Reconfigure multicast promiscuous mode */
+		ixgbe_ndo_set_vf_mc_promisc(adapter->netdev, i,
+					    adapter->vfinfo[i].mc_promisc_allowed);
 	}
 }
 
@@ -8165,6 +8169,7 @@ static const struct net_device_ops ixgbe_netdev_ops = {
 	.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_set_vf_mc_promisc	= ixgbe_ndo_set_vf_mc_promisc,
 	.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 615f651..42b24a0 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
@@ -117,8 +117,11 @@ static int __ixgbe_enable_sriov(struct ixgbe_adapter *adapter)
 			 */
 			adapter->vfinfo[i].rss_query_enabled = 0;
 
-			/* Turn multicast promiscuous mode off for all VFs */
+			/* Disallow VF multicast promiscuous capability
+			 * and turn it off for all VFs
+			 */
 			adapter->vfinfo[i].mc_promisc = false;
+			adapter->vfinfo[i].mc_promisc_allowed = false;
 		}
 
 		return 0;
@@ -1068,7 +1071,7 @@ static int ixgbe_set_vf_mc_promisc(struct ixgbe_adapter *adapter,
 
 	adapter->vfinfo[vf].mc_promisc = enable;
 
-	if (enable)
+	if (enable && adapter->vfinfo[vf].mc_promisc_allowed)
 		return ixgbe_enable_vf_mc_promisc(adapter, vf);
 	else
 		return ixgbe_disable_vf_mc_promisc(adapter, vf);
@@ -1492,6 +1495,30 @@ int ixgbe_ndo_set_vf_rss_query_en(struct net_device *netdev, int vf,
 	return 0;
 }
 
+int ixgbe_ndo_set_vf_mc_promisc(struct net_device *netdev, int vf, bool setting)
+{
+	struct ixgbe_adapter *adapter = netdev_priv(netdev);
+
+	if (vf >= adapter->num_vfs)
+		return -EINVAL;
+
+	/* nothing to do */
+	if (adapter->vfinfo[vf].mc_promisc_allowed == setting)
+		return 0;
+
+	adapter->vfinfo[vf].mc_promisc_allowed = setting;
+
+	/* if VF requests multicast promiscuous */
+	if (adapter->vfinfo[vf].mc_promisc) {
+		if (setting)
+			ixgbe_enable_vf_mc_promisc(adapter, vf);
+		else
+			ixgbe_disable_vf_mc_promisc(adapter, vf);
+	}
+
+	return 0;
+}
+
 int ixgbe_ndo_get_vf_config(struct net_device *netdev,
 			    int vf, struct ifla_vf_info *ivi)
 {
@@ -1506,5 +1533,6 @@ int ixgbe_ndo_get_vf_config(struct net_device *netdev,
 	ivi->qos = adapter->vfinfo[vf].pf_qos;
 	ivi->spoofchk = adapter->vfinfo[vf].spoofchk_enabled;
 	ivi->rss_query_en = adapter->vfinfo[vf].rss_query_enabled;
+	ivi->mc_promisc = adapter->vfinfo[vf].mc_promisc_allowed;
 	return 0;
 }
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h
index 2c197e6..bf5b8f1 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h
@@ -49,6 +49,8 @@ int ixgbe_ndo_set_vf_bw(struct net_device *netdev, int vf, int min_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_set_vf_mc_promisc(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

[net-next 09/11] ixgbe: Fix IOSF SB access issues

[Jeff Kirsher]

From: Mark Rustad <mark.d.rustad@intel.com>

IOSF is the Intel On-chip System Fabric used in SOCs. IOSF SB is
the IOSF SideBand message interface. This patch serializes IOSF SB
access using both phy bits in the SWFW_SEMAPHORE register. It also
adds a helper function to wait for IOSF SB accesses to complete.
Use the new function to perform this wait before each access, as
specified in the datasheet, in addition to using it to wait for
IOSF SB read/write completion.

Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
Tested-by: Phil Schmitt <phillip.j.schmitt@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c | 102 ++++++++++++++++----------
 1 file changed, 65 insertions(+), 37 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c
index cf5cf81..58ab7d9 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c
@@ -103,6 +103,39 @@ static s32 ixgbe_init_eeprom_params_X550(struct ixgbe_hw *hw)
 	return 0;
 }
 
+/**
+ * ixgbe_iosf_wait - Wait for IOSF command completion
+ * @hw: pointer to hardware structure
+ * @ctrl: pointer to location to receive final IOSF control value
+ *
+ * Return: failing status on timeout
+ *
+ * Note: ctrl can be NULL if the IOSF control register value is not needed
+ */
+static s32 ixgbe_iosf_wait(struct ixgbe_hw *hw, u32 *ctrl)
+{
+	u32 i, command;
+
+	/* Check every 10 usec to see if the address cycle completed.
+	 * The SB IOSF BUSY bit will clear when the operation is
+	 * complete.
+	 */
+	for (i = 0; i < IXGBE_MDIO_COMMAND_TIMEOUT; i++) {
+		command = IXGBE_READ_REG(hw, IXGBE_SB_IOSF_INDIRECT_CTRL);
+		if (!(command & IXGBE_SB_IOSF_CTRL_BUSY))
+			break;
+		usleep_range(10, 20);
+	}
+	if (ctrl)
+		*ctrl = command;
+	if (i == IXGBE_MDIO_COMMAND_TIMEOUT) {
+		hw_dbg(hw, "IOSF wait timed out\n");
+		return IXGBE_ERR_PHY;
+	}
+
+	return 0;
+}
+
 /** ixgbe_read_iosf_sb_reg_x550 - Writes a value to specified register of the
  *  IOSF device
  *  @hw: pointer to hardware structure
@@ -113,7 +146,17 @@ static s32 ixgbe_init_eeprom_params_X550(struct ixgbe_hw *hw)
 static s32 ixgbe_read_iosf_sb_reg_x550(struct ixgbe_hw *hw, u32 reg_addr,
 				       u32 device_type, u32 *data)
 {
-	u32 i, command, error;
+	u32 gssr = IXGBE_GSSR_PHY1_SM | IXGBE_GSSR_PHY0_SM;
+	u32 command, error;
+	s32 ret;
+
+	ret = hw->mac.ops.acquire_swfw_sync(hw, gssr);
+	if (ret)
+		return ret;
+
+	ret = ixgbe_iosf_wait(hw, NULL);
+	if (ret)
+		goto out;
 
 	command = ((reg_addr << IXGBE_SB_IOSF_CTRL_ADDR_SHIFT) |
 		   (device_type << IXGBE_SB_IOSF_CTRL_TARGET_SELECT_SHIFT));
@@ -121,17 +164,7 @@ static s32 ixgbe_read_iosf_sb_reg_x550(struct ixgbe_hw *hw, u32 reg_addr,
 	/* Write IOSF control register */
 	IXGBE_WRITE_REG(hw, IXGBE_SB_IOSF_INDIRECT_CTRL, command);
 
-	/* Check every 10 usec to see if the address cycle completed.
-	 * The SB IOSF BUSY bit will clear when the operation is
-	 * complete
-	 */
-	for (i = 0; i < IXGBE_MDIO_COMMAND_TIMEOUT; i++) {
-		usleep_range(10, 20);
-
-		command = IXGBE_READ_REG(hw, IXGBE_SB_IOSF_INDIRECT_CTRL);
-		if ((command & IXGBE_SB_IOSF_CTRL_BUSY) == 0)
-			break;
-	}
+	ret = ixgbe_iosf_wait(hw, &command);
 
 	if ((command & IXGBE_SB_IOSF_CTRL_RESP_STAT_MASK) != 0) {
 		error = (command & IXGBE_SB_IOSF_CTRL_CMPL_ERR_MASK) >>
@@ -140,14 +173,12 @@ static s32 ixgbe_read_iosf_sb_reg_x550(struct ixgbe_hw *hw, u32 reg_addr,
 		return IXGBE_ERR_PHY;
 	}
 
-	if (i == IXGBE_MDIO_COMMAND_TIMEOUT) {
-		hw_dbg(hw, "Read timed out\n");
-		return IXGBE_ERR_PHY;
-	}
-
-	*data = IXGBE_READ_REG(hw, IXGBE_SB_IOSF_INDIRECT_DATA);
+	if (!ret)
+		*data = IXGBE_READ_REG(hw, IXGBE_SB_IOSF_INDIRECT_DATA);
 
-	return 0;
+out:
+	hw->mac.ops.release_swfw_sync(hw, gssr);
+	return ret;
 }
 
 /** ixgbe_read_ee_hostif_data_X550 - Read EEPROM word using a host interface
@@ -789,7 +820,17 @@ static s32 ixgbe_get_link_capabilities_X550em(struct ixgbe_hw *hw,
 static s32 ixgbe_write_iosf_sb_reg_x550(struct ixgbe_hw *hw, u32 reg_addr,
 					u32 device_type, u32 data)
 {
-	u32 i, command, error;
+	u32 gssr = IXGBE_GSSR_PHY1_SM | IXGBE_GSSR_PHY0_SM;
+	u32 command, error;
+	s32 ret;
+
+	ret = hw->mac.ops.acquire_swfw_sync(hw, gssr);
+	if (ret)
+		return ret;
+
+	ret = ixgbe_iosf_wait(hw, NULL);
+	if (ret)
+		goto out;
 
 	command = ((reg_addr << IXGBE_SB_IOSF_CTRL_ADDR_SHIFT) |
 		   (device_type << IXGBE_SB_IOSF_CTRL_TARGET_SELECT_SHIFT));
@@ -800,17 +841,7 @@ static s32 ixgbe_write_iosf_sb_reg_x550(struct ixgbe_hw *hw, u32 reg_addr,
 	/* Write IOSF data register */
 	IXGBE_WRITE_REG(hw, IXGBE_SB_IOSF_INDIRECT_DATA, data);
 
-	/* Check every 10 usec to see if the address cycle completed.
-	 * The SB IOSF BUSY bit will clear when the operation is
-	 * complete
-	 */
-	for (i = 0; i < IXGBE_MDIO_COMMAND_TIMEOUT; i++) {
-		usleep_range(10, 20);
-
-		command = IXGBE_READ_REG(hw, IXGBE_SB_IOSF_INDIRECT_CTRL);
-		if ((command & IXGBE_SB_IOSF_CTRL_BUSY) == 0)
-			break;
-	}
+	ret = ixgbe_iosf_wait(hw, &command);
 
 	if ((command & IXGBE_SB_IOSF_CTRL_RESP_STAT_MASK) != 0) {
 		error = (command & IXGBE_SB_IOSF_CTRL_CMPL_ERR_MASK) >>
@@ -819,12 +850,9 @@ static s32 ixgbe_write_iosf_sb_reg_x550(struct ixgbe_hw *hw, u32 reg_addr,
 		return IXGBE_ERR_PHY;
 	}
 
-	if (i == IXGBE_MDIO_COMMAND_TIMEOUT) {
-		hw_dbg(hw, "Write timed out\n");
-		return IXGBE_ERR_PHY;
-	}
-
-	return 0;
+out:
+	hw->mac.ops.release_swfw_sync(hw, gssr);
+	return ret;
 }
 
 /** ixgbe_setup_ixfi_x550em - Configure the KR PHY for iXFI mode.
-- 
2.1.0

[net-next 10/11] ixgbe: Release semaphore bits in the right order

[Jeff Kirsher]

From: Mark Rustad <mark.d.rustad@intel.com>

The global semaphore bits should be released in the reverse of the
order that they were taken, so correct that.

Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
Tested-by: Phil Schmitt <phillip.j.schmitt@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c
index f5f948d..0a8b5e4 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c
@@ -696,14 +696,14 @@ static void ixgbe_release_swfw_sync_semaphore(struct ixgbe_hw *hw)
 
 	/* Release both semaphores by writing 0 to the bits REGSMP and SMBI */
 
-	swsm = IXGBE_READ_REG(hw, IXGBE_SWSM);
-	swsm &= ~IXGBE_SWSM_SMBI;
-	IXGBE_WRITE_REG(hw, IXGBE_SWSM, swsm);
-
 	swsm = IXGBE_READ_REG(hw, IXGBE_SWFW_SYNC);
 	swsm &= ~IXGBE_SWFW_REGSMP;
 	IXGBE_WRITE_REG(hw, IXGBE_SWFW_SYNC, swsm);
 
+	swsm = IXGBE_READ_REG(hw, IXGBE_SWSM);
+	swsm &= ~IXGBE_SWSM_SMBI;
+	IXGBE_WRITE_REG(hw, IXGBE_SWSM, swsm);
+
 	IXGBE_WRITE_FLUSH(hw);
 }
 
-- 
2.1.0

[net-next 11/11] ixgbe: Use a signed type to hold error codes

[Jeff Kirsher]

From: Mark Rustad <mark.d.rustad@intel.com>

Because error codes are negative, it only makes sense to
consistently use signed types when handling them. Also remove
some explicit comparisons with 0 on these variables.

Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
Tested-by: Phil Schmitt <phillip.j.schmitt@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 10 +++++-----
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c    |  4 ++--
 drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c     |  4 ++--
 drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c    |  4 ++--
 4 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
index eafa9ec..9f6fb19 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
@@ -3053,7 +3053,7 @@ static int ixgbe_get_module_info(struct net_device *dev,
 {
 	struct ixgbe_adapter *adapter = netdev_priv(dev);
 	struct ixgbe_hw *hw = &adapter->hw;
-	u32 status;
+	s32 status;
 	u8 sff8472_rev, addr_mode;
 	bool page_swap = false;
 
@@ -3061,14 +3061,14 @@ static int ixgbe_get_module_info(struct net_device *dev,
 	status = hw->phy.ops.read_i2c_eeprom(hw,
 					     IXGBE_SFF_SFF_8472_COMP,
 					     &sff8472_rev);
-	if (status != 0)
+	if (status)
 		return -EIO;
 
 	/* addressing mode is not supported */
 	status = hw->phy.ops.read_i2c_eeprom(hw,
 					     IXGBE_SFF_SFF_8472_SWAP,
 					     &addr_mode);
-	if (status != 0)
+	if (status)
 		return -EIO;
 
 	if (addr_mode & IXGBE_SFF_ADDRESSING_MODE) {
@@ -3095,7 +3095,7 @@ static int ixgbe_get_module_eeprom(struct net_device *dev,
 {
 	struct ixgbe_adapter *adapter = netdev_priv(dev);
 	struct ixgbe_hw *hw = &adapter->hw;
-	u32 status = IXGBE_ERR_PHY_ADDR_INVALID;
+	s32 status = IXGBE_ERR_PHY_ADDR_INVALID;
 	u8 databyte = 0xFF;
 	int i = 0;
 
@@ -3112,7 +3112,7 @@ static int ixgbe_get_module_eeprom(struct net_device *dev,
 		else
 			status = hw->phy.ops.read_i2c_sff8472(hw, i, &databyte);
 
-		if (status != 0)
+		if (status)
 			return -EIO;
 
 		data[i - ee->offset] = databyte;
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 36072af..66adbd0 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -4761,7 +4761,7 @@ static int ixgbe_non_sfp_link_config(struct ixgbe_hw *hw)
 {
 	u32 speed;
 	bool autoneg, link_up = false;
-	u32 ret = IXGBE_ERR_LINK_SETUP;
+	int ret = IXGBE_ERR_LINK_SETUP;
 
 	if (hw->mac.ops.check_link)
 		ret = hw->mac.ops.check_link(hw, &speed, &link_up, false);
@@ -8026,7 +8026,7 @@ static int ixgbe_ndo_bridge_setlink(struct net_device *dev,
 		return -EINVAL;
 
 	nla_for_each_nested(attr, br_spec, rem) {
-		u32 status;
+		int status;
 		__u16 mode;
 
 		if (nla_type(attr) != IFLA_BRIDGE_MODE)
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c
index 8a2be44..af828f8 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c
@@ -317,14 +317,14 @@ bool ixgbe_check_reset_blocked(struct ixgbe_hw *hw)
  **/
 static s32 ixgbe_get_phy_id(struct ixgbe_hw *hw)
 {
-	u32 status;
+	s32 status;
 	u16 phy_id_high = 0;
 	u16 phy_id_low = 0;
 
 	status = hw->phy.ops.read_reg(hw, MDIO_DEVID1, MDIO_MMD_PMAPMD,
 				      &phy_id_high);
 
-	if (status == 0) {
+	if (!status) {
 		hw->phy.id = (u32)(phy_id_high << 16);
 		status = hw->phy.ops.read_reg(hw, MDIO_DEVID2, MDIO_MMD_PMAPMD,
 					      &phy_id_low);
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c
index 58ab7d9..b023698 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c
@@ -1063,7 +1063,7 @@ static s32 ixgbe_setup_kr_x550em(struct ixgbe_hw *hw)
  **/
 static s32 ixgbe_setup_internal_phy_x550em(struct ixgbe_hw *hw)
 {
-	u32 status;
+	s32 status;
 	u16 lasi, autoneg_status, speed;
 	ixgbe_link_speed force_speed;
 
@@ -1205,7 +1205,7 @@ static enum ixgbe_media_type ixgbe_get_media_type_X550em(struct ixgbe_hw *hw)
  **/
 static s32 ixgbe_init_ext_t_x550em(struct ixgbe_hw *hw)
 {
-	u32 status;
+	s32 status;
 	u16 reg;
 	u32 retries = 2;
 
-- 
2.1.0

Re: [net-next 07/11] if_link: Add VF multicast promiscuous control

[Or Gerlitz]

On Sat, May 2, 2015 at 1:42 PM, Jeff Kirsher
<jeffrey.t.kirsher@intel.com> wrote:
> From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
>
> Add netlink directives and ndo entry to allow VF multicast promiscuous mode.
>
> This controls the permission to enter VF multicast promiscuous mode.
> The administrator will dedicatedly allow multicast promiscuous per VF.
>
> When the VF is under multicast promiscuous mode, all multicast packets are
> sent to the VF.
>
> Don't allow VF multicast promiscuous if the VM isn't fully trusted.

Guys,

I don't think the discussion we held in the past [1] on the matter
actually converged. Few open points that came up while
debating it internally with Rony:

1. maybe what we we actually want here an API that states a VF to be
privileged/trusted and then we can over load the feature set of being
such?

2. the suggested API only allows either unlimited number of mulicast
groups per VF or limited number, both numbers are vendor dependent,
right? maybe what we need for this specific matter is specifying how
many multicast groups are allowed for a VF?

[1] http://marc.info/?t=142439436800007&r=1&w=2


>
> CC: Sy Choi Jong <sy.jong.choi@intel.com>
> Signed-off-by: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
> Reviewed-by: Hayato Momma <h-momma@ce.jp.nec.com>
> Tested-by: Phil Schmitt <phillip.j.schmitt@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> ---
>  include/linux/if_link.h      |  1 +
>  include/linux/netdevice.h    |  3 +++
>  include/uapi/linux/if_link.h |  6 ++++++
>  net/core/rtnetlink.c         | 19 +++++++++++++++++--
>  4 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/if_link.h b/include/linux/if_link.h
> index da49299..df212f4 100644
> --- a/include/linux/if_link.h
> +++ b/include/linux/if_link.h
> @@ -15,5 +15,6 @@ struct ifla_vf_info {
>         __u32 min_tx_rate;
>         __u32 max_tx_rate;
>         __u32 rss_query_en;
> +       __u32 mc_promisc;
>  };
>  #endif /* _LINUX_IF_LINK_H */
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index dbad4d7..66ffe63 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -874,6 +874,7 @@ typedef u16 (*select_queue_fallback_t)(struct net_device *dev,
>   * int (*ndo_set_vf_rate)(struct net_device *dev, int vf, int min_tx_rate,
>   *                       int max_tx_rate);
>   * int (*ndo_set_vf_spoofchk)(struct net_device *dev, int vf, bool setting);
> + * int (*ndo_set_vf_mc_promisc)(struct net_device *dev, int vf, bool setting);
>   * int (*ndo_get_vf_config)(struct net_device *dev,
>   *                         int vf, struct ifla_vf_info *ivf);
>   * int (*ndo_set_vf_link_state)(struct net_device *dev, int vf, int link_state);
> @@ -1095,6 +1096,8 @@ struct net_device_ops {
>                                                    int max_tx_rate);
>         int                     (*ndo_set_vf_spoofchk)(struct net_device *dev,
>                                                        int vf, bool setting);
> +       int                     (*ndo_set_vf_mc_promisc)(struct net_device *dev,
> +                                                        int vf, bool setting);
>         int                     (*ndo_get_vf_config)(struct net_device *dev,
>                                                      int vf,
>                                                      struct ifla_vf_info *ivf);
> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
> index d9cd192..44c3bbe 100644
> --- a/include/uapi/linux/if_link.h
> +++ b/include/uapi/linux/if_link.h
> @@ -468,6 +468,7 @@ enum {
>         IFLA_VF_RSS_QUERY_EN,   /* RSS Redirection Table and Hash Key query
>                                  * on/off switch
>                                  */
> +       IFLA_VF_MC_PROMISC,     /* Multicast Promiscuous allow/disallow */
>         __IFLA_VF_MAX,
>  };
>
> @@ -517,6 +518,11 @@ struct ifla_vf_rss_query_en {
>         __u32 setting;
>  };
>
> +struct ifla_vf_mc_promisc {
> +       __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 358d52a..e8a5801 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -819,7 +819,8 @@ static inline int rtnl_vfinfo_size(const struct net_device *dev,
>                          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_rss_query_en)));
> +                        nla_total_size(sizeof(struct ifla_vf_rss_query_en)) +
> +                        nla_total_size(sizeof(struct ifla_vf_mc_promisc)));
>                 return size;
>         } else
>                 return 0;
> @@ -1134,6 +1135,7 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
>                         struct ifla_vf_spoofchk vf_spoofchk;
>                         struct ifla_vf_link_state vf_linkstate;
>                         struct ifla_vf_rss_query_en vf_rss_query_en;
> +                       struct ifla_vf_mc_promisc vf_mc_promisc;
>
>                         /*
>                          * Not all SR-IOV capable drivers support the
> @@ -1143,6 +1145,7 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
>                          */
>                         ivi.spoofchk = -1;
>                         ivi.rss_query_en = -1;
> +                       ivi.mc_promisc = -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
> @@ -1156,7 +1159,8 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
>                                 vf_tx_rate.vf =
>                                 vf_spoofchk.vf =
>                                 vf_linkstate.vf =
> -                               vf_rss_query_en.vf = ivi.vf;
> +                               vf_rss_query_en.vf =
> +                               vf_mc_promisc.vf = ivi.vf;
>
>                         memcpy(vf_mac.mac, ivi.mac, sizeof(ivi.mac));
>                         vf_vlan.vlan = ivi.vlan;
> @@ -1167,6 +1171,7 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
>                         vf_spoofchk.setting = ivi.spoofchk;
>                         vf_linkstate.link_state = ivi.linkstate;
>                         vf_rss_query_en.setting = ivi.rss_query_en;
> +                       vf_mc_promisc.setting = ivi.mc_promisc;
>                         vf = nla_nest_start(skb, IFLA_VF_INFO);
>                         if (!vf) {
>                                 nla_nest_cancel(skb, vfinfo);
> @@ -1520,6 +1525,16 @@ static int do_setvfinfo(struct net_device *dev, struct nlattr *attr)
>                                                             ivrssq_en->setting);
>                         break;
>                 }
> +               case IFLA_VF_MC_PROMISC: {
> +                       struct ifla_vf_mc_promisc *ivm;
> +
> +                       ivm = nla_data(vf);
> +                       err = -EOPNOTSUPP;
> +                       if (ops->ndo_set_vf_mc_promisc)
> +                               err = ops->ndo_set_vf_mc_promisc(dev, ivm->vf,
> +                                                                ivm->setting);
> +                       break;
> +               }
>                 default:
>                         err = -EINVAL;
>                         break;
> --
> 2.1.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [net-next 00/11][pull request] Intel Wired LAN Driver Updates 2015-05-02

[David Miller]

From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Sat,  2 May 2015 03:42:27 -0700

> This series contains updates to igb, e100, e1000e and ixgbe.

Jeff I would suggest resending this series with the various
VF multicast changes removed as those seem to need some more
discussion.

Thanks.

Re: [net-next 00/11][pull request] Intel Wired LAN Driver Updates 2015-05-02

[Jeff Kirsher]

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

On Sun, 2015-05-03 at 23:36 -0400, David Miller wrote:
> From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Date: Sat,  2 May 2015 03:42:27 -0700
> 
> > This series contains updates to igb, e100, e1000e and ixgbe.
> 
> Jeff I would suggest resending this series with the various
> VF multicast changes removed as those seem to need some more
> discussion.
> 
> Thanks.
> 

Ok, I will re-work the series.

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

RE: [net-next 07/11] if_link: Add VF multicast promiscuous control

[Skidmore, Donald C]

> -----Original Message-----
> From: Or Gerlitz [mailto:gerlitz.or@gmail.com]
> Sent: Sunday, May 03, 2015 7:16 AM
> To: Kirsher, Jeffrey T
> Cc: David Miller; Hiroshi Shimamoto; Linux Netdev List;
> nhorman@redhat.com; sassmann@redhat.com; jogreene@redhat.com;
> Choi, Sy Jong; Edward Cree; Skidmore, Donald C; Rony Efraim
> Subject: Re: [net-next 07/11] if_link: Add VF multicast promiscuous control
> 
> On Sat, May 2, 2015 at 1:42 PM, Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> wrote:
> > From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
> >
> > Add netlink directives and ndo entry to allow VF multicast promiscuous
> mode.
> >
> > This controls the permission to enter VF multicast promiscuous mode.
> > The administrator will dedicatedly allow multicast promiscuous per VF.
> >
> > When the VF is under multicast promiscuous mode, all multicast packets
> > are sent to the VF.
> >
> > Don't allow VF multicast promiscuous if the VM isn't fully trusted.
> 
> Guys,
> 
> I don't think the discussion we held in the past [1] on the matter actually
> converged. Few open points that came up while debating it internally with
> Rony:
> 
> 1. maybe what we we actually want here an API that states a VF to be
> privileged/trusted and then we can over load the feature set of being such?

I suggested this originally, but there was push back as it was thought too generic as the definition of what being a "trusted" vendor would differ from driver to driver.  Personally I still like the idea of having one mode saying that we "trust" a given VF.  Then that VF can request whatever it support it wants from the PF regardless of possible negative impact on other VF's.  What is possible to support would then be left to the interface between the VF and PF.  This of course would be dependent on what the given HW could support and would mean this mode would mean different things for different adapters and I do see why some might see this as a concern. 

> 
> 2. the suggested API only allows either unlimited number of mulicast groups
> per VF or limited number, both numbers are vendor dependent, right?
> maybe what we need for this specific matter is specifying how many
> multicast groups are allowed for a VF?

I believe the idea behind this interface was that it would allow VF's to request unlimited multicast group as opposed to the current behavior of each adapter offering some limited number.  This limit is of course defined by a given adapters HW/SW limitations.  Up until now you could keep asking for new multicast until the PF replied with an error.  So we never really exported this information before.  This new mode just allows us to never reach the point that the PF would deny a VF request to join a MC group.  Seems to me that an additional interface to provide the max number of supported multicast groups would be new functionality that could be independent of this patch and in fact could exist even without this patch.  Or am I missing what you're asking for here. :)

> 
> [1] http://marc.info/?t=142439436800007&r=1&w=2
> 
> 
> >
> > CC: Sy Choi Jong <sy.jong.choi@intel.com>
> > Signed-off-by: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
> > Reviewed-by: Hayato Momma <h-momma@ce.jp.nec.com>
> > Tested-by: Phil Schmitt <phillip.j.schmitt@intel.com>
> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> > ---
> >  include/linux/if_link.h      |  1 +
> >  include/linux/netdevice.h    |  3 +++
> >  include/uapi/linux/if_link.h |  6 ++++++
> >  net/core/rtnetlink.c         | 19 +++++++++++++++++--
> >  4 files changed, 27 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/if_link.h b/include/linux/if_link.h index
> > da49299..df212f4 100644
> > --- a/include/linux/if_link.h
> > +++ b/include/linux/if_link.h
> > @@ -15,5 +15,6 @@ struct ifla_vf_info {
> >         __u32 min_tx_rate;
> >         __u32 max_tx_rate;
> >         __u32 rss_query_en;
> > +       __u32 mc_promisc;
> >  };
> >  #endif /* _LINUX_IF_LINK_H */
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index dbad4d7..66ffe63 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -874,6 +874,7 @@ typedef u16 (*select_queue_fallback_t)(struct
> net_device *dev,
> >   * int (*ndo_set_vf_rate)(struct net_device *dev, int vf, int min_tx_rate,
> >   *                       int max_tx_rate);
> >   * int (*ndo_set_vf_spoofchk)(struct net_device *dev, int vf, bool
> > setting);
> > + * int (*ndo_set_vf_mc_promisc)(struct net_device *dev, int vf, bool
> > + setting);
> >   * int (*ndo_get_vf_config)(struct net_device *dev,
> >   *                         int vf, struct ifla_vf_info *ivf);
> >   * int (*ndo_set_vf_link_state)(struct net_device *dev, int vf, int
> > link_state); @@ -1095,6 +1096,8 @@ struct net_device_ops {
> >                                                    int max_tx_rate);
> >         int                     (*ndo_set_vf_spoofchk)(struct net_device *dev,
> >                                                        int vf, bool
> > setting);
> > +       int                     (*ndo_set_vf_mc_promisc)(struct net_device *dev,
> > +                                                        int vf, bool
> > + setting);
> >         int                     (*ndo_get_vf_config)(struct net_device *dev,
> >                                                      int vf,
> >                                                      struct
> > ifla_vf_info *ivf); diff --git a/include/uapi/linux/if_link.h
> > b/include/uapi/linux/if_link.h index d9cd192..44c3bbe 100644
> > --- a/include/uapi/linux/if_link.h
> > +++ b/include/uapi/linux/if_link.h
> > @@ -468,6 +468,7 @@ enum {
> >         IFLA_VF_RSS_QUERY_EN,   /* RSS Redirection Table and Hash Key
> query
> >                                  * on/off switch
> >                                  */
> > +       IFLA_VF_MC_PROMISC,     /* Multicast Promiscuous allow/disallow */
> >         __IFLA_VF_MAX,
> >  };
> >
> > @@ -517,6 +518,11 @@ struct ifla_vf_rss_query_en {
> >         __u32 setting;
> >  };
> >
> > +struct ifla_vf_mc_promisc {
> > +       __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
> > 358d52a..e8a5801 100644
> > --- a/net/core/rtnetlink.c
> > +++ b/net/core/rtnetlink.c
> > @@ -819,7 +819,8 @@ static inline int rtnl_vfinfo_size(const struct
> net_device *dev,
> >                          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_rss_query_en)));
> > +                        nla_total_size(sizeof(struct ifla_vf_rss_query_en)) +
> > +                        nla_total_size(sizeof(struct
> > + ifla_vf_mc_promisc)));
> >                 return size;
> >         } else
> >                 return 0;
> > @@ -1134,6 +1135,7 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct
> net_device *dev,
> >                         struct ifla_vf_spoofchk vf_spoofchk;
> >                         struct ifla_vf_link_state vf_linkstate;
> >                         struct ifla_vf_rss_query_en vf_rss_query_en;
> > +                       struct ifla_vf_mc_promisc vf_mc_promisc;
> >
> >                         /*
> >                          * Not all SR-IOV capable drivers support the
> > @@ -1143,6 +1145,7 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct
> net_device *dev,
> >                          */
> >                         ivi.spoofchk = -1;
> >                         ivi.rss_query_en = -1;
> > +                       ivi.mc_promisc = -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
> > @@ -1156,7 +1159,8 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct
> net_device *dev,
> >                                 vf_tx_rate.vf =
> >                                 vf_spoofchk.vf =
> >                                 vf_linkstate.vf =
> > -                               vf_rss_query_en.vf = ivi.vf;
> > +                               vf_rss_query_en.vf =
> > +                               vf_mc_promisc.vf = ivi.vf;
> >
> >                         memcpy(vf_mac.mac, ivi.mac, sizeof(ivi.mac));
> >                         vf_vlan.vlan = ivi.vlan; @@ -1167,6 +1171,7 @@
> > static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
> >                         vf_spoofchk.setting = ivi.spoofchk;
> >                         vf_linkstate.link_state = ivi.linkstate;
> >                         vf_rss_query_en.setting = ivi.rss_query_en;
> > +                       vf_mc_promisc.setting = ivi.mc_promisc;
> >                         vf = nla_nest_start(skb, IFLA_VF_INFO);
> >                         if (!vf) {
> >                                 nla_nest_cancel(skb, vfinfo); @@
> > -1520,6 +1525,16 @@ static int do_setvfinfo(struct net_device *dev, struct
> nlattr *attr)
> >                                                             ivrssq_en->setting);
> >                         break;
> >                 }
> > +               case IFLA_VF_MC_PROMISC: {
> > +                       struct ifla_vf_mc_promisc *ivm;
> > +
> > +                       ivm = nla_data(vf);
> > +                       err = -EOPNOTSUPP;
> > +                       if (ops->ndo_set_vf_mc_promisc)
> > +                               err = ops->ndo_set_vf_mc_promisc(dev, ivm->vf,
> > +                                                                ivm->setting);
> > +                       break;
> > +               }
> >                 default:
> >                         err = -EINVAL;
> >                         break;
> > --
> > 2.1.0
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe netdev" in
> > the body of a message to majordomo@vger.kernel.org More majordomo
> info
> > at  http://vger.kernel.org/majordomo-info.html

RE: [net-next 07/11] if_link: Add VF multicast promiscuous control

[Hiroshi Shimamoto]

> > -----Original Message-----
> > From: Or Gerlitz [mailto:gerlitz.or@gmail.com]
> > Sent: Sunday, May 03, 2015 7:16 AM
> > To: Kirsher, Jeffrey T
> > Cc: David Miller; Hiroshi Shimamoto; Linux Netdev List;
> > nhorman@redhat.com; sassmann@redhat.com; jogreene@redhat.com;
> > Choi, Sy Jong; Edward Cree; Skidmore, Donald C; Rony Efraim
> > Subject: Re: [net-next 07/11] if_link: Add VF multicast promiscuous control
> >
> > On Sat, May 2, 2015 at 1:42 PM, Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> > wrote:
> > > From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
> > >
> > > Add netlink directives and ndo entry to allow VF multicast promiscuous
> > mode.
> > >
> > > This controls the permission to enter VF multicast promiscuous mode.
> > > The administrator will dedicatedly allow multicast promiscuous per VF.
> > >
> > > When the VF is under multicast promiscuous mode, all multicast packets
> > > are sent to the VF.
> > >
> > > Don't allow VF multicast promiscuous if the VM isn't fully trusted.
> >
> > Guys,
> >
> > I don't think the discussion we held in the past [1] on the matter actually
> > converged. Few open points that came up while debating it internally with
> > Rony:
> >
> > 1. maybe what we we actually want here an API that states a VF to be
> > privileged/trusted and then we can over load the feature set of being such?
> 
> I suggested this originally, but there was push back as it was thought too generic as the definition of what being a "trusted"
> vendor would differ from driver to driver.  Personally I still like the idea of having one mode saying that we "trust"
> a given VF.  Then that VF can request whatever it support it wants from the PF regardless of possible negative impact
> on other VF's.  What is possible to support would then be left to the interface between the VF and PF.  This of course
> would be dependent on what the given HW could support and would mean this mode would mean different things for different
> adapters and I do see why some might see this as a concern.

The point is granularity, right?
Allow everything or allow subset of features.

> 
> >
> > 2. the suggested API only allows either unlimited number of mulicast groups
> > per VF or limited number, both numbers are vendor dependent, right?
> > maybe what we need for this specific matter is specifying how many
> > multicast groups are allowed for a VF?
> 
> I believe the idea behind this interface was that it would allow VF's to request unlimited multicast group as opposed
> to the current behavior of each adapter offering some limited number.  This limit is of course defined by a given adapters
> HW/SW limitations.  Up until now you could keep asking for new multicast until the PF replied with an error.  So we never
> really exported this information before.  This new mode just allows us to never reach the point that the PF would deny
> a VF request to join a MC group.  Seems to me that an additional interface to provide the max number of supported multicast
> groups would be new functionality that could be independent of this patch and in fact could exist even without this patch.
> Or am I missing what you're asking for here. :)

I think that the current limitation of multicast on ixgbevf comes from the
implementation of mailbox API between VF and PF which has 32 words.

By the way, our requirement is to make VF promiscuous mode for SDN/NFV usage.
And there is a feature to enable in HW, we'd like to use it.
I know there is a possibility of performance degradation.

thanks,
Hiroshi

RE: [net-next 07/11] if_link: Add VF multicast promiscuous control

[Skidmore, Donald C]

> -----Original Message-----
> From: Hiroshi Shimamoto [mailto:h-shimamoto@ct.jp.nec.com]
> Sent: Wednesday, May 06, 2015 10:55 PM
> To: Skidmore, Donald C; Or Gerlitz; Kirsher, Jeffrey T
> Cc: David Miller; Linux Netdev List; nhorman@redhat.com;
> sassmann@redhat.com; jogreene@redhat.com; Choi, Sy Jong; Edward Cree;
> Rony Efraim
> Subject: RE: [net-next 07/11] if_link: Add VF multicast promiscuous control
> 
> > > -----Original Message-----
> > > From: Or Gerlitz [mailto:gerlitz.or@gmail.com]
> > > Sent: Sunday, May 03, 2015 7:16 AM
> > > To: Kirsher, Jeffrey T
> > > Cc: David Miller; Hiroshi Shimamoto; Linux Netdev List;
> > > nhorman@redhat.com; sassmann@redhat.com; jogreene@redhat.com;
> Choi,
> > > Sy Jong; Edward Cree; Skidmore, Donald C; Rony Efraim
> > > Subject: Re: [net-next 07/11] if_link: Add VF multicast promiscuous
> > > control
> > >
> > > On Sat, May 2, 2015 at 1:42 PM, Jeff Kirsher
> > > <jeffrey.t.kirsher@intel.com>
> > > wrote:
> > > > From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
> > > >
> > > > Add netlink directives and ndo entry to allow VF multicast
> > > > promiscuous
> > > mode.
> > > >
> > > > This controls the permission to enter VF multicast promiscuous mode.
> > > > The administrator will dedicatedly allow multicast promiscuous per VF.
> > > >
> > > > When the VF is under multicast promiscuous mode, all multicast
> > > > packets are sent to the VF.
> > > >
> > > > Don't allow VF multicast promiscuous if the VM isn't fully trusted.
> > >
> > > Guys,
> > >
> > > I don't think the discussion we held in the past [1] on the matter
> > > actually converged. Few open points that came up while debating it
> > > internally with
> > > Rony:
> > >
> > > 1. maybe what we we actually want here an API that states a VF to be
> > > privileged/trusted and then we can over load the feature set of being
> such?
> >
> > I suggested this originally, but there was push back as it was thought too
> generic as the definition of what being a "trusted"
> > vendor would differ from driver to driver.  Personally I still like the idea of
> having one mode saying that we "trust"
> > a given VF.  Then that VF can request whatever it support it wants
> > from the PF regardless of possible negative impact on other VF's.
> > What is possible to support would then be left to the interface
> > between the VF and PF.  This of course would be dependent on what the
> given HW could support and would mean this mode would mean different
> things for different adapters and I do see why some might see this as a
> concern.
> 
> The point is granularity, right?
> Allow everything or allow subset of features.

Nice way to sum it up.  The trick being with the subset of features path is not all hardware can/will support everything.  Also I worry about worry about the feature list growing requiring more and more nobs on the PF to allow/disallow granular behavior that could brake VF isolation.  With a simple hint to the PF that a given VF is "trusted" would allow all that complexity to be contained in the mailbox protocol between the PF/VF.

All that said I realize others are concerned with the ambiguousness of such a field and can certainly live with your implementation.

> 
> >
> > >
> > > 2. the suggested API only allows either unlimited number of mulicast
> > > groups per VF or limited number, both numbers are vendor dependent,
> right?
> > > maybe what we need for this specific matter is specifying how many
> > > multicast groups are allowed for a VF?
> >
> > I believe the idea behind this interface was that it would allow VF's
> > to request unlimited multicast group as opposed to the current
> > behavior of each adapter offering some limited number.  This limit is
> > of course defined by a given adapters HW/SW limitations.  Up until now
> > you could keep asking for new multicast until the PF replied with an
> > error.  So we never really exported this information before.  This new
> mode just allows us to never reach the point that the PF would deny a VF
> request to join a MC group.  Seems to me that an additional interface to
> provide the max number of supported multicast groups would be new
> functionality that could be independent of this patch and in fact could exist
> even without this patch.
> > Or am I missing what you're asking for here. :)
> 
> I think that the current limitation of multicast on ixgbevf comes from the
> implementation of mailbox API between VF and PF which has 32 words.
> 

In the end the limit on number of MC groups, if you don't use promiscuous mode, is the size of the multicast table array.  We could be sharing this table better between all users rather than the arbitrary limit, but you would hit a hard limit due to the size of the table.

> By the way, our requirement is to make VF promiscuous mode for SDN/NFV
> usage.
> And there is a feature to enable in HW, we'd like to use it.
> I know there is a possibility of performance degradation.
> 
> thanks,
> Hiroshi

I think your method is the way to go, in that if you ask for more than we allow per VF and the PF has this ability enabled we just put the VF into multicast promiscuous mode.  However I don't see the advantage of having an interface to tell how many groups need to be requested before this happens.  If you were worried about the performance degradation of entering promiscuous multicast don't allow it in the PF, which of course will be the default.

Thanks,
-Don Skidmore <donald.c.skidmore@intel.com>

RE: [net-next 07/11] if_link: Add VF multicast promiscuous control

[Hiroshi Shimamoto]

> > -----Original Message-----
> > From: Hiroshi Shimamoto [mailto:h-shimamoto@ct.jp.nec.com]
> > Sent: Wednesday, May 06, 2015 10:55 PM
> > To: Skidmore, Donald C; Or Gerlitz; Kirsher, Jeffrey T
> > Cc: David Miller; Linux Netdev List; nhorman@redhat.com;
> > sassmann@redhat.com; jogreene@redhat.com; Choi, Sy Jong; Edward Cree;
> > Rony Efraim
> > Subject: RE: [net-next 07/11] if_link: Add VF multicast promiscuous control
> >
> > > > -----Original Message-----
> > > > From: Or Gerlitz [mailto:gerlitz.or@gmail.com]
> > > > Sent: Sunday, May 03, 2015 7:16 AM
> > > > To: Kirsher, Jeffrey T
> > > > Cc: David Miller; Hiroshi Shimamoto; Linux Netdev List;
> > > > nhorman@redhat.com; sassmann@redhat.com; jogreene@redhat.com;
> > Choi,
> > > > Sy Jong; Edward Cree; Skidmore, Donald C; Rony Efraim
> > > > Subject: Re: [net-next 07/11] if_link: Add VF multicast promiscuous
> > > > control
> > > >
> > > > On Sat, May 2, 2015 at 1:42 PM, Jeff Kirsher
> > > > <jeffrey.t.kirsher@intel.com>
> > > > wrote:
> > > > > From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
> > > > >
> > > > > Add netlink directives and ndo entry to allow VF multicast
> > > > > promiscuous
> > > > mode.
> > > > >
> > > > > This controls the permission to enter VF multicast promiscuous mode.
> > > > > The administrator will dedicatedly allow multicast promiscuous per VF.
> > > > >
> > > > > When the VF is under multicast promiscuous mode, all multicast
> > > > > packets are sent to the VF.
> > > > >
> > > > > Don't allow VF multicast promiscuous if the VM isn't fully trusted.
> > > >
> > > > Guys,
> > > >
> > > > I don't think the discussion we held in the past [1] on the matter
> > > > actually converged. Few open points that came up while debating it
> > > > internally with
> > > > Rony:
> > > >
> > > > 1. maybe what we we actually want here an API that states a VF to be
> > > > privileged/trusted and then we can over load the feature set of being
> > such?
> > >
> > > I suggested this originally, but there was push back as it was thought too
> > generic as the definition of what being a "trusted"
> > > vendor would differ from driver to driver.  Personally I still like the idea of
> > having one mode saying that we "trust"
> > > a given VF.  Then that VF can request whatever it support it wants
> > > from the PF regardless of possible negative impact on other VF's.
> > > What is possible to support would then be left to the interface
> > > between the VF and PF.  This of course would be dependent on what the
> > given HW could support and would mean this mode would mean different
> > things for different adapters and I do see why some might see this as a
> > concern.
> >
> > The point is granularity, right?
> > Allow everything or allow subset of features.
> 
> Nice way to sum it up.  The trick being with the subset of features path is not all hardware can/will support everything.
> Also I worry about worry about the feature list growing requiring more and more nobs on the PF to allow/disallow granular
> behavior that could brake VF isolation.  With a simple hint to the PF that a given VF is "trusted" would allow all that
> complexity to be contained in the mailbox protocol between the PF/VF.
> 
> All that said I realize others are concerned with the ambiguousness of such a field and can certainly live with your
> implementation.

I see, it seems better to have a single knob which indicates "trust this VF" and PF will allow requests which
might hurt performance or security from trusted VF, instead of creating a knob for multicast promiscuous,
a knob for feature X and so on.

I will make a patch to implement that "trusted knob" instead of allowing MC promiscuous.
Is there any comment?

> 
> >
> > >
> > > >
> > > > 2. the suggested API only allows either unlimited number of mulicast
> > > > groups per VF or limited number, both numbers are vendor dependent,
> > right?
> > > > maybe what we need for this specific matter is specifying how many
> > > > multicast groups are allowed for a VF?
> > >
> > > I believe the idea behind this interface was that it would allow VF's
> > > to request unlimited multicast group as opposed to the current
> > > behavior of each adapter offering some limited number.  This limit is
> > > of course defined by a given adapters HW/SW limitations.  Up until now
> > > you could keep asking for new multicast until the PF replied with an
> > > error.  So we never really exported this information before.  This new
> > mode just allows us to never reach the point that the PF would deny a VF
> > request to join a MC group.  Seems to me that an additional interface to
> > provide the max number of supported multicast groups would be new
> > functionality that could be independent of this patch and in fact could exist
> > even without this patch.
> > > Or am I missing what you're asking for here. :)
> >
> > I think that the current limitation of multicast on ixgbevf comes from the
> > implementation of mailbox API between VF and PF which has 32 words.
> >
> 
> In the end the limit on number of MC groups, if you don't use promiscuous mode, is the size of the multicast table array.
> We could be sharing this table better between all users rather than the arbitrary limit, but you would hit a hard limit
> due to the size of the table.

Just to clarify the current implementation, I think there is no hard limit of MC address.
The ixgbe driver uses the MTA (multicast table array) and the MTA is shared among all VFs. VF requests
to register 30 multicast address hash values at most and PF will set the corresponding bit in the MTA.
When multicast packet comes in, NIC checks the MTA bit and transmit the packet every VF. 82599 uses the
12 bits hash value of MC address and there is the MTA which is 4096 bits array, the MTA covers all MC
address to filter. I think if all bits on in the MTA, it means that no MC packet is dropped.

thanks,
Hiroshi

> 
> > By the way, our requirement is to make VF promiscuous mode for SDN/NFV
> > usage.
> > And there is a feature to enable in HW, we'd like to use it.
> > I know there is a possibility of performance degradation.
> >
> > thanks,
> > Hiroshi
> 
> I think your method is the way to go, in that if you ask for more than we allow per VF and the PF has this ability enabled
> we just put the VF into multicast promiscuous mode.  However I don't see the advantage of having an interface to tell
> how many groups need to be requested before this happens.  If you were worried about the performance degradation of entering
> promiscuous multicast don't allow it in the PF, which of course will be the default.
> 
> Thanks,
> -Don Skidmore <donald.c.skidmore@intel.com>
> 
> 

RE: [net-next 07/11] if_link: Add VF multicast promiscuous control

[Hiroshi Shimamoto]

> > > -----Original Message-----
> > > From: Hiroshi Shimamoto [mailto:h-shimamoto@ct.jp.nec.com]
> > > Sent: Wednesday, May 06, 2015 10:55 PM
> > > To: Skidmore, Donald C; Or Gerlitz; Kirsher, Jeffrey T
> > > Cc: David Miller; Linux Netdev List; nhorman@redhat.com;
> > > sassmann@redhat.com; jogreene@redhat.com; Choi, Sy Jong; Edward Cree;
> > > Rony Efraim
> > > Subject: RE: [net-next 07/11] if_link: Add VF multicast promiscuous control
> > >
> > > > > -----Original Message-----
> > > > > From: Or Gerlitz [mailto:gerlitz.or@gmail.com]
> > > > > Sent: Sunday, May 03, 2015 7:16 AM
> > > > > To: Kirsher, Jeffrey T
> > > > > Cc: David Miller; Hiroshi Shimamoto; Linux Netdev List;
> > > > > nhorman@redhat.com; sassmann@redhat.com; jogreene@redhat.com;
> > > Choi,
> > > > > Sy Jong; Edward Cree; Skidmore, Donald C; Rony Efraim
> > > > > Subject: Re: [net-next 07/11] if_link: Add VF multicast promiscuous
> > > > > control
> > > > >
> > > > > On Sat, May 2, 2015 at 1:42 PM, Jeff Kirsher
> > > > > <jeffrey.t.kirsher@intel.com>
> > > > > wrote:
> > > > > > From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
> > > > > >
> > > > > > Add netlink directives and ndo entry to allow VF multicast
> > > > > > promiscuous
> > > > > mode.
> > > > > >
> > > > > > This controls the permission to enter VF multicast promiscuous mode.
> > > > > > The administrator will dedicatedly allow multicast promiscuous per VF.
> > > > > >
> > > > > > When the VF is under multicast promiscuous mode, all multicast
> > > > > > packets are sent to the VF.
> > > > > >
> > > > > > Don't allow VF multicast promiscuous if the VM isn't fully trusted.
> > > > >
> > > > > Guys,
> > > > >
> > > > > I don't think the discussion we held in the past [1] on the matter
> > > > > actually converged. Few open points that came up while debating it
> > > > > internally with
> > > > > Rony:
> > > > >
> > > > > 1. maybe what we we actually want here an API that states a VF to be
> > > > > privileged/trusted and then we can over load the feature set of being
> > > such?
> > > >
> > > > I suggested this originally, but there was push back as it was thought too
> > > generic as the definition of what being a "trusted"
> > > > vendor would differ from driver to driver.  Personally I still like the idea of
> > > having one mode saying that we "trust"
> > > > a given VF.  Then that VF can request whatever it support it wants
> > > > from the PF regardless of possible negative impact on other VF's.
> > > > What is possible to support would then be left to the interface
> > > > between the VF and PF.  This of course would be dependent on what the
> > > given HW could support and would mean this mode would mean different
> > > things for different adapters and I do see why some might see this as a
> > > concern.
> > >
> > > The point is granularity, right?
> > > Allow everything or allow subset of features.
> >
> > Nice way to sum it up.  The trick being with the subset of features path is not all hardware can/will support everything.
> > Also I worry about worry about the feature list growing requiring more and more nobs on the PF to allow/disallow granular
> > behavior that could brake VF isolation.  With a simple hint to the PF that a given VF is "trusted" would allow all that
> > complexity to be contained in the mailbox protocol between the PF/VF.
> >
> > All that said I realize others are concerned with the ambiguousness of such a field and can certainly live with your
> > implementation.
> 
> I see, it seems better to have a single knob which indicates "trust this VF" and PF will allow requests which
> might hurt performance or security from trusted VF, instead of creating a knob for multicast promiscuous,
> a knob for feature X and so on.
> 
> I will make a patch to implement that "trusted knob" instead of allowing MC promiscuous.
> Is there any comment?

Any comments?
Is that the way to go ahead this series?

thanks,
Hiroshi


> 
> >
> > >
> > > >
> > > > >
> > > > > 2. the suggested API only allows either unlimited number of mulicast
> > > > > groups per VF or limited number, both numbers are vendor dependent,
> > > right?
> > > > > maybe what we need for this specific matter is specifying how many
> > > > > multicast groups are allowed for a VF?
> > > >
> > > > I believe the idea behind this interface was that it would allow VF's
> > > > to request unlimited multicast group as opposed to the current
> > > > behavior of each adapter offering some limited number.  This limit is
> > > > of course defined by a given adapters HW/SW limitations.  Up until now
> > > > you could keep asking for new multicast until the PF replied with an
> > > > error.  So we never really exported this information before.  This new
> > > mode just allows us to never reach the point that the PF would deny a VF
> > > request to join a MC group.  Seems to me that an additional interface to
> > > provide the max number of supported multicast groups would be new
> > > functionality that could be independent of this patch and in fact could exist
> > > even without this patch.
> > > > Or am I missing what you're asking for here. :)
> > >
> > > I think that the current limitation of multicast on ixgbevf comes from the
> > > implementation of mailbox API between VF and PF which has 32 words.
> > >
> >
> > In the end the limit on number of MC groups, if you don't use promiscuous mode, is the size of the multicast table array.
> > We could be sharing this table better between all users rather than the arbitrary limit, but you would hit a hard limit
> > due to the size of the table.
> 
> Just to clarify the current implementation, I think there is no hard limit of MC address.
> The ixgbe driver uses the MTA (multicast table array) and the MTA is shared among all VFs. VF requests
> to register 30 multicast address hash values at most and PF will set the corresponding bit in the MTA.
> When multicast packet comes in, NIC checks the MTA bit and transmit the packet every VF. 82599 uses the
> 12 bits hash value of MC address and there is the MTA which is 4096 bits array, the MTA covers all MC
> address to filter. I think if all bits on in the MTA, it means that no MC packet is dropped.
> 
> thanks,
> Hiroshi
> 
> >
> > > By the way, our requirement is to make VF promiscuous mode for SDN/NFV
> > > usage.
> > > And there is a feature to enable in HW, we'd like to use it.
> > > I know there is a possibility of performance degradation.
> > >
> > > thanks,
> > > Hiroshi
> >
> > I think your method is the way to go, in that if you ask for more than we allow per VF and the PF has this ability enabled
> > we just put the VF into multicast promiscuous mode.  However I don't see the advantage of having an interface to tell
> > how many groups need to be requested before this happens.  If you were worried about the performance degradation of
> entering
> > promiscuous multicast don't allow it in the PF, which of course will be the default.
> >
> > Thanks,
> > -Don Skidmore <donald.c.skidmore@intel.com>
> >
> >

RE: [net-next 07/11] if_link: Add VF multicast promiscuous control

[Skidmore, Donald C]

> -----Original Message-----
> From: Hiroshi Shimamoto [mailto:h-shimamoto@ct.jp.nec.com]
> Sent: Monday, May 11, 2015 4:55 PM
> To: Skidmore, Donald C; Or Gerlitz; Kirsher, Jeffrey T
> Cc: David Miller; Linux Netdev List; nhorman@redhat.com;
> sassmann@redhat.com; jogreene@redhat.com; Choi, Sy Jong; Edward Cree;
> Rony Efraim
> Subject: RE: [net-next 07/11] if_link: Add VF multicast promiscuous control
> 
> > > > -----Original Message-----
> > > > From: Hiroshi Shimamoto [mailto:h-shimamoto@ct.jp.nec.com]
> > > > Sent: Wednesday, May 06, 2015 10:55 PM
> > > > To: Skidmore, Donald C; Or Gerlitz; Kirsher, Jeffrey T
> > > > Cc: David Miller; Linux Netdev List; nhorman@redhat.com;
> > > > sassmann@redhat.com; jogreene@redhat.com; Choi, Sy Jong; Edward
> > > > Cree; Rony Efraim
> > > > Subject: RE: [net-next 07/11] if_link: Add VF multicast
> > > > promiscuous control
> > > >
> > > > > > -----Original Message-----
> > > > > > From: Or Gerlitz [mailto:gerlitz.or@gmail.com]
> > > > > > Sent: Sunday, May 03, 2015 7:16 AM
> > > > > > To: Kirsher, Jeffrey T
> > > > > > Cc: David Miller; Hiroshi Shimamoto; Linux Netdev List;
> > > > > > nhorman@redhat.com; sassmann@redhat.com;
> jogreene@redhat.com;
> > > > Choi,
> > > > > > Sy Jong; Edward Cree; Skidmore, Donald C; Rony Efraim
> > > > > > Subject: Re: [net-next 07/11] if_link: Add VF multicast
> > > > > > promiscuous control
> > > > > >
> > > > > > On Sat, May 2, 2015 at 1:42 PM, Jeff Kirsher
> > > > > > <jeffrey.t.kirsher@intel.com>
> > > > > > wrote:
> > > > > > > From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
> > > > > > >
> > > > > > > Add netlink directives and ndo entry to allow VF multicast
> > > > > > > promiscuous
> > > > > > mode.
> > > > > > >
> > > > > > > This controls the permission to enter VF multicast promiscuous
> mode.
> > > > > > > The administrator will dedicatedly allow multicast promiscuous per
> VF.
> > > > > > >
> > > > > > > When the VF is under multicast promiscuous mode, all
> > > > > > > multicast packets are sent to the VF.
> > > > > > >
> > > > > > > Don't allow VF multicast promiscuous if the VM isn't fully trusted.
> > > > > >
> > > > > > Guys,
> > > > > >
> > > > > > I don't think the discussion we held in the past [1] on the
> > > > > > matter actually converged. Few open points that came up while
> > > > > > debating it internally with
> > > > > > Rony:
> > > > > >
> > > > > > 1. maybe what we we actually want here an API that states a VF
> > > > > > to be privileged/trusted and then we can over load the feature
> > > > > > set of being
> > > > such?
> > > > >
> > > > > I suggested this originally, but there was push back as it was
> > > > > thought too
> > > > generic as the definition of what being a "trusted"
> > > > > vendor would differ from driver to driver.  Personally I still
> > > > > like the idea of
> > > > having one mode saying that we "trust"
> > > > > a given VF.  Then that VF can request whatever it support it
> > > > > wants from the PF regardless of possible negative impact on other
> VF's.
> > > > > What is possible to support would then be left to the interface
> > > > > between the VF and PF.  This of course would be dependent on
> > > > > what the
> > > > given HW could support and would mean this mode would mean
> > > > different things for different adapters and I do see why some
> > > > might see this as a concern.
> > > >
> > > > The point is granularity, right?
> > > > Allow everything or allow subset of features.
> > >
> > > Nice way to sum it up.  The trick being with the subset of features path is
> not all hardware can/will support everything.
> > > Also I worry about worry about the feature list growing requiring
> > > more and more nobs on the PF to allow/disallow granular behavior
> > > that could brake VF isolation.  With a simple hint to the PF that a given VF
> is "trusted" would allow all that complexity to be contained in the mailbox
> protocol between the PF/VF.
> > >
> > > All that said I realize others are concerned with the ambiguousness
> > > of such a field and can certainly live with your implementation.
> >
> > I see, it seems better to have a single knob which indicates "trust
> > this VF" and PF will allow requests which might hurt performance or
> > security from trusted VF, instead of creating a knob for multicast
> promiscuous, a knob for feature X and so on.
> >
> > I will make a patch to implement that "trusted knob" instead of allowing
> MC promiscuous.
> > Is there any comment?
> 
> Any comments?
> Is that the way to go ahead this series?
> 
> thanks,
> Hiroshi

Clearly I am ok with the idea.  :) 

If the change isn't too difficult to implement, maybe just submit the path.  That will most likely get more attention.

Thanks,
-Don <donald.c.skidmore@intel.com>

> 
> 
> >
> > >
> > > >
> > > > >
> > > > > >
> > > > > > 2. the suggested API only allows either unlimited number of
> > > > > > mulicast groups per VF or limited number, both numbers are
> > > > > > vendor dependent,
> > > > right?
> > > > > > maybe what we need for this specific matter is specifying how
> > > > > > many multicast groups are allowed for a VF?
> > > > >
> > > > > I believe the idea behind this interface was that it would allow
> > > > > VF's to request unlimited multicast group as opposed to the
> > > > > current behavior of each adapter offering some limited number.
> > > > > This limit is of course defined by a given adapters HW/SW
> > > > > limitations.  Up until now you could keep asking for new
> > > > > multicast until the PF replied with an error.  So we never
> > > > > really exported this information before.  This new
> > > > mode just allows us to never reach the point that the PF would
> > > > deny a VF request to join a MC group.  Seems to me that an
> > > > additional interface to provide the max number of supported
> > > > multicast groups would be new functionality that could be
> > > > independent of this patch and in fact could exist even without this
> patch.
> > > > > Or am I missing what you're asking for here. :)
> > > >
> > > > I think that the current limitation of multicast on ixgbevf comes
> > > > from the implementation of mailbox API between VF and PF which has
> 32 words.
> > > >
> > >
> > > In the end the limit on number of MC groups, if you don't use
> promiscuous mode, is the size of the multicast table array.
> > > We could be sharing this table better between all users rather than
> > > the arbitrary limit, but you would hit a hard limit due to the size of the
> table.
> >
> > Just to clarify the current implementation, I think there is no hard limit of
> MC address.
> > The ixgbe driver uses the MTA (multicast table array) and the MTA is
> > shared among all VFs. VF requests to register 30 multicast address hash
> values at most and PF will set the corresponding bit in the MTA.
> > When multicast packet comes in, NIC checks the MTA bit and transmit
> > the packet every VF. 82599 uses the
> > 12 bits hash value of MC address and there is the MTA which is 4096
> > bits array, the MTA covers all MC address to filter. I think if all bits on in the
> MTA, it means that no MC packet is dropped.
> >
> > thanks,
> > Hiroshi
> >
> > >
> > > > By the way, our requirement is to make VF promiscuous mode for
> > > > SDN/NFV usage.
> > > > And there is a feature to enable in HW, we'd like to use it.
> > > > I know there is a possibility of performance degradation.
> > > >
> > > > thanks,
> > > > Hiroshi
> > >
> > > I think your method is the way to go, in that if you ask for more
> > > than we allow per VF and the PF has this ability enabled we just put
> > > the VF into multicast promiscuous mode.  However I don't see the
> > > advantage of having an interface to tell how many groups need to be
> > > requested before this happens.  If you were worried about the
> > > performance degradation of
> > entering
> > > promiscuous multicast don't allow it in the PF, which of course will be the
> default.
> > >
> > > Thanks,
> > > -Don Skidmore <donald.c.skidmore@intel.com>
> > >
> > >

RE: [net-next 07/11] if_link: Add VF multicast promiscuous control

[Hiroshi Shimamoto]

> > -----Original Message-----
> > From: Hiroshi Shimamoto [mailto:h-shimamoto@ct.jp.nec.com]
> > Sent: Monday, May 11, 2015 4:55 PM
> > To: Skidmore, Donald C; Or Gerlitz; Kirsher, Jeffrey T
> > Cc: David Miller; Linux Netdev List; nhorman@redhat.com;
> > sassmann@redhat.com; jogreene@redhat.com; Choi, Sy Jong; Edward Cree;
> > Rony Efraim
> > Subject: RE: [net-next 07/11] if_link: Add VF multicast promiscuous control
> >
> > > > > -----Original Message-----
> > > > > From: Hiroshi Shimamoto [mailto:h-shimamoto@ct.jp.nec.com]
> > > > > Sent: Wednesday, May 06, 2015 10:55 PM
> > > > > To: Skidmore, Donald C; Or Gerlitz; Kirsher, Jeffrey T
> > > > > Cc: David Miller; Linux Netdev List; nhorman@redhat.com;
> > > > > sassmann@redhat.com; jogreene@redhat.com; Choi, Sy Jong; Edward
> > > > > Cree; Rony Efraim
> > > > > Subject: RE: [net-next 07/11] if_link: Add VF multicast
> > > > > promiscuous control
> > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Or Gerlitz [mailto:gerlitz.or@gmail.com]
> > > > > > > Sent: Sunday, May 03, 2015 7:16 AM
> > > > > > > To: Kirsher, Jeffrey T
> > > > > > > Cc: David Miller; Hiroshi Shimamoto; Linux Netdev List;
> > > > > > > nhorman@redhat.com; sassmann@redhat.com;
> > jogreene@redhat.com;
> > > > > Choi,
> > > > > > > Sy Jong; Edward Cree; Skidmore, Donald C; Rony Efraim
> > > > > > > Subject: Re: [net-next 07/11] if_link: Add VF multicast
> > > > > > > promiscuous control
> > > > > > >
> > > > > > > On Sat, May 2, 2015 at 1:42 PM, Jeff Kirsher
> > > > > > > <jeffrey.t.kirsher@intel.com>
> > > > > > > wrote:
> > > > > > > > From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
> > > > > > > >
> > > > > > > > Add netlink directives and ndo entry to allow VF multicast
> > > > > > > > promiscuous
> > > > > > > mode.
> > > > > > > >
> > > > > > > > This controls the permission to enter VF multicast promiscuous
> > mode.
> > > > > > > > The administrator will dedicatedly allow multicast promiscuous per
> > VF.
> > > > > > > >
> > > > > > > > When the VF is under multicast promiscuous mode, all
> > > > > > > > multicast packets are sent to the VF.
> > > > > > > >
> > > > > > > > Don't allow VF multicast promiscuous if the VM isn't fully trusted.
> > > > > > >
> > > > > > > Guys,
> > > > > > >
> > > > > > > I don't think the discussion we held in the past [1] on the
> > > > > > > matter actually converged. Few open points that came up while
> > > > > > > debating it internally with
> > > > > > > Rony:
> > > > > > >
> > > > > > > 1. maybe what we we actually want here an API that states a VF
> > > > > > > to be privileged/trusted and then we can over load the feature
> > > > > > > set of being
> > > > > such?
> > > > > >
> > > > > > I suggested this originally, but there was push back as it was
> > > > > > thought too
> > > > > generic as the definition of what being a "trusted"
> > > > > > vendor would differ from driver to driver.  Personally I still
> > > > > > like the idea of
> > > > > having one mode saying that we "trust"
> > > > > > a given VF.  Then that VF can request whatever it support it
> > > > > > wants from the PF regardless of possible negative impact on other
> > VF's.
> > > > > > What is possible to support would then be left to the interface
> > > > > > between the VF and PF.  This of course would be dependent on
> > > > > > what the
> > > > > given HW could support and would mean this mode would mean
> > > > > different things for different adapters and I do see why some
> > > > > might see this as a concern.
> > > > >
> > > > > The point is granularity, right?
> > > > > Allow everything or allow subset of features.
> > > >
> > > > Nice way to sum it up.  The trick being with the subset of features path is
> > not all hardware can/will support everything.
> > > > Also I worry about worry about the feature list growing requiring
> > > > more and more nobs on the PF to allow/disallow granular behavior
> > > > that could brake VF isolation.  With a simple hint to the PF that a given VF
> > is "trusted" would allow all that complexity to be contained in the mailbox
> > protocol between the PF/VF.
> > > >
> > > > All that said I realize others are concerned with the ambiguousness
> > > > of such a field and can certainly live with your implementation.
> > >
> > > I see, it seems better to have a single knob which indicates "trust
> > > this VF" and PF will allow requests which might hurt performance or
> > > security from trusted VF, instead of creating a knob for multicast
> > promiscuous, a knob for feature X and so on.
> > >
> > > I will make a patch to implement that "trusted knob" instead of allowing
> > MC promiscuous.
> > > Is there any comment?
> >
> > Any comments?
> > Is that the way to go ahead this series?
> >
> > thanks,
> > Hiroshi
> 
> Clearly I am ok with the idea.  :)
> 
> If the change isn't too difficult to implement, maybe just submit the path.  That will most likely get more attention.

okay, I'll submit a new patches.
BTW, which tree should I make patches against for?
Because Jeff's tree still have previous patches.

thanks,
Hiroshi

> 
> Thanks,
> -Don <donald.c.skidmore@intel.com>
> 
> >
> >
> > >
> > > >
> > > > >
> > > > > >
> > > > > > >
> > > > > > > 2. the suggested API only allows either unlimited number of
> > > > > > > mulicast groups per VF or limited number, both numbers are
> > > > > > > vendor dependent,
> > > > > right?
> > > > > > > maybe what we need for this specific matter is specifying how
> > > > > > > many multicast groups are allowed for a VF?
> > > > > >
> > > > > > I believe the idea behind this interface was that it would allow
> > > > > > VF's to request unlimited multicast group as opposed to the
> > > > > > current behavior of each adapter offering some limited number.
> > > > > > This limit is of course defined by a given adapters HW/SW
> > > > > > limitations.  Up until now you could keep asking for new
> > > > > > multicast until the PF replied with an error.  So we never
> > > > > > really exported this information before.  This new
> > > > > mode just allows us to never reach the point that the PF would
> > > > > deny a VF request to join a MC group.  Seems to me that an
> > > > > additional interface to provide the max number of supported
> > > > > multicast groups would be new functionality that could be
> > > > > independent of this patch and in fact could exist even without this
> > patch.
> > > > > > Or am I missing what you're asking for here. :)
> > > > >
> > > > > I think that the current limitation of multicast on ixgbevf comes
> > > > > from the implementation of mailbox API between VF and PF which has
> > 32 words.
> > > > >
> > > >
> > > > In the end the limit on number of MC groups, if you don't use
> > promiscuous mode, is the size of the multicast table array.
> > > > We could be sharing this table better between all users rather than
> > > > the arbitrary limit, but you would hit a hard limit due to the size of the
> > table.
> > >
> > > Just to clarify the current implementation, I think there is no hard limit of
> > MC address.
> > > The ixgbe driver uses the MTA (multicast table array) and the MTA is
> > > shared among all VFs. VF requests to register 30 multicast address hash
> > values at most and PF will set the corresponding bit in the MTA.
> > > When multicast packet comes in, NIC checks the MTA bit and transmit
> > > the packet every VF. 82599 uses the
> > > 12 bits hash value of MC address and there is the MTA which is 4096
> > > bits array, the MTA covers all MC address to filter. I think if all bits on in the
> > MTA, it means that no MC packet is dropped.
> > >
> > > thanks,
> > > Hiroshi
> > >
> > > >
> > > > > By the way, our requirement is to make VF promiscuous mode for
> > > > > SDN/NFV usage.
> > > > > And there is a feature to enable in HW, we'd like to use it.
> > > > > I know there is a possibility of performance degradation.
> > > > >
> > > > > thanks,
> > > > > Hiroshi
> > > >
> > > > I think your method is the way to go, in that if you ask for more
> > > > than we allow per VF and the PF has this ability enabled we just put
> > > > the VF into multicast promiscuous mode.  However I don't see the
> > > > advantage of having an interface to tell how many groups need to be
> > > > requested before this happens.  If you were worried about the
> > > > performance degradation of
> > > entering
> > > > promiscuous multicast don't allow it in the PF, which of course will be the
> > default.
> > > >
> > > > Thanks,
> > > > -Don Skidmore <donald.c.skidmore@intel.com>
> > > >
> > > >

Re: [net-next 07/11] if_link: Add VF multicast promiscuous control

[Jeff Kirsher]

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

On Tue, 2015-05-12 at 00:33 +0000, Hiroshi Shimamoto wrote:
> > > -----Original Message-----
> > > From: Hiroshi Shimamoto [mailto:h-shimamoto@ct.jp.nec.com]
> > > Sent: Monday, May 11, 2015 4:55 PM
> > > To: Skidmore, Donald C; Or Gerlitz; Kirsher, Jeffrey T
> > > Cc: David Miller; Linux Netdev List; nhorman@redhat.com;
> > > sassmann@redhat.com; jogreene@redhat.com; Choi, Sy Jong; Edward
> Cree;
> > > Rony Efraim
> > > Subject: RE: [net-next 07/11] if_link: Add VF multicast
> promiscuous control
> > >
> > > > > > -----Original Message-----
> > > > > > From: Hiroshi Shimamoto [mailto:h-shimamoto@ct.jp.nec.com]
> > > > > > Sent: Wednesday, May 06, 2015 10:55 PM
> > > > > > To: Skidmore, Donald C; Or Gerlitz; Kirsher, Jeffrey T
> > > > > > Cc: David Miller; Linux Netdev List; nhorman@redhat.com;
> > > > > > sassmann@redhat.com; jogreene@redhat.com; Choi, Sy Jong;
> Edward
> > > > > > Cree; Rony Efraim
> > > > > > Subject: RE: [net-next 07/11] if_link: Add VF multicast
> > > > > > promiscuous control
> > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Or Gerlitz [mailto:gerlitz.or@gmail.com]
> > > > > > > > Sent: Sunday, May 03, 2015 7:16 AM
> > > > > > > > To: Kirsher, Jeffrey T
> > > > > > > > Cc: David Miller; Hiroshi Shimamoto; Linux Netdev List;
> > > > > > > > nhorman@redhat.com; sassmann@redhat.com;
> > > jogreene@redhat.com;
> > > > > > Choi,
> > > > > > > > Sy Jong; Edward Cree; Skidmore, Donald C; Rony Efraim
> > > > > > > > Subject: Re: [net-next 07/11] if_link: Add VF multicast
> > > > > > > > promiscuous control
> > > > > > > >
> > > > > > > > On Sat, May 2, 2015 at 1:42 PM, Jeff Kirsher
> > > > > > > > <jeffrey.t.kirsher@intel.com>
> > > > > > > > wrote:
> > > > > > > > > From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
> > > > > > > > >
> > > > > > > > > Add netlink directives and ndo entry to allow VF
> multicast
> > > > > > > > > promiscuous
> > > > > > > > mode.
> > > > > > > > >
> > > > > > > > > This controls the permission to enter VF multicast
> promiscuous
> > > mode.
> > > > > > > > > The administrator will dedicatedly allow multicast
> promiscuous per
> > > VF.
> > > > > > > > >
> > > > > > > > > When the VF is under multicast promiscuous mode, all
> > > > > > > > > multicast packets are sent to the VF.
> > > > > > > > >
> > > > > > > > > Don't allow VF multicast promiscuous if the VM isn't
> fully trusted.
> > > > > > > >
> > > > > > > > Guys,
> > > > > > > >
> > > > > > > > I don't think the discussion we held in the past [1] on
> the
> > > > > > > > matter actually converged. Few open points that came up
> while
> > > > > > > > debating it internally with
> > > > > > > > Rony:
> > > > > > > >
> > > > > > > > 1. maybe what we we actually want here an API that
> states a VF
> > > > > > > > to be privileged/trusted and then we can over load the
> feature
> > > > > > > > set of being
> > > > > > such?
> > > > > > >
> > > > > > > I suggested this originally, but there was push back as it
> was
> > > > > > > thought too
> > > > > > generic as the definition of what being a "trusted"
> > > > > > > vendor would differ from driver to driver.  Personally I
> still
> > > > > > > like the idea of
> > > > > > having one mode saying that we "trust"
> > > > > > > a given VF.  Then that VF can request whatever it support
> it
> > > > > > > wants from the PF regardless of possible negative impact
> on other
> > > VF's.
> > > > > > > What is possible to support would then be left to the
> interface
> > > > > > > between the VF and PF.  This of course would be dependent
> on
> > > > > > > what the
> > > > > > given HW could support and would mean this mode would mean
> > > > > > different things for different adapters and I do see why
> some
> > > > > > might see this as a concern.
> > > > > >
> > > > > > The point is granularity, right?
> > > > > > Allow everything or allow subset of features.
> > > > >
> > > > > Nice way to sum it up.  The trick being with the subset of
> features path is
> > > not all hardware can/will support everything.
> > > > > Also I worry about worry about the feature list growing
> requiring
> > > > > more and more nobs on the PF to allow/disallow granular
> behavior
> > > > > that could brake VF isolation.  With a simple hint to the PF
> that a given VF
> > > is "trusted" would allow all that complexity to be contained in
> the mailbox
> > > protocol between the PF/VF.
> > > > >
> > > > > All that said I realize others are concerned with the
> ambiguousness
> > > > > of such a field and can certainly live with your
> implementation.
> > > >
> > > > I see, it seems better to have a single knob which indicates
> "trust
> > > > this VF" and PF will allow requests which might hurt performance
> or
> > > > security from trusted VF, instead of creating a knob for
> multicast
> > > promiscuous, a knob for feature X and so on.
> > > >
> > > > I will make a patch to implement that "trusted knob" instead of
> allowing
> > > MC promiscuous.
> > > > Is there any comment?
> > >
> > > Any comments?
> > > Is that the way to go ahead this series?
> > >
> > > thanks,
> > > Hiroshi
> > 
> > Clearly I am ok with the idea.  :)
> > 
> > If the change isn't too difficult to implement, maybe just submit
> the path.  That will most likely get more attention.
> 
> okay, I'll submit a new patches.
> BTW, which tree should I make patches against for?
> Because Jeff's tree still have previous patches.

So sorry for the delay, been dealing with some health issues.
I have removed your previous series of patches from my next-queue tree,
so you can re-spin your patches based on the dev-queue branch.

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

RE: [net-next 07/11] if_link: Add VF multicast promiscuous control

[Hiroshi Shimamoto]

> Subject: Re: [net-next 07/11] if_link: Add VF multicast promiscuous control
> 
> On Tue, 2015-05-12 at 00:33 +0000, Hiroshi Shimamoto wrote:
> > > > -----Original Message-----
> > > > From: Hiroshi Shimamoto [mailto:h-shimamoto@ct.jp.nec.com]
> > > > Sent: Monday, May 11, 2015 4:55 PM
> > > > To: Skidmore, Donald C; Or Gerlitz; Kirsher, Jeffrey T
> > > > Cc: David Miller; Linux Netdev List; nhorman@redhat.com;
> > > > sassmann@redhat.com; jogreene@redhat.com; Choi, Sy Jong; Edward
> > Cree;
> > > > Rony Efraim
> > > > Subject: RE: [net-next 07/11] if_link: Add VF multicast
> > promiscuous control
> > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Hiroshi Shimamoto [mailto:h-shimamoto@ct.jp.nec.com]
> > > > > > > Sent: Wednesday, May 06, 2015 10:55 PM
> > > > > > > To: Skidmore, Donald C; Or Gerlitz; Kirsher, Jeffrey T
> > > > > > > Cc: David Miller; Linux Netdev List; nhorman@redhat.com;
> > > > > > > sassmann@redhat.com; jogreene@redhat.com; Choi, Sy Jong;
> > Edward
> > > > > > > Cree; Rony Efraim
> > > > > > > Subject: RE: [net-next 07/11] if_link: Add VF multicast
> > > > > > > promiscuous control
> > > > > > >
> > > > > > > > > -----Original Message-----
> > > > > > > > > From: Or Gerlitz [mailto:gerlitz.or@gmail.com]
> > > > > > > > > Sent: Sunday, May 03, 2015 7:16 AM
> > > > > > > > > To: Kirsher, Jeffrey T
> > > > > > > > > Cc: David Miller; Hiroshi Shimamoto; Linux Netdev List;
> > > > > > > > > nhorman@redhat.com; sassmann@redhat.com;
> > > > jogreene@redhat.com;
> > > > > > > Choi,
> > > > > > > > > Sy Jong; Edward Cree; Skidmore, Donald C; Rony Efraim
> > > > > > > > > Subject: Re: [net-next 07/11] if_link: Add VF multicast
> > > > > > > > > promiscuous control
> > > > > > > > >
> > > > > > > > > On Sat, May 2, 2015 at 1:42 PM, Jeff Kirsher
> > > > > > > > > <jeffrey.t.kirsher@intel.com>
> > > > > > > > > wrote:
> > > > > > > > > > From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
> > > > > > > > > >
> > > > > > > > > > Add netlink directives and ndo entry to allow VF
> > multicast
> > > > > > > > > > promiscuous
> > > > > > > > > mode.
> > > > > > > > > >
> > > > > > > > > > This controls the permission to enter VF multicast
> > promiscuous
> > > > mode.
> > > > > > > > > > The administrator will dedicatedly allow multicast
> > promiscuous per
> > > > VF.
> > > > > > > > > >
> > > > > > > > > > When the VF is under multicast promiscuous mode, all
> > > > > > > > > > multicast packets are sent to the VF.
> > > > > > > > > >
> > > > > > > > > > Don't allow VF multicast promiscuous if the VM isn't
> > fully trusted.
> > > > > > > > >
> > > > > > > > > Guys,
> > > > > > > > >
> > > > > > > > > I don't think the discussion we held in the past [1] on
> > the
> > > > > > > > > matter actually converged. Few open points that came up
> > while
> > > > > > > > > debating it internally with
> > > > > > > > > Rony:
> > > > > > > > >
> > > > > > > > > 1. maybe what we we actually want here an API that
> > states a VF
> > > > > > > > > to be privileged/trusted and then we can over load the
> > feature
> > > > > > > > > set of being
> > > > > > > such?
> > > > > > > >
> > > > > > > > I suggested this originally, but there was push back as it
> > was
> > > > > > > > thought too
> > > > > > > generic as the definition of what being a "trusted"
> > > > > > > > vendor would differ from driver to driver.  Personally I
> > still
> > > > > > > > like the idea of
> > > > > > > having one mode saying that we "trust"
> > > > > > > > a given VF.  Then that VF can request whatever it support
> > it
> > > > > > > > wants from the PF regardless of possible negative impact
> > on other
> > > > VF's.
> > > > > > > > What is possible to support would then be left to the
> > interface
> > > > > > > > between the VF and PF.  This of course would be dependent
> > on
> > > > > > > > what the
> > > > > > > given HW could support and would mean this mode would mean
> > > > > > > different things for different adapters and I do see why
> > some
> > > > > > > might see this as a concern.
> > > > > > >
> > > > > > > The point is granularity, right?
> > > > > > > Allow everything or allow subset of features.
> > > > > >
> > > > > > Nice way to sum it up.  The trick being with the subset of
> > features path is
> > > > not all hardware can/will support everything.
> > > > > > Also I worry about worry about the feature list growing
> > requiring
> > > > > > more and more nobs on the PF to allow/disallow granular
> > behavior
> > > > > > that could brake VF isolation.  With a simple hint to the PF
> > that a given VF
> > > > is "trusted" would allow all that complexity to be contained in
> > the mailbox
> > > > protocol between the PF/VF.
> > > > > >
> > > > > > All that said I realize others are concerned with the
> > ambiguousness
> > > > > > of such a field and can certainly live with your
> > implementation.
> > > > >
> > > > > I see, it seems better to have a single knob which indicates
> > "trust
> > > > > this VF" and PF will allow requests which might hurt performance
> > or
> > > > > security from trusted VF, instead of creating a knob for
> > multicast
> > > > promiscuous, a knob for feature X and so on.
> > > > >
> > > > > I will make a patch to implement that "trusted knob" instead of
> > allowing
> > > > MC promiscuous.
> > > > > Is there any comment?
> > > >
> > > > Any comments?
> > > > Is that the way to go ahead this series?
> > > >
> > > > thanks,
> > > > Hiroshi
> > >
> > > Clearly I am ok with the idea.  :)
> > >
> > > If the change isn't too difficult to implement, maybe just submit
> > the path.  That will most likely get more attention.
> >
> > okay, I'll submit a new patches.
> > BTW, which tree should I make patches against for?
> > Because Jeff's tree still have previous patches.
> 
> So sorry for the delay, been dealing with some health issues.
> I have removed your previous series of patches from my next-queue tree,
> so you can re-spin your patches based on the dev-queue branch.

thanks, I will submit patches against that branch.