From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mx.groups.io with SMTP id smtpd.web11.1890.1601016387964395341 for ; Thu, 24 Sep 2020 23:46:28 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=P4d22IAx; spf=pass (domain: redhat.com, ip: 63.128.21.124, mailfrom: lersek@redhat.com) Dkim-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1601016387; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=vHXVk6aa9AAPcumaU1Dy9GyFqDyedizrkbe/EqiC/gs=; b=P4d22IAxK7vJBDEf7VB6q/70A6EJ9XVFLA50T/GJGrdJ7N48LpLnRoSnzzvoKtECcpnWcs p87SgGRwUtPkkEXZJSkvV7vgbIfqKkTZeu7LPuZKYQkEOs17TEBRq4V9vtZiMEj3LFydHW 7sxWDJP506C1oqoBfUkMbwIY4drWGHg= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-176-8jGXoqJQP1-j9fH_BQy9xQ-1; Fri, 25 Sep 2020 02:46:19 -0400 X-MC-Unique: 8jGXoqJQP1-j9fH_BQy9xQ-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 8361864142; Fri, 25 Sep 2020 06:46:18 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-178.ams2.redhat.com [10.36.112.178]) by smtp.corp.redhat.com (Postfix) with ESMTP id C975760C17; Fri, 25 Sep 2020 06:46:16 +0000 (UTC) Subject: Re: [PATCH v1 1/1] UefiCpuPkg: Remove PEI/DXE instances of CpuTimerLib. To: "Ni, Ray" , "Lou, Yun" , "devel@edk2.groups.io" Cc: "Dong, Eric" , "Kumar, Rahul1" References: <20200925035755.3442-1-yun.lou@intel.com> From: "Laszlo Ersek" Message-ID: <9ae864d9-7eb7-2e60-f043-299a7f7b74a7@redhat.com> Date: Fri, 25 Sep 2020 08:46:15 +0200 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 09/25/20 07:25, Ni, Ray wrote: > Reviewed-by: Ray Ni Acked-by: Laszlo Ersek Thanks Laszlo > >> -----Original Message----- >> From: Lou, Yun >> Sent: Friday, September 25, 2020 11:58 AM >> To: devel@edk2.groups.io >> Cc: Lou, Yun ; Ni, Ray ; Dong, Eric ; Laszlo Ersek >> ; Kumar, Rahul1 >> 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 >> Cc: Ray Ni >> Cc: Eric Dong >> Cc: Laszlo Ersek >> Cc: Rahul Kumar >> --- >> 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.
>> >> - SPDX-License-Identifier: BSD-2-Clause-Patent >> >> - >> >> -**/ >> >> - >> >> -#include >> >> -#include >> >> -#include >> >> -#include >> >> - >> >> -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.
>> >> - SPDX-License-Identifier: BSD-2-Clause-Patent >> >> - >> >> -**/ >> >> - >> >> -#include >> >> -#include >> >> -#include >> >> -#include >> >> -#include >> >> - >> >> -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.
>> >> -# 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.
>> >> -// >> >> -// 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.
>> >> -# 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.
>> >> -// >> >> -// 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.
>> >> +# Copyright (c) 2007 - 2020, Intel Corporation. All rights reserved.
>> >> # >> >> # 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 >