* [edk2-devel] [PATCH v2 0/6] Refine SMM CPU Sync flow and abstract SmmCpuSyncLib
@ 2023-11-30 6:31 Wu, Jiaxin
2023-11-30 6:31 ` [edk2-devel] [PATCH v2 1/6] UefiCpuPkg/PiSmmCpuDxeSmm: Optimize Semaphore Sync between BSP and AP Wu, Jiaxin
` (5 more replies)
0 siblings, 6 replies; 11+ messages in thread
From: Wu, Jiaxin @ 2023-11-30 6:31 UTC (permalink / raw)
To: devel
Cc: Laszlo Ersek, Eric Dong, Ray Ni, Zeng Star, Gerd Hoffmann,
Rahul Kumar, Ard Biesheuvel, Jiewen Yao, Jordan Justen, Guo Dong,
Sean Rhodes, James Lu, Gua Guo
The series patches are to refine SMM CPU Sync flow.
After the refinement, SmmCpuSyncLib is abstracted for
any user to provide different SMM CPU Sync implementation.
Compared to V1, has following refinement & changes:
1. Remove below patch from this series patches:
UefiCpuPkg/PiSmmCpuDxeSmm: Reduce times of BSP and AP Sync for SMM Exit
Reason: Plan to separate that patch into another patch set.
2. Refine the patch according Laszlo & Ray's comments:
a. Removed Change-Id in the patches
b. Optimized the description to avoid the confusing.
c. Fixed wrong function comment to make it correct and understandable.
d. Use an incomplete structure type to aviod the VOID*.
e. Corrected all functions params "in" & "out".
f. Added lots of comments for the library to explain the
operation & behavior.
g. Fixed inconsistent return types from lib APIs.
h. Use SafeIntLib for all calculations to prevent overflows.
i. Remove the UefiCpuPkg/UefiCpuLibs.dsc.inc
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Zeng Star <star.zeng@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Guo Dong <guo.dong@intel.com>
Cc: Sean Rhodes <sean@starlabs.systems>
Cc: James Lu <james.lu@intel.com>
Cc: Gua Guo <gua.guo@intel.com>
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
Jiaxin Wu (6):
UefiCpuPkg/PiSmmCpuDxeSmm: Optimize Semaphore Sync between BSP and AP
UefiCpuPkg: Adds SmmCpuSyncLib library class
UefiCpuPkg: Implements SmmCpuSyncLib library instance
OvmfPkg: Specifies SmmCpuSyncLib instance
UefiPayloadPkg: Specifies SmmCpuSyncLib instance
UefiCpuPkg/PiSmmCpuDxeSmm: Consume SmmCpuSyncLib
OvmfPkg/CloudHv/CloudHvX64.dsc | 2 +
OvmfPkg/OvmfPkgIa32.dsc | 2 +
OvmfPkg/OvmfPkgIa32X64.dsc | 2 +
OvmfPkg/OvmfPkgX64.dsc | 1 +
UefiCpuPkg/Include/Library/SmmCpuSyncLib.h | 278 +++++++++
UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.c | 690 +++++++++++++++++++++
UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.inf | 39 ++
UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 275 ++++----
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 6 +-
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 1 +
UefiCpuPkg/UefiCpuPkg.dec | 3 +
UefiCpuPkg/UefiCpuPkg.dsc | 3 +
UefiPayloadPkg/UefiPayloadPkg.dsc | 1 +
13 files changed, 1132 insertions(+), 171 deletions(-)
create mode 100644 UefiCpuPkg/Include/Library/SmmCpuSyncLib.h
create mode 100644 UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.c
create mode 100644 UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.inf
--
2.16.2.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111892): https://edk2.groups.io/g/devel/message/111892
Mute This Topic: https://groups.io/mt/102889290/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 11+ messages in thread
* [edk2-devel] [PATCH v2 1/6] UefiCpuPkg/PiSmmCpuDxeSmm: Optimize Semaphore Sync between BSP and AP
2023-11-30 6:31 [edk2-devel] [PATCH v2 0/6] Refine SMM CPU Sync flow and abstract SmmCpuSyncLib Wu, Jiaxin
@ 2023-11-30 6:31 ` Wu, Jiaxin
2023-11-30 6:31 ` [edk2-devel] [PATCH v2 2/6] UefiCpuPkg: Adds SmmCpuSyncLib library class Wu, Jiaxin
` (4 subsequent siblings)
5 siblings, 0 replies; 11+ messages in thread
From: Wu, Jiaxin @ 2023-11-30 6:31 UTC (permalink / raw)
To: devel
Cc: Laszlo Ersek, Eric Dong, Ray Ni, Zeng Star, Rahul Kumar,
Gerd Hoffmann
This patch is to define 3 new functions (WaitForBsp & ReleaseBsp &
ReleaseOneAp) used for the semaphore sync between BSP & AP. With the
change, BSP and AP Sync flow will be easy understand as below:
BSP: ReleaseAllAPs or ReleaseOneAp --> AP: WaitForBsp
BSP: WaitForAllAPs <-- AP: ReleaseBsp
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Zeng Star <star.zeng@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
---
UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 72 ++++++++++++++++++++++++++++-------
1 file changed, 58 insertions(+), 14 deletions(-)
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index b279f5dfcc..54542262a2 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -120,10 +120,11 @@ LockdownSemaphore (
return Value;
}
/**
+ Used for BSP to wait all APs.
Wait all APs to performs an atomic compare exchange operation to release semaphore.
@param NumberOfAPs AP number
**/
@@ -139,10 +140,11 @@ WaitForAllAPs (
WaitForSemaphore (mSmmMpSyncData->CpuData[BspIndex].Run);
}
}
/**
+ Used for BSP to release all APs.
Performs an atomic compare exchange operation to release semaphore
for each AP.
**/
VOID
@@ -157,10 +159,52 @@ ReleaseAllAPs (
ReleaseSemaphore (mSmmMpSyncData->CpuData[Index].Run);
}
}
}
+/**
+ Used for BSP to release one AP.
+
+ @param ApSem IN: 32-bit unsigned integer
+ OUT: original integer + 1
+**/
+VOID
+ReleaseOneAp (
+ IN OUT volatile UINT32 *ApSem
+ )
+{
+ ReleaseSemaphore (ApSem);
+}
+
+/**
+ Used for AP to wait BSP.
+
+ @param ApSem IN: 32-bit unsigned integer
+ OUT: original integer - 1
+**/
+VOID
+WaitForBsp (
+ IN OUT volatile UINT32 *ApSem
+ )
+{
+ WaitForSemaphore (ApSem);
+}
+
+/**
+ Used for AP to release BSP.
+
+ @param BspSem IN: 32-bit unsigned integer
+ OUT: original integer + 1
+**/
+VOID
+ReleaseBsp (
+ IN OUT volatile UINT32 *BspSem
+ )
+{
+ ReleaseSemaphore (BspSem);
+}
+
/**
Check whether the index of CPU perform the package level register
programming during System Management Mode initialization.
The index of Processor specified by mPackageFirstThreadIndex[PackageIndex]
@@ -632,11 +676,11 @@ BSPHandler (
// Signal all APs it's time for backup MTRRs
//
ReleaseAllAPs ();
//
- // WaitForSemaphore() may wait for ever if an AP happens to enter SMM at
+ // WaitForAllAPs() may wait for ever if an AP happens to enter SMM at
// exactly this point. Please make sure PcdCpuSmmMaxSyncLoops has been set
// to a large enough value to avoid this situation.
// Note: For HT capable CPUs, threads within a core share the same set of MTRRs.
// We do the backup first and then set MTRR to avoid race condition for threads
// in the same core.
@@ -652,11 +696,11 @@ BSPHandler (
// Let all processors program SMM MTRRs together
//
ReleaseAllAPs ();
//
- // WaitForSemaphore() may wait for ever if an AP happens to enter SMM at
+ // WaitForAllAPs() may wait for ever if an AP happens to enter SMM at
// exactly this point. Please make sure PcdCpuSmmMaxSyncLoops has been set
// to a large enough value to avoid this situation.
//
ReplaceOSMtrrs (CpuIndex);
@@ -898,50 +942,50 @@ APHandler (
if ((SyncMode == SmmCpuSyncModeTradition) || SmmCpuFeaturesNeedConfigureMtrrs ()) {
//
// Notify BSP of arrival at this point
//
- ReleaseSemaphore (mSmmMpSyncData->CpuData[BspIndex].Run);
+ ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run);
}
if (SmmCpuFeaturesNeedConfigureMtrrs ()) {
//
// Wait for the signal from BSP to backup MTRRs
//
- WaitForSemaphore (mSmmMpSyncData->CpuData[CpuIndex].Run);
+ WaitForBsp (mSmmMpSyncData->CpuData[CpuIndex].Run);
//
// Backup OS MTRRs
//
MtrrGetAllMtrrs (&Mtrrs);
//
// Signal BSP the completion of this AP
//
- ReleaseSemaphore (mSmmMpSyncData->CpuData[BspIndex].Run);
+ ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run);
//
// Wait for BSP's signal to program MTRRs
//
- WaitForSemaphore (mSmmMpSyncData->CpuData[CpuIndex].Run);
+ WaitForBsp (mSmmMpSyncData->CpuData[CpuIndex].Run);
//
// Replace OS MTRRs with SMI MTRRs
//
ReplaceOSMtrrs (CpuIndex);
//
// Signal BSP the completion of this AP
//
- ReleaseSemaphore (mSmmMpSyncData->CpuData[BspIndex].Run);
+ ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run);
}
while (TRUE) {
//
// Wait for something to happen
//
- WaitForSemaphore (mSmmMpSyncData->CpuData[CpuIndex].Run);
+ WaitForBsp (mSmmMpSyncData->CpuData[CpuIndex].Run);
//
// Check if BSP wants to exit SMM
//
if (!(*mSmmMpSyncData->InsideSmm)) {
@@ -977,16 +1021,16 @@ APHandler (
if (SmmCpuFeaturesNeedConfigureMtrrs ()) {
//
// Notify BSP the readiness of this AP to program MTRRs
//
- ReleaseSemaphore (mSmmMpSyncData->CpuData[BspIndex].Run);
+ ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run);
//
// Wait for the signal from BSP to program MTRRs
//
- WaitForSemaphore (mSmmMpSyncData->CpuData[CpuIndex].Run);
+ WaitForBsp (mSmmMpSyncData->CpuData[CpuIndex].Run);
//
// Restore OS MTRRs
//
SmmCpuFeaturesReenableSmrr ();
@@ -994,26 +1038,26 @@ APHandler (
}
//
// Notify BSP the readiness of this AP to Reset states/semaphore for this processor
//
- ReleaseSemaphore (mSmmMpSyncData->CpuData[BspIndex].Run);
+ ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run);
//
// Wait for the signal from BSP to Reset states/semaphore for this processor
//
- WaitForSemaphore (mSmmMpSyncData->CpuData[CpuIndex].Run);
+ WaitForBsp (mSmmMpSyncData->CpuData[CpuIndex].Run);
//
// Reset states/semaphore for this processor
//
*(mSmmMpSyncData->CpuData[CpuIndex].Present) = FALSE;
//
// Notify BSP the readiness of this AP to exit SMM
//
- ReleaseSemaphore (mSmmMpSyncData->CpuData[BspIndex].Run);
+ ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run);
}
/**
Checks whether the input token is the current used token.
@@ -1277,11 +1321,11 @@ InternalSmmStartupThisAp (
mSmmMpSyncData->CpuData[CpuIndex].Status = CpuStatus;
if (mSmmMpSyncData->CpuData[CpuIndex].Status != NULL) {
*mSmmMpSyncData->CpuData[CpuIndex].Status = EFI_NOT_READY;
}
- ReleaseSemaphore (mSmmMpSyncData->CpuData[CpuIndex].Run);
+ ReleaseOneAp (mSmmMpSyncData->CpuData[CpuIndex].Run);
if (Token == NULL) {
AcquireSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Busy);
ReleaseSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Busy);
}
--
2.16.2.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111893): https://edk2.groups.io/g/devel/message/111893
Mute This Topic: https://groups.io/mt/102889291/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [edk2-devel] [PATCH v2 2/6] UefiCpuPkg: Adds SmmCpuSyncLib library class
2023-11-30 6:31 [edk2-devel] [PATCH v2 0/6] Refine SMM CPU Sync flow and abstract SmmCpuSyncLib Wu, Jiaxin
2023-11-30 6:31 ` [edk2-devel] [PATCH v2 1/6] UefiCpuPkg/PiSmmCpuDxeSmm: Optimize Semaphore Sync between BSP and AP Wu, Jiaxin
@ 2023-11-30 6:31 ` Wu, Jiaxin
2023-12-06 1:08 ` Ni, Ray
2023-11-30 6:31 ` [edk2-devel] [PATCH v2 3/6] UefiCpuPkg: Implements SmmCpuSyncLib library instance Wu, Jiaxin
` (3 subsequent siblings)
5 siblings, 1 reply; 11+ messages in thread
From: Wu, Jiaxin @ 2023-11-30 6:31 UTC (permalink / raw)
To: devel
Cc: Laszlo Ersek, Eric Dong, Ray Ni, Zeng Star, Gerd Hoffmann,
Rahul Kumar
Intel is planning to provide different SMM CPU Sync implementation
along with some specific registers to improve the SMI performance,
hence need SmmCpuSyncLib Library for Intel.
This patch is to:
1.Adds SmmCpuSyncLib Library class in UefiCpuPkg.dec.
2.Adds SmmCpuSyncLib.h function declaration header file.
For the new SmmCpuSyncLib, it provides 3 sets of APIs:
1. ContextInit/ContextDeinit/ContextReset:
ContextInit() is called in driver's entrypoint to allocate and
initialize the SMM CPU Sync context. ContextDeinit() is called in
driver's unload function to deinitialize SMM CPU Sync context.
ContextReset() is called before CPU exist SMI, which allows CPU to
check into the next SMI from this point.
2. GetArrivedCpuCount/CheckInCpu/CheckOutCpu/LockDoor:
When SMI happens, all processors including BSP enter to SMM mode by
calling CheckInCpu(). The elected BSP calls LockDoor() so that
CheckInCpu() will return the error code after that. CheckOutCpu() can
be called in error handling flow for the CPU who calls CheckInCpu()
earlier. GetArrivedCpuCount() returns the number of checked-in CPUs.
3. WaitForAPs/ReleaseOneAp/WaitForBsp/ReleaseBsp
WaitForAPs() & ReleaseOneAp() are called from BSP to wait the number
of APs and release one specific AP. WaitForBsp() & ReleaseBsp() are
called from APs to wait and release BSP. The 4 APIs are used to
synchronize the running flow among BSP and APs. BSP and AP Sync flow
can be easy understand as below:
BSP: ReleaseOneAp --> AP: WaitForBsp
BSP: WaitForAPs <-- AP: ReleaseBsp
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Zeng Star <star.zeng@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
---
UefiCpuPkg/Include/Library/SmmCpuSyncLib.h | 278 +++++++++++++++++++++++++++++
UefiCpuPkg/UefiCpuPkg.dec | 3 +
2 files changed, 281 insertions(+)
create mode 100644 UefiCpuPkg/Include/Library/SmmCpuSyncLib.h
diff --git a/UefiCpuPkg/Include/Library/SmmCpuSyncLib.h b/UefiCpuPkg/Include/Library/SmmCpuSyncLib.h
new file mode 100644
index 0000000000..22935cc006
--- /dev/null
+++ b/UefiCpuPkg/Include/Library/SmmCpuSyncLib.h
@@ -0,0 +1,278 @@
+/** @file
+Library that provides SMM CPU Sync related operations.
+The lib provides 3 sets of APIs:
+1. ContextInit/ContextDeinit/ContextReset:
+ContextInit() is called in driver's entrypoint to allocate and initialize the SMM CPU Sync context.
+ContextDeinit() is called in driver's unload function to deinitialize the SMM CPU Sync context.
+ContextReset() is called before CPU exist SMI, which allows CPU to check into the next SMI from this point.
+
+2. GetArrivedCpuCount/CheckInCpu/CheckOutCpu/LockDoor:
+When SMI happens, all processors including BSP enter to SMM mode by calling CheckInCpu().
+The elected BSP calls LockDoor() so that CheckInCpu() will return the error code after that.
+CheckOutCpu() can be called in error handling flow for the CPU who calls CheckInCpu() earlier.
+GetArrivedCpuCount() returns the number of checked-in CPUs.
+
+3. WaitForAPs/ReleaseOneAp/WaitForBsp/ReleaseBsp
+WaitForAPs() & ReleaseOneAp() are called from BSP to wait the number of APs and release one specific AP.
+WaitForBsp() & ReleaseBsp() are called from APs to wait and release BSP.
+The 4 APIs are used to synchronize the running flow among BSP and APs. BSP and AP Sync flow can be
+easy understand as below:
+BSP: ReleaseOneAp --> AP: WaitForBsp
+BSP: WaitForAPs <-- AP: ReleaseBsp
+
+Copyright (c) 2023, Intel Corporation. All rights reserved.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef SMM_CPU_SYNC_LIB_H_
+#define SMM_CPU_SYNC_LIB_H_
+
+#include <Uefi/UefiBaseType.h>
+
+//
+// Opaque structure for SMM CPU Sync context.
+//
+typedef struct SMM_CPU_SYNC_CTX SMM_CPU_SYNC_CTX;
+
+/**
+ Create and initialize the SMM CPU Sync context.
+
+ SmmCpuSyncContextInit() function is to allocate and initialize the SMM CPU Sync context.
+
+ @param[in] NumberOfCpus The number of Logical Processors in the system.
+ @param[out] SmmCpuSyncCtx Pointer to the new created and initialized SMM CPU Sync context object.
+ NULL will be returned if any error happen during init.
+
+ @retval RETURN_SUCCESS The SMM CPU Sync context was successful created and initialized.
+ @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx is NULL.
+ @retval RETURN_BUFFER_TOO_SMALL Overflow happen
+ @retval RETURN_OUT_OF_RESOURCES There are not enough resources available to create and initialize SMM CPU Sync context.
+
+**/
+RETURN_STATUS
+EFIAPI
+SmmCpuSyncContextInit (
+ IN UINTN NumberOfCpus,
+ OUT SMM_CPU_SYNC_CTX **SmmCpuSyncCtx
+ );
+
+/**
+ Deinit an allocated SMM CPU Sync context.
+
+ SmmCpuSyncContextDeinit() function is to deinitialize SMM CPU Sync context, the resources allocated in
+ SmmCpuSyncContextInit() will be freed.
+
+ Note: This function only can be called after SmmCpuSyncContextInit() return success.
+
+ @param[in,out] SmmCpuSyncCtx Pointer to the SMM CPU Sync context object to be deinitialized.
+
+ @retval RETURN_SUCCESS The SMM CPU Sync context was successful deinitialized.
+ @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx is NULL.
+
+**/
+RETURN_STATUS
+EFIAPI
+SmmCpuSyncContextDeinit (
+ IN OUT SMM_CPU_SYNC_CTX *SmmCpuSyncCtx
+ );
+
+/**
+ Reset SMM CPU Sync context.
+
+ SmmCpuSyncContextReset() function is to reset SMM CPU Sync context to the initialized state.
+
+ @param[in,out] SmmCpuSyncCtx Pointer to the SMM CPU Sync context object to be reset.
+
+ @retval RETURN_SUCCESS The SMM CPU Sync context was successful reset.
+ @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx is NULL.
+
+**/
+RETURN_STATUS
+EFIAPI
+SmmCpuSyncContextReset (
+ IN OUT SMM_CPU_SYNC_CTX *SmmCpuSyncCtx
+ );
+
+/**
+ Get current number of arrived CPU in SMI.
+
+ For traditional CPU synchronization method, BSP might need to know the current number of arrived CPU in
+ SMI to make sure all APs in SMI. This API can be for that purpose.
+
+ @param[in] SmmCpuSyncCtx Pointer to the SMM CPU Sync context object.
+ @param[in,out] CpuCount Current count of arrived CPU in SMI.
+
+ @retval RETURN_SUCCESS Get current number of arrived CPU in SMI successfully.
+ @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx or CpuCount is NULL.
+ @retval RETURN_ABORTED Function Aborted due to the door has been locked by SmmCpuSyncLockDoor() function.
+
+**/
+RETURN_STATUS
+EFIAPI
+SmmCpuSyncGetArrivedCpuCount (
+ IN SMM_CPU_SYNC_CTX *SmmCpuSyncCtx,
+ IN OUT UINTN *CpuCount
+ );
+
+/**
+ Performs an atomic operation to check in CPU.
+
+ When SMI happens, all processors including BSP enter to SMM mode by calling SmmCpuSyncCheckInCpu().
+
+ @param[in,out] SmmCpuSyncCtx Pointer to the SMM CPU Sync context object.
+ @param[in] CpuIndex Check in CPU index.
+
+ @retval RETURN_SUCCESS Check in CPU (CpuIndex) successfully.
+ @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx is NULL.
+ @retval RETURN_ABORTED Check in CPU failed due to SmmCpuSyncLockDoor() has been called by one elected CPU.
+
+**/
+RETURN_STATUS
+EFIAPI
+SmmCpuSyncCheckInCpu (
+ IN OUT SMM_CPU_SYNC_CTX *SmmCpuSyncCtx,
+ IN UINTN CpuIndex
+ );
+
+/**
+ Performs an atomic operation to check out CPU.
+
+ CheckOutCpu() can be called in error handling flow for the CPU who calls CheckInCpu() earlier.
+
+ @param[in,out] SmmCpuSyncCtx Pointer to the SMM CPU Sync context object.
+ @param[in] CpuIndex Check out CPU index.
+
+ @retval RETURN_SUCCESS Check out CPU (CpuIndex) successfully.
+ @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx is NULL.
+ @retval RETURN_NOT_READY The CPU is not checked-in.
+ @retval RETURN_ABORTED Check out CPU failed due to SmmCpuSyncLockDoor() has been called by one elected CPU.
+
+**/
+RETURN_STATUS
+EFIAPI
+SmmCpuSyncCheckOutCpu (
+ IN OUT SMM_CPU_SYNC_CTX *SmmCpuSyncCtx,
+ IN UINTN CpuIndex
+ );
+
+/**
+ Performs an atomic operation lock door for CPU checkin or checkout.
+
+ After this function:
+ CPU can not check in via SmmCpuSyncCheckInCpu().
+ CPU can not check out via SmmCpuSyncCheckOutCpu().
+ CPU can not get number of arrived CPU in SMI via SmmCpuSyncGetArrivedCpuCount(). The number of
+ arrived CPU in SMI will be returned in CpuCount.
+
+ The CPU specified by CpuIndex is elected to lock door.
+
+ @param[in,out] SmmCpuSyncCtx Pointer to the SMM CPU Sync context object.
+ @param[in] CpuIndex Indicate which CPU to lock door.
+ @param[in,out] CpuCount Number of arrived CPU in SMI after look door.
+
+ @retval RETURN_SUCCESS Lock door for CPU successfully.
+ @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx or CpuCount is NULL.
+
+**/
+RETURN_STATUS
+EFIAPI
+SmmCpuSyncLockDoor (
+ IN OUT SMM_CPU_SYNC_CTX *SmmCpuSyncCtx,
+ IN UINTN CpuIndex,
+ IN OUT UINTN *CpuCount
+ );
+
+/**
+ Used by the BSP to wait for APs.
+
+ The number of APs need to be waited is specified by NumberOfAPs. The BSP is specified by BspIndex.
+
+ Note: This function is blocking mode, and it will return only after the number of APs released by
+ calling SmmCpuSyncReleaseBsp():
+ BSP: WaitForAPs <-- AP: ReleaseBsp
+
+ @param[in,out] SmmCpuSyncCtx Pointer to the SMM CPU Sync context object.
+ @param[in] NumberOfAPs Number of APs need to be waited by BSP.
+ @param[in] BspIndex The BSP Index to wait for APs.
+
+ @retval RETURN_SUCCESS BSP to wait for APs successfully.
+ @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx is NULL or NumberOfAPs > total number of processors in system.
+
+**/
+RETURN_STATUS
+EFIAPI
+SmmCpuSyncWaitForAPs (
+ IN OUT SMM_CPU_SYNC_CTX *SmmCpuSyncCtx,
+ IN UINTN NumberOfAPs,
+ IN UINTN BspIndex
+ );
+
+/**
+ Used by the BSP to release one AP.
+
+ The AP is specified by CpuIndex. The BSP is specified by BspIndex.
+
+ @param[in,out] SmmCpuSyncCtx Pointer to the SMM CPU Sync context object.
+ @param[in] CpuIndex Indicate which AP need to be released.
+ @param[in] BspIndex The BSP Index to release AP.
+
+ @retval RETURN_SUCCESS BSP to release one AP successfully.
+ @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx is NULL or CpuIndex is same as BspIndex.
+
+**/
+RETURN_STATUS
+EFIAPI
+SmmCpuSyncReleaseOneAp (
+ IN OUT SMM_CPU_SYNC_CTX *SmmCpuSyncCtx,
+ IN UINTN CpuIndex,
+ IN UINTN BspIndex
+ );
+
+/**
+ Used by the AP to wait BSP.
+
+ The AP is specified by CpuIndex. The BSP is specified by BspIndex.
+
+ Note: This function is blocking mode, and it will return only after the AP released by
+ calling SmmCpuSyncReleaseOneAp():
+ BSP: ReleaseOneAp --> AP: WaitForBsp
+
+ @param[in,out] SmmCpuSyncCtx Pointer to the SMM CPU Sync context object.
+ @param[in] CpuIndex Indicate which AP wait BSP.
+ @param[in] BspIndex The BSP Index to be waited.
+
+ @retval RETURN_SUCCESS AP to wait BSP successfully.
+ @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx is NULL or CpuIndex is same as BspIndex.
+
+**/
+RETURN_STATUS
+EFIAPI
+SmmCpuSyncWaitForBsp (
+ IN OUT SMM_CPU_SYNC_CTX *SmmCpuSyncCtx,
+ IN UINTN CpuIndex,
+ IN UINTN BspIndex
+ );
+
+/**
+ Used by the AP to release BSP.
+
+ The AP is specified by CpuIndex. The BSP is specified by BspIndex.
+
+ @param[in,out] SmmCpuSyncCtx Pointer to the SMM CPU Sync context object.
+ @param[in] CpuIndex Indicate which AP release BSP.
+ @param[in] BspIndex The BSP Index to be released.
+
+ @retval RETURN_SUCCESS AP to release BSP successfully.
+ @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx is NULL or CpuIndex is same as BspIndex.
+
+**/
+RETURN_STATUS
+EFIAPI
+SmmCpuSyncReleaseBsp (
+ IN OUT SMM_CPU_SYNC_CTX *SmmCpuSyncCtx,
+ IN UINTN CpuIndex,
+ IN UINTN BspIndex
+ );
+
+#endif
diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
index 0b5431dbf7..20ab079219 100644
--- a/UefiCpuPkg/UefiCpuPkg.dec
+++ b/UefiCpuPkg/UefiCpuPkg.dec
@@ -62,10 +62,13 @@
CpuPageTableLib|Include/Library/CpuPageTableLib.h
## @libraryclass Provides functions for manipulating smram savestate registers.
MmSaveStateLib|Include/Library/MmSaveStateLib.h
+ ## @libraryclass Provides functions for SMM CPU Sync Operation.
+ SmmCpuSyncLib|Include/Library/SmmCpuSyncLib.h
+
[LibraryClasses.RISCV64]
## @libraryclass Provides functions to manage MMU features on RISCV64 CPUs.
##
RiscVMmuLib|Include/Library/BaseRiscVMmuLib.h
--
2.16.2.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111894): https://edk2.groups.io/g/devel/message/111894
Mute This Topic: https://groups.io/mt/102889292/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [edk2-devel] [PATCH v2 3/6] UefiCpuPkg: Implements SmmCpuSyncLib library instance
2023-11-30 6:31 [edk2-devel] [PATCH v2 0/6] Refine SMM CPU Sync flow and abstract SmmCpuSyncLib Wu, Jiaxin
2023-11-30 6:31 ` [edk2-devel] [PATCH v2 1/6] UefiCpuPkg/PiSmmCpuDxeSmm: Optimize Semaphore Sync between BSP and AP Wu, Jiaxin
2023-11-30 6:31 ` [edk2-devel] [PATCH v2 2/6] UefiCpuPkg: Adds SmmCpuSyncLib library class Wu, Jiaxin
@ 2023-11-30 6:31 ` Wu, Jiaxin
2023-12-06 3:18 ` Ni, Ray
2023-11-30 6:31 ` [edk2-devel] [PATCH v2 4/6] OvmfPkg: Specifies SmmCpuSyncLib instance Wu, Jiaxin
` (2 subsequent siblings)
5 siblings, 1 reply; 11+ messages in thread
From: Wu, Jiaxin @ 2023-11-30 6:31 UTC (permalink / raw)
To: devel
Cc: Laszlo Ersek, Eric Dong, Ray Ni, Zeng Star, Gerd Hoffmann,
Rahul Kumar
Implements SmmCpuSyncLib Library instance. The instance refers the
existing SMM CPU driver (PiSmmCpuDxeSmm) sync implementation
and behavior:
1.Abstract Counter and Run semaphores into SmmCpuSyncCtx.
2.Abstract CPU arrival count operation to
SmmCpuSyncGetArrivedCpuCount(), SmmCpuSyncCheckInCpu(),
SmmCpuSyncCheckOutCpu(), SmmCpuSyncLockDoor().
Implementation is aligned with existing SMM CPU driver.
3. Abstract SMM CPU Sync flow to:
BSP: SmmCpuSyncReleaseOneAp --> AP: SmmCpuSyncWaitForBsp
BSP: SmmCpuSyncWaitForAPs <-- AP: SmmCpuSyncReleaseBsp
Semaphores release & wait during sync flow is same as existing SMM
CPU driver.
4.Same operation to Counter and Run semaphores by leverage the atomic
compare exchange.
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Zeng Star <star.zeng@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
---
UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.c | 690 +++++++++++++++++++++
UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.inf | 39 ++
UefiCpuPkg/UefiCpuPkg.dsc | 3 +
3 files changed, 732 insertions(+)
create mode 100644 UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.c
create mode 100644 UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.inf
diff --git a/UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.c b/UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.c
new file mode 100644
index 0000000000..307a6b3d78
--- /dev/null
+++ b/UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.c
@@ -0,0 +1,690 @@
+/** @file
+ SMM CPU Sync lib implementation.
+ The lib provides 3 sets of APIs:
+ 1. ContextInit/ContextDeinit/ContextReset:
+ ContextInit() is called in driver's entrypoint to allocate and initialize the SMM CPU Sync context.
+ ContextDeinit() is called in driver's unload function to deinitialize the SMM CPU Sync context.
+ ContextReset() is called before CPU exist SMI, which allows CPU to check into the next SMI from this point.
+
+ 2. GetArrivedCpuCount/CheckInCpu/CheckOutCpu/LockDoor:
+ When SMI happens, all processors including BSP enter to SMM mode by calling CheckInCpu().
+ The elected BSP calls LockDoor() so that CheckInCpu() will return the error code after that.
+ CheckOutCpu() can be called in error handling flow for the CPU who calls CheckInCpu() earlier.
+ GetArrivedCpuCount() returns the number of checked-in CPUs.
+
+ 3. WaitForAPs/ReleaseOneAp/WaitForBsp/ReleaseBsp
+ WaitForAPs() & ReleaseOneAp() are called from BSP to wait the number of APs and release one specific AP.
+ WaitForBsp() & ReleaseBsp() are called from APs to wait and release BSP.
+ The 4 APIs are used to synchronize the running flow among BSP and APs. BSP and AP Sync flow can be
+ easy understand as below:
+ BSP: ReleaseOneAp --> AP: WaitForBsp
+ BSP: WaitForAPs <-- AP: ReleaseBsp
+
+ Copyright (c) 2023, Intel Corporation. All rights reserved.<BR>
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <Base.h>
+#include <Uefi.h>
+#include <Library/UefiLib.h>
+#include <Library/BaseLib.h>
+#include <Library/DebugLib.h>
+#include <Library/SafeIntLib.h>
+#include <Library/SynchronizationLib.h>
+#include <Library/DebugLib.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/SmmServicesTableLib.h>
+#include <Library/MemoryAllocationLib.h>
+#include <Library/SmmCpuSyncLib.h>
+
+typedef struct {
+ ///
+ /// Indicate how many CPU entered SMM.
+ ///
+ volatile UINT32 *Counter;
+} SMM_CPU_SYNC_SEMAPHORE_GLOBAL;
+
+typedef struct {
+ ///
+ /// Used for control each CPU continue run or wait for signal
+ ///
+ volatile UINT32 *Run;
+} SMM_CPU_SYNC_SEMAPHORE_CPU;
+
+struct SMM_CPU_SYNC_CTX {
+ ///
+ /// All global semaphores' pointer in SMM CPU Sync
+ ///
+ SMM_CPU_SYNC_SEMAPHORE_GLOBAL *GlobalSem;
+ ///
+ /// All semaphores for each processor in SMM CPU Sync
+ ///
+ SMM_CPU_SYNC_SEMAPHORE_CPU *CpuSem;
+ ///
+ /// The number of processors in the system.
+ /// This does not indicate the number of processors that entered SMM.
+ ///
+ UINTN NumberOfCpus;
+ ///
+ /// Address of global and each CPU semaphores
+ ///
+ UINTN *SemBlock;
+ ///
+ /// Size of global and each CPU semaphores
+ ///
+ UINTN SemBlockPages;
+};
+
+/**
+ Performs an atomic compare exchange operation to get semaphore.
+ The compare exchange operation must be performed using MP safe
+ mechanisms.
+
+ @param[in,out] Sem IN: 32-bit unsigned integer
+ OUT: original integer - 1
+
+ @retval Original integer - 1
+
+**/
+UINT32
+InternalWaitForSemaphore (
+ IN OUT volatile UINT32 *Sem
+ )
+{
+ UINT32 Value;
+
+ for ( ; ;) {
+ Value = *Sem;
+ if ((Value != 0) &&
+ (InterlockedCompareExchange32 (
+ (UINT32 *)Sem,
+ Value,
+ Value - 1
+ ) == Value))
+ {
+ break;
+ }
+
+ CpuPause ();
+ }
+
+ return Value - 1;
+}
+
+/**
+ Performs an atomic compare exchange operation to release semaphore.
+ The compare exchange operation must be performed using MP safe
+ mechanisms.
+
+ @param[in,out] Sem IN: 32-bit unsigned integer
+ OUT: original integer + 1
+
+ @retval Original integer + 1
+
+**/
+UINT32
+InternalReleaseSemaphore (
+ IN OUT volatile UINT32 *Sem
+ )
+{
+ UINT32 Value;
+
+ do {
+ Value = *Sem;
+ } while (Value + 1 != 0 &&
+ InterlockedCompareExchange32 (
+ (UINT32 *)Sem,
+ Value,
+ Value + 1
+ ) != Value);
+
+ return Value + 1;
+}
+
+/**
+ Performs an atomic compare exchange operation to lock semaphore.
+ The compare exchange operation must be performed using MP safe
+ mechanisms.
+
+ @param[in,out] Sem IN: 32-bit unsigned integer
+ OUT: -1
+
+ @retval Original integer
+
+**/
+UINT32
+InternalLockdownSemaphore (
+ IN OUT volatile UINT32 *Sem
+ )
+{
+ UINT32 Value;
+
+ do {
+ Value = *Sem;
+ } while (InterlockedCompareExchange32 (
+ (UINT32 *)Sem,
+ Value,
+ (UINT32)-1
+ ) != Value);
+
+ return Value;
+}
+
+/**
+ Create and initialize the SMM CPU Sync context.
+
+ SmmCpuSyncContextInit() function is to allocate and initialize the SMM CPU Sync context.
+
+ @param[in] NumberOfCpus The number of Logical Processors in the system.
+ @param[out] SmmCpuSyncCtx Pointer to the new created and initialized SMM CPU Sync context object.
+ NULL will be returned if any error happen during init.
+
+ @retval RETURN_SUCCESS The SMM CPU Sync context was successful created and initialized.
+ @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx is NULL.
+ @retval RETURN_BUFFER_TOO_SMALL Overflow happen
+ @retval RETURN_OUT_OF_RESOURCES There are not enough resources available to create and initialize SMM CPU Sync context.
+
+**/
+RETURN_STATUS
+EFIAPI
+SmmCpuSyncContextInit (
+ IN UINTN NumberOfCpus,
+ OUT SMM_CPU_SYNC_CTX **SmmCpuSyncCtx
+ )
+{
+ RETURN_STATUS Status;
+
+ UINTN CpuSemInCtxSize;
+ UINTN CtxSize;
+
+ UINTN OneSemSize;
+ UINTN GlobalSemSize;
+ UINTN OneCpuSemSize;
+ UINTN CpuSemSize;
+ UINTN TotalSemSize;
+
+ UINTN SemAddr;
+ UINTN CpuIndex;
+
+ if (SmmCpuSyncCtx == NULL) {
+ return RETURN_INVALID_PARAMETER;
+ }
+
+ //
+ // Count the CtxSize
+ //
+ Status = SafeUintnMult (NumberOfCpus, sizeof (SMM_CPU_SYNC_SEMAPHORE_CPU), &CpuSemInCtxSize);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ Status = SafeUintnAdd (sizeof (SMM_CPU_SYNC_CTX), CpuSemInCtxSize, &CtxSize);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ Status = SafeUintnAdd (CtxSize, sizeof (SMM_CPU_SYNC_SEMAPHORE_GLOBAL), &CtxSize);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ //
+ // Allocate CtxSize buffer for the *SmmCpuSyncCtx
+ //
+ *SmmCpuSyncCtx = NULL;
+ *SmmCpuSyncCtx = (SMM_CPU_SYNC_CTX *)AllocatePages (EFI_SIZE_TO_PAGES (CtxSize));
+ ASSERT (*SmmCpuSyncCtx != NULL);
+ if (*SmmCpuSyncCtx == NULL) {
+ return RETURN_OUT_OF_RESOURCES;
+ }
+
+ (*SmmCpuSyncCtx)->GlobalSem = (SMM_CPU_SYNC_SEMAPHORE_GLOBAL *)((UINT8 *)(*SmmCpuSyncCtx) + sizeof (SMM_CPU_SYNC_CTX));
+ (*SmmCpuSyncCtx)->CpuSem = (SMM_CPU_SYNC_SEMAPHORE_CPU *)((UINT8 *)(*SmmCpuSyncCtx) + sizeof (SMM_CPU_SYNC_CTX) + sizeof (SMM_CPU_SYNC_SEMAPHORE_GLOBAL));
+ (*SmmCpuSyncCtx)->NumberOfCpus = NumberOfCpus;
+
+ //
+ // Count the TotalSemSize
+ //
+ OneSemSize = GetSpinLockProperties ();
+
+ Status = SafeUintnMult (OneSemSize, sizeof (SMM_CPU_SYNC_SEMAPHORE_GLOBAL) / sizeof (VOID *), &GlobalSemSize);
+ if (EFI_ERROR (Status)) {
+ goto ON_ERROR;
+ }
+
+ Status = SafeUintnMult (OneSemSize, sizeof (SMM_CPU_SYNC_SEMAPHORE_CPU) / sizeof (VOID *), &OneCpuSemSize);
+ if (EFI_ERROR (Status)) {
+ goto ON_ERROR;
+ }
+
+ Status = SafeUintnMult (NumberOfCpus, OneCpuSemSize, &CpuSemSize);
+ if (EFI_ERROR (Status)) {
+ goto ON_ERROR;
+ }
+
+ Status = SafeUintnAdd (GlobalSemSize, CpuSemSize, &TotalSemSize);
+ if (EFI_ERROR (Status)) {
+ goto ON_ERROR;
+ }
+
+ DEBUG ((DEBUG_INFO, "[%a] - One Semaphore Size = 0x%x\n", __func__, OneSemSize));
+ DEBUG ((DEBUG_INFO, "[%a] - Total Semaphores Size = 0x%x\n", __func__, TotalSemSize));
+
+ //
+ // Allocate for Semaphores in the *SmmCpuSyncCtx
+ //
+ (*SmmCpuSyncCtx)->SemBlockPages = EFI_SIZE_TO_PAGES (TotalSemSize);
+ (*SmmCpuSyncCtx)->SemBlock = AllocatePages ((*SmmCpuSyncCtx)->SemBlockPages);
+ ASSERT ((*SmmCpuSyncCtx)->SemBlock != NULL);
+ if ((*SmmCpuSyncCtx)->SemBlock == NULL) {
+ Status = RETURN_OUT_OF_RESOURCES;
+ goto ON_ERROR;
+ }
+
+ ZeroMem ((*SmmCpuSyncCtx)->SemBlock, TotalSemSize);
+
+ //
+ // Assign Global Semaphore pointer
+ //
+ SemAddr = (UINTN)(*SmmCpuSyncCtx)->SemBlock;
+ (*SmmCpuSyncCtx)->GlobalSem->Counter = (UINT32 *)SemAddr;
+ *(*SmmCpuSyncCtx)->GlobalSem->Counter = 0;
+ DEBUG ((DEBUG_INFO, "[%a] - (*SmmCpuSyncCtx)->GlobalSem->Counter Address: 0x%08x\n", __func__, (UINTN)(*SmmCpuSyncCtx)->GlobalSem->Counter));
+
+ SemAddr += GlobalSemSize;
+
+ //
+ // Assign CPU Semaphore pointer
+ //
+ for (CpuIndex = 0; CpuIndex < NumberOfCpus; CpuIndex++) {
+ (*SmmCpuSyncCtx)->CpuSem[CpuIndex].Run = (UINT32 *)(SemAddr + (CpuSemSize / NumberOfCpus) * CpuIndex);
+ *(*SmmCpuSyncCtx)->CpuSem[CpuIndex].Run = 0;
+ DEBUG ((DEBUG_INFO, "[%a] - (*SmmCpuSyncCtx)->CpuSem[%d].Run Address: 0x%08x\n", __func__, CpuIndex, (UINTN)(*SmmCpuSyncCtx)->CpuSem[CpuIndex].Run));
+ }
+
+ return RETURN_SUCCESS;
+
+ON_ERROR:
+ FreePages (*SmmCpuSyncCtx, EFI_SIZE_TO_PAGES (CtxSize));
+ return Status;
+}
+
+/**
+ Deinit an allocated SMM CPU Sync context.
+
+ SmmCpuSyncContextDeinit() function is to deinitialize SMM CPU Sync context, the resources allocated in
+ SmmCpuSyncContextInit() will be freed.
+
+ Note: This function only can be called after SmmCpuSyncContextInit() return success.
+
+ @param[in,out] SmmCpuSyncCtx Pointer to the SMM CPU Sync context object to be deinitialized.
+
+ @retval RETURN_SUCCESS The SMM CPU Sync context was successful deinitialized.
+ @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx is NULL.
+
+**/
+RETURN_STATUS
+EFIAPI
+SmmCpuSyncContextDeinit (
+ IN OUT SMM_CPU_SYNC_CTX *SmmCpuSyncCtx
+ )
+{
+ SMM_CPU_SYNC_CTX *Ctx;
+ UINTN CtxSize;
+
+ if (SmmCpuSyncCtx == NULL) {
+ return RETURN_INVALID_PARAMETER;
+ }
+
+ Ctx = (SMM_CPU_SYNC_CTX *)SmmCpuSyncCtx;
+
+ //
+ // Don't need Safe multiplication & Addition here since it has already checked in
+ // SmmCpuSyncContextInit(), and this function only can be called after SmmCpuSyncContextInit()
+ // return success.
+ //
+ CtxSize = sizeof (SMM_CPU_SYNC_CTX) + sizeof (SMM_CPU_SYNC_SEMAPHORE_GLOBAL) + sizeof (SMM_CPU_SYNC_SEMAPHORE_CPU) * (Ctx->NumberOfCpus);
+
+ FreePages (Ctx->SemBlock, Ctx->SemBlockPages);
+
+ FreePages (Ctx, EFI_SIZE_TO_PAGES (CtxSize));
+
+ return RETURN_SUCCESS;
+}
+
+/**
+ Reset SMM CPU Sync context.
+
+ SmmCpuSyncContextReset() function is to reset SMM CPU Sync context to the initialized state.
+
+ @param[in,out] SmmCpuSyncCtx Pointer to the SMM CPU Sync context object to be reset.
+
+ @retval RETURN_SUCCESS The SMM CPU Sync context was successful reset.
+ @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx is NULL.
+
+**/
+RETURN_STATUS
+EFIAPI
+SmmCpuSyncContextReset (
+ IN OUT SMM_CPU_SYNC_CTX *SmmCpuSyncCtx
+ )
+{
+ SMM_CPU_SYNC_CTX *Ctx;
+
+ if (SmmCpuSyncCtx == NULL) {
+ return RETURN_INVALID_PARAMETER;
+ }
+
+ Ctx = (SMM_CPU_SYNC_CTX *)SmmCpuSyncCtx;
+
+ *Ctx->GlobalSem->Counter = 0;
+
+ return RETURN_SUCCESS;
+}
+
+/**
+ Get current number of arrived CPU in SMI.
+
+ For traditional CPU synchronization method, BSP might need to know the current number of arrived CPU in
+ SMI to make sure all APs in SMI. This API can be for that purpose.
+
+ @param[in] SmmCpuSyncCtx Pointer to the SMM CPU Sync context object.
+ @param[in,out] CpuCount Current count of arrived CPU in SMI.
+
+ @retval RETURN_SUCCESS Get current number of arrived CPU in SMI successfully.
+ @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx or CpuCount is NULL.
+ @retval RETURN_ABORTED Function Aborted due to the door has been locked by SmmCpuSyncLockDoor() function.
+
+**/
+RETURN_STATUS
+EFIAPI
+SmmCpuSyncGetArrivedCpuCount (
+ IN SMM_CPU_SYNC_CTX *SmmCpuSyncCtx,
+ IN OUT UINTN *CpuCount
+ )
+{
+ SMM_CPU_SYNC_CTX *Ctx;
+
+ if ((SmmCpuSyncCtx == NULL) || (CpuCount == NULL)) {
+ return RETURN_INVALID_PARAMETER;
+ }
+
+ Ctx = (SMM_CPU_SYNC_CTX *)SmmCpuSyncCtx;
+
+ if (*Ctx->GlobalSem->Counter < 0) {
+ return RETURN_ABORTED;
+ }
+
+ *CpuCount = *Ctx->GlobalSem->Counter;
+
+ return RETURN_SUCCESS;
+}
+
+/**
+ Performs an atomic operation to check in CPU.
+
+ When SMI happens, all processors including BSP enter to SMM mode by calling SmmCpuSyncCheckInCpu().
+
+ @param[in,out] SmmCpuSyncCtx Pointer to the SMM CPU Sync context object.
+ @param[in] CpuIndex Check in CPU index.
+
+ @retval RETURN_SUCCESS Check in CPU (CpuIndex) successfully.
+ @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx is NULL.
+ @retval RETURN_ABORTED Check in CPU failed due to SmmCpuSyncLockDoor() has been called by one elected CPU.
+
+**/
+RETURN_STATUS
+EFIAPI
+SmmCpuSyncCheckInCpu (
+ IN OUT SMM_CPU_SYNC_CTX *SmmCpuSyncCtx,
+ IN UINTN CpuIndex
+ )
+{
+ SMM_CPU_SYNC_CTX *Ctx;
+
+ if (SmmCpuSyncCtx == NULL) {
+ return RETURN_INVALID_PARAMETER;
+ }
+
+ Ctx = (SMM_CPU_SYNC_CTX *)SmmCpuSyncCtx;
+
+ //
+ // Check to return if Counter has already been locked.
+ //
+ if ((INT32)InternalReleaseSemaphore (Ctx->GlobalSem->Counter) <= 0) {
+ return RETURN_ABORTED;
+ }
+
+ return RETURN_SUCCESS;
+}
+
+/**
+ Performs an atomic operation to check out CPU.
+
+ CheckOutCpu() can be called in error handling flow for the CPU who calls CheckInCpu() earlier.
+
+ @param[in,out] SmmCpuSyncCtx Pointer to the SMM CPU Sync context object.
+ @param[in] CpuIndex Check out CPU index.
+
+ @retval RETURN_SUCCESS Check out CPU (CpuIndex) successfully.
+ @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx is NULL.
+ @retval RETURN_NOT_READY The CPU is not checked-in.
+ @retval RETURN_ABORTED Check out CPU failed due to SmmCpuSyncLockDoor() has been called by one elected CPU.
+
+**/
+RETURN_STATUS
+EFIAPI
+SmmCpuSyncCheckOutCpu (
+ IN OUT SMM_CPU_SYNC_CTX *SmmCpuSyncCtx,
+ IN UINTN CpuIndex
+ )
+{
+ SMM_CPU_SYNC_CTX *Ctx;
+
+ if (SmmCpuSyncCtx == NULL) {
+ return RETURN_INVALID_PARAMETER;
+ }
+
+ Ctx = (SMM_CPU_SYNC_CTX *)SmmCpuSyncCtx;
+
+ if (*Ctx->GlobalSem->Counter == 0) {
+ return RETURN_NOT_READY;
+ }
+
+ if ((INT32)InternalWaitForSemaphore (Ctx->GlobalSem->Counter) < 0) {
+ return RETURN_ABORTED;
+ }
+
+ return RETURN_SUCCESS;
+}
+
+/**
+ Performs an atomic operation lock door for CPU checkin or checkout.
+
+ After this function:
+ CPU can not check in via SmmCpuSyncCheckInCpu().
+ CPU can not check out via SmmCpuSyncCheckOutCpu().
+ CPU can not get number of arrived CPU in SMI via SmmCpuSyncGetArrivedCpuCount(). The number of
+ arrived CPU in SMI will be returned in CpuCount.
+
+ The CPU specified by CpuIndex is elected to lock door.
+
+ @param[in,out] SmmCpuSyncCtx Pointer to the SMM CPU Sync context object.
+ @param[in] CpuIndex Indicate which CPU to lock door.
+ @param[in,out] CpuCount Number of arrived CPU in SMI after look door.
+
+ @retval RETURN_SUCCESS Lock door for CPU successfully.
+ @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx or CpuCount is NULL.
+
+**/
+RETURN_STATUS
+EFIAPI
+SmmCpuSyncLockDoor (
+ IN OUT SMM_CPU_SYNC_CTX *SmmCpuSyncCtx,
+ IN UINTN CpuIndex,
+ IN OUT UINTN *CpuCount
+ )
+{
+ SMM_CPU_SYNC_CTX *Ctx;
+
+ if ((SmmCpuSyncCtx == NULL) || (CpuCount == NULL)) {
+ return RETURN_INVALID_PARAMETER;
+ }
+
+ Ctx = (SMM_CPU_SYNC_CTX *)SmmCpuSyncCtx;
+
+ *CpuCount = InternalLockdownSemaphore (Ctx->GlobalSem->Counter);
+
+ return RETURN_SUCCESS;
+}
+
+/**
+ Used by the BSP to wait for APs.
+
+ The number of APs need to be waited is specified by NumberOfAPs. The BSP is specified by BspIndex.
+
+ Note: This function is blocking mode, and it will return only after the number of APs released by
+ calling SmmCpuSyncReleaseBsp():
+ BSP: WaitForAPs <-- AP: ReleaseBsp
+
+ @param[in,out] SmmCpuSyncCtx Pointer to the SMM CPU Sync context object.
+ @param[in] NumberOfAPs Number of APs need to be waited by BSP.
+ @param[in] BspIndex The BSP Index to wait for APs.
+
+ @retval RETURN_SUCCESS BSP to wait for APs successfully.
+ @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx is NULL or NumberOfAPs > total number of processors in system.
+
+**/
+RETURN_STATUS
+EFIAPI
+SmmCpuSyncWaitForAPs (
+ IN OUT SMM_CPU_SYNC_CTX *SmmCpuSyncCtx,
+ IN UINTN NumberOfAPs,
+ IN UINTN BspIndex
+ )
+{
+ SMM_CPU_SYNC_CTX *Ctx;
+
+ if (SmmCpuSyncCtx == NULL) {
+ return RETURN_INVALID_PARAMETER;
+ }
+
+ Ctx = (SMM_CPU_SYNC_CTX *)SmmCpuSyncCtx;
+
+ if (NumberOfAPs > Ctx->NumberOfCpus) {
+ return RETURN_INVALID_PARAMETER;
+ }
+
+ while (NumberOfAPs-- > 0) {
+ InternalWaitForSemaphore (Ctx->CpuSem[BspIndex].Run);
+ }
+
+ return RETURN_SUCCESS;
+}
+
+/**
+ Used by the BSP to release one AP.
+
+ The AP is specified by CpuIndex. The BSP is specified by BspIndex.
+
+ @param[in,out] SmmCpuSyncCtx Pointer to the SMM CPU Sync context object.
+ @param[in] CpuIndex Indicate which AP need to be released.
+ @param[in] BspIndex The BSP Index to release AP.
+
+ @retval RETURN_SUCCESS BSP to release one AP successfully.
+ @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx is NULL or CpuIndex is same as BspIndex.
+
+**/
+RETURN_STATUS
+EFIAPI
+SmmCpuSyncReleaseOneAp (
+ IN OUT SMM_CPU_SYNC_CTX *SmmCpuSyncCtx,
+ IN UINTN CpuIndex,
+ IN UINTN BspIndex
+ )
+{
+ SMM_CPU_SYNC_CTX *Ctx;
+
+ if ((SmmCpuSyncCtx == NULL) || (BspIndex == CpuIndex)) {
+ return RETURN_INVALID_PARAMETER;
+ }
+
+ Ctx = (SMM_CPU_SYNC_CTX *)SmmCpuSyncCtx;
+
+ InternalReleaseSemaphore (Ctx->CpuSem[CpuIndex].Run);
+
+ return RETURN_SUCCESS;
+}
+
+/**
+ Used by the AP to wait BSP.
+
+ The AP is specified by CpuIndex. The BSP is specified by BspIndex.
+
+ Note: This function is blocking mode, and it will return only after the AP released by
+ calling SmmCpuSyncReleaseOneAp():
+ BSP: ReleaseOneAp --> AP: WaitForBsp
+
+ @param[in,out] SmmCpuSyncCtx Pointer to the SMM CPU Sync context object.
+ @param[in] CpuIndex Indicate which AP wait BSP.
+ @param[in] BspIndex The BSP Index to be waited.
+
+ @retval RETURN_SUCCESS AP to wait BSP successfully.
+ @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx is NULL or CpuIndex is same as BspIndex.
+
+**/
+RETURN_STATUS
+EFIAPI
+SmmCpuSyncWaitForBsp (
+ IN OUT SMM_CPU_SYNC_CTX *SmmCpuSyncCtx,
+ IN UINTN CpuIndex,
+ IN UINTN BspIndex
+ )
+{
+ SMM_CPU_SYNC_CTX *Ctx;
+
+ if ((SmmCpuSyncCtx == NULL) || (BspIndex == CpuIndex)) {
+ return RETURN_INVALID_PARAMETER;
+ }
+
+ Ctx = (SMM_CPU_SYNC_CTX *)SmmCpuSyncCtx;
+
+ InternalWaitForSemaphore (Ctx->CpuSem[CpuIndex].Run);
+
+ return RETURN_SUCCESS;
+}
+
+/**
+ Used by the AP to release BSP.
+
+ The AP is specified by CpuIndex. The BSP is specified by BspIndex.
+
+ @param[in,out] SmmCpuSyncCtx Pointer to the SMM CPU Sync context object.
+ @param[in] CpuIndex Indicate which AP release BSP.
+ @param[in] BspIndex The BSP Index to be released.
+
+ @retval RETURN_SUCCESS AP to release BSP successfully.
+ @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx is NULL or CpuIndex is same as BspIndex.
+
+**/
+RETURN_STATUS
+EFIAPI
+SmmCpuSyncReleaseBsp (
+ IN OUT SMM_CPU_SYNC_CTX *SmmCpuSyncCtx,
+ IN UINTN CpuIndex,
+ IN UINTN BspIndex
+ )
+{
+ SMM_CPU_SYNC_CTX *Ctx;
+
+ if ((SmmCpuSyncCtx == NULL) || (BspIndex == CpuIndex)) {
+ return RETURN_INVALID_PARAMETER;
+ }
+
+ Ctx = (SMM_CPU_SYNC_CTX *)SmmCpuSyncCtx;
+
+ InternalReleaseSemaphore (Ctx->CpuSem[BspIndex].Run);
+
+ return RETURN_SUCCESS;
+}
diff --git a/UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.inf b/UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.inf
new file mode 100644
index 0000000000..6bb1895577
--- /dev/null
+++ b/UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.inf
@@ -0,0 +1,39 @@
+## @file
+# SMM CPU Synchronization lib.
+#
+# This is SMM CPU Synchronization lib used for SMM CPU sync operations.
+#
+# Copyright (c) 2023, Intel Corporation. All rights reserved.<BR>
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+ INF_VERSION = 0x00010005
+ BASE_NAME = SmmCpuSyncLib
+ FILE_GUID = 1ca1bc1a-16a4-46ef-956a-ca500fd3381f
+ MODULE_TYPE = DXE_SMM_DRIVER
+ LIBRARY_CLASS = SmmCpuSyncLib|DXE_SMM_DRIVER
+
+[Sources]
+ SmmCpuSyncLib.c
+
+[Packages]
+ MdePkg/MdePkg.dec
+ MdeModulePkg/MdeModulePkg.dec
+ UefiCpuPkg/UefiCpuPkg.dec
+
+[LibraryClasses]
+ UefiLib
+ BaseLib
+ DebugLib
+ PrintLib
+ SafeIntLib
+ SynchronizationLib
+ BaseMemoryLib
+ SmmServicesTableLib
+ MemoryAllocationLib
+
+[Pcd]
+
+[Protocols]
diff --git a/UefiCpuPkg/UefiCpuPkg.dsc b/UefiCpuPkg/UefiCpuPkg.dsc
index 074fd77461..f264031c77 100644
--- a/UefiCpuPkg/UefiCpuPkg.dsc
+++ b/UefiCpuPkg/UefiCpuPkg.dsc
@@ -23,10 +23,11 @@
#
!include MdePkg/MdeLibs.dsc.inc
[LibraryClasses]
+ SafeIntLib|MdePkg/Library/BaseSafeIntLib/BaseSafeIntLib.inf
BaseLib|MdePkg/Library/BaseLib/BaseLib.inf
BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
CpuLib|MdePkg/Library/BaseCpuLib/BaseCpuLib.inf
DebugLib|MdePkg/Library/BaseDebugLibNull/BaseDebugLibNull.inf
SerialPortLib|MdePkg/Library/BaseSerialPortLibNull/BaseSerialPortLibNull.inf
@@ -54,10 +55,11 @@
CacheMaintenanceLib|MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
PciLib|MdePkg/Library/BasePciLibPciExpress/BasePciLibPciExpress.inf
PciExpressLib|MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf
SmmCpuPlatformHookLib|UefiCpuPkg/Library/SmmCpuPlatformHookLibNull/SmmCpuPlatformHookLibNull.inf
SmmCpuFeaturesLib|UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
+ SmmCpuSyncLib|UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.inf
PeCoffGetEntryPointLib|MdePkg/Library/BasePeCoffGetEntryPointLib/BasePeCoffGetEntryPointLib.inf
PeCoffExtraActionLib|MdePkg/Library/BasePeCoffExtraActionLibNull/BasePeCoffExtraActionLibNull.inf
TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
CcExitLib|UefiCpuPkg/Library/CcExitLibNull/CcExitLibNull.inf
MicrocodeLib|UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
@@ -154,10 +156,11 @@
UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.inf
UefiCpuPkg/Library/SmmCpuPlatformHookLibNull/SmmCpuPlatformHookLibNull.inf
UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.inf
+ UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.inf
UefiCpuPkg/Library/CcExitLibNull/CcExitLibNull.inf
UefiCpuPkg/PiSmmCommunication/PiSmmCommunicationPei.inf
UefiCpuPkg/PiSmmCommunication/PiSmmCommunicationSmm.inf
UefiCpuPkg/SecCore/SecCore.inf
UefiCpuPkg/SecCore/SecCoreNative.inf
--
2.16.2.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111895): https://edk2.groups.io/g/devel/message/111895
Mute This Topic: https://groups.io/mt/102889293/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [edk2-devel] [PATCH v2 4/6] OvmfPkg: Specifies SmmCpuSyncLib instance
2023-11-30 6:31 [edk2-devel] [PATCH v2 0/6] Refine SMM CPU Sync flow and abstract SmmCpuSyncLib Wu, Jiaxin
` (2 preceding siblings ...)
2023-11-30 6:31 ` [edk2-devel] [PATCH v2 3/6] UefiCpuPkg: Implements SmmCpuSyncLib library instance Wu, Jiaxin
@ 2023-11-30 6:31 ` Wu, Jiaxin
2023-11-30 6:31 ` [edk2-devel] [PATCH v2 5/6] UefiPayloadPkg: " Wu, Jiaxin
2023-11-30 6:31 ` [edk2-devel] [PATCH v2 6/6] UefiCpuPkg/PiSmmCpuDxeSmm: Consume SmmCpuSyncLib Wu, Jiaxin
5 siblings, 0 replies; 11+ messages in thread
From: Wu, Jiaxin @ 2023-11-30 6:31 UTC (permalink / raw)
To: devel
Cc: Laszlo Ersek, Ard Biesheuvel, Jiewen Yao, Jordan Justen,
Eric Dong, Ray Ni, Zeng Star, Rahul Kumar, Gerd Hoffmann
This patch is to specify SmmCpuSyncLib instance for OvmfPkg.
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Zeng Star <star.zeng@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
---
OvmfPkg/CloudHv/CloudHvX64.dsc | 2 ++
OvmfPkg/OvmfPkgIa32.dsc | 2 ++
OvmfPkg/OvmfPkgIa32X64.dsc | 2 ++
OvmfPkg/OvmfPkgX64.dsc | 1 +
4 files changed, 7 insertions(+)
diff --git a/OvmfPkg/CloudHv/CloudHvX64.dsc b/OvmfPkg/CloudHv/CloudHvX64.dsc
index 821ad1b9fa..f735b69a37 100644
--- a/OvmfPkg/CloudHv/CloudHvX64.dsc
+++ b/OvmfPkg/CloudHv/CloudHvX64.dsc
@@ -183,10 +183,12 @@
PeiHardwareInfoLib|OvmfPkg/Library/HardwareInfoLib/PeiHardwareInfoLib.inf
DxeHardwareInfoLib|OvmfPkg/Library/HardwareInfoLib/DxeHardwareInfoLib.inf
ImagePropertiesRecordLib|MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.inf
!if $(SMM_REQUIRE) == FALSE
LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
+!else
+ SmmCpuSyncLib|UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.inf
!endif
CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/CustomizedDisplayLib.inf
FrameBufferBltLib|MdeModulePkg/Library/FrameBufferBltLib/FrameBufferBltLib.inf
MemEncryptTdxLib|OvmfPkg/Library/BaseMemEncryptTdxLib/BaseMemEncryptTdxLib.inf
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index bce2aedcd7..b05b13b18c 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -188,10 +188,12 @@
PeiHardwareInfoLib|OvmfPkg/Library/HardwareInfoLib/PeiHardwareInfoLib.inf
DxeHardwareInfoLib|OvmfPkg/Library/HardwareInfoLib/DxeHardwareInfoLib.inf
ImagePropertiesRecordLib|MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.inf
!if $(SMM_REQUIRE) == FALSE
LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
+!else
+ SmmCpuSyncLib|UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.inf
!endif
CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/CustomizedDisplayLib.inf
FrameBufferBltLib|MdeModulePkg/Library/FrameBufferBltLib/FrameBufferBltLib.inf
!if $(SOURCE_DEBUG_ENABLE) == TRUE
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 631e909a54..5a16eb7abe 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -193,10 +193,12 @@
PeiHardwareInfoLib|OvmfPkg/Library/HardwareInfoLib/PeiHardwareInfoLib.inf
DxeHardwareInfoLib|OvmfPkg/Library/HardwareInfoLib/DxeHardwareInfoLib.inf
ImagePropertiesRecordLib|MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.inf
!if $(SMM_REQUIRE) == FALSE
LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
+!else
+ SmmCpuSyncLib|UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.inf
!endif
CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/CustomizedDisplayLib.inf
FrameBufferBltLib|MdeModulePkg/Library/FrameBufferBltLib/FrameBufferBltLib.inf
!if $(SOURCE_DEBUG_ENABLE) == TRUE
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 4ea3008cc6..6bb4c777b9 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -209,10 +209,11 @@
!if $(SMM_REQUIRE) == FALSE
LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
CcProbeLib|OvmfPkg/Library/CcProbeLib/DxeCcProbeLib.inf
!else
CcProbeLib|MdePkg/Library/CcProbeLibNull/CcProbeLibNull.inf
+ SmmCpuSyncLib|UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.inf
!endif
CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/CustomizedDisplayLib.inf
FrameBufferBltLib|MdeModulePkg/Library/FrameBufferBltLib/FrameBufferBltLib.inf
!if $(SOURCE_DEBUG_ENABLE) == TRUE
--
2.16.2.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111896): https://edk2.groups.io/g/devel/message/111896
Mute This Topic: https://groups.io/mt/102889294/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [edk2-devel] [PATCH v2 5/6] UefiPayloadPkg: Specifies SmmCpuSyncLib instance
2023-11-30 6:31 [edk2-devel] [PATCH v2 0/6] Refine SMM CPU Sync flow and abstract SmmCpuSyncLib Wu, Jiaxin
` (3 preceding siblings ...)
2023-11-30 6:31 ` [edk2-devel] [PATCH v2 4/6] OvmfPkg: Specifies SmmCpuSyncLib instance Wu, Jiaxin
@ 2023-11-30 6:31 ` Wu, Jiaxin
2023-12-06 0:09 ` Guo, Gua
2023-11-30 6:31 ` [edk2-devel] [PATCH v2 6/6] UefiCpuPkg/PiSmmCpuDxeSmm: Consume SmmCpuSyncLib Wu, Jiaxin
5 siblings, 1 reply; 11+ messages in thread
From: Wu, Jiaxin @ 2023-11-30 6:31 UTC (permalink / raw)
To: devel
Cc: Laszlo Ersek, Guo Dong, Sean Rhodes, James Lu, Gua Guo, Ray Ni,
Zeng Star
This patch is to specify SmmCpuSyncLib instance for UefiPayloadPkg.
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Guo Dong <guo.dong@intel.com>
Cc: Sean Rhodes <sean@starlabs.systems>
Cc: James Lu <james.lu@intel.com>
Cc: Gua Guo <gua.guo@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Zeng Star <star.zeng@intel.com>
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
---
UefiPayloadPkg/UefiPayloadPkg.dsc | 1 +
1 file changed, 1 insertion(+)
diff --git a/UefiPayloadPkg/UefiPayloadPkg.dsc b/UefiPayloadPkg/UefiPayloadPkg.dsc
index a65f9d5b83..b8b13ad201 100644
--- a/UefiPayloadPkg/UefiPayloadPkg.dsc
+++ b/UefiPayloadPkg/UefiPayloadPkg.dsc
@@ -253,10 +253,11 @@
#
MtrrLib|UefiCpuPkg/Library/MtrrLib/MtrrLib.inf
LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
MicrocodeLib|UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
CpuPageTableLib|UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableLib.inf
+ SmmCpuSyncLib|UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.inf
#
# Platform
#
!if $(CPU_TIMER_LIB_ENABLE) == TRUE && $(UNIVERSAL_PAYLOAD) == TRUE
--
2.16.2.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111897): https://edk2.groups.io/g/devel/message/111897
Mute This Topic: https://groups.io/mt/102889295/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [edk2-devel] [PATCH v2 6/6] UefiCpuPkg/PiSmmCpuDxeSmm: Consume SmmCpuSyncLib
2023-11-30 6:31 [edk2-devel] [PATCH v2 0/6] Refine SMM CPU Sync flow and abstract SmmCpuSyncLib Wu, Jiaxin
` (4 preceding siblings ...)
2023-11-30 6:31 ` [edk2-devel] [PATCH v2 5/6] UefiPayloadPkg: " Wu, Jiaxin
@ 2023-11-30 6:31 ` Wu, Jiaxin
5 siblings, 0 replies; 11+ messages in thread
From: Wu, Jiaxin @ 2023-11-30 6:31 UTC (permalink / raw)
To: devel
Cc: Laszlo Ersek, Eric Dong, Ray Ni, Zeng Star, Gerd Hoffmann,
Rahul Kumar
There is the SmmCpuSyncLib Library class define the SMM CPU sync
flow, which is aligned with existing SMM CPU driver sync behavior.
This patch is to consume SmmCpuSyncLib instance directly.
With this change, SMM CPU Sync flow/logic can be customized
with different implementation no matter for any purpose, e.g.
performance tuning, handle specific register, etc.
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Zeng Star <star.zeng@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
---
UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 317 +++++++++------------------
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 6 +-
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 1 +
3 files changed, 110 insertions(+), 214 deletions(-)
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index 54542262a2..e37c03d0e5 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -27,122 +27,10 @@ MM_COMPLETION mSmmStartupThisApToken;
//
// Processor specified by mPackageFirstThreadIndex[PackageIndex] will do the package-scope register check.
//
UINT32 *mPackageFirstThreadIndex = NULL;
-/**
- Performs an atomic compare exchange operation to get semaphore.
- The compare exchange operation must be performed using
- MP safe mechanisms.
-
- @param Sem IN: 32-bit unsigned integer
- OUT: original integer - 1
- @return Original integer - 1
-
-**/
-UINT32
-WaitForSemaphore (
- IN OUT volatile UINT32 *Sem
- )
-{
- UINT32 Value;
-
- for ( ; ;) {
- Value = *Sem;
- if ((Value != 0) &&
- (InterlockedCompareExchange32 (
- (UINT32 *)Sem,
- Value,
- Value - 1
- ) == Value))
- {
- break;
- }
-
- CpuPause ();
- }
-
- return Value - 1;
-}
-
-/**
- Performs an atomic compare exchange operation to release semaphore.
- The compare exchange operation must be performed using
- MP safe mechanisms.
-
- @param Sem IN: 32-bit unsigned integer
- OUT: original integer + 1
- @return Original integer + 1
-
-**/
-UINT32
-ReleaseSemaphore (
- IN OUT volatile UINT32 *Sem
- )
-{
- UINT32 Value;
-
- do {
- Value = *Sem;
- } while (Value + 1 != 0 &&
- InterlockedCompareExchange32 (
- (UINT32 *)Sem,
- Value,
- Value + 1
- ) != Value);
-
- return Value + 1;
-}
-
-/**
- Performs an atomic compare exchange operation to lock semaphore.
- The compare exchange operation must be performed using
- MP safe mechanisms.
-
- @param Sem IN: 32-bit unsigned integer
- OUT: -1
- @return Original integer
-
-**/
-UINT32
-LockdownSemaphore (
- IN OUT volatile UINT32 *Sem
- )
-{
- UINT32 Value;
-
- do {
- Value = *Sem;
- } while (InterlockedCompareExchange32 (
- (UINT32 *)Sem,
- Value,
- (UINT32)-1
- ) != Value);
-
- return Value;
-}
-
-/**
- Used for BSP to wait all APs.
- Wait all APs to performs an atomic compare exchange operation to release semaphore.
-
- @param NumberOfAPs AP number
-
-**/
-VOID
-WaitForAllAPs (
- IN UINTN NumberOfAPs
- )
-{
- UINTN BspIndex;
-
- BspIndex = mSmmMpSyncData->BspIndex;
- while (NumberOfAPs-- > 0) {
- WaitForSemaphore (mSmmMpSyncData->CpuData[BspIndex].Run);
- }
-}
-
/**
Used for BSP to release all APs.
Performs an atomic compare exchange operation to release semaphore
for each AP.
@@ -154,57 +42,15 @@ ReleaseAllAPs (
{
UINTN Index;
for (Index = 0; Index < mMaxNumberOfCpus; Index++) {
if (IsPresentAp (Index)) {
- ReleaseSemaphore (mSmmMpSyncData->CpuData[Index].Run);
+ SmmCpuSyncReleaseOneAp (mSmmMpSyncData->SmmCpuSyncCtx, Index, gSmmCpuPrivate->SmmCoreEntryContext.CurrentlyExecutingCpu);
}
}
}
-/**
- Used for BSP to release one AP.
-
- @param ApSem IN: 32-bit unsigned integer
- OUT: original integer + 1
-**/
-VOID
-ReleaseOneAp (
- IN OUT volatile UINT32 *ApSem
- )
-{
- ReleaseSemaphore (ApSem);
-}
-
-/**
- Used for AP to wait BSP.
-
- @param ApSem IN: 32-bit unsigned integer
- OUT: original integer - 1
-**/
-VOID
-WaitForBsp (
- IN OUT volatile UINT32 *ApSem
- )
-{
- WaitForSemaphore (ApSem);
-}
-
-/**
- Used for AP to release BSP.
-
- @param BspSem IN: 32-bit unsigned integer
- OUT: original integer + 1
-**/
-VOID
-ReleaseBsp (
- IN OUT volatile UINT32 *BspSem
- )
-{
- ReleaseSemaphore (BspSem);
-}
-
/**
Check whether the index of CPU perform the package level register
programming during System Management Mode initialization.
The index of Processor specified by mPackageFirstThreadIndex[PackageIndex]
@@ -285,42 +131,53 @@ GetSmmDelayedBlockedDisabledCount (
BOOLEAN
AllCpusInSmmExceptBlockedDisabled (
VOID
)
{
+ RETURN_STATUS Status;
+
+ UINTN CpuCount;
UINT32 BlockedCount;
UINT32 DisabledCount;
+ CpuCount = 0;
BlockedCount = 0;
DisabledCount = 0;
+ Status = SmmCpuSyncGetArrivedCpuCount (mSmmMpSyncData->SmmCpuSyncCtx, &CpuCount);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "AllCpusInSmmExceptBlockedDisabled: SmmCpuSyncGetArrivedCpuCount return error %r!\n", Status));
+ CpuDeadLoop ();
+ return FALSE;
+ }
+
//
- // Check to make sure mSmmMpSyncData->Counter is valid and not locked.
+ // Check to make sure the CPU arrival count is valid and not locked.
//
- ASSERT (*mSmmMpSyncData->Counter <= mNumberOfCpus);
+ ASSERT (CpuCount <= mNumberOfCpus);
//
// Check whether all CPUs in SMM.
//
- if (*mSmmMpSyncData->Counter == mNumberOfCpus) {
+ if (CpuCount == mNumberOfCpus) {
return TRUE;
}
//
// Check for the Blocked & Disabled Exceptions Case.
//
GetSmmDelayedBlockedDisabledCount (NULL, &BlockedCount, &DisabledCount);
//
- // *mSmmMpSyncData->Counter might be updated by all APs concurrently. The value
+ // The CPU arrival count might be updated by all APs concurrently. The value
// can be dynamic changed. If some Aps enter the SMI after the BlockedCount &
- // DisabledCount check, then the *mSmmMpSyncData->Counter will be increased, thus
- // leading the *mSmmMpSyncData->Counter + BlockedCount + DisabledCount > mNumberOfCpus.
+ // DisabledCount check, then the CPU arrival count will be increased, thus
+ // leading the retrieved CPU arrival count + BlockedCount + DisabledCount > mNumberOfCpus.
// since the BlockedCount & DisabledCount are local variable, it's ok here only for
// the checking of all CPUs In Smm.
//
- if (*mSmmMpSyncData->Counter + BlockedCount + DisabledCount >= mNumberOfCpus) {
+ if (CpuCount + BlockedCount + DisabledCount >= mNumberOfCpus) {
return TRUE;
}
return FALSE;
}
@@ -384,23 +241,35 @@ IsLmceSignaled (
VOID
SmmWaitForApArrival (
VOID
)
{
+ RETURN_STATUS Status;
+
+ UINTN CpuCount;
UINT64 Timer;
UINTN Index;
BOOLEAN LmceEn;
BOOLEAN LmceSignal;
UINT32 DelayedCount;
UINT32 BlockedCount;
PERF_FUNCTION_BEGIN ();
+ CpuCount = 0;
DelayedCount = 0;
BlockedCount = 0;
- ASSERT (*mSmmMpSyncData->Counter <= mNumberOfCpus);
+ Status = SmmCpuSyncGetArrivedCpuCount (mSmmMpSyncData->SmmCpuSyncCtx, &CpuCount);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "SmmWaitForApArrival: SmmCpuSyncGetArrivedCpuCount return error %r!\n", Status));
+ CpuDeadLoop ();
+ PERF_FUNCTION_END ();
+ return;
+ }
+
+ ASSERT (CpuCount <= mNumberOfCpus);
LmceEn = FALSE;
LmceSignal = FALSE;
if (mMachineCheckSupported) {
LmceEn = IsLmceOsEnabled ();
@@ -431,10 +300,21 @@ SmmWaitForApArrival (
}
CpuPause ();
}
+ //
+ // Check the CpuCount after Sync with APs 1st.
+ //
+ Status = SmmCpuSyncGetArrivedCpuCount (mSmmMpSyncData->SmmCpuSyncCtx, &CpuCount);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "SmmWaitForApArrival: SmmCpuSyncGetArrivedCpuCount return error %r!\n", Status));
+ CpuDeadLoop ();
+ PERF_FUNCTION_END ();
+ return;
+ }
+
//
// Not all APs have arrived, so we need 2nd round of timeout. IPIs should be sent to ALL none present APs,
// because:
// a) Delayed AP may have just come out of the delayed state. Blocked AP may have just been brought out of blocked state by some AP running
// normal mode code. These APs need to be guaranteed to have an SMI pending to insure that once they are out of delayed / blocked state, they
@@ -447,11 +327,11 @@ SmmWaitForApArrival (
// d) We don't add code to check SMI disabling status to skip sending IPI to SMI disabled APs, because:
// - In traditional flow, SMI disabling is discouraged.
// - In relaxed flow, CheckApArrival() will check SMI disabling status before calling this function.
// In both cases, adding SMI-disabling checking code increases overhead.
//
- if (*mSmmMpSyncData->Counter < mNumberOfCpus) {
+ if (CpuCount < mNumberOfCpus) {
//
// Send SMI IPIs to bring outside processors in
//
for (Index = 0; Index < mMaxNumberOfCpus; Index++) {
if (!(*(mSmmMpSyncData->CpuData[Index].Present)) && (gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId != INVALID_APIC_ID)) {
@@ -610,18 +490,22 @@ VOID
BSPHandler (
IN UINTN CpuIndex,
IN SMM_CPU_SYNC_MODE SyncMode
)
{
+ RETURN_STATUS Status;
+
+ UINTN CpuCount;
UINTN Index;
MTRR_SETTINGS Mtrrs;
UINTN ApCount;
BOOLEAN ClearTopLevelSmiResult;
UINTN PresentCount;
ASSERT (CpuIndex == mSmmMpSyncData->BspIndex);
- ApCount = 0;
+ CpuCount = 0;
+ ApCount = 0;
PERF_FUNCTION_BEGIN ();
//
// Flag BSP's presence
@@ -659,28 +543,35 @@ BSPHandler (
// Wait for APs to arrive
//
SmmWaitForApArrival ();
//
- // Lock the counter down and retrieve the number of APs
+ // Lock door for late comming CPU checkin and retrieve the Arrived number of APs
//
*mSmmMpSyncData->AllCpusInSync = TRUE;
- ApCount = LockdownSemaphore (mSmmMpSyncData->Counter) - 1;
+
+ Status = SmmCpuSyncLockDoor (mSmmMpSyncData->SmmCpuSyncCtx, CpuIndex, &CpuCount);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "BSPHandler: SmmCpuSyncLockDoor return error %r!\n", Status));
+ CpuDeadLoop ();
+ }
+
+ ApCount = CpuCount - 1;
//
// Wait for all APs to get ready for programming MTRRs
//
- WaitForAllAPs (ApCount);
+ SmmCpuSyncWaitForAPs (mSmmMpSyncData->SmmCpuSyncCtx, ApCount, CpuIndex);
if (SmmCpuFeaturesNeedConfigureMtrrs ()) {
//
// Signal all APs it's time for backup MTRRs
//
ReleaseAllAPs ();
//
- // WaitForAllAPs() may wait for ever if an AP happens to enter SMM at
+ // SmmCpuSyncWaitForAPs() may wait for ever if an AP happens to enter SMM at
// exactly this point. Please make sure PcdCpuSmmMaxSyncLoops has been set
// to a large enough value to avoid this situation.
// Note: For HT capable CPUs, threads within a core share the same set of MTRRs.
// We do the backup first and then set MTRR to avoid race condition for threads
// in the same core.
@@ -688,28 +579,28 @@ BSPHandler (
MtrrGetAllMtrrs (&Mtrrs);
//
// Wait for all APs to complete their MTRR saving
//
- WaitForAllAPs (ApCount);
+ SmmCpuSyncWaitForAPs (mSmmMpSyncData->SmmCpuSyncCtx, ApCount, CpuIndex);
//
// Let all processors program SMM MTRRs together
//
ReleaseAllAPs ();
//
- // WaitForAllAPs() may wait for ever if an AP happens to enter SMM at
+ // SmmCpuSyncWaitForAPs() may wait for ever if an AP happens to enter SMM at
// exactly this point. Please make sure PcdCpuSmmMaxSyncLoops has been set
// to a large enough value to avoid this situation.
//
ReplaceOSMtrrs (CpuIndex);
//
// Wait for all APs to complete their MTRR programming
//
- WaitForAllAPs (ApCount);
+ SmmCpuSyncWaitForAPs (mSmmMpSyncData->SmmCpuSyncCtx, ApCount, CpuIndex);
}
}
//
// The BUSY lock is initialized to Acquired state
@@ -741,14 +632,21 @@ BSPHandler (
// make those APs to exit SMI synchronously. APs which arrive later will be excluded and
// will run through freely.
//
if ((SyncMode != SmmCpuSyncModeTradition) && !SmmCpuFeaturesNeedConfigureMtrrs ()) {
//
- // Lock the counter down and retrieve the number of APs
+ // Lock door for late comming CPU checkin and retrieve the Arrived number of APs
//
*mSmmMpSyncData->AllCpusInSync = TRUE;
- ApCount = LockdownSemaphore (mSmmMpSyncData->Counter) - 1;
+ Status = SmmCpuSyncLockDoor (mSmmMpSyncData->SmmCpuSyncCtx, CpuIndex, &CpuCount);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "BSPHandler: SmmCpuSyncLockDoor return error %r!\n", Status));
+ CpuDeadLoop ();
+ }
+
+ ApCount = CpuCount - 1;
+
//
// Make sure all APs have their Present flag set
//
while (TRUE) {
PresentCount = 0;
@@ -771,11 +669,11 @@ BSPHandler (
ReleaseAllAPs ();
//
// Wait for all APs to complete their pending tasks
//
- WaitForAllAPs (ApCount);
+ SmmCpuSyncWaitForAPs (mSmmMpSyncData->SmmCpuSyncCtx, ApCount, CpuIndex);
if (SmmCpuFeaturesNeedConfigureMtrrs ()) {
//
// Signal APs to restore MTRRs
//
@@ -788,11 +686,11 @@ BSPHandler (
MtrrSetAllMtrrs (&Mtrrs);
//
// Wait for all APs to complete MTRR programming
//
- WaitForAllAPs (ApCount);
+ SmmCpuSyncWaitForAPs (mSmmMpSyncData->SmmCpuSyncCtx, ApCount, CpuIndex);
}
//
// Stop source level debug in BSP handler, the code below will not be
// debugged.
@@ -816,11 +714,11 @@ BSPHandler (
//
// Gather APs to exit SMM synchronously. Note the Present flag is cleared by now but
// WaitForAllAps does not depend on the Present flag.
//
- WaitForAllAPs (ApCount);
+ SmmCpuSyncWaitForAPs (mSmmMpSyncData->SmmCpuSyncCtx, ApCount, CpuIndex);
//
// At this point, all APs should have exited from APHandler().
// Migrate the SMM MP performance logging to standard SMM performance logging.
// Any SMM MP performance logging after this point will be migrated in next SMI.
@@ -842,11 +740,11 @@ BSPHandler (
}
//
// Allow APs to check in from this point on
//
- *mSmmMpSyncData->Counter = 0;
+ SmmCpuSyncContextReset (mSmmMpSyncData->SmmCpuSyncCtx);
*mSmmMpSyncData->AllCpusInSync = FALSE;
mSmmMpSyncData->AllApArrivedWithException = FALSE;
PERF_FUNCTION_END ();
}
@@ -912,21 +810,21 @@ APHandler (
if (!(*mSmmMpSyncData->InsideSmm)) {
//
// Give up since BSP is unable to enter SMM
// and signal the completion of this AP
- // Reduce the mSmmMpSyncData->Counter!
+ // Reduce the CPU arrival count!
//
- WaitForSemaphore (mSmmMpSyncData->Counter);
+ SmmCpuSyncCheckOutCpu (mSmmMpSyncData->SmmCpuSyncCtx, CpuIndex);
return;
}
} else {
//
// Don't know BSP index. Give up without sending IPI to BSP.
- // Reduce the mSmmMpSyncData->Counter!
+ // Reduce the CPU arrival count!
//
- WaitForSemaphore (mSmmMpSyncData->Counter);
+ SmmCpuSyncCheckOutCpu (mSmmMpSyncData->SmmCpuSyncCtx, CpuIndex);
return;
}
}
//
@@ -942,50 +840,50 @@ APHandler (
if ((SyncMode == SmmCpuSyncModeTradition) || SmmCpuFeaturesNeedConfigureMtrrs ()) {
//
// Notify BSP of arrival at this point
//
- ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run);
+ SmmCpuSyncReleaseBsp (mSmmMpSyncData->SmmCpuSyncCtx, CpuIndex, BspIndex);
}
if (SmmCpuFeaturesNeedConfigureMtrrs ()) {
//
// Wait for the signal from BSP to backup MTRRs
//
- WaitForBsp (mSmmMpSyncData->CpuData[CpuIndex].Run);
+ SmmCpuSyncWaitForBsp (mSmmMpSyncData->SmmCpuSyncCtx, CpuIndex, BspIndex);
//
// Backup OS MTRRs
//
MtrrGetAllMtrrs (&Mtrrs);
//
// Signal BSP the completion of this AP
//
- ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run);
+ SmmCpuSyncReleaseBsp (mSmmMpSyncData->SmmCpuSyncCtx, CpuIndex, BspIndex);
//
// Wait for BSP's signal to program MTRRs
//
- WaitForBsp (mSmmMpSyncData->CpuData[CpuIndex].Run);
+ SmmCpuSyncWaitForBsp (mSmmMpSyncData->SmmCpuSyncCtx, CpuIndex, BspIndex);
//
// Replace OS MTRRs with SMI MTRRs
//
ReplaceOSMtrrs (CpuIndex);
//
// Signal BSP the completion of this AP
//
- ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run);
+ SmmCpuSyncReleaseBsp (mSmmMpSyncData->SmmCpuSyncCtx, CpuIndex, BspIndex);
}
while (TRUE) {
//
// Wait for something to happen
//
- WaitForBsp (mSmmMpSyncData->CpuData[CpuIndex].Run);
+ SmmCpuSyncWaitForBsp (mSmmMpSyncData->SmmCpuSyncCtx, CpuIndex, BspIndex);
//
// Check if BSP wants to exit SMM
//
if (!(*mSmmMpSyncData->InsideSmm)) {
@@ -1021,16 +919,16 @@ APHandler (
if (SmmCpuFeaturesNeedConfigureMtrrs ()) {
//
// Notify BSP the readiness of this AP to program MTRRs
//
- ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run);
+ SmmCpuSyncReleaseBsp (mSmmMpSyncData->SmmCpuSyncCtx, CpuIndex, BspIndex);
//
// Wait for the signal from BSP to program MTRRs
//
- WaitForBsp (mSmmMpSyncData->CpuData[CpuIndex].Run);
+ SmmCpuSyncWaitForBsp (mSmmMpSyncData->SmmCpuSyncCtx, CpuIndex, BspIndex);
//
// Restore OS MTRRs
//
SmmCpuFeaturesReenableSmrr ();
@@ -1038,26 +936,26 @@ APHandler (
}
//
// Notify BSP the readiness of this AP to Reset states/semaphore for this processor
//
- ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run);
+ SmmCpuSyncReleaseBsp (mSmmMpSyncData->SmmCpuSyncCtx, CpuIndex, BspIndex);
//
// Wait for the signal from BSP to Reset states/semaphore for this processor
//
- WaitForBsp (mSmmMpSyncData->CpuData[CpuIndex].Run);
+ SmmCpuSyncWaitForBsp (mSmmMpSyncData->SmmCpuSyncCtx, CpuIndex, BspIndex);
//
// Reset states/semaphore for this processor
//
*(mSmmMpSyncData->CpuData[CpuIndex].Present) = FALSE;
//
// Notify BSP the readiness of this AP to exit SMM
//
- ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run);
+ SmmCpuSyncReleaseBsp (mSmmMpSyncData->SmmCpuSyncCtx, CpuIndex, BspIndex);
}
/**
Checks whether the input token is the current used token.
@@ -1321,11 +1219,11 @@ InternalSmmStartupThisAp (
mSmmMpSyncData->CpuData[CpuIndex].Status = CpuStatus;
if (mSmmMpSyncData->CpuData[CpuIndex].Status != NULL) {
*mSmmMpSyncData->CpuData[CpuIndex].Status = EFI_NOT_READY;
}
- ReleaseOneAp (mSmmMpSyncData->CpuData[CpuIndex].Run);
+ SmmCpuSyncReleaseOneAp (mSmmMpSyncData->SmmCpuSyncCtx, CpuIndex, gSmmCpuPrivate->SmmCoreEntryContext.CurrentlyExecutingCpu);
if (Token == NULL) {
AcquireSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Busy);
ReleaseSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Busy);
}
@@ -1450,11 +1348,11 @@ InternalSmmStartupAllAPs (
//
// Decrease the count to mark this processor(AP or BSP) as finished.
//
if (ProcToken != NULL) {
- WaitForSemaphore (&ProcToken->RunningApCount);
+ InterlockedDecrement (&ProcToken->RunningApCount);
}
}
}
ReleaseAllAPs ();
@@ -1725,14 +1623,15 @@ SmiRendezvous (
//
goto Exit;
} else {
//
// Signal presence of this processor
- // mSmmMpSyncData->Counter is increased here!
- // "ReleaseSemaphore (mSmmMpSyncData->Counter) == 0" means BSP has already ended the synchronization.
+ // CPU check in here!
+ // "SmmCpuSyncCheckInCpu (mSmmMpSyncData->SmmCpuSyncCtx, CpuIndex)" return error means failed
+ // to check in CPU. BSP has already ended the synchronization.
//
- if (ReleaseSemaphore (mSmmMpSyncData->Counter) == 0) {
+ if (RETURN_ERROR (SmmCpuSyncCheckInCpu (mSmmMpSyncData->SmmCpuSyncCtx, CpuIndex))) {
//
// BSP has already ended the synchronization, so QUIT!!!
// Existing AP is too late now to enter SMI since BSP has already ended the synchronization!!!
//
@@ -1824,12 +1723,10 @@ SmiRendezvous (
} else {
APHandler (CpuIndex, ValidSmi, mSmmMpSyncData->EffectiveSyncMode);
}
}
- ASSERT (*mSmmMpSyncData->CpuData[CpuIndex].Run == 0);
-
//
// Wait for BSP's signal to exit SMI
//
while (*mSmmMpSyncData->AllCpusInSync) {
CpuPause ();
@@ -1945,12 +1842,10 @@ InitializeSmmCpuSemaphores (
SemaphoreBlock = AllocatePages (Pages);
ASSERT (SemaphoreBlock != NULL);
ZeroMem (SemaphoreBlock, TotalSize);
SemaphoreAddr = (UINTN)SemaphoreBlock;
- mSmmCpuSemaphores.SemaphoreGlobal.Counter = (UINT32 *)SemaphoreAddr;
- SemaphoreAddr += SemaphoreSize;
mSmmCpuSemaphores.SemaphoreGlobal.InsideSmm = (BOOLEAN *)SemaphoreAddr;
SemaphoreAddr += SemaphoreSize;
mSmmCpuSemaphores.SemaphoreGlobal.AllCpusInSync = (BOOLEAN *)SemaphoreAddr;
SemaphoreAddr += SemaphoreSize;
mSmmCpuSemaphores.SemaphoreGlobal.PFLock = (SPIN_LOCK *)SemaphoreAddr;
@@ -1960,12 +1855,10 @@ InitializeSmmCpuSemaphores (
SemaphoreAddr += SemaphoreSize;
SemaphoreAddr = (UINTN)SemaphoreBlock + GlobalSemaphoresSize;
mSmmCpuSemaphores.SemaphoreCpu.Busy = (SPIN_LOCK *)SemaphoreAddr;
SemaphoreAddr += ProcessorCount * SemaphoreSize;
- mSmmCpuSemaphores.SemaphoreCpu.Run = (UINT32 *)SemaphoreAddr;
- SemaphoreAddr += ProcessorCount * SemaphoreSize;
mSmmCpuSemaphores.SemaphoreCpu.Present = (BOOLEAN *)SemaphoreAddr;
mPFLock = mSmmCpuSemaphores.SemaphoreGlobal.PFLock;
mConfigSmmCodeAccessCheckLock = mSmmCpuSemaphores.SemaphoreGlobal.CodeAccessCheckLock;
@@ -1980,10 +1873,12 @@ VOID
EFIAPI
InitializeMpSyncData (
VOID
)
{
+ RETURN_STATUS Status;
+
UINTN CpuIndex;
if (mSmmMpSyncData != NULL) {
//
// mSmmMpSyncDataSize includes one structure of SMM_DISPATCHER_MP_SYNC_DATA, one
@@ -2009,32 +1904,34 @@ InitializeMpSyncData (
}
}
mSmmMpSyncData->EffectiveSyncMode = mCpuSmmSyncMode;
- mSmmMpSyncData->Counter = mSmmCpuSemaphores.SemaphoreGlobal.Counter;
+ Status = SmmCpuSyncContextInit (gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus, &(mSmmMpSyncData->SmmCpuSyncCtx));
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "InitializeMpSyncData: SmmCpuSyncContextInit return error %r!\n", Status));
+ CpuDeadLoop ();
+ return;
+ }
+
mSmmMpSyncData->InsideSmm = mSmmCpuSemaphores.SemaphoreGlobal.InsideSmm;
mSmmMpSyncData->AllCpusInSync = mSmmCpuSemaphores.SemaphoreGlobal.AllCpusInSync;
ASSERT (
- mSmmMpSyncData->Counter != NULL && mSmmMpSyncData->InsideSmm != NULL &&
+ mSmmMpSyncData->SmmCpuSyncCtx != NULL && mSmmMpSyncData->InsideSmm != NULL &&
mSmmMpSyncData->AllCpusInSync != NULL
);
- *mSmmMpSyncData->Counter = 0;
*mSmmMpSyncData->InsideSmm = FALSE;
*mSmmMpSyncData->AllCpusInSync = FALSE;
mSmmMpSyncData->AllApArrivedWithException = FALSE;
for (CpuIndex = 0; CpuIndex < gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus; CpuIndex++) {
mSmmMpSyncData->CpuData[CpuIndex].Busy =
(SPIN_LOCK *)((UINTN)mSmmCpuSemaphores.SemaphoreCpu.Busy + mSemaphoreSize * CpuIndex);
- mSmmMpSyncData->CpuData[CpuIndex].Run =
- (UINT32 *)((UINTN)mSmmCpuSemaphores.SemaphoreCpu.Run + mSemaphoreSize * CpuIndex);
mSmmMpSyncData->CpuData[CpuIndex].Present =
(BOOLEAN *)((UINTN)mSmmCpuSemaphores.SemaphoreCpu.Present + mSemaphoreSize * CpuIndex);
*(mSmmMpSyncData->CpuData[CpuIndex].Busy) = 0;
- *(mSmmMpSyncData->CpuData[CpuIndex].Run) = 0;
*(mSmmMpSyncData->CpuData[CpuIndex].Present) = FALSE;
}
}
}
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index 20ada465c2..607d5d8b74 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -52,10 +52,11 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Library/PeCoffGetEntryPointLib.h>
#include <Library/RegisterCpuFeaturesLib.h>
#include <Library/PerformanceLib.h>
#include <Library/CpuPageTableLib.h>
#include <Library/MmSaveStateLib.h>
+#include <Library/SmmCpuSyncLib.h>
#include <AcpiCpuData.h>
#include <CpuHotPlugData.h>
#include <Register/Intel/Cpuid.h>
@@ -403,11 +404,10 @@ SmmRelocationSemaphoreComplete (
///
typedef struct {
SPIN_LOCK *Busy;
volatile EFI_AP_PROCEDURE2 Procedure;
volatile VOID *Parameter;
- volatile UINT32 *Run;
volatile BOOLEAN *Present;
PROCEDURE_TOKEN *Token;
EFI_STATUS *Status;
} SMM_CPU_DATA_BLOCK;
@@ -421,29 +421,28 @@ typedef struct {
//
// Pointer to an array. The array should be located immediately after this structure
// so that UC cache-ability can be set together.
//
SMM_CPU_DATA_BLOCK *CpuData;
- volatile UINT32 *Counter;
volatile UINT32 BspIndex;
volatile BOOLEAN *InsideSmm;
volatile BOOLEAN *AllCpusInSync;
volatile SMM_CPU_SYNC_MODE EffectiveSyncMode;
volatile BOOLEAN SwitchBsp;
volatile BOOLEAN *CandidateBsp;
volatile BOOLEAN AllApArrivedWithException;
EFI_AP_PROCEDURE StartupProcedure;
VOID *StartupProcArgs;
+ SMM_CPU_SYNC_CTX *SmmCpuSyncCtx;
} SMM_DISPATCHER_MP_SYNC_DATA;
#define SMM_PSD_OFFSET 0xfb00
///
/// All global semaphores' pointer
///
typedef struct {
- volatile UINT32 *Counter;
volatile BOOLEAN *InsideSmm;
volatile BOOLEAN *AllCpusInSync;
SPIN_LOCK *PFLock;
SPIN_LOCK *CodeAccessCheckLock;
} SMM_CPU_SEMAPHORE_GLOBAL;
@@ -451,11 +450,10 @@ typedef struct {
///
/// All semaphores for each processor
///
typedef struct {
SPIN_LOCK *Busy;
- volatile UINT32 *Run;
volatile BOOLEAN *Present;
SPIN_LOCK *Token;
} SMM_CPU_SEMAPHORE_CPU;
///
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
index 5d52ed7d13..e92b8c747d 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
@@ -101,10 +101,11 @@
SmmCpuFeaturesLib
PeCoffGetEntryPointLib
PerformanceLib
CpuPageTableLib
MmSaveStateLib
+ SmmCpuSyncLib
[Protocols]
gEfiSmmAccess2ProtocolGuid ## CONSUMES
gEfiMpServiceProtocolGuid ## CONSUMES
gEfiSmmConfigurationProtocolGuid ## PRODUCES
--
2.16.2.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111898): https://edk2.groups.io/g/devel/message/111898
Mute This Topic: https://groups.io/mt/102889296/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH v2 5/6] UefiPayloadPkg: Specifies SmmCpuSyncLib instance
2023-11-30 6:31 ` [edk2-devel] [PATCH v2 5/6] UefiPayloadPkg: " Wu, Jiaxin
@ 2023-12-06 0:09 ` Guo, Gua
0 siblings, 0 replies; 11+ messages in thread
From: Guo, Gua @ 2023-12-06 0:09 UTC (permalink / raw)
To: Wu, Jiaxin, devel@edk2.groups.io
Cc: Laszlo Ersek, Dong, Guo, Rhodes, Sean, Lu, James, Ni, Ray,
Zeng, Star
Reviewed-by: Gua Guo <gua.guo@intel.com>
-----Original Message-----
From: Wu, Jiaxin <jiaxin.wu@intel.com>
Sent: Thursday, November 30, 2023 2:32 PM
To: devel@edk2.groups.io
Cc: Laszlo Ersek <lersek@redhat.com>; Dong, Guo <guo.dong@intel.com>; Rhodes, Sean <sean@starlabs.systems>; Lu, James <james.lu@intel.com>; Guo, Gua <gua.guo@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: [PATCH v2 5/6] UefiPayloadPkg: Specifies SmmCpuSyncLib instance
This patch is to specify SmmCpuSyncLib instance for UefiPayloadPkg.
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Guo Dong <guo.dong@intel.com>
Cc: Sean Rhodes <sean@starlabs.systems>
Cc: James Lu <james.lu@intel.com>
Cc: Gua Guo <gua.guo@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Zeng Star <star.zeng@intel.com>
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
---
UefiPayloadPkg/UefiPayloadPkg.dsc | 1 +
1 file changed, 1 insertion(+)
diff --git a/UefiPayloadPkg/UefiPayloadPkg.dsc b/UefiPayloadPkg/UefiPayloadPkg.dsc
index a65f9d5b83..b8b13ad201 100644
--- a/UefiPayloadPkg/UefiPayloadPkg.dsc
+++ b/UefiPayloadPkg/UefiPayloadPkg.dsc
@@ -253,10 +253,11 @@
#
MtrrLib|UefiCpuPkg/Library/MtrrLib/MtrrLib.inf
LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
MicrocodeLib|UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
CpuPageTableLib|UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableLib.inf
+ SmmCpuSyncLib|UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.inf
#
# Platform
#
!if $(CPU_TIMER_LIB_ENABLE) == TRUE && $(UNIVERSAL_PAYLOAD) == TRUE
--
2.16.2.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112085): https://edk2.groups.io/g/devel/message/112085
Mute This Topic: https://groups.io/mt/102889295/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH v2 2/6] UefiCpuPkg: Adds SmmCpuSyncLib library class
2023-11-30 6:31 ` [edk2-devel] [PATCH v2 2/6] UefiCpuPkg: Adds SmmCpuSyncLib library class Wu, Jiaxin
@ 2023-12-06 1:08 ` Ni, Ray
0 siblings, 0 replies; 11+ messages in thread
From: Ni, Ray @ 2023-12-06 1:08 UTC (permalink / raw)
To: Wu, Jiaxin, devel@edk2.groups.io
Cc: Laszlo Ersek, Dong, Eric, Zeng, Star, Gerd Hoffmann,
Kumar, Rahul R
Patch is good to me.
Only one comment: The file header comments should have 2 space chars in the beginning of each line.
Thanks,
Ray
> -----Original Message-----
> From: Wu, Jiaxin <jiaxin.wu@intel.com>
> Sent: Thursday, November 30, 2023 2:32 PM
> To: devel@edk2.groups.io
> Cc: Laszlo Ersek <lersek@redhat.com>; Dong, Eric <eric.dong@intel.com>; Ni,
> Ray <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Gerd Hoffmann
> <kraxel@redhat.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>
> Subject: [PATCH v2 2/6] UefiCpuPkg: Adds SmmCpuSyncLib library class
>
> Intel is planning to provide different SMM CPU Sync implementation
> along with some specific registers to improve the SMI performance,
> hence need SmmCpuSyncLib Library for Intel.
>
> This patch is to:
> 1.Adds SmmCpuSyncLib Library class in UefiCpuPkg.dec.
> 2.Adds SmmCpuSyncLib.h function declaration header file.
>
> For the new SmmCpuSyncLib, it provides 3 sets of APIs:
>
> 1. ContextInit/ContextDeinit/ContextReset:
> ContextInit() is called in driver's entrypoint to allocate and
> initialize the SMM CPU Sync context. ContextDeinit() is called in
> driver's unload function to deinitialize SMM CPU Sync context.
> ContextReset() is called before CPU exist SMI, which allows CPU to
> check into the next SMI from this point.
>
> 2. GetArrivedCpuCount/CheckInCpu/CheckOutCpu/LockDoor:
> When SMI happens, all processors including BSP enter to SMM mode by
> calling CheckInCpu(). The elected BSP calls LockDoor() so that
> CheckInCpu() will return the error code after that. CheckOutCpu() can
> be called in error handling flow for the CPU who calls CheckInCpu()
> earlier. GetArrivedCpuCount() returns the number of checked-in CPUs.
>
> 3. WaitForAPs/ReleaseOneAp/WaitForBsp/ReleaseBsp
> WaitForAPs() & ReleaseOneAp() are called from BSP to wait the number
> of APs and release one specific AP. WaitForBsp() & ReleaseBsp() are
> called from APs to wait and release BSP. The 4 APIs are used to
> synchronize the running flow among BSP and APs. BSP and AP Sync flow
> can be easy understand as below:
> BSP: ReleaseOneAp --> AP: WaitForBsp
> BSP: WaitForAPs <-- AP: ReleaseBsp
>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Zeng Star <star.zeng@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
> ---
> UefiCpuPkg/Include/Library/SmmCpuSyncLib.h | 278
> +++++++++++++++++++++++++++++
> UefiCpuPkg/UefiCpuPkg.dec | 3 +
> 2 files changed, 281 insertions(+)
> create mode 100644 UefiCpuPkg/Include/Library/SmmCpuSyncLib.h
>
> diff --git a/UefiCpuPkg/Include/Library/SmmCpuSyncLib.h
> b/UefiCpuPkg/Include/Library/SmmCpuSyncLib.h
> new file mode 100644
> index 0000000000..22935cc006
> --- /dev/null
> +++ b/UefiCpuPkg/Include/Library/SmmCpuSyncLib.h
> @@ -0,0 +1,278 @@
> +/** @file
> +Library that provides SMM CPU Sync related operations.
> +The lib provides 3 sets of APIs:
> +1. ContextInit/ContextDeinit/ContextReset:
> +ContextInit() is called in driver's entrypoint to allocate and initialize the SMM
> CPU Sync context.
> +ContextDeinit() is called in driver's unload function to deinitialize the SMM
> CPU Sync context.
> +ContextReset() is called before CPU exist SMI, which allows CPU to check
> into the next SMI from this point.
> +
> +2. GetArrivedCpuCount/CheckInCpu/CheckOutCpu/LockDoor:
> +When SMI happens, all processors including BSP enter to SMM mode by
> calling CheckInCpu().
> +The elected BSP calls LockDoor() so that CheckInCpu() will return the error
> code after that.
> +CheckOutCpu() can be called in error handling flow for the CPU who calls
> CheckInCpu() earlier.
> +GetArrivedCpuCount() returns the number of checked-in CPUs.
> +
> +3. WaitForAPs/ReleaseOneAp/WaitForBsp/ReleaseBsp
> +WaitForAPs() & ReleaseOneAp() are called from BSP to wait the number of
> APs and release one specific AP.
> +WaitForBsp() & ReleaseBsp() are called from APs to wait and release BSP.
> +The 4 APIs are used to synchronize the running flow among BSP and APs. BSP
> and AP Sync flow can be
> +easy understand as below:
> +BSP: ReleaseOneAp --> AP: WaitForBsp
> +BSP: WaitForAPs <-- AP: ReleaseBsp
> +
> +Copyright (c) 2023, Intel Corporation. All rights reserved.<BR>
> +SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#ifndef SMM_CPU_SYNC_LIB_H_
> +#define SMM_CPU_SYNC_LIB_H_
> +
> +#include <Uefi/UefiBaseType.h>
> +
> +//
> +// Opaque structure for SMM CPU Sync context.
> +//
> +typedef struct SMM_CPU_SYNC_CTX SMM_CPU_SYNC_CTX;
> +
> +/**
> + Create and initialize the SMM CPU Sync context.
> +
> + SmmCpuSyncContextInit() function is to allocate and initialize the SMM
> CPU Sync context.
> +
> + @param[in] NumberOfCpus The number of Logical
> Processors in the system.
> + @param[out] SmmCpuSyncCtx Pointer to the new created and
> initialized SMM CPU Sync context object.
> + NULL will be returned if any
> error happen during init.
> +
> + @retval RETURN_SUCCESS The SMM CPU Sync context
> was successful created and initialized.
> + @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx is NULL.
> + @retval RETURN_BUFFER_TOO_SMALL Overflow happen
> + @retval RETURN_OUT_OF_RESOURCES There are not enough
> resources available to create and initialize SMM CPU Sync context.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +SmmCpuSyncContextInit (
> + IN UINTN NumberOfCpus,
> + OUT SMM_CPU_SYNC_CTX **SmmCpuSyncCtx
> + );
> +
> +/**
> + Deinit an allocated SMM CPU Sync context.
> +
> + SmmCpuSyncContextDeinit() function is to deinitialize SMM CPU Sync
> context, the resources allocated in
> + SmmCpuSyncContextInit() will be freed.
> +
> + Note: This function only can be called after SmmCpuSyncContextInit()
> return success.
> +
> + @param[in,out] SmmCpuSyncCtx Pointer to the SMM CPU Sync
> context object to be deinitialized.
> +
> + @retval RETURN_SUCCESS The SMM CPU Sync context
> was successful deinitialized.
> + @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx is NULL.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +SmmCpuSyncContextDeinit (
> + IN OUT SMM_CPU_SYNC_CTX *SmmCpuSyncCtx
> + );
> +
> +/**
> + Reset SMM CPU Sync context.
> +
> + SmmCpuSyncContextReset() function is to reset SMM CPU Sync context to
> the initialized state.
> +
> + @param[in,out] SmmCpuSyncCtx Pointer to the SMM CPU Sync
> context object to be reset.
> +
> + @retval RETURN_SUCCESS The SMM CPU Sync context
> was successful reset.
> + @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx is NULL.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +SmmCpuSyncContextReset (
> + IN OUT SMM_CPU_SYNC_CTX *SmmCpuSyncCtx
> + );
> +
> +/**
> + Get current number of arrived CPU in SMI.
> +
> + For traditional CPU synchronization method, BSP might need to know the
> current number of arrived CPU in
> + SMI to make sure all APs in SMI. This API can be for that purpose.
> +
> + @param[in] SmmCpuSyncCtx Pointer to the SMM CPU Sync
> context object.
> + @param[in,out] CpuCount Current count of arrived CPU in
> SMI.
> +
> + @retval RETURN_SUCCESS Get current number of arrived
> CPU in SMI successfully.
> + @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx or CpuCount
> is NULL.
> + @retval RETURN_ABORTED Function Aborted due to the
> door has been locked by SmmCpuSyncLockDoor() function.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +SmmCpuSyncGetArrivedCpuCount (
> + IN SMM_CPU_SYNC_CTX *SmmCpuSyncCtx,
> + IN OUT UINTN *CpuCount
> + );
> +
> +/**
> + Performs an atomic operation to check in CPU.
> +
> + When SMI happens, all processors including BSP enter to SMM mode by
> calling SmmCpuSyncCheckInCpu().
> +
> + @param[in,out] SmmCpuSyncCtx Pointer to the SMM CPU Sync
> context object.
> + @param[in] CpuIndex Check in CPU index.
> +
> + @retval RETURN_SUCCESS Check in CPU (CpuIndex)
> successfully.
> + @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx is NULL.
> + @retval RETURN_ABORTED Check in CPU failed due to
> SmmCpuSyncLockDoor() has been called by one elected CPU.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +SmmCpuSyncCheckInCpu (
> + IN OUT SMM_CPU_SYNC_CTX *SmmCpuSyncCtx,
> + IN UINTN CpuIndex
> + );
> +
> +/**
> + Performs an atomic operation to check out CPU.
> +
> + CheckOutCpu() can be called in error handling flow for the CPU who calls
> CheckInCpu() earlier.
> +
> + @param[in,out] SmmCpuSyncCtx Pointer to the SMM CPU Sync
> context object.
> + @param[in] CpuIndex Check out CPU index.
> +
> + @retval RETURN_SUCCESS Check out CPU (CpuIndex)
> successfully.
> + @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx is NULL.
> + @retval RETURN_NOT_READY The CPU is not checked-in.
> + @retval RETURN_ABORTED Check out CPU failed due to
> SmmCpuSyncLockDoor() has been called by one elected CPU.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +SmmCpuSyncCheckOutCpu (
> + IN OUT SMM_CPU_SYNC_CTX *SmmCpuSyncCtx,
> + IN UINTN CpuIndex
> + );
> +
> +/**
> + Performs an atomic operation lock door for CPU checkin or checkout.
> +
> + After this function:
> + CPU can not check in via SmmCpuSyncCheckInCpu().
> + CPU can not check out via SmmCpuSyncCheckOutCpu().
> + CPU can not get number of arrived CPU in SMI via
> SmmCpuSyncGetArrivedCpuCount(). The number of
> + arrived CPU in SMI will be returned in CpuCount.
> +
> + The CPU specified by CpuIndex is elected to lock door.
> +
> + @param[in,out] SmmCpuSyncCtx Pointer to the SMM CPU Sync
> context object.
> + @param[in] CpuIndex Indicate which CPU to lock door.
> + @param[in,out] CpuCount Number of arrived CPU in SMI
> after look door.
> +
> + @retval RETURN_SUCCESS Lock door for CPU successfully.
> + @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx or CpuCount
> is NULL.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +SmmCpuSyncLockDoor (
> + IN OUT SMM_CPU_SYNC_CTX *SmmCpuSyncCtx,
> + IN UINTN CpuIndex,
> + IN OUT UINTN *CpuCount
> + );
> +
> +/**
> + Used by the BSP to wait for APs.
> +
> + The number of APs need to be waited is specified by NumberOfAPs. The
> BSP is specified by BspIndex.
> +
> + Note: This function is blocking mode, and it will return only after the
> number of APs released by
> + calling SmmCpuSyncReleaseBsp():
> + BSP: WaitForAPs <-- AP: ReleaseBsp
> +
> + @param[in,out] SmmCpuSyncCtx Pointer to the SMM CPU Sync
> context object.
> + @param[in] NumberOfAPs Number of APs need to be
> waited by BSP.
> + @param[in] BspIndex The BSP Index to wait for APs.
> +
> + @retval RETURN_SUCCESS BSP to wait for APs
> successfully.
> + @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx is NULL or
> NumberOfAPs > total number of processors in system.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +SmmCpuSyncWaitForAPs (
> + IN OUT SMM_CPU_SYNC_CTX *SmmCpuSyncCtx,
> + IN UINTN NumberOfAPs,
> + IN UINTN BspIndex
> + );
> +
> +/**
> + Used by the BSP to release one AP.
> +
> + The AP is specified by CpuIndex. The BSP is specified by BspIndex.
> +
> + @param[in,out] SmmCpuSyncCtx Pointer to the SMM CPU Sync
> context object.
> + @param[in] CpuIndex Indicate which AP need to be
> released.
> + @param[in] BspIndex The BSP Index to release AP.
> +
> + @retval RETURN_SUCCESS BSP to release one AP
> successfully.
> + @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx is NULL or
> CpuIndex is same as BspIndex.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +SmmCpuSyncReleaseOneAp (
> + IN OUT SMM_CPU_SYNC_CTX *SmmCpuSyncCtx,
> + IN UINTN CpuIndex,
> + IN UINTN BspIndex
> + );
> +
> +/**
> + Used by the AP to wait BSP.
> +
> + The AP is specified by CpuIndex. The BSP is specified by BspIndex.
> +
> + Note: This function is blocking mode, and it will return only after the AP
> released by
> + calling SmmCpuSyncReleaseOneAp():
> + BSP: ReleaseOneAp --> AP: WaitForBsp
> +
> + @param[in,out] SmmCpuSyncCtx Pointer to the SMM CPU Sync
> context object.
> + @param[in] CpuIndex Indicate which AP wait BSP.
> + @param[in] BspIndex The BSP Index to be waited.
> +
> + @retval RETURN_SUCCESS AP to wait BSP successfully.
> + @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx is NULL or
> CpuIndex is same as BspIndex.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +SmmCpuSyncWaitForBsp (
> + IN OUT SMM_CPU_SYNC_CTX *SmmCpuSyncCtx,
> + IN UINTN CpuIndex,
> + IN UINTN BspIndex
> + );
> +
> +/**
> + Used by the AP to release BSP.
> +
> + The AP is specified by CpuIndex. The BSP is specified by BspIndex.
> +
> + @param[in,out] SmmCpuSyncCtx Pointer to the SMM CPU Sync
> context object.
> + @param[in] CpuIndex Indicate which AP release BSP.
> + @param[in] BspIndex The BSP Index to be released.
> +
> + @retval RETURN_SUCCESS AP to release BSP successfully.
> + @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx is NULL or
> CpuIndex is same as BspIndex.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +SmmCpuSyncReleaseBsp (
> + IN OUT SMM_CPU_SYNC_CTX *SmmCpuSyncCtx,
> + IN UINTN CpuIndex,
> + IN UINTN BspIndex
> + );
> +
> +#endif
> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
> index 0b5431dbf7..20ab079219 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dec
> +++ b/UefiCpuPkg/UefiCpuPkg.dec
> @@ -62,10 +62,13 @@
> CpuPageTableLib|Include/Library/CpuPageTableLib.h
>
> ## @libraryclass Provides functions for manipulating smram savestate
> registers.
> MmSaveStateLib|Include/Library/MmSaveStateLib.h
>
> + ## @libraryclass Provides functions for SMM CPU Sync Operation.
> + SmmCpuSyncLib|Include/Library/SmmCpuSyncLib.h
> +
> [LibraryClasses.RISCV64]
> ## @libraryclass Provides functions to manage MMU features on
> RISCV64 CPUs.
> ##
> RiscVMmuLib|Include/Library/BaseRiscVMmuLib.h
>
> --
> 2.16.2.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112086): https://edk2.groups.io/g/devel/message/112086
Mute This Topic: https://groups.io/mt/102889292/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH v2 3/6] UefiCpuPkg: Implements SmmCpuSyncLib library instance
2023-11-30 6:31 ` [edk2-devel] [PATCH v2 3/6] UefiCpuPkg: Implements SmmCpuSyncLib library instance Wu, Jiaxin
@ 2023-12-06 3:18 ` Ni, Ray
2023-12-06 4:23 ` Wu, Jiaxin
0 siblings, 1 reply; 11+ messages in thread
From: Ni, Ray @ 2023-12-06 3:18 UTC (permalink / raw)
To: Wu, Jiaxin, devel@edk2.groups.io
Cc: Laszlo Ersek, Dong, Eric, Zeng, Star, Gerd Hoffmann,
Kumar, Rahul R
> +typedef struct {
> + ///
> + /// Indicate how many CPU entered SMM.
> + ///
> + volatile UINT32 *Counter;
> +} SMM_CPU_SYNC_SEMAPHORE_GLOBAL;
> +
> +typedef struct {
> + ///
> + /// Used for control each CPU continue run or wait for signal
> + ///
> + volatile UINT32 *Run;
> +} SMM_CPU_SYNC_SEMAPHORE_CPU;
> +
> +struct SMM_CPU_SYNC_CTX {
1. How about "SMM_CPU_SYNC_CONTEXT"?
> + ///
> + /// All global semaphores' pointer in SMM CPU Sync
> + ///
> + SMM_CPU_SYNC_SEMAPHORE_GLOBAL *GlobalSem;
2. There is only one GlobalSem. Can you directly use "volatile UINT32 *Counter" instead of "GlobalSem"?
> + ///
> + /// All semaphores for each processor in SMM CPU Sync
> + ///
> + SMM_CPU_SYNC_SEMAPHORE_CPU *CpuSem;
3. Can we use "volatile UINT32 **Run" instead of pointing to another structure?
Run points to an array where each element is a UINT32 *. Count of array equals to NumberOfCpus.
> + ///
> + /// The number of processors in the system.
> + /// This does not indicate the number of processors that entered SMM.
> + ///
> + UINTN NumberOfCpus;
> + ///
> + /// Address of global and each CPU semaphores
> + ///
> + UINTN *SemBlock;
4. How about "SemBuffer"?
> + ///
> + /// Size of global and each CPU semaphores
> + ///
> + UINTN SemBlockPages;
5. How about "SemBufferSize"?
> +};
> +
> +EFIAPI
> +SmmCpuSyncContextInit (
> + IN UINTN NumberOfCpus,
> + OUT SMM_CPU_SYNC_CTX **SmmCpuSyncCtx
> + )
> +{
> + RETURN_STATUS Status;
> +
> + UINTN CpuSemInCtxSize;
> + UINTN CtxSize;
> +
> + UINTN OneSemSize;
> + UINTN GlobalSemSize;
> + UINTN OneCpuSemSize;
> + UINTN CpuSemSize;
> + UINTN TotalSemSize;
> +
> + UINTN SemAddr;
> + UINTN CpuIndex;
6. Can you remove the empty lines among the local variable declarations?
> +
> + if (SmmCpuSyncCtx == NULL) {
> + return RETURN_INVALID_PARAMETER;
> + }
> +
> + //
> + // Count the CtxSize
> + //
> + Status = SafeUintnMult (NumberOfCpus, sizeof
> (SMM_CPU_SYNC_SEMAPHORE_CPU), &CpuSemInCtxSize);
7. Is there a reason to use SafeUintxxx()? I don't believe the multiplication could exceed MAX_UINTN.
But using SafeUintxxx() makes code hard to read.
> + //
> + // Allocate CtxSize buffer for the *SmmCpuSyncCtx
> + //
> + *SmmCpuSyncCtx = NULL;
8. This is not needed.
> + Status = SafeUintnMult (OneSemSize, sizeof
> (SMM_CPU_SYNC_SEMAPHORE_GLOBAL) / sizeof (VOID *), &GlobalSemSize);
> + if (EFI_ERROR (Status)) {
> + goto ON_ERROR;
9. If you don't use SafeUintxxx(), you don't need "ON_ERROR" label and the "goto".
> +/**
> + Performs an atomic operation to check in CPU.
> +
> + When SMI happens, all processors including BSP enter to SMM mode by
> calling SmmCpuSyncCheckInCpu().
> +
> + @param[in,out] SmmCpuSyncCtx Pointer to the SMM CPU Sync context
> object.
> + @param[in] CpuIndex Check in CPU index.
> +
> + @retval RETURN_SUCCESS Check in CPU (CpuIndex) successfully.
> + @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx is NULL.
> + @retval RETURN_ABORTED Check in CPU failed due to
> SmmCpuSyncLockDoor() has been called by one elected CPU.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +SmmCpuSyncCheckInCpu (
> + IN OUT SMM_CPU_SYNC_CTX *SmmCpuSyncCtx,
> + IN UINTN CpuIndex
> + )
> +{
> + SMM_CPU_SYNC_CTX *Ctx;
> +
> + if (SmmCpuSyncCtx == NULL) {
> + return RETURN_INVALID_PARAMETER;
> + }
10. Can we use "ASSERT" instead of if-check? We can explicitly mention the behavior in API comments.
> +
> + Ctx = (SMM_CPU_SYNC_CTX *)SmmCpuSyncCtx;
11. Why do we need the type cast?
> +
> +/**
> + Performs an atomic operation lock door for CPU checkin or checkout.
> +
> + After this function:
> + CPU can not check in via SmmCpuSyncCheckInCpu().
> + CPU can not check out via SmmCpuSyncCheckOutCpu().
> + CPU can not get number of arrived CPU in SMI via
> SmmCpuSyncGetArrivedCpuCount(). The number of
> + arrived CPU in SMI will be returned in CpuCount.
12. I agree that after locking, cpu cannot "checkin".
But can cpu "checkout" or "getArrivedcount"? I need to double check.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112087): https://edk2.groups.io/g/devel/message/112087
Mute This Topic: https://groups.io/mt/102889293/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH v2 3/6] UefiCpuPkg: Implements SmmCpuSyncLib library instance
2023-12-06 3:18 ` Ni, Ray
@ 2023-12-06 4:23 ` Wu, Jiaxin
0 siblings, 0 replies; 11+ messages in thread
From: Wu, Jiaxin @ 2023-12-06 4:23 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io
Cc: Laszlo Ersek, Dong, Eric, Zeng, Star, Gerd Hoffmann,
Kumar, Rahul R
> > +struct SMM_CPU_SYNC_CTX {
>
> 1. How about "SMM_CPU_SYNC_CONTEXT"?
Agree.
>
> > + ///
> > + /// All global semaphores' pointer in SMM CPU Sync
> > + ///
> > + SMM_CPU_SYNC_SEMAPHORE_GLOBAL *GlobalSem;
>
> 2. There is only one GlobalSem. Can you directly use "volatile UINT32
> *Counter" instead of "GlobalSem"?
I think it's not the big deal to combine into one structure or separate into different structures.
Separating different attribute field into different structures will benefit the code readability & maintainability & expansibility. It's easy to understand which field is for GlobalSem, and which field is for CpuSem.
With separated structures, it will very easy coding the later logic for semaphore memory allocation/free. Because you know we need 64 bytes alignment for semaphore, in the later coding, Run[cpuIndex] can't meet requirement for that since it's UINT32 type. That will bring a lot of inconvenience for coding and very easy to make mistake. Besides, we only need allcoate/free one continuous Sembuffer for all semaphores, and easy for consumer point to next with CpuSem[CpuIndex].Run.
>
> > + ///
> > + /// All semaphores for each processor in SMM CPU Sync
> > + ///
> > + SMM_CPU_SYNC_SEMAPHORE_CPU *CpuSem;
>
> 3. Can we use "volatile UINT32 **Run" instead of pointing to another
> structure?
> Run points to an array where each element is a UINT32 *. Count of array
> equals to NumberOfCpus.
>
>
Same as above explained.
>
> > + ///
> > + /// The number of processors in the system.
> > + /// This does not indicate the number of processors that entered SMM.
> > + ///
> > + UINTN NumberOfCpus;
> > + ///
> > + /// Address of global and each CPU semaphores
> > + ///
> > + UINTN *SemBlock;
> 4. How about "SemBuffer"?
>
Agree.
>
>
> > + ///
> > + /// Size of global and each CPU semaphores
> > + ///
> > + UINTN SemBlockPages;
>
> 5. How about "SemBufferSize"?
Agree, you want it to be bytes instead of pages?
>
> > +};
> > +
> > +EFIAPI
> > +SmmCpuSyncContextInit (
> > + IN UINTN NumberOfCpus,
> > + OUT SMM_CPU_SYNC_CTX **SmmCpuSyncCtx
> > + )
> > +{
> > + RETURN_STATUS Status;
> > +
> > + UINTN CpuSemInCtxSize;
> > + UINTN CtxSize;
> > +
> > + UINTN OneSemSize;
> > + UINTN GlobalSemSize;
> > + UINTN OneCpuSemSize;
> > + UINTN CpuSemSize;
> > + UINTN TotalSemSize;
> > +
> > + UINTN SemAddr;
> > + UINTN CpuIndex;
>
> 6. Can you remove the empty lines among the local variable declarations?
Agree. I just wanted it to be more readable: no empty lines means it's group usage for some purpose, that was my original coding style, but I can change it since you don't like it. :).
>
> > +
> > + if (SmmCpuSyncCtx == NULL) {
> > + return RETURN_INVALID_PARAMETER;
> > + }
> > +
> > + //
> > + // Count the CtxSize
> > + //
> > + Status = SafeUintnMult (NumberOfCpus, sizeof
> > (SMM_CPU_SYNC_SEMAPHORE_CPU), &CpuSemInCtxSize);
>
> 7. Is there a reason to use SafeUintxxx()? I don't believe the multiplication
> could exceed MAX_UINTN.
> But using SafeUintxxx() makes code hard to read.
>
This is comments from Laszlo:
Please use SafeIntLib for all calculations, to prevent overflows. This
means primarily (but not exclusively) memory size calculations, for
allocations
From API perspective, it's possible since the NumberOfCpus defined as UNITN, right?
> > + //
> > + // Allocate CtxSize buffer for the *SmmCpuSyncCtx
> > + //
> > + *SmmCpuSyncCtx = NULL;
> 8. This is not needed.
>
> > + Status = SafeUintnMult (OneSemSize, sizeof
> > (SMM_CPU_SYNC_SEMAPHORE_GLOBAL) / sizeof (VOID *),
> &GlobalSemSize);
> > + if (EFI_ERROR (Status)) {
> > + goto ON_ERROR;
>
> 9. If you don't use SafeUintxxx(), you don't need "ON_ERROR" label and the
> "goto".
>
Yes, we can discuss whether need SafeUintxxx(), for me, it's both ok. SafeUintxxx doesn't do bad things.
>
> > +/**
> > + Performs an atomic operation to check in CPU.
> > +
> > + When SMI happens, all processors including BSP enter to SMM mode by
> > calling SmmCpuSyncCheckInCpu().
> > +
> > + @param[in,out] SmmCpuSyncCtx Pointer to the SMM CPU Sync
> context
> > object.
> > + @param[in] CpuIndex Check in CPU index.
> > +
> > + @retval RETURN_SUCCESS Check in CPU (CpuIndex) successfully.
> > + @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx is NULL.
> > + @retval RETURN_ABORTED Check in CPU failed due to
> > SmmCpuSyncLockDoor() has been called by one elected CPU.
> > +
> > +**/
> > +RETURN_STATUS
> > +EFIAPI
> > +SmmCpuSyncCheckInCpu (
> > + IN OUT SMM_CPU_SYNC_CTX *SmmCpuSyncCtx,
> > + IN UINTN CpuIndex
> > + )
> > +{
> > + SMM_CPU_SYNC_CTX *Ctx;
> > +
> > + if (SmmCpuSyncCtx == NULL) {
> > + return RETURN_INVALID_PARAMETER;
> > + }
>
> 10. Can we use "ASSERT" instead of if-check? We can explicitly mention the
> behavior in API comments.
Assert can be added by caller for status if wanted. The API only follow the definition, right? Or add assert before if check?
>
> > +
> > + Ctx = (SMM_CPU_SYNC_CTX *)SmmCpuSyncCtx;
>
> 11. Why do we need the type cast?
Oh, yes, we don't need that, original code defined the ctx as void, I forget update this.
>
> > +
> > +/**
> > + Performs an atomic operation lock door for CPU checkin or checkout.
> > +
> > + After this function:
> > + CPU can not check in via SmmCpuSyncCheckInCpu().
> > + CPU can not check out via SmmCpuSyncCheckOutCpu().
> > + CPU can not get number of arrived CPU in SMI via
> > SmmCpuSyncGetArrivedCpuCount(). The number of
> > + arrived CPU in SMI will be returned in CpuCount.
> 12. I agree that after locking, cpu cannot "checkin".
> But can cpu "checkout" or "getArrivedcount"? I need to double check.
>
Existing implementation logic or SMM_CPU_SYNC_CONTEXT definition doesn't support the checkout or getArrivedcount after lock. I don't want implementation support such kind of case, so added that. Or can we return unsupported status if locked directly? then we can remove that words.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112089): https://edk2.groups.io/g/devel/message/112089
Mute This Topic: https://groups.io/mt/102889293/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-12-06 4:23 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-30 6:31 [edk2-devel] [PATCH v2 0/6] Refine SMM CPU Sync flow and abstract SmmCpuSyncLib Wu, Jiaxin
2023-11-30 6:31 ` [edk2-devel] [PATCH v2 1/6] UefiCpuPkg/PiSmmCpuDxeSmm: Optimize Semaphore Sync between BSP and AP Wu, Jiaxin
2023-11-30 6:31 ` [edk2-devel] [PATCH v2 2/6] UefiCpuPkg: Adds SmmCpuSyncLib library class Wu, Jiaxin
2023-12-06 1:08 ` Ni, Ray
2023-11-30 6:31 ` [edk2-devel] [PATCH v2 3/6] UefiCpuPkg: Implements SmmCpuSyncLib library instance Wu, Jiaxin
2023-12-06 3:18 ` Ni, Ray
2023-12-06 4:23 ` Wu, Jiaxin
2023-11-30 6:31 ` [edk2-devel] [PATCH v2 4/6] OvmfPkg: Specifies SmmCpuSyncLib instance Wu, Jiaxin
2023-11-30 6:31 ` [edk2-devel] [PATCH v2 5/6] UefiPayloadPkg: " Wu, Jiaxin
2023-12-06 0:09 ` Guo, Gua
2023-11-30 6:31 ` [edk2-devel] [PATCH v2 6/6] UefiCpuPkg/PiSmmCpuDxeSmm: Consume SmmCpuSyncLib Wu, Jiaxin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox