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

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

(+ 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..34d4be3867647e0fdad7356c949af8cd3ede7164 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..983936f3738a74bb5d5e08e012973df240958a8b 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")
>