[Qemu-devel] RFC: usb input pipelining / speedup patchset v2

[Qemu-devel] RFC: usb input pipelining / speedup patchset v2

[Hans de Goede]

Here is v2 of my usb input pipelining and speedup patchset + some assorted
fixes, taking into account all the remarks from the discussion surrounding
v1, Please review!

Note this is still RFC status as I would like to run some more tests before
this going upstream.

Thanks & Regards,

Hans

[Qemu-devel] [PATCH 01/22] 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 c2f08e3..671c712 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);
 }
 
@@ -887,6 +888,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 02/22] 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 671c712..600d095 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -991,7 +991,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);
@@ -1010,9 +1011,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;
     }
 }
@@ -1110,9 +1108,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 03/22] 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 04/22] 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 05/22] 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 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 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 d9d4918..bbfa441 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -1244,7 +1244,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;
 
@@ -2509,12 +2509,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,
@@ -2743,7 +2737,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 06/22] 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 bbfa441..58e788b 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) \
@@ -1529,6 +1530,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;
+        }
     }
 }
 
@@ -2503,8 +2507,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

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

[Gerd Hoffmann]

On 10/15/12 12:38, Hans de Goede wrote:
> 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.

Why not just set async_stepdown to 0?

cheers,
  Gerd

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

[Hans de Goede]

Hi,

On 10/15/2012 01:17 PM, Gerd Hoffmann wrote:
> On 10/15/12 12:38, Hans de Goede wrote:
>> 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.
>
> Why not just set async_stepdown to 0?

We already do that whenever we run a package completion (it get sets when
we move to the executing stage). What this patch does is request the
frame timer to run again in 500 usecs instead of after 1 ms, thus making
us see and process async transfers faster when they are queued up in
response to just completed packages (which we've told the guest about with
the int interrupt). This makes the USB-bus / device idle time between
any 2 transfers of the 3 transfer involving USB storage BOT time shorter,
thereby speeding things up.

Regards,

Hans

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

[Gerd Hoffmann]

On 10/15/12 15:00, Hans de Goede wrote:
> Hi,
> 
> On 10/15/2012 01:17 PM, Gerd Hoffmann wrote:
>> On 10/15/12 12:38, Hans de Goede wrote:
>>> 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.
>>
>> Why not just set async_stepdown to 0?
> 
> We already do that whenever we run a package completion (it get sets when
> we move to the executing stage). What this patch does is request the
> frame timer to run again in 500 usecs instead of after 1 ms, thus making
> us see and process async transfers faster when they are queued up in
> response to just completed packages (which we've told the guest about with
> the int interrupt). This makes the USB-bus / device idle time between
> any 2 transfers of the 3 transfer involving USB storage BOT time shorter,
> thereby speeding things up.

Don't feel like having two mechanisms for wakeup rate control.  Can't we
integrate this with async_stepdown?  Changing the baseline maybe, so
stepdown=0 doesn't mean 1000 Hz but 2000 Hz?

cheers,
  Gerd

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

[Hans de Goede]

Hi,

On 10/17/2012 01:01 PM, Gerd Hoffmann wrote:
> On 10/15/12 15:00, Hans de Goede wrote:
>> Hi,
>>
>> On 10/15/2012 01:17 PM, Gerd Hoffmann wrote:
>>> On 10/15/12 12:38, Hans de Goede wrote:
>>>> 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.
>>>
>>> Why not just set async_stepdown to 0?
>>
>> We already do that whenever we run a package completion (it get sets when
>> we move to the executing stage). What this patch does is request the
>> frame timer to run again in 500 usecs instead of after 1 ms, thus making
>> us see and process async transfers faster when they are queued up in
>> response to just completed packages (which we've told the guest about with
>> the int interrupt). This makes the USB-bus / device idle time between
>> any 2 transfers of the 3 transfer involving USB storage BOT time shorter,
>> thereby speeding things up.
>
> Don't feel like having two mechanisms for wakeup rate control.  Can't we
> integrate this with async_stepdown?  Changing the baseline maybe, so
> stepdown=0 doesn't mean 1000 Hz but 2000 Hz?

That is actually close to what I wanted to do at first (I wanted to use
stepdown=-1 for the faster wakeup case). But there are 2 problems with this:

1) It causes migration issues when migrating to / from an old version
2) We don't want to change the wakeup rate when the interrupt flag gets set
as pending, but when it actually gets committed, and we only want to change
the wakeup rate when the int was requested by an async packet, not when it
was requested by a periodic packet, so we will need the int_req_by_async
flag anyways, as which point this seemed the cleanest way.

Regards,

Hans

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

[Gerd Hoffmann]

  Hi,

> 1) It causes migration issues when migrating to / from an old version

With -1 yes, shifting the scale shoudn't be a that big issue though as
it is just a optimization.

> 2) We don't want to change the wakeup rate when the interrupt flag gets set
> as pending, but when it actually gets committed, and we only want to change
> the wakeup rate when the int was requested by an async packet, not when it
> was requested by a periodic packet, so we will need the int_req_by_async
> flag anyways, as which point this seemed the cleanest way.

Missed that little details, ok then.

cheers,
  Gerd

[Qemu-devel] [PATCH 07/22] ehci: Detect going in circles when filling the queue

[Hans de Goede]

For ctrl endpoints Windows (atleast Win7) creates circular td lists, so far
these were not a problem because we would stop filling the queue if altnext
was set. Since further patches in this patchset remove the altnext check this
does become a problem and we need detection for going in circles.

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

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 58e788b..79a9ad5 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -2072,7 +2072,7 @@ static int ehci_fill_queue(EHCIPacket *p)
 {
     EHCIQueue *q = p->queue;
     EHCIqtd qtd = p->qtd;
-    uint32_t qtdaddr;
+    uint32_t qtdaddr, start_addr = p->qtdaddr;
 
     for (;;) {
         if (NLPTR_TBIT(qtd.altnext) == 0) {
@@ -2082,6 +2082,13 @@ static int ehci_fill_queue(EHCIPacket *p)
             break;
         }
         qtdaddr = qtd.next;
+        /*
+         * Detect circular td lists, Windows creates these, counting on the
+         * active bit going low after exection to make the queue stop.
+         */
+        if (qtdaddr == start_addr) {
+            break;
+        }
         get_dwords(q->ehci, NLPTR_GET(qtdaddr),
                    (uint32_t *) &qtd, sizeof(EHCIqtd) >> 2);
         ehci_trace_qtd(q, NLPTR_GET(qtdaddr), &qtd);
-- 
1.7.12.1

[Qemu-devel] [PATCH 08/22] xhci: Add a xhci_ep_nuke_one_xfer helper function

[Hans de Goede]

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

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index e0ca690..0d90578 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -1081,6 +1081,35 @@ static TRBCCode xhci_enable_ep(XHCIState *xhci, unsigned int slotid,
     return CC_SUCCESS;
 }
 
+static int xhci_ep_nuke_one_xfer(XHCITransfer *t)
+{
+    int killed = 0;
+
+    if (t->running_async) {
+        usb_cancel_packet(&t->packet);
+        t->running_async = 0;
+        t->cancelled = 1;
+        DPRINTF("xhci: cancelling transfer %d, waiting for it to complete...\n", i);
+        killed = 1;
+    }
+    if (t->running_retry) {
+        XHCIEPContext *epctx = t->xhci->slots[t->slotid-1].eps[t->epid-1];
+        if (epctx) {
+            epctx->retry = NULL;
+            qemu_del_timer(epctx->kick_timer);
+        }
+        t->running_retry = 0;
+    }
+    if (t->trbs) {
+        g_free(t->trbs);
+    }
+
+    t->trbs = NULL;
+    t->trb_count = t->trb_alloced = 0;
+
+    return killed;
+}
+
 static int xhci_ep_nuke_xfers(XHCIState *xhci, unsigned int slotid,
                                unsigned int epid)
 {
@@ -1102,25 +1131,7 @@ static int xhci_ep_nuke_xfers(XHCIState *xhci, unsigned int slotid,
 
     xferi = epctx->next_xfer;
     for (i = 0; i < TD_QUEUE; i++) {
-        XHCITransfer *t = &epctx->transfers[xferi];
-        if (t->running_async) {
-            usb_cancel_packet(&t->packet);
-            t->running_async = 0;
-            t->cancelled = 1;
-            DPRINTF("xhci: cancelling transfer %d, waiting for it to complete...\n", i);
-            killed++;
-        }
-        if (t->running_retry) {
-            t->running_retry = 0;
-            epctx->retry = NULL;
-            qemu_del_timer(epctx->kick_timer);
-        }
-        if (t->trbs) {
-            g_free(t->trbs);
-        }
-
-        t->trbs = NULL;
-        t->trb_count = t->trb_alloced = 0;
+        killed += xhci_ep_nuke_one_xfer(&epctx->transfers[xferi]);
         xferi = (xferi + 1) % TD_QUEUE;
     }
     return killed;
-- 
1.7.12.1

[Qemu-devel] [PATCH 09/22] usb: Rename __usb_packet_complete to usb_packet_complete_one

[Hans de Goede]

And make it available for use outside of core.c

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb.h      | 1 +
 hw/usb/core.c | 8 ++++----
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/usb.h b/hw/usb.h
index 48c8926..01dd423 100644
--- a/hw/usb.h
+++ b/hw/usb.h
@@ -370,6 +370,7 @@ USBDevice *usb_find_device(USBPort *port, uint8_t addr);
 
 int usb_handle_packet(USBDevice *dev, USBPacket *p);
 void usb_packet_complete(USBDevice *dev, USBPacket *p);
+void usb_packet_complete_one(USBDevice *dev, USBPacket *p);
 void usb_cancel_packet(USBPacket * p);
 
 void usb_ep_init(USBDevice *dev);
diff --git a/hw/usb/core.c b/hw/usb/core.c
index b9f1f7a..e2e31ca 100644
--- a/hw/usb/core.c
+++ b/hw/usb/core.c
@@ -412,10 +412,11 @@ int usb_handle_packet(USBDevice *dev, USBPacket *p)
     return ret;
 }
 
-static void __usb_packet_complete(USBDevice *dev, USBPacket *p)
+void usb_packet_complete_one(USBDevice *dev, USBPacket *p)
 {
     USBEndpoint *ep = p->ep;
 
+    assert(QTAILQ_FIRST(&ep->queue) == p);
     assert(p->result != USB_RET_ASYNC && p->result != USB_RET_NAK);
 
     if (p->result < 0) {
@@ -435,8 +436,7 @@ void usb_packet_complete(USBDevice *dev, USBPacket *p)
     int ret;
 
     usb_packet_check_state(p, USB_PACKET_ASYNC);
-    assert(QTAILQ_FIRST(&ep->queue) == p);
-    __usb_packet_complete(dev, p);
+    usb_packet_complete_one(dev, p);
 
     while (!ep->halted && !QTAILQ_EMPTY(&ep->queue)) {
         p = QTAILQ_FIRST(&ep->queue);
@@ -450,7 +450,7 @@ void usb_packet_complete(USBDevice *dev, USBPacket *p)
             break;
         }
         p->result = ret;
-        __usb_packet_complete(ep->dev, p);
+        usb_packet_complete_one(ep->dev, p);
     }
 }
 
-- 
1.7.12.1

[Qemu-devel] [PATCH 10/22] usb: Add USB_RET_ADD_TO_QUEUE packet result code

[Hans de Goede]

This can be used by usb-device code which wishes to process an entire endpoint
queue at once, to do this the usb-device code returns USB_RET_ADD_TO_QUEUE
from its handle_data class method and defines a flush_ep_queue class method
to call when the hcd is done queuing up packets.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb.h          | 21 +++++++++++++++------
 hw/usb/bus.c      |  8 ++++++++
 hw/usb/core.c     |  4 ++++
 hw/usb/hcd-ehci.c |  4 ++++
 hw/usb/hcd-musb.c |  1 +
 hw/usb/hcd-ohci.c |  2 ++
 hw/usb/hcd-uhci.c | 16 +++++++++++-----
 hw/usb/hcd-xhci.c |  6 ++++++
 8 files changed, 51 insertions(+), 11 deletions(-)

diff --git a/hw/usb.h b/hw/usb.h
index 01dd423..435cd42 100644
--- a/hw/usb.h
+++ b/hw/usb.h
@@ -38,12 +38,13 @@
 #define USB_TOKEN_IN    0x69 /* device -> host */
 #define USB_TOKEN_OUT   0xe1 /* host -> device */
 
-#define USB_RET_NODEV   (-1)
-#define USB_RET_NAK     (-2)
-#define USB_RET_STALL   (-3)
-#define USB_RET_BABBLE  (-4)
-#define USB_RET_IOERROR (-5)
-#define USB_RET_ASYNC   (-6)
+#define USB_RET_NODEV             (-1)
+#define USB_RET_NAK               (-2)
+#define USB_RET_STALL             (-3)
+#define USB_RET_BABBLE            (-4)
+#define USB_RET_IOERROR           (-5)
+#define USB_RET_ASYNC             (-6)
+#define USB_RET_ADD_TO_QUEUE      (-7)
 
 #define USB_SPEED_LOW   0
 #define USB_SPEED_FULL  1
@@ -293,6 +294,12 @@ typedef struct USBDeviceClass {
     void (*set_interface)(USBDevice *dev, int interface,
                           int alt_old, int alt_new);
 
+    /*
+     * Called when the hcd is done queuing packets for an endpoint, only
+     * necessary for devices which can return USB_RET_ADD_TO_QUEUE.
+     */
+    void (*flush_ep_queue)(USBDevice *dev, USBEndpoint *ep);
+
     const char *product_desc;
     const USBDesc *usb_desc;
 } USBDeviceClass;
@@ -507,6 +514,8 @@ int usb_device_handle_data(USBDevice *dev, USBPacket *p);
 void usb_device_set_interface(USBDevice *dev, int interface,
                               int alt_old, int alt_new);
 
+void usb_device_flush_ep_queue(USBDevice *dev, USBEndpoint *ep);
+
 const char *usb_device_get_product_desc(USBDevice *dev);
 
 const USBDesc *usb_device_get_usb_desc(USBDevice *dev);
diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index b649360..8066291 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -181,6 +181,14 @@ void usb_device_set_interface(USBDevice *dev, int interface,
     }
 }
 
+void usb_device_flush_ep_queue(USBDevice *dev, USBEndpoint *ep)
+{
+    USBDeviceClass *klass = USB_DEVICE_GET_CLASS(dev);
+    if (klass->flush_ep_queue) {
+        klass->flush_ep_queue(dev, ep);
+    }
+}
+
 static int usb_qdev_init(DeviceState *qdev)
 {
     USBDevice *dev = USB_DEVICE(qdev);
diff --git a/hw/usb/core.c b/hw/usb/core.c
index e2e31ca..014e3ac 100644
--- a/hw/usb/core.c
+++ b/hw/usb/core.c
@@ -393,6 +393,10 @@ int usb_handle_packet(USBDevice *dev, USBPacket *p)
         if (ret == USB_RET_ASYNC) {
             usb_packet_set_state(p, USB_PACKET_ASYNC);
             QTAILQ_INSERT_TAIL(&p->ep->queue, p, queue);
+        } else if (ret == USB_RET_ADD_TO_QUEUE) {
+            usb_packet_set_state(p, USB_PACKET_QUEUED);
+            QTAILQ_INSERT_TAIL(&p->ep->queue, p, queue);
+            ret = USB_RET_ASYNC;
         } else {
             /*
              * When pipelining is enabled usb-devices must always return async,
diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 79a9ad5..29365d9 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -2070,6 +2070,7 @@ static int ehci_state_horizqh(EHCIQueue *q)
 
 static int ehci_fill_queue(EHCIPacket *p)
 {
+    USBEndpoint *ep = p->packet.ep;
     EHCIQueue *q = p->queue;
     EHCIqtd qtd = p->qtd;
     uint32_t qtdaddr, start_addr = p->qtdaddr;
@@ -2105,6 +2106,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_device_flush_ep_queue(ep->dev, ep);
+    }
     return p->usb_status;
 }
 
diff --git a/hw/usb/hcd-musb.c b/hw/usb/hcd-musb.c
index 0bb5c7b..4896d38 100644
--- a/hw/usb/hcd-musb.c
+++ b/hw/usb/hcd-musb.c
@@ -635,6 +635,7 @@ static void musb_packet(MUSBState *s, MUSBEndPoint *ep,
     ret = usb_handle_packet(dev, &ep->packey[dir].p);
 
     if (ret == USB_RET_ASYNC) {
+        usb_device_flush_ep_queue(dev, uep);
         ep->status[dir] = len;
         return;
     }
diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index c36184a..a3207f0 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -816,6 +816,7 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed,
         usb_packet_addbuf(&ohci->usb_packet, ohci->usb_buf, len);
         ret = usb_handle_packet(dev, &ohci->usb_packet);
         if (ret == USB_RET_ASYNC) {
+            usb_device_flush_ep_queue(dev, ep);
             return 1;
         }
     }
@@ -1018,6 +1019,7 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed)
         DPRINTF("ret=%d\n", ret);
 #endif
         if (ret == USB_RET_ASYNC) {
+            usb_device_flush_ep_queue(dev, ep);
             ohci->async_td = addr;
             return 1;
         }
diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index 600d095..46e544b 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -818,7 +818,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;
@@ -870,6 +871,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);
@@ -983,7 +987,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;
@@ -1005,7 +1009,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;
         }
@@ -1013,12 +1017,14 @@ static void uhci_fill_queue(UHCIState *s, UHCI_TD *td)
         assert(int_mask == 0);
         plink = ptd.link;
     }
+    usb_device_flush_ep_queue(ep->dev, 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;
@@ -1089,7 +1095,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);
@@ -1108,7 +1114,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;
 
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 0d90578..53e1ea3 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -1661,6 +1661,7 @@ static int xhci_fire_transfer(XHCIState *xhci, XHCITransfer *xfer, XHCIEPContext
 static void xhci_kick_ep(XHCIState *xhci, unsigned int slotid, unsigned int epid)
 {
     XHCIEPContext *epctx;
+    USBEndpoint *ep = NULL;
     uint64_t mfindex;
     int length;
     int i;
@@ -1754,12 +1755,14 @@ static void xhci_kick_ep(XHCIState *xhci, unsigned int slotid, unsigned int epid
         if (epid == 1) {
             if (xhci_fire_ctl_transfer(xhci, xfer) >= 0) {
                 epctx->next_xfer = (epctx->next_xfer + 1) % TD_QUEUE;
+                ep = xfer->packet.ep;
             } else {
                 fprintf(stderr, "xhci: error firing CTL transfer\n");
             }
         } else {
             if (xhci_fire_transfer(xhci, xfer, epctx) >= 0) {
                 epctx->next_xfer = (epctx->next_xfer + 1) % TD_QUEUE;
+                ep = xfer->packet.ep;
             } else {
                 if (!xfer->iso_xfer) {
                     fprintf(stderr, "xhci: error firing data transfer\n");
@@ -1776,6 +1779,9 @@ static void xhci_kick_ep(XHCIState *xhci, unsigned int slotid, unsigned int epid
             break;
         }
     }
+    if (ep) {
+        usb_device_flush_ep_queue(ep->dev, ep);
+    }
 }
 
 static TRBCCode xhci_enable_slot(XHCIState *xhci, unsigned int slotid)
-- 
1.7.12.1

[Qemu-devel] [PATCH 11/22] usb: Move clearing of queue on halt to the core

[Hans de Goede]

hcds which queue up more then one packet at once (uhci, ehci and xhci),
must clear the queue after an error which has caused the queue to halt.

Currently this is handled as a special case inside the hcd code, this
patch instead adds an USB_RET_REMOVE_FROM_QUEUE packet result code, teaches
the 3 hcds about this and moves the clearing of the queue on a halt into
the USB core.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb.h          |  1 +
 hw/usb/core.c     |  8 +++++++-
 hw/usb/hcd-ehci.c | 22 ++++++++--------------
 hw/usb/hcd-uhci.c | 22 ++++++----------------
 hw/usb/hcd-xhci.c |  4 ++++
 5 files changed, 26 insertions(+), 31 deletions(-)

diff --git a/hw/usb.h b/hw/usb.h
index 435cd42..ead03c9 100644
--- a/hw/usb.h
+++ b/hw/usb.h
@@ -45,6 +45,7 @@
 #define USB_RET_IOERROR           (-5)
 #define USB_RET_ASYNC             (-6)
 #define USB_RET_ADD_TO_QUEUE      (-7)
+#define USB_RET_REMOVE_FROM_QUEUE (-8)
 
 #define USB_SPEED_LOW   0
 #define USB_SPEED_FULL  1
diff --git a/hw/usb/core.c b/hw/usb/core.c
index 014e3ac..5a97a0e 100644
--- a/hw/usb/core.c
+++ b/hw/usb/core.c
@@ -442,8 +442,14 @@ void usb_packet_complete(USBDevice *dev, USBPacket *p)
     usb_packet_check_state(p, USB_PACKET_ASYNC);
     usb_packet_complete_one(dev, p);
 
-    while (!ep->halted && !QTAILQ_EMPTY(&ep->queue)) {
+    while (!QTAILQ_EMPTY(&ep->queue)) {
         p = QTAILQ_FIRST(&ep->queue);
+        if (ep->halted) {
+            /* Empty the queue on a halt */
+            p->result = USB_RET_REMOVE_FROM_QUEUE;
+            dev->port->ops->complete(dev->port, p);
+            continue;
+        }
         if (p->state == USB_PACKET_ASYNC) {
             break;
         }
diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 29365d9..e6f7642 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -1456,8 +1456,15 @@ static void ehci_async_complete_packet(USBPort *port, USBPacket *packet)
     }
 
     p = container_of(packet, EHCIPacket, packet);
-    trace_usb_ehci_packet_action(p->queue, p, "wakeup");
     assert(p->async == EHCI_ASYNC_INFLIGHT);
+
+    if (packet->result == USB_RET_REMOVE_FROM_QUEUE) {
+        trace_usb_ehci_packet_action(p->queue, p, "remove");
+        ehci_free_packet(p);
+        return;
+    }
+
+    trace_usb_ehci_packet_action(p->queue, p, "wakeup");
     p->async = EHCI_ASYNC_FINISHED;
     p->usb_status = packet->result;
 
@@ -2214,19 +2221,6 @@ 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 46e544b..00dc9d5 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -744,22 +744,6 @@ 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;
@@ -918,6 +902,12 @@ static void uhci_async_complete(USBPort *port, USBPacket *packet)
     UHCIAsync *async = container_of(packet, UHCIAsync, packet);
     UHCIState *s = async->queue->uhci;
 
+    if (packet->result == USB_RET_REMOVE_FROM_QUEUE) {
+        uhci_async_unlink(async);
+        uhci_async_cancel(async);
+        return;
+    }
+
     if (async->isoc) {
         UHCI_TD td;
         uint32_t link = async->td;
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 53e1ea3..5648cc7 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -2834,6 +2834,10 @@ static void xhci_complete(USBPort *port, USBPacket *packet)
 {
     XHCITransfer *xfer = container_of(packet, XHCITransfer, packet);
 
+    if (packet->result == USB_RET_REMOVE_FROM_QUEUE) {
+        xhci_ep_nuke_one_xfer(xfer);
+        return;
+    }
     xhci_complete_packet(xfer, packet->result);
     xhci_kick_ep(xfer->xhci, xfer->slotid, xfer->epid);
 }
-- 
1.7.12.1

[Qemu-devel] [PATCH 12/22] usb: Move short-not-ok handling to the core

[Hans de Goede]

After a short-not-ok packet ending short, we should not advance the queue.
Move enforcing this to the core, rather then handling it in the hcd code.

This may result in the queue now actually containing multiple input packets
(which would not happen before), and this requires special handling in
combination with pipelining, so disable pipleining for input endpoints
(for now).

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb.h            |  4 +++-
 hw/usb/core.c       |  6 ++++--
 hw/usb/hcd-ehci.c   |  9 ++++-----
 hw/usb/hcd-musb.c   |  2 +-
 hw/usb/hcd-ohci.c   |  4 ++--
 hw/usb/hcd-uhci.c   |  7 ++++---
 hw/usb/hcd-xhci.c   |  2 +-
 hw/usb/host-linux.c |  3 ++-
 hw/usb/redirect.c   | 18 ++++++++++++------
 9 files changed, 33 insertions(+), 22 deletions(-)

diff --git a/hw/usb.h b/hw/usb.h
index ead03c9..1fcf79c 100644
--- a/hw/usb.h
+++ b/hw/usb.h
@@ -351,6 +351,7 @@ struct USBPacket {
     USBEndpoint *ep;
     QEMUIOVector iov;
     uint64_t parameter; /* control transfers */
+    bool short_not_ok;
     int result; /* transfer length or USB_RET_* status code */
     /* Internal use by the USB layer.  */
     USBPacketState state;
@@ -360,7 +361,8 @@ struct USBPacket {
 void usb_packet_init(USBPacket *p);
 void usb_packet_set_state(USBPacket *p, USBPacketState state);
 void usb_packet_check_state(USBPacket *p, USBPacketState expected);
-void usb_packet_setup(USBPacket *p, int pid, USBEndpoint *ep, uint64_t id);
+void usb_packet_setup(USBPacket *p, int pid, USBEndpoint *ep, uint64_t id,
+                      bool short_not_ok);
 void usb_packet_addbuf(USBPacket *p, void *ptr, size_t len);
 int usb_packet_map(USBPacket *p, QEMUSGList *sgl);
 void usb_packet_unmap(USBPacket *p, QEMUSGList *sgl);
diff --git a/hw/usb/core.c b/hw/usb/core.c
index 5a97a0e..f4a5ad2 100644
--- a/hw/usb/core.c
+++ b/hw/usb/core.c
@@ -423,7 +423,7 @@ void usb_packet_complete_one(USBDevice *dev, USBPacket *p)
     assert(QTAILQ_FIRST(&ep->queue) == p);
     assert(p->result != USB_RET_ASYNC && p->result != USB_RET_NAK);
 
-    if (p->result < 0) {
+    if (p->result < 0 || (p->short_not_ok && (p->result < p->iov.size))) {
         ep->halted = true;
     }
     usb_packet_set_state(p, USB_PACKET_COMPLETE);
@@ -532,7 +532,8 @@ void usb_packet_set_state(USBPacket *p, USBPacketState state)
     p->state = state;
 }
 
-void usb_packet_setup(USBPacket *p, int pid, USBEndpoint *ep, uint64_t id)
+void usb_packet_setup(USBPacket *p, int pid, USBEndpoint *ep, uint64_t id,
+                      bool short_not_ok)
 {
     assert(!usb_packet_is_inflight(p));
     assert(p->iov.iov != NULL);
@@ -541,6 +542,7 @@ void usb_packet_setup(USBPacket *p, int pid, USBEndpoint *ep, uint64_t id)
     p->ep = ep;
     p->result = 0;
     p->parameter = 0;
+    p->short_not_ok = short_not_ok;
     qemu_iovec_reset(&p->iov);
     usb_packet_set_state(p, USB_PACKET_SETUP);
 }
diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index e6f7642..4dfe0f3 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -1550,6 +1550,7 @@ static int ehci_execute(EHCIPacket *p, const char *action)
     USBEndpoint *ep;
     int ret;
     int endp;
+    bool spd;
 
     assert(p->async == EHCI_ASYNC_NONE ||
            p->async == EHCI_ASYNC_INITIALIZED);
@@ -1589,7 +1590,8 @@ static int ehci_execute(EHCIPacket *p, const char *action)
             return USB_RET_PROCERR;
         }
 
-        usb_packet_setup(&p->packet, p->pid, ep, p->qtdaddr);
+        spd = (p->pid == USB_TOKEN_IN && NLPTR_TBIT(p->qtd.altnext) == 0);
+        usb_packet_setup(&p->packet, p->pid, ep, p->qtdaddr, spd);
         usb_packet_map(&p->packet, &p->sgl);
         p->async = EHCI_ASYNC_INITIALIZED;
     }
@@ -1659,7 +1661,7 @@ static int ehci_process_itd(EHCIState *ehci,
             dev = ehci_find_device(ehci, devaddr);
             ep = usb_ep_get(dev, pid, endp);
             if (ep && ep->type == USB_ENDPOINT_XFER_ISOC) {
-                usb_packet_setup(&ehci->ipacket, pid, ep, addr);
+                usb_packet_setup(&ehci->ipacket, pid, ep, addr, false);
                 usb_packet_map(&ehci->ipacket, &ehci->isgl);
                 ret = usb_handle_packet(dev, &ehci->ipacket);
                 assert(ret != USB_RET_ASYNC);
@@ -2083,9 +2085,6 @@ static int ehci_fill_queue(EHCIPacket *p)
     uint32_t qtdaddr, start_addr = p->qtdaddr;
 
     for (;;) {
-        if (NLPTR_TBIT(qtd.altnext) == 0) {
-            break;
-        }
         if (NLPTR_TBIT(qtd.next) != 0) {
             break;
         }
diff --git a/hw/usb/hcd-musb.c b/hw/usb/hcd-musb.c
index 4896d38..f65fa3c 100644
--- a/hw/usb/hcd-musb.c
+++ b/hw/usb/hcd-musb.c
@@ -627,7 +627,7 @@ static void musb_packet(MUSBState *s, MUSBEndPoint *ep,
     dev = usb_find_device(&s->port, ep->faddr[idx]);
     uep = usb_ep_get(dev, pid, ep->type[idx] & 0xf);
     usb_packet_setup(&ep->packey[dir].p, pid, uep,
-                     (dev->addr << 16) | (uep->nr << 8) | pid);
+                     (dev->addr << 16) | (uep->nr << 8) | pid, false);
     usb_packet_addbuf(&ep->packey[dir].p, ep->buf[idx], len);
     ep->packey[dir].ep = ep;
     ep->packey[dir].dir = dir;
diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index a3207f0..8741d0f 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -812,7 +812,7 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed,
     } else {
         dev = ohci_find_device(ohci, OHCI_BM(ed->flags, ED_FA));
         ep = usb_ep_get(dev, pid, OHCI_BM(ed->flags, ED_EN));
-        usb_packet_setup(&ohci->usb_packet, pid, ep, addr);
+        usb_packet_setup(&ohci->usb_packet, pid, ep, addr, false);
         usb_packet_addbuf(&ohci->usb_packet, ohci->usb_buf, len);
         ret = usb_handle_packet(dev, &ohci->usb_packet);
         if (ret == USB_RET_ASYNC) {
@@ -1012,7 +1012,7 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed)
         }
         dev = ohci_find_device(ohci, OHCI_BM(ed->flags, ED_FA));
         ep = usb_ep_get(dev, pid, OHCI_BM(ed->flags, ED_EN));
-        usb_packet_setup(&ohci->usb_packet, pid, ep, addr);
+        usb_packet_setup(&ohci->usb_packet, pid, ep, addr, !flag_r);
         usb_packet_addbuf(&ohci->usb_packet, ohci->usb_buf, pktlen);
         ret = usb_handle_packet(dev, &ohci->usb_packet);
 #ifdef DEBUG_PACKET
diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index 00dc9d5..953897b 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -808,6 +808,7 @@ static int uhci_handle_td(UHCIState *s, uint32_t addr, UHCI_TD *td,
     UHCIAsync *async;
     int len = 0, max_len;
     uint8_t pid;
+    bool spd;
     USBDevice *dev;
     USBEndpoint *ep;
 
@@ -852,13 +853,14 @@ static int uhci_handle_td(UHCIState *s, uint32_t addr, UHCI_TD *td,
 
     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);
     if (ep_ret) {
         *ep_ret = ep;
     }
-    usb_packet_setup(&async->packet, pid, ep, addr);
+    usb_packet_setup(&async->packet, pid, ep, addr, spd);
     qemu_sglist_add(&async->sgl, td->buffer, max_len);
     usb_packet_map(&async->packet, &async->sgl);
 
@@ -985,8 +987,7 @@ static void uhci_fill_queue(UHCIState *s, UHCI_TD *td, struct USBEndpoint *ep)
     UHCI_TD ptd;
     int ret;
 
-    ptd.ctrl = td->ctrl;
-    while (is_valid(plink) && !(ptd.ctrl & TD_CTRL_SPD)) {
+    while (is_valid(plink)) {
         pci_dma_read(&s->dev, plink & ~0xf, &ptd, sizeof(ptd));
         le32_to_cpus(&ptd.link);
         le32_to_cpus(&ptd.ctrl);
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 5648cc7..f7b37cd 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -1455,7 +1455,7 @@ static int xhci_setup_packet(XHCITransfer *xfer)
         ep = usb_ep_get(dev, dir, xfer->epid >> 1);
     }
 
-    usb_packet_setup(&xfer->packet, dir, ep, xfer->trbs[0].addr);
+    usb_packet_setup(&xfer->packet, dir, ep, xfer->trbs[0].addr, false);
     xhci_xfer_map(xfer);
     DPRINTF("xhci: setup packet pid 0x%x addr %d ep %d\n",
             xfer->packet.pid, dev->addr, ep->nr);
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);
                 }
 
diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index 022ba42..84b9705 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -1293,6 +1293,16 @@ static void usbredir_interface_info(void *priv,
     }
 }
 
+static void usbredir_set_pipeline(USBRedirDevice *dev, struct USBEndpoint *uep)
+{
+    if (uep->type != USB_ENDPOINT_XFER_BULK) {
+        return;
+    }
+    if (uep->pid == USB_TOKEN_OUT) {
+        uep->pipeline = true;
+    }
+}
+
 static void usbredir_ep_info(void *priv,
     struct usb_redir_ep_info_header *ep_info)
 {
@@ -1334,9 +1344,7 @@ static void usbredir_ep_info(void *priv,
             dev->endpoint[i].max_packet_size =
                 usb_ep->max_packet_size = ep_info->max_packet_size[i];
         }
-        if (ep_info->type[i] == usb_redir_type_bulk) {
-            usb_ep->pipeline = true;
-        }
+        usbredir_set_pipeline(dev, usb_ep);
     }
 }
 
@@ -1597,9 +1605,7 @@ static int usbredir_post_load(void *priv, int version_id)
         usb_ep->type = dev->endpoint[i].type;
         usb_ep->ifnum = dev->endpoint[i].interface;
         usb_ep->max_packet_size = dev->endpoint[i].max_packet_size;
-        if (dev->endpoint[i].type == usb_redir_type_bulk) {
-            usb_ep->pipeline = true;
-        }
+        usbredir_set_pipeline(dev, usb_ep);
     }
     return 0;
 }
-- 
1.7.12.1

[Qemu-devel] [PATCH 13/22] usb: Add an int_req flag to USBPacket

[Hans de Goede]

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb.h          |  3 ++-
 hw/usb/core.c     |  3 ++-
 hw/usb/hcd-ehci.c |  6 ++++--
 hw/usb/hcd-musb.c |  2 +-
 hw/usb/hcd-ohci.c |  7 +++++--
 hw/usb/hcd-uhci.c |  3 ++-
 hw/usb/hcd-xhci.c | 13 +++++++++++--
 7 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/hw/usb.h b/hw/usb.h
index 1fcf79c..3a6cc84 100644
--- a/hw/usb.h
+++ b/hw/usb.h
@@ -352,6 +352,7 @@ struct USBPacket {
     QEMUIOVector iov;
     uint64_t parameter; /* control transfers */
     bool short_not_ok;
+    bool int_req;
     int result; /* transfer length or USB_RET_* status code */
     /* Internal use by the USB layer.  */
     USBPacketState state;
@@ -362,7 +363,7 @@ void usb_packet_init(USBPacket *p);
 void usb_packet_set_state(USBPacket *p, USBPacketState state);
 void usb_packet_check_state(USBPacket *p, USBPacketState expected);
 void usb_packet_setup(USBPacket *p, int pid, USBEndpoint *ep, uint64_t id,
-                      bool short_not_ok);
+                      bool short_not_ok, bool int_req);
 void usb_packet_addbuf(USBPacket *p, void *ptr, size_t len);
 int usb_packet_map(USBPacket *p, QEMUSGList *sgl);
 void usb_packet_unmap(USBPacket *p, QEMUSGList *sgl);
diff --git a/hw/usb/core.c b/hw/usb/core.c
index f4a5ad2..87a513f 100644
--- a/hw/usb/core.c
+++ b/hw/usb/core.c
@@ -533,7 +533,7 @@ void usb_packet_set_state(USBPacket *p, USBPacketState state)
 }
 
 void usb_packet_setup(USBPacket *p, int pid, USBEndpoint *ep, uint64_t id,
-                      bool short_not_ok)
+                      bool short_not_ok, bool int_req)
 {
     assert(!usb_packet_is_inflight(p));
     assert(p->iov.iov != NULL);
@@ -543,6 +543,7 @@ void usb_packet_setup(USBPacket *p, int pid, USBEndpoint *ep, uint64_t id,
     p->result = 0;
     p->parameter = 0;
     p->short_not_ok = short_not_ok;
+    p->int_req = int_req;
     qemu_iovec_reset(&p->iov);
     usb_packet_set_state(p, USB_PACKET_SETUP);
 }
diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 4dfe0f3..18cf097 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -1591,7 +1591,8 @@ static int ehci_execute(EHCIPacket *p, const char *action)
         }
 
         spd = (p->pid == USB_TOKEN_IN && NLPTR_TBIT(p->qtd.altnext) == 0);
