[PATCH 0/3] Update GDB and RVCT symbols loading to use the Image base address

[PATCH 0/3] Update GDB and RVCT symbols loading to use the Image base address

[Marvin Häuser]

Both GDB and RVCT debugging libraries currently use .text address
based symbols loading for different reasons. As the current code is
making assumptions that are not guaranteed, update both usages to
use the Image base address instead.

Marvin Häuser (3):
  EmulatorPkg: Use Image base address for GDB symbols loading
  ArmPkg: Use Image base address for GDB symbols loading
  ArmPkg: Use Image base address for RVCT symbols loading

 ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c |  8 ++++----
 ArmPkg/Library/RvdPeCoffExtraActionLib/RvdPeCoffExtraActionLib.c     |  4 ++--
 EmulatorPkg/Unix/Host/Host.c                                         | 12 ++++++------
 EmulatorPkg/Unix/GdbRun.sh                                           |  2 +-
 EmulatorPkg/Unix/lldbefi.py                                          |  2 +-
 5 files changed, 14 insertions(+), 14 deletions(-)

-- 
2.31.1

[PATCH 1/3] EmulatorPkg: Use Image base address for GDB symbols loading

[Marvin Häuser]

GDB symbols are currently loaded by specifying the .text section
address. It is assumed to be the value of the PE/COFF SizeOfHeaders
field. This may not be the case for various reasons, including a
sufficiently strict Image section alignment. Use the "-o" parameter
to specify the Image base address instead. This works because the GCC
linker scripts are designed to emit Image section addresses that are
equal to those of the final PE/COFF Image.

Cc: Andrew Fish <afish@apple.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
 EmulatorPkg/Unix/Host/Host.c | 12 ++++++------
 EmulatorPkg/Unix/GdbRun.sh   |  2 +-
 EmulatorPkg/Unix/lldbefi.py  |  2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/EmulatorPkg/Unix/Host/Host.c b/EmulatorPkg/Unix/Host/Host.c
index b4e5510613c8..f5b7d6709e47 100644
--- a/EmulatorPkg/Unix/Host/Host.c
+++ b/EmulatorPkg/Unix/Host/Host.c
@@ -1042,7 +1042,7 @@ PrintLoadAddress (
   } else {
     fprintf (stderr,
       "0x%08lx Loading %s with entry point 0x%08lx\n",
-      (unsigned long)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders),
+      (unsigned long)ImageContext->ImageAddress,
       ImageContext->PdbPointer,
       (unsigned long)ImageContext->EntryPoint
       );
