Hi Alex,
On Fri, 2009-01-16 at 14:13 -0700, Alex Williamson wrote:
> VLAN filtering allows the hypervisor to drop packets from VLANs
> that we're not a part of, further reducing the number of extraneous
> packets recieved. This makes use of the VLAN virtqueue command class.
> The ENABLE command is used both to activate the filter and verify the
> existence of the functionality on the backend.
>
> Also, this fixes a sizing issue for MAX_PACKET_LEN when using VLANs.
> VLANS add 4 bytes of header, resulting in a maximum full packet size
> of 1518 bytes (destination MAC + source MAC + VLAN + ethernet type +
> MTU). Withouth this, a VLAN over virtio_net is likely to get a
> truncation error in the backend.
Looks good, but could you split the MAX_PACKET_LEN change out to a
separate patch, send to davem for 2.6.29. We should also get this into
stable.
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 9be0d6a..8d4bc83 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -24,6 +24,7 @@
> #include <linux/virtio.h>
> #include <linux/virtio_net.h>
> #include <linux/scatterlist.h>
> +#include <linux/if_vlan.h>
>
> static int napi_weight = 128;
> module_param(napi_weight, int, 0444);
> @@ -38,7 +39,7 @@ 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 MAX_PACKET_LEN (ETH_HLEN + VLAN_HLEN + ETH_DATA_LEN)
> #define GOOD_COPY_LEN 128
>
> struct virtnet_info
> @@ -743,6 +744,28 @@ set_mode:
> dev->name, allmulti ? "en" : "dis");
> }
>
> +static void virnet_vlan_rx_add_vid(struct net_device *dev, u16 vid)
> +{
> + struct virtnet_info *vi = netdev_priv(dev);
> + u16 id = vid;
Indentation mixup.
> +
> + if (virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
> + VIRTIO_NET_CTRL_VLAN_ADD, &id, sizeof(id)))
> + printk(KERN_WARNING "%s: Failed to add VLAN ID %d.\n",
> + dev->name, id);
> +}
> +
> +static void virnet_vlan_rx_kill_vid(struct net_device *dev, u16 vid)
> +{
> + struct virtnet_info *vi = netdev_priv(dev);
> + u16 id = vid;
Ditto.
> +
> + if (virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
> + VIRTIO_NET_CTRL_VLAN_KILL, &id, sizeof(id)))
> + printk(KERN_WARNING "%s: Failed to kill VLAN ID %d.\n",
> + dev->name, id);
> +}
> +
> static struct ethtool_ops virtnet_ethtool_ops = {
> .set_tx_csum = virtnet_set_tx_csum,
> .set_sg = ethtool_op_set_sg,
> @@ -761,15 +784,17 @@ static int virtnet_change_mtu(struct net_device *dev,
> int new_mtu)
> }
>
> static const struct net_device_ops virtnet_netdev = {
> - .ndo_open = virtnet_open,
> - .ndo_stop = virtnet_close,
> - .ndo_start_xmit = start_xmit,
> - .ndo_validate_addr = eth_validate_addr,
> - .ndo_set_mac_address = virtnet_set_mac_address,
> - .ndo_set_rx_mode = virtnet_set_rx_mode,
> - .ndo_change_mtu = virtnet_change_mtu,
> + .ndo_open = virtnet_open,
> + .ndo_stop = virtnet_close,
> + .ndo_start_xmit = start_xmit,
> + .ndo_validate_addr = eth_validate_addr,
> + .ndo_set_mac_address = virtnet_set_mac_address,
> + .ndo_set_rx_mode = virtnet_set_rx_mode,
> + .ndo_change_mtu = virtnet_change_mtu,
> + .ndo_vlan_rx_add_vid = virnet_vlan_rx_add_vid,
> + .ndo_vlan_rx_kill_vid = virnet_vlan_rx_kill_vid,
> #ifdef CONFIG_NET_POLL_CONTROLLER
> - .ndo_poll_controller = virtnet_netpoll,
> + .ndo_poll_controller = virtnet_netpoll,
> #endif
> };
>
> @@ -863,6 +888,7 @@ static int virtnet_probe(struct virtio_device *vdev)
> vi->cvq = NULL;
> else {
> unsigned int entries;
> + u8 vlan_filter = 1;
>
> /*
> * We use a separate stack variable here because the
> @@ -878,6 +904,14 @@ static int virtnet_probe(struct virtio_device *vdev)
> "MAC filter table allocation failed.\n");
> mac_entries = 0;
> }
> +
> + /* Enable VLAN filtering if supported by the backend */
> + if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
> + VIRTIO_NET_CTRL_VLAN_ENABLE,
> + &vlan_filter, sizeof(vlan_filter))) {
> + printk(KERN_INFO "virtio_net: VLAN filter enabled\n");
Do we need this KERN_INFO? KERN_DEBUG, maybe?
> + dev->features |= NETIF_F_HW_VLAN_FILTER;
> + }
> }
>
> /* Initialize our empty receive and send queues. */
> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index 84086a6..a95028d 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -99,4 +99,19 @@ typedef __u8 virtio_net_ctrl_ack;
> #define VIRTIO_NET_CTRL_MAC_TABLE_ALLOC 0
> #define VIRTIO_NET_CTRL_MAC_TABLE_SET 1
>
> +/*
> + * Control VLAN filtering
> + *
> + * The VLAN filter table is controlled via a simple ADD/KILL interface.
> + * VLAN IDs not added will be dropped. Kill is the opposite of add.
> + * Both commands expect an out entry containing a 2 byte VLAN ID.
> + * The ENABLE command expects an out entry containing a single byte,
> + * zero to disable, non-zero to enable. The default state is disabled
> + * for compatibility.
> + */
> +#define VIRTIO_NET_CTRL_VLAN 2
> + #define VIRTIO_NET_CTRL_VLAN_ENABLE 0
> + #define VIRTIO_NET_CTRL_VLAN_ADD 1
> + #define VIRTIO_NET_CTRL_VLAN_KILL 2
Not too keen on "kill" - it matches the linux API, but "remove" or
"delete" is probably more sensible.
Cheers,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
