+/**
+  Timer Interrupt Handler.
+
+  @param InterruptType    The type of interrupt that occured
+  @param SystemContext    A pointer to the system context when the interrupt 
occured
+**/
+VOID
+EFIAPI
+TimerInterruptHandler (
+  IN EFI_EXCEPTION_TYPE  InterruptType,
+  IN EFI_SYSTEM_CONTEXT  SystemContext
+  )
+{
+  EFI_TPL  OriginalTPL;
+  UINT64   RiscvTimer;
+
+  OriginalTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL);
+  if (mTimerNotifyFunction != NULL) {
+    mTimerNotifyFunction (mTimerPeriod);
+  }
+
+  RiscVDisableTimerInterrupt (); // Disable SMode timer int
+  RiscVClearPendingTimerInterrupt ();
+  if (mTimerPeriod == 0) {
+    gBS->RestoreTPL (OriginalTPL);
+    RiscVDisableTimerInterrupt (); // Disable SMode timer int
+    return;
+  }
+
+  RiscvTimer               = RiscVReadTimer ();
+  SbiSetTimer (RiscvTimer += mTimerPeriod);
+  gBS->RestoreTPL (OriginalTPL);
+  RiscVEnableTimerInterrupt (); // enable SMode timer int
+}

This design looks as though it does not support nested timer interrupts. The call to RestoreTPL() may invoke callbacks that may themselves include delay loops that wait upon further timer interrupts. With the above code, those timer interrupts will never arrive since the timer interrupt is disabled at the point that you call RestoreTPL().

This will break device drivers such as those for USB network devices that rely on nested timer interrupts.


The easiest fix is to use NestedTimerInterruptLib and change the code as follows:

Add at the start of the function:

  STATIC NESTED_INTERRUPT_STATE NestedInterruptState;

Change:

  OriginalTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL);

to

  OriginalTPL = NestedInterruptRaiseTPL ();

Change:

  gBS->RestoreTPL (OriginalTPL);
  RiscVEnableTimerInterrupt (); // enable SMode timer int

to

  RiscVEnableTimerInterrupt (); // enable SMode timer int
NestedInterruptRestoreTPL (OriginalTpl, SystemContext, &NestedInterruptState);

Note that the timer interrupt is then correctly re-enabled before the call to NestedInterruptRestoreTPL(). NestedInterruptTplLib takes care of the mess required behind the scenes to make this all provably safe.


You'll also need to extend Library/NestedInterruptTplLib/Iret.c to support MDE_CPU_RISCV64: it should be fairly obvious what's needed there.


See commit https://github.com/tianocore/edk2/commit/a086f4a63b for the equivalent fix being applied to OVMF.


You can find the background leading up to the creation of NestedInterruptTplLib at:

  https://bugzilla.tianocore.org/show_bug.cgi?id=4162
  https://github.com/tianocore/edk2/commit/9bf473da4c
  https://github.com/tianocore/edk2/commit/a24fbd6061
  https://github.com/tianocore/edk2/commit/a086f4a63b

Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99868): https://edk2.groups.io/g/devel/message/99868
Mute This Topic: https://groups.io/mt/96593498/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-


Reply via email to