bhyve: xhci: do not interrupt while the event handler is busy

[wanpengqian_gmail.com]

Summary

pci_xhci_assert_interrupt() set IMAN.IP and the ERDP Event Handler Busy flag
and raised an interrupt for every event, even while EHB was already set, that
is while the guest's handler was still working through the Event Ring. Per
xHCI 4.17.2 an interrupter does not interrupt again while the event handler is
busy; it fires again when software clears EHB (an ERDP write) and the ring is
not empty.

Follow that, and have EHB mean what the spec says. While EHB is set only post
the event. When an ERDP write clears EHB, recount the ring and raise again if
events remain, so an event that arrived between the handler's last read and
its ERDP write is not lost. A controller reset returns the interrupter
registers to their defaults so an EHB left by the firmware cannot block the
first interrupt.

Crucially, set EHB only when an interrupt is actually signalled -- IMAN.IE and
USBCMD.INTE both on -- not merely when an event is posted, and signal a pending
interrupt once the guest turns IE or INTE on (xHCI 4.17.2: the interrupt is
asserted while IP, IE and INTE are all set; INTE is stored before the run
posts its port change events). Testing across guests showed this has to be
general: a guest that posts events before enabling interrupts would otherwise
latch EHB with nothing signalled and have every later event held back.
FreeBSD and Linux enable interrupts first and were already correct; Windows
enables the controller and interrupts in one USBCMD write, and macOS programs
IMAN before HCRST and enables IE only after the run -- both need the pending
interrupt delivered when interrupts come on.

Found and fixed in keelOS (keelos.dev).
Signed-off-by: Wanpeng Qian <[e-mail]>
Sponsored by: keelos.dev

Test Plan

Builds with and without WITH_BHYVE_SNAPSHOT; each patch in the series compiles
on its own.

Regression on the nested FreeBSD-main tester (FreeBSD guest, xhci,tablet):

• INTx, xhci on a pin shared with ahci0: boots, irq idle 0/s, no interrupt storm (VM exits ~1200/s), ~29 handler calls across boot.
• MSI, xhci on its own pin: boots, idle 0/s, ~30 at boot, the tablet delivers.

These match the numbers before this revision; FreeBSD and Linux enable
interrupts before traffic and were already correct.

The general rule was validated on the two guests that post events before
enabling interrupts, on real hardware (keelOS):

• Windows 10 enables the controller and interrupts in one USBCMD write (RS | INTE); with the earlier revision every event after the run was held back by an EHB set with nothing signalled and Windows reset the controller every ~5 s. With "EHB only when signalled" plus the interrupt delivered when INTE comes on, Windows starts cleanly and a 5-minute drag storm causes 0 resets.
• macOS Sonoma programs IMAN before HCRST and turns IE on only after the run; the earlier revision left EHB stuck and its command completion was never delivered (no USB). With this revision macOS USB works.

Found and fixed in keelOS.

[kevans]

What's your takeaway on the results from your testing? I don't have a well-informed perspective here, but that feels like a modest but respectable enough win to make it worth doing (particularly when we finish getting USB device passthrough and can then support a wider variety of devices and usage patterns).

[wanpengqian_gmail.com]

My takeaway: this is mainly a correctness fix, with a modest efficiency win on top.

The correctness part is what led us here. Under INTx, bhyve's xHCI left its line asserted after the firmware started the controller, so a FreeBSD guest on its own pin ran xhci_interrupt() ~18,000 times in ten idle seconds, and a guest sharing the pin (with the AHCI, or an e1000) could wedge in an interrupt storm. D60274 fixed the idle case; this revision removes the rest. The ones left after D60274 were spurious: an event arriving while the guest's handler ran got acknowledged through ERDP, but the IP/EHB state was left so that every later entry found nothing pending. With the EHB handling the guest now takes exactly one interrupt per line raise -- 29 across a boot -- and the re-arm after an ERDP write that clears EHB with the ring non-empty makes sure no event is dropped in the process (it fires 23 times under INTx and 99 under MSI during a 3,500-move burst, every event accounted for).

The efficiency part is the MSI side: one message per handler run instead of one per event batch -- 27 messages for 27 handler calls at boot, where stock sent 99 for 42. Modest, as you say, but it is fewer VM exits on a hot path, and I agree it matters more once USB device passthrough widens the range of devices and traffic patterns.

For context on where this comes from: we are building keelOS (keelos.dev), a bhyve-based virtualization platform for personal and small-office use. Running real guest workloads on it -- Windows, macOS and Linux, with checkpoint/restore for live reload and migration, PCI passthrough, and netmap/VALE networking -- keeps turning up bhyve and vmm issues that we fix and send upstream. This xHCI one we hit on a Windows guest resuming from hibernation: its xHCI shared an I/O APIC pin with an e1000, and the line the firmware left asserted became an interrupt storm during resume, before the guest re-enabled MSI.

Others from the same effort already in review: snapshot/restore correctness (RTC, NVMe, PCI BAR restore, pending-event and INTx state across checkpoints), a task-switch EFLAGS fix, e1000 interrupt and receive-address fixes, netmap empty-packet handling for VALE, and NVMe boot/shutdown parallelism. Glad to be pointed at better ways to structure or test any of them -- still finding my feet with the process here.

[kevans]

In D60413#1386055, @wanpengqian_gmail.com wrote:

My takeaway: this is mainly a correctness fix, with a modest efficiency win on top.

[...]

Thanks, that makes sense.

For context on where this comes from: we are building keelOS (keelos.dev), a bhyve-based virtualization platform for personal and small-office use. Running real guest workloads on it -- Windows, macOS and Linux, with checkpoint/restore for live reload and migration, PCI passthrough, and netmap/VALE networking -- keeps turning up bhyve and vmm issues that we fix and send upstream. This xHCI one we hit on a Windows guest resuming from hibernation: its xHCI shared an I/O APIC pin with an e1000, and the line the firmware left asserted became an interrupt storm during resume, before the guest re-enabled MSI.

Oh, that's neat!

Others from the same effort already in review: snapshot/restore correctness (RTC, NVMe, PCI BAR restore, pending-event and INTx state across checkpoints), a task-switch EFLAGS fix, e1000 interrupt and receive-address fixes, netmap empty-packet handling for VALE, and NVMe boot/shutdown parallelism. Glad to be pointed at better ways to structure or test any of them -- still finding my feet with the process here.

Nice, appreciate the work here. I can't really speak for bhyve, but throwing out my $0.02 from glancing at your review list and from my brief interactions with you for some positive reinforcement: I think you're doing well here and the granularity of change that you're pitching (on average, I haven't looked at every change) is about what we would nomally like to see from newer or less frequent contributors. I also really appreciate the additional testing details/measurements to get a feel for how you're validating the work. I would note that you've opened quite a few reviews in just a week so it may take us a bit to get around to all of them, but I don't see a reason offhand that they wiil languish beyond reviewer capacity issues- if they start to get a bit stale, it's probably worth a ping.

[wanpengqian_gmail.com]

Thanks, that's genuinely helpful to hear, and good to know the cadence is about right.

On the volume: we've been building keelOS for a while and had already made most of these fixes and been carrying them in-tree. We only recently decided to feed back the ones we're confident belong upstream instead of keeping them forever, so the burst this week is us catching up on that backlog, not our steady rate. No rush on any of them; I'll ping if something goes stale.

One thing I'd value your read on before I open more. A good part of our work is support for older guests: letting bhyve boot legacy-BIOS operating systems (a SeaBIOS/SeaVGABIOS path and the VGA modes they drive), PC-style PCI interrupt routing for those guests, a PS/2 keyboard for guests started without firmware, and similar. Some of it is plain bug fixes we have already sent -- the task-switch EFLAGS one, for instance -- but a lot of it is added functionality rather than a fix. Is that kind of feature work something bhyve would want upstream, or is it better kept out of tree? I'd rather not fill the queue with features if the appetite is mainly for fixes.

[kevans]

Cc'ing @jhb specifically, as he may be better equipped to answer re: appetite for features in support of legacy BIOS.

[wanpengqian_gmail.com]

EHB only when an interrupt is signalled; deliver a pending interrupt when IE/INTE turn on; INTE stored before the run (Windows, macOS). New parent: the event-ring lock fix.