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

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

[Nhi Pham via groups.io]

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;

-- 
2.25.1



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

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

[Ard Biesheuvel]

On Fri, 19 Jan 2024 at 05:58, Nhi Pham via groups.io
<nhi=os.amperecomputing.com@groups.io> 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>

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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[levi.yun]

Except, 8bit transfer encoding.

Tested-by: levi.yun <yeoreum.yun@arm.com>
Reviewed-by: levi.yun <yeoreum.yun@arm.com>

On 19/01/2024 10:39, Ard Biesheuvel via groups.io wrote:
> On Fri, 19 Jan 2024 at 05:58, Nhi Pham via groups.io
> <nhi=os.amperecomputing.com@groups.io> 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>
> 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
>>
>>
>>
>>
>>
>>
>
> 
>
>

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.


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

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

[Laszlo Ersek]

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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Nhi Pham via groups.io]

On 1/19/2024 5:39 PM, Ard Biesheuvel wrote:
> 
> 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)
> 

Thanks Ard, I will resend the patch with 8bit transfer encoding.


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

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

[Nhi Pham via groups.io]

On 1/19/2024 6:51 PM, levi.yun wrote:
> Except, 8bit transfer encoding.
> 
> Tested-by: levi.yun <yeoreum.yun@arm.com>
> Reviewed-by: levi.yun <yeoreum.yun@arm.com>

Thanks Levi Yun for testing.

-Nhi


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

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

[Ni, Ray]

Reviewed-by: Ray Ni <ray.ni@intel.com>

PI spec does not define the REPLACE_TRUE op-code.
Existing dispatchers (DXE, SMM and Standalone MM) use REPLACE_TRUE to optimize the protocol evaluation. PEI dispatcher does not use this optimization as the Depex binary might be in flash.

Now this patch only modifies standalone MM version to not use the optimization. I think it's a right way.



Because the optimization cannot handle a case:
1. driver A installs protocol #a.
2. driver B depends on protocol #a.
3. driver A uninstalls the protocol #a in a callback (protocol callback, timer callback, or a service provided by A that driver B invokes)
4. driver B gets dispatched after the callback. <--- problem here. The protocol #a does not exist!!

@Kinney, Michael D, do you have history data of which optimization result it achieved? Do you think that the optimization can be in CoreLocateProtocol() instead of in the callers?

Thanks,
Ray
> -----Original Message-----
> From: Nhi Pham <nhi@os.amperecomputing.com>
> Sent: Friday, January 19, 2024 12:57 PM
> To: devel@edk2.groups.io
> Cc: ardb+tianocore@kernel.org; Ni, Ray <ray.ni@intel.com>;
> sami.mujawar@arm.com; lersek@redhat.com; Nhi Pham
> <nhi@os.amperecomputing.com>
> Subject: [PATCH 1/1] StandaloneMmPkg/Core: Remove optimization for
> depex evaluation
> 
> 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;
> 
> --
> 2.25.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114133): https://edk2.groups.io/g/devel/message/114133
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Michael D Kinney]

Hi Ray,

+Andrew Fish

That optimization was imported into git history 17 years ago, so
it has effectively always been there.  I do not recall the performance
improvement at the time the optimization was originally implemented.

The difference in behavior is that caching the result may miss
an uninstall later before dispatch. Always evaluating right before 
dispatch is safer because is guarantees that the expression is TRUE
and all conditions met when the image is started.

Your example is possible, but not likely to appear in practice for
the types of protocols used in dependency expressions.

Protocols that are uninstalled are more typically associated with 
the UEFI Driver model for buses/devices that can be added/removed.

If CoreLocateProtocol() was optimized, then perhaps this optimization
would have never been implemented.

I see no harm in removing the optimization, especially for only
Standalone MM.

If there is a need to treat DEPEX section of all images as const,
then there are other places that the cached evaluation could be
stored to enable this specific optimization for all image types.

Best regards,

Mike

> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Sunday, January 21, 2024 7:05 PM
> To: Nhi Pham <nhi@os.amperecomputing.com>; devel@edk2.groups.io; Kinney,
> Michael D <michael.d.kinney@intel.com>
> Cc: ardb+tianocore@kernel.org; sami.mujawar@arm.com; lersek@redhat.com
> Subject: RE: [PATCH 1/1] StandaloneMmPkg/Core: Remove optimization for
> depex evaluation
> 
> Reviewed-by: Ray Ni <ray.ni@intel.com>
> 
> PI spec does not define the REPLACE_TRUE op-code.
> Existing dispatchers (DXE, SMM and Standalone MM) use REPLACE_TRUE to
> optimize the protocol evaluation. PEI dispatcher does not use this
> optimization as the Depex binary might be in flash.
> 
> Now this patch only modifies standalone MM version to not use the
> optimization. I think it's a right way.
> 
> 
> 
> Because the optimization cannot handle a case:
> 1. driver A installs protocol #a.
> 2. driver B depends on protocol #a.
> 3. driver A uninstalls the protocol #a in a callback (protocol callback,
> timer callback, or a service provided by A that driver B invokes)
> 4. driver B gets dispatched after the callback. <--- problem here. The
> protocol #a does not exist!!
> 
> @Kinney, Michael D, do you have history data of which optimization
> result it achieved? Do you think that the optimization can be in
> CoreLocateProtocol() instead of in the callers?
> 
> Thanks,
> Ray
> > -----Original Message-----
> > From: Nhi Pham <nhi@os.amperecomputing.com>
> > Sent: Friday, January 19, 2024 12:57 PM
> > To: devel@edk2.groups.io
> > Cc: ardb+tianocore@kernel.org; Ni, Ray <ray.ni@intel.com>;
> > sami.mujawar@arm.com; lersek@redhat.com; Nhi Pham
> > <nhi@os.amperecomputing.com>
> > Subject: [PATCH 1/1] StandaloneMmPkg/Core: Remove optimization for
> > depex evaluation
> >
> > 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;
> >
> > --
> > 2.25.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114156): https://edk2.groups.io/g/devel/message/114156
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]
-=-=-=-=-=-=-=-=-=-=-=-