[PATCH 1/2][RFC] virtio_net: Enable setting MAC, promisc, and allmulti mode

[PATCH 1/2][RFC] virtio_net: Enable setting MAC, promisc, and allmulti mode

[Alex Williamson]

virtio_net: Enable setting MAC, promisc, and allmulti mode

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

 drivers/net/virtio_net.c   |   79 ++++++++++++++++++++++++++++++++++++++++----
 include/linux/virtio_net.h |   11 ++++++
 2 files changed, 82 insertions(+), 8 deletions(-)


diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 3af5e33..f502edd 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -41,7 +41,14 @@ struct virtnet_info
 	struct virtqueue *rvq, *svq;
 	struct net_device *dev;
 	struct napi_struct napi;
-	unsigned int status;
+	union {
+		u16 raw;
+		struct {
+			u16 link:1;
+			u16 promisc:1;
+			u16 allmulti:1;
+		} bits;
+	} status; 
 
 	/* The skb we couldn't send because buffers were full. */
 	struct sk_buff *last_xmit_skb;
@@ -476,6 +483,54 @@ static int virtnet_set_tx_csum(struct net_device *dev, u32 data)
 	return ethtool_op_set_tx_hw_csum(dev, data);
 }
 
+static int virtnet_set_mac_address(struct net_device *dev, void *p)
+{
+	struct virtnet_info *vi = netdev_priv(dev);
+	struct virtio_device *vdev = vi->vdev;
+	struct sockaddr *addr = p;
+
+	if (!is_valid_ether_addr(addr->sa_data))
+		return -EADDRNOTAVAIL;
+
+	memcpy(dev->dev_addr, addr->sa_data, dev->addr_len);
+
+	vdev->config->set(vdev, offsetof(struct virtio_net_config, mac),
+			  dev->dev_addr, dev->addr_len);
+
+	return 0;
+}
+
+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;
+
+	if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_STATUS))
+		return;
+
+	if (dev->flags & IFF_PROMISC)
+		status |= VIRTIO_NET_S_PROMISC;
+	else
+		status &= ~VIRTIO_NET_S_PROMISC;
+
+	if (dev->flags & IFF_ALLMULTI)
+		status |= VIRTIO_NET_S_ALLMULTI;
+	else
+		status &= ~VIRTIO_NET_S_ALLMULTI;
+		
+	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));
+	}
+}
+
 static struct ethtool_ops virtnet_ethtool_ops = {
 	.set_tx_csum = virtnet_set_tx_csum,
 	.set_sg = ethtool_op_set_sg,
@@ -494,14 +549,15 @@ static void virtnet_update_status(struct virtnet_info *vi)
                               &v, sizeof(v));
 
 	/* Ignore unknown (future) status bits */
-	v &= VIRTIO_NET_S_LINK_UP;
+	v &= VIRTIO_NET_S_LINK_UP | VIRTIO_NET_S_PROMISC |
+             VIRTIO_NET_S_ALLMULTI;
 
-	if (vi->status == v)
+	if (vi->status.raw == v)
 		return;
 
-	vi->status = v;
+	vi->status.raw = v;
 
-	if (vi->status & VIRTIO_NET_S_LINK_UP) {
+	if (vi->status.bits.link) {
 		netif_carrier_on(vi->dev);
 		netif_wake_queue(vi->dev);
 	} else {
@@ -563,8 +619,17 @@ static int virtnet_probe(struct virtio_device *vdev)
 		vdev->config->get(vdev,
 				  offsetof(struct virtio_net_config, mac),
 				  dev->dev_addr, dev->addr_len);
-	} else
+	} else {
+		struct sockaddr addr;
+
 		random_ether_addr(dev->dev_addr);
+		memset(&addr, 0, sizeof(addr));
+		memcpy(&addr.sa_data, dev->dev_addr, dev->addr_len);
+		virtnet_set_mac_address(dev, &addr);
+	}
+
+	dev->set_mac_address = virtnet_set_mac_address;
+	dev->set_rx_mode = virtnet_set_rx_mode;
 
 	/* Set up our device-specific information */
 	vi = netdev_priv(dev);
@@ -621,7 +686,7 @@ static int virtnet_probe(struct virtio_device *vdev)
 		goto unregister;
 	}
 
-	vi->status = VIRTIO_NET_S_LINK_UP;
+	vi->status.raw = VIRTIO_NET_S_LINK_UP;
 	virtnet_update_status(vi);
 
 	pr_debug("virtnet: registered device %s\n", dev->name);
diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index d9174be..5a70edb 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -23,6 +23,8 @@
 #define VIRTIO_NET_F_STATUS	16	/* virtio_net_config.status available */
 
 #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 */
 
 struct virtio_net_config
 {
@@ -30,7 +32,14 @@ struct virtio_net_config
 	__u8 mac[6];
 	/* Status supplied by host; see VIRTIO_NET_F_STATUS and VIRTIO_NET_S_*
 	 * bits above */
-	__u16 status;
+	union {
+		__u16 raw;
+		struct {
+			__u16 link:1;
+			__u16 promisc:1;
+			__u16 allmulti:1;
+		} bits;
+       } status;
 } __attribute__((packed));
 
 /* This is the first element of the scatter-gather list.  If you don't

Re: [PATCH 1/2][RFC] virtio_net: Enable setting MAC, promisc, and allmulti mode

[Mark McLoughlin]

On Wed, 2009-01-07 at 11:06 -0700, Alex Williamson wrote:
> virtio_net: Enable setting MAC, promisc, and allmulti mode
> 
> Signed-off-by: Alex Williamson <alex.williamson@hp.com>
> ---
> 
>  drivers/net/virtio_net.c   |   79 ++++++++++++++++++++++++++++++++++++++++----
>  include/linux/virtio_net.h |   11 ++++++
>  2 files changed, 82 insertions(+), 8 deletions(-)
> 
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 3af5e33..f502edd 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -41,7 +41,14 @@ struct virtnet_info
>  	struct virtqueue *rvq, *svq;
>  	struct net_device *dev;
>  	struct napi_struct napi;
> -	unsigned int status;
> +	union {
> +		u16 raw;
> +		struct {
> +			u16 link:1;
> +			u16 promisc:1;
> +			u16 allmulti:1;
> +		} bits;
> +	} status; 
>  
>  	/* The skb we couldn't send because buffers were full. */
>  	struct sk_buff *last_xmit_skb;
> @@ -476,6 +483,54 @@ static int virtnet_set_tx_csum(struct net_device *dev, u32 data)
>  	return ethtool_op_set_tx_hw_csum(dev, data);
>  }
>  
> +static int virtnet_set_mac_address(struct net_device *dev, void *p)
> +{
> +	struct virtnet_info *vi = netdev_priv(dev);
> +	struct virtio_device *vdev = vi->vdev;
> +	struct sockaddr *addr = p;
> +
> +	if (!is_valid_ether_addr(addr->sa_data))
> +		return -EADDRNOTAVAIL;
> +
> +	memcpy(dev->dev_addr, addr->sa_data, dev->addr_len);

Should just use eth_mac_addr() here.

> +
> +	vdev->config->set(vdev, offsetof(struct virtio_net_config, mac),
> +			  dev->dev_addr, dev->addr_len);
> +
> +	return 0;
> +}
> +
> +static void virtnet_set_rx_mode(struct net_device *dev)
> +{

Not sure, but it might have been more appropriate to implement
change_rx_flags() for this ... but set_rx_mode() is probably correct
given the next patch.

> +	struct virtnet_info *vi = netdev_priv(dev);
> +	struct virtio_device *vdev = vi->vdev;
> +	u16 status = vi->status.raw;
> +
> +	if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_STATUS))
> +		return;
> +
> +	if (dev->flags & IFF_PROMISC)
> +		status |= VIRTIO_NET_S_PROMISC;
> +	else
> +		status &= ~VIRTIO_NET_S_PROMISC;
> +
> +	if (dev->flags & IFF_ALLMULTI)
> +		status |= VIRTIO_NET_S_ALLMULTI;
> +	else
> +		status &= ~VIRTIO_NET_S_ALLMULTI;
> +		
> +	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));
> +	}
> +}
> +
>  static struct ethtool_ops virtnet_ethtool_ops = {
>  	.set_tx_csum = virtnet_set_tx_csum,
>  	.set_sg = ethtool_op_set_sg,
> @@ -494,14 +549,15 @@ static void virtnet_update_status(struct virtnet_info *vi)
>                                &v, sizeof(v));
>  
>  	/* Ignore unknown (future) status bits */
> -	v &= VIRTIO_NET_S_LINK_UP;
> +	v &= VIRTIO_NET_S_LINK_UP | VIRTIO_NET_S_PROMISC |
> +             VIRTIO_NET_S_ALLMULTI;
>  
> -	if (vi->status == v)
> +	if (vi->status.raw == v)
>  		return;
>  
> -	vi->status = v;
> +	vi->status.raw = v;
>  
> -	if (vi->status & VIRTIO_NET_S_LINK_UP) {
> +	if (vi->status.bits.link) {
>  		netif_carrier_on(vi->dev);
>  		netif_wake_queue(vi->dev);
>  	} else {
> @@ -563,8 +619,17 @@ static int virtnet_probe(struct virtio_device *vdev)
>  		vdev->config->get(vdev,
>  				  offsetof(struct virtio_net_config, mac),
>  				  dev->dev_addr, dev->addr_len);
> -	} else
> +	} else {
> +		struct sockaddr addr;
> +
>  		random_ether_addr(dev->dev_addr);
> +		memset(&addr, 0, sizeof(addr));
> +		memcpy(&addr.sa_data, dev->dev_addr, dev->addr_len);
> +		virtnet_set_mac_address(dev, &addr);

I'd prefer just to vdev->config->set() directly here. set_mac_address()
isn't really appropriate.

> +	}
> +
> +	dev->set_mac_address = virtnet_set_mac_address;
> +	dev->set_rx_mode = virtnet_set_rx_mode;
>  
>  	/* Set up our device-specific information */
>  	vi = netdev_priv(dev);
> @@ -621,7 +686,7 @@ static int virtnet_probe(struct virtio_device *vdev)
>  		goto unregister;
>  	}
>  
> -	vi->status = VIRTIO_NET_S_LINK_UP;
> +	vi->status.raw = VIRTIO_NET_S_LINK_UP;
>  	virtnet_update_status(vi);
>  
>  	pr_debug("virtnet: registered device %s\n", dev->name);
> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index d9174be..5a70edb 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -23,6 +23,8 @@
>  #define VIRTIO_NET_F_STATUS	16	/* virtio_net_config.status available */
>  
>  #define VIRTIO_NET_S_LINK_UP	1	/* Link is up */
> +#define VIRTIO_NET_S_PROMISC	2	/* Promiscuous mode */

