Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

StandaloneMM fixes for x86 #5814

Closed
wants to merge 4 commits into from
Closed

StandaloneMM fixes for x86 #5814

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
15624ed
GenFv: Add EFI_FV_FILETYPE_MM_STANDALONE
PatrickRudolph Jun 4, 2024
1d23ede
StandaloneMmPkg: Support XIP
PatrickRudolph Jun 4, 2024
900b69e
StandaloneMmPkg/Core: Allocate runtime memory on heap
PatrickRudolph Jun 12, 2024
2e35918
StandaloneMmPkg: Fix building on GCC
PatrickRudolph Jun 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions BaseTools/Source/C/GenFv/GenFvInternalLib.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3537,6 +3537,7 @@ Routine Description:
case EFI_FV_FILETYPE_COMBINED_PEIM_DRIVER:
case EFI_FV_FILETYPE_DRIVER:
case EFI_FV_FILETYPE_DXE_CORE:
case EFI_FV_FILETYPE_MM_STANDALONE:
break;
case EFI_FV_FILETYPE_FIRMWARE_VOLUME_IMAGE:
//
Expand Down Expand Up @@ -3698,6 +3699,7 @@ Routine Description:

case EFI_FV_FILETYPE_DRIVER:
case EFI_FV_FILETYPE_DXE_CORE:
case EFI_FV_FILETYPE_MM_STANDALONE:
//
// Check if section-alignment and file-alignment match or not
//
Expand Down
105 changes: 67 additions & 38 deletions StandaloneMmPkg/Core/Dispatcher.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,49 +134,75 @@ MmLoadImage (
return Status;
}

PageCount = (UINTN)EFI_SIZE_TO_PAGES ((UINTN)ImageContext.ImageSize + ImageContext.SectionAlignment);
DstBuffer = (UINTN)(-1);

Status = MmAllocatePages (
AllocateMaxAddress,
EfiRuntimeServicesCode,
PageCount,
&DstBuffer
);
if (EFI_ERROR (Status)) {
return Status;
}

ImageContext.ImageAddress = (EFI_PHYSICAL_ADDRESS)DstBuffer;
PageCount = EFI_SIZE_TO_PAGES ((UINTN)ImageContext.ImageSize + ImageContext.SectionAlignment);

//
// Align buffer on section boundary
// XIP image that ImageAddress is same to Image handle.
//
ImageContext.ImageAddress += ImageContext.SectionAlignment - 1;
ImageContext.ImageAddress &= ~((EFI_PHYSICAL_ADDRESS)(ImageContext.SectionAlignment - 1));
if (ImageContext.ImageAddress == (EFI_PHYSICAL_ADDRESS)(UINTN)DriverEntry->Pe32Data) {
DstBuffer = (UINTN)ImageContext.ImageAddress;

//
// Load the image to our new buffer
//
Status = PeCoffLoaderLoadImage (&ImageContext);
if (EFI_ERROR (Status)) {
MmFreePages (DstBuffer, PageCount);
return Status;
}
//
// 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;
}
Comment on lines +145 to +159
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

} else {
DstBuffer = (UINTN)(-1);

Status = MmAllocatePages (
AllocateMaxAddress,
EfiRuntimeServicesCode,
PageCount,
&DstBuffer
);
if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_INFO, "MmLoadImage failed to allocate memory\n"));

//
// Flush the instruction cache so the image data are written before we execute it
//
InvalidateInstructionCacheRange ((VOID *)(UINTN)ImageContext.ImageAddress, (UINTN)ImageContext.ImageSize);
return Status;
}

ImageContext.ImageAddress = (EFI_PHYSICAL_ADDRESS)DstBuffer;

//
// Align buffer on section boundary
//
ImageContext.ImageAddress += ImageContext.SectionAlignment - 1;
ImageContext.ImageAddress &= ~((EFI_PHYSICAL_ADDRESS)(ImageContext.SectionAlignment - 1));

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

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

//
// Flush the instruction cache so the image data is written before we execute it
//
InvalidateInstructionCacheRange ((VOID *)(UINTN)ImageContext.ImageAddress, (UINTN)ImageContext.ImageSize);
}

//
// Save Image EntryPoint in DriverEntry
Expand All @@ -192,7 +218,10 @@ MmLoadImage (
(VOID **)&DriverEntry->LoadedImage
);
if (EFI_ERROR (Status)) {
MmFreePages (DstBuffer, PageCount);
if (ImageContext.ImageAddress != (EFI_PHYSICAL_ADDRESS)(UINTN)DriverEntry->Pe32Data) {
MmFreePages (DstBuffer, PageCount);
}

return Status;
}

Expand Down
23 changes: 23 additions & 0 deletions StandaloneMmPkg/Core/Pool.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,20 @@

#include "StandaloneMmCore.h"

#if defined (__GNUC__)
#define ALIGN_EFI_PAGE __attribute__((aligned(EFI_PAGE_SIZE)))
#elif defined (_MSC_VER)
#define ALIGN_EFI_PAGE __declspec(align(EFI_PAGE_SIZE))
#elif defined (__SUNPRO_C)
#define ALIGN_EFI_PAGE
#pragma align EFI_PAGE_SIZE(one,two80)
#else
/* not fatal, might waste a bit of memory */
#define ALIGN_EFI_PAGE
#endif

ALIGN_EFI_PAGE STATIC UINT8 FreePoolOnHeap[FixedPcdGet32 (PcdRuntimeMemoryOnHeapSize)];

LIST_ENTRY mMmPoolLists[MAX_POOL_INDEX];
//
// To cache the MMRAM base since when Loading modules At fixed address feature is enabled,
Expand Down Expand Up @@ -63,6 +77,15 @@ MmInitializeMemoryServices (
MmramRanges[Index].RegionState
);
}

if (FixedPcdGetBool (PcdRuntimeMemoryOnHeap)) {
MmAddMemoryRegion (
(UINTN)FreePoolOnHeap,
sizeof (FreePoolOnHeap),
EfiConventionalMemory,
EFI_CACHEABLE
);
}
Comment on lines +80 to +88
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@LeviYeoReum LeviYeoReum Sep 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (
.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

/**
Expand Down
2 changes: 2 additions & 0 deletions StandaloneMmPkg/Core/StandaloneMmCore.inf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,8 @@

[Pcd]
gStandaloneMmPkgTokenSpaceGuid.PcdFwVolMmMaxEncapsulationDepth ##CONSUMES
gStandaloneMmPkgTokenSpaceGuid.PcdRuntimeMemoryOnHeap ##CONSUMES
gStandaloneMmPkgTokenSpaceGuid.PcdRuntimeMemoryOnHeapSize ##CONSUMES

#
# This configuration fails for CLANGPDB, which does not support PIE in the GCC
Expand Down
55 changes: 35 additions & 20 deletions StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ CheckBufferAddr (
@retval EFI_UNSUPPORTED Operation not supported.
**/
EFI_STATUS
EFIAPI
PiMmStandaloneMmCpuDriverEntry (
IN UINTN EventId,
IN UINTN CpuNumber,
Expand Down Expand Up @@ -147,24 +148,35 @@ PiMmStandaloneMmCpuDriverEntry (
NsCommBufferSize = ((EFI_MM_COMMUNICATE_HEADER *)NsCommBufferAddr)->MessageLength +
sizeof (EFI_MM_COMMUNICATE_HEADER);

GuidedEventContext = NULL;
// Now that the secure world can see the normal world buffer, allocate
// memory to copy the communication buffer to the secure world.
Status = mMmst->MmAllocatePool (
EfiRuntimeServicesData,
NsCommBufferSize,
(VOID **)&GuidedEventContext
);

if (Status != EFI_SUCCESS) {
DEBUG ((DEBUG_ERROR, "Mem alloc failed - 0x%x\n", EventId));
return EFI_OUT_OF_RESOURCES;
if (FixedPcdGetBool (PcdRuntimeMemoryOnHeap)) {
//
// 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.
//
Comment on lines +153 to +158
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

GuidedEventContext = (VOID *)NsCommBufferAddr;
} else {
GuidedEventContext = NULL;
// Now that the secure world can see the normal world buffer, allocate
// memory to copy the communication buffer to the secure world.
Status = mMmst->MmAllocatePool (
EfiRuntimeServicesData,
NsCommBufferSize,
(VOID **)&GuidedEventContext
);

if (Status != EFI_SUCCESS) {
DEBUG ((DEBUG_ERROR, "Mem alloc failed - 0x%x\n", EventId));
return EFI_OUT_OF_RESOURCES;
}

// X1 contains the VA of the normal world memory accessible from
// secure world.
CopyMem (GuidedEventContext, (CONST VOID *)NsCommBufferAddr, NsCommBufferSize);
}

// X1 contains the VA of the normal world memory accessible from
// secure world.
CopyMem (GuidedEventContext, (CONST VOID *)NsCommBufferAddr, NsCommBufferSize);

// Stash the pointer to the allocated Event Context for this CPU
PerCpuGuidedEventContext[CpuNumber] = GuidedEventContext;

Expand All @@ -188,11 +200,14 @@ PiMmStandaloneMmCpuDriverEntry (

// Free the memory allocation done earlier and reset the per-cpu context
ASSERT (GuidedEventContext);
CopyMem ((VOID *)NsCommBufferAddr, (CONST VOID *)GuidedEventContext, NsCommBufferSize);

Status = mMmst->MmFreePool ((VOID *)GuidedEventContext);
if (Status != EFI_SUCCESS) {
return EFI_OUT_OF_RESOURCES;
if (!FixedPcdGetBool (PcdRuntimeMemoryOnHeap)) {
CopyMem ((VOID *)NsCommBufferAddr, (CONST VOID *)GuidedEventContext, NsCommBufferSize);

Status = mMmst->MmFreePool ((VOID *)GuidedEventContext);
if (Status != EFI_SUCCESS) {
return EFI_OUT_OF_RESOURCES;
}
}

PerCpuGuidedEventContext[CpuNumber] = NULL;
Expand Down
1 change: 1 addition & 0 deletions StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ GetGuidedHobData (
driver endpoint descriptor.
**/
EFI_STATUS
EFIAPI
StandaloneMmCpuInitialize (
IN EFI_HANDLE ImageHandle, // not actual imagehandle
IN EFI_MM_SYSTEM_TABLE *SystemTable // not actual systemtable
Expand Down
3 changes: 3 additions & 0 deletions StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.inf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@
gEfiMmConfigurationProtocolGuid # PROTOCOL ALWAYS_PRODUCED
gEfiMmCpuProtocolGuid # PROTOCOL ALWAYS_PRODUCED

[Pcd]
gStandaloneMmPkgTokenSpaceGuid.PcdRuntimeMemoryOnHeap ##CONSUMES

[Guids]
gEfiHobListGuid
gEfiMmPeiMmramMemoryReserveGuid
Expand Down
2 changes: 2 additions & 0 deletions StandaloneMmPkg/Include/StandaloneMmCpu.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

typedef
EFI_STATUS
EFIAPI
(*PI_MM_CPU_DRIVER_ENTRYPOINT) (
IN UINTN EventId,
IN UINTN CpuNumber,
Expand Down Expand Up @@ -59,6 +60,7 @@ extern EFI_MM_CONFIGURATION_PROTOCOL mMmConfig;
@retval EFI_OUT_OF_RESOURCES Out of resources.
@retval EFI_UNSUPPORTED Operation not supported.
**/
EFIAPI
EFI_STATUS
PiMmStandaloneMmCpuDriverEntry (
IN UINTN EventId,
Expand Down
9 changes: 9 additions & 0 deletions StandaloneMmPkg/StandaloneMmPkg.dec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,3 +53,12 @@
# in the MM phase. Minimum value is 1. Sections nested more deeply are rejected.
# @Prompt Maximum permitted FwVol section nesting depth (exclusive) in MM.
gStandaloneMmPkgTokenSpaceGuid.PcdFwVolMmMaxEncapsulationDepth|0x10|UINT32|0x00000001
## Use a fixed size memory buffer on the heap as free memory pool.
# Using this option will increase the binary size, but removes the callers need
# to provide a memory range in the 'secure world' that can be used for runtime
# memory allocations.
# You need at least PcdVariableStoreSize + MAX(PcdMaxVolatileVariableSize, PcdMaxVariableSize,
# PcdMaxAuthVariableSize, PcdMaxHardwareErrorVariableSize) + NvVariableLength of memory.

gStandaloneMmPkgTokenSpaceGuid.PcdRuntimeMemoryOnHeap|FALSE|BOOLEAN|0x00000002
gStandaloneMmPkgTokenSpaceGuid.PcdRuntimeMemoryOnHeapSize|0x100000|UINT32|0x00000003
Loading