Re: [edk2-devel] [PATCH v1 1/1] StandaloneMmPkg/StandaloneMmHobLib: Remove HOB creation

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

On 5/2/2024 4:31 AM, Oliver Smith-Denny via groups.io wrote:
> Hi folks, returning to this thread because I noticed that HOB
> creation still exists in StandaloneMmCore for ARM:
> 
> https://github.com/tianocore/edk2/blob/5d4c5253e8bbc0baa8837fcd868925212df85201/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c
> 
> As far as I can tell, there is only this one file that creates 6
> HOBs from StandaloneMmCore. Per our earlier discussion that led to
> disabling HOB creation in StandaloneMm, I think that this falls into
> the case where StandaloneMm is a HOB consumer phase, not a producer
> phase and so it should not be creating these HOBs. On the x64 side,
> all of the StandaloneMm HOB creation functions ASSERT with the
> comment that StandaloneMm is a HOB consumer phase and should not
> be creating HOBs.

The HOB creation in StandaloneMmCoreEntryPoint is exclusively consumed 
by the MM_CORE_STANDALONE module 
(StandaloneMmPkg/Core/StandaloneMmCore.inf), not a MM_STANDALONE module. 
Per my understanding earlier, the MM_CORE_STANDALONE is a producer and 
other MM_STANDALONE modules are consumers. So we removed the HOB 
creation in the StandaloneMmHobLib which is consumed by MM_STANDALONE 
modules.

Also, we still retain the HOB creation in the 
StandaloneMmPkg/Library/StandaloneMmCoreHobLib library instance.

Regards,
Nhi

