From 887938160e5d631c56ee115b1817613a60184138 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Sat, 17 Nov 2012 12:11:49 +0100 Subject: [PATCH 1/7] uhci: Add a completions_only flag for async completions Add a completions_only flag, and set this when running process_frame for async completion handling, this fixes 2 issues in a single patch: 1) It makes sure async completed packets get written to guest mem immediately, even if all the bandwidth for the frame was consumed from the timer run process_frame. This is necessary as delaying their writeback to the next frame can cause the completion to get lost on migration. 2) The calling of process_frame from a bh on async completion causes iso tds to get server more often they should, messing up usb sound class device timing. By only processing completed packets, the iso tds get skipped fixing this. Signed-off-by: Hans de Goede Signed-off-by: Gerd Hoffmann --- hw/usb/hcd-uhci.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c index 2838d2164..ef326330d 100644 --- a/hw/usb/hcd-uhci.c +++ b/hw/usb/hcd-uhci.c @@ -152,6 +152,7 @@ struct UHCIState { QEMUBH *bh; uint32_t frame_bytes; uint32_t frame_bandwidth; + bool completions_only; UHCIPort ports[NB_PORTS]; /* Interrupts that should be raised at the end of the current frame. */ @@ -891,6 +892,10 @@ static int uhci_handle_td(UHCIState *s, UHCIQueue *q, uint32_t qh_addr, goto done; } + if (s->completions_only) { + return TD_RESULT_ASYNC_CONT; + } + /* Allocate new packet */ if (q == NULL) { USBDevice *dev = uhci_find_device(s, (td->token >> 8) & 0x7f); @@ -960,9 +965,9 @@ static void uhci_async_complete(USBPort *port, USBPacket *packet) } async->done = 1; - if (s->frame_bytes < s->frame_bandwidth) { - qemu_bh_schedule(s->bh); - } + /* Force processing of this packet *now*, needed for migration */ + s->completions_only = true; + qemu_bh_schedule(s->bh); } static int is_valid(uint32_t link) @@ -1054,7 +1059,7 @@ static void uhci_process_frame(UHCIState *s) qhdb_reset(&qhdb); for (cnt = FRAME_MAX_LOOPS; is_valid(link) && cnt; cnt--) { - if (s->frame_bytes >= s->frame_bandwidth) { + if (!s->completions_only && s->frame_bytes >= s->frame_bandwidth) { /* We've reached the usb 1.1 bandwidth, which is 1280 bytes/frame, stop processing */ trace_usb_uhci_frame_stop_bandwidth(); @@ -1170,6 +1175,7 @@ static void uhci_frame_timer(void *opaque) /* prepare the timer for the next frame */ s->expire_time += (get_ticks_per_sec() / FRAME_TIMER_FREQ); s->frame_bytes = 0; + s->completions_only = false; qemu_bh_cancel(s->bh); if (!(s->cmd & UHCI_CMD_RS)) { From 1cbdde909f70fd15ff85f068a6318b73865c7fa3 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Sat, 17 Nov 2012 12:11:50 +0100 Subject: [PATCH 2/7] uhci: Don't allow the guest to set port-enabled when there is no dev connected It is possible for device disconnect and the guest trying to reset the port (because of USB xact errors prior to the disconnect getting signaled) to race, when we hit this race, the guest will write the port-control register with its pre-disconnect value + the reset bit set, after which we have a disconnected device with its port-enabled bit set in its port-control register, which is no good :) Signed-off-by: Hans de Goede Signed-off-by: Gerd Hoffmann --- hw/usb/hcd-uhci.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c index ef326330d..078be2ad6 100644 --- a/hw/usb/hcd-uhci.c +++ b/hw/usb/hcd-uhci.c @@ -556,6 +556,10 @@ static void uhci_ioport_writew(void *opaque, uint32_t addr, uint32_t val) } } port->ctrl &= UHCI_PORT_READ_ONLY; + /* enabled may only be set if a device is connected */ + if (!(port->ctrl & UHCI_PORT_CCS)) { + val &= ~UHCI_PORT_EN; + } port->ctrl |= (val & ~UHCI_PORT_READ_ONLY); /* some bits are reset when a '1' is written to them */ port->ctrl &= ~(val & UHCI_PORT_WRITE_CLEAR); From 71d2c9cf656cb8b55a71057c1943ade197c1bb5b Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Sat, 17 Nov 2012 12:11:51 +0100 Subject: [PATCH 3/7] uhci: Fix double unlink uhci_async_cancel() already does a uhci_async_unlink(). Signed-off-by: Hans de Goede Signed-off-by: Gerd Hoffmann --- hw/usb/hcd-uhci.c | 1 - 1 file changed, 1 deletion(-) diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c index 078be2ad6..8e478030a 100644 --- a/hw/usb/hcd-uhci.c +++ b/hw/usb/hcd-uhci.c @@ -963,7 +963,6 @@ static void uhci_async_complete(USBPort *port, USBPacket *packet) UHCIState *s = async->queue->uhci; if (packet->status == USB_RET_REMOVE_FROM_QUEUE) { - uhci_async_unlink(async); uhci_async_cancel(async); return; } From 33c1a6856f06fccd7cbfe53e06f9ebbe95bd565f Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Sat, 17 Nov 2012 12:15:01 +0100 Subject: [PATCH 4/7] usb-bt: Return NAK instead of STALL when interrupt ep has no data I noticed this while making all devices with interrupt endpoints properly do wakeup. While at it also add wakeup support. Note that I've not tested this, but returning STALL for an interrupt ep which has no data is cleary the wrong thing to do. Signed-off-by: Hans de Goede Signed-off-by: Gerd Hoffmann --- hw/usb/dev-bluetooth.c | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/hw/usb/dev-bluetooth.c b/hw/usb/dev-bluetooth.c index bfb96bf9f..39984f53e 100644 --- a/hw/usb/dev-bluetooth.c +++ b/hw/usb/dev-bluetooth.c @@ -27,6 +27,7 @@ struct USBBtState { USBDevice dev; struct HCIInfo *hci; + USBEndpoint *intr; int config; @@ -290,10 +291,7 @@ static inline void usb_bt_fifo_dequeue(struct usb_hci_in_fifo_s *fifo, { int len; - if (likely(!fifo->len)) { - p->status = USB_RET_STALL; - return; - } + assert(fifo->len != 0); len = MIN(p->iov.size, fifo->fifo[fifo->start].len); usb_packet_copy(p, fifo->fifo[fifo->start].data, len); @@ -422,14 +420,26 @@ static void usb_bt_handle_data(USBDevice *dev, USBPacket *p) case USB_TOKEN_IN: switch (p->ep->nr) { case USB_EVT_EP: + if (s->evt.len == 0) { + p->status = USB_RET_NAK; + break; + } usb_bt_fifo_dequeue(&s->evt, p); break; case USB_ACL_EP: + if (s->evt.len == 0) { + p->status = USB_RET_STALL; + break; + } usb_bt_fifo_dequeue(&s->acl, p); break; case USB_SCO_EP: + if (s->evt.len == 0) { + p->status = USB_RET_STALL; + break; + } usb_bt_fifo_dequeue(&s->sco, p); break; @@ -467,6 +477,9 @@ static void usb_bt_out_hci_packet_event(void *opaque, { struct USBBtState *s = (struct USBBtState *) opaque; + if (s->evt.len == 0) { + usb_wakeup(s->intr); + } usb_bt_fifo_enqueue(&s->evt, data, len); } @@ -489,8 +502,12 @@ static void usb_bt_handle_destroy(USBDevice *dev) static int usb_bt_initfn(USBDevice *dev) { + struct USBBtState *s = DO_UPCAST(struct USBBtState, dev, dev); + usb_desc_create_serial(dev); usb_desc_init(dev); + s->intr = usb_ep_get(dev, USB_TOKEN_IN, USB_EVT_EP); + return 0; } From c4020746ff49b2156b4f98672c077d1a3b86fa8b Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Sat, 17 Nov 2012 12:15:02 +0100 Subject: [PATCH 5/7] usb-smartcard-reader: Properly NAK interrupt eps when we've no events When we've no data to return from the interrupt endpoint, return NAK rather then a 0 length packet. CC: Alon Levy Signed-off-by: Hans de Goede Signed-off-by: Gerd Hoffmann --- hw/usb/dev-smartcard-reader.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c index 190fcd62d..de955b709 100644 --- a/hw/usb/dev-smartcard-reader.c +++ b/hw/usb/dev-smartcard-reader.c @@ -1002,6 +1002,8 @@ static void ccid_handle_data(USBDevice *dev, USBPacket *p) "handle_data: int_in: notify_slot_change %X, " "requested len %zd\n", s->bmSlotICCState, p->iov.size); + } else { + p->status = USB_RET_NAK; } break; default: From 234e810cce018daf2030e04e399a17b744fa3e0d Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Sat, 17 Nov 2012 12:26:56 +0100 Subject: [PATCH 6/7] usb-redir: Split usb_handle_interrupt_data into separate in/out functions No functional changes. Signed-off-by: Hans de Goede Signed-off-by: Gerd Hoffmann --- hw/usb/redirect.c | 148 ++++++++++++++++++++++++---------------------- 1 file changed, 77 insertions(+), 71 deletions(-) diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c index 0c95e6b05..66637a85d 100644 --- a/hw/usb/redirect.c +++ b/hw/usb/redirect.c @@ -610,80 +610,82 @@ static void usbredir_handle_bulk_data(USBRedirDevice *dev, USBPacket *p, p->status = USB_RET_ASYNC; } -static void usbredir_handle_interrupt_data(USBRedirDevice *dev, - USBPacket *p, uint8_t ep) +static void usbredir_handle_interrupt_in_data(USBRedirDevice *dev, + USBPacket *p, uint8_t ep) { - if (ep & USB_DIR_IN) { - /* Input interrupt endpoint, buffered packet input */ - struct buf_packet *intp; - int status, len; + /* Input interrupt endpoint, buffered packet input */ + struct buf_packet *intp; + int status, len; - if (!dev->endpoint[EP2I(ep)].interrupt_started && - !dev->endpoint[EP2I(ep)].interrupt_error) { - struct usb_redir_start_interrupt_receiving_header start_int = { - .endpoint = ep, - }; - /* No id, we look at the ep when receiving a status back */ - usbredirparser_send_start_interrupt_receiving(dev->parser, 0, - &start_int); - usbredirparser_do_write(dev->parser); - DPRINTF("interrupt recv started ep %02X\n", ep); - dev->endpoint[EP2I(ep)].interrupt_started = 1; - /* We don't really want to drop interrupt packets ever, but - having some upper limit to how much we buffer is good. */ - dev->endpoint[EP2I(ep)].bufpq_target_size = 1000; - dev->endpoint[EP2I(ep)].bufpq_dropping_packets = 0; - } - - intp = QTAILQ_FIRST(&dev->endpoint[EP2I(ep)].bufpq); - if (intp == NULL) { - DPRINTF2("interrupt-token-in ep %02X, no intp\n", ep); - /* Check interrupt_error for stream errors */ - status = dev->endpoint[EP2I(ep)].interrupt_error; - dev->endpoint[EP2I(ep)].interrupt_error = 0; - if (status) { - usbredir_handle_status(dev, p, status); - } else { - p->status = USB_RET_NAK; - } - return; - } - DPRINTF("interrupt-token-in ep %02X status %d len %d\n", ep, - intp->status, intp->len); - - status = intp->status; - len = intp->len; - if (len > p->iov.size) { - ERROR("received int data is larger then packet ep %02X\n", ep); - len = p->iov.size; - status = usb_redir_babble; - } - usb_packet_copy(p, intp->data, len); - bufp_free(dev, intp, ep); - usbredir_handle_status(dev, p, status); - } else { - /* Output interrupt endpoint, normal async operation */ - struct usb_redir_interrupt_packet_header interrupt_packet; - uint8_t buf[p->iov.size]; - - DPRINTF("interrupt-out ep %02X len %zd id %"PRIu64"\n", ep, - p->iov.size, p->id); - - if (usbredir_already_in_flight(dev, p->id)) { - p->status = USB_RET_ASYNC; - return; - } - - interrupt_packet.endpoint = ep; - interrupt_packet.length = p->iov.size; - - usb_packet_copy(p, buf, p->iov.size); - usbredir_log_data(dev, "interrupt data out:", buf, p->iov.size); - usbredirparser_send_interrupt_packet(dev->parser, p->id, - &interrupt_packet, buf, p->iov.size); + if (!dev->endpoint[EP2I(ep)].interrupt_started && + !dev->endpoint[EP2I(ep)].interrupt_error) { + struct usb_redir_start_interrupt_receiving_header start_int = { + .endpoint = ep, + }; + /* No id, we look at the ep when receiving a status back */ + usbredirparser_send_start_interrupt_receiving(dev->parser, 0, + &start_int); usbredirparser_do_write(dev->parser); - p->status = USB_RET_ASYNC; + DPRINTF("interrupt recv started ep %02X\n", ep); + dev->endpoint[EP2I(ep)].interrupt_started = 1; + /* We don't really want to drop interrupt packets ever, but + having some upper limit to how much we buffer is good. */ + dev->endpoint[EP2I(ep)].bufpq_target_size = 1000; + dev->endpoint[EP2I(ep)].bufpq_dropping_packets = 0; } + + intp = QTAILQ_FIRST(&dev->endpoint[EP2I(ep)].bufpq); + if (intp == NULL) { + DPRINTF2("interrupt-token-in ep %02X, no intp\n", ep); + /* Check interrupt_error for stream errors */ + status = dev->endpoint[EP2I(ep)].interrupt_error; + dev->endpoint[EP2I(ep)].interrupt_error = 0; + if (status) { + usbredir_handle_status(dev, p, status); + } else { + p->status = USB_RET_NAK; + } + return; + } + DPRINTF("interrupt-token-in ep %02X status %d len %d\n", ep, + intp->status, intp->len); + + status = intp->status; + len = intp->len; + if (len > p->iov.size) { + ERROR("received int data is larger then packet ep %02X\n", ep); + len = p->iov.size; + status = usb_redir_babble; + } + usb_packet_copy(p, intp->data, len); + bufp_free(dev, intp, ep); + usbredir_handle_status(dev, p, status); +} + +static void usbredir_handle_interrupt_out_data(USBRedirDevice *dev, + USBPacket *p, uint8_t ep) +{ + /* Output interrupt endpoint, normal async operation */ + struct usb_redir_interrupt_packet_header interrupt_packet; + uint8_t buf[p->iov.size]; + + DPRINTF("interrupt-out ep %02X len %zd id %"PRIu64"\n", ep, + p->iov.size, p->id); + + if (usbredir_already_in_flight(dev, p->id)) { + p->status = USB_RET_ASYNC; + return; + } + + interrupt_packet.endpoint = ep; + interrupt_packet.length = p->iov.size; + + usb_packet_copy(p, buf, p->iov.size); + usbredir_log_data(dev, "interrupt data out:", buf, p->iov.size); + usbredirparser_send_interrupt_packet(dev->parser, p->id, + &interrupt_packet, buf, p->iov.size); + usbredirparser_do_write(dev->parser); + p->status = USB_RET_ASYNC; } static void usbredir_stop_interrupt_receiving(USBRedirDevice *dev, @@ -729,7 +731,11 @@ static void usbredir_handle_data(USBDevice *udev, USBPacket *p) usbredir_handle_bulk_data(dev, p, ep); break; case USB_ENDPOINT_XFER_INT: - usbredir_handle_interrupt_data(dev, p, ep); + if (ep & USB_DIR_IN) { + usbredir_handle_interrupt_in_data(dev, p, ep); + } else { + usbredir_handle_interrupt_out_data(dev, p, ep); + } break; default: ERROR("handle_data ep %02X has unknown type %d\n", ep, From 723aedd53281cfa0997457cb156a59909a75f5a8 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Sat, 17 Nov 2012 12:26:57 +0100 Subject: [PATCH 7/7] usb-redir: Don't handle interrupt output packets async Instead report them as successfully completed directly on submission, this has 2 advantages: 1) This matches the timing of interrupt output packets on real hardware, with the previous async handling, if an ep has an interval of say 500 ms, then there would be 500+ ms between the submission and the guest seeing the completion, as we wont do the write back until the qh gets polled again. And in the mean time the guest may very well have timed out, as the guest can reasonable expect a much quicker completion. 2) This fixes interrupt output packets potentially getting send twice surrounding a migration. As we delay the writeback to guest memory until the qh gets polled again, there is a window between completion and writeback where migration can happen, in this case the destination will not know about the completion, and it will execute the packet *again* But it does also come with a disadvantage: 1) If the actual interrupt out to the real usb device fails, there is no way to report this back to the guest. This patch assumes however that interrupt outs in practice never fail, as they are only used by specialized drivers, which are unlikely to issue illegal requests (unlike general class drivers which often issue requests which some devices don't implement). And that thus the advantages outway the disadvantage. Signed-off-by: Hans de Goede Signed-off-by: Gerd Hoffmann --- hw/usb/redirect.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c index 66637a85d..490c90fae 100644 --- a/hw/usb/redirect.c +++ b/hw/usb/redirect.c @@ -662,21 +662,22 @@ static void usbredir_handle_interrupt_in_data(USBRedirDevice *dev, usbredir_handle_status(dev, p, status); } +/* + * Handle interrupt out data, the usbredir protocol expects us to do this + * async, so that it can report back a completion status. But guests will + * expect immediate completion for an interrupt endpoint, and handling this + * async causes migration issues. So we report success directly, counting + * on the fact that output interrupt packets normally always succeed. + */ static void usbredir_handle_interrupt_out_data(USBRedirDevice *dev, USBPacket *p, uint8_t ep) { - /* Output interrupt endpoint, normal async operation */ struct usb_redir_interrupt_packet_header interrupt_packet; uint8_t buf[p->iov.size]; DPRINTF("interrupt-out ep %02X len %zd id %"PRIu64"\n", ep, p->iov.size, p->id); - if (usbredir_already_in_flight(dev, p->id)) { - p->status = USB_RET_ASYNC; - return; - } - interrupt_packet.endpoint = ep; interrupt_packet.length = p->iov.size; @@ -685,7 +686,6 @@ static void usbredir_handle_interrupt_out_data(USBRedirDevice *dev, usbredirparser_send_interrupt_packet(dev->parser, p->id, &interrupt_packet, buf, p->iov.size); usbredirparser_do_write(dev->parser); - p->status = USB_RET_ASYNC; } static void usbredir_stop_interrupt_receiving(USBRedirDevice *dev, @@ -1647,11 +1647,13 @@ static void usbredir_interrupt_packet(void *priv, uint64_t id, /* bufp_alloc also adds the packet to the ep queue */ bufp_alloc(dev, data, data_len, interrupt_packet->status, ep); } else { - USBPacket *p = usbredir_find_packet_by_id(dev, ep, id); - if (p) { - usbredir_handle_status(dev, p, interrupt_packet->status); - p->actual_length = interrupt_packet->length; - usb_packet_complete(&dev->dev, p); + /* + * We report output interrupt packets as completed directly upon + * submission, so all we can do here if one failed is warn. + */ + if (interrupt_packet->status) { + WARNING("interrupt output failed status %d ep %02X id %"PRIu64"\n", + interrupt_packet->status, ep, id); } } }