Skip to content

ipmi_kcs: extract no_std core and add C ABI staticlib for shared us - #3836

Draft
Manish Ranjan Mahanta (marma-dev) wants to merge 3 commits into
microsoft:mainfrom
marma-dev:user/marma/ipmi-kcs-ffi
Draft

Manish Ranjan Mahanta (marma-dev) wants to merge 3 commits into
microsoft:mainfrom
marma-dev:user/marma/ipmi-kcs-ffi

Conversation

@marma-dev

@marma-dev Manish Ranjan Mahanta (marma-dev) commented Jun 29, 2026

Copy link
Copy Markdown

Summary

Refactors the IPMI KCS BMC device into a portable no_std core and wraps it in a C-ABI static library so the exact same emulator can be linked into the C++ Legacy HCL stack — eliminating a parallel C++ reimplementation of the KCS state machine and SEL handling.

This is PR 3 of 4 in the IPMI KCS stack (stacked on ipmi_kcs-sel):

  • PR1 — the core ipmi_kcs device + resource handle.
  • PR2 — host the device in OpenHCL/underhill with SEL forwarding.
  • PR3 (this) — extract the no_std core + C-ABI staticlib for cross-language reuse.
  • PR4 — package/publish the staticlib via the flowey pipeline.

What's included

New no_std core — vm/devices/ipmi_kcs_core/

The protocol/SEL/KCS logic is moved out of ipmi_kcs into a dependency-light, no_std + alloc crate:

  • lib.rsKcsDevice state machine (io_read/io_write/reset), KcsError, port range constants.
  • protocol.rs — moved verbatim from ipmi_kcs.
  • sel.rs — SEL record assembly (now no_std).
  • sink.rsSelSink / NullSelSink / BmcClock / SelDeps traits.
  • Optional features: inspect (gated derive(Inspect)) and trace (gated tracelimit), both no-ops when off so the core stays portable.

New C-ABI library — vm/devices/ipmi_kcs_ffi/

A staticlib + rlib crate exposing a flat C interface over the core:

  • src/lib.rsipmi_kcs_new / _free / _io_read / _io_write / _reset / _data_port / _status_port. SEL egress and the clock are supplied by the C++ caller via SelCallback / ClockCallback function pointers; CSelSink / CClock wrap the opaque context pointer.
  • ipmi_kcs.h — hand-authored C header for the C++ consumer.
  • Registered as a workspace leaf member (same model as the existing vmgs_lib staticlib).

ipmi_kcs device crate

Now a thin OpenVMM integration layer that re-exports the core and keeps only the OpenVMM-specific glue (ChipsetDevice, PortIoIntercept, save/restore, resolver). lib.rs shrinks ~600 lines; sink.rs is removed (moved to core).

Design notes

  • Single source of truth — the KCS/IPMI/SEL behavior now lives in one Rust crate consumed by OpenVMM, OpenHCL, and C++ HCL. No divergence between hosts.
  • C++ owns I/O policy — egress and time are injected across the FFI boundary via function pointers, mirroring the trait-injection pattern used in-tree (SelSink / BmcClock).
  • Small footprint — the FFI surface measures ~4.7 KB of code (.text 4576 B + .rdata 128 B + .pdata 144 B); the CRT is already present in the host C++ binary.

Testing

  • cargo test across the stack — 27 tests pass (core 22 + ipmi_kcs integration 3 + ffi 2).
  • Builds clean for x86_64-unknown-linux-musl and x86_64-pc-windows-msvc.
  • Fork validation pipeline produces the ipmi_kcs_ffi-x86_64-pc-windows-msvc staticlib artifact; dumpbin confirms the 7 exported C symbols.
  • The artifact links cleanly into the Legacy HCL (hcl/fw) build with no unresolved symbols.

Risk

Low — pure refactor plus additive crates. The ipmi_kcs device's externally observable behavior is unchanged (verified by the moved-over tests); no existing configuration consumes the new FFI crate.

Copilot AI review requested due to automatic review settings June 29, 2026 16:40
@github-actions github-actions Bot added the unsafe Related to unsafe code label Jun 29, 2026
@github-actions

Copy link
Copy Markdown

⚠️ Unsafe Code Detected

This PR modifies files containing unsafe Rust code. Extra scrutiny is required during review.

For more on why we check whole files, instead of just diffs, check out the Rustonomicon

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors the IPMI KCS BMC device into a reusable no_std + alloc core crate and adds a C-ABI staticlib wrapper so the same KCS/SEL implementation can be linked from non-Rust hosts (e.g., Legacy HCL C++), while keeping ipmi_kcs as a thin OpenVMM integration layer.

Changes:

  • Added ipmi_kcs_core (no_std + alloc) containing the KCS state machine, IPMI protocol types, and SEL handling with injectable sink/clock traits.
  • Added ipmi_kcs_ffi staticlib exposing a flat C ABI plus a hand-authored ipmi_kcs.h header.
  • Updated OpenVMM integration (ipmi_kcs, resources/resolver registration, workspace wiring) to consume the shared core.

Reviewed changes

Copilot reviewed 16 out of 17 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
vm/devices/ipmi_kcs/src/resolver.rs Adds a chipset resource resolver for the OpenVMM IPMI KCS device.
vm/devices/ipmi_kcs/src/lib.rs Implements the thin ChipsetDevice/PIO wrapper over ipmi_kcs_core + basic integration tests.
vm/devices/ipmi_kcs/Cargo.toml New crate manifest wiring ipmi_kcs to ipmi_kcs_core (inspect/trace enabled).
vm/devices/ipmi_kcs_resources/src/lib.rs Defines the resource handle (IpmiKcsHandle) for VM resource plumbing.
vm/devices/ipmi_kcs_resources/Cargo.toml New crate manifest for the resource handle crate.
vm/devices/ipmi_kcs_ffi/src/lib.rs Implements the C ABI wrapper over KcsDevice and includes ABI-driven unit tests.
vm/devices/ipmi_kcs_ffi/ipmi_kcs.h Provides the C header for consuming the staticlib from C/C++.
vm/devices/ipmi_kcs_ffi/Cargo.toml Declares staticlib + rlib crate types and depends on the core.
vm/devices/ipmi_kcs_core/src/sink.rs Adds portable sink/clock traits (SelSink, BmcClock) and dependency bundle (SelDeps).
vm/devices/ipmi_kcs_core/src/sel.rs Implements SEL storage + SEL IPMI command handling and tests.
vm/devices/ipmi_kcs_core/src/protocol.rs Defines KCS/IPMI protocol constants and open enums + tests.
vm/devices/ipmi_kcs_core/src/lib.rs Implements the core KCS state machine and integrates SEL + tests.
vm/devices/ipmi_kcs_core/Cargo.toml New core crate manifest with inspect/trace features.
openvmm/openvmm_resources/src/lib.rs Registers the IPMI KCS resolver under cfg(guest_arch = "x86_64").
openvmm/openvmm_resources/Cargo.toml Adds ipmi_kcs dependency for openvmm_resources (currently host-arch gated).
Cargo.toml Adds new workspace members and workspace dependency paths for the new crates.
Cargo.lock Records the newly added workspace packages and dependency edges.

Comment on lines 108 to 112
[target.'cfg(target_arch = "x86_64")'.dependencies]
ipmi_kcs.workspace = true
serial_16550.workspace = true
serial_debugcon.workspace = true

fn kcs_get_device_id() {
let mut dev = new_device();
let resp = kcs_transfer(&mut dev, &[0x18, 0x01]);
assert!(resp.len() >= 3, "response too short: {:?}", resp);
Manish Ranjan Mahanta added 3 commits June 30, 2026 09:55
Import the IPMI KCS device crate and add production-readiness changes over the PoC:

- SelSink trait for forwarding SEL entries to a host (no-op by default); enables OpenHCL to publish guest SEL to host ETW.

- BmcClock trait to remove std::time dependency from the SEL store; SystemClock default, injectable for paravisor.

- with_deps constructors on IpmiKcsDevice/SelStore; register resolver in openvmm_resources (x86_64).

- Tests for sink egress and injected clock.
- protocol.rs: use open_enum's inner #![expect(missing_docs)] idiom instead of an outer #[allow], so it reaches the generated associated constants (protocol module is pub).

- sel.rs: SelStore::new() is only used by tests now that the lib paths construct via with_deps; move it into a #[cfg(test)] impl.

- Cargo.lock: add ipmi_kcs / ipmi_kcs_resources entries.
Split the IPMI KCS BMC into a reusable core so OpenHCL (Rust) and the Legacy HCL C++ (OS Repo) share one implementation instead of duplicating it:

- ipmi_kcs_core (new): no_std + alloc KCS state machine + SEL. protocol/sel moved here; SystemClock/std::time removed in favor of injected BmcClock; inspect and trace are optional features (off by default) so minimal consumers link neither.

- ipmi_kcs: now a thin OpenVMM integration layer wrapping ipmi_kcs_core::KcsDevice (ChipsetDevice/PortIoIntercept/InspectMut/save-restore + std SystemClock).

- ipmi_kcs_ffi (new): C ABI staticlib (+ ipmi_kcs.h) exposing new/free/io_read/io_write/reset with SEL + clock callbacks; std shim per the vmgs_lib precedent.

Tests: core 22, integration 3, ffi 2 (incl. C-ABI Get Device ID + SEL callback). 0 warnings.
@benhillis

Copy link
Copy Markdown
Member

Manish Ranjan Mahanta (@marma-dev), active PR #4396 appears to replace this no_std/C-ABI direction, which also has unresolved input-safety issues. Could you close it, or revive only the distinct remaining work? Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

unsafe Related to unsafe code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants