Skip to content

Commit

Permalink
[CHERRY-PICK] StandaloneMmPkg/Core: Remove optimization for depex eva…
Browse files Browse the repository at this point in the history
…luation (#706)

## Description

The current dependency evaluator violates the memory access permission
when patching depex grammar directly in the read-only depex memory area.

Laszlo pointed out the optimization issue in the thread (1) "Memory
Attribute for depex section" and provided suggested patch to remove the
perf optimization.

In my testing, removing the optimization does not make significant perf
reduction. That makes sense that StandaloneMM dispatcher only searches
in MM protocol database and does not depend on UEFI/DXE protocol
database. Also, we don't have many protocols in StandaloneMM like
UEFI/DXE.

From Laszlo,

"The patch removes the EFI_DEP_REPLACE_TRUE handling altogether, plus it
CONST-ifies the Iterator pointer (which points into the DEPEX section),
so that the compiler catch any possible accesses at *build time* that
would write to the write-protected DEPEX memory area."

(1) https://edk2.groups.io/g/devel/message/113531

Signed-off-by: Nhi Pham <[email protected]>
Tested-by: levi.yun <[email protected]>
Reviewed-by: levi.yun <[email protected]>
Reviewed-by: Ray Ni <[email protected]>

(cherry picked from commit 2ddae5d)

- [ ] Impacts functionality?
- **Functionality** - Does the change ultimately impact how firmware
functions?
- Examples: Add a new library, publish a new PPI, update an algorithm,
...
- [ ] Impacts security?
- **Security** - Does the change have a direct security impact on an
application,
    flow, or firmware?
  - Examples: Crypto algorithm change, buffer overflow fix, parameter
    validation improvement, ...
- [ ] Breaking change?
- **Breaking change** - Will anyone consuming this change experience a
break
    in build or boot behavior?
- Examples: Add a new library class, move a module to a different repo,
call
    a function in a new library class in a pre-existing module, ...
- [ ] Includes tests?
  - **Tests** - Does the change include any explicit test code?
  - Examples: Unit tests, integration tests, robot tests, ...
- [ ] Includes documentation?
- **Documentation** - Does the change contain explicit documentation
additions
    outside direct code modifications (and comments)?
- Examples: Update readme file, add feature readme file, link to
documentation
    on an a separate Web page, ...

## How This Was Tested

- QemuSbsaPkg boot (with StandaloneMmPkg core)

## Integration Instructions

- N/A

Co-authored-by: Laszlo Ersek <[email protected]>
  • Loading branch information
makubacki and lersek authored Jan 30, 2024
1 parent e774afb commit 376207b
Showing 1 changed file with 7 additions and 30 deletions.
37 changes: 7 additions & 30 deletions StandaloneMmPkg/Core/Dependency.c
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,6 @@

#include "StandaloneMmCore.h"

///
/// EFI_DEP_REPLACE_TRUE - Used to dynamically patch the dependency expression
/// to save time. A EFI_DEP_PUSH is evaluated one an
/// replaced with EFI_DEP_REPLACE_TRUE. If PI spec's Vol 2
/// Driver Execution Environment Core Interface use 0xff
/// as new DEPEX opcode. EFI_DEP_REPLACE_TRUE should be
/// defined to a new value that is not conflicting with PI spec.
///
#define EFI_DEP_REPLACE_TRUE 0xff

///
/// Define the initial size of the dependency expression evaluation stack
///
Expand Down Expand Up @@ -170,12 +160,12 @@ MmIsSchedulable (
IN EFI_MM_DRIVER_ENTRY *DriverEntry
)
{
EFI_STATUS Status;
UINT8 *Iterator;
BOOLEAN Operator;
BOOLEAN Operator2;
EFI_GUID DriverGuid;
VOID *Interface;
EFI_STATUS Status;
CONST UINT8 *Iterator;
BOOLEAN Operator;
BOOLEAN Operator2;
EFI_GUID DriverGuid;
VOID *Interface;

Operator = FALSE;
Operator2 = FALSE;
Expand Down Expand Up @@ -253,8 +243,7 @@ MmIsSchedulable (
Status = PushBool (FALSE);
} else {
DEBUG ((DEBUG_DISPATCH, " PUSH GUID(%g) = TRUE\n", &DriverGuid));
*Iterator = EFI_DEP_REPLACE_TRUE;
Status = PushBool (TRUE);
Status = PushBool (TRUE);
}

if (EFI_ERROR (Status)) {
Expand Down Expand Up @@ -356,18 +345,6 @@ MmIsSchedulable (
DEBUG ((DEBUG_DISPATCH, " RESULT = %a\n", Operator ? "TRUE" : "FALSE"));
return Operator;

case EFI_DEP_REPLACE_TRUE:
CopyMem (&DriverGuid, Iterator + 1, sizeof (EFI_GUID));
DEBUG ((DEBUG_DISPATCH, " PUSH GUID(%g) = TRUE\n", &DriverGuid));
Status = PushBool (TRUE);
if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_DISPATCH, " RESULT = FALSE (Unexpected error)\n"));
return FALSE;
}

Iterator += sizeof (EFI_GUID);
break;

default:
DEBUG ((DEBUG_DISPATCH, " RESULT = FALSE (Unknown opcode)\n"));
goto Done;
Expand Down

0 comments on commit 376207b

Please sign in to comment.