Skip to content

Conversation

@Flickdm
Copy link
Member

@Flickdm Flickdm commented Dec 19, 2025

Description

This pull request refactors how the MM System Table is accessed within the StandaloneMmDriverEntryPoint.c file. Instead of passing the system table pointer around, it introduces a cached global pointer, simplifying function calls and improving code clarity. The unnecessary dependency on MmServicesTableLib is also removed.

Refactoring of MM System Table access:

  • Introduced a global variable mCachedMmSystemTable to cache the MM System Table pointer during driver initialization and replaced all usages of direct MmSystemTable or gMmst with this cached pointer throughout the file (StandaloneMmDriverEntryPoint.c). [1] [2] [3] [4]

Dependency cleanup:

  • Removed the unused MmServicesTableLib from both the includes in StandaloneMmDriverEntryPoint.c and the LibraryClasses section in the INF file (StandaloneMmDriverEntryPoint.inf). [1] [2]
  • Impacts functionality?
  • Impacts security?
  • Breaking change?
  • Includes tests?
  • Includes documentation?

How This Was Tested

QemuQ35

Integration Instructions

N/A

@Flickdm Flickdm requested review from apop5, kuqin12 and os-d December 19, 2025 19:48
@codecov-commenter
Copy link

codecov-commenter commented Dec 19, 2025

Codecov Report

❌ Patch coverage is 0% with 8 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (release/202502@1009044). Learn more about missing BASE report.

Files with missing lines Patch % Lines
...eMmDriverEntryPoint/StandaloneMmDriverEntryPoint.c 0.00% 8 Missing ⚠️
Additional details and impacted files
@@                Coverage Diff                @@
##             release/202502    #1605   +/-   ##
=================================================
  Coverage                  ?    2.40%           
=================================================
  Files                     ?     1256           
  Lines                     ?   303812           
  Branches                  ?     2650           
=================================================
  Hits                      ?     7314           
  Misses                    ?   296442           
  Partials                  ?       56           
Flag Coverage Δ
MdeModulePkg 1.55% <ø> (?)
MdePkg 5.45% <0.00%> (?)
UefiCpuPkg 4.92% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@makubacki
Copy link
Member

@Flickdm, global service tables accessed through service tables libraries is a common pattern. Is there more to this change? Afaict, the main issue with the previous code was that it used gMmst instead of MmSystemTable in the gMmst->MmHandleProtocol() call in the constructor.

@os-d
Copy link
Contributor

os-d commented Dec 19, 2025

@Flickdm, global service tables accessed through service tables libraries is a common pattern. Is there more to this change? Afaict, the main issue with the previous code was that it used gMmst instead of MmSystemTable in the gMmst->MmHandleProtocol() call in the constructor.

This is for OneCrypto, from a Sean request that I echoed. If the entry point lib doesn't use the services table lib, then the multiphase binary doesn't have to link in the services table lib. This helps to reduce the dependency chain; if something in the future that OneCrypto was relying on added a call to gMmst, we currently wouldn't catch it until it broke in runtime. If the library isn't linked in, we'd catch it at build time.

Copy link
Contributor

@os-d os-d left a comment

Choose a reason for hiding this comment

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

Not everything (including INF) covered by MU_CHANGE tags and commenting out old lines instead of removing.

@Flickdm
Copy link
Member Author

Flickdm commented Dec 19, 2025

Not everything (including INF) covered by MU_CHANGE tags and commenting out old lines instead of removing.

I can make that change real quick

@Flickdm Flickdm force-pushed the pr/StandaloneMmDriverCleanup branch from bc7905b to 2e0c743 Compare December 19, 2025 20:40
@makubacki
Copy link
Member

This is for OneCrypto, from a Sean request that I echoed. If the entry point lib doesn't use the services table lib, then the multiphase binary doesn't have to link in the services table lib. This helps to reduce the dependency chain; if something in the future that OneCrypto was relying on added a call to gMmst, we currently wouldn't catch it until it broke in runtime. If the library isn't linked in, we'd catch it at build time.

Is that code allowed to call the same services from the MmSystemTable pointer passed to their constructors and the entry point?

@os-d
Copy link
Contributor

os-d commented Dec 19, 2025

Is that code allowed to call the same services from the MmSystemTable pointer passed to their constructors and the entry point?

In the MM part of the driver, yes. But that is only done when loaded by StMM Core (when it calls _ModuleEntryPoint() which calls the constructors). In the DXE side of the driver, definitely not, but that path doesn't exist because there is a custom entry point and so the library constructors aren't called. But that's the concern, that the DXE side should never accidentally call gMmst, which this change lets us enforce at build time.

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.

I'm fine to approve the change for the one crypto use case. A few points:

  • This is unintuitive for the normal MM case. MmServicesTableLib already does exactly this and exposes the MM services table through a public symbol that is well established. This redundantly caches the same pointer in the same module when it is linked against MmServicesTableLib (which is nearly all MM modules).
  • It is also unintuitive to say that a library class restricted to certain module types cannot use another library class of the same type for the purpose that the services exclusive to the module type cannot be used.

However, there is little practical downside to this change, and it is not a breaking change, I just want to raise those points that you'll likely encounter when pushing it more broadly.

//
// Cached pointer to the MM System Table that is set during driver initialization.
//
EFI_MM_SYSTEM_TABLE *mCachedMmSystemTable = NULL;
Copy link
Member

Choose a reason for hiding this comment

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

nit: We don't normally include "cached" in the name of global pointer variables. I think mMmSystemTable is sufficient. But this is personal preference, feel free to keep this name if you like it.

@Flickdm Flickdm closed this Dec 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants