Skip to content

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

Open
kuqin12 wants to merge 29 commits intoOpenDevicePartnership:mainfrom
kuqin12:bu2
Open

[patina_mm] refactor current folder structure to allow external consumption#1413
kuqin12 wants to merge 29 commits intoOpenDevicePartnership:mainfrom
kuqin12:bu2

Conversation

@kuqin12
Copy link
Contributor

@kuqin12 kuqin12 commented Mar 17, 2026

Description

Current patina_mm carries some definitions that can be shared by external users (mm communication header, buffer status, etc).

This change moves those files to a sub-module that does not require alloc.

It also separates the supervisor related definitions into its own file.

  • Impacts functionality?
  • Impacts security?
  • Breaking change?
  • Includes tests?
  • Includes documentation?

How This Was Tested

This was tested with local builds and booted to UEFI shell.

Integration Instructions

N/A

@patina-automation
Copy link
Contributor

patina-automation bot commented Mar 17, 2026

✅ QEMU Validation Passed

All QEMU validation jobs completed successfully.

Note: Q35 is only built on Windows hosts (QEMU boot is disabled due to a QEMU vfat issue).

Workflow run: https://github.com/OpenDevicePartnership/patina/actions/runs/23364111544

Boot Time to EFI Shell

Platform Elapsed
Q35 (Linux Host) 28.8s
SBSA (Linux Host) 35.2s

Dependencies

Repository Ref
patina bca160e
patina-dxe-core-qemu 20381f1
patina-fw-patcher 3b8900d
patina-qemu firmware v2.0.0
patina-qemu build script 2e45e9b

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

@github-actions github-actions bot added the impact:non-functional Does not have a functional impact label Mar 17, 2026
};

let request_bytes = unblock_mem_request.to_bytes();
let request_bytes = unblock_mem_request.as_bytes().to_vec();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

??

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure what is going on with this one...

@codecov
Copy link

codecov bot commented Mar 19, 2026

Codecov Report

❌ Patch coverage is 88.63636% with 5 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
.../management_mode/protocol/mm_supervisor_request.rs 80.77% 5 Missing ⚠️

📢 Thoughts on this report? Let us know!

@kuqin12 kuqin12 marked this pull request as ready for review March 20, 2026 00:41
@kuqin12 kuqin12 self-assigned this Mar 20, 2026
@kuqin12 kuqin12 requested review from Javagedes and makubacki March 20, 2026 00:41
@Javagedes
Copy link
Contributor

@makubacki Curious to get your thoughts here but seeing as this particular crate (patina_mm) is a component, it does not make sense to carry public definitions that will be used by others. The intent of component crates is to only really be consumed by platforms when registering components or by other components when they use services provided by the component.

I'm wondering if we should move common definitions for patina_mm related things into the patina crate rather than here. With the current implementation this will create a weird dependency where the supervisor depends on this component.

@makubacki
Copy link
Collaborator

@makubacki Curious to get your thoughts here...

I haven't looked at the PR changes yet, but patina_mm was originally written to provide components and supporting configs and services. That code had to proceed conservatively until the broader looming MM changes were ready. There might be some code in there that makes sense to refactor based on those larger MM changes.

I agree with your reasoning. Components provide functionality, not public definitions. Yes, common definitions should be moved to the sdk/patina crate.

@kuqin12
Copy link
Contributor Author

kuqin12 commented Mar 20, 2026

I'm wondering if we should move common definitions for patina_mm related things into the patina crate rather than here. With the current implementation this will create a weird dependency where the supervisor depends on this component.

I agree with your reasoning. Components provide functionality, not public definitions. Yes, common definitions should be moved to the sdk/patina crate.

Sounds good. Will update accordingly.

@kuqin12
Copy link
Contributor Author

kuqin12 commented Mar 20, 2026

Okay, moved the non-alloc related definitions (header structures, constants, protocols) to patina and updated the consumers to use the new definitions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

impact:non-functional Does not have a functional impact

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants