[edk2-devel] [Patch v2 0/2] MdeModulePkg/Core/Dxe: Set MemoryTypeInfo bin range from HOB

[edk2-devel] [Patch v2 0/2] MdeModulePkg/Core/Dxe: Set MemoryTypeInfo bin range from HOB

[Michael D Kinney]

New in V2
=========
* Break single patch into 2 patches
  * Moves GCD Initialization before RT data allocations
  * Set MemoryTypeInfo bin range from HOB feature
* Update description of Memory Type Information GUID to 
  describe additional use case as Owner GUID in a Resource
  Descriptor HOB for preferred memory range for Memory Type 
  Information bins.
  
Provide an optional method for PEI to declare a specific address
range to use for the Memory Type Information bins. The current
algorithm uses heuristics that tends to place the Memory Type
Information bins in the same location, but memory configuration
changes across boots or algorithm changes across a firmware
updates could potentially change the Memory Type Information bin
location.

If the HOB List contains a Resource Descriptor HOB that
describes tested system memory and has an Owner GUID of
gEfiMemoryTypeInformationGuid, then use the address range
described by the Resource Descriptor HOB as the preferred
location of the Memory Type Information bins. If this HOB is
not detected, then the current behavior is preserved.

The HOB with an Owner GUID of gEfiMemoryTypeInformationGuid
is ignored for the following conditions:
* The HOB with an Owner GUID of gEfiMemoryTypeInformationGuid
  is smaller than the Memory Type Information bins.
* The HOB list contains more than one Resource Descriptor HOB
  with an owner GUID of gEfiMemoryTypeInformationGuid.
* The Resource Descriptor HOB with an Owner GUID of
  gEfiMemoryTypeInformationGuid is the same Resource Descriptor
  HOB that that describes the PHIT memory range.

Update the DxeMain initialization order to initialize GCD
services before any runtime allocations are performed.  This
is required to prevent runtime data fragmentation when the
UEFI System Table and UEFI Runtime Service Table are allocated
before both the memory and GCD services are initialized.

Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Aaron Li <aaron.li@intel.com>
Cc: Liu Yun <yun.y.liu@intel.com>
Cc: Andrew Fish <afish@apple.com>
Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>

Michael D Kinney (2):
  MdeModulePkg/Core/Dxe: Initialize GCD before RT memory allocations
  MdeModulePkg/Core/Dxe: Set MemoryTypeInfo bin range from HOB

 MdeModulePkg/Core/Dxe/DxeMain.h               |   6 ++
 MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c       |  23 ++--
 MdeModulePkg/Core/Dxe/Gcd/Gcd.c               |  72 ++++++++++++-
 MdeModulePkg/Core/Dxe/Image/Image.c           |   1 -
 MdeModulePkg/Core/Dxe/Mem/Page.c              | 101 ++++++++++++++++++
 .../Include/Guid/MemoryTypeInformation.h      |  14 ++-
 6 files changed, 199 insertions(+), 18 deletions(-)

-- 
2.40.1.windows.1


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

[edk2-devel] [Patch v2 1/2] MdeModulePkg/Core/Dxe: Initialize GCD before RT memory allocations

[Michael D Kinney]

