From: "Laszlo Ersek" <lersek@redhat.com>
To: "Ni, Ray" <ray.ni@intel.com>, "Lou, Yun" <yun.lou@intel.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: "Dong, Eric" <eric.dong@intel.com>,
"Kumar, Rahul1" <rahul1.kumar@intel.com>
Subject: Re: [PATCH v1 1/1] UefiCpuPkg: Remove PEI/DXE instances of CpuTimerLib.
Date: Fri, 25 Sep 2020 08:46:15 +0200 [thread overview]
Message-ID: <9ae864d9-7eb7-2e60-f043-299a7f7b74a7@redhat.com> (raw)
In-Reply-To: <BY5PR11MB4007F8970DC9D6C614A11A208C360@BY5PR11MB4007.namprd11.prod.outlook.com>
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
>
next prev parent reply other threads:[~2020-09-25 6:46 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-25 3:57 [PATCH v1 1/1] UefiCpuPkg: Remove PEI/DXE instances of CpuTimerLib yun.lou
2020-09-25 5:25 ` Ni, Ray
2020-09-25 6:46 ` Laszlo Ersek [this message]
2020-09-25 14:14 ` [edk2-devel] " Zeng, Star
2021-03-25 7:58 ` Jason Lou
2021-04-01 8:41 ` Jason Lou
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9ae864d9-7eb7-2e60-f043-299a7f7b74a7@redhat.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox