Re: [edk2-devel] [PATCH v1 2/2] StandaloneMmPkg: Fix HOB space and heap space conflicted issue

[Nhi Pham via groups.io]

Hi Ard and Ming,

I have been seeing an issue with StandaloneMM HobLib that can be fixed 
by this patch as well.

The function CreateHob() in the HobLib instance 
StandaloneMmPkg/Library/StandaloneMmCoreHobLib/StandaloneMmCoreHobLib.inf 
does not work at all. The HobList is early created by the 
StandaloneMmCoreEntryPoint then it is relocated on the heap memory by 
StandaloneMmCore. But the FreeMemoryTop and FreeMemoryBottom are not 
updated accordingly and the HOB free memory top is overlapped with the 
heap space. This causes the CreateHob() function to not work as 
expected. Introducing the PcdMemoryHobSize is reasonable to fix this issue.

I tested this patch in my end.

Tested-by: Nhi Pham <n...@os.amperecomputing.com>

Thanks,

-Nhi

On 5/12/2022 5:09 PM, Ming Huang via groups.io wrote:

在 5/3/22 5:10 PM, Ard Biesheuvel 写道:
On Wed, 9 Feb 2022 at 13:26, Ming Huang <huangm...@linux.alibaba.com> wrote:
The heap space will be rewrote if a StandloneMmPkg module create HOB
by BuildGuidHob() interface and write data to HOB space.
Can you elaborate? What is supposed to happen and why, and what is
happening instead?
I tried my best to explain the issue:

-----------------------------<--HandOffHob->EfiFreeMemoryTop
|                           |
|                           |
|                           |
|                           |
|                           |
|                           |
|          mMmMemoryMap     |
|---------------------------|<--HandOffHob->EfiFreeMemoryBottom
|          HobEnd           |
|---------------------------|<--HandOffHob->EfiEndOfHobList
|                           |
|          Hob #1           |
-----------------------------<--MmHobStart
1 The mMmMemoryMap which use for free page is on above the HobEnd.
2 Create a hob by BuildGuidHob(), the HobEnd will move up and cover
   the structure or list using by free page.

After this patch, there is a pre-allocation space for creating hob.

-----------------------------<--HandOffHob->EfiFreeMemoryTop
|                           |
|                           |
|                           |
|                           |
|         mMmMemoryMap      |
|---------------------------|<--HandOffHob->EfiFreeMemoryBottom
|          Hob free space   | by PcdMemoryHobSize
|---------------------------|
|          HobEnd           |
|---------------------------|<--HandOffHob->EfiEndOfHobList
|                           |
|          Hob #1           |
-----------------------------<--MmHobStart

Add a PCD PcdMemoryHobSize for pre-allocation a space to create HOB to
fix this issue.

Signed-off-by: Ming Huang <huangm...@linux.alibaba.com>
---
  StandaloneMmPkg/Core/StandaloneMmCore.c   | 17 ++++++++++++++++-
  StandaloneMmPkg/Core/StandaloneMmCore.inf |  3 +++
  StandaloneMmPkg/StandaloneMmPkg.dec       |  2 ++
  3 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.c 
b/StandaloneMmPkg/Core/StandaloneMmCore.c
index d221f1d111..1cf259d946 100644
--- a/StandaloneMmPkg/Core/StandaloneMmCore.c
+++ b/StandaloneMmPkg/Core/StandaloneMmCore.c
@@ -512,6 +512,9 @@ StandaloneMmMain (
    EFI_MMRAM_DESCRIPTOR            *MmramRanges;
    UINTN                           MmramRangeCount;
    EFI_HOB_FIRMWARE_VOLUME         *BfvHob;
+  EFI_HOB_HANDOFF_INFO_TABLE      *HandOffHobNew;
+  EFI_HOB_HANDOFF_INFO_TABLE      *HandOffHobOrg;
+  UINT64                          MaxHobSize = PcdGet64 (PcdMemoryHobSize);

    ProcessLibraryConstructorList (HobStart, &gMmCoreMmst);

@@ -619,10 +622,22 @@ StandaloneMmMain (
    //
    HobSize = GetHobListSize (HobStart);
    DEBUG ((DEBUG_INFO, "HobSize - 0x%x\n", HobSize));
-  MmHobStart = AllocatePool (HobSize);
+  ASSERT (HobSize <= MaxHobSize);
+  MmHobStart = AllocatePool (MaxHobSize);
    DEBUG ((DEBUG_INFO, "MmHobStart - 0x%x\n", MmHobStart));
    ASSERT (MmHobStart != NULL);
    CopyMem (MmHobStart, HobStart, HobSize);
+  //
+  // Initlialize the new HOB table
+  //
+  HandOffHobOrg = (EFI_HOB_HANDOFF_INFO_TABLE *)HobStart;
+  HandOffHobNew = (EFI_HOB_HANDOFF_INFO_TABLE *)MmHobStart;
+  HandOffHobNew->EfiEndOfHobList = (EFI_PHYSICAL_ADDRESS)MmHobStart +
+    (HandOffHobOrg->EfiEndOfHobList - (EFI_PHYSICAL_ADDRESS)HobStart);
+  HandOffHobNew->EfiFreeMemoryBottom = HandOffHobNew->EfiEndOfHobList +
+                                       sizeof (EFI_HOB_GENERIC_HEADER);
+  HandOffHobNew->EfiFreeMemoryTop = (EFI_PHYSICAL_ADDRESS)MmHobStart + 
MaxHobSize;
+
    Status = MmInstallConfigurationTable (&gMmCoreMmst, &gEfiHobListGuid, 
MmHobStart, HobSize);
    ASSERT_EFI_ERROR (Status);

diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.inf 
b/StandaloneMmPkg/Core/StandaloneMmCore.inf
index c44b9ff333..37e6135d73 100644
--- a/StandaloneMmPkg/Core/StandaloneMmCore.inf
+++ b/StandaloneMmPkg/Core/StandaloneMmCore.inf
@@ -76,6 +76,9 @@
    gEfiEventExitBootServicesGuid
    gEfiEventReadyToBootGuid

+[FixedPcd]
+  gStandaloneMmPkgTokenSpaceGuid.PcdMemoryHobSize
+
  #
  # This configuration fails for CLANGPDB, which does not support PIE in the GCC
  # sense. Such however is required for ARM family StandaloneMmCore
diff --git a/StandaloneMmPkg/StandaloneMmPkg.dec 
b/StandaloneMmPkg/StandaloneMmPkg.dec
index 46784d94e4..cf554676e2 100644
--- a/StandaloneMmPkg/StandaloneMmPkg.dec
+++ b/StandaloneMmPkg/StandaloneMmPkg.dec
@@ -48,3 +48,5 @@
    gEfiStandaloneMmNonSecureBufferGuid      = { 0xf00497e3, 0xbfa2, 0x41a1, { 
0x9d, 0x29, 0x54, 0xc2, 0xe9, 0x37, 0x21, 0xc5 }}
    gEfiArmTfCpuDriverEpDescriptorGuid       = { 0x6ecbd5a1, 0xc0f8, 0x4702, { 
0x83, 0x01, 0x4f, 0xc2, 0xc5, 0x47, 0x0a, 0x51 }}

+[PcdsFixedAtBuild]
+  gStandaloneMmPkgTokenSpaceGuid.PcdMemoryHobSize|0x00000000|UINT64|0x00000004
--
2.17.1










-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107793): https://edk2.groups.io/g/devel/message/107793
Mute This Topic: https://groups.io/mt/89020085/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-