Re: [edk2-devel] [PATCH v2 1/1] OvmfPkg/NestedInterruptTplLib: replace ASSERT() with a warning logged.

["Ard Biesheuvel" <ardb@kernel.org>]

On Sat, 6 May 2023 at 01:27, Michael Brown <mcb30@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