Re: [edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: Decrease NumApsExecuting only for ApInitConfig

["Laszlo Ersek" <lersek@redhat.com>]

Hi Ray,

On 05/31/19 11:42, Ni, Ray wrote:
> The patch fixes the bug that the memory under 1MB is modified by
> firmware in S3 boot.
> 
> Root cause is a racing condition in MpInitLib:
> 1. BSP: WakeUpByInitSipiSipi is set by NotifyOnS3SmmInitDonePpi()
> 2. BSP: WakeUpAP() wakes all APs to run certain procedure.
>   2.1. AllocateResetVector() uses <1MB memory for wake up vector.
>   2.1. FillExchangeInfoData() resets NumApsExecuting to 0.
>   2.2. WaitApWakeup() waits AP to clear WAKEUP_AP_SIGNAL.
> 3. AP: ApWakeupFunction() clears WAKEUP_AP_SIGNAL to inform BSP.
> 5. BSP: FreeResetVector() restores the <1MB memory
> 4. AP: ApWakeupFunction() calls the certain procedure.
>   4.1. NumApsExecuting is decreased.
> 
> #4.1 happens after the 1MB memory is restored so the result is
> memory below 1MB is changed by #4.1
> It happens only when the AP executes procedure a bit longer.
> AP returns back to ApWakeupFunction() from procedure after
> BSP restores the <1MB memory.
> 
> Since NumApsExecuting is only used when InitFlag == ApInitConfig
> for counting the processor count.
> The patch moves the NumApsExecuting decrease to the path when
> InitFlag == ApInitConfig.
> 
> Signed-off-by: Ray Ni <ray.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Nandagopal Sathyanarayanan <nandagopal.sathyanarayanan@intel.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 3337488892..84f1cb92e3 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 - 2018, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2016 - 2019, Intel Corporation. All rights reserved.<BR>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>  
>  **/
> @@ -619,6 +619,8 @@ ApWakeupFunction (
>        RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE);
>        InitializeApData (CpuMpData, ProcessorNumber, BistData, ApTopOfStack);
>        ApStartupSignalBuffer = CpuMpData->CpuData[ProcessorNumber].StartupApSignal;
> +
> +      InterlockedDecrement ((UINT32 *) &CpuMpData->MpCpuExchangeInfo->NumApsExecuting);
>      } else {
>        //
>        // Execute AP function if AP is ready
> @@ -698,7 +700,6 @@ ApWakeupFunction (
>      // AP finished executing C code
>      //
>      InterlockedIncrement ((UINT32 *) &CpuMpData->FinishedCount);
> -    InterlockedDecrement ((UINT32 *) &CpuMpData->MpCpuExchangeInfo->NumApsExecuting);
>  
>      //
>      // Place AP is specified loop mode
> @@ -805,6 +806,7 @@ FillExchangeInfoData (
>    ExchangeInfo->CFunction       = (UINTN) ApWakeupFunction;
>    ExchangeInfo->ApIndex         = 0;
>    ExchangeInfo->NumApsExecuting = 0;
> +  DEBUG ((DEBUG_ERROR, "NumApsExecuting = %p\n", &ExchangeInfo->NumApsExecuting));
>    ExchangeInfo->InitFlag        = (UINTN) CpuMpData->InitFlag;
>    ExchangeInfo->CpuInfo         = (CPU_INFO_IN_HOB *) (UINTN) CpuMpData->CpuInfoInHob;
>    ExchangeInfo->CpuMpData       = CpuMpData;
> @@ -1598,6 +1600,7 @@ MpInitLibInitialize (
>    CpuMpData->Buffer           = Buffer;
>    CpuMpData->CpuApStackSize   = ApStackSize;
>    CpuMpData->BackupBuffer     = BackupBufferAddr;
> +  DEBUG ((DEBUG_ERROR, "BackupBuffer = %p\n", BackupBufferAddr));
>    CpuMpData->BackupBufferSize = ApResetVectorSize;
>    CpuMpData->WakeupBuffer     = (UINTN) -1;
>    CpuMpData->CpuCount         = 1;
> 

(1) I think we should not use DEBUG_ERROR for informational or even
debug messages. We should use DEBUG_INFO or DEBUG_VERBOSE.

(2) %p should be used for logging values of type (VOID*). The address of
"ExchangeInfo->NumApsExecuting" is "close enough" (at least the
expression produces a pointer value), but "BackupBufferAddr" has type
"UINTN". For printing UINTN, the portable way is to cast the value to
UINT64, and log it with %Lx.

(3) The commit message states "NumApsExecuting is only used when
InitFlag == ApInitConfig". This may be the intent, but my reading of the
assembly code does not confirm it.

It is true that NumApsExecuting is monitored by the BSP in WakeUpAP()
only if (InitFlag == ApInitConfig).

It is also true that "LongModeStart" in
"UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm" increments
NumApsExecuting only when (InitFlag == ApInitConfig).

However, in "Flat32Start", in
"UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm", we increment
NumApsExecuting *first*, and check (InitFlag == ApInitConfig) only second.

I think the behavior of the IA32 assembly is unintended. I suggest that
we fix that first, in a separate patch. And then the correctness of the
present patch may be seen more easily. Then we can state:
- only CollectProcessorCount() sets InitFlag to ApInitConfig,
- the monitoring of NumApsExecuting is restricted to ApInitConfig,
- the incrementing of NumApsExecuting is restricted to ApInitConfig
  (after patch v2 1/2),
- and so the decrementing should be similarly restricted
  (in patch v2 2/2 -- i.e., this patch).

Thanks
Laszlo