Michael, do you have any updated patch? Thanks, Ray ________________________________ From: Michael Brown <mc...@ipxe.org> Sent: Friday, March 1, 2024 19:10 To: Paolo Bonzini <pbonz...@redhat.com> Cc: Ni, Ray <ray...@intel.com>; devel@edk2.groups.io <devel@edk2.groups.io>; Kinney, Michael D <michael.d.kin...@intel.com>; Liming Gao <gaolim...@byosoft.com.cn>; Laszlo Ersek <ler...@redhat.com> 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 <mc...@ipxe.org> 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 (#116352): https://edk2.groups.io/g/devel/message/116352 Mute This Topic: https://groups.io/mt/104642317/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
