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

Previous thread: pull request: wireless-2.6 2009-01-29 by John W. Linville on Thursday, January 29, 2009 - 2:27 pm. (2 messages)

Next thread: tun new missing ioctl32 by Michael Tokarev on Thursday, January 29, 2009 - 4:43 pm. (3 messages)
From: Alex Williamson
Date: Thursday, January 29, 2009 - 4:05 pm

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

From: Alex Williamson
Date: Thursday, January 29, 2009 - 4:05 pm

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 ...
From: Rusty Russell
Date: Thursday, January 29, 2009 - 10:08 pm

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


Or "return status == VIRTIO_NET_OK;" ?

See what I mean about nit-picking?
Rusty.
--

From: Alex Williamson
Date: Thursday, January 29, 2009 - 4:05 pm

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   ...
From: Rusty Russell
Date: Thursday, January 29, 2009 - 10:30 pm

Hmm, this check duplicates the one in virtnet_send_command; after


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

Cheers,
Rusty.
--

From: Alex Williamson
Date: Thursday, January 29, 2009 - 4:05 pm

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, ...
From: Rusty Russell
Date: Thursday, January 29, 2009 - 10:46 pm

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



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

From: Alex Williamson
Date: Thursday, January 29, 2009 - 4:05 pm

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);
 ...
From: Rusty Russell
Date: Thursday, January 29, 2009 - 11:01 pm

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

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

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

Cheers,
Rusty.
--

From: David Miller
Date: Thursday, January 29, 2009 - 6:49 pm

From: Alex Williamson <alex.williamson@hp.com>

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

Thanks.
--

From: Rusty Russell
Date: Friday, January 30, 2009 - 12:05 am

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

From: David Miller
Date: Friday, January 30, 2009 - 12:12 am

From: Rusty Russell <rusty@rustcorp.com.au>


Right, no hurry.
--

From: Rusty Russell
Date: Thursday, January 29, 2009 - 10:03 pm

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

From: Alex Williamson
Date: Sunday, February 1, 2009 - 1:05 pm

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

From: Alex Williamson
Date: Sunday, February 1, 2009 - 1:05 pm

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 = {
 ...
From: Rusty Russell
Date: Monday, February 2, 2009 - 2:52 am

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

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

From: Alex Williamson
Date: Monday, February 2, 2009 - 2:34 pm

Hi Rusty,


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

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

Alex

-- 
Alex Williamson                             HP Open Source & Linux Org.

--

From: Rusty Russell
Date: Monday, February 2, 2009 - 7:54 pm

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


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


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

Thanks,
Rusty.
--

From: Alex Williamson
Date: Sunday, February 1, 2009 - 1:05 pm

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 ...
From: Rusty Russell
Date: Monday, February 2, 2009 - 2:40 am

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

From: Anthony Liguori
Date: Monday, February 2, 2009 - 7:10 am

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


--

From: Alex Williamson
Date: Sunday, February 1, 2009 - 1:05 pm

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,
 ...
From: Alex Williamson
Date: Sunday, February 1, 2009 - 1:05 pm

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,
 				  ...
From: Rusty Russell
Date: Monday, February 2, 2009 - 2:57 am

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

Previous thread: pull request: wireless-2.6 2009-01-29 by John W. Linville on Thursday, January 29, 2009 - 2:27 pm. (2 messages)

Next thread: tun new missing ioctl32 by Michael Tokarev on Thursday, January 29, 2009 - 4:43 pm. (3 messages)