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