[PATCH 2/2][RFC] virtio_net: Add MAC fitler table support

[PATCH 2/2][RFC] virtio_net: Add MAC fitler table support

[Alex Williamson]

virtio_net: Add MAC fitler table support

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

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


diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index f502edd..d751711 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -505,6 +505,8 @@ static void virtnet_set_rx_mode(struct net_device *dev)
 	struct virtnet_info *vi = netdev_priv(dev);
 	struct virtio_device *vdev = vi->vdev;
 	u16 status = vi->status.raw;
+	struct dev_addr_list *uc_ptr, *mc_ptr;
+	int i;
 
 	if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_STATUS))
 		return;
@@ -519,11 +521,55 @@ static void virtnet_set_rx_mode(struct net_device *dev)
 	else
 		status &= ~VIRTIO_NET_S_ALLMULTI;
 		
-	if (dev->uc_count)
+	if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_MAC_TABLE)) {
+		if (dev->uc_count)
+			status |= VIRTIO_NET_S_PROMISC;
+		if (dev->mc_count)
+			status |= VIRTIO_NET_S_ALLMULTI;
+		if (status != vi->status.raw) {
+			vi->status.raw = status;
+			vdev->config->set(vdev,
+					  offsetof(struct virtio_net_config,
+					  status), &vi->status,
+					  sizeof(vi->status));
+		}
+		return;
+	}
+
+	if (dev->uc_count > 16) {
 		status |= VIRTIO_NET_S_PROMISC;
-	if (dev->mc_count)
+		if (dev->mc_count > 16)
+			status |= VIRTIO_NET_S_ALLMULTI;
+	} else if (dev->uc_count + dev->mc_count > 16)
 		status |= VIRTIO_NET_S_ALLMULTI;
 
+	if ((dev->uc_count && !(status & VIRTIO_NET_S_PROMISC)) ||
+	    (dev->mc_count && !(status & VIRTIO_NET_S_ALLMULTI)))
+		status |= VIRTIO_NET_S_MAC_TABLE;
+	else
+		status &= ~VIRTIO_NET_S_MAC_TABLE;
+
+	uc_ptr = dev->uc_list;
+	mc_ptr = dev->mc_list;
+
+	for (i = 0; i < 16; i++) {
+		uint8_t entry[8] = { 0 };
+
+		if (uc_ptr && !(status & VIRTIO_NET_S_PROMISC)) {
+			memcpy(entry, uc_ptr->da_addr, 6);
+			entry[7] = 1;
+			uc_ptr = uc_ptr->next;
+		} else if (mc_ptr && !(status & VIRTIO_NET_S_ALLMULTI)) {
+			memcpy(entry, mc_ptr->da_addr, 6);
+			entry[7] = 1;
+			mc_ptr = mc_ptr->next;
+		}
+
+		vdev->config->set(vdev, offsetof(struct virtio_net_config,
+				  mac_table) + (sizeof(entry) * i),
+				  &entry, sizeof(entry));
+	}
+
 	if (status != vi->status.raw) {
 		vi->status.raw = status;
 		vdev->config->set(vdev, offsetof(struct virtio_net_config,
@@ -744,7 +790,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_STATUS,
+	VIRTIO_NET_F_STATUS, VIRTIO_NET_F_MAC_TABLE,
 	VIRTIO_F_NOTIFY_ON_EMPTY,
 };
 
diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index 5a70edb..905319b 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -21,10 +21,12 @@
 #define VIRTIO_NET_F_HOST_ECN	13	/* Host can handle TSO[6] w/ ECN in. */
 #define VIRTIO_NET_F_HOST_UFO	14	/* Host can handle UFO in. */
 #define VIRTIO_NET_F_STATUS	16	/* virtio_net_config.status available */
+#define VIRTIO_NET_F_MAC_TABLE	17	/* Additional MAC addresses */
 
 #define VIRTIO_NET_S_LINK_UP	1	/* Link is up */
 #define VIRTIO_NET_S_PROMISC	2	/* Promiscuous mode */
 #define VIRTIO_NET_S_ALLMULTI	4	/* All-multicast mode */
+#define VIRTIO_NET_S_MAC_TABLE	8	/* Enable MAC filter table */
 
 struct virtio_net_config
 {
@@ -38,8 +40,10 @@ struct virtio_net_config
 			__u16 link:1;
 			__u16 promisc:1;
 			__u16 allmulti:1;
+			__u16 mac_table:1;
 		} bits;
-       } status;
+	} status;
+	__u64 mac_table[16];
 } __attribute__((packed));
 
 /* This is the first element of the scatter-gather list.  If you don't

Re: [PATCH 2/2][RFC] virtio_net: Add MAC fitler table support

[Mark McLoughlin]

On Wed, 2009-01-07 at 11:06 -0700, Alex Williamson wrote:
> virtio_net: Add MAC fitler table support
> 
> Signed-off-by: Alex Williamson <alex.williamson@hp.com>
> ---
> 
>  drivers/net/virtio_net.c   |   52 +++++++++++++++++++++++++++++++++++++++++---
>  include/linux/virtio_net.h |    6 ++++-
>  2 files changed, 54 insertions(+), 4 deletions(-)
> 
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index f502edd..d751711 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -505,6 +505,8 @@ static void virtnet_set_rx_mode(struct net_device *dev)
>  	struct virtnet_info *vi = netdev_priv(dev);
>  	struct virtio_device *vdev = vi->vdev;
>  	u16 status = vi->status.raw;
> +	struct dev_addr_list *uc_ptr, *mc_ptr;
> +	int i;
>  
>  	if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_STATUS))
>  		return;
> @@ -519,11 +521,55 @@ static void virtnet_set_rx_mode(struct net_device *dev)
>  	else
>  		status &= ~VIRTIO_NET_S_ALLMULTI;
>  		
> -	if (dev->uc_count)
> +	if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_MAC_TABLE)) {
> +		if (dev->uc_count)
> +			status |= VIRTIO_NET_S_PROMISC;
> +		if (dev->mc_count)
> +			status |= VIRTIO_NET_S_ALLMULTI;

Maybe a goto set_status here?

> +		if (status != vi->status.raw) {
> +			vi->status.raw = status;
> +			vdev->config->set(vdev,
> +					  offsetof(struct virtio_net_config,
> +					  status), &vi->status,
> +					  sizeof(vi->status));
> +		}
> +		return;
> +	}
> +
> +	if (dev->uc_count > 16) {
>  		status |= VIRTIO_NET_S_PROMISC;
> -	if (dev->mc_count)
> +		if (dev->mc_count > 16)
> +			status |= VIRTIO_NET_S_ALLMULTI;
> +	} else if (dev->uc_count + dev->mc_count > 16)
>  		status |= VIRTIO_NET_S_ALLMULTI;
>  
> +	if ((dev->uc_count && !(status & VIRTIO_NET_S_PROMISC)) ||
> +	    (dev->mc_count && !(status & VIRTIO_NET_S_ALLMULTI)))
> +		status |= VIRTIO_NET_S_MAC_TABLE;
> +	else
> +		status &= ~VIRTIO_NET_S_MAC_TABLE;
> +
> +	uc_ptr = dev->uc_list;
> +	mc_ptr = dev->mc_list;
> +
> +	for (i = 0; i < 16; i++) {
> +		uint8_t entry[8] = { 0 };
> +
> +		if (uc_ptr && !(status & VIRTIO_NET_S_PROMISC)) {
> +			memcpy(entry, uc_ptr->da_addr, 6);

Use ETH_ALEN.

> +			entry[7] = 1;
> +			uc_ptr = uc_ptr->next;
> +		} else if (mc_ptr && !(status & VIRTIO_NET_S_ALLMULTI)) {
> +			memcpy(entry, mc_ptr->da_addr, 6);
> +			entry[7] = 1;
> +			mc_ptr = mc_ptr->next;
> +		}
> +
> +		vdev->config->set(vdev, offsetof(struct virtio_net_config,
> +				  mac_table) + (sizeof(entry) * i),
> +				  &entry, sizeof(entry));
> +	}
> +
>  	if (status != vi->status.raw) {
>  		vi->status.raw = status;
>  		vdev->config->set(vdev, offsetof(struct virtio_net_config,
> @@ -744,7 +790,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_STATUS,
> +	VIRTIO_NET_F_STATUS, VIRTIO_NET_F_MAC_TABLE,
>  	VIRTIO_F_NOTIFY_ON_EMPTY,
>  };
>  
> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index 5a70edb..905319b 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -21,10 +21,12 @@
>  #define VIRTIO_NET_F_HOST_ECN	13	/* Host can handle TSO[6] w/ ECN in. */
>  #define VIRTIO_NET_F_HOST_UFO	14	/* Host can handle UFO in. */
>  #define VIRTIO_NET_F_STATUS	16	/* virtio_net_config.status available */
> +#define VIRTIO_NET_F_MAC_TABLE	17	/* Additional MAC addresses */
>  
>  #define VIRTIO_NET_S_LINK_UP	1	/* Link is up */
>  #define VIRTIO_NET_S_PROMISC	2	/* Promiscuous mode */
>  #define VIRTIO_NET_S_ALLMULTI	4	/* All-multicast mode */
> +#define VIRTIO_NET_S_MAC_TABLE	8	/* Enable MAC filter table */
>  
>  struct virtio_net_config
>  {
> @@ -38,8 +40,10 @@ struct virtio_net_config
>  			__u16 link:1;
>  			__u16 promisc:1;
>  			__u16 allmulti:1;
> +			__u16 mac_table:1;
>  		} bits;
> -       } status;
> +	} status;
> +	__u64 mac_table[16];

