[PATCH 1/3] OvmfPkg: Send EOI before RestoreTPL() in timer interrupt handlers

[PATCH 1/3] OvmfPkg: Send EOI before RestoreTPL() in timer interrupt handlers

[Michael Brown]

Deferring the EOI until after the call to RestoreTPL() means that any
callbacks invoked by RestoreTPL() will run with timer interrupt
delivery disabled.  If any such callbacks themselves rely on timers to
implement timeout loops, then the callbacks will get stuck in an
infinite loop from which the system will never recover.

This reverts commit 239b50a86 ("OvmfPkg: End timer interrupt later to
avoid stack overflow under load").

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=4162
Signed-off-by: Michael Brown <mcb30@ipxe.org>
---
 OvmfPkg/8254TimerDxe/Timer.c                  | 5 ++---
 OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.c | 5 ++---
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/OvmfPkg/8254TimerDxe/Timer.c b/OvmfPkg/8254TimerDxe/Timer.c
index e49a438b44..7feaaaba2b 100644
--- a/OvmfPkg/8254TimerDxe/Timer.c
+++ b/OvmfPkg/8254TimerDxe/Timer.c
@@ -80,6 +80,8 @@ TimerInterruptHandler (
 
   OriginalTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL);
 
+  mLegacy8259->EndOfInterrupt (mLegacy8259, Efi8259Irq0);
+
   if (mTimerNotifyFunction != NULL) {
     //
     // @bug : This does not handle missed timer interrupts
@@ -88,9 +90,6 @@ TimerInterruptHandler (
   }
 
   gBS->RestoreTPL (OriginalTPL);
-
-  DisableInterrupts ();
-  mLegacy8259->EndOfInterrupt (mLegacy8259, Efi8259Irq0);
 }
 
 /**
diff --git a/OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.c b/OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.c
index cbc17c979c..3e04b49d6b 100644
--- a/OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.c
+++ b/OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.c
@@ -62,6 +62,8 @@ TimerInterruptHandler (
 
   OriginalTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL);
 
+  SendApicEoi ();
+
   if (mTimerNotifyFunction != NULL) {
     //
     // @bug : This does not handle missed timer interrupts
@@ -70,9 +72,6 @@ TimerInterruptHandler (
   }
 
   gBS->RestoreTPL (OriginalTPL);
-
-  DisableInterrupts ();
-  SendApicEoi ();
 }
 
 /**
-- 
2.38.1

[PATCH 2/3] OvmfPkg: Add library to handle TPL from within nested interrupt handlers

[Michael Brown]

UEFI requires us to support nested interrupts, but provides no way for
an interrupt handler to call RestoreTPL() without implicitly
re-enabling interrupts.  In a virtual machine, it is possible for a
large burst of interrupts to arrive.  We must prevent such a burst
from leading to stack underrun, while continuing to allow nested
interrupts to occur.

This can be achieved by allowing, when provably safe to do so, an
inner interrupt handler to return from the interrupt without restoring
the TPL and with interrupts remaining disabled after IRET, with the
deferred call to RestoreTPL() then being issued from the outer
interrupt handler.  This is necessarily messy and involves direct
manipulation of the interrupt stack frame, and so should not be
implemented as open-coded logic within each interrupt handler.

Add the Nested Interrupt TPL Library (NestedInterruptTplLib) to
provide helper functions that can be used by nested interrupt handlers
in place of RaiseTPL()/RestoreTPL().

Example call tree for a timer interrupt occurring at TPL_APPLICATION
with a nested timer interrupt that makes its own call to RestoreTPL():

  outer TimerInterruptHandler()
    InterruptedTPL == TPL_APPLICATION
    ...
    IsrState->InProgressRestoreTPL = TPL_APPLICATION;
    gBS->RestoreTPL (TPL_APPLICATION);
      EnableInterrupts();
      dispatch a TPL_CALLBACK event
        gEfiCurrentTpl = TPL_CALLBACK;
        nested timer interrupt occurs
        inner TimerInterruptHandler()
          InterruptedTPL == TPL_CALLBACK
          ...
          IsrState->InProgressRestoreTPL = TPL_CALLBACK;
          gBS->RestoreTPL (TPL_CALLBACK);
            EnableInterrupts();
          DisableInterrupts();
          IsrState->InProgressRestoreTPL = TPL_APPLICATION;
          IRET re-enables interrupts
      ... finish dispatching TPL_CALLBACK events ...
      gEfiCurrentTpl = TPL_APPLICATION;
    DisableInterrupts();
    IsrState->InProgressRestoreTPL = 0;
    sees IsrState->DeferredRestoreTPL == FALSE and returns
    IRET re-enables interrupts

Example call tree for a timer interrupt occurring at TPL_APPLICATION
with a nested timer interrupt that defers its call to RestoreTPL() to
the outer instance of the interrupt handler:

  outer TimerInterruptHandler()
    InterruptedTPL == TPL_APPLICATION
    ...
    IsrState->InProgressRestoreTPL = TPL_APPLICATION;
    gBS->RestoreTPL (TPL_APPLICATION);
      EnableInterrupts();
      dispatch a TPL_CALLBACK event
      ... finish dispatching TPL_CALLBACK events ...
      gEfiCurrentTpl = TPL_APPLICATION;
      nested timer interrupt occurs
      inner TimerInterruptHandler()
        InterruptedTPL == TPL_APPLICATION;
        ...
        sees InterruptedTPL == IsrState->InProgressRestoreTPL
        IsrState->DeferredRestoreTPL = TRUE;
        DisableInterruptsOnIret();
        IRET returns without re-enabling interrupts
    DisableInterrupts();
    IsrState->InProgressRestoreTPL = 0;
    sees IsrState->DeferredRestoreTPL == TRUE and loops
    IsrState->InProgressRestoreTPL = TPL_APPLICATION;
    gBS->RestoreTPL (TPL_APPLICATION);  <-- deferred call
      EnableInterrupts();
    DisableInterrupts();
    IsrState->InProgressRestoreTPL = 0;
    sees IsrState->DeferredRestoreTPL == FALSE and returns
    IRET re-enables interrupts

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=4162
Signed-off-by: Michael Brown <mcb30@ipxe.org>
---
 .../Include/Library/NestedInterruptTplLib.h   |  87 +++++++
 OvmfPkg/Library/NestedInterruptTplLib/Iret.c  |  62 +++++
 OvmfPkg/Library/NestedInterruptTplLib/Iret.h  |  19 ++
 .../NestedInterruptTplLib.inf                 |  35 +++
 OvmfPkg/Library/NestedInterruptTplLib/Tpl.c   | 216 ++++++++++++++++++
 OvmfPkg/OvmfPkg.dec                           |   4 +
 6 files changed, 423 insertions(+)
 create mode 100644 OvmfPkg/Include/Library/NestedInterruptTplLib.h
 create mode 100644 OvmfPkg/Library/NestedInterruptTplLib/Iret.c
 create mode 100644 OvmfPkg/Library/NestedInterruptTplLib/Iret.h
 create mode 100644 OvmfPkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
 create mode 100644 OvmfPkg/Library/NestedInterruptTplLib/Tpl.c

diff --git a/OvmfPkg/Include/Library/NestedInterruptTplLib.h b/OvmfPkg/Include/Library/NestedInterruptTplLib.h
new file mode 100644
index 0000000000..0ead6e4b34
--- /dev/null
+++ b/OvmfPkg/Include/Library/NestedInterruptTplLib.h
@@ -0,0 +1,87 @@
+/** @file
+  Handle raising and lowering TPL from within nested interrupt handlers.
+
+  Allows interrupt handlers to safely raise and lower the TPL to
+  dispatch event notifications, correctly allowing for nested
+  interrupts to occur without risking stack exhaustion.
+
+  Copyright (C) 2022, Fen Systems Ltd.
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#ifndef __NESTED_INTERRUPT_TPL_LIB__
+#define __NESTED_INTERRUPT_TPL_LIB__
+
+#include <Uefi/UefiBaseType.h>
+#include <Uefi/UefiSpec.h>
+#include <Protocol/DebugSupport.h>
+
+///
+/// State shared between all invocations of a nested interrupt handler.
+///
+typedef struct {
+  ///
+  /// Highest TPL that is currently the target of a call to
+  /// RestoreTPL() by an instance of this interrupt handler.
+  ///
+  EFI_TPL    InProgressRestoreTPL;
+  ///
+  /// Flag used to defer a call to RestoreTPL() from an inner instance
+  /// of the interrupt handler to an outer instance of the same
+  /// interrupt handler.
+  ///
+  BOOLEAN    DeferredRestoreTPL;
+} NESTED_INTERRUPT_STATE;
+
+/**
+  Raise the task priority level to TPL_HIGH_LEVEL.
+
+  @param  None.
+
+  @return The task priority level at which the interrupt occurred.
+**/
+EFI_TPL
+EFIAPI
+NestedInterruptRaiseTPL (
+  VOID
+  );
+
+/**
+  Lower the task priority back to the value at which the interrupt
+  occurred.
+
+  This is unfortunately messy.  UEFI requires us to support nested
+  interrupts, but provides no way for an interrupt handler to call
+  RestoreTPL() without implicitly re-enabling interrupts.  In a
+  virtual machine, it is possible for a large burst of interrupts to
+  arrive.  We must prevent such a burst from leading to stack
+  exhaustion, while continuing to allow nested interrupts to occur.
+
+  Since nested interrupts are permitted, an interrupt handler may be
+  invoked as an inner interrupt handler while an outer instance of the
+  same interrupt handler is still inside its call to RestoreTPL().
+
+  To avoid stack exhaustion, this call may therefore (when provably
+  safe to do so) defer the actual TPL lowering to be performed by an
+  outer instance of the same interrupt handler.
+
+  @param InterruptedTPL        The task priority level at which the interrupt
+                               occurred, as previously returned from
+                               NestedInterruptRaiseTPL().
+
+  @param SystemContext         A pointer to the system context when the
+                               interrupt occurred.
+
+  @param IsrState              A pointer to the state shared between all
+                               invocations of the nested interrupt handler.
+**/
+VOID
+EFIAPI
+NestedInterruptRestoreTPL (
+  IN EFI_TPL                     InterruptedTPL,
+  IN OUT EFI_SYSTEM_CONTEXT      SystemContext,
+  IN OUT NESTED_INTERRUPT_STATE  *IsrState
+  );
+
+#endif // __NESTED_INTERRUPT_TPL_LIB__
diff --git a/OvmfPkg/Library/NestedInterruptTplLib/Iret.c b/OvmfPkg/Library/NestedInterruptTplLib/Iret.c
new file mode 100644
index 0000000000..f6b2c51b6c
--- /dev/null
+++ b/OvmfPkg/Library/NestedInterruptTplLib/Iret.c
@@ -0,0 +1,62 @@
+/** @file
+  Force interrupt handler to return with interrupts still disabled.
+
+  Copyright (C) 2022, Fen Systems Ltd.
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include <Library/BaseLib.h>
+#include <Library/DebugLib.h>
+
+#include "Iret.h"
+
+/**
+  Force interrupt handler to return with interrupts still disabled.
+
+  @param SystemContext         A pointer to the system context when the
+                               interrupt occurred.
+**/
+VOID
+DisableInterruptsOnIret (
+  IN OUT EFI_SYSTEM_CONTEXT  SystemContext
+  )
+{
+ #if defined (MDE_CPU_X64)
+
+  IA32_EFLAGS32  Rflags;
+
+  //
+  // Get flags from system context.
+  //
+  Rflags.UintN = SystemContext.SystemContextX64->Rflags;
+  ASSERT (Rflags.Bits.IF);
+
+  //
+  // Clear interrupts-enabled flag.
+  //
+  Rflags.Bits.IF                         = 0;
+  SystemContext.SystemContextX64->Rflags = Rflags.UintN;
+
+ #elif defined (MDE_CPU_IA32)
+
+  IA32_EFLAGS32  Eflags;
+
+  //
+  // Get flags from system context.
+  //
+  Eflags.UintN = SystemContext.SystemContextIa32->Eflags;
+  ASSERT (Eflags.Bits.IF);
+
+  //
+  // Clear interrupts-enabled flag.
+  //
+  Eflags.Bits.IF                          = 0;
+  SystemContext.SystemContextIa32->Eflags = Eflags.UintN;
+
+ #else
+
+  #error "Unsupported CPU"
+
+ #endif
+}
diff --git a/OvmfPkg/Library/NestedInterruptTplLib/Iret.h b/OvmfPkg/Library/NestedInterruptTplLib/Iret.h
new file mode 100644
index 0000000000..278c1e22b3
--- /dev/null
+++ b/OvmfPkg/Library/NestedInterruptTplLib/Iret.h
@@ -0,0 +1,19 @@
+/** @file
+  Force interrupt handler to return with interrupts still disabled.
+
+  Copyright (C) 2022, Fen Systems Ltd.
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#ifndef _IRET_H_
+#define _IRET_H_
+
+#include <Protocol/DebugSupport.h>
+
+VOID
+DisableInterruptsOnIret (
+  IN OUT EFI_SYSTEM_CONTEXT  SystemContext
+  );
+
+#endif // _IRET_H_
diff --git a/OvmfPkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf b/OvmfPkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
new file mode 100644
index 0000000000..5eafb41978
--- /dev/null
+++ b/OvmfPkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
@@ -0,0 +1,35 @@
+## @file
+# Handle raising and lowering TPL from within nested interrupt handlers.
+#
+# Allows interrupt handlers to safely raise and lower the TPL to
+# dispatch event notifications, correctly allowing for nested
+# interrupts to occur without risking stack exhaustion.
+#
+# Copyright (C) 2022, Fen Systems Ltd.
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+##
+
+[Defines]
+  INF_VERSION    = 1.29
+  BASE_NAME      = NestedInterruptTplLib
+  FILE_GUID      = 8df39823-2f9e-4ef2-b971-243b44c32c67
+  MODULE_TYPE    = DXE_DRIVER
+  VERSION_STRING = 1.0
+  LIBRARY_CLASS  = NestedInterruptTplLib|DXE_DRIVER
+
+[Sources]
+  Tpl.c
+  Iret.c
+
+[Packages]
+  MdePkg/MdePkg.dec
+  OvmfPkg/OvmfPkg.dec
+
+[LibraryClasses]
+  BaseLib
+  DebugLib
+  UefiBootServicesTableLib
+
+[Depex.common.DXE_DRIVER]
+  TRUE
diff --git a/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c b/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c
new file mode 100644
index 0000000000..e19d98878e
--- /dev/null
+++ b/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c
@@ -0,0 +1,216 @@
+/** @file
+  Handle raising and lowering TPL from within nested interrupt handlers.
+
+  Allows interrupt handlers to safely raise and lower the TPL to
+  dispatch event notifications, correctly allowing for nested
+  interrupts to occur without risking stack exhaustion.
+
+  Copyright (C) 2022, Fen Systems Ltd.
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include <Library/BaseLib.h>
+#include <Library/DebugLib.h>
+#include <Library/NestedInterruptTplLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+
+#include "Iret.h"
+
+/**
+  Raise the task priority level to TPL_HIGH_LEVEL.
+
+  @param  None.
+
+  @return The task priority level at which the interrupt occurred.
+**/
+EFI_TPL
+EFIAPI
+NestedInterruptRaiseTPL (
+  VOID
+  )
+{
+  EFI_TPL  InterruptedTPL;
+
+  //
+  // Raise TPL and assert that we were called from within an interrupt
+  // handler (i.e. with TPL below TPL_HIGH_LEVEL but with interrupts
+  // disabled).
+  //
+  ASSERT (GetInterruptState () == FALSE);
+  InterruptedTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL);
+  ASSERT (InterruptedTPL < TPL_HIGH_LEVEL);
+
+  return InterruptedTPL;
+}
+
+/**
+  Lower the task priority back to the value at which the interrupt
+  occurred.
+
+  This is unfortunately messy.  UEFI requires us to support nested
+  interrupts, but provides no way for an interrupt handler to call
+  RestoreTPL() without implicitly re-enabling interrupts.  In a
+  virtual machine, it is possible for a large burst of interrupts to
+  arrive.  We must prevent such a burst from leading to stack
+  exhaustion, while continuing to allow nested interrupts to occur.
+
+  Since nested interrupts are permitted, an interrupt handler may be
+  invoked as an inner interrupt handler while an outer instance of the
+  same interrupt handler is still inside its call to RestoreTPL().
+
+  To avoid stack exhaustion, this call may therefore (when provably
+  safe to do so) defer the actual TPL lowering to be performed by an
+  outer instance of the same interrupt handler.
+
+  @param InterruptedTPL        The task priority level at which the interrupt
+                               occurred, as previously returned from
+                               NestedInterruptRaiseTPL().
+
+  @param SystemContext         A pointer to the system context when the
+                               interrupt occurred.
+
+  @param IsrState              A pointer to the state shared between all
+                               invocations of the nested interrupt handler.
+**/
+VOID
+EFIAPI
+NestedInterruptRestoreTPL (
+  IN EFI_TPL                     InterruptedTPL,
+  IN OUT EFI_SYSTEM_CONTEXT      SystemContext,
+  IN OUT NESTED_INTERRUPT_STATE  *IsrState
+  )
+{
+  EFI_TPL  SavedInProgressRestoreTPL;
+  BOOLEAN  DeferredRestoreTPL;
+
+  //
+  // If the TPL at which this interrupt occurred is equal to that of
+  // the in-progress RestoreTPL() for an outer instance of the same
+  // interrupt handler, then that outer handler's call to RestoreTPL()
+  // must have finished dispatching all event notifications.  This
+  // interrupt must therefore have occurred at the point that the
+  // outer handler's call to RestoreTPL() had finished and was about
+  // to return to the outer handler.
+  //
+  // If we were to call RestoreTPL() at this point, then we would open
+  // up the possibility for unlimited stack consumption in the event
+  // of an interrupt storm.  We therefore cannot safely call
+  // RestoreTPL() from within this stack frame (i.e. from within this
+  // instance of the interrupt handler).
+  //
+  // Instead, we arrange to return from this interrupt with the TPL
+  // still at TPL_HIGH_LEVEL and with interrupts disabled, and to
+  // defer our call to RestoreTPL() to the in-progress outer instance
+  // of the same interrupt handler.
+  //
+  if (InterruptedTPL == IsrState->InProgressRestoreTPL) {
+    //
+    // Trigger outer instance of this interrupt handler to perform the
+    // RestoreTPL() call that we cannot issue at this point without
+    // risking stack exhaustion.
+    //
+    ASSERT (IsrState->DeferredRestoreTPL == FALSE);
+    IsrState->DeferredRestoreTPL = TRUE;
+
+    //
+    // DEFERRAL INVOCATION POINT
+    //
+    // Return from this interrupt handler with interrupts still
+    // disabled (by clearing the "interrupts-enabled" bit in the CPU
+    // flags that will be restored by the IRET or equivalent
+    // instruction).
+    //
+    // This ensures that no further interrupts may occur before
+    // control reaches the outer interrupt handler's RestoreTPL() loop
+    // at the point marked "DEFERRAL RETURN POINT" (see below).
+    //
+    DisableInterruptsOnIret (SystemContext);
+    return;
+  }
+
+  //
+  // If the TPL at which this interrupt occurred is higher than that
+  // of the in-progress RestoreTPL() for an outer instance of the same
+  // interrupt handler, then that outer handler's call to RestoreTPL()
+  // must still be dispatching event notifications.
+  //
+  // We must therefore call RestoreTPL() at this point to allow more
+  // event notifications to be dispatched, since those event
+  // notification callback functions may themselves be waiting upon
+  // other events.
+  //
+  // We cannot avoid creating a new stack frame for this call to
+  // RestoreTPL(), but the total number of such stack frames is
+  // intrinsically limited by the number of distinct TPLs.
+  //
+  // We may need to issue the call to RestoreTPL() more than once, if
+  // an inner instance of the same interrupt handler needs to defer
+  // its RestoreTPL() call to be performed from within this stack
+  // frame (see above).
+  //
+  while (TRUE) {
+    //
+    // Check shared state loop invariants.
+    //
+    ASSERT (IsrState->InProgressRestoreTPL < InterruptedTPL);
+    ASSERT (IsrState->DeferredRestoreTPL == FALSE);
+
+    //
+    // Record the in-progress RestoreTPL() value in the shared state
+    // where it will be visible to an inner instance of the same
+    // interrupt handler, in case a nested interrupt occurs during our
+    // call to RestoreTPL().
+    //
+    SavedInProgressRestoreTPL      = IsrState->InProgressRestoreTPL;
+    IsrState->InProgressRestoreTPL = InterruptedTPL;
+
+    //
+    // Call RestoreTPL() to allow event notifications to be
+    // dispatched.  This will implicitly re-enable interrupts.
+    //
+    gBS->RestoreTPL (InterruptedTPL);
+
+    //
+    // Re-disable interrupts after the call to RestoreTPL() to ensure
+    // that we have exclusive access to the shared state.
+    //
+    DisableInterrupts ();
+
+    //
+    // DEFERRAL RETURN POINT
+    //
+    // An inner instance of the same interrupt handler may have chosen
+    // to defer its RestoreTPL() call to be performed from within this
+    // stack frame.  If so, it is guaranteed that no further event
+    // notifications or interrupts have been processed between the
+    // DEFERRAL INVOCATION POINT (see above) and this DEFERRAL RETURN
+    // POINT.
+    //
+
+    //
+    // Restore the locally saved in-progress RestoreTPL() value in the
+    // shared state, now that our call to RestoreTPL() has returned
+    // and is therefore no longer in progress.
+    //
+    ASSERT (IsrState->InProgressRestoreTPL == InterruptedTPL);
+    IsrState->InProgressRestoreTPL = SavedInProgressRestoreTPL;
+
+    //
+    // Check (and clear) the shared state to see if an inner instance
+    // of the same interrupt handler deferred its call to
+    // RestoreTPL().
+    //
+    DeferredRestoreTPL           = IsrState->DeferredRestoreTPL;
+    IsrState->DeferredRestoreTPL = FALSE;
+
+    //
+    // If no inner interrupt handler deferred its call to
+    // RestoreTPL(), then the TPL has been successfully restored and
+    // we may return from the interrupt handler.
+    //
+    if (DeferredRestoreTPL == FALSE) {
+      return;
+    }
+  }
+}
diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 5f5556c67c..b2e0b7dfe2 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -38,6 +38,10 @@
   #
   MemEncryptTdxLib|Include/Library/MemEncryptTdxLib.h
 
