* UefiCpuPkg/MpInitLib: Fix S3 resume hang issue. @ 2018-07-18 5:10 Eric Dong 2018-07-19 4:40 ` Ni, Ruiyu 2018-07-19 11:56 ` Laszlo Ersek 0 siblings, 2 replies; 4+ messages in thread From: Eric Dong @ 2018-07-18 5:10 UTC (permalink / raw) To: edk2-devel; +Cc: Laszlo Ersek, Ruiyu Ni 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 <lersek@redhat.com> Cc: Ruiyu Ni <ruiyu.ni@intel.com> Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Eric Dong <eric.dong@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(-) 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 + ) +{ +} + +/** + 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.15.0.windows.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: UefiCpuPkg/MpInitLib: Fix S3 resume hang issue. 2018-07-18 5:10 UefiCpuPkg/MpInitLib: Fix S3 resume hang issue Eric Dong @ 2018-07-19 4:40 ` Ni, Ruiyu 2018-07-19 11:56 ` Laszlo Ersek 1 sibling, 0 replies; 4+ messages in thread From: Ni, Ruiyu @ 2018-07-19 4:40 UTC (permalink / raw) To: Dong, Eric, edk2-devel@lists.01.org; +Cc: Laszlo Ersek Reviewed-by: Ruiyu Ni <Ruiyu.nI@intel.com> Thanks/Ray > -----Original Message----- > From: Dong, Eric > Sent: Wednesday, July 18, 2018 1:10 PM > To: edk2-devel@lists.01.org > Cc: Laszlo Ersek <lersek@redhat.com>; Ni, Ruiyu <ruiyu.ni@intel.com> > Subject: UefiCpuPkg/MpInitLib: Fix S3 resume hang issue. > > 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 <lersek@redhat.com> > Cc: Ruiyu Ni <ruiyu.ni@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Eric Dong <eric.dong@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(-) > > 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 > + ) > +{ > +} > + > +/** > + 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.15.0.windows.1 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: UefiCpuPkg/MpInitLib: Fix S3 resume hang issue. 2018-07-18 5:10 UefiCpuPkg/MpInitLib: Fix S3 resume hang issue Eric Dong 2018-07-19 4:40 ` Ni, Ruiyu @ 2018-07-19 11:56 ` Laszlo Ersek 2018-07-19 12:09 ` Dong, Eric 1 sibling, 1 reply; 4+ messages in thread From: Laszlo Ersek @ 2018-07-19 11:56 UTC (permalink / raw) To: Eric Dong, edk2-devel; +Cc: Ruiyu Ni 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 <lersek@redhat.com> > Cc: Ruiyu Ni <ruiyu.ni@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Eric Dong <eric.dong@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? > +/** > + 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 <lersek@redhat.com> Thanks Laszlo ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: UefiCpuPkg/MpInitLib: Fix S3 resume hang issue. 2018-07-19 11:56 ` Laszlo Ersek @ 2018-07-19 12:09 ` Dong, Eric 0 siblings, 0 replies; 4+ messages in thread From: Dong, Eric @ 2018-07-19 12:09 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu Hi Laszlo, > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > Laszlo Ersek > Sent: Thursday, July 19, 2018 7:57 PM > To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org > Cc: Ni, Ruiyu <ruiyu.ni@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 <lersek@redhat.com> > > Cc: Ruiyu Ni <ruiyu.ni@intel.com> > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Eric Dong <eric.dong@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 <lersek@redhat.com> > > Thanks > Laszlo > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-07-19 12:09 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-07-18 5:10 UefiCpuPkg/MpInitLib: Fix S3 resume hang issue Eric Dong 2018-07-19 4:40 ` Ni, Ruiyu 2018-07-19 11:56 ` Laszlo Ersek 2018-07-19 12:09 ` Dong, Eric
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox