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

[Guo Heyi <heyi.guo@linaro.org>]

Hi Marc and Ard,

I found the timer re-enable code was added by Ard for special reason:

commit b1a633434ddc5fc28de817debd963f7845fb78c7
Author: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Date:   Thu Sep 18 21:16:47 2014 +0000

    ArmPkg/TimerDxe: add workaround for KVM timer interrupt handling

So this line of code cannot be removed and I will add an ISB after enabling
timer.

Another strange thing is that we got lots of "Spurious GIC interrupt" error
messages when timer enable code was removed, though at last it could boot to EFI
shell. Please let me know if you have any idea about the possible reason of this
issue.

Regards,
Heyi

On Wed, Mar 14, 2018 at 02:50:41PM +0000, Ard Biesheuvel wrote:
> 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.