From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id C56B1AC171D for ; Fri, 19 Jan 2024 17:42:21 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=9ZMnO4x0nEj2OCbObjtZky0CzSX5YIu3cPpAltxcMJk=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:User-Agent:Subject:To:Cc:References:From:Autocrypt:In-Reply-To:Feedback-ID:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1705686140; v=1; b=QvXFccpgUz9d+7UqGmce7Nqcit4UsW7W4tXz+Z28nGjpifN4T7s56fzwhdFUKP0nArae0Goa MuZEqjAZRRcsqudQEmUgRrQ2Yj+iwx597qt6GFB/XG8SvOyvVsIB86+6LAa/MTpnXM3LGeJXmMR gYIopUOTe6F/vAZJVvTE/5fg= X-Received: by 127.0.0.2 with SMTP id D7SwYY7687511xw4llXjSPm0; Fri, 19 Jan 2024 09:42:20 -0800 X-Received: from a7-17.smtp-out.eu-west-1.amazonses.com (a7-17.smtp-out.eu-west-1.amazonses.com [54.240.7.17]) by mx.groups.io with SMTP id smtpd.web11.1009.1705686139391070199 for ; Fri, 19 Jan 2024 09:42:19 -0800 Message-ID: <0102018d22d0f904-3aa0bdff-abe9-4c74-8316-0dc17122f372-000000@eu-west-1.amazonses.com> Date: Fri, 19 Jan 2024 17:42:17 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe driver To: devel@edk2.groups.io, ray.ni@intel.com, Laszlo Ersek , "kraxel@redhat.com" Cc: Pedro Falcato , "Kinney, Michael D" , "Desimone, Nathaniel L" , "Kumar, Rahul R" , "Liu, Zhiguang" References: <20240115080325.147-1-ray.ni@intel.com> <20240115080325.147-2-ray.ni@intel.com> <0102018d11ac8fe8-1ce1e102-af57-426f-bafc-7297bec4799a-000000@eu-west-1.amazonses.com> <0102018d12d8bd9f-d209332f-f501-498e-b43c-3b0cc4f7ef7b-000000@eu-west-1.amazonses.com> <0102018d17080a28-370d97eb-b73c-4811-a9ea-05cfb459ba37-000000@eu-west-1.amazonses.com> From: "Michael Brown" Autocrypt: addr=mcb30@ipxe.org; keydata= xsFNBGPmfF4BEAC3vcU4aLC/9Uy/rTpmYujbqxQNZje9E34jGvLxO3uYwj4BeHj1Nn5T2TDM Gkc4ngk+mGPsJsIn69YU5cfVN+ch9O7FVfsn6egZsCNeLy6Qz0o//gBaWJodFBeawuBjXXyV HnQZa1p7bA/Lws8minW7NrZ7XZgEBaiVm1v1dNbLEoWR8UL2AMtph5loCQ5jPYQNqp/wH9El /R30GjXvAd1riWyJR2TWSN23J9rnuH2Ue+N4yEnWxAsBQ6M/NFQ5z42w4mYdsnzy1w3PulrL icpSixXHkm3lQcKGtKKX41HvJukSpxCgbHfuHGEJZ7bdhgRic1DHKav0JR8kQhx3gnPh06z8 1Teu2NKkSsTR3Iv6E2x6Yy6H34lKWzBzd8TLNSevesDD/L6NU/HxT9AxrTBuypk9PZGe2VH1 W03XnR/0Mnr0QqQBXcIAERdgNzRJY4VKF75vedf8IooZFUQ4RUlqH+x3aZB9nJ9ET77mPaNi SQVQBxE68uzb7eh2Kf6z7ftOYpWPw1v5HyB3oMmafEDG36SIvNF2wnmNaLQDRnAbTcy4ERgy tpJ3wtQDJeXOePLv8hJ3q7DSuePl7cwz4xy0ZHglW/EXRXLnyRRACfDGowyENoStg06qF+qm edGu1wNtmDZ/lypWm/CkzzpUDFeGP5BLZlqwVX4hn88llfvVzwARAQABzR5NaWNoYWVsIEJy b3duIDxtY2IzMEBpcHhlLm9yZz7CwZEEEwEIADsWIQTgD69MBpjBm2slMvwCNbEKAOtEUAUC Y+Z8nwIbAwULCQgHAgIiAgYVCgkICwIEFgIDAQIeBwIXgAAKCRACNbEKAOtEUFlhD/9ElIUg JxBXpIbF8s7u79OdXLld2Z1DfVmhP5Q+GilPvEeAWHhp689S9B88aNvpwW5zJfxlxcJZO0ay jc7E/vtdNrkXGWNEEXBgdve6m+uL+pW/i5E2htqxbLyfgTJKmsvJ8graHbwrrBS/PA8KuwVJ eAGbBNi3f1gyQQWrLqfTkUpLtuj7A76iVVk0G0a78L69Al84qhK2imqpFJoZt1F8h0Z5ddGv mvf2M/DZp87UXvXjy7X6r7msbMZa6S/Jv0dtWHeZGl3Xu3qzbtjlqFyz2Q7TibHiirsgg/CV BsbH/LLbi/aNCCQ/85C6jAMB0lNzcVZ7ZiKKo+vBNMTycDFk70LA9yjlNf7exHejoXmPkLmH ddapYZ4dzwdOiJlaTu8NZgzXUCt3RDDA1qmZrAOBF/F+tPILAEhenl9kj3blD3mPV2SrWLWY dbahY9BsylUhj/qE1ik5CJXrPotmJhok9Vpg07xKDpVnZXuWLGNIE8018UumO7phLrWQwLb1 wJdN7PG165w4UWf4aQphfwaMKOVU3WDghz3aVSP9rgtm3RsUcYHPKx8IaPcDh2yf0bgG386i Axx3U3UQeyz2Pb9Vigo6DmPwXjLkFr/dukvVLVJLVkUab9ZhhERzWTEEMifUVEK2rGNvA87L VKJ2zOyxWx1e0CPj6fcGbkJ0D10XLs7BTQRj5nxeARAAz18zv2ksRiM6eEKG0qzpiKHVYlVy wtjla+m9wuAIwm314tffY5hjQN46uwTstdhQirjywF1EmcS6KNGiIjmoLim+dqyFP5d/UF5A VjLt0TYq7HjadIxbm2/CvcRnNJ01FkD99xLxV0hFTUAWAUX1mNqQ3MmWIjV89wiT06uuAUog m+jG3RRDyWbUnVELR60mhzccKsaEsjO/HqIERvBwL7tlOJewlPrVyz9Zed9Nhhv0KDAYmdEm kIEEbOfsjRu5I6nIY3NrX+QP9+nmgxADlsjvLXTSU0fT/g7IPEl3gpsQZAbgmrlGcPtvXod8 P4iOmL8GJDU1RdBE9TBOLEbu9UlDRD4zr6tdzRpB9wvXdtSUcNCdHVqJTfq2qjIlBk7x+zQD ayhxzDvTMxD/93K6txKXmVVtfMBsmt9KuD2JBUEAExjsLHqzg48nQg8wF9JYWCWGBb36qpd0 yC6VPzhSLe2Ov3/GyV5ZshO046+OiGxEeaHCwMnDTZF9xrQ5paCwWedlWKvGM2zB64AHuk+M v2ABK/gbDO7eS6p+xz11oD1NHr1HQLRtknfClIqj9AmjgX9maD+4GUrmHaxmkNilIukahotd Un9Up2gX05Wy/S3H/v8RB0kxwWg2Wh065dnyCF4Doe18bcYZvM+iMJmUBag6aDfQlryM04K7 z4ITYDkAEQEAAcLBdgQYAQgAIBYhBOAPr0wGmMGbayUy/AI1sQoA60RQBQJj5nxeAhsMAAoJ EAI1sQoA60RQZj4QAIkiRDVNWynZ4kEdpqmf6hpD++Zycz+LMne4iGRsiyyTf/rPNgskNLrU JD555yDvFiEAhOI27R8YNCJj5byXRDa/Bm6ueClFia+POibt28UEdyOFU9PVcgFaU+VxaBIP rHacHL6A7UKFjmBN7o8VkVF2xXlmFge795mP4/Y3t6qfWUTodrpw1w1t5/bZxZdWqX4pUCpY fEx87jm60+Mj0Tb4VPWXz0UD1q1BDcdYxNa2ISLaJhGJmjjks9eqdFOhPo1fTINMNWF2Alxi jA6WNT8nn9lm1kav75EMYMc8WIR9tb03i+IuKNp2IWwTGBqIUyQj00BhHkZQFl4HxZhV0gXE AWu34Q/Z7hOUXGXq2tvYCxDeaQb2wks93e62lrrUm1JGhPWkVoCI8Md8N2mkonqIfMK8lQ0W WbkYHdKBkgDqhDypNNhkjWNX3JL1kL0c3rqGL381iBAZaGQPygyCx2xH9PDNp59W6u8sXb13 +UX+kXdWU+KYbMTVoO/t4MxUJg6nXPJHz9NCkyluI820l+2OtXZZy0u196evIlUdD6RoTrNK z5OgFxNctVi9BPsQea9du+JlYJ460vZNPz180oczj7iqffd+p9DmAkeK25njWhg3qPeXiNZN 45J9eMChSOaJ0GMGUQndIIxz7PO8IzjbkSHLG5CKrR3MaphMB/0L 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: 2024.01.19-54.240.7.17 Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,mcb30@ipxe.org List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: 5sYUYHThqa1TIN7C99ajOcwux7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=QvXFccpg; dmarc=none; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io On 19/01/2024 13:14, Ni, Ray wrote: > So, the interrupt re-entrance we want to avoid is “env:NOTIFY”  -> > “env:NOTIFY”, or “env:CALLBACK” -> “env:CALLBACK”, or “env:APPLICATION” > -> “env:APPLICATION”. Because it’s endless. > > NestedTplInterruptLib was written to avoid it. Yes, precisely this. > 2. Some questions on NestedInterruptTplLib. > > 1. Can we remove DisableInterruptsOnIret()? That means the inner > interrupt handler would returns to the outer world with interrupt > enabled and TPL==HIGH. But I don’t see any issue with that. Using DisableInterruptsOnIret() allows us to guarantee that absolutely nothing happens between the "DEFERRAL INVOCATION POINT" and "DEFERRAL RETURN POINT" described in the comments in Tpl.c. If we don't use DisableInterruptsOnIret() then we lose this guarantee, and the situation becomes even more complex than it already is. I don't personally feel able to reason through all the possible circumstances that could arise if an interrupt were to occur between "DEFERRAL INVOCATION POINT" and "DEFERRAL RETURN POINT", so I don't feel safe removing the use of DisableInterruptsOnIret(). I have a vague memory that I was still experiencing some kind of crashes before I added DisableInterruptsOnIret(), but I cannot now remember any details, sorry. > 2. If DxeCore can be changed, do you have an easier-to-understand > solution? It really took me 2 days to understand why > NestedInterruptTplLib is written in today’s way. The ability to change DxeCore doesn't help, unfortunately. If we could change the prototype of RaiseTPL() and RestoreTPL() to include a flag indicating whether or not interrupts should be enabled at the point that RestoreTPL() returns, then that would allow for an easier-to-understand solution. This would require making a breaking change to the UEFI specification, though, so it's not a viable solution. I do appreciate that it's difficult to understand the internals of NestedInterruptTplLib. It's fundamentally having to solve a very difficult problem within the constraints of the UEFI API. I think the solution that NestedInterruptTplLib provides is as simple as it's possible to get, and it does at least have the advantage that all of the complexity is hidden inside the library: the caller gets to just change two lines: - OriginalTPL = gBS->RaiseTPL(TPL_HIGH_LEVEL); + OriginalTPL = NestedInterruptRaiseTPL(); ... - gBS->RestoreTPL(OriginalTPL); + NestedInterruptRestoreTPL(OriginalTPL, Context, &State); I'll send through a patch to move NestedInterruptTplLib to MdeModulePkg. Thanks, Michael -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#114091): https://edk2.groups.io/g/devel/message/114091 Mute This Topic: https://groups.io/mt/103734961/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-