+ Shaker Get Outlook for iOS ________________________________ From: Ashish Singhal Sent: Tuesday, October 12, 2021 8:56:58 AM To: Marc Zyngier ; Ard Biesheuvel Cc: edk2-devel-groups-io ; Leif Lindholm ; Ard Biesheuvel Subject: Re: [PATCH v2] ArmPkg/TimerDxe: Delay End Of Interrupt Signal Marc, 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." 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. Thanks Ashish ________________________________ From: Marc Zyngier Sent: Tuesday, October 12, 2021 2:57 AM To: Ard Biesheuvel ; Ashish Singhal Cc: edk2-devel-groups-io ; Leif Lindholm ; Ard Biesheuvel Subject: Re: [PATCH v2] ArmPkg/TimerDxe: Delay End Of Interrupt Signal External email: Use caution opening links or attachments On Mon, 11 Oct 2021 23:24:38 +0100, Ard Biesheuvel wrote: > > (+ Marc) > > On Mon, 11 Oct 2021 at 23:40, Ashish Singhal wrote: > > > > In 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. > > > > Signed-off-by: Ashish Singhal > > Please don't spam us with almost identical versions of the same patch > without even documenting what the difference is. > > > > --- > > ArmPkg/Drivers/TimerDxe/TimerDxe.c | 9 +++++---- > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c b/ArmPkg/Drivers/TimerDxe/TimerDxe.c > > index 0370620fae..46e5bbf287 100644 > > --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c > > +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c > > @@ -300,10 +300,6 @@ TimerInterruptHandler ( > > // > > OriginalTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL); > > > > - // Signal end of interrupt early to help avoid losing subsequent ticks > > - // from long duration handlers > > - gInterrupt->EndOfInterrupt (gInterrupt, Source); > > - > > // Check if the timer interrupt is active > > if ((ArmGenericTimerGetTimerCtrlReg () ) & ARM_ARCH_TIMER_ISTATUS) { > > > > @@ -335,6 +331,11 @@ TimerInterruptHandler ( > > ArmInstructionSynchronizationBarrier (); > > } > > > > + // In 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. > > + gInterrupt->EndOfInterrupt (gInterrupt, Source); > > + > > gBS->RestoreTPL (OriginalTPL); > > } > > This isn't a requirement if you haven't re-enabled interrupts in PSTATE (and the TPL being raised seems to indicate that interrupts are actively masked here). The timer is a level interrupt, and lowering the level takes time. If your GIC implementation is good, it will notice that the lowering of the level quickly, before you can reach the point where you re-enable interrupts. If it is slow (or badly emulated if this is actually done in a hypervisor), you'll get a spurious interrupt. In any case, moving the EOI around doesn't make things any better. You are just moving the goal post, without additional guarantees that the level has been retired. As a consequence, I don't think this patch makes much sense. M. -- Without deviation from the norm, progress is not possible.