<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=us-ascii"> <style type="text/css" style="display:none;"> P {margin-top:0;margin-bottom:0;} </style> </head> <body dir="ltr"> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> Marc,</div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> <br> </div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> In the document ARM062-1010708621-30 (AArch64 Programmer's Guides Generic Timer), towards the end of section 3.4 it says: "When writing an interrupt handler for the timers, it is important that software clears the interrupt before deactivating the interrupt in the GIC. Otherwise, the GIC will re-signal the same interrupt again."</div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> <br> </div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> My change was in accordance with this. We only clear the interrupt when we update the compare value but were signaling EOI before that going against the guidance in the document.</div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> <br> </div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> Thanks</div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> Ashish</div> <div id="appendonsend"></div> <hr style="display:inline-block;width:98%" tabindex="-1"> <div id="divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" style="font-size:11pt" color="#000000"><b>From:</b> Marc Zyngier <maz@kernel.org><br> <b>Sent:</b> Tuesday, October 12, 2021 2:57 AM<br> <b>To:</b> Ard Biesheuvel <ardb@kernel.org>; Ashish Singhal <ashishsingha@nvidia.com><br> <b>Cc:</b> edk2-devel-groups-io <devel@edk2.groups.io>; Leif Lindholm <leif@nuviainc.com>; Ard Biesheuvel <ardb+tianocore@kernel.org><br> <b>Subject:</b> Re: [PATCH v2] ArmPkg/TimerDxe: Delay End Of Interrupt Signal</font> <div> </div> </div> <div class="BodyFragment"><font size="2"><span style="font-size:11pt;"> <div class="PlainText">External email: Use caution opening links or attachments<br> <br> <br> On Mon, 11 Oct 2021 23:24:38 +0100,<br> Ard Biesheuvel <ardb@kernel.org> wrote:<br> ><br> > (+ Marc)<br> ><br> > On Mon, 11 Oct 2021 at 23:40, Ashish Singhal <ashishsingha@nvidia.com> wrote:<br> > ><br> > > In an interrupt handler for the timers, it is important that<br> > > software clears the interrupt before deactivating the interrupt<br> > > in the GIC. Otherwise the GIC will re-signal the same interrupt<br> > > again.<br> > ><br> > > Signed-off-by: Ashish Singhal <ashishsingha@nvidia.com><br> ><br> > Please don't spam us with almost identical versions of the same patch<br> > without even documenting what the difference is.<br> ><br> ><br> > > ---<br> > > ArmPkg/Drivers/TimerDxe/TimerDxe.c | 9 +++++----<br> > > 1 file changed, 5 insertions(+), 4 deletions(-)<br> > ><br> > > diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c b/ArmPkg/Drivers/TimerDxe/TimerDxe.c<br> > > index 0370620fae..46e5bbf287 100644<br> > > --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c<br> > > +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c<br> > > @@ -300,10 +300,6 @@ TimerInterruptHandler (<br> > > //<br> > > OriginalTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL);<br> > ><br> > > - // Signal end of interrupt early to help avoid losing subsequent ticks<br> > > - // from long duration handlers<br> > > - gInterrupt->EndOfInterrupt (gInterrupt, Source);<br> > > -<br> > > // Check if the timer interrupt is active<br> > > if ((ArmGenericTimerGetTimerCtrlReg () ) & ARM_ARCH_TIMER_ISTATUS) {<br> > ><br> > > @@ -335,6 +331,11 @@ TimerInterruptHandler (<br> > > ArmInstructionSynchronizationBarrier ();<br> > > }<br> > ><br> > > + // In an interrupt handler for the timers, it is important that software clears the interrupt<br> > > + // before deactivating the interrupt in the GIC. Otherwise the GIC will re-signal the same<br> > > + // interrupt again.<br> > > + gInterrupt->EndOfInterrupt (gInterrupt, Source);<br> > > +<br> > > gBS->RestoreTPL (OriginalTPL);<br> > > }<br> > ><br> <br> This isn't a requirement if you haven't re-enabled interrupts in<br> PSTATE (and the TPL being raised seems to indicate that interrupts are<br> actively masked here).<br> <br> The timer is a level interrupt, and lowering the level takes time. If<br> your GIC implementation is good, it will notice that the lowering of<br> the level quickly, before you can reach the point where you re-enable<br> interrupts. If it is slow (or badly emulated if this is actually done<br> in a hypervisor), you'll get a spurious interrupt.<br> <br> In any case, moving the EOI around doesn't make things any better. You<br> are just moving the goal post, without additional guarantees that the<br> level has been retired.<br> <br> As a consequence, I don't think this patch makes much sense.<br> <br> M.<br> <br> --<br> Without deviation from the norm, progress is not possible.<br> </div> </span></font></div> </body> </html> <div width="1" style="color:white;clear:both">_._,_._,_</div> <hr> Groups.io Links:<p> You receive all messages sent to this group. <p> <a target="_blank" href="https://edk2.groups.io/g/devel/message/81831">View/Reply Online (#81831)</a> | | <a target="_blank" href="https://groups.io/mt/86248479/1813853">Mute This Topic</a> | <a href="https://edk2.groups.io/g/devel/post">New Topic</a><br> <a href="https://edk2.groups.io/g/devel/editsub/1813853">Your Subscription</a> | <a href="mailto:devel+owner@edk2.groups.io">Contact Group Owner</a> | <a href="https://edk2.groups.io/g/devel/unsub">Unsubscribe</a> [edk2-devel-archive@redhat.com]<br> <div width="1" style="color:white;clear:both">_._,_._,_</div>