[Qemu-devel] [RFC PATCH 0/4] Model specific function for nic announcement

[Qemu-devel] [RFC PATCH 0/4] Model specific function for nic announcement

[Jason Wang]

This series of patch tries to solve the problem of migration with nics who have
multiple mac addresses or vlans. Qemu currently only send gratuitous packet for
first mac address, so after migration other mac addresses or vlans were broken.

The information of mac addresses and vlans were often stored in model specific
structure, so a model specific function were introduced and used after
migration during self announcement. A sample announcing function for virtio-net
is also implemented in this series. Other model could be also done in this
way. The previous method is kept for the model who does not implement model
specific function.

While there's still issues which need your comments and need to be solved.

1 Virtio specification allows filtering any number of destination mac addresses
  which looks impossible for the migration. Then we need to record an unlimited
  numbers of mac address which is not safe.

2 Virtio specification allows filtering of vlan and mac address but neither the
spec nor the implementation could decide the mappings between vlans and mac
addresses. This could make it impossible to send correct tagged gratuitous
packet.

For issue 1, the number of mac addresses were limited in this series.
For issue 2, I suspect it needs the modification of guest drivers to send the
mappings in order to make migration work. Or is there any method to get this
without touching guest drivers? For safety, all the mac addresses were only
announced when there's no vlan in guest which means the vlan after migration is
still broken.

This patchset is just an RFC and need your suggestions and comments.

Thanks.

---

Jason Wang (4):
      net: move announce_self_create to net.c
      net: Introduce model specific nic announce function
      virtio-net: Limit the num of uni/multicast mac addresses
      virtio-net: implement virtio-net specific announce function


 hw/virtio-net.c |   47 ++++++++++++++++++++++++++++++++++++++++++-----
 net.c           |   31 +++++++++++++++++++++++++++++++
 net.h           |    3 +++
 savevm.c        |   42 ++++++++----------------------------------
 4 files changed, 84 insertions(+), 39 deletions(-)

-- 
Jason Wang

[Qemu-devel] [RFC PATCH 1/4] net: move announce_self_create to net.c

[Jason Wang]

Export and move announce_self_create to net.c in order to be used by model
specific announcing function.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 net.c    |   31 +++++++++++++++++++++++++++++++
 net.h    |    1 +
 savevm.c |   32 --------------------------------
 3 files changed, 32 insertions(+), 32 deletions(-)

diff --git a/net.c b/net.c
index 3d0fde7..5ec6912 100644
--- a/net.c
+++ b/net.c
@@ -42,6 +42,37 @@ static QTAILQ_HEAD(, VLANClientState) non_vlan_clients;
 
 int default_net = 1;
 
+#ifndef ETH_P_RARP
+#define ETH_P_RARP 0x8035
+#endif
+#define ARP_HTYPE_ETH 0x0001
+#define ARP_PTYPE_IP 0x0800
+#define ARP_OP_REQUEST_REV 0x3
+
+int announce_self_create(uint8_t *buf, uint8_t *mac_addr)
+{
+    /* Ethernet header. */
+    memset(buf, 0xff, 6);         /* destination MAC addr */
+    memcpy(buf + 6, mac_addr, 6); /* source MAC addr */
+    *(uint16_t *)(buf + 12) = htons(ETH_P_RARP); /* ethertype */
+
+    /* RARP header. */
+    *(uint16_t *)(buf + 14) = htons(ARP_HTYPE_ETH); /* hardware addr space */
+    *(uint16_t *)(buf + 16) = htons(ARP_PTYPE_IP); /* protocol addr space */
+    *(buf + 18) = 6; /* hardware addr length (ethernet) */
+    *(buf + 19) = 4; /* protocol addr length (IPv4) */
+    *(uint16_t *)(buf + 20) = htons(ARP_OP_REQUEST_REV); /* opcode */
+    memcpy(buf + 22, mac_addr, 6); /* source hw addr */
+    memset(buf + 28, 0x00, 4);     /* source protocol addr */
+    memcpy(buf + 32, mac_addr, 6); /* target hw addr */
+    memset(buf + 38, 0x00, 4);     /* target protocol addr */
+
+    /* Padding to get up to 60 bytes (ethernet min packet size, minus FCS). */
+    memset(buf + 42, 0x00, 18);
+
+    return 60; /* len (FCS will be added by hardware) */
+}
+
 /***********************************************************/
 /* network device redirectors */
 
diff --git a/net.h b/net.h
index 518cf9c..e3f643c 100644
--- a/net.h
+++ b/net.h
@@ -177,5 +177,6 @@ int do_netdev_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
 void qdev_set_nic_properties(DeviceState *dev, NICInfo *nd);
 
 int net_handle_fd_param(Monitor *mon, const char *param);
+int announce_self_create(uint8_t *buf, uint8_t *mac_addr);
 
 #endif
diff --git a/savevm.c b/savevm.c
index 6fa7a5f..545d511 100644
--- a/savevm.c
+++ b/savevm.c
@@ -86,38 +86,6 @@
 
 #define SELF_ANNOUNCE_ROUNDS 5
 
