[Qemu-devel] [PATCH][RFC] qemu:virtio-net: Use TUNSETTXFILTER for MAC filtering

[Qemu-devel] [PATCH][RFC] qemu:virtio-net: Use TUNSETTXFILTER for MAC filtering

[Alex Williamson]

Now that virtio-net knows what packets the guest wants to see, we
can start moving the filtering down the stack.  This patch adds
an interface to set the software filter in the tap device.  It's
fairly limited, but we can back it up with our own filtering if it
overflows.

Here are a couple issues I'm still pondering:
 - Is the fd_rx_filter() interface sufficiently generic
 - Should vlan_set_hw_rx_filter() live in net.c or elsewhere
 - Is it ok to call fd_rx_filter() against all the vlan clients.  I
   exit on the first one, which covers the simple config.

Insterested in feedback.  Thanks,

Alex

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

 hw/virtio-net.c |   57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 net.c           |   28 +++++++++++++++++++++++++++
 net.h           |    3 +++
 3 files changed, 88 insertions(+), 0 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 62153e9..2556f42 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -15,6 +15,7 @@
 #include "net.h"
 #include "qemu-timer.h"
 #include "virtio-net.h"
+#include <net/if.h>
 
 #define VIRTIO_NET_VM_VERSION    6
 
@@ -35,6 +36,7 @@ typedef struct VirtIONet
     int mergeable_rx_bufs;
     int promisc;
     int allmulti;
+    int hw_mac_filter;
     struct {
         int in_use;
         uint8_t *macs;
@@ -88,6 +90,51 @@ static void virtio_net_set_link_status(VLANClientState *vc)
         virtio_notify_config(&n->vdev);
 }
 
+static int vlan_set_hw_rx_filter(VLANState *vlan, int flags,
+                                 int count, uint8_t *buf)
+{
+    VLANClientState *vc;
+
+    for (vc = vlan->first_client; vc != NULL; vc = vc->next) {
+        int ret;
+
+        if (!vc->fd_rx_filter)
+            continue;
+
+        ret = vc->fd_rx_filter(vc->opaque, flags, count, buf);
+        return (ret == count);
+    } 
+    return 0;
+}
+
+static void virtio_net_set_hw_rx_filter(VirtIONet *n)
+{
+    static const uint8_t bcast[] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
+    uint8_t *buf;
+    int flags = 0;
+
+    if (n->promisc)
+        flags |= IFF_PROMISC;
+    if (n->allmulti)
+        flags |= IFF_ALLMULTI;
+
+    buf = qemu_mallocz((n->mac_table.in_use + 2) * ETH_ALEN);
+    if (!buf) {
+        fprintf(stderr, "virtio-net no memory for set_rx_filter\n");
+        vlan_set_hw_rx_filter(n->vc->vlan, IFF_PROMISC, 0, NULL);
+        n->hw_mac_filter = 0;
+        return;
+    }
+
+    memcpy(&buf[ETH_ALEN*0], n->mac, ETH_ALEN);
+    memcpy(&buf[ETH_ALEN*1], bcast, ETH_ALEN);
+    memcpy(&buf[ETH_ALEN*2], n->mac_table.macs, n->mac_table.in_use * ETH_ALEN);
+
+    n->hw_mac_filter = vlan_set_hw_rx_filter(n->vc->vlan, flags,
+                                             n->mac_table.in_use + 2, buf);
+    qemu_free(buf);
+}
+
 static void virtio_net_reset(VirtIODevice *vdev)
 {
     VirtIONet *n = to_virtio_net(vdev);
@@ -99,6 +146,7 @@ static void virtio_net_reset(VirtIODevice *vdev)
     /* Flush any MAC and VLAN filter table state */
     n->mac_table.in_use = 0;
     memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
+    virtio_net_set_hw_rx_filter(n);
     memset(n->vlans, 0, MAX_VLAN >> 3);
 }
 
@@ -247,6 +295,10 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
 
         virtqueue_push(vq, &elem, sizeof(status));
         virtio_notify(vdev, vq);
+
+        if (ctrl.class == VIRTIO_NET_CTRL_RX_MODE ||
+            ctrl.class == VIRTIO_NET_CTRL_MAC)
+            virtio_net_set_hw_rx_filter(n);
     }
 }
 
@@ -334,6 +386,9 @@ static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)
             return 0;
     }
 
+    if (n->hw_mac_filter)
+        return 1;
+
     if ((ptr[0] & 1) && n->allmulti)
         return 1;
 
@@ -552,6 +607,8 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
     if (version_id >= 6)
         qemu_get_buffer(f, (uint8_t *)n->vlans, MAX_VLAN >> 3);
 
