From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by ml01.01.org (Postfix) with ESMTP id A4A681A1DFE for ; Fri, 12 Aug 2016 01:44:01 -0700 (PDT) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga103.jf.intel.com with ESMTP; 12 Aug 2016 01:43:57 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.28,509,1464678000"; d="scan'208";a="1034347229" Received: from shzintpr01.sh.intel.com (HELO [10.7.209.21]) ([10.239.4.80]) by orsmga002.jf.intel.com with ESMTP; 12 Aug 2016 01:43:56 -0700 To: Paolo Bonzini , edk2-devel@lists.01.org References: <1470883079-4472-1-git-send-email-star.zeng@intel.com> <0229321a-849c-b264-7b26-146d6608c754@redhat.com> Cc: Michael D Kinney , Liming Gao From: "Zeng, Star" Message-ID: Date: Fri, 12 Aug 2016 16:43:26 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <0229321a-849c-b264-7b26-146d6608c754@redhat.com> Subject: Re: [PATCH] PcAtChipsetPkg AcpiTimerLib: Get more accurate TSC Frequency X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 12 Aug 2016 08:44:01 -0000 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit On 2016/8/12 16:31, Paolo Bonzini wrote: > > > On 11/08/2016 04:37, Star Zeng wrote: >> Minimize the code overhead between the two TSC reads by adding >> new internal API to calculate TSC Frequency instead of reusing >> MicroSecondDelay (). >> >> Cc: Michael D Kinney >> Cc: Liming Gao >> Cc: Paul A Lohr >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Star Zeng >> --- >> PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c | 56 +++++++++++++++++++++- >> .../Library/AcpiTimerLib/BaseAcpiTimerLib.c | 33 ++++++++----- >> .../Library/AcpiTimerLib/DxeAcpiTimerLib.c | 31 ++++++++---- >> 3 files changed, 99 insertions(+), 21 deletions(-) >> >> diff --git a/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c b/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c >> index 806a4f7ce24c..e6fea231123d 100644 >> --- a/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c >> +++ b/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c >> @@ -1,7 +1,7 @@ >> /** @file >> ACPI Timer implements one instance of Timer Library. >> >> - Copyright (c) 2013 - 2015, Intel Corporation. All rights reserved.
>> + Copyright (c) 2013 - 2016, Intel Corporation. All rights reserved.
>> This program and the accompanying materials >> are licensed and made available under the terms and conditions of the BSD License >> which accompanies this distribution. The full text of the license may be found at >> @@ -335,3 +335,57 @@ GetTimeInNanoSecond ( >> >> return NanoSeconds; >> } >> + >> +/** >> + Calculate TSC frequency. >> + >> + The TSC counting frequency is determined by comparing how far it counts >> + during a 100us period as determined by the ACPI timer. The ACPI timer is >> + used because it counts at a known frequency. >> + The TSC is sampled, followed by waiting for ACPI_TIMER_FREQUENCY / 10000 >> + clocks of the ACPI timer, or 100us. The TSC is then sampled again. The >> + difference multiplied by 10000 is the TSC frequency. There will be a small >> + error because of the overhead of reading the ACPI timer. An attempt is >> + made to determine and compensate for this error. >> + >> + @return The number of TSC counts per second. >> + >> +**/ >> +UINT64 >> +InternalCalculateTscFrequency ( >> + VOID >> + ) >> +{ >> + UINT64 StartTSC; >> + UINT64 EndTSC; >> + UINT16 TimerAddr; >> + UINT32 Ticks; >> + UINT64 TscFrequency; >> + BOOLEAN InterruptState; >> + >> + InterruptState = SaveAndDisableInterrupts (); >> + >> + TimerAddr = InternalAcpiGetAcpiTimerIoPort (); >> + Ticks = IoRead32 (TimerAddr) + (ACPI_TIMER_FREQUENCY / 10000); // Set Ticks to 100us in the future > > ACPI_TIMER_FREQUENCY is 3579545, thus you're waiting 357 ticks but the > actual result of the division is much closer to 358. The error is only > 0.26%, but it's so simple to reduce it that I think it's worth it. Just > change (ACPI_TIMER_FREQUENCY / 10000) to (ACPI_TIMER_FREQUENCY + 5000) / > 10000. Paolo, It is nice to have it I think, and I prefer to use (ACPI_TIMER_FREQUENCY + 5000) / 10000. Since I have pushed this patch, do you mind to contribute the proposal with a new patch? Thanks, Star > > Another possibility is to count 343 ticks and multiply by 10436; 343 * > 10436 is almost exactly ACPI_TIMER_FREQUENCY. > > Paolo > >> + StartTSC = AsmReadTsc (); // Get base value for the TSC >> + // >> + // Wait until the ACPI timer has counted 100us. >> + // Timer wrap-arounds are handled correctly by this function. >> + // When the current ACPI timer value is greater than 'Ticks', the while loop will exit. >> + // >> + while (((Ticks - IoRead32 (TimerAddr)) & BIT23) == 0) { >> + CpuPause(); >> + } >> + EndTSC = AsmReadTsc (); // TSC value 100us later >> + >> + TscFrequency = MultU64x32 ( >> + (EndTSC - StartTSC), // Number of TSC counts in 100us >> + 10000 // Number of 100us in a second >> + ); >> + >> + SetInterruptState (InterruptState); >> + >> + return TscFrequency; >> +} >> + >> diff --git a/PcAtChipsetPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c b/PcAtChipsetPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c >> index 21fdb79908b8..8819ebcfccef 100644 >> --- a/PcAtChipsetPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c >> +++ b/PcAtChipsetPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c >> @@ -1,7 +1,7 @@ >> /** @file >> ACPI Timer implements one instance of Timer Library. >> >> - Copyright (c) 2013 - 2014, Intel Corporation. All rights reserved.
>> + Copyright (c) 2013 - 2016, Intel Corporation. All rights reserved.
>> This program and the accompanying materials >> are licensed and made available under the terms and conditions of the BSD License >> which accompanies this distribution. The full text of the license may be found at >> @@ -17,6 +17,26 @@ >> #include >> >> /** >> + Calculate TSC frequency. >> + >> + The TSC counting frequency is determined by comparing how far it counts >> + during a 100us period as determined by the ACPI timer. The ACPI timer is >> + used because it counts at a known frequency. >> + The TSC is sampled, followed by waiting for ACPI_TIMER_FREQUENCY / 10000 >> + clocks of the ACPI timer, or 100us. The TSC is then sampled again. The >> + difference multiplied by 10000 is the TSC frequency. There will be a small >> + error because of the overhead of reading the ACPI timer. An attempt is >> + made to determine and compensate for this error. >> + >> + @return The number of TSC counts per second. >> + >> +**/ >> +UINT64 >> +InternalCalculateTscFrequency ( >> + VOID >> + ); >> + >> +/** >> Internal function to retrieves the 64-bit frequency in Hz. >> >> Internal function to retrieves the 64-bit frequency in Hz. >> @@ -29,14 +49,5 @@ InternalGetPerformanceCounterFrequency ( >> VOID >> ) >> { >> - BOOLEAN InterruptState; >> - UINT64 Count; >> - UINT64 Frequency; >> - >> - InterruptState = SaveAndDisableInterrupts (); >> - Count = GetPerformanceCounter (); >> - MicroSecondDelay (100); >> - Frequency = MultU64x32 (GetPerformanceCounter () - Count, 10000); >> - SetInterruptState (InterruptState); >> - return Frequency; >> + return InternalCalculateTscFrequency (); >> } >> diff --git a/PcAtChipsetPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.c b/PcAtChipsetPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.c >> index 6f5c07a4f0b4..7f7b0f8f6294 100644 >> --- a/PcAtChipsetPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.c >> +++ b/PcAtChipsetPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.c >> @@ -1,7 +1,7 @@ >> /** @file >> ACPI Timer implements one instance of Timer Library. >> >> - Copyright (c) 2013 - 2014, Intel Corporation. All rights reserved.
>> + Copyright (c) 2013 - 2016, Intel Corporation. All rights reserved.
>> This program and the accompanying materials >> are licensed and made available under the terms and conditions of the BSD License >> which accompanies this distribution. The full text of the license may be found at >> @@ -16,6 +16,26 @@ >> #include >> #include >> >> +/** >> + Calculate TSC frequency. >> + >> + The TSC counting frequency is determined by comparing how far it counts >> + during a 100us period as determined by the ACPI timer. The ACPI timer is >> + used because it counts at a known frequency. >> + The TSC is sampled, followed by waiting for ACPI_TIMER_FREQUENCY / 10000 >> + clocks of the ACPI timer, or 100us. The TSC is then sampled again. The >> + difference multiplied by 10000 is the TSC frequency. There will be a small >> + error because of the overhead of reading the ACPI timer. An attempt is >> + made to determine and compensate for this error. >> + >> + @return The number of TSC counts per second. >> + >> +**/ >> +UINT64 >> +InternalCalculateTscFrequency ( >> + VOID >> + ); >> + >> // >> // Cached performance counter frequency >> // >> @@ -34,15 +54,8 @@ InternalGetPerformanceCounterFrequency ( >> VOID >> ) >> { >> - BOOLEAN InterruptState; >> - UINT64 Count; >> - >> if (mPerformanceCounterFrequency == 0) { >> - InterruptState = SaveAndDisableInterrupts (); >> - Count = GetPerformanceCounter ();