Re: [PATCH] ArmPkg/Gic: force GIC driver to run before CPU arch protocol driver

[Leif Lindholm <leif.lindholm@linaro.org>]

On Tue, Feb 06, 2018 at 12:04:16PM +0000, Ard Biesheuvel wrote:
> Currently, the GIC driver has a static dependency on the CPU arch protocol
> driver, so it can register its IRQ handler at init time. This means there
> is a window between dispatch of the CPU driver and dispatch of the GIC
> driver where any unexpected GIC state may trigger an interrupt which we
> are not set up to handle yet. Note that this is even the case if we enter
> UEFI with interrupts disabled at the CPU, given that any TPL manipulation
> involving TPL_HIGH_LEVEL will unconditionally enable IRQs at the CPU side
> regardless of whether they were enabled to begin with (but only as soon as
> the CPU arch protocol is actually installed)
> 
> So let's reorder the GIC driver with the CPU driver, and let it run its
> initialization that puts the GIC into a known state before enabling
> interrupts. Move its installation of its IRQ handler to a protocol notify
> callback on the CPU arch protocol so that it runs as soon as it becomes
> available.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
> 
> This fixes an issue observed with GICv3 guests running under KVM.
> 
>  ArmPkg/ArmPkg.dec                       |  2 +
>  ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c | 69 +++++++++++++-------
>  ArmPkg/Drivers/ArmGic/ArmGicDxe.h       |  1 +
>  ArmPkg/Drivers/ArmGic/ArmGicDxe.inf     |  5 +-
>  ArmPkg/Drivers/CpuDxe/CpuDxe.inf        |  2 +-
>  5 files changed, 54 insertions(+), 25 deletions(-)
> 
> diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
> index 5dbd019e2966..a55b6268ff85 100644
> --- a/ArmPkg/ArmPkg.dec
> +++ b/ArmPkg/ArmPkg.dec
> @@ -48,6 +48,8 @@ [Guids.common]
>    # Include/Guid/ArmMpCoreInfo.h
>    gArmMpCoreInfoGuid = { 0xa4ee0728, 0xe5d7, 0x4ac5,  {0xb2, 0x1e, 0x65, 0x8e, 0xd8, 0x57, 0xe8, 0x34} }
>  
> +  gArmGicDxeFileGuid = { 0xde371f7c, 0xdec4, 0x4d21, { 0xad, 0xf1, 0x59, 0x3a, 0xbc, 0xc1, 0x58, 0x82 } }
> +
>  [Ppis]
>    ## Include/Ppi/ArmMpCoreInfo.h
>    gArmMpCoreInfoPpiGuid = { 0x6847cc74, 0xe9ec, 0x4f8f, {0xa2, 0x9d, 0xab, 0x44, 0xe7, 0x54, 0xa8, 0xfc} }
> diff --git a/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c b/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c
> index bff8d983cf02..e1adcd3bc6d3 100644
> --- a/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c
> +++ b/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c
> @@ -121,6 +121,44 @@ RegisterInterruptSource (
>    }
>  }
>  
> +STATIC VOID *mCpuArchProtocolNotifyEventRegistration;
> +
> +STATIC
> +VOID
> +EFIAPI
> +CpuArchEventProtocolNotify (
> +  IN  EFI_EVENT       Event,
> +  IN  VOID            *Context
> +  )
> +{
> +  EFI_CPU_ARCH_PROTOCOL   *Cpu;
> +  EFI_STATUS              Status;
> +
> +  // Get the CPU protocol that this driver requires.
> +  Status = gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID **)&Cpu);
> +  if (EFI_ERROR (Status)) {
> +    return;
> +  }
> +
> +  // Unregister the default exception handler.
> +  Status = Cpu->RegisterInterruptHandler (Cpu, ARM_ARCH_EXCEPTION_IRQ, NULL);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "%a: Cpu->RegisterInterruptHandler() - %r\n",
> +      __FUNCTION__, Status));
> +    return;
> +  }
> +
> +  // Register to receive interrupts
> +  Status = Cpu->RegisterInterruptHandler (Cpu, ARM_ARCH_EXCEPTION_IRQ,
> +                  Context);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "%a: Cpu->RegisterInterruptHandler() - %r\n",
> +      __FUNCTION__, Status));
> +  }
> +
> +  gBS->CloseEvent (Event);
> +}
> +
>  EFI_STATUS
>  InstallAndRegisterInterruptService (
>    IN EFI_HARDWARE_INTERRUPT_PROTOCOL   *InterruptProtocol,
> @@ -130,7 +168,6 @@ InstallAndRegisterInterruptService (
>    )
>  {
>    EFI_STATUS               Status;
> -  EFI_CPU_ARCH_PROTOCOL   *Cpu;
>    CONST UINTN              RihArraySize =
>      (sizeof(HARDWARE_INTERRUPT_HANDLER) * mGicNumInterrupts);
>  
> @@ -152,27 +189,15 @@ InstallAndRegisterInterruptService (
>      return Status;
>    }
>  
> -  // Get the CPU protocol that this driver requires.
> -  Status = gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID **)&Cpu);
> -  if (EFI_ERROR (Status)) {
> -    return Status;
> -  }
> -
> -  // Unregister the default exception handler.
> -  Status = Cpu->RegisterInterruptHandler (Cpu, ARM_ARCH_EXCEPTION_IRQ, NULL);
> -  if (EFI_ERROR (Status)) {
> -    return Status;
> -  }
> -
> -  // Register to receive interrupts
> -  Status = Cpu->RegisterInterruptHandler (
> -                  Cpu,
> -                  ARM_ARCH_EXCEPTION_IRQ,
> -                  InterruptHandler
> -                  );
> -  if (EFI_ERROR (Status)) {
> -    return Status;
> -  }
> +  //
> +  // Install the interrupt handler as soon as the CPU arch protocol appears.
> +  //
> +  EfiCreateProtocolNotifyEvent (
> +    &gEfiCpuArchProtocolGuid,
> +    TPL_CALLBACK,
> +    CpuArchEventProtocolNotify,
> +    InterruptHandler,
> +    &mCpuArchProtocolNotifyEventRegistration);
>  
>    // Register for an ExitBootServicesEvent
>    Status = gBS->CreateEvent (
> diff --git a/ArmPkg/Drivers/ArmGic/ArmGicDxe.h b/ArmPkg/Drivers/ArmGic/ArmGicDxe.h
> index 610ffacc7eb0..f6b75d729601 100644
> --- a/ArmPkg/Drivers/ArmGic/ArmGicDxe.h
> +++ b/ArmPkg/Drivers/ArmGic/ArmGicDxe.h
> @@ -21,6 +21,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>  #include <Library/IoLib.h>
>  #include <Library/MemoryAllocationLib.h>
>  #include <Library/UefiBootServicesTableLib.h>
> +#include <Library/UefiLib.h>
>  
>  #include <Protocol/Cpu.h>
>  #include <Protocol/HardwareInterrupt.h>
> diff --git a/ArmPkg/Drivers/ArmGic/ArmGicDxe.inf b/ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
> index d5921533fb68..24b02ef30e83 100644
> --- a/ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
> +++ b/ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
> @@ -16,7 +16,7 @@
>  [Defines]
>    INF_VERSION                    = 0x00010005
>    BASE_NAME                      = ArmGicDxe
> -  FILE_GUID                      = DE371F7C-DEC4-4D21-ADF1-593ABCC15882
> +  FILE_GUID                      = DE371F7C-DEC4-4D21-ADF1-593ABCC15882 # gArmGicDxeFileGuid
>    MODULE_TYPE                    = DXE_DRIVER
>    VERSION_STRING                 = 1.0
>  
> @@ -45,6 +45,7 @@ [LibraryClasses]
>    UefiDriverEntryPoint
>    IoLib
>    PcdLib
> +  UefiLib
>  
>  [Protocols]
>    gHardwareInterruptProtocolGuid
> @@ -58,4 +59,4 @@ [Pcd.common]
>    gArmTokenSpaceGuid.PcdArmGicV3WithV2Legacy
>  
>  [Depex]
> -  gEfiCpuArchProtocolGuid
> +  TRUE
> diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
> index d068e06803ed..cda549922e9c 100644
> --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
> +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
> @@ -76,4 +76,4 @@ [FeaturePcd.common]
>    gArmTokenSpaceGuid.PcdDebuggerExceptionSupport
>  
>  [Depex]
> -  TRUE
> +  AFTER gArmGicDxeFileGuid
> -- 
> 2.11.0
>