From patchwork Tue Mar 3 08:14:29 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Gerd Hoffmann X-Patchwork-Id: 445572 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id D7A87140140 for ; Tue, 3 Mar 2015 19:15:03 +1100 (AEDT) Received: from localhost ([::1]:33761 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YShyn-0005W2-Ms for incoming@patchwork.ozlabs.org; Tue, 03 Mar 2015 03:15:01 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54998) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YShyR-0004yE-AF for qemu-devel@nongnu.org; Tue, 03 Mar 2015 03:14:40 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YShyN-0006Qr-4q for qemu-devel@nongnu.org; Tue, 03 Mar 2015 03:14:39 -0500 Received: from mx1.redhat.com ([209.132.183.28]:54310) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YShyM-0006QJ-SL for qemu-devel@nongnu.org; Tue, 03 Mar 2015 03:14:35 -0500 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t238EYcT005351 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Tue, 3 Mar 2015 03:14:34 -0500 Received: from nilsson.home.kraxel.org (ovpn-116-39.ams2.redhat.com [10.36.116.39]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t238EWZU015543; Tue, 3 Mar 2015 03:14:33 -0500 Received: by nilsson.home.kraxel.org (Postfix, from userid 500) id D057E81A07; Tue, 3 Mar 2015 09:14:31 +0100 (CET) From: Gerd Hoffmann To: qemu-devel@nongnu.org Date: Tue, 3 Mar 2015 09:14:29 +0100 Message-Id: <1425370469-15755-2-git-send-email-kraxel@redhat.com> In-Reply-To: <1425370469-15755-1-git-send-email-kraxel@redhat.com> References: <1425370469-15755-1-git-send-email-kraxel@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: Laszlo Ersek , Gerd Hoffmann Subject: [Qemu-devel] [PULL 1/1] xhci: generate a Transfer Event for each Transfer TRB with the IOC bit set X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org From: Laszlo Ersek At the moment, when the XHCI driver in edk2 (MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf) runs on QEMU, with the options -device nec-usb-xhci -device usb-kbd it crashes with: ASSERT MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c(1759): TrsRing != ((void*) 0) The crash hits in the following edk2 call sequence (all files under MdeModulePkg/Bus/): UsbEnumerateNewDev() [Usb/UsbBusDxe/UsbEnumer.c] UsbBuildDescTable() [Usb/UsbBusDxe/UsbDesc.c] UsbGetDevDesc() [Usb/UsbBusDxe/UsbDesc.c] UsbCtrlGetDesc(USB_REQ_GET_DESCRIPTOR) [Usb/UsbBusDxe/UsbDesc.c] UsbCtrlRequest() [Usb/UsbBusDxe/UsbDesc.c] UsbHcControlTransfer() [Usb/UsbBusDxe/UsbUtility.c] XhcControlTransfer() [Pci/XhciDxe/Xhci.c] XhcCreateUrb() [Pci/XhciDxe/XhciSched.c] XhcCreateTransferTrb() [Pci/XhciDxe/XhciSched.c] XhcExecTransfer() [Pci/XhciDxe/XhciSched.c] XhcCheckUrbResult() [Pci/XhciDxe/XhciSched.c] // // look for TRB_TYPE_DATA_STAGE event [1] // // // Store a copy of the device descriptor, as the hub device // needs this info to configure endpoint. [2] // UsbSetConfig() [Usb/UsbBusDxe/UsbDesc.c] UsbCtrlRequest(USB_REQ_SET_CONFIG) [Usb/UsbBusDxe/UsbDesc.c] UsbHcControlTransfer() [Usb/UsbBusDxe/UsbUtility.c] XhcControlTransfer() [Pci/XhciDxe/Xhci.c] XhcSetConfigCmd() [Pci/XhciDxe/XhciSched.c] XhcInitializeEndpointContext() [Pci/XhciDxe/XhciSched.c] // // allocate transfer ring for the endpoint [3] // USBKeyboardDriverBindingStart() [Usb/UsbKbDxe/EfiKey.c] UsbIoAsyncInterruptTransfer() [Usb/UsbBusDxe/UsbBus.c] UsbHcAsyncInterruptTransfer() [Usb/UsbBusDxe/UsbUtility.c] XhcAsyncInterruptTransfer() [Pci/XhciDxe/Xhci.c] XhcCreateUrb() [Pci/XhciDxe/Xhci.c] XhcCreateTransferTrb() [Pci/XhciDxe/XhciSched.c] XhcSyncTrsRing() [Pci/XhciDxe/XhciSched.c] ASSERT (TrsRing != NULL) [4] UsbEnumerateNewDev() in the USB bus driver issues a GET_DESCRIPTOR request, in order to determine the number of configurations that the endpoint supports. The requests consists of three stages (three TRBs), setup, data, and status. The length of the response is determined in [1], namely from the transfer event that the host controller generates in response to the request's middle stage (ie. the data stage). If the length of the answer is correct (a full GET_DESCRIPTOR request takes 18 bytes), then the XHCI driver that underlies the USB bus driver "snoops" (caches) the descriptor data for later [2]. Later, the USB bus driver sends a SET_CONFIG request. The underlying XHCI driver allocates a transfer ring for the endpoint, relying on the data snooped and cached in step [2]. Finally, the USB keyboard driver submits an asynchronous interrupt transfer to manage the keyboard. As part of this it asserts [4] that the ring has been allocated in step [3]. And this ASSERT() fires. The root cause can be found in the way QEMU handles the initial GET_DESCRIPTOR request. Again, that request consists of three stages (TRBs, Transfer Request Blocks), "setup", "data", and "status". The XhcCreateTransferTrb() function sets the IOC ("Interrupt on Completion") flag in each of these TRBs. According to the XHCI specification, the host controller shall generate a Transfer Event in response to *each* individual TRB of the request that had the IOC flag set. This means that QEMU should queue three events: setup, data, and status, for edk2's XHCI driver. However, QEMU only generates two events: - one for the setup (ie. 1st) stage, - another for the status (ie. 3rd) stage. No event is generated for the middle (ie. data) stage. The loop in QEMU's xhci_xfer_report() function runs three times, but due to the "reported" variable, only the first and the last TRBs elicit events, the middle (data stage) results in no event queued. As a consequence: - When handling the GET_DESCRIPTOR request, XhcCheckUrbResult() in [1] does not update the response length from zero. - XhcControlTransfer() thinks that the response is invalid (it has zero length payload instead of 18 bytes), hence [2] is not reached; the device descriptor is not stashed for later, and the number of possible configurations is left at zero. - When handling the SET_CONFIG request, (NumConfigurations == 0) from above prevents the allocation of the endpoint's transfer ring. - When the keyboard driver tries to use the endpoint, the ASSERT() blows up. The solution is to correct the emulation in QEMU, and to generate a transfer event whenever IOC is set in a TRB. The patch replaces !reported && (IOC || foo) == !reported && IOC || !reported && foo with IOC || (!reported && foo) == IOC || !reported && foo which only changes how reported && IOC is handled. (Namely, it now generates an event.) Tested with edk2 built for "qemu-system-aarch64 -M virt" (ie. "ArmVirtualizationQemu.dsc", aka "AAVMF"), and guest Linux. Signed-off-by: Laszlo Ersek Signed-off-by: Gerd Hoffmann --- hw/usb/hcd-xhci.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c index 776699b..828c2a7 100644 --- a/hw/usb/hcd-xhci.c +++ b/hw/usb/hcd-xhci.c @@ -1767,9 +1767,18 @@ static void xhci_xfer_report(XHCITransfer *xfer) break; } - if (!reported && ((trb->control & TRB_TR_IOC) || - (shortpkt && (trb->control & TRB_TR_ISP)) || - (xfer->status != CC_SUCCESS && left == 0))) { + /* + * XHCI 1.1, 4.11.3.1 Transfer Event TRB -- "each Transfer TRB + * encountered with its IOC flag set to '1' shall generate a Transfer + * Event." + * + * Otherwise, longer transfers can have multiple data TRBs (for scatter + * gather). Short transfers and errors should be reported once per + * transfer only. + */ + if ((trb->control & TRB_TR_IOC) || + (!reported && ((shortpkt && (trb->control & TRB_TR_ISP)) || + (xfer->status != CC_SUCCESS && left == 0)))) { event.slotid = xfer->slotid; event.epid = xfer->epid; event.length = (trb->status & 0x1ffff) - chunk;