+    virtio_net_set_hw_rx_filter(n);
+
     if (n->tx_timer_active) {
         qemu_mod_timer(n->tx_timer,
                        qemu_get_clock(vm_clock) + TX_TIMER_INTERVAL);
diff --git a/net.c b/net.c
index 8d9b3de..fcf5272 100644
--- a/net.c
+++ b/net.c
@@ -685,6 +685,33 @@ static ssize_t tap_receive_iov(void *opaque, const struct iovec *iov,
 }
 #endif
 
+static int tap_rx_filter(void *opaque, unsigned int flags, int count,
+                         uint8_t *list)
+{
+    TAPState *s = opaque;
+    struct tun_filter *filter;
+    int ret;
+
+    if (flags & IFF_PROMISC)
+        count = 0;
+
+    filter = qemu_mallocz(sizeof(*filter) + (count * ETH_ALEN));
+    if (!filter)
+	return -1;
+
+    memcpy(filter->addr, list, count * ETH_ALEN);
+    filter->count += count;
+
+    if (flags & IFF_ALLMULTI)
+        filter->flags |= TUN_FLT_ALLMULTI;
+  
+    ret = ioctl(s->fd, TUNSETTXFILTER, filter);
+
+    qemu_free(filter);
+
+    return ret;
+}
+
 static void tap_receive(void *opaque, const uint8_t *buf, int size)
 {
     TAPState *s = opaque;
@@ -735,6 +762,7 @@ static TAPState *net_tap_fd_init(VLANState *vlan,
 #ifdef HAVE_IOVEC
     s->vc->fd_readv = tap_receive_iov;
 #endif
+    s->vc->fd_rx_filter = tap_rx_filter;
     qemu_set_fd_handler(s->fd, tap_send, NULL, s);
     snprintf(s->vc->info_str, sizeof(s->vc->info_str), "fd=%d", fd);
     return s;
diff --git a/net.h b/net.h
index 291807a..3e915dc 100644
--- a/net.h
+++ b/net.h
@@ -11,12 +11,15 @@ typedef struct VLANClientState VLANClientState;
 
 typedef void (LinkStatusChanged)(VLANClientState *);
 
+typedef int (IORXFilter)(void *, unsigned int , int , uint8_t *);
+
 struct VLANClientState {
     IOReadHandler *fd_read;
     IOReadvHandler *fd_readv;
     /* Packets may still be sent if this returns zero.  It's used to
        rate-limit the slirp code.  */
     IOCanRWHandler *fd_can_read;
+    IORXFilter *fd_rx_filter;
     LinkStatusChanged *link_status_changed;
     int link_down;
     void *opaque;

[Qemu-devel] Re: [PATCH][RFC] qemu:virtio-net: Use TUNSETTXFILTER for MAC filtering

[Mark McLoughlin]

On Thu, 2009-02-05 at 21:51 -0700, Alex Williamson wrote:
> Now that virtio-net knows what packets the guest wants to see, we
> can start moving the filtering down the stack.  This patch adds
> an interface to set the software filter in the tap device.  It's
> fairly limited, but we can back it up with our own filtering if it
> overflows.

Looks good to me.

Note also the new world order where we don't handle qemu_malloc()
failures.

> Here are a couple issues I'm still pondering:
>  - Is the fd_rx_filter() interface sufficiently generic
>  - Should vlan_set_hw_rx_filter() live in net.c or elsewhere
>  - Is it ok to call fd_rx_filter() against all the vlan clients.  I
>    exit on the first one, which covers the simple config.

All of these are related to the problem that we're trying to add
optimizations for the simple config (i.e. one NIC associated with one
tap interface) without any clean representation of the simple config in
the API.

It's not strictly needed in this case - the VLAN API for setting filters
could aggregate the filters of all VLAN clients - but what you really
want is to only have the filter apply if virtio-net is paired with a tap
interface.

Cheers,
Mark.

[Qemu-devel] Re: [PATCH][RFC] qemu:virtio-net: Use TUNSETTXFILTER for MAC filtering

[Anthony Liguori]

Alex Williamson wrote:
> Now that virtio-net knows what packets the guest wants to see, we
> can start moving the filtering down the stack.  This patch adds
> an interface to set the software filter in the tap device.  It's
> fairly limited, but we can back it up with our own filtering if it
> overflows.
>
> Here are a couple issues I'm still pondering:
>  - Is the fd_rx_filter() interface sufficiently generic
>  - Should vlan_set_hw_rx_filter() live in net.c or elsewhere
>  - Is it ok to call fd_rx_filter() against all the vlan clients.  I
>    exit on the first one, which covers the simple config.
>
> Insterested in feedback.  Thanks,
>
> Alex
>
> Signed-off-by: Alex Williamson <alex.williamson@hp.com>
> ---
>
>  hw/virtio-net.c |   57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  net.c           |   28 +++++++++++++++++++++++++++
>  net.h           |    3 +++
>  3 files changed, 88 insertions(+), 0 deletions(-)
>
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index 62153e9..2556f42 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -15,6 +15,7 @@
>  #include "net.h"
>  #include "qemu-timer.h"
>  #include "virtio-net.h"
> +#include <net/if.h>
>  
>  #define VIRTIO_NET_VM_VERSION    6
>  
> @@ -35,6 +36,7 @@ typedef struct VirtIONet
>      int mergeable_rx_bufs;
>      int promisc;
>      int allmulti;
> +    int hw_mac_filter;
>      struct {
>          int in_use;
>          uint8_t *macs;
> @@ -88,6 +90,51 @@ static void virtio_net_set_link_status(VLANClientState *vc)
>          virtio_notify_config(&n->vdev);
>  }
>  
> +static int vlan_set_hw_rx_filter(VLANState *vlan, int flags,
> +                                 int count, uint8_t *buf)
> +{
> +    VLANClientState *vc;
> +
> +    for (vc = vlan->first_client; vc != NULL; vc = vc->next) {
> +        int ret;
> +
> +        if (!vc->fd_rx_filter)
> +            continue;
> +
> +        ret = vc->fd_rx_filter(vc->opaque, flags, count, buf);
> +        return (ret == count);
> +    } 
> +    return 0;
> +}
>   

This should go in net.c.

> +static void virtio_net_set_hw_rx_filter(VirtIONet *n)
> +{
> +    static const uint8_t bcast[] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
> +    uint8_t *buf;
> +    int flags = 0;
> +
> +    if (n->promisc)
> +        flags |= IFF_PROMISC;
> +    if (n->allmulti)
> +        flags |= IFF_ALLMULTI;
> +
> +    buf = qemu_mallocz((n->mac_table.in_use + 2) * ETH_ALEN);
> +    if (!buf) {
>   
Don't need to handle these failures anymore.
> +static int tap_rx_filter(void *opaque, unsigned int flags, int count,
> +                         uint8_t *list)
>   

Instead of having each network device do it's own filtering if the VLAN 
doesn't support it, I think we should move the software filtering to the 
VLAN layer so that we aren't duplicating code in each network device.

Also, instead of using IFF_xxx, I think we should introduce our own 
flags.  net/if.h doesn't exist on Windows most likely.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH][RFC] qemu:virtio-net: Use TUNSETTXFILTER for MAC filtering

[Paul Brook]

On Friday 06 February 2009, Alex Williamson wrote:
> Now that virtio-net knows what packets the guest wants to see, we
> can start moving the filtering down the stack.  This patch adds
> an interface to set the software filter in the tap device.  It's
> fairly limited, but we can back it up with our own filtering if it
> overflows.

I think you've got the abstraction wrong here. Devices certainly shouldn't 
care what else is connected to the vlan.

Remember that a qemu vlan is an arbitrary set of network devices. You need to 
be able to support multiple devices all with their own filters.

There should be two interface points between the the vlan and the device:
1) The device can indicate that it only cares about incoming (vlan to device) 
packets sent to a subset of MAC addresses.
2) The vlan can request that a device discards outgoing (device to vlan) 
packets that don't match a subset of MAC addresses.

Typically a emulated NIC will implement (1) and a host interface will 
implement (2).

The generic vlan code should be responsible for using the information provided 
by (1) to set (2) appropriately. Remember that network devices can be 
hotplugged.


On a related note, you need to add comments making it clear that this is a 
performance optimisation, and must not be relied on. A device may still 
receive packets that should have been filtered out. If a device needs 
reliable filtering (most "real" hardware does), then this still needs to be 
implemented per-device.

Paul

Re: [Qemu-devel] [PATCH][RFC] qemu:virtio-net: Use TUNSETTXFILTER for MAC filtering

[Alex Williamson]

Hi Paul,

On Fri, 2009-02-06 at 15:12 +0000, Paul Brook wrote:
> On Friday 06 February 2009, Alex Williamson wrote:
> > Now that virtio-net knows what packets the guest wants to see, we
> > can start moving the filtering down the stack.  This patch adds
> > an interface to set the software filter in the tap device.  It's
> > fairly limited, but we can back it up with our own filtering if it
> > overflows.
> 
> I think you've got the abstraction wrong here. Devices certainly shouldn't 
> care what else is connected to the vlan.
> 
> Remember that a qemu vlan is an arbitrary set of network devices. You need to 
> be able to support multiple devices all with their own filters.

In the model I'm considering, and I need to change the code to enforce
this, there are two devices on the vlan.  One side is the backend of a
NIC that's exported to the guest.  The other side is a tap or socket or
whatever transports packets into and out of our userspace instance.  If
there are more devices, the vlan needs to be treated as a shared media
with filtering left to the emulated NIC.

> There should be two interface points between the the vlan and the device:
> 1) The device can indicate that it only cares about incoming (vlan to device) 
> packets sent to a subset of MAC addresses.
> 2) The vlan can request that a device discards outgoing (device to vlan) 
> packets that don't match a subset of MAC addresses.
> 
> Typically a emulated NIC will implement (1) and a host interface will 
> implement (2).
> 
> The generic vlan code should be responsible for using the information provided 
> by (1) to set (2) appropriately. Remember that network devices can be 
> hotplugged.

Do you think it's worthwhile for the vlan to save and consolidate
filtering from multiple (1) sources to program (2)?  My interface is
effectively short circuiting and letting the (1) agent set (2).  As you
mention below, this is an optimization, so I think it's reasonable that
if we exceed a 1x1 configuration of (1)s and (2)s, we need to reset back
to a shared media model and do filtering in the (1) agent, which is how
I think we should handle hotplug.

> On a related note, you need to add comments making it clear that this is a 
> performance optimisation, and must not be relied on. A device may still 
> receive packets that should have been filtered out. If a device needs 
> reliable filtering (most "real" hardware does), then this still needs to be 
> implemented per-device.

Yes, I'll add something.  Thanks for the comments,

Alex

[Qemu-devel] Re: [PATCH][RFC] qemu:virtio-net: Use TUNSETTXFILTER for MAC filtering

[Alex Williamson]

On Fri, 2009-02-06 at 07:59 -0600, Anthony Liguori wrote:
> 
> Instead of having each network device do it's own filtering if the VLAN 
> doesn't support it, I think we should move the software filtering to the 
> VLAN layer so that we aren't duplicating code in each network device.
> 
> Also, instead of using IFF_xxx, I think we should introduce our own 
> flags.  net/if.h doesn't exist on Windows most likely.

Thanks for the comments, I'll look into making these changes.

Alex

[Qemu-devel] Re: [PATCH][RFC] qemu:virtio-net: Use TUNSETTXFILTER for MAC filtering

[Alex Williamson]

On Fri, 2009-02-06 at 07:47 +0000, Mark McLoughlin wrote:
> On Thu, 2009-02-05 at 21:51 -0700, Alex Williamson wrote:
> > Here are a couple issues I'm still pondering:
> >  - Is the fd_rx_filter() interface sufficiently generic
> >  - Should vlan_set_hw_rx_filter() live in net.c or elsewhere
> >  - Is it ok to call fd_rx_filter() against all the vlan clients.  I
> >    exit on the first one, which covers the simple config.
> 
> All of these are related to the problem that we're trying to add
> optimizations for the simple config (i.e. one NIC associated with one
> tap interface) without any clean representation of the simple config in
> the API.
> 
> It's not strictly needed in this case - the VLAN API for setting filters
> could aggregate the filters of all VLAN clients - but what you really
> want is to only have the filter apply if virtio-net is paired with a tap
> interface.

Yes, or more precisely, it should only be applied if there are exactly
two devices on the vlan, and the "other" device implements fd_rx_filter.
I'll enforce that in the code.  Thanks,

Alex

Re: [Qemu-devel] [PATCH][RFC] qemu:virtio-net: Use TUNSETTXFILTER for MAC filtering

[Paul Brook]

> > There should be two interface points between the the vlan and the device:
> > 1) The device can indicate that it only cares about incoming (vlan to
> > device) packets sent to a subset of MAC addresses.
> > 2) The vlan can request that a device discards outgoing (device to vlan)
> > packets that don't match a subset of MAC addresses.
> >
> > Typically a emulated NIC will implement (1) and a host interface will
> > implement (2).
> >
> > The generic vlan code should be responsible for using the information
> > provided by (1) to set (2) appropriately. Remember that network devices
> > can be hotplugged.
>
> Do you think it's worthwhile for the vlan to save and consolidate
> filtering from multiple (1) sources to program (2)?  My interface is
> effectively short circuiting and letting the (1) agent set (2).  As you
> mention below, this is an optimization, so I think it's reasonable that
> if we exceed a 1x1 configuration of (1)s and (2)s, we need to reset back
> to a shared media model and do filtering in the (1) agent, which is how
> I think we should handle hotplug.

My point is that the devices themselves shouldn't know or care about this. 
Obviously a trivial implementation of combining multiple sources is to 
disable the filter if there is more than one other device.

Paul