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 08:37, Paolo Bonzini wrote:
> So I am starting to see more and more similarities between the two
> approaches.  I went a step further with fresh mind, removing the while
> loop... and basically reinvented your and Michael's patch. :) The only
> difference in the logic is a slightly different handling of
> mInterruptedTplMask in CoreRestoreTpl(), which is a bit safer in my
> case.
> 
> However, my roundabout way of getting to the same patch resulted in
> very different comments. Personally, I found the large text at the
> head of mInterruptedTplMask a bit too much, and the ones inside the
> function too focused on "how" and not "why". Maybe it's my exposure to
> NestedInterruptTplLib, but I find that a much smaller text can achieve
> the same purpose, by explaining the logic instead of the individual
> steps.
> 
> My version is attached, feel free to reuse it (either entirely or
> partially) for a hypothetical v2. Apologies to you and Mike K for the
> confusion!

I much prefer this version of the patch, because the explanations are 
easier to follow and to reason about.


Minor point (applies to both your and Ray's versions):

- The use of gCpu->GetInterruptState() vs CoreSetInterruptState() is 
inconsistent.  It feels as though CoreGetInterruptState() ought to 
exist.  I assume that the PI spec defines whether interrupts are enabled 
or disabled prior to the CPU arch protocol being installed, so it should 
be possible to have a well-defined return value from 
CoreGetInterruptState().


Major point:

There is an implicit assumption that if interrupts are disabled when 
RaiseTPL() is called then we must have been called from an interrupt 
handler.  How sure are we that this assumption is correct?

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().  Maybe this is what we want?  If so, then we 
should probably phrase the comments in these terms instead of in terms 
of being called from an interrupt handler.


Thanks,

Michael



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