Hi Laszlo, > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of > Laszlo Ersek > Sent: Thursday, July 19, 2018 7:57 PM > To: Dong, Eric <eric.d...@intel.com>; edk2-devel@lists.01.org > Cc: Ni, Ruiyu <ruiyu...@intel.com> > Subject: Re: [edk2] UefiCpuPkg/MpInitLib: Fix S3 resume hang issue. > > 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 <ler...@redhat.com> > > Cc: Ruiyu Ni <ruiyu...@intel.com> > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Eric Dong <eric.d...@intel.com> > > --- > > 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 <Library/PeiServicesLib.h> > > +#include <Guid/S3SmmInitDone.h> > > + > > +/** > > + 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? >
Yes. > > +/** > > + 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 <ler...@redhat.com> > > Thanks > Laszlo > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel