* [PATCH v2 0/4] UefiCpuPkg/StandaloneMmCpuFeaturesLib: Add Standalone MM support
@ 2021-02-13 0:58 Michael Kubacki
2021-02-13 0:58 ` [PATCH v2 1/4] UefiCpuPkg/SmmCpuFeaturesLib: Move multi-instance function decl to header Michael Kubacki
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Michael Kubacki @ 2021-02-13 0:58 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Laszlo Ersek, Rahul Kumar
From: Michael Kubacki <michael.kubacki@microsoft.com>
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3218
This patch series is v2 of the following v1 patch:
https://edk2.groups.io/g/devel/message/71626
Due to some pre-existing design issues in the library that affected
that patch, it was suggested to first address those issues and then
add the new INF StandaloneMmCpuFeaturesLib.inf. The first two
patches in this series primarily address those concerns.
The present SmmCpuFeaturesLib implementation in UefiCpuPkg can be
useful for IA32/X64 platforms that need a library instance for
a Standalone MM environment. Much of the logic can be reused and a
new INF can isolate the differences unique to Standalone MM.
PATCH v2 1/4 and PATCH v2 2/4 focus on fixing pre-existing design
issues.
PATCH v2 3/4 and PATCH v2 4/4 focus on the changes needed to add
Standalone MM support.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
Michael Kubacki (4):
UefiCpuPkg/SmmCpuFeaturesLib: Move multi-instance function decl to
header
UefiCpuPkg/SmmCpuFeaturesLib: Cleanup library constructors
UefiCpuPkg/SmmCpuFeaturesLib: Abstract PcdCpuMaxLogicalProcessorNumber
UefiCpuPkg/SmmCpuFeaturesLib: Add Standalone MM support
UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 34 ++++---------
UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibNoStm.c | 25 +++++++++-
UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c | 24 ++-------
UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.c | 51 ++++++++++++++++++++
UefiCpuPkg/Library/SmmCpuFeaturesLib/TraditionalMmCpuFeaturesLib.c | 28 +++++++++++
UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h | 48 ++++++++++++++++++
UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf | 2 +
UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf | 2 +
UefiCpuPkg/Library/SmmCpuFeaturesLib/{SmmCpuFeaturesLib.inf => StandaloneMmCpuFeaturesLib.inf} | 20 +++++---
UefiCpuPkg/UefiCpuPkg.dsc | 1 +
10 files changed, 182 insertions(+), 53 deletions(-)
create mode 100644 UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.c
create mode 100644 UefiCpuPkg/Library/SmmCpuFeaturesLib/TraditionalMmCpuFeaturesLib.c
create mode 100644 UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h
copy UefiCpuPkg/Library/SmmCpuFeaturesLib/{SmmCpuFeaturesLib.inf => StandaloneMmCpuFeaturesLib.inf} (56%)
--
2.28.0.windows.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/4] UefiCpuPkg/SmmCpuFeaturesLib: Move multi-instance function decl to header
2021-02-13 0:58 [PATCH v2 0/4] UefiCpuPkg/StandaloneMmCpuFeaturesLib: Add Standalone MM support Michael Kubacki
@ 2021-02-13 0:58 ` Michael Kubacki
2021-02-15 19:23 ` Laszlo Ersek
2021-02-13 0:58 ` [PATCH v2 2/4] UefiCpuPkg/SmmCpuFeaturesLib: Cleanup library constructors Michael Kubacki
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Michael Kubacki @ 2021-02-13 0:58 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Laszlo Ersek, Rahul Kumar
From: Michael Kubacki <michael.kubacki@microsoft.com>
FinishSmmCpuFeaturesInitializeProcessor() is a multi-instance
internal library function that is currently not declared in a
header file but embedded in "SmmCpuFeaturesLib.c".
This change cleans up the declaration moving it to a new header
file "CpuFeaturesLib.h" and removing the local declaration in
"SmmCpuFeaturesLib.c".
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---
UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 11 +---------
UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibNoStm.c | 1 +
UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c | 1 +
UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h | 22 ++++++++++++++++++++
UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf | 1 +
UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf | 1 +
6 files changed, 27 insertions(+), 10 deletions(-)
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
index 8fed18cf0e17..75bde752785a 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
@@ -15,6 +15,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Library/DebugLib.h>
#include <Register/Intel/Cpuid.h>
#include <Register/Intel/SmramSaveStateMap.h>
+#include "CpuFeaturesLib.h"
//
// Machine Specific Registers (MSRs)
@@ -35,16 +36,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#define SMM_FEATURES_LIB_IA32_MCA_CAP 0x17D
#define SMM_CODE_ACCESS_CHK_BIT BIT58
-/**
- Internal worker function that is called to complete CPU initialization at the
- end of SmmCpuFeaturesInitializeProcessor().
-
-**/
-VOID
-FinishSmmCpuFeaturesInitializeProcessor (
- VOID
- );
-
//
// Set default value to assume SMRR is not supported
//
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibNoStm.c b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibNoStm.c
index 3e63c5e27f98..c562582ccee0 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibNoStm.c
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibNoStm.c
@@ -9,6 +9,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <PiSmm.h>
#include <Library/SmmCpuFeaturesLib.h>
+#include "CpuFeaturesLib.h"
/**
Internal worker function that is called to complete CPU initialization at the
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c
index f7f8afacffb5..b5aad41fdb64 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c
@@ -21,6 +21,7 @@
#include <Protocol/MpService.h>
+#include "CpuFeaturesLib.h"
#include "SmmStm.h"
#define TXT_EVTYPE_BASE 0x400
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h b/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h
new file mode 100644
index 000000000000..4645bbb066c9
--- /dev/null
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h
@@ -0,0 +1,22 @@
+/** @file
+ Internal library function definitions.
+
+ Copyright (c) Microsoft Corporation.
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef _CPU_FEATURES_LIB_H_
+#define _CPU_FEATURES_LIB_H_
+
+/**
+ Internal worker function that is called to complete CPU initialization at the
+ end of SmmCpuFeaturesInitializeProcessor().
+
+**/
+VOID
+FinishSmmCpuFeaturesInitializeProcessor (
+ VOID
+ );
+
+#endif
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
index dd828baf69cb..a6d8467d26aa 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
@@ -17,6 +17,7 @@ [Defines]
CONSTRUCTOR = SmmCpuFeaturesLibConstructor
[Sources]
+ CpuFeaturesLib.h
SmmCpuFeaturesLib.c
SmmCpuFeaturesLibNoStm.c
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
index 50b9cc871302..89cd252ef44e 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
@@ -18,6 +18,7 @@ [Defines]
CONSTRUCTOR = SmmCpuFeaturesLibStmConstructor
[Sources]
+ CpuFeaturesLib.h
SmmCpuFeaturesLib.c
SmmStm.c
SmmStm.h
--
2.28.0.windows.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 2/4] UefiCpuPkg/SmmCpuFeaturesLib: Cleanup library constructors
2021-02-13 0:58 [PATCH v2 0/4] UefiCpuPkg/StandaloneMmCpuFeaturesLib: Add Standalone MM support Michael Kubacki
2021-02-13 0:58 ` [PATCH v2 1/4] UefiCpuPkg/SmmCpuFeaturesLib: Move multi-instance function decl to header Michael Kubacki
@ 2021-02-13 0:58 ` Michael Kubacki
2021-02-15 19:31 ` Laszlo Ersek
2021-02-13 0:58 ` [PATCH v2 3/4] UefiCpuPkg/SmmCpuFeaturesLib: Abstract PcdCpuMaxLogicalProcessorNumber Michael Kubacki
2021-02-13 0:58 ` [PATCH v2 4/4] UefiCpuPkg/SmmCpuFeaturesLib: Add Standalone MM support Michael Kubacki
3 siblings, 1 reply; 11+ messages in thread
From: Michael Kubacki @ 2021-02-13 0:58 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Laszlo Ersek, Rahul Kumar
From: Michael Kubacki <michael.kubacki@microsoft.com>
There's currently two library instances:
1. SmmCpuFeaturesLib
2. SmmCpuFeaturesLibStm
There's two constructor functions:
1. SmmCpuFeaturesLibConstructor()
2. SmmCpuFeaturesLibStmConstructor()
SmmCpuFeaturesLibConstructor() is called by
SmmCpuFeaturesLibStmConstructor() since the functionality in that
function is required by both library instances.
The declaration for SmmCpuFeaturesLibConstructor() is embedded in
"SmmStm.c" instead of being declared in a header file. Further,
that constructor function is called by the STM specific constructor.
This change moves the common code to a function called
CpuFeaturesLibInitialization() which is declared in an internal
library header file "CpuFeaturesLib.h". Each constructor simply
calls this function to perform the common functionality.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---
UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 19 +++++++----------
UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibNoStm.c | 22 ++++++++++++++++++++
UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c | 21 ++-----------------
UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h | 12 +++++++++++
4 files changed, 43 insertions(+), 31 deletions(-)
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
index 75bde752785a..e74f87f3f266 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
@@ -2,6 +2,7 @@
The CPU specific programming for PiSmmCpuDxeSmm module.
Copyright (c) 2010 - 2019, Intel Corporation. All rights reserved.<BR>
+Copyright (c) Microsoft Corporation.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -63,19 +64,15 @@ BOOLEAN mNeedConfigureMtrrs = TRUE;
BOOLEAN *mSmrrEnabled;
/**
- The constructor function
+ Performs library initialization.
- @param[in] ImageHandle The firmware allocated handle for the EFI image.
- @param[in] SystemTable A pointer to the EFI System Table.
-
- @retval EFI_SUCCESS The constructor always returns EFI_SUCCESS.
+ This initialization function contains common functionality shared betwen all
+ library instance constructors.
**/
-EFI_STATUS
-EFIAPI
-SmmCpuFeaturesLibConstructor (
- IN EFI_HANDLE ImageHandle,
- IN EFI_SYSTEM_TABLE *SystemTable
+VOID
+CpuFeaturesLibInitialization (
+ VOID
)
{
UINT32 RegEax;
@@ -162,8 +159,6 @@ SmmCpuFeaturesLibConstructor (
//
mSmrrEnabled = (BOOLEAN *)AllocatePool (sizeof (BOOLEAN) * PcdGet32 (PcdCpuMaxLogicalProcessorNumber));
ASSERT (mSmrrEnabled != NULL);
-
- return EFI_SUCCESS;
}
/**
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibNoStm.c b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibNoStm.c
index c562582ccee0..eebbbfd00a83 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibNoStm.c
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibNoStm.c
@@ -3,6 +3,7 @@ The CPU specific programming for PiSmmCpuDxeSmm module when STM support
is not included.
Copyright (c) 2010 - 2016, Intel Corporation. All rights reserved.<BR>
+Copyright (c) Microsoft Corporation.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -82,3 +83,24 @@ SmmCpuFeaturesInstallSmiHandler (
)
{
}
+
+/**
+ The constructor function for the library instance without STM.
+
+ @param[in] ImageHandle The firmware allocated handle for the EFI image.
+ @param[in] SystemTable A pointer to the EFI System Table.
+
+ @retval EFI_SUCCESS The constructor always returns EFI_SUCCESS.
+
+**/
+EFI_STATUS
+EFIAPI
+SmmCpuFeaturesLibConstructor (
+ IN EFI_HANDLE ImageHandle,
+ IN EFI_SYSTEM_TABLE *SystemTable
+ )
+{
+ CpuFeaturesLibInitialization ();
+
+ return EFI_SUCCESS;
+}
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c
index b5aad41fdb64..4b6bf958cedf 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c
@@ -30,22 +30,6 @@
#define RDWR_ACCS 3
#define FULL_ACCS 7
-/**
- The constructor function
-
- @param[in] ImageHandle The firmware allocated handle for the EFI image.
- @param[in] SystemTable A pointer to the EFI System Table.
-
- @retval EFI_SUCCESS The constructor always returns EFI_SUCCESS.
-
-**/
-EFI_STATUS
-EFIAPI
-SmmCpuFeaturesLibConstructor (
- IN EFI_HANDLE ImageHandle,
- IN EFI_SYSTEM_TABLE *SystemTable
- );
-
EFI_HANDLE mStmSmmCpuHandle = NULL;
BOOLEAN mLockLoadMonitor = FALSE;
@@ -138,10 +122,9 @@ SmmCpuFeaturesLibStmConstructor (
SmmCpuFeaturesLibStmSmiEntryFixupAddress ();
//
- // Call the common constructor function
+ // Perform library initialization common across all instances
//
- Status = SmmCpuFeaturesLibConstructor (ImageHandle, SystemTable);
- ASSERT_EFI_ERROR (Status);
+ CpuFeaturesLibInitialization ();
//
// Lookup the MP Services Protocol
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h b/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h
index 4645bbb066c9..f9a758e82558 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h
@@ -9,6 +9,18 @@
#ifndef _CPU_FEATURES_LIB_H_
#define _CPU_FEATURES_LIB_H_
+/**
+ Performs library initialization.
+
+ This initialization function contains common functionality shared betwen all
+ library instance constructors.
+
+**/
+VOID
+CpuFeaturesLibInitialization (
+ VOID
+ );
+
/**
Internal worker function that is called to complete CPU initialization at the
end of SmmCpuFeaturesInitializeProcessor().
--
2.28.0.windows.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 3/4] UefiCpuPkg/SmmCpuFeaturesLib: Abstract PcdCpuMaxLogicalProcessorNumber
2021-02-13 0:58 [PATCH v2 0/4] UefiCpuPkg/StandaloneMmCpuFeaturesLib: Add Standalone MM support Michael Kubacki
2021-02-13 0:58 ` [PATCH v2 1/4] UefiCpuPkg/SmmCpuFeaturesLib: Move multi-instance function decl to header Michael Kubacki
2021-02-13 0:58 ` [PATCH v2 2/4] UefiCpuPkg/SmmCpuFeaturesLib: Cleanup library constructors Michael Kubacki
@ 2021-02-13 0:58 ` Michael Kubacki
2021-02-15 19:35 ` Laszlo Ersek
2021-02-13 0:58 ` [PATCH v2 4/4] UefiCpuPkg/SmmCpuFeaturesLib: Add Standalone MM support Michael Kubacki
3 siblings, 1 reply; 11+ messages in thread
From: Michael Kubacki @ 2021-02-13 0:58 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Laszlo Ersek, Rahul Kumar
From: Michael Kubacki <michael.kubacki@microsoft.com>
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3218
Adds a new function called GetCpuMaxLogicalProcessorNumber() to
return the number of maximum CPU logical processors (currently
gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber).
This allows the the mechanism used to retrieve the CPU maximum
logical processor number to be abstracted from the logic that
needs the value.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---
UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 2 +-
UefiCpuPkg/Library/SmmCpuFeaturesLib/TraditionalMmCpuFeaturesLib.c | 28 ++++++++++++++++++++
UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h | 14 ++++++++++
UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf | 1 +
UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf | 1 +
5 files changed, 45 insertions(+), 1 deletion(-)
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
index e74f87f3f266..a494988898b8 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
@@ -157,7 +157,7 @@ CpuFeaturesLibInitialization (
//
// Allocate array for state of SMRR enable on all CPUs
//
- mSmrrEnabled = (BOOLEAN *)AllocatePool (sizeof (BOOLEAN) * PcdGet32 (PcdCpuMaxLogicalProcessorNumber));
+ mSmrrEnabled = (BOOLEAN *)AllocatePool (sizeof (BOOLEAN) * GetCpuMaxLogicalProcessorNumber ());
ASSERT (mSmrrEnabled != NULL);
}
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/TraditionalMmCpuFeaturesLib.c b/UefiCpuPkg/Library/SmmCpuFeaturesLib/TraditionalMmCpuFeaturesLib.c
new file mode 100644
index 000000000000..6a4eff755d92
--- /dev/null
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/TraditionalMmCpuFeaturesLib.c
@@ -0,0 +1,28 @@
+/** @file
+ Traditional MM CPU specific programming.
+
+ Copyright (c) Microsoft Corporation.<BR>
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <Base.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.
+
+**/
+UINT32
+GetCpuMaxLogicalProcessorNumber (
+ VOID
+ )
+{
+ return PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
+}
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h b/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h
index f9a758e82558..1a72ba4e2e13 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h
@@ -31,4 +31,18 @@ FinishSmmCpuFeaturesInitializeProcessor (
VOID
);
+/**
+ 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.
+
+**/
+UINT32
+GetCpuMaxLogicalProcessorNumber (
+ VOID
+ );
+
#endif
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
index a6d8467d26aa..b32dbeab9383 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
@@ -20,6 +20,7 @@ [Sources]
CpuFeaturesLib.h
SmmCpuFeaturesLib.c
SmmCpuFeaturesLibNoStm.c
+ TraditionalMmCpuFeaturesLib.c
[Packages]
MdePkg/MdePkg.dec
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
index 89cd252ef44e..f72ef0a88088 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
@@ -22,6 +22,7 @@ [Sources]
SmmCpuFeaturesLib.c
SmmStm.c
SmmStm.h
+ TraditionalMmCpuFeaturesLib.c
[Sources.Ia32]
Ia32/SmmStmSupport.c
--
2.28.0.windows.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 4/4] UefiCpuPkg/SmmCpuFeaturesLib: Add Standalone MM support
2021-02-13 0:58 [PATCH v2 0/4] UefiCpuPkg/StandaloneMmCpuFeaturesLib: Add Standalone MM support Michael Kubacki
` (2 preceding siblings ...)
2021-02-13 0:58 ` [PATCH v2 3/4] UefiCpuPkg/SmmCpuFeaturesLib: Abstract PcdCpuMaxLogicalProcessorNumber Michael Kubacki
@ 2021-02-13 0:58 ` Michael Kubacki
2021-02-15 20:33 ` Laszlo Ersek
3 siblings, 1 reply; 11+ messages in thread
From: Michael Kubacki @ 2021-02-13 0:58 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Laszlo Ersek, Rahul Kumar
From: Michael Kubacki <michael.kubacki@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.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Michael Kubacki <michael.kubacki@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>
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.
+
+
+**/
+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
+
+[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
--
2.28.0.windows.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/4] UefiCpuPkg/SmmCpuFeaturesLib: Move multi-instance function decl to header
2021-02-13 0:58 ` [PATCH v2 1/4] UefiCpuPkg/SmmCpuFeaturesLib: Move multi-instance function decl to header Michael Kubacki
@ 2021-02-15 19:23 ` Laszlo Ersek
0 siblings, 0 replies; 11+ messages in thread
From: Laszlo Ersek @ 2021-02-15 19:23 UTC (permalink / raw)
To: mikuback, devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar
On 02/13/21 01:58, mikuback@linux.microsoft.com wrote:
> From: Michael Kubacki <michael.kubacki@microsoft.com>
>
> FinishSmmCpuFeaturesInitializeProcessor() is a multi-instance
> internal library function that is currently not declared in a
> header file but embedded in "SmmCpuFeaturesLib.c".
>
> This change cleans up the declaration moving it to a new header
> file "CpuFeaturesLib.h" and removing the local declaration in
> "SmmCpuFeaturesLib.c".
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> ---
> UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 11 +---------
> UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibNoStm.c | 1 +
> UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c | 1 +
> UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h | 22 ++++++++++++++++++++
> UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf | 1 +
> UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf | 1 +
> 6 files changed, 27 insertions(+), 10 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> index 8fed18cf0e17..75bde752785a 100644
> --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> @@ -15,6 +15,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> #include <Library/DebugLib.h>
> #include <Register/Intel/Cpuid.h>
> #include <Register/Intel/SmramSaveStateMap.h>
> +#include "CpuFeaturesLib.h"
>
> //
> // Machine Specific Registers (MSRs)
> @@ -35,16 +36,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> #define SMM_FEATURES_LIB_IA32_MCA_CAP 0x17D
> #define SMM_CODE_ACCESS_CHK_BIT BIT58
>
> -/**
> - Internal worker function that is called to complete CPU initialization at the
> - end of SmmCpuFeaturesInitializeProcessor().
> -
> -**/
> -VOID
> -FinishSmmCpuFeaturesInitializeProcessor (
> - VOID
> - );
> -
> //
> // Set default value to assume SMRR is not supported
> //
> diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibNoStm.c b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibNoStm.c
> index 3e63c5e27f98..c562582ccee0 100644
> --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibNoStm.c
> +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibNoStm.c
> @@ -9,6 +9,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>
> #include <PiSmm.h>
> #include <Library/SmmCpuFeaturesLib.h>
> +#include "CpuFeaturesLib.h"
>
> /**
> Internal worker function that is called to complete CPU initialization at the
> diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c
> index f7f8afacffb5..b5aad41fdb64 100644
> --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c
> +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c
> @@ -21,6 +21,7 @@
>
> #include <Protocol/MpService.h>
>
> +#include "CpuFeaturesLib.h"
> #include "SmmStm.h"
>
> #define TXT_EVTYPE_BASE 0x400
> diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h b/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h
> new file mode 100644
> index 000000000000..4645bbb066c9
> --- /dev/null
> +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h
> @@ -0,0 +1,22 @@
> +/** @file
> + Internal library function definitions.
> +
> + Copyright (c) Microsoft Corporation.
> + SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#ifndef _CPU_FEATURES_LIB_H_
> +#define _CPU_FEATURES_LIB_H_
> +
> +/**
> + Internal worker function that is called to complete CPU initialization at the
> + end of SmmCpuFeaturesInitializeProcessor().
> +
> +**/
> +VOID
> +FinishSmmCpuFeaturesInitializeProcessor (
> + VOID
> + );
> +
> +#endif
> diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
> index dd828baf69cb..a6d8467d26aa 100644
> --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
> +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
> @@ -17,6 +17,7 @@ [Defines]
> CONSTRUCTOR = SmmCpuFeaturesLibConstructor
>
> [Sources]
> + CpuFeaturesLib.h
> SmmCpuFeaturesLib.c
> SmmCpuFeaturesLibNoStm.c
>
> diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
> index 50b9cc871302..89cd252ef44e 100644
> --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
> +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
> @@ -18,6 +18,7 @@ [Defines]
> CONSTRUCTOR = SmmCpuFeaturesLibStmConstructor
>
> [Sources]
> + CpuFeaturesLib.h
> SmmCpuFeaturesLib.c
> SmmStm.c
> SmmStm.h
>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/4] UefiCpuPkg/SmmCpuFeaturesLib: Cleanup library constructors
2021-02-13 0:58 ` [PATCH v2 2/4] UefiCpuPkg/SmmCpuFeaturesLib: Cleanup library constructors Michael Kubacki
@ 2021-02-15 19:31 ` Laszlo Ersek
0 siblings, 0 replies; 11+ messages in thread
From: Laszlo Ersek @ 2021-02-15 19:31 UTC (permalink / raw)
To: mikuback, devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar
On 02/13/21 01:58, mikuback@linux.microsoft.com wrote:
> From: Michael Kubacki <michael.kubacki@microsoft.com>
>
> There's currently two library instances:
> 1. SmmCpuFeaturesLib
> 2. SmmCpuFeaturesLibStm
>
> There's two constructor functions:
> 1. SmmCpuFeaturesLibConstructor()
> 2. SmmCpuFeaturesLibStmConstructor()
>
> SmmCpuFeaturesLibConstructor() is called by
> SmmCpuFeaturesLibStmConstructor() since the functionality in that
> function is required by both library instances.
>
> The declaration for SmmCpuFeaturesLibConstructor() is embedded in
> "SmmStm.c" instead of being declared in a header file. Further,
> that constructor function is called by the STM specific constructor.
>
> This change moves the common code to a function called
> CpuFeaturesLibInitialization() which is declared in an internal
> library header file "CpuFeaturesLib.h". Each constructor simply
> calls this function to perform the common functionality.
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> ---
> UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 19 +++++++----------
> UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibNoStm.c | 22 ++++++++++++++++++++
> UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c | 21 ++-----------------
> UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h | 12 +++++++++++
> 4 files changed, 43 insertions(+), 31 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> index 75bde752785a..e74f87f3f266 100644
> --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> @@ -2,6 +2,7 @@
> The CPU specific programming for PiSmmCpuDxeSmm module.
>
> Copyright (c) 2010 - 2019, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) Microsoft Corporation.<BR>
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
> **/
> @@ -63,19 +64,15 @@ BOOLEAN mNeedConfigureMtrrs = TRUE;
> BOOLEAN *mSmrrEnabled;
>
> /**
> - The constructor function
> + Performs library initialization.
>
> - @param[in] ImageHandle The firmware allocated handle for the EFI image.
> - @param[in] SystemTable A pointer to the EFI System Table.
> -
> - @retval EFI_SUCCESS The constructor always returns EFI_SUCCESS.
> + This initialization function contains common functionality shared betwen all
> + library instance constructors.
>
> **/
> -EFI_STATUS
> -EFIAPI
> -SmmCpuFeaturesLibConstructor (
> - IN EFI_HANDLE ImageHandle,
> - IN EFI_SYSTEM_TABLE *SystemTable
> +VOID
> +CpuFeaturesLibInitialization (
> + VOID
> )
> {
> UINT32 RegEax;
> @@ -162,8 +159,6 @@ SmmCpuFeaturesLibConstructor (
> //
> mSmrrEnabled = (BOOLEAN *)AllocatePool (sizeof (BOOLEAN) * PcdGet32 (PcdCpuMaxLogicalProcessorNumber));
> ASSERT (mSmrrEnabled != NULL);
> -
> - return EFI_SUCCESS;
> }
>
> /**
> diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibNoStm.c b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibNoStm.c
> index c562582ccee0..eebbbfd00a83 100644
> --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibNoStm.c
> +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibNoStm.c
> @@ -3,6 +3,7 @@ The CPU specific programming for PiSmmCpuDxeSmm module when STM support
> is not included.
>
> Copyright (c) 2010 - 2016, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) Microsoft Corporation.<BR>
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
> **/
> @@ -82,3 +83,24 @@ SmmCpuFeaturesInstallSmiHandler (
> )
> {
> }
> +
> +/**
> + The constructor function for the library instance without STM.
> +
> + @param[in] ImageHandle The firmware allocated handle for the EFI image.
> + @param[in] SystemTable A pointer to the EFI System Table.
> +
> + @retval EFI_SUCCESS The constructor always returns EFI_SUCCESS.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +SmmCpuFeaturesLibConstructor (
> + IN EFI_HANDLE ImageHandle,
> + IN EFI_SYSTEM_TABLE *SystemTable
> + )
> +{
> + CpuFeaturesLibInitialization ();
> +
> + return EFI_SUCCESS;
> +}
> diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c
> index b5aad41fdb64..4b6bf958cedf 100644
> --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c
> +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c
> @@ -30,22 +30,6 @@
> #define RDWR_ACCS 3
> #define FULL_ACCS 7
>
> -/**
> - The constructor function
> -
> - @param[in] ImageHandle The firmware allocated handle for the EFI image.
> - @param[in] SystemTable A pointer to the EFI System Table.
> -
> - @retval EFI_SUCCESS The constructor always returns EFI_SUCCESS.
> -
> -**/
> -EFI_STATUS
> -EFIAPI
> -SmmCpuFeaturesLibConstructor (
> - IN EFI_HANDLE ImageHandle,
> - IN EFI_SYSTEM_TABLE *SystemTable
> - );
> -
> EFI_HANDLE mStmSmmCpuHandle = NULL;
>
> BOOLEAN mLockLoadMonitor = FALSE;
> @@ -138,10 +122,9 @@ SmmCpuFeaturesLibStmConstructor (
> SmmCpuFeaturesLibStmSmiEntryFixupAddress ();
>
> //
> - // Call the common constructor function
> + // Perform library initialization common across all instances
> //
> - Status = SmmCpuFeaturesLibConstructor (ImageHandle, SystemTable);
> - ASSERT_EFI_ERROR (Status);
> + CpuFeaturesLibInitialization ();
>
> //
> // Lookup the MP Services Protocol
> diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h b/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h
> index 4645bbb066c9..f9a758e82558 100644
> --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h
> +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h
> @@ -9,6 +9,18 @@
> #ifndef _CPU_FEATURES_LIB_H_
> #define _CPU_FEATURES_LIB_H_
>
> +/**
> + Performs library initialization.
> +
> + This initialization function contains common functionality shared betwen all
> + library instance constructors.
> +
> +**/
> +VOID
> +CpuFeaturesLibInitialization (
> + VOID
> + );
> +
> /**
> Internal worker function that is called to complete CPU initialization at the
> end of SmmCpuFeaturesInitializeProcessor().
>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/4] UefiCpuPkg/SmmCpuFeaturesLib: Abstract PcdCpuMaxLogicalProcessorNumber
2021-02-13 0:58 ` [PATCH v2 3/4] UefiCpuPkg/SmmCpuFeaturesLib: Abstract PcdCpuMaxLogicalProcessorNumber Michael Kubacki
@ 2021-02-15 19:35 ` Laszlo Ersek
0 siblings, 0 replies; 11+ messages in thread
From: Laszlo Ersek @ 2021-02-15 19:35 UTC (permalink / raw)
To: mikuback, devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar
On 02/13/21 01:58, mikuback@linux.microsoft.com wrote:
> From: Michael Kubacki <michael.kubacki@microsoft.com>
>
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3218
>
> Adds a new function called GetCpuMaxLogicalProcessorNumber() to
> return the number of maximum CPU logical processors (currently
> gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber).
>
> This allows the the mechanism used to retrieve the CPU maximum
> logical processor number to be abstracted from the logic that
> needs the value.
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> ---
> UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 2 +-
> UefiCpuPkg/Library/SmmCpuFeaturesLib/TraditionalMmCpuFeaturesLib.c | 28 ++++++++++++++++++++
> UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h | 14 ++++++++++
> UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf | 1 +
> UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf | 1 +
> 5 files changed, 45 insertions(+), 1 deletion(-)
>
> diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> index e74f87f3f266..a494988898b8 100644
> --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> @@ -157,7 +157,7 @@ CpuFeaturesLibInitialization (
> //
> // Allocate array for state of SMRR enable on all CPUs
> //
> - mSmrrEnabled = (BOOLEAN *)AllocatePool (sizeof (BOOLEAN) * PcdGet32 (PcdCpuMaxLogicalProcessorNumber));
> + mSmrrEnabled = (BOOLEAN *)AllocatePool (sizeof (BOOLEAN) * GetCpuMaxLogicalProcessorNumber ());
> ASSERT (mSmrrEnabled != NULL);
> }
>
> diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/TraditionalMmCpuFeaturesLib.c b/UefiCpuPkg/Library/SmmCpuFeaturesLib/TraditionalMmCpuFeaturesLib.c
> new file mode 100644
> index 000000000000..6a4eff755d92
> --- /dev/null
> +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/TraditionalMmCpuFeaturesLib.c
> @@ -0,0 +1,28 @@
> +/** @file
> + Traditional MM CPU specific programming.
> +
> + Copyright (c) Microsoft Corporation.<BR>
> + SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Base.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.
> +
> +**/
> +UINT32
> +GetCpuMaxLogicalProcessorNumber (
> + VOID
> + )
> +{
> + return PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
> +}
> diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h b/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h
> index f9a758e82558..1a72ba4e2e13 100644
> --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h
> +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h
> @@ -31,4 +31,18 @@ FinishSmmCpuFeaturesInitializeProcessor (
> VOID
> );
>
> +/**
> + 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.
> +
> +**/
> +UINT32
> +GetCpuMaxLogicalProcessorNumber (
> + VOID
> + );
> +
> #endif
> diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
> index a6d8467d26aa..b32dbeab9383 100644
> --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
> +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
> @@ -20,6 +20,7 @@ [Sources]
> CpuFeaturesLib.h
> SmmCpuFeaturesLib.c
> SmmCpuFeaturesLibNoStm.c
> + TraditionalMmCpuFeaturesLib.c
>
> [Packages]
> MdePkg/MdePkg.dec
> diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
> index 89cd252ef44e..f72ef0a88088 100644
> --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
> +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
> @@ -22,6 +22,7 @@ [Sources]
> SmmCpuFeaturesLib.c
> SmmStm.c
> SmmStm.h
> + TraditionalMmCpuFeaturesLib.c
>
> [Sources.Ia32]
> Ia32/SmmStmSupport.c
>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 4/4] UefiCpuPkg/SmmCpuFeaturesLib: Add Standalone MM support
2021-02-13 0:58 ` [PATCH v2 4/4] UefiCpuPkg/SmmCpuFeaturesLib: Add Standalone MM support Michael Kubacki
@ 2021-02-15 20:33 ` Laszlo Ersek
2021-02-16 20:11 ` Michael Kubacki
0 siblings, 1 reply; 11+ messages in thread
From: Laszlo Ersek @ 2021-02-15 20:33 UTC (permalink / raw)
To: mikuback, devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar
On 02/13/21 01:58, mikuback@linux.microsoft.com wrote:
> From: Michael Kubacki <michael.kubacki@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.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Signed-off-by: Michael Kubacki <michael.kubacki@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.)
> 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.)
> +
> +
> +**/
> +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.)
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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 4/4] UefiCpuPkg/SmmCpuFeaturesLib: Add Standalone MM support
2021-02-15 20:33 ` Laszlo Ersek
@ 2021-02-16 20:11 ` Michael Kubacki
2021-02-17 14:19 ` Laszlo Ersek
0 siblings, 1 reply; 11+ messages in thread
From: Michael Kubacki @ 2021-02-16 20:11 UTC (permalink / raw)
To: Laszlo Ersek, devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar
On 2/15/2021 12:33 PM, Laszlo Ersek wrote:
> On 02/13/21 01:58, mikuback@linux.microsoft.com wrote:
>> From: Michael Kubacki <michael.kubacki@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.dong@intel.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Rahul Kumar <rahul1.kumar@intel.com>
>> Signed-off-by: Michael Kubacki <michael.kubacki@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
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
>>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 4/4] UefiCpuPkg/SmmCpuFeaturesLib: Add Standalone MM support
2021-02-16 20:11 ` Michael Kubacki
@ 2021-02-17 14:19 ` Laszlo Ersek
0 siblings, 0 replies; 11+ messages in thread
From: Laszlo Ersek @ 2021-02-17 14:19 UTC (permalink / raw)
To: Michael Kubacki, devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar
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, mikuback@linux.microsoft.com wrote:
>>> From: Michael Kubacki <michael.kubacki@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.dong@intel.com>
>>> Cc: Ray Ni <ray.ni@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Cc: Rahul Kumar <rahul1.kumar@intel.com>
>>> Signed-off-by: Michael Kubacki <michael.kubacki@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
>>>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-02-17 14:19 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-02-13 0:58 [PATCH v2 0/4] UefiCpuPkg/StandaloneMmCpuFeaturesLib: Add Standalone MM support Michael Kubacki
2021-02-13 0:58 ` [PATCH v2 1/4] UefiCpuPkg/SmmCpuFeaturesLib: Move multi-instance function decl to header Michael Kubacki
2021-02-15 19:23 ` Laszlo Ersek
2021-02-13 0:58 ` [PATCH v2 2/4] UefiCpuPkg/SmmCpuFeaturesLib: Cleanup library constructors Michael Kubacki
2021-02-15 19:31 ` Laszlo Ersek
2021-02-13 0:58 ` [PATCH v2 3/4] UefiCpuPkg/SmmCpuFeaturesLib: Abstract PcdCpuMaxLogicalProcessorNumber Michael Kubacki
2021-02-15 19:35 ` Laszlo Ersek
2021-02-13 0:58 ` [PATCH v2 4/4] UefiCpuPkg/SmmCpuFeaturesLib: Add Standalone MM support Michael Kubacki
2021-02-15 20:33 ` Laszlo Ersek
2021-02-16 20:11 ` Michael Kubacki
2021-02-17 14:19 ` Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox