From: "Michael Brown" <mcb30@ipxe.org>
To: devel@edk2.groups.io
Cc: Michael Brown <mcb30@ipxe.org>, Laszlo Ersek <lersek@redhat.com>,
Paolo Bonzini <pbonzini@redhat.com>
Subject: [PATCH 0/3] OvmfPkg: Fix handling of nested interrupts
Date: Fri, 9 Dec 2022 10:20:18 +0000 [thread overview]
Message-ID: <01020184f6666d2f-ac1beed5-7683-4209-bf13-282fc0ca754f-000000@eu-west-1.amazonses.com> (raw)
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
next reply other threads:[~2022-12-09 10:20 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-09 10:20 Michael Brown [this message]
2022-12-09 10:27 ` [edk2-devel] [PATCH 0/3] OvmfPkg: Fix handling of nested interrupts Ard Biesheuvel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=01020184f6666d2f-ac1beed5-7683-4209-bf13-282fc0ca754f-000000@eu-west-1.amazonses.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox