Re: [PATCH] PcAtChipsetPkg AcpiTimerLib: Get more accurate TSC Frequency

["Zeng, Star" <star.zeng@intel.com>]

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 <michael.d.kinney@intel.com>
>> Cc: Liming Gao <liming.gao@intel.com>
>> Cc: Paul A Lohr <paul.a.lohr@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Star Zeng <star.zeng@intel.com>
>> ---
>>  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.<BR>
>> +  Copyright (c) 2013 - 2016, Intel Corporation. All rights reserved.<BR>
>>    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.<BR>
>> +  Copyright (c) 2013 - 2016, Intel Corporation. All rights reserved.<BR>
>>    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 <Library/BaseLib.h>
>>
>>  /**
>> +  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.<BR>
>> +  Copyright (c) 2013 - 2016, Intel Corporation. All rights reserved.<BR>
>>    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 <Library/TimerLib.h>
>>  #include <Library/BaseLib.h>
>>
>> +/**
>> +  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 ();