subject:"Re\: \[edk2\] UefiCpuPkg\/MpInitLib\: Fix S3 resume hang issue."

Re: [edk2] UefiCpuPkg/MpInitLib: Fix S3 resume hang issue.

2018-07-19 Thread Dong, Eric

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 ; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu 
> 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 
> > 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.
> > +  //
> > +  BOOLEANWakeUpByInitSipiSipi;
> >  };
> >
> >  extern EFI_GUID mCpuInitMpLibHobGuid;
> > diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
> b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
> > index fa84e39

Re: [edk2] UefiCpuPkg/MpInitLib: Fix S3 resume hang issue.

2018-07-19 Thread Laszlo Ersek

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.
> +  //
> +  BOOLEANWakeUpByInitSipiSipi;
>  };
>
>  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

Re: [edk2] UefiCpuPkg/MpInitLib: Fix S3 resume hang issue.

2018-07-18 Thread Ni, Ruiyu

Reviewed-by: Ruiyu Ni 

Thanks/Ray

> -Original Message-
> From: Dong, Eric
> Sent: Wednesday, July 18, 2018 1:10 PM
> To: edk2-devel@lists.01.org
> Cc: Laszlo Ersek ; Ni, Ruiyu 
> 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 
> 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(-)
> 
> 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.
> +  //
> +  BOOLEANWakeUpByInitSipiSipi;
>  };
> 
>  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 
> +
>

Re: [edk2] UefiCpuPkg/MpInitLib: Fix S3 resume hang issue.

Re: [edk2] UefiCpuPkg/MpInitLib: Fix S3 resume hang issue.

Re: [edk2] UefiCpuPkg/MpInitLib: Fix S3 resume hang issue.

3 matches

Site Navigation

Mail list logo

Footer information