openhcl: add IPMI KCS emulation and SEL forwarding

[ayusharora221204]

Overview

Add a minimal virtual IPMI BMC to OpenHCL so VTL0 guests can use the KCS interface and forward System Event Log records to the host.

Changes

• Implement a bounded IPMI KCS/SEL device with save/restore support.
• Expose the x64 port I/O registers at 0xCA2/0xCA3 and the ARM64 MMIO registers at 0xEFFE7000/0xEFFE7004.
• Gate the UEFI-only device through the EnableIpmi device-platform setting and include the setting in attestation and UEFI configuration.
• Forward completed SEL records through GET as rate-limited, fire-and-forget notifications using the 22-byte host wire contract.
• Add architecture-specific chipset resources, device resolution, manifest wiring, and protocol coverage.

Validation

• Windows IPMIDRV starts and exposes an active Microsoft_IPMI instance.
• Add SEL succeeds and the completed record reaches the host IPMI ETW provider.

[Copilot]

🔵 Needs a closer look

It introduces a new chipset device and expands a guest↔host protocol surface area, so it warrants careful human review of contract compatibility and failure-handling paths.

Pull request overview

This PR adds a minimal virtual IPMI BMC to OpenHCL, exposing a KCS interface to VTL0 guests and forwarding completed SEL records to the host over GET. It threads a new EnableIpmi device-platform setting through UEFI config and attestation claims, and wires up architecture-specific chipset resources and resolvers.

Changes:

• Introduce a new ipmi_kcs chipset device crate implementing KCS + bounded SEL with save/restore and rate-limited host forwarding.
• Extend GET protocol and transport/device plumbing to send “fire-and-forget” IPMI SEL host notifications.
• Add a new device-platform setting (EnableIpmi) and wire it through UEFI flags, attestation, and VM manifest/chipset construction.

File summaries

File	Description
vmm_core/vm_manifest_builder/src/lib.rs	Adds manifest-builder opt-in and resource handle wiring for the IPMI KCS device.
vm/loader/src/uefi/config.rs	Adds ipmi_enabled to UEFI config flags and tests its bit position.
vm/devices/get/guest_emulation_transport/src/resolver.rs	Adds a GET-backed resolver for the IPMI SEL event sink.
vm/devices/get/guest_emulation_transport/src/process_loop.rs	Adds a new host notification message path for IPMI SEL.
vm/devices/get/guest_emulation_transport/src/lib.rs	Updates DPS parsing tests and adds a GET notification test for IPMI SEL.
vm/devices/get/guest_emulation_transport/src/client.rs	Adds client API to enqueue IPMI SEL host notifications.
vm/devices/get/guest_emulation_transport/src/api.rs	Adds an exported IpmiSelRecord type and platform setting field.
vm/devices/get/guest_emulation_device/src/test_utilities.rs	Extends test harness to decode/store IPMI SEL notifications.
vm/devices/get/guest_emulation_device/src/lib.rs	Adds parsing/acceptance of the new IPMI SEL host notification.
vm/devices/get/get_protocol/src/lib.rs	Defines the IPMI_SEL host notification ID and its 22-byte wire struct.
vm/devices/get/get_protocol/src/dps_json.rs	Adds EnableIpmi JSON setting parsing and tests.
vm/devices/chipset/ipmi_kcs/src/tests.rs	Adds comprehensive unit tests for KCS/SEL behavior, forwarding, and save/restore.
vm/devices/chipset/ipmi_kcs/src/sel.rs	Implements bounded SEL storage, timestamps, and forwarding rate limiting.
vm/devices/chipset/ipmi_kcs/src/save_restore.rs	Implements validated save/restore state for the device.
vm/devices/chipset/ipmi_kcs/src/resolver.rs	Adds resource resolution for time source + SEL sink, producing chipset devices.
vm/devices/chipset/ipmi_kcs/src/protocol.rs	Implements minimal IPMI command handling and completion staging.
vm/devices/chipset/ipmi_kcs/src/lib.rs	Adds the core KCS state machine + public device API/types.
vm/devices/chipset/ipmi_kcs/src/device.rs	Implements AMD64 PIO and ARM64 MMIO intercept adapters, plus inspect + save/restore.
vm/devices/chipset/ipmi_kcs/Cargo.toml	New crate definition and dependencies.
vm/devices/chipset_resources/src/lib.rs	Adds shared IPMI SEL sink traits/types and IPMI KCS resource handles/addresses.
openhcl/underhill_core/src/worker.rs	Validates UEFI-only gating; wires GET resolver and chipset opt-in based on DPS.
openhcl/underhill_core/src/loader/mod.rs	Emits the new ipmi_enabled bit into UEFI config.
openhcl/underhill_attestation/src/lib.rs	Threads ipmi_enabled into attestation config structures/tests.
openhcl/underhill_attestation/src/igvm_attest/mod.rs	Updates attestation serialization expectations for ipmi-enabled.
openhcl/underhill_attestation/src/hardware_key_sealing.rs	Updates tests/inputs to include the new field.
openhcl/openvmm_hcl_resources/src/lib.rs	Registers the new IpmiKcsResolver.
openhcl/openvmm_hcl_resources/Cargo.toml	Adds ipmi_kcs dependency.
openhcl/openhcl_attestation_protocol/src/igvm_attest/get.rs	Adds ipmi_enabled to runtime claims with serde defaults.
Cargo.toml	Adds workspace dependency entry for the new ipmi_kcs crate.
Cargo.lock	Locks the new crate into the workspace graph.

Review details

• Files reviewed: 29/30 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🔵 Needs a closer look

The GET-backed SEL event sink currently reports forwarded notifications as accepted even when the underlying mesh send can silently drop messages, which can break SEL forwarding accounting/semantics.

Review details

Suppressed comments (1)

vm/devices/get/guest_emulation_transport/src/resolver.rs:54

• SelEventSink::try_send always returns Accepted even though GuestEmulationTransportClient::ipmi_sel ultimately uses mesh::Sender::send, which can silently drop messages when the receiver is closed. This can cause the IPMI KCS device to over-report forwarded SEL records and under-report drops. Consider having ipmi_sel (or ProcessLoopControl::notify) return whether the notification was actually queued (e.g., using Sender::is_closed()), and map that to SelEventDisposition::Dropped here.

        self.0.ipmi_sel(record_id, record);
        SelEventDisposition::Accepted
    }

• Files reviewed: 29/30 changed files
• Comments generated: 0 new
• Review effort level: Lite

[github-actions]

4 Petri tests failed (1 unstable)

[Chris Oo (chris-oo)]

It should be straightforward to add a vmm_test for this for both linux and windows, could you do that as part of this PR?

[Copilot]

🟡 Changes recommended

Multiple #[derive(MeshPayload)] structs had new fields inserted mid-struct, which changes derived field numbering and risks breaking compatibility with mixed-version binaries.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 38/39 changed files
• Comments generated: 4
• Review effort level: Lite

[github-actions]

4 Petri tests failed (2 unstable)

[Chris Oo (chris-oo)]

some initial feedback.

do you also plan on adding openvmm support as a follow up?

[Copilot]

🔵 Needs a closer look

It introduces a new guest-facing emulated device and extends host notification/attestation plumbing across multiple subsystems, warranting careful human review beyond minor findings.

Review details

• Files reviewed: 42/43 changed files
• Comments generated: 1
• Review effort level: Lite

[github-actions]

9 Petri tests failed (1 unstable)

[Copilot]

🟡 Changes recommended

Critical and moderate review findings remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (5)

Previously missed (1) — in code that hasn't changed since the last review.

vm/devices/chipset/ipmi_kcs/Cargo.toml:5

• ipmi_kcs introduces a new guest-visible chipset device and architecture-specific registers, but no Guide page or code-to-Guide mapping is added. Please add the corresponding device/OpenHCL reference, including UEFI-only enablement and SEL forwarding, or explicitly track the required follow-up so the documented device surface stays discoverable.

petri/src/vm/mod.rs:1553

• with_ipmi is documented for OpenHCL UEFI but accepts OpenHCL Linux-direct and non-OpenHCL firmware as well. Enabling it for OpenHCL Linux-direct reaches Underhill's runtime IPMI KCS is only supported with UEFI firmware error, while other firmware silently has no device. Validate enabled IPMI against Firmware::OpenhclUefi here, as other firmware-specific builder options do.

    pub fn with_ipmi(mut self, enable: bool) -> Self {
        self.config.ipmi_enabled = enable;
        self

vm/devices/chipset/ipmi_kcs/src/lib.rs:4

• This PR adds a user-visible OpenHCL emulated device with guest-facing PIO/MMIO addresses, an EnableIpmi setting, and SEL forwarding, but there is no corresponding Guide page or SUMMARY/doc-code mapping entry. Please add the emulated-device reference so the new configuration and interface are discoverable and kept in sync.

//! A minimal virtual IPMI BMC with a byte-oriented KCS interface.

vm/devices/get/get_resources/src/lib.rs:85

• These fields are inserted into the middle of GuestEmulationDeviceHandle, which derives MeshPayload. Mesh field numbers come from struct order (support/mesh/mesh_derive/src/lib.rs:51-54), so older/newer components will decode every subsequent field incorrectly; append new fields after smbios or use an explicitly versioned encoding instead.
  vm/devices/get/guest_emulation_transport/src/resolver.rs:49
• SelEventSink exposes Dropped so forwarding statistics can distinguish records that were not accepted, but this resolver always returns Accepted. GuestEmulationTransportClient::ipmi_sel uses a fire-and-forget sender that can silently discard messages when the GET process loop/channel is closed, so transport shutdown will count lost SEL records as forwarded. Make enqueue failure observable or only return Accepted when the record is actually queued.

• Files reviewed: 43/44 changed files
• Comments generated: 3
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

It introduces a real compatibility risk by inserting new fields into MeshPayload structs (wire encoding changes), and includes at least one correctness bug (timestamp conversion wraps on overflow) that should be fixed before approval.

Review details

Suppressed comments (4)

Previously missed (1) — in code that hasn't changed since the last review.

vm/devices/chipset/ipmi_kcs/src/sel.rs:333

• adjusted_timestamp casts i64 to u32 with as, which will wrap on values > u32::MAX (e.g. far-future host clocks) and produce incorrect SEL timestamps. Since the wire format is 32-bit, clamp the adjusted timestamp into [0, u32::MAX] instead of wrapping.

openhcl/openhcl_attestation_protocol/src/igvm_attest/get.rs:496

• AttestationVmConfig derives MeshPayload, whose derive documentation notes that field numbers come from struct order and inserting/removing fields breaks existing binaries (support/mesh/mesh_derive/src/lib.rs:51-56). Adding ipmi_enabled in the middle of the struct will change the wire encoding for all subsequent fields and risks mixed-version breakage; new fields should be appended at the end (or the type should move to an explicitly-tagged encoding like Protobuf if version tolerance is required).

        pub console_enabled: bool,
        /// Whether the serial console, if enabled, is interactive
        pub interactive_console_enabled: bool,
        /// Whether the IPMI KCS interface is enabled
        pub ipmi_enabled: bool,

vm/devices/get/get_resources/src/lib.rs:86

• GuestEmulationDeviceHandle derives MeshPayload, whose derive documentation notes that field numbers come from struct order and inserting/removing fields breaks existing binaries (support/mesh/mesh_derive/src/lib.rs:51-56). Inserting ipmi_sel_event_send / enable_ipmi here will shift the encoding of later fields and can break mixed-version host/guest or tooling; append new fields at the end of the struct (or migrate the handle to Protobuf if it must be version-tolerant).
  vm/devices/chipset/ipmi_kcs/src/protocol.rs:29
• process_ipmi_message copies the entire fixed-size [u8; KCS_MESSAGE_MAX] request buffer into a local variable, even though only request_len bytes are meaningful. This adds avoidable work on every IPMI message; you can copy only the populated prefix into a local buffer (still satisfying the borrow checker) before parsing.

    pub(crate) fn process_ipmi_message(&mut self) {
        // Keep a local copy so the command handlers can mutably borrow the device.
        let request = self.transaction.request;
        let request = &request[..self.transaction.request_len];
        let Ok((header, data)) = MessageHeader::read_from_prefix(request) else {

• Files reviewed: 44/45 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

There are confirmed wire-compatibility risks from inserting new fields into existing MeshPayload structs (field-numbering-by-order), which can break mixed-version components.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (4)

vm/devices/get/get_resources/src/lib.rs:86

• GuestEmulationDeviceHandle derives MeshPayload, whose derive docs warn that field numbers are implicitly assigned from struct field order; inserting new fields here (ipmi_sel_event_send, enable_ipmi) shifts the encoding of all subsequent fields and breaks mixed-version components. Move new fields to the end of the struct (or switch this handle to an explicitly tagged encoding like Protobuf) to keep existing field numbers stable.
  openhcl/openhcl_attestation_protocol/src/igvm_attest/get.rs:496
• AttestationVmConfig derives MeshPayload and its field numbers are derived from struct order; inserting ipmi_enabled here changes the encoding for subsequent fields and can break mixed-version binaries. Append new fields at the end of the struct (or migrate the type to an explicitly tagged encoding) to preserve wire compatibility.

        pub console_enabled: bool,
        /// Whether the serial console, if enabled, is interactive
        pub interactive_console_enabled: bool,
        /// Whether the IPMI KCS interface is enabled
        pub ipmi_enabled: bool,

vm/devices/chipset/ipmi_kcs/src/protocol.rs:28

• process_ipmi_message copies the entire fixed-size request buffer ([u8; KCS_MESSAGE_MAX]) into a local variable before slicing it, adding an unnecessary 64-byte copy on every IPMI message. You can borrow the existing buffer directly instead.

        // Keep a local copy so the command handlers can mutably borrow the device.
        let request = self.transaction.request;
        let request = &request[..self.transaction.request_len];

vm/devices/chipset/ipmi_kcs/src/sel.rs:238

• ClearSelRequest.reservation_id is currently ignored; if reservation commands are advertised/used, callers may expect mismatched reservations to fail with COMPLETION_RESERVATION_CANCELED (0xC5). Validating the reservation here would align behavior with the reservation support bit and the defined completion code.

    fn clear_sel(&mut self, data: &[u8], out: &mut [u8; KCS_MESSAGE_MAX]) -> usize {
        let Ok((request, _)) = ClearSelRequest::read_from_prefix(data) else {
            return invalid_length(out);
        };

• Files reviewed: 44/45 changed files
• Comments generated: 1
• Review effort level: Lite

[github-actions]

6 Petri tests failed (1 unstable)

[Steven Malis (smalis-msft)]

LGTM once all copilot comments are addressed too

[Copilot]

🔵 Needs a closer look

It introduces backward-incompatible field reordering in existing MeshPayload structs (e.g., GuestEmulationDeviceHandle, AttestationVmConfig) which can break mixed-version mesh communication.

Review details

Suppressed comments (4)

Previously missed (1) — in code that hasn't changed since the last review.

vmm_core/vm_manifest_builder/src/lib.rs:397

• with_ipmi_kcs panics on non-UEFI chipset types via assert!, but the doc comment currently only says "supported only". Documenting the panic (consistent with with_uefi) would make the API contract clearer for callers.

vm/devices/get/get_resources/src/lib.rs:86

• GuestEmulationDeviceHandle derives MeshPayload, where field numbers are implicit and depend on struct field order. Inserting ipmi_sel_event_send here changes the wire encoding for all following fields, which can break mixed-version components communicating over mesh.
  vm/devices/get/get_resources/src/lib.rs:94
• GuestEmulationDeviceHandle is MeshPayload, so adding enable_ipmi in the middle of the struct changes the on-the-wire field numbering for later fields. If this handle is used across version boundaries, this is a breaking change.
  openhcl/openhcl_attestation_protocol/src/igvm_attest/get.rs:497
• AttestationVmConfig derives MeshPayload; inserting ipmi_enabled here changes the field numbering for secure_boot, tpm_enabled, etc. That can break mesh compatibility with older binaries expecting the previous encoding. Consider appending new fields at the end (or switching to an explicitly-tagged encoding) to preserve backward compatibility.

        /// Whether the serial console is enabled
        pub console_enabled: bool,
        /// Whether the serial console, if enabled, is interactive
        pub interactive_console_enabled: bool,
        /// Whether the IPMI KCS interface is enabled
        pub ipmi_enabled: bool,
        /// Whether secure boot is enabled

• Files reviewed: 47/48 changed files
• Comments generated: 0 new
• Review effort level: Lite

[github-actions]

2 Petri tests failed (0 unstable)