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

["Laszlo Ersek" <lersek@redhat.com>]

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.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/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.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
>>>>>>
>>>>>>
>>>>>>
>>>>>> 
>>>>>>
>