Re: [edk2-devel] [PATCH v3 4/5] MdeModulePkg: Add self-tests for NestedInterruptTplLib

["Ni, Ray" <ray.ni@intel.com>]

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