The heap space will be rewrote if a StandloneMmPkg module create HOB
by BuildGuidHob() interface and write data to HOB space.
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 (#86527): https://edk2.groups.io/g/devel/message/86527
Mute This Topic: https://groups.io/mt/89020085/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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?

> 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 (#89479): https://edk2.groups.io/g/devel/message/89479
Mute This Topic: https://groups.io/mt/89020085/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

在 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 (#89701): https://edk2.groups.io/g/devel/message/89701
Mute This Topic: https://groups.io/mt/89020085/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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 (#107793): https://edk2.groups.io/g/devel/message/107793
Mute This Topic: https://groups.io/mt/89020085/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Wed, 16 Aug 2023 at 10:56, Nhi Pham <nhi@amperemail.onmicrosoft.com> 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 for reminding me.

So if the HOB creation is completely broken, are we sure this is the
correct fix? Wouldn't it be better to update FreeMemoryTop and
FreeMemoryBottom to the correct values?


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108145): https://edk2.groups.io/g/devel/message/108145
Mute This Topic: https://groups.io/mt/89020085/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Ard,

Thanks for your response on this patch. Please see my reply inline...

On 8/30/2023 8:10 PM, Ard Biesheuvel wrote:
> [EXTERNAL EMAIL NOTICE: This email originated from an external sender. Please be mindful of safe email handling and proprietary information protection practices.]
>
>
> On Wed, 16 Aug 2023 at 10:56, Nhi Pham <nhi@amperemail.onmicrosoft.com> 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 for reminding me.
>
> So if the HOB creation is completely broken, are we sure this is the
> correct fix? Wouldn't it be better to update FreeMemoryTop and
> FreeMemoryBottom to the correct values?

Per your question, I had a deep dive into the HOB today. I think I need 
to clarify further the issue to make sure that we are on the same page.

1. When the base of the HOB list (HobStart) is reallocated in the MM 
heap memory, the Hob->EfiEndOfHobList must be adjusted accordingly 
compared to the new HOB base. Currently, we are missing that. That 
causes the CreateHob() function does not work. The GetNextHob() function 
always look up the old HOB list instead of the new HOB list.

2. The Memory Allocation StandaloneMmPkg/Core/Page.c does not update the 
Hob->EfiFreeMemoryTop, this may cause the MM heap space and HOB space to 
be overlapped at some points.

It sounds like the issue on the memory allocation in StandaloneMM. For 
#1, I think we should write a new patch for it. For #2, could you advise?

Thanks,

-Nhi



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108185): https://edk2.groups.io/g/devel/message/108185
Mute This Topic: https://groups.io/mt/89020085/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 8/31/2023 1:20 AM, Nhi Pham via groups.io wrote:
> Hi Ard,
> 
> Thanks for your response on this patch. Please see my reply inline...
> 
> On 8/30/2023 8:10 PM, Ard Biesheuvel wrote:
>> [EXTERNAL EMAIL NOTICE: This email originated from an external sender. 
>> Please be mindful of safe email handling and proprietary information 
>> protection practices.]
>>
>>
>> On Wed, 16 Aug 2023 at 10:56, Nhi Pham 
>> <nhi@amperemail.onmicrosoft.com> 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 for reminding me.
>>
>> So if the HOB creation is completely broken, are we sure this is the
>> correct fix? Wouldn't it be better to update FreeMemoryTop and
>> FreeMemoryBottom to the correct values?
> 
> Per your question, I had a deep dive into the HOB today. I think I need 
> to clarify further the issue to make sure that we are on the same page.
> 
> 1. When the base of the HOB list (HobStart) is reallocated in the MM 
> heap memory, the Hob->EfiEndOfHobList must be adjusted accordingly 
> compared to the new HOB base. Currently, we are missing that. That 
> causes the CreateHob() function does not work. The GetNextHob() function 
> always look up the old HOB list instead of the new HOB list.
> 
> 2. The Memory Allocation StandaloneMmPkg/Core/Page.c does not update the 
> Hob->EfiFreeMemoryTop, this may cause the MM heap space and HOB space to 
> be overlapped at some points.
> 
> It sounds like the issue on the memory allocation in StandaloneMM. For 
> #1, I think we should write a new patch for it. For #2, could you advise?
> 

If I am understanding this correctly, this is only an issue when
HOBs are created in StMM, i.e. not from HOBs that are passed in. Is this
correct?

If so, is HOB creation in StMM and supported use case? The only instance
a quick search turns up is the ARM StMM Core entry, where some
information from TF-A is converted to HOB format. Do we have any other
use cases (and curious more on this use case). My thought process would
be that StMM would not create any HOBs. Depending on FW configuration,
it may receive HOBs from PEI.

In PEI/DXE to StMM communication, we should be using the
MM_Communicate interface.

But in StMM to StMM communication, I would expect we would go through
interfaces like protocols, PCDs, or UEFI variables. Any reason for HOBs
that I'm not thinking of? I'm wondering if the answer here is to
address what seemingly is one (or a very small set) of use cases and
remove the HOB creation code in StMM altogether.

Thanks,
Oliver


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108232): https://edk2.groups.io/g/devel/message/108232
Mute This Topic: https://groups.io/mt/89020085/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Oliver, thank you for taking a look at this patch.

