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

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

[Laszlo Ersek]

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

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

[Ni, Ray]

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]
> -=-=-=-=-=-=
> 

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

[Dong, Eric]

[-- 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 --]