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

[Paolo Bonzini <pbonzini@redhat.com>]

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.

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 ();
> -    MicroSecondDelay (100);
> -    mPerformanceCounterFrequency = MultU64x32 (GetPerformanceCounter () - Count, 10000);
> -    SetInterruptState (InterruptState);
> +    mPerformanceCounterFrequency = InternalCalculateTscFrequency ();
>    }
>    return  mPerformanceCounterFrequency;
>  }
>