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

["Paolo Bonzini" <pbonzini@redhat.com>]

On Fri, Mar 1, 2024 at 10:27 AM Michael Brown <mcb30@ipxe.org> wrote:
> > My version is attached, feel free to reuse it (either entirely or
> > partially) for a hypothetical v2. Apologies to you and Mike K for the
> > confusion!
>
> I much prefer this version of the patch, because the explanations are
> easier to follow and to reason about.

Thanks! I find the new logic to be quite appealing... now that I
understand it. Hopefully this provides a way to find the best of both
worlds.

> There is an implicit assumption that if interrupts are disabled when
> RaiseTPL() is called then we must have been called from an interrupt
> handler.  How sure are we that this assumption is correct?
>
> It's possible that it doesn't matter.  The new logic will effectively
> mean that RestoreTPL() will restore not only the TPL but also the
> interrupts-enabled state to whatever existed at the time of the
> corresponding RaiseTPL().

Right: that's what my comment says

+  // However, when the handler calls RestoreTPL
+  // before returning, we want to keep interrupts disabled.  This
+  // restores the exact state at the beginning of the handler,
+  // before the call to RaiseTPL(): low TPL and interrupts disabled.

but indeed it applies beyond interrupt handlers. It might even be a bugfix.

> Maybe this is what we want?  If so, then we
> should probably phrase the comments in these terms instead of in terms
> of being called from an interrupt handler.

I think phrasing the comments with reference to interrupt handlers is
fine, but it may be worth adding a comment to either
mInterruptedTplMask or CoreRestoreTpl(), like

+/// NOTE: Strictly speaking, this applies to anything that
+/// calls RaiseTPL() with interrupts disabled, not just
+/// interrupt handlers.  Interrupt handlers are just the case
+/// that we care the most about, because of the potential
+/// for stack overflow.

Paolo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116228): https://edk2.groups.io/g/devel/message/116228
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]
-=-=-=-=-=-=-=-=-=-=-=-