-        usb_packet_setup(&p->packet, p->pid, ep, p->qtdaddr, spd);
+        usb_packet_setup(&p->packet, p->pid, ep, p->qtdaddr, spd,
+                         (p->qtd.token & QTD_TOKEN_IOC) != 0);
         usb_packet_map(&p->packet, &p->sgl);
         p->async = EHCI_ASYNC_INITIALIZED;
     }
@@ -1661,7 +1662,8 @@ static int ehci_process_itd(EHCIState *ehci,
             dev = ehci_find_device(ehci, devaddr);
             ep = usb_ep_get(dev, pid, endp);
             if (ep && ep->type == USB_ENDPOINT_XFER_ISOC) {
-                usb_packet_setup(&ehci->ipacket, pid, ep, addr, false);
+                usb_packet_setup(&ehci->ipacket, pid, ep, addr, false,
+                                 (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);
diff --git a/hw/usb/hcd-musb.c b/hw/usb/hcd-musb.c
index f65fa3c..a6105d3 100644
--- a/hw/usb/hcd-musb.c
+++ b/hw/usb/hcd-musb.c
@@ -627,7 +627,7 @@ static void musb_packet(MUSBState *s, MUSBEndPoint *ep,
     dev = usb_find_device(&s->port, ep->faddr[idx]);
     uep = usb_ep_get(dev, pid, ep->type[idx] & 0xf);
     usb_packet_setup(&ep->packey[dir].p, pid, uep,
-                     (dev->addr << 16) | (uep->nr << 8) | pid, false);
+                     (dev->addr << 16) | (uep->nr << 8) | pid, false, true);
     usb_packet_addbuf(&ep->packey[dir].p, ep->buf[idx], len);
     ep->packey[dir].ep = ep;
     ep->packey[dir].dir = dir;
diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index 8741d0f..4c77600 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -810,9 +810,11 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed,
     if (completion) {
         ret = ohci->usb_packet.result;
     } else {
+        bool int_req = relative_frame_number == frame_count &&
+                       OHCI_BM(iso_td.flags, TD_DI) == 0;
         dev = ohci_find_device(ohci, OHCI_BM(ed->flags, ED_FA));
         ep = usb_ep_get(dev, pid, OHCI_BM(ed->flags, ED_EN));
-        usb_packet_setup(&ohci->usb_packet, pid, ep, addr, false);
+        usb_packet_setup(&ohci->usb_packet, pid, ep, addr, false, int_req);
         usb_packet_addbuf(&ohci->usb_packet, ohci->usb_buf, len);
         ret = usb_handle_packet(dev, &ohci->usb_packet);
         if (ret == USB_RET_ASYNC) {
@@ -1012,7 +1014,8 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed)
         }
         dev = ohci_find_device(ohci, OHCI_BM(ed->flags, ED_FA));
         ep = usb_ep_get(dev, pid, OHCI_BM(ed->flags, ED_EN));
-        usb_packet_setup(&ohci->usb_packet, pid, ep, addr, !flag_r);
+        usb_packet_setup(&ohci->usb_packet, pid, ep, addr, !flag_r,
+                         OHCI_BM(td.flags, TD_DI) == 0);
         usb_packet_addbuf(&ohci->usb_packet, ohci->usb_buf, pktlen);
         ret = usb_handle_packet(dev, &ohci->usb_packet);
 #ifdef DEBUG_PACKET
diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index 953897b..4a1ea6b 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -860,7 +860,8 @@ 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, addr, spd,
+                     (td->ctrl & TD_CTRL_IOC) != 0);
     qemu_sglist_add(&async->sgl, td->buffer, max_len);
     usb_packet_map(&async->packet, &async->sgl);
 
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index f7b37cd..53f030b 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -1437,7 +1437,8 @@ static int xhci_setup_packet(XHCITransfer *xfer)
     XHCIPort *port;
     USBDevice *dev;
     USBEndpoint *ep;
-    int dir;
+    bool int_req;
+    int i, dir;
 
     dir = xfer->in_xfer ? USB_TOKEN_IN : USB_TOKEN_OUT;
 
@@ -1455,7 +1456,15 @@ static int xhci_setup_packet(XHCITransfer *xfer)
         ep = usb_ep_get(dev, dir, xfer->epid >> 1);
     }
 
-    usb_packet_setup(&xfer->packet, dir, ep, xfer->trbs[0].addr, false);
+    int_req = false;
+    for (i = 0; i < xfer->trb_count; i++) {
+        if (xfer->trbs[i].control & TRB_TR_IOC) {
+            int_req = true;
+            break;
+        }
+    }
+    usb_packet_setup(&xfer->packet, dir, ep, xfer->trbs[0].addr, false,
+                     int_req);
     xhci_xfer_map(xfer);
     DPRINTF("xhci: setup packet pid 0x%x addr %d ep %d\n",
             xfer->packet.pid, dev->addr, ep->nr);
-- 
1.7.12.1

Re: [Qemu-devel] [PATCH 13/22] usb: Add an int_req flag to USBPacket

[Gerd Hoffmann]

On 10/15/12 12:38, Hans de Goede wrote:
> -    usb_packet_setup(&xfer->packet, dir, ep, xfer->trbs[0].addr, false);
> +    int_req = false;
> +    for (i = 0; i < xfer->trb_count; i++) {
> +        if (xfer->trbs[i].control & TRB_TR_IOC) {
> +            int_req = true;
> +            break;
> +        }
> +    }

Guess we better add a new int_req field to XHCITransfer and fill it when
walking the trb list _anyway_, so we don't have to loop again.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 13/22] usb: Add an int_req flag to USBPacket

[Hans de Goede]

Hi,

On 10/17/2012 01:04 PM, Gerd Hoffmann wrote:
> On 10/15/12 12:38, Hans de Goede wrote:
>> -    usb_packet_setup(&xfer->packet, dir, ep, xfer->trbs[0].addr, false);
>> +    int_req = false;
>> +    for (i = 0; i < xfer->trb_count; i++) {
>> +        if (xfer->trbs[i].control & TRB_TR_IOC) {
>> +            int_req = true;
>> +            break;
>> +        }
>> +    }
>
> Guess we better add a new int_req field to XHCITransfer and fill it when
> walking the trb list _anyway_, so we don't have to loop again.

Ok, I will fix this for the next respin of the patchset.

Regards,

Hans

[Qemu-devel] [PATCH 14/22] usb: Add packet combining functions

[Hans de Goede]

Currently we only do pipelining for output endpoints, since to properly
support short-not-ok semantics we can only have one outstanding input
packet. Since the ehci and uhci controllers have a limited per td packet
size guests will split large input transfers to into multiple packets,
and since we don't pipeline these, this comes with a serious performance
penalty.

This patch adds helper functions to (re-)combine packets which belong to 1
transfer at the guest device-driver level into 1 large transger. This can be
used by (redirection) usb-devices to enable pipelining for input endpoints.

This patch will combine packets together until a transfer terminating packet
is encountered. A terminating packet is a packet which meets one or more 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 usb-redir and ehci code, 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                 |  21 ++++++
 hw/usb/Makefile.objs     |   2 +-
 hw/usb/combined-packet.c | 183 +++++++++++++++++++++++++++++++++++++++++++++++
 hw/usb/core.c            |   1 +
 4 files changed, 206 insertions(+), 1 deletion(-)
 create mode 100644 hw/usb/combined-packet.c

diff --git a/hw/usb.h b/hw/usb.h
index 3a6cc84..3c8ba01 100644
--- a/hw/usb.h
+++ b/hw/usb.h
@@ -160,6 +160,7 @@ typedef struct USBBusOps USBBusOps;
 typedef struct USBPort USBPort;
 typedef struct USBDevice USBDevice;
 typedef struct USBPacket USBPacket;
+typedef struct USBCombinedPacket USBCombinedPacket;
 typedef struct USBEndpoint USBEndpoint;
 
 typedef struct USBDesc USBDesc;
@@ -292,6 +293,14 @@ typedef struct USBDeviceClass {
      */
     int (*handle_data)(USBDevice *dev, USBPacket *p);
 
+    /*
+     * Process / cancel combined packets, called from
+     * usb_ep_combine_input_packets() / usb_combined_packet_cancel().
+     * Only called for devices which call these functions themselves.
+     */
+    int (*handle_combined_data)(USBDevice *dev, USBPacket *p);
+    void (*cancel_combined_packet)(USBDevice *dev, USBPacket *p);
+
     void (*set_interface)(USBDevice *dev, int interface,
                           int alt_old, int alt_new);
 
@@ -356,7 +365,15 @@ struct USBPacket {
     int result; /* transfer length or USB_RET_* status code */
     /* Internal use by the USB layer.  */
     USBPacketState state;
+    USBCombinedPacket *combined;
     QTAILQ_ENTRY(USBPacket) queue;
+    QTAILQ_ENTRY(USBPacket) combined_entry;
+};
+
+struct USBCombinedPacket {
+    USBPacket *first;
+    QTAILQ_HEAD(packets_head, USBPacket) packets;
+    QEMUIOVector iov;
 };
 
 void usb_packet_init(USBPacket *p);
@@ -399,6 +416,10 @@ 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);
 
+void usb_ep_combine_input_packets(USBEndpoint *ep);
+void usb_combined_input_packet_complete(USBDevice *dev, USBPacket *p);
+void usb_combined_packet_cancel(USBDevice *dev, USBPacket *p);
+
 void usb_attach(USBPort *port);
 void usb_detach(USBPort *port);
 void usb_port_reset(USBPort *port);
diff --git a/hw/usb/Makefile.objs b/hw/usb/Makefile.objs
index 4225136..b193321 100644
--- a/hw/usb/Makefile.objs
+++ b/hw/usb/Makefile.objs
@@ -7,7 +7,7 @@ hw-obj-y += libhw.o
 hw-obj-$(CONFIG_SMARTCARD) += dev-smartcard-reader.o
 hw-obj-$(CONFIG_USB_REDIR) += redirect.o
 
-common-obj-y += core.o bus.o desc.o dev-hub.o
+common-obj-y += core.o combined-packet.o bus.o desc.o dev-hub.o
 common-obj-y += host-$(HOST_USB).o dev-bluetooth.o
 common-obj-y += dev-hid.o dev-storage.o dev-wacom.o
 common-obj-y += dev-serial.o dev-network.o dev-audio.o
diff --git a/hw/usb/combined-packet.c b/hw/usb/combined-packet.c
new file mode 100644
index 0000000..b6fcb2c
--- /dev/null
+++ b/hw/usb/combined-packet.c
@@ -0,0 +1,183 @@
+/*
+ * QEMU USB packet combining code (for input pipelining)
+ *
+ * Copyright(c) 2012 Red Hat, Inc.
+ *
+ * Red Hat Authors:
+ * Hans de Goede <hdegoede@redhat.com>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or(at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+#include "qemu-common.h"
+#include "hw/usb.h"
+#include "iov.h"
+#include "trace.h"
+
+static void usb_combined_packet_add(USBCombinedPacket *combined, USBPacket *p)
+{
+    qemu_iovec_concat(&combined->iov, &p->iov, 0, p->iov.size);
+    QTAILQ_INSERT_TAIL(&combined->packets, p, combined_entry);
+    p->combined = combined;
+}
+
+static void usb_combined_packet_remove(USBCombinedPacket *combined,
+                                       USBPacket *p)
+{
+    assert(p->combined == combined);
+    p->combined = NULL;
+    QTAILQ_REMOVE(&combined->packets, p, combined_entry);
+}
+
+/* Also handles completion of non combined packets for pipelined input eps */
+void usb_combined_input_packet_complete(USBDevice *dev, USBPacket *p)
+{
+    USBCombinedPacket *combined = p->combined;
+    USBEndpoint *ep = p->ep;
+    USBPacket *next;
+    enum { completing, complete, leftover };
+    int result, state = completing;
+    bool short_not_ok;
+
+    if (combined == NULL) {
+        usb_packet_complete_one(dev, p);
+        goto leave;
+    }
+
+    assert(combined->first == p && p == QTAILQ_FIRST(&combined->packets));
+
+    result = combined->first->result;
+    short_not_ok = QTAILQ_LAST(&combined->packets, packets_head)->short_not_ok;
+
+    QTAILQ_FOREACH_SAFE(p, &combined->packets, combined_entry, next) {
+        if (state == completing) {
+            /* Distribute data over uncombined packets */
+            if (result >= p->iov.size) {
+                p->result = p->iov.size;
+            } else {
+                /* Send short or error packet to complete the transfer */
+                p->result = result;
+                state = complete;
+            }
+            p->short_not_ok = short_not_ok;
+            usb_combined_packet_remove(combined, p);
+            usb_packet_complete_one(dev, p);
+            result -= p->result;
+        } else {
+            /* Remove any leftover packets from the queue */
+            state = leftover;
+            p->result = USB_RET_REMOVE_FROM_QUEUE;
+            dev->port->ops->complete(dev->port, p);
+        }
+    }
+    /* 
+     * If we had leftover packets the hcd driver will have cancelled them
+     * and usb_combined_packet_cancel has already freed combined!
+     */
+    if (state != leftover) {
+        g_free(combined);
+    }
+leave:
+    /* Check if there are packets in the queue waiting for our completion */
+    usb_ep_combine_input_packets(ep);
+}
+
+/* May only be called for combined packets! */
+void usb_combined_packet_cancel(USBDevice *dev, USBPacket *p)
+{
+    USBCombinedPacket *combined = p->combined;
+    USBDeviceClass *klass = USB_DEVICE_GET_CLASS(dev);
+    assert(combined != NULL);
+    assert(klass->cancel_combined_packet != NULL);
+
+    usb_combined_packet_remove(combined, p);
+    if (p == combined->first) {
+        klass->cancel_combined_packet(dev, p);
+    }
+    if (QTAILQ_EMPTY(&combined->packets)) {
+        g_free(combined);
+    }
+}
+
+/*
+ * 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_combine_input_packets(USBEndpoint *ep)
+{
+    USBPacket *p, *u, *next, *prev = NULL, *first = NULL;
+    USBPort *port = ep->dev->port;
+    USBDeviceClass *klass = USB_DEVICE_GET_CLASS(ep->dev);
+    int ret;
+
+    assert(ep->pipeline);
+    assert(ep->pid == USB_TOKEN_IN);
+    assert(klass->handle_combined_data != NULL);
+
+    QTAILQ_FOREACH_SAFE(p, &ep->queue, queue, next) {
+        /* Empty the queue on a halt */
+        if (ep->halted) {
+            p->result = USB_RET_REMOVE_FROM_QUEUE;
+            port->ops->complete(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 (first->combined == NULL) {
+                USBCombinedPacket *combined = g_new0(USBCombinedPacket, 1);
+
+                combined->first = first;
+                QTAILQ_INIT(&combined->packets);
+                qemu_iovec_init(&combined->iov, 2);
+                usb_combined_packet_add(combined, first);
+            }
+            usb_combined_packet_add(first->combined, p);
+        } 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 ||
+                next == NULL) {
+            ret = klass->handle_combined_data(ep->dev, first);
+            assert(ret == USB_RET_ASYNC);
+            if (first->combined) {
+                QTAILQ_FOREACH(u, &first->combined->packets, combined_entry) {
+                    usb_packet_set_state(u, USB_PACKET_ASYNC);
+                }
+            } else {
+                usb_packet_set_state(first, USB_PACKET_ASYNC);
+            }
+            first = NULL;
+            prev = p;
+        }
+    }
+}
diff --git a/hw/usb/core.c b/hw/usb/core.c
index 87a513f..99b9fdb 100644
--- a/hw/usb/core.c
+++ b/hw/usb/core.c
@@ -544,6 +544,7 @@ void usb_packet_setup(USBPacket *p, int pid, USBEndpoint *ep, uint64_t id,
     p->parameter = 0;
     p->short_not_ok = short_not_ok;
     p->int_req = int_req;
+    p->combined = NULL;
     qemu_iovec_reset(&p->iov);
     usb_packet_set_state(p, USB_PACKET_SETUP);
 }
-- 
1.7.12.1

Re: [Qemu-devel] [PATCH 14/22] usb: Add packet combining functions

[Gerd Hoffmann]

  Hi,

> +    /*
> +     * Process / cancel combined packets, called from
> +     * usb_ep_combine_input_packets() / usb_combined_packet_cancel().
> +     * Only called for devices which call these functions themselves.
> +     */
> +    int (*handle_combined_data)(USBDevice *dev, USBPacket *p);
> +    void (*cancel_combined_packet)(USBDevice *dev, USBPacket *p);

Do we really need these?  I think it isn't much work for the callers to
do that themself.  Saves them providing a callback.  And makes the code
flow easier to follow by removing a pointless indirection.

For handle_combined_data we probably must make
usb_ep_combine_input_packets return a status code.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 14/22] usb: Add packet combining functions

[Hans de Goede]

Hi,

On 10/17/2012 01:29 PM, Gerd Hoffmann wrote:
>    Hi,
>
>> +    /*
>> +     * Process / cancel combined packets, called from
>> +     * usb_ep_combine_input_packets() / usb_combined_packet_cancel().
>> +     * Only called for devices which call these functions themselves.
>> +     */
>> +    int (*handle_combined_data)(USBDevice *dev, USBPacket *p);
>> +    void (*cancel_combined_packet)(USBDevice *dev, USBPacket *p);
>
> Do we really need these?

For handle_combined_data, yes, as usb_ep_combine_input_packets can
cause multiple packets to get submitted, since if a combined packet
ends with a packet, which does not have its short_not_ok flag set,
another packet can be safely pipelined after it. This is not
useful for usb mass storage, but very usefull for usb serial
port converters.

I actually first did not have cancel_combined_packet :) Instead
I had usb_combined_packet_cancel() return a boolean indicating
if it had handled the cancel, or if the caller (which would be
a device's cancel method) needs to handle the cancel itself.

I personally find the cancel_combined_packet callback somewhat
cleaner, esp. since with the return boolean method,
after calling usb_combined_packet_cancel(p) p->combined will be
NULL, so if the device's cancel method needs access to p->combined
to deal with combined packets, things get more difficult.

But if you prefer getting rid of the callback we can do that.

> I think it isn't much work for the callers to
> do that themself.  Saves them providing a callback.  And makes the code
> flow easier to follow by removing a pointless indirection.
>
> For handle_combined_data we probably must make
> usb_ep_combine_input_packets return a status code.

Regards,

Hans

Re: [Qemu-devel] [PATCH 14/22] usb: Add packet combining functions

[Gerd Hoffmann]

  Hi,

> For handle_combined_data, yes, as usb_ep_combine_input_packets can
> cause multiple packets to get submitted, since if a combined packet
> ends with a packet, which does not have its short_not_ok flag set,
> another packet can be safely pipelined after it. This is not
> useful for usb mass storage, but very usefull for usb serial
> port converters.

Ah, I see.  You can have a queue with -- say -- 16 packets which will
get combined into 4 groups with 4 packets each -> 4 callbacks.

I think handle_combined_data() should get USBCombinedPacket passed
instead of USBPacket.  It is cleaner API-wise.  Likewise for the other
usb_combined_* functions.

Allowing to call usb_ep_combine_input_packets on any endpoint (except
iso which is a special case anyway) would be good too I think, then it
is possible for usb drivers to operate on USBCombinedPackets everywhere.

BTW:  I think USBPacketGroup would be a better name for USBCombinedPacket.

cheers,
  Gerd

[Qemu-devel] [PATCH 15/22] combined-packet: Add a workaround for Linux usbfs + live migration

[Hans de Goede]

Older versions (anything but the latest) of Linux usbfs + libusb(x),
will submit larger (bulk) transfers split into multiple 16k submissions,
which means that rather then all tds getting linked into the queue in
one atomic operarion they get linked in a bunch at a time, which could
cause problems if:
1) We scan the queue while libusb is in the middle of submitting a split
   bulk transfer
