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 <[email protected]>; [email protected] > Cc: Gerd Hoffmann <[email protected]>; Laszlo Ersek <[email protected]> > 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: [email protected] Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