@@ -1148,7 +1148,7 @@ GdbScriptAddImage (
     if (FeaturePcdGet (PcdEmulatorLazyLoadSymbols)) {
       GdbTempFile = fopen (gGdbWorkingFileName, "a");
       if (GdbTempFile != NULL) {
-        long unsigned int SymbolsAddr = (long unsigned int)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders);
+        long unsigned int SymbolsAddr = (long unsigned int)ImageContext->ImageAddress;
         mScriptSymbolChangesCount++;
         fprintf (
           GdbTempFile,
@@ -1159,7 +1159,7 @@ GdbScriptAddImage (
           );
         fclose (GdbTempFile);
         // This is for the lldb breakpoint only
-        SecGdbScriptBreak (ImageContext->PdbPointer, strlen (ImageContext->PdbPointer) + 1, (long unsigned int)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders), 1);
+        SecGdbScriptBreak (ImageContext->PdbPointer, strlen (ImageContext->PdbPointer) + 1, (long unsigned int)ImageContext->ImageAddress, 1);
       } else {
         ASSERT (FALSE);
       }
@@ -1168,9 +1168,9 @@ GdbScriptAddImage (
       if (GdbTempFile != NULL) {
         fprintf (
           GdbTempFile,
-          "add-symbol-file %s 0x%08lx\n",
+          "add-symbol-file %s -o 0x%08lx\n",
           ImageContext->PdbPointer,
-          (long unsigned int)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders)
+          (long unsigned int)ImageContext->ImageAddress
           );
         fclose (GdbTempFile);
 
@@ -1180,7 +1180,7 @@ GdbScriptAddImage (
         // Also used for the lldb breakpoint script. The lldb breakpoint script does
         // not use the file, it uses the arguments.
         //
-        SecGdbScriptBreak (ImageContext->PdbPointer, strlen (ImageContext->PdbPointer) + 1, (long unsigned int)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders), 1);
+        SecGdbScriptBreak (ImageContext->PdbPointer, strlen (ImageContext->PdbPointer) + 1, (long unsigned int)ImageContext->ImageAddress, 1);
       } else {
         ASSERT (FALSE);
       }
diff --git a/EmulatorPkg/Unix/GdbRun.sh b/EmulatorPkg/Unix/GdbRun.sh
index b050ad5e2c5c..6fea1c9d5b82 100644
--- a/EmulatorPkg/Unix/GdbRun.sh
+++ b/EmulatorPkg/Unix/GdbRun.sh
@@ -41,7 +41,7 @@ set $SymbolFileChangesCount = 0
 #
 define AddFirmwareSymbolFile
   if $SymbolFileChangesCount < $arg0
-    add-symbol-file $arg1 $arg2
+    add-symbol-file $arg1 -o $arg2
     set $SymbolFileChangesCount = $arg0
   end
 end
diff --git a/EmulatorPkg/Unix/lldbefi.py b/EmulatorPkg/Unix/lldbefi.py
index c3fb2675cbc1..4f4c04509e58 100755
--- a/EmulatorPkg/Unix/lldbefi.py
+++ b/EmulatorPkg/Unix/lldbefi.py
@@ -395,7 +395,7 @@ def LoadEmulatorEfiSymbols(frame, bp_loc , internal_dict):
 
     debugger = frame.thread.process.target.debugger
     if frame.FindVariable ("AddSymbolFlag").GetValueAsUnsigned() == 1:
-        LoadAddress = frame.FindVariable ("LoadAddress").GetValueAsUnsigned() - 0x240
+        LoadAddress = frame.FindVariable ("LoadAddress").GetValueAsUnsigned()
 
         debugger.HandleCommand ("target modules add  %s" % FileName)
         print "target modules load --slid 0x%x %s" % (LoadAddress, FileName)
-- 
2.31.1

[PATCH 1/1] MdeModulePkg: Move PiSmmCoreMemoryAllocationLib into PiSmmCore

[Marvin Häuser]

PiSmmCoreMemoryAllocationLib duplicates private definitions of
PiSmmCore, namely the SMM_CORE_PRIVATE_DATA structure. Move this code
into PiSmmCore, so that the struct definition can be consumed
directly instead.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
 MdeModulePkg/{Library/PiSmmCoreMemoryAllocationLib/MemoryAllocationLib.c => Core/PiSmmCore/MemoryAllocation.c} |   3 +-
 MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf                                                                      |   1 +
 MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationLib.inf                             |   5 +-
 MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationProfileLib.inf                      |   6 +-
 MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationServices.h                          | 185 --------------------
 5 files changed, 10 insertions(+), 190 deletions(-)

diff --git a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/MemoryAllocationLib.c b/MdeModulePkg/Core/PiSmmCore/MemoryAllocation.c
similarity index 96%
rename from MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/MemoryAllocationLib.c
rename to MdeModulePkg/Core/PiSmmCore/MemoryAllocation.c
index fd20a779cdcc..fb99174c9d8d 100644
--- a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/MemoryAllocationLib.c
+++ b/MdeModulePkg/Core/PiSmmCore/MemoryAllocation.c
@@ -22,7 +22,8 @@
 #include <Library/UefiBootServicesTableLib.h>
 #include <Library/BaseMemoryLib.h>
 #include <Library/DebugLib.h>
-#include "PiSmmCoreMemoryAllocationServices.h"
+#include "PiSmmCore.h"
+#include "PiSmmCorePrivateData.h"
 
 #include <Library/MemoryProfileLib.h>
 
diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
index c8bfae3860fc..85628f927134 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
@@ -37,6 +37,7 @@ [Sources]
   SmiHandlerProfile.c
   HeapGuard.c
   HeapGuard.h
+  MemoryAllocation.c
 
 [Packages]
   MdePkg/MdePkg.dec
diff --git a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationLib.inf b/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationLib.inf
index 5c51c48b0b1e..8812c9604103 100644
--- a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationLib.inf
+++ b/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationLib.inf
@@ -19,6 +19,9 @@ [Defines]
   VERSION_STRING                 = 1.0
   PI_SPECIFICATION_VERSION       = 0x0001000A
   LIBRARY_CLASS                  = MemoryAllocationLib|SMM_CORE
+  #
+  # This function is defined in PiSmmCore.
+  #
   CONSTRUCTOR                    = PiSmmCoreMemoryAllocationLibConstructor
 
 #
@@ -28,8 +31,6 @@ [Defines]
 #
 
 [Sources]
-  MemoryAllocationLib.c
-  PiSmmCoreMemoryAllocationServices.h
   PiSmmCoreMemoryProfileLibNull.c
 
 [Packages]
diff --git a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationProfileLib.inf b/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationProfileLib.inf
index 89658c0f6ccb..c3b8a4fdce7b 100644
--- a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationProfileLib.inf
+++ b/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationProfileLib.inf
@@ -19,6 +19,9 @@ [Defines]
   VERSION_STRING                 = 1.0
   PI_SPECIFICATION_VERSION       = 0x0001000A
   LIBRARY_CLASS                  = MemoryAllocationLib|SMM_CORE
+  #
+  # This function is defined in PiSmmCore.
+  #
   CONSTRUCTOR                    = PiSmmCoreMemoryAllocationLibConstructor
   LIBRARY_CLASS                  = MemoryProfileLib|SMM_CORE
   CONSTRUCTOR                    = PiSmmCoreMemoryProfileLibConstructor
@@ -30,8 +33,6 @@ [Defines]
 #
 
 [Sources]
-  MemoryAllocationLib.c
-  PiSmmCoreMemoryAllocationServices.h
   PiSmmCoreMemoryProfileLib.c
   PiSmmCoreMemoryProfileServices.h
 
@@ -43,6 +44,7 @@ [LibraryClasses]
   DebugLib
   BaseMemoryLib
   UefiBootServicesTableLib
+  MemoryAllocationLib
 
 [Guids]
   gEdkiiMemoryProfileGuid   ## SOMETIMES_CONSUMES   ## GUID # Locate protocol
diff --git a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationServices.h b/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationServices.h
deleted file mode 100644
index 789fcf2c01ea..000000000000
--- a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationServices.h
+++ /dev/null
@@ -1,185 +0,0 @@
-/** @file
-  Contains function prototypes for Memory Services in the SMM Core.
-
-  This header file borrows the PiSmmCore Memory Allocation services as the primitive
-  for memory allocation.
-
-  Copyright (c) 2008 - 2018, Intel Corporation. All rights reserved.<BR>
-  SPDX-License-Identifier: BSD-2-Clause-Patent
-
-**/
-
-#ifndef _PI_SMM_CORE_MEMORY_ALLOCATION_SERVICES_H_
-#define _PI_SMM_CORE_MEMORY_ALLOCATION_SERVICES_H_
-
-//
-// It should be aligned with the definition in PiSmmCore.
-//
-typedef struct {
-  UINTN                           Signature;
-
-  ///
-  /// The ImageHandle passed into the entry point of the SMM IPL.  This ImageHandle
-  /// is used by the SMM Core to fill in the ParentImageHandle field of the Loaded
-  /// Image Protocol for each SMM Driver that is dispatched by the SMM Core.
-  ///
-  EFI_HANDLE                      SmmIplImageHandle;
-
-  ///
-  /// The number of SMRAM ranges passed from the SMM IPL to the SMM Core.  The SMM
-  /// Core uses these ranges of SMRAM to initialize the SMM Core memory manager.
-  ///
-  UINTN                           SmramRangeCount;
-
-  ///
-  /// A table of SMRAM ranges passed from the SMM IPL to the SMM Core.  The SMM
-  /// Core uses these ranges of SMRAM to initialize the SMM Core memory manager.
-  ///
-  EFI_SMRAM_DESCRIPTOR            *SmramRanges;
-
-  ///
-  /// The SMM Foundation Entry Point.  The SMM Core fills in this field when the
-  /// SMM Core is initialized.  The SMM IPL is responsbile for registering this entry
-  /// point with the SMM Configuration Protocol.  The SMM Configuration Protocol may
-  /// not be available at the time the SMM IPL and SMM Core are started, so the SMM IPL
-  /// sets up a protocol notification on the SMM Configuration Protocol and registers
-  /// the SMM Foundation Entry Point as soon as the SMM Configuration Protocol is
-  /// available.
-  ///
-  EFI_SMM_ENTRY_POINT             SmmEntryPoint;
-
-  ///
-  /// Boolean flag set to TRUE while an SMI is being processed by the SMM Core.
-  ///
-  BOOLEAN                         SmmEntryPointRegistered;
-
-  ///
-  /// Boolean flag set to TRUE while an SMI is being processed by the SMM Core.
-  ///
-  BOOLEAN                         InSmm;
-
-  ///
-  /// This field is set by the SMM Core then the SMM Core is initialized.  This field is
-  /// used by the SMM Base 2 Protocol and SMM Communication Protocol implementations in
-  /// the SMM IPL.
-  ///
-  EFI_SMM_SYSTEM_TABLE2           *Smst;
-
-  ///
-  /// This field is used by the SMM Communicatioon Protocol to pass a buffer into
-  /// a software SMI handler and for the software SMI handler to pass a buffer back to
-  /// the caller of the SMM Communication Protocol.
-  ///
-  VOID                            *CommunicationBuffer;
-
-  ///
-  /// This field is used by the SMM Communicatioon Protocol to pass the size of a buffer,
-  /// in bytes, into a software SMI handler and for the software SMI handler to pass the
-  /// size, in bytes, of a buffer back to the caller of the SMM Communication Protocol.
-  ///
-  UINTN                           BufferSize;
-
-  ///
-  /// This field is used by the SMM Communication Protocol to pass the return status from
-  /// a software SMI handler back to the caller of the SMM Communication Protocol.
-  ///
-  EFI_STATUS                      ReturnStatus;
-
-  EFI_PHYSICAL_ADDRESS            PiSmmCoreImageBase;
-  UINT64                          PiSmmCoreImageSize;
-  EFI_PHYSICAL_ADDRESS            PiSmmCoreEntryPoint;
-} SMM_CORE_PRIVATE_DATA;
-
-/**
-  Called to initialize the memory service.
-
-  @param   SmramRangeCount       Number of SMRAM Regions
-  @param   SmramRanges           Pointer to SMRAM Descriptors
-
-**/
-VOID
-SmmInitializeMemoryServices (
-  IN UINTN                 SmramRangeCount,
-  IN EFI_SMRAM_DESCRIPTOR  *SmramRanges
-  );
-
-/**
-  Allocates pages from the memory map.
-
-  @param  Type                   The type of allocation to perform
-  @param  MemoryType             The type of memory to turn the allocated pages
-                                 into
-  @param  NumberOfPages          The number of pages to allocate
-  @param  Memory                 A pointer to receive the base allocated memory
-                                 address
-
-  @retval EFI_INVALID_PARAMETER  Parameters violate checking rules defined in spec.
-  @retval EFI_NOT_FOUND          Could not allocate pages match the requirement.
-  @retval EFI_OUT_OF_RESOURCES   No enough pages to allocate.
-  @retval EFI_SUCCESS            Pages successfully allocated.
-
-**/
-EFI_STATUS
-EFIAPI
-SmmAllocatePages (
-  IN      EFI_ALLOCATE_TYPE         Type,
-  IN      EFI_MEMORY_TYPE           MemoryType,
-  IN      UINTN                     NumberOfPages,
-  OUT     EFI_PHYSICAL_ADDRESS      *Memory
-  );
-
-/**
-  Frees previous allocated pages.
-
-  @param  Memory                 Base address of memory being freed
-  @param  NumberOfPages          The number of pages to free
-
-  @retval EFI_NOT_FOUND          Could not find the entry that covers the range
-  @retval EFI_INVALID_PARAMETER  Address not aligned
-  @return EFI_SUCCESS            Pages successfully freed.
-
-**/
-EFI_STATUS
-EFIAPI
-SmmFreePages (
-  IN      EFI_PHYSICAL_ADDRESS      Memory,
-  IN      UINTN                     NumberOfPages
-  );
-
-/**
-  Allocate pool of a particular type.
-
-  @param  PoolType               Type of pool to allocate
-  @param  Size                   The amount of pool to allocate
-  @param  Buffer                 The address to return a pointer to the allocated
-                                 pool
-
-  @retval EFI_INVALID_PARAMETER  PoolType not valid
-  @retval EFI_OUT_OF_RESOURCES   Size exceeds max pool size or allocation failed.
-  @retval EFI_SUCCESS            Pool successfully allocated.
-
-**/
-EFI_STATUS
-EFIAPI
-SmmAllocatePool (
-  IN      EFI_MEMORY_TYPE           PoolType,
-  IN      UINTN                     Size,
-  OUT     VOID                      **Buffer
-  );
-
-/**
-  Frees pool.
-
-  @param  Buffer                 The allocated pool entry to free
-
-  @retval EFI_INVALID_PARAMETER  Buffer is not a valid value.
-  @retval EFI_SUCCESS            Pool successfully freed.
-
-**/
-EFI_STATUS
-EFIAPI
-SmmFreePool (
-  IN      VOID                      *Buffer
-  );
-
-#endif
-- 
2.31.1

Re: [PATCH 1/1] MdeModulePkg: Move PiSmmCoreMemoryAllocationLib into PiSmmCore

[Ni, Ray]

Marvin,
Your patch moves the memory allocation lib implementation to PiSmmCore.
Can you remove the PiSmmCoreMemoryAllocationLib.inf completely? (or what forbids you remove this lib instance?)

Thanks,
Ray

-----Original Message-----
From: Marvin Häuser <mhaeuser@posteo.de> 
Sent: Sunday, August 22, 2021 3:56 AM
To: devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Vitaly Cheptsov <vit9696@protonmail.com>
Subject: [PATCH 1/1] MdeModulePkg: Move PiSmmCoreMemoryAllocationLib into PiSmmCore

PiSmmCoreMemoryAllocationLib duplicates private definitions of
PiSmmCore, namely the SMM_CORE_PRIVATE_DATA structure. Move this code
into PiSmmCore, so that the struct definition can be consumed
directly instead.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
 MdeModulePkg/{Library/PiSmmCoreMemoryAllocationLib/MemoryAllocationLib.c => Core/PiSmmCore/MemoryAllocation.c} |   3 +-
 MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf                                                                      |   1 +
 MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationLib.inf                             |   5 +-
 MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationProfileLib.inf                      |   6 +-
 MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationServices.h                          | 185 --------------------
 5 files changed, 10 insertions(+), 190 deletions(-)

diff --git a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/MemoryAllocationLib.c b/MdeModulePkg/Core/PiSmmCore/MemoryAllocation.c
similarity index 96%
rename from MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/MemoryAllocationLib.c
rename to MdeModulePkg/Core/PiSmmCore/MemoryAllocation.c
index fd20a779cdcc..fb99174c9d8d 100644
--- a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/MemoryAllocationLib.c
+++ b/MdeModulePkg/Core/PiSmmCore/MemoryAllocation.c
@@ -22,7 +22,8 @@
 #include <Library/UefiBootServicesTableLib.h>

 #include <Library/BaseMemoryLib.h>

 #include <Library/DebugLib.h>

-#include "PiSmmCoreMemoryAllocationServices.h"

+#include "PiSmmCore.h"

+#include "PiSmmCorePrivateData.h"

 

 #include <Library/MemoryProfileLib.h>

 

diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
index c8bfae3860fc..85628f927134 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
@@ -37,6 +37,7 @@ [Sources]
   SmiHandlerProfile.c

   HeapGuard.c

   HeapGuard.h

+  MemoryAllocation.c

 

 [Packages]

   MdePkg/MdePkg.dec

diff --git a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationLib.inf b/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationLib.inf
index 5c51c48b0b1e..8812c9604103 100644
--- a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationLib.inf
+++ b/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationLib.inf
@@ -19,6 +19,9 @@ [Defines]
   VERSION_STRING                 = 1.0

   PI_SPECIFICATION_VERSION       = 0x0001000A

   LIBRARY_CLASS                  = MemoryAllocationLib|SMM_CORE

+  #

+  # This function is defined in PiSmmCore.

+  #

   CONSTRUCTOR                    = PiSmmCoreMemoryAllocationLibConstructor

 

 #

@@ -28,8 +31,6 @@ [Defines]
 #

 

 [Sources]

-  MemoryAllocationLib.c

-  PiSmmCoreMemoryAllocationServices.h

   PiSmmCoreMemoryProfileLibNull.c

 

 [Packages]

diff --git a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationProfileLib.inf b/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationProfileLib.inf
index 89658c0f6ccb..c3b8a4fdce7b 100644
--- a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationProfileLib.inf
+++ b/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationProfileLib.inf
@@ -19,6 +19,9 @@ [Defines]
   VERSION_STRING                 = 1.0

   PI_SPECIFICATION_VERSION       = 0x0001000A

   LIBRARY_CLASS                  = MemoryAllocationLib|SMM_CORE

+  #

+  # This function is defined in PiSmmCore.

+  #

   CONSTRUCTOR                    = PiSmmCoreMemoryAllocationLibConstructor

   LIBRARY_CLASS                  = MemoryProfileLib|SMM_CORE

   CONSTRUCTOR                    = PiSmmCoreMemoryProfileLibConstructor

@@ -30,8 +33,6 @@ [Defines]
 #

 

 [Sources]

-  MemoryAllocationLib.c

-  PiSmmCoreMemoryAllocationServices.h

   PiSmmCoreMemoryProfileLib.c

   PiSmmCoreMemoryProfileServices.h

 

@@ -43,6 +44,7 @@ [LibraryClasses]
   DebugLib

   BaseMemoryLib

   UefiBootServicesTableLib

+  MemoryAllocationLib

 

 [Guids]

   gEdkiiMemoryProfileGuid   ## SOMETIMES_CONSUMES   ## GUID # Locate protocol

diff --git a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationServices.h b/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationServices.h
deleted file mode 100644
index 789fcf2c01ea..000000000000
--- a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationServices.h
+++ /dev/null
@@ -1,185 +0,0 @@
-/** @file

-  Contains function prototypes for Memory Services in the SMM Core.

-

-  This header file borrows the PiSmmCore Memory Allocation services as the primitive

-  for memory allocation.

-

-  Copyright (c) 2008 - 2018, Intel Corporation. All rights reserved.<BR>

-  SPDX-License-Identifier: BSD-2-Clause-Patent

-

-**/

-

-#ifndef _PI_SMM_CORE_MEMORY_ALLOCATION_SERVICES_H_

-#define _PI_SMM_CORE_MEMORY_ALLOCATION_SERVICES_H_

-

-//

-// It should be aligned with the definition in PiSmmCore.

-//

-typedef struct {

-  UINTN                           Signature;

-

-  ///

-  /// The ImageHandle passed into the entry point of the SMM IPL.  This ImageHandle

-  /// is used by the SMM Core to fill in the ParentImageHandle field of the Loaded

-  /// Image Protocol for each SMM Driver that is dispatched by the SMM Core.

-  ///

-  EFI_HANDLE                      SmmIplImageHandle;

-

-  ///

-  /// The number of SMRAM ranges passed from the SMM IPL to the SMM Core.  The SMM

-  /// Core uses these ranges of SMRAM to initialize the SMM Core memory manager.

-  ///

-  UINTN                           SmramRangeCount;

-

-  ///

-  /// A table of SMRAM ranges passed from the SMM IPL to the SMM Core.  The SMM

-  /// Core uses these ranges of SMRAM to initialize the SMM Core memory manager.

-  ///

-  EFI_SMRAM_DESCRIPTOR            *SmramRanges;

-

-  ///

-  /// The SMM Foundation Entry Point.  The SMM Core fills in this field when the

-  /// SMM Core is initialized.  The SMM IPL is responsbile for registering this entry

-  /// point with the SMM Configuration Protocol.  The SMM Configuration Protocol may

-  /// not be available at the time the SMM IPL and SMM Core are started, so the SMM IPL

-  /// sets up a protocol notification on the SMM Configuration Protocol and registers

-  /// the SMM Foundation Entry Point as soon as the SMM Configuration Protocol is

-  /// available.

-  ///

-  EFI_SMM_ENTRY_POINT             SmmEntryPoint;

-

-  ///

-  /// Boolean flag set to TRUE while an SMI is being processed by the SMM Core.

-  ///

-  BOOLEAN                         SmmEntryPointRegistered;

-

-  ///

-  /// Boolean flag set to TRUE while an SMI is being processed by the SMM Core.

-  ///

-  BOOLEAN                         InSmm;

-

-  ///

-  /// This field is set by the SMM Core then the SMM Core is initialized.  This field is

-  /// used by the SMM Base 2 Protocol and SMM Communication Protocol implementations in

-  /// the SMM IPL.

-  ///

-  EFI_SMM_SYSTEM_TABLE2           *Smst;

-

-  ///

-  /// This field is used by the SMM Communicatioon Protocol to pass a buffer into

-  /// a software SMI handler and for the software SMI handler to pass a buffer back to

-  /// the caller of the SMM Communication Protocol.

-  ///

-  VOID                            *CommunicationBuffer;

-

-  ///

-  /// This field is used by the SMM Communicatioon Protocol to pass the size of a buffer,

-  /// in bytes, into a software SMI handler and for the software SMI handler to pass the

-  /// size, in bytes, of a buffer back to the caller of the SMM Communication Protocol.

-  ///

-  UINTN                           BufferSize;

-

-  ///

-  /// This field is used by the SMM Communication Protocol to pass the return status from

-  /// a software SMI handler back to the caller of the SMM Communication Protocol.

-  ///

-  EFI_STATUS                      ReturnStatus;

-

-  EFI_PHYSICAL_ADDRESS            PiSmmCoreImageBase;

-  UINT64                          PiSmmCoreImageSize;

-  EFI_PHYSICAL_ADDRESS            PiSmmCoreEntryPoint;

-} SMM_CORE_PRIVATE_DATA;

-

-/**

-  Called to initialize the memory service.

-

-  @param   SmramRangeCount       Number of SMRAM Regions

-  @param   SmramRanges           Pointer to SMRAM Descriptors

-

-**/

-VOID

-SmmInitializeMemoryServices (

-  IN UINTN                 SmramRangeCount,

-  IN EFI_SMRAM_DESCRIPTOR  *SmramRanges

-  );

-

-/**

-  Allocates pages from the memory map.

-

-  @param  Type                   The type of allocation to perform

-  @param  MemoryType             The type of memory to turn the allocated pages

-                                 into

-  @param  NumberOfPages          The number of pages to allocate

-  @param  Memory                 A pointer to receive the base allocated memory

-                                 address

-

-  @retval EFI_INVALID_PARAMETER  Parameters violate checking rules defined in spec.

-  @retval EFI_NOT_FOUND          Could not allocate pages match the requirement.

-  @retval EFI_OUT_OF_RESOURCES   No enough pages to allocate.

-  @retval EFI_SUCCESS            Pages successfully allocated.

-

-**/

-EFI_STATUS

-EFIAPI

-SmmAllocatePages (

-  IN      EFI_ALLOCATE_TYPE         Type,

-  IN      EFI_MEMORY_TYPE           MemoryType,

-  IN      UINTN                     NumberOfPages,

-  OUT     EFI_PHYSICAL_ADDRESS      *Memory

-  );

-

-/**

-  Frees previous allocated pages.

-

-  @param  Memory                 Base address of memory being freed

-  @param  NumberOfPages          The number of pages to free

-

-  @retval EFI_NOT_FOUND          Could not find the entry that covers the range

-  @retval EFI_INVALID_PARAMETER  Address not aligned

-  @return EFI_SUCCESS            Pages successfully freed.

-

-**/

-EFI_STATUS

-EFIAPI

-SmmFreePages (

-  IN      EFI_PHYSICAL_ADDRESS      Memory,

-  IN      UINTN                     NumberOfPages

-  );

-

-/**

-  Allocate pool of a particular type.

-

-  @param  PoolType               Type of pool to allocate

-  @param  Size                   The amount of pool to allocate

-  @param  Buffer                 The address to return a pointer to the allocated

-                                 pool

-

-  @retval EFI_INVALID_PARAMETER  PoolType not valid

-  @retval EFI_OUT_OF_RESOURCES   Size exceeds max pool size or allocation failed.

-  @retval EFI_SUCCESS            Pool successfully allocated.

-

-**/

-EFI_STATUS

-EFIAPI

-SmmAllocatePool (

-  IN      EFI_MEMORY_TYPE           PoolType,

-  IN      UINTN                     Size,

-  OUT     VOID                      **Buffer

-  );

-

-/**

-  Frees pool.

-

-  @param  Buffer                 The allocated pool entry to free

-

-  @retval EFI_INVALID_PARAMETER  Buffer is not a valid value.

-  @retval EFI_SUCCESS            Pool successfully freed.

-

-**/

-EFI_STATUS

-EFIAPI

-SmmFreePool (

-  IN      VOID                      *Buffer

-  );

-

-#endif

-- 
2.31.1

Re: [PATCH 1/1] MdeModulePkg: Move PiSmmCoreMemoryAllocationLib into PiSmmCore

[Marvin Häuser]

Hey Ray,

Thanks for your response!

1) It would disrupt platform builds that use this INF.
2) We'd need a new library to satisfy MemoryAllocationLib dependencies. 
If using the generic SMM one, libraries linked against the core would 
start using the indirect table calls over the direct calls for 
practically no reason, at least I see none at the moment.

More or less I saw no reason to do it, as this is a change that needs no 
platform maintainer attention, but if you have suggestion on how to 
improve the patch, I'd be open to it of course.

Best regards,
Marvin

On 01/09/2021 06:21, Ni, Ray wrote:
> Marvin,
> Your patch moves the memory allocation lib implementation to PiSmmCore.
> Can you remove the PiSmmCoreMemoryAllocationLib.inf completely? (or what forbids you remove this lib instance?)
>
> Thanks,
> Ray
>
> -----Original Message-----
> From: Marvin Häuser <mhaeuser@posteo.de>
> Sent: Sunday, August 22, 2021 3:56 AM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Vitaly Cheptsov <vit9696@protonmail.com>
> Subject: [PATCH 1/1] MdeModulePkg: Move PiSmmCoreMemoryAllocationLib into PiSmmCore
>
> PiSmmCoreMemoryAllocationLib duplicates private definitions of
> PiSmmCore, namely the SMM_CORE_PRIVATE_DATA structure. Move this code
> into PiSmmCore, so that the struct definition can be consumed
> directly instead.
>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Vitaly Cheptsov <vit9696@protonmail.com>
> Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
> ---
>   MdeModulePkg/{Library/PiSmmCoreMemoryAllocationLib/MemoryAllocationLib.c => Core/PiSmmCore/MemoryAllocation.c} |   3 +-
>   MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf                                                                      |   1 +
>   MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationLib.inf                             |   5 +-
>   MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationProfileLib.inf                      |   6 +-
>   MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationServices.h                          | 185 --------------------
>   5 files changed, 10 insertions(+), 190 deletions(-)
>
> diff --git a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/MemoryAllocationLib.c b/MdeModulePkg/Core/PiSmmCore/MemoryAllocation.c
> similarity index 96%
> rename from MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/MemoryAllocationLib.c
> rename to MdeModulePkg/Core/PiSmmCore/MemoryAllocation.c
> index fd20a779cdcc..fb99174c9d8d 100644
> --- a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/MemoryAllocationLib.c
> +++ b/MdeModulePkg/Core/PiSmmCore/MemoryAllocation.c
> @@ -22,7 +22,8 @@
>   #include <Library/UefiBootServicesTableLib.h>
>
>   #include <Library/BaseMemoryLib.h>
>
>   #include <Library/DebugLib.h>
>
> -#include "PiSmmCoreMemoryAllocationServices.h"
>
> +#include "PiSmmCore.h"
>
> +#include "PiSmmCorePrivateData.h"
>
>   
>
>   #include <Library/MemoryProfileLib.h>
>
>   
>
> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
> index c8bfae3860fc..85628f927134 100644
> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
> @@ -37,6 +37,7 @@ [Sources]
>     SmiHandlerProfile.c
>
>     HeapGuard.c
>
>     HeapGuard.h
>
> +  MemoryAllocation.c
>
>   
>
>   [Packages]
>
>     MdePkg/MdePkg.dec
>
> diff --git a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationLib.inf b/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationLib.inf
> index 5c51c48b0b1e..8812c9604103 100644
> --- a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationLib.inf
> +++ b/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationLib.inf
> @@ -19,6 +19,9 @@ [Defines]
>     VERSION_STRING                 = 1.0
>
>     PI_SPECIFICATION_VERSION       = 0x0001000A
>
>     LIBRARY_CLASS                  = MemoryAllocationLib|SMM_CORE
>
> +  #
>
> +  # This function is defined in PiSmmCore.
>
> +  #
>
>     CONSTRUCTOR                    = PiSmmCoreMemoryAllocationLibConstructor
>
>   
>
>   #
>
> @@ -28,8 +31,6 @@ [Defines]
>   #
>
>   
>
>   [Sources]
>
> -  MemoryAllocationLib.c
>
> -  PiSmmCoreMemoryAllocationServices.h
>
>     PiSmmCoreMemoryProfileLibNull.c
>
>   
>
>   [Packages]
>
> diff --git a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationProfileLib.inf b/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationProfileLib.inf
> index 89658c0f6ccb..c3b8a4fdce7b 100644
> --- a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationProfileLib.inf
> +++ b/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationProfileLib.inf
> @@ -19,6 +19,9 @@ [Defines]
>     VERSION_STRING                 = 1.0
>
>     PI_SPECIFICATION_VERSION       = 0x0001000A
>
>     LIBRARY_CLASS                  = MemoryAllocationLib|SMM_CORE
>
> +  #
>
> +  # This function is defined in PiSmmCore.
>
> +  #
>
>     CONSTRUCTOR                    = PiSmmCoreMemoryAllocationLibConstructor
>
>     LIBRARY_CLASS                  = MemoryProfileLib|SMM_CORE
>
>     CONSTRUCTOR                    = PiSmmCoreMemoryProfileLibConstructor
>
> @@ -30,8 +33,6 @@ [Defines]
>   #
>
>   
>
>   [Sources]
>
> -  MemoryAllocationLib.c
>
> -  PiSmmCoreMemoryAllocationServices.h
>
>     PiSmmCoreMemoryProfileLib.c
>
>     PiSmmCoreMemoryProfileServices.h
>
>   
>
> @@ -43,6 +44,7 @@ [LibraryClasses]
>     DebugLib
>
>     BaseMemoryLib
>
>     UefiBootServicesTableLib
>
> +  MemoryAllocationLib
>
>   
>
>   [Guids]
>
>     gEdkiiMemoryProfileGuid   ## SOMETIMES_CONSUMES   ## GUID # Locate protocol
>
> diff --git a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationServices.h b/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationServices.h
> deleted file mode 100644
> index 789fcf2c01ea..000000000000
> --- a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationServices.h
> +++ /dev/null
> @@ -1,185 +0,0 @@
> -/** @file
>
> -  Contains function prototypes for Memory Services in the SMM Core.
>
> -
>
> -  This header file borrows the PiSmmCore Memory Allocation services as the primitive
>
> -  for memory allocation.
>
> -
>
> -  Copyright (c) 2008 - 2018, Intel Corporation. All rights reserved.<BR>
>
> -  SPDX-License-Identifier: BSD-2-Clause-Patent
>
> -
>
> -**/
>
> -
>
> -#ifndef _PI_SMM_CORE_MEMORY_ALLOCATION_SERVICES_H_
>
> -#define _PI_SMM_CORE_MEMORY_ALLOCATION_SERVICES_H_
>
> -
>
> -//
>
> -// It should be aligned with the definition in PiSmmCore.
>
> -//
>
> -typedef struct {
>
> -  UINTN                           Signature;
>
> -
>
> -  ///
>
> -  /// The ImageHandle passed into the entry point of the SMM IPL.  This ImageHandle
>
> -  /// is used by the SMM Core to fill in the ParentImageHandle field of the Loaded
>
> -  /// Image Protocol for each SMM Driver that is dispatched by the SMM Core.
>
> -  ///
>
> -  EFI_HANDLE                      SmmIplImageHandle;
>
> -
>
> -  ///
>
> -  /// The number of SMRAM ranges passed from the SMM IPL to the SMM Core.  The SMM
>
> -  /// Core uses these ranges of SMRAM to initialize the SMM Core memory manager.
>
> -  ///
>
> -  UINTN                           SmramRangeCount;
>
> -
>
> -  ///
>
> -  /// A table of SMRAM ranges passed from the SMM IPL to the SMM Core.  The SMM
>
> -  /// Core uses these ranges of SMRAM to initialize the SMM Core memory manager.
>
> -  ///
>
> -  EFI_SMRAM_DESCRIPTOR            *SmramRanges;
>
> -
>
> -  ///
>
> -  /// The SMM Foundation Entry Point.  The SMM Core fills in this field when the
>
> -  /// SMM Core is initialized.  The SMM IPL is responsbile for registering this entry
>
> -  /// point with the SMM Configuration Protocol.  The SMM Configuration Protocol may
>
> -  /// not be available at the time the SMM IPL and SMM Core are started, so the SMM IPL
>
> -  /// sets up a protocol notification on the SMM Configuration Protocol and registers
>
> -  /// the SMM Foundation Entry Point as soon as the SMM Configuration Protocol is
>
> -  /// available.
>
> -  ///
>
> -  EFI_SMM_ENTRY_POINT             SmmEntryPoint;
>
> -
>
> -  ///
>
> -  /// Boolean flag set to TRUE while an SMI is being processed by the SMM Core.
>
> -  ///
>
> -  BOOLEAN                         SmmEntryPointRegistered;
>
> -
>
> -  ///
>
> -  /// Boolean flag set to TRUE while an SMI is being processed by the SMM Core.
>
> -  ///
>
> -  BOOLEAN                         InSmm;
>
> -
>
> -  ///
>
> -  /// This field is set by the SMM Core then the SMM Core is initialized.  This field is
>
> -  /// used by the SMM Base 2 Protocol and SMM Communication Protocol implementations in
>
> -  /// the SMM IPL.
>
> -  ///
>
> -  EFI_SMM_SYSTEM_TABLE2           *Smst;
>
> -
>
> -  ///
>
> -  /// This field is used by the SMM Communicatioon Protocol to pass a buffer into
>
> -  /// a software SMI handler and for the software SMI handler to pass a buffer back to
>
> -  /// the caller of the SMM Communication Protocol.
>
> -  ///
>
> -  VOID                            *CommunicationBuffer;
>
> -
>
> -  ///
>
> -  /// This field is used by the SMM Communicatioon Protocol to pass the size of a buffer,
>
> -  /// in bytes, into a software SMI handler and for the software SMI handler to pass the
>
> -  /// size, in bytes, of a buffer back to the caller of the SMM Communication Protocol.
>
> -  ///
>
> -  UINTN                           BufferSize;
>
> -
>
> -  ///
>
> -  /// This field is used by the SMM Communication Protocol to pass the return status from
>
> -  /// a software SMI handler back to the caller of the SMM Communication Protocol.
>
> -  ///
>
> -  EFI_STATUS                      ReturnStatus;
>
> -
>
> -  EFI_PHYSICAL_ADDRESS            PiSmmCoreImageBase;
>
> -  UINT64                          PiSmmCoreImageSize;
>
> -  EFI_PHYSICAL_ADDRESS            PiSmmCoreEntryPoint;
>
> -} SMM_CORE_PRIVATE_DATA;
>
> -
>
> -/**
>
> -  Called to initialize the memory service.
>
> -
>
> -  @param   SmramRangeCount       Number of SMRAM Regions
>
> -  @param   SmramRanges           Pointer to SMRAM Descriptors
>
> -
>
> -**/
>
> -VOID
>
> -SmmInitializeMemoryServices (
>
> -  IN UINTN                 SmramRangeCount,
>
> -  IN EFI_SMRAM_DESCRIPTOR  *SmramRanges
>
> -  );
>
> -
>
> -/**
>
> -  Allocates pages from the memory map.
>
> -
>
> -  @param  Type                   The type of allocation to perform
>
> -  @param  MemoryType             The type of memory to turn the allocated pages
>
> -                                 into
>
> -  @param  NumberOfPages          The number of pages to allocate
>
> -  @param  Memory                 A pointer to receive the base allocated memory
>
> -                                 address
>
> -
>
> -  @retval EFI_INVALID_PARAMETER  Parameters violate checking rules defined in spec.
>
> -  @retval EFI_NOT_FOUND          Could not allocate pages match the requirement.
>
> -  @retval EFI_OUT_OF_RESOURCES   No enough pages to allocate.
>
> -  @retval EFI_SUCCESS            Pages successfully allocated.
>
> -
>
> -**/
>
> -EFI_STATUS
>
> -EFIAPI
>
> -SmmAllocatePages (
>
> -  IN      EFI_ALLOCATE_TYPE         Type,
>
> -  IN      EFI_MEMORY_TYPE           MemoryType,
>
> -  IN      UINTN                     NumberOfPages,
>
> -  OUT     EFI_PHYSICAL_ADDRESS      *Memory
>
> -  );
>
> -
>
> -/**
>
> -  Frees previous allocated pages.
>
> -
>
> -  @param  Memory                 Base address of memory being freed
>
> -  @param  NumberOfPages          The number of pages to free
>
> -
>
> -  @retval EFI_NOT_FOUND          Could not find the entry that covers the range
>
> -  @retval EFI_INVALID_PARAMETER  Address not aligned
>
> -  @return EFI_SUCCESS            Pages successfully freed.
>
> -
>
> -**/
>
> -EFI_STATUS
>
> -EFIAPI
>
> -SmmFreePages (
>
> -  IN      EFI_PHYSICAL_ADDRESS      Memory,
>
> -  IN      UINTN                     NumberOfPages
>
> -  );
>
> -
>
> -/**
>
> -  Allocate pool of a particular type.
>
> -
>
> -  @param  PoolType               Type of pool to allocate
>
> -  @param  Size                   The amount of pool to allocate
>
> -  @param  Buffer                 The address to return a pointer to the allocated
>
> -                                 pool
>
> -
>
> -  @retval EFI_INVALID_PARAMETER  PoolType not valid
>
> -  @retval EFI_OUT_OF_RESOURCES   Size exceeds max pool size or allocation failed.
>
> -  @retval EFI_SUCCESS            Pool successfully allocated.
>
> -
>
> -**/
>
> -EFI_STATUS
>
> -EFIAPI
>
> -SmmAllocatePool (
>
> -  IN      EFI_MEMORY_TYPE           PoolType,
>
> -  IN      UINTN                     Size,
>
> -  OUT     VOID                      **Buffer
>
> -  );
>
> -
>
> -/**
>
> -  Frees pool.
>
> -
>
> -  @param  Buffer                 The allocated pool entry to free
>
> -
>
> -  @retval EFI_INVALID_PARAMETER  Buffer is not a valid value.
>
> -  @retval EFI_SUCCESS            Pool successfully freed.
>
> -
>
> -**/
>
> -EFI_STATUS
>
> -EFIAPI
>
> -SmmFreePool (
>
> -  IN      VOID                      *Buffer
>
> -  );
>
> -
>
> -#endif
>

Re: [PATCH 1/1] MdeModulePkg: Move PiSmmCoreMemoryAllocationLib into PiSmmCore

[Ni, Ray]

Overall, the patch looks good to me.
Can you remove the "CONSTRUCTOR                    = PiSmmCoreMemoryAllocationLibConstructor" from PiSmmCoreMemoryAllocationProfileLib.inf?
With that change, Reviewed-by: Ray Ni <ray.ni@intel.com>

More replies started with "[ray]".

-----Original Message-----
From: Marvin Häuser <mhaeuser@posteo.de> 
Sent: Wednesday, September 1, 2021 3:18 PM
To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>; Vitaly Cheptsov <vit9696@protonmail.com>
Subject: Re: [PATCH 1/1] MdeModulePkg: Move PiSmmCoreMemoryAllocationLib into PiSmmCore

Hey Ray,

Thanks for your response!

1) It would disrupt platform builds that use this INF.
[ray] I see:) I agree we cannot break platforms that list the INF path in DSC.


2) We'd need a new library to satisfy MemoryAllocationLib dependencies. 
If using the generic SMM one, libraries linked against the core would start using the indirect table calls over the direct calls for practically no reason, at least I see none at the moment.
[ray] I see:) For example. UefiLib linked by PiSmmCore depends on MemoryAllocationLib. We need to at least provide a dummy lib for it to pass the dependency check from base tools.

[ray] I thought you could let PiSmmCore directly call the PiSmmCoreMemoryAllocationLibConstructor () in entrypoint to eliminate the needs of referring the constructor in PiSmmCoreMemoryAllocationLib.inf.
        But then I realized that constructors of other libraries may call AllocatePages/Pool(). Calling PiSmmCoreMemoryAllocationLibConstructor() in entrypoint forbids those memory lib API calls from constructors.

More or less I saw no reason to do it, as this is a change that needs no platform maintainer attention, but if you have suggestion on how to improve the patch, I'd be open to it of course.

Best regards,
Marvin

On 01/09/2021 06:21, Ni, Ray wrote:
> Marvin,
> Your patch moves the memory allocation lib implementation to PiSmmCore.
> Can you remove the PiSmmCoreMemoryAllocationLib.inf completely? (or 
> what forbids you remove this lib instance?)
>
> Thanks,
> Ray
>
> -----Original Message-----
> From: Marvin Häuser <mhaeuser@posteo.de>
> Sent: Sunday, August 22, 2021 3:56 AM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A 
> <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray 
> <ray.ni@intel.com>; Vitaly Cheptsov <vit9696@protonmail.com>
> Subject: [PATCH 1/1] MdeModulePkg: Move PiSmmCoreMemoryAllocationLib 
> into PiSmmCore
>
> PiSmmCoreMemoryAllocationLib duplicates private definitions of 
> PiSmmCore, namely the SMM_CORE_PRIVATE_DATA structure. Move this code 
> into PiSmmCore, so that the struct definition can be consumed directly 
> instead.
>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Vitaly Cheptsov <vit9696@protonmail.com>
> Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
> ---
>   MdeModulePkg/{Library/PiSmmCoreMemoryAllocationLib/MemoryAllocationLib.c => Core/PiSmmCore/MemoryAllocation.c} |   3 +-
>   MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf                                                                      |   1 +
>   MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationLib.inf                             |   5 +-
>   MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationProfileLib.inf                      |   6 +-
>   MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationServices.h                          | 185 --------------------
>   5 files changed, 10 insertions(+), 190 deletions(-)
>
> diff --git 
> a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/MemoryAllocationLi
> b.c b/MdeModulePkg/Core/PiSmmCore/MemoryAllocation.c
> similarity index 96%
> rename from 
> MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/MemoryAllocationLib.
> c rename to MdeModulePkg/Core/PiSmmCore/MemoryAllocation.c
> index fd20a779cdcc..fb99174c9d8d 100644
> --- 
> a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/MemoryAllocationLi
> b.c
> +++ b/MdeModulePkg/Core/PiSmmCore/MemoryAllocation.c
> @@ -22,7 +22,8 @@
>   #include <Library/UefiBootServicesTableLib.h>
>
>   #include <Library/BaseMemoryLib.h>
>
>   #include <Library/DebugLib.h>
>
> -#include "PiSmmCoreMemoryAllocationServices.h"
>
> +#include "PiSmmCore.h"
>
> +#include "PiSmmCorePrivateData.h"
>
>   
>
>   #include <Library/MemoryProfileLib.h>
>
>   
>
> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf 
> b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
> index c8bfae3860fc..85628f927134 100644
> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
> @@ -37,6 +37,7 @@ [Sources]
>     SmiHandlerProfile.c
>
>     HeapGuard.c
>
>     HeapGuard.h
>
> +  MemoryAllocation.c
>
>   
>
>   [Packages]
>
>     MdePkg/MdePkg.dec
>
> diff --git 
> a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAll
> ocationLib.inf 
> b/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAll
> ocationLib.inf index 5c51c48b0b1e..8812c9604103 100644
> --- 
> a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAll
> ocationLib.inf
> +++ b/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemor
> +++ yAllocationLib.inf
> @@ -19,6 +19,9 @@ [Defines]
>     VERSION_STRING                 = 1.0
>
>     PI_SPECIFICATION_VERSION       = 0x0001000A
>
>     LIBRARY_CLASS                  = MemoryAllocationLib|SMM_CORE
>
> +  #
>
> +  # This function is defined in PiSmmCore.
>
> +  #
>
>     CONSTRUCTOR                    = PiSmmCoreMemoryAllocationLibConstructor
>
>   
>
>   #
>
> @@ -28,8 +31,6 @@ [Defines]
>   #
>
>   
>
>   [Sources]
>
> -  MemoryAllocationLib.c
>
> -  PiSmmCoreMemoryAllocationServices.h
>
>     PiSmmCoreMemoryProfileLibNull.c
>
>   
>
>   [Packages]
>
> diff --git 
> a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAll
> ocationProfileLib.inf 
> b/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAll
> ocationProfileLib.inf index 89658c0f6ccb..c3b8a4fdce7b 100644
> --- 
> a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAll
> ocationProfileLib.inf
> +++ b/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemor
> +++ yAllocationProfileLib.inf
> @@ -19,6 +19,9 @@ [Defines]
>     VERSION_STRING                 = 1.0
>
>     PI_SPECIFICATION_VERSION       = 0x0001000A
>
>     LIBRARY_CLASS                  = MemoryAllocationLib|SMM_CORE
>
> +  #
>
> +  # This function is defined in PiSmmCore.
>
> +  #
>
>     CONSTRUCTOR                    = PiSmmCoreMemoryAllocationLibConstructor
>
>     LIBRARY_CLASS                  = MemoryProfileLib|SMM_CORE
>
>     CONSTRUCTOR                    = PiSmmCoreMemoryProfileLibConstructor
>
> @@ -30,8 +33,6 @@ [Defines]
>   #
>
>   
>
>   [Sources]
>
> -  MemoryAllocationLib.c
>
> -  PiSmmCoreMemoryAllocationServices.h
>
>     PiSmmCoreMemoryProfileLib.c
>
>     PiSmmCoreMemoryProfileServices.h
>
>   
>
> @@ -43,6 +44,7 @@ [LibraryClasses]
>     DebugLib
>
>     BaseMemoryLib
>
>     UefiBootServicesTableLib
>
> +  MemoryAllocationLib
>
>   
>
>   [Guids]
>
>     gEdkiiMemoryProfileGuid   ## SOMETIMES_CONSUMES   ## GUID # Locate protocol
>
> diff --git 
> a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAll
> ocationServices.h 
> b/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAll
> ocationServices.h
> deleted file mode 100644
> index 789fcf2c01ea..000000000000
> --- 
> a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAll
> ocationServices.h
> +++ /dev/null
> @@ -1,185 +0,0 @@
> -/** @file
>
> -  Contains function prototypes for Memory Services in the SMM Core.
>
> -
>
> -  This header file borrows the PiSmmCore Memory Allocation services 
> as the primitive
>
> -  for memory allocation.
>
> -
>
> -  Copyright (c) 2008 - 2018, Intel Corporation. All rights 
> reserved.<BR>
>
> -  SPDX-License-Identifier: BSD-2-Clause-Patent
>
> -
>
> -**/
>
> -
>
> -#ifndef _PI_SMM_CORE_MEMORY_ALLOCATION_SERVICES_H_
>
> -#define _PI_SMM_CORE_MEMORY_ALLOCATION_SERVICES_H_
>
> -
>
> -//
>
> -// It should be aligned with the definition in PiSmmCore.
>
> -//
>
> -typedef struct {
>
> -  UINTN                           Signature;
>
> -
>
> -  ///
>
> -  /// The ImageHandle passed into the entry point of the SMM IPL.  
> This ImageHandle
>
> -  /// is used by the SMM Core to fill in the ParentImageHandle field 
> of the Loaded
>
> -  /// Image Protocol for each SMM Driver that is dispatched by the SMM Core.
>
> -  ///
>
> -  EFI_HANDLE                      SmmIplImageHandle;
>
> -
>
> -  ///
>
> -  /// The number of SMRAM ranges passed from the SMM IPL to the SMM 
> Core.  The SMM
>
> -  /// Core uses these ranges of SMRAM to initialize the SMM Core memory manager.
>
> -  ///
>
> -  UINTN                           SmramRangeCount;
>
> -
>
> -  ///
>
> -  /// A table of SMRAM ranges passed from the SMM IPL to the SMM 
> Core.  The SMM
>
> -  /// Core uses these ranges of SMRAM to initialize the SMM Core memory manager.
>
> -  ///
>
> -  EFI_SMRAM_DESCRIPTOR            *SmramRanges;
>
> -
>
> -  ///
>
> -  /// The SMM Foundation Entry Point.  The SMM Core fills in this 
> field when the
>
> -  /// SMM Core is initialized.  The SMM IPL is responsbile for 
> registering this entry
>
> -  /// point with the SMM Configuration Protocol.  The SMM 
> Configuration Protocol may
>
> -  /// not be available at the time the SMM IPL and SMM Core are 
> started, so the SMM IPL
>
> -  /// sets up a protocol notification on the SMM Configuration 
> Protocol and registers
>
> -  /// the SMM Foundation Entry Point as soon as the SMM Configuration 
> Protocol is
>
> -  /// available.
>
> -  ///
>
> -  EFI_SMM_ENTRY_POINT             SmmEntryPoint;
>
> -
>
> -  ///
>
> -  /// Boolean flag set to TRUE while an SMI is being processed by the SMM Core.
>
> -  ///
>
> -  BOOLEAN                         SmmEntryPointRegistered;
>
> -
>
> -  ///
>
> -  /// Boolean flag set to TRUE while an SMI is being processed by the SMM Core.
>
> -  ///
>
> -  BOOLEAN                         InSmm;
>
> -
>
> -  ///
>
> -  /// This field is set by the SMM Core then the SMM Core is 
> initialized.  This field is
>
> -  /// used by the SMM Base 2 Protocol and SMM Communication Protocol 
> implementations in
>
> -  /// the SMM IPL.
>
> -  ///
>
> -  EFI_SMM_SYSTEM_TABLE2           *Smst;
>
> -
>
> -  ///
>
> -  /// This field is used by the SMM Communicatioon Protocol to pass a 
> buffer into
>
> -  /// a software SMI handler and for the software SMI handler to pass 
> a buffer back to
>
> -  /// the caller of the SMM Communication Protocol.
>
> -  ///
>
> -  VOID                            *CommunicationBuffer;
>
> -
>
> -  ///
>
> -  /// This field is used by the SMM Communicatioon Protocol to pass 
> the size of a buffer,
>
> -  /// in bytes, into a software SMI handler and for the software SMI 
> handler to pass the
>
> -  /// size, in bytes, of a buffer back to the caller of the SMM Communication Protocol.
>
> -  ///
>
> -  UINTN                           BufferSize;
>
> -
>
> -  ///
>
> -  /// This field is used by the SMM Communication Protocol to pass 
> the return status from
>
> -  /// a software SMI handler back to the caller of the SMM Communication Protocol.
>
> -  ///
>
> -  EFI_STATUS                      ReturnStatus;
>
> -
>
> -  EFI_PHYSICAL_ADDRESS            PiSmmCoreImageBase;
>
> -  UINT64                          PiSmmCoreImageSize;
>
> -  EFI_PHYSICAL_ADDRESS            PiSmmCoreEntryPoint;
>
> -} SMM_CORE_PRIVATE_DATA;
>
> -
>
> -/**
>
> -  Called to initialize the memory service.
>
> -
>
> -  @param   SmramRangeCount       Number of SMRAM Regions
>
> -  @param   SmramRanges           Pointer to SMRAM Descriptors
>
> -
>
> -**/
>
> -VOID
>
> -SmmInitializeMemoryServices (
>
> -  IN UINTN                 SmramRangeCount,
>
> -  IN EFI_SMRAM_DESCRIPTOR  *SmramRanges
>
> -  );
>
> -
>
> -/**
>
> -  Allocates pages from the memory map.
>
> -
>
> -  @param  Type                   The type of allocation to perform
>
> -  @param  MemoryType             The type of memory to turn the allocated pages
>
> -                                 into
>
> -  @param  NumberOfPages          The number of pages to allocate
>
> -  @param  Memory                 A pointer to receive the base allocated memory
>
> -                                 address
>
> -
>
> -  @retval EFI_INVALID_PARAMETER  Parameters violate checking rules defined in spec.
>
> -  @retval EFI_NOT_FOUND          Could not allocate pages match the requirement.
>
> -  @retval EFI_OUT_OF_RESOURCES   No enough pages to allocate.
>
> -  @retval EFI_SUCCESS            Pages successfully allocated.
>
> -
>
> -**/
>
> -EFI_STATUS
>
> -EFIAPI
>
> -SmmAllocatePages (
>
> -  IN      EFI_ALLOCATE_TYPE         Type,
>
> -  IN      EFI_MEMORY_TYPE           MemoryType,
>
> -  IN      UINTN                     NumberOfPages,
>
> -  OUT     EFI_PHYSICAL_ADDRESS      *Memory
>
> -  );
>
> -
>
> -/**
>
> -  Frees previous allocated pages.
>
> -
>
> -  @param  Memory                 Base address of memory being freed
>
> -  @param  NumberOfPages          The number of pages to free
>
> -
>
> -  @retval EFI_NOT_FOUND          Could not find the entry that covers the range
>
> -  @retval EFI_INVALID_PARAMETER  Address not aligned
>
> -  @return EFI_SUCCESS            Pages successfully freed.
>
> -
>
> -**/
>
> -EFI_STATUS
>
> -EFIAPI
>
> -SmmFreePages (
>
> -  IN      EFI_PHYSICAL_ADDRESS      Memory,
>
> -  IN      UINTN                     NumberOfPages
>
> -  );
>
> -
>
> -/**
>
> -  Allocate pool of a particular type.
>
> -
>
> -  @param  PoolType               Type of pool to allocate
>
> -  @param  Size                   The amount of pool to allocate
>
> -  @param  Buffer                 The address to return a pointer to the allocated
>
> -                                 pool
>
> -
>
> -  @retval EFI_INVALID_PARAMETER  PoolType not valid
>
> -  @retval EFI_OUT_OF_RESOURCES   Size exceeds max pool size or allocation failed.
>
> -  @retval EFI_SUCCESS            Pool successfully allocated.
>
> -
>
> -**/
>
> -EFI_STATUS
>
> -EFIAPI
>
> -SmmAllocatePool (
>
> -  IN      EFI_MEMORY_TYPE           PoolType,
>
> -  IN      UINTN                     Size,
>
> -  OUT     VOID                      **Buffer
>
> -  );
>
> -
>
> -/**
>
> -  Frees pool.
>
> -
>
> -  @param  Buffer                 The allocated pool entry to free
>
> -
>
> -  @retval EFI_INVALID_PARAMETER  Buffer is not a valid value.
>
> -  @retval EFI_SUCCESS            Pool successfully freed.
>
> -
>
> -**/
>
> -EFI_STATUS
>
> -EFIAPI
>
> -SmmFreePool (
>
> -  IN      VOID                      *Buffer
>
> -  );
>
> -
>
> -#endif
>

Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: Move PiSmmCoreMemoryAllocationLib into PiSmmCore

[Marvin Häuser]

On 02/09/2021 12:53, Ni, Ray wrote:
> Overall, the patch looks good to me.

Thanks!

> Can you remove the "CONSTRUCTOR                    = PiSmmCoreMemoryAllocationLibConstructor" from PiSmmCoreMemoryAllocationProfileLib.inf?

And "LIBRARY_CLASS                  = MemoryAllocationLib|SMM_CORE" too? 
Otherwise this is a broken MemoryAllocationLib implementation. Removing 
this will break any platform that uses this implementation, but I cannot 
see any in the edk2 tree.

Best regards,
Marvin

> With that change, Reviewed-by: Ray Ni <ray.ni@intel.com>
>
> More replies started with "[ray]".
>
> -----Original Message-----
> From: Marvin Häuser <mhaeuser@posteo.de>
> Sent: Wednesday, September 1, 2021 3:18 PM
> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>; Vitaly Cheptsov <vit9696@protonmail.com>
> Subject: Re: [PATCH 1/1] MdeModulePkg: Move PiSmmCoreMemoryAllocationLib into PiSmmCore
>
> Hey Ray,
>
> Thanks for your response!
>
> 1) It would disrupt platform builds that use this INF.
> [ray] I see:) I agree we cannot break platforms that list the INF path in DSC.
>
>
> 2) We'd need a new library to satisfy MemoryAllocationLib dependencies.
> If using the generic SMM one, libraries linked against the core would start using the indirect table calls over the direct calls for practically no reason, at least I see none at the moment.
> [ray] I see:) For example. UefiLib linked by PiSmmCore depends on MemoryAllocationLib. We need to at least provide a dummy lib for it to pass the dependency check from base tools.
>
> [ray] I thought you could let PiSmmCore directly call the PiSmmCoreMemoryAllocationLibConstructor () in entrypoint to eliminate the needs of referring the constructor in PiSmmCoreMemoryAllocationLib.inf.
>          But then I realized that constructors of other libraries may call AllocatePages/Pool(). Calling PiSmmCoreMemoryAllocationLibConstructor() in entrypoint forbids those memory lib API calls from constructors.
>
> More or less I saw no reason to do it, as this is a change that needs no platform maintainer attention, but if you have suggestion on how to improve the patch, I'd be open to it of course.
>
> Best regards,
> Marvin
>
> On 01/09/2021 06:21, Ni, Ray wrote:
>> Marvin,
>> Your patch moves the memory allocation lib implementation to PiSmmCore.
>> Can you remove the PiSmmCoreMemoryAllocationLib.inf completely? (or
>> what forbids you remove this lib instance?)
>>
>> Thanks,
>> Ray
>>
>> -----Original Message-----
>> From: Marvin Häuser <mhaeuser@posteo.de>
>> Sent: Sunday, August 22, 2021 3:56 AM
>> To: devel@edk2.groups.io
>> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
>> <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray
>> <ray.ni@intel.com>; Vitaly Cheptsov <vit9696@protonmail.com>
>> Subject: [PATCH 1/1] MdeModulePkg: Move PiSmmCoreMemoryAllocationLib
>> into PiSmmCore
>>
>> PiSmmCoreMemoryAllocationLib duplicates private definitions of
>> PiSmmCore, namely the SMM_CORE_PRIVATE_DATA structure. Move this code
>> into PiSmmCore, so that the struct definition can be consumed directly
>> instead.
>>
>> Cc: Jian J Wang <jian.j.wang@intel.com>
>> Cc: Hao A Wu <hao.a.wu@intel.com>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Cc: Vitaly Cheptsov <vit9696@protonmail.com>
>> Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
>> ---
>>    MdeModulePkg/{Library/PiSmmCoreMemoryAllocationLib/MemoryAllocationLib.c => Core/PiSmmCore/MemoryAllocation.c} |   3 +-
>>    MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf                                                                      |   1 +
>>    MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationLib.inf                             |   5 +-
>>    MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationProfileLib.inf                      |   6 +-
>>    MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationServices.h                          | 185 --------------------
>>    5 files changed, 10 insertions(+), 190 deletions(-)
>>
>> diff --git
>> a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/MemoryAllocationLi
>> b.c b/MdeModulePkg/Core/PiSmmCore/MemoryAllocation.c
>> similarity index 96%
>> rename from
>> MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/MemoryAllocationLib.
>> c rename to MdeModulePkg/Core/PiSmmCore/MemoryAllocation.c
>> index fd20a779cdcc..fb99174c9d8d 100644
>> ---
>> a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/MemoryAllocationLi
>> b.c
>> +++ b/MdeModulePkg/Core/PiSmmCore/MemoryAllocation.c
>> @@ -22,7 +22,8 @@
>>    #include <Library/UefiBootServicesTableLib.h>
>>
>>    #include <Library/BaseMemoryLib.h>
>>
>>    #include <Library/DebugLib.h>
>>
>> -#include "PiSmmCoreMemoryAllocationServices.h"
>>
>> +#include "PiSmmCore.h"
>>
>> +#include "PiSmmCorePrivateData.h"
>>
>>    
>>
>>    #include <Library/MemoryProfileLib.h>
>>
>>    
>>
>> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
>> b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
>> index c8bfae3860fc..85628f927134 100644
>> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
>> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
>> @@ -37,6 +37,7 @@ [Sources]
>>      SmiHandlerProfile.c
>>
>>      HeapGuard.c
>>
>>      HeapGuard.h
>>
>> +  MemoryAllocation.c
>>
>>    
>>
>>    [Packages]
>>
>>      MdePkg/MdePkg.dec
>>
>> diff --git
>> a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAll
>> ocationLib.inf
>> b/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAll
>> ocationLib.inf index 5c51c48b0b1e..8812c9604103 100644
>> ---
>> a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAll
>> ocationLib.inf
>> +++ b/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemor
>> +++ yAllocationLib.inf
>> @@ -19,6 +19,9 @@ [Defines]
>>      VERSION_STRING                 = 1.0
>>
>>      PI_SPECIFICATION_VERSION       = 0x0001000A
>>
>>      LIBRARY_CLASS                  = MemoryAllocationLib|SMM_CORE
>>
>> +  #
>>
>> +  # This function is defined in PiSmmCore.
>>
>> +  #
>>
>>      CONSTRUCTOR                    = PiSmmCoreMemoryAllocationLibConstructor
>>
>>    
>>
>>    #
>>
>> @@ -28,8 +31,6 @@ [Defines]
>>    #
>>
>>    
>>
>>    [Sources]
>>
>> -  MemoryAllocationLib.c
>>
>> -  PiSmmCoreMemoryAllocationServices.h
>>
>>      PiSmmCoreMemoryProfileLibNull.c
>>
>>    
>>
>>    [Packages]
>>
>> diff --git
>> a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAll
>> ocationProfileLib.inf
>> b/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAll
>> ocationProfileLib.inf index 89658c0f6ccb..c3b8a4fdce7b 100644
>> ---
>> a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAll
>> ocationProfileLib.inf
>> +++ b/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemor
>> +++ yAllocationProfileLib.inf
>> @@ -19,6 +19,9 @@ [Defines]
>>      VERSION_STRING                 = 1.0
>>
>>      PI_SPECIFICATION_VERSION       = 0x0001000A
>>
>>      LIBRARY_CLASS                  = MemoryAllocationLib|SMM_CORE
>>
>> +  #
>>
>> +  # This function is defined in PiSmmCore.
>>
>> +  #
>>
>>      CONSTRUCTOR                    = PiSmmCoreMemoryAllocationLibConstructor
>>
>>      LIBRARY_CLASS                  = MemoryProfileLib|SMM_CORE
>>
>>      CONSTRUCTOR                    = PiSmmCoreMemoryProfileLibConstructor
>>
>> @@ -30,8 +33,6 @@ [Defines]
>>    #
>>
>>    
>>
>>    [Sources]
>>
>> -  MemoryAllocationLib.c
>>
>> -  PiSmmCoreMemoryAllocationServices.h
>>
>>      PiSmmCoreMemoryProfileLib.c
>>
>>      PiSmmCoreMemoryProfileServices.h
>>
>>    
>>
>> @@ -43,6 +44,7 @@ [LibraryClasses]
>>      DebugLib
>>
>>      BaseMemoryLib
>>
>>      UefiBootServicesTableLib
>>
>> +  MemoryAllocationLib
>>
>>    
>>
>>    [Guids]
>>
>>      gEdkiiMemoryProfileGuid   ## SOMETIMES_CONSUMES   ## GUID # Locate protocol
>>
>> diff --git
>> a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAll
>> ocationServices.h
>> b/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAll
>> ocationServices.h
>> deleted file mode 100644
>> index 789fcf2c01ea..000000000000
>> ---
>> a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAll
>> ocationServices.h
>> +++ /dev/null
>> @@ -1,185 +0,0 @@
>> -/** @file
>>
>> -  Contains function prototypes for Memory Services in the SMM Core.
>>
>> -
>>
>> -  This header file borrows the PiSmmCore Memory Allocation services
>> as the primitive
>>
>> -  for memory allocation.
>>
>> -
>>
>> -  Copyright (c) 2008 - 2018, Intel Corporation. All rights
>> reserved.<BR>
>>
>> -  SPDX-License-Identifier: BSD-2-Clause-Patent
>>
>> -
>>
>> -**/
>>
>> -
>>
>> -#ifndef _PI_SMM_CORE_MEMORY_ALLOCATION_SERVICES_H_
>>
>> -#define _PI_SMM_CORE_MEMORY_ALLOCATION_SERVICES_H_
>>
>> -
>>
>> -//
>>
>> -// It should be aligned with the definition in PiSmmCore.
>>
>> -//
>>
>> -typedef struct {
>>
>> -  UINTN                           Signature;
>>
>> -
>>
>> -  ///
>>
>> -  /// The ImageHandle passed into the entry point of the SMM IPL.
>> This ImageHandle
>>
>> -  /// is used by the SMM Core to fill in the ParentImageHandle field
>> of the Loaded
>>
>> -  /// Image Protocol for each SMM Driver that is dispatched by the SMM Core.
>>
>> -  ///
>>
>> -  EFI_HANDLE                      SmmIplImageHandle;
>>
>> -
>>
>> -  ///
>>
>> -  /// The number of SMRAM ranges passed from the SMM IPL to the SMM
>> Core.  The SMM
>>
>> -  /// Core uses these ranges of SMRAM to initialize the SMM Core memory manager.
>>
>> -  ///
>>
>> -  UINTN                           SmramRangeCount;
>>
>> -
>>
>> -  ///
>>
>> -  /// A table of SMRAM ranges passed from the SMM IPL to the SMM
>> Core.  The SMM
>>
>> -  /// Core uses these ranges of SMRAM to initialize the SMM Core memory manager.
>>
>> -  ///
>>
>> -  EFI_SMRAM_DESCRIPTOR            *SmramRanges;
>>
>> -
>>
>> -  ///
>>
>> -  /// The SMM Foundation Entry Point.  The SMM Core fills in this
>> field when the
>>
>> -  /// SMM Core is initialized.  The SMM IPL is responsbile for
>> registering this entry
>>
>> -  /// point with the SMM Configuration Protocol.  The SMM
>> Configuration Protocol may
>>
>> -  /// not be available at the time the SMM IPL and SMM Core are
>> started, so the SMM IPL
>>
>> -  /// sets up a protocol notification on the SMM Configuration
>> Protocol and registers
>>
>> -  /// the SMM Foundation Entry Point as soon as the SMM Configuration
>> Protocol is
>>
>> -  /// available.
>>
>> -  ///
>>
>> -  EFI_SMM_ENTRY_POINT             SmmEntryPoint;
>>
>> -
>>
>> -  ///
>>
>> -  /// Boolean flag set to TRUE while an SMI is being processed by the SMM Core.
>>
>> -  ///
>>
>> -  BOOLEAN                         SmmEntryPointRegistered;
>>
>> -
>>
>> -  ///
>>
>> -  /// Boolean flag set to TRUE while an SMI is being processed by the SMM Core.
>>
>> -  ///
>>
>> -  BOOLEAN                         InSmm;
>>
>> -
>>
>> -  ///
>>
>> -  /// This field is set by the SMM Core then the SMM Core is
>> initialized.  This field is
>>
>> -  /// used by the SMM Base 2 Protocol and SMM Communication Protocol
>> implementations in
>>
>> -  /// the SMM IPL.
>>
>> -  ///
>>
>> -  EFI_SMM_SYSTEM_TABLE2           *Smst;
>>
>> -
>>
>> -  ///
>>
>> -  /// This field is used by the SMM Communicatioon Protocol to pass a
>> buffer into
>>
>> -  /// a software SMI handler and for the software SMI handler to pass
>> a buffer back to
>>
>> -  /// the caller of the SMM Communication Protocol.
>>
>> -  ///
>>
>> -  VOID                            *CommunicationBuffer;
>>
>> -
>>
>> -  ///
>>
>> -  /// This field is used by the SMM Communicatioon Protocol to pass
>> the size of a buffer,
>>
>> -  /// in bytes, into a software SMI handler and for the software SMI
>> handler to pass the
>>
>> -  /// size, in bytes, of a buffer back to the caller of the SMM Communication Protocol.
>>
>> -  ///
>>
>> -  UINTN                           BufferSize;
>>
>> -
>>
>> -  ///
>>
>> -  /// This field is used by the SMM Communication Protocol to pass
>> the return status from
>>
>> -  /// a software SMI handler back to the caller of the SMM Communication Protocol.
>>
>> -  ///
>>
>> -  EFI_STATUS                      ReturnStatus;
>>
>> -
>>
>> -  EFI_PHYSICAL_ADDRESS            PiSmmCoreImageBase;
>>
>> -  UINT64                          PiSmmCoreImageSize;
>>
>> -  EFI_PHYSICAL_ADDRESS            PiSmmCoreEntryPoint;
>>
>> -} SMM_CORE_PRIVATE_DATA;
>>
>> -
>>
>> -/**
>>
>> -  Called to initialize the memory service.
>>
>> -
>>
>> -  @param   SmramRangeCount       Number of SMRAM Regions
>>
>> -  @param   SmramRanges           Pointer to SMRAM Descriptors
>>
>> -
>>
>> -**/
>>
>> -VOID
>>
>> -SmmInitializeMemoryServices (
>>
>> -  IN UINTN                 SmramRangeCount,
>>
>> -  IN EFI_SMRAM_DESCRIPTOR  *SmramRanges
>>
>> -  );
>>
>> -
>>
>> -/**
>>
>> -  Allocates pages from the memory map.
>>
>> -
>>
>> -  @param  Type                   The type of allocation to perform
>>
>> -  @param  MemoryType             The type of memory to turn the allocated pages
>>
>> -                                 into
>>
>> -  @param  NumberOfPages          The number of pages to allocate
>>
>> -  @param  Memory                 A pointer to receive the base allocated memory
>>
>> -                                 address
>>
>> -
>>
>> -  @retval EFI_INVALID_PARAMETER  Parameters violate checking rules defined in spec.
>>
>> -  @retval EFI_NOT_FOUND          Could not allocate pages match the requirement.
>>
>> -  @retval EFI_OUT_OF_RESOURCES   No enough pages to allocate.
>>
>> -  @retval EFI_SUCCESS            Pages successfully allocated.
>>
>> -
>>
>> -**/
>>
>> -EFI_STATUS
>>
>> -EFIAPI
>>
>> -SmmAllocatePages (
>>
>> -  IN      EFI_ALLOCATE_TYPE         Type,
>>
>> -  IN      EFI_MEMORY_TYPE           MemoryType,
>>
>> -  IN      UINTN                     NumberOfPages,
>>
>> -  OUT     EFI_PHYSICAL_ADDRESS      *Memory
>>
>> -  );
>>
>> -
>>
>> -/**
>>
>> -  Frees previous allocated pages.
>>
>> -
>>
>> -  @param  Memory                 Base address of memory being freed
>>
>> -  @param  NumberOfPages          The number of pages to free
>>
>> -
>>
>> -  @retval EFI_NOT_FOUND          Could not find the entry that covers the range
>>
>> -  @retval EFI_INVALID_PARAMETER  Address not aligned
>>
>> -  @return EFI_SUCCESS            Pages successfully freed.
>>
>> -
>>
>> -**/
>>
>> -EFI_STATUS
>>
>> -EFIAPI
>>
>> -SmmFreePages (
>>
>> -  IN      EFI_PHYSICAL_ADDRESS      Memory,
>>
>> -  IN      UINTN                     NumberOfPages
>>
>> -  );
>>
>> -
>>
>> -/**
>>
>> -  Allocate pool of a particular type.
>>
>> -
>>
>> -  @param  PoolType               Type of pool to allocate
>>
>> -  @param  Size                   The amount of pool to allocate
>>
>> -  @param  Buffer                 The address to return a pointer to the allocated
>>
>> -                                 pool
>>
>> -
>>
>> -  @retval EFI_INVALID_PARAMETER  PoolType not valid
>>
>> -  @retval EFI_OUT_OF_RESOURCES   Size exceeds max pool size or allocation failed.
>>
>> -  @retval EFI_SUCCESS            Pool successfully allocated.
>>
>> -
>>
>> -**/
>>
>> -EFI_STATUS
>>
>> -EFIAPI
>>
>> -SmmAllocatePool (
>>
>> -  IN      EFI_MEMORY_TYPE           PoolType,
>>
>> -  IN      UINTN                     Size,
>>
>> -  OUT     VOID                      **Buffer
>>
>> -  );
>>
>> -
>>
>> -/**
>>
>> -  Frees pool.
>>
>> -
>>
>> -  @param  Buffer                 The allocated pool entry to free
>>
>> -
>>
>> -  @retval EFI_INVALID_PARAMETER  Buffer is not a valid value.
>>
>> -  @retval EFI_SUCCESS            Pool successfully freed.
>>
>> -
>>
>> -**/
>>
>> -EFI_STATUS
>>
>> -EFIAPI
>>
>> -SmmFreePool (
>>
>> -  IN      VOID                      *Buffer
>>
>> -  );
>>
>> -
>>
>> -#endif
>>
>
>
> 
>
>

回复: [edk2-devel] [PATCH 1/1] MdeModulePkg: Move PiSmmCoreMemoryAllocationLib into PiSmmCore

[gaoliming]

Marvin:
  Yes. We need to keep CONSTRUCTOR and LIBRARY_CLASS in PiSmmCoreMemoryAllocationLib.inf that is required to make sure SmmCore build pass. Otherwise, we have to provide another PiSmmCore MemoryAllocationLib library instance INF in PiSmmCore module, and update every platform DSC to use it. To avoid the unnecessary impact to the platform, I prefer current change to keep MdeModulePkg\Library\PiSmmCoreMemoryAllocationLib.

Thanks
Liming
> -----邮件原件-----
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Marvin
> H?user
> 发送时间: 2021年9月2日 21:22
> 收件人: devel@edk2.groups.io; ray.ni@intel.com
> 抄送: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>; Vitaly Cheptsov
> <vit9696@protonmail.com>
> 主题: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: Move
> PiSmmCoreMemoryAllocationLib into PiSmmCore
> 
> On 02/09/2021 12:53, Ni, Ray wrote:
> > Overall, the patch looks good to me.
> 
> Thanks!
> 
> > Can you remove the "CONSTRUCTOR                    =
> PiSmmCoreMemoryAllocationLibConstructor" from
> PiSmmCoreMemoryAllocationProfileLib.inf?
> 
> And "LIBRARY_CLASS                  =
> MemoryAllocationLib|SMM_CORE" too?
> Otherwise this is a broken MemoryAllocationLib implementation. Removing
> this will break any platform that uses this implementation, but I cannot
> see any in the edk2 tree.
> 
> Best regards,
> Marvin
> 
> > With that change, Reviewed-by: Ray Ni <ray.ni@intel.com>
> >
> > More replies started with "[ray]".
> >
> > -----Original Message-----
> > From: Marvin Häuser <mhaeuser@posteo.de>
> > Sent: Wednesday, September 1, 2021 3:18 PM
> > To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>; Vitaly Cheptsov
> <vit9696@protonmail.com>
> > Subject: Re: [PATCH 1/1] MdeModulePkg: Move
> PiSmmCoreMemoryAllocationLib into PiSmmCore
> >
> > Hey Ray,
> >
> > Thanks for your response!
> >
> > 1) It would disrupt platform builds that use this INF.
> > [ray] I see:) I agree we cannot break platforms that list the INF path in DSC.
> >
> >
> > 2) We'd need a new library to satisfy MemoryAllocationLib dependencies.
> > If using the generic SMM one, libraries linked against the core would start
> using the indirect table calls over the direct calls for practically no reason, at
> least I see none at the moment.
> > [ray] I see:) For example. UefiLib linked by PiSmmCore depends on
> MemoryAllocationLib. We need to at least provide a dummy lib for it to pass
> the dependency check from base tools.
> >
> > [ray] I thought you could let PiSmmCore directly call the
> PiSmmCoreMemoryAllocationLibConstructor () in entrypoint to eliminate the
> needs of referring the constructor in PiSmmCoreMemoryAllocationLib.inf.
> >          But then I realized that constructors of other libraries may call
> AllocatePages/Pool(). Calling PiSmmCoreMemoryAllocationLibConstructor() in
> entrypoint forbids those memory lib API calls from constructors.
> >
> > More or less I saw no reason to do it, as this is a change that needs no
> platform maintainer attention, but if you have suggestion on how to improve
> the patch, I'd be open to it of course.
> >
> > Best regards,
> > Marvin
> >
> > On 01/09/2021 06:21, Ni, Ray wrote:
> >> Marvin,
> >> Your patch moves the memory allocation lib implementation to
> PiSmmCore.
> >> Can you remove the PiSmmCoreMemoryAllocationLib.inf completely? (or
> >> what forbids you remove this lib instance?)
> >>
> >> Thanks,
> >> Ray
> >>
> >> -----Original Message-----
> >> From: Marvin Häuser <mhaeuser@posteo.de>
> >> Sent: Sunday, August 22, 2021 3:56 AM
> >> To: devel@edk2.groups.io
> >> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> >> <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray
> >> <ray.ni@intel.com>; Vitaly Cheptsov <vit9696@protonmail.com>
> >> Subject: [PATCH 1/1] MdeModulePkg: Move
> PiSmmCoreMemoryAllocationLib
> >> into PiSmmCore
> >>
> >> PiSmmCoreMemoryAllocationLib duplicates private definitions of
> >> PiSmmCore, namely the SMM_CORE_PRIVATE_DATA structure. Move this
> code
> >> into PiSmmCore, so that the struct definition can be consumed directly
> >> instead.
> >>
> >> Cc: Jian J Wang <jian.j.wang@intel.com>
> >> Cc: Hao A Wu <hao.a.wu@intel.com>
> >> Cc: Eric Dong <eric.dong@intel.com>
> >> Cc: Ray Ni <ray.ni@intel.com>
> >> Cc: Vitaly Cheptsov <vit9696@protonmail.com>
> >> Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
> >> ---
> >>
> MdeModulePkg/{Library/PiSmmCoreMemoryAllocationLib/MemoryAllocation
> Lib.c => Core/PiSmmCore/MemoryAllocation.c} |   3 +-
> >>    MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
> |   1 +
> >>
> MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemor
> yAllocationLib.inf                             |   5 +-
> >>
> MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemor
> yAllocationProfileLib.inf                      |   6 +-
> >>
> MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemor
> yAllocationServices.h                          | 185 --------------------
> >>    5 files changed, 10 insertions(+), 190 deletions(-)
> >>
> >> diff --git
> >>
> a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/MemoryAllocatio
> nLi
> >> b.c b/MdeModulePkg/Core/PiSmmCore/MemoryAllocation.c
> >> similarity index 96%
> >> rename from
> >>
> MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/MemoryAllocationL
> ib.
> >> c rename to MdeModulePkg/Core/PiSmmCore/MemoryAllocation.c
> >> index fd20a779cdcc..fb99174c9d8d 100644
> >> ---
> >>
> a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/MemoryAllocatio
> nLi
> >> b.c
> >> +++ b/MdeModulePkg/Core/PiSmmCore/MemoryAllocation.c
> >> @@ -22,7 +22,8 @@
> >>    #include <Library/UefiBootServicesTableLib.h>
> >>
> >>    #include <Library/BaseMemoryLib.h>
> >>
> >>    #include <Library/DebugLib.h>
> >>
> >> -#include "PiSmmCoreMemoryAllocationServices.h"
> >>
> >> +#include "PiSmmCore.h"
> >>
> >> +#include "PiSmmCorePrivateData.h"
> >>
> >>
> >>
> >>    #include <Library/MemoryProfileLib.h>
> >>
> >>
> >>
> >> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
> >> b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
> >> index c8bfae3860fc..85628f927134 100644
> >> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
> >> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
> >> @@ -37,6 +37,7 @@ [Sources]
> >>      SmiHandlerProfile.c
> >>
> >>      HeapGuard.c
> >>
> >>      HeapGuard.h
> >>
> >> +  MemoryAllocation.c
> >>
> >>
> >>
> >>    [Packages]
> >>
> >>      MdePkg/MdePkg.dec
> >>
> >> diff --git
> >>
> a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMem
> oryAll
> >> ocationLib.inf
> >>
> b/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMem
> oryAll
> >> ocationLib.inf index 5c51c48b0b1e..8812c9604103 100644
> >> ---
> >>
> a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMem
> oryAll
> >> ocationLib.inf
> >> +++
> b/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMem
> or
> >> +++ yAllocationLib.inf
> >> @@ -19,6 +19,9 @@ [Defines]
> >>      VERSION_STRING                 = 1.0
> >>
> >>      PI_SPECIFICATION_VERSION       = 0x0001000A
> >>
> >>      LIBRARY_CLASS                  =
> MemoryAllocationLib|SMM_CORE
> >>
> >> +  #
> >>
> >> +  # This function is defined in PiSmmCore.
> >>
> >> +  #
> >>
> >>      CONSTRUCTOR                    =
> PiSmmCoreMemoryAllocationLibConstructor
> >>
> >>
> >>
> >>    #
> >>
> >> @@ -28,8 +31,6 @@ [Defines]
> >>    #
> >>
> >>
> >>
> >>    [Sources]
> >>
> >> -  MemoryAllocationLib.c
> >>
> >> -  PiSmmCoreMemoryAllocationServices.h
> >>
> >>      PiSmmCoreMemoryProfileLibNull.c
> >>
> >>
> >>
> >>    [Packages]
> >>
> >> diff --git
> >>
> a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMem
> oryAll
> >> ocationProfileLib.inf
> >>
> b/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMem
> oryAll
> >> ocationProfileLib.inf index 89658c0f6ccb..c3b8a4fdce7b 100644
> >> ---
> >>
> a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMem
> oryAll
> >> ocationProfileLib.inf
> >> +++
> b/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMem
> or
> >> +++ yAllocationProfileLib.inf
> >> @@ -19,6 +19,9 @@ [Defines]
> >>      VERSION_STRING                 = 1.0
> >>
> >>      PI_SPECIFICATION_VERSION       = 0x0001000A
> >>
> >>      LIBRARY_CLASS                  =
> MemoryAllocationLib|SMM_CORE
> >>
> >> +  #
> >>
> >> +  # This function is defined in PiSmmCore.
> >>
> >> +  #
> >>
> >>      CONSTRUCTOR                    =
> PiSmmCoreMemoryAllocationLibConstructor
> >>
> >>      LIBRARY_CLASS                  =
> MemoryProfileLib|SMM_CORE
> >>
> >>      CONSTRUCTOR                    =
> PiSmmCoreMemoryProfileLibConstructor
> >>
> >> @@ -30,8 +33,6 @@ [Defines]
> >>    #
> >>
> >>
> >>
> >>    [Sources]
> >>
> >> -  MemoryAllocationLib.c
> >>
> >> -  PiSmmCoreMemoryAllocationServices.h
> >>
> >>      PiSmmCoreMemoryProfileLib.c
> >>
> >>      PiSmmCoreMemoryProfileServices.h
> >>
> >>
> >>
> >> @@ -43,6 +44,7 @@ [LibraryClasses]
> >>      DebugLib
> >>
> >>      BaseMemoryLib
> >>
> >>      UefiBootServicesTableLib
> >>
> >> +  MemoryAllocationLib
> >>
> >>
> >>
> >>    [Guids]
> >>
> >>      gEdkiiMemoryProfileGuid   ## SOMETIMES_CONSUMES   ##
> GUID # Locate protocol
> >>
> >> diff --git
> >>
> a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMem
> oryAll
> >> ocationServices.h
> >>
> b/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMem
> oryAll
> >> ocationServices.h
> >> deleted file mode 100644
> >> index 789fcf2c01ea..000000000000
> >> ---
> >>
> a/MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMem
> oryAll
> >> ocationServices.h
> >> +++ /dev/null
> >> @@ -1,185 +0,0 @@
> >> -/** @file
> >>
> >> -  Contains function prototypes for Memory Services in the SMM Core.
> >>
> >> -
> >>
> >> -  This header file borrows the PiSmmCore Memory Allocation services
> >> as the primitive
> >>
> >> -  for memory allocation.
> >>
> >> -
> >>
> >> -  Copyright (c) 2008 - 2018, Intel Corporation. All rights
> >> reserved.<BR>
> >>
> >> -  SPDX-License-Identifier: BSD-2-Clause-Patent
> >>
> >> -
> >>
> >> -**/
> >>
> >> -
> >>
> >> -#ifndef _PI_SMM_CORE_MEMORY_ALLOCATION_SERVICES_H_
> >>
> >> -#define _PI_SMM_CORE_MEMORY_ALLOCATION_SERVICES_H_
> >>
> >> -
> >>
> >> -//
> >>
> >> -// It should be aligned with the definition in PiSmmCore.
> >>
> >> -//
> >>
> >> -typedef struct {
> >>
> >> -  UINTN                           Signature;
> >>
> >> -
> >>
> >> -  ///
> >>
> >> -  /// The ImageHandle passed into the entry point of the SMM IPL.
> >> This ImageHandle
> >>
> >> -  /// is used by the SMM Core to fill in the ParentImageHandle field
> >> of the Loaded
> >>
> >> -  /// Image Protocol for each SMM Driver that is dispatched by the SMM
> Core.
> >>
> >> -  ///
> >>
> >> -  EFI_HANDLE                      SmmIplImageHandle;
> >>
> >> -
> >>
> >> -  ///
> >>
> >> -  /// The number of SMRAM ranges passed from the SMM IPL to the
> SMM
> >> Core.  The SMM
> >>
> >> -  /// Core uses these ranges of SMRAM to initialize the SMM Core
> memory manager.
> >>
> >> -  ///
> >>
> >> -  UINTN                           SmramRangeCount;
> >>
> >> -
> >>
> >> -  ///
> >>
> >> -  /// A table of SMRAM ranges passed from the SMM IPL to the SMM
> >> Core.  The SMM
> >>
> >> -  /// Core uses these ranges of SMRAM to initialize the SMM Core
> memory manager.
> >>
> >> -  ///
> >>
> >> -  EFI_SMRAM_DESCRIPTOR            *SmramRanges;
> >>
> >> -
> >>
> >> -  ///
> >>
> >> -  /// The SMM Foundation Entry Point.  The SMM Core fills in this
> >> field when the
> >>
> >> -  /// SMM Core is initialized.  The SMM IPL is responsbile for
> >> registering this entry
> >>
> >> -  /// point with the SMM Configuration Protocol.  The SMM
> >> Configuration Protocol may
> >>
> >> -  /// not be available at the time the SMM IPL and SMM Core are
> >> started, so the SMM IPL
> >>
> >> -  /// sets up a protocol notification on the SMM Configuration
> >> Protocol and registers
> >>
> >> -  /// the SMM Foundation Entry Point as soon as the SMM Configuration
> >> Protocol is
> >>
> >> -  /// available.
> >>
> >> -  ///
> >>
> >> -  EFI_SMM_ENTRY_POINT             SmmEntryPoint;
> >>
> >> -
> >>
> >> -  ///
> >>
> >> -  /// Boolean flag set to TRUE while an SMI is being processed by the
> SMM Core.
> >>
> >> -  ///
> >>
> >> -  BOOLEAN                         SmmEntryPointRegistered;
> >>
> >> -
> >>
> >> -  ///
> >>
> >> -  /// Boolean flag set to TRUE while an SMI is being processed by the
> SMM Core.
> >>
> >> -  ///
> >>
> >> -  BOOLEAN                         InSmm;
> >>
> >> -
> >>
> >> -  ///
> >>
> >> -  /// This field is set by the SMM Core then the SMM Core is
> >> initialized.  This field is
> >>
> >> -  /// used by the SMM Base 2 Protocol and SMM Communication
> Protocol
> >> implementations in
> >>
> >> -  /// the SMM IPL.
> >>
> >> -  ///
> >>
> >> -  EFI_SMM_SYSTEM_TABLE2           *Smst;
> >>
> >> -
> >>
> >> -  ///
> >>
> >> -  /// This field is used by the SMM Communicatioon Protocol to pass a
> >> buffer into
> >>
> >> -  /// a software SMI handler and for the software SMI handler to pass
> >> a buffer back to
> >>
> >> -  /// the caller of the SMM Communication Protocol.
> >>
> >> -  ///
> >>
> >> -  VOID                            *CommunicationBuffer;
> >>
> >> -
> >>
> >> -  ///
> >>
> >> -  /// This field is used by the SMM Communicatioon Protocol to pass
> >> the size of a buffer,
> >>
> >> -  /// in bytes, into a software SMI handler and for the software SMI
> >> handler to pass the
> >>
> >> -  /// size, in bytes, of a buffer back to the caller of the SMM
> Communication Protocol.
> >>
> >> -  ///
> >>
> >> -  UINTN                           BufferSize;
> >>
> >> -
> >>
> >> -  ///
> >>
> >> -  /// This field is used by the SMM Communication Protocol to pass
> >> the return status from
> >>
> >> -  /// a software SMI handler back to the caller of the SMM
> Communication Protocol.
> >>
> >> -  ///
> >>
> >> -  EFI_STATUS                      ReturnStatus;
> >>
> >> -
> >>
> >> -  EFI_PHYSICAL_ADDRESS            PiSmmCoreImageBase;
> >>
> >> -  UINT64                          PiSmmCoreImageSize;
> >>
> >> -  EFI_PHYSICAL_ADDRESS            PiSmmCoreEntryPoint;
> >>
> >> -} SMM_CORE_PRIVATE_DATA;
> >>
> >> -
> >>
> >> -/**
> >>
> >> -  Called to initialize the memory service.
> >>
> >> -
> >>
> >> -  @param   SmramRangeCount       Number of SMRAM Regions
> >>
> >> -  @param   SmramRanges           Pointer to SMRAM Descriptors
> >>
> >> -
> >>
> >> -**/
> >>
> >> -VOID
> >>
> >> -SmmInitializeMemoryServices (
> >>
> >> -  IN UINTN                 SmramRangeCount,
> >>
> >> -  IN EFI_SMRAM_DESCRIPTOR  *SmramRanges
> >>
> >> -  );
> >>
> >> -
> >>
> >> -/**
> >>
> >> -  Allocates pages from the memory map.
> >>
> >> -
> >>
> >> -  @param  Type                   The type of allocation to
> perform
> >>
> >> -  @param  MemoryType             The type of memory to turn
> the allocated pages
> >>
> >> -                                 into
> >>
> >> -  @param  NumberOfPages          The number of pages to
> allocate
> >>
> >> -  @param  Memory                 A pointer to receive the base
> allocated memory
> >>
> >> -                                 address
> >>
> >> -
> >>
> >> -  @retval EFI_INVALID_PARAMETER  Parameters violate checking rules
> defined in spec.
> >>
> >> -  @retval EFI_NOT_FOUND          Could not allocate pages match
> the requirement.
> >>
> >> -  @retval EFI_OUT_OF_RESOURCES   No enough pages to allocate.
> >>
> >> -  @retval EFI_SUCCESS            Pages successfully allocated.
> >>
> >> -
> >>
> >> -**/
> >>
> >> -EFI_STATUS
> >>
> >> -EFIAPI
> >>
> >> -SmmAllocatePages (
> >>
> >> -  IN      EFI_ALLOCATE_TYPE         Type,
> >>
> >> -  IN      EFI_MEMORY_TYPE           MemoryType,
> >>
> >> -  IN      UINTN                     NumberOfPages,
> >>
> >> -  OUT     EFI_PHYSICAL_ADDRESS      *Memory
> >>
> >> -  );
> >>
> >> -
> >>
> >> -/**
> >>
> >> -  Frees previous allocated pages.
> >>
> >> -
> >>
> >> -  @param  Memory                 Base address of memory being
> freed
> >>
> >> -  @param  NumberOfPages          The number of pages to free
> >>
> >> -
> >>
> >> -  @retval EFI_NOT_FOUND          Could not find the entry that
> covers the range
> >>
> >> -  @retval EFI_INVALID_PARAMETER  Address not aligned
> >>
> >> -  @return EFI_SUCCESS            Pages successfully freed.
> >>
> >> -
> >>
> >> -**/
> >>
> >> -EFI_STATUS
> >>
> >> -EFIAPI
> >>
> >> -SmmFreePages (
> >>
> >> -  IN      EFI_PHYSICAL_ADDRESS      Memory,
> >>
> >> -  IN      UINTN                     NumberOfPages
> >>
> >> -  );
> >>
> >> -
> >>
> >> -/**
> >>
> >> -  Allocate pool of a particular type.
> >>
> >> -
> >>
> >> -  @param  PoolType               Type of pool to allocate
> >>
> >> -  @param  Size                   The amount of pool to allocate
> >>
> >> -  @param  Buffer                 The address to return a pointer
> to the allocated
> >>
> >> -                                 pool
> >>
> >> -
> >>
> >> -  @retval EFI_INVALID_PARAMETER  PoolType not valid
> >>
> >> -  @retval EFI_OUT_OF_RESOURCES   Size exceeds max pool size or
> allocation failed.
> >>
> >> -  @retval EFI_SUCCESS            Pool successfully allocated.
> >>
> >> -
> >>
> >> -**/
> >>
> >> -EFI_STATUS
> >>
> >> -EFIAPI
> >>
> >> -SmmAllocatePool (
> >>
> >> -  IN      EFI_MEMORY_TYPE           PoolType,
> >>
> >> -  IN      UINTN                     Size,
> >>
> >> -  OUT     VOID                      **Buffer
> >>
> >> -  );
> >>
> >> -
> >>
> >> -/**
> >>
> >> -  Frees pool.
> >>
> >> -
> >>
> >> -  @param  Buffer                 The allocated pool entry to free
> >>
> >> -
> >>
> >> -  @retval EFI_INVALID_PARAMETER  Buffer is not a valid value.
> >>
> >> -  @retval EFI_SUCCESS            Pool successfully freed.
> >>
> >> -
> >>
> >> -**/
> >>
> >> -EFI_STATUS
> >>
> >> -EFIAPI
> >>
> >> -SmmFreePool (
> >>
> >> -  IN      VOID                      *Buffer
> >>
> >> -  );
> >>
> >> -
> >>
> >> -#endif
> >>
> >
> >
> >
> >
> >
> 
> 
> 
> 
> 

[PATCH 1/5] MdeModulePkg/PeiCore: Align fixed-address error behaviour

[Marvin Häuser]

Update the control flow to take the same actions for failed
fixed-address loading as if the feature was disabled. This
primarily removes code duplication.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Debkumar De <debkumar.de@intel.com>
Cc: Harry Han <harry.han@intel.com>
Cc: Catharine West <catharine.west@intel.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
 MdeModulePkg/Core/Pei/Image/Image.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/MdeModulePkg/Core/Pei/Image/Image.c b/MdeModulePkg/Core/Pei/Image/Image.c
index 5af3895191a5..94adbed82e44 100644
--- a/MdeModulePkg/Core/Pei/Image/Image.c
+++ b/MdeModulePkg/Core/Pei/Image/Image.c
@@ -364,18 +364,18 @@ LoadAndRelocatePeCoffImage (
       AlignImageSize += ImageContext.SectionAlignment;
     }
 
+    Status = EFI_UNSUPPORTED;
+
     if (PcdGet64(PcdLoadModuleAtFixAddressEnable) != 0 && (Private->HobList.HandoffInformationTable->BootMode != BOOT_ON_S3_RESUME)) {
       Status = GetPeCoffImageFixLoadingAssignedAddress(&ImageContext, Private);
       if (EFI_ERROR (Status)){
         DEBUG ((EFI_D_INFO|EFI_D_LOAD, "LOADING MODULE FIXED ERROR: Failed to load module at fixed address. \n"));
-        //
-        // The PEIM is not assigned valid address, try to allocate page to load it.
-        //
-        Status = PeiServicesAllocatePages (EfiBootServicesCode,
-                                           EFI_SIZE_TO_PAGES ((UINT32) AlignImageSize),
-                                           &ImageContext.ImageAddress);
       }
-    } else {
+    }
+    if (EFI_ERROR (Status)){
+      //
+      // The PEIM is not assigned valid address, try to allocate page to load it.
+      //
       Status = PeiServicesAllocatePages (EfiBootServicesCode,
                                          EFI_SIZE_TO_PAGES ((UINT32) AlignImageSize),
                                          &ImageContext.ImageAddress);
-- 
2.31.1

[PATCH 2/5] MdeModulePkg/DxeCore: Align fixed-address error behaviour

[Marvin Häuser]

Update the control flow to take the same actions for failed
fixed-address loading as if the feature was disabled. This allows
Images to still be loaded to their preferred address in the case of
a mismatch between fixed-address and preferred address, and also
ensures correct handling of stripped relocations.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
 MdeModulePkg/Core/Dxe/Image/Image.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c b/MdeModulePkg/Core/Dxe/Image/Image.c
index 641a5715b112..0a1def0572bc 100644
--- a/MdeModulePkg/Core/Dxe/Image/Image.c
+++ b/MdeModulePkg/Core/Dxe/Image/Image.c
@@ -565,6 +565,7 @@ CoreLoadPeImage (
   EFI_STATUS                Status;
   BOOLEAN                   DstBufAlocated;
   UINTN                     Size;
+  PHYSICAL_ADDRESS          PreferredAddress;
 
   ZeroMem (&Image->ImageContext, sizeof (Image->ImageContext));
 
@@ -642,6 +643,8 @@ CoreLoadPeImage (
     // a specified address.
     //
     if (PcdGet64(PcdLoadModuleAtFixAddressEnable) != 0 ) {
+      PreferredAddress = Image->ImageContext.ImageAddress;
+
       Status = GetPeCoffImageFixLoadingAssignedAddress (&(Image->ImageContext));
 
       if (EFI_ERROR (Status))  {
@@ -649,15 +652,10 @@ CoreLoadPeImage (
           // If the code memory is not ready, invoke CoreAllocatePage with AllocateAnyPages to load the driver.
           //
           DEBUG ((EFI_D_INFO|EFI_D_LOAD, "LOADING MODULE FIXED ERROR: Loading module at fixed address failed since specified memory is not available.\n"));
-
-          Status = CoreAllocatePages (
-                     AllocateAnyPages,
-                     (EFI_MEMORY_TYPE) (Image->ImageContext.ImageCodeMemoryType),
-                     Image->NumberOfPages,
-                     &Image->ImageContext.ImageAddress
-                     );
+          Image->ImageContext.ImageAddress = PreferredAddress;
       }
-    } else {
+    }
+    if (EFI_ERROR (Status)) {
       if (Image->ImageContext.ImageAddress >= 0x100000 || Image->ImageContext.RelocationsStripped) {
         Status = CoreAllocatePages (
                    AllocateAddress,
-- 
2.31.1

[PATCH 3/5] MdeModulePkg/DxeCore: Check for fixed-address Image relocations

[Marvin Häuser]

Especially in the relative mode, fixed-address loading may not target
the preferred Image base address. In this case, Image relocations are
required to load the Image. Add the necessary check for this.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
 MdeModulePkg/Core/Dxe/Image/Image.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c b/MdeModulePkg/Core/Dxe/Image/Image.c
index 0a1def0572bc..a19be4c2a4a9 100644
--- a/MdeModulePkg/Core/Dxe/Image/Image.c
+++ b/MdeModulePkg/Core/Dxe/Image/Image.c
@@ -647,7 +647,13 @@ CoreLoadPeImage (
 
       Status = GetPeCoffImageFixLoadingAssignedAddress (&(Image->ImageContext));
 
-      if (EFI_ERROR (Status))  {
+      if (!EFI_ERROR (Status))  {
+          if (PreferredAddress != Image->ImageContext.ImageAddress && Image->ImageContext.RelocationsStripped) {
+            Status = EFI_UNSUPPORTED;
+            DEBUG ((EFI_D_INFO|EFI_D_LOAD, "LOADING MODULE FIXED ERROR: Loading module at fixed address failed since relocations have been stripped.\n"));
+            Image->ImageContext.ImageAddress = PreferredAddress;
+          }
+      } else {
           //
           // If the code memory is not ready, invoke CoreAllocatePage with AllocateAnyPages to load the driver.
           //
-- 
2.31.1

[PATCH 4/5] MdeModulePkg/PiSmmIpl: Disallow stripped Image relocations

[Marvin Häuser]

The SMM stack does not support loading Images to preferred addresses
in any way. Add checks that Image relocations have not been stripped.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
 MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
index 599a0cd01d80..d70b6e8ff46d 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
@@ -1049,6 +1049,15 @@ ExecuteSmmCoreFromSmram (
   if (EFI_ERROR (Status)) {
     return Status;
   }
+
+  //
+  // Stripped Image relocations are not supported for both fixed-address and
+  // dynamic loading.
+  //
+  if (ImageContext.RelocationsStripped) {
+    return EFI_UNSUPPORTED;
+  }
+
   //
   // if Loading module at Fixed Address feature is enabled, the SMM core driver will be loaded to
   // the address assigned by build tool.
-- 
2.31.1

[PATCH 5/5] MdeModulePkg/PiSmmCore: Disallow stripped Image relocations

[Marvin Häuser]

The SMM stack does not support loading Images to preferred addresses
in any way. Add checks that Image relocations have not been stripped.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
 MdeModulePkg/Core/PiSmmCore/Dispatcher.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/MdeModulePkg/Core/PiSmmCore/Dispatcher.c b/MdeModulePkg/Core/PiSmmCore/Dispatcher.c
index 76ee9e0b89cc..69ea61e13434 100644
--- a/MdeModulePkg/Core/PiSmmCore/Dispatcher.c
+++ b/MdeModulePkg/Core/PiSmmCore/Dispatcher.c
@@ -447,6 +447,18 @@ SmmLoadImage (
     }
     return Status;
   }
+
+  //
+  // Stripped Image relocations are not supported for both fixed-address and
+  // dynamic loading.
+  //
+  if (ImageContext.RelocationsStripped) {
+    if (Buffer != NULL) {
+      gBS->FreePool (Buffer);
+    }
+    return EFI_UNSUPPORTED;
+  }
+
   //
   // if Loading module at Fixed Address feature is enabled, then  cut out a memory range started from TESG BASE
   // to hold the Smm driver code
-- 
2.31.1

Re: [edk2-devel] [PATCH 1/5] MdeModulePkg/PeiCore: Align fixed-address error behaviour

[Marvin Häuser]

Good day,

Is someone firm with the details around git send-mail? I explicitly 
disabled threading (i.e. "git config sendemail.thread" yields "false"), 
and the original patch file does not contain any "In-Reply-To" header. 
Yet I can see it being added referring to the cover letter of a 
different patch in the git log, with no explanation at all. Any pointers 
so I don't keep sending brokenly-threaded sets by accident? Sorry!

Best regards,
Marvin

On 21/08/2021 21:55, Marvin Häuser wrote:
> Update the control flow to take the same actions for failed
> fixed-address loading as if the feature was disabled. This
> primarily removes code duplication.
>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Debkumar De <debkumar.de@intel.com>
> Cc: Harry Han <harry.han@intel.com>
> Cc: Catharine West <catharine.west@intel.com>
> Cc: Vitaly Cheptsov <vit9696@protonmail.com>
> Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
> ---
>   MdeModulePkg/Core/Pei/Image/Image.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/MdeModulePkg/Core/Pei/Image/Image.c b/MdeModulePkg/Core/Pei/Image/Image.c
> index 5af3895191a5..94adbed82e44 100644
> --- a/MdeModulePkg/Core/Pei/Image/Image.c
> +++ b/MdeModulePkg/Core/Pei/Image/Image.c
> @@ -364,18 +364,18 @@ LoadAndRelocatePeCoffImage (
>         AlignImageSize += ImageContext.SectionAlignment;
>
>       }
>
>   
>
> +    Status = EFI_UNSUPPORTED;
>
> +
>
>       if (PcdGet64(PcdLoadModuleAtFixAddressEnable) != 0 && (Private->HobList.HandoffInformationTable->BootMode != BOOT_ON_S3_RESUME)) {
>
>         Status = GetPeCoffImageFixLoadingAssignedAddress(&ImageContext, Private);
>
>         if (EFI_ERROR (Status)){
>
>           DEBUG ((EFI_D_INFO|EFI_D_LOAD, "LOADING MODULE FIXED ERROR: Failed to load module at fixed address. \n"));
>
> -        //
>
> -        // The PEIM is not assigned valid address, try to allocate page to load it.
>
> -        //
>
> -        Status = PeiServicesAllocatePages (EfiBootServicesCode,
>
> -                                           EFI_SIZE_TO_PAGES ((UINT32) AlignImageSize),
>
> -                                           &ImageContext.ImageAddress);
>
>         }
>
> -    } else {
>
> +    }
>
> +    if (EFI_ERROR (Status)){
>
> +      //
>
> +      // The PEIM is not assigned valid address, try to allocate page to load it.
>
> +      //
>
>         Status = PeiServicesAllocatePages (EfiBootServicesCode,
>
>                                            EFI_SIZE_TO_PAGES ((UINT32) AlignImageSize),
>
>                                            &ImageContext.ImageAddress);
>

Re: [edk2-devel] [PATCH 1/5] MdeModulePkg/PeiCore: Align fixed-address error behaviour

[Marvin Häuser]

Got it: 
https://public-inbox.org/git/4db7759c-2123-533b-9f89-954c07f5832a@posteo.de/T/#u

Sorry, once more, for the "spam" :)

Best regards,
Marvin

On 21/08/2021 22:10, Marvin Häuser wrote:
> Good day,
>
> Is someone firm with the details around git send-mail? I explicitly 
> disabled threading (i.e. "git config sendemail.thread" yields 
> "false"), and the original patch file does not contain any 
> "In-Reply-To" header. Yet I can see it being added referring to the 
> cover letter of a different patch in the git log, with no explanation 
> at all. Any pointers so I don't keep sending brokenly-threaded sets by 
> accident? Sorry!
>
> Best regards,
> Marvin
>
> On 21/08/2021 21:55, Marvin Häuser wrote:
>> Update the control flow to take the same actions for failed
>> fixed-address loading as if the feature was disabled. This
>> primarily removes code duplication.
>>
>> Cc: Jian J Wang <jian.j.wang@intel.com>
>> Cc: Hao A Wu <hao.a.wu@intel.com>
>> Cc: Dandan Bi <dandan.bi@intel.com>
>> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>> Cc: Debkumar De <debkumar.de@intel.com>
>> Cc: Harry Han <harry.han@intel.com>
>> Cc: Catharine West <catharine.west@intel.com>
>> Cc: Vitaly Cheptsov <vit9696@protonmail.com>
>> Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
>> ---
>>   MdeModulePkg/Core/Pei/Image/Image.c | 14 +++++++-------
>>   1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/MdeModulePkg/Core/Pei/Image/Image.c 
>> b/MdeModulePkg/Core/Pei/Image/Image.c
>> index 5af3895191a5..94adbed82e44 100644
>> --- a/MdeModulePkg/Core/Pei/Image/Image.c
>> +++ b/MdeModulePkg/Core/Pei/Image/Image.c
>> @@ -364,18 +364,18 @@ LoadAndRelocatePeCoffImage (
>>         AlignImageSize += ImageContext.SectionAlignment;
>>
>>       }
>>
>>
>> +    Status = EFI_UNSUPPORTED;
>>
>> +
>>
>>       if (PcdGet64(PcdLoadModuleAtFixAddressEnable) != 0 && 
>> (Private->HobList.HandoffInformationTable->BootMode != 
>> BOOT_ON_S3_RESUME)) {
>>
>>         Status = 
>> GetPeCoffImageFixLoadingAssignedAddress(&ImageContext, Private);
>>
>>         if (EFI_ERROR (Status)){
>>
>>           DEBUG ((EFI_D_INFO|EFI_D_LOAD, "LOADING MODULE FIXED ERROR: 
>> Failed to load module at fixed address. \n"));
>>
>> -        //
>>
>> -        // The PEIM is not assigned valid address, try to allocate 
>> page to load it.
>>
>> -        //
>>
>> -        Status = PeiServicesAllocatePages (EfiBootServicesCode,
>>
>> -                                           EFI_SIZE_TO_PAGES 
>> ((UINT32) AlignImageSize),
>>
>> - &ImageContext.ImageAddress);
>>
>>         }
>>
>> -    } else {
>>
>> +    }
>>
>> +    if (EFI_ERROR (Status)){
>>
>> +      //
>>
>> +      // The PEIM is not assigned valid address, try to allocate 
>> page to load it.
>>
>> +      //
>>
>>         Status = PeiServicesAllocatePages (EfiBootServicesCode,
>>
>>                                            EFI_SIZE_TO_PAGES 
>> ((UINT32) AlignImageSize),
>>
>> &ImageContext.ImageAddress);
>>
>

[PATCH 2/3] ArmPkg: Use Image base address for GDB symbols loading

[Marvin Häuser]

GDB symbols are currently loaded by specifying the .text section
address. It is assumed to be the value of the PE/COFF SizeOfHeaders
field. This may not be the case for various reasons, including a
sufficiently strict Image section alignment. Use the "-o" parameter
to specify the Image base address instead. This works because the GCC
linker scripts are designed to emit Image section addresses that are
equal to those of the final PE/COFF Image.

Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
 ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c | 6 +++---
 ArmPkg/Library/RvdPeCoffExtraActionLib/RvdPeCoffExtraActionLib.c     | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
index 3f88e84372ee..2ca42c19c03f 100644
--- a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
+++ b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
@@ -84,11 +84,11 @@ PeCoffLoaderRelocateImageExtraAction (
     DEBUG ((EFI_D_LOAD | EFI_D_INFO, "load /a /ni /np %a &0x%p\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders)));
 #else
     // Print out the command for the DS-5 to load symbols for this image
-    DEBUG ((EFI_D_LOAD | EFI_D_INFO, "add-symbol-file %a 0x%p\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders)));
+    DEBUG ((EFI_D_LOAD | EFI_D_INFO, "add-symbol-file %a -o 0x%p\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)ImageContext->ImageAddress));
 #endif
 #elif __GNUC__
     // This may not work correctly if you generate PE/COFF directly as then the Offset would not be required
-    DEBUG ((EFI_D_LOAD | EFI_D_INFO, "add-symbol-file %a 0x%p\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders)));
+    DEBUG ((EFI_D_LOAD | EFI_D_INFO, "add-symbol-file %a -o 0x%p\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)ImageContext->ImageAddress));
 #else
     DEBUG ((EFI_D_LOAD | EFI_D_INFO, "Loading driver at 0x%11p EntryPoint=0x%11p\n", (VOID *)(UINTN) ImageContext->ImageAddress, FUNCTION_ENTRY_POINT (ImageContext->EntryPoint)));
 #endif
@@ -125,7 +125,7 @@ PeCoffLoaderUnloadImageExtraAction (
     DEBUG ((DEBUG_LOAD | DEBUG_INFO, "unload symbols_only %a\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp))));
 #elif __GNUC__
     // This may not work correctly if you generate PE/COFF directly as then the Offset would not be required
-    DEBUG ((DEBUG_LOAD | DEBUG_INFO, "remove-symbol-file %a 0x%08x\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders)));
+    DEBUG ((DEBUG_LOAD | DEBUG_INFO, "remove-symbol-file %a 0x%08x\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)ImageContext->ImageAddress));
 #else
     DEBUG ((DEBUG_LOAD | DEBUG_INFO, "Unloading %a\n", ImageContext->PdbPointer));
 #endif
diff --git a/ArmPkg/Library/RvdPeCoffExtraActionLib/RvdPeCoffExtraActionLib.c b/ArmPkg/Library/RvdPeCoffExtraActionLib/RvdPeCoffExtraActionLib.c
index 80e531921c7a..0b78554f6347 100644
--- a/ArmPkg/Library/RvdPeCoffExtraActionLib/RvdPeCoffExtraActionLib.c
+++ b/ArmPkg/Library/RvdPeCoffExtraActionLib/RvdPeCoffExtraActionLib.c
@@ -116,7 +116,7 @@ PeCoffLoaderRelocateImageExtraAction (
 #if (__ARMCC_VERSION < 500000)
   AsciiSPrint (Buffer, sizeof(Buffer), "load /a /ni /np \"%a\" &0x%08x\n", ImageContext->PdbPointer, (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders));
 #else
-  AsciiSPrint (Buffer, sizeof(Buffer), "add-symbol-file %a 0x%08x\n", ImageContext->PdbPointer, (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders));
+  AsciiSPrint (Buffer, sizeof(Buffer), "add-symbol-file %a -o 0x%08x\n", ImageContext->PdbPointer, (UINTN)ImageContext->ImageAddress);
 #endif
   DeCygwinPathIfNeeded (&Buffer[16]);
 
-- 
2.31.1

[PATCH 3/3] ArmPkg: Use Image base address for RVCT symbols loading

[Marvin Häuser]

The initial import of the RVCT debug code used 0 for the address of
the first Image section [1]. This commit has been made in 2010, when
"--ro-base 0" was still passed as a build argument [2]. In 2015, this
was changed to mirror the PE/COFF memory layout the same way that ELF
does [3]. Update the corresponding debug code to no longer manually
account for the PE/COFF header offset.

[1] https://github.com/mhaeuser/edk2/commit/62e797a489341b13a095b3cd7c0f7bd9eac12efe
[2] https://github.com/mhaeuser/edk2/blob/62e797a489341b13a095b3cd7c0f7bd9eac12efe/BaseTools/Conf/tools_def.template#L2299-L2300
[3] https://github.com/tianocore/edk2/commit/b12ef6b964f8fc0532432ebffd67748648487133

Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
 ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c | 2 +-
 ArmPkg/Library/RvdPeCoffExtraActionLib/RvdPeCoffExtraActionLib.c     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
index 2ca42c19c03f..f65463be96b0 100644
--- a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
+++ b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
@@ -81,7 +81,7 @@ PeCoffLoaderRelocateImageExtraAction (
 #ifdef __CC_ARM
 #if (__ARMCC_VERSION < 500000)
     // Print out the command for the RVD debugger to load symbols for this image
-    DEBUG ((EFI_D_LOAD | EFI_D_INFO, "load /a /ni /np %a &0x%p\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders)));
+    DEBUG ((EFI_D_LOAD | EFI_D_INFO, "load /a /ni /np %a &0x%p\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)ImageContext->ImageAddress));
 #else
     // Print out the command for the DS-5 to load symbols for this image
     DEBUG ((EFI_D_LOAD | EFI_D_INFO, "add-symbol-file %a -o 0x%p\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)ImageContext->ImageAddress));
diff --git a/ArmPkg/Library/RvdPeCoffExtraActionLib/RvdPeCoffExtraActionLib.c b/ArmPkg/Library/RvdPeCoffExtraActionLib/RvdPeCoffExtraActionLib.c
index 0b78554f6347..0a5e8db294ea 100644
--- a/ArmPkg/Library/RvdPeCoffExtraActionLib/RvdPeCoffExtraActionLib.c
+++ b/ArmPkg/Library/RvdPeCoffExtraActionLib/RvdPeCoffExtraActionLib.c
@@ -114,7 +114,7 @@ PeCoffLoaderRelocateImageExtraAction (
   CHAR8 Buffer[256];
 
 #if (__ARMCC_VERSION < 500000)
-  AsciiSPrint (Buffer, sizeof(Buffer), "load /a /ni /np \"%a\" &0x%08x\n", ImageContext->PdbPointer, (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders));
+  AsciiSPrint (Buffer, sizeof(Buffer), "load /a /ni /np \"%a\" &0x%08x\n", ImageContext->PdbPointer, (UINTN)ImageContext->ImageAddress);
 #else
   AsciiSPrint (Buffer, sizeof(Buffer), "add-symbol-file %a -o 0x%08x\n", ImageContext->PdbPointer, (UINTN)ImageContext->ImageAddress);
 #endif
-- 
2.31.1