[RFC PATCH 1/4] net: Add support to netdev ops for changing hardware queue MAC and VLAN filters

[RFC PATCH 1/4] net: Add support to netdev ops for changing hardware queue MAC and VLAN filters

[Jeff Kirsher]

From: Williams, Mitch A <mitch.a.williams@intel.com>

Signed-off-by: Mitch Williams <mitch.a.williams@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 include/linux/netdevice.h |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 7043f85..6a70365 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -610,6 +610,8 @@ struct netdev_queue {
  *	this function is called when a VLAN id is unregistered.
  *
  * void (*ndo_poll_controller)(struct net_device *dev);
+ * int (*ndo_set_queue_mac)(struct net_device *dev, int queue, u8* mac);
+ * int (*ndo_set_queue_vlan)(struct net_device *dev, int queue, u16 vlan);
  */
 #define HAVE_NET_DEVICE_OPS
 struct net_device_ops {
@@ -659,6 +661,10 @@ struct net_device_ops {
 #define HAVE_NETDEV_POLL
 	void                    (*ndo_poll_controller)(struct net_device *dev);
 #endif
+	int			(*ndo_set_queue_mac)(struct net_device *dev,
+						     int queue, u8 *mac);
+	int			(*ndo_set_queue_vlan)(struct net_device *dev,
+						      int queue, u16 vlan);
 #if defined(CONFIG_FCOE) || defined(CONFIG_FCOE_MODULE)
 	int			(*ndo_fcoe_enable)(struct net_device *dev);
 	int			(*ndo_fcoe_disable)(struct net_device *dev);

[RFC PATCH 2/4] rtnetlink: Add support to rtnetlink for setting hardware queue MAC and VLAN filters

[Jeff Kirsher]

From: Williams, Mitch A <mitch.a.williams@intel.com>

Signed-off-by: Mitch Williams <mitch.a.williams@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 include/linux/if_link.h |   14 ++++++++++++++
 net/core/rtnetlink.c    |   27 +++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 0 deletions(-)

diff --git a/include/linux/if_link.h b/include/linux/if_link.h
index 1d3b242..44d3726 100644
--- a/include/linux/if_link.h
+++ b/include/linux/if_link.h
@@ -78,6 +78,8 @@ enum {
 #define IFLA_LINKINFO IFLA_LINKINFO
 	IFLA_NET_NS_PID,
 	IFLA_IFALIAS,
+	IFLA_QUEUE_MAC,		/* Hardware queue specific attributes */
+	IFLA_QUEUE_VLAN,
 	__IFLA_MAX
 };
 
@@ -181,4 +183,16 @@ struct ifla_vlan_qos_mapping {
 	__u32 to;
 };
 
+/* subqueue managment section */
+
+struct ifla_queue_mac {
+	__u32 queue;
+	__u8 mac[32];	/* MAX_ADDR_LEN */
+};
+
+struct ifla_queue_vlan {
+	__u32 queue;
+	__u32 vlan;
+};
+
 #endif /* _LINUX_IF_LINK_H */
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 33148a5..ad65683 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -725,6 +725,10 @@ const struct nla_policy ifla_policy[IFLA_MAX+1] = {
 	[IFLA_LINKINFO]		= { .type = NLA_NESTED },
 	[IFLA_NET_NS_PID]	= { .type = NLA_U32 },
 	[IFLA_IFALIAS]	        = { .type = NLA_STRING, .len = IFALIASZ-1 },
+	[IFLA_QUEUE_MAC]	= { .type = NLA_BINARY,
+				    .len = sizeof(struct ifla_queue_mac) },
+	[IFLA_QUEUE_VLAN]	= { .type = NLA_BINARY,
+				    .len = sizeof(struct ifla_queue_vlan) },
 };
 EXPORT_SYMBOL(ifla_policy);
 
@@ -898,6 +902,29 @@ static int do_setlink(struct net_device *dev, struct ifinfomsg *ifm,
 		write_unlock_bh(&dev_base_lock);
 	}
 
+	if (tb[IFLA_QUEUE_MAC]) {
+		struct ifla_queue_mac *iqm;
+		iqm = nla_data(tb[IFLA_QUEUE_MAC]);
+		write_lock_bh(&dev_base_lock);
+		if (ops->ndo_set_queue_mac)
+			err = ops->ndo_set_queue_mac(dev, iqm->queue, iqm->mac);
+		write_unlock_bh(&dev_base_lock);
+		if (err < 0)
+			goto errout;
+		modified = 1;
+	}
+
+	if (tb[IFLA_QUEUE_VLAN]) {
+		struct ifla_queue_vlan *iqv;
+		iqv = nla_data(tb[IFLA_QUEUE_VLAN]);
+		write_lock_bh(&dev_base_lock);
+		if (ops->ndo_set_queue_vlan)
+			ops->ndo_set_queue_vlan(dev, iqv->queue, iqv->vlan);
+		write_unlock_bh(&dev_base_lock);
+		if (err < 0)
+			goto errout;
+		modified = 1;
+	}
 	err = 0;
 
 errout:

[RFC PATCH 3/4] igb: Add support to igb for setting MAC and VLAN filters to hardware queues

[Jeff Kirsher]

From: Williams, Mitch A <mitch.a.williams@intel.com>

This is currently only supported in SR-IOV mode.

Signed-off-by: Mitch Williams <mitch.a.williams@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 drivers/net/igb/e1000_82575.h |    3 +
 drivers/net/igb/e1000_regs.h  |    1 
 drivers/net/igb/igb.h         |    2 +
 drivers/net/igb/igb_main.c    |   90 ++++++++++++++++++++++++++++++++++++-----
 4 files changed, 86 insertions(+), 10 deletions(-)

diff --git a/drivers/net/igb/e1000_82575.h b/drivers/net/igb/e1000_82575.h
index b3808ca..c2d2b33 100644
--- a/drivers/net/igb/e1000_82575.h
+++ b/drivers/net/igb/e1000_82575.h
@@ -214,6 +214,9 @@ struct e1000_adv_tx_context_desc {
 #define E1000_VLVF_LVLAN          0x00100000
 #define E1000_VLVF_VLANID_ENABLE  0x80000000
 
+#define E1000_VMVIR_VLANA_DEFAULT	0x40000000 /* Always use default VLAN */
+#define E1000_VMVIR_VLANA_NEVER		0x80000000 /* Never insert VLAN tag */
+
 #define E1000_IOVCTL 0x05BBC
 #define E1000_IOVCTL_REUSE_VFQ 0x00000001
 
diff --git a/drivers/net/igb/e1000_regs.h b/drivers/net/igb/e1000_regs.h
index 934e03b..42e8528 100644
--- a/drivers/net/igb/e1000_regs.h
+++ b/drivers/net/igb/e1000_regs.h
@@ -307,6 +307,7 @@
 #define E1000_VMOLR(_n)        (0x05AD0 + (4 * (_n)))
 #define E1000_VLVF(_n)         (0x05D00 + (4 * (_n))) /* VLAN Virtual Machine
                                                        * Filter - RW */
+#define E1000_VMVIR(_n)        (0x03700 + (4 * (_n)))
 
 #define wr32(reg, value) (writel(value, hw->hw_addr + reg))
 #define rd32(reg) (readl(hw->hw_addr + reg))
diff --git a/drivers/net/igb/igb.h b/drivers/net/igb/igb.h
index 63abd1c..0a55cd2 100644
--- a/drivers/net/igb/igb.h
+++ b/drivers/net/igb/igb.h
@@ -75,11 +75,13 @@ struct vf_data_storage {
 	u16 vlans_enabled;
 	u32 flags;
 	unsigned long last_nack;
+	u16 pf_vlan; /* When set, guest VLAN config not allowed. */
 };
 
 #define IGB_VF_FLAG_CTS            0x00000001 /* VF is clear to send data */
 #define IGB_VF_FLAG_UNI_PROMISC    0x00000002 /* VF has unicast promisc */
 #define IGB_VF_FLAG_MULTI_PROMISC  0x00000004 /* VF has multicast promisc */
+#define IGB_VF_FLAG_PF_SET_MAC     0x00000008 /* PF has set MAC address */
 
 /* RX descriptor control thresholds.
  * PTHRESH - MAC will consider prefetch if it has fewer than this number of
diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
index 0cab5e2..67773e3 100644
--- a/drivers/net/igb/igb_main.c
+++ b/drivers/net/igb/igb_main.c
@@ -128,6 +128,9 @@ static void igb_msg_task(struct igb_adapter *);
 static void igb_vmm_control(struct igb_adapter *);
 static int igb_set_vf_mac(struct igb_adapter *, int, unsigned char *);
 static void igb_restore_vf_multicasts(struct igb_adapter *adapter);
+static int igb_set_queue_mac(struct net_device *netdev, int queue, u8 *mac);
+static int igb_set_queue_vlan(struct net_device *netdev, int queue, u16 vlan);
+
 
 #ifdef CONFIG_PM
 static int igb_suspend(struct pci_dev *, pm_message_t);
@@ -1302,6 +1305,8 @@ static const struct net_device_ops igb_netdev_ops = {
 	.ndo_vlan_rx_register	= igb_vlan_rx_register,
 	.ndo_vlan_rx_add_vid	= igb_vlan_rx_add_vid,
 	.ndo_vlan_rx_kill_vid	= igb_vlan_rx_kill_vid,
+	.ndo_set_queue_mac	= igb_set_queue_mac,
+	.ndo_set_queue_vlan	= igb_set_queue_vlan,
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller	= igb_netpoll,
 #endif
@@ -2379,7 +2384,8 @@ static void igb_rlpml_set(struct igb_adapter *adapter)
 	wr32(E1000_RLPML, max_frame_size);
 }
 
-static inline void igb_set_vmolr(struct igb_adapter *adapter, int vfn)
+static inline void igb_set_vmolr(struct igb_adapter *adapter,
+				 int vfn, bool aupe)
 {
 	struct e1000_hw *hw = &adapter->hw;
 	u32 vmolr;
@@ -2392,8 +2398,11 @@ static inline void igb_set_vmolr(struct igb_adapter *adapter, int vfn)
 		return;
 
 	vmolr = rd32(E1000_VMOLR(vfn));
-	vmolr |= E1000_VMOLR_AUPE |        /* Accept untagged packets */
-	         E1000_VMOLR_STRVLAN;      /* Strip vlan tags */
+	vmolr |= E1000_VMOLR_STRVLAN;      /* Strip vlan tags */
+	if (aupe)
+		vmolr |= E1000_VMOLR_AUPE;   /* Accept untagged packets */
+	else
+		vmolr &= ~(E1000_VMOLR_AUPE); /* Tagged packets ONLY */
 
 	/* clear all bits that might not be set */
 	vmolr &= ~(E1000_VMOLR_BAM | E1000_VMOLR_RSSE);
@@ -2464,7 +2473,7 @@ void igb_configure_rx_ring(struct igb_adapter *adapter,
 	wr32(E1000_SRRCTL(reg_idx), srrctl);
 
 	/* set filtering for VMDQ pools */
-	igb_set_vmolr(adapter, reg_idx & 0x7);
+	igb_set_vmolr(adapter, reg_idx & 0x7, true);
 
 	/* enable receive descriptor fetching */
 	rxdctl = rd32(E1000_RXDCTL(reg_idx));
@@ -4352,6 +4361,49 @@ static s32 igb_vlvf_set(struct igb_adapter *adapter, u32 vid, bool add, u32 vf)
 	return -1;
 }
 
+static void igb_set_vmvir(struct igb_adapter *adapter, u32 vid, u32 vf)
+{
+	struct e1000_hw *hw = &adapter->hw;
+
+	if (vid)
+		wr32(E1000_VMVIR(vf), (vid | E1000_VMVIR_VLANA_DEFAULT));
+	else
+		wr32(E1000_VMVIR(vf), 0);
+}
+
+static int igb_pf_set_vf_vlan(struct igb_adapter *adapter, int vf, u16 vlan)
+{
+	int err = 0;
+
+	if (vlan) {
+		err = igb_vlvf_set(adapter, vlan, true, vf);
+		if (err)
+			goto out;
+		igb_set_vmvir(adapter, vlan, vf);
+		igb_set_vmolr(adapter, vf, false);
+		adapter->vf_data[vf].pf_vlan = vlan;
+	} else if (adapter->vf_data[vf].pf_vlan) {
+		err = igb_vlvf_set(adapter, adapter->vf_data[vf].pf_vlan,
+				   false, vf);
+		if (err)
+			goto out;
+		igb_set_vmvir(adapter, vlan, vf);
+		igb_set_vmolr(adapter, vf, true);
+		adapter->vf_data[vf].pf_vlan = 0;
+	}
+out:
+	return err;
+}
+
+static int igb_set_queue_vlan(struct net_device *netdev, int queue, u16 vlan)
+{
+	struct igb_adapter *adapter = netdev_priv(netdev);
+	if ((queue > adapter->vfs_allocated_count) ||
+	    (queue == 0) || (vlan > 4095))
+		return -EINVAL;
+	return igb_pf_set_vf_vlan(adapter, queue - 1, vlan);
+}
+
 static int igb_set_vf_vlan(struct igb_adapter *adapter, u32 *msgbuf, u32 vf)
 {
 	int add = (msgbuf[0] & E1000_VT_MSGINFO_MASK) >> E1000_VT_MSGINFO_SHIFT;
@@ -4362,15 +4414,18 @@ static int igb_set_vf_vlan(struct igb_adapter *adapter, u32 *msgbuf, u32 vf)
 
 static inline void igb_vf_reset(struct igb_adapter *adapter, u32 vf)
 {
-	/* clear all flags */
-	adapter->vf_data[vf].flags = 0;
+	/* clear flags */
+	adapter->vf_data[vf].flags &= ~(IGB_VF_FLAG_PF_SET_MAC);
 	adapter->vf_data[vf].last_nack = jiffies;
 
 	/* reset offloads to defaults */
-	igb_set_vmolr(adapter, vf);
+	igb_set_vmolr(adapter, vf, true);
 
 	/* reset vlans for device */
-	igb_clear_vf_vfta(adapter, vf);
+	if (adapter->vf_data[vf].pf_vlan)
+		igb_pf_set_vf_vlan(adapter, adapter->vf_data[vf].pf_vlan, vf);
+	else
+		igb_clear_vf_vfta(adapter, vf);
 
 	/* reset multicast table array for vf */
 	adapter->vf_data[vf].num_vf_mc_hashes = 0;
@@ -4384,7 +4439,8 @@ static void igb_vf_reset_event(struct igb_adapter *adapter, u32 vf)
 	unsigned char *vf_mac = adapter->vf_data[vf].vf_mac_addresses;
 
 	/* generate a new mac address as we were hotplug removed/added */
-	random_ether_addr(vf_mac);
+	if (!(adapter->vf_data[vf].flags & IGB_VF_FLAG_PF_SET_MAC))
+		random_ether_addr(vf_mac);
 
 	/* process remaining reset events */
 	igb_vf_reset(adapter, vf);
@@ -4493,7 +4549,10 @@ static void igb_rcv_msg_from_vf(struct igb_adapter *adapter, u32 vf)
 		retval = igb_set_vf_rlpml(adapter, msgbuf[1], vf);
 		break;
 	case E1000_VF_SET_VLAN:
-		retval = igb_set_vf_vlan(adapter, msgbuf, vf);
+		if (adapter->vf_data[vf].pf_vlan)
+			retval = -1;
+		else
+			retval = igb_set_vf_vlan(adapter, msgbuf, vf);
 		break;
 	default:
 		dev_err(&pdev->dev, "Unhandled Msg %08x\n", msgbuf[0]);
@@ -5843,6 +5902,17 @@ static int igb_set_vf_mac(struct igb_adapter *adapter,
 	return 0;
 }
 
+static int igb_set_queue_mac(struct net_device *netdev, int queue, u8 *mac)
+{
+	struct igb_adapter *adapter = netdev_priv(netdev);
+	if (!is_valid_ether_addr(mac) ||
+	    (queue > adapter->vfs_allocated_count) ||
+	    (queue == 0))
+		return -EINVAL;
+	adapter->vf_data[queue - 1].flags |= IGB_VF_FLAG_PF_SET_MAC;
+	return igb_set_vf_mac(adapter, queue - 1, mac);
+}
+
 static void igb_vmm_control(struct igb_adapter *adapter)
 {
 	struct e1000_hw *hw = &adapter->hw;

[RFC PATCH 4/4] igbvf: Make error message more enlightening

[Jeff Kirsher]

From: Williams, Mitch A <mitch.a.williams@intel.com>

The most likely cause of this error is that the PF interface is not up,
so let's make that very explicit.

Signed-off-by: Mitch Williams <mitch.a.williams@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 drivers/net/igbvf/netdev.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/net/igbvf/netdev.c b/drivers/net/igbvf/netdev.c
index fad7f34..a3c706c 100644
--- a/drivers/net/igbvf/netdev.c
+++ b/drivers/net/igbvf/netdev.c
@@ -2720,7 +2720,8 @@ static int __devinit igbvf_probe(struct pci_dev *pdev,
 	err = hw->mac.ops.reset_hw(hw);
 	if (err) {
 		dev_info(&pdev->dev,
-		         "PF still in reset state, assigning new address\n");
+			 "PF still in reset state, assigning new address. "
+			 "Is the PF interface up?\n");
 		random_ether_addr(hw->mac.addr);
 	} else {
 		err = hw->mac.ops.read_mac_addr(hw);

Re: [RFC PATCH 1/4] net: Add support to netdev ops for changing hardware queue MAC and VLAN filters

[Ben Hutchings]

On Tue, 2009-11-17 at 13:50 -0800, Jeff Kirsher wrote:
> From: Williams, Mitch A <mitch.a.williams@intel.com>
> 
> Signed-off-by: Mitch Williams <mitch.a.williams@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> ---
> 
>  include/linux/netdevice.h |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 7043f85..6a70365 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -610,6 +610,8 @@ struct netdev_queue {
>   *	this function is called when a VLAN id is unregistered.
>   *
>   * void (*ndo_poll_controller)(struct net_device *dev);
> + * int (*ndo_set_queue_mac)(struct net_device *dev, int queue, u8* mac);
> + * int (*ndo_set_queue_vlan)(struct net_device *dev, int queue, u16 vlan);
>   */
>  #define HAVE_NET_DEVICE_OPS
>  struct net_device_ops {
> @@ -659,6 +661,10 @@ struct net_device_ops {
>  #define HAVE_NETDEV_POLL
>  	void                    (*ndo_poll_controller)(struct net_device *dev);
>  #endif
> +	int			(*ndo_set_queue_mac)(struct net_device *dev,
> +						     int queue, u8 *mac);
> +	int			(*ndo_set_queue_vlan)(struct net_device *dev,
> +						      int queue, u16 vlan);
[...]

How do you remove a filter?

What about filtering on both MAC address and VLAN (our new controller
supports that)?

It seems like this could be defined as an extension to the existing
ethtool RX flow filter API.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

RE: [RFC PATCH 1/4] net: Add support to netdev ops for changing hardware queue MAC and VLAN filters

[Williams, Mitch A]

>From: Ben Hutchings [mailto:bhutchings@solarflare.com]
>Sent: Wednesday, November 18, 2009 11:53 AM

>How do you remove a filter?
>

You remove a filter by setting it to 0 on that queue. Works for either MAC or VLAN.

>What about filtering on both MAC address and VLAN (our new controller
>supports that)?

Setting a MAC filter doesn't blow away the VLAN filter, or vice-versa. So just run 'ip' twice to set the filters. Our hardware does it too, and it works fine for me:

$ ip link set eth1 queue 1 mac 00:11:22:33:44:55
$ ip link set eth1 queue 1 vlan 10

-Mitch

RE: [RFC PATCH 1/4] net: Add support to netdev ops for changing hardware queue MAC and VLAN filters

[Ben Hutchings]

On Wed, 2009-11-18 at 14:37 -0700, Williams, Mitch A wrote:
> 
> >From: Ben Hutchings [mailto:bhutchings@solarflare.com]
> >Sent: Wednesday, November 18, 2009 11:53 AM
> 
> >How do you remove a filter?
> >
> 
> You remove a filter by setting it to 0 on that queue. Works for either MAC or VLAN.
> 
> >What about filtering on both MAC address and VLAN (our new controller
> >supports that)?
> 
> Setting a MAC filter doesn't blow away the VLAN filter, or vice-versa. So just run 'ip' twice to set the filters. Our hardware does it too, and it works fine for me:
> 
> $ ip link set eth1 queue 1 mac 00:11:22:33:44:55
> $ ip link set eth1 queue 1 vlan 10

Hmm, this is not what I would expect.  I'm used to filters being defined
independently of queues in a hash table or CAM.  In that case, setting
multiple filters pointing to one queue will result in the logical
disjunction of the filters.  Do I understand correctly that you have
exactly one of each type of filter per queue, with multiple filters
interpreted as a logical conjunction?

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

RE: [RFC PATCH 1/4] net: Add support to netdev ops for changing hardware queue MAC and VLAN filters

[Williams, Mitch A]

>From: Ben Hutchings [mailto:bhutchings@solarflare.com]
>Sent: Wednesday, November 18, 2009 3:14 PM
>> Setting a MAC filter doesn't blow away the VLAN filter, or vice-versa. So
>just run 'ip' twice to set the filters. Our hardware does it too, and it
>works fine for me:
>>
>> $ ip link set eth1 queue 1 mac 00:11:22:33:44:55
>> $ ip link set eth1 queue 1 vlan 10
>
>Hmm, this is not what I would expect.  I'm used to filters being defined
>independently of queues in a hash table or CAM.  In that case, setting
>multiple filters pointing to one queue will result in the logical
>disjunction of the filters.  Do I understand correctly that you have
>exactly one of each type of filter per queue, with multiple filters
>interpreted as a logical conjunction?
>

Well, it's weirder than that.

In the case of SR-IOV on our hardware, these filters are perfect - no hash tables are required. (We do use hash tables when we have a bunch of multicast addresses, but that's not what this is about.)

MAC filters deny packets by default, so you won't get anything without a valid MAC filter on the queue.

A queue with no VLAN filters will receive packets from all VLANs, albeit with the tags passed up intact.  So in that sense, the VLAN filters are default-allow.  

However, once you enable any VLAN filter, the hardware starts stripping tags and begins to deny packets by default.

Based on these semantics, the filtering operation that I've described above makes perfect sense.

If I understand you correctly, you'd like to be able to apply both types of filter on a single command line:

$ip link set eth1 queue 1 mac <something> vlan <something>

Obviously, this can be done, but it would complicate the code significantly.

-Mitch

RE: [RFC PATCH 1/4] net: Add support to netdev ops for changing hardware queue MAC and VLAN filters

[Ben Hutchings]

On Wed, 2009-11-18 at 16:33 -0700, Williams, Mitch A wrote:
[...]
> In the case of SR-IOV on our hardware, these filters are perfect - no
> hash tables are required. (We do use hash tables when we have a bunch
> of multicast addresses, but that's not what this is about.)
> 
> MAC filters deny packets by default, so you won't get anything without
> a valid MAC filter on the queue.
> 
> A queue with no VLAN filters will receive packets from all VLANs,
> albeit with the tags passed up intact.  So in that sense, the VLAN
> filters are default-allow.  
> 
> However, once you enable any VLAN filter, the hardware starts
> stripping tags and begins to deny packets by default.
>
> Based on these semantics, the filtering operation that I've described
> above makes perfect sense.
[...]

But they are not the semantics we would want in supposedly generic
netdev operations.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

RE: [RFC PATCH 1/4] net: Add support to netdev ops for changing hardware queue MAC and VLAN filters

[Williams, Mitch A]

>From: Ben Hutchings [mailto:bhutchings@solarflare.com]
>Sent: Thursday, November 19, 2009 6:55 AM


>[...]
>> In the case of SR-IOV on our hardware, these filters are perfect - no
>> hash tables are required. (We do use hash tables when we have a bunch
>> of multicast addresses, but that's not what this is about.)
>>
>> MAC filters deny packets by default, so you won't get anything without
>> a valid MAC filter on the queue.
>>
>> A queue with no VLAN filters will receive packets from all VLANs,
>> albeit with the tags passed up intact.  So in that sense, the VLAN
>> filters are default-allow.
>>
>> However, once you enable any VLAN filter, the hardware starts
>> stripping tags and begins to deny packets by default.
>>
>> Based on these semantics, the filtering operation that I've described
>> above makes perfect sense.
>[...]
>
>But they are not the semantics we would want in supposedly generic
>netdev operations.
>

OK, point taken.  Specific hardware features/limitations shouldn't affect kernel policy.

However, I'm still back to code complexity, and general usage models.

Please explain specifically what you perceive to be the difference between:

$ ip link set eth1 queue 1 mac <blah>
$ ip link set eth1 queue 1 vlan <foo>

and

$ ip link set eth1 queue 1 mac <blah> vlan <foo>

The two filter types are, in my mind, completely orthogonal.  You can have one, or the other, or both, or neither. What do we gain by glomming both options on one command line?  And is this worth the tradeoff of more complex code?

-Mitch

Re: [RFC PATCH 1/4] net: Add support to netdev ops for changing hardware queue MAC and VLAN filters

[Patrick McHardy]

Williams, Mitch A wrote:
> However, I'm still back to code complexity, and general usage models.
> 
> Please explain specifically what you perceive to be the difference between:
> 
> $ ip link set eth1 queue 1 mac <blah>
> $ ip link set eth1 queue 1 vlan <foo>
> 
> and
> 
> $ ip link set eth1 queue 1 mac <blah> vlan <foo>
> 
> The two filter types are, in my mind, completely orthogonal.  You can have one, or the other, or both, or neither. What do we gain by glomming both options on one command line?  And is this worth the tradeoff of more complex code?

One argument would be that "ip link show" should probably display
all filters, so they all need to be included in the dump message.

And this is exactly the same message type used for configuring
links and the API is supposed to be symetric, meaning you can
send a dump message with NLM_F_REQUEST to the kernel again and
it (re-)creates the same object.

RE: [RFC PATCH 1/4] net: Add support to netdev ops for changing hardware queue MAC and VLAN filters

[Ben Hutchings]

On Thu, 2009-11-19 at 11:43 -0700, Williams, Mitch A wrote:
[...]
> OK, point taken.  Specific hardware features/limitations shouldn't affect kernel policy.
> 
> However, I'm still back to code complexity, and general usage models.
> 
> Please explain specifically what you perceive to be the difference between:
> 
> $ ip link set eth1 queue 1 mac <blah>
> $ ip link set eth1 queue 1 vlan <foo>
> 
> and
> 
> $ ip link set eth1 queue 1 mac <blah> vlan <foo>
> 
> The two filter types are, in my mind, completely orthogonal.  You can
> have one, or the other, or both, or neither. What do we gain by
> glomming both options on one command line?  And is this worth the
> tradeoff of more complex code?

I think you need to state clearly what semantics you are now proposing
before I can make any judgement on them.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

RE: [RFC PATCH 1/4] net: Add support to netdev ops for changing hardware queue MAC and VLAN filters

[Williams, Mitch A]

>From: Patrick McHardy [mailto:kaber@trash.net]
>Sent: Thursday, November 19, 2009 10:59 AM

[snip]
>
>One argument would be that "ip link show" should probably display
>all filters, so they all need to be included in the dump message.
>
>And this is exactly the same message type used for configuring
>links and the API is supposed to be symetric, meaning you can
>send a dump message with NLM_F_REQUEST to the kernel again and
>it (re-)creates the same object.

Yeah, we've indentified that as a need - that's one reason we sent this out as an RFC. I'll implement the "show" semantics before we repost.

-Mitch

RE: [RFC PATCH 1/4] net: Add support to netdev ops for changing hardware queue MAC and VLAN filters

[Williams, Mitch A]

>From: Ben Hutchings [mailto:bhutchings@solarflare.com]
>Sent: Thursday, November 19, 2009 10:59 AM

>> Please explain specifically what you perceive to be the difference
>between:
>>
>> $ ip link set eth1 queue 1 mac <blah>
>> $ ip link set eth1 queue 1 vlan <foo>
>>
>> and
>>
>> $ ip link set eth1 queue 1 mac <blah> vlan <foo>
>>
>> The two filter types are, in my mind, completely orthogonal.  You can
>> have one, or the other, or both, or neither. What do we gain by
>> glomming both options on one command line?  And is this worth the
>> tradeoff of more complex code?
>
>I think you need to state clearly what semantics you are now proposing
>before I can make any judgement on them.
>

OK, now I'm really confused, Ben. It seems that we are both asking each other the same question.

What I'm proposing is really the same as we have now for single-queue devices:

- A MAC filter is enabled by default. If you want to change the MAC address, you use a tool (ip or ifconfig) to change it.

- A VLAN filter is not enabled by default. If you want to filter on VLANs, then you load the 8021q module, and enable a filter.

The MAC filter is configured completely separately from the VLAN filter. Either one can be changed without affecting the other one and, in fact, you use two different tools to do these operations.

For SR-IOV VF devices, my proposed changes implement exactly the same thing.  You use one command to change the MAC address.  You use another command to change the VLAN filter. Changing one does not affect the other.

In this case, we use the same tool for both operations, but they're still separate operations.

-Mitch

N.B.
The major difference in VLAN filtering is that this implementation allows the VF to participate in only one VLAN, and the filter is applied independently of the VF driver. So you can put a specific VM on a VLAN without its knowledge. If you want the VM to have more intelligent VLAN filtering, you don't use this filter, and you load 8021q in the VM and set your filters there.

RE: [RFC PATCH 1/4] net: Add support to netdev ops for changing hardware queue MAC and VLAN filters

[Ben Hutchings]

On Thu, 2009-11-19 at 12:34 -0700, Williams, Mitch A wrote:
> >From: Ben Hutchings [mailto:bhutchings@solarflare.com]
> >Sent: Thursday, November 19, 2009 10:59 AM
> 
> >> Please explain specifically what you perceive to be the difference
> >between:
> >>
> >> $ ip link set eth1 queue 1 mac <blah>
> >> $ ip link set eth1 queue 1 vlan <foo>
> >>
> >> and
> >>
> >> $ ip link set eth1 queue 1 mac <blah> vlan <foo>
> >>
> >> The two filter types are, in my mind, completely orthogonal.  You can
> >> have one, or the other, or both, or neither. What do we gain by
> >> glomming both options on one command line?  And is this worth the
> >> tradeoff of more complex code?
> >
> >I think you need to state clearly what semantics you are now proposing
> >before I can make any judgement on them.
> >
> 
> OK, now I'm really confused, Ben. It seems that we are both asking
> each other the same question.
> 
> What I'm proposing is really the same as we have now for single-queue
> devices:
> 
> - A MAC filter is enabled by default. If you want to change the MAC
> address, you use a tool (ip or ifconfig) to change it.
> 
> - A VLAN filter is not enabled by default. If you want to filter on
> VLANs, then you load the 8021q module, and enable a filter.

This doesn't seem quite the same to me, but I'll not argue this.

> The MAC filter is configured completely separately from the VLAN
> filter. Either one can be changed without affecting the other one and,
> in fact, you use two different tools to do these operations.
> 
> For SR-IOV VF devices, my proposed changes implement exactly the same
> thing.  You use one command to change the MAC address.  You use
> another command to change the VLAN filter. Changing one does not
> affect the other.
> 
> In this case, we use the same tool for both operations, but they're
> still separate operations.

This makes some sense, and I accept that it's rather different from
filtering for delivery to the host.

> N.B.
> The major difference in VLAN filtering is that this implementation
> allows the VF to participate in only one VLAN, and the filter is
> applied independently of the VF driver. So you can put a specific VM
> on a VLAN without its knowledge. If you want the VM to have more
> intelligent VLAN filtering, you don't use this filter, and you load
> 8021q in the VM and set your filters there.

How does this interact with use of multiple queues within a single
function?  Are the specified queue numbers really interpreted as RX
queue indices or as function numbers?

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

RE: [RFC PATCH 1/4] net: Add support to netdev ops for changing hardware queue MAC and VLAN filters

[Williams, Mitch A]

>From: Ben Hutchings [mailto:bhutchings@solarflare.com]
>Sent: Monday, November 23, 2009 5:23 AM
[snip]

>
>How does this interact with use of multiple queues within a single
>function?  Are the specified queue numbers really interpreted as RX
>queue indices or as function numbers?
>
>Ben.

Yeah, that is ambiguous.  Would it be better if we changed the name of the parameter to 'vf' instead of 'queue' to make it explicit?

This would give us:
$ ip link set eth1 vf 1 mac <blah>

The issue of which VF goes with which PF device can be deduced in userspace via sysfs.

If we want to make this apply to non SR-IOV queues, then we'll add a new parameter later.

Works for you?

-Mitch

RE: [RFC PATCH 1/4] net: Add support to netdev ops for changing hardware queue MAC and VLAN filters

[Ben Hutchings]

On Mon, 2009-11-23 at 12:52 -0700, Williams, Mitch A wrote:
> >From: Ben Hutchings [mailto:bhutchings@solarflare.com]
> >Sent: Monday, November 23, 2009 5:23 AM
> [snip]
> 
> >
> >How does this interact with use of multiple queues within a single
> >function?  Are the specified queue numbers really interpreted as RX
> >queue indices or as function numbers?
> >
> >Ben.
> 
> Yeah, that is ambiguous.  Would it be better if we changed the name of the parameter to 'vf' instead of 'queue' to make it explicit?
> 
> This would give us:
> $ ip link set eth1 vf 1 mac <blah>
> 
> The issue of which VF goes with which PF device can be deduced in userspace via sysfs.
> 
> If we want to make this apply to non SR-IOV queues, then we'll add a new parameter later.
> 
> Works for you?

OK, this sounds reasonable.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

RE: [RFC PATCH 1/4] net: Add support to netdev ops for changing hardware queue MAC and VLAN filters

[Williams, Mitch A]

>From: Ben Hutchings [mailto:bhutchings@solarflare.com]
>Sent: Tuesday, November 24, 2009 6:18 AM


>> [snip]
>>
>> >
>> >How does this interact with use of multiple queues within a single
>> >function?  Are the specified queue numbers really interpreted as RX
>> >queue indices or as function numbers?
>> >
>> >Ben.
>>
>> Yeah, that is ambiguous.  Would it be better if we changed the name of
>the parameter to 'vf' instead of 'queue' to make it explicit?
>>
>> This would give us:
>> $ ip link set eth1 vf 1 mac <blah>
>>
>> The issue of which VF goes with which PF device can be deduced in
>userspace via sysfs.
>>
>> If we want to make this apply to non SR-IOV queues, then we'll add a new
>parameter later.
>>
>> Works for you?
>
>OK, this sounds reasonable.
>
>Ben.
>

Thanks for looking at this stuff, Ben. I appreciate it. I'll rework the patches and send them out in a few days.

-Mitch

Re: [RFC PATCH 1/4] net: Add support to netdev ops for changing hardware queue MAC and VLAN filters

[Simon Horman]

On Mon, Nov 23, 2009 at 12:52:48PM -0700, Williams, Mitch A wrote:
> >From: Ben Hutchings [mailto:bhutchings@solarflare.com]
> >Sent: Monday, November 23, 2009 5:23 AM
> [snip]
> 
> >
> >How does this interact with use of multiple queues within a single
> >function?  Are the specified queue numbers really interpreted as RX
> >queue indices or as function numbers?
> >
> >Ben.
> 
> Yeah, that is ambiguous.  Would it be better if we changed the name of the parameter to 'vf' instead of 'queue' to make it explicit?
> 
> This would give us:
> $ ip link set eth1 vf 1 mac <blah>
> 
> The issue of which VF goes with which PF device can be deduced in
> userspace via sysfs.

Does this mean that the configuration of filtering for a VF needs
to be done where the interface for the VF exists - e.g. in a KVM
guest/Xen domU?

In terms of dealing with interfaces and the way that tools such as ip work
that makes a lot of sense. But I wonder if it actually makes more sense
from an administrative point of view to have this configuration go through
the PF - e.g. the KVM host/Xen domO.

RE: [RFC PATCH 1/4] net: Add support to netdev ops for changing hardware queue MAC and VLAN filters

[Williams, Mitch A]

>From: Simon Horman [mailto:horms@verge.net.au]
>Sent: Sunday, November 29, 2009 10:03 PM

>> The issue of which VF goes with which PF device can be deduced in
>> userspace via sysfs.
>
>Does this mean that the configuration of filtering for a VF needs
>to be done where the interface for the VF exists - e.g. in a KVM
>guest/Xen domU?
>

No, all of the configuration is done through the PF device.  What I was saying was that, given a specific VF PCI device (which would be passed through to the VM), you can use sysfs to determine which PF owns it, and then run the ip command to tell the PF to configure the VF.

>In terms of dealing with interfaces and the way that tools such as ip work
>that makes a lot of sense. But I wonder if it actually makes more sense
>from an administrative point of view to have this configuration go through
>the PF - e.g. the KVM host/Xen domO.

>From a policy and security standpoint, you can't allow the VM to handle its own hardware configuration. The host/hypervisor/VM Manager/boss has to handle this or you lose many of the advantages of virtualization (i.e. isolation, security, stability, etc).

-Mitch

Re: [RFC PATCH 1/4] net: Add support to netdev ops for changing hardware queue MAC and VLAN filters

[Simon Horman]

On Mon, Nov 30, 2009 at 11:36:08AM -0700, Williams, Mitch A wrote:
> >From: Simon Horman [mailto:horms@verge.net.au]
> >Sent: Sunday, November 29, 2009 10:03 PM
> 
> >> The issue of which VF goes with which PF device can be deduced in
> >> userspace via sysfs.
> >
> >Does this mean that the configuration of filtering for a VF needs
> >to be done where the interface for the VF exists - e.g. in a KVM
> >guest/Xen domU?
> >
> 
> No, all of the configuration is done through the PF device.  What I was
> saying was that, given a specific VF PCI device (which would be passed
> through to the VM), you can use sysfs to determine which PF owns it, and
> then run the ip command to tell the PF to configure the VF.

Understood, sorry I missed that the first time around.

> >In terms of dealing with interfaces and the way that tools such as ip work
> >that makes a lot of sense. But I wonder if it actually makes more sense
> >from an administrative point of view to have this configuration go through
> >the PF - e.g. the KVM host/Xen domO.
> 
> >From a policy and security standpoint, you can't allow the VM to handle
> >its own hardware configuration. The host/hypervisor/VM Manager/boss has
> >to handle this or you lose many of the advantages of virtualization
> >(i.e. isolation, security, stability, etc).

Yes, agreed.