[edk2-devel] [PATCH v2] UefiCpuPkg: Remove PEI/DXE instances of CpuTimerLib.
Ni, Ray
ray.ni at intel.com
Fri Apr 9 00:45:04 UTC 2021
Laszlo,
OVMF isn't using this timerlib so I will assume you doesn't care about this change.
Thanks,
Ray
> -----Original Message-----
> From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Ni, Ray
> Sent: Thursday, April 8, 2021 8:37 AM
> To: Lou, Yun <yun.lou at intel.com>; devel at edk2.groups.io
> Cc: Dong, Eric <eric.dong at intel.com>; Laszlo Ersek <lersek at redhat.com>;
> Kumar, Rahul1 <rahul1.kumar at 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.ni at intel.com>
>
>
> > -----Original Message-----
> > From: Lou, Yun <yun.lou at intel.com>
> > Sent: Wednesday, April 7, 2021 4:16 PM
> > To: devel at edk2.groups.io
> > Cc: Lou, Yun <yun.lou at intel.com>; Ni, Ray <ray.ni at intel.com>; Dong, Eric
> <eric.dong at intel.com>; Laszlo Ersek
> > <lersek at redhat.com>; Kumar, Rahul1 <rahul1.kumar at intel.com>
> > Subject: [PATCH v2] UefiCpuPkg: Remove PEI/DXE instances of
> CpuTimerLib.
> >
> > From: Jason Lou <yun.lou at 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 at intel.com>
> > Cc: Ray Ni <ray.ni at intel.com>
> > Cc: Eric Dong <eric.dong at intel.com>
> > Cc: Laszlo Ersek <lersek at redhat.com>
> > Cc: Rahul Kumar <rahul1.kumar at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list