Re: [PATCH v2] UefiCpuPkg: Remove PEI/DXE instances of CpuTimerLib.

["Ni, Ray" <ray.ni@intel.com>]

Thanks for providing the detailed commit messages.
Reviewed-by: Ray Ni <ray.ni@intel.com>


> -----Original Message-----
> From: Lou, Yun <yun.lou@intel.com>
> Sent: Wednesday, April 7, 2021 4:16 PM
> 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 v2] UefiCpuPkg: Remove PEI/DXE instances of CpuTimerLib.
> 
> From: Jason Lou <yun.lou@intel.com>
> 
> 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
> frequency 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, this
> constructor function is called, it will take extra time to calculate TSC
> frequency.
> The time it takes to get TSC frequency 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 server platform and collected following data:
> 1. Average time required to find CpuCrystalFrequencyHob: about 2000 ns.
> 2. Average time required to find the last Hob: about 2700 ns.
> 2. Average time required to calculate TSC frequency: about 450 ns.
> 
> Reference code:
>     //
>     // Calculate average time required 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 required to find Hob = %d ns\n", \
>           DivU64x32(DivU64x64Remainder(MultU64x32((Ticks2 - Ticks1), 1000000000), *CpuCrystalCounterFrequency, NULL),
> 1000)));
>     }
> 
>     //
>     // Calculate average time required 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 required 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                         |  2 -
>  7 files changed, 252 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/CpuTimerLib/DxeCpuTimerLib.c b/UefiCpuPkg/Library/CpuTimerLib/DxeCpuTimerLib.c
> deleted file mode 100644
> index 269e5a3e83..0000000000
> --- 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 91a7212056..0000000000
> --- 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 6c83549c87..0000000000
> --- 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 f55b92abac..0000000000
> --- 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 7af0fc44a6..0000000000
> --- 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 49beb44908..0000000000
> --- 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 98c4c53465..c16cf8d1b9 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dsc
> +++ b/UefiCpuPkg/UefiCpuPkg.dsc
> @@ -116,8 +116,6 @@
>    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
> 
>    UefiCpuPkg/Library/CpuCacheInfoLib/PeiCpuCacheInfoLib.inf
> 
>    UefiCpuPkg/Library/CpuCacheInfoLib/DxeCpuCacheInfoLib.inf
> 
> 
> 
> --
> 2.28.0.windows.1