Re: [edk2-devel] [PATCH v3 4/5] MdeModulePkg: Add self-tests for NestedInterruptTplLib
On 1/23/24 18:41, Michael Brown wrote:
> On 23/01/2024 16:55, Laszlo Ersek wrote:
>>> + ///
>>> + /// Number of self-tests performed.
>>> + ///
>>> + UINTN SelfTestCount;
>>> } NESTED_INTERRUPT_STATE;
>>> /**
>>
>> I suggest that the new field be UINT32. The (exclusive) limit is a
>> 32-bit PCD. Making the counter (potentially) wider than the limit is not
>> useful, but it's also a bit of a complication for the debug messages
>> (see below).
>
> Great suggestion, thanks!
>
>>> + ///
>>> + /// Perform a limited number of self-tests on the first few
>>> + /// invocations.
>>> + ///
>>> + if ((IsrState->DeferredRestoreTPL == FALSE) &&
>>
>> This comment applies to several locations in the patch:
>>
>> BOOLEANs should not be checked using explicit "== TRUE" and "== FALSE"
>> operators / comparisons; they should only be evaluated in their logical
>> contexts:
>
> We've had this conversation before :)
>
> https://edk2.groups.io/g/devel/message/104369
Oops, sorry :)
>
> I personally find that the "!" operators get visually lost in the EDK2
> coding style with its very long variable and function names. That said,
> I'm happy to omit all of the explicit comparisons, but I should then add
> a precursor patch that changes the existing code style first. Thoughts?
I'm unsure.
Some people prefer (! Expression) over (!Expression), i.e., the extra
space, but I'm unsure if uncrustify tolerates that, and whether it
matches the edk2 coding style, regardless of uncrustify. How about:
!(Expression)
such as
if (!(IsrState->DeferredRestoreTPL) &&
? Does that make it more readable? (I believe this form would not trip
up uncrustify, and that it would satisfy the coding style too.)
With none of those working, the explicit ==TRUE / ==FALSE would not
"break" the coding style any stronger either, so in that case, feel free
(from my side) to just stick with the syntax in the patch (and in the
pre-patch code).
>
>>> +VOID
>>> +NestedInterruptSelfTest (
>>> + IN NESTED_INTERRUPT_STATE *IsrState
>>> + )
>>> +{
>>> + UINTN SelfTestCount;
>>> + UINTN TimeOut;
>>
>> Did this pass the uncrustify check? I think uncrustify would insist on
>> inserting two spaces here.
>>
>> For running uncrustify locally:
>
> You are right, and thank you for reminding me how to run the EDK2
> version of uncrustify. I've fixed up everything it identified.
>
>>> + // This error represents a bug in the platform-specific timer
>>> + // interrupt handler.
>>> + //
>>> + DEBUG ((
>>> + DEBUG_ERROR,
>>> + "Nested interrupt self-test %u/%u failed: no nested interrupt\n",
>>> + SelfTestCount,
>>> + NUMBER_OF_SELF_TESTS
>>> + ));
>>> + ASSERT (FALSE);
>>> +}
>>
>> I'd prefer something stronger than just ASSERT (FALSE) here, but -- per
>> previous discussion -- we don't have a generally accepted "panic" API
>> yet, and CpuDeadLoop() is not suitable for all platforms, so this
>> should do.
>>
>> With my trivial comments addressed:
>>
>> Acked-by: Laszlo Ersek
>>
>> Comment on the general idea: I much like that the self-test is active on
>> every boot (without high costs).
>
> Thank you!
>
>> Side idea: technically we could merge the first two patches in
>> separation (pending MdeModulePkg maintainer approval), and then consider
>> the last three patches as new improvements (possibly needing longer
>> review). This kind of splitting has both advantages and disadvantages;
>> the advantage is that the code movement / upstreaming to MdeModulePkg is
>> not blocked by (somewhat) unrelated discussion. The disadvantages are
>> that more admin work is needed (more posting, and more PRs), and that
>> patches in the series that one might consider to belong together will
>> fly apart in the git history. So I just figured I'd raise the option.
>
> I'm happy to work either way, and shall await instruction.
>
> In the absence of any instruction to the contrary, I'll send out a v4
> tomorrow with everyone's suggestions and tags included, but without the
> above-mentioned precursor patch to remove the boolean comparisons.
A v4 like that sounds good (as far as I'm aware, the first two patches
have no maintainer review yet, anyway).
Thanks!
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114289): https://edk2.groups.io/g/devel/message/114289
Mute This Topic: https://groups.io/mt/103911608/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v3 4/5] MdeModulePkg: Add self-tests for NestedInterruptTplLib
Thank you for adding the self-test code!
Thanks,
Ray
> -Original Message-
> From: Ni, Ray
> Sent: Wednesday, January 24, 2024 6:25 PM
> To: Michael Brown ; devel@edk2.groups.io
> Cc: Gerd Hoffmann ; Laszlo Ersek
> Subject: RE: [PATCH v3 4/5] MdeModulePkg: Add self-tests for
> NestedInterruptTplLib
>
> > +//
> > +// Number of self-tests to perform.
> > +//
> > +#define NUMBER_OF_SELF_TESTS \
> > + (FixedPcdGet32 (PcdNestedInterruptNumberOfSelfTests))
>
> 1. can we avoid defining a PCD? I do not see a need that each platform needs
> to use a different count. How about just define it as 1?
>
> > +
> > +STATIC
> > +VOID
> > +NestedInterruptSelfTest (
> > + IN NESTED_INTERRUPT_STATE *IsrState
> > + );
> > +
> > /**
> >Raise the task priority level to TPL_HIGH_LEVEL.
> >
> > @@ -211,6 +223,16 @@ NestedInterruptRestoreTPL (
> > //
> > DisableInterrupts ();
> >
> > +///
> > +/// Perform a limited number of self-tests on the first few
> > +/// invocations.
>
> 2. the comment could mention that the self-test only is performed when
> no nested interrupt happens in gBS->RestoreTpl() call in above.
>
> > +///
> > +if ((IsrState->DeferredRestoreTPL == FALSE) &&
> > + (IsrState->SelfTestCount < NUMBER_OF_SELF_TESTS)) {
>
> 3. might have coding style issue.
>
> > + //
> > + // The test has failed and we will halt the system. Disable
> > + // interrupts now so that any test-induced interrupt storm does not
> > + // prevent the fatal error messages from being displayed correctly.
> > + //
>
> 4. The comment might be wrong. It does not indicate the test has failed.
> It's possible that the timer period is very long that causes no timer
> interrupt
> happens till now.
> In that case, IsrState->DeferredRestoreTPL is FALSE. That's not an issue IMO.
>
> Or, how about we stall for ever to wait for the timer interrupt instead of
> waiting just 1 second.
> We could wait for the IsrState->DeferredRestoreTPL is TRUE.
>
> > + DisableInterrupts();
> > +
> > + //
> > + // If we observe that DeferredRestoreTPL is TRUE then this indicates
> > + // that an interrupt did occur and NestedInterruptRestoreTPL() chose
> > + // to defer the RestoreTPL() call to the outer handler, but that
> > + // DisableInterruptsOnIret() failed to cause interrupts to be
> > + // disabled after the IRET or equivalent instruction.
>
> 5. The comment "failed to cause interrupts to be disabled after IRET" can be
> inside the if-condition.
>
> > + //
> > + // This error represents a bug in the architecture-specific
> > + // implementation of DisableInterruptsOnIret().
> > + //
> > + if (IsrState->DeferredRestoreTPL == TRUE) {
> > +DEBUG ((
> > + DEBUG_ERROR,
> > + "Nested interrupt self-test %u/%u failed: interrupts still
> enabled\n",
> > + SelfTestCount,
> > + NUMBER_OF_SELF_TESTS
> > + ));
> > +ASSERT (FALSE);
> > + }
> > +
>
>
> > + //
> > + // If no timer interrupt occurred then this indicates that the timer
> > + // interrupt handler failed to rearm the timer before calling
> > + // NestedInterruptRestoreTPL(). This would prevent nested
> > + // interrupts from occurring at all, which would break
> > + // e.g. callbacks at TPL_CALLBACK that themselves wait on further
> > + // timer events.
> > + //
> > + // This error represents a bug in the platform-specific timer
> > + // interrupt handler.
> > + //
> > + DEBUG ((
> > +DEBUG_ERROR,
> > +"Nested interrupt self-test %u/%u failed: no nested interrupt\n",
> > +SelfTestCount,
> > +NUMBER_OF_SELF_TESTS
> > +));
> > + ASSERT (FALSE);
>
> 6. If we change the above code to wait forever until the DeferredRestoreTPL
> is TRUE,
> the above debug message and ASSERT() can be removed. Agree?
>
> > +}
> > --
> > 2.43.0
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114273): https://edk2.groups.io/g/devel/message/114273
Mute This Topic: https://groups.io/mt/103911608/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v3 4/5] MdeModulePkg: Add self-tests for NestedInterruptTplLib
> +//
> +// Number of self-tests to perform.
> +//
> +#define NUMBER_OF_SELF_TESTS \
> + (FixedPcdGet32 (PcdNestedInterruptNumberOfSelfTests))
1. can we avoid defining a PCD? I do not see a need that each platform needs
to use a different count. How about just define it as 1?
> +
> +STATIC
> +VOID
> +NestedInterruptSelfTest (
> + IN NESTED_INTERRUPT_STATE *IsrState
> + );
> +
> /**
>Raise the task priority level to TPL_HIGH_LEVEL.
>
> @@ -211,6 +223,16 @@ NestedInterruptRestoreTPL (
> //
> DisableInterrupts ();
>
> +///
> +/// Perform a limited number of self-tests on the first few
> +/// invocations.
2. the comment could mention that the self-test only is performed when
no nested interrupt happens in gBS->RestoreTpl() call in above.
> +///
> +if ((IsrState->DeferredRestoreTPL == FALSE) &&
> + (IsrState->SelfTestCount < NUMBER_OF_SELF_TESTS)) {
3. might have coding style issue.
> + //
> + // The test has failed and we will halt the system. Disable
> + // interrupts now so that any test-induced interrupt storm does not
> + // prevent the fatal error messages from being displayed correctly.
> + //
4. The comment might be wrong. It does not indicate the test has failed.
It's possible that the timer period is very long that causes no timer interrupt
happens till now.
In that case, IsrState->DeferredRestoreTPL is FALSE. That's not an issue IMO.
Or, how about we stall for ever to wait for the timer interrupt instead of
waiting just 1 second.
We could wait for the IsrState->DeferredRestoreTPL is TRUE.
> + DisableInterrupts();
> +
> + //
> + // If we observe that DeferredRestoreTPL is TRUE then this indicates
> + // that an interrupt did occur and NestedInterruptRestoreTPL() chose
> + // to defer the RestoreTPL() call to the outer handler, but that
> + // DisableInterruptsOnIret() failed to cause interrupts to be
> + // disabled after the IRET or equivalent instruction.
5. The comment "failed to cause interrupts to be disabled after IRET" can be
inside the if-condition.
> + //
> + // This error represents a bug in the architecture-specific
> + // implementation of DisableInterruptsOnIret().
> + //
> + if (IsrState->DeferredRestoreTPL == TRUE) {
> +DEBUG ((
> + DEBUG_ERROR,
> + "Nested interrupt self-test %u/%u failed: interrupts still enabled\n",
> + SelfTestCount,
> + NUMBER_OF_SELF_TESTS
> + ));
> +ASSERT (FALSE);
> + }
> +
> + //
> + // If no timer interrupt occurred then this indicates that the timer
> + // interrupt handler failed to rearm the timer before calling
> + // NestedInterruptRestoreTPL(). This would prevent nested
> + // interrupts from occurring at all, which would break
> + // e.g. callbacks at TPL_CALLBACK that themselves wait on further
> + // timer events.
> + //
> + // This error represents a bug in the platform-specific timer
> + // interrupt handler.
> + //
> + DEBUG ((
> +DEBUG_ERROR,
> +"Nested interrupt self-test %u/%u failed: no nested interrupt\n",
> +SelfTestCount,
> +NUMBER_OF_SELF_TESTS
> +));
> + ASSERT (FALSE);
6. If we change the above code to wait forever until the DeferredRestoreTPL is
TRUE,
the above debug message and ASSERT() can be removed. Agree?
> +}
> --
> 2.43.0
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114272): https://edk2.groups.io/g/devel/message/114272
Mute This Topic: https://groups.io/mt/103911608/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v3 4/5] MdeModulePkg: Add self-tests for NestedInterruptTplLib
On 23/01/2024 16:55, Laszlo Ersek wrote:
+ ///
+ /// Number of self-tests performed.
+ ///
+ UINTN SelfTestCount;
} NESTED_INTERRUPT_STATE;
/**
I suggest that the new field be UINT32. The (exclusive) limit is a
32-bit PCD. Making the counter (potentially) wider than the limit is not
useful, but it's also a bit of a complication for the debug messages
(see below).
Great suggestion, thanks!
+///
+/// Perform a limited number of self-tests on the first few
+/// invocations.
+///
+if ((IsrState->DeferredRestoreTPL == FALSE) &&
This comment applies to several locations in the patch:
BOOLEANs should not be checked using explicit "== TRUE" and "== FALSE"
operators / comparisons; they should only be evaluated in their logical
contexts:
We've had this conversation before :)
https://edk2.groups.io/g/devel/message/104369
I personally find that the "!" operators get visually lost in the EDK2
coding style with its very long variable and function names. That said,
I'm happy to omit all of the explicit comparisons, but I should then add
a precursor patch that changes the existing code style first. Thoughts?
+VOID
+NestedInterruptSelfTest (
+ IN NESTED_INTERRUPT_STATE *IsrState
+ )
+{
+ UINTN SelfTestCount;
+ UINTN TimeOut;
Did this pass the uncrustify check? I think uncrustify would insist on
inserting two spaces here.
For running uncrustify locally:
You are right, and thank you for reminding me how to run the EDK2
version of uncrustify. I've fixed up everything it identified.
+ // This error represents a bug in the platform-specific timer
+ // interrupt handler.
+ //
+ DEBUG ((
+DEBUG_ERROR,
+"Nested interrupt self-test %u/%u failed: no nested interrupt\n",
+SelfTestCount,
+NUMBER_OF_SELF_TESTS
+));
+ ASSERT (FALSE);
+}
I'd prefer something stronger than just ASSERT (FALSE) here, but -- per
previous discussion -- we don't have a generally accepted "panic" API
yet, and CpuDeadLoop() is not suitable for all platforms, so this should do.
With my trivial comments addressed:
Acked-by: Laszlo Ersek
Comment on the general idea: I much like that the self-test is active on
every boot (without high costs).
Thank you!
Side idea: technically we could merge the first two patches in
separation (pending MdeModulePkg maintainer approval), and then consider
the last three patches as new improvements (possibly needing longer
review). This kind of splitting has both advantages and disadvantages;
the advantage is that the code movement / upstreaming to MdeModulePkg is
not blocked by (somewhat) unrelated discussion. The disadvantages are
that more admin work is needed (more posting, and more PRs), and that
patches in the series that one might consider to belong together will
fly apart in the git history. So I just figured I'd raise the option.
I'm happy to work either way, and shall await instruction.
In the absence of any instruction to the contrary, I'll send out a v4
tomorrow with everyone's suggestions and tags included, but without the
above-mentioned precursor patch to remove the boolean comparisons.
Thanks,
Michael
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114227): https://edk2.groups.io/g/devel/message/114227
Mute This Topic: https://groups.io/mt/103911608/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v3 4/5] MdeModulePkg: Add self-tests for NestedInterruptTplLib
only superficial comments:
On 1/23/24 16:31, Michael Brown wrote:
> Add the ability to perform self-tests on the first few invocations of
> NestedInterruptRestoreTPL(), to verify that:
>
> - the timer interrupt handler correctly rearms the timer interrupt
> before calling NestedInterruptRestoreTPL(), and
>
> - the architecture-specific DisableInterruptsOnIret() implementation
> correctly causes interrupts to be disabled after the IRET or
> equivalent instruction.
>
> Any test failure will be treated as fatal and will halt the system
> with an appropriate diagnostic message.
>
> Each test invocation adds approximately one timer tick of delay to the
> overall system startup time.
>
> Only one test is performed by default (to avoid unnecessary system
> startup delay). The number of tests performed can be controlled via
> PcdNestedInterruptNumberOfSelfTests at build time.
>
> Signed-off-by: Michael Brown
> ---
> MdeModulePkg/MdeModulePkg.dec | 4 +
> .../NestedInterruptTplLib.inf | 3 +
> .../Include/Library/NestedInterruptTplLib.h | 4 +
> .../Library/NestedInterruptTplLib/Tpl.c | 129 ++
> 4 files changed, 140 insertions(+)
(This is not even a comment, just a hint :) consider passing
"--stat=1000 --stat-graph-width=20" to git, when formatting the patches.
Those options deal well with the extremely long filenames / pathnames in
edk2.)
>
> diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
> index d6fb729af5a7..efd32c197b18 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -1142,6 +1142,10 @@ [PcdsFixedAtBuild]
># @Prompt Enable large address image loading.
>
> gEfiMdeModulePkgTokenSpaceGuid.PcdImageLargeAddressLoad|TRUE|BOOLEAN|0x30001059
>
> + ## Number of NestedInterruptTplLib self-tests to perform at startup.
> + # @Prompt Number of NestedInterruptTplLib self-tests.
> +
> gEfiMdeModulePkgTokenSpaceGuid.PcdNestedInterruptNumberOfSelfTests|1|UINT32|0x30001060
> +
> [PcdsFixedAtBuild, PcdsPatchableInModule]
>## Dynamic type PCD can be registered callback function for Pcd setting
> action.
># PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum number of
> callback function
> diff --git
> a/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
> b/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
> index f130d6dcd213..e67d899b9446 100644
> --- a/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
> +++ b/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
> @@ -34,3 +34,6 @@ [LibraryClasses]
>
> [Depex.common.DXE_DRIVER]
>TRUE
> +
> +[Pcd]
> + gEfiMdeModulePkgTokenSpaceGuid.PcdNestedInterruptNumberOfSelfTests
> diff --git a/MdeModulePkg/Include/Library/NestedInterruptTplLib.h
> b/MdeModulePkg/Include/Library/NestedInterruptTplLib.h
> index 0ead6e4b346a..7dd934577e99 100644
> --- a/MdeModulePkg/Include/Library/NestedInterruptTplLib.h
> +++ b/MdeModulePkg/Include/Library/NestedInterruptTplLib.h
> @@ -32,6 +32,10 @@ typedef struct {
>/// interrupt handler.
>///
>BOOLEANDeferredRestoreTPL;
> + ///
> + /// Number of self-tests performed.
> + ///
> + UINTN SelfTestCount;
> } NESTED_INTERRUPT_STATE;
>
> /**
I suggest that the new field be UINT32. The (exclusive) limit is a
32-bit PCD. Making the counter (potentially) wider than the limit is not
useful, but it's also a bit of a complication for the debug messages
(see below).
> diff --git a/MdeModulePkg/Library/NestedInterruptTplLib/Tpl.c
> b/MdeModulePkg/Library/NestedInterruptTplLib/Tpl.c
> index 99af553ab189..dfe22331204f 100644
> --- a/MdeModulePkg/Library/NestedInterruptTplLib/Tpl.c
> +++ b/MdeModulePkg/Library/NestedInterruptTplLib/Tpl.c
> @@ -17,6 +17,18 @@
>
> #include "Iret.h"
>
> +//
> +// Number of self-tests to perform.
> +//
> +#define NUMBER_OF_SELF_TESTS \
> + (FixedPcdGet32 (PcdNestedInterruptNumberOfSelfTests))
> +
> +STATIC
> +VOID
> +NestedInterruptSelfTest (
> + IN NESTED_INTERRUPT_STATE *IsrState
> + );
> +
> /**
>Raise the task priority level to TPL_HIGH_LEVEL.
>
> @@ -211,6 +223,16 @@ NestedInterruptRestoreTPL (
> //
> DisableInterrupts ();
>
> +///
> +/// Perform a limited number of self-tests on the first few
> +/// invocations.
> +///
> +if ((IsrState->DeferredRestoreTPL == FALSE) &&
This comment applies to several locations in the patch:
BOOLEANs should not be checked using explicit "== TRUE" and "== FALSE"
operators / comparisons; they should only be evaluated in their logical
contexts:
(Foo)
(!Bar)
etc
> + (IsrState->SelfTestCount < NUMBER_OF_SELF_TESTS)) {
> + IsrState->SelfTestCount++;
> + NestedInterruptSelfTest (IsrState);
> +}
> +
> //
> // DEFERRAL RETURN POINT
> //
> @@ -248,3 +270,110 @@ NestedInterruptRestoreTPL (
> }
>}
> }
> +
> +/**
> + Perform in
[edk2-devel] [PATCH v3 4/5] MdeModulePkg: Add self-tests for NestedInterruptTplLib
Add the ability to perform self-tests on the first few invocations of
NestedInterruptRestoreTPL(), to verify that:
- the timer interrupt handler correctly rearms the timer interrupt
before calling NestedInterruptRestoreTPL(), and
- the architecture-specific DisableInterruptsOnIret() implementation
correctly causes interrupts to be disabled after the IRET or
equivalent instruction.
Any test failure will be treated as fatal and will halt the system
with an appropriate diagnostic message.
Each test invocation adds approximately one timer tick of delay to the
overall system startup time.
Only one test is performed by default (to avoid unnecessary system
startup delay). The number of tests performed can be controlled via
PcdNestedInterruptNumberOfSelfTests at build time.
Signed-off-by: Michael Brown
---
MdeModulePkg/MdeModulePkg.dec | 4 +
.../NestedInterruptTplLib.inf | 3 +
.../Include/Library/NestedInterruptTplLib.h | 4 +
.../Library/NestedInterruptTplLib/Tpl.c | 129 ++
4 files changed, 140 insertions(+)
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index d6fb729af5a7..efd32c197b18 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -1142,6 +1142,10 @@ [PcdsFixedAtBuild]
# @Prompt Enable large address image loading.
gEfiMdeModulePkgTokenSpaceGuid.PcdImageLargeAddressLoad|TRUE|BOOLEAN|0x30001059
+ ## Number of NestedInterruptTplLib self-tests to perform at startup.
+ # @Prompt Number of NestedInterruptTplLib self-tests.
+
gEfiMdeModulePkgTokenSpaceGuid.PcdNestedInterruptNumberOfSelfTests|1|UINT32|0x30001060
+
[PcdsFixedAtBuild, PcdsPatchableInModule]
## Dynamic type PCD can be registered callback function for Pcd setting
action.
# PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum number of
callback function
diff --git
a/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
b/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
index f130d6dcd213..e67d899b9446 100644
--- a/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
+++ b/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
@@ -34,3 +34,6 @@ [LibraryClasses]
[Depex.common.DXE_DRIVER]
TRUE
+
+[Pcd]
+ gEfiMdeModulePkgTokenSpaceGuid.PcdNestedInterruptNumberOfSelfTests
diff --git a/MdeModulePkg/Include/Library/NestedInterruptTplLib.h
b/MdeModulePkg/Include/Library/NestedInterruptTplLib.h
index 0ead6e4b346a..7dd934577e99 100644
--- a/MdeModulePkg/Include/Library/NestedInterruptTplLib.h
+++ b/MdeModulePkg/Include/Library/NestedInterruptTplLib.h
@@ -32,6 +32,10 @@ typedef struct {
/// interrupt handler.
///
BOOLEANDeferredRestoreTPL;
+ ///
+ /// Number of self-tests performed.
+ ///
+ UINTN SelfTestCount;
} NESTED_INTERRUPT_STATE;
/**
diff --git a/MdeModulePkg/Library/NestedInterruptTplLib/Tpl.c
b/MdeModulePkg/Library/NestedInterruptTplLib/Tpl.c
index 99af553ab189..dfe22331204f 100644
--- a/MdeModulePkg/Library/NestedInterruptTplLib/Tpl.c
+++ b/MdeModulePkg/Library/NestedInterruptTplLib/Tpl.c
@@ -17,6 +17,18 @@
#include "Iret.h"
+//
+// Number of self-tests to perform.
+//
+#define NUMBER_OF_SELF_TESTS \
+ (FixedPcdGet32 (PcdNestedInterruptNumberOfSelfTests))
+
+STATIC
+VOID
+NestedInterruptSelfTest (
+ IN NESTED_INTERRUPT_STATE *IsrState
+ );
+
/**
Raise the task priority level to TPL_HIGH_LEVEL.
@@ -211,6 +223,16 @@ NestedInterruptRestoreTPL (
//
DisableInterrupts ();
+///
+/// Perform a limited number of self-tests on the first few
+/// invocations.
+///
+if ((IsrState->DeferredRestoreTPL == FALSE) &&
+ (IsrState->SelfTestCount < NUMBER_OF_SELF_TESTS)) {
+ IsrState->SelfTestCount++;
+ NestedInterruptSelfTest (IsrState);
+}
+
//
// DEFERRAL RETURN POINT
//
@@ -248,3 +270,110 @@ NestedInterruptRestoreTPL (
}
}
}
+
+/**
+ Perform internal self-test.
+
+ Induce a delay to force a nested timer interrupt to take place, and
+ verify that the nested interrupt behaves as required.
+
+ @param IsrState A pointer to the state shared between all
+ invocations of the nested interrupt handler.
+**/
+VOID
+NestedInterruptSelfTest (
+ IN NESTED_INTERRUPT_STATE *IsrState
+ )
+{
+ UINTN SelfTestCount;
+ UINTN TimeOut;
+
+ //
+ // Record number of this self-test for debug messages.
+ //
+ SelfTestCount = IsrState->SelfTestCount;
+
+ //
+ // Re-enable interrupts and stall for up to one second to induce at
+ // least one more timer interrupt.
+ //
+ // This mimics the effect of an interrupt having occurred precisely
+ // at the end of our call to RestoreTPL(), with interrupts having
+ // been re-enabled by RestoreTPL() and with the interrupt happening
+ // to occur after the TPL has already been lowered back down to
+ // InterruptedTPL. (This is the scenar
