Page MenuHomeFreeBSD

bhyve: xhci: serialise the device callbacks without the controller lock
Needs ReviewPublic

Authored by wanpengqian_gmail.com on Wed, Oct 7, 6:22 AM.
Tags
None
Referenced Files
F175420961: D60432.diff
Sat, Oct 10, 6:02 PM
F175409845: D60432.id.diff
Sat, Oct 10, 4:12 PM
F175382305: D60432.diff
Sat, Oct 10, 10:52 AM
F175382296: D60432.diff
Sat, Oct 10, 10:52 AM
F175358656: D60432.id189006.diff
Sat, Oct 10, 6:44 AM
F175358430: D60432.id189006.diff
Sat, Oct 10, 6:41 AM
F175358422: D60432.diff
Sat, Oct 10, 6:41 AM
F175358420: D60432.id189006.diff
Sat, Oct 10, 6:41 AM
Subscribers

Details

Reviewers
kevans
Group Reviewers
bhyve
Summary

A backend delivers completions and hotplug events through hci_intr /
hci_event from its own thread (usb_mouse: the console thread; usb_passthru:
its libusb and hotplug threads), and usb_passthru does so holding the
endpoint's xfer lock, which the vCPU takes under sc->mtx. So the callbacks
cannot take sc->mtx; give each piece of state they touch its own leaf lock:

  • the endpoint's transfer ring state and queued blocks: the endpoint's xfer lock. device_doorbell() takes it before reading ep_ringaddr/ep_ccs and the stream rings; handle_transfer() and try_usb_xfer() run with it held (try_usb_xfer() used to lock it again inside handle_transfer(), which on the default error-checking mutex failed and then released the lock early). Reset/Stop Endpoint and Set TR Dequeue update the ring under it.
  • the port state: port_mtx.
  • the interrupter state (IMAN.IP, ERDP.EHB, USBSTS.EINT, INTx/MSI): intr_mtx, taken by assert/deassert_interrupt() and the IMAN/ERDP writes.
  • USBSTS: atomic.
  • the event ring: event_mtx (D59779).

The lock order is sc->mtx -> xfer lock -> { port_mtx | event_mtx | intr_mtx }.

Without this the tablet's reports (console thread) walked the transfer ring
and wrote the event ring concurrently with a vCPU doorbell: on a Windows 10
guest driven over remote desktop, fast pointer input made Windows reset the
controller again and again (Stop Endpoint then HCRST) and finally drop the
tablet.

Test Plan

Nested FreeBSD-main tester (FreeBSD guest, xhci,tablet), full stack D51735 + D59779-D59785 + this revision + D60413 + D60274:

  • INTx, xhci sharing a pin with ahci0: boots in 30 s, idle interrupt rate 0/s, 50 slow tablet moves -> 96 xhci_interrupt() calls / 2424 evdev bytes, 3500-move burst -> 2252 calls / 49248 bytes, detach/attach 51 calls.
  • MSI, xhci on its own pin: 28 interrupts at boot, idle 0/s, 76 / 2424, burst 3168 / 108864, detach/attach 30 calls.
  • No controller reset, no lost event; bhyve stays up.

This revision alone on the series (without D60413/D60274) boots and delivers under MSI as well (72 / 2424, burst 4219 / 89856).
Builds with and without WITH_BHYVE_SNAPSHOT.
The Windows 10 reproduction (keelOS, scripted RDP drag storm: repeated Stop Endpoint + HCRST, tablet dropped) was done with the previous revision of this review; not yet re-run with this one.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 77795
Build 74678: arc lint + arc unit

Event Timeline

I note that I have D59779 that was about to land; it might make sense to rebase this onto that one and use the finer-grained event mutex? I think both the original author of that one and I overlooked this one.

Thanks for the pointer, I had missed D59779. I applied its diff on main and stacked this commit on top: no textual overlap, it builds, and the locking is consistent with the order you documented (sc->mtx -> [xfer_lock ->] event_mtx), since this path takes sc->mtx before pci_xhci_insert_event() takes event_mtx.

I don't think event_mtx can replace this one, though. The race here is wider than the er_enq_idx / er_events_cnt bookkeeping: pci_xhci_dev_intr() also writes PORTSC (U3 resume), reads the endpoint context and then calls pci_xhci_device_doorbell(), which walks the endpoint's transfer ring, updates the endpoint and stream contexts and the dequeue pointers, and ends in pci_xhci_assert_interrupt() touching IMAN/USBSTS and the INTx/MSI state. All of that runs on the backend thread with no lock while a vCPU can be in the same code under sc->mtx (a doorbell write for the same endpoint, runtime-register writes, HCRST). The Windows symptom we chased (repeated Stop Endpoint + HCRST during fast tablet input over RDP, then the tablet gone) comes from that wider race, so serialising only the event-ring counters leaves it in place.

To fold this into D59779's scheme, event_mtx would have to cover the whole device interrupt path (doorbell processing, port state, interrupt assertion), i.e. it would grow into sc->mtx and could no longer stay outside pci_generate_msi() as your comment requires. So I'd keep both: D59779 for the fine-grained event-ring bookkeeping (which the passthru libusb thread needs), this one for the hci_intr path. The two can land in either order; I've added D59779 as a parent here to record the stacking. D60413 (the child of this one) touches pci_xhci_reset() and the ERDP handlers, so it will need a small merge against D59779's locking (take event_mtx around the memset / pci_xhci_update_er_events_cnt() calls); I'll update it once D59779 lands.

Regression with main + D59779 + this + D60413 + D60274 on the nested FreeBSD-main tester (FreeBSD guest, xhci,tablet): the guest boots with xhci on a pin shared with ahci0 under INTx and on its own pin under MSI (28 interrupts at boot), idle interrupt rate 0/s in both; 50 slow tablet moves give 76 xhci_interrupt() calls and 2424 bytes on the evdev node in both modes; a burst of 3500 moves gives 2190 calls / 46464 bytes (INTx) and 2988 calls / 94176 bytes (MSI), no controller reset, no event lost. Same numbers as without D59779.

Yeah, sorry, I was actually thinking of a WIP one that isn't yet finished: https://paste.fbsd.dev/7kvf/ -- the prototype for this one was a pass with claude for the fun of it, and I haven't had time yet to sanity check what it did or what I asked it to do. I was going to switch USBSTS to stdatomic rather than putting it under any lock in particular, but I'm not sure that's actually the sensible thing to do.

wanpengqian_gmail.com retitled this revision from bhyve: xhci: take the controller lock in the device interrupt path to bhyve: xhci: serialise the device callbacks without the controller lock.
wanpengqian_gmail.com edited the test plan for this revision. (Show Details)

New version: fine-grained locks (xfer lock for the ring state, intr_mtx, port_mtx + atomic USBSTS from kevans WIP), stacked on D51735 + D59779-D59785; see the comment on D60432

Thanks -- the WIP changed my picture, so here is what I found and what I did with it.

First, I don't think the self-deadlock described in the WIP's port_mtx comment can happen on FreeBSD: libusb_control_transfer() in our libusb is libusb20_dev_request_sync(), a synchronous USB_DO_REQUEST ioctl, so it never pumps the event loop; and libusb10_handle_events_sub() invokes transfer callbacks with the context unlocked. hci_intr()/hci_event() therefore only ever run on a backend's own thread (usb_mouse: the console thread; usb_passthru: its libusb thread and the hotplug thread), never re-entrantly on a vCPU that holds sc->mtx. (The recursive-mutex comment in D52166's usb_passthru.c describes upstream libusb-1.0 behaviour, not ours.)

What does rule out sc->mtx in the callbacks is lock order: D52166's completion callback calls hci_intr() with the endpoint's xfer lock held, and the vCPU takes that same lock under sc->mtx (handle_transfer), so a callback taking sc->mtx is ABBA. So I followed your direction and finished the WIP; this revision's new diff is that, on top of D51735 + D59779-D59785, with your port_mtx / atomic USBSTS parts kept as they were and you as co-author (please commandeer if you'd rather own it):

  • The endpoint's ring state (ep_ringaddr/ep_ccs, the stream rings, the queued blocks) is covered by that endpoint's xfer lock: device_doorbell() takes it before reading the dequeue state, handle_transfer() and try_usb_xfer() run with it held, and Reset/Stop Endpoint and Set TR Dequeue Pointer update the ring under it. Incidentally, today try_usb_xfer() locks the xfer again inside handle_transfer(); on the default (error-checking) mutex the second lock fails and the inner unlock drops the lock early, so the existing lock did not actually cover the data path. This is the part that fixes the Windows tablet case from the previous revision.
  • A leaf intr_mtx for the interrupter state (IMAN.IP, ERDP.EHB, USBSTS.EINT, the INTx line / MSI), taken by assert/deassert_interrupt() and the IMAN/ERDP writes, so a callback and the vCPU don't race on those read-modify-writes (D60413's EHB logic needs this).
  • port_mtx and atomic USBSTS from your paste; I rewrote the port_mtx comment to the lock-order rationale and added a summary of the scheme next to the locks.

Order: sc->mtx -> xfer lock -> { port_mtx | event_mtx | intr_mtx }; none of the leaves is held across a call into a backend or across pci_generate_msi() except intr_mtx, which is a leaf that protects exactly that. One pre-existing hole I left alone: disable_ep() frees ep_xfer under sc->mtx while a callback on another thread may still be using it; that wants a refcount or a per-slot lock and seemed out of scope here.

D60413 and D60274 are rebased on this version (D60413 touches pci_xhci_reset() and the ERDP handlers, now under intr_mtx/event_mtx).

Test (nested FreeBSD-main tester, FreeBSD guest with xhci,tablet, the full stack D51735 + D59779-D59785 + this + D60413 + D60274): INTx with xhci sharing a pin with ahci0: boots in 30 s, idle interrupt rate 0/s, 50 slow tablet moves 96 xhci_interrupt() calls / 2424 evdev bytes, 3500-move burst 2252 calls / 49248 bytes, detach/attach 51 calls; MSI on its own pin: 28 interrupts at boot, idle 0/s, 76 / 2424, burst 3168 / 108864, detach/attach 30 calls; no controller reset, no lost event. This revision alone on the series (without D60413/D60274) also boots and delivers under MSI (72 / 2424, burst 4219 / 89856). Builds with and without WITH_BHYVE_SNAPSHOT. The Windows reproduction (keelOS, scripted RDP drag storm) was done against the previous revision; I have not re-run it with this one yet.

wanpengqian_gmail.com edited the test plan for this revision. (Show Details)