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 · 2026-03-11T16:14:30Z

components/patina_mm/src/protocol/mm_supervisor_request.rs

+    ///
+    /// Returns `None` if the slice is too small or misaligned.
+    pub fn from_bytes(bytes: &[u8]) -> Option<Self> {
+        if bytes.len() < Self::SIZE {


I believe you can skip this indexing check by doing:

Self::read_from_bytes(&bytes.get(..Self::SIZE)?).ok()

<And in the other places you do similar, but I won't comment on them all>

components/patina_mm/src/protocol/mm_supervisor_request.rs

Javagedes · 2026-03-11T16:21:14Z

components/patina_mm/src/protocol/mm_supervisor_request.rs

+// GUID for gMmSupervisorRequestHandlerGuid
+// { 0x8c633b23, 0x1260, 0x4ea6, { 0x83, 0xf, 0x7d, 0xdc, 0x97, 0x38, 0x21, 0x11 } }
+/// GUID for the MM Supervisor Request Handler protocol.
+pub const MM_SUPERVISOR_REQUEST_HANDLER_GUID: Guid = Guid::from_fields(


Maybe @makubacki can comment, but I believe we have an intent to use our own types, such as patina::BinaryGuid

That's correct. Converting existing code is tracked in #1105. But for new code, we should use it. BinaryGuid is also more ergonomic to work with.

Thanks, will update to use BinaryGuid.

Javagedes · 2026-03-11T16:21:53Z

components/patina_mm/src/protocol/mm_supervisor_request.rs

+// Unblock Memory Params
+// ============================================================================
+
+use r_efi::efi;


This should be at the top of the file.

components/patina_mm/src/protocol/mm_supervisor_request.rs

kuqin12 · 2026-03-11T16:32:23Z

components/patina_mm/src/protocol/mm_supervisor_request.rs

+// Unblock Memory Params
+// ============================================================================
+
+use r_efi::efi;


components/patina_mm/src/protocol/mm_supervisor_request.rs

kuqin12 · 2026-03-11T16:34:55Z

core/patina_internal_mm_common/src/lib.rs

+//! Shared type definitions for MM supervisor and user cores.
+//!
+//! This crate provides the communication structures and enumerations that define
+//! the ABI between the supervisor (ring 0) and user (ring 3) MM modules.


Change the folder name, it should be common to supv only

kuqin12 · 2026-03-11T16:35:41Z

core/patina_internal_mm_alloc/src/lib.rs

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


Also only common to supv and user. Unless DXE environment will provide a common layer.

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.

components/patina_mm/src/protocol/mm_supervisor_request.rs

Javagedes · 2026-03-11T16:29:56Z

patina_mm_user_core/src/pool_allocator.rs

+/// Implementors provide the actual page allocation mechanism. The supervisor
+/// implements this with direct SMRAM bitmap tracking; the user core implements
+/// this by issuing syscalls to the supervisor.
+pub trait PageAllocatorBackend: Send + Sync {


nit: I think this should just be called PageAllocator.

I also don't think you need this is_initialized function. Just let allocate_pages / free_pages return an PageAllocError with maybe a enum variation of NotReady / NotInitialized / etc.

Javagedes · 2026-03-11T16:30:20Z

patina_mm_user_core/src/pool_allocator.rs

+    fn is_initialized(&self) -> bool;
+}
+
+// ============================================================================


We should get rid of all these AI-esque header block comment things.

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

core/patina_internal_mm_alloc/src/lib.rs

@@ -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

patina_mm_user_core/src/pool_allocator.rs

+    ///
+    /// 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

patina_mm_user_core/src/protocol_db.rs

@@ -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

patina_mm_user_core/src/mm_dispatcher.rs

@@ -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

core/patina_internal_cpu/src/save_state/mod.rs

Use the newer crate layout as mentioned earlier.

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

core/patina_internal_cpu/src/save_state/mod.rs

@@ -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

sdk/patina/src/timer/mod.rs

@@ -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.

…cePartnership#1400) ## Description patina_mtrr 1.1.5 changed the `Mtrr` trait's `get_memory_ranges()` return type from `Vec<MtrrMemoryRange>` to `impl IntoIterator<Item = MtrrMemoryRange>`. The mockall `mock!` block still uses the concrete Vec type, triggering the `refining_impl_trait_internal` lint. This commit adds an allow attribute to the `get_memory_ranges()` method in the mock to suppress the lint. - [ ] Impacts functionality? - [ ] Impacts security? - [ ] Breaking change? - [ ] Includes tests? - [ ] Includes documentation? ## How This Was Tested - `cargo make all` ## Integration Instructions - N/A Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>

…penDevicePartnership#1402) ## Description Closes OpenDevicePartnership#583 Closes OpenDevicePartnership#1396 Adds safety comments to the remaining unsafe blocks in patina_dxe_core and enables the clippy lint for missing safety docs. This allows future contributions to have the lint enforced. - [ ] Impacts functionality? - [ ] Impacts security? - [ ] Breaking change? - [ ] Includes tests? - [x] Includes documentation? ## How This Was Tested - `cargo make all` with the `undocumented_safety_blocks` clippy lint enabled in `patina_dxe_core` ## Integration Instructions - N/A - Safety comments that have no functional impact Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>

This function wraps `efi::Guid::from_bytes()` which is const. It is made const as well. Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>

Add Ord and PartialOrd derives to BinaryGuid, enabling its use as a BTreeMap key. Adds a test to verify BinaryGuid's derived ordering matches Guid<'a>'s byte-order comparison for a set of GUID pairs. Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>

This change adds a few standard GUID definitions. This is to pave way for later usage in rust supervisor.

Converts all GUID constants, trait associated types, struct fields, and other uses to Patina GUID types across the codebase. Key changes: - GUID constants mostly use `BinaryGuid::from_string()` since strings are more readable than fields. In some cases, a field may have been a simple numbering sequence like "123456...", in which case it may have been left as `from_fields`. - Changed `ProtocolInterface::PROTOCOL_GUID` changed to use `BinaryGuid`, eliminating duplicated GUID values in implementations. - Updated `FromHob::HOB_GUID` and `HobParser`s `BTreeMap` key to `BinaryGuid`. - Updated `#[repr(C)]` struct fields (GuidHob, FV/FFS headers, MM communicate header) to use `BinaryGuid` since it provides binary-compatible GUID storage. - Updated the `FromHob` proc macro to use `BinaryGuid::from_string()`. - Updated a lot of code to use Patina GUID types instead of the r-efi GUID type. There were a few places, particularly in UEFI Spec FFI interfaces, where the `efi::Guid` type is still used. This is intentional to potentially simplify the r-efi 6.0.0 integration. In code that interacts with those interfaces, `From` and `Into` functions are used to simply convert between `BinaryGuid` and `efi::Guid`. Some code that exclusively interacts with those interfaces, might also use `efi::Guid` for local GUID definitions instead of Patina GUID types. Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>

Patina-paging 11.0.2 introduces support for reading/editing existing 5-level page tables on AArch64. This is required for EDK2 20511 based system that have FEAT_LPA2 support. To use this change, this commit switches to use the new `open_active` interface for both AArch64 and x64, which also simplifies the consumer by handling level detection in the paging library.

writer.rs tests import `crate::reader::AdvancedLogReader`, but the reader module is conditionally compiled behind `#[cfg(any(doc, feature = "reader"))]`. This works when default features are enabled since the "component" default feature transitively enables "reader". This change gates `tests` on `#[cfg(all(test, feature = "reader"))]` so it compiles cleanly when default features are disabled. Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>

Without `--no-default-features` testing, feature-gate regressions go undetected. Code that accidentally uses a gated module or dependency outside its `cfg` fence compiles fine under default features but breaks for consumers that disable defaults. Since workspace crates like `patina_ffs_extractors` gate optional compression backends (brotli, crc32, lzma) behind features, and `patina/patina_internal_collections` gates alloc-dependent code behind an "alloc" default, verifying the `--no-default-features` build is useful to catch accidental dependencies on gated code. Add a higher level `check-no-default-features` task with two parallel subtasks: - `check-no-default-features-code`: - `cargo check --no-default-features` - `check-no-default-features-tests`: - `cargo test --no-run --no-default-features` Run the task in `cargo make all`. Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>

## Description Current patina_stacktrace is pulling in alloc from source code. However, as basic functionality as the crate provides, the dependency is improperly included. This change removes the dependency. Resolves OpenDevicePartnership#1410 - [ ] Impacts functionality? - [ ] Impacts security? - [ ] Breaking change? - [ ] Includes tests? - [ ] Includes documentation? ## How This Was Tested The change is not a functional change and was tested with local build. ## Integration Instructions N/A

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

patina_mm_supervisor_core/src/lib.rs

+#![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

patina_mm_supervisor_core/src/lib.rs

+    /// 
+    /// 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

patina_mm_supervisor_core/src/lib.rs

+                .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

patina_mm_supervisor_core/src/lib.rs

+        }
+
+        // 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

patina_mm_supervisor_core/src/mailbox.rs

+
+/// 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

patina_mm_supervisor_core/src/supervisor_handlers.rs

+// 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

patina_mm_user_core/src/lib.rs

+/// 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

patina_mm_user_core/src/lib.rs

+}
+
+// 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

patina_mm_user_core/src/protocol_db.rs

@@ -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

sdk/patina/src/timer/mod.rs

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


Will create a separate PR for this for better readability.

Update the status to indicate that the repository is no longer in a "beta" stage, but also add a note about the expected stability of the main branch and the need to verify the readiness of new components. Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>

Clarifies the expectations for contributions that are AI-assisted to Patina. Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>

Add some more detail to clarify common misunderstandings about Patina. Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>

) ## Description This change guards the usage of allocators behind "alloc" to a few remaining instances in Patina SDK. So that the crate can provide more fundamental functionalities without dependencies on allocator. In addition, it removes the `extern crate alloc;` within the crate and only declare it in the `lib.rs`, which serves as a central feature controller that governs the entirety of the crate. Resolves OpenDevicePartnership#1403 - [x] Impacts functionality? - [ ] Impacts security? - [ ] Breaking change? - [ ] Includes tests? - [ ] Includes documentation? ## How This Was Tested This was tested by building a binary without global allocator and booted to UEFI shell on Q35. ## Integration Instructions Nothing in addition to building the binary with `--features alloc` if a binary needs allocator functionality.

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

