Re: [edk2-devel] RFC: Another solution to the nested interrupt issue

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

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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-