On Thu, Feb 29, 2024 at 2:04 PM Ray Ni <ray.ni@intel.com> wrote:
> @@ -134,9 +262,9 @@ CoreRestoreTpl (
>    }
>
>    //
> -  // Set the new value
> +  // Set the new TPL with interrupt disabled.
>    //
> -
> +  CoreSetInterruptState (FALSE);
>    gEfiCurrentTpl = NewTpl;
>
>    //
> @@ -144,7 +272,22 @@ CoreRestoreTpl (
>    // interrupts are enabled
>    //
>    if (gEfiCurrentTpl < TPL_HIGH_LEVEL) {
> -    CoreSetInterruptState (TRUE);
> +    if ((INTN)gEfiCurrentTpl > HighBitSet64 (mInterruptedTplMask)) {
> +      //
> +      // Only enable interrupts if restoring to a level above the highest
> +      // interrupted TPL level.  This allows interrupt nesting, but only for
> +      // events at higher TPL level than the current TPL level.
> +      //
> +      CoreSetInterruptState (TRUE);
> +    } else {
> +      //
> +      // Clear interrupted TPL level mask, but do not re-enable interrupts here
> +      // This will return to CoreTimerTick() and interrupts will be re-enabled
> +      // when the timer interrupt handlers returns from interrupt context.
> +      //
> +      ASSERT ((INTN)gEfiCurrentTpl == HighBitSet64 (mInterruptedTplMask));
> +      mInterruptedTplMask &= ~(UINTN)(1 << gEfiCurrentTpl);
> +    }
>    }

Ok, now I understand what's going on and it's indeed the same logic as
NestedInterruptTplLib, with DisableInterruptsOnIret() replaced by
skipping CoreSetInterruptState(TRUE). It's similar to what I proposed
elsewhere in the thread, just written differenty.

I agree with Michael Brown that the spec is unclear on the state of
the interrupt flag on exit from gBS->RestoreTPL(), but perhaps this
change is feasible if the interrupt handlers just raise the TPL first
and restore it last.

Just as an exercise for me to understand the code better, I tried
rewriting the code in terms of the CoreRestoreTplInternal() function
that I proposed. I find it easier to read, but I guess that's a bit in
the eye of the beholder, and it is a little more defensively coded. I
attach it (untested beyond compilation) for reference.

Paolo


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116192): https://edk2.groups.io/g/devel/message/116192
Mute This Topic: https://groups.io/mt/104642317/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-