Re: [PATCH] PcAtChipsetPkg AcpiTimerLib: Wait 363 ACPI timer counts to get TSC Freq

["Kinney, Michael D" <michael.d.kinney@intel.com>]

Hi Paolo,

We did some experiments to wait for the next ACPI Timer tick, and they did 
not improve the results.

On real HW, the I/O Port read of the ACPI Timer takes ~1 uS, and an ACPI Timer
tick is about 0.25 uS, so the action to read the counter takes 4 times longer
than a single count.

If the I/O port read was much faster than the tick period, then adding the loop
would make sense.

Mike



> -----Original Message-----
> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini
> Sent: Wednesday, August 17, 2016 3:31 AM
> To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> <liming.gao@intel.com>
> Subject: Re: [edk2] [PATCH] PcAtChipsetPkg AcpiTimerLib: Wait 363 ACPI timer counts
> to get TSC Freq
> 
> 
> 
> On 17/08/2016 04:26, Star Zeng wrote:
> > Compute the number of ticks to wait to measure TSC frequency.
> > Instead of (ACPI_TIMER_FREQUENCY / 10000) = 357 and 357 * 10000 = 3570000,
> > use 363 * 9861 = 3579543 Hz which is within 2 Hz of ACPI_TIMER_FREQUENCY.
> > 363 counts is a calibration time of 101.4 uS.
> >
> > The idea comes from Michael and Paolo.
> >
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.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 | 32 +++++++++++++---------
> >  .../Library/AcpiTimerLib/BaseAcpiTimerLib.c        | 14 +++++-----
> >  .../Library/AcpiTimerLib/DxeAcpiTimerLib.c         | 14 +++++-----
> >  3 files changed, 33 insertions(+), 27 deletions(-)
> >
> > diff --git a/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c
> b/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c
> > index e6fea231123d..020031e3f4a5 100644
> > --- a/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c
> > +++ b/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c
> > @@ -340,13 +340,13 @@ GetTimeInNanoSecond (
> >    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.
> > +  during a 101.4 us 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 363 counts of the ACPI timer,
> > +  or 101.4 us. The TSC is then sampled again. The difference multiplied by
> > +  9861 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.
> >
> > @@ -366,22 +366,28 @@ InternalCalculateTscFrequency (
> >    InterruptState = SaveAndDisableInterrupts ();
> >
> >    TimerAddr = InternalAcpiGetAcpiTimerIoPort ();
> > -  Ticks = IoRead32 (TimerAddr) + (ACPI_TIMER_FREQUENCY / 10000);    // Set Ticks
> to 100us in the future
> > +  //
> > +  // Compute the number of ticks to wait to measure TSC frequency.
> > +  // Use 363 * 9861 = 3579543 Hz which is within 2 Hz of ACPI_TIMER_FREQUENCY.
> > +  // 363 counts is a calibration time of 101.4 uS.
> > +  //
> > +  Ticks = IoRead32 (TimerAddr) + 363;
> >
> >    StartTSC = AsmReadTsc ();                                         // Get base
> value for the TSC
> >    //
> > -  // Wait until the ACPI timer has counted 100us.
> > +  // Wait until the ACPI timer has counted 101.4 us.
> >    // Timer wrap-arounds are handled correctly by this function.
> > -  // When the current ACPI timer value is greater than 'Ticks', the while loop
> will exit.
> > +  // 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
> > +  EndTSC = AsmReadTsc ();                                           // TSC value
> 101.4 us later
> >
> >    TscFrequency = MultU64x32 (
> > -                   (EndTSC - StartTSC),                             // Number of
> TSC counts in 100us
> > -                   10000                                            // Number of
> 100us in a second
> > +                   (EndTSC - StartTSC),                             // Number of
> TSC counts in 101.4 us
> > +                   9861                                             // Number of
> 101.4 us in a second
> >                     );
> >
> >    SetInterruptState (InterruptState);
> > diff --git a/PcAtChipsetPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c
> b/PcAtChipsetPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c
> > index 8819ebcfccef..29521f8b220b 100644
> > --- a/PcAtChipsetPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c
> > +++ b/PcAtChipsetPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c
> > @@ -20,13 +20,13 @@
> >    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.
> > +  during a 101.4 us 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 363 counts of the ACPI timer,
> > +  or 101.4 us. The TSC is then sampled again. The difference multiplied by
> > +  9861 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.
> >
> > diff --git a/PcAtChipsetPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.c
> b/PcAtChipsetPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.c
> > index 7f7b0f8f6294..b141c680fb82 100644
> > --- a/PcAtChipsetPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.c
> > +++ b/PcAtChipsetPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.c
> > @@ -20,13 +20,13 @@
> >    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.
> > +  during a 101.4 us 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 363 counts of the ACPI timer,
> > +  or 101.4 us. The TSC is then sampled again. The difference multiplied by
> > +  9861 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.
> >
> >
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> On top of this you can add the logic to wait for the beginning of a tick
> before calling rdtsc.
> 
> Paolo