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

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

On Mon, 8 May 2023 at 08:46, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 5/6/23 01:57, Ard Biesheuvel wrote:
> > 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
> >
>
> I can be entirely wrong here, but:
>
> - we looked for a solution (or workaround) to the original problem that
> stays within the boundaries of OvmfPkg, so sinking tweaks into the core
> TPL manipulation functions isn't ideal
>
> - regarding the TimerInterruptHandler() function(s) that do live in
> OvmfPkg, there had been tweaks to signaling end-of-interrupt (which I
> understand as sort of equivalent to your suggestion, as unless/until you
> signal EOI, no more interrupts will be *generated*), but those had not
> helped. The EOI was either too early and so we got the unbounded
> nesting, or it was too late, and no interrupts were generated while (for
> example) TPL_CALLBACK code would depend on timers with CheckEvent. See
> bug 4162 -- that was what prompted Michael to revert the EOI placement
> tweak and to implement NestedInterruptLib.
>
> Apologies if there are further interpretations of disarming the timer
> that I'm missing!
>

No, I think you've summed it up. And in any case, it is not OVMF's job
to police what the loader is doing to that extent - we should flag it
as a violation but beyond that, I don't think we should try to
interfere with this non-compliant behavior.