* [PATCH v1 1/1] UefiCpuPkg/SmmCpuFeaturesLib: Add Standalone MM support
@ 2021-02-12 4:11 Michael Kubacki
2021-02-12 9:10 ` Laszlo Ersek
0 siblings, 1 reply; 3+ messages in thread
From: Michael Kubacki @ 2021-02-12 4:11 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 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 | 18 +++----
UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibNoStm.c | 2 +-
UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c | 10 ++--
UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.c | 50 +++++++++++++++++++
UefiCpuPkg/Library/SmmCpuFeaturesLib/TraditionalMmCpuFeaturesLib.c | 51 ++++++++++++++++++++
UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h | 39 +++++++++++++++
UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf | 4 +-
UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf | 2 +
UefiCpuPkg/Library/SmmCpuFeaturesLib/{SmmCpuFeaturesLib.inf => StandaloneMmCpuFeaturesLib.inf} | 16 +++---
UefiCpuPkg/UefiCpuPkg.dsc | 1 +
10 files changed, 169 insertions(+), 24 deletions(-)
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
index 8fed18cf0e17..bb45188dbfe0 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
@@ -6,7 +6,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>
@@ -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)
@@ -72,19 +73,14 @@ 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.
+ @retval EFI_SUCCESS This function always returns success.
**/
EFI_STATUS
-EFIAPI
-SmmCpuFeaturesLibConstructor (
- IN EFI_HANDLE ImageHandle,
- IN EFI_SYSTEM_TABLE *SystemTable
+CpuFeaturesLibInitialization (
+ VOID
)
{
UINT32 RegEax;
@@ -169,7 +165,7 @@ SmmCpuFeaturesLibConstructor (
//
// 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);
return EFI_SUCCESS;
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibNoStm.c b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibNoStm.c
index 3e63c5e27f98..0bd9483e97cf 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibNoStm.c
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibNoStm.c
@@ -7,7 +7,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
**/
-#include <PiSmm.h>
+#include <PiMm.h>
#include <Library/SmmCpuFeaturesLib.h>
/**
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c
index f7f8afacffb5..be394528bcaa 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>
@@ -30,17 +30,17 @@
#define FULL_ACCS 7
/**
- The constructor function
+ The Traditional MM library constructor.
@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.
+ @retval EFI_SUCCESS This constructor always returns success.
**/
EFI_STATUS
EFIAPI
-SmmCpuFeaturesLibConstructor (
+TraditionalMmCpuFeaturesLibConstructor (
IN EFI_HANDLE ImageHandle,
IN EFI_SYSTEM_TABLE *SystemTable
);
@@ -139,7 +139,7 @@ SmmCpuFeaturesLibStmConstructor (
//
// Call the common constructor function
//
- Status = SmmCpuFeaturesLibConstructor (ImageHandle, SystemTable);
+ Status = TraditionalMmCpuFeaturesLibConstructor (ImageHandle, SystemTable);
ASSERT_EFI_ERROR (Status);
//
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.c b/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.c
new file mode 100644
index 000000000000..ff27052a24b3
--- /dev/null
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.c
@@ -0,0 +1,50 @@
+/** @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
+ )
+{
+ return CpuFeaturesLibInitialization ();
+}
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/TraditionalMmCpuFeaturesLib.c b/UefiCpuPkg/Library/SmmCpuFeaturesLib/TraditionalMmCpuFeaturesLib.c
new file mode 100644
index 000000000000..842eed6d52b9
--- /dev/null
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/TraditionalMmCpuFeaturesLib.c
@@ -0,0 +1,51 @@
+/** @file
+ Traditional 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
+
+**/
+
+#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 PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
+}
+
+/**
+ The Traditional MM library constructor.
+
+ @param[in] ImageHandle The firmware allocated handle for the EFI image.
+ @param[in] SystemTable A pointer to the EFI System Table.
+
+ @retval EFI_SUCCESS This constructor always returns success.
+
+**/
+EFI_STATUS
+EFIAPI
+TraditionalMmCpuFeaturesLibConstructor (
+ IN EFI_HANDLE ImageHandle,
+ IN EFI_SYSTEM_TABLE *SystemTable
+ )
+{
+ return CpuFeaturesLibInitialization ();
+}
+
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h b/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h
new file mode 100644
index 000000000000..143bd5ea3e72
--- /dev/null
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h
@@ -0,0 +1,39 @@
+/** @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_
+
+/**
+ Performs library initialization.
+
+ @retval EFI_SUCCESS This function always returns success.
+
+**/
+EFI_STATUS
+CpuFeaturesLibInitialization (
+ 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 dd828baf69cb..6ad36f0c3c9b 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
@@ -14,9 +14,11 @@ [Defines]
MODULE_TYPE = DXE_SMM_DRIVER
VERSION_STRING = 1.0
LIBRARY_CLASS = SmmCpuFeaturesLib
- CONSTRUCTOR = SmmCpuFeaturesLibConstructor
+ CONSTRUCTOR = TraditionalMmCpuFeaturesLibConstructor
[Sources]
+ CpuFeaturesLib.h
+ TraditionalMmCpuFeaturesLib.c
SmmCpuFeaturesLib.c
SmmCpuFeaturesLibNoStm.c
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
index 50b9cc871302..cf06751af235 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
@@ -18,6 +18,8 @@ [Defines]
CONSTRUCTOR = SmmCpuFeaturesLibStmConstructor
[Sources]
+ CpuFeaturesLib.h
+ TraditionalMmCpuFeaturesLib.c
SmmCpuFeaturesLib.c
SmmStm.c
SmmStm.h
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf b/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.inf
similarity index 56%
copy from UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
copy to UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.inf
index dd828baf69cb..432db8e8fe1c 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.inf
@@ -1,22 +1,26 @@
## @file
-# The CPU specific programming for PiSmmCpuDxeSmm module.
+# 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 = SmmCpuFeaturesLib
+ BASE_NAME = StandaloneMmCpuFeaturesLib
MODULE_UNI_FILE = SmmCpuFeaturesLib.uni
- FILE_GUID = FC3DC10D-D271-422a-AFF3-CBCF70344431
- MODULE_TYPE = DXE_SMM_DRIVER
+ FILE_GUID = BB554A2D-F5DF-41D3-8C62-46476A2B2B18
+ MODULE_TYPE = MM_STANDALONE
VERSION_STRING = 1.0
+ PI_SPECIFICATION_VERSION = 0x00010032
LIBRARY_CLASS = SmmCpuFeaturesLib
- CONSTRUCTOR = SmmCpuFeaturesLibConstructor
+ CONSTRUCTOR = StandaloneMmCpuFeaturesLibConstructor
[Sources]
+ CpuFeaturesLib.h
+ StandaloneMmCpuFeaturesLib.c
SmmCpuFeaturesLib.c
SmmCpuFeaturesLibNoStm.c
@@ -30,5 +34,5 @@ [LibraryClasses]
MemoryAllocationLib
DebugLib
-[Pcd]
+[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] 3+ messages in thread* Re: [PATCH v1 1/1] UefiCpuPkg/SmmCpuFeaturesLib: Add Standalone MM support
2021-02-12 4:11 [PATCH v1 1/1] UefiCpuPkg/SmmCpuFeaturesLib: Add Standalone MM support Michael Kubacki
@ 2021-02-12 9:10 ` Laszlo Ersek
2021-02-13 1:03 ` Michael Kubacki
0 siblings, 1 reply; 3+ messages in thread
From: Laszlo Ersek @ 2021-02-12 9:10 UTC (permalink / raw)
To: mikuback, devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar
Hi Michael,
On 02/12/21 05:11, 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 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 | 18 +++----
> UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibNoStm.c | 2 +-
> UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c | 10 ++--
> UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.c | 50 +++++++++++++++++++
> UefiCpuPkg/Library/SmmCpuFeaturesLib/TraditionalMmCpuFeaturesLib.c | 51 ++++++++++++++++++++
> UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h | 39 +++++++++++++++
> UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf | 4 +-
> UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf | 2 +
> UefiCpuPkg/Library/SmmCpuFeaturesLib/{SmmCpuFeaturesLib.inf => StandaloneMmCpuFeaturesLib.inf} | 16 +++---
> UefiCpuPkg/UefiCpuPkg.dsc | 1 +
> 10 files changed, 169 insertions(+), 24 deletions(-)
I've found this patch surprisingly difficult to review.
The reason for that is two-fold, as much as I can determine.
(1) This patch should be split in two parts, namely:
(1a) interface extraction
(1b) adding the new library instance
(2) *However*, the real problem is that the pre-patch status is a mess
already. Consider the "SmmCpuFeaturesLibStm.inf" instance:
- this instance has a constructor function called
SmmCpuFeaturesLibStmConstructor()
- the "SmmStm.c" file declares the *other* instance's constructor
function, and does so without a header file
- the STM instance's constructor calls the other instance's constructor
like a normal function.
I would say that the original commit that introduced the STM instance
*like this* created technical debt.
Regardless of whether someone agrees with me on that or not, *after*
your patch, we'll have a totally awkward situation where *some*
polymorphism is implemented correctly -- namely, the mechanism
introduced by your patch, with two constructors calling a common helper
function --, while at the *same time*, some *other* polymorphism is
implemented incorrectly, or at least differently -- with a constructor
calling another instance's constructor.
It gets worse. There is *already* (i.e., pre-patch) a multi-instance
internal function, namely FinishSmmCpuFeaturesInitializeProcessor(). But
that function is not declared in any header file; the declaration is
embedded in "SmmCpuFeaturesLib.c"
All this is completely mind-boggling, and the main reason why this patch
is so difficult to review. For me anyway.
So here's what I suggest / request. In total, we're going to need *four*
patches.
(3) In the first patch, please start untangling the current (pre-patch)
mess. Namely:
(3a) Introduce the new header file ("CpuFeaturesLib.h"), but with *only*
the FinishSmmCpuFeaturesInitializeProcessor() declaration.
(3b) Remove the (embedded) declaration from its current spot.
(3c) Consume the new header file in *three* C files that define, or
call, FinishSmmCpuFeaturesInitializeProcessor().
(3d) Extend the INF files with the new header file.
(4) In the second patch, continue untangling the current mess:
(4a) declare the CpuFeaturesLibInitialization() function in the header
file added in (3a)
(4b) Rename the SmmCpuFeaturesLibConstructor() function in
"SmmCpuFeaturesLib.c" to CpuFeaturesLibInitialization()
(4c) In "SmmStm.c", remove the declaration of SmmCpuFeaturesLibConstructor()
(4d) In "SmmStm.c", call CpuFeaturesLibInitialization() rather than
SmmCpuFeaturesLibConstructor()
(4e) Re-introduce the SmmCpuFeaturesLibConstructor() function, but to
the file "SmmCpuFeaturesLibNoStm.c". The implementation should be just a
call to CpuFeaturesLibInitialization().
This step establishes the *good* pattern, from your current patch, for
the *existent* status.
(5) In the third patch:
(5a) declare the GetCpuMaxLogicalProcessorNumber() function in
"CpuFeaturesLib.h"
(5b) call GetCpuMaxLogicalProcessorNumber() rather than PcdGet32() in
CpuFeaturesLibInitialization(), in "SmmCpuFeaturesLib.c"
(5c) Introduce the "TraditionalMmCpuFeaturesLib.c" file, containing the
GetCpuMaxLogicalProcessorNumber() implementation only -- calling PcdGet32()
(5d) add the new C file to both INF files
(5e) This patch should not rename any files, should not rename any
functions, and should not change any <PiSmm.h> include directives.
(6) In the fourth patch:
(6a) introduce the files
StandaloneMmCpuFeaturesLib.c
StandaloneMmCpuFeaturesLib.inf
like they are in the present patch.
(6b) extend the DSC file with the new library instance
(6c) modify the <PiSmm.h> #include directives wherever necessary, but
*only* in those files where the update is really necessary.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v1 1/1] UefiCpuPkg/SmmCpuFeaturesLib: Add Standalone MM support
2021-02-12 9:10 ` Laszlo Ersek
@ 2021-02-13 1:03 ` Michael Kubacki
0 siblings, 0 replies; 3+ messages in thread
From: Michael Kubacki @ 2021-02-13 1:03 UTC (permalink / raw)
To: Laszlo Ersek, devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar
Hi Laszlo,
Thank you for the quick and detailed feedback. This is definitely the
right direction, I suppose someone had to clean it up :).
All of your suggestions are present in v2:
https://edk2.groups.io/g/devel/message/71656
Regards,
Michael
On 2/12/2021 1:10 AM, Laszlo Ersek wrote:
> Hi Michael,
>
> On 02/12/21 05:11, 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 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 | 18 +++----
>> UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibNoStm.c | 2 +-
>> UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c | 10 ++--
>> UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.c | 50 +++++++++++++++++++
>> UefiCpuPkg/Library/SmmCpuFeaturesLib/TraditionalMmCpuFeaturesLib.c | 51 ++++++++++++++++++++
>> UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h | 39 +++++++++++++++
>> UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf | 4 +-
>> UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf | 2 +
>> UefiCpuPkg/Library/SmmCpuFeaturesLib/{SmmCpuFeaturesLib.inf => StandaloneMmCpuFeaturesLib.inf} | 16 +++---
>> UefiCpuPkg/UefiCpuPkg.dsc | 1 +
>> 10 files changed, 169 insertions(+), 24 deletions(-)
>
> I've found this patch surprisingly difficult to review.
>
> The reason for that is two-fold, as much as I can determine.
>
> (1) This patch should be split in two parts, namely:
>
> (1a) interface extraction
>
> (1b) adding the new library instance
>
>
> (2) *However*, the real problem is that the pre-patch status is a mess
> already. Consider the "SmmCpuFeaturesLibStm.inf" instance:
>
> - this instance has a constructor function called
> SmmCpuFeaturesLibStmConstructor()
>
> - the "SmmStm.c" file declares the *other* instance's constructor
> function, and does so without a header file
>
> - the STM instance's constructor calls the other instance's constructor
> like a normal function.
>
>
> I would say that the original commit that introduced the STM instance
> *like this* created technical debt.
>
> Regardless of whether someone agrees with me on that or not, *after*
> your patch, we'll have a totally awkward situation where *some*
> polymorphism is implemented correctly -- namely, the mechanism
> introduced by your patch, with two constructors calling a common helper
> function --, while at the *same time*, some *other* polymorphism is
> implemented incorrectly, or at least differently -- with a constructor
> calling another instance's constructor.
>
> It gets worse. There is *already* (i.e., pre-patch) a multi-instance
> internal function, namely FinishSmmCpuFeaturesInitializeProcessor(). But
> that function is not declared in any header file; the declaration is
> embedded in "SmmCpuFeaturesLib.c"
>
> All this is completely mind-boggling, and the main reason why this patch
> is so difficult to review. For me anyway.
>
> So here's what I suggest / request. In total, we're going to need *four*
> patches.
>
>
> (3) In the first patch, please start untangling the current (pre-patch)
> mess. Namely:
>
> (3a) Introduce the new header file ("CpuFeaturesLib.h"), but with *only*
> the FinishSmmCpuFeaturesInitializeProcessor() declaration.
>
> (3b) Remove the (embedded) declaration from its current spot.
>
> (3c) Consume the new header file in *three* C files that define, or
> call, FinishSmmCpuFeaturesInitializeProcessor().
>
> (3d) Extend the INF files with the new header file.
>
>
> (4) In the second patch, continue untangling the current mess:
>
> (4a) declare the CpuFeaturesLibInitialization() function in the header
> file added in (3a)
>
> (4b) Rename the SmmCpuFeaturesLibConstructor() function in
> "SmmCpuFeaturesLib.c" to CpuFeaturesLibInitialization()
>
> (4c) In "SmmStm.c", remove the declaration of SmmCpuFeaturesLibConstructor()
>
> (4d) In "SmmStm.c", call CpuFeaturesLibInitialization() rather than
> SmmCpuFeaturesLibConstructor()
>
> (4e) Re-introduce the SmmCpuFeaturesLibConstructor() function, but to
> the file "SmmCpuFeaturesLibNoStm.c". The implementation should be just a
> call to CpuFeaturesLibInitialization().
>
> This step establishes the *good* pattern, from your current patch, for
> the *existent* status.
>
>
> (5) In the third patch:
>
> (5a) declare the GetCpuMaxLogicalProcessorNumber() function in
> "CpuFeaturesLib.h"
>
> (5b) call GetCpuMaxLogicalProcessorNumber() rather than PcdGet32() in
> CpuFeaturesLibInitialization(), in "SmmCpuFeaturesLib.c"
>
> (5c) Introduce the "TraditionalMmCpuFeaturesLib.c" file, containing the
> GetCpuMaxLogicalProcessorNumber() implementation only -- calling PcdGet32()
>
> (5d) add the new C file to both INF files
>
> (5e) This patch should not rename any files, should not rename any
> functions, and should not change any <PiSmm.h> include directives.
>
>
> (6) In the fourth patch:
>
> (6a) introduce the files
>
> StandaloneMmCpuFeaturesLib.c
> StandaloneMmCpuFeaturesLib.inf
>
> like they are in the present patch.
>
> (6b) extend the DSC file with the new library instance
>
> (6c) modify the <PiSmm.h> #include directives wherever necessary, but
> *only* in those files where the update is really necessary.
>
> Thanks
> Laszlo
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-02-13 1:03 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-02-12 4:11 [PATCH v1 1/1] UefiCpuPkg/SmmCpuFeaturesLib: Add Standalone MM support Michael Kubacki
2021-02-12 9:10 ` Laszlo Ersek
2021-02-13 1:03 ` Michael Kubacki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox