Laszlo, Here is a proposed fix on top of your v3 patch services to address a race condition that was introduced by the v3 patch series call to StartupAllAPs() with SingleThread set to TRUE in the MP initialization. If the APs have not entered their idle loop before StartupAllAPs() is called, then some of the APs will be in an unexpected state, and StartupAllAPs() will hang. This is the hang condition that is only seen when SingleThread parameter is set to TRUE and StartupAllAPs() is called very shortly after mAPsAlreadyInitFinished is set to TRUE that releases the APs to complete their initialization.
I added an internal function that checks to see if all APs are in the sleeping state in the idle loop. On the first call to StartupAllAPs(), this internal function is used to make sure the APs are in a state that is compatible with StartupAllAPs() being used. I put this check in the first call to StartupAllAPs(), so we do not take the delay to wait for the APs unconditionally in the MP init code. Other work can get done while the APs work their way to their idle loop sleeping state. The one item remaining is to have a timeout with an ASSERT() if timeout is triggered in first call to StartupAllAPs() waiting for the APs to enter idle loop. I also reordered some of the actions InitializeMpSupport(), so the MP Services Protocol and call to StartupAllAPs() are done as late as possible to give APs more time to get to idle loop. Please merge this into your patch series. Best regards, Mike Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Michael Kinney <[email protected]> 24d1f93110e1f73c70ff3705950c119693328deb UefiCpuPkg/CpuDxe/CpuMp.c | 93 ++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 75 insertions(+), 18 deletions(-) diff --git a/UefiCpuPkg/CpuDxe/CpuMp.c b/UefiCpuPkg/CpuDxe/CpuMp.c index fbe43f5..4af0361 100644 --- a/UefiCpuPkg/CpuDxe/CpuMp.c +++ b/UefiCpuPkg/CpuDxe/CpuMp.c @@ -29,6 +29,7 @@ VOID *mApStackStart = 0; volatile BOOLEAN mAPsAlreadyInitFinished = FALSE; volatile BOOLEAN mStopCheckAllAPsStatus = TRUE; +BOOLEAN mStartupAllAPsCalled = FALSE; EFI_MP_SERVICES_PROTOCOL mMpServicesTemplate = { GetNumberOfProcessors, @@ -313,6 +314,47 @@ CheckAndUpdateAllAPsToIdleState ( } /** + Check if all APs are in state CpuStateSleeping. + + Return TRUE if all APs are in the CpuStateSleeping state. Do not + check the state of the BSP or any disabled APs. + + @retval TRUE All APs are in CpuStateSleeping state. + @retval FALSE One or more APs are not in CpuStateSleeping state. + +**/ +BOOLEAN +CheckAllAPsSleeping ( + VOID + ) +{ + UINTN ProcessorNumber; + CPU_DATA_BLOCK *CpuData; + + for (ProcessorNumber = 0; ProcessorNumber < mMpSystemData.NumberOfProcessors; ProcessorNumber++) { + CpuData = &mMpSystemData.CpuDatas[ProcessorNumber]; + if (TestCpuStatusFlag (CpuData, PROCESSOR_AS_BSP_BIT)) { + // + // Skip BSP + // + continue; + } + + if (!TestCpuStatusFlag (CpuData, PROCESSOR_ENABLED_BIT)) { + // + // Skip Disabled processors + // + continue; + } + + if (GetApState (CpuData) != CpuStateSleeping) { + return FALSE; + } + } + return TRUE; +} + +/** If the timeout expires before all APs returns from Procedure, we should forcibly terminate the executing AP and fill FailedList back by StartupAllAPs(). @@ -647,6 +689,18 @@ StartupAllAPs ( // mStopCheckAllAPsStatus = TRUE; + if (!mStartupAllAPsCalled) { + // + // If this is first call to StartAllAPs(), then + // wait for all APs to enter idle loop. + // + while (!CheckAllAPsSleeping ()) { + CpuPause(); + } + + mStartupAllAPsCalled = TRUE; + } + for (Number = 0; Number < mMpSystemData.NumberOfProcessors; Number++) { CpuData = &mMpSystemData.CpuDatas[Number]; if (TestCpuStatusFlag (CpuData, PROCESSOR_AS_BSP_BIT)) { @@ -1700,6 +1754,9 @@ InitializeMpSupport ( sizeof (CPU_DATA_BLOCK) * mMpSystemData.NumberOfProcessors, mMpSystemData.CpuDatas); + // + // Release all APs to complete initialization and enter idle loop + // mAPsAlreadyInitFinished = TRUE; // @@ -1707,6 +1764,23 @@ InitializeMpSupport ( // CollectBistDataFromHob (); + if (mMpSystemData.NumberOfProcessors > 1 && mMpSystemData.NumberOfProcessors < gMaxLogicalProcessorNumber) { + if (mApStackStart != NULL) { + FreePages (mApStackStart, EFI_SIZE_TO_PAGES ( + (gMaxLogicalProcessorNumber - mMpSystemData.NumberOfProcessors) * + gApStackSize)); + } + } + + Status = gBS->CreateEvent ( + EVT_SIGNAL_EXIT_BOOT_SERVICES, + TPL_CALLBACK, + ExitBootServicesCallback, + NULL, + &mExitBootServicesEvent + ); + ASSERT_EFI_ERROR (Status); + // // Synchronize MTRR settings to APs. // @@ -1721,28 +1795,11 @@ InitializeMpSupport ( NULL // FailedCpuList ); ASSERT (Status == EFI_SUCCESS || Status == EFI_NOT_STARTED); - + Status = gBS->InstallMultipleProtocolInterfaces ( &mMpServiceHandle, &gEfiMpServiceProtocolGuid, &mMpServicesTemplate, NULL ); ASSERT_EFI_ERROR (Status); - - if (mMpSystemData.NumberOfProcessors > 1 && mMpSystemData.NumberOfProcessors < gMaxLogicalProcessorNumber) { - if (mApStackStart != NULL) { - FreePages (mApStackStart, EFI_SIZE_TO_PAGES ( - (gMaxLogicalProcessorNumber - mMpSystemData.NumberOfProcessors) * - gApStackSize)); - } - } - - Status = gBS->CreateEvent ( - EVT_SIGNAL_EXIT_BOOT_SERVICES, - TPL_CALLBACK, - ExitBootServicesCallback, - NULL, - &mExitBootServicesEvent - ); - ASSERT_EFI_ERROR (Status); } >-----Original Message----- >From: edk2-devel [mailto:[email protected]] On Behalf Of >Laszlo Ersek >Sent: Wednesday, October 14, 2015 3:26 PM >To: [email protected] >Subject: [edk2] [PATCH v3 04/52] UefiCpuPkg: CpuDxe: broadcast MTRR >changes to APs > >The > > Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/CpuArchDxe > >driver applies any MTRR changes to APs, if the EFI_MP_SERVICES_PROTOCOL >is >available. We should do the same. > >Additionally, the broadcast should occur at MP startup as well, not only >when MTRR settings are changed. The inspiration is taken from > > Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/CpuMpDxe/ > >(see the EarlyMpInit() function and its call sites in >"ProcessorConfig.c"). > >Contributed-under: TianoCore Contribution Agreement 1.0 >Signed-off-by: Laszlo Ersek <[email protected]> >Reviewed-by: Jeff Fan <[email protected]> >--- > >Notes: > v3: > - Although Jeff reviewed this, I'm not committing it just yet, because > contextually it is on top of Mike's pending patches. Picked up the R-b > tag from Jeff though. No code changes. > > v2: > - This patch replaces the following patches from v1: > - UefiCpuPkg: CpuDxe: optionally save MTRR settings to AcpiNVS memory > block > - UefiCpuPkg: CpuDxe: broadcast MTRR changes to APs > - UefiCpuPkg: CpuDxe: sync MTRR settings to APs at MP startup > - UefiCpuPkg: CpuDxe: provide EFI_MP_SERVICES_PROTOCOL when there's >no > AP > > The first v1 patch was deemed inappropriate for general use, and Mike > suggested a good alternative for OVMF (=> grab MTRR settings in > CpuS3DataDxe at EndOfDxe). > > The second and third v1 patches are now squashed together into this v2 > patch; they are small and belong together logically. > > The fourth v1 patch is redundant now; the same has been covered by > Mike. > > UefiCpuPkg/CpuDxe/CpuMp.h | 13 ++++++++ > UefiCpuPkg/CpuDxe/CpuDxe.c | 26 +++++++++++++++ > UefiCpuPkg/CpuDxe/CpuMp.c | 34 +++++++++++++++++++- > 3 files changed, 72 insertions(+), 1 deletion(-) > >diff --git a/UefiCpuPkg/CpuDxe/CpuMp.h b/UefiCpuPkg/CpuDxe/CpuMp.h >index d2866e4..503f3ae 100644 >--- a/UefiCpuPkg/CpuDxe/CpuMp.h >+++ b/UefiCpuPkg/CpuDxe/CpuMp.h >@@ -643,5 +643,18 @@ ResetApStackless ( > IN UINT32 ProcessorId > ); > >+/** >+ A minimal wrapper function that allows MtrrSetAllMtrrs() to be passed to >+ EFI_MP_SERVICES_PROTOCOL.StartupAllAPs() as Procedure. >+ >+ @param[in] Buffer Pointer to an MTRR_SETTINGS object, to be passed to >+ MtrrSetAllMtrrs(). >+**/ >+VOID >+EFIAPI >+SetMtrrsFromBuffer ( >+ IN VOID *Buffer >+ ); >+ > #endif // _CPU_MP_H_ > >diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.c b/UefiCpuPkg/CpuDxe/CpuDxe.c >index c9df4e1..daf97bd 100644 >--- a/UefiCpuPkg/CpuDxe/CpuDxe.c >+++ b/UefiCpuPkg/CpuDxe/CpuDxe.c >@@ -350,6 +350,9 @@ CpuSetMemoryAttributes ( > { > RETURN_STATUS Status; > MTRR_MEMORY_CACHE_TYPE CacheType; >+ EFI_STATUS MpStatus; >+ EFI_MP_SERVICES_PROTOCOL *MpService; >+ MTRR_SETTINGS MtrrSettings; > > if (!IsMtrrSupported ()) { > return EFI_UNSUPPORTED; >@@ -405,6 +408,29 @@ CpuSetMemoryAttributes ( > CacheType > ); > >+ if (!RETURN_ERROR (Status)) { >+ MpStatus = gBS->LocateProtocol ( >+ &gEfiMpServiceProtocolGuid, >+ NULL, >+ (VOID **)&MpService >+ ); >+ // >+ // Synchronize the update with all APs >+ // >+ if (!EFI_ERROR (MpStatus)) { >+ MtrrGetAllMtrrs (&MtrrSettings); >+ MpStatus = MpService->StartupAllAPs ( >+ MpService, // This >+ SetMtrrsFromBuffer, // Procedure >+ TRUE, // SingleThread >+ NULL, // WaitEvent >+ 0, // TimeoutInMicrosecsond >+ &MtrrSettings, // ProcedureArgument >+ NULL // FailedCpuList >+ ); >+ ASSERT (MpStatus == EFI_SUCCESS || MpStatus == EFI_NOT_STARTED); >+ } >+ } > return (EFI_STATUS) Status; > } > >diff --git a/UefiCpuPkg/CpuDxe/CpuMp.c b/UefiCpuPkg/CpuDxe/CpuMp.c >index da3686e..fbe43f5 100644 >--- a/UefiCpuPkg/CpuDxe/CpuMp.c >+++ b/UefiCpuPkg/CpuDxe/CpuMp.c >@@ -1626,6 +1626,22 @@ ExitBootServicesCallback ( > } > > /** >+ A minimal wrapper function that allows MtrrSetAllMtrrs() to be passed to >+ EFI_MP_SERVICES_PROTOCOL.StartupAllAPs() as Procedure. >+ >+ @param[in] Buffer Pointer to an MTRR_SETTINGS object, to be passed to >+ MtrrSetAllMtrrs(). >+**/ >+VOID >+EFIAPI >+SetMtrrsFromBuffer ( >+ IN VOID *Buffer >+ ) >+{ >+ MtrrSetAllMtrrs (Buffer); >+} >+ >+/** > Initialize Multi-processor support. > > **/ >@@ -1634,7 +1650,8 @@ InitializeMpSupport ( > VOID > ) > { >- EFI_STATUS Status; >+ EFI_STATUS Status; >+ MTRR_SETTINGS MtrrSettings; > > gMaxLogicalProcessorNumber = (UINTN) PcdGet32 >(PcdCpuMaxLogicalProcessorNumber); > if (gMaxLogicalProcessorNumber < 1) { >@@ -1690,6 +1707,21 @@ InitializeMpSupport ( > // > CollectBistDataFromHob (); > >+ // >+ // Synchronize MTRR settings to APs. >+ // >+ MtrrGetAllMtrrs (&MtrrSettings); >+ Status = mMpServicesTemplate.StartupAllAPs ( >+ &mMpServicesTemplate, // This >+ SetMtrrsFromBuffer, // Procedure >+ TRUE, // SingleThread >+ NULL, // WaitEvent >+ 0, // >TimeoutInMicrosecsond >+ &MtrrSettings, // ProcedureArgument >+ NULL // FailedCpuList >+ ); >+ ASSERT (Status == EFI_SUCCESS || Status == EFI_NOT_STARTED); >+ > Status = gBS->InstallMultipleProtocolInterfaces ( > &mMpServiceHandle, > &gEfiMpServiceProtocolGuid, &mMpServicesTemplate, >-- >1.8.3.1 > > >_______________________________________________ >edk2-devel mailing list >[email protected] >https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