> 
> On ARM this is more complicated, as all of this information would
> seem to originate from TF-A and so we would need TF-A to produce
> these HOBs to tell StandaloneMm the information it needs to
> operate. Today TF-A already has to communicate this information, the
> HOBs are just created in StandaloneMmCore instead of in TF-A.
> 
> Curious to get other folks thoughts here on this paradigm.
> 
> Thanks,
> Oliver
> 
> On 12/5/2023 5:47 AM, Nhi Pham wrote:
>> According to the discussion in "StandaloneMmPkg: Fix HOB space and
>> heap space conflicted issue" [1], Standalone MM modules should be HOB
>> consumers where HOB is read-only. Therefore, this patch removes the
>> supported functions for HOB creation in the StandaloneMmHobLib.
>>
>> [1] https://edk2.groups.io/g/devel/message/108333
>>
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Cc: Sami Mujawar <sami.mujawar@arm.com>
>> Cc: Oliver Smith-Denny <osde@linux.microsoft.com>
>> Signed-off-by: Nhi Pham <nhiphambka@gmail.com>
>> ---
>>   StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.c | 
>> 171 ++++++--------------
>>   1 file changed, 51 insertions(+), 120 deletions(-)
>>
>> diff --git 
>> a/StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.c 
>> b/StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.c
>> index ee61bdd227d0..bef66d167494 100644
>> --- a/StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.c
>> +++ b/StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.c
>> @@ -1,5 +1,5 @@
>>   /** @file
>>
>> -  HOB Library implementation for Standalone MM Core.
>>
>> +  HOB Library implementation for Standalone MM modules.
>>
>>
>>   Copyright (c) 2006 - 2014, Intel Corporation. All rights reserved.<BR>
>>
>>   Copyright (c) 2017 - 2018, ARM Limited. All rights reserved.<BR>
>>
>> @@ -250,48 +250,13 @@ GetBootModeHob (
>>     return HandOffHob->BootMode;
>>
>>   }
>>
>>
>> -VOID *
>>
>> -CreateHob (
>>
>> -  IN  UINT16  HobType,
>>
>> -  IN  UINT16  HobLength
>>
>> -  )
>>
>> -{
>>
>> -  EFI_HOB_HANDOFF_INFO_TABLE  *HandOffHob;
>>
>> -  EFI_HOB_GENERIC_HEADER      *HobEnd;
>>
>> -  EFI_PHYSICAL_ADDRESS        FreeMemory;
>>
>> -  VOID                        *Hob;
>>
>> -
>>
>> -  HandOffHob = GetHobList ();
>>
>> -
>>
>> -  HobLength = (UINT16)((HobLength + 0x7) & (~0x7));
>>
>> -
>>
>> -  FreeMemory = HandOffHob->EfiFreeMemoryTop - 
>> HandOffHob->EfiFreeMemoryBottom;
>>
>> -
>>
>> -  if (FreeMemory < HobLength) {
>>
>> -    return NULL;
>>
>> -  }
>>
>> -
>>
>> -  Hob                                        = (VOID 
>> *)(UINTN)HandOffHob->EfiEndOfHobList;
>>
>> -  ((EFI_HOB_GENERIC_HEADER *)Hob)->HobType   = HobType;
>>
>> -  ((EFI_HOB_GENERIC_HEADER *)Hob)->HobLength = HobLength;
>>
>> -  ((EFI_HOB_GENERIC_HEADER *)Hob)->Reserved  = 0;
>>
>> -
>>
>> -  HobEnd                      = (EFI_HOB_GENERIC_HEADER *)((UINTN)Hob 
>> + HobLength);
>>
>> -  HandOffHob->EfiEndOfHobList = (EFI_PHYSICAL_ADDRESS)(UINTN)HobEnd;
>>
>> -
>>
>> -  HobEnd->HobType   = EFI_HOB_TYPE_END_OF_HOB_LIST;
>>
>> -  HobEnd->HobLength = sizeof (EFI_HOB_GENERIC_HEADER);
>>
>> -  HobEnd->Reserved  = 0;
>>
>> -  HobEnd++;
>>
>> -  HandOffHob->EfiFreeMemoryBottom = (EFI_PHYSICAL_ADDRESS)(UINTN)HobEnd;
>>
>> -
>>
>> -  return Hob;
>>
>> -}
>>
>> -
>>
>>   /**
>>
>>     Builds a HOB for a loaded PE32 module.
>>
>>
>>     This function builds a HOB for a loaded PE32 module.
>>
>> +  It can only be invoked by Standalone MM Core.
>>
>> +  For Standalone MM drivers, it will ASSERT() since HOB is read only 
>> for Standalone MM drivers.
>>
>> +
>>
>>     If ModuleName is NULL, then ASSERT().
>>
>>     If there is no additional space for HOB creation, then ASSERT().
>>
>>
>> @@ -310,33 +275,19 @@ BuildModuleHob (
>>     IN EFI_PHYSICAL_ADDRESS  EntryPoint
>>
>>     )
>>
>>   {
>>
>> -  EFI_HOB_MEMORY_ALLOCATION_MODULE  *Hob;
>>
>> -
>>
>> -  ASSERT (
>>
>> -    ((MemoryAllocationModule & (EFI_PAGE_SIZE - 1)) == 0) &&
>>
>> -    ((ModuleLength & (EFI_PAGE_SIZE - 1)) == 0)
>>
>> -    );
>>
>> -
>>
>> -  Hob = CreateHob (EFI_HOB_TYPE_MEMORY_ALLOCATION, sizeof 
>> (EFI_HOB_MEMORY_ALLOCATION_MODULE));
>>
>> -
>>
>> -  CopyGuid (&(Hob->MemoryAllocationHeader.Name), 
>> &gEfiHobMemoryAllocModuleGuid);
>>
>> -  Hob->MemoryAllocationHeader.MemoryBaseAddress = 
>> MemoryAllocationModule;
>>
>> -  Hob->MemoryAllocationHeader.MemoryLength      = ModuleLength;
>>
>> -  Hob->MemoryAllocationHeader.MemoryType        = EfiBootServicesCode;
>>
>> -
>>
>>     //
>>
>> -  // Zero the reserved space to match HOB spec
>>
>> +  // HOB is read only for Standalone MM drivers
>>
>>     //
>>
>> -  ZeroMem (Hob->MemoryAllocationHeader.Reserved, sizeof 
>> (Hob->MemoryAllocationHeader.Reserved));
>>
>> -
>>
>> -  CopyGuid (&Hob->ModuleName, ModuleName);
>>
>> -  Hob->EntryPoint = EntryPoint;
>>
>> +  ASSERT (FALSE);
>>
>>   }
>>
>>
>>   /**
>>
>>     Builds a HOB that describes a chunk of system memory.
>>
>>
>>     This function builds a HOB that describes a chunk of system memory.
>>
>> +  It can only be invoked by Standalone MM Core.
>>
>> +  For Standalone MM drivers, it will ASSERT() since HOB is read only 
>> for Standalone MM drivers.
>>
>> +
>>
>>     If there is no additional space for HOB creation, then ASSERT().
>>
>>
>>     @param  ResourceType        The type of resource described by this 
>> HOB.
>>
>> @@ -354,15 +305,10 @@ BuildResourceDescriptorHob (
>>     IN UINT64                       NumberOfBytes
>>
>>     )
>>
>>   {
>>
>> -  EFI_HOB_RESOURCE_DESCRIPTOR  *Hob;
>>
>> -
>>
>> -  Hob = CreateHob (EFI_HOB_TYPE_RESOURCE_DESCRIPTOR, sizeof 
>> (EFI_HOB_RESOURCE_DESCRIPTOR));
>>
>> -  ASSERT (Hob != NULL);
>>
>> -
>>
>> -  Hob->ResourceType      = ResourceType;
>>
>> -  Hob->ResourceAttribute = ResourceAttribute;
>>
>> -  Hob->PhysicalStart     = PhysicalStart;
>>
>> -  Hob->ResourceLength    = NumberOfBytes;
>>
>> +  //
>>
>> +  // HOB is read only for Standalone MM drivers
>>
>> +  //
>>
>> +  ASSERT (FALSE);
>>
>>   }
>>
>>
>>   /**
>>
>> @@ -371,6 +317,9 @@ BuildResourceDescriptorHob (
>>     This function builds a customized HOB tagged with a GUID for 
>> identification
>>
>>     and returns the start address of GUID HOB data so that caller can 
>> fill the customized data.
>>
>>     The HOB Header and Name field is already stripped.
>>
>> +  It can only be invoked by Standalone MM Core.
>>
>> +  For Standalone MM drivers, it will ASSERT() since HOB is read only 
>> for Standalone MM drivers.
>>
>> +
>>
>>     If Guid is NULL, then ASSERT().
>>
>>     If there is no additional space for HOB creation, then ASSERT().
>>
>>     If DataLength >= (0x10000 - sizeof (EFI_HOB_GUID_TYPE)), then 
>> ASSERT().
>>
>> @@ -388,16 +337,11 @@ BuildGuidHob (
>>     IN UINTN           DataLength
>>
>>     )
>>
>>   {
>>
>> -  EFI_HOB_GUID_TYPE  *Hob;
>>
>> -
>>
>>     //
>>
>> -  // Make sure that data length is not too long.
>>
>> +  // HOB is read only for Standalone MM drivers
>>
>>     //
>>
>> -  ASSERT (DataLength <= (0xffff - sizeof (EFI_HOB_GUID_TYPE)));
>>
>> -
>>
>> -  Hob = CreateHob (EFI_HOB_TYPE_GUID_EXTENSION, (UINT16)(sizeof 
>> (EFI_HOB_GUID_TYPE) + DataLength));
>>
>> -  CopyGuid (&Hob->Name, Guid);
>>
>> -  return Hob + 1;
>>
>> +  ASSERT (FALSE);
>>
>> +  return NULL;
>>
>>   }
>>
>>
>>   /**
>>
>> @@ -406,6 +350,9 @@ BuildGuidHob (
>>     This function builds a customized HOB tagged with a GUID for 
>> identification,
>>
>>     copies the input data to the HOB data field and returns the start 
>> address of the GUID HOB data.
>>
>>     The HOB Header and Name field is already stripped.
>>
>> +  It can only be invoked by Standalone MM Core.
>>
>> +  For Standalone MM drivers, it will ASSERT() since HOB is read only 
>> for Standalone MM drivers.
>>
>> +
>>
>>     If Guid is NULL, then ASSERT().
>>
>>     If Data is NULL and DataLength > 0, then ASSERT().
>>
>>     If there is no additional space for HOB creation, then ASSERT().
>>
>> @@ -426,19 +373,20 @@ BuildGuidDataHob (
>>     IN UINTN           DataLength
>>
>>     )
>>
>>   {
>>
>> -  VOID  *HobData;
>>
>> -
>>
>> -  ASSERT (Data != NULL || DataLength == 0);
>>
>> -
>>
>> -  HobData = BuildGuidHob (Guid, DataLength);
>>
>> -
>>
>> -  return CopyMem (HobData, Data, DataLength);
>>
>> +  //
>>
>> +  // HOB is read only for Standalone MM drivers
>>
>> +  //
>>
>> +  ASSERT (FALSE);
>>
>> +  return NULL;
>>
>>   }
>>
>>
>>   /**
>>
>>     Builds a Firmware Volume HOB.
>>
>>
>>     This function builds a Firmware Volume HOB.
>>
>> +  It can only be invoked by Standalone MM Core.
>>
>> +  For Standalone MM drivers, it will ASSERT() since HOB is read only 
>> for Standalone MM drivers.
>>
>> +
>>
>>     If there is no additional space for HOB creation, then ASSERT().
>>
>>
>>     @param  BaseAddress   The base address of the Firmware Volume.
>>
>> @@ -452,18 +400,19 @@ BuildFvHob (
>>     IN UINT64                Length
>>
>>     )
>>
>>   {
>>
>> -  EFI_HOB_FIRMWARE_VOLUME  *Hob;
>>
>> -
>>
>> -  Hob = CreateHob (EFI_HOB_TYPE_FV, sizeof (EFI_HOB_FIRMWARE_VOLUME));
>>
>> -
>>
>> -  Hob->BaseAddress = BaseAddress;
>>
>> -  Hob->Length      = Length;
>>
>> +  //
>>
>> +  // HOB is read only for Standalone MM drivers
>>
>> +  //
>>
>> +  ASSERT (FALSE);
>>
>>   }
>>
>>
>>   /**
>>
>>     Builds a EFI_HOB_TYPE_FV2 HOB.
>>
>>
>>     This function builds a EFI_HOB_TYPE_FV2 HOB.
>>
>> +  It can only be invoked by Standalone MM Core.
>>
>> +  For Standalone MM drivers, it will ASSERT() since HOB is read only 
>> for Standalone MM drivers.
>>
>> +
>>
>>     If there is no additional space for HOB creation, then ASSERT().
>>
>>
>>     @param  BaseAddress   The base address of the Firmware Volume.
>>
>> @@ -481,20 +430,19 @@ BuildFv2Hob (
>>     IN CONST    EFI_GUID              *FileName
>>
>>     )
>>
>>   {
>>
>> -  EFI_HOB_FIRMWARE_VOLUME2  *Hob;
>>
>> -
>>
>> -  Hob = CreateHob (EFI_HOB_TYPE_FV2, sizeof (EFI_HOB_FIRMWARE_VOLUME2));
>>
>> -
>>
>> -  Hob->BaseAddress = BaseAddress;
>>
>> -  Hob->Length      = Length;
>>
>> -  CopyGuid (&Hob->FvName, FvName);
>>
>> -  CopyGuid (&Hob->FileName, FileName);
>>
>> +  //
>>
>> +  // HOB is read only for Standalone MM drivers
>>
>> +  //
>>
>> +  ASSERT (FALSE);
>>
>>   }
>>
>>
>>   /**
>>
>>     Builds a HOB for the CPU.
>>
>>
>>     This function builds a HOB for the CPU.
>>
>> +  It can only be invoked by Standalone MM Core.
>>
>> +  For Standalone MM drivers, it will ASSERT() since HOB is read only 
>> for Standalone MM drivers.
>>
>> +
>>
>>     If there is no additional space for HOB creation, then ASSERT().
>>
>>
>>     @param  SizeOfMemorySpace   The maximum physical memory 
>> addressability of the processor.
>>
>> @@ -508,23 +456,19 @@ BuildCpuHob (
>>     IN UINT8  SizeOfIoSpace
>>
>>     )
>>
>>   {
>>
>> -  EFI_HOB_CPU  *Hob;
>>
>> -
>>
>> -  Hob = CreateHob (EFI_HOB_TYPE_CPU, sizeof (EFI_HOB_CPU));
>>
>> -
>>
>> -  Hob->SizeOfMemorySpace = SizeOfMemorySpace;
>>
>> -  Hob->SizeOfIoSpace     = SizeOfIoSpace;
>>
>> -
>>
>>     //
>>
>> -  // Zero the reserved space to match HOB spec
>>
>> +  // HOB is read only for Standalone MM drivers
>>
>>     //
>>
>> -  ZeroMem (Hob->Reserved, sizeof (Hob->Reserved));
>>
>> +  ASSERT (FALSE);
>>
>>   }
>>
>>
>>   /**
>>
>>     Builds a HOB for the memory allocation.
>>
>>
>>     This function builds a HOB for the memory allocation.
>>
>> +  It can only be invoked by Standalone MM Core.
>>
>> +  For Standalone MM drivers, it will ASSERT() since HOB is read only 
>> for Standalone MM drivers.
>>
>> +
>>
>>     If there is no additional space for HOB creation, then ASSERT().
>>
>>
>>     @param  BaseAddress   The 64 bit physical address of the memory.
>>
>> @@ -540,23 +484,10 @@ BuildMemoryAllocationHob (
>>     IN EFI_MEMORY_TYPE       MemoryType
>>
>>     )
>>
>>   {
>>
>> -  EFI_HOB_MEMORY_ALLOCATION  *Hob;
>>
>> -
>>
>> -  ASSERT (
>>
>> -    ((BaseAddress & (EFI_PAGE_SIZE - 1)) == 0) &&
>>
>> -    ((Length & (EFI_PAGE_SIZE - 1)) == 0)
>>
>> -    );
>>
>> -
>>
>> -  Hob = CreateHob (EFI_HOB_TYPE_MEMORY_ALLOCATION, sizeof 
>> (EFI_HOB_MEMORY_ALLOCATION));
>>
>> -
>>
>> -  ZeroMem (&(Hob->AllocDescriptor.Name), sizeof (EFI_GUID));
>>
>> -  Hob->AllocDescriptor.MemoryBaseAddress = BaseAddress;
>>
>> -  Hob->AllocDescriptor.MemoryLength      = Length;
>>
>> -  Hob->AllocDescriptor.MemoryType        = MemoryType;
>>
>>     //
>>
>> -  // Zero the reserved space to match HOB spec
>>
>> +  // HOB is read only for Standalone MM drivers
>>
>>     //
>>
>> -  ZeroMem (Hob->AllocDescriptor.Reserved, sizeof 
>> (Hob->AllocDescriptor.Reserved));
>>
>> +  ASSERT (FALSE);
>>
>>   }
>>
>>
>>   /**
>>
> 
> 
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118552): https://edk2.groups.io/g/devel/message/118552
Mute This Topic: https://groups.io/mt/103018869/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-