[Qemu-devel] [PATCH 0/3] [PULL] slirp: ARP table improvements

[Qemu-devel] [PATCH 0/3] [PULL] slirp: ARP table improvements

[Jan Kiszka]

The following changes since commit 927d721777e73339f73719f36eaf400ab641366c:

  microblaze: Add missing call to qemu_init_vcpu. (2011-07-31 06:40:13 +0200)

are available in the git repository at:
  git://git.kiszka.org/qemu.git queues/slirp

Anthony asked me to look after slirp patches, and I agreed. So here
comes the first pull request. It improves the so far minimalistic ARP
support of slirp by avoiding premature packet drops when addressing not
yet resolved client IPs.


CC: Fabien Chouteau <chouteau@adacore.com>

Fabien Chouteau (2):
  Simple ARP table
  Delayed IP packets

Jan Kiszka (1):
  slirp: Take maintainer token

 MAINTAINERS       |    5 +-
 Makefile.objs     |    2 +-
 slirp/arp_table.c |   95 +++++++++++++++++++++++++++++++++++++
 slirp/bootp.c     |   21 +++++---
 slirp/if.c        |   28 +++++++++--
 slirp/main.h      |    2 +-
 slirp/mbuf.c      |    2 +
 slirp/mbuf.h      |    2 +
 slirp/slirp.c     |  135 ++++++++++++++++++++++-------------------------------
 slirp/slirp.h     |   47 +++++++++++++++++-
 10 files changed, 241 insertions(+), 98 deletions(-)
 create mode 100644 slirp/arp_table.c

-- 
1.7.3.4

[Qemu-devel] [PATCH 1/3] slirp: Take maintainer token

[Jan Kiszka]

Anthony asked me to pick up the maintenance of this subsystem, and I
agreed.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 MAINTAINERS |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 6115e4e..7cbcd7e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -431,9 +431,10 @@ S: Maintained
 F: net/
 
 SLIRP
-M: qemu-devel@nongnu.org
-S: Orphan
+M: Jan Kiszka <jan.kiszka@siemens.com>
+S: Maintained
 F: slirp/
+T: git://git.kiszka.org/qemu.git queues/slirp
 
 Usermode Emulation
 ------------------
-- 
1.7.3.4

[Qemu-devel] [PATCH 2/3] Simple ARP table

[Jan Kiszka]

From: Fabien Chouteau <chouteau@adacore.com>

This patch adds a simple ARP table in Slirp and also adds handling of
gratuitous ARP requests.

Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 Makefile.objs     |    2 +-
 slirp/arp_table.c |   95 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 slirp/bootp.c     |   21 +++++++----
 slirp/slirp.c     |   63 +++++++++--------------------------
 slirp/slirp.h     |   47 ++++++++++++++++++++++++--
 5 files changed, 169 insertions(+), 59 deletions(-)
 create mode 100644 slirp/arp_table.c

diff --git a/Makefile.objs b/Makefile.objs
index 6991a9f..0c10557 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -151,7 +151,7 @@ common-obj-y += qemu-timer.o qemu-timer-common.o
 
 slirp-obj-y = cksum.o if.o ip_icmp.o ip_input.o ip_output.o
 slirp-obj-y += slirp.o mbuf.o misc.o sbuf.o socket.o tcp_input.o tcp_output.o
-slirp-obj-y += tcp_subr.o tcp_timer.o udp.o bootp.o tftp.o
+slirp-obj-y += tcp_subr.o tcp_timer.o udp.o bootp.o tftp.o arp_table.o
 common-obj-$(CONFIG_SLIRP) += $(addprefix slirp/, $(slirp-obj-y))
 
 # xen backend driver support
diff --git a/slirp/arp_table.c b/slirp/arp_table.c
new file mode 100644
index 0000000..820dee2
--- /dev/null
+++ b/slirp/arp_table.c
@@ -0,0 +1,95 @@
+/*
+ * ARP table
+ *
+ * Copyright (c) 2011 AdaCore
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "slirp.h"
+
+void arp_table_add(Slirp *slirp, int ip_addr, uint8_t ethaddr[ETH_ALEN])
+{
+    const in_addr_t broadcast_addr =
+        ~slirp->vnetwork_mask.s_addr | slirp->vnetwork_addr.s_addr;
+    ArpTable *arptbl = &slirp->arp_table;
+    int i;
+
+    DEBUG_CALL("arp_table_add");
+    DEBUG_ARG("ip = 0x%x", ip_addr);
+    DEBUG_ARGS((dfd, " hw addr = %02x:%02x:%02x:%02x:%02x:%02x\n",
+                ethaddr[0], ethaddr[1], ethaddr[2],
+                ethaddr[3], ethaddr[4], ethaddr[5]));
+
+    /* Check 0.0.0.0/8 invalid source-only addresses */
+    assert((ip_addr & htonl(~(0xf << 28))) != 0);
+
+    if (ip_addr == 0xffffffff || ip_addr == broadcast_addr) {
+        /* Do not register broadcast addresses */
+        return;
+    }
+
+    /* Search for an entry */
+    for (i = 0; i < ARP_TABLE_SIZE; i++) {
+        if (arptbl->table[i].ar_sip == ip_addr) {
+            /* Update the entry */
+            memcpy(arptbl->table[i].ar_sha, ethaddr, ETH_ALEN);
+            return;
+        }
+    }
+
+    /* No entry found, create a new one */
+    arptbl->table[arptbl->next_victim].ar_sip = ip_addr;
+    memcpy(arptbl->table[arptbl->next_victim].ar_sha,  ethaddr, ETH_ALEN);
+    arptbl->next_victim = (arptbl->next_victim + 1) % ARP_TABLE_SIZE;
+}
+
+bool arp_table_search(Slirp *slirp, int in_ip_addr,
+                      uint8_t out_ethaddr[ETH_ALEN])
+{
+    const in_addr_t broadcast_addr =
+        ~slirp->vnetwork_mask.s_addr | slirp->vnetwork_addr.s_addr;
+    ArpTable *arptbl = &slirp->arp_table;
+    int i;
+
+    DEBUG_CALL("arp_table_search");
+    DEBUG_ARG("ip = 0x%x", in_ip_addr);
+
+    /* Check 0.0.0.0/8 invalid source-only addresses */
+    assert((in_ip_addr & htonl(~(0xf << 28))) != 0);
+
+    /* If broadcast address */
+    if (in_ip_addr == 0xffffffff || in_ip_addr == broadcast_addr) {
+        /* return Ethernet broadcast address */
+        memset(out_ethaddr, 0xff, ETH_ALEN);
+        return 1;
+    }
+
+    for (i = 0; i < ARP_TABLE_SIZE; i++) {
+        if (arptbl->table[i].ar_sip == in_ip_addr) {
+            memcpy(out_ethaddr, arptbl->table[i].ar_sha,  ETH_ALEN);
+            DEBUG_ARGS((dfd, " found hw addr = %02x:%02x:%02x:%02x:%02x:%02x\n",
+                        out_ethaddr[0], out_ethaddr[1], out_ethaddr[2],
+                        out_ethaddr[3], out_ethaddr[4], out_ethaddr[5]));
+            return 1;
+        }
+    }
+
+    return 0;
+}
diff --git a/slirp/bootp.c b/slirp/bootp.c
index 1eb2ed1..efd1fe7 100644
--- a/slirp/bootp.c
+++ b/slirp/bootp.c
@@ -149,6 +149,7 @@ static void bootp_reply(Slirp *slirp, const struct bootp_t *bp)
     struct in_addr preq_addr;
     int dhcp_msg_type, val;
     uint8_t *q;
+    uint8_t client_ethaddr[ETH_ALEN];
 
     /* extract exact DHCP msg type */
     dhcp_decode(bp, &dhcp_msg_type, &preq_addr);
@@ -164,8 +165,9 @@ static void bootp_reply(Slirp *slirp, const struct bootp_t *bp)
     if (dhcp_msg_type != DHCPDISCOVER &&
         dhcp_msg_type != DHCPREQUEST)
         return;
-    /* XXX: this is a hack to get the client mac address */
-    memcpy(slirp->client_ethaddr, bp->bp_hwaddr, 6);
+
+    /* Get client's hardware address from bootp request */
+    memcpy(client_ethaddr, bp->bp_hwaddr, ETH_ALEN);
 
     m = m_get(slirp);
     if (!m) {
@@ -178,25 +180,25 @@ static void bootp_reply(Slirp *slirp, const struct bootp_t *bp)
 
     if (dhcp_msg_type == DHCPDISCOVER) {
         if (preq_addr.s_addr != htonl(0L)) {
-            bc = request_addr(slirp, &preq_addr, slirp->client_ethaddr);
+            bc = request_addr(slirp, &preq_addr, client_ethaddr);
             if (bc) {
                 daddr.sin_addr = preq_addr;
             }
         }
         if (!bc) {
          new_addr:
-            bc = get_new_addr(slirp, &daddr.sin_addr, slirp->client_ethaddr);
+            bc = get_new_addr(slirp, &daddr.sin_addr, client_ethaddr);
             if (!bc) {
                 DPRINTF("no address left\n");
                 return;
             }
         }
-        memcpy(bc->macaddr, slirp->client_ethaddr, 6);
+        memcpy(bc->macaddr, client_ethaddr, ETH_ALEN);
     } else if (preq_addr.s_addr != htonl(0L)) {
-        bc = request_addr(slirp, &preq_addr, slirp->client_ethaddr);
+        bc = request_addr(slirp, &preq_addr, client_ethaddr);
         if (bc) {
             daddr.sin_addr = preq_addr;
-            memcpy(bc->macaddr, slirp->client_ethaddr, 6);
+            memcpy(bc->macaddr, client_ethaddr, ETH_ALEN);
         } else {
             daddr.sin_addr.s_addr = 0;
         }
@@ -209,6 +211,9 @@ static void bootp_reply(Slirp *slirp, const struct bootp_t *bp)
         }
     }
 
+    /* Update ARP table for this IP address */
+    arp_table_add(slirp, daddr.sin_addr.s_addr, client_ethaddr);
+
     saddr.sin_addr = slirp->vhost_addr;
     saddr.sin_port = htons(BOOTP_SERVER);
 
@@ -218,7 +223,7 @@ static void bootp_reply(Slirp *slirp, const struct bootp_t *bp)
     rbp->bp_xid = bp->bp_xid;
     rbp->bp_htype = 1;
     rbp->bp_hlen = 6;
-    memcpy(rbp->bp_hwaddr, bp->bp_hwaddr, 6);
+    memcpy(rbp->bp_hwaddr, bp->bp_hwaddr, ETH_ALEN);
 
     rbp->bp_yiaddr = daddr.sin_addr; /* Client IP address */
     rbp->bp_siaddr = saddr.sin_addr; /* Server IP address */
diff --git a/slirp/slirp.c b/slirp/slirp.c
index df787ea..4a9a4d5 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -31,11 +31,11 @@
 struct in_addr loopback_addr;
 
 /* emulated hosts use the MAC addr 52:55:IP:IP:IP:IP */
-static const uint8_t special_ethaddr[6] = {
+static const uint8_t special_ethaddr[ETH_ALEN] = {
     0x52, 0x55, 0x00, 0x00, 0x00, 0x00
 };
 
-static const uint8_t zero_ethaddr[6] = { 0, 0, 0, 0, 0, 0 };
+static const uint8_t zero_ethaddr[ETH_ALEN] = { 0, 0, 0, 0, 0, 0 };
 
 /* XXX: suppress those select globals */
 fd_set *global_readfds, *global_writefds, *global_xfds;
@@ -599,42 +599,8 @@ void slirp_select_poll(fd_set *readfds, fd_set *writefds, fd_set *xfds,
 	 global_xfds = NULL;
 }
 
-#define ETH_ALEN 6
-#define ETH_HLEN 14
-
-#define ETH_P_IP	0x0800		/* Internet Protocol packet	*/
-#define ETH_P_ARP	0x0806		/* Address Resolution packet	*/
-
-#define	ARPOP_REQUEST	1		/* ARP request			*/
-#define	ARPOP_REPLY	2		/* ARP reply			*/
-
-struct ethhdr
-{
-	unsigned char	h_dest[ETH_ALEN];	/* destination eth addr	*/
-	unsigned char	h_source[ETH_ALEN];	/* source ether addr	*/
-	unsigned short	h_proto;		/* packet type ID field	*/
-};
-
-struct arphdr
-{
-	unsigned short	ar_hrd;		/* format of hardware address	*/
-	unsigned short	ar_pro;		/* format of protocol address	*/
-	unsigned char	ar_hln;		/* length of hardware address	*/
-	unsigned char	ar_pln;		/* length of protocol address	*/
-	unsigned short	ar_op;		/* ARP opcode (command)		*/
-
-	 /*
-	  *	 Ethernet looks like this : This bit is variable sized however...
-	  */
-	unsigned char		ar_sha[ETH_ALEN];	/* sender hardware address	*/
-	uint32_t		ar_sip;			/* sender IP address		*/
-	unsigned char		ar_tha[ETH_ALEN];	/* target hardware address	*/
-	uint32_t		ar_tip	;		/* target IP address		*/
-} __attribute__((packed));
-
 static void arp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
 {
-    struct ethhdr *eh = (struct ethhdr *)pkt;
     struct arphdr *ah = (struct arphdr *)(pkt + ETH_HLEN);
     uint8_t arp_reply[max(ETH_HLEN + sizeof(struct arphdr), 64)];
     struct ethhdr *reh = (struct ethhdr *)arp_reply;
@@ -645,6 +611,12 @@ static void arp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
     ar_op = ntohs(ah->ar_op);
     switch(ar_op) {
     case ARPOP_REQUEST:
+        if (ah->ar_tip == ah->ar_sip) {
+            /* Gratuitous ARP */
+            arp_table_add(slirp, ah->ar_sip, ah->ar_sha);
+            return;
+        }
+
         if ((ah->ar_tip & slirp->vnetwork_mask.s_addr) ==
             slirp->vnetwork_addr.s_addr) {
             if (ah->ar_tip == slirp->vnameserver_addr.s_addr ||
@@ -657,8 +629,8 @@ static void arp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
             return;
         arp_ok:
             memset(arp_reply, 0, sizeof(arp_reply));
-            /* XXX: make an ARP request to have the client address */
-            memcpy(slirp->client_ethaddr, eh->h_source, ETH_ALEN);
+
+            arp_table_add(slirp, ah->ar_sip, ah->ar_sha);
 
             /* ARP request for alias/dns mac address */
             memcpy(reh->h_dest, pkt + ETH_ALEN, ETH_ALEN);
@@ -679,11 +651,7 @@ static void arp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
         }
         break;
     case ARPOP_REPLY:
-        /* reply to request of client mac address ? */
-        if (!memcmp(slirp->client_ethaddr, zero_ethaddr, ETH_ALEN) &&
-            ah->ar_sip == slirp->client_ipaddr.s_addr) {
-            memcpy(slirp->client_ethaddr, ah->ar_sha, ETH_ALEN);
-        }
+        arp_table_add(slirp, ah->ar_sip, ah->ar_sha);
         break;
     default:
         break;
@@ -729,15 +697,16 @@ void if_encap(Slirp *slirp, const uint8_t *ip_data, int ip_data_len)
 {
     uint8_t buf[1600];
     struct ethhdr *eh = (struct ethhdr *)buf;
+    uint8_t ethaddr[ETH_ALEN];
+    const struct ip *iph = (const struct ip *)ip_data;
 
     if (ip_data_len + ETH_HLEN > sizeof(buf))
         return;
-    
-    if (!memcmp(slirp->client_ethaddr, zero_ethaddr, ETH_ALEN)) {
+
+    if (!arp_table_search(slirp, iph->ip_dst.s_addr, ethaddr)) {
         uint8_t arp_req[ETH_HLEN + sizeof(struct arphdr)];
         struct ethhdr *reh = (struct ethhdr *)arp_req;
         struct arphdr *rah = (struct arphdr *)(arp_req + ETH_HLEN);
-        const struct ip *iph = (const struct ip *)ip_data;
 
         /* If the client addr is not known, there is no point in
            sending the packet to it. Normally the sender should have
@@ -765,7 +734,7 @@ void if_encap(Slirp *slirp, const uint8_t *ip_data, int ip_data_len)
         slirp->client_ipaddr = iph->ip_dst;
         slirp_output(slirp->opaque, arp_req, sizeof(arp_req));
     } else {
-        memcpy(eh->h_dest, slirp->client_ethaddr, ETH_ALEN);
+        memcpy(eh->h_dest, ethaddr, ETH_ALEN);
         memcpy(eh->h_source, special_ethaddr, ETH_ALEN - 4);
         /* XXX: not correct */
         memcpy(&eh->h_source[2], &slirp->vhost_addr, 4);
diff --git a/slirp/slirp.h b/slirp/slirp.h
index 16bb6ba..2a070e6 100644
--- a/slirp/slirp.h
+++ b/slirp/slirp.h
@@ -170,6 +170,48 @@ int inet_aton(const char *cp, struct in_addr *ia);
 /* osdep.c */
 int qemu_socket(int domain, int type, int protocol);
 
+#define ETH_ALEN 6
+#define ETH_HLEN 14
+
+#define ETH_P_IP  0x0800        /* Internet Protocol packet  */
+#define ETH_P_ARP 0x0806        /* Address Resolution packet */
+
+#define ARPOP_REQUEST 1         /* ARP request */
+#define ARPOP_REPLY   2         /* ARP reply   */
+
+struct ethhdr {
+    unsigned char  h_dest[ETH_ALEN];   /* destination eth addr */
+    unsigned char  h_source[ETH_ALEN]; /* source ether addr    */
+    unsigned short h_proto;            /* packet type ID field */
+};
+
+struct arphdr {
+    unsigned short ar_hrd;      /* format of hardware address */
+    unsigned short ar_pro;      /* format of protocol address */
+    unsigned char  ar_hln;      /* length of hardware address */
+    unsigned char  ar_pln;      /* length of protocol address */
+    unsigned short ar_op;       /* ARP opcode (command)       */
+
+    /*
+     *  Ethernet looks like this : This bit is variable sized however...
+     */
+    unsigned char ar_sha[ETH_ALEN]; /* sender hardware address */
+    uint32_t      ar_sip;           /* sender IP address       */
+    unsigned char ar_tha[ETH_ALEN]; /* target hardware address */
+    uint32_t      ar_tip;           /* target IP address       */
+} __attribute__((packed));
+
+#define ARP_TABLE_SIZE 16
+
+typedef struct ArpTable {
+    struct arphdr table[ARP_TABLE_SIZE];
+    int next_victim;
+} ArpTable;
+
+void arp_table_add(Slirp *slirp, int ip_addr, uint8_t ethaddr[ETH_ALEN]);
+
+bool arp_table_search(Slirp *slirp, int in_ip_addr,
+                      uint8_t out_ethaddr[ETH_ALEN]);
 
 struct Slirp {
     QTAILQ_ENTRY(Slirp) entry;
@@ -181,9 +223,6 @@ struct Slirp {
     struct in_addr vdhcp_startaddr;
     struct in_addr vnameserver_addr;
 
-    /* ARP cache for the guest IP addresses (XXX: allow many entries) */
-    uint8_t client_ethaddr[6];
-
     struct in_addr client_ipaddr;
     char client_hostname[33];
 
@@ -227,6 +266,8 @@ struct Slirp {
     char *tftp_prefix;
     struct tftp_session tftp_sessions[TFTP_SESSIONS_MAX];
 
+    ArpTable arp_table;
+
     void *opaque;
 };
 
-- 
1.7.3.4

[Qemu-devel] [PATCH 3/3] Delayed IP packets

[Jan Kiszka]

From: Fabien Chouteau <chouteau@adacore.com>

In the current implementation, if Slirp tries to send an IP packet to a client
with an unknown hardware address, the packet is simply dropped and an ARP
request is sent (if_encap in slirp/slirp.c).

With this patch, Slirp will send the ARP request, re-queue the packet and try
to send it later. The packet is dropped after one second if the ARP reply is
not received.

Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 slirp/if.c    |   28 +++++++++++++++++++---
 slirp/main.h  |    2 +-
 slirp/mbuf.c  |    2 +
 slirp/mbuf.h  |    2 +
 slirp/slirp.c |   72 +++++++++++++++++++++++++++++++-------------------------
 5 files changed, 69 insertions(+), 37 deletions(-)

diff --git a/slirp/if.c b/slirp/if.c
index 0f04e13..2d79e45 100644
--- a/slirp/if.c
+++ b/slirp/if.c
@@ -6,6 +6,7 @@
  */
 
 #include <slirp.h>
+#include "qemu-timer.h"
 
 #define ifs_init(ifm) ((ifm)->ifs_next = (ifm)->ifs_prev = (ifm))
 
@@ -105,6 +106,9 @@ if_output(struct socket *so, struct mbuf *ifm)
 	ifs_init(ifm);
 	insque(ifm, ifq);
 
+        /* Expiration date = Now + 1 second */
+        ifm->expiration_date = qemu_get_clock_ns(rt_clock) + 1000000000ULL;
+
 diddit:
 	slirp->if_queued++;
 
@@ -153,6 +157,9 @@ diddit:
 void
 if_start(Slirp *slirp)
 {
+    int requeued = 0;
+    uint64_t now;
+
 	struct mbuf *ifm, *ifqt;
 
 	DEBUG_CALL("if_start");
@@ -165,6 +172,8 @@ if_start(Slirp *slirp)
         if (!slirp_can_output(slirp->opaque))
             return;
 
+        now = qemu_get_clock_ns(rt_clock);
+
 	/*
 	 * See which queue to get next packet from
 	 * If there's something in the fastq, select it immediately
@@ -199,11 +208,22 @@ if_start(Slirp *slirp)
 		   ifm->ifq_so->so_nqueued = 0;
 	}
 
-	/* Encapsulate the packet for sending */
-        if_encap(slirp, (uint8_t *)ifm->m_data, ifm->m_len);
-
-        m_free(ifm);
+        if (ifm->expiration_date < now) {
+            /* Expired */
+            m_free(ifm);
+        } else {
+            /* Encapsulate the packet for sending */
+            if (if_encap(slirp, ifm)) {
+                m_free(ifm);
+            } else {
+                /* re-queue */
+                insque(ifm, ifqt);
+                requeued++;
+            }
+        }
 
 	if (slirp->if_queued)
 	   goto again;
+
+        slirp->if_queued = requeued;
 }
diff --git a/slirp/main.h b/slirp/main.h
index 0dd8d81..028df4b 100644
--- a/slirp/main.h
+++ b/slirp/main.h
@@ -42,5 +42,5 @@ extern int tcp_keepintvl;
 #define PROTO_PPP 0x2
 #endif
 
-void if_encap(Slirp *slirp, const uint8_t *ip_data, int ip_data_len);
+int if_encap(Slirp *slirp, struct mbuf *ifm);
 ssize_t slirp_send(struct socket *so, const void *buf, size_t len, int flags);
diff --git a/slirp/mbuf.c b/slirp/mbuf.c
index ce2eb84..c699c75 100644
--- a/slirp/mbuf.c
+++ b/slirp/mbuf.c
@@ -70,6 +70,8 @@ m_get(Slirp *slirp)
 	m->m_len = 0;
         m->m_nextpkt = NULL;
         m->m_prevpkt = NULL;
+        m->arp_requested = false;
+        m->expiration_date = (uint64_t)-1;
 end_error:
 	DEBUG_ARG("m = %lx", (long )m);
 	return m;
diff --git a/slirp/mbuf.h b/slirp/mbuf.h
index b74544b..55170e5 100644
--- a/slirp/mbuf.h
+++ b/slirp/mbuf.h
@@ -86,6 +86,8 @@ struct mbuf {
 		char	m_dat_[1]; /* ANSI don't like 0 sized arrays */
 		char	*m_ext_;
 	} M_dat;
+    bool     arp_requested;
+    uint64_t expiration_date;
 };
 
 #define m_next		m_hdr.mh_next
diff --git a/slirp/slirp.c b/slirp/slirp.c
index 4a9a4d5..a86cc6e 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -692,55 +692,63 @@ void slirp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
     }
 }
 
-/* output the IP packet to the ethernet device */
-void if_encap(Slirp *slirp, const uint8_t *ip_data, int ip_data_len)
+/* Output the IP packet to the ethernet device. Returns 0 if the packet must be
+ * re-queued.
+ */
+int if_encap(Slirp *slirp, struct mbuf *ifm)
 {
     uint8_t buf[1600];
     struct ethhdr *eh = (struct ethhdr *)buf;
     uint8_t ethaddr[ETH_ALEN];
-    const struct ip *iph = (const struct ip *)ip_data;
+    const struct ip *iph = (const struct ip *)ifm->m_data;
 
-    if (ip_data_len + ETH_HLEN > sizeof(buf))
-        return;
+    if (ifm->m_len + ETH_HLEN > sizeof(buf)) {
+        return 1;
+    }
 
     if (!arp_table_search(slirp, iph->ip_dst.s_addr, ethaddr)) {
         uint8_t arp_req[ETH_HLEN + sizeof(struct arphdr)];
         struct ethhdr *reh = (struct ethhdr *)arp_req;
         struct arphdr *rah = (struct arphdr *)(arp_req + ETH_HLEN);
 
-        /* If the client addr is not known, there is no point in
-           sending the packet to it. Normally the sender should have
-           done an ARP request to get its MAC address. Here we do it
-           in place of sending the packet and we hope that the sender
-           will retry sending its packet. */
-        memset(reh->h_dest, 0xff, ETH_ALEN);
-        memcpy(reh->h_source, special_ethaddr, ETH_ALEN - 4);
-        memcpy(&reh->h_source[2], &slirp->vhost_addr, 4);
-        reh->h_proto = htons(ETH_P_ARP);
-        rah->ar_hrd = htons(1);
-        rah->ar_pro = htons(ETH_P_IP);
-        rah->ar_hln = ETH_ALEN;
-        rah->ar_pln = 4;
-        rah->ar_op = htons(ARPOP_REQUEST);
-        /* source hw addr */
-        memcpy(rah->ar_sha, special_ethaddr, ETH_ALEN - 4);
-        memcpy(&rah->ar_sha[2], &slirp->vhost_addr, 4);
-        /* source IP */
-        rah->ar_sip = slirp->vhost_addr.s_addr;
-        /* target hw addr (none) */
-        memset(rah->ar_tha, 0, ETH_ALEN);
-        /* target IP */
-        rah->ar_tip = iph->ip_dst.s_addr;
-        slirp->client_ipaddr = iph->ip_dst;
-        slirp_output(slirp->opaque, arp_req, sizeof(arp_req));
+        if (!ifm->arp_requested) {
+            /* If the client addr is not known, send an ARP request */
+            memset(reh->h_dest, 0xff, ETH_ALEN);
+            memcpy(reh->h_source, special_ethaddr, ETH_ALEN - 4);
+            memcpy(&reh->h_source[2], &slirp->vhost_addr, 4);
+            reh->h_proto = htons(ETH_P_ARP);
+            rah->ar_hrd = htons(1);
+            rah->ar_pro = htons(ETH_P_IP);
+            rah->ar_hln = ETH_ALEN;
+            rah->ar_pln = 4;
+            rah->ar_op = htons(ARPOP_REQUEST);
+
+            /* source hw addr */
+            memcpy(rah->ar_sha, special_ethaddr, ETH_ALEN - 4);
+            memcpy(&rah->ar_sha[2], &slirp->vhost_addr, 4);
+
+            /* source IP */
+            rah->ar_sip = slirp->vhost_addr.s_addr;
+
+            /* target hw addr (none) */
+            memset(rah->ar_tha, 0, ETH_ALEN);
+
+            /* target IP */
+            rah->ar_tip = iph->ip_dst.s_addr;
+            slirp->client_ipaddr = iph->ip_dst;
+            slirp_output(slirp->opaque, arp_req, sizeof(arp_req));
+            ifm->arp_requested = true;
+        }
+        return 0;
     } else {
         memcpy(eh->h_dest, ethaddr, ETH_ALEN);
         memcpy(eh->h_source, special_ethaddr, ETH_ALEN - 4);
         /* XXX: not correct */
         memcpy(&eh->h_source[2], &slirp->vhost_addr, 4);
         eh->h_proto = htons(ETH_P_IP);
-        memcpy(buf + sizeof(struct ethhdr), ip_data, ip_data_len);
-        slirp_output(slirp->opaque, buf, ip_data_len + ETH_HLEN);
+        memcpy(buf + sizeof(struct ethhdr), ifm->m_data, ifm->m_len);
+        slirp_output(slirp->opaque, buf, ifm->m_len + ETH_HLEN);
+        return 1;
     }
 }
 
-- 
1.7.3.4

Re: [Qemu-devel] [PATCH 3/3] Delayed IP packets

[Amit Shah]

On (Wed) 03 Aug 2011 [13:24:22], Jan Kiszka wrote:
> From: Fabien Chouteau <chouteau@adacore.com>
> 
> In the current implementation, if Slirp tries to send an IP packet to a client
> with an unknown hardware address, the packet is simply dropped and an ARP
> request is sent (if_encap in slirp/slirp.c).
> 
> With this patch, Slirp will send the ARP request, re-queue the packet and try
> to send it later. The packet is dropped after one second if the ARP reply is
> not received.

This patch causes a segfault when guests wake up from hibernate.

Recipe:
1. Start guest with -net user -net nic,model=virtio
2. (guest) ping 10.0.2.2
3. (guest) echo "disk" > /sys/power/state
4. Re-start guest with same command line
5. Ping has stopped receiving replies.
6. Kill that ping process and start a new one.  qemu segfaults.

This needs the not-upstream-yet virtio S4 handling patches, found at
http://thread.gmane.org/gmane.linux.kernel/1197141

The backtrace is:

(gdb) bt
#0  0x00007ffff7e421f7 in slirp_insque (a=0x0, b=0x7ffff8f95d50) at
/home/amit/src/qemu/slirp/misc.c:27
#1  0x00007ffff7e40738 in if_start (slirp=0x7ffff8a9cdf0) at
/home/amit/src/qemu/slirp/if.c:194
#2  0x00007ffff7e44828 in slirp_select_poll (readfds=0x7fffffffd930,
writefds=0x7fffffffd9b0, xfds=0x7fffffffda30, select_error=0)
    at /home/amit/src/qemu/slirp/slirp.c:588
#3  0x00007ffff7e110f1 in main_loop_wait (nonblocking=<optimized out>)
at /home/amit/src/qemu/vl.c:1549
#4  0x00007ffff7d7dc47 in main_loop () at
/home/amit/src/qemu/vl.c:1579
#5  main (argc=<optimized out>, argv=<optimized out>, envp=<optimized
out>) at /home/amit/src/qemu/vl.c:3574


Reverting the patch keeps the ping going on after resume.  

Leaving the patch in context:

> 
> Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  slirp/if.c    |   28 +++++++++++++++++++---
>  slirp/main.h  |    2 +-
>  slirp/mbuf.c  |    2 +
>  slirp/mbuf.h  |    2 +
>  slirp/slirp.c |   72 +++++++++++++++++++++++++++++++-------------------------
>  5 files changed, 69 insertions(+), 37 deletions(-)
> 
> diff --git a/slirp/if.c b/slirp/if.c
> index 0f04e13..2d79e45 100644
> --- a/slirp/if.c
> +++ b/slirp/if.c
> @@ -6,6 +6,7 @@
>   */
>  
>  #include <slirp.h>
> +#include "qemu-timer.h"
>  
>  #define ifs_init(ifm) ((ifm)->ifs_next = (ifm)->ifs_prev = (ifm))
>  
> @@ -105,6 +106,9 @@ if_output(struct socket *so, struct mbuf *ifm)
>  	ifs_init(ifm);
>  	insque(ifm, ifq);
>  
> +        /* Expiration date = Now + 1 second */
> +        ifm->expiration_date = qemu_get_clock_ns(rt_clock) + 1000000000ULL;
> +
>  diddit:
>  	slirp->if_queued++;
>  
> @@ -153,6 +157,9 @@ diddit:
>  void
>  if_start(Slirp *slirp)
>  {
> +    int requeued = 0;
> +    uint64_t now;
> +
>  	struct mbuf *ifm, *ifqt;
>  
>  	DEBUG_CALL("if_start");
> @@ -165,6 +172,8 @@ if_start(Slirp *slirp)
>          if (!slirp_can_output(slirp->opaque))
>              return;
>  
> +        now = qemu_get_clock_ns(rt_clock);
> +
>  	/*
>  	 * See which queue to get next packet from
>  	 * If there's something in the fastq, select it immediately
> @@ -199,11 +208,22 @@ if_start(Slirp *slirp)
>  		   ifm->ifq_so->so_nqueued = 0;
>  	}
>  
> -	/* Encapsulate the packet for sending */
> -        if_encap(slirp, (uint8_t *)ifm->m_data, ifm->m_len);
> -
> -        m_free(ifm);
> +        if (ifm->expiration_date < now) {
> +            /* Expired */
> +            m_free(ifm);
> +        } else {
> +            /* Encapsulate the packet for sending */
> +            if (if_encap(slirp, ifm)) {
> +                m_free(ifm);
> +            } else {
> +                /* re-queue */
> +                insque(ifm, ifqt);
> +                requeued++;
> +            }
> +        }
>  
>  	if (slirp->if_queued)
>  	   goto again;
> +
> +        slirp->if_queued = requeued;
>  }
> diff --git a/slirp/main.h b/slirp/main.h
> index 0dd8d81..028df4b 100644
> --- a/slirp/main.h
> +++ b/slirp/main.h
> @@ -42,5 +42,5 @@ extern int tcp_keepintvl;
>  #define PROTO_PPP 0x2
>  #endif
>  
> -void if_encap(Slirp *slirp, const uint8_t *ip_data, int ip_data_len);
> +int if_encap(Slirp *slirp, struct mbuf *ifm);
>  ssize_t slirp_send(struct socket *so, const void *buf, size_t len, int flags);
> diff --git a/slirp/mbuf.c b/slirp/mbuf.c
> index ce2eb84..c699c75 100644
> --- a/slirp/mbuf.c
> +++ b/slirp/mbuf.c
> @@ -70,6 +70,8 @@ m_get(Slirp *slirp)
>  	m->m_len = 0;
>          m->m_nextpkt = NULL;
>          m->m_prevpkt = NULL;
> +        m->arp_requested = false;
> +        m->expiration_date = (uint64_t)-1;
>  end_error:
>  	DEBUG_ARG("m = %lx", (long )m);
>  	return m;
> diff --git a/slirp/mbuf.h b/slirp/mbuf.h
> index b74544b..55170e5 100644
> --- a/slirp/mbuf.h
> +++ b/slirp/mbuf.h
> @@ -86,6 +86,8 @@ struct mbuf {
>  		char	m_dat_[1]; /* ANSI don't like 0 sized arrays */
>  		char	*m_ext_;
>  	} M_dat;
> +    bool     arp_requested;
> +    uint64_t expiration_date;
>  };
>  
>  #define m_next		m_hdr.mh_next
> diff --git a/slirp/slirp.c b/slirp/slirp.c
> index 4a9a4d5..a86cc6e 100644
> --- a/slirp/slirp.c
> +++ b/slirp/slirp.c
> @@ -692,55 +692,63 @@ void slirp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
>      }
>  }
>  
> -/* output the IP packet to the ethernet device */
> -void if_encap(Slirp *slirp, const uint8_t *ip_data, int ip_data_len)
> +/* Output the IP packet to the ethernet device. Returns 0 if the packet must be
> + * re-queued.
> + */
> +int if_encap(Slirp *slirp, struct mbuf *ifm)
>  {
>      uint8_t buf[1600];
>      struct ethhdr *eh = (struct ethhdr *)buf;
>      uint8_t ethaddr[ETH_ALEN];
> -    const struct ip *iph = (const struct ip *)ip_data;
> +    const struct ip *iph = (const struct ip *)ifm->m_data;
>  
> -    if (ip_data_len + ETH_HLEN > sizeof(buf))
> -        return;
> +    if (ifm->m_len + ETH_HLEN > sizeof(buf)) {
> +        return 1;
> +    }
>  
>      if (!arp_table_search(slirp, iph->ip_dst.s_addr, ethaddr)) {
>          uint8_t arp_req[ETH_HLEN + sizeof(struct arphdr)];
>          struct ethhdr *reh = (struct ethhdr *)arp_req;
>          struct arphdr *rah = (struct arphdr *)(arp_req + ETH_HLEN);
>  
> -        /* If the client addr is not known, there is no point in
> -           sending the packet to it. Normally the sender should have
> -           done an ARP request to get its MAC address. Here we do it
> -           in place of sending the packet and we hope that the sender
> -           will retry sending its packet. */
> -        memset(reh->h_dest, 0xff, ETH_ALEN);
> -        memcpy(reh->h_source, special_ethaddr, ETH_ALEN - 4);
> -        memcpy(&reh->h_source[2], &slirp->vhost_addr, 4);
> -        reh->h_proto = htons(ETH_P_ARP);
> -        rah->ar_hrd = htons(1);
> -        rah->ar_pro = htons(ETH_P_IP);
> -        rah->ar_hln = ETH_ALEN;
> -        rah->ar_pln = 4;
> -        rah->ar_op = htons(ARPOP_REQUEST);
> -        /* source hw addr */
> -        memcpy(rah->ar_sha, special_ethaddr, ETH_ALEN - 4);
> -        memcpy(&rah->ar_sha[2], &slirp->vhost_addr, 4);
> -        /* source IP */
> -        rah->ar_sip = slirp->vhost_addr.s_addr;
> -        /* target hw addr (none) */
> -        memset(rah->ar_tha, 0, ETH_ALEN);
> -        /* target IP */
> -        rah->ar_tip = iph->ip_dst.s_addr;
> -        slirp->client_ipaddr = iph->ip_dst;
> -        slirp_output(slirp->opaque, arp_req, sizeof(arp_req));
> +        if (!ifm->arp_requested) {
> +            /* If the client addr is not known, send an ARP request */
> +            memset(reh->h_dest, 0xff, ETH_ALEN);
> +            memcpy(reh->h_source, special_ethaddr, ETH_ALEN - 4);
> +            memcpy(&reh->h_source[2], &slirp->vhost_addr, 4);
> +            reh->h_proto = htons(ETH_P_ARP);
> +            rah->ar_hrd = htons(1);
> +            rah->ar_pro = htons(ETH_P_IP);
> +            rah->ar_hln = ETH_ALEN;
> +            rah->ar_pln = 4;
> +            rah->ar_op = htons(ARPOP_REQUEST);
> +
> +            /* source hw addr */
> +            memcpy(rah->ar_sha, special_ethaddr, ETH_ALEN - 4);
> +            memcpy(&rah->ar_sha[2], &slirp->vhost_addr, 4);
> +
> +            /* source IP */
> +            rah->ar_sip = slirp->vhost_addr.s_addr;
> +
> +            /* target hw addr (none) */
> +            memset(rah->ar_tha, 0, ETH_ALEN);
> +
> +            /* target IP */
> +            rah->ar_tip = iph->ip_dst.s_addr;
> +            slirp->client_ipaddr = iph->ip_dst;
> +            slirp_output(slirp->opaque, arp_req, sizeof(arp_req));
> +            ifm->arp_requested = true;
> +        }
> +        return 0;
>      } else {
>          memcpy(eh->h_dest, ethaddr, ETH_ALEN);
>          memcpy(eh->h_source, special_ethaddr, ETH_ALEN - 4);
>          /* XXX: not correct */
>          memcpy(&eh->h_source[2], &slirp->vhost_addr, 4);
>          eh->h_proto = htons(ETH_P_IP);
> -        memcpy(buf + sizeof(struct ethhdr), ip_data, ip_data_len);
> -        slirp_output(slirp->opaque, buf, ip_data_len + ETH_HLEN);
> +        memcpy(buf + sizeof(struct ethhdr), ifm->m_data, ifm->m_len);
> +        slirp_output(slirp->opaque, buf, ifm->m_len + ETH_HLEN);
> +        return 1;
>      }
>  }
>  
> -- 
> 1.7.3.4
> 
> 

		Amit

Re: [Qemu-devel] [PATCH 3/3] Delayed IP packets

[Jan Kiszka]

On 2011-09-29 18:06, Amit Shah wrote:
> On (Wed) 03 Aug 2011 [13:24:22], Jan Kiszka wrote:
>> From: Fabien Chouteau <chouteau@adacore.com>
>>
>> In the current implementation, if Slirp tries to send an IP packet to a client
>> with an unknown hardware address, the packet is simply dropped and an ARP
>> request is sent (if_encap in slirp/slirp.c).
>>
>> With this patch, Slirp will send the ARP request, re-queue the packet and try
>> to send it later. The packet is dropped after one second if the ARP reply is
>> not received.
> 
> This patch causes a segfault when guests wake up from hibernate.
> 
> Recipe:
> 1. Start guest with -net user -net nic,model=virtio
> 2. (guest) ping 10.0.2.2
> 3. (guest) echo "disk" > /sys/power/state
> 4. Re-start guest with same command line
> 5. Ping has stopped receiving replies.
> 6. Kill that ping process and start a new one.  qemu segfaults.

Can't reproduce, I'm not getting stable hibernation here even without
any network configured.

Could you check if the recent pull request [1] changes the picture for you?

Thanks,
Jan

[1] http://thread.gmane.org/gmane.comp.emulators.qemu/118992

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 3/3] Delayed IP packets

[Amit Shah]

On (Thu) 29 Sep 2011 [19:41:33], Jan Kiszka wrote:
> On 2011-09-29 18:06, Amit Shah wrote:
> > On (Wed) 03 Aug 2011 [13:24:22], Jan Kiszka wrote:
> >> From: Fabien Chouteau <chouteau@adacore.com>
> >>
> >> In the current implementation, if Slirp tries to send an IP packet to a client
> >> with an unknown hardware address, the packet is simply dropped and an ARP
> >> request is sent (if_encap in slirp/slirp.c).
> >>
> >> With this patch, Slirp will send the ARP request, re-queue the packet and try
> >> to send it later. The packet is dropped after one second if the ARP reply is
> >> not received.
> > 
> > This patch causes a segfault when guests wake up from hibernate.
> > 
> > Recipe:
> > 1. Start guest with -net user -net nic,model=virtio
> > 2. (guest) ping 10.0.2.2
> > 3. (guest) echo "disk" > /sys/power/state
> > 4. Re-start guest with same command line
> > 5. Ping has stopped receiving replies.
> > 6. Kill that ping process and start a new one.  qemu segfaults.
> 
> Can't reproduce, I'm not getting stable hibernation here even without
> any network configured.

With virtio devices and the patches applied?  Can you tell me what
you're seeing?

