Repository navigation
openhcl, opentmk: use uart_16550 for 16550 serial ports - #4645
Philipp Schuster (phip1611) wants to merge 2 commits into
Conversation
|
This PR modifies files containing For more on why we check whole files, instead of just diffs, check out the Rustonomicon |
e56afd8 to
be09db0
Compare
be09db0 to
51c3765
Compare
There was a problem hiding this comment.
🟡 Changes recommended
OpenHCL now ignores the advertised serial speed and always reconfigures COM3 to 115200 baud.
1 open finding
What changed in this PR
Replaces hand-written x86 16550 serial drivers in OpenHCL and OpenTMK with the shared uart_16550 crate.
Changes:
- Adds shared UART configuration and PIO-backed serial handling.
- Adds TDX and SNP custom UART backends.
- Updates dependencies and removes legacy serial implementations.
| File | Description |
|---|---|
Cargo.toml |
Adds the temporary uart_16550 Git dependency. |
Cargo.lock |
Locks UART and updated transitive dependencies. |
opentmk/opentmk_executor/Cargo.toml |
Adds the UART dependency. |
opentmk/opentmk_executor/src/serial.rs |
Migrates executor serial I/O. |
opentmk/opentmk_core/Cargo.toml |
Adds the UART dependency. |
opentmk/opentmk_core/src/tmk_logger.rs |
Migrates the COM2 logger. |
opentmk/opentmk_core/src/arch/x86_64/serial.rs |
Replaces the custom serial driver. |
openhcl/sidecar/Cargo.toml |
Adds the UART dependency. |
openhcl/sidecar/src/arch/x86_64/mod.rs |
Migrates sidecar COM3 output. |
openhcl/openhcl_boot/Cargo.toml |
Adds the UART dependency. |
openhcl/openhcl_boot/src/boot_logger.rs |
Migrates boot logging and initialization. |
openhcl/openhcl_boot/src/arch/x86_64/tdx.rs |
Implements the TDX UART backend. |
openhcl/openhcl_boot/src/arch/x86_64/snp.rs |
Implements the SNP UART backend. |
openhcl/minimal_rt/Cargo.toml |
Adds the UART dependency. |
openhcl/minimal_rt/src/arch/mod.rs |
Updates architecture exports. |
openhcl/minimal_rt/src/arch/x86_64/serial.rs |
Provides shared COM3 construction and configuration. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
51c3765 to
157aab8
Compare
bd2267d to
2529bd4
Compare
2529bd4 to
5a0f779
Compare
|
Hi Philipp, and thanks for the contributions. I'm hesitant to take this change, as we've had situations in the past where we've needed to do weird non-standard things in our devices for compatibility reasons, and those sorts of changes wouldn't make sense to upstream into a common shared crate. That said, I don't think we've ever had a need to do that with serial, since it's a pretty well-defined case. We'll discuss this internally. |
|
We discussed and are happy to take this once the updated version is released so it's no longer a git dependency. |
5a0f779 to
9fc3d68
Compare
|
cool. I just released v0.9.0 - I've updated the PR. |
| use opentmk_core::arch::serial::Serial; | ||
| pub use opentmk_core::arch::serial::SerialPort; | ||
| #[cfg(target_arch = "x86_64")] | ||
| use { |
There was a problem hiding this comment.
nit: keep usings unmerged
There was a problem hiding this comment.
applies to other files too
|
Other than the one style nit I think this LGTM |
|
Gotta love those broken SNP tests. You don't need to do anything for them, that's our (longstanding) bug |
Replace the local 16550 driver with the uart_16550 crate, so we share one maintained and hardware-tested driver instead of carrying our own. Unlike the old driver, uart_16550 checks that the device is present, verifies that the FIFO works, and asserts DTR and RTS. The old driver left DTR and RTS unset, so a remote using hardware flow control would not send to us.
Replace minimal_rt's 16550 driver with the uart_16550 crate. The TDX and SNP boot logs plug in their port I/O as custom backends, so all serial output goes through the same driver. uart_16550 is also more robust on real or quirky devices. It resets the whole line state (DLAB, word length, parity, break) instead of relying on what firmware left behind. It detects a missing device via the scratch register instead of writing to nothing. It checks that the FIFO actually works and otherwise falls back to sending byte by byte. It also discards stale input and sets the modem control lines like Linux. Unlike the old driver, uart_16550 programs the baud rate. The boot logger uses the speed that the host advertises in the device tree, which is also passed to Linux, so both agree on it. A failed TDCALL port read now returns all ones, as with SNP, instead of panicking. The host controls that result, and the driver then treats the device as absent.
9fc3d68 to
0d120aa
Compare
Replace the hand-rolled 16550 drivers in OpenHCL (boot shim, sidecar) and opentmk with the uart_16550 crate. All serial output now goes through one maintained driver; the TDX and SNP boot logs plug in their tdcall/GHCB port I/O as custom backends.
Why
I maintain uart_16550. It got a major overhaul this year with a focus on real hardware, and it already runs in Patina, Microsoft's Rust UEFI firmware (patina#1694). Compared to the replaced drivers, it:
Inside VMs, the output doesn't change visibly; the gains show up on physical or quirky UARTs.
Behavior changes
current-speedfrom the device tree that Linux also uses.Notes
uart_16550is a git dependency on its main branch, as the required changes are not released yet. This is temporary until the next release.LLM Disclosure: I used an LLM for assistance to navigate the repository. All changes verified manually. I did not run any integration tests of openvmm.