2) While this bulk transfer is pending we migrate to another host.

The problem is that after 2, the new host will rescan the queue and
combine the packets in one large transfer, where as 1) has caused the
original host to see them as 2 transfers. This patch fixes this by stopping
combinging if we detect a 16k transfer with its int_req flag set.

This should not adversely effect performance for other cases as:
1) Linux never sets the interrupt flag on packets other then the last
2) Windows does set the in_req flag on each td, but will submit large
transfers in 20k tds thus never triggering the check

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

diff --git a/hw/usb/combined-packet.c b/hw/usb/combined-packet.c
index b6fcb2c..d347324 100644
--- a/hw/usb/combined-packet.c
+++ b/hw/usb/combined-packet.c
@@ -120,7 +120,7 @@ void usb_ep_combine_input_packets(USBEndpoint *ep)
     USBPacket *p, *u, *next, *prev = NULL, *first = NULL;
     USBPort *port = ep->dev->port;
     USBDeviceClass *klass = USB_DEVICE_GET_CLASS(ep->dev);
-    int ret;
+    int ret, totalsize;
 
     assert(ep->pipeline);
     assert(ep->pid == USB_TOKEN_IN);
@@ -165,8 +165,11 @@ void usb_ep_combine_input_packets(USBEndpoint *ep)
         }
 
         /* Is this packet the last one of a (combined) transfer? */
+        totalsize = (p->combined) ? p->combined->iov.size : p->iov.size;
         if ((p->iov.size % ep->max_packet_size) != 0 || !p->short_not_ok ||
-                next == NULL) {
+                next == NULL ||
+                /* Work around for Linux usbfs bulk splitting + migration */
+                (totalsize == 16348 && p->int_req)) {
             ret = klass->handle_combined_data(ep->dev, first);
             assert(ret == USB_RET_ASYNC);
             if (first->combined) {
-- 
1.7.12.1

[Qemu-devel] [PATCH 16/22] 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 84b9705..99e25d4 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 17/22] usb-redir: Add support for 32 bits bulk packet length

[Hans de Goede]

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 configure         | 2 +-
 hw/usb/redirect.c | 7 ++++++-
 2 files changed, 7 insertions(+), 2 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 99e25d4..030522e 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;
@@ -1479,7 +1484,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 18/22] usb-redir: Add support for input pipelining

[Hans de Goede]

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

diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index 030522e..f715281 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -29,6 +29,7 @@
 #include "qemu-timer.h"
 #include "monitor.h"
 #include "sysemu.h"
+#include "iov.h"
 
 #include <dirent.h>
 #include <sys/ioctl.h>
@@ -322,6 +323,11 @@ static void usbredir_cancel_packet(USBDevice *udev, USBPacket *p)
 {
     USBRedirDevice *dev = DO_UPCAST(USBRedirDevice, dev, udev);
 
+    if (p->combined) {
+        usb_combined_packet_cancel(udev, p);
+        return;
+    }
+
     packet_id_queue_add(&dev->cancelled, p->id);
     usbredirparser_send_cancel_data_packet(dev->parser, p->id);
     usbredirparser_do_write(dev->parser);
@@ -575,17 +581,18 @@ static int usbredir_handle_bulk_data(USBRedirDevice *dev, USBPacket *p,
                                       uint8_t ep)
 {
     struct usb_redir_bulk_packet_header bulk_packet;
+    size_t size = (p->combined) ? p->combined->iov.size : p->iov.size;
 
-    DPRINTF("bulk-out ep %02X len %zd id %"PRIu64"\n", ep, p->iov.size, p->id);
+    DPRINTF("bulk-out ep %02X len %zd id %"PRIu64"\n", ep, size, p->id);
 
     if (usbredir_already_in_flight(dev, p->id)) {
         return USB_RET_ASYNC;
     }
 
     bulk_packet.endpoint  = ep;
-    bulk_packet.length    = p->iov.size;
+    bulk_packet.length    = size;
     bulk_packet.stream_id = 0;
-    bulk_packet.length_high = p->iov.size >> 16;
+    bulk_packet.length_high = size >> 16;
     assert(bulk_packet.length_high == 0 ||
            usbredirparser_peer_has_cap(dev->parser,
                                        usb_redir_cap_32bits_bulk_length));
@@ -594,11 +601,16 @@ static int usbredir_handle_bulk_data(USBRedirDevice *dev, USBPacket *p,
         usbredirparser_send_bulk_packet(dev->parser, p->id,
                                         &bulk_packet, NULL, 0);
     } else {
-        uint8_t buf[p->iov.size];
-        usb_packet_copy(p, buf, p->iov.size);
-        usbredir_log_data(dev, "bulk data out:", buf, p->iov.size);
+        uint8_t buf[size];
+        if (p->combined) {
+            iov_to_buf(p->combined->iov.iov, p->combined->iov.niov,
+                       0, buf, size);
+        } else {
+            usb_packet_copy(p, buf, size);
+        }
+        usbredir_log_data(dev, "bulk data out:", buf, size);
         usbredirparser_send_bulk_packet(dev->parser, p->id,
-                                        &bulk_packet, buf, p->iov.size);
+                                        &bulk_packet, buf, size);
     }
     usbredirparser_do_write(dev->parser);
     return USB_RET_ASYNC;
@@ -715,6 +727,9 @@ static int usbredir_handle_data(USBDevice *udev, USBPacket *p)
     case USB_ENDPOINT_XFER_ISOC:
         return usbredir_handle_iso_data(dev, p, ep);
     case USB_ENDPOINT_XFER_BULK:
+        if (p->pid == USB_TOKEN_IN && p->ep->pipeline) {
+            return USB_RET_ADD_TO_QUEUE;
+        }
         return usbredir_handle_bulk_data(dev, p, ep);
     case USB_ENDPOINT_XFER_INT:
         return usbredir_handle_interrupt_data(dev, p, ep);
@@ -725,6 +740,20 @@ static int usbredir_handle_data(USBDevice *udev, USBPacket *p)
     }
 }
 
+static void usbredir_flush_ep_queue(USBDevice *dev, USBEndpoint *ep)
+{
+    if (ep->pid == USB_TOKEN_IN && ep->pipeline) {
+        usb_ep_combine_input_packets(ep);
+    }
+}
+
+static int usbredir_handle_combined_data(USBDevice *udev, USBPacket *p)
+{
+    USBRedirDevice *dev = DO_UPCAST(USBRedirDevice, dev, udev);
+
+    return usbredir_handle_bulk_data(dev, p, p->ep->nr | USB_DIR_IN);
+}
+
 static int usbredir_set_config(USBRedirDevice *dev, USBPacket *p,
                                 int config)
 {
@@ -1310,6 +1339,11 @@ static void usbredir_set_pipeline(USBRedirDevice *dev, struct USBEndpoint *uep)
     if (uep->pid == USB_TOKEN_OUT) {
         uep->pipeline = true;
     }
+    if (uep->pid == USB_TOKEN_IN && uep->max_packet_size != 0 &&
+        usbredirparser_peer_has_cap(dev->parser,
+                                    usb_redir_cap_32bits_bulk_length)) {
+        uep->pipeline = true;
+    }
 }
 
 static void usbredir_ep_info(void *priv,
@@ -1492,11 +1526,17 @@ static void usbredir_bulk_packet(void *priv, uint64_t id,
 
     p = usbredir_find_packet_by_id(dev, ep, id);
     if (p) {
+        size_t size = (p->combined) ? p->combined->iov.size : p->iov.size;
         len = usbredir_handle_status(dev, bulk_packet->status, len);
         if (len > 0) {
             usbredir_log_data(dev, "bulk data in:", data, data_len);
-            if (data_len <= p->iov.size) {
-                usb_packet_copy(p, data, data_len);
+            if (data_len <= size) {
+                if (p->combined) {
+                    iov_from_buf(p->combined->iov.iov, p->combined->iov.niov,
+                                 0, data, data_len);
+                } else {
+                    usb_packet_copy(p, data, data_len);
+                }
             } else {
                 ERROR("bulk got more data then requested (%d > %zd)\n",
                       data_len, p->iov.size);
@@ -1504,7 +1544,11 @@ static void usbredir_bulk_packet(void *priv, uint64_t id,
             }
         }
         p->result = len;
-        usb_packet_complete(&dev->dev, p);
+        if (p->pid == USB_TOKEN_IN && p->ep->pipeline) {
+            usb_combined_input_packet_complete(&dev->dev, p);
+        } else {
+            usb_packet_complete(&dev->dev, p);
+        }
     }
     free(data);
 }
@@ -1911,6 +1955,9 @@ static void usbredir_class_initfn(ObjectClass *klass, void *data)
     uc->handle_reset   = usbredir_handle_reset;
     uc->handle_data    = usbredir_handle_data;
     uc->handle_control = usbredir_handle_control;
+    uc->flush_ep_queue = usbredir_flush_ep_queue;
+    uc->handle_combined_data = usbredir_handle_combined_data;
+    uc->cancel_combined_packet = usbredir_cancel_packet;
     dc->vmsd           = &usbredir_vmstate;
     dc->props          = usbredir_properties;
 }
-- 
1.7.12.1

[Qemu-devel] [PATCH 19/22] usb-redir: Add an usbredir_setup_usb_eps() helper function

[Hans de Goede]

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

diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index f715281..9e88779 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -1346,17 +1346,35 @@ static void usbredir_set_pipeline(USBRedirDevice *dev, struct USBEndpoint *uep)
     }
 }
 
+static void usbredir_setup_usb_eps(USBRedirDevice *dev)
+{
+    struct USBEndpoint *usb_ep;
+    int i, pid;
+
+    for (i = 0; i < MAX_ENDPOINTS; i++) {
+        pid = (i & 0x10) ? USB_TOKEN_IN : USB_TOKEN_OUT;
+        usb_ep = usb_ep_get(&dev->dev, pid, i & 0x0f);
+        usb_ep->type = dev->endpoint[i].type;
+        usb_ep->ifnum = dev->endpoint[i].interface;
+        usb_ep->max_packet_size = dev->endpoint[i].max_packet_size;
+        usbredir_set_pipeline(dev, usb_ep);
+    }
+}
+
 static void usbredir_ep_info(void *priv,
     struct usb_redir_ep_info_header *ep_info)
 {
     USBRedirDevice *dev = priv;
-    struct USBEndpoint *usb_ep;
     int i;
 
     for (i = 0; i < MAX_ENDPOINTS; i++) {
         dev->endpoint[i].type = ep_info->type[i];
         dev->endpoint[i].interval = ep_info->interval[i];
         dev->endpoint[i].interface = ep_info->interface[i];
+        if (usbredirparser_peer_has_cap(dev->parser,
+                                     usb_redir_cap_ep_info_max_packet_size)) {
+            dev->endpoint[i].max_packet_size = ep_info->max_packet_size[i];
+        }
         switch (dev->endpoint[i].type) {
         case usb_redir_type_invalid:
             break;
@@ -1377,18 +1395,8 @@ static void usbredir_ep_info(void *priv,
             usbredir_device_disconnect(dev);
             return;
         }
-        usb_ep = usb_ep_get(&dev->dev,
-                            (i & 0x10) ? USB_TOKEN_IN : USB_TOKEN_OUT,
-                            i & 0x0f);
-        usb_ep->type = dev->endpoint[i].type;
-        usb_ep->ifnum = dev->endpoint[i].interface;
-        if (usbredirparser_peer_has_cap(dev->parser,
-                                     usb_redir_cap_ep_info_max_packet_size)) {
-            dev->endpoint[i].max_packet_size =
-                usb_ep->max_packet_size = ep_info->max_packet_size[i];
-        }
-        usbredir_set_pipeline(dev, usb_ep);
     }
+    usbredir_setup_usb_eps(dev);
 }
 
 static void usbredir_configuration_status(void *priv, uint64_t id,
@@ -1630,8 +1638,6 @@ static void usbredir_pre_save(void *priv)
 static int usbredir_post_load(void *priv, int version_id)
 {
     USBRedirDevice *dev = priv;
-    struct USBEndpoint *usb_ep;
-    int i;
 
     switch (dev->device_info.speed) {
     case usb_redir_speed_low:
@@ -1651,15 +1657,8 @@ static int usbredir_post_load(void *priv, int version_id)
     }
     dev->dev.speedmask = (1 << dev->dev.speed);
 
-    for (i = 0; i < MAX_ENDPOINTS; i++) {
-        usb_ep = usb_ep_get(&dev->dev,
-                            (i & 0x10) ? USB_TOKEN_IN : USB_TOKEN_OUT,
-                            i & 0x0f);
-        usb_ep->type = dev->endpoint[i].type;
-        usb_ep->ifnum = dev->endpoint[i].interface;
-        usb_ep->max_packet_size = dev->endpoint[i].max_packet_size;
-        usbredir_set_pipeline(dev, usb_ep);
-    }
+    usbredir_setup_usb_eps(dev);
+
     return 0;
 }
 
-- 
1.7.12.1

[Qemu-devel] [PATCH 20/22] usb-redir: Use reject rather the disconnect on bad ep info

[Hans de Goede]

So that the client gets a notification about us disconnecting the device.

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

diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index 9e88779..26f501a 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -1382,7 +1382,8 @@ static void usbredir_ep_info(void *priv,
         case usb_redir_type_interrupt:
             if (dev->endpoint[i].interval == 0) {
                 ERROR("Received 0 interval for isoc or irq endpoint\n");
-                usbredir_device_disconnect(dev);
+                usbredir_reject_device(dev);
+                return;
             }
             /* Fall through */
         case usb_redir_type_control:
@@ -1392,7 +1393,7 @@ static void usbredir_ep_info(void *priv,
             break;
         default:
             ERROR("Received invalid endpoint type\n");
-            usbredir_device_disconnect(dev);
+            usbredir_reject_device(dev);
             return;
         }
     }
-- 
1.7.12.1

[Qemu-devel] [PATCH 21/22] usb-redir: Allow to attach USB 2.0 devices to 1.1 host controller

[Hans de Goede]

From: Jan Kiszka <jan.kiszka@siemens.com>

This follows the logic of host-linux: If a 2.0 device has no ISO
endpoint and no interrupt endpoint with a packet size > 64, we can
attach it also to an 1.1 host controller. In case the redir server does
not report endpoint sizes, play safe and remove the 1.1 compatibility as
well. Moreover, if we detect a conflicting change in the configuration
after the device was already attached, it will be disconnected
immediately.

HdG: Several small cleanups and fixes

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/redirect.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index 26f501a..63b7010 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -106,6 +106,7 @@ struct USBRedirDevice {
     struct usb_redir_interface_info_header interface_info;
     struct usbredirfilter_rule *filter_rules;
     int filter_rules_count;
+    int compatible_speedmask;
 };
 
 static void usbredir_hello(void *priv, struct usb_redir_hello_header *h);
@@ -974,6 +975,7 @@ static void usbredir_do_attach(void *opaque)
     }
 
     if (usb_device_attach(&dev->dev) != 0) {
+        WARNING("rejecting device due to speed mismatch\n");
         usbredir_reject_device(dev);
     }
 }
@@ -1094,6 +1096,9 @@ static int usbredir_initfn(USBDevice *udev)
     /* We'll do the attach once we receive the speed from the usb-host */
     udev->auto_attach = 0;
 
+    /* Will be cleared during setup when we find conflicts */
+    dev->compatible_speedmask = USB_SPEED_MASK_FULL;
+
     /* Let the backend know we are ready */
     qemu_chr_fe_open(dev->cs);
     qemu_chr_add_handlers(dev->cs, &usbredir_chr_handlers, dev);
@@ -1237,6 +1242,7 @@ static void usbredir_device_connect(void *priv,
     case usb_redir_speed_low:
         speed = "low speed";
         dev->dev.speed = USB_SPEED_LOW;
+        dev->compatible_speedmask &= ~USB_SPEED_MASK_FULL;
         break;
     case usb_redir_speed_full:
         speed = "full speed";
@@ -1270,7 +1276,7 @@ static void usbredir_device_connect(void *priv,
              device_connect->device_class);
     }
 
-    dev->dev.speedmask = (1 << dev->dev.speed);
+    dev->dev.speedmask = (1 << dev->dev.speed) | dev->compatible_speedmask;
     dev->device_info = *device_connect;
 
     if (usbredir_check_filter(dev)) {
@@ -1310,6 +1316,7 @@ static void usbredir_device_disconnect(void *priv)
     dev->interface_info.interface_count = NO_INTERFACE_INFO;
     dev->dev.addr = 0;
     dev->dev.speed = 0;
+    dev->compatible_speedmask = USB_SPEED_MASK_FULL;
 }
 
 static void usbredir_interface_info(void *priv,
@@ -1331,6 +1338,12 @@ static void usbredir_interface_info(void *priv,
     }
 }
 
+static void usbredir_mark_speed_incompatible(USBRedirDevice *dev, int speed)
+{
+    dev->compatible_speedmask &= ~(1 << speed);
+    dev->dev.speedmask = (1 << dev->dev.speed) | dev->compatible_speedmask;
+}
+
 static void usbredir_set_pipeline(USBRedirDevice *dev, struct USBEndpoint *uep)
 {
     if (uep->type != USB_ENDPOINT_XFER_BULK) {
@@ -1379,7 +1392,14 @@ static void usbredir_ep_info(void *priv,
         case usb_redir_type_invalid:
             break;
         case usb_redir_type_iso:
+            usbredir_mark_speed_incompatible(dev, USB_SPEED_FULL);
+            /* Fall through */
         case usb_redir_type_interrupt:
+            if (!usbredirparser_peer_has_cap(dev->parser,
+                                     usb_redir_cap_ep_info_max_packet_size) ||
+                    ep_info->max_packet_size[i] > 64) {
+                usbredir_mark_speed_incompatible(dev, USB_SPEED_FULL);
+            }
             if (dev->endpoint[i].interval == 0) {
                 ERROR("Received 0 interval for isoc or irq endpoint\n");
                 usbredir_reject_device(dev);
@@ -1397,6 +1417,14 @@ static void usbredir_ep_info(void *priv,
             return;
         }
     }
+    /* The new ep info may have caused a speed incompatibility, recheck */
+    if (dev->dev.attached &&
+            !(dev->dev.port->speedmask & dev->dev.speedmask)) {
+        ERROR("Device no longer matches speed after endpoint info change, "
+              "disconnecting!\n");
+        usbredir_reject_device(dev);
+        return;
+    }
     usbredir_setup_usb_eps(dev);
 }
 
-- 
1.7.12.1

[Qemu-devel] [PATCH 22/22] usb-redir: Allow redirecting super speed devices to high speed controllers

[Hans de Goede]

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

diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index 63b7010..ae815b9 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -1097,7 +1097,7 @@ static int usbredir_initfn(USBDevice *udev)
     udev->auto_attach = 0;
 
     /* Will be cleared during setup when we find conflicts */
-    dev->compatible_speedmask = USB_SPEED_MASK_FULL;
+    dev->compatible_speedmask = USB_SPEED_MASK_FULL | USB_SPEED_MASK_HIGH;
 
     /* Let the backend know we are ready */
     qemu_chr_fe_open(dev->cs);
@@ -1243,10 +1243,12 @@ static void usbredir_device_connect(void *priv,
         speed = "low speed";
         dev->dev.speed = USB_SPEED_LOW;
         dev->compatible_speedmask &= ~USB_SPEED_MASK_FULL;
+        dev->compatible_speedmask &= ~USB_SPEED_MASK_HIGH;
         break;
     case usb_redir_speed_full:
         speed = "full speed";
         dev->dev.speed = USB_SPEED_FULL;
+        dev->compatible_speedmask &= ~USB_SPEED_MASK_HIGH;
         break;
     case usb_redir_speed_high:
         speed = "high speed";
@@ -1316,7 +1318,7 @@ static void usbredir_device_disconnect(void *priv)
     dev->interface_info.interface_count = NO_INTERFACE_INFO;
     dev->dev.addr = 0;
     dev->dev.speed = 0;
-    dev->compatible_speedmask = USB_SPEED_MASK_FULL;
+    dev->compatible_speedmask = USB_SPEED_MASK_FULL | USB_SPEED_MASK_HIGH;
 }
 
 static void usbredir_interface_info(void *priv,
@@ -1393,6 +1395,7 @@ static void usbredir_ep_info(void *priv,
             break;
         case usb_redir_type_iso:
             usbredir_mark_speed_incompatible(dev, USB_SPEED_FULL);
+            usbredir_mark_speed_incompatible(dev, USB_SPEED_HIGH);
             /* Fall through */
         case usb_redir_type_interrupt:
             if (!usbredirparser_peer_has_cap(dev->parser,
@@ -1400,6 +1403,11 @@ static void usbredir_ep_info(void *priv,
                     ep_info->max_packet_size[i] > 64) {
                 usbredir_mark_speed_incompatible(dev, USB_SPEED_FULL);
             }
+            if (!usbredirparser_peer_has_cap(dev->parser,
+                                     usb_redir_cap_ep_info_max_packet_size) ||
+                    ep_info->max_packet_size[i] > 1024) {
+                usbredir_mark_speed_incompatible(dev, USB_SPEED_HIGH);
+            }
             if (dev->endpoint[i].interval == 0) {
                 ERROR("Received 0 interval for isoc or irq endpoint\n");
                 usbredir_reject_device(dev);
@@ -1530,6 +1538,17 @@ static void usbredir_control_packet(void *priv, uint64_t id,
     DPRINTF("ctrl-in status %d len %d id %"PRIu64"\n", control_packet->status,
             len, id);
 
+    /* Fix up USB-3 ep0 maxpacket size to allow superspeed connected devices
+     * to work redirected to a not superspeed capable hcd */
+    if (dev->dev.speed == USB_SPEED_SUPER &&
+            !((dev->dev.port->speedmask & USB_SPEED_MASK_SUPER)) &&
+            control_packet->requesttype == 0x80 &&
+            control_packet->request == 6 &&
+            control_packet->value == 0x100 && control_packet->index == 0 &&
+            data_len >= 18 && data[7] == 9) {
+        data[7] = 64;
+    }
+
     p = usbredir_find_packet_by_id(dev, 0, id);
     if (p) {
         len = usbredir_handle_status(dev, control_packet->status, len);
-- 
1.7.12.1

[Qemu-devel] usb: input-pipelining + speedups v3

[Hans de Goede]

This is the final version of the input pipelining patchset, please
add these patches to your tree for Anthony.

Changes since the v2 rfc:
- Move the settnig of the int_req flag for xhci, as you requested
- Add "ehci: Retry to fill the queue while waiting for td completion" patch
- Lots of tests run with the code without issues :)

Thanks & Regards,

Hans

[Qemu-devel] [PATCH 21/22] usb-redir: Allow to attach USB 2.0 devices to 1.1 host controller

[Hans de Goede]

From: Jan Kiszka <jan.kiszka@siemens.com>

This follows the logic of host-linux: If a 2.0 device has no ISO
endpoint and no interrupt endpoint with a packet size > 64, we can
attach it also to an 1.1 host controller. In case the redir server does
not report endpoint sizes, play safe and remove the 1.1 compatibility as
well. Moreover, if we detect a conflicting change in the configuration
after the device was already attached, it will be disconnected
immediately.

HdG: Several small cleanups and fixes

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/redirect.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index b1fc7ef..d2859b7 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -106,6 +106,7 @@ struct USBRedirDevice {
     struct usb_redir_interface_info_header interface_info;
     struct usbredirfilter_rule *filter_rules;
     int filter_rules_count;
+    int compatible_speedmask;
 };
 
 static void usbredir_hello(void *priv, struct usb_redir_hello_header *h);
@@ -974,6 +975,7 @@ static void usbredir_do_attach(void *opaque)
     }
 
     if (usb_device_attach(&dev->dev) != 0) {
+        WARNING("rejecting device due to speed mismatch\n");
         usbredir_reject_device(dev);
     }
 }
@@ -1094,6 +1096,9 @@ static int usbredir_initfn(USBDevice *udev)
     /* We'll do the attach once we receive the speed from the usb-host */
     udev->auto_attach = 0;
 
+    /* Will be cleared during setup when we find conflicts */
+    dev->compatible_speedmask = USB_SPEED_MASK_FULL;
+
     /* Let the backend know we are ready */
     qemu_chr_fe_open(dev->cs);
     qemu_chr_add_handlers(dev->cs, &usbredir_chr_handlers, dev);
@@ -1233,6 +1238,7 @@ static void usbredir_device_connect(void *priv,
     case usb_redir_speed_low:
         speed = "low speed";
         dev->dev.speed = USB_SPEED_LOW;
+        dev->compatible_speedmask &= ~USB_SPEED_MASK_FULL;
         break;
     case usb_redir_speed_full:
         speed = "full speed";
@@ -1266,7 +1272,7 @@ static void usbredir_device_connect(void *priv,
              device_connect->device_class);
     }
 
-    dev->dev.speedmask = (1 << dev->dev.speed);
+    dev->dev.speedmask = (1 << dev->dev.speed) | dev->compatible_speedmask;
     dev->device_info = *device_connect;
 
     if (usbredir_check_filter(dev)) {
@@ -1306,6 +1312,7 @@ static void usbredir_device_disconnect(void *priv)
     dev->interface_info.interface_count = NO_INTERFACE_INFO;
     dev->dev.addr = 0;
     dev->dev.speed = 0;
+    dev->compatible_speedmask = USB_SPEED_MASK_FULL;
 }
 
 static void usbredir_interface_info(void *priv,
@@ -1327,6 +1334,12 @@ static void usbredir_interface_info(void *priv,
     }
 }
 
+static void usbredir_mark_speed_incompatible(USBRedirDevice *dev, int speed)
+{
+    dev->compatible_speedmask &= ~(1 << speed);
+    dev->dev.speedmask = (1 << dev->dev.speed) | dev->compatible_speedmask;
+}
+
 static void usbredir_set_pipeline(USBRedirDevice *dev, struct USBEndpoint *uep)
 {
     if (uep->type != USB_ENDPOINT_XFER_BULK) {
@@ -1375,7 +1388,14 @@ static void usbredir_ep_info(void *priv,
         case usb_redir_type_invalid:
             break;
         case usb_redir_type_iso:
+            usbredir_mark_speed_incompatible(dev, USB_SPEED_FULL);
+            /* Fall through */
         case usb_redir_type_interrupt:
+            if (!usbredirparser_peer_has_cap(dev->parser,
+                                     usb_redir_cap_ep_info_max_packet_size) ||
+                    ep_info->max_packet_size[i] > 64) {
+                usbredir_mark_speed_incompatible(dev, USB_SPEED_FULL);
+            }
             if (dev->endpoint[i].interval == 0) {
                 ERROR("Received 0 interval for isoc or irq endpoint\n");
                 usbredir_reject_device(dev);
@@ -1393,6 +1413,14 @@ static void usbredir_ep_info(void *priv,
             return;
         }
     }
+    /* The new ep info may have caused a speed incompatibility, recheck */
+    if (dev->dev.attached &&
+            !(dev->dev.port->speedmask & dev->dev.speedmask)) {
+        ERROR("Device no longer matches speed after endpoint info change, "
+              "disconnecting!\n");
+        usbredir_reject_device(dev);
+        return;
+    }
     usbredir_setup_usb_eps(dev);
 }
 
-- 
1.7.12.1