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

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

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