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...@intel.com
> 抄送: Wang, Jian J <jian.j.w...@intel.com>; Wu, Hao A
> <hao.a...@intel.com>; Dong, Eric <eric.d...@intel.com>; Vitaly Cheptsov
> <vit9...@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...@intel.com>
> >
> > More replies started with "[ray]".
> >
> > -----Original Message-----
> > From: Marvin Häuser <mhaeu...@posteo.de>
> > Sent: Wednesday, September 1, 2021 3:18 PM
> > To: Ni, Ray <ray...@intel.com>; devel@edk2.groups.io
> > Cc: Wang, Jian J <jian.j.w...@intel.com>; Wu, Hao A
> <hao.a...@intel.com>; Dong, Eric <eric.d...@intel.com>; Vitaly Cheptsov
> <vit9...@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 <mhaeu...@posteo.de>
> >> Sent: Sunday, August 22, 2021 3:56 AM
> >> To: devel@edk2.groups.io
> >> Cc: Wang, Jian J <jian.j.w...@intel.com>; Wu, Hao A
> >> <hao.a...@intel.com>; Dong, Eric <eric.d...@intel.com>; Ni, Ray
> >> <ray...@intel.com>; Vitaly Cheptsov <vit9...@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.w...@intel.com>
> >> Cc: Hao A Wu <hao.a...@intel.com>
> >> Cc: Eric Dong <eric.d...@intel.com>
> >> Cc: Ray Ni <ray...@intel.com>
> >> Cc: Vitaly Cheptsov <vit9...@protonmail.com>
> >> Signed-off-by: Marvin Häuser <mhaeu...@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
> >>
> >
> >
> >
> >
> >
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#80186): https://edk2.groups.io/g/devel/message/80186
Mute This Topic: https://groups.io/mt/85342914/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
