On 10/17/15 18:15, Kinney, Michael D wrote: > Laszlo, > > I agree with many of your points. > > If you prefer, I can consolidate the UefiCpuPkg updates including 6 > items you list below into my UefiCpuPkg series and get that checked > in first so you can focus on OVMF SMM.
That's greatly appreciated; I can see them in your v4. Thank you! Laszlo > > Thanks, > > Mike > >> -----Original Message----- >> From: edk2-devel [mailto:[email protected]] On Behalf Of >> Laszlo Ersek >> Sent: Friday, October 16, 2015 11:04 PM >> To: Kinney, Michael D; [email protected] >> Subject: Re: [edk2] [PATCH v3 04/52] UefiCpuPkg: CpuDxe: broadcast MTRR >> changes to APs >> >> On 10/16/15 23:17, Kinney, Michael D wrote: >>> 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. >> >> Many thanks for this! >> >> I have two comments: >> >> >> First, I don't think that it is my patch "UefiCpuPkg: CpuDxe: broadcast >> MTRR changes to APs" that *introduces* the race condition, with the >> early call to StartupAllAps(). I believe that the race condition exists >> in the current code, and my patch only exposes it. >> >> Namely, my patch adds the StartupAllAps() call right before the MP >> services protocol interface is installed. Considering a case where my >> patch is not applied, but another driver registers a protocol notify >> callback for MP services, that callback would be invoked on the stack of >> InstallMultipleProtocolInterfaces(), while some of the APs might not >> have entered their idle loop yet. If the callback in that other driver >> located the protocol instance quickly, and called StartupAllAps() with >> similar arguments (which would be a valid thing to do), the same issue >> could be triggered. So I think the race is already there; the protocol >> interface gets installed too early. >> >> Which leads me to think that the code you add here under >> (!mStartupAllAPsCalled) should be executed unconditionally, right after >> "mAPsAlreadyInitFinished is set to TRUE that releases the APs to >> complete their initialization". We shouldn't install the MP services >> protocol interface until initialization is complete, and allows for any >> kinds of calls through that interface. >> >> >> Second, I think I'll put aside the SMM series for a short period, and >> work on a smaller UefiCpuPkg + OvmfPkg series. I'm thinking about six >> patches: >> >> (1) split out the crux of this patch (without the code rearrangement) >> and state that it fixes the existent (although mostly un-triggered) race >> condition >> >> (2) refine the above by introducing the timeout you mention. I'm thinking >> >> GetTimeInNanoSecond (GetPerformanceCounter ()) >> >> should be a good way to measure the elapsed time; it seems to be a >> pattern used elsewhere in the tree. This will also require a new PCD. >> What default would you suggest for the timeout? >> >> (3) add the rearrangement that gives more time to the APs to enter their >> idle loops >> >> (4) Include my MTRR broadcast patch >> >> (5) Separately, implement your other suggestion with >> gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds, from >> <http://thread.gmane.org/gmane.comp.bios.edk2.devel/2415/focus=141501>. >> >> (6) Xiao Guangrong provided us with the info for wording a commit for >> OvmfPkg where the timeout from (5) would be increased. >> >> Obviously I'll keep your authorship wherever appropriate. >> >> Once these are in, I'd resume working on the SMM series. >> >> >> ... Actually, what do you think about committing your v2 (or maybe v3) >> UefiCpuPkg series first? We've been trying to do many things in >> parallel, and it's getting a bit chaotic & hard to track. So I'm thinking: >> >> - your set goes in first >> - I'd post the series with the above 6 patches >> - once it's in, I'd return to the SMM series. >> >> What's your opinion? >> >> Thanks! >> Laszlo >> >>> >>> 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 _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
