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

["Michael Brown" <mcb30@ipxe.org>]

On 29/02/2024 16:43, Kinney, Michael D wrote:
> Hi Michael,
> 
> Can you provide a pointer to the UEFI Spec statement this breaks?

II-9.7.1.3 RestoreTPL():

"When the DXE Foundation is notified that the EFI_CPU_ARCH_PROTOCOL has 
been installed, then the full version of the Boot Service RestoreTPL() 
can be made available.  When an attempt is made to restore the TPL level 
to level below EFI_TPL_HIGH_LEVEL, then the DXE Foundation should use 
the services of the EFI_CPU_ARCH_PROTOCOL to enable interrupts."

I suspect this is sufficient to veto the proposed design, though we 
could argue that the loosely worded "should" is technically not "must".


If we still want to proceed with this design, then I have several other 
questions:

- How does the proposed patch react to an interrupt occurring 
(illegally) at TPL_HIGH_LEVEL (as happens with some versions of 
Windows)?  As far as I can tell, it will result in mInterruptedTplMask 
having bit 31 set but never cleared.  What impact will this have?

- How does the proposed patch react to potentially mismatched 
RaisedTPL()/RestoreTPL() calls (e.g. oldTpl = RaiseTPL(TPL_CALLBACK) 
followed by RaiseTPL(TPL_NOTIFY) followed by a single RestoreTPL(oldTpl))?


I believe the proposed patch is attempting to establish a new invariant 
as follows:

Once an interrupt has occured at a given TPL, then that *TPL* is 
conceptually considered to be in an "interrupted" state.  The *only* 
thing that can clear this "interrupted" state from the TPL is to return 
from the interrupt handler.

Note that this conceptual definition does not perfectly align with the 
bit flags in mInterruptedTplMask, since those bits will necessarily be 
set only some time after the interrupt occurs, and will have to be 
cleared before returning from the interrupt.  However, it is the 
conceptual definition that is relevant to the invariant.

The new invariant is that no code may execute at an "interrupted" TPL 
with interrupts enabled.  It is legitimate for code to raise to a higher 
TPL and to enable interrupts while there, and it is legitimate for code 
to execute in an "interrupted" TPL with interrupts disabled, but it is 
not legitimate for any code to reenable interrupts while still at an 
"interrupted" TPL.

It would be good to call out this invariant explicitly, so that authors 
of interrupt handlers are aware of the restrictions.  It would also 
clarify some of the logic (e.g. it provides the reason why interrupts 
must be disabled before lowering gEfiCurrentTpl in CoreRestoreTpl()).

It's also generally easier to reason about a stated invariant than to 
extrapolate from a list of complicated examples.

Thanks,

Michael



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