You're using two bytes per entry to indicate the flag is valid. Why not
an array of 6 byte entries with a count of how many entries are valid?

That would also keep the virtio-net I/O space under 128 bytes.

Cheers,
Mark.

Re: [PATCH 2/2][RFC] virtio_net: Add MAC fitler table support

[Alex Williamson]

On Fri, 2009-01-09 at 11:34 +0000, Mark McLoughlin wrote:
> On Wed, 2009-01-07 at 11:06 -0700, Alex Williamson wrote:
> > @@ -38,8 +40,10 @@ struct virtio_net_config
> >  			__u16 link:1;
> >  			__u16 promisc:1;
> >  			__u16 allmulti:1;
> > +			__u16 mac_table:1;
> >  		} bits;
> > -       } status;
> > +	} status;
> > +	__u64 mac_table[16];
> 
> You're using two bytes per entry to indicate the flag is valid. Why
> not
> an array of 6 byte entries with a count of how many entries are valid?
> 
> That would also keep the virtio-net I/O space under 128 bytes.

Thanks for the comments, they look correct.  For the mac_table, that's
exactly how I'm thinking of doing it with virt-queues.  The problem with
that here is that the driver has direct access to the table and doesn't
necessarily have to make it contiguous.  If we expose a table, I think
the more space efficient way is to make the table size a multiple of 8
and have a bitmap of valid entries.

The virt-queue implementation I'm thinking of has 2 interfaces, ALLOC
and SET.  ALLOC can only be called once by the guest driver and
allocates a mac filter table supporting the given number of entries.
The SET interface provides a contiguous list of MACs, fitting into the
size previously allocated.  SET can also be used to clear if called with
no entries.  I'm steering away from a dynamically changeable size as
that implies locking or some kind of linked list implementation, or
both.  Thanks,

Alex

-- 
Alex Williamson                             HP Open Source & Linux Org.

Re: [PATCH 2/2][RFC] virtio_net: Add MAC fitler table support

[Rusty Russell]

On Thursday 08 January 2009 04:36:03 Alex Williamson wrote:
> virtio_net: Add MAC fitler table support

Ah, I see.  You really want multiple mac addresses, not just multicast
filtering?

Anthony, you think a control channel?  We can add a virtqueue, but it seems like a lot of work...

Rusty.

Re: [PATCH 2/2][RFC] virtio_net: Add MAC fitler table support

[Alex Williamson]

On Sat, 2009-01-10 at 21:48 +1030, Rusty Russell wrote:
> On Thursday 08 January 2009 04:36:03 Alex Williamson wrote:
> > virtio_net: Add MAC fitler table support
> 
> Ah, I see.  You really want multiple mac addresses, not just multicast
> filtering?
> 
> Anthony, you think a control channel?  We can add a virtqueue, but it
> seems like a lot of work...

Right, a MAC filter table followed by a VLAN filter table.  I've got a
virtqueue control channel mostly coded up, I should have something to
send out early next week for comments.  Thanks,

Alex

-- 
Alex Williamson                             HP Open Source & Linux Org.

Re: [PATCH 2/2][RFC] virtio_net: Add MAC fitler table support

[Anthony Liguori]

Rusty Russell wrote:
> On Thursday 08 January 2009 04:36:03 Alex Williamson wrote:
>   
>> virtio_net: Add MAC fitler table support
>>     
>
> Ah, I see.  You really want multiple mac addresses, not just multicast
> filtering?
>
> Anthony, you think a control channel?  We can add a virtqueue, but it seems like a lot of work...
>   

I think it's the only way to solve the problem in a virtio friendly way.

Another option would be extending the config space by a very large 
size.  We would have to make some changes to virtio-pci to switch to 
MMIO but that's easy enough.  From a high level perspective, I don't 
like the idea of having the config space be extremely large and used as 
a communication mechanism between guests.  It really should be for 
device configuration data that's relatively static.  I think we abuse 
the config space in the virtio-balloon driver.

Ideally, you'd have an area of guest memory sized by the guest (so there 
was no intrinsic limit on table size) that was given to the host to use 
as the filter tables.  The only way this works with virtio is if you 
send this over a virtqueue in the form of messages.  You could write a 
pfn to the config space but then you lose all the mapping/unmapping 
abstraction that virtqueue gives you (even though we don't do anything 
useful with that abstraction today :-)).

So yeah, I think a control queue is the way to go.

Regards,

Anthony Liguori

> Rusty.
>   

Re: [PATCH 2/2][RFC] virtio_net: Add MAC fitler table support

[Alex Williamson]

On Sat, 2009-01-10 at 12:18 -0600, Anthony Liguori wrote:
> Ideally, you'd have an area of guest memory sized by the guest (so there 
> was no intrinsic limit on table size) that was given to the host to use 
> as the filter tables.  The only way this works with virtio is if you 
> send this over a virtqueue in the form of messages.  You could write a 
> pfn to the config space but then you lose all the mapping/unmapping 
> abstraction that virtqueue gives you (even though we don't do anything 
> useful with that abstraction today :-)).

Hmm, that's not quite how I was implementing it.  The uc_list and
mc_list are stored up in the netdev level, so there's not much point in
duplicating it in the guest virtio-net driver.  The interface I was
working on has two commands.  The first tells the host to allocate the
MAC filter table for a guest provided number of entries (perhaps a
module parameter, with reasonable default).  The other is a set command
with an sg entry providing a buffer of all the MAC entries for the
table.  If sg entries are no more than a page, this limits us to ~680
MAC table entries, which I think is far more than any piece of real
hardware (and large enough that you'd probably want to turn on
promiscuous already).  The VLAN equivalent is a bit easier since by
definition there are 4k possible VLANs.  There I think a set bit/clear
bit message interface is appropriate (and maybe a clear all for a reset
condition).  Let me know if that sounds reasonable.  Thanks,

Alex

-- 
Alex Williamson                             HP Open Source & Linux Org.

Re: [PATCH 2/2][RFC] virtio_net: Add MAC fitler table support

[Anthony Liguori]

Alex Williamson wrote:
> Hmm, that's not quite how I was implementing it.  The uc_list and
> mc_list are stored up in the netdev level, so there's not much point in
> duplicating it in the guest virtio-net driver.  The interface I was
> working on has two commands.  The first tells the host to allocate the
> MAC filter table for a guest provided number of entries (perhaps a
> module parameter, with reasonable default).  The other is a set command
> with an sg entry providing a buffer of all the MAC entries for the
> table.  If sg entries are no more than a page, this limits us to ~680
> MAC table entries, which I think is far more than any piece of real
> hardware (and large enough that you'd probably want to turn on
> promiscuous already).

Yeah, this is what I would have done although maybe it's worth allowing 
a partial update of the filter table.  Once you're using a command 
interface, a protocol like you describe makes sense.  I was simply going 
the through the logic that led me to suggest a command interface in the 
first place.

>   The VLAN equivalent is a bit easier since by
> definition there are 4k possible VLANs.  There I think a set bit/clear
> bit message interface is appropriate (and maybe a clear all for a reset
> condition).  Let me know if that sounds reasonable.  Thanks,
>   

Yeah, sounds reasonable to me.

Regards,

Anthony Liguori

> Alex
>
>