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

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

[Ashish Singhal]

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@nvidia.com>
---
 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);
 }
 
-- 
2.17.1

Re: [PATCH v2] ArmPkg/TimerDxe: Delay End Of Interrupt Signal

[Ard Biesheuvel]

(+ Marc)

On Mon, 11 Oct 2021 at 23:40, Ashish Singhal <ashishsingha@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@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);
>  }
>
> --
> 2.17.1
>

Re: [PATCH v2] ArmPkg/TimerDxe: Delay End Of Interrupt Signal

[Ashish Singhal]

[-- Attachment #1: Type: text/plain, Size: 3651 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 5990 bytes --]

Re: [PATCH v2] ArmPkg/TimerDxe: Delay End Of Interrupt Signal

[Ashish Singhal]

[-- Attachment #1: Type: text/plain, Size: 4104 bytes --]

+ Shaker

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

[-- Attachment #2: Type: text/html, Size: 6848 bytes --]

Re: [PATCH v2] ArmPkg/TimerDxe: Delay End Of Interrupt Signal

[Ashish Singhal]

From: Marc Zyngier <maz@kernel.org>
Sent: Tuesday, October 12, 2021 9:44 AM
To: Ashish Singhal <ashishsingha@nvidia.com>
Cc: Ard Biesheuvel <ardb@kernel.org>; edk2-devel-groups-io <devel@edk2.groups.io>; Leif Lindholm <leif@nuviainc.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>
Subject: Re: [PATCH v2] ArmPkg/TimerDxe: Delay End Of Interrupt Signal 
 
External email: Use caution opening links or attachments


Ashish,

Please don't top post, and please use plain text.

On Tue, 12 Oct 2021 15:56:58 +0100,
Ashish Singhal <ashishsingha@nvidia.com> wrote:
>
> 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."

This document is a waste of valuable bits, unfortunately, and isn't an
architecture reference.

> 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.

There is no such requirement in the GIC architecture, as it makes no
guarantee on how much time it takes for a change of level to be
observed. Given that, this change is pretty much immaterial.

        M.

--
Without deviation from the norm, progress is not possible.

Marc,

What do you suggest should be the proper fix for getting timer interrupts even when ISTATUS bit is not set? Should we ignore them the way it is in current implementation? I am OK to file a bug for this if you think that is a better way to discuss this.

Shanker,

Any thoughts on this?

Thanks
Ashish

Re: [PATCH v2] ArmPkg/TimerDxe: Delay End Of Interrupt Signal

[Ashish Singhal]

From: Marc Zyngier <maz@kernel.org>
Sent: Tuesday, October 12, 2021 10:27 AM
To: Ashish Singhal <ashishsingha@nvidia.com>
Cc: Shanker Donthineni <sdonthineni@nvidia.com>; Ard Biesheuvel <ardb@kernel.org>; edk2-devel-groups-io <devel@edk2.groups.io>; Leif Lindholm <leif@nuviainc.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>
Subject: Re: [PATCH v2] ArmPkg/TimerDxe: Delay End Of Interrupt Signal 
 
External email: Use caution opening links or attachments


On Tue, 12 Oct 2021 17:11:36 +0100,
Ashish Singhal <ashishsingha@nvidia.com> wrote:
>
> Marc,
>
> What do you suggest should be the proper fix for getting timer
> interrupts even when ISTATUS bit is not set? Should we ignore them
> the way it is in current implementation? I am OK to file a bug for
> this if you think that is a better way to discuss this.

I don't think there is anything to fix.

Yes, the order in EDKII is odd. No, changing the order doesn't give
any extra guarantee. Spurious interrupts can always happen. Broken (or
slow) HW and bad emulation are more susceptible to it.

Now, how often do you see that? On which HW?

        M.

--
Without deviation from the norm, progress is not possible.

Marc,

We see at least one spurious interrupt after every valid timer interrupt. While both valid and spurious interrupt has the correct source, spurious interrupt does not have ISTATUS bit set. We are seeing this on Silicon and not on the emulation platform. Delaying EOI signal to GIC does take the spurious interrupt out as with the new flow we clear the interrupt before signaling EOI so that next time only a valid interrupt can be triggered and not the old interrupt which was still not cleared while signaling EOI to GIC.

Thanks
Ashish

Thanks
Ashish

Re: [PATCH v2] ArmPkg/TimerDxe: Delay End Of Interrupt Signal

[Ashish Singhal]

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



From: Marc Zyngier <maz@kernel.org>
Sent: Tuesday, October 12, 2021 10:27 AM
To: Ashish Singhal <ashishsingha@nvidia.com>
Cc: Shanker Donthineni <sdonthineni@nvidia.com>; Ard Biesheuvel <ardb@kernel.org>; edk2-devel-groups-io <devel@edk2.groups.io>; Leif Lindholm <leif@nuviainc.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>
Subject: Re: [PATCH v2] ArmPkg/TimerDxe: Delay End Of Interrupt Signal 
 
External email: Use caution opening links or attachments


On Tue, 12 Oct 2021 17:11:36 +0100,
Ashish Singhal <ashishsingha@nvidia.com> wrote:
>
> Marc,
>
> What do you suggest should be the proper fix for getting timer
> interrupts even when ISTATUS bit is not set? Should we ignore them
> the way it is in current implementation? I am OK to file a bug for
> this if you think that is a better way to discuss this.

I don't think there is anything to fix.

Yes, the order in EDKII is odd. No, changing the order doesn't give
any extra guarantee. Spurious interrupts can always happen. Broken (or
slow) HW and bad emulation are more susceptible to it.

Now, how often do you see that? On which HW?

        M.

--
Without deviation from the norm, progress is not possible.

Marc,

We see at least one spurious interrupt after every valid timer interrupt. While both valid and spurious interrupt has the correct source, spurious interrupt does not have ISTATUS bit set. We are seeing this on Silicon and not on the emulation platform. Delaying EOI signal to GIC does take the spurious interrupt out as with the new flow we clear the interrupt before signaling EOI so that next time only a valid interrupt can be triggered and not the old interrupt which was still not cleared while signaling EOI to GIC.

Thanks
Ashish

Thanks
Ashish

Marc,

I can confirm that with the current code on edk2, we get 1 spurious interrupt for every 1 valid interrupt from GIC. With the change I proposed, we do not get the spurious interrupt at all.

Thanks
Ashish

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

[Jeff Fan]

[-- Attachment #1: Type: text/plain, Size: 4477 bytes --]

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
Date: 2021-10-12 23:38
To: Marc Zyngier; Ard Biesheuvel; Shanker Donthineni
CC: edk2-devel-groups-io; Leif Lindholm; Ard Biesheuvel
Subject: Re: [edk2-devel] [PATCH v2] ArmPkg/TimerDxe: Delay End Of Interrupt Signal
+ Shaker
From: Ashish Singhal <ashishsingha@nvidia.com>
Sent: Tuesday, October 12, 2021 8:56:58 AM
To: Marc Zyngier <maz@kernel.org>; Ard Biesheuvel <ardb@kernel.org>
Cc: edk2-devel-groups-io <devel@edk2.groups.io>; Leif Lindholm <leif@nuviainc.com>; Ard Biesheuvel <ardb+tianocore@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@kernel.org>
Sent: Tuesday, October 12, 2021 2:57 AM
To: Ard Biesheuvel <ardb@kernel.org>; Ashish Singhal <ashishsingha@nvidia.com>
Cc: edk2-devel-groups-io <devel@edk2.groups.io>; Leif Lindholm <leif@nuviainc.com>; Ard Biesheuvel <ardb+tianocore@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@kernel.org> wrote:
>
> (+ Marc)
>
> On Mon, 11 Oct 2021 at 23:40, Ashish Singhal <ashishsingha@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@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.


[-- Attachment #2: Type: text/html, Size: 9402 bytes --]

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

[Ashish Singhal]

[-- Attachment #1: Type: text/plain, Size: 6301 bytes --]

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@byosoft.com.cn <fanjianfeng@byosoft.com.cn>
Sent: Tuesday, October 12, 2021 8:32 PM
To: 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>
Cc: devel@edk2.groups.io <devel@edk2.groups.io>; Leif Lindholm <leif@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@groups.io>
Date: 2021-10-12 23:38
To: Marc Zyngier<mailto:maz@kernel.org>; Ard Biesheuvel<mailto:ardb@kernel.org>; Shanker Donthineni<mailto:sdonthineni@nvidia.com>
CC: edk2-devel-groups-io<mailto:devel@edk2.groups.io>; Leif Lindholm<mailto:leif@nuviainc.com>; Ard Biesheuvel<mailto:ardb+tianocore@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@nvidia.com>
Sent: Tuesday, October 12, 2021 8:56:58 AM
To: Marc Zyngier <maz@kernel.org>; Ard Biesheuvel <ardb@kernel.org>
Cc: edk2-devel-groups-io <devel@edk2.groups.io>; Leif Lindholm <leif@nuviainc.com>; Ard Biesheuvel <ardb+tianocore@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@kernel.org>
Sent: Tuesday, October 12, 2021 2:57 AM
To: Ard Biesheuvel <ardb@kernel.org>; Ashish Singhal <ashishsingha@nvidia.com>
Cc: edk2-devel-groups-io <devel@edk2.groups.io>; Leif Lindholm <leif@nuviainc.com>; Ard Biesheuvel <ardb+tianocore@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@kernel.org> wrote:
>
> (+ Marc)
>
> On Mon, 11 Oct 2021 at 23:40, Ashish Singhal <ashishsingha@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@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.


[-- Attachment #2: Type: text/html, Size: 12324 bytes --]