[Rebase & FF] Making StandaloneMmIplPei compatible with supervisor #1583
Conversation
StandaloneMmPkg/Drivers/MmCommunicationDxe/MmCommunicationDxe.c
Outdated
Show resolved
Hide resolved
| Size = sizeof (CommunicateHeader); | ||
| Status = Communicate (NULL, &CommunicateHeader, &Size); | ||
| ASSERT_EFI_ERROR (Status); | ||
| // ASSERT_EFI_ERROR (Status); |
There was a problem hiding this comment.
Not sure what to do with this.
There was a problem hiding this comment.
This is PRed as is. Will see if others have different opinions.
There was a problem hiding this comment.
For clarity, can you provide a simple comparison of design for this point?
There was a problem hiding this comment.
The issue behind this is that the supervised Standalone MM hosts this handler in supervisor channel. Thus to invoke this handler, we need to indicate the given MM communicate call is targeting supervisor handler pool (we have that wrapped in separate PPI and protocol).
The call here will issue an MMI with user mode data populated and will end up with EFI_NOT_FOUND error because there is not handler for driver dispatching in the user space.
The removal of the asserts here is basically telling this IPL to ignore the error, and it will be handled later through a supv specific module.
There was a problem hiding this comment.
I haven't followed the full code path, but would one of these options work?
- Make separate INF with the function that starts the call stack different for a "communicate" and "non-communicate" version of StandaloneMmIplPei.
- Setup a unique/appropriate status code return type that can bubble back up in this case and only assert if an error code and not that return type.
There was a problem hiding this comment.
oh, i see :) then we will bury that in the ring3 broker layer. That is fine with me. Let me try that in a bit. Thanks~!
There was a problem hiding this comment.
I ended up doing the second option, given the ring 3 broker is not dispatched yet. And the change will likely look like this: kuqin12/mu_feature_mm_supv@348e63e
There was a problem hiding this comment.
I think that's a little more preferable anyway to reduce platform integration overhead by avoiding two IPL INFs to pick from.
There was a problem hiding this comment.
Okay, this change is updated.
The current module checks the value of gEfiAcpiVariableGuid hobs and could assert if there is no such hob available. However, these hobs are only available if a platform elects to support S3. Thus this change moves the hob copy logic behind a PCD check to prevent unnecessary asserts. Signed-off-by: Kun Qin <[email protected]>
kuqin12
changed the title
kuqin12
changed the title
The current module checks the return status of MmIplDispatchMmDrivers. Any failure would cause the system to assert. However, this should not be deemed as fatal since the result could be entirely implementation defined on the Standalone MM core side. This change loosens the check and not to do asserts on dispatcher requests. Signed-off-by: Kun Qin <[email protected]>
kuqin12
force-pushed
the
dxe_mm_merge
branch
from
14b1357 to
d41a127
Compare
2 participants
Description
Current implementation of the
StandaloneMmIplPeihas a few design differences compared to MM supervisor.This will introduce more maintenance burden as we move to the next phase of MM supervisor.
This change is made to try to converge the 2 designs, at least from the perspective of IPL, in order to use the IPL as is and still conform to the MM supervisor modelity.
For details on how to complete these options and their meaning refer to CONTRIBUTING.md.
How This Was Tested
This is tested on QEMU Q35 and booted to UEFI shell.
Integration Instructions
N/A