[Qemu-devel] [PATCH 1/2] Usb: create the receive queue for the virtual USB NIC

[rongqing.li]

From: Roy.Li <address@hidden>

The virtual USB NIC originally used a fixed buffer to receive packets which
only store 1 packet at a time, which is easy to overrun with packets if the
guest does not consume it quickly, and always lost packets at the below case:

        The emulated machine is using an USB-ethernet adapter,
        it is connected to the network using SLIRP and I'm
        dumping the traffic in a .pcap file.  As per the following
        command line :
        -net nic,model=usb,vlan=1 -net user,vlan=1 -net 
dump,vlan=1,file=/tmp/pkt.pcap
        Every time that two packets are coming in a row from
        the host, the usb-net code will receive the first one,
        then returns 0 to can_receive call since it has a
        1 packet long queue. But as the dump code is always
        ready to receive, qemu_can_send_packet will return
        true and the next packet will discard the previous
        one in the usb-net code.

Commit 60c07d933c(net: fix qemu_can_send_packet logic) is trying to fix,
but the logic is wrong and introduce other bug. Now replace the fixed buffer
with a receive queue which can accept a variable number of packets to fix
this bug.

Signed-off-by: Roy.Li <address@hidden>
---
 hw/usb/dev-network.c |   87 ++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 63 insertions(+), 24 deletions(-)

diff --git a/hw/usb/dev-network.c b/hw/usb/dev-network.c
index c84892c..0c867b7 100644
--- a/hw/usb/dev-network.c
+++ b/hw/usb/dev-network.c
@@ -87,6 +87,7 @@ enum usbstring_idx {
 #define STATUS_BYTECOUNT               16   /* 8 byte header + data */
 
 #define ETH_FRAME_LEN                  1514 /* Max. octets in frame sans FCS */
+#define MAX_RCV_QUEUE                  45 /* Max length of receiving queue */
 
 static const USBDescStrings usb_net_stringtable = {
     [STRING_MANUFACTURER]       = "QEMU",
@@ -622,6 +623,12 @@ struct rndis_response {
     uint8_t buf[0];
 };
 
+struct USBNet_buffer {
+    QTAILQ_ENTRY(USBNet_buffer) entries;
+    unsigned int in_ptr, in_len;
+    uint8_t in_buf[0];
+};
+
 typedef struct USBNetState {
     USBDevice dev;
 
@@ -636,8 +643,8 @@ typedef struct USBNetState {
     uint8_t out_buf[2048];
 
     USBPacket *inpkt;
-    unsigned int in_ptr, in_len;
-    uint8_t in_buf[2048];
+    QTAILQ_HEAD(USBNet_buffer_head, USBNet_buffer) rndis_netbuf;
+    uint32_t buf_len;
 
     char usbstring_mac[13];
     NICState *nic;
@@ -867,6 +874,17 @@ static void rndis_clear_responsequeue(USBNetState *s)
     }
 }
 
+static void rndis_clear_netbuffer(USBNetState *s)
+{
+    struct USBNet_buffer *r;
+
+    while ((r = s->rndis_netbuf.tqh_first)) {
+        QTAILQ_REMOVE(&s->rndis_netbuf, r, entries);
+        g_free(r);
+    }
+    s->buf_len = 0;
+}
+
 static int rndis_init_response(USBNetState *s, rndis_init_msg_type *buf)
 {
     rndis_init_cmplt_type *resp =
@@ -1025,7 +1043,8 @@ static int rndis_parse(USBNetState *s, uint8_t *data, int 
length)
 
     case RNDIS_RESET_MSG:
         rndis_clear_responsequeue(s);
-        s->out_ptr = s->in_ptr = s->in_len = 0;
+        rndis_clear_netbuffer(s);
+        s->out_ptr = 0;
         return rndis_reset_response(s, (rndis_reset_msg_type *) data);
 
     case RNDIS_KEEPALIVE_MSG:
@@ -1134,25 +1153,39 @@ static int usb_net_handle_datain(USBNetState *s, 
USBPacket *p)
 {
     int ret = USB_RET_NAK;
 
-    if (s->in_ptr > s->in_len) {
-        s->in_ptr = s->in_len = 0;
-        ret = USB_RET_NAK;
+    struct USBNet_buffer *r = s->rndis_netbuf.tqh_first;
+
+    if (!r) {
         return ret;
     }
-    if (!s->in_len) {
-        ret = USB_RET_NAK;
+
+    if (r->in_ptr > r->in_len) {
+        s->buf_len--;
+        QTAILQ_REMOVE(&s->rndis_netbuf, r, entries);
+        g_free(r);
+        return ret;
+    }
+
+    if (!r->in_len) {
+        s->buf_len--;
+        QTAILQ_REMOVE(&s->rndis_netbuf, r, entries);
+        g_free(r);
         return ret;
     }
-    ret = s->in_len - s->in_ptr;
+
+    ret = r->in_len - r->in_ptr;
     if (ret > p->iov.size) {
         ret = p->iov.size;
     }
-    usb_packet_copy(p, &s->in_buf[s->in_ptr], ret);
-    s->in_ptr += ret;
-    if (s->in_ptr >= s->in_len &&
-                    (is_rndis(s) || (s->in_len & (64 - 1)) || !ret)) {
+
+    usb_packet_copy(p, &r->in_buf[r->in_ptr], ret);
+    r->in_ptr += ret;
+    if (r->in_ptr >= r->in_len &&
+                    (is_rndis(s) || (r->in_len & (64 - 1)) || !ret)) {
         /* no short packet necessary */
-        s->in_ptr = s->in_len = 0;
+        s->buf_len--;
+        QTAILQ_REMOVE(&s->rndis_netbuf, r, entries);
+        g_free(r);
     }
 
 #ifdef TRAFFIC_DEBUG
@@ -1251,15 +1284,16 @@ static ssize_t usbnet_receive(NetClientState *nc, const 
uint8_t *buf, size_t siz
 {
     USBNetState *s = DO_UPCAST(NICState, nc, nc)->opaque;
     struct rndis_packet_msg_type *msg;
+    struct USBNet_buffer *r;
 
     if (is_rndis(s)) {
-        msg = (struct rndis_packet_msg_type *) s->in_buf;
         if (s->rndis_state != RNDIS_DATA_INITIALIZED) {
             return -1;
         }
-        if (size + sizeof(struct rndis_packet_msg_type) > sizeof(s->in_buf))
-            return -1;
 
+        r = g_malloc0(sizeof(struct USBNet_buffer) + \
+                      sizeof(struct rndis_packet_msg_type) + size);
+        msg = (struct rndis_packet_msg_type *) r->in_buf;
         memset(msg, 0, sizeof(struct rndis_packet_msg_type));
         msg->MessageType = cpu_to_le32(RNDIS_PACKET_MSG);
         msg->MessageLength = cpu_to_le32(size + sizeof(struct 
rndis_packet_msg_type));
@@ -1274,14 +1308,16 @@ static ssize_t usbnet_receive(NetClientState *nc, const 
uint8_t *buf, size_t siz
          * msg->Reserved;
          */
         memcpy(msg + 1, buf, size);
-        s->in_len = size + sizeof(struct rndis_packet_msg_type);
+        r->in_len = size + sizeof(struct rndis_packet_msg_type);
     } else {
-        if (size > sizeof(s->in_buf))
-            return -1;
-        memcpy(s->in_buf, buf, size);
-        s->in_len = size;
+        r = g_malloc0(sizeof(struct USBNet_buffer) + size);
+        memcpy(r->in_buf, buf, size);
+        r->in_len = size;
     }
-    s->in_ptr = 0;
+    r->in_ptr = 0;
+    s->buf_len++;
+    QTAILQ_INSERT_TAIL(&s->rndis_netbuf, r, entries);
+
     return size;
 }
 
@@ -1293,7 +1329,7 @@ static int usbnet_can_receive(NetClientState *nc)
         return 1;
     }
 
-    return !s->in_len;
+    return s->buf_len < MAX_RCV_QUEUE;
 }
 
 static void usbnet_cleanup(NetClientState *nc)
@@ -1309,6 +1345,7 @@ static void usb_net_handle_destroy(USBDevice *dev)
 
     /* TODO: remove the nd_table[] entry */
     rndis_clear_responsequeue(s);
+    rndis_clear_netbuffer(s);
     qemu_del_net_client(&s->nic->nc);
 }
 
@@ -1329,12 +1366,14 @@ static int usb_net_initfn(USBDevice *dev)
 
     s->rndis_state = RNDIS_UNINITIALIZED;
     QTAILQ_INIT(&s->rndis_resp);
+    QTAILQ_INIT(&s->rndis_netbuf);
 
     s->medium = 0;     /* NDIS_MEDIUM_802_3 */
     s->speed = 1000000; /* 100MBps, in 100Bps units */
     s->media_state = 0;        /* NDIS_MEDIA_STATE_CONNECTED */;
     s->filter = 0;
     s->vendorid = 0x1234;
+    s->buf_len = 0;
 
     qemu_macaddr_default_if_unset(&s->conf.macaddr);
     s->nic = qemu_new_nic(&net_usbnet_info, &s->conf,
-- 
1.7.4.1