Skip to content

Commit 0bc604c

Browse files
niruiyuPatchew Applier
authored andcommitted
RFC: Another solution to the nested interrupt issue
--_000_MN6PR11MB82443B73244CEF84481629F08C7A2MN6PR11MB8244namp_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable All, This mail is to bring another approach to solve the stack-overflow due to n= ested interrupts. Michael solved this problem in OVMF through NestedInterru= ptTplLib. 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. T= he 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 A= PIs in NestedInterruptTplLib. 2. Do not raise/restore TPL in the TimerInterruptHandler(). So that on= ly DxeCore restores the TPL. And when DxeCore restores the TPL, the interru= pt 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 TimerInterruptHandle= r() from implementing the way tianocore#1. Agree on the draft patch? My 2nd question is can we set a rule that TimerInterruptHandler() should NO= T restore TPL so that way tianocore#2 (changing DxeCore) is enough to solve the stac= k 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.t= ianocore.org/show_bug.cgi?id=3D4162) and comments in NestedInterruptTplLib.= I learned a lot from them and they are quite interesting! Thanks, Ray -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- 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/1787277 Group Owner: [email protected] Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076= /xyzzy [[email protected]] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- <html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"> <head> <meta http-equiv="Content-Type" content="text/html; charset=us-ascii"> <meta name="Generator" content="Microsoft Word 15 (filtered medium)"> <style><!-- /* Font Definitions */ @font-face {font-family:"Cambria Math"; panose-1:2 4 5 3 5 4 6 3 2 4;} @font-face {font-family:DengXian; panose-1:2 1 6 0 3 1 1 1 1 1;} @font-face {font-family:Calibri; panose-1:2 15 5 2 2 2 4 3 2 4;} @font-face {font-family:Aptos;} @font-face {font-family:DengXian; panose-1:2 1 6 0 3 1 1 1 1 1;} /* Style Definitions */ p.MsoNormal, li.MsoNormal, div.MsoNormal {margin:0cm; font-size:11.0pt; font-family:"Calibri",sans-serif; mso-ligatures:standardcontextual;} a:link, span.MsoHyperlink {mso-style-priority:99; color:#0563C1; text-decoration:underline;} p.MsoListParagraph, li.MsoListParagraph, div.MsoListParagraph {mso-style-priority:34; margin:0cm; text-indent:21.0pt; font-size:11.0pt; font-family:"Calibri",sans-serif; mso-ligatures:standardcontextual;} span.EmailStyle17 {mso-style-type:personal-compose; font-family:"Calibri",sans-serif; color:windowtext;} .MsoChpDefault {mso-style-type:export-only; font-size:11.0pt; font-family:"Calibri",sans-serif;} /* Page Definitions */ @page WordSection1 {size:612.0pt 792.0pt; margin:72.0pt 90.0pt 72.0pt 90.0pt;} div.WordSection1 {page:WordSection1;} /* List Definitions */ @list l0 {mso-list-id:1439645819; mso-list-type:hybrid; mso-list-template-ids:-972123134 722654836 67698713 67698715 67698703 67698713 67698715 67698703 67698713 67698715;} @list l0:level1 {mso-level-tab-stop:none; mso-level-number-position:left; margin-left:18.0pt; text-indent:-18.0pt;} @list l0:level2 {mso-level-number-format:alpha-lower; mso-level-text:"%2\)"; mso-level-tab-stop:none; mso-level-number-position:left; margin-left:44.0pt; text-indent:-22.0pt;} @list l0:level3 {mso-level-number-format:roman-lower; mso-level-tab-stop:none; mso-level-number-position:right; margin-left:66.0pt; text-indent:-22.0pt;} @list l0:level4 {mso-level-tab-stop:none; mso-level-number-position:left; margin-left:88.0pt; text-indent:-22.0pt;} @list l0:level5 {mso-level-number-format:alpha-lower; mso-level-text:"%5\)"; mso-level-tab-stop:none; mso-level-number-position:left; margin-left:110.0pt; text-indent:-22.0pt;} @list l0:level6 {mso-level-number-format:roman-lower; mso-level-tab-stop:none; mso-level-number-position:right; margin-left:132.0pt; text-indent:-22.0pt;} @list l0:level7 {mso-level-tab-stop:none; mso-level-number-position:left; margin-left:154.0pt; text-indent:-22.0pt;} @list l0:level8 {mso-level-number-format:alpha-lower; mso-level-text:"%8\)"; mso-level-tab-stop:none; mso-level-number-position:left; margin-left:176.0pt; text-indent:-22.0pt;} @list l0:level9 {mso-level-number-format:roman-lower; mso-level-tab-stop:none; mso-level-number-position:right; margin-left:198.0pt; text-indent:-22.0pt;} ol {margin-bottom:0cm;} ul {margin-bottom:0cm;} --></style><!--[if gte mso 9]><xml> <o:shapedefaults v:ext="edit" spidmax="1026" /> </xml><![endif]--><!--[if gte mso 9]><xml> <o:shapelayout v:ext="edit"> <o:idmap v:ext="edit" data="1" /> </o:shapelayout></xml><![endif]--> </head> <body lang="ZH-CN" link="#0563C1" vlink="#954F72" style="word-wrap:break-word;text-justify-trim:punctuation"> <div class="WordSection1"> <p class="MsoNormal"><span lang="EN-US" style="font-size:10.5pt">All,<o:p></o:p></span></p> <p class="MsoNormal"><span lang="EN-US" style="font-size:10.5pt">This mail is to bring another approach to solve the stack-overflow due to nested interrupts. Michael solved this problem in OVMF through NestedInterruptTplLib.<o:p></o:p></span></p> <p class="MsoNormal"><span lang="EN-US" style="font-size:10.5pt"><o:p>&nbsp;</o:p></span></p> <p class="MsoNormal"><span lang="EN-US" style="font-size:10.5pt">I made a draft patch as attached &tianocore#8220;DxeCore.diff&tianocore#8221;. 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 &tianocore#8220;IRET&tianocore#8221;.<o:p></o:p></span></p> <p class="MsoNormal"><span lang="EN-US" style="font-size:10.5pt"><o:p>&nbsp;</o:p></span></p> <p class="MsoNormal"><span lang="EN-US" style="font-size:10.5pt">So, a timer driver has two ways to implement its timer interrupt handler:<o:p></o:p></span></p> <p class="MsoListParagraph" style="margin-left:18.0pt;text-indent:-18.0pt;mso-list:l0 level1 lfo1"> <![if !supportLists]><span lang="EN-US" style="font-size:10.5pt"><span style="mso-list:Ignore">1.<span style="font:7.0pt &quot;Times New Roman&quot;">&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; </span></span></span><![endif]><span lang="EN-US" style="font-size:10.5pt">Do raise/restore TPL in the TimerInterruptHandler(). But call the APIs in NestedInterruptTplLib.<o:p></o:p></span></p> <p class="MsoListParagraph" style="margin-left:18.0pt;text-indent:-18.0pt;mso-list:l0 level1 lfo1"> <![if !supportLists]><span lang="EN-US" style="font-size:10.5pt"><span style="mso-list:Ignore">2.<span style="font:7.0pt &quot;Times New Roman&quot;">&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; </span></span></span><![endif]><span lang="EN-US" style="font-size:10.5pt">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 &tianocore#8220;IRET&tianocore#8221;).<o:p></o:p></span></p> <p class="MsoNormal"><span lang="EN-US" style="font-size:10.5pt"><o:p>&nbsp;</o:p></span></p> <p class="MsoNormal"><span lang="EN-US" style="font-size:10.5pt"><o:p>&nbsp;</o:p></span></p> <p class="MsoNormal"><span lang="EN-US" style="font-size:10.5pt">Implementing the logic in DxeCore does not prevent the TimerInterruptHandler() from implementing the way tianocore#1.<o:p></o:p></span></p> <p class="MsoNormal"><span lang="EN-US" style="font-size:10.5pt">Agree on the draft patch?<o:p></o:p></span></p> <p class="MsoNormal"><span lang="EN-US" style="font-size:10.5pt"><o:p>&nbsp;</o:p></span></p> <p class="MsoNormal"><span lang="EN-US" style="font-size:10.5pt">My 2<sup>nd</sup> question is can we set a rule that TimerInterruptHandler() should NOT restore TPL so that way tianocore#2 (changing DxeCore) is enough to solve the stack overflow issue due to nested interrupts.<o:p></o:p></span></p> <p class="MsoNormal"><span lang="EN-US" style="font-size:10.5pt"><o:p>&nbsp;</o:p></span></p> <p class="MsoNormal"><span lang="EN-US" style="font-size:10.5pt">I was aware of the discussion between Laszlo and Michael in end of 2022 but never dig deeply as today into this problem.<o:p></o:p></span></p> <p class="MsoNormal"><span lang="EN-US" style="font-size:10.5pt">I really appreciate the long discussion in the bugzilla (<a href="https://bugzilla.tianocore.org/show_bug.cgi?id=4162">https://bugzilla.tianocore.org/show_bug.cgi?id=4162</a>) and comments in NestedInterruptTplLib. I learned a lot from them and they are quite interesting!<o:p></o:p></span></p> <div> <p class="MsoNormal"><span lang="EN-US" style="font-family:&quot;Aptos&quot;,sans-serif;color:black;mso-ligatures:none"><o:p>&nbsp;</o:p></span></p> <p class="MsoNormal"><span lang="EN-US" style="font-family:&quot;Aptos&quot;,sans-serif;color:black;mso-ligatures:none">Thanks,<o:p></o:p></span></p> </div> <p class="MsoNormal"><span lang="EN-US" style="font-family:&quot;Aptos&quot;,sans-serif;color:black;mso-ligatures:none">Ray</span><span lang="EN-US"><o:p></o:p></span></p> </div> </body> </html> <div width="1" style="color:white;clear:both">_._,_._,_</div> <hr> Groups.io Links:<p> You receive all messages sent to this group. <p> <a target="_blank" href="https://edk2.groups.io/g/devel/message/114369">View/Reply Online (#114369)</a> | | <a target="_blank" href="https://groups.io/mt/103950154/1787277">Mute This Topic</a> | <a href="https://edk2.groups.io/g/devel/post">New Topic</a> <br> <a href="https://edk2.groups.io/g/devel/editsub/1787277">Your Subscription</a> | <a href="mailto:[email protected]">Contact Group Owner</a> | <a href="https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy">Unsubscribe</a> [[email protected]]<br> <div width="1" style="color:white;clear:both">_._,_._,_</div> Message-Id: <MN6PR11MB82443B73244CEF84481629F08C7A2@MN6PR11MB8244.namprd11.prod.outlook.com>
1 parent ff52277 commit 0bc604c

File tree

2 files changed

+60
-5
lines changed

2 files changed

+60
-5
lines changed

MdeModulePkg/Core/Dxe/Event/Timer.c

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -176,6 +176,11 @@ CoreInitializeTimer (
176176
ASSERT_EFI_ERROR (Status);
177177
}
178178

179+
VOID
180+
CoreRestoreTplDeferEnableInterrupt (
181+
IN EFI_TPL NewTpl
182+
);
183+
179184
/**
180185
Called by the platform code to process a tick.
181186
@@ -190,11 +195,9 @@ CoreTimerTick (
190195
)
191196
{
192197
IEVENT *Event;
198+
EFI_TPL OriginalTpl;
193199

194-
//
195-
// Check runtiem flag in case there are ticks while exiting boot services
196-
//
197-
CoreAcquireLock (&mEfiSystemTimeLock);
200+
OriginalTpl = CoreRaiseTpl (TPL_HIGH_LEVEL);
198201

199202
//
200203
// Update the system time
@@ -213,7 +216,7 @@ CoreTimerTick (
213216
}
214217
}
215218

216-
CoreReleaseLock (&mEfiSystemTimeLock);
219+
CoreRestoreTplDeferEnableInterrupt (OriginalTpl); //CoreRestoreTpl (OriginalTpl);
217220
}
218221

219222
/**

MdeModulePkg/Core/Dxe/Event/Tpl.c

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,3 +147,55 @@ CoreRestoreTpl (
147147
CoreSetInterruptState (TRUE);
148148
}
149149
}
150+
151+
152+
VOID
153+
CoreRestoreTplDeferEnableInterrupt (
154+
IN EFI_TPL NewTpl
155+
)
156+
{
157+
EFI_TPL OldTpl;
158+
EFI_TPL PendingTpl;
159+
160+
OldTpl = gEfiCurrentTpl;
161+
if (NewTpl > OldTpl) {
162+
DEBUG ((DEBUG_ERROR, "FATAL ERROR - RestoreTpl with NewTpl(0x%x) > OldTpl(0x%x)\n", NewTpl, OldTpl));
163+
ASSERT (FALSE);
164+
}
165+
166+
ASSERT (VALID_TPL (NewTpl));
167+
168+
//
169+
// If lowering below HIGH_LEVEL, make sure
170+
// interrupts are enabled
171+
//
172+
173+
if ((OldTpl >= TPL_HIGH_LEVEL) && (NewTpl < TPL_HIGH_LEVEL)) {
174+
gEfiCurrentTpl = TPL_HIGH_LEVEL;
175+
}
176+
177+
//
178+
// Dispatch any pending events
179+
//
180+
while (gEventPending != 0) {
181+
PendingTpl = (UINTN)HighBitSet64 (gEventPending);
182+
if (PendingTpl <= NewTpl) {
183+
break;
184+
}
185+
186+
gEfiCurrentTpl = PendingTpl;
187+
if (gEfiCurrentTpl < TPL_HIGH_LEVEL) {
188+
CoreSetInterruptState (TRUE);
189+
}
190+
191+
CoreDispatchEventNotifies (gEfiCurrentTpl);
192+
}
193+
194+
//
195+
// Disable interrupt before restoring to the original TPL.
196+
// This is to avoid the nest interrupt happens in the same TPL
197+
// which will be endless.
198+
//
199+
CoreSetInterruptState (FALSE);
200+
gEfiCurrentTpl = NewTpl;
201+
}

0 commit comments

Comments
 (0)