From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from a7-20.smtp-out.eu-west-1.amazonses.com (a7-20.smtp-out.eu-west-1.amazonses.com [54.240.7.20]) by mx.groups.io with SMTP id smtpd.web10.11907.1683329227503251324 for ; Fri, 05 May 2023 16:27:08 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@ipxe.org header.s=cphpx6z2rfcgehlykjjh3gknqe3hsoe2 header.b=jslSvi8r; spf=pass (domain: eu-west-1.amazonses.com, ip: 54.240.7.20, mailfrom: 01020187ee3d92cc-eb212c44-2e49-4ca2-992c-a2d7d3b03f6f-000000@eu-west-1.amazonses.com) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/simple; s=cphpx6z2rfcgehlykjjh3gknqe3hsoe2; d=ipxe.org; t=1683329225; h=Message-ID:Date:MIME-Version:To:Cc:References:From:Subject:In-Reply-To:Content-Type:Content-Transfer-Encoding; bh=knyuBmFLaHuIyjEn+MGc9Xkjzk0XPvRBGM8uNs38hoA=; b=jslSvi8rfJiozSk19nH59DdPbQK7HUfWPyvW3iU5+nzgZIMAu+E0v4iNqL6bL8rD M985QdMlg1T2LCMzSFj/ykFes9LrT13CxuyDOXhqbAqEBBuFKUPCLr6Pox9/tcIogxP yBlFZQJ3PunGW3J5+xJUVwREbm+46C5CbdVXVx5VtwanJF3I0hpPxx3CW1EgPHZ8oEK P+ZotqumcavXr0ypyUsUckPbxDQTudj58L6N7TWDqGqJm9QGK1WdGczWHr1jvGiRVJ1 jLKWczimtBnvcG/HBAqpx0yu9uIqTz8AVMWkZdy0FDnjOUAHPris2AujSxtKKzkHgio Vjpv8Ne2AA== DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/simple; s=shh3fegwg5fppqsuzphvschd53n6ihuv; d=amazonses.com; t=1683329225; h=Message-ID:Date:MIME-Version:To:Cc:References:From:Subject:In-Reply-To:Content-Type:Content-Transfer-Encoding:Feedback-ID; bh=knyuBmFLaHuIyjEn+MGc9Xkjzk0XPvRBGM8uNs38hoA=; b=U0bfS0a2h6GdKn32D4uTaXwcFaFBpwO1XV2M3Z7a3ibco2txz/B9pDQBAHKWO0f6 H/kYrpC8rsHiM9oE6Cn0lYU3oRvldy//MLnuAw80iZyhRBn/7fPvKlH7WRcprs2oCtR +ClarKdy/iCiDrsf/mdMiUnxLc5n8BwwRSO/x/Qc= Message-ID: <01020187ee3d92cc-eb212c44-2e49-4ca2-992c-a2d7d3b03f6f-000000@eu-west-1.amazonses.com> Date: Fri, 5 May 2023 23:27:05 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.6.0 To: devel@edk2.groups.io, lersek@redhat.com, kraxel@redhat.com Cc: Oliver Steffen , Pawel Polawski , Jiewen Yao , Ard Biesheuvel , Jordan Justen References: <20230503071954.266637-1-kraxel@redhat.com> <01020187ec402266-6d4dee99-5a0d-4105-abaf-419c2a5607cc-000000@eu-west-1.amazonses.com> From: "Michael Brown" Subject: Re: [edk2-devel] [PATCH v2 1/1] OvmfPkg/NestedInterruptTplLib: replace ASSERT() with a warning logged. In-Reply-To: X-Spam-Status: No, score=-2.9 required=5.0 tests=ALL_TRUSTED,BAYES_00 autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on blyat.fensystems.co.uk Feedback-ID: 1.eu-west-1.fspj4M/5bzJ9NLRzJP0PaxRwxrpZqiDQJ1IF94CF2TA=:AmazonSES X-SES-Outgoing: 2023.05.05-54.240.7.20 Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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