| Message ID | 1520901090-96694-2-git-send-email-heyi.guo@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | ArmPkg/TimerDxe: Add ISB for timer compare value reload | expand |
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... _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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. Thanks and regards, Heyi > > Thanks, > > M. > -- > Jazz is not dead. It just smells funny... _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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. Thanks, M. -- Jazz is not dead, it just smell funny. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 15 March 2018 at 07:11, Guo Heyi <heyi.guo@linaro.org> wrote: > 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. > I'm not sure. IIUC, the KVM issue that required this has been fixed long ago, and I don't want to carry this forever. Marc? > 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. > No idea, sorry. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 15/03/18 07:30, Ard Biesheuvel wrote: > On 15 March 2018 at 07:11, Guo Heyi <heyi.guo@linaro.org> wrote: >> 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. >> > > I'm not sure. IIUC, the KVM issue that required this has been fixed > long ago, and I don't want to carry this forever. Marc? This has been fixed quite a while ago: commit f120cd6533d21075ab103ae6c225b1697853660d Author: Marc Zyngier <marc.zyngier@arm.com> Date: Mon Jun 23 13:59:13 2014 +0100 KVM: arm/arm64: timer: Allow the timer to control the active state In order to remove the crude hack where we sneak the masked bit into the timer's control register, make use of the phys_irq_map API control the active state of the interrupt. This causes some limited changes to allow for potential error propagation. Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >> 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. >> > > No idea, sorry. Me neither. You'd have to dump the control register before and after, and work out what is being changed exactly. Thanks, M. -- Jazz is not dead. It just smells funny... _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 15 March 2018 at 09:40, Marc Zyngier <marc.zyngier@arm.com> wrote: > On 15/03/18 07:30, Ard Biesheuvel wrote: >> On 15 March 2018 at 07:11, Guo Heyi <heyi.guo@linaro.org> wrote: >>> 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. >>> >> >> I'm not sure. IIUC, the KVM issue that required this has been fixed >> long ago, and I don't want to carry this forever. Marc? > > This has been fixed quite a while ago: > > commit f120cd6533d21075ab103ae6c225b1697853660d > Author: Marc Zyngier <marc.zyngier@arm.com> > Date: Mon Jun 23 13:59:13 2014 +0100 > > KVM: arm/arm64: timer: Allow the timer to control the active state > > In order to remove the crude hack where we sneak the masked bit > into the timer's control register, make use of the phys_irq_map > API control the active state of the interrupt. > > This causes some limited changes to allow for potential error > propagation. > > Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > Does this mean we can lose this as well? https://github.com/tianocore/edk2/blob/master/ArmPkg/Library/ArmGenericTimerVirtCounterLib/ArmGenericTimerVirtCounterLib.c#L31 >>> 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. >>> >> >> No idea, sorry. > > Me neither. You'd have to dump the control register before and after, > and work out what is being changed exactly. > > Thanks, > > M. > -- > Jazz is not dead. It just smells funny... _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 15/03/18 09:52, Ard Biesheuvel wrote: > On 15 March 2018 at 09:40, Marc Zyngier <marc.zyngier@arm.com> wrote: >> On 15/03/18 07:30, Ard Biesheuvel wrote: >>> On 15 March 2018 at 07:11, Guo Heyi <heyi.guo@linaro.org> wrote: >>>> 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. >>>> >>> >>> I'm not sure. IIUC, the KVM issue that required this has been fixed >>> long ago, and I don't want to carry this forever. Marc? >> >> This has been fixed quite a while ago: >> >> commit f120cd6533d21075ab103ae6c225b1697853660d >> Author: Marc Zyngier <marc.zyngier@arm.com> >> Date: Mon Jun 23 13:59:13 2014 +0100 >> >> KVM: arm/arm64: timer: Allow the timer to control the active state >> >> In order to remove the crude hack where we sneak the masked bit >> into the timer's control register, make use of the phys_irq_map >> API control the active state of the interrupt. >> >> This causes some limited changes to allow for potential error >> propagation. >> >> Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >> > > Does this mean we can lose this as well? > > https://github.com/tianocore/edk2/blob/master/ArmPkg/Library/ArmGenericTimerVirtCounterLib/ArmGenericTimerVirtCounterLib.c#L31 Indeed, you should be able to remove this as well. Thanks, M. -- Jazz is not dead. It just smells funny... _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 (); }