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

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

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?

Thanks,

Michael