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

[Marc Zyngier <marc.zyngier@arm.com>]

On 13/03/18 00:31, Heyi Guo 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>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> ---
> 
> Notes:
>     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..32abee8726a0 100644
> --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> @@ -337,6 +337,7 @@ TimerInterruptHandler (
>  
>      // Set next compare value
>      ArmGenericTimerSetCompareVal (CompareValue);
> +    ArmInstructionSynchronizationBarrier ();
>      ArmGenericTimerEnableTimer ();
>    }

Sorry for being pedantic here, but it would make more sense if ISB was
placed after the enabling of the timer. Otherwise, you only force the
update of the comparator. But on the other hand, the timer was never
disabled the first place, in which case you'd wonder why you're trying
to enable it again.

So either you leave the ISB here and remove the enable call, or move the
ISB after the enable.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...