Re: [PATCH] ArmPkg: Fix double GIC EIOR write per interrupt

["Cohen, Eugene" <eugene@hp.com>]

Guys, sorry to join so late, something about timezones...  Let me try to provide some context and history.

> > it does change the contract we have with registered interrupt handlers
> 
> Looks like it does not:
> From edk2\EmbeddedPkg\Include\Protocol\HardwareInterrupt.h:
> 
> " Abstraction for hardware based interrupt routine
> 
>   ...The driver implementing
>   this protocol is responsible for clearing the pending interrupt in the
>   interrupt routing hardware. The HARDWARE_INTERRUPT_HANDLER is
> responsible
>   for clearing interrupt sources from individual devices."

I think you are reading more deeply into this verbiage than was intended.  >From a separation-of-concerns perspective one driver is concerned with writing to the hardware that generates the interrupt (handler) and another is concerned with writing to the hardware for the interrupt controller to signal the end of interrupt.  So all this is saying is that "the code that touches the interrupt controller is implemented in the driver that publishes this protocol".  It does not say how this code is activated, only who is responsible for poking the register.

The historical expectation is that the handler driver calls the EOI interface in the protocol.  (If it was the opposite then this interface wouldn't even exist since the interrupt controller driver could just do it implicitly.)  You're next question will probably be why it was designed this way - for that we'll have to ask Andrew Fish (added).

I did a little digging and see that the PC-AT chipset implemented an 8259 interrupt protocol (IntelFrameworkPkg\Include\Protocol\Legacy8259.h) that is quite similar to HwInterrupt.  Notice the explicit EndOfInterrupt interface here and how it's used by the timer driver at PcAtChipsetPkg\8254TimerDxe\Timer.c(88).

Given this I asked that you keep the EndOfInterrupt in the handler driver(s) and remove the auto-EOI in the interrupt controller driver, at least for cases where a driver handled the interrupt.

Feel free to clarify the text in the protocol header to align with this - the current wording is not very clear.

Thanks,

Eugene

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Ard Biesheuvel
> Sent: Monday, August 08, 2016 5:51 AM
> To: Alexei Fedorov <Alexei.Fedorov@arm.com>
> Cc: edk2-devel@lists.01.org <edk2-devel@ml01.01.org>; Heyi Guo
> <heyi.guo@linaro.org>; Leif Lindholm <leif.lindholm@linaro.org>; Cohen,
> Eugene <eugene@hp.com>
> Subject: Re: [edk2] [PATCH] ArmPkg: Fix double GIC EIOR write per interrupt
> 
> On 8 August 2016 at 13:08, Ard Biesheuvel <ard.biesheuvel@linaro.org>
> wrote:
> > On 8 August 2016 at 13:06, Alexei Fedorov <Alexei.Fedorov@arm.com>
> wrote:
> >> Timer's pending interrupt is cleared  HARDWARE_INTERRUPT_HANDLER
> TimerInterruptHandler()  in when timer is re-programmed for the next shot.
> >
> > Yes, that is the timer side.
> >
> >> Does it mean that TimerDxe driver is part
> EFI_HARDWARE_INTERRUPT_PROTOCOL?
> >>
> >
> > No. The peripheral and the GIC each have their own mask and enable
> > registers for the interrupt. That is exactly why the comment describes
> > in detail which part is the responsibility of the GIC, and which is
> > the responsibility of the peripheral.
> >
> 
> ... and actually, looking at TimerInterruptHandler (), I don't see the point of
> re-enabling the interrupt early, given that
> 
> 308: OriginalTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL);
> 
> disables interrupts globally, and only re-enables them on line 346, at which
> point the mTimerNotifyFunction() has already returned.
> 
> So I propose we simply do
> 
> diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> index 1169d426b255..f0fcb05757ac 100644
> --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> @@ -308,10 +308,7 @@ TimerInterruptHandler (
>    OriginalTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL);
> 
>    // Check if the timer interrupt is active
> -  if ((ArmGenericTimerGetTimerCtrlReg () ) & ARM_ARCH_TIMER_ISTATUS)
> {
> -
> -    // Signal end of interrupt early to help avoid losing subsequent
> ticks from long duration handlers
> -    gInterrupt->EndOfInterrupt (gInterrupt, Source);
> +  while ((ArmGenericTimerGetTimerCtrlReg () ) &
> ARM_ARCH_TIMER_ISTATUS)
> + {
> 
>      if (mTimerNotifyFunction) {
>        mTimerNotifyFunction (mTimerPeriod * mElapsedPeriod);
> 
> so that if the condition exists that we know will trigger the interrupt
> immediately as soon as we unmask it, we simply enter the loop again just like
> we would when taking the [nested] interrupt.
> 
> @Heyi: any thoughts?
> 
> --
> Ard.
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel