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

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

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.  :)

Thanks,

Michael



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