* [PATCH] UefiCpuPkg/MpInitLib: Decrease NumApsExecuting only for ApInitConfig @ 2019-05-31 9:42 Ni, Ray 2019-06-03 15:20 ` [edk2-devel] " Laszlo Ersek 0 siblings, 1 reply; 3+ messages in thread From: Ni, Ray @ 2019-05-31 9:42 UTC (permalink / raw) To: devel; +Cc: Eric Dong, Nandagopal Sathyanarayanan 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 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: Decrease NumApsExecuting only for ApInitConfig 2019-05-31 9:42 [PATCH] UefiCpuPkg/MpInitLib: Decrease NumApsExecuting only for ApInitConfig Ni, Ray @ 2019-06-03 15:20 ` Laszlo Ersek 2019-06-04 10:46 ` Ni, Ray 0 siblings, 1 reply; 3+ messages in thread From: Laszlo Ersek @ 2019-06-03 15:20 UTC (permalink / raw) To: devel, ray.ni; +Cc: Eric Dong, Nandagopal Sathyanarayanan 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: Decrease NumApsExecuting only for ApInitConfig 2019-06-03 15:20 ` [edk2-devel] " Laszlo Ersek @ 2019-06-04 10:46 ` Ni, Ray 0 siblings, 0 replies; 3+ messages in thread From: Ni, Ray @ 2019-06-04 10:46 UTC (permalink / raw) To: devel@edk2.groups.io, 'lersek@redhat.com' Cc: Dong, Eric, Sathyanarayanan, Nandagopal > -----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 > > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-06-04 10:46 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-05-31 9:42 [PATCH] UefiCpuPkg/MpInitLib: Decrease NumApsExecuting only for ApInitConfig Ni, Ray 2019-06-03 15:20 ` [edk2-devel] " Laszlo Ersek 2019-06-04 10:46 ` Ni, Ray
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox