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

[Mark McLoughlin <markmc@redhat.com>]

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.