[Qemu-devel] uhci: Cleanups, fixes and improvements

[Qemu-devel] uhci: Cleanups, fixes and improvements

[Hans de Goede]

Here is a patchset with assorted uhci cleanups / fixes / improvments,
please add these to your tree for Anthony.

Regards,

Hans

[Qemu-devel] [PATCH 01/17] usb: Enforce iso endpoints never returing USB_RET_ASYNC

[Hans de Goede]

ehci was already testing for this, and we depend in various places
on no devices doing this, so lets move the check for this to the
usb core.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/core.c     | 1 +
 hw/usb/hcd-ehci.c | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/usb/core.c b/hw/usb/core.c
index 99b9fdb..ab37f6f 100644
--- a/hw/usb/core.c
+++ b/hw/usb/core.c
@@ -391,6 +391,7 @@ int usb_handle_packet(USBDevice *dev, USBPacket *p)
     if (QTAILQ_EMPTY(&p->ep->queue) || p->ep->pipeline) {
         ret = usb_process_one(p);
         if (ret == USB_RET_ASYNC) {
+            assert(p->ep->type != USB_ENDPOINT_XFER_ISOC);
             usb_packet_set_state(p, USB_PACKET_ASYNC);
             QTAILQ_INSERT_TAIL(&p->ep->queue, p, queue);
         } else if (ret == USB_RET_ADD_TO_QUEUE) {
diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 35b69c2..1e521dd 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -1667,7 +1667,6 @@ static int ehci_process_itd(EHCIState *ehci,
                                  (itd->transact[i] & ITD_XACT_IOC) != 0);
                 usb_packet_map(&ehci->ipacket, &ehci->isgl);
                 ret = usb_handle_packet(dev, &ehci->ipacket);
-                assert(ret != USB_RET_ASYNC);
                 usb_packet_unmap(&ehci->ipacket, &ehci->isgl);
             } else {
                 DPRINTF("ISOCH: attempt to addess non-iso endpoint\n");
-- 
1.7.12.1

[Qemu-devel] [PATCH 02/17] uhci: No need to handle async completion of isoc packets

[Hans de Goede]

No devices ever return async for isoc endpoints and the core
already enforces this.

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

diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index 4a1ea6b..129792b 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -101,7 +101,6 @@ struct UHCIAsync {
     UHCIQueue *queue;
     QTAILQ_ENTRY(UHCIAsync) next;
     uint32_t  td;
-    uint8_t   isoc;
     uint8_t   done;
 };
 
@@ -849,7 +848,6 @@ static int uhci_handle_td(UHCIState *s, uint32_t addr, UHCI_TD *td,
      * for initial isochronous requests
      */
     async->queue->valid = 32;
-    async->isoc = td->ctrl & TD_CTRL_IOS;
 
     max_len = ((td->token >> 21) + 1) & 0x7ff;
     pid = td->token & 0xff;
@@ -911,30 +909,9 @@ static void uhci_async_complete(USBPort *port, USBPacket *packet)
         return;
     }
 
-    if (async->isoc) {
-        UHCI_TD td;
-        uint32_t link = async->td;
-        uint32_t int_mask = 0, val;
-
-        pci_dma_read(&s->dev, link & ~0xf, &td, sizeof(td));
-        le32_to_cpus(&td.link);
-        le32_to_cpus(&td.ctrl);
-        le32_to_cpus(&td.token);
-        le32_to_cpus(&td.buffer);
-
-        uhci_async_unlink(async);
-        uhci_complete_td(s, &td, async, &int_mask);
-        s->pending_int_mask |= int_mask;
-
-        /* update the status bits of the TD */
-        val = cpu_to_le32(td.ctrl);
-        pci_dma_write(&s->dev, (link & ~0xf) + 4, &val, sizeof(val));
-        uhci_async_free(async);
-    } else {
-        async->done = 1;
-        if (s->frame_bytes < s->frame_bandwidth) {
-            qemu_bh_schedule(s->bh);
-        }
+    async->done = 1;
+    if (s->frame_bytes < s->frame_bandwidth) {
+        qemu_bh_schedule(s->bh);
     }
 }
 
-- 
1.7.12.1

[Qemu-devel] [PATCH 03/17] uhci: cleanup: Add an unlink call to uhci_async_cancel()

[Hans de Goede]

All callers of uhci_async_cancel() call uhci_async_unlink() first, so
lets move the unlink call to uhci_async_cancel()

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

diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index 129792b..82dd5c2 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -232,6 +232,7 @@ static void uhci_async_unlink(UHCIAsync *async)
 
 static void uhci_async_cancel(UHCIAsync *async)
 {
+    uhci_async_unlink(async);
     trace_usb_uhci_packet_cancel(async->queue->token, async->td, async->done);
     if (!async->done)
         usb_cancel_packet(&async->packet);
@@ -266,7 +267,6 @@ static void uhci_async_validate_end(UHCIState *s)
         }
         while (!QTAILQ_EMPTY(&queue->asyncs)) {
             async = QTAILQ_FIRST(&queue->asyncs);
-            uhci_async_unlink(async);
             uhci_async_cancel(async);
         }
         uhci_queue_free(queue);
@@ -284,7 +284,6 @@ static void uhci_async_cancel_device(UHCIState *s, USBDevice *dev)
                 curr->packet.ep->dev != dev) {
                 continue;
             }
-            uhci_async_unlink(curr);
             uhci_async_cancel(curr);
         }
     }
