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

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

TimerDriverSetTimerPeriod() {
  Tpl = gBS->RaiseTPL (TPL_HIGH_LEVEL);
  HpetEnable (FALSE);


Mike,
In above code, it's possible that timer interrupt happens after TPL is HIGH but before
HPET hardware is stopped.
When that happens, EOI sending logic in the timer interrupt handler function doesn't run.

So, there might be two options to fix the issue:
1. Send EOI again
  Tpl = gBS->RaiseTPL (TPL_HIGH_LEVEL); // CLI
  HpetEnable (FALSE); // timer interrupt might happen before HPET is really disabled.
  SendApicEoi ();

2. Disable HPET with interrupt enabled.
  HpetEnable (FALSE);
  Tpl = gBS->RaiseTPL (TPL_HIGH_LEVEL); // CLI

Dun is testing the 2nd option.

Thanks,
Ray

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray
> Sent: Thursday, October 27, 2022 10:33 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io;
> Tan, Dun <dun.tan@intel.com>
> 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
> 
> Mike,
> I agree there might be a bug in HPET timer driver where the EOI should be
> issued within that driver.
> Original idea was to avoid changing a driver used heavily in production
> firmware
> due to a new unit test failure and that test scenario is not common in
> production
> firmware.
> 
> We would like to do more analysis on the impact if sending EOI when
> disabling timer
> and then move the EOI sending from the test code to HPET timer.
> 
> Thanks,
> Ray
> 
> > -----Original Message-----
> > From: Kinney, Michael D <michael.d.kinney@intel.com>
> > Sent: Thursday, October 27, 2022 10:26 AM
> > To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Tan, Dun
> > <dun.tan@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> > 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
> >
> > 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
> > >
> > >
> > >
> > >
> > >
> 
> 
> 
> 
>