<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);"> Hello Jeff,</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 for the reference you provided of the change made by you. Leveraging a similar change resolves the problem 90 percent for me as I do not get the ISR interrupted for the most part because of another timer interrupt. However, even with your change, during the ISR there are few instructions during which IRQ is enabled and we may be interrupted by a timer interrupt during that time unless I am understanding it wrong.</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> fanjianfeng@byosoft.com.cn <fanjianfeng@byosoft.com.cn><br> <b>Sent:</b> Tuesday, October 12, 2021 8:32 PM<br> <b>To:</b> devel@edk2.groups.io <devel@edk2.groups.io>; Ashish Singhal <ashishsingha@nvidia.com>; Marc Zyngier <maz@kernel.org>; Ard Biesheuvel <ardb@kernel.org>; Shanker Donthineni <sdonthineni@nvidia.com><br> <b>Cc:</b> devel@edk2.groups.io <devel@edk2.groups.io>; Leif Lindholm <leif@nuviainc.com><br> <b>Subject:</b> Re: Re: [edk2-devel] [PATCH v2] ArmPkg/TimerDxe: Delay End Of Interrupt Signal</font> <div> </div> </div> <style>  </style> <div> <table bgcolor="#FFEB9C" border="1"> <tbody> <tr> <td><font face="verdana" color="black" size="1"><b>External email: Use caution opening links or attachments</b> </font></td> </tr> </tbody> </table> <br> <div> <div style=""><span style="color:rgb(0,0,0); background-color:rgba(0,0,0,0)">OVMF did a similare change on Time Driver, please refer to</span><span style="background-color:transparent"> </span><a href="https://github.com/tianocore/edk2/commit/239b50a863704f7960525799eda82de061c7c458" style="background-color:transparent">https://github.com/tianocore/edk2/commit/239b50a863704f7960525799eda82de061c7c458</a><span style="color:rgb(0,0,0); background-color:rgba(0,0,0,0)"> </span></div> <div style=""><span style="color:rgb(0,0,0); background-color:rgba(0,0,0,0)"><br> </span></div> <div style=""><span style="color:rgb(0,0,0); background-color:rgba(0,0,0,0)">I do not think this will be apply for ArmPkg/TimerDxe. </span></div> <div style=""><span style="color:rgb(0,0,0); background-color:rgba(0,0,0,0)"><br> </span></div> <div style=""><span style="color:rgb(0,0,0); background-color:rgba(0,0,0,0)">If one real issue happened on platform, it seems that interrupt was reenabled by reigstered timer event functions.</span></div> <div style=""><span style="color:rgb(0,0,0); background-color:rgba(0,0,0,0)"><br> </span></div> <div style="">Jeff</div> <blockquote style="margin-top:0px; margin-bottom:0px; margin-left:0.5em; margin-right:inherit"> <div> </div> <div style="border:none; border-top:solid #B5C4DF 1.0pt; padding:3.0pt 0cm 0cm 0cm"> <div style="padding-right:8px; padding-left:8px; font-size:12px; font-family:tahoma; color:#000000; background:#efefef; padding-bottom:8px; padding-top:8px"> <div><b>From:</b> <a href="mailto:ashishsingha=nvidia.com@groups.io">Ashish Singhal via groups.io</a></div> <div><b>Date:</b> 2021-10-12 23:38</div> <div><b>To:</b> <a href="mailto:maz@kernel.org">Marc Zyngier</a>; <a href="mailto:ardb@kernel.org"> Ard Biesheuvel</a>; <a href="mailto:sdonthineni@nvidia.com">Shanker Donthineni</a></div> <div><b>CC:</b> <a href="mailto:devel@edk2.groups.io">edk2-devel-groups-io</a>; <a href="mailto:leif@nuviainc.com"> Leif Lindholm</a>; <a href="mailto:ardb+tianocore@kernel.org">Ard Biesheuvel</a></div> <div><b>Subject:</b> Re: [edk2-devel] [PATCH v2] ArmPkg/TimerDxe: Delay End Of Interrupt Signal</div> </div> </div> <div> <div class="x_FoxDiv20211013095835688114"> <div dir="ltr"> <div></div> <div> <div> <div dir="ltr"></div> </div> <div id="x_ms-outlook-mobile-signature"> <div>+ Shaker</div> <div dir="ltr"><br> </div> Get <a href="https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Faka.ms%2Fo0ukef&data=04%7C01%7Cashishsingha%40nvidia.com%7Cd95be02a5bbd4f91f94b08d98df1bdd0%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637696892678162925%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=pzX87I2uF%2F%2Fl%2FG2tRpJc9WICona0FKJY%2BhdoplHpFHo%3D&reserved=0" originalsrc="https://aka.ms/o0ukef" shash="q/8A3A8rd2T7ABcdyVBRgFfnfYKAi5WZMFupFq2e8X+kV4GwpxfIBTPmrEzwrFW2hL0q823MMz3H/OammUBu/s2oBxwKS+wDG2Msco+IQd2zwDoZs7zyV/cHxun9bX8tx5x7G51rsGJ1avlPhGs2g0vOakkMl7ynU/P5iq9XchY="> Outlook for iOS</a></div> </div> </div> <hr tabindex="-1" style="display:inline-block; width:98%"> <div id="x_divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" color="#000000" style="font-size:11pt"><b>From:</b> Ashish Singhal <ashishsingha@nvidia.com><br> <b>Sent:</b> Tuesday, October 12, 2021 8:56:58 AM<br> <b>To:</b> Marc Zyngier <maz@kernel.org>; Ard Biesheuvel <ardb@kernel.org><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 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="x_x_appendonsend"></div> <hr tabindex="-1" style="display:inline-block; width:98%"> <div id="x_x_divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" color="#000000" style="font-size:11pt"><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="x_x_BodyFragment"><font size="2"><span style="font-size:11pt"> <div class="x_x_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> </div> </div> </div> </blockquote> </div> </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/81938">View/Reply Online (#81938)</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>