+  ##  @libraryclass  Handle TPL changes within nested interrupt handlers
+  #
+  NestedInterruptTplLib|Include/Library/NestedInterruptTplLib.h
+
   ##  @libraryclass  Save and restore variables using a file
   #
   NvVarsFileLib|Include/Library/NvVarsFileLib.h
-- 
2.38.1

[PATCH 3/3] OvmfPkg: Use NestedInterruptTplLib in nested interrupt handlers

[Michael Brown]

Prevent stack underrun in the event of a timer interrupt storm in
LocalApicTimerDxe and 8254TimerDxe interrupt handlers by using the
helper functions provided by NestedInterruptTplLib.

This fixes the same problem as addressed in commit 239b50a86
("OvmfPkg: End timer interrupt later to avoid stack overflow under
load"), but does so without breaking nested timer interrupts.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2815
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=4162
Signed-off-by: Michael Brown <mcb30@ipxe.org>
---
 OvmfPkg/8254TimerDxe/8254Timer.inf              | 1 +
 OvmfPkg/8254TimerDxe/Timer.c                    | 9 ++++++---
 OvmfPkg/AmdSev/AmdSevX64.dsc                    | 1 +
 OvmfPkg/CloudHv/CloudHvX64.dsc                  | 1 +
 OvmfPkg/IntelTdx/IntelTdxX64.dsc                | 1 +
 OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.c   | 9 ++++++---
 OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf | 1 +
 OvmfPkg/Microvm/MicrovmX64.dsc                  | 1 +
 OvmfPkg/OvmfPkgIa32.dsc                         | 1 +
 OvmfPkg/OvmfPkgIa32X64.dsc                      | 1 +
 OvmfPkg/OvmfPkgX64.dsc                          | 1 +
 OvmfPkg/OvmfXen.dsc                             | 1 +
 12 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/OvmfPkg/8254TimerDxe/8254Timer.inf b/OvmfPkg/8254TimerDxe/8254Timer.inf
index 8a07c8247e..8fbab896f0 100644
--- a/OvmfPkg/8254TimerDxe/8254Timer.inf
+++ b/OvmfPkg/8254TimerDxe/8254Timer.inf
@@ -24,6 +24,7 @@
   UefiBootServicesTableLib
   BaseLib
   DebugLib
+  NestedInterruptTplLib
   UefiDriverEntryPoint
   IoLib
 
diff --git a/OvmfPkg/8254TimerDxe/Timer.c b/OvmfPkg/8254TimerDxe/Timer.c
index 7feaaaba2b..5382b01615 100644
--- a/OvmfPkg/8254TimerDxe/Timer.c
+++ b/OvmfPkg/8254TimerDxe/Timer.c
@@ -6,6 +6,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
 
+#include <Library/NestedInterruptTplLib.h>
+
 #include "Timer.h"
 
 //
@@ -76,9 +78,10 @@ TimerInterruptHandler (
   IN EFI_SYSTEM_CONTEXT  SystemContext
   )
 {
-  EFI_TPL  OriginalTPL;
+  STATIC NESTED_INTERRUPT_STATE  NestedInterruptState;
+  EFI_TPL                        OriginalTPL;
 
-  OriginalTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL);
+  OriginalTPL = NestedInterruptRaiseTPL ();
 
   mLegacy8259->EndOfInterrupt (mLegacy8259, Efi8259Irq0);
 
@@ -89,7 +92,7 @@ TimerInterruptHandler (
     mTimerNotifyFunction (mTimerPeriod);
   }
 
-  gBS->RestoreTPL (OriginalTPL);
+  NestedInterruptRestoreTPL (OriginalTPL, SystemContext, &NestedInterruptState);
 }
 
 /**
diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
index 8f7cae787e..3433725697 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.dsc
+++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
@@ -353,6 +353,7 @@
 !endif
   PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
   MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
+  NestedInterruptTplLib|OvmfPkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
   QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
   QemuLoadImageLib|OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
 
diff --git a/OvmfPkg/CloudHv/CloudHvX64.dsc b/OvmfPkg/CloudHv/CloudHvX64.dsc
index ce277cb239..6d5a702b05 100644
--- a/OvmfPkg/CloudHv/CloudHvX64.dsc
+++ b/OvmfPkg/CloudHv/CloudHvX64.dsc
@@ -404,6 +404,7 @@
 !endif
   PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
   MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
+  NestedInterruptTplLib|OvmfPkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
   QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
   QemuLoadImageLib|OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf
 
diff --git a/OvmfPkg/IntelTdx/IntelTdxX64.dsc b/OvmfPkg/IntelTdx/IntelTdxX64.dsc
index 3458926515..994d09c751 100644
--- a/OvmfPkg/IntelTdx/IntelTdxX64.dsc
+++ b/OvmfPkg/IntelTdx/IntelTdxX64.dsc
@@ -314,6 +314,7 @@
   LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf
   PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
   MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
+  NestedInterruptTplLib|OvmfPkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
   QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
   QemuLoadImageLib|OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf
 
diff --git a/OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.c b/OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.c
index 3e04b49d6b..67f4dcde37 100644
--- a/OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.c
+++ b/OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.c
@@ -8,6 +8,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
 
+#include <Library/NestedInterruptTplLib.h>
+
 #include "LocalApicTimerDxe.h"
 
 //
@@ -58,9 +60,10 @@ TimerInterruptHandler (
   IN EFI_SYSTEM_CONTEXT  SystemContext
   )
 {
-  EFI_TPL  OriginalTPL;
+  STATIC NESTED_INTERRUPT_STATE  NestedInterruptState;
+  EFI_TPL                        OriginalTPL;
 
-  OriginalTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL);
+  OriginalTPL = NestedInterruptRaiseTPL ();
 
   SendApicEoi ();
 
@@ -71,7 +74,7 @@ TimerInterruptHandler (
     mTimerNotifyFunction (mTimerPeriod);
   }
 
-  gBS->RestoreTPL (OriginalTPL);
+  NestedInterruptRestoreTPL (OriginalTPL, SystemContext, &NestedInterruptState);
 }
 
 /**
diff --git a/OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf b/OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
index 3ad28a148c..b85965c75e 100644
--- a/OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
+++ b/OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
@@ -27,6 +27,7 @@
   UefiBootServicesTableLib
   BaseLib
   DebugLib
+  NestedInterruptTplLib
   UefiDriverEntryPoint
   LocalApicLib
 
diff --git a/OvmfPkg/Microvm/MicrovmX64.dsc b/OvmfPkg/Microvm/MicrovmX64.dsc
index 994a02d301..db449384c9 100644
--- a/OvmfPkg/Microvm/MicrovmX64.dsc
+++ b/OvmfPkg/Microvm/MicrovmX64.dsc
@@ -403,6 +403,7 @@
   PciPcdProducerLib|OvmfPkg/Fdt/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf
   PciExpressLib|OvmfPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf
   MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
+  NestedInterruptTplLib|OvmfPkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
   QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
   QemuLoadImageLib|OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf
 
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 6f774baf90..15ce1261ec 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -409,6 +409,7 @@
 !endif
   PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
   MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
+  NestedInterruptTplLib|OvmfPkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
   QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
   QemuLoadImageLib|OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf
 
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index c851764dec..da8d182b54 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -415,6 +415,7 @@
 !endif
   PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
   MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
+  NestedInterruptTplLib|OvmfPkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
   QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
   QemuLoadImageLib|OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf
 
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 63c3a47aea..b2d24a798b 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -435,6 +435,7 @@
 !endif
   PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
   MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
+  NestedInterruptTplLib|OvmfPkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
   QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
   QemuLoadImageLib|OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf
 
diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
index 8bb497088b..c328987e84 100644
--- a/OvmfPkg/OvmfXen.dsc
+++ b/OvmfPkg/OvmfXen.dsc
@@ -340,6 +340,7 @@
 !endif
   PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
   MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
+  NestedInterruptTplLib|OvmfPkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
   QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
 
 [LibraryClasses.common.UEFI_APPLICATION]
-- 
2.38.1

Re: [edk2-devel] [PATCH 2/3] OvmfPkg: Add library to handle TPL from within nested interrupt handlers

[Gerd Hoffmann]

  Hi,

[ Cc'ing ard ]

> Add the Nested Interrupt TPL Library (NestedInterruptTplLib) to
> provide helper functions that can be used by nested interrupt handlers
> in place of RaiseTPL()/RestoreTPL().

So, in a nutshell this makes inner interrupt handler only run callbacks
with a priority higher than the priority of the outer interrupt handler.
Anything with equal or lower priority is deferred to the outer handler.
That way we never nest at the same priority, which in turn limits
nesting to the number of different priorities in use.

Correct?

The idea looks sane to me and I couldn't spot any logic errors in this.

take care,
  Gerd

Re: [edk2-devel] [PATCH 2/3] OvmfPkg: Add library to handle TPL from within nested interrupt handlers

[Michael Brown]

On 09/12/2022 15:02, Gerd Hoffmann wrote:
>> Add the Nested Interrupt TPL Library (NestedInterruptTplLib) to
>> provide helper functions that can be used by nested interrupt handlers
>> in place of RaiseTPL()/RestoreTPL().
> 
> So, in a nutshell this makes inner interrupt handler only run callbacks
> with a priority higher than the priority of the outer interrupt handler.
> Anything with equal or lower priority is deferred to the outer handler.

Essentially, yes, though for accuracy I would replace "run callbacks 
with" with "restore TPL to":

   The inner interrupt handler will only restore TPL to a priority
   higher than the priority of the outer interrupt handler. Anything
   with equal or lower priority is deferred to the outer handler.

The distinction matters because in the deferral case the inner interrupt 
will not run any callbacks at all: the running of callbacks is then 
deferred in its entirety to the outer interrupt handler (with a hard 
guarantee that the deferred running of callbacks will take place 
immediately).

> That way we never nest at the same priority, which in turn limits
> nesting to the number of different priorities in use.
> 
> Correct?

Yes, limiting nesting to the number of different priorities in use is 
exactly what this patch does.

> The idea looks sane to me and I couldn't spot any logic errors in this.

Great! :)

Thanks,

Michael

Re: [edk2-devel] [PATCH 2/3] OvmfPkg: Add library to handle TPL from within nested interrupt handlers

[Michael Brown]

On 09/12/2022 15:22, Michael Brown wrote:
> On 09/12/2022 15:02, Gerd Hoffmann wrote:
>>> Add the Nested Interrupt TPL Library (NestedInterruptTplLib) to 
>>> provide helper functions that can be used by nested interrupt
>>> handlers in place of RaiseTPL()/RestoreTPL().
>> 
>> <snip>
> 
>> The idea looks sane to me and I couldn't spot any logic errors in
>> this.
> 
> Great! :)

Ping.  Is there anything else that needs to be done before this gets 
merged?  It would be good to have the regression bug fixed.

Thanks,

Michael

Re: [edk2-devel] [PATCH 2/3] OvmfPkg: Add library to handle TPL from within nested interrupt handlers

[Laszlo Ersek]

On 12/20/22 16:02, Michael Brown wrote:
> On 09/12/2022 15:22, Michael Brown wrote:
>> On 09/12/2022 15:02, Gerd Hoffmann wrote:
>>>> Add the Nested Interrupt TPL Library (NestedInterruptTplLib) to
>>>> provide helper functions that can be used by nested interrupt
>>>> handlers in place of RaiseTPL()/RestoreTPL().
>>>
>>> <snip>
>>
>>> The idea looks sane to me and I couldn't spot any logic errors in
>>> this.
>>
>> Great! :)
> 
> Ping.  Is there anything else that needs to be done before this gets
> merged?  It would be good to have the regression bug fixed.
> 
> Thanks,
> 
> Michael
> 

series
Acked-by: Laszlo Ersek <lersek@redhat.com>

Ard, can you please help merge this?

Thanks!
Laszlo

Re: [edk2-devel] [PATCH 2/3] OvmfPkg: Add library to handle TPL from within nested interrupt handlers

[Ard Biesheuvel]

On Wed, 21 Dec 2022 at 09:07, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 12/20/22 16:02, Michael Brown wrote:
> > On 09/12/2022 15:22, Michael Brown wrote:
> >> On 09/12/2022 15:02, Gerd Hoffmann wrote:
> >>>> Add the Nested Interrupt TPL Library (NestedInterruptTplLib) to
> >>>> provide helper functions that can be used by nested interrupt
> >>>> handlers in place of RaiseTPL()/RestoreTPL().
> >>>
> >>> <snip>
> >>
> >>> The idea looks sane to me and I couldn't spot any logic errors in
> >>> this.
> >>
> >> Great! :)
> >
> > Ping.  Is there anything else that needs to be done before this gets
> > merged?  It would be good to have the regression bug fixed.
> >
> > Thanks,
> >
> > Michael
> >
>
> series
> Acked-by: Laszlo Ersek <lersek@redhat.com>
>
> Ard, can you please help merge this?
>

Sure.

For the ARM case, it seems the situation is slightly different, given
that we reprogram the timer for the next interrupt in
TimerInterruptHandler (), so I don't think we can accumulate pending
interrupts in the same way as x86.

So i'm fine with making this fix specific to OVMF/x86.

Queued as #3829