The current behaviour is PROMISC, right?

So, by not setting this flag you get PROMISC anyway?

Does that suggest we need a feature flag for this, or make zero reflect
the current host behaviour ... i.e. change the bit to S_CHASTE ?

(Chaste is the antonym to promiscuous, apparently :-)

> +#define VIRTIO_NET_S_ALLMULTI	4	/* All-multicast mode */

An issue here is that the LINK_UP bit is controlled by the host, but the
PROMISC and ALLMULTI are controlled by the guest - i.e. they can race.

Suggest giving the top 8 bits to the guest and keeping the bottom 8 bits
for the host.

>  struct virtio_net_config
>  {
> @@ -30,7 +32,14 @@ struct virtio_net_config
>  	__u8 mac[6];
>  	/* Status supplied by host; see VIRTIO_NET_F_STATUS and VIRTIO_NET_S_*
>  	 * bits above */
> -	__u16 status;
> +	union {
> +		__u16 raw;
> +		struct {
> +			__u16 link:1;
> +			__u16 promisc:1;
> +			__u16 allmulti:1;
> +		} bits;
> +       } status;

Agree with Anthony on this, much rather we continued using the #defines.

Cheers,
Mark.

Re: [PATCH 1/2][RFC] virtio_net: Enable setting MAC, promisc, and allmulti mode

[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:
> > --- a/include/linux/virtio_net.h
> > +++ b/include/linux/virtio_net.h
> > @@ -23,6 +23,8 @@
> >  #define VIRTIO_NET_F_STATUS	16	/* virtio_net_config.status available */
> >  
> >  #define VIRTIO_NET_S_LINK_UP	1	/* Link is up */
> > +#define VIRTIO_NET_S_PROMISC	2	/* Promiscuous mode */

Thanks for the rest of your comments and tips, I'll incorporate them.

> The current behaviour is PROMISC, right?
> 
> So, by not setting this flag you get PROMISC anyway?
> 
> Does that suggest we need a feature flag for this, or make zero reflect
> the current host behaviour ... i.e. change the bit to S_CHASTE ?
> 
> (Chaste is the antonym to promiscuous, apparently :-)

We could just have the "power on" state of virtio-net set the PROMISC
status bit, so it's up to the driver to clear it.  That's what I did for
the case of doing a load-vm from an older save state.  I think that will
give us the right behavior, but it's not clear to me if we need another
feature bit.

> > +#define VIRTIO_NET_S_ALLMULTI	4	/* All-multicast mode */
> 
> An issue here is that the LINK_UP bit is controlled by the host, but the
> PROMISC and ALLMULTI are controlled by the guest - i.e. they can race.
> 
> Suggest giving the top 8 bits to the guest and keeping the bottom 8 bits
> for the host.

Good thought, I was simply ignoring writes to the link bit.  AFAIK, it's
not uncommon that a status register on real hardware might have both RO
and RW bits next to each other.  I'm also toying with the idea that if
the mac table and vlan table gets moved to a virt-queue, maybe setting
and clearing these bits should be done via the vq too.  Thoughts?

> >  struct virtio_net_config
> >  {
> > @@ -30,7 +32,14 @@ struct virtio_net_config
> >  	__u8 mac[6];
> >  	/* Status supplied by host; see VIRTIO_NET_F_STATUS and VIRTIO_NET_S_*
> >  	 * bits above */
> > -	__u16 status;
> > +	union {
> > +		__u16 raw;
> > +		struct {
> > +			__u16 link:1;
> > +			__u16 promisc:1;
> > +			__u16 allmulti:1;
> > +		} bits;
> > +       } status;
> 
> Agree with Anthony on this, much rather we continued using the #defines.

Yeah, I do too now.  I've got a local copy working this way and it's no
messier.  Thanks,

Alex

-- 
Alex Williamson                             HP Open Source & Linux Org.

Re: [PATCH 1/2][RFC] virtio_net: Enable setting MAC, promisc, and allmulti mode

[Rusty Russell]

On Thursday 08 January 2009 04:36:02 Alex Williamson wrote:
> virtio_net: Enable setting MAC, promisc, and allmulti mode

Hi Alex,

   There's nothing wrong with this idea: I assume you have an actual usage
for this rather than it being an abstract improvement?

> @@ -41,7 +41,14 @@ struct virtnet_info
>  	struct virtqueue *rvq, *svq;
>  	struct net_device *dev;
>  	struct napi_struct napi;
> -	unsigned int status;
> +	union {
> +		u16 raw;
> +		struct {
> +			u16 link:1;
> +			u16 promisc:1;
> +			u16 allmulti:1;
> +		} bits;
> +	} status;

I don't think this works, as it depends on bitfield endian.

> @@ -30,7 +32,14 @@ struct virtio_net_config
>  	__u8 mac[6];
>  	/* Status supplied by host; see VIRTIO_NET_F_STATUS and VIRTIO_NET_S_*
>  	 * bits above */
> -	__u16 status;
> +	union {
> +		__u16 raw;
> +		struct {
> +			__u16 link:1;
> +			__u16 promisc:1;
> +			__u16 allmulti:1;
> +		} bits;
> +       } status;
>  } __attribute__((packed));

As does this.  I think we need to leave the status bitfield as is.

Thanks,
Rusty.