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

["Laszlo Ersek" <lersek@redhat.com>]

On 5/6/23 01:27, Michael Brown 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?

Sure, absolutely!

As I wrote elsewhere: if you can revalidate the code with a new, less
strict set of invariants, and update the comments, I think that would be
the perfect workaround.

Thank you,
Laszlo