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...@intel.com> > Cc: Eric Dong <eric.d...@intel.com> > Cc: Nandagopal Sathyanarayanan <nandagopal.sathyanaraya...@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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#41826): https://edk2.groups.io/g/devel/message/41826 Mute This Topic: https://groups.io/mt/31878551/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-