Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/DxeCore: Fix stack overflow issue due to nested interrupts

["Michael D Kinney" <michael.d.kinney@intel.com>]

I think one advantage of this new proposal is to prevent 
an extra level of nesting and use of stack resources in 
that extra level.

The nesting depth is then both predictable and minimized
for a given set of supported TPL levels.

Mike

> -----Original Message-----
> From: Michael Brown <mcb30@ipxe.org>
> Sent: Thursday, February 29, 2024 11:22 AM
> To: devel@edk2.groups.io; pbonzini@redhat.com; Ni, Ray
> <ray.ni@intel.com>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Liming Gao
> <gaoliming@byosoft.com.cn>; Laszlo Ersek <lersek@redhat.com>
> Subject: Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/DxeCore: Fix stack
> overflow issue due to nested interrupts
> 
> On 29/02/2024 19:04, Paolo Bonzini wrote:
> > On 2/29/24 14:02, Ray Ni wrote:
> >> In the end, it will lower the TPL to TPL_APPLICATION with interrupt
> >> enabled.
> >>
> >> However, it's possible that another timer interrupt happens just in
> >> the end
> >> of RestoreTPL() function when TPL is TPL_APPLICATION.
> >
> > How do non-OVMF platforms solve the issue?  Do they just have the
> same
> > bug as in https://bugzilla.tianocore.org/show_bug.cgi?id=4162 ?
> 
> Yes, they have that bug (or one of the bugs mentioned in that long
> discussion, depending on which particular implementation choices have
> been made).
> 
> > The design of NestedInterruptTplLib is that each nested interrupt
> must
> > increase the TPL, but if I understand correctly there is a hole here:
> >
> >    //
> >    // Call RestoreTPL() to allow event notifications to be
> >    // dispatched.  This will implicitly re-enable interrupts.
> >    //
> >    gBS->RestoreTPL (InterruptedTPL);
> >
> >    //
> >    // Re-disable interrupts after the call to RestoreTPL() to ensure
> >    // that we have exclusive access to the shared state.
> >    //
> >    DisableInterrupts ();
> >
> > because gBS->RestoreTPL will unconditionally enable interrupts if
> > InterruptedTPL < TPL_HIGH_LEVEL.
> 
> There's no hole there.
> 
> Yes, interrupts will be temporarily reenabled, but the whole function
> of
> NestedInterruptTplLib is to safely allow for this window in which
> interrupts have been (annoyingly) enabled by RestoreTPL().
> 
> If another interrupt *does* occur within that window, the inner
> interrupt handler will call NestedInterruptRestoreTPL(), which will
> take
> the code path leading to the "DEFERRAL INVOCATION POINT", and will
> therefore *not* call RestoreTPL() within that stack frame.  The inner
> interrupt returns at TPL_HIGH_LEVEL with interrupts still disabled, and
> execution is therefore guaranteed to immediately reach the "DEFERRAL
> RETURN POINT" in the outer interrupt handler.  The deferred call to
> RestoreTPL() is then safely executed in the context of the outer
> interrupt handler (i.e. with zero increase in stack usage, hence a
> guarantee of no stack overflow).
> 
> See the comments in the code for further details - I made them fairly
> extensive.  :)
> 
> > If possible, the easiest solution would be to merge
> > NestedInterruptTplLib into Core DXE.
> 
> The question with that approach would be how to cleanly violate the
> abstraction layer that separates the timer interrupt handler (existing
> in a separate DXE driver executable) from the implementation of
> CoreRestoreTplInternal() (existing in core DXE and not exposed via the
> boot services table).
> 
> Thanks,
> 
> Michael



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