On Fri, 19 Jan 2024 at 05:58, 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>
Thanks for the patch. This looks good to me in principle, only the
patch got mangled by your MTA
Please resend using 8bit transfer encoding. (You can use
BaseTools/Scripts/SetupGit.py to configure Git for you)
> ---
> 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 (#114041): https://edk2.groups.io/g/devel/message/114041
Mute This Topic: https://groups.io/mt/103824815/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
