Hi Peter,
sorry about poking you -- I'm aware you've been on vacation, but in
<http://lists.nongnu.org/archive/html/qemu-devel/2015-03/msg00174.html>
you said you still inteded to pull stuff.
So, can you please pull this? :) I'd like to enable the XHCI driver in
both OVMF and AAVMF, and I'd like to reference the qemu commit in the
edk2 commit messages.
Thanks!
Laszlo
On 03/03/15 09:14, Gerd Hoffmann wrote:
> From: Laszlo Ersek <ler...@redhat.com>
>
> 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 <ler...@redhat.com>
> Signed-off-by: Gerd Hoffmann <kra...@redhat.com>
> ---
> 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;
>
