[edk2-devel] [Patch 1/1] UefiCpuPkg/Library/MpInitLib: Fix AP VolatileRegisters race condition
Zeng, Star
star.zeng at intel.com
Tue Jan 26 01:18:08 UTC 2021
> -----Original Message-----
> From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Laszlo
> Ersek
> Sent: Tuesday, January 26, 2021 5:18 AM
> To: Zeng, Star <star.zeng at intel.com>; Ni, Ray <ray.ni at intel.com>; Kinney,
> Michael D <michael.d.kinney at intel.com>; devel at edk2.groups.io
> Cc: Dong, Eric <eric.dong at intel.com>; Kumar, Rahul1
> <rahul1.kumar at 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.zeng at intel.com> to this patch.
Thanks,
Star
>
> Thanks
> Laszlo
>
> >> -----Original Message-----
> >> From: Ni, Ray <ray.ni at intel.com>
> >> Sent: Monday, January 25, 2021 4:53 PM
> >> To: Zeng, Star <star.zeng at intel.com>; Kinney, Michael D
> >> <michael.d.kinney at intel.com>; devel at edk2.groups.io
> >> Cc: Dong, Eric <eric.dong at intel.com>; Laszlo Ersek
> >> <lersek at redhat.com>; Kumar, Rahul1 <rahul1.kumar at 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.zeng at intel.com>
> >>> Sent: Monday, January 25, 2021 4:43 PM
> >>> To: Kinney, Michael D <michael.d.kinney at intel.com>;
> >>> devel at edk2.groups.io
> >>> Cc: Dong, Eric <eric.dong at intel.com>; Ni, Ray <ray.ni at intel.com>;
> >>> Laszlo Ersek <lersek at redhat.com>; Kumar, Rahul1
> >>> <rahul1.kumar at intel.com>; Zeng, Star <star.zeng at 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.kinney at intel.com>
> >>>> Sent: Monday, January 25, 2021 1:16 PM
> >>>> To: Zeng, Star <star.zeng at intel.com>; devel at edk2.groups.io; Kinney,
> >>>> Michael D <michael.d.kinney at intel.com>
> >>>> Cc: Dong, Eric <eric.dong at intel.com>; Ni, Ray <ray.ni at intel.com>;
> >>>> Laszlo Ersek <lersek at redhat.com>; Kumar, Rahul1
> >>>> <rahul1.kumar at 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.zeng at intel.com>
> >>>>> Sent: Sunday, January 24, 2021 7:15 PM
> >>>>> To: devel at edk2.groups.io; Kinney, Michael D
> >>>>> <michael.d.kinney at intel.com>
> >>>>> Cc: Dong, Eric <eric.dong at intel.com>; Ni, Ray <ray.ni at intel.com>;
> >>>>> Laszlo Ersek <lersek at redhat.com>; Kumar, Rahul1
> >>>>> <rahul1.kumar at intel.com>; Zeng, Star <star.zeng at 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of
> >>>>>> Michael D Kinney
> >>>>>> Sent: Saturday, January 23, 2021 1:10 AM
> >>>>>> To: devel at edk2.groups.io
> >>>>>> Cc: Dong, Eric <eric.dong at intel.com>; Ni, Ray <ray.ni at intel.com>;
> >>>>>> Laszlo Ersek <lersek at redhat.com>; Kumar,
> >>>>>> Rahul1 <rahul1.kumar at 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.dong at intel.com>
> >>>>>> Cc: Ray Ni <ray.ni at intel.com>
> >>>>>> Cc: Laszlo Ersek <lersek at redhat.com>
> >>>>>> Cc: Rahul Kumar <rahul1.kumar at intel.com>
> >>>>>> Signed-off-by: Michael D Kinney <michael.d.kinney at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list