public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH] ArmPkg/Gic: force GIC driver to run before CPU arch protocol driver
@ 2018-02-06 12:04 Ard Biesheuvel
  2018-02-06 13:01 ` Leif Lindholm
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Ard Biesheuvel @ 2018-02-06 12:04 UTC (permalink / raw)
  To: edk2-devel, leif.lindholm; +Cc: marc.zyngier, Ard Biesheuvel

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

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



^ permalink raw reply related	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-03-21 13:10 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-02-06 12:04 [PATCH] ArmPkg/Gic: force GIC driver to run before CPU arch protocol driver Ard Biesheuvel
2018-02-06 13:01 ` Leif Lindholm
2018-02-06 14:55 ` Marc Zyngier
2018-02-06 19:01   ` Ard Biesheuvel
2018-03-21 13:15 ` Shannon Zhao

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox