[Patch] PcAtChipsetPkg/HpetTimerDxe: Fix race condition in SetTimerPeriod()

[Patch] PcAtChipsetPkg/HpetTimerDxe: Fix race condition in SetTimerPeriod()

[Michael Kinney]

https://bugzilla.tianocore.org/show_bug.cgi?id=182

The function TimerDriverSetTimerPeriod() disables the HPET timer
while the HPET timer HW is reprogrammed with a new timer period.
However, the MMIO write to disable the HPET timer HW can be
delayed and an HPET timer interrupt may be processed in the middle
of reprogramming the HPET timer HW and this may produced unexpected
results.

The fix is to raise TPL to TPL_HIGH_LEVEL in
TimerDriverSetTimerPeriod() during the time the HPET timer HW is
reprogrammed.  This guarantees that no timer interrupts are
processed during reprogramming.

The TimerDriverGenerateSoftInterrupt() function in this same
driver also raises TPL to TPL_HIGH_LEVEL, so this fix matches
the logic that is already used in another function for the same
reason.

Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Michael Kinney <michael.d.kinney@intel.com>
---
 PcAtChipsetPkg/HpetTimerDxe/HpetTimer.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/PcAtChipsetPkg/HpetTimerDxe/HpetTimer.c b/PcAtChipsetPkg/HpetTimerDxe/HpetTimer.c
index 0ed8743..c62c3a9 100644
--- a/PcAtChipsetPkg/HpetTimerDxe/HpetTimer.c
+++ b/PcAtChipsetPkg/HpetTimerDxe/HpetTimer.c
@@ -492,11 +492,17 @@ TimerDriverSetTimerPeriod (
   IN UINT64                   TimerPeriod
   )
 {
+  EFI_TPL                        Tpl;
   UINT64                         MainCounter;
   UINT64                         Delta;
   UINT64                         CurrentComparator;
   HPET_TIMER_MSI_ROUTE_REGISTER  HpetTimerMsiRoute;
-  
+
+  //
+  // Disable interrupts
+  //
+  Tpl = gBS->RaiseTPL (TPL_HIGH_LEVEL);
+
   //
   // Disable HPET timer when adjusting the timer period
   //
@@ -616,7 +622,12 @@ TimerDriverSetTimerPeriod (
   // is disabled.
   //
   HpetEnable (TRUE);
-  
+
+  //
+  // Restore interrupts
+  //
+  gBS->RestoreTPL (Tpl);
+
   return EFI_SUCCESS;
 }
 
-- 
2.6.3.windows.1

Re: [Patch] PcAtChipsetPkg/HpetTimerDxe: Fix race condition in SetTimerPeriod()

[Ni, Ruiyu]

Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
>-----Original Message-----
>From: Kinney, Michael D
>Sent: Thursday, October 27, 2016 6:31 AM
>To: edk2-devel@lists.01.org
>Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
>Subject: [Patch] PcAtChipsetPkg/HpetTimerDxe: Fix race condition in SetTimerPeriod()
>
>https://bugzilla.tianocore.org/show_bug.cgi?id=182
>
>The function TimerDriverSetTimerPeriod() disables the HPET timer
>while the HPET timer HW is reprogrammed with a new timer period.
>However, the MMIO write to disable the HPET timer HW can be
>delayed and an HPET timer interrupt may be processed in the middle
>of reprogramming the HPET timer HW and this may produced unexpected
>results.
>
>The fix is to raise TPL to TPL_HIGH_LEVEL in
>TimerDriverSetTimerPeriod() during the time the HPET timer HW is
>reprogrammed.  This guarantees that no timer interrupts are
>processed during reprogramming.
>
>The TimerDriverGenerateSoftInterrupt() function in this same
>driver also raises TPL to TPL_HIGH_LEVEL, so this fix matches
>the logic that is already used in another function for the same
>reason.
>
>Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>Contributed-under: TianoCore Contribution Agreement 1.0
>Signed-off-by: Michael Kinney <michael.d.kinney@intel.com>
>---
> PcAtChipsetPkg/HpetTimerDxe/HpetTimer.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
>diff --git a/PcAtChipsetPkg/HpetTimerDxe/HpetTimer.c b/PcAtChipsetPkg/HpetTimerDxe/HpetTimer.c
>index 0ed8743..c62c3a9 100644
>--- a/PcAtChipsetPkg/HpetTimerDxe/HpetTimer.c
>+++ b/PcAtChipsetPkg/HpetTimerDxe/HpetTimer.c
>@@ -492,11 +492,17 @@ TimerDriverSetTimerPeriod (
>   IN UINT64                   TimerPeriod
>   )
> {
>+  EFI_TPL                        Tpl;
>   UINT64                         MainCounter;
>   UINT64                         Delta;
>   UINT64                         CurrentComparator;
>   HPET_TIMER_MSI_ROUTE_REGISTER  HpetTimerMsiRoute;
>-
>+
>+  //
>+  // Disable interrupts
>+  //
>+  Tpl = gBS->RaiseTPL (TPL_HIGH_LEVEL);
>+
>   //
>   // Disable HPET timer when adjusting the timer period
>   //
>@@ -616,7 +622,12 @@ TimerDriverSetTimerPeriod (
>   // is disabled.
>   //
>   HpetEnable (TRUE);
>-
>+
>+  //
>+  // Restore interrupts
>+  //
>+  gBS->RestoreTPL (Tpl);
>+
>   return EFI_SUCCESS;
> }
>
>--
>2.6.3.windows.1