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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 14 March 2018 at 07:45, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On Wed, 14 Mar 2018 00:25:09 +0000,
> Guo Heyi wrote:
>>
>> On Tue, Mar 13, 2018 at 09:33:33AM +0000, Marc Zyngier wrote:
>> > 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.
>> Yes, I also had such question and hesitated at this place :)
>> >
>> > So either you leave the ISB here and remove the enable call, or move the
>> > ISB after the enable.
>>
>> If we are going to remove the enable call, is it better to be changed in a
>> separate patch? It seems not related with adding ISB, though it is only a
>> one-line change.
>
> I guess a separate patch doesn't hurt, but that's for Ard and Leif to
> decide.
>

Yes, please.