public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Michael Brown" <mcb30@ipxe.org>
To: devel@edk2.groups.io, lersek@redhat.com, kraxel@redhat.com
Cc: Pedro Falcato <pedro.falcato@gmail.com>,
	ray.ni@intel.com,  Michael D Kinney <michael.d.kinney@intel.com>,
	 Nate DeSimone <nathaniel.l.desimone@intel.com>,
	 Rahul Kumar <rahul1.kumar@intel.com>
Subject: Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe driver
Date: Tue, 16 Jan 2024 15:16:50 +0000	[thread overview]
Message-ID: <0102018d12d8bd9f-d209332f-f501-498e-b43c-3b0cc4f7ef7b-000000@eu-west-1.amazonses.com> (raw)
In-Reply-To: <a204767d-3e1e-12d5-7aa7-5ebc3f3306cb@redhat.com>

On 16/01/2024 14:34, Laszlo Ersek wrote:
> On 1/16/24 10:48, Michael Brown wrote:
> IOW, my impression is that NestedInterruptTplLib can certainly handle
> all scenarios thrown at it, but where it really matters is in the face
> of an interrupt storm (not just "normal nesting"), and a storm is
> unlikely (or even impossible?) on physical hardware.
> 
> ... Oh, scratch that. "Interrupt storm" simply means that interrupts are
> being delivered at a rate higher than the handler routine can service
> them. IOW, the "storm" is not that interrupts are delivered *very
> rapidly* in an absoulte sense. If interrupts are delivered at normal
> frequency, but the handler is too slow to service *even that rate*, then
> that also qualifies as "storm", because the nesting depth will *keep
> growing*. It's not really the growth rate that matters; what matter is
> the *trend*, i.e., the fact that there *is* growth (the stack gets
> deeper and deeper). The stack might not overflow immediately, and if the
> handler speeds up (for whatever reason), the stack might recover, but
> there is nothing to prevent an overflow.
> 
> So, in the end, I think you've convinced me.

:)

>> I'm happy to send a patch to migrate NestedInterruptTplLib to
>> MdeModulePkg, so that it can be consumed outside of OvmfPkg.  Shall I do
>> this?
> 
> Sounds like a valid idea to me.
> 
> Could be greatly supported by a test case (to be run on the bare metal)
> installing a slow handler that *eventually* exhausted the stack, when
> not using NestedInterruptTplLib.
> 
> (FWIW, IIRC, the UEFI spec warns about this -- it says something like,
> "return from TPL_HIGH as soon as you can, otherwise the system will
> become unstable".)
> 
> Sorry for the wall of text, I find this very difficult to reason about.

I also find it very difficult to reason about, which is why 
NestedInterruptRestoreTpl() has 126 lines of comments providing a 
semi-formal proof of correctness for a mere 15 statements of C code!

In particular, I find it difficult to reason about when it would be safe 
for a platform to *not* use NestedInterruptTplLib.  It's clearly 
empirically difficult to trigger stack underflow via an interrupt 
"storm" on physical hardware, but I'm not convinced it's impossible.

I find it mentally easier to rely on the hard guarantee that 
NestedInterruptTplLib provides: that nested interrupts will continue to 
be delivered but that the number of interrupt-induced stack frames is 
bounded by the (small, finite) number of distinct TPL levels in existence.



While developing NestedInterruptTplLib, I did hack together a test case 
for a slow handler that would deliberately induce an interrupt storm, 
since I needed this to test that my code was working.  When triggered, 
this test would cause the machine to effectively hang due to servicing 
an endless storm of timer interrupts.  Before NestedInterruptTplLib, the 
stack would soon underflow and would typically cause a reboot (or other 
crash).  With NestedInterruptTplLib the machine would continue to 
service interrupts indefinitely.

How might such a test case be included in upstream EDK2?  I'm 
peripherally aware of EDK2 test infrastructure such as UEFI SCT, but 
I've never interacted with it yet.

Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113908): https://edk2.groups.io/g/devel/message/113908
Mute This Topic: https://groups.io/mt/103734961/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



  reply	other threads:[~2024-01-16 15:16 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-01-15  8:03 [edk2-devel] Add LocalApicTimerDxe driver in UefiCpuPkg Ni, Ray
2024-01-15  8:03 ` [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe driver Ni, Ray
2024-01-15  8:59   ` Pedro Falcato
2024-01-15 18:10     ` Michael D Kinney
2024-01-16 10:02       ` Laszlo Ersek
2024-01-16 17:17         ` Michael D Kinney
2024-01-15 19:30     ` Laszlo Ersek
2024-01-16  8:47       ` Gerd Hoffmann
2024-01-16  9:48         ` Michael Brown
2024-01-16 14:34           ` Laszlo Ersek
2024-01-16 15:16             ` Michael Brown [this message]
2024-01-16 15:37               ` Laszlo Ersek
2024-01-17  7:11                 ` Ni, Ray
2024-01-17 10:46                   ` Michael Brown
2024-01-19 13:14                     ` Ni, Ray
2024-01-19 17:42                       ` Michael Brown
2024-01-19 23:44                         ` Ni, Ray
2024-01-20  0:49                           ` Michael Brown
2024-01-22 10:15                         ` Gerd Hoffmann
2024-01-15  8:03 ` [edk2-devel] [PATCH 2/6] UefiCpuPkg/LocalApicTimerDxe: Remove NestedInterruptTplLib call Ni, Ray
2024-01-15  8:03 ` [edk2-devel] [PATCH 3/6] UefiCpuPkg: Add LocalApicTimerDxe driver in DSC file Ni, Ray
2024-01-15  8:03 ` [edk2-devel] [PATCH 4/6] UefiCpuPkg/LocalApicTimerDxe: Enhance Timer Frequency calculation logic Ni, Ray
2024-01-15  8:03 ` [edk2-devel] [PATCH 5/6] UefiCpuPkg/LocalApicTimerDxe: warn if APIC Timer is used by other code Ni, Ray
2024-01-15  8:03 ` [edk2-devel] [PATCH 6/6] UefiCpuPkg/LocalApicTimerDxe: Passing fixed timer period is not a bug Ni, Ray

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=0102018d12d8bd9f-d209332f-f501-498e-b43c-3b0cc4f7ef7b-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