MmSupervisorPkg: Update PlatformIntegration documentation#574
Conversation
mu-automation
Bot
added
type:documentation
Update PlatformIntegrationSteps with additional information and debugging steps. Clarify the platform integration requirements. Update drivers recommended to be removed.
apop5
force-pushed
the
personal/apop5/documentation
branch
from
daa196a to
333c8d8
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #574 +/- ##
========================================
+ Coverage 0.34% 0.52% +0.18%
========================================
Files 133 133
Lines 20179 20107 -72
Branches 60 60
========================================
+ Hits 70 106 +36
+ Misses 20082 19996 -86
+ Partials 27 5 -22
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| 1. Add the DSC sections below. This is the basic setup required, but platforms may require additional | ||
| drivers for proper function. |
There was a problem hiding this comment.
Was it intentional to drop "libraries" from the previous text? It is true that additional drivers or libraries might need to be added.
| 1. Add the DSC sections below. This is the basic setup required, but platforms may require additional | ||
| drivers for proper function. | ||
|
|
||
| ``` bash |
There was a problem hiding this comment.
Should we try to put this into a DSC that is built to have the build catch issues moving forward?
There was a problem hiding this comment.
based on the experience with integrating this, the modifications are meant to showcase a minimal set of changes to integrate, Basically, to show that MM Standalone entry points need to change to supervisor entry point lib, that BaseLib, etc need to go though SysCall interface.
Trying to create a fully working example is going to be of limited use. As a recent example, a platform provided specific library implementations of PciLib. These created dependency chains which were not able to be satisfied, and required tweaking platform level library instance choices.
I think the documentation is cleaner to provide more of a "these are the library instances which mm supervisor provides, and there will be integrator work required" is a better documentation route than a flat set of driver/library instances where the integrator thinks that is all that is needed.
There was a problem hiding this comment.
The purpose of putting this into a generic DSC form would be the same reason we run compilation tests in Rust documentation examples. It ensures the bare minimum example actually compiles with mu_basecore.
A lot of this can still be explained with comments in the DSC:
to show that MM Standalone entry points need to change to supervisor entry point lib, that BaseLib, etc need to go though SysCall interface.
This, it technically a different problem:
As a recent example, a platform provided specific library implementations of PciLib. These created dependency chains which were not able to be satisfied, and required tweaking platform level library instance choices.
This wouldn't attempt to solve that problem. But it would verify that references in MmSupervisor, UefiCpuPkg, StandaloneMmPkg, etc. paths are accurate per current versions of the mu_basecore and mu_feature_mm_supv code. For example, if DXE MM IPl support is dropped and these docs still reference stale files, it would be caught during build.
To be clear, I'm not proposing in any way that these be in a DSC that is made available to or included by a platform. Simply, that they are part of the MmSupervisorPkg build so the paths are validated. That said, I think everything you mentioned still stands with actually compiling the examples in a DSC file that has comments. I'm also fine to not do this since we've gotten this far without it, but with null libs and other mechanisms to stub out dependencies, it seems like more upside than downside to verify the example in the build.
| # Note: If chosen timer lib is platform dependent, and needs to be initialized before | ||
| # MmSupervisor is executed. | ||
| TimerLib|PcAtChipsetPkg/Library/AcpiTimerLib/StandaloneAcpiTimerLib.inf |
There was a problem hiding this comment.
| # Note: If chosen timer lib is platform dependent, and needs to be initialized before | |
| # MmSupervisor is executed. | |
| TimerLib|PcAtChipsetPkg/Library/AcpiTimerLib/StandaloneAcpiTimerLib.inf | |
| # Note: If chosen timer lib is platform dependent, and needs to be initialized before | |
| # MmSupervisor is executed. This example uses the Standalone MM ACPI timer library. | |
| TimerLib|PcAtChipsetPkg/Library/AcpiTimerLib/StandaloneAcpiTimerLib.inf |
There was a problem hiding this comment.
I'm in favor of PEI MM IPL, but these instructions assume that. I don't have a problem with it, but is the intention not to support DXE launch anymore?
There was a problem hiding this comment.
DXE launch has not been maintained for a long time. (and I actually meant to delete the driver for a while) Is there case we might be using it sometime in the future?
There was a problem hiding this comment.
The Edk2 standalone MM is only supporting PEI launch. To my understanding, we are following that example and not supporting DXE launch anymore.
There was a problem hiding this comment.
I'm not aware of a use case and I'm in favor of removing it. In that case, I think we should remove the code. If the code cleanup is done at this time, the documentation should clearly say it is not cleaned up, pending removal, and not covered by these instructions.
There was a problem hiding this comment.
I just double checked the codebase. Maybe I was on an old commit last week. But the DXE launch support was removed at some point. So let's just remove that in the doc as well.
There was a problem hiding this comment.
I just double checked the codebase. Maybe I was on an old commit last week. But the DXE launch support was removed at some point. So let's just remove that in the doc as well.
|
This PR has been automatically marked as stale because it has not had activity in 60 days. It will be closed if no further activity occurs within 7 days. Thank you for your contributions. |
|
This pull request has been automatically been closed because it did not have any activity in 60 days and no follow up within 7 days after being marked stale. Thank you for your contributions. |
|
not stale... |
|
This PR has been automatically marked as stale because it has not had activity in 60 days. It will be closed if no further activity occurs within 7 days. Thank you for your contributions. |
apop5
added
state:backlog
4 participants
Description
Update PlatformIntegrationSteps with additional information and debugging steps. Clarify the platform integration requirements. Update drivers recommended to be removed.
How This Was Tested
Local CI to check spelling and markdown
Integration Instructions
No integration necessary.