Hi Laszlo,
> -----Original Message----- > From: Laszlo Ersek [mailto:[email protected]] > Sent: Wednesday, July 25, 2018 8:12 PM > To: Dong, Eric <[email protected]>; [email protected] > Cc: Ni, Ruiyu <[email protected]> > Subject: Re: [edk2] [Patch v3 3/3] UefiCpuPkg/MpInitLib: Not use disabled AP > when call StartAllAPs. > > Hi Eric, > > On 07/25/18 09:50, Eric Dong wrote: > > Base on UEFI spec requirement, StartAllAPs function should not use the > > APs which has been disabled before. This patch just change current > > code to follow this rule. > > > > V3 changes: > > Only called by StartUpAllAps, WakeUpAp will not wake up the disabled > > APs, in other cases also need to include the disabled APs, such as > > CpuDxe driver start up and ChangeApLoopCallback function. > > > > Cc: Laszlo Ersek <[email protected]> > > Cc: Ruiyu Ni <[email protected]> > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Eric Dong <[email protected]> > > --- > > UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 2 +- > > UefiCpuPkg/Library/MpInitLib/MpLib.c | 27 +++++++++++++++++---------- > > UefiCpuPkg/Library/MpInitLib/MpLib.h | 4 +++- > > 3 files changed, 21 insertions(+), 12 deletions(-) > > > > diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > > b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > > index d82e9aea45..c17daa0fc0 100644 > > --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > > +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > > @@ -306,7 +306,7 @@ MpInitChangeApLoopCallback ( > > CpuMpData->PmCodeSegment = GetProtectedModeCS (); > > CpuMpData->ApLoopMode = PcdGet8 (PcdCpuApLoopMode); > > mNumberToFinish = CpuMpData->CpuCount - 1; > > - WakeUpAP (CpuMpData, TRUE, 0, RelocateApLoop, NULL); > > + WakeUpAP (CpuMpData, TRUE, 0, RelocateApLoop, NULL, TRUE); > > while (mNumberToFinish > 0) { > > CpuPause (); > > } > > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c > > b/UefiCpuPkg/Library/MpInitLib/MpLib.c > > index 0e57cc86bf..1a81062a3f 100644 > > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > > @@ -470,7 +470,7 @@ CollectProcessorCount ( > > // > > CpuMpData->InitFlag = ApInitConfig; > > CpuMpData->X2ApicEnable = FALSE; > > - WakeUpAP (CpuMpData, TRUE, 0, NULL, NULL); > > + WakeUpAP (CpuMpData, TRUE, 0, NULL, NULL, TRUE); > > CpuMpData->InitFlag = ApInitDone; > > ASSERT (CpuMpData->CpuCount <= PcdGet32 > (PcdCpuMaxLogicalProcessorNumber)); > > // > > @@ -491,7 +491,7 @@ CollectProcessorCount ( > > // > > // Wakeup all APs to enable x2APIC mode > > // > > - WakeUpAP (CpuMpData, TRUE, 0, ApFuncEnableX2Apic, NULL); > > + WakeUpAP (CpuMpData, TRUE, 0, ApFuncEnableX2Apic, NULL, TRUE); > > // > > // Wait for all known APs finished > > // > > @@ -969,6 +969,7 @@ FreeResetVector ( > > @param[in] ProcessorNumber The handle number of specified > processor > > @param[in] Procedure The function to be invoked by AP > > @param[in] ProcedureArgument The argument to be passed into AP > > function > > + @param[in] WakeUpDisabledAps Whether need to wake up disabled APs > in broadcast mode. > > **/ > > VOID > > WakeUpAP ( > > @@ -976,7 +977,8 @@ WakeUpAP ( > > IN BOOLEAN Broadcast, > > IN UINTN ProcessorNumber, > > IN EFI_AP_PROCEDURE Procedure, OPTIONAL > > - IN VOID *ProcedureArgument OPTIONAL > > + IN VOID *ProcedureArgument, OPTIONAL > > + IN BOOLEAN WakeUpDisabledAps > > ) > > { > > volatile MP_CPU_EXCHANGE_INFO *ExchangeInfo; > > @@ -1010,6 +1012,10 @@ WakeUpAP ( > > for (Index = 0; Index < CpuMpData->CpuCount; Index++) { > > if (Index != CpuMpData->BspNumber) { > > CpuData = &CpuMpData->CpuData[Index]; > > + if (GetApState (CpuData) == CpuStateDisabled > && !WakeUpDisabledAps) { > > + continue; > > + } > > + > > CpuData->ApFunction = (UINTN) Procedure; > > CpuData->ApFunctionArgument = (UINTN) ProcedureArgument; > > SetApState (CpuData, CpuStateReady); > > I think something *might* be missing from the patch here, but I'm not sure. > > Namely, is it possible that we can reach WakeUpAP() with: > > (Broadcast && WakeUpDisabledAps) > > *after* some APs have been disabled? > > Because, in that case, we set the AP state from Disabled to Ready (and the AP > will perform the Procedure as necessary) -- however, once the AP is done, we > do not seem to re-set its state to Disabled. > > Is that correct? Yes, for current use cases, we don't have a valid case need to re-set the AP to Disabled state. > > I tried to collect all the WakeUpAp() calls that pass TRUE for both booleans > mentioned above. Their callers are as follows: > > - MpInitLibInitialize() > - CollectProcessorCount() > - MpInitChangeApLoopCallback() > > From these three, MpInitLibInitialize() and CollectProcessorCount() run > before the protocol or PPI user has any chance to disable a CPU, so it looks > impossible to reach the problematic code path I'm describing from those > functions. > > What about MpInitChangeApLoopCallback()?... Hm, that function is only > called as a notification function for: > > - the exit-boot-services event, and > - the legacy boot event > > After which the MP protocol is unusable anyway, so it doesn't matter if we > don't flip the AP status back to Disabled. > > OK, so this patch looks correct to me; can you please update the commit > message: > > "WakeUpAP() is called with (Broadcast && WakeUpDisabledAps) from > MpInitLibInitialize(), CollectProcessorCount() and > MpInitChangeApLoopCallback() only. The first two run before the PPI or > Protocol user has a chance to disable any APs. The last one runs in response > to > the ExitBootServices and LegacyBoot events, after which the MP protocol is > unusable. For this reason, it doesn't matter that an originally disabled AP's > state is not restored to Disabled, when > WakeUpAP() is called with (Broadcast && WakeUpDisabledAps)." > Good suggestion, will include it in the commit messages. > Thanks! > Laszlo > > > @@ -1289,7 +1295,7 @@ ResetProcessorToIdleState ( > > CpuMpData = GetCpuMpData (); > > > > CpuMpData->InitFlag = ApInitReconfig; > > - WakeUpAP (CpuMpData, FALSE, ProcessorNumber, NULL, NULL); > > + WakeUpAP (CpuMpData, FALSE, ProcessorNumber, NULL, NULL, TRUE); > > while (CpuMpData->FinishedCount < 1) { > > CpuPause (); > > } > > @@ -1439,7 +1445,8 @@ CheckAllAPs ( > > FALSE, > > (UINT32) NextProcessorNumber, > > CpuMpData->Procedure, > > - CpuMpData->ProcArguments > > + CpuMpData->ProcArguments, > > + TRUE > > ); > > } > > } > > @@ -1711,7 +1718,7 @@ MpInitLibInitialize ( > > // > > // Wakeup APs to do some AP initialize sync > > // > > - WakeUpAP (CpuMpData, TRUE, 0, ApInitializeSync, CpuMpData); > > + WakeUpAP (CpuMpData, TRUE, 0, ApInitializeSync, CpuMpData, > > + TRUE); > > // > > // Wait for all APs finished initialization > > // > > @@ -1906,7 +1913,7 @@ SwitchBSPWorker ( > > // > > // Need to wakeUp AP (future BSP). > > // > > - WakeUpAP (CpuMpData, FALSE, ProcessorNumber, FutureBSPProc, > > CpuMpData); > > + WakeUpAP (CpuMpData, FALSE, ProcessorNumber, FutureBSPProc, > > + CpuMpData, TRUE); > > > > AsmExchangeRole (&CpuMpData->BSPInfo, &CpuMpData->APInfo); > > > > @@ -2240,14 +2247,14 @@ StartupAllAPsWorker ( > > CpuMpData->WaitEvent = WaitEvent; > > > > if (!SingleThread) { > > - WakeUpAP (CpuMpData, TRUE, 0, Procedure, ProcedureArgument); > > + WakeUpAP (CpuMpData, TRUE, 0, Procedure, ProcedureArgument, > > + FALSE); > > } else { > > for (ProcessorNumber = 0; ProcessorNumber < ProcessorCount; > ProcessorNumber++) { > > if (ProcessorNumber == CallerNumber) { > > continue; > > } > > if (CpuMpData->CpuData[ProcessorNumber].Waiting) { > > - WakeUpAP (CpuMpData, FALSE, ProcessorNumber, Procedure, > ProcedureArgument); > > + WakeUpAP (CpuMpData, FALSE, ProcessorNumber, Procedure, > > + ProcedureArgument, TRUE); > > break; > > } > > } > > @@ -2359,7 +2366,7 @@ StartupThisAPWorker ( > > CpuData->ExpectedTime = CalculateTimeout (TimeoutInMicroseconds, > &CpuData->CurrentTime); > > CpuData->TotalTime = 0; > > > > - WakeUpAP (CpuMpData, FALSE, ProcessorNumber, Procedure, > > ProcedureArgument); > > + WakeUpAP (CpuMpData, FALSE, ProcessorNumber, Procedure, > > + ProcedureArgument, TRUE); > > > > // > > // If WaitEvent is NULL, execute in blocking mode. > > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h > > b/UefiCpuPkg/Library/MpInitLib/MpLib.h > > index 5002b7e9c0..fe7a917e49 100644 > > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h > > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h > > @@ -373,6 +373,7 @@ GetModeTransitionBuffer ( > > @param[in] ProcessorNumber The handle number of specified > processor > > @param[in] Procedure The function to be invoked by AP > > @param[in] ProcedureArgument The argument to be passed into AP > > function > > + @param[in] WakeUpDisabledAps Whether need to wake up disabled APs > in broadcast mode. > > **/ > > VOID > > WakeUpAP ( > > @@ -380,7 +381,8 @@ WakeUpAP ( > > IN BOOLEAN Broadcast, > > IN UINTN ProcessorNumber, > > IN EFI_AP_PROCEDURE Procedure, OPTIONAL > > - IN VOID *ProcedureArgument OPTIONAL > > + IN VOID *ProcedureArgument, OPTIONAL > > + IN BOOLEAN WakeUpDisabledAps OPTIONAL > > ); > > > > /** > > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
