REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3179
When BSP first time wakes all APs, each AP atomically increases
CpuMpData->CpuCount and CpuMpData->FinishedCount.
Each AP atomically increases CpuMpData->NumApsExecuting
in early assembly code and decreases it before it enters to HLT or
MWAIT state.
Putting them together, the 3 variables are changed in the following order:
1. NumApsExecuting++ // in assembly
2. CpuCpunt++
4. FinishedCount++
3. NumApsExecuting-- // in C
BSP waits for a certain timeout and then polls NumApsExecuting
until it drops to zero. It assumes all APs are waken up concurrently
and NumApsExecuting only drops to zero when all APs have checked in.
Then it additionally waits for FinishedCount == CpuCount - 1.
(FinishedCount doesn't include BSP while CpuCount includes BSP.)
There is no need to additionally wait for
FinishedCount == CpuCount - 1 because when NumApsExecuting == 0,
the number of increments of FinishedCount and CpuCount should equal.
This patch simplifies the code to remove "CpuCount++" in
ApWakeupFunction() and assigns FinishedCount + 1 to CpuCount after
WakeUpAP().
Signed-off-by: Ray Ni <ray.ni@intel.com>
---
UefiCpuPkg/Library/MpInitLib/MpLib.c | 16 +++++-----------
1 file changed, 5 insertions(+), 11 deletions(-)
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 8b1f7f84ba..2568986d8c 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -1,7 +1,7 @@
/** @file
CPU MP Initialize Library common functions.
- Copyright (c) 2016 - 2020, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2016 - 2021, Intel Corporation. All rights reserved.<BR>
Copyright (c) 2020, AMD Inc. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -485,14 +485,12 @@ CollectProcessorCount (
CpuMpData->InitFlag = ApInitConfig;
WakeUpAP (CpuMpData, TRUE, 0, NULL, NULL, TRUE);
CpuMpData->InitFlag = ApInitDone;
- ASSERT (CpuMpData->CpuCount <= PcdGet32 (PcdCpuMaxLogicalProcessorNumber));
//
- // Wait for all APs finished the initialization
+ // When InitFlag == ApInitConfig, WakeUpAP () guarantees all APs are checked in.
+ // FinishedCount is the number of check-in APs.
//
- while (CpuMpData->FinishedCount < (CpuMpData->CpuCount - 1)) {
- CpuPause ();
- }
-
+ CpuMpData->CpuCount = CpuMpData->FinishedCount + 1;
+ ASSERT (CpuMpData->CpuCount <= PcdGet32 (PcdCpuMaxLogicalProcessorNumber));
//
// Enable x2APIC mode if
@@ -751,10 +749,6 @@ ApWakeupFunction (
CurrentApicMode = GetApicMode ();
while (TRUE) {
if (CpuMpData->InitFlag == ApInitConfig) {
- //
- // Add CPU number
- //
- InterlockedIncrement ((UINT32 *) &CpuMpData->CpuCount);
ProcessorNumber = ApIndex;
//
// This is first time AP wakeup, get BIST information from AP stack
--
2.27.0.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70757): https://edk2.groups.io/g/devel/message/70757
Mute This Topic: https://groups.io/mt/80124850/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 01/26/21 06:50, Ni, Ray wrote: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3179 > > When BSP first time wakes all APs, each AP atomically increases > CpuMpData->CpuCount and CpuMpData->FinishedCount. > Each AP atomically increases CpuMpData->NumApsExecuting > in early assembly code and decreases it before it enters to HLT or > MWAIT state. > > Putting them together, the 3 variables are changed in the following order: > 1. NumApsExecuting++ // in assembly > 2. CpuCpunt++ > 4. FinishedCount++ > 3. NumApsExecuting-- // in C > > BSP waits for a certain timeout and then polls NumApsExecuting > until it drops to zero. It assumes all APs are waken up concurrently > and NumApsExecuting only drops to zero when all APs have checked in. > > Then it additionally waits for FinishedCount == CpuCount - 1. > (FinishedCount doesn't include BSP while CpuCount includes BSP.) > > There is no need to additionally wait for > FinishedCount == CpuCount - 1 because when NumApsExecuting == 0, > the number of increments of FinishedCount and CpuCount should equal. > > This patch simplifies the code to remove "CpuCount++" in > ApWakeupFunction() and assigns FinishedCount + 1 to CpuCount after > WakeUpAP(). > > Signed-off-by: Ray Ni <ray.ni@intel.com> > --- > UefiCpuPkg/Library/MpInitLib/MpLib.c | 16 +++++----------- > 1 file changed, 5 insertions(+), 11 deletions(-) > > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c > index 8b1f7f84ba..2568986d8c 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > @@ -1,7 +1,7 @@ > /** @file > CPU MP Initialize Library common functions. > > - Copyright (c) 2016 - 2020, Intel Corporation. All rights reserved.<BR> > + Copyright (c) 2016 - 2021, Intel Corporation. All rights reserved.<BR> > Copyright (c) 2020, AMD Inc. All rights reserved.<BR> > > SPDX-License-Identifier: BSD-2-Clause-Patent > @@ -485,14 +485,12 @@ CollectProcessorCount ( > CpuMpData->InitFlag = ApInitConfig; > WakeUpAP (CpuMpData, TRUE, 0, NULL, NULL, TRUE); > CpuMpData->InitFlag = ApInitDone; > - ASSERT (CpuMpData->CpuCount <= PcdGet32 (PcdCpuMaxLogicalProcessorNumber)); > // > - // Wait for all APs finished the initialization > + // When InitFlag == ApInitConfig, WakeUpAP () guarantees all APs are checked in. > + // FinishedCount is the number of check-in APs. > // > - while (CpuMpData->FinishedCount < (CpuMpData->CpuCount - 1)) { > - CpuPause (); > - } > - > + CpuMpData->CpuCount = CpuMpData->FinishedCount + 1; > + ASSERT (CpuMpData->CpuCount <= PcdGet32 (PcdCpuMaxLogicalProcessorNumber)); > > // > // Enable x2APIC mode if > @@ -751,10 +749,6 @@ ApWakeupFunction ( > CurrentApicMode = GetApicMode (); > while (TRUE) { > if (CpuMpData->InitFlag == ApInitConfig) { > - // > - // Add CPU number > - // > - InterlockedIncrement ((UINT32 *) &CpuMpData->CpuCount); > ProcessorNumber = ApIndex; > // > // This is first time AP wakeup, get BIST information from AP stack > In ApWakeupFunction(), there is a stretch of code (pre-patch) where CpuCount has been incremented, but FinishedCount hasn't, yet. In that part of the AP code, the PAUSE loop in CollectProcessorCount() is waiting (running on the BSP). But, said part of the AP code is (more broadly) bracketed by the NumApsExecuting increment / decrement as well. And NumApsExecuting==0 is waited-for in WakeUpAP(). So this patch looks OK to me. I didn't try to verify the patch more thoroughly than described above; OTOH, on QEMU, the *other* branch in WakeUpAP() is supposed to be active (i.e. the one where "PcdCpuBootLogicalProcessorNumber" is nonzero). In that case, TimedWaitForApFinish() will wait until FinishedCount reaches (PcdGet32 (PcdCpuBootLogicalProcessorNumber) - 1) exactly, so the NumApsExecuting==0 check is not relevant in the first place. IOW, I think this patch cannot regress behavior on QEMU. Acked-by: Laszlo Ersek <lersek@redhat.com> Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#70771): https://edk2.groups.io/g/devel/message/70771 Mute This Topic: https://groups.io/mt/80124850/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.