[PATCH v2 0/4] virtio_net: Add MAC and VLAN filtering

[PATCH v2 0/4] virtio_net: Add MAC and VLAN filtering

[Alex Williamson]

This series adds infrastructure for a new control virtqueue and
makes use of it to support set_rx_mode, unicast and multicast address
lists, and supporting a hypervisor based VLAN filter.  The goal is to
make the virtio-net device support more of the features of a physical
NIC and allow the hypervisor to discard packets we don't want.

This version incorporates suggestions from Rusty.  The MAC filter table
size is now managed by the hypervisor.  We treat it as inifinite and
rely on the hypervisor to fall back to promiscuous or all-multi mode
as their resources allow.  The point that made this finally sink in
as the right way to go was the idea of bonding directly to a NIC and
using this interface to manipulate a hardware filter (I hope someone
is working on that in QEMU/KVM).  I've also changed the send_command()
function to return bool.  I'm not completely sure it's everything
you're looking for Rusty, but it does seem cleaner.  Let me know if
this is closer to what you're thinking.  Thanks,

Alex

---

Alex Williamson (4):
      virtio_net: Add support for VLAN filtering in the hypervisor
      virtio_net: Add a MAC filter table
      virtio_net: Add a set_rx_mode interface
      virtio_net: Add a virtqueue for outbound control commands


 drivers/net/virtio_net.c   |  198 +++++++++++++++++++++++++++++++++++++++++++-
 include/linux/virtio_net.h |   61 ++++++++++++++
 2 files changed, 256 insertions(+), 3 deletions(-)

-- 
Alex Williamson

[PATCH v2 1/4] virtio_net: Add a virtqueue for outbound control commands

[Alex Williamson]

This will be used for RX mode, MAC filter table, VLAN filtering, etc...

The control transaction consists of one or more "out" sg entries and
one or more "in" sg entries.  The first out entry contains a header
defining the class and command.  Additional out entries may provide
data for the command.  The last in entry provides a status response
back from the command.

Virtqueues typically run asynchronous, running a callback function
when there's data in the channel.  We can't readily make use of this
in the command paths where we need to use this.  Instead, we kick
the virtqueue and spin.  The kick causes an I/O write, triggering an
immediate trap into the hypervisor.

Signed-off-by: Alex Williamson <alex.williamson@hp.com>
---

 drivers/net/virtio_net.c   |   54 ++++++++++++++++++++++++++++++++++++++++++--
 include/linux/virtio_net.h |   18 +++++++++++++++
 2 files changed, 69 insertions(+), 3 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index bf65978..c909444 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -40,7 +40,7 @@ module_param(gso, bool, 0444);
 struct virtnet_info
 {
 	struct virtio_device *vdev;
-	struct virtqueue *rvq, *svq;
+	struct virtqueue *rvq, *svq, *cvq;
 	struct net_device *dev;
 	struct napi_struct napi;
 	unsigned int status;
@@ -605,6 +605,41 @@ static int virtnet_open(struct net_device *dev)
 	return 0;
 }
 
+static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
+				 void *data, unsigned int len)
+{
+	struct scatterlist sg[3];
+	struct virtio_net_ctrl_hdr ctrl;
+	virtio_net_ctrl_ack status;
+	unsigned int tmp;
+	int i = 0;
+
+	if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ))
+		return false;
+
+	sg_init_table(sg, len ? 3 : 2);
+
+	sg_set_buf(&sg[i++], &ctrl, sizeof(ctrl));
+	if (len)
+		sg_set_buf(&sg[i++], data, len);
+	sg_set_buf(&sg[i], &status, sizeof(status));
+
+	ctrl.class = class;
+	ctrl.cmd = cmd;
+
+	status = ~0;
+
+	if (vi->cvq->vq_ops->add_buf(vi->cvq, sg, i, 1, vi) != 0)
+		BUG();
+
+	vi->cvq->vq_ops->kick(vi->cvq);
+
+	while (!vi->cvq->vq_ops->get_buf(vi->cvq, &tmp))
+		cpu_relax();
+
+	return status == VIRTIO_NET_OK ? true : false;
+}
+
 static int virtnet_close(struct net_device *dev)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
@@ -771,6 +806,14 @@ static int virtnet_probe(struct virtio_device *vdev)
 		goto free_recv;
 	}
 
+	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ)) {
+		vi->cvq = vdev->config->find_vq(vdev, 2, NULL);
+		if (IS_ERR(vi->cvq)) {
+			err = PTR_ERR(vi->svq);
+			goto free_send;
+		}
+	}
+
 	/* Initialize our empty receive and send queues. */
 	skb_queue_head_init(&vi->recv);
 	skb_queue_head_init(&vi->send);
@@ -783,7 +826,7 @@ static int virtnet_probe(struct virtio_device *vdev)
 	err = register_netdev(dev);
 	if (err) {
 		pr_debug("virtio_net: registering device failed\n");
-		goto free_send;
+		goto free_ctrl;
 	}
 
 	/* Last of all, set up some receive buffers. */
@@ -803,6 +846,9 @@ static int virtnet_probe(struct virtio_device *vdev)
 
 unregister:
 	unregister_netdev(dev);
+free_ctrl:
+	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ))
+		vdev->config->del_vq(vi->cvq);
 free_send:
 	vdev->config->del_vq(vi->svq);
 free_recv:
@@ -834,6 +880,8 @@ static void virtnet_remove(struct virtio_device *vdev)
 
 	vdev->config->del_vq(vi->svq);
 	vdev->config->del_vq(vi->rvq);
+	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ))
+		vdev->config->del_vq(vi->cvq);
 	unregister_netdev(vi->dev);
 
 	while (vi->pages)
@@ -853,7 +901,7 @@ static unsigned int features[] = {
 	VIRTIO_NET_F_HOST_TSO4, VIRTIO_NET_F_HOST_UFO, VIRTIO_NET_F_HOST_TSO6,
 	VIRTIO_NET_F_HOST_ECN, VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6,
 	VIRTIO_NET_F_GUEST_ECN, /* We don't yet handle UFO input. */
-	VIRTIO_NET_F_MRG_RXBUF, VIRTIO_NET_F_STATUS,
+	VIRTIO_NET_F_MRG_RXBUF, VIRTIO_NET_F_STATUS, VIRTIO_NET_F_CTRL_VQ,
 	VIRTIO_F_NOTIFY_ON_EMPTY,
 };
 
diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index f76bd4a..fd0981c 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -22,6 +22,7 @@
 #define VIRTIO_NET_F_HOST_UFO	14	/* Host can handle UFO in. */
 #define VIRTIO_NET_F_MRG_RXBUF	15	/* Host can merge receive buffers. */
 #define VIRTIO_NET_F_STATUS	16	/* virtio_net_config.status available */
+#define VIRTIO_NET_F_CTRL_VQ	17	/* Control channel available */
 
 #define VIRTIO_NET_S_LINK_UP	1	/* Link is up */
 
@@ -58,4 +59,21 @@ struct virtio_net_hdr_mrg_rxbuf {
 	__u16 num_buffers;	/* Number of merged rx buffers */
 };
 
+/*
+ * Control virtqueue data structures
+ *
+ * The control virtqueue expects a header in the first sg entry
+ * and an ack/status response in the last entry.  Data for the
+ * command goes in between.
+ */
+struct virtio_net_ctrl_hdr {
+	__u8 class;
+	__u8 cmd;
+} __attribute__((packed));
+
+typedef __u8 virtio_net_ctrl_ack;
+
+#define VIRTIO_NET_OK     0
+#define VIRTIO_NET_ERR    1
+
 #endif /* _LINUX_VIRTIO_NET_H */

Re: [PATCH v2 1/4] virtio_net: Add a virtqueue for outbound control commands

[Rusty Russell]

On Friday 30 January 2009 09:35:07 Alex Williamson wrote:
> +static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> +				 void *data, unsigned int len)
> +{
> +	struct scatterlist sg[3];
> +	struct virtio_net_ctrl_hdr ctrl;
> +	virtio_net_ctrl_ack status;
> +	unsigned int tmp;
> +	int i = 0;
> +
> +	if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ))
> +		return false;
> +
> +	sg_init_table(sg, len ? 3 : 2);

I'd test data rather than len here.

> +	sg_set_buf(&sg[i++], &ctrl, sizeof(ctrl));
> +	if (len)
> +		sg_set_buf(&sg[i++], data, len);
> +	sg_set_buf(&sg[i], &status, sizeof(status));
> +
> +	ctrl.class = class;
> +	ctrl.cmd = cmd;
> +
> +	status = ~0;
> +
> +	if (vi->cvq->vq_ops->add_buf(vi->cvq, sg, i, 1, vi) != 0)
> +		BUG();

Hmm, I prefer initialize buffer, then set sg to refer to it.  Seems to make
more sense to me.

> +	while (!vi->cvq->vq_ops->get_buf(vi->cvq, &tmp))
> +		cpu_relax();

This probably deserves a comment about why it's OK to spin here.

> +	return status == VIRTIO_NET_OK ? true : false;

Or "return status == VIRTIO_NET_OK;" ?

See what I mean about nit-picking?
Rusty.

[PATCH v2 2/4] virtio_net: Add a set_rx_mode interface

[Alex Williamson]

Make use of the RX_MODE control virtqueue class to enable the
set_rx_mode netdev interface.  This allows us to selectively
enable/disable promiscuous and allmulti mode so we don't see
packets we don't want.  We'll automatically enable these as
needed if additional unicast or multicast addresses are
requested.

Signed-off-by: Alex Williamson <alex.williamson@hp.com>
---

 drivers/net/virtio_net.c   |   26 ++++++++++++++++++++++++++
 include/linux/virtio_net.h |   10 ++++++++++
 2 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index c909444..b247558 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -660,6 +660,30 @@ static int virtnet_set_tx_csum(struct net_device *dev, u32 data)
 	return ethtool_op_set_tx_hw_csum(dev, data);
 }
 
+static void virtnet_set_rx_mode(struct net_device *dev)
+{
+	struct virtnet_info *vi = netdev_priv(dev);
+	u8 promisc, allmulti;
+
+	if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_RX))
+		return;
+
+	promisc = ((dev->flags & IFF_PROMISC) != 0 || dev->uc_count > 0);
+	allmulti = ((dev->flags & IFF_ALLMULTI) != 0 || dev->mc_count > 0);
+
+	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
+				  VIRTIO_NET_CTRL_RX_PROMISC,
+				  &promisc, sizeof(promisc)))
+		printk(KERN_WARNING "%s: Failed to %sable promisc mode.\n",
+		       dev->name, promisc ? "en" : "dis");
+
+	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
+				  VIRTIO_NET_CTRL_RX_ALLMULTI,
+				  &allmulti, sizeof(allmulti)))
+		printk(KERN_WARNING "%s: Failed to %sable allmulti mode.\n",
+		       dev->name, allmulti ? "en" : "dis");
+}
+
 static struct ethtool_ops virtnet_ethtool_ops = {
 	.set_tx_csum = virtnet_set_tx_csum,
 	.set_sg = ethtool_op_set_sg,
@@ -684,6 +708,7 @@ static const struct net_device_ops virtnet_netdev = {
 	.ndo_start_xmit      = start_xmit,
 	.ndo_validate_addr   = eth_validate_addr,
 	.ndo_set_mac_address = virtnet_set_mac_address,
+	.ndo_set_rx_mode     = virtnet_set_rx_mode,
 	.ndo_change_mtu	     = virtnet_change_mtu,
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller = virtnet_netpoll,
@@ -902,6 +927,7 @@ static unsigned int features[] = {
 	VIRTIO_NET_F_HOST_ECN, VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6,
 	VIRTIO_NET_F_GUEST_ECN, /* We don't yet handle UFO input. */
 	VIRTIO_NET_F_MRG_RXBUF, VIRTIO_NET_F_STATUS, VIRTIO_NET_F_CTRL_VQ,
+	VIRTIO_NET_F_CTRL_RX,
 	VIRTIO_F_NOTIFY_ON_EMPTY,
 };
 
diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index fd0981c..aac09ec 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -23,6 +23,7 @@
 #define VIRTIO_NET_F_MRG_RXBUF	15	/* Host can merge receive buffers. */
 #define VIRTIO_NET_F_STATUS	16	/* virtio_net_config.status available */
 #define VIRTIO_NET_F_CTRL_VQ	17	/* Control channel available */
+#define VIRTIO_NET_F_CTRL_RX	18	/* Control channel RX mode support */
 
 #define VIRTIO_NET_S_LINK_UP	1	/* Link is up */
 
@@ -76,4 +77,13 @@ typedef __u8 virtio_net_ctrl_ack;
 #define VIRTIO_NET_OK     0
 #define VIRTIO_NET_ERR    1
 
+/*
+ * Control the RX mode, ie. promisucous and allmulti.  PROMISC and
+ * ALLMULTI commands require an "out" sg entry containing a 1 byte
+ * state value, zero = disable, non-zero = enable.
+ */
+#define VIRTIO_NET_CTRL_RX    0
+ #define VIRTIO_NET_CTRL_RX_PROMISC      0
+ #define VIRTIO_NET_CTRL_RX_ALLMULTI     1
+
 #endif /* _LINUX_VIRTIO_NET_H */

Re: [PATCH v2 2/4] virtio_net: Add a set_rx_mode interface

[Rusty Russell]

On Friday 30 January 2009 09:35:12 Alex Williamson wrote: 
> +static void virtnet_set_rx_mode(struct net_device *dev)
> +{
> +	struct virtnet_info *vi = netdev_priv(dev);
> +	u8 promisc, allmulti;
> +
> +	if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_RX))
> +		return;

Hmm, this check duplicates the one in virtnet_send_command; after
all your patches, is it ever called with !VIRTIO_NET_F_CTRL_RX?  Maybe it should be a BUG_ON in there?

> +	promisc = ((dev->flags & IFF_PROMISC) != 0 || dev->uc_count > 0);
> +	allmulti = ((dev->flags & IFF_ALLMULTI) != 0 || dev->mc_count > 0);
> +
> +	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
> +				  VIRTIO_NET_CTRL_RX_PROMISC,
> +				  &promisc, sizeof(promisc)))
> +		printk(KERN_WARNING "%s: Failed to %sable promisc mode.\n",
> +		       dev->name, promisc ? "en" : "dis");
> +
> +	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
> +				  VIRTIO_NET_CTRL_RX_ALLMULTI,
> +				  &allmulti, sizeof(allmulti)))
> +		printk(KERN_WARNING "%s: Failed to %sable allmulti mode.\n",
> +		       dev->name, allmulti ? "en" : "dis");
> +}

Hmm, we can't do anything with this error.  I'd be very tempted to define the API to say "this can't fail".  Leave this code in as a sanity check, but have a comment to that effect?

> +#define VIRTIO_NET_CTRL_RX    0
> + #define VIRTIO_NET_CTRL_RX_PROMISC      0
> + #define VIRTIO_NET_CTRL_RX_ALLMULTI     1

Comment above/beside these two perhaps?
/* Supported if VIRTIO_NET_F_CTRL_RX */

Cheers,
Rusty.

[PATCH v2 3/4] virtio_net: Add a MAC filter table

[Alex Williamson]

Make use of the MAC control virtqueue class to support a MAC
filter table.  The filter table is managed by the hypervisor.
We consider the table to be available if the CTRL_MAC feature
bit is set.  We leave it to the hypervisor to manage the table
and enable promiscuous or all-multi mode as necessary depending
on the resources available to it.

Signed-off-by: Alex Williamson <alex.williamson@hp.com>
---

 drivers/net/virtio_net.c   |   89 +++++++++++++++++++++++++++++++++++++++++++-
 include/linux/virtio_net.h |   17 ++++++++
 2 files changed, 103 insertions(+), 3 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index b247558..23610ce 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -664,12 +664,22 @@ static void virtnet_set_rx_mode(struct net_device *dev)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
 	u8 promisc, allmulti;
+	struct scatterlist sg[4];
+	struct virtio_net_ctrl_hdr ctrl;
+	virtio_net_ctrl_ack status;
+	u8 *uc_buf = NULL, *mc_buf = NULL;
+	unsigned int tmp;
 
 	if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_RX))
 		return;
 
-	promisc = ((dev->flags & IFF_PROMISC) != 0 || dev->uc_count > 0);
-	allmulti = ((dev->flags & IFF_ALLMULTI) != 0 || dev->mc_count > 0);
+	promisc = ((dev->flags & IFF_PROMISC) != 0);
+	allmulti = ((dev->flags & IFF_ALLMULTI) != 0);
+
+	if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_MAC)) {
+		promisc |= (dev->uc_count > 0);
+		allmulti |= (dev->mc_count > 0);
+	}
 
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
 				  VIRTIO_NET_CTRL_RX_PROMISC,
@@ -682,6 +692,79 @@ static void virtnet_set_rx_mode(struct net_device *dev)
 				  &allmulti, sizeof(allmulti)))
 		printk(KERN_WARNING "%s: Failed to %sable allmulti mode.\n",
 		       dev->name, allmulti ? "en" : "dis");
+
+	if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_MAC))
+		return;
+
+	sg_init_table(sg, 4);
+
+	sg_set_buf(&sg[0], &ctrl, sizeof(ctrl));
+	sg_set_buf(&sg[3], &status, sizeof(status));
+
+	ctrl.class = VIRTIO_NET_CTRL_MAC;
+	ctrl.cmd = VIRTIO_NET_CTRL_MAC_TABLE_SET;
+
+	status = ~0;
+
+	if (dev->uc_count) {
+		u8 *cur;
+		struct dev_addr_list *uc;
+		int i;
+
+		cur = uc_buf = kzalloc(dev->uc_count * ETH_ALEN, GFP_ATOMIC);
+		if (!uc_buf) {
+			printk(KERN_WARNING "%s: No memory for UC list\n",
+			       dev->name);
+			return;
+		}
+
+		uc = dev->uc_list;
+		for (i = 0; i < dev->uc_count; i++) {
+			memcpy(cur, uc->da_addr, ETH_ALEN);
+			cur += ETH_ALEN;
+			uc = uc->next;
+		}
+
+		sg_set_buf(&sg[1], uc_buf, dev->uc_count * ETH_ALEN);
+	}
+
+	if (dev->mc_count) {
+		u8 *cur;
+		struct dev_addr_list *mc;
+		int i;
+
+		cur = mc_buf = kzalloc(dev->mc_count * ETH_ALEN, GFP_ATOMIC);
+		if (!mc_buf) {
+			printk(KERN_WARNING "%s: No memory for MC list\n",
+			       dev->name);
+			goto free_uc;
+		}
+
+		mc = dev->mc_list;
+		for (i = 0; i < dev->mc_count; i++) {
+			memcpy(cur, mc->da_addr, ETH_ALEN);
+			cur += ETH_ALEN;
+			mc = mc->next;
+		}
+
+		sg_set_buf(&sg[2], mc_buf, dev->mc_count * ETH_ALEN);
+	}
+
+	if (vi->cvq->vq_ops->add_buf(vi->cvq, sg, 3, 1, vi) != 0)
+		BUG();
+
+	vi->cvq->vq_ops->kick(vi->cvq);
+
+	while (!vi->cvq->vq_ops->get_buf(vi->cvq, &tmp))
+		cpu_relax();
+
+	if (status != VIRTIO_NET_OK)
+		printk(KERN_WARNING "%s: Failed to set MAC filter table (%d)\n",
+		       dev->name, status);
+
+	kfree(mc_buf);
+free_uc:
+	kfree(uc_buf);
 }
 
 static struct ethtool_ops virtnet_ethtool_ops = {
@@ -927,7 +1010,7 @@ static unsigned int features[] = {
 	VIRTIO_NET_F_HOST_ECN, VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6,
 	VIRTIO_NET_F_GUEST_ECN, /* We don't yet handle UFO input. */
 	VIRTIO_NET_F_MRG_RXBUF, VIRTIO_NET_F_STATUS, VIRTIO_NET_F_CTRL_VQ,
-	VIRTIO_NET_F_CTRL_RX,
+	VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_MAC,
 	VIRTIO_F_NOTIFY_ON_EMPTY,
 };
 
diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index aac09ec..c8e945a 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -24,6 +24,7 @@
 #define VIRTIO_NET_F_STATUS	16	/* virtio_net_config.status available */
 #define VIRTIO_NET_F_CTRL_VQ	17	/* Control channel available */
 #define VIRTIO_NET_F_CTRL_RX	18	/* Control channel RX mode support */
+#define VIRTIO_NET_F_CTRL_MAC	19	/* Control channel MAC filtering */
 
 #define VIRTIO_NET_S_LINK_UP	1	/* Link is up */
 
@@ -86,4 +87,20 @@ typedef __u8 virtio_net_ctrl_ack;
  #define VIRTIO_NET_CTRL_RX_PROMISC      0
  #define VIRTIO_NET_CTRL_RX_ALLMULTI     1
 
+/*
+ * Control the MAC filter table.
+ *
+ * The MAC filter table is managed by the hypervisor, the guest should
+ * assume the size is infinite.  Filtering should be considered
+ * non-perfect, ie. based on hypervisor resources, the guest may
+ * received packets from sources not specified in the filter list.
+ *
+ * In addition to the class/cmd header, the TABLE_SET command requires
+ * two out scatterlists.  The first is a concatenated byte stream of the
+ * ETH_ALEN unicast MAC addresses for the table, the second is the same
+ * for multicast addresses.
+ */
+#define VIRTIO_NET_CTRL_MAC    1
+ #define VIRTIO_NET_CTRL_MAC_TABLE_SET        0
+
 #endif /* _LINUX_VIRTIO_NET_H */

Re: [PATCH v2 3/4] virtio_net: Add a MAC filter table

[Rusty Russell]

On Friday 30 January 2009 09:35:18 Alex Williamson wrote:
>  	struct virtnet_info *vi = netdev_priv(dev);
>  	u8 promisc, allmulti;
> +	struct scatterlist sg[4];
> +	struct virtio_net_ctrl_hdr ctrl;
> +	virtio_net_ctrl_ack status;
> +	u8 *uc_buf = NULL, *mc_buf = NULL;
> +	unsigned int tmp;
>  
>  	if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_RX))
>  		return;

It's kind of annoying that we have our virtnet_send_command helper and
yet we can't use it here.

Hmm, maybe we can kill two birds in one stone?  I was a bit nervous about the virtnet_send_command data arg because the pointer must not be vmalloc'ed memory.  If we make the arg to virtnet_send_command an sg[] in place of data & len, we force the caller to do the sg_set_buf (which means they *should* be aware of the limitation on what can be put in an sg), and we can make a copy (ideally we could chain it, but it's not possible to chain a const sg[], so let's BUG_ON if it's too big to fit on the stack).

Then this code can use it.

> -	promisc = ((dev->flags & IFF_PROMISC) != 0 || dev->uc_count > 0);
> -	allmulti = ((dev->flags & IFF_ALLMULTI) != 0 || dev->mc_count > 0);
> +	promisc = ((dev->flags & IFF_PROMISC) != 0);
> +	allmulti = ((dev->flags & IFF_ALLMULTI) != 0);
> +
> +	if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_MAC)) {
> +		promisc |= (dev->uc_count > 0);
> +		allmulti |= (dev->mc_count > 0);
> +	}
>  
>  	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
>  				  VIRTIO_NET_CTRL_RX_PROMISC,
> @@ -682,6 +692,79 @@ static void virtnet_set_rx_mode(struct net_device *dev)
>  				  &allmulti, sizeof(allmulti)))
>  		printk(KERN_WARNING "%s: Failed to %sable allmulti mode.\n",
>  		       dev->name, allmulti ? "en" : "dis");
> +
> +	if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_MAC))
> +		return;

I think we can now fold this feature into VIRTIO_NET_F_CTRL_RX and assert that you must support mac filtering.  Because it's now trivial to support in the host (use promisc!), and it simplifies the implementation.

> +	if (dev->uc_count) {
...
> +		sg_set_buf(&sg[1], uc_buf, dev->uc_count * ETH_ALEN);
...
> +	if (dev->mc_count) {
...
> +		sg_set_buf(&sg[2], mc_buf, dev->mc_count * ETH_ALEN);

We made the decision a while ago not to rely on scatter gather boundaries to define the API.  So you'll actually need a structure to contain the counts unfortunately (technically you only need one count, since the other can be intuited, but that seems silly, and this way you could theoretically add more fields without problems with future feature bits).

It also means you can do a single kmalloc, and skip the if here, if you wanted.

Cheers,
Rusty.

[PATCH v2 4/4] virtio_net: Add support for VLAN filtering in the hypervisor

[Alex Williamson]

VLAN filtering allows the hypervisor to drop packets from VLANs
that we're not a part of, further reducing the number of extraneous
packets recieved.  This makes use of the VLAN virtqueue command class.
The CTRL_VLAN feature bit tells us whether the backend supports VLAN
filtering.

Signed-off-by: Alex Williamson <alex.williamson@hp.com>
---

 drivers/net/virtio_net.c   |   37 ++++++++++++++++++++++++++++++++++++-
 include/linux/virtio_net.h |   16 ++++++++++++++++
 2 files changed, 52 insertions(+), 1 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 23610ce..14ee139 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -767,6 +767,26 @@ free_uc:
 	kfree(uc_buf);
 }
 
+static void virnet_vlan_rx_add_vid(struct net_device *dev, u16 vid)
+{
+	struct virtnet_info *vi = netdev_priv(dev);
+
+	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
+				  VIRTIO_NET_CTRL_VLAN_ADD, &vid, sizeof(vid)))
+		printk(KERN_WARNING "%s: Failed to add VLAN ID %d.\n",
+		       dev->name, vid);
+}
+
+static void virnet_vlan_rx_kill_vid(struct net_device *dev, u16 vid)
+{
+	struct virtnet_info *vi = netdev_priv(dev);
+
+	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
+				  VIRTIO_NET_CTRL_VLAN_DEL, &vid, sizeof(vid)))
+		printk(KERN_WARNING "%s: Failed to kill VLAN ID %d.\n",
+		       dev->name, vid);
+}
+
 static struct ethtool_ops virtnet_ethtool_ops = {
 	.set_tx_csum = virtnet_set_tx_csum,
 	.set_sg = ethtool_op_set_sg,
@@ -793,6 +813,8 @@ static const struct net_device_ops virtnet_netdev = {
 	.ndo_set_mac_address = virtnet_set_mac_address,
 	.ndo_set_rx_mode     = virtnet_set_rx_mode,
 	.ndo_change_mtu	     = virtnet_change_mtu,
+	.ndo_vlan_rx_add_vid = virnet_vlan_rx_add_vid,
+	.ndo_vlan_rx_kill_vid = virnet_vlan_rx_kill_vid,
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller = virtnet_netpoll,
 #endif
@@ -920,6 +942,19 @@ static int virtnet_probe(struct virtio_device *vdev)
 			err = PTR_ERR(vi->svq);
 			goto free_send;
 		}
+
+		if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VLAN)) {
+			u8 enable = 1;
+
+			/* Enable VLAN filtering */
+			if (virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
+						 VIRTIO_NET_CTRL_VLAN_ENABLE,
+						 &enable, sizeof(enable)))
+				dev->features |= NETIF_F_HW_VLAN_FILTER;
+			else
+				printk(KERN_WARNING "virtio_net: "
+				       "Failed to enable VLAN filter\n");
+		}
 	}
 
 	/* Initialize our empty receive and send queues. */
@@ -1010,7 +1045,7 @@ static unsigned int features[] = {
 	VIRTIO_NET_F_HOST_ECN, VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6,
 	VIRTIO_NET_F_GUEST_ECN, /* We don't yet handle UFO input. */
 	VIRTIO_NET_F_MRG_RXBUF, VIRTIO_NET_F_STATUS, VIRTIO_NET_F_CTRL_VQ,
-	VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_MAC,
+	VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_MAC, VIRTIO_NET_F_CTRL_VLAN,
 	VIRTIO_F_NOTIFY_ON_EMPTY,
 };
 
diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index c8e945a..8733a66 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -25,6 +25,7 @@
 #define VIRTIO_NET_F_CTRL_VQ	17	/* Control channel available */
 #define VIRTIO_NET_F_CTRL_RX	18	/* Control channel RX mode support */
 #define VIRTIO_NET_F_CTRL_MAC	19	/* Control channel MAC filtering */
+#define VIRTIO_NET_F_CTRL_VLAN	20	/* Control channel VLAN filtering */
 
 #define VIRTIO_NET_S_LINK_UP	1	/* Link is up */
 
@@ -103,4 +104,19 @@ typedef __u8 virtio_net_ctrl_ack;
 #define VIRTIO_NET_CTRL_MAC    1
  #define VIRTIO_NET_CTRL_MAC_TABLE_SET        0
 
+/*
+ * Control VLAN filtering
+ *
+ * The VLAN filter table is controlled via a simple ADD/DEL interface.
+ * VLAN IDs not added will be dropped.  Del is the opposite of add.
+ * Both commands expect an out entry containing a 2 byte VLAN ID.
+ * The ENABLE command expects an out entry containing a single byte,
+ * zero to disable, non-zero to enable.  The default state is disabled
+ * for compatibility.
+ */
+#define VIRTIO_NET_CTRL_VLAN       2
+ #define VIRTIO_NET_CTRL_VLAN_ENABLE          0
+ #define VIRTIO_NET_CTRL_VLAN_ADD             1
+ #define VIRTIO_NET_CTRL_VLAN_DEL             2
+
 #endif /* _LINUX_VIRTIO_NET_H */

Re: [PATCH v2 4/4] virtio_net: Add support for VLAN filtering in the hypervisor

[Rusty Russell]

On Friday 30 January 2009 09:35:23 Alex Williamson wrote:
> +	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
> +				  VIRTIO_NET_CTRL_VLAN_ADD, &vid, sizeof(vid)))
> +		printk(KERN_WARNING "%s: Failed to add VLAN ID %d.\n",
> +		       dev->name, vid);
> +}

Hmm, I should have mentioned dev_warn() in previous patches.

And possibly (since we never expect it to happen), we should move it into virtnet_send_command to save all the callers sweating on it.  It'd be
less informative tho.

> +		if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VLAN)) {
> +			u8 enable = 1;
> +
> +			/* Enable VLAN filtering */
> +			if (virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
> +						 VIRTIO_NET_CTRL_VLAN_ENABLE,
> +						 &enable, sizeof(enable)))
> +				dev->features |= NETIF_F_HW_VLAN_FILTER;
> +			else
> +				printk(KERN_WARNING "virtio_net: "
> +				       "Failed to enable VLAN filter\n");
> +		}

We can kill VLAN_ENABLE.  We activate it by acking the feature bit.

So this becomes:
	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VLAN))
		dev->features |= NETIF_F_HW_VLAN_FILTER;

> + * The VLAN filter table is controlled via a simple ADD/DEL interface.
> + * VLAN IDs not added will be dropped.  Del is the opposite of add.
> + * Both commands expect an out entry containing a 2 byte VLAN ID.

s/will be dropped/may be filtered out by the host/ ?

Cheers,
Rusty.

Re: [PATCH v2 0/4] virtio_net: Add MAC and VLAN filtering

[David Miller]

From: Alex Williamson <alex.williamson@hp.com>
Date: Thu, 29 Jan 2009 16:05:02 -0700

> I'm not completely sure it's everything you're looking for Rusty,
> but it does seem cleaner.  Let me know if this is closer to what
> you're thinking.

Rusty, are these good?  If so, would you like you or I to take them?

Thanks.

Re: [PATCH v2 0/4] virtio_net: Add MAC and VLAN filtering

[Rusty Russell]

On Friday 30 January 2009 12:19:13 David Miller wrote:
> From: Alex Williamson <alex.williamson@hp.com>
> Date: Thu, 29 Jan 2009 16:05:02 -0700
> 
> > I'm not completely sure it's everything you're looking for Rusty,
> > but it does seem cleaner.  Let me know if this is closer to what
> > you're thinking.
> 
> Rusty, are these good?  If so, would you like you or I to take them?

Still nitpicking.  We'll wrestle a bit more, then I'll ack them to you.

These are 2.6.30 material, so no huge hurry AFAICT?

Thanks,
Rusty.

Re: [PATCH v2 0/4] virtio_net: Add MAC and VLAN filtering

[David Miller]

From: Rusty Russell <rusty@rustcorp.com.au>
Date: Fri, 30 Jan 2009 17:35:57 +1030

> On Friday 30 January 2009 12:19:13 David Miller wrote:
> > From: Alex Williamson <alex.williamson@hp.com>
> > Date: Thu, 29 Jan 2009 16:05:02 -0700
> > 
> > > I'm not completely sure it's everything you're looking for Rusty,
> > > but it does seem cleaner.  Let me know if this is closer to what
> > > you're thinking.
> > 
> > Rusty, are these good?  If so, would you like you or I to take them?
> 
> Still nitpicking.  We'll wrestle a bit more, then I'll ack them to you.

Sounds fine to me.

> These are 2.6.30 material, so no huge hurry AFAICT?

Right, no hurry.

Re: [PATCH v2 0/4] virtio_net: Add MAC and VLAN filtering

[Rusty Russell]

On Friday 30 January 2009 09:35:02 Alex Williamson wrote:
> I'm not completely sure it's everything
> you're looking for Rusty, but it does seem cleaner.  Let me know if
> this is closer to what you're thinking.

Yep, now I'll go through the patches more carefully.

I know it's a pain to get this kind of nit-picking feedback, but it's an ordeal I force on everyone the first time they make a major contribution; it ensures the code "feels" consistent and means that when I read the code in future I'm not surprised by things which aren't *quite* how I expect.

Note that I tend to comment on *everything* where I would have done it differently.  So I expect you to argue at least half of them.

Thanks!
Rusty.

[PATCH v3 0/4] virtio_net: Add MAC and VLAN filtering

[Alex Williamson]

This series adds infrastructure for a new control virtqueue and
makes use of it to support set_rx_mode, unicast and multicast address
lists, and supporting a hypervisor based VLAN filter.  The goal is to
make the virtio-net device support more of the features of a physical
NIC and allow the hypervisor to discard packets we don't want.

This version incorporates further suggestions from Rusty.  The
send_command() helper function now takes a scatterlist pointer so
that we're not limited to using it in for commands that take only
take one or no data args.  The caller now needs to specify how
many entries are out/in, and the helper function adds the header
and status.  We'll now BUG if send_command is called without the vq
available, but set_rx_mode needs a graceful exit since we can't
dynamically tell the upper netdev layers not to call in.  I left the
MAC_SET_TABLE call using two scatter entries even though it could
now be done with one, and are allocated as one.  The F_CTRL_MAC
feature is now folded into F_CTRL_RX, but I left the separate class
for it.  VLAN_ENABLE is now gone.  I was concerned that we needed
some way to disable VLAN filtering, but after looking at e1000, I
think it's probably sufficient for the backend to prioritize promisc
over VLAN as a way to disable it.  I switched errors to dev_warn, but
left them as the caller's responsibility so we can get useful error
messages.  Thanks for the comments, please provide more ;^)  Thanks,

Alex

---

Alex Williamson (4):
      virtio_net: Add support for VLAN filtering in the hypervisor
      virtio_net: Add a MAC filter table
      virtio_net: Add a set_rx_mode interface
      virtio_net: Add a virtqueue for outbound control commands


 drivers/net/virtio_net.c   |  166 +++++++++++++++++++++++++++++++++++++++++++-
 include/linux/virtio_net.h |   67 ++++++++++++++++++
 2 files changed, 230 insertions(+), 3 deletions(-)

-- 
Alex Williamson

[PATCH v3 1/4] virtio_net: Add a virtqueue for outbound control commands

[Alex Williamson]

This will be used for RX mode, MAC filter table, VLAN filtering, etc...

The control transaction consists of one or more "out" sg entries and
one or more "in" sg entries.  The first out entry contains a header
defining the class and command.  Additional out entries may provide
data for the command.  The last in entry provides a status response
back from the command.

Virtqueues typically run asynchronous, running a callback function
when there's data in the channel.  We can't readily make use of this
in the command paths where we need to use this.  Instead, we kick
the virtqueue and spin.  The kick causes an I/O write, triggering an
immediate trap into the hypervisor.

Signed-off-by: Alex Williamson <alex.williamson@hp.com>
---

 drivers/net/virtio_net.c   |   66 ++++++++++++++++++++++++++++++++++++++++++--
 include/linux/virtio_net.h |   20 +++++++++++++
 2 files changed, 83 insertions(+), 3 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index bf65978..d84e176 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -40,7 +40,7 @@ module_param(gso, bool, 0444);
 struct virtnet_info
 {
 	struct virtio_device *vdev;
-	struct virtqueue *rvq, *svq;
+	struct virtqueue *rvq, *svq, *cvq;
 	struct net_device *dev;
 	struct napi_struct napi;
 	unsigned int status;
@@ -605,6 +605,53 @@ static int virtnet_open(struct net_device *dev)
 	return 0;
 }
 
+/*
+ * Send command via the control virtqueue and check status.  Commands
+ * supported by the hypervisor, as indicated by feature bits, should
+ * never fail unless improperly formated.
+ */
+static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
+				 struct scatterlist data[], int out, int in)
+{
+	struct scatterlist sg[VIRTIO_NET_MAX_CTRL_ARGS];
+	struct virtio_net_ctrl_hdr ctrl;
+	virtio_net_ctrl_ack status = ~0;
+	unsigned int tmp;
+
+	if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ)) {
+		BUG();  /* Caller should know better */
+		return false;
+	}
+
+	out++; /* Add header */
+	in++; /* Add return status */
+
+	BUG_ON(out + in > VIRTIO_NET_MAX_CTRL_ARGS);
+
+	ctrl.class = class;
+	ctrl.cmd = cmd;
+
+	sg_init_table(sg, out + in);
+
+	sg_set_buf(&sg[0], &ctrl, sizeof(ctrl));
+	memcpy(&sg[1], data, sizeof(struct scatterlist) * (out + in - 2));
+	sg_set_buf(&sg[out + in - 1], &status, sizeof(status));
+
+	if (vi->cvq->vq_ops->add_buf(vi->cvq, sg, out, in, vi) != 0)
+		BUG();
+
+	vi->cvq->vq_ops->kick(vi->cvq);
+
+	/*
+	 * Spin for a response, the kick causes an ioport write, trapping
+	 * into the hypervisor, so the request should be handled immediately.
+	 */
+	while (!vi->cvq->vq_ops->get_buf(vi->cvq, &tmp))
+		cpu_relax();
+
+	return status == VIRTIO_NET_OK;
+}
+
 static int virtnet_close(struct net_device *dev)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
@@ -771,6 +818,14 @@ static int virtnet_probe(struct virtio_device *vdev)
 		goto free_recv;
 	}
 
+	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ)) {
+		vi->cvq = vdev->config->find_vq(vdev, 2, NULL);
+		if (IS_ERR(vi->cvq)) {
+			err = PTR_ERR(vi->svq);
+			goto free_send;
+		}
+	}
+
 	/* Initialize our empty receive and send queues. */
 	skb_queue_head_init(&vi->recv);
 	skb_queue_head_init(&vi->send);
@@ -783,7 +838,7 @@ static int virtnet_probe(struct virtio_device *vdev)
 	err = register_netdev(dev);
 	if (err) {
 		pr_debug("virtio_net: registering device failed\n");
-		goto free_send;
+		goto free_ctrl;
 	}
 
 	/* Last of all, set up some receive buffers. */
@@ -803,6 +858,9 @@ static int virtnet_probe(struct virtio_device *vdev)
 
 unregister:
 	unregister_netdev(dev);
+free_ctrl:
+	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ))
+		vdev->config->del_vq(vi->cvq);
 free_send:
 	vdev->config->del_vq(vi->svq);
 free_recv:
@@ -834,6 +892,8 @@ static void virtnet_remove(struct virtio_device *vdev)
 
 	vdev->config->del_vq(vi->svq);
 	vdev->config->del_vq(vi->rvq);
+	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ))
+		vdev->config->del_vq(vi->cvq);
 	unregister_netdev(vi->dev);
 
 	while (vi->pages)
@@ -853,7 +913,7 @@ static unsigned int features[] = {
 	VIRTIO_NET_F_HOST_TSO4, VIRTIO_NET_F_HOST_UFO, VIRTIO_NET_F_HOST_TSO6,
 	VIRTIO_NET_F_HOST_ECN, VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6,
 	VIRTIO_NET_F_GUEST_ECN, /* We don't yet handle UFO input. */
-	VIRTIO_NET_F_MRG_RXBUF, VIRTIO_NET_F_STATUS,
+	VIRTIO_NET_F_MRG_RXBUF, VIRTIO_NET_F_STATUS, VIRTIO_NET_F_CTRL_VQ,
 	VIRTIO_F_NOTIFY_ON_EMPTY,
 };
 
diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index f76bd4a..f28a72a 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -22,6 +22,7 @@
 #define VIRTIO_NET_F_HOST_UFO	14	/* Host can handle UFO in. */
 #define VIRTIO_NET_F_MRG_RXBUF	15	/* Host can merge receive buffers. */
 #define VIRTIO_NET_F_STATUS	16	/* virtio_net_config.status available */
+#define VIRTIO_NET_F_CTRL_VQ	17	/* Control channel available */
 
 #define VIRTIO_NET_S_LINK_UP	1	/* Link is up */
 
@@ -58,4 +59,23 @@ struct virtio_net_hdr_mrg_rxbuf {
 	__u16 num_buffers;	/* Number of merged rx buffers */
 };
 
+/*
+ * Control virtqueue data structures
+ *
+ * The control virtqueue expects a header in the first sg entry
+ * and an ack/status response in the last entry.  Data for the
+ * command goes in between.
+ */
+struct virtio_net_ctrl_hdr {
+	__u8 class;
+	__u8 cmd;
+} __attribute__((packed));
+
+typedef __u8 virtio_net_ctrl_ack;
+
+#define VIRTIO_NET_OK     0
+#define VIRTIO_NET_ERR    1
+
+#define VIRTIO_NET_MAX_CTRL_ARGS   2
+
 #endif /* _LINUX_VIRTIO_NET_H */

Re: [PATCH v3 1/4] virtio_net: Add a virtqueue for outbound control commands

[Rusty Russell]

On Monday 02 February 2009 06:35:10 Alex Williamson wrote:

> +static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> +				 struct scatterlist data[], int out, int in)
> +{
> +	struct scatterlist sg[VIRTIO_NET_MAX_CTRL_ARGS];
...
> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index f76bd4a..f28a72a 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
...
> +#define VIRTIO_NET_MAX_CTRL_ARGS   2

I think this definition belongs at the top of virtio_net.c; the others
are userspace-exposed, this is really an internal issue, no?

Other than that, you can add Acked-by: Rusty Russell <rusty@rustcorp.com.au>
and send to netdev for DaveM's tree.

Thanks!
Rusty.

Re: [PATCH v3 1/4] virtio_net: Add a virtqueue for outbound control commands

[Anthony Liguori]

Hi Alex,

Can you post the userspace side of this to qemu-devel (against QEMU SVN) 
and I'll review/apply.  I've already looked through the series a couple 
times and I don't think there are any major issues.

Regards,

Anthony Liguori

Rusty Russell wrote:
> On Monday 02 February 2009 06:35:10 Alex Williamson wrote:
>
>   
>> +static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
>> +				 struct scatterlist data[], int out, int in)
>> +{
>> +	struct scatterlist sg[VIRTIO_NET_MAX_CTRL_ARGS];
>>     
> ...
>   
>> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
>> index f76bd4a..f28a72a 100644
>> --- a/include/linux/virtio_net.h
>> +++ b/include/linux/virtio_net.h
>>     
> ...
>   
>> +#define VIRTIO_NET_MAX_CTRL_ARGS   2
>>     
>
> I think this definition belongs at the top of virtio_net.c; the others
> are userspace-exposed, this is really an internal issue, no?
>
> Other than that, you can add Acked-by: Rusty Russell <rusty@rustcorp.com.au>
> and send to netdev for DaveM's tree.
>
> Thanks!
> Rusty.
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   

[PATCH v3 2/4] virtio_net: Add a set_rx_mode interface

[Alex Williamson]

Make use of the RX_MODE control virtqueue class to enable the
set_rx_mode netdev interface.  This allows us to selectively
enable/disable promiscuous and allmulti mode so we don't see
packets we don't want.  For now, we automatically enable these
as needed if additional unicast or multicast addresses are
requested.

Signed-off-by: Alex Williamson <alex.williamson@hp.com>
---

 drivers/net/virtio_net.c   |   32 ++++++++++++++++++++++++++++++++
 include/linux/virtio_net.h |   13 ++++++++++++-
 2 files changed, 44 insertions(+), 1 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index d84e176..f43b9d3 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -672,6 +672,36 @@ static int virtnet_set_tx_csum(struct net_device *dev, u32 data)
 	return ethtool_op_set_tx_hw_csum(dev, data);
 }
 
+static void virtnet_set_rx_mode(struct net_device *dev)
+{
+	struct virtnet_info *vi = netdev_priv(dev);
+	struct scatterlist sg;
+	u8 promisc, allmulti;
+
+	/* We can't dynamicaly set ndo_set_rx_mode, so return gracefully */
+	if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_RX))
+		return;
+
+	promisc = ((dev->flags & IFF_PROMISC) != 0 || dev->uc_count > 0);
+	allmulti = ((dev->flags & IFF_ALLMULTI) != 0 || dev->mc_count > 0);
+
+	sg_set_buf(&sg, &promisc, sizeof(promisc));
+
+	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
+				  VIRTIO_NET_CTRL_RX_PROMISC,
+				  &sg, 1, 0))
+		dev_warn(&dev->dev, "Failed to %sable promisc mode.\n",
+			 promisc ? "en" : "dis");
+
+	sg_set_buf(&sg, &allmulti, sizeof(allmulti));
+
+	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
+				  VIRTIO_NET_CTRL_RX_ALLMULTI,
+				  &sg, 1, 0))
+		dev_warn(&dev->dev, "Failed to %sable allmulti mode.\n",
+			 allmulti ? "en" : "dis");
+}
+
 static struct ethtool_ops virtnet_ethtool_ops = {
 	.set_tx_csum = virtnet_set_tx_csum,
 	.set_sg = ethtool_op_set_sg,
@@ -696,6 +726,7 @@ static const struct net_device_ops virtnet_netdev = {
 	.ndo_start_xmit      = start_xmit,
 	.ndo_validate_addr   = eth_validate_addr,
 	.ndo_set_mac_address = virtnet_set_mac_address,
+	.ndo_set_rx_mode     = virtnet_set_rx_mode,
 	.ndo_change_mtu	     = virtnet_change_mtu,
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller = virtnet_netpoll,
@@ -914,6 +945,7 @@ static unsigned int features[] = {
 	VIRTIO_NET_F_HOST_ECN, VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6,
 	VIRTIO_NET_F_GUEST_ECN, /* We don't yet handle UFO input. */
 	VIRTIO_NET_F_MRG_RXBUF, VIRTIO_NET_F_STATUS, VIRTIO_NET_F_CTRL_VQ,
+	VIRTIO_NET_F_CTRL_RX,
 	VIRTIO_F_NOTIFY_ON_EMPTY,
 };
 
diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index f28a72a..69569e1 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -23,6 +23,7 @@
 #define VIRTIO_NET_F_MRG_RXBUF	15	/* Host can merge receive buffers. */
 #define VIRTIO_NET_F_STATUS	16	/* virtio_net_config.status available */
 #define VIRTIO_NET_F_CTRL_VQ	17	/* Control channel available */
+#define VIRTIO_NET_F_CTRL_RX	18	/* Control channel RX mode support */
 
 #define VIRTIO_NET_S_LINK_UP	1	/* Link is up */
 
@@ -76,6 +77,16 @@ typedef __u8 virtio_net_ctrl_ack;
 #define VIRTIO_NET_OK     0
 #define VIRTIO_NET_ERR    1
 
-#define VIRTIO_NET_MAX_CTRL_ARGS   2
+#define VIRTIO_NET_MAX_CTRL_ARGS   3
+
+/*
+ * Control the RX mode, ie. promisucous and allmulti.  PROMISC and
+ * ALLMULTI commands require an "out" sg entry containing a 1 byte
+ * state value, zero = disable, non-zero = enable.  These commands
+ * are supported with the VIRTIO_NET_F_CTRL_RX feature.
+ */
+#define VIRTIO_NET_CTRL_RX    0
+ #define VIRTIO_NET_CTRL_RX_PROMISC      0
+ #define VIRTIO_NET_CTRL_RX_ALLMULTI     1
 
 #endif /* _LINUX_VIRTIO_NET_H */

Re: [PATCH v3 2/4] virtio_net: Add a set_rx_mode interface

[Rusty Russell]

On Monday 02 February 2009 06:35:15 Alex Williamson wrote:
> +	sg_set_buf(&sg, &promisc, sizeof(promisc));
> +
> +	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
> +				  VIRTIO_NET_CTRL_RX_PROMISC,
> +				  &sg, 1, 0))

Hmm, can we use sg_init_one(&sg, &promisc, sizeof(promisc)) then pass two sg
to virtnet_send_command?  ie.  change virtnet_send_command to:

	static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
                                struct scatterlist *out, struct scatterlist *in);

NULL = no sg, otherwise it's assumed to be a nicely terminated sg?  Neater for
the callers I think...

> -#define VIRTIO_NET_MAX_CTRL_ARGS   2
> +#define VIRTIO_NET_MAX_CTRL_ARGS   3

Oh, and this should probably be called VIRTNET_SEND_COMMAND_SG_MAX, and be
this value minus 2 (ie. informative for the caller of virtnet_send_command).

Thanks,
Rusty.
PS.  Oh, this actual patch was fine: Acked-by: Rusty Russell <rusty@rustcorp.com.au>

Re: [PATCH v3 2/4] virtio_net: Add a set_rx_mode interface

[Alex Williamson]

Hi Rusty,

On Mon, 2009-02-02 at 20:22 +1030, Rusty Russell wrote:
> On Monday 02 February 2009 06:35:15 Alex Williamson wrote:
> > +	sg_set_buf(&sg, &promisc, sizeof(promisc));
> > +
> > +	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
> > +				  VIRTIO_NET_CTRL_RX_PROMISC,
> > +				  &sg, 1, 0))
> 
> Hmm, can we use sg_init_one(&sg, &promisc, sizeof(promisc)) then pass two sg
> to virtnet_send_command?  ie.  change virtnet_send_command to:
> 
> 	static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
>                                 struct scatterlist *out, struct scatterlist *in);
> 
> NULL = no sg, otherwise it's assumed to be a nicely terminated sg?  Neater for
> the callers I think...

I think the caller still ends up passing the counts of out and in
entries here or else we have to walk the lists in send_command().
Doable, but doesn't seem to be common practice that I can find.  We
should also then clear the termination once we copy the sg[] into our
own, but functions to do that don't seem to exist (need an
sg_unmark_end()).  I had avoided anything that would call
sg_init_table() on the caller provided sg[] to avoid that problem, does
it need to be addressed?

> > -#define VIRTIO_NET_MAX_CTRL_ARGS   2
> > +#define VIRTIO_NET_MAX_CTRL_ARGS   3
> 
> Oh, and this should probably be called VIRTNET_SEND_COMMAND_SG_MAX, and be
> this value minus 2 (ie. informative for the caller of virtnet_send_command).

Yes, and yes to moving it to the c file, no need to expose it.  Thanks,

Alex

-- 
Alex Williamson                             HP Open Source & Linux Org.

Re: [PATCH v3 2/4] virtio_net: Add a set_rx_mode interface

[Rusty Russell]

On Tuesday 03 February 2009 08:04:07 Alex Williamson wrote:
> Hi Rusty,
> 
> On Mon, 2009-02-02 at 20:22 +1030, Rusty Russell wrote:
> > 	static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> >                                 struct scatterlist *out, struct scatterlist *in);
... 
> I think the caller still ends up passing the counts of out and in
> entries here or else we have to walk the lists in send_command().
> Doable, but doesn't seem to be common practice that I can find.

Yes, I've frequently expressed unhappiness with the API.  I lost that debate
and it doesn't seem fruitful to return to it.

It's not speed-critical, so just iterate.  I refuse to make *my* APIs suck.

> We
> should also then clear the termination once we copy the sg[] into our
> own, but functions to do that don't seem to exist (need an
> sg_unmark_end()).

Ideally we would chain them, but the caller would have to allocate an extra
sg element.

Implement sg_unmark_end() in a separate patch; be sure to CC Jens.

> I had avoided anything that would call
> sg_init_table() on the caller provided sg[] to avoid that problem, does
> it need to be addressed?

I don't think so: those args should in fact be const struct scatterlist *.

Thanks,
Rusty.

[PATCH v3 3/4] virtio_net: Add a MAC filter table

[Alex Williamson]

Make use of the MAC control virtqueue class to support a MAC
filter table.  The filter table is managed by the hypervisor.
We consider the table to be available if the CTRL_RX feature
bit is set.  We leave it to the hypervisor to manage the table
and enable promiscuous or all-multi mode as necessary depending
on the resources available to it.

Signed-off-by: Alex Williamson <alex.williamson@hp.com>
---

 drivers/net/virtio_net.c   |   53 ++++++++++++++++++++++++++++++++++++++------
 include/linux/virtio_net.h |   25 ++++++++++++++++++++-
 2 files changed, 70 insertions(+), 8 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index f43b9d3..e03eebf 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -675,31 +675,70 @@ static int virtnet_set_tx_csum(struct net_device *dev, u32 data)
 static void virtnet_set_rx_mode(struct net_device *dev)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
-	struct scatterlist sg;
+	struct scatterlist sg[2];
 	u8 promisc, allmulti;
+	struct virtio_net_ctrl_mac *mac_data;
+	struct dev_addr_list *addr;
+	void *buf;
+	int i;
 
 	/* We can't dynamicaly set ndo_set_rx_mode, so return gracefully */
 	if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_RX))
 		return;
 
-	promisc = ((dev->flags & IFF_PROMISC) != 0 || dev->uc_count > 0);
-	allmulti = ((dev->flags & IFF_ALLMULTI) != 0 || dev->mc_count > 0);
+	promisc = ((dev->flags & IFF_PROMISC) != 0);
+	allmulti = ((dev->flags & IFF_ALLMULTI) != 0);
 
-	sg_set_buf(&sg, &promisc, sizeof(promisc));
+	sg_set_buf(sg, &promisc, sizeof(promisc));
 
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
 				  VIRTIO_NET_CTRL_RX_PROMISC,
-				  &sg, 1, 0))
+				  sg, 1, 0))
 		dev_warn(&dev->dev, "Failed to %sable promisc mode.\n",
 			 promisc ? "en" : "dis");
 
-	sg_set_buf(&sg, &allmulti, sizeof(allmulti));
+	sg_set_buf(sg, &allmulti, sizeof(allmulti));
 
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
 				  VIRTIO_NET_CTRL_RX_ALLMULTI,
-				  &sg, 1, 0))
+				  sg, 1, 0))
 		dev_warn(&dev->dev, "Failed to %sable allmulti mode.\n",
 			 allmulti ? "en" : "dis");
+
+	/* MAC filter - use one buffer for both lists */
+	mac_data = buf = kzalloc(((dev->uc_count + dev->mc_count) * ETH_ALEN) +
+				 (2 * sizeof(mac_data->entries)), GFP_ATOMIC);
+	if (!buf) {
+		dev_warn(&dev->dev, "No memory for MAC address buffer\n");
+		return;
+	}
+
+	/* Store the unicast list and count in the front of the buffer */
+	mac_data->entries = dev->uc_count;
+	addr = dev->uc_list;
+	for (i = 0; i < dev->uc_count; i++, addr = addr->next)
+		memcpy(&mac_data->macs[i * ETH_ALEN], addr->da_addr, ETH_ALEN);
+
+	sg_set_buf(&sg[0], mac_data,
+		   sizeof(mac_data->entries) + (dev->uc_count * ETH_ALEN));
+
+	/* multicast list and count fill the end */
+	mac_data = (void *)&mac_data->macs[dev->uc_count * ETH_ALEN];
+
+	mac_data->entries = dev->mc_count;
+	addr = dev->mc_list;
+	for (i = 0; i < dev->mc_count; i++, addr = addr->next)
+		memcpy(&mac_data->macs[i * ETH_ALEN], addr->da_addr, ETH_ALEN);
+
+	sg_set_buf(&sg[1], mac_data,
+		   sizeof(mac_data->entries) + (dev->mc_count * ETH_ALEN));
+
+	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC,
+				  VIRTIO_NET_CTRL_MAC_TABLE_SET,
+				  sg, 2, 0))
+		dev_warn(&dev->dev, "Failed to set MAC fitler table.\n");
+
+	kfree(buf);
 }
 
 static struct ethtool_ops virtnet_ethtool_ops = {
diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index 69569e1..06d9dd6 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -77,7 +77,7 @@ typedef __u8 virtio_net_ctrl_ack;
 #define VIRTIO_NET_OK     0
 #define VIRTIO_NET_ERR    1
 
-#define VIRTIO_NET_MAX_CTRL_ARGS   3
+#define VIRTIO_NET_MAX_CTRL_ARGS   4
 
 /*
  * Control the RX mode, ie. promisucous and allmulti.  PROMISC and
@@ -89,4 +89,27 @@ typedef __u8 virtio_net_ctrl_ack;
  #define VIRTIO_NET_CTRL_RX_PROMISC      0
  #define VIRTIO_NET_CTRL_RX_ALLMULTI     1
 
+/*
+ * Control the MAC filter table.
+ *
+ * The MAC filter table is managed by the hypervisor, the guest should
+ * assume the size is infinite.  Filtering should be considered
+ * non-perfect, ie. based on hypervisor resources, the guest may
+ * received packets from sources not specified in the filter list.
+ *
+ * In addition to the class/cmd header, the TABLE_SET command requires
+ * two out scatterlists.  Each contains a 4 byte count of entries followed
+ * by a concatenated byte stream of the ETH_ALEN MAC addresses.  The
+ * first sg list contains unicast addresses, the second is for multicast.
+ * This functionality is present if the VIRTIO_NET_F_CTRL_RX feature
+ * is available.
+ */
+struct virtio_net_ctrl_mac {
+	__u32 entries;
+	__u8 macs[];
+} __attribute__((packed));
+
+#define VIRTIO_NET_CTRL_MAC    1
+ #define VIRTIO_NET_CTRL_MAC_TABLE_SET        0
+
 #endif /* _LINUX_VIRTIO_NET_H */

Re: [PATCH v3 3/4] virtio_net: Add a MAC filter table

[Rusty Russell]

On Monday 02 February 2009 06:35:20 Alex Williamson wrote:
> + * In addition to the class/cmd header, the TABLE_SET command requires
> + * two out scatterlists.  Each contains a 4 byte count of entries followed
> + * by a concatenated byte stream of the ETH_ALEN MAC addresses.  The
> + * first sg list contains unicast addresses, the second is for multicast.
> + * This functionality is present if the VIRTIO_NET_F_CTRL_RX feature
> + * is available.
> + */
> +struct virtio_net_ctrl_mac {
> +	__u32 entries;
> +	__u8 macs[];
> +} __attribute__((packed));

Hmm, can we make this more explicit?  eg.

struct virtio_net_ctrl_mac {
	__u32 unicast_entries, multicast_entriues;
	__u8 macs[ETH_ALEN][];
} __attribute__((packed));

Might simplify the code a tiny bit, too...

Thanks,
Rusty.

[PATCH v3 4/4] virtio_net: Add support for VLAN filtering in the hypervisor

[Alex Williamson]

VLAN filtering allows the hypervisor to drop packets from VLANs
that we're not a part of, further reducing the number of extraneous
packets recieved.  This makes use of the VLAN virtqueue command class.
The CTRL_VLAN feature bit tells us whether the backend supports VLAN
filtering.

Signed-off-by: Alex Williamson <alex.williamson@hp.com>
---

 drivers/net/virtio_net.c   |   31 ++++++++++++++++++++++++++++++-
 include/linux/virtio_net.h |   13 +++++++++++++
 2 files changed, 43 insertions(+), 1 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index e03eebf..d16107b 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -741,6 +741,30 @@ static void virtnet_set_rx_mode(struct net_device *dev)
 	kfree(buf);
 }
 
+static void virnet_vlan_rx_add_vid(struct net_device *dev, u16 vid)
+{
+	struct virtnet_info *vi = netdev_priv(dev);
+	struct scatterlist sg;
+
+	sg_set_buf(&sg, &vid, sizeof(vid));
+
+	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
+				  VIRTIO_NET_CTRL_VLAN_ADD, &sg, 1, 0))
+		dev_warn(&dev->dev, "Failed to add VLAN ID %d.\n", vid);
+}
+
+static void virnet_vlan_rx_kill_vid(struct net_device *dev, u16 vid)
+{
+	struct virtnet_info *vi = netdev_priv(dev);
+	struct scatterlist sg;
+
+	sg_set_buf(&sg, &vid, sizeof(vid));
+
+	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
+				  VIRTIO_NET_CTRL_VLAN_DEL, &sg, 1, 0))
+		dev_warn(&dev->dev, "Failed to kill VLAN ID %d.\n", vid);
+}
+
 static struct ethtool_ops virtnet_ethtool_ops = {
 	.set_tx_csum = virtnet_set_tx_csum,
 	.set_sg = ethtool_op_set_sg,
@@ -767,6 +791,8 @@ static const struct net_device_ops virtnet_netdev = {
 	.ndo_set_mac_address = virtnet_set_mac_address,
 	.ndo_set_rx_mode     = virtnet_set_rx_mode,
 	.ndo_change_mtu	     = virtnet_change_mtu,
+	.ndo_vlan_rx_add_vid = virnet_vlan_rx_add_vid,
+	.ndo_vlan_rx_kill_vid = virnet_vlan_rx_kill_vid,
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller = virtnet_netpoll,
 #endif
@@ -894,6 +920,9 @@ static int virtnet_probe(struct virtio_device *vdev)
 			err = PTR_ERR(vi->svq);
 			goto free_send;
 		}
+
+		if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VLAN))
+			dev->features |= NETIF_F_HW_VLAN_FILTER;
 	}
 
 	/* Initialize our empty receive and send queues. */
@@ -984,7 +1013,7 @@ static unsigned int features[] = {
 	VIRTIO_NET_F_HOST_ECN, VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6,
 	VIRTIO_NET_F_GUEST_ECN, /* We don't yet handle UFO input. */
 	VIRTIO_NET_F_MRG_RXBUF, VIRTIO_NET_F_STATUS, VIRTIO_NET_F_CTRL_VQ,
-	VIRTIO_NET_F_CTRL_RX,
+	VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN,
 	VIRTIO_F_NOTIFY_ON_EMPTY,
 };
 
diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index 06d9dd6..f22bb39 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -24,6 +24,7 @@
 #define VIRTIO_NET_F_STATUS	16	/* virtio_net_config.status available */
 #define VIRTIO_NET_F_CTRL_VQ	17	/* Control channel available */
 #define VIRTIO_NET_F_CTRL_RX	18	/* Control channel RX mode support */
+#define VIRTIO_NET_F_CTRL_VLAN	19	/* Control channel VLAN filtering */
 
 #define VIRTIO_NET_S_LINK_UP	1	/* Link is up */
 
@@ -112,4 +113,16 @@ struct virtio_net_ctrl_mac {
 #define VIRTIO_NET_CTRL_MAC    1
  #define VIRTIO_NET_CTRL_MAC_TABLE_SET        0
 
+/*
+ * Control VLAN filtering
+ *
+ * The VLAN filter table is controlled via a simple ADD/DEL interface.
+ * VLAN IDs not added may be filterd by the hypervisor.  Del is the
+ * opposite of add.  Both commands expect an out entry containing a 2
+ * byte VLAN ID.
+ */
+#define VIRTIO_NET_CTRL_VLAN       2
+ #define VIRTIO_NET_CTRL_VLAN_ADD             0
+ #define VIRTIO_NET_CTRL_VLAN_DEL             1
+
 #endif /* _LINUX_VIRTIO_NET_H */