From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:4864:20::341; helo=mail-wm1-x341.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wm1-x341.google.com (mail-wm1-x341.google.com [IPv6:2a00:1450:4864:20::341]) (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 D4B1621B02822 for ; Tue, 20 Nov 2018 08:17:25 -0800 (PST) Received: by mail-wm1-x341.google.com with SMTP id g131so2675489wmg.3 for ; Tue, 20 Nov 2018 08:17:25 -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=3GPjUJ/tijf5epM7Ny4rmZ0/rzmKgjQtZv1kI6qKI6w=; b=Ge788FdBp7xTLOSJ5OSM3lvkYYDp7rQ214WN4tn1Nglk6imLQTR78RAAGE+03qmYK+ 02l5joSD4urihJnitA5zurOjMM1tHUJONwzppsgHewwqzLdkwsAs3Zx1AfPhNdk0Vm30 459g2co9upk56CGPd411gf4YkHKm3Mx2NtXP0= 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=3GPjUJ/tijf5epM7Ny4rmZ0/rzmKgjQtZv1kI6qKI6w=; b=nWg42EDkEH0968dyyTRPBlLAy0WWv/Mqmlecqmj2VbT/dIdvNaAT+dCRQP9c7ZkJpu EDYpZSlDQSWR6aapZM/3ad2CwfymdQX1mFb4tBibZxZlkHjYZslxlaXryRS5nbcJMO/R NQVFAh2tw4RkPzr+f+yEa9MjvvgrCMseTa6GVCe8GFwWPAECOS+UYySOlzq0nD6jUd/F QAMW5s7lt0nxXxsN+ykdAQkoX4DEqAudFoiEvXeq6SMjveidfGgb8urOv4pEVTQb+gog A3Peqm+oxRab0mfu5oV/eu/xKmNdmCgzscdkcRpJ5+BYHC73eSuLVRDB+oG937u8cBig OCag== X-Gm-Message-State: AGRZ1gJpGSFfzpN8+QvbptdcYHtitKq485WTmYW+wQCCCyTcCaPyDG6z KCeREKG8kfFSo6q6CktS3eBhwYqji/I= X-Google-Smtp-Source: AFSGD/V4UX06LuiFOrWJWzYjiOWdQlx3rAmZK0RCeszEHaoSdW9nYVkSFhUH/nGHUDyaI4cXW5y81Q== X-Received: by 2002:a7b:cd16:: with SMTP id f22mr2691683wmj.88.1542730643914; Tue, 20 Nov 2018 08:17:23 -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 a12sm27506293wro.18.2018.11.20.08.17.22 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 20 Nov 2018 08:17:23 -0800 (PST) Date: Tue, 20 Nov 2018 16:17:21 +0000 From: Leif Lindholm To: Ard Biesheuvel Cc: edk2-devel@lists.01.org Message-ID: <20181120161720.ipk3z6pazznlobp2@bivouac.eciton.net> References: <20181120154433.14167-1-ard.biesheuvel@linaro.org> MIME-Version: 1.0 In-Reply-To: <20181120154433.14167-1-ard.biesheuvel@linaro.org> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [PATCH v2] ArmPkg/ArmSmcPsciResetSystemLib: add missing call to ExitBootServices() X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 20 Nov 2018 16:17:26 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Tue, Nov 20, 2018 at 04:44:33PM +0100, Ard Biesheuvel wrote: > Our poor man's implementation of EnterS3WithImmediateWake () currently > sets a high TPL level to disable interrupts, and simply calls the > PEI entrypoint again after disabling the MMU. > > Unfortunately, this is not sufficient: DMA capable devices such as > network controllers or USB controllers may still be enabled and > writing to memory, e.g., in response to incoming network packets. > > So instead, do the full ExitBootServices() dance: allocate space and > get the memory map, call ExitBootServices(), and in case it fails, get > the memory map again and call ExitBootServices() again. This ensures > that all cleanup related to DMA capable devices is performed before > doing the warm reset. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ard Biesheuvel > --- > v2: - add asm routines to ensure that we don't access memory after > disabling the MMU and caches Fun times. Reviewed-by: Leif Lindholm > - set MemMap to NULL > > ArmPkg/Library/ArmSmcPsciResetSystemLib/AArch64/Reset.S | 30 +++++++++++ > ArmPkg/Library/ArmSmcPsciResetSystemLib/AArch64/Reset.asm | 35 ++++++++++++ > ArmPkg/Library/ArmSmcPsciResetSystemLib/Arm/Reset.S | 29 ++++++++++ > ArmPkg/Library/ArmSmcPsciResetSystemLib/Arm/Reset.asm | 34 ++++++++++++ > ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c | 57 +++++++++++++++++--- > ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf | 8 +++ > 6 files changed, 187 insertions(+), 6 deletions(-) > > diff --git a/ArmPkg/Library/ArmSmcPsciResetSystemLib/AArch64/Reset.S b/ArmPkg/Library/ArmSmcPsciResetSystemLib/AArch64/Reset.S > new file mode 100644 > index 000000000000..1edca941cb8f > --- /dev/null > +++ b/ArmPkg/Library/ArmSmcPsciResetSystemLib/AArch64/Reset.S > @@ -0,0 +1,30 @@ > +/** @file > + ResetSystemLib implementation using PSCI calls > + > + Copyright (c) 2018, Linaro Ltd. All rights reserved.
> + > + This program and the accompanying materials > + are licensed and made available under the terms and conditions of the BSD License > + which accompanies this distribution. The full text of the license may be found at > + http://opensource.org/licenses/bsd-license.php > + > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > + > +**/ > + > +#include > + > +ASM_FUNC(DisableMmuAndReenterPei) > + stp x29, x30, [sp, #-16]! > + mov x29, sp > + > + bl ArmDisableMmu > + > + // no memory accesses after MMU and caches have been disabled > + > + MOV64 (x0, FixedPcdGet64 (PcdFvBaseAddress)) > + blr x0 > + > + // never returns > + nop > diff --git a/ArmPkg/Library/ArmSmcPsciResetSystemLib/AArch64/Reset.asm b/ArmPkg/Library/ArmSmcPsciResetSystemLib/AArch64/Reset.asm > new file mode 100644 > index 000000000000..b3dca150c150 > --- /dev/null > +++ b/ArmPkg/Library/ArmSmcPsciResetSystemLib/AArch64/Reset.asm > @@ -0,0 +1,35 @@ > +/** @file > + ResetSystemLib implementation using PSCI calls > + > + Copyright (c) 2018, Linaro Ltd. All rights reserved.
> + > + This program and the accompanying materials > + are licensed and made available under the terms and conditions of the BSD License > + which accompanies this distribution. The full text of the license may be found at > + http://opensource.org/licenses/bsd-license.php > + > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > + > +**/ > + > + AREA Reset, CODE, READONLY > + > + EXPORT DisableMmuAndReenterPei > + IMPORT ArmDisableMmu > + > +DisableMmuAndReenterPei > + stp x29, x30, [sp, #-16]! > + mov x29, sp > + > + bl ArmDisableMmu > + > + ; no memory accesses after MMU and caches have been disabled > + > + movl x0, FixedPcdGet64 (PcdFvBaseAddress) > + blr x0 > + > + ; never returns > + nop > + > + END > diff --git a/ArmPkg/Library/ArmSmcPsciResetSystemLib/Arm/Reset.S b/ArmPkg/Library/ArmSmcPsciResetSystemLib/Arm/Reset.S > new file mode 100644 > index 000000000000..b6fe2bd82baa > --- /dev/null > +++ b/ArmPkg/Library/ArmSmcPsciResetSystemLib/Arm/Reset.S > @@ -0,0 +1,29 @@ > +/** @file > + ResetSystemLib implementation using PSCI calls > + > + Copyright (c) 2018, Linaro Ltd. All rights reserved.
> + > + This program and the accompanying materials > + are licensed and made available under the terms and conditions of the BSD License > + which accompanies this distribution. The full text of the license may be found at > + http://opensource.org/licenses/bsd-license.php > + > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > + > +**/ > + > +#include > + > +ASM_FUNC(DisableMmuAndReenterPei) > + push {lr} > + > + bl ArmDisableMmu > + > + // no memory accesses after MMU and caches have been disabled > + > + MOV32 (r0, FixedPcdGet64 (PcdFvBaseAddress)) > + blx r0 > + > + // never returns > + nop > diff --git a/ArmPkg/Library/ArmSmcPsciResetSystemLib/Arm/Reset.asm b/ArmPkg/Library/ArmSmcPsciResetSystemLib/Arm/Reset.asm > new file mode 100644 > index 000000000000..f098b352418b > --- /dev/null > +++ b/ArmPkg/Library/ArmSmcPsciResetSystemLib/Arm/Reset.asm > @@ -0,0 +1,34 @@ > +/** @file > + ResetSystemLib implementation using PSCI calls > + > + Copyright (c) 2018, Linaro Ltd. All rights reserved.
> + > + This program and the accompanying materials > + are licensed and made available under the terms and conditions of the BSD License > + which accompanies this distribution. The full text of the license may be found at > + http://opensource.org/licenses/bsd-license.php > + > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > + > +**/ > + > + INCLUDE AsmMacroExport.inc > + PRESERVE8 > + > + IMPORT ArmDisableMmu > + > +RVCT_ASM_EXPORT DisableMmuAndReenterPei > + push {lr} > + > + bl ArmDisableMmu > + > + ; no memory accesses after MMU and caches have been disabled > + > + mov32 r0, FixedPcdGet64 (PcdFvBaseAddress) > + blx r0 > + > + ; never returns > + nop > + > + END > diff --git a/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c b/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c > index 10ceafd14d5d..3f7b8ae0b169 100644 > --- a/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c > +++ b/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c > @@ -1,7 +1,7 @@ > /** @file > ResetSystemLib implementation using PSCI calls > > - Copyright (c) 2017, Linaro Ltd. All rights reserved.
> + Copyright (c) 2017 - 2018, Linaro Ltd. All rights reserved.
> > This program and the accompanying materials > are licensed and made available under the terms and conditions of the BSD License > @@ -81,6 +81,8 @@ ResetShutdown ( > ArmCallSmc (&ArmSmcArgs); > } > > +VOID DisableMmuAndReenterPei (VOID); > + > /** > This function causes the system to enter S3 and then wake up immediately. > > @@ -92,7 +94,12 @@ EnterS3WithImmediateWake ( > VOID > ) > { > - VOID (*Reset)(VOID); > + EFI_PHYSICAL_ADDRESS Alloc; > + EFI_MEMORY_DESCRIPTOR *MemMap; > + UINTN MemMapSize; > + UINTN MapKey, DescriptorSize; > + UINT32 DescriptorVersion; > + EFI_STATUS Status; > > if (FeaturePcdGet (PcdArmReenterPeiForCapsuleWarmReboot) && > !EfiAtRuntime ()) { > @@ -101,11 +108,49 @@ EnterS3WithImmediateWake ( > // immediate wake (which is used by capsule update) by disabling the MMU > // and interrupts, and jumping to the PEI entry point. > // > - Reset = (VOID (*)(VOID))(UINTN)FixedPcdGet64 (PcdFvBaseAddress); > > - gBS->RaiseTPL (TPL_HIGH_LEVEL); > - ArmDisableMmu (); > - Reset (); > + // > + // Obtain the size of the memory map > + // > + MemMapSize = 0; > + MemMap = NULL; > + Status = gBS->GetMemoryMap (&MemMapSize, MemMap, &MapKey, &DescriptorSize, > + &DescriptorVersion); > + ASSERT (Status == EFI_BUFFER_TOO_SMALL); > + > + // > + // Add some slack to the allocation to cater for changes in the memory > + // map if ExitBootServices () fails the first time around. > + // > + MemMapSize += SIZE_4KB; > + Status = gBS->AllocatePages (AllocateAnyPages, EfiBootServicesData, > + EFI_SIZE_TO_PAGES (MemMapSize), &Alloc); > + ASSERT_EFI_ERROR (Status); > + > + MemMap = (EFI_MEMORY_DESCRIPTOR *)(UINTN)Alloc; > + > + Status = gBS->GetMemoryMap (&MemMapSize, MemMap, &MapKey, &DescriptorSize, > + &DescriptorVersion); > + ASSERT_EFI_ERROR (Status); > + > + Status = gBS->ExitBootServices (gImageHandle, MapKey); > + if (EFI_ERROR (Status)) { > + // > + // ExitBootServices () may fail the first time around if an event fired > + // right after the call to GetMemoryMap() which allocated or freed memory. > + // Since that first call to ExitBootServices () will disarm the timer, > + // this is guaranteed not to happen again, so one additional attempt > + // should suffice. > + // > + Status = gBS->GetMemoryMap (&MemMapSize, MemMap, &MapKey, &DescriptorSize, > + &DescriptorVersion); > + ASSERT_EFI_ERROR (Status); > + > + Status = gBS->ExitBootServices (gImageHandle, MapKey); > + ASSERT_EFI_ERROR (Status); > + } > + > + DisableMmuAndReenterPei (); > } > } > > diff --git a/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf b/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf > index 19021cd1e8b6..3b8925c6f9cd 100644 > --- a/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf > +++ b/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf > @@ -21,6 +21,14 @@ > VERSION_STRING = 1.0 > LIBRARY_CLASS = ResetSystemLib > > +[Sources.AARCH64] > + AArch64/Reset.S | GCC > + AArch64/Reset.asm | MSFT > + > +[Sources.ARM] > + Arm/Reset.S | GCC > + Arm/Reset.asm | RVCT > + > [Sources] > ArmSmcPsciResetSystemLib.c > > -- > 2.17.1 >