Re: [edk2-devel] [Patch 1/1] UefiCpuPkg/Library/MpInitLib: Fix AP VolatileRegisters race condition

["Zeng, Star" <star.zeng@intel.com>]

> -----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.zeng@intel.com>; Ni, Ray <ray.ni@intel.com>; Kinney,
> Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul1
> <rahul1.kumar@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@intel.com> to this patch.


Thanks,
Star

> 
> Thanks
> Laszlo
> 
> >> -----Original Message-----
> >> From: Ni, Ray <ray.ni@intel.com>
> >> Sent: Monday, January 25, 2021 4:53 PM
> >> To: Zeng, Star <star.zeng@intel.com>; Kinney, Michael D
> >> <michael.d.kinney@intel.com>; devel@edk2.groups.io
> >> Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek
> >> <lersek@redhat.com>; Kumar, Rahul1 <rahul1.kumar@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@intel.com>
> >>> Sent: Monday, January 25, 2021 4:43 PM
> >>> To: Kinney, Michael D <michael.d.kinney@intel.com>;
> >>> devel@edk2.groups.io
> >>> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>;
> >>> Laszlo Ersek <lersek@redhat.com>; Kumar, Rahul1
> >>> <rahul1.kumar@intel.com>; Zeng, Star <star.zeng@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@intel.com>
> >>>> Sent: Monday, January 25, 2021 1:16 PM
> >>>> To: Zeng, Star <star.zeng@intel.com>; devel@edk2.groups.io; Kinney,
> >>>> Michael D <michael.d.kinney@intel.com>
> >>>> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>;
> >>>> Laszlo Ersek <lersek@redhat.com>; Kumar, Rahul1
> >>>> <rahul1.kumar@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@intel.com>
> >>>>> Sent: Sunday, January 24, 2021 7:15 PM
> >>>>> To: devel@edk2.groups.io; Kinney, Michael D
> >>>>> <michael.d.kinney@intel.com>
> >>>>> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>;
> >>>>> Laszlo Ersek <lersek@redhat.com>; Kumar, Rahul1
> >>>>> <rahul1.kumar@intel.com>; Zeng, Star <star.zeng@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.dong@intel.com>; Ni, Ray <ray.ni@intel.com>;
> >>>>>> Laszlo Ersek <lersek@redhat.com>; Kumar,
> >>>>>> Rahul1 <rahul1.kumar@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@intel.com>
> >>>>>> Cc: Ray Ni <ray.ni@intel.com>
> >>>>>> Cc: Laszlo Ersek <lersek@redhat.com>
> >>>>>> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> >>>>>> Signed-off-by: Michael D Kinney <michael.d.kinney@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
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >
> 
> 
> 
> 
>