Re: [edk2-devel] [edk2 1/1] RiscV64 Timer: fix tick duration accounting

["Sunil V L" <sunilvl@ventanamicro.com>]

On Tue, Feb 21, 2023 at 10:37:23AM +0530, Sunil V L via groups.io wrote:
> Hi Andrei,
> 
> Thank you very much for fixing this!
> 
> On Fri, Feb 17, 2023 at 10:31:49PM -0600, Andrei Warkentin wrote:
> > The TimerDxe implementation doesn't account for the physical
> > time passed due to timer handler execution or (perhaps even
> > more importantly) time spent with interrupts masked.
> > 
> > Other implementations (e.g. like the Arm one) do. If the
> > timer tick is always incremented at a fixed rate, then
> > you can slow down UEFI's perception of time by running
> > long sections of code in a critical section.
> > 
> > Cc: Sunil V L <sunilvl@ventanamicro.com>
> > Cc: Daniel Schaefer <git@danielschaefer.me>
> > Signed-off-by: Andrei Warkentin <andrei.warkentin@intel.com>
> > ---
> >  UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c | 31 ++++++++++++--------
> >  1 file changed, 18 insertions(+), 13 deletions(-)
> > 
> > diff --git a/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c b/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c
> > index 0ecefddf1f18..f65c64c296cd 100644
> > --- a/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c
> > +++ b/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c
> > @@ -41,6 +41,7 @@ STATIC EFI_TIMER_NOTIFY  mTimerNotifyFunction;
> >  // The current period of the timer interrupt
> >  //
> >  STATIC UINT64  mTimerPeriod = 0;
> > +STATIC UINT64  mLastPeriodStart = 0;
> >  
> >  /**
> >    Timer Interrupt Handler.
> > @@ -55,26 +56,32 @@ TimerInterruptHandler (
> >    IN EFI_SYSTEM_CONTEXT  SystemContext
> >    )
> >  {
> > -  EFI_TPL  OriginalTPL;
> > -  UINT64   RiscvTimer;
> > +  EFI_TPL OriginalTPL;
> > +  UINT64  PeriodStart = RiscVReadTimer ();
> >  
> >    OriginalTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL);
> >    if (mTimerNotifyFunction != NULL) {
> > -    mTimerNotifyFunction (mTimerPeriod);
> > +    //
> > +    // For any number of reasons, the ticks could be coming
> > +    // in slower than mTimerPeriod. For example, some code
> > +    // is doing a *lot* of stuff inside an EFI_TPL_HIGH
> > +    // critical section, and this should not cause the EFI
> > +    // time to increment slower. So when we take an interrupt,
> > +    // account for the actual time passed.
> > +    //
> > +    mTimerNotifyFunction (PeriodStart - mLastPeriodStart);
> 
> Shouldn't we consider debug case like ARM does?
> 
Hi Andrei,

Please ignore above comment. It is actually consistent with other
architectures.

Thanks,
Sunil