On 9/2/2023 3:43 AM, Oliver Smith-Denny wrote:
> On 8/31/2023 1:20 AM, Nhi Pham via groups.io wrote:
>> Hi Ard,
>>
>> Thanks for your response on this patch. Please see my reply inline...
>>
>> On 8/30/2023 8:10 PM, Ard Biesheuvel wrote:
>>> [EXTERNAL EMAIL NOTICE: This email originated from an external 
>>> sender. Please be mindful of safe email handling and proprietary 
>>> information protection practices.]
>>>
>>>
>>> On Wed, 16 Aug 2023 at 10:56, Nhi Pham 
>>> <nhi@amperemail.onmicrosoft.com> 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 for reminding me.
>>>
>>> So if the HOB creation is completely broken, are we sure this is the
>>> correct fix? Wouldn't it be better to update FreeMemoryTop and
>>> FreeMemoryBottom to the correct values?
>>
>> Per your question, I had a deep dive into the HOB today. I think I 
>> need to clarify further the issue to make sure that we are on the 
>> same page.
>>
>> 1. When the base of the HOB list (HobStart) is reallocated in the MM 
>> heap memory, the Hob->EfiEndOfHobList must be adjusted accordingly 
>> compared to the new HOB base. Currently, we are missing that. That 
>> causes the CreateHob() function does not work. The GetNextHob() 
>> function always look up the old HOB list instead of the new HOB list.
>>
>> 2. The Memory Allocation StandaloneMmPkg/Core/Page.c does not update 
>> the Hob->EfiFreeMemoryTop, this may cause the MM heap space and HOB 
>> space to be overlapped at some points.
>>
>> It sounds like the issue on the memory allocation in StandaloneMM. 
>> For #1, I think we should write a new patch for it. For #2, could you 
>> advise?
>>
>
> If I am understanding this correctly, this is only an issue when
> HOBs are created in StMM, i.e. not from HOBs that are passed in. Is this
> correct?
Yes, the issue only occurs when HOB are created in StandaloneMM by the 
HOB library instance 
StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.inf
>
> If so, is HOB creation in StMM and supported use case? The only instance
I think it is intended to work as the CreateHob() function is implemented.
> a quick search turns up is the ARM StMM Core entry, where some
> information from TF-A is converted to HOB format. Do we have any other
> use cases (and curious more on this use case). My thought process would
> be that StMM would not create any HOBs. Depending on FW configuration,
> it may receive HOBs from PEI.

I have a use case when enabling the UEFI Variable driver running in 
StandaloneMM. Instead of using the PCDs, the in-memory NVRAM region is 
allocated **dynamically** at boot time in the StMM secure memory. Then, 
they will be passed into the gVariableFlashInfoHobGuid for being 
consumed by other variable MM drivers.

>
> In PEI/DXE to StMM communication, we should be using the
> MM_Communicate interface.
>
> But in StMM to StMM communication, I would expect we would go through
> interfaces like protocols, PCDs, or UEFI variables. Any reason for HOBs
> that I'm not thinking of? I'm wondering if the answer here is to
> address what seemingly is one (or a very small set) of use cases and
> remove the HOB creation code in StMM altogether.
>
> Thanks,
> Oliver


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108269): https://edk2.groups.io/g/devel/message/108269
Mute This Topic: https://groups.io/mt/89020085/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 9/4/2023 7:20 PM, Nhi Pham wrote:
> On 9/2/2023 3:43 AM, Oliver Smith-Denny wrote:
>> On 8/31/2023 1:20 AM, Nhi Pham via groups.io wrote:
>>
>> If I am understanding this correctly, this is only an issue when
>> HOBs are created in StMM, i.e. not from HOBs that are passed in. Is this
>> correct?
> Yes, the issue only occurs when HOB are created in StandaloneMM by the 
> HOB library instance 
> StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.inf
>>
>> If so, is HOB creation in StMM and supported use case? The only instance
> I think it is intended to work as the CreateHob() function is implemented.

Well, that may just be a copy/paste sort of thing :).

>> a quick search turns up is the ARM StMM Core entry, where some
>> information from TF-A is converted to HOB format. Do we have any other
>> use cases (and curious more on this use case). My thought process would
>> be that StMM would not create any HOBs. Depending on FW configuration,
>> it may receive HOBs from PEI.
> 
> I have a use case when enabling the UEFI Variable driver running in 
> StandaloneMM. Instead of using the PCDs, the in-memory NVRAM region is 
> allocated **dynamically** at boot time in the StMM secure memory. Then, 
> they will be passed into the gVariableFlashInfoHobGuid for being 
> consumed by other variable MM drivers.
> 

I do believe that per the PI spec, we should have HOB producer and HOB
consumer phases, where in this case PEI (if it was the launching entity
for StMM) is the HOB producer and StMM is the HOB producer. This is the
same pattern the PI spec details for PEI and DXE, where DXE is not
intended to create new HOBs, but just to consume information from the
previous phase.

As I mentioned, there are other interfaces for passing information
within a phase, such as protocols, dynamic PCDs, variables, etc. that
are built for this application. I think it is useful to adhere to the
model for HOBs (which are hand off blocks, one phase handing information
to another phase) and that we will create more issues if we rely on
HOB consumer phases producing HOBs.

My proposal would be to remove the HOB creation code from StMM
completely. I believe in your use case that you are describing a dynamic
PCD or a protocol could work to pass the information.

If we are saying that prior to your patch that HOB creation in StMM was
completely broken, anyway, it seems that folks were not relying on this
code?

Thanks,
Oliver


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108293): https://edk2.groups.io/g/devel/message/108293
Mute This Topic: https://groups.io/mt/89020085/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 9/6/2023 4:29 AM, Oliver Smith-Denny wrote:

> On 9/4/2023 7:20 PM, Nhi Pham wrote:
>> On 9/2/2023 3:43 AM, Oliver Smith-Denny wrote:
>>> On 8/31/2023 1:20 AM, Nhi Pham via groups.io wrote:
>>>
>>> If I am understanding this correctly, this is only an issue when
>>> HOBs are created in StMM, i.e. not from HOBs that are passed in. Is 
>>> this
>>> correct?
>> Yes, the issue only occurs when HOB are created in StandaloneMM by 
>> the HOB library instance 
>> StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.inf
>>>
>>> If so, is HOB creation in StMM and supported use case? The only 
>>> instance
>> I think it is intended to work as the CreateHob() function is 
>> implemented.
>
> Well, that may just be a copy/paste sort of thing :).
>
>>> a quick search turns up is the ARM StMM Core entry, where some
>>> information from TF-A is converted to HOB format. Do we have any other
>>> use cases (and curious more on this use case). My thought process would
>>> be that StMM would not create any HOBs. Depending on FW configuration,
>>> it may receive HOBs from PEI.
>>
>> I have a use case when enabling the UEFI Variable driver running in 
>> StandaloneMM. Instead of using the PCDs, the in-memory NVRAM region 
>> is allocated **dynamically** at boot time in the StMM secure memory. 
>> Then, they will be passed into the gVariableFlashInfoHobGuid for 
>> being consumed by other variable MM drivers.
>>
>
> I do believe that per the PI spec, we should have HOB producer and HOB
> consumer phases, where in this case PEI (if it was the launching entity
> for StMM) is the HOB producer and StMM is the HOB producer. This is the
> same pattern the PI spec details for PEI and DXE, where DXE is not
> intended to create new HOBs, but just to consume information from the
> previous phase.
>
> As I mentioned, there are other interfaces for passing information
> within a phase, such as protocols, dynamic PCDs, variables, etc. that
> are built for this application. I think it is useful to adhere to the
> model for HOBs (which are hand off blocks, one phase handing information
> to another phase) and that we will create more issues if we rely on
> HOB consumer phases producing HOBs.
Thanks Oliver for the explanation. That makes sense to me.
>
> My proposal would be to remove the HOB creation code from StMM
> completely. I believe in your use case that you are describing a dynamic
> PCD or a protocol could work to pass the information.
I think the dynamic PCD is supposed to not being supported in 
StandaloneMM and protocol does not fit because the Variable Flash Info 
is created in PEI for UEFI variable non-MM flow and in StMM for UEFI 
variable MM flow.
>
> If we are saying that prior to your patch that HOB creation in StMM was
> completely broken, anyway, it seems that folks were not relying on this
> code?

Right, it is just my curiosity that I don't see any showcase for 
Variable MM + Variable Flash Info HOB in StandaloneMM.

Adding Michael Kubacki as the owner of the Variable Flash Info HOB for 
getting further input.

>
> Thanks,
> Oliver


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108310): https://edk2.groups.io/g/devel/message/108310
Mute This Topic: https://groups.io/mt/89020085/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

I am a bit confused.

The HOB list in standalone MM is read-only. Why could any module call BuildGuidHob() to modify the HOB.

I saw Oliver mentioned something about StMM. I don't know what that is. But it seems that's ARM specific. Then, I don't think it's proper to modify code here for a specific arch ARM.

Thanks,
Ray
________________________________
From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Oliver Smith-Denny <osde@linux.microsoft.com>
Sent: Wednesday, September 6, 2023 5:29 AM
To: Nhi Pham <nhi@amperemail.onmicrosoft.com>; devel@edk2.groups.io <devel@edk2.groups.io>; nhi@os.amperecomputing.com <nhi@os.amperecomputing.com>; Ard Biesheuvel <ardb@kernel.org>
Cc: huangming@linux.alibaba.com <huangming@linux.alibaba.com>; Sami Mujawar <sami.mujawar@arm.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Yao, Jiewen <jiewen.yao@intel.com>; Supreeth Venkatesh <supreeth.venkatesh@arm.com>; ming.huang-@outlook.com <ming.huang-@outlook.com>
Subject: Re: [edk2-devel] [PATCH v1 2/2] StandaloneMmPkg: Fix HOB space and heap space conflicted issue

On 9/4/2023 7:20 PM, Nhi Pham wrote:
> On 9/2/2023 3:43 AM, Oliver Smith-Denny wrote:
>> On 8/31/2023 1:20 AM, Nhi Pham via groups.io wrote:
>>
>> If I am understanding this correctly, this is only an issue when
>> HOBs are created in StMM, i.e. not from HOBs that are passed in. Is this
>> correct?
> Yes, the issue only occurs when HOB are created in StandaloneMM by the
> HOB library instance
> StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.inf
>>
>> If so, is HOB creation in StMM and supported use case? The only instance
> I think it is intended to work as the CreateHob() function is implemented.

Well, that may just be a copy/paste sort of thing :).

>> a quick search turns up is the ARM StMM Core entry, where some
>> information from TF-A is converted to HOB format. Do we have any other
>> use cases (and curious more on this use case). My thought process would
>> be that StMM would not create any HOBs. Depending on FW configuration,
>> it may receive HOBs from PEI.
>
> I have a use case when enabling the UEFI Variable driver running in
> StandaloneMM. Instead of using the PCDs, the in-memory NVRAM region is
> allocated **dynamically** at boot time in the StMM secure memory. Then,
> they will be passed into the gVariableFlashInfoHobGuid for being
> consumed by other variable MM drivers.
>

I do believe that per the PI spec, we should have HOB producer and HOB
consumer phases, where in this case PEI (if it was the launching entity
for StMM) is the HOB producer and StMM is the HOB producer. This is the
same pattern the PI spec details for PEI and DXE, where DXE is not
intended to create new HOBs, but just to consume information from the
previous phase.

As I mentioned, there are other interfaces for passing information
within a phase, such as protocols, dynamic PCDs, variables, etc. that
are built for this application. I think it is useful to adhere to the
model for HOBs (which are hand off blocks, one phase handing information
to another phase) and that we will create more issues if we rely on
HOB consumer phases producing HOBs.

My proposal would be to remove the HOB creation code from StMM
completely. I believe in your use case that you are describing a dynamic
PCD or a protocol could work to pass the information.

If we are saying that prior to your patch that HOB creation in StMM was
completely broken, anyway, it seems that folks were not relying on this
code?

Thanks,
Oliver







-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108306): https://edk2.groups.io/g/devel/message/108306
Mute This Topic: https://groups.io/mt/89020085/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 9/6/2023 1:33 PM, Ni, Ray wrote:
> [EXTERNAL EMAIL NOTICE: This email originated from an external sender. 
> Please be mindful of safe email handling and proprietary information 
> protection practices.]
> 
> I am a bit confused.
> 
> The HOB list in standalone MM is read-only. Why could any module call 
> BuildGuidHob() to modify the HOB.
> 
> I saw Oliver mentioned something about StMM. I don't know what that is. 
> But it seems that's ARM specific. Then, I don't think it's proper to 
> modify code here for a specific arch ARM.

The HOB creation is available in the 
StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.inf. If 
other architectures also use that instance, I think the issue is not 
specific to ARM.

Regards,
-Nhi
> 
> Thanks,
> Ray
> ------------------------------------------------------------------------
> *From:* devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Oliver 
> Smith-Denny <osde@linux.microsoft.com>
> *Sent:* Wednesday, September 6, 2023 5:29 AM
> *To:* Nhi Pham <nhi@amperemail.onmicrosoft.com>; devel@edk2.groups.io 
> <devel@edk2.groups.io>; nhi@os.amperecomputing.com 
> <nhi@os.amperecomputing.com>; Ard Biesheuvel <ardb@kernel.org>
> *Cc:* huangming@linux.alibaba.com <huangming@linux.alibaba.com>; Sami 
> Mujawar <sami.mujawar@arm.com>; Ard Biesheuvel 
> <ardb+tianocore@kernel.org>; Yao, Jiewen <jiewen.yao@intel.com>; 
> Supreeth Venkatesh <supreeth.venkatesh@arm.com>; ming.huang-@outlook.com 
> <ming.huang-@outlook.com>
> *Subject:* Re: [edk2-devel] [PATCH v1 2/2] StandaloneMmPkg: Fix HOB 
> space and heap space conflicted issue
> On 9/4/2023 7:20 PM, Nhi Pham wrote:
>> On 9/2/2023 3:43 AM, Oliver Smith-Denny wrote:
>>> On 8/31/2023 1:20 AM, Nhi Pham via groups.io wrote:
>>>
>>> If I am understanding this correctly, this is only an issue when
>>> HOBs are created in StMM, i.e. not from HOBs that are passed in. Is this
>>> correct?
>> Yes, the issue only occurs when HOB are created in StandaloneMM by the 
>> HOB library instance 
>> StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.inf
>>>
>>> If so, is HOB creation in StMM and supported use case? The only instance
>> I think it is intended to work as the CreateHob() function is implemented.
> 
> Well, that may just be a copy/paste sort of thing :).
> 
>>> a quick search turns up is the ARM StMM Core entry, where some
>>> information from TF-A is converted to HOB format. Do we have any other
>>> use cases (and curious more on this use case). My thought process would
>>> be that StMM would not create any HOBs. Depending on FW configuration,
>>> it may receive HOBs from PEI.
>> 
>> I have a use case when enabling the UEFI Variable driver running in 
>> StandaloneMM. Instead of using the PCDs, the in-memory NVRAM region is 
>> allocated **dynamically** at boot time in the StMM secure memory. Then, 
>> they will be passed into the gVariableFlashInfoHobGuid for being 
>> consumed by other variable MM drivers.
>> 
> 
> I do believe that per the PI spec, we should have HOB producer and HOB
> consumer phases, where in this case PEI (if it was the launching entity
> for StMM) is the HOB producer and StMM is the HOB producer. This is the
> same pattern the PI spec details for PEI and DXE, where DXE is not
> intended to create new HOBs, but just to consume information from the
> previous phase.
> 
> As I mentioned, there are other interfaces for passing information
> within a phase, such as protocols, dynamic PCDs, variables, etc. that
> are built for this application. I think it is useful to adhere to the
> model for HOBs (which are hand off blocks, one phase handing information
> to another phase) and that we will create more issues if we rely on
> HOB consumer phases producing HOBs.
> 
> My proposal would be to remove the HOB creation code from StMM
> completely. I believe in your use case that you are describing a dynamic
> PCD or a protocol could work to pass the information.
> 
> If we are saying that prior to your patch that HOB creation in StMM was
> completely broken, anyway, it seems that folks were not relying on this
> code?
> 
> Thanks,
> Oliver
> 
> 
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108311): https://edk2.groups.io/g/devel/message/108311
Mute This Topic: https://groups.io/mt/89020085/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Wed, 6 Sept 2023 at 09:56, Nhi Pham <nhi@amperemail.onmicrosoft.com> wrote:
>
> On 9/6/2023 1:33 PM, Ni, Ray wrote:
> > [EXTERNAL EMAIL NOTICE: This email originated from an external sender.
> > Please be mindful of safe email handling and proprietary information
> > protection practices.]
> >
> > I am a bit confused.
> >
> > The HOB list in standalone MM is read-only. Why could any module call
> > BuildGuidHob() to modify the HOB.
> >
> > I saw Oliver mentioned something about StMM. I don't know what that is.
> > But it seems that's ARM specific. Then, I don't think it's proper to
> > modify code here for a specific arch ARM.
>
> The HOB creation is available in the
> StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.inf. If
> other architectures also use that instance, I think the issue is not
> specific to ARM.
>

The question here is whether the implementation follows the PI spec,
and whether HOB creation should be supported to begin with.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108315): https://edk2.groups.io/g/devel/message/108315
Mute This Topic: https://groups.io/mt/89020085/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 9/6/2023 1:50 AM, Ard Biesheuvel wrote:
> On Wed, 6 Sept 2023 at 09:56, Nhi Pham <nhi@amperemail.onmicrosoft.com> wrote:
>>
>> On 9/6/2023 1:33 PM, Ni, Ray wrote:
>>> [EXTERNAL EMAIL NOTICE: This email originated from an external sender.
>>> Please be mindful of safe email handling and proprietary information
>>> protection practices.]
>>>
>>> I am a bit confused.
>>>
>>> The HOB list in standalone MM is read-only. Why could any module call
>>> BuildGuidHob() to modify the HOB.
>>>
>>> I saw Oliver mentioned something about StMM. I don't know what that is.
>>> But it seems that's ARM specific. Then, I don't think it's proper to
>>> modify code here for a specific arch ARM.
>>
>> The HOB creation is available in the
>> StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.inf. If
>> other architectures also use that instance, I think the issue is not
>> specific to ARM.
>>
> 
> The question here is whether the implementation follows the PI spec,
> and whether HOB creation should be supported to begin with.
> 

My reading of the PI spec is that this implementation does not follow
it. However, the PI spec is not very explicit about Standalone MM in
general, but particularly in relation to HOBs.

However, in the generic HOB section of PI spec v1.7, Vol. 3, section 4
(entitled HOB Design Discussion) it explicitly lays out that there are
HOB producer phases and HOB consumer phases. It uses PEI as a HOB
producer phase and DXE as a HOB consumer phase and explicitly says
that the HOB consumer phase must treat HOBs as read-only memory, per
Ray's comment.

In vol. 4, section 2.2, in discussing the Standalone MM entry point,
the document talks about the HOB list being passed to Standalone MM
to consume, which per the reading of the above section would classify
Standalone MM as a HOB consumer phase, where HOBs should then be
read-only.

So, I believe that we should not support HOB creation in Standalone MM
and instead rely on other mechanisms to pass information within the
phase. Per Nhi's other email in this thread, we should have the
discussion on how to solve that specific problem and that may well
lead to a discussion on whether HOBs are in fact the right mechanism
here, but I tend to lean towards leaving something as architectural as
HOBs to what the PI spec defines and using different mechanisms to
accomplish in-phase communication.

Does this reading of the spec align with others' expectations? As I
mentioned to Ray in another thread, Standalone MM feels like it could
have extra clarification in a few areas in the PI spec.

Thanks,
Oliver


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108333): https://edk2.groups.io/g/devel/message/108333
Mute This Topic: https://groups.io/mt/89020085/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 9/6/2023 11:22 PM, Oliver Smith-Denny wrote:
> On 9/6/2023 1:50 AM, Ard Biesheuvel wrote:
>> On Wed, 6 Sept 2023 at 09:56, Nhi Pham 
>> <nhi@amperemail.onmicrosoft.com> wrote:
>>>
>>> On 9/6/2023 1:33 PM, Ni, Ray wrote:
>>>> [EXTERNAL EMAIL NOTICE: This email originated from an external sender.
>>>> Please be mindful of safe email handling and proprietary information
>>>> protection practices.]
>>>>
>>>> I am a bit confused.
>>>>
>>>> The HOB list in standalone MM is read-only. Why could any module call
>>>> BuildGuidHob() to modify the HOB.
>>>>
>>>> I saw Oliver mentioned something about StMM. I don't know what that 
>>>> is.
>>>> But it seems that's ARM specific. Then, I don't think it's proper to
>>>> modify code here for a specific arch ARM.
>>>
>>> The HOB creation is available in the
>>> StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.inf. If
>>> other architectures also use that instance, I think the issue is not
>>> specific to ARM.
>>>
>>
>> The question here is whether the implementation follows the PI spec,
>> and whether HOB creation should be supported to begin with.
>>
>
> My reading of the PI spec is that this implementation does not follow
> it. However, the PI spec is not very explicit about Standalone MM in
> general, but particularly in relation to HOBs.
>
> However, in the generic HOB section of PI spec v1.7, Vol. 3, section 4
> (entitled HOB Design Discussion) it explicitly lays out that there are
> HOB producer phases and HOB consumer phases. It uses PEI as a HOB
> producer phase and DXE as a HOB consumer phase and explicitly says
> that the HOB consumer phase must treat HOBs as read-only memory, per
> Ray's comment.
>
> In vol. 4, section 2.2, in discussing the Standalone MM entry point,
> the document talks about the HOB list being passed to Standalone MM
> to consume, which per the reading of the above section would classify
> Standalone MM as a HOB consumer phase, where HOBs should then be
> read-only.
>
> So, I believe that we should not support HOB creation in Standalone MM
> and instead rely on other mechanisms to pass information within the
> phase. Per Nhi's other email in this thread, we should have the
> discussion on how to solve that specific problem and that may well
> lead to a discussion on whether HOBs are in fact the right mechanism
> here, but I tend to lean towards leaving something as architectural as
> HOBs to what the PI spec defines and using different mechanisms to
> accomplish in-phase communication.
Thanks Oliver so much for that. I agree. We should focus on my specific 
problem with UEFI Variable Flash Info in StandaloneMM in another thread.
>
> Does this reading of the spec align with others' expectations? As I
> mentioned to Ray in another thread, Standalone MM feels like it could
> have extra clarification in a few areas in the PI spec.

Thanks again. The HOB library should be updated to remove the HOB 
creation once we have the clarification.

Regards,

Nhi

>
> Thanks,
> Oliver


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108403): https://edk2.groups.io/g/devel/message/108403
Mute This Topic: https://groups.io/mt/89020085/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 9/7/2023 10:38 PM, Nhi Pham wrote:
> On 9/6/2023 11:22 PM, Oliver Smith-Denny wrote:
>> On 9/6/2023 1:50 AM, Ard Biesheuvel wrote:
>>> On Wed, 6 Sept 2023 at 09:56, Nhi Pham
>>> <nhi@amperemail.onmicrosoft.com> wrote:
>>>>
>>>> On 9/6/2023 1:33 PM, Ni, Ray wrote:
>>>>> [EXTERNAL EMAIL NOTICE: This email originated from an external sender.
>>>>> Please be mindful of safe email handling and proprietary information
>>>>> protection practices.]
>>>>>
>>>>> I am a bit confused.
>>>>>
>>>>> The HOB list in standalone MM is read-only. Why could any module call
>>>>> BuildGuidHob() to modify the HOB.
>>>>>
>>>>> I saw Oliver mentioned something about StMM. I don't know what that
>>>>> is.
>>>>> But it seems that's ARM specific. Then, I don't think it's proper to
>>>>> modify code here for a specific arch ARM.
>>>>
>>>> The HOB creation is available in the
>>>> StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.inf. If
>>>> other architectures also use that instance, I think the issue is not
>>>> specific to ARM.
>>>>
>>>
>>> The question here is whether the implementation follows the PI spec,
>>> and whether HOB creation should be supported to begin with.
>>>
>>
>> My reading of the PI spec is that this implementation does not follow
>> it. However, the PI spec is not very explicit about Standalone MM in
>> general, but particularly in relation to HOBs.
>>
>> However, in the generic HOB section of PI spec v1.7, Vol. 3, section 4
>> (entitled HOB Design Discussion) it explicitly lays out that there are
>> HOB producer phases and HOB consumer phases. It uses PEI as a HOB
>> producer phase and DXE as a HOB consumer phase and explicitly says
>> that the HOB consumer phase must treat HOBs as read-only memory, per
>> Ray's comment.
>>
>> In vol. 4, section 2.2, in discussing the Standalone MM entry point,
>> the document talks about the HOB list being passed to Standalone MM
>> to consume, which per the reading of the above section would classify
>> Standalone MM as a HOB consumer phase, where HOBs should then be
>> read-only.
>>
>> So, I believe that we should not support HOB creation in Standalone MM
>> and instead rely on other mechanisms to pass information within the
>> phase. Per Nhi's other email in this thread, we should have the
>> discussion on how to solve that specific problem and that may well
>> lead to a discussion on whether HOBs are in fact the right mechanism
>> here, but I tend to lean towards leaving something as architectural as
>> HOBs to what the PI spec defines and using different mechanisms to
>> accomplish in-phase communication.
> Thanks Oliver so much for that. I agree. We should focus on my specific
> problem with UEFI Variable Flash Info in StandaloneMM in another thread.
>>
>> Does this reading of the spec align with others' expectations? As I
>> mentioned to Ray in another thread, Standalone MM feels like it could
>> have extra clarification in a few areas in the PI spec.
> 
> Thanks again. The HOB library should be updated to remove the HOB
> creation once we have the clarification.

I found that we can hook a platform NULL library class to the
StandaloneMmCore to create UEFI Variable Flash Info HOB. This way is
compliant to the spec as the HOB is created in MM_CORE_STANDALONE phase.

Should I write a patch to remove the HOB creation in StandaloneMmHobLib?

Regards,
Nhi



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111903): https://edk2.groups.io/g/devel/message/111903
Mute This Topic: https://groups.io/mt/89020085/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Yes please!

Thanks,
Ray
> -----Original Message-----
> From: Nhi Pham <nhi@os.amperecomputing.com>
> Sent: Thursday, November 30, 2023 9:59 PM
> To: Oliver Smith-Denny <osde@linux.microsoft.com>; devel@edk2.groups.io;
> ardb@kernel.org
> Cc: Ni, Ray <ray.ni@intel.com>; huangming@linux.alibaba.com; Sami Mujawar
> <sami.mujawar@arm.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>;
> Yao, Jiewen <jiewen.yao@intel.com>; Supreeth Venkatesh
> <supreeth.venkatesh@arm.com>; ming.huang-@outlook.com
> Subject: Re: [edk2-devel] [PATCH v1 2/2] StandaloneMmPkg: Fix HOB space
> and heap space conflicted issue
> 
> On 9/7/2023 10:38 PM, Nhi Pham wrote:
> > On 9/6/2023 11:22 PM, Oliver Smith-Denny wrote:
> >> On 9/6/2023 1:50 AM, Ard Biesheuvel wrote:
> >>> On Wed, 6 Sept 2023 at 09:56, Nhi Pham
> >>> <nhi@amperemail.onmicrosoft.com> wrote:
> >>>>
> >>>> On 9/6/2023 1:33 PM, Ni, Ray wrote:
> >>>>> [EXTERNAL EMAIL NOTICE: This email originated from an external
> sender.
> >>>>> Please be mindful of safe email handling and proprietary information
> >>>>> protection practices.]
> >>>>>
> >>>>> I am a bit confused.
> >>>>>
> >>>>> The HOB list in standalone MM is read-only. Why could any module call
> >>>>> BuildGuidHob() to modify the HOB.
> >>>>>
> >>>>> I saw Oliver mentioned something about StMM. I don't know what that
> >>>>> is.
> >>>>> But it seems that's ARM specific. Then, I don't think it's proper to
> >>>>> modify code here for a specific arch ARM.
> >>>>
> >>>> The HOB creation is available in the
> >>>>
> StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.inf.
> If
> >>>> other architectures also use that instance, I think the issue is not
> >>>> specific to ARM.
> >>>>
> >>>
> >>> The question here is whether the implementation follows the PI spec,
> >>> and whether HOB creation should be supported to begin with.
> >>>
> >>
> >> My reading of the PI spec is that this implementation does not follow
> >> it. However, the PI spec is not very explicit about Standalone MM in
> >> general, but particularly in relation to HOBs.
> >>
> >> However, in the generic HOB section of PI spec v1.7, Vol. 3, section 4
> >> (entitled HOB Design Discussion) it explicitly lays out that there are
> >> HOB producer phases and HOB consumer phases. It uses PEI as a HOB
> >> producer phase and DXE as a HOB consumer phase and explicitly says
> >> that the HOB consumer phase must treat HOBs as read-only memory, per
> >> Ray's comment.
> >>
> >> In vol. 4, section 2.2, in discussing the Standalone MM entry point,
> >> the document talks about the HOB list being passed to Standalone MM
> >> to consume, which per the reading of the above section would classify
> >> Standalone MM as a HOB consumer phase, where HOBs should then be
> >> read-only.
> >>
> >> So, I believe that we should not support HOB creation in Standalone MM
> >> and instead rely on other mechanisms to pass information within the
> >> phase. Per Nhi's other email in this thread, we should have the
> >> discussion on how to solve that specific problem and that may well
> >> lead to a discussion on whether HOBs are in fact the right mechanism
> >> here, but I tend to lean towards leaving something as architectural as
> >> HOBs to what the PI spec defines and using different mechanisms to
> >> accomplish in-phase communication.
> > Thanks Oliver so much for that. I agree. We should focus on my specific
> > problem with UEFI Variable Flash Info in StandaloneMM in another thread.
> >>
> >> Does this reading of the spec align with others' expectations? As I
> >> mentioned to Ray in another thread, Standalone MM feels like it could
> >> have extra clarification in a few areas in the PI spec.
> >
> > Thanks again. The HOB library should be updated to remove the HOB
> > creation once we have the clarification.
> 
> I found that we can hook a platform NULL library class to the
> StandaloneMmCore to create UEFI Variable Flash Info HOB. This way is
> compliant to the spec as the HOB is created in MM_CORE_STANDALONE
> phase.
> 
> Should I write a patch to remove the HOB creation in StandaloneMmHobLib?
> 
> Regards,
> Nhi



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111969): https://edk2.groups.io/g/devel/message/111969
Mute This Topic: https://groups.io/mt/89020085/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Wed, 6 Sept 2023 at 08:33, Ni, Ray <ray.ni@intel.com> wrote:
>
> I am a bit confused.
>
> The HOB list in standalone MM is read-only. Why could any module call BuildGuidHob() to modify the HOB.
>
> I saw Oliver mentioned something about StMM. I don't know what that is. But it seems that's ARM specific. Then, I don't think it's proper to modify code here for a specific arch ARM.
>

Hello Ray,

StMM == Standalone MM

Does the PI spec document that the standalone MM hob list is
read-only? If so, the HOB build routines shouldn't be exposed (and/or
ASSERT in DEBUG builds). If not, they should work.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108308): https://edk2.groups.io/g/devel/message/108308
Mute This Topic: https://groups.io/mt/89020085/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-