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

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

Evan,
 
> I'd like to re-think our GIC EIOR changes in the light of comments from
> Heyi Guo. (inserted before Eugene's e-mail below)
> Despite Eugene's cogent advocacy of the change, and the fact that
> Alexei's fix is now accepted, I have come to the conclusion that it is not
> the best thing to do.  As Heyi points out, it opens a race condition
> where the Timer interrupt line is still asserted after the GIC EIOR has
> been written.
> The timer IRQ needs to be de-asserted before the EIOR write, or the
> GIC sees the IRQ and latches it ("active and pending").
> This is clearly an error, and a minor mystery is that we do not see that
> on our Juno boards.

According to the GIC architecture spec for level-sensitive interrupts are pending only based on the immediate state of the signal, there is no latching, see "Control of the pending status of level-sensitive interrupts" in the GICv2 Architecture Specification:

"
For an edge-triggered interrupt, the includes pending status is latched on either a write to the GICD_ISPENDRn or
the assertion of the interrupt signal to the GIC. However, for a level-sensitive interrupt, the includes pending status
either:
  • is latched on a write to the GICD_ISPENDRn
  • follows the state of the interrupt signal to the GIC, without any latching.
"

So there's no "memory" for level sensitive interrupts and an edge triggered interrupt will only latch on an edge, not an EOI so I'm not sure there's an issue.

>From what I can tell the primary purpose of EOI is for interrupt priority management - when you issue the EOI a priority drop occurs allowing the next-highest priority interrupt to be serviced, if any.  But since we are not making use of nested interrupts (i.e. IRQ is masked the whole time during interrupt processing) I don't think the EOI sequencing matters.  This is based on about 10 minutes of me reading this GIC spec so please correct any confusion I may have.  I agree the double-EOI has to go away no matter what.

> This is very much at odds with Eugene's position (which, BTW,  is not a
> stance I find comfortable).
> Further, it implies removing any EIOR writes from extant interrupt
> handlers - however, since they already have the "double write"
> problem, that is not really a concern.  (BTW, the GIC spec is very clear
> that "A write to this register must correspond to the most recent valid
> read from an Interrupt Acknowledge Register... otherwise the system
> behavior is UNPREDICTABLE", so the double write is not good.)

I did some detective work and the double-write problem goes way back to the BeagleBoard/Omap35xx code from 2010 where the NEWIRQAGR register was written multiple times as well.  This code actually writes the EOI-like NEWIRQAGR register two times in addition to the additional EOI interface write:

  // Needed to prevent infinite nesting when Time Driver lowers TPL
  MmioWrite32 (INTCPS_CONTROL, INTCPS_CONTROL_NEWIRQAGR);
  ArmDataSynchronizationBarrier ();

  InterruptHandler = gRegisteredInterruptHandlers[Vector];
  if (InterruptHandler != NULL) {
    // Call the registered interrupt handler.
    InterruptHandler (Vector, SystemContext);
  }

  // Needed to clear after running the handler
  MmioWrite32 (INTCPS_CONTROL, INTCPS_CONTROL_NEWIRQAGR);
  ArmDataSynchronizationBarrier ();

Note the comment about infinite nesting - I don't know what this is referring to since I would expect TPL to remain at HIGH_LEVEL for the duration of timer interrupt processing.  I'd really like to get Mr. Fish to chime in on this one.

> A more elegant solution is for the GIC register access to be done as
> part of the GIC handling (i.e. revert the GIC code, and remove the EIOR
> from the Timer handling.)  This also caters for any surreptitious use of
> other interrupts "under the covers".
> As an additional benefit, it clearly partitions the peripheral specific
> handling from the GIC interface.

I have no specific objection to this - let's just find a way to do this that maintains compatibility with the existing HardwareInterrupt protocol definition - maybe return EFI_UNSUPPORTED (or perhaps just EFI_SUCCESS) for the EOI protocol interface on systems that don't want to expose this functionality.  Then an old driver could try to use the EOI interface and on old systems it will issue the EOI and on new systems it will do nothing, deferring the real EOI to when the ISR returns.

Eugene