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

[Evan Lloyd <Evan.Lloyd@arm.com>]

Hi.
(Note - this is not "top-posting", it is a discussion point, with references appended.)

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.

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.

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

Does anyone know of a concrete reason why we should not restore the EOI to the GIC handling and remove it from the timer ISR?
Eugene - you express a strongly reasoned case - can I talk you round here?

Regards,
Evan

> Hi Alexei,
>
> Thanks for your explanation. However, when I tested it on D02, it didn't act as expected, i.e. we did see a subsequent interrupt triggered >immediately when gBS->RestoreTPL was called in which IRQ is enabled, and I had set timer interrupt period to some fairly large value (e.g. 5 >seconds).
>
> So I'd like to confirm, is it declared in GIC specification that clearing interrupt source will also clear pending status in GICD?
>
> Thanks and regards.
>
> Heyi

...
> From: Linaro-uefi [mailto:linaro-uefi-bounces@lists.linaro.org] On Behalf Of Heyi Guo
> Sent: 02 August 2016 02:28
> To: Ard Biesheuvel
> Cc: Linaro UEFI Mailman List
> Subject: Re: [Linaro-uefi] [linaro-uefi] Two write to EOIR in a single interrupt
>
> Hi Leif and Ard,
>
> There might be another issue in timer interrupt handler. Timer interrupt seems to be level triggered, so is it OK to write EOIR before clearing the > > > interrupt source, i.e. updating compare value register?
>
> Heyi

>-----Original Message-----
>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard
>Biesheuvel
>Sent: 08 August 2016 14:51
>To: Cohen, Eugene
>Cc: Alexei Fedorov; edk2-devel@lists.01.org; Leif Lindholm; Andrew Fish
>(afish@apple.com); Heyi Guo
>Subject: Re: [edk2] [PATCH] ArmPkg: Fix double GIC EIOR write per interrupt
>
>On 8 August 2016 at 15:42, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> On 8 August 2016 at 15:22, Cohen, Eugene <eugene@hp.com> wrote:
>>> 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 for the context. I did some archaeology of my own, and it turns
>> out that this code was introduced by Andrew in git commit
>> 1bfda055dfbc52678 (svn #11293) more than 5 years ago.
>>
>> In any case, it appears we are in agreement that it is in fact
>> incorrect (and deviates from the other implementations) to signal EOI
>> in the GIC driver, and so I suppose Alexei's patch is good (and we
>> only need to clarify the comment that he quoted in this thread).
>>
>> My primary concern was that we change the contract with existing
>> handlers, but if there was a contract to begin with, we were already
>> violating it, and so any out of tree breakage is not really our
>> problem :-)
>>
>
>Pushed as 7989300df7
>_______________________________________________
>edk2-devel mailing list
>edk2-devel@lists.01.org
>https://lists.01.org/mailman/listinfo/edk2-devel

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.