From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id CE902210C1EDB for ; Wed, 25 Jul 2018 05:11:35 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 057CEB6324; Wed, 25 Jul 2018 12:11:35 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-225.rdu2.redhat.com [10.10.120.225]) by smtp.corp.redhat.com (Postfix) with ESMTP id 50E402026D68; Wed, 25 Jul 2018 12:11:34 +0000 (UTC) To: Eric Dong , edk2-devel@lists.01.org Cc: Ruiyu Ni References: <20180725075020.240-1-eric.dong@intel.com> <20180725075020.240-4-eric.dong@intel.com> From: Laszlo Ersek Message-ID: <80ecf1ce-a189-4a81-abb6-48eba2d3dbd9@redhat.com> Date: Wed, 25 Jul 2018 14:11:33 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <20180725075020.240-4-eric.dong@intel.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.2]); Wed, 25 Jul 2018 12:11:35 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.2]); Wed, 25 Jul 2018 12:11:35 +0000 (UTC) for IP:'10.11.54.4' DOMAIN:'int-mx04.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [Patch v3 3/3] UefiCpuPkg/MpInitLib: Not use disabled AP when call StartAllAPs. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 25 Jul 2018 12:11:36 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > Cc: Ruiyu Ni > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Eric Dong > --- > 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? 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)." 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 > ); > > /** >