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

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

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 <mcb30@ipxe.org>; devel@edk2.groups.io
> Cc: Gerd Hoffmann <kraxel@redhat.com>; Laszlo Ersek <lersek@redhat.com>
> 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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-