> -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo > Ersek > Sent: Tuesday, January 26, 2021 5:18 AM > To: Zeng, Star <star.z...@intel.com>; Ni, Ray <ray...@intel.com>; Kinney, > Michael D <michael.d.kin...@intel.com>; devel@edk2.groups.io > Cc: Dong, Eric <eric.d...@intel.com>; Kumar, Rahul1 > <rahul1.ku...@intel.com> > Subject: Re: [edk2-devel] [Patch 1/1] UefiCpuPkg/Library/MpInitLib: Fix AP > VolatileRegisters race condition > > 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.
Make sense, Reviewed-by: Star Zeng <star.z...@intel.com> to this patch. Thanks, Star > > 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/MpIn > >> i > >>>>> 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 (#70747): https://edk2.groups.io/g/devel/message/70747 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] -=-=-=-=-=-=-=-=-=-=-=-
