Re: [edk2-devel] [PATCH v3 3/5] MdeModulePkg: Do nothing on NestedInterruptRestoreTPL(TPL_HIGH_LEVEL)

["Laszlo Ersek" <lersek@redhat.com>]

On 1/23/24 17:59, Michael Brown wrote:
> On 23/01/2024 16:32, Laszlo Ersek wrote:
>> On 1/23/24 16:31, Michael Brown wrote:
>>> At TPL_HIGH_LEVEL, CPU interrupts are disabled (as per the UEFI
>>> specification) and so we should never encounter a situation in which
>>> an interrupt occurs at TPL_HIGH_LEVEL.
>>>
>>> Restoring TPL to TPL_HIGH_LEVEL is always a no-op.  Return immediately
>>> from NestedInterruptRestoreTPL(TPL_HIGH_LEVEL), so that we do not need
>>> to consider the effect of this possible invariant violation on the
>>> remainder of the logic.
>>
>> Feels like the handling logic might as well be "panic" here (except edk2
>> does not have a central panic API that's suitable for all platforms). I
>> probably missed the previous discussion that led to this patch. Either
>> way, it seems reasonable.
>>
>> Acked-by: Laszlo Ersek <lersek@redhat.com>
> 
> Thank you.  We can't panic because there are some bootloaders (*cough*
> Microsoft *cough*) that illegally call RaiseTPL(TPL_HIGH_LEVEL) and then
> even more illegally enable interrupts via STI.  Gerd tracked this down
> before, which lead to commit
> 
>   https://github.com/tianocore/edk2/commit/bee67e0c1
> 
> I found another way to trigger a RestoreTPL(TPL_HIGH_LEVEL) while I was
> testing the self-tests by deliberately breaking
> DisableInterruptsOnIret() to fail to disable interrupts.  This also
> induces a situation in which we end up at TPL_HIGH_LEVEL with interrupts
> enabled.
> 
> This ended up triggering an assertion (due to the invariant violation)
> in NestedInterruptRestoreTPL() before reaching the point in the
> self-test that would have reported a more meaningful error message.
> 
> Adding the bypass is justifiable on its own merits (as per the reasoning
> in the commit), and it avoids needing to add clutter to the complex
> logic in NestedInterruptRestoreTPL() just to ensure that the self-test
> fails on the desired ASSERT().
> 
> I decided against trying to explain all that in the commit message, but
> I can have a go if you think it needs to be captured.  :)

Ugh, no, thanks :) This should suffice! :)
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114286): https://edk2.groups.io/g/devel/message/114286
Mute This Topic: https://groups.io/mt/103911606/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-