diff mbox

2 issues with qemu-master / 1.2 ehci code

Message ID 502DE550.90503@redhat.com
State New
Headers show

Commit Message

Hans de Goede Aug. 17, 2012, 6:31 a.m. UTC 
Hi,

On 08/16/2012 09:26 PM, Gerd Hoffmann wrote:

<snip>

>> I can get things working by turning ehci_fill_queue() into a nop...
>> Investigating this further. But if you've any insights they would be
>> appreciated. I'm thinking this may be caused by packets completing
>> out of order, which could be caused by the special handling of some
>> ctrl commands ...
>
> usbredir wouldn't see multiple parallel inflight requests per endpoint
> unless you explicitly enable this using usb_ep_set_pipeline().  Doing
> that on bulk endpoints should give you a nice performance boost for usb
> sticks.  Doing it on the control endpoint is asking for trouble ;)
>
> Can you turn on all ehci tracepoints & send me a log?

No need for that, I've been debugging this almost the entire day yesterday
and I've significantly narrowed it down further, the problem is that
the assert triggers when:
1) There are more packets then 1 in the queue (iow fill_queue did something)
2) A packet completion is not followed by an advqueue call

2) happens when a packet fails, and the queue should be halted, in this case
ehci_state_writeback() correctly does not call advqueue, bot does a horizqh
instead. The problem is that once this happens p->qtdaddr == q->qtdaddr is no
longer true ...

I've already come up with multiple approaches to try and fix this, but none
work sofar :|


Another problem with failing packets is that hw/usb/core.c will happily execute
the next packet in the ep queue, even though the spec says the ep-queue should
be halted, giving the guest a chance to cancel transfers after the failed one
without them ever executing. I've a poc patch fixing this too.

I've attached a wip patch with my work on this so-far. Note that currently
the halted bit at the ehci level never gets cleared (since we've queues at 2
levels, we need to track halting at 2 levels too)

Regards,

Hans

Comments

Gerd Hoffmann Aug. 17, 2012, 7:07 a.m. UTC | #1 
Hi,

> 2) happens when a packet fails, and the queue should be halted, in
> this case

Should we just cancel all queued packets on endpoint halts then?  If the
guest decides to go on we'll easily re-queue everything (with the
existing code).  If the guest does something else we don't have to do
anything special.

Not canceling, then trying to figure what the new state of the already
queued packets is could become tricky ...

> Another problem with failing packets is that hw/usb/core.c will
> happily execute the next packet in the ep queue, even though the spec
> says the ep-queue should be halted, giving the guest a chance to
> cancel transfers after the failed one without them ever executing.
> I've a poc patch fixing this too.

Indeed, the core should stop processing them.  Question is what to do
then.  If the host controller cancels all packets anyway we don't have
to do much extra work on the core.  Just stop processing on error and
implicitly un-halt the endpoint when the queue becomes empty.  Maybe
some extra state tracking and asserts() to catch bugs.

cheers,
  Gerd
Hans de Goede Aug. 17, 2012, 7:16 a.m. UTC | #2 
Hi,

On 08/17/2012 09:07 AM, Gerd Hoffmann wrote:
>    Hi,
>
>> 2) happens when a packet fails, and the queue should be halted, in
>> this case
>
> Should we just cancel all queued packets on endpoint halts then?  If the
> guest decides to go on we'll easily re-queue everything (with the
> existing code).  If the guest does something else we don't have to do
> anything special.
>
> Not canceling, then trying to figure what the new state of the already
> queued packets is could become tricky ...
>

Yeah, actually I've come to the same conclusion, after wasting almost a day
trying to get things to work the "figure what the new state of the already
queued packets is" way, and that indeed is not the way to go!

Spo I've just written a patch cancelling all the queued up packets, testing
that now :)

>> Another problem with failing packets is that hw/usb/core.c will
>> happily execute the next packet in the ep queue, even though the spec
>> says the ep-queue should be halted, giving the guest a chance to
>> cancel transfers after the failed one without them ever executing.
>> I've a poc patch fixing this too.
>
> Indeed, the core should stop processing them.  Question is what to do
> then.  If the host controller cancels all packets anyway we don't have
> to do much extra work on the core.  Just stop processing on error and
> implicitly un-halt the endpoint when the queue becomes empty.  Maybe
> some extra state tracking and asserts() to catch bugs.

Right, also done in my wip patch. I'll go test it, then split it up in
multiple patches and then submit. We really should get this in 1.2 btw!
diff mbox

Patch

diff --git a/hw/usb.h b/hw/usb.h
index 432ccae..6fa3088 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;
 };
@@ -375,6 +376,8 @@  void usb_ep_set_max_packet_size(USBDevice *dev, int pid, int ep,
                                 uint16_t raw);
 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);
+void usb_ep_process_queue(USBEndpoint *ep);
+void usb_ep_clear_halt(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 c7e5bc0..c9ad57e 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,33 +410,26 @@  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. */
-void usb_packet_complete(USBDevice *dev, USBPacket *p)
+static void __usb_packet_complete(USBDevice *dev, USBPacket *p)
 {
     USBEndpoint *ep = p->ep;
-    int ret;
 
-    usb_packet_check_state(p, USB_PACKET_ASYNC);
-    assert(QTAILQ_FIRST(&ep->queue) == p);
+    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);
+}
 
-    while (!QTAILQ_EMPTY(&ep->queue)) {
-        p = QTAILQ_FIRST(&ep->queue);
-        if (p->state == USB_PACKET_ASYNC) {
-            break;
-        }
-        usb_packet_check_state(p, USB_PACKET_QUEUED);
-        ret = usb_process_one(p);
-        if (ret == USB_RET_ASYNC) {
-            usb_packet_set_state(p, USB_PACKET_ASYNC);
-            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);
-    }
+void usb_packet_complete(USBDevice *dev, USBPacket *p)
+{
+    USBEndpoint *ep = p->ep;
+
+    usb_packet_check_state(p, USB_PACKET_ASYNC);
+    assert(QTAILQ_FIRST(&ep->queue) == p);
+    __usb_packet_complete(dev, p);
+    usb_ep_process_queue(ep);
 }
 
 /* Cancel an active packet.  The packed must have been deferred by
@@ -702,3 +703,30 @@  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;
 }
+
+void usb_ep_process_queue(USBEndpoint *ep)
+{
+    USBPacket *p;
+    int ret;
+
+    while (!ep->halted && !QTAILQ_EMPTY(&ep->queue)) {
+        p = QTAILQ_FIRST(&ep->queue);
+        if (p->state == USB_PACKET_ASYNC) {
+            break;
+        }
+        usb_packet_check_state(p, USB_PACKET_QUEUED);
+        ret = usb_process_one(p);
+        if (ret == USB_RET_ASYNC) {
+            usb_packet_set_state(p, USB_PACKET_ASYNC);
+            break;
+        }
+        p->result = ret;
+        __usb_packet_complete(ep->dev, p);
+    }
+}
+
+void usb_ep_clear_halt(USBEndpoint *ep)
+{
+    ep->halted = false;
+    usb_ep_process_queue(ep);
+}
diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 41d663d..c8d2d96 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -366,6 +366,7 @@  struct EHCIQueue {
     uint64_t ts;
     int async;
     int revalidate;
+    bool halted;
 
     /* cached data from guest - needs to be flushed
      * when guest removes an entry (doorbell, handshake sequence)
@@ -740,6 +741,7 @@  static EHCIPacket *ehci_alloc_packet(EHCIQueue *q)
 
 static void ehci_free_packet(EHCIPacket *p)
 {
+    printf("queue %p packet %p free\n", p->queue, p);
     trace_usb_ehci_packet_action(p->queue, p, "free");
     if (p->async == EHCI_ASYNC_INFLIGHT) {
         usb_cancel_packet(&p->packet);
@@ -1773,6 +1775,7 @@  static EHCIQueue *ehci_state_fetchqh(EHCIState *ehci, int async)
     if (q->revalidate && (q->qh.epchar      != qh.epchar ||
                           q->qh.epcap       != qh.epcap  ||
                           q->qh.current_qtd != qh.current_qtd)) {
+        printf("queue %p packet %p qtdaddr %08x freeing because of cancel\n", q, NULL, q->qtdaddr);
         ehci_free_queue(q);
         q = ehci_alloc_queue(ehci, entry, async);
         q->seen++;
@@ -1786,6 +1789,7 @@  static EHCIQueue *ehci_state_fetchqh(EHCIState *ehci, int async)
     if (q->dev != NULL && q->dev->addr != devaddr) {
         if (!QTAILQ_EMPTY(&q->packets)) {
             /* should not happen (guest bug) */
+            printf("guest bug 2!!!\n");
             while ((p = QTAILQ_FIRST(&q->packets)) != NULL) {
                 ehci_free_packet(p);
             }
@@ -1796,18 +1800,22 @@  static EHCIQueue *ehci_state_fetchqh(EHCIState *ehci, int async)
         q->dev = ehci_find_device(q->ehci, devaddr);
     }
 
-    if (p && p->async == EHCI_ASYNC_INFLIGHT) {
+    if (!q->halted && p && p->async == EHCI_ASYNC_INFLIGHT) {
+//        if (p->packet.ep->halted && !(q->qh.token & QTD_TOKEN_HALT)) {
+//            usb_ep_clear_halt(p->packet.ep);
+//        }
         /* I/O still in progress -- skip queue */
         ehci_set_state(ehci, async, EST_HORIZONTALQH);
         goto out;
     }
-    if (p && p->async == EHCI_ASYNC_FINISHED) {
+    if (!q->halted && p && p->async == EHCI_ASYNC_FINISHED) {
         /* I/O finished -- continue processing queue */
         trace_usb_ehci_packet_action(p->queue, p, "complete");
+        if ((q->qh.epchar & QH_EPCHAR_EP_MASK) == 0)
+             printf("queue %p packet %p qtdaddr %08x completing from fetchqh\n", q, p, p->qtdaddr);
         ehci_set_state(ehci, async, EST_EXECUTING);
         goto out;
     }
-
     if (async && (q->qh.epchar & QH_EPCHAR_H)) {
 
         /*  EHCI spec version 1.0 Section 4.8.3 & 4.10.1 */
@@ -1842,6 +1850,8 @@  static EHCIQueue *ehci_state_fetchqh(EHCIState *ehci, int async)
         ehci_set_state(ehci, async, EST_FETCHQTD);
 
     } else {
+        if ((q->qh.epchar & QH_EPCHAR_EP_MASK) == 0)
+            printf("queue %p packet %p qtdaddr %08x advqueue fetchqh\n", q, NULL, q->qtdaddr);
         /*  EHCI spec version 1.0 Section 4.10.2 */
         ehci_set_state(ehci, async, EST_ADVANCEQUEUE);
     }
@@ -1951,19 +1961,31 @@  static int ehci_state_fetchqtd(EHCIQueue *q)
     p = QTAILQ_FIRST(&q->packets);
     while (p != NULL && p->qtdaddr != q->qtdaddr) {
         /* should not happen (guest bug) */
+        printf("guest bug!!!\n");
         ehci_free_packet(p);
         p = QTAILQ_FIRST(&q->packets);
     }
     if (p != NULL) {
         ehci_qh_do_overlay(q);
-        if (p->async == EHCI_ASYNC_INFLIGHT) {
+        switch (p->async) {
+        case EHCI_ASYNC_NONE:
+            ehci_set_state(q->ehci, q->async, EST_EXECUTING);
+            break;
+        case EHCI_ASYNC_INFLIGHT:
             ehci_set_state(q->ehci, q->async, EST_HORIZONTALQH);
-        } else {
+            break;
+        case EHCI_ASYNC_FINISHED:
+            trace_usb_ehci_packet_action(q, p, "complete");
+            if ((q->qh.epchar & QH_EPCHAR_EP_MASK) == 0)
+                printf("queue %p packet %p qtdaddr %08x completing from fetchqtd\n", q, p, p->qtdaddr);
             ehci_set_state(q->ehci, q->async, EST_EXECUTING);
+            break;
         }
         again = 1;
     } else if (qtd.token & QTD_TOKEN_ACTIVE) {
         p = ehci_alloc_packet(q);
+        if ((q->qh.epchar & QH_EPCHAR_EP_MASK) == 0)
+            printf("queue %p packet %p qtdaddr %08x ep %d added (fetchqtd)\n", q, p, q->qtdaddr, (q->qh.epchar & QH_EPCHAR_EP_MASK) >> QH_EPCHAR_EP_SH);
         p->qtdaddr = q->qtdaddr;
         p->qtd = qtd;
         ehci_set_state(q->ehci, q->async, EST_EXECUTE);
@@ -2012,6 +2034,8 @@  static void ehci_fill_queue(EHCIPacket *p)
             break;
         }
         p = ehci_alloc_packet(q);
+        if ((q->qh.epchar & QH_EPCHAR_EP_MASK) == 0)
+            printf("queue %p packet %p qtdaddr %08x added (fill_queue)\n", q, p, qtdaddr);
         p->qtdaddr = qtdaddr;
         p->qtd = qtd;
         p->usb_status = ehci_execute(p, "queue");
@@ -2076,10 +2100,15 @@  static int ehci_state_executing(EHCIQueue *q)
     EHCIPacket *p = QTAILQ_FIRST(&q->packets);
     int again = 0;
 
+    if ((q->qh.epchar & QH_EPCHAR_EP_MASK) == 0)
+        printf("queue %p packet %p qtdaddr %08x completing, async %d\n", q, p, p->qtdaddr, p->async);
+
     assert(p != NULL);
     assert(p->qtdaddr == q->qtdaddr);
 
     ehci_execute_complete(q);
+    if ((q->qh.epchar & QH_EPCHAR_EP_MASK) == 0)
+        printf("queue %p packet %p qtdaddr %08x completed, status %d\n", q, p, p->qtdaddr, p->usb_status);
     if (p->usb_status == USB_RET_ASYNC) {
         goto out;
     }
@@ -2137,6 +2166,7 @@  static int ehci_state_writeback(EHCIQueue *q)
      * bit is clear.
      */
     if (q->qh.token & QTD_TOKEN_HALT) {
+        q->halted = true;
         ehci_set_state(q->ehci, q->async, EST_HORIZONTALQH);
         again = 1;
     } else {