[Qemu-devel] [PATCH 0/7] virtio-net: Filter cleanup/improvements

[Qemu-devel] [PATCH 0/7] virtio-net: Filter cleanup/improvements

[Alex Williamson]

This series cleans up a few things around packet filtering.  I've probably
gone a little overboard on breaking up patches, if we want to avoid bumping
the save version_id so much, these could be mostly lumped together.  The
main features here are more efficient handling of the filtering between
unicast and multicast, better overflow tracking, adding more RX modes,
and increasing the size of the filter table and control queue depth.

I took the reserved version_id 7 for vnet header support into consideration
on the first patch.  I think we should be able to safely add the guts later
with the placeholder.  If there are other RX mode controls we should add,
let me know, now would be a good time to round out any other flags we can
think of.  Thanks,

Alex

---

Alex Williamson (7):
      virtio-net: Increase filter and control limits
      virtio-net: Add new RX filter controls
      virtio-net: MAC filter optimization
      virtio-net: Fix MAC filter overflow handling
      virtio-net: reorganize receive_filter()
      virtio-net: Use a byte to store RX mode flags
      virtio-net: Add version_id 7 placeholder for vnet header support


 hw/virtio-net.c |  133 +++++++++++++++++++++++++++++++++++++++++++++----------
 hw/virtio-net.h |   14 ++++--
 2 files changed, 118 insertions(+), 29 deletions(-)

-- 

[Qemu-devel] [PATCH 1/7] virtio-net: Add version_id 7 placeholder for vnet header support

[Alex Williamson]

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

 hw/virtio-net.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 60aa6da..9471d9e 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -16,7 +16,7 @@
 #include "qemu-timer.h"
 #include "virtio-net.h"
 
-#define VIRTIO_NET_VM_VERSION    6
+#define VIRTIO_NET_VM_VERSION    7
 
 #define MAC_TABLE_ENTRIES    32
 #define MAX_VLAN    (1 << 12)   /* Per 802.1Q definition */
@@ -523,6 +523,7 @@ static void virtio_net_save(QEMUFile *f, void *opaque)
     qemu_put_be32(f, n->mac_table.in_use);
     qemu_put_buffer(f, n->mac_table.macs, n->mac_table.in_use * ETH_ALEN);
     qemu_put_buffer(f, (uint8_t *)n->vlans, MAX_VLAN >> 3);
+    qemu_put_be32(f, 0); /* vnet-hdr placeholder */
 }
 
 static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
@@ -562,6 +563,12 @@ 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);
 
+    if (version_id >= 7 && qemu_get_be32(f)) {
+        fprintf(stderr,
+                "virtio-net: saved image requires vnet header support\n");
+        exit(1);
+    }
+
     if (n->tx_timer_active) {
         qemu_mod_timer(n->tx_timer,
                        qemu_get_clock(vm_clock) + TX_TIMER_INTERVAL);

[Qemu-devel] [PATCH 2/7] virtio-net: Use a byte to store RX mode flags

[Alex Williamson]

There's no need to save 4 bytes for promisc and allmulti.
Use one byte each just to avoid the overhead of a bitmap.

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

 hw/virtio-net.c |   19 ++++++++++++-------
 1 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 9471d9e..de5a59f 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -16,7 +16,7 @@
 #include "qemu-timer.h"
 #include "virtio-net.h"
 
-#define VIRTIO_NET_VM_VERSION    7
+#define VIRTIO_NET_VM_VERSION    8
 
 #define MAC_TABLE_ENTRIES    32
 #define MAX_VLAN    (1 << 12)   /* Per 802.1Q definition */
@@ -33,8 +33,8 @@ typedef struct VirtIONet
     QEMUTimer *tx_timer;
     int tx_timer_active;
     int mergeable_rx_bufs;
-    int promisc;
-    int allmulti;
+    uint8_t promisc;
+    uint8_t allmulti;
     struct {
         int in_use;
         uint8_t *macs;
@@ -518,8 +518,8 @@ static void virtio_net_save(QEMUFile *f, void *opaque)
     qemu_put_be32(f, n->tx_timer_active);
     qemu_put_be32(f, n->mergeable_rx_bufs);
     qemu_put_be16(f, n->status);
-    qemu_put_be32(f, n->promisc);
-    qemu_put_be32(f, n->allmulti);
+    qemu_put_byte(f, n->promisc);
+    qemu_put_byte(f, n->allmulti);
     qemu_put_be32(f, n->mac_table.in_use);
     qemu_put_buffer(f, n->mac_table.macs, n->mac_table.in_use * ETH_ALEN);
     qemu_put_buffer(f, (uint8_t *)n->vlans, MAX_VLAN >> 3);
@@ -543,8 +543,13 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
         n->status = qemu_get_be16(f);
 
     if (version_id >= 4) {
-        n->promisc = qemu_get_be32(f);
-        n->allmulti = qemu_get_be32(f);
+        if (version_id < 8) {
+            n->promisc = qemu_get_be32(f);
+            n->allmulti = qemu_get_be32(f);
+        } else {
+            n->promisc = qemu_get_byte(f);
+            n->allmulti = qemu_get_byte(f);
+        }
     }
 
     if (version_id >= 5) {

[Qemu-devel] [PATCH 3/7] virtio-net: reorganize receive_filter()

[Alex Williamson]

Reorganize receive_filter to better handle the split between
unicast and multicast filtering.  This allows us to skip the
broadcast check on unicast packets and leads to more opportunities
for optimization.

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

 hw/virtio-net.c |   19 +++++++++++--------
 1 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index de5a59f..395b735 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -344,14 +344,17 @@ static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)
             return 0;
     }
 
-    if ((ptr[0] & 1) && n->allmulti)
-        return 1;
-
-    if (!memcmp(ptr, bcast, sizeof(bcast)))
-        return 1;
-
-    if (!memcmp(ptr, n->mac, ETH_ALEN))
-        return 1;
+    if (ptr[0] & 1) { // multicast
+        if (!memcmp(ptr, bcast, sizeof(bcast))) {
+            return 1;
+        } else if (n->allmulti) {
+            return 1;
+        }
+    } else { // unicast
+        if (!memcmp(ptr, n->mac, ETH_ALEN)) {
+            return 1;
+        }
+    }
 
     for (i = 0; i < n->mac_table.in_use; i++) {
         if (!memcmp(ptr, &n->mac_table.macs[i * ETH_ALEN], ETH_ALEN))

[Qemu-devel] [PATCH 4/7] virtio-net: Fix MAC filter overflow handling

[Alex Williamson]

Overloading the promisc and allmulti flags for indicating filter
table overflow makes it difficult to track the actual requested
operating mode.  Split these out into separate flags.

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

 hw/virtio-net.c |   31 +++++++++++++++++++++++--------
 1 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 395b735..64515a3 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -16,7 +16,7 @@
 #include "qemu-timer.h"
 #include "virtio-net.h"
 
-#define VIRTIO_NET_VM_VERSION    8
+#define VIRTIO_NET_VM_VERSION    9
 
 #define MAC_TABLE_ENTRIES    32
 #define MAX_VLAN    (1 << 12)   /* Per 802.1Q definition */
@@ -37,6 +37,8 @@ typedef struct VirtIONet
     uint8_t allmulti;
     struct {
         int in_use;
+        uint8_t multi_overflow;
+        uint8_t uni_overflow;
         uint8_t *macs;
     } mac_table;
     uint32_t *vlans;
@@ -98,6 +100,8 @@ static void virtio_net_reset(VirtIODevice *vdev)
 
     /* Flush any MAC and VLAN filter table state */
     n->mac_table.in_use = 0;
+    n->mac_table.multi_overflow = 0;
+    n->mac_table.uni_overflow = 0;
     memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
     memset(n->vlans, 0, MAX_VLAN >> 3);
 }
@@ -168,6 +172,8 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
         return VIRTIO_NET_ERR;
 
     n->mac_table.in_use = 0;
+    n->mac_table.uni_overflow = 0;
+    n->mac_table.multi_overflow = 0;
     memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
 
     mac_data.entries = ldl_le_p(elem->out_sg[1].iov_base);
@@ -181,8 +187,7 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
                mac_data.entries * ETH_ALEN);
         n->mac_table.in_use += mac_data.entries;
     } else {
-        n->promisc = 1;
-        return VIRTIO_NET_OK;
+        n->mac_table.uni_overflow = 1;
     }
 
     mac_data.entries = ldl_le_p(elem->out_sg[2].iov_base);
@@ -197,8 +202,9 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
                    elem->out_sg[2].iov_base + sizeof(mac_data),
                    mac_data.entries * ETH_ALEN);
             n->mac_table.in_use += mac_data.entries;
-        } else
-            n->allmulti = 1;
+        } else {
+            n->mac_table.multi_overflow = 1;
+        }
     }
 
     return VIRTIO_NET_OK;
@@ -347,11 +353,13 @@ static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)
     if (ptr[0] & 1) { // multicast
         if (!memcmp(ptr, bcast, sizeof(bcast))) {
             return 1;
-        } else if (n->allmulti) {
+        } else if (n->allmulti || n->mac_table.multi_overflow) {
             return 1;
         }
     } else { // unicast
-        if (!memcmp(ptr, n->mac, ETH_ALEN)) {
+        if (n->mac_table.uni_overflow) {
+            return 1;
+        } else if (!memcmp(ptr, n->mac, ETH_ALEN)) {
             return 1;
         }
     }
@@ -527,6 +535,8 @@ static void virtio_net_save(QEMUFile *f, void *opaque)
     qemu_put_buffer(f, n->mac_table.macs, n->mac_table.in_use * ETH_ALEN);
     qemu_put_buffer(f, (uint8_t *)n->vlans, MAX_VLAN >> 3);
     qemu_put_be32(f, 0); /* vnet-hdr placeholder */
+    qemu_put_byte(f, n->mac_table.multi_overflow);
+    qemu_put_byte(f, n->mac_table.uni_overflow);
 }
 
 static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
@@ -563,7 +573,7 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
                             n->mac_table.in_use * ETH_ALEN);
         } else if (n->mac_table.in_use) {
             qemu_fseek(f, n->mac_table.in_use * ETH_ALEN, SEEK_CUR);
-            n->promisc = 1;
+            n->mac_table.multi_overflow = n->mac_table.uni_overflow = 1;
             n->mac_table.in_use = 0;
         }
     }
@@ -577,6 +587,11 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
         exit(1);
     }
 
+    if (version_id >= 9) {
+        n->mac_table.multi_overflow = qemu_get_byte(f);
+        n->mac_table.uni_overflow = qemu_get_byte(f);
+    }
+
     if (n->tx_timer_active) {
         qemu_mod_timer(n->tx_timer,
                        qemu_get_clock(vm_clock) + TX_TIMER_INTERVAL);

[Qemu-devel] [PATCH 5/7] virtio-net: MAC filter optimization

[Alex Williamson]

The MAC filter table is received from the guest as two separate
buffers, one with unicast entries, the other with multicast
entries.  If we track the index dividing the two sets, we can
avoid searching the part of the table with the wrong type of
entries.

We could store this index as part of the save image, but its
trivially easy to discover it on load.

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

 hw/virtio-net.c |   29 +++++++++++++++++++++++++----
 1 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 64515a3..5239cc0 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -37,6 +37,7 @@ typedef struct VirtIONet
     uint8_t allmulti;
     struct {
         int in_use;
+        int first_multi;
         uint8_t multi_overflow;
         uint8_t uni_overflow;
         uint8_t *macs;
@@ -100,6 +101,7 @@ static void virtio_net_reset(VirtIODevice *vdev)
 
     /* Flush any MAC and VLAN filter table state */
     n->mac_table.in_use = 0;
+    n->mac_table.first_multi = 0;
     n->mac_table.multi_overflow = 0;
     n->mac_table.uni_overflow = 0;
     memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
@@ -172,6 +174,7 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
         return VIRTIO_NET_ERR;
 
     n->mac_table.in_use = 0;
+    n->mac_table.first_multi = 0;
     n->mac_table.uni_overflow = 0;
     n->mac_table.multi_overflow = 0;
     memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
@@ -190,6 +193,8 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
         n->mac_table.uni_overflow = 1;
     }
 
+    n->mac_table.first_multi = n->mac_table.in_use;
+
     mac_data.entries = ldl_le_p(elem->out_sg[2].iov_base);
 
     if (sizeof(mac_data.entries) +
@@ -356,17 +361,24 @@ static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)
         } else if (n->allmulti || n->mac_table.multi_overflow) {
             return 1;
         }
+
+        for (i = n->mac_table.first_multi; i < n->mac_table.in_use; i++) {
+            if (!memcmp(ptr, &n->mac_table.macs[i * ETH_ALEN], ETH_ALEN)) {
+                return 1;
+            }
+        }
     } else { // unicast
         if (n->mac_table.uni_overflow) {
             return 1;
         } else if (!memcmp(ptr, n->mac, ETH_ALEN)) {
             return 1;
         }
-    }
 
-    for (i = 0; i < n->mac_table.in_use; i++) {
-        if (!memcmp(ptr, &n->mac_table.macs[i * ETH_ALEN], ETH_ALEN))
-            return 1;
+        for (i = 0; i < n->mac_table.first_multi; i++) {
+            if (!memcmp(ptr, &n->mac_table.macs[i * ETH_ALEN], ETH_ALEN)) {
+                return 1;
+            }
+        }
     }
 
     return 0;
@@ -542,6 +554,7 @@ static void virtio_net_save(QEMUFile *f, void *opaque)
 static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
 {
     VirtIONet *n = opaque;
+    int i;
 
     if (version_id < 2 || version_id > VIRTIO_NET_VM_VERSION)
         return -EINVAL;
@@ -592,6 +605,14 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
         n->mac_table.uni_overflow = qemu_get_byte(f);
     }
 
+    /* Find the first multicast entry in the saved MAC filter */
+    for (i = 0; i < n->mac_table.in_use; i++) {
+        if (n->mac_table.macs[i * ETH_ALEN] & 1) {
+            break;
+        }
+    }
+    n->mac_table.first_multi = i;
+
     if (n->tx_timer_active) {
         qemu_mod_timer(n->tx_timer,
                        qemu_get_clock(vm_clock) + TX_TIMER_INTERVAL);

[Qemu-devel] [PATCH 6/7] virtio-net: Add new RX filter controls

[Alex Williamson]

Add a few new RX modes to better control the receive_filter.  These
are all fairly obvious features that hardware could provide.

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

 hw/virtio-net.c |   40 ++++++++++++++++++++++++++++++++++++----
 hw/virtio-net.h |   14 ++++++++++----
 2 files changed, 46 insertions(+), 8 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 5239cc0..ecb58de 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -16,7 +16,7 @@
 #include "qemu-timer.h"
 #include "virtio-net.h"
 
-#define VIRTIO_NET_VM_VERSION    9
+#define VIRTIO_NET_VM_VERSION    10
 
 #define MAC_TABLE_ENTRIES    32
 #define MAX_VLAN    (1 << 12)   /* Per 802.1Q definition */
@@ -35,6 +35,10 @@ typedef struct VirtIONet
     int mergeable_rx_bufs;
     uint8_t promisc;
     uint8_t allmulti;
+    uint8_t alluni;
+    uint8_t nomulti;
+    uint8_t nouni;
+    uint8_t nobcast;
     struct {
         int in_use;
         int first_multi;
@@ -98,6 +102,10 @@ static void virtio_net_reset(VirtIODevice *vdev)
     /* Reset back to compatibility mode */
     n->promisc = 1;
     n->allmulti = 0;
+    n->alluni = 0;
+    n->nomulti = 0;
+    n->nouni = 0;
+    n->nobcast = 0;
 
     /* Flush any MAC and VLAN filter table state */
     n->mac_table.in_use = 0;
@@ -114,7 +122,8 @@ static uint32_t virtio_net_get_features(VirtIODevice *vdev)
                         (1 << VIRTIO_NET_F_STATUS) |
                         (1 << VIRTIO_NET_F_CTRL_VQ) |
                         (1 << VIRTIO_NET_F_CTRL_RX) |
-                        (1 << VIRTIO_NET_F_CTRL_VLAN);
+                        (1 << VIRTIO_NET_F_CTRL_VLAN) |
+                        (1 << VIRTIO_NET_F_CTRL_RX_EXTRA);
 
     return features;
 }
@@ -157,6 +166,14 @@ static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd,
         n->promisc = on;
     else if (cmd == VIRTIO_NET_CTRL_RX_MODE_ALLMULTI)
         n->allmulti = on;
+    else if (cmd == VIRTIO_NET_CTRL_RX_MODE_ALLUNI)
+        n->alluni = on;
+    else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOMULTI)
+        n->nomulti = on;
+    else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOUNI)
+        n->nouni = on;
+    else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOBCAST)
+        n->nobcast = on;
     else
         return VIRTIO_NET_ERR;
 
