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

["Ni, Ray" <ray.ni@intel.com>]

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
>