From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4864:20::143; helo=mail-it1-x143.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it1-x143.google.com (mail-it1-x143.google.com [IPv6:2607:f8b0:4864:20::143]) (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 48CB22117B552 for ; Wed, 21 Nov 2018 03:05:24 -0800 (PST) Received: by mail-it1-x143.google.com with SMTP id c9so8246875itj.1 for ; Wed, 21 Nov 2018 03:05:24 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=f/0xBfIaw0O/D8QFJorzDiIT8oqRFd0cq81vRHN6DsQ=; b=i55P9I93vymLfwZGw9RyeGJFj2mIMyx5PvRJD0ZY5J43lT6iMgK1mKAcJ5/COCnFmc OtbEtZzlu/OsCKtZ4YZbdIPtTFDm1HE3uw9TksNSJeD2lJoTCrbbpWeynEEaqIoKvw+G JPjn9mf3ZqunxUbOhCHEeIqLsm+67mDyCutlQ= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=f/0xBfIaw0O/D8QFJorzDiIT8oqRFd0cq81vRHN6DsQ=; b=aQtqKFSAktXjA/V31TwlSvmjvLMrOIznih1sQqD/a/+2h46sm+S5b687dDD2AfIg4t y+MzlTeC7/Bmga2UtxGoVpQ+ZcNyB49GsNbT+u2MGwBCWwEEvYU9NNLSUvO0VjSXF1Hp LkhXIDLBb9KdCJXI2vtPgTWkW+csbCoF8LrcMCxj8Ws6/mUbuiajma5G3KUo5YPVlo5P ehGgCNFnb/DJ6R50W96mHH8nxDp8gm75EEtbRPntcUpT+16cAJ/Nr3SLIF2ucBuk2yTN E81Zs7JtbnciEFl+HfPJpLhezZyk8rSM76yqjZ2qPTTaz57L5TFF1L1oS97uDEZykBhp EWng== X-Gm-Message-State: AA+aEWZd9cO/NrBOgHP/TgyYuA4+uYneulAcAGcKMA3InE/AlSm3Mcvm R0tzhpj89pCwYU/ovbmV37fcNjrexWVcp0QwDErn5Q== X-Google-Smtp-Source: AFSGD/U6SlISXcs2jJ0ouCe4eenygkOeNRSEmPy9QoY5EQ+UVJVMc82pGT8EPC87dVHNw1G3wGYfs+U2lD/P3HtiJ1M= X-Received: by 2002:a02:734b:: with SMTP id a11mr4282538jae.62.1542798323053; Wed, 21 Nov 2018 03:05:23 -0800 (PST) MIME-Version: 1.0 References: <20181120154433.14167-1-ard.biesheuvel@linaro.org> <20181120161720.ipk3z6pazznlobp2@bivouac.eciton.net> In-Reply-To: <20181120161720.ipk3z6pazznlobp2@bivouac.eciton.net> From: Ard Biesheuvel Date: Wed, 21 Nov 2018 12:05:11 +0100 Message-ID: To: Leif Lindholm Cc: "edk2-devel@lists.01.org" 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: Wed, 21 Nov 2018 11:05:24 -0000 Content-Type: text/plain; charset="UTF-8" On Tue, 20 Nov 2018 at 17:17, Leif Lindholm wrote: > > 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 > Thanks Pushed as 6556224e1f262d1d3d3aecab6e86b14edefe95b7 > > - 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 > >