@@ -357,7 +374,9 @@ static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)
 
     if (ptr[0] & 1) { // multicast
         if (!memcmp(ptr, bcast, sizeof(bcast))) {
-            return 1;
+            return !n->nobcast;
+        } else if (n->nomulti) {
+            return 0;
         } else if (n->allmulti || n->mac_table.multi_overflow) {
             return 1;
         }
@@ -368,7 +387,9 @@ static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)
             }
         }
     } else { // unicast
-        if (n->mac_table.uni_overflow) {
+        if (n->nouni) {
+            return 0;
+        } else if (n->alluni || n->mac_table.uni_overflow) {
             return 1;
         } else if (!memcmp(ptr, n->mac, ETH_ALEN)) {
             return 1;
@@ -549,6 +570,10 @@ static void virtio_net_save(QEMUFile *f, void *opaque)
     qemu_put_be32(f, 0); /* vnet-hdr placeholder */
     qemu_put_byte(f, n->mac_table.multi_overflow);
     qemu_put_byte(f, n->mac_table.uni_overflow);
+    qemu_put_byte(f, n->alluni);
+    qemu_put_byte(f, n->nomulti);
+    qemu_put_byte(f, n->nouni);
+    qemu_put_byte(f, n->nobcast);
 }
 
 static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
@@ -605,6 +630,13 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
         n->mac_table.uni_overflow = qemu_get_byte(f);
     }
 
+    if (version_id >= 10) {
+        n->alluni = qemu_get_byte(f);
+        n->nomulti = qemu_get_byte(f);
+        n->nouni = qemu_get_byte(f);
+        n->nobcast = qemu_get_byte(f);
+    }
+
     /* Find the first multicast entry in the saved MAC filter */
     for (i = 0; i < n->mac_table.in_use; i++) {
         if (n->mac_table.macs[i * ETH_ALEN] & 1) {
diff --git a/hw/virtio-net.h b/hw/virtio-net.h
index 390fe10..2085181 100644
--- a/hw/virtio-net.h
+++ b/hw/virtio-net.h
@@ -43,6 +43,7 @@
 #define VIRTIO_NET_F_CTRL_VQ    17      /* Control channel available */
 #define VIRTIO_NET_F_CTRL_RX    18      /* Control channel RX mode support */
 #define VIRTIO_NET_F_CTRL_VLAN  19      /* Control channel VLAN filtering */
+#define VIRTIO_NET_F_CTRL_RX_EXTRA 20   /* Extra RX mode control support */
 
 #define VIRTIO_NET_S_LINK_UP    1       /* Link is up */
 
@@ -103,14 +104,19 @@ typedef uint8_t virtio_net_ctrl_ack;
 #define VIRTIO_NET_ERR    1
 
 /*
- * Control the RX mode, ie. promisucous and allmulti.  PROMISC and
- * ALLMULTI commands require an "out" sg entry containing a 1 byte
- * state value, zero = disable, non-zero = enable.  These commands
- * are supported with the VIRTIO_NET_F_CTRL_RX feature.
+ * Control the RX mode, ie. promisucous, allmulti, etc...
+ * All commands require an "out" sg entry containing a 1 byte
+ * state value, zero = disable, non-zero = enable.  Commands
+ * 0 and 1 are supported with the VIRTIO_NET_F_CTRL_RX feature.
+ * Commands 2-5 are added with VIRTIO_NET_F_CTRL_RX_EXTRA.
  */
 #define VIRTIO_NET_CTRL_RX_MODE    0
  #define VIRTIO_NET_CTRL_RX_MODE_PROMISC      0
  #define VIRTIO_NET_CTRL_RX_MODE_ALLMULTI     1
+ #define VIRTIO_NET_CTRL_RX_MODE_ALLUNI       2
+ #define VIRTIO_NET_CTRL_RX_MODE_NOMULTI      3
+ #define VIRTIO_NET_CTRL_RX_MODE_NOUNI        4
+ #define VIRTIO_NET_CTRL_RX_MODE_NOBCAST      5
 
 /*
  * Control the MAC filter table.

Re: [Qemu-devel] [PATCH 6/7] virtio-net: Add new RX filter controls

[Michael S. Tsirkin]

On Fri, Jun 05, 2009 at 02:47:18PM -0600, Alex Williamson wrote:
> Add a few new RX modes to better control the receive_filter.  These
> are all fairly obvious features that hardware could provide.

Could you add a bit more detail on motivation for these
features please?


> 
> Signed-off-by: Alex Williamson <alex.williamson@hp.com>
> ---
> 
>  hw/virtio-net.c |   40 ++++++++++++++++++++++++++++++++++++----
>  hw/virtio-net.h |   14 ++++++++++----
>  2 files changed, 46 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index 5239cc0..ecb58de 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -16,7 +16,7 @@
>  #include "qemu-timer.h"
>  #include "virtio-net.h"
>  
> -#define VIRTIO_NET_VM_VERSION    9
> +#define VIRTIO_NET_VM_VERSION    10
>  
>  #define MAC_TABLE_ENTRIES    32
>  #define MAX_VLAN    (1 << 12)   /* Per 802.1Q definition */
> @@ -35,6 +35,10 @@ typedef struct VirtIONet
>      int mergeable_rx_bufs;
>      uint8_t promisc;
>      uint8_t allmulti;
> +    uint8_t alluni;
> +    uint8_t nomulti;
> +    uint8_t nouni;
> +    uint8_t nobcast;
>      struct {
>          int in_use;
>          int first_multi;
> @@ -98,6 +102,10 @@ static void virtio_net_reset(VirtIODevice *vdev)
>      /* Reset back to compatibility mode */
>      n->promisc = 1;
>      n->allmulti = 0;
> +    n->alluni = 0;
> +    n->nomulti = 0;
> +    n->nouni = 0;
> +    n->nobcast = 0;
>  
>      /* Flush any MAC and VLAN filter table state */
>      n->mac_table.in_use = 0;
> @@ -114,7 +122,8 @@ static uint32_t virtio_net_get_features(VirtIODevice *vdev)
>                          (1 << VIRTIO_NET_F_STATUS) |
>                          (1 << VIRTIO_NET_F_CTRL_VQ) |
>                          (1 << VIRTIO_NET_F_CTRL_RX) |
> -                        (1 << VIRTIO_NET_F_CTRL_VLAN);
> +                        (1 << VIRTIO_NET_F_CTRL_VLAN) |
> +                        (1 << VIRTIO_NET_F_CTRL_RX_EXTRA);
>  
>      return features;
>  }
> @@ -157,6 +166,14 @@ static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd,
>          n->promisc = on;
>      else if (cmd == VIRTIO_NET_CTRL_RX_MODE_ALLMULTI)
>          n->allmulti = on;
> +    else if (cmd == VIRTIO_NET_CTRL_RX_MODE_ALLUNI)
> +        n->alluni = on;
> +    else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOMULTI)
> +        n->nomulti = on;
> +    else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOUNI)
> +        n->nouni = on;
> +    else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOBCAST)
> +        n->nobcast = on;
>      else
>          return VIRTIO_NET_ERR;
>  
> @@ -357,7 +374,9 @@ static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)
>  
>      if (ptr[0] & 1) { // multicast
>          if (!memcmp(ptr, bcast, sizeof(bcast))) {
> -            return 1;
> +            return !n->nobcast;
> +        } else if (n->nomulti) {
> +            return 0;
>          } else if (n->allmulti || n->mac_table.multi_overflow) {
>              return 1;
>          }
> @@ -368,7 +387,9 @@ static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)
>              }
>          }
>      } else { // unicast
> -        if (n->mac_table.uni_overflow) {
> +        if (n->nouni) {
> +            return 0;
> +        } else if (n->alluni || n->mac_table.uni_overflow) {
>              return 1;
>          } else if (!memcmp(ptr, n->mac, ETH_ALEN)) {
>              return 1;
> @@ -549,6 +570,10 @@ static void virtio_net_save(QEMUFile *f, void *opaque)
>      qemu_put_be32(f, 0); /* vnet-hdr placeholder */
>      qemu_put_byte(f, n->mac_table.multi_overflow);
>      qemu_put_byte(f, n->mac_table.uni_overflow);
> +    qemu_put_byte(f, n->alluni);
> +    qemu_put_byte(f, n->nomulti);
> +    qemu_put_byte(f, n->nouni);
> +    qemu_put_byte(f, n->nobcast);
>  }
>  
>  static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
> @@ -605,6 +630,13 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
>          n->mac_table.uni_overflow = qemu_get_byte(f);
>      }
>  
> +    if (version_id >= 10) {
> +        n->alluni = qemu_get_byte(f);
> +        n->nomulti = qemu_get_byte(f);
> +        n->nouni = qemu_get_byte(f);
> +        n->nobcast = qemu_get_byte(f);
> +    }
> +
>      /* Find the first multicast entry in the saved MAC filter */
>      for (i = 0; i < n->mac_table.in_use; i++) {
>          if (n->mac_table.macs[i * ETH_ALEN] & 1) {
> diff --git a/hw/virtio-net.h b/hw/virtio-net.h
> index 390fe10..2085181 100644
> --- a/hw/virtio-net.h
> +++ b/hw/virtio-net.h
> @@ -43,6 +43,7 @@
>  #define VIRTIO_NET_F_CTRL_VQ    17      /* Control channel available */
>  #define VIRTIO_NET_F_CTRL_RX    18      /* Control channel RX mode support */
>  #define VIRTIO_NET_F_CTRL_VLAN  19      /* Control channel VLAN filtering */
> +#define VIRTIO_NET_F_CTRL_RX_EXTRA 20   /* Extra RX mode control support */
>  
>  #define VIRTIO_NET_S_LINK_UP    1       /* Link is up */
>  
> @@ -103,14 +104,19 @@ typedef uint8_t virtio_net_ctrl_ack;
>  #define VIRTIO_NET_ERR    1
>  
>  /*
> - * Control the RX mode, ie. promisucous and allmulti.  PROMISC and
> - * ALLMULTI commands require an "out" sg entry containing a 1 byte
> - * state value, zero = disable, non-zero = enable.  These commands
> - * are supported with the VIRTIO_NET_F_CTRL_RX feature.
> + * Control the RX mode, ie. promisucous, allmulti, etc...
> + * All commands require an "out" sg entry containing a 1 byte
> + * state value, zero = disable, non-zero = enable.  Commands
> + * 0 and 1 are supported with the VIRTIO_NET_F_CTRL_RX feature.
> + * Commands 2-5 are added with VIRTIO_NET_F_CTRL_RX_EXTRA.
>   */
>  #define VIRTIO_NET_CTRL_RX_MODE    0
>   #define VIRTIO_NET_CTRL_RX_MODE_PROMISC      0
>   #define VIRTIO_NET_CTRL_RX_MODE_ALLMULTI     1
> + #define VIRTIO_NET_CTRL_RX_MODE_ALLUNI       2
> + #define VIRTIO_NET_CTRL_RX_MODE_NOMULTI      3
> + #define VIRTIO_NET_CTRL_RX_MODE_NOUNI        4
> + #define VIRTIO_NET_CTRL_RX_MODE_NOBCAST      5
>  
>  /*
>   * Control the MAC filter table.
> 
> 

-- 
MST

Re: [Qemu-devel] [PATCH 6/7] virtio-net: Add new RX filter controls

[Alex Williamson]

On Sat, Jun 6, 2009 at 2:48 PM, Michael S. Tsirkin<mst@redhat.com> wrote:
> On Fri, Jun 05, 2009 at 02:47:18PM -0600, Alex Williamson wrote:
>> Add a few new RX modes to better control the receive_filter.  These
>> are all fairly obvious features that hardware could provide.
>
> Could you add a bit more detail on motivation for these
> features please?

Sure, e1000 offers RX controls which separate unicast and multicast
promiscuous, these are replicated with allmulti, which has a direct
mapping for Linux, and thus already exists, and alluni, to accept all
unicast packets.  With these we could actually do away with
promiscuous, but since we have to dig into the header to determine the
type of packet, I chose to leave promiscuous separate.  Promiscuous
also filters before vlan filtering, so has a slightly different
meaning (maybe all-multi/uni should too?).

e1000 also allows the driver to selectively enable/disable RX of
packets to the broadcast address.  This is replicated with the
all/no-bcast options.  Finally, there may be cases where we want to
receive only unicast or only multicast address for special purpose
network devices.  This is provided by the nouni and nomulti options.
A proprietary guest know as DMX intends to make use of these extra
modes.  Are there any other interesting, useful and lightweight packet
filters we could implement?  Thanks,

Alex

Re: [Qemu-devel] [PATCH 6/7] virtio-net: Add new RX filter controls

[Anthony Liguori]

Alex Williamson wrote:
> e1000 also allows the driver to selectively enable/disable RX of
> packets to the broadcast address.  This is replicated with the
> all/no-bcast options.  Finally, there may be cases where we want to
> receive only unicast or only multicast address for special purpose
> network devices.  This is provided by the nouni and nomulti options.
> A proprietary guest know as DMX intends to make use of these extra
> modes.  Are there any other interesting, useful and lightweight packet
> filters we could implement?  Thanks,
>   

I've been thinking about whether doing VLAN filtering/tagging within 
QEMU would make sense.  It could potentially simplify bridge setups 
tremendously.  Today, if you want to isolate VMs on separate vlans, it 
involves creating multiple bridges which gets ugly quickly.

Of course, for that to be useful, it should happen at the generic 
networking layer in QEMU, not specific to virtio-net.

Regards,

Anthony Liguori

> Alex
>
>
>   

Re: [Qemu-devel] [PATCH 6/7] virtio-net: Add new RX filter controls

[Daniel P. Berrange]

On Mon, Jun 08, 2009 at 02:18:04PM -0500, Anthony Liguori wrote:
> Alex Williamson wrote:
> >e1000 also allows the driver to selectively enable/disable RX of
> >packets to the broadcast address.  This is replicated with the
> >all/no-bcast options.  Finally, there may be cases where we want to
> >receive only unicast or only multicast address for special purpose
> >network devices.  This is provided by the nouni and nomulti options.
> >A proprietary guest know as DMX intends to make use of these extra
> >modes.  Are there any other interesting, useful and lightweight packet
> >filters we could implement?  Thanks,
> >  
> 
> I've been thinking about whether doing VLAN filtering/tagging within 
> QEMU would make sense.  It could potentially simplify bridge setups 
> tremendously.  Today, if you want to isolate VMs on separate vlans, it 
> involves creating multiple bridges which gets ugly quickly.

The downside of that would be that you're trusting the integrity of
QEMU for VLAN filtering. If QEMU got compromised then it could get
outside the configured VLAN, which is not possible if the VLAN stuff
is done by the kernel (assuming the QEMU process does not have the
capabilities to add itself to other bridges).

Regards,
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [Qemu-devel] [PATCH 6/7] virtio-net: Add new RX filter controls

[Anthony Liguori]

Daniel P. Berrange wrote:
> On Mon, Jun 08, 2009 at 02:18:04PM -0500, Anthony Liguori wrote:
>   
>> Alex Williamson wrote:
>>     
>>> e1000 also allows the driver to selectively enable/disable RX of
>>> packets to the broadcast address.  This is replicated with the
>>> all/no-bcast options.  Finally, there may be cases where we want to
>>> receive only unicast or only multicast address for special purpose
>>> network devices.  This is provided by the nouni and nomulti options.
>>> A proprietary guest know as DMX intends to make use of these extra
>>> modes.  Are there any other interesting, useful and lightweight packet
>>> filters we could implement?  Thanks,
>>>  
>>>       
>> I've been thinking about whether doing VLAN filtering/tagging within 
>> QEMU would make sense.  It could potentially simplify bridge setups 
>> tremendously.  Today, if you want to isolate VMs on separate vlans, it 
>> involves creating multiple bridges which gets ugly quickly.
>>     
>
> The downside of that would be that you're trusting the integrity of
> QEMU for VLAN filtering. If QEMU got compromised then it could get
> outside the configured VLAN, which is not possible if the VLAN stuff
> is done by the kernel (assuming the QEMU process does not have the
> capabilities to add itself to other bridges).
>   

I guess that you can do:

tunctl -p -t tap0
ifconfig tap0 0.0.0.0 up
vconfig add tap0 32
brctl addif br0 tap0

And then use tap0.32 as your device for QEMU.  The awkward thing though 
is that I don't think you can use TUNSETIFF to set the tun device name 
to tap0.32.

But basically, this is the level of functionality that I think is need.  
The current mechanism of:

vconfig add eth0 32
brctl addif br0 eth0.32
tunctl -p -t tap0
ifconfig tap0 0.0.0.0 up
brctl addif br0 tap0

Is a pain because then you need a bridge for every possible vlan.  
Things get even more complicated when you have to deal with live 
migration and nested vlan tags.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 6/7] virtio-net: Add new RX filter controls

[Daniel P. Berrange]

On Mon, Jun 08, 2009 at 04:03:50PM -0500, Anthony Liguori wrote:
> Daniel P. Berrange wrote:
> >On Mon, Jun 08, 2009 at 02:18:04PM -0500, Anthony Liguori wrote:
> >  
> >>Alex Williamson wrote:
> >>    
> >>>e1000 also allows the driver to selectively enable/disable RX of
> >>>packets to the broadcast address.  This is replicated with the
> >>>all/no-bcast options.  Finally, there may be cases where we want to
> >>>receive only unicast or only multicast address for special purpose
> >>>network devices.  This is provided by the nouni and nomulti options.
> >>>A proprietary guest know as DMX intends to make use of these extra
> >>>modes.  Are there any other interesting, useful and lightweight packet
> >>>filters we could implement?  Thanks,
> >>> 
> >>>      
> >>I've been thinking about whether doing VLAN filtering/tagging within 
> >>QEMU would make sense.  It could potentially simplify bridge setups 
> >>tremendously.  Today, if you want to isolate VMs on separate vlans, it 
> >>involves creating multiple bridges which gets ugly quickly.
> >>    
> >
> >The downside of that would be that you're trusting the integrity of
> >QEMU for VLAN filtering. If QEMU got compromised then it could get
> >outside the configured VLAN, which is not possible if the VLAN stuff
> >is done by the kernel (assuming the QEMU process does not have the
> >capabilities to add itself to other bridges).
> >  
> 
> I guess that you can do:
> 
> tunctl -p -t tap0
> ifconfig tap0 0.0.0.0 up
> vconfig add tap0 32
> brctl addif br0 tap0
> 
> And then use tap0.32 as your device for QEMU.  The awkward thing though 
> is that I don't think you can use TUNSETIFF to set the tun device name 
> to tap0.32.
> 
> But basically, this is the level of functionality that I think is need.  
> The current mechanism of:
> 
> vconfig add eth0 32
> brctl addif br0 eth0.32
> tunctl -p -t tap0
> ifconfig tap0 0.0.0.0 up
> brctl addif br0 tap0
> 
> Is a pain because then you need a bridge for every possible vlan.  
> Things get even more complicated when you have to deal with live 
> migration and nested vlan tags.

Yeah, one bridge per VLAN is the way most people I know currently do
VLANs with Xen/KVM. In fact they're typically more complicated, because
they'll first put a couple of NICs into a bonding device, Then create 
VLAN devices on the the bond, then put each into a bridge, before finally
adding the TAP devices. 

Personally I'd just say the bridge config task is the management tool's 
problem to deal with. A mgmt tool UI shouldn't really need to expose 
the raw details of physical NICs, bond devices, VLAN devices & bridge 
devices to the user. Instead allow them to say 'create a network sharing
two physical NICs on VLAN 53', and then have it automagically setup the
neccessary individal devices  behind the scenes.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [Qemu-devel] [PATCH 6/7] virtio-net: Add new RX filter controls

[Jamie Lokier]

Daniel P. Berrange wrote:
> Personally I'd just say the bridge config task is the management tool's 
> problem to deal with. A mgmt tool UI shouldn't really need to expose 
> the raw details of physical NICs, bond devices, VLAN devices & bridge 
> devices to the user. Instead allow them to say 'create a network sharing
> two physical NICs on VLAN 53', and then have it automagically setup the
> neccessary individal devices  behind the scenes.

That's a nice idea, but it depends so intimately on the host's network
configuration, that it doesn't work in practice (in my experince),
except in some rather fixed configurations.

The problem is: when you attach a bridge device, then all the host's
IP configuration has to be done on the bridge device instead of the
host's network interfaces.

That means either:

    - Host's network configuration (outside the management tool) must
      create bridges in advance (with one port) _just_ so that VM
      management can attach to the bridge.  It's hardly transparent.
      Doesn't work at all with NetworkManager or any ordinary host
      network configurations, for example.  And you have to do it in
      advance, not when starting VMs.

    - Or, the VM management tool must create bridges when VMs are
      started, and then copy the IP configuration from the host
      network interface to the bridge, and it must somehow trick the
      host's DHCP client to moving to the bridge interface, etc.  This
      doesn't work with NetworkManager either.

Quite possibly it's a mess because of the way Linux does bridging, but
still it is.

I haven't found any solution which works on a laptop running
NetworkManager or other automatic network binding service.

For servers with static IPs and simple network configuration it is
easier, and of course a VM management tool can always handle specific
cases like that, if told to.  But even on servers, I find if they have
a complex host network configuration (e.g. policy routing tables),
adding bridges for the VMs is not something that can be done
automatically.

Ideally, there would be a way to add a "bridge for VM" which hangs off
the edge of the host's networking, instead of disruptively having to
be in the middle.

The pcap interface is close to that for ease of configurability, but a
bridge would behave better, especially with multiple VMs, and maybe
perform better.

-- Jamie

[Qemu-devel] Re: [PATCH 6/7] virtio-net: Add new RX filter controls

[Jan Kiszka]

Jamie Lokier wrote:
> Daniel P. Berrange wrote:
>> Personally I'd just say the bridge config task is the management tool's 
>> problem to deal with. A mgmt tool UI shouldn't really need to expose 
>> the raw details of physical NICs, bond devices, VLAN devices & bridge 
>> devices to the user. Instead allow them to say 'create a network sharing
>> two physical NICs on VLAN 53', and then have it automagically setup the
>> neccessary individal devices  behind the scenes.
> 
> That's a nice idea, but it depends so intimately on the host's network
> configuration, that it doesn't work in practice (in my experince),
> except in some rather fixed configurations.
> 
> The problem is: when you attach a bridge device, then all the host's
> IP configuration has to be done on the bridge device instead of the
> host's network interfaces.
> 
> That means either:
> 
>     - Host's network configuration (outside the management tool) must
>       create bridges in advance (with one port) _just_ so that VM
>       management can attach to the bridge.  It's hardly transparent.
>       Doesn't work at all with NetworkManager or any ordinary host
>       network configurations, for example.  And you have to do it in
>       advance, not when starting VMs.
> 
>     - Or, the VM management tool must create bridges when VMs are
>       started, and then copy the IP configuration from the host
>       network interface to the bridge, and it must somehow trick the
>       host's DHCP client to moving to the bridge interface, etc.  This
>       doesn't work with NetworkManager either.
> 
> Quite possibly it's a mess because of the way Linux does bridging, but
> still it is.
> 
> I haven't found any solution which works on a laptop running
> NetworkManager or other automatic network binding service.
> 
> For servers with static IPs and simple network configuration it is
> easier, and of course a VM management tool can always handle specific
> cases like that, if told to.  But even on servers, I find if they have
> a complex host network configuration (e.g. policy routing tables),
> adding bridges for the VMs is not something that can be done
> automatically.
> 
> Ideally, there would be a way to add a "bridge for VM" which hangs off
> the edge of the host's networking, instead of disruptively having to
> be in the middle.
> 
> The pcap interface is close to that for ease of configurability, but a
> bridge would behave better, especially with multiple VMs, and maybe
> perform better.

Pcap on Linux suffers from the limitation that injected packets are not
visible to the host, thus guest<->host communication doesn't work. The
same is true for PF_PACKET (or does libpcap actually use that
internally?). Haven't analyzed the reasons in details yet, but I bet
it's not solvable in user space.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH 6/7] virtio-net: Add new RX filter controls

[Jamie Lokier]

Jan Kiszka wrote:
> > The pcap interface is close to that for ease of configurability, but a
> > bridge would behave better, especially with multiple VMs, and maybe
> > perform better.
> 
> Pcap on Linux suffers from the limitation that injected packets are not
> visible to the host, thus guest<->host communication doesn't work.

Ew.

> The same is true for PF_PACKET (or does libpcap actually use that
> internally?).

Yes it does.

> Haven't analyzed the reasons in details yet, but I bet
> it's not solvable in user space.

I think that's probably right, and good solutions would be:

   - A new option to the kernel bridging to attach a bridge
     to an existing net interface in a way which allows the interface's IP
     configuration to keep working

   - An alternate pcap mode which makes packets visible to the host.

   - An "auto-bridging" tap device mode, where it's told which network
     interface to bridge to, with an invisible bridge.

Bridging would be better than pcap because it can more easily take
advantage of multiple MAC address support in the network interface
(like macvlan), to filter properly, although I don't know if the
existing Linux bridge code does that.  And it more closely resembles
what you'd do with physical machines instead of VMs, which is plug
them into a switch.

-- Jamie

[Qemu-devel] Re: [PATCH 6/7] virtio-net: Add new RX filter controls

[Michael S. Tsirkin]

On Tue, Jun 09, 2009 at 05:42:32PM +0200, Jan Kiszka wrote:
> The same is true for PF_PACKET (or does libpcap actually use that
> internally?). Haven't analyzed the reasons in details yet, but I bet
> it's not solvable in user space.
> 
> Jan

I think you can load the veth module, and attach veth to a bridge.

-- 
MST

[Qemu-devel] Re: [PATCH 6/7] virtio-net: Add new RX filter controls

[Jan Kiszka]

Michael S. Tsirkin wrote:
> On Tue, Jun 09, 2009 at 05:42:32PM +0200, Jan Kiszka wrote:
>> The same is true for PF_PACKET (or does libpcap actually use that
>> internally?). Haven't analyzed the reasons in details yet, but I bet
>> it's not solvable in user space.
>>
>> Jan
> 
> I think you can load the veth module, and attach veth to a bridge.

Sorry, my brain is not yet working at full speed: What do we gain for
the initial problem that we want to bridge to an existing network device
without having to move management tools like dhcpcd to the corresponding
brX?

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH 6/7] virtio-net: Add new RX filter controls

[Michael S. Tsirkin]

On Wed, Jun 10, 2009 at 10:58:44AM +0200, Jan Kiszka wrote:
> Michael S. Tsirkin wrote:
> > On Tue, Jun 09, 2009 at 05:42:32PM +0200, Jan Kiszka wrote:
> >> The same is true for PF_PACKET (or does libpcap actually use that
> >> internally?). Haven't analyzed the reasons in details yet, but I bet
> >> it's not solvable in user space.
> >>
> >> Jan
> > 
> > I think you can load the veth module, and attach veth to a bridge.
> 
> Sorry, my brain is not yet working at full speed: What do we gain for
> the initial problem that we want to bridge to an existing network device
> without having to move management tools like dhcpcd to the corresponding
> brX?
> 
> Jan

Nothing :). I was only saying that IIUC the problem is not with
PF_PACKET itself - I think that PF_PACKET + veth can be used
as a replacement for tap.

-- 
MST

Re: [Qemu-devel] Re: [PATCH 6/7] virtio-net: Add new RX filter controls

[Gleb Natapov]

On Wed, Jun 10, 2009 at 12:07:59PM +0300, Michael S. Tsirkin wrote:
> On Wed, Jun 10, 2009 at 10:58:44AM +0200, Jan Kiszka wrote:
> > Michael S. Tsirkin wrote:
> > > On Tue, Jun 09, 2009 at 05:42:32PM +0200, Jan Kiszka wrote:
> > >> The same is true for PF_PACKET (or does libpcap actually use that
> > >> internally?). Haven't analyzed the reasons in details yet, but I bet
> > >> it's not solvable in user space.
> > >>
> > >> Jan
> > > 
> > > I think you can load the veth module, and attach veth to a bridge.
> > 
> > Sorry, my brain is not yet working at full speed: What do we gain for
> > the initial problem that we want to bridge to an existing network device
> > without having to move management tools like dhcpcd to the corresponding
> > brX?
> > 
> > Jan
> 
> Nothing :). I was only saying that IIUC the problem is not with
> PF_PACKET itself - I think that PF_PACKET + veth can be used
> as a replacement for tap.
> 
And the point is...?

--
			Gleb.

Re: [Qemu-devel] Re: [PATCH 6/7] virtio-net: Add new RX filter controls

[Michael S. Tsirkin]

On Wed, Jun 10, 2009 at 12:13:21PM +0300, Gleb Natapov wrote:
> On Wed, Jun 10, 2009 at 12:07:59PM +0300, Michael S. Tsirkin wrote:
> > On Wed, Jun 10, 2009 at 10:58:44AM +0200, Jan Kiszka wrote:
> > > Michael S. Tsirkin wrote:
> > > > On Tue, Jun 09, 2009 at 05:42:32PM +0200, Jan Kiszka wrote:
> > > >> The same is true for PF_PACKET (or does libpcap actually use that
> > > >> internally?). Haven't analyzed the reasons in details yet, but I bet
> > > >> it's not solvable in user space.
> > > >>
> > > >> Jan
> > > > 
> > > > I think you can load the veth module, and attach veth to a bridge.
> > > 
> > > Sorry, my brain is not yet working at full speed: What do we gain for
> > > the initial problem that we want to bridge to an existing network device
> > > without having to move management tools like dhcpcd to the corresponding
> > > brX?
> > > 
> > > Jan
> > 
> > Nothing :). I was only saying that IIUC the problem is not with
> > PF_PACKET itself - I think that PF_PACKET + veth can be used
> > as a replacement for tap.
> > 
> And the point is...?

tap requires bridging, PF_PACKET can attach to a physical device.

-- 
MST

Re: [Qemu-devel] Re: [PATCH 6/7] virtio-net: Add new RX filter controls

[Gleb Natapov]

On Wed, Jun 10, 2009 at 12:17:55PM +0300, Michael S. Tsirkin wrote:
> On Wed, Jun 10, 2009 at 12:13:21PM +0300, Gleb Natapov wrote:
> > On Wed, Jun 10, 2009 at 12:07:59PM +0300, Michael S. Tsirkin wrote:
> > > On Wed, Jun 10, 2009 at 10:58:44AM +0200, Jan Kiszka wrote:
> > > > Michael S. Tsirkin wrote:
> > > > > On Tue, Jun 09, 2009 at 05:42:32PM +0200, Jan Kiszka wrote:
> > > > >> The same is true for PF_PACKET (or does libpcap actually use that
> > > > >> internally?). Haven't analyzed the reasons in details yet, but I bet
> > > > >> it's not solvable in user space.
> > > > >>
> > > > >> Jan
> > > > > 
> > > > > I think you can load the veth module, and attach veth to a bridge.
> > > > 
> > > > Sorry, my brain is not yet working at full speed: What do we gain for
> > > > the initial problem that we want to bridge to an existing network device
> > > > without having to move management tools like dhcpcd to the corresponding
> > > > brX?
> > > > 
> > > > Jan
> > > 
> > > Nothing :). I was only saying that IIUC the problem is not with
> > > PF_PACKET itself - I think that PF_PACKET + veth can be used
> > > as a replacement for tap.
> > > 
> > And the point is...?
> 
> tap requires bridging, PF_PACKET can attach to a physical device.
> 
Why tap requires bridging? User requires bridging, so he uses bridge.
Look above, you wrote "I think you can load the veth module, and attach
veth to a bridge."

--
			Gleb.

Re: [Qemu-devel] Re: [PATCH 6/7] virtio-net: Add new RX filter controls

[Michael S. Tsirkin]

On Wed, Jun 10, 2009 at 12:22:35PM +0300, Gleb Natapov wrote:
> On Wed, Jun 10, 2009 at 12:17:55PM +0300, Michael S. Tsirkin wrote:
> > On Wed, Jun 10, 2009 at 12:13:21PM +0300, Gleb Natapov wrote:
> > > On Wed, Jun 10, 2009 at 12:07:59PM +0300, Michael S. Tsirkin wrote:
> > > > On Wed, Jun 10, 2009 at 10:58:44AM +0200, Jan Kiszka wrote:
> > > > > Michael S. Tsirkin wrote:
> > > > > > On Tue, Jun 09, 2009 at 05:42:32PM +0200, Jan Kiszka wrote:
> > > > > >> The same is true for PF_PACKET (or does libpcap actually use that
> > > > > >> internally?). Haven't analyzed the reasons in details yet, but I bet
> > > > > >> it's not solvable in user space.
> > > > > >>
> > > > > >> Jan
> > > > > > 
> > > > > > I think you can load the veth module, and attach veth to a bridge.
> > > > > 
> > > > > Sorry, my brain is not yet working at full speed: What do we gain for
> > > > > the initial problem that we want to bridge to an existing network device
> > > > > without having to move management tools like dhcpcd to the corresponding
> > > > > brX?
> > > > > 
> > > > > Jan
> > > > 
> > > > Nothing :). I was only saying that IIUC the problem is not with
> > > > PF_PACKET itself - I think that PF_PACKET + veth can be used
> > > > as a replacement for tap.
> > > > 
> > > And the point is...?
> > 
> > tap requires bridging, PF_PACKET can attach to a physical device.
> > 
> Why tap requires bridging? User requires bridging, so he uses bridge.

Yea, good point :)

> Look above, you wrote "I think you can load the veth module, and attach
> veth to a bridge."


-- 
MST

Re: [Qemu-devel] [PATCH 6/7] virtio-net: Add new RX filter controls

[Alex Williamson]

On Mon, Jun 8, 2009 at 1:18 PM, Anthony Liguori<anthony@codemonkey.ws> wrote:
> Alex Williamson wrote:
>>
>> e1000 also allows the driver to selectively enable/disable RX of
>> packets to the broadcast address.  This is replicated with the
>> all/no-bcast options.  Finally, there may be cases where we want to
>> receive only unicast or only multicast address for special purpose
>> network devices.  This is provided by the nouni and nomulti options.
>> A proprietary guest know as DMX intends to make use of these extra
>> modes.  Are there any other interesting, useful and lightweight packet
>> filters we could implement?  Thanks,
>>
>
> I've been thinking about whether doing VLAN filtering/tagging within QEMU
> would make sense.  It could potentially simplify bridge setups tremendously.
>  Today, if you want to isolate VMs on separate vlans, it involves creating
> multiple bridges which gets ugly quickly.

IIRC, you have to be careful that the host NIC doesn't strip the VLAN
tag itself, which means you want the VLAN guests on a non-VLAN bridge.
 It's all rather confusing and I wouldn't be surprised if there's some
dependency on how much offloading the host NIC does.  We do have VLAN
filtering in virtio-net today, but of course it would be better if it
was done at a generic network level in QEMU or pushed deeper into the
host.  Tagging doesn't make much sense at the level we're doing it
now.

Alex

[Qemu-devel] [PATCH 7/7] virtio-net: Increase filter and control limits

[Alex Williamson]

Increase the size of the perfect filter table and control queue depth.
This should give us more headroom in the MAC filter and is known to be
needed by at least one guest user.  Increasing the control queue depth
allows a guest to feed several commands back to back if they so desire
rather than using the send and wait approach Linux uses.

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

 hw/virtio-net.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index ecb58de..efaff04 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -18,7 +18,7 @@
 
 #define VIRTIO_NET_VM_VERSION    10
 
-#define MAC_TABLE_ENTRIES    32
+#define MAC_TABLE_ENTRIES    64
 #define MAX_VLAN    (1 << 12)   /* Per 802.1Q definition */
 
 typedef struct VirtIONet
@@ -685,7 +685,7 @@ VirtIODevice *virtio_net_init(DeviceState *dev)
     n->vdev.reset = virtio_net_reset;
     n->rx_vq = virtio_add_queue(&n->vdev, 256, virtio_net_handle_rx);
     n->tx_vq = virtio_add_queue(&n->vdev, 256, virtio_net_handle_tx);
-    n->ctrl_vq = virtio_add_queue(&n->vdev, 16, virtio_net_handle_ctrl);
+    n->ctrl_vq = virtio_add_queue(&n->vdev, 64, virtio_net_handle_ctrl);
     qdev_get_macaddr(dev, n->mac);
     n->status = VIRTIO_NET_S_LINK_UP;
     n->vc = qdev_get_vlan_client(dev,

Re: [Qemu-devel] [PATCH 7/7] virtio-net: Increase filter and control limits

[Michael S. Tsirkin]

On Fri, Jun 05, 2009 at 02:47:23PM -0600, Alex Williamson wrote:
> Increase the size of the perfect filter table and control queue depth.
> This should give us more headroom in the MAC filter and is known to be
> needed by at least one guest user.

Just curious - which guest is that?

> Increasing the control queue depth
> allows a guest to feed several commands back to back if they so desire
> rather than using the send and wait approach Linux uses.
> 
> Signed-off-by: Alex Williamson <alex.williamson@hp.com>
> ---
> 
>  hw/virtio-net.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index ecb58de..efaff04 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -18,7 +18,7 @@
>  
>  #define VIRTIO_NET_VM_VERSION    10
>  
> -#define MAC_TABLE_ENTRIES    32
> +#define MAC_TABLE_ENTRIES    64
>  #define MAX_VLAN    (1 << 12)   /* Per 802.1Q definition */
>  
>  typedef struct VirtIONet
> @@ -685,7 +685,7 @@ VirtIODevice *virtio_net_init(DeviceState *dev)
>      n->vdev.reset = virtio_net_reset;
>      n->rx_vq = virtio_add_queue(&n->vdev, 256, virtio_net_handle_rx);
>      n->tx_vq = virtio_add_queue(&n->vdev, 256, virtio_net_handle_tx);
> -    n->ctrl_vq = virtio_add_queue(&n->vdev, 16, virtio_net_handle_ctrl);
> +    n->ctrl_vq = virtio_add_queue(&n->vdev, 64, virtio_net_handle_ctrl);
>      qdev_get_macaddr(dev, n->mac);
>      n->status = VIRTIO_NET_S_LINK_UP;
>      n->vc = qdev_get_vlan_client(dev,
> 
> 

-- 
MST

Re: [Qemu-devel] [PATCH 7/7] virtio-net: Increase filter and control limits

[Alex Williamson]

On Sat, Jun 6, 2009 at 2:44 PM, Michael S. Tsirkin<mst@redhat.com> wrote:
> On Fri, Jun 05, 2009 at 02:47:23PM -0600, Alex Williamson wrote:
>> Increase the size of the perfect filter table and control queue depth.
>> This should give us more headroom in the MAC filter and is known to be
>> needed by at least one guest user.
>
> Just curious - which guest is that?

A proprietary OS called DMX.  They'd like to be able to support 40-ish
perfect filter entries via the MAC filter table, as well as push out
several control commands in a row.

Alex

Re: [Qemu-devel] [PATCH 0/7] virtio-net: Filter cleanup/improvements

[Mark McLoughlin]

Hi Alex,

On Fri, 2009-06-05 at 14:46 -0600, Alex Williamson wrote:

> This series cleans up a few things around packet filtering.  I've probably
> gone a little overboard on breaking up patches, if we want to avoid bumping
> the save version_id so much, these could be mostly lumped together.  The
> main features here are more efficient handling of the filtering between
> unicast and multicast, better overflow tracking, adding more RX modes,
> and increasing the size of the filter table and control queue depth.
> 
> I took the reserved version_id 7 for vnet header support into consideration
> on the first patch.  I think we should be able to safely add the guts later
> with the placeholder.  If there are other RX mode controls we should add,
> let me know, now would be a good time to round out any other flags we can
> think of. 

The whole series looks good to me. Please make sure to send Rusty a
patch with the VIRTIO_NET_F_CTRL_RX_EXTRA etc. header additions.

The savevm version_id 9 bump is a little gratuitous, but I don't think
it's a big deal - I'd prefer to see cleanly separated patches like this,
and it wouldn't have been easy to e.g. split out the savevm changes into
a patch of their own at the end.

The vnet header version_id reservation is a good idea.

I've given this stuff some light testing, but would you care to outline
some instructions for some basic tests for as much as possible of this
stuff using a Linux guest? i.e. broadcast, multicast, promisc, mac
table, vlan table etc. - that's quite a set of tests you need to cover
all those cases.

Cheers,
Mark.

Re: [Qemu-devel] [PATCH 0/7] virtio-net: Filter cleanup/improvements

[Alex Williamson]

Hi Mark,

On Tue, 2009-06-09 at 19:25 +0000, Mark McLoughlin wrote:
> On Fri, 2009-06-05 at 14:46 -0600, Alex Williamson wrote:
> 
> > This series cleans up a few things around packet filtering.  I've probably
> > gone a little overboard on breaking up patches, if we want to avoid bumping
> > the save version_id so much, these could be mostly lumped together.  The
> > main features here are more efficient handling of the filtering between
> > unicast and multicast, better overflow tracking, adding more RX modes,
> > and increasing the size of the filter table and control queue depth.
> > 
> > I took the reserved version_id 7 for vnet header support into consideration
> > on the first patch.  I think we should be able to safely add the guts later
> > with the placeholder.  If there are other RX mode controls we should add,
> > let me know, now would be a good time to round out any other flags we can
> > think of. 
> 
> The whole series looks good to me. Please make sure to send Rusty a
> patch with the VIRTIO_NET_F_CTRL_RX_EXTRA etc. header additions.
> 
> The savevm version_id 9 bump is a little gratuitous, but I don't think
> it's a big deal - I'd prefer to see cleanly separated patches like this,
> and it wouldn't have been easy to e.g. split out the savevm changes into
> a patch of their own at the end.
> 
> The vnet header version_id reservation is a good idea.

Thanks for the comments.  Do you mean the version 8 jump is gratuitous
(make the promisc/allmulti fields smaller)?  It is, but it was annoying
me, especially since I didn't want to waste even more space with these
new options.  Version 9 adds the overflow fields, which I think is
necessary or else we run into problems if a guest only reprograms the
MAC filter table w/o first setting those RX mode fields.  Then we don't
have the info to remember if the guest requested promisc or we enabled
it ourselves.

> I've given this stuff some light testing, but would you care to outline
> some instructions for some basic tests for as much as possible of this
> stuff using a Linux guest? i.e. broadcast, multicast, promisc, mac
> table, vlan table etc. - that's quite a set of tests you need to cover
> all those cases.

Sure, some of the new features can't be tested in Linux, so I'm relying
on some test reports from DMX to prove that it works.  I typically test
promiscuous and all multi using the ifconfig flags to enable them,
tcpdump is also a good test for promisc.  You'll need to add some
instrumentation to the filter function to know when packets are getting
dropped in qemu and the absence of dropped packets means they're getting
through.  For the MAC filter table, I use macvlans:

ip link add link eth0 eth$i type macvlan
dhclient eth$i

Add some instrumentation to the MAC table, watch it fill up.  Eventually
you'll run out of entries (it helps to reduce the table size for
testing), at which point we should set the multi overflow and continue
filling unicast (multicast MACs stop getting dropped).  Those will fill
up, and we'll enable uni overflow.  Actually getting traffic to each MAC
is a little more difficult.  You need to use arp -s to insert an ARP
cache entry for the macvlan to make sure the traffic isn't filtered.

VLANs I typically test by, again, watching what gets dropped.  For
instance, setup a remote system on a VLAN, ping the broadcast, see that
packets from that MAC are dropped.  Enable the VLAN on the guest using
vconfig and see that packets are no longer dropped, and there's
connectivity on the VLAN.

AFAIK, Linux doesn't have any obvious way to test alluni, nouni, nomulti
or the broadcast switches.  The logic for these is pretty simple though
and some of them track aspects of the MAC filter table overflow.
Thanks,

Alex

Re: [Qemu-devel] [PATCH 0/7] virtio-net: Filter cleanup/improvements

[Rusty Russell]

On Wed, 10 Jun 2009 04:55:10 am Mark McLoughlin wrote:
> Hi Alex,
>
> On Fri, 2009-06-05 at 14:46 -0600, Alex Williamson wrote:
> > This series cleans up a few things around packet filtering.  I've
> > probably gone a little overboard on breaking up patches, if we want to
> > avoid bumping the save version_id so much, these could be mostly lumped
> > together.  The main features here are more efficient handling of the
> > filtering between unicast and multicast, better overflow tracking, adding
> > more RX modes, and increasing the size of the filter table and control
> > queue depth.
>
> The whole series looks good to me. Please make sure to send Rusty a
> patch with the VIRTIO_NET_F_CTRL_RX_EXTRA etc. header additions.

Remain unconvinced that it's worth a feature bit.  It'd pretty obscure (hell, 
even multicast is pretty obscure IRL).

Thanks,
Rusty.

Re: [Qemu-devel] [PATCH 0/7] virtio-net: Filter cleanup/improvements

[Alex Williamson]

On Wed, 2009-06-10 at 16:21 +0930, Rusty Russell wrote:
> On Wed, 10 Jun 2009 04:55:10 am Mark McLoughlin wrote:
> > Hi Alex,
> >
> > On Fri, 2009-06-05 at 14:46 -0600, Alex Williamson wrote:
> > > This series cleans up a few things around packet filtering.  I've
> > > probably gone a little overboard on breaking up patches, if we want to
> > > avoid bumping the save version_id so much, these could be mostly lumped
> > > together.  The main features here are more efficient handling of the
> > > filtering between unicast and multicast, better overflow tracking, adding
> > > more RX modes, and increasing the size of the filter table and control
> > > queue depth.
> >
> > The whole series looks good to me. Please make sure to send Rusty a
> > patch with the VIRTIO_NET_F_CTRL_RX_EXTRA etc. header additions.
> 
> Remain unconvinced that it's worth a feature bit.  It'd pretty obscure (hell, 
> even multicast is pretty obscure IRL).

I wasn't sure about the feature bit, so I decided to error on the side
of caution and add it.  It seems to fit the existing model of knowing
what's available via features instead of probing.  If you're opposed to
it, that part can be dropped.  Thanks,

Alex

Re: [Qemu-devel] [PATCH 0/7] virtio-net: Filter cleanup/improvements

[Mark McLoughlin]

On Wed, 2009-06-10 at 16:21 +0930, Rusty Russell wrote:
> On Wed, 10 Jun 2009 04:55:10 am Mark McLoughlin wrote:
> > Hi Alex,
> >
> > On Fri, 2009-06-05 at 14:46 -0600, Alex Williamson wrote:
> > > This series cleans up a few things around packet filtering.  I've
> > > probably gone a little overboard on breaking up patches, if we want to
> > > avoid bumping the save version_id so much, these could be mostly lumped
> > > together.  The main features here are more efficient handling of the
> > > filtering between unicast and multicast, better overflow tracking, adding
> > > more RX modes, and increasing the size of the filter table and control
> > > queue depth.
> >
> > The whole series looks good to me. Please make sure to send Rusty a
> > patch with the VIRTIO_NET_F_CTRL_RX_EXTRA etc. header additions.
> 
> Remain unconvinced that it's worth a feature bit.  It'd pretty obscure (hell, 
> even multicast is pretty obscure IRL).

Yeah, we probably don't need the feature bit - the guest can just handle
a VIRTIO_NET_ERR status if the host doesn't implement it.

Alex - if you agree, could you post a qemu patch to kill the
CTRL_RX_EXTRA feature? And also post a linux/virtio_net.h patch to add
the new rx modes?

Thanks,
Mark.

Re: [Qemu-devel] [PATCH 0/7] virtio-net: Filter cleanup/improvements

[Alex Williamson]

On Fri, 2009-06-12 at 18:07 +0100, Mark McLoughlin wrote:
> On Wed, 2009-06-10 at 16:21 +0930, Rusty Russell wrote:
> > On Wed, 10 Jun 2009 04:55:10 am Mark McLoughlin wrote:
> > > Hi Alex,
> > >
> > > On Fri, 2009-06-05 at 14:46 -0600, Alex Williamson wrote:
> > > > This series cleans up a few things around packet filtering.  I've
> > > > probably gone a little overboard on breaking up patches, if we want to
> > > > avoid bumping the save version_id so much, these could be mostly lumped
> > > > together.  The main features here are more efficient handling of the
> > > > filtering between unicast and multicast, better overflow tracking, adding
> > > > more RX modes, and increasing the size of the filter table and control
> > > > queue depth.
> > >
> > > The whole series looks good to me. Please make sure to send Rusty a
> > > patch with the VIRTIO_NET_F_CTRL_RX_EXTRA etc. header additions.
> > 
> > Remain unconvinced that it's worth a feature bit.  It'd pretty obscure (hell, 
> > even multicast is pretty obscure IRL).
> 
> Yeah, we probably don't need the feature bit - the guest can just handle
> a VIRTIO_NET_ERR status if the host doesn't implement it.
> 
> Alex - if you agree, could you post a qemu patch to kill the
> CTRL_RX_EXTRA feature? And also post a linux/virtio_net.h patch to add
> the new rx modes?

Two votes against it, I'll drop the feature bit.  I'll post the qemu
patch in a minute and follow-up with the linux patch once that goes in.
Thanks,

Alex