From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:400c:c0c::243; helo=mail-wr0-x243.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wr0-x243.google.com (mail-wr0-x243.google.com [IPv6:2a00:1450:400c:c0c::243]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 95D78223972B3 for ; Tue, 6 Feb 2018 04:55:38 -0800 (PST) Received: by mail-wr0-x243.google.com with SMTP id v15so1825336wrb.8 for ; Tue, 06 Feb 2018 05:01:21 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=zXV82ELdapd4rNpx49WHIHVaDXhSuqqNJpg3xP4I8pE=; b=RtXPM+gF9NIUjw+hvNsqBwG3a+n3IIwKflSw9UFlu4GJIwjke98Kc7R1zrbC6zpIf2 VStXhHfGVbc68DMZ2Ks5VRgpORHcLq5Nys164U1xGeugdxNE6ojcN8lAsG2Shbtwu5sT PIHF+7vv/KEdLPTXjPy8naQYNyiqkXf+r2aJE= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=zXV82ELdapd4rNpx49WHIHVaDXhSuqqNJpg3xP4I8pE=; b=Lxbq3xM4MYk+q0d/9BYXoigrCOTRF9pC+Txy52S2xdG7kjYFvYGxqxA+RBcV0Y4BV2 REU68ooYkCQytXOcNvJN/zBuvrmlFWO+IVb+Sz8gXzLMvc2ukWhR9DJiFccHJ/n8WBs7 dU/AFtcr3dBwOPQM1NBeaNwrFTmoShtyJ/WdiPQkvG1KsfQthgHp/WYFDg4iU3s8KjI4 aL53VahsDDXAJaVOOcXci088atrH8f+1IR25Z1I7zlPdR8taSX00b/s48K7FnPmsW59d ovcb2mLPgVI+ATWQ8O231+c1Ym5eHavGx4a8EEr2GxLU0cb5NPMTS1TuzSJAHaqHzjOm nRLA== X-Gm-Message-State: APf1xPAL/0GmE05U7dSX68UafbQIZwn907015JkETUBVHO9f3hBDavbB jFFcJc8ZKr3ZyBXGwOB/VtpAHA== X-Google-Smtp-Source: AH8x225IiVFyIWP+1s7v2pUk7AgR/NTBK4SccCCJ1PKMqbmOdi+fjE5sebw658xemYOqCXfDIPICsA== X-Received: by 10.223.143.66 with SMTP id p60mr2099458wrb.81.1517922079092; Tue, 06 Feb 2018 05:01:19 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id n24sm12159543wmi.21.2018.02.06.05.01.17 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 06 Feb 2018 05:01:17 -0800 (PST) Date: Tue, 6 Feb 2018 13:01:16 +0000 From: Leif Lindholm To: Ard Biesheuvel Cc: edk2-devel@lists.01.org, marc.zyngier@arm.com Message-ID: <20180206130116.ejxc3f2djg6y3dtm@bivouac.eciton.net> References: <20180206120416.17462-1-ard.biesheuvel@linaro.org> MIME-Version: 1.0 In-Reply-To: <20180206120416.17462-1-ard.biesheuvel@linaro.org> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [PATCH] ArmPkg/Gic: force GIC driver to run before CPU arch protocol driver X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 06 Feb 2018 12:55:39 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 Reviewed-by: Leif Lindholm > --- > > 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 > #include > #include > +#include > > #include > #include > 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 >