Initial implementation of supervisor by kuqin12 · Pull Request #1385 · OpenDevicePartnership/patina

kuqin12 · 2026-03-11T00:32:33Z

Description

This is the initial implementation of MM supervisor and user core in Rust.

How This Was Tested

This was tested on QEMU Q35 platform and booted to OS desktop as well as passed supervisor test app.

Integration Instructions

The integration guide is listed in: https://github.com/kuqin12/mu_feature_mm_supv/blob/personal/kuqin/supv_init/SeaPkg/Docs/PlatformIntegration/PlatformIntegrationSteps.md#integraion-guide-for-rust-based-supervisor. Because it provides the implementation of MM supervisor init module.

patina-automation · 2026-03-11T00:32:42Z

⌛ QEMU Validation Pending

QEMU validation is pending on successful CI completion.

Note: Any previous results are available in this comment's edit history.

This comment was automatically generated by the Patina QEMU PR Validation workflow.

Javagedes

I'm very concerned about the number of global statics in your implementation. I understand it may actually be necessary to be able to perform our SEA validation on the supervisor.

We need to consolidate the statics if possible. If we cannot (due to SEA validation requirements), then we need to make trait abstractions around each of the different statics so that at runtime they all link up, but for testing you can use mockall to mock the different statics. e.g. something like:

static MY_STATIC: SomeStatic = SomeStatic::new();

trait MyTrait {
    fn interface_fn1();
    fn interface_fn2();
}

impl MyTrait for SomeStatic {
    fn interface_fn1() { MY_STATIC.interface_fn1() }
    fn interface_fn2() { MY_STATIC.interface_fn2() }
}

This will allow us to use mockall with the trait to do proper mocking.

Javagedes · 2026-03-11T16:33:11Z

@@ -0,0 +1,335 @@
+//! Shared MM Pool Allocator


It really feels like we need to pull out some of the re-usable bits from the patina-dxe-core allocation functionality. I understand not all of that. But as much as possible.

The patian-dxe-core allocation may be useful. But the page allocator for DXE is too GCD centric, which is not a thing in MM. I thought about bringing in GCD, but gave up due to complicated logic and excessive TPL usage.

I briefly looked at the pool allocator from dxe environment. Admittedly, I was scared off by its boot services named statics... I probably need to revisit it.

This is fair. The GCD is very complex, especially if you don't need it.

You could still possibly use some fundamentals like the RBT / BST.

There are also some allocator crates you could use depending on your needs. Just hoping you don't need to completely re-invent the wheel :).

Yeah, I understand :) The original thought is that we will need to have something to get other functionalities going. I will create an issue on this topic so that we do not lose it.

Javagedes · 2026-03-11T16:42:54Z

+    ///
+    /// Protected by `lock`. The raw pointer is `!Send` but the outer struct
+    /// provides `Send + Sync` via the lock.
+    head: Mutex<*mut PoolBlockHeader>,


I don't have a great answer here as I'm just doing a skim / pass through this code, but I really think we need a better abstraction around the head here and all of the pointer work you are doing. Not necessarily for this PR but as a task.

Javagedes · 2026-03-11T18:14:52Z

@@ -0,0 +1,92 @@
+//! Protocol/Handle Database


we do not want to carry two implementations of the protocol db. We need to abstract or allow configuration enough that the existing protocol db implementation meets your needs.

:) that's what I thought as well. then the tpl based lock got in the way and I took the shorter route. Like the allocator comment, we will do that once this stabilizes.

Javagedes · 2026-03-11T18:16:22Z

@@ -0,0 +1,378 @@
+//! MM Driver Dispatcher


We should not implement a whole new dispatcher. We should abstract and allow configurations enough that we can re-use the existing implementation.

Javagedes · 2026-03-11T18:17:56Z

Use the newer crate layout as mentioned earlier.

Javagedes · 2026-03-11T18:19:47Z

@@ -0,0 +1,397 @@
+//! SMRAM Save State Architecture Definitions


I don't see a service here. Are you sure this is the right location?

Javagedes · 2026-03-11T18:20:13Z

@@ -0,0 +1,135 @@
+//! Arch-specific timer functionality


This already exists does it not? I'm confused.

This does, but it was carried inside the dxe specific module. I am trying to get this into a common place. Any suggestions on where that should be? Probably not a component?

Will create a separate PR for this for better readability.

kuqin12 · 2026-03-17T04:24:26Z

+#![cfg(target_arch = "x86_64")]
+#![feature(coverage_attribute)]
+
+#![allow(incomplete_features)]


This is for the generic_const_exprs so that the statics will be materialized based on the number of CPUs.

kuqin12 · 2026-03-17T04:26:29Z

+    /// 
+    /// On the first call (initialization phase), this function returns after init is complete.
+    /// On subsequent calls, BSP enters the request loop and APs enter the holding pen (neither returns).
+    pub fn entry_point(&'static self, cpu_index: usize, hob_list: *const c_void) {


Agreed. This is a fully loaded monolithic flow. Will need to break up based on functionality here.

kuqin12 · 2026-03-17T04:28:51Z

+                .ok_or(PolicyInitError::NullHobList)?
+        };
+
+        let mut supv_comm_buffer = 0 as u64;


Would you have better suggestions here? It is meant to collect the values and should be validated once they are all collected. The validation may not be there yet, but that is in the plan.

kuqin12 · 2026-03-17T04:38:36Z

