| Message ID | 1425370469-15755-2-git-send-email-kraxel@redhat.com |
|---|---|
| State | New |
| Headers | show |
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; >
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
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
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
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;