On Sat, 6 May 2023 at 01:27, Michael Brown <mc...@ipxe.org> wrote: > > On 05/05/2023 19:56, Laszlo Ersek wrote: > > I don't like the patch. For two reasons: > > > > (1) It papers over the actual issue. The problem should be fixed where > > it is, if possible. > > Agreed, but (as you have shown in > https://bugzilla.redhat.com/show_bug.cgi?id=2189136) the bug lies in > Windows code rather than in EDK2 code. If the goal is to allow these > buggy Windows builds to still be used with OVMF, then the only option is > to paper over the issue. We should do this only if it can be proven > safe to do so, of course. > > > (2) With the patch applied, NestedInterruptRaiseTPL() can return > > TPL_HIGH_LEVEL (as "InterruptedTPL"). Consequently, > > TimerInterruptHandler() [OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.c] > > may pass TPL_HIGH_LEVEL back to NestedInterruptRestoreTPL(), as > > "InterruptedTPL". > > > > I believe that this in turn may invalidate at least one comment in > > NestedInterruptRestoreTPL(): > > > > // > > // Call RestoreTPL() to allow event notifications to be > > // dispatched. This will implicitly re-enable interrupts. > > // > > gBS->RestoreTPL (InterruptedTPL); > > > > Restoring TPL_HIGH_LEVEL does not re-enable interrupts -- nominally anyways. > > I agree that the comment is invalidated, but as far as I can tell the > logic remains safe. > > I will put together a patch to update the comments in > NestedInterruptTplLib to address the possibility of an interrupt > occurring (illegally) at TPL_HIGH_LEVEL. > > > (a) Make LocalApicTimerDxe Xen-specific again. It's only the OVMF Xen > > platform that really *needs* NestedInterruptTplLib. (Don't get me wrong: > > NestedInterruptTplLib is technically correct in all circumstances, but > > in practice it happens to be too strict.) > > > > (b) For the non-Xen OVMF platforms, re-create a LocalApicTimerDxe > > variant that effectively has commits a086f4a63bc0 and a24fbd606125 > > reverted. (We should keep 9bf473da4c1d.) This returns us to > > pre-239b50a86370 status -- that is, a timer interrupt handler that (a) > > does not try to be smart about nested interrupts, therefore one that is > > much simpler, and (b) is more tolerant of the Windows / cdboot.efi spec > > violation, (c) is vulnerable to the timer interrupt storm seen on Xen, > > but will never run on Xen. (Only the OVMF Xen platform is supposed to be > > launched on Xen.) > > I'm less keen on this because it reduces the runtime exposure of a very > complex piece of code, and will effectively cause that code to become > unmaintained. > > It's also satisfying (to me) that NestedInterruptTplLib provides a > provable upper bound on stack consumption due to interrupts, which can't > be guaranteed by the simpler pre-239b50a86370 scheme. > > Could we defer judgement until after I've fully reasoned through (and > documented) how NestedInterruptTplLib will work in the presence of > interrupts occurring at TPL_HIGH_LEVEL? >
Would it be feasible for our firmware implementation to disable the timer interrupt at the timer end as well? E.g., RaiseTPL(HIGH):: CLI disarm timer RestoreTPL:: <complain if HIGH and interrupts enabled at CPU side> re-arm timer STI -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#104164): https://edk2.groups.io/g/devel/message/104164 Mute This Topic: https://groups.io/mt/98656860/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
