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.william...@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.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
