OvmfPkg: Fix handling of nested interrupts

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

Posted by Michael Brown 1 year, 4 months ago

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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97173): https://edk2.groups.io/g/devel/message/97173
Mute This Topic: https://groups.io/mt/95557693/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Ard Biesheuvel 1 year, 4 months ago

(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
>
>
>
> 
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97177): https://edk2.groups.io/g/devel/message/97177
Mute This Topic: https://groups.io/mt/95557693/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-