Re: [edk2-devel] [PATCH] UefiCpuPkg: Restore HpetTimer after CpuExceptionHandlerLib test

["Michael D Kinney" <michael.d.kinney@intel.com>]

Hi Ray,

I do not think it makes sense for the CpuExceptionHandlerLib unit
test to directly access the IO APIC.  It assumes that an IO APIC
based timer interrupt is in use.

Is it possible that this is a bug/race condition in the HpetDxe driver.

Calling the Timer AP to set timer period from 0 -> Non Zero enables
the timer.  There could be a pending timer interrupt before this 
transition that would need to be cleared.

Calling the Timer AP to set the timer period from Non Zero -> 0
disables the generation of timer interrupts.  We need to make
sure this disable action does not generate even one more
pending timer interrupt after the timer is disabled.  And
we need to make sure a timer interrupt does not enter the 
pending state in the logic that is disabling the timer.
We may need to always do an extra clear after the timer
interrupt is disabled in case a timer interrupt is asserted
during the disable logic.

I suspect this unit test use case is not commonly exercised.
Normal boots will go from 0 -> Non Zero period and may then
optionally change the timer period.  But going from
Non Zero -> 0 timer period is not likely used.

Seems like a good reason to add some Timer AP unit tests too.

Thanks,
 
Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray
> Sent: Wednesday, October 26, 2022 7:08 PM
> To: Tan, Dun <dun.tan@intel.com>; devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>
> Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg: Restore HpetTimer after CpuExceptionHandlerLib test
> 
> Reviewed-by: Ray Ni <ray.ni@intel.com>
> 
> > -----Original Message-----
> > From: Tan, Dun <dun.tan@intel.com>
> > Sent: Thursday, October 20, 2022 11:01 AM
> > To: devel@edk2.groups.io
> > Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar,
> > Rahul R <rahul.r.kumar@intel.com>
> > Subject: [PATCH] UefiCpuPkg: Restore HpetTimer after
> > CpuExceptionHandlerLib test
> >
> > Disable/Restore HpetTimer before and after running the Dxe
> > CpuExceptionHandlerLib unit test module. During the UnitTest, a
> > new Idt is initialized for the test. There is no handler for timer
> > intrrupt in this new idt. After the test module, HpetTimer does
> > not work any more since the comparator value register and main
> > counter value register for timer does not match. To fix this issue,
> > disable/restore HpetTimer before and after Unit Test if HpetTimer
> > driver has been dispatched. Besides, send Apic EOI before restore
> > HpetTimer.
> >
> > Signed-off-by: Dun Tan <dun.tan@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Rahul Kumar <rahul1.kumar@intel.com>
> > ---
> >
> > UefiCpuPkg/Library/CpuExceptionHandlerLib/UnitTest/DxeCpuExceptionHan
> > dlerLibUnitTest.inf |  2 ++
> >
> > UefiCpuPkg/Library/CpuExceptionHandlerLib/UnitTest/DxeCpuExceptionHan
> > dlerUnitTest.c      | 33 ++++++++++++++++++++++++++++++++-
> >  2 files changed, 34 insertions(+), 1 deletion(-)
> >
> > diff --git
> > a/UefiCpuPkg/Library/CpuExceptionHandlerLib/UnitTest/DxeCpuExceptionH
> > andlerLibUnitTest.inf
> > b/UefiCpuPkg/Library/CpuExceptionHandlerLib/UnitTest/DxeCpuExceptionH
> > andlerLibUnitTest.inf
> > index e3dbe7b9ab..24f905936c 100644
> > ---
> > a/UefiCpuPkg/Library/CpuExceptionHandlerLib/UnitTest/DxeCpuExceptionH
> > andlerLibUnitTest.inf
> > +++
> > b/UefiCpuPkg/Library/CpuExceptionHandlerLib/UnitTest/DxeCpuExceptionH
> > andlerLibUnitTest.inf
> > @@ -43,6 +43,7 @@
> >    HobLib
> >    UefiBootServicesTableLib
> >    CpuPageTableLib
> > +  LocalApicLib
> >
> >  [Guids]
> >    gEfiHobMemoryAllocStackGuid
> > @@ -53,6 +54,7 @@
> >
> >  [Protocols]
> >    gEfiMpServiceProtocolGuid
> > +  gEfiTimerArchProtocolGuid
> >
> >  [Depex]
> >    gEfiMpServiceProtocolGuid
> > diff --git
> > a/UefiCpuPkg/Library/CpuExceptionHandlerLib/UnitTest/DxeCpuExceptionH
> > andlerUnitTest.c
> > b/UefiCpuPkg/Library/CpuExceptionHandlerLib/UnitTest/DxeCpuExceptionH
> > andlerUnitTest.c
> > index 917fc549bf..045f39fa00 100644
> > ---
> > a/UefiCpuPkg/Library/CpuExceptionHandlerLib/UnitTest/DxeCpuExceptionH
> > andlerUnitTest.c
> > +++
> > b/UefiCpuPkg/Library/CpuExceptionHandlerLib/UnitTest/DxeCpuExceptionH
> > andlerUnitTest.c
> > @@ -8,6 +8,8 @@
> >
> >  #include "CpuExceptionHandlerTest.h"
> >  #include <Library/UefiBootServicesTableLib.h>
> > +#include <Library/LocalApicLib.h>
> > +#include <Protocol/Timer.h>
> >
> >  /**
> >    Initialize Bsp Idt with a new Idt table and return the IA32_DESCRIPTOR
> > buffer.
> > @@ -162,8 +164,12 @@ CpuExceptionHandlerTestEntry (
> >  {
> >    EFI_STATUS                  Status;
> >    UNIT_TEST_FRAMEWORK_HANDLE  Framework;
> > +  EFI_TIMER_ARCH_PROTOCOL     *TimerArchProtocol;
> > +  UINT64                      TimerPeriod;
> >
> > -  Framework = NULL;
> > +  Framework         = NULL;
> > +  TimerArchProtocol = NULL;
> > +  TimerPeriod       = 0;
> >
> >    DEBUG ((DEBUG_INFO, "%a v%a\n", UNIT_TEST_APP_NAME,
> > UNIT_TEST_APP_VERSION));
> >
> > @@ -182,11 +188,36 @@ CpuExceptionHandlerTestEntry (
> >      goto EXIT;
> >    }
> >
> > +  //
> > +  // If HpetTimer driver has been dispatched, disable HpetTimer before Unit
> > Test.
> > +  //
> > +  gBS->LocateProtocol (&gEfiTimerArchProtocolGuid, NULL, (VOID
> > **)&TimerArchProtocol);
> > +  if (TimerArchProtocol != NULL) {
> > +    Status = TimerArchProtocol->GetTimerPeriod (TimerArchProtocol,
> > &TimerPeriod);
> > +    ASSERT_EFI_ERROR (Status);
> > +    if (TimerPeriod > 0) {
> > +      DEBUG ((DEBUG_INFO, "HpetTimer has been dispatched. Disable
> > HpetTimer.\n"));
> > +      Status = TimerArchProtocol->SetTimerPeriod (TimerArchProtocol, 0);
> > +      ASSERT_EFI_ERROR (Status);
> > +    }
> > +  }
> > +
> >    //
> >    // Execute the tests.
> >    //
> >    Status = RunAllTestSuites (Framework);
> >
> > +  //
> > +  // Restore HpetTimer after Unit Test.
> > +  // Send APIC EOI before SetTimerPeriod.
> > +  //
> > +  if ((TimerArchProtocol != NULL) && (TimerPeriod > 0)) {
> > +    DEBUG ((DEBUG_INFO, "Restore HpetTimer after
> > DxeCpuExceptionHandlerLib UnitTest.\n"));
> > +    SendApicEoi ();
> > +    Status = TimerArchProtocol->SetTimerPeriod (TimerArchProtocol,
> > TimerPeriod);
> > +    ASSERT_EFI_ERROR (Status);
> > +  }
> > +
> >  EXIT:
> >    if (Framework) {
> >      FreeUnitTestFramework (Framework);
> > --
> > 2.31.1.windows.1
> 
> 
> 
> 
>