Re: [edk2-devel] [PATCH 1/1] StandaloneMmPkg/Core: Remove optimization for depex evaluation

["Laszlo Ersek" <lersek@redhat.com>]

On 1/19/24 05:56, Nhi Pham wrote:
> From: Laszlo Ersek <lersek@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 <nhi@os.amperecomputing.com>
> ---
>  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;

looks good to me, thanks! (Can't give an R-b or A-b for a patch that was
originally written by me, just confirming that the code and commit
message look good.)



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114047): https://edk2.groups.io/g/devel/message/114047
Mute This Topic: https://groups.io/mt/103824815/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-