openhcl: add IPMI KCS emulation and SEL forwarding by ayusharora221204 · Pull Request #4396 · microsoft/openvmm

ayusharora221204 · 2026-09-08T06:56:36Z

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.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: dda9bd55-db87-46d6-a512-15d8a769b9a3

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.

+impl SelEventSink for GetIpmiSelEventSink {
+    fn try_send(
+        &mut self,
+        record_id: u16,
+        record: [u8; IPMI_SEL_RECORD_SIZE],
+    ) -> SelEventDisposition {
+        self.0.ipmi_sel(record_id, record);
+        SelEventDisposition::Accepted
+    }


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 · 2026-09-08T10:43:12Z

4 Petri tests failed (1 unstable)

Chris Oo (chris-oo) · 2026-09-08T18:25:13Z

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?

Enable IPMI in OpenVMM-hosted OpenHCL tests and observe host SEL notifications. Add Linux and Windows guest coverage for the KCS Add SEL path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: dda9bd55-db87-46d6-a512-15d8a769b9a3

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

        /// Whether the serial console, if enabled, is interactive
        pub interactive_console_enabled: bool,
+        /// Whether the IPMI KCS interface is enabled
+        #[serde(default, skip_serializing_if = "is_false")]
+        pub ipmi_enabled: bool,


        /// Notification of firmware events.
        pub firmware_event_send: Option<mesh::Sender<FirmwareEvent>>,
+        /// Test observer for IPMI SEL notifications received from OpenHCL.
+        pub ipmi_sel_event_send: Option<mesh::Sender<IpmiSelEvent>>,
        /// Enable secure boot.


        /// Enable battery.
        pub enable_battery: bool,
+        /// Enable the IPMI KCS interface.
+        pub enable_ipmi: bool,
        /// Suppress attestation and disable TPM state persistence.


+    /// An IPMI SEL notification received from OpenHCL.
+    #[derive(Debug, Clone, Copy, MeshPayload, PartialEq, Eq)]
+    pub struct IpmiSelEvent {
+        /// BMC-assigned SEL record identifier.
+        pub record_id: u16,
+        /// Completed SEL record.
+        pub record: [u8; 16],
+    }


github-actions · 2026-09-09T11:40:53Z

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?

Chris Oo (chris-oo) · 2026-09-09T21:35:21Z

        Signer,
    }

+    fn is_false(value: &bool) -> bool {


Chris Oo (chris-oo) · 2026-09-09T21:36:06Z

        /// Whether the serial console, if enabled, is interactive
        pub interactive_console_enabled: bool,
+        /// Whether the IPMI KCS interface is enabled
+        #[serde(default, skip_serializing_if = "is_false")]


is this really the right thing to do for runtime claims? shouldn't we always include it whether its' true or false, like the other things?

The reason for is_false and skip_serializing_if was to preserve the previous serialized VM configuration when IPMI is disabled. I was concerned because this JSON is also used as input to hardware-key derivation, so even adding "ipmi-enabled": false changes that input for existing VMs. However, I agree that the runtime-claims contract should be consistent with the other Boolean fields and explicitly include both true and false. I think it is reasonable to follow the existing pattern here.

Chris Oo (chris-oo) · 2026-09-09T21:36:58Z

    );

