Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe driver

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

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]
-=-=-=-=-=-=-=-=-=-=-=-