Page MenuHomeFreeBSD

Add loader support for xhci debug
Needs ReviewPublic

Authored by thj on Oct 30 2025, 1:02 PM.
Tags
None
Referenced Files
F145182113: D53472.diff
Mon, Feb 16, 8:38 PM
Unknown Object (File)
Sat, Feb 7, 6:45 PM
Unknown Object (File)
Sat, Jan 31, 2:20 PM
Unknown Object (File)
Mon, Jan 26, 9:25 AM
Unknown Object (File)
Sun, Jan 25, 3:18 AM
Unknown Object (File)
Dec 22 2025, 12:48 AM
Unknown Object (File)
Dec 18 2025, 2:48 PM
Unknown Object (File)
Dec 14 2025, 9:16 PM

Details

Summary

This patch implements loader support for xhci debug and carries both the shared
code for both loader and the kernel.

I think there is still probably stuff to do in here, but I would really like
input from some other people on the overall structure and how to proceed.

Description

xhci debug requires an efi implementation with a working pcie bus and
implements the minimum of xhci required to interface with the controller and to
put a port into debug mode.

xhci debug functions as a console in loader and can be used from cons_probe
(quite early in loader/main.c:main). The cable detection process takes a long
time (on the order of a second) to run making enabling xhci debug by default an
impossible proposition.

It is possible to append "udbc" to the static console list in
efi/loader/main.c:main and a future patch will add this as a build option.
There are outstanding bugs with doing this.

Theory of Operation

udbc probes with the normal console list and allocates and creates its required
data structures during its probe/attach process.

Once probed udbc can be used as an input and output console from the C efi and
loader.

The data structures allocated for udbc are kept in a softc which is kept in
sync with the kernel. During boot the softc is bundled up and passed to the
kernel as a machine dependent module (MODINFOMD_XHCI_DEBUG).

The kernel picks up this data in parse_preload_data.

xhci debug is functional once the kernel has picked up the data structures, but
we are lacking an early pci controller to be able to do the required pcie
transactions until the xhci bus probes much later.

When used with a supported kernel, udbc is able to provide a console for
loader, with a gap between kernel start and xhci probe. xhci debug should
continue to be usable until the xhci controller is reset or the pcie bus is
reset.

Other platforms do not support its use during system or bus suspend - it isn't
clear if this is from experience or from attempts to make the problem space
smaller.

When the console wants to send data it adds the output bytes to a work_queue
and then uses the xhci trbs to submit this to the bus.

Periodically (on output and when polled by the console system) usb transfers
are driven and any input bytes are pulled from a work queue as rx data.

Usage

target: Build loader with support and deploy/install it

Connect your debug target to your debug host via a xhci debug cable (a usb3
cable with vbus removed on one end).

host: On your debug host load the udbc kernel module (freebsd 15 or later).

target: From loader menu break to a command prompt and enter

set console="udbc"
or
set console="comconsole udbc"

host: once the cable/target is detected you will have a functioning usb serial

console device

