Re: [edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: Don't increase CpuCount in ApWakeupFunction

["Ni, Ray" <ray.ni@intel.com>]

Somehow I forgot to add "Cc" tag in the patch resulting the package maintainers are not CCed.
Add them back.

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray
> Sent: Tuesday, January 26, 2021 1:50 PM
> To: devel@edk2.groups.io
> Subject: [edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: Don't increase
> CpuCount in ApWakeupFunction
> 
> 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/1712937
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ray.ni@intel.com]
> -=-=-=-=-=-=
>