opentmk: acpi shutdown to stop the testvm by mayank-microsoft · Pull Request #3255 · microsoft/openvmm

mayank-microsoft · 2026-04-11T17:29:35Z

No description provided.

github-actions · 2026-04-11T17:29:55Z

⚠️ 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

Pull request overview

Adds an ACPI-based shutdown path to OpenTMK’s UEFI runtime so test VMs can power off cleanly after completion/panic, and introduces a local Flowey pipeline to build/package OpenTMK into a bootable VHD for convenience.

Changes:

Implement ACPI S5 shutdown for OpenTMK UEFI (including FADT parsing + runtime shutdown writes).
Refactor ACPI-related errors into a typed AcpiError carried by TmkError::AcpiError.
Add local-only Flowey plumbing (build-opentmk) to build OpenTMK and package it into a VHD.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
opentmk/src/uefi/rt.rs	Attempt ACPI shutdown on panic instead of spinning forever.
opentmk/src/uefi/mod.rs	Attempt ACPI shutdown after test completion.
opentmk/src/uefi/acpi_wrap.rs	Extend ACPI context to locate FADT and derive a shutdown mechanism.
opentmk/src/tmkdefs.rs	Introduce `AcpiError` and wrap it in `TmkError::AcpiError(AcpiError)`.
opentmk/src/devices/shutdown.rs	New module implementing legacy + HW-reduced ACPI S5 shutdown writes.
opentmk/src/devices/mod.rs	Export the new `shutdown` device module.
opentmk/src/arch/x86_64/mod.rs	Make the `io` module public for reuse by shutdown code.
opentmk/src/arch/x86_64/io.rs	Add word I/O helpers and improve docs for port I/O primitives.
flowey/flowey_lib_hvlite/src/lib.rs	Export the new `build_opentmk` flow node.
flowey/flowey_lib_hvlite/src/build_opentmk.rs	New node to build OpenTMK as a UEFI binary artifact.
flowey/flowey_lib_hvlite/src/_jobs/mod.rs	Register a new local job for building OpenTMK.
flowey/flowey_lib_hvlite/src/_jobs/local_build_opentmk.rs	Local job to package the EFI binary into a fixed VHD.
flowey/flowey_hvlite/src/pipelines/mod.rs	Add a new `build-opentmk` CLI pipeline entrypoint.
flowey/flowey_hvlite/src/pipelines/build_opentmk.rs	Implement the local-only pipeline wiring for `build-opentmk`.

Copilot · 2026-04-11T17:34:22Z

opentmk/src/uefi/acpi_wrap.rs

+        let x_pm1a: GenericAddress =
+            fadt_field!(x_pm1a_cnt_blk, GenericAddress, GenericAddress::default());
+        let x_pm1b: GenericAddress =
+            fadt_field!(x_pm1b_cnt_blk, GenericAddress, GenericAddress::default());
+        let sleep_reg: GenericAddress =


parse_sleep_mechanism reads GenericAddress via read_unaligned into acpi_spec::fadt::GenericAddress, but that type contains Rust enums (AddressSpaceId, AddressWidth). Because the FADT contents come from firmware (untrusted), an invalid discriminant would make this read undefined behavior. Parse these fields as raw bytes (e.g., a local GenericAddressRaw { addr_space_id: u8, ..., access_size: u8, address: u64 }) and only convert to the enum after validating/handling unknown values.

Copilot · 2026-04-11T17:34:22Z

opentmk/src/uefi/acpi_wrap.rs

+        // Prefer extended addresses over legacy.
+        let x_pm1a_addr = x_pm1a.address;
+        let effective_pm1a = if x_pm1a_addr != 0 {
+            x_pm1a_addr as u16
+        } else {
+            pm1a as u16
+        };


x_pm1a.address/pm1a_cnt_blk are truncated to u16 without validating the address space or range. If the FADT provides a SystemMemory GAS or an address > 0xFFFF, this will silently pick the wrong I/O port. Please validate that the chosen PM1* control block is in SystemIO space and fits in u16 (otherwise fall back to the HW-reduced sleep control path or return an ACPI error).

Copilot · 2026-04-11T17:34:22Z

opentmk/src/devices/shutdown.rs

+                let port = reg_addr as u16;
+
+                // Hyper-V workaround: the sleep_control_reg often points one
+                // byte into PM1_CNT (e.g. 0x405 when PM1_CNT is at 0x404).
+                // The hypervisor only handles word-width writes at the aligned
+                // base, so mask off the low bit and do a 16-bit outw.
+                let pm1_cnt_port = port & !1;
+                log::info!(
+                    "ACPI HW-reduced shutdown: writing word 0x{:04x} to PM1_CNT port 0x{:x}",
+                    word_value,
+                    pm1_cnt_port
+                );
+                crate::arch::io::outw(pm1_cnt_port, word_value);


In the SystemIO HW-reduced path, sleep_reg.address is cast to u16 (let port = reg_addr as u16;) without checking it fits. Since the address originates from firmware, validate reg_addr <= u16::MAX (and ideally that the register is actually byte/word addressable) before issuing outw/outb, otherwise the write may hit an unintended port.

Copilot · 2026-04-11T17:34:22Z

opentmk/src/uefi/mod.rs

+    // Attempt a clean ACPI S5 shutdown. On UEFI this never returns --
+    // it either powers off the VM or spins forever internally.
+    // The loop below is unreachable but satisfies the Status return type.
+    let _ = crate::devices::shutdown::shutdown();


The comments say the ACPI shutdown call “never returns” / the following loop is “unreachable”, but shutdown() can return Err (e.g., if FADT parsing fails). Consider either updating the comment to reflect that the loop is reachable on shutdown failure, or explicitly handle/log the error result so failures don’t look like a hang.

Suggested change

// Attempt a clean ACPI S5 shutdown. On UEFI this never returns --

// it either powers off the VM or spins forever internally.

// The loop below is unreachable but satisfies the Status return type.

let _ = crate::devices::shutdown::shutdown();

// Attempt a clean ACPI S5 shutdown. This should normally power off the

// VM. If shutdown fails or returns unexpectedly, log the error and fall

// back to spinning here.

if let Err(err) = crate::devices::shutdown::shutdown() {

log::error!("ACPI shutdown failed: {err:?}");

}

Copilot · 2026-04-11T17:34:23Z

opentmk/src/uefi/rt.rs

+    // Best-effort ACPI shutdown on panic. On UEFI this never returns --
+    // it either powers off the VM or spins forever internally.
+    let _ = crate::devices::shutdown::shutdown();
+    // Unreachable on UEFI, but required for the `-> !` return type.


This comment claims the shutdown attempt “never returns”, but shutdown() can return Err (e.g., if it can’t determine a shutdown mechanism). Please adjust the comment and/or log/handle the error result so a shutdown failure is diagnosable.

Suggested change

// Best-effort ACPI shutdown on panic. On UEFI this never returns --

// it either powers off the VM or spins forever internally.

let _ = crate::devices::shutdown::shutdown();

// Unreachable on UEFI, but required for the `-> !` return type.

// Best-effort ACPI shutdown on panic. If shutdown cannot be performed,

// log the failure and fall back to spinning forever below.

if let Err(err) = crate::devices::shutdown::shutdown() {

log::error!("Failed to shut down after panic: {}", err);

}

// Fallback path if shutdown returns instead of powering off the VM.

mebersol · 2026-04-13T18:25:43Z

opentmk/src/devices/shutdown.rs

+use crate::tmkdefs::TmkError;
+use crate::tmkdefs::TmkResult;
+
+// --- Legacy PM1 constants ---


no chance to use the 'true' defines here?

Copilot AI review requested due to automatic review settings April 11, 2026 17:29

github-actions bot added the unsafe Related to unsafe code label Apr 11, 2026

Copilot started reviewing on behalf of mayank-microsoft April 11, 2026 17:30 View session

mayank-microsoft added 2 commits April 11, 2026 17:31

opentmk: ACPI shutdown

f3afb9c

opentmk: acpi shutdown

83a9c53

mayank-microsoft force-pushed the mayank/acpi-shutdown branch from a4b05d7 to 83a9c53 Compare April 11, 2026 17:32

Copilot AI reviewed Apr 11, 2026

View reviewed changes

mebersol reviewed Apr 13, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

opentmk: acpi shutdown to stop the testvm#3255

opentmk: acpi shutdown to stop the testvm#3255
mayank-microsoft wants to merge 2 commits intomicrosoft:mainfrom
mayank-microsoft:mayank/acpi-shutdown

mayank-microsoft commented Apr 11, 2026

Uh oh!

github-actions bot commented Apr 11, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 11, 2026

Uh oh!

Copilot AI Apr 11, 2026

Uh oh!

Copilot AI Apr 11, 2026

Uh oh!

Copilot AI Apr 11, 2026

Uh oh!

Copilot AI Apr 11, 2026

Uh oh!

mebersol Apr 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

-    // Attempt a clean ACPI S5 shutdown. On UEFI this never returns --
-    // it either powers off the VM or spins forever internally.
-    // The loop below is unreachable but satisfies the Status return type.
-    let _ = crate::devices::shutdown::shutdown();
+    // Attempt a clean ACPI S5 shutdown. This should normally power off the
+    // VM. If shutdown fails or returns unexpectedly, log the error and fall
+    // back to spinning here.
+    if let Err(err) = crate::devices::shutdown::shutdown() {
+        log::error!("ACPI shutdown failed: {err:?}");
+    }

Conversation

mayank-microsoft commented Apr 11, 2026

Uh oh!

github-actions bot commented Apr 11, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

Uh oh!

mebersol Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants