public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH 0/3] OvmfPkg: Fix handling of nested interrupts
@ 2022-12-09 10:20 Michael Brown
  2022-12-09 10:27 ` [edk2-devel] " Ard Biesheuvel
  0 siblings, 1 reply; 2+ messages in thread
From: Michael Brown @ 2022-12-09 10:20 UTC (permalink / raw)
  To: devel; +Cc: Michael Brown, Laszlo Ersek, Paolo Bonzini

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


^ permalink raw reply	[flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-12-09 10:28 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-12-09 10:20 [PATCH 0/3] OvmfPkg: Fix handling of nested interrupts Michael Brown
2022-12-09 10:27 ` [edk2-devel] " Ard Biesheuvel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox