From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout01.posteo.de (mout01.posteo.de [185.67.36.65]) by mx.groups.io with SMTP id smtpd.web11.6763.1630588931196411937 for ; Thu, 02 Sep 2021 06:22:12 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@posteo.de header.s=2017 header.b=rAuerPAS; spf=pass (domain: posteo.de, ip: 185.67.36.65, mailfrom: mhaeuser@posteo.de) Received: from submission (posteo.de [89.146.220.130]) by mout01.posteo.de (Postfix) with ESMTPS id 3685724002B for ; Thu, 2 Sep 2021 15:22:07 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1630588928; bh=px8eC7LGmfbYgr4e1IlLrp2/fjmCBUK0T3E02K60y9Y=; h=Subject:To:Cc:From:Date:From; b=rAuerPASNWm5xS0h61L0d7EtzPUd1+dAuvqA/3uQVp4MSaFp2ra10+E8bsIqCDzIg +gQib48ZRpMcR3l6iDpWiPgaALu95FZQiScGtCruPLsNt9nT4voEdH/diJSIq3ixL2 lyPArQUOxCTOuR9cuTouS8hOJG6FNC/DeGUX0OkdXu1yWJQbyQewnqjtmOhW4Woa8j BXqhTaVM0ATD6t8P/jxMsnpYDW0bysw7149KQINJ2AqLmaqNZzxCmHGclldt+MfBHA vUy2LtdITLq3RUcgdJo6/s4BZn/74CYDEyutkTM0lVa2U1wWSv8SitnF1mUlSiFMDk yvfe0JRhpsvzw== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4H0hRL5Nqdz6tmD; Thu, 2 Sep 2021 15:22:06 +0200 (CEST) Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: Move PiSmmCoreMemoryAllocationLib into PiSmmCore To: devel@edk2.groups.io, ray.ni@intel.com Cc: "Wang, Jian J" , "Wu, Hao A" , "Dong, Eric" , Vitaly Cheptsov References: From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= Message-ID: Date: Thu, 2 Sep 2021 13:22:04 +0000 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: quoted-printable On 02/09/2021 12:53, Ni, Ray wrote: > Overall, the patch looks good to me. Thanks! > Can you remove the "CONSTRUCTOR =3D PiSmmCoreMemoryAll= ocationLibConstructor" from PiSmmCoreMemoryAllocationProfileLib.inf? And "LIBRARY_CLASS=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 =3D MemoryAllocationLib|SMM_C= ORE" too?=20 Otherwise this is a broken MemoryAllocationLib implementation. Removing=20 this will break any platform that uses this implementation, but I cannot=20 see any in the edk2 tree. Best regards, Marvin > With that change, Reviewed-by: Ray Ni > > More replies started with "[ray]". > > -----Original Message----- > From: Marvin H=C3=A4user > Sent: Wednesday, September 1, 2021 3:18 PM > To: Ni, Ray ; devel@edk2.groups.io > Cc: Wang, Jian J ; Wu, Hao A ;= Dong, Eric ; Vitaly Cheptsov > 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 sta= rt 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 MemoryA= llocationLib. We need to at least provide a dummy lib for it to pass the de= pendency check from base tools. > > [ray] I thought you could let PiSmmCore directly call the PiSmmCoreMemory= AllocationLibConstructor () in entrypoint to eliminate the needs of referri= ng the constructor in PiSmmCoreMemoryAllocationLib.inf. > But then I realized that constructors of other libraries may cal= l AllocatePages/Pool(). Calling PiSmmCoreMemoryAllocationLibConstructor() i= n 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=C3=A4user >> Sent: Sunday, August 22, 2021 3:56 AM >> To: devel@edk2.groups.io >> Cc: Wang, Jian J ; Wu, Hao A >> ; Dong, Eric ; Ni, Ray >> ; Vitaly Cheptsov >> 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 >> Cc: Hao A Wu >> Cc: Eric Dong >> Cc: Ray Ni >> Cc: Vitaly Cheptsov >> Signed-off-by: Marvin H=C3=A4user >> --- >> MdeModulePkg/{Library/PiSmmCoreMemoryAllocationLib/MemoryAllocationLi= b.c =3D> Core/PiSmmCore/MemoryAllocation.c} | 3 +- >> MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf = | 1 + >> MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllo= cationLib.inf | 5 +- >> MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllo= cationProfileLib.inf | 6 +- >> MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllo= cationServices.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 >> >> #include >> >> #include >> >> -#include "PiSmmCoreMemoryAllocationServices.h" >> >> +#include "PiSmmCore.h" >> >> +#include "PiSmmCorePrivateData.h" >> >> =20 >> >> #include >> >> =20 >> >> 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 >> >> =20 >> >> [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 =3D 1.0 >> >> PI_SPECIFICATION_VERSION =3D 0x0001000A >> >> LIBRARY_CLASS =3D MemoryAllocationLib|SMM_CORE >> >> + # >> >> + # This function is defined in PiSmmCore. >> >> + # >> >> CONSTRUCTOR =3D PiSmmCoreMemoryAllocationLibCons= tructor >> >> =20 >> >> # >> >> @@ -28,8 +31,6 @@ [Defines] >> # >> >> =20 >> >> [Sources] >> >> - MemoryAllocationLib.c >> >> - PiSmmCoreMemoryAllocationServices.h >> >> PiSmmCoreMemoryProfileLibNull.c >> >> =20 >> >> [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 =3D 1.0 >> >> PI_SPECIFICATION_VERSION =3D 0x0001000A >> >> LIBRARY_CLASS =3D MemoryAllocationLib|SMM_CORE >> >> + # >> >> + # This function is defined in PiSmmCore. >> >> + # >> >> CONSTRUCTOR =3D PiSmmCoreMemoryAllocationLibCons= tructor >> >> LIBRARY_CLASS =3D MemoryProfileLib|SMM_CORE >> >> CONSTRUCTOR =3D PiSmmCoreMemoryProfileLibConstru= ctor >> >> @@ -30,8 +33,6 @@ [Defines] >> # >> >> =20 >> >> [Sources] >> >> - MemoryAllocationLib.c >> >> - PiSmmCoreMemoryAllocationServices.h >> >> PiSmmCoreMemoryProfileLib.c >> >> PiSmmCoreMemoryProfileServices.h >> >> =20 >> >> @@ -43,6 +44,7 @@ [LibraryClasses] >> DebugLib >> >> BaseMemoryLib >> >> UefiBootServicesTableLib >> >> + MemoryAllocationLib >> >> =20 >> >> [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.
>> >> - 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 S= MM Core. >> >> - /// >> >> - BOOLEAN SmmEntryPointRegistered; >> >> - >> >> - /// >> >> - /// Boolean flag set to TRUE while an SMI is being processed by the S= MM 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 Communi= cation 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 Communicatio= n 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 allocat= ed pages >> >> - into >> >> - @param NumberOfPages The number of pages to allocate >> >> - @param Memory A pointer to receive the base allocate= d memory >> >> - address >> >> - >> >> - @retval EFI_INVALID_PARAMETER Parameters violate checking rules defi= ned in spec. >> >> - @retval EFI_NOT_FOUND Could not allocate pages match the req= uirement. >> >> - @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 t= he 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 allocati= on 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 >> > > >=20 > >