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 4039A21BADAB2 for ; Thu, 19 Jul 2018 04:56:56 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 08BB740753A4; Thu, 19 Jul 2018 11:56:56 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-95.rdu2.redhat.com [10.10.120.95]) by smtp.corp.redhat.com (Postfix) with ESMTP id E9C501C5AA; Thu, 19 Jul 2018 11:56:54 +0000 (UTC) To: Eric Dong , edk2-devel@lists.01.org Cc: Ruiyu Ni References: <20180718051009.19748-1-eric.dong@intel.com> From: Laszlo Ersek Message-ID: Date: Thu, 19 Jul 2018 13:56:53 +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: <20180718051009.19748-1-eric.dong@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Thu, 19 Jul 2018 11:56:56 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Thu, 19 Jul 2018 11:56:56 +0000 (UTC) for IP:'10.11.54.5' DOMAIN:'int-mx05.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: UefiCpuPkg/MpInitLib: Fix S3 resume hang issue. 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: Thu, 19 Jul 2018 11:56:57 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hi, On 07/18/18 07:10, Eric Dong wrote: > When resume from S3 and CPU loop mode is MWait mode, > if driver calls APs to do task at EndOfPei point, the > APs can't been wake up and bios hang at that point. > > The root cause is PiSmmCpuDxeSmm driver wakes up APs > with HLT mode during S3 resume phase to do SMM relocation. > After this task, PiSmmCpuDxeSmm driver not restore APs > context which make CpuMpPei driver saved wake up buffer > not works. > > The solution for this issue is let CpuMpPei driver hook > S3SmmInitDone ppi notification. In this notify function, > it check whether Cpu Loop mode is not HLT mode. If yes, > CpuMpPei driver will set a flag to force BSP use INIT-SIPI > -SIPI command to wake up the APs. > > Cc: Laszlo Ersek > Cc: Ruiyu Ni > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Eric Dong > --- > UefiCpuPkg/Library/MpInitLib/MpLib.c | 16 ++++- > UefiCpuPkg/Library/MpInitLib/MpLib.h | 9 +++ > UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf | 4 ++ > UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 89 +++++++++++++++++++++++++++ > 4 files changed, 116 insertions(+), 2 deletions(-) I see this patch is already committed (58942277bcbf); I have two comments in retrospect: > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c > index 722db2a01f..e5c701ddeb 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > @@ -985,13 +985,15 @@ WakeUpAP ( > CpuMpData->FinishedCount = 0; > ResetVectorRequired = FALSE; > > - if (CpuMpData->ApLoopMode == ApInHltLoop || > + if (CpuMpData->WakeUpByInitSipiSipi || > CpuMpData->InitFlag != ApInitDone) { > ResetVectorRequired = TRUE; > AllocateResetVector (CpuMpData); > FillExchangeInfoData (CpuMpData); > SaveLocalApicTimerSetting (CpuMpData); > - } else if (CpuMpData->ApLoopMode == ApInMwaitLoop) { > + } > + > + if (CpuMpData->ApLoopMode == ApInMwaitLoop) { > // > // Get AP target C-state each time when waking up AP, > // for it maybe updated by platform again > @@ -1076,6 +1078,13 @@ WakeUpAP ( > if (ResetVectorRequired) { > FreeResetVector (CpuMpData); > } > + > + // > + // After one round of Wakeup Ap actions, need to re-sync ApLoopMode with > + // WakeUpByInitSipiSipi flag. WakeUpByInitSipiSipi flag maybe changed by > + // S3SmmInitDone Ppi. > + // > + CpuMpData->WakeUpByInitSipiSipi = (CpuMpData->ApLoopMode == ApInHltLoop); > } > > /** > @@ -1648,6 +1657,9 @@ MpInitLibInitialize ( > // > CpuMpData->ApLoopMode = ApLoopMode; > DEBUG ((DEBUG_INFO, "AP Loop Mode is %d\n", CpuMpData->ApLoopMode)); > + > + CpuMpData->WakeUpByInitSipiSipi = (CpuMpData->ApLoopMode == ApInHltLoop); > + > // > // Set up APs wakeup signal buffer > // > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h > index 6958080ac1..9d0b866d09 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h > @@ -250,6 +250,15 @@ struct _CPU_MP_DATA { > UINT32 ProcessorFlags; > UINT64 MicrocodeDataAddress; > UINT32 MicrocodeRevision; > + > + // > + // Whether need to use Init-Sipi-Sipi to wake up the APs. > + // Two cases need to set this value to TRUE. One is in HLT > + // loop mode, the other is resume from S3 which loop mode > + // will be hardcode change to HLT mode by PiSmmCpuDxeSmm > + // driver. > + // > + BOOLEAN WakeUpByInitSipiSipi; > }; > > extern EFI_GUID mCpuInitMpLibHobGuid; > diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf > index fa84e392af..43a3b3b036 100644 > --- a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf > +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf > @@ -44,6 +44,7 @@ > [Packages] > MdePkg/MdePkg.dec > UefiCpuPkg/UefiCpuPkg.dec > + MdeModulePkg/MdeModulePkg.dec > > [LibraryClasses] > BaseLib > @@ -54,6 +55,7 @@ > CpuLib > UefiCpuLib > SynchronizationLib > + PeiServicesLib > > [Pcd] > gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber ## CONSUMES > @@ -64,3 +66,5 @@ > gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode ## CONSUMES > gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate ## SOMETIMES_CONSUMES > > +[Guids] > + gEdkiiS3SmmInitDoneGuid > \ No newline at end of file > diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c > index 92f28681e4..06d966b227 100644 > --- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c > @@ -13,6 +13,87 @@ > **/ > > #include "MpLib.h" > +#include > +#include > + > +/** > + S3 SMM Init Done notification function. > + > + @param PeiServices Indirect reference to the PEI Services Table. > + @param NotifyDesc Address of the notification descriptor data structure. > + @param InvokePpi Address of the PPI that was invoked. > + > + @retval EFI_SUCCESS The function completes successfully. > + > +**/ > +EFI_STATUS > +EFIAPI > +NotifyOnS3SmmInitDonePpi ( > + IN EFI_PEI_SERVICES **PeiServices, > + IN EFI_PEI_NOTIFY_DESCRIPTOR *NotifyDesc, > + IN VOID *InvokePpi > + ); > + > + > +// > +// Global function > +// > +EFI_PEI_NOTIFY_DESCRIPTOR mS3SmmInitDoneNotifyDesc = { > + EFI_PEI_PPI_DESCRIPTOR_NOTIFY_CALLBACK | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST, > + &gEdkiiS3SmmInitDoneGuid, > + NotifyOnS3SmmInitDonePpi > +}; > + > +/** > + The function prototype for invoking a function on an Application Processor. > + > + This definition is used by the UEFI MP Serices Protocol, and the > + PI SMM System Table. > + > + @param[in,out] Buffer The pointer to private data buffer. > +**/ > +VOID > +EmptyApProcedure ( > + IN OUT VOID * Buffer > + ) > +{ > +} > + (1) This function seems useless -- can you please submit a patch to remove it? > +/** > + S3 SMM Init Done notification function. > + > + @param PeiServices Indirect reference to the PEI Services Table. > + @param NotifyDesc Address of the notification descriptor data structure. > + @param InvokePpi Address of the PPI that was invoked. > + > + @retval EFI_SUCCESS The function completes successfully. > + > +**/ > +EFI_STATUS > +EFIAPI > +NotifyOnS3SmmInitDonePpi ( > + IN EFI_PEI_SERVICES **PeiServices, > + IN EFI_PEI_NOTIFY_DESCRIPTOR *NotifyDesc, > + IN VOID *InvokePpi > + ) > +{ > + CPU_MP_DATA *CpuMpData; > + > + CpuMpData = GetCpuMpData (); > + > + // > + // PiSmmCpuDxeSmm driver hardcode change the loop mode to HLT mode. > + // So in this notify function, code need to check the current loop > + // mode, if it is not HLT mode, code need to change loop mode back > + // to the original mode. > + // > + if (CpuMpData->ApLoopMode != ApInHltLoop) { > + CpuMpData->WakeUpByInitSipiSipi = TRUE; > + } > + > + return EFI_SUCCESS; > +} > + > > /** > Enable Debug Agent to support source debugging on AP function. > @@ -240,7 +321,15 @@ InitMpGlobalData ( > IN CPU_MP_DATA *CpuMpData > ) > { > + EFI_STATUS Status; > + > SaveCpuMpData (CpuMpData); > + > + /// > + /// Install Notify > + /// > + Status = PeiServicesNotifyPpi (&mS3SmmInitDoneNotifyDesc); > + ASSERT_EFI_ERROR (Status); > } > > /** > (2) Because OVMF uses the "UefiCpuPkg.dec" default for PcdCpuApLoopMode (value 1, "Place AP in the Hlt-Loop state"), this patch should mostly be a no-op for OVMF; WakeUpByInitSipiSipi is always TRUE (and NotifyOnS3SmmInitDonePpi() basically does nothing). So, I regression-tested this change, and it's fine. I see that the PPI notify is registered, and then called, but there's no other change to S3 resume behavior: > Loading PEIM at 0x0000086FCC0 EntryPoint=0x00000875367 CpuMpPei.efi > AP Loop Mode is 1 > WakeupBufferStart = 9F000, WakeupBufferSize = 0 > TimedWaitForApFinish: reached FinishedApLimit=7 in 0 microseconds > APIC MODE is 1 > MpInitLib: Find 8 processors in system. > Register PPI Notify: [EdkiiS3SmmInitDone] > Does not find any stored CPU BIST information from PPI! > APICID - 0x00000000, BIST - 0x00000000 > APICID - 0x00000001, BIST - 0x00000000 > APICID - 0x00000002, BIST - 0x00000000 > APICID - 0x00000003, BIST - 0x00000000 > APICID - 0x00000004, BIST - 0x00000000 > APICID - 0x00000005, BIST - 0x00000000 > APICID - 0x00000006, BIST - 0x00000000 > APICID - 0x00000007, BIST - 0x00000000 > Install PPI: [EfiSecPlatformInformation2Ppi] > Install PPI: [EfiPeiMpServicesPpi] > [...] > Call AsmDisablePaging64() to return to S3 Resume in PEI Phase > S3ResumeExecuteBootScript() > Close all SMRAM regions before executing boot script > Lock all SMRAM regions before executing boot script > Signal S3SmmInitDone > Install PPI: [EdkiiS3SmmInitDone] > Notify: PPI Guid: [EdkiiS3SmmInitDone], Peim notify entry point: 87674B > Signal [EdkiiS3SmmInitDone] to SMM - Enter > Locate Smm Communicate Ppi failed (Not Found)! > PeiS3ResumeState - 7EF62118 > Enable X64 and transfer control to Standalone Boot Script Executor > [...] Regression-tested-by: Laszlo Ersek Thanks Laszlo