Re: [edk2-devel] [PATCH v1 1/1] UefiCpuPkg: Remove PEI/DXE instances of CpuTimerLib.

["Zeng, Star" <star.zeng@intel.com>]

A little surprised by "Average time taken to find CpuCrystalFrequencyHob: about 2000 ns".
It depends on the hob is built early or late?
Seemingly, NanoSecondDelay() will always have some deviation.

Thanks,
Star
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
Sent: Friday, September 25, 2020 2:46 PM
To: Ni, Ray <ray.ni@intel.com>; Lou, Yun <yun.lou@intel.com>; devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
Subject: Re: [edk2-devel] [PATCH v1 1/1] UefiCpuPkg: Remove PEI/DXE instances of CpuTimerLib.

On 09/25/20 07:25, Ni, Ray wrote:
> Reviewed-by: Ray Ni <ray.ni@intel.com>

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

Thanks
Laszlo

> 
>> -----Original Message-----
>> From: Lou, Yun <yun.lou@intel.com>
>> Sent: Friday, September 25, 2020 11:58 AM
>> To: devel@edk2.groups.io
>> Cc: Lou, Yun <yun.lou@intel.com>; Ni, Ray <ray.ni@intel.com>; Dong, 
>> Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Kumar, 
>> Rahul1 <rahul1.kumar@intel.com>
>> Subject: [PATCH v1 1/1] UefiCpuPkg: Remove PEI/DXE instances of CpuTimerLib.
>>
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2832
>>
>> 1. Remove PEI instance(PeiCpuTimerLib).
>> PeiCpuTimerLib is currently designed to save time by getting CPU TSC frequncy from Hob.
>> BaseCpuTimerLib is designed to calculate TSC frequency by using CPUID[15h] each time.
>> The time it takes to find CpuCrystalFrequencyHob (about 2000ns) is 
>> much longer than it takes to calculate TSC frequency with CPUID[15h] 
>> (about 450ns), which means using BaseCpuTimerLib to trigger a delay 
>> is more accurate than using PeiCpuTimerLib, recommend to use BaseCpuTimerLib instead of PeiCpuTimerLib.
>>
>> 2. Remove DXE instance(DxeCpuTimerLib).
>> DxeCpuTimerLib is designed to calculate TSC frequency with CPUID[15h] 
>> in its constructor function, then save it in a global variable. For 
>> this design, once the driver containing this instance is running, the constructor function is called, it will take extra time to calculate TSC frequency.
>> The time it takes to get TSC frequncy from global variable is shorter 
>> than it takes to calculate TSC frequency with CPUID[15h], but 450ns is a short time, the impact on the platform is very limited.
>> In addition, in order to simplify the code, recommend to use BaseCpuTimerLib instead of DxeCpuTimerLib.
>>
>> I did some experiments on one Intel server platform and collected the following data:
>> 1. Average time taken to find CpuCrystalFrequencyHob: about 2000 ns.
>> 2. Average time taken to calculate TSC frequency: about 450 ns.
>>
>> Reference code:
>>     //
>>     // Calculate average time taken to find Hob.
>>     //
>>     DEBUG((DEBUG_ERROR, "[PeiCpuTimerLib] GetPerformanceCounterFrequency - GetFirstGuidHob (1000 cycles)\n"));
>>     Ticks1 = AsmReadTsc();
>>     for (i = 0; i < 1000; i++) {
>>       GuidHob = GetFirstGuidHob (&mCpuCrystalFrequencyHobGuid);
>>     }
>>     Ticks2 = AsmReadTsc();
>>
>>     if (GuidHob == NULL) {
>>       DEBUG((DEBUG_ERROR, "[PeiCpuTimerLib]  - CpuCrystalFrequencyHob can not be found!\n"));
>>     } else {
>>       DEBUG((DEBUG_ERROR, "[PeiCpuTimerLib]  - Average time taken to find Hob = %d ns\n", \
>>           DivU64x32(DivU64x64Remainder(MultU64x32((Ticks2 - Ticks1), 
>> 1000000000), *CpuCrystalCounterFrequency, NULL), 1000)));
>>     }
>>
>>     //
>>     // Calculate average time taken to calculate CPU frequency.
>>     //
>>     DEBUG((DEBUG_ERROR, "[PeiCpuTimerLib] 
>> GetPerformanceCounterFrequency - CpuidCoreClockCalculateTscFrequency
>> (1000 cycles)\n"));
>>     Ticks1 = AsmReadTsc();
>>     for (i = 0; i < 1000; i++) {
>>       Freq = CpuidCoreClockCalculateTscFrequency ();
>>     }
>>     Ticks2 = AsmReadTsc();
>>     DEBUG((DEBUG_ERROR, "[PeiCpuTimerLib]  - Average time taken to calculate TSC frequency = %d ns\n", \
>>         DivU64x32(DivU64x64Remainder(MultU64x32((Ticks2 - Ticks1), 
>> 1000000000), *CpuCrystalCounterFrequency, NULL), 1000)));
>>
>> Signed-off-by: Jason Lou <yun.lou@intel.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Rahul Kumar <rahul1.kumar@intel.com>
>> ---
>>  UefiCpuPkg/Library/CpuTimerLib/DxeCpuTimerLib.c   | 85 --------------------
>>  UefiCpuPkg/Library/CpuTimerLib/PeiCpuTimerLib.c   | 58 -------------
>>  UefiCpuPkg/Library/CpuTimerLib/DxeCpuTimerLib.inf | 37 ---------  
>> UefiCpuPkg/Library/CpuTimerLib/DxeCpuTimerLib.uni | 17 ----  
>> UefiCpuPkg/Library/CpuTimerLib/PeiCpuTimerLib.inf | 36 ---------  
>> UefiCpuPkg/Library/CpuTimerLib/PeiCpuTimerLib.uni | 17 ----
>>  UefiCpuPkg/UefiCpuPkg.dsc                         |  4 +-
>>  7 files changed, 1 insertion(+), 253 deletions(-)
>>
>> diff --git a/UefiCpuPkg/Library/CpuTimerLib/DxeCpuTimerLib.c 
>> b/UefiCpuPkg/Library/CpuTimerLib/DxeCpuTimerLib.c
>> deleted file mode 100644
>> index 269e5a3e83d7..000000000000
>> --- a/UefiCpuPkg/Library/CpuTimerLib/DxeCpuTimerLib.c
>> +++ /dev/null
>> @@ -1,85 +0,0 @@
>> -/** @file
>>
>> -  CPUID Leaf 0x15 for Core Crystal Clock frequency instance of Timer Library.
>>
>> -
>>
>> -  Copyright (c) 2019 Intel Corporation. All rights reserved.<BR>
>>
>> -  SPDX-License-Identifier: BSD-2-Clause-Patent
>>
>> -
>>
>> -**/
>>
>> -
>>
>> -#include <PiDxe.h>
>>
>> -#include <Library/TimerLib.h>
>>
>> -#include <Library/BaseLib.h>
>>
>> -#include <Library/HobLib.h>
>>
>> -
>>
>> -extern GUID mCpuCrystalFrequencyHobGuid;
>>
>> -
>>
>> -/**
>>
>> -  CPUID Leaf 0x15 for Core Crystal Clock Frequency.
>>
>> -
>>
>> -  The TSC counting frequency is determined by using CPUID leaf 0x15. Frequency in MHz = Core XTAL frequency * EBX/EAX.
>>
>> -  In newer flavors of the CPU, core xtal frequency is returned in ECX or 0 if not supported.
>>
>> -  @return The number of TSC counts per second.
>>
>> -
>>
>> -**/
>>
>> -UINT64
>>
>> -CpuidCoreClockCalculateTscFrequency (
>>
>> -  VOID
>>
>> -  );
>>
>> -
>>
>> -//
>>
>> -// Cached CPU Crystal counter frequency
>>
>> -//
>>
>> -UINT64  mCpuCrystalCounterFrequency = 0;
>>
>> -
>>
>> -
>>
>> -/**
>>
>> -  Internal function to retrieves the 64-bit frequency in Hz.
>>
>> -
>>
>> -  Internal function to retrieves the 64-bit frequency in Hz.
>>
>> -
>>
>> -  @return The frequency in Hz.
>>
>> -
>>
>> -**/
>>
>> -UINT64
>>
>> -InternalGetPerformanceCounterFrequency (
>>
>> -  VOID
>>
>> -  )
>>
>> -{
>>
>> -  return mCpuCrystalCounterFrequency;
>>
>> -}
>>
>> -
>>
>> -/**
>>
>> -  The constructor function is to initialize CpuCrystalCounterFrequency.
>>
>> -
>>
>> -  @param  ImageHandle   The firmware allocated handle for the EFI image.
>>
>> -  @param  SystemTable   A pointer to the EFI System Table.
>>
>> -
>>
>> -  @retval EFI_SUCCESS   The constructor always returns RETURN_SUCCESS.
>>
>> -
>>
>> -**/
>>
>> -EFI_STATUS
>>
>> -EFIAPI
>>
>> -DxeCpuTimerLibConstructor (
>>
>> -  IN EFI_HANDLE        ImageHandle,
>>
>> -  IN EFI_SYSTEM_TABLE  *SystemTable
>>
>> -  )
>>
>> -{
>>
>> -  EFI_HOB_GUID_TYPE   *GuidHob;
>>
>> -
>>
>> -  //
>>
>> -  // Initialize CpuCrystalCounterFrequency
>>
>> -  //
>>
>> -  GuidHob = GetFirstGuidHob (&mCpuCrystalFrequencyHobGuid);
>>
>> -  if (GuidHob != NULL) {
>>
>> -    mCpuCrystalCounterFrequency = *(UINT64*)GET_GUID_HOB_DATA (GuidHob);
>>
>> -  } else {
>>
>> -    mCpuCrystalCounterFrequency = CpuidCoreClockCalculateTscFrequency ();
>>
>> -  }
>>
>> -
>>
>> -  if (mCpuCrystalCounterFrequency == 0) {
>>
>> -    return EFI_UNSUPPORTED;
>>
>> -  }
>>
>> -
>>
>> -  return EFI_SUCCESS;
>>
>> -}
>>
>> -
>>
>> diff --git a/UefiCpuPkg/Library/CpuTimerLib/PeiCpuTimerLib.c 
>> b/UefiCpuPkg/Library/CpuTimerLib/PeiCpuTimerLib.c
>> deleted file mode 100644
>> index 91a721205653..000000000000
>> --- a/UefiCpuPkg/Library/CpuTimerLib/PeiCpuTimerLib.c
>> +++ /dev/null
>> @@ -1,58 +0,0 @@
>> -/** @file
>>
>> -  CPUID Leaf 0x15 for Core Crystal Clock frequency instance as PEI Timer Library.
>>
>> -
>>
>> -  Copyright (c) 2019 Intel Corporation. All rights reserved.<BR>
>>
>> -  SPDX-License-Identifier: BSD-2-Clause-Patent
>>
>> -
>>
>> -**/
>>
>> -
>>
>> -#include <PiPei.h>
>>
>> -#include <Library/TimerLib.h>
>>
>> -#include <Library/BaseLib.h>
>>
>> -#include <Library/HobLib.h>
>>
>> -#include <Library/DebugLib.h>
>>
>> -
>>
>> -extern GUID mCpuCrystalFrequencyHobGuid;
>>
>> -
>>
>> -/**
>>
>> -  CPUID Leaf 0x15 for Core Crystal Clock Frequency.
>>
>> -
>>
>> -  The TSC counting frequency is determined by using CPUID leaf 0x15. Frequency in MHz = Core XTAL frequency * EBX/EAX.
>>
>> -  In newer flavors of the CPU, core xtal frequency is returned in ECX or 0 if not supported.
>>
>> -  @return The number of TSC counts per second.
>>
>> -
>>
>> -**/
>>
>> -UINT64
>>
>> -CpuidCoreClockCalculateTscFrequency (
>>
>> -  VOID
>>
>> -  );
>>
>> -
>>
>> -/**
>>
>> -  Internal function to retrieves the 64-bit frequency in Hz.
>>
>> -
>>
>> -  Internal function to retrieves the 64-bit frequency in Hz.
>>
>> -
>>
>> -  @return The frequency in Hz.
>>
>> -
>>
>> -**/
>>
>> -UINT64
>>
>> -InternalGetPerformanceCounterFrequency (
>>
>> -  VOID
>>
>> -  )
>>
>> -{
>>
>> -  UINT64              *CpuCrystalCounterFrequency;
>>
>> -  EFI_HOB_GUID_TYPE   *GuidHob;
>>
>> -
>>
>> -  CpuCrystalCounterFrequency = NULL;
>>
>> -  GuidHob = GetFirstGuidHob (&mCpuCrystalFrequencyHobGuid);
>>
>> -  if (GuidHob == NULL) {
>>
>> -    CpuCrystalCounterFrequency  = (UINT64*)BuildGuidHob(&mCpuCrystalFrequencyHobGuid, sizeof
>> (*CpuCrystalCounterFrequency));
>>
>> -    ASSERT (CpuCrystalCounterFrequency != NULL);
>>
>> -    *CpuCrystalCounterFrequency = CpuidCoreClockCalculateTscFrequency ();
>>
>> -  } else {
>>
>> -    CpuCrystalCounterFrequency = (UINT64*)GET_GUID_HOB_DATA (GuidHob);
>>
>> -  }
>>
>> -
>>
>> -  return  *CpuCrystalCounterFrequency;
>>
>> -}
>>
>> -
>>
>> diff --git a/UefiCpuPkg/Library/CpuTimerLib/DxeCpuTimerLib.inf 
>> b/UefiCpuPkg/Library/CpuTimerLib/DxeCpuTimerLib.inf
>> deleted file mode 100644
>> index 6c83549c87da..000000000000
>> --- a/UefiCpuPkg/Library/CpuTimerLib/DxeCpuTimerLib.inf
>> +++ /dev/null
>> @@ -1,37 +0,0 @@
>> -## @file
>>
>> -#  DXE CPU Timer Library
>>
>> -#
>>
>> -#  Provides basic timer support using CPUID Leaf 0x15 XTAL 
>> frequency. The performance
>>
>> -#  counter features are provided by the processors time stamp counter.
>>
>> -#
>>
>> -#  Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
>>
>> -#  SPDX-License-Identifier: BSD-2-Clause-Patent
>>
>> -#
>>
>> -##
>>
>> -
>>
>> -[Defines]
>>
>> -  INF_VERSION                    = 0x00010005
>>
>> -  BASE_NAME                      = DxeCpuTimerLib
>>
>> -  FILE_GUID                      = F22CC0DA-E7DB-4E4D-ABE2-A608188233A2
>>
>> -  MODULE_TYPE                    = DXE_DRIVER
>>
>> -  VERSION_STRING                 = 1.0
>>
>> -  LIBRARY_CLASS                  = TimerLib|DXE_CORE DXE_DRIVER DXE_RUNTIME_DRIVER DXE_SMM_DRIVER
>> UEFI_APPLICATION UEFI_DRIVER SMM_CORE
>>
>> -  CONSTRUCTOR                    = DxeCpuTimerLibConstructor
>>
>> -  MODULE_UNI_FILE                = DxeCpuTimerLib.uni
>>
>> -
>>
>> -[Sources]
>>
>> -  CpuTimerLib.c
>>
>> -  DxeCpuTimerLib.c
>>
>> -
>>
>> -[Packages]
>>
>> -  MdePkg/MdePkg.dec
>>
>> -  UefiCpuPkg/UefiCpuPkg.dec
>>
>> -
>>
>> -[LibraryClasses]
>>
>> -  BaseLib
>>
>> -  PcdLib
>>
>> -  DebugLib
>>
>> -  HobLib
>>
>> -
>>
>> -[Pcd]
>>
>> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuCoreCrystalClockFrequency  ## 
>> CONSUMES
>>
>> diff --git a/UefiCpuPkg/Library/CpuTimerLib/DxeCpuTimerLib.uni 
>> b/UefiCpuPkg/Library/CpuTimerLib/DxeCpuTimerLib.uni
>> deleted file mode 100644
>> index f55b92abace7..000000000000
>> --- a/UefiCpuPkg/Library/CpuTimerLib/DxeCpuTimerLib.uni
>> +++ /dev/null
>> @@ -1,17 +0,0 @@
>> -// /** @file
>>
>> -// DXE CPU Timer Library
>>
>> -//
>>
>> -// Provides basic timer support using CPUID Leaf 0x15 XTAL 
>> frequency.  The performance
>>
>> -// counter features are provided by the processors time stamp counter.
>>
>> -//
>>
>> -// Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
>>
>> -//
>>
>> -// SPDX-License-Identifier: BSD-2-Clause-Patent
>>
>> -//
>>
>> -// **/
>>
>> -
>>
>> -
>>
>> -#string STR_MODULE_ABSTRACT             #language en-US "CPU Timer Library"
>>
>> -
>>
>> -#string STR_MODULE_DESCRIPTION          #language en-US "Provides basic timer support using CPUID Leaf 0x15 XTAL
>> frequency."
>>
>> -
>>
>> diff --git a/UefiCpuPkg/Library/CpuTimerLib/PeiCpuTimerLib.inf 
>> b/UefiCpuPkg/Library/CpuTimerLib/PeiCpuTimerLib.inf
>> deleted file mode 100644
>> index 7af0fc44a65d..000000000000
>> --- a/UefiCpuPkg/Library/CpuTimerLib/PeiCpuTimerLib.inf
>> +++ /dev/null
>> @@ -1,36 +0,0 @@
>> -## @file
>>
>> -#  PEI CPU Timer Library
>>
>> -#
>>
>> -#  Provides basic timer support using CPUID Leaf 0x15 XTAL 
>> frequency. The performance
>>
>> -#  counter features are provided by the processors time stamp counter.
>>
>> -#
>>
>> -#  Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
>>
>> -#  SPDX-License-Identifier: BSD-2-Clause-Patent
>>
>> -#
>>
>> -##
>>
>> -
>>
>> -[Defines]
>>
>> -  INF_VERSION                    = 0x00010005
>>
>> -  BASE_NAME                      = PeiCpuTimerLib
>>
>> -  FILE_GUID                      = 2B13DE00-1A5F-4DD7-A298-01B08AF1015A
>>
>> -  MODULE_TYPE                    = BASE
>>
>> -  VERSION_STRING                 = 1.0
>>
>> -  LIBRARY_CLASS                  = TimerLib|PEI_CORE PEIM
>>
>> -  MODULE_UNI_FILE                = PeiCpuTimerLib.uni
>>
>> -
>>
>> -[Sources]
>>
>> -  CpuTimerLib.c
>>
>> -  PeiCpuTimerLib.c
>>
>> -
>>
>> -[Packages]
>>
>> -  MdePkg/MdePkg.dec
>>
>> -  UefiCpuPkg/UefiCpuPkg.dec
>>
>> -
>>
>> -[LibraryClasses]
>>
>> -  BaseLib
>>
>> -  PcdLib
>>
>> -  DebugLib
>>
>> -  HobLib
>>
>> -
>>
>> -[Pcd]
>>
>> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuCoreCrystalClockFrequency  ## 
>> CONSUMES
>>
>> diff --git a/UefiCpuPkg/Library/CpuTimerLib/PeiCpuTimerLib.uni 
>> b/UefiCpuPkg/Library/CpuTimerLib/PeiCpuTimerLib.uni
>> deleted file mode 100644
>> index 49beb44908d6..000000000000
>> --- a/UefiCpuPkg/Library/CpuTimerLib/PeiCpuTimerLib.uni
>> +++ /dev/null
>> @@ -1,17 +0,0 @@
>> -// /** @file
>>
>> -// PEI CPU Timer Library
>>
>> -//
>>
>> -// Provides basic timer support using CPUID Leaf 0x15 XTAL 
>> frequency.  The performance
>>
>> -// counter features are provided by the processors time stamp counter.
>>
>> -//
>>
>> -// Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
>>
>> -//
>>
>> -// SPDX-License-Identifier: BSD-2-Clause-Patent
>>
>> -//
>>
>> -// **/
>>
>> -
>>
>> -
>>
>> -#string STR_MODULE_ABSTRACT             #language en-US "CPU Timer Library"
>>
>> -
>>
>> -#string STR_MODULE_DESCRIPTION          #language en-US "Provides basic timer support using CPUID Leaf 0x15 XTAL
>> frequency."
>>
>> -
>>
>> diff --git a/UefiCpuPkg/UefiCpuPkg.dsc b/UefiCpuPkg/UefiCpuPkg.dsc 
>> index b2b6d78a71b0..e915b5c81b66 100644
>> --- a/UefiCpuPkg/UefiCpuPkg.dsc
>> +++ b/UefiCpuPkg/UefiCpuPkg.dsc
>> @@ -1,7 +1,7 @@
>>  ## @file
>>
>>  #  UefiCpuPkg Package
>>
>>  #
>>
>> -#  Copyright (c) 2007 - 2019, Intel Corporation. All rights 
>> reserved.<BR>
>>
>> +#  Copyright (c) 2007 - 2020, Intel Corporation. All rights 
>> +reserved.<BR>
>>
>>  #
>>
>>  #  SPDX-License-Identifier: BSD-2-Clause-Patent
>>
>>  #
>>
>> @@ -107,8 +107,6 @@ [Components]
>>    
>> UefiCpuPkg/Library/SecPeiDxeTimerLibUefiCpu/SecPeiDxeTimerLibUefiCpu.
>> inf
>>
>>    UefiCpuPkg/Application/Cpuid/Cpuid.inf
>>
>>    UefiCpuPkg/Library/CpuTimerLib/BaseCpuTimerLib.inf
>>
>> -  UefiCpuPkg/Library/CpuTimerLib/DxeCpuTimerLib.inf
>>
>> -  UefiCpuPkg/Library/CpuTimerLib/PeiCpuTimerLib.inf
>>
>>
>>
>>  [Components.IA32, Components.X64]
>>
>>    UefiCpuPkg/CpuDxe/CpuDxe.inf
>>
>> --
>> 2.28.0.windows.1
>