diff mbox

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

Message ID 20090129230518.2672.20841.stgit@debian.lart (mailing list archive)
State Accepted, archived
Headers show

Commit Message

Alex Williamson Jan. 29, 2009, 11:05 p.m. UTC 
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(-)


--
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

Comments

Rusty Russell Jan. 30, 2009, 5:46 a.m. UTC | #1 
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.
--
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
diff mbox

Patch

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 */