-#ifndef ETH_P_RARP
-#define ETH_P_RARP 0x8035
-#endif
-#define ARP_HTYPE_ETH 0x0001
-#define ARP_PTYPE_IP 0x0800
-#define ARP_OP_REQUEST_REV 0x3
-
-static int announce_self_create(uint8_t *buf,
-				uint8_t *mac_addr)
-{
-    /* Ethernet header. */
-    memset(buf, 0xff, 6);         /* destination MAC addr */
-    memcpy(buf + 6, mac_addr, 6); /* source MAC addr */
-    *(uint16_t *)(buf + 12) = htons(ETH_P_RARP); /* ethertype */
-
-    /* RARP header. */
-    *(uint16_t *)(buf + 14) = htons(ARP_HTYPE_ETH); /* hardware addr space */
-    *(uint16_t *)(buf + 16) = htons(ARP_PTYPE_IP); /* protocol addr space */
-    *(buf + 18) = 6; /* hardware addr length (ethernet) */
-    *(buf + 19) = 4; /* protocol addr length (IPv4) */
-    *(uint16_t *)(buf + 20) = htons(ARP_OP_REQUEST_REV); /* opcode */
-    memcpy(buf + 22, mac_addr, 6); /* source hw addr */
-    memset(buf + 28, 0x00, 4);     /* source protocol addr */
-    memcpy(buf + 32, mac_addr, 6); /* target hw addr */
-    memset(buf + 38, 0x00, 4);     /* target protocol addr */
-
-    /* Padding to get up to 60 bytes (ethernet min packet size, minus FCS). */
-    memset(buf + 42, 0x00, 18);
-
-    return 60; /* len (FCS will be added by hardware) */
-}
-
 static void qemu_announce_self_iter(NICState *nic, void *opaque)
 {
     uint8_t buf[60];

[Qemu-devel] [RFC PATCH 2/4] net: Introduce model specific nic announce function

[Jason Wang]

This patch introduce a function pointer in NetClientInfo which is called during
self announcement to do the model specific mac address announcement. Previous
method were kept for the model without its own implementation.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 net.h    |    2 ++
 savevm.c |   10 ++++++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/net.h b/net.h
index e3f643c..4fa5603 100644
--- a/net.h
+++ b/net.h
@@ -42,6 +42,7 @@ typedef ssize_t (NetReceive)(VLANClientState *, const uint8_t *, size_t);
 typedef ssize_t (NetReceiveIOV)(VLANClientState *, const struct iovec *, int);
 typedef void (NetCleanup) (VLANClientState *);
 typedef void (LinkStatusChanged)(VLANClientState *);
+typedef void (NetAnnounce)(VLANClientState *);
 
 typedef struct NetClientInfo {
     net_client_type type;
@@ -53,6 +54,7 @@ typedef struct NetClientInfo {
     NetCleanup *cleanup;
     LinkStatusChanged *link_status_changed;
     NetPoll *poll;
+    NetAnnounce *announce;
 } NetClientInfo;
 
 struct VLANClientState {
diff --git a/savevm.c b/savevm.c
index 545d511..c16ee1b 100644
--- a/savevm.c
+++ b/savevm.c
@@ -90,10 +90,16 @@ static void qemu_announce_self_iter(NICState *nic, void *opaque)
 {
     uint8_t buf[60];
     int len;
+    NetAnnounce *func = nic->nc.info->announce;
 
-    len = announce_self_create(buf, nic->conf->macaddr.a);
+    if (func == NULL) {
+        len = announce_self_create(buf, nic->conf->macaddr.a);
 
-    qemu_send_packet_raw(&nic->nc, buf, len);
+        qemu_send_packet_raw(&nic->nc, buf, len);
+    }
+    else {
+        func(&nic->nc);
+    }
 }
 
 

[Qemu-devel] [RFC PATCH 3/4] virtio-net: Limit the num of uni/multicast mac addresses

[Jason Wang]

In order to send gratuitous packet for each mac address, we need to keep
track every macaddress after migration which is obviously conflicted with the
overload policy currently used in qemu which just forward all packets to guest
when it can't record all the macaddress. So we need to limit the num of
uni/multicast mac addresses in qemu.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/virtio-net.c |   17 ++++++++++++-----
 1 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 0a9cae2..79afb65 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -291,7 +291,9 @@ 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->mac_table.uni_overflow = 1;
+        /* Gratuitous packet could not be built properly in overflow mode, so we
+         * never allow uni_overflow here. */
+        return VIRTIO_NET_ERR;
     }
 
     n->mac_table.first_multi = n->mac_table.in_use;
@@ -309,7 +311,9 @@ 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->mac_table.multi_overflow = 1;
+            /* Gratuitous packet could not be built properly in overflow mode,
+             * so we never allow multi_overflow here. */
+            return VIRTIO_NET_ERR;
         }
     }
 
@@ -844,9 +848,8 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
             qemu_get_buffer(f, n->mac_table.macs,
                             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->mac_table.multi_overflow = n->mac_table.uni_overflow = 1;
-            n->mac_table.in_use = 0;
+            error_report("virtio-net: Overflow was not permitted.");
+            return -EINVAL;
         }
     }
  
@@ -873,6 +876,10 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
     if (version_id >= 9) {
         n->mac_table.multi_overflow = qemu_get_byte(f);
         n->mac_table.uni_overflow = qemu_get_byte(f);
+        if (n->mac_table.multi_overflow || n->mac_table.uni_overflow) {
+            error_report("virtio-net: Overflow was not permitted");
+            return -EINVAL;
+        }
     }
 
     if (version_id >= 10) {

[Qemu-devel] [RFC PATCH 4/4] virtio-net: implement virtio-net specific announce function

[Jason Wang]

This patch tries to announce every mac address for one virtio nic after
migration. The primary mac address is announced unconditionally and other
macaddress are only announced when no vlan is configurated in guest.

For guest with vlan, what we need do is to send tagged gratutious packet, but
current virtio-net implementation can not distinguish the mapping between mac
addresses and vlan. And I believe this function must be implemented with the
modifications of guest drivers ( vlan_dev_info ).

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/virtio-net.c |   30 ++++++++++++++++++++++++++++++
 1 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 79afb65..0b47ac2 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -922,6 +922,35 @@ static void virtio_net_cleanup(VLANClientState *nc)
     n->nic = NULL;
 }
 
+static void virtio_net_announce(VLANClientState *nc)
+{
+    uint8_t buf[60];
+    bool has_vlan = false;
+    int len;
+    int i;
+    VirtIONet *n = DO_UPCAST(NICState, nc, nc)->opaque;
+
+    len = announce_self_create(buf, &n->mac[0]);
+    qemu_send_packet_raw(nc, buf, len);
+
+    for (i = 0; i < (MAX_VLAN >> 5); i++) {
+        if (n->vlans[i] & ~0u) {
+            has_vlan = true;
+            break;
+        }
+    }
+
+    if (!has_vlan) {
+	/* It's safe to announce the rest of mac addresses when there's no vlan
+	 * in guest */
+        for (i = 0; i < n->mac_table.in_use; i++) {
+            mac_debug(&n->mac_table.macs[i * ETH_ALEN]);
+            len = announce_self_create(buf, &n->mac_table.macs[i * ETH_ALEN]);
+            qemu_send_packet_raw(nc, buf, len);
+        }
+    }
+}
+
 static NetClientInfo net_virtio_info = {
     .type = NET_CLIENT_TYPE_NIC,
     .size = sizeof(NICState),
@@ -929,6 +958,7 @@ static NetClientInfo net_virtio_info = {
     .receive = virtio_net_receive,
         .cleanup = virtio_net_cleanup,
     .link_status_changed = virtio_net_set_link_status,
+    .announce = virtio_net_announce,
 };
 
 static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)

[Qemu-devel] Re: [RFC PATCH 0/4] Model specific function for nic announcement

[Michael S. Tsirkin]

On Mon, Sep 27, 2010 at 08:50:44PM +0800, Jason Wang wrote:
> This series of patch tries to solve the problem of migration with nics who have
> multiple mac addresses or vlans. Qemu currently only send gratuitous packet for
> first mac address, so after migration other mac addresses or vlans were broken.
> 
> The information of mac addresses and vlans were often stored in model specific
> structure, so a model specific function were introduced and used after
> migration during self announcement. A sample announcing function for virtio-net
> is also implemented in this series. Other model could be also done in this
> way. The previous method is kept for the model who does not implement model
> specific function.
> 
> While there's still issues which need your comments and need to be solved.
> 
> 1 Virtio specification allows filtering any number of destination mac addresses
>   which looks impossible for the migration. Then we need to record an unlimited
>   numbers of mac address which is not safe.
> 
> 2 Virtio specification allows filtering of vlan and mac address but neither the
> spec nor the implementation could decide the mappings between vlans and mac
> addresses. This could make it impossible to send correct tagged gratuitous
> packet.
> For issue 1, the number of mac addresses were limited in this series.
> For issue 2, I suspect it needs the modification of guest drivers to send the
> mappings in order to make migration work. Or is there any method to get this
> without touching guest drivers? For safety, all the mac addresses were only
> announced when there's no vlan in guest which means the vlan after migration is
> still broken.
> 
> This patchset is just an RFC and need your suggestions and comments.
> 
> Thanks.

For 1, all guests actually do is on error status is print
a warning. This does not sound all that useful, does it?
I note that we also have a similar issue with promisc mode.

For 2, we could at least make it work for a common case of 1 mac,
which would already cover many users.

> ---
> 
> Jason Wang (4):
>       net: move announce_self_create to net.c
>       net: Introduce model specific nic announce function
>       virtio-net: Limit the num of uni/multicast mac addresses
>       virtio-net: implement virtio-net specific announce function
> 
> 
>  hw/virtio-net.c |   47 ++++++++++++++++++++++++++++++++++++++++++-----
>  net.c           |   31 +++++++++++++++++++++++++++++++
>  net.h           |    3 +++
>  savevm.c        |   42 ++++++++----------------------------------
>  4 files changed, 84 insertions(+), 39 deletions(-)
> 
> -- 
> Jason Wang