> Could you check if the recent pull request [1] changes the picture for you?

Thanks, that series fixes the problem.

		Amit

Re: [Qemu-devel] [PATCH 3/3] Delayed IP packets

[Jan Kiszka]

On 2011-09-29 19:50, Amit Shah wrote:
> On (Thu) 29 Sep 2011 [19:41:33], Jan Kiszka wrote:
>> On 2011-09-29 18:06, Amit Shah wrote:
>>> On (Wed) 03 Aug 2011 [13:24:22], Jan Kiszka wrote:
>>>> From: Fabien Chouteau <chouteau@adacore.com>
>>>>
>>>> In the current implementation, if Slirp tries to send an IP packet to a client
>>>> with an unknown hardware address, the packet is simply dropped and an ARP
>>>> request is sent (if_encap in slirp/slirp.c).
>>>>
>>>> With this patch, Slirp will send the ARP request, re-queue the packet and try
>>>> to send it later. The packet is dropped after one second if the ARP reply is
>>>> not received.
>>>
>>> This patch causes a segfault when guests wake up from hibernate.
>>>
>>> Recipe:
>>> 1. Start guest with -net user -net nic,model=virtio
>>> 2. (guest) ping 10.0.2.2
>>> 3. (guest) echo "disk" > /sys/power/state
>>> 4. Re-start guest with same command line
>>> 5. Ping has stopped receiving replies.
>>> 6. Kill that ping process and start a new one.  qemu segfaults.
>>
>> Can't reproduce, I'm not getting stable hibernation here even without
>> any network configured.
> 
> With virtio devices and the patches applied?  Can you tell me what
> you're seeing?

