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

["Dong, Eric" <eric.dong@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4562 bytes --]

Reviewed-by: Eric Dong <eric.dong@intel.com>

From: Ni, Ray <ray.ni@intel.com>
Sent: Thursday, January 28, 2021 11:27 AM
To: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
Cc: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
Subject: RE: [edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: Don't increase CpuCount in ApWakeupFunction

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<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Ni, Ray
> Sent: Tuesday, January 26, 2021 1:50 PM
> To: devel@edk2.groups.io<mailto: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<mailto: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<mailto:devel+owner@edk2.groups.io>
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ray.ni@intel.com]
> -=-=-=-=-=-=
>

[-- Attachment #2: Type: text/html, Size: 15701 bytes --]