| Message ID | 1425312173-10281-1-git-send-email-lersek@redhat.com |
|---|---|
| State | New |
| Headers | show |
Hi, > 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.) Added to usb patch queue. thanks, Gerd
Cc: qemu-stable@nongnu.org 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 <lersek@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; >
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;
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> --- hw/usb/hcd-xhci.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)