* [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: [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
* 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
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