Javagedes reviewed Mar 11, 2026

View reviewed changes

components/patina_mm/src/protocol/mm_supervisor_request.rs Show resolved Hide resolved

Javagedes reviewed Mar 11, 2026

View reviewed changes

components/patina_mm/src/protocol/mm_supervisor_request.rs Show resolved Hide resolved

Javagedes reviewed Mar 11, 2026

View reviewed changes

kuqin12 commented Mar 11, 2026

View reviewed changes

Javagedes reviewed Mar 11, 2026

View reviewed changes

makubacki and others added 6 commits March 13, 2026 16:33

sdk: Make BinaryGuid::from_bytes() const

dabe562

This function wraps `efi::Guid::from_bytes()` which is const. It is made const as well. Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>

sdk: Add a few standard GUID definitions

e8656a9

This change adds a few standard GUID definitions. This is to pave way for later usage in rust supervisor.

kuqin12 force-pushed the supv_init branch 2 times, most recently from 8a5c38f to c0baea8 Compare March 16, 2026 21:52

cfernald and others added 4 commits March 16, 2026 14:57

kuqin12 commented Mar 17, 2026

View reviewed changes

makubacki and others added 5 commits March 17, 2026 13:56

CONTRIBUTING.md: Add Patina AI Policy

52a22f5

Clarifies the expectations for contributions that are AI-assisted to Patina. Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>

README.md: Elaborate on key points in the Background section

1488bf0

Add some more detail to clarify common misunderstandings about Patina. Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>

Perf timer based changes

d466355

kuqin12 added 8 commits March 17, 2026 21:40

Patina MM related changes

e04c6f3

[supv] save state support for supv

2b6abb8

[supv] policy support

6a1f0fe

[supv] supv syscall index definitions

0e214ed

[supv] supervisor core changes

db9abb8

[user] user module changes

2c97772

[supv] cargo changes

c122ffd

patina_internal_cpu

960e451

kuqin12 force-pushed the supv_init branch from c0baea8 to 960e451 Compare March 18, 2026 04:43

kuqin12 mentioned this pull request Mar 18, 2026

[patina_mm] refactor current folder structure to allow external consumption #1413

Open

5 tasks

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

Conversation

kuqin12 commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How This Was Tested

Integration Instructions

Uh oh!

patina-automation bot commented Mar 11, 2026

⌛ QEMU Validation Pending

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Javagedes left a comment

Choose a reason for hiding this comment

Uh oh!

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!

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!

kuqin12 commented Mar 11, 2026 •

edited

Loading