Re: [patch 1/3] UefiCpuPkg/S3Resume: Remove useless pref code

[Laszlo Ersek <lersek@redhat.com>]

On 01/24/18 09:01, Dandan Bi wrote:
> Our new performance infrastructure can support to dump performance
> date form ACPI table in OS. So we can remove the old pref code to
> write performance data to OS.
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Dandan Bi <dandan.bi@intel.com>
> ---
>  UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c  | 131 ---------------------
>  .../Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf   |   3 +-
>  2 files changed, 1 insertion(+), 133 deletions(-)
> 
> diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> index d7d2a4d..4d77689 100644
> --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> @@ -20,11 +20,10 @@
>  
>  #include <PiPei.h>
>  
>  #include <Guid/AcpiS3Context.h>
>  #include <Guid/BootScriptExecutorVariable.h>
> -#include <Guid/Performance.h>
>  #include <Guid/ExtendedFirmwarePerformance.h>
>  #include <Guid/EndOfS3Resume.h>
>  #include <Ppi/ReadOnlyVariable2.h>
>  #include <Ppi/S3Resume2.h>
>  #include <Ppi/SmmAccess.h>
> @@ -284,136 +283,10 @@ GLOBAL_REMOVE_IF_UNREFERENCED IA32_GDT mGdtEntries[] = {
>  GLOBAL_REMOVE_IF_UNREFERENCED CONST IA32_DESCRIPTOR mGdt = {
>    sizeof (mGdtEntries) - 1,
>    (UINTN) mGdtEntries
>    };
>  
> -/**
> -  Performance measure function to get S3 detailed performance data.
> -
> -  This function will getS3 detailed performance data and saved in pre-reserved ACPI memory.
> -**/
> -VOID
> -WriteToOsS3PerformanceData (
> -  VOID
> -  )
> -{
> -  EFI_STATUS                                    Status;
> -  EFI_PHYSICAL_ADDRESS                          mAcpiLowMemoryBase;
> -  PERF_HEADER                                   *PerfHeader;
> -  PERF_DATA                                     *PerfData;
> -  UINT64                                        Ticker;
> -  UINTN                                         Index;
> -  EFI_PEI_READ_ONLY_VARIABLE2_PPI               *VariableServices;
> -  UINTN                                         VarSize;
> -  UINTN                                         LogEntryKey;
> -  CONST VOID                                    *Handle;
> -  CONST CHAR8                                   *Token;
> -  CONST CHAR8                                   *Module;
> -  UINT64                                        StartTicker;
> -  UINT64                                        EndTicker;
> -  UINT64                                        StartValue;
> -  UINT64                                        EndValue;
> -  BOOLEAN                                       CountUp;
> -  UINT64                                        Freq;
> -
> -  //
> -  // Retrieve time stamp count as early as possible
> -  //
> -  Ticker = GetPerformanceCounter ();
> -
> -  Freq   = GetPerformanceCounterProperties (&StartValue, &EndValue);
> -
> -  Freq   = DivU64x32 (Freq, 1000);
> -
> -  Status = PeiServicesLocatePpi (
> -             &gEfiPeiReadOnlyVariable2PpiGuid,
> -             0,
> -             NULL,
> -             (VOID **) &VariableServices
> -             );
> -  if (EFI_ERROR (Status)) {
> -    return;
> -  }
> -
> -  VarSize   = sizeof (EFI_PHYSICAL_ADDRESS);
> -  Status = VariableServices->GetVariable (
> -                               VariableServices,
> -                               L"PerfDataMemAddr",
> -                               &gPerformanceProtocolGuid,
> -                               NULL,
> -                               &VarSize,
> -                               &mAcpiLowMemoryBase
> -                               );
> -  if (EFI_ERROR (Status)) {
> -    DEBUG ((EFI_D_ERROR, "Fail to retrieve variable to log S3 performance data \n"));
> -    return;
> -  }
> -
> -  PerfHeader = (PERF_HEADER *) (UINTN) mAcpiLowMemoryBase;
> -
> -  if (PerfHeader->Signiture != PERFORMANCE_SIGNATURE) {
> -    DEBUG ((EFI_D_ERROR, "Performance data in ACPI memory get corrupted! \n"));
> -    return;
> -  }
> -
> -  //
> -  // Record total S3 resume time.
> -  //
> -  if (EndValue >= StartValue) {
> -    PerfHeader->S3Resume = Ticker - StartValue;
> -    CountUp              = TRUE;
> -  } else {
> -    PerfHeader->S3Resume = StartValue - Ticker;
> -    CountUp              = FALSE;
> -  }
> -
> -  //
> -  // Get S3 detailed performance data
> -  //
> -  Index = 0;
> -  LogEntryKey = 0;
> -  while ((LogEntryKey = GetPerformanceMeasurement (
> -                          LogEntryKey,
> -                          &Handle,
> -                          &Token,
> -                          &Module,
> -                          &StartTicker,
> -                          &EndTicker)) != 0) {
> -    if (EndTicker != 0) {
> -      PerfData = &PerfHeader->S3Entry[Index];
> -
> -      //
> -      // Use File Handle to specify the different performance log for PEIM.
> -      // File Handle is the base address of PEIM FFS file.
> -      //
> -      if ((AsciiStrnCmp (Token, "PEIM", PEI_PERFORMANCE_STRING_SIZE) == 0) && (Handle != NULL)) {
> -        AsciiSPrint (PerfData->Token, PERF_TOKEN_LENGTH, "0x%11p", Handle);
> -      } else {
> -        AsciiStrnCpyS (PerfData->Token, PERF_TOKEN_SIZE, Token, PERF_TOKEN_LENGTH);
> -      }
> -      if (StartTicker == 1) {
> -        StartTicker = StartValue;
> -      }
> -      if (EndTicker == 1) {
> -        EndTicker = StartValue;
> -      }
> -      Ticker = CountUp? (EndTicker - StartTicker) : (StartTicker - EndTicker);
> -      PerfData->Duration = (UINT32) DivU64x32 (Ticker, (UINT32) Freq);
> -
> -      //
> -      // Only Record > 1ms performance data so that more big performance can be recorded.
> -      //
> -      if ((Ticker > Freq) && (++Index >= PERF_PEI_ENTRY_MAX_NUM)) {
> -        //
> -        // Reach the maximum number of PEI performance log entries.
> -        //
> -        break;
> -      }
> -    }
> -  }
> -  PerfHeader->S3EntryNum = (UINT32) Index;
> -}
>  
>  /**
>    The function will check if current waking vector is long mode.
>  
>    @param  AcpiS3Context                 a pointer to a structure of ACPI_S3_CONTEXT
> @@ -602,14 +475,10 @@ S3ResumeBootOs (
>    //
>    // report status code on S3 resume
>    //
>    REPORT_STATUS_CODE (EFI_PROGRESS_CODE, EFI_SOFTWARE_PEI_MODULE | EFI_SW_PEI_PC_OS_WAKE);
>  
> -  PERF_CODE (
> -    WriteToOsS3PerformanceData ();
> -    );
> -
>    AsmTransferControl = (ASM_TRANSFER_CONTROL)(UINTN)PeiS3ResumeState->AsmTransferControl;
>    if (Facs->XFirmwareWakingVector != 0) {
>      //
>      // Switch to native waking vector
>      //
> diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> index 15fa2d1..9522ede 100644
> --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> @@ -3,11 +3,11 @@
>  #
>  # This module works with StandAloneBootScriptExecutor to S3 resume to OS.
>  # This module will excute the boot script saved during last boot and after that,
>  # control is passed to OS waking up handler.
>  #
> -# Copyright (c) 2010 - 2017, Intel Corporation. All rights reserved.<BR>
> +# Copyright (c) 2010 - 2018, Intel Corporation. All rights reserved.<BR>
>  # Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
>  #
>  # This program and the accompanying materials are
>  # licensed and made available under the terms and conditions of the BSD License
>  # which accompanies this distribution.  The full text of the license may be found at
> @@ -71,11 +71,10 @@
>    PrintLib
>  
>  [Guids]
>    gEfiBootScriptExecutorVariableGuid            ## SOMETIMES_CONSUMES ## UNDEFINED # LockBox
>    gEfiBootScriptExecutorContextGuid             ## SOMETIMES_CONSUMES ## UNDEFINED # LockBox
> -  gPerformanceProtocolGuid                      ## SOMETIMES_CONSUMES ## Variable:L"PerfDataMemAddr"
>    ## SOMETIMES_CONSUMES ## HOB
>    ## SOMETIMES_CONSUMES ## UNDEFINED # LockBox
>    gEfiAcpiVariableGuid
>    gEfiAcpiS3ContextGuid                         ## SOMETIMES_CONSUMES ## UNDEFINED # LockBox
>    gEdkiiEndOfS3ResumeGuid                       ## SOMETIMES_CONSUMES ## UNDEFINED # Used to do smm communication
> 

Looks good to me.

Specifically for use in OVMF, this logic has always been inactive, since
we don't yet have PEI-phase (r/o) variable access.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo