> -----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...@intel.com> > Cc: Dong, Eric <eric.d...@intel.com>; Sathyanarayanan, Nandagopal > <nandagopal.sathyanaraya...@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...@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.
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 > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#41857): https://edk2.groups.io/g/devel/message/41857 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] -=-=-=-=-=-=-=-=-=-=-=-