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

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

[Alex Williamson]

Number of entries configurable via module param.

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

 drivers/net/virtio_net.c   |   71 +++++++++++++++++++++++++++++++++++++++++++-
 include/linux/virtio_net.h |    4 ++
 2 files changed, 73 insertions(+), 2 deletions(-)


diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index b18dd4c..1f021c4 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -32,6 +32,11 @@ static int csum = 1, gso = 1;
 module_param(csum, bool, 0444);
 module_param(gso, bool, 0444);
 
+static unsigned int mac_entries = 16;
+module_param(mac_entries, uint, 0444);
+MODULE_PARM_DESC(mac_entries,
+	"Number of entries in the MAC filter table.");
+
 /* FIXME: MTU in config. */
 #define MAX_PACKET_LEN (ETH_HLEN+ETH_DATA_LEN)
 #define GOOD_COPY_LEN	128
@@ -669,9 +674,61 @@ static void virtnet_set_rx_mode(struct net_device *dev)
 	struct virtnet_info *vi = netdev_priv(dev);
 	u8 promisc, allmulti;
 
-	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 (dev->uc_count > mac_entries) {
+		promisc = 1;
+		if (dev->mc_count > mac_entries)
+			allmulti = 1;
+	} else if (dev->uc_count + dev->mc_count > mac_entries)
+		allmulti = 1;
+
+	if (!promisc && (dev->uc_count || (dev->mc_count && !allmulti))) {
+		u8 *buf, *cur;
+		int count, i;
+		struct dev_addr_list *uc_ptr, *mc_ptr;
+
+		count = dev->uc_count + (allmulti ? 0 : dev->mc_count);
 
+		buf = kzalloc(count * ETH_ALEN, GFP_ATOMIC);
+		if (!buf) {
+			promisc = 1;
+			goto set_status;
+		}
+
+		cur = buf;
+		uc_ptr = dev->uc_list;
+		mc_ptr = dev->mc_list;
+
+		for (i = 0; i < dev->uc_count; i++) {
+			memcpy(cur, uc_ptr->da_addr, ETH_ALEN);
+			cur += ETH_ALEN;
+			uc_ptr = uc_ptr->next;
+		}
+		if (!allmulti) {
+			for (i = 0; i < dev->mc_count; i++) {
+				memcpy(cur, mc_ptr->da_addr, ETH_ALEN);
+				cur += ETH_ALEN;
+				mc_ptr = mc_ptr->next;
+			}
+		}
+		if (virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC_TABLE,
+					 VIRTIO_NET_CTRL_MAC_TABLE_SET,
+					 buf, count * ETH_ALEN)) {
+			printk(KERN_WARNING "%s: failed to program MAC filter "
+			       "table, running in promiscuous mode.\n",
+			       dev->name);
+			promisc = 1;
+		}
+		kfree(buf);
+	} else {
+		/* Set an empty MAC table - disabled */
+		virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC_TABLE,
+				     VIRTIO_NET_CTRL_MAC_TABLE_SET, NULL, 0);
+	}
+
+set_status:
 	virtnet_send_command(vi, VIRTIO_NET_CTRL_RX_MODE,
 			     VIRTIO_NET_CTRL_RX_MODE_PROMISC,
 			     &promisc, sizeof(promisc));
@@ -799,6 +856,16 @@ static int virtnet_probe(struct virtio_device *vdev)
 	vi->cvq = vdev->config->find_vq(vdev, 2, NULL);
 	if (IS_ERR(vi->cvq))
 		vi->cvq = NULL;
+	else {
+		unsigned int entries;
+
+		entries = mac_entries = min(mac_entries,
+					(unsigned int)(PAGE_SIZE / ETH_ALEN));
+		if (virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC_TABLE,
+					 VIRTIO_NET_CTRL_MAC_TABLE_ALLOC,
+					 &entries, sizeof(entries)))
+			mac_entries = 0;
+	}
 
 	/* Initialize our empty receive and send queues. */
 	skb_queue_head_init(&vi->recv);
diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index 80cd7d3..31235a0 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -60,4 +60,8 @@ struct virtio_net_hdr_mrg_rxbuf {
  #define VIRTIO_NET_CTRL_RX_MODE_PROMISC      0
  #define VIRTIO_NET_CTRL_RX_MODE_ALLMULTI     1
 
+#define VIRTIO_NET_CTRL_MAC_TABLE  1
+ #define VIRTIO_NET_CTRL_MAC_TABLE_ALLOC      0
+ #define VIRTIO_NET_CTRL_MAC_TABLE_SET        1
+
 #endif /* _LINUX_VIRTIO_NET_H */

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

[Mark McLoughlin]

On Tue, 2009-01-13 at 14:23 -0700, Alex Williamson wrote:
> Number of entries configurable via module param.

Could do with some more details here, like explaining that it means the
we no longer run the NIC in promiscuous mode and the guest only receives
packets destined for it.

> Signed-off-by: Alex Williamson <alex.williamson@hp.com
> ---
> 
>  drivers/net/virtio_net.c   |   71 +++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/virtio_net.h |    4 ++
>  2 files changed, 73 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index b18dd4c..1f021c4 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -32,6 +32,11 @@ static int csum = 1, gso = 1;
>  module_param(csum, bool, 0444);
>  module_param(gso, bool, 0444);
>  
> +static unsigned int mac_entries = 16;
> +module_param(mac_entries, uint, 0444);
> +MODULE_PARM_DESC(mac_entries,
> +	"Number of entries in the MAC filter table.");

Not sure it needs to be configurable? No harm, I guess.

>  /* FIXME: MTU in config. */
>  #define MAX_PACKET_LEN (ETH_HLEN+ETH_DATA_LEN)
>  #define GOOD_COPY_LEN	128
> @@ -669,9 +674,61 @@ static void virtnet_set_rx_mode(struct net_device *dev)
>  	struct virtnet_info *vi = netdev_priv(dev);
>  	u8 promisc, allmulti;
>  
> -	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 (dev->uc_count > mac_entries) {
> +		promisc = 1;
> +		if (dev->mc_count > mac_entries)
> +			allmulti = 1;
> +	} else if (dev->uc_count + dev->mc_count > mac_entries)
> +		allmulti = 1;
> +
> +	if (!promisc && (dev->uc_count || (dev->mc_count && !allmulti))) {
> +		u8 *buf, *cur;
> +		int count, i;
> +		struct dev_addr_list *uc_ptr, *mc_ptr;
> +
> +		count = dev->uc_count + (allmulti ? 0 : dev->mc_count);
>  
> +		buf = kzalloc(count * ETH_ALEN, GFP_ATOMIC);
> +		if (!buf) {
> +			promisc = 1;
> +			goto set_status;

s/set_status/set_mode/ maybe

> +		}
> +
> +		cur = buf;
> +		uc_ptr = dev->uc_list;
> +		mc_ptr = dev->mc_list;
> +
> +		for (i = 0; i < dev->uc_count; i++) {
> +			memcpy(cur, uc_ptr->da_addr, ETH_ALEN);
> +			cur += ETH_ALEN;
> +			uc_ptr = uc_ptr->next;
> +		}
> +		if (!allmulti) {
> +			for (i = 0; i < dev->mc_count; i++) {
> +				memcpy(cur, mc_ptr->da_addr, ETH_ALEN);
> +				cur += ETH_ALEN;
> +				mc_ptr = mc_ptr->next;
> +			}
> +		}
> +		if (virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC_TABLE,
> +					 VIRTIO_NET_CTRL_MAC_TABLE_SET,
> +					 buf, count * ETH_ALEN)) {
> +			printk(KERN_WARNING "%s: failed to program MAC filter "
> +			       "table, running in promiscuous mode.\n",

Any way to re-jig that so the grepable text is all on one line?

> +			       dev->name);
> +			promisc = 1;
> +		}
> +		kfree(buf);
> +	} else {
> +		/* Set an empty MAC table - disabled */
> +		virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC_TABLE,
> +				     VIRTIO_NET_CTRL_MAC_TABLE_SET, NULL, 0);
> +	}
> +
> +set_status:
>  	virtnet_send_command(vi, VIRTIO_NET_CTRL_RX_MODE,
>  			     VIRTIO_NET_CTRL_RX_MODE_PROMISC,
>  			     &promisc, sizeof(promisc));
> @@ -799,6 +856,16 @@ static int virtnet_probe(struct virtio_device *vdev)
>  	vi->cvq = vdev->config->find_vq(vdev, 2, NULL);
>  	if (IS_ERR(vi->cvq))
>  		vi->cvq = NULL;
> +	else {
> +		unsigned int entries;
> +
> +		entries = mac_entries = min(mac_entries,
> +					(unsigned int)(PAGE_SIZE / ETH_ALEN));
> +		if (virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC_TABLE,
> +					 VIRTIO_NET_CTRL_MAC_TABLE_ALLOC,
> +					 &entries, sizeof(entries)))
> +			mac_entries = 0;
> +	}

Should have a warning here.

Also, no need for the extra variable or the case, is there?

>  	/* Initialize our empty receive and send queues. */
>  	skb_queue_head_init(&vi->recv);
> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index 80cd7d3..31235a0 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -60,4 +60,8 @@ struct virtio_net_hdr_mrg_rxbuf {
>   #define VIRTIO_NET_CTRL_RX_MODE_PROMISC      0
>   #define VIRTIO_NET_CTRL_RX_MODE_ALLMULTI     1
>  
> +#define VIRTIO_NET_CTRL_MAC_TABLE  1
> + #define VIRTIO_NET_CTRL_MAC_TABLE_ALLOC      0
> + #define VIRTIO_NET_CTRL_MAC_TABLE_SET        1

I'd prefer:

#define VIRTIO_NET_CMD_ALLOC_MAC_TABLE 1
#define VIRTIO_NET_CMD_SET_MAC_TABLE   2

Also, could do with some comments here to note e.g.:

  - alloc before set
  - subsequent allocs will fail
  - table limited to PAGE_SIZE (hmm, who's PAGE_SIZE?)
  - format of the table

Cheers,
Mark.

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

[Alex Williamson]

On Wed, 2009-01-14 at 10:15 +0000, Mark McLoughlin wrote:
> On Tue, 2009-01-13 at 14:23 -0700, Alex Williamson wrote:
> > Number of entries configurable via module param.
> 
> Could do with some more details here, like explaining that it means the
> we no longer run the NIC in promiscuous mode and the guest only receives
> packets destined for it.

Yep, thanks.

> > +	else {
> > +		unsigned int entries;
> > +
> > +		entries = mac_entries = min(mac_entries,
> > +					(unsigned int)(PAGE_SIZE / ETH_ALEN));
> > +		if (virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC_TABLE,
> > +					 VIRTIO_NET_CTRL_MAC_TABLE_ALLOC,
> > +					 &entries, sizeof(entries)))
> > +			mac_entries = 0;
> > +	}
> 
> Should have a warning here.

I agree.

> Also, no need for the extra variable or the case, is there?

Initially, that's what I thought too... but it doesn't work, I always
got 0 on the backend.  I assume it has something to do with the memory
segment mac_entries lives in as a static variable.

> >  	/* Initialize our empty receive and send queues. */
> >  	skb_queue_head_init(&vi->recv);
> > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> > index 80cd7d3..31235a0 100644
> > --- a/include/linux/virtio_net.h
> > +++ b/include/linux/virtio_net.h
> > @@ -60,4 +60,8 @@ struct virtio_net_hdr_mrg_rxbuf {
> >   #define VIRTIO_NET_CTRL_RX_MODE_PROMISC      0
> >   #define VIRTIO_NET_CTRL_RX_MODE_ALLMULTI     1
> >  
> > +#define VIRTIO_NET_CTRL_MAC_TABLE  1
> > + #define VIRTIO_NET_CTRL_MAC_TABLE_ALLOC      0
> > + #define VIRTIO_NET_CTRL_MAC_TABLE_SET        1
> 
> I'd prefer:
> 
> #define VIRTIO_NET_CMD_ALLOC_MAC_TABLE 1
> #define VIRTIO_NET_CMD_SET_MAC_TABLE   2

Hmm, I'm still leaning towards the class/cmd, which would allow the
backend to logically split class commands into sub-functions.  But i'd
be happy to rename these to VIRTIO_NET_CMD_...

> Also, could do with some comments here to note e.g.:
> 
>   - alloc before set
>   - subsequent allocs will fail
>   - table limited to PAGE_SIZE (hmm, who's PAGE_SIZE?)
>   - format of the table

Agree, thanks for the comments.

Alex

-- 
Alex Williamson                             HP Open Source & Linux Org.

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

[Mark McLoughlin]

On Wed, 2009-01-14 at 09:20 -0700, Alex Williamson wrote:
> > Also, no need for the extra variable or the case, is there?
> 
> Initially, that's what I thought too... but it doesn't work, I always
> got 0 on the backend.  I assume it has something to do with the memory
> segment mac_entries lives in as a static variable.

Bizarre - it'd be good to understand exactly why. Add a comment to that
effect, at least.

Cheers,
Mark.