The dequeue pointer should only be updated in case the transfer
is actually completed. If we update it for inflight transfers
we will not pick them up again after migration, which easily
triggers with HID devices as they typically have a pending
transfer, waiting for user input to happen.
Fixes: 243afe858b
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1451631
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Tested-by: Laurent Vivier <lvivier@redhat.com>
Message-id: 20170608074122.32099-1-kraxel@redhat.com
Moved structs and defines to hcd-xhci.h.
Move nec controller variant to hcd-xhci-nec.c.
No functional changes.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Message-id: 20170517103313.8459-1-kraxel@redhat.com
The strict td link limit added by commit "05f43d4 xhci: limit the
number of link trbs we are willing to process" causes problems with
Windows guests. Let's raise the limit.
This change is analogous to:
commit ab6b1105a2
Author: Gerd Hoffmann <kraxel@redhat.com>
Date: Tue Mar 7 09:40:18 2017 +0100
ohci: relax link check
Signed-off-by: Ladi Prosek <lprosek@redhat.com>
Message-id: 20170512102100.22675-1-lprosek@redhat.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
slotid and epid were deleted from XHCITransfer in commit d6fcb29.
Also deleting one unused forward declaration.
Signed-off-by: Ladi Prosek <lprosek@redhat.com>
Message-id: 20170511125314.24549-2-lprosek@redhat.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
When done processing a endpoint ring we must update the dequeue pointer
in the endpoint context in guest memory. This is needed to make sure
the guest has a correct view of things and also to make live migration
work properly, because xhci post_load restores alot of the state from
xhci data structures in guest memory.
Add xhci_set_ep_state() call to do that.
The recursive calls stopped by commit
ddb603ab6c had the (unintentional) side
effect to hiding this bug. xhci_set_ep_state() was called before
processing, to set the state to running, which updated the dequeue
pointer too.
Reported-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Tested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Message-id: 20170331102521.29253-1-kraxel@redhat.com
Split xhci properties into common and nec specific.
Move the backward compat flags to nec, so the new qemu-xhci
devices doesn't carry on the compatibiity stuff.
Move the msi/msix switches too and just enable msix for qemu-xhci.
Also move the intrs and slots properties. Wasn't a great idea to
make them configurable in the first place, nobody needs this.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Message-id: 1487663432-10410-1-git-send-email-kraxel@redhat.com
Turn existing TYPE_XHCI into an abstract base class.
Create two child classes, TYPE_NEC_XHCI (same name as old xhci
controller) and TYPE_QEMU_XHCI (using an ID from our namespace).
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Message-id: 1486382139-30630-3-git-send-email-kraxel@redhat.com
The nec/renesas driver problems have finally been debugged and root
caused, see commit "7da76e1 xhci: fix event queue IRQ handling".
It's pretty clear now that
(a) The whole "driver can't handle ring full" story is most likely
wrong.
(b) The ER_FULL_HACK workaround based on the false assumtion doesn't
much. It avoids the driver crashing (without commit 7da76e1), but
it doesn't make usb work.
(c) With 7da76e1 applied it doesn't trigger any more.
So, lets kill it. Or, to be exact, lets almost kill it. Some data
fields are kept unused in the state struct, for live migration backward
compatibility.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Message-id: 1486382139-30630-2-git-send-email-kraxel@redhat.com
Limits should be big enough that normal guest should not hit it.
Add a tracepoint to log them, just in case. Also, while being
at it, log the existing link trb limit too.
Reported-by: 李强 <liqiang6-s@360.cn>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Message-id: 1486383669-6421-1-git-send-email-kraxel@redhat.com
The qemu xhci emulation doesn't handle the ERDP_EHB flag correctly.
When the host adapter queues a new event the ERDP_EHB flag is set. The
flag is cleared (via w1c) by the guest when it updates the ERDP (event
ring dequeue pointer) register to notify the host adapter which events
it has fetched.
An IRQ must be raised in case the ERDP_EHB flag flips from clear to set.
If the flag is set already (which implies there are events queued up
which are not yet processed by the guest) xhci must *not* raise a IRQ.
Qemu got that wrong and raised an IRQ on every event, thereby generating
spurious interrupts in case we've queued events faster than the guest
processed them. This patch fixes that.
With that change in place we also have to check ERDP updates, to see
whenever the guest has fetched all queued events. In case there are
still pending events set ERDP_EHB and raise an IRQ again, to make sure
the events don't linger unseen forever.
The linux kernel driver and the microsoft windows driver (shipped with
win8+) can deal with the spurious interrupts without problems. The
renesas windows driver (v2.1.39) which can be used on older windows
versions is quite upset though. It does spurious ERDP updates now and
then (not every time, seems we must hit a race window for this to
happen), which in turn makes the qemu xhci emulation think the event
ring is full. Things go south from here ...
tl;dr: This is the "fix xhci on win7" patch.
Cc: M.Cerveny@computer.org
Cc: 1373228@bugs.launchpad.net
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Message-id: 1486104705-13761-1-git-send-email-kraxel@redhat.com
xhci_submit and xhci_fire_ctl_transfer are is called from
xhci_kick_epctx processing loop only, so there is no need to call
xhci_kick_epctx make sure processing continues. Also eecursive calls
into xhci_kick_epctx can cause trouble.
Drop the xhci_kick_epctx calls.
Cc: 1653384@bugs.launchpad.net
Fixes: 94b037f2a4
Reported-by: Fabian Lesniak <fabian@lesniak-it.de>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Message-id: 1485790607-31399-4-git-send-email-kraxel@redhat.com
Make clear that this isn't guaranteed to actually complete the transfer,
the usb packet can still be in flight after calling that function.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Message-id: 1485790607-31399-3-git-send-email-kraxel@redhat.com
msix_init() reports errors with error_report(), which is wrong when
it's used in realize(). The same issue was fixed for msi_init() in
commit 1108b2f. In order to make the API change as small as possible,
leave the return value check to later patch.
For some devices(like e1000e, vmxnet3, nvme) who won't fail because of
msix_init's failure, suppress the error report by passing NULL error
object.
Bonus: add comment for msix_init.
CC: Jiri Pirko <jiri@resnulli.us>
CC: Gerd Hoffmann <kraxel@redhat.com>
CC: Dmitry Fleytman <dmitry@daynix.com>
CC: Jason Wang <jasowang@redhat.com>
CC: Michael S. Tsirkin <mst@redhat.com>
CC: Hannes Reinecke <hare@suse.de>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Alex Williamson <alex.williamson@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Marcel Apfelbaum <marcel@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
usb_xhci_realize() corrects invalid values of property "intrs"
automatically, but the uncorrected value is passed to msi_init(),
which chokes on invalid values. Delay that until after the
correction.
Resources allocated by usb_xhci_init() are leaked when msi_init()
fails. Fix by calling it after msi_init().
CC: Gerd Hoffmann <kraxel@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Marcel Apfelbaum <marcel@redhat.com>
CC: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Acked-by: Marcel Apfelbaum <marcel@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
The vmstate_pci_device and vmstate_pcie_devices differ
just in the size of one buffer; combine the two using a _TEST
macro.
I think this is safe as long as everywhere which currently
uses either of these two uses the right type.
One thing that concerns me is that some places use pci_device_load/save
which does some irq mangling, but others just use the VMSTATE_PCI_DEVICE
macro - how are they getting the same irq mangling?
This passes a smoke test migrate of:
./x86_64-softmmu/qemu-system-x86_64 -M pc,accel=kvm -m 1024
./littlefed20.img -device e1000e -device virtio-net -device
e1000 -device virtio-rng -device megasas -device megasas-gen2 -device
ioh3420 -device nec-usb-xhci
to an unmodified qemu.
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Message-Id: <20161214195829.18241-1-dgilbert@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
All callsites have a XHCIEPContext pointer anyway, so we can just pass
it directly instead of fiddeling with slotid and epid.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Message-id: 1474965172-30321-9-git-send-email-kraxel@redhat.com
xhci_kick_epctx is a xhci_kick_ep variant which takes an XHCIEPContext
as input instead of slotid and epid. So in case we have a XHCIEPContext
at hand at the callsite we can just pass it directly.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Message-id: 1474965172-30321-7-git-send-email-kraxel@redhat.com
xhci has a fixed number of 24 (TD_QUEUE) XHCITransfer structs per
endpoint, which turns out to be a problem for usb3 devices with 32 (or
more) bulk streams. xhci re-checks the trb rings on every finished
transfer to make sure it'll pick up any pending work. But that scheme
breaks in case the first transfer of a ring can't be started because we
ran out of XHCITransfer structs already.
So remove static XHCITransfer array from XHCIEPContext. Use a linked
list instead, and allocate/free XHCITransfer as needed. Add helper
functions to allocate & initialize and to cleanup & release
XHCITransfer structs. That also simplifies trb management, we never
have to realloc XHCITransfer->trbs because we don't reuse XHCITransfer
structs any more.
New dynamic limit for in-flight xhci transfers per endpoint is
number-of-streams + 16.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Message-id: 1474965172-30321-5-git-send-email-kraxel@redhat.com
EV_QUEUE must not change because an array of that size is part of live
migration data. Hard-code current value there, so we can touch TD_QUEUE
without breaking live migration.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Message-id: 1474965172-30321-3-git-send-email-kraxel@redhat.com
Needed to avoid we run in circles forever in case the guest builds
an endless loop with link trbs.
Reported-by: Li Qiang <liqiang6-s@360.cn>
Tested-by: P J P <ppandit@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Message-id: 1476096382-7981-1-git-send-email-kraxel@redhat.com
If the xhci uses msix, it doesn't free the corresponding
memory, thus leading a memory leak. This patch avoid this.
Signed-off-by: Li Qiang <liqiang6-s@360.cn>
Message-id: 57d7d2e0.d4301c0a.d13e9.9a55@mx.google.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Previously the code would incorrectly report the remainder as 8 bytes. A
remainder of 0 bytes should be reported when the SETUP packet is
successfully transferred. Found using FreeBSD's XHCI driver.
Signed-off-by: Hans Petter Selasky <hps@selasky.org>
[ kraxel: codestyle fixup ]
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
A static analysis tool called BEAM detected possible side effect from
assert() calling a helper which may change an XHCI ring after every call.
This moves xhci_ring_fetch() out of assert() so it will be called
with and without enabled debug.
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Message-id: 1468812548-31868-1-git-send-email-aik@ozlabs.ru
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
This patch is a rough fix to "hw/usb/core.c:401: usb_handle_packet:
Assertion `dev->state == 3' failed.". Qemu will crash when a usb3
device redirect to Windows7 VM via nec-usb-xhci.
In extensible-host-controler-interface-usb-xhci.pdf P94(4.6.5
Address Device):
• If the Block Set Address Request (BSR) flag = ‘1’
• If the slot is in the Enabled state:
...
• Set the Slot State in the Output Slot Context to Default.
BSR = ‘1’: Enabled state to Default state; BSR = ‘0’: Default state
to Addressed state. Try to call usb_device_reset to set device state
to USB_STATE_DEFAULT in xhci_address_slot wether bsr is zero.
Signed-off-by: Zhang Shuaiyi <zhang_syi@massclouds.com>
Message-id: 1467258640-11921-1-git-send-email-zhang_syi@massclouds.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
msi_init() reports errors with error_report(), which is wrong
when it's used in realize().
Fix by converting it to Error.
Fix its callers to handle failure instead of ignoring it.
For those callers who don't handle the failure, it might happen:
when user want msi on, but he doesn't get what he want because of
msi_init fails silently.
cc: Gerd Hoffmann <kraxel@redhat.com>
cc: John Snow <jsnow@redhat.com>
cc: Dmitry Fleytman <dmitry@daynix.com>
cc: Jason Wang <jasowang@redhat.com>
cc: Michael S. Tsirkin <mst@redhat.com>
cc: Hannes Reinecke <hare@suse.de>
cc: Paolo Bonzini <pbonzini@redhat.com>
cc: Alex Williamson <alex.williamson@redhat.com>
cc: Markus Armbruster <armbru@redhat.com>
cc: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Hannes Reinecke <hare@suse.com>
>From bit to enum OnOffAuto
cc: Gerd Hoffmann <kraxel@redhat.com>
cc: Michael S. Tsirkin <mst@redhat.com>
cc: Markus Armbruster <armbru@redhat.com>
cc: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
This patch is a rough fix to a memory corruption we are observing when
running VMs with xhci USB controller and OVMF firmware.
Specifically, on the following call chain
xhci_reset
xhci_disable_slot
xhci_disable_ep
xhci_set_ep_state
QEMU overwrites guest memory using stale guest addresses.
This doesn't happen when the guest (firmware) driver sets up xhci for
the first time as there are no slots configured yet. However when the
firmware hands over the control to the OS some slots and endpoints are
already set up with their context in the guest RAM. Now the OS' driver
resets the controller again and xhci_set_ep_state then reads and writes
that memory which is now owned by the OS.
As a quick fix, skip calling xhci_set_ep_state in xhci_disable_ep if the
device context base address array pointer is zero (indicating we're in
the HC reset and no DMA is possible).
Cc: qemu-stable@nongnu.org
Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
Message-id: 1462384435-1034-1-git-send-email-rkagan@virtuozzo.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
All the callers for xhci_dma_write_u32s() are using mostly 5 * uint32_t
in len. To avoid unbound stack warning for the function, make it
statically allocated, and assert when it's not big enough in the
future.
Signed-off-by: Peter Xu <peterx@redhat.com>
Message-id: 1457661106-9569-1-git-send-email-peterx@redhat.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Clean up includes so that osdep.h is included first and headers
which it implies are not included manually.
This commit was created with scripts/clean-includes.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Message-id: 1453832250-766-20-git-send-email-peter.maydell@linaro.org
g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer,
for two reasons. One, it catches multiplication overflowing size_t.
Two, it returns T * rather than void *, which lets the compiler catch
more type errors.
This commit only touches allocations with size arguments of the form
sizeof(T). Same Coccinelle semantic patch as in commit b45c03f.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
The free() and g_free() functions both happily accept
NULL on any platform QEMU builds on. As such putting a
conditional 'if (foo)' check before calls to 'free(foo)'
merely serves to bloat the lines of code.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
This reverts commit 4e8cfbe114.
We should not poll via timer, and with ccid being fixed
to properly notify us about pending transfers we don't have to.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Value from xfer->packet.ep is assigned to ep here, but that
stored value is not used before it is overwritten. Remove it.
Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
When we find a IOC bit set on a setup trb and therefore queue an event,
that should not stop events being generated for following data trbs.
So clear the 'reported' flag.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
This makes xhci generate multiple short packet events in case of
multi-trb transfers. Which is wrong. We need to fix this in a
different way.
This reverts commit aa6857891d.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
A bunch of fixes all over the place.
All of ACPI refactoring has been merged.
Legacy pci commands have been dropped.
virtio header cleanup
initial patches from virtio-1.0 branch
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
iQEcBAABAgAGBQJU/CoXAAoJECgfDbjSjVRpX7EH/RMmgtsDO4wvqJu++lHvkB/q
kSaXZYTpJTo0i5JE7n2brwuXA4902tTg9g5TMUpGPh9Pt2QRg7RTgGC1vqZyOBos
MPw+4BO2v66S6qgX7bOf222z7r64cHTY7pLkQlrfD4usPlu2eusZ64UTW6Ru51fW
WF9E9aunbl+HnuCGq6Iez3sCLscTBJpU/lEr6oSyHhuq3aa0CjjraEeV0E/QcwJG
HTUeFymL8NFvlXZblsLI++VOv7Mxpi6yiCQ5XoKpFgGMvidwo41Aso6gB3ySGxOd
w8O3Nbu77Iw/StDRNCg/5/GapabMKh2bE4UCsYY5OS63ZtD0fl0CCblhzm/ZFPw=
=LY/j
-----END PGP SIGNATURE-----
Merge remote-tracking branch 'remotes/mst/tags/for_upstream' into staging
pci, pc, virtio fixes and cleanups
A bunch of fixes all over the place.
All of ACPI refactoring has been merged.
Legacy pci commands have been dropped.
virtio header cleanup
initial patches from virtio-1.0 branch
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
* remotes/mst/tags/for_upstream: (130 commits)
acpi: drop unused code
aml-build: comment fix
acpi-build: fix typo in comment
acpi: update generated files
vhost user:support vhost user nic for non msi guests
aml-build: fix build for glib < 2.22
acpi: update generated files
Makefile.target: binary depends on config-devices
acpi-test-data: update after pci rewrite
acpi, mem-hotplug: use PC_DIMM_SLOT_PROP in acpi_memory_plug_cb().
pci-hotplug-old: Has been dead for five major releases, bury
pci: Give a few helpers internal linkage
acpi: make build_*() routines static to aml-build.c
pc: acpi: remove not used anymore ssdt-[misc|pcihp].hex.generated blobs
pc: acpi-build: drop template patching and create PCI bus tree dynamically
tests: ACPI: update pc/SSDT.bridge due to new alg of PCI tree creation
pc: acpi-build: simplify PCI bus tree generation
tests: add ACPI blobs for qemu with bridge cases
tests: bios-tables-test: add support for testing bridges
tests: ACPI test blobs update due to PCI0._CRS changes
...
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Conflicts:
hw/pci/pci-hotplug-old.c
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>
Convert the device models where initialization obviously can't fail.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Gonglei <arei.gonglei@huawei.com>
