[edk2-devel] [Patch 1/1] UefiCpuPkg/Library/MpInitLib: Fix AP VolatileRegisters race condition
Laszlo Ersek
lersek at redhat.com
Mon Jan 25 21:17:32 UTC 2021
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 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/MpIni
>>>>> 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 (#70742): https://edk2.groups.io/g/devel/message/70742
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