Skip to content

Conversation

@K-Pavana
Copy link

@K-Pavana K-Pavana commented Dec 11, 2025

Description

Needed to integrate Intel ref code without the change due to a build issue.


Due to PEIM will do following MM notify event under API mode:

  1. MM end of dxe notify Event
  2. MM ready to lock notify Event
  3. MM ready to boot notify Event
  4. MM exit boot services notify Event

It will conflict with the notify event in MmCommunicationDxe.inf on edk2 bootloader under API mode, so split following MmEvent to MmCommunicationNotifyDxe.inf, and avoid run this driver under API mode.

Cc: Ray Ni [email protected]
Cc: Star Zeng [email protected]
Cc: Jiaxin Wu [email protected]
Cc: Dun Tan [email protected]
Cc: Khor Swee Aun [email protected]
Cc: Sami Mujawar [email protected]
Cc: Ard Biesheuvel [email protected]
(cherry picked from commit e44cb97)

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

How This Was Tested

Code that uses this change on a server platform

Integration Instructions

Todo

Due to PEIM will do following MM notify event under API mode:
1.MM end of dxe notify Event
2.MM ready to lock notify Event
3.MM ready to boot notify Event
4.MM exit boot services notify Event
It will conflict with the notify event in MmCommunicationDxe.inf
on edk2 bootloader under API mode, so split following MmEvent to
MmCommunicationNotifyDxe.inf, and avoid run this driver under API
mode.

Signed-off-by: Hongbin1 Zhang <[email protected]>
Cc: Ray Ni <[email protected]>
Cc: Star Zeng <[email protected]>
Cc: Jiaxin Wu <[email protected]>
Cc: Dun Tan <[email protected]>
Cc: Khor Swee Aun <[email protected]>
Cc: Sami Mujawar <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
(cherry picked from commit e44cb97)
Copy link
Member

@makubacki makubacki left a comment

Choose a reason for hiding this comment

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

@K-Pavana. I've updated the integration instructions in the PR description for "N/A" to "Todo". There are definitely integration instructions and this is a breaking change.

Please update the integration instructions to explain:

  1. What platforms are impacted by this change (e.g. those that use MmCommunicationDxe in StandaloneMmPkg)
  2. What is expected of those platforms (e.g. add the new module and keep the original)
  3. What is expected by platforms that use Mu MM Supervisor communication

@makubacki makubacki added the impact:breaking-change Requires integration attention label Dec 11, 2025
@kuqin12
Copy link
Contributor

kuqin12 commented Dec 11, 2025

@makubacki do you understand the commit message? Where in PEIM are they doing these events?

@makubacki
Copy link
Member

makubacki commented Dec 12, 2025

@makubacki do you understand the commit message? Where in PEIM are they doing these events?

Given it is talking about "API mode", I assume what it means is that:

"FSP has API mode and dispatch mode. Because dispatch mode is PI-compatible, it can register real DXE handlers. Because API mode is intended to work with non-PI bootloaders, all of its event handlers must be registered in PEI (a PEIM) because FSP hosts its own PEI core in API mode. However, nothing stops API mode from being used by a PI-compatible (edk2) wrapper that has DXE. In that case, we now have a DXE driver, MmCommunicationDxe that duplicates functionality with the FSP API mode PEIM. Let's avoid that by splitting the conflicting code to a separate driver that is not included in API mode."

//
// Lock the SMRAM (Note: Locking SMRAM may not be supported on all platforms)
//
mSmmAccess->Lock (mSmmAccess);
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is a cherry-pick, but where is the close call?

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

Labels

impact:breaking-change Requires integration attention

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants