On 02/16/21 21:11, Michael Kubacki wrote: > On 2/15/2021 12:33 PM, Laszlo Ersek wrote: >> On 02/13/21 01:58, mikub...@linux.microsoft.com wrote: >>> From: Michael Kubacki <michael.kuba...@microsoft.com> >>> >>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3218 >>> >>> Adds an INF for StandaloneMmCpuFeaturesLib, which supports building >>> the SmmCpuFeaturesLib code for Standalone MM. Minimal code changes >>> are made to allow reuse of existing code for Standalone MM. >>> >>> The original INF file names are left intact (continue to use SMM >>> terminology) to retain backward compatibility with platforms that >>> use those INFs. Similarly, the pre-existing C file names are >>> unchanged to be consistent with the INF file names. >>> >>> Cc: Eric Dong <eric.d...@intel.com> >>> Cc: Ray Ni <ray...@intel.com> >>> Cc: Laszlo Ersek <ler...@redhat.com> >>> Cc: Rahul Kumar <rahul1.ku...@intel.com> >>> Signed-off-by: Michael Kubacki <michael.kuba...@microsoft.com> >>> --- >>> UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c >>> | 2 +- >>> UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibNoStm.c >>> | 2 +- >>> UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c >>> | 2 +- >>> UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.c >>> | 51 ++++++++++++++++++++ >>> UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.inf >>> | 38 +++++++++++++++ >>> UefiCpuPkg/UefiCpuPkg.dsc >>> | 1 + >>> 6 files changed, 93 insertions(+), 3 deletions(-) >>> >>> diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c >>> b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c >>> index a494988898b8..990fdb098865 100644 >>> --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c >>> +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c >>> @@ -7,7 +7,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent >>> **/ >>> -#include <PiSmm.h> >>> +#include <PiMm.h> >>> #include <Library/SmmCpuFeaturesLib.h> >>> #include <Library/BaseLib.h> >>> #include <Library/MtrrLib.h> >>> diff --git >>> a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibNoStm.c >>> b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibNoStm.c >>> index eebbbfd00a83..5946081afbb7 100644 >>> --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibNoStm.c >>> +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibNoStm.c >>> @@ -8,7 +8,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent >>> **/ >>> -#include <PiSmm.h> >>> +#include <PiMm.h> >>> #include <Library/SmmCpuFeaturesLib.h> >>> #include "CpuFeaturesLib.h" >>> diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c >>> b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c >>> index 4b6bf958cedf..cda1ab8e78de 100644 >>> --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c >>> +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c >>> @@ -6,7 +6,7 @@ >>> **/ >>> -#include <PiSmm.h> >>> +#include <PiMm.h> >>> #include <Library/BaseLib.h> >>> #include <Library/BaseMemoryLib.h> >>> #include <Library/MemoryAllocationLib.h> >> >> (1) Do you really need to update this header file? It's never built for >> the standalone MM lib instance. (If you do it for consistency, I guess >> that's OK, but then it should be mentioned in the commit message.) >> > I did update it for consistency. I will note this in the commit message > in v3. >> >>> diff --git >>> a/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.c >>> b/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.c >>> new file mode 100644 >>> index 000000000000..114b177e5e57 >>> --- /dev/null >>> +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.c >>> @@ -0,0 +1,51 @@ >>> +/** @file >>> +Standalone MM CPU specific programming. >>> + >>> +Copyright (c) 2010 - 2019, Intel Corporation. All rights reserved.<BR> >>> +Copyright (c) Microsoft Corporation.<BR> >>> +SPDX-License-Identifier: BSD-2-Clause-Patent >>> + >>> +**/ >>> + >>> +#include <PiMm.h> >>> +#include <Library/PcdLib.h> >>> +#include "CpuFeaturesLib.h" >>> + >>> +/** >>> + Gets the maximum number of logical processors from the PCD >>> PcdCpuMaxLogicalProcessorNumber. >>> + >>> + This access is abstracted from the PCD services to enforce that >>> the PCD be >>> + FixedAtBuild in the Standalone MM build of this driver. >>> + >>> + @retval The value of PcdCpuMaxLogicalProcessorNumber. >> >> (2) Sorry, I'm just noticing -- given that we don't provide a constant >> return value here, we should use "@return", not "@retval". >> >> (Applies to all occurrences of the GetCpuMaxLogicalProcessorNumber() >> documentation.) >> >> > I will update to "@return" in v3. >>> + >>> + >>> +**/ >>> +UINT32 >>> +GetCpuMaxLogicalProcessorNumber ( >>> + VOID >>> + ) >>> +{ >>> + return FixedPcdGet32 (PcdCpuMaxLogicalProcessorNumber); >>> +} >>> + >>> +/** >>> + The Standalone MM library constructor. >>> + >>> + @param[in] ImageHandle Image handle of this driver. >>> + @param[in] SystemTable A Pointer to the EFI System Table. >>> + >>> + @retval EFI_SUCCESS This constructor always returns success. >>> + >>> +**/ >>> +EFI_STATUS >>> +EFIAPI >>> +StandaloneMmCpuFeaturesLibConstructor ( >>> + IN EFI_HANDLE ImageHandle, >>> + IN EFI_MM_SYSTEM_TABLE *SystemTable >>> + ) >>> +{ >>> + CpuFeaturesLibInitialization (); >>> + >>> + return EFI_SUCCESS; >>> +} >>> diff --git >>> a/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.inf >>> b/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.inf >>> new file mode 100644 >>> index 000000000000..64f0a0853337 >>> --- /dev/null >>> +++ >>> b/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.inf >>> @@ -0,0 +1,38 @@ >>> +## @file >>> +# Standalone MM CPU specific programming. >>> +# >>> +# Copyright (c) 2009 - 2016, Intel Corporation. All rights >>> reserved.<BR> >>> +# Copyright (c) Microsoft Corporation.<BR> >>> +# SPDX-License-Identifier: BSD-2-Clause-Patent >>> +# >>> +## >>> + >>> +[Defines] >>> + INF_VERSION = 0x00010005 >>> + BASE_NAME = StandaloneMmCpuFeaturesLib >>> + MODULE_UNI_FILE = SmmCpuFeaturesLib.uni >>> + FILE_GUID = BB554A2D-F5DF-41D3-8C62-46476A2B2B18 >>> + MODULE_TYPE = MM_STANDALONE >>> + VERSION_STRING = 1.0 >>> + PI_SPECIFICATION_VERSION = 0x00010032 >>> + LIBRARY_CLASS = SmmCpuFeaturesLib >>> + CONSTRUCTOR = >>> StandaloneMmCpuFeaturesLibConstructor >>> + >>> +[Sources] >>> + CpuFeaturesLib.h >>> + StandaloneMmCpuFeaturesLib.c >>> + SmmCpuFeaturesLib.c >>> + SmmCpuFeaturesLibNoStm.c >> >> (3) Darn. I'm displeased with my previous feedback. Unfortunately, I >> couldn't foresee the end result of my v1 comments, at such a distance. >> >> With v2 applied, we build the SmmCpuFeaturesLibConstructor() function >> from "SmmCpuFeaturesLibNoStm.c" for the StandaloneMmCpuFeaturesLib >> instance as well -- we just never call it. It's not really clean. >> >> The "feature map" (?) is something like this: >> >> traditional, traditional, standalone, >> no STM STM no STM >> >> entry point type DXE DXE MM >> >> lib inst. init. basic STM basic >> >> processor init. basic STM basic >> >> PCD access any any fixed >> >> The first two properties, i.e. "entry point type" and "library instance >> initialization", dictate the constructor implementation *together*. And >> that suggests we need three separate files (DXE-basic, DXE-STM, >> MM-basic). In other words, the SmmCpuFeaturesLibConstructor() function >> should be re-added to yet another new file, in patch#2. >> >> On the other hand, creating yet another C file with just the DXE-basic >> constructor seems awkward. :( (This C file would be listed in >> "SmmCpuFeaturesLib.inf", but not "StandaloneMmCpuFeaturesLib.inf".) >> >> Any suggestions / preferences? >> >> (I apologize again for missing this in the v1 review.) >> > I'm leaning toward adding it in a new file in patch #2. > > If that's done, is there a preference for the file name? > > After reviewing the current set of files, the usage of > "SmmCpuFeaturesLib.c" in slightly inconsistent with the other .c files > that handle their instance specific logic. > > Given that SmmCpuFeaturesLib.inf will now have an instance-specific > file, would it be more intuitive to have that file be named > "SmmCpuFeaturesLib.c" and rename the current file shared across all > instances to "SmmCpuFeaturesLibCommon.c"? > > If the rename occurred, the final file set would look like as below > across the instances. > > SmmCpuFeaturesLib.inf: > [Sources] > CpuFeaturesLib.h > SmmCpuFeaturesLib.c --> Will contain SmmCpuFeaturesLibConstructor() > SmmCpuFeaturesLibCommon.c > SmmCpuFeaturesLibNoStm.c > TraditionalMmCpuFeaturesLib.c > > SmmCpuFeaturesLibStm.inf: > [Sources] > CpuFeaturesLib.h > SmmCpuFeaturesLibCommon.c > SmmStm.c > SmmStm.h > TraditionalMmCpuFeaturesLib.c > > StandaloneMmCpuFeaturesLib.inf: > [Sources] > CpuFeaturesLib.h > StandaloneMmCpuFeaturesLib.c > SmmCpuFeaturesLibCommon.c > SmmCpuFeaturesLibNoStm.c
Looks good! Thanks! Laszlo >>> + >>> +[Packages] >>> + MdePkg/MdePkg.dec >>> + UefiCpuPkg/UefiCpuPkg.dec >>> + >>> +[LibraryClasses] >>> + BaseLib >>> + DebugLib >>> + MemoryAllocationLib >>> + PcdLib >>> + >>> +[FixedPcd] >>> + gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber >>> ## SOMETIMES_CONSUMES >>> diff --git a/UefiCpuPkg/UefiCpuPkg.dsc b/UefiCpuPkg/UefiCpuPkg.dsc >>> index 9128cef076dd..7db419471deb 100644 >>> --- a/UefiCpuPkg/UefiCpuPkg.dsc >>> +++ b/UefiCpuPkg/UefiCpuPkg.dsc >>> @@ -154,6 +154,7 @@ [Components.IA32, Components.X64] >>> >>> UefiCpuPkg/Library/SmmCpuPlatformHookLibNull/SmmCpuPlatformHookLibNull.inf >>> >>> UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf >>> UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf >>> + UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.inf >>> UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.inf >>> UefiCpuPkg/PiSmmCommunication/PiSmmCommunicationPei.inf >>> UefiCpuPkg/PiSmmCommunication/PiSmmCommunicationSmm.inf >>> > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#71745): https://edk2.groups.io/g/devel/message/71745 Mute This Topic: https://groups.io/mt/80599748/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-