[PATCH] UefiCpuPkg/MpInitLib: Decrease NumApsExecuting only for ApInitConfig

[PATCH] UefiCpuPkg/MpInitLib: Decrease NumApsExecuting only for ApInitConfig

[Ni, Ray]

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;
-- 
2.21.0.windows.1

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

[Laszlo Ersek]

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

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

[Ni, Ray]

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
> Ersek
> Sent: Monday, June 3, 2019 11:21 PM
> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>; Sathyanarayanan, Nandagopal
> <nandagopal.sathyanarayanan@intel.com>
> Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: Decrease
> NumApsExecuting only for ApInitConfig
> 
> 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.

Oops. I should have removed the 2 debug messages when making the patch.
I will remove that message in V2.

> 
> (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.

Yes I will remove the 2 debug messages.

> 
> (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).

Though that requires I make additional testing, I agree it makes code more clean😊
Will do that in V2.

> 
> Thanks
> Laszlo
> 
>