Michael, I still want to see if the RestoreTpl2 that does not enable interrupt is added as a protocol, and how simple the lib could be. The reason is about maintainability. I can image that one day people would question the Lib implementation if some timer event issue appears. If the Lib is easy to understand, the suspicion could be avoided. And if the correctness of the Lib can be proven by a thorough test, that will be better. But it seems to me the Lib can only be proven as correct with careful code review, like some multi-threaded logic. thanks, ray ________________________________ From: Michael Brown Sent: Saturday, January 20, 2024 1:42 AM To: devel@edk2.groups.io ; Ni, Ray ; Laszlo Ersek ; kraxel@redhat.com Cc: Pedro Falcato ; Kinney, Michael D ; Desimone, Nathaniel L ; Kumar, Rahul R ; Liu, Zhiguang Subject: Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe driver 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 (#114102): https://edk2.groups.io/g/devel/message/114102 Mute This Topic: https://groups.io/mt/103734961/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-