No, I didn't patch my guest. I was using standard IDE with an emulated
NIC (or without) against a 3.1-rc3 (or so) guest.

> 
>> Could you check if the recent pull request [1] changes the picture for you?
> 
> Thanks, that series fixes the problem.

Perfect! Right in time. :)

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 3/3] Delayed IP packets

[Amit Shah]

On (Thu) 29 Sep 2011 [19:53:47], Jan Kiszka wrote:
> >> Can't reproduce, I'm not getting stable hibernation here even without
> >> any network configured.
> > 
> > With virtio devices and the patches applied?  Can you tell me what
> > you're seeing?
> 
> No, I didn't patch my guest. I was using standard IDE with an emulated
> NIC (or without) against a 3.1-rc3 (or so) guest.

Strange, using qemu.git and an F14 guest (2.6.38) using this cmd line:

./x86_64-softmmu/qemu-system-x86_64 -m 512 /guests/f14-suspend.qcow2 -net none -enable-kvm -smp 2

I could successfully hibernate and resume.

> >> Could you check if the recent pull request [1] changes the picture for you?
> > 
> > Thanks, that series fixes the problem.
> 
> Perfect! Right in time. :)

And people say slirp is neglected and unmaintainable :-)

		Amit

Re: [Qemu-devel] [PATCH 3/3] Delayed IP packets

[Jan Kiszka]

On 2011-09-29 20:05, Amit Shah wrote:
> On (Thu) 29 Sep 2011 [19:53:47], Jan Kiszka wrote:
>>>> Can't reproduce, I'm not getting stable hibernation here even without
>>>> any network configured.
>>>
>>> With virtio devices and the patches applied?  Can you tell me what
>>> you're seeing?
>>
>> No, I didn't patch my guest. I was using standard IDE with an emulated
>> NIC (or without) against a 3.1-rc3 (or so) guest.
> 
> Strange, using qemu.git and an F14 guest (2.6.38) using this cmd line:
> 
> ./x86_64-softmmu/qemu-system-x86_64 -m 512 /guests/f14-suspend.qcow2 -net none -enable-kvm -smp 2
> 
> I could successfully hibernate and resume.

-cpu qemu64,-kvmclock makes it work. Would have to check a different
kernel version to find out if it's a guest kernel or qemu/kvm issue.

> 
>>>> Could you check if the recent pull request [1] changes the picture for you?
>>>
>>> Thanks, that series fixes the problem.
>>
>> Perfect! Right in time. :)
> 
> And people say slirp is neglected and unmaintainable :-)

Who says this? I think slirp just needed some attention. It already
presented two fairly old bugs to me, but strangely right after I adopted
it... ;)

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 3/3] Delayed IP packets

[Amit Shah]

On (Thu) 29 Sep 2011 [20:19:42], Jan Kiszka wrote:
> On 2011-09-29 20:05, Amit Shah wrote:
> > On (Thu) 29 Sep 2011 [19:53:47], Jan Kiszka wrote:
> >>>> Can't reproduce, I'm not getting stable hibernation here even without
> >>>> any network configured.
> >>>
> >>> With virtio devices and the patches applied?  Can you tell me what
> >>> you're seeing?
> >>
> >> No, I didn't patch my guest. I was using standard IDE with an emulated
> >> NIC (or without) against a 3.1-rc3 (or so) guest.
> > 
> > Strange, using qemu.git and an F14 guest (2.6.38) using this cmd line:
> > 
> > ./x86_64-softmmu/qemu-system-x86_64 -m 512 /guests/f14-suspend.qcow2 -net none -enable-kvm -smp 2
> > 
> > I could successfully hibernate and resume.
> 
> -cpu qemu64,-kvmclock makes it work. Would have to check a different
> kernel version to find out if it's a guest kernel or qemu/kvm issue.

Aha, yes.  I disable kvmclock in my kernels as that has known issues
with hibernate.  I haven't checked if the 2.6.38 f14 kernel had it
disabled, or maybe I didn't hit its problem point soon enough.

		Amit

Re: [Qemu-devel] [PATCH 3/3] Delayed IP packets

[Alexander Graf]

On 29.09.2011, at 18:06, Amit Shah wrote:

> On (Wed) 03 Aug 2011 [13:24:22], Jan Kiszka wrote:
>> From: Fabien Chouteau <chouteau@adacore.com>
>> 
>> In the current implementation, if Slirp tries to send an IP packet to a client
>> with an unknown hardware address, the packet is simply dropped and an ARP
>> request is sent (if_encap in slirp/slirp.c).
>> 
>> With this patch, Slirp will send the ARP request, re-queue the packet and try
>> to send it later. The packet is dropped after one second if the ARP reply is
>> not received.
> 
> This patch causes a segfault when guests wake up from hibernate.
> 
> Recipe:
> 1. Start guest with -net user -net nic,model=virtio
> 2. (guest) ping 10.0.2.2
> 3. (guest) echo "disk" > /sys/power/state
> 4. Re-start guest with same command line
> 5. Ping has stopped receiving replies.
> 6. Kill that ping process and start a new one.  qemu segfaults.
> 
> This needs the not-upstream-yet virtio S4 handling patches, found at
> http://thread.gmane.org/gmane.linux.kernel/1197141
> 
> The backtrace is:
> 
> (gdb) bt
> #0  0x00007ffff7e421f7 in slirp_insque (a=0x0, b=0x7ffff8f95d50) at
> /home/amit/src/qemu/slirp/misc.c:27
> #1  0x00007ffff7e40738 in if_start (slirp=0x7ffff8a9cdf0) at
> /home/amit/src/qemu/slirp/if.c:194
> #2  0x00007ffff7e44828 in slirp_select_poll (readfds=0x7fffffffd930,
> writefds=0x7fffffffd9b0, xfds=0x7fffffffda30, select_error=0)
>    at /home/amit/src/qemu/slirp/slirp.c:588
> #3  0x00007ffff7e110f1 in main_loop_wait (nonblocking=<optimized out>)
> at /home/amit/src/qemu/vl.c:1549
> #4  0x00007ffff7d7dc47 in main_loop () at
> /home/amit/src/qemu/vl.c:1579
> #5  main (argc=<optimized out>, argv=<optimized out>, envp=<optimized
> out>) at /home/amit/src/qemu/vl.c:3574
> 
> 
> Reverting the patch keeps the ping going on after resume.  

I get the same thing with yesterday's HEAD (close to 1.0-rc3), but without hibernation.

I'm running KVM Autotest on PPC machines to check my ppc-next queue and every single test failed for me because of segmentation faults in the slirp code. Reverting this patch (and the follow-up patch which fixes the struct mbuf definition) makes all tests not segfault for me, so I'm fairly sure this is the offending one :).

I'm not saying that the patch is actually wrong - maybe it only exposes another bug that was only hidden so far. Either way, the breakage looks pretty much like memory corruption to me.

Also, I'm having a hard time reproducing the problem manually. It triggers every time in Autotest, but never when I try to trigger it manually. Essentially Autotest is merely trying to connect to the guest using ssh every couple of seconds, so I don't know why I can't reproduce it without it.

Please fix or revert this for 1.0.


Alex

Re: [Qemu-devel] [PATCH 0/3] [PULL] slirp: ARP table improvements

[Anthony Liguori]

On 08/03/2011 06:24 AM, Jan Kiszka wrote:
> The following changes since commit 927d721777e73339f73719f36eaf400ab641366c:
>
>    microblaze: Add missing call to qemu_init_vcpu. (2011-07-31 06:40:13 +0200)
>
> are available in the git repository at:
>    git://git.kiszka.org/qemu.git queues/slirp
>
> Anthony asked me to look after slirp patches, and I agreed. So here
> comes the first pull request. It improves the so far minimalistic ARP
> support of slirp by avoiding premature packet drops when addressing not
> yet resolved client IPs.
>
>
> CC: Fabien Chouteau<chouteau@adacore.com>

Pulled.  Thanks for watching over the slirp bits!

Regards,

Anthony Liguori

>
> Fabien Chouteau (2):
>    Simple ARP table
>    Delayed IP packets
>
> Jan Kiszka (1):
>    slirp: Take maintainer token
>
>   MAINTAINERS       |    5 +-
>   Makefile.objs     |    2 +-
>   slirp/arp_table.c |   95 +++++++++++++++++++++++++++++++++++++
>   slirp/bootp.c     |   21 +++++---
>   slirp/if.c        |   28 +++++++++--
>   slirp/main.h      |    2 +-
>   slirp/mbuf.c      |    2 +
>   slirp/mbuf.h      |    2 +
>   slirp/slirp.c     |  135 ++++++++++++++++++++++-------------------------------
>   slirp/slirp.h     |   47 +++++++++++++++++-
>   10 files changed, 241 insertions(+), 98 deletions(-)
>   create mode 100644 slirp/arp_table.c
>