It's been a while. I will try to check if the patch can handle the case described by Michael below.
OldTpl = gBS->RaiseTPL (TPL_HIGH_LEVEL); ... gBS->RestoreTPL (OldTpl); gBS->RestoreTPL (OldTpl); or OldTpl = gBS->RaiseTPL (TPL_HIGH_LEVEL - 1); gBS->RaiseTPL (TPL_HIGH_LEVEL); .. gBS->RestoreTPL (OldTpl); Michael, more cases in your mind? Thanks, Ray ________________________________ From: [email protected] <[email protected]> on behalf of Ni, Ray <[email protected]> Sent: Tuesday, March 5, 2024 12:19 To: Michael Brown <[email protected]>; Paolo Bonzini <[email protected]> Cc: [email protected] <[email protected]>; Kinney, Michael D <[email protected]>; Liming Gao <[email protected]>; Laszlo Ersek <[email protected]> Subject: Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/DxeCore: Fix stack overflow issue due to nested interrupts Michael, do you have any updated patch? Thanks, Ray ________________________________ From: Michael Brown <[email protected]> Sent: Friday, March 1, 2024 19:10 To: Paolo Bonzini <[email protected]> Cc: Ni, Ray <[email protected]>; [email protected] <[email protected]>; Kinney, Michael D <[email protected]>; Liming Gao <[email protected]>; Laszlo Ersek <[email protected]> Subject: Re: [PATCH 2/2] MdeModulePkg/DxeCore: Fix stack overflow issue due to nested interrupts On 01/03/2024 09:33, Paolo Bonzini wrote: > On Fri, Mar 1, 2024 at 10:27 AM Michael Brown <[email protected]> wrote: >> 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. Right. I'm leaning towards treating this as a bugfix: essentially tightening up the semantics of RestoreTPL() to mean: - any callbacks in the range OldTpl < Tpl < gEfiCurrentTpl will be dispatched with interrupts unconditionally enabled - the TPL will be restored to OldTpl - the interrupt state will be restored to the value it had when the TPL was last raised from OldTpl It feels as though this should be able to be cleanly modelled with a single global state array BOOLEAN mSavedInterruptState[TPL_HIGH_LEVEL] (or possibly a bitmask, though using the array avoids having to disable interrupts just to write a value). I still need to think through the subtleties, to make sure it could cope with pathological edge cases such as OldTpl = gBS->RaiseTPL (TPL_HIGH_LEVEL); ... gBS->RestoreTPL (OldTpl); gBS->RestoreTPL (OldTpl); or OldTpl = gBS->RaiseTPL (TPL_HIGH_LEVEL - 1); gBS->RaiseTPL (TPL_HIGH_LEVEL); .. gBS->RestoreTPL (OldTpl); I think that at least one of the above pathological usage patterns would break the existing mInterruptedTplMask patches, since they currently clear state in RestoreTPL() and so will not correctly handle a duplicate call to RestoreTPL(). I'll try to get a patch put together over the weekend. Thanks, Michael -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#119602): https://edk2.groups.io/g/devel/message/119602 Mute This Topic: https://groups.io/mt/104642317/21656 Group Owner: [email protected] Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
