[Qemu-devel] [PATCH 00/12] RFC: usb: input pipelining support and other speedups

[Qemu-devel] [PATCH 00/12] RFC: usb: input pipelining support and other speedups

[Hans de Goede]

This patchset started out as adding support for pipelining of input bulk
transfers, although input pipelining is a bit tricky to do, there are 2 good
reasons to do it:

1) Performance, I expect this to ie significantly speed-up reading from USB
mass storage devices.

2) Currently reading from a USB<->serial converter, ie a pl2303 based one is
unreliable when the serial transport is not using flowcontrol. The problem
is that 115200 bps takes aprox 200 64 bytes packets / sec, which means we
must not exceed a roundtip time average of 5 ms over a period of 20 ms. If
within any 20 ms period we exceed that round trip time average the internal
buffer of the pl2303 will likely overflow. Input pipelining greatly improve
things here as: it combines the previous 4 packets submitted per device-driver
level read into 1 big read, reducing the number of packets / sec by a factor 4,
and it allows to have multiple reads outstanding.

I had working on this on my mind for a while because of 1), and 2) has made
me spend the necessary time to get this working now. Even though 2 was my
main reason to start working on this now, most of my testing sofar has
focussed on 1, resulting in some more speedup patches and a total improvement
of the read speed from a cheap USB thumb drive from 4.4 MB/s to 10.8 MB/s or
a factor of 2.4 !

The reason this patch-set is currently still RFC is because I need to run more
tests wrt 2). Despite the RFC status reviews are very much welcome now!

Regards,

Hans

[Qemu-devel] [PATCH 01/12] usb-redir: When a packet contains data on a stall, ignore the stall

[Hans de Goede]

It is possbile for bulk packets to transfer some of the data and
to then stall. ATM our usb core allows us to return either data, or an
error, not both. For now return the data rather then the stall when this
happens, counting on further packets to detect the stall.
In the future we should fix the qemu usb core to handle packets
completing with some data and an error.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/redirect.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index 022ba42..ae7393f 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -1152,6 +1152,10 @@ static int usbredir_handle_status(USBRedirDevice *dev,
     case usb_redir_success:
         return actual_len;
     case usb_redir_stall:
+        if (actual_len > 0) {
+            WARNING("got both data and stall, returning data\n");
+            return actual_len;
+        }
         return USB_RET_STALL;
     case usb_redir_cancelled:
         /*
-- 
1.7.12.1

[Qemu-devel] [PATCH 02/12] usb-redir: Add support for 32 bits bulk packet length

[Hans de Goede]

And also switch to using usb_ep_set_pipeline(), rather then modifying
the ep directly.

This is a preparation patch for adding input queuing support.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 configure         |  2 +-
 hw/usb/redirect.c | 20 +++++++++++++++++---
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/configure b/configure
index 248b871..036c38a 100755
--- a/configure
+++ b/configure
@@ -2768,7 +2768,7 @@ fi
 
 # check for usbredirparser for usb network redirection support
 if test "$usb_redir" != "no" ; then
-    if $pkg_config --atleast-version=0.5 libusbredirparser-0.5 >/dev/null 2>&1 ; then
+    if $pkg_config --atleast-version=0.5.3 libusbredirparser-0.5 >/dev/null 2>&1 ; then
         usb_redir="yes"
         usb_redir_cflags=$($pkg_config --cflags libusbredirparser-0.5 2>/dev/null)
         usb_redir_libs=$($pkg_config --libs libusbredirparser-0.5 2>/dev/null)
diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index ae7393f..19fe00b 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -585,6 +585,10 @@ static int usbredir_handle_bulk_data(USBRedirDevice *dev, USBPacket *p,
     bulk_packet.endpoint  = ep;
     bulk_packet.length    = p->iov.size;
     bulk_packet.stream_id = 0;
+    bulk_packet.length_high = p->iov.size >> 16;
+    assert(bulk_packet.length_high == 0 ||
+           usbredirparser_peer_has_cap(dev->parser,
+                                       usb_redir_cap_32bits_bulk_length));
 
     if (ep & USB_DIR_IN) {
         usbredirparser_send_bulk_packet(dev->parser, p->id,
@@ -906,6 +910,7 @@ static void usbredir_create_parser(USBRedirDevice *dev)
     usbredirparser_caps_set_cap(caps, usb_redir_cap_filter);
     usbredirparser_caps_set_cap(caps, usb_redir_cap_ep_info_max_packet_size);
     usbredirparser_caps_set_cap(caps, usb_redir_cap_64bits_ids);
+    usbredirparser_caps_set_cap(caps, usb_redir_cap_32bits_bulk_length);
 
     if (runstate_check(RUN_STATE_INMIGRATE)) {
         flags |= usbredirparser_fl_no_hello;
@@ -1302,7 +1307,7 @@ static void usbredir_ep_info(void *priv,
 {
     USBRedirDevice *dev = priv;
     struct USBEndpoint *usb_ep;
-    int i;
+    int i, pid;
 
     for (i = 0; i < MAX_ENDPOINTS; i++) {
         dev->endpoint[i].type = ep_info->type[i];
@@ -1339,7 +1344,16 @@ static void usbredir_ep_info(void *priv,
                 usb_ep->max_packet_size = ep_info->max_packet_size[i];
         }
         if (ep_info->type[i] == usb_redir_type_bulk) {
-            usb_ep->pipeline = true;
+            if (i & 0x10) {
+                if (!usbredirparser_peer_has_cap(dev->parser,
+                                       usb_redir_cap_32bits_bulk_length)) {
+                    continue; /* No input pipeline without 32 bits length! */
+                }
+                pid = USB_TOKEN_IN;
+            } else {
+                pid = USB_TOKEN_OUT;
+            }
+            usb_ep_set_pipeline(&dev->dev, pid, i & 0x0f, true);
         }
     }
 }
@@ -1471,7 +1485,7 @@ static void usbredir_bulk_packet(void *priv, uint64_t id,
 {
     USBRedirDevice *dev = priv;
     uint8_t ep = bulk_packet->endpoint;
-    int len = bulk_packet->length;
+    int len = (bulk_packet->length_high << 16) | bulk_packet->length;
     USBPacket *p;
 
     DPRINTF("bulk-in status %d ep %02X len %d id %"PRIu64"\n",
-- 
1.7.12.1

[Qemu-devel] [PATCH 03/12] usb-host-linux: Only enabling pipeling for output endpoints

[Hans de Goede]

With the upcoming input pipelining support, large input packets
may get submitted to the device, which require special handling when
the packets ends up being split again by usb-host-linux due to usbfs
limitations. The exact demands for properly handling larger split input
transfers is explained in detail in this libusb commit:
https://github.com/libusbx/libusbx/commit/ede02ba91920f9be787a7f3cd006c5a4b92b5eab

Specifically in the large comment block the commit adds. Note that IMHO
it would be better to just port usb-host-linux to libusb and let libusb
worry about such usbfs details, rather then reproducing all this code
inside host-linux.c

Note that the not proper handling of this currently already can be a problem
when used in combination with an emulated xhci controller, as that can
already submit large enough transfers to trigger this.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/host-linux.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/usb/host-linux.c b/hw/usb/host-linux.c
index 44f1a64..3a258b4 100644
--- a/hw/usb/host-linux.c
+++ b/hw/usb/host-linux.c
@@ -1224,7 +1224,8 @@ static int usb_linux_update_endp_table(USBHostDevice *s)
                 usb_ep_set_type(&s->dev, pid, ep, type);
                 usb_ep_set_ifnum(&s->dev, pid, ep, interface);
                 if ((s->options & (1 << USB_HOST_OPT_PIPELINE)) &&
-                    (type == USB_ENDPOINT_XFER_BULK)) {
+                    (type == USB_ENDPOINT_XFER_BULK) &&
+                    (pid == USB_TOKEN_OUT)) {
                     usb_ep_set_pipeline(&s->dev, pid, ep, true);
                 }
 
-- 
1.7.12.1

[Qemu-devel] [PATCH 04/12] usb: Add support for input pipelining

[Hans de Goede]

Currently we effectively only do pipelining for output endpoints, since
controllers will stop filling the queue for a packet with stop the queue
on a short read semantics enabled.

This causes large input transfers to get split into multiple packets, which
comes with a serious performance penalty.

This patch makes it possible to do pipelining for input endpoints, by
re-combining packets which belong to 1 transfer at the guest device-driver
level into 1 large packet before sending them to the device, this is mostly
useful as a significant performance improvement for redirection of real USB
devices.

This patch will combine packets together into 1 until a transfer terminating
packet is encountered. A terminating packet is a packet which meets one of
the following conditions:
1) The packet size is *not* a multiple of the endpoint max packet size
2) The packet does *not* have its short-not-ok flag set
3) The packet has its interrupt-on-complete flag set