+        }
+
+        // Store communication buffer configuration.
+        COMM_BUFFER_CONFIG.call_once(|| CommBufferConfig {


This is a one-time initialization by taking the prepared buffer from HOB value. Then following MMIs will consume them rather than always going through the preparation. So in that sense I do want to cache these values, and once only. Any suggestion on how to avoid statics here?

kuqin12 · 2026-03-17T04:44:18Z

+
+/// A single AP's mailbox for communication with the BSP.
+#[repr(C, align(64))] // Cache-line aligned to avoid false sharing
+pub struct ApMailbox {


C side has much more complicated data structure to host these functionalities. But I cut the functionalities some and only kept the atomic part. But I agree, this should not be structured to be C like layout.

kuqin12 · 2026-03-17T04:57:16Z

+// GUID for gEfiDxeMmReadyToLockProtocolGuid
+// { 0x60ff8964, 0xe906, 0x41d0, { 0xaf, 0xed, 0xf2, 0x41, 0xe9, 0x74, 0xe0, 0x8e } }
+/// GUID for the DXE MM Ready To Lock protocol.
+pub const EFI_DXE_MM_READY_TO_LOCK_PROTOCOL_GUID: efi::Guid = efi::Guid::from_fields(


kuqin12 · 2026-03-17T04:57:31Z

+/// GUID for depex data HOBs paired with driver `MemoryAllocationModule` HOBs.
+///
+/// `gMmSupervisorDepexHobGuid`
+pub const MM_SUPERVISOR_DEPEX_HOB_GUID: efi::Guid = efi::Guid::from_fields(


kuqin12 · 2026-03-17T04:58:41Z

+}
+
+// SAFETY: MmUserCore is designed to be shared across threads with proper synchronization.
+unsafe impl Send for MmUserCore {}


This was inherited from supervisor module. Will remove.

kuqin12 · 2026-03-17T05:00:22Z

@@ -0,0 +1,92 @@
+//! Protocol/Handle Database


:) that's what I thought as well. then the tpl based lock got in the way and I took the shorter route. Like the allocator comment, we will do that once this stabilizes.

kuqin12 · 2026-03-17T05:01:24Z

@@ -0,0 +1,135 @@
+//! Arch-specific timer functionality


Will create a separate PR for this for better readability.

kuqin12 · 2026-04-06T23:17:56Z

This is not a component...

kuqin12 · 2026-04-06T23:19:02Z

+including:
+
+- `UserCommandType` — Supervisor-to-user command enumeration
+- `MM_COMM_BUFFER_HOB_GUID` — Shared GUID for the communication buffer HOB


Stale doc. And rename the crate...

kuqin12 · 2026-04-06T23:19:22Z

+/// A call gate allows privilege level transitions through a far call instruction.
+#[repr(C, packed)]
+#[derive(Debug, Clone, Copy, Default)]
+pub struct CallGateDescriptor {


Is this really needed?

kuqin12 · 2026-04-06T23:19:31Z

kuqin12 · 2026-04-06T23:19:46Z

+//!   GS base addresses, allowing access to per-CPU data in the syscall handler.
+//!
+
+#![allow(unsafe_op_in_unsafe_fn)]


kuqin12 · 2026-04-06T23:26:25Z

+// { 0x3efafe72, 0x3dbf, 0x4341, { 0xad, 0x04, 0x1c, 0xb6, 0xe8, 0xb6, 0x8e, 0x5e }}
+/// GUID used in MemoryAllocationModule HOBs to identify MM Supervisor module allocations.
+pub const MM_SUPERVISOR_HOB_MEMORY_ALLOC_MODULE_GUID: efi::Guid =
+    efi::Guid::from_fields(0x3efafe72, 0x3dbf, 0x4341, 0xad, 0x04, &[0x1c, 0xb6, 0xe8, 0xb6, 0x8e, 0x5e]);


Update this.

kuqin12 · 2026-04-06T23:27:00Z

+///
+/// `gMmSupervisorHobMemoryAllocModuleGuid`
+pub const MM_SUPERVISOR_HOB_MEMORY_ALLOC_MODULE_GUID: efi::Guid =
+    efi::Guid::from_fields(0x3efafe72, 0x3dbf, 0x4341, 0xad, 0x04, &[0x1c, 0xb6, 0xe8, 0xb6, 0x8e, 0x5e]);


Update this

kuqin12 · 2026-04-06T23:27:28Z

+
+// GUID for gEfiSmmSmramMemoryGuid
+// { 0x6dadf1d1, 0xd4cc, 0x4910, { 0xbb, 0x6e, 0x82, 0xb1, 0xfd, 0x80, 0xff, 0x3d }}
+pub const SMM_SMRAM_MEMORY_GUID: efi::Guid =


Update this

kuqin12 · 2026-04-06T23:28:40Z

+// GUID for gEfiDxeMmReadyToLockProtocolGuid
+// { 0x60ff8964, 0xe906, 0x41d0, { 0xaf, 0xed, 0xf2, 0x41, 0xe9, 0x74, 0xe0, 0x8e } }
+/// GUID for the DXE MM Ready To Lock protocol.
+pub const EFI_DXE_MM_READY_TO_LOCK_PROTOCOL_GUID: efi::Guid =


kuqin12 · 2026-04-06T23:29:05Z

+/// GUID used in `MemoryAllocationModule` HOBs to identify MM Supervisor module allocations.
+///
+/// `gMmSupervisorHobMemoryAllocModuleGuid`
+pub const MM_SUPERVISOR_HOB_MEMORY_ALLOC_MODULE_GUID: efi::Guid =


Adds a new "Hardware Access" section to the developer documentation to host guides and conventions for accessing hardware in Patina. Right now, this includes a new page on Memory-Mapped I/O (MMIO) that explains why MMIO must be accessed through raw pointers in Rust, and why we're using the safe-mmio crate to do so. Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>

Replace raw pointer volatile reads/writes for the PL011 UART code with safe-mmio abstractions. ARM PrimeCell UART (PL011) register properties taken from: https://developer.arm.com/documentation/ddi0183/g/programmers-model/summary-of-registers Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>

Since the `patina` crate serves as the underlying SDK, this change allows consumers to use `patina::mmio` instead of depending on `safe-mmio` directly for the reasons stated in the mmio.md file update. Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>

The rand crate is being rejected by cargo-deny because some of it's interfaces expose UB through safe functions. This commit updates to a fixed version to resolve the deny.

…ePartnership#1463) ## Description The patina implementation uses QWORD String event type for MODULE_DB_STOP_END_ID instead of MODULE_DB_END_ID, which is causing asserts when parsing the FDPT as this in unexpected. This commit fixes this to be consistent with the EDKII implementation. - [x] Impacts functionality? - [ ] Impacts security? - [ ] Breaking change? - [ ] Includes tests? - [ ] Includes documentation? ## How This Was Tested Tested on Q35 by parsing from shell with `dp` ## Integration Instructions N/A

An automatically created pull request to update the version of the repo due to a release. Co-authored-by: patina-automation[bot] <patina@microsoft.com>

…penDevicePartnership#1509) ## Description Allows the memory range details to be visible if the call to `GCD.add_memory_space()` fails. - [ ] Impacts functionality? - [ ] Impacts security? - [ ] Breaking change? - [ ] Includes tests? - [ ] Includes documentation? ## How This Was Tested - `cargo make all` - Boot and examine when the message is printed. ## Integration Instructions - N/A Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>

## Description **Status: Approved** Closes OpenDevicePartnership#1487 Defines a code first process for Patina. At this time, the focus is on UEFI Forum specifications with the expectation that the process can be adapted for other standards bodies and specifications as needed in the future. - [ ] Impacts functionality? - [ ] Impacts security? - [ ] Breaking change? - [ ] Includes tests? - [x] Includes documentation? ## How This Was Tested - cspell and markdownlint ## Integration Instructions - N/A --------- Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>

Adds a GitHub issue template that is used to submit code first tracking issues. The template and how it is used is described in the Patina Code First RFC: https://github.com/OpenDevicePartnership/patina/blob/HEAD/docs/src/rfc/text/0027-code-first.md Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>

Adds a page to give a high-level intro to the Patina Code First process and link to the RFC. This page is meant to serve as a place in the book to get a quick understanding of when the process applies and how it used with the formal definition being maintained in the RFC. Also updates text in code_first/template.md to escape square brackets so they might not be interpreted as a markdown link. Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>

This adds a doc for guidance on writing C FFIs. Signed-off-by: Oliver Smith-Denny <osde@microsoft.com>

## Description Updates dependencies to the latest compatible versions. The latest mdbook release is 0.5.2 but the latest mdbook-admonish release 1.20.0 is not compatible with mdbook 0.5.x. So, mdbook is updated to the latest 0.4.x release, which is 0.4.52. The CSS file for mdbook-admonish is checked into the repo, per the admonish documentation for compatibility and reproducible builds. The `multilingual` key is removed from `book.toml` since it will be deprecated in mdbook 0.5.x. - [ ] Impacts functionality? - [ ] Impacts security? - [ ] Breaking change? - [ ] Includes tests? - [ ] Includes documentation? ## How This Was Tested - Install mdbook related crates - `mdbook build ./docs` - `mdbook serve ./docs` ## Integration Instructions - N/A Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>

…hip#1516) ## Description Adds the `#[serial]` attribute to the `test_uefi_routine()` unit test since it modifies the global `EXCEPTION_HANDLERS` array (matching neighboring functions that register exception handlers) and the `CALLBACK_INVOKED` global (which is only in the `tests` module), which might be used by other tests in the future. Also sets `CALLBACK_INVOKED` to `false` at the start of the test so that the test can assert that the callback was flipped to true by the test. Sets it back to `false` when done for clean exit state. This is currently the only test using it, so it's more for future-proofing than anything else. - [ ] Impacts functionality? - [ ] Impacts security? - [ ] Breaking change? - [x] Includes tests? - [ ] Includes documentation? ## How This Was Tested - `cargo make all` ## Integration Instructions - N/A Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>

Today, trying to run mdbook tests locally is a bit of a pain because: 1. The `dev` profile is used to build dependencies used by code in the mdbook, and it has LTO enabled. This means that the resulting rlibs don't contain machine code and can't be linked by `rustdoc --test` when running the doctests embedded in the mdbook. This results in link errors. 2. mdbook defaults to using the stable toolchain ignoring the `rust-toolchain.toml` file, which means that you have to manually switch to the nightly toolchain before running the tests. 3. The dependencies have to be built separately (`cargo make build-lib`). (which are built using the toolchain in `rust-toolchain.toml`) This is accounted for in CI with manual steps: https://github.com/OpenDevicePartnership/patina-devops/blob/main/.github/workflows/MdbookWorkflow.yml This commit simplifies all of this by introducing a new profile specifically for mdbook that inherits from `dev` but disables LTO and adding a `cargo-make` task to build the dependencies using this profile and run `mdbook test` using the toolchain specified in `rust-toolchain.toml`. Another task was added called `serve-mdbook` to make it easier to open the mdbook locally similar to `doc-open` for the docs. - Test mdbook code: `cargo make test-mdbook` - Serve mdbook locally: `cargo make serve-mdbook` Because `test-mdbook` is added to `all`, this also has the advantage that mdbook tests are run during normal local checks. Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com> fixup

I could not find "playpen" in the mdbook documentation: https://rust-lang.github.io/mdBook/ As far as I can tell, "playpen" is a deprecated name for "playground". This changes the name of the section in book.toml to align with current mdbook documentation. Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>

Several UEFI protocol entry points dereference caller-supplied pointers directly (`*ptr` or `ptr.as_mut()`). This can be undefined behavior when the C caller passes an unaligned address. The UEFI ABI does not guarantee that out-parameters or input structures meet Rust's natural alignment for the pointee type, so most loads and stores through a raw FFI pointer must go through `read_unaligned()` / `write_unaligned()`. While we've followed this pattern in the past, some cases were missed that are updated in this change. Also adds a Pointer Alignment section to the FFI Authoring doc (ffi.md) that describes the rationale along with a few examples. Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>

Two UEFI entry points dereferenced caller-supplied pointers without validating them: - patina_dxe_core: `copy_mem()` and `set_mem()` called `core::ptr::copy()` / `slice::from_raw_parts_mut()` on a null pointer, if the caller passed one, with a non-zero length. - patina_adv_logger: `adv_log_write()` constructed a slice from `buffer` and a reference from `this` with no validation. The boot services functions now `debug_assert!()` on null with a non-zero length to catch caller bugs clearly in debug builds, then fall through to a safe early return in release. Since the function does not return a value, this prevents a panic from propagating across the FFI boundary. `adv_log_write()` now returns `EFI_INVALID_PARAMETER` when `this` is null or when `buffer` is null`. The SAFETY comments are updated to reflect the new invariants. Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>

CPU Arch Protocol functions call a common helper to convert the raw `this` pointer to a reference. Before, the helper panicked on null pointers. This commit changes the helper to return `None` on null pointers, so that the FFI entry points can return `EFI_INVALID_PARAMETER` instead of panicking across the FFI boundary. Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>

There are currently no unit tests for this module. Though the test support needed to write tests without much overhead are in place, such as UartNull. This commit adds some basic tests for `adv_log_write` that verify the pointer validation logic and calls to write the message when expectations are valid. This commit also adds the `serial_test` crate as a dev-dependency and annotates the tests that mutate module-level or global state with `#[serial]`. Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com> temp

…DevicePartnership#1521) ## Description `init_test_logger()` is used by various modules to set up logging during tests. This was originally setup to use `env_logger` when `RUST_LOG` is set allowing simple control of log messages during interactive test runs and providing partial coverage of log messages in tests with the following description given (OpenDevicePartnership#1093): > Since Patina relies on the `log::*` API for logging, not > configuring a default logger can cause `log::*` calls to appear > completely uncovered in llvm-cov reports, slightly reducing > overall coverage. Adding a simple environment based logger avoids > this issue. > Although this change enables coverage for `log::*`, > there are still cases that cannot be covered. For example, in the > function below, there is no way to get coverage for lines 6 and 7. > ``` > 1 1 fn print() { > 2 1 let mut i = 0; > 3 11 for _ in 0..10 { > 4 10 log::info!( > 5 10 "This will be covered {} {}", > 6 i, > 7 "This and the above line will not be covered." > 8 ); > 9 10 i = i + 1; > 10 } > 11 1 } > ``` This commit adds coverage of the log lines. `init_test_logger` previously defaulted `env_logger` to `LevelFilter::Off` when `RUST_LOG` was unset. That sets `STATIC_MAX_LEVEL` to `Off` and installs a logger whose `enabled()` returns false, so every `log::*!` macro short-circuits before it gets to the formatting and dispatch logic. Under `cargo-llvm-cov` this leaves log call sites flagged as uncovered, even though the surrounding code is exercised by tests. This change switches the unset-`RUST_LOG` path to a tiny custom `log::Log` implementation that reports every record as enabled but discards it, combined with `set_max_level(Trace)`. This allows tests to stay silent, but `log_enabled!()` now returns true so the formatting branch of each `log::*!` invocation actually runs and gets counted by coverage. The `RUST_LOG`-set path is unchanged and still routes through `env_logger` for real output during interactive debugging. --- - [ ] Impacts functionality? - [ ] Impacts security? - [ ] Breaking change? - [x] Includes tests? - [ ] Includes documentation? ## How This Was Tested - `cargo make coverage` before and after ## Integration Instructions - N/A Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>

…p#1522) synced local file(s) with [OpenDevicePartnership/patina-devops](https://github.com/OpenDevicePartnership/patina-devops). 🤖: View the [Repo File Sync Configuration File](https://github.com/OpenDevicePartnership/patina-devops/blob/main/.sync/Files.yml) to see how files are synced. --- This PR was created automatically by the [repo-file-sync-action](https://github.com/BetaHuhn/repo-file-sync-action) workflow run [#26311056562](https://github.com/OpenDevicePartnership/patina-devops/actions/runs/26311056562) Signed-off-by: Patina Automation Bot <patina@microsoft.com> Co-authored-by: patina-automation[bot] <196234736+patina-automation[bot]@users.noreply.github.com>

## Description The current handler unregister logic was broken because the null check will never hit. This is because the `extern "efiapi" fn` will not be null pointer and the casting to c_void will just be optimized out. The change moves the hosted handler type to an option to check the validity of the incoming parameters. - [x] Impacts functionality? - [ ] Impacts security? - [ ] Breaking change? - [ ] Includes tests? - [ ] Includes documentation? ## How This Was Tested This was tested on physical hardware and can unregister handler successfully. ## Integration Instructions N/A

…#1527) The updated goblin added some internal checks for PE consistency that is failing for aarch64 with our test binary. This commit updates the check to allow for either the patina or the goblin failures to be accepted.

The unstable alloc_error_handler feature is no longer required as the default_alloc_error_handler is stabilized. Patina is not doing anything special in this case, just panicking, so drop the unstable feature. Signed-off-by: Oliver Smith-Denny <osde@microsoft.com>

Currently when various states occur, such as drivers not dispatched, only the GUID of the driver is printed. This is useful, but also it is nicer to have the UI name printed so the GUID does not have to be cross-referenced. This parses the UI name from the FV, if present, and also prints it.

…ership#1524) ## Description The module could be used in the user environment, and `cli` will cause #GP. This change will check on the interrupt state and opt out the `cli` if it is already disabled. - [ ] Impacts functionality? - [ ] Impacts security? - [ ] Breaking change? - [ ] Includes tests? - [ ] Includes documentation? ## How This Was Tested This was tested for building the uart module for ring 3 module and booted to OS desktop properly. ## Integration Instructions N/A

github-actions Bot added the impact:security Has a security impact label Mar 11, 2026

kuqin12 marked this pull request as draft March 11, 2026 00:33

kuqin12 changed the base branch from main to feature/supv March 11, 2026 01:15

kuqin12 force-pushed the supv_init branch from 7417dcd to c4b9fa9 Compare March 11, 2026 09:51