[edk2-devel] [Patch 1/1] UefiCpuPkg/Library/MpInitLib: Fix AP VolatileRegisters race condition
Laszlo Ersek
lersek at redhat.com
Sat Jan 23 02:02:19 UTC 2021
+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 at intel.com>
> Cc: Ray Ni <ray.ni at intel.com>
> Cc: Laszlo Ersek <lersek at redhat.com>
> Cc: Rahul Kumar <rahul1.kumar at intel.com>
> Signed-off-by: Michael D Kinney <michael.d.kinney at intel.com>
> ---
> UefiCpuPkg/Library/MpInitLib/MpLib.c | 31 ++++++++++++++++------------
> 1 file changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 681fa79b4cff..8b1f7f84bad6 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -769,15 +769,6 @@ ApWakeupFunction (
> RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE);
> InitializeApData (CpuMpData, ProcessorNumber, BistData, ApTopOfStack);
> ApStartupSignalBuffer = CpuMpData->CpuData[ProcessorNumber].StartupApSignal;
> -
> - //
> - // Delay decrementing the APs executing count when SEV-ES is enabled
> - // to allow the APs to issue an AP_RESET_HOLD before the BSP possibly
> - // performs another INIT-SIPI-SIPI sequence.
> - //
> - if (!CpuMpData->SevEsIsEnabled) {
> - InterlockedDecrement ((UINT32 *) &CpuMpData->MpCpuExchangeInfo->NumApsExecuting);
> - }
> } else {
> //
> // Execute AP function if AP is ready
> @@ -866,19 +857,33 @@ ApWakeupFunction (
> }
> }
>
> + if (CpuMpData->ApLoopMode == ApInHltLoop) {
> + //
> + // Save AP volatile registers
> + //
> + SaveVolatileRegisters (&CpuMpData->CpuData[ProcessorNumber].VolatileRegisters);
> + }
> +
> //
> // AP finished executing C code
> //
> InterlockedIncrement ((UINT32 *) &CpuMpData->FinishedCount);
>
> + if (CpuMpData->InitFlag == ApInitConfig) {
> + //
> + // Delay decrementing the APs executing count when SEV-ES is enabled
> + // to allow the APs to issue an AP_RESET_HOLD before the BSP possibly
> + // performs another INIT-SIPI-SIPI sequence.
> + //
> + if (!CpuMpData->SevEsIsEnabled) {
> + InterlockedDecrement ((UINT32 *) &CpuMpData->MpCpuExchangeInfo->NumApsExecuting);
> + }
> + }
> +
> //
> // Place AP is specified loop mode
> //
> if (CpuMpData->ApLoopMode == ApInHltLoop) {
> - //
> - // Save AP volatile registers
> - //
> - SaveVolatileRegisters (&CpuMpData->CpuData[ProcessorNumber].VolatileRegisters);
> //
> // Place AP in HLT-loop
> //
>
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 at redhat.com>
Thanks
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70700): https://edk2.groups.io/g/devel/message/70700
Mute This Topic: https://groups.io/mt/80035510/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list