StandaloneMM fixes for x86 #5814

PatrickRudolph · 2024-06-24T12:45:44Z

Description

StandaloneMM fixes for x86.

How This Was Tested

Build using edk2-platform.

Integration Instructions

N/A

EFI_FV_FILETYPE_MM_STANDALONE contains PE32 images, so add it to the list to make sure the files are also rebased. Cc: Bob Feng <[email protected]> Cc: Liming Gao <[email protected]> Cc: Rebecca Cran <[email protected]> Cc: Yuwei Chen <[email protected]> Signed-off-by: Patrick Rudolph <[email protected]>

PE32 in StandaloneMmPkg can be loaded into memory can be executed in place (XiP). That saves memory as they don't need to be relocated to memory when already residing in memory. Cc: Ard Biesheuvel <[email protected]> Cc: Jiaxin Wu <[email protected]> Cc: Ray Ni <[email protected]> Cc: Sami Mujawar <[email protected]> Signed-off-by: Patrick Rudolph <[email protected]>

Currently the StandaloneMm loader needs to find a free memory range in the secure world and provide it to StandaloneMM to use it as free pool for runtime memory allocations. The caller might not now how much memory space to allocate. In addition allocating a separate memory region in the trusted MM memory space increases the complexity on the MM loader side. Add a PCD that allows to use a fixed size memory buffer on the heap as free memory pool. Using this option will increase the binary size, but it removes the callers need to provide a memory range that can be used as runtime memory. Cc: Ard Biesheuvel <[email protected]> Cc: Jiaxin Wu <[email protected]> Cc: Ray Ni <[email protected]> Cc: Sami Mujawar <[email protected]> Signed-off-by: Patrick Rudolph <[email protected]>

Add missing EFIAPI qualifier. Cc: Ard Biesheuvel <[email protected]> Cc: Jiaxin Wu <[email protected]> Cc: Ray Ni <[email protected]> Cc: Sami Mujawar <[email protected]> Signed-off-by: Patrick Rudolph <[email protected]>

ardbiesheuvel

SEC and PEI images can execute in place because there are tight restrictions on the use of global variables and statically allocated buffers. This means that such PE images cannot have BSS sections, and so the compile time footprint of the image is identical to the runtime footprint.

Is it guaranteed that this is also the case for standalone MM drivers? Where is this covered in the PI specification? Note that XIP generally means execute from flash, which is read-only. XIP from writable memory (or cache-as-RAM) is a different use case altogether, so before accepting these changes, I need to be convinced that the PI spec and the base tools cover this use case as well.

lgao4 · 2024-07-22T10:25:14Z

The change in BaseTools is good to me.

niruiyu · 2024-07-31T09:05:17Z

StandaloneMmPkg/Core/Dispatcher.c

+    //
+    // Load the image to our new buffer
+    //
+    Status = PeCoffLoaderLoadImage (&ImageContext);
+    if (EFI_ERROR (Status)) {
+      return Status;
+    }

-  //
-  // Relocate the image in our new buffer
-  //
-  Status = PeCoffLoaderRelocateImage (&ImageContext);
-  if (EFI_ERROR (Status)) {
-    MmFreePages (DstBuffer, PageCount);
-    return Status;
-  }
+    //
+    // Relocate the image in our new buffer
+    //
+    Status = PeCoffLoaderRelocateImage (&ImageContext);
+    if (EFI_ERROR (Status)) {
+      return Status;
+    }


Can you use the same code maximum for XIP and non-XIP images?

Similar code flow is in MdeModulePkg/Core/Pei/Image/Image.c: LoadAndRelocatePeCoffImage().

niruiyu · 2024-07-31T09:12:01Z

StandaloneMmPkg/Core/Pool.c

+
+  if (FixedPcdGetBool (PcdRuntimeMemoryOnHeap)) {
+    MmAddMemoryRegion (
+      (UINTN)FreePoolOnHeap,
+      sizeof (FreePoolOnHeap),
+      EfiConventionalMemory,
+      EFI_CACHEABLE
+      );
+  }


I guess the ARM standalone MM binary is loaded by some other module and that module allocates enough memory for the FreePoolOnHeap. MmCore can just use the FreePoolOnHeap as the free memory.
The concept looks good to me.

Can you move the logic to StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c? So that the MmCore still finds the MMRAM from the HOB while the HOB is produced by CreateHobList.c and points to FreePoolOnHeap defined in that library.

StanaloneMm is a HOB consumer and not a HOB producer.
Although the Arm code currently creates HOBs in StandaloneMm, there is a patch series at #6116 to remove the HOB creation.
Therefore, I think this approach discussed here is not the correct way forward :)

I think it's okay that the Standalone MM core modifies some of the HOB contents. The HOB read-only restriction applies to all MM drivers.

See

edk2/StandaloneMmPkg/Core/StandaloneMmCore.c

Line 656 in 0f9dbb4

MigrateMemoryAllocationHobs (

.

But, modification should be done after StandaloneMmCore call InitializeMmHobList

(See

edk2/StandaloneMmPkg/Core/StandaloneMmCore.c

Line 856 in 0f9dbb4

gHobList = InitializeMmHobList (HobStart, MmramRanges, MmramRangeCount);

)

Becuase StandaloneMmEntryPoint's Hob list passed in #6116., Could be readonly memory from firmware.

So, I think it's reasonable to modify contents in here.

However, I'm not sure which platform whether there is the case of
"The caller might not know how much memoryspace to allocate."

Since when StandaloneMm loaded, the Heap memory space is specified by manifest file and delivered always to StandaloneMm, I don't know it's useful for Arm.

niruiyu · 2024-07-31T09:19:49Z

StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c

+    // When runtime memory is allocated on the heap the buffer allocated by
+    // MmAllocatePool is within the MM 'secure world'. The MMI handler will
+    // assert as it checks again if the passed buffer is outside the 'secure world'.
+    // Thus drop the separate allocation and directly pass the NS buffer to
+    // the MMI handler.
+    //


old logic is to always allocate MMRAM and copy Non-Secure comm buffer into MMRAM.
I do not understand why it's not needed when FreePoolOnHeap is used.

niruiyu · 2024-07-31T09:24:33Z

SEC and PEI images can execute in place because there are tight restrictions on the use of global variables and statically allocated buffers. This means that such PE images cannot have BSS sections, and so the compile time footprint of the image is identical to the runtime footprint.

I always have doubts about XIP. Thanks for clarifying the restriction of no BSS section:)

Is it guaranteed that this is also the case for standalone MM drivers? Where is this covered in the PI specification? Note that XIP generally means execute from flash, which is read-only. XIP from writable memory (or cache-as-RAM) is a different use case altogether, so before accepting these changes, I need to be convinced that the PI spec and the base tools cover this use case as well.

I do not think PI spec mandates anything on XIP. I am curious what mechanism has been there to check the non-existence of the BSS section for SEC and PEI.

github-actions · 2024-09-29T23:39:23Z

This PR has been automatically marked as stale because it has not had activity in 60 days. It will be closed if no further activity occurs within 7 days. Thank you for your contributions.

mergify · 2024-09-29T23:39:59Z

PR can not be merged due to conflict. Please rebase and resubmit

github-actions · 2024-11-29T23:39:59Z

This PR has been automatically marked as stale because it has not had activity in 60 days. It will be closed if no further activity occurs within 7 days. Thank you for your contributions.

mergify · 2024-11-29T23:41:06Z

PR can not be merged due to conflict. Please rebase and resubmit

PatrickRudolph added 4 commits June 24, 2024 15:20

StandaloneMmPkg: Fix building on GCC

2e35918

Add missing EFIAPI qualifier. Cc: Ard Biesheuvel <[email protected]> Cc: Jiaxin Wu <[email protected]> Cc: Ray Ni <[email protected]> Cc: Sami Mujawar <[email protected]> Signed-off-by: Patrick Rudolph <[email protected]>

PatrickRudolph force-pushed the UpstreamStandaloneMM branch from 6b98243 to 2e35918 Compare June 24, 2024 13:21

PatrickRudolph marked this pull request as ready for review June 25, 2024 05:55

leiflindholm requested review from a user, lgao4, BobCF, YuweiChen1110, jiaxinwu, niruiyu, ardbiesheuvel and samimujawar July 19, 2024 12:02

ardbiesheuvel reviewed Jul 21, 2024

View reviewed changes

niruiyu requested changes Jul 31, 2024

View reviewed changes

github-actions bot added the stale Due to lack of updates, this item is pending deletion. label Sep 29, 2024

github-actions bot removed the stale Due to lack of updates, this item is pending deletion. label Sep 30, 2024

github-actions bot added the stale Due to lack of updates, this item is pending deletion. label Nov 29, 2024

github-actions bot removed the stale Due to lack of updates, this item is pending deletion. label Nov 30, 2024

PatrickRudolph closed this Dec 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

StandaloneMM fixes for x86 #5814

StandaloneMM fixes for x86 #5814

PatrickRudolph commented Jun 24, 2024

ardbiesheuvel left a comment

lgao4 commented Jul 22, 2024

niruiyu Jul 31, 2024

niruiyu Jul 31, 2024

LeviYeoReum Sep 30, 2024 •

edited

Loading

niruiyu Dec 4, 2024

LeviYeoReum Dec 4, 2024

niruiyu Jul 31, 2024

niruiyu commented Jul 31, 2024

github-actions bot commented Sep 29, 2024

mergify bot commented Sep 29, 2024

github-actions bot commented Nov 29, 2024

mergify bot commented Nov 29, 2024

StandaloneMM fixes for x86 #5814

StandaloneMM fixes for x86 #5814

Conversation

PatrickRudolph commented Jun 24, 2024

Description

How This Was Tested

Integration Instructions

ardbiesheuvel left a comment

Choose a reason for hiding this comment

lgao4 commented Jul 22, 2024

niruiyu Jul 31, 2024

Choose a reason for hiding this comment

niruiyu Jul 31, 2024

Choose a reason for hiding this comment

LeviYeoReum Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

niruiyu Dec 4, 2024

Choose a reason for hiding this comment

LeviYeoReum Dec 4, 2024

Choose a reason for hiding this comment

niruiyu Jul 31, 2024

Choose a reason for hiding this comment

niruiyu commented Jul 31, 2024

github-actions bot commented Sep 29, 2024

mergify bot commented Sep 29, 2024

github-actions bot commented Nov 29, 2024

mergify bot commented Nov 29, 2024

LeviYeoReum Sep 30, 2024 •

edited

Loading