[edk2-devel] RFC: Another solution to the nested interrupt issue

["Ni, Ray" <ray.ni@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 1896 bytes --]

All,
This mail is to bring another approach to solve the stack-overflow due to nested interrupts. Michael solved this problem in OVMF through NestedInterruptTplLib.

I made a draft patch as attached "DxeCore.diff". The patch simply to avoid the interrupt in enable state when TPL is dropped to the interrupted TPL. The interrupt will be enabled later through "IRET".

So, a timer driver has two ways to implement its timer interrupt handler:

1.      Do raise/restore TPL in the TimerInterruptHandler(). But call the APIs in NestedInterruptTplLib.

2.      Do not raise/restore TPL in the TimerInterruptHandler(). So that only DxeCore restores the TPL. And when DxeCore restores the TPL, the interrupt is not enabled when TPL is dropped to the interrupted TPL (as it will be enabled later by "IRET").


Implementing the logic in DxeCore does not prevent the TimerInterruptHandler() from implementing the way #1.
Agree on the draft patch?

My 2nd question is can we set a rule that TimerInterruptHandler() should NOT restore TPL so that way #2 (changing DxeCore) is enough to solve the stack overflow issue due to nested interrupts.

I was aware of the discussion between Laszlo and Michael in end of 2022 but never dig deeply as today into this problem.
I really appreciate the long discussion in the bugzilla (https://bugzilla.tianocore.org/show_bug.cgi?id=4162) and comments in NestedInterruptTplLib. I learned a lot from them and they are quite interesting!

Thanks,
Ray


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114369): https://edk2.groups.io/g/devel/message/114369
Mute This Topic: https://groups.io/mt/103950154/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #1.2: Type: text/html, Size: 8700 bytes --]

[-- Attachment #2: DxeCore.diff --]
[-- Type: application/octet-stream, Size: 2457 bytes --]

diff --git a/MdeModulePkg/Core/Dxe/Event/Timer.c b/MdeModulePkg/Core/Dxe/Event/Timer.c
index 29e507c67c..8e8b9d6592 100644
--- a/MdeModulePkg/Core/Dxe/Event/Timer.c
+++ b/MdeModulePkg/Core/Dxe/Event/Timer.c
@@ -176,6 +176,11 @@ CoreInitializeTimer (
   ASSERT_EFI_ERROR (Status);
 }
 
+VOID
+CoreRestoreTplDeferEnableInterrupt (
+  IN EFI_TPL  NewTpl
+  );
+
 /**
   Called by the platform code to process a tick.
 
@@ -190,11 +195,9 @@ CoreTimerTick (
   )
 {
   IEVENT  *Event;
+  EFI_TPL  OriginalTpl;
 
-  //
-  // Check runtiem flag in case there are ticks while exiting boot services
-  //
-  CoreAcquireLock (&mEfiSystemTimeLock);
+  OriginalTpl = CoreRaiseTpl (TPL_HIGH_LEVEL);
 
   //
   // Update the system time
@@ -213,7 +216,7 @@ CoreTimerTick (
     }
   }
 
-  CoreReleaseLock (&mEfiSystemTimeLock);
+  CoreRestoreTplDeferEnableInterrupt (OriginalTpl);   //CoreRestoreTpl (OriginalTpl);
 }
 
 /**
diff --git a/MdeModulePkg/Core/Dxe/Event/Tpl.c b/MdeModulePkg/Core/Dxe/Event/Tpl.c
index b33f80573c..8d4b92ef4e 100644
--- a/MdeModulePkg/Core/Dxe/Event/Tpl.c
+++ b/MdeModulePkg/Core/Dxe/Event/Tpl.c
@@ -147,3 +147,55 @@ CoreRestoreTpl (
     CoreSetInterruptState (TRUE);
   }
 }
+
+
+VOID
+CoreRestoreTplDeferEnableInterrupt (
+  IN EFI_TPL  NewTpl
+  )
+{
+  EFI_TPL  OldTpl;
+  EFI_TPL  PendingTpl;
+
+  OldTpl = gEfiCurrentTpl;
+  if (NewTpl > OldTpl) {
+    DEBUG ((DEBUG_ERROR, "FATAL ERROR - RestoreTpl with NewTpl(0x%x) > OldTpl(0x%x)\n", NewTpl, OldTpl));
+    ASSERT (FALSE);
+  }
+
+  ASSERT (VALID_TPL (NewTpl));
+
+  //
+  // If lowering below HIGH_LEVEL, make sure
+  // interrupts are enabled
+  //
+
+  if ((OldTpl >= TPL_HIGH_LEVEL) &&  (NewTpl < TPL_HIGH_LEVEL)) {
+    gEfiCurrentTpl = TPL_HIGH_LEVEL;
+  }
+
+  //
+  // Dispatch any pending events
+  //
+  while (gEventPending != 0) {
+    PendingTpl = (UINTN)HighBitSet64 (gEventPending);
+    if (PendingTpl <= NewTpl) {
+      break;
+    }
+
+    gEfiCurrentTpl = PendingTpl;
+    if (gEfiCurrentTpl < TPL_HIGH_LEVEL) {
+      CoreSetInterruptState (TRUE);
+    }
+
+    CoreDispatchEventNotifies (gEfiCurrentTpl);
+  }
+
+  //
+  // Disable interrupt before restoring to the original TPL.
+  // This is to avoid the nest interrupt happens in the same TPL
+  // which will be endless.
+  //
+  CoreSetInterruptState (FALSE);
+  gEfiCurrentTpl = NewTpl;
+}
\ No newline at end of file