Laszlo, OVMF isn't using this timerlib so I will assume you doesn't care about this change.
Thanks, Ray > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray > Sent: Thursday, April 8, 2021 8:37 AM > To: Lou, Yun <yun....@intel.com>; devel@edk2.groups.io > Cc: Dong, Eric <eric.d...@intel.com>; Laszlo Ersek <ler...@redhat.com>; > Kumar, Rahul1 <rahul1.ku...@intel.com> > Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg: Remove PEI/DXE > instances of CpuTimerLib. > > Thanks for providing the detailed commit messages. > Reviewed-by: Ray Ni <ray...@intel.com> > > > > -----Original Message----- > > From: Lou, Yun <yun....@intel.com> > > Sent: Wednesday, April 7, 2021 4:16 PM > > To: devel@edk2.groups.io > > Cc: Lou, Yun <yun....@intel.com>; Ni, Ray <ray...@intel.com>; Dong, Eric > <eric.d...@intel.com>; Laszlo Ersek > > <ler...@redhat.com>; Kumar, Rahul1 <rahul1.ku...@intel.com> > > Subject: [PATCH v2] UefiCpuPkg: Remove PEI/DXE instances of > CpuTimerLib. > > > > From: Jason Lou <yun....@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....@intel.com> > > Cc: Ray Ni <ray...@intel.com> > > Cc: Eric Dong <eric.d...@intel.com> > > Cc: Laszlo Ersek <ler...@redhat.com> > > Cc: Rahul Kumar <rahul1.ku...@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.i > nf > > > > 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 > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#73887): https://edk2.groups.io/g/devel/message/73887 Mute This Topic: https://groups.io/mt/81910860/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