Update the DxeMain initialization order to initialize GCD
services before any runtime allocations are performed.  This
is required to prevent runtime data fragmentation when the
UEFI System Table and UEFI Runtime Service Table are allocated
before both the memory and GCD services are initialized.

Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Aaron Li <aaron.li@intel.com>
Cc: Liu Yun <yun.y.liu@intel.com>
Cc: Andrew Fish <afish@apple.com>
Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
---
 MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c | 23 ++++++++++++++---------
 MdeModulePkg/Core/Dxe/Image/Image.c     |  1 -
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
index 0e0f9769b99d..17d510a287e5 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
+++ b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
@@ -276,6 +276,18 @@ DxeMain (
 
   MemoryProfileInit (HobStart);
 
+  //
+  // Start the Image Services.
+  //
+  Status = CoreInitializeImageServices (HobStart);
+  ASSERT_EFI_ERROR (Status);
+
+  //
+  // Initialize the Global Coherency Domain Services
+  //
+  Status = CoreInitializeGcdServices (&HobStart, MemoryBaseAddress, MemoryLength);
+  ASSERT_EFI_ERROR (Status);
+
   //
   // Allocate the EFI System Table and EFI Runtime Service Table from EfiRuntimeServicesData
   // Use the templates to initialize the contents of the EFI System Table and EFI Runtime Services Table
@@ -289,16 +301,9 @@ DxeMain (
   gDxeCoreST->RuntimeServices = gDxeCoreRT;
 
   //
-  // Start the Image Services.
+  // Update DXE Core Loaded Image Protocol with allocated UEFI System Table
   //
-  Status = CoreInitializeImageServices (HobStart);
-  ASSERT_EFI_ERROR (Status);
-
-  //
-  // Initialize the Global Coherency Domain Services
-  //
-  Status = CoreInitializeGcdServices (&HobStart, MemoryBaseAddress, MemoryLength);
-  ASSERT_EFI_ERROR (Status);
+  gDxeCoreLoadedImage->SystemTable = gDxeCoreST;
 
   //
   // Call constructor for all libraries
diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c b/MdeModulePkg/Core/Dxe/Image/Image.c
index 6bc3a549ae5e..3fc74eb9fd6b 100644
--- a/MdeModulePkg/Core/Dxe/Image/Image.c
+++ b/MdeModulePkg/Core/Dxe/Image/Image.c
@@ -222,7 +222,6 @@ CoreInitializeImageServices (
   Image->ImageBasePage    = DxeCoreImageBaseAddress;
   Image->NumberOfPages    = (UINTN)(EFI_SIZE_TO_PAGES ((UINTN)(DxeCoreImageLength)));
   Image->Tpl              = gEfiCurrentTpl;
-  Image->Info.SystemTable = gDxeCoreST;
   Image->Info.ImageBase   = (VOID *)(UINTN)DxeCoreImageBaseAddress;
   Image->Info.ImageSize   = DxeCoreImageLength;
 
-- 
2.40.1.windows.1



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

[edk2-devel] [Patch v2 2/2] MdeModulePkg/Core/Dxe: Set MemoryTypeInfo bin range from HOB

[Michael D Kinney]

Provide an optional method for PEI to declare a specific address
range to use for the Memory Type Information bins. The current
algorithm uses heuristics that tends to place the Memory Type
Information bins in the same location, but memory configuration
changes across boots or algorithm changes across a firmware
updates could potentially change the Memory Type Information bin
location.

If the HOB List contains a Resource Descriptor HOB that
describes tested system memory and has an Owner GUID of
gEfiMemoryTypeInformationGuid, then use the address range
described by the Resource Descriptor HOB as the preferred
location of the Memory Type Information bins. If this HOB is
not detected, then the current behavior is preserved.

The HOB with an Owner GUID of gEfiMemoryTypeInformationGuid
is ignored for the following conditions:
* The HOB with an Owner GUID of gEfiMemoryTypeInformationGuid
  is smaller than the Memory Type Information bins.
* The HOB list contains more than one Resource Descriptor HOB
  with an owner GUID of gEfiMemoryTypeInformationGuid.
* The Resource Descriptor HOB with an Owner GUID of
  gEfiMemoryTypeInformationGuid is the same Resource Descriptor
  HOB that that describes the PHIT memory range.

Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Aaron Li <aaron.li@intel.com>
Cc: Liu Yun <yun.y.liu@intel.com>
Cc: Andrew Fish <afish@apple.com>
Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
---
 MdeModulePkg/Core/Dxe/DxeMain.h               |   6 ++
 MdeModulePkg/Core/Dxe/Gcd/Gcd.c               |  72 ++++++++++++-
 MdeModulePkg/Core/Dxe/Mem/Page.c              | 101 ++++++++++++++++++
 .../Include/Guid/MemoryTypeInformation.h      |  14 ++-
 4 files changed, 185 insertions(+), 8 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h
index 86a7be2f5188..53e26703f8c7 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.h
+++ b/MdeModulePkg/Core/Dxe/DxeMain.h
@@ -277,6 +277,12 @@ CoreInitializePool (
   VOID
   );
 
+VOID
+CoreSetMemoryTypeInformationRange (
+  IN EFI_PHYSICAL_ADDRESS  Start,
+  IN UINT64                Length
+  );
+
 /**
   Called to initialize the memory map and add descriptors to
   the current descriptor list.
diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
index 792cd2e0af23..c450d1bf2531 100644
--- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
+++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
@@ -2238,6 +2238,8 @@ CoreInitializeMemoryServices (
   EFI_HOB_HANDOFF_INFO_TABLE   *PhitHob;
   EFI_HOB_RESOURCE_DESCRIPTOR  *ResourceHob;
   EFI_HOB_RESOURCE_DESCRIPTOR  *PhitResourceHob;
+  EFI_HOB_RESOURCE_DESCRIPTOR  *MemoryTypeInformationResourceHob;
+  UINTN                        Count;
   EFI_PHYSICAL_ADDRESS         BaseAddress;
   UINT64                       Length;
   UINT64                       Attributes;
@@ -2289,12 +2291,47 @@ CoreInitializeMemoryServices (
   //
   // See if a Memory Type Information HOB is available
   //
-  GuidHob = GetFirstGuidHob (&gEfiMemoryTypeInformationGuid);
+  MemoryTypeInformationResourceHob = NULL;
+  GuidHob                          = GetFirstGuidHob (&gEfiMemoryTypeInformationGuid);
   if (GuidHob != NULL) {
     EfiMemoryTypeInformation = GET_GUID_HOB_DATA (GuidHob);
     DataSize                 = GET_GUID_HOB_DATA_SIZE (GuidHob);
     if ((EfiMemoryTypeInformation != NULL) && (DataSize > 0) && (DataSize <= (EfiMaxMemoryType + 1) * sizeof (EFI_MEMORY_TYPE_INFORMATION))) {
       CopyMem (&gMemoryTypeInformation, EfiMemoryTypeInformation, DataSize);
+
+      //
+      // Look for Resource Descriptor HOB with a ResourceType of System Memory
+      // and an Owner GUID of gEfiMemoryTypeInformationGuid. If more than 1 is
+      // found, then set MemoryTypeInformationResourceHob to NULL.
+      //
+      Count = 0;
+      for (Hob.Raw = *HobStart; !END_OF_HOB_LIST (Hob); Hob.Raw = GET_NEXT_HOB (Hob)) {
+        if (GET_HOB_TYPE (Hob) != EFI_HOB_TYPE_RESOURCE_DESCRIPTOR) {
+          continue;
+        }
+
+        ResourceHob = Hob.ResourceDescriptor;
+        if (!CompareGuid (&ResourceHob->Owner, &gEfiMemoryTypeInformationGuid)) {
+          continue;
+        }
+
+        Count++;
+        if (ResourceHob->ResourceType != EFI_RESOURCE_SYSTEM_MEMORY) {
+          continue;
+        }
+
+        if ((ResourceHob->ResourceAttribute & MEMORY_ATTRIBUTE_MASK) != TESTED_MEMORY_ATTRIBUTES) {
+          continue;
+        }
+
+        if (ResourceHob->ResourceLength >= CalculateTotalMemoryBinSizeNeeded ()) {
+          MemoryTypeInformationResourceHob = ResourceHob;
+        }
+      }
+
+      if (Count > 1) {
+        MemoryTypeInformationResourceHob = NULL;
+      }
     }
   }
 
@@ -2344,6 +2381,15 @@ CoreInitializeMemoryServices (
     PhitResourceHob = ResourceHob;
     Found           = TRUE;
 
+    //
+    // If a Memory Type Information Resource HOB was found and is the same
+    // Resource HOB that describes the PHIT HOB, then ignore the Memory Type
+    // Information Resource HOB.
+    //
+    if (MemoryTypeInformationResourceHob == PhitResourceHob) {
+      MemoryTypeInformationResourceHob = NULL;
+    }
+
     //
     // Compute range between PHIT EfiMemoryTop and the end of the Resource Descriptor HOB
     //
@@ -2387,8 +2433,9 @@ CoreInitializeMemoryServices (
   if (Length < MinimalMemorySizeNeeded) {
     //
     // Search all the resource descriptor HOBs from the highest possible addresses down for a memory
-    // region that is big enough to initialize the DXE core.  Always skip the PHIT Resource HOB.
-    // The max address must be within the physically addressible range for the processor.
+    // region that is big enough to initialize the DXE core.  Always skip the PHIT Resource HOB
+    // and the Memory Type Information Resource HOB. The max address must be within the physically
+    // addressable range for the processor.
     //
     HighAddress = MAX_ALLOC_ADDRESS;
     for (Hob.Raw = *HobStart; !END_OF_HOB_LIST (Hob); Hob.Raw = GET_NEXT_HOB (Hob)) {
@@ -2399,6 +2446,13 @@ CoreInitializeMemoryServices (
         continue;
       }
 
+      //
+      // Skip the Resource Descriptor HOB that contains Memory Type Information bins
+      //
+      if (Hob.ResourceDescriptor == MemoryTypeInformationResourceHob) {
+        continue;
+      }
+
       //
       // Skip all HOBs except Resource Descriptor HOBs
       //
@@ -2466,6 +2520,18 @@ CoreInitializeMemoryServices (
     Capabilities = CoreConvertResourceDescriptorHobAttributesToCapabilities (EfiGcdMemoryTypeSystemMemory, Attributes);
   }
 
+  if (MemoryTypeInformationResourceHob != NULL) {
+    //
+    // If a Memory Type Information Resource HOB was found, then use the address
+    // range of the  Memory Type Information Resource HOB as the preferred
+    // address range for the Memory Type Information bins.
+    //
+    CoreSetMemoryTypeInformationRange (
+      MemoryTypeInformationResourceHob->PhysicalStart,
+      MemoryTypeInformationResourceHob->ResourceLength
+      );
+  }
+
   //
   // Declare the very first memory region, so the EFI Memory Services are available.
   //
diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
index 6497af573353..458c62090265 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -532,6 +532,107 @@ CoreLoadingFixedAddressHook (
   return;
 }
 
+/**
+  Sets the preferred memory range to use for the Memory Type Information bins.
+  This service must be called before fist call to CoreAddMemoryDescriptor().
+
+  If the location of the Memory Type Information bins has already been
+  established or the size of the range provides is smaller than all the
+  Memory Type Information bins, then the range provides is not used.
+
+  @param  Start   The start address of the Memory Type Information range.
+  @param  Length  The size, in bytes, of the Memory Type Information range.
+**/
+VOID
+CoreSetMemoryTypeInformationRange (
+  IN EFI_PHYSICAL_ADDRESS  Start,
+  IN UINT64                Length
+  )
+{
+  EFI_PHYSICAL_ADDRESS  Top;
+  EFI_MEMORY_TYPE       Type;
+  UINTN                 Index;
+  UINTN                 Size;
+
+  //
+  // Return if Memory Type Information bin locations have already been set
+  //
+  if (mMemoryTypeInformationInitialized) {
+    return;
+  }
+
+  //
+  // Return if size of the Memory Type Information bins is greater than Length
+  //
+  Size = 0;
+  for (Index = 0; gMemoryTypeInformation[Index].Type != EfiMaxMemoryType; Index++) {
+    //
+    // Make sure the memory type in the gMemoryTypeInformation[] array is valid
+    //
+    Type = (EFI_MEMORY_TYPE)(gMemoryTypeInformation[Index].Type);
+    if ((UINT32)Type > EfiMaxMemoryType) {
+      continue;
+    }
+
+    Size += EFI_PAGES_TO_SIZE (gMemoryTypeInformation[Index].NumberOfPages);
+  }
+
+  if (Size > Length) {
+    return;
+  }
+
+  //
+  // Loop through each memory type in the order specified by the
+  // gMemoryTypeInformation[] array
+  //
+  Top = Start + Length;
+  for (Index = 0; gMemoryTypeInformation[Index].Type != EfiMaxMemoryType; Index++) {
+    //
+    // Make sure the memory type in the gMemoryTypeInformation[] array is valid
+    //
+    Type = (EFI_MEMORY_TYPE)(gMemoryTypeInformation[Index].Type);
+    if ((UINT32)Type > EfiMaxMemoryType) {
+      continue;
+    }
+
+    if (gMemoryTypeInformation[Index].NumberOfPages != 0) {
+      mMemoryTypeStatistics[Type].MaximumAddress = Top - 1;
+      Top                                       -= EFI_PAGES_TO_SIZE (gMemoryTypeInformation[Index].NumberOfPages);
+      mMemoryTypeStatistics[Type].BaseAddress    = Top;
+
+      //
+      // If the current base address is the lowest address so far, then update
+      // the default maximum address
+      //
+      if (mMemoryTypeStatistics[Type].BaseAddress < mDefaultMaximumAddress) {
+        mDefaultMaximumAddress = mMemoryTypeStatistics[Type].BaseAddress - 1;
+      }
+
+      mMemoryTypeStatistics[Type].NumberOfPages   = gMemoryTypeInformation[Index].NumberOfPages;
+      gMemoryTypeInformation[Index].NumberOfPages = 0;
+    }
+  }
+
+  //
+  // If the number of pages reserved for a memory type is 0, then all
+  // allocations for that type should be in the default range.
+  //
+  for (Type = (EFI_MEMORY_TYPE)0; Type < EfiMaxMemoryType; Type++) {
+    for (Index = 0; gMemoryTypeInformation[Index].Type != EfiMaxMemoryType; Index++) {
+      if (Type == (EFI_MEMORY_TYPE)gMemoryTypeInformation[Index].Type) {
+        mMemoryTypeStatistics[Type].InformationIndex = Index;
+      }
+    }
+
+    mMemoryTypeStatistics[Type].CurrentNumberOfPages = 0;
+    if (mMemoryTypeStatistics[Type].MaximumAddress == MAX_ALLOC_ADDRESS) {
+      mMemoryTypeStatistics[Type].MaximumAddress = mDefaultMaximumAddress;
+    }
+  }
+
+  mMemoryTypeInformationInitialized = TRUE;
+}
+
 /**
   Called to initialize the memory map and add descriptors to
   the current descriptor list.
diff --git a/MdeModulePkg/Include/Guid/MemoryTypeInformation.h b/MdeModulePkg/Include/Guid/MemoryTypeInformation.h
index e97d88765ece..65adcf478b80 100644
--- a/MdeModulePkg/Include/Guid/MemoryTypeInformation.h
+++ b/MdeModulePkg/Include/Guid/MemoryTypeInformation.h
@@ -1,14 +1,18 @@
 /** @file
   This file defines:
-  * Memory Type Information GUID for HOB and Variable.
+  * Memory Type Information GUID for Guided HOB and Variable.
   * Memory Type Information Variable Name.
   * Memory Type Information GUID HOB data structure.
 
-  The memory type information HOB and variable can
-  be used to store the information for each memory type in Variable or HOB.
+  The memory type information HOB and variable can be used to store information
+  for each memory type in Variable or HOB.
 
-Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
-SPDX-License-Identifier: BSD-2-Clause-Patent
+  The Memory Type Information GUID can also be optionally used as the Owner
+  field of a Resource Descriptor HOB to provide the preferred memory range
+  for the memory types described in the Memory Type Information GUID HOB.
+
+  Copyright (c) 2006, Intel Corporation. All rights reserved.<BR>
+  SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
 
-- 
2.40.1.windows.1



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

回复: [edk2-devel] [Patch v2 0/2] MdeModulePkg/Core/Dxe: Set MemoryTypeInfo bin range from HOB

[gaoliming via groups.io]

Mike:
  Thanks for your update. This patch set is good to me. Reviewed-by: Liming
Gao <gaoliming@byosoft.com.cn>

Thanks
Liming
> -----邮件原件-----
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Michael D
> Kinney
> 发送时间: 2024年1月29日 11:45
> 收件人: devel@edk2.groups.io
> 抄送: Liming Gao <gaoliming@byosoft.com.cn>; Aaron Li
> <aaron.li@intel.com>; Liu Yun <yun.y.liu@intel.com>; Andrew Fish
> <afish@apple.com>
> 主题: [edk2-devel] [Patch v2 0/2] MdeModulePkg/Core/Dxe: Set
> MemoryTypeInfo bin range from HOB
> 
> New in V2
> =========
> * Break single patch into 2 patches
>   * Moves GCD Initialization before RT data allocations
>   * Set MemoryTypeInfo bin range from HOB feature
> * Update description of Memory Type Information GUID to
>   describe additional use case as Owner GUID in a Resource
>   Descriptor HOB for preferred memory range for Memory Type
>   Information bins.
> 
> Provide an optional method for PEI to declare a specific address
> range to use for the Memory Type Information bins. The current
> algorithm uses heuristics that tends to place the Memory Type
> Information bins in the same location, but memory configuration
> changes across boots or algorithm changes across a firmware
> updates could potentially change the Memory Type Information bin
> location.
> 
> If the HOB List contains a Resource Descriptor HOB that
> describes tested system memory and has an Owner GUID of
> gEfiMemoryTypeInformationGuid, then use the address range
> described by the Resource Descriptor HOB as the preferred
> location of the Memory Type Information bins. If this HOB is
> not detected, then the current behavior is preserved.
> 
> The HOB with an Owner GUID of gEfiMemoryTypeInformationGuid
> is ignored for the following conditions:
> * The HOB with an Owner GUID of gEfiMemoryTypeInformationGuid
>   is smaller than the Memory Type Information bins.
> * The HOB list contains more than one Resource Descriptor HOB
>   with an owner GUID of gEfiMemoryTypeInformationGuid.
> * The Resource Descriptor HOB with an Owner GUID of
>   gEfiMemoryTypeInformationGuid is the same Resource Descriptor
>   HOB that that describes the PHIT memory range.
> 
> Update the DxeMain initialization order to initialize GCD
> services before any runtime allocations are performed.  This
> is required to prevent runtime data fragmentation when the
> UEFI System Table and UEFI Runtime Service Table are allocated
> before both the memory and GCD services are initialized.
> 
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Aaron Li <aaron.li@intel.com>
> Cc: Liu Yun <yun.y.liu@intel.com>
> Cc: Andrew Fish <afish@apple.com>
> Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
> 
> Michael D Kinney (2):
>   MdeModulePkg/Core/Dxe: Initialize GCD before RT memory allocations
>   MdeModulePkg/Core/Dxe: Set MemoryTypeInfo bin range from HOB
> 
>  MdeModulePkg/Core/Dxe/DxeMain.h               |   6 ++
>  MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c       |  23 ++--
>  MdeModulePkg/Core/Dxe/Gcd/Gcd.c               |  72
> ++++++++++++-
>  MdeModulePkg/Core/Dxe/Image/Image.c           |   1 -
>  MdeModulePkg/Core/Dxe/Mem/Page.c              | 101
> ++++++++++++++++++
>  .../Include/Guid/MemoryTypeInformation.h      |  14 ++-
>  6 files changed, 199 insertions(+), 18 deletions(-)
> 
> --
> 2.40.1.windows.1
> 
> 
> 
> 





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

Re: [edk2-devel] [Patch v2 1/2] MdeModulePkg/Core/Dxe: Initialize GCD before RT memory allocations

[Laszlo Ersek]

On 1/29/24 04:45, Michael D Kinney wrote:
> Update the DxeMain initialization order to initialize GCD
> services before any runtime allocations are performed.  This
> is required to prevent runtime data fragmentation when the
> UEFI System Table and UEFI Runtime Service Table are allocated
> before both the memory and GCD services are initialized.
> 
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Aaron Li <aaron.li@intel.com>
> Cc: Liu Yun <yun.y.liu@intel.com>
> Cc: Andrew Fish <afish@apple.com>
> Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
> ---
>  MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c | 23 ++++++++++++++---------
>  MdeModulePkg/Core/Dxe/Image/Image.c     |  1 -
>  2 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> index 0e0f9769b99d..17d510a287e5 100644
> --- a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> +++ b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> @@ -276,6 +276,18 @@ DxeMain (
>  
>    MemoryProfileInit (HobStart);
>  
> +  //
> +  // Start the Image Services.
> +  //
> +  Status = CoreInitializeImageServices (HobStart);
> +  ASSERT_EFI_ERROR (Status);
> +
> +  //
> +  // Initialize the Global Coherency Domain Services
> +  //
> +  Status = CoreInitializeGcdServices (&HobStart, MemoryBaseAddress, MemoryLength);
> +  ASSERT_EFI_ERROR (Status);
> +
>    //
>    // Allocate the EFI System Table and EFI Runtime Service Table from EfiRuntimeServicesData
>    // Use the templates to initialize the contents of the EFI System Table and EFI Runtime Services Table
> @@ -289,16 +301,9 @@ DxeMain (
>    gDxeCoreST->RuntimeServices = gDxeCoreRT;
>  
>    //
> -  // Start the Image Services.
> +  // Update DXE Core Loaded Image Protocol with allocated UEFI System Table
>    //
> -  Status = CoreInitializeImageServices (HobStart);
> -  ASSERT_EFI_ERROR (Status);
> -
> -  //
> -  // Initialize the Global Coherency Domain Services
> -  //
> -  Status = CoreInitializeGcdServices (&HobStart, MemoryBaseAddress, MemoryLength);
> -  ASSERT_EFI_ERROR (Status);
> +  gDxeCoreLoadedImage->SystemTable = gDxeCoreST;
>  
>    //
>    // Call constructor for all libraries
> diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c b/MdeModulePkg/Core/Dxe/Image/Image.c
> index 6bc3a549ae5e..3fc74eb9fd6b 100644
> --- a/MdeModulePkg/Core/Dxe/Image/Image.c
> +++ b/MdeModulePkg/Core/Dxe/Image/Image.c
> @@ -222,7 +222,6 @@ CoreInitializeImageServices (
>    Image->ImageBasePage    = DxeCoreImageBaseAddress;
>    Image->NumberOfPages    = (UINTN)(EFI_SIZE_TO_PAGES ((UINTN)(DxeCoreImageLength)));
>    Image->Tpl              = gEfiCurrentTpl;
> -  Image->Info.SystemTable = gDxeCoreST;
>    Image->Info.ImageBase   = (VOID *)(UINTN)DxeCoreImageBaseAddress;
>    Image->Info.ImageSize   = DxeCoreImageLength;
>  

Reviewed-by: Laszlo Ersek <lersek@redhat.com>



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

Re: [edk2-devel] [Patch v2 2/2] MdeModulePkg/Core/Dxe: Set MemoryTypeInfo bin range from HOB

[Laszlo Ersek]

On 1/29/24 04:45, Michael D Kinney wrote:
> Provide an optional method for PEI to declare a specific address
> range to use for the Memory Type Information bins. The current
> algorithm uses heuristics that tends to place the Memory Type
> Information bins in the same location, but memory configuration
> changes across boots or algorithm changes across a firmware
> updates could potentially change the Memory Type Information bin
> location.
> 
> If the HOB List contains a Resource Descriptor HOB that
> describes tested system memory and has an Owner GUID of
> gEfiMemoryTypeInformationGuid, then use the address range
> described by the Resource Descriptor HOB as the preferred
> location of the Memory Type Information bins. If this HOB is
> not detected, then the current behavior is preserved.
> 
> The HOB with an Owner GUID of gEfiMemoryTypeInformationGuid
> is ignored for the following conditions:
> * The HOB with an Owner GUID of gEfiMemoryTypeInformationGuid
>   is smaller than the Memory Type Information bins.
> * The HOB list contains more than one Resource Descriptor HOB
>   with an owner GUID of gEfiMemoryTypeInformationGuid.
> * The Resource Descriptor HOB with an Owner GUID of
>   gEfiMemoryTypeInformationGuid is the same Resource Descriptor
>   HOB that that describes the PHIT memory range.
> 
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Aaron Li <aaron.li@intel.com>
> Cc: Liu Yun <yun.y.liu@intel.com>
> Cc: Andrew Fish <afish@apple.com>
> Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
> ---
>  MdeModulePkg/Core/Dxe/DxeMain.h               |   6 ++
>  MdeModulePkg/Core/Dxe/Gcd/Gcd.c               |  72 ++++++++++++-
>  MdeModulePkg/Core/Dxe/Mem/Page.c              | 101 ++++++++++++++++++
>  .../Include/Guid/MemoryTypeInformation.h      |  14 ++-
>  4 files changed, 185 insertions(+), 8 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h
> index 86a7be2f5188..53e26703f8c7 100644
> --- a/MdeModulePkg/Core/Dxe/DxeMain.h
> +++ b/MdeModulePkg/Core/Dxe/DxeMain.h
> @@ -277,6 +277,12 @@ CoreInitializePool (
>    VOID
>    );
>  
> +VOID
> +CoreSetMemoryTypeInformationRange (
> +  IN EFI_PHYSICAL_ADDRESS  Start,
> +  IN UINT64                Length
> +  );
> +
>  /**
>    Called to initialize the memory map and add descriptors to
>    the current descriptor list.
> diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> index 792cd2e0af23..c450d1bf2531 100644
> --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> @@ -2238,6 +2238,8 @@ CoreInitializeMemoryServices (
>    EFI_HOB_HANDOFF_INFO_TABLE   *PhitHob;
>    EFI_HOB_RESOURCE_DESCRIPTOR  *ResourceHob;
>    EFI_HOB_RESOURCE_DESCRIPTOR  *PhitResourceHob;
> +  EFI_HOB_RESOURCE_DESCRIPTOR  *MemoryTypeInformationResourceHob;
> +  UINTN                        Count;
>    EFI_PHYSICAL_ADDRESS         BaseAddress;
>    UINT64                       Length;
>    UINT64                       Attributes;
> @@ -2289,12 +2291,47 @@ CoreInitializeMemoryServices (
>    //
>    // See if a Memory Type Information HOB is available
>    //
> -  GuidHob = GetFirstGuidHob (&gEfiMemoryTypeInformationGuid);
> +  MemoryTypeInformationResourceHob = NULL;
> +  GuidHob                          = GetFirstGuidHob (&gEfiMemoryTypeInformationGuid);
>    if (GuidHob != NULL) {
>      EfiMemoryTypeInformation = GET_GUID_HOB_DATA (GuidHob);
>      DataSize                 = GET_GUID_HOB_DATA_SIZE (GuidHob);
>      if ((EfiMemoryTypeInformation != NULL) && (DataSize > 0) && (DataSize <= (EfiMaxMemoryType + 1) * sizeof (EFI_MEMORY_TYPE_INFORMATION))) {
>        CopyMem (&gMemoryTypeInformation, EfiMemoryTypeInformation, DataSize);
> +
> +      //
> +      // Look for Resource Descriptor HOB with a ResourceType of System Memory
> +      // and an Owner GUID of gEfiMemoryTypeInformationGuid. If more than 1 is
> +      // found, then set MemoryTypeInformationResourceHob to NULL.
> +      //
> +      Count = 0;
> +      for (Hob.Raw = *HobStart; !END_OF_HOB_LIST (Hob); Hob.Raw = GET_NEXT_HOB (Hob)) {
> +        if (GET_HOB_TYPE (Hob) != EFI_HOB_TYPE_RESOURCE_DESCRIPTOR) {
> +          continue;
> +        }
> +
> +        ResourceHob = Hob.ResourceDescriptor;
> +        if (!CompareGuid (&ResourceHob->Owner, &gEfiMemoryTypeInformationGuid)) {
> +          continue;
> +        }
> +
> +        Count++;
> +        if (ResourceHob->ResourceType != EFI_RESOURCE_SYSTEM_MEMORY) {
> +          continue;
> +        }
> +
> +        if ((ResourceHob->ResourceAttribute & MEMORY_ATTRIBUTE_MASK) != TESTED_MEMORY_ATTRIBUTES) {
> +          continue;
> +        }
> +
> +        if (ResourceHob->ResourceLength >= CalculateTotalMemoryBinSizeNeeded ()) {
> +          MemoryTypeInformationResourceHob = ResourceHob;
> +        }
> +      }
> +
> +      if (Count > 1) {
> +        MemoryTypeInformationResourceHob = NULL;
> +      }
>      }
>    }
>  
> @@ -2344,6 +2381,15 @@ CoreInitializeMemoryServices (
>      PhitResourceHob = ResourceHob;
>      Found           = TRUE;
>  
> +    //
> +    // If a Memory Type Information Resource HOB was found and is the same
> +    // Resource HOB that describes the PHIT HOB, then ignore the Memory Type
> +    // Information Resource HOB.
> +    //
> +    if (MemoryTypeInformationResourceHob == PhitResourceHob) {
> +      MemoryTypeInformationResourceHob = NULL;
> +    }
> +
>      //
>      // Compute range between PHIT EfiMemoryTop and the end of the Resource Descriptor HOB
>      //
> @@ -2387,8 +2433,9 @@ CoreInitializeMemoryServices (
>    if (Length < MinimalMemorySizeNeeded) {
>      //
>      // Search all the resource descriptor HOBs from the highest possible addresses down for a memory
> -    // region that is big enough to initialize the DXE core.  Always skip the PHIT Resource HOB.
> -    // The max address must be within the physically addressible range for the processor.
> +    // region that is big enough to initialize the DXE core.  Always skip the PHIT Resource HOB
> +    // and the Memory Type Information Resource HOB. The max address must be within the physically
> +    // addressable range for the processor.
>      //
>      HighAddress = MAX_ALLOC_ADDRESS;
>      for (Hob.Raw = *HobStart; !END_OF_HOB_LIST (Hob); Hob.Raw = GET_NEXT_HOB (Hob)) {
> @@ -2399,6 +2446,13 @@ CoreInitializeMemoryServices (
>          continue;
>        }
>  
> +      //
> +      // Skip the Resource Descriptor HOB that contains Memory Type Information bins
> +      //
> +      if (Hob.ResourceDescriptor == MemoryTypeInformationResourceHob) {
> +        continue;
> +      }
> +
>        //
>        // Skip all HOBs except Resource Descriptor HOBs
>        //
> @@ -2466,6 +2520,18 @@ CoreInitializeMemoryServices (
>      Capabilities = CoreConvertResourceDescriptorHobAttributesToCapabilities (EfiGcdMemoryTypeSystemMemory, Attributes);
>    }
>  
> +  if (MemoryTypeInformationResourceHob != NULL) {
> +    //
> +    // If a Memory Type Information Resource HOB was found, then use the address
> +    // range of the  Memory Type Information Resource HOB as the preferred
> +    // address range for the Memory Type Information bins.
> +    //
> +    CoreSetMemoryTypeInformationRange (
> +      MemoryTypeInformationResourceHob->PhysicalStart,
> +      MemoryTypeInformationResourceHob->ResourceLength
> +      );
> +  }
> +
>    //
>    // Declare the very first memory region, so the EFI Memory Services are available.
>    //
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
> index 6497af573353..458c62090265 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> @@ -532,6 +532,107 @@ CoreLoadingFixedAddressHook (
>    return;
>  }
>  
> +/**
> +  Sets the preferred memory range to use for the Memory Type Information bins.
> +  This service must be called before fist call to CoreAddMemoryDescriptor().
> +
> +  If the location of the Memory Type Information bins has already been
> +  established or the size of the range provides is smaller than all the
> +  Memory Type Information bins, then the range provides is not used.
> +
> +  @param  Start   The start address of the Memory Type Information range.
> +  @param  Length  The size, in bytes, of the Memory Type Information range.
> +**/
> +VOID
> +CoreSetMemoryTypeInformationRange (
> +  IN EFI_PHYSICAL_ADDRESS  Start,
> +  IN UINT64                Length
> +  )
> +{
> +  EFI_PHYSICAL_ADDRESS  Top;
> +  EFI_MEMORY_TYPE       Type;
> +  UINTN                 Index;
> +  UINTN                 Size;
> +
> +  //
> +  // Return if Memory Type Information bin locations have already been set
> +  //
> +  if (mMemoryTypeInformationInitialized) {
> +    return;
> +  }
> +
> +  //
> +  // Return if size of the Memory Type Information bins is greater than Length
> +  //
> +  Size = 0;
> +  for (Index = 0; gMemoryTypeInformation[Index].Type != EfiMaxMemoryType; Index++) {
> +    //
> +    // Make sure the memory type in the gMemoryTypeInformation[] array is valid
> +    //
> +    Type = (EFI_MEMORY_TYPE)(gMemoryTypeInformation[Index].Type);
> +    if ((UINT32)Type > EfiMaxMemoryType) {
> +      continue;
> +    }
> +
> +    Size += EFI_PAGES_TO_SIZE (gMemoryTypeInformation[Index].NumberOfPages);
> +  }
> +
> +  if (Size > Length) {
> +    return;
> +  }
> +
> +  //
> +  // Loop through each memory type in the order specified by the
> +  // gMemoryTypeInformation[] array
> +  //
> +  Top = Start + Length;
> +  for (Index = 0; gMemoryTypeInformation[Index].Type != EfiMaxMemoryType; Index++) {
> +    //
> +    // Make sure the memory type in the gMemoryTypeInformation[] array is valid
> +    //
> +    Type = (EFI_MEMORY_TYPE)(gMemoryTypeInformation[Index].Type);
> +    if ((UINT32)Type > EfiMaxMemoryType) {
> +      continue;
> +    }
> +
> +    if (gMemoryTypeInformation[Index].NumberOfPages != 0) {
> +      mMemoryTypeStatistics[Type].MaximumAddress = Top - 1;
> +      Top                                       -= EFI_PAGES_TO_SIZE (gMemoryTypeInformation[Index].NumberOfPages);
> +      mMemoryTypeStatistics[Type].BaseAddress    = Top;
> +
> +      //
> +      // If the current base address is the lowest address so far, then update
> +      // the default maximum address
> +      //
> +      if (mMemoryTypeStatistics[Type].BaseAddress < mDefaultMaximumAddress) {
> +        mDefaultMaximumAddress = mMemoryTypeStatistics[Type].BaseAddress - 1;
> +      }
> +
> +      mMemoryTypeStatistics[Type].NumberOfPages   = gMemoryTypeInformation[Index].NumberOfPages;
> +      gMemoryTypeInformation[Index].NumberOfPages = 0;
> +    }
> +  }
> +
> +  //
> +  // If the number of pages reserved for a memory type is 0, then all
> +  // allocations for that type should be in the default range.
> +  //
> +  for (Type = (EFI_MEMORY_TYPE)0; Type < EfiMaxMemoryType; Type++) {
> +    for (Index = 0; gMemoryTypeInformation[Index].Type != EfiMaxMemoryType; Index++) {
> +      if (Type == (EFI_MEMORY_TYPE)gMemoryTypeInformation[Index].Type) {
> +        mMemoryTypeStatistics[Type].InformationIndex = Index;
> +      }
> +    }
> +
> +    mMemoryTypeStatistics[Type].CurrentNumberOfPages = 0;
> +    if (mMemoryTypeStatistics[Type].MaximumAddress == MAX_ALLOC_ADDRESS) {
> +      mMemoryTypeStatistics[Type].MaximumAddress = mDefaultMaximumAddress;
> +    }
> +  }
> +
> +  mMemoryTypeInformationInitialized = TRUE;
> +}
> +
>  /**
>    Called to initialize the memory map and add descriptors to
>    the current descriptor list.
> diff --git a/MdeModulePkg/Include/Guid/MemoryTypeInformation.h b/MdeModulePkg/Include/Guid/MemoryTypeInformation.h
> index e97d88765ece..65adcf478b80 100644
> --- a/MdeModulePkg/Include/Guid/MemoryTypeInformation.h
> +++ b/MdeModulePkg/Include/Guid/MemoryTypeInformation.h
> @@ -1,14 +1,18 @@
>  /** @file
>    This file defines:
> -  * Memory Type Information GUID for HOB and Variable.
> +  * Memory Type Information GUID for Guided HOB and Variable.
>    * Memory Type Information Variable Name.
>    * Memory Type Information GUID HOB data structure.
>  
> -  The memory type information HOB and variable can
> -  be used to store the information for each memory type in Variable or HOB.
> +  The memory type information HOB and variable can be used to store information
> +  for each memory type in Variable or HOB.
>  
> -Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
> -SPDX-License-Identifier: BSD-2-Clause-Patent
> +  The Memory Type Information GUID can also be optionally used as the Owner
> +  field of a Resource Descriptor HOB to provide the preferred memory range
> +  for the memory types described in the Memory Type Information GUID HOB.
> +
> +  Copyright (c) 2006, Intel Corporation. All rights reserved.<BR>
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
>  
>  **/
>  

Thanks. I've not checked everything in minute detail, but I've made a
reasonable effort to review this.

Some comments (no deal breaker though):

- the "Count" counting is a bit stricter than truly necessary. I'd
expect the Count++ to be exactly where the
"MemoryTypeInformationResourceHob = ResourceHob" assignment is as well.
However, the posted version is also fine; it's fine to be stricter than
necessary. (And the code matches the commit message.)

- the early return point under "locations have already been set" didn't
get a DEBUG_ERROR after all, but that's not a big deal either

- I find it a bit confusing that we trust the gMemoryTypeInformation
array to be terminated with Type==EfiMaxMemoryType, but we still need to
/ want to skip any elements with Type > EfiMaxMemoryType. If I trusted
the array to be terminated properly, I'd also trust it not to contain
entries with types that are even beyond the terminator type. Near the
end of that function, we also trust the Type fields in
gMemoryTypeInformation to be unique. But, still, more checking cannot hurt.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>



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

Re: [edk2-devel] [Patch v2 2/2] MdeModulePkg/Core/Dxe: Set MemoryTypeInfo bin range from HOB

[Michael D Kinney]

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Monday, January 29, 2024 12:07 PM
> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>; Li, Aaron
> <aaron.li@intel.com>; Liu, Yun Y <yun.y.liu@intel.com>; Andrew Fish
> <afish@apple.com>
> Subject: Re: [edk2-devel] [Patch v2 2/2] MdeModulePkg/Core/Dxe: Set
> MemoryTypeInfo bin range from HOB
> 
> On 1/29/24 04:45, Michael D Kinney wrote:
> > Provide an optional method for PEI to declare a specific address
> > range to use for the Memory Type Information bins. The current
> > algorithm uses heuristics that tends to place the Memory Type
> > Information bins in the same location, but memory configuration
> > changes across boots or algorithm changes across a firmware
> > updates could potentially change the Memory Type Information bin
> > location.
> >
> > If the HOB List contains a Resource Descriptor HOB that
> > describes tested system memory and has an Owner GUID of
> > gEfiMemoryTypeInformationGuid, then use the address range
> > described by the Resource Descriptor HOB as the preferred
> > location of the Memory Type Information bins. If this HOB is
> > not detected, then the current behavior is preserved.
> >
> > The HOB with an Owner GUID of gEfiMemoryTypeInformationGuid
> > is ignored for the following conditions:
> > * The HOB with an Owner GUID of gEfiMemoryTypeInformationGuid
> >   is smaller than the Memory Type Information bins.
> > * The HOB list contains more than one Resource Descriptor HOB
> >   with an owner GUID of gEfiMemoryTypeInformationGuid.
> > * The Resource Descriptor HOB with an Owner GUID of
> >   gEfiMemoryTypeInformationGuid is the same Resource Descriptor
> >   HOB that that describes the PHIT memory range.
> >
> > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > Cc: Aaron Li <aaron.li@intel.com>
> > Cc: Liu Yun <yun.y.liu@intel.com>
> > Cc: Andrew Fish <afish@apple.com>
> > Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
> > ---
> >  MdeModulePkg/Core/Dxe/DxeMain.h               |   6 ++
> >  MdeModulePkg/Core/Dxe/Gcd/Gcd.c               |  72 ++++++++++++-
> >  MdeModulePkg/Core/Dxe/Mem/Page.c              | 101
> ++++++++++++++++++
> >  .../Include/Guid/MemoryTypeInformation.h      |  14 ++-
> >  4 files changed, 185 insertions(+), 8 deletions(-)
> >
> > diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h
> b/MdeModulePkg/Core/Dxe/DxeMain.h
> > index 86a7be2f5188..53e26703f8c7 100644
> > --- a/MdeModulePkg/Core/Dxe/DxeMain.h
> > +++ b/MdeModulePkg/Core/Dxe/DxeMain.h
> > @@ -277,6 +277,12 @@ CoreInitializePool (
> >    VOID
> >    );
> >
> > +VOID
> > +CoreSetMemoryTypeInformationRange (
> > +  IN EFI_PHYSICAL_ADDRESS  Start,
> > +  IN UINT64                Length
> > +  );
> > +
> >  /**
> >    Called to initialize the memory map and add descriptors to
> >    the current descriptor list.
> > diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> > index 792cd2e0af23..c450d1bf2531 100644
> > --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> > +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> > @@ -2238,6 +2238,8 @@ CoreInitializeMemoryServices (
> >    EFI_HOB_HANDOFF_INFO_TABLE   *PhitHob;
> >    EFI_HOB_RESOURCE_DESCRIPTOR  *ResourceHob;
> >    EFI_HOB_RESOURCE_DESCRIPTOR  *PhitResourceHob;
> > +  EFI_HOB_RESOURCE_DESCRIPTOR  *MemoryTypeInformationResourceHob;
> > +  UINTN                        Count;
> >    EFI_PHYSICAL_ADDRESS         BaseAddress;
> >    UINT64                       Length;
> >    UINT64                       Attributes;
> > @@ -2289,12 +2291,47 @@ CoreInitializeMemoryServices (
> >    //
> >    // See if a Memory Type Information HOB is available
> >    //
> > -  GuidHob = GetFirstGuidHob (&gEfiMemoryTypeInformationGuid);
> > +  MemoryTypeInformationResourceHob = NULL;
> > +  GuidHob                          = GetFirstGuidHob
> (&gEfiMemoryTypeInformationGuid);
> >    if (GuidHob != NULL) {
> >      EfiMemoryTypeInformation = GET_GUID_HOB_DATA (GuidHob);
> >      DataSize                 = GET_GUID_HOB_DATA_SIZE (GuidHob);
> >      if ((EfiMemoryTypeInformation != NULL) && (DataSize > 0) &&
> (DataSize <= (EfiMaxMemoryType + 1) * sizeof
> (EFI_MEMORY_TYPE_INFORMATION))) {
> >        CopyMem (&gMemoryTypeInformation, EfiMemoryTypeInformation,
> DataSize);
> > +
> > +      //
> > +      // Look for Resource Descriptor HOB with a ResourceType of
> System Memory
> > +      // and an Owner GUID of gEfiMemoryTypeInformationGuid. If more
> than 1 is
> > +      // found, then set MemoryTypeInformationResourceHob to NULL.
> > +      //
> > +      Count = 0;
> > +      for (Hob.Raw = *HobStart; !END_OF_HOB_LIST (Hob); Hob.Raw =
> GET_NEXT_HOB (Hob)) {
> > +        if (GET_HOB_TYPE (Hob) != EFI_HOB_TYPE_RESOURCE_DESCRIPTOR) {
> > +          continue;
> > +        }
> > +
> > +        ResourceHob = Hob.ResourceDescriptor;
> > +        if (!CompareGuid (&ResourceHob->Owner,
> &gEfiMemoryTypeInformationGuid)) {
> > +          continue;
> > +        }
> > +
> > +        Count++;
> > +        if (ResourceHob->ResourceType != EFI_RESOURCE_SYSTEM_MEMORY)
> {
> > +          continue;
> > +        }
> > +
> > +        if ((ResourceHob->ResourceAttribute & MEMORY_ATTRIBUTE_MASK)
> != TESTED_MEMORY_ATTRIBUTES) {
> > +          continue;
> > +        }
> > +
> > +        if (ResourceHob->ResourceLength >=
> CalculateTotalMemoryBinSizeNeeded ()) {
> > +          MemoryTypeInformationResourceHob = ResourceHob;
> > +        }
> > +      }
> > +
> > +      if (Count > 1) {
> > +        MemoryTypeInformationResourceHob = NULL;
> > +      }
> >      }
> >    }
> >
> > @@ -2344,6 +2381,15 @@ CoreInitializeMemoryServices (
> >      PhitResourceHob = ResourceHob;
> >      Found           = TRUE;
> >
> > +    //
> > +    // If a Memory Type Information Resource HOB was found and is the
> same
> > +    // Resource HOB that describes the PHIT HOB, then ignore the
> Memory Type
> > +    // Information Resource HOB.
> > +    //
> > +    if (MemoryTypeInformationResourceHob == PhitResourceHob) {
> > +      MemoryTypeInformationResourceHob = NULL;
> > +    }
> > +
> >      //
> >      // Compute range between PHIT EfiMemoryTop and the end of the
> Resource Descriptor HOB
> >      //
> > @@ -2387,8 +2433,9 @@ CoreInitializeMemoryServices (
> >    if (Length < MinimalMemorySizeNeeded) {
> >      //
> >      // Search all the resource descriptor HOBs from the highest
> possible addresses down for a memory
> > -    // region that is big enough to initialize the DXE core.  Always
> skip the PHIT Resource HOB.
> > -    // The max address must be within the physically addressible
> range for the processor.
> > +    // region that is big enough to initialize the DXE core.  Always
> skip the PHIT Resource HOB
> > +    // and the Memory Type Information Resource HOB. The max address
> must be within the physically
> > +    // addressable range for the processor.
> >      //
> >      HighAddress = MAX_ALLOC_ADDRESS;
> >      for (Hob.Raw = *HobStart; !END_OF_HOB_LIST (Hob); Hob.Raw =
> GET_NEXT_HOB (Hob)) {
> > @@ -2399,6 +2446,13 @@ CoreInitializeMemoryServices (
> >          continue;
> >        }
> >
> > +      //
> > +      // Skip the Resource Descriptor HOB that contains Memory Type
> Information bins
> > +      //
> > +      if (Hob.ResourceDescriptor == MemoryTypeInformationResourceHob)
> {
> > +        continue;
> > +      }
> > +
> >        //
> >        // Skip all HOBs except Resource Descriptor HOBs
> >        //
> > @@ -2466,6 +2520,18 @@ CoreInitializeMemoryServices (
> >      Capabilities =
> CoreConvertResourceDescriptorHobAttributesToCapabilities
> (EfiGcdMemoryTypeSystemMemory, Attributes);
> >    }
> >
> > +  if (MemoryTypeInformationResourceHob != NULL) {
> > +    //
> > +    // If a Memory Type Information Resource HOB was found, then use
> the address
> > +    // range of the  Memory Type Information Resource HOB as the
> preferred
> > +    // address range for the Memory Type Information bins.
> > +    //
> > +    CoreSetMemoryTypeInformationRange (
> > +      MemoryTypeInformationResourceHob->PhysicalStart,
> > +      MemoryTypeInformationResourceHob->ResourceLength
> > +      );
> > +  }
> > +
> >    //
> >    // Declare the very first memory region, so the EFI Memory Services
> are available.
> >    //
> > diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c
> b/MdeModulePkg/Core/Dxe/Mem/Page.c
> > index 6497af573353..458c62090265 100644
> > --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> > +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> > @@ -532,6 +532,107 @@ CoreLoadingFixedAddressHook (
> >    return;
> >  }
> >
> > +/**
> > +  Sets the preferred memory range to use for the Memory Type
> Information bins.
> > +  This service must be called before fist call to
> CoreAddMemoryDescriptor().
> > +
> > +  If the location of the Memory Type Information bins has already
> been
> > +  established or the size of the range provides is smaller than all
> the
> > +  Memory Type Information bins, then the range provides is not used.
> > +
> > +  @param  Start   The start address of the Memory Type Information
> range.
> > +  @param  Length  The size, in bytes, of the Memory Type Information
> range.
> > +**/
> > +VOID
> > +CoreSetMemoryTypeInformationRange (
> > +  IN EFI_PHYSICAL_ADDRESS  Start,
> > +  IN UINT64                Length
> > +  )
> > +{
> > +  EFI_PHYSICAL_ADDRESS  Top;
> > +  EFI_MEMORY_TYPE       Type;
> > +  UINTN                 Index;
> > +  UINTN                 Size;
> > +
> > +  //
> > +  // Return if Memory Type Information bin locations have already
> been set
> > +  //
> > +  if (mMemoryTypeInformationInitialized) {
> > +    return;
> > +  }
> > +
> > +  //
> > +  // Return if size of the Memory Type Information bins is greater
> than Length
> > +  //
> > +  Size = 0;
> > +  for (Index = 0; gMemoryTypeInformation[Index].Type !=
> EfiMaxMemoryType; Index++) {
> > +    //
> > +    // Make sure the memory type in the gMemoryTypeInformation[]
> array is valid
> > +    //
> > +    Type = (EFI_MEMORY_TYPE)(gMemoryTypeInformation[Index].Type);
> > +    if ((UINT32)Type > EfiMaxMemoryType) {
> > +      continue;
> > +    }
> > +
> > +    Size += EFI_PAGES_TO_SIZE
> (gMemoryTypeInformation[Index].NumberOfPages);
> > +  }
> > +
> > +  if (Size > Length) {
> > +    return;
> > +  }
> > +
> > +  //
> > +  // Loop through each memory type in the order specified by the
> > +  // gMemoryTypeInformation[] array
> > +  //
> > +  Top = Start + Length;
> > +  for (Index = 0; gMemoryTypeInformation[Index].Type !=
> EfiMaxMemoryType; Index++) {
> > +    //
> > +    // Make sure the memory type in the gMemoryTypeInformation[]
> array is valid
> > +    //
> > +    Type = (EFI_MEMORY_TYPE)(gMemoryTypeInformation[Index].Type);
> > +    if ((UINT32)Type > EfiMaxMemoryType) {
> > +      continue;
> > +    }
> > +
> > +    if (gMemoryTypeInformation[Index].NumberOfPages != 0) {
> > +      mMemoryTypeStatistics[Type].MaximumAddress = Top - 1;
> > +      Top                                       -= EFI_PAGES_TO_SIZE
> (gMemoryTypeInformation[Index].NumberOfPages);
> > +      mMemoryTypeStatistics[Type].BaseAddress    = Top;
> > +
> > +      //
> > +      // If the current base address is the lowest address so far,
> then update
> > +      // the default maximum address
> > +      //
> > +      if (mMemoryTypeStatistics[Type].BaseAddress <
> mDefaultMaximumAddress) {
> > +        mDefaultMaximumAddress =
> mMemoryTypeStatistics[Type].BaseAddress - 1;
> > +      }
> > +
> > +      mMemoryTypeStatistics[Type].NumberOfPages   =
> gMemoryTypeInformation[Index].NumberOfPages;
> > +      gMemoryTypeInformation[Index].NumberOfPages = 0;
> > +    }
> > +  }
> > +
> > +  //
> > +  // If the number of pages reserved for a memory type is 0, then all
> > +  // allocations for that type should be in the default range.
> > +  //
> > +  for (Type = (EFI_MEMORY_TYPE)0; Type < EfiMaxMemoryType; Type++) {
> > +    for (Index = 0; gMemoryTypeInformation[Index].Type !=
> EfiMaxMemoryType; Index++) {
> > +      if (Type ==
> (EFI_MEMORY_TYPE)gMemoryTypeInformation[Index].Type) {
> > +        mMemoryTypeStatistics[Type].InformationIndex = Index;
> > +      }
> > +    }
> > +
> > +    mMemoryTypeStatistics[Type].CurrentNumberOfPages = 0;
> > +    if (mMemoryTypeStatistics[Type].MaximumAddress ==
> MAX_ALLOC_ADDRESS) {
> > +      mMemoryTypeStatistics[Type].MaximumAddress =
> mDefaultMaximumAddress;
> > +    }
> > +  }
> > +
> > +  mMemoryTypeInformationInitialized = TRUE;
> > +}
> > +
> >  /**
> >    Called to initialize the memory map and add descriptors to
> >    the current descriptor list.
> > diff --git a/MdeModulePkg/Include/Guid/MemoryTypeInformation.h
> b/MdeModulePkg/Include/Guid/MemoryTypeInformation.h
> > index e97d88765ece..65adcf478b80 100644
> > --- a/MdeModulePkg/Include/Guid/MemoryTypeInformation.h
> > +++ b/MdeModulePkg/Include/Guid/MemoryTypeInformation.h
> > @@ -1,14 +1,18 @@
> >  /** @file
> >    This file defines:
> > -  * Memory Type Information GUID for HOB and Variable.
> > +  * Memory Type Information GUID for Guided HOB and Variable.
> >    * Memory Type Information Variable Name.
> >    * Memory Type Information GUID HOB data structure.
> >
> > -  The memory type information HOB and variable can
> > -  be used to store the information for each memory type in Variable
> or HOB.
> > +  The memory type information HOB and variable can be used to store
> information
> > +  for each memory type in Variable or HOB.
> >
> > -Copyright (c) 2006 - 2018, Intel Corporation. All rights
> reserved.<BR>
> > -SPDX-License-Identifier: BSD-2-Clause-Patent
> > +  The Memory Type Information GUID can also be optionally used as the
> Owner
> > +  field of a Resource Descriptor HOB to provide the preferred memory
> range
> > +  for the memory types described in the Memory Type Information GUID
> HOB.
> > +
> > +  Copyright (c) 2006, Intel Corporation. All rights reserved.<BR>
> > +  SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> >  **/
> >
> 
> Thanks. I've not checked everything in minute detail, but I've made a
> reasonable effort to review this.
> 
> Some comments (no deal breaker though):
> 
> - the "Count" counting is a bit stricter than truly necessary. I'd
> expect the Count++ to be exactly where the
> "MemoryTypeInformationResourceHob = ResourceHob" assignment is as well.
> However, the posted version is also fine; it's fine to be stricter than
> necessary. (And the code matches the commit message.)
> 
> - the early return point under "locations have already been set" didn't
> get a DEBUG_ERROR after all, but that's not a big deal either

I posted a V3 with the DEBUG_ERROR message.  It likely arrived while you
were reviewing V2.

> 
> - I find it a bit confusing that we trust the gMemoryTypeInformation
> array to be terminated with Type==EfiMaxMemoryType, but we still need to
> / want to skip any elements with Type > EfiMaxMemoryType. If I trusted
> the array to be terminated properly, I'd also trust it not to contain
> entries with types that are even beyond the terminator type. Near the
> end of that function, we also trust the Type fields in
> gMemoryTypeInformation to be unique. But, still, more checking cannot
> hurt.

I agree there are some inconsistencies here.  It would be better if all
the code that operates on these arrays was also passed the exact size of
the array to guarantee checking beyond end of array. Would require another
pass through all the code to add the extra info and checks.

> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thank you for the detailed review.



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