On Mon, 22 Jan 2024 at 03:34, Nhi Pham via groups.io
<nhi=os.amperecomputing....@groups.io> wrote:
>
> From: Laszlo Ersek <ler...@redhat.com>
>
> 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 <n...@os.amperecomputing.com>
> Tested-by: levi.yun <yeoreum....@arm.com>
> Reviewed-by: levi.yun <yeoreum....@arm.com>
Merged as #5289
Thanks,
> ---
> StandaloneMmPkg/Core/Dependency.c | 37 ++++----------------
> 1 file changed, 7 insertions(+), 30 deletions(-)
>
> diff --git a/StandaloneMmPkg/Core/Dependency.c
> b/StandaloneMmPkg/Core/Dependency.c
> index 440fe3e45238..2bcb07d34666 100644
> --- a/StandaloneMmPkg/Core/Dependency.c
> +++ b/StandaloneMmPkg/Core/Dependency.c
> @@ -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
> ///
> @@ -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;
> @@ -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)) {
> @@ -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;
> --
> 2.25.1
>
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114243): https://edk2.groups.io/g/devel/message/114243
Mute This Topic: https://groups.io/mt/103879487/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
