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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 8 August 2016 at 12:25, Alexei Fedorov <Alexei.Fedorov@arm.com> wrote:
>
>> 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."
>

Thanks for digging that up!

So after this change, the driver implementing the hardware interrupt
protocol no longer clears the pending interrupt in the interrupt
routing hardware. This means that we are not only changing the
existing contract, we are also violating the spec.


>
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard Biesheuvel
> Sent: 06 August 2016 09:25
> To: Evan Lloyd; Cohen, Eugene
> Cc: edk2-devel@lists.01.org; Heyi Guo; Leif Lindholm
> Subject: Re: [edk2] [PATCH] ArmPkg: Fix double GIC EIOR write per interrupt
>
> (+ Eugene)
>
> On 5 August 2016 at 18:59,  <evan.lloyd@arm.com> wrote:
>> From: Alexei <Alexei.Fedorov@arm.com>
>>
>> This commit fixes a bug in the GIC v2 and v3 drivers where the
>> GICC_EOIR (End Of Interrupt Register) is written twice for a single interrupt.
>> GicV(2|3)IrqInterruptHandler() calls the Interrupt Handler and then
>> GicV(2|3)EndOfInterrupt() on exit:
>>
>>  InterruptHandler = gRegisteredInterruptHandlers[GicInterrupt];
>>  if (InterruptHandler != NULL) {
>>    // Call the registered interrupt handler.
>>    InterruptHandler (GicInterrupt, SystemContext);  } else {
>>    DEBUG ((EFI_D_ERROR, "Spurious GIC interrupt: 0x%x\n",
>> GicInterrupt));  }
>>
>>  GicV2EndOfInterrupt (&gHardwareInterruptV2Protocol, GicInterrupt);
>>
>> , although gInterrupt->EndOfInterrupt() has already been called by
>> InterruptHandler().
>>
>> The fix moves the EndOfInterrupt() call inside the else case for
>> unregistered/spurious interrupts.  This removes a potential race
>> condition that might have lost interrupts.
>>
>
> I understand that this solves the problem, but it does change the contract we have with registered interrupt handlers, and we don't know how this may be used out of tree. I know UEFI only supports polling for drivers, but are there any other cases (debug?) where we may break other people's code by doing this?
>
>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Alexei Fedorov <alexei.fedorov@arm.com>
>> Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
>> ---
>>
>> Code is available at:
>> https://github.com/EvanLloyd/tianocore/tree/EOIR_v1
>>
>>  ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c | 5 ++---
>> ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 5 ++---
>>  2 files changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
>> b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
>> index
>> 036eb5cd6bf6845dd2b03b62c933c1dedaef7251..34d4be3867647e0fdad7356c949a
>> f8cd3ede7164 100644
>> --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
>> +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
>> @@ -2,7 +2,7 @@
>>
>>  Copyright (c) 2009, Hewlett-Packard Company. All rights reserved.<BR>
>> Portions copyright (c) 2010, Apple Inc. All rights reserved.<BR>
>> -Portions copyright (c) 2011-2015, ARM Ltd. All rights reserved.<BR>
>> +Portions copyright (c) 2011-2016, ARM Ltd. All rights reserved.<BR>
>>
>>  This program and the accompanying materials  are licensed and made
>> available under the terms and conditions of the BSD License @@ -178,9
>> +178,8 @@ GicV2IrqInterruptHandler (
>>      InterruptHandler (GicInterrupt, SystemContext);
>>    } else {
>>      DEBUG ((EFI_D_ERROR, "Spurious GIC interrupt: 0x%x\n",
>> GicInterrupt));
>> +    GicV2EndOfInterrupt (&gHardwareInterruptV2Protocol,
>> + GicInterrupt);
>>    }
>> -
>> -  GicV2EndOfInterrupt (&gHardwareInterruptV2Protocol, GicInterrupt);
>> }
>>
>>  //
>> diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
>> b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
>> index
>> 106c669fcb8777dfaad609c0ce9f5b572727a3ff..983936f3738a74bb5d5e08e01297
>> 3df240958a8b 100644
>> --- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
>> +++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
>> @@ -1,6 +1,6 @@
>>  /** @file
>>  *
>> -*  Copyright (c) 2011-2015, ARM Limited. All rights reserved.
>> +*  Copyright (c) 2011-2016, ARM Limited. All rights reserved.
>>  *
>>  *  This program and the accompanying materials
>>  *  are licensed and made available under the terms and conditions of
>> the BSD License @@ -169,9 +169,8 @@ GicV3IrqInterruptHandler (
>>      InterruptHandler (GicInterrupt, SystemContext);
>>    } else {
>>      DEBUG ((EFI_D_ERROR, "Spurious GIC interrupt: 0x%x\n",
>> GicInterrupt));
>> +    GicV3EndOfInterrupt (&gHardwareInterruptV3Protocol,
>> + GicInterrupt);
>>    }
>> -
>> -  GicV3EndOfInterrupt (&gHardwareInterruptV3Protocol, GicInterrupt);
>> }
>>
>>  //
>> --
>> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
>>
> _______________________________________________
> 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.
>