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 <ler...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
