On 25/01/2024 13:54, Ni, Ray wrote:
I don't disagree with the approach, but it does break the API as per the
UEFI PI specification (version 1.8 section II-12.10), and so this is not
something that can just be dropped in as an EDK2 code change.
You think that the TimerInterruptHandler() doesn't raise/restore TPL
which would violate the PI spec as PI spec says " NotifyFunction ... executes at
EFI_TPL_HIGH_LEVEL."?
I do not think the PI spec requires TimerInterruptHandler() raises TPL
to HIGH before invoking NotifyFunction. It just means the NotifyFunction
will execute at TPL_HIGH.
If the caller is not supposed to raise TPL to TPL_HIGH_LEVEL before
calling NotifyFunction, then the statement "This function executes at
EFI_TPL_HIGH_LEVEL" in the PI specification is meaningless. There is no
other possible interpretation besides "the caller must raise TPL to
TPL_HIGH_LEVEL before calling this function".
If you review HpetTimer driver, it does not raise TPL to HIGH before
invoking NotifyFunction.
That would then be a bug in HpetTimer, which ought to be fixed. If
HpetTimer were to be used on a platform where the NotifyFunction
correctly assumes that it is called at TPL_HIGH_LEVEL and does something
that would break at a lower level, then this could lead to undefined
behaviour.
And I think implementing the DxeCore changes as attached does not
prevent the TimerInterruptHandler() from calling raise/restore TPL.
No, but a spec-conforming timer interrupt handler could not take
advantage of the feature, because it would have to raise to
TPL_HIGH_LEVEL before calling the NotifyFunction. (Any raise/restore
within the NotifyFunction would then have no effect.)
So, with the changes done in DxeCore, a timer driver could either
not raise/restore TPL in TimerInterruptHandler(), or it calls
NestedInterruptTplLib if it wants.
As a pure code change, I do agree that it solves the problem and it's a
much simpler approach. However, it is a breaking change to the
specification and I think it would need be handled as such.
The minimal specification change I can think of that would make this
possible would be to relax the wording on NotifyFunction in the next
version of the PI specification to say that
* the NotifyFunction can be called at any TPL level
* the NotifyFunction will raise TPL to TPL_HIGH_LEVEL, restore TPL back
to the original TPL before returning
* the NotifyFunction may re-enable interrupts during its execution, and
that the caller must be prepared to be re-entered before NotifyFunction
returns
* the timer interrupt must have been rearmed before calling NotifyFunction
* the NotifyFunction must guarantee that it never reaches a state in
which the TPL has been restored to the original level with CPU
interrupts enabled.
This would be backwards compatible with the existing behaviour. A
caller written to the current specification would call NotifyFunction at
TPL_HIGH_LEVEL and so any RaiseTPL/RestoreTPL done within a
NotifyFunction complying to the new specification would be a no-op anyway.
A caller written to the new specification would have to check the
supported version of the PI specification (which I assume is available
in some system configuration table somewhere) to know that it was safe
to call NotifyFunction without first raising to TPL_HIGH_LEVEL.
This approach would at least avoid the need for an ARCH2_PROTOCOL
variant, which is potentially lower impact.
Thanks,
Michael
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114407): https://edk2.groups.io/g/devel/message/114407
Mute This Topic: https://groups.io/mt/103950154/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
