[edk2-devel] [PATCH] UefiCpuPkg: Restore HpetTimer after CpuExceptionHandlerLib test
duntan
dun.tan at intel.com
Thu Oct 27 03:08:34 UTC 2022
Hi Ray, Mike,
The 2nd option works in qsp boot.
Thanks,
Dun
-----Original Message-----
From: Ni, Ray <ray.ni at intel.com>
Sent: Thursday, October 27, 2022 11:03 AM
To: devel at edk2.groups.io; Ni, Ray <ray.ni at intel.com>; Kinney, Michael D <michael.d.kinney at intel.com>; Tan, Dun <dun.tan at intel.com>
Cc: Dong, Eric <eric.dong at intel.com>; Kumar, Rahul R <rahul.r.kumar at intel.com>
Subject: RE: [edk2-devel] [PATCH] UefiCpuPkg: Restore HpetTimer after CpuExceptionHandlerLib test
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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Ni, Ray
> Sent: Thursday, October 27, 2022 10:33 AM
> To: Kinney, Michael D <michael.d.kinney at intel.com>;
> devel at edk2.groups.io; Tan, Dun <dun.tan at intel.com>
> Cc: Dong, Eric <eric.dong at intel.com>; Kumar, Rahul R
> <rahul.r.kumar at 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 at intel.com>
> > Sent: Thursday, October 27, 2022 10:26 AM
> > To: devel at edk2.groups.io; Ni, Ray <ray.ni at intel.com>; Tan, Dun
> > <dun.tan at intel.com>; Kinney, Michael D <michael.d.kinney at intel.com>
> > Cc: Dong, Eric <eric.dong at intel.com>; Kumar, Rahul R
> > <rahul.r.kumar at 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Ni,
> > Ray
> > > Sent: Wednesday, October 26, 2022 7:08 PM
> > > To: Tan, Dun <dun.tan at intel.com>; devel at edk2.groups.io
> > > Cc: Dong, Eric <eric.dong at intel.com>; Kumar, Rahul R
> > <rahul.r.kumar at intel.com>
> > > Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg: Restore HpetTimer
> > > after
> > CpuExceptionHandlerLib test
> > >
> > > Reviewed-by: Ray Ni <ray.ni at intel.com>
> > >
> > > > -----Original Message-----
> > > > From: Tan, Dun <dun.tan at intel.com>
> > > > Sent: Thursday, October 20, 2022 11:01 AM
> > > > To: devel at edk2.groups.io
> > > > Cc: Dong, Eric <eric.dong at intel.com>; Ni, Ray
> > > > <ray.ni at intel.com>;
> Kumar,
> > > > Rahul R <rahul.r.kumar at 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 at intel.com>
> > > > Cc: Eric Dong <eric.dong at intel.com>
> > > > Cc: Ray Ni <ray.ni at intel.com>
> > > > Cc: Rahul Kumar <rahul1.kumar at 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
> > >
> > >
> > >
> > >
> > >
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#95630): https://edk2.groups.io/g/devel/message/95630
Mute This Topic: https://groups.io/mt/94446624/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list