On 01/25/21 12:04, Zeng, Star wrote: > BTW: > > Do you think it worth or not to add the check like below in > Edk2\UefiCpuPkg\Library\MpInitLib\PeiMpLib.c GetCpuMpData()? If the assert > was there, it would facilitate the debug? > > CpuMpData = (CPU_MP_DATA *) (Idtr.Base + Idtr.Limit + 1); > + ASSERT (CpuMpData != NULL);
Could be worthwhile, but in a separate patch, IMO. Thanks Laszlo >> -----Original Message----- >> From: Ni, Ray <ray...@intel.com> >> Sent: Monday, January 25, 2021 4:53 PM >> To: Zeng, Star <star.z...@intel.com>; Kinney, Michael D >> <michael.d.kin...@intel.com>; devel@edk2.groups.io >> Cc: Dong, Eric <eric.d...@intel.com>; Laszlo Ersek <ler...@redhat.com>; >> Kumar, Rahul1 <rahul1.ku...@intel.com> >> Subject: RE: [edk2-devel] [Patch 1/1] UefiCpuPkg/Library/MpInitLib: Fix AP >> VolatileRegisters race condition >> >> Star, >> You are right. There is no sequence requirement between FinishedCount++ >> and NumApsExecuting--. >> >> In fact, I have submitted another bugzilla >> https://bugzilla.tianocore.org/show_bug.cgi?id=3179. >> >> With that Bugzilla, the wait on (FinishedCount == CpuCount - 1) will be >> removed for the 1st wake up case. >> >> Thanks, >> Ray >> >> >>> -----Original Message----- >>> From: Zeng, Star <star.z...@intel.com> >>> Sent: Monday, January 25, 2021 4:43 PM >>> To: Kinney, Michael D <michael.d.kin...@intel.com>; >>> devel@edk2.groups.io >>> Cc: Dong, Eric <eric.d...@intel.com>; Ni, Ray <ray...@intel.com>; >>> Laszlo Ersek <ler...@redhat.com>; Kumar, Rahul1 >>> <rahul1.ku...@intel.com>; Zeng, Star <star.z...@intel.com> >>> Subject: RE: [edk2-devel] [Patch 1/1] UefiCpuPkg/Library/MpInitLib: >>> Fix AP VolatileRegisters race condition >>> >>> Mike, >>> >>> Oh, see it. >>> There is no sequence dependence between FinishedCount increment and >>> NumApsExecuting decrement, right? >>> >>> InterlockedIncrement ((UINT32 *) &CpuMpData->FinishedCount); >>> >>> InterlockedDecrement ((UINT32 *) &CpuMpData- >>> MpCpuExchangeInfo- >>>> NumApsExecuting); >>> >>> >>> Thanks, >>> Star >>> >>>> -----Original Message----- >>>> From: Kinney, Michael D <michael.d.kin...@intel.com> >>>> Sent: Monday, January 25, 2021 1:16 PM >>>> To: Zeng, Star <star.z...@intel.com>; devel@edk2.groups.io; Kinney, >>>> Michael D <michael.d.kin...@intel.com> >>>> Cc: Dong, Eric <eric.d...@intel.com>; Ni, Ray <ray...@intel.com>; >>>> Laszlo Ersek <ler...@redhat.com>; Kumar, Rahul1 >>>> <rahul1.ku...@intel.com> >>>> Subject: RE: [edk2-devel] [Patch 1/1] UefiCpuPkg/Library/MpInitLib: >>>> Fix AP VolatileRegisters race condition >>>> >>>> Hi Star, >>>> >>>> That line is only active when (CpuMpData->SevEsIsEnabled) is TRUE. >>>> >>>> The race condition addressed by this BZ is for systems with >>>> SecEsIsEnabled FALSE. >>>> >>>> From comments in this file the SecEsIsEnabled cases have already >>>> been handled. >>>> >>>> Mike >>>> >>>>> -----Original Message----- >>>>> From: Zeng, Star <star.z...@intel.com> >>>>> Sent: Sunday, January 24, 2021 7:15 PM >>>>> To: devel@edk2.groups.io; Kinney, Michael D >>>>> <michael.d.kin...@intel.com> >>>>> Cc: Dong, Eric <eric.d...@intel.com>; Ni, Ray <ray...@intel.com>; >>>>> Laszlo Ersek <ler...@redhat.com>; Kumar, Rahul1 >>>>> <rahul1.ku...@intel.com>; Zeng, Star <star.z...@intel.com> >>>>> Subject: RE: [edk2-devel] [Patch 1/1] UefiCpuPkg/Library/MpInitLib: >>>>> Fix AP VolatileRegisters race condition >>>>> >>>>> Does >>>>> >>>> >>> >> https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/MpIni >>>>> tLib/MpLib.c#L909 (also decrements >>>>> NumApsExecuting) also need be handled? >>>>> >>>>> Thanks, >>>>> Star >>>>> >>>>>> -----Original Message----- >>>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of >>>>>> Michael D Kinney >>>>>> Sent: Saturday, January 23, 2021 1:10 AM >>>>>> To: devel@edk2.groups.io >>>>>> Cc: Dong, Eric <eric.d...@intel.com>; Ni, Ray >>>>>> <ray...@intel.com>; Laszlo Ersek <ler...@redhat.com>; Kumar, >>>>>> Rahul1 <rahul1.ku...@intel.com> >>>>>> Subject: [edk2-devel] [Patch 1/1] UefiCpuPkg/Library/MpInitLib: >>>>>> Fix AP VolatileRegisters race condition >>>>>> >>>>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3182 >>>>>> >>>>>> Fix the order of operations in ApWakeupFunction() when >>>>>> PcdCpuApLoopMode is set to HLT mode that uses INIT-SIPI-SIPI to >>> wake >>>>>> APs. In this mode, volatile state is restored and saved each >>>>>> time a INIT-SIPI-SIPI is sent to an AP to request a function to >>>>>> be executed on the AP. When the function is completed the >>>>>> volatile state of the AP is saved. However, the counters >>>>>> NumApsExecuting and FinishedCount are updated before the >>>>>> volatile state is saved. This allows for a race condition >>>>>> window for the BSP that is waiting on these counters to request >>>>>> a new INIT-SIPI-SIPI before all the APs have completely saved >>>>>> their volatile state. The fix is to save the AP volatile state >>>>>> before updating the NumApsExecuting and FinishedCount counters. >>>>>> >>>>>> Cc: Eric Dong <eric.d...@intel.com> >>>>>> Cc: Ray Ni <ray...@intel.com> >>>>>> Cc: Laszlo Ersek <ler...@redhat.com> >>>>>> Cc: Rahul Kumar <rahul1.ku...@intel.com> >>>>>> Signed-off-by: Michael D Kinney <michael.d.kin...@intel.com> >>>>>> --- >>>>>> UefiCpuPkg/Library/MpInitLib/MpLib.c | 31 >>>>>> ++++++++++++++++------------ >>>>>> 1 file changed, 18 insertions(+), 13 deletions(-) >>>>>> >>>>>> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c >>>>>> b/UefiCpuPkg/Library/MpInitLib/MpLib.c >>>>>> index 681fa79b4cff..8b1f7f84bad6 100644 >>>>>> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c >>>>>> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c >>>>>> @@ -769,15 +769,6 @@ ApWakeupFunction ( >>>>>> RestoreVolatileRegisters >>>>>> (&CpuMpData->CpuData[0].VolatileRegisters, >>>>>> FALSE); >>>>>> InitializeApData (CpuMpData, ProcessorNumber, BistData, >>>>>> ApTopOfStack); >>>>>> ApStartupSignalBuffer = CpuMpData- >>>>>>> CpuData[ProcessorNumber].StartupApSignal; >>>>>> - >>>>>> - // >>>>>> - // Delay decrementing the APs executing count when SEV-ES is >>>> enabled >>>>>> - // to allow the APs to issue an AP_RESET_HOLD before the BSP >>>> possibly >>>>>> - // performs another INIT-SIPI-SIPI sequence. >>>>>> - // >>>>>> - if (!CpuMpData->SevEsIsEnabled) { >>>>>> - InterlockedDecrement ((UINT32 *) &CpuMpData- >>>>>>> MpCpuExchangeInfo->NumApsExecuting); >>>>>> - } >>>>>> } else { >>>>>> // >>>>>> // Execute AP function if AP is ready @@ -866,19 +857,33 >>>>>> @@ ApWakeupFunction ( >>>>>> } >>>>>> } >>>>>> >>>>>> + if (CpuMpData->ApLoopMode == ApInHltLoop) { >>>>>> + // >>>>>> + // Save AP volatile registers >>>>>> + // >>>>>> + SaveVolatileRegisters (&CpuMpData- >>>>>>> CpuData[ProcessorNumber].VolatileRegisters); >>>>>> + } >>>>>> + >>>>>> // >>>>>> // AP finished executing C code >>>>>> // >>>>>> InterlockedIncrement ((UINT32 *) >>>>>> &CpuMpData->FinishedCount); >>>>>> >>>>>> + if (CpuMpData->InitFlag == ApInitConfig) { >>>>>> + // >>>>>> + // Delay decrementing the APs executing count when SEV-ES >>>>>> + is >>>> enabled >>>>>> + // to allow the APs to issue an AP_RESET_HOLD before the >>>>>> + BSP >>>> possibly >>>>>> + // performs another INIT-SIPI-SIPI sequence. >>>>>> + // >>>>>> + if (!CpuMpData->SevEsIsEnabled) { >>>>>> + InterlockedDecrement ((UINT32 *) &CpuMpData- >>>>>>> MpCpuExchangeInfo->NumApsExecuting); >>>>>> + } >>>>>> + } >>>>>> + >>>>>> // >>>>>> // Place AP is specified loop mode >>>>>> // >>>>>> if (CpuMpData->ApLoopMode == ApInHltLoop) { >>>>>> - // >>>>>> - // Save AP volatile registers >>>>>> - // >>>>>> - SaveVolatileRegisters (&CpuMpData- >>>>>>> CpuData[ProcessorNumber].VolatileRegisters); >>>>>> // >>>>>> // Place AP in HLT-loop >>>>>> // >>>>>> -- >>>>>> 2.29.2.windows.2 >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#70742): https://edk2.groups.io/g/devel/message/70742 Mute This Topic: https://groups.io/mt/80035510/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