-    // Read measured config from VTL0 memory. When restoring, it is already gone.
-    let (firmware_type, mut measured_vtl0_info, load_kind) = {


where did this go?

The measured-VTL0 block was not deleted, it moved earlier. We need firmware_type earlier to reject IPMI on PCAT/Linux-direct before generating runtime claims or deriving hardware keys.

Chris Oo (chris-oo) · 2026-09-09T21:38:16Z

+use sel::RateLimiter;
+use sel::SelState;
+
+/// Maximum KCS request or response size.


are these spec defns for the ipmi device? probably should be in some kind of spec subcrate?

Agreed. These are device specification definitions. I’ll separate those into an IPMI spec/protocol crate.

Chris Oo (chris-oo) · 2026-09-09T21:39:03Z

+
+    /// Creates a virtual BMC with a best-effort SEL event sink.
+    pub fn with_event_sink(
+        clock: impl TrustedClock + 'static,


hm is it better to have the caller give us a box here, rather than us boxing it ourselves?

Yes, that fits. The caller can give us an explicit box here.

Chris Oo (chris-oo) · 2026-09-09T21:40:15Z

@@ -0,0 +1,108 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT License.
+


missing module comments (applies to all files missing them)

Ohh yeah, I’ll add module-level documentation to the IPMI KCS source files describing each module’s responsibility.

Chris Oo (chris-oo) · 2026-09-09T21:42:16Z

+        }
+    }
+
+    fn get_sel_info(&mut self, out: &mut [u8; KCS_MESSAGE_MAX]) -> usize {


a lot of these functions here do this manual byte munging/editing, is there no way to use zerocopy here with header/struct definitions to make it more obvious what we're writing to the buffer?

Agreed. Fixed-format requests, responses and response headers can use zerocopy structures. But Get SEL Entry has a runtime length record payload determined by the requested offset and byte count, so I think only that trailing payload will retain a small bounded slice copy.

Chris Oo (chris-oo) · 2026-09-09T21:43:04Z

    use super::*;

    #[test]
-    fn smoke_test_sample() {


was it intentional this test was removed?

The sample parsing was not intentionally removed, the test was renamed and extended to verify that the existing sample defaults IPMI to false. I can restore smoke_test_sample and add a separate IPMI default/parsing test.

Chris Oo (chris-oo) · 2026-09-09T21:45:55Z

    type Input<'a> = ();
 }

+/// The size of an IPMI System Event Log record.


is this really the right places for these defns?

The resource handle belongs in chipset_resources because it follows the existing device-resource pattern. I can move the IPMI-specific sink definitions into the existing chipset_resources::ipmi_kcs module for clearer organization. The SEL record-size constant is a protocol definition and can move to the proposed IPMI protocol crate.

Chris Oo (chris-oo) · 2026-09-09T21:46:09Z

+
+/// Result of attempting to forward an IPMI SEL record.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum SelEventDisposition {


hm not really a fan of this name, seems long/wordy?

umm...yeah, maybe it can be changed to SendOutcome

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: dda9bd55-db87-46d6-a512-15d8a769b9a3

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

+        let request = self.transaction.request;
+        let request = &request[..self.transaction.request_len];


github-actions · 2026-09-10T08:35:42Z

9 Petri tests failed (1 unstable)

Ayush Arora and others added 2 commits September 7, 2026 16:00

add IPMI KCS and SEL core

6675144

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: dda9bd55-db87-46d6-a512-15d8a769b9a3

openhcl: wire IPMI KCS and SEL forwarding

6a74794

Copilot AI lite review requested due to automatic review settings September 8, 2026 06:56

ayusharora221204 requested a review from a team as a code owner September 8, 2026 06:56

Copilot started reviewing on behalf of ayusharora221204 September 8, 2026 06:57 View session

Copilot AI reviewed Sep 8, 2026

View reviewed changes

Merge upstream main into IPMI KCS branch

366279f

Copilot AI review requested due to automatic review settings September 8, 2026 09:14

Copilot started reviewing on behalf of ayusharora221204 September 8, 2026 09:15 View session

Copilot AI reviewed Sep 8, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings September 9, 2026 10:10

Copilot started reviewing on behalf of ayusharora221204 September 9, 2026 10:10 View session

Copilot AI reviewed Sep 9, 2026

View reviewed changes

Chris Oo (chris-oo) reviewed Sep 9, 2026

View reviewed changes

refactor: address IPMI review feedback

bbd2123

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: dda9bd55-db87-46d6-a512-15d8a769b9a3

Copilot AI review requested due to automatic review settings September 10, 2026 07:02

Copilot started reviewing on behalf of ayusharora221204 September 10, 2026 07:02 View session

Copilot AI reviewed Sep 10, 2026

View reviewed changes

Comment thread vm/devices/chipset/ipmi_kcs/src/protocol.rs

Comment on lines +26 to +27

let request = self.transaction.request;

let request = &request[..self.transaction.request_len];

ayusharora221204 requested a review from Chris Oo (chris-oo) September 10, 2026 11:06

		@@ -0,0 +1,108 @@
		// Copyright (c) Microsoft Corporation.
		// Licensed under the MIT License.

		let request = self.transaction.request;
		let request = &request[..self.transaction.request_len];

Uh oh!

Conversation

ayusharora221204 commented Sep 8, 2026

Overview

Changes

Validation

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Suppressed comments (1)

Uh oh!

github-actions Bot commented Sep 8, 2026

Uh oh!

Chris Oo (chris-oo) commented Sep 8, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

github-actions Bot commented Sep 9, 2026

Uh oh!

Chris Oo (chris-oo) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ayusharora221204 Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

github-actions Bot commented Sep 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

ayusharora221204 Sep 10, 2026 •

edited

Loading