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

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

[Andrei Warkentin]

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);
   }
 
-  RiscVDisableTimerInterrupt (); // Disable SMode timer int
-  RiscVClearPendingTimerInterrupt ();
   if (mTimerPeriod == 0) {
+    RiscVDisableTimerInterrupt ();
     gBS->RestoreTPL (OriginalTPL);
-    RiscVDisableTimerInterrupt (); // Disable SMode timer int
     return;
   }
 
-  RiscvTimer               = RiscVReadTimer ();
-  SbiSetTimer (RiscvTimer += mTimerPeriod);
+  mLastPeriodStart = PeriodStart;
+  SbiSetTimer (PeriodStart += mTimerPeriod);
+  RiscVEnableTimerInterrupt (); // enable SMode timer int
   gBS->RestoreTPL (OriginalTPL);
-  RiscVEnableTimerInterrupt (); // enable SMode timer int
 }
 
 /**
@@ -154,8 +161,6 @@ TimerDriverSetTimerPeriod (
   IN UINT64                   TimerPeriod
   )
 {
-  UINT64  RiscvTimer;
-
   DEBUG ((DEBUG_INFO, "TimerDriverSetTimerPeriod(0x%lx)\n", TimerPeriod));
 
   if (TimerPeriod == 0) {
@@ -165,8 +170,8 @@ TimerDriverSetTimerPeriod (
   }
 
   mTimerPeriod = TimerPeriod / 10; // convert unit from 100ns to 1us
-  RiscvTimer   = RiscVReadTimer ();
-  SbiSetTimer (RiscvTimer + mTimerPeriod);
+  mLastPeriodStart = RiscVReadTimer ();
+  SbiSetTimer (mLastPeriodStart + mTimerPeriod);
 
   mCpu->EnableInterrupt (mCpu);
   RiscVEnableTimerInterrupt (); // enable SMode timer int
-- 
2.25.1

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

[Sunil V L]

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?

Also, looks like there are some uncrustify errors. Please run manually
and fix them.

https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Code-Formatting#manual-usage---generate-file-list-via-stdin

Thanks,
Sunil

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

[Sunil V L]

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