Re: [edk2-devel] [PATCH 0/3] OvmfPkg: Fix handling of nested interrupts

["Ard Biesheuvel" <ardb@kernel.org>]

(cc Gerd)

On Fri, 9 Dec 2022 at 11:20, Michael Brown <mcb30@ipxe.org> wrote:
>
> Commit 239b50a86 ("OvmfPkg: End timer interrupt later to avoid stack
> overflow under load") deferred the call to SendApicEoi() until after
> the call to RestoreTPL(), in order to avoid the potential stack
> underflow as described in bug 2815.
>
> This has the unwanted side effect that any callbacks invoked as a
> result of RestoreTPL() will now run before the EOI is sent to the
> APIC.  Until the callbacks return, no further timer interrupts will be
> raised and the system time value in mEfiSystemTime will remain
> unchanging.
>
> If any callbacks invoked by RestoreTPL() include a timeout loop based
> on mEfiSystemTime (e.g. via SetTimer() and CheckEvent()), this loop
> will never time out and the system will appear to hang.
>
> This situation can be reproduced by e.g. attaching a USB network card
> connected via an xHCI USB controller.  The "system poll timer"
> callback to MnpSystemPoll() will eventually cause the following
> sequence of events:
>
> - Hardware timer interrupt occurs
> - TimerInterruptHandler() in LocalApicTimerDxe is called
> - TimerInterruptHandler() calls RaiseTPL(TPL_HIGH_LEVEL)
> - TimerInterruptHandler() calls RestoreTPL() before sending EOI
> - RestoreTPL() triggers a call to MnpSystemPoll()
> - MnpSystemPoll() ends up calling through to XhcExecTransfer() to poll
>   the USB device
> - XhcExecTransfer() enters a loop waiting for the transfer to complete
>   or time out
> - XhcExecTransfer() uses CheckEvent() to check for the timeout
>   condition
> - Since EOI has not been sent, no further timer interrupts will occur
>   and so CheckEvent() always returns EFI_NOT_READY and
>   XhcExecTransfer() waits indefinitely in its timeout loop
> - User sees a totally stuck system that is unresponsive to any input
>   other than a hard reset
>
> This patch series reverts commit 239b50a86 ("OvmfPkg: End timer
> interrupt later to avoid stack overflow under load") and introduces
> instead a new abstraction NestedInterruptTplLib to handle the
> intricacies required to safely allow nested interrupts without
> changing the core UEFI abstractions for RaiseTPL() and RestoreTPL().
>
> The NestedInterruptTplLib could in principle be used by all timer
> interrupt handlers including those used on physical hardware
> platforms.  Doing so would allow those platforms to provide a
> guarantee against interrupt-induced stack underflow.
>
> To simplify the EDK2 review process, the current patch series modifies
> only platforms within OvmfPkg, since those are the only platforms
> currently suffering from the regression.
>
> 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
>
> Michael Brown (3):
>   OvmfPkg: Send EOI before RestoreTPL() in timer interrupt handlers
>   OvmfPkg: Add library to handle TPL from within nested interrupt
>     handlers
>   OvmfPkg: Use NestedInterruptTplLib in nested interrupt handlers
>
>  OvmfPkg/8254TimerDxe/8254Timer.inf            |   1 +
>  OvmfPkg/8254TimerDxe/Timer.c                  |  14 +-
>  OvmfPkg/AmdSev/AmdSevX64.dsc                  |   1 +
>  OvmfPkg/CloudHv/CloudHvX64.dsc                |   1 +
>  .../Include/Library/NestedInterruptTplLib.h   |  87 +++++++
>  OvmfPkg/IntelTdx/IntelTdxX64.dsc              |   1 +
>  OvmfPkg/Library/NestedInterruptTplLib/Iret.c  |  62 +++++
>  OvmfPkg/Library/NestedInterruptTplLib/Iret.h  |  19 ++
>  .../NestedInterruptTplLib.inf                 |  35 +++
>  OvmfPkg/Library/NestedInterruptTplLib/Tpl.c   | 216 ++++++++++++++++++
>  OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.c |  14 +-
>  .../LocalApicTimerDxe/LocalApicTimerDxe.inf   |   1 +
>  OvmfPkg/Microvm/MicrovmX64.dsc                |   1 +
>  OvmfPkg/OvmfPkg.dec                           |   4 +
>  OvmfPkg/OvmfPkgIa32.dsc                       |   1 +
>  OvmfPkg/OvmfPkgIa32X64.dsc                    |   1 +
>  OvmfPkg/OvmfPkgX64.dsc                        |   1 +
>  OvmfPkg/OvmfXen.dsc                           |   1 +
>  18 files changed, 449 insertions(+), 12 deletions(-)
>  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
>
> --
> 2.38.1
>
>
>
> 
>
>