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

[Laszlo Ersek <lersek@redhat.com>]

small update to my review comments:

On 01/24/18 16:48, Laszlo Ersek wrote:
> 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.

Again, please reference the TianoCore BZ and/or commit hashes and/or
mailing list subjects/URLs, for specifying "new performance infrastructure".

With that change, my R-b stands.

Thanks
Laszlo

>>
>> 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>