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

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

On 01/03/2024 09:33, Paolo Bonzini wrote:
> On Fri, Mar 1, 2024 at 10:27 AM Michael Brown <mcb30@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 (#116232): https://edk2.groups.io/g/devel/message/116232
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]
-=-=-=-=-=-=-=-=-=-=-=-