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

["Ni, Ray" <ray.ni@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4254 bytes --]

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 <mcb30@ipxe.org>
Sent: Saturday, January 20, 2024 1:42 AM
To: devel@edk2.groups.io <devel@edk2.groups.io>; Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; kraxel@redhat.com <kraxel@redhat.com>
Cc: Pedro Falcato <pedro.falcato@gmail.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Liu, Zhiguang <zhiguang.liu@intel.com>
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]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 6203 bytes --]