[Qemu-devel] [PULL 0/1] xhci: generate a Transfer Event for each Transfer TRB with the IOC bit set

[Qemu-devel] [PULL 0/1] xhci: generate a Transfer Event for each Transfer TRB with the IOC bit set

[Gerd Hoffmann]

  Hi,

Here comes the usb patch queue with a single bugfix.

pleaae pull,
  Gerd

The following changes since commit 0856579cac2f1dacecd847cfcd89680d26ff78f5:

  Revert "Merge remote-tracking branch 'remotes/ehabkost/tags/x86-pull-request' into staging" (2015-03-03 00:29:17 +0000)

are available in the git repository at:

  git://git.kraxel.org/qemu tags/pull-usb-20150303-1

for you to fetch changes up to aa6857891df614c620e6e9fc4bc4af6e0e49cafd:

  xhci: generate a Transfer Event for each Transfer TRB with the IOC bit set (2015-03-03 08:36:58 +0100)

----------------------------------------------------------------
xhci: generate a Transfer Event for each Transfer TRB with the IOC bit set

----------------------------------------------------------------
Laszlo Ersek (1):
      xhci: generate a Transfer Event for each Transfer TRB with the IOC bit set

 hw/usb/hcd-xhci.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

[Qemu-devel] [PULL 1/1] xhci: generate a Transfer Event for each Transfer TRB with the IOC bit set

[Gerd Hoffmann]

From: Laszlo Ersek <lersek@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 <lersek@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@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;
-- 
1.8.3.1

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

[Laszlo Ersek]

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 <lersek@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 <lersek@redhat.com>
> Signed-off-by: Gerd Hoffmann <kraxel@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;
> 

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

[Gerd Hoffmann]

On Mi, 2015-03-04 at 10:03 +0100, Laszlo Ersek wrote:
> 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.

You can, just fetch the pull tag and grab the commit id from it.
Merging the pull will not change it.  Only in case I have to rebase &
redo the pull the id will change, which is highly unlikely with this
one-patch pull request.  And of course http://git.qemu.org/ links will
only work after the merge.

cheers,
  Gerd

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

[Peter Maydell]

On 4 March 2015 at 18:03, Laszlo Ersek <lersek@redhat.com> wrote:
> 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.

This pull request is only one day old! Even when I'm at work
you shouldn't expect a one-day turnaround on pullreqs. (It
might happen if you get lucky with the timing, but it
probably won't, especially since I generally process pulls
first-in-first-out.) A week is probably a more reasonable
point to prod at.

-- PMM

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

[Laszlo Ersek]

On 03/04/15 15:10, Peter Maydell wrote:
> On 4 March 2015 at 18:03, Laszlo Ersek <lersek@redhat.com> wrote:
>> 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.
> 
> This pull request is only one day old! Even when I'm at work
> you shouldn't expect a one-day turnaround on pullreqs. (It
> might happen if you get lucky with the timing, but it
> probably won't, especially since I generally process pulls
> first-in-first-out.) A week is probably a more reasonable
> point to prod at.

Apologies then. /me hides
Laszlo

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

[Peter Maydell]

On 3 March 2015 at 17:14, Gerd Hoffmann <kraxel@redhat.com> wrote:
>   Hi,
>
> Here comes the usb patch queue with a single bugfix.
>
> pleaae pull,
>   Gerd
>
> The following changes since commit 0856579cac2f1dacecd847cfcd89680d26ff78f5:
>
>   Revert "Merge remote-tracking branch 'remotes/ehabkost/tags/x86-pull-request' into staging" (2015-03-03 00:29:17 +0000)
>
> are available in the git repository at:
>
>   git://git.kraxel.org/qemu tags/pull-usb-20150303-1
>
> for you to fetch changes up to aa6857891df614c620e6e9fc4bc4af6e0e49cafd:
>
>   xhci: generate a Transfer Event for each Transfer TRB with the IOC bit set (2015-03-03 08:36:58 +0100)
>
> ----------------------------------------------------------------
> xhci: generate a Transfer Event for each Transfer TRB with the IOC bit set
>
> ----------------------------------------------------------------
> Laszlo Ersek (1):
>       xhci: generate a Transfer Event for each Transfer TRB with the IOC bit set
>
>  hw/usb/hcd-xhci.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)

Applied, thanks.

-- PMM