feat(opentmk): opentmk framework with first testcase #1210

mayank-microsoft · 2025-04-21T18:48:23Z

No description provided.

feat: opentmk init feat: opentmk init feat: opentmk init feat: opentmk init feat: opentmk init feat: opentmk init feat: init 1 feat: init 2 feat: init 1 feat: opentmk feat: opentmk init 3 feat: opentmk init 4 feat: opentmk init 4

…ll the registers

Cargo.toml

smalis-msft · 2025-04-22T15:27:15Z

xtask/src/tasks/guest_test/uefi/gpt_efi_disk.rs

@@ -22,7 +22,7 @@ pub fn create_gpt_efi_disk(out_img: &Path, with_files: &[(&Path, &Path)]) -> Res
        ));
    }

-    let disk_size = 1024 * 1024 * 32; // 32MB disk should be enough for our tests
+    let disk_size = 1024 * 1024 * 512; // 32MB disk should be enough for our tests


Why the increase?

the image size of the compiled application goes beyond 300MB, So we had to increase the disk size.

Should probably make it a parameter then, so that guest-test-uefi doesn't also get bigger.

This is still open.

opentmk/README.md

opentmk/build_deploy.sh

smalis-msft · 2025-04-22T15:29:41Z

opentmk/Cargo.toml

+edition.workspace = true
+rust-version.workspace = true
+
+[dependencies]


nit: sort and switch to dotted syntax for crates that don't need features. Also move all crates to be workspaced.

@smalis-msft There is one issue where I want to use the serde package with default features off as I want to use a no_std env. Should I disable the feature in workspace?

Yes, you'll have to.

opentmk/src/main.rs

opentmk/src/slog.rs

opentmk/src/sync.rs

opentmk/src/slog.rs

opentmk/src/uefi/init.rs

opentmk/src/uefi/hypvctx.rs

smalis-msft · 2025-04-22T15:48:38Z

opentmk/src/uefi/alloc.rs

+    pub unsafe fn init(&self, size: usize) -> bool {
+        let pages = ((SIZE_1MB * size) / 4096) + 1;
+        let size = pages * 4096;
+        let mem: Result<core::ptr::NonNull<u8>, uefi::Error> = boot::allocate_pages(AllocateType::AnyPages, MemoryType::BOOT_SERVICES_DATA, pages);


Why the two different allocators?

this is there to handler cases where we need to do the allocation before we exit boot services. After exit boot services we cant depend on the uefi allocator and we need to switch.

I see. Is there some way we can make this cleaner? Like funadamentally splitting up these two allocators and switching between them at a higher level?

These are fundamentally 2 different allocators, but they switch based on a flag.

opentmk/src/uefi/tests/hv_processor.rs

smalis-msft · 2025-04-22T15:52:35Z

opentmk/src/uefi/tests/mod.rs

+pub mod hv_processor;
+pub mod hv_misc;
+
+pub fn run_test() {


I think what we'll want here is some sort of type definition for a Test that has a string name and other niceties. Then we can have an array of Tests and decide what tests to run and how at runtime.

Right the v1 plan is to have a config file which we can read using uefi APIs. The config can define the test to run.

opentmk/src/uefi/tests/hv_processor.rs

opentmk/src/uefi/hypvctx.rs

sluck-msft · 2025-04-25T22:04:14Z

opentmk/src/arch/aarch64/hypercall.rs

+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT License.
+
+/// Writes a synthehtic register to tell the hypervisor the OS ID for the boot shim.


Suggested change

/// Writes a synthehtic register to tell the hypervisor the OS ID for the boot shim.

/// Writes a synthetic register to tell the hypervisor the OS ID for the boot shim.

sluck-msft · 2025-04-25T22:07:06Z

opentmk/src/slog.rs

@@ -0,0 +1,242 @@
+#![feature(panic_location)]


Does "slog" refer to serial log? It's sort of an unfortunate file name.

Yeah. I am working on it. Steven suggested to abstract the module into two layers on how we log and the structure of the log.

sluck-msft · 2025-04-25T22:31:37Z

opentmk/src/uefi/hypvctx.rs

+
+const ALIGNMENT: usize = 4096;
+
+type ComandTable =


Suggested change

type ComandTable =

type CommandTable =

sluck-msft · 2025-04-25T23:14:01Z

opentmk/src/uefi/context.rs

+pub trait TestCtxTrait {
+    fn get_vp_count(&self) -> u32;
+    fn get_current_vp(&self) -> u32;
+    fn get_current_vtl(&self) -> Vtl;


Is this (and similar methods, like switching VTLs) for the current VP?

Similar questions for get_register, etc

yes. these APIs are for current VP. (I'll add more documentation to explicitly call that out)

sluck-msft · 2025-04-25T23:14:27Z

opentmk/src/uefi/context.rs

+
+    fn setup_partition_vtl(&mut self, vtl: Vtl);
+    fn setup_interrupt_handler(&mut self);
+    fn set_interupt_idx(&mut self, interrupt_idx: u8, handler: fn());


Suggested change

fn set_interupt_idx(&mut self, interrupt_idx: u8, handler: fn());

fn set_interrupt_idx(&mut self, interrupt_idx: u8, handler: fn());

sluck-msft · 2025-04-26T00:27:09Z

opentmk/src/uefi/hypvctx.rs

+            .get_mut(&vp_index)
+            .unwrap()
+            .push_back((cmd, vtl));
+        if vp_index == self.my_vp_idx && self.hvcall.vtl != vtl {


Does vtl refer to the VTL you want the current VP to be on, or the VTL you want the target vp to run the command on?

This VTL switch is for a case where we schedule a cmd on the current VP. The current context will switch to the target VTL on current VP and execute the command. The command is expected to return to initial VTL by calling HvCall/HvReturn.

sluck-msft · 2025-04-26T00:31:05Z

opentmk/src/uefi/hypercall.rs

+
+        let header = hvdef::hypercall::ModifyVtlProtectionMask {
+            partition_id: hvdef::HV_PARTITION_ID_SELF,
+            map_flags: hvdef::HV_MAP_GPA_PERMISSIONS_NONE,


You could make this an input parameter? And then other functions like apply_vtl2_protections could build on top of it.

Makes sense, I'll work o abstracting this more.

sluck-msft · 2025-04-26T00:37:50Z

opentmk/src/arch/x86_64/interrupt_handler_register.rs

+extern "x86-interrupt" fn no_op(stack_frame: InterruptStackFrame) {}
+
+pub fn register_interrupt_handler(idt: &mut InterruptDescriptorTable) {
+    register_interrupt_handler!(idt, 0, handler_0);


Some of these are architectural (and might also be in x86 defs), it might be nice to include the actual name for them.

sluck-msft · 2025-04-26T00:41:08Z

opentmk/src/uefi/tests/hv_processor.rs

+                    let vtl = ctx.get_current_vtl();
+                    infolog!("vtl: {:?}", vtl);
+                    tmk_assert!(vtl == Vtl::Vtl1, format!("vtl should be Vtl0 for VP {}", i));
+                    tx.send(());


Eventually would be nice to check that the register state is what we expect, but that might be difficult if execution keeps going.

Which registers would we want to assert? We can probably think of how we can do it.

A lot of them, eventually. We need to make sure that we correctly get this shared/private split correct, to the extent that we can:

https://learn.microsoft.com/en-us/virtualization/hyper-v-on-windows/tlfs/vsm#virtual-processor-state-isolation

sluck-msft · 2025-04-26T00:47:13Z

opentmk/src/uefi/tests/hv_misc.rs

+            val != 0xAA,
+            "heap memory should not be accessible from vtl0"


Seems like we could eventually use the interrupt handler that you set on 0x30 to check that we did get a memory intercept?

(Pretty cool to all this in action though :))

smalis-msft

Apologies if I'm repeating something I said in the previous review, there's a lot of code here and it's been a little while.

Also, you'll need to run cargo xtask fmt and have it succeed on this code before it can be merged.

That all said, this is shaping up really really nicely. Please don't let the large number of comments discourage you, I'm really happy with how this is shaping up.

smalis-msft · 2025-05-27T16:33:06Z

Cargo.toml

@@ -42,6 +42,9 @@ members = [
  "vm/loader/igvmfilegen",
  "vm/vmgs/vmgs_lib",
  "vm/vmgs/vmgstool",
+  # opentmk
+  "opentmk/opentmk",
+  "opentmk/sync"


If something is a dependency below it shouldn't be a member, members only needs to be top-level crates.

smalis-msft · 2025-05-27T16:34:07Z

Cargo.toml

@@ -520,6 +529,7 @@ winapi = "0.3"
 windows = "0.59"
 windows-service = "0.7"
 windows-sys = "0.52"
+x86_64 = "0.15.2"


How much functionality from x86_64 are we using? How difficult of a lift would it be to fill in things like x86defs and safe_intrinsics, or just call intrinsics directly?

smalis-msft · 2025-05-27T16:35:17Z

Cargo.toml

@@ -445,9 +451,11 @@ jiff = "0.1"
 kvm-bindings = "0.7"
 # Use of these specific REPO will go away when changes are taken upstream.
 landlock = "0.3.1"
+lazy_static = { version = "1.4.0", features = ["spin_no_std"] }


Don't specify features in the workspace-level Cargo.toml, since that applies them to every crate whether they want them or not. Move it into the specific crates' Cargo.tomls that need it.

smalis-msft · 2025-05-27T16:35:51Z

xtask/src/tasks/guest_test/uefi/gpt_efi_disk.rs

@@ -22,7 +22,7 @@ pub fn create_gpt_efi_disk(out_img: &Path, with_files: &[(&Path, &Path)]) -> Res
        ));
    }

-    let disk_size = 1024 * 1024 * 32; // 32MB disk should be enough for our tests
+    let disk_size = 1024 * 1024 * 512; // 32MB disk should be enough for our tests


This is still open.

smalis-msft · 2025-05-27T16:36:58Z

Cargo.lock

+
+[[package]]
+name = "spin"
+version = "0.10.0"


Can we unify on the same version of spin that lazy_static is pulling in, so we only have the one?

smalis-msft · 2025-05-27T17:14:36Z

opentmk/opentmk/src/uefi/alloc.rs

+        unsafe {
+            self.locked_heap.lock().init(ptr, size);
+        }
+        *self.use_locked_heap.lock().borrow_mut() = true;


If we're always setting this to true do we still need the uefi allocator? Can we get away with just using the linked_list_allocator directly in all cases?

If so, is there some way we could make this type be an enum with a variant for each allocator instead of keeping both around forever?

smalis-msft · 2025-05-27T17:14:59Z

opentmk/opentmk/src/uefi/tests/mod.rs

@@ -0,0 +1,13 @@
+#![allow(dead_code)]


nit: get rid of all these allow(dead_code)s everywhere

smalis-msft · 2025-05-27T17:23:16Z

opentmk/opentmk/src/arch/mod.rs

+
+//! Imports and re-exports architecture-specific implementations.
+
+mod x86_64;


Is this not building the arm code currently?

smalis-msft · 2025-05-27T17:23:49Z

opentmk/opentmk/src/tests/mod.rs

+
+pub fn run_test() {
+    let mut ctx = HvTestCtx::new();
+    hv_processor::exec(&mut ctx);


What's the difference between the tests in here and the tests in uefi/ ?

smalis-msft · 2025-05-27T17:24:32Z

opentmk/opentmk/src/arch/x86_64/serial.rs

+///
+/// The caller must be sure that the given port is safe to write to, and that the
+/// given value is safe for it.
+unsafe fn outb(port: u16, data: u8) {


Is this stuff all shared with the boot shim too? If so we should try to move it somewhere shared instead of duplicating.

mayank-microsoft added 4 commits April 21, 2025 18:46

feat: opentmk init

6d32033

feat: opentmk init feat: opentmk init feat: opentmk init feat: opentmk init feat: opentmk init feat: opentmk init feat: init 1 feat: init 2 feat: init 1 feat: opentmk feat: opentmk init 3 feat: opentmk init 4 feat: opentmk init 4

feat: added docs

351199b

refactor(opentmk): interrupt handler doesn't need to save state for a…

3e1673e

…ll the registers

reafctor(opentmk): remove dead code

056bf7d

mayank-microsoft force-pushed the target-opentmk branch from c3f74c0 to 056bf7d Compare April 21, 2025 19:14

mayank-microsoft requested review from smalis-msft and sluck-msft April 21, 2025 19:30