From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web12.1437.1611367349083724832 for ; Fri, 22 Jan 2021 18:02:29 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=TlgZRy79; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1611367348; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=7ca3OKYJ+brchYvIyjWFmUnjcjUSlznE6RGlw4dSMgU=; b=TlgZRy79F4JH9jxzhhcJXBqZy4qCHpJzmUiD/I/vHEmnuk74zcr77UvaOkcomjODFml5NX ABWN15xaSC/ci+rvjdGdquKShgHV15RcrfqPNHRbfP4pXtQtgDFo+4Ae0j0MtwVP0CCkd+ YSZb33VOFI7UolOoAnSNSvmHciljjWo= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-507-Ogj6K-GdM0aGtJ3rr0CVjw-1; Fri, 22 Jan 2021 21:02:24 -0500 X-MC-Unique: Ogj6K-GdM0aGtJ3rr0CVjw-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 6DCBEE744; Sat, 23 Jan 2021 02:02:22 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-81.ams2.redhat.com [10.36.113.81]) by smtp.corp.redhat.com (Postfix) with ESMTP id 46FF55D6CF; Sat, 23 Jan 2021 02:02:20 +0000 (UTC) Subject: Re: [edk2-devel] [Patch 1/1] UefiCpuPkg/Library/MpInitLib: Fix AP VolatileRegisters race condition To: devel@edk2.groups.io, michael.d.kinney@intel.com Cc: Eric Dong , Ray Ni , Rahul Kumar , Tom Lendacky References: <20210122171020.589-1-michael.d.kinney@intel.com> From: "Laszlo Ersek" Message-ID: Date: Sat, 23 Jan 2021 03:02:19 +0100 MIME-Version: 1.0 In-Reply-To: <20210122171020.589-1-michael.d.kinney@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit +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 > Cc: Ray Ni > Cc: Laszlo Ersek > Cc: Rahul Kumar > Signed-off-by: Michael D Kinney > --- > 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 Thanks Laszlo