* [edk2-devel] [PATCH v1 1/7] UefiCpuPkg/PiSmmCpuDxeSmm: Optimize Semaphore Sync between BSP and AP
2023-11-03 15:30 [edk2-devel] [PATCH v1 0/7] Refine SMM CPU Sync flow and abstract SmmCpuSyncLib Wu, Jiaxin
@ 2023-11-03 15:30 ` Wu, Jiaxin
2023-11-07 8:28 ` Laszlo Ersek
2023-11-03 15:30 ` [edk2-devel] [PATCH v1 2/7] UefiCpuPkg/PiSmmCpuDxeSmm: Reduce times of BSP and AP Sync for SMM Exit Wu, Jiaxin
` (5 subsequent siblings)
6 siblings, 1 reply; 21+ messages in thread
From: Wu, Jiaxin @ 2023-11-03 15:30 UTC (permalink / raw)
To: devel; +Cc: 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
Change-Id: I0fb25e26e1015e918800f4d8d62e5276dcd5b5b1
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 25d058c5b9..e96c7f51d6 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 0
+**/
+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
+ // WaitForBsp() 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
+ // WaitForBsp() 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 (#110637): https://edk2.groups.io/g/devel/message/110637
Mute This Topic: https://groups.io/mt/102366297/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] 21+ messages in thread* Re: [edk2-devel] [PATCH v1 1/7] UefiCpuPkg/PiSmmCpuDxeSmm: Optimize Semaphore Sync between BSP and AP
2023-11-03 15:30 ` [edk2-devel] [PATCH v1 1/7] UefiCpuPkg/PiSmmCpuDxeSmm: Optimize Semaphore Sync between BSP and AP Wu, Jiaxin
@ 2023-11-07 8:28 ` Laszlo Ersek
2023-11-07 10:27 ` Laszlo Ersek
0 siblings, 1 reply; 21+ messages in thread
From: Laszlo Ersek @ 2023-11-07 8:28 UTC (permalink / raw)
To: devel, jiaxin.wu; +Cc: Eric Dong, Ray Ni, Zeng Star, Rahul Kumar, Gerd Hoffmann
On 11/3/23 16:30, Wu, Jiaxin wrote:
> 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
>
> Change-Id: I0fb25e26e1015e918800f4d8d62e5276dcd5b5b1
(1) please remove this; not useful upstream
> 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 25d058c5b9..e96c7f51d6 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 0
(2) wrong comment; WaitForSemaphore() says "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
> + // WaitForBsp() 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
> + // WaitForBsp() 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);
>
(3) I believe (but am not fully sure) that the above comment updates are
wrong. Both contexts belong to BSPHandler(), where the BSP orchestrates
MTRR programming on all processors (which must occur on all processors
at the same time). The comments explain that the BSP releases the APs to
do their jobs, and then waits for them to finish. We have two
WaitForAllAPs() calls. IOW, the BSP does not wait for the BSP, but the
APs. Thus, the updated comments should say WaitForAllAPs(), not
WaitForBsp().
> @@ -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);
> }
Looks OK to me otherwise.
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110822): https://edk2.groups.io/g/devel/message/110822
Mute This Topic: https://groups.io/mt/102366297/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] 21+ messages in thread* Re: [edk2-devel] [PATCH v1 1/7] UefiCpuPkg/PiSmmCpuDxeSmm: Optimize Semaphore Sync between BSP and AP
2023-11-07 8:28 ` Laszlo Ersek
@ 2023-11-07 10:27 ` Laszlo Ersek
0 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2023-11-07 10:27 UTC (permalink / raw)
To: devel, jiaxin.wu; +Cc: Eric Dong, Ray Ni, Zeng Star, Rahul Kumar, Gerd Hoffmann
On 11/7/23 09:28, Laszlo Ersek wrote:
> On 11/3/23 16:30, Wu, Jiaxin wrote:
>> 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
>>
>> Change-Id: I0fb25e26e1015e918800f4d8d62e5276dcd5b5b1
>
> (1) please remove this; not useful upstream
... this comment applies to all patches
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110836): https://edk2.groups.io/g/devel/message/110836
Mute This Topic: https://groups.io/mt/102366297/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] 21+ messages in thread
* [edk2-devel] [PATCH v1 2/7] UefiCpuPkg/PiSmmCpuDxeSmm: Reduce times of BSP and AP Sync for SMM Exit
2023-11-03 15:30 [edk2-devel] [PATCH v1 0/7] Refine SMM CPU Sync flow and abstract SmmCpuSyncLib Wu, Jiaxin
2023-11-03 15:30 ` [edk2-devel] [PATCH v1 1/7] UefiCpuPkg/PiSmmCpuDxeSmm: Optimize Semaphore Sync between BSP and AP Wu, Jiaxin
@ 2023-11-03 15:30 ` Wu, Jiaxin
2023-11-07 9:47 ` Laszlo Ersek
2023-11-03 15:30 ` [edk2-devel] [PATCH v1 3/7] UefiCpuPkg: Adds SmmCpuSyncLib library class Wu, Jiaxin
` (4 subsequent siblings)
6 siblings, 1 reply; 21+ messages in thread
From: Wu, Jiaxin @ 2023-11-03 15:30 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Zeng Star, Rahul Kumar, Gerd Hoffmann
After review, there are unnecessary steps for BSP and AP sync for SMM
exit. This patch is to reduce one round BSP and AP sync so as to improve
SMI performance:
BSP: WaitForAllAPs <-- AP: ReleaseBsp
BSP: ReleaseAllAPs --> AP: WaitForBsp
Change-Id: Ic33f42f3daa7ff1847e524d0c3d9cd4fcdefa61b
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 | 44 +++++++++++++++++++----------------
1 file changed, 24 insertions(+), 20 deletions(-)
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index e96c7f51d6..5a42a5dd12 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -665,11 +665,13 @@ BSPHandler (
//
*mSmmMpSyncData->AllCpusInSync = TRUE;
ApCount = LockdownSemaphore (mSmmMpSyncData->Counter) - 1;
//
- // Wait for all APs to get ready for programming MTRRs
+ // Wait for all APs:
+ // 1. Make sure all Aps have set the Present.
+ // 2. Get ready for programming MTRRs.
//
WaitForAllAPs (ApCount);
if (SmmCpuFeaturesNeedConfigureMtrrs ()) {
//
@@ -768,16 +770,16 @@ BSPHandler (
// Notify all APs to exit
//
*mSmmMpSyncData->InsideSmm = FALSE;
ReleaseAllAPs ();
- //
- // Wait for all APs to complete their pending tasks
- //
- WaitForAllAPs (ApCount);
-
if (SmmCpuFeaturesNeedConfigureMtrrs ()) {
+ //
+ // Wait for all APs to complete their pending tasks
+ //
+ WaitForAllAPs (ApCount);
+
//
// Signal APs to restore MTRRs
//
ReleaseAllAPs ();
@@ -789,23 +791,23 @@ BSPHandler (
//
// Wait for all APs to complete MTRR programming
//
WaitForAllAPs (ApCount);
+
+ //
+ // Signal APs to Reset states/semaphore for this processor
+ //
+ ReleaseAllAPs ();
}
//
// Stop source level debug in BSP handler, the code below will not be
// debugged.
//
InitializeDebugAgent (DEBUG_AGENT_INIT_EXIT_SMI, NULL, NULL);
- //
- // Signal APs to Reset states/semaphore for this processor
- //
- ReleaseAllAPs ();
-
//
// Perform pending operations for hot-plug
//
SmmCpuUpdate ();
@@ -941,10 +943,12 @@ APHandler (
*(mSmmMpSyncData->CpuData[CpuIndex].Present) = TRUE;
if ((SyncMode == SmmCpuSyncModeTradition) || SmmCpuFeaturesNeedConfigureMtrrs ()) {
//
// Notify BSP of arrival at this point
+ // 1. Set the Present.
+ // 2. Get ready for programming MTRRs.
//
ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run);
}
if (SmmCpuFeaturesNeedConfigureMtrrs ()) {
@@ -1033,21 +1037,21 @@ APHandler (
//
// Restore OS MTRRs
//
SmmCpuFeaturesReenableSmrr ();
MtrrSetAllMtrrs (&Mtrrs);
- }
- //
- // Notify BSP the readiness of this AP to Reset states/semaphore for this processor
- //
- ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run);
+ //
+ // Notify BSP the readiness of this AP to Reset states/semaphore for this processor
+ //
+ ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run);
- //
- // Wait for the signal from BSP to Reset states/semaphore for this processor
- //
- WaitForBsp (mSmmMpSyncData->CpuData[CpuIndex].Run);
+ //
+ // Wait for the signal from BSP to Reset states/semaphore for this processor
+ //
+ WaitForBsp (mSmmMpSyncData->CpuData[CpuIndex].Run);
+ }
//
// Reset states/semaphore for this processor
//
*(mSmmMpSyncData->CpuData[CpuIndex].Present) = FALSE;
--
2.16.2.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110638): https://edk2.groups.io/g/devel/message/110638
Mute This Topic: https://groups.io/mt/102366299/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] 21+ messages in thread* Re: [edk2-devel] [PATCH v1 2/7] UefiCpuPkg/PiSmmCpuDxeSmm: Reduce times of BSP and AP Sync for SMM Exit
2023-11-03 15:30 ` [edk2-devel] [PATCH v1 2/7] UefiCpuPkg/PiSmmCpuDxeSmm: Reduce times of BSP and AP Sync for SMM Exit Wu, Jiaxin
@ 2023-11-07 9:47 ` Laszlo Ersek
0 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2023-11-07 9:47 UTC (permalink / raw)
To: devel, jiaxin.wu; +Cc: Eric Dong, Ray Ni, Zeng Star, Rahul Kumar, Gerd Hoffmann
On 11/3/23 16:30, Wu, Jiaxin wrote:
> After review, there are unnecessary steps for BSP and AP sync for SMM
> exit. This patch is to reduce one round BSP and AP sync so as to improve
> SMI performance:
> BSP: WaitForAllAPs <-- AP: ReleaseBsp
> BSP: ReleaseAllAPs --> AP: WaitForBsp
>
> Change-Id: Ic33f42f3daa7ff1847e524d0c3d9cd4fcdefa61b
(1) please drop this
> 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 | 44 +++++++++++++++++++----------------
> 1 file changed, 24 insertions(+), 20 deletions(-)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index e96c7f51d6..5a42a5dd12 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -665,11 +665,13 @@ BSPHandler (
> //
> *mSmmMpSyncData->AllCpusInSync = TRUE;
> ApCount = LockdownSemaphore (mSmmMpSyncData->Counter) - 1;
>
> //
> - // Wait for all APs to get ready for programming MTRRs
> + // Wait for all APs:
> + // 1. Make sure all Aps have set the Present.
(2.1) This comment ("set the Present") is confusing.
Either say "set the Present *flag*", or just "set Present".
(2.2) This comment update is unrelated to the performance optimization;
it just documents existent *and preserved* behavior.
It's very confusing to see this in the same patch. The comment documents
behavior that *precedes* the "Wait for something to happen" loop and the
"Invoke the scheduled procedure" action in APHandler(), but the
performance optimization is *after* that loop. (As the subject says, the
optimization is for the exit path.)
So please split the comment update to a separate patch.
> + // 2. Get ready for programming MTRRs.
> //
> WaitForAllAPs (ApCount);
>
> if (SmmCpuFeaturesNeedConfigureMtrrs ()) {
> //
> @@ -768,16 +770,16 @@ BSPHandler (
> // Notify all APs to exit
> //
> *mSmmMpSyncData->InsideSmm = FALSE;
> ReleaseAllAPs ();
>
> - //
> - // Wait for all APs to complete their pending tasks
> - //
> - WaitForAllAPs (ApCount);
> -
> if (SmmCpuFeaturesNeedConfigureMtrrs ()) {
> + //
> + // Wait for all APs to complete their pending tasks
> + //
> + WaitForAllAPs (ApCount);
> +
> //
> // Signal APs to restore MTRRs
> //
> ReleaseAllAPs ();
>
> @@ -789,23 +791,23 @@ BSPHandler (
>
> //
> // Wait for all APs to complete MTRR programming
> //
> WaitForAllAPs (ApCount);
> +
> + //
> + // Signal APs to Reset states/semaphore for this processor
> + //
> + ReleaseAllAPs ();
> }
>
> //
> // Stop source level debug in BSP handler, the code below will not be
> // debugged.
> //
> InitializeDebugAgent (DEBUG_AGENT_INIT_EXIT_SMI, NULL, NULL);
>
> - //
> - // Signal APs to Reset states/semaphore for this processor
> - //
> - ReleaseAllAPs ();
> -
> //
> // Perform pending operations for hot-plug
> //
> SmmCpuUpdate ();
>
> @@ -941,10 +943,12 @@ APHandler (
> *(mSmmMpSyncData->CpuData[CpuIndex].Present) = TRUE;
>
> if ((SyncMode == SmmCpuSyncModeTradition) || SmmCpuFeaturesNeedConfigureMtrrs ()) {
> //
> // Notify BSP of arrival at this point
> + // 1. Set the Present.
(3) Same as (2) (both sub-points).
> + // 2. Get ready for programming MTRRs.
> //
> ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run);
> }
>
> if (SmmCpuFeaturesNeedConfigureMtrrs ()) {
> @@ -1033,21 +1037,21 @@ APHandler (
> //
> // Restore OS MTRRs
> //
> SmmCpuFeaturesReenableSmrr ();
> MtrrSetAllMtrrs (&Mtrrs);
> - }
>
> - //
> - // Notify BSP the readiness of this AP to Reset states/semaphore for this processor
> - //
> - ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run);
> + //
> + // Notify BSP the readiness of this AP to Reset states/semaphore for this processor
> + //
> + ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run);
>
> - //
> - // Wait for the signal from BSP to Reset states/semaphore for this processor
> - //
> - WaitForBsp (mSmmMpSyncData->CpuData[CpuIndex].Run);
> + //
> + // Wait for the signal from BSP to Reset states/semaphore for this processor
> + //
> + WaitForBsp (mSmmMpSyncData->CpuData[CpuIndex].Run);
> + }
>
> //
> // Reset states/semaphore for this processor
> //
> *(mSmmMpSyncData->CpuData[CpuIndex].Present) = FALSE;
(4.1) If SmmCpuFeaturesNeedConfigureMtrrs() returns TRUE, then:
- the AP logic in unchanged
- the BSP logic changes, because InitializeDebugAgent() is invoked with
the APs released
(4.2) If SmmCpuFeaturesNeedConfigureMtrrs() returns FALSE, then:
- the AP logic omits an empty rendezvous with the BSP
- the BSP logic also omits the empty rendezvous with the APs, but it's
cheating: the rendezvous on the BSP side is originally *not* empty even
when there is no need to configure MTRRs -- the rendezvous protects the
InitializeDebugAgent() call.
(4.3) So, we need yet another precursor patch for this performance
optimization. Namely, you need to show that calling
InitializeDebugAgent() with DEBUG_AGENT_INIT_EXIT_SMI, with all the APs
released, is safe. Put differently, a precursor patch is needed that
does nothing other than *reordering* the ReleaseAllAPs() and
InitializeDebugAgent() calls in BSPHandler(). You need to justify and
document that change in a separate patch, and once that is in place, you
can employ this performance optimization.
(5) After the performance optimization, the comments on the final
semaphore operations inside the MTRR branch are misleading. Those
comments say
Signal APs to Reset states/semaphore for this processor
on the BSP side, and they say
Notify BSP the readiness of this AP to Reset states/semaphore for this
processor
and
Wait for the signal from BSP to Reset states/semaphore for this
processor
on the AP side.
The problem is, this structuring and this wording imply that "state
resetting" is *conditional* on MTRR programming, which is of course not
true. State resetting must happen in all cases, it's only the
*additional rendezvous* that is necessary for, and conditional on, MTRR
configuration.
Put differently:
- Before the performance optimization, we have a rendezvous that
*unconditionally* happens (it is independent of MTRR configuration). The
good side of that is that we can directly tie this unconditional
rendezvous to the final "state resetting", and therefore the comments on
the rendezvous logic are correct, before the patch. However, the bad
side (before the patch) is that this approach wastes performance, when
MTRR config is unneeded.
- After the performance optimization, the rendezvous occurs only when it
is really required, for MTRR config. This is good. The bad side is that
the original comments no longer fit -- the rendezvous is now tied to
MTRR config, *not* the final state resetting!
All in all, the comments on the BSP side should be:
if (SmmCpuFeaturesNeedConfigureMtrrs ()) {
//
// Sync with the APs just before MTRR restoration
//
WaitForAllAPs (ApCount);
ReleaseAllAPs ();
//
// Restore OS MTRRs
//
SmmCpuFeaturesReenableSmrr ();
MtrrSetAllMtrrs (&Mtrrs);
//
// Sync with the APs just after MTRR restoration
//
WaitForAllAPs (ApCount);
ReleaseAllAPs ();
}
And on the AP side:
if (SmmCpuFeaturesNeedConfigureMtrrs ()) {
//
// Sync with the BSP just before MTRR restoration
//
ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run);
WaitForBsp (mSmmMpSyncData->CpuData[CpuIndex].Run);
//
// Restore OS MTRRs
//
SmmCpuFeaturesReenableSmrr ();
MtrrSetAllMtrrs (&Mtrrs);
//
// Sync with the BSP just after MTRR restoration
//
ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run);
WaitForBsp (mSmmMpSyncData->CpuData[CpuIndex].Run);
}
* In summary:
- First patch: update the comment in the top part of the BSP and AP
handlers.
- Second patch: invert the order of ReleaseAllAPs() and
InitializeDebugAgent(DEBUG_AGENT_INIT_EXIT_SMI) in BSPHandler().
Furthermore, explain in the commit message that this reordering is valid.
- Third patch: the perf optimization. Restrict the rendezvous to when
MTRR configuration is needed. While at it, (a) explain the nature of the
opimization in the commit message (i.e., that without MTRR config, we
have a superfluous rendezvous), because the current commit message is
useless, (b) clean up the comments on the before and after
synchronizations (see proposal above).
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110829): https://edk2.groups.io/g/devel/message/110829
Mute This Topic: https://groups.io/mt/102366299/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] 21+ messages in thread
* [edk2-devel] [PATCH v1 3/7] UefiCpuPkg: Adds SmmCpuSyncLib library class
2023-11-03 15:30 [edk2-devel] [PATCH v1 0/7] Refine SMM CPU Sync flow and abstract SmmCpuSyncLib Wu, Jiaxin
2023-11-03 15:30 ` [edk2-devel] [PATCH v1 1/7] UefiCpuPkg/PiSmmCpuDxeSmm: Optimize Semaphore Sync between BSP and AP Wu, Jiaxin
2023-11-03 15:30 ` [edk2-devel] [PATCH v1 2/7] UefiCpuPkg/PiSmmCpuDxeSmm: Reduce times of BSP and AP Sync for SMM Exit Wu, Jiaxin
@ 2023-11-03 15:30 ` Wu, Jiaxin
2023-11-07 10:26 ` Laszlo Ersek
2023-11-03 15:30 ` [edk2-devel] [PATCH v1 4/7] UefiCpuPkg: Implements SmmCpuSyncLib library instance Wu, Jiaxin
` (3 subsequent siblings)
6 siblings, 1 reply; 21+ messages in thread
From: Wu, Jiaxin @ 2023-11-03 15:30 UTC (permalink / raw)
To: devel; +Cc: 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.
Change-Id: Ib7f5e317526e8b9e29b65e072bdb485dbd678817
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 | 191 +++++++++++++++++++++++++++++
UefiCpuPkg/UefiCpuPkg.dec | 3 +
2 files changed, 194 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..b9b190c516
--- /dev/null
+++ b/UefiCpuPkg/Include/Library/SmmCpuSyncLib.h
@@ -0,0 +1,191 @@
+/** @file
+Library that provides SMM CPU Sync related operations.
+
+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>
+
+/**
+ Creates and Init a new Smm Cpu Sync context.
+
+ @param[in] NumberOfCpus The number of processors in the system.
+
+ @return Pointer to an allocated Smm Cpu Sync context object.
+ If the creation failed, returns NULL.
+
+**/
+VOID *
+EFIAPI
+SmmCpuSyncContextInit (
+ IN UINTN NumberOfCpus
+ );
+
+/**
+ Deinit an allocated Smm Cpu Sync context object.
+
+ @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object to be released.
+
+**/
+VOID
+EFIAPI
+SmmCpuSyncContextDeinit (
+ IN VOID *SmmCpuSyncCtx
+ );
+
+/**
+ Reset Smm Cpu Sync context object.
+
+ @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object to be released.
+
+**/
+VOID
+EFIAPI
+SmmCpuSyncContextReset (
+ IN VOID *SmmCpuSyncCtx
+ );
+
+/**
+ Get current arrived CPU count.
+
+ @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object to be released.
+
+ @return Current number of arrived CPU count.
+ -1: indicate the door has been locked.
+
+**/
+UINT32
+EFIAPI
+SmmCpuSyncGetArrivedCpuCount (
+ IN VOID *SmmCpuSyncCtx
+ );
+
+/**
+ Performs an atomic operation to check in CPU.
+ Check in CPU successfully if the returned arrival CPU count value is
+ positive, otherwise indicate the door has been locked, the CPU can
+ not checkin.
+
+ @param[in] SmmCpuSyncCtx Pointer to the Smm CPU Sync context object to be released.
+ @param[in] CpuIndex Pointer to the CPU Index to checkin.
+
+ @return Positive value (>0): CPU arrival count number after check in CPU successfully.
+ Nonpositive value (<=0): check in CPU failure.
+
+**/
+INT32
+EFIAPI
+SmmCpuSyncCheckInCpu (
+ IN VOID *SmmCpuSyncCtx,
+ IN UINTN CpuIndex
+ );
+
+/**
+ Performs an atomic operation to check out CPU.
+ Check out CPU successfully if the returned arrival CPU count value is
+ nonnegative, otherwise indicate the door has been locked, the CPU can
+ not checkout.
+
+ @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object to be released.
+ @param[in] CpuIndex Pointer to the Cpu Index to checkout.
+
+ @return Nonnegative value (>=0): CPU arrival count number after check out CPU successfully.
+ Negative value (<0): Check out CPU failure.
+
+
+**/
+INT32
+EFIAPI
+SmmCpuSyncCheckOutCpu (
+ IN VOID *SmmCpuSyncCtx,
+ IN UINTN CpuIndex
+ );
+
+/**
+ Performs an atomic operation lock door for CPU checkin or checkout.
+ With this function, CPU can not check in via SmmCpuSyncCheckInCpu () or
+ check out via SmmCpuSyncCheckOutCpu ().
+
+ @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object to be released.
+ @param[in] CpuIndex Pointer to the Cpu Index to lock door.
+
+ @return CPU arrival count number.
+
+**/
+UINT32
+EFIAPI
+SmmCpuSyncLockDoor (
+ IN VOID *SmmCpuSyncCtx,
+ IN UINTN CpuIndex
+ );
+
+/**
+ Used for BSP to wait all APs.
+
+ @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object.
+ @param[in] NumberOfAPs Number of APs need to wait.
+ @param[in] BspIndex Pointer to the BSP Index.
+
+**/
+VOID
+EFIAPI
+SmmCpuSyncWaitForAllAPs (
+ IN VOID *SmmCpuSyncCtx,
+ IN UINTN NumberOfAPs,
+ IN UINTN BspIndex
+ );
+
+/**
+ Used for BSP to release one AP.
+
+ @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object.
+ @param[in] CpuIndex Pointer to the Cpu Index, indicate which AP need to be released.
+ @param[in] BspIndex Pointer to the BSP Index.
+
+**/
+VOID
+EFIAPI
+SmmCpuSyncReleaseOneAp (
+ IN VOID *SmmCpuSyncCtx,
+ IN UINTN CpuIndex,
+ IN UINTN BspIndex
+ );
+
+/**
+ Used for AP to wait BSP.
+
+ @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object.
+ @param[in] CpuIndex Pointer to the Cpu Index, indicate which AP wait BSP.
+ @param[in] BspIndex Pointer to the BSP Index.
+
+**/
+VOID
+EFIAPI
+SmmCpuSyncWaitForBsp (
+ IN VOID *SmmCpuSyncCtx,
+ IN UINTN CpuIndex,
+ IN UINTN BspIndex
+ );
+
+/**
+ Used for AP to release BSP.
+
+ @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object.
+ @param[in] CpuIndex Pointer to the Cpu Index, indicate which AP release BSP.
+ @param[in] BspIndex Pointer to the BSP Index.
+
+**/
+VOID
+EFIAPI
+SmmCpuSyncReleaseBsp (
+ IN VOID *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 (#110639): https://edk2.groups.io/g/devel/message/110639
Mute This Topic: https://groups.io/mt/102366300/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] 21+ messages in thread* Re: [edk2-devel] [PATCH v1 3/7] UefiCpuPkg: Adds SmmCpuSyncLib library class
2023-11-03 15:30 ` [edk2-devel] [PATCH v1 3/7] UefiCpuPkg: Adds SmmCpuSyncLib library class Wu, Jiaxin
@ 2023-11-07 10:26 ` Laszlo Ersek
2023-11-07 10:29 ` Laszlo Ersek
2023-11-13 3:15 ` Ni, Ray
0 siblings, 2 replies; 21+ messages in thread
From: Laszlo Ersek @ 2023-11-07 10:26 UTC (permalink / raw)
To: devel, jiaxin.wu; +Cc: Eric Dong, Ray Ni, Zeng Star, Gerd Hoffmann, Rahul Kumar
On 11/3/23 16:30, Wu, Jiaxin wrote:
> 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.
>
> Change-Id: Ib7f5e317526e8b9e29b65e072bdb485dbd678817
> 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 | 191 +++++++++++++++++++++++++++++
> UefiCpuPkg/UefiCpuPkg.dec | 3 +
> 2 files changed, 194 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..b9b190c516
> --- /dev/null
> +++ b/UefiCpuPkg/Include/Library/SmmCpuSyncLib.h
> @@ -0,0 +1,191 @@
> +/** @file
> +Library that provides SMM CPU Sync related operations.
> +
> +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>
> +
> +/**
> + Creates and Init a new Smm Cpu Sync context.
> +
> + @param[in] NumberOfCpus The number of processors in the system.
> +
> + @return Pointer to an allocated Smm Cpu Sync context object.
> + If the creation failed, returns NULL.
> +
> +**/
> +VOID *
> +EFIAPI
> +SmmCpuSyncContextInit (
> + IN UINTN NumberOfCpus
> + );
(1) I agree that the internals of the context should be hidden, but
(VOID*) is not the right way. Instead, please use an incomplete
structure type.
That is, in the library header class file, do the following:
typedef struct SMM_CPU_SYNC_CTX SMM_CPU_SYNC_CTX;
and then make all these functions return and take
SMM_CPU_SYNC_CTX *
The library users will still not know what is inside SMM_CPU_SYNC_CTX,
just like with (VOID*), but the compiler will be able to perform much
stronger type checking than with (VOID*).
And then in the library *instance(s)*, you can complete the incomplete type:
struct SMM_CPU_SYNC_CTX {
...
};
> +
> +/**
> + Deinit an allocated Smm Cpu Sync context object.
> +
> + @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object to be released.
> +
> +**/
> +VOID
> +EFIAPI
> +SmmCpuSyncContextDeinit (
> + IN VOID *SmmCpuSyncCtx
> + );
> +
> +/**
> + Reset Smm Cpu Sync context object.
> +
> + @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object to be released.
> +
> +**/
> +VOID
> +EFIAPI
> +SmmCpuSyncContextReset (
> + IN VOID *SmmCpuSyncCtx
> + );
> +
(2) The documentation on these two functions (deinit and reset) is
contradictory. Both say "object to be released". That cannot be right.
I'm sure one of these functions just internally resets the object, but
doesn't actually *free* the SMRAM; while the other function releases the
SMRAM too. So please clean up the comments.
(3) By definition, in a function that resets the internals of an object,
you cannot specify "IN" for that function. It must be OUT.
> +/**
> + Get current arrived CPU count.
> +
> + @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object to be released.
(4) Bogus comment.
> +
> + @return Current number of arrived CPU count.
> + -1: indicate the door has been locked.
(5) The return type is UINT32, so -1 is somewhat confusing. Either write
MAX_UINT32 or (UINT32)-1, for clarity, please.
(6) More importantly -- what is "arrived CPU count", and what is "door
has been locked"?
Please add a large comment to the top of the library class header that
explains the operation / concepts of the context. What operations and
behaviors are defined for this data type?
> +
> +**/
> +UINT32
> +EFIAPI
> +SmmCpuSyncGetArrivedCpuCount (
> + IN VOID *SmmCpuSyncCtx
> + );
> +
> +/**
> + Performs an atomic operation to check in CPU.
> + Check in CPU successfully if the returned arrival CPU count value is
> + positive, otherwise indicate the door has been locked, the CPU can
> + not checkin.
I hope this will be understandable after you explain the data type
comprehensively.
> +
> + @param[in] SmmCpuSyncCtx Pointer to the Smm CPU Sync context object to be released.
(7) bogus comment
> + @param[in] CpuIndex Pointer to the CPU Index to checkin.
(8) This parameter is not a pointer. :/
> +
> + @return Positive value (>0): CPU arrival count number after check in CPU successfully.
> + Nonpositive value (<=0): check in CPU failure.
(9) To be honest, I don't like how inconsistent these return types and
values are.
- in SmmCpuSyncContextInit(), you return NULL on failure.
- in SmmCpuSyncGetArrivedCpuCount(), you return MAX_UINT32 on failure;
and on success, you can use return values larger than MAX_INT32.
- in SmmCpuSyncCheckInCpu() below, you return -1 on failure; and on
success, you *cannot* use return values larger than MAX_INT32, for
representing the arrived CPU count.
Please clean up this mess.
All functions should have RETURN_STATUS for return type, and all results
should be produced via OUT paramaters.
(For example, the Init() function can fail for two reasons, minimally:
(a) the allocated memory almost certainly depends on the NumberOfCpus
parameter, so you'll have a multiplication there, and all
multiplications must be checked against integer overflow, (b) if the
multiplication succeeds, you can still run out of SMRAM. It's nice to
return different error codes for (a) and (b))
Furthermore, counts of objects should be generally represented as UINTN
values, unless you have a good (explained!) reason for using a different
type.
> +
> +**/
> +INT32
> +EFIAPI
> +SmmCpuSyncCheckInCpu (
> + IN VOID *SmmCpuSyncCtx,
(10) "IN" is bogus here; when this function succeeds, it obviously
changes the internals of context. Make it "IN OUT" perhaps. (Don't
forget to update the corresponding @param documentation either!)
> + IN UINTN CpuIndex
> + );
> +
> +/**
> + Performs an atomic operation to check out CPU.
> + Check out CPU successfully if the returned arrival CPU count value is
> + nonnegative, otherwise indicate the door has been locked, the CPU can
> + not checkout.
> +
> + @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object to be released.
> + @param[in] CpuIndex Pointer to the Cpu Index to checkout.
> +
> + @return Nonnegative value (>=0): CPU arrival count number after check out CPU successfully.
> + Negative value (<0): Check out CPU failure.
> +
> +
> +**/
> +INT32
> +EFIAPI
> +SmmCpuSyncCheckOutCpu (
> + IN VOID *SmmCpuSyncCtx,
> + IN UINTN CpuIndex
> + );
(11) Remarks (7) through (10) apply here as well.
> +
> +/**
> + Performs an atomic operation lock door for CPU checkin or checkout.
> + With this function, CPU can not check in via SmmCpuSyncCheckInCpu () or
> + check out via SmmCpuSyncCheckOutCpu ().
> +
> + @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object to be released.
(12) bogus comment
> + @param[in] CpuIndex Pointer to the Cpu Index to lock door.
(13) impossible to make any sense of, without the comprehensive data
type description
> +
> + @return CPU arrival count number.
(14) why is it necessary for outputting the arrived number when locking?
> +
> +**/
> +UINT32
> +EFIAPI
> +SmmCpuSyncLockDoor (
> + IN VOID *SmmCpuSyncCtx,
(15) should be IN OUT (in the @param desc too)
> + IN UINTN CpuIndex
> + );
> +
> +/**
> + Used for BSP to wait all APs.
(16) This is a new library class, let's try to make it grammatically
correct and understandable. "Used by the BSP to wait for all the APs".
> +
> + @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object.
> + @param[in] NumberOfAPs Number of APs need to wait.
(17) "Number of APs to wait for". The current explanation suggests we
are making some APs wait.
> + @param[in] BspIndex Pointer to the BSP Index.
(18) Not a pointer.
> +
> +**/
> +VOID
> +EFIAPI
> +SmmCpuSyncWaitForAllAPs (
> + IN VOID *SmmCpuSyncCtx,
(19) Almost certainly IN OUT (-> @param too)
> + IN UINTN NumberOfAPs,
> + IN UINTN BspIndex
> + );
> +
> +/**
> + Used for BSP to release one AP.
(20) "used by"
> +
> + @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object.
> + @param[in] CpuIndex Pointer to the Cpu Index, indicate which AP need to be released.
> + @param[in] BspIndex Pointer to the BSP Index.
(21) neither index is a pointer :/
> +
> +**/
> +VOID
> +EFIAPI
> +SmmCpuSyncReleaseOneAp (
> + IN VOID *SmmCpuSyncCtx,
(22) IN OUT etc
sorry I'm exhausted; this is shoddy code. Please take much more care and
invest more time in the details next time.
Laszlo
> + IN UINTN CpuIndex,
> + IN UINTN BspIndex
> + );
> +
> +/**
> + Used for AP to wait BSP.
> +
> + @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object.
> + @param[in] CpuIndex Pointer to the Cpu Index, indicate which AP wait BSP.
> + @param[in] BspIndex Pointer to the BSP Index.
> +
> +**/
> +VOID
> +EFIAPI
> +SmmCpuSyncWaitForBsp (
> + IN VOID *SmmCpuSyncCtx,
> + IN UINTN CpuIndex,
> + IN UINTN BspIndex
> + );
> +
> +/**
> + Used for AP to release BSP.
> +
> + @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object.
> + @param[in] CpuIndex Pointer to the Cpu Index, indicate which AP release BSP.
> + @param[in] BspIndex Pointer to the BSP Index.
> +
> +**/
> +VOID
> +EFIAPI
> +SmmCpuSyncReleaseBsp (
> + IN VOID *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
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110834): https://edk2.groups.io/g/devel/message/110834
Mute This Topic: https://groups.io/mt/102366300/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] 21+ messages in thread* Re: [edk2-devel] [PATCH v1 3/7] UefiCpuPkg: Adds SmmCpuSyncLib library class
2023-11-07 10:26 ` Laszlo Ersek
@ 2023-11-07 10:29 ` Laszlo Ersek
2023-11-13 3:15 ` Ni, Ray
1 sibling, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2023-11-07 10:29 UTC (permalink / raw)
To: devel, jiaxin.wu; +Cc: Eric Dong, Ray Ni, Zeng Star, Gerd Hoffmann, Rahul Kumar
On 11/7/23 11:26, Laszlo Ersek wrote:
> Furthermore, counts of objects should be generally represented as UINTN
> values, unless you have a good (explained!) reason for using a different
> type.
If the argument is that we need atomics for manipulating CPU counts, and
that we don't want to assume wider-than-32-bit atomics, then that is a
*valid* argument, but it absolutely must be documented (and then *all*
CPU counts in the library class header must be UINT32 *consistently*;
not mixing UINT32 with UINTN with INT32).
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110837): https://edk2.groups.io/g/devel/message/110837
Mute This Topic: https://groups.io/mt/102366300/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] 21+ messages in thread
* Re: [edk2-devel] [PATCH v1 3/7] UefiCpuPkg: Adds SmmCpuSyncLib library class
2023-11-07 10:26 ` Laszlo Ersek
2023-11-07 10:29 ` Laszlo Ersek
@ 2023-11-13 3:15 ` Ni, Ray
2023-11-13 10:43 ` Laszlo Ersek
1 sibling, 1 reply; 21+ messages in thread
From: Ni, Ray @ 2023-11-13 3:15 UTC (permalink / raw)
To: Laszlo Ersek, devel@edk2.groups.io, Wu, Jiaxin
Cc: Dong, Eric, Zeng, Star, Gerd Hoffmann, Kumar, Rahul R
[-- Attachment #1: Type: text/plain, Size: 2935 bytes --]
I provided 4 comments in below, starting with "[Ray".
Sorry for the poor new Outlook that I am not able to put ">" in the original email.
Thanks,
Ray
________________________________
(1) I agree that the internals of the context should be hidden, but
(VOID*) is not the right way. Instead, please use an incomplete
structure type.
That is, in the library header class file, do the following:
typedef struct SMM_CPU_SYNC_CTX SMM_CPU_SYNC_CTX;
and then make all these functions return and take
SMM_CPU_SYNC_CTX *
The library users will still not know what is inside SMM_CPU_SYNC_CTX,
just like with (VOID*), but the compiler will be able to perform much
stronger type checking than with (VOID*).
And then in the library *instance(s)*, you can complete the incomplete type:
struct SMM_CPU_SYNC_CTX {
...
};
[Ray.1] Good suggestion. I still remember you taught me this technique when I
wrote the FrameBufferBltLib.
(3) By definition, in a function that resets the internals of an object,
you cannot specify "IN" for that function. It must be OUT.
[Ray.2] I have a different opinion about IN/OUT. I think we should use "IN OUT".
Please add a large comment to the top of the library class header that
explains the operation / concepts of the context. What operations and
behaviors are defined for this data type?
[Ray.3] Good suggestions.
The lib provides 3 sets of APIs:
1. Init/Deinit
Init() is called in driver's entrypoint for allocating the storage and initialize the content of sync object.
Deinit() is called in driver's unload function for undoing what has been done in Init().
2. CheckInCpu/CheckOutCpu/LockDoor/GetArrivedCpuCount
When SMI happens, all processors including BSP enter to SMM mode by calling CheckInCpu().
The elected BSP calls LockDoor() so that CheckInCpu() after that returns failure code.
CheckOutCpu() is called in error handling flow for the CPU who calls CheckInCpu() earlier.
GetArrivedCpuCount() returns the number of checked-in CPUs.
3. WaitForAllAPs/ReleaseOneAp/WaitForBsp/ReleaseBsp
First 2 APIs are called from BSP.
Latter 2 APIs are called from APs.
The 4 APIs are used to synchronize the running flow among BSP and APs.
> +
> + @return CPU arrival count number.
(14) why is it necessary for outputting the arrived number when locking?
[Ray.4] As I explained above, when BSP locks the door, it might need to know how many CPUs are in the SMM "room".
Maybe today the number is not cared by BSP.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111118): https://edk2.groups.io/g/devel/message/111118
Mute This Topic: https://groups.io/mt/102366300/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
[-- Attachment #2: Type: text/html, Size: 6898 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [edk2-devel] [PATCH v1 3/7] UefiCpuPkg: Adds SmmCpuSyncLib library class
2023-11-13 3:15 ` Ni, Ray
@ 2023-11-13 10:43 ` Laszlo Ersek
0 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2023-11-13 10:43 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io, Wu, Jiaxin
Cc: Dong, Eric, Zeng, Star, Gerd Hoffmann, Kumar, Rahul R
On 11/13/23 04:15, Ni, Ray wrote:
> I provided 4 comments in below, starting with "[Ray".
>
> Sorry for the poor new Outlook that I am not able to put ">" in the
> original email.
>
> Thanks,
> Ray
>
> ------------------------------------------------------------------------
>
> (1) I agree that the internals of the context should be hidden, but
> (VOID*) is not the right way. Instead, please use an incomplete
> structure type.
>
> That is, in the library header class file, do the following:
>
> typedef struct SMM_CPU_SYNC_CTX SMM_CPU_SYNC_CTX;
>
> and then make all these functions return and take
>
> SMM_CPU_SYNC_CTX *
>
> The library users will still not know what is inside SMM_CPU_SYNC_CTX,
> just like with (VOID*), but the compiler will be able to perform much
> stronger type checking than with (VOID*).
>
> And then in the library *instance(s)*, you can complete the incomplete type:
>
> struct SMM_CPU_SYNC_CTX {
> ...
> };
>
> [Ray.1] Good suggestion. I still remember you taught me this technique
> when I
> wrote the FrameBufferBltLib.
>
> (3) By definition, in a function that resets the internals of an object,
> you cannot specify "IN" for that function. It must be OUT.
>
> [Ray.2] I have a different opinion about IN/OUT. I think we should use
> "IN OUT".
OK! I can see that "reset" may rely on previous information in the
structure. Furthermore, "IN OUT" may indeed better reflect the
requirement that the object being reset must have been initialized
previously (i.e. that it must be a valid object at the time of the
"reset" call).
>
>
> Please add a large comment to the top of the library class header that
> explains the operation / concepts of the context. What operations and
> behaviors are defined for this data type?
>
> [Ray.3] Good suggestions.
> The lib provides 3 sets of APIs:
> 1. Init/Deinit
> Init() is called in driver's entrypoint for allocating the storage and
> initialize the content of sync object.
> Deinit() is called in driver's unload function for undoing what has been
> done in Init().
>
> 2. CheckInCpu/CheckOutCpu/LockDoor/GetArrivedCpuCount
> When SMI happens, all processors including BSP enter to SMM mode by
> calling CheckInCpu().
> The elected BSP calls LockDoor() so that CheckInCpu() after that returns
> failure code.
> CheckOutCpu() is called in error handling flow for the CPU who calls
> CheckInCpu() earlier.
> GetArrivedCpuCount() returns the number of checked-in CPUs.
>
> 3. WaitForAllAPs/ReleaseOneAp/WaitForBsp/ReleaseBsp
> First 2 APIs are called from BSP.
> Latter 2 APIs are called from APs.
> The 4 APIs are used to synchronize the running flow among BSP and APs.
This sounds really nice in fact; I'd be glad to see it captured in the
comments!
>
>> +
>> + @return CPU arrival count number.
>
> (14) why is it necessary for outputting the arrived number when locking?
> [Ray.4] As I explained above, when BSP locks the door, it might need to
> know how many CPUs are in the SMM "room".
> Maybe today the number is not cared by BSP.
>
This helps, thanks. Please do capture it in the comments.
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111134): https://edk2.groups.io/g/devel/message/111134
Mute This Topic: https://groups.io/mt/102366300/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] 21+ messages in thread
* [edk2-devel] [PATCH v1 4/7] UefiCpuPkg: Implements SmmCpuSyncLib library instance
2023-11-03 15:30 [edk2-devel] [PATCH v1 0/7] Refine SMM CPU Sync flow and abstract SmmCpuSyncLib Wu, Jiaxin
` (2 preceding siblings ...)
2023-11-03 15:30 ` [edk2-devel] [PATCH v1 3/7] UefiCpuPkg: Adds SmmCpuSyncLib library class Wu, Jiaxin
@ 2023-11-03 15:30 ` Wu, Jiaxin
2023-11-07 10:46 ` Laszlo Ersek
2023-11-07 10:47 ` Laszlo Ersek
2023-11-03 15:30 ` [edk2-devel] [PATCH v1 5/7] OvmfPkg: Specifies SmmCpuSyncLib instance Wu, Jiaxin
` (2 subsequent siblings)
6 siblings, 2 replies; 21+ messages in thread
From: Wu, Jiaxin @ 2023-11-03 15:30 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Zeng Star, Gerd Hoffmann, Rahul Kumar
Implements SmmCpuSyncLib Library class. The instance follows the
existing SMM CPU driver (PiSmmCpuDxeSmm) sync implementation:
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: SmmCpuSyncWaitForAllAPs <-- 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.
Change-Id: I5a004637f8b24a90594a794092548b850b187493
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 | 481 +++++++++++++++++++++
UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.inf | 38 ++
UefiCpuPkg/UefiCpuLibs.dsc.inc | 15 +
UefiCpuPkg/UefiCpuPkg.dsc | 1 +
4 files changed, 535 insertions(+)
create mode 100644 UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.c
create mode 100644 UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.inf
create mode 100644 UefiCpuPkg/UefiCpuLibs.dsc.inc
diff --git a/UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.c b/UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.c
new file mode 100644
index 0000000000..3bc3ebe49a
--- /dev/null
+++ b/UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.c
@@ -0,0 +1,481 @@
+/** @file
+ SMM CPU Sync lib implementation.
+
+ 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/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;
+
+typedef struct {
+ ///
+ /// 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;
+} SMM_CPU_SYNC_CTX;
+
+/**
+ 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
+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 Sem IN: 32-bit unsigned integer
+ OUT: original integer + 1
+
+ @return 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 Sem IN: 32-bit unsigned integer
+ OUT: -1
+
+ @return Original integer
+
+**/
+UINT32
+InternalLockdownSemaphore (
+ IN OUT volatile UINT32 *Sem
+ )
+{
+ UINT32 Value;
+
+ do {
+ Value = *Sem;
+ } while (InterlockedCompareExchange32 (
+ (UINT32 *)Sem,
+ Value,
+ (UINT32)-1
+ ) != Value);
+
+ return Value;
+}
+
+/**
+ Creates and Init a new Smm Cpu Sync context.
+
+ @param[in] NumberOfCpus The number of processors in the system.
+
+ @return Pointer to an allocated Smm Cpu Sync context object.
+ If the creation failed, returns NULL.
+
+**/
+VOID *
+EFIAPI
+SmmCpuSyncContextInit (
+ IN UINTN NumberOfCpus
+ )
+{
+ SMM_CPU_SYNC_CTX *Ctx;
+ UINTN CtxSize;
+ UINTN OneSemSize;
+ UINTN GlobalSemSize;
+ UINTN CpuSemSize;
+ UINTN TotalSemSize;
+ UINTN SemAddr;
+ UINTN CpuIndex;
+
+ Ctx = NULL;
+
+ //
+ // Allocate for the Ctx
+ //
+ CtxSize = sizeof (SMM_CPU_SYNC_CTX) + sizeof (SMM_CPU_SYNC_SEMAPHORE_GLOBAL) + sizeof (SMM_CPU_SYNC_SEMAPHORE_CPU) * NumberOfCpus;
+ Ctx = (SMM_CPU_SYNC_CTX *)AllocatePages (EFI_SIZE_TO_PAGES (CtxSize));
+ ASSERT (Ctx != NULL);
+ Ctx->GlobalSem = (SMM_CPU_SYNC_SEMAPHORE_GLOBAL *)((UINT8 *)Ctx + sizeof (SMM_CPU_SYNC_CTX));
+ Ctx->CpuSem = (SMM_CPU_SYNC_SEMAPHORE_CPU *)((UINT8 *)Ctx + sizeof (SMM_CPU_SYNC_CTX) + sizeof (SMM_CPU_SYNC_SEMAPHORE_GLOBAL));
+ Ctx->NumberOfCpus = NumberOfCpus;
+
+ //
+ // Allocate for Semaphores in the Ctx
+ //
+ OneSemSize = GetSpinLockProperties ();
+ GlobalSemSize = (sizeof (SMM_CPU_SYNC_SEMAPHORE_GLOBAL) / sizeof (VOID *)) * OneSemSize;
+ CpuSemSize = (sizeof (SMM_CPU_SYNC_SEMAPHORE_CPU) / sizeof (VOID *)) * OneSemSize * NumberOfCpus;
+ TotalSemSize = GlobalSemSize + CpuSemSize;
+ DEBUG ((DEBUG_INFO, "[%a] - One Semaphore Size = 0x%x\n", __FUNCTION__, OneSemSize));
+ DEBUG ((DEBUG_INFO, "[%a] - Total Semaphores Size = 0x%x\n", __FUNCTION__, TotalSemSize));
+ Ctx->SemBlockPages = EFI_SIZE_TO_PAGES (TotalSemSize);
+ Ctx->SemBlock = AllocatePages (Ctx->SemBlockPages);
+ ASSERT (Ctx->SemBlock != NULL);
+ ZeroMem (Ctx->SemBlock, TotalSemSize);
+
+ SemAddr = (UINTN)Ctx->SemBlock;
+
+ //
+ // Assign Global Semaphore pointer
+ //
+ Ctx->GlobalSem->Counter = (UINT32 *)SemAddr;
+ *Ctx->GlobalSem->Counter = 0;
+ DEBUG ((DEBUG_INFO, "[%a] - Ctx->GlobalSem->Counter Address: 0x%08x\n", __FUNCTION__, (UINTN)Ctx->GlobalSem->Counter));
+
+ SemAddr += GlobalSemSize;
+
+ //
+ // Assign CPU Semaphore pointer
+ //
+ for (CpuIndex = 0; CpuIndex < NumberOfCpus; CpuIndex++) {
+ Ctx->CpuSem[CpuIndex].Run = (UINT32 *)(SemAddr + (CpuSemSize / NumberOfCpus) * CpuIndex);
+ *Ctx->CpuSem[CpuIndex].Run = 0;
+ DEBUG ((DEBUG_INFO, "[%a] - Ctx->CpuSem[%d].Run Address: 0x%08x\n", __FUNCTION__, CpuIndex, (UINTN)Ctx->CpuSem[CpuIndex].Run));
+ }
+
+ //
+ // Return the new created Smm Cpu Sync context
+ //
+ return (VOID *)Ctx;
+}
+
+/**
+ Deinit an allocated Smm Cpu Sync context object.
+
+ @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object to be released.
+
+**/
+VOID
+EFIAPI
+SmmCpuSyncContextDeinit (
+ IN VOID *SmmCpuSyncCtx
+ )
+{
+ SMM_CPU_SYNC_CTX *Ctx;
+ UINTN CtxSize;
+
+ ASSERT (SmmCpuSyncCtx != NULL);
+ Ctx = (SMM_CPU_SYNC_CTX *)SmmCpuSyncCtx;
+
+ 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));
+}
+
+/**
+ Reset Smm Cpu Sync context object.
+
+ @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object to be released.
+
+**/
+VOID
+EFIAPI
+SmmCpuSyncContextReset (
+ IN VOID *SmmCpuSyncCtx
+ )
+{
+ SMM_CPU_SYNC_CTX *Ctx;
+
+ ASSERT (SmmCpuSyncCtx != NULL);
+ Ctx = (SMM_CPU_SYNC_CTX *)SmmCpuSyncCtx;
+
+ *Ctx->GlobalSem->Counter = 0;
+}
+
+/**
+ Get current arrived CPU count.
+
+ @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object to be released.
+
+ @return Current number of arrived CPU count.
+ -1: indicate the door has been locked.
+
+**/
+UINT32
+EFIAPI
+SmmCpuSyncGetArrivedCpuCount (
+ IN VOID *SmmCpuSyncCtx
+ )
+{
+ SMM_CPU_SYNC_CTX *Ctx;
+
+ ASSERT (SmmCpuSyncCtx != NULL);
+ Ctx = (SMM_CPU_SYNC_CTX *)SmmCpuSyncCtx;
+
+ if (*Ctx->GlobalSem->Counter < 0) {
+ return (UINT32)-1;
+ }
+
+ return *Ctx->GlobalSem->Counter;
+}
+
+/**
+ Performs an atomic operation to check in CPU.
+ Check in CPU successfully if the returned arrival CPU count value is
+ positive, otherwise indicate the door has been locked, the CPU can
+ not checkin.
+
+ @param[in] SmmCpuSyncCtx Pointer to the Smm CPU Sync context object to be released.
+ @param[in] CpuIndex Pointer to the CPU Index to checkin.
+
+ @return Positive value (>0): CPU arrival count number after check in CPU successfully.
+ Nonpositive value (<=0): check in CPU failure.
+
+**/
+INT32
+EFIAPI
+SmmCpuSyncCheckInCpu (
+ IN VOID *SmmCpuSyncCtx,
+ IN UINTN CpuIndex
+ )
+{
+ SMM_CPU_SYNC_CTX *Ctx;
+
+ ASSERT (SmmCpuSyncCtx != NULL);
+ Ctx = (SMM_CPU_SYNC_CTX *)SmmCpuSyncCtx;
+
+ return (INT32)InternalReleaseSemaphore (Ctx->GlobalSem->Counter);
+}
+
+/**
+ Performs an atomic operation to check out CPU.
+ Check out CPU successfully if the returned arrival CPU count value is
+ nonnegative, otherwise indicate the door has been locked, the CPU can
+ not checkout.
+
+ @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object to be released.
+ @param[in] CpuIndex Pointer to the Cpu Index to checkout.
+
+ @return Nonnegative value (>=0): CPU arrival count number after check out CPU successfully.
+ Negative value (<0): Check out CPU failure.
+
+
+**/
+INT32
+EFIAPI
+SmmCpuSyncCheckOutCpu (
+ IN VOID *SmmCpuSyncCtx,
+ IN UINTN CpuIndex
+ )
+{
+ SMM_CPU_SYNC_CTX *Ctx;
+
+ ASSERT (SmmCpuSyncCtx != NULL);
+ Ctx = (SMM_CPU_SYNC_CTX *)SmmCpuSyncCtx;
+
+ return (INT32)InternalWaitForSemaphore (Ctx->GlobalSem->Counter);
+}
+
+/**
+ Performs an atomic operation lock door for CPU checkin or checkout.
+ With this function, CPU can not check in via SmmCpuSyncCheckInCpu () or
+ check out via SmmCpuSyncCheckOutCpu ().
+
+ @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object to be released.
+ @param[in] CpuIndex Pointer to the Cpu Index to lock door.
+
+ @return CPU arrival count number.
+
+**/
+UINT32
+EFIAPI
+SmmCpuSyncLockDoor (
+ IN VOID *SmmCpuSyncCtx,
+ IN UINTN CpuIndex
+ )
+{
+ SMM_CPU_SYNC_CTX *Ctx;
+
+ ASSERT (SmmCpuSyncCtx != NULL);
+ Ctx = (SMM_CPU_SYNC_CTX *)SmmCpuSyncCtx;
+
+ return InternalLockdownSemaphore (Ctx->GlobalSem->Counter);
+}
+
+/**
+ Used for BSP to wait all APs.
+
+ @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object.
+ @param[in] NumberOfAPs Number of APs need to wait.
+ @param[in] BspIndex Pointer to the BSP Index.
+
+**/
+VOID
+EFIAPI
+SmmCpuSyncWaitForAllAPs (
+ IN VOID *SmmCpuSyncCtx,
+ IN UINTN NumberOfAPs,
+ IN UINTN BspIndex
+ )
+{
+ SMM_CPU_SYNC_CTX *Ctx;
+
+ ASSERT (SmmCpuSyncCtx != NULL);
+ Ctx = (SMM_CPU_SYNC_CTX *)SmmCpuSyncCtx;
+
+ while (NumberOfAPs-- > 0) {
+ InternalWaitForSemaphore (Ctx->CpuSem[BspIndex].Run);
+ }
+}
+
+/**
+ Used for BSP to release one AP.
+
+ @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object.
+ @param[in] CpuIndex Pointer to the Cpu Index, indicate which AP need to be released.
+ @param[in] BspIndex Pointer to the BSP Index.
+
+**/
+VOID
+EFIAPI
+SmmCpuSyncReleaseOneAp (
+ IN VOID *SmmCpuSyncCtx,
+ IN UINTN CpuIndex,
+ IN UINTN BspIndex
+ )
+{
+ SMM_CPU_SYNC_CTX *Ctx;
+
+ ASSERT (SmmCpuSyncCtx != NULL);
+ Ctx = (SMM_CPU_SYNC_CTX *)SmmCpuSyncCtx;
+
+ InternalReleaseSemaphore (Ctx->CpuSem[CpuIndex].Run);
+}
+
+/**
+ Used for AP to wait BSP.
+
+ @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object.
+ @param[in] CpuIndex Pointer to the Cpu Index, indicate which AP wait BSP.
+ @param[in] BspIndex Pointer to the BSP Index.
+
+**/
+VOID
+EFIAPI
+SmmCpuSyncWaitForBsp (
+ IN VOID *SmmCpuSyncCtx,
+ IN UINTN CpuIndex,
+ IN UINTN BspIndex
+ )
+{
+ SMM_CPU_SYNC_CTX *Ctx;
+
+ ASSERT (SmmCpuSyncCtx != NULL);
+ Ctx = (SMM_CPU_SYNC_CTX *)SmmCpuSyncCtx;
+
+ InternalWaitForSemaphore (Ctx->CpuSem[CpuIndex].Run);
+}
+
+/**
+ Used for AP to release BSP.
+
+ @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object.
+ @param[in] CpuIndex Pointer to the Cpu Index, indicate which AP release BSP.
+ @param[in] BspIndex Pointer to the BSP Index.
+
+**/
+VOID
+EFIAPI
+SmmCpuSyncReleaseBsp (
+ IN VOID *SmmCpuSyncCtx,
+ IN UINTN CpuIndex,
+ IN UINTN BspIndex
+ )
+{
+ SMM_CPU_SYNC_CTX *Ctx;
+
+ ASSERT (SmmCpuSyncCtx != NULL);
+ Ctx = (SMM_CPU_SYNC_CTX *)SmmCpuSyncCtx;
+
+ InternalReleaseSemaphore (Ctx->CpuSem[BspIndex].Run);
+}
diff --git a/UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.inf b/UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.inf
new file mode 100644
index 0000000000..86475ce64b
--- /dev/null
+++ b/UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.inf
@@ -0,0 +1,38 @@
+## @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
+ SynchronizationLib
+ BaseMemoryLib
+ SmmServicesTableLib
+ MemoryAllocationLib
+
+[Pcd]
+
+[Protocols]
diff --git a/UefiCpuPkg/UefiCpuLibs.dsc.inc b/UefiCpuPkg/UefiCpuLibs.dsc.inc
new file mode 100644
index 0000000000..6b9b362729
--- /dev/null
+++ b/UefiCpuPkg/UefiCpuLibs.dsc.inc
@@ -0,0 +1,15 @@
+## @file
+# UefiCpu DSC include file for [LibraryClasses*] section of all Architectures.
+#
+# This file can be included to the [LibraryClasses*] section(s) of a platform DSC file
+# by using "!include UefiCpuPkg/UefiCpuLibs.dsc.inc" to specify the library instances
+# of some EDKII basic/common library classes in UefiCpuPkg.
+#
+# Copyright (c) 2023, Intel Corporation. All rights reserved.<BR>
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[LibraryClasses]
+ SmmCpuSyncLib|UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.inf
\ No newline at end of file
diff --git a/UefiCpuPkg/UefiCpuPkg.dsc b/UefiCpuPkg/UefiCpuPkg.dsc
index 074fd77461..338f18eb98 100644
--- a/UefiCpuPkg/UefiCpuPkg.dsc
+++ b/UefiCpuPkg/UefiCpuPkg.dsc
@@ -21,10 +21,11 @@
#
# External libraries to build package
#
!include MdePkg/MdeLibs.dsc.inc
+!include UefiCpuPkg/UefiCpuLibs.dsc.inc
[LibraryClasses]
BaseLib|MdePkg/Library/BaseLib/BaseLib.inf
BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
CpuLib|MdePkg/Library/BaseCpuLib/BaseCpuLib.inf
--
2.16.2.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110640): https://edk2.groups.io/g/devel/message/110640
Mute This Topic: https://groups.io/mt/102366301/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] 21+ messages in thread* Re: [edk2-devel] [PATCH v1 4/7] UefiCpuPkg: Implements SmmCpuSyncLib library instance
2023-11-03 15:30 ` [edk2-devel] [PATCH v1 4/7] UefiCpuPkg: Implements SmmCpuSyncLib library instance Wu, Jiaxin
@ 2023-11-07 10:46 ` Laszlo Ersek
2023-11-07 10:47 ` Laszlo Ersek
1 sibling, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2023-11-07 10:46 UTC (permalink / raw)
To: devel, jiaxin.wu; +Cc: Eric Dong, Ray Ni, Zeng Star, Gerd Hoffmann, Rahul Kumar
On 11/3/23 16:30, Wu, Jiaxin wrote:
> Implements SmmCpuSyncLib Library class. The instance follows the
> existing SMM CPU driver (PiSmmCpuDxeSmm) sync implementation:
> 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: SmmCpuSyncWaitForAllAPs <-- 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.
>
> Change-Id: I5a004637f8b24a90594a794092548b850b187493
> 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 | 481 +++++++++++++++++++++
> UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.inf | 38 ++
> UefiCpuPkg/UefiCpuLibs.dsc.inc | 15 +
> UefiCpuPkg/UefiCpuPkg.dsc | 1 +
> 4 files changed, 535 insertions(+)
> create mode 100644 UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.c
> create mode 100644 UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.inf
> create mode 100644 UefiCpuPkg/UefiCpuLibs.dsc.inc
I won't review this library instance in detail until the lib class
header is cleaned up. Just some high-level comments:
- Please use SafeIntLib for all calculations, to prevent overflows. This
means primarily (but not exclusively) memory size calculations, for
allocations
- Implement proper error checking. ASSERT()s are unacceptable for
catching errors, even if the existent drivers do that.
- Semaphore primitives should call CpuDeadLoop() whenever they detect --
and they *should* detect -- counter underflow or overflow.
At the start of my career at Red Hat, one of the bugs I worked on was a
bug in the Xen hypervisor, in SMP initialization. Xen wanted to program
the MTRRs on all processors at the same time (quite funnily, it's the
exact same topic as your patch #2), and it used counters (semaphores,
spinlocks) for BSP-AP synchronization. Unfortunately, it used single
byte, signed counters. The logic worked fine until you only had 128
processors. When you had more processors, like 160 or 192, the SMP
bringup seemingly completed, but then Xen would crash with very weird
symptoms, later. The problem was of course that the BSP and the APs got
out of sync, the MTRR programming was inconsistent (which is undefined
behavior in itself, per Intel SDM), and then the APs were running around
wherever like a loose herd of cats.
Moral of the story: your synchronization primitives are *never
permitted* to fail. If they fail, you must hang *immediately*, to
contain the damage.
Note that the InternalWaitForSemaphore() and InternalReleaseSemaphore()
functions already detect integer overflow to an extent -- they don't
overwrite the counter with the overflowed / underflowed value, and they
properly output the error as well ("release" returning 0 is clearly an
overflow that was caught, and "wait" returning MAX_UINT32 is clearly an
underflow that was caught).
The problem is that these error conditions are never checked by the
callers; what's more, whenever such an error occurs, it's effectively
impossible for the caller to do anything -- it's guaranteed to be a
consequence of a programming error somewhere. So it's best not to litter
the call sites with error checks, but to call CpuDeadLoop() inside the
primitives whenever a semaphore is busted.
I understand that I'm asking for things that differ from the original
code, but every time we abstract something, we should make an honest
effort to clean up and fix existent bugs.
Laszlo
>
> diff --git a/UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.c b/UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.c
> new file mode 100644
> index 0000000000..3bc3ebe49a
> --- /dev/null
> +++ b/UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.c
> @@ -0,0 +1,481 @@
> +/** @file
> + SMM CPU Sync lib implementation.
> +
> + 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/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;
> +
> +typedef struct {
> + ///
> + /// 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;
> +} SMM_CPU_SYNC_CTX;
> +
> +/**
> + 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
> +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 Sem IN: 32-bit unsigned integer
> + OUT: original integer + 1
> +
> + @return 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 Sem IN: 32-bit unsigned integer
> + OUT: -1
> +
> + @return Original integer
> +
> +**/
> +UINT32
> +InternalLockdownSemaphore (
> + IN OUT volatile UINT32 *Sem
> + )
> +{
> + UINT32 Value;
> +
> + do {
> + Value = *Sem;
> + } while (InterlockedCompareExchange32 (
> + (UINT32 *)Sem,
> + Value,
> + (UINT32)-1
> + ) != Value);
> +
> + return Value;
> +}
> +
> +/**
> + Creates and Init a new Smm Cpu Sync context.
> +
> + @param[in] NumberOfCpus The number of processors in the system.
> +
> + @return Pointer to an allocated Smm Cpu Sync context object.
> + If the creation failed, returns NULL.
> +
> +**/
> +VOID *
> +EFIAPI
> +SmmCpuSyncContextInit (
> + IN UINTN NumberOfCpus
> + )
> +{
> + SMM_CPU_SYNC_CTX *Ctx;
> + UINTN CtxSize;
> + UINTN OneSemSize;
> + UINTN GlobalSemSize;
> + UINTN CpuSemSize;
> + UINTN TotalSemSize;
> + UINTN SemAddr;
> + UINTN CpuIndex;
> +
> + Ctx = NULL;
> +
> + //
> + // Allocate for the Ctx
> + //
> + CtxSize = sizeof (SMM_CPU_SYNC_CTX) + sizeof (SMM_CPU_SYNC_SEMAPHORE_GLOBAL) + sizeof (SMM_CPU_SYNC_SEMAPHORE_CPU) * NumberOfCpus;
> + Ctx = (SMM_CPU_SYNC_CTX *)AllocatePages (EFI_SIZE_TO_PAGES (CtxSize));
> + ASSERT (Ctx != NULL);
> + Ctx->GlobalSem = (SMM_CPU_SYNC_SEMAPHORE_GLOBAL *)((UINT8 *)Ctx + sizeof (SMM_CPU_SYNC_CTX));
> + Ctx->CpuSem = (SMM_CPU_SYNC_SEMAPHORE_CPU *)((UINT8 *)Ctx + sizeof (SMM_CPU_SYNC_CTX) + sizeof (SMM_CPU_SYNC_SEMAPHORE_GLOBAL));
> + Ctx->NumberOfCpus = NumberOfCpus;
> +
> + //
> + // Allocate for Semaphores in the Ctx
> + //
> + OneSemSize = GetSpinLockProperties ();
> + GlobalSemSize = (sizeof (SMM_CPU_SYNC_SEMAPHORE_GLOBAL) / sizeof (VOID *)) * OneSemSize;
> + CpuSemSize = (sizeof (SMM_CPU_SYNC_SEMAPHORE_CPU) / sizeof (VOID *)) * OneSemSize * NumberOfCpus;
> + TotalSemSize = GlobalSemSize + CpuSemSize;
> + DEBUG ((DEBUG_INFO, "[%a] - One Semaphore Size = 0x%x\n", __FUNCTION__, OneSemSize));
> + DEBUG ((DEBUG_INFO, "[%a] - Total Semaphores Size = 0x%x\n", __FUNCTION__, TotalSemSize));
> + Ctx->SemBlockPages = EFI_SIZE_TO_PAGES (TotalSemSize);
> + Ctx->SemBlock = AllocatePages (Ctx->SemBlockPages);
> + ASSERT (Ctx->SemBlock != NULL);
> + ZeroMem (Ctx->SemBlock, TotalSemSize);
> +
> + SemAddr = (UINTN)Ctx->SemBlock;
> +
> + //
> + // Assign Global Semaphore pointer
> + //
> + Ctx->GlobalSem->Counter = (UINT32 *)SemAddr;
> + *Ctx->GlobalSem->Counter = 0;
> + DEBUG ((DEBUG_INFO, "[%a] - Ctx->GlobalSem->Counter Address: 0x%08x\n", __FUNCTION__, (UINTN)Ctx->GlobalSem->Counter));
> +
> + SemAddr += GlobalSemSize;
> +
> + //
> + // Assign CPU Semaphore pointer
> + //
> + for (CpuIndex = 0; CpuIndex < NumberOfCpus; CpuIndex++) {
> + Ctx->CpuSem[CpuIndex].Run = (UINT32 *)(SemAddr + (CpuSemSize / NumberOfCpus) * CpuIndex);
> + *Ctx->CpuSem[CpuIndex].Run = 0;
> + DEBUG ((DEBUG_INFO, "[%a] - Ctx->CpuSem[%d].Run Address: 0x%08x\n", __FUNCTION__, CpuIndex, (UINTN)Ctx->CpuSem[CpuIndex].Run));
> + }
> +
> + //
> + // Return the new created Smm Cpu Sync context
> + //
> + return (VOID *)Ctx;
> +}
> +
> +/**
> + Deinit an allocated Smm Cpu Sync context object.
> +
> + @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object to be released.
> +
> +**/
> +VOID
> +EFIAPI
> +SmmCpuSyncContextDeinit (
> + IN VOID *SmmCpuSyncCtx
> + )
> +{
> + SMM_CPU_SYNC_CTX *Ctx;
> + UINTN CtxSize;
> +
> + ASSERT (SmmCpuSyncCtx != NULL);
> + Ctx = (SMM_CPU_SYNC_CTX *)SmmCpuSyncCtx;
> +
> + 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));
> +}
> +
> +/**
> + Reset Smm Cpu Sync context object.
> +
> + @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object to be released.
> +
> +**/
> +VOID
> +EFIAPI
> +SmmCpuSyncContextReset (
> + IN VOID *SmmCpuSyncCtx
> + )
> +{
> + SMM_CPU_SYNC_CTX *Ctx;
> +
> + ASSERT (SmmCpuSyncCtx != NULL);
> + Ctx = (SMM_CPU_SYNC_CTX *)SmmCpuSyncCtx;
> +
> + *Ctx->GlobalSem->Counter = 0;
> +}
> +
> +/**
> + Get current arrived CPU count.
> +
> + @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object to be released.
> +
> + @return Current number of arrived CPU count.
> + -1: indicate the door has been locked.
> +
> +**/
> +UINT32
> +EFIAPI
> +SmmCpuSyncGetArrivedCpuCount (
> + IN VOID *SmmCpuSyncCtx
> + )
> +{
> + SMM_CPU_SYNC_CTX *Ctx;
> +
> + ASSERT (SmmCpuSyncCtx != NULL);
> + Ctx = (SMM_CPU_SYNC_CTX *)SmmCpuSyncCtx;
> +
> + if (*Ctx->GlobalSem->Counter < 0) {
> + return (UINT32)-1;
> + }
> +
> + return *Ctx->GlobalSem->Counter;
> +}
> +
> +/**
> + Performs an atomic operation to check in CPU.
> + Check in CPU successfully if the returned arrival CPU count value is
> + positive, otherwise indicate the door has been locked, the CPU can
> + not checkin.
> +
> + @param[in] SmmCpuSyncCtx Pointer to the Smm CPU Sync context object to be released.
> + @param[in] CpuIndex Pointer to the CPU Index to checkin.
> +
> + @return Positive value (>0): CPU arrival count number after check in CPU successfully.
> + Nonpositive value (<=0): check in CPU failure.
> +
> +**/
> +INT32
> +EFIAPI
> +SmmCpuSyncCheckInCpu (
> + IN VOID *SmmCpuSyncCtx,
> + IN UINTN CpuIndex
> + )
> +{
> + SMM_CPU_SYNC_CTX *Ctx;
> +
> + ASSERT (SmmCpuSyncCtx != NULL);
> + Ctx = (SMM_CPU_SYNC_CTX *)SmmCpuSyncCtx;
> +
> + return (INT32)InternalReleaseSemaphore (Ctx->GlobalSem->Counter);
> +}
> +
> +/**
> + Performs an atomic operation to check out CPU.
> + Check out CPU successfully if the returned arrival CPU count value is
> + nonnegative, otherwise indicate the door has been locked, the CPU can
> + not checkout.
> +
> + @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object to be released.
> + @param[in] CpuIndex Pointer to the Cpu Index to checkout.
> +
> + @return Nonnegative value (>=0): CPU arrival count number after check out CPU successfully.
> + Negative value (<0): Check out CPU failure.
> +
> +
> +**/
> +INT32
> +EFIAPI
> +SmmCpuSyncCheckOutCpu (
> + IN VOID *SmmCpuSyncCtx,
> + IN UINTN CpuIndex
> + )
> +{
> + SMM_CPU_SYNC_CTX *Ctx;
> +
> + ASSERT (SmmCpuSyncCtx != NULL);
> + Ctx = (SMM_CPU_SYNC_CTX *)SmmCpuSyncCtx;
> +
> + return (INT32)InternalWaitForSemaphore (Ctx->GlobalSem->Counter);
> +}
> +
> +/**
> + Performs an atomic operation lock door for CPU checkin or checkout.
> + With this function, CPU can not check in via SmmCpuSyncCheckInCpu () or
> + check out via SmmCpuSyncCheckOutCpu ().
> +
> + @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object to be released.
> + @param[in] CpuIndex Pointer to the Cpu Index to lock door.
> +
> + @return CPU arrival count number.
> +
> +**/
> +UINT32
> +EFIAPI
> +SmmCpuSyncLockDoor (
> + IN VOID *SmmCpuSyncCtx,
> + IN UINTN CpuIndex
> + )
> +{
> + SMM_CPU_SYNC_CTX *Ctx;
> +
> + ASSERT (SmmCpuSyncCtx != NULL);
> + Ctx = (SMM_CPU_SYNC_CTX *)SmmCpuSyncCtx;
> +
> + return InternalLockdownSemaphore (Ctx->GlobalSem->Counter);
> +}
> +
> +/**
> + Used for BSP to wait all APs.
> +
> + @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object.
> + @param[in] NumberOfAPs Number of APs need to wait.
> + @param[in] BspIndex Pointer to the BSP Index.
> +
> +**/
> +VOID
> +EFIAPI
> +SmmCpuSyncWaitForAllAPs (
> + IN VOID *SmmCpuSyncCtx,
> + IN UINTN NumberOfAPs,
> + IN UINTN BspIndex
> + )
> +{
> + SMM_CPU_SYNC_CTX *Ctx;
> +
> + ASSERT (SmmCpuSyncCtx != NULL);
> + Ctx = (SMM_CPU_SYNC_CTX *)SmmCpuSyncCtx;
> +
> + while (NumberOfAPs-- > 0) {
> + InternalWaitForSemaphore (Ctx->CpuSem[BspIndex].Run);
> + }
> +}
> +
> +/**
> + Used for BSP to release one AP.
> +
> + @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object.
> + @param[in] CpuIndex Pointer to the Cpu Index, indicate which AP need to be released.
> + @param[in] BspIndex Pointer to the BSP Index.
> +
> +**/
> +VOID
> +EFIAPI
> +SmmCpuSyncReleaseOneAp (
> + IN VOID *SmmCpuSyncCtx,
> + IN UINTN CpuIndex,
> + IN UINTN BspIndex
> + )
> +{
> + SMM_CPU_SYNC_CTX *Ctx;
> +
> + ASSERT (SmmCpuSyncCtx != NULL);
> + Ctx = (SMM_CPU_SYNC_CTX *)SmmCpuSyncCtx;
> +
> + InternalReleaseSemaphore (Ctx->CpuSem[CpuIndex].Run);
> +}
> +
> +/**
> + Used for AP to wait BSP.
> +
> + @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object.
> + @param[in] CpuIndex Pointer to the Cpu Index, indicate which AP wait BSP.
> + @param[in] BspIndex Pointer to the BSP Index.
> +
> +**/
> +VOID
> +EFIAPI
> +SmmCpuSyncWaitForBsp (
> + IN VOID *SmmCpuSyncCtx,
> + IN UINTN CpuIndex,
> + IN UINTN BspIndex
> + )
> +{
> + SMM_CPU_SYNC_CTX *Ctx;
> +
> + ASSERT (SmmCpuSyncCtx != NULL);
> + Ctx = (SMM_CPU_SYNC_CTX *)SmmCpuSyncCtx;
> +
> + InternalWaitForSemaphore (Ctx->CpuSem[CpuIndex].Run);
> +}
> +
> +/**
> + Used for AP to release BSP.
> +
> + @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object.
> + @param[in] CpuIndex Pointer to the Cpu Index, indicate which AP release BSP.
> + @param[in] BspIndex Pointer to the BSP Index.
> +
> +**/
> +VOID
> +EFIAPI
> +SmmCpuSyncReleaseBsp (
> + IN VOID *SmmCpuSyncCtx,
> + IN UINTN CpuIndex,
> + IN UINTN BspIndex
> + )
> +{
> + SMM_CPU_SYNC_CTX *Ctx;
> +
> + ASSERT (SmmCpuSyncCtx != NULL);
> + Ctx = (SMM_CPU_SYNC_CTX *)SmmCpuSyncCtx;
> +
> + InternalReleaseSemaphore (Ctx->CpuSem[BspIndex].Run);
> +}
> diff --git a/UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.inf b/UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.inf
> new file mode 100644
> index 0000000000..86475ce64b
> --- /dev/null
> +++ b/UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.inf
> @@ -0,0 +1,38 @@
> +## @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
> + SynchronizationLib
> + BaseMemoryLib
> + SmmServicesTableLib
> + MemoryAllocationLib
> +
> +[Pcd]
> +
> +[Protocols]
> diff --git a/UefiCpuPkg/UefiCpuLibs.dsc.inc b/UefiCpuPkg/UefiCpuLibs.dsc.inc
> new file mode 100644
> index 0000000000..6b9b362729
> --- /dev/null
> +++ b/UefiCpuPkg/UefiCpuLibs.dsc.inc
> @@ -0,0 +1,15 @@
> +## @file
> +# UefiCpu DSC include file for [LibraryClasses*] section of all Architectures.
> +#
> +# This file can be included to the [LibraryClasses*] section(s) of a platform DSC file
> +# by using "!include UefiCpuPkg/UefiCpuLibs.dsc.inc" to specify the library instances
> +# of some EDKII basic/common library classes in UefiCpuPkg.
> +#
> +# Copyright (c) 2023, Intel Corporation. All rights reserved.<BR>
> +#
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[LibraryClasses]
> + SmmCpuSyncLib|UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.inf
> \ No newline at end of file
> diff --git a/UefiCpuPkg/UefiCpuPkg.dsc b/UefiCpuPkg/UefiCpuPkg.dsc
> index 074fd77461..338f18eb98 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dsc
> +++ b/UefiCpuPkg/UefiCpuPkg.dsc
> @@ -21,10 +21,11 @@
> #
> # External libraries to build package
> #
>
> !include MdePkg/MdeLibs.dsc.inc
> +!include UefiCpuPkg/UefiCpuLibs.dsc.inc
>
> [LibraryClasses]
> BaseLib|MdePkg/Library/BaseLib/BaseLib.inf
> BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
> CpuLib|MdePkg/Library/BaseCpuLib/BaseCpuLib.inf
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110838): https://edk2.groups.io/g/devel/message/110838
Mute This Topic: https://groups.io/mt/102366301/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] 21+ messages in thread* Re: [edk2-devel] [PATCH v1 4/7] UefiCpuPkg: Implements SmmCpuSyncLib library instance
2023-11-03 15:30 ` [edk2-devel] [PATCH v1 4/7] UefiCpuPkg: Implements SmmCpuSyncLib library instance Wu, Jiaxin
2023-11-07 10:46 ` Laszlo Ersek
@ 2023-11-07 10:47 ` Laszlo Ersek
1 sibling, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2023-11-07 10:47 UTC (permalink / raw)
To: devel, jiaxin.wu; +Cc: Eric Dong, Ray Ni, Zeng Star, Gerd Hoffmann, Rahul Kumar
On 11/3/23 16:30, Wu, Jiaxin wrote:
> diff --git a/UefiCpuPkg/UefiCpuLibs.dsc.inc b/UefiCpuPkg/UefiCpuLibs.dsc.inc
> new file mode 100644
> index 0000000000..6b9b362729
> --- /dev/null
> +++ b/UefiCpuPkg/UefiCpuLibs.dsc.inc
> @@ -0,0 +1,15 @@
> +## @file
> +# UefiCpu DSC include file for [LibraryClasses*] section of all Architectures.
> +#
> +# This file can be included to the [LibraryClasses*] section(s) of a platform DSC file
> +# by using "!include UefiCpuPkg/UefiCpuLibs.dsc.inc" to specify the library instances
> +# of some EDKII basic/common library classes in UefiCpuPkg.
> +#
> +# Copyright (c) 2023, Intel Corporation. All rights reserved.<BR>
> +#
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[LibraryClasses]
> + SmmCpuSyncLib|UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.inf
> \ No newline at end of file
Missing newline at EOF...
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110839): https://edk2.groups.io/g/devel/message/110839
Mute This Topic: https://groups.io/mt/102366301/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] 21+ messages in thread
* [edk2-devel] [PATCH v1 5/7] OvmfPkg: Specifies SmmCpuSyncLib instance
2023-11-03 15:30 [edk2-devel] [PATCH v1 0/7] Refine SMM CPU Sync flow and abstract SmmCpuSyncLib Wu, Jiaxin
` (3 preceding siblings ...)
2023-11-03 15:30 ` [edk2-devel] [PATCH v1 4/7] UefiCpuPkg: Implements SmmCpuSyncLib library instance Wu, Jiaxin
@ 2023-11-03 15:30 ` Wu, Jiaxin
2023-11-07 10:59 ` Laszlo Ersek
2023-11-03 15:30 ` [edk2-devel] [PATCH v1 6/7] UefiPayloadPkg: " Wu, Jiaxin
2023-11-03 15:30 ` [edk2-devel] [PATCH v1 7/7] UefiCpuPkg/PiSmmCpuDxeSmm: Consume SmmCpuSyncLib Wu, Jiaxin
6 siblings, 1 reply; 21+ messages in thread
From: Wu, Jiaxin @ 2023-11-03 15:30 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Jiewen Yao, Jordan Justen, Eric Dong, Ray Ni,
Zeng Star, Rahul Kumar, Gerd Hoffmann
The SmmCpuSyncLib instance is included in UefiCpuLibs.dsc.inc.
This patch is to specify SmmCpuSyncLib instance in OvmfPkg by
using "!include UefiCpuPkg/UefiCpuLibs.dsc.inc".
Change-Id: I2ab1737425e26a7bfc4f564b3b7f15ca5c2268fb
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 | 1 +
OvmfPkg/OvmfPkgIa32.dsc | 1 +
OvmfPkg/OvmfPkgIa32X64.dsc | 1 +
OvmfPkg/OvmfPkgX64.dsc | 1 +
4 files changed, 4 insertions(+)
diff --git a/OvmfPkg/CloudHv/CloudHvX64.dsc b/OvmfPkg/CloudHv/CloudHvX64.dsc
index c23c7eaf6c..e65767fe16 100644
--- a/OvmfPkg/CloudHv/CloudHvX64.dsc
+++ b/OvmfPkg/CloudHv/CloudHvX64.dsc
@@ -122,10 +122,11 @@
# Library Class section - list of all Library Classes needed by this Platform.
#
################################################################################
!include MdePkg/MdeLibs.dsc.inc
+!include UefiCpuPkg/UefiCpuLibs.dsc.inc
[LibraryClasses]
PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf
ResetSystemLib|OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index ed3a19feeb..07d16e6935 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -125,10 +125,11 @@
# Library Class section - list of all Library Classes needed by this Platform.
#
################################################################################
!include MdePkg/MdeLibs.dsc.inc
+!include UefiCpuPkg/UefiCpuLibs.dsc.inc
[LibraryClasses]
PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf
ResetSystemLib|OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 16ca139b29..8d243b7075 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -130,10 +130,11 @@
# Library Class section - list of all Library Classes needed by this Platform.
#
################################################################################
!include MdePkg/MdeLibs.dsc.inc
+!include UefiCpuPkg/UefiCpuLibs.dsc.inc
[LibraryClasses]
PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf
ResetSystemLib|OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index dc1a0942aa..6343789152 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -143,10 +143,11 @@
# Library Class section - list of all Library Classes needed by this Platform.
#
################################################################################
!include MdePkg/MdeLibs.dsc.inc
+!include UefiCpuPkg/UefiCpuLibs.dsc.inc
[LibraryClasses]
PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf
ResetSystemLib|OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
--
2.16.2.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110641): https://edk2.groups.io/g/devel/message/110641
Mute This Topic: https://groups.io/mt/102366302/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] 21+ messages in thread* Re: [edk2-devel] [PATCH v1 5/7] OvmfPkg: Specifies SmmCpuSyncLib instance
2023-11-03 15:30 ` [edk2-devel] [PATCH v1 5/7] OvmfPkg: Specifies SmmCpuSyncLib instance Wu, Jiaxin
@ 2023-11-07 10:59 ` Laszlo Ersek
0 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2023-11-07 10:59 UTC (permalink / raw)
To: devel, jiaxin.wu
Cc: Ard Biesheuvel, Jiewen Yao, Jordan Justen, Eric Dong, Ray Ni,
Zeng Star, Rahul Kumar, Gerd Hoffmann
On 11/3/23 16:30, Wu, Jiaxin wrote:
> The SmmCpuSyncLib instance is included in UefiCpuLibs.dsc.inc.
> This patch is to specify SmmCpuSyncLib instance in OvmfPkg by
> using "!include UefiCpuPkg/UefiCpuLibs.dsc.inc".
>
> Change-Id: I2ab1737425e26a7bfc4f564b3b7f15ca5c2268fb
> 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 | 1 +
> OvmfPkg/OvmfPkgIa32.dsc | 1 +
> OvmfPkg/OvmfPkgIa32X64.dsc | 1 +
> OvmfPkg/OvmfPkgX64.dsc | 1 +
> 4 files changed, 4 insertions(+)
>
> diff --git a/OvmfPkg/CloudHv/CloudHvX64.dsc b/OvmfPkg/CloudHv/CloudHvX64.dsc
> index c23c7eaf6c..e65767fe16 100644
> --- a/OvmfPkg/CloudHv/CloudHvX64.dsc
> +++ b/OvmfPkg/CloudHv/CloudHvX64.dsc
> @@ -122,10 +122,11 @@
> # Library Class section - list of all Library Classes needed by this Platform.
> #
> ################################################################################
>
> !include MdePkg/MdeLibs.dsc.inc
> +!include UefiCpuPkg/UefiCpuLibs.dsc.inc
>
> [LibraryClasses]
> PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
> TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf
> ResetSystemLib|OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index ed3a19feeb..07d16e6935 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -125,10 +125,11 @@
> # Library Class section - list of all Library Classes needed by this Platform.
> #
> ################################################################################
>
> !include MdePkg/MdeLibs.dsc.inc
> +!include UefiCpuPkg/UefiCpuLibs.dsc.inc
>
> [LibraryClasses]
> PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
> TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf
> ResetSystemLib|OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 16ca139b29..8d243b7075 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -130,10 +130,11 @@
> # Library Class section - list of all Library Classes needed by this Platform.
> #
> ################################################################################
>
> !include MdePkg/MdeLibs.dsc.inc
> +!include UefiCpuPkg/UefiCpuLibs.dsc.inc
>
> [LibraryClasses]
> PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
> TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf
> ResetSystemLib|OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index dc1a0942aa..6343789152 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -143,10 +143,11 @@
> # Library Class section - list of all Library Classes needed by this Platform.
> #
> ################################################################################
>
> !include MdePkg/MdeLibs.dsc.inc
> +!include UefiCpuPkg/UefiCpuLibs.dsc.inc
>
> [LibraryClasses]
> PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
> TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf
> ResetSystemLib|OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
I am undecided about this patch.
More precisely, I'm undecided that this is the right way to introduce
"UefiCpuLibs.dsc.inc".
In my opinion,"UefiCpuLibs.dsc.inc" should be a general-purpose include
file.
But, at this moment, the include file only resolves the "SmmCpuSyncLib"
class -- and that resolution is useless for any IA32/X64 platform that
does not use the SMM machinery. In particular, the resolution is useless
even for OVMF X64 (for example) if it is built without SMM_REQUIRE.
Furthermore, OVMF X64 uses a bunch of other UefiCpuPkg libraries:
BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
CcExitLibNull/CcExitLibNull.inf
CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
CpuPageTableLib/CpuPageTableLib.inf
MicrocodeLib/MicrocodeLib.inf
MmSaveStateLib/AmdMmSaveStateLib.inf
MpInitLib/DxeMpInitLib.inf
MpInitLib/PeiMpInitLib.inf
MpInitLibUp/MpInitLibUp.inf
MtrrLib/MtrrLib.inf
SmmCpuRendezvousLib/SmmCpuRendezvousLib.inf
It's weird to put SmmCpuSyncLib in the new DSC include file, but not
CpuPageTableLib or MtrrLib (for example), which seem much more generic.
For now I'd suggest avoiding "UefiCpuLibs.dsc.inc" altogether, and
resolving just "SmmCpuSyncLib" in the OVMF DSC files -- and only when
SMM_REQUIRE is defined on the build command line (i.e., when
PiSmmCpuDxeSmm is built into the firmware platform).
We can certainly introduce "UefiCpuLibs.dsc.inc" later, but for that:
- we need to collect all UefiCpuPkg library classes and instances that
are commonly used (and then probably use multiple module type
sections!). Example: "OvmfPkg/Include/Dsc/OvmfTpmLibs.dsc.inc"
- some of those resolutions should be conditional, and so we'd have to
properly prefix macro names like SMM_REQUIRE with something like
UEFI_CPU_. Basically introduce a namespace for those build time macros.
Example: "NetworkPkg/NetworkDefines.dsc.inc", which uses the NETWORK_
macro prefix for enabling various features.
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110840): https://edk2.groups.io/g/devel/message/110840
Mute This Topic: https://groups.io/mt/102366302/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] 21+ messages in thread
* [edk2-devel] [PATCH v1 6/7] UefiPayloadPkg: Specifies SmmCpuSyncLib instance
2023-11-03 15:30 [edk2-devel] [PATCH v1 0/7] Refine SMM CPU Sync flow and abstract SmmCpuSyncLib Wu, Jiaxin
` (4 preceding siblings ...)
2023-11-03 15:30 ` [edk2-devel] [PATCH v1 5/7] OvmfPkg: Specifies SmmCpuSyncLib instance Wu, Jiaxin
@ 2023-11-03 15:30 ` Wu, Jiaxin
2023-11-06 1:11 ` Guo, Gua
2023-11-03 15:30 ` [edk2-devel] [PATCH v1 7/7] UefiCpuPkg/PiSmmCpuDxeSmm: Consume SmmCpuSyncLib Wu, Jiaxin
6 siblings, 1 reply; 21+ messages in thread
From: Wu, Jiaxin @ 2023-11-03 15:30 UTC (permalink / raw)
To: devel; +Cc: Guo Dong, Sean Rhodes, James Lu, Gua Guo, Ray Ni, Zeng Star
The SmmCpuSyncLib instance is included in UefiCpuLibs.dsc.inc.
This patch is to specify SmmCpuSyncLib instance in UefiPayloadPkg
by using "!include UefiCpuPkg/UefiCpuLibs.dsc.inc".
Change-Id: Ib303a9cdf260ac1ffc146e5f2e68834dec00ff25
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 af9308ef8e..6f6d815c07 100644
--- a/UefiPayloadPkg/UefiPayloadPkg.dsc
+++ b/UefiPayloadPkg/UefiPayloadPkg.dsc
@@ -169,10 +169,11 @@
# Library Class section - list of all Library Classes needed by this Platform.
#
################################################################################
!include MdePkg/MdeLibs.dsc.inc
+!include UefiCpuPkg/UefiCpuLibs.dsc.inc
[LibraryClasses]
#
# Entry point
#
--
2.16.2.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110642): https://edk2.groups.io/g/devel/message/110642
Mute This Topic: https://groups.io/mt/102366304/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] 21+ messages in thread* Re: [edk2-devel] [PATCH v1 6/7] UefiPayloadPkg: Specifies SmmCpuSyncLib instance
2023-11-03 15:30 ` [edk2-devel] [PATCH v1 6/7] UefiPayloadPkg: " Wu, Jiaxin
@ 2023-11-06 1:11 ` Guo, Gua
0 siblings, 0 replies; 21+ messages in thread
From: Guo, Gua @ 2023-11-06 1:11 UTC (permalink / raw)
To: Wu, Jiaxin, devel@edk2.groups.io
Cc: 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: Friday, November 3, 2023 11:30 PM
To: devel@edk2.groups.io
Cc: 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 v1 6/7] UefiPayloadPkg: Specifies SmmCpuSyncLib instance
The SmmCpuSyncLib instance is included in UefiCpuLibs.dsc.inc.
This patch is to specify SmmCpuSyncLib instance in UefiPayloadPkg by using "!include UefiCpuPkg/UefiCpuLibs.dsc.inc".
Change-Id: Ib303a9cdf260ac1ffc146e5f2e68834dec00ff25
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 af9308ef8e..6f6d815c07 100644
--- a/UefiPayloadPkg/UefiPayloadPkg.dsc
+++ b/UefiPayloadPkg/UefiPayloadPkg.dsc
@@ -169,10 +169,11 @@
# Library Class section - list of all Library Classes needed by this Platform.
#
################################################################################
!include MdePkg/MdeLibs.dsc.inc
+!include UefiCpuPkg/UefiCpuLibs.dsc.inc
[LibraryClasses]
#
# Entry point
#
--
2.16.2.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110692): https://edk2.groups.io/g/devel/message/110692
Mute This Topic: https://groups.io/mt/102366304/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] 21+ messages in thread
* [edk2-devel] [PATCH v1 7/7] UefiCpuPkg/PiSmmCpuDxeSmm: Consume SmmCpuSyncLib
2023-11-03 15:30 [edk2-devel] [PATCH v1 0/7] Refine SMM CPU Sync flow and abstract SmmCpuSyncLib Wu, Jiaxin
` (5 preceding siblings ...)
2023-11-03 15:30 ` [edk2-devel] [PATCH v1 6/7] UefiPayloadPkg: " Wu, Jiaxin
@ 2023-11-03 15:30 ` Wu, Jiaxin
2023-11-07 11:00 ` Laszlo Ersek
6 siblings, 1 reply; 21+ messages in thread
From: Wu, Jiaxin @ 2023-11-03 15:30 UTC (permalink / raw)
To: devel; +Cc: 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.
Change-Id: Id034de47b85743c125f0d76420947e0dd9e69518
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 | 256 +++++----------------------
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 6 +-
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 1 +
3 files changed, 49 insertions(+), 214 deletions(-)
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index 5a42a5dd12..a30b2aa234 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 0
-**/
-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]
@@ -292,35 +138,35 @@ AllCpusInSmmExceptBlockedDisabled (
BlockedCount = 0;
DisabledCount = 0;
//
- // 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 (SmmCpuSyncGetArrivedCpuCount (mSmmMpSyncData->SmmCpuSyncCtx) <= mNumberOfCpus);
//
// Check whether all CPUs in SMM.
//
- if (*mSmmMpSyncData->Counter == mNumberOfCpus) {
+ if (SmmCpuSyncGetArrivedCpuCount (mSmmMpSyncData->SmmCpuSyncCtx) == 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 (SmmCpuSyncGetArrivedCpuCount (mSmmMpSyncData->SmmCpuSyncCtx) + BlockedCount + DisabledCount >= mNumberOfCpus) {
return TRUE;
}
return FALSE;
}
@@ -396,11 +242,11 @@ SmmWaitForApArrival (
PERF_FUNCTION_BEGIN ();
DelayedCount = 0;
BlockedCount = 0;
- ASSERT (*mSmmMpSyncData->Counter <= mNumberOfCpus);
+ ASSERT (SmmCpuSyncGetArrivedCpuCount (mSmmMpSyncData->SmmCpuSyncCtx) <= mNumberOfCpus);
LmceEn = FALSE;
LmceSignal = FALSE;
if (mMachineCheckSupported) {
LmceEn = IsLmceOsEnabled ();
@@ -447,11 +293,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 (SmmCpuSyncGetArrivedCpuCount (mSmmMpSyncData->SmmCpuSyncCtx) < 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)) {
@@ -548,11 +394,11 @@ WaitForAllAPsNotBusy (
Check whether it is an present AP.
@param CpuIndex The AP index which calls this function.
@retval TRUE It's a present AP.
- @retval TRUE This is not an AP or it is not present.
+ @retval FALSE This is not an AP or it is not present.
**/
BOOLEAN
IsPresentAp (
IN UINTN CpuIndex
@@ -659,30 +505,30 @@ 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;
+ ApCount = SmmCpuSyncLockDoor (mSmmMpSyncData->SmmCpuSyncCtx, CpuIndex) - 1;
//
// Wait for all APs:
// 1. Make sure all Aps have set the Present.
// 2. Get ready for programming MTRRs.
//
- WaitForAllAPs (ApCount);
+ SmmCpuSyncWaitForAllAPs (mSmmMpSyncData->SmmCpuSyncCtx, ApCount, CpuIndex);
if (SmmCpuFeaturesNeedConfigureMtrrs ()) {
//
// Signal all APs it's time for backup MTRRs
//
ReleaseAllAPs ();
//
- // WaitForBsp() may wait for ever if an AP happens to enter SMM at
+ // SmmCpuSyncWaitForBsp() 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.
@@ -690,28 +536,28 @@ BSPHandler (
MtrrGetAllMtrrs (&Mtrrs);
//
// Wait for all APs to complete their MTRR saving
//
- WaitForAllAPs (ApCount);
+ SmmCpuSyncWaitForAllAPs (mSmmMpSyncData->SmmCpuSyncCtx, ApCount, CpuIndex);
//
// Let all processors program SMM MTRRs together
//
ReleaseAllAPs ();
//
- // WaitForBsp() may wait for ever if an AP happens to enter SMM at
+ // SmmCpuSyncWaitForBsp() 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);
+ SmmCpuSyncWaitForAllAPs (mSmmMpSyncData->SmmCpuSyncCtx, ApCount, CpuIndex);
}
}
//
// The BUSY lock is initialized to Acquired state
@@ -743,14 +589,14 @@ 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;
+ ApCount = SmmCpuSyncLockDoor (mSmmMpSyncData->SmmCpuSyncCtx, CpuIndex) - 1;
//
// Make sure all APs have their Present flag set
//
while (TRUE) {
PresentCount = 0;
@@ -774,11 +620,11 @@ BSPHandler (
if (SmmCpuFeaturesNeedConfigureMtrrs ()) {
//
// Wait for all APs to complete their pending tasks
//
- WaitForAllAPs (ApCount);
+ SmmCpuSyncWaitForAllAPs (mSmmMpSyncData->SmmCpuSyncCtx, ApCount, CpuIndex);
//
// Signal APs to restore MTRRs
//
ReleaseAllAPs ();
@@ -790,11 +636,11 @@ BSPHandler (
MtrrSetAllMtrrs (&Mtrrs);
//
// Wait for all APs to complete MTRR programming
//
- WaitForAllAPs (ApCount);
+ SmmCpuSyncWaitForAllAPs (mSmmMpSyncData->SmmCpuSyncCtx, ApCount, CpuIndex);
//
// Signal APs to Reset states/semaphore for this processor
//
ReleaseAllAPs ();
@@ -818,11 +664,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);
+ SmmCpuSyncWaitForAllAPs (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.
@@ -844,11 +690,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 ();
}
@@ -914,21 +760,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;
}
}
//
@@ -946,50 +792,50 @@ APHandler (
//
// Notify BSP of arrival at this point
// 1. Set the Present.
// 2. Get ready for programming MTRRs.
//
- 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)) {
@@ -1025,43 +871,43 @@ 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 ();
MtrrSetAllMtrrs (&Mtrrs);
//
// 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.
@@ -1325,11 +1171,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);
}
@@ -1454,11 +1300,11 @@ InternalSmmStartupAllAPs (
//
// Decrease the count to mark this processor(AP or BSP) as finished.
//
if (ProcToken != NULL) {
- WaitForSemaphore (&ProcToken->RunningApCount);
+ InterlockedDecrement (&ProcToken->RunningApCount);
}
}
}
ReleaseAllAPs ();
@@ -1729,14 +1575,14 @@ 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!
+ // "RSmmCpuSyncIncreaseArrivalCount (mSmmMpSyncData->SmmCpuSyncCtx) <= 0" means BSP has already ended the synchronization.
//
- if (ReleaseSemaphore (mSmmMpSyncData->Counter) == 0) {
+ if (SmmCpuSyncCheckInCpu (mSmmMpSyncData->SmmCpuSyncCtx, CpuIndex) <= 0) {
//
// BSP has already ended the synchronization, so QUIT!!!
// Existing AP is too late now to enter SMI since BSP has already ended the synchronization!!!
//
@@ -1828,12 +1674,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 ();
@@ -1949,12 +1793,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;
@@ -1964,12 +1806,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;
@@ -2003,32 +1843,28 @@ InitializeMpSyncData (
mSmmMpSyncData->BspIndex = (UINT32)-1;
}
mSmmMpSyncData->EffectiveSyncMode = mCpuSmmSyncMode;
- mSmmMpSyncData->Counter = mSmmCpuSemaphores.SemaphoreGlobal.Counter;
+ mSmmMpSyncData->SmmCpuSyncCtx = SmmCpuSyncContextInit (gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus);
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 654935dc76..b7bb937fbb 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;
+ VOID *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 (#110643): https://edk2.groups.io/g/devel/message/110643
Mute This Topic: https://groups.io/mt/102366305/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] 21+ messages in thread* Re: [edk2-devel] [PATCH v1 7/7] UefiCpuPkg/PiSmmCpuDxeSmm: Consume SmmCpuSyncLib
2023-11-03 15:30 ` [edk2-devel] [PATCH v1 7/7] UefiCpuPkg/PiSmmCpuDxeSmm: Consume SmmCpuSyncLib Wu, Jiaxin
@ 2023-11-07 11:00 ` Laszlo Ersek
2023-11-07 11:47 ` Wu, Jiaxin
0 siblings, 1 reply; 21+ messages in thread
From: Laszlo Ersek @ 2023-11-07 11:00 UTC (permalink / raw)
To: devel, jiaxin.wu; +Cc: Eric Dong, Ray Ni, Zeng Star, Gerd Hoffmann, Rahul Kumar
On 11/3/23 16:30, Wu, Jiaxin wrote:
> 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.
>
> Change-Id: Id034de47b85743c125f0d76420947e0dd9e69518
> 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 | 256 +++++----------------------
> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 6 +-
> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 1 +
> 3 files changed, 49 insertions(+), 214 deletions(-)
I'll first review this in the v2 posting.
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110841): https://edk2.groups.io/g/devel/message/110841
Mute This Topic: https://groups.io/mt/102366305/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] 21+ messages in thread
* Re: [edk2-devel] [PATCH v1 7/7] UefiCpuPkg/PiSmmCpuDxeSmm: Consume SmmCpuSyncLib
2023-11-07 11:00 ` Laszlo Ersek
@ 2023-11-07 11:47 ` Wu, Jiaxin
0 siblings, 0 replies; 21+ messages in thread
From: Wu, Jiaxin @ 2023-11-07 11:47 UTC (permalink / raw)
To: Laszlo Ersek, devel@edk2.groups.io
Cc: Dong, Eric, Ni, Ray, Zeng, Star, Gerd Hoffmann, Kumar, Rahul R
Thanks Laszlo, much appreciate all comments. I will check one by one for fix in next version. /Jiaxin
> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Tuesday, November 7, 2023 7:01 PM
> To: devel@edk2.groups.io; Wu, Jiaxin <jiaxin.wu@intel.com>
> Cc: 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: Re: [edk2-devel] [PATCH v1 7/7] UefiCpuPkg/PiSmmCpuDxeSmm:
> Consume SmmCpuSyncLib
>
> On 11/3/23 16:30, Wu, Jiaxin wrote:
> > 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.
> >
> > Change-Id: Id034de47b85743c125f0d76420947e0dd9e69518
> > 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 | 256 +++++----------------
> ------
> > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 6 +-
> > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 1 +
> > 3 files changed, 49 insertions(+), 214 deletions(-)
>
> I'll first review this in the v2 posting.
>
> Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110843): https://edk2.groups.io/g/devel/message/110843
Mute This Topic: https://groups.io/mt/102366305/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 21+ messages in thread