Known issues/caveats

  • there is a stall during loader start up which leads to a single screen of loader menu being rendered then no more if configured as a default console in efi/loader/main.c:main or loader.conf
  • usb-c cables require an adapter to enable debug adapter mode (I'm waiting for pcbs to verify this)
  • the "dead time" when udbc isn't available in the kernel is at the most annoying time.

Diff Detail

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

Event Timeline

thj requested review of this revision.Oct 30 2025, 1:02 PM
stand/efi/loader/xhci_dbc_dma.c
283

Should this be dma?

sys/dev/usb/controller/xhci_dbc.c
1055

where is label defined?

stand/efi/loader/xhci_dbc_cons.c
212

This should technically set errno = 0 before and check it after for overflow. Same below.

232

I'd feel a little better if the outer block here had braces

stand/efi/loader/xhci_dbc_dma.c
72

This kinda feels like it should be going through uintptr_t instead (along with the other UINTN usage here

83

Consider asserting the condition instead, since there shouldn't be any other path leading to this that leaves us with a non-NULL.

93
127

howmany?

131
sys/dev/usb/controller/xhci_dbc.c
644

This is presumably synchronizing with trb_push_locked? I think the barrier application is questionable at best, and this should probably be using proper atomics on both sides...

722

Ditto above note about atomics

thj marked 7 inline comments as done.Wed, Feb 11, 10:49 AM
kevans added inline comments.
sys/dev/usb/controller/xhci_dbc.c
644

More specifically, I'm pretty sure all of these loads from trb->dwTrb3 with rmb throughout should be an appropriate atomic(9) load_acq, and trb_push_locked should be rearranged such that the last write to the trb should be a store_rel to dwTrb3. This does mean getting rid of a cleaner memset, but I think the more narrow barrier is worth it. CC @kib, maybe

stand/efi/loader/bootinfo.c
469

Do you intent to pass pointer to the struct, or the struct itself? If a pointer, what makes the memory allocation survive to the kernel runtime?

stand/efi/loader/xhci_dbc_cons.h
28

Do we need include braces for double-inclusion protection?

Same question for all added headers.

sys/dev/usb/controller/xhci_dbc.c
30

The sys/systm.h should go first, then sys/param.h is not needed

35

We have _STANDALONE I remember. IMO it is (much) better to write #ifdef _STANDALONE than #ifndef _KERNEL.

644

What is the semantic needed from this barrier?

652

And from this one?

Hope these are helpful, lemme know if you have any questions / concerns. Thanks!

stand/efi/loader/bootinfo.c
73

Can you put this into a new file? bootinfo.c doesn't need this infection... It's shared with kboot, and I'd prefer we keep niche things like this out of there.

Maybe xhci_dbc_meta.c is a better place to implement the usbdbc_add_metadata routine I suggest below? It doesn't fit well with the other files, but it's also small so could fit into xhci_dbc_cons.c.

469

I'd prefer this be something like usbdbc_add_metaddata(kfp); and this code be elsewhere.

stand/efi/loader/xhci_dbc_cons.c
74

where's device_t defined? It looks like we're reading it from the PCI device, so that has me scratching my head... It's just the device id, so device_t seems like an odd choice. There doesn't seem any reason to make this "device_t" since all the definitions are self contained.

111

silly style nit: No space after (void).
But also, why have it return bool if we're just going to ignore it?

178

same comment

182

Please fix this to free it.

stand/efi/loader/xhci_dbc_pci.h
68

Yea, I'm not comfortable with this type punning for such a small gain below. There's no real need that I can see for doing this, apart from the one shared bit of code that can be done in a different manner.

sys/dev/usb/controller/xhci_dbc.c
35

I made all my comments about this before I Saw this coment.

55

This is unused and is the only use of device_t in this file.

119

In new code, we don't need (or really want) blank lines like this.

148

_STANDALONE

sys/dev/usb/controller/xhci_dbc.h
30

We usually spell this #ifdef _STANDALONE rather than !KERNEL.

143

same comment. Avoid !defined(KERNEL) constructs please.

201

I'd have two ifdefs. One for the kernel. and then one for _STANDALONE. The code will fail to build when one or the other isn't defined, which is what we want. In the loader, it's means someone messed up CFLAGS. and if we're not in the loader or in the kernel, we want to fail because we've not built out that other environment.

sys/dev/usb/controller/xhci_dbc_private.h
30

#if defined(KERNEL) is the right construct

44

Same applies here for earlier _STANDALONE comments.

sys/dev/usb/controller/xhci_pci.h
29

here, and elsewhere, sys/cdefs.h is almost certainly not needed.

42

See comment in another review, but make this take device_id, class, etc so we don't need device_t to seep into the loader. When I commented on the other review, I didn't notice the need to do the latter. Make the driver and the loader have a wrapper that extracts this information and calls this.