[Qemu-devel] [PATCH 1/3] usb: Halt ep queue en cancel pending packets on a packet error

[Qemu-devel] [PATCH 1/3] usb: Halt ep queue en cancel pending packets on a packet error

[Hans de Goede]

For controllers which queue up more then 1 packet at a time, we must halt the
ep queue, and inside the controller code cancel all pending packets on an
error.

There are multiple reasons for this:
1) Guests expect the controllers to halt ep queues on error, so that they
get the opportunity to cancel transfers which the scheduled after the failing
one, before processing continues

2) Not cancelling queued up packets after a failed transfer also messes up
the controller state machine, in the case of EHCI causing the following
assert to trigger: "assert(p->qtdaddr == q->qtdaddr)" at hcd-ehci.c:2075

3) For bulk endpoints with pipelining enabled (redirection to a real USB
device), we must cancel all the transfers after this a failed one so that:
a) If they've completed already, they are not processed further causing more
   stalls to be reported, originating from the same failed transfer
b) If still in flight, they are cancelled before the guest does
   a clear stall, otherwise the guest and device can loose sync!

Note this patch only touches the ehci and uhci controller changes, since AFAIK
no other controllers actually queue up multiple transfer. If I'm wrong on this
other controllers need to be updated too!

Also note that this patch was heavily tested with the ehci code, where I had
a reproducer for a device causing a transfer to fail. The uhci code is not
tested with actually failing transfers and could do with a thorough review!

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb.h          |  1 +
 hw/usb/core.c     | 32 +++++++++++++++++++++++++-------
 hw/usb/hcd-ehci.c | 13 +++++++++++++
 hw/usb/hcd-uhci.c | 16 ++++++++++++++++
 4 files changed, 55 insertions(+), 7 deletions(-)

diff --git a/hw/usb.h b/hw/usb.h
index 432ccae..e574477 100644
--- a/hw/usb.h
+++ b/hw/usb.h
@@ -179,6 +179,7 @@ struct USBEndpoint {
     uint8_t ifnum;
     int max_packet_size;
     bool pipeline;
+    bool halted;
     USBDevice *dev;
     QTAILQ_HEAD(, USBPacket) queue;
 };
diff --git a/hw/usb/core.c b/hw/usb/core.c
index c7e5bc0..7ed0e45 100644
--- a/hw/usb/core.c
+++ b/hw/usb/core.c
@@ -382,12 +382,20 @@ int usb_handle_packet(USBDevice *dev, USBPacket *p)
     usb_packet_check_state(p, USB_PACKET_SETUP);
     assert(p->ep != NULL);
 
+    /* Submitting a new packet clears halt */
+    p->ep->halted = false;
+
     if (QTAILQ_EMPTY(&p->ep->queue) || p->ep->pipeline) {
         ret = usb_process_one(p);
         if (ret == USB_RET_ASYNC) {
             usb_packet_set_state(p, USB_PACKET_ASYNC);
             QTAILQ_INSERT_TAIL(&p->ep->queue, p, queue);
         } else {
+            /*
+             * When pipelining is enabled usb-devices must always return async,
+             * otherwise packets can complete out of order!
+             */
+            assert(!p->ep->pipeline);
             p->result = ret;
             usb_packet_set_state(p, USB_PACKET_COMPLETE);
         }
@@ -402,6 +410,20 @@ int usb_handle_packet(USBDevice *dev, USBPacket *p)
 /* 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. */
+static void __usb_packet_complete(USBDevice *dev, USBPacket *p)
+{
+    USBEndpoint *ep = p->ep;
+
+    assert(p->result != USB_RET_ASYNC && p->result != USB_RET_NAK);
+
+    if (p->result < 0) {
+        ep->halted = true;
+    }
+    usb_packet_set_state(p, USB_PACKET_COMPLETE);
+    QTAILQ_REMOVE(&ep->queue, p, queue);
+    dev->port->ops->complete(dev->port, p);
+}
+
 void usb_packet_complete(USBDevice *dev, USBPacket *p)
 {
     USBEndpoint *ep = p->ep;
@@ -409,11 +431,9 @@ void usb_packet_complete(USBDevice *dev, USBPacket *p)
 
     usb_packet_check_state(p, USB_PACKET_ASYNC);
     assert(QTAILQ_FIRST(&ep->queue) == p);
-    usb_packet_set_state(p, USB_PACKET_COMPLETE);
-    QTAILQ_REMOVE(&ep->queue, p, queue);
-    dev->port->ops->complete(dev->port, p);
+    __usb_packet_complete(dev, p);
 
-    while (!QTAILQ_EMPTY(&ep->queue)) {
+    while (!ep->halted && !QTAILQ_EMPTY(&ep->queue)) {
         p = QTAILQ_FIRST(&ep->queue);
         if (p->state == USB_PACKET_ASYNC) {
             break;
@@ -425,9 +445,7 @@ void usb_packet_complete(USBDevice *dev, USBPacket *p)
             break;
         }
         p->result = ret;
-        usb_packet_set_state(p, USB_PACKET_COMPLETE);
-        QTAILQ_REMOVE(&ep->queue, p, queue);
-        dev->port->ops->complete(dev->port, p);
+        __usb_packet_complete(ep->dev, p);
     }
 }
 
diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 41d663d..a6b8527 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -2137,6 +2137,19 @@ static int ehci_state_writeback(EHCIQueue *q)
      * bit is clear.
      */
     if (q->qh.token & QTD_TOKEN_HALT) {
+        /*
+         * We should not do any further processing on a halted queue!
+         * This is esp. important for bulk endpoints with pipelining enabled
+         * (redirection to a real USB device), where we must cancel all the
+         * transfers after this one so that:
+         * 1) If they've completed already, they are not processed further
+         *    causing more stalls, originating from the same failed transfer
+         * 2) If still in flight, they are cancelled before the guest does
+         *    a clear stall, otherwise the guest and device can loose sync!
+         */
+        while ((p = QTAILQ_FIRST(&q->packets)) != NULL) {
+            ehci_free_packet(p);
+        }
         ehci_set_state(q->ehci, q->async, EST_HORIZONTALQH);
         again = 1;
     } else {
diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index 1ace2a4..8987734 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -748,6 +748,22 @@ static int uhci_complete_td(UHCIState *s, UHCI_TD *td, UHCIAsync *async, uint32_
     return TD_RESULT_COMPLETE;
 
 out:
+    /*
+     * We should not do any further processing on a queue with errors!
+     * This is esp. important for bulk endpoints with pipelining enabled
+     * (redirection to a real USB device), where we must cancel all the
+     * transfers after this one so that:
+     * 1) If they've completed already, they are not processed further
+     *    causing more stalls, originating from the same failed transfer
+     * 2) If still in flight, they are cancelled before the guest does
+     *    a clear stall, otherwise the guest and device can loose sync!
+     */
+    while (!QTAILQ_EMPTY(&async->queue->asyncs)) {
+        UHCIAsync *as = QTAILQ_FIRST(&async->queue->asyncs);
+        uhci_async_unlink(as);
+        uhci_async_cancel(as);
+    }
+
     switch(ret) {
     case USB_RET_STALL:
         td->ctrl |= TD_CTRL_STALL;
-- 
1.7.11.4

[Qemu-devel] [PATCH 2/3] usb: controllers do not need to check for babble themselves

[Hans de Goede]

If an (emulated) usb-device tries to write more data to a packet then
its iov len, this will trigger an assert in usb_packet_copy(), and if
a driver somehow circumvents that check and writes more data to the
iov then there is space, we have a much bigger problem then not correctly
reporting babble to the guest.

In practice babble will only happen with (real) redirected devices, and there
both the usb-host os and the qemu usb-device code already check for it.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/hcd-ehci.c | 4 ----
 hw/usb/hcd-uhci.c | 5 -----
 2 files changed, 9 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index a6b8527..4750175 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -1460,10 +1460,6 @@ static void ehci_execute_complete(EHCIQueue *q)
             assert(0);
             break;
         }
-    } else if ((p->usb_status > p->tbytes) && (p->pid == USB_TOKEN_IN)) {
-        p->usb_status = USB_RET_BABBLE;
-        q->qh.token |= (QTD_TOKEN_HALT | QTD_TOKEN_BABBLE);
-        ehci_raise_irq(q->ehci, USBSTS_ERRINT);
     } else {
         // TODO check 4.12 for splits
 
diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index 8987734..4e43d20 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -729,11 +729,6 @@ static int uhci_complete_td(UHCIState *s, UHCI_TD *td, UHCIAsync *async, uint32_
         *int_mask |= 0x01;
 
     if (pid == USB_TOKEN_IN) {
-        if (len > max_len) {
-            ret = USB_RET_BABBLE;
-            goto out;
-        }
-
         if ((td->ctrl & TD_CTRL_SPD) && len < max_len) {
             *int_mask |= 0x02;
             /* short packet: do not update QH */
-- 
1.7.11.4

[Qemu-devel] [PATCH 3/3] ehci: simplify ehci_state_executing

[Hans de Goede]

ehci_state_executing does not need to check for p->usb_status == USB_RET_ASYNC
or USB_RET_PROCERR, since ehci_execute_complete already does a similar check
and will trigger an assert if either value is encountered.

USB_RET_ASYNC should never be the packet status when execute_complete runs
for obvious reasons, and USB_RET_PROCERR is only used by ehci_state_execute /
ehci_execute not by ehci_state_executing / ehci_execute_complete.

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

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 4750175..378b42b 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -2070,19 +2070,11 @@ out:
 static int ehci_state_executing(EHCIQueue *q)
 {
     EHCIPacket *p = QTAILQ_FIRST(&q->packets);
-    int again = 0;
 
     assert(p != NULL);
     assert(p->qtdaddr == q->qtdaddr);
 
     ehci_execute_complete(q);
-    if (p->usb_status == USB_RET_ASYNC) {
-        goto out;
-    }
-    if (p->usb_status == USB_RET_PROCERR) {
-        again = -1;
-        goto out;
-    }
 
     // 4.10.3
     if (!q->async) {
@@ -2100,11 +2092,8 @@ static int ehci_state_executing(EHCIQueue *q)
         ehci_set_state(q->ehci, q->async, EST_WRITEBACK);
     }
 
-    again = 1;
-
-out:
     ehci_flush_qh(q);
-    return again;
+    return 1;
 }
 
 
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 1/3] usb: Halt ep queue en cancel pending packets on a packet error

[Gerd Hoffmann]

  Hi,

> Note this patch only touches the ehci and uhci controller changes, since AFAIK
> no other controllers actually queue up multiple transfer. If I'm wrong on this
> other controllers need to be updated too!

xhci does it too (although it is hard to test as xhci can happily submit
256k transfers where ehci and uhci have to use a bunch of smaller
packets instead).

Some minor nits below (the other two patches look good):

> +    /* Submitting a new packet clears halt */
> +    p->ep->halted = false;

check that the queue is empty when halted is set (i.e. enforce cancel on
error) ?

> +
>      if (QTAILQ_EMPTY(&p->ep->queue) || p->ep->pipeline) {
>          ret = usb_process_one(p);
>          if (ret == USB_RET_ASYNC) {
>              usb_packet_set_state(p, USB_PACKET_ASYNC);
>              QTAILQ_INSERT_TAIL(&p->ep->queue, p, queue);
>          } else {
> +            /*
> +             * When pipelining is enabled usb-devices must always return async,
> +             * otherwise packets can complete out of order!
> +             */
> +            assert(!p->ep->pipeline);

Strictly speaking returning something != async is fine for the first
package in the queue, but I guess in practice this doesn't matter as
enabling pipelining doesn't make sense unless you actually go async.

>              p->result = ret;
>              usb_packet_set_state(p, USB_PACKET_COMPLETE);
>          }
> @@ -402,6 +410,20 @@ int usb_handle_packet(USBDevice *dev, USBPacket *p)
>  /* 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. */

That comments should be ...

> +static void __usb_packet_complete(USBDevice *dev, USBPacket *p)
> +{
> +    USBEndpoint *ep = p->ep;
> +
> +    assert(p->result != USB_RET_ASYNC && p->result != USB_RET_NAK);
> +
> +    if (p->result < 0) {
> +        ep->halted = true;
> +    }
> +    usb_packet_set_state(p, USB_PACKET_COMPLETE);
> +    QTAILQ_REMOVE(&ep->queue, p, queue);
> +    dev->port->ops->complete(dev->port, p);
> +}
> +

... here, where the function for the external users is.

>  void usb_packet_complete(USBDevice *dev, USBPacket *p)
>  {

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 1/3] usb: Halt ep queue en cancel pending packets on a packet error

[Hans de Goede]

Hi,

On 08/17/2012 12:30 PM, Gerd Hoffmann wrote:
>    Hi,
>
>> Note this patch only touches the ehci and uhci controller changes, since AFAIK
>> no other controllers actually queue up multiple transfer. If I'm wrong on this
>> other controllers need to be updated too!
>
> xhci does it too (although it is hard to test as xhci can happily submit
> 256k transfers where ehci and uhci have to use a bunch of smaller
> packets instead).
>

Good point, I'm completely unfamiliar with the xhci code I'm afraid (at some point
in time this will need to change, but not right now), any chance you could fill in
the xhci part of this patch?  Feel free to merge it with my patch so as to avoid
having a "broken" state in git.

> Some minor nits below (the other two patches look good):
>
>> +    /* Submitting a new packet clears halt */
>> +    p->ep->halted = false;
>
> check that the queue is empty when halted is set (i.e. enforce cancel on
> error) ?

Good point will fix.

>
>> +
>>       if (QTAILQ_EMPTY(&p->ep->queue) || p->ep->pipeline) {
>>           ret = usb_process_one(p);
>>           if (ret == USB_RET_ASYNC) {
>>               usb_packet_set_state(p, USB_PACKET_ASYNC);
>>               QTAILQ_INSERT_TAIL(&p->ep->queue, p, queue);
>>           } else {
>> +            /*
>> +             * When pipelining is enabled usb-devices must always return async,
>> +             * otherwise packets can complete out of order!
>> +             */
>> +            assert(!p->ep->pipeline);
>
> Strictly speaking returning something != async is fine for the first
> package in the queue, but I guess in practice this doesn't matter as
> enabling pipelining doesn't make sense unless you actually go async.
>
>>               p->result = ret;
>>               usb_packet_set_state(p, USB_PACKET_COMPLETE);
>>           }
>> @@ -402,6 +410,20 @@ int usb_handle_packet(USBDevice *dev, USBPacket *p)
>>   /* 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. */
>
> That comments should be ...
>
>> +static void __usb_packet_complete(USBDevice *dev, USBPacket *p)
>> +{
>> +    USBEndpoint *ep = p->ep;
>> +
>> +    assert(p->result != USB_RET_ASYNC && p->result != USB_RET_NAK);
>> +
>> +    if (p->result < 0) {
>> +        ep->halted = true;
>> +    }
>> +    usb_packet_set_state(p, USB_PACKET_COMPLETE);
>> +    QTAILQ_REMOVE(&ep->queue, p, queue);
>> +    dev->port->ops->complete(dev->port, p);
>> +}
>> +
>
> ... here, where the function for the external users is.
>

Fixed, will send a new version (without the xhci changes) right away, to
cut back on the spam I'm only going to resend 1/3.

Regards,

Hans

Re: [Qemu-devel] [PATCH 1/3] usb: Halt ep queue en cancel pending packets on a packet error

[Andreas Färber]

Not being too familiar with the USB code I wonder if $subject was
supposed to say "and cancel"?

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 1/3] usb: Halt ep queue en cancel pending packets on a packet error

[Hans de Goede]

Hi,

On 08/17/2012 07:03 PM, Andreas Färber wrote:
> Not being too familiar with the USB code I wonder if $subject was
> supposed to say "and cancel"?

Yes, "en" is Dutch for and, no idea what I was thinking, sorry about
that :)

Regards,

Hans