Re: [edk2-devel] Memory Attribute for depex section

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

Hi,

On 1/8/24 11:11, Nhi Pham via groups.io wrote:
> Hi Ard, all,
>
> Could you please help explain how the depex section in an image is
> mapped in terms of memory attribute?
>
> As my observation, dispatcher locates[1] the depex section inside the
> module image and write[2] an evaluated data to the depex if necessary
> for scheduled boot order. The problem that the depex section is now in
> RO+X memory due to a part of the module image, so a writing to depex
> would cause data abort. I'm unsure whether this issue is generic in
> EDK2 or not.
>
> I think of two approaches:
>
> #1 Relocate the depex section to heap memory for dependency
> #evaluation?
>
> #2 EDK2 build tool to support granting write permission for depex
> #section.
>
> [1] StandaloneMmPkg/Core/FwVol.c:236
> [2] StandaloneMmPkg/Core/Dependency.c:256

here's my understanding of the issue.

In Platform Init 1.8, section II-10.7 describes the depex instruction
set for the DXE DISPATCHER. In particular, section II-10.7.3 describes
the PUSH opcode as follows:

  PUSH <Protocol GUID>

  Pushes a Boolean value onto the stack. If the GUID is present in the
  handle database, then a TRUE is pushed onto the stack. If the GUID is
  not present in the handle database, then a FALSE is pushed onto the
  stack. The test for the GUID in the handle database may be performed
  with the Boot Service LocateProtocol().

This basically says that every time the dispatcher sees a PUSH opcode in
a depex, it has to look up the protocol GUID in the protocol database.

Now, as far as I can tell, edk2's DXE Core implementation wanted to
optimize this. (And this goes back to ancient commit 28a00297189c, from
2007.) Here's the idea AIUI:

- Assume the protocol is not found, i.e. we push a FALSE. This will
likely mean that the driver whose DEPEX contains this PUSH opcode cannot
be dispatched just yet. In other words, we're going to have to
re-evaluate this PUSH at least once more, possibly multiple times. We
cannot optimize anything here, because the needed protocol is not
present yet, but it may become available later. When we next evaluate
the DEPEX for this driver, we'll check again if the protocol has become
available by then.

- Assume the protocol is found, i.e. we push a TRUE. The optimization
here is that we assume the protocol will not *disappear*, once
available. The evaluation of the driver's *entire* DEPEX may still
result in FALSE, so it's not guaranteed that the driver can be
dispatched right now. However, given our assumption that the protocol
we've just found will not disappear during DXE driver dispatch, we might
want to "cache" the current successful protocol lookup, and when we
*next* evaluate the DEPEX for this same driver (assuming we cannot
dispatch it right now due to something else missing from its DEPEX), we
don't want to perform the *same* protocol lookup again -- we'll just
want to remember it was already there last time.

And the way the DXE core seems to implement this optimization (i.e., how
it "remembers success") is that it patches the DEPEX in-place: it
replaces the PUSH opcode with a special (edk2-specific) opcode called
EFI_DEP_REPLACE_TRUE (in addition to pushing the TRUE of course, to the
eval stack). This opcode is functionally identical to the plain (and
standard) TRUE opcode, so the next time the dispatcher evaluates the
same depex and sees EFI_DEP_REPLACE_TRUE it will just push TRUE, the
difference with the TRUE opcode is that EFI_DEP_REPLACE_TRUE also
provides "room" for the -- otherwise useless -- original protocol GUID
that used to be the argument of the PUSH opcode. The dispatcher ignores
the protocol GUID on EFI_DEP_REPLACE_TRUE, beyond logging it.

EFI_DEP_REPLACE_TRUE is documented in the code as follows
("MdeModulePkg/Core/Dxe/DxeMain.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

This documentation is hard to understand due to English grammar errors.
It means to say:

"Used to dynamically patch the dependency expression to save time.  An
EFI_DEP_PUSH opcode is evaluated *once*, *and* replaced with
EFI_DEP_REPLACE_TRUE. If PI spec's Vol 2 Driver Execution Environment
Core Interface *starts using* 0xff as new DEPEX opcode *in the future*,
*then* EFI_DEP_REPLACE_TRUE should be *redefined* to a new value that is
not conflicting with *said new* PI spec."

Over time, this optimization (hack) has made its way into the
traditional PiSmmCore, and finally into the standalone SMM core,
apparently.

In summary, this seems like a performance optimization, and should make
no functional difference. If we remove it, then driver dispatch could
become a bit slower (because we'd re-evaluate such PUSH opcodes too that
had previously succeeded in locating the given protocol GUID), but then
we'd not try to overwrite the depex section of the driver image in
memory.

I think that keeping the depex section read-only is valuable, so I'd
rule out #2. I'd also not start with option #1 -- copying the depex to
heap memory, just so this optimization can succeed. I'd simply start by
removing the optimization, and measuring how much driver dispatch slows
down in practice, on various platforms.

Can you try this? (I have only build-tested and "uncrustified" it.)

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.

-----------------------------
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;
-----------------------------

Thanks
Laszlo




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