Re: [Patch 2/3] UefiCpuPkg/S3Resume2Pei: Send S3 resume finished event to SmmCore.

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

Hi Ray,

I think we can just base on DXE phase bits to prepare the communication buffer. I send new patches base on it. please check them.

Thanks,
Eric

-----Original Message-----
From: Ni, Ruiyu 
Sent: Wednesday, October 11, 2017 11:25 AM
To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
Cc: Yao, Jiewen <jiewen.yao@intel.com>
Subject: RE: [Patch 2/3] UefiCpuPkg/S3Resume2Pei: Send S3 resume finished event to SmmCore.

Eric,
We may have 4 combinations for PEI DXE CPU architecture:
1. 32bit PEI + 32bit DXE: sizeof (UINTN) == sizeof (UINT32) && NOT PcdDxeIplSwitchToLongMode
2. 32bit PEI + 64bit DXE: sizeof (UINTN) == sizeof (UINT32) && PcdDxeIplSwitchToLongMode
3. 64bit PEI + 32bit DXE: NA!!!
4. 64bit PEI + 64bit DXE: sizeof (UINTN) != sizeof (UINT32) && NOT PcdDxeIplSwitchToLongMode

For #4, your code treats MessageLength as 4-byte, but actually it should be 8-byte.
So how about we just check PcdDxeIplSwitchToLongMode, when it's FALSE, sizeof (MessageLength) equals to sizeof (UINTN).
Otherwise, sizeof (MessageLength) equals to 8.

So you only need to define:
> +typedef struct {
> +  EFI_GUID  HeaderGuid;
> +  UINT64    MessageLength;
> +  UINT8     Data[1];
> +} EFI_SMM_COMMUNICATE_HEADER_IA64;

And I recommend to change the structure name to X64_EFI_SMM_COMMUNICATE_HEADER or
IA64_EFI_SMM_COMMUNICATE_HEADER. I am neutral using X64 or IA64, but don't want to have EFI_ in the beginning.

Thanks/Ray

> -----Original Message-----
> From: Dong, Eric
> Sent: Wednesday, October 11, 2017 10:23 AM
> To: edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: [Patch 2/3] UefiCpuPkg/S3Resume2Pei: Send S3 resume finished
> event to SmmCore.
> 
> Driver will send S3 resume finished event to SmmCore through communicate
> buffer after it signals EndOfPei event.
> 
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
>  UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c  | 85
> ++++++++++++++++++++++
>  .../Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf   |  4 +
>  2 files changed, 89 insertions(+)
> 
> diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> index e53ed21..8350eb9 100644
> --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> @@ -28,6 +28,9 @@
>  #include <Ppi/SmmAccess.h>
>  #include <Ppi/PostBootScriptTable.h>
>  #include <Ppi/EndOfPeiPhase.h>
> +#include <Ppi/SmmCommunication.h>
> +
> +#include <Protocol/SmmEndOfS3Resume.h>
> 
>  #include <Library/DebugLib.h>
>  #include <Library/BaseLib.h>
> @@ -151,6 +154,23 @@ typedef union {
>    UINT64    Uint64;
>  } PAGE_TABLE_1G_ENTRY;
> 
> +//
> +// Define two type of smm communicate headers.
> +// One for 32 bits  PEI + 64 bits DXE, the other for 32 bits PEI + 32 bits DXE
> case.
> +//
> +typedef struct {
> +
> +  EFI_GUID  HeaderGuid;
> +  UINT32    MessageLength;
> +  UINT8     Data[1];
> +} EFI_SMM_COMMUNICATE_HEADER_IA32;
> +
> +typedef struct {
> +  EFI_GUID  HeaderGuid;
> +  UINT64    MessageLength;
> +  UINT8     Data[1];
> +} EFI_SMM_COMMUNICATE_HEADER_IA64;
> +
>  #pragma pack()
> 
>  //
> @@ -430,6 +450,65 @@ IsLongModeWakingVector (  }
> 
>  /**
> +  Send EndOfS3Resume event to SmmCore through communication buffer
> way.
> +
> +  @retval  EFI_SUCCESS                 Return send the event success.
> +**/
> +EFI_STATUS
> +SignalEndOfS3Resume (
> +  VOID
> +  )
> +{
> +  EFI_STATUS                         Status;
> +  EFI_PEI_SMM_COMMUNICATION_PPI      *SmmCommunicationPpi;
> +  UINTN                              CommSize;
> +  EFI_SMM_COMMUNICATE_HEADER_IA32    Header32;
> +  EFI_SMM_COMMUNICATE_HEADER_IA64    Header64;
> +  VOID                               *CommBuffer;
> +
> +  DEBUG ((EFI_D_INFO, "SignalEndOfS3Resume - Enter\n"));
> +
> +  //
> +  // Detect whether current is 32 bits PEI + 64 bits DXE case.
> +  //
> +  if ((sizeof(UINTN) == sizeof(UINT32)) && (FeaturePcdGet
> (PcdDxeIplSwitchToLongMode))) {
> +    CommBuffer = &Header64;
> +    Header64.MessageLength = 0;
> +    CommSize = sizeof (EFI_SMM_COMMUNICATE_HEADER_IA64);
> +  } else {
> +    CommBuffer = &Header32;
> +    Header32.MessageLength = 0;
> +    CommSize = sizeof (EFI_SMM_COMMUNICATE_HEADER_IA32);
> +  }
> +  CopyGuid (CommBuffer, &gEdkiiSmmEndOfS3ResumeProtocolGuid);
> +
> +  //
> +  // Get needed resource
> +  //
> +  Status = PeiServicesLocatePpi (
> +             &gEfiPeiSmmCommunicationPpiGuid,
> +             0,
> +             NULL,
> +             (VOID **)&SmmCommunicationPpi
> +             );
> +  ASSERT_EFI_ERROR (Status);
> +
> +  //
> +  // Send command
> +  //
> +  Status = SmmCommunicationPpi->Communicate (
> +                                  SmmCommunicationPpi,
> +                                  (VOID *)CommBuffer,
> +                                  &CommSize
> +                                  );
> +  ASSERT_EFI_ERROR (Status);
> +
> +  DEBUG ((EFI_D_INFO, "SignalEndOfS3Resume - Exit (%r)\n", Status));
> +
> +  return Status;
> +}
> +
> +/**
>    Jump to OS waking vector.
>    The function will install boot script done PPI, report S3 resume status code,
> and then jump to OS waking vector.
> 
> @@ -504,6 +583,12 @@ S3ResumeBootOs (
>    ASSERT_EFI_ERROR (Status);
> 
>    //
> +  // Signal EndOfS3Resume event.
> +  //
> +  Status = SignalEndOfS3Resume ();
> +  ASSERT_EFI_ERROR (Status);
> +
> +  //
>    // report status code on S3 resume
>    //
>    REPORT_STATUS_CODE (EFI_PROGRESS_CODE,
> EFI_SOFTWARE_PEI_MODULE | EFI_SW_PEI_PC_OS_WAKE); diff --git
> a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> index d514523..943f114 100644
> --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> @@ -85,6 +85,10 @@
>    gPeiSmmAccessPpiGuid                          ## SOMETIMES_CONSUMES
>    gPeiPostScriptTablePpiGuid                    ## SOMETIMES_PRODUCES
>    gEfiEndOfPeiSignalPpiGuid                     ## SOMETIMES_PRODUCES
> +  gEfiPeiSmmCommunicationPpiGuid                ## SOMETIMES_CONSUMES
> +
> +[Protocols]
> +  gEdkiiSmmEndOfS3ResumeProtocolGuid            ##
> SOMETIMES_CONSUMES
> 
>  [FeaturePcd]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode         ##
> CONSUMES
> --
> 2.7.0.windows.1