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

["Michael D Kinney" <michael.d.kinney@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3832 bytes --]

Hi Paolo,

The proposed change does not disable interrupts at TPL below TPL_HIGH_LEVEL when processing event handlers.

It only prevents interrupts being enabled in the window from the last event processed in a timer interrupt and the return from the timer interrupt handler.

This is a window where the only control flows are in the DXE Core and the exit of the timer interrupt handler.

Mike

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Paolo Bonzini
Sent: Thursday, February 29, 2024 9:40 AM
To: Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Michael Brown <mcb30@ipxe.org>; edk2-devel-groups-io <devel@edk2.groups.io>; Ni, Ray <ray.ni@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Laszlo Ersek <lersek@redhat.com>
Subject: Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/DxeCore: Fix stack overflow issue due to nested interrupts


Il gio 29 feb 2024, 17:45 Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> ha scritto:
Hi Michael,

Can you provide a pointer to the UEFI Spec statement this breaks?

The spec does say that interrupts are disabled for TPL_HIGH_LEVEL, but indeed it doesn't say they are always enabled at lower levels. However, if the interrupts aren't always enabled whenever you're below TPL_HIGH_LEVEL, you get priority inversions (and deadlocks).

For example, if you end up running with interrupts disabled at TPL_CALLBACK, you are disabling the dispatching of timers at TPL_NOTIFY.

I guess this can be deduced from these two passages:

- "The functions in these queues are invoked in FIFO order, starting with the highest priority level queue and proceeding to the lowest priority queue that is unmasked by the current TPL"

- "If Type is TimerRelative and TriggerTime is 0, then the timer event will be signaled on the next timer tick" (in the description of gBS->SetTimer)

Paolo


Thanks,

Mike

> -----Original Message-----
> From: Michael Brown <mcb30@ipxe.org<mailto:mcb30@ipxe.org>>
> Sent: Thursday, February 29, 2024 5:23 AM
> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Liming Gao
> <gaoliming@byosoft.com.cn<mailto:gaoliming@byosoft.com.cn>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; Paolo
> Bonzini <pbonzini@redhat.com<mailto:pbonzini@redhat.com>>
> Subject: Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/DxeCore: Fix stack
> overflow issue due to nested interrupts
>
> On 29/02/2024 13:02, Ni, Ray wrote:
> > A ideal solution is to not keep the interrupt disabled when
> > RestoreTPL(TPL_HIGH -> not TPL_HIGH) is executed in the timer
> interrupt
> > context because the interrupt handler will re-enable the interrupt
> with
> > arch specific instructions (e.g.: IRET for x86).
> >
> > The patch introduces mInterruptedTplMask which tells RestoreTPL() if
> > it's called in the interrupt context and whether it should defer
> enabling
> > the interrupt.
>
> NACK.  This breaks the specification-defined behaviour for
> RestoreTPL().
>
> What guarantees do we have that there is no code anywhere in the world
> that relies upon RestoreTPL() unconditionally re-enabling interrupts.
>
> I also find this code substantially harder to follow than
> NestedInterruptTplLib (which does not break any specified behaviour).
>
> Thanks,
>
> Michael



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



[-- Attachment #2: Type: text/html, Size: 8624 bytes --]