From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by mx.groups.io with SMTP id smtpd.web10.10072.1670581694840894615 for ; Fri, 09 Dec 2022 02:28:15 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=qYko0a5q; spf=pass (domain: kernel.org, ip: 145.40.68.75, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id BD4E4B82786 for ; Fri, 9 Dec 2022 10:28:12 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 6E054C433EF for ; Fri, 9 Dec 2022 10:28:11 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1670581691; bh=rgAI4ipEmoNrVvlltAD3wqBQkYS/3OfKUHVWWZ24Nwg=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=qYko0a5qZaXgsFniH1+AjNm49L7sqVUYTpHld6KXFMt+qr5xDn4CifPbSPn4YoapA wLZbtVf9xBoEsrMRxDBPMC5n+W+wWrBovMlcoka3uNT2lBiYMuZrWhqddNbkCHbV9d qtrDOyLKpjEyClJlxOh7eO/Tmw40rHE4r2cLVqVVR+qec/26AJCIxgBJN0vQCDMV9S hsoNmAfd7dn+cQn0874Sl+78lYBwbAkmp9gXdvAY/MO5WjVsZTYZuke/aOuLDtgfKk GaYAu7d+rubJtW3GAeMavsWioU426FqzsUv3kIWCljjFZ8mPTOxZz+SzozB0grpaVw FmRxZKR4qqGVg== Received: by mail-lj1-f178.google.com with SMTP id f20so4485349lja.4 for ; Fri, 09 Dec 2022 02:28:11 -0800 (PST) X-Gm-Message-State: ANoB5pnE6jrTeOgzGzdVzzrjRzsLx+jxEV/kfVPtwvZtYEEHeMG9QE/y 2Hocw2C/3N/Adg9anz4+u4v0yBbNan87aR+Aac0= X-Google-Smtp-Source: AA0mqf6BQLcLPDF0tGVtFhD9XNjIPBwjp5cuFByy2Y1gBhBFsnIgWcpiQ+8pCCM3dC3C1mZxPosTYDSsVhdDIHZjKxU= X-Received: by 2002:a05:651c:153:b0:279:bbff:a928 with SMTP id c19-20020a05651c015300b00279bbffa928mr11378634ljd.415.1670581689415; Fri, 09 Dec 2022 02:28:09 -0800 (PST) MIME-Version: 1.0 References: <01020184f6666d2f-ac1beed5-7683-4209-bf13-282fc0ca754f-000000@eu-west-1.amazonses.com> In-Reply-To: <01020184f6666d2f-ac1beed5-7683-4209-bf13-282fc0ca754f-000000@eu-west-1.amazonses.com> From: "Ard Biesheuvel" Date: Fri, 9 Dec 2022 11:27:58 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH 0/3] OvmfPkg: Fix handling of nested interrupts To: devel@edk2.groups.io, mcb30@ipxe.org, Gerd Hoffmann Cc: Laszlo Ersek , Paolo Bonzini Content-Type: text/plain; charset="UTF-8" (cc Gerd) On Fri, 9 Dec 2022 at 11:20, Michael Brown 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 > Cc: Paolo Bonzini > 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 > > > > > >