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

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

On 19/01/2024 23:44, Ni, Ray wrote:
> 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.

RestoreTpl() always has to enable interrupts during its execution, since 
interrupts must be allowed to occur while callbacks are running 
(otherwise the callbacks may break due to the system time freezing).

The only alternative approach I am aware of would be to add a 
RestoreTPLEx() call to EFI_BOOT_SERVICES, with an additional parameter 
EnableInterruptsAtRestoredTpl.

RestoreTPLEx() would then:

1. For each TPL between EfiCurrentTpl and OldTpl:
    a) enable interrupts
    b) dispatch any callbacks registered at this TPL
    c) disable interrupts

2. Re-enable interrupts before returning if 
EnableInterruptsAtRestoredTpl is TRUE.

The implementation of RestoreTPL() would then become just a call to 
RestoreTPLEx(OldTPl, (OldTpl < TPL_HIGH_LEVEL)).

This would require a change to the EFI_BOOT_SERVICES table definition, 
which is something that I don't think has happened in the 18 years since 
the UEFI specification was released.  There's a very good chance that 
such a table change would break something, somewhere.

RestoreTPLEx() could be installed as a protocol instead, but it seems 
very messy to have something so fundamental as TPL management and event 
dispatch handled through an installable (and therefore uninstallable) 
protocol.  Are there any other instances where deep internals of DxeCore 
are exposed in this way?

Lastly: I think the RestoreTPLEx approach ought to work, but I have not 
done any testing on it (and have no intention of trying it, unless Intel 
wants to fund the work).  NestedInterruptTplLib has been quite 
thoroughly tested by now.

> 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.

It's relatively easy to test with a deliberately broken ISR: that's how 
I tested it during development.

The semi-formal proof is an added bonus.  Testing shows that the 
symptoms have gone away, but the semi-formal proof is what gives 
confidence (to me, at least) that the problem has actually been fixed 
properly.

Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114104): https://edk2.groups.io/g/devel/message/114104
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]
-=-=-=-=-=-=-=-=-=-=-=-