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

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

On 28/04/2023 10:10, Gerd Hoffmann wrote:
> OVMF can't guarantee that the ASSERT() doesn't happen.  Misbehaving
> EFI applications can trigger this.  So log a warning instead and try
> to continue.
> 
> Reproducer: Fetch windows 11 22H2 iso image, boot it in qemu with OVMF.
> 
> Traced to BootServices->Stall() being called with IPL=TPL_HIGH_LEVEL
> and Interrupts /enabled/ while windows is booting.

Do we know how the system ended up in a state of being at TPL_HIGH_LEVEL 
with interrupts enabled?  This ought not to be possible: the code in 
EDK2 will (as far as I can tell) always maintain the invariant that 
interrupts are disabled while at TPL_HIGH_LEVEL.

> --- a/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c
> +++ b/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c
> @@ -39,7 +39,15 @@ NestedInterruptRaiseTPL (
>     //
>     ASSERT (GetInterruptState () == FALSE);
>     InterruptedTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL);
> -  ASSERT (InterruptedTPL < TPL_HIGH_LEVEL);
> +  if (InterruptedTPL >= TPL_HIGH_LEVEL) {
> +    DEBUG ((
> +      DEBUG_WARN,
> +      "%a: Called at IPL %d, trying to fixup and continue...\n",
> +      __func__,
> +      InterruptedTPL
> +      ));
> +    InterruptedTPL = TPL_HIGH_LEVEL - 1;
> +  }

This workaround looks wrong to me: it will cause the subsequent call to 
NestedInterruptRestoreTPL() to drop the TPL back down to 
TPL_HIGH_LEVEL-1, which is lower than it would have been when the 
interrupt occurred.  The completed hardware interrupt would then result 
in an overall change of TPL, which cannot be correct.

Thanks,

Michael