From 0cd089e937f28a1f6f4db711f643df36f8f5dfe3 Mon Sep 17 00:00:00 2001 From: Phil Dennis-Jordan Date: Wed, 25 Jan 2017 18:24:35 +0100 Subject: [PATCH 1/9] hw/usb/dev-hid: Improve guest compatibility of usb-tablet 1. Set bInterfaceProtocol to 0x00 for usb-tablet. This should be non-zero for boot protocol devices only, which the usb-tablet is not. 2. Set the usb-tablet's usage to "mouse" in the report descriptor. The boot protocol of 0x02 specifically confused OS X/macOS' HID driver stack, causing it to generate additional bogus HID events with relative motion in addition to the tablet's absolute coordinate events. Absolute pointing devices with HID Report Descriptor usage of 0x01 (pointing) are treated by the macOS HID driver as analog sticks, and absolute coordinates are not directly translated to absolute mouse cursor positions. Changing it to 0x02 (mouse) fixes the problem, and does not have any adverse effect in other operating systems and windowing systems. (VMWare does the same thing.) Signed-off-by: Phil Dennis-Jordan Message-id: 1485365075-32702-1-git-send-email-phil@philjordan.eu Signed-off-by: Gerd Hoffmann --- hw/usb/dev-hid.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/hw/usb/dev-hid.c b/hw/usb/dev-hid.c index 24d05f76f9..dda0bf0df0 100644 --- a/hw/usb/dev-hid.c +++ b/hw/usb/dev-hid.c @@ -144,7 +144,7 @@ static const USBDescIface desc_iface_tablet = { .bInterfaceNumber = 0, .bNumEndpoints = 1, .bInterfaceClass = USB_CLASS_HID, - .bInterfaceProtocol = 0x02, + .bInterfaceProtocol = 0x00, .ndesc = 1, .descs = (USBDescOther[]) { { @@ -174,7 +174,7 @@ static const USBDescIface desc_iface_tablet2 = { .bInterfaceNumber = 0, .bNumEndpoints = 1, .bInterfaceClass = USB_CLASS_HID, - .bInterfaceProtocol = 0x02, + .bInterfaceProtocol = 0x00, .ndesc = 1, .descs = (USBDescOther[]) { { @@ -487,7 +487,7 @@ static const uint8_t qemu_mouse_hid_report_descriptor[] = { static const uint8_t qemu_tablet_hid_report_descriptor[] = { 0x05, 0x01, /* Usage Page (Generic Desktop) */ - 0x09, 0x01, /* Usage (Pointer) */ + 0x09, 0x02, /* Usage (Mouse) */ 0xa1, 0x01, /* Collection (Application) */ 0x09, 0x01, /* Usage (Pointer) */ 0xa1, 0x00, /* Collection (Physical) */ From e306b2fd3bce369feb05e2510299f81d71bc6092 Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Tue, 31 Jan 2017 14:52:06 +0100 Subject: [PATCH 2/9] usb/uas: more verbose error message Print some more details in case we get a unknown control request, to ease trouble-shooting. Signed-off-by: Gerd Hoffmann Message-id: 1485870727-21956-1-git-send-email-kraxel@redhat.com --- hw/usb/dev-uas.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c index 3a8ff18b1b..da2fb7017e 100644 --- a/hw/usb/dev-uas.c +++ b/hw/usb/dev-uas.c @@ -653,7 +653,8 @@ static void usb_uas_handle_control(USBDevice *dev, USBPacket *p, if (ret >= 0) { return; } - error_report("%s: unhandled control request", __func__); + error_report("%s: unhandled control request (req 0x%x, val 0x%x, idx 0x%x", + __func__, request, value, index); p->status = USB_RET_STALL; } From 811ad5d8f1d3f35240043fe880d34dce6f2097a3 Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Tue, 31 Jan 2017 14:52:07 +0100 Subject: [PATCH 3/9] usb: accept usb3 control requests Windows 10 reportedly sends these, so accept them in case the device in question is a superspeed (usb3) device. Signed-off-by: Gerd Hoffmann Message-id: 1485870727-21956-2-git-send-email-kraxel@redhat.com --- hw/usb/desc.c | 7 +++++++ include/hw/usb.h | 2 ++ 2 files changed, 9 insertions(+) diff --git a/hw/usb/desc.c b/hw/usb/desc.c index 7828e52c6f..c36bf30e4f 100644 --- a/hw/usb/desc.c +++ b/hw/usb/desc.c @@ -774,6 +774,13 @@ int usb_desc_handle_control(USBDevice *dev, USBPacket *p, trace_usb_set_device_feature(dev->addr, value, ret); break; + case DeviceOutRequest | USB_REQ_SET_SEL: + case DeviceOutRequest | USB_REQ_SET_ISOCH_DELAY: + if (dev->speed == USB_SPEED_SUPER) { + ret = 0; + } + break; + case InterfaceRequest | USB_REQ_GET_INTERFACE: if (index < 0 || index >= dev->ninterfaces) { break; diff --git a/include/hw/usb.h b/include/hw/usb.h index 43838c9f5d..c42b29c866 100644 --- a/include/hw/usb.h +++ b/include/hw/usb.h @@ -135,6 +135,8 @@ #define USB_REQ_GET_INTERFACE 0x0A #define USB_REQ_SET_INTERFACE 0x0B #define USB_REQ_SYNCH_FRAME 0x0C +#define USB_REQ_SET_SEL 0x30 +#define USB_REQ_SET_ISOCH_DELAY 0x31 #define USB_DEVICE_SELF_POWERED 0 #define USB_DEVICE_REMOTE_WAKEUP 1 From f94d18d6c6df388fde196d3ab252f57e33843a8b Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Mon, 30 Jan 2017 16:36:44 +0100 Subject: [PATCH 4/9] xhci: only free completed transfers Most callsites check already, one was missed. Cc: 1653384@bugs.launchpad.net Fixes: 94b037f2a451b3dc855f9f2c346e5049a361bd55 Reported-by: Fabian Lesniak Signed-off-by: Gerd Hoffmann Message-id: 1485790607-31399-2-git-send-email-kraxel@redhat.com --- hw/usb/hcd-xhci.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c index f8106789d8..6a1d3dc7d0 100644 --- a/hw/usb/hcd-xhci.c +++ b/hw/usb/hcd-xhci.c @@ -2198,7 +2198,9 @@ static void xhci_kick_epctx(XHCIEPContext *epctx, unsigned int streamid) xhci_complete_packet(xfer); } assert(!xfer->running_retry); - xhci_ep_free_xfer(epctx->retry); + if (xfer->complete) { + xhci_ep_free_xfer(epctx->retry); + } epctx->retry = NULL; } From 13e8ff7abbf1dde46280536ab4fae5012661b8b0 Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Mon, 30 Jan 2017 16:36:45 +0100 Subject: [PATCH 5/9] xhci: rename xhci_complete_packet to xhci_try_complete_packet Make clear that this isn't guaranteed to actually complete the transfer, the usb packet can still be in flight after calling that function. Signed-off-by: Gerd Hoffmann Message-id: 1485790607-31399-3-git-send-email-kraxel@redhat.com --- hw/usb/hcd-xhci.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c index 6a1d3dc7d0..7e863d3f8f 100644 --- a/hw/usb/hcd-xhci.c +++ b/hw/usb/hcd-xhci.c @@ -1897,7 +1897,7 @@ static int xhci_setup_packet(XHCITransfer *xfer) return 0; } -static int xhci_complete_packet(XHCITransfer *xfer) +static int xhci_try_complete_packet(XHCITransfer *xfer) { if (xfer->packet.status == USB_RET_ASYNC) { trace_usb_xhci_xfer_async(xfer); @@ -2002,7 +2002,7 @@ static int xhci_fire_ctl_transfer(XHCIState *xhci, XHCITransfer *xfer) usb_handle_packet(xfer->packet.ep->dev, &xfer->packet); - xhci_complete_packet(xfer); + xhci_try_complete_packet(xfer); if (!xfer->running_async && !xfer->running_retry) { xhci_kick_epctx(xfer->epctx, 0); } @@ -2106,7 +2106,7 @@ static int xhci_submit(XHCIState *xhci, XHCITransfer *xfer, XHCIEPContext *epctx } usb_handle_packet(xfer->packet.ep->dev, &xfer->packet); - xhci_complete_packet(xfer); + xhci_try_complete_packet(xfer); if (!xfer->running_async && !xfer->running_retry) { xhci_kick_epctx(xfer->epctx, xfer->streamid); } @@ -2185,7 +2185,7 @@ static void xhci_kick_epctx(XHCIEPContext *epctx, unsigned int streamid) } usb_handle_packet(xfer->packet.ep->dev, &xfer->packet); assert(xfer->packet.status != USB_RET_NAK); - xhci_complete_packet(xfer); + xhci_try_complete_packet(xfer); } else { /* retry nak'ed transfer */ if (xhci_setup_packet(xfer) < 0) { @@ -2195,7 +2195,7 @@ static void xhci_kick_epctx(XHCIEPContext *epctx, unsigned int streamid) if (xfer->packet.status == USB_RET_NAK) { return; } - xhci_complete_packet(xfer); + xhci_try_complete_packet(xfer); } assert(!xfer->running_retry); if (xfer->complete) { @@ -3492,7 +3492,7 @@ static void xhci_complete(USBPort *port, USBPacket *packet) xhci_ep_nuke_one_xfer(xfer, 0); return; } - xhci_complete_packet(xfer); + xhci_try_complete_packet(xfer); xhci_kick_epctx(xfer->epctx, xfer->streamid); if (xfer->complete) { xhci_ep_free_xfer(xfer); From ddb603ab6c981c1d67cb42266fc700c33e5b2d8f Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Mon, 30 Jan 2017 16:36:46 +0100 Subject: [PATCH 6/9] xhci: don't kick in xhci_submit and xhci_fire_ctl_transfer xhci_submit and xhci_fire_ctl_transfer are is called from xhci_kick_epctx processing loop only, so there is no need to call xhci_kick_epctx make sure processing continues. Also eecursive calls into xhci_kick_epctx can cause trouble. Drop the xhci_kick_epctx calls. Cc: 1653384@bugs.launchpad.net Fixes: 94b037f2a451b3dc855f9f2c346e5049a361bd55 Reported-by: Fabian Lesniak Signed-off-by: Gerd Hoffmann Message-id: 1485790607-31399-4-git-send-email-kraxel@redhat.com --- hw/usb/hcd-xhci.c | 8 -------- 1 file changed, 8 deletions(-) diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c index 7e863d3f8f..f89d8dafba 100644 --- a/hw/usb/hcd-xhci.c +++ b/hw/usb/hcd-xhci.c @@ -2001,11 +2001,7 @@ static int xhci_fire_ctl_transfer(XHCIState *xhci, XHCITransfer *xfer) xfer->packet.parameter = trb_setup->parameter; usb_handle_packet(xfer->packet.ep->dev, &xfer->packet); - xhci_try_complete_packet(xfer); - if (!xfer->running_async && !xfer->running_retry) { - xhci_kick_epctx(xfer->epctx, 0); - } return 0; } @@ -2105,11 +2101,7 @@ static int xhci_submit(XHCIState *xhci, XHCITransfer *xfer, XHCIEPContext *epctx return -1; } usb_handle_packet(xfer->packet.ep->dev, &xfer->packet); - xhci_try_complete_packet(xfer); - if (!xfer->running_async && !xfer->running_retry) { - xhci_kick_epctx(xfer->epctx, xfer->streamid); - } return 0; } From 96d87bdda3919bb16f754b3d3fd1227e1f38f13c Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Thu, 2 Feb 2017 12:36:12 +0100 Subject: [PATCH 7/9] xhci: guard xhci_kick_epctx against recursive calls Track xhci_kick_epctx processing being active in a variable. Check the variable before calling xhci_kick_epctx from xhci_kick_ep. Add an assert to make sure we don't call recursively into xhci_kick_epctx. Cc: 1653384@bugs.launchpad.net Fixes: 94b037f2a451b3dc855f9f2c346e5049a361bd55 Reported-by: Fabian Lesniak Signed-off-by: Gerd Hoffmann Message-id: 1486035372-3621-1-git-send-email-kraxel@redhat.com Message-id: 1485790607-31399-5-git-send-email-kraxel@redhat.com --- hw/usb/hcd-xhci.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c index f89d8dafba..1878dad2ce 100644 --- a/hw/usb/hcd-xhci.c +++ b/hw/usb/hcd-xhci.c @@ -390,6 +390,7 @@ struct XHCIEPContext { dma_addr_t pctx; unsigned int max_psize; uint32_t state; + uint32_t kick_active; /* streams */ unsigned int max_pstreams; @@ -2131,6 +2132,9 @@ static void xhci_kick_ep(XHCIState *xhci, unsigned int slotid, return; } + if (epctx->kick_active) { + return; + } xhci_kick_epctx(epctx, streamid); } @@ -2146,6 +2150,7 @@ static void xhci_kick_epctx(XHCIEPContext *epctx, unsigned int streamid) int i; trace_usb_xhci_ep_kick(epctx->slotid, epctx->epid, streamid); + assert(!epctx->kick_active); /* If the device has been detached, but the guest has not noticed this yet the 2 above checks will succeed, but we must NOT continue */ @@ -2217,6 +2222,7 @@ static void xhci_kick_epctx(XHCIEPContext *epctx, unsigned int streamid) } assert(ring->dequeue != 0); + epctx->kick_active++; while (1) { length = xhci_ring_chain_length(xhci, ring); if (length <= 0) { @@ -2253,6 +2259,7 @@ static void xhci_kick_epctx(XHCIEPContext *epctx, unsigned int streamid) break; } } + epctx->kick_active--; ep = xhci_epid_to_usbep(epctx); if (ep) { From c7dfbf322595ded4e70b626bf83158a9f3807c6a Mon Sep 17 00:00:00 2001 From: Prasad J Pandit Date: Fri, 3 Feb 2017 00:52:28 +0530 Subject: [PATCH 8/9] usb: ccid: check ccid apdu length CCID device emulator uses Application Protocol Data Units(APDU) to exchange command and responses to and from the host. The length in these units couldn't be greater than 65536. Add check to ensure the same. It'd also avoid potential integer overflow in emulated_apdu_from_guest. Reported-by: Li Qiang Signed-off-by: Prasad J Pandit Message-id: 20170202192228.10847-1-ppandit@redhat.com Signed-off-by: Gerd Hoffmann --- hw/usb/dev-smartcard-reader.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c index 89e11b68c4..1325ea1659 100644 --- a/hw/usb/dev-smartcard-reader.c +++ b/hw/usb/dev-smartcard-reader.c @@ -967,7 +967,7 @@ static void ccid_on_apdu_from_guest(USBCCIDState *s, CCID_XferBlock *recv) DPRINTF(s, 1, "%s: seq %d, len %d\n", __func__, recv->hdr.bSeq, len); ccid_add_pending_answer(s, (CCID_Header *)recv); - if (s->card) { + if (s->card && len <= BULK_OUT_DATA_SIZE) { ccid_card_apdu_from_guest(s->card, recv->abData, len); } else { DPRINTF(s, D_WARN, "warning: discarded apdu\n"); From 7da76e12cc5cc902dda4c168d8d608fd4e61cbc5 Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Fri, 3 Feb 2017 07:51:45 +0100 Subject: [PATCH 9/9] xhci: fix event queue IRQ handling The qemu xhci emulation doesn't handle the ERDP_EHB flag correctly. When the host adapter queues a new event the ERDP_EHB flag is set. The flag is cleared (via w1c) by the guest when it updates the ERDP (event ring dequeue pointer) register to notify the host adapter which events it has fetched. An IRQ must be raised in case the ERDP_EHB flag flips from clear to set. If the flag is set already (which implies there are events queued up which are not yet processed by the guest) xhci must *not* raise a IRQ. Qemu got that wrong and raised an IRQ on every event, thereby generating spurious interrupts in case we've queued events faster than the guest processed them. This patch fixes that. With that change in place we also have to check ERDP updates, to see whenever the guest has fetched all queued events. In case there are still pending events set ERDP_EHB and raise an IRQ again, to make sure the events don't linger unseen forever. The linux kernel driver and the microsoft windows driver (shipped with win8+) can deal with the spurious interrupts without problems. The renesas windows driver (v2.1.39) which can be used on older windows versions is quite upset though. It does spurious ERDP updates now and then (not every time, seems we must hit a race window for this to happen), which in turn makes the qemu xhci emulation think the event ring is full. Things go south from here ... tl;dr: This is the "fix xhci on win7" patch. Cc: M.Cerveny@computer.org Cc: 1373228@bugs.launchpad.net Signed-off-by: Gerd Hoffmann Message-id: 1486104705-13761-1-git-send-email-kraxel@redhat.com --- hw/usb/hcd-xhci.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c index 1878dad2ce..54b3901c8c 100644 --- a/hw/usb/hcd-xhci.c +++ b/hw/usb/hcd-xhci.c @@ -789,11 +789,15 @@ static void xhci_msix_update(XHCIState *xhci, int v) static void xhci_intr_raise(XHCIState *xhci, int v) { PCIDevice *pci_dev = PCI_DEVICE(xhci); + bool pending = (xhci->intr[v].erdp_low & ERDP_EHB); xhci->intr[v].erdp_low |= ERDP_EHB; xhci->intr[v].iman |= IMAN_IP; xhci->usbsts |= USBSTS_EINT; + if (pending) { + return; + } if (!(xhci->intr[v].iman & IMAN_IE)) { return; } @@ -3352,6 +3356,15 @@ static void xhci_runtime_write(void *ptr, hwaddr reg, intr->erdp_low &= ~ERDP_EHB; } intr->erdp_low = (val & ~ERDP_EHB) | (intr->erdp_low & ERDP_EHB); + if (val & ERDP_EHB) { + dma_addr_t erdp = xhci_addr64(intr->erdp_low, intr->erdp_high); + unsigned int dp_idx = (erdp - intr->er_start) / TRB_SIZE; + if (erdp >= intr->er_start && + erdp < (intr->er_start + TRB_SIZE * intr->er_size) && + dp_idx != intr->er_ep_idx) { + xhci_intr_raise(xhci, v); + } + } break; case 0x1c: /* ERDP high */ intr->erdp_high = val;