@@ -297,7 +296,6 @@ static void uhci_async_cancel_all(UHCIState *s)
 
     QTAILQ_FOREACH_SAFE(queue, &s->queues, next, nq) {
         QTAILQ_FOREACH_SAFE(curr, &queue->asyncs, next, n) {
-            uhci_async_unlink(curr);
             uhci_async_cancel(curr);
         }
         uhci_queue_free(queue);
-- 
1.7.12.1

[Qemu-devel] [PATCH 04/17] uhci: Don't retry on error

[Hans de Goede]

Since we are either dealing with emulated devices, where retrying is
not going to help, or with redirected devices where the host OS will
have already retried, don't bother retrying on failed transfers.

Also move some common/indentical code out of all the error cases
into the generic error path.

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

diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index 82dd5c2..eecd291 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -696,10 +696,6 @@ static USBDevice *uhci_find_device(UHCIState *s, uint8_t addr)
 static void uhci_async_complete(USBPort *port, USBPacket *packet);
 static void uhci_process_frame(UHCIState *s);
 
-/* return -1 if fatal error (frame must be stopped)
-          0 if TD successful
-          1 if TD unsuccessful or inactive
-*/
 static int uhci_complete_td(UHCIState *s, UHCI_TD *td, UHCIAsync *async, uint32_t *int_mask)
 {
     int len = 0, max_len, err, ret;
@@ -742,60 +738,40 @@ static int uhci_complete_td(UHCIState *s, UHCI_TD *td, UHCIAsync *async, uint32_
 
 out:
     switch(ret) {
+    case USB_RET_NAK:
+        td->ctrl |= TD_CTRL_NAK;
+        return TD_RESULT_NEXT_QH;
+
     case USB_RET_STALL:
         td->ctrl |= TD_CTRL_STALL;
-        td->ctrl &= ~TD_CTRL_ACTIVE;
-        s->status |= UHCI_STS_USBERR;
-        if (td->ctrl & TD_CTRL_IOC) {
-            *int_mask |= 0x01;
-        }
-        uhci_update_irq(s);
         trace_usb_uhci_packet_complete_stall(async->queue->token, async->td);
-        return TD_RESULT_NEXT_QH;
+        err = TD_RESULT_NEXT_QH;
+        break;
 
     case USB_RET_BABBLE:
         td->ctrl |= TD_CTRL_BABBLE | TD_CTRL_STALL;
-        td->ctrl &= ~TD_CTRL_ACTIVE;
-        s->status |= UHCI_STS_USBERR;
-        if (td->ctrl & TD_CTRL_IOC) {
-            *int_mask |= 0x01;
-        }
-        uhci_update_irq(s);
         /* frame interrupted */
         trace_usb_uhci_packet_complete_babble(async->queue->token, async->td);
-        return TD_RESULT_STOP_FRAME;
-
-    case USB_RET_NAK:
-        td->ctrl |= TD_CTRL_NAK;
-        if (pid == USB_TOKEN_SETUP)
-            break;
-        return TD_RESULT_NEXT_QH;
+        err = TD_RESULT_STOP_FRAME;
+        break;
 
     case USB_RET_IOERROR:
     case USB_RET_NODEV:
     default:
-	break;
+        td->ctrl |= TD_CTRL_TIMEOUT;
+        td->ctrl &= ~(3 << TD_CTRL_ERROR_SHIFT);
+        trace_usb_uhci_packet_complete_error(async->queue->token, async->td);
+        err = TD_RESULT_NEXT_QH;
+        break;
     }
 
-    /* Retry the TD if error count is not zero */
-
-    td->ctrl |= TD_CTRL_TIMEOUT;
-    err = (td->ctrl >> TD_CTRL_ERROR_SHIFT) & 3;
-    if (err != 0) {
-        err--;
-        if (err == 0) {
-            td->ctrl &= ~TD_CTRL_ACTIVE;
-            s->status |= UHCI_STS_USBERR;
-            if (td->ctrl & TD_CTRL_IOC)
-                *int_mask |= 0x01;
-            uhci_update_irq(s);
-            trace_usb_uhci_packet_complete_error(async->queue->token,
-                                                 async->td);
-        }
+    td->ctrl &= ~TD_CTRL_ACTIVE;
+    s->status |= UHCI_STS_USBERR;
+    if (td->ctrl & TD_CTRL_IOC) {
+        *int_mask |= 0x01;
     }
-    td->ctrl = (td->ctrl & ~(3 << TD_CTRL_ERROR_SHIFT)) |
-        (err << TD_CTRL_ERROR_SHIFT);
-    return TD_RESULT_NEXT_QH;
+    uhci_update_irq(s);
+    return err;
 }
 
 static int uhci_handle_td(UHCIState *s, uint32_t addr, UHCI_TD *td,
-- 
1.7.12.1

[Qemu-devel] [PATCH 05/17] uhci: Drop unnecessary forward declaration of some static functions

[Hans de Goede]

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

diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index eecd291..7dfedef 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -693,9 +693,6 @@ static USBDevice *uhci_find_device(UHCIState *s, uint8_t addr)
     return NULL;
 }
 
-static void uhci_async_complete(USBPort *port, USBPacket *packet);
-static void uhci_process_frame(UHCIState *s);
-
 static int uhci_complete_td(UHCIState *s, UHCI_TD *td, UHCIAsync *async, uint32_t *int_mask)
 {
     int len = 0, max_len, err, ret;
-- 
1.7.12.1

[Qemu-devel] [PATCH 06/17] uhci: Move emptying of the queue's asyncs' queue to uhci_queue_free

[Hans de Goede]

Cleanup: all callers of uhci_queue_free first unconditionally cancel
all remaining asyncs in the queue, so lets move this to uhci_queue_free().

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

diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index 7dfedef..0335f3c 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -160,6 +160,8 @@ typedef struct UHCI_QH {
     uint32_t el_link;
 } UHCI_QH;
 
+static void uhci_async_cancel(UHCIAsync *async);
+
 static inline int32_t uhci_queue_token(UHCI_TD *td)
 {
     /* covers ep, dev, pid -> identifies the endpoint */
@@ -189,6 +191,12 @@ static UHCIQueue *uhci_queue_get(UHCIState *s, UHCI_TD *td)
 static void uhci_queue_free(UHCIQueue *queue)
 {
     UHCIState *s = queue->uhci;
+    UHCIAsync *async;
+
+    while (!QTAILQ_EMPTY(&queue->asyncs)) {
+        async = QTAILQ_FIRST(&queue->asyncs);
+        uhci_async_cancel(async);
+    }
 
     trace_usb_uhci_queue_del(queue->token);
     QTAILQ_REMOVE(&s->queues, queue, next);
@@ -259,17 +267,11 @@ static void uhci_async_validate_begin(UHCIState *s)
 static void uhci_async_validate_end(UHCIState *s)
 {
     UHCIQueue *queue, *n;
-    UHCIAsync *async;
 
     QTAILQ_FOREACH_SAFE(queue, &s->queues, next, n) {
-        if (queue->valid > 0) {
-            continue;
+        if (!queue->valid) {
+            uhci_queue_free(queue);
         }
-        while (!QTAILQ_EMPTY(&queue->asyncs)) {
-            async = QTAILQ_FIRST(&queue->asyncs);
-            uhci_async_cancel(async);
-        }
-        uhci_queue_free(queue);
     }
 }
 
@@ -292,12 +294,8 @@ static void uhci_async_cancel_device(UHCIState *s, USBDevice *dev)
 static void uhci_async_cancel_all(UHCIState *s)
 {
     UHCIQueue *queue, *nq;
-    UHCIAsync *curr, *n;
 
     QTAILQ_FOREACH_SAFE(queue, &s->queues, next, nq) {
-        QTAILQ_FOREACH_SAFE(curr, &queue->asyncs, next, n) {
-            uhci_async_cancel(curr);
-        }
         uhci_queue_free(queue);
     }
 }
-- 
1.7.12.1

[Qemu-devel] [PATCH 07/17] uhci: Rename UHCIAsync->td to UHCIAsync->td_addr

[Hans de Goede]

We use the name td both to refer to a UHCI_TD read from guest memory as
well as to refer to the guest address where a td is stored, switch over
to always use td_addr in the second case for consistency.

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

diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index 0335f3c..7589a5b 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -100,7 +100,7 @@ struct UHCIAsync {
     QEMUSGList sgl;
     UHCIQueue *queue;
     QTAILQ_ENTRY(UHCIAsync) next;
-    uint32_t  td;
+    uint32_t  td_addr;
     uint8_t   done;
 };
 
@@ -203,22 +203,22 @@ static void uhci_queue_free(UHCIQueue *queue)
     g_free(queue);
 }
 
-static UHCIAsync *uhci_async_alloc(UHCIQueue *queue, uint32_t addr)
+static UHCIAsync *uhci_async_alloc(UHCIQueue *queue, uint32_t td_addr)
 {
     UHCIAsync *async = g_new0(UHCIAsync, 1);
 
     async->queue = queue;
-    async->td = addr;
+    async->td_addr = td_addr;
     usb_packet_init(&async->packet);
     pci_dma_sglist_init(&async->sgl, &queue->uhci->dev, 1);
-    trace_usb_uhci_packet_add(async->queue->token, async->td);
+    trace_usb_uhci_packet_add(async->queue->token, async->td_addr);
 
     return async;
 }
 
 static void uhci_async_free(UHCIAsync *async)
 {
-    trace_usb_uhci_packet_del(async->queue->token, async->td);
+    trace_usb_uhci_packet_del(async->queue->token, async->td_addr);
     usb_packet_cleanup(&async->packet);
     qemu_sglist_destroy(&async->sgl);
     g_free(async);
@@ -228,20 +228,21 @@ static void uhci_async_link(UHCIAsync *async)
 {
     UHCIQueue *queue = async->queue;
     QTAILQ_INSERT_TAIL(&queue->asyncs, async, next);
-    trace_usb_uhci_packet_link_async(async->queue->token, async->td);
+    trace_usb_uhci_packet_link_async(async->queue->token, async->td_addr);
 }
 
 static void uhci_async_unlink(UHCIAsync *async)
 {
     UHCIQueue *queue = async->queue;
     QTAILQ_REMOVE(&queue->asyncs, async, next);
-    trace_usb_uhci_packet_unlink_async(async->queue->token, async->td);
+    trace_usb_uhci_packet_unlink_async(async->queue->token, async->td_addr);
 }
 
 static void uhci_async_cancel(UHCIAsync *async)
 {
     uhci_async_unlink(async);
-    trace_usb_uhci_packet_cancel(async->queue->token, async->td, async->done);
+    trace_usb_uhci_packet_cancel(async->queue->token, async->td_addr,
+                                 async->done);
     if (!async->done)
         usb_cancel_packet(&async->packet);
     usb_packet_unmap(&async->packet, &async->sgl);
@@ -300,7 +301,8 @@ static void uhci_async_cancel_all(UHCIState *s)
     }
 }
 
-static UHCIAsync *uhci_async_find_td(UHCIState *s, uint32_t addr, UHCI_TD *td)
+static UHCIAsync *uhci_async_find_td(UHCIState *s, uint32_t td_addr,
+                                     UHCI_TD *td)
 {
     uint32_t token = uhci_queue_token(td);
     UHCIQueue *queue;
@@ -316,7 +318,7 @@ static UHCIAsync *uhci_async_find_td(UHCIState *s, uint32_t addr, UHCI_TD *td)
     }
 
     QTAILQ_FOREACH(async, &queue->asyncs, next) {
-        if (async->td == addr) {
+        if (async->td_addr == td_addr) {
             return async;
         }
     }
@@ -722,13 +724,14 @@ static int uhci_complete_td(UHCIState *s, UHCI_TD *td, UHCIAsync *async, uint32_
             *int_mask |= 0x02;
             /* short packet: do not update QH */
             trace_usb_uhci_packet_complete_shortxfer(async->queue->token,
-                                                    async->td);
+                                                     async->td_addr);
             return TD_RESULT_NEXT_QH;
         }
     }
 
     /* success */
-    trace_usb_uhci_packet_complete_success(async->queue->token, async->td);
+    trace_usb_uhci_packet_complete_success(async->queue->token,
+                                           async->td_addr);
     return TD_RESULT_COMPLETE;
 
 out:
@@ -739,14 +742,16 @@ out:
 
     case USB_RET_STALL:
         td->ctrl |= TD_CTRL_STALL;
-        trace_usb_uhci_packet_complete_stall(async->queue->token, async->td);
+        trace_usb_uhci_packet_complete_stall(async->queue->token,
+                                             async->td_addr);
         err = TD_RESULT_NEXT_QH;
         break;
 
     case USB_RET_BABBLE:
         td->ctrl |= TD_CTRL_BABBLE | TD_CTRL_STALL;
         /* frame interrupted */
-        trace_usb_uhci_packet_complete_babble(async->queue->token, async->td);
+        trace_usb_uhci_packet_complete_babble(async->queue->token,
+                                              async->td_addr);
         err = TD_RESULT_STOP_FRAME;
         break;
 
@@ -755,7 +760,8 @@ out:
     default:
         td->ctrl |= TD_CTRL_TIMEOUT;
         td->ctrl &= ~(3 << TD_CTRL_ERROR_SHIFT);
-        trace_usb_uhci_packet_complete_error(async->queue->token, async->td);
+        trace_usb_uhci_packet_complete_error(async->queue->token,
+                                             async->td_addr);
         err = TD_RESULT_NEXT_QH;
         break;
     }
@@ -769,7 +775,7 @@ out:
     return err;
 }
 
-static int uhci_handle_td(UHCIState *s, uint32_t addr, UHCI_TD *td,
+static int uhci_handle_td(UHCIState *s, uint32_t td_addr, UHCI_TD *td,
                           uint32_t *int_mask, bool queuing,
                           struct USBEndpoint **ep_ret)
 {
@@ -792,7 +798,7 @@ static int uhci_handle_td(UHCIState *s, uint32_t addr, UHCI_TD *td,
         return TD_RESULT_NEXT_QH;
     }
 
-    async = uhci_async_find_td(s, addr, td);
+    async = uhci_async_find_td(s, td_addr, td);
     if (async) {
         /* Already submitted */
         async->queue->valid = 32;
@@ -811,7 +817,7 @@ static int uhci_handle_td(UHCIState *s, uint32_t addr, UHCI_TD *td,
     }
 
     /* Allocate new packet */
-    async = uhci_async_alloc(uhci_queue_get(s, td), addr);
+    async = uhci_async_alloc(uhci_queue_get(s, td), td_addr);
 
     /* valid needs to be large enough to handle 10 frame delay
      * for initial isochronous requests
@@ -827,7 +833,7 @@ static int uhci_handle_td(UHCIState *s, uint32_t addr, UHCI_TD *td,
     if (ep_ret) {
         *ep_ret = ep;
     }
-    usb_packet_setup(&async->packet, pid, ep, addr, spd,
+    usb_packet_setup(&async->packet, pid, ep, td_addr, spd,
                      (td->ctrl & TD_CTRL_IOC) != 0);
     qemu_sglist_add(&async->sgl, td->buffer, max_len);
     usb_packet_map(&async->packet, &async->sgl);
-- 
1.7.12.1

[Qemu-devel] [PATCH 08/17] uhci: Add uhci_read_td() helper function

[Hans de Goede]

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

diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index 7589a5b..6d2db7f 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -693,6 +693,15 @@ static USBDevice *uhci_find_device(UHCIState *s, uint8_t addr)
     return NULL;
 }
 
+static void uhci_read_td(UHCIState *s, UHCI_TD *td, uint32_t link)
+{
+    pci_dma_read(&s->dev, link & ~0xf, td, sizeof(*td));
+    le32_to_cpus(&td->link);
+    le32_to_cpus(&td->ctrl);
+    le32_to_cpus(&td->token);
+    le32_to_cpus(&td->buffer);
+}
+
 static int uhci_complete_td(UHCIState *s, UHCI_TD *td, UHCIAsync *async, uint32_t *int_mask)
 {
     int len = 0, max_len, err, ret;
@@ -941,11 +950,7 @@ static void uhci_fill_queue(UHCIState *s, UHCI_TD *td, struct USBEndpoint *ep)
     int ret;
 
     while (is_valid(plink)) {
-        pci_dma_read(&s->dev, plink & ~0xf, &ptd, sizeof(ptd));
-        le32_to_cpus(&ptd.link);
-        le32_to_cpus(&ptd.ctrl);
-        le32_to_cpus(&ptd.token);
-        le32_to_cpus(&ptd.buffer);
+        uhci_read_td(s, &ptd, plink);
         if (!(ptd.ctrl & TD_CTRL_ACTIVE)) {
             break;
         }
@@ -1031,11 +1036,7 @@ static void uhci_process_frame(UHCIState *s)
         }
 
         /* TD */
-        pci_dma_read(&s->dev, link & ~0xf, &td, sizeof(td));
-        le32_to_cpus(&td.link);
-        le32_to_cpus(&td.ctrl);
-        le32_to_cpus(&td.token);
-        le32_to_cpus(&td.buffer);
+        uhci_read_td(s, &td, link);
         trace_usb_uhci_td_load(curr_qh & ~0xf, link & ~0xf, td.ctrl, td.token);
 
         old_td_ctrl = td.ctrl;
-- 
1.7.12.1

[Qemu-devel] [PATCH 09/17] uhci: Make uhci_fill_queue() actually operate on an UHCIQueue

[Hans de Goede]

And move its calling point to handle_td, this removes the ep_ret ugliness,
and prepates the way for further cleanups in the follow-up patches in this
patch-set.

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

diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index 6d2db7f..2bbc6fb 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -161,6 +161,7 @@ typedef struct UHCI_QH {
 } UHCI_QH;
 
 static void uhci_async_cancel(UHCIAsync *async);
+static void uhci_queue_fill(UHCIQueue *q, UHCI_TD *td, struct USBEndpoint *ep);
 
 static inline int32_t uhci_queue_token(UHCI_TD *td)
 {
@@ -784,9 +785,8 @@ out:
     return err;
 }
 
-static int uhci_handle_td(UHCIState *s, uint32_t td_addr, UHCI_TD *td,
-                          uint32_t *int_mask, bool queuing,
-                          struct USBEndpoint **ep_ret)
+static int uhci_handle_td(UHCIState *s, UHCIQueue *q,
+                          UHCI_TD *td, uint32_t td_addr, uint32_t *int_mask)
 {
     UHCIAsync *async;
     int len = 0, max_len;
@@ -794,6 +794,7 @@ static int uhci_handle_td(UHCIState *s, uint32_t td_addr, UHCI_TD *td,
     bool spd;
     USBDevice *dev;
     USBEndpoint *ep;
+    bool queuing = (q != NULL);
 
     /* Is active ? */
     if (!(td->ctrl & TD_CTRL_ACTIVE)) {
@@ -826,7 +827,10 @@ static int uhci_handle_td(UHCIState *s, uint32_t td_addr, UHCI_TD *td,
     }
 
     /* Allocate new packet */
-    async = uhci_async_alloc(uhci_queue_get(s, td), td_addr);
+    if (q == NULL) {
+        q = uhci_queue_get(s, td);
+    }
+    async = uhci_async_alloc(q, td_addr);
 
     /* valid needs to be large enough to handle 10 frame delay
      * for initial isochronous requests
@@ -839,9 +843,6 @@ static int uhci_handle_td(UHCIState *s, uint32_t td_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, td_addr, spd,
                      (td->ctrl & TD_CTRL_IOC) != 0);
     qemu_sglist_add(&async->sgl, td->buffer, max_len);
@@ -870,6 +871,9 @@ static int uhci_handle_td(UHCIState *s, uint32_t td_addr, UHCI_TD *td,
  
     if (len == USB_RET_ASYNC) {
         uhci_async_link(async);
+        if (!queuing) {
+            uhci_queue_fill(q, td, ep);
+        }
         return TD_RESULT_ASYNC_START;
     }
 
@@ -941,24 +945,23 @@ static int qhdb_insert(QhDb *db, uint32_t addr)
     return 0;
 }
 
-static void uhci_fill_queue(UHCIState *s, UHCI_TD *td, struct USBEndpoint *ep)
+static void uhci_queue_fill(UHCIQueue *q, UHCI_TD *td, struct USBEndpoint *ep)
 {
     uint32_t int_mask = 0;
     uint32_t plink = td->link;
-    uint32_t token = uhci_queue_token(td);
     UHCI_TD ptd;
     int ret;
 
     while (is_valid(plink)) {
-        uhci_read_td(s, &ptd, plink);
+        uhci_read_td(q->uhci, &ptd, plink);
         if (!(ptd.ctrl & TD_CTRL_ACTIVE)) {
             break;
         }
-        if (uhci_queue_token(&ptd) != token) {
+        if (uhci_queue_token(&ptd) != q->token) {
             break;
         }
         trace_usb_uhci_td_queue(plink & ~0xf, ptd.ctrl, ptd.token);
-        ret = uhci_handle_td(s, plink, &ptd, &int_mask, true, NULL);
+        ret = uhci_handle_td(q->uhci, q, &ptd, plink, &int_mask);
         if (ret == TD_RESULT_ASYNC_CONT) {
             break;
         }
@@ -973,7 +976,6 @@ 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;
@@ -1040,7 +1042,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, &curr_ep);
+        ret = uhci_handle_td(s, NULL, &td, link, &int_mask);
         if (old_td_ctrl != td.ctrl) {
             /* update the status bits of the TD */
             val = cpu_to_le32(td.ctrl);
@@ -1059,7 +1061,6 @@ 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, curr_ep);
             link = curr_qh ? qh.link : td.link;
             continue;
 
-- 
1.7.12.1

[Qemu-devel] [PATCH 10/17] uhci: Store ep in UHCIQueue

[Hans de Goede]

This avoids the need to repeatedly lookup the device, and ep.

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

diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index 2bbc6fb..8214a21 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -107,6 +107,7 @@ struct UHCIAsync {
 struct UHCIQueue {
     uint32_t  token;
     UHCIState *uhci;
+    USBEndpoint *ep;
     QTAILQ_ENTRY(UHCIQueue) next;
     QTAILQ_HEAD(, UHCIAsync) asyncs;
     int8_t    valid;
@@ -161,7 +162,7 @@ typedef struct UHCI_QH {
 } UHCI_QH;
 
 static void uhci_async_cancel(UHCIAsync *async);
-static void uhci_queue_fill(UHCIQueue *q, UHCI_TD *td, struct USBEndpoint *ep);
+static void uhci_queue_fill(UHCIQueue *q, UHCI_TD *td);
 
 static inline int32_t uhci_queue_token(UHCI_TD *td)
 {
@@ -169,7 +170,7 @@ static inline int32_t uhci_queue_token(UHCI_TD *td)
     return td->token & 0x7ffff;
 }
 
-static UHCIQueue *uhci_queue_get(UHCIState *s, UHCI_TD *td)
+static UHCIQueue *uhci_queue_get(UHCIState *s, UHCI_TD *td, USBEndpoint *ep)
 {
     uint32_t token = uhci_queue_token(td);
     UHCIQueue *queue;
@@ -183,6 +184,7 @@ static UHCIQueue *uhci_queue_get(UHCIState *s, UHCI_TD *td)
     queue = g_new0(UHCIQueue, 1);
     queue->uhci = s;
     queue->token = token;
+    queue->ep = ep;
     QTAILQ_INIT(&queue->asyncs);
     QTAILQ_INSERT_HEAD(&s->queues, queue, next);
     trace_usb_uhci_queue_add(queue->token);
@@ -790,11 +792,9 @@ static int uhci_handle_td(UHCIState *s, UHCIQueue *q,
 {
     UHCIAsync *async;
     int len = 0, max_len;
-    uint8_t pid;
     bool spd;
-    USBDevice *dev;
-    USBEndpoint *ep;
     bool queuing = (q != NULL);
+    uint8_t pid = td->token & 0xff;
 
     /* Is active ? */
     if (!(td->ctrl & TD_CTRL_ACTIVE)) {
@@ -828,7 +828,9 @@ static int uhci_handle_td(UHCIState *s, UHCIQueue *q,
 
     /* Allocate new packet */
     if (q == NULL) {
-        q = uhci_queue_get(s, td);
+        USBDevice *dev = uhci_find_device(s, (td->token >> 8) & 0x7f);
+        USBEndpoint *ep = usb_ep_get(dev, pid, (td->token >> 15) & 0xf);
+        q = uhci_queue_get(s, td, ep);
     }
     async = uhci_async_alloc(q, td_addr);
 
@@ -838,12 +840,8 @@ static int uhci_handle_td(UHCIState *s, UHCIQueue *q,
     async->queue->valid = 32;
 
     max_len = ((td->token >> 21) + 1) & 0x7ff;
-    pid = td->token & 0xff;
     spd = (pid == USB_TOKEN_IN && (td->ctrl & TD_CTRL_SPD) != 0);
-
-    dev = uhci_find_device(s, (td->token >> 8) & 0x7f);
-    ep = usb_ep_get(dev, pid, (td->token >> 15) & 0xf);
-    usb_packet_setup(&async->packet, pid, ep, td_addr, spd,
+    usb_packet_setup(&async->packet, pid, q->ep, td_addr, spd,
                      (td->ctrl & TD_CTRL_IOC) != 0);
     qemu_sglist_add(&async->sgl, td->buffer, max_len);
     usb_packet_map(&async->packet, &async->sgl);
@@ -851,13 +849,13 @@ static int uhci_handle_td(UHCIState *s, UHCIQueue *q,
     switch(pid) {
     case USB_TOKEN_OUT:
     case USB_TOKEN_SETUP:
-        len = usb_handle_packet(dev, &async->packet);
+        len = usb_handle_packet(q->ep->dev, &async->packet);
         if (len >= 0)
             len = max_len;
         break;
 
     case USB_TOKEN_IN:
-        len = usb_handle_packet(dev, &async->packet);
+        len = usb_handle_packet(q->ep->dev, &async->packet);
         break;
 
     default:
@@ -872,7 +870,7 @@ static int uhci_handle_td(UHCIState *s, UHCIQueue *q,
     if (len == USB_RET_ASYNC) {
         uhci_async_link(async);
         if (!queuing) {
-            uhci_queue_fill(q, td, ep);
+            uhci_queue_fill(q, td);
         }
         return TD_RESULT_ASYNC_START;
     }
@@ -945,7 +943,7 @@ static int qhdb_insert(QhDb *db, uint32_t addr)
     return 0;
 }
 
-static void uhci_queue_fill(UHCIQueue *q, UHCI_TD *td, struct USBEndpoint *ep)
+static void uhci_queue_fill(UHCIQueue *q, UHCI_TD *td)
 {
     uint32_t int_mask = 0;
     uint32_t plink = td->link;
@@ -969,7 +967,7 @@ static void uhci_queue_fill(UHCIQueue *q, UHCI_TD *td, struct USBEndpoint *ep)
         assert(int_mask == 0);
         plink = ptd.link;
     }
-    usb_device_flush_ep_queue(ep->dev, ep);
+    usb_device_flush_ep_queue(q->ep->dev, q->ep);
 }
 
 static void uhci_process_frame(UHCIState *s)
-- 
1.7.12.1

[Qemu-devel] [PATCH 11/17] uhci: Immediately free queues on device disconnect

[Hans de Goede]

There is no need to just cancel any in-flight packets, and then wait
for validate-end to clean things up, we can simply clean things up
immediately on device removal.

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

diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index 8214a21..a8b74bd 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -281,16 +281,11 @@ static void uhci_async_validate_end(UHCIState *s)
 
 static void uhci_async_cancel_device(UHCIState *s, USBDevice *dev)
 {
-    UHCIQueue *queue;
-    UHCIAsync *curr, *n;
+    UHCIQueue *queue, *n;
 
-    QTAILQ_FOREACH(queue, &s->queues, next) {
-        QTAILQ_FOREACH_SAFE(curr, &queue->asyncs, next, n) {
-            if (!usb_packet_is_inflight(&curr->packet) ||
-                curr->packet.ep->dev != dev) {
-                continue;
-            }
-            uhci_async_cancel(curr);
+    QTAILQ_FOREACH_SAFE(queue, &s->queues, next, n) {
+        if (queue->ep->dev == dev) {
+            uhci_queue_free(queue, "cancel-device");
         }
     }
 }
-- 
1.7.12.1

[Qemu-devel] [PATCH 12/17] uhci: Verify queue has not been changed by guest

[Hans de Goede]

According to the spec a guest can unlink a qh, and then as soon as frindex
has changed by 1 since the unlink, assume it is idle and re-use it. However
for various reasons, we cannot simply consider a qh as unlinked if we've not
seen it for 1 frame. This means that it is possible for a guest to re-use /
restart the queue while we still see its old state. This patch adds a safety
check for this, and "early" retires queues when they were changed by the guest.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/hcd-uhci.c | 62 ++++++++++++++++++++++++++++++++++++++++---------------
 trace-events      |  2 +-
 2 files changed, 46 insertions(+), 18 deletions(-)

diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index a8b74bd..0984bee 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -105,6 +105,7 @@ struct UHCIAsync {
 };
 
 struct UHCIQueue {
+    uint32_t  qh_addr;
     uint32_t  token;
     UHCIState *uhci;
     USBEndpoint *ep;
@@ -170,20 +171,15 @@ static inline int32_t uhci_queue_token(UHCI_TD *td)
     return td->token & 0x7ffff;
 }
 
-static UHCIQueue *uhci_queue_get(UHCIState *s, UHCI_TD *td, USBEndpoint *ep)
+static UHCIQueue *uhci_queue_new(UHCIState *s, uint32_t qh_addr, UHCI_TD *td,
+                                 USBEndpoint *ep)
 {
-    uint32_t token = uhci_queue_token(td);
     UHCIQueue *queue;
 
-    QTAILQ_FOREACH(queue, &s->queues, next) {
-        if (queue->token == token) {
-            return queue;
-        }
-    }
-
     queue = g_new0(UHCIQueue, 1);
     queue->uhci = s;
-    queue->token = token;
+    queue->qh_addr = qh_addr;
+    queue->token = uhci_queue_token(td);
     queue->ep = ep;
     QTAILQ_INIT(&queue->asyncs);
     QTAILQ_INSERT_HEAD(&s->queues, queue, next);
@@ -191,7 +187,7 @@ static UHCIQueue *uhci_queue_get(UHCIState *s, UHCI_TD *td, USBEndpoint *ep)
     return queue;
 }
 
-static void uhci_queue_free(UHCIQueue *queue)
+static void uhci_queue_free(UHCIQueue *queue, const char *reason)
 {
     UHCIState *s = queue->uhci;
     UHCIAsync *async;
@@ -201,11 +197,35 @@ static void uhci_queue_free(UHCIQueue *queue)
         uhci_async_cancel(async);
     }
 
-    trace_usb_uhci_queue_del(queue->token);
+    trace_usb_uhci_queue_del(queue->token, reason);
     QTAILQ_REMOVE(&s->queues, queue, next);
     g_free(queue);
 }
 
+static UHCIQueue *uhci_queue_find(UHCIState *s, UHCI_TD *td)
+{
+    uint32_t token = uhci_queue_token(td);
+    UHCIQueue *queue;
+
+    QTAILQ_FOREACH(queue, &s->queues, next) {
+        if (queue->token == token) {
+            return queue;
+        }
+    }
+    return NULL;
+}
+
+static bool uhci_queue_verify(UHCIQueue *queue, uint32_t qh_addr, UHCI_TD *td,
+                              uint32_t td_addr, bool queuing)
+{
+    UHCIAsync *first = QTAILQ_FIRST(&queue->asyncs);
+
+    return queue->qh_addr == qh_addr &&
+           queue->token == uhci_queue_token(td) &&
+           (queuing || !(td->ctrl & TD_CTRL_ACTIVE) || first == NULL ||
+            first->td_addr == td_addr);
+}
+
 static UHCIAsync *uhci_async_alloc(UHCIQueue *queue, uint32_t td_addr)
 {
     UHCIAsync *async = g_new0(UHCIAsync, 1);
@@ -274,7 +294,7 @@ static void uhci_async_validate_end(UHCIState *s)
 
     QTAILQ_FOREACH_SAFE(queue, &s->queues, next, n) {
         if (!queue->valid) {
-            uhci_queue_free(queue);
+            uhci_queue_free(queue, "validate-end");
         }
     }
 }
@@ -295,7 +315,7 @@ static void uhci_async_cancel_all(UHCIState *s)
     UHCIQueue *queue, *nq;
 
     QTAILQ_FOREACH_SAFE(queue, &s->queues, next, nq) {
-        uhci_queue_free(queue);
+        uhci_queue_free(queue, "cancel-all");
     }
 }
 
@@ -782,7 +802,7 @@ out:
     return err;
 }
 
-static int uhci_handle_td(UHCIState *s, UHCIQueue *q,
+static int uhci_handle_td(UHCIState *s, UHCIQueue *q, uint32_t qh_addr,
                           UHCI_TD *td, uint32_t td_addr, uint32_t *int_mask)
 {
     UHCIAsync *async;
@@ -791,6 +811,14 @@ static int uhci_handle_td(UHCIState *s, UHCIQueue *q,
     bool queuing = (q != NULL);
     uint8_t pid = td->token & 0xff;
 
+    if (q == NULL) {
+        q = uhci_queue_find(s, td);
+        if (q && !uhci_queue_verify(q, qh_addr, td, td_addr, queuing)) {
+            uhci_queue_free(q, "guest re-used qh");
+            q = NULL;
+        }
+    }
+
     /* Is active ? */
     if (!(td->ctrl & TD_CTRL_ACTIVE)) {
         /*
@@ -825,7 +853,7 @@ static int uhci_handle_td(UHCIState *s, UHCIQueue *q,
     if (q == NULL) {
         USBDevice *dev = uhci_find_device(s, (td->token >> 8) & 0x7f);
         USBEndpoint *ep = usb_ep_get(dev, pid, (td->token >> 15) & 0xf);
-        q = uhci_queue_get(s, td, ep);
+        q = uhci_queue_new(s, qh_addr, td, ep);
     }
     async = uhci_async_alloc(q, td_addr);
 
@@ -954,7 +982,7 @@ static void uhci_queue_fill(UHCIQueue *q, UHCI_TD *td)
             break;
         }
         trace_usb_uhci_td_queue(plink & ~0xf, ptd.ctrl, ptd.token);
-        ret = uhci_handle_td(q->uhci, q, &ptd, plink, &int_mask);
+        ret = uhci_handle_td(q->uhci, q, q->qh_addr, &ptd, plink, &int_mask);
         if (ret == TD_RESULT_ASYNC_CONT) {
             break;
         }
@@ -1035,7 +1063,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, NULL, &td, link, &int_mask);
+        ret = uhci_handle_td(s, NULL, curr_qh, &td, link, &int_mask);
         if (old_td_ctrl != td.ctrl) {
             /* update the status bits of the TD */
             val = cpu_to_le32(td.ctrl);
diff --git a/trace-events b/trace-events
index f5b5097..6647fc4 100644
--- a/trace-events
+++ b/trace-events
@@ -283,7 +283,7 @@ usb_uhci_mmio_writew(uint32_t addr, uint32_t val) "addr 0x%04x, val 0x%04x"
 usb_uhci_mmio_readl(uint32_t addr, uint32_t val) "addr 0x%04x, ret 0x%08x"
 usb_uhci_mmio_writel(uint32_t addr, uint32_t val) "addr 0x%04x, val 0x%08x"
 usb_uhci_queue_add(uint32_t token) "token 0x%x"
-usb_uhci_queue_del(uint32_t token) "token 0x%x"
+usb_uhci_queue_del(uint32_t token, const char *reason) "token 0x%x: %s"
 usb_uhci_packet_add(uint32_t token, uint32_t addr) "token 0x%x, td 0x%x"
 usb_uhci_packet_link_async(uint32_t token, uint32_t addr) "token 0x%x, td 0x%x"
 usb_uhci_packet_unlink_async(uint32_t token, uint32_t addr) "token 0x%x, td 0x%x"
-- 
1.7.12.1

[Qemu-devel] [PATCH 13/17] uhci: Detect guest td re-use

[Hans de Goede]

A td can be reused by the guest in a different queue, before we notice
the original queue has been unlinked. So search for tds by addr only, detect
guest td reuse, and cancel the original queue, this is necessary to keep our
packet ids unique.

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

diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index 0984bee..c4f2f98 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -319,28 +319,18 @@ static void uhci_async_cancel_all(UHCIState *s)
     }
 }
 
-static UHCIAsync *uhci_async_find_td(UHCIState *s, uint32_t td_addr,
-                                     UHCI_TD *td)
+static UHCIAsync *uhci_async_find_td(UHCIState *s, uint32_t td_addr)
 {
-    uint32_t token = uhci_queue_token(td);
     UHCIQueue *queue;
     UHCIAsync *async;
 
     QTAILQ_FOREACH(queue, &s->queues, next) {
-        if (queue->token == token) {
-            break;
-        }
-    }
-    if (queue == NULL) {
-        return NULL;
-    }
-
-    QTAILQ_FOREACH(async, &queue->asyncs, next) {
-        if (async->td_addr == td_addr) {
-            return async;
+        QTAILQ_FOREACH(async, &queue->asyncs, next) {
+            if (async->td_addr == td_addr) {
+                return async;
+            }
         }
     }
-
     return NULL;
 }
 
@@ -805,11 +795,21 @@ out:
 static int uhci_handle_td(UHCIState *s, UHCIQueue *q, uint32_t qh_addr,
                           UHCI_TD *td, uint32_t td_addr, uint32_t *int_mask)
 {
-    UHCIAsync *async;
     int len = 0, max_len;
     bool spd;
     bool queuing = (q != NULL);
     uint8_t pid = td->token & 0xff;
+    UHCIAsync *async = uhci_async_find_td(s, td_addr);
+
+    if (async) {
+        if (uhci_queue_verify(async->queue, qh_addr, td, td_addr, queuing)) {
+            assert(q == NULL || q == async->queue);
+            q = async->queue;
+        } else {
+            uhci_queue_free(async->queue, "guest re-used pending td");
+            async = NULL;
+        }
+    }
 
     if (q == NULL) {
         q = uhci_queue_find(s, td);
@@ -831,7 +831,6 @@ static int uhci_handle_td(UHCIState *s, UHCIQueue *q, uint32_t qh_addr,
         return TD_RESULT_NEXT_QH;
     }
 
-    async = uhci_async_find_td(s, td_addr, td);
     if (async) {
         /* Already submitted */
         async->queue->valid = 32;
-- 
1.7.12.1

[Qemu-devel] [PATCH 14/17] uhci: When the guest marks a pending td non-active, cancel the queue

[Hans de Goede]

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

diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index c4f2f98..592ad8d 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -821,6 +821,10 @@ static int uhci_handle_td(UHCIState *s, UHCIQueue *q, uint32_t qh_addr,
 
     /* Is active ? */
     if (!(td->ctrl & TD_CTRL_ACTIVE)) {
+        if (async) {
+            /* Guest marked a pending td non-active, cancel the queue */
+            uhci_queue_free(async->queue, "pending td non-active");
+        }
         /*
          * ehci11d spec page 22: "Even if the Active bit in the TD is already
          * cleared when the TD is fetched ... an IOC interrupt is generated"
-- 
1.7.12.1

[Qemu-devel] [PATCH 15/17] uhci: Always mark a queue valid when we encounter it

[Hans de Goede]

Before this patch we would not mark a queue valid when its head was a
non-active td. This causes us to misbehave in the following scenario:

1) queue with multiple input transfers queued
2) We hit some latency issue, causing qemu to get behind processing frames
3) When qemu gets to run again, it notices the first transfer ends short,
   marking the head td non-active
4) It now processes 32+ frames in a row without giving the guest a chance
   to run since it is behind
5) valid is decreased to 0, causing the queue to get cancelled also cancelling
   already queued up further input transfers
6) guest gets to run, notices the inactive td, cleanups up further tds
   from the short transfer, and lets the queue continue at the first td of
   the next input transfer
7) we re-start the queue, issuing the second input transfer for the *second*
   time, and any data read by the first time we issued it has been lost

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

diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index 592ad8d..beeb3fd 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -183,6 +183,9 @@ static UHCIQueue *uhci_queue_new(UHCIState *s, uint32_t qh_addr, UHCI_TD *td,
     queue->ep = ep;
     QTAILQ_INIT(&queue->asyncs);
     QTAILQ_INSERT_HEAD(&s->queues, queue, next);
+    /* valid needs to be large enough to handle 10 frame delay
+     * for initial isochronous requests */
+    queue->valid = 32;
     trace_usb_uhci_queue_add(queue->token);
     return queue;
 }
@@ -819,6 +822,10 @@ static int uhci_handle_td(UHCIState *s, UHCIQueue *q, uint32_t qh_addr,
         }
     }
 
+    if (q) {
+        q->valid = 32;
+    }
+
     /* Is active ? */
     if (!(td->ctrl & TD_CTRL_ACTIVE)) {
         if (async) {
@@ -836,9 +843,6 @@ static int uhci_handle_td(UHCIState *s, UHCIQueue *q, uint32_t qh_addr,
     }
 
     if (async) {
-        /* Already submitted */
-        async->queue->valid = 32;
-
         if (!async->done)
             return TD_RESULT_ASYNC_CONT;
         if (queuing) {
@@ -860,11 +864,6 @@ static int uhci_handle_td(UHCIState *s, UHCIQueue *q, uint32_t qh_addr,
     }
     async = uhci_async_alloc(q, td_addr);
 
-    /* valid needs to be large enough to handle 10 frame delay
-     * for initial isochronous requests
-     */
-    async->queue->valid = 32;
-
     max_len = ((td->token >> 21) + 1) & 0x7ff;
     spd = (pid == USB_TOKEN_IN && (td->ctrl & TD_CTRL_SPD) != 0);
     usb_packet_setup(&async->packet, pid, q->ep, td_addr, spd,
-- 
1.7.12.1

[Qemu-devel] [PATCH 16/17] uhci: Retry to fill the queue while waiting for td completion

[Hans de Goede]

If the guest is using multiple transfers to try and keep the usb bus busy /
used at maximum efficiency, currently we would see / do the following:

1) submit transfer 1 to the device
2) submit transfer 2 to the device
3) report transfer 1 completion to guest
4) report transfer 2 completion to guest
5) submit transfer 1 to the device
6) report transfer 1 completion to guest
7) submit transfer 2 to the device
8) report transfer 2 completion to guest
etc.

So after the initial submission we would effectively only have 1 transfer
in flight, rather then 2. This is caused by us not checking the queue for
addition of new transfers by the guest (ie the resubmission of a recently
finished transfer), while waiting for a pending transfer to complete.
This patch does add a check for this, changing the sequence to:

1) submit transfer 1 to the device
2) submit transfer 2 to the device
3) report transfer 1 completion to guest
4) submit transfer 1 to the device
5) report transfer 2 completion to guest
6) submit transfer 2 to the device
etc.

Thus keeping 2 transfers in flight (most of the time, and always 1),
as intended by the guest.

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

diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index beeb3fd..a9e06ef 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -110,7 +110,7 @@ struct UHCIQueue {
     UHCIState *uhci;
     USBEndpoint *ep;
     QTAILQ_ENTRY(UHCIQueue) next;
-    QTAILQ_HEAD(, UHCIAsync) asyncs;
+    QTAILQ_HEAD(asyncs_head, UHCIAsync) asyncs;
     int8_t    valid;
 };
 
@@ -843,15 +843,25 @@ static int uhci_handle_td(UHCIState *s, UHCIQueue *q, uint32_t qh_addr,
     }
 
     if (async) {
-        if (!async->done)
-            return TD_RESULT_ASYNC_CONT;
         if (queuing) {
             /* we are busy filling the queue, we are not prepared
                to consume completed packages then, just leave them
                in async state */
             return TD_RESULT_ASYNC_CONT;
         }
+        if (!async->done) {
+            UHCI_TD last_td;
+            UHCIAsync *last = QTAILQ_LAST(&async->queue->asyncs, asyncs_head);
+            /*
+             * While we are waiting for the current td to complete, the guest
+             * may have added more tds to the queue. Note we re-read the td
+             * rather then caching it, as we want to see guest made changes!
+             */
+            uhci_read_td(s, &last_td, last->td_addr);
+            uhci_queue_fill(async->queue, &last_td);
 
+            return TD_RESULT_ASYNC_CONT;
+        }
         uhci_async_unlink(async);
         goto done;
     }
-- 
1.7.12.1

[Qemu-devel] [PATCH 17/17] uhci: Use only one queue for ctrl endpoints

[Hans de Goede]

ctrl endpoints use different pids for different phases of a control
transfer, this patch makes us use only one queue for a ctrl ep, rather
then 3.

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

diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index a9e06ef..b6b972f 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -167,8 +167,13 @@ static void uhci_queue_fill(UHCIQueue *q, UHCI_TD *td);
 
 static inline int32_t uhci_queue_token(UHCI_TD *td)
 {
-    /* covers ep, dev, pid -> identifies the endpoint */
-    return td->token & 0x7ffff;
+    if ((td->token & (0xf << 15)) == 0) {
+        /* ctrl ep, cover ep and dev, not pid! */
+        return td->token & 0x7ff00;
+    } else {
+        /* covers ep, dev, pid -> identifies the endpoint */
+        return td->token & 0x7ffff;
+    }
 }
 
 static UHCIQueue *uhci_queue_new(UHCIState *s, uint32_t qh_addr, UHCI_TD *td,
-- 
1.7.12.1

Re: [Qemu-devel] uhci: Cleanups, fixes and improvements

[Gerd Hoffmann]

On 10/24/12 18:31, Hans de Goede wrote:
> Here is a patchset with assorted uhci cleanups / fixes / improvments,
> please add these to your tree for Anthony.
> 
> Regards,
> 
> Hans

Patch added to usb patch queue.

thanks,
  Gerd