From 23c4f60cf79f29ab5eff55a02c72bb504804d02a Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 29 Feb 2024 23:54:50 +0100 Subject: [PATCH 1/2] MdeModulePkg: introduce CoreRestoreTplInternal() Content-Type: text/plain; charset=UTF-8 Introduce a function that restores the TPL just like gBS->RestoreTPL(), but can optionally return with interrupts disabled. This can be used from interrupt handlers, so that any nested interrupt handler will only see an elevated TPL and not the value on entry to the interrupt handler. Signed-off-by: Paolo Bonzini --- MdeModulePkg/Core/Dxe/Event/Tpl.c | 46 +++++++++++++++++++++---------- 1 file changed, 31 insertions(+), 15 deletions(-) diff --git a/MdeModulePkg/Core/Dxe/Event/Tpl.c b/MdeModulePkg/Core/Dxe/Event/Tpl.c index b33f80573c..fe95ea3896 100644 --- a/MdeModulePkg/Core/Dxe/Event/Tpl.c +++ b/MdeModulePkg/Core/Dxe/Event/Tpl.c @@ -90,10 +90,11 @@ CoreRaiseTpl ( @param NewTpl New, lower, task priority **/ -VOID +static VOID EFIAPI -CoreRestoreTpl ( - IN EFI_TPL NewTpl +CoreRestoreTplInternal ( + IN EFI_TPL NewTpl, + IN BOOLEAN DesiredInterruptState ) { EFI_TPL OldTpl; @@ -107,11 +108,6 @@ CoreRestoreTpl ( 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; } @@ -126,6 +122,11 @@ CoreRestoreTpl ( } gEfiCurrentTpl = PendingTpl; + + // + // If lowering below HIGH_LEVEL, make sure + // interrupts are enabled + // if (gEfiCurrentTpl < TPL_HIGH_LEVEL) { CoreSetInterruptState (TRUE); } @@ -134,16 +135,31 @@ CoreRestoreTpl ( } // - // Set the new value + // Set the new TPL with interrupt disabled. If DesiredInterruptState + // is FALSE, this ensures that any nested interrupt handler will only + // see an elevated TPL and not NewTpl. // - + CoreSetInterruptState (FALSE); gEfiCurrentTpl = NewTpl; - // - // If lowering below HIGH_LEVEL, make sure - // interrupts are enabled - // - if (gEfiCurrentTpl < TPL_HIGH_LEVEL) { + if (DesiredInterruptState) { + ASSERT(gEfiCurrentTpl < TPL_HIGH_LEVEL); CoreSetInterruptState (TRUE); } } + +/** + Lowers the task priority to the previous value. If the new + priority unmasks events at a higher priority, they are dispatched. + + @param NewTpl New, lower, task priority + +**/ +VOID +EFIAPI +CoreRestoreTpl ( + IN EFI_TPL NewTpl + ) +{ + CoreRestoreTplInternal (NewTpl, NewTpl < TPL_HIGH_LEVEL); +} -- 2.43.2 From 97b07f53f9a0711eb55e02e8af591eee09971668 Mon Sep 17 00:00:00 2001 From: Michael D Kinney Date: Fri, 1 Mar 2024 00:40:53 +0100 Subject: [PATCH 2/2] MdeModulePkg: fix stack overflow issue due to nested interrupts Content-Type: text/plain; charset=UTF-8 This is a heavily simplified version of the logic in the OvmfPkg NestedInterruptTplLib. Because the new CoreRestoreTplInternal() function allows to lower the TPL while interrupts are disabled, there is no need for the complex deferred execution mechanism. Instead, CoreRaiseTpl() uses the current state of the interrupt flag to second guess whether it's being called from an interrupt handler; and when restoring the outer TPL at the end of the handler, interrupts remain disabled until IRET and there cannot be nested invocation of the interrupt handler at the same TPL. Signed-off-by: Michael D Kinney Signed-off-by: Ray Ni [Rewritten the CoreRestoreTpl part. - Paolo] Signed-off-by: Paolo Bonzini --- MdeModulePkg/Core/Dxe/Event/Tpl.c | 73 ++++++++++++++++++++++++++++++- 1 file changed, 72 insertions(+), 1 deletion(-) diff --git a/MdeModulePkg/Core/Dxe/Event/Tpl.c b/MdeModulePkg/Core/Dxe/Event/Tpl.c index fe95ea3896..715b0626b8 100644 --- a/MdeModulePkg/Core/Dxe/Event/Tpl.c +++ b/MdeModulePkg/Core/Dxe/Event/Tpl.c @@ -9,6 +9,11 @@ SPDX-License-Identifier: BSD-2-Clause-Patent #include "DxeMain.h" #include "Event.h" +/// +/// Bit mask of TPLs that were interrupted during RestoreTPL(). +/// +static UINTN mInterruptedTplMask = 0; + /** Set Interrupt State. @@ -59,6 +64,7 @@ CoreRaiseTpl ( ) { EFI_TPL OldTpl; + BOOLEAN InterruptState; OldTpl = gEfiCurrentTpl; if (OldTpl > NewTpl) { @@ -72,7 +78,31 @@ CoreRaiseTpl ( // If raising to high level, disable interrupts // if ((NewTpl >= TPL_HIGH_LEVEL) && (OldTpl < TPL_HIGH_LEVEL)) { - CoreSetInterruptState (FALSE); + // + // When gCpu is NULL, InterruptState is TRUE. + // Calling CoreSetInterruptState() with TRUE is safe as CoreSetInterruptState() will directly return + // when gCpu is NULL. + // + InterruptState = TRUE; + if (gCpu != NULL) { + gCpu->GetInterruptState (gCpu, &InterruptState); + } + + if (InterruptState) { + // + // Interrupts are currently enabled. + // Disable them for going to HIGH level. + // + CoreSetInterruptState (FALSE); + } else { + // + // Within an interrupt handler. Save the TPL that was interrupted; + // It must be higher than the previously interrupted TPL, since + // CoreRestoreTpl unwinds all handlers up to the requested TPL. + // + ASSERT ((INTN)gEfiCurrentTpl > HighBitSet64 (mInterruptedTplMask)); + mInterruptedTplMask |= (UINTN)(1 << gEfiCurrentTpl); + } } // @@ -161,5 +191,46 @@ CoreRestoreTpl ( IN EFI_TPL NewTpl ) { + BOOLEAN InInterruptHandler = FALSE; + + // + // Unwind the nested interrupt handlers up to the required + // TPL, paying attention not to overflow the stack. While + // not strictly necessary according to the specification, + // accept the possibility that multiple RaiseTPL calls are + // undone by a single RestoreTPL + // + while ((INTN)NewTpl <= HighBitSet64 (mInterruptedTplMask)) { + UINTN InterruptedTpl = HighBitSet64 (mInterruptedTplMask); + mInterruptedTplMask &= ~(UINTN)(1 << InterruptedTpl); + + ASSERT (GetInterruptState () == FALSE); + InInterruptHandler = TRUE; + + // + // Take the TPL down a notch to allow event notifications to be + // dispatched. This will implicitly re-enable interrupts (if + // InterruptedTPL is below TPL_HIGH_LEVEL), even though we are + // still inside the interrupt handler, but the new TPL will + // be set while they are disabled. + // + // DesiredInterruptState must be FALSE to ensure that the + // stack does not blow up. If we used, as in the final call + // below, "InterruptedTpl < TPL_HIGH_LEVEL", the timer interrupt + // handler could be invoked repeatedly in the small window between + // CoreSetInterruptState (TRUE) and the IRET instruction. + // + CoreRestoreTplInternal (InterruptedTpl, FALSE); + + if (InterruptedTpl == NewTpl) { + break; + } + } + + // + // If we get here with InInterruptHandler == TRUE, an interrupt + // handler forgot to restore the TPL. + // + ASSERT (!InInterruptHandler); CoreRestoreTplInternal (NewTpl, NewTpl < TPL_HIGH_LEVEL); } -- 2.43.2