Re: [Qemu-devel] [PATCH] xhci: generate a Transfer Event for each Transfer TRB with the IOC bit set

[Paolo Bonzini]

Cc: address@hidden

On 02/03/2015 17:02, Laszlo Ersek wrote:
> 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 <address@hidden>
> ---
>  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;
>