The short-not-ok flag of the combined packet is that of the terminating packet.
Multiple combined packets may be submitted to the device, if the combined
packets do not have their short-not-ok flag set, enabling true pipelining.

If a combined packet does have its short-not-ok flag set the queue will
wait with submitting further packets to the device until that packet has
completed.

Once enabled in the ehci driver, this improves the speed (MB/s) of a Linux
guest reading from a USB mass storage device by a factor of 1.2 - 1.5.

And the main reason why I started working on this, when reading from a pl2303
USB<->serial converter, it combines the previous 4 packets submitted per
device-driver level read into 1 big read, reducing the number of packets / sec
by a factor 4, and it allows to have multiple reads outstanding. This allows
for much better latency tolerance without the pl2303's internal buffer
overflowing (which was happening at 115200 bps, without serial flow control).

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb.h         |  24 ++++++++
 hw/usb/core.c    | 176 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
 hw/usb/dev-hub.c |   9 +++
 3 files changed, 205 insertions(+), 4 deletions(-)

diff --git a/hw/usb.h b/hw/usb.h
index 2ecd8e9..57d200b 100644
--- a/hw/usb.h
+++ b/hw/usb.h
@@ -312,6 +312,15 @@ typedef struct USBPortOps {
      * the packet originated when a hub is involved.
      */
     void (*complete)(USBPort *port, USBPacket *p);
+    /*
+     * Called by the usb-core when a previous queued up packet
+     * (usb_handle_packet returned USB_RET_ASYNC) gets removed from the
+     * queue, without completing or being cancelled.
+     * This happens with input pipelinging when multiple packets are combined
+     * into one large transfer, and that transfer ends with a short read.
+     * Mandatory for controllers which set supports_queuing on their ports.
+     */
+    void (*remove_from_queue)(USBPort *port, USBPacket *p);
 } USBPortOps;
 
 /* USB port on which a device can be connected */
@@ -322,6 +331,7 @@ struct USBPort {
     USBPortOps *ops;
     void *opaque;
     int index; /* internal port index, may be used with the opaque */
+    bool supports_queuing;
     QTAILQ_ENTRY(USBPort) next;
 };
 
@@ -347,7 +357,12 @@ struct USBPacket {
     int result; /* transfer length or USB_RET_* status code */
     /* Internal use by the USB layer.  */
     USBPacketState state;
+    USBPacket *combined_in;
+    QTAILQ_HEAD(, USBPacket) uncombined_packets;
     QTAILQ_ENTRY(USBPacket) queue;
+    /* Flags set by the hcd when queuing is supported */
+    bool short_not_ok;
+    bool int_req;
 };
 
 void usb_packet_init(USBPacket *p);
@@ -387,6 +402,15 @@ int usb_ep_get_max_packet_size(USBDevice *dev, int pid, int ep);
 void usb_ep_set_pipeline(USBDevice *dev, int pid, int ep, bool enabled);
 USBPacket *usb_ep_find_packet_by_id(USBDevice *dev, int pid, int ep,
                                     uint64_t id);
+static inline bool usb_ep_input_pipeline(USBEndpoint *ep)
+{
+    return ep->pid == USB_TOKEN_IN && ep->pipeline;
+}
+static inline bool usb_ep_output_pipeline(USBEndpoint *ep)
+{
+    return ep->pid == USB_TOKEN_OUT && ep->pipeline;
+}
+void usb_ep_process_queue(USBEndpoint *ep);
 
 void usb_attach(USBPort *port);
 void usb_detach(USBPort *port);
diff --git a/hw/usb/core.c b/hw/usb/core.c
index b9f1f7a..54db17b 100644
--- a/hw/usb/core.c
+++ b/hw/usb/core.c
@@ -388,7 +388,8 @@ int usb_handle_packet(USBDevice *dev, USBPacket *p)
         p->ep->halted = false;
     }
 
-    if (QTAILQ_EMPTY(&p->ep->queue) || p->ep->pipeline) {
+    if (!usb_ep_input_pipeline(p->ep) &&
+            (QTAILQ_EMPTY(&p->ep->queue) || usb_ep_output_pipeline(p->ep))) {
         ret = usb_process_one(p);
         if (ret == USB_RET_ASYNC) {
             usb_packet_set_state(p, USB_PACKET_ASYNC);
@@ -398,7 +399,8 @@ int usb_handle_packet(USBDevice *dev, USBPacket *p)
              * When pipelining is enabled usb-devices must always return async,
              * otherwise packets can complete out of order!
              */
-            assert(!p->ep->pipeline || QTAILQ_EMPTY(&p->ep->queue));
+            assert(!usb_ep_output_pipeline(p->ep) ||
+                   QTAILQ_EMPTY(&p->ep->queue));
             if (ret != USB_RET_NAK) {
                 p->result = ret;
                 usb_packet_set_state(p, USB_PACKET_COMPLETE);
@@ -412,6 +414,21 @@ int usb_handle_packet(USBDevice *dev, USBPacket *p)
     return ret;
 }
 
+static void usb_add_packet_to_uncombined(USBPacket *dst, USBPacket *src)
+{
+    qemu_iovec_concat(&dst->iov, &src->iov, 0, src->iov.size);
+    QTAILQ_REMOVE(&dst->ep->queue, src, queue);
+    QTAILQ_INSERT_TAIL(&dst->uncombined_packets, src, queue);
+    src->combined_in = dst;
+}
+
+static void usb_remove_packet_from_uncombined(USBPacket *dst, USBPacket *src)
+{
+    QTAILQ_REMOVE(&dst->uncombined_packets, src, queue);
+    usb_packet_set_state(src, USB_PACKET_COMPLETE);
+    qemu_iovec_destroy(&dst->iov);
+}
+
 static void __usb_packet_complete(USBDevice *dev, USBPacket *p)
 {
     USBEndpoint *ep = p->ep;
@@ -421,11 +438,133 @@ static void __usb_packet_complete(USBDevice *dev, USBPacket *p)
     if (p->result < 0) {
         ep->halted = true;
     }
+
     usb_packet_set_state(p, USB_PACKET_COMPLETE);
+
+    /* If this is a combined packet, split it */
+    if (!QTAILQ_EMPTY(&p->uncombined_packets)) {
+        USBPacket *uncombined, *next;
+        bool need_zero_packet = true;
+        bool free_p = true;
+
+        QTAILQ_FOREACH_SAFE(uncombined, &p->uncombined_packets, queue, next) {
+            if (p->result > 0) {
+                /* Distribute data over uncombined packets */
+                if (p->result >= uncombined->iov.size) {
+                    uncombined->result = uncombined->iov.size;
+                } else {
+                    if (p->short_not_ok) {
+                        ep->halted = true;
+                    }
+                    uncombined->result = p->result;
+                    need_zero_packet = false;
+                }
+                p->result -= uncombined->result;
+                usb_remove_packet_from_uncombined(p, uncombined);
+                dev->port->ops->complete(dev->port, uncombined);
+            } else if (need_zero_packet) {
+                /* Send terminating 0 or error status packet */
+                if (p->short_not_ok) {
+                    ep->halted = true;
+                }
+                uncombined->result = p->result;
+                usb_remove_packet_from_uncombined(p, uncombined);
+                dev->port->ops->complete(dev->port, uncombined);
+                need_zero_packet = false;
+            } else {
+                /* Remove any leftover packets from the queue */
+                dev->port->ops->remove_from_queue(dev->port, uncombined);
+                free_p = false; /* remove_from_queue calls usb_cancel_packet */
+            }
+        }
+        if (free_p) {
+            QTAILQ_REMOVE(&ep->queue, p, queue);
+            g_free(p);
+        }
+        return;
+    }
+
+    if (p->short_not_ok && (p->result < p->iov.size)) {
+        ep->halted = true;
+    }
     QTAILQ_REMOVE(&ep->queue, p, queue);
     dev->port->ops->complete(dev->port, p);
 }
 
+/*
+ * Large input transfers can get split into multiple input packets, this
+ * function recombines them, removing the short_not_ok checks which all but
+ * the last packet of such splits transfers have, thereby allowing input
+ * transfer pipelining (which we cannot do on short_not_ok transfers)
+ */
+void usb_ep_process_queue(USBEndpoint *ep)
+{
+    USBPacket *u, *n, *p, *next, *prev = NULL, *first = NULL;
+    USBPort *port = ep->dev->port;
+
+    if (!usb_ep_input_pipeline(ep)) {
+        return;
+    }
+
+    QTAILQ_FOREACH_SAFE(p, &ep->queue, queue, next) {
+        /* Empty the queue on a halt */
+        if (ep->halted) {
+            if (!QTAILQ_EMPTY(&p->uncombined_packets)) {
+                QTAILQ_FOREACH_SAFE(u, &p->uncombined_packets, queue, n) {
+                    port->ops->remove_from_queue(port, u);
+                }
+            } else {
+                port->ops->remove_from_queue(port, p);
+            }
+            continue;
+        }
+
+        /* Skip packets already submitted to the device */
+        if (p->state == USB_PACKET_ASYNC) {
+            prev = p;
+            continue;
+        }
+        usb_packet_check_state(p, USB_PACKET_QUEUED);
+
+        /*
+         * If the previous (combined) packet has the short_not_ok flag set
+         * stop, as we must not submit packets to the device after a transfer
+         * ending with short_not_ok packet.
+         */
+        if (prev && prev->short_not_ok) {
+            break;
+        }
+
+        if (first) {
+            if (QTAILQ_EMPTY(&first->uncombined_packets)) {
+                USBPacket *new_first = g_new0(USBPacket, 1);
+
+                usb_packet_init(new_first);
+                usb_packet_setup(new_first, first->pid, first->ep, first->id);
+                new_first->state = USB_PACKET_QUEUED;
+
+                usb_add_packet_to_uncombined(new_first, first);
+                QTAILQ_INSERT_BEFORE(p, new_first, queue);
+                first = new_first;
+            }
+            usb_add_packet_to_uncombined(first, p);
+            first->short_not_ok = p->short_not_ok;
+        } else {
+            first = p;
+        }
+
+        /* Is this packet the last one of a (combined) transfer? */
+        if ((p->iov.size % ep->max_packet_size) != 0 || !p->short_not_ok ||
+                p->int_req || next == NULL) {
+            int ret = usb_process_one(first);
+            assert(ret == USB_RET_ASYNC);
+            usb_packet_set_state(first, USB_PACKET_ASYNC);
+            prev = first;
+            first = NULL;
+        }
+    }
+}
+
 /* Notify the controller that an async packet is complete.  This should only
    be called for packets previously deferred by returning USB_RET_ASYNC from
    handle_packet. */
@@ -438,6 +577,11 @@ void usb_packet_complete(USBDevice *dev, USBPacket *p)
     assert(QTAILQ_FIRST(&ep->queue) == p);
     __usb_packet_complete(dev, p);
 
+    if (usb_ep_input_pipeline(ep)) {
+        usb_ep_process_queue(ep);
+        return;
+    }
+
     while (!ep->halted && !QTAILQ_EMPTY(&ep->queue)) {
         p = QTAILQ_FIRST(&ep->queue);
         if (p->state == USB_PACKET_ASYNC) {
@@ -459,19 +603,34 @@ void usb_packet_complete(USBDevice *dev, USBPacket *p)
    completed.  */
 void usb_cancel_packet(USBPacket * p)
 {
-    bool callback = (p->state == USB_PACKET_ASYNC);
+    USBPacket *free_me = NULL;
+    bool callback;
+
     assert(usb_packet_is_inflight(p));
+
+    /* Is this part of a combined packet? */
+    if (p->combined_in) {
+        usb_remove_packet_from_uncombined(p->combined_in, p);
+        if (!QTAILQ_EMPTY(&p->combined_in->uncombined_packets)) {
+            return;
+        }
+        free_me = p = p->combined_in;
+    }
+
+    callback = (p->state == USB_PACKET_ASYNC);
     usb_packet_set_state(p, USB_PACKET_CANCELED);
     QTAILQ_REMOVE(&p->ep->queue, p, queue);
     if (callback) {
         usb_device_cancel_packet(p->ep->dev, p);
     }
+    g_free(free_me);
 }
 
 
 void usb_packet_init(USBPacket *p)
 {
     qemu_iovec_init(&p->iov, 1);
+    QTAILQ_INIT(&p->uncombined_packets);
 }
 
 static const char *usb_packet_state_name(USBPacketState state)
@@ -531,6 +690,9 @@ void usb_packet_setup(USBPacket *p, int pid, USBEndpoint *ep, uint64_t id)
     p->ep = ep;
     p->result = 0;
     p->parameter = 0;
+    p->combined_in = NULL;
+    p->short_not_ok = false;
+    p->int_req = false;
     qemu_iovec_reset(&p->iov);
     usb_packet_set_state(p, USB_PACKET_SETUP);
 }
@@ -724,7 +886,13 @@ int usb_ep_get_max_packet_size(USBDevice *dev, int pid, int ep)
 void usb_ep_set_pipeline(USBDevice *dev, int pid, int ep, bool enabled)
 {
     struct USBEndpoint *uep = usb_ep_get(dev, pid, ep);
-    uep->pipeline = enabled;
+
+    if (pid == USB_TOKEN_IN) {
+        uep->pipeline =
+            enabled && uep->max_packet_size != 0 && dev->port->supports_queuing;
+    } else {
+        uep->pipeline = enabled;
+    }
 }
 
 USBPacket *usb_ep_find_packet_by_id(USBDevice *dev, int pid, int ep,
diff --git a/hw/usb/dev-hub.c b/hw/usb/dev-hub.c
index 8fd30df..d326281 100644
--- a/hw/usb/dev-hub.c
+++ b/hw/usb/dev-hub.c
@@ -222,6 +222,13 @@ static void usb_hub_complete(USBPort *port, USBPacket *packet)
     s->dev.port->ops->complete(s->dev.port, packet);
 }
 
+static void usb_hub_remove_from_queue(USBPort *port, USBPacket *packet)
+{
+    USBHubState *s = port->opaque;
+
+    s->dev.port->ops->remove_from_queue(s->dev.port, packet);
+}
+
 static USBDevice *usb_hub_find_device(USBDevice *dev, uint8_t addr)
 {
     USBHubState *s = DO_UPCAST(USBHubState, dev, dev);
@@ -512,6 +519,7 @@ static USBPortOps usb_hub_port_ops = {
     .child_detach = usb_hub_child_detach,
     .wakeup = usb_hub_wakeup,
     .complete = usb_hub_complete,
+    .remove_from_queue = usb_hub_remove_from_queue,
 };
 
 static int usb_hub_initfn(USBDevice *dev)
@@ -529,6 +537,7 @@ static int usb_hub_initfn(USBDevice *dev)
                           &port->port, s, i, &usb_hub_port_ops,
                           USB_SPEED_MASK_LOW | USB_SPEED_MASK_FULL);
         usb_port_location(&port->port, dev->port, i+1);
+        port->port.supports_queuing = dev->port->supports_queuing;
     }
     usb_hub_handle_reset(dev);
     return 0;
-- 
1.7.12.1

Re: [Qemu-devel] [PATCH 04/12] usb: Add support for input pipelining

[Gerd Hoffmann]

On 10/08/12 09:51, Hans de Goede wrote:
> Currently we effectively only do pipelining for output endpoints, since
> controllers will stop filling the queue for a packet with stop the queue
> on a short read semantics enabled.

> This causes large input transfers to get split into multiple packets, which
> comes with a serious performance penalty.
> 
> This patch makes it possible to do pipelining for input endpoints, by
> re-combining packets which belong to 1 transfer at the guest device-driver
> level into 1 large packet before sending them to the device, this is mostly
> useful as a significant performance improvement for redirection of real USB
> devices.

This splitting and recombining is too much magic in the core layer for
my taste.

I think the core work flow should stay as-is.  Instead we should add
more meta data to USBPacket (stop-queue-on-short-read bit, maybe
interrupt-on-complete too).  Continue queuing packets even with
stop-queue-on-short-read set.  Maybe add a callback to notify USBDevice
when the host controller is done filling the queue, so USBDevices can
process all queued packets in one go (bottom half would work too
though).  Then expect USBDevices to get things right based on the
USBPacket flags passed on (i.e. have host-linux use
USBFS_URB_BULK_CONTINUATION as needed).

This way usb-host and usbredir will see what is really going on instead
of having the usb core magically fixing up stuff for them.  I think this
will serve us better long-term.  Maybe you are seeing this "data *and*
stall" issue (patch 1) exactly because of all this combining & splitting
logic?

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 04/12] usb: Add support for input pipelining

[Hans de Goede]

Hi,

On 10/08/2012 02:50 PM, Gerd Hoffmann wrote:
> On 10/08/12 09:51, Hans de Goede wrote:
>> Currently we effectively only do pipelining for output endpoints, since
>> controllers will stop filling the queue for a packet with stop the queue
>> on a short read semantics enabled.
>
>> This causes large input transfers to get split into multiple packets, which
>> comes with a serious performance penalty.
>>
>> This patch makes it possible to do pipelining for input endpoints, by
>> re-combining packets which belong to 1 transfer at the guest device-driver
>> level into 1 large packet before sending them to the device, this is mostly
>> useful as a significant performance improvement for redirection of real USB
>> devices.
>
> This splitting and recombining is too much magic in the core layer for
> my taste.

I disagree, let me try to explain below.

> I think the core work flow should stay as-is.  Instead we should add
> more meta data to USBPacket (stop-queue-on-short-read bit, maybe
> interrupt-on-complete too).

We agree on the meta data :)

> Continue queuing packets even with
> stop-queue-on-short-read set.

This can only happen if we combine them first. We *must* ensure that
we do not read additional data after a short completion and add
that to a packet belonging to the previous short ended transfer, or
if we end up cancelling after the short completion, throwing away that
data, otherwise the guest and device can get out of sync. Which is exactly
what was happening with the uhci code, before I added the SPD tests there.

Another important reason for combining is that in most cases, we only
see short-not-ok flags because the device-driver in the guest has asked
for a single bulk in transfer which does not fit in a single td, so
it gets split, and to make sure that if it ends up short, the problem
of data from another part of the protocol (ie usb mass storage bulk only
transport data versus sense) ending up in tds belonging to the previous
transfer does not happen, the short-not-ok flag gets set on all
packets but the last one. Notice btw that the xhci controller has no
notion of short-not-ok, instead it simply allows for very large
bulk transfers to be submitted in one "atomic" entity.

So far to not cause this mixup of to which protocol phase in-data belongs,
we do not queue up packets to the device past a short-not-ok boundary,
but this means for serial <-> usb converters that instead of doing:

submit 1st 256 byte bulk in
submit 2nd 256 byte bulk in
<wait for completions>
<after a single transfer completion>
resubmit 256 byte bulk in

We do:
submit 1st 64/256 byte bulk in
<wait for completion>
<on full 64 byte completion>
submit 2nd 64/256 byte bulk in
<etc>

Meaning that for receiving 256 bytes we pay 4 times the round trip
time instead of 1 time, and since we do not submit further packets
after a short-not-ok packet, that we don't have a 2nd 256 byte
transfer queued up ready to receive more data while the 1st
one is being processed. Making reading from such a device highly
unreliable if there is any sort of load on the host.

The whole combining "magic" results in a single 256 byte packet, which
does not have its short-not-ok flag set, allowing true pipelining and
having another in transfer queued up to keep the device busy will the
1st one is being processed.

Re-combining the packets into a single transfer, is the *only* way to
ensure that a short completion cannot end up reading some data from
the next protocol phase.

One possible alternative to re-combining would be to pass the
short-not-ok flag along the chain all the way down into usbfs, but
that won't work since the USBFS_URB_SHORT_NOT_OK flag does not
work with XHCI controllers! There has even been discussion about
outright refusing packet submissions with that flag set in newer
Linux kernels, but that would cause regressions for apps which set
it when they don't really need it (and most don't). So since more
and more machines are using XHCI controllers, passing the
short-not-ok flag along is not a valid option.

