* [Patch 1/1] UefiCpuPkg/Library/MpInitLib: Fix AP VolatileRegisters race condition
@ 2021-01-22 17:10 Michael D Kinney
2021-01-23 2:02 ` [edk2-devel] " Laszlo Ersek
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Michael D Kinney @ 2021-01-22 17:10 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Laszlo Ersek, Rahul Kumar
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [Patch 1/1] UefiCpuPkg/Library/MpInitLib: Fix AP VolatileRegisters race condition
2021-01-22 17:10 [Patch 1/1] UefiCpuPkg/Library/MpInitLib: Fix AP VolatileRegisters race condition Michael D Kinney
@ 2021-01-23 2:02 ` Laszlo Ersek
2021-01-26 6:47 ` Philippe Mathieu-Daudé
2021-01-23 5:10 ` Dong, Eric
2021-01-25 3:14 ` [edk2-devel] " Zeng, Star
2 siblings, 1 reply; 13+ messages in thread
From: Laszlo Ersek @ 2021-01-23 2:02 UTC (permalink / raw)
To: devel, michael.d.kinney; +Cc: Eric Dong, Ray Ni, Rahul Kumar, Tom Lendacky
+Tom, comments below
On 01/22/21 18:10, Michael D Kinney wrote:
> 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
> //
>
This patch can only be reviewed with "git show -W", and even then, it's
not easy. :)
I managed to understand it in multiple steps (I think).
Step#1: extract the NumApsExecuting decrement from its current position,
so that it stand entirely alone, with its gating condition repeated. No
change of functionality.
This can be expressed with the following (no-op) patch:
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 681fa79b4cff..c13ddb5d9807 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -720,255 +720,257 @@ EFIAPI
> ApWakeupFunction (
> IN MP_CPU_EXCHANGE_INFO *ExchangeInfo,
> IN UINTN ApIndex
> )
> {
> CPU_MP_DATA *CpuMpData;
> UINTN ProcessorNumber;
> EFI_AP_PROCEDURE Procedure;
> VOID *Parameter;
> UINT32 BistData;
> volatile UINT32 *ApStartupSignalBuffer;
> CPU_INFO_IN_HOB *CpuInfoInHob;
> UINT64 ApTopOfStack;
> UINTN CurrentApicMode;
>
> //
> // AP finished assembly code and begin to execute C code
> //
> CpuMpData = ExchangeInfo->CpuMpData;
>
> //
> // AP's local APIC settings will be lost after received INIT IPI
> // We need to re-initialize them at here
> //
> ProgramVirtualWireMode ();
> //
> // Mask the LINT0 and LINT1 so that AP doesn't enter the system timer interrupt handler.
> //
> DisableLvtInterrupts ();
> SyncLocalApicTimerSetting (CpuMpData);
>
> CurrentApicMode = GetApicMode ();
> while (TRUE) {
> if (CpuMpData->InitFlag == ApInitConfig) {
> //
> // Add CPU number
> //
> InterlockedIncrement ((UINT32 *) &CpuMpData->CpuCount);
> ProcessorNumber = ApIndex;
> //
> // This is first time AP wakeup, get BIST information from AP stack
> //
> ApTopOfStack = CpuMpData->Buffer + (ProcessorNumber + 1) * CpuMpData->CpuApStackSize;
> BistData = *(UINT32 *) ((UINTN) ApTopOfStack - sizeof (UINTN));
> //
> // CpuMpData->CpuData[0].VolatileRegisters is initialized based on BSP environment,
> // to initialize AP in InitConfig path.
> // NOTE: IDTR.BASE stored in CpuMpData->CpuData[0].VolatileRegisters points to a different IDT shared by all APs.
> //
> 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
> //
> GetProcessorNumber (CpuMpData, &ProcessorNumber);
> //
> // Clear AP start-up signal when AP waken up
> //
> ApStartupSignalBuffer = CpuMpData->CpuData[ProcessorNumber].StartupApSignal;
> InterlockedCompareExchange32 (
> (UINT32 *) ApStartupSignalBuffer,
> WAKEUP_AP_SIGNAL,
> 0
> );
>
> if (CpuMpData->InitFlag == ApInitReconfig) {
> //
> // ApInitReconfig happens when:
> // 1. AP is re-enabled after it's disabled, in either PEI or DXE phase.
> // 2. AP is initialized in DXE phase.
> // In either case, use the volatile registers value derived from BSP.
> // NOTE: IDTR.BASE stored in CpuMpData->CpuData[0].VolatileRegisters points to a
> // different IDT shared by all APs.
> //
> RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE);
> } else {
> if (CpuMpData->ApLoopMode == ApInHltLoop) {
> //
> // Restore AP's volatile registers saved before AP is halted
> //
> RestoreVolatileRegisters (&CpuMpData->CpuData[ProcessorNumber].VolatileRegisters, TRUE);
> } else {
> //
> // The CPU driver might not flush TLB for APs on spot after updating
> // page attributes. AP in mwait loop mode needs to take care of it when
> // woken up.
> //
> CpuFlushTlb ();
> }
> }
>
> if (GetApState (&CpuMpData->CpuData[ProcessorNumber]) == CpuStateReady) {
> Procedure = (EFI_AP_PROCEDURE)CpuMpData->CpuData[ProcessorNumber].ApFunction;
> Parameter = (VOID *) CpuMpData->CpuData[ProcessorNumber].ApFunctionArgument;
> if (Procedure != NULL) {
> SetApState (&CpuMpData->CpuData[ProcessorNumber], CpuStateBusy);
> //
> // Enable source debugging on AP function
> //
> EnableDebugAgent ();
> //
> // Invoke AP function here
> //
> Procedure (Parameter);
> CpuInfoInHob = (CPU_INFO_IN_HOB *) (UINTN) CpuMpData->CpuInfoInHob;
> if (CpuMpData->SwitchBspFlag) {
> //
> // Re-get the processor number due to BSP/AP maybe exchange in AP function
> //
> GetProcessorNumber (CpuMpData, &ProcessorNumber);
> CpuMpData->CpuData[ProcessorNumber].ApFunction = 0;
> CpuMpData->CpuData[ProcessorNumber].ApFunctionArgument = 0;
> ApStartupSignalBuffer = CpuMpData->CpuData[ProcessorNumber].StartupApSignal;
> CpuInfoInHob[ProcessorNumber].ApTopOfStack = CpuInfoInHob[CpuMpData->NewBspNumber].ApTopOfStack;
> } else {
> if (CpuInfoInHob[ProcessorNumber].ApicId != GetApicId () ||
> CpuInfoInHob[ProcessorNumber].InitialApicId != GetInitialApicId ()) {
> if (CurrentApicMode != GetApicMode ()) {
> //
> // If APIC mode change happened during AP function execution,
> // we do not support APIC ID value changed.
> //
> ASSERT (FALSE);
> CpuDeadLoop ();
> } else {
> //
> // Re-get the CPU APICID and Initial APICID if they are changed
> //
> CpuInfoInHob[ProcessorNumber].ApicId = GetApicId ();
> CpuInfoInHob[ProcessorNumber].InitialApicId = GetInitialApicId ();
> }
> }
> }
> }
> SetApState (&CpuMpData->CpuData[ProcessorNumber], CpuStateFinished);
> }
> }
>
> + 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);
> + }
> + }
> +
> //
> // AP finished executing C code
> //
> InterlockedIncrement ((UINT32 *) &CpuMpData->FinishedCount);
>
> //
> // Place AP is specified loop mode
> //
> if (CpuMpData->ApLoopMode == ApInHltLoop) {
> //
> // Save AP volatile registers
> //
> SaveVolatileRegisters (&CpuMpData->CpuData[ProcessorNumber].VolatileRegisters);
> //
> // Place AP in HLT-loop
> //
> while (TRUE) {
> DisableInterrupts ();
> if (CpuMpData->SevEsIsEnabled) {
> MSR_SEV_ES_GHCB_REGISTER Msr;
> GHCB *Ghcb;
> UINT64 Status;
> BOOLEAN DoDecrement;
> BOOLEAN InterruptState;
>
> DoDecrement = (BOOLEAN) (CpuMpData->InitFlag == ApInitConfig);
>
> while (TRUE) {
> Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB);
> Ghcb = Msr.Ghcb;
>
> VmgInit (Ghcb, &InterruptState);
>
> if (DoDecrement) {
> DoDecrement = FALSE;
>
> //
> // Perform the delayed decrement just before issuing the first
> // VMGEXIT with AP_RESET_HOLD.
> //
> InterlockedDecrement ((UINT32 *) &CpuMpData->MpCpuExchangeInfo->NumApsExecuting);
> }
>
> Status = VmgExit (Ghcb, SVM_EXIT_AP_RESET_HOLD, 0, 0);
> if ((Status == 0) && (Ghcb->SaveArea.SwExitInfo2 != 0)) {
> VmgDone (Ghcb, InterruptState);
> break;
> }
>
> VmgDone (Ghcb, InterruptState);
> }
>
> //
> // Awakened in a new phase? Use the new CpuMpData
> //
> if (CpuMpData->NewCpuMpData != NULL) {
> CpuMpData = CpuMpData->NewCpuMpData;
> }
>
> MpInitLibSevEsAPReset (Ghcb, CpuMpData);
> } else {
> CpuSleep ();
> }
> CpuPause ();
> }
> }
> while (TRUE) {
> DisableInterrupts ();
> if (CpuMpData->ApLoopMode == ApInMwaitLoop) {
> //
> // Place AP in MWAIT-loop
> //
> AsmMonitor ((UINTN) ApStartupSignalBuffer, 0, 0);
> if (*ApStartupSignalBuffer != WAKEUP_AP_SIGNAL) {
> //
> // Check AP start-up signal again.
> // If AP start-up signal is not set, place AP into
> // the specified C-state
> //
> AsmMwait (CpuMpData->ApTargetCState << 4, 0);
> }
> } else if (CpuMpData->ApLoopMode == ApInRunLoop) {
> //
> // Place AP in Run-loop
> //
> CpuPause ();
> } else {
> ASSERT (FALSE);
> }
>
> //
> // If AP start-up signal is written, AP is waken up
> // otherwise place AP in loop again
> //
> if (*ApStartupSignalBuffer == WAKEUP_AP_SIGNAL) {
> break;
> }
> }
> }
> }
>
> /**
> Wait for AP wakeup and write AP start-up signal till AP is waken up.
>
> @param[in] ApStartupSignalBuffer Pointer to AP wakeup signal
> **/
Step#2: similarly, extract the SaveVolatileRegisters() call from its
current position, together with its gating condition. No change of
functionality. Expressed with the following (no-op) patch:
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index c13ddb5d9807..d450244738fa 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -720,257 +720,260 @@ EFIAPI
> ApWakeupFunction (
> IN MP_CPU_EXCHANGE_INFO *ExchangeInfo,
> IN UINTN ApIndex
> )
> {
> CPU_MP_DATA *CpuMpData;
> UINTN ProcessorNumber;
> EFI_AP_PROCEDURE Procedure;
> VOID *Parameter;
> UINT32 BistData;
> volatile UINT32 *ApStartupSignalBuffer;
> CPU_INFO_IN_HOB *CpuInfoInHob;
> UINT64 ApTopOfStack;
> UINTN CurrentApicMode;
>
> //
> // AP finished assembly code and begin to execute C code
> //
> CpuMpData = ExchangeInfo->CpuMpData;
>
> //
> // AP's local APIC settings will be lost after received INIT IPI
> // We need to re-initialize them at here
> //
> ProgramVirtualWireMode ();
> //
> // Mask the LINT0 and LINT1 so that AP doesn't enter the system timer interrupt handler.
> //
> DisableLvtInterrupts ();
> SyncLocalApicTimerSetting (CpuMpData);
>
> CurrentApicMode = GetApicMode ();
> while (TRUE) {
> if (CpuMpData->InitFlag == ApInitConfig) {
> //
> // Add CPU number
> //
> InterlockedIncrement ((UINT32 *) &CpuMpData->CpuCount);
> ProcessorNumber = ApIndex;
> //
> // This is first time AP wakeup, get BIST information from AP stack
> //
> ApTopOfStack = CpuMpData->Buffer + (ProcessorNumber + 1) * CpuMpData->CpuApStackSize;
> BistData = *(UINT32 *) ((UINTN) ApTopOfStack - sizeof (UINTN));
> //
> // CpuMpData->CpuData[0].VolatileRegisters is initialized based on BSP environment,
> // to initialize AP in InitConfig path.
> // NOTE: IDTR.BASE stored in CpuMpData->CpuData[0].VolatileRegisters points to a different IDT shared by all APs.
> //
> RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE);
> InitializeApData (CpuMpData, ProcessorNumber, BistData, ApTopOfStack);
> ApStartupSignalBuffer = CpuMpData->CpuData[ProcessorNumber].StartupApSignal;
> } else {
> //
> // Execute AP function if AP is ready
> //
> GetProcessorNumber (CpuMpData, &ProcessorNumber);
> //
> // Clear AP start-up signal when AP waken up
> //
> ApStartupSignalBuffer = CpuMpData->CpuData[ProcessorNumber].StartupApSignal;
> InterlockedCompareExchange32 (
> (UINT32 *) ApStartupSignalBuffer,
> WAKEUP_AP_SIGNAL,
> 0
> );
>
> if (CpuMpData->InitFlag == ApInitReconfig) {
> //
> // ApInitReconfig happens when:
> // 1. AP is re-enabled after it's disabled, in either PEI or DXE phase.
> // 2. AP is initialized in DXE phase.
> // In either case, use the volatile registers value derived from BSP.
> // NOTE: IDTR.BASE stored in CpuMpData->CpuData[0].VolatileRegisters points to a
> // different IDT shared by all APs.
> //
> RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE);
> } else {
> if (CpuMpData->ApLoopMode == ApInHltLoop) {
> //
> // Restore AP's volatile registers saved before AP is halted
> //
> RestoreVolatileRegisters (&CpuMpData->CpuData[ProcessorNumber].VolatileRegisters, TRUE);
> } else {
> //
> // The CPU driver might not flush TLB for APs on spot after updating
> // page attributes. AP in mwait loop mode needs to take care of it when
> // woken up.
> //
> CpuFlushTlb ();
> }
> }
>
> if (GetApState (&CpuMpData->CpuData[ProcessorNumber]) == CpuStateReady) {
> Procedure = (EFI_AP_PROCEDURE)CpuMpData->CpuData[ProcessorNumber].ApFunction;
> Parameter = (VOID *) CpuMpData->CpuData[ProcessorNumber].ApFunctionArgument;
> if (Procedure != NULL) {
> SetApState (&CpuMpData->CpuData[ProcessorNumber], CpuStateBusy);
> //
> // Enable source debugging on AP function
> //
> EnableDebugAgent ();
> //
> // Invoke AP function here
> //
> Procedure (Parameter);
> CpuInfoInHob = (CPU_INFO_IN_HOB *) (UINTN) CpuMpData->CpuInfoInHob;
> if (CpuMpData->SwitchBspFlag) {
> //
> // Re-get the processor number due to BSP/AP maybe exchange in AP function
> //
> GetProcessorNumber (CpuMpData, &ProcessorNumber);
> CpuMpData->CpuData[ProcessorNumber].ApFunction = 0;
> CpuMpData->CpuData[ProcessorNumber].ApFunctionArgument = 0;
> ApStartupSignalBuffer = CpuMpData->CpuData[ProcessorNumber].StartupApSignal;
> CpuInfoInHob[ProcessorNumber].ApTopOfStack = CpuInfoInHob[CpuMpData->NewBspNumber].ApTopOfStack;
> } else {
> if (CpuInfoInHob[ProcessorNumber].ApicId != GetApicId () ||
> CpuInfoInHob[ProcessorNumber].InitialApicId != GetInitialApicId ()) {
> if (CurrentApicMode != GetApicMode ()) {
> //
> // If APIC mode change happened during AP function execution,
> // we do not support APIC ID value changed.
> //
> ASSERT (FALSE);
> CpuDeadLoop ();
> } else {
> //
> // Re-get the CPU APICID and Initial APICID if they are changed
> //
> CpuInfoInHob[ProcessorNumber].ApicId = GetApicId ();
> CpuInfoInHob[ProcessorNumber].InitialApicId = GetInitialApicId ();
> }
> }
> }
> }
> SetApState (&CpuMpData->CpuData[ProcessorNumber], CpuStateFinished);
> }
> }
>
> 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);
> }
> }
>
> //
> // AP finished executing C code
> //
> InterlockedIncrement ((UINT32 *) &CpuMpData->FinishedCount);
>
> - //
> - // Place AP is specified loop mode
> - //
> if (CpuMpData->ApLoopMode == ApInHltLoop) {
> //
> // Save AP volatile registers
> //
> SaveVolatileRegisters (&CpuMpData->CpuData[ProcessorNumber].VolatileRegisters);
> + }
> +
> + //
> + // Place AP is specified loop mode
> + //
> + if (CpuMpData->ApLoopMode == ApInHltLoop) {
> //
> // Place AP in HLT-loop
> //
> while (TRUE) {
> DisableInterrupts ();
> if (CpuMpData->SevEsIsEnabled) {
> MSR_SEV_ES_GHCB_REGISTER Msr;
> GHCB *Ghcb;
> UINT64 Status;
> BOOLEAN DoDecrement;
> BOOLEAN InterruptState;
>
> DoDecrement = (BOOLEAN) (CpuMpData->InitFlag == ApInitConfig);
>
> while (TRUE) {
> Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB);
> Ghcb = Msr.Ghcb;
>
> VmgInit (Ghcb, &InterruptState);
>
> if (DoDecrement) {
> DoDecrement = FALSE;
>
> //
> // Perform the delayed decrement just before issuing the first
> // VMGEXIT with AP_RESET_HOLD.
> //
> InterlockedDecrement ((UINT32 *) &CpuMpData->MpCpuExchangeInfo->NumApsExecuting);
> }
>
> Status = VmgExit (Ghcb, SVM_EXIT_AP_RESET_HOLD, 0, 0);
> if ((Status == 0) && (Ghcb->SaveArea.SwExitInfo2 != 0)) {
> VmgDone (Ghcb, InterruptState);
> break;
> }
>
> VmgDone (Ghcb, InterruptState);
> }
>
> //
> // Awakened in a new phase? Use the new CpuMpData
> //
> if (CpuMpData->NewCpuMpData != NULL) {
> CpuMpData = CpuMpData->NewCpuMpData;
> }
>
> MpInitLibSevEsAPReset (Ghcb, CpuMpData);
> } else {
> CpuSleep ();
> }
> CpuPause ();
> }
> }
> while (TRUE) {
> DisableInterrupts ();
> if (CpuMpData->ApLoopMode == ApInMwaitLoop) {
> //
> // Place AP in MWAIT-loop
> //
> AsmMonitor ((UINTN) ApStartupSignalBuffer, 0, 0);
> if (*ApStartupSignalBuffer != WAKEUP_AP_SIGNAL) {
> //
> // Check AP start-up signal again.
> // If AP start-up signal is not set, place AP into
> // the specified C-state
> //
> AsmMwait (CpuMpData->ApTargetCState << 4, 0);
> }
> } else if (CpuMpData->ApLoopMode == ApInRunLoop) {
> //
> // Place AP in Run-loop
> //
> CpuPause ();
> } else {
> ASSERT (FALSE);
> }
>
> //
> // If AP start-up signal is written, AP is waken up
> // otherwise place AP in loop again
> //
> if (*ApStartupSignalBuffer == WAKEUP_AP_SIGNAL) {
> break;
> }
> }
> }
> }
>
> /**
> Wait for AP wakeup and write AP start-up signal till AP is waken up.
>
> @param[in] ApStartupSignalBuffer Pointer to AP wakeup signal
> **/
At this point, we can observe the order of operations back-to-back:
(i) decrement NumApsExecuting,
(ii) increment FinishedCount,
(iii) save volatile registers.
This is wrong, the order should be the reverse -- minimally, saving the
volatile registers should be the first operation. So pivot both of those
operations that are at the extremes, around the middle operation. That
is step#3, the only step that changes behavior:
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index d450244738fa..8b1f7f84bad6 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -720,260 +720,260 @@ EFIAPI
> ApWakeupFunction (
> IN MP_CPU_EXCHANGE_INFO *ExchangeInfo,
> IN UINTN ApIndex
> )
> {
> CPU_MP_DATA *CpuMpData;
> UINTN ProcessorNumber;
> EFI_AP_PROCEDURE Procedure;
> VOID *Parameter;
> UINT32 BistData;
> volatile UINT32 *ApStartupSignalBuffer;
> CPU_INFO_IN_HOB *CpuInfoInHob;
> UINT64 ApTopOfStack;
> UINTN CurrentApicMode;
>
> //
> // AP finished assembly code and begin to execute C code
> //
> CpuMpData = ExchangeInfo->CpuMpData;
>
> //
> // AP's local APIC settings will be lost after received INIT IPI
> // We need to re-initialize them at here
> //
> ProgramVirtualWireMode ();
> //
> // Mask the LINT0 and LINT1 so that AP doesn't enter the system timer interrupt handler.
> //
> DisableLvtInterrupts ();
> SyncLocalApicTimerSetting (CpuMpData);
>
> CurrentApicMode = GetApicMode ();
> while (TRUE) {
> if (CpuMpData->InitFlag == ApInitConfig) {
> //
> // Add CPU number
> //
> InterlockedIncrement ((UINT32 *) &CpuMpData->CpuCount);
> ProcessorNumber = ApIndex;
> //
> // This is first time AP wakeup, get BIST information from AP stack
> //
> ApTopOfStack = CpuMpData->Buffer + (ProcessorNumber + 1) * CpuMpData->CpuApStackSize;
> BistData = *(UINT32 *) ((UINTN) ApTopOfStack - sizeof (UINTN));
> //
> // CpuMpData->CpuData[0].VolatileRegisters is initialized based on BSP environment,
> // to initialize AP in InitConfig path.
> // NOTE: IDTR.BASE stored in CpuMpData->CpuData[0].VolatileRegisters points to a different IDT shared by all APs.
> //
> RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE);
> InitializeApData (CpuMpData, ProcessorNumber, BistData, ApTopOfStack);
> ApStartupSignalBuffer = CpuMpData->CpuData[ProcessorNumber].StartupApSignal;
> } else {
> //
> // Execute AP function if AP is ready
> //
> GetProcessorNumber (CpuMpData, &ProcessorNumber);
> //
> // Clear AP start-up signal when AP waken up
> //
> ApStartupSignalBuffer = CpuMpData->CpuData[ProcessorNumber].StartupApSignal;
> InterlockedCompareExchange32 (
> (UINT32 *) ApStartupSignalBuffer,
> WAKEUP_AP_SIGNAL,
> 0
> );
>
> if (CpuMpData->InitFlag == ApInitReconfig) {
> //
> // ApInitReconfig happens when:
> // 1. AP is re-enabled after it's disabled, in either PEI or DXE phase.
> // 2. AP is initialized in DXE phase.
> // In either case, use the volatile registers value derived from BSP.
> // NOTE: IDTR.BASE stored in CpuMpData->CpuData[0].VolatileRegisters points to a
> // different IDT shared by all APs.
> //
> RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE);
> } else {
> if (CpuMpData->ApLoopMode == ApInHltLoop) {
> //
> // Restore AP's volatile registers saved before AP is halted
> //
> RestoreVolatileRegisters (&CpuMpData->CpuData[ProcessorNumber].VolatileRegisters, TRUE);
> } else {
> //
> // The CPU driver might not flush TLB for APs on spot after updating
> // page attributes. AP in mwait loop mode needs to take care of it when
> // woken up.
> //
> CpuFlushTlb ();
> }
> }
>
> if (GetApState (&CpuMpData->CpuData[ProcessorNumber]) == CpuStateReady) {
> Procedure = (EFI_AP_PROCEDURE)CpuMpData->CpuData[ProcessorNumber].ApFunction;
> Parameter = (VOID *) CpuMpData->CpuData[ProcessorNumber].ApFunctionArgument;
> if (Procedure != NULL) {
> SetApState (&CpuMpData->CpuData[ProcessorNumber], CpuStateBusy);
> //
> // Enable source debugging on AP function
> //
> EnableDebugAgent ();
> //
> // Invoke AP function here
> //
> Procedure (Parameter);
> CpuInfoInHob = (CPU_INFO_IN_HOB *) (UINTN) CpuMpData->CpuInfoInHob;
> if (CpuMpData->SwitchBspFlag) {
> //
> // Re-get the processor number due to BSP/AP maybe exchange in AP function
> //
> GetProcessorNumber (CpuMpData, &ProcessorNumber);
> CpuMpData->CpuData[ProcessorNumber].ApFunction = 0;
> CpuMpData->CpuData[ProcessorNumber].ApFunctionArgument = 0;
> ApStartupSignalBuffer = CpuMpData->CpuData[ProcessorNumber].StartupApSignal;
> CpuInfoInHob[ProcessorNumber].ApTopOfStack = CpuInfoInHob[CpuMpData->NewBspNumber].ApTopOfStack;
> } else {
> if (CpuInfoInHob[ProcessorNumber].ApicId != GetApicId () ||
> CpuInfoInHob[ProcessorNumber].InitialApicId != GetInitialApicId ()) {
> if (CurrentApicMode != GetApicMode ()) {
> //
> // If APIC mode change happened during AP function execution,
> // we do not support APIC ID value changed.
> //
> ASSERT (FALSE);
> CpuDeadLoop ();
> } else {
> //
> // Re-get the CPU APICID and Initial APICID if they are changed
> //
> CpuInfoInHob[ProcessorNumber].ApicId = GetApicId ();
> CpuInfoInHob[ProcessorNumber].InitialApicId = GetInitialApicId ();
> }
> }
> }
> }
> SetApState (&CpuMpData->CpuData[ProcessorNumber], CpuStateFinished);
> }
> }
>
> + 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);
> }
> }
>
> - //
> - // AP finished executing C code
> - //
> - InterlockedIncrement ((UINT32 *) &CpuMpData->FinishedCount);
> -
> - if (CpuMpData->ApLoopMode == ApInHltLoop) {
> - //
> - // Save AP volatile registers
> - //
> - SaveVolatileRegisters (&CpuMpData->CpuData[ProcessorNumber].VolatileRegisters);
> - }
> -
> //
> // Place AP is specified loop mode
> //
> if (CpuMpData->ApLoopMode == ApInHltLoop) {
> //
> // Place AP in HLT-loop
> //
> while (TRUE) {
> DisableInterrupts ();
> if (CpuMpData->SevEsIsEnabled) {
> MSR_SEV_ES_GHCB_REGISTER Msr;
> GHCB *Ghcb;
> UINT64 Status;
> BOOLEAN DoDecrement;
> BOOLEAN InterruptState;
>
> DoDecrement = (BOOLEAN) (CpuMpData->InitFlag == ApInitConfig);
>
> while (TRUE) {
> Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB);
> Ghcb = Msr.Ghcb;
>
> VmgInit (Ghcb, &InterruptState);
>
> if (DoDecrement) {
> DoDecrement = FALSE;
>
> //
> // Perform the delayed decrement just before issuing the first
> // VMGEXIT with AP_RESET_HOLD.
> //
> InterlockedDecrement ((UINT32 *) &CpuMpData->MpCpuExchangeInfo->NumApsExecuting);
> }
>
> Status = VmgExit (Ghcb, SVM_EXIT_AP_RESET_HOLD, 0, 0);
> if ((Status == 0) && (Ghcb->SaveArea.SwExitInfo2 != 0)) {
> VmgDone (Ghcb, InterruptState);
> break;
> }
>
> VmgDone (Ghcb, InterruptState);
> }
>
> //
> // Awakened in a new phase? Use the new CpuMpData
> //
> if (CpuMpData->NewCpuMpData != NULL) {
> CpuMpData = CpuMpData->NewCpuMpData;
> }
>
> MpInitLibSevEsAPReset (Ghcb, CpuMpData);
> } else {
> CpuSleep ();
> }
> CpuPause ();
> }
> }
> while (TRUE) {
> DisableInterrupts ();
> if (CpuMpData->ApLoopMode == ApInMwaitLoop) {
> //
> // Place AP in MWAIT-loop
> //
> AsmMonitor ((UINTN) ApStartupSignalBuffer, 0, 0);
> if (*ApStartupSignalBuffer != WAKEUP_AP_SIGNAL) {
> //
> // Check AP start-up signal again.
> // If AP start-up signal is not set, place AP into
> // the specified C-state
> //
> AsmMwait (CpuMpData->ApTargetCState << 4, 0);
> }
> } else if (CpuMpData->ApLoopMode == ApInRunLoop) {
> //
> // Place AP in Run-loop
> //
> CpuPause ();
> } else {
> ASSERT (FALSE);
> }
>
> //
> // If AP start-up signal is written, AP is waken up
> // otherwise place AP in loop again
> //
> if (*ApStartupSignalBuffer == WAKEUP_AP_SIGNAL) {
> break;
> }
> }
> }
> }
>
> /**
> Wait for AP wakeup and write AP start-up signal till AP is waken up.
>
> @param[in] ApStartupSignalBuffer Pointer to AP wakeup signal
> **/
When we squash steps #1 through #3, we get *precisely* the posted patch.
A further improvement from the patch is that the relative order between
incrementing FinishedCount, and decrementing NumApsExecuting, is now
consistent between the SEV-ES-enabled and SEV-ES-disabled cases.
Regardless of SEV-ES, we now incrementing FinishedCount first, and
decrement NumApsExecuting second. Without the patch, the relative order
between these two operations gets inverted upon negating the SEV-ES
enablement.
I would prefer this patch to be broken up into the three steps that I
reconstructed above (steps #1 and #2 being no-op refactorings), but I
don't insist.
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Patch 1/1] UefiCpuPkg/Library/MpInitLib: Fix AP VolatileRegisters race condition
2021-01-22 17:10 [Patch 1/1] UefiCpuPkg/Library/MpInitLib: Fix AP VolatileRegisters race condition Michael D Kinney
2021-01-23 2:02 ` [edk2-devel] " Laszlo Ersek
@ 2021-01-23 5:10 ` Dong, Eric
2021-01-25 3:14 ` [edk2-devel] " Zeng, Star
2 siblings, 0 replies; 13+ messages in thread
From: Dong, Eric @ 2021-01-23 5:10 UTC (permalink / raw)
To: Kinney, Michael D, devel@edk2.groups.io
Cc: Ni, Ray, Laszlo Ersek, Kumar, Rahul1
Reviewed-by: Eric Dong <eric.dong@intel.com>
-----Original Message-----
From: Michael D Kinney <michael.d.kinney@intel.com>
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: [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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [Patch 1/1] UefiCpuPkg/Library/MpInitLib: Fix AP VolatileRegisters race condition
2021-01-22 17:10 [Patch 1/1] UefiCpuPkg/Library/MpInitLib: Fix AP VolatileRegisters race condition Michael D Kinney
2021-01-23 2:02 ` [edk2-devel] " Laszlo Ersek
2021-01-23 5:10 ` Dong, Eric
@ 2021-01-25 3:14 ` Zeng, Star
2021-01-25 5:15 ` Michael D Kinney
2 siblings, 1 reply; 13+ messages in thread
From: Zeng, Star @ 2021-01-25 3:14 UTC (permalink / raw)
To: devel@edk2.groups.io, Kinney, Michael D
Cc: Dong, Eric, Ni, Ray, Laszlo Ersek, Kumar, Rahul1, Zeng, Star
Does https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/MpInitLib/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
>
>
>
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [Patch 1/1] UefiCpuPkg/Library/MpInitLib: Fix AP VolatileRegisters race condition
2021-01-25 3:14 ` [edk2-devel] " Zeng, Star
@ 2021-01-25 5:15 ` Michael D Kinney
2021-01-25 8:43 ` Zeng, Star
0 siblings, 1 reply; 13+ messages in thread
From: Michael D Kinney @ 2021-01-25 5:15 UTC (permalink / raw)
To: Zeng, Star, devel@edk2.groups.io, Kinney, Michael D
Cc: Dong, Eric, Ni, Ray, Laszlo Ersek, Kumar, Rahul1
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/MpInitLib/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
> >
> >
> >
> >
> >
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [Patch 1/1] UefiCpuPkg/Library/MpInitLib: Fix AP VolatileRegisters race condition
2021-01-25 5:15 ` Michael D Kinney
@ 2021-01-25 8:43 ` Zeng, Star
2021-01-25 8:53 ` Ni, Ray
0 siblings, 1 reply; 13+ messages in thread
From: Zeng, Star @ 2021-01-25 8:43 UTC (permalink / raw)
To: Kinney, Michael D, devel@edk2.groups.io
Cc: Dong, Eric, Ni, Ray, Laszlo Ersek, Kumar, Rahul1, Zeng, Star
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
> > >
> > >
> > >
> > >
> > >
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [Patch 1/1] UefiCpuPkg/Library/MpInitLib: Fix AP VolatileRegisters race condition
2021-01-25 8:43 ` Zeng, Star
@ 2021-01-25 8:53 ` Ni, Ray
2021-01-25 11:04 ` Zeng, Star
0 siblings, 1 reply; 13+ messages in thread
From: Ni, Ray @ 2021-01-25 8:53 UTC (permalink / raw)
To: Zeng, Star, Kinney, Michael D, devel@edk2.groups.io
Cc: Dong, Eric, Laszlo Ersek, Kumar, Rahul1
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
> > > >
> > > >
> > > >
> > > >
> > > >
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [Patch 1/1] UefiCpuPkg/Library/MpInitLib: Fix AP VolatileRegisters race condition
2021-01-25 8:53 ` Ni, Ray
@ 2021-01-25 11:04 ` Zeng, Star
2021-01-25 21:17 ` Laszlo Ersek
0 siblings, 1 reply; 13+ messages in thread
From: Zeng, Star @ 2021-01-25 11:04 UTC (permalink / raw)
To: Ni, Ray, Kinney, Michael D, devel@edk2.groups.io
Cc: Dong, Eric, Laszlo Ersek, Kumar, Rahul1, Zeng, Star
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);
Thanks,
Star
> -----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
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [Patch 1/1] UefiCpuPkg/Library/MpInitLib: Fix AP VolatileRegisters race condition
2021-01-25 11:04 ` Zeng, Star
@ 2021-01-25 21:17 ` Laszlo Ersek
2021-01-26 1:18 ` Zeng, Star
2021-01-26 2:26 ` Ni, Ray
0 siblings, 2 replies; 13+ messages in thread
From: Laszlo Ersek @ 2021-01-25 21:17 UTC (permalink / raw)
To: Zeng, Star, Ni, Ray, Kinney, Michael D, devel@edk2.groups.io
Cc: Dong, Eric, Kumar, Rahul1
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
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [Patch 1/1] UefiCpuPkg/Library/MpInitLib: Fix AP VolatileRegisters race condition
2021-01-25 21:17 ` Laszlo Ersek
@ 2021-01-26 1:18 ` Zeng, Star
2021-01-26 2:26 ` Ni, Ray
1 sibling, 0 replies; 13+ messages in thread
From: Zeng, Star @ 2021-01-26 1:18 UTC (permalink / raw)
To: devel@edk2.groups.io, lersek@redhat.com, Ni, Ray,
Kinney, Michael D
Cc: Dong, Eric, Kumar, Rahul1, Zeng, Star
> -----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
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >
>
>
>
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [Patch 1/1] UefiCpuPkg/Library/MpInitLib: Fix AP VolatileRegisters race condition
2021-01-25 21:17 ` Laszlo Ersek
2021-01-26 1:18 ` Zeng, Star
@ 2021-01-26 2:26 ` Ni, Ray
2021-01-26 3:34 ` Zeng, Star
1 sibling, 1 reply; 13+ messages in thread
From: Ni, Ray @ 2021-01-26 2:26 UTC (permalink / raw)
To: Laszlo Ersek, Zeng, Star, Kinney, Michael D, devel@edk2.groups.io
Cc: Dong, Eric, Kumar, Rahul1
> > CpuMpData = (CPU_MP_DATA *) (Idtr.Base + Idtr.Limit + 1);
> > + ASSERT (CpuMpData != NULL);
In this case, Idtr.Base, Idtr.Limit both equal to zero and CpuMpData is 1.
ASSERT () cannot trigger assertion here.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [Patch 1/1] UefiCpuPkg/Library/MpInitLib: Fix AP VolatileRegisters race condition
2021-01-26 2:26 ` Ni, Ray
@ 2021-01-26 3:34 ` Zeng, Star
0 siblings, 0 replies; 13+ messages in thread
From: Zeng, Star @ 2021-01-26 3:34 UTC (permalink / raw)
To: Ni, Ray, Laszlo Ersek, Kinney, Michael D, devel@edk2.groups.io
Cc: Dong, Eric, Kumar, Rahul1, Zeng, Star
> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Tuesday, January 26, 2021 10:26 AM
> To: Laszlo Ersek <lersek@redhat.com>; 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>; Kumar, Rahul1
> <rahul1.kumar@intel.com>
> Subject: RE: [edk2-devel] [Patch 1/1] UefiCpuPkg/Library/MpInitLib: Fix AP
> VolatileRegisters race condition
>
> > > CpuMpData = (CPU_MP_DATA *) (Idtr.Base + Idtr.Limit + 1);
> > > + ASSERT (CpuMpData != NULL);
>
> In this case, Idtr.Base, Idtr.Limit both equal to zero and CpuMpData is 1.
> ASSERT () cannot trigger assertion here.
Oh, got it. Then the assert may be like below to check Idtr.Base.
ASSERT (Idtr.Base != 0);
Thanks,
Star
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [Patch 1/1] UefiCpuPkg/Library/MpInitLib: Fix AP VolatileRegisters race condition
2021-01-23 2:02 ` [edk2-devel] " Laszlo Ersek
@ 2021-01-26 6:47 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-01-26 6:47 UTC (permalink / raw)
To: devel, michael.d.kinney
Cc: lersek, Eric Dong, Ray Ni, Rahul Kumar, Tom Lendacky
Hi Michael,
On 1/23/21 3:02 AM, Laszlo Ersek wrote:
> +Tom, comments below
>
> On 01/22/21 18:10, Michael D Kinney wrote:
>> 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(-)
...
> When we squash steps #1 through #3, we get *precisely* the posted patch.
>
> A further improvement from the patch is that the relative order between
> incrementing FinishedCount, and decrementing NumApsExecuting, is now
> consistent between the SEV-ES-enabled and SEV-ES-disabled cases.
> Regardless of SEV-ES, we now incrementing FinishedCount first, and
> decrement NumApsExecuting second. Without the patch, the relative order
> between these two operations gets inverted upon negating the SEV-ES
> enablement.
>
> I would prefer this patch to be broken up into the three steps that I
> reconstructed above (steps #1 and #2 being no-op refactorings), but I
> don't insist.
I agree with Laszlo, it was easier to understand your changes looking
at his explanation. Eventually adding Star's assert() on top.
Regards,
Phil.
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-01-26 6:47 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-01-22 17:10 [Patch 1/1] UefiCpuPkg/Library/MpInitLib: Fix AP VolatileRegisters race condition Michael D Kinney
2021-01-23 2:02 ` [edk2-devel] " Laszlo Ersek
2021-01-26 6:47 ` Philippe Mathieu-Daudé
2021-01-23 5:10 ` Dong, Eric
2021-01-25 3:14 ` [edk2-devel] " Zeng, Star
2021-01-25 5:15 ` Michael D Kinney
2021-01-25 8:43 ` Zeng, Star
2021-01-25 8:53 ` Ni, Ray
2021-01-25 11:04 ` Zeng, Star
2021-01-25 21:17 ` Laszlo Ersek
2021-01-26 1:18 ` Zeng, Star
2021-01-26 2:26 ` Ni, Ray
2021-01-26 3:34 ` Zeng, Star
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox