[edk2-devel] [PATCH v2] ArmPkg/TimerDxe: Delay End Of Interrupt Signal

Thu Oct 14 04:54:18 UTC 2021

Hello Jeff,

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.

Thanks
Ashish
________________________________
From: fanjianfeng at byosoft.com.cn <fanjianfeng at byosoft.com.cn>
Sent: Tuesday, October 12, 2021 8:32 PM
To: devel at edk2.groups.io <devel at edk2.groups.io>; Ashish Singhal <ashishsingha at nvidia.com>; Marc Zyngier <maz at kernel.org>; Ard Biesheuvel <ardb at kernel.org>; Shanker Donthineni <sdonthineni at nvidia.com>
Cc: devel at edk2.groups.io <devel at edk2.groups.io>; Leif Lindholm <leif at nuviainc.com>
Subject: Re: Re: [edk2-devel] [PATCH v2] ArmPkg/TimerDxe: Delay End Of Interrupt Signal

External email: Use caution opening links or attachments

OVMF did a similare change on Time Driver, please refer to https://github.com/tianocore/edk2/commit/239b50a863704f7960525799eda82de061c7c458

I do not think this will be apply for ArmPkg/TimerDxe.

If one real issue happened on platform, it seems that interrupt was reenabled by reigstered timer event functions.

Jeff

From: Ashish Singhal via groups.io<mailto:ashishsingha=nvidia.com at groups.io>
Date: 2021-10-12 23:38
To: Marc Zyngier<mailto:maz at kernel.org>; Ard Biesheuvel<mailto:ardb at kernel.org>; Shanker Donthineni<mailto:sdonthineni at nvidia.com>
CC: edk2-devel-groups-io<mailto:devel at edk2.groups.io>; Leif Lindholm<mailto:leif at nuviainc.com>; Ard Biesheuvel<mailto:ardb+tianocore at kernel.org>
Subject: Re: [edk2-devel] [PATCH v2] ArmPkg/TimerDxe: Delay End Of Interrupt Signal
+ Shaker

Get Outlook for iOS<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>
________________________________
From: Ashish Singhal <ashishsingha at nvidia.com>
Sent: Tuesday, October 12, 2021 8:56:58 AM
To: Marc Zyngier <maz at kernel.org>; Ard Biesheuvel <ardb at kernel.org>
Cc: edk2-devel-groups-io <devel at edk2.groups.io>; Leif Lindholm <leif at nuviainc.com>; Ard Biesheuvel <ardb+tianocore at kernel.org>
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 <maz at kernel.org>
Sent: Tuesday, October 12, 2021 2:57 AM
To: Ard Biesheuvel <ardb at kernel.org>; Ashish Singhal <ashishsingha at nvidia.com>
Cc: edk2-devel-groups-io <devel at edk2.groups.io>; Leif Lindholm <leif at nuviainc.com>; Ard Biesheuvel <ardb+tianocore at kernel.org>
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 <ardb at kernel.org> wrote:
>
> (+ Marc)
>
> On Mon, 11 Oct 2021 at 23:40, Ashish Singhal <ashishsingha at nvidia.com> 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 <ashishsingha at nvidia.com>
>
> 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.

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#81938): https://edk2.groups.io/g/devel/message/81938
Mute This Topic: https://groups.io/mt/86248479/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20211014/3bdd31de/attachment.htm>