[edk2-devel] [PATCH 1/1] MdeModulePkg: Move PiSmmCoreMemoryAllocationLib into PiSmmCore
Marvin Häuser
mhaeuser at posteo.de
Thu Sep 2 13:22:04 UTC 2021
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 at intel.com>
>
> More replies started with "[ray]".
>
> -----Original Message-----
> From: Marvin Häuser <mhaeuser at posteo.de>
> Sent: Wednesday, September 1, 2021 3:18 PM
> To: Ni, Ray <ray.ni at intel.com>; devel at edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang at intel.com>; Wu, Hao A <hao.a.wu at intel.com>; Dong, Eric <eric.dong at intel.com>; Vitaly Cheptsov <vit9696 at 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 at posteo.de>
>> Sent: Sunday, August 22, 2021 3:56 AM
>> To: devel at edk2.groups.io
>> Cc: Wang, Jian J <jian.j.wang at intel.com>; Wu, Hao A
>> <hao.a.wu at intel.com>; Dong, Eric <eric.dong at intel.com>; Ni, Ray
>> <ray.ni at intel.com>; Vitaly Cheptsov <vit9696 at 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 at intel.com>
>> Cc: Hao A Wu <hao.a.wu at intel.com>
>> Cc: Eric Dong <eric.dong at intel.com>
>> Cc: Ray Ni <ray.ni at intel.com>
>> Cc: Vitaly Cheptsov <vit9696 at protonmail.com>
>> Signed-off-by: Marvin Häuser <mhaeuser at 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
>>
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#80158): https://edk2.groups.io/g/devel/message/80158
Mute This Topic: https://groups.io/mt/85048608/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list