[PATCH v3 0/1] ArmPkg/TimerDxe: Add ISB for timer compare value reload

[PATCH v3 0/1] ArmPkg/TimerDxe: Add ISB for timer compare value reload

[Heyi Guo]

After rebasing to edk2 commit 5e3719a, we found D05 would hang after printing a
lot of "Spurious interrupt" messages. The issue would gone away if we restored
the removal of "enable interrupt source":
  gInterrupt->EnableInterruptSource (gInterrupt, Source);

It can also be fixed if we add a "ISB" after reloading timer compare value, and
we agree that it makes sense to do that.

v3:
- Implement 1 comment from Marc.

Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Marc Zyngier <marc.zyngier@arm.com>


Heyi Guo (1):
  ArmPkg/TimerDxe: Add ISB for timer compare value reload

 ArmPkg/Drivers/TimerDxe/TimerDxe.c | 1 +
 1 file changed, 1 insertion(+)

-- 
2.7.4

[PATCH v3 1/1] ArmPkg/TimerDxe: Add ISB for timer compare value reload

[Heyi Guo]

If timer interrupt is level sensitive, reloading timer compare
register has a side effect of clearing GIC pending status, so a "ISB"
is needed to make sure this instruction is executed before enabling
CPU IRQ, or else we may get spurious timer interrupts.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
Signed-off-by: Yi Li <phoenix.liyi@huawei.com>
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Marc Zyngier <marc.zyngier@arm.com>
---

Notes:
    v3:
    - Move ISB after enabling timer [Marc]
    
    v2:
    - Use ISB instead of DSB [Marc]
    - Update commit message accordingly.

 ArmPkg/Drivers/TimerDxe/TimerDxe.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
index 33d7c922221f..a3202fa056f3 100644
--- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c
+++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
@@ -338,6 +338,7 @@ TimerInterruptHandler (
     // Set next compare value
     ArmGenericTimerSetCompareVal (CompareValue);
     ArmGenericTimerEnableTimer ();
+    ArmInstructionSynchronizationBarrier ();
   }
 
   gBS->RestoreTPL (OriginalTPL);
-- 
2.7.4

Re: [PATCH v3 1/1] ArmPkg/TimerDxe: Add ISB for timer compare value reload

[Ard Biesheuvel]

On 15 March 2018 at 07:17, Heyi Guo <heyi.guo@linaro.org> wrote:
> If timer interrupt is level sensitive, reloading timer compare
> register has a side effect of clearing GIC pending status, so a "ISB"
> is needed to make sure this instruction is executed before enabling
> CPU IRQ, or else we may get spurious timer interrupts.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
> Signed-off-by: Yi Li <phoenix.liyi@huawei.com>
> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Marc Zyngier <marc.zyngier@arm.com>


Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Pushed as ac9b530e6b47

Thanks

> ---
>
> Notes:
>     v3:
>     - Move ISB after enabling timer [Marc]
>
>     v2:
>     - Use ISB instead of DSB [Marc]
>     - Update commit message accordingly.
>
>  ArmPkg/Drivers/TimerDxe/TimerDxe.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> index 33d7c922221f..a3202fa056f3 100644
> --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> @@ -338,6 +338,7 @@ TimerInterruptHandler (
>      // Set next compare value
>      ArmGenericTimerSetCompareVal (CompareValue);
>      ArmGenericTimerEnableTimer ();
> +    ArmInstructionSynchronizationBarrier ();
>    }
>
>    gBS->RestoreTPL (OriginalTPL);
> --
> 2.7.4
>