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

["Nhi Pham via groups.io" <nhi=os.amperecomputing.com@groups.io>]

Just a gentle ping!

Hi Ard, can you provide your feedback on this patch?

Thanks,

-Nhi

On 8/16/2023 3:55 PM, Nhi Pham via groups.io wrote:
> 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 <nhi@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 
>>> <huangming@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 <huangming@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 (#108075): https://edk2.groups.io/g/devel/message/108075
Mute This Topic: https://groups.io/mt/89020085/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-