public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Andrei Warkentin" <andrei.warkentin@intel.com>
To: devel@edk2.groups.io
Cc: Andrei Warkentin <andrei.warkentin@intel.com>,
	Daniel Schaefer <git@danielschaefer.me>,
	Sunil V L <sunilvl@ventanamicro.com>
Subject: [edk2 6/7] [PATCH v2] UefiCpuPkg: CpuTimerDxeRiscV64: fix tick duration accounting
Date: Mon,  6 Mar 2023 15:26:14 -0600	[thread overview]
Message-ID: <20230306212615.7400-7-andrei.warkentin@intel.com> (raw)
In-Reply-To: <20230306212615.7400-1-andrei.warkentin@intel.com>

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: Daniel Schaefer <git@danielschaefer.me>
Reviewed-by: Sunil V L <sunilvl@ventanamicro.com>
Signed-off-by: Andrei Warkentin <andrei.warkentin@intel.com>
---
 UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c | 33 +++++++++++---------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c b/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c
index db153f715e60..50f57a9780f0 100644
--- a/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c
+++ b/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c
@@ -40,7 +40,8 @@ STATIC EFI_TIMER_NOTIFY  mTimerNotifyFunction;
 //
 // The current period of the timer interrupt
 //
-STATIC UINT64  mTimerPeriod = 0;
+STATIC UINT64  mTimerPeriod     = 0;
+STATIC UINT64  mLastPeriodStart = 0;
 
 /**
   Timer Interrupt Handler.
@@ -56,25 +57,31 @@ TimerInterruptHandler (
   )
 {
   EFI_TPL  OriginalTPL;
-  UINT64   RiscvTimer;
+  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) {
@@ -164,9 +169,9 @@ TimerDriverSetTimerPeriod (
     return EFI_SUCCESS;
   }
 
-  mTimerPeriod = TimerPeriod / 10; // convert unit from 100ns to 1us
-  RiscvTimer   = RiscVReadTimer ();
-  SbiSetTimer (RiscvTimer + mTimerPeriod);
+  mTimerPeriod     = TimerPeriod / 10; // convert unit from 100ns to 1us
+  mLastPeriodStart = RiscVReadTimer ();
+  SbiSetTimer (mLastPeriodStart + mTimerPeriod);
 
   mCpu->EnableInterrupt (mCpu);
   RiscVEnableTimerInterrupt (); // enable SMode timer int
-- 
2.25.1


  parent reply	other threads:[~2023-03-06 21:26 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-03-06 21:26 [edk2 0/7] v3 Assorted fixes to core RISC-V and RiscVVirt Andrei Warkentin
2023-03-06 21:26 ` [edk2 1/7] OvmfPkg: RiscVVirt: add SATA support Andrei Warkentin
2023-03-06 21:26 ` [edk2 2/7] MdePkg: BasePeCoffLib: Allow AArch64 and x64 images in ImageFormatSupported Andrei Warkentin
2023-03-06 21:26 ` [edk2 3/7] MdePkg: BaseLib: don't log in RISCV InternalSwitchStack Andrei Warkentin
2023-03-06 21:26 ` [edk2 4/7] MdePkg: BaseCpuLib: Fix RISCV CpuSleep symbol name Andrei Warkentin
2023-03-06 21:26 ` [edk2 5/7] MdeModulePkg: Dxe: add RISCV64 to mMachineTypeInfo Andrei Warkentin
2023-03-06 21:26 ` Andrei Warkentin [this message]
2023-03-06 21:26 ` [edk2 7/7] [PATCH v3] UefiCpuPkg: BaseRiscV64CpuExceptionHandlerLib: clean up exception handling Andrei Warkentin
2023-03-08 18:37 ` [edk2-devel] [edk2 0/7] v3 Assorted fixes to core RISC-V and RiscVVirt Sunil V L

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230306212615.7400-7-andrei.warkentin@intel.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox