On 2/15/2021 12:33 PM, Laszlo Ersek wrote:
On 02/13/21 01:58, [email protected] wrote:
From: Michael Kubacki <[email protected]>
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 <[email protected]>
Cc: Ray Ni <[email protected]>
Cc: Laszlo Ersek <[email protected]>
Cc: Rahul Kumar <[email protected]>
Signed-off-by: Michael Kubacki <[email protected]>
---
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
Thanks,
Michael
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 (#71699): https://edk2.groups.io/g/devel/message/71699
Mute This Topic: https://groups.io/mt/80599748/21656
Group Owner: [email protected]
Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]]
-=-=-=-=-=-=-=-=-=-=-=-
