Re: UefiCpuPkg/MpInitLib: Fix S3 resume hang issue.

["Dong, Eric" <eric.dong@intel.com>]

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