So lets try to split this discussion into multiple questions:

1) Can we agree that re-combining input packets back into their original
transfers as done at the guest device driver level, is useful and given
the serial <-> usb converters case, even necessary to have ?


2) If we agree on 1), what is the right place to do them ?

To be able to properly combine packets into one larger transfer, an
uncombine them again later, some coordination is needed between the
entity doing the combining / uncombining and the hcd code:

2a) The combining entity needs to know when the hcd is done with
queuing up packets for an ep.

2b) When uncombining it is possible that less packets are actually
used / filled with data, then were combined into one transfer. When
this happens the hcd code will stop processing the queue after the
short packet, and wait for the guest to restart it. When the guest
restarts the queue it will be modified and the un-used packets will
be gone from it. Since the hcd code keeps its own queue, when
when uncombining the hcd code needs to be told to forget about
the unused packets.

Note that currently we have the same problem on a halt of the ep
queue already, and currently we expect the hcd to completely empty
the queue on a stall. I'm planning to change this code path over
to also let the core tell the hcd to forget about the packets it
had queued beyond the stall, as this seems something to do at the
core level rather then reproducing it in every hcd emulation
separately.


Given the necessary close interaction between the hcd code and
the combine / uncombine code + not wanting to reproduce the
same code in both host-linux.c and redirect.c I believe that
the usb core is the right place for this.

 > Maybe add a callback to notify USBDevice
> when the host controller is done filling the queue, so USBDevices can
> process all queued packets in one go (bottom half would work too
> though).

Changing the callback which I added for this to a bh is fine with me,
I think that both from a making clear what is going on when reading
the code, and from a having all the data hot in cache pov, the callback
is better though.

> Then expect USBDevices to get things right based on the
> USBPacket flags passed on (i.e. have host-linux use
> USBFS_URB_BULK_CONTINUATION as needed).

Note that the need for host-linux to use USBFS_URB_BULK_CONTINUATION +
SHORT_NOT_OK flags with older kernels, and to simply no longer split with
newer kernels, already exists! A single ehci td can span 20k, and we
split at 16k, so an input transfer ending short in the first 16k will
already cause the wrong thing to happen (the sense data of usb-ms gets read
as the next 4k), without input pipelining coming into play at all, and with
the xhci emulation we can already get much larger in transfers...

The reason why I disable input pipelining for host-linux for now in my
patchset is because it will make the problem worse, not because the
problem is new.

For the full details on all the magic needed to correctly handle
larger input transfers on usbdevfs with all possible different
kernel versions, please see:
https://github.com/libusbx/libusbx/commit/ede02ba91920f9be787a7f3cd006c5a4b92b5eab

Or just switch to libusb ...

> This way usb-host and usbredir will see what is really going on instead
> of having the usb core magically fixing up stuff for them.

I believe this is based on you thinking simply passing along a short-not-ok
flag all the way down into usbfs will be enough, but it definitely is not
enough, since the XHCI hardware has no notion of short-not-ok, the only
way to do input pipelinig with an XHCI attached device is to re-combine
the split packets into a single large input transfer. And the proper
place for such combining code is in the core IMHO.

> I think this
> will serve us better long-term.  Maybe you are seeing this "data *and*
> stall" issue (patch 1) exactly because of all this combining & splitting
> logic?

I'm not seeing the data and stall condition in practice at all, but while
designing all this I noticed that this can happen. a single ehci td
can be a multiple of the endpoints maxPacketSize and then the endpoint
can respond with maxPacketSize data, maxPacketSize data, stall at which
point we will have both data and a stall condition in one.

And the Linux ehci + usbfs + libusb + usbredir code all handle passing
along both data and a stall as a result for one packet / transfer,
so this is something which eventually we will need to support in
the qemu usb core too. For now I decided on the quick fix in usbredir
to cover the eventuality of this actually happening and added a
warning to see if this actually ever happens (which it seems not to
in my testing done sofar).

Regards,

Hans

Re: [Qemu-devel] [PATCH 04/12] usb: Add support for input pipelining

[Gerd Hoffmann]

  Hi,

> One possible alternative to re-combining would be to pass the
> short-not-ok flag along the chain all the way down into usbfs,

Yes, this is what I have in mind.

> but
> that won't work since the USBFS_URB_SHORT_NOT_OK flag does not
> work with XHCI controllers!

Oops.

> So lets try to split this discussion into multiple questions:
> 
> 1) Can we agree that re-combining input packets back into their original
> transfers as done at the guest device driver level, is useful and given
> the serial <-> usb converters case, even necessary to have ?

Given that the USBFS_URB_SHORT_NOT_OK doesn't work there is no way
around that I guess.

> 2) If we agree on 1), what is the right place to do them ?
> 
> To be able to properly combine packets into one larger transfer, an
> uncombine them again later, some coordination is needed between the
> entity doing the combining / uncombining and the hcd code:
> 
> 2a) The combining entity needs to know when the hcd is done with
> queuing up packets for an ep.

Yes.  We can add a USBDeviceClass method for that.

> 2b) When uncombining it is possible that less packets are actually
> used / filled with data, then were combined into one transfer. When
> this happens the hcd code will stop processing the queue after the
> short packet, and wait for the guest to restart it. When the guest
> restarts the queue it will be modified and the un-used packets will
> be gone from it. Since the hcd code keeps its own queue, when
> when uncombining the hcd code needs to be told to forget about
> the unused packets.

Yes.  We can add a new USBPortOps for that, or reuse
USBPortOps->complete with USBPacket->result == USB_RET_NOT_USED.

> Note that currently we have the same problem on a halt of the ep
> queue already, and currently we expect the hcd to completely empty
> the queue on a stall. I'm planning to change this code path over
> to also let the core tell the hcd to forget about the packets it
> had queued beyond the stall, as this seems something to do at the
> core level rather then reproducing it in every hcd emulation
> separately.

The core can and should do that for packets it owns (USBPacketState ==
USB_PACKET_QUEUED) because they are not (yet) passed to the USBDevice.

Packets owned by USBDevice (USBPacketState == USB_PACKET_ASYNC) must be
handled by the USBDevice itself.

> Given the necessary close interaction between the hcd code and
> the combine / uncombine code + not wanting to reproduce the
> same code in both host-linux.c and redirect.c I believe that
> the usb core is the right place for this.

I think the combining should happen just before submitting the transfer
to usbfs, in usb-host.  I'd like usb-host see what is really going on,
and the usb core not hiding this by magically creating jumbo packets.

Likewise I think usbredir should do the combining on the *server* side,
not in the qemu redir code.

>> Maybe add a callback to notify USBDevice
>> when the host controller is done filling the queue, so USBDevices can
>> process all queued packets in one go (bottom half would work too
>> though).
> 
> Changing the callback which I added for this to a bh is fine with me,
> I think that both from a making clear what is going on when reading
> the code, and from a having all the data hot in cache pov, the callback
> is better though.

Yes, callback is probably better, that is just a little minor detail though.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 04/12] usb: Add support for input pipelining

[Hans de Goede]

Hi,

On 10/09/2012 11:21 AM, Gerd Hoffmann wrote:

<snip>

>> So lets try to split this discussion into multiple questions:
>>
>> 1) Can we agree that re-combining input packets back into their original
>> transfers as done at the guest device driver level, is useful and given
>> the serial <-> usb converters case, even necessary to have ?
>
> Given that the USBFS_URB_SHORT_NOT_OK doesn't work there is no way
> around that I guess.
>

Yes, short-not-ok not stopping the queue on a short packet is an
unpleasant surprise, it had all 3 of Alan Stern, Sarah Sharp and me
surprised quite a bit while working on fixing the problem some
people were seeing with redirecting some USB mass storage
devices, when those devices were plugged into an XHCI port.

>> 2) If we agree on 1), what is the right place to do them ?
>>
>> To be able to properly combine packets into one larger transfer, an
>> uncombine them again later, some coordination is needed between the
>> entity doing the combining / uncombining and the hcd code:
>>
>> 2a) The combining entity needs to know when the hcd is done with
>> queuing up packets for an ep.
>
> Yes.  We can add a USBDeviceClass method for that.
>

Ok, this assumes that the combining / uncombining gets moved down
into the usb-device level code. Which clearly has your preference,
but I don't completely agree with, so lets have that discussion first,
once we've decided what to do there, details like this will likely
sort out themselves.

>> 2b) When uncombining it is possible that less packets are actually
>> used / filled with data, then were combined into one transfer. When
>> this happens the hcd code will stop processing the queue after the
>> short packet, and wait for the guest to restart it. When the guest
>> restarts the queue it will be modified and the un-used packets will
>> be gone from it. Since the hcd code keeps its own queue, when
>> when uncombining the hcd code needs to be told to forget about
>> the unused packets.
>
> Yes.  We can add a new USBPortOps for that, or reuse
> USBPortOps->complete with USBPacket->result == USB_RET_NOT_USED.

I had the same thought about using a new result for this, thinking
more about this using a new result code is better, so I'll go go
that.

>> Note that currently we have the same problem on a halt of the ep
>> queue already, and currently we expect the hcd to completely empty
>> the queue on a stall. I'm planning to change this code path over
>> to also let the core tell the hcd to forget about the packets it
>> had queued beyond the stall, as this seems something to do at the
>> core level rather then reproducing it in every hcd emulation
>> separately.
>
> The core can and should do that for packets it owns (USBPacketState ==
> USB_PACKET_QUEUED) because they are not (yet) passed to the USBDevice.
>
> Packets owned by USBDevice (USBPacketState == USB_PACKET_ASYNC) must be
> handled by the USBDevice itself.

Getting offtopic a bit here, but this not how we currently handle
things, currently the hcd code cancels packets after a queue halt,
rather then the device-code itself. But this is another discussion,
so lets save this for later.

>> Given the necessary close interaction between the hcd code and
>> the combine / uncombine code + not wanting to reproduce the
>> same code in both host-linux.c and redirect.c I believe that
>> the usb core is the right place for this.
>
> I think the combining should happen just before submitting the transfer
> to usbfs, in usb-host.  I'd like usb-host see what is really going on,
> and the usb core not hiding this by magically creating jumbo packets.

Note that with my current code linux-host can see it is a combined packet
if it wants to already. But this all really boils down to the next bit:

> Likewise I think usbredir should do the combining on the *server* side,
> not in the qemu redir code.

I think that moving the combining to the server / usb-redir-host side is
*not* a good idea. There is nothing to be gained by doing so, and a lot to
loose:
1) The usb-redir-host has a lot less info to work with, as it does not have
access to the actual tds which lead to the packets being send. The protocol
can be extended to send what we need to do the combining now, but what if
later on it turns out we need more meta data for some corner case ...
2) Doing the combining at the usb-redir-host leads to more packets being send
and received, increasing the protocol overhead
3) Doing the combining at the usb-redir-host means that instead of a single
packet being received back, multiple will be send back, which may not all
be received in one go, causing multiple vm-exits rather then just one.
4) The combining/uncombining logic will be the same for usb-redir or
linux-host redirection, implementing this all twice and then keeping it
in sync is not a good way to spend our time.

Notice that arguments 2 and 3 could be made for combining output packets
too, but what we do their now is nice and KISS, and the possible speed
improvement is not worth the complication IMHO.

So again lets try to split the discussion in answering multiple questions:

1) I believe it is better, and would greatly prefer to, do the combining
on the qemu side rather then on the usb-redir-host side, do you agree?

2) If you agree with 1, then I assume you agree we will want to share
the combining code between host-linux.c (or host-* for that matter) and
redirect.c ?

3) If you agree with 2, then all we need is a place for the shared logic
to live, we could put it in a new file called input-pipeline.c ?

4) Since you would like to keep the core clean, I'm fine with moving the
calling of the combining-functions into the usb-redir device code
(and the same calls can then later be added to host-linux.c).

Regards,

Hans

Re: [Qemu-devel] [PATCH 04/12] usb: Add support for input pipelining

[Gerd Hoffmann]

  Hi,

>>> 2a) The combining entity needs to know when the hcd is done with
>>> queuing up packets for an ep.
>>
>> Yes.  We can add a USBDeviceClass method for that.
> 
> Ok, this assumes that the combining / uncombining gets moved down
> into the usb-device level code. Which clearly has your preference,
> but I don't completely agree with, so lets have that discussion first,
> once we've decided what to do there, details like this will likely
> sort out themselves.

Yes.

>> The core can and should do that for packets it owns (USBPacketState ==
>> USB_PACKET_QUEUED) because they are not (yet) passed to the USBDevice.
>>
>> Packets owned by USBDevice (USBPacketState == USB_PACKET_ASYNC) must be
>> handled by the USBDevice itself.
> 
> Getting offtopic a bit here, but this not how we currently handle
> things, currently the hcd code cancels packets after a queue halt,

Ah, right.  Well, that should continue to work.  USB_RET_NOT_USED would
make the hcd code just free the packet, and anything not-yet freed will
be zapped by the queue halt handling.

> Notice that arguments 2 and 3 could be made for combining output packets
> too, but what we do their now is nice and KISS, and the possible speed
> improvement is not worth the complication IMHO.

Handling in/out eps the same way has its merits too (code sharing).

> So again lets try to split the discussion in answering multiple questions:
> 
> 1) I believe it is better, and would greatly prefer to, do the combining
> on the qemu side rather then on the usb-redir-host side, do you agree?

Your call, you know the bits much better than I do.
Your arguments make sense.

> 2) If you agree with 1, then I assume you agree we will want to share
> the combining code between host-linux.c (or host-* for that matter) and
> redirect.c ?

I'm not sure there is that much to share.

A helper function which takes a USBEndpoint and returns an iovec for all
USBPackets lined up there would probably be useful.  Likewise for one
for completing the packets (takes xfer length + status, then fill
USBPacket->result & call usb_packet_complete for each packet).

But beyond that?

> 3) If you agree with 2, then all we need is a place for the shared logic
> to live, we could put it in a new file called input-pipeline.c ?

Just stick the helpers into hw/usb/core.c?

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 04/12] usb: Add support for input pipelining

[Hans de Goede]

Hi,

On 10/09/2012 03:31 PM, Gerd Hoffmann wrote:

<snip>

>>> The core can and should do that for packets it owns (USBPacketState ==
>>> USB_PACKET_QUEUED) because they are not (yet) passed to the USBDevice.
>>>
>>> Packets owned by USBDevice (USBPacketState == USB_PACKET_ASYNC) must be
>>> handled by the USBDevice itself.
>>
>> Getting offtopic a bit here, but this not how we currently handle
>> things, currently the hcd code cancels packets after a queue halt,
>
> Ah, right.  Well, that should continue to work.  USB_RET_NOT_USED would
> make the hcd code just free the packet, and anything not-yet freed will
> be zapped by the queue halt handling.
>

Right.

<snip>

>> 2) If you agree with 1, then I assume you agree we will want to share
>> the combining code between host-linux.c (or host-* for that matter) and
>> redirect.c ?
>
> I'm not sure there is that much to share.
>

It is not much, but you rightly called it "magic" before, as it is somewhat
tricky code, so better to write (and debug) it once :)

> A helper function which takes a USBEndpoint and returns an iovec for all
> USBPackets lined up there would probably be useful.  Likewise for one
> for completing the packets (takes xfer length + status, then fill
> USBPacket->result & call usb_packet_complete for each packet).

That is more or less what I had in mind yes :)

> But beyond that?
>
>> 3) If you agree with 2, then all we need is a place for the shared logic
>> to live, we could put it in a new file called input-pipeline.c ?
>
> Just stick the helpers into hw/usb/core.c?

I think core.c is getting a bit crowded, so I'll go with a new file for
the next revision, if you don't like that moving the functions someplace
else is easy :)

I'll start working on a new reworked version of the patchset which hopefully
will be more to your liking :)

Regards,

Hans

[Qemu-devel] [PATCH 05/12] uhci: Properly unmap packets on cancel / invalid pid

[Hans de Goede]

Packets with an invalid pid, or which were cancelled have
usb_packet_map() called on them on init, but not usb_packet_unmap()
before being freed.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/hcd-uhci.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index cdc8bc3..70b76e5 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -236,6 +236,7 @@ static void uhci_async_cancel(UHCIAsync *async)
     trace_usb_uhci_packet_cancel(async->queue->token, async->td, async->done);
     if (!async->done)
         usb_cancel_packet(&async->packet);
+    usb_packet_unmap(&async->packet, &async->sgl);
     uhci_async_free(async);
 }
 
@@ -879,6 +880,7 @@ static int uhci_handle_td(UHCIState *s, uint32_t addr, UHCI_TD *td,
 
     default:
         /* invalid pid : frame interrupted */
+        usb_packet_unmap(&async->packet, &async->sgl);
         uhci_async_free(async);
         s->status |= UHCI_STS_HCPERR;
         uhci_update_irq(s);
-- 
1.7.12.1

[Qemu-devel] [PATCH 06/12] uhci: Move checks to continue queuing to uhci_fill_queue()

[Hans de Goede]

Rather then having a special check to start queuing after the first packet,
and then another check for the other packets in uhci_fill_queue(), simply
check the previous packet beforehand in uhci_fill_queue()

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/hcd-uhci.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index 70b76e5..124d43a 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -983,7 +983,8 @@ static void uhci_fill_queue(UHCIState *s, UHCI_TD *td)
     UHCI_TD ptd;
     int ret;
 
-    while (is_valid(plink)) {
+    ptd.ctrl = td->ctrl;
+    while (is_valid(plink) && !(ptd.ctrl & TD_CTRL_SPD)) {
         pci_dma_read(&s->dev, plink & ~0xf, &ptd, sizeof(ptd));
         le32_to_cpus(&ptd.link);
         le32_to_cpus(&ptd.ctrl);
@@ -1002,9 +1003,6 @@ static void uhci_fill_queue(UHCIState *s, UHCI_TD *td)
         }
         assert(ret == TD_RESULT_ASYNC_START);
         assert(int_mask == 0);
-        if (ptd.ctrl & TD_CTRL_SPD) {
-            break;
-        }
         plink = ptd.link;
     }
 }
@@ -1102,9 +1100,7 @@ static void uhci_process_frame(UHCIState *s)
 
         case TD_RESULT_ASYNC_START:
             trace_usb_uhci_td_async(curr_qh & ~0xf, link & ~0xf);
-            if (is_valid(td.link) && !(td.ctrl & TD_CTRL_SPD)) {
-                uhci_fill_queue(s, &td);
-            }
+            uhci_fill_queue(s, &td);
             link = curr_qh ? qh.link : td.link;
             continue;
 
-- 
1.7.12.1

[Qemu-devel] [PATCH 07/12] uhci: Add support for input queuing

[Hans de Goede]

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/hcd-uhci.c | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index 124d43a..63f2161 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -678,6 +678,13 @@ static void uhci_wakeup(USBPort *port1)
     }
 }
 
+static void uhci_remove_from_queue(USBPort *port, USBPacket *p)
+{
+    UHCIAsync *async = DO_UPCAST(UHCIAsync, packet, p);
+    uhci_async_unlink(async);
+    uhci_async_cancel(async);
+}
+
 static USBDevice *uhci_find_device(UHCIState *s, uint8_t addr)
 {
     USBDevice *dev;
@@ -818,7 +825,8 @@ out:
 }
 
 static int uhci_handle_td(UHCIState *s, uint32_t addr, UHCI_TD *td,
-                          uint32_t *int_mask, bool queuing)
+                          uint32_t *int_mask, bool queuing,
+                          struct USBEndpoint **ep_ret)
 {
     UHCIAsync *async;
     int len = 0, max_len;
@@ -862,6 +870,9 @@ static int uhci_handle_td(UHCIState *s, uint32_t addr, UHCI_TD *td,
 
     dev = uhci_find_device(s, (td->token >> 8) & 0x7f);
     ep = usb_ep_get(dev, pid, (td->token >> 15) & 0xf);
+    if (ep_ret) {
+        *ep_ret = ep;
+    }
     usb_packet_setup(&async->packet, pid, ep, addr);
     qemu_sglist_add(&async->sgl, td->buffer, max_len);
     usb_packet_map(&async->packet, &async->sgl);
@@ -875,6 +886,8 @@ static int uhci_handle_td(UHCIState *s, uint32_t addr, UHCI_TD *td,
         break;
 
     case USB_TOKEN_IN:
+        async->packet.short_not_ok = ((td->ctrl & TD_CTRL_SPD) != 0);
+        async->packet.int_req = ((td->ctrl & TD_CTRL_IOC) != 0);
         len = usb_handle_packet(dev, &async->packet);
         break;
 
@@ -975,7 +988,7 @@ static int qhdb_insert(QhDb *db, uint32_t addr)
     return 0;
 }
 
-static void uhci_fill_queue(UHCIState *s, UHCI_TD *td)
+static void uhci_fill_queue(UHCIState *s, UHCI_TD *td, struct USBEndpoint *ep)
 {
     uint32_t int_mask = 0;
     uint32_t plink = td->link;
@@ -984,7 +997,8 @@ static void uhci_fill_queue(UHCIState *s, UHCI_TD *td)
     int ret;
 
     ptd.ctrl = td->ctrl;
-    while (is_valid(plink) && !(ptd.ctrl & TD_CTRL_SPD)) {
+    while (is_valid(plink) &&
+            (usb_ep_input_pipeline(ep) || !(ptd.ctrl & TD_CTRL_SPD))) {
         pci_dma_read(&s->dev, plink & ~0xf, &ptd, sizeof(ptd));
         le32_to_cpus(&ptd.link);
         le32_to_cpus(&ptd.ctrl);
@@ -997,7 +1011,7 @@ static void uhci_fill_queue(UHCIState *s, UHCI_TD *td)
             break;
         }
         trace_usb_uhci_td_queue(plink & ~0xf, ptd.ctrl, ptd.token);
-        ret = uhci_handle_td(s, plink, &ptd, &int_mask, true);
+        ret = uhci_handle_td(s, plink, &ptd, &int_mask, true, NULL);
         if (ret == TD_RESULT_ASYNC_CONT) {
             break;
         }
@@ -1005,12 +1019,14 @@ static void uhci_fill_queue(UHCIState *s, UHCI_TD *td)
         assert(int_mask == 0);
         plink = ptd.link;
     }
+    usb_ep_process_queue(ep);
 }
 
 static void uhci_process_frame(UHCIState *s)
 {
     uint32_t frame_addr, link, old_td_ctrl, val, int_mask;
     uint32_t curr_qh, td_count = 0;
+    struct USBEndpoint *curr_ep;
     int cnt, ret;
     UHCI_TD td;
     UHCI_QH qh;
@@ -1081,7 +1097,7 @@ static void uhci_process_frame(UHCIState *s)
         trace_usb_uhci_td_load(curr_qh & ~0xf, link & ~0xf, td.ctrl, td.token);
 
         old_td_ctrl = td.ctrl;
-        ret = uhci_handle_td(s, link, &td, &int_mask, false);
+        ret = uhci_handle_td(s, link, &td, &int_mask, false, &curr_ep);
         if (old_td_ctrl != td.ctrl) {
             /* update the status bits of the TD */
             val = cpu_to_le32(td.ctrl);
@@ -1100,7 +1116,7 @@ static void uhci_process_frame(UHCIState *s)
 
         case TD_RESULT_ASYNC_START:
             trace_usb_uhci_td_async(curr_qh & ~0xf, link & ~0xf);
-            uhci_fill_queue(s, &td);
+            uhci_fill_queue(s, &td, curr_ep);
             link = curr_qh ? qh.link : td.link;
             continue;
 
@@ -1202,6 +1218,7 @@ static USBPortOps uhci_port_ops = {
     .child_detach = uhci_child_detach,
     .wakeup = uhci_wakeup,
     .complete = uhci_async_complete,
+    .remove_from_queue = uhci_remove_from_queue,
 };
 
 static USBBusOps uhci_bus_ops = {
@@ -1238,6 +1255,7 @@ static int usb_uhci_common_initfn(PCIDevice *dev)
         USBPort *ports[NB_PORTS];
         for(i = 0; i < NB_PORTS; i++) {
             ports[i] = &s->ports[i].port;
+            ports[i]->supports_queuing = true;
         }
         if (usb_register_companion(s->masterbus, ports, NB_PORTS,
                 s->firstport, s, &uhci_port_ops,
@@ -1247,6 +1265,7 @@ static int usb_uhci_common_initfn(PCIDevice *dev)
     } else {
         usb_bus_new(&s->bus, &uhci_bus_ops, &s->dev.qdev);
         for (i = 0; i < NB_PORTS; i++) {
+            s->ports[i].port.supports_queuing = true;
             usb_register_port(&s->bus, &s->ports[i].port, s, i, &uhci_port_ops,
                               USB_SPEED_MASK_LOW | USB_SPEED_MASK_FULL);
         }
-- 
1.7.12.1

[Qemu-devel] [PATCH 08/12] ehci: Get rid of packet tbytes field

[Hans de Goede]

This field is used in some places to track the tbytes field of the token, but
in other places the field is used directly, use it directly everywhere for
consistency.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/hcd-ehci.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 3acd881a..6945992 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -362,7 +362,6 @@ struct EHCIPacket {
     USBPacket packet;
     QEMUSGList sgl;
     int pid;
-    uint32_t tbytes;
     enum async_state async;
     int usb_status;
 };
@@ -1505,15 +1504,16 @@ static void ehci_execute_complete(EHCIQueue *q)
         }
     } else {
         // TODO check 4.12 for splits
+        uint32_t tbytes = get_field(q->qh.token, QTD_TOKEN_TBYTES);
 
-        if (p->tbytes && p->pid == USB_TOKEN_IN) {
-            p->tbytes -= p->usb_status;
+        if (tbytes && p->pid == USB_TOKEN_IN) {
+            tbytes -= p->usb_status;
         } else {
-            p->tbytes = 0;
+            tbytes = 0;
         }
 
-        DPRINTF("updating tbytes to %d\n", p->tbytes);
-        set_field(&q->qh.token, p->tbytes, QTD_TOKEN_TBYTES);
+        DPRINTF("updating tbytes to %d\n", tbytes);
+        set_field(&q->qh.token, tbytes, QTD_TOKEN_TBYTES);
     }
     ehci_finish_transfer(q, p->usb_status);
     usb_packet_unmap(&p->packet, &p->sgl);
@@ -1544,8 +1544,7 @@ static int ehci_execute(EHCIPacket *p, const char *action)
         return USB_RET_PROCERR;
     }
 
-    p->tbytes = (p->qtd.token & QTD_TOKEN_TBYTES_MASK) >> QTD_TOKEN_TBYTES_SH;
-    if (p->tbytes > BUFF_SIZE) {
+    if (get_field(p->qtd.token, QTD_TOKEN_TBYTES) > BUFF_SIZE) {
         ehci_trace_guest_bug(p->queue->ehci,
                              "guest requested more bytes than allowed");
         return USB_RET_PROCERR;
@@ -1582,10 +1581,9 @@ static int ehci_execute(EHCIPacket *p, const char *action)
 
     trace_usb_ehci_packet_action(p->queue, p, action);
     ret = usb_handle_packet(p->queue->dev, &p->packet);
-    DPRINTF("submit: qh %x next %x qtd %x pid %x len %zd "
-            "(total %d) endp %x ret %d\n",
+    DPRINTF("submit: qh %x next %x qtd %x pid %x len %zd endp %x ret %d\n",
             q->qhaddr, q->qh.next, q->qtdaddr, q->pid,
-            q->packet.iov.size, q->tbytes, endp, ret);
+            q->packet.iov.size, endp, ret);
 
     if (ret > BUFF_SIZE) {
         fprintf(stderr, "ret from usb_handle_packet > BUFF_SIZE\n");
-- 
1.7.12.1

[Qemu-devel] [PATCH 09/12] ehci: Set int flag on a short input packet

[Hans de Goede]

According to 4.15.1.2 an interrupt must be raised when a short packet
is received. If we don't do this it may take a significant time for
the guest to notice a short trasnfer has completed, since only the last td
will have its IOC flag set, and a short transfer may complete in an earlier
packet.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/hcd-ehci.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 6945992..d9d4918 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -1508,6 +1508,10 @@ static void ehci_execute_complete(EHCIQueue *q)
 
         if (tbytes && p->pid == USB_TOKEN_IN) {
             tbytes -= p->usb_status;
+            if (tbytes) {
+                /* 4.15.1.2 must raise int on a short input packet */
+                ehci_raise_irq(q->ehci, USBSTS_INT);
+            }
         } else {
             tbytes = 0;
         }
-- 
1.7.12.1

[Qemu-devel] [PATCH 10/12] ehci: Add support for input queuing

[Hans de Goede]

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/hcd-ehci.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index d9d4918..444db15 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -353,13 +353,13 @@ enum async_state {
 };
 
 struct EHCIPacket {
+    USBPacket packet;
     EHCIQueue *queue;
     QTAILQ_ENTRY(EHCIPacket) next;
 
     EHCIqtd qtd;           /* copy of current QTD (being worked on) */
     uint32_t qtdaddr;      /* address QTD read from                 */
 
-    USBPacket packet;
     QEMUSGList sgl;
     int pid;
     enum async_state async;
@@ -990,6 +990,21 @@ static void ehci_wakeup(USBPort *port)
     qemu_bh_schedule(s->async_bh);
 }
 
+static void ehci_remove_from_queue(USBPort *port, USBPacket *usb_p)
+{
+    EHCIState *s = port->opaque;
+    uint32_t portsc = s->portsc[port->index];
+    EHCIPacket *p = DO_UPCAST(EHCIPacket, packet, usb_p);
+
+    if (portsc & PORTSC_POWNER) {
+        USBPort *companion = s->companion_ports[port->index];
+        companion->ops->remove_from_queue(companion, usb_p);
+        return;
+    }
+
+    ehci_free_packet(p);
+}
+
 static int ehci_register_companion(USBBus *bus, USBPort *ports[],
                                    uint32_t portcount, uint32_t firstport)
 {
@@ -1021,6 +1036,8 @@ static int ehci_register_companion(USBBus *bus, USBPort *ports[],
         s->companion_ports[firstport + i] = ports[i];
         s->ports[firstport + i].speedmask |=
             USB_SPEED_MASK_LOW | USB_SPEED_MASK_FULL;
+        /* Only claim to support queuing if our companion does too */
+        s->ports[firstport + i].supports_queuing = ports[i]->supports_queuing;
         /* Ensure devs attached before the initial reset go to the companion */
         s->portsc[firstport + i] = PORTSC_POWNER;
     }
@@ -1580,6 +1597,10 @@ static int ehci_execute(EHCIPacket *p, const char *action)
 
         usb_packet_setup(&p->packet, p->pid, ep, p->qtdaddr);
         usb_packet_map(&p->packet, &p->sgl);
+        if (p->pid == USB_TOKEN_IN && NLPTR_TBIT(p->qtd.altnext) == 0) {
+            p->packet.short_not_ok = true;
+        }
+        p->packet.int_req = ((p->qtd.token & QTD_TOKEN_IOC) != 0);
         p->async = EHCI_ASYNC_INITIALIZED;
     }
 
@@ -2071,7 +2092,8 @@ static int ehci_fill_queue(EHCIPacket *p)
     uint32_t qtdaddr;
 
     for (;;) {
-        if (NLPTR_TBIT(qtd.altnext) == 0) {
+        if (!usb_ep_input_pipeline(p->packet.ep) &&
+                NLPTR_TBIT(qtd.altnext) == 0) {
             break;
         }
         if (NLPTR_TBIT(qtd.next) != 0) {
@@ -2094,6 +2116,9 @@ static int ehci_fill_queue(EHCIPacket *p)
         assert(p->usb_status == USB_RET_ASYNC);
         p->async = EHCI_ASYNC_INFLIGHT;
     }
+    if (p->usb_status != USB_RET_PROCERR) {
+        usb_ep_process_queue(p->packet.ep);
+    }
     return p->usb_status;
 }
 
@@ -2548,6 +2573,7 @@ static USBPortOps ehci_port_ops = {
     .child_detach = ehci_child_detach,
     .wakeup = ehci_wakeup,
     .complete = ehci_async_complete_packet,
+    .remove_from_queue = ehci_remove_from_queue,
 };
 
 static USBBusOps ehci_bus_ops = {
@@ -2740,6 +2766,7 @@ static int usb_ehci_initfn(PCIDevice *dev)
         usb_register_port(&s->bus, &s->ports[i], s, i, &ehci_port_ops,
                           USB_SPEED_MASK_HIGH);
         s->ports[i].dev = 0;
+        s->ports[i].supports_queuing = true;
     }
 
     s->frame_timer = qemu_new_timer_ns(vm_clock, ehci_frame_timer, s);
-- 
1.7.12.1

[Qemu-devel] [PATCH 11/12] ehci: Improve latency of interrupt delivery and async schedule scanning

[Hans de Goede]

While doing various performance tests of reading from USB mass storage devices
I noticed the following::
1) When an async handled packet completes, we don't immediately report an
   interrupt to the guest, instead we wait for the frame-timer to run and
   report it from there
2) If 1) has been fixed and an async handled packet takes a while to complete,
   then async_stepdown will become a high value, which means that there
   will be a large latency before any new packets queued by the guest in
   response to the interrupt get seen

1) was done deliberately as part of commit f0ad01f92:
http://www.kraxel.org/cgit/qemu/commit/?h=usb.57&id=f0ad01f92ca02eee7cadbfd225c5de753ebd5fce

Since setting reporting the interrupt immediately on async packet completion
was causing issues with Linux guests, I believe this recently fixed Linux
bug explains why this is happening:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=361aabf395e4a23cf554cf4ec0c0c6963b8beb01

Note that we can *not* count on this fix being present in all Linux guests!

I was hoping that the recently added support for Interrupt Threshold Control
would fix the issues with Linux guests, but adding a simple ehci_commit_irq()
call to ehci_async_bh() still caused problems with Linux guests.

The problem is, that when doing ehci_commit_irq() from ehci_async_bh(),
the "old" frindex value is used to calculate usbsts_frindex, and when
the frame-timer then runs possibly very shortly after ehci_async_bh(),
it increases the frame-timer, and thus any interrupts raised from that
frame-timer run, will also get reported to the guest immediately, rather
then being delayed to the next frame-timer run.

Luckily the solution for this is simple, this means that we need to
increase frindex before calling ehci_commit_irq() from ehci_async_bh(),
which in the end boils down to simple calling ehci_frame_timer() instead
of ehci_async_bh() from the bh.

This may seem like it causes a lot of extra work to be done, but this
is not true. Any work done from the frame-timer processing the periodic
schedule is work which then does not need to be done the next time the
frame timer runs, also the frame-timer will re-arm itself at (possibly)
a later time then it was armed for saving a vmexit at that time.

As an additional advantage moving to simply calling the frame-timer also
fixes 2) as the packet completion will have set async_stepdown to 0, and
the re-arming of the timer with an async_stepdown of 0 ensures that any
newly queued up packets get seen in a reasonable amount of time.

This improves the speed (MB/s) of a Linux guest reading from a USB mass
storage device by a factor of 1.5 - 1.7 with input pipelining disabled,
and by a factor of 1.8 with input pipelining enabled.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/hcd-ehci.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 444db15..fb3547d 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -1261,7 +1261,7 @@ static void ehci_opreg_write(void *ptr, target_phys_addr_t addr,
             s->usbcmd = val; /* Set usbcmd for ehci_update_halt() */
             ehci_update_halt(s);
             s->async_stepdown = 0;
-            qemu_mod_timer(s->frame_timer, qemu_get_clock_ns(vm_clock));
+            qemu_bh_schedule(s->async_bh);
         }
         break;
 
@@ -2534,12 +2534,6 @@ static void ehci_frame_timer(void *opaque)
     }
 }
 
-static void ehci_async_bh(void *opaque)
-{
-    EHCIState *ehci = opaque;
-    ehci_advance_async_state(ehci);
-}
-
 static const MemoryRegionOps ehci_mmio_caps_ops = {
     .read = ehci_caps_read,
     .valid.min_access_size = 1,
@@ -2770,7 +2764,7 @@ static int usb_ehci_initfn(PCIDevice *dev)
     }
 
     s->frame_timer = qemu_new_timer_ns(vm_clock, ehci_frame_timer, s);
-    s->async_bh = qemu_bh_new(ehci_async_bh, s);
+    s->async_bh = qemu_bh_new(ehci_frame_timer, s);
     QTAILQ_INIT(&s->aqueues);
     QTAILQ_INIT(&s->pqueues);
     usb_packet_init(&s->ipacket);
-- 
1.7.12.1

[Qemu-devel] [PATCH 12/12] ehci: Speed up the timer of raising int from the async schedule

[Hans de Goede]

Often the guest will queue up new packets in response to a packet, in the
async schedule with its IOC flag set, completing. By speeding up the
frame-timer, we notice these new packets earlier. This increases the
speed (MB/s) of a Linux guest reading from a USB mass storage device by a
factor of 1.15 on top of the "Improve latency of interrupt delivery"
speed-ups, both with and without input pipelining enabled.

I've not tested the speed-up of this patch without the
"Improve latency of interrupt delivery" patch.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/hcd-ehci.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index fb3547d..5056618 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -443,6 +443,7 @@ struct EHCIState {
 
     uint64_t last_run_ns;
     uint32_t async_stepdown;
+    bool int_req_by_async;
 };
 
 #define SET_LAST_RUN_CLOCK(s) \
@@ -1546,6 +1547,9 @@ static void ehci_execute_complete(EHCIQueue *q)
 
     if (q->qh.token & QTD_TOKEN_IOC) {
         ehci_raise_irq(q->ehci, USBSTS_INT);
+        if (q->async) {
+            q->ehci->int_req_by_async = true;
+        }
     }
 }
 
@@ -2528,8 +2532,15 @@ static void ehci_frame_timer(void *opaque)
     }
 
     if (need_timer) {
-        expire_time = t_now + (get_ticks_per_sec()
+        /* If we've raised int, we speed up the timer, so that we quickly
+         * notice any new packets queued up in response */
+        if (ehci->int_req_by_async && (ehci->usbsts & USBSTS_INT)) {
+            expire_time = t_now + get_ticks_per_sec() / (FRAME_TIMER_FREQ * 2);
+            ehci->int_req_by_async = false;
+        } else {
+            expire_time = t_now + (get_ticks_per_sec()
                                * (ehci->async_stepdown+1) / FRAME_TIMER_FREQ);
+        }
         qemu_mod_timer(ehci->frame_timer, expire_time);
     